tls: rotate the live certificate on setSecureContext() and Bun.serve reload({tls}) - #33365
tls: rotate the live certificate on setSecureContext() and Bun.serve reload({tls})#33365robobun wants to merge 11 commits into
Conversation
|
Updated 6:58 AM PT - Jul 29th, 2026
❌ @robobun, your commit bb4392c has some failures in 🧪 To try this PR locally: bunx bun-pr 33365That installs a local version of the PR into your bun-33365 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds runtime TLS context replacement for listening servers. It introduces a uSockets SSL_CTX swap API, Rust and Listener bindings, live ChangesLive TLS Secure Context Rotation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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 touches SSL_CTX lifetime management across C/Rust/JS FFI boundaries and changes live TLS certificate handling — worth a human pass on the ref-counting and the ALPNProtocols behavior change.
Extended reasoning...
Overview
This PR makes tls.Server.setSecureContext() actually rebuild the live SSL_CTX on an already-listening server (previously a silent no-op). It spans three languages: a new C function us_listen_socket_set_ssl_ctx() in usockets that swaps the listen socket's default SSL_CTX and re-arms SNI callbacks; a Rust Listener::set_secure_context() that parses fresh TLS options, builds a new context, and swaps it in with proper cleanup on failure; a JS-side hook in tls.ts that invokes the native path when _handle exists; and a struct field change from Option<NonNull<SSL_CTX>> to Cell<Option<NonNull<SSL_CTX>>> plus a new server_name field. It also changes setSecureContext() to stop clearing ALPNProtocols when omitted (matching Node). Three new tests cover cert rotation, addContext/ALPN preservation, and failure-path rollback.
Security risks
This is squarely security-sensitive code: live TLS certificate rotation, SSL_CTX reference counting across an FFI boundary, and SNI callback re-registration. The primary risks are (a) an SSL_CTX ref-count imbalance leading to UAF or leak — the C function up-refs before dropping and the Rust side releases the retiring ref only after the swap succeeds, which looks correct, but the ownership handoff crosses three layers; (b) the SNI-tree entry migration checking node->ctx == old to distinguish the listen-time hint from user addContext() entries — subtle enough to warrant scrutiny; (c) the re-entrancy guard where parsing options can run user JS that closes the server, handled by re-reading this.listener.get() after SSLConfig::from_js.
Level of scrutiny
High. Per the repo's own review guidance, TLS/crypto paths and manual ref-counting across FFI are the most-blocked category. The change is well-tested and the reasoning is clearly documented, but the correctness of SSL_CTX_up_ref/SSL_CTX_free pairing across us_listen_socket_set_ssl_ctx → ListenSocket::set_ssl_ctx → Listener::set_secure_context (particularly the double up-ref: one in C for ls->ssl_ctx, one in Rust kept as this.secure_ctx) deserves a maintainer's eyes.
Other factors
The ALPNProtocols behavior change reverses a prior explicit decision (the removed comment said "An omitted ALPNProtocols clears the previous call's protocols"). The PR argues Node never touches ALPN in setSecureContext(), which is correct, but a maintainer should confirm they're comfortable with the semantic reversal. The tests are solid — they exercise the SNI path with the bind hostname, verify addContext survives, and check the failure path leaves the live context intact. No CODEOWNERS check performed, but packages/bun-usockets and TLS bindings typically warrant owner review.
|
Two things worth writing down for whoever reviews this, since both were flagged as wanting a human pass.
|
| ref | taken by |
|---|---|
| 1 | Listener::secure_ctx (the one create_ssl_context() returns) |
| 2 | ls->ssl_ctx (us_internal_init_listen_socket up_refs) |
| 3 | the SNI-tree node for the bind hostname (us_listen_socket_add_server_name up_refs) |
setSecureContext() builds N (refcount 1, owned by Rust) and then:
us_listen_socket_set_ssl_ctx()up_refsNand stores it inls->ssl_ctx→N= 2- if the SNI node still points at
O, it up_refsN, freesO's node reference, and repoints →N= 3 - it unrefs
O'sls->ssl_ctxreference - back in Rust,
secure_ctx.replace(N)freesO's listener reference and keepsN's build reference →N= 3
So N lands on exactly the three references listen() would have left, O drops to zero once the last live SSL goes away, and teardown is unchanged (us_internal_listen_socket_ssl_free drops two, Listener::deinit drops one). Every reference is taken before the matching one is dropped, so even N == O would be a no-op rather than a use-after-free. If set_ssl_ctx() reports a non-TLS listener, Rust frees N's single reference and nothing else moves.
Empirically: 50 back-to-back rotations leave sslCtxLiveCount() (the atomic us_ssl_ctx_build_raw bumps and the ex_data free callback decrements) unchanged at its pre-rotation value, and the new tests run under ASAN.
The node->ctx == old check in step 2 is what keeps an addContext("<bind hostname>", ctx) entry from being clobbered: Listener::add_server_name removes and re-adds the node with the caller's own context, so it no longer points at the default one and the migration skips it. There is a test for exactly this.
The ALPNProtocols change
Node's Server.prototype.setSecureContext (lib/_tls_wrap.js) assigns pfx/key/cert/ca/ciphers/minVersion/... and rebuilds _sharedCreds. ALPNProtocols is not in that list; it is only read by the Server constructor. Confirmed against Node 26:
const s = tls.createServer({ key, cert, ALPNProtocols: ["h2", "http/1.1"] });
s.setSecureContext({ key, cert });
s.ALPNProtocols; // <Buffer 02 68 32 08 68 74 74 70 2f 31 2e 31> — unchangedClearing it was harmless while setSecureContext() was a no-op. Once the rotation is live it would drop the server's ALPN list mid-flight, and that is precisely the path @grpc/grpc-js takes (Http2SecureServer extends tls.Server, and grpc-js calls setSecureContext() on credential reload). So the two changes have to land together.
The second commit is unrelated to the fix: SNICallback runs even when the requested servername matches the bind hostname was dialing localhost by name after binding to it, which is ECONNREFUSED on a dual-stack host because the two resolutions need not agree (Node splits the same way). It now dials the address the server reports and passes servername explicitly; the property under test is unchanged.
There was a problem hiding this comment.
No bugs found, but this spans C/Rust/JS with manual SSL_CTX refcounting and SNI-tree mutation on a live listener — worth a human pass on the lifecycle and the ALPNProtocols behavior change.
Extended reasoning...
Overview
This PR makes tls.Server.setSecureContext() actually rebuild the live TLS context on an already-listening server (previously a silent no-op). It threads a new us_listen_socket_set_ssl_ctx() primitive through uSockets C code, adds Rust FFI bindings in ListenSocket.rs, implements Listener::set_secure_context() in Listener.rs (converting secure_ctx to a Cell and persisting the bind-hostname server_name), and wires it from src/js/node/tls.ts. It also changes setSecureContext() to preserve ALPNProtocols when omitted rather than clearing it. Three new tests cover rotation, SNI/ALPN preservation, and the failure path.
Security risks
This is squarely security-sensitive: it swaps the SSL_CTX that every subsequent accepted connection handshakes with, rewires the SNI servername/select-certificate callbacks onto the fresh context, and moves the bind-hostname's SNI-tree node across contexts. A refcount imbalance here would either leak an SSL_CTX (memory) or free one still referenced by an in-flight accept (UAF in BoringSSL). The C function is careful — it up-refs before freeing so ctx == old is a no-op — and the Rust side re-reads the listener after SSLConfig::from_js (which can run user JS) and frees the new ctx on the non-TLS-listener bail-out. I didn't spot a leak or UAF, but manual refcounting across an FFI boundary in the TLS accept path is exactly the class of change that benefits from a second set of eyes.
Level of scrutiny
High. Per the repo's own review guidance, TLS/crypto paths and manual reference-count balancing across C↔Rust are the most-blocked category. The change also alters user-visible Node-compat behavior (ALPNProtocols is now preserved on omit instead of cleared), which the description justifies against Node semantics but is a behavior change nonetheless.
Other factors
The PR is well-structured with a clear root-cause explanation, differential testing against Node, and an sslCtxLiveCount() leak check across 50 rotations mentioned in the description. The bug-hunting system found nothing. Test coverage looks solid (rotation with SNI, addContext/ALPN survival, bad-cert rollback). The unrelated hunk in the SNICallback bind-hostname test is a flakiness fix (dial the resolved address instead of re-resolving "localhost"). None of that changes the fact that this is a multi-layer TLS lifecycle change that shouldn't be auto-approved.
13d135e to
0e96bdc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/node/tls.ts (1)
1387-1391: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winNormalize
rejectUnauthorizedto a strict boolean before storing.This stores the raw
options.rejectUnauthorizedwhen defined. The new live-rotation path at Line 1419 (this[buntls](0, undefined, false)[0]) now feedsthis._rejectUnauthorizedstraight into the live native listener context, so a non-boolean falsy value likenull(typeof null !== "undefined"is true) is stored verbatim and can weaken client-cert verification on an mTLS server — Node only disables verification when the value is explicitlyfalse.Root cause is the raw assignment here; the changed rotation call amplifies the impact.
Based on learnings,
options.rejectUnauthorizedmust be normalized to a strict boolean (e.g.rejectUnauthorized !== false) anywhere it is read, includingServer.prototype.setSecureContext, because non-boolean falsy values may bypass certificate verification.🛡️ Proposed fix
const rejectUnauthorized = options.rejectUnauthorized; if (typeof rejectUnauthorized !== "undefined") { - this._rejectUnauthorized = rejectUnauthorized; + this._rejectUnauthorized = rejectUnauthorized !== false; } else this._rejectUnauthorized = rejectUnauthorizedDefault();🤖 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/js/node/tls.ts` around lines 1387 - 1391, Normalize rejectUnauthorized to a strict boolean wherever it is read, including the shown initialization logic and Server.prototype.setSecureContext. Store true unless the option is explicitly false (for example, using rejectUnauthorized !== false), while preserving rejectUnauthorizedDefault() when the option is undefined, so live rotation via buntls receives only boolean values.Source: Learnings
🤖 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.
Outside diff comments:
In `@src/js/node/tls.ts`:
- Around line 1387-1391: Normalize rejectUnauthorized to a strict boolean
wherever it is read, including the shown initialization logic and
Server.prototype.setSecureContext. Store true unless the option is explicitly
false (for example, using rejectUnauthorized !== false), while preserving
rejectUnauthorizedDefault() when the option is undefined, so live rotation via
buntls receives only boolean values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6de8dfa5-6d7d-49ea-898e-906e08253a40
📒 Files selected for processing (3)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/libusockets.hsrc/js/node/tls.ts
|
Re Applied in aff360d — the field is now normalized to a strict boolean ( For the record, the rotation path could not actually weaken verification even before the normalization: the native consumer reads the option as One more thing worth noting for the reviewer: Node's |
1 similar comment
|
Re Applied in aff360d — the field is now normalized to a strict boolean ( For the record, the rotation path could not actually weaken verification even before the normalization: the native consumer reads the option as One more thing worth noting for the reviewer: Node's |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/runtime/socket/Listener.rs (2)
793-797: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not report a failed native context swap as success.
When
set_ssl_ctxreturnsfalse, the new context is freed butundefinedis returned. The caller therefore observes success while JavaScript credentials and the active native context diverge. Throw an appropriate error while retaining the old context.As per coding guidelines, never swallow a failure or signal success on one.
🤖 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/socket/Listener.rs` around lines 793 - 797, In the `set_ssl_ctx` failure branch, do not return `Ok(JSValue::UNDEFINED)` after freeing `ctx`; preserve the existing context and return an appropriate JavaScript error instead. Update the surrounding listener method to propagate the failure while retaining the current cleanup of the newly created native context.Source: Coding guidelines
756-760: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRotate TLS contexts for Windows named-pipe listeners.
This returns success for an active TLS
NamedPipe, so subsequent connections retain the old certificate while JavaScript state reflects the replacement. Implement the corresponding named-pipe context swap and cover it with a Windows test.As per coding guidelines, cover every platform branch and the full behavioral variant matrix.
🤖 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/socket/Listener.rs` around lines 756 - 760, The TLS rotation guard in the listener rotation method incorrectly excludes active Windows named-pipe listeners. Add the named-pipe branch to swap its active TLS context while preserving the existing JS-side options, and retain the unlistened/non-TLS behavior. Update the Windows test coverage to exercise active and unlistened named pipes, TLS and non-TLS cases, and successful rotation with subsequent connections using the replacement certificate.Source: Coding guidelines
src/js/node/tls.ts (1)
1416-1423: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake live secure-context updates transactional.
All TLS fields are committed before the native rebuild. If rebuilding throws, the old listener context remains active but
this.key,this.cert, and related fields retain the rejected configuration; a later re-listen can then fail unexpectedly. Build from candidate values and commit only after success, or restore the previous state incatch.As per coding guidelines, error paths must leave cross-layer state consistent.
🤖 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/js/node/tls.ts` around lines 1416 - 1423, Make setSecureContext’s live update transactional: preserve the existing TLS fields, construct the native listener context from the candidate configuration, and only commit the candidate fields after setListenerSecureContext succeeds. If rebuilding throws, restore or retain every related field (including key, cert, and options) so JavaScript and native listener state remain consistent.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/js/node/tls.ts`:
- Around line 1416-1423: Make setSecureContext’s live update transactional:
preserve the existing TLS fields, construct the native listener context from the
candidate configuration, and only commit the candidate fields after
setListenerSecureContext succeeds. If rebuilding throws, restore or retain every
related field (including key, cert, and options) so JavaScript and native
listener state remain consistent.
In `@src/runtime/socket/Listener.rs`:
- Around line 793-797: In the `set_ssl_ctx` failure branch, do not return
`Ok(JSValue::UNDEFINED)` after freeing `ctx`; preserve the existing context and
return an appropriate JavaScript error instead. Update the surrounding listener
method to propagate the failure while retaining the current cleanup of the newly
created native context.
- Around line 756-760: The TLS rotation guard in the listener rotation method
incorrectly excludes active Windows named-pipe listeners. Add the named-pipe
branch to swap its active TLS context while preserving the existing JS-side
options, and retain the unlistened/non-TLS behavior. Update the Windows test
coverage to exercise active and unlistened named pipes, TLS and non-TLS cases,
and successful rotation with subsequent connections using the replacement
certificate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97b127c4-5835-42e5-b828-6299b83ed2c2
📒 Files selected for processing (2)
src/js/node/tls.tssrc/runtime/socket/Listener.rs
|
Addressing the three findings in the latest review:
Windows named-pipe TLS listeners (Listener.rs:756-760) — good catch, fixed in 6829525. Transactional JS fields (tls.ts:1416-1423) — not changing this. Node is not transactional here either, verified against Node 26: Node's |
|
CI status on this branch at bb4392c (build 84990): the diff is green for its scope. The only non-passing lanes never ran a test:
Both are macOS CI fleet issues, not this diff. |
There was a problem hiding this comment.
The requestCert/rejectUnauthorized fix in 1396bff looks right and the two new tests cover both failure modes I flagged. Deferring to a human for the overall SSL_CTX refcount accounting across the C/Rust boundary and the verify-mode semantics — this is live TLS certificate rotation and worth a second pair of eyes.
Extended reasoning...
Overview
This PR makes tls.Server.prototype.setSecureContext() actually rebuild the live SSL_CTX on a listening server (previously a silent no-op). It adds us_listen_socket_set_ssl_ctx() in usockets C, a Rust FFI wrapper in ListenSocket.rs, Listener::set_secure_context() in Listener.rs (plus converting secure_ctx to a Cell and the Windows named-pipe ctx likewise), the JS-side call in tls.ts with preserved ALPNProtocols/requestCert/rejectUnauthorized and the net.ts verify-mode clamp, and five new tests in node-tls-server.test.ts.
Security risks
This is squarely security-sensitive: it swaps the live SSL_CTX on a TLS listener, re-arms SNI callbacks, migrates the bind-hostname SNI-tree entry, and derives the rebuilt context's SSL_VERIFY_* mode from JS-side requestCert/rejectUnauthorized. My earlier review caught that the rotation path bypassed net.ts's listen-time !requestCert → rejectUnauthorized=false clamp and cleared _requestCert on omission — both would have shifted the verify mode on rotation (certless clients rejected, or mTLS servers silently dropping CertificateRequest). That's fixed in 1396bff with tests. The refcount accounting (three owned refs per context, up-ref-before-free ordering, ctx == old no-op safety) is documented in a PR comment and reads correctly to me, and the rejectUnauthorized normalization is fail-closed (!== false).
Level of scrutiny
High. This is production-critical TLS certificate rotation touching BoringSSL SSL_CTX lifetime management across a C/Rust FFI boundary, SNI-tree mutation, and Node-compat verify-mode semantics. A mistake here is either a UAF on SSL_CTX, a leaked context per rotation, or a silent weakening/strengthening of client-cert enforcement after a cert renew — none of which fail loudly.
Other factors
The PR has been through several review rounds with substantive fixes (aff360d normalized rejectUnauthorized, 6829525 added the Windows named-pipe swap and the debug_assert! on the unreachable non-TLS-listener branch, 1396bff fixed the verify-mode regression I flagged). Test coverage is good — five new cases including the two verify-mode regressions, the failure path, and SNI/ALPN survival — and the author reports sslCtxLiveCount() is stable across 50 rotations under ASAN. The bug hunter found nothing on the current head. I'm comfortable with the JS-side changes; the cross-language refcount plumbing and the SNI-tree migration in openssl.c are where I'd want a maintainer who knows that code to sign off.
1396bff to
9f06e57
Compare
There was a problem hiding this comment.
The verify-mode issue I flagged earlier is addressed in 9f06e57 — _requestCert/_rejectUnauthorized are now preserved when omitted and the net.ts listen-time clamp is applied before the native rebuild, with two new tests covering both directions. No further issues found this pass.
That said, this is live SSL_CTX rotation with hand-tracked refcounts across C/Rust/JS plus SNI-tree migration and verify-mode semantics, so it's worth a human sign-off.
What was reviewed:
SSL_CTXreference accounting throughus_listen_socket_set_ssl_ctx→secure_ctx.replaceon both the Uws and Windows named-pipe arms, and theListenerType::None/!swappedrelease paths.- The SNI-tree migration's
node->ctx == oldguard vs.addContext()entries, and re-arming ofsni_cb/us_select_cert_cbon the fresh context. setSecureContextoption handling:ALPNProtocolspreserved on omission,rejectUnauthorizednormalized to a strict boolean, and the!requestCert → rejectUnauthorized = falseclamp matching net.ts's listen path.
Extended reasoning...
Overview
This PR makes tls.Server.prototype.setSecureContext() actually rebuild the live SSL_CTX on a listening server (previously a silent no-op). It spans six files: a new us_listen_socket_set_ssl_ctx() in usockets C (openssl.c, libusockets.h), a Rust FFI wrapper (ListenSocket.rs), Listener::set_secure_context() plus Cell-wrapping of secure_ctx/server_name and the Windows named-pipe context (Listener.rs), the JS integration in tls.ts, and five new tests in node-tls-server.test.ts.
Security risks
This is squarely security-sensitive: it swaps the server's TLS identity mid-flight and touches the CTX-level verify mode (SSL_VERIFY_PEER / SSL_VERIFY_FAIL_IF_NO_PEER_CERT). The verify-mode regression I flagged on a prior revision — where a rotation could either start hard-failing certless clients or stop sending CertificateRequest on an mTLS server — has been fixed and is now covered by two dedicated tests. The rejectUnauthorized normalization is fail-closed (value !== false), and the native Listener.reject_unauthorized gate is deliberately left at its listen-time value so a rotation cannot relax post-handshake enforcement. The C swap takes every new reference before dropping any old one, so ctx == old is a safe no-op rather than a UAF.
Level of scrutiny
High. Manual SSL_CTX refcounting across a C/Rust FFI boundary, an SNI-tree pointer rewrite, and verify-mode semantics that gate client-cert enforcement are exactly the kind of change CLAUDE.md flags for human review. The refcount table in the PR thread checks out against the code, and the author reports sslCtxLiveCount() is stable across 50 rotations under ASAN, but a maintainer familiar with the usockets SSL lifecycle should confirm the SNI-tree migration and the Windows named-pipe arm.
Other factors
The PR has already been through two substantive review rounds (Windows named-pipe support added, rejectUnauthorized normalization, the verify-mode fix), each with new tests. The five new tests exercise the swap itself, SNI/ALPN survival, the failure path, and both verify-mode directions; the PR description documents differential testing against Node 26. The unrelated fetch-gzip CI failure is confirmed to reproduce on main. Given the security surface and the cross-language refcount choreography, deferring to a human reviewer rather than auto-approving.
There was a problem hiding this comment.
The requestCert/rejectUnauthorized fix in 9f06e57 addresses my earlier finding — the clamp is now applied before the rebuild and both fields are preserved on omission, with tests covering both directions. No further issues found this pass, but given the scope (live SSL_CTX swap with manual refcounting across the C/Rust/JS boundary, verify-mode handling, plus a Windows-only named-pipe arm) this should get a human review before merging.
Checked this run: the SSL_CTX reference accounting in us_listen_socket_set_ssl_ctx (up_ref before free, SNI-node migration guarded by node->ctx == old); the ListenerType::None re-read after SSLConfig::from_js in case user JS closed the server mid-parse; the secure_ctx Cell release in deinit and the error-path cleanup guard; and that on_server_name is not re-armed on rotation (it doesn't need to be — us_select_cert_cb is reinstalled from ls->on_server_name inside the C swap).
Extended reasoning...
Overview
This PR makes tls.Server.prototype.setSecureContext() actually rebuild the live SSL_CTX on a listening server instead of only updating JS-side fields. It adds us_listen_socket_set_ssl_ctx() in usockets (C), a Rust FFI wrapper in ListenSocket.rs, Listener::set_secure_context() in Listener.rs (with a Windows named-pipe branch), and wires it from tls.ts's setSecureContext. It also fixes setSecureContext clearing ALPNProtocols and (after my earlier review) preserves requestCert/rejectUnauthorized across rotations and re-applies net.ts's listen-time verify-mode clamp. Five new tests cover cert rotation, SNI/ALPN survival, failure-path preservation, and the two verify-mode regressions.
Security risks
This is squarely in security-sensitive territory: it swaps the SSL_CTX a TLS server uses for subsequent handshakes, moves an SNI-tree entry, reinstalls SNI callbacks, and touches how SSL_VERIFY_PEER / SSL_VERIFY_FAIL_IF_NO_PEER_CERT are derived on the rebuilt context. A refcount imbalance is a UAF on an SSL_CTX; a wrong verify-mode derivation either locks out all clients or silently stops requesting client certificates on an mTLS server. The PR thread contains a detailed refcount accounting (3 owned refs before and after, every up_ref precedes its matching free) and reports sslCtxLiveCount() unchanged over 50 rotations under ASAN, which is reassuring but doesn't remove the need for a human pass on the C swap and the Windows arm.
Level of scrutiny
High. Per the repo guidelines this falls under "security-sensitive code (auth, crypto, permissions)" — live TLS certificate/verify-mode rotation with manual SSL_CTX_up_ref/SSL_CTX_free across an FFI boundary, plus a #[cfg(windows)] branch that CI on this run may or may not have exercised. The change is well-reasoned and well-tested, but it is not mechanical.
Other factors
My earlier finding (rotation shifting the CTX-level verify mode) was addressed in 9f06e57 with the exact fix suggested plus two tests that fail on the prior commit. The remaining CI red (fetch-gzip.test.ts, a napi flake) was investigated by the author and reproduces on main with this diff stashed, so it's unrelated. The ALPNProtocols behavior change is a Node-compat correction bundled with this PR because it becomes load-bearing once rotation is live — that coupling is explained in the thread. Given all that, deferring rather than approving.
The native SSL_CTX is built once, from the server's options, when listen() runs. setSecureContext() only reassigned the JS-side option fields, so a listening server kept serving the certificate it started with and live certificate rotation silently did nothing. Rebuild the context and swap it into the listen socket, carrying the SNI tree entry the bind hostname was registered under. Sockets already accepted keep the certificate they handshook with; subsequent handshakes get the new one, matching Node. Also stop clearing ALPNProtocols: Node's setSecureContext() replaces the default context only and leaves the server's ALPN list alone. Unrelated to the fix, make the SNICallback bind-hostname test dial the address listen() reported instead of re-resolving "localhost"; on a dual-stack host the two resolutions need not agree (Node splits the same way), so the test was ECONNREFUSED there.
Node only disables verification on an explicit `false`; null, 0 and the empty string all keep it enabled. setSecureContext() now feeds this field back into the native listener on every rotation, so fail closed.
set_secure_context() early-returned for ListenerType::NamedPipe, so a TLS server listening on a Windows named pipe kept serving its original certificate after setSecureContext(). Swap WindowsNamedPipeListeningContext.ctx the same way; each accept up_refs per connection so the retiring context stays alive for established clients. Also documents (via debug_assert) why us_listen_socket_set_ssl_ctx cannot report failure on the Uws arm.
…text() The rotation path handed the raw [buntls] output to the native context builder, bypassing the clamp net.ts applies at listen() time (`if (!tls.requestCert) tls.rejectUnauthorized = false`). On a server with `ca` but no `requestCert` the rebuilt context ended up with SSL_VERIFY_FAIL_IF_NO_PEER_CERT, so every certless client was aborted at the handshake after a rotation. setSecureContext() was also clearing _requestCert when omitted, so a key/cert-only rotation on an mTLS server rebuilt the context with request_cert=0 and stopped sending CertificateRequest. Node's setSecureContext() never touches requestCert/rejectUnauthorized; only the constructor sets them. Keep both when omitted, normalize rejectUnauthorized to a strict boolean when passed, and apply the listen-time clamp before rebuilding.
…te it
server.reload({tls}) was a silent no-op: ServerConfig::from_js parsed the
replacement tls options, but on_reload_from_zig never looked at ssl_config,
so new connections kept receiving the original certificate and a mismatched
key/cert pair or garbage PEM was accepted without error.
reload() now builds the replacement SSL_CTX up front, so a bad PEM
(ERR_OSSL_PEM_NO_START_LINE) or key/cert mismatch
(ERR_OSSL_X509_KEY_VALUES_MISMATCH) throws the same error Bun.serve()
throws at startup and no handler or route is swapped. A valid rotation is
applied to the listen socket via us_listen_socket_set_ssl_ctx, so
subsequent handshakes use the new certificate while connections already
accepted keep the one they handshook with.
The --hot reload path goes through the same on_reload_from_zig, so it
picks up the rotation too.
d2f56c4 to
99b40c9
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
…the rebase Main removed it in #35002; the rebase onto that change resolved the ListenSocket.rs conflict by keeping both sides, which re-introduced the wrapper, its FFI extern, and the NonNull import with no callers.
…ls}) uWS's addServerName registers a separate per-domain context in the SNI tree, not the app's default ssl_ctx, so us_listen_socket_set_ssl_ctx's `node->ctx == old` check never held on that path and a client sending SNI matching the configured serverName kept receiving the retired certificate. Add a force flag so the Bun.serve caller (which has no addContext) moves the entry unconditionally; node:tls keeps the heuristic so an addContext() entry on the bind hostname survives.
… context; read serverName from its listen-time slot Two follow-ups to cac9561 for Bun.serve({tls:{serverName}}).reload({tls}): - us_listen_socket_set_ssl_ctx no longer sets us_sni_ex_idx on the rotated context. That context is also ls->ssl_ctx, so stamping node->user (uWS's per-domain HttpRouter*) onto it routed every no-SNI client via that router, whose UserRoute* entries were freed when reload() rebuilt self.user_routes. After the swap node->ctx == ls->ssl_ctx anyway, so both SNI-matching and no-SNI clients fall through to the freshly reloaded default router. - on_reload_from_zig read the SNI-migration hostname from self.config.ssl_config and then overwrote it with the incoming config, so a reload that omitted or changed serverName made the next one pass the wrong hostname and skip the SNI swap. Store the listen-time name once on the server struct (same as Listener.server_name on the node:tls path) and read that instead.
…ve rebuild throws The next-staging pattern keeps every JS-side validator throw before any this.* mutation, but the native setListenerSecureContext() call ran after the commit. A bad PEM or key/cert mismatch left this.cert/this.key at the rejected values while the native listener still served the previous certificate, so a later listen() or the STARTTLS wrap (both of which read those fields directly) failed with the same OSSL error the caller had already caught. Snapshot before committing and restore on throw.
There was a problem hiding this comment.
No new findings on this pass — every issue raised in earlier rounds (dead find_server_name_userdata binding, force_sni ex_data stamping the domainRouter onto the default ctx, serverName drift across reloads, torn this.cert on a failing native rebuild) has been addressed and is now covered by a test. Deferring to a human because this is ~580 lines across the TLS certificate-rotation path with manual SSL_CTX refcounting over FFI, which is squarely in the security-sensitive / memory-safety category the approval bar excludes.
What was reviewed:
us_listen_socket_set_ssl_ctxrefcount ordering — up_ref before free on bothls->ssl_ctxand the SNI node;ctx == oldis a no-op, not UAF.Listener::set_secure_context— the fresh ctx is freed on every non-swap path (!swapped,ListenerType::Noneafter re-read, NamedPipe replace);secure_ctx.replacedrops exactly the listener's prior ref.on_reload_from_zigthrow ordering — context build fails beforeclear_routes()/handler swap;SSL_CTX_freeruns regardless ofswapped.- tls.ts snapshot/restore —
commit(prev)on throw keepsthis.cert/buildSharedCredsconsistent; the!requestCert => rejectUnauthorized=falseclamp mirrors net.ts's listen path.
Extended reasoning...
Overview
Wires live TLS certificate rotation on both server surfaces: tls.Server.prototype.setSecureContext() and Bun.serve().reload({tls}). Adds a new usockets C primitive us_listen_socket_set_ssl_ctx() that swaps a listen socket's default SSL_CTX, re-arms SNI callbacks, and migrates the bind-hostname SNI-tree entry. Rust wrappers land in uws_sys/ListenSocket.rs and uws_sys/App.rs; Listener::set_secure_context() and on_reload_from_zig() drive them. tls.ts gains a snapshot/commit/restore around the now-fallible native rebuild, plus ALPNProtocols/requestCert preservation fixes. Seven new tests across two files.
Security risks
This is TLS credential-handling code. The interesting failure modes are (a) leaking or double-freeing SSL_CTX refs across the swap, (b) leaving an SNI-tree entry pointing at a freed context, (c) silently accepting a bad rotation, and (d) changing verify-mode semantics (requestCert/rejectUnauthorized) on rotation. Each is exercised: refcounts are up-then-down on both the default and node contexts; the SNI node's ctx is replaced only when it pointed at the retiring default (or force_sni for uWS's separate domainCtx); bad PEM / key-cert mismatch throws before any swap on both surfaces; and the !requestCert => rejectUnauthorized=false clamp is applied before rebuild with a dedicated test. The earlier UAF (per-domain router ex_data on the rotated default ctx) was fixed in 5a6ac3b and has a regression test.
Level of scrutiny
High. Manual SSL_CTX_up_ref/SSL_CTX_free across a C/Rust FFI boundary, on a hot path that every accepted TLS socket depends on, with two intentionally-scoped-out variants (h3, array-form tls) documented in the description. This went through 8 iterations with two 🔴-severity fixes during review. It is well past the "simple/mechanical" bar for auto-approval.
Other factors
Test coverage is thorough for the in-scope surface (rotation observed via CN, SNI-match and no-SNI clients, addContext survival, ALPN survival, failure-path rollback, requestCert preservation, close+relisten after a rejected rotation). All prior review threads are resolved. The Windows named-pipe ctx field change from Option<NonNull<...>> to Cell<Option<NonNull<...>>> looks mechanically correct (get_accepted_by up_refs per accept, deinit frees via .take()), but I could not exercise it. Given the security-sensitive surface and the review history, a human should sign off.
1 participant
What does this PR do?
Live certificate rotation was a silent no-op on both TLS server surfaces:
tls.Server.prototype.setSecureContext()succeeded, emitted no error, and the server kept serving its original certificate until the process restarted. Since live rotation (certbot/ACME renew hooks, k8s secret reloads) is the only reason the method exists, the first symptom was an expired-certificate outage weeks later.Bun.serve().reload({tls})swapped the handler but ignoredtlsentirely, and a mismatched key/cert pair or garbage PEM was accepted without error, so a bad rotation failed silently while the server kept serving the retired certificate.Reproduction
The
tls.Serverpath also happens throughhttp2.createSecureServer()(Http2SecureServer extends tls.Server), which is how@grpc/grpc-jsrotates credentials.Cause
The native
SSL_CTXis built once at listen time.setSecureContext()only reassigned the JS-side fields (this.key,this.cert, ...) that[buntls]reads at listen, so nothing rebuilt the context the listen socket hands to each accepted socket.on_reload_from_zigparsed the replacementtlsintonew_config.ssl_configand then dropped it without touching the listener.Fix
us_listen_socket_set_ssl_ctx()(usockets) swaps the listen socket's defaultSSL_CTX, re-arming the SNI callbacks on the new context and moving the SNI-tree entry the bind hostname was registered under. AnaddContext()entry on that same name is left alone, since it is not the listen-time hint.Listener::set_secure_context()parses the fresh options, builds the context, performs the swap and releases the retiring reference.tls.tscalls it fromsetSecureContext()whenever the server is already listening.on_reload_from_zigbuilds the replacement context first, throwsERR_OSSL_PEM_NO_START_LINE/ERR_OSSL_X509_KEY_VALUES_MISMATCH(matchingBun.serve()startup) before any handler or route is swapped, then applies the rotation viaus_listen_socket_set_ssl_ctx. The--hotreload path goes through the same function, so it picks up the rotation too.Sockets already accepted keep the certificate they handshook with (their
SSL_newholds its own reference); only subsequent handshakes see the new one, matching Node's documented semantics.addContext()entries,SNICallback, and the server'sALPNProtocolsall survive the swap.A rotation whose context fails to build (bad PEM, key/cert mismatch, unusable cipher list) throws on both surfaces and leaves the live context untouched.
setSecureContext()also stops clearingALPNProtocols: Node replaces the default context only and never touches the ALPN list; clearing it meant a cert rotation on an HTTP/2 server dropped the server's ALPN state. And it now preservesrequestCert/rejectUnauthorizedwhen omitted (Node only sets them in the constructor) and applies net.ts's listen-time!requestCert => rejectUnauthorized = falseclamp before rebuilding, so a server withcabut norequestCertdoes not start rejecting every certless client after a rotation.The
Bun.serverotation covers the default certificate and theserverNameentry on a single-objecttlsconfig. Two sibling forms are intentionally out of scope here (neither is a regression;reload({tls})was a no-op for every form before):http3: trueoption keeps its QUIC context'sSSL_CTX(there is nous_quic_socket_context_set_ssl_ctxprimitive yet).tls: [primary, ...secondary]: onlyprimaryis validated and rotated; secondary entries (new_config.sni) are not touched, so their SNI-tree entries keep serving their original certificate. Rotating those requires iterating the per-domain contexts and routers, which is its own change.How did you verify your code works?
test/js/node/tls/node-tls-server.test.tsgains five cases andtest/js/bun/http/bun-serve-ssl.test.tsgains two, each of which fails onmainand passes here:addContext()entries andALPNProtocolssurvive the swaprequestCert/rejectUnauthorizedare preserved across a rotationBun.serve().reload({tls})rotates the default certificateBun.serve().reload({tls})rejects a mismatched pair withERR_OSSL_X509_KEY_VALUES_MISMATCHand garbage PEM withERR_OSSL_PEM_NO_START_LINE, and keeps serving the previous certificateDifferentially checked against Node for the default context, the SNI-tree entry,
addContext,SNICallback, ALPN, the failure path, andsetSecureContext()beforelisten(). 50 back-to-back rotations leavesslCtxLiveCount()unchanged, so noSSL_CTXleaks.Unrelated to the fix, one commit makes
SNICallback runs even when the requested servername matches the bind hostnamedial the addresslisten()reported instead of resolvinglocalhosta second time; the name has both an A and an AAAA record on a dual-stack host and the two resolutions need not agree.[review] gate passed · iteration 8 · 11 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 8
evidence per changed file