Skip to content

Add support for ECH on Android 37#9383

Open
yschimke wants to merge 3 commits into
square:masterfrom
yschimke:testing_ech
Open

Add support for ECH on Android 37#9383
yschimke wants to merge 3 commits into
square:masterfrom
yschimke:testing_ech

Conversation

@yschimke

@yschimke yschimke commented Mar 19, 2026

Copy link
Copy Markdown
Collaborator
  • Adds an Android 17 platform path that uses public Android APIs for ALPN, session tickets, domain encryption policy, and ECH socket configuration.
  • Uses Android DnsResolver HTTPS/SVCB lookups to capture ECH configuration records alongside address resolution.
  • Introduces typed ECH DNS records so Dns implementations can expose platform-specific ECH data without returning raw Any values.
  • Applies Android EchConfigList values to TLS sockets when the active EchMode attempts ECH.
  • Uses NetworkSecurityPolicy.getDomainEncryptionMode() to select the ECH mode for each host.
  • Retries once without ECH when the platform reports an ECH configuration mismatch and the active mode permits fallback.
  • Tags live external ECH checks as Remote so they don't run in normal CI.
  • Includes CI cleanup for the AGP source-set API change and Android localhost cleartext test traffic.

Validation is mostly local Android/JVM compile and API checks, plus host-side tests for the DNS/ECH policy plumbing. The live ECH tests remain opt-in because
they depend on external servers.

@github-advanced-security

This comment has been minimized.

@yschimke yschimke changed the title Testing ech Add support for ECH on Android 37 May 4, 2026
@yschimke yschimke marked this pull request as ready for review May 4, 2026 10:19
@yschimke yschimke requested a review from swankjesse May 4, 2026 10:51
@yschimke yschimke force-pushed the testing_ech branch 6 times, most recently from 4d5b91f to b2652bb Compare May 4, 2026 12:26
Comment thread .github/workflows/build.yml Dismissed
emulator-options: >
-no-window
-gpu swiftshader_indirect
-noaudio
@ardakazanci

Copy link
Copy Markdown

waiting. thanks.

@swankjesse swankjesse left a comment

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.

Still working through this. The tech is solid but I have lots of API questions

Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread .github/workflows/build.yml Outdated
Comment thread android-test/src/androidTest/java/okhttp/android/test/EchTest.kt
Comment thread android-test/build.gradle.kts Outdated
Comment thread okhttp/src/commonJvmAndroid/kotlin/okhttp3/Dns.kt Outdated
internal class AndroidDnsResolverDns internal constructor(
private val dnsResolver: AndroidDnsLookup = AndroidDnsResolver(),
) : Dns,
EchAware {

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 don’t have a good intuition on whether ECH should be glued to DNS in this way. I think it’s probably a mistake to structure it this way, because if you decorate your Dns interface you lose ECH

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 they are tied, if you change Dns, you better change ECH lookups.

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 tried a field.

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.

But I still think we will lose it if you override platform AsyncDns with a custom Dns. Not sure we have a fix for that.


override fun lookup(hostname: String): List<InetAddress> {
val result = dnsResolver.lookup(hostname)
result.echConfig?.let {

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.

Why cache these?

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.

Is it because our API assumes you’re gonna call lookup and then getEchConfig in that order? ew?

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.

Yeah, I don't want to do Dns twice. Having the dns records becomes a side effect.

Open to other ideas, maybe we can call again, but ensure it's a caching instance that will reuse?

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 this is optimal - other choices

  • make a second request later - slower and might change in meantime, it seems better if Dns + Ech are done together
  • ? - I don't have another alternative...

handlerThread.start()
DnsResolver(PlatformRegistry.applicationContext!!, handlerThread.looper)
},
private val executor: Executor = Executor { it.run() },

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 eeek, something dragonous is happening here

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.

After a brief investigation, it looks fine. The callback isn’t actually doing any work

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'll comment.

DnsResolver(PlatformRegistry.applicationContext!!, handlerThread.looper)
},
private val executor: Executor = Executor { it.run() },
private val timeoutSeconds: Long = 5L,

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 don’t like that seconds is the unit, and I don’t like that the default is specified here and not far away in some caller that has more context

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.

addressed the unit, but not the location.

handlerThread.start()
DnsResolver(PlatformRegistry.applicationContext!!, handlerThread.looper)
},
private val executor: Executor = Executor { it.run() },

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.

After a brief investigation, it looks fine. The callback isn’t actually doing any work

Comment thread okhttp/src/androidMain/kotlin/okhttp3/internal/platform/AndroidDnsResolverDns.kt Outdated
val isSupported: Boolean = (isAndroid && Build.VERSION.SDK_INT >= 37)

@ChecksSdkIntAtLeast(37)
fun buildIfSupported(): Platform? = if (isSupported) Android17Platform() else 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.

OH MAN, today I learned there’s also a thing called C Extensions 22, and we probably aught to figure out how to support this on whatever devices can do it, which is more complex?!

https://developer.android.com/reference/android/security/NetworkSecurityPolicy#getDomainEncryptionMode(java.lang.String)

I chose violence.
https://bsky.app/profile/swank.ca/post/3mnuhrpsbrc2g

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'm not sure we have something to do here, since 37 is C?

Comment thread okhttp/src/androidMain/kotlin/okhttp3/android/AndroidAsyncDns.kt
* Typical implementations are backed by Android's `DnsResolver`, OkHttp's DnsOverHttps, or other
* resolver libraries. Implementations must be safe for concurrent use.
*/
internal fun interface AsyncDns {

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.

Should we make this public? Probably as follow up.

* See the License for the specific language governing permissions and
* limitations under the License.
*/
package okhttp3

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.

Should move to internal package.

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.

Maybe awkward if it's exposed on public APIs

* The asynchronous DNS resolver, or null to resolve with [dns] only. When set, the connection
* path can use HTTPS/SVCB records it returns (including Encrypted Client Hello configuration).
*/
internal val asyncDns: AsyncDns? = builder.asyncDns

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.

Should setting dns null this out?

},
// executor is only used for handoff, so a minimal direct Executor
private val executor: Executor = Executor { it.run() },
private val timeoutMillis: Int = 5_000,

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.

TODO - move outside AndroidAsyncDns, something Call timeout related?

Comment thread android-test/src/androidTest/java/okhttp/android/test/OkHttpTest.kt Outdated
Comment thread android-test/build.gradle.kts
claude added 2 commits June 27, 2026 20:45
Introduce a call-aware async DNS path that can carry ECH configuration
from HTTPS/SVCB records, and wire ECH into the Android and JVM platforms.

- New public AsyncDns / DnsResult and okhttp3.ech (EchConfig, EchMode,
  EchModeConfiguration) APIs.
- Internal call-aware resolution (Resolve.kt, BlockingAsyncDns) that
  prefers a platform AsyncDns and attaches ECH to the RealCall, falling
  back to the public Dns when none is configured.
- An explicitly configured Dns now takes precedence over the platform
  AsyncDns: OkHttpClient.Builder.dns() clears asyncDns so a custom Dns is
  honored (this also disables ECH, which only the async resolver carries).
- Android17 platform support: AndroidAsyncDns (DnsResolver-backed, with a
  system-resolver fallback for localhost/loopback/lenient names) and
  Android17SocketAdapter wrapping the Conscrypt SNI handling.
- Platform.configureTlsExtensions gains a call parameter; update the
  mockwebserver callers and compile against API 37 (agp 9.2.0).
- EchTest and AndroidSocketAdapter tests, plus OkHttpTest updates for the
  AsyncDns/ECH behavior and the API 37 emulator.
- android-test/android-test-app build config for API 37 (compileApi 37;
  Robolectric unit tests disabled while no android-all artifact exists),
  network security config and manifest for the ECH test endpoints.
- Robolectric --add-opens workaround in base-conventions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants