tls: implement OCSP stapling (OCSPRequest / OCSPResponse) - #33297
tls: implement OCSP stapling (OCSPRequest / OCSPResponse)#33297robobun wants to merge 2 commits into
Conversation
|
Updated 10:56 AM PT - Jul 9th, 2026
❌ @robobun, your commit 3159ecf has 4 failures in
🧪 To try this PR locally: bunx bun-pr 33297That installs a local version of the PR into your bun-33297 --bun |
WalkthroughThis PR adds OCSP stapling support across native TLS, Rust socket bindings, JS TLS/net plumbing, tests, and build graph output declaration handling. It wires OCSP request callbacks, stapled response delivery, async handshake suspension/resumption, and client request flags. ChangesOCSP Stapling Implementation
ChangesBuild graph updates
🚥 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/node/net.ts`:
- Line 783: The early return in the net state handling reads state.failed twice,
which triggers the JS lint rule; update the logic around the state.failed check
to cache that value in a local variable before using it in the condition and
return path. Keep the change within the same control flow in the relevant net.ts
state handling so the returned value still comes from state.failed while
avoiding the duplicate property read.
In `@src/runtime/socket/Listener.rs`:
- Around line 2111-2118: In `Listener::set_ocsp_response` handling, remove the
empty-bytes shortcut so explicit `Buffer`/TypedArray results always call
`tls.socket.get().set_ocsp_response(...)`, including `Buffer.alloc(0)`, and only
treat `null`/`undefined` as “no staple”. Keep the existing failure path with
`report(1)` for any rejected OCSP response, and update the async `resumeOCSP`
path to mirror the same validation logic if it has the same `bytes.is_empty()`
guard.
In `@src/runtime/socket/tls_socket_functions.rs`:
- Around line 1407-1412: The async OCSP flow in `resumeOCSP()` is ignoring the
failure from `socket.set_ocsp_response(bytes)` and still calling
`socket.ocsp_resolve()`, unlike the synchronous path. Update `resumeOCSP()` (and
the surrounding OCSP handling in `tls_socket_functions.rs`) to check the return
value from `set_ocsp_response`, and if it fails, surface that failure back to JS
so `failOCSPConnection(...)` can terminate the socket instead of resuming the
handshake. Keep the successful path unchanged, but only invoke `ocsp_resolve()`
when the OCSP response was accepted.
In `@src/runtime/socket/uws_dispatch.rs`:
- Around line 312-314: The OCSP response dispatch in
`TLSSocket::on_ocsp_response` is ignoring a `JsResult`, which can leave a
pending JS exception before returning to BoringSSL. Update the call site in
`uws_dispatch` to either handle the `Err` immediately by clearing/reporting the
exception before crossing the C ABI, or change `TLSSocket::on_ocsp_response` to
fully consume its error state and return `()`. Make sure the exception state is
not dropped after the JS-entrant call.
🪄 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: e6b478f5-6cde-4f17-920d-251b24bca2bd
📒 Files selected for processing (18)
packages/bun-usockets/src/context.cpackages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/libusockets.hsrc/js/node/net.tssrc/js/node/tls.tssrc/jsc/generated.rssrc/runtime/socket/Handlers.rssrc/runtime/socket/Listener.rssrc/runtime/socket/SocketConfig.bindv2.tssrc/runtime/socket/socket_body.rssrc/runtime/socket/sockets.classes.tssrc/runtime/socket/tls_socket_functions.rssrc/runtime/socket/uws_dispatch.rssrc/uws_sys/ListenSocket.rssrc/uws_sys/socket.rssrc/uws_sys/us_socket_t.rstest/js/node/tls/node-tls-ocsp.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this wires new BoringSSL callbacks (cert_cb / tlsext_status_cb) onto every SSL_CTX and adds a new async handshake-suspension path with JS re-entering from inside the handshake — worth a human pass on the TLS/memory-safety side.
Extended reasoning...
Overview
Implements OCSP stapling for node:tls ('OCSPRequest' / 'OCSPResponse' events, requestOCSP option). The change spans 18 files across the C usockets layer (openssl.c, context.c, headers), Rust FFI/dispatch (uws_sys/, runtime/socket/), JS built-ins (net.ts, tls.ts), codegen inputs, and a new 10-case test file. Roughly 700 net-new lines.
Key mechanisms: registers SSL_CTX_set_cert_cb and SSL_CTX_set_tlsext_status_cb on every SSL_CTX built via us_ssl_ctx_build_raw; adds a new per-SSL ex_data slot (us_ssl_ocsp_pending_idx) with malloc'd suspension state; extends the handshake driver to treat SSL_ERROR_WANT_X509_LOOKUP as a park state; dispatches into JS from inside cert_cb / the OCSP status callback (with loop-state save/restore); adds resolve_issuer (X509 refcount management via chain certs + trust store); and adds two new JS-visible methods (requestOCSP, resumeOCSP) plus two new socket-handler slots.
Security risks
This is TLS-handshake-critical code. The new cert_cb is installed on every SSL context — including client contexts and the HTTP-client thread's contexts — and short-circuits based on SSL_is_server / listener-ex-data / status_request presence. A logic error there would affect all TLS connections, not just those opting into OCSP. JS runs from inside BoringSSL callbacks and can synchronously destroy the socket; the PR handles this via the existing ssl_in_use deferral and loop-state save/restore, but this re-entrancy surface is exactly the class the repo's review guidelines flag as most-blocked. The staple bytes are user-controlled and passed to SSL_set_ocsp_response (BoringSSL copies), and issuer resolution walks the trust store with +1 X509 refs.
Level of scrutiny
High. Per the approval guidelines this is (a) large and cross-cutting, (b) touches security-sensitive TLS/crypto paths, (c) introduces new global behavior on every SSL_CTX, and (d) involves FFI memory management (ex_data free callbacks, X509 refcounting, malloc'd suspension state) plus JS re-entrancy from inside the handshake. None of that is mechanical.
Other factors
The implementation is well-structured, closely mirrors the existing async-SNICallback suspension pattern, and comes with a thorough test matrix plus a differential against Node v26.3.0. The bug-hunting pass found nothing. That raises confidence, but doesn't change the scrutiny level a TLS-handshake change warrants — deferring to a human reviewer.
|
Addressed the review in 9714433. Three of the four findings were real:
Declined the fourth ( Node's Re-verified after the change: all 151 |
There was a problem hiding this comment.
I didn't find any bugs, but this is a substantial TLS-handshake change spanning BoringSSL callbacks (C), Rust FFI/dispatch with in-handshake JS reentrancy, and X509 refcount handling — worth a human pass on the memory-safety and abort/suspend semantics.
Extended reasoning...
Overview
This PR wires OCSP stapling through the full stack: new cert_cb/tlsext_status_cb hooks and an async-suspension ex_data slot in openssl.c, SSL_ERROR_WANT_X509_LOOKUP handling in the read/handshake loops, Rust FFI wrappers in uws_sys, two new extern "C" dispatches (us_dispatch_ocsp_request/_response) that invoke JS from inside the BoringSSL handshake, DER encoding and X509 issuer resolution with manual X509_up_ref/X509_STORE_CTX lifecycle in tls_socket_functions.rs, new prototype methods (requestOCSP/resumeOCSP) via .classes.ts, and the 'OCSPRequest'/'OCSPResponse' event plumbing in net.ts/tls.ts. 18 files, ~700 net lines.
Security risks
OCSP stapling is a TLS security feature and the change sits directly in the server/client handshake path. The tri-state abort_handshake out-param (0=proceed, 1=abort, 2=suspend) and its interaction with SSL_set_SSL_CTX (SNI context swap) determine whether a handshake completes, aborts, or parks; getting the ordering or a fall-through wrong could complete a handshake without the intended staple or hang a connection. The client-side status callback runs before the session is established so a listener can destroy the socket to reject a staple — the PR adds if (self.destroyed) return guards in the handshake completion path to make that stick. There's also new manual memory management: a us_calloc'd per-SSL pending struct freed via ex_data destructor, and resolve_issuer returning a +1 X509 that the caller must free.
Level of scrutiny
High. This is production TLS code with new C allocations, BoringSSL callback registration on every SSL_CTX, JS reentrancy from inside SSL_do_handshake/SSL_read (bracketed by us_internal_ssl_loop_state_save/restore), and cross-language ownership transfers. It follows the existing async-SNI suspension pattern closely, which is reassuring, but the surface area and criticality warrant a maintainer familiar with the uSockets SSL layer.
Other factors
Test coverage is solid (10 cases including sync/async/null/error/TLSv1.2/issuer/destroy-in-listener, plus the vendored Node test-tls-ocsp-callback.js), and the PR description documents a differential run against Node v26.3.0. Two CodeRabbit comments remain marked unresolved: the empty-buffer-as-no-staple behavior appears intentional (backed by a test and a code comment citing Node's semantics), and the JsResult-drop concern looks addressed (on_ocsp_response now returns () and consumes exceptions internally). No human review has happened yet.
|
CI caught a real crash in the first version of this, fixed in d90f4b7. Flagging it because it's the kind of thing a reviewer should know the shape of. Every proxy test segfaulted. The client-side OCSP callback recovered struct loop_ssl_data *lsd = BIO_get_data(SSL_get_wbio(ssl));
struct us_socket_t *s = lsd ? lsd->ssl_socket : NULL;and I gated it on nothing, on the reasoning that the callback only fires once a connection enabled stapling, and the only thing that enables stapling is That reasoning was wrong. The fix marks the SSLs whose BIO pair Verified after the fix: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/node/tls/node-tls-ocsp.test.ts`:
- Around line 242-247: The OCSP warning assertion in node-tls-ocsp.test.ts is
too strict because warnedOCSPUnsupported is process-global and only logs once.
Update the test around the secureConnect/secureConnection checks to either run
this case in an isolated subprocess or assert more flexibly so it passes whether
warnings contains the OCSP message or is empty due to prior requestOCSP calls.
🪄 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: 8d3463e6-3ea3-49dc-bf1a-c5920dc38150
📒 Files selected for processing (7)
packages/bun-usockets/src/crypto/openssl.csrc/js/node/net.tssrc/runtime/socket/socket_body.rssrc/runtime/socket/tls_socket_functions.rssrc/runtime/socket/uws_dispatch.rssrc/uws_sys/socket.rstest/js/node/tls/node-tls-ocsp.test.ts
|
The bug
"Next build" is the hole. A For a struct that crosses the C++/Rust FFI boundary, that links a binary whose C++ side memcpys past the end of the Rust side's stack slot: Without ASAN it's silent stack corruption. It reproduces on any incremental build that adds or removes a field from a The fixDeclare the headers as outputs of the codegen edge, so ninja rebuilds the objects whose depfiles name them. The other undeclared-output cases named in that comment ( Why it surfaced hereThe Also in this commit, both from review: a DataView OCSP response is no longer rejected by a guard whose own error message listed DataView as acceptable, and the Duplex-transport case moved to a subprocess because the warning it asserts on fires once per process. |
On the alpine
|
| build | branch | files touched |
|---|---|---|
| #68197 | child-process-spawn-error-contract |
src/js/node/child_process.ts |
| #68094 | node26-dgram-compat |
dgram / udp |
| #67992 | net-pending-after-half-close |
net |
The test asserts that the implicit once('connect') listener is collectable after the event fires: globalThis.gc() (a synchronous full collect), then one setImmediate, then the FinalizationRegistry callback must already have run. It is a conservative-stack-scan coin flip, and musl-x64 loses it sometimes. There is no object-graph edge from this diff to the listener closure: the only thing it added to the plain-TCP connect path was one more protected JSValue pointing at a module-level function.
That said, the callback had no business being on that path at all, so e9e191c takes it off:
ocspResponseno longer sits on the default client handler table. A client can only ever receive anOCSPResponseif it connected withrequestOCSP, soclientHandlersFor()picks the OCSP-carrying table only then. Every plainnet.connect()now protects one fewerJSValueand skips onewithAsyncContextIfNeeded(). This mirrorsserverHandlersFor(), which already strips the SNI/ALPN members from a listener that configures neither.- The fd-adoption table (
SocketHandlers) is only ever installed on a plain socket that never upgrades to TLS, so its OCSP members were dead and are gone.
test/js/node/tls/node-tls-ocsp.test.ts still passes 13/13, and test/js/node/tls/ has the same three failures with and without the diff (two fail on the released binary too; the third is a debug-build timeout).
There was a problem hiding this comment.
All prior findings are addressed and I didn't spot anything new, but this wires new BoringSSL callbacks (cert_cb / tlsext_status_cb) onto every SSL_CTX, adds a handshake-suspension state, and carries an unrelated bindgenv2 build fix — someone familiar with openssl.c should sign off on the design.
Extended reasoning...
Overview
Implements node:tls OCSP stapling end-to-end: server-side 'OCSPRequest' via BoringSSL's cert_cb (with async handshake suspension over SSL_ERROR_WANT_X509_LOOKUP), client-side 'OCSPResponse' via the legacy status callback, plus the Rust/JS plumbing across bun-usockets, uws_sys, socket_body.rs, Listener.rs, net.ts/tls.ts, and generated bindings. It also folds in a scripts/build/codegen.ts / bindgenv2 fix so generated headers are declared ninja outputs. 21 files, ~600 lines added.
Security risks
This is TLS handshake code. SSL_CTX_set_cert_cb and SSL_CTX_set_tlsext_status_cb are now registered on every context (gated at runtime by listener/kind checks and the new us_ssl_loop_bio_idx marker), and SSL_ERROR_WANT_X509_LOOKUP is added to the pause conditions in both ssl_update_handshake and us_internal_ssl_on_data. JS listeners run from inside SSL_do_handshake and can destroy the socket reentrantly. The PR already surfaced one real ASAN crash during development (proxy tunnels' SSLWrapper BIO being misread as loop_ssl_data), which was fixed with the ex-data marker — but that's exactly the class of hazard that warrants a maintainer's eye on the callback-registration and BIO-recovery design.
Level of scrutiny
High. Native TLS handshake state machine + cross-language FFI (C ↔ Rust ↔ JS) + reentrant user callbacks + memory-lifetime management via SSL ex_data. The bindgenv2 build-system change is sound in isolation but is a separate concern riding in the same PR.
Other factors
Test coverage is thorough (12 cases + a subprocess fixture, differential-verified against Node v26.3.0, plus the 151 vendored test-tls-* and proxy suites re-run). All four CodeRabbit findings and my three prior inline findings were addressed with code changes and pinned tests. Nothing outstanding blocks review; this just isn't a change I can approve without a human who knows packages/bun-usockets/src/crypto/openssl.c confirming the hook placement and suspension semantics.
|
Agreed on where the eyes are needed. A reviewer's map of the three decisions that are actually load-bearing, so whoever knows 1. Why
2. Why it is registered on every
3. Why the client status callback gates on an ex_data marker ( This is the one that bit. The callback recovers The handshake suspension itself reuses the existing async- On the bindgenv2 change riding along: it is separable, and I'd rather it landed on its own if that's easier. It is here because adding a field to a |
Status after the rebase: diff is green, CI is flaking on tests every other PR hits
I checked So: the diff is green, the new OCSP test is 16/16, all previously-attributed red is gone, and the four remaining failures are flake every other open PR sees. The one thing this is waiting on is a reviewer familiar with |
Rebased onto fc865b3 across #31859's Handlers rework: the OCSP callbacks are now two more fields on JSSocketHandlers (JSInternalFieldObjectImpl<16>), and the socket dispatches use the ThisPtr + Rc<Handlers> shape the sibling dispatches moved to. Server side: BoringSSL's cert_cb (the one server hook that runs after the ClientHello extensions are parsed, after select_certificate_cb's SNI context swap, and can pause) dispatches 'OCSPRequest' with the connection's certificate and issuer as DER; an asynchronous listener suspends the handshake (SSL_ERROR_WANT_X509_LOOKUP, parked like the existing async-SNICallback suspension) until handle.resumeOCSP() re-drives it. callback(err) surfaces as 'tlsClientError' and drops the connection. Client side: requestOCSP enables stapling before the ClientHello goes out, and BoringSSL's legacy OCSP callback delivers the response from inside the handshake via us_dispatch_ocsp_response, so destroying the socket in the listener still aborts the session. Destroying a client socket from inside a handshake callback no longer emits 'secureConnect'; us_internal_ssl_close settles the handshake on its way out. TLS over a generic Duplex / a Windows named pipe (the SSLWrapper engine) warns once instead of silently accepting requestOCSP. scripts/build/codegen.ts declares bindgenv2's generated headers as ninja outputs, so adding a field to a .bindv2.ts dictionary no longer leaves an object file compiled against the old struct layout.
e9e191c to
2011126
Compare
Node's onOCSP (internal/tls/wrap.js) gates on `if (response)`, not `response != null`: callback(null, 0) / callback(null, false) / callback(null, "") complete the handshake unstapled. A truthy non-Buffer is still rejected the way Node's native setOCSPResponse would (THROW_AND_RETURN_IF_NOT_BUFFER), only routed through 'tlsClientError' instead of landing as an uncaught exception in the listener's call stack.
1 participant
node:tlsacceptedrequestOCSPand documented the'OCSPRequest'/'OCSPResponse'events, then ignored all three. A server that served staples from anOCSPRequestlistener never saw the event, and a client that setrequestOCSP: truenever heard back, so code that believes it is stapling silently isn't.Repro
Cause
Nothing in the TLS layer was wired to BoringSSL's OCSP hooks.
requestOCSPwas dropped on the floor, no server hook looked at the ClientHello'sstatus_requestextension, and no response was ever staged on the connection.Fix
Server (
'OCSPRequest'). BoringSSL'scert_cbis the one server hook that runs after the ClientHello extensions are parsed (soSSL_get_tlsext_status_typeis meaningful) and afterselect_certificate_cb's SNI context swap (which re-derives the connection'sCERTand would otherwise discard a response staged earlier), and can pause the handshake.us_cert_cbdispatches the event with the connection's certificate and issuer as DER buffers; a listener that answers synchronously stages its response inline, and an asynchronous one suspends the handshake (SSL_ERROR_WANT_X509_LOOKUP, parked the same way the existing async-SNICallbacksuspension is) untilhandle.resumeOCSP()re-drives it.callback(err)surfaces as'tlsClientError'and drops the connection.It is registered on every
SSL_CTXrather than only on listeners that want it, becauseSSL_set_SSL_CTXre-derives the per-connectionCERTfrom the SNI-selected context andcert_cbtravels with theCERT. It short-circuits unless the handshake is a server one whose listener registered a handler and whose ClientHello asked for stapling.Client (
'OCSPResponse').requestOCSPenables stapling on the connection before the ClientHello goes out, and BoringSSL's legacy OCSP callback (which it calls fromssl_verify_peer_cert, explicitly "to emulate OpenSSL's client OCSP callback") delivers the response from inside the handshake, the point Node emits the event from, so destroying the socket in the listener to reject a staple still prevents the session from being established.The JS side of that dispatch is only registered when a connection actually asked for a staple (
clientHandlersFor), the same wayserverHandlersForalready strips the SNI/ALPN members from a listener that configures neither, so a plainnet.connect()doesn't protect a callback that can never fire.Also fixed along the way: destroying a client socket from inside a handshake callback still emitted
'secureConnect', becauseus_internal_ssl_closesettles the handshake on its way out.Verification
test/js/node/tls/node-tls-ocsp.test.tscovers async/sync/null responses, a DataView response, an empty response, falsy primitives handed back as the response, no listener, a client that never asks, TLSv1.2, issuer resolution,callback(err), a non-Buffer response, and destroying the client from the'OCSPResponse'listener. 15 of the 16 fail onmain; the sixteenth is the negative contract (no'OCSPRequest'without a client that asks for it).Node's own
test/parallel/test-tls-ocsp-callback.js(already vendored, skipped without anopensslCLI next to the binary) now passes.Differential against node v26.3.0 across the variant matrix
Identical output on both runtimes for: async staple on TLS 1.3, synchronous staple,
callback(null, null), a client that does not request stapling, a server with no listener, TLS 1.2, issuer resolved viaca,callback(err),SNICallback+ stapling on the same connection, and a 4 KiB response.Regression runs: all 151
test/js/node/test/parallel/test-tls-*.js,test/js/node/tls/andtest/js/node/net/(each has the same failures with and without this diff: two of them also fail on a releasedbunin the same container, the third is a debug-build timeout), andtest/js/bun/net/with no new failures.bun run rust:check-allis clean across all 10 targets.Rebase onto #31859
#31859 replaced
Handlers's thirteen protectedJSValues with a GC-visitedJSInternalFieldObjectImplcell, and moved the socket dispatches toThisPtr<Self>+Rc<Handlers>+ anOption<ThisPtr<TLSSocket>>ext slot. The rebase ports OCSP onto that design rather than onto the removed one:Bun::JSSocketHandlersgrows from 14 to 16 internal fields (OCSPRequest,OCSPResponse); the RustFieldenum andCALLBACK_COUNTfollow.Handlers::validate_callbacksreturns the two new entries at the end of its array, andHandlersexposeson_ocsp_request()/on_ocsp_response()accessors.NewSocket::on_ocsp_response,us_dispatch_ocsp_responseandus_dispatch_ocsp_requestnow match the sibling dispatches (ThisPtr<Self>,handlers.enter()scope,Option<ThisPtr<TLSSocket>>ext slot) rather than raw*mut Self+enter_ref.The squashed commit is
2011126fon top offc865b398e; the seven pre-rebase commits are ine9e191cif per-step history is useful.Also in this PR
scripts/build/codegen.tsandsrc/codegen/bindgenv2/script.tscarry a build-graph fix that is not about TLS: bindgenv2's generated.hfiles were never declared as ninja outputs, so adding a field to a.bindv2.tsdictionary (which this PR does) could leave an object file compiled against the old struct layout and link a binary that memcpys past the end of the other side's stack slot. It is explained in full here and is separable if you would rather land it on its own.Not covered
TLS over a generic
Duplexstream (tls.connect({ socket: someDuplexStream })) and Windows named pipes run on theSSLWrapperengine rather than aus_socket_t. Its private memory BIO leavesus_ocsp_status_cbwith no socket to dispatch'OCSPResponse'on, and the parked-queue mechanism its'session'/'keylog'events use drains after the handshake, which is the wrong side of'secureConnect'for this event. Node honorsrequestOCSPthere, so this is a real compat gap, just a narrower one than the transport this PR fixes.Rather than leave the same accepted-but-ignored shape behind,
requestOCSP()now reports whether it took effect and the connect path warns once when it did not, instead of puttingstatus_requeston the wire and discarding the answer. Making it actually work needs a synchronous dispatch path through the SSLWrapper engine; that belongs in its own change.Out of scope
socket.renegotiate()was raised alongside this. BoringSSL does not implement caller-initiated renegotiation in either role (ssl_lib.cc: "Caller-initiated renegotiation is not supported"), and Bun already reports that through the callback asERR_TLS_RENEGOTIATION_UNSUPPORTEDrather than silently doing nothing, so there is nothing to change there without patching BoringSSL.[review] gate passed · iteration 7 · 23 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 7
evidence per changed file