node:tls: accept Ed25519 server certificates during client verification#32235
node:tls: accept Ed25519 server certificates during client verification#32235robobun wants to merge 1 commit into
Conversation
|
Updated 9:01 PM PT - Jun 16th, 2026
✅ @robobun, your commit 2f40188bc708026cde6e1b62e7fc5ea4f1367b4c passed in 🧪 To try this PR locally: bunx bun-pr 32235That installs a local version of the PR into your bun-32235 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a BoringSSL/OpenSSL helper that sets verification signature-algorithm preferences (including Ed25519 and P-521), applies it when building SSL_CTX for standard TLS and QUIC clients, and adds tests validating connections for Ed25519, RSA, and ECDSA server certificates. ChangesEd25519 certificate signature algorithm verification
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![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 `@packages/bun-usockets/src/quic.c`:
- Around line 1127-1128: Add a QUIC/HTTP3 client test that covers the
us_create_quic_client_context(...) code path and verifies an Ed25519 server
certificate so this SSL_CTX change cannot regress; specifically, extend the
existing TLS client test matrix to include an HTTP/3/QUIC client case (mirroring
the tls.connect() TCP case) that performs a QUIC connection to the test server
using us_create_quic_client_context(...) and asserts the server certificate is
accepted when signed with Ed25519 (use the same test certificate fixtures and
expectations as the TCP Ed25519 case). Ensure the new test exercises the
QUIC-only code path that calls us_ssl_ctx_set_verify_signature_algorithms(ssl)
and fails if Ed25519 verification is broken.
In `@test/js/node/tls/node-tls-connect.test.ts`:
- Around line 678-684: The test currently calls server.close() in the finally
block without awaiting it; change the cleanup to await the server shutdown by
replacing server.close() with awaiting its callback (e.g. await new
Promise<void>(resolve => server.close(resolve))) so the finally block only
returns after the TLS server and accepted sockets are fully closed; update the
finally in the test that references the server variable accordingly.
🪄 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: 7d509436-aae2-4592-8cae-9cb1b81a8a34
📒 Files selected for processing (3)
packages/bun-usockets/src/crypto/openssl.cpackages/bun-usockets/src/quic.ctest/js/node/tls/node-tls-connect.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for addressing the P-521 gap and the test cleanup — the change looks correct and well-tested to me, but since it now hardcodes the verify sigalg allowlist for every TLS/QUIC connection in Bun, I'd like a human to sign off on the final list.
Extended reasoning...
Overview
This PR adds an explicit SSL_CTX_set_verify_algorithm_prefs call in us_ssl_ctx_build_raw (openssl.c) and in us_create_quic_client_context (quic.c), supplying a hardcoded list of accepted peer signature algorithms — BoringSSL's default kVerifySignatureAlgorithms plus SSL_SIGN_ED25519 and SSL_SIGN_ECDSA_SECP521R1_SHA512. It adds a 4-algorithm (ed25519/rsa/ecdsa/p521) tls.connect matrix test and an HTTP/3 Ed25519 regression test.
Security risks
The change widens the set of signature algorithms the TLS client will accept when verifying a server (or, in mTLS, a client) certificate. Ed25519 and ECDSA P-521 are both standard, secure algorithms that OpenSSL and Node.js already accept by default, so this aligns Bun with Node rather than weakening security. The list otherwise mirrors BoringSSL's existing defaults verbatim. I don't see an injection, auth-bypass, or downgrade vector here. The notable risk surface is that Bun now owns this list explicitly: future BoringSSL changes to its default verify list will no longer flow through automatically.
Level of scrutiny
This is core TLS/crypto code on the certificate-verification path, executed for every node:tls / Bun.connect / fetch / node:https / HTTP/3 connection. Per the approval guidelines I shouldn't auto-approve security-sensitive crypto changes even when they look correct — the policy decision to take ownership of the verify sigalg list (and exactly which entries belong in it) is worth a human reviewer's eyes.
Other factors
All prior review feedback (my P-521 note, CodeRabbit's await-close and QUIC-regression asks) is addressed in 33ea391. The bug-hunting system found no issues on the latest revision. The CI failure looks like an unrelated musl LTO linking problem, not this change. Test coverage is good and proves no regression for RSA/ECDSA.
|
CI status for maintainers: the diff is green; the red lanes are unrelated flaky/environment tests, not this change. This PR only touches BoringSSL verify-sigalgs (
None touch the signature-algorithm code this PR changes. I have already used my one retrigger (62259), so these need a re-run on green infra rather than another retrigger from me. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Cna you double check this list matches what node does?
|
Good call, it did not match, and I adjusted it. I parsed the raw ClientHello each runtime emits and compared the Node v24 (OpenSSL 3) sends, in order: My list before led with BoringSSL has no This is now a strict subsequence of Node’s list (identical order, only the 4 BoringSSL-unsupported families missing). Dropping sha1 means Bun no longer accepts SHA-1-signed peer signatures, matching Node (and TLS 1.3 forbids sha1 anyway). Rebuilding to confirm the emitted bytes against the Node capture. |
|
Confirmed after rebuild, parsing the actual ClientHello bytes each runtime emits: Bun’s No test regressions from the reorder + sha1 drop: node-tls connect/cert/server (61 pass), the ed25519/rsa/ecdsa/p521 matrix (4 pass), and the h3 suite incl. the Ed25519 case (51 pass). Pushed in 95803e4. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bun-usockets/src/crypto/openssl.c (1)
457-490:⚠️ Potential issue | 🟠 MajorCheck
SSL_CTX_set_verify_algorithm_prefsreturn value, matching the cipher-list error handling.The helper
us_ssl_ctx_set_verify_signature_algorithms()discards the return value ofSSL_CTX_set_verify_algorithm_prefs()by returningvoid. In the same file,SSL_CTX_set_cipher_list()is checked withif (!)and fails the context on error — apply the same pattern here. If signature-algorithm setup fails silently, the context retains BoringSSL's default list, recreating the Ed25519/P-521 verification failure this PR fixes. Change the helper to returnint(matching BoringSSL's actual return type), check both call sites (openssl.c:510 inus_ssl_ctx_build_rawand quic.c:1128), and callssl_ctx_build_fail()or equivalent on failure.🤖 Prompt for 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. In `@packages/bun-usockets/src/crypto/openssl.c` around lines 457 - 490, The function us_ssl_ctx_set_verify_signature_algorithms() currently returns void and does not check the return value of SSL_CTX_set_verify_algorithm_prefs(), allowing silent failures that leave BoringSSL's default algorithm list in place, recreating the Ed25519/P-521 verification issue. Change the function to return int (matching BoringSSL's actual return type for SSL_CTX_set_verify_algorithm_prefs), check its return value, and call ssl_ctx_build_fail() or equivalent on failure. Update both call sites in us_ssl_ctx_build_raw and the corresponding call in quic.c to check the return value of us_ssl_ctx_set_verify_signature_algorithms() and handle errors consistently with how cipher-list setup errors are currently handled.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@packages/bun-usockets/src/crypto/openssl.c`:
- Around line 457-490: The function us_ssl_ctx_set_verify_signature_algorithms()
currently returns void and does not check the return value of
SSL_CTX_set_verify_algorithm_prefs(), allowing silent failures that leave
BoringSSL's default algorithm list in place, recreating the Ed25519/P-521
verification issue. Change the function to return int (matching BoringSSL's
actual return type for SSL_CTX_set_verify_algorithm_prefs), check its return
value, and call ssl_ctx_build_fail() or equivalent on failure. Update both call
sites in us_ssl_ctx_build_raw and the corresponding call in quic.c to check the
return value of us_ssl_ctx_set_verify_signature_algorithms() and handle errors
consistently with how cipher-list setup errors are currently handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e30cc620-2137-4d3f-85f0-56060b3f5108
📒 Files selected for processing (1)
packages/bun-usockets/src/crypto/openssl.c
|
Addressed the verify-sigalgs return-value check in 92bca83: |
There was a problem hiding this comment.
No issues found and my earlier P-521 note was addressed — but this now hardcodes the verify signature_algorithms list for every TLS/QUIC context (and drops rsa_pkcs1_sha1 vs BoringSSL's previous default), so it's worth a maintainer sign-off before merge.
Extended reasoning...
Overview
The PR adds a static us_verify_signature_algorithms[] list in packages/bun-usockets/src/crypto/openssl.c and applies it via SSL_CTX_set_verify_algorithm_prefs in both us_ssl_ctx_build_raw (every node:tls / Bun.connect / fetch / node:https context) and us_create_quic_client_context in quic.c. This makes Bun's TLS client advertise Ed25519 and ECDSA P-521 in the signature_algorithms extension so it can verify servers presenting those leaf certs (fixing #32234). Tests add a 4-algorithm matrix in node-tls-connect.test.ts and an HTTP/3 Ed25519 regression in fetch-http3-client.test.ts. Since my last comment the list was also reordered to be a strict subsequence of Node/OpenSSL's order, rsa_pkcs1_sha1 was dropped to match Node, and the helper now returns the BoringSSL result so callers fail the context on error rather than silently falling back.
Security risks
This is squarely security-sensitive: it replaces BoringSSL's curated default verify-sigalg list with a Bun-owned hardcoded one for every TLS handshake. The additions (Ed25519, P-521) are widely accepted and match Node/OpenSSL, so they don't weaken the trust posture. The notable behavior change is removing SSL_SIGN_RSA_PKCS1_SHA1 from what Bun previously advertised — that's a hardening (and matches Node), but it is a user-visible change for anyone connecting to a TLS 1.2 peer that can only sign with SHA-1. I don't see an injection/auth-bypass vector; the error-path change in quic.c returns NULL from us_create_quic_client_context on failure, which callers should already handle since SSL_CTX_new could already fail there.
Level of scrutiny
Moderate-to-high. The diff is small and well-commented, the root cause is clearly explained, and the test matrix proves both the fix and non-regression for RSA/P-256. But because the new array now governs the verify sigalgs for every SSL_CTX Bun creates (client and mTLS-server), and because it intentionally diverges from BoringSSL's default (adds two, drops one), a maintainer should confirm the policy choice — particularly the SHA-1 drop and that owning this list going forward is the desired approach vs. e.g. only appending to BoringSSL's default.
Other factors
All prior review threads (CodeRabbit's QUIC-test request, awaited server.close, my P-521 note) are resolved in the current diff. The author verified the emitted ClientHello bytes against Node's and confirmed the full TLS matrix + h3 suite stay green; the only red CI lanes are an unrelated musl LTO build infra issue. No CODEOWNERS file restricts these paths. Given the security-sensitive scope I'm deferring rather than approving.
robobun
force-pushed
the
farm/a5715f05/tls-ed25519-verify-sigalgs
branch
from
92bca83 to
8831729
Compare
BoringSSL's default verify signature-algorithm list omits Ed25519 and ECDSA P-521, so Bun's TLS client never advertised them in the signature_algorithms extension and could not verify a server presenting such a leaf: the handshake failed, the peer certificate came back empty, and hostname verification rejected with "Cert does not contain a DNS name". RSA and ECDSA P-256/P-384 were unaffected; Node verifies all. Set explicit verify signature-algorithm preferences on the client SSL_CTX, matching Node.js/OpenSSL's default order restricted to the algorithms BoringSSL implements. Applied once in the shared us_ssl_ctx_build_raw (node:tls / Bun.connect / fetch / node:https) and in the QUIC client context, so Ed25519/P-521 server and mTLS client certificates are accepted everywhere. The helper returns the BoringSSL result so callers fail the context on error instead of silently falling back to the default list. Tests: a tls.connect matrix (ed25519/rsa/ecdsa/p521) in node-tls-connect.test.ts and an HTTP/3 Ed25519 regression in fetch-http3-client.test.ts. Fixes #32234
robobun
force-pushed
the
farm/a5715f05/tls-ed25519-verify-sigalgs
branch
from
8831729 to
2f40188
Compare
2 participants
Fixes #32234
Problem
A TLS client connecting to a server that presents an Ed25519 certificate always fails hostname verification:
RSA and ECDSA P-256/P-384 certificates work; Node verifies all of them. The peer certificate comes back empty (
subjectaltname: undefinedeven in detailed mode), so the internalcheckServerIdentityfinds no DNS names and rejects withERR_TLS_CERT_ALTNAME_INVALID. Real-world impact: connecting to servers whose PKI issues Ed25519 certs (e.g. NATS behind cert-manager) works under Node but breaks under Bun.During review this turned out to affect ECDSA P-521 leaf certificates too, for the same underlying reason.
Root cause
BoringSSL's default accepted signature-algorithm list for verifying a peer certificate (
kVerifySignatureAlgorithmsinssl/extensions.cc) omitsSSL_SIGN_ED25519andSSL_SIGN_ECDSA_SECP521R1_SHA512, even though its signing list (kSignSignatureAlgorithms) includes both. Bun never overrode this, so the client never advertised Ed25519 or P-521 in thesignature_algorithmsextension. A TLS 1.3 handshake against an Ed25519 or P-521 leaf then could not complete; the connection degraded with an empty peer certificate, which surfaced as the bogus hostname error. This also explains why Bun's server presents Ed25519 fine (it signs) while its client cannot verify it. OpenSSL (and therefore Node) accepts both by default.Fix
Set explicit verify signature-algorithm preferences on the client
SSL_CTX(BoringSSL's default list plusSSL_SIGN_ED25519andSSL_SIGN_ECDSA_SECP521R1_SHA512), matching the OpenSSL defaults Node relies on. The list is applied once in the sharedus_ssl_ctx_build_rawinpackages/bun-usockets/src/crypto/openssl.c, which backs every node:tls /Bun.connect/ fetch / node:https context, and reused for the QUIC client context inquic.cso HTTP/3 stays consistent. Because theSSL_CTXis mode-neutral, the same list governs verifying client certificates, so a Bun server doing mTLS now accepts these client certs too, matching Node.The fixing line is the
SSL_CTX_set_verify_algorithm_prefscall (via the sharedus_ssl_ctx_set_verify_signature_algorithmshelper); the rest is the algorithm list and its reuse in QUIC.Verification
test/js/node/tls/node-tls-connect.test.ts: a matrix test stands up atls.createServerwith an Ed25519, RSA, ECDSA P-256, and ECDSA P-521 leaf (eachsubjectAltName=DNS:localhost,IP:127.0.0.1) and connects withtls.connect({ ca, servername: "localhost" }), assertingauthorized === trueandgetPeerCertificate(true).subjectaltname === "DNS:localhost, IP Address:127.0.0.1". Before the fix, Ed25519 and P-521 fail (ERR_TLS_CERT_ALTNAME_INVALID) while RSA and P-256 pass, proving the fix does not regress them.test/js/web/fetch/fetch-http3-client.test.ts: an HTTP/3 test connects to an Ed25519 server viafetch(url, { protocol: "http3" }), which exercises the QUIC client context. Without the fix the h3 handshake fails withHTTP3HandshakeFailedduringsignature_algorithmsnegotiation (before the verify callback, sorejectUnauthorized: falsecannot mask it); with the fix it returns 200.Rebased onto main (resolving conflicts from #31155) and squashed into a single commit; the empty
ci: retriggercommit was dropped. Conflict resolution: #31155 added explicitminVersion/maxVersionhandling inus_ssl_ctx_build_rawat the same spot this PR adds the verify-sigalgs call, so its version handling is kept and the sigalgs call runs right after it (the previously-hardcodedTLS1_2_VERSIONline is dropped in favor of #31155s option-aware one). Innode-tls-connect.test.tsboth branches appended tests at end-of-file; both are kept. Matrix + h3 tests verified green on the rebased tree.