Add support for ECH on Android 37#9383
Conversation
This comment has been minimized.
This comment has been minimized.
4d5b91f to
b2652bb
Compare
| emulator-options: > | ||
| -no-window | ||
| -gpu swiftshader_indirect | ||
| -noaudio |
|
waiting. thanks. |
swankjesse
left a comment
There was a problem hiding this comment.
Still working through this. The tech is solid but I have lots of API questions
| internal class AndroidDnsResolverDns internal constructor( | ||
| private val dnsResolver: AndroidDnsLookup = AndroidDnsResolver(), | ||
| ) : Dns, | ||
| EchAware { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I think they are tied, if you change Dns, you better change ECH lookups.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Is it because our API assumes you’re gonna call lookup and then getEchConfig in that order? ew?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() }, |
There was a problem hiding this comment.
Ooooh eeek, something dragonous is happening here
There was a problem hiding this comment.
After a brief investigation, it looks fine. The callback isn’t actually doing any work
| DnsResolver(PlatformRegistry.applicationContext!!, handlerThread.looper) | ||
| }, | ||
| private val executor: Executor = Executor { it.run() }, | ||
| private val timeoutSeconds: Long = 5L, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
addressed the unit, but not the location.
| handlerThread.start() | ||
| DnsResolver(PlatformRegistry.applicationContext!!, handlerThread.looper) | ||
| }, | ||
| private val executor: Executor = Executor { it.run() }, |
There was a problem hiding this comment.
After a brief investigation, it looks fine. The callback isn’t actually doing any work
| val isSupported: Boolean = (isAndroid && Build.VERSION.SDK_INT >= 37) | ||
|
|
||
| @ChecksSdkIntAtLeast(37) | ||
| fun buildIfSupported(): Platform? = if (isSupported) Android17Platform() else null |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm not sure we have something to do here, since 37 is C?
| * 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 { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Should move to internal package.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
TODO - move outside AndroidAsyncDns, something Call timeout related?
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.
DnsResolverHTTPS/SVCB lookups to capture ECH configuration records alongside address resolution.Dnsimplementations can expose platform-specific ECH data without returning rawAnyvalues.EchConfigListvalues to TLS sockets when the activeEchModeattempts ECH.NetworkSecurityPolicy.getDomainEncryptionMode()to select the ECH mode for each host.Remoteso they don't run in normal CI.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.