From 2235a1a359ffabd72689db58b9af5873e0a9b38a Mon Sep 17 00:00:00 2001 From: Max Goodhart Date: Thu, 7 Dec 2023 19:53:36 -0800 Subject: [PATCH] fix(node/tls): fix NotValidForName for host set via socket / servername (#21441) This PR is an attempt to fix https://github.com/denoland/deno/issues/20293, in which node modules connecting to databases fail due to TLS errors. I ran into this attempting to use [node-postgres](https://github.com/brianc/node-postgres) to connect to a [Neon](https://neon.tech) database. Investigating via `--inspect-brk` led me to notice that the hostname eventually passed to `Deno.startTls` was null. The hostname is determined by the following code: https://github.com/denoland/deno/blob/f6b889b43219e3c9be770c8b2758bff3048ddcbd/ext/node/polyfills/_tls_wrap.ts#L87-L89 This logic doesn't appear to be correct. I couldn't find reference to `servername` existing on the `secureContext` in either Node's or Deno's docs. There's a lot of scope here, and it's my first time reading through this code, so I could be missing something! Node uses [the following logic](https://github.com/nodejs/node/blob/2e458d973638d01fcb6a0d7d611e0120a94f4d35/lib/_tls_wrap.js#L1679-L1682 ) to determine the hostname for certificate validation: ``` const hostname = options.servername || options.host || (options.socket && options.socket._host) || 'localhost'; ``` This PR updates the `TLSSocket` polyfill to use behave similarly (though I omitted the default to `localhost` at the end; I'm not sure if including it is necessary or correct). With this change, `node-postgres` connects to my TLS endpoint successfully (aside: Neon requires SNI, which also works as expected). --- I tried to update the tests in https://github.com/denoland/deno/blob/main/cli/tests/unit_node/tls_test.ts to exercise this change, but the test fails for me on `main` on Linux. I investigated briefly and noticed that the test fixture `cli/tests/testdata/tls/localhost.crt` doesn't appear to include the `subjectAltName` specified in `domains.txt`. I believe the certificate isn't matching `localhost`, but that's where I ended investigating. --- ext/node/polyfills/_tls_wrap.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ext/node/polyfills/_tls_wrap.ts b/ext/node/polyfills/_tls_wrap.ts index 416bd4136f..1e130a543d 100644 --- a/ext/node/polyfills/_tls_wrap.ts +++ b/ext/node/polyfills/_tls_wrap.ts @@ -84,8 +84,7 @@ export class TLSSocket extends net.Socket { constructor(socket: any, opts: any = kEmptyObject) { const tlsOptions = { ...opts }; - let hostname = tlsOptions?.secureContext?.servername; - hostname = opts.host; + const hostname = opts.servername ?? opts.host ?? socket._host; tlsOptions.hostname = hostname; const _cert = tlsOptions?.secureContext?.cert;