From 19fdc4f8105b59c6044dc61097cbe9ddb498c547 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 25 Sep 2026 02:46:41 +0000 Subject: [PATCH 1/6] tls: a forceful close releases the final handshake flight its socket still holds A TLS 1.3 client finishes its handshake before it sends its final flight (Certificate, CertificateVerify, Finished). usockets holds that flight across the handshake callback. us_internal_ssl_close flushed it for every close code, so a client that destroyed its socket in the callback (a checkServerIdentity verdict, a destroy() in 'secureConnect') still sent its certificate to the server it refused, and the server reported 'secureConnection'. us_internal_ssl_close now flushes the batch its socket owns for a graceful close only. A forceful close releases it and marks the socket fatal. The rule has no role test, so a TLS 1.2 server that destroys in its handshake callback keeps its Finished. Three routes sent the flight before the callback could refuse: - A read that completes the handshake and then fails now reports the handshake before it closes. - The flight is held also while another socket's spill is pending. - A write to another TLS socket from the callback no longer flushes it. --- packages/bun-types/bun.d.ts | 16 +- packages/bun-usockets/src/crypto/openssl.c | 82 ++++-- .../net/tls-reject-before-client-cert.test.ts | 49 +++- test/js/node/tls/node-tls-connect.test.ts | 234 ++++++++++++++- test/js/node/tls/tls-client-close-fixture.mjs | 271 ++++++++++++++++++ .../js/node/tls/tls-server-refuse-fixture.mjs | 58 ++++ 6 files changed, 681 insertions(+), 29 deletions(-) create mode 100644 test/js/node/tls/tls-client-close-fixture.mjs create mode 100644 test/js/node/tls/tls-server-refuse-fixture.mjs diff --git a/packages/bun-types/bun.d.ts b/packages/bun-types/bun.d.ts index 221aa2fea1f6..9ada427e6701 100644 --- a/packages/bun-types/bun.d.ts +++ b/packages/bun-types/bun.d.ts @@ -6765,7 +6765,7 @@ declare module "bun" { /** * Forcefully closes the socket connection immediately. This is an abrupt termination, unlike the graceful shutdown initiated by `end()`. * It uses `SO_LINGER` with `l_onoff=1` and `l_linger=0` before calling `close(2)`. - * Consider using {@link close close()} or {@link end end()} for graceful shutdowns. + * Consider using {@link end end()} for a graceful shutdown. * * @example * ```ts @@ -7140,12 +7140,20 @@ declare module "bun" { upgradeTLS(options: TLSUpgradeOptions): [raw: Socket, tls: Socket]; /** - * Closes the socket. + * Closes the socket at once. Data that `write()` already accepted is still + * delivered, then the connection ends with a TCP FIN. A TLS socket sends no + * `close_notify` alert. * - * This is a wrapper around `end()` and `shutdown()`. + * Called from the `handshake` callback of a TLS socket, `close()` turns the + * peer down: the rest of the handshake is not sent. For a TLS 1.3 client + * that includes its certificate. {@link terminate terminate()} does the + * same. A socket with no `handshake` callback gets its `open` callback at + * that point, so the same holds there. + * + * Use {@link end end()} for a graceful close. * * @see {@link end} - * @see {@link shutdown} + * @see {@link terminate} */ close(): void; diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index ee944b75caff..ba016f38891e 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -542,22 +542,21 @@ static int BIO_s_custom_write(BIO *bio, const char *data, int length) { return length; } - if (loop_ssl_data->ssl_write_batching && - loop_ssl_data->ssl_write_batch_len && - loop_ssl_data->ssl_write_batch_owner != loop_ssl_data->ssl_socket) { - /* The batch holds another socket's records (a JS callback in this - * dispatch wrote to a second TLS socket on the same loop while the - * first socket's flight was held). Deliver them to their owner first so - * each socket's records stay in order. */ - ssl_flush_write_batch(loop_ssl_data, loop_ssl_data->ssl_write_batch_owner); - } - - if (loop_ssl_data->ssl_write_batching && loop_ssl_data->ssl_spill_owner == NULL) { + /* The batch can hold another socket's records: a JS callback in this + * dispatch wrote to a second TLS socket on the same loop while the first + * socket's flight was held. That flight stays held, because its owner can + * still turn the peer down, and this socket's records are written through. */ + int batch_is_foreign = loop_ssl_data->ssl_write_batch_len && + loop_ssl_data->ssl_write_batch_owner != loop_ssl_data->ssl_socket; + + if (loop_ssl_data->ssl_write_batching && !batch_is_foreign && + loop_ssl_data->ssl_spill_owner != loop_ssl_data->ssl_socket) { /* Append the sealed record; the batch hits the kernel once, after * SSL_write returns. Reporting the full length keeps BoringSSL sealing - * the next record instead of parking a partial one. Skipped while a - * spill occupies the slot: a later short flush could not park its - * remainder without clobbering that socket's pending ciphertext. */ + * the next record instead of parking a partial one. Skipped while this + * socket's own spill is pending: those bytes go first. us_internal_ssl_write + * does not batch while any spill is pending, so with another socket's + * spill this is the handshake hold of us_internal_ssl_on_data. */ unsigned int needed = loop_ssl_data->ssl_write_batch_len + (unsigned int)length; if (needed > loop_ssl_data->ssl_write_batch_cap) { unsigned int new_cap = loop_ssl_data->ssl_write_batch_cap ? loop_ssl_data->ssl_write_batch_cap : 65536; @@ -2020,14 +2019,26 @@ struct us_socket_t *us_internal_ssl_close(struct us_socket_t *s, int code, void return s; } { - /* Ciphertext batched in this dispatch and still held (the handshake's - * final flight, a fatal alert sealed by a failing SSL_read) goes to the - * wire before the close_notify/FIN this teardown sends. A partial write - * spills; the graceful-close deferral below then waits for the drain. */ + /* Ciphertext batched in this dispatch and still held: the handshake's + * final flight, or a fatal alert sealed by a failing SSL_read (the driver + * closes with code 0 for that). A graceful close sends it before its + * close_notify/FIN. A partial write spills; the graceful-close deferral + * below then waits for the drain. + * A forceful close from inside the dispatch is the owner turning the peer + * down, so the flight is released: under TLS 1.3 it carries the client's + * Certificate. node drops its pending output when destroy() runs in the + * handshake callback: + * https://github.com/nodejs/node/blob/v26.10.0/src/crypto/crypto_tls.cc#L1409-L1433 + * The peer never gets our Finished, so it could not read a close_notify. */ struct loop_ssl_data *loop_ssl_data = (struct loop_ssl_data *)s->group->loop->data.ssl_data; if (loop_ssl_data && loop_ssl_data->ssl_write_batch_len && loop_ssl_data->ssl_write_batch_owner == s && !us_socket_is_closed(s)) { - ssl_flush_write_batch(loop_ssl_data, s); + if (code == LIBUS_SOCKET_CLOSE_CODE_CLEAN_SHUTDOWN) { + ssl_flush_write_batch(loop_ssl_data, s); + } else { + ssl_release_batch(s->group->loop, s); + s->ssl_fatal_error = 1; + } } } /* Neither node's `_handle.close()` (FAST_SHUTDOWN, no reason) nor a graceful @@ -2353,6 +2364,23 @@ struct us_socket_t *us_internal_ssl_on_writable(struct us_socket_t *s) { return s; } +/* A read that finished the handshake and then failed (a bad record, broken + * framing) reports the handshake before it closes, as the no-data completion + * in us_internal_ssl_on_data does. The owner may turn the peer down there, and + * that close releases the held flight: a peer must not collect it by sending + * junk behind its Finished. Returns 0 when the socket is gone. */ +static int ssl_report_handshake_before_failed_read(struct us_socket_t *s, struct loop_ssl_data *loop_ssl_data) { + if (s->ssl_handshake_state != HANDSHAKE_PENDING || !SSL_is_init_finished(s_ssl(s))) return 1; + ERR_clear_error(); + ssl_trigger_handshake(s, 1); + if (ssl_gone(s)) return 0; + loop_ssl_data->ssl_socket = s; + if (loop_ssl_data->ssl_write_batch_len && loop_ssl_data->ssl_write_batch_owner == s) { + ssl_flush_write_batch(loop_ssl_data, s); + } + return 1; +} + struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, int length) { /* See ssl_update_handshake: start this socket's SSL processing with a clean * per-thread error queue so a captured reason cannot belong to another @@ -2404,12 +2432,15 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i * down right after the handshake (node's post-verify destroy, #40653) * close with the second segment unread, which turns its FIN teardown * into an RST and a bogus ECONNRESET at this side. Node's memory BIO - * drained once per cycle has the same single-segment shape. Gated on a - * free spill slot like us_internal_ssl_write, so a short flush cannot - * clobber another socket's pending ciphertext. */ + * drained once per cycle has the same single-segment shape. The hold + * also decides whether the owner's refusal in on_handshake can still + * withhold the flight, so another socket's pending spill does not switch + * it off: a short flush then fails this socket (ssl_flush_write_batch) + * and leaves that spill alone. A spill of this socket's own goes first, + * so its records are written through as before. */ int hs_batching = s->ssl_handshake_state == HANDSHAKE_PENDING && !loop_ssl_data->ssl_write_batching && - !loop_ssl_data->ssl_spill_owner; + loop_ssl_data->ssl_spill_owner != s; if (hs_batching) loop_ssl_data->ssl_write_batching = 1; unsigned char ssl_was_in_use = s->ssl_in_use; s->ssl_in_use = 1; @@ -2502,6 +2533,7 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i return s; } + if (!ssl_report_handshake_before_failed_read(s, loop_ssl_data)) return NULL; if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { ssl_park_fatal_reason(s); } @@ -2514,6 +2546,7 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i /* If the BIO still has unread ciphertext at this point, the TLS * framing is broken — close. */ if (loop_ssl_data->ssl_read_input_length) { + if (!ssl_report_handshake_before_failed_read(s, loop_ssl_data)) return NULL; return ssl_close(s, 0, NULL); } /* SSL_read drove the handshake to completion but returned no app @@ -2709,6 +2742,9 @@ int us_internal_ssl_write(struct us_socket_t *s, const char *data, int length) { * layers above fire 'finish' and close before the data reached the wire. */ int outer_batching = loop_ssl_data->ssl_write_batching; int batching = (loop_ssl_data->ssl_spill_owner == NULL); + /* A flight held across on_handshake while another socket's spill is pending + * goes before the records this write sends through. */ + if (!batching && !ssl_flush_write_batch(loop_ssl_data, s)) return 0; loop_ssl_data->ssl_write_batching = batching; int total = 0; diff --git a/test/js/bun/net/tls-reject-before-client-cert.test.ts b/test/js/bun/net/tls-reject-before-client-cert.test.ts index ba60ebd2c9c3..f2bcd6c87d4d 100644 --- a/test/js/bun/net/tls-reject-before-client-cert.test.ts +++ b/test/js/bun/net/tls-reject-before-client-cert.test.ts @@ -20,6 +20,7 @@ import { afterAll, describe, expect, test } from "bun:test"; import { tls as harnessTls, isWindows, tempDir } from "harness"; import { randomUUID } from "node:crypto"; import { readFileSync } from "node:fs"; +import https from "node:https"; import net from "node:net"; import { join } from "node:path"; import { Duplex } from "node:stream"; @@ -159,7 +160,9 @@ const httpOk = (socket: tls.TLSSocket) => socket.on("data", () => socket.write("HTTP/1.1 200 OK\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok")); // The handshake is all this test needs: drop the connection once it is done. -const dropAfterHandshake = (socket: tls.TLSSocket) => socket.destroy(); +// Not inside 'secure': a TLS 1.2 server has not sent its Finished there yet, +// and a destroy() in that callback turns the client down. +const dropAfterHandshake = (socket: tls.TLSSocket) => setImmediate(() => socket.destroy()); // Postgres: answer the 8-byte SSLRequest with "S", then TLS starts. const postgresPrelude = (socket: net.Socket) => @@ -906,6 +909,50 @@ describe.each(["TLSv1.3", "TLSv1.2"] as const)( }, ); +// node:tls decides the name in JS, in the handshake callback. Under TLS 1.3 +// the client's final flight, with its certificate, is still held at that +// point, and the destroy() of the refusal drops it. Under TLS 1.2 the +// certificate leaves before the server's Finished, in node too. +// node-tls-connect.test.ts runs every route in node as well. +describe("TLSv1.3: a node:tls client sends no client certificate to a server whose certificate names another host", () => { + const maxVersion = "TLSv1.3"; + + test("tls.connect", async () => { + await using srv = await mtlsServer({ identity: otherHost, maxVersion }); + const err = await tlsOutcome(tls.connect({ host: "localhost", port: srv.port, ...otherHostMtls })); + expect(err?.code).toBe("ERR_TLS_CERT_ALTNAME_INVALID"); + await srv.seen.closed; + expect(srv.seen.peerCN).toBeNull(); + expect(srv.seen.clientTlsBytes).toBe(srv.seen.clientHelloBytes); + }); + + test("tls.connect with a checkServerIdentity function that returns an Error", async () => { + await using srv = await mtlsServer({ identity: otherHost, maxVersion }); + const checkServerIdentity = () => Object.assign(new Error("not the pinned key"), { code: "ERR_PINNED_KEY" }); + const err = await tlsOutcome( + tls.connect({ host: "localhost", port: srv.port, servername: "agent1", ...otherHostMtls, checkServerIdentity }), + ); + expect(err?.code).toBe("ERR_PINNED_KEY"); + await srv.seen.closed; + expect(srv.seen.peerCN).toBeNull(); + expect(srv.seen.clientTlsBytes).toBe(srv.seen.clientHelloBytes); + }); + + test("https.request", async () => { + await using srv = await mtlsServer({ identity: otherHost, onSecure: httpOk, maxVersion }); + const err = await new Promise(resolve => + https + .request({ host: "localhost", port: srv.port, agent: false, ...otherHostMtls }, () => resolve(null)) + .on("error", resolve) + .end(), + ); + expect(err?.code).toBe("ERR_TLS_CERT_ALTNAME_INVALID"); + await srv.seen.closed; + expect(srv.seen.peerCN).toBeNull(); + expect(srv.seen.clientTlsBytes).toBe(srv.seen.clientHelloBytes); + }); +}); + // The inline reject is installed only when the client's policy rejects a bad // chain. A client that accepts one must still complete the handshake, and // the server then sees the client certificate as before. diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index ceebd3025092..1a2d3656427b 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -1,7 +1,7 @@ import { heapStats } from "bun:jsc"; import { describe, expect, it } from "bun:test"; import { once } from "events"; -import { writeFileSync } from "fs"; +import { readFileSync, writeFileSync } from "fs"; import { bunEnv, bunExe, tls as COMMON_CERT_, isASAN, nodeExe, tempDir } from "harness"; import https from "https"; import net from "net"; @@ -11,6 +11,9 @@ import tls, { checkServerIdentity, connect as tlsConnect, TLSSocket } from "tls" import type { AddressInfo } from "net"; import { Duplex } from "node:stream"; +import { pathToFileURL } from "node:url"; +import { report as closeReport } from "./tls-client-close-fixture.mjs"; +import { report as refuseReport } from "./tls-server-refuse-fixture.mjs"; const symbolConnectOptions = Symbol.for("::buntlsconnectoptions::"); @@ -2135,6 +2138,91 @@ it("ending a TLS 1.3 socket from its handshake callback still completes the serv await clientClosed.promise; }); +// terminate() and close() are the forceful closes of the Bun socket API. From +// the handshake callback they turn the server down, like the native reject of +// an unauthorized server does: the held final flight, which carries the client +// certificate, must not go out. end() and shutdown() are graceful and still +// send it. A socket with no handshake callback gets its open callback at that +// point. A plain TCP relay counts what the client sent. +describe("a TLS 1.3 Bun.connect client that closes once its handshake is done", () => { + const keys = join(import.meta.dir, "..", "test", "fixtures", "keys"); + const pem = (name: string) => readFileSync(join(keys, name), "utf8"); + type Close = "end" | "shutdown" | "terminate" | "close"; + + async function run(callback: "handshake" | "open", method: Close) { + const serverSaw = Promise.withResolvers(); + await using listener = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + tls: { key: pem("agent1-key.pem"), cert: pem("agent1-cert.pem"), requestCert: true, rejectUnauthorized: false }, + socket: { + handshake(socket, success, error) { + const peerCN = (socket.getPeerCertificate() as tls.PeerCertificate)?.subject?.CN; + serverSaw.resolve(success ? `handshake:${peerCN}` : `fail:${(error as NodeJS.ErrnoException)?.code}`); + }, + data() {}, + error() {}, + close() {}, + }, + }); + const fromClient: Buffer[] = []; + const relay = net.createServer(downstream => { + const upstream = net.connect(listener.port, "127.0.0.1"); + downstream.on("data", chunk => { + fromClient.push(chunk); + upstream.write(chunk); + }); + upstream.on("data", chunk => downstream.write(chunk)); + downstream.on("end", () => upstream.end()); + upstream.on("end", () => downstream.end()); + downstream.on("error", () => upstream.destroy()); + upstream.on("error", () => downstream.destroy()); + }); + await once(relay.listen(0, "127.0.0.1"), "listening"); + try { + const clientClosed = Promise.withResolvers(); + await Bun.connect({ + hostname: "127.0.0.1", + port: (relay.address() as AddressInfo).port, + tls: { + ca: pem("ca1-cert.pem"), + serverName: "agent1", + key: pem("agent3-key.pem"), + cert: pem("agent3-cert.pem"), + }, + socket: { + [callback]: (socket: { [method in Close]: () => void }) => void socket[method](), + data() {}, + error() {}, + close: () => clientClosed.resolve(), + }, + }); + const [server] = await Promise.all([serverSaw.promise, clientClosed.promise]); + const bytes = Buffer.concat(fromClient); + return { server, sentAfterClientHello: bytes.length > 5 + bytes.readUInt16BE(3) }; + } finally { + relay.close(); + } + } + + it.each([ + ["handshake", "terminate"], + ["handshake", "close"], + ["open", "terminate"], + ["open", "close"], + ] as const)("%s: %s() sends nothing after the ClientHello", async (callback, method) => { + expect(await run(callback, method)).toEqual({ server: "fail:ECONNRESET", sentAfterClientHello: false }); + }); + + it.each([ + ["handshake", "end"], + ["handshake", "shutdown"], + ["open", "end"], + ] as const)("%s: %s() still completes the server's handshake", async (callback, method) => { + expect(await run(callback, method)).toEqual({ server: "handshake:agent3", sentAfterClientHello: true }); + }); +}); + // End-to-end shape of the issue: a Node TLS 1.3 server that rejects the // client's certificate does so AFTER the client saw 'secureConnect' (TLS 1.3 // clients finish first), and destroys the raw socket with no close_notify. @@ -2490,6 +2578,150 @@ describe.each([ }); }); +// Runs report(mode, version) of a fixture module in node, for every row at +// once. Resolves with node's version and the reports in the order of the rows. +async function reportsFromNode(fixture: string, rows: (readonly [version: string, mode: string])[]) { + const script = ` + import { report } from ${JSON.stringify(pathToFileURL(join(import.meta.dir, fixture)).href)}; + const rows = ${JSON.stringify(rows)}; + const reports = await Promise.all(rows.map(([version, mode]) => report(mode, version))); + console.log(JSON.stringify({ node: process.versions.node, reports })); + process.exit(0); + `; + await using proc = Bun.spawn({ + cmd: [nodeExe()!, "--input-type=module", "-e", script], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + const { node, reports } = JSON.parse(stdout) as { node: string; reports: unknown[] }; + expect(exitCode).toBe(0); + return { node: node.split(".").map(Number), reports }; +} + +// A TLS 1.3 client, and a client that resumes a TLS 1.2 session, finishes its +// handshake before it sends its final flight. The handshake callback runs in +// between. A client that destroys the socket there turns the server down, so +// that flight must not go out: under TLS 1.3 it carries the client +// certificate, and it lets the server report a connection that the client +// never used. node drops its pending output on destroy(): +// https://github.com/nodejs/node/blob/v26.10.0/src/crypto/crypto_tls.cc#L1409-L1433 +// The last test runs the same rows in node, so the reports are pinned to it. +describe("how a TLS client's way of closing reaches the server", () => { + const delivered = (data: string, alerts: number, client = ["close:false"]) => ({ + client, + server: { event: "secureConnection", peerCN: "agent3", data, error: null }, + sentAfterClientHello: true, + alerts, + }); + const turnedDown = (...client: string[]) => ({ + client, + server: { event: "tlsClientError", code: "ECONNRESET" }, + sentAfterClientHello: false, + alerts: 0, + }); + const altnameInvalid = "error:ERR_TLS_CERT_ALTNAME_INVALID"; + const backpressured = " while another TLS socket is backpressured"; + + const rows = [ + ["TLSv1.3", "checkServerIdentity", turnedDown(altnameInvalid, "close:true")], + ["TLSv1.3", "checkServerIdentity function", turnedDown("error:ERR_PINNED_KEY", "close:true")], + ["TLSv1.3", "a junk record behind the server's Finished", turnedDown(altnameInvalid, "close:true")], + ["TLSv1.3", "tls.connect({ socket })", turnedDown(altnameInvalid, "close:true")], + ["TLSv1.3", "https.request", turnedDown(altnameInvalid)], + ["TLSv1.3", "http2.connect", turnedDown(altnameInvalid)], + ["TLSv1.3", "destroy()", turnedDown("close:false")], + ["TLSv1.3", "destroy(error)", turnedDown("error:ERR_REFUSED", "close:true")], + ["TLSv1.3", "destroy() from process.nextTick", turnedDown("close:false")], + ["TLSv1.3", "destroy() from queueMicrotask", turnedDown("close:false")], + ["TLSv1.3", "destroy() on a resumed session", turnedDown("reused:true", "close:false")], + ["TLSv1.2", "destroy() on a resumed session", turnedDown("reused:true", "close:false")], + + // The flight is held with or without other traffic on the event loop. + ["TLSv1.3", "checkServerIdentity" + backpressured, turnedDown(altnameInvalid, "close:true")], + ["TLSv1.3", "destroy()" + backpressured, turnedDown("close:false")], + [ + "TLSv1.3", + "checkServerIdentity function that writes to another TLS socket", + turnedDown("error:ERR_PINNED_KEY", "close:true"), + ], + + // A graceful close still sends the flight. + ["TLSv1.3", "end()", delivered("", 1)], + ["TLSv1.3", "end(data)", delivered("hello", 1)], + ["TLSv1.3", "destroySoon()", delivered("", 1)], + ["TLSv1.3", "end()" + backpressured, delivered("", 1)], + + // In a full TLS 1.2 handshake the client's flight leaves before the server's Finished. + ["TLSv1.2", "checkServerIdentity", delivered("", 0, [altnameInvalid, "close:true"])], + ["TLSv1.2", "destroy()", delivered("", 0)], + ] as const; + + // Since node 26.10.0 an end() or a write() made in 'secureConnect' waits for + // the callback to return, and a destroy() in the same callback drops it: + // https://github.com/nodejs/node/pull/65105 + // Up to node 26.9 these four delivered the flight. + const sinceNode2610 = [ + ["TLSv1.3", "end() then destroy()", turnedDown("close:false")], + ["TLSv1.3", "write('') then destroy()", turnedDown("close:false")], + ["TLSv1.3", "end('') then destroy()", turnedDown("close:false")], + ["TLSv1.2", "end() then destroy()", delivered("", 0)], + ] as const; + + it.each([...rows, ...sinceNode2610])("%s %s", async (version, mode, expected) => { + expect(await closeReport(mode, version)).toEqual(expected); + }); + + // node 26.10 drops this write too. Bun sends it with the flight, as node did up to 26.9. + it("TLSv1.3 write() then destroy() sends the data with the flight", async () => { + expect(await closeReport("write() then destroy()", "TLSv1.3")).toEqual(delivered("hello", 0)); + }); + + it.skipIf(!nodeExe())("node gives the same reports", async () => { + const { node, reports } = await reportsFromNode( + "tls-client-close-fixture.mjs", + [...rows, ...sinceNode2610].map(([version, mode]) => [version, mode]), + ); + expect(reports.slice(0, rows.length)).toEqual(rows.map(([, , expected]) => expected)); + if (node[0] > 26 || (node[0] === 26 && node[1] >= 10)) { + expect(reports.slice(rows.length)).toEqual(sinceNode2610.map(([, , expected]) => expected)); + } + }); +}); + +// The same rule from the server's side. In a full TLS 1.2 handshake the +// server's Finished is the last message, and the server's handshake callback +// runs before it is sent. A server that destroys the socket there turns the +// client down, so the Finished must not go out: the client then reports a +// connection that the server never had. In TLS 1.3 the server has nothing left +// to send at that point. +describe("a server that turns the client down once its handshake is done", () => { + const turnedDown = { secureConnect: false, error: "ECONNRESET" }; + const connected = { secureConnect: true, error: null }; + + const rows = [ + ["TLSv1.2", "requestCert and rejectUnauthorized", { client: turnedDown, server: "tlsClientError" }], + ["TLSv1.2", "destroy() in 'secureConnection'", { client: turnedDown, server: "secureConnection" }], + ["TLSv1.2", "end() in 'secureConnection'", { client: connected, server: "secureConnection" }], + ["TLSv1.3", "requestCert and rejectUnauthorized", { client: connected, server: "tlsClientError" }], + ["TLSv1.3", "destroy() in 'secureConnection'", { client: connected, server: "secureConnection" }], + ] as const; + + it.each(rows)("%s %s", async (version, mode, expected) => { + expect(await refuseReport(mode, version)).toEqual(expected); + }); + + it.skipIf(!nodeExe())("node gives the same reports", async () => { + const { reports } = await reportsFromNode( + "tls-server-refuse-fixture.mjs", + rows.map(([version, mode]) => [version, mode]), + ); + expect(reports).toEqual(rows.map(([, , expected]) => expected)); + }); +}); + it.each(["TLSv1.3", "TLSv1.2"] as const)( "%s: re-checks server identity on a resumed session (cross-servername resume must not authorize)", async version => { diff --git a/test/js/node/tls/tls-client-close-fixture.mjs b/test/js/node/tls/tls-client-close-fixture.mjs new file mode 100644 index 000000000000..afa897b68ac7 --- /dev/null +++ b/test/js/node/tls/tls-client-close-fixture.mjs @@ -0,0 +1,271 @@ +// How a TLS client's way of closing reaches the server. report(mode, version) +// makes one connection and resolves with what the server and the wire saw. +// node-tls-connect.test.ts runs it in bun and in node, and expects the same +// reports from both. +// +// The server asks for a client certificate and accepts any. A plain TCP relay +// in front of it records what the client sent. The report is about the last +// connection of the mode: +// - `client` lists the first error and the 'close' event of the client. +// - `server` is what the server saw: "secureConnection" with the CN of the +// client certificate, the data it read and the error of its socket, or +// "tlsClientError" with the error code. +// - `sentAfterClientHello` says whether the client sent anything after its +// first record. A client that turns the server down does not. +// - `alerts` counts the client's alert records. A TLS 1.3 alert travels as an +// application data record of 19 bytes: the alert, the inner content type and +// the AEAD tag. Nothing else a mode sends has that size. +// +// A mode that ends with "while another TLS socket is backpressured" first +// opens a second TLS connection whose peer stops reading, and writes to it +// until the kernel takes no more. +import { once } from "node:events"; +import { readFileSync } from "node:fs"; +import http2 from "node:http2"; +import https from "node:https"; +import net from "node:net"; +import { join } from "node:path"; +import tls from "node:tls"; + +const keys = join(import.meta.dirname, "..", "test", "fixtures", "keys"); +const pem = name => readFileSync(join(keys, name)); + +// One application data record that does not decrypt. +const junkRecord = Buffer.concat([Buffer.from([23, 3, 3, 0, 16]), Buffer.alloc(16, 0xa5)]); + +function wire(bytes) { + let alerts = 0; + for (let at = 0; at + 5 <= bytes.length; at += 5 + bytes.readUInt16BE(at + 3)) { + const type = bytes[at]; + if (type === 21 || (type === 23 && bytes.readUInt16BE(at + 3) === 19)) alerts++; + } + const clientHello = bytes.length >= 5 ? 5 + bytes.readUInt16BE(3) : 0; + return { sentAfterClientHello: bytes.length > clientHello, alerts }; +} + +// What a 'secureConnect' listener does with the socket. +const inSecureConnect = { + "destroy()": socket => socket.destroy(), + "destroy(error)": socket => socket.destroy(Object.assign(new Error("refused"), { code: "ERR_REFUSED" })), + "destroy() from process.nextTick": socket => process.nextTick(() => socket.destroy()), + "destroy() from queueMicrotask": socket => queueMicrotask(() => socket.destroy()), + "end()": socket => socket.end(), + "end(data)": socket => socket.end("hello"), + "write() then destroy()": socket => { + socket.write("hello"); + socket.destroy(); + }, + "write('') then destroy()": socket => { + socket.write(""); + socket.destroy(); + }, + "end('') then destroy()": socket => { + socket.end(""); + socket.destroy(); + }, + "end() then destroy()": socket => { + socket.end(); + socket.destroy(); + }, + "destroySoon()": socket => socket.destroySoon(), +}; + +const backpressured = " while another TLS socket is backpressured"; + +// A TLS connection of this process whose peer is a TCP relay. `stall()` makes +// the relay stop reading and writes 32 MB: the kernel takes a part, and the +// rest of the sealed records waits in the TLS layer. +async function secondConnection() { + const server = tls.createServer({ key: pem("agent1-key.pem"), cert: pem("agent1-cert.pem") }, socket => { + socket.on("error", () => {}); + socket.resume(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + let downstream; + const relay = net.createServer(socket => { + downstream = socket; + const upstream = net.connect(server.address().port, "127.0.0.1"); + downstream.on("data", chunk => upstream.write(chunk)); + upstream.on("data", chunk => downstream.write(chunk)); + downstream.on("error", () => upstream.destroy()); + upstream.on("error", () => downstream.destroy()); + downstream.on("close", () => upstream.destroy()); + }); + await once(relay.listen(0, "127.0.0.1"), "listening"); + const socket = tls.connect({ + host: "127.0.0.1", + port: relay.address().port, + ca: pem("ca1-cert.pem"), + servername: "agent1", + }); + socket.on("error", () => {}); + await once(socket, "secureConnect"); + return { + socket, + stall() { + downstream.pause(); + socket.write(Buffer.alloc(32 * 1024 * 1024)); + }, + close() { + socket.destroy(); + downstream.destroy(); + relay.close(); + server.close(); + }, + }; +} + +export async function report(fullMode, version) { + const mode = fullMode.endsWith(backpressured) ? fullMode.slice(0, -backpressured.length) : fullMode; + const second = + mode !== fullMode || mode === "checkServerIdentity function that writes to another TLS socket" + ? await secondConnection() + : null; + if (mode !== fullMode) second.stall(); + // agent1 is signed by ca1 and names only "agent1". + let serverSaw = Promise.withResolvers(); + const server = tls.createServer({ + key: pem("agent1-key.pem"), + cert: pem("agent1-cert.pem"), + requestCert: true, + rejectUnauthorized: false, + minVersion: version, + maxVersion: version, + ALPNProtocols: mode === "http2.connect" ? ["h2"] : undefined, + }); + server.on("secureConnection", socket => { + // BoringSSL sends its TLS 1.3 tickets with the first write of the server. + if (mode === "destroy() on a resumed session") socket.write("x"); + const peerCN = socket.getPeerCertificate()?.subject?.CN ?? null; + let data = ""; + let error = null; + socket.on("data", chunk => (data += chunk)); + socket.on("error", err => (error = err.code)); + socket.on("close", () => serverSaw.resolve({ event: "secureConnection", peerCN, data, error })); + }); + server.on("tlsClientError", error => serverSaw.resolve({ event: "tlsClientError", code: error.code })); + await once(server.listen(0, "127.0.0.1"), "listening"); + + let fromClient = []; + const relay = net.createServer(downstream => { + fromClient = []; + let serverChunks = 0; + const upstream = net.connect(server.address().port, "127.0.0.1"); + downstream.on("data", chunk => { + fromClient.push(chunk); + upstream.write(chunk); + }); + upstream.on("data", chunk => { + // The server's first chunk is its whole flight, up to its Finished. + const junk = mode === "a junk record behind the server's Finished" && serverChunks++ === 0; + downstream.write(junk ? Buffer.concat([chunk, junkRecord]) : chunk); + }); + downstream.on("end", () => upstream.end()); + upstream.on("end", () => downstream.end()); + downstream.on("error", () => upstream.destroy()); + upstream.on("error", () => downstream.destroy()); + }); + await once(relay.listen(0, "127.0.0.1"), "listening"); + + const port = relay.address().port; + // `servername: "agent1"` is the name the certificate carries. + const accepted = { + host: "127.0.0.1", + port, + ca: pem("ca1-cert.pem"), + servername: "agent1", + key: pem("agent3-key.pem"), + cert: pem("agent3-cert.pem"), + }; + const refused = { ...accepted, servername: "not-agent1" }; + const pinnedKeyError = () => Object.assign(new Error("not the pinned key"), { code: "ERR_PINNED_KEY" }); + + let client = []; + function watch(socket) { + // node reports the junk record too, after the error of the refusal. + socket.on("error", error => client.some(event => event.startsWith("error:")) || client.push(`error:${error.code}`)); + return new Promise(resolve => + socket.on("close", hadError => { + client.push(`close:${hadError}`); + resolve(); + }), + ); + } + + // A full handshake that keeps its session, then the options of a second one. + async function resume(second) { + const first = tls.connect(accepted); + const firstClosed = watch(first); + // A TLS 1.3 ticket arrives after the handshake, so the socket has to read. + first.resume(); + const [session] = await once(first, "session"); + first.end(); + await Promise.all([firstClosed, serverSaw.promise]); + client = []; + serverSaw = Promise.withResolvers(); + return { ...second, session }; + } + + let closed; + switch (mode) { + case "checkServerIdentity": + case "a junk record behind the server's Finished": + closed = watch(tls.connect(refused)); + break; + case "checkServerIdentity function": + closed = watch(tls.connect({ ...accepted, checkServerIdentity: pinnedKeyError })); + break; + case "checkServerIdentity function that writes to another TLS socket": { + const checkServerIdentity = () => { + second.socket.write("checked"); + return pinnedKeyError(); + }; + closed = watch(tls.connect({ ...accepted, checkServerIdentity })); + break; + } + case "destroy() on a resumed session": { + const socket = tls.connect(await resume(accepted), () => { + client.push(`reused:${socket.isSessionReused()}`); + socket.destroy(); + }); + closed = watch(socket); + break; + } + case "tls.connect({ socket })": { + const raw = net.connect(port, "127.0.0.1"); + raw.on("error", () => {}); + await once(raw, "connect"); + closed = watch(tls.connect({ ...refused, socket: raw })); + break; + } + case "https.request": { + const request = https.request({ ...refused, agent: false }); + closed = new Promise(resolve => + request.on("error", error => { + client.push(`error:${error.code}`); + resolve(); + }), + ); + request.end(); + break; + } + case "http2.connect": { + const session = http2.connect(`https://127.0.0.1:${port}`, refused); + session.on("error", error => client.push(`error:${error.code}`)); + closed = new Promise(resolve => session.on("close", resolve)); + break; + } + default: { + const act = inSecureConnect[mode]; + if (!act) throw new Error(`unknown mode ${mode}`); + const socket = tls.connect(accepted, () => act(socket)); + closed = watch(socket); + } + } + + const [saw] = await Promise.all([serverSaw.promise, closed]); + second?.close(); + relay.close(); + server.close(); + return { client, server: saw, ...wire(Buffer.concat(fromClient)) }; +} diff --git a/test/js/node/tls/tls-server-refuse-fixture.mjs b/test/js/node/tls/tls-server-refuse-fixture.mjs new file mode 100644 index 000000000000..e4d5346796dc --- /dev/null +++ b/test/js/node/tls/tls-server-refuse-fixture.mjs @@ -0,0 +1,58 @@ +// A TLS server that turns the client down once its own handshake is done. +// report(mode, version) makes one connection and resolves with what both +// sides saw. node-tls-connect.test.ts runs it in bun and in node, and expects +// the same reports from both. +// +// In a full TLS 1.2 handshake the server's Finished is the last message, and +// the server's handshake callback runs before it is sent. In TLS 1.3 the +// server has nothing left to send at that point. +import { once } from "node:events"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import tls from "node:tls"; + +const keys = join(import.meta.dirname, "..", "test", "fixtures", "keys"); +const pem = name => readFileSync(join(keys, name)); + +export async function report(mode, version) { + // The client certificate (agent3) is not signed by ca1, so the server cannot verify it. + const serverDone = Promise.withResolvers(); + let serverEvent = null; + const server = tls.createServer({ + key: pem("agent1-key.pem"), + cert: pem("agent1-cert.pem"), + ca: pem("ca1-cert.pem"), + requestCert: true, + rejectUnauthorized: mode === "requestCert and rejectUnauthorized", + minVersion: version, + maxVersion: version, + }); + server.on("secureConnection", socket => { + serverEvent = "secureConnection"; + socket.on("error", () => {}); + socket.on("close", () => serverDone.resolve()); + if (mode === "destroy() in 'secureConnection'") socket.destroy(); + else socket.end(); + }); + server.on("tlsClientError", () => { + serverEvent = "tlsClientError"; + serverDone.resolve(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + + const client = { secureConnect: false, error: null }; + const socket = tls.connect({ + host: "127.0.0.1", + port: server.address().port, + ca: pem("ca1-cert.pem"), + servername: "agent1", + key: pem("agent3-key.pem"), + cert: pem("agent3-cert.pem"), + }); + socket.on("secureConnect", () => (client.secureConnect = true)); + socket.on("error", error => (client.error = error.code)); + socket.resume(); + await Promise.all([new Promise(resolve => socket.on("close", resolve)), serverDone.promise]); + server.close(); + return { client, server: serverEvent }; +} From b97b93fa8eeb89d3f9a293cbccae995b055e1a98 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 25 Sep 2026 03:41:14 +0000 Subject: [PATCH 2/6] tls: report the handshake in the close_notify arm of the read too A close_notify in the read that finishes the handshake reached the handshake callback only through the retry of a parked write. The arm now reports the handshake itself, like the two arms for a failed read, so it does not depend on that flag. The new row seals a close_notify under the server's application traffic secret and lets the relay send it behind the server's Finished. --- packages/bun-usockets/src/crypto/openssl.c | 18 ++++---- test/js/node/tls/node-tls-connect.test.ts | 1 + test/js/node/tls/tls-client-close-fixture.mjs | 41 +++++++++++++++++-- 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index ba016f38891e..3dc4a7d2b180 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -2364,12 +2364,13 @@ struct us_socket_t *us_internal_ssl_on_writable(struct us_socket_t *s) { return s; } -/* A read that finished the handshake and then failed (a bad record, broken - * framing) reports the handshake before it closes, as the no-data completion - * in us_internal_ssl_on_data does. The owner may turn the peer down there, and - * that close releases the held flight: a peer must not collect it by sending - * junk behind its Finished. Returns 0 when the socket is gone. */ -static int ssl_report_handshake_before_failed_read(struct us_socket_t *s, struct loop_ssl_data *loop_ssl_data) { +/* A read that finished the handshake and then ends the connection (the peer's + * close_notify, a bad record, broken framing) reports the handshake before it + * closes, as the no-data completion in us_internal_ssl_on_data does. The owner + * may turn the peer down there, and that close releases the held flight: a + * peer must not collect it by sending an alert or junk behind its Finished. + * Returns 0 when the socket is gone. */ +static int ssl_report_handshake_before_close(struct us_socket_t *s, struct loop_ssl_data *loop_ssl_data) { if (s->ssl_handshake_state != HANDSHAKE_PENDING || !SSL_is_init_finished(s_ssl(s))) return 1; ERR_clear_error(); ssl_trigger_handshake(s, 1); @@ -2485,6 +2486,7 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i if (ssl_gone(s)) return NULL; err = SSL_ERROR_SSL; } else if (err == SSL_ERROR_ZERO_RETURN) { + if (!ssl_report_handshake_before_close(s, loop_ssl_data)) return NULL; /* Remote close_notify. A NewSessionTicket that rode in ahead of the * close_notify was parked by the new-session callback; deliver it * first (wire order - the ticket preceded these bytes, and Node's @@ -2533,7 +2535,7 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i return s; } - if (!ssl_report_handshake_before_failed_read(s, loop_ssl_data)) return NULL; + if (!ssl_report_handshake_before_close(s, loop_ssl_data)) return NULL; if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { ssl_park_fatal_reason(s); } @@ -2546,7 +2548,7 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i /* If the BIO still has unread ciphertext at this point, the TLS * framing is broken — close. */ if (loop_ssl_data->ssl_read_input_length) { - if (!ssl_report_handshake_before_failed_read(s, loop_ssl_data)) return NULL; + if (!ssl_report_handshake_before_close(s, loop_ssl_data)) return NULL; return ssl_close(s, 0, NULL); } /* SSL_read drove the handshake to completion but returned no app diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index 1a2d3656427b..d13b364aba3a 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -2629,6 +2629,7 @@ describe("how a TLS client's way of closing reaches the server", () => { ["TLSv1.3", "checkServerIdentity", turnedDown(altnameInvalid, "close:true")], ["TLSv1.3", "checkServerIdentity function", turnedDown("error:ERR_PINNED_KEY", "close:true")], ["TLSv1.3", "a junk record behind the server's Finished", turnedDown(altnameInvalid, "close:true")], + ["TLSv1.3", "a close_notify behind the server's Finished", turnedDown(altnameInvalid, "close:true")], ["TLSv1.3", "tls.connect({ socket })", turnedDown(altnameInvalid, "close:true")], ["TLSv1.3", "https.request", turnedDown(altnameInvalid)], ["TLSv1.3", "http2.connect", turnedDown(altnameInvalid)], diff --git a/test/js/node/tls/tls-client-close-fixture.mjs b/test/js/node/tls/tls-client-close-fixture.mjs index afa897b68ac7..1fc344edc669 100644 --- a/test/js/node/tls/tls-client-close-fixture.mjs +++ b/test/js/node/tls/tls-client-close-fixture.mjs @@ -19,6 +19,7 @@ // A mode that ends with "while another TLS socket is backpressured" first // opens a second TLS connection whose peer stops reading, and writes to it // until the kernel takes no more. +import { createCipheriv, createHmac } from "node:crypto"; import { once } from "node:events"; import { readFileSync } from "node:fs"; import http2 from "node:http2"; @@ -33,6 +34,30 @@ const pem = name => readFileSync(join(keys, name)); // One application data record that does not decrypt. const junkRecord = Buffer.concat([Buffer.from([23, 3, 3, 0, 16]), Buffer.alloc(16, 0xa5)]); +// A close_notify alert as the first record under the server's application +// traffic secret (RFC 8446, sections 5.2 and 7.3). A TLS 1.3 server may send +// records under that secret right behind its Finished. OpenSSL and BoringSSL +// do not send this alert during their handshake, so the relay seals it. +function sealedCloseNotify(serverHello, secret) { + const cipherSuite = serverHello.readUInt16BE(44 + serverHello[43]); + const [hash, cipher, keyLength] = { + 0x1301: ["sha256", "aes-128-gcm", 16], + 0x1302: ["sha384", "aes-256-gcm", 32], + 0x1303: ["sha256", "chacha20-poly1305", 32], + }[cipherSuite]; + const expandLabel = (label, length) => + createHmac(hash, secret) + .update( + Buffer.concat([Buffer.from([0, length, 6 + label.length]), Buffer.from("tls13 " + label), Buffer.from([0, 1])]), + ) + .digest() + .subarray(0, length); + const header = Buffer.from([23, 3, 3, 0, 19]); + const seal = createCipheriv(cipher, expandLabel("key", keyLength), expandLabel("iv", 12), { authTagLength: 16 }); + seal.setAAD(header); + return Buffer.concat([header, seal.update(Buffer.from([1, 0, 21])), seal.final(), seal.getAuthTag()]); +} + function wire(bytes) { let alerts = 0; for (let at = 0; at + 5 <= bytes.length; at += 5 + bytes.readUInt16BE(at + 3)) { @@ -144,6 +169,11 @@ export async function report(fullMode, version) { socket.on("close", () => serverSaw.resolve({ event: "secureConnection", peerCN, data, error })); }); server.on("tlsClientError", error => serverSaw.resolve({ event: "tlsClientError", code: error.code })); + const serverSecret = Promise.withResolvers(); + server.on("keylog", line => { + const [label, , secret] = line.toString().trim().split(" "); + if (label === "SERVER_TRAFFIC_SECRET_0") serverSecret.resolve(Buffer.from(secret, "hex")); + }); await once(server.listen(0, "127.0.0.1"), "listening"); let fromClient = []; @@ -155,10 +185,14 @@ export async function report(fullMode, version) { fromClient.push(chunk); upstream.write(chunk); }); - upstream.on("data", chunk => { + upstream.on("data", async chunk => { // The server's first chunk is its whole flight, up to its Finished. - const junk = mode === "a junk record behind the server's Finished" && serverChunks++ === 0; - downstream.write(junk ? Buffer.concat([chunk, junkRecord]) : chunk); + if (serverChunks++ === 0 && mode === "a junk record behind the server's Finished") { + chunk = Buffer.concat([chunk, junkRecord]); + } else if (serverChunks === 1 && mode === "a close_notify behind the server's Finished") { + chunk = Buffer.concat([chunk, sealedCloseNotify(chunk, await serverSecret.promise)]); + } + downstream.write(chunk); }); downstream.on("end", () => upstream.end()); upstream.on("end", () => downstream.end()); @@ -210,6 +244,7 @@ export async function report(fullMode, version) { switch (mode) { case "checkServerIdentity": case "a junk record behind the server's Finished": + case "a close_notify behind the server's Finished": closed = watch(tls.connect(refused)); break; case "checkServerIdentity function": From 98550586bbc94180047d549d0b0322aa5efb72bd Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 25 Sep 2026 04:32:51 +0000 Subject: [PATCH 3/6] tls: hold the final flight only while the spill slot is free, as before With another socket's spill pending, the rest of a flight that the kernel takes in part has no place to wait. The earlier commits of this branch held the flight in that case too, and a short write then stalled the connection. The handshake batching and us_internal_ssl_write have their gates of main again, so that flight is written through and BoringSSL retries it. Of the reads that end the connection, only the failing read reports the handshake before it closes. The close_notify arm already reports it through ssl_retry_parked_write, and a read that leaves ciphertext behind has not finished the handshake. No test fails without those two calls. The relay of the test fixture opens the server's handshake records to find its Finished, so the record it adds does not depend on how TCP splits the flight. --- packages/bun-types/bun.d.ts | 9 +- packages/bun-usockets/src/crypto/openssl.c | 54 +++----- test/js/node/tls/node-tls-connect.test.ts | 6 - test/js/node/tls/tls-client-close-fixture.mjs | 120 +++++++++++------- 4 files changed, 94 insertions(+), 95 deletions(-) diff --git a/packages/bun-types/bun.d.ts b/packages/bun-types/bun.d.ts index 9ada427e6701..3a69f1af2a23 100644 --- a/packages/bun-types/bun.d.ts +++ b/packages/bun-types/bun.d.ts @@ -7145,10 +7145,11 @@ declare module "bun" { * `close_notify` alert. * * Called from the `handshake` callback of a TLS socket, `close()` turns the - * peer down: the rest of the handshake is not sent. For a TLS 1.3 client - * that includes its certificate. {@link terminate terminate()} does the - * same. A socket with no `handshake` callback gets its `open` callback at - * that point, so the same holds there. + * peer down: Bun drops the part of the handshake that it has not sent yet. + * For a TLS 1.3 client that is usually its last flight, which carries the + * client certificate. {@link terminate terminate()} does the same. A socket + * with no `handshake` callback gets its `open` callback at that point, so + * the same holds there. * * Use {@link end end()} for a graceful close. * diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index 3dc4a7d2b180..490b36125137 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -550,13 +550,12 @@ static int BIO_s_custom_write(BIO *bio, const char *data, int length) { loop_ssl_data->ssl_write_batch_owner != loop_ssl_data->ssl_socket; if (loop_ssl_data->ssl_write_batching && !batch_is_foreign && - loop_ssl_data->ssl_spill_owner != loop_ssl_data->ssl_socket) { + loop_ssl_data->ssl_spill_owner == NULL) { /* Append the sealed record; the batch hits the kernel once, after * SSL_write returns. Reporting the full length keeps BoringSSL sealing - * the next record instead of parking a partial one. Skipped while this - * socket's own spill is pending: those bytes go first. us_internal_ssl_write - * does not batch while any spill is pending, so with another socket's - * spill this is the handshake hold of us_internal_ssl_on_data. */ + * the next record instead of parking a partial one. Skipped while a + * spill occupies the slot: a later short flush could not park its + * remainder without clobbering that socket's pending ciphertext. */ unsigned int needed = loop_ssl_data->ssl_write_batch_len + (unsigned int)length; if (needed > loop_ssl_data->ssl_write_batch_cap) { unsigned int new_cap = loop_ssl_data->ssl_write_batch_cap ? loop_ssl_data->ssl_write_batch_cap : 65536; @@ -2364,24 +2363,6 @@ struct us_socket_t *us_internal_ssl_on_writable(struct us_socket_t *s) { return s; } -/* A read that finished the handshake and then ends the connection (the peer's - * close_notify, a bad record, broken framing) reports the handshake before it - * closes, as the no-data completion in us_internal_ssl_on_data does. The owner - * may turn the peer down there, and that close releases the held flight: a - * peer must not collect it by sending an alert or junk behind its Finished. - * Returns 0 when the socket is gone. */ -static int ssl_report_handshake_before_close(struct us_socket_t *s, struct loop_ssl_data *loop_ssl_data) { - if (s->ssl_handshake_state != HANDSHAKE_PENDING || !SSL_is_init_finished(s_ssl(s))) return 1; - ERR_clear_error(); - ssl_trigger_handshake(s, 1); - if (ssl_gone(s)) return 0; - loop_ssl_data->ssl_socket = s; - if (loop_ssl_data->ssl_write_batch_len && loop_ssl_data->ssl_write_batch_owner == s) { - ssl_flush_write_batch(loop_ssl_data, s); - } - return 1; -} - struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, int length) { /* See ssl_update_handshake: start this socket's SSL processing with a clean * per-thread error queue so a captured reason cannot belong to another @@ -2433,15 +2414,12 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i * down right after the handshake (node's post-verify destroy, #40653) * close with the second segment unread, which turns its FIN teardown * into an RST and a bogus ECONNRESET at this side. Node's memory BIO - * drained once per cycle has the same single-segment shape. The hold - * also decides whether the owner's refusal in on_handshake can still - * withhold the flight, so another socket's pending spill does not switch - * it off: a short flush then fails this socket (ssl_flush_write_batch) - * and leaves that spill alone. A spill of this socket's own goes first, - * so its records are written through as before. */ + * drained once per cycle has the same single-segment shape. Gated on a + * free spill slot like us_internal_ssl_write, so a short flush cannot + * clobber another socket's pending ciphertext. */ int hs_batching = s->ssl_handshake_state == HANDSHAKE_PENDING && !loop_ssl_data->ssl_write_batching && - loop_ssl_data->ssl_spill_owner != s; + !loop_ssl_data->ssl_spill_owner; if (hs_batching) loop_ssl_data->ssl_write_batching = 1; unsigned char ssl_was_in_use = s->ssl_in_use; s->ssl_in_use = 1; @@ -2486,7 +2464,6 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i if (ssl_gone(s)) return NULL; err = SSL_ERROR_SSL; } else if (err == SSL_ERROR_ZERO_RETURN) { - if (!ssl_report_handshake_before_close(s, loop_ssl_data)) return NULL; /* Remote close_notify. A NewSessionTicket that rode in ahead of the * close_notify was parked by the new-session callback; deliver it * first (wire order - the ticket preceded these bytes, and Node's @@ -2535,7 +2512,16 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i return s; } - if (!ssl_report_handshake_before_close(s, loop_ssl_data)) return NULL; + if (s->ssl_handshake_state == HANDSHAKE_PENDING && SSL_is_init_finished(s_ssl(s))) { + /* The read that finished the handshake failed on a later record. + * Report the handshake before the close, as the no-data completion + * below does. The owner may turn the peer down there, and that + * close releases the held flight: a peer must not collect it with + * a bad record behind its Finished. */ + ERR_clear_error(); + ssl_trigger_handshake(s, 1); + if (ssl_gone(s)) return NULL; + } if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { ssl_park_fatal_reason(s); } @@ -2548,7 +2534,6 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i /* If the BIO still has unread ciphertext at this point, the TLS * framing is broken — close. */ if (loop_ssl_data->ssl_read_input_length) { - if (!ssl_report_handshake_before_close(s, loop_ssl_data)) return NULL; return ssl_close(s, 0, NULL); } /* SSL_read drove the handshake to completion but returned no app @@ -2744,9 +2729,6 @@ int us_internal_ssl_write(struct us_socket_t *s, const char *data, int length) { * layers above fire 'finish' and close before the data reached the wire. */ int outer_batching = loop_ssl_data->ssl_write_batching; int batching = (loop_ssl_data->ssl_spill_owner == NULL); - /* A flight held across on_handshake while another socket's spill is pending - * goes before the records this write sends through. */ - if (!batching && !ssl_flush_write_batch(loop_ssl_data, s)) return 0; loop_ssl_data->ssl_write_batching = batching; int total = 0; diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index d13b364aba3a..a3085794dbf7 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -2623,7 +2623,6 @@ describe("how a TLS client's way of closing reaches the server", () => { alerts: 0, }); const altnameInvalid = "error:ERR_TLS_CERT_ALTNAME_INVALID"; - const backpressured = " while another TLS socket is backpressured"; const rows = [ ["TLSv1.3", "checkServerIdentity", turnedDown(altnameInvalid, "close:true")], @@ -2639,10 +2638,6 @@ describe("how a TLS client's way of closing reaches the server", () => { ["TLSv1.3", "destroy() from queueMicrotask", turnedDown("close:false")], ["TLSv1.3", "destroy() on a resumed session", turnedDown("reused:true", "close:false")], ["TLSv1.2", "destroy() on a resumed session", turnedDown("reused:true", "close:false")], - - // The flight is held with or without other traffic on the event loop. - ["TLSv1.3", "checkServerIdentity" + backpressured, turnedDown(altnameInvalid, "close:true")], - ["TLSv1.3", "destroy()" + backpressured, turnedDown("close:false")], [ "TLSv1.3", "checkServerIdentity function that writes to another TLS socket", @@ -2653,7 +2648,6 @@ describe("how a TLS client's way of closing reaches the server", () => { ["TLSv1.3", "end()", delivered("", 1)], ["TLSv1.3", "end(data)", delivered("hello", 1)], ["TLSv1.3", "destroySoon()", delivered("", 1)], - ["TLSv1.3", "end()" + backpressured, delivered("", 1)], // In a full TLS 1.2 handshake the client's flight leaves before the server's Finished. ["TLSv1.2", "checkServerIdentity", delivered("", 0, [altnameInvalid, "close:true"])], diff --git a/test/js/node/tls/tls-client-close-fixture.mjs b/test/js/node/tls/tls-client-close-fixture.mjs index 1fc344edc669..68a809412af8 100644 --- a/test/js/node/tls/tls-client-close-fixture.mjs +++ b/test/js/node/tls/tls-client-close-fixture.mjs @@ -15,11 +15,7 @@ // - `alerts` counts the client's alert records. A TLS 1.3 alert travels as an // application data record of 19 bytes: the alert, the inner content type and // the AEAD tag. Nothing else a mode sends has that size. -// -// A mode that ends with "while another TLS socket is backpressured" first -// opens a second TLS connection whose peer stops reading, and writes to it -// until the kernel takes no more. -import { createCipheriv, createHmac } from "node:crypto"; +import { createCipheriv, createDecipheriv, createHmac } from "node:crypto"; import { once } from "node:events"; import { readFileSync } from "node:fs"; import http2 from "node:http2"; @@ -34,11 +30,10 @@ const pem = name => readFileSync(join(keys, name)); // One application data record that does not decrypt. const junkRecord = Buffer.concat([Buffer.from([23, 3, 3, 0, 16]), Buffer.alloc(16, 0xa5)]); -// A close_notify alert as the first record under the server's application -// traffic secret (RFC 8446, sections 5.2 and 7.3). A TLS 1.3 server may send -// records under that secret right behind its Finished. OpenSSL and BoringSSL -// do not send this alert during their handshake, so the relay seals it. -function sealedCloseNotify(serverHello, secret) { +// The record protection keys of a TLS 1.3 traffic secret (RFC 8446, section +// 7.3). `serverHello` starts with the ServerHello record, which names the +// cipher suite. +function trafficKeys(serverHello, secret) { const cipherSuite = serverHello.readUInt16BE(44 + serverHello[43]); const [hash, cipher, keyLength] = { 0x1301: ["sha256", "aes-128-gcm", 16], @@ -52,12 +47,56 @@ function sealedCloseNotify(serverHello, secret) { ) .digest() .subarray(0, length); + return { cipher, key: expandLabel("key", keyLength), iv: expandLabel("iv", 12) }; +} + +// A close_notify alert as the first record under the server's application +// traffic secret (RFC 8446, section 5.2). A TLS 1.3 server may send records +// under that secret right behind its Finished. OpenSSL and BoringSSL do not +// send this alert during their handshake, so the relay seals it. +function sealedCloseNotify(serverHello, secret) { + const { cipher, key, iv } = trafficKeys(serverHello, secret); const header = Buffer.from([23, 3, 3, 0, 19]); - const seal = createCipheriv(cipher, expandLabel("key", keyLength), expandLabel("iv", 12), { authTagLength: 16 }); + const seal = createCipheriv(cipher, key, iv, { authTagLength: 16 }); seal.setAAD(header); return Buffer.concat([header, seal.update(Buffer.from([1, 0, 21])), seal.final(), seal.getAuthTag()]); } +// The length of the server's first flight: all records up to the one that +// completes its Finished. 0 while `bytes` does not hold the whole flight. The +// relay opens the records under the server's handshake traffic secret to find +// the Finished, so the way TCP splits the flight does not matter. +function flightLength(bytes, secret) { + let keys; + let messages = Buffer.alloc(0); + let sequence = 0; + for (let at = 0; at + 5 <= bytes.length; ) { + const end = at + 5 + bytes.readUInt16BE(at + 3); + if (end > bytes.length) return 0; + if (bytes[at] === 23) { + keys ??= trafficKeys(bytes, secret); + const nonce = Buffer.from(keys.iv); + nonce[11] ^= sequence++; + const open = createDecipheriv(keys.cipher, keys.key, nonce, { authTagLength: 16 }); + open.setAAD(bytes.subarray(at, at + 5)); + open.setAuthTag(bytes.subarray(end - 16, end)); + const inner = Buffer.concat([open.update(bytes.subarray(at + 5, end - 16)), open.final()]); + // A record holds its content, then the content type, then zero padding. + const contentType = inner.findLastIndex(byte => byte !== 0); + messages = Buffer.concat([messages, inner.subarray(0, contentType)]); + for (let message = 0; message + 4 <= messages.length; ) { + const next = message + 4 + messages.readUIntBE(message + 1, 3); + if (next > messages.length) break; + // Handshake message type 20 is Finished. + if (messages[message] === 20) return end; + message = next; + } + } + at = end; + } + return 0; +} + function wire(bytes) { let alerts = 0; for (let at = 0; at + 5 <= bytes.length; at += 5 + bytes.readUInt16BE(at + 3)) { @@ -95,31 +134,16 @@ const inSecureConnect = { "destroySoon()": socket => socket.destroySoon(), }; -const backpressured = " while another TLS socket is backpressured"; - -// A TLS connection of this process whose peer is a TCP relay. `stall()` makes -// the relay stop reading and writes 32 MB: the kernel takes a part, and the -// rest of the sealed records waits in the TLS layer. +// A second TLS connection of this process. async function secondConnection() { const server = tls.createServer({ key: pem("agent1-key.pem"), cert: pem("agent1-cert.pem") }, socket => { socket.on("error", () => {}); socket.resume(); }); await once(server.listen(0, "127.0.0.1"), "listening"); - let downstream; - const relay = net.createServer(socket => { - downstream = socket; - const upstream = net.connect(server.address().port, "127.0.0.1"); - downstream.on("data", chunk => upstream.write(chunk)); - upstream.on("data", chunk => downstream.write(chunk)); - downstream.on("error", () => upstream.destroy()); - upstream.on("error", () => downstream.destroy()); - downstream.on("close", () => upstream.destroy()); - }); - await once(relay.listen(0, "127.0.0.1"), "listening"); const socket = tls.connect({ host: "127.0.0.1", - port: relay.address().port, + port: server.address().port, ca: pem("ca1-cert.pem"), servername: "agent1", }); @@ -127,26 +151,16 @@ async function secondConnection() { await once(socket, "secureConnect"); return { socket, - stall() { - downstream.pause(); - socket.write(Buffer.alloc(32 * 1024 * 1024)); - }, close() { socket.destroy(); - downstream.destroy(); - relay.close(); server.close(); }, }; } -export async function report(fullMode, version) { - const mode = fullMode.endsWith(backpressured) ? fullMode.slice(0, -backpressured.length) : fullMode; +export async function report(mode, version) { const second = - mode !== fullMode || mode === "checkServerIdentity function that writes to another TLS socket" - ? await secondConnection() - : null; - if (mode !== fullMode) second.stall(); + mode === "checkServerIdentity function that writes to another TLS socket" ? await secondConnection() : null; // agent1 is signed by ca1 and names only "agent1". let serverSaw = Promise.withResolvers(); const server = tls.createServer({ @@ -169,28 +183,36 @@ export async function report(fullMode, version) { socket.on("close", () => serverSaw.resolve({ event: "secureConnection", peerCN, data, error })); }); server.on("tlsClientError", error => serverSaw.resolve({ event: "tlsClientError", code: error.code })); - const serverSecret = Promise.withResolvers(); + // The server derives its secrets before it sends the flight they protect. + const secrets = {}; server.on("keylog", line => { const [label, , secret] = line.toString().trim().split(" "); - if (label === "SERVER_TRAFFIC_SECRET_0") serverSecret.resolve(Buffer.from(secret, "hex")); + secrets[label] = Buffer.from(secret, "hex"); }); await once(server.listen(0, "127.0.0.1"), "listening"); + // What the relay puts behind the server's Finished, in the same write. + const behindFinished = { + "a junk record behind the server's Finished": () => junkRecord, + "a close_notify behind the server's Finished": flight => sealedCloseNotify(flight, secrets.SERVER_TRAFFIC_SECRET_0), + }[mode]; + let fromClient = []; const relay = net.createServer(downstream => { fromClient = []; - let serverChunks = 0; + let flight = behindFinished ? Buffer.alloc(0) : null; const upstream = net.connect(server.address().port, "127.0.0.1"); downstream.on("data", chunk => { fromClient.push(chunk); upstream.write(chunk); }); - upstream.on("data", async chunk => { - // The server's first chunk is its whole flight, up to its Finished. - if (serverChunks++ === 0 && mode === "a junk record behind the server's Finished") { - chunk = Buffer.concat([chunk, junkRecord]); - } else if (serverChunks === 1 && mode === "a close_notify behind the server's Finished") { - chunk = Buffer.concat([chunk, sealedCloseNotify(chunk, await serverSecret.promise)]); + upstream.on("data", chunk => { + if (flight) { + flight = Buffer.concat([flight, chunk]); + const length = flightLength(flight, secrets.SERVER_HANDSHAKE_TRAFFIC_SECRET); + if (!length) return; + chunk = Buffer.concat([flight.subarray(0, length), behindFinished(flight), flight.subarray(length)]); + flight = null; } downstream.write(chunk); }); From 8722be0d76b7d6f9b3ec082afaf0e673ae946dc0 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 25 Sep 2026 19:34:48 +0000 Subject: [PATCH 4/6] node:tls: destroy() in the handshake callback releases the held final flight The release is node:net's now. Socket.prototype._destroy asks the native layer to drop the handshake flight that usockets holds across the handshake callback (us_socket_release_held_flight). It asks at its entry, because the raw half of a shared-fd pair closes its handle two loop turns later. us_internal_ssl_close sends a held flight for every close code again, as on main, so close() and terminate() of Bun.connect and Bun.listen keep their behaviour. The bun.d.ts edits are gone. A read that finishes the handshake and then gets a renegotiation request reports the handshake and sends the flight before it renegotiates. Before, the flight stayed in the loop's batch for the life of the connection. --- packages/bun-types/bun.d.ts | 17 +-- packages/bun-usockets/src/crypto/openssl.c | 68 +++++++---- packages/bun-usockets/src/libusockets.h | 4 + src/js/node/net.ts | 10 ++ src/runtime/socket/mod.rs | 3 +- src/runtime/socket/socket_body.rs | 18 +++ src/uws_sys/socket.rs | 8 ++ src/uws_sys/us_socket_t.rs | 7 ++ test/js/node/tls/node-tls-connect.test.ts | 69 +++++------ test/js/node/tls/tls-client-close-fixture.mjs | 115 +++++++++++++++--- 10 files changed, 220 insertions(+), 99 deletions(-) diff --git a/packages/bun-types/bun.d.ts b/packages/bun-types/bun.d.ts index 3a69f1af2a23..221aa2fea1f6 100644 --- a/packages/bun-types/bun.d.ts +++ b/packages/bun-types/bun.d.ts @@ -6765,7 +6765,7 @@ declare module "bun" { /** * Forcefully closes the socket connection immediately. This is an abrupt termination, unlike the graceful shutdown initiated by `end()`. * It uses `SO_LINGER` with `l_onoff=1` and `l_linger=0` before calling `close(2)`. - * Consider using {@link end end()} for a graceful shutdown. + * Consider using {@link close close()} or {@link end end()} for graceful shutdowns. * * @example * ```ts @@ -7140,21 +7140,12 @@ declare module "bun" { upgradeTLS(options: TLSUpgradeOptions): [raw: Socket, tls: Socket]; /** - * Closes the socket at once. Data that `write()` already accepted is still - * delivered, then the connection ends with a TCP FIN. A TLS socket sends no - * `close_notify` alert. + * Closes the socket. * - * Called from the `handshake` callback of a TLS socket, `close()` turns the - * peer down: Bun drops the part of the handshake that it has not sent yet. - * For a TLS 1.3 client that is usually its last flight, which carries the - * client certificate. {@link terminate terminate()} does the same. A socket - * with no `handshake` callback gets its `open` callback at that point, so - * the same holds there. - * - * Use {@link end end()} for a graceful close. + * This is a wrapper around `end()` and `shutdown()`. * * @see {@link end} - * @see {@link terminate} + * @see {@link shutdown} */ close(): void; diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index 490b36125137..514b9fb260a2 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -1128,6 +1128,19 @@ void us_socket_set_inline_reject(struct us_socket_t *s) { SSL_set_verify(s_ssl(s), SSL_VERIFY_PEER, us_inline_reject_verify_callback); } +/* node:net destroy() inside the handshake callback turns the peer down. The + * flight that is held across that callback is dropped, and the session takes + * no more writes: the peer never gets our Finished. node drops its pending + * output there: + * https://github.com/nodejs/node/blob/v26.10.0/src/crypto/crypto_tls.cc#L1409-L1433 */ +void us_socket_release_held_flight(struct us_socket_t *s) { + if (!s->ssl || us_socket_is_closed(s)) return; + struct loop_ssl_data *loop_ssl_data = (struct loop_ssl_data *)s->group->loop->data.ssl_data; + if (!loop_ssl_data || !loop_ssl_data->ssl_write_batch_len || loop_ssl_data->ssl_write_batch_owner != s) return; + ssl_release_batch(s->group->loop, s); + s->ssl_fatal_error = 1; +} + /* Drop the strdup'd passphrase. Called as soon as private-key load completes * (the only consumer of the passwd_cb), so the secret never outlives ctx * construction and SSL_CTX_free() is sufficient on every later path. Also @@ -2018,26 +2031,14 @@ struct us_socket_t *us_internal_ssl_close(struct us_socket_t *s, int code, void return s; } { - /* Ciphertext batched in this dispatch and still held: the handshake's - * final flight, or a fatal alert sealed by a failing SSL_read (the driver - * closes with code 0 for that). A graceful close sends it before its - * close_notify/FIN. A partial write spills; the graceful-close deferral - * below then waits for the drain. - * A forceful close from inside the dispatch is the owner turning the peer - * down, so the flight is released: under TLS 1.3 it carries the client's - * Certificate. node drops its pending output when destroy() runs in the - * handshake callback: - * https://github.com/nodejs/node/blob/v26.10.0/src/crypto/crypto_tls.cc#L1409-L1433 - * The peer never gets our Finished, so it could not read a close_notify. */ + /* Ciphertext batched in this dispatch and still held (the handshake's + * final flight, a fatal alert sealed by a failing SSL_read) goes to the + * wire before the close_notify/FIN this teardown sends. A partial write + * spills; the graceful-close deferral below then waits for the drain. */ struct loop_ssl_data *loop_ssl_data = (struct loop_ssl_data *)s->group->loop->data.ssl_data; if (loop_ssl_data && loop_ssl_data->ssl_write_batch_len && loop_ssl_data->ssl_write_batch_owner == s && !us_socket_is_closed(s)) { - if (code == LIBUS_SOCKET_CLOSE_CODE_CLEAN_SHUTDOWN) { - ssl_flush_write_batch(loop_ssl_data, s); - } else { - ssl_release_batch(s->group->loop, s); - s->ssl_fatal_error = 1; - } + ssl_flush_write_batch(loop_ssl_data, s); } } /* Neither node's `_handle.close()` (FAST_SHUTDOWN, no reason) nor a graceful @@ -2363,6 +2364,27 @@ struct us_socket_t *us_internal_ssl_on_writable(struct us_socket_t *s) { return s; } +/* A read can finish the handshake and then leave the arms of + * us_internal_ssl_on_data that report it: a later record fails, or the peer + * asks to renegotiate. Report the handshake first, as those arms do. The owner + * may turn the peer down there. If it does not, the flight that was held for + * it goes out before anything else. Returns 0 when the socket is gone. */ +static int ssl_report_finished_handshake(struct us_socket_t *s, struct loop_ssl_data *loop_ssl_data) { + if (s->ssl_handshake_state != HANDSHAKE_PENDING || !SSL_is_init_finished(s_ssl(s))) return 1; + char *saved_input = loop_ssl_data->ssl_read_input; + unsigned int saved_length = loop_ssl_data->ssl_read_input_length; + unsigned int saved_offset = loop_ssl_data->ssl_read_input_offset; + ERR_clear_error(); + ssl_trigger_handshake(s, 1); + if (ssl_gone(s)) return 0; + loop_ssl_data->ssl_read_input = saved_input; + loop_ssl_data->ssl_read_input_length = saved_length; + loop_ssl_data->ssl_read_input_offset = saved_offset; + loop_ssl_data->ssl_socket = s; + ssl_flush_write_batch(loop_ssl_data, s); + return 1; +} + struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, int length) { /* See ssl_update_handshake: start this socket's SSL processing with a clean * per-thread error queue so a captured reason cannot belong to another @@ -2460,6 +2482,7 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i if (err != SSL_ERROR_WANT_READ && err != SSL_ERROR_WANT_WRITE && err != SSL_ERROR_PENDING_CERTIFICATE) { if (err == SSL_ERROR_WANT_RENEGOTIATE) { + if (!ssl_report_finished_handshake(s, loop_ssl_data)) return NULL; if (ssl_renegotiate(s)) continue; if (ssl_gone(s)) return NULL; err = SSL_ERROR_SSL; @@ -2512,16 +2535,7 @@ struct us_socket_t *us_internal_ssl_on_data(struct us_socket_t *s, char *data, i return s; } - if (s->ssl_handshake_state == HANDSHAKE_PENDING && SSL_is_init_finished(s_ssl(s))) { - /* The read that finished the handshake failed on a later record. - * Report the handshake before the close, as the no-data completion - * below does. The owner may turn the peer down there, and that - * close releases the held flight: a peer must not collect it with - * a bad record behind its Finished. */ - ERR_clear_error(); - ssl_trigger_handshake(s, 1); - if (ssl_gone(s)) return NULL; - } + if (!ssl_report_finished_handshake(s, loop_ssl_data)) return NULL; if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { ssl_park_fatal_reason(s); } diff --git a/packages/bun-usockets/src/libusockets.h b/packages/bun-usockets/src/libusockets.h index b4578da63772..00f06b020a49 100644 --- a/packages/bun-usockets/src/libusockets.h +++ b/packages/bun-usockets/src/libusockets.h @@ -382,6 +382,10 @@ void us_socket_start_tls_handshake(us_socket_r s) nonnull_fn_decl; * server that fails verification. Must run before the handshake is driven * (on_open, or between adopt_tls and start_tls_handshake). No-op otherwise. */ void us_socket_set_inline_reject(us_socket_r s) nonnull_fn_decl; +/* Drops the handshake flight that is held for `s` across its handshake + * callback, for an owner that turns the peer down there. No-op when `s` holds + * none. */ +void us_socket_release_held_flight(us_socket_r s) nonnull_fn_decl; /* ── Listen ─────────────────────────────────────────────────────────────── * The listener owns: an embedded group for accepted sockets, the SSL_CTX diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 29948a00884a..bc30a6542a0e 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -262,6 +262,9 @@ const addServerName = $newRustFunction("Listener.rs", "jsAddServerName", 3); const upgradeDuplexToTLS = $newRustFunction("runtime/socket/socket.rs", "jsUpgradeDuplexToTLS", 2); // tls.connect({ socket }) upgrade: hostname policy stays with this JS layer. const upgradeTLSDeferred = $newRustFunction("runtime/socket/socket.rs", "jsUpgradeTLSDeferred", 2); +// destroy() inside the handshake callback turns the peer down: the native layer +// drops the handshake flight that it holds for that callback. +const releaseHeldFlight = $newRustFunction("runtime/socket/socket.rs", "jsReleaseHeldFlight", 1); const isNamedPipeSocket = $newRustFunction("runtime/socket/socket.rs", "jsIsNamedPipeSocket", 1); const getBufferedAmount = $newRustFunction("runtime/socket/socket.rs", "jsGetBufferedAmount", 1); @@ -2392,6 +2395,13 @@ Socket.prototype._destroy = function _destroy(err, callback) { $debug("close"); if (this._handle) { $debug("close handle"); + // node drops the pending output of a TLS socket that is destroyed in its + // handshake callback. Some branches below close the handle a loop turn + // later, after the native layer has sent the flight, so ask for it here. + // https://github.com/nodejs/node/blob/v26.10.0/src/crypto/crypto_tls.cc#L1409-L1433 + if (typeof this[bunTlsSymbol] === "function" || this._handle[kAdoptedTLSRaw]) { + releaseHeldFlight(this._handle); + } const isException = err ? true : false; // `bytesRead` and `kBytesWritten` should be accessible after `.destroy()` // this[kBytesRead] = this._handle.bytesRead; diff --git a/src/runtime/socket/mod.rs b/src/runtime/socket/mod.rs index bfe0c55935a1..3ab10173c510 100644 --- a/src/runtime/socket/mod.rs +++ b/src/runtime/socket/mod.rs @@ -121,7 +121,8 @@ pub(crate) use udp_socket::UDPSocket; pub(crate) mod socket { pub(crate) use super::socket_body::{ js_create_socket_pair, js_get_buffered_amount, js_is_named_pipe_socket, - js_set_socket_options, js_upgrade_duplex_to_tls, js_upgrade_tls_deferred, testing_ap_is, + js_release_held_flight, js_set_socket_options, js_upgrade_duplex_to_tls, + js_upgrade_tls_deferred, testing_ap_is, }; } diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index b8e2221930fc..4225793f3d33 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -4652,6 +4652,24 @@ pub(crate) fn js_upgrade_tls_deferred( Err(global.throw(format_args!("Expected a socket instance"))) } +/// node:net's `destroy()`: a socket that is destroyed inside its handshake +/// callback turns the peer down, so the flight held for that callback is +/// dropped. The raw half of an `upgradeTLS` pair shares the TLS socket. +#[bun_jsc::host_fn] +pub(crate) fn js_release_held_flight( + _global: &JSGlobalObject, + callframe: &CallFrame, +) -> JsResult { + jsc::mark_binding!(); + let [socket] = callframe.arguments_as_array::<1>(); + if let Some(this) = socket.as_class_ref::() { + this.socket.get().release_held_flight(); + } else if let Some(this) = socket.as_class_ref::() { + this.socket.get().release_held_flight(); + } + Ok(JSValue::UNDEFINED) +} + #[bun_jsc::host_fn] pub(crate) fn js_upgrade_duplex_to_tls( global: &JSGlobalObject, diff --git a/src/uws_sys/socket.rs b/src/uws_sys/socket.rs index 933188762a57..2add82515541 100644 --- a/src/uws_sys/socket.rs +++ b/src/uws_sys/socket.rs @@ -582,6 +582,14 @@ impl NewSocketHandler { } } + /// Drop the handshake flight that usockets holds across the handshake + /// callback. An SSLWrapper-backed socket holds none. + pub fn release_held_flight(&self) { + if let InternalSocket::Connected(s) = self.socket { + sock(s).release_held_flight(); + } + } + /// The session an SSLWrapper-backed socket got last from the new-session callback, borrowed. pub fn wrapper_latest_session(&self) -> *mut bun_boringssl_sys::SSL_SESSION { match self.socket { diff --git a/src/uws_sys/us_socket_t.rs b/src/uws_sys/us_socket_t.rs index 353d874fe40a..e00b98c85483 100644 --- a/src/uws_sys/us_socket_t.rs +++ b/src/uws_sys/us_socket_t.rs @@ -309,6 +309,12 @@ impl us_socket_t { c::us_socket_set_inline_reject(self); } + /// Drop the handshake flight that is held across the handshake callback. + /// No-op when the socket holds none. + pub fn release_held_flight(&mut self) { + c::us_socket_release_held_flight(self); + } + /// Feed bytes that were already read off the wire (e.g. a ClientHello the /// plain-TCP layer consumed before the upgrade) through the same decrypt /// path as bytes arriving from the kernel. @@ -598,6 +604,7 @@ mod c { ) -> *mut us_socket_t; pub(super) safe fn us_socket_start_tls_handshake(s: &mut us_socket_t); pub(super) safe fn us_socket_set_inline_reject(s: &mut us_socket_t); + pub(super) safe fn us_socket_release_held_flight(s: &mut us_socket_t); } } diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index a3085794dbf7..9d93101dbb93 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -2138,16 +2138,14 @@ it("ending a TLS 1.3 socket from its handshake callback still completes the serv await clientClosed.promise; }); -// terminate() and close() are the forceful closes of the Bun socket API. From -// the handshake callback they turn the server down, like the native reject of -// an unauthorized server does: the held final flight, which carries the client -// certificate, must not go out. end() and shutdown() are graceful and still -// send it. A socket with no handshake callback gets its open callback at that -// point. A plain TCP relay counts what the client sent. +// The release of the held flight is node:net's. A socket of the Bun socket API +// keeps its behaviour: whatever it calls in its handshake callback, its final +// flight goes out and the server completes its handshake. A socket with no +// handshake callback gets its open callback at that point. describe("a TLS 1.3 Bun.connect client that closes once its handshake is done", () => { const keys = join(import.meta.dir, "..", "test", "fixtures", "keys"); const pem = (name: string) => readFileSync(join(keys, name), "utf8"); - type Close = "end" | "shutdown" | "terminate" | "close"; + type Close = "end" | "shutdown" | "close"; async function run(callback: "handshake" | "open", method: Close) { const serverSaw = Promise.withResolvers(); @@ -2206,19 +2204,12 @@ describe("a TLS 1.3 Bun.connect client that closes once its handshake is done", } it.each([ - ["handshake", "terminate"], ["handshake", "close"], - ["open", "terminate"], ["open", "close"], - ] as const)("%s: %s() sends nothing after the ClientHello", async (callback, method) => { - expect(await run(callback, method)).toEqual({ server: "fail:ECONNRESET", sentAfterClientHello: false }); - }); - - it.each([ ["handshake", "end"], ["handshake", "shutdown"], ["open", "end"], - ] as const)("%s: %s() still completes the server's handshake", async (callback, method) => { + ] as const)("%s: %s() completes the server's handshake", async (callback, method) => { expect(await run(callback, method)).toEqual({ server: "handshake:agent3", sentAfterClientHello: true }); }); }); @@ -2585,7 +2576,7 @@ async function reportsFromNode(fixture: string, rows: (readonly [version: string import { report } from ${JSON.stringify(pathToFileURL(join(import.meta.dir, fixture)).href)}; const rows = ${JSON.stringify(rows)}; const reports = await Promise.all(rows.map(([version, mode]) => report(mode, version))); - console.log(JSON.stringify({ node: process.versions.node, reports })); + console.log(JSON.stringify({ reports })); process.exit(0); `; await using proc = Bun.spawn({ @@ -2596,9 +2587,9 @@ async function reportsFromNode(fixture: string, rows: (readonly [version: string }); const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); expect(stderr).toBe(""); - const { node, reports } = JSON.parse(stdout) as { node: string; reports: unknown[] }; + const { reports } = JSON.parse(stdout) as { reports: unknown[] }; expect(exitCode).toBe(0); - return { node: node.split(".").map(Number), reports }; + return { reports }; } // A TLS 1.3 client, and a client that resumes a TLS 1.2 session, finishes its @@ -2630,6 +2621,7 @@ describe("how a TLS client's way of closing reaches the server", () => { ["TLSv1.3", "a junk record behind the server's Finished", turnedDown(altnameInvalid, "close:true")], ["TLSv1.3", "a close_notify behind the server's Finished", turnedDown(altnameInvalid, "close:true")], ["TLSv1.3", "tls.connect({ socket })", turnedDown(altnameInvalid, "close:true")], + ["TLSv1.3", "tls.connect({ socket }) and a destroy() of that socket", turnedDown("close:false")], ["TLSv1.3", "https.request", turnedDown(altnameInvalid)], ["TLSv1.3", "http2.connect", turnedDown(altnameInvalid)], ["TLSv1.3", "destroy()", turnedDown("close:false")], @@ -2649,40 +2641,35 @@ describe("how a TLS client's way of closing reaches the server", () => { ["TLSv1.3", "end(data)", delivered("hello", 1)], ["TLSv1.3", "destroySoon()", delivered("", 1)], + // A server that resumes a TLS 1.2 session can ask for a renegotiation right + // behind its Finished. The client reports the handshake and sends its own + // Finished before it answers with a new ClientHello. + [ + "TLSv1.2", + "a HelloRequest behind the server's Finished", + { + client: ["reused:true", "ChangeCipherSpec", "Handshake", "Handshake", "close:false"], + server: { event: "tlsClientError", code: "ECONNRESET" }, + sentAfterClientHello: true, + alerts: 0, + }, + ], + // In a full TLS 1.2 handshake the client's flight leaves before the server's Finished. ["TLSv1.2", "checkServerIdentity", delivered("", 0, [altnameInvalid, "close:true"])], ["TLSv1.2", "destroy()", delivered("", 0)], ] as const; - // Since node 26.10.0 an end() or a write() made in 'secureConnect' waits for - // the callback to return, and a destroy() in the same callback drops it: - // https://github.com/nodejs/node/pull/65105 - // Up to node 26.9 these four delivered the flight. - const sinceNode2610 = [ - ["TLSv1.3", "end() then destroy()", turnedDown("close:false")], - ["TLSv1.3", "write('') then destroy()", turnedDown("close:false")], - ["TLSv1.3", "end('') then destroy()", turnedDown("close:false")], - ["TLSv1.2", "end() then destroy()", delivered("", 0)], - ] as const; - - it.each([...rows, ...sinceNode2610])("%s %s", async (version, mode, expected) => { + it.each(rows)("%s %s", async (version, mode, expected) => { expect(await closeReport(mode, version)).toEqual(expected); }); - // node 26.10 drops this write too. Bun sends it with the flight, as node did up to 26.9. - it("TLSv1.3 write() then destroy() sends the data with the flight", async () => { - expect(await closeReport("write() then destroy()", "TLSv1.3")).toEqual(delivered("hello", 0)); - }); - it.skipIf(!nodeExe())("node gives the same reports", async () => { - const { node, reports } = await reportsFromNode( + const { reports } = await reportsFromNode( "tls-client-close-fixture.mjs", - [...rows, ...sinceNode2610].map(([version, mode]) => [version, mode]), + rows.map(([version, mode]) => [version, mode]), ); - expect(reports.slice(0, rows.length)).toEqual(rows.map(([, , expected]) => expected)); - if (node[0] > 26 || (node[0] === 26 && node[1] >= 10)) { - expect(reports.slice(rows.length)).toEqual(sinceNode2610.map(([, , expected]) => expected)); - } + expect(reports).toEqual(rows.map(([, , expected]) => expected)); }); }); diff --git a/test/js/node/tls/tls-client-close-fixture.mjs b/test/js/node/tls/tls-client-close-fixture.mjs index 68a809412af8..aa1be4b80d44 100644 --- a/test/js/node/tls/tls-client-close-fixture.mjs +++ b/test/js/node/tls/tls-client-close-fixture.mjs @@ -97,6 +97,57 @@ function flightLength(bytes, secret) { return 0; } +// A HelloRequest as the first record behind the Finished of a TLS 1.2 server +// that resumed a session with TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (RFC 5246, +// sections 5, 6.3 and 7.4.1.1, and RFC 5288). `hello` starts with the hello +// record of each side, which carries its random at offset 11. +function sealedHelloRequest(clientHello, serverHello, masterSecret) { + const seed = Buffer.concat([ + Buffer.from("key expansion"), + serverHello.subarray(11, 43), + clientHello.subarray(11, 43), + ]); + let a = seed; + let keyBlock = Buffer.alloc(0); + while (keyBlock.length < 40) { + a = createHmac("sha256", masterSecret).update(a).digest(); + keyBlock = Buffer.concat([keyBlock, createHmac("sha256", masterSecret).update(a).update(seed).digest()]); + } + // The Finished was record 0 under the server's key, so this is record 1. + const sequence = Buffer.from([0, 0, 0, 0, 0, 0, 0, 1]); + const helloRequest = Buffer.from([0, 0, 0, 0]); + const seal = createCipheriv( + "aes-128-gcm", + keyBlock.subarray(16, 32), + Buffer.concat([keyBlock.subarray(36, 40), sequence]), + { + authTagLength: 16, + }, + ); + seal.setAAD(Buffer.concat([sequence, Buffer.from([22, 3, 3, 0, helloRequest.length])])); + const body = Buffer.concat([sequence, seal.update(helloRequest), seal.final(), seal.getAuthTag()]); + return Buffer.concat([Buffer.from([22, 3, 3, 0, body.length]), body]); +} + +// The types of the whole records in `bytes`. +function recordTypes(bytes) { + const types = []; + for (let at = 0; at + 5 <= bytes.length && at + 5 + bytes.readUInt16BE(at + 3) <= bytes.length; ) { + types.push({ 20: "ChangeCipherSpec", 21: "Alert", 22: "Handshake", 23: "ApplicationData" }[bytes[at]]); + at += 5 + bytes.readUInt16BE(at + 3); + } + return types; +} + +// The length of the first flight of a TLS 1.2 server that resumes a session: +// ServerHello, ChangeCipherSpec, Finished. 0 for any other start. +function resumedFlightLength(bytes) { + if (recordTypes(bytes).slice(0, 3).join() !== "Handshake,ChangeCipherSpec,Handshake") return 0; + let at = 0; + for (let record = 0; record < 3; record++) at += 5 + bytes.readUInt16BE(at + 3); + return at; +} + function wire(bytes) { let alerts = 0; for (let at = 0; at + 5 <= bytes.length; at += 5 + bytes.readUInt16BE(at + 3)) { @@ -115,22 +166,6 @@ const inSecureConnect = { "destroy() from queueMicrotask": socket => queueMicrotask(() => socket.destroy()), "end()": socket => socket.end(), "end(data)": socket => socket.end("hello"), - "write() then destroy()": socket => { - socket.write("hello"); - socket.destroy(); - }, - "write('') then destroy()": socket => { - socket.write(""); - socket.destroy(); - }, - "end('') then destroy()": socket => { - socket.end(""); - socket.destroy(); - }, - "end() then destroy()": socket => { - socket.end(); - socket.destroy(); - }, "destroySoon()": socket => socket.destroySoon(), }; @@ -158,6 +193,8 @@ async function secondConnection() { }; } +const helloRequestMode = "a HelloRequest behind the server's Finished"; + export async function report(mode, version) { const second = mode === "checkServerIdentity function that writes to another TLS socket" ? await secondConnection() : null; @@ -171,6 +208,7 @@ export async function report(mode, version) { minVersion: version, maxVersion: version, ALPNProtocols: mode === "http2.connect" ? ["h2"] : undefined, + ciphers: mode === helloRequestMode ? "ECDHE-RSA-AES128-GCM-SHA256" : undefined, }); server.on("secureConnection", socket => { // BoringSSL sends its TLS 1.3 tickets with the first write of the server. @@ -198,15 +236,42 @@ export async function report(mode, version) { }[mode]; let fromClient = []; + // What the client sent behind the HelloRequest, once its new ClientHello is in. + const behindHelloRequest = Promise.withResolvers(); const relay = net.createServer(downstream => { fromClient = []; let flight = behindFinished ? Buffer.alloc(0) : null; + let resumedFlight = mode === helloRequestMode ? Buffer.alloc(0) : null; + let frozenAt = -1; const upstream = net.connect(server.address().port, "127.0.0.1"); downstream.on("data", chunk => { fromClient.push(chunk); - upstream.write(chunk); + if (frozenAt < 0) return void upstream.write(chunk); + // The new ClientHello is the one long record. It leaves last. + const sent = Buffer.concat(fromClient).subarray(frozenAt); + for (let at = 0; at + 5 <= sent.length; at += 5 + sent.readUInt16BE(at + 3)) { + if (sent.readUInt16BE(at + 3) > 100 && at + 5 + sent.readUInt16BE(at + 3) <= sent.length) { + behindHelloRequest.resolve(recordTypes(sent)); + } + } }); upstream.on("data", chunk => { + if (frozenAt >= 0) return; + if (resumedFlight) { + resumedFlight = Buffer.concat([resumedFlight, chunk]); + const length = resumedFlightLength(resumedFlight); + if (length) { + const sent = Buffer.concat(fromClient); + frozenAt = sent.length; + const helloRequest = sealedHelloRequest(sent, resumedFlight, secrets.CLIENT_RANDOM); + return void downstream.write(Buffer.concat([resumedFlight.subarray(0, length), helloRequest])); + } + // A full handshake starts with three handshake records. Only the + // first flight of a connection can be a resumed one. + if (recordTypes(resumedFlight).length < 3) return; + chunk = resumedFlight; + resumedFlight = null; + } if (flight) { flight = Buffer.concat([flight, chunk]); const length = flightLength(flight, secrets.SERVER_HANDSHAKE_TRAFFIC_SECRET); @@ -295,6 +360,22 @@ export async function report(mode, version) { closed = watch(tls.connect({ ...refused, socket: raw })); break; } + case helloRequestMode: { + // The relay keeps what the client sends from here on, so the server sees no more of it. + const socket = tls.connect(await resume(accepted), () => client.push(`reused:${socket.isSessionReused()}`)); + closed = watch(socket); + client.push(...(await behindHelloRequest.promise)); + socket.destroy(); + break; + } + case "tls.connect({ socket }) and a destroy() of that socket": { + // The raw socket closes its handle two loop turns after its destroy(). + const raw = net.connect(port, "127.0.0.1"); + raw.on("error", () => {}); + await once(raw, "connect"); + closed = watch(tls.connect({ ...accepted, socket: raw }, () => raw.destroy())); + break; + } case "https.request": { const request = https.request({ ...refused, agent: false }); closed = new Promise(resolve => From 4b9f6df93600b1c6928c653cd626081c4f96736e Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 25 Sep 2026 19:42:36 +0000 Subject: [PATCH 5/6] node:net: read the handle once in _destroy, and shorten the new comments --- src/js/node/net.ts | 14 +++++--------- src/runtime/socket/socket_body.rs | 5 ++--- src/uws_sys/socket.rs | 3 +-- src/uws_sys/us_socket_t.rs | 1 - 4 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/js/node/net.ts b/src/js/node/net.ts index bc30a6542a0e..ab2460c8de07 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -262,8 +262,7 @@ const addServerName = $newRustFunction("Listener.rs", "jsAddServerName", 3); const upgradeDuplexToTLS = $newRustFunction("runtime/socket/socket.rs", "jsUpgradeDuplexToTLS", 2); // tls.connect({ socket }) upgrade: hostname policy stays with this JS layer. const upgradeTLSDeferred = $newRustFunction("runtime/socket/socket.rs", "jsUpgradeTLSDeferred", 2); -// destroy() inside the handshake callback turns the peer down: the native layer -// drops the handshake flight that it holds for that callback. +// destroy() in the handshake callback: drops the handshake flight that the native layer holds. const releaseHeldFlight = $newRustFunction("runtime/socket/socket.rs", "jsReleaseHeldFlight", 1); const isNamedPipeSocket = $newRustFunction("runtime/socket/socket.rs", "jsIsNamedPipeSocket", 1); const getBufferedAmount = $newRustFunction("runtime/socket/socket.rs", "jsGetBufferedAmount", 1); @@ -2395,19 +2394,16 @@ Socket.prototype._destroy = function _destroy(err, callback) { $debug("close"); if (this._handle) { $debug("close handle"); - // node drops the pending output of a TLS socket that is destroyed in its - // handshake callback. Some branches below close the handle a loop turn - // later, after the native layer has sent the flight, so ask for it here. - // https://github.com/nodejs/node/blob/v26.10.0/src/crypto/crypto_tls.cc#L1409-L1433 - if (typeof this[bunTlsSymbol] === "function" || this._handle[kAdoptedTLSRaw]) { - releaseHeldFlight(this._handle); - } const isException = err ? true : false; // `bytesRead` and `kBytesWritten` should be accessible after `.destroy()` // this[kBytesRead] = this._handle.bytesRead; this[kBytesWritten] = this._handle.bytesWritten; const currentHandle = this._handle; + // Ahead of every branch: two of them close the handle a loop turn later, when the flight has left. + if (typeof this[bunTlsSymbol] === "function" || currentHandle[kAdoptedTLSRaw]) { + releaseHeldFlight(currentHandle); + } if (this.resetAndClosing) { this.resetAndClosing = false; // resetAndDestroy() must send an RST (not a graceful FIN) so the peer sees diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index 4225793f3d33..20c536b2a32a 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -4652,9 +4652,7 @@ pub(crate) fn js_upgrade_tls_deferred( Err(global.throw(format_args!("Expected a socket instance"))) } -/// node:net's `destroy()`: a socket that is destroyed inside its handshake -/// callback turns the peer down, so the flight held for that callback is -/// dropped. The raw half of an `upgradeTLS` pair shares the TLS socket. +/// node:net's `destroy()`: drops the handshake flight held for the handshake callback. #[bun_jsc::host_fn] pub(crate) fn js_release_held_flight( _global: &JSGlobalObject, @@ -4665,6 +4663,7 @@ pub(crate) fn js_release_held_flight( if let Some(this) = socket.as_class_ref::() { this.socket.get().release_held_flight(); } else if let Some(this) = socket.as_class_ref::() { + // The raw half of an `upgradeTLS` pair shares the socket of its TLS half. this.socket.get().release_held_flight(); } Ok(JSValue::UNDEFINED) diff --git a/src/uws_sys/socket.rs b/src/uws_sys/socket.rs index 2add82515541..006d444caeaa 100644 --- a/src/uws_sys/socket.rs +++ b/src/uws_sys/socket.rs @@ -582,8 +582,7 @@ impl NewSocketHandler { } } - /// Drop the handshake flight that usockets holds across the handshake - /// callback. An SSLWrapper-backed socket holds none. + /// Drop the handshake flight that usockets holds across the handshake callback. pub fn release_held_flight(&self) { if let InternalSocket::Connected(s) = self.socket { sock(s).release_held_flight(); diff --git a/src/uws_sys/us_socket_t.rs b/src/uws_sys/us_socket_t.rs index e00b98c85483..c838be68adc6 100644 --- a/src/uws_sys/us_socket_t.rs +++ b/src/uws_sys/us_socket_t.rs @@ -310,7 +310,6 @@ impl us_socket_t { } /// Drop the handshake flight that is held across the handshake callback. - /// No-op when the socket holds none. pub fn release_held_flight(&mut self) { c::us_socket_release_held_flight(self); } From 3156461f76d260b910ef4141fb2632cc94996be8 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 25 Sep 2026 20:02:45 +0000 Subject: [PATCH 6/6] test: pin the certificate verdict behind a junk record --- test/js/node/tls/node-tls-connect.test.ts | 8 ++++++++ test/js/node/tls/tls-client-close-fixture.mjs | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index 9d93101dbb93..8205fe9f5c9d 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -2664,6 +2664,14 @@ describe("how a TLS client's way of closing reaches the server", () => { expect(await closeReport(mode, version)).toEqual(expected); }); + // node gives this verdict too. It also reports the junk record as an error of the socket. + it("TLSv1.3 a junk record behind the Finished does not change the verdict on the certificate", async () => { + const mode = "a junk record behind the Finished of a server that the client does not verify"; + expect(await closeReport(mode, "TLSv1.3")).toEqual( + turnedDown("authorized:false", "UNABLE_TO_VERIFY_LEAF_SIGNATURE", "close:false"), + ); + }); + it.skipIf(!nodeExe())("node gives the same reports", async () => { const { reports } = await reportsFromNode( "tls-client-close-fixture.mjs", diff --git a/test/js/node/tls/tls-client-close-fixture.mjs b/test/js/node/tls/tls-client-close-fixture.mjs index aa1be4b80d44..505be7b9afb5 100644 --- a/test/js/node/tls/tls-client-close-fixture.mjs +++ b/test/js/node/tls/tls-client-close-fixture.mjs @@ -233,6 +233,7 @@ export async function report(mode, version) { const behindFinished = { "a junk record behind the server's Finished": () => junkRecord, "a close_notify behind the server's Finished": flight => sealedCloseNotify(flight, secrets.SERVER_TRAFFIC_SECRET_0), + "a junk record behind the Finished of a server that the client does not verify": () => junkRecord, }[mode]; let fromClient = []; @@ -360,6 +361,15 @@ export async function report(mode, version) { closed = watch(tls.connect({ ...refused, socket: raw })); break; } + case "a junk record behind the Finished of a server that the client does not verify": { + // The verdict on the certificate must not depend on the record that follows the Finished. + const socket = tls.connect({ ...accepted, ca: undefined, rejectUnauthorized: false }, () => { + client.push(`authorized:${socket.authorized}`, String(socket.authorizationError)); + socket.destroy(); + }); + closed = watch(socket); + break; + } case helloRequestMode: { // The relay keeps what the client sends from here on, so the server sees no more of it. const socket = tls.connect(await resume(accepted), () => client.push(`reused:${socket.isSessionReused()}`));