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.

Sign up for GitHub

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
Open

Add support for SCRAM-SHA-256-PLUS i.e. channel binding #3356

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
65e1c72
Added support for SCRAM-SHA-256-PLUS i.e. channel binding
jawj Jan 10, 2025
a338521
Requested tweaks to channel binding
jawj Jan 14, 2025
8ab2b17
Additional tweaks to channel binding
jawj Jan 14, 2025
37f9285
Fixed lint complaints
jawj Jan 14, 2025
ae6ab3f
Update packages/pg/lib/crypto/sasl.js
jawj Jan 16, 2025
152396f
Update packages/pg/lib/crypto/sasl.js
jawj Jan 16, 2025
f899f8f
Update packages/pg/lib/client.js
jawj Jan 16, 2025
52e656c
Tweaks to channel binding
jawj Jan 16, 2025
1003bff
Merge branch 'add-scram-sha-256-plus' of https://github.com/jawj/node…
jawj Jan 16, 2025
50ee305
Now using homegrown certificate signature algorithm identification
jawj Jan 20, 2025
b3a8757
Update ssl.mdx with channel binding changes
jawj Jan 20, 2025
67a6e9c
Allow for config object being undefined when assigning enableChannelB…
jawj Jan 20, 2025
b604068
Fixed a test failing on an updated error message
jawj Jan 20, 2025
88cbe49
Removed - from hash names like SHA-256 for legacy crypto (Node 14 and…
jawj Jan 21, 2025
05690bb
Removed packageManager key from package.json
jawj Jan 22, 2025
d9fdccf
Added some SASL/channel binding unit tests
jawj Jan 28, 2025
9a91cb7
Added a unit test for continueSession to check expected SASL session …
jawj Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/pg/lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
this.database = this.connectionParameters.database
this.port = this.connectionParameters.port
this.host = this.connectionParameters.host
this.enableChannelBinding = true

// "hiding" the password so it doesn't show up in stack traces
// or if the client is console.logged
Expand Down Expand Up @@ -258,7 +259,7 @@
_handleAuthSASL(msg) {
this._checkPgPass(() => {
try {
this.saslSession = sasl.startSession(msg.mechanisms)
this.saslSession = sasl.startSession(msg.mechanisms, this.enableChannelBinding && this.connection.stream)
this.connection.sendSASLInitialResponseMessage(this.saslSession.mechanism, this.saslSession.response)
} catch (err) {
this.connection.emit('error', err)
Expand All @@ -268,7 +269,7 @@

async _handleAuthSASLContinue(msg) {
try {
await sasl.continueSession(this.saslSession, this.password, msg.data)
await sasl.continueSession(this.saslSession, this.password, msg.data, this.enableChannelBinding && this.connection.stream)

Check failure on line 272 in packages/pg/lib/client.js

View workflow job for this annotation

GitHub Actions / lint 

Replace `this.saslSession,·this.password,·msg.data,·this.enableChannelBinding·&&·this.connection.stream` with `⏎········this.saslSession,⏎········this.password,⏎········msg.data,⏎········this.enableChannelBinding·&&·this.connection.stream⏎······`
this.connection.sendSCRAMClientFinalMessage(this.saslSession.response)
} catch (err) {
this.connection.emit('error', err)
Expand Down
59 changes: 52 additions & 7 deletions packages/pg/lib/crypto/sasl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,40 @@
'use strict'
const crypto = require('./utils')
const tls = require('tls');

Check failure on line 3 in packages/pg/lib/crypto/sasl.js

View workflow job for this annotation

GitHub Actions / lint 

Delete `;`

function startSession(mechanisms) {
if (mechanisms.indexOf('SCRAM-SHA-256') === -1) {
throw new Error('SASL: Only mechanism SCRAM-SHA-256 is currently supported')
function startSession(mechanisms, stream) {
const candidates = ['SCRAM-SHA-256']
if (stream) candidates.unshift('SCRAM-SHA-256-PLUS') // higher-priority, so placed first

Check failure on line 7 in packages/pg/lib/crypto/sasl.js

View workflow job for this annotation

GitHub Actions / lint 

Delete `·`

let mechanism
for (const candidate of candidates) {
if (mechanisms.indexOf(candidate) !== -1) {
mechanism = candidate
break
}
}
jawj marked this conversation as resolved.
Show resolved Hide resolved

if (!mechanism) {
throw new Error('SASL: Only mechanisms ' + candidates.join(' and ') + ' are supported')
}

if (mechanism === 'SCRAM-SHA-256-PLUS' && !(stream instanceof tls.TLSSocket)) {
jawj marked this conversation as resolved.
Show resolved Hide resolved
// this should never happen if we are really talking to a Postgres server
throw new Error('SASL: Mechanism SCRAM-SHA-256-PLUS requires a secure connection')
}

const clientNonce = crypto.randomBytes(18).toString('base64')
const gs2Header = mechanism === 'SCRAM-SHA-256-PLUS' ? 'p=tls-server-end-point' : stream ? 'y' : 'n'

return {
mechanism: 'SCRAM-SHA-256',
mechanism,
clientNonce,
response: 'n,,n=*,r=' + clientNonce,
response: gs2Header + ',,n=*,r=' + clientNonce,
message: 'SASLInitialResponse',
}
}

async function continueSession(session, password, serverData) {
async function continueSession(session, password, serverData, stream) {
if (session.message !== 'SASLInitialResponse') {
throw new Error('SASL: Last message was not SASLInitialResponse')
}
Expand All @@ -40,7 +58,34 @@

var clientFirstMessageBare = 'n=*,r=' + session.clientNonce
var serverFirstMessage = 'r=' + sv.nonce + ',s=' + sv.salt + ',i=' + sv.iteration
var clientFinalMessageWithoutProof = 'c=biws,r=' + sv.nonce

// without channel binding:
let channelBinding = stream ? 'eSws' : 'biws' // 'y,,' or 'n,,', base64-encoded

// override if channel binding is in use:
if (session.mechanism === 'SCRAM-SHA-256-PLUS') {
const peerCert = stream.getPeerCertificate().raw
const x509 = await import('@peculiar/x509')
jawj marked this conversation as resolved.
Show resolved Hide resolved
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.

const sigAlgo = parsedCert.signatureAlgorithm
if (!sigAlgo) {
throw new Error('Could not extract signature algorithm from certificate')
}
const hash = sigAlgo.hash
if (!hash) {
throw new Error('Could not extract hash from certificate signature algorithm')
}
let hashName = hash.name
if (!hashName) {
throw new Error('Could not extract name from certificate signature algorithm hash')
}
if (/^(md5)|(sha-?1)$/i.test(hashName)) hashName = 'SHA-256' // for MD5 and SHA-1, we substitute SHA-256

Check failure on line 82 in packages/pg/lib/crypto/sasl.js

View workflow job for this annotation

GitHub Actions / lint 

Delete `·`
const certHash = await crypto.hashByName(hashName, peerCert)
const bindingData = Buffer.concat([Buffer.from('p=tls-server-end-point,,'), Buffer.from(certHash)])
channelBinding = bindingData.toString('base64')
}

var clientFinalMessageWithoutProof = 'c=' + channelBinding + ',r=' + sv.nonce
var authMessage = clientFirstMessageBare + ',' + serverFirstMessage + ',' + clientFinalMessageWithoutProof

var saltBytes = Buffer.from(sv.salt, 'base64')
Expand Down
5 changes: 5 additions & 0 deletions packages/pg/lib/crypto/utils-legacy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ function sha256(text) {
return nodeCrypto.createHash('sha256').update(text).digest()
}

function hashByName(hashName, text) {
return nodeCrypto.createHash(hashName).update(text).digest()
}

function hmacSha256(key, msg) {
return nodeCrypto.createHmac('sha256', key).update(msg).digest()
}
Expand All @@ -32,6 +36,7 @@ module.exports = {
randomBytes: nodeCrypto.randomBytes,
deriveKey,
sha256,
hashByName,
hmacSha256,
md5,
}
5 changes: 5 additions & 0 deletions packages/pg/lib/crypto/utils-webcrypto.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module.exports = {
randomBytes,
deriveKey,
sha256,
hashByName,
hmacSha256,
md5,
}
Expand Down Expand Up @@ -60,6 +61,10 @@ async function sha256(text) {
return await subtleCrypto.digest('SHA-256', text)
}

async function hashByName(hashName, text) {
return await subtleCrypto.digest(hashName, text)
}

/**
* Sign the message with the given key
* @param {ArrayBuffer} keyBuffer
Expand Down
1 change: 1 addition & 0 deletions packages/pg/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"author": "Brian Carlson <[email protected]>",
"main": "./lib",
"dependencies": {
"@peculiar/x509": "^1.12.3",
jawj marked this conversation as resolved.
Show resolved Hide resolved
"pg-connection-string": "^2.7.0",
"pg-pool": "^3.7.0",
"pg-protocol": "^1.7.0",
Expand Down
15 changes: 14 additions & 1 deletion packages/pg/test/integration/client/sasl-scram-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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).

const client = new pg.Client(config)
let usingSasl = false
client.connection.once('authenticationSASL', () => {
usingSasl = true
})
client.enableChannelBinding = true // default
jawj marked this conversation as resolved.
Show resolved Hide resolved
await client.connect()
assert.ok(usingSasl, 'Should be using SASL for authentication')
await client.end()
})

suite.testAsync('can connect using sasl/scram (channel binding disabled)', async () => {
const client = new pg.Client(config)
let usingSasl = false
client.connection.once('authenticationSASL', () => {
usingSasl = true
})
client.enableChannelBinding = false
await client.connect()
assert.ok(usingSasl, 'Should be using SASL for authentication')
await client.end()
Expand Down
Loading