node:tls: report the fatal TLS alert when a handshake over a Duplex fails - #32929
node:tls: report the fatal TLS alert when a handshake over a Duplex fails#32929robobun wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesThe SSL wrapper now preserves fatal BoringSSL handshake reasons and reports them as specific TLS errors. Node TLS tests cover ALPN mismatch and fatal TLS error reporting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 1:00 AM PT - Jul 31st, 2026
❌ @Jarred-Sumner, your commit 51ac82e has 5 failures in
🧪 To try this PR locally: bunx bun-pr 32929That installs a local version of the PR into your bun-32929 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
I read both before adding any #9365: the reported callback is #20727: the symptom there is a hang. Per the reporter's own trace the fatal alert is received and the socket is closed, but no error ever reaches JS, so the promise never settles. This PR does not change whether any callback fires, only which error value the handshake callback carries once it does, and it only touches the |
![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 issues — the fix cleanly mirrors the C path's ssl_dispatch_parked_reason and the stack-backed reason pointer follows the same synchronous-clone contract — but since this is unsafe Rust in the TLS handshake path it's worth a human look.
Extended reasoning...
Overview
This PR fixes error reporting in SSLWrapper::update_handshake_state (src/uws/lib.rs), the Rust TLS wrapper used when tls.connect() is given a generic Duplex (or Windows named pipe) instead of a real socket. On a fatal handshake failure it now captures ERR_peek_last_error() before ERR_clear_error() and passes it to the handshake callback as an EPROTO-shaped us_bun_verify_error_t, instead of falling through to the unrelated X509 verify result. A new test in node-tls-connect.test.ts covers both the direct and duplex-proxy paths for the ALPN-mismatch alert.
Security risks
The change is in error reporting only — it does not alter what handshakes are accepted or rejected, nor any verification logic. The only memory-safety consideration is that us_bun_verify_error_t.reason now points into a stack [u8; 256] that lives for the duration of the synchronous trigger_handshake_callback. I verified this matches the existing C-path contract exactly (ssl_dispatch_parked_reason in openssl.c:1420-1435 uses a stack char reason[] the same way), and that the two SSLWrapper consumers (UpgradedDuplex::on_handshake, WindowsNamedPipe::on_handshake) immediately clone code/reason into owned Box<CStr> before any later use. net.ts already recognizes the EPROTO shape and routes it through tlsHandshakeError. I see no new attack surface.
Level of scrutiny
Medium-high. The logic itself is small and well-contained, and it brings the Rust wrapper into parity with a pattern that already exists and is tested in the C path. However, it is unsafe Rust (FFI, raw-pointer-backed C strings, the laundered-self re-entrancy pattern) inside the TLS handshake state machine, so it warrants a human reviewer who knows this code rather than a bot rubber-stamp.
Other factors
No CODEOWNERS apply to the touched paths. The bug-hunting system found nothing. The added test is well-targeted (asserts the exact ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL code and that checkServerIdentity is never invoked) and runs under both transport variants in the existing matrix. The PR description demonstrates the before/after behavior against Node. CI is still building.
There was a problem hiding this comment.
I didn't find any issues — the stack-buffer lifetime and EPROTO shape both check out against the C path — but this is unsafe Rust on the TLS handshake error path with a raw-pointer lifetime contract, so a human pass would be worthwhile.
Extended reasoning...
Overview
This PR fixes error reporting in SSLWrapper::update_handshake_state (src/uws/lib.rs), the Rust TLS engine used when tls.connect() is given a Duplex or Windows named pipe instead of a native socket. Previously, on a fatal handshake failure (SSL_ERROR_SSL/SSL_ERROR_SYSCALL), the code called ERR_clear_error() immediately and then reported SSL_get_verify_result() of a non-existent peer certificate, surfacing a phantom UNABLE_TO_GET_ISSUER_CERT instead of the real alert (e.g. no_application_protocol). The fix peeks the BoringSSL error queue before clearing it and passes the captured reason through the handshake callback as the same {error_no: -71, code: "EPROTO", reason} shape the uSockets C path uses in ssl_dispatch_parked_reason (openssl.c:1431-1432). A new test in node-tls-connect.test.ts covers both the direct and duplex-proxy variants.
Security risks
The change is in TLS handshake code, which is security-adjacent, but it only affects which error is reported after a handshake has already definitively failed. It does not alter whether handshakes succeed, certificate verification, or trust decisions — set_fatal_error(is_fatal) and trigger_close_callback() still fire exactly as before. If anything, surfacing the real failure reason rather than a misleading certificate error is a net positive for clients that key retry/fallback logic on e.code. No injection, auth-bypass, or data-exposure surface is introduced.
Level of scrutiny
This warrants a human look despite being small and well-tested:
- It is unsafe Rust touching BoringSSL FFI on a TLS error path.
- The new
peek_fatal_ssl_errorreturns aus_bun_verify_error_twhosereasonfield points into a caller-provided stack[u8; 256]. I traced the consumption chain (trigger_handshake_callback→UpgradedDuplex::on_handshake(clones into ownedCertError) →DuplexUpgradeContext::on_handshake→TLSSocket::on_handshake→verify_error_to_js) and it is fully synchronous, withreason_bufdeclared inupdate_handshake_stateoutliving the entire call. This mirrors the C path's stack-localchar reason[256]exactly. But pointer-lifetime contracts across a multi-hop fn-pointer vtable are the kind of thing a maintainer should sign off on.
Other factors
- The fix is a faithful port of the existing C-path mechanism (
ssl_dispatch_parked_reasonin openssl.c), so the design is already established. - The
is_fatalboolean is hoisted but semantically unchanged;ERR_clear_error()still runs in the same place. - The new test slots into the existing direct/duplex-proxy matrix and asserts the exact
code/libraryplus thatcheckServerIdentityis never invoked, which would catch regressions in either engine. - The author's analysis of why this does not fix #9365/#20727 is careful and correct, which raises confidence in the change itself.
There was a problem hiding this comment.
I didn't find any issues — the EPROTO shape, the -71 error_no, and the stack-buffer lifetime all line up with ssl_dispatch_parked_reason in openssl.c, and UpgradedDuplex::on_handshake clones the reason synchronously — but this is unsafe Rust touching raw-pointer lifetimes in the TLS handshake path, so it's worth a human look.
Extended reasoning...
Overview
This PR fixes error reporting in SSLWrapper::update_handshake_state (src/uws/lib.rs), the TLS engine used when tls.connect() is given a generic Duplex or Windows named pipe. On a fatal handshake failure (SSL_ERROR_SSL/SSL_ERROR_SYSCALL), the wrapper was calling ERR_clear_error() immediately after SSL_get_error(), discarding the queued BoringSSL alert reason, then reporting the unrelated X509 verify result instead. The fix peeks ERR_peek_last_error() into a 256-byte stack buffer before the clear and passes it to the handshake callback as {error_no: -71, code: "EPROTO", reason: buf} — exactly the shape ssl_dispatch_parked_reason in packages/bun-usockets/src/crypto/openssl.c already produces for the native TCP path. net.ts already recognizes code === "EPROTO" as a protocol failure and decomposes it into the ERR_SSL_* code. A regression test is added to the existing tls.connect / tls.connect using duplex proxy matrix.
Security risks
None identified. The change only affects which error value is reported when a handshake has already failed; it doesn't change verification logic, trust decisions, or whether the handshake succeeds. If anything it improves security ergonomics by surfacing the real failure reason instead of a misleading certificate error.
Level of scrutiny
This warrants human review. The change is small and well-argued, but it's unsafe Rust in the TLS handshake path: it constructs a us_bun_verify_error_t whose reason field points into a stack-local [u8; 256], hands that struct through a callback vtable, and relies on the callback chain consuming it synchronously before the frame unwinds. I traced this and it holds — reason_buf lives for all of update_handshake_state, trigger_handshake_callback runs synchronously within it, and UpgradedDuplex::on_handshake immediately boxes the CStr into this.ssl_error before forwarding (matching the C path's stack-local reason[] in ssl_dispatch_parked_reason). But pointer-lifetime reasoning across an FFI/callback boundary in a TLS engine is exactly the kind of thing a maintainer should sign off on.
Other factors
- The fix is a faithful port of existing, already-shipped logic from
openssl.c(.error = -71, .code = "EPROTO", .reason = reason), reducing novel-design risk. - No CODEOWNERS entries cover the modified files.
- The test is well-targeted: it runs in both the direct and duplex-proxy variants, asserts the exact
code/library, and verifiescheckServerIdentityis never invoked. - robobun reported CI failures on an earlier commit (4173c41); two "ci: retrigger" commits followed, so the current build status should be confirmed before merge.
|
The remaining CI red is all infrastructure; neither run has a single failing test. Across both Buildkite builds on this PR (65810 and the retriggered 66005), zero test files failed. Every red job is one of:
None of those mechanisms can be affected by this diff, which only changes which error object the TLS handshake callback carries after a handshake has already failed. The change itself is verified: the new test fails on an unfixed build (the duplex variant reports |
…ails When tls.connect() is given a generic Duplex (or a Windows named pipe), the handshake runs through SSLWrapper in src/uws/lib.rs rather than the uSockets C path. On a fatal handshake failure it called ERR_clear_error() immediately after SSL_get_error(), discarding the alert reason BoringSSL had queued, and then built the handshake error from SSL_get_verify_result() of a certificate that was never received. A server rejecting the ALPN list therefore surfaced as UNABLE_TO_GET_ISSUER_CERT (or, with no verify result at all, fell through to checkServerIdentity() against an empty cert and produced ERR_TLS_CERT_ALTNAME_INVALID) instead of ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL. Capture ERR_peek_last_error() before the clear and dispatch it as the EPROTO verify error, the same shape ssl_dispatch_parked_reason() already uses on the C path (which is why direct TCP tls.connect was unaffected). net.ts then recognizes it as a protocol failure, surfaces the real ERR_SSL_* code, and never runs the identity check.
…horized: false On the duplex upgrade path, a handshake_failure alert with verification disabled previously fell through to secureConnect because the discarded OpenSSL reason left only a phantom X509 code that the protocol-failure check does not recognise. The fix in the previous commit makes this error correctly; this test locks it in across both the native-socket and duplex-proxy variants.
ff4f9f7 to
dbcb9d8
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased onto main (import-list conflict in Also switched the peek from Both duplex-proxy tests fail on main and pass with this change; the direct |
The inline note is redundant with peek_fatal_ssl_error's name and its position before ERR_clear_error(); the doc comment's lifetime remark is visible from the body (reason: buf.as_ptr().cast()). Keep only the openssl.c cross-reference.
ssl_park_fatal_reason in openssl.c reads ERR_peek_error() (the oldest queue entry, which is the root cause and what Node reports); this helper was reading ERR_peek_last_error() (the newest, a wrapping context entry). For an alert that pushes a single entry (ALPN mismatch) the two agree, but for a handshake_failure alert BoringSSL pushes the alert reason first and HANDSHAKE_FAILURE_ON_CLIENT_HELLO on top, so the duplex path diverged from the native path. With both paths reading the root-cause entry the handshake_failure test can assert the exact code.
There was a problem hiding this comment.
The ERR_peek_last_error → ERR_peek_error fix in 40f57c2 addresses my earlier finding — both engines now report the same root-cause code and the test asserts it exactly. No further issues found; deferring to a human for the final sign-off since this is TLS handshake error-handling.
What was reviewed:
peek_fatal_ssl_errornow matchesssl_park_fatal_reasonin openssl.c (sameERR_peek_errorposition, same{-71, "EPROTO", reason}shape).reason_buflifetime: stack-local, live across the synchronoustrigger_handshake_callback; every consumer (UpgradedDuplex,WindowsNamedPipe,TLSSocket::on_handshake,ProxyTunnel,WebSocketProxyTunnel) clones or ignores the pointer before returning.- Tests assert exact codes in both the direct and duplex-proxy variants; the handshake_failure regex is gone.
Extended reasoning...
Overview
This PR fixes error reporting in SSLWrapper::update_handshake_state (src/uws/lib.rs) — the memory-BIO TLS engine used for tls.connect({ socket: <Duplex> }) and Windows named pipes. On a fatal handshake failure it was calling ERR_clear_error() immediately after SSL_get_error(), discarding the queued alert reason, then reporting whatever SSL_get_verify_result() said about a certificate that was never received. The fix peeks the oldest queued error before clearing and passes it through as the same EPROTO shape the native uSockets C path already produces via ssl_dispatch_parked_reason.
Two tests are added to the existing tls.connect / tls.connect using duplex proxy matrix in test/js/node/tls/node-tls-connect.test.ts.
Security risks
The change is confined to which error object the handshake callback carries after SSL_do_handshake has already failed. It does not alter any verification decision — is_fatal was already computed before this PR; the only new behavior is reading the queued reason instead of the (meaningless) X509 verify result. It actually closes a security-relevant misbehavior: with rejectUnauthorized: false on the duplex path, a fatal handshake alert previously fell through to secureConnect on a TLS layer that was never established.
The stack-local reason_buf pointer stored in us_bun_verify_error_t was checked against every SSLWrapper on_handshake consumer: UpgradedDuplex and WindowsNamedPipe clone into an owned CertError, TLSSocket::on_handshake boxes code_bytes()/reason_bytes(), and ProxyTunnel/WebSocketProxyTunnel only read error_no or clone via HTTPCertError::from_verify_error. None retains the raw pointer past the synchronous call. The C path this mirrors (ssl_dispatch_parked_reason, openssl.c:1652-1658) uses the identical stack-local pattern.
Level of scrutiny
TLS handshake error handling warrants a maintainer look even for a small diff. The change itself is narrow (~25 net lines in lib.rs), directly ports a mechanism that already exists in the native path, and is covered by tests that assert exact error codes on both engines — but it lives in SSLWrapper, which every duplex/named-pipe TLS flow goes through.
Other factors
- I previously flagged that the helper read
ERR_peek_last_error()(newest entry) instead ofERR_peek_error()(oldest / root cause) — 40f57c2 fixes that, matching openssl.c:1627 and Node'scrypto_tls.cc. The handshake_failure test was correspondingly tightened from a regex to the exactERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILUREcode. - The comment-cop lint on verbose comments was addressed in 873f747.
- All prior inline threads on this PR are resolved.
- CI on 40f57c2 was still building at the time of the last robobun status update; earlier builds had only infrastructure failures (artifact-download timeouts, expired agent slots) with zero test failures.
|
All 12
|
There was a problem hiding this comment.
The ERR_peek_last_error → ERR_peek_error switch from my earlier review is applied and the handshake_failure test now pins the exact code — thanks. I didn't find anything else, but this is native TLS handshake-failure handling with a stack-buffer pointer crossing an FFI callback, so I'd like a human to sign off.
What was reviewed:
peek_fatal_ssl_errorshape (error_no: -71,code: "EPROTO",reason→ stack buf) matchesssl_dispatch_parked_reasonin openssl.c line-for-line.reason_buflifetime: declared in the same scope asfatal_reason, live across the synchronoustrigger_handshake_callback; both consumers (UpgradedDuplex::on_handshakeandSocketBody::on_handshake) clone into ownedCertError/StoredVerifyErrorbefore returning.ERR_peek_error()reads the oldest queue entry, matching openssl.c:1627 and Node'scrypto_tls.cc; both engines now agree on the exact code and the tests assert it.
Extended reasoning...
Overview
The PR fixes SSLWrapper::update_handshake_state in src/uws/lib.rs so that when SSL_do_handshake fails with SSL_ERROR_SSL/SSL_ERROR_SYSCALL, the queued BoringSSL error reason is captured via ERR_peek_error() before ERR_clear_error() discards it, and passed to the handshake callback as an EPROTO-shaped us_bun_verify_error_t. Without this the duplex/named-pipe TLS path reported a phantom UNABLE_TO_GET_ISSUER_CERT (from SSL_get_verify_result on a peer that never sent a cert) or, with rejectUnauthorized: false, fell through to a spurious secureConnect. Two new tests in node-tls-connect.test.ts cover both an ALPN-mismatch alert and a raw handshake_failure alert, in both the native and duplex-proxy variants of the existing test matrix.
Security risks
This is TLS handshake code, so security-adjacent by nature. The change strictly improves error reporting on an already-failed handshake — no verification decision is altered, and the secureConnect-on-a-dead-session case (which was the security-relevant symptom) is closed. I don't see a way for this to weaken any check: the new branch only fires when SSL_get_error has already returned a fatal code, and it substitutes the real BoringSSL reason for a meaningless verify-result on a nonexistent certificate. Still, native TLS handshake handling is exactly the kind of code the approval guidelines flag for human review.
Level of scrutiny
High. Native FFI, BoringSSL error-queue semantics, and a raw-pointer (reason) into a stack buffer that must outlive a callback that runs JS. I traced the buffer's lifetime through both consumers of the SSLWrapper handshake callback (UpgradedDuplex::on_handshake at src/runtime/socket/UpgradedDuplex.rs:167-183 and SocketBody::on_handshake at src/runtime/socket/socket_body.rs:1695-1705) and confirmed both clone the code/reason bytes into owned storage synchronously before the stack frame unwinds — the same contract the C path's stack-local reason[] in ssl_dispatch_parked_reason (openssl.c:1652-1658) already relies on, and which the existing verify_error field comment ("EPROTO reasons are stack-copied in uSockets") documents. I also confirmed ERR_error_string_n NUL-terminates into the zero-initialized 256-byte buffer, so CStr::from_ptr on the consumer side is sound.
Other factors
My earlier review flagged that the helper originally read ERR_peek_last_error() (newest entry) instead of ERR_peek_error() (oldest / root cause), diverging from ssl_park_fatal_reason and Node. That was fixed in 40f57c2, the test's regex was tightened to an exact-code assertion, and the divergence comment removed. The comment-cop bot's complaints about long comments were also addressed (the doc comment is now a one-line cross-reference). All prior inline threads are resolved. Given it's native TLS code I'm deferring rather than approving, but I have no remaining concerns of my own.
2 participants
When
tls.connect()is handed a genericDuplexviaoptions.socket(or a Windows named pipe, or since #34598 anet.Socketwith unflushed writes), the handshake runs throughSSLWrapperinsrc/uws/lib.rsinstead of the uSockets C path. On a fatal handshake failure that wrapper calledERR_clear_error()immediately afterSSL_get_error(), discarding the alert reason BoringSSL had queued, and then built the handshake error out ofSSL_get_verify_result()of a certificate the peer never sent.This has two user-visible failure modes depending on the client's verification policy:
UNABLE_TO_GET_ISSUER_CERT(or falls through toERR_TLS_CERT_ALTNAME_INVALIDagainst an empty cert) instead of the real alert. Retry/fallback logic keyed one.codegoes chasing a certificate issue that does not exist.rejectUnauthorized: false, the phantom X509 code does not matchnet.ts's protocol-failure check, so the handshake handler falls through toonClientHandshakeCompleteand the socket emitssecureConnecton a TLS layer that was never established.getCipher()returns all-null.Repro (secureConnect case)
error: ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILUREsecureConnecterror: ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILUREThe direct-TCP path (
tls.connect({host, port})) was already correct:ssl_park_fatal_reasoninopenssl.cpeeks the queued reason andssl_dispatch_parked_reasonemits it asEPROTObefore anything touches the verify result.SSLWrapperhad no equivalent.Fix
In
SSLWrapper::update_handshake_state, captureERR_peek_error()before theERR_clear_error()whenSSL_get_errorreportedSSL_ERROR_SSL/SSL_ERROR_SYSCALL, and pass it to the handshake callback as the sameEPROTOshape the C path's parked-reason dispatch uses.net.tsalready recognizes that shape as a protocol failure, decomposes it into theERR_SSL_*code, and never reaches the identity check or thesecureConnectemission.ERR_peek_error()reads the oldest queue entry, matchingssl_park_fatal_reasonand Node'scrypto_tls.cc. For ahandshake_failurealert BoringSSL pushes the alert reason first and aHANDSHAKE_FAILURE_ON_CLIENT_HELLOwrapper on top; reading the oldest gives the root cause, so the native-socket and duplex paths now agree onERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE.The
reasonpointer refers to a stack buffer that is live for the duration of the synchronous callback, which clones the string immediately; this matches the C path's stack-localreason[]inssl_dispatch_parked_reason.Tests
Two new cases in the
tls.connect/tls.connect using duplex proxymatrix intest/js/node/tls/node-tls-connect.test.ts:no_application_protocolalert) with verification on: asserts the exactERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOLcode and thatcheckServerIdentitynever runs.handshake_failurealert withrejectUnauthorized: false: assertserror(notsecureConnect) with the exactERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILUREcode.Both duplex-proxy variants fail on the unfixed build:
and pass with the fix. The direct
tls.connectvariants pass either way (covering the unaffected C path), and both variants now assert the same exact code.