From fe8d0079ecdfeb61cea6ba041ed6845d6fffa43e Mon Sep 17 00:00:00 2001 From: Ciro Spaciari Date: Sun, 20 Oct 2024 18:58:14 -0700 Subject: [PATCH] tls(Server) fix connectionListener and make alpnProtocol more compatible with node.js (#14695) Co-authored-by: cirospaciari --- src/bun.js/api/bun/socket.zig | 6 +--- src/js/node/net.ts | 40 +++++++++++++++-------- src/js/node/tls.ts | 5 +-- test/js/node/tls/node-tls-server.test.ts | 41 ++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 22 deletions(-) diff --git a/src/bun.js/api/bun/socket.zig b/src/bun.js/api/bun/socket.zig index 535b535e6a5845..c29e7b98a5f5db 100644 --- a/src/bun.js/api/bun/socket.zig +++ b/src/bun.js/api/bun/socket.zig @@ -1295,17 +1295,13 @@ fn selectALPNCallback( if (protos.len == 0) { return BoringSSL.SSL_TLSEXT_ERR_NOACK; } - const status = BoringSSL.SSL_select_next_proto(bun.cast([*c][*c]u8, out), outlen, protos.ptr, @as(c_uint, @intCast(protos.len)), in, inlen); - // Previous versions of Node.js returned SSL_TLSEXT_ERR_NOACK if no protocol // match was found. This would neither cause a fatal alert nor would it result // in a useful ALPN response as part of the Server Hello message. // We now return SSL_TLSEXT_ERR_ALERT_FATAL in that case as per Section 3.2 // of RFC 7301, which causes a fatal no_application_protocol alert. - const expected = if (comptime BoringSSL.OPENSSL_NPN_NEGOTIATED == 1) BoringSSL.SSL_TLSEXT_ERR_OK else BoringSSL.SSL_TLSEXT_ERR_ALERT_FATAL; - - return if (status == expected) 1 else 0; + return if (status == BoringSSL.OPENSSL_NPN_NEGOTIATED) BoringSSL.SSL_TLSEXT_ERR_OK else BoringSSL.SSL_TLSEXT_ERR_ALERT_FATAL; } else { return BoringSSL.SSL_TLSEXT_ERR_NOACK; } diff --git a/src/js/node/net.ts b/src/js/node/net.ts index db7a087eb72374..868de1d27bf086 100644 --- a/src/js/node/net.ts +++ b/src/js/node/net.ts @@ -84,6 +84,10 @@ function closeNT(callback, err) { callback(err); } +function detachAfterFinish() { + this[bunSocketInternal] = null; +} + var SocketClass; const Socket = (function (InternalSocket) { SocketClass = InternalSocket; @@ -164,7 +168,7 @@ const Socket = (function (InternalSocket) { self._secureEstablished = !!success; self.emit("secure", self); - + self.alpnProtocol = socket.alpnProtocol; const { checkServerIdentity } = self[bunTLSConnectOptions]; if (!verifyError && typeof checkServerIdentity === "function" && self.servername) { const cert = self.getPeerCertificate(true); @@ -291,10 +295,7 @@ const Socket = (function (InternalSocket) { if (typeof connectionListener == "function") { this.pauseOnConnect = pauseOnConnect; - if (isTLS) { - // add secureConnection event handler - self.once("secureConnection", () => connectionListener.$call(self, _socket)); - } else { + if (!isTLS) { connectionListener.$call(self, _socket); } } @@ -312,6 +313,7 @@ const Socket = (function (InternalSocket) { self._secureEstablished = !!success; self.servername = socket.getServername(); const server = self.server; + self.alpnProtocol = socket.alpnProtocol; if (self._requestCert || self._rejectUnauthorized) { if (verifyError) { self.authorized = false; @@ -329,7 +331,11 @@ const Socket = (function (InternalSocket) { } else { self.authorized = true; } - self.server.emit("secureConnection", self); + const connectionListener = server[bunSocketServerOptions]?.connectionListener; + if (typeof connectionListener == "function") { + connectionListener.$call(server, self); + } + server.emit("secureConnection", self); // after secureConnection event we emmit secure and secureConnect self.emit("secure", self); self.emit("secureConnect", verifyError); @@ -685,14 +691,23 @@ const Socket = (function (InternalSocket) { } _destroy(err, callback) { - const socket = this[bunSocketInternal]; - if (socket) { + const { ending } = this._writableState; + // lets make sure that the writable side is closed + if (!ending) { + // at this state destroyed will be true but we need to close the writable side + this._writableState.destroyed = false; + this.end(); + // we now restore the destroyed flag + this._writableState.destroyed = true; + } + + if (this.writableFinished) { + // closed we can detach the socket this[bunSocketInternal] = null; - // we still have a socket, call end before destroy - process.nextTick(endNT, socket, callback, err); - return; + } else { + // lets wait for the finish event before detaching the socket + this.once("finish", detachAfterFinish); } - // no socket, just destroy process.nextTick(closeNT, callback, err); } @@ -706,7 +721,6 @@ const Socket = (function (InternalSocket) { this.#final_callback = callback; } else { // emit FIN not allowing half open - this[bunSocketInternal] = null; process.nextTick(endNT, socket, callback); } } diff --git a/src/js/node/tls.ts b/src/js/node/tls.ts index 1df7d858cf5042..942a61e5fefd49 100644 --- a/src/js/node/tls.ts +++ b/src/js/node/tls.ts @@ -330,6 +330,7 @@ const TLSSocket = (function (InternalTLSSocket) { #socket; #checkServerIdentity; #session; + alpnProtocol = null; constructor(socket, options) { super(socket instanceof InternalTCPSocket ? options : options || socket); @@ -503,10 +504,6 @@ const TLSSocket = (function (InternalTLSSocket) { throw Error("Not implented in Bun yet"); } - get alpnProtocol() { - return this[bunSocketInternal]?.alpnProtocol; - } - [buntls](port, host) { return { socket: this.#socket, diff --git a/test/js/node/tls/node-tls-server.test.ts b/test/js/node/tls/node-tls-server.test.ts index 6b0b9c393c7182..1dc41d31e6659a 100644 --- a/test/js/node/tls/node-tls-server.test.ts +++ b/test/js/node/tls/node-tls-server.test.ts @@ -6,6 +6,7 @@ import { tmpdir } from "os"; import { join } from "path"; import type { PeerCertificate } from "tls"; import tls, { connect, createServer, rootCertificates, Server, TLSSocket } from "tls"; +import { once } from "node:events"; const { describe, expect, it, createCallCheckCtx } = createTest(import.meta.path); @@ -662,3 +663,43 @@ it("tls.rootCertificates should exists", () => { expect(rootCertificates.length).toBeGreaterThan(0); expect(typeof rootCertificates[0]).toBe("string"); }); + +it("connectionListener should emit the right amount of times, and with alpnProtocol available", async () => { + let count = 0; + const promises = []; + const server: Server = createServer( + { + ...COMMON_CERT, + ALPNProtocols: ["bun"], + }, + socket => { + count++; + expect(socket.alpnProtocol).toBe("bun"); + socket.end(); + }, + ); + server.setMaxListeners(100); + + server.listen(0); + await once(server, "listening"); + for (let i = 0; i < 50; i++) { + const { promise, resolve } = Promise.withResolvers(); + promises.push(promise); + const socket = connect( + { + ca: COMMON_CERT.cert, + rejectUnauthorized: false, + port: server.address().port, + ALPNProtocols: ["bun"], + }, + () => { + socket.on("close", resolve); + socket.resume(); + socket.end(); + }, + ); + } + + await Promise.all(promises); + expect(count).toBe(50); +});