feat(quiche)!: add mTLS client auth and TLS server name control#325
Conversation
Upstream work for the TLS and certificates gaps tracked in moq-dev/moq#2296, where moq-native's quiche backend can't reach quinn/noq parity without new API here. Inbound mTLS: ServerBuilder::with_client_auth takes a ClientAuth policy (None/Optional/Required) that applies to both the static-cert and cert-resolver paths, and the verified chain is exposed via Connection::peer_certificates for callers that need a peer identity. Roots are parsed at build time rather than inside the TLS hook: a ConnectionHook can only report failure by returning None, which falls back to a context with no client authentication at all. Empty roots are rejected as a config error instead of failing closed on every client. For the same reason the server forces Settings::verify_peer off, since quiche applies it after the hook and would overwrite the verification mode, quietly turning a required client certificate into an optional one. Client hostname override: ClientBuilder::with_server_name decouples the TLS name from the dial target. quiche's set_host_name drives both SNI and the certificate hostname check, so one setter covers both. Also fixes Connection::server_name always returning None. It was read off the SslRef before start(), where quiche is still holding the Initial packet as unparsed bytes, so BoringSSL had never seen the ClientHello. It's now captured post-handshake alongside the peer chain. Clients consequently report the SNI they sent, where they previously reported None. BREAKING CHANGE: Incoming::server_name and Incoming::alpn are removed. Both described values that only exist once the handshake completes, so they returned None where the type invited callers to read them. They remain on Connection, which is only reachable via Incoming::accept or Connecting::established, so holding one already proves the handshake finished and a None now means genuinely absent rather than not yet known. Use ServerBuilder::with_cert_resolver to act on SNI mid-handshake. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Resolves a conflict with #322, which dropped ClientBuilder::with_metrics and its M type parameter from the same client builder this branch adds with_server_name to. Kept the new server_name field and took main's `..self` struct literals. This branch had spelled those literals out only because moving `metrics` explicitly was what kept it from tripping the dead_code lint; with the field gone, that reason goes with it.
#325 landed ClientBuilder::with_server_name on main with the same field and the same connect_with_config approach this branch used, so keep main's and drop this branch's duplicate. Also drop connect_to. It was proposed as the enabler for address-family-aware DNS selection, but the existing surface already covers that: std's ToSocketAddrs parses IP literals before falling back to DNS, so connect(&addr.ip().to_string(), port) already dials a chosen address without a lookup, and #325's with_server_name supplies the name to verify. connect_to only saved a stringify-and-reparse. Taking the first DNS result is also a bug rather than a knob to route around -- binding 0.0.0.0 and resolving AAAA first fails the same way -- so the fix belongs in connect() picking an address compatible with the socket, not in new API that every caller must opt into. Tracked separately along with the implicit-bind EINVAL that CodeRabbit found on this PR. What remains is what only this branch provides: keep-alive, the GSO opt-out, and the IPv6-literal URL fix. Server-side conflicts were with #325's with_client_auth landing beside these builder methods; both are kept. tests/verify.rs returns to main's copy, since #325's tests/server_name.rs now covers SNI. Those tests dial a hostname, so the IPv6-literal path is covered by the new tests/connect.rs, whose IPv6 case fails without the fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Upstream work for the TLS and certificates section of moq-dev/moq#2296. Those gaps split across two repos — this covers the parts that genuinely need new API here. Outbound client certs and the SNI cert resolver were already exposed (
ez::ClientBuilder::with_single_cert,ServerBuilder::with_cert_resolver), so those items are puremoq-nativewiring and aren't touched.Inbound mTLS + peer identity
ServerBuilder::with_client_authtakes a newClientAuthpolicy —None(default),Optional(roots), orRequired(roots)— applied to both the static-cert and cert-resolver paths. The verified chain comes back viaConnection::peer_certificates(), which is whatmoq-nativeneeds forRequest::peer_identity(reachable asrequest.conn().peer_certificates()).Two decisions worth a reviewer's attention:
ConnectionHookcan only report failure by returningNone, and tokio-quiche then falls back to a context with no client auth — a silent security downgrade. Empty roots are rejected as a config error rather than fail-closed on every client.Settings::verify_peeroff. quiche applies that flag after the hook and would overwrite the verification mode, quietly turningRequiredintoOptional.ClientAuthis the server-side expression of the same knob. Documented onwith_settingsand pinned by a test.Client hostname override
ClientBuilder::with_server_namedecouples the TLS name from the dial target, on both the raw-QUIC and WebTransport builders. quiche'sset_host_namedrives SNI and the certificate hostname check together, so one setter covers--client-tls-host-name.Fix:
server_name()always returnedNonePre-existing bug, found while testing the override.
ez::Server::run_socketread SNI off theSslRefbeforestart()— but at that point quiche is still holding the Initial packet as unparsed bytes inparams.initial_pkt, so BoringSSL had never seen the ClientHello andSSL_get_servernamereturned NULL. The cert-resolver path was unaffected, since its callback runs mid-handshake.It's now captured post-handshake via
qconn.server_name(), alongside the peer chain.Behavior change: clients now report the SNI they sent, where they previously reported
None(only the server path ever set it). That follows fromSSL_get_servername, but it's observable if anything relied onNoneto infer "this is the client side".Breaking:
Incoming::server_name/Incoming::alpnremovedBoth described values that only exist once the handshake completes, so they returned
Nonewhere the type invited you to read them. They remain onConnection, which is only reachable throughIncoming::acceptorConnecting::established— so holding one already proves the handshake finished, and aNonenow means genuinely absent (no SNI sent, no ALPN negotiated) rather than not-yet-known.with_cert_resolverremains the way to act on SNI mid-handshake, the last point a server can still choose a certificate or refuse.Safe for the known consumer:
moq-native's quiche backend already readspeer_addr()offIncomingandalpn()offConnectionafteraccept(). Nothing in this repo used either method.While documenting that invariant I found
poll_handshakewas keying offalpn.is_some()as its "established" signal, whileconnected()setsalpntoNonewhen nothing is negotiated. It holds today only becauseQuicSettings::alpndefaults to[b"h3"]; a caller passingSettingswith an emptyalpnwould have hung inaccept()forever. Now an explicitestablishedflag, so the guarantee doesn't rest on that coincidence.Tests
23 tests pass;
just checkandjust testare green.tests/client_auth.rs— required/optional × present/absent/untrusted, the peer chain the server observes, empty-roots rejection, and theverify_peerdowngrade guard. The load-bearing assertions are the negative ones: an untrusted chain must fail the handshake, and a required cert must stay required.tests/server_name.rs— SNI reporting, the override (accept + the reject that proves the check is real), and that SNI reaches aCertResolver. The two accessor tests fail without the fix (left: None, right: Some("localhost")).🤖 Generated with Claude Code