fetch: cache client TLS sessions for resumption - #36598
Conversation
Every fetch() that opens a fresh TLS connection currently pays a full handshake (2 RTTs plus certificate chain verification), because openssl.c's SSL_SESS_CACHE_NO_INTERNAL mode routes resumable sessions only to the new-session callback, and that callback returns 0 for every SSL that is not a Bun.connect / node:tls socket. The 64-socket keep-alive pool hides this for warm connections, but a fan-out across more than 64 origins, a parallel cold burst, or any idle gap beyond the pool's 5-minute eviction falls through to a full handshake each time. node:https.Agent already has a JS-side session cache; this brings the same to fetch. us_ssl_new_session_cb gains a per-SSL session sink: an ex_data slot holding an owner pointer plus an on_new_session callback, checked before the existing is-socket gate. When present the callback receives each session with one SSL_SESSION_up_ref (no i2d serialize, no pending queue); the slot's free callback releases the owner on SSL_free. Each HTTPContext<true> (one per interned SSLConfig) carries a 32-entry LRU keyed on (connect hostname, port, proxy_auth_hash), the same tuple the keep-alive pool uses for direct TLS, so a cached session never crosses an SNI / Host-header-override boundary the pool wouldn't. on_open offers any cached session via SSL_set_session alongside the SNI / ALPN setup and installs an unarmed sink; on_handshake arms it only after checkServerIdentity passes. Tickets delivered before that (TLS 1.2 fires inside SSL_do_handshake) are parked on the sink and flushed once armed, so an unverified handshake never seeds the cache. Connections with rejectUnauthorized:false, a JS checkServerIdentity callback, or a unix-socket path skip the cache entirely. BoringSSL's session_dup copies x509_peer / x509_chain into the resumed session, so checkServerIdentity still re-verifies the hostname on a resumed connection. BUN_FEATURE_FLAG_DISABLE_FETCH_TLS_SESSION_CACHE disables the cache. The test drives two fetches against a node:tls server that answers with Connection: close; the server observes isSessionReused()=[false,true] with the cache and [false,false] with the feature flag set.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. 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 (11)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Updated 4:05 AM PT - Aug 1st, 2026
❌ @Jarred-Sumner, your commit bed49a9 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36598That installs a local version of the PR into your bun-36598 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Cover both TLS 1.2 and TLS 1.3 in the test fixture by pinning the server's
min/max version. TLS 1.2 delivers the session inside SSL_do_handshake (before
on_handshake can verify the peer), TLS 1.3 as a post-handshake
NewSessionTicket; the two hit the pending/arm path and the direct-insert path
respectively. Add a hostname-mismatch variant: a trusted chain whose SAN does
not cover the TLS servername must fail ERR_TLS_CERT_ALTNAME_INVALID on every
attempt and the server must never observe a resumed handshake, proving the
sink is never armed for a rejected identity.
Fix the exit leak ASAN caught on the install lane: the static https_context
never drops its keep-alive pool and the HTTP thread's TLS root is not scanned
by LSan, so a SessionSink parked on a pooled socket's SSL survives to exit.
drain_for_exit() now clears the session cache and walks every socket group
(default and per-SSLConfig custom contexts) to free the sink via the new
us_socket_group_clear_session_sinks.
Trim the flagged comment blocks and move the eligibility check into
session_cache::eligible(). Use bun_core::heap::{into_raw, destroy} for the
Box round-trip to match the rest of src/http.
There was a problem hiding this comment.
Thanks for addressing the exit-time sink leak (via drain_for_exit + us_socket_group_clear_session_sinks) and switching to bun_core::heap::{into_raw,destroy}. No new issues found this pass, but this is security-sensitive TLS session-resumption code with cross-FFI lifetime management, so it should get a human sign-off.
What was reviewed:
- The arm-after-
checkServerIdentitygating andeligible()exclusions (rejectUnauthorized:false, JS checkServerIdentity, unix socket) — a failed identity check reachesclose_and_failwithoutarm(), so the parked TLS 1.2 ticket is dropped inSessionSink::drop. - Cache keying vs the keep-alive pool tuple, including the
proxy_auth_hash/ Host-override branch inon_open. SSL_SESSIONrefcount balance acrosstake/insert/install/sink callbacks and the LRU eviction path.us_socket_group_clear_session_sinkswalkshead_sockets, covering both pooled and in-flight SSLs at exit.
Extended reasoning...
Overview
This PR adds a client-side TLS session cache to fetch() so a second cold connection to the same origin can resume at 1 RTT. It touches: packages/bun-usockets/src/crypto/openssl.c (new per-SSL ex_data "session sink" slot with a free callback, plus a group-wide sink-clearing helper), src/http/session_cache.rs (new 32-entry LRU + SessionSink FFI owner + install/arm/drain_for_exit), src/http/lib.rs (on_open installs the sink pre-handshake and offers a cached session via SSL_set_session), src/http/HTTPContext.rs (on_handshake arms the sink only after checkServerIdentity passes), src/http/HTTPThread.rs (exit-time drain of sinks and cache for the default and every custom SSL context), a new feature-flag env var, and new tests covering TLS 1.2/1.3 resumption, the disable flag, and the wrong-SAN negative case.
Changes since prior review
Commit 0613d16 addressed both prior findings: the exit-time Box<SessionSink> leak is now handled by drain_for_exit → us_socket_group_clear_session_sinks (walks group->head_sockets, so covers both pooled keep-alive and in-flight SSLs), and the raw Box::{into,from}_raw calls were replaced with bun_core::heap::{into_raw,destroy} per repo convention. The bug-hunting system found no new issues on this revision.
Security risks
This is squarely security-sensitive. A resumed TLS session skips the Certificate message, so caching a session from an unverified handshake would launder a bad peer into a later strict caller. The design guards against this: the sink is installed unarmed in on_open, and arm() is called only after on_handshake observes handshake_success && reject_unauthorized && !did_have_handshaking_error && check_server_identity(...) == true. Connections with rejectUnauthorized: false, a JS checkServerIdentity callback (signals::CertErrors), or a unix-socket path are excluded via eligible() and never install a sink. The cache key is (hostname, port, proxy_auth_hash) scoped to one HTTPContext<true> per interned SSLConfig, matching the keep-alive pool's isolation boundary. The PR description asserts BoringSSL's ssl_crypto_x509_session_dup copies the peer chain into the resumed session so check_server_identity still sees the leaf on resume — that claim, and whether proxy_auth_hash fully covers the Host-header SNI override for the direct-TLS-via-HTTP-proxy case (the want_tunnel branch passes the hash but the tunnel's inner TLS is a separate SSL*), are the pieces most worth a human reviewer's eye.
Level of scrutiny
High. This is new cross-FFI memory management (Rust Box → C ex_data slot → BoringSSL free callback → Rust drop), on the TLS handshake path, with a security invariant ("never cache before identity is verified") whose failure mode is silent. REVIEW.md's "Security checks fail closed and cover every path to the protected effect" and "Key pools/caches on every parameter that influenced establishment" both apply directly.
Other factors
Test coverage is good for what it covers (TLS 1.2 and 1.3 resumption, feature-flag disable, wrong-SAN doesn't seed the cache), but there's no test for the proxy-tunnel path or for cross-SSLConfig isolation. The comment-cop bot has several unresolved "paragraph-long comment" flags on session_cache.rs — those are style-only and shouldn't block, but the author may want to trim before merge. Given the domain, deferring to a human is the right call regardless of the clean bug-hunt result.
|
Add extensive tests covering security-related edgecases |
…x_data The BoringSSL build drops BORINGSSL_REQUIRE_MEMORY_HOOKS under ASAN so Mach-O/COFF fall back to libc, but on ELF the weak hook symbols still resolve to the Rust definitions, and OPENSSL_memory_alloc hard-coded mi_malloc. Every OPENSSL_malloc allocation (an SSL's ex_data array, sessions, certs) therefore lived in a mimalloc page LeakSanitizer does not scan, so any libc-backed allocation reachable only through one (the per-SSL session sink and its owner Box) was reported as a direct leak when bun install exited via Global::exit. Route the three hooks through bun_alloc::default_alloc (mimalloc, libc under cfg(bun_asan)) so release behaviour is unchanged and ASAN builds see the full chain from the static https_context through the SSL to its ex_data. Drop the drain_for_exit / us_socket_group_clear_session_sinks shutdown walk added in 0613d16; it only ran on the VirtualMachine shutdown path and was the wrong layer for an LSAN-reachability problem. Expand the test fixture to cover the remaining security edge cases in one subprocess per TLS version: default resume, hostname-mismatch rejection, JS checkServerIdentity exclusion, port isolation, and connect-hostname isolation.
|
Security edge-case coverage added in 3e0579a (both TLS 1.2 and TLS 1.3):
The exit-time sink report was LSan reachability, not a real leak: on ELF the weak |
There was a problem hiding this comment.
No bugs found, but this adds security-sensitive TLS session resumption to fetch() and changes the process-wide OPENSSL_memory_alloc hooks — worth a maintainer look, especially since Jarred is already engaged.
What was reviewed:
- Session-sink lifecycle:
install→ ex_data slot →sink_on_freeonSSL_free;pendingticket freed on Drop when handshake fails beforearm(). - Cache-key isolation matches the keep-alive pool tuple;
eligible()excludesrejectUnauthorized: false, JScheckServerIdentity, and unix sockets so unverified sessions never seed the cache. OPENSSL_memory_allocrerouted frommimalloctodefault_alloc— release behavior unchanged (still mimalloc), ASAN builds now use libc so LSan can scan ex_data.
Extended reasoning...
Overview
This PR adds a per-HTTPContext<true> 32-entry LRU cache of SSL_SESSION references keyed on (hostname, port, proxy_auth_hash), wired into fetch's TLS connect path via a new per-SSL ex_data "session sink" in bun-usockets/src/crypto/openssl.c. The sink is installed pre-handshake in HTTPClient::on_open and armed only after checkServerIdentity passes in Handler::on_handshake, so TLS 1.2 tickets (delivered inside SSL_do_handshake) are parked until verification and TLS 1.3 tickets (post-handshake) insert directly. It also reroutes the process-wide OPENSSL_memory_alloc/free/get_size hooks in src/boringssl/lib.rs from hard-coded mimalloc to bun_alloc::default_alloc (mimalloc in release, libc under cfg(bun_asan)), and makes default_alloc::usable_size public/uncfg'd to support that.
Security risks
TLS session resumption is security-sensitive: a resumed session skips the Certificate message and restores the stored verify_result, so caching an unverified or wrong-hostname session would launder it into a later strict caller. The design guards against this by (a) scoping the cache per-interned-SSLConfig HTTPContext, (b) keying on the same tuple the keep-alive pool uses, (c) excluding rejectUnauthorized: false / JS checkServerIdentity / unix-socket paths in eligible(), and (d) only arming the sink after native checkServerIdentity succeeds. The tests cover mismatch/port/hostname isolation and the JS-callback bypass for both TLS 1.2 and 1.3. I did not find a path where an unverified session enters the cache, but the interaction between proxy_auth_hash computation in on_open (which branches on want_tunnel || http_proxy.is_none()) and the pool-key logic is subtle enough to merit maintainer eyes.
Level of scrutiny
High. This is a new caching layer in the TLS handshake path with FFI lifecycle management (Box'd Rust struct owned by a BoringSSL ex_data slot, freed via C callback), plus a change to process-wide allocator hooks that affect every BoringSSL allocation. The earlier iteration had an LSan-reported exit leak whose fix evolved from an explicit drain_for_exit sweep to the allocator-hook reroute — a design pivot worth confirming.
Other factors
Jarred-Sumner has already requested (and received) additional security-edge-case tests, so a maintainer is actively engaged. My prior inline findings (heap-helper convention, exit-time leak) are resolved. The default_alloc::usable_size visibility/cfg change and the OPENSSL_memory_* hook change are outside the session-cache feature proper and affect all BoringSSL usage — that broadening deserves explicit sign-off.
|
CI on 3e0579a: the session-sink leak is gone. One new [new] failure remains, and it's a pre-existing leak the allocator fix exposed rather than caused:
Everything else in build 86653 is flaky (passed on retry / passed alone) or the alpine agent-creation infra failure. Handed the crypto leak off as a separate task since it's in a different subsystem and unrelated to session resumption. Happy to fold a fix into this PR instead if that's preferable. |
There was a problem hiding this comment.
I didn't find any bugs, but this needs a human sign-off: it adds a TLS session cache on a security-sensitive path (session resumption gated on checkServerIdentity), and the OPENSSL_memory_alloc rerouting through default_alloc affects every BoringSSL allocation process-wide, not just this feature. Jarred asked for extensive security edge-case tests — the added coverage looks reasonable to me but that's his call, and there's one new CI failure (crypto generateKeyPair leak exposed by the allocator change) being handed off to a separate task.
What was reviewed:
- Session sink lifecycle:
install→ ex_data slot →SSL_free→sink_on_free→heap::destroy; theus_mallocOOM path callson_free(owner)so the Rust box isn't leaked. arm()gating: only reached after nativecheckServerIdentitypasses underreject_unauthorized=true;eligible()excludes lax verification, JS callback, and unix-socket paths.- Cache key matches the keep-alive pool tuple;
take/insertboth bound byMAX_KEEPALIVE_HOSTNAMEand free the transferredSSL_SESSIONref on the reject path. default_alloc::usable_sizevisibility widening — checked that it's still null-safe and the ASAN/release split is preserved.
Extended reasoning...
Overview
Adds a 32-entry per-HTTPContext<true> LRU of SSL_SESSION refs so fetch() can resume TLS sessions on fresh connects. New C-side per-SSL ex_data slot in openssl.c holding a session-sink callback pair; new src/http/session_cache.rs (262 lines) with the cache, sink, and install/arm entry points; wiring in HTTPClient::on_open and Handler::on_handshake; a feature-flag kill switch; and a rerouting of the OPENSSL_memory_* weak-symbol hooks from hard-coded mimalloc to bun_alloc::default_alloc so LSan can trace through BoringSSL's ex_data array under ASAN. ~600 lines total including a subprocess fixture and 4 new tests × 2 TLS versions.
Security risks
This is squarely security-sensitive. Session resumption skips the Certificate message and restores the stored verify_result, so caching an unverified session would let a later strict caller inherit a laundered verdict. The PR guards this by (a) installing the sink unarmed, (b) only calling arm() after the native checkServerIdentity passes inside the reject_unauthorized=true branch, and (c) excluding rejectUnauthorized:false, JS checkServerIdentity, and unix-socket connects via eligible(). The cache key is the same (hostname, port, proxy_auth_hash) tuple the keep-alive pool uses and is scoped per interned SSLConfig, so it shouldn't cross an SNI/CA boundary the pool wouldn't. I traced these paths and the gating looks correct, but this is exactly the class of change REVIEW.md flags for maintainer review ("Never remove a flag you don't understand in a TLS/crypto path", "security flags on pooled sessions are monotonic").
Level of scrutiny
High. Beyond the TLS gating itself, the OPENSSL_memory_alloc change is a process-wide behavior change to how every BoringSSL allocation is serviced (mimalloc directly → default_alloc, which is mimalloc in release / libc under ASAN). The PR description says release behavior is identical, and reading bun_alloc::default_alloc that appears true, but it's a rider on a feature PR that a maintainer should explicitly accept. It also exposed one new CI leak (crypto generateKeyPair + process.exit inside callback) that the author is deferring.
Other factors
Jarred explicitly requested extensive security edge-case tests; the author added mismatch/JS-callback/port-isolation/host-isolation/kill-switch coverage for both TLS 1.2 and 1.3. Whether that's sufficient is a maintainer judgment. My two prior inline findings (heap-helper convention, exit-time LSan report) were both addressed. The drain_for_exit approach mentioned in a resolved thread appears to have been superseded by the allocator fix — the diff no longer contains it, which is fine (the allocator fix makes the sinks reachable from the static root so LSan doesn't report them).
|
Heads up: the OPENSSL memory hook change here made a pre-existing generateKeyPair leak visible to LeakSanitizer, so test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts (crypto-generateKeyPair subtest) now fails on Linux x64-asan runs, e.g. build 89031. The fix for the underlying leak is #36657, which needs a small rebase since half of its diff landed here. |
## What
Give `fetch()` a client-side TLS session cache so a second cold
connection to an origin resumes at 1 RTT instead of running a full
handshake with certificate chain verification.
## Why
`us_ssl_ctx_from_options` sets `SSL_SESS_CACHE_NO_INTERNAL`, so
BoringSSL never stores or looks up client sessions itself; the
application must. `us_ssl_new_session_cb` only parks sessions for
`Bun.connect` / `node:tls` sockets (the `us_ssl_is_socket_ex_idx` gate)
and returns 0 for everything else. Nothing under `src/http/` ever calls
`SSL_set_session`, so every fetch that misses the 64-socket keep-alive
pool (more than 64 origins, a parallel cold burst, any idle gap past the
5-minute eviction) pays the full 2-RTT handshake plus chain walk.
`node:https.Agent` already has a JS-side session cache
(`src/js/node/https.ts`); this is specifically `fetch`.
Deno gets this by default: one `Arc<rustls::ClientConfig>` per client
means rustls' `Resumption::in_memory_sessions(256)` applies.
## How
**`packages/bun-usockets/src/crypto/openssl.c`**: add a per-SSL
session-sink ex_data slot (`us_ssl_session_sink_idx`) holding `{owner,
on_new_session, on_free}`. `us_ssl_new_session_cb` checks it before the
existing is-socket gate and, when present, calls the owner's callback
with one `SSL_SESSION_up_ref`'d session (no `i2d_SSL_SESSION`, no
pending queue). The slot's ex_data free callback calls `on_free(owner)`
then `us_free`s the small wrapper. `us_ssl_set_session_sink` /
`us_ssl_get_session_sink_owner` are exported in `libusockets.h`.
**`src/http/session_cache.rs`** (new): a 32-entry LRU living on
`HTTPContext<true>` (one per interned `SSLConfig`), keyed on the same
`(connect hostname, port, proxy_auth_hash)` tuple the keep-alive pool
uses for direct TLS so a cached session never crosses an SNI /
Host-override boundary the pool wouldn't. Entries own one `SSL_SESSION`
reference; `Drop` frees it.
**`src/http/lib.rs` `HTTPClient::on_open`**: alongside SNI/ALPN setup
(pre-handshake, `SSL_is_init_finished == 0`), look up the cache and call
`SSL_set_session` on a hit, then install an unarmed sink carrying the
cache key.
**`src/http/HTTPContext.rs` `Handler::on_handshake`**: arm the sink only
after `checkServerIdentity` passes. TLS 1.2 fires the new-session
callback inside `SSL_do_handshake` (before `on_handshake`), so those
tickets are parked on the sink and flushed once armed; TLS 1.3 tickets
arrive post-handshake and insert directly. A handshake that fails
verification never seeds the cache: `close_and_fail` closes the socket,
`SSL_free` drops the sink, and the parked ticket is released.
Connections with `rejectUnauthorized: false`, a JS `checkServerIdentity`
callback, or a unix-socket path skip the cache entirely.
Resumed connections still re-verify the hostname: BoringSSL's
`ssl_crypto_x509_session_dup` copies `x509_peer` / `x509_chain` into the
resumed session, so `SSL_get_peer_cert_chain` in `check_server_identity`
sees the stored leaf.
`BUN_FEATURE_FLAG_DISABLE_FETCH_TLS_SESSION_CACHE=1` disables it.
## Testing
`test/js/web/fetch/fetch.tls.test.ts` gains a `client-side TLS session
resumption` describe: a fixture runs a `node:tls` server that answers
with `Connection: close` and records `socket.isSessionReused()` per
connection, then two sequential fetches with a trusted `ca`.
```
# before (system bun)
[false, false]
# after
[false, true]
# after, with BUN_FEATURE_FLAG_DISABLE_FETCH_TLS_SESSION_CACHE=1
[false, false]
```
Related: oven-sh#25185 (this covers the `fetch()` client side).
<!-- robobun:evidence:begin -->
---
**[review]** gate passed · iteration 1 · 11 files touched
<details><summary>fails on main (without fix)</summary>
```console
ASAN without fix: 3 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/web/fetch/fetch.tls.test.ts
bun test v1.4.0 (bed49a9)
test/js/web/fetch/fetch.tls.test.ts:
(pass) fetch-tls > fetch with valid tls and non-native checkServerIdentity that throws should reject [1233.53ms]
(pass) fetch-tls > fetch with rejectUnauthorized: false should not call checkServerIdentity [94.69ms]
(pass) fetch-tls > fetch with valid tls should not throw [1942.62ms]
(pass) fetch-tls > re-derives the Host header and TLS verification hostname from the redirect target on a cross-origin redirect [2208.06ms]
(pass) fetch-tls > can handle multiple requests with non native checkServerIdentity [2057.20ms]
(pass) fetch-tls > fetch with valid tls and non-native checkServerIdentity should work [2007.75ms]
237 | expect({
238 | default: r.default,
239 | checkServerIdentity: r.checkServerIdentity,
240 | portIsolation: r.portIsolation,
241 | hostIsolation: r.hostIsolation,
242 | }).toEqual({
^
error: expect(received).toEqual(expected)
{
"che
... (truncated)
release without fix: 5 FAILED
bun test v1.4.0-canary.1 (1498d7b)
test/js/web/fetch/fetch.tls.test.ts:
(pass) fetch-tls > fetch with checkServerIdentity failing should throw [40.70ms]
(pass) fetch-tls > fetch with self-sign tls should throw [75.35ms]
(pass) fetch-tls > fetch with invalid tls should throw [53.71ms]
(pass) fetch-tls > fetch with self-sign certificate tls + rejectUnauthorized: false should not throw [54.81ms]
(pass) fetch-tls > fetch with invalid tls + rejectUnauthorized: false should not throw [56.72ms]
237 | expect({
238 | default: r.default,
239 | checkServerIdentity: r.checkServerIdentity,
240 | portIsolation: r.portIsolation,
241 | hostIsolation: r.hostIsolation,
242 | }).toEqual({
^
error: expect(received).toEqual(expected)
{
"checkServerIdentity": [
false,
false,
],
"default": [
false,
- true,
+ false,
],
"hostIsolation": [
false,
false,
],
"portIsolation": {
"a": [
false,
],
"b": [
false,
],
},
}
- Expected - 1
+ Received + 1
at <anonymous> (/workspace/bun/test/js
... (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/mechgate.xml" test/js/web/fetch/fetch.tls.test.ts
bun test v1.4.0 (bed49a9)
test/js/web/fetch/fetch.tls.test.ts:
(pass) fetch-tls > fetch with valid tls and non-native checkServerIdentity that throws should reject [1187.76ms]
(pass) fetch-tls > fetch with valid tls should not throw [1842.09ms]
(pass) fetch-tls > can handle multiple requests with non native checkServerIdentity [1927.28ms]
(pass) fetch-tls > fetch with rejectUnauthorized: false should not call checkServerIdentity [198.10ms]
(pass) fetch-tls > re-derives the Host header and TLS verification hostname from the redirect target on a cross-origin redirect [2127.65ms]
(pass) fetch-tls > fetch with valid tls and non-native checkServerIdentity should work [1983.12ms]
(pass) fetch-tls > client-side TLS session resumption > caches only verified sessions keyed on (host, port) (TLSv1.2) [3721.64ms]
(pass) fetch-tls > client-side TLS session resumption > is disabled by BUN_FEATURE_FLAG_DISABLE_FETCH_TLS_SESSION_CACHE (TLSv1.3) [3815.08ms]
(pass) fetch-tls > client-side TLS session resumption > i
... (truncated)
release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
target linux-x64-gnu
build type Release
build dir ./build/release
revision bed49a9
features baseline
22 deps, 108 codegen, 1171 objects in 3780ms
ninja: Entering directory `/workspace/bun/build/release'
[1/1234] gen ProcessBindingConstants.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingConstants.cpp
[2/1234] gen JSBuffer.lut.h
Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp
[3/1234] gen bindgenv2
[4/1234] gen ProcessBindingBuffer.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingBuffer.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingBuffer.cpp
[5/1234] gen ProcessBindingFs.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingFs.lut.h from /workspace/bun/src/jsc/bindings/ProcessBindingFs.cpp
[6/1234] gen ProcessBindingHTTPParser.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingHTTPParser.lut.h from /workspace/bun/src/jsc/bindings/ProcessBin
... (truncated)
```
</details>
<details><summary>diff hotspot</summary>
```
packages/bun-usockets/src/crypto/openssl.c | 65 ++++-
packages/bun-usockets/src/libusockets.h | 7 +
src/boringssl/lib.rs | 24 +-
src/bun_alloc/lib.rs | 3 +-
src/bun_core/env_var.rs | 1 +
src/http/HTTPContext.rs | 10 +-
src/http/HTTPThread.rs | 3 +
src/http/lib.rs | 24 ++
src/http/session_cache.rs | 262 +++++++++++++++++++++
.../fetch/fetch.tls.session-resumption-fixture.ts | 134 +++++++++++
test/js/web/fetch/fetch.tls.test.ts | 77 ++++++
11 files changed, 597 insertions(+), 13 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 6 12 0
packages/bun-usockets/src/libusockets.h 3 3 0
src/boringssl/lib.rs 2 1 0
src/bun_alloc/lib.rs 2 1 0
src/bun_core/env_var.rs 2 3 0
src/http/HTTPContext.rs 5 4 0
src/http/HTTPThread.rs 6 5 0
src/http/lib.rs 6 3 0
src/http/session_cache.rs 1 12 0
…st/js/web/fetch/fetch.tls.session-resumption-fixture.ts 1 9 0
test/js/web/fetch/fetch.tls.test.ts 3 9 0
```
</details>
<!-- robobun:evidence:end -->
---------
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>
…llback (#36986) ## What `test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts` (the crypto-generateKeyPair fixture) fails on every Linux x64-asan run since #36598 landed (builds [89023](https://buildkite.com/bun/bun/builds/89023), [89031](https://buildkite.com/bun/bun/builds/89031)): ``` direct leak of 24b in run (src/runtime/node/node_crypto_binding.rs:85:21) +34 more SUMMARY: AddressSanitizer: 1480 byte(s) leaked in 35 allocation(s). #6 EVP_PKEY_keygen vendor/boringssl/crypto/evp/evp_ctx.cc #7 Bun::KeyPairJobCtx::runTask src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp:23 #8 Bun__RsaKeyPairJobCtx__runTask src/jsc/bindings/node/crypto/CryptoGenRsaKeyPair.cpp:26 ``` ## Cause The 11 extern crypto job ctxs (generateKeyPair x5, sign/verify, diffieHellman, hkdf, generatePrime, checkPrime, generateKey) completed by invoking the JS callback from inside C++ `runFromJS` while the ctx was still alive; the ctx was freed only after `then()` returned. A callback that never returns (the fixture calls `process.exit(0)` inside it) stranded everything the ctx still owned: the generated `EVP_PKEY`, `KeyObjectData` refs, `BIGNUM`s. The leak is pre-existing; #36598 made it observable by routing `OPENSSL_malloc` through libc under ASAN. Whether LSan reported the other job types too was codegen luck (their pointers happened to be reachable by the conservative stack scan); `generateKeyPair`'s `EVP_PKEY` sits behind two FastMalloc indirections and was reported deterministically. ## Fix Make it structurally impossible for a job ctx to hold native resources across user JS: the native side never sees the callback. - `runFromJS` keeps its name (the JS-thread half, paired with the work-pool half `runTask`) but no longer receives the callback. It returns `JSCallbackArgs`, a small by-value type whose constructors are the only producers, so bodies read `return { err };` or `return { jsNull(), publicKey, privateKey };`. The extern "C" shims copy it through a typed out-pointer (C linkage cannot return a class type); the Rust side consumes it as a slice. - The Rust `extern_crypto_job!` plumbing does, in order: run `runFromJS` to produce the arguments, free the ctx (`ctx_deinit`), invoke the callback. The invariant lives in one place and applies to every job type. - Shutdown release: a completion task enqueued but not yet dispatched when `process.exit()` runs (exit racing the work pool) used to be re-queued at shutdown, stranding the ctx the same way. `AnyTaskJob` now carries an erased release entry and the shutdown release frees the job without running its completion. A completion posted after the final drain is not recoverable without joining the work pool (which would block exit); `test-crypto-op-during-process-exit.js` stays in `no-validate-leaksan.txt` for that sliver, now with an accurate comment. - The caught-export-exception paths encoded the `JSC::Exception` cell itself, so the callback's err argument was not the thrown Error (not `instanceof Error`, no `code`). They now use `Exception::value()`, matching node: JWK export of an unsupported curve surfaces `ERR_CRYPTO_JWK_UNSUPPORTED_CURVE`. No behavior change otherwise: - `Bun__EventLoop__runCallback{1,2,3}` were Rust's `EventLoop::run_callback` exported to C++. The plumbing now calls `run_callback` directly: same enter/exit bracketing, same pending-exception gate, same unhandled-exception reporting, same synchronous timing. This made `runCallback1`/`runCallback3` dead (the crypto bodies were their last callers), so their exports and declarations are deleted; `runCallback2` stays for the webview backends. - Callback arity is preserved per path (observable via `arguments.length`): error paths pass 1 arg, results 2, generateKeyPair success 3. - Exception paths are preserved: a throw out of argument production skips the callback and reports unhandled, as before. Each `runFromJS` checks its `ThrowScope` after every call that can throw (`RETURN_IF_EXCEPTION`), since the check that used to happen inside the nested `runCallbackN` call now happens after the C++ scope destructs; `BUN_JSC_validateExceptionChecks` verifies this on the asan lane. - The produced `JSValue`s live on the `then()` stack frame between production and invocation, which JSC's conservative scan covers; they are JS-heap values, so freeing the ctx first cannot invalidate them. - Perf: same number of FFI crossings, no allocation added. The Rust-native crypto jobs (pbkdf2, scrypt, random) already had the ordering property: they resolve promises or queue the callback via nextTick, so their ctx drops before user JS runs. The synchronous-callback extern jobs were the gap. ## Verification New tests in `crypto.key-objects.test.ts`: - `isASAN`-gated leak suite: children run with `BUN_DESTRUCT_VM_ON_EXIT=1` and `detect_leaks=1` (the asan lane's configuration) and call `process.exit(0)` from the callback of each job type: generateKeyPair (KeyObject and encrypted PEM outputs), sign, diffieHellman, hkdf, checkPrime, generateKey, plus an exit-before-completion-dispatch case (busy-spin so the queued completion is never dispatched). - An export-error test: `generateKeyPair('ec', { namedCurve: 'secp224r1', ...jwk encodings })` asserts the callback err is `instanceof Error` with code `ERR_CRYPTO_JWK_UNSUPPORTED_CURVE` (matches node; fails on main, which passes the Exception cell). Results: - unfixed build (src stashed): both generateKeyPair leak tests fail with the exact CI signature (`Direct leak of 24 byte(s)` in `EVP_PKEY_keygen` via `KeyPairJobCtx::runTask`) - fixed build: all pass, including under `BUN_JSC_validateExceptionChecks=1`, and ec/ed25519 keypair and verify probes run leak-clean as well - `AsyncLocalStorage-tracking.test.ts`: 74 pass, 0 fail (all async-context crypto fixtures, against both bun and node) - `crypto.test.ts` (369), `crypto.key-objects.test.ts` (117), and 37 node parallel files (`test-crypto-keygen*`, `test-crypto-sign-verify`, `test-crypto-hkdf`, `test-crypto-dh-stateless`, `test-crypto-*prime*`) all pass The break landed with #36598 (which made the leak visible); #36657 proposed clearing individual ctx fields before the callback, and this PR supersedes that approach with the ordering guarantee in the job plumbing instead of per-field resets. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts test/js/node/crypto/crypto.key-objects.test.ts <!-- robobun:evidence:end -->
…llback (oven-sh#36986) ## What `test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts` (the crypto-generateKeyPair fixture) fails on every Linux x64-asan run since oven-sh#36598 landed (builds [89023](https://buildkite.com/bun/bun/builds/89023), [89031](https://buildkite.com/bun/bun/builds/89031)): ``` direct leak of 24b in run (src/runtime/node/node_crypto_binding.rs:85:21) +34 more SUMMARY: AddressSanitizer: 1480 byte(s) leaked in 35 allocation(s). #6 EVP_PKEY_keygen vendor/boringssl/crypto/evp/evp_ctx.cc #7 Bun::KeyPairJobCtx::runTask src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp:23 #8 Bun__RsaKeyPairJobCtx__runTask src/jsc/bindings/node/crypto/CryptoGenRsaKeyPair.cpp:26 ``` ## Cause The 11 extern crypto job ctxs (generateKeyPair x5, sign/verify, diffieHellman, hkdf, generatePrime, checkPrime, generateKey) completed by invoking the JS callback from inside C++ `runFromJS` while the ctx was still alive; the ctx was freed only after `then()` returned. A callback that never returns (the fixture calls `process.exit(0)` inside it) stranded everything the ctx still owned: the generated `EVP_PKEY`, `KeyObjectData` refs, `BIGNUM`s. The leak is pre-existing; oven-sh#36598 made it observable by routing `OPENSSL_malloc` through libc under ASAN. Whether LSan reported the other job types too was codegen luck (their pointers happened to be reachable by the conservative stack scan); `generateKeyPair`'s `EVP_PKEY` sits behind two FastMalloc indirections and was reported deterministically. ## Fix Make it structurally impossible for a job ctx to hold native resources across user JS: the native side never sees the callback. - `runFromJS` keeps its name (the JS-thread half, paired with the work-pool half `runTask`) but no longer receives the callback. It returns `JSCallbackArgs`, a small by-value type whose constructors are the only producers, so bodies read `return { err };` or `return { jsNull(), publicKey, privateKey };`. The extern "C" shims copy it through a typed out-pointer (C linkage cannot return a class type); the Rust side consumes it as a slice. - The Rust `extern_crypto_job!` plumbing does, in order: run `runFromJS` to produce the arguments, free the ctx (`ctx_deinit`), invoke the callback. The invariant lives in one place and applies to every job type. - Shutdown release: a completion task enqueued but not yet dispatched when `process.exit()` runs (exit racing the work pool) used to be re-queued at shutdown, stranding the ctx the same way. `AnyTaskJob` now carries an erased release entry and the shutdown release frees the job without running its completion. A completion posted after the final drain is not recoverable without joining the work pool (which would block exit); `test-crypto-op-during-process-exit.js` stays in `no-validate-leaksan.txt` for that sliver, now with an accurate comment. - The caught-export-exception paths encoded the `JSC::Exception` cell itself, so the callback's err argument was not the thrown Error (not `instanceof Error`, no `code`). They now use `Exception::value()`, matching node: JWK export of an unsupported curve surfaces `ERR_CRYPTO_JWK_UNSUPPORTED_CURVE`. No behavior change otherwise: - `Bun__EventLoop__runCallback{1,2,3}` were Rust's `EventLoop::run_callback` exported to C++. The plumbing now calls `run_callback` directly: same enter/exit bracketing, same pending-exception gate, same unhandled-exception reporting, same synchronous timing. This made `runCallback1`/`runCallback3` dead (the crypto bodies were their last callers), so their exports and declarations are deleted; `runCallback2` stays for the webview backends. - Callback arity is preserved per path (observable via `arguments.length`): error paths pass 1 arg, results 2, generateKeyPair success 3. - Exception paths are preserved: a throw out of argument production skips the callback and reports unhandled, as before. Each `runFromJS` checks its `ThrowScope` after every call that can throw (`RETURN_IF_EXCEPTION`), since the check that used to happen inside the nested `runCallbackN` call now happens after the C++ scope destructs; `BUN_JSC_validateExceptionChecks` verifies this on the asan lane. - The produced `JSValue`s live on the `then()` stack frame between production and invocation, which JSC's conservative scan covers; they are JS-heap values, so freeing the ctx first cannot invalidate them. - Perf: same number of FFI crossings, no allocation added. The Rust-native crypto jobs (pbkdf2, scrypt, random) already had the ordering property: they resolve promises or queue the callback via nextTick, so their ctx drops before user JS runs. The synchronous-callback extern jobs were the gap. ## Verification New tests in `crypto.key-objects.test.ts`: - `isASAN`-gated leak suite: children run with `BUN_DESTRUCT_VM_ON_EXIT=1` and `detect_leaks=1` (the asan lane's configuration) and call `process.exit(0)` from the callback of each job type: generateKeyPair (KeyObject and encrypted PEM outputs), sign, diffieHellman, hkdf, checkPrime, generateKey, plus an exit-before-completion-dispatch case (busy-spin so the queued completion is never dispatched). - An export-error test: `generateKeyPair('ec', { namedCurve: 'secp224r1', ...jwk encodings })` asserts the callback err is `instanceof Error` with code `ERR_CRYPTO_JWK_UNSUPPORTED_CURVE` (matches node; fails on main, which passes the Exception cell). Results: - unfixed build (src stashed): both generateKeyPair leak tests fail with the exact CI signature (`Direct leak of 24 byte(s)` in `EVP_PKEY_keygen` via `KeyPairJobCtx::runTask`) - fixed build: all pass, including under `BUN_JSC_validateExceptionChecks=1`, and ec/ed25519 keypair and verify probes run leak-clean as well - `AsyncLocalStorage-tracking.test.ts`: 74 pass, 0 fail (all async-context crypto fixtures, against both bun and node) - `crypto.test.ts` (369), `crypto.key-objects.test.ts` (117), and 37 node parallel files (`test-crypto-keygen*`, `test-crypto-sign-verify`, `test-crypto-hkdf`, `test-crypto-dh-stateless`, `test-crypto-*prime*`) all pass The break landed with oven-sh#36598 (which made the leak visible); oven-sh#36657 proposed clearing individual ctx fields before the callback, and this PR supersedes that approach with the ordering guarantee in the job plumbing instead of per-field resets. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/async_hooks/AsyncLocalStorage-tracking.test.ts test/js/node/crypto/crypto.key-objects.test.ts <!-- robobun:evidence:end -->
2 participants
What
Give
fetch()a client-side TLS session cache so a second cold connection to an origin resumes at 1 RTT instead of running a full handshake with certificate chain verification.Why
us_ssl_ctx_from_optionssetsSSL_SESS_CACHE_NO_INTERNAL, so BoringSSL never stores or looks up client sessions itself; the application must.us_ssl_new_session_cbonly parks sessions forBun.connect/node:tlssockets (theus_ssl_is_socket_ex_idxgate) and returns 0 for everything else. Nothing undersrc/http/ever callsSSL_set_session, so every fetch that misses the 64-socket keep-alive pool (more than 64 origins, a parallel cold burst, any idle gap past the 5-minute eviction) pays the full 2-RTT handshake plus chain walk.node:https.Agentalready has a JS-side session cache (src/js/node/https.ts); this is specificallyfetch.Deno gets this by default: one
Arc<rustls::ClientConfig>per client means rustls'Resumption::in_memory_sessions(256)applies.How
packages/bun-usockets/src/crypto/openssl.c: add a per-SSL session-sink ex_data slot (us_ssl_session_sink_idx) holding{owner, on_new_session, on_free}.us_ssl_new_session_cbchecks it before the existing is-socket gate and, when present, calls the owner's callback with oneSSL_SESSION_up_ref'd session (noi2d_SSL_SESSION, no pending queue). The slot's ex_data free callback callson_free(owner)thenus_frees the small wrapper.us_ssl_set_session_sink/us_ssl_get_session_sink_ownerare exported inlibusockets.h.src/http/session_cache.rs(new): a 32-entry LRU living onHTTPContext<true>(one per internedSSLConfig), keyed on the same(connect hostname, port, proxy_auth_hash)tuple the keep-alive pool uses for direct TLS so a cached session never crosses an SNI / Host-override boundary the pool wouldn't. Entries own oneSSL_SESSIONreference;Dropfrees it.src/http/lib.rsHTTPClient::on_open: alongside SNI/ALPN setup (pre-handshake,SSL_is_init_finished == 0), look up the cache and callSSL_set_sessionon a hit, then install an unarmed sink carrying the cache key.src/http/HTTPContext.rsHandler::on_handshake: arm the sink only aftercheckServerIdentitypasses. TLS 1.2 fires the new-session callback insideSSL_do_handshake(beforeon_handshake), so those tickets are parked on the sink and flushed once armed; TLS 1.3 tickets arrive post-handshake and insert directly. A handshake that fails verification never seeds the cache:close_and_failcloses the socket,SSL_freedrops the sink, and the parked ticket is released. Connections withrejectUnauthorized: false, a JScheckServerIdentitycallback, or a unix-socket path skip the cache entirely.Resumed connections still re-verify the hostname: BoringSSL's
ssl_crypto_x509_session_dupcopiesx509_peer/x509_chaininto the resumed session, soSSL_get_peer_cert_chainincheck_server_identitysees the stored leaf.BUN_FEATURE_FLAG_DISABLE_FETCH_TLS_SESSION_CACHE=1disables it.Testing
test/js/web/fetch/fetch.tls.test.tsgains aclient-side TLS session resumptiondescribe: a fixture runs anode:tlsserver that answers withConnection: closeand recordssocket.isSessionReused()per connection, then two sequential fetches with a trustedca.Related: #25185 (this covers the
fetch()client side).[review] gate passed · iteration 1 · 11 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file