Repository navigation
fetch: honor per-request TLS options on the HTTP/3 client - #31420
Jarred-Sumner wants to merge 3 commits into
Conversation
The HTTP/3 client previously ignored the request's tls options (ca, cert, key, serverName): every connection verified against the default trust store, so fetch with a custom CA failed the handshake and client certificates were never presented, unlike the TCP path. Mirror the TCP client's one-SSL-context-per-interned-SSLConfig design: - quic.c: factor the engine construction tail of us_create_quic_client_context into a shared helper and add us_create_quic_client_context_with_options, which builds the SSL_CTX from us_bun_socket_context_options_t via us_ssl_ctx_build_raw (same primitive the QUIC server context and the TCP client use) before installing the existing custom verify callback. - ClientContext keeps a per-interned-SSLConfig quic context registry (only for configs with requires_custom_request_ctx, matching HTTPThread's gate) and uses tls.serverName as the SNI when set. - ClientSession stores the config and includes it in matches(), so sessions with different TLS configs are never coalesced (same keying as the TCP keep-alive pool and the h2 session pool). rejectUnauthorized-only requests keep using the default context, and the default client context construction is unchanged.
WalkthroughThis PR enables per-request TLS configuration for HTTP/3 QUIC clients. It introduces a C FFI layer for options-based context creation, implements a Rust-level per-SSLConfig context cache with lazy initialization and centralized unsafe access, integrates context selection into the connection flow with TLS-aware SNI extraction, extends session matching to respect TLS config boundaries, updates HTTP/3 eligibility gating, and provides comprehensive test coverage. ChangesPer-Request TLS for HTTP/3 QUIC
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
|
Updated 1:25 PM PT - May 27th, 2026
✅ @Jarred-Sumner, your commit 2faf71662bbf3f3b9df088547e02b0c4822bc8cc passed in 🧪 To try this PR locally: bunx bun-pr 31420That installs a local version of the PR into your bun-31420 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this adds new SSL_CTX construction and per-config session keying in the TLS verification path for the HTTP/3 client — security-sensitive enough that a human should sign off.
Extended reasoning...
Overview
This PR makes the experimental HTTP/3 fetch client honor per-request tls options (ca, cert/key, serverName). It touches six files: quic.c/quic.h (new us_create_quic_client_context_with_options that builds an SSL_CTX from us_bun_socket_context_options_t via the existing us_ssl_ctx_build_raw, with the engine-construction tail factored into a shared static helper), src/uws_sys/quic/Context.rs (FFI binding for the new constructor), src/http/h3_client/ClientContext.rs (a custom_contexts registry keyed by interned SSLConfig pointer, plus tls.serverName → SNI), src/http/h3_client/ClientSession.rs (store tls_props and include it in matches() so sessions with different TLS configs are not coalesced), and new tests covering tls.ca, wrong-CA rejection, mTLS, non-coalescing, and the Alt-Svc upgrade path.
Security risks
This is squarely in TLS-verification territory. The change reuses the existing us_quic_client_verify callback and the same us_ssl_ctx_build_raw primitive the QUIC server and TCP client already use, and the requires_custom_request_ctx gate mirrors HTTPThread::connect. Session coalescing now keys on the interned-config pointer (via SSLConfig::raw_ptr), which is the same identity scheme the TCP keep-alive pool uses. I did not spot a way for a request without a custom CA to piggyback on a custom-CA session (the matches() pointer-equality check covers that, and there is a test for it). Still, mistakes here affect certificate validation, so the cost of a subtle bug is high.
Level of scrutiny
Medium-high. The feature is behind an experimental flag and the default code path (us_create_quic_client_context) is byte-for-byte unchanged after the refactor, so blast radius for users who have not opted in is small. But the change is not mechanical: it adds a new C entry point, new unsafe FFI in Rust, a process-lifetime registry of lsquic engines (one per distinct config, never freed — called out in the PR notes), and changes session-reuse keying. Those are real design decisions a maintainer should glance at.
Other factors
No CODEOWNERS cover these paths. Test coverage for the new behavior is good (positive ca, wrong ca, mTLS accept/reject, non-coalescing, Alt-Svc subprocess). The bug-hunting system found nothing. There are no prior human review comments to address. Given the security-sensitive nature plus the moderate size, I am deferring rather than approving.
…tom trust options are supported
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/web/fetch/fetch-http3-client.test.ts (1)
712-719: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
-efor this single-file subprocess fixture.This helper only executes one generated script, so the temp directory and
cwdsetup are unnecessary here.♻️ Suggested simplification
-import { bunEnv, bunExe, expiredTls, tempDir, tls } from "harness"; +import { bunEnv, bunExe, expiredTls, tls } from "harness"; ... async function run(extra: { env?: Record<string, string>; args?: string[]; opts?: unknown }) { - using dir = tempDir("h3-altsvc", { - "fixture.ts": fixture(extra.opts ?? { tls: { rejectUnauthorized: false } }), - }); await using proc = Bun.spawn({ - cmd: [bunExe(), ...(extra.args ?? []), "fixture.ts"], + cmd: [bunExe(), ...(extra.args ?? []), "-e", fixture(extra.opts ?? { tls: { rejectUnauthorized: false } })], env: { ...bunEnv, ...extra.env }, - cwd: String(dir), stderr: "pipe", });As per coding guidelines, "For single-file tests, prefer
-eovertempDir" and "Use-eflag withbunExe()for single-file tests; for multi-file tests, create a temporary directory usingtempDirfrom harness`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/js/web/fetch/fetch-http3-client.test.ts` around lines 712 - 719, The test helper function run currently creates a tempDir with "fixture.ts" and launches it via Bun.spawn using cwd, which is unnecessary for a single-file fixture; instead, remove the tempDir and cwd usage and invoke bunExe() with the "-e" flag to evaluate the generated script text directly (update call sites that build the script via fixture(extra.opts...) to pass the script string to Bun.spawn via cmd: [bunExe(), "-e", script] and preserve env merging and args handling), and drop tempDir(...) and cwd: String(dir) references so run uses direct "-e" execution for single-file subprocesses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http/h3_client/ClientContext.rs`:
- Around line 211-245: The code caches a quic::Context per interned SSLConfig in
context_for using custom_contexts and never frees them, leaking FDs and QUIC
loop registrations; fix by making custom_contexts bounded or reference-counted
and teardown contexts when unused: in context_for (where you push
(config.clone(), ctx)) change the entry to include a usage count or use an
LRU/bounded map keyed by cfg_ptr, increment the count when a ClientSession is
created for that config and decrement on session drop (hook into ClientSession
creation/destructor), and when evicting or refcount reaches zero call the QUIC
teardown API and undo the registration (e.g., invoke the loop unregister
equivalent to callbacks::register(Self::ctx_mut(ctx)) and free the quic::Context
via the library’s context deletion function) before removing the entry from
custom_contexts; ensure qctx_mut(), ctx_mut(), and context_for logic use the
same keys so lookups remain correct.
---
Outside diff comments:
In `@test/js/web/fetch/fetch-http3-client.test.ts`:
- Around line 712-719: The test helper function run currently creates a tempDir
with "fixture.ts" and launches it via Bun.spawn using cwd, which is unnecessary
for a single-file fixture; instead, remove the tempDir and cwd usage and invoke
bunExe() with the "-e" flag to evaluate the generated script text directly
(update call sites that build the script via fixture(extra.opts...) to pass the
script string to Bun.spawn via cmd: [bunExe(), "-e", script] and preserve env
merging and args handling), and drop tempDir(...) and cwd: String(dir)
references so run uses direct "-e" execution for single-file subprocesses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 956f3231-612e-43af-b4d1-dbadc077963f
📒 Files selected for processing (7)
packages/bun-usockets/src/quic.cpackages/bun-usockets/src/quic.hsrc/http/h3_client/ClientContext.rssrc/http/h3_client/ClientSession.rssrc/http/lib.rssrc/uws_sys/quic/Context.rstest/js/web/fetch/fetch-http3-client.test.ts
💤 Files with no reviewable changes (1)
- src/http/lib.rs
There was a problem hiding this comment.
I didn't find any bugs, but this changes TLS trust-store / cert-verification plumbing and session coalescing for the HTTP/3 client across C and unsafe Rust FFI, so it's worth a human look.
Extended reasoning...
Overview
This PR makes the experimental HTTP/3 fetch client honor per-request tls options (ca, cert/key, serverName). It refactors us_create_quic_client_context in quic.c to share an engine-construction tail and adds us_create_quic_client_context_with_options (building the SSL_CTX via the existing us_ssl_ctx_build_raw), exposes it through new Rust FFI in uws_sys/quic/Context.rs, and wires it into ClientContext (a per-interned-SSLConfig registry of process-lifetime quic contexts) and ClientSession (storing the config and including it in matches() so differently-configured sessions are never coalesced). lib.rs drops the requires_custom_request_ctx guard so these requests now reach the h3 path. Tests cover custom CA accept/reject, mTLS, session non-coalescing, and the Alt-Svc upgrade path.
Security risks
This is squarely security-sensitive: it controls which trust store and client certificate are used for QUIC TLS handshakes, and which existing pooled session a request is allowed to reuse. A mistake in matches() keying or in the requires_custom_request_ctx gate could let a request with a custom CA piggyback on a default-trust (or rejectUnauthorized:false) session, or vice-versa. The new C path replaces the default Mozilla/system root store with whatever us_ssl_ctx_build_raw derives from user options and reinstalls the same us_quic_client_verify custom-verify callback — the design mirrors the TCP path and looks correct, but the consequences of a subtle bug here are MITM-class.
Level of scrutiny
High. Even though the feature is behind BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP3_CLIENT / protocol: "http3", it touches BoringSSL SSL_CTX construction, X.509 verification flow, SNI selection, and session-pool keying, plus new unsafe Rust FFI with raw-pointer lifetime invariants (ctx_mut, context_for) and process-lifetime leaked engines/UDP endpoints per distinct SSLConfig.
Other factors
The implementation is well-structured, closely mirrors the existing TCP-path design (interned SSLConfig → dedicated SSL context, same requires_custom_request_ctx gate), and ships solid test coverage (54/54 passing per the description, including negative cases and non-coalescing). The bug-hunting system found no issues. Still, given the TLS-trust surface and cross-language unsafe code, this is exactly the kind of change a human reviewer should sign off on rather than a bot.
What
The experimental HTTP/3 client (
protocol: "http3"/BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP3_CLIENT) ignored the request'stlsoptions. Every QUIC connection verified the server certificate against the default trust store, sofetch(url, { protocol: "http3", tls: { ca } })failed withHTTP3HandshakeFailedeven when the CA was correct, and client certificates (cert/key) were never presented — unlike the same options over the TCP path.This makes the HTTP/3 client honor
ca,cert/key, andserverName, mirroring the TCP client's one-SSL-context-per-interned-SSLConfigdesign:us_create_quic_client_contextinto a shared static helper and addus_create_quic_client_context_with_options, which builds theSSL_CTXfromus_bun_socket_context_options_tvia the existingus_ssl_ctx_build_raw(the same primitive the QUIC server context and the TCP client use — Nodecasemantics, exclusive trust store, client cert/key loading) before applying the existing TLS 1.3 + custom-verify setup. The default client context construction is unchanged.SSLConfigquic contexts, gated onrequires_custom_request_ctxexactly likeHTTPThread::connect, sotls: { rejectUnauthorized: false }-only requests keep sharing the default context.tls.serverNameis used as the SNI / verification name when set.matches(), so sessions with different TLS configs are never coalesced — same keying as the TCP keep-alive pool (HTTPContext) and the h2 session pool.checkServerIdentityremains unsupported on h3 (the existingCertErrorsguards inlib.rsare unchanged). Default (flag-off) behavior is untouched: the new C function is additive and the new Rust code is only reachable from the h3 connect path.Why
Custom-CA and mTLS deployments could not use the HTTP/3 client at all; the options were silently ignored, which also meant the only workaround was
rejectUnauthorized: false.Verification
bun bd test test/js/web/fetch/fetch-http3-client.test.ts— 54/54 pass, including new tests:tls.cawith the harness self-signed cert succeeds over forced http3 (fails on current Bun withHTTP3HandshakeFailed)tls.cais rejectedrequestCert: true, rejectUnauthorized: true, caaccepts a fetch presentingcert/keyand rejects one withouttls.ca(subprocess fixture with the experimental flag)test/js/web/fetch/fetch-http3-adversarial.test.ts: identical results before/after this change on the same machine.DEPTH_ZERO_SELF_SIGNED_CERT-style failure). Node comparison is not applicable (undici has no per-requesttlsoption and no h3 client); the TCP fetch path is the reference behavior.cargo checkclean for the touched crates on linux-x64, windows-x64 (msvc), and darwin-aarch64; clippy clean.Notes
SSLConfiggets its own lsquic engine + lazily-created UDP endpoint, kept for the process lifetime (the existing default client context is also never freed). Configs are interned and typically few; eviction can be added later if needed.ca/certcurrently maps toConnectionRefusedon the h3 path (TCP maps it toFailedToOpenSocket); can be refined later by threading the error out of the new C constructor.