Skip to content

node:tls: call SNICallback with the connection's TLSSocket as this on a listening server - #46099

Draft
robobun wants to merge 1 commit into
mainfrom
robobun/f9a5454c/sni-callback-this-tlssocket
Draft

robobun wants to merge 1 commit into
mainfrom
robobun/f9a5454c/sni-callback-this-tlssocket

Conversation

@robobun

@robobun robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • On a listening tls.Server or http2.createSecureServer(), Bun runs the SNICallback with this set to the server. Node v26.3.0 passes the connection's TLSSocket.
  • A callback that uses its socket (this.destroy()) throws a TypeError: the client gets ECONNRESET, the server prints nothing. No affected user is known.
  • Cause: ServerHandlers.serverName (src/js/node/net.ts:1222) calls cb.$call(server, ...), and server is the tls.Server.

Fix

  • serverName takes the receiver from the connection's handle: socketHandle.data is the TLSSocket.
  • Correct because Node's loadSNI calls owner._SNICallback(...) on that socket, which 'secureConnection' passes later.
  • Verified: test/js/node/tls/node-tls-sni-callback-receiver.test.ts (node:test). 7 of 10 tests fail on main. All pass with the fix and on Node.
  • Self-reviewed: 22 concerns raised, 19 addressed. 3 are the setKeyCert() defects below.

Background

  • serverName is an internal Bun.listen socket handler, called when a ClientHello has a server name.
  • data is the JS value on a native socket. An accepted socket starts with its listener's data, then onconnection stores the TLSSocket before any read.
  • Considered passing the socket first natively. That changes a handler Bun.listen users can reach.

Downsides

  • A callback that relied on this === server now gets the TLSSocket. The server is this.server.
  • this.setKeyCert(ctx) in the callback now runs on a listening server, where it loses ALPN and ignores a later refusal. node:tls: setKeyCert() keeps the connection on its own SSL_CTX #46258 fixes that and must merge first.
  • serverName executes 38 bytecodes per ClientHello (36 before), with no new allocation. The binary grows 12 bytes.
Notes

Who is affected. No affected user is known. The difference was found by a read of the code. #44140 reported it, and a maintainer closed that issue with this comment: "Use handoff tool. GitHub issues are for user-facing issues and a user did not run into this issue". This change comes through that handoff. Related to #44140.

Two published packages have an SNICallback that needs its socket. Their callbacks were read in the npm tarballs. The packages were not run.

  • acme-bot@1.4.2, lib/acme_challenge_server.js:131: const ctx = this.server.ctx_map.get(servername). With this change this.server is the server, as on Node.
  • @vampaz/vite-plugin-local-tls@0.2.0, dist/index.js:2392: this.setKeyCert(selectContext(hostname)), and its 'secureConnection' listener reads socket.alpnProtocol === "h2". With this change the handshake completes with the selected certificate, but ALPN is not negotiated. See "Not changed here".

What an operator sees on main.

  • The callback uses the socket in the call: the TypeError drops the handshake. The client reports ECONNRESET ("Client network socket disconnected before secure TLS connection was established"). The server goes on and prints nothing.
  • An async callback uses the socket: TypeError: this.destroy is not a function. (In 'this.destroy()', 'this.destroy' is undefined) is an unhandled rejection, and the process exits.
  • The callback reads a property, for example this.remoteAddress: the value is undefined and nothing throws.

Repro. Run with node and with bun:

const tls = require("node:tls"); const fs = require("node:fs"); const { once } = require("node:events");
const fx = n => fs.readFileSync("test/js/node/tls/fixtures/" + n);
(async () => {
  let seen;
  const server = tls.createServer({ key: fx("agent2-key.pem"), cert: fx("agent2-cert.pem"),
    SNICallback(name, cb) { seen = { ctor: this.constructor.name, isTLSSocket: this instanceof tls.TLSSocket, isServer: this === server }; cb(null, undefined); } }, s => s.end());
  await new Promise(r => server.listen(0, "127.0.0.1", r));
  const c = tls.connect({ port: server.address().port, host: "127.0.0.1", servername: "a.test", rejectUnauthorized: false });
  c.on("error", () => {}); c.resume();
  await once(c, "close");
  server.close();
  console.log(JSON.stringify(seen));
})();
node v26.3.0:      {"ctor":"TLSSocket","isTLSSocket":true,"isServer":false}
bun main:          {"ctor":"Server","isTLSSocket":false,"isServer":true}
bun, this branch:  {"ctor":"TLSSocket","isTLSSocket":true,"isServer":false}

Where the callback runs, and what this is.

Node v26.3.0 main this branch
tls.createServer().listen(port) TLSSocket Server TLSSocket
tls.createServer().listen(path) TLSSocket Server TLSSocket
http2.createSecureServer().listen(port) TLSSocket Http2SecureServer TLSSocket
server.emit("connection", netSocket) TLSSocket TLSSocket TLSSocket
new TLSSocket(netSocket, { isServer: true, SNICallback }) TLSSocket TLSSocket TLSSocket

listen({ fd }) and a cluster worker use the same handler. They are not in the tests. ALPNCallback already gets the TLSSocket on main, through socket.data in ServerHandlers.alpnCallback.

Tests. The new file runs on node:test, so bun bd test and node --test run the same text. Its last test runs the file on Node when node is on the PATH.

  • One test for each row of the table. Two handshakes are in flight at once, one callback answers in the call and one later. Each this must be the socket that 'secureConnection' (or 'secure') passes for that server name, with this.server the server.
  • A connection accepted before server.close() still gets its socket.
  • this.destroy() drops the connection, and 'tlsClientError' passes the same socket.
  • An error answered in the call, and one answered on a later turn, reach 'tlsClientError' with the same socket.
  • this.servername is the requested name is a todo on Bun and a plain test on Node.
  • Debug ASAN build of this branch: 10 pass, 1 todo. The same build with the src/ of main 91e7e3b: 3 pass, 7 fail, 1 todo. The 3 are the two rows that main has right and the Node run.
  • node --test test/js/node/tls/node-tls-sni-callback-receiver.test.ts, Node v26.3.0: 10 pass.
  • test/js/node/tls/node-tls-context.test.ts: 83 pass. test/js/bun/net/socket.test.ts -t "selection callback": 2 pass (the raw serverName handler still gets the data of the listener).
  • Vendored, exit 0: test-tls-sni-option.js, test-tls-sni-servername.js, test-tls-snicallback-error.js, test-tls-starttls-server.js, test-tls-empty-sni-context.js, test-tls-psk-alpn-callback-exception-handling.js, test-tls-secure-context-usage-order.js, test-tls-sni-server-client.js, test-tls-alpn-server-client.js.
  • tsc for src/js and for the test file, prettier and oxlint are clean.
  • CI at 340efb0 (build 129047), all 11 test lanes: the two Windows lanes give 9 pass, 2 todo. The other nine (macOS x64 and aarch64, Debian 13 x64, aarch64 and x64 ASAN, Ubuntu 25.04 x64 and aarch64, Alpine 3.23 x64 and aarch64) give 10 pass, 1 todo. On Windows the listen(path) row dials a named pipe and is a todo: the TLS listener on a named pipe does not ask for the server name (src/runtime/socket/Listener.rs, the TODO in the named pipe arm). The Node run of the file exits 0 on Windows. Whether that row passes on Node there is not known, because a todo cannot fail the run.

Measurements (main 5900354 against this change, taken before the rebase to 91e7e3b, which does not touch net.ts).

  • serverName builtin: 57 -> 60 instructions (+11 bytes). onconnection, open, initAcceptedTLSSocket, handshake, serverHandlersFor, onSNIResolution and consumeSNIResult are identical (BUN_JSC_dumpGeneratedBytecodes, debug builds).
  • Executed bytecodes in serverName for each SNI ClientHello: 36 -> 38 (+2). For each accept, onconnection 85, open 23, initAcceptedTLSSocket 56 and handshake 87 on both (JSC profiler at the Baseline tier, difference of runs with 100 and 300 handshakes, clients in a Node process).
  • Allocations for each SNI handshake: +0. serverName has one new_object and one bind call on both builds, each executed once for each ClientHello. After 2,000 handshakes and a GC the heap holds 227 and 262 objects fewer than before on main, 206 and 234 fewer with this change (heapStats(), two runs each).
  • Native code: no native file changes. Builtin text: node/net.js 102,552 -> 102,564 bytes, and the embedded module blob InternalModuleRegistryConstants.bin 2,704,147 -> 2,704,159 bytes (release codegen).
  • Servers without an SNICallback: the handler table that net.Server, tls.Server, a tls.Server with an ALPNCallback and http2.createSecureServer() give to Bun.listen has no serverName. 0 dispatches in 20 accepts for each.
  • A script of 14 scenarios (the rows above, this.destroy(), this.setKeyCert(), cb(err) in the call and later, a ClientHello after close(), a connection dropped by maxConnections): 10 rows equal Node v26.3.0, 3 on main. The 4 listen() rows differ in typeof this._SNICallback and this.servername only.

Not changed here. These are about what the socket is and does inside the callback. The items that compare with Node were run on Node v26.3.0 and on this branch. They are the same on the rows that main has right.

  • this.setKeyCert(ctx) in the callback. The native set_key_cert (src/runtime/socket/tls_socket_functions.rs) switches the connection to the user's SSL_CTX, which has neither the ALPN callback nor the callback that waits for the SNICallback's answer. After it, ALPN is not negotiated (Node negotiates it). An answer that comes later is not awaited: with cb(new Error("denied")) 50 ms after setKeyCert() the client reads the server's data, where Node refuses. If the context also went through addContext(), the server has an ALPNCallback and the answer comes from a microtask, a debug build stops at the assertion vendor/boringssl/ssl/s3_both.cc:537 in tls_next_message. On main these need the socket of emit("connection") or of the 'connection' event. Setting SNICallback silently disables ALPN negotiation on a TLS server #41061 was the user report for ALPN after an SNI answer, and tls: consolidate the open TLS fixes (both engines, node:tls, node:https, WebSocket, SQL) #44618 fixed the three answer paths. Open node:tls: run ALPNCallback before BoringSSL picks the certificate and the session #43216 rewrites this function for another purpose. The fix is a pull request of its own, which must merge before this one.
  • this.servername is undefined until the handshake ends. Node assigns it for every server socket when the ClientHello arrives, also without an SNICallback, so this.servername in an ALPNCallback differs in the same way. A store in this handler would cover only servers with an SNICallback. It would also hold the Latin-1 form that the handler gets, where the handshake handler later assigns the UTF-8 form of getServername(). A check that a callback makes on this.servername does not see the name.
  • An accepted socket has no _SNICallback of its own. The callback stays on the server.
  • this.end() in the callback lets the handshake complete. Node resets the connection.
  • this.getProtocol() in the callback is TLSv1.2 for a TLS 1.3 handshake.
  • On TLS 1.3 Node runs the ALPNCallback before the SNICallback. Bun runs the SNICallback first on every version. So what the SNICallback stores on this is there in the ALPNCallback on Bun, and not on Node with TLS 1.3.
  • A refusal answered in the call takes effect when the callback returns. Node destroys the socket inside the answer. So answer(err); this.destroy() reports ECONNRESET where Node reports err.
  • A second answer() is ignored. Node destroys the socket with ERR_MULTIPLE_CALLBACK.
  • From the code, not run: if a replaced server._handle.onconnection has attached no TLSSocket when the ClientHello arrives, the data of the handle is still the server, and the callback gets the server.
  • The callback does not run at all over a Duplex (node:tls: ask for the server name on the stream TLS engine #45471), for https.createServer() (Node's https server doesn't have addContext function #12157, and open node:https: thread SNICallback through to the TLS listener #36707 adds it with the https server as this), and for http2SecureServer.emit("connection", netSocket). From the code, it does not run on a Windows named pipe.

Other callbacks whose this differs from Node. They have other causes and are not changed here: onread.callback (#42420), and the callback of res.end(callback), which Node runs with the response as this.

Other designs.

  • Give each accepted socket the callback as its own _SNICallback, as Node does. Every accepted TLS connection then pays a store that only servers with an SNICallback need.
  • Store servername on the socket before the call. Left out for the reasons in the this.servername item above.
  • node:tls: ask for the server name on the stream TLS engine #45471 splits the native dispatch for the stream TLS engine and keeps what it passes. A merge of the two branches has no conflict.

On a tls.Server that listens, the user's SNICallback ran with the
tls.Server as `this`. Node calls it as a method of the server-side
TLSSocket of the connection (loadSNI, lib/internal/tls/wrap.js).

ServerHandlers.serverName still reads the callback from the `data` that
the native dispatch passes. It now takes the receiver from the
connection's handle, whose `data` is the TLSSocket. The same handler
serves http2.createSecureServer and every form of listen().

The test file runs on node:test, so the same file passes on Node.js.
@robobun

robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 6:45 PM PT - Oct 19th, 2026

❌ @robobun, your commit 340efb0 has 1 failures in Build #129047 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 46099

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

bun-46099 --bun

@robobun

robobun commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: draft until #46258 merges.

Reproduced with the script in the description. Node v26.3.0 prints TLSSocket, Bun main prints Server, this branch prints TLSSocket. test/js/node/tls/node-tls-sni-callback-receiver.test.ts fails 7 of 10 tests on main. It passes on this branch and on Node.

#46258 is the fix for setKeyCert() inside an SNICallback. It must merge first (see Downsides).

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