Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(node/tls) Fix NotValidForName for host set via socket / servername #21441

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

chromakode
Copy link
Contributor

@chromakode chromakode commented Dec 3, 2023

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 to Deno.startTls was null. The hostname is determined by the following code:

let hostname = tlsOptions?.secureContext?.servername;
hostname = opts.host;
tlsOptions.hostname = hostname;

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 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.

@CLAassistant
Copy link

CLAassistant commented Dec 3, 2023

CLA assistant check
All committers have signed the CLA.

@chromakode
Copy link
Contributor Author

Hey @littledivy, bumping since you were looking into #20293. This looks like a pretty high impact fix for node libs connecting over TLS.

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@littledivy littledivy enabled auto-merge (squash) December 8, 2023 03:28
@littledivy littledivy merged commit 2235a1a into denoland:main Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Error: read UNKNOWN" error when using TLS to connect to database using mysql2
3 participants