Skip to content

tls: keep what a client sends behind its ClientHello while an SNICallback is pending - #46073

Open
robobun wants to merge 1 commit into
mainfrom
robobun/3ca04097/tls-fd-engine-sni-early-data
Open

robobun wants to merge 1 commit into
mainfrom
robobun/3ca04097/tls-fd-engine-sni-early-data

Conversation

@robobun

@robobun robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A tls.Server whose SNICallback answers later closes a TLS 1.3 client that sends early data: tlsClientError ECONNRESET, no ServerHello. Node completes the handshake. node:tls: ask for the server name on the stream TLS engine #45471 found this, not a user.
  • Cause: SSL_read stops with SSL_ERROR_PENDING_CERTIFICATE, and us_internal_ssl_on_data (openssl.c:2711) closes on any unread byte. The records behind the ClientHello are such bytes.

Fix

  • The read path keeps the unread bytes on the connection, at most 6 + 16384: a ChangeCipherSpec plus the early data BoringSSL skips.
  • After the answer, us_socket_sni_resolve drives the handshake as before, reads the kept bytes through us_internal_ssl_on_data, and closes a socket whose handshake failed.
  • The raw tap moves to the read dispatch in loop.c, so the raw half sees kept bytes once.
  • Verified: node-tls-server.test.ts and socket.test.ts (57 new tests, 51 fail on main). Also test/js/node/tls, test/js/bun/net, 640 vendored Node tests. Self-reviewed: 18 concerns raised, 16 addressed.

Background

  • The fd TLS engine (openssl.c) reads through one BIO per event loop, a window onto one recv(). Nothing can wait in it.
  • us_select_cert_cb answers "retry" while the SNICallback is pending, which suspends the handshake behind the ClientHello.
  • Considered: feeding SSL_do_handshake (misreports a kept close_notify), a read BIO per SSL (every socket pays), no reads while suspended (hides a FIN).

Downsides

  • More than 16390 bytes still close the connection. Node buffers them and lets OpenSSL judge.
  • A suspended connection holds up to 16394 bytes more (21 KiB of RSS measured).
  • Release binary: +512 bytes of text. TLS echo round trip: 1818 → 1803 instructions.
Notes

Where this comes from. #45471 makes the other server TLS engine (SSLWrapper, for a stream that is no net.Socket) ask the SNICallback. Its body says that the fd engine "drops at the first byte" and that this "is reported separately". There is no issue for it. It is not a regression: the suspended handshake arrived with #31155, and 1.4.0 to 1.4.3 close such a client.

Who can hit it. All four conditions must hold.

  1. The server is a tls.Server, an http2 secure server, a server-side new TLSSocket(netSocket, { isServer: true, SNICallback }), or a Bun.listen with the serverName hook. https.createServer does not ask the SNICallback on Bun (0 calls in a probe, 1 on Node).
  2. The SNICallback answers after its call returned.
  3. The client sends TLS 1.3 early data.
  4. The client holds a ticket that allows early data, from another TLS server for the same name. A ticket of Bun or of Node v26.3.0 says Max Early Data: 0 (openssl sess_id). A ticket of openssl s_server -early_data says 16384.

Repro. openssl s_server -tls1_3 -early_data issues a ticket. Then openssl s_client -tls1_3 -servername tenant.test -sess_in <ticket> -early_data <file> (OpenSSL 3.5.7) connects to a tls.createServer whose SNICallback answers 50 ms later.

  • Node v26.3.0: the client prints subject=... CN=agent3, Early data was rejected, served. The server prints secureConnection tenant.test.
  • main: the client prints the subject of its old session and Early data was rejected. The server prints tlsClientError ECONNRESET.
  • This branch: the same lines as Node. http2.createSecureServer answers the recorded flight with a ServerHello too.

What the source diff holds (+73/-20 in openssl.c and loop.c, and a comment in internal.h).

  • The keep. ssl_hold_input is new and cold. The SSL_ERROR_PENDING_CERTIFICATE arm of us_internal_ssl_on_data calls it where the old line closed. SSL_ERROR_WANT_READ shares that arm and cannot leave a byte unread: BIO_s_custom_read reports a retry only for an empty window.
  • The storage. us_ssl_rare_t.sni_held is one block per suspended connection. us_ssl_rare_free frees a block that no answer took. us_ssl_rare_t stays 64 bytes, and a static assert holds that: the renegotiation window start is now uint32_t seconds. time(NULL) has a resolution of one second, so the window is the same.
  • The resume. us_socket_sni_resolve takes the block, calls ssl_update_handshake as before (this sends the flight and arms the retry of a parked write again) and then calls the reader. A handshake that the re-drive ended, by a refusal or by the owner's shutdown(), does not read the block.
  • ssl_close_if_fatal after the re-drive, as in ssl_on_writable. On main, a handshake that fails in the step after the answer (no shared cipher, for example) leaves a Bun.listen socket open until its peer leaves. With the kept bytes, more connections would reach that state. node:tls destroys such a socket itself: its events are the same on main and on this branch, on both doors.
  • The raw tap. us_internal_ssl_on_data has three callers: the recv() in loop.c, us_socket_tls_feed, and now us_socket_sni_resolve. Only the first brings new bytes from the wire, so the tap is there. upgradeTLS arms the tap after its us_socket_tls_feed.

The bound. BoringSSL skips at most 16384 bytes of rejected early data (kMaxEarlyDataSkipped in vendor/boringssl/ssl/tls_record.cc). It discards the ChangeCipherSpec before it counts. So the engine keeps 6 + 16384 bytes, and BoringSSL judges what the engine kept after the answer, as it does for a synchronous SNICallback. Measured with openssl s_client -early_data of OpenSSL 3.5.7, which sends this early data in one record, and a ticket that allows 16384 bytes:

Early data Bun, synchronous SNICallback This branch, asynchronous Node v26.3.0
16362 bytes served served served
16363 bytes ERR_SSL_TOO_MUCH_SKIPPED_EARLY_DATA, alert 10 ECONNRESET before the answer served
16384 bytes ERR_SSL_TOO_MUCH_SKIPPED_EARLY_DATA, alert 10 not run served
  • 16362 bytes of early data in one record are 16384 bytes on the wire. With the ChangeCipherSpec that is the bound. A client that puts the same early data in more records reaches the budget of BoringSSL sooner, with a synchronous SNICallback too.
  • Node sets no bound of its own. 32 MiB of undecryptable records behind a ClientHello give ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC after the answer.
  • node:tls: ask for the server name on the stream TLS engine #45471 keeps 16384 bytes on the stream engine. The pull that lands second takes the number of the first, and folds the two copies of the test harness and of the recorded flight into one door matrix.

Other events while bytes are kept. A probe ran 7 sequences on listen() and on emit("connection", netSocket), each with a ChangeCipherSpec kept and with nothing behind the ClientHello: the answer alone, a handshakeTimeout of 1 ms, a write on the server's socket, destroy(), server.close(), a second answer, and a GC.

  • On this branch the kept bytes change no event in any of the 14, in 3 runs.
  • Against Node v26.3.0, 10 of the 14 have the same events. In 2 of those 10 the order differs between runs, by the race of the 1 ms timer or of the close with the read of the ClientHello.
  • The other 4 differ with nothing kept too, so this change does not bring them. Bun ignores a second answer, and Node reports ERR_MULTIPLE_CALLBACK. A write before the answer differs because 'connection' of a listening server hands out the TLS socket in Bun and the raw socket in Node. After a plaintext write on the raw socket, the server reports ERR_SSL_WRONG_VERSION_NUMBER in Bun and ECONNRESET in Node.

Tests. No test needs the openssl CLI. The rows write records behind the ClientHello of a real client, or replay the first flight of openssl s_client -early_data (320 bytes: a ClientHello that offers early data, a ChangeCipherSpec, one record of early data).

  • node-tls-server.test.ts, describe "what the peer sends before the SNICallback answered", on three doors: listen(), server.emit("connection", netSocket), and the same after the socket read the ClientHello (the bytes then arrive through us_socket_tls_feed). 51 tests run, 45 fail on main. The 6 that pass on main expect a drop before the answer.
  • Rows: a record cut over two reads, and cut over the answer. 16390 bytes kept and 16391 dropped, in one read and summed over two. A refusal, and a peer that leaves while bytes are kept. The same end as with a synchronous answer for a ChangeCipherSpec, an undecryptable record, and a close_notify behind a TLS 1.2 ClientHello. A write that the server made before the answer. A resume nested in the ALPNCallback and in the SNICallback of another connection, and an answer from the 'data' listener of another TLS socket. The recorded flight: the handshake is open at 16384 skipped bytes and over at 16385.
  • One row for a standalone server-side TLSSocket, which reports on the socket itself.
  • socket.test.ts, 5 rows for Bun.listen: a kept ChangeCipherSpec, a handshake that fails on a kept record or in the step after the answer (the engine closes it), and shutdown() before the answer (one report, the record is not read).
  • Mutants of the fix against these tests, on debug builds (failing tests / timeouts): replay deleted 28 / 0, replay run twice 9 / 0, bound + 1 6 / 0, bound - 1 6 / 0, the check for a pending handshake deleted 1 / 0, ssl_close_if_fatal deleted 2 / 0, kept bytes shown to the raw tap 2 / 0, a hold that overwrites 2 / 0, the loop-state restore in us_select_cert_cb deleted 6 / 0. The reset of the read window after the replay decides no test. It keeps the window of the loop from pointing at freed memory.
  • Suites on the debug ASAN build, each file in its own process: test/js/node/tls (28 files), test/js/bun/net (6 files), the 186 vendored test-tls-* and the 454 vendored test-net-*, test-https-*, test-http2-*. No test fails that passes on main. The failures on both are: 5 tests that need the public network, test-tls-client-allow-partial-trust-chain.js, test-https-timeout.js, and tests that pass the 5 s limit of a debug build in the build container (tls.DEFAULT_CIPHERS reaches a client, concurrent Workers all see the same CA certificate lists, an ALPNCallback over a Duplex can drop its connection, the final handshake flight and the first write leave in one send()).
  • Not run: Windows and macOS. The change has no platform branch.

Measurements (two release builds with ThinLTO, linux x64: main 4354b80, and this change on it).

  • size: text 106747353 → 106747865 (+512), data and bss equal, file size equal.
  • Functions (linker map): us_internal_dispatch_ready_poll 4427 → 4540 bytes, us_internal_ssl_on_data 2143 → 2116, us_socket_sni_resolve 497 → 711, us_ssl_rare_free 151 → 165, ssl_hold_input 282 (new, .text.unlikely). 88 other sections of loop.c and openssl.c have the same size. ssl_update_handshake, BIO_s_custom_read, BIO_s_custom_write, us_internal_ssl_writev and us_select_cert_cb have 0 differing instructions.
  • Executed instructions in loop.c and openssl.c (qemu-x86_64 -one-insn-per-tb -d exec, per function, client and server in one process):
    • Plain TCP echo round trip (2 reads): us_internal_dispatch_ready_poll 298 → 296.
    • TLS echo round trip (2 reads, 2 writes): 1818.0 → 1803.5. The dispatch goes 294 → 298. The reader goes 386 → 368, because the compiler now inlines ssl_retry_parked_write.
    • TLS connection with no SNICallback or a synchronous one (both sides): -5. us_ssl_rare_free is +3 per SSL_free.
    • TLS connection with an asynchronous SNICallback and nothing kept: us_socket_sni_resolve 42 → 74 per answer, no allocation.
    • ssl_hold_input: 0 runs in these workloads. 1 run per connection that replays the recorded flight (1 realloc of 37 bytes, 1 free).
  • Syscalls over 300 TLS echo round trips (qemu -strace): recvfrom 601 → 603, sendto 602 → 602, epoll_ctl 11 → 11.
  • Memory: 1000 connections with 16390 bytes behind the ClientHello and no answer hold 20.7 to 20.8 KiB of RSS each, over three rounds on one server. The block of 16394 bytes is in the 20 KiB class of mimalloc. RSS after the close is at the level of a run with nothing kept, and does not grow over the rounds.
  • The replay (gdb on a debug build): us_dispatch_data and us_dispatch_ssl_raw_tap have 0 calls under us_socket_sni_resolve over the describe.
  • Wall clock, 12 interleaved runs of 40000 TLS echo round trips: main 836.5 ms, this branch 774.9 ms, a copy of main 788.2 ms. The difference (-7.4 %) is inside the noise (5.8 % between main and its copy).
  • strace, perf, valgrind and bloaty are not in the build container.

Self-review. 18 concerns, 16 addressed in the code, in the tests or in this text. Not done:

  • A tracking issue for this defect and for the skip budget of BoringSSL. Both are written down here.
  • Which clients connect again by themselves after the close. That was not verified here, so this text does not say it.

Still open, as on main.

  • handshakeTimeout on a tls.Server reports the connection and leaves it open, as in Node. So the kept bytes stay until the answer or the close of the socket.
  • BoringSSL counts the headers and tags of the records that it skips. A client that sends 16363 to 16384 bytes of early data is refused by every TLS server of Bun, with a synchronous SNICallback too (table above). Node serves it.

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/tls/node-tls-server.test.ts, test/js/bun/net/socket.test.ts

@robobun

robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:07 AM PT - Oct 21st, 2026

❌ @robobun, your commit f899e03 has 1 failures in Build #129828 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 46073

That installs a local version of the PR into your bun-46073 executable, so you can run:

bun-46073 --bun

@robobun

robobun commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator Author

Status: reproduced on main with openssl s_client -tls1_3 -early_data (OpenSSL 3.5.7, ticket of openssl s_server -early_data) against a tls.createServer whose SNICallback answers 50 ms later. The server reports tlsClientError ECONNRESET and sends no ServerHello. Node v26.3.0 serves the same client. The new tests replay that first flight without the CLI.

Pull request: #46073

@coderabbitai

coderabbitai Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 0b89994b-af42-4230-9155-98c2c46d5f89
📥 Commits

Reviewing files that changed from the base of the PR and between 91e7e3b and 66f2dc2.

📒 Files selected for processing: 5
  • packages/bun-usockets/src/crypto/openssl.c
  • packages/bun-usockets/src/internal/internal.h
  • packages/bun-usockets/src/loop.c
  • test/js/bun/net/socket.test.ts
  • test/js/node/tls/node-tls-server.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

The TLS input path now retains unread data while an SNI certificate callback suspends the handshake, then replays it after resolution. TLS raw-tap dispatch occurs in the loop before SSL processing. Renegotiation-window tracking now uses seconds.

Changes

Async SNI input buffering

Layer / File(s) Summary
TLS input dispatch and buffering
packages/bun-usockets/src/loop.c, packages/bun-usockets/src/crypto/openssl.c, packages/bun-usockets/src/internal/internal.h
The loop sends TLS data to the raw tap before SSL processing. SSL state can retain unread input during a pending certificate callback, subject to a cumulative size limit.
SNI resumption and queued-record validation
packages/bun-usockets/src/crypto/openssl.c, test/js/bun/net/socket.test.ts, test/js/node/tls/node-tls-server.test.ts
SNI resolution replays held input through SSL processing when the socket is alive and the handshake is pending. Tests cover queued and fragmented records, buffer limits, callback outcomes, and disconnects.

Renegotiation-window tracking

Layer / File(s) Summary
Renegotiation-window time units
packages/bun-usockets/src/crypto/openssl.c
Renegotiation-window tracking records timestamps and compares elapsed time in seconds.

Suggested reviewers: jarred-sumner, dylan-conway

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 66f2d

The identified test-style concern does not apply, and nested handshake replay preserves each connection’s read state. The change is mergeable after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the primary TLS fix: retaining client data while an asynchronous SNICallback is pending.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description clearly explains the problem, implementation, limitations, and verification results. It does not use the template headings exactly, but it includes the required information for what th…
Full details: Description check

Explanation

The description clearly explains the problem, implementation, limitations, and verification results. It does not use the template headings exactly, but it includes the required information for what the PR does and how it was verified.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

…back is pending

A server socket of the fd TLS engine whose SNICallback has not answered
closed the connection when any byte was still unread behind the
ClientHello. A TLS 1.3 client that sends early data puts a
ChangeCipherSpec and application_data records there, so it got no
ServerHello.

us_internal_ssl_on_data now moves those bytes to the connection. It
keeps at most 6 + 16384 bytes: the ChangeCipherSpec record and the early
data that BoringSSL skips behind it. More still drops the connection.

After the answer, us_socket_sni_resolve drives the handshake as before
and then reads the kept bytes through us_internal_ssl_on_data, the
reader that a recv() uses. A handshake that the answer ended does not
read them. us_socket_sni_resolve also closes a socket whose handshake
failed in the re-drive, as ssl_on_writable does: a Bun.listen socket
stayed open until its peer left.

The raw tap of an upgraded socket moves from us_internal_ssl_on_data to
the read dispatch in loop.c, so the raw half is not shown the kept bytes
a second time. us_ssl_rare_t keeps its 64 bytes: the renegotiation
window start is stored in seconds.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant