pass servername to tls.connect

[chaporgin]

This addresses the inability to connect using SNI to a TCP proxy hosting multiple Postgres instances behind itself. This is, for example, the case of https://neon.tech. With this patch, it is possible to establish a connection to the Neon compute-node by specifying postgres('database_url', { ssl: true }) while connecting from Javascript. Without this patch, the servername is not passed.

See nodejs docs for details on servername option and SNI extension to TLS.

[chaporgin]

@porsager , please have a look.

[chaporgin]

@porsager I saw that tests exist in this repo, but they are run manually. And saw test run was unsuccessful. I tried to run them locally with act from nektos, but failed. So I think I fixed the test but waiting for you to run them. Sorry for pinging/

[porsager]

There u go 😉 I don't think I can change that I need to manually allow the very first test run for a user, but now they should at least run for you 😉

[bonesoul]

also looking for this to get working with neon.tech

[chaporgin]

@porsager still cannot run the workflow, but updated the tests. Please have a look.

[chaporgin]

Oh, the tests passed somehow. I did not trigger the workflow.
Can we squash & merge it?

[porsager]

Great, perhaps we can have @bonesoul give your branch a shot to see if it works before I look closer + merge?

[chaporgin]

@bonesoul could you give it a try?

[bonesoul]

well I dropped neon.tech support on my project cause of the issue.

[porsager]

Hi @AntonyC .. Finally had some time to check this out and give neon.tech a try.. I'm curious I think simply doing the following solves the issue with minimal changes:

     socket = tls.connect({
       socket,
+      servername: socket.host,
       ...(ssl === 'require' || ssl === 'allow' || ssl === 'prefer'
         ? { rejectUnauthorized: false }

[chaporgin]

@porsager , thanks for the proposal!

tls.connect docs say "must" for the host name not an IP address. So I think blindly passing servername which can be an IP is undefined behavior.

servername: Server name for the SNI (Server Name Indication) TLS extension. It is the name of the host being connected to, and must be a host name, and not an IP address. It can be used by a multi-homed server to choose the correct certificate to present to the client, see the SNICallback option to tls.createServer().

[porsager]

Oh right - it also seems to generate a pretty error in node complaining if it's an IP.

Ok, so since socket.host contains the current host being connected to, this should be enough right?

     socket = tls.connect({
       socket,
+      servername: net.isIP(socket.host) ? undefind : socket.host,
       ...(ssl === 'require' || ssl === 'allow' || ssl === 'prefer'
         ? { rejectUnauthorized: false }

To include deno support, the following could be added to polyfill.js instead of mixing platform specifics into the main src

  import { Buffer } from 'https://deno.land/[email protected]/node/buffer.ts'
+ import { isIP } from 'https://deno.land/[email protected]/node/net.ts'

  const events = () => ({ data: [], error: [], drain: [], connect: [], secureConnect: [], close: [] })

  export const net = {
+   isIP,
    createServer() {
      const server =  {

Would also be nice if you could remove the artifacts from cjs and deno builds in the PR.

[boromisp]

FYI there are workarounds for neon.tech: https://neon.tech/docs/connect/connectivity-issues, some of them would probably work with postgres.js as-is. Nonetheless, having SNI work will be nicer.

[porsager]

I'm gonna add my suggestion above unless @AntonyC has any objections or wants to edit his PR to match it. I've tested it works fine with neon

[chaporgin]

Looks good to me, no expert on deno

[porsager]

Cool.. want me to push it or do you wanna edit this pr?

[chaporgin]

Thanks!