tls: reject certificate chains containing RSA keys below 2048 bits - #38591
tls: reject certificate chains containing RSA keys below 2048 bits#38591robobun wants to merge 3 commits into
Conversation
BoringSSL's X.509 verifier has no minimum public key size, so a chain
built through an RSA-512 intermediate, an RSA-1024 leaf or an RSA-1024
trust anchor verified cleanly on every Bun client and server path.
Node (OpenSSL at its default security level 2) rejects all of them.
uSockets now verifies every peer chain through
us_internal_x509_verify_cert, installed on each SSL_CTX with
SSL_CTX_set_cert_verify_callback and shared with the QUIC client's
custom verify. After a clean X509_verify_cert it walks the built chain
and fails any certificate whose RSA key is below 2048 bits or whose EC
key is below 224 bits, reporting the failure through the connection's
verify callback exactly like one of BoringSSL's own errors, so the
rejectUnauthorized / inline-reject / authorizationError flows are
unchanged. node:tls and Bun.connect report code UNSPECIFIED with
OpenSSL's reason strings, matching node; fetch reports
CA_KEY_TOO_SMALL / EE_KEY_TOO_SMALL.
test/js/node/tls/fixtures/{ca2,agent3,agent10}-cert.pem were the
pre-2024 node fixtures anchored at a 1024-bit ca2; they are replaced
with node's current versions (same keys), already vendored under
test/js/node/test/fixtures/keys.
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (11)
📒 Files selected for processing (10)
Comment |
|
Status: fixed; the diff is green on every lane that ran it, and CI is blocked on infrastructure rather than on this change. Reproduced against the released binary (1.4.0-canary) with a Bun server presenting each of the three fixture chains (RSA-512 intermediate, RSA-1024 leaf, RSA-1024 trust anchor): CI: build 96249 (first revision) passed 177 of 177 executed lanes; its macOS 14 arm64 test job expired in the agent queue. Build 96678 (current revision, comment-only delta) passed 161 lanes; the Windows arm64 and Windows 2019 x64 test jobs were cancelled because agents could not be created (leaked arm64 runner VMs have the quota pinned, reported separately), and one Debian x64 ASAN agent timed out three unrelated tests ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the TLS certificate-verification path for every connection (installing SSL_CTX_set_cert_verify_callback on every context) and tightens a user-facing security policy — RSA-1024 chains that Bun previously accepted will now fail — a maintainer should confirm the 2048-bit floor (vs. the browser-style 1024 floor the description offers as an alternative) and the overall approach.
What was reviewed:
us_internal_x509_verify_certwrapping and its interaction withus_verify_callback/ the inline-reject callback — the policy error is set on the store ctx and the per-connection callback still decides whether the handshake continues.- Error-code plumbing: 1001/1002 fall through
us_X509_error_codetoUNSPECIFIED(matching Node) and are named on the fetch path. - DSA/Ed25519 handling in
us_x509_key_too_small— the default-accept for non-RSA/EC key types was examined and ruled out as a gap. - Fixture refresh (
ca2/agent3/agent10) keeps the same subjects and private keys, so existing tests that load them still verify.
Extended reasoning...
Overview
This PR closes a real security gap: BoringSSL's X.509 verifier has no key-size floor, so Bun accepted TLS chains built through RSA-512/1024 keys that Node (via OpenSSL security level 2) rejects. The fix wraps X509_verify_cert with a post-verification chain walk that rejects RSA < 2048 and EC < 224, installed on every SSL_CTX via SSL_CTX_set_cert_verify_callback and called directly from the QUIC client's custom verify. Error reporting matches Node's UNSPECIFIED code with OpenSSL's reason strings on node:tls, and adds named EE_KEY_TOO_SMALL/CA_KEY_TOO_SMALL codes on the fetch path. Three existing 1024-bit test fixtures are refreshed to Node's current 2048-bit versions, and eight new fixtures plus 15 tests cover weak intermediates, leaves, and trust anchors as both server and client certificates.
Security risks
This is a security hardening that fails closed — the change only adds rejection cases on top of a chain that already verified. The implementation only runs the policy check after X509_verify_cert returns success and the store ctx error is still X509_V_OK, so it cannot mask a real verification failure or turn one into a success. The verify-callback dispatch mirrors how BoringSSL surfaces its own errors, preserving the existing rejectUnauthorized / authorized / inline-reject flows. The DSA-falls-through concern was examined: DSA is not in BoringSSL's default sigalgs and is effectively unusable in TLS 1.2+, so accepting it in us_x509_key_too_small is not a practical bypass. No new attack surface is introduced.
Level of scrutiny
High. This installs a cert_verify_callback that replaces BoringSSL's default verification entry point on every SSL_CTX built by uSockets, affecting tls.connect, https, fetch, Bun.connect, Bun.serve, WebSocket, and the duplex/named-pipe TLS wrappers. It also changes user-visible behavior: RSA-1024 certificates that previously worked will now be rejected. The PR description explicitly flags the 2048 vs. 1024 threshold as a maintainer decision. Security-policy changes of this scope should be signed off by a human.
Other factors
The implementation is careful and well-documented, test coverage is thorough (positive and negative controls, both client- and server-side chains, both tls.connect and fetch, fixture-generation recipe recorded), and the PR description enumerates every path it covers and the one it does not (node:quic's Rust-built contexts). No bugs were found in the automated review. The fixture refresh is a mechanical swap to Node's current copies of the same three files. Nonetheless, the combination of security-critical C code, a breaking behavior change, and an open policy question makes this unsuitable for auto-approval.
|
No code changes from the automated review; the one thing it asks a maintainer to confirm is the threshold, which is deliberately left as a single decision point:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it modifies the core TLS certificate-verification path (installing an SSL_CTX_set_cert_verify_callback on every context) and introduces a user-visible behavior change whose threshold the author explicitly flagged for maintainer confirmation, a human look is warranted before merge.
What was reviewed:
us_internal_x509_verify_cert— the wrapper only tightens afterX509_verify_certsucceeds and preserves an error a verify callback already waved through, so it fails closed and doesn't loosen anything.- Error-code plumbing (1001/1002) — checked they don't collide with BoringSSL's
X509_V_ERR_*range and are mapped consistently acrossus_X509_error_reason,get_cert_error_from_no, andFetchTasklet. - Fixture refresh — the replaced
ca2/agent3/agent10certs keep the same subjects/keys and the existing tests in the two touched files still pass. - The comment-cop flag on
src/http/lib.rs:1517is on a pre-existing doc comment that this PR only extended by a few words; it does not describe a workaround.
Extended reasoning...
Overview
This PR hardens TLS peer-chain verification by rejecting certificates with RSA keys < 2048 bits or EC keys < 224 bits, matching Node's OpenSSL security-level-2 default. It touches packages/bun-usockets/src/crypto/openssl.c (the new us_internal_x509_verify_cert wrapper and SSL_CTX_set_cert_verify_callback installation), quic.c (routes the QUIC client's manual verify through the same function), libusockets.h / internal.h (two new US_X509_V_ERR_* codes and the internal prototype), and Rust-side error mapping in src/http/{lib,error}.rs and FetchTasklet.rs. It also refreshes three vendored Node fixtures from 1024-bit to their current 2048-bit versions, adds eight new weak-key fixtures, adds test suites in node-tls-cert.test.ts and fetch-tls-cert.test.ts, and adds one sentence to the fetch docs.
Security risks
The change is security-hardening, and the implementation only adds checks after X509_verify_cert has already returned success with X509_V_OK, so it cannot make an invalid chain valid. However, it replaces the cert-verify entry point on every SSL_CTX Bun builds, so any subtle mistake in how the verify callback is threaded (e.g. SSL_get_verify_callback retrieval, error propagation to SSL_get_verify_result) would affect all TLS connections. I traced the flow and it looks correct: the policy error is set on the store ctx, then delivered through the connection's own verify callback exactly as BoringSSL delivers native X509_V_ERR_* values, so us_verify_callback / the inline-reject callback / rejectUnauthorized semantics are preserved. The new error numbers (1001/1002) are outside BoringSSL's 0–67 range so X509_verify_cert_error_string and us_X509_error_code fall through to their defaults as intended.
Level of scrutiny
High. This is the TLS trust path — the guidelines explicitly exclude crypto/auth code from auto-approval. Beyond that, the PR itself surfaces a policy decision for a maintainer: whether the floor should be 2048/224 (Node-compat, as pushed) or 1024/160 (browser-compat). That is a product-level call about how much real-world breakage (RSA-1024 chains still exist in some private/enterprise PKIs) is acceptable, and the author left it as a one-constant knob precisely so a maintainer can weigh in.
Other factors
- The github-actions comment-cop flagged
src/http/lib.rs:1517; that flag is on the pre-existing three-line doc comment aboveget_cert_error_from_no, which this PR only minimally reworded. It's not a paragraph justifying a workaround, so I read it as a false positive rather than something requiring a code change. - Test coverage is thorough (three weak-chain shapes × three delivery modes + two positive controls, plus fetch coverage), and the gate evidence shows the new tests fail on main and pass on the PR under both ASAN and release.
- Because the fixture refresh touches
ca2/agent3/agent10, other TLS tests across the tree that consume those fixtures are indirectly affected; the PR body reportstest/js/node/tls/passes, but that's another reason for a human/CI check.
|
Pushed bd4f8f0: the comment block above The second automated review also found nothing to change; the only open item remains the 2048 vs 1024 floor noted above. The earlier CI build (96249) had every executed lane green, with only the macOS 14 arm64 test lane expiring in the agent queue, so the retrigger push is there to get that lane a run. |
There was a problem hiding this comment.
I reviewed this PR and the automated bug hunt found no issues. Because it rewires the TLS certificate-verification path (installing SSL_CTX_set_cert_verify_callback on every uSockets context) and introduces a user-visible behavior change whose threshold the PR itself flags for maintainer sign-off (2048-bit RSA / 224-bit EC vs. a browser-style 1024 floor), a human review is still warranted.
What was reviewed:
us_internal_x509_verify_cert: only runs the policy afterX509_verify_certsucceeds and the store-ctx error is stillX509_V_OK, so it can't mask a real BoringSSL failure or override an error a verify callback already waved through; sets the error on the store ctx and re-invokes the connection's verify callback sorejectUnauthorized/ inline-reject semantics are preserved.- Error-code plumbing: the new
US_X509_V_ERR_*values (1001/1002) are outside BoringSSL's 0–67 range and are mirrored inget_cert_error_from_no;us_X509_error_codefalls through toUNSPECIFIED(matching Node), andus_X509_error_reasonsupplies OpenSSL's wording. - DSA keys falling through
us_x509_key_too_smallwas raised and ruled out — BoringSSL's verifier already rejects DSA-keyed certificates before this policy runs. - Fixture refresh: the replaced
ca2/agent3/agent10PEMs keep the same subjects and private keys (only the CA key size changed), so existing tests that reference them by CN/key are unaffected.
Extended reasoning...
Overview
This PR closes a Node-compat security gap: BoringSSL has no OpenSSL-style "security level", so Bun accepted TLS chains built through arbitrarily weak RSA/EC keys (including factorable RSA-512 intermediates). The fix wraps X509_verify_cert in us_internal_x509_verify_cert, which walks the verified chain and rejects any certificate with an RSA key < 2048 bits or EC key < 224 bits. The wrapper is installed on every SSL_CTX built by us_ssl_ctx_build_raw via SSL_CTX_set_cert_verify_callback, and the QUIC client's hand-rolled verify is switched to call the same wrapper. Two new verify-result codes are plumbed through libusockets.h, src/http/{lib,error}.rs, and FetchTasklet.rs; us_X509_error_reason supplies OpenSSL's reason strings so node:tls messages match Node. Three pre-2024 Node fixtures (ca2/agent3/agent10) are refreshed to their 2048-bit versions, eight new weak-key fixtures are added, and 15 new test cases cover tls.connect (both rejectUnauthorized states), server-side client-cert verification, and fetch.
Security risks
This is squarely security-sensitive code — it replaces the certificate-verification entry point for every TLS connection Bun makes or accepts. The direction is a strict tightening (fail closed), and the wrapper is careful to only add restrictions after BoringSSL's own verify succeeded with X509_V_OK, so it cannot accidentally accept a chain BoringSSL would have rejected. The policy failure is delivered by setting the store-ctx error and calling the connection's existing verify callback with ok=0, which preserves the us_verify_callback / inline-reject / SSL_get_verify_result flow. I didn't find a bypass, but any change at this layer deserves a maintainer's eyes.
Level of scrutiny
High. Beyond the security-critical path, this is an intentional user-visible behavior change: RSA-1024 chains that Bun currently accepts will start failing. The PR author explicitly leaves the 2048/224 threshold as a maintainer decision (vs. a browser-style 1024 floor), and the robobun follow-up comment reiterates that a maintainer should confirm it. That's a product/compat call an automated review shouldn't make.
Other factors
Test coverage is thorough (control chains, three weak-key positions, both client and server verification, both tls.connect and fetch) and the gate evidence shows 12 failures on the unfixed build vs. clean on the fix. The comment-cop bot flagged src/http/lib.rs, but the actual diff there is two one-line trailing comments pointing at the constant definitions (added in commit bd4f8f0), not a paragraph-length justification — that flag looks like noise. The one candidate the finder agents raised (DSA keys not checked) was correctly ruled out: BoringSSL's chain builder rejects DSA-keyed certificates outright, so the policy check never sees one.
|
For the record on the DSA question the latest automated review raised and dismissed, the exact reason it cannot reach this check: BoringSSL still parses DSA SubjectPublicKeyInfos, but its EVP DSA method has no signing or verification implementation ( |
|
Updated 5:04 PM PT - Aug 14th, 2026
❌ @robobun, your commit bd4f8f0 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 38591That installs a local version of the PR into your bun-38591 --bun |
2 participants
Problem
root (RSA-2048, trusted via ca) -> intermediate CA with an RSA-512 key -> leafis authorized on every client path (tls.connectreportsauthorized: true,https/fetchsucceed,Bun.connecthandshakes,wss://opens), and so are an RSA-1024 leaf and an RSA-1024 trust anchor. An RSA-512 key can be factored in hours, after which any name can be minted under that CA and only Bun accepts it.authorized: false,authorizationError: "UNSPECIFIED", and withrejectUnauthorizedthe error is{ code: "UNSPECIFIED", message: "CA certificate key too weak" }(or"EE certificate key too weak"for the leaf). That is OpenSSL's default security level 2 (112-bit: RSA-2048 / P-224), which OpenSSL applies to the leaf and to every issuer up to and including the trust anchor (crypto/x509/x509_vfy.c,check_auth_level/check_key_level).evp.htells callers to bound RSA sizes themselves withEVP_PKEY_bits), and uSockets hands chain verification straight to it (packages/bun-usockets/src/crypto/openssl.c, theSSL_CTX_set_verify/us_verify_callbacksetup). Nothing on Bun's side looked at key sizes.Fix
us_internal_x509_verify_cert(openssl.c) wrapsX509_verify_cert: when the chain verified cleanly it walks the built chain and fails any certificate whose RSA key is below 2048 bits or whose EC key is below 224 bits, with two uSockets-defined results,US_X509_V_ERR_EE_KEY_TOO_SMALL(chain[0]) andUS_X509_V_ERR_CA_KEY_TOO_SMALL(any issuer, trust anchor included). BoringSSL's ownX509_V_ERR_*numbers end at 67 and 66/67 already mean something else there, so OpenSSL's numbers could not be reused.SSL_CTXbuilt byus_ssl_ctx_build_rawinstalls it throughSSL_CTX_set_cert_verify_callback, so it covers all TCP clients and servers (server certs and requested client certs alike, including the duplex / named-pipe TLS wrappers, which build their contexts there too). The QUIC client's custom verify (quic.c), which calls the verifier by hand, calls the same function.X509_STORE_CTXand the connection's verify callback is invoked withok = 0. That is what keeps the existing flows unchanged:us_verify_callbacklets the handshake finish and JS reads the verdict (authorized/authorizationError/rejectUnauthorized), the inline-reject callback from tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598 records it and suppresses the client's final flight, and a context with no callback (the QUIC client's bare store ctx) simply fails. An error an earlier callback already waved through is left as the verdict.us_ssl_socket_verify_error_from_sslsupplies OpenSSL's reason strings for the two codes; the code string falls throughus_X509_error_code's default,UNSPECIFIED, which is also what node reports (node's table has no names for these either).fetch, which names every X509 result, maps them toCA_KEY_TOO_SMALL/EE_KEY_TOO_SMALL(src/http/lib.rs,error.rs,FetchTasklet.rs). One sentence added to the fetch docs.test/js/node/tls/fixtures/{ca2,agent3,agent10}-cert.pemwere the pre-2024 node fixtures, anchored at a 1024-bitca2, so dozens of existing tests intest/js/node/tlsstarted failing with "CA certificate key too weak", exactly as they would on current node. They are replaced with node's current versions of the same three files (same private keys; byte-identical copies of what is already vendored undertest/js/node/test/fixtures/keys). The only other sub-2048 certificate undertest/is node'sagent11fixture, which tests only load into a server and never verify.US_MIN_RSA_KEY_BITSconstant; a browser-style 1024 floor (which would still close the RSA-512/768 hole) is a one-constant change if preferred.test/js/node/tls/node-tls-cert.test.ts("certificate chains containing a weak public key"): for an RSA-512 intermediate, an RSA-1024 leaf and an RSA-1024 anchor,rejectUnauthorized: falsereportsUNSPECIFIED, the default abortstls.connectwith node's code and message, and a server withrequestCertrejects the same chains as client certificates; two all-RSA-2048 controls stay authorized. 9 of 11 cases fail on the unfixed build.test/js/node/tls/fetch-tls-cert.test.ts: the same three server chains reject withCA_KEY_TOO_SMALL/EE_KEY_TOO_SMALLand the control still fetches; 3 of 4 fail on the unfixed build.bun bd test test/js/node/tls/(after the fixture refresh): everything passes except two tests that fail identically on the released binary in this container (localhostresolving to::1for listen but127.0.0.1for connect, and a root-store Worker test that needs more than 5 s on a debug+ASAN build).Bun.connectprobed by hand against the same fixtures: authorized /UNSPECIFIED: CA certificate key too weak/EE certificate key too weak, handshake failure when rejecting; the released binary authorizes all three.test-tls-client-verify,test-tls-client-reject,test-tls-ca-concat,test-tls-peer-certificate,test-https-strict,test-tls-multiple-cas-as-string,test-tls-add-context,test-tls-sni-optionpass on the debug build (node's current fixtures are all 2048-bit or P-256+, so they double as a regression net for the threshold).X509_verify_certthe same way; its digest check slots intous_x509_chain_policy_erroronce either lands. tls: make handshake success match socket.authorized; extend X509 error-code table #35186 proposes naming every X509 code on thenode:tlspaths; if that direction is taken, these two codes should get names there too.node:quicbuilds its ownSSL_CTXin Rust and does not go through uSockets, so it is not covered here.Background
caoption or the bundled roots) and checks signatures, validity, names, and so on.X509_verify_certis that routine;X509_STORE_CTXis its per-verification state, andX509_STORE_CTX_get0_chainreturns the chain it built, leaf first.SSL_set_verify): a per-connection hook BoringSSL calls for each problem it finds during verification; returning 1 continues anyway. Bun always returns 1 and reads the final verdict afterwards withSSL_get_verify_result, so that JS-levelrejectUnauthorizedcan decide. The "inline reject" variant (tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598) additionally notes the failure so a rejecting client never sends its final handshake flight. The error set last on the store ctx is whatSSL_get_verify_resultreturns, which is why this change sets its error there and then calls the callback.SSL_CTX_set_cert_verify_callback: a BoringSSL hook that replaces theX509_verify_certcall itself for every connection on a context (distinct from the per-problem verify callback above, which BoringSSL still passes along). It is how the policy runs once, in one place, for all connections.Repro against the released binary (1.4.0-canary) vs node v26.3.0
Bun server serving each chain (node servers refuse to load them); client probes with
caset to the chain's root.With this change, Bun's output for all three is identical to node's, and
fetchrejects withCA_KEY_TOO_SMALL/EE_KEY_TOO_SMALLcarrying the same messages.openssl verify -auth_level 2gives the same three verdicts (errors 67, 66, 67) and accepts the control chain.[review] gate passed · iteration 4 · 21 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 4
evidence per changed file
root cause · written by the author bot
Bun's TLS chain verification used BoringSSL's default verifier, which unlike Node's OpenSSL security level applies no minimum public-key size, so a chain containing an RSA-512 or similarly weak key as the trust anchor, an intermediate, or the leaf was accepted on every client path. The fix installs a cert-verify wrapper on every context that, after X509_verify_cert succeeds, walks the verified chain and rejects RSA keys under 2048 bits or EC keys under 224 bits with two new error codes that flow through the existing verify callback and rejectUnauthorized handling, with the QUIC client route…