fetch: report OpenSSL reason (ERR_SSL_*) for non-certificate TLS handshake failures - #34921
fetch: report OpenSSL reason (ERR_SSL_*) for non-certificate TLS handshake failures#34921robobun wants to merge 3 commits into
Conversation
When fetch()'s TLS handshake fails for a reason unrelated to certificate
verification (server sends a fatal ALPN alert, peer is not speaking TLS,
or the socket closes mid-handshake), the error was reported as
UNKNOWN_CERTIFICATE_VERIFICATION_ERROR even with rejectUnauthorized:false,
where a certificate error is impossible by construction. Node reports the
same scenarios as ERR_SSL_WRONG_VERSION_NUMBER / the alert identity.
uSockets already dispatches these as {error_no: -71, code: "EPROTO",
reason: <openssl error string>} (ssl_dispatch_parked_reason) or
{error_no: -46, code: "ECONNRESET"} (ssl_trigger_handshake_econnreset);
the HTTP client was feeding the negative error_no into
get_cert_error_from_no(), whose X509_V_ERR_* table sent it to the
catch-all UNKNOWN_CERTIFICATE_VERIFICATION_ERROR.
Fix: the handshake-failure branch in HTTPContext::on_handshake and
ProxyTunnel::on_handshake now captures the sentinel's code and reason
into an owned TLSHandshakeError on the client result (same side-channel
shape as dns_hostname) and fails with Error::TLSHandshakeFailed;
FetchTasklet::on_reject derives Node's ERR_SSL_<REASON> code from the
OpenSSL reason string. SSLWrapper::update_handshake_state is taught to
peek ERR_peek_last_error before clearing the queue so the proxy-tunnel
inner-TLS path carries the same identity. ProxyTunnel::on_handshake also
hoists did_have_handshaking_error before the success branch so a failed
inner handshake no longer falls through to ConnectionRefused.
|
Updated 7:05 AM PT - Jul 21st, 2026
✅ @robobun, your commit a944c97c9898ae90258f8f1fa822e65957b6d874 passed in 🧪 To try this PR locally: bunx bun-pr 34921That installs a local version of the PR into your bun-34921 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Intentional. As noted in the PR body this supersedes #34182 and subsumes #31950: those map the uSockets handshake-failure sentinel to a generic |
WalkthroughChangesThe HTTP client now captures non-certificate TLS handshake reasons from OpenSSL, propagates them through client results, and maps them to structured fetch errors. Tests cover ALPN failures, plaintext HTTPS responses, and invalid proxy tunnel data. TLS handshake failure reporting
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I audited the other SSLWrapper::update_handshake_state consumers for the new -71/EPROTO dispatch shape: UpgradedDuplex/WindowsNamedPipe route to TLSSocket::on_handshake which already handles negative error_no (owned code_bytes()/reason_bytes() copies, same as the direct openssl.c path), and WebSocketProxyTunnel::on_handshake gates on error_no != 0 which is unchanged. Also confirmed ERR_reason_error_string returning NULL is handled — ZStr::from_c_ptr and reason_bytes() both map null to empty, and node_error_code() falls back to EPROTO.
Extended reasoning...
The SSLWrapper change in src/uws/lib.rs is a shared helper touched by four handshake consumers (ProxyTunnel, UpgradedDuplex, WindowsNamedPipe, WebSocketProxyTunnel). I traced each to confirm the new {error_no: -71, code: "EPROTO", reason: ERR_reason_error_string(...)} shape is compatible: TLSSocket::on_handshake (socket_body.rs:1642, reached via UpgradedDuplex/WindowsNamedPipe) already boxes code/reason as owned bytes and was designed for this shape from the direct openssl.c path; WebSocketProxyTunnel gates on error_no != 0. I also checked the null-reason edge: ERR_reason_error_string can return NULL, but ZStr::from_c_ptr (bun_core/util.rs:236) and us_bun_verify_error_t::reason_bytes both map null to empty, and node_error_code() falls back to the sentinel code. Deferring to human review given this touches TLS handshake error handling across the HTTP client, proxy tunnel, and shared SSLWrapper.
There was a problem hiding this comment.
Beyond the inline nit, I also checked: ERR_reason_error_string returning NULL in the SSLWrapper path — reason_bytes() maps null to b"", and node_error_code()/FetchTasklet fall back to EPROTO / "TLS handshake failed"; and InternalState::reset() — the new tls_handshake_error field is cleared via ..Default::default() on redirect/retry.
Extended reasoning...
This run's inline finding is a code-style nit (magic 8 vs b"ERR_SSL_".len()). While reviewing the Rust side I traced the null-pointer path for the SSLWrapper's ERR_reason_error_string result and the redirect/retry reset of the new state field; both are handled. The PR is otherwise a well-scoped error-reporting fix, but it touches TLS handshake failure control flow across HTTPContext/ProxyTunnel/SSLWrapper (including the did_have_handshaking_error hoist in ProxyTunnel), so I'm leaving it for human review rather than approving.
There was a problem hiding this comment.
No issues found, but this touches TLS handshake error handling across the HTTP client, ProxyTunnel, and SSLWrapper (BoringSSL FFI), plus a behavior change hoisting did_have_handshaking_error in ProxyTunnel::on_handshake — worth a human look.
What was reviewed:
error_no < 0correctly discriminates the uSockets-71/-46sentinels from non-negativeX509_V_ERR_*codes; ownedBox<[u8]>copy is required sincessl_dispatch_parked_reason'sreasonis a stack buffer.ERR_reason_error_stringreturning NULL is handled (reason_bytes()→b""→node_error_code()falls back toEPROTO; FetchTasklet falls back to"TLS handshake failed").tls_handshake_errorplumbing throughInternalState/HTTPClientResult/detach_lifetimemirrors the existingdns_hostnameside-channel exactly.- Test covers both the direct uSockets path and the SSLWrapper inner-TLS path; both prior nits (try/finally ordering,
ERR_SSL_prefix length) addressed.
Extended reasoning...
Overview
This PR fixes fetch() misreporting non-certificate TLS handshake failures (ALPN mismatch, non-TLS peer behind https://, mid-handshake close) as UNKNOWN_CERTIFICATE_VERIFICATION_ERROR. It captures the OpenSSL reason from the uSockets handshake-failure sentinel and surfaces Node-compatible ERR_SSL_<REASON> codes. Touches 9 files: a new TLSHandshakeError struct in HTTPCertError.rs, a new Error::TLSHandshakeFailed variant, error_no < 0 branching in both HTTPContext::on_handshake and ProxyTunnel::on_handshake, ERR_peek_last_error-before-clear in SSLWrapper::update_handshake_state, plumbing through InternalState/HTTPClientResult/lib.rs, and mapping in FetchTasklet.rs. ~268 net lines including a 3-scenario test.
Security risks
Low. This only changes which error is reported when a TLS handshake fails — it does not touch the success path, certificate verification logic, rejectUnauthorized gating, or checkServerIdentity. A wrong error code here is a diagnostic issue, not an auth bypass. The ProxyTunnel hoist of did_have_handshaking_error before the handshake_success branch makes the failure branch actually reachable (previously fell through to ConnectionRefused), which is strictly more accurate reporting.
Level of scrutiny
Moderate. The change is error-path-only and well-tested, but it spans the HTTP client's TLS handshake handlers, the SSLWrapper BoringSSL state machine, and cross-thread result plumbing. The SSLWrapper change introduces new BoringSSL FFI calls (ERR_peek_last_error/ERR_reason_error_string) inside the laundered-self update_handshake_state path, and ProxyTunnel::on_handshake gains a state write (this.state.tls_handshake_error = ...) inside the aliasing-sensitive callback shape. I traced the NLL/aliasing contracts at both sites and they hold (the &mut HTTPClient borrow ends before close_from_callback), but this is exactly the kind of code where a maintainer familiar with the ProxyTunnel aliasing discipline should confirm.
Other factors
- Both prior nits I raised (server
.listen()insidetry, deriving theERR_SSL_prefix length) were addressed in b2b8aa2 and a944c97. - The test exercises all three failure shapes (ALPN alert via direct uSockets path, non-TLS peer via direct path, garbage-in-tunnel via
SSLWrapperpath) and asserts specificERR_SSL_*codes rather than just "not a cert error". - I checked that
ERR_reason_error_stringreturning NULL degrades cleanly (reason_bytes()→ empty →node_error_code()returnsEPROTO, message falls back to the static string). - The
tls_handshake_errorfield follows the exact same owned-side-channel pattern asdns_hostnamethrough everyHTTPClientResultconstruction/detach site inlib.rs. - The pre-existing
HTTPCertError::from_verify_errorstill runs on the sentinel struct (widening a stack-bufferreasonto&'static ZStr), but only.error_nois read from it in the new negative branch; the owned copy comes from the rawssl_error— no new unsoundness introduced.
Deferring because 9 files across TLS handshake error handling + a ProxyTunnel behavior change is beyond "simple/mechanical", not because anything looks wrong.
|
CI on a944c97 (build 76832): 194/196 jobs passed. The new The two remaining The five annotated failures ( Ready for review; the only red is the darwin arm64 agent outage. |
1 participant
Non-certificate TLS handshake failures in
fetch()were being reported asUNKNOWN_CERTIFICATE_VERIFICATION_ERROR, even withtls: { rejectUnauthorized: false }where a certificate error is impossible by construction. This sends operators hunting through CA stores when the actual fault is an ALPN mismatch, a non-TLS peer behind anhttps://URL, or a mid-handshake connection close. Node reports the same scenarios with the OpenSSL reason (ERR_SSL_WRONG_VERSION_NUMBER,ERR_SSL_TLSV1_ALERT_NO_APPLICATION_PROTOCOL, ...).Reproduction
Before:
After:
Cause
uSockets already dispatches these failures with a non-X509 sentinel:
ssl_dispatch_parked_reasoninpackages/bun-usockets/src/crypto/openssl.csends{error_no: -71, code: "EPROTO", reason: ERR_error_string_n(...)}for a fatal protocol error, andssl_trigger_handshake_econnresetsends{error_no: -46, code: "ECONNRESET"}for a mid-handshake close. The HTTP client's handshake-failure branch inHTTPContext::on_handshakeandProxyTunnel::on_handshakefed the negativeerror_nointoget_cert_error_from_no, whoseX509_V_ERR_*lookup table maps any unknown value toUNKNOWN_CERTIFICATE_VERIFICATION_ERROR.The proxy-tunnel inner-TLS path had two additional defects:
SSLWrapper::update_handshake_statecleared the BoringSSL error queue before the handshake callback fired, discarding the reason; andProxyTunnel::on_handshakeonly setdid_have_handshaking_errorinside its success branch, so a failed inner handshake always fell through toConnectionRefused.Fix
src/http/HTTPCertError.rs: newTLSHandshakeErrorstruct that captures an owned copy of the sentinel'scode/reason(the EPROTOreasonis a stack buffer in uSockets, so an owned copy is required to outliveon_handshake), andnode_error_code()which derivesERR_SSL_<REASON>from the last:-separated segment of the OpenSSL error string, matching Node'sThrowCryptoError.src/http/HTTPContext.rs,src/http/ProxyTunnel.rs: in the handshake-failure branch, a negativeerror_no(X509_V_ERR_*values are all non-negative) records aTLSHandshakeErroron the client state and fails with the newError::TLSHandshakeFailedinstead of routing through the X509 lookup.ProxyTunnel::on_handshakenow hoistsdid_have_handshaking_errorbefore branching on success, matchingHTTPContext.src/uws/lib.rs:SSLWrapper::update_handshake_statepeeksERR_peek_last_error()before clearing the queue onSSL_ERROR_SSLand dispatches the same-71/EPROTO shape uSockets does, withreasonfrom BoringSSL's staticERR_reason_error_stringtable. Mirrorsssl_park_fatal_reason/ssl_dispatch_parked_reasoninopenssl.c.src/http/{error.rs, InternalState.rs, lib.rs}: add theTLSHandshakeFailedvariant and plumbtls_handshake_errorthroughInternalStateandHTTPClientResult(same side-channel shape asdns_hostname).src/runtime/webcore/fetch/FetchTasklet.rs: whenfail == TLSHandshakeFailed, reportcode = node_error_code()andmessage = reason.Testing
New test in
test/js/web/fetch/fetch.tls.test.tsstands up three hostile peers (a TLS server offering only an ALPN protocol fetch never advertises, a plain TCP server behindhttps://, and a CONNECT proxy that feeds garbage into the tunnel) and asserts each rejects withcode: /^ERR_SSL_/and a message carrying the OpenSSL reason; none may surface a certificate verification error. Fails on the unfixed build with exactly theUNKNOWN_CERTIFICATE_VERIFICATION_ERROR/ConnectionRefusedmislabels above.Supersedes #34182 and subsumes #31950: those map the sentinel to a generic
ERR_TLS_HANDSHAKE_FAILEDorEPROTOcode without the OpenSSL reason. This PR carries the reason through to a Node-compatibleERR_SSL_*code.[review] gate passed · iteration 0 · 9 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 0
evidence per changed file