node:tls: return the ticket-bearing session from getSession()/getTLSTicket() on TLS 1.3 - #36475
Conversation
…icket() on TLS 1.3 BoringSSL delivers a TLS 1.3 NewSessionTicket only to the new-session callback; it never updates the SSL's established_session, so SSL_get_session() returns a snapshot without the ticket. The blob getSession() returned could not resume (full handshake on reconnect), and getTLSTicket() returned undefined. Node's OpenSSL updates the connection's session with each ticket, so both accessors work there. Keep a reference to the most recent SSL_SESSION the new-session callback saw in an ex_data slot on the SSL, and read from it in getSession() and getTLSTicket() (falling back to SSL_get_session() before any ticket arrives). The reference is released on SSL_free via the ex_data free hook. TLS 1.2 is unchanged: the new-session callback there receives the established_session itself.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesTLS session retrieval
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
No issues found, but this touches BoringSSL SSL_SESSION refcounting in openssl.c — worth a human sign-off.
What was reviewed:
- ex_data slot lifecycle:
SSL_SESSION_up_refon stash,SSL_SESSION_freeon replace, free-hook onSSL_free— balanced on all paths, matches the neighboringus_ssl_pending_session_idxpattern. - Borrowed-session lifetime in
get_session/get_tls_ticket: no user JS or network read runs between borrow andi2d_SSL_SESSION/get0_ticket, so the pointer stays valid across the JS buffer allocation. - Other
SSL_get_sessioncallers (openssl.c:1613,ncrypto.cpp:2983) only read the protocol version — established_session is correct there, no sibling fix needed. - Tests cover TLS 1.2/1.3 × direct/duplex-wrapped, assert
isSessionReused()on both ends, wireerrorto reject.
Extended reasoning...
Overview
Fixes TLSSocket.getSession() / getTLSTicket() on TLS 1.3 clients to return the ticket-bearing session (matching Node), instead of BoringSSL's unresumable established_session snapshot. Touches:
packages/bun-usockets/src/crypto/openssl.c: new SSL ex_data slot holding a ref to the most recentSSL_SESSION*delivered tous_ssl_new_session_cb, with a free hook and aus_ssl_get_new_session()accessor.packages/bun-usockets/src/libusockets.h: header decl for the accessor.src/runtime/socket/tls_socket_functions.rs:current_session()helper that prefers the stashed session, falls back toSSL_get_session(); bothget_sessionandget_tls_ticketnow call it.test/js/node/tls/node-tls-connect.test.ts: newdescribe.eachover TLSv1.2/TLSv1.3 covering direct-TCP and duplex-wrapped resumption + ticket presence.
Security risks
This is a Node-compat fix for session-resumption accessors, not a change to any verification/auth path. The security-relevant aspect is native memory safety: the new ex_data slot holds a +1 SSL_SESSION ref. I traced the refcount — up_ref on stash, free of the previous on replace, and us_ssl_new_session_ref_free on SSL_free — and it's balanced on every terminal path. SSL_set_ex_data return isn't checked, but that matches every other ex_data write in the file and only fails on OOM. No security regression identified.
Level of scrutiny
High — this is C-level BoringSSL refcounting inside the TLS stack, the most-blocked category in REVIEW.md. Even though the pattern mirrors the adjacent us_ssl_pending_session_idx/us_ssl_pending_keylog_idx slots exactly and the diff is small, native crypto memory management warrants a maintainer's eyes rather than bot approval.
Other factors
- The PR description cites BoringSSL's own
ssl.hdocs for whySSL_get_session()is unresumable on TLS 1.3, and correctly notes TLS 1.2 is unchanged (ssl_update_cachepasses established_session itself to the callback). - I checked the two other in-tree
SSL_get_sessioncall sites (us_internal_verify_peer_certificatein openssl.c andSSLPointer::verifyPeerCertificatein ncrypto.cpp) — both only readSSL_SESSION_get_protocol_version, which established_session reports correctly, so they don't need the same treatment. - The Rust-side borrowed pointer from
us_ssl_get_new_sessionis documented as valid until the next NewSessionTicket or SSL_free; both consumers use it synchronously with only a JS buffer allocation in between (no user callbacks, no network reads), so it can't be invalidated mid-use. - Related PR #33514 takes a different approach (per-socket ticket bytes in Rust); a maintainer should decide which lands.
- No CODEOWNERS on the changed paths.
On a TLS 1.3 client,
socket.getSession()called after the'session'events have fired returns a session that cannot resume: passing it totls.connect({ session })results in a full handshake (isSessionReused() === falseon both ends). The Buffer the'session'event carries does resume.getTLSTicket()returnsundefinedon the same connection. Node returns the latest ticket-bearing session from both accessors and it resumes. TLS 1.2 already matched Node.Repro
Cause
BoringSSL's
SSL_get_session()returnsssl->s3->established_session, set once at handshake completion. When a TLS 1.3 NewSessionTicket arrives post-handshake, BoringSSL builds a newSSL_SESSION(tls13_create_session_with_ticket) and hands it only to the new-session callback;established_sessionis never updated. This is documented inssl.h:Node uses OpenSSL, which replaces the connection's session with the ticket-bearing one, so
SSL_get_session()reflects it there.Bun already receives these sessions in
us_ssl_new_session_cb(where the'session'event's payload comes from) but only serialized them for the event queue.Fix
Keep a reference to the most recent
SSL_SESSION*the new-session callback saw in an ex_data slot on the SSL, and havegetSession()/getTLSTicket()read from it (falling back toSSL_get_session()before any ticket arrives or on sockets that never opted into session events). The reference is released onSSL_freevia the ex_data free hook, and replaced on each subsequent ticket.TLS 1.2 is unchanged:
new_session_cbthere receivesestablished_sessionitself (ssl_update_cache), so reading from the ex_data slot yields the same session.Verification
Against node v26.3.0 on loopback, after the first
'session'event:getSession()resumesgetTLSTicket()getSession()resumesgetTLSTicket()New tests in
test/js/node/tls/node-tls-connect.test.tscover both protocol versions on the direct-TCP and duplex-wrapped paths; the TLS 1.3 cases fail onmain(clientReused: false,serverReused: [false, false]) and pass here.Related: #33514 fixes
getTLSTicket()by tracking ticket bytes in Rust per-socket state; this change addresses both accessors at the usockets layer with no per-socket state.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file