Repository navigation
Conversation
WalkthroughRemoves ChangesTLS Hostname Mismatch Authorization Removal
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 9:45 PM PT - Jul 8th, 2026
✅ @robobun, your commit 5e4bcea919969e27e4a1cc11e2283f2bd8e87c9a passed in 🧪 To try this PR locally: bunx bun-pr 32359That installs a local version of the PR into your bun-32359 --bun |
There was a problem hiding this comment.
The change looks correct and well-tested, but it removes a native TLS hostname check that #31339 deliberately added — reverting that for Bun.connect (where there's no JS-layer checkServerIdentity fallback) is a security-adjacent product decision a human should sign off on.
Extended reasoning...
Overview
This PR reverts the native bun_boringssl::check_server_identity() call from on_handshake in src/runtime/socket/socket_body.rs, drops the Flags::HOSTNAME_MISMATCH bitflag and its get_authorization_error branch, and rewrites/extends test/js/node/tls/node-tls-connect-hostname-verification.test.ts to lock in the restored semantics. The author addressed both of my earlier inline nits (stale Zig cross-reference comment, redundant matching-SNI sub-test) in follow-up commits fe48dff and e948994.
Security risks
For node:tls, hostname verification is still enforced in the JS layer (src/js/node/net.ts calls checkServerIdentity and produces ERR_TLS_CERT_ALTNAME_INVALID), and the four pre-existing tests in this file continue to assert that path — so tls.connect users are unaffected security-wise. For the raw Bun.connect API, however, this restores the Zig-era behavior where the native handshake reports only the chain-verify result and hostname matching is the caller's responsibility. That matches Bun 1.3.14, but #31339 intentionally added the native check; reverting it means Bun.connect callers who trust a CA but don't do their own hostname check will see authorized=true for a mismatched cert. Whether that's the intended contract for Bun.connect is a product/security call.
Level of scrutiny
High. Anything that removes a hostname-verification step from a TLS handshake path — even a redundant one — falls squarely into security-sensitive territory that warrants human review per my approval guidelines.
Other factors
The Rust diff is small and mechanical (pure deletion back to authorized = success == 1), Flags::HOSTNAME_MISMATCH has no remaining references, and the new tests are clear and were verified to fail-before/pass-after. CI shows musl build-cpp failures and -no-pie linker warnings that appear to be pre-existing infra noise unrelated to this diff. No correctness bugs found.
…ompat The Rust port of on_handshake (socket_body.rs) gained a bun_boringssl::check_server_identity() call in #31339 that the Zig reference (socket.zig onHandshake) never had. When the cert chain verifies but the peer cert's SAN/CN does not cover the connect hostname/servername, this flipped authorized to false, set a HOSTNAME_MISMATCH flag, and passed success=false to the JS handshake callback. Downstream effects: - tls.connect sets _secureEstablished = !!success, now false where Node and Bun 1.3.14 both report true - Bun.connect socket.authorized is now false where Zig returned true - socket.getAuthorizationError() returns an Error with the non-Node-standard code HOSTNAME_MISMATCH instead of null - a user-supplied options.checkServerIdentity that intentionally accepts the mismatch can no longer make _secureEstablished/success true Hostname verification is already performed in the JS layer (src/js/node/net.ts) via the user-overridable checkServerIdentity callback, which produces the Node-standard ERR_TLS_CERT_ALTNAME_INVALID. The native layer should report only the BoringSSL cert-chain verify result, matching the Zig reference. Revert on_handshake and get_authorization_error to match the Zig reference, drop the HOSTNAME_MISMATCH flag, and replace the test that locked in the divergent behavior with one asserting Zig/Node semantics. Add a node:tls test (verified against Node) that a user-supplied checkServerIdentity returning undefined yields _secureEstablished=true, authorized=true.
With the native hostname check removed, authorized depends only on chain verification; the matching-SNI case traverses the identical path as the mismatch case and adds no coverage.
e948994 to
0c95909
Compare
0c95909 to
5e4bcea
Compare
There was a problem hiding this comment.
All my prior nits are addressed and I didn't find further issues, but this removes native hostname verification from the TLS handshake path — a security-relevant semantic change (especially for direct Bun.connect users, who have no JS-layer checkServerIdentity fallback) that warrants a human sign-off even though the Zig-parity/Node-compat rationale is sound.
Extended reasoning...
Overview
This PR reverts the Rust on_handshake in src/runtime/socket/socket_body.rs to match the Zig reference: it deletes the bun_boringssl::check_server_identity() call, the HOSTNAME_MISMATCH bitflag, and the corresponding branch in get_authorization_error, so authorized is now purely success == 1 (chain verification). It updates the two mirrored handshake-handler comment blocks in src/js/node/net.ts to drop stale references to a "native hostname verdict", and rewrites/extends test/js/node/tls/node-tls-connect-hostname-verification.test.ts to lock in the restored semantics for both tls.connect (with a user checkServerIdentity override) and Bun.connect.
Security risks
This is a TLS certificate-verification change that removes a hostname check from the native layer. For node:tls, the JS layer already performs hostname verification via the user-overridable checkServerIdentity, so behavior there is Node-compatible and not weakened. For raw Bun.connect, however, there is no JS-layer hostname check — after this PR, a CA-trusted cert with a mismatched CN/SAN reports authorized=true and getAuthorizationError()=null. That matches Bun 1.3.14 (Zig) behavior, and the PR explicitly asserts it in a test, but it is still a deliberate decision to leave hostname verification to the caller for the Bun-native API. That's the kind of security-posture call a maintainer should ratify rather than a bot.
Level of scrutiny
High. TLS verification semantics are on the "never remove a flag you don't understand in a TLS/crypto path" list in the repo guidelines. The rationale here is well-argued (Zig parity, Node compat, the JS layer is the canonical place for this per Node's design, and #31339 introduced the divergence), and the diff itself is small and mechanically clean — but the effect is that a security check moves from "enforced natively" back to "caller's responsibility" for one API surface.
Other factors
All three of my earlier inline nits (bitflags comment accuracy, redundant matching-SNI sub-test, stale "native hostname verdict" comment blocks) have been addressed in follow-up commits, and the bug-hunting system found nothing. The test changes look correct and the pre-existing ERR_TLS_CERT_ALTNAME_INVALID tests remain in place, confirming the JS-side check still fires. I'm deferring solely because of the security-sensitive nature of the surface, not because of any concrete defect.
|
Closing: superseded by #33755, which addresses the same Node compatibility concerns with a different design. #33755 keeps the native All the Node compatibility issues from the original report are resolved on main:
The remaining difference from this PR is that |
…r end() (#44290) Regression from #42181 (in no release). Merge before 1.4.3 is tagged, and before #43957 and #43962. ### Problem - After `end()` during its handshake, a node:tls client accepts a trusted certificate for another name: no error, and it reads the server's data. Node and Bun 1.4.2 report `ERR_TLS_CERT_ALTNAME_INVALID`. - `on_handshake` (`src/runtime/socket/socket_body.rs`) passed its name verdict to the handler as the success flag. After `end()`, `onClientHandshake` (`src/js/node/net.ts:496`) reads a failure with no error as the client's own close and skips `checkServerIdentity`. ### Fix - `on_handshake` no longer runs the native name check for a node:tls socket. The handler's flag is the handshake result. - `Bun.connect` and `upgradeTLS` sockets do not change. - Verified: `node-tls-duplex-end-verify.test.ts`, 19 new tests. `main` fails 18. Node v26.3.0 passes 49, skips 1. - Self-reviewed: 10 concerns raised, 10 addressed. One is #44422 (`onClientHandshake` reads some failed handshakes as established). ### Background - node:tls checks the name in JS (`checkServerIdentity`), on sockets that `DEFERS_SERVER_IDENTITY` marks. The native check ran for them: another name gave `(false, null)`, like a handshake that fails after this side's FIN. - Considered a `getAuthorizationError()` test in the guard, or a second flag variable: both keep an unused native check. ### Downsides - Needs a maintainer's yes: the internal `socket._handle.authorized` and `getAuthorizationError()` of a node:tls client drop the name check (no reader in `src/js` or `test`). The public values do not change. - Still open (#43957): only the first such connection of a process reports if its server keeps its side open. <details><summary>Notes</summary> **The regression** (`tls.connect({ socket })` on a connected socket, then `end()`. Trusted chain, certificate for another name, server in the same process, 3 runs each) | Runtime | Output | |---|---| | Node v26.3.0 | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | | Bun 1.4.2 (744846f) | `error ERR_TLS_CERT_ALTNAME_INVALID, close` | | Bun 1.3.13 | `end, finish, error ECONNRESET, close` | | canary 1.4.3-canary.1 (367d939, has #42181) | `finish, end, close`, no error | | this PR | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | - #42181 (44e51b7) is on `main` and is not an ancestor of the tag `bun-v1.4.2`. The client handler at that tag has no such guard. **Merge order, and why** - This PR first. Both later PRs let more handshakes complete after `end()`, and on `main` each of those accepts a certificate for another name. - #43957 removes the engine defect that today keeps the second and later connections of a process from completing such a handshake. Merged before this PR, it turns one silently accepted connection into all of them. Measured with five connections in one process (`tls.connect({ socket })`, another name, server keeps its side open): on `main` 1 of 5 handshakes completes, and the client reports no error for it. On `main` with the source change of #43957, 5 of 5 complete with no error. - #43962 sends a client's FIN after its ClientHello. It contains this PR's commits. - With #43957 only the test file conflicts: both PRs insert a block. Keep both. **Where the native check came from** - #31339 added the native name check for all clients. - #32359 proposed to remove it and was closed for #33755. #33755 added `DEFERS_SERVER_IDENTITY`: the check stayed enforced for `Bun.connect`, and for node:tls it was computed but not enforced. It took node:tls sockets out of `reject_unauthorized` and out of the error argument. - #42181 then added the guard for "failed after our own FIN", which is the same pair as "computed, another name". - #43863 says "Each connection has one server name check", and lists node:tls as "checked after the handshake, in JS". - This PR is the node:tls half of #32359. It does not touch the check for `Bun.connect`. - #43865, #43924 and #43947 are the fixes of the chain check in the same test file. **What the client handler gets from the fd engine** (`packages/bun-usockets/src/crypto/openssl.c`) `on_handshake` calls the handler with `(socket, flag, error)`. For a node:tls client: | Case | Before | Now | |---|---|---| | Handshake completed, chain and name good | `true, null` | `true, null` | | Handshake completed, chain good, another name | `false, null` | `true, null` | | Handshake completed, chain bad | `true, X509 error` | `true, X509 error` | | Chain refused inside the handshake | `false, X509 error` | `false, X509 error` | | Handshake failed after this side's FIN | `false, null` | `false, null` | | Handshake failed with a TLS reason | `false, EPROTO` | `false, EPROTO` | | The peer closed before the handshake finished | `false, ECONNRESET` | `false, ECONNRESET` | - Row 2 and row 5 were the same pair. The guard that #42181 added for row 5 also caught row 2 once the client had ended. - With no `end()`, row 2 already reached `checkServerIdentity`, because the guard tests `writableFinished`. So a client that does not end behaves as before. - `Bun.connect` and `upgradeTLS` sockets do not carry `DEFERS_SERVER_IDENTITY`. Their flag is `authorized`, as documented. **Rows that this PR does not change** The engine for a Duplex, a named pipe and TLS inside TLS (`SSLWrapper`, `src/uws/lib.rs`) reports no TLS reason. A failed handshake arrives as `(false, null)` after a verified chain and as `(false, X509 code)` otherwise. `onClientHandshake` reads both as an established session unless the client rejects the code. Measured on this PR with a client over a Duplex that does not call `end()`: | The peer answers the ClientHello with | `rejectUnauthorized` | `main`, Bun 1.4.2 and this PR | Node v26.3.0 | |---|---|---|---| | a fatal `handshake_failure` alert | `true` | `error UNABLE_TO_GET_ISSUER_CERT, close` | `error ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE, close` | | a fatal `handshake_failure` alert | `false` | `secureConnect authorized=false authError=UNABLE_TO_GET_ISSUER_CERT`, `end` | `error ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE, close` | | bytes that are not TLS | `false` | `secureConnect authorized=false authError=UNABLE_TO_GET_ISSUER_CERT`, `end` | `error ERR_SSL_WRONG_VERSION_NUMBER, close` | | a `close_notify` alert | either | no event | `end, error ECONNRESET, close` | - These rows are older than this PR: Bun 1.4.2 prints them too. The native socket is marked unusable after a failed handshake (`transport_unusable` in `on_handshake`). - Owners: #32929 (the engine reports the fatal alert over a Duplex), #44223 and #44021 (a refused renegotiation). - #44422 makes a handshake that did not complete terminal in `onClientHandshake`, as it is in the server handler. This PR is the step before it: while `(false, null)` could be a completed handshake, that arm could not go. It stays out of this PR so that this PR can land, or be reverted, alone. **What the native handle of a node:tls client reports** (trusted chain, another name, `rejectUnauthorized: false`, measured) | Value | `main` | This PR | |---|---|---| | `socket.authorized` | `false` | `false` | | `socket.authorizationError` | `ERR_TLS_CERT_ALTNAME_INVALID` | `ERR_TLS_CERT_ALTNAME_INVALID` | | `socket._handle.authorized` | `false` | `true` | | `socket._handle.getAuthorizationError()` | `ERR_TLS_CERT_ALTNAME_INVALID` | `null` | - The native check for these sockets had three effects on `main`: the flag in row 2 above, and the two internal values. - The in-handshake check (`server_identity`) already left node:tls sockets out. Both sites now agree. - `Flags::HOSTNAME_MISMATCH` has no reader on `main`. This PR does not remove it. - The hunk that stops the native check came from an optional finding of an automated review. No person has reviewed it yet. **Measured** (Linux x64, debug builds, Node v26.3.0, server in its own Node process, 40 s watchdog) `tls.connect({ socket })` on a connected socket, then `end()`. The chain is trusted, the certificate is for another name, `rejectUnauthorized: true`: | Server | TLS | Node | `main` (d115f54) | This PR | |---|---|---|---|---| | keeps its side open | 1.3 | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | `finish`, never exits | same as Node | | ordinary | 1.3 | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | `finish, end, close`, no error | same as Node | | keeps its side open | 1.2 | `finish`, never exits | `finish`, never exits | same as Node | | ordinary | 1.2 | `finish, end, error ECONNRESET, close` | same as Node | same as Node | - `end()` in the same tick and `end()` one `setImmediate` later give the same lines. - In the new tests the handshake completes after the client's `end()` under TLS 1.2 and over a Duplex as well. There `main` delivers `secret-banner` to a client with `rejectUnauthorized: true`. - A client that passes its own `checkServerIdentity` is asked: one test asserts the name that it gets, and `authorized=true` when it accepts. **Still open, the engine** - Each line of the table above is the first connection of its process. Five such connections, one after the other in one process, against a server that keeps its side open: with this PR the first reports and the other four print `finish` only. `main` does the same when the name is good. The flight that the first socket seals after its FIN takes the event loop's one spill slot and never leaves it. #43957 is open for that. - `tls.connect(port)` followed at once by `end()` still sends its FIN before its ClientHello, so no handshake runs there. #43962 changes that order. - #43957 is open for the spill slot. Measured with its source change on this PR: all five connections report, for another name and for a good name. - A handshake that fails after `end()` with no certificate still reports nothing. The tests of #42181 and #43947 for that case pass. **Tests** - 19 new tests against `main`: the name check after `end()` over a Duplex, over TCP behind a record proxy (TLS 1.2 and 1.3) and on a connected `net.Socket` (3 each), `end()` and `end("")` in the turn of `tls.connect()` over a socket that is still connecting and over a Duplex (6), and a resumed session (1, Bun only). - 7 of them are the commit of another branch (`robobun/baba0385/tls-early-end-hostname-check`), taken as it is. The resumed-session test now asserts `isSessionReused()`. - `main` (4b02e10) fails 18 of the 19. The one that passes is `end("")` over a Duplex. **Suites** (this machine ran at a load average of 200 to 800) - On this head: `node-tls-duplex-end-verify` (50 of 50), `node-tls-connect` (121), `tls-reject-before-client-cert` (121), `node-https-agent-checkserveridentity-reuse` (30), `renegotiation` (21), `node-tls-wrapped-socket-close` (15), `node-tls-connect-hostname-verification` (11), `node-tls-upgrade` (5), `node-tls-raw-end` (4), `node-https-checkServerIdentity` (4). - On the head before the rebase: 14 vendored Node tests that use `checkServerIdentity` or the name error, `fetch.tls` (61). `socket.test.ts`, `node-tls-cert` and `node-tls-server` had only failures that `main` has on this machine too (timeouts of child processes, and one test that needs `www.example.com`). - Not run on this machine: macOS, Windows. </details>
Repro
Local server presents a cert for
CN=agent1signed byca1. Client trustsca1and connects with a servername that does not match the cert.For
Bun.connectwithtls: { ca, serverName: "localhost" }:successsocket.authorizedgetAuthorizationError()truetruenullfalsefalse{ code: "HOSTNAME_MISMATCH" }For
tls.connectwithcheckServerIdentity: () => undefined(user override that accepts):_secureEstablishedauthorizedtruetruetruetruetrue(since #31155)true#31155 already worked around the
_secureEstablishedside innet.tsby hardcoding it totruewith a comment referencing "the native hostname verdict"; this PR removes the native verdict itself and drops that now-stale comment.Cause
The Rust
on_handshake(src/runtime/socket/socket_body.rs) gained abun_boringssl::check_server_identity()call in #31339 that the Zig reference (socket.zigonHandshake) never had. When the cert chain verifies but the peer cert's SAN/CN does not cover the connect hostname/servername, this flippedauthorizedtofalse, set aHOSTNAME_MISMATCHflag, and passedsuccess=falseto the JS handshake callback.Hostname verification for
node:tlsis already performed in the JS layer (src/js/node/net.tshandshake handlers) via the user-overridablecheckServerIdentitycallback, which produces the Node-standardERR_TLS_CERT_ALTNAME_INVALID. The native double-check means:Bun.connectsocket.authorizedisfalsewhere the Zig reference returnedtruesocket.getAuthorizationError()returns an Error with the non-Node codeHOSTNAME_MISMATCHinstead ofnullsuccess=false, so any consumer reading it (including the pre-net,tls: port Node.js net/tls compatibility tests and fix the gaps they surface — half-open/reset/write semantics, server TLSSocket wrap, session/keylog, SNICallback/ALPNCallback, pfx, OpenSSL error shapes, addCACert, local binding (+305 tests) #31155node:tls) sees the session as not establishedFix
Revert
on_handshakeandget_authorization_errorto match the Zig reference (authorized = success == 1, no hostname check), drop theHOSTNAME_MISMATCHflag, and replace theBun.connecttest added in #31339 that locked in the divergent behavior with one asserting the Zig/Node semantics. Drop the stale "native hostname verdict" reference from the twonet.tsclient handshake comments.Verification
test/js/node/tls/node-tls-connect-hostname-verification.test.ts:user checkServerIdentity that accepts a mismatch yields _secureEstablished=true, authorized=true(verified to pass under Node)Bun.connect TLS handshake reports chain verification onlyasserting{success: true, authorized: true, authorizationError: null, verifyError: null}on hostname mismatch with a CA-trusted certWith
src/atorigin/main: 5 pass / 1 fail (the Bun.connect test). With the fix: 6 pass / 0 fail. The four pre-existing tests in the file, which cover the JS-sidecheckServerIdentityproducingERR_TLS_CERT_ALTNAME_INVALID, still pass.Rebase note
Conflicted with #31859 in the
Flagsbitflags block (that PR removedOWNS_HANDLERSat bit 10, this one removesHOSTNAME_MISMATCHat bit 11); resolution keeps neither.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file