fix(node/tls) Fix NotValidForName for host set via socket / servername #21441
+1
−2
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is an attempt to fix #20293, in which node modules connecting to databases fail due to TLS errors. I ran into this attempting to use node-postgres to connect to a Neon database.
Investigating via
--inspect-brk
led me to notice that the hostname eventually passed toDeno.startTls
was null. The hostname is determined by the following code:deno/ext/node/polyfills/_tls_wrap.ts
Lines 87 to 89 in f6b889b
This logic doesn't appear to be correct. I couldn't find reference to
servername
existing on thesecureContext
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 to determine the hostname for certificate validation:
This PR updates the
TLSSocket
polyfill to use behave similarly (though I omitted the default tolocalhost
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 fixturecli/tests/testdata/tls/localhost.crt
doesn't appear to include thesubjectAltName
specified indomains.txt
. I believe the certificate isn't matchinglocalhost
, but that's where I ended investigating.