Skip to content

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.

Sign up for GitHub

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

Draft
yschimke wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Handle ECH Retry #9611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 23 additions & 14 deletions android-test/src/androidTest/java/okhttp/android/test/EchTest.kt
View file
Open in desktop
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 @@ -77,21 +75,34 @@ class EchTest(
}

@Test
fun staleEchConfigIsNotRetried() {
val rejection = client.echRejectionFrom("https://stale.tls-ech.dev/")
fun staleEchConfigIsRetried() {
val body = client.get("https://stale.tls-ech.dev/")

// 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("You are using ECH")
assertThat(body).doesNotContain("not using ECH")
}

@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

@yschimke yschimke Jul 26, 2026

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.

Think I got think backwards, re-reading.

@bjiang7 bjiang7 Jul 27, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

// TODO: Add a fixture whose public hostname fails authentication.
val verifiedHostnames = mutableListOf<String>()
val hostnameVerifier = client.hostnameVerifier
val client =
client
.newBuilder()
.hostnameVerifier { hostname, session ->
verifiedHostnames += hostname
hostnameVerifier.verify(hostname, session)
}
.build()

// TODO retry with these, then assert "You are using ECH" like tlsEchDevUsesEch.
assertThat(rejection.hasRetryConfigList()).isTrue()
assertThat(rejection.publicHostname).isEqualTo("public.tls-ech.dev")
val body = client.get("https://wrong.tls-ech.dev/")

assertThat(body).contains("You are using ECH")
assertThat(verifiedHostnames).contains("public.tls-ech.dev")
}

/**
Expand All @@ -104,8 +115,6 @@ class EchTest(

/**
* 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
18 changes: 18 additions & 0 deletions okhttp/src/androidMain/kotlin/okhttp3/internal/platform/Android10Platform.kt
View file
Open in desktop
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,20 @@ 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

return EchRetryConfig(
publicHostname = exception.publicHostname ?: return null,
configList =
exception.retryConfigList
?.toBytes()
?.toByteString()
?: return null,
)
}

override fun getSelectedProtocol(sslSocket: SSLSocket): String? =
// No TLS extensions if the socket class is custom.
socketAdapters.find { it.matchesSocket(sslSocket) }?.getSelectedProtocol(sslSocket)
Expand Down
58 changes: 54 additions & 4 deletions okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/ConnectPlan.kt
View file
Open in desktop
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 @@ -161,6 +167,7 @@ class ConnectPlan internal constructor(
check(!isReady) { "already connected" }

val connectionSpecs = route.address.connectionSpecs
var offeredEchRetryConfig: EchRetryConfig? = null
var retryTlsConnection: ConnectPlan? = null
var success = false

Expand Down Expand Up @@ -204,7 +211,21 @@ class ConnectPlan internal constructor(
retryTlsConnection = tlsEquipPlan.nextConnectionSpec(connectionSpecs, sslSocket)

@swankjesse swankjesse Jul 26, 2026

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 think we make nextConnectionSpec() accept an EchRetryConfig, rather than having two competing mechanisms for deciding how to retry

@swankjesse swankjesse Jul 26, 2026

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.

Even better, maybe nextConnectionSpec() should accept an SSLException


connectionSpec.apply(sslSocket, isFallback = tlsEquipPlan.isTlsFallback)
connectTls(sslSocket, connectionSpec)
try {
connectTls(sslSocket, connectionSpec)
} catch (e: SSLException) {
val echRetryConfig = Platform.get().getEchRetryConfig(e)
if (
echRetryConfig != null &&
route.address.hostnameVerifier!!.verify(

@swankjesse swankjesse Jul 26, 2026

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 surprises me. Is there an attack that this defends against?

echRetryConfig.publicHostname,
sslSocket.session,
)
) {
offeredEchRetryConfig = echRetryConfig
}
throw e
}
call.eventListener.secureConnectEnd(call, handshake)
} else {
javaNetSocket = rawSocket
Expand Down Expand Up @@ -239,9 +260,37 @@ class ConnectPlan internal constructor(
call.eventListener.connectFailed(call, route.socketAddress, route.proxy, null, e)
connectionPool.connectionListener.connectFailed(route, call, e)

if (!retryOnConnectionFailure || !retryTlsHandshake(e)) {
retryTlsConnection = null
}
retryTlsConnection =
when {
echRetryConfig == null && offeredEchRetryConfig != null -> {
// TODO: Should an ECH retry honor retryOnConnectionFailure?
// Typically Conscrypt throwing EchConfigMismatchException
copy(
route =
Route(
address = route.address,
proxy = route.proxy,
socketAddress = route.socketAddress,
echConfigList = offeredEchRetryConfig.configList,
),
echRetryConfig = offeredEchRetryConfig,
)
}

echRetryConfig != null && offeredEchRetryConfig != null -> {
// TODO: Should we treat an untrusted or missing ECH retry config as an ordinary
// SSLException and try another connection spec?
Comment on lines +281 to +282

@swankjesse swankjesse Jul 26, 2026

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 believe that if we have ECH information, we shouldn’t let ourselves be downgraded.

null
}

retryOnConnectionFailure && retryTlsHandshake(e) -> {
retryTlsConnection
}

else -> {
null
}
}

return ConnectResult(
plan = this,
Expand Down Expand Up @@ -561,6 +610,7 @@ class ConnectPlan internal constructor(
tunnelRequest = tunnelRequest,
connectionSpecIndex = connectionSpecIndex,
isTlsFallback = isTlsFallback,
echRetryConfig = echRetryConfig,
)

fun closeQuietly() {
Expand Down
5 changes: 5 additions & 0 deletions okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/dns/-DnsMessage.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ data class DnsMessage(
}
}

internal data class EchRetryConfig(

@swankjesse swankjesse Jul 26, 2026

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.

Move this to its own file

val configList: ByteString,
val publicHostname: String,
)

@OkHttpInternalApi
data class Question(
val name: String,
Expand Down
5 changes: 5 additions & 0 deletions okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/platform/Platform.kt
View file
Open in desktop
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