The rest of the roadmap: DNS, HTTP/2, Alt-Svc, TLS policy, client certificates and SvcParams - #64
Merged
Merged
Conversation
…ether racing works Three suites on the DNS machinery the resolver matrix already put in place. `HttpsRecordTest` (#19) asks what an `HTTPS` record's parameters *mean* rather than whether they arrived. The two pieces of RFC 9460 that are easy to get wrong and invisible when they are: §7.1.1 implies the service's default ALPN, so Cloudflare's `alpn=h3,h2` means three protocols and not two — verified against the raw record, which really does list only two — and a record with no `alpn` at all leaves the list null rather than defaulted, which is the other side of the same rule. An absent `port` means 443 rather than 0, which is OkHttp supplying the default rather than the record carrying it. And a record led by `h3` still connects over HTTP/2, since OkHttp has no HTTP/3. `DnsFailureTest` (#20) asks what a failure looks like from the caller's side. SERVFAIL and NXDOMAIN have to be distinguishable — today they are, in the crudest way available: SERVFAIL carries a message and NXDOMAIN's is null, so the assertion is that they *differ* rather than pinning either wording. The finding worth having is the third case: a resolver answering `429` raises a plain `IOException`, not `UnknownHostException`, so code catching the latter to mean "bad name" never sees a rate limit and code using it to decide whether to retry decides backwards. `HappyEyeballsTest` (#21) puts an RFC 5737 blackhole first and requires the connection to happen anyway, and requires two blackholes to fail as a `ConnectException` inside one timeout budget rather than two. The v6-only case skips with its reason recorded: this runner has no IPv6, and so has no GitHub runner, and a v6 case that cannot tell "no route" from "raced past" would pass vacuously. Two things surfaced writing them, and both changed the code rather than the prose. Some resolvers turn a DNSSEC validation failure into an HTTP 502, which the matrix could not represent — it caught `UnknownHostException` only, so the case failed rather than recording. `Outcome.ERRORED` now holds it, distinct from both "no answer" and "unreachable", and it is the same distinction `DnsFailureTest` asserts. And every client that pins addresses now sets `Proxy.NO_PROXY`: with a proxy in the way OkHttp connects to the proxy and the addresses under test are never dialled, so the suite would pass having tested nothing. This repository's own sandbox proxies outbound HTTPS, which is how that was noticed rather than reasoned about. `sigok`/`sigfail` also join the resolver matrix, which is #20's reporting bullet — which resolvers validate DNSSEC, published beside everything else they disagree about, with no new machinery. Verified live: all of `HttpsRecordTest`, `DnsFailureTest` and the matrix pass, and three of four Happy Eyeballs cases pass with the fourth skipping for the reason above. Progresses #5, #19. Closes #20, #21. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
…ot take `Http2Test` (#11) runs against `nghttp2.org` — the reference implementation, and a stack sharing no code with OkHttp's. MockWebServer speaks HTTP/2 using OkHttp's own framing, HPACK encoder and flow control, which makes it excellent for testing OkHttp against what OkHttp believes and useless for testing it against what anyone else believes. Four questions: ALPN selects h2, a body past the initial window arrives whole, a 6 KB header survives a foreign HPACK decoder, and twenty concurrent streams all finish. `Http2CoalescingTest` (#11) is the fifth, and it is in `containers` because the conditions have to be made rather than hoped for. Two names on one certificate at one address is not something the public web reliably offers: `cloudflare.com` and `www.cloudflare.com` share a certificate and sit on *different* edge addresses, so they cannot coalesce however well the client behaves — checked rather than assumed, after a first draft asserted they would and CI would have been right to disagree. `test-server` mints a leaf covering both names via `CERT_HOSTS`, a `Dns` points them at the one container, and the assertion is one connection for two names. `AltSvcTest` (#12) is a regression guard on a fallback nobody looks at. OkHttp has no HTTP/3, so an origin advertising `h3` in `Alt-Svc` must be ignored and the connection must stay on HTTP/2 — a client that mishandled the header would upgrade to a protocol it cannot speak, or treat an unknown protocol name as an error, or quietly drop to HTTP/1.1 and halve its throughput. The second case reuses the client, because `Alt-Svc` arrives on the first response and a client acting on it would act on the *next* request. Beside it is the part with a shelf life: `AltSvcReport` records which origins offer h3 and what OkHttp used instead, published as its own table. Today every row reads "offers h3 / h2", which is the correct outcome and an uninteresting assertion. The day HTTP/3 lands, that table is where it shows up. Two figures are the server's rather than the client's, and are commented as such: nghttp2's httpbin caps `/bytes` at 100 KB, so asking for a megabyte measures the cap, and `quic.rocks:4433` is in issue #12 and not here because it answers 502 today. Verified: `Http2Test` 4/4 and `AltSvcTest` 6/6 live. `Http2CoalescingTest` needs Docker, so its mechanism was verified against `test-server` running natively — both names, one connection, h2 on each — rather than left for CI to discover. Progresses #5. Closes #11, #12. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
yschimke
changed the title
…about revocation `ConnectionSpecTest` (#14) points OkHttp at `test-server`'s port-per-version listeners, which are there so that a refusal is the spec refusing rather than two ends failing to find common ground by accident. `RESTRICTED_TLS` reaches 1.2 and 1.3, reports the version honestly, and negotiates a suite the spec actually offered; a spec pinned to 1.3 against a listener pinned to 1.2 fails with something a caller can read rather than a bare connection reset. What it does not assert is who refuses TLS 1.0, and that is deliberate. Modern JDKs disable 1.0 and 1.1 through `jdk.tls.disabledAlgorithms` before any spec is consulted, so the client never offers them and the failure that comes back is `protocol_version` from the *server*, rejecting an offer too new for the listener. Measured rather than assumed: even `COMPATIBLE_TLS`, which permits the old versions, cannot reach those listeners on this JDK. Claiming "RESTRICTED_TLS refused it" would credit the library for the platform's work, and would keep passing on a JDK where the spec had stopped doing anything. `PinningTest` and `FixturePinningTest` (#16) split by whether the thing being tested is a promise. `CertificatePinner` is OkHttp's own, so the wrong-pin case is asserted in public — refusal *and* a message listing the peer's real pins, since the pin a caller needs is the one they got wrong. The positive case is against the fixture: pinning a live public chain means pinning something that rotates, and the fixture mints its CA per container so the pin is computed from the chain in front of us and is correct by construction. Revocation and Certificate Transparency are recorded rather than asserted, because neither is a promise anybody made. The JVM does not check revocation unless asked, Android varies by release, and OkHttp enforces no SCTs — a suite insisting `revoked.badssl.com` must be refused would report documented behaviour as a defect. `TlsPolicyReport` writes the answers per platform and the status page shows them in neutral colours. The fixture's own chain answers the CT half by existing: privately issued, no SCTs, and it connects. Three things were wrong on the first pass and are now right. The pinning failure message lists the peer chain *before* the configured pin, so an assertion reading the far side of it found nothing — it now scans for pins rather than relying on ordering. A recording test asserted the connection succeeded, and badssl.com timed out mid-run: a timeout is not an answer, so it skips now, which keeps an outage out of a column meant for policy. And `MODERN_TLS`'s versions were asserted with `isIn` against a single value, which is `isEqualTo` wearing a hat. Verified: `PinningTest` 3/3 live. `ConnectionSpecTest`, `FixturePinningTest` and `Http2CoalescingTest` need Docker, so each was verified against `test-server` running natively first — the spec matrix across all four listeners, and pinning right-pin-accepted against wrong-pin-refused. The new status-page tables were rendered headless in Chromium from a real collector run. Progresses #5. Closes #14, #16. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
… tested `test-server` requested client certificates and never required them, which is right for reporting what a client offered and useless for testing that it offered anything: a client with none was served just the same, so "presented" and "ignored" were indistinguishable. There is now an `mtls` listener that requires and verifies one against the fixture CA, and `/client.pem` serves an identity that CA has signed — fetched at run time for the same reason `/ca.pem` is, since the CA is minted per process and anything committed here could not have been signed by it. The client key is PKCS#8 rather than SEC1, because okhttp-tls's `HeldCertificate.decode` reads `BEGIN PRIVATE KEY` and not `BEGIN EC PRIVATE KEY`. The server-side leaves stay SEC1: Go reads both, and this is the only key a client of this fixture has to parse. `ClientCertificateTest` (#15) asserts the certificate is presented and that the server names the subject it verified — asserting the status alone would pass against a listener that had quietly stopped asking. It asserts the same on a second connection, with the pool emptied in between, so key material read once and cached would pass the first case and fail that one. And omitting the certificate has to fail as `certificate_required` rather than as a bare reset, which is what turns an outage-looking failure into a fixable one. The fourth case is the one worth knowing about, and it is not the one you would guess: a certificate from a CA the server does not accept behaves *exactly* like having none. The server advertises which issuers it will take, the JDK's key manager finds no match, and sends nothing — so the server reports a missing certificate rather than an untrusted one. Asserted as the same failure, which is the honest statement of the behaviour rather than a wish about it. `PublicClientCertificateTest` (#15) is the reality check against `client.badssl.com`, which requests rather than requires and answers `400` when nothing arrives. Worth knowing on its own: a missing client certificate can reach an application as an HTTP status rather than as a TLS error. It loads badssl's published PKCS#12 through a `KeyStore` and a `KeyManagerFactory` rather than okhttp-tls, because `HandshakeCertificates` has no route in from one and converting it first would mean testing the conversion. badssl.com timed out mid-run twice while this was being written, so its clients carry a 30-second timeout — it is slow rather than broken — and a timeout skips rather than fails. A read timeout is a result about badssl's bandwidth, not about client certificates, and the preflight cannot catch one that arrives after it has already passed. Verified against `test-server` running natively, since these need Docker: identity accepted and the subject echoed, presented again after `evictAll`, `certificate_required` without one, and a stranger CA producing that same alert. `PublicClientCertificateTest` passes live, 2/2. Progresses #5. Closes #15. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
…m a right one `HttpSemanticsTest` (#9) runs against `test-server`, where the awkward cases can be arranged exactly rather than hoped for. `GoHttpbinTest` asks a similar family of questions of go-httpbin, which is the value of having both: two independent servers agreeing is evidence, and either disagreeing is the interesting result. The two worth naming. Redirects: 301, 302 and 303 rewrite a POST to a GET while 307 and 308 keep both the method *and* the body — a preserved method with a lost body is the worst of both and is a mistake a client can make in exactly one place. And `Authorization` is dropped on a redirect to another host and kept on one to the same host: forwarding it is a credential leak somebody else triggers, and never forwarding it breaks every authenticated redirect, so both halves are asserted. The second host is a second name for the same container, so the hop changes the host and nothing else. Also: gzip is undone on the way in and `deflate` is not — the fixture sends deflate whether asked or not and OkHttp hands it over untouched, so a caller who assumes "compressed responses are handled" gets bytes rather than text. A redirect chain past the limit fails with something to read rather than looping. 204 and 304 carry no body. A cookie round-trips given a jar, and only given one. A read timeout and a call timeout are told apart — and that case now asserts the call timeout is *not* a `SocketTimeoutException`, because the latter extends `InterruptedIOException` and the original assertion was satisfied by either, which is to say it distinguished nothing. The pinning fix is the more interesting one. CI failed `aPinForADifferentCertificateIsRefused` where the same test had passed locally, because the "different" pin was derived by editing the last character of the correct one. A pin is base64 of 32 bytes, so its final character carries four significant bits and two of padding: swapping `A=` for `B=` changes only padding and decodes to the identical hash. The fixture mints a fresh certificate per container, so that collided about one run in sixteen — a test that would have looked like a mystery flake for as long as anyone was willing to re-run it. The wrong pin is now taken from a different certificate, which cannot collide. Verified against `test-server` running natively, since these need Docker: redirect limit reported as `Too many follow-up requests: 21`, the cookie returned as `flavour=ginger`, the read timeout arriving as `SocketTimeoutException` and the call timeout as a plain `InterruptedIOException`, and both the flipped and the stranger pin refused. Progresses #5, #9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
…about them The public names all carry the same two parameters — `alpn` and the address hints — so `no-default-alpn`, `mandatory`, an unregistered key and AliasMode go untested for want of anywhere to test them against. The ECH fixture's DoH resolver now serves a name per parameter, each differing from an ordinary record in exactly one way, and `SvcParamTest` (#19) asks what OkHttp makes of each. Three are promises and are asserted. `no-default-alpn` removes the implied `http/1.1`, so `alpn=h2` means one protocol rather than two — a client ignoring it would believe an origin speaks something it has explicitly disclaimed. `mandatory=alpn` names something every client understands, so the record must be *used*; discarding it would be treating "mandatory" as "too hard". An unregistered key must be skipped rather than spoil the record, because the registry exists to be extended and a client that rejected the unknown would stop understanding names the moment anyone adopted something new. The fourth is a finding: OkHttp surfaces no metadata at all for an AliasMode record. Following an alias is arguably a resolver's job rather than a client's, so that is recorded rather than asserted — what is asserted is the half that is a promise, that an AliasMode record must not break ordinary resolution. Two build details. The suite needs `Dns.Record`, so the containers module gains the same 5.5.0 source-set gate the network module has, and reports the skip. And the fixture's Go source arrives by directory rather than by depending on `:ech-fixture`: that module's Kotlin uses `Path.of` and `Files.writeString`, which do not exist on the Java 8 floor these suites now compile for. Verified by running the resolver natively — built against the Go toolchain its Dockerfile pins — and querying it with a real `DnsOverHttps`: `[h2]` alone for the no-default-alpn name, `[h2, http/1.1]` and port 8443 for the mandatory and unknown-parameter names, and for the alias name an A record and no metadata. Progresses #5. Closes #19. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
yschimke
changed the title
…ead on the wire The rest of #9's bullets, against the same fixture. The cache case asserts the wiring rather than the policy: given a `Cache` and a response that permits caching, the second call is answered locally, and `networkResponse` being absent is what says so — a revalidation would still be a round trip. The authenticator is asked once and its answer is used, with the credential arriving on the *retry* rather than the first request: that is the whole shape of the mechanism, and a client that sent it eagerly would leak it to every server that never asked. Its companion is the bound — an authenticator that never gives up is stopped by OkHttp after 21 attempts rather than hammering somebody's login endpoint forever, which is the only thing standing between a mistaken `Authenticator` and an infinite loop. Multipart is where a client writes a body it also has to describe — boundaries, part headers, a trailing delimiter — and getting any of it wrong produces a request the server parses into something else rather than rejecting. `Expect: 100-continue` rides along in the same case. And `theRequestHeadIsRecorded` reads the raw listener, which is the only endpoint that reports what OkHttp actually sent rather than what Go parsed: `net/http` canonicalises header names and drops their order, and both are half of how a CDN fingerprints a client. Almost nothing is asserted about it, for the reason `ClientHelloTest` asserts almost nothing — the header set is a platform decision and pinning it would turn an upgrade into a failure. What is asserted is that the request is well-formed HTTP/1.1 and carries `Accept-Encoding: gzip`, which OkHttp adds itself and transparent decompression depends on. Verified natively: the second `/cache` request served with no network response, the authenticator consulted exactly once for a good credential, an always-retry authenticator stopped with `Too many follow-up requests: 21`, multipart and `100-continue` both echoed back, and the raw head reading `GET /probe HTTP/1.1 | Host | Connection | Accept-Encoding: gzip | User-Agent`. Progresses #5. Closes #9. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
Kotlin does not care and the build has no linter to say so, but every other file here is alphabetical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LTGos7kiusobSSixhsD19N
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9, #11, #12, #14, #15, #16, #19, #20, #21. Progresses #5.
Working the roadmap on the one designated branch, so this grew commit by commit. Seven commits, one per area — each reviewable on its own.
HttpsRecordTest,DnsFailureTest,HappyEyeballsTestHttp2Test,AltSvcTest,Http2CoalescingTestConnectionSpecTest,PinningTest,FixturePinningTestClientCertificateTest,PublicClientCertificateTestHttpSemanticsTestSvcParamTest+ fixture resolverThe findings worth reading
Several of these produced answers I didn't expect, and each one changed the test rather than the prose.
429is not anUnknownHostException. It raises a plainIOException, so code catching the former to mean "bad name" never sees a rate limit, and code using it to decide whether to retry decides backwards. Some resolvers also turn a DNSSEC failure into HTTP502, which the matrix could not represent — hence a newerroredoutcome.deflateis not decompressed. OkHttp offers and decodes gzip only; a caller assuming "compressed responses are handled" gets bytes.HTTPSrecord. Recorded, not asserted — following an alias is arguably the resolver's job.COMPATIBLE_TLS, which permits it. So those cases assert the refusal and nothing about its author.no-default-alpnworks.alpn=h2alone reports[h2, http/1.1]; with the parameter it reports[h2].Authenticatoris bounded at 21 attempts, which is the only thing between a mistaken authenticator and an infinite loop against somebody's login endpoint.Three tests that were wrong before CI ever saw them
Proxy.NO_PROXY— with a proxy in the way OkHttp connects to the proxy and the addresses under test are never dialled, so the suite passes having tested nothing. Caught because this repo's sandbox proxies HTTPS; it also meant the firstaddressHintsAreReachableasserted nothing.cloudflare.comandwww.cloudflare.comshare a certificate and sit on different edge addresses. Coalescing moved to the fixture, where the conditions are made rather than hoped for.aPinForADifferentCertificateIsRefused; a local run had passed. A pin is base64 of 32 bytes, so its last character carries four significant bits and two of padding —…A=and…B=decode identically. The fixture mints a fresh certificate per container, so it would have looked like a mystery flake forever. The wrong pin now comes from a different certificate.Fixture changes
test-servergains anmtlslistener that requires a client certificate (every other listener merely requests one, so "presented" and "ignored" were indistinguishable),/client.pemserving an identity its CA signed, and the ClientHello's extension IDs in/tls— most of what a JA3/JA4 fingerprint is built from, with GREASE values named and0xfe0dcalled out.The ECH fixture's DoH resolver gains four names publishing the SvcParams the internet doesn't.
Verification
Everything reachable from here was run live:
HttpsRecordTest5/5,DnsFailureTest4/4,DohMatrixTest7/7,Http2Test4/4,AltSvcTest6/6,PinningTest3/3,PublicClientCertificateTest2/2,HappyEyeballsTest3 + 1 skipped (no IPv6 on this runner, or any GitHub one).The container suites need Docker, which this environment lacks — so rather than leave them for CI to discover, each was verified against the fixtures running natively: the ConnectionSpec matrix across all four version listeners, coalescing (two names, one connection, h2 on each), mTLS (identity accepted and echoed, presented again after
evictAll,certificate_requiredwithout one), pinning both ways, redirects and credentials across hosts, the redirect limit, cookies, the cache, authenticator behaviour and its bound, multipart, the raw request head, and every SvcParam record through a realDnsOverHttps.Also compiles against the pinned 5.4.0 and against
-PtestJavaVersion=8, picking up #63's floor — which caughtDns.Callback.onRecords's parameter beinglast, notdone.Not in here
#30 is deployment infrastructure — a host, a DNS name, inbound 443, a Watchtower — not a test suite, and not something reachable from this environment. It stays open.
Two of #9's smaller items are also absent: digest auth needs an implementation OkHttp does not ship, and brotli/zstd need optional modules that are not on the classpath. Everything else on that issue is covered.