Handshake returns cleaned peer certificates #5311
Conversation
okhttp/src/main/java/okhttp3/internal/connection/RealConnection.kt
Outdated
Show resolved
Hide resolved
|
@swankjesse Any advice for testing here? We test RealConnection through CallTest, and the JVM code around KeyManager will likely send the clean certificates. |
|
This situation is frustrating. Cleaning the certificates could be non-trivial, so this has the potential of breaking or slowing code that works otherwise. I’d like to land 4.1 without this, and then put this in 4.2 early with lots of bake time. |
|
That said, the change itself looks great! As for testing . . . wanna test with a fake cleaner? |
That route isn't great. It makes new internal APIs for setting the cleaner in OkHttpClient.Builder. Cleaner currently comes from the Platform, or via the TrustManager if you customise that. But they are on a public API, so they tend to show up for Java clients.
|
|
@swankjesse I'm tempted to move this into sslSocketSession.handshake() But also wondering whether we should retain the uncleaned certificates? Handshake could have both, potentially interesting when trying to understand why a decision was made. e.g. if certificates are served that have multiple root CAs for different clients. Not sure if that happens in the wild. |
|
We can hide things from Java clients with a |
|
Can we always use the cleaner from the pinner? What’s the benefit of wiring it through the connection? If that one is null we shouldn’t bother. I’m still unsatisfied with the performance cost here. It has a non-zero cost and a nearly-zero benefit. |
OK, so cost is cleaning certificates when we aren't currently doing certificate pinning. The benefit is that callers can rely on the handshake certificates being those that we trust and validate. If we choose to expose the handshake certificates (public API), I'd argue that we must provide the validated ones. I'll take a look if we can defer cleaning until these are requested in the handshake. |
|
Now deferring cleaning until we either
But only done at most once. |
yschimke
changed the title
|
n.b. I removed the test because the change otherwise becomes awkward with OkHttpClient, since that constructor generally always fetches a new cleaner from Platform :( |
| } | ||
| } | ||
|
|
||
| internal fun check(hostname: String, cleanedPeerCertificatesFn: () -> List<X509Certificate>) { |
There was a problem hiding this comment.
I think passing a lambda here is too surprising; it might be better to make this take cleanedCertificates, and then make the one callsite guard the call with like hasPins() check
There was a problem hiding this comment.
It's not a simple check without cost if there is pinning in place. e.g. a client configures pinning on their API host, but not on the CDN (unknown hosts).
In this case, findMatchingPins(hostname) is the check we need to do externally rather than hasPins.
There was a problem hiding this comment.
It's also internal API, for a class with public API.
| } | ||
| } | ||
|
|
||
| internal fun check(hostname: String, cleanedPeerCertificatesFn: () -> List<X509Certificate>) { |
| @get:JvmName( | ||
| "localCertificates") val localCertificates: List<Certificate>, | ||
|
|
||
| // Delayed provider of peerCertificates, to allow lazy cleaning. |
|
I think this is a very nice improvement. |
#5138
Pass through clean certificates with only a single pass of cert cleaning.