Handle ECH Retry - #9611
Conversation
|
@swankjesse I don't think Conscrypt ECH PR supported this |
|
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.
|
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. |
swankjesse
left a comment
swankjesse
left a comment
There was a problem hiding this comment.
This is excellent. Some feedback and I’d like another look before merge. I promise to not make you wait so long on followups
| val routePlanner = factory.newRoutePlanner(client, address) | ||
| val route = factory.newRoute(address) | ||
| val connectionSpecs = listOf(ConnectionSpec.MODERN_TLS, ConnectionSpec.COMPATIBLE_TLS) | ||
| val socket = createSocketWithEnabledProtocols(TlsVersion.TLS_1_2, TlsVersion.TLS_1_1) |
There was a problem hiding this comment.
This one’s curious 'cause it’s missing TLS 1.3
| .planConnect() | ||
| .planWithCurrentOrInitialConnectionSpec(connectionSpecs, socket) | ||
|
|
||
| // A new DNS result must not influence a retry of the previous ECH configuration. |
| assertThat(retryTlsHandshake(retryableException)).isTrue() | ||
| } | ||
|
|
||
| @Test fun echRetryConfigIsUsedOnceWithoutTlsFallback() { |
There was a problem hiding this comment.
Maybe another test, echRetryConfigIsNotRecursive() ?
| socket.close() | ||
| } | ||
|
|
||
| @Test fun untrustedEchRetryConfigIsNotRetried() { |
There was a problem hiding this comment.
I believe the spec wants us to retry if the public name verifies
| } | ||
|
|
||
| /** https://www.rfc-editor.org/rfc/rfc9848.html#section-5.1 */ | ||
| @Test fun echAddressesDoNotFallBackToNonEch() { |
There was a problem hiding this comment.
I believe they do fall back to non-ech, if the public name from DNS verifies
There was a problem hiding this comment.
I don't think Conscrypt tells us that? Unless we check the session that just failed a handshake?
There was a problem hiding this comment.
Rather, we fall back to no ECH if there's no retry config and our hostname verification succeeds.
3 participants
A few assorted changes