diff --git a/src/js/node/net.ts b/src/js/node/net.ts index 58e05eb224ee..ca11f036885a 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -142,7 +142,6 @@ const kSyncWriteFd = Symbol("kSyncWriteFd"); const kSetKeepAliveInitialDelay = Symbol("kSetKeepAliveInitialDelay"); const kConnectOptions = Symbol("connect-options"); const kAttach = Symbol("kAttach"); -const kCloseRawConnection = Symbol("kCloseRawConnection"); const kupgraded = Symbol("kupgraded"); const kAdoptedTLSRaw = Symbol("kAdoptedTLSRaw"); const ksocket = Symbol("ksocket"); @@ -249,6 +248,23 @@ function closeAdoptedTLSRawNowNT(handle, self, isException) { handle.close(onSocketHandleClosed); setImmediate(emitCloseNT, self, isException); } +// A TLS socket takes the transport it was layered over down with it, like +// node's TLSWrap does its parent wrap: on the tick after _destroy, so the TLS +// socket's 'error' still sees the transport intact and the transport's 'close' +// precedes the TLS socket's. The raw twin of an adopted fd is closed by the +// TLS handle, so it is detached rather than closed a second time. +function destroyUpgradedTransportNT(upgraded) { + if (upgraded.destroyed) return; + if (upgraded._handle?.[kAdoptedTLSRaw]) { + upgraded._handle = null; + } + upgraded.destroy(); +} +// Handle-less TLS socket: its 'close' is queued behind the transport's. +function destroyUpgradedTransportAndCloseNT(upgraded, self, hasError) { + destroyUpgradedTransportNT(upgraded); + process.nextTick(emitCloseNT, self, hasError); +} function detachSocket(self) { if (!self) self = this; self._handle = null; @@ -1863,14 +1879,6 @@ Socket.prototype[kAttach] = function (port, socket) { SocketHandlers.drain(socket); }; -Socket.prototype[kCloseRawConnection] = function () { - const connection = this[kupgraded]; - connection.connecting = false; - connection._handle = null; - connection.unref(); - connection.destroy(); -}; - Socket.prototype.connect = function connect(...args) { $debug("Socket.prototype.connect"); { @@ -1989,8 +1997,10 @@ Socket.prototype.connect = function connect(...args) { // if is named pipe socket we can upgrade it using the same wrapper than we use for duplex upgradeDuplex = isNamedPipeSocket(socket) || hasUnflushedWrites(connection); } + // Before the upgrade, which may wait for 'connect': from here on + // destroying this socket destroys the connection too. + this[kupgraded] = connection; if (upgradeDuplex) { - this[kupgraded] = connection; const [result, events] = upgradeDuplexToTLS(connection, { data: { self: this, req: { oncomplete: afterConnect } }, tls, @@ -2006,7 +2016,6 @@ Socket.prototype.connect = function connect(...args) { // connecting (e.g. tls.connect({ socket: net.connect(port) })) must be // upgraded once it emits 'connect'. if (socket && !connection.connecting) { - this[kupgraded] = connection; const result = upgradeTLSDeferred(socket, { data: { self: this, req: { oncomplete: afterConnect } }, tls, @@ -2018,7 +2027,6 @@ Socket.prototype.connect = function connect(...args) { // replace socket connection._handle = raw; raw[kAdoptedTLSRaw] = true; - this.once("end", this[kCloseRawConnection]); raw.connecting = false; this._handle = tls; } else { @@ -2028,20 +2036,14 @@ Socket.prototype.connect = function connect(...args) { } else { // wait to be connected connection.once("connect", () => { - // The TLS socket may have been destroyed before the underlying - // socket connected (e.g. tls.connect({ socket }).destroy()); don't - // start a handshake on a dead socket. - if (this.destroyed) { - connection.destroy(); - return; - } + // Destroyed while connecting: _destroy is taking the connection down. + if (this.destroyed) return; const socket = connection._handle; if (!upgradeDuplex && socket) { // if is named pipe socket we can upgrade it using the same wrapper than we use for duplex upgradeDuplex = isNamedPipeSocket(socket) || hasUnflushedWrites(connection); } if (upgradeDuplex) { - this[kupgraded] = connection; const [result, events] = upgradeDuplexToTLS(connection, { data: { self: this, req: { oncomplete: afterConnect } }, tls, @@ -2053,7 +2055,6 @@ Socket.prototype.connect = function connect(...args) { connection.on("close", events[3]); this._handle = result; } else { - this[kupgraded] = connection; const result = upgradeTLSDeferred(socket, { data: { self: this, req: { oncomplete: afterConnect } }, tls, @@ -2065,7 +2066,6 @@ Socket.prototype.connect = function connect(...args) { // replace socket connection._handle = raw; raw[kAdoptedTLSRaw] = true; - this.once("end", this[kCloseRawConnection]); raw.connecting = false; this._handle = tls; } else { @@ -2141,14 +2141,7 @@ Socket.prototype._destroy = function _destroy(err, callback) { $debug("Socket.prototype._destroy"); this.connecting = false; - // Tear down a wrapped generic duplex with this socket: the native handle's - // close only flushes close_notify and lets the wrapper drain; without an - // explicit destroy here a late RST on the underlying transport can surface - // as an unhandled error after this socket is gone. const upgraded = this[kupgraded]; - if (upgraded && !(upgraded instanceof Socket) && !upgraded.destroyed) { - upgraded.destroy?.(); - } // Close an fd adopted for synchronous writes (node closes the wrapping // libuv handle here). Leave stdio fds 0-2 open: process.stdout/stderr and @@ -2214,9 +2207,15 @@ Socket.prototype._destroy = function _destroy(err, callback) { this._sockname = null; } callback(err); + // Queued after the 'error' that callback(err) queues. + if (upgraded) process.nextTick(destroyUpgradedTransportNT, upgraded); } else { callback(err); - process.nextTick(emitCloseNT, this, err ? true : false); + if (upgraded) { + process.nextTick(destroyUpgradedTransportAndCloseNT, upgraded, this, err ? true : false); + } else { + process.nextTick(emitCloseNT, this, err ? true : false); + } } const server = this._server; @@ -2418,7 +2417,6 @@ Socket.prototype[Symbol.for("::bunUpgradeServerTLS::")] = function (connection, const [raw, tlsHandle] = result; connection._handle = raw; raw[kAdoptedTLSRaw] = true; - this.once("end", this[kCloseRawConnection]); raw.connecting = false; this._handle = tlsHandle; this.emit(kUpgradeAttached); diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index dc5623688e61..4e5e23d59170 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -1308,9 +1308,11 @@ it("SNICallback runs even when the requested servername matches the bind hostnam }); server.listen(0, "localhost"); await once(server, "listening"); - const port = (server.address() as AddressInfo).port; - // host: "localhost" defaults servername to "localhost" - the bind hostname. - const client = connect({ port, host: "localhost", rejectUnauthorized: false }); + const { port, address } = server.address() as AddressInfo; + // Connect to the literal bound address ("localhost" may resolve to a + // different family than the one the server picked) and request the bind + // hostname via SNI. + const client = connect({ port, host: address, servername: "localhost", rejectUnauthorized: false }); await once(client, "secureConnect"); expect(sniCalls).toBe(1); // The peer certificate must be the SNICallback's RSA cert, not COMMON_CERT. @@ -2019,6 +2021,152 @@ it("destroys a server wrap whose socket was destroyed before the deferred upgrad } }); +// The converse of the test above: destroying the wrap has to take the wrapped +// net.Socket down with it (node's TLSWrap close() destroys its parent wrap). +// Otherwise the accepted socket never emits 'close', stays in the net.Server's +// connection count and server.close(cb) never calls back. Releasing it from the +// wrap's 'end' only covered a graceful shutdown: a handshake failure destroys +// the wrap without ever emitting 'end'. Node's order of events, asserted below: +// the wrap's 'error' / the server's 'tlsClientError' first, with the wrapped +// socket still intact, then the wrapped socket's 'close', then the wrap's. +describe("destroying a server-side TLSSocket wrap destroys the wrapped net.Socket", () => { + type Accepted = { raw: net.Socket; events: string[]; settled: Promise }; + + // Logs both sockets' lifecycle events in order. `settled` resolves once the + // wrap's 'close' has been logged, plus the raw socket's if one is still due + // (a raw socket that was never destroyed, the bug, would never emit it, so + // that case settles immediately and fails on the log). Plain listeners rather + // than events.once(): the wrap emits 'error' before 'close' here. + function observe(raw: net.Socket, wrap: TLSSocket, events: string[] = []): Accepted { + const settled = Promise.withResolvers(); + raw.on("close", hadError => events.push(`raw close hadError=${hadError}`)); + wrap.on("error", (err: Error & { code?: string }) => + events.push(`wrap error ${err.code} raw.destroyed=${raw.destroyed}`), + ); + wrap.on("close", hadError => { + events.push(`wrap close hadError=${hadError} raw.destroyed=${raw.destroyed}`); + if (raw.destroyed && !events.some(event => event.startsWith("raw close"))) raw.once("close", settled.resolve); + else settled.resolve(); + }); + return { raw, events, settled: settled.promise }; + } + + async function listenRaw(onConnection: (raw: net.Socket) => Accepted | Promise) { + const accepted = Promise.withResolvers(); + const rawServer = net.createServer(raw => accepted.resolve(onConnection(raw))); + rawServer.on("error", accepted.reject); + rawServer.listen(0, "127.0.0.1"); + await once(rawServer, "listening"); + return { rawServer, port: (rawServer.address() as AddressInfo).port, accepted: accepted.promise }; + } + + async function expectReleased(rawServer: net.Server, { raw, events, settled }: Accepted, expected: string[]) { + await settled; + expect(events).toEqual(expected); + expect(raw.destroyed).toBe(true); + const connections = Promise.withResolvers(); + rawServer.getConnections((err, count) => (err ? connections.reject(err) : connections.resolve(count))); + expect(await connections.promise).toBe(0); + // With the connection released, close() completes. + const serverClosed = once(rawServer, "close"); + rawServer.close(); + await serverClosed; + } + + it("when the handshake fails", async () => { + const { rawServer, port, accepted } = await listenRaw(raw => + observe(raw, new TLSSocket(raw, { isServer: true, ...COMMON_CERT })), + ); + // A plain TCP peer whose first bytes are not a ClientHello. + const client = net.connect(port, "127.0.0.1"); + client.on("error", () => {}); + try { + const conn = await accepted; + client.write("this is not a TLS ClientHello\r\n"); + await expectReleased(rawServer, conn, [ + "wrap error ERR_SSL_WRONG_VERSION_NUMBER raw.destroyed=false", + "raw close hadError=false", + "wrap close hadError=true raw.destroyed=true", + ]); + } finally { + client.destroy(); + rawServer.close(); + } + }); + + it("when a tls.Server rejects the client certificate of a connection handed to it via emit('connection')", async () => { + // STARTTLS-style: a plain net.Server accepts the connection and hands it to + // a tls.Server, which wraps it. The rejected handshake has to release the + // connection from the net.Server that accepted it. + const tlsServer = createServer({ ...COMMON_CERT, requestCert: true, rejectUnauthorized: true }); + tlsServer.on("secureConnection", s => s.destroy(new Error("unexpected secureConnection"))); + const { rawServer, port, accepted } = await listenRaw(raw => { + const observed = Promise.withResolvers(); + // The wrap is only reachable through 'tlsClientError', which is emitted + // right before it is destroyed: observe it from inside the listener. + tlsServer.once("tlsClientError", (err: Error & { code?: string }, wrap) => { + observed.resolve(observe(raw, wrap, [`tlsClientError ${err.code} raw.destroyed=${raw.destroyed}`])); + }); + tlsServer.emit("connection", raw); + return observed.promise; + }); + // The client presents a self-signed certificate the server has no CA for. + const client = connect({ port, host: "127.0.0.1", ...COMMON_CERT, rejectUnauthorized: false }); + client.on("error", () => {}); + try { + await expectReleased(rawServer, await accepted, [ + "tlsClientError DEPTH_ZERO_SELF_SIGNED_CERT raw.destroyed=false", + "wrap error DEPTH_ZERO_SELF_SIGNED_CERT raw.destroyed=false", + "raw close hadError=false", + "wrap close hadError=true raw.destroyed=true", + ]); + } finally { + client.destroy(); + rawServer.close(); + tlsServer.close(); + } + }); + + it("when the wrap is destroyed in the tick it was created, before the socket was adopted", async () => { + const { rawServer, port, accepted } = await listenRaw(raw => { + const wrap = new TLSSocket(raw, { isServer: true, ...COMMON_CERT }); + const conn = observe(raw, wrap); + wrap.destroy(); + return conn; + }); + const client = net.connect(port, "127.0.0.1"); + client.on("error", () => {}); + const clientClosed = new Promise(resolve => client.once("close", () => resolve())); + try { + const conn = await accepted; + await conn.settled; + // The raw socket still owned the fd, so its 'close' follows its handle + // close, a later phase than the handle-less wrap's 'close': the order of + // the two is not part of the contract here, the set of events is. + conn.events.sort(); + await expectReleased(rawServer, conn, [ + "raw close hadError=false", + "wrap close hadError=false raw.destroyed=true", + ]); + // Destroying the raw socket is also what closes the connection the peer + // is holding. + await clientClosed; + } finally { + client.destroy(); + rawServer.close(); + } + }); + + it("when the wrap runs the TLS engine over a socket that has no handle", async () => { + const raw = new net.Socket(); + const wrap = new TLSSocket(raw, { isServer: true, ...COMMON_CERT }); + const { events, settled } = observe(raw, wrap); + wrap.destroy(); + await settled; + expect(events).toEqual(["raw close hadError=false", "wrap close hadError=false raw.destroyed=true"]); + }); +}); + it("exposes the server-side peer verification result via socket.ssl.verifyError()", async () => { // Node's server path consults the same TLSWrap.verifyError() that clients // use, so the shim must be populated for server sockets too: diff --git a/test/js/node/tls/node-tls-upgrade.test.ts b/test/js/node/tls/node-tls-upgrade.test.ts index b374abd6ea22..62c27f02882a 100644 --- a/test/js/node/tls/node-tls-upgrade.test.ts +++ b/test/js/node/tls/node-tls-upgrade.test.ts @@ -1,9 +1,136 @@ -import { expect, test } from "bun:test"; +import { describe, expect, test } from "bun:test"; import { once } from "events"; import { tls as certs } from "harness"; import net from "net"; import tls from "tls"; +// The net.Socket handed to tls.connect({ socket }) goes down with the TLS +// socket (node's TLSWrap close() destroys its parent wrap), so a STARTTLS +// caller keyed on the plain socket's 'close' sees it even when the upgrade +// fails. Releasing it from the TLS socket's 'end' only covered a graceful +// shutdown: a failed handshake destroys the TLS socket without emitting 'end'. +// Node's order of events, asserted below: the TLS socket's 'error' first, with +// the plain socket still intact (a caller can still read its remoteAddress +// there), then the plain socket's 'close', then the TLS socket's 'close'. +describe("destroying tls.connect({ socket }) destroys the wrapped net.Socket", () => { + // Logs both sockets' lifecycle events in order. Resolves once the TLS socket + // has closed, plus the plain socket's 'close' if one is still due (a plain + // socket that was never destroyed, the bug, would never emit it, so that case + // resolves right away and fails on the log). Plain listeners rather than + // events.once(): the TLS socket emits 'error' before 'close' here. + function observe(plain: net.Socket, secure: tls.TLSSocket) { + const events: string[] = []; + const { promise, resolve } = Promise.withResolvers(); + plain.on("error", (err: Error & { code?: string }) => events.push(`plain error ${err.code}`)); + plain.on("close", hadError => events.push(`plain close hadError=${hadError}`)); + secure.on("error", (err: Error & { code?: string }) => + events.push(`secure error ${err.code} plain.destroyed=${plain.destroyed}`), + ); + secure.on("close", hadError => { + events.push(`secure close hadError=${hadError} plain.destroyed=${plain.destroyed}`); + if (plain.destroyed && !events.some(event => event.startsWith("plain close"))) { + plain.once("close", () => resolve(events)); + } else { + resolve(events); + } + }); + return promise; + } + + async function failedUpgrade(server: net.Server, waitForConnect: boolean, options: tls.ConnectionOptions) { + const plain = net.connect({ host: "127.0.0.1", port: (server.address() as net.AddressInfo).port }); + if (waitForConnect) await once(plain, "connect"); + return observe(plain, tls.connect({ socket: plain, servername: "localhost", ...options })); + } + + describe.each([ + ["an already connected socket", true], + ["a socket that is still connecting", false], + ])("handshake against a non-TLS peer over %s", (_, waitForConnect) => { + test.concurrent("destroys the wrapped socket after the error", async () => { + await using server = net.createServer(peer => { + peer.on("error", () => {}); + peer.on("data", () => peer.write("this is not a TLS ServerHello\r\n")); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + expect(await failedUpgrade(server, waitForConnect, {})).toEqual([ + "secure error ERR_SSL_WRONG_VERSION_NUMBER plain.destroyed=false", + "plain close hadError=false", + "secure close hadError=true plain.destroyed=true", + ]); + }); + }); + + test.concurrent("certificate verification failure destroys the wrapped socket after the error", async () => { + // The server's certificate is self-signed and the client is given no `ca`. + await using server = tls.createServer(certs, socket => socket.on("error", () => {})); + server.on("tlsClientError", () => {}); + await once(server.listen(0, "127.0.0.1"), "listening"); + expect(await failedUpgrade(server, true, { rejectUnauthorized: true })).toEqual([ + "secure error DEPTH_ZERO_SELF_SIGNED_CERT plain.destroyed=false", + "plain close hadError=false", + "secure close hadError=true plain.destroyed=true", + ]); + }); + + test.concurrent("destroy() while the wrapped socket is still connecting destroys it too", async () => { + // Nothing is listening on the port any more, so the connect would fail: + // a wrapped socket left behind would surface that as an error of its own. + const server = net.createServer(); + await once(server.listen(0, "127.0.0.1"), "listening"); + const { port } = server.address() as net.AddressInfo; + await new Promise(resolve => server.close(() => resolve())); + + const plain = net.connect({ host: "127.0.0.1", port }); + const secure = tls.connect({ socket: plain, servername: "localhost" }); + const events = observe(plain, secure); + secure.destroy(); + // The plain socket still owns its (connecting) handle, so its 'close' + // follows the handle close, a later phase than the handle-less TLS + // socket's 'close': the set of events is the contract here, not the order. + expect((await events).sort()).toEqual([ + "plain close hadError=false", + "secure close hadError=false plain.destroyed=true", + ]); + }); + + test.concurrent("destroy() right after wrapping a connected socket destroys it and the connection", async () => { + // The socket was adopted synchronously, so the TLS socket owns the fd by + // now: destroying it has to release the plain socket and close the + // connection the peer is holding. + const peerClosed = Promise.withResolvers(); + await using server = net.createServer(peer => { + peer.on("error", () => {}); + peer.on("close", () => peerClosed.resolve()); + peer.resume(); + }); + await once(server.listen(0, "127.0.0.1"), "listening"); + + const plain = net.connect({ host: "127.0.0.1", port: (server.address() as net.AddressInfo).port }); + await once(plain, "connect"); + const secure = tls.connect({ socket: plain, servername: "localhost" }); + const events = observe(plain, secure); + secure.destroy(); + expect(await events).toEqual(["plain close hadError=false", "secure close hadError=false plain.destroyed=true"]); + await peerClosed.promise; + }); + + test.concurrent("a graceful shutdown after the handshake destroys the wrapped socket", async () => { + // No error involved: the TLS socket ends, the peer ends back and the TLS + // socket's own destroy takes the plain socket down. + await using server = tls.createServer(certs, socket => socket.on("error", () => {})); + await once(server.listen(0, "127.0.0.1"), "listening"); + + const plain = net.connect({ host: "127.0.0.1", port: (server.address() as net.AddressInfo).port }); + await once(plain, "connect"); + const secure = tls.connect({ socket: plain, servername: "localhost", ca: certs.cert }); + const events = observe(plain, secure); + await once(secure, "secureConnect"); + secure.end(); + expect(await events).toEqual(["plain close hadError=false", "secure close hadError=false plain.destroyed=true"]); + }); +}); + test("should be able to upgrade a paused socket and also have backpressure on it #15438", async () => { // enought to trigger backpressure const payload = Buffer.alloc(16 * 1024 * 4, "b").toString("utf8");