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

Fallback to non-SSL connection #2720

Open
gabegorelick opened this issue Mar 14, 2022 · 4 comments
Open

Fallback to non-SSL connection #2720

gabegorelick opened this issue Mar 14, 2022 · 4 comments

Comments

@gabegorelick
Copy link
Contributor

The default sslmode in libpq is prefer:

first try an SSL connection; if that fails, try a non-SSL connection

It should be possible to have similar fallback behavior in node-postgres.

Currently, the following will fail with a The server does not support SSL connections error if the DB does not support TLS:

const client = new Client({ ssl: { rejectUnauthorized: true } });
await client.connect();

This requires knowing ahead of time whether the server supports TLS or not, even if we don't care about TLS.

@charmander
Copy link
Collaborator

This requires knowing ahead of time whether the server supports TLS or not, even if we don't care about TLS.

Not ruling out support, but: if you don’t care about TLS, don’t turn it on. If it’s a connection where it makes sense to have TLS, don’t be vulnerable to MITM to save one environment variable that distinguishes production from development or whatever the case may be.

@gabegorelick
Copy link
Contributor Author

if you don’t care about TLS, don’t turn it on.

There's a use case for connecting to a server where you don't know whether it's listening on TLS or not, as evidenced by libpq's support for this use case. In those cases, you often don't care about verifying the cert because you were happy to connect without TLS anyway. This can be a OK if the connection is secured at a different layer.

E.g. someone gives you a secure tunnel to their DB along with connection info, but neglected to specify if they're using TLS. This is pretty common, since people know to provide hostname, port, username, password, DB name, etc but often forget about ssl mode and certs. Or they, rightly or wrongly, just don't want to worry about certs since everything is already tunneled.

@oren-nonamesecurity
Copy link

+1
We don't always know in advance where our code will be used, and external DB is allowed.
A "prefer"/"allow" logic is missing here.

almereyda added a commit to CirclesUBI/api-server that referenced this issue Jun 13, 2023
This fixes a regression with databases connections that set an explicit
`sslmode`. Previously we had disabled it for all cases, assuming no
parameter would be provided.

This time we adapt to the possible case, in which an SSL mode is provided,
but in a notation not known by PostgreSQL.
We have seen cases, in which `no-verify` is specified for configuring the
`api-server` `psql` client, which we set to `require`, instead of the (possibly
insecure) default of `prefer`.

To cite https://www.postgresql.org/docs/current/libpq-ssl.html#LIBQ-SSL-CERTIFICATES

    "By default, PostgreSQL will not perform
    any verification of the server certificate."

This differs from the default in the Postgres library we use `pq`,
citing https://node-postgres.com/announcements#2020-02-25

    "Now we will use the default ssl options to tls.connect
    which includes rejectUnauthorized being enabled.
    This means your connection attempt may fail
    if you are using a self-signed cert."

As per that announcement, the behaviour is intentionally inherited from:

- https://nodejs.org/api/tls.html#tls_tls_connect_options_callback

As specifying an `sslmode`, even with `no-verify`, implies that we want to enforce
a secure connection, we are falling back to a sane default, `require`.

Reference:

- https://www.postgresql.org/docs/current/libpq-ssl.html#LIBPQ-SSL-SSLMODE-STATEMENTS
- https://github.com/brianc/node-postgres/blob/46cfb25baf8fdba87f71c3888fcb0021eaf829d3/packages/pg-connection-string/README.md?plain=1#L69-L72
- https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/connection-parameters.js#L30-L31
- https://github.com/brianc/node-postgres/blob/master/packages/pg-connection-string/index.js#L98-L101
- brianc/node-postgres#2720
almereyda added a commit to CirclesUBI/api-server that referenced this issue Jun 13, 2023
This fixes a regression with databases connections that set an explicit
`sslmode`. Previously we had disabled it for all cases, assuming no
parameter would be provided.

This time we adapt to the possible case, in which an SSL mode is provided,
but in a notation not known by PostgreSQL.
We have seen cases, in which `no-verify` is specified for configuring the
`api-server` `psql` client, which we set to `require`, instead of the (possibly
insecure) default of `prefer`.

To cite https://www.postgresql.org/docs/current/libpq-ssl.html#LIBQ-SSL-CERTIFICATES

    "By default, PostgreSQL will not perform
    any verification of the server certificate."

This differs from the default in the Postgres library we use `pq`,
citing https://node-postgres.com/announcements#2020-02-25

    "Now we will use the default ssl options to tls.connect
    which includes rejectUnauthorized being enabled.
    This means your connection attempt may fail
    if you are using a self-signed cert."

As per that announcement, the behaviour is intentionally inherited from:

- https://nodejs.org/api/tls.html#tls_tls_connect_options_callback

As specifying an `sslmode`, even with `no-verify`, implies that we want to enforce
a secure connection, we are falling back to a sane default, `require`.

Reference:

- https://www.postgresql.org/docs/current/libpq-ssl.html#LIBPQ-SSL-SSLMODE-STATEMENTS
- https://github.com/brianc/node-postgres/blob/46cfb25baf8fdba87f71c3888fcb0021eaf829d3/packages/pg-connection-string/README.md?plain=1#L69-L72
- https://github.com/brianc/node-postgres/blob/master/packages/pg/lib/connection-parameters.js#L30-L31
- https://github.com/brianc/node-postgres/blob/master/packages/pg-connection-string/index.js#L98-L101
- brianc/node-postgres#2720
@Themanwithoutaplan
Copy link

There's always a risk associated with dropping down or disabling SSL so this should ideally be controlled parametrically. After playing around it seems the best option is to explicitly pass ssl = false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants