Conversation
BoringSSL's SSL_CTX_set_timeout only sets the lifetime of TLS 1.2 and earlier sessions; TLS 1.3 sessions take theirs from a separate SSL_CTX_set_session_psk_dhe_timeout knob, so a configured sessionTimeout left TLS 1.3 tickets at the 2-day default. OpenSSL (and so Node) applies the one value to every protocol version; set both knobs to match.
|
Warning Review limit reached
Next review available in: 3 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 (3)
Comment |
|
Status: reproduced on bun 1.4.0 ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line BoringSSL call to make sessionTimeout reach TLS 1.3 tickets, matching Node/OpenSSL semantics.
What was reviewed
- Confirmed
SSL_CTX_set_session_psk_dhe_timeoutis BoringSSL's documented TLS 1.3 counterpart toSSL_CTX_set_timeout; the call sits inside the existing> 0guard so unset/0 still keeps both library defaults. - Security direction: this makes a user-configured resumption window actually apply to TLS 1.3 (previously silently left at 2 days) — no default is weakened.
- New tests follow harness conventions (port 0,
using, error/close wired to reject, skew-tolerant assertions) and cover TLS 1.2/1.3, client cap,Bun.serve, and the omitted-option defaults.
Extended reasoning...
Overview
The PR adds one call, SSL_CTX_set_session_psk_dhe_timeout(ssl_context, options.session_timeout), alongside the existing SSL_CTX_set_timeout call in us_ssl_ctx_build_raw (packages/bun-usockets/src/crypto/openssl.c:1330), updates the field comment in libusockets.h, and adds a describe("sessionTimeout") block with five tests to test/js/node/tls/node-tls-context.test.ts. BoringSSL, unlike OpenSSL, keeps TLS ≤1.2 and TLS 1.3 session lifetimes on two separate SSL_CTX fields; Node's single sessionTimeout option maps to one OpenSSL call that covers both, so bun (built against BoringSSL) needs both calls to match. us_ssl_ctx_build_raw is the single choke point every SSL_CTX in bun goes through (node:tls, Bun.serve/SNI, Bun.listen/connect, QUIC), so no sibling site needs the same edit.
Security risks
None introduced. The new call only fires when the user explicitly passes sessionTimeout > 0, and it makes that value take effect on TLS 1.3 tickets instead of being silently ignored in favor of BoringSSL's 2-day default. In the common case (users shortening the resumption window), this is a strict security improvement. In the uncommon case of a user setting a value above 2 days, the longer lifetime is what they explicitly requested and matches Node; TLS 1.3 resumption still runs a fresh (EC)DHE, which is exactly why BoringSSL's own default for it is longer than for TLS 1.2. No default is changed, no verification is bypassed, no flag is removed. The int → uint32_t argument conversion is safe inside the > 0 guard.
Level of scrutiny
Medium. TLS code deserves care, but the actual code delta is a single call to a documented BoringSSL API whose semantics are stated in BoringSSL's own header comments ("lifetime for TLS 1.3 sessions"), placed exactly where its TLS 1.2 sibling already lives, guarded identically. This is the archetypal "BoringSSL split one OpenSSL knob into two, call the second one" fix — mechanical and low-ambiguity.
Other factors
The test additions are well-constructed: they parse the serialized SSL_SESSION DER for the timeout [2] and ticketLifetimeHint [9] context-tagged fields, use a bounded skew derived from wall-clock elapsed time rather than exact equality (avoiding second-boundary flakes), wire error/close to reject the awaited promise, use port: 0 and using/try-finally cleanup, and cover the variant matrix (TLS 1.2 vs 1.3, server vs client option, tls.createServer vs Bun.serve, option omitted). The PR description documents that the three TLS 1.3 cases fail with 172800 on the unfixed build and pass with the fix, and that adjacent suites (ssl-ctx-cache.test.ts, Node's test-tls-session-timeout*.js) still pass. No CODEOWNERS entry covers these files. No prior human review comments are outstanding.
|
Updated 10:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 9167ab5 has some failures in 🧪 To try this PR locally: bunx bun-pr 38145That installs a local version of the PR into your bun-38145 --bun |
This branch has not been deployed
2 participants
Problem
The
sessionTimeoutTLS option (tls.createServer/createSecureContext/tls.connect,Bun.serve({ tls }),Bun.listen({ tls })) only changes the lifetime of TLS 1.2 sessions. TLS 1.3 tickets, which is what every current client negotiates, still carry BoringSSL's 2-day default and stay resumable for 2 days no matter what was configured.tls.createServer({ key, cert, sessionTimeout: 7 }), read back withopenssl s_client -sess_out+openssl sess_id -text:Cause:
us_ssl_ctx_build_raw(packages/bun-usockets/src/crypto/openssl.c:1325) applies the option withSSL_CTX_set_timeout()alone. In BoringSSL that function sets the lifetime of "TLS 1.2 (or earlier) sessions" only (vendor/boringssl/include/openssl/ssl.h,SSL_CTX_set_timeout); TLS 1.3 sessions take theirs fromSSL_CTX'ssession_psk_dhe_timeout(vendor/boringssl/ssl/ssl_session.cc,ssl_get_new_session), which nothing in bun ever set.Fix
us_ssl_ctx_build_rawalso callsSSL_CTX_set_session_psk_dhe_timeout()with the same value, inside the existing> 0guard, so omitting the option (or0) still leaves both BoringSSL defaults in place.SSL_CTX_set_timeout(), and in OpenSSL that one value is stamped onto sessions of every protocol version. Bun exposes one option too, so it has to reach both of BoringSSL's knobs. Every TLS 1.3 lifetime in BoringSSL (fresh tickets, tickets re-issued on resumption, and the client-side cap on received tickets) readssession_psk_dhe_timeout, so this one call covers all of them.us_ssl_ctx_build_rawis the single place anySSL_CTXis built (us_ssl_ctx_from_optionsfor node:tls,Bun.serveincluding SNI contexts,Bun.listen/Bun.connect;quic.cdirectly for HTTP/3, which is TLS 1.3 only and so ignored the option entirely until now), so every entry point picks this up.describe("sessionTimeout")intest/js/node/tls/node-tls-context.test.tsreads the lifetime out of the client's serialized session ('session'event) fortls.createServeron TLS 1.3 and TLS 1.2,tls.connect({ sessionTimeout })(client-side cap),Bun.serve({ tls }), and the defaults when the option is omitted. The three TLS 1.3 cases fail on the current build with172800, the TLS 1.2 and defaults cases pass before and after; all five pass with the fix.openssl s_clientprobe above prints 7 for both versions; asessionTimeout: 3server resumes a TLS 1.3 ticket right away and refuses it after 4 s (before the fix it still resumed it), same as TLS 1.2 already did;ssl-ctx-cache.test.ts,node-tls-server.test.tsand Node'stest-tls-session-timeout*.jsstill pass.Bun.TLSOptionstypings for this option and currently documents it as TLS 1.2 only; its wording can drop that caveat once this lands.Background
NewSessionTicketright after the handshake; TLS 1.3 one or moreNewSessionTicketmessages after it, which BoringSSL flushes with the server's first write). The ticket carries a lifetime in seconds; the server refuses tickets older than that and the client stops offering them.sessionTimeoutis that lifetime.SSL_CTXbecause it treats the two protocol versions differently:session_timeout(set bySSL_CTX_set_timeout, default 2 hours) for TLS <= 1.2, where resumption reuses the old key material, andsession_psk_dhe_timeout(set bySSL_CTX_set_session_psk_dhe_timeout, default 2 days) for TLS 1.3, where resumption mixes in a fresh key exchange. OpenSSL has a single timeout for both, which is why the one Node option maps onto one OpenSSL call but two BoringSSL calls.'session'event emits (i2d_SSL_SESSION): a TLS 1.3 client session storesmin(server's ticket lifetime, client's own psk_dhe timeout)in itstimeoutfield, a TLS 1.2 client session stores the server's value inticketLifetimeHintand the client's own timeout intimeout. Both peers count lifetimes from ticket issuance, so a handshake that straddles second boundaries reports that many seconds less than configured; the test allows for the seconds the connection took instead of asserting an exact value.