-
Notifications
You must be signed in to change notification settings - Fork 9.3k
Handle ECH Retry #9611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Handle ECH Retry #9611
Changes from all commits
34b5d29
859cb99
f136497
0a553c2
e72b035
e0db22e
cb44c95
693f04e
959eae0
94eeab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,7 +275,7 @@ class FakeDns( | |
| } | ||
|
|
||
| is ResourceRecord.IpAddress -> { | ||
| val ipAddressRecord = Dns.Record.IpAddress(request.hostname, resourceRecord.address) | ||
| val ipAddressRecord = Dns.Record.IpAddress(resourceRecord.name, resourceRecord.address) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ooooh tricky
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems more correct? |
||
| when (resourceRecord.address) { | ||
| is Inet4Address -> ipv4Records += ipAddressRecord | ||
| is Inet6Address -> ipv6Records += ipAddressRecord | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ import java.net.Socket as JavaNetSocket | |
| import java.net.UnknownServiceException | ||
| import java.security.cert.X509Certificate | ||
| import java.util.concurrent.TimeUnit | ||
| import javax.net.ssl.SSLException | ||
| import javax.net.ssl.SSLPeerUnverifiedException | ||
| import javax.net.ssl.SSLSocket | ||
| import okhttp3.CertificatePinner | ||
|
|
@@ -38,6 +39,7 @@ import okhttp3.internal.closeQuietly | |
| import okhttp3.internal.concurrent.TaskRunner | ||
| import okhttp3.internal.concurrent.withLock | ||
| import okhttp3.internal.connection.RoutePlanner.ConnectResult | ||
| import okhttp3.internal.dns.EchRetryConfig | ||
| import okhttp3.internal.http.ExchangeCodec | ||
| import okhttp3.internal.http1.Http1ExchangeCodec | ||
| import okhttp3.internal.platform.Platform | ||
|
|
@@ -73,6 +75,7 @@ class ConnectPlan internal constructor( | |
| private val tunnelRequest: Request?, | ||
| internal val connectionSpecIndex: Int, | ||
| internal val isTlsFallback: Boolean, | ||
| private val echRetryConfig: EchRetryConfig? = null, | ||
| ) : RoutePlanner.Plan, | ||
| ExchangeCodec.Carrier { | ||
| /** True if this connect was canceled; typically because it lost a race. */ | ||
|
|
@@ -98,10 +101,12 @@ class ConnectPlan internal constructor( | |
| get() = protocol != null | ||
|
|
||
| private fun copy( | ||
| route: Route = this.route, | ||
| attempt: Int = this.attempt, | ||
| tunnelRequest: Request? = this.tunnelRequest, | ||
| connectionSpecIndex: Int = this.connectionSpecIndex, | ||
| isTlsFallback: Boolean = this.isTlsFallback, | ||
| echRetryConfig: EchRetryConfig? = this.echRetryConfig, | ||
| ): ConnectPlan = | ||
| ConnectPlan( | ||
| taskRunner = taskRunner, | ||
|
|
@@ -120,6 +125,7 @@ class ConnectPlan internal constructor( | |
| tunnelRequest = tunnelRequest, | ||
| connectionSpecIndex = connectionSpecIndex, | ||
| isTlsFallback = isTlsFallback, | ||
| echRetryConfig = echRetryConfig, | ||
| ) | ||
|
|
||
| override fun connectTcp(): ConnectResult { | ||
|
|
@@ -200,11 +206,13 @@ class ConnectPlan internal constructor( | |
| val tlsEquipPlan = planWithCurrentOrInitialConnectionSpec(connectionSpecs, sslSocket) | ||
| val connectionSpec = connectionSpecs[tlsEquipPlan.connectionSpecIndex] | ||
|
|
||
| // Figure out the next connection spec in case we need a retry. | ||
| retryTlsConnection = tlsEquipPlan.nextConnectionSpec(connectionSpecs, sslSocket) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m walking through this, and trying to cover all our bases. This sets up a mechanism to attempt the next connection spec in sequence, and it’s replaced with code to attempt the next ECH config. If the ECH config fails, do we attempt the next connection spec? I suppose we don’t per the ECH doc, which is also weird!
(We shouldn’t fall back using our normal fallback mechanism, and that’s difficult to test)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should clarify, one badly configured server/region is more likely than poorly attempted hacking, so I don't like reducing one of the "free" wins of distributed computing, retries. But the change was really intended to just be to include the possible mismatch failure in the next choice, so it can't happen before connectTls.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this cover the fallback concern for you? |
||
|
|
||
| connectionSpec.apply(sslSocket, isFallback = tlsEquipPlan.isTlsFallback) | ||
| connectTls(sslSocket, connectionSpec) | ||
| try { | ||
| connectTls(sslSocket, connectionSpec) | ||
| } catch (e: SSLException) { | ||
| retryTlsConnection = tlsEquipPlan.nextConnectionSpec(connectionSpecs, sslSocket, e) | ||
| throw e | ||
| } | ||
| call.eventListener.secureConnectEnd(call, handshake) | ||
| } else { | ||
| javaNetSocket = rawSocket | ||
|
|
@@ -239,10 +247,6 @@ class ConnectPlan internal constructor( | |
| call.eventListener.connectFailed(call, route.socketAddress, route.proxy, null, e) | ||
| connectionPool.connectionListener.connectFailed(route, call, e) | ||
|
|
||
| if (!retryOnConnectionFailure || !retryTlsHandshake(e)) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note to self... this behavior isn’t removed, we just compute a null |
||
| retryTlsConnection = null | ||
| } | ||
|
|
||
| return ConnectResult( | ||
| plan = this, | ||
| nextPlan = retryTlsConnection, | ||
|
|
@@ -479,7 +483,7 @@ class ConnectPlan internal constructor( | |
| sslSocket: SSLSocket, | ||
| ): ConnectPlan { | ||
| if (connectionSpecIndex != -1) return this | ||
| return nextConnectionSpec(connectionSpecs, sslSocket) | ||
| return nextCompatibleConnectionSpec(connectionSpecs, sslSocket) | ||
| ?: throw UnknownServiceException( | ||
| "Unable to find acceptable protocols." + | ||
| " isFallback=$isTlsFallback," + | ||
|
|
@@ -489,12 +493,66 @@ class ConnectPlan internal constructor( | |
| } | ||
|
|
||
| /** | ||
| * Returns a copy of this connection with the next connection spec to try, or null if no other | ||
| * compatible connection specs are available. | ||
| * Returns a copy of this connection that recovers from [sslException], or null if the failure | ||
| * should not be retried. | ||
| */ | ||
| internal fun nextConnectionSpec( | ||
| connectionSpecs: List<ConnectionSpec>, | ||
| sslSocket: SSLSocket, | ||
| sslException: SSLException, | ||
| ): ConnectPlan? { | ||
| if (!retryOnConnectionFailure) return null | ||
|
|
||
| val offeredEchRetryConfig = Platform.get().getEchRetryConfig(sslException) | ||
| if (offeredEchRetryConfig != null) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This condition surprises me. The logic looks incorrect for the case where the server securely disables ECH. In particular I would expect the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the validation comments here #9611 (comment) Which one do you think is which? |
||
| // TODO should we emit an event that we considered ech retry? | ||
|
|
||
| // https://www.rfc-editor.org/rfc/rfc9849.html#section-6.1.6 | ||
| val retryable = | ||
| when (offeredEchRetryConfig.configList) { | ||
| // The server securely disabled ECH. Retry unless we already disabled ECH. | ||
| null -> echRetryConfig == null || echRetryConfig.configList != null | ||
|
|
||
| // A retry config in response to a retry config signals a misconfigured server. | ||
| else -> echRetryConfig == null | ||
| } | ||
| if (!retryable) return null | ||
|
|
||
| // Validate the publicHostname against the session certificate | ||
| // The session is protected by the outer client hello (e.g. cloudflare-ech.com) | ||
| // not the origin server | ||
| val hostnameVerifier = route.address.hostnameVerifier!! | ||
| if (!hostnameVerifier.verify(offeredEchRetryConfig.publicHostname, sslSocket.session)) { | ||
| return null | ||
| } | ||
|
|
||
| return copy( | ||
| route = | ||
| Route( | ||
| address = route.address, | ||
| proxy = route.proxy, | ||
| socketAddress = route.socketAddress, | ||
| echConfigList = offeredEchRetryConfig.configList, | ||
| ), | ||
| // echRetryConfig.configList is possibly null to retry with ECH disabled | ||
| echRetryConfig = offeredEchRetryConfig, | ||
| ) | ||
| } | ||
|
|
||
| // If this was already in response to an ech retry, we are done for this | ||
| // connection | ||
| if (echRetryConfig != null || !retryTlsHandshake(sslException)) return null | ||
|
|
||
| return nextCompatibleConnectionSpec(connectionSpecs, sslSocket) | ||
| } | ||
|
|
||
| /** | ||
| * Returns a copy of this connection with the next compatible connection spec, or null if none | ||
| * are available. | ||
| */ | ||
| private fun nextCompatibleConnectionSpec( | ||
| connectionSpecs: List<ConnectionSpec>, | ||
| sslSocket: SSLSocket, | ||
| ): ConnectPlan? { | ||
| for (i in connectionSpecIndex + 1 until connectionSpecs.size) { | ||
| if (connectionSpecs[i].isCompatible(sslSocket)) { | ||
|
|
@@ -561,6 +619,7 @@ class ConnectPlan internal constructor( | |
| tunnelRequest = tunnelRequest, | ||
| connectionSpecIndex = connectionSpecIndex, | ||
| isTlsFallback = isTlsFallback, | ||
| echRetryConfig = echRetryConfig, | ||
| ) | ||
|
|
||
| fun closeQuietly() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /* | ||
| * Copyright (c) 2026 OkHttp Authors | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package okhttp3.internal.dns | ||
|
|
||
| import okio.ByteString | ||
|
|
||
| /** | ||
| * ECH retry config. Sent by a server when the ECH configuration we offered has fallen out of sync | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doc says ‘Sent by a server’ but we need a mechanism to lookup the public name when the server replies without this extension. Does this class represent the (I think it’s probably something describing the client state, because I don’t think the publicHostname is included in the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| * with the one it accepts: its TTL expired, or the server rotated to a new configuration. (For | ||
| * example, Cloudflare publishes one configuration at a time and rotates it hourly, honoring the | ||
| * previous one for a further 4 hours. A configuration cached past that grace period earns a retry | ||
| * config.) | ||
| * | ||
| * If a new [configList] is present, the server securely replaced our ECH configuration, and it | ||
| * must only be used when [publicHostname] can be validated against the certificate from the | ||
| * SSLSession (the outer client hello). Authenticating the public name is what makes this safe: | ||
| * https://www.rfc-editor.org/rfc/rfc9849.html#section-6.1.7 | ||
| * | ||
| * A null [configList] means the server offered no usable retry configuration, which securely | ||
| * disables ECH. Retry without ECH. | ||
| * | ||
| * https://www.rfc-editor.org/rfc/rfc9849.html#section-6.1.6 | ||
| */ | ||
| internal data class EchRetryConfig( | ||
| /** The client-facing server's name from `ECHConfig.contents.public_name`. */ | ||
| val publicHostname: String, | ||
| /** updated ECH configList or null to retry without ECH */ | ||
| val configList: ByteString?, | ||
| ) | ||
Uh oh!
There was an error while loading. Please reload this page.