Conversation
|
Updated 2:08 PM PT - Jul 25th, 2026
❌ @robobun, your commit 06f5cdb has 1 failures in
🧪 To try this PR locally: bunx bun-pr 28691That installs a local version of the PR into your bun-28691 --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:
WalkthroughAdded TLS session-ticket key support to the native Listener, exposed JS bindings for getTicketKeys/setTicketKeys with argument validation, updated Node.js tls.Server wrappers to call the native bindings, and added regression tests covering retrieval, setting, validation, and pre-listen behavior. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bun.js/api/bun/socket/Listener.zig`:
- Around line 924-943: The setTicketKeys function currently throws "Server is
not listening" when this.socket_context is null; change it to silently no-op by
returning .js_undefined instead to match the JS tls.ts wrapper behavior. Locate
the code in setTicketKeys where you read const ctx = this.socket_context orelse
return global.throw("Server is not listening", .{}); and replace that throw-path
with a plain return .js_undefined; so that when socket_context is null the
function returns undefined rather than raising an exception. Ensure you keep the
rest of the function unchanged (including the ssl check and ticket key
validations).
In `@test/regression/issue/14604.test.ts`:
- Around line 1-5: Add a single-line comment with the GitHub issue URL at the
very top of the test file test/regression/issue/14604.test.ts (above the
existing imports). The comment should follow repository convention (one line,
e.g. `// https://github.com/<org>/<repo>/issues/14604`) so it’s discoverable
alongside the test that references this issue; place it before the import
statements that include `import { expect, test } from "bun:test";`.
🪄 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: 441b39fb-98c7-4b8d-9a16-2b755b9f1958
📥 Commits
Reviewing files that changed from the base of the PR and between 17616ae and dbccc02f59fba458f4d36b609600a064282cb301.
📒 Files selected for processing (3)
src/bun.js/api/bun/socket/Listener.zigsrc/js/node/tls.tstest/regression/issue/14604.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/14604.test.ts`:
- Around line 50-63: Add an additional assertion path that exercises the
DataView branch: create a DataView over a new ArrayBuffer(48) (populate its
bytes to 0xab via a Uint8Array view of that buffer), call
server.setTicketKeys(dataView), then call server.getTicketKeys() and assert
byteLength is 48 and first/last bytes equal 0xab; reuse the existing test setup
(test name, tls_mod.createServer, server.listen, once) and the same expectations
to keep JS validator and Zig coercion aligned.
- Around line 65-70: Add a complementary assertion that calling getTicketKeys()
on the created server before it starts listening does not throw: in the same
test where you create the server via tls_mod.createServer(tls) and assert
setTicketKeys is a no-op, also call server.getTicketKeys() and assert it does
not throw (e.g., expect(() => server.getTicketKeys()).not.toThrow()), then close
the server; reference the existing test and the server variable to locate where
to insert this assertion.
🪄 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: 8a5ffa46-9e45-4310-be32-1ef9aeada3e1
📥 Commits
Reviewing files that changed from the base of the PR and between dbccc02f59fba458f4d36b609600a064282cb301 and 2281bec4d4403c2ba8e1159be643df4a80d195c9.
📒 Files selected for processing (3)
src/bun.js/api/bun/socket/Listener.zigsrc/js/node/tls.tstest/regression/issue/14604.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/regression/issue/14604.test.ts`:
- Around line 43-48: The test that calls tls_mod.createServer and invokes
(server as any).setTicketKeys should be converted to an async test that uses
`await using` (or `using`) to deterministically dispose the server instead of
manually calling server.close(); specifically replace the synchronous test body
with an async function that does `await using (const server =
tls_mod.createServer(tls)) { ... }` (or equivalent) so assertions run inside the
using block and the server is always closed even if an assertion throws; apply
the same change to the other test that allocates `server` in this file (the
similar test around the later range) so both use deterministic disposal rather
than trailing server.close().
🪄 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: 5b2ef405-481c-45cc-84d1-87f2df64466e
📥 Commits
Reviewing files that changed from the base of the PR and between 2281bec4d4403c2ba8e1159be643df4a80d195c9 and 52d835dccf5928f010b342a9a4cfe750aba22695.
📒 Files selected for processing (2)
src/bun.js/api/bun/socket/Listener.zigtest/regression/issue/14604.test.ts
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
4e6faf9 to
8427161
Compare
|
Wow. This is wild. |
8427161 to
f13ed5a
Compare
e2a3e88 to
55108c7
Compare
There was a problem hiding this comment.
No further issues found after the 780b107 fix — deferring to a human since this adds new unsafe Rust/BoringSSL FFI for TLS session ticket key material.
Extended reasoning...
Overview
This PR wires up tls.Server.getTicketKeys() / setTicketKeys() end-to-end: two new FFI declarations in src/boringssl_sys/boringssl.rs, ~100 lines of new Rust in src/runtime/socket/Listener.rs (an ssl_ctx() helper, get_ticket_keys/set_ticket_keys impls, and two #[bun_jsc::host_fn] shims with unsafe FFI and pointer derefs), ~20 lines in src/js/node/tls.ts replacing the stubs and validating input, and a 97-line regression test. All prior inline feedback (mine and CodeRabbit's) has been addressed and resolved; the most recent fix in 780b107 restores the Node-compatible validation text so test-tls-ticket-invalid-arg.js passes again.
Security risks
Session ticket keys are cryptographic key material used to encrypt TLS session state. The implementation is a thin pass-through to BoringSSL's SSL_CTX_{get,set}_tlsext_ticket_keys with a fixed 48-byte length check on both the JS and Rust sides, so there's no obvious injection, overflow, or key-exposure surface introduced. get_ticket_keys copies into a stack [u8; 48] and returns a fresh Buffer; set_ticket_keys length-checks before the FFI call. The unsafe blocks are narrowly scoped and commented. Still, this is exactly the category (TLS/crypto + raw FFI) where a second pair of human eyes is the right bar.
Level of scrutiny
Medium-high. The change is conceptually simple (two BoringSSL accessors), but it spans the JS↔native boundary, adds new unsafe Rust against an SSL_CTX*, and handles cryptographic key material in a production-critical path. That puts it outside the "simple/mechanical" envelope I'd auto-approve.
Other factors
The bug-hunting system found nothing on the current revision. Test coverage is reasonable (round-trip set→get, length/type validation, Uint8Array/DataView inputs, pre-listen behavior), and the upstream Node parallel test test-tls-ticket-invalid-arg.js is now expected to pass. The robobun CI status comment still reflects the previous commit (d23a99e) with musl build failures and the now-fixed ticket-invalid-arg failures; CI for 780b107 hadn't reported when I reviewed. The author declined the test-file-location nit, which is fine — it was non-blocking.
There was a problem hiding this comment.
All prior feedback has been addressed and I didn't find anything new in this revision, but since this adds native unsafe FFI handling TLS session-ticket key material it's worth a human pass before merging.
Extended reasoning...
Overview
This PR implements tls.Server.getTicketKeys() / setTicketKeys(), replacing "Not implented in Bun yet" stubs. It touches four files: two FFI declarations in boringssl_sys/boringssl.rs, ~20 lines of JS glue in src/js/node/tls.ts, ~100 lines of new native Rust (with several unsafe blocks) in src/runtime/socket/Listener.rs, and a new 7-case test file.
Security risks
TLS session-ticket keys are cryptographic secrets used to encrypt resumption tickets; the implementation passes raw 48-byte buffers between JS and BoringSSL via unsafe FFI, with pointer casts and lifetime reasoning around SSL_CTX* (including a Windows named-pipe branch). A mistake here could leak key material, read/write past buffer bounds, or operate on a freed context. The validation looks correct (48-byte length checked on both JS and Rust sides; ssl_ctx() handles the not-listening case), but this is exactly the kind of code that benefits from a second pair of eyes.
Level of scrutiny
Medium-high. This is production TLS-server code, not sandbox/experimental. It's a first-time implementation of a Node.js-compat surface rather than a refactor, and it crosses the JS↔Rust↔BoringSSL boundary with unsafe. The change is moderately sized but conceptually focused.
Other factors
The PR has gone through many review iterations (wrong error macro, Windows named-pipe SSL_CTX path, banned-API usage, Zig→Rust field rename after refactor, Node-compat error text) — all of which the author fixed promptly and are now resolved. The current revision passed the bug-hunting system cleanly. Test coverage is good (Buffer/Uint8Array/DataView inputs, length/type validation, pre-listen behavior, round-trip persistence). CI build #63141 on 5bbf4bf is in progress. Given the security-sensitive surface and the amount of churn it took to get here, deferring to a human reviewer rather than auto-approving.
|
Diff is green on all lanes it touches. The red lanes across the last few builds are unrelated flakes, none of them in TLS/ticket-key code:
Scraped build annotations show zero failures mentioning tls / ticket / Listener / boringssl. The new regression test ( |
5bbf4bf to
73cf25f
Compare
|
Rebased onto main and force-pushed. The only conflict was trivial: main renamed the binding helper from |
|
Good follow-up note. The Separately, pushed b57b2aa: the |
robobun
changed the title
|
Pushed A report surfaced that with only the stubs fixed, the common deployment is still broken: the I earlier called the option "better as its own change", but since it is ~45 lines of JS on top of the exact native methods this PR already adds, and a second PR would have to rewrite the same two prototype methods, folding it in here is less total review than two PRs. The Rust in this PR is unchanged by the new commit; the delta is What the new commit adds:
Verified locally: all 13 cases across both test files pass under |
There was a problem hiding this comment.
The expanded scope (wiring the ticketKeys constructor option through kRealListen, pre-listen _ticketKeys buffering with a randomBytes(48) default, and the new end-to-end resumption suite) looks correct and the bug hunt came back clean — but this is TLS session-ticket key material plumbed through new unsafe BoringSSL FFI, so deferring to the maintainer pass you already flagged.
Extended reasoning...
Overview
Since my last review (2026-06-27) the PR grew substantially: commit 3e7b850 addressed the pre-existing follow-up I'd noted by (a) stashing options.ticketKeys / setTicketKeys() input on this._ticketKeys before listen, (b) applying it in net.ts kRealListen immediately after Bun.listen() builds the SSL_CTX, (c) making getTicketKeys() return the stashed keys (or a lazily-generated randomBytes(48)) pre-listen instead of throwing, and (d) adding a 195-line end-to-end suite in test/js/node/tls/node-tls-ticket-keys.test.ts that verifies cross-server session resumption with shared keys actually works on the wire. The native side (Listener.rs get_ticket_keys/set_ticket_keys/ssl_ctx(), boringssl.rs externs) is unchanged from what I last looked at.
Security risks
Session ticket keys are the symmetric secret that protects TLS resumption state — getting the plumbing wrong (e.g. silently dropping user-supplied keys, or leaking/reusing default keys across contexts) has direct forward-secrecy implications. The implementation looks right: the JS layer copies caller buffers (Buffer.from(new Uint8Array(...))) so post-call mutation can't change what's applied; the native side enforces the 48-byte length and reads/writes through a live SSL_CTX*; and the e2e tests assert both that shared keys enable resumption and that distinct keys prevent it. Still, this is exactly the kind of crypto-adjacent surface where a maintainer should sign off.
Level of scrutiny
High. New unsafe Rust FFI into BoringSSL (SSL_CTX_get/set_tlsext_ticket_keys), changes to node:tls server lifecycle, and a hook added inside net.ts kRealListen. The author explicitly requested a maintainer pass on the native FFI back on 2026-06-17, and the scope has only grown since.
Other factors
All prior review threads (mine and CodeRabbit's) are resolved; the bug-hunting system found nothing on the current diff. The robobun status comment still shows ❌ on build #65459 for b57b2aa — the author previously characterized the red lanes as unrelated flakes, but the latest commit (3e7b850) doesn't yet have a reported green build, so I'm not approving on CI grounds either.
|
Verified 3e7b850 (the
|
|
Heads up on build #65866 (3e7b850): the ~20 red |
…tTicketKeys Fixes #14604. tls.Server.getTicketKeys() and tls.Server.setTicketKeys() were stubs that threw 'Not implented in Bun yet', and the ticketKeys constructor option was validated and then discarded. Two servers created with the same ticketKeys (the standard cluster / multi-instance TLS deployment) could never resume each other's sessions, so every cross-instance reconnect was a full handshake. Native side (Listener.rs, boringssl_sys): jsGetTicketKeys / jsSetTicketKeys host functions backed by SSL_CTX_get/set_tlsext_ticket_keys on the listener's secure_ctx (or the Windows named-pipe context's ctx). JS side (tls.ts, net.ts): the key material is stored on the Server as _ticketKeys. setTicketKeys() copies the caller's bytes there and applies them to the live SSL_CTX when one exists; the ticketKeys option routes through the same method. getTicketKeys() reads from the live SSL_CTX when listening; before listen() it returns the stored bytes, generating them lazily on first access (Node's constructor-time SSL_CTX has the same effect). kRealListen applies the stored bytes to the freshly built SSL_CTX before the first accept.
3e7b850 to
485d7b6
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto current main (df6c7ee) as a single squashed commit. Main had drifted since June: Verified locally on
Fail-before on released bun: 9 fail, 4 pass. |
- Make jsGetTicketKeys/jsSetTicketKeys accept a native SecureContext in
addition to a Listener, and apply _ticketKeys in buildSharedCreds so
server.emit('connection', sock) without listen() honors the keys.
- Opt buildSharedCreds out of the digest-interned SSL_CTX cache when
_ticketKeys is set so per-server key material doesn't leak into other
consumers of the same cert.
- setTicketKeys length check now throws ERR_INTERNAL_ASSERTION (Error),
matching Node's internal/assert path.
- Pin the SSL_CTX's default ticket keys on first getTicketKeys() after
listen() so BoringSSL's 48h auto-rotation doesn't change what future
calls return (Node/OpenSSL fixes the bytes at ctx creation).
- Fold the 14604 regression tests into node-tls-ticket-keys.test.ts
(not a regression: the API was never implemented) and add a test for
the emit('connection') path.
c00d5e6 to
c2f0e58
Compare
|
Self-review addressed in 667c0b7..cbb8737:
|
The post-listen getTicketKeys pin was the one _ticketKeys undefined->defined transition that left a stale cached _sharedCreds, so injected sockets kept handshaking under the old interned SSL_CTX while getTicketKeys reported the pinned key. Also read sc.context into a local in buildSharedCreds to satisfy no-duplicate-conditional-property-access.
There was a problem hiding this comment.
06f5cdb addresses both remaining nits — the post-listen pin branch now invalidates _sharedCreds like the other two sites, and the Verification block matches the single 9-test file. No new findings this pass; deferring only because the native side (new unsafe BoringSSL FFI in Listener.rs + the SSL_CTX cache opt-out when ticketKeys is set) is TLS-adjacent and worth a maintainer look.
What was reviewed: the three _ticketKeys undefined→defined transitions now all drop _sharedCreds consistently; buildSharedCreds opts out of the digest-interned cache so per-server ticket keys can't leak onto a shared SSL_CTX; the 48-byte length is enforced in both JS and Rust before the FFI call; SSL_CTX_set_tlsext_ticket_keys copies its input so the borrowed byte_slice() lifetime is sound.
Extended reasoning...
Overview
Implements tls.Server ticketKeys option + getTicketKeys()/setTicketKeys(). Five files: two BoringSSL FFI declarations in boringssl_sys/boringssl.rs; ~110 lines of new Rust in Listener.rs (two #[host_fn]s + helpers, four unsafe blocks calling SSL_CTX_get/set_tlsext_ticket_keys); ~60 lines in tls.ts replacing the two stub methods and threading _ticketKeys through buildSharedCreds/setSecureContext; a 6-line hook in net.ts kRealListen; and a 278-line test file with 9 tests including two end-to-end cross-server resumption checks.
Security risks
Session ticket keys are security-relevant (a leaked key lets an attacker decrypt captured TLS 1.2 sessions), but this PR wires an existing OpenSSL/BoringSSL API through to JS rather than changing the security model. The one meaningful risk — writing per-server ticket keys onto a digest-interned SSL_CTX shared with unrelated consumers — is handled: buildSharedCreds passes cached = (ticketKeys === undefined) so a keyed context is createPrivate, not intern. The 48-byte length is validated on the JS side before any native call and again in Rust before the FFI. SSL_CTX_set_tlsext_ticket_keys memcpy's its input, so passing a borrowed byte_slice() view is sound. No new user-controlled data reaches an allocation size or index.
Level of scrutiny
Medium-high. It's a Node-compat feature fill, not a design change, and it's been through ~5 review rounds with substantive fixes each time (the injected-emit('connection') path, error-code parity, cache opt-out, _sharedCreds invalidation). But it adds unsafe FFI into BoringSSL and touches the per-VM SSL_CTX cache policy, both of which the repo's REVIEW.md flags as areas where a maintainer should sign off.
Other factors
All prior findings from my earlier passes are addressed in the current diff; the bug-hunting system found nothing this run. Test coverage is solid: cross-server resumption over both the listen() and emit('connection') paths, negative resumption with mismatched keys, Buffer/Uint8Array/DataView round-trips, copy-semantics for both get and set, and error-code assertions matching Node's ERR_INVALID_ARG_TYPE/ERR_INTERNAL_ASSERTION. The Windows named-pipe ssl_ctx() branch is untested (no Windows-specific test), which is worth noting but consistent with the surrounding code.
…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 branch has not been deployed
2 participants
Fixes #14604
Problem
Three related gaps in
tls.Server:ticketKeysserver option was validated (type, 48 bytes) and then discarded. Two servers created with the sameticketKeys, the standardcluster/ multi-instance TLS deployment, could never resume each other's sessions, so every cross-instance reconnect was a full handshake.server.getTicketKeys()andserver.setTicketKeys()were stubs that threwNot implented in Bun yet.typeof server.getTicketKeys === "function") reported the API as supported.Node v26.3.0 for reference: the option is applied to the
SSL_CTXand both methods work before and afterlisten(), and on servers that neverlisten()but receive sockets viaserver.emit('connection', sock)(the pattern in Node's owntest-tls-ticket.js).Cause
validateSecureContextOptionscheckedoptions.ticketKeysbut nothing consumed it, and the prototype methods were never wired to BoringSSL'sSSL_CTX_get_tlsext_ticket_keys/SSL_CTX_set_tlsext_ticket_keys(same 48-bytename || hmac || aeslayout as OpenSSL).Fix
Native side (
Listener.rs,boringssl_sys):jsGetTicketKeys/jsSetTicketKeyshost functions backed bySSL_CTX_get/set_tlsext_ticket_keys. They accept either aListener(thelisten()path'ssecure_ctx, or the Windows named-pipe ctx) or a nativeSecureContext(theemit('connection')path's_sharedCreds.context).JS side (
tls.ts,net.ts):Serveras_ticketKeys.setTicketKeys()copies the caller's bytes there and applies them to the liveSSL_CTXwhen one exists; theticketKeysoption routes through the same method.getTicketKeys()reads from the liveSSL_CTXwhen listening; beforelisten()it returns the stored bytes, generating them lazily on first access. Afterlisten(), the first read pins the bytes on theSSL_CTXso BoringSSL's default-key auto-rotation does not change what later calls return (Node/OpenSSL fixes them at ctx creation).kRealListenapplies the stored bytes to the freshly builtSSL_CTXbefore the first accept.buildSharedCredsapplies them to the lazily builtSecureContextfor injected connections, opting out of the digest-internedSSL_CTXcache for that ctx so per-server key material does not leak into unrelated consumers.setTicketKeysdrops_sharedCredsso the next injected socket rebuilds with the new keys.setTicketKeysvalidation matches Node:ERR_INVALID_ARG_TYPEfor a non-buffer,ERR_INTERNAL_ASSERTIONfor the wrong length (Node usesinternal/assertthere).Verification
The new suite includes the cross-instance resumption repro (two
listen()servers sharingticketKeys, client captures a ticket from A and resumes on B withisSessionReused() === true), theemit('connection')variant of the same, and round-trips throughBuffer/Uint8Array/DataView.Also passing under
bun bd:node-tls-context.test.ts,node-tls-connect.test.ts,ssl-ctx-cache.test.ts, and upstreamtest-tls-basic-validations.js,test-tls-ticket-invalid-arg.js.[review] gate passed · iteration 18 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 18
evidence per changed file
root cause · written by the author bot
Bun's tls.Server shipped getTicketKeys and setTicketKeys as unimplemented stubs that threw, so session ticket key management and cross-server session resumption did not work as in Node.js. The fix implements both methods natively by exposing SSL_CTX_get_tlsext_ticket_keys and SSL_CTX_set_tlsext_ticket_keys through the Listener, with 48 byte length and type validation enforced on both the JavaScript and native sides, and adds a ticketKeys constructor option that is buffered pre listen and applied once a handle exists. Because Bun interns SSL_CTX objects by configuration digest, servers with …