Skip to content

Commit 8991405

Browse files
bartlomiejuclaude
andauthored
fix(ext/node): auto-start server-side STARTTLS handshake, add TLS upgrade tests (#33303)
- **Server-side STARTTLS never started the handshake (#31759):** creating a `TLSSocket` via `new tls.TLSSocket(socket, { isServer: true })` for STARTTLS protocols (SMTP, IMAP, XMPP, etc.) never called `_start()`, so the TLS handshake was never initiated and raw ClientHello bytes appeared as `data` events. Fixed by auto-calling `_start()` on `nextTick` when the socket is server-side and already connected. - **STARTTLS socket upgrade SNI regression tests (#33296):** added tests for the `tls.connect({ socket })` pattern used by `npm:pg`, covering SNI propagation from the `host` option and from `socket._host`. Closes #31759 Closes #33296 --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 33bee46 commit 8991405

2 files changed

Lines changed: 196 additions & 0 deletions

File tree

ext/node/polyfills/_tls_wrap.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -495,6 +495,18 @@ TLSSocket.prototype._init = function (socket, wrap) {
495495
assert(!socket);
496496
this.connecting = true;
497497
}
498+
499+
// Auto-start server-side TLS when the underlying socket is already
500+
// connected. This handles the STARTTLS pattern where a plain TCP socket
501+
// is wrapped with `new TLSSocket(socket, { isServer: true })` after a
502+
// plaintext exchange (SMTP, IMAP, XMPP, PostgreSQL, etc.).
503+
// Use nextTick so the caller can attach event listeners first.
504+
// Client-side sockets are started by tls.connect() instead.
505+
if (options.isServer && wrap && !this.connecting) {
506+
nextTick(() => {
507+
if (!this.destroyed) this._start();
508+
});
509+
}
498510
};
499511

500512
TLSSocket.prototype.renegotiate = function (_options, callback) {

tests/unit_node/tls_test.ts

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,6 +529,190 @@ BnRlc3RDQTCB
529529
(tls as any).setDefaultCACertificates([testCert]);
530530
});
531531

532+
// https://github.com/denoland/deno/issues/31759
533+
// Server-side STARTTLS: new tls.TLSSocket(socket, { isServer: true }) must
534+
// auto-start the TLS handshake without requiring an explicit _start() call.
535+
// This is used by SMTP, IMAP, XMPP, and similar STARTTLS protocols.
536+
Deno.test("tls.TLSSocket server-side STARTTLS auto-starts handshake", async () => {
537+
const { promise, resolve, reject } = Promise.withResolvers<void>();
538+
539+
const server = net.createServer((rawSocket) => {
540+
rawSocket.write("READY");
541+
rawSocket.once("data", (data) => {
542+
if (data.toString() === "STARTTLS") {
543+
rawSocket.write("OK", () => {
544+
// Server-side STARTTLS: no explicit _start() call
545+
const tlsSocket = new tls.TLSSocket(rawSocket, {
546+
isServer: true,
547+
key,
548+
cert,
549+
// deno-lint-ignore no-explicit-any
550+
} as any);
551+
tlsSocket.on("secure", () => {
552+
tlsSocket.write("SECURE");
553+
});
554+
tlsSocket.on("error", () => {});
555+
});
556+
}
557+
});
558+
});
559+
560+
server.listen(0, () => {
561+
// deno-lint-ignore no-explicit-any
562+
const port = (server.address() as any).port;
563+
const socket = net.connect({ host: "localhost", port });
564+
socket.once("data", (greeting) => {
565+
assertEquals(greeting.toString(), "READY");
566+
socket.write("STARTTLS");
567+
socket.once("data", (response) => {
568+
assertEquals(response.toString(), "OK");
569+
const tlsSocket = tls.connect({
570+
socket,
571+
host: "localhost",
572+
ca: rootCaCert,
573+
});
574+
tlsSocket.on("secureConnect", () => {
575+
assert(tlsSocket.authorized);
576+
});
577+
tlsSocket.setEncoding("utf8");
578+
tlsSocket.on("data", (d) => {
579+
assertEquals(d, "SECURE");
580+
tlsSocket.destroy();
581+
server.close();
582+
resolve();
583+
});
584+
tlsSocket.on("error", (err: Error) => {
585+
server.close();
586+
reject(err);
587+
});
588+
});
589+
});
590+
});
591+
592+
await promise;
593+
});
594+
595+
// https://github.com/denoland/deno/issues/33296
596+
// Regression test: tls.connect({ socket, host }) must send SNI derived from host.
597+
// pg (PostgreSQL client) does STARTTLS: exchanges plaintext over TCP then calls
598+
// tls.connect({ socket, host }) to upgrade. Without SNI, SNI-dependent servers
599+
// (e.g. Neon PostgreSQL) drop the connection.
600+
Deno.test("tls.connect socket upgrade sends SNI from host option", async () => {
601+
const { promise, resolve, reject } = Promise.withResolvers<void>();
602+
603+
// Server that checks SNI was received
604+
const server = net.createServer((rawSocket) => {
605+
rawSocket.once("data", (data) => {
606+
if (data.toString() === "STARTTLS") {
607+
rawSocket.write("OK", () => {
608+
const tlsSocket = new tls.TLSSocket(rawSocket, {
609+
isServer: true,
610+
key,
611+
cert,
612+
// deno-lint-ignore no-explicit-any
613+
} as any);
614+
// deno-lint-ignore no-explicit-any
615+
(tlsSocket as any)._start();
616+
tlsSocket.on("secure", () => {
617+
tlsSocket.write("hello");
618+
});
619+
tlsSocket.on("error", () => {});
620+
});
621+
}
622+
});
623+
});
624+
625+
server.listen(0, () => {
626+
// deno-lint-ignore no-explicit-any
627+
const port = (server.address() as any).port;
628+
const socket = net.connect({ host: "localhost", port });
629+
socket.on("connect", () => {
630+
// Exchange plaintext first (like pg SSLRequest/S)
631+
socket.write("STARTTLS");
632+
socket.once("data", (data) => {
633+
assertEquals(data.toString(), "OK");
634+
// Upgrade to TLS with host but no explicit servername
635+
const tlsSocket = tls.connect({
636+
socket,
637+
host: "localhost",
638+
ca: rootCaCert,
639+
});
640+
tlsSocket.on("secureConnect", () => {
641+
assert(tlsSocket.authorized, "Connection should be authorized");
642+
tlsSocket.destroy();
643+
server.close();
644+
resolve();
645+
});
646+
tlsSocket.on("error", (err: Error) => {
647+
server.close();
648+
reject(err);
649+
});
650+
});
651+
});
652+
});
653+
654+
await promise;
655+
});
656+
657+
// https://github.com/denoland/deno/issues/33296
658+
// Regression test: tls.connect({ socket }) without host should derive SNI
659+
// from the underlying socket's _host property.
660+
Deno.test("tls.connect socket upgrade derives SNI from socket._host", async () => {
661+
const { promise, resolve, reject } = Promise.withResolvers<void>();
662+
663+
const server = net.createServer((rawSocket) => {
664+
rawSocket.once("data", (data) => {
665+
if (data.toString() === "STARTTLS") {
666+
rawSocket.write("OK", () => {
667+
const tlsSocket = new tls.TLSSocket(rawSocket, {
668+
isServer: true,
669+
key,
670+
cert,
671+
// deno-lint-ignore no-explicit-any
672+
} as any);
673+
// deno-lint-ignore no-explicit-any
674+
(tlsSocket as any)._start();
675+
tlsSocket.on("secure", () => {
676+
tlsSocket.write("hello");
677+
});
678+
tlsSocket.on("error", () => {});
679+
});
680+
}
681+
});
682+
});
683+
684+
server.listen(0, () => {
685+
// deno-lint-ignore no-explicit-any
686+
const port = (server.address() as any).port;
687+
// Connect with host="localhost" so socket._host is set
688+
const socket = net.connect({ host: "localhost", port });
689+
socket.on("connect", () => {
690+
socket.write("STARTTLS");
691+
socket.once("data", (data) => {
692+
assertEquals(data.toString(), "OK");
693+
// Upgrade without host or servername - should use socket._host
694+
const tlsSocket = tls.connect({
695+
socket,
696+
// No host or servername!
697+
ca: rootCaCert,
698+
rejectUnauthorized: false,
699+
});
700+
tlsSocket.on("secureConnect", () => {
701+
tlsSocket.destroy();
702+
server.close();
703+
resolve();
704+
});
705+
tlsSocket.on("error", (err: Error) => {
706+
server.close();
707+
reject(err);
708+
});
709+
});
710+
});
711+
});
712+
713+
await promise;
714+
});
715+
532716
// https://github.com/denoland/deno/issues/30170
533717
Deno.test("tls.connect strips trailing dot from servername", async () => {
534718
const listener = Deno.listenTls({

0 commit comments

Comments
 (0)