node:tls: fix getCipher().version, getEphemeralKeyInfo(), client STARTTLS, and checkServerIdentity exception handling - #32824
node:tls: fix getCipher().version, getEphemeralKeyInfo(), client STARTTLS, and checkServerIdentity exception handling#32824robobun wants to merge 6 commits into
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughTLS handshake handling now defers uncaught ChangesTLS handshake, STARTTLS, and cipher/ephemeral key reporting
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
|
Updated 9:32 AM PT - Jul 9th, 2026
❌ @robobun, your commit 1c65194 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 32824That installs a local version of the PR into your bun-32824 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Checked both before adding any #32239 uses #22870 is about |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing both nits in b5b3ab7 — nothing further from me, but four separate behavior changes across the TLS handshake path (checkServerIdentity exception routing, rejectUnauthorized in the bare-constructor path, and the BoringSSL FFI rewrites) is more than I'm comfortable auto-approving without a human look.
Extended reasoning...
Overview
This PR fixes four node:tls Node-compat divergences across three runtime files plus tests:
src/runtime/socket/tls_socket_functions.rs: rewritesget_cipher()version reporting (replacingSSL_CIPHER_get_versionwithSSL_CIPHER_get_min_version+SSL_CIPHER_get_kx_nid) andget_ephemeral_key_info()(replacing the brokenSSL_get_privatekeypath withSSL_get_negotiated_group), plus removes ~7 now-dead FFI declarations.src/js/node/net.ts: extractsrunCheckServerIdentity()so a user-thrown exception incheckServerIdentityis rethrown viaprocess.nextTickas an uncaught exception (matching Node) instead of being downgraded to a socket'error'by the native handshake error router. Applied to bothSocketHandlersandSocketHandlers2.src/js/node/tls.ts:TLSSocketconstructor now honors its ownrejectUnauthorizedoption, and_start()now passes the wrapped socket toconnect()for the client-STARTTLS pattern instead of throwingERR_MISSING_ARGS.- Six new tests in
node-tls-connect.test.ts, plus updates to the BoringSSL test helper to reflect thatgetEphemeralKeyInfo()now actually works.
Since my prior review, commit b5b3ab7 addressed both of my inline nits: the getEphemeralKeyInfo() empty-result assertions now use .toStrictEqual({type: undefined, name: undefined, size: undefined}), and the checkServerIdentity fixture test now includes stderr in the combined assertion object.
Security risks
This is squarely security-sensitive territory. Two changes in particular touch certificate-verification flow:
- The
rejectUnauthorizedconstructor change affects whether a TLS client rejects an untrusted peer on the bare-new TLSSocket(socket, opts)path. The change looks correct (it only sets_rejectUnauthorizedwhen the option is explicitly provided, preserving the env-var default otherwise), but any change to this flag's plumbing deserves a careful human read. - The
checkServerIdentityexception-routing change alters control flow during the post-handshake verify step: when the user callback throws, the handler now early-returns (abandoning the rest of the handshake handler —secureConnectnever fires,authorizedis never set) and rethrows on the next tick. This matches Node, but it's a behavior change in how identity-check failures surface.
The Rust FFI changes (getCipher().version, getEphemeralKeyInfo()) are read-only/informational and don't affect the security posture of the connection itself.
Level of scrutiny
High. node:tls handshake handling, certificate verification routing, and rejectUnauthorized are exactly the kind of code where a subtle mistake silently weakens security. The changes are well-reasoned, well-commented, and each has a dedicated test that fails on main, but per policy I don't auto-approve security-sensitive crypto/TLS changes regardless of apparent quality.
Other factors
- No CODEOWNERS cover these paths.
- The bug-hunting system found nothing this run; my two prior nits were test-precision only and have been addressed.
- The PR description is thorough and the author already verified (and declined to link) the two auto-suggested issues, which is a good signal.
- CI build #65367 was triggered for the latest commit; I did not verify its status.
|
Head is 1c65194, rebased onto current main. All seven review findings addressed and their threads resolved; both review bots report no remaining issues on this head. Review-found additions since the rebase:
Ten new tests, all with a clear fail-before diff. The full test file is 25 pass / 0 fail under Build 71057 finished 282 passed, 4 red. None of the four touch
No failure in any |
…TTLS, checkServerIdentity exceptions
getCipher().version always reported BoringSSL's hardcoded "TLSv1/SSLv3"
placeholder. Rebuild Node's strings from SSL_CIPHER_get_min_version plus
the key-exchange NID for the pre-1.2 suites.
getEphemeralKeyInfo() read the client's own private key (always empty on
a client without a certificate). Report the negotiated (EC)DHE group via
SSL_get_negotiated_group instead, matching Node for TLS <= 1.2 and its
{} on TLS 1.3 and static-RSA key exchanges.
An exception thrown from a user checkServerIdentity was caught by the
native handshake handler's error routing and emitted as a socket
'error'. Node does not guard the callback; defer the rethrow so it
escapes as an uncaught exception and 'secureConnect' does not fire.
new tls.TLSSocket(socket, { isServer: false }) + _start() threw
ERR_MISSING_ARGS because connect() received no port, path, or socket.
Hand the wrapped socket to connect(), and honor the constructor's own
rejectUnauthorized, which that path never routed through
Socket.prototype.connect.
Deleted the now-dead FFI: SSL_get_privatekey, EVP_PKEY_id,
EVP_PKEY_bits, EVP_PKEY_get1_EC_KEY (which deliberately leaked the
EC_KEY), EC_KEY_get0_group, EC_GROUP_get_curve_name,
SSL_CIPHER_get_version, the EVP_PKEY/EC_KEY/EC_GROUP opaque handles,
and the EVP_PKEY_DH/X25519/X448 constants.
…tderr in tests
Node's getEphemeralKeyInfo() always returns the same three own keys on a
client, with undefined values when no ephemeral key applies; toEqual
ignored undefined-valued keys and the comments wrongly described the
result as a bare {}. Use toStrictEqual against the exact shape, and
include the spawned fixture's stderr in the asserted object so an
unexpected crash trace shows up in the failure diff.
b5b3ab7 to
1faed51
Compare
A resumed TLS 1.2 abbreviated handshake has no ServerKeyExchange, so Node's SSL_get_peer_tmp_key is empty and getEphemeralKeyInfo returns the three undefined keys. BoringSSL serializes the original handshake's group_id into the session blob, so SSL_get_negotiated_group would surface it on the resumed connection; gate it on SSL_session_reused.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tls.ts`:
- Around line 943-944: Normalize rejectUnauthorized to a real boolean in the TLS
option handling instead of storing the raw value: update the client-side
assignment in the relevant tls constructor/path so only an explicit false
disables verification, and mirror the same normalization in the server-side
constructor path as well. Use the existing rejectUnauthorized option handling in
the TLS initialization flow to locate both spots and ensure null, 0, and empty
string do not bypass certificate checks.
In `@test/js/node/tls/node-tls-connect.test.ts`:
- Around line 854-902: The `session` variable in `getEphemeralKeyInfo()` is
hoisted with `var` inside the `full` connection block, which makes the scope
unclear. Declare `session` with `let` before the `try` that uses `connect()`,
then assign it from `full.getSession()` so it remains available after
`full.destroy()` while making the test easier to read and maintain.
🪄 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: 5226c5a3-e919-4cab-9a09-82c7ce41a750
📒 Files selected for processing (6)
src/js/node/net.tssrc/js/node/tls.tssrc/runtime/socket/tls_socket_functions.rstest/js/node/test/common/boringssl.jstest/js/node/test/parallel/test-tls-client-getephemeralkeyinfo.jstest/js/node/tls/node-tls-connect.test.ts
…fication Node's CVE-2021-22939 fix checks `rejectUnauthorized !== false`: every other value, including null, keeps certificate verification on. Bun's client connect paths stored the raw option value and then checked it for truthiness, so rejectUnauthorized: null silently bypassed verification where Node rejects. Normalize to a real boolean at each store site (the TLSSocket constructor, Server setSecureContext, and the three Socket.prototype.connect TLS paths). The bindgen layer already rejects 0/""/1 with ERR_INVALID_ARG_TYPE, so null was the only non-boolean that reached these sites. Also: declare `session` with let before the try in the resumption test instead of hoisting with var.
…t socket
Socket.prototype.connect unconditionally overwrites this.servername from
its options object, so a setServername() call between `new
TLSSocket(socket, { isServer: false })` and _start() was wiped and no
SNI was sent. Node preserves the value and sends it. Forward the
current servername in the connect() options so the handshake carries it.
Node's onConnectSecure guards the user callback with `!this.isSessionReused()`: the identity was verified on the original full handshake, so a resumed abbreviated handshake skips it. Bun called it every time, so a side-effecting or throwing callback diverged (and the new nextTick rethrow made a throw on the resumed call fatal). Add the same guard to both handshake handlers.
There was a problem hiding this comment.
All prior review feedback has been addressed and no new issues found, but this touches security-sensitive TLS paths (rejectUnauthorized normalization, checkServerIdentity exception routing, handshake FFI) so it's worth a human sign-off.
Extended reasoning...
Overview
This PR fixes six node:tls client divergences from Node.js across three source files and three test files (~490 lines net). The changes span:
src/runtime/socket/tls_socket_functions.rs: rewritesget_cipherversion reporting andget_ephemeral_key_infoto derive results fromSSL_CIPHER_get_min_version/SSL_get_negotiated_groupinstead of the previous (broken/leaking)SSL_get_privatekeypath; deletes seven now-dead FFI declarations.src/js/node/net.ts: refactors both handshake handlers to routecheckServerIdentitythrough a newrunCheckServerIdentityhelper that defers a rethrow viaprocess.nextTick(so a user throw becomes an uncaught exception, matching Node), adds!self.isSessionReused()guards, and normalizesrejectUnauthorizedto!== falseat three connect sites.src/js/node/tls.ts: makes_start()forward the wrapped socket + servername toconnect()for the client STARTTLS pattern, honorsrejectUnauthorizedin theTLSSocketconstructor, and normalizes it in serversetSecureContext.- Ten new tests plus updates to two existing test helpers that encoded the old (wrong) behavior.
Security risks
This PR is squarely in security-sensitive territory:
rejectUnauthorizednormalization across five sites directly controls whether TLS certificate verification runs. The change is a hardening (only explicitfalsedisables verification, fixing a pre-existingnullbypass matching CVE-2021-22939), but any error here could either bypass verification or break legitimate opt-outs.checkServerIdentityrethrow changes error routing from a catchable socket'error'to an uncaught exception. This matches Node, but widens blast radius for throwing callbacks — mitigated by the newisSessionReused()guard so it never fires on resumption.- New BoringSSL FFI declarations (
SSL_version,SSL_get_negotiated_group,SSL_CIPHER_get_min_version,SSL_CIPHER_get_kx_nid) — all declaredsafe fnunder the module's opaque-ZST convention; signatures look correct against BoringSSL headers.
I don't see a security regression — the rejectUnauthorized change is strictly a hardening, and the fail-closed direction is correct. But TLS verification logic is exactly where a subtle mistake has outsized consequences.
Level of scrutiny
High. This is production TLS handshake code. The individual changes are well-reasoned, empirically verified against Node v26.3.0, and thoroughly tested (each of the ten new tests has a documented fail-before/pass-after diff). But the combination of certificate-verification gating, exception-routing changes, and FFI surface changes in one PR warrants a maintainer's eyes rather than bot-only approval.
Other factors
- All seven prior review findings (mine and CodeRabbit's) were addressed with follow-up commits and empirical re-verification against Node; every thread is resolved.
- The bug-hunting system found no issues on the current head (1c65194).
- Test coverage is strong: each fix has a dedicated test asserting the exact Node-matching behavior, and the PR description documents pass/fail on both ASAN debug and release builds.
- The dead-code deletion (EVP_PKEY/EC_KEY FFI) is clean — grepped and confirmed no remaining callers.
Given the security-sensitive scope, deferring to a human reviewer per the approval guidelines.
|
Heads-up on the |
|
Ran into the
The variant with both guards plus ten tests for |
1 participant
Fixes eight
node:tlsclient divergences from Node, found by a differential probe run against a real Node v26tls.Server(only the client side exercised Bun). The probe output is byte-identical to Node after this change, except client-initiatedrenegotiate()on TLS 1.2, which BoringSSL does not support.getCipher().versionis always"TLSv1/SSLv3"BoringSSL hardcodes
SSL_CIPHER_get_version()to that string. Node (OpenSSL) reports the cipher's minimum protocol version. Rebuilt the same strings fromSSL_CIPHER_get_min_version(): TLS 1.3 and SHA-2 AEAD suites directly, and for the pre-1.2 suitesSSL_CIPHER_get_kx_nid()splits the ECC ones ("TLSv1.0", defined for TLS 1.0 by RFC 4492) from the SSLv3-era rest, matching OpenSSL's table exactly.getEphemeralKeyInfo()returns{}on a TLS 1.2 ECDHE connectionNode reports
{ type: "ECDH", name: "X25519", size: 253 }. The old code calledSSL_get_privatekey(), which is the client's own key and is null for a client without a certificate, so the result was always empty (and leaked anEC_KEYon the one path that got further). Replaced withSSL_get_negotiated_group(). Node returns{}on TLS 1.3 and on a static-RSA key exchange; both are matched.An exception from a user
checkServerIdentitybecomes a socket'error'Node does not guard the callback: a throw aborts
onConnectSecureand reaches the process as an uncaught exception, and'secureConnect'never fires. Bun's native handshake dispatcher routes any exception from the JS handshake handler into the socket's error handler, which downgraded the throw intosocket.emit('error', err). The JS handler now catches only the user callback, abandons the handshake, and defers the rethrow out of the native try frame. The callback is also now skipped on a resumed session, matching Node's!this.isSessionReused()guard (found in review; Bun was calling it twice).new tls.TLSSocket(socket, { isServer: false })+_start()throwsERR_MISSING_ARGSThe client STARTTLS pattern.
_start()calledconnect()with no port, path, or socket. It now hands the wrapped socket toconnect(), the same thingtls.connect({ socket })does. The constructor also honors its ownrejectUnauthorizednow: that path never reachesSocket.prototype.connect, which was the only place the option was read. AsetServername()call between construction and_start()is now forwarded through to the handshake as the client SNI (found in review; Node sends it, Bun was wiping it).getEphemeralKeyInfo()on a resumed TLS 1.2 sessionFound in review. A resumed abbreviated handshake has no ServerKeyExchange, so Node's
SSL_get_peer_tmp_keyis empty and the three keys areundefined. BoringSSL serializes the original handshake'sgroup_idinto the session blob, soSSL_get_negotiated_groupwould surface it; gated onSSL_session_reused.rejectUnauthorized: nullbypassed certificate verificationFound in review, pre-existing on
tls.connect(). Node only disables verification on an explicitfalse(CVE-2021-22939); Bun stored the raw option value and then checked it for truthiness, sonullsilently skipped rejection where Node rejects. Normalized to!== falseat each client store site (theTLSSocketconstructor, the threeSocket.prototype.connectTLS paths, and serversetSecureContext). The bindgen layer already rejects0/""/1withERR_INVALID_ARG_TYPE, sonullwas the only non-boolean that reached these sites.Deleted dead code
Made dead by the
getEphemeralKeyInforewrite and removed in the same change: theSSL_get_privatekey,EVP_PKEY_id,EVP_PKEY_bits,EVP_PKEY_get1_EC_KEY,EC_KEY_get0_group,EC_GROUP_get_curve_name, andSSL_CIPHER_get_versionFFI declarations, theEVP_PKEY/EC_KEY/EC_GROUPopaque handles, and theEVP_PKEY_DH/EVP_PKEY_X25519/EVP_PKEY_X448constants.Tests
Ten new tests in
test/js/node/tls/node-tls-connect.test.ts; all ten fail on a build without the corresponding fix and pass with the fix. The existinggetCipherassertions on"TLSv1/SSLv3"and thetestEphemeralKeyInfoUnsupportedhelper intest/js/node/test/common/boringssl.jsencoded the old behavior and are updated (the helper is renamed totestEphemeralKeyInfoEcdheOnly; the DHE parts of the original Node test still cannot run because BoringSSL has no DHE cipher suites).The full
test/js/node/tls/directory passes with the change; the only failures are two that reproduce identically onmainwithout it.Not addressed, from the same probe, because BoringSSL has no client-initiated renegotiation:
client.renegotiate()on a TLS 1.2 connection returnsfalsewhere Node renegotiates.[review] gate passed · iteration 7 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 7
evidence per changed file