Repository navigation
tls: report the handshake's own failure when an early write drives it - #33390
Conversation
SSL_write drives the handshake while it is unfinished, so a caller that writes before 'secureConnect' - every https.request, which flushes its headers as soon as the socket connects - hits the fatal handshake error from inside us_internal_ssl_write. That path drained the OpenSSL error queue without parking the reason, leaving the handshake dispatch with nothing to report: node:tls saw a null verifyError, treated the dead session as established, and fell through to checkServerIdentity against an empty peer certificate. Park the reason there the way the handshake and read paths already do, extracting the shared helper, and drop a reason still owned by a socket being detached.
WalkthroughRefactors OpenSSL fatal error handling in bun-usockets by extracting a shared ChangesSSL Fatal Reason Parking Refactor
Sequence Diagram(s)sequenceDiagram
participant ComponentA
participant ComponentB
ComponentA->>ComponentB: observable interaction
Related Issues: None found. Related PRs: None found. Suggested labels: bun-usockets, tls, tests Suggested reviewers: None found. 🥕 A rabbit hops through sockets deep, 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:15 PM PT - Jul 5th, 2026
❌ @robobun, your commit 72dcc8d has some failures in 🧪 To try this PR locally: bunx bun-pr 33390That installs a local version of the PR into your bun-33390 --bun |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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, but this touches the native TLS handshake error path and shared per-loop fatal-error state in openssl.c, so it's worth a human look.
Extended reasoning...
Overview
This PR fixes error misclassification in Bun's TLS layer: when a caller writes to a TLS socket before secureConnect (as https.request does), a handshake failure discovered inside SSL_write was having its OpenSSL reason dropped via ERR_clear_error(), causing the subsequent handshake dispatch to fall through to an empty X509 verdict and ultimately surface as ERR_TLS_CERT_ALTNAME_INVALID instead of the real ERR_SSL_NO_SUPPORTED_VERSIONS_ENABLED.
The fix extracts the existing park-reason logic (previously duplicated between the handshake and read paths) into ssl_park_fatal_reason() and calls it from the write path as well. It also adds cleanup of the per-loop parked reason in us_internal_ssl_detach() so a new socket reusing the same address can't inherit a stale reason. Two new tests in node-tls-connect.test.ts cover both the mechanism (early write on tls.connect) and the reported symptom (https.request).
Security risks
The change is in packages/bun-usockets/src/crypto/openssl.c — the TLS handshake state machine. It only affects which error string is reported when the handshake has already fatally failed; it does not change whether a handshake succeeds, how certificates are verified, or what data flows. The parked-reason slot is per-loop shared state keyed by socket pointer, and the PR's detach cleanup and the helper's HANDSHAKE_COMPLETED guard are specifically about preventing cross-socket misattribution. I don't see a way this weakens security, but the shared-state ownership model here is subtle enough that a reviewer familiar with this file should confirm.
Level of scrutiny
TLS handshake error handling in native C, with per-loop shared mutable state (ssl_last_fatal_error / ssl_last_fatal_error_owner) that multiple sockets on the same loop touch. The extraction is behavior-preserving for the two existing call sites (the ssl_update_handshake site already guarantees HANDSHAKE_PENDING at line 1677 before reaching the park, so the added != HANDSHAKE_COMPLETED guard is a no-op there; the read path already had the guard). The write-path addition is the actual fix. This deserves human review given the security-sensitive area and the cross-socket state involved.
Other factors
The PR description is thorough, the root cause is well-explained, and verification covers the relevant node parallel tests plus Bun's own TLS/HTTP suites. The bug-hunting system found no issues. No CODEOWNERS entry covers this path. CI (#68637) was still building at review time.
|
Writing out the parked-reason ownership model, since that's the part worth a careful read. The invariant: a parked reason is consumed or cleared before its socket can be reused, and is never visible to another socket.
On the extraction being behavior-preserving: at the CI (build #68637, still running): one failed job, |
There was a problem hiding this comment.
I didn't find any issues — the extraction is behavior-preserving at the two existing sites and the detach cleanup mirrors ssl_release_spill — but this touches native TLS handshake error-handling with per-loop shared state, so it's worth a human look at the parked-reason ownership invariants.
Extended reasoning...
Overview
This PR fixes misclassified TLS handshake errors when a write is issued before secureConnect (the https.request path). It touches packages/bun-usockets/src/crypto/openssl.c:
- Extracts the duplicated "park the OpenSSL fatal reason" block into
ssl_park_fatal_reason()and calls it from the handshake, read, and (newly) write paths. - Clears a still-parked reason in
us_internal_ssl_detach()so a socket reusing the same address can't inherit it, mirroring the adjacentssl_release_spill()pattern. - Adds two hermetic tests in
node-tls-connect.test.ts(local server, port 0) covering both the mechanism (tls.connect+ early write) and the user-facing symptom (https.requestwith an impossible version window).
Security risks
The change affects which error is reported when the handshake has already failed — it does not touch verification logic, and ssl_fatal_error = 1 was already being set at the write site before this change. I don't see a way for this to weaken TLS validation. The main correctness risk is the per-loop shared ssl_last_fatal_error / ssl_last_fatal_error_owner state: a reason parked by one socket must never be reported for another. The helper's HANDSHAKE_COMPLETED guard, the owner-gated dispatch (which zeroes the slot before running JS), the relocation hook, and the new detach clear all cooperate to maintain that — the author's four-point invariant writeup in the PR thread matches what I read in the code.
Level of scrutiny
Native C in the TLS handshake path with loop-wide shared state and subtle ownership invariants across sockets warrants human review even for a small, well-reasoned diff. The refactor itself is behavior-preserving (the added HANDSHAKE_COMPLETED guard is a no-op at the ssl_update_handshake site since that function only runs while HANDSHAKE_PENDING, and the read path already had the guard verbatim), so the only real behavioral delta is at us_internal_ssl_write plus the defensive detach clear.
Other factors
The bug-hunting system found nothing. Tests follow repo conventions (await using, once(), exact error-code assertions, positive control that a satisfiable version window still succeeds). CI on the earlier build had one unrelated worker_threads ASAN failure and a Windows flake that passed on retry; the current build (#68662) is still running.
CI statusBoth builds are in, and nothing red has touched this diff. I'm not going to keep re-triggering, so here is the picture for whoever picks this up. Build #68662 (re-run, complete): 284 of 287 jobs passed, and not a single test failed. The two red jobs are both The build carries no test-failure annotation at all. The one Build #68637 (first run, 283 jobs passed). Every failure was on a lane this change cannot reach — and every one of them passed on the re-run above, which is the clearest evidence they were flake:
The change is ~40 lines confined to the SSL paths of Locally, each suite above was run against a build of the base commit and against the same build with the patch applied; the failure sets were identical ( |
…faces (+24 tests, 155→179 of 221 upstream passing) (#32630) ## What this does Syncs Bun's vendored `node:tls` test suite to Node v26.3.0 and fixes the compatibility gaps the upstream suite surfaced. **Tests added: 24 upstream `node:tls` test files newly vendored (all passing), 34 existing vendored files updated to their verbatim v26.3.0 content, plus 29 new Bun-side regression tests for the review fixes.** Coverage of Node v26.3.0's own `test-tls*` suite, before → after: | upstream suite (Node v26.3.0) | upstream files | before (main) | after (this PR) | | --- | --- | --- | --- | | `test/parallel/test-tls*` | 217 | 151 vendored & passing (**69.6%**) | 175 (**80.6%**) | | `test/sequential/test-tls*` | 4 | 4 (**100%**) | 4 (**100%**) | | **total** | **221** | **155 (70.1%)** | **179 (81.0%)** | (The 4 sequential files were already all vendored; they are updated to their v26.3.0 content here. ~13 of the missing parallel files structurally require `--expose-internals` and can never pass unmodified, so 179/221 is ~85% of the reachable maximum.) ## Runtime fixes - **net**: accepted sockets are no longer force-resumed into flowing mode, so bytes that arrive before a `'data'` listener (or before a delayed `new tls.TLSSocket(socket, { isServer: true })` wrap) are buffered like Node instead of being emitted with no listener; the EOF path now triggers `'end'` itself (`push(null)` + `read(0)`, like Node's `onStreamRead`) so the `allowHalfOpen: false` write-side teardown still runs without a consumer; sockets whose readable side was never engaged are torn down at EOF so abandoned connections don't hold the event loop; `onread.callback` receives the user's buffer (or factory result) with the chunk copied in, sliced to its length; a factory that has not yet produced a `Uint8Array` gets Node's literal `true` (and its `false` return still pauses); a zero-length `onread` buffer destroys the socket with `read ENOBUFS` (libuv's "user can't handle the read" contract) instead of looping; and a callback that throws mid-chunk fails the socket closed instead of leaving a silent gap. - **rejectUnauthorized**: only a literal `false` disables peer verification, matching Node's `options.rejectUnauthorized !== false` — `null`/`0`/`''` now keep verification on (previously `null` silently disabled it), and non-boolean values no longer throw `ERR_INVALID_ARG_TYPE` out of the native option converter (Node never type-checks the option). - **tls.Server**: plain sockets handed in via `server.emit('connection', socket)` are upgraded (STARTTLS pattern); `pauseOnConnect` no longer stalls the handshake and is observable from the `'secureConnection'` listener; `honorCipherOrder`/`secureOptions` now reach the SSL context; `allowPartialTrustChain` is forwarded to the native context; `key` entries given as `{ pem, passphrase }` and `sessionTimeout: null` are normalized on the server path the same way `createSecureContext()` normalizes them. (Note: Node v26.3.0's own `tls.Server` currently *drops* `allowPartialTrustChain` — `Server.setSecureContext`'s option whitelist in `lib/internal/tls/wrap.js` predates the option — so Bun follows Node's documented contract here, verified against a built v26.3.0 binary; everything else in this PR was verified to match the binary's behavior exactly.) - **TLSSocket**: server-side wraps adopt the fd one tick after construction so handshake failures driven by already-buffered bytes reach listeners attached right after the constructor, and a standalone wrap that fails its handshake emits `'error'`; a bare `new tls.TLSSocket()` only rejects unauthorized peers when asked (Node's `_init` semantics) while `tls.connect()` keeps the secure default; `getCertificate()` returns `{}` with no local cert; `setMaxSendFragment()` validates its argument; `authorizationError` is `null` after a clean client-cert verification; `socket.ssl` provides `verifyError()` and `fd` like Node's TLSWrap. - **tls.connect()**: defaults `ciphers` to `tls.DEFAULT_CIPHERS` so a runtime assignment to `tls.DEFAULT_CIPHERS` is observed by the next connect that omits the option. - **SecureContext options**: implements `crl` (`PEM_read_bio_X509_CRL` → `X509_STORE_add_crl` + `X509_V_FLAG_CRL_CHECK`, matching Node's `SecureContext::AddCRL`), `sessionTimeout` (`SSL_CTX_set_timeout`), `allowPartialTrustChain` (`X509_V_FLAG_PARTIAL_CHAIN`), and `sigalgs` (`SSL_CTX_set1_sigalgs_list`). All four flow through the full SSLConfig stack and participate in the SSL context cache key. A context still sharing the process-wide default root store gets its own copy before any of these mutate it (the same copy-on-write as Node's `SecureContext::GetCertStoreOwnedByThisSecureContext`), so a `crl`/`allowPartialTrustChain` context can never poison default-CA verification for the rest of the process. CRL material is also counted in `SecureContext.memoryCost`. - **Client rejection order**: a `rejectUnauthorized: true` client is destroyed with the verification error before `secureConnect` fires, matching Node's `onConnectSecure` (https://github.com/nodejs/node/blob/v26.3.0/lib/internal/tls/wrap.js#L1674-L1687). Exactly like Node, the native handshake itself is never aborted by the verify callback — Node's clients use `SSL_VERIFY_NONE` and "reject in javascript" (https://github.com/nodejs/node/blob/v26.3.0/src/crypto/crypto_tls.cc#L1236-L1244, #L84-L95). An earlier revision's native `setVerifyStrict()` was removed after review: it was inert (BoringSSL's `SSL_set_verify` keeps the previously-installed callback when handed NULL), and had it worked it would have diverged from Node. - **Certificate chain**: BoringSSL's handshake-time auto-chain is enabled the way Node enables OpenSSL's (`SSL_CTX_clear_mode(SSL_MODE_NO_AUTO_CHAIN)`), and a context with no user `ca` gets the shared default root store like Node's `addRootCerts()`. A leaf-only `cert` therefore presents the intermediate found via `ca`, `caFile`, `pfx` extras, `addCACert`, `NODE_EXTRA_CA_CERTS`, or the system roots — and only the leaf's actual issuer path is sent; `ca` entries unrelated to it are never presented. - **Cipher handling**: cipher-string validation mirrors `SSL_CTX_set_cipher_list` (only fails when nothing can match) and is grounded in BoringSSL's cipher table; TLS 1.3 suite names are stripped before reaching the native cipher list; `ecdhCurve` strings BoringSSL cannot set are rejected like Node. - **OpenSSL layer**: handshake failures report the oldest (root) error-queue entry like Node; a missing key passphrase now fails with `BAD_DECRYPT` instead of `BAD_PASSWORD_READ`. - **setDefaultCACertificates()**: matches Node's error contract (skips non-PEM elements, `ERR_CRYPTO_OPERATION_FAILED` when nothing parses, BoringSSL's PEM error code for undecodable blocks). - **CLI**: registers `--tls-min-v*`/`--tls-max-v*` and refuses `--tls-min-v1.3` with `--tls-max-v1.2` at parse time, matching Node's `node_options.cc` check. - **opensslCli detection**: reports `false` (not `null`) when no usable OpenSSL CLI is present, and treats LibreSSL as unusable since the upstream tests assume the OpenSSL CLI's option syntax. ## Test changes - 24 upstream tls tests newly vendored, 34 existing vendored tls tests updated to their verbatim v26.3.0 versions (plus `common/crypto.js`). - 8 tests keep the copies main already runs because their v26 versions need features Bun does not implement yet (OCSP stapling, server session-cache callbacks, `openssl s_client` interop that requires close_notify on `end()`, SSL record-error surfacing). - 29 new Bun-side regression tests in `test/js/node/tls/node-tls-{server,connect,context}.test.ts`, `test/js/node/tls/fetch-tls-cert.test.ts`, `test/js/node/tls/ssl-ctx-cache.test.ts` and `test/js/node/net/node-net.test.ts` covering: the zero-length `onread` buffer and the paused-`onread` tail redelivery, `rejectUnauthorized: null/0/''/false` (with a verifiable-client control), server-side `allowPartialTrustChain` (boolean and truthy non-boolean), `key: [{ pem }]`/`[{ pem, passphrase }]` and `sessionTimeout: null` on `tls.createServer`, the presented certificate chain excluding unrelated `ca` entries (inline `ca` and `certFile`/`caFile`), mTLS over a STARTTLS wrap (via `tls.Server` and via a bare `new TLSSocket(socket, { isServer: true })`), `TLSSocket._requestCert` defaults (Node's `_init` table), a throwing `setSecureContext()` not corrupting the STARTTLS credentials, `fetch()` surfacing `InvalidCRL` and honoring a lone `tls.sigalgs`, the assignable `socket.ssl` property, and a subprocess proving a `crl` server cannot poison the process-wide default CA store. ## Notes for reviewers **Known red check: `robobun/evidence` ("diff hygiene check found TODO/FIXME markers in added lines").** The three flagged markers are upstream Node comments inside test files this PR vendors verbatim from v26.3.0 (`test-tls-client-getephemeralkeyinfo.js`, `test-tls-env-bad-extra-ca.js`, `test-tls-ticket.js`); they are Node's own action items, not Bun's. 32 vendored test files already in-tree carry the same markers — they simply are not "added lines" in their PRs' diffs. Rewording them was tried and reverted: vendored upstream files have to stay byte-identical to be diffable against Node. Needs a gate override or a vendored-path exclusion. All review comments are addressed (the later rounds land as individual commits on top so they can be re-reviewed in isolation): - zero-length `onread` buffer: fixed as `read ENOBUFS` + destroy, which is what Node/libuv actually do (Node performs no up-front length validation, so the suggested `ERR_INVALID_ARG_VALUE` would itself be a divergence). - shared default CA store mutated by `crl`/`allowPartialTrustChain`: fixed with the same clone-on-write `us_ssl_ctx_add_ca_cert` already used, extracted into a shared helper. - presented chain built from every `ca` entry: replaced with an `X509_verify_cert` issuer-path walk. - `allowPartialTrustChain` dropped on the `tls.Server` path: fixed as suggested, with one caveat the review (and I) initially missed — running the scenario against a built Node v26.3.0 shows Node's `tls.Server` *also* drops the option (its `setSecureContext` whitelist predates it), so this makes Bun match Node's documented contract, not Node's literal behavior. - `{ pem, passphrase }` keys / `sessionTimeout: null` bypassing normalization on the `tls.Server` path, `crl` missing from `approx_cert_bytes()`, primordials in the new parsing paths: all fixed as suggested (`setVerifyStrict`, whose arity was also flagged, was later removed outright — see above). - `rejectUnauthorized: null` (pre-existing divergence): fixed at its real source in `net.ts`/`tls.connect()`/`tls.Server` rather than only at the flagged line. - From the later review rounds: a `ca` that parses to zero certificates (e.g. a key PEM, tolerated like Node) is now treated as an intentionally-empty pin set instead of being silently replaced with the default roots — node fails such a connection closed and Bun previously verified it against the default roots (a fail-open; regression-tested via a child process whose default roots *can* verify the chain); `socket.ssl.verifyError()` is populated for server-side sockets (Node's own server path reads the same `TLSWrap.verifyError()`); a `tls.Server` no longer emits `tlsClientError` for an unverifiable client it *accepts* (`rejectUnauthorized: false`) — Node only surfaces the failure when rejecting (on the rejecting path Bun's `tlsClientError` carries the deterministic verify code where node surfaces an incidental `ECONNRESET` from its silent destroy; left as the more useful shape); `onread` no longer invokes the callback for data that races the pause (nothing is delivered between a `false` return and `resume()`, per Node's `readStop` contract); invalid `ecdhCurve` errors carry Node's `code: 'ERR_CRYPTO_OPERATION_FAILED'` without changing `String(err)`; and `sigalgs` is validated on every secure context (`createSecureContext`/`connect`/`addContext`), not only on `tls.Server`. - From the second-pass review: `setSecureContext` now stages every option and commits after the last validator, so a call that throws late cannot leave the STARTTLS wrap serving the rejected key/cert (and it stashes a snapshot, not the caller's live object); an `onread` callback holding the `true` sentinel can pause, and one that throws destroys the socket instead of leaving a byte gap; the eager auto-chain walk was replaced with BoringSSL's own handshake-time auto-chain plus Node's default-root seeding (which also makes `NODE_EXTRA_CA_CERTS` intermediates work and de-duplicates `addCACert`); the hand-mirrored BoringSSL group/cipher tables are pinned by a test and called out in the upgrade doc. - From the follow-up review: a context given `crl`/`allowPartialTrustChain` with no `ca` now seeds its own private copy of the default roots and keeps it through socket attach, so those options are honored exactly like Node instead of being silently dropped (`tls.connect({ crl })` with no `ca` fails closed with `UNABLE_TO_GET_CRL` in both runtimes — regression-tested); `fetch()` maps an invalid `crl` through its own `InitError` arm; the `socket.ssl` accessor accepts assignment like Node's plain property; and a `fetch()` whose only TLS option is `sigalgs` now builds its own SSL context instead of dropping the option (regression-tested via a deliberate no-common-sigalgs handshake failure). - The commit also carries autofix.ci's two-file docs formatting fixup (`docs/guides/util/base64.mdx`, `docs/runtime/web-apis.mdx`): current `main` has formatting drift in those files, so the bot re-adds that fixup to every push of this branch; folding it in keeps the branch at a single commit and leaves the bot nothing to do. ## Not covered yet (follow-ups) ### Blocked by BoringSSL public-API limitations - **TLS-1.3 verify-data APIs** (`tlsSocket.getFinished()` / `getPeerFinished()`): BoringSSL's `SSL_get_finished`/`SSL_get_peer_finished` return 0 for TLS 1.3 (`vendor/boringssl/ssl/ssl_lib.cc:1558`), so the verify data those tests assert has no public source. **Correction**: an earlier revision of this section also claimed Bun's TLS-1.3 *server* completed its handshake one flight early (before the client's Finished). Checking the vendored source and probing both runtimes shows that is not the case — BoringSSL's server handshake only returns early when 0-RTT early data is accepted (`vendor/boringssl/ssl/tls13_server.cc#L1247`), which Bun does not enable, `SSL_CB_HANDSHAKE_DONE` is a public completion signal regardless, and node and Bun order `client secureConnect → server secureConnection` identically over loopback. - **close_notify on `end()`** (4+ tests): BoringSSL has no TLS half-close — once a shutdown is initiated, further I/O on that `SSL` fails with `SSL_R_PROTOCOL_IS_SHUTDOWN` (re-verified in the vendored source). Bun therefore sends a TCP FIN instead of close_notify on `.end()` so the documented write+end-then-read pattern keeps working (see the comment in `us_internal_ssl_shutdown`); sending close_notify there would require BoringSSL-level half-close support. `openssl s_client` treats the missing close_notify as truncation. ### Known gap (follow-up): per-SNI contexts on the STARTTLS wrap path - `server.addContext()` certificates — and even an explicit `SNICallback` — are not consulted for sockets handed to a `tls.Server` via `server.emit('connection', socket)`: the wrapped socket serves the base context's certificate, while node serves the per-SNI one (dual-runtime probe, both forms). The cause is native, not a missing option forward: Bun's JS-SNI dispatch hangs off BoringSSL's `select_certificate_cb`, which is `SSL_CTX`-level and registered on each *listener's* own context, and an adopted (STARTTLS) socket runs on the user's shared, deliberately callback-free `SecureContext` context — so the ClientHello's `server_name` never reaches JS there. Honoring it needs per-socket SNI dispatch for adopted sockets (BoringSSL's per-SSL `SSL_set_cert_cb` plus the listener path's pause/resume), a self-contained native follow-up. The normal `tls.Server` accept path honors `addContext()` and is covered by the vendored SNI tests. ### Environment note (not a Bun behavior gap) - `test/js/node/test/parallel/test-tls-get-ca-certificates-node-use-system-ca.js` only runs where system certificates exist (macOS) and is incompatible with a color-forcing environment: it interpolates a child's `-p` stdout into a second `-p` script, so `FORCE_COLOR` in the inherited env injects ANSI escapes into that script and breaks it — **node v26.3.0 fails it the same way under `FORCE_COLOR=1`**. Bun's behavior under test is correct (`NODE_USE_SYSTEM_CA=1` and `--use-system-ca` produce the same `tls.getCACertificates('default')`, verified against the node binary); this PR's only change to the file is its trailing newline. ### Known gaps closed after review (both were first listed here as follow-ups) - ~~**STARTTLS + mTLS**~~ — **now fixed in this PR.** Two real causes, both found by comparing against a built node v26.3.0: (1) a socket adopted by `upgradeTLS` never applied `requestCert`/`rejectUnauthorized` per socket (Node does it in `TLSWrap::SetVerifyMode`, https://github.com/nodejs/node/blob/v26.3.0/src/crypto/crypto_tls.cc#L1225-L1234; the shared `SecureContext` SSL_CTX is deliberately mode-neutral), and (2) `getPeerCertificate()` / `getEphemeralKeyInfo()` decided "server or client?" from the JS handlers' mode — which stays client-mode for adopted sockets — instead of the SSL's own handshake side (`SSL_is_server`), so an mTLS STARTTLS server reported `{}` for a client certificate it had in fact requested and verified. Both regression-tested (`tls.Server` STARTTLS wrap, and a direct `new TLSSocket(socket, { isServer: true, secureContext, requestCert: true })` whose secure context lacks `requestCert`); the dual-runtime probe now matches node on both scenarios. One nuance kept as-is: Node computes `socket.authorized` only for sockets owned by a `tls.Server`, so a standalone server wrap keeps the `_init` default (`false`) even after verifying the peer; Bun fills it in from the verify result (a strict superset of information). - ~~**`onread.callback()` returning `false` mid-chunk**~~ — **now fixed in this PR.** Node stops the *handle* (`readStop`) and loses nothing: its reads are bounded by the user buffer, so the bytes the callback has not consumed wait in the kernel until `readStart`. Bun's emulation has already read the whole native chunk, so it now keeps the undelivered tail and redelivers it — never re-entrantly — when the socket resumes, before the kernel flow restarts (and ahead of any data that raced the pause). Regression-tested (a 12-byte burst through a 4-byte `onread.buffer`, pausing after the first slice, still delivers `abcd,efgh,ijkl`); the dual-runtime probe shows the identical slice sequence in node v26.3.0. ### Substantial native features - **TLS over generic duplex streams** (9 tests) — requires reworking the `UpgradedDuplex.rs` TLS engine state machine. - **Server session-cache/ticket callbacks** (`newSession`/`resumeSession`, 2 tests) — requires async `SSL_CTX_sess_set_new_cb`/`get_cb` bridging with handshake suspension. - OCSP stapling, PSK server options, per-worker `--use-system-ca`, native default-CA-store override for `fetch()`, `https.Server.setSecureContext` (Bun's https uses `Bun.serve` directly rather than `tls.Server`). ### Structural - 13 upstream tests require `--expose-internals`/Node-internal modules (`internal/test/binding`, `internal/net`, `internal/timers`, etc.) and cannot pass unmodified. ### Prior art checked Electron also runs Node.js on BoringSSL; their `patches/node` and `patches/boringssl` were reviewed and contain no patches addressing the TLS-1.3 handshake-completion timing, `SSL_get_peer_finished` for TLS 1.3, close_notify half-close, TLS-over-duplex, or session-cache callback areas above — their BoringSSL-compat work is limited to cipher/algorithm availability gating and test relaxation. The gaps documented here have no existing third-party fix to borrow. ## How this was verified - **Against a built Node v26.3.0 binary**: the same script (11 scenarios over real sockets — the `rejectUnauthorized` matrix client- and server-side, a zero-length `onread` buffer, the paused-`onread` tail redelivery, the presented certificate chain with an unrelated `ca` entry, the CRL/shared-store isolation in a `NODE_EXTRA_CA_CERTS` child process, a `[{pem,passphrase}]` + `sessionTimeout: null` server, and the two STARTTLS mTLS wraps) was run under `node v26.3.0` and under this branch's `bun-debug`; **15 of 16 output lines are byte-identical** (down to `read ENOBUFS`). The one intentional difference is the server-side `allowPartialTrustChain` case described above, where Node's implementation drops the documented option. - All **330** vendored upstream `test-tls*`/`test-net*` files pass against a debug build of this revision, and the four Bun-side `node:tls`/`node:net` test files pass (135 tests, 13 of them new regression tests for the review fixes). - The two `rejectUnauthorized` fixes were additionally proven non-vacuous by reverting just those lines and watching their 4 regression tests fail. - Cross-target Rust compilation was verified on the previous revision via `bun run rust:check-all`; the Rust delta since is one field added to an existing accounting function. CI re-validates the full matrix. --- **Rebased onto current `main`** (post-#33390/#33388). Two fixes this branch carried were independently landed upstream in the meantime — #33390 (a handshake driven by an early write reports its own failure; `main`'s `ssl_park_fatal_reason()` helper is the same logic this branch had inline, so the rebase adopts the helper) and #33388 (the `upgradeTLS` open-handler UAF). Everything else is unchanged; `main`'s newly-added tls/net tests pass against this branch's changes. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 27 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/net/socket.test.ts test/js/node/net/node-net.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Alistair Smith <hi@alistair.sh> Co-authored-by: robobun <117481402+robobun@users.noreply.github.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…write like the read and handshake drivers (#37675) ### What `us_internal_ssl_write` was the one handshake-driving call in `openssl.c` (`SSL_write` runs the handshake while it is pending) that did not follow the `ssl_in_use` protocol `ssl_update_handshake` (around `SSL_do_handshake`) and `us_internal_ssl_on_data` (around `SSL_read`) use. This makes the write path consistent with them: - A write issued while `ssl_in_use` is set, i.e. from a callback that `SSL_read` / `SSL_do_handshake` on this same socket is currently running, is parked instead of calling `SSL_write`: it returns 0 and sets `ssl_write_wants_read`, exactly what a write issued before the handshake already gets on the `WANT_READ` path. The caller buffers, and the existing `ssl_retry_parked_write` dispatches writable once the handshake completes. - `SSL_write` itself now runs with `ssl_in_use` set and honors `ssl_pending_detach` in its epilogue, the same shape as the other two drivers (`SSL_write` drives the handshake while it is pending, so it is a driver too). ### Observable before / after The first point is reachable today. The server-side ALPN and SNI selection callbacks run from inside the `SSL_read` that is processing the ClientHello, and both hand the accepted socket to JS (`Bun.listen`'s `alpnCallback` / `serverName` hooks, which `node:tls`'s `ALPNCallback` / `SNICallback` are built on). A `socket.write()` there re-entered BoringSSL on the SSL it was still working on and the handshake failed: ```ts const server = tls.createServer({ key, cert, ALPNCallback({ protocols }) { conn.write("hi"); return protocols[0]; } }); server.on("connection", s => (conn = s)); // client: error:10000438:SSL routines:OPENSSL_internal:TLSV1_ALERT_INTERNAL_ERROR // server: tlsClientError error:100000c2:SSL routines:OPENSSL_internal:PROTOCOL_IS_SHUTDOWN ``` With this change the handshake completes and the bytes are delivered right after it, ahead of anything written from `secureConnection`. At the `Bun.listen` level the write reports 0 and `drain` fires once the handshake is done, which is the contract a pre-handshake write already has. (`socket.end("bye")` from inside the callback goes through the same path and now works as well.) The second point has no reachable trigger at the moment: `us_internal_ssl_write` zeroes the read-input window before calling `SSL_write`, so the ClientHello-driven callbacks cannot fire from inside it, the session and keylog callbacks are parked in ex_data and flushed after the SSL call returns, and `us_socket_raw_write` (what the BIO calls) never closes the socket. It is included so the write driver keeps the same invariants as the other two if that ever changes; if the deferred close did fire, the epilogue resets the batching state (so leftover records cannot leak into the next socket's batch), runs the close, and reports 0 like any write to a socket that is going away. `us_internal_ssl_shutdown` is intentionally left alone. It is reachable from the same callbacks (`socket.shutdown()` from inside the ALPN/SNI selection), but BoringSSL's `SSL_shutdown` returns 1 without touching any state while the handshake is in progress (`ssl_lib.cc`, "If we are in the middle of a handshake, silently succeed"), so the only effect is the TCP FIN and the connection the caller asked to shut down goes away; verified under the ASAN build from both callbacks. There is nothing to defer there, unlike a write, which has bytes to deliver after the handshake. ### Why this is the right place The guard has to live in `us_internal_ssl_write`: it is the only function that knows whether a BoringSSL call on this `SSL*` is on the stack, and every caller (`Bun.listen` sockets, `node:tls`, uWS) already treats 0 as "buffer and wait for writable", so parking needs no changes above it. Buffering in usockets or gating on the JS side would add a second buffer for a case the existing parked-write machinery already handles. It keys on `ssl_in_use` rather than on the handshake state because the two are different conditions: before the handshake completes, a write issued from outside any BoringSSL call is still meant to reach `SSL_write` (the https client flushes its headers on connect and that write drives the handshake; the tail of this function reports the handshake failure for it, #33390), so only a write issued while a BoringSSL call is actually on the stack is parked. ### Tests - `test/js/node/tls/node-tls-server.test.ts`: `write()` from inside `ALPNCallback` and from inside `SNICallback`; asserts the client receives the callback's bytes followed by the `secureConnection` bytes, the ALPN selection still applies, and no `tlsClientError` fires. Fails on main with the errors quoted above. - `test/js/bun/net/socket.test.ts`: the same through `Bun.listen`'s `alpnCallback` and `serverName` hooks, asserting the exact sequence `write:0` -> `handshake:true` -> `drain` -> `close` and that the data written from `drain` arrives. On main the sequence is `write:0` -> `handshake:false` -> `close`. Both pass 15/15 reruns with the debug (ASAN) build. Also ran `test/js/node/tls/`, `test/js/bun/net/socket.test.ts`, `tcp-server.test.ts`, `test/js/web/fetch/fetch.tls.test.ts`, `test/js/bun/http/bun-serve-ssl.test.ts` and `test/js/node/http2/node-http2.test.js` locally; the only failures are unrelated to this change (plain-TCP `localhost` listen/connect family mismatch in this container, reported separately, and a few subprocess-spawning tests that sit right at the 5s default timeout under the debug build here and pass with a longer timeout).
2 participants
What does this PR do?
Makes a TLS handshake failure report its own reason when the caller wrote to the socket before
'secureConnect', instead of falling through to a certificate error.Symptom. An impossible TLS version window (
minVersion > maxVersion) is reported byhttps.requestas a certificate problem, pointing at the cert instead of at the configuration that is actually broken.tls.connectwith the same options already reports it correctly.Node reports the configuration error on both (
ERR_SSL_NO_PROTOCOLS_AVAILABLE, OpenSSL's spelling of the same reason). Misclassified like this, the failure sends people to rotate certificates that are fine, and any retry/allowlist logic keyed on the error code takes the wrong branch.Cause.
SSL_writedrives the handshake while it has not finished, so a caller that writes before'secureConnect'discovers the handshake failure from insideus_internal_ssl_write. The https client is exactly such a caller: it flushes the request headers as soon as the socket connects, which is the whole difference from the plaintls.connectrepro above.That write path dropped the reason:
ssl_fatal_errormakesus_internal_ssl_is_shut_down()true, so thessl_update_handshake()that follows never reachesSSL_do_handshakeand dispatchesssl_trigger_handshake(s, 0)with nothing parked.ssl_dispatch_parked_reason()finds no reason and falls back to the X509 verify verdict, which is empty.net.tsthen seeshandshake(success=false, verifyError=null)— the shape of a native hostname-verdict failure — treats the dead session as established, and runscheckServerIdentity()against an empty peer certificate, producingERR_TLS_CERT_ALTNAME_INVALID.Visible directly at the native dispatch, with and without an early write:
The handshake path and the read path already park the reason before clearing the queue; only the write path did not.
Fix. Extract that parking into
ssl_park_fatal_reason()(handshake, read and write paths now share it) and call it fromus_internal_ssl_write, so the handshake dispatch that the fatal write triggers reportsEPROTOplus the OpenSSL reason.net.tsalready routes that throughtlsHandshakeError()intoERR_SSL_<REASON>.The helper keeps the existing "only park while the handshake is unfinished" guard: that dispatch is the only consumer, so a reason captured after the handshake completed would linger and be misreported as another socket's failure. For the same reason, a parked reason still owned by a socket being detached is now cleared, mirroring the spill-slot release right above it. The full ownership invariant is written out in a comment below.
This is the
us_socket_tpath. #32929 fixes a different source of the sameverifyError = nullfall-through on the Duplex/SSLWrapperpath; the two are complementary.How did you verify your code works?
test/js/node/tls/node-tls-connect.test.tsgains two tests — the mechanism (tls.connect+ a write before'secureConnect') and the reported symptom (https.request), the latter also asserting that a satisfiable window over the same cert/CA/servername still returns a response, so the version range is provably the only thing that failed.Both fail on the unfixed build with
ERR_TLS_CERT_ALTNAME_INVALIDand pass with the fix:The rest of the file (15 pass),
test/js/node/tls/,test/js/node/http/,test/js/bun/net/and the fetch/serve TLS suites show no delta against the same build without the patch. Node'stest-tls-min-max-version,test-tls-handshake-error,test-tls-destroy-whilst-write,test-tls-close-error,test-tls-close-event-after-writeandtest-tls-server-failed-handshake-emits-clienterrorpass.