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
|
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.
| } | ||
| } | ||
|
|
||
| internal data class EchRetryConfig( |
There was a problem hiding this comment.
Move this to its own file
| // TODO: Should we treat an untrusted or missing ECH retry config as an ordinary | ||
| // SSLException and try another connection spec? |
There was a problem hiding this comment.
I believe that if we have ECH information, we shouldn’t let ourselves be downgraded.
| @@ -204,7 +211,21 @@ class ConnectPlan internal constructor( | |||
| retryTlsConnection = tlsEquipPlan.nextConnectionSpec(connectionSpecs, sslSocket) | |||
There was a problem hiding this comment.
I think we make nextConnectionSpec() accept an EchRetryConfig, rather than having two competing mechanisms for deciding how to retry
There was a problem hiding this comment.
Even better, maybe nextConnectionSpec() should accept an SSLException
| 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." |
3 participants
No description provided.