Handle ECH Retry - #9611
Conversation
|
@swankjesse I don't think Conscrypt ECH PR supported this |
| fun differentPublicHostnameIsVerifiedBeforeRetry() { | ||
| // The outer certificate authenticates public.tls-ech.dev, | ||
| // so the retry config may be used if it matches. | ||
| // https://www.rfc-editor.org/rfc/rfc9849.html#section-6.1.6 |
There was a problem hiding this comment.
Think I got think backwards, re-reading.
There was a problem hiding this comment.
Yes, you should only be verifying the public hostname against the certificate, not against the request. Conscrypt isn't able to do the hostname validation itself, which is why it's necessary to do it here in OkHttp.
FYI @tweksteen
There was a problem hiding this comment.
Just to confirm, at this point our SSLSession is dead, but we can safely read the certificates that were presented.
But are the certificates on the SSLSession trusted? Does Conscrypt already clean the certs?
|
Quick search for evidence https://go.dev/src/crypto/tls/handshake_client_test.go#L2808 |
|
I'll ask at work tomorrow. |
swankjesse
left a comment
swankjesse
left a comment
There was a problem hiding this comment.
I’d like to be particularly careful with landing this as it introduces new attack vectors and is not yet possible to test with MockWebServer + JVM Conscrypt.
I think it’s definitely good to have special recovery code for ECH handshake failures. But my first instinct is that special recovery code should mostly be about reducing the opportunity for downgrade attacks.
This is attempting to do something else, and I would like to understand the problem it solves before we land this.
| val echRetryConfig = Platform.get().getEchRetryConfig(e) | ||
| if ( | ||
| echRetryConfig != null && | ||
| route.address.hostnameVerifier!!.verify( |
There was a problem hiding this comment.
This surprises me. Is there an attack that this defends against?
|
Just chiming in here to state that the ECH recovery flow is a pretty important part of the spec, and so should be implemented if OkHttp is to say it supports ECH. The reason why is because there's no other way to recover from a mismatch in ECH config between the client and server (i.e. synchronization issues). It's a performance hit to have to fall into this codepath (and shouldn't be happening in most cases), but it is needed so the consequences of things being out of sync is latency rather than outages. I also asked BoringSSL a long time ago about this when doing the Android implementation, because I had the same question of how important the recovery flow was. Their stance was "This behavior is pretty crucial to allow servers to safely deploy ECH because DNS records can be stale and we need to make it possible for servers to recover from mismatches. The existence of any client that gets this wrong means a server now cannot deploy ECH in a rollback-safe way." |
|
Making it safer for server operators to confidently deploy ECH is good. |
| client | ||
| .newBuilder() | ||
| .connectionSpecs(listOf(ConnectionSpec.RESTRICTED_TLS, ConnectionSpec.MODERN_TLS)) | ||
| .eventListener( |
There was a problem hiding this comment.
this wasn't valid, and after the refactor, isn't effective.
3 participants
A few assorted changes