tls: report a fatal TLS alert received after the handshake completes - #33294
tls: report a fatal TLS alert received after the handshake completes#33294robobun wants to merge 3 commits into
Conversation
|
Updated 4:32 PM PT - Jul 9th, 2026
❌ @robobun, your commit 8b2182b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 33294That installs a local version of the PR into your bun-33294 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
WalkthroughAdds fatal post-handshake TLS error dispatch from OpenSSL through Rust into JS error handling. The new path reports these errors immediately, normalizes them in ChangesPost-handshake TLS error reporting
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
I checked both before adding the #20727 is a fatal alert during the handshake ( The handshake-time capture landed in #31155, well after the 1.2.17 that issue was filed against. This PR covers the post-handshake variant, which is adjacent but not what #20727 describes, and that issue also runs through #27985 is Identical with and without the change, because the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
CI statusBuild #71179 on the rebased head This PR's two test files ran on every What failed, and why it is not mine:
The proxy stress test is No retrigger. Both remaining failures are hitting ~30% of builds across branches right now; a re-roll would reproduce them. The diff is green and ready for review. Local verification at
|
us_internal_ssl_on_data parked the OpenSSL reason for a fatal SSL error only while the handshake was still pending, so an alert that arrives afterwards was dropped and the connection closed with no 'error' event. Under TLS 1.3 that is exactly where a server's mTLS rejection lands, because the client finishes its handshake one flight before the server has validated the client certificate, which left an authentication failure indistinguishable from a clean end-of-connection. Dispatch the reason to the socket's JS error handler before the close, the way Node's TLSWrap onerror does, and shape it into the ERR_SSL_* error in node:net. Covers the client (certificate_required) and accepted server sockets (bad record MAC).
The test bound the listener to "localhost" and then connected to "localhost" again, assuming both resolve to the same family. On a dual-stack host listen() picks ::1 while connect() picks 127.0.0.1 and the connection is refused before the SNICallback can run. Dial the address the listener reports and pass the bind hostname through `servername`, which is the property the test is actually asserting.
The SSL_read loop accumulates decrypted bytes across iterations, and the fatal-error exit was the only one that never handed them to the consumer: a peer that writes application data and the failing record into one segment lost the data. The ZERO_RETURN sibling right above, and Node's ClearOut, both dispatch what was decrypted before reporting.
92a3217 to
8b2182b
Compare
…alert The 'Fail to complete client's chain' comment described the pre-change behaviour (Bun aborts without an alert). The server now sends the fatal alert like Node; fetch still surfaces ECONNRESET over TLS 1.3 because the alert arrives post-handshake (#33294), so the assertion is unchanged.
…alert The 'Fail to complete client's chain' comment described the pre-change behaviour (Bun aborts without an alert). The server now sends the fatal alert like Node; fetch still surfaces ECONNRESET over TLS 1.3 because the alert arrives post-handshake (#33294), so the assertion is unchanged.
1 participant
A fatal TLS alert that arrives after the handshake has completed is silently dropped: the connection ends with a clean close and no
'error'event. Under TLS 1.3 that is exactly where a server's mTLS rejection lands, because the client finishes its handshake one flight before the server has even looked at the client certificate. A client rejected for missing or invalid credentials cannot tell that apart from an ordinary end-of-connection, so retry/failover logic keyed on why the connection ended sees flaky networking instead of "you are not authenticated".Repro
The same hole exists on an accepted server socket: a record that fails to authenticate after the handshake completed closes the connection with no
'error', where Node reportsERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC.Cause
On a fatal
SSL_ERROR_SSLinsideus_internal_ssl_on_data,ssl_park_fatal_reasoncaptures the OpenSSL reason only while the handshake is still pending, because the handshake-failure dispatch is the only thing that reads it. Once the handshake has completed there is nothing left to route the reason to, so theERR_clear_error()inside the helper throws awayTLSV1_ALERT_CERTIFICATE_REQUIREDandssl_closereports an ordinary disconnect. Node routes the same condition throughTLSWrap::ClearOutto the wrap'sonerror.Fix
openssl.c: when the handshake has already completed and the SSL error queue holds a reason, capture it on the stack beforessl_park_fatal_reasonclears the queue, build theEPROTOverify error, and hand it to a newus_dispatch_ssl_errorbefore closing. The JS handler may destroy the socket, so liveness is rechecked after the dispatch. The branch now also hands over any plaintext the sameSSL_readloop already decrypted, because a peer can put application data and the failing record in one segment; theZERO_RETURNsibling right above, andClearOut, both do this.uws_dispatch.rs/socket_body.rs: the dispatch only reachesbun_socket_tlssockets (same shape asus_dispatch_session/us_dispatch_keylog) and calls the socket'serrorhandler with the decomposed error.net.ts: a post-handshakeEPROTOis reshaped by the existingtlsHandshakeError()into theERR_SSL_*error and emitted rather than destroying the socket, mirroring Node'sonerror, so the close that follows still delivers'end'and'close'.bun_socket_tlsis also the kind rawBun.connect({ tls })/Bun.listen({ tls })sockets carry, and theirerrorhandler is optional. A raw-API user who never registered one now gets anuncaughtExceptionfor a post-handshake fatal alert where they previously saw onlyclose(). That escalation is intentional: it is the same thing that happens today when theirdatacallback throws, it matches the EventEmitter unhandled-'error'contract, and suppressing it when no handler is installed would reintroduce, for exactly those users, the silent drop this PR exists to remove.After the fix, the event sequence matches Node exactly:
ERR_SSL_TLSV1_ALERT_CERTIFICATE_REQUIREDrather than Node'sERR_SSL_TLSV13_ALERT_CERTIFICATE_REQUIRED: BoringSSL names alert 116TLSV1_ALERT_CERTIFICATE_REQUIRED, and the rest of Bun's TLS error codes already follow BoringSSL's spelling (seetest/js/node/test/common/boringssl.jsand the existingERR_SSL_TLSV1_ALERT_PROTOCOL_VERSIONexpectations).node:httpsrides onnode:tls, so it picks this up too:Deliberately not covered here
Two sibling sites share the pattern and are left alone on purpose:
tls.connect({ socket })with a genericDuplex(and Windows named pipes) drives the handshake throughSSLWrapperinsrc/uws/lib.rsrather thanus_internal_ssl_on_data, and swallows the same alert. Wrapping a realnet.Socketis covered, since that upgrades the native socket in place.fetch()runs onSocketKind::HttpClientTls, whichus_dispatch_ssl_errorskips, so it still reportsECONNRESET. Bun's HTTP client has its own error model and no Node differential to match here.Closing the first means adding an error channel to
ssl_wrapper::Handlers, which every consumer constructs (UpgradedDuplex,WindowsNamedPipe,WindowsNamedPipeContext,ProxyTunnel,WebSocketProxyTunnel,HTTPContext), pulling the fetch and WebSocket tunnels into the blast radius. #32929 is already open against the sameERR_clear_error()sites in that file for the handshake-time variant, so it belongs there or in a follow-up rather than here.Verification
Two tests, both failing on
mainand passing with this change:node-tls-connect.test.ts- "reports the server's fatal alert rejecting a missing client certificate": asserts the fullsecureConnect/error/end/close(hadError=false)sequence plus the error'scode,libraryandreason.node-tls-server.test.ts- "reports a corrupted record on the accepted socket instead of closing cleanly": injects an unauthenticatableapplication_datarecord after the server has written its first bytes (so the handshake has provably completed) and assertsERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC.node-tls-server.test.ts- "delivers data decrypted alongside a fatal record before reporting the error": splices a corrupt record onto a real client record through a plain TCP relay, so both reach the server in onerecv()and theSSL_readloop decrypts the first before the second fails. Asserts["data:ping", "error:ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC"], in that order.The second commit fixes an unrelated hermeticity bug in a neighbouring test in the same file. "SNICallback runs even when the requested servername matches the bind hostname" binds the listener to
localhostand then connects tolocalhostagain, assuming both resolve to the same family. On a host that prefers IPv6 they do not, and the test fails before the callback can run:It now dials the address the listener reports and passes the bind hostname through
servername, which is the property it is actually asserting. Same assertions, no resolver dependency.Rebase notes
ssl_park_fatal_reason. The post-handshake branch now peeks the error queue before the helper clears it, and dispatches afterwards; the helper still handles the handshake-pending side unchanged.Handlersinto a GC-visited cell and the dispatch entry points toThisPtr<Self>.on_ssl_errorandus_dispatch_ssl_errorare written against that API now:ThisPtr,has_handlers(),handlers.enter()/exit_scope(scope), same shape ason_session.All three of this PR's tests were re-verified after the rebase: they fail with
src/+packages/reverted tomainand pass with the change. 151/151 nodetest-tls-*.jsports still pass.Suites run against the change (all deltas vs
mainare zero)test-tls-*.jsNode ports: 151/151 passtest-https-*.jsNode ports: 44 pass, 1 pre-existing hang (test-https-timeout.js, hangs onmaintoo under debug+ASAN)test/js/node/tls/,test/js/node/net/,test/js/node/http2/,test/js/bun/net/,test/js/web/fetch/: failure sets identical tomain(checked by stashingsrc/+packages/and rebuilding)