Bun.serve: honor requestCert/rejectUnauthorized on per-serverName tls entries - #36174
Merged
Merged
Conversation
Contributor
|
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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughPer-serverName TLS entries now propagate client-certificate policies through Rust, C, uWS, and OpenSSL layers. SNI selection applies verification settings and session isolation, handshake handling enforces SNI-specific rejection, and HTTPS tests cover trusted, untrusted, missing, and unmatched client certificates. ChangesPer-SNI client certificate policy
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/runtime/server/mod.rs (1)
2736-2754: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPropagate per-SNI client-cert policy to HTTP/3
src/runtime/server/mod.rs:2736-2740drops theapply_client_cert_policybit that the uWS path passes; withhttp3: true, a hostname usingrequestCert/rejectUnauthorizedcan bypass client-cert enforcement on H3. Thread the flag through the H3 wrapper and C++ shim too.🤖 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 `@src/runtime/server/mod.rs` around lines 2736 - 2754, Update the HTTP/3 SNI registration flow around add_server_name_with_options to pass the per-SNI apply_client_cert_policy flag, matching the uWS path. Thread this value through the H3 wrapper and its C++ shim, preserving requestCert/rejectUnauthorized enforcement for HTTP/3 hostnames.
🤖 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 `@test/js/bun/http/bun-serve-ssl.test.ts`:
- Around line 155-222: Extend the per-serverName TLS test around the request
helper and “requestCert/rejectUnauthorized…” case to explicitly exercise session
resumption: establish and retain a resumable session under lenient.example.com
or the default context, then reuse it when connecting to admin.example.com
without a client certificate. Assert the resumed strict-context connection is
rejected, while preserving the existing strict, trusted, untrusted, lenient, and
default assertions.
---
Outside diff comments:
In `@src/runtime/server/mod.rs`:
- Around line 2736-2754: Update the HTTP/3 SNI registration flow around
add_server_name_with_options to pass the per-SNI apply_client_cert_policy flag,
matching the uWS path. Thread this value through the H3 wrapper and its C++
shim, preserving requestCert/rejectUnauthorized enforcement for HTTP/3
hostnames.
🪄 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: 50a1eb85-4672-4f35-a8ea-14edc170aee4
📒 Files selected for processing (8)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/libusockets.hpackages/bun-uws/src/App.hpackages/bun-uws/src/HttpContext.hsrc/runtime/server/mod.rssrc/uws_sys/App.rssrc/uws_sys/libuwsockets.cpptest/js/bun/http/bun-serve-ssl.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Beyond the inline note, the HTTP/3 sibling at src/runtime/server/mod.rs:2740 (h3_app.add_server_name_with_options) was also checked for the same per-SNI policy gap and ruled out. This touches TLS client-cert auth and session-resumption paths across C/C++/Rust, so leaving for human review.
Extended reasoning...
This PR wires per-serverName requestCert/rejectUnauthorized through the SNI context switch in openssl.c, adds session-id-context isolation to prevent resumption bypass, and makes the handshake close decision consult the selected context. It is security-sensitive (TLS mutual auth gating) and spans BoringSSL ex_data, uWS handshake handling, and Rust FFI — not something to approve without human eyes. The one inline finding is a test-coverage nit on the sid_ctx hardening clause; the primary behavior is covered by the new test. The HTTP/3 add_server_name_with_options call in the same loop was examined as a potential unfixed sibling and ruled out by verifiers.
Collaborator
|
Updated 6:42 PM PT - Jul 29th, 2026
✅ @Jarred-Sumner, your commit 96c8f17eb0518e2aba56c8f23d42f2d5ed639343 passed in 🧪 To try this PR locally: bunx bun-pr 36174That installs a local version of the PR into your bun-36174 --bun |
Jarred-Sumner
force-pushed
the
claude/serve-sni-request-cert
branch
2 times, most recently
from
July 28, 2026 15:01
2084146 to
1955ec5
Compare
robobun
force-pushed
the
claude/serve-sni-request-cert
branch
from
July 28, 2026 21:00
1955ec5 to
bbf9533
Compare
Jarred-Sumner
force-pushed
the
claude/serve-sni-request-cert
branch
from
July 29, 2026 12:51
bbf9533 to
664cb5c
Compare
Jarred-Sumner
force-pushed
the
claude/serve-sni-request-cert
branch
2 times, most recently
from
July 29, 2026 17:52
7879dd2 to
2911f8b
Compare
…NI entries Each extra entry in `tls: [...]` is a full TLS config for a serverName, but its requestCert/rejectUnauthorized never took effect for that hostname: the SNI context switch (SSL_set_SSL_CTX) copies only the certificate, so the connection kept the default entry's verify mode, and the HTTP layer's close-on-verify-error decision was keyed only on the default entry. Record each per-serverName entry's client-certificate policy on its SSL_CTX. Every SNI switch path (sni_cb, the early select-certificate fallback and its resume path) now goes through one helper that adds the selected entry's requestCert/rejectUnauthorized to the connection, and the handshake handler consults the selected context for the close decision. Per-entry contexts get their own session-id context so a session established under another context is not resumed under a gated entry. The default entry and node:tls SecureContexts (server-level policy) are untouched, and the change is additive: an entry can add a client-certificate requirement for its name but never removes the default entry's.
Jarred-Sumner
force-pushed
the
claude/serve-sni-request-cert
branch
from
July 29, 2026 23:43
2911f8b to
96c8f17
Compare
Contributor
There was a problem hiding this comment.
Both of my earlier inline concerns are now addressed — the H3 gap is scoped in the description, and a1cc2d8 adds the session-resumption test that pins the SSL_CTX_set_session_id_context clause. I didn't find any remaining bugs, but this is a client-certificate auth gate threaded through the BoringSSL SNI-switch and handshake-close paths, so it's worth a human sign-off.
What was reviewed:
- All five
SSL_set_SSL_CTXsites in openssl.c now route throughus_ssl_apply_selected_ctx; policy is additive (OR'd into inherited verify_mode), so a per-name entry can't relax the default entry's requirement. us_socket_server_name_reject_unauthorizedguardss->ssland the ex-idx before dereferencing; node:tls SecureContexts have no ex_data set and fall through unchanged.- The two Rust callers pass
apply_client_cert_policycorrectly (false for the default entry's own hostname registration, true for non-default SNI entries). - Test wires
error/closeto resolve, usesport: 0, and covers gated-no-cert / trusted / untrusted / lenient / default plus cross-context session resumption.
Extended reasoning...
Overview
This PR makes requestCert / rejectUnauthorized on a non-default serverName entry in Bun.serve({ tls: [...] }) actually take effect. It touches:
- openssl.c: adds
us_ctx_sni_policy_ex_idx(packed policy on the per-name SSL_CTX),us_ssl_ctx_set_sni_policy(records policy + stamps a unique session-id context),us_ssl_apply_selected_ctx(the one helper everySSL_set_SSL_CTXsite now goes through, reapplyingSSL_VERIFY_PEER/SSL_VERIFY_FAIL_IF_NO_PEER_CERTon the connection), andus_socket_server_name_reject_unauthorized(queried by the handshake close decision). - HttpContext.h:
onHandshakenow ORs the per-SNI reject flag into the app-level one. - App.h / libuwsockets.cpp / uws_sys/App.rs / mod.rs: plumb an
apply_client_cert_policybool throughaddServerNameso only non-default per-serverNameentries get the policy stamped (the default entry's own hostname registration passesfalse). - bun-serve-ssl.test.ts: two new tests — the fresh-handshake matrix (gated no-cert / trusted / untrusted / lenient / default) and a session-resumption bypass test.
Security risks
This is a security gate — mutual-TLS enforcement per hostname. The specific risks I checked:
- Bypass via session resumption: a session from an ungated name resumed under a gated name would skip client auth. Mitigated by the per-context
SSL_CTX_set_session_id_contextand now covered by an automated test that fails when those two lines are removed. - Policy relaxation:
us_ssl_apply_selected_ctxORs into the inherited verify mode and only whenUS_SNI_POLICY_REQUEST_CERTis set — a per-name entry cannot clear a bit the default entry set. The PR description explicitly scopes it as additive-only. - node:tls SecureContext interference: those contexts never get
us_ssl_ctx_set_sni_policycalled on them, soSSL_CTX_get_ex_datareturns NULL → policy 0 → verify mode untouched, preserving server-level behavior. - Coverage of every SNI switch path: sni_cb, select_cert_cb (async resume, sync resume with resumed_ls, dyn, and the listener fallback) all go through the new helper.
- HTTP/3: not gated — pre-existing gap (no H3 mTLS at all), now stated in the PR description; Jarred agreed with this scoping.
Level of scrutiny
High. This is TLS authentication code in a hand-maintained BoringSSL integration layer, spanning C/C++/Rust. REVIEW.md specifically calls out TLS/crypto paths and "security checks fail closed and cover every path to the protected effect" as areas where human review is warranted. The change is well-structured (one shared helper, additive semantics, session-id isolation) and the tests are strong, but auto-approval is not appropriate for a new auth gate.
Other factors
- CI passed on the latest commit (Build #85540).
- Both prior inline comments (mine and CodeRabbit's) are resolved by a1cc2d8.
- The atomic
us_sni_sid_ctx_counteris process-global, which is fine — it only needs to be unique per process, and workers building SNI contexts concurrently is the reason for_Atomic. - No bugs surfaced by the bug-hunting system this run.
springmin
pushed a commit
to springmin/bun
that referenced
this pull request
Aug 4, 2026
… entries (oven-sh#36174) ### What does this PR do? With `Bun.serve({ tls: [ ...one entry per serverName... ] })`, the `requestCert` / `rejectUnauthorized` set on a non-default `serverName` entry did not take effect for connections to that name: the SNI context switch only carried over the certificate, and the handshake handler's close-on-verification-error decision was keyed on the default entry only. Client-certificate settings configured on a per-hostname entry were silently ignored for that hostname. Now each per-`serverName` entry's client-certificate policy is recorded on its context and applied when SNI selects it — through one helper shared by every switch path (servername callback, the early select-certificate fallback, and the resume path) — and the handshake handler consults the selected context for the close decision. Per-name contexts also get their own session-id context so a session established under another context is not resumed under a gated entry. Scope notes: the default entry and `node:tls` SecureContexts (server-level policy) are unchanged, and per-name policy is additive — an entry can add a client-certificate requirement for its name but does not remove one imposed by the default entry. ### How did you verify your code works? Added a test to `test/js/bun/http/bun-serve-ssl.test.ts`: with `tls: [default, {serverName: "admin.example.com", ca, requestCert, rejectUnauthorized}, {serverName: "lenient.example.com", ca, requestCert, rejectUnauthorized: false}]`, a client to `admin.example.com` without a certificate (or with one not chaining to the entry's `ca`) is refused, with a valid one is served, and the lenient and default names are served. `bun bd test test/js/bun/http/bun-serve-ssl.test.ts` passes (17/17); the new test fails on the current release. Also drove the debug build across all requestCert × rejectUnauthorized combinations and confirmed a session resumed from the default name is not accepted under the gated name. ## Scope note: HTTP/3 This applies the per-`serverName` `requestCert` / `rejectUnauthorized` policy on the TLS-over-TCP listener. Bun's HTTP/3 server does not yet have client-certificate support at all, so an H3 connection is not client-cert gated regardless of this change — that is a pre-existing capability gap, not something introduced here, and it is out of scope for this PR. --------- Co-authored-by: robobun <117481402+robobun@users.noreply.github.com>
This was referenced Aug 8, 2026
This was referenced Sep 17, 2026
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 25, 2026
…context (#42998) ### Problem - `tls.Server` with `ca: caA` plus `addContext("b.test", { ca: caB, key, cert })` refuses a caA client certificate at `b.test` (`UNABLE_TO_VERIFY_LEAF_SIGNATURE`). The same client keeps `a.test`'s `session`, offers it as `servername: "b.test"`, and the handler runs with `authorized: true`. TLS 1.3 and 1.2. - A resumed handshake skips client authentication. BoringSSL resumes only when the session's id context equals the connection's (`vendor/boringssl/ssl/ssl_session.cc:474`). No `node:tls` context sets one. - #36174 fixed this for `Bun.serve` `serverName` entries only. nginx fixed the class as CVE-2025-23419. ### Fix - Each `SSL_CTX` a server can serve gets the SHA-256 of its options as its session id context (`create_ssl_context_with_digest`): the `tls.Server` and `Bun.listen` listeners, `SSLContextCache`, `tls.createSecureContext()`. `addCACert` folds its certificate in. - BoringSSL checks the id after the SNI switch, and `SSL_set_SSL_CTX` copies the selected context's id. A mismatch is not an error: a full handshake runs and verifies the certificate against that context's CA. - The id is content-derived, so an `SNICallback` that builds a context per handshake still resumes. Client sockets keep no id: one `SSL_CTX` backs clients and servers, and a client aborts a resume when the ids differ. - Verified: `test/js/node/tls/node-tls-context.test.ts`, 12 new tests, 8 fail on main, each client clear covered by one that fails without it. Other suites and vendored tests: see the notes. ### Background - Session resumption: the client presents a ticket from an earlier connection, both sides skip the certificate exchange, and the server reports the verdict stored in the session. - Session id context: an opaque string on an `SSL_CTX`, copied into each new session. BoringSSL refuses a session whose id differs, and tells servers to partition sessions between SNI hosts this way (`include/openssl/ssl.h:2199-2207`). RFC 6066 section 3 allows resuming only a session established for the requested name. - SNI context: `addContext()` and `SNICallback` pick an `SSL_CTX` per ClientHello. The ticket keys belong to the listener default context, so every context of that listener decrypts its tickets. <details><summary>Notes</summary> **Outcome table** (client certificate issued by caA, session from `a.test` offered at `b.test`, which trusts only caB): | | bun main | this PR | |---|---|---| | `authorized` | `true` | `false` | | `authorizationError` | none | `UNABLE_TO_VERIFY_LEAF_SIGNATURE` | | `isSessionReused()` | `true` | `false` | | handler reached with the default `rejectUnauthorized` | yes | no | | `a.test` session at `a.test` | resumed | resumed | An independent client (`openssl s_client -servername a.test -sess_out`, then `-servername b.test -sess_in`) shows the same before and after, so the door does not depend on Bun's client. **Why the id is derived from the options and not from the context object.** #36174 uses a counter, which fits `Bun.serve` because those contexts are built once per server. A `node:tls` `SNICallback` commonly calls `tls.createSecureContext()` for each handshake. With an id per object such a server would never resume. With the options digest, two contexts built from the same options share sessions, which is safe because they authenticate clients the same way. The digest is the existing `BunSocketContextOptions::digest()`, the key of `SSLContextCache`: inline PEM content, and path plus mtime and size for the file options. It is broader than the client-certificate policy, so a change to an unrelated option (`ciphers`, `sigalgs`) also ends resumption for old sessions. That errs toward a full handshake. **Why the server side needs no code at the SNI switch.** `SSL_set_SSL_CTX` replaces the connection's `CERT` with a copy of the new context's `CERT`, and the session id context is a field of `CERT`. So the servername callback, the static SNI tree, the asynchronous `SNICallback` resume and `socket.setKeyCert()` all move the connection to the id of the selected context with no extra call. **Context sharing.** `SSLContextCache` shares one `SSL_CTX` between all users of the same options, clients and servers, which is why the client side clears the id per socket. **Client clears.** The two places that build an `SSL` are `us_internal_ssl_attach` (socket) and `SSLWrapper::init_with_ctx` (duplex, named pipe). `setKeyCert` is the only other caller of `SSL_set_SSL_CTX`, and on a client Bun applies it (Node ignores it there), so it needs the clear too. Tests: `lets a client offer a session under other client options` covers the first two (TCP and Duplex), `lets a client that calls setKeyCert() still resume` the third. Each fails with `ERR_SSL_ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT` when its clear is removed. A future `SSL_new` site for a client needs the same line. There is no lint for that. **Landing order.** #28691 (server `ticketKeys`) should land after this one: shared ticket keys let one process decrypt another's tickets, which extends the same door across servers and processes, and it needs a negative test for that. The options digest keeps cross-process resumption working for identical options, which a counter or a pointer would not. #33483 and #42050 touch the SNI switch and `setSecureContext`; they do not set a session id context and do not conflict. **Not covered.** `addCACert()` on a context that already serves connections: a handshake that selected the context before the call, and verifies its client certificate after it, issues a session under the old id. A second context built from the same options without that CA can then resume it. The window is one round trip and needs that sibling context. On `main` any context resumes any session, so this PR narrows that door. Node allows `addCACert()` at any time, so this PR adds no refusal. TLS 1.2 with an `ALPNCallback` that calls `socket.setKeyCert(ctx)`: BoringSSL decides TLS 1.2 resumption before the ALPN callback runs, so a session id context cannot reach that path. TLS 1.3 with the same setup is covered, because ALPN runs before session selection there. The `sessionIdContext` option is still accepted and dropped; honoring it belongs with #28691. `node:quic` has its own context path and is not changed. **Suites run on the debug build:** `node-tls-context.test.ts` (29 pass), `node-tls-connect.test.ts` (57 pass), `node-tls-server.test.ts` (77 pass, 1 failure that also fails without this diff: `localhost` resolves to `::1` first in this container), `node-tls-cert.test.ts`, `node-tls-namedpipes.test.ts`, `ssl-ctx-cache.test.ts`, `node-tls-upgrade.test.ts`, `node-tls-internals.test.ts`, `renegotiation.test.ts`, `node-tls-connect-hostname-verification.test.ts`, `bun-serve-ssl.test.ts` (18 pass), `fetch.tls.test.ts` (41 pass), `fetch-session.test.ts` (32 pass), `proxy.test.ts` (90 pass). Vendored Node tests: `test-tls-add-context`, `test-tls-sni-option`, `test-tls-sni-server-client`, `test-tls-sni-servername`, `test-tls-snicallback-error`, `test-tls-empty-sni-context`, `test-tls-client-resume`, `test-tls-client-resume-12`, `test-https-client-resume`, `test-tls-ticket`, `test-tls-ticket-cluster`, `test-tls-session-cache`, `test-tls-secure-session`, `test-tls-connect-secure-context`, `test-tls-secure-context-usage-order`, `test-tls-alpn-server-client`, `test-tls-psk-circuit`, `test-tls-psk-server`, `test-tls-reuse-host-from-socket`, `test-https-agent-session-reuse`, `test-https-agent-disable-session-reuse`, `test-https-agent-session-eviction`, `test-https-agent-session-injection`, `test-https-agent-sni`. **Comments.** The rationale for the deliberate refusal (RFC 6066 section 3, the BoringSSL header) sits on `us_ssl_apply_selected_ctx` in `openssl.c`, next to the SNI switch, and on the test block. The Rust call sites carry one line each. **Self-reviewed:** 6 concerns raised, 6 addressed (default-`rejectUnauthorized` coverage, the Windows named-pipe listener, the `setKeyCert` clear test, the scope claim above, the deliberate-refusal comments at the helper and the test, and the landing order). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 9 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 8 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-context.test.ts bun test v1.4.3 (c6b7fcb) test/js/node/tls/node-tls-context.test.ts: (pass) tls.Server > addContext [898.64ms] (pass) tls.Server > should select the most recently added SecureContext [175.29ms] (pass) tls.Server > should allow multiple CA [175.40ms] (pass) tls.Server > should allow multiple CA in newline-separated strings [53.03ms] (pass) tls.Server > SNI tls.Server + tls.connect [286.16ms] 475 | 476 | // b.test verifies the certificate again, against ca2, whichever 477 | // context's session the client offers. 478 | const fromDefault = await connect(server, "b.test", { session: atDefault.session }); 479 | const fromA = await connect(server, "b.test", { session: atA.session }); 480 | expect([fromDefault.seen, fromA.seen]).toEqual([refused("b.test"), refused("b.test")]); ^ error: expect(received).toEqual(expected) [ { - "authorized": false, - "error": "UNABLE_TO_VERIFY_LEAF_SIGNATURE", + "autho ... (truncated) release without fix: all passed bun test v1.4.3-canary.1 (e0d54a6) test/js/node/tls/node-tls-context.test.ts: (pass) tls.Server > addContext [29.23ms] (pass) tls.Server > should select the most recently added SecureContext [6.86ms] (pass) tls.Server > should allow multiple CA [4.38ms] (pass) tls.Server > should allow multiple CA in newline-separated strings [2.53ms] (pass) tls.Server > SNI tls.Server + tls.connect [11.38ms] (pass) session resumption across SNI contexts (TLSv1.3) > runs a full handshake under a context that trusts another CA [20.26ms] (pass) session resumption across SNI contexts (TLSv1.3) > keeps a refused client out of the handler with the default rejectUnauthorized [9.98ms] (pass) session resumption across SNI contexts (TLSv1.3) > covers the contexts an SNICallback builds for each handshake [10.63ms] (pass) session resumption across SNI contexts (TLSv1.3) > covers contexts that differ only by addCACert [11.52ms] (pass) session resumption across SNI contexts (TLSv1.3) > lets a client offer a session under other client options [9.06ms] (pass) session resumption across SNI contexts (TLSv1.3) > lets a client that calls setKeyCert() still resume [4.72ms] (pass) session resumption ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-context.test.ts bun test v1.4.3 (c6b7fcb) test/js/node/tls/node-tls-context.test.ts: (pass) tls.Server > addContext [765.80ms] (pass) tls.Server > should select the most recently added SecureContext [124.51ms] (pass) tls.Server > should allow multiple CA [201.14ms] (pass) tls.Server > should allow multiple CA in newline-separated strings [75.66ms] (pass) tls.Server > SNI tls.Server + tls.connect [278.18ms] (pass) session resumption across SNI contexts (TLSv1.3) > runs a full handshake under a context that trusts another CA [495.27ms] (pass) session resumption across SNI contexts (TLSv1.3) > keeps a refused client out of the handler with the default rejectUnauthorized [221.28ms] (pass) session resumption across SNI contexts (TLSv1.3) > covers the contexts an SNICallback builds for each handshake [164.83ms] (pass) session resumption across SNI contexts (TLSv1.3) > covers contexts that differ only by addCACert [121.09ms] (pass) session resumption across SNI contexts (TLSv1.3) > lets a client offer a session und ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 783ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/129] gen generated_host_exports.rs generated_host_exports.rs: 121 exports (host=5, lazy=10, generic=106, rust=0); 242 extern-C blocks audited [2/129] gen cpp.rs (cppbind) [3/129] gen JSSink.{cpp,h,lut.h,rs} generated_jssink.rs: 7 sinks, 84 exported symbols Generating /workspace/bun/build/release/codegen/JSSink.lut.h from /workspace/bun/build/release/codegen/JSSink.lut.txt [4/129] gen JS modules (bundle-modules) Preprocess modules (12205ms) Bundle modules (134ms) Postprocesss modules (434ms) Bundle Functions (731ms) Generate Code (51ms) [13.57s] Bundled "src/js" for production 2600 kb 197 internal modules 13 native modules 50 internal functions across 16 files [4/27] cargo bun_runtime → libbun_runtime.a �[1m�[33mwarning�[0m�[1m: binary `bun_shim_impl` should have a kebab-case name�[0m �[1m�[94m|�[0m �[1m�[94m 1�[0m �[1m�[94m|�[0m /workspace/bun/build/release/rust-target/.../bun_shim_impl �[1m�[94m|�[0m �[1m�[33m^^^^^^^^^^^^^�[0m �[1m�[94m|� ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` packages/bun-usockets/src/crypto/openssl.c | 15 +- src/boringssl_sys/boringssl.rs | 10 ++ src/runtime/api/bun/SSLContextCache.rs | 2 +- src/runtime/api/bun/SecureContext.rs | 27 ++- src/runtime/socket/Listener.rs | 5 +- src/runtime/socket/tls_socket_functions.rs | 4 + src/uws/lib.rs | 6 +- src/uws_sys/SocketContext.rs | 19 ++ test/js/node/tls/node-tls-context.test.ts | 280 ++++++++++++++++++++++++++++- 9 files changed, 356 insertions(+), 12 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests packages/bun-usockets/src/crypto/openssl.c 5 2 23 src/boringssl_sys/boringssl.rs 1 3 23 src/runtime/api/bun/SSLContextCache.rs 1 1 23 src/runtime/api/bun/SecureContext.rs 2 3 23 src/runtime/socket/Listener.rs 3 2 23 src/runtime/socket/tls_socket_functions.rs 1 1 23 src/uws/lib.rs 1 2 24 src/uws_sys/SocketContext.rs 3 2 23 test/js/node/tls/node-tls-context.test.ts 6 7 23 ``` </details> <!-- robobun:evidence:end -->
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.
What does this PR do?
With
Bun.serve({ tls: [ ...one entry per serverName... ] }), therequestCert/rejectUnauthorizedset on a non-defaultserverNameentry did not take effect for connections to that name: the SNI context switch only carried over the certificate, and the handshake handler's close-on-verification-error decision was keyed on the default entry only. Client-certificate settings configured on a per-hostname entry were silently ignored for that hostname.Now each per-
serverNameentry's client-certificate policy is recorded on its context and applied when SNI selects it — through one helper shared by every switch path (servername callback, the early select-certificate fallback, and the resume path) — and the handshake handler consults the selected context for the close decision. Per-name contexts also get their own session-id context so a session established under another context is not resumed under a gated entry.Scope notes: the default entry and
node:tlsSecureContexts (server-level policy) are unchanged, and per-name policy is additive — an entry can add a client-certificate requirement for its name but does not remove one imposed by the default entry.How did you verify your code works?
Added a test to
test/js/bun/http/bun-serve-ssl.test.ts: withtls: [default, {serverName: "admin.example.com", ca, requestCert, rejectUnauthorized}, {serverName: "lenient.example.com", ca, requestCert, rejectUnauthorized: false}], a client toadmin.example.comwithout a certificate (or with one not chaining to the entry'sca) is refused, with a valid one is served, and the lenient and default names are served.bun bd test test/js/bun/http/bun-serve-ssl.test.tspasses (17/17); the new test fails on the current release. Also drove the debug build across all requestCert × rejectUnauthorized combinations and confirmed a session resumed from the default name is not accepted under the gated name.Scope note: HTTP/3
This applies the per-
serverNamerequestCert/rejectUnauthorizedpolicy on the TLS-over-TCP listener. Bun's HTTP/3 server does not yet have client-certificate support at all, so an H3 connection is not client-cert gated regardless of this change — that is a pre-existing capability gap, not something introduced here, and it is out of scope for this PR.