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

Add support for DB_SSLMODE=require in streaming API #25483

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zeeZ
Copy link

@zeeZ zeeZ commented Jun 17, 2023

There were several attempts to fix #11445, the latest I've found in #23960 (which is a fix for #21431).

Sadly the way it was done is by adding no-verify instead of require. While no-verify is supported by the node module, it is not supported by any other standard libraries, including the ruby ones.

Since there is no distiction made in documentation between DB_SSLMODE for streaming and the other backends and it's assumed the same values are used everywhere, commonly used values need to be supported.

A lot of workarounds already exist that essentially do the same.

Fixes #11445
Fixes #26034

@zeeZ zeeZ changed the title Add support for PG_SSLMODE=require in streaming API Add support for DB_SSLMODE=require in streaming API Jul 31, 2023
@ThisIsMissEm
Copy link
Contributor

I don't think this change alone would be enough, as often with hosts that require SSL mode, you need to pass not just ssl.rejectUnauthorized as false but also need to pass a custom CA certificate.

See: https://node-postgres.com/features/ssl

Essentially unlike ruby's pg module, which seems to forward all the tls stuff to the pg C library, node-postgres forwards a bunch of the connect stuff to the tls module in node.js, which means that there's potentially differences in how the options are handled.

@zeeZ
Copy link
Author

zeeZ commented Oct 14, 2023

Note that this "documentation" only provides examples and does not state whether a CA is required or not. Drilling down the links further to the creation of TLS sockets and then the reference to server sockets (for some obscure reason), you get the following:

rejectUnauthorized If not false the server will reject any connection which is not authorized with the list of supplied CAs. This option only has an effect if requestCert is true. Default: true.

To me this says when true a CA certificate does NOT need to be supplied. I've also verified that this is the case here.

Technically require with a CA explicitly set (which makes it behave like verify-ca) would require additional CA verification, but this is not implented in this project. This pull request is to change the naming from something made up to PG library standard nomenclature.

I'm not sure why the pg-connection-string from the same library is copied and reimplemented here, but they've also got it wrong and a proper fix to this has been stuck in limbo for over a year now: brianc/node-postgres#2709

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
streaming Streaming server
Projects
None yet
3 participants