fix(pg-connection-string): get closer to libpq semantics for `sslmode`

[pmalouin]

Summary

We have found that the handling of the sslmode connection string parameter is inconsistent with other PG libraries and with the reference libpq documentation. This PR proposes some changes to sslmode behavior that are more aligned with libpq.

Detailed sslmode behavior

Here is the list of all sslmode values and their expected behavior with this PR:

sslmode=verify-full

Require an SSL connection and verify the CA and server identity. No changes in this PR.

sslmode=verify-ca (changed)

Require an SSL connection and verify the CA, but not the server identity. This is achieved by setting ssl.checkServerIdentity to a no-op function (see docs). Previously, this mode behaved like verify-full but that was not consistent with the libpq implementation.

sslmode=require (changed)

If a root CA is provided via the sslrootcert parameter of the connection string, it behaves like verify-ca. Otherwise, require an SSL connection but do not verify CA and server identity (ssl.rejectUnauthorized is set to false). Previously, this mode behaved like verify-full but that was not consistent with the libpq implementation.

sslmode=no-verify

Require an SSL connection but do not verify CA and server identity (ssl.rejectUnauthorized is set to false). No changes in this PR. Note: this mode is not documented in libpq and does not appear to be broadly supported in other libraries, but has been kept as-is for the sake of backwards-compatibility. One option could be to mark it as deprecated since sslmode=require could be an alternative, but doing so might have little value for this project.

sslmode=prefer (changed)

Require an SSL connection but do not verify CA and server identity (ssl.rejectUnauthorized is set to false). Previously, this mode behaved like verify-full but that was not consistent with the libpq implementation. In reality, this mode should be even less strict and support a fallback logic from SSL to non-SSL connection if SSL is not accepted by the server. Implementing a fallback logic seems to be more complex to solve and I did not dare touch this, but this could eventually be addressed if users of this library deem this mode valuable.

sslmode=allow

Not supported by this library. No changes in this PR. For this mode also, there could be an opportunity to implement a fallback logic from non-SSL to SSL, but I did not dare touch this and I don't have data that suggests that this might be valuable for this project.

sslmode=disable

Only try a non-SSL connection. No changes in this PR

An important note is that this PR potentially introduces semver breaking changes, in particular because it relaxes the security constraints of some sslmode values:

• sslmode=prefer is less strict, users should switch to sslmode=verify-full to keep parity.
• sslmode=require is less strict, users should switch to sslmode=verify-full to keep parity.
• sslmode=verify-ca is less strict, users should switch to sslmode=verify-full to keep parity.

Prior discussions about sslmode

I believe this PR addresses concerns raised in these two GH issues in the past: https://github.com/brianc/node-postgres/issues/2281 https://github.com/brianc/node-postgres/issues/2009

In particular, there has been one case where the sslmode=verify-ca is currently too strict when connecting through AWS RDS Proxy, but the work-around of using sslmode=no-verify would disable CA verification completely.

Other languages/libraries and their support for sslmode

Just as a reference, these two libraries are also trying to be more or less consistent with libpq:

• Golang - pq
  • Supports disable, require, verify-ca andverify-full only.
• PostgreSQL JDBC Driver
  • Supports all 6 modes.

---

Thanks for considering this change and please let me know how I can polish this further for acceptance 🙏

[cyx]

@charmander do we have an idea when this will land / get released as part of pg9?

[ceefour]

Please merge this as it fixes #2281 and #2375

[ceefour]

Help @brianc to merge this

[ThisIsMissEm]

@brianc / @hjr3 what's the status on this? We've a PR on Mastodon blocked by this change: https://github.com/mastodon/mastodon/pull/25483

[charmander]

@ThisIsMissEm Is that PR blocked on this? It looks more like it would be made unnecessary by this.

[ThisIsMissEm]

@ThisIsMissEm Is that PR blocked on this? It looks more like it would be made unnecessary by this.

I'm saying "blocked" in the sense of if upstream is going to fix, that'd be preferable to a fix in our code for it

[charmander]

@ThisIsMissEm On second read, it looks like the Mastodon PR is necessary either way. Unless you were planning on deleting all of that code if this were merged and released soon enough?

[hjr3]

This PR introduces breaking changes. We should probably:

• merge https://github.com/brianc/node-postgres/pull/3128
• release version 2.7.0
• merge this PR
• see if there are any other breaking changes that need to be made
• release version 3.0

[ThisIsMissEm]

This PR introduces breaking changes. We should probably:

• merge feat(pg-connection-string): ClientConfig helper functions #3128
• release version 2.7.0
• merge this PR
• see if there are any other breaking changes that need to be made
• release version 3.0

@hjr3 that sounds good to me!