Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions android-test/src/androidTest/java/okhttp/android/test/EchTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ import app.cash.burst.Burst
import assertk.assertThat
import assertk.assertions.contains
import assertk.assertions.doesNotContain
import assertk.assertions.isEqualTo
import assertk.assertions.isFalse
import assertk.assertions.isTrue
import okhttp3.HttpUrl.Companion.toHttpUrl
import okhttp3.OkHttpClient
import okhttp3.Request
Expand Down Expand Up @@ -72,40 +70,60 @@ class EchTest(
fun tlsEchDevUsesEch() {
val body = client.get("https://tls-ech.dev/")

// Only the heading identifies the server we reached; every page links to all of the others.
assertThat(body).contains("<h1>tls-ech.dev</h1>")
assertThat(body).contains("You are using ECH")
assertThat(body).doesNotContain("not using ECH")
}

/** Port 444, because port 443 is the plain tls-ech.dev server. */
@Test
fun staleEchConfigIsNotRetried() {
val rejection = client.echRejectionFrom("https://stale.tls-ech.dev/")
fun staleEchConfigIsRetried() {
val body = client.get("https://stale.tls-ech.dev:444/")

// TODO retry with these, then assert "You are using ECH" like tlsEchDevUsesEch.
assertThat(rejection.hasRetryConfigList()).isTrue()
assertThat(rejection.publicHostname).isEqualTo("public.tls-ech.dev")
assertThat(body).contains("<h1>stale.tls-ech.dev</h1>")
assertThat(body).contains("You are using ECH")
assertThat(body).doesNotContain("not using ECH")
}

/** Port 445, because port 443 is the plain tls-ech.dev server. */
@Test
fun wrongPublicNameIsNotRetried() {
val rejection = client.echRejectionFrom("https://wrong.tls-ech.dev/")
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
Comment thread
yschimke marked this conversation as resolved.
val verifiedHostnames = mutableListOf<String>()
val hostnameVerifier = client.hostnameVerifier
val client =
client
.newBuilder()
.hostnameVerifier { hostname, session ->
verifiedHostnames += hostname
hostnameVerifier.verify(hostname, session)
}
.build()

val body = client.get("https://wrong.tls-ech.dev:445/")

// TODO retry with these, then assert "You are using ECH" like tlsEchDevUsesEch.
assertThat(rejection.hasRetryConfigList()).isTrue()
assertThat(rejection.publicHostname).isEqualTo("public.tls-ech.dev")
assertThat(body).contains("<h1>wrong.tls-ech.dev</h1>")
assertThat(body).contains("You are using ECH")
assertThat(verifiedHostnames).contains("public.tls-ech.dev")
}

/**
* TLS 1.2 cannot carry ECH.
*
* Port 446, because port 443 is the plain tls-ech.dev server.
*/
@Test
fun tls12OffersNothingToRetryWith() {
assertThat(client.echRejectionFrom("https://tls12.tls-ech.dev/").hasRetryConfigList()).isFalse()
val rejection = client.echRejectionFrom("https://tls12.tls-ech.dev:446/")

assertThat(rejection.hasRetryConfigList()).isFalse()
}

/**
* Makes the call at [url] and returns the ECH rejection it fails with.
*
* TODO handle EchConfigMismatchException.retry_configs.
*/
private fun OkHttpClient.echRejectionFrom(url: String): EchConfigMismatchException {
val body =
Expand Down
2 changes: 1 addition & 1 deletion okhttp-testing-support/src/main/kotlin/okhttp3/FakeDns.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooooh tricky

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,20 @@ package okhttp3.internal.platform

import android.annotation.SuppressLint
import android.content.Context
import android.net.ssl.EchConfigMismatchException
import android.os.Build
import android.os.StrictMode
import android.security.NetworkSecurityPolicy
import android.util.CloseGuard
import android.util.Log
import javax.net.ssl.SSLContext
import javax.net.ssl.SSLException
import javax.net.ssl.SSLSocket
import javax.net.ssl.SSLSocketFactory
import javax.net.ssl.X509TrustManager
import okhttp3.Protocol
import okhttp3.internal.SuppressSignatureCheck
import okhttp3.internal.dns.EchRetryConfig
import okhttp3.internal.platform.AndroidPlatform.Companion.Tag
import okhttp3.internal.platform.android.Android10SocketAdapter
import okhttp3.internal.platform.android.Android17SocketAdapter
Expand All @@ -39,6 +42,7 @@ import okhttp3.internal.platform.android.DeferredSocketAdapter
import okhttp3.internal.tls.CertificateChainCleaner
import okhttp3.internal.tls.TrustRootIndex
import okio.ByteString
import okio.ByteString.Companion.toByteString

/** Android 10+ (API 29+). */
@SuppressSignatureCheck
Expand Down Expand Up @@ -86,6 +90,24 @@ class Android10Platform :
?.configureTlsExtensions(sslSocket, hostname, protocols, echConfigList)
}

@SuppressLint("NewApi")
internal override fun getEchRetryConfig(exception: SSLException): EchRetryConfig? {
if (Build.VERSION.SDK_INT < 37 || exception !is EchConfigMismatchException) return null

// From https://cs.android.com/android/platform/superproject/+/android-latest-release:external/conscrypt/platform/src/main/java/org/conscrypt/Platform.java;bpv=0
// we can get neither, publicHostname only, or both. Conscrypt only hands us an EchConfigList
// if it is non-empty and self-consistent; BoringSSL does the real validation (version checks
// and such) when we hand the list back to it.
return EchRetryConfig(
publicHostname = exception.publicHostname ?: return null,
// An absent retry config list is how a server securely disables ECH.
configList =
exception.retryConfigList
?.toBytes()
?.toByteString(),
)
}

override fun getSelectedProtocol(sslSocket: SSLSocket): String? =
// No TLS extensions if the socket class is custom.
socketAdapters.find { it.matchesSocket(sslSocket) }?.getSelectedProtocol(sslSocket)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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. */
Expand All @@ -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,
Expand All @@ -120,6 +125,7 @@ class ConnectPlan internal constructor(
tunnelRequest = tunnelRequest,
connectionSpecIndex = connectionSpecIndex,
isTlsFallback = isTlsFallback,
echRetryConfig = echRetryConfig,
)

override fun connectTcp(): ConnectResult {
Expand Down Expand Up @@ -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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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!

If the server rejects ECH, the client proceeds with the handshake, authenticating for ECHConfig.contents.public_name as described in Section 6.1.7. If authentication or the handshake fails, the client MUST return a failure to the calling application.

(We shouldn’t fall back using our normal fallback mechanism, and that’s difficult to test)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this cover the fallback concern for you?

    // If this was already in response to an ech retry, we are done for this
    // connection
    if (echRetryConfig != null || !retryTlsHandshake(sslException)) return null


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
Expand Down Expand Up @@ -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)) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 elsewhere

retryTlsConnection = null
}

return ConnectResult(
plan = this,
nextPlan = retryTlsConnection,
Expand Down Expand Up @@ -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," +
Expand All @@ -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) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 offeredEchRetryConfig to be null in the case where the server rejects ECH (distinct from offeredEchRetryConfig.configList == null)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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)) {
Expand Down Expand Up @@ -561,6 +619,7 @@ class ConnectPlan internal constructor(
tunnelRequest = tunnelRequest,
connectionSpecIndex = connectionSpecIndex,
isTlsFallback = isTlsFallback,
echRetryConfig = echRetryConfig,
)

fun closeQuietly() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,10 +181,14 @@ class RouteSelector internal constructor(

val routes = dnsLookup(proxy, socketHost, socketPort)

// If DNS advertises ECH for any route, don't permit a retry without ECH.
val echRoutes = routes.filter { it.echConfigList != null }
val routesToTry = echRoutes.ifEmpty { routes }

// Try each address for best behavior in mixed IPv4/IPv6 environments.
return when {
fastFallback -> reorderForHappyEyeballs(routes)
else -> routes
fastFallback -> reorderForHappyEyeballs(routesToTry)
else -> routesToTry
}
}

Expand Down
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 encrypted_client_hello extension? Or is it a higher-level object that describes the TLS client’s state?

(I think it’s probably something describing the client state, because I don’t think the publicHostname is included in the encrypted_client_hello extension)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://cs.android.com/android/platform/superproject/+/android-latest-release:external/boringssl/src/include/openssl/ssl.h;l=4352?q=SSL_get0_ech_name_override&ss=android%2Fplatform%2Fsuperproject:external%2F&start=11

// SSL_get0_ech_name_override, if |ssl| is a client and the server rejected ECH,
// sets |*out_name| and |*out_name_len| to point to a buffer containing the ECH
// public name. Otherwise, the buffer will be empty.
//
// When offering ECH as a client, this function should be called during the
// certificate verification callback (see |SSL_CTX_set_custom_verify|). If
// |*out_name_len| is non-zero, the caller should verify the certificate against
// the result, interpreted as a DNS name, rather than the true server name. In
// this case, the handshake will never succeed and is only used to authenticate
// retry configs. See also |SSL_get0_ech_retry_configs|.
OPENSSL_EXPORT void SSL_get0_ech_name_override(const SSL *ssl,
                                               const char **out_name,
                                               size_t *out_name_len);
  std::string_view ech_name_override = GetECHNameOverride();
  if (!ech_name_override.empty()) {
    // If ECH was offered but not negotiated, BoringSSL will ask to verify a
    // different name than the origin. If verification succeeds, we continue the
    // handshake, but BoringSSL will not report success from SSL_do_handshake().
    // If all else succeeds, BoringSSL will report |SSL_R_ECH_REJECTED|, mapped
    // to |ERR_R_ECH_NOT_NEGOTIATED|. |ech_name_override| is only used to
    // authenticate GetECHRetryConfigs().
    DCHECK(!ssl_config_.ech_config_list.empty());
    used_ech_name_override_ = true;

* 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?,
)
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import java.util.logging.Logger
import javax.net.ssl.ExtendedSSLSession
import javax.net.ssl.SNIHostName
import javax.net.ssl.SSLContext
import javax.net.ssl.SSLException
import javax.net.ssl.SSLSocket
import javax.net.ssl.SSLSocketFactory
import javax.net.ssl.TrustManager
Expand All @@ -34,6 +35,7 @@ import javax.net.ssl.X509TrustManager
import okhttp3.Dns
import okhttp3.OkHttpClient
import okhttp3.Protocol
import okhttp3.internal.dns.EchRetryConfig
import okhttp3.internal.publicsuffix.PublicSuffixDatabase
import okhttp3.internal.readFieldOrNull
import okhttp3.internal.tls.BasicCertificateChainCleaner
Expand Down Expand Up @@ -122,6 +124,9 @@ open class Platform {
) {
}

/** Returns the ECH retry configuration carried by [exception]. */
internal open fun getEchRetryConfig(exception: SSLException): EchRetryConfig? = null

/** Called after the TLS handshake to release resources allocated by [configureTlsExtensions]. */
open fun afterHandshake(sslSocket: SSLSocket) {
}
Expand Down
Loading
Loading