diff --git a/packages/bun-usockets/src/crypto/openssl.c b/packages/bun-usockets/src/crypto/openssl.c index 42a214d8cfbc..5652038e53a8 100644 --- a/packages/bun-usockets/src/crypto/openssl.c +++ b/packages/bun-usockets/src/crypto/openssl.c @@ -451,7 +451,7 @@ void *us_socket_session_sink(struct us_socket_t *s) { /* socket.c — raw TCP FIN that does NOT re-enter the SSL layer. */ extern void us_internal_socket_raw_shutdown(struct us_socket_t *s); -static void ssl_update_handshake(struct us_socket_t *s); +static void ssl_update_handshake(struct us_socket_t *s, int fin_ends_handshake); static int ssl_flush_write_batch(struct loop_ssl_data *loop_ssl_data, struct us_socket_t *s); static int us_ssl_inline_reject_tripped(struct us_socket_t *s); static inline int ssl_gone(struct us_socket_t *s); @@ -1101,9 +1101,11 @@ static int us_cert_verify_cb(X509_STORE_CTX *ctx, void *arg) { (void)arg; int ok = X509_verify_cert(ctx); SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); - if (!ssl || SSL_is_server(ssl) || X509_STORE_CTX_get_error(ctx) != X509_V_OK) return ok; + if (!ssl) return ok; void *wrapper = us_ssl_wrapper(ssl); struct us_socket_t *s = wrapper ? NULL : us_ssl_socket(ssl); + if (s) s->ssl_peer_chain_checked = 1; + if (SSL_is_server(ssl) || X509_STORE_CTX_get_error(ctx) != X509_V_OK) return ok; int verdict = wrapper ? us_ssl_wrapper_server_identity(wrapper, ssl) : s ? us_dispatch_server_identity(s, ssl) : US_IDENTITY_UNCHECKED; @@ -1700,6 +1702,7 @@ void us_internal_ssl_attach(struct us_socket_t *s, SSL_CTX *ctx, s->ssl_inline_reject = 0; s->ssl_verify_failed = 0; s->ssl_identity_checked = 0; + s->ssl_peer_chain_checked = 0; s->ssl_sni_pending = US_SNI_NONE; s->ssl_sni_resolver = 0; s->ssl_has_pending_events = 0; @@ -1805,13 +1808,27 @@ struct us_bun_verify_error_t us_ssl_socket_verify_error_from_ssl(SSL *ssl) { return (struct us_bun_verify_error_t){.error = x509_verify_error, .code = code, .reason = reason}; } +/* A sent FIN, a sent close_notify or a fatal error says nothing about the + * peer's certificate: on an open socket the SSL alone answers, like node's + * TLSWrap::VerifyError + * (https://github.com/nodejs/node/blob/v26.3.0/src/crypto/crypto_tls.cc#L1840-L1853). */ struct us_bun_verify_error_t us_internal_ssl_verify_error(struct us_socket_t *s) { - if (!s->ssl || !s_ssl(s) || us_socket_is_closed(s) || us_internal_ssl_is_shut_down(s)) { + if (!s->ssl || !s_ssl(s) || us_socket_is_closed(s)) { return (struct us_bun_verify_error_t){.error = 0, .code = NULL, .reason = NULL}; } return us_ssl_socket_verify_error_from_ssl(s_ssl(s)); } +/* After our own FIN a failed handshake reports the SSL's verdict only for a + * chain that it checked: node:tls reads a failure with an X509 code as an + * established session. */ +static struct us_bun_verify_error_t ssl_failed_handshake_verify_error(struct us_socket_t *s) { + if (us_internal_ssl_is_shut_down(s) && !s->ssl_peer_chain_checked) { + return (struct us_bun_verify_error_t){.error = 0, .code = NULL, .reason = NULL}; + } + return us_internal_ssl_verify_error(s); +} + /* ── Handshake state machine ─────────────────────────────────────────────── */ /* Park the fatal OpenSSL reason behind a failed SSL_* call where the @@ -1903,7 +1920,7 @@ static void ssl_trigger_handshake(struct us_socket_t *s, int success) { /* A finished handshake reports the SSL's X509 verdict in every socket state. */ struct us_bun_verify_error_t verify_error = success && s->ssl ? us_ssl_socket_verify_error_from_ssl(s_ssl(s)) - : us_internal_ssl_verify_error(s); + : ssl_failed_handshake_verify_error(s); us_dispatch_handshake(s, success, verify_error); } @@ -2057,7 +2074,7 @@ struct us_socket_t *us_internal_ssl_close(struct us_socket_t *s, int code, void return us_internal_socket_close_raw(s, code, reason); } ssl_set_loop_data(s); - ssl_update_handshake(s); + ssl_update_handshake(s, 1); if (ssl_gone(s)) return s; if (s->ssl_handshake_state != HANDSHAKE_COMPLETED) { @@ -2094,7 +2111,11 @@ struct us_socket_t *us_internal_ssl_close(struct us_socket_t *s, int code, void } #define ssl_close us_internal_ssl_close -static void ssl_update_handshake(struct us_socket_t *s) { +/* `fin_ends_handshake` is 0 only from the writable event, which the read path + * re-enters while a handshake is in progress: the socket keeps reading after + * our FIN or close_notify, and the peer's next flight can still complete that + * handshake. For every other caller a half-closed socket's handshake is over. */ +static void ssl_update_handshake(struct us_socket_t *s, int fin_ends_handshake) { /* The OpenSSL error queue is per-thread and another socket's failure (a * server and a client commonly share this thread) may have left entries on * it; clear it before this socket's handshake step so any reason captured @@ -2113,8 +2134,9 @@ static void ssl_update_handshake(struct us_socket_t *s) { return; } - if (us_socket_is_closed(s) || us_internal_ssl_is_shut_down(s) || - (s_ssl(s) && SSL_get_shutdown(s_ssl(s)) & SSL_RECEIVED_SHUTDOWN)) { + if (us_socket_is_closed(s) || s->ssl_fatal_error || + (SSL_get_shutdown(s_ssl(s)) & SSL_RECEIVED_SHUTDOWN) || + (fin_ends_handshake && us_internal_ssl_is_shut_down(s))) { ssl_trigger_handshake(s, 0); return; } @@ -2188,7 +2210,7 @@ struct us_socket_t *us_internal_ssl_on_open(struct us_socket_t *s, int is_client if (!result || ssl_gone(result)) return result; /* Kick the handshake immediately — some peers stall waiting for ClientHello. */ ssl_set_loop_data(result); - ssl_update_handshake(result); + ssl_update_handshake(result, 1); return result; } @@ -2331,7 +2353,7 @@ struct us_socket_t *us_internal_ssl_on_writable(struct us_socket_t *s) { return us_internal_ssl_close(s, s->ssl_pending_close_code, NULL); } } - ssl_update_handshake(s); + ssl_update_handshake(s, 0); if (ssl_gone(s)) return s; if (s->ssl_read_wants_write) { @@ -2859,7 +2881,7 @@ void us_socket_sni_resolve(struct us_socket_t *s, struct ssl_ctx_st *ctx, int er loop_ssl_data->ssl_sni_resolved_ctx = ctx; /* may be NULL = default ctx */ } /* Re-drive the handshake; select_cert_cb re-fires and consumes the state. */ - ssl_update_handshake(s); + ssl_update_handshake(s, 1); if (loop_ssl_data->ssl_sni_resolved_ctx) SSL_CTX_free(loop_ssl_data->ssl_sni_resolved_ctx); loop_ssl_data->ssl_sni_resolved_ctx = outer_ctx; } @@ -2912,7 +2934,7 @@ struct us_socket_t *us_socket_adopt_tls(struct us_socket_t *s, void us_socket_start_tls_handshake(struct us_socket_t *s) { if (!s->ssl || us_socket_is_closed(s)) return; ssl_set_loop_data(s); - ssl_update_handshake(s); + ssl_update_handshake(s, 1); } /* ── SNI on listen sockets ───────────────────────────────────────────────── */ diff --git a/packages/bun-usockets/src/internal/internal.h b/packages/bun-usockets/src/internal/internal.h index 65120a0251f8..a2569774ceaa 100644 --- a/packages/bun-usockets/src/internal/internal.h +++ b/packages/bun-usockets/src/internal/internal.h @@ -338,6 +338,8 @@ struct us_socket_t { unsigned char ssl_verify_failed : 1; /* The owner checked the server's name inside this handshake. */ unsigned char ssl_identity_checked : 1; + /* The peer sent a certificate chain in this handshake, and it was checked. */ + unsigned char ssl_peer_chain_checked : 1; /* US_SNI_*: an async SNICallback has the handshake suspended. */ unsigned char ssl_sni_pending : 2; /* Server-side socket adopted into TLS with its own SNICallback. */ diff --git a/src/runtime/socket/socket_body.rs b/src/runtime/socket/socket_body.rs index b8e2221930fc..f0979ed9a6e8 100644 --- a/src/runtime/socket/socket_body.rs +++ b/src/runtime/socket/socket_body.rs @@ -1833,6 +1833,11 @@ impl NewSocket { // node:tls sockets defer the hostname verdict: their JS layer applies // `checkServerIdentity` (default or user override) itself. let flags = this.flags.get(); + // node:tls closes its own sockets, and nothing marks the ones its servers accept. + let failed_native_client = SSL + && success == 0 + && !flags.contains(Flags::DEFERS_SERVER_IDENTITY) + && !this.acts_as_tls_server(); // Deliberately independent of `success`: the inline-reject path // dispatches with success=0 after suppressing the client Finished, and // REJECTED must still be set there or the write-refusal guards are @@ -1841,6 +1846,7 @@ impl NewSocket { // failure flavor). let reject_unauthorized = flags.contains(Flags::REJECT_UNAUTHORIZED) && (verify_failed + || failed_native_client || (hostname_mismatch && !flags.contains(Flags::DEFERS_SERVER_IDENTITY))); // A handshake that failed outright (success == 0 with no policy // verdict: protocol error, peer alert, EOF mid-handshake) never has a @@ -2403,7 +2409,8 @@ impl NewSocket { ) -> JsResult { jsc::mark_binding!(); - if this.socket.get().is_detached() { + let socket = this.socket.get(); + if socket.is_detached() { // The verdict must survive the forced close. return Ok(this .stored_verify_error_to_js(global) @@ -2412,7 +2419,7 @@ impl NewSocket { // this error can change if called in different stages of hanshake // is very usefull to have this feature depending on the user workflow - let ssl_error = this.socket.get().get_verify_error(); + let ssl_error = socket.get_verify_error(); // `on_handshake` stores the name verdict, with its full message, for the in-handshake check too. if ssl_error.error_no == 0 || ssl_error.error_no == uws::us_bun_verify_error_t::HOSTNAME_MISMATCH @@ -2423,6 +2430,11 @@ impl NewSocket { if ssl_error.error_no == 0 { return Ok(JSValue::NULL); } + } else if this.flags.get().contains(Flags::HANDSHAKE_COMPLETE) && socket.is_shutdown() { + // What `on_handshake` reported stands. + return Ok(this + .stored_verify_error_to_js(global) + .unwrap_or(JSValue::NULL)); } let code: &[u8] = ssl_error.code_bytes(); diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index 1ae11107fcce..44a5482f23a6 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -15,8 +15,10 @@ import { tempDir, tls, } from "harness"; +import { once } from "node:events"; import net from "node:net"; import { join } from "node:path"; +import { Duplex } from "node:stream"; import { createSecureContext, connect as tlsConnect } from "node:tls"; describe.concurrent("socket", () => { it("should throw when a socket from a file descriptor has a bad file descriptor", async () => { @@ -3748,6 +3750,325 @@ Reo= expect(t.serverReceived.join("")).toBe("client-app-data\n"); }); }); + + // A sent FIN says nothing about the peer's certificate, and the socket keeps + // reading, so the handshake still completes and still has to check it. The + // tests hold the peer's flight until the FIN arrived, so the order does not + // depend on timing. Not concurrent: a flight that a half-closed socket can no + // longer send holds the loop's one spill slot until that socket closes. + describe("shutdown() while the handshake runs", () => { + // A connection that the peer answers: what was written before it has been + // read by the time it resolves. + async function pendingReadsDone() { + const server = net.createServer(socket => socket.end("x")); + await once(server.listen(0, "127.0.0.1"), "listening"); + const socket = net.connect((server.address() as net.AddressInfo).port, "127.0.0.1"); + await once(socket, "data"); + socket.destroy(); + server.close(); + } + + for (const trusted of [false, true]) { + it(`a client reads the check of ${trusted ? "a trusted" : "an untrusted"} server certificate`, async () => { + const handshake = Promise.withResolvers<{ + successArg: boolean; + authorizedGetter: boolean; + callbackCode: string | null; + getterCode: string | null; + }>(); + const closed = Promise.withResolvers(); + using server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + tls: trusted ? { key: SERVER_KEY, cert: SERVER_CRT } : { key: ROGUE_KEY, cert: ROGUE_CRT }, + socket: { open() {}, handshake() {}, data() {}, close() {}, error() {} }, + }); + // Holds the server's flight until the client's FIN arrived. + const relayed: net.Socket[] = []; + const relay = net.createServer({ allowHalfOpen: true }, downstream => { + const upstream = net.connect({ port: server.port, host: "127.0.0.1", allowHalfOpen: true }); + relayed.push(downstream, upstream); + const held: Buffer[] = []; + let sawClientFin = false; + downstream.on("data", data => upstream.write(data)); + downstream.on("end", () => { + sawClientFin = true; + for (const data of held.splice(0)) downstream.write(data); + }); + upstream.on("data", data => (sawClientFin ? downstream.write(data) : held.push(data))); + downstream.on("error", () => {}); + upstream.on("error", () => {}); + downstream.on("close", () => upstream.destroy()); + upstream.on("close", () => downstream.destroy()); + }); + await once(relay.listen(0, "127.0.0.1"), "listening"); + try { + using _client = await Bun.connect({ + hostname: "127.0.0.1", + port: (relay.address() as net.AddressInfo).port, + tls: { ca: CA_CRT, serverName: "localhost", rejectUnauthorized: false }, + socket: { + // The ClientHello leaves when `open` returns. + open(socket) { + setImmediate(() => socket.shutdown()); + }, + handshake(socket, success, authorizationError) { + handshake.resolve({ + successArg: success, + authorizedGetter: socket.authorized, + callbackCode: (authorizationError as NodeJS.ErrnoException | null)?.code ?? null, + getterCode: (socket.getAuthorizationError() as NodeJS.ErrnoException | null)?.code ?? null, + }); + }, + data() {}, + close() { + closed.resolve(); + }, + error(_socket, err) { + handshake.reject(err); + }, + connectError(_socket, err) { + handshake.reject(err); + closed.reject(err); + }, + }, + }); + expect(await handshake.promise).toEqual( + trusted + ? { successArg: true, authorizedGetter: true, callbackCode: null, getterCode: null } + : { + successArg: true, + authorizedGetter: false, + callbackCode: "UNABLE_TO_VERIFY_LEAF_SIGNATURE", + getterCode: "UNABLE_TO_VERIFY_LEAF_SIGNATURE", + }, + ); + } finally { + for (const socket of relayed) socket.destroy(); + relay.close(); + } + await closed.promise; + }); + + for (const pieces of [1, 2]) { + const certificate = trusted ? "a trusted" : "an untrusted"; + it(`a rejecting server reads the check of ${certificate} client certificate that arrives in ${pieces} read(s)`, async () => { + const handshake = Promise.withResolvers<{ + successArg: boolean; + authorizedGetter: boolean; + error: string | null; + }>(); + const serverClosed = Promise.withResolvers(); + const serverData = Promise.withResolvers(); + const serverReceived: string[] = []; + let serverSocket: Socket | undefined; + using server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + tls: { key: SERVER_KEY, cert: SERVER_CRT, ca: CA_CRT, requestCert: true }, + socket: { + open(socket) { + serverSocket = socket; + }, + handshake(socket, success, authorizationError) { + handshake.resolve({ + successArg: success, + authorizedGetter: socket.authorized, + error: authorizationError?.message ?? null, + }); + }, + data(_socket, data) { + serverReceived.push(data.toString()); + serverData.resolve(data.toString()); + }, + close() { + serverClosed.resolve(); + }, + error() {}, + }, + }); + + // The client sends its ClientHello, then holds its certificate until + // the server's FIN arrived. The server half-closes as soon as its own + // flight is out, like a server that ends its idle connections. + const raw = net.connect({ port: server.port, host: "127.0.0.1", allowHalfOpen: true }); + raw.on("error", handshake.reject); + const held: Buffer[] = []; + let sawServerFin = false; + let sentClientHello = false; + const transport = new Duplex({ + read() {}, + write(chunk, _encoding, callback) { + if (!sentClientHello || sawServerFin) raw.write(chunk); + else held.push(chunk); + sentClientHello = true; + callback(); + }, + }); + raw.once("data", () => serverSocket!.shutdown()); + raw.on("data", data => transport.push(data)); + raw.on("end", async () => { + const flight = Buffer.concat(held.splice(0)); + const size = Math.ceil(flight.length / pieces); + for (let offset = 0; offset < flight.length; offset += size) { + if (offset > 0) await pendingReadsDone(); + raw.write(flight.subarray(offset, offset + size)); + } + sawServerFin = true; + for (const chunk of held.splice(0)) raw.write(chunk); + }); + const client = tlsConnect({ + socket: transport, + servername: "localhost", + rejectUnauthorized: false, + ...(trusted ? { key: SERVER_KEY, cert: SERVER_CRT } : { key: ROGUE_KEY, cert: ROGUE_CRT }), + minVersion: "TLSv1.3", + }); + client.on("error", () => {}); + client.on("secureConnect", () => client.write("client-app-data\n")); + + try { + if (trusted) { + expect(await handshake.promise).toEqual({ successArg: true, authorizedGetter: true, error: null }); + expect(await serverData.promise).toBe("client-app-data\n"); + } else { + expect(await handshake.promise).toEqual({ + successArg: true, + authorizedGetter: false, + error: UNTRUSTED_MESSAGE, + }); + await serverClosed.promise; + expect(serverReceived).toEqual([]); + } + } finally { + client.destroy(); + raw.destroy(); + } + }); + } + } + + it("a rejecting client closes when its handshake fails with no error", async () => { + const events: string[] = []; + const closed = Promise.withResolvers(); + // Never answers, and keeps the connection open after the client's FIN. + const peer = net.createServer({ allowHalfOpen: true }, socket => socket.on("error", () => {})); + await once(peer.listen(0, "127.0.0.1"), "listening"); + try { + using _client = await Bun.connect({ + hostname: "127.0.0.1", + port: (peer.address() as net.AddressInfo).port, + tls: true, + socket: { + // Before the ClientHello, so the handshake never starts. + open(socket) { + socket.shutdown(); + }, + handshake(_socket, success, authorizationError) { + events.push(`handshake success=${success} error=${authorizationError?.message ?? null}`); + }, + data() {}, + close() { + events.push("close"); + closed.resolve(); + }, + error() {}, + connectError(_socket, err) { + closed.reject(err); + }, + }, + }); + await closed.promise; + expect(events).toEqual(["handshake success=false error=null", "close"]); + } finally { + peer.close(); + } + }); + + // The SSL has no certificate to judge when the peer never sent one. That + // must not replace the reason the handshake reported. + it("getAuthorizationError() keeps the reason a handshake failed", async () => { + const handshake = Promise.withResolvers<{ + successArg: boolean; + callbackCode: string | null; + getterCode: string | null; + }>(); + // Answers the ClientHello with bytes that are not TLS. + const plain = net.createServer(socket => { + socket.on("error", () => {}); + socket.once("data", () => socket.write("HTTP/1.1 400 Bad Request\r\n\r\n")); + }); + await once(plain.listen(0, "127.0.0.1"), "listening"); + try { + using _client = await Bun.connect({ + hostname: "127.0.0.1", + port: (plain.address() as net.AddressInfo).port, + tls: { rejectUnauthorized: false }, + socket: { + open() {}, + handshake(socket, success, authorizationError) { + handshake.resolve({ + successArg: success, + callbackCode: (authorizationError as NodeJS.ErrnoException | null)?.code ?? null, + getterCode: (socket.getAuthorizationError() as NodeJS.ErrnoException | null)?.code ?? null, + }); + }, + data() {}, + close() {}, + error() {}, + connectError(_socket, err) { + handshake.reject(err); + }, + }, + }); + expect(await handshake.promise).toEqual({ successArg: false, callbackCode: "EPROTO", getterCode: "EPROTO" }); + } finally { + plain.close(); + } + }); + + it("getAuthorizationError() agrees with a handshake that failed with no error", async () => { + const handshake = Promise.withResolvers<{ + successArg: boolean; + callbackCode: string | null; + getterCode: string | null; + }>(); + // Never answers, and keeps the connection open after the client's FIN. + const peer = net.createServer({ allowHalfOpen: true }, socket => socket.on("error", () => {})); + await once(peer.listen(0, "127.0.0.1"), "listening"); + try { + using client = await Bun.connect({ + hostname: "127.0.0.1", + port: (peer.address() as net.AddressInfo).port, + tls: { rejectUnauthorized: false }, + socket: { + // Before the ClientHello, so the handshake never starts. + open(socket) { + socket.shutdown(); + }, + handshake(socket, success, authorizationError) { + handshake.resolve({ + successArg: success, + callbackCode: (authorizationError as NodeJS.ErrnoException | null)?.code ?? null, + getterCode: (socket.getAuthorizationError() as NodeJS.ErrnoException | null)?.code ?? null, + }); + }, + data() {}, + close() {}, + error() {}, + connectError(_socket, err) { + handshake.reject(err); + }, + }, + }); + expect(await handshake.promise).toEqual({ successArg: false, callbackCode: null, getterCode: null }); + // This client does not reject, and its peer did not close: the socket is still open. + expect(client.getAuthorizationError()).toBeNull(); + } finally { + peer.close(); + } + }); + }); }); // Linux-only: uses /proc/self/fd to find and close the connected socket's fd diff --git a/test/js/node/tls/node-tls-connect.test.ts b/test/js/node/tls/node-tls-connect.test.ts index 12535208ed16..64cf3abbcf50 100644 --- a/test/js/node/tls/node-tls-connect.test.ts +++ b/test/js/node/tls/node-tls-connect.test.ts @@ -2764,6 +2764,17 @@ describe.each([ }); }); + // A handshake that our own FIN ended is no reason to close a client that rejects unauthorized peers. + it.skipIf(!exe)("end() leaves a client that rejects unauthorized peers open for the peer's close", async () => { + expect(await run("end-strict")).toEqual({ + log: ["connect secureConnecting=true", "finish"], + peerSawFin: true, + writableFinished: true, + readyState: "readOnly", + destroyed: false, + }); + }); + it.skipIf(!exe)("destroySoon() closes the socket", async () => { expect(await run("destroySoon")).toEqual({ log: ["connect secureConnecting=true", "finish", "close"], diff --git a/test/js/node/tls/node-tls-duplex-end-verify.test.ts b/test/js/node/tls/node-tls-duplex-end-verify.test.ts index 1f72ae79008f..35671bf4d96c 100644 --- a/test/js/node/tls/node-tls-duplex-end-verify.test.ts +++ b/test/js/node/tls/node-tls-duplex-end-verify.test.ts @@ -3,7 +3,7 @@ import assert from "node:assert"; import fs from "node:fs"; import net from "node:net"; -import { Duplex } from "node:stream"; +import { Duplex, duplexPair } from "node:stream"; import { test } from "node:test"; import tls from "node:tls"; @@ -12,13 +12,37 @@ const cert = fs.readFileSync(new URL("./fixtures/agent1-cert.pem", import.meta.u // agent2 is signed by a CA that neither side trusts here, so it is the client certificate the server must refuse. const clientKey = fs.readFileSync(new URL("./fixtures/agent2-key.pem", import.meta.url)); const clientCert = fs.readFileSync(new URL("./fixtures/agent2-cert.pem", import.meta.url)); +// The controls trust the peer: ca1 signed the server's certificate, and the client's certificate is its own issuer. +const serverCA = fs.readFileSync(new URL("./fixtures/ca1-cert.pem", import.meta.url)); +const clientCA = clientCert; // Both runtimes refuse an untrusted client. Bun reports the certificate check to 'tlsClientError', Node reports how // the connection ended. const isBun = process.versions.bun !== undefined; +// Resolves when this process has read what its sockets had received by the time of the call: a new connection that +// the peer answers takes more turns of the event loop than a read that is already due. +async function pendingReadsDone() { + const server = net.createServer(socket => socket.end("x")); + await new Promise(listening => server.listen(0, "127.0.0.1", listening)); + const socket = net.connect(server.address().port, "127.0.0.1"); + await new Promise(answered => socket.once("data", answered)); + socket.destroy(); + server.close(); +} + +// Passes `flight` to `deliver` in `pieces` parts. The receiver has read each part before the next one leaves, so with +// two pieces it reads the flight in two parts, split inside a record. +async function inPieces(deliver, flight, pieces) { + const size = Math.ceil(flight.length / pieces); + for (let offset = 0; offset < flight.length; offset += size) { + if (offset > 0) await pendingReadsDone(); + deliver(flight.subarray(offset, offset + size)); + } +} + // A client wraps a Duplex in TLS and calls end() right after its first flight left, so the handshake is still running. -// The server's certificate is not trusted. Returns the ordered events of the client. -async function endMidHandshake(rejectUnauthorized) { +// The server's certificate is not trusted, unless `trusted`. Returns the ordered events of the client. +async function endMidHandshake(rejectUnauthorized, { pieces = 1, trusted = false } = {}) { const events = []; const { promise, resolve } = Promise.withResolvers(); const server = tls.createServer({ key, cert }, socket => { @@ -44,10 +68,19 @@ async function endMidHandshake(rejectUnauthorized) { callback(); }, }); - raw.on("data", data => duplex.push(data)); - raw.on("end", () => duplex.push(null)); - raw.on("close", () => duplex.destroy()); - const client = tls.connect({ socket: duplex, servername: "agent1", rejectUnauthorized }); + let delivered = Promise.resolve(); + raw.on("data", data => { + if (pieces === 1) duplex.push(data); + else delivered = delivered.then(() => inPieces(part => duplex.push(part), data, pieces)); + }); + raw.on("end", () => delivered.then(() => duplex.push(null))); + raw.on("close", () => delivered.then(() => duplex.destroy())); + const client = tls.connect({ + socket: duplex, + servername: "agent1", + rejectUnauthorized, + ...(trusted && { ca: serverCA }), + }); client.on("secureConnect", () => { events.push(`secureConnect authorized=${client.authorized} authError=${client.authorizationError}`); // A connection that was let through must not keep this test waiting. @@ -81,6 +114,24 @@ test("end() while the handshake runs still reports the failed check on the socke ); }); +test("over a Duplex: the server's final flight arrives in two reads after end()", async () => { + const events = await endMidHandshake(false, { pieces: 2 }); + const secureConnect = events.find(event => event.startsWith("secureConnect")); + assert.strictEqual( + secureConnect, + "secureConnect authorized=false authError=UNABLE_TO_VERIFY_LEAF_SIGNATURE", + events.join(", "), + ); +}); + +for (const pieces of [1, 2]) { + test(`over a Duplex: end() while the handshake runs keeps a trusted certificate authorized, final flight in ${pieces} read(s)`, async () => { + const events = await endMidHandshake(true, { pieces, trusted: true }); + const secureConnect = events.find(event => event.startsWith("secureConnect")); + assert.strictEqual(secureConnect, "secureConnect authorized=true authError=null", events.join(", ")); + }); +} + // The tests below put a TCP proxy between the two peers. The proxy acts on TLS records, not on TCP chunks, so the // moment of each step does not depend on how the kernel splits the byte stream. const CHANGE_CIPHER_SPEC = 0x14; @@ -99,12 +150,21 @@ function eachRecord(socket, onRecord) { } // Starts `server` and a proxy in front of it. `wire(downstream, upstream)` connects the two sockets of each proxied -// connection. Returns the port of the proxy and a function that closes everything. -async function behindProxy(server, wire, proxyOptions = {}) { +// connection. With `overDuplex` the server does not get a TCP socket: its transport is one end of a Duplex pair, and +// `upstream` is the other end. Returns the port of the proxy and a function that closes everything. +async function behindProxy(server, wire, proxyOptions = {}, overDuplex = false) { await new Promise(listening => server.listen(0, "127.0.0.1", listening)); const proxied = []; const proxy = net.createServer(proxyOptions, downstream => { - const upstream = net.connect({ port: server.address().port, host: "127.0.0.1", ...proxyOptions }); + let upstream; + if (overDuplex) { + const [transport, otherEnd] = duplexPair(); + transport.on("error", () => {}); + upstream = otherEnd; + server.emit("connection", transport); + } else { + upstream = net.connect({ port: server.address().port, host: "127.0.0.1", ...proxyOptions }); + } proxied.push(downstream, upstream); upstream.on("error", () => {}); downstream.on("error", () => {}); @@ -125,9 +185,9 @@ async function behindProxy(server, wire, proxyOptions = {}) { // The same shape on a plain TCP socket. The client calls end() when its last handshake flight left. The proxy holds // the server's final flight until the client's FIN arrived, so the handshake completes on a socket that is already -// shut down. The proxy never forwards the FIN, so the server keeps writing. The server's certificate is not trusted. -// Returns the ordered events of the client. -async function endMidHandshakeOverTcp(maxVersion, rejectUnauthorized) { +// shut down. The proxy never forwards the FIN, so the server keeps writing. The server's certificate is not trusted, +// unless `trusted`. Returns the ordered events of the client. +async function endMidHandshakeOverTcp(maxVersion, rejectUnauthorized, { pieces = 1, trusted = false } = {}) { const events = []; const { promise, resolve } = Promise.withResolvers(); const server = tls.createServer({ key, cert, maxVersion }, socket => { @@ -155,10 +215,10 @@ async function endMidHandshakeOverTcp(maxVersion, rejectUnauthorized) { } sawChangeCipherSpec ||= record[0] === CHANGE_CIPHER_SPEC; }); - downstream.on("end", () => { + downstream.on("end", async () => { + await inPieces(part => downstream.write(part), Buffer.concat(held.splice(0)), pieces); clientEnded = true; - // One write, so the client reads the flight in one piece. - if (held.length > 0) downstream.write(Buffer.concat(held.splice(0))); + for (const chunk of held.splice(0)) downstream.write(chunk); }); upstream.on("data", chunk => { if (ending && !clientEnded) held.push(chunk); @@ -168,7 +228,14 @@ async function endMidHandshakeOverTcp(maxVersion, rejectUnauthorized) { { allowHalfOpen: true }, ); - client = tls.connect({ port, host: "127.0.0.1", servername: "agent1", rejectUnauthorized, maxVersion }); + client = tls.connect({ + port, + host: "127.0.0.1", + servername: "agent1", + rejectUnauthorized, + maxVersion, + ...(trusted && { ca: serverCA }), + }); client.on("secureConnect", () => { events.push(`secureConnect authorized=${client.authorized} authError=${client.authorizationError}`); setImmediate(() => client.destroy()); @@ -202,12 +269,37 @@ for (const maxVersion of ["TLSv1.2", "TLSv1.3"]) { }); } +// Our own FIN does not end a handshake that is still running. A flight that arrives in two reads completes it as well. +for (const maxVersion of ["TLSv1.2", "TLSv1.3"]) { + test(`${maxVersion} on a TCP socket: the server's final flight arrives in two reads after end()`, async () => { + const events = await endMidHandshakeOverTcp(maxVersion, false, { pieces: 2 }); + const secureConnect = events.find(event => event.startsWith("secureConnect")); + assert.strictEqual( + secureConnect, + "secureConnect authorized=false authError=UNABLE_TO_VERIFY_LEAF_SIGNATURE", + events.join(", "), + ); + }); + + for (const pieces of [1, 2]) { + test(`${maxVersion} on a TCP socket: end() while the handshake runs keeps a trusted certificate authorized, final flight in ${pieces} read(s)`, async () => { + const events = await endMidHandshakeOverTcp(maxVersion, true, { pieces, trusted: true }); + const secureConnect = events.find(event => event.startsWith("secureConnect")); + assert.strictEqual(secureConnect, "secureConnect authorized=true authError=null", events.join(", ")); + }); + } +} + // The server side of the same shape. A server that asks for a client certificate calls end() on its socket while the // handshake runs. The proxy holds the client's Certificate..Finished flight until the server's FIN arrived, so the -// handshake completes on a socket that is already shut down. The client's certificate is not trusted. -// With `afterHandshakeTimeout` the end() comes from the server's 'tlsClientError' listener, once the handshake -// timeout was reported. Returns the ordered events of the server. -async function serverEndMidHandshake(rejectUnauthorized, afterHandshakeTimeout = false) { +// handshake completes on a socket that is already shut down. The client's certificate is not trusted, unless +// `trusted`. With `afterHandshakeTimeout` the end() comes from the server's 'tlsClientError' listener, once the +// handshake timeout was reported. Returns the ordered events of the server. +async function serverEndMidHandshake( + rejectUnauthorized, + afterHandshakeTimeout = false, + { pieces = 1, trusted = false, overDuplex = false } = {}, +) { const events = []; const { promise, resolve } = Promise.withResolvers(); let serverSocket; @@ -220,6 +312,7 @@ async function serverEndMidHandshake(rejectUnauthorized, afterHandshakeTimeout = minVersion: "TLSv1.3", maxVersion: "TLSv1.3", ...(afterHandshakeTimeout && { handshakeTimeout: 50 }), + ...(trusted && { ca: clientCA }), }, socket => { events.push(`secureConnection authorized=${socket.authorized} authError=${socket.authorizationError}`); @@ -258,13 +351,14 @@ async function serverEndMidHandshake(rejectUnauthorized, afterHandshakeTimeout = if (held.length === 1 && !afterHandshakeTimeout) serverSocket.end(); }); upstream.on("data", chunk => downstream.write(chunk)); - upstream.on("end", () => { + upstream.on("end", async () => { + await inPieces(part => upstream.write(part), Buffer.concat(held.splice(0)), pieces); serverEnded = true; - // One write, so the server reads the flight in one piece. - upstream.write(Buffer.concat(held.splice(0))); + for (const record of held.splice(0)) upstream.write(record); }); }, { allowHalfOpen: true }, + overDuplex, ); const client = tls.connect({ @@ -307,6 +401,87 @@ test("a server that end()s while the handshake runs does not accept an untrusted ]); }); +for (const overDuplex of [false, true]) { + const transport = overDuplex ? "over a Duplex" : "on a TCP socket"; + + test(`${transport}: a server that end()s reports the failed check of a flight that arrives in two reads`, async () => { + const events = await serverEndMidHandshake(false, false, { pieces: 2, overDuplex }); + assert.strictEqual( + events[0], + "secureConnection authorized=false authError=DEPTH_ZERO_SELF_SIGNED_CERT", + events.join(", "), + ); + }); + + test(`${transport}: a server that end()s does not accept an untrusted client certificate that arrives in two reads`, async () => { + assert.deepStrictEqual(await serverEndMidHandshake(true, false, { pieces: 2, overDuplex }), [ + isBun ? "tlsClientError DEPTH_ZERO_SELF_SIGNED_CERT" : "tlsClientError ECONNRESET", + ]); + }); + + for (const pieces of [1, 2]) { + test(`${transport}: a server that end()s keeps a trusted client certificate authorized, flight in ${pieces} read(s)`, async () => { + assert.deepStrictEqual(await serverEndMidHandshake(true, false, { pieces, trusted: true, overDuplex }), [ + "secureConnection authorized=true authError=null", + "data privileged-command authorized=true", + ]); + }); + } +} + +// A client can offer the session of an earlier connection. Its certificate check is not the check of a handshake that +// the peer never answered. +for (const maxVersion of ["TLSv1.2", "TLSv1.3"]) { + test(`${maxVersion}: end() before the handshake does not report the check of an offered session`, async () => { + const server = tls.createServer({ key, cert, maxVersion }, socket => { + socket.on("error", () => {}); + socket.end("x"); + }); + await new Promise(listening => server.listen(0, "127.0.0.1", listening)); + const first = tls.connect({ port: server.address().port, host: "127.0.0.1", rejectUnauthorized: false }); + first.on("error", () => {}); + let session; + first.on("session", ticket => (session = ticket)); + await new Promise(connected => first.once("secureConnect", connected)); + assert.strictEqual(first.authorizationError, "UNABLE_TO_VERIFY_LEAF_SIGNATURE"); + first.resume(); + await new Promise(closed => first.once("close", closed)); + // TLS 1.3 gives the session with the 'session' event only. + if (maxVersion === "TLSv1.2") session ??= first.getSession(); + assert.ok(session?.length > 0, "the first connection gave no session to offer"); + server.close(); + + // Accepts the connection and never answers. + const sawFin = Promise.withResolvers(); + const accepted = []; + const silent = net.createServer({ allowHalfOpen: true }, socket => { + accepted.push(socket); + socket.on("data", () => {}); + socket.on("error", () => {}); + socket.on("end", sawFin.resolve); + }); + await new Promise(listening => silent.listen(0, "127.0.0.1", listening)); + const events = []; + const client = tls.connect({ port: silent.address().port, host: "127.0.0.1", rejectUnauthorized: false, session }); + for (const event of ["secureConnect", "finish", "error", "close"]) { + client.on(event, arg => events.push(arg?.code ? `${event} ${arg.code}` : event)); + } + client.on("connect", () => client.end()); + try { + await Promise.all([new Promise(finished => client.once("finish", finished)), sawFin.promise]); + await pendingReadsDone(); + assert.deepStrictEqual( + { events, authorized: client.authorized, authorizationError: client.authorizationError ?? null }, + { events: ["finish"], authorized: false, authorizationError: null }, + ); + } finally { + client.destroy(); + for (const socket of accepted) socket.destroy(); + silent.close(); + } + }); +} + test("a server that end()s after a handshake timeout does not accept an untrusted client certificate", async () => { assert.deepStrictEqual(await serverEndMidHandshake(true, true), [ "tlsClientError ERR_TLS_HANDSHAKE_TIMEOUT", diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index a672381930ae..1c60fe289820 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -1247,6 +1247,74 @@ it("an asynchronous SNICallback error aborts the suspended handshake with tlsCli await once(server, "close"); }); +it("a socket that end()s while an asynchronous SNICallback is pending reports tlsClientError, then 'close'", async () => { + const events: string[] = []; + const sniCalled = Promise.withResolvers(); + const sawServerFin = Promise.withResolvers(); + const closed = Promise.withResolvers(); + let answerSni: (() => void) | undefined; + let serverSocket: net.Socket | undefined; + const server: Server = createServer({ + ...COMMON_CERT, + requestCert: true, + rejectUnauthorized: true, + SNICallback: (_name, cb) => { + answerSni = () => cb(null, tls.createSecureContext({ ...COMMON_CERT })); + sniCalled.resolve(); + }, + }); + server.on("secureConnection", () => events.push("secureConnection")); + server.on("tlsClientError", err => events.push(`tlsClientError ${(err as NodeJS.ErrnoException).code}`)); + server.on("connection", socket => { + serverSocket = socket; + socket.on("error", () => {}); + socket.on("close", hadError => { + events.push(`close hadError=${hadError}`); + closed.resolve(); + }); + }); + server.listen(0); + await once(server, "listening"); + // The proxy does not pass the server's FIN on, so the client stays connected. + const proxied: net.Socket[] = []; + const proxy = net.createServer({ allowHalfOpen: true }, downstream => { + const upstream = net.connect({ + port: (server.address() as AddressInfo).port, + host: "127.0.0.1", + allowHalfOpen: true, + }); + proxied.push(downstream, upstream); + downstream.on("data", chunk => upstream.write(chunk)); + upstream.on("data", chunk => downstream.write(chunk)); + upstream.on("end", () => sawServerFin.resolve()); + downstream.on("error", () => {}); + upstream.on("error", () => {}); + }); + proxy.listen(0, "127.0.0.1"); + await once(proxy, "listening"); + const client = connect({ + port: (proxy.address() as AddressInfo).port, + host: "127.0.0.1", + servername: "pending.example.com", + rejectUnauthorized: false, + }); + client.on("error", () => {}); + try { + await sniCalled.promise; + serverSocket!.end(); + await sawServerFin.promise; + // The handshake fails inside this call: the socket is shut down. + answerSni!(); + await closed.promise; + expect(events).toEqual(["tlsClientError ECONNRESET", "close hadError=true"]); + } finally { + client.destroy(); + for (const socket of proxied) socket.destroy(); + proxy.close(); + server.close(); + } +}); + it("destroying the connection while an asynchronous SNICallback is pending does not crash", async () => { let resolveLater: (() => void) | undefined; const server: Server = createServer({ diff --git a/test/js/node/tls/tls-shutdown-before-handshake-fixture.mjs b/test/js/node/tls/tls-shutdown-before-handshake-fixture.mjs index dc6a1b6deca8..377c96bbf737 100644 --- a/test/js/node/tls/tls-shutdown-before-handshake-fixture.mjs +++ b/test/js/node/tls/tls-shutdown-before-handshake-fixture.mjs @@ -41,9 +41,9 @@ async function stalledPeer() { }; } -if (mode === "end" || mode === "destroySoon") { +if (mode === "end" || mode === "end-strict" || mode === "destroySoon") { const peer = await stalledPeer(); - const client = tls.connect({ port: peer.port, host: "127.0.0.1", rejectUnauthorized: false }); + const client = tls.connect({ port: peer.port, host: "127.0.0.1", rejectUnauthorized: mode === "end-strict" }); for (const event of ["secureConnect", "finish", "error", "close"]) { client.on(event, arg => log.push(arg?.code ? `${event}:${arg.code}` : event)); } @@ -51,10 +51,10 @@ if (mode === "end" || mode === "destroySoon") { // and with this peer it never will. client.on("connect", () => { log.push(`connect secureConnecting=${client.secureConnecting}`); - client[mode](); + client[mode === "destroySoon" ? "destroySoon" : "end"](); }); - await Promise.all([once(client, mode === "end" ? "finish" : "close"), peer.sawFin]); + await Promise.all([once(client, mode === "destroySoon" ? "close" : "finish"), peer.sawFin]); const { writableFinished, readyState, destroyed } = client; client.destroy();