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 SCRAM-SHA-256-PLUS i.e. channel binding #3356

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

jawj
Copy link

@jawj jawj commented Jan 10, 2025

Hi all. I hope you'll consider this patch, which adds support for SCRAM-SHA-256-PLUS.

SCRAM-SHA-256-PLUS on Postgres enables tls-server-end-point channel binding, where the client sends the server a hash of the certificate it received as part of the TLS handshake. This prevents some kinds of MITM attacks where the attacker obtains a certificate that appears valid for the server, but is not actually the server's.

So far I've tested it working against Neon (who support SCRAM-SHA-256-PLUS) and Supabase (who don't), and with the new client.enableChannelBinding flag both true and false.

Feel free to make any changes you think appropriate.

@jawj jawj changed the title Added support for SCRAM-SHA-256-PLUS i.e. channel binding Add support for SCRAM-SHA-256-PLUS i.e. channel binding Jan 10, 2025
packages/pg/package.json Outdated Show resolved Hide resolved
Copy link
Owner

@brianc brianc left a comment

Choose a reason for hiding this comment

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

thanks for the PR! Put a few comments on here. My main concern is changing the default behavior - as much as its a pain on users to enable the feature, I'd rather it be an opt-in thing vs a default...otherwise it's a slight breaking change, which requires a new semver major. Would be nice to add a callout in the docs about this as well...though I can handle that if you don't want to

@jawj
Copy link
Author

jawj commented Jan 14, 2025

Thanks @brianc. I've made those changes and added a short section to the docs.

However, while writing the docs it struck me that I have only provided the opt-in option directly on Client. How should this feature be exposed for those using pool.query?

packages/pg/lib/crypto/sasl.js Outdated Show resolved Hide resolved
packages/pg/lib/crypto/sasl.js Outdated Show resolved Hide resolved
packages/pg/lib/crypto/sasl.js Outdated Show resolved Hide resolved
// override if channel binding is in use:
if (session.mechanism === 'SCRAM-SHA-256-PLUS') {
const peerCert = stream.getPeerCertificate().raw
const parsedCert = new x509.X509Certificate(peerCert)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of a separate package, could stream.getPeerCertificate().fingerprint256 be used with a fixed selection of SHA-256? Or is that not the same hash?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Never mind – that wouldn’t be spec-compliant and I missed that the hash wasn’t being used for anything more than its name anyway.

Copy link
Author

Choose a reason for hiding this comment

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

I do appreciate that bringing in a whole X509 parsing library (and then parsing the whole certificate, when all we actually need is the signature algorithm) feels like overkill.

I did actually have a go at doing the minimum necessary parsing manually: see https://gist.github.com/jawj/04a90e51196ac054d6741c8d079d9cff

The reason I didn't go with that in the PR is that I haven't been able to find a list of either (a) what signature algorithms could theoretically be used or even (b) what signature algorithms would cover 99% of cases.

But I strongly suspect that the cases covered by this code would be almost all of them, and any missing ones might be plugged if people filed issues about them. So this could be another option?

Copy link
Owner

Choose a reason for hiding this comment

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

But I strongly suspect that the cases covered by this code would be almost all of them, and any missing ones might be plugged if people filed issues about them. So this could be another option?

I am absolutely totally down w/ the "make it mostly work and patch if different algorithms show up later" mode if it removes the requirement to pull in an entire dependency! I wouldn't say it's a mandatory change but certainly would be welcome. 😄 I have tried very hard over the years to keep as many 3rd party dependencies out of the code as possible just because....well...left-pad and all that stuff, ya know?

Copy link
Author

Choose a reason for hiding this comment

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

@brianc Good to hear: I'm all for that approach. I've done a bit more work on this code (including putting a base64 hash of the public cert in the error message, to make it easy to report failures in a way we can investigate) and removed the dependency.

packages/pg/lib/client.js Outdated Show resolved Hide resolved
@@ -45,12 +45,25 @@ if (!config.user || !config.password) {
return
}

suite.testAsync('can connect using sasl/scram', async () => {
suite.testAsync('can connect using sasl/scram (channel binding enabled)', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests would work just as well if there were no implementation of channel binding at all, so this PR probably needs a targeted test from someone.

Copy link
Author

@jawj jawj Jan 20, 2025

Choose a reason for hiding this comment

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

I think in an ideal world we would test using SSL certs with a variety of different signature algorithms. But I'm not sure how easily that fits with the current test setup.

Locally, I have been using:

#!/bin/zsh
docker run --rm --name postgres-tls-cert \
  -p 5432:5432 \
  -e POSTGRES_USER=frodo \
  -e POSTGRES_PASSWORD=friend \
  -v ./certs:/etc/pgssl \
  postgres:17.2-bookworm \
  -c ssl=on \
  -c ssl_cert_file=/etc/pgssl/server-$1-cert.pem \
  -c ssl_key_file=/etc/pgssl/server-$1-key.pem

Which is run like: ./docker-pg.sh ecdsa

  • And this test:
SCRAM_TEST_PGDATABASE=frodo \                                          
  SCRAM_TEST_PGUSER=frodo \
  SCRAM_TEST_PGPASSWORD=friend \
  PGSSLMODE=no-verify \
  node --tls-keylog=/path/to/keylog.txt packages/pg/test/integration/client/sasl-scram-tests.js

(The --tls-keylog enables the use of Wireshark if needed).

@brianc
Copy link
Owner

brianc commented Jan 17, 2025

How should this feature be exposed for those using pool.query?

hmm good question - would need to make it something that's thread through the pool to its create method as well. The pool passes its own constructor options to the client which probably if i were designing the API today from scratch I wouldn't do but..that ship sailed like 10 years ago 🙃 - so it should just be able to be set on the pool as well?

@brianc
Copy link
Owner

brianc commented Jan 17, 2025

@jawj ruh roh! looks like a test is failing (should be an easy fix). There are a lot of tests - some are duplicates, etc. If you just wanna run a subset locally for faster turn around you can run make test-unit and/or make test-integration from the ./packages/pg subfolder. In this case it was a unit test that broke (meaning you don't need an instance of postgres running locally)

@jawj
Copy link
Author

jawj commented Jan 20, 2025

I think the tests should be passing again now.

@jawj
Copy link
Author

jawj commented Jan 20, 2025

BTW, you may find this interesting: https://bytebybyte.dev/?postgres

(Writing this was how I figured out how channel binding works).

@jawj jawj requested review from brianc and charmander January 20, 2025 17:47
@jawj
Copy link
Author

jawj commented Jan 21, 2025

I just noticed that legacy crypto expects SHA256 rather than SHA-256, and so on, which is now fixed.

But a potential annoyance is that SubtleCrypto supports a very limited range of hash functions compared to Node's legacy crypto. If people run into this as a problem with real certificates, it could be worth considering using legacy crypto, when available, more widely.

@jawj
Copy link
Author

jawj commented Jan 28, 2025

OK, this seems to have stalled a little in relation to testing.

To summarise the previous position:

  • I had already added an integration test to ensure that SCRAM-SHA-256-PLUS is used (and the connection succeeds) when it's offered and channel binding is enabled. I think this may be failing in CI because an SSL connection is not used (PGTESTNOSSL: true). I guess I can add a check for SSL in the test, and abort if it's not used, but of course that means it won't be testing what it's meant to in CI. What do you think?

  • What's not currently tested automatically (via integration tests) is compatibility with various certificate signature algorithms. I think this would be nice to have, but it involves larger changes to the testing/CI infrastructure than I'm comfortable making. I have, however, successfully tested with the OpenSSL DSA, ECDSA, CECDSA, PSS and Ed25519 server certificates locally (Ed448 and ecdsa-brainpoolP256r1 are failing at the TLS-handshake stage, so the question of channel binding doesn't come up).

I've just added some more unit tests:

  • A few unit tests to ensure that SASL startSession returns the expected data when channel binding is and isn't enabled and SCRAM-SHA-256-PLUS is and isn't offered.

  • A similar unit test for SASL continueSession with mechanism: "SCRAM-SHA-256-PLUS", which includes a minimal ASN.1 certificate structure that can be parsed for a signature hash algorithm.

As part of the testing, I realised I have not provided a way to enforce (rather than just enable) channel binding. But since the client's response is different when it does and doesn't think the server supports channel binding, I don't think this makes a big difference to the risk of MITM attacks.

@brianc and @charmander, how do you think we can move this forward?

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.

3 participants