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

Bun.sql: SCRAM-SHA-256-PLUS (channel binding) support #16700

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jawj
Copy link

@jawj jawj commented Jan 24, 2025

What does this PR do?

Adds support for SCRAM-SHA-256-PLUS auth (i.e. tls-server-end-point channel binding) to Bun.sql, and uses it if the server offers it.

This is my first time reading or writing Zig, so it may be inelegant or even wrong in places.

How did you verify your code works?

I've done some manual testing locally using the OpenSSL test certs with this Docker script:

#!/bin/zsh
docker run --rm --name postgres-tls-cert \
  -p 5432:5432 \
  -e POSTGRES_USER=frodo \
  -e POSTGRES_PASSWORD=friend \
  -v ./openssl/test/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

# e.g. ./docker-pg.sh ecdsa

And this command:

bun run build && echo 'import { sql } from "bun"; console.log(await sql`SELECT now()`)' \
| DATABASE_URL="postgresql://frodo:friend@localhost:5432/frodo?sslmode=require" build/debug/bun-debug run -

I'm not sure the channel binding code as it currently stands would deal correctly with RSA-PSS or Ed25519 certificates. But I can't check this, because it seems Bun.sql doesn't support those certificates in the first place? (I get ERR_POSTGRES_CONNECTION_CLOSED locally and LOG: could not accept SSL connection: no suitable signature algorithm in the Postgres logs when I use these, even using the unmodified Bun 1.2.0 release).

I haven't written any automated tests so far. If you're otherwise willing to merge this, perhaps you could give a few pointers on these?

@Jarred-Sumner
Copy link
Collaborator

Wow awesome.

I'm not sure the channel binding code as it currently stands would deal correctly with RSA-PSS or Ed25519 certificates. But I can't check this, because it seems Bun.sql doesn't support those certificates in the first place

@cirospaciari knows more than me about this, but I wonder if we need to pass tls: { ca: ... in the Bun.SQL constructor? It's also possible there's a missing configuration somewhere when creating the SSL_CTX. One other thought is some misconfiguration when we upgrade the socket from TCP -> TLS (since thats how postgres does it, iirc) versus most other socket-related code in Bun where we start with TLS at initialization time

This looks directionally good to me but we would need a test of some kind before we can merge it

@jawj
Copy link
Author

jawj commented Jan 24, 2025

@cirospaciari I guess this may also be a bug report, then. :)

If it set up a Postgres instance using the script above, with either ./docker-pg.sh ed25519 or ./docker-pg.sh pss , I can connect fine with psql, but not with Bun. In both cases it looks like this:

Georges-Neon-MBP-M3 george 15:50 ~ % echo 'SELECT * FROM pg_stat_ssl' | psql "postgresql://frodo:friend@localhost:5432/frodo?sslmode=require"
 pid | ssl | version |         cipher         | bits | client_dn | client_serial | issuer_dn 
-----+-----+---------+------------------------+------+-----------+---------------+-----------
 157 | t   | TLSv1.3 | TLS_AES_256_GCM_SHA384 |  256 |           |               | 
(1 row)

Georges-Neon-MBP-M3 george 15:50 ~ % echo 'import { sql } from "bun"; console.log(await sql`SELECT * FROM pg_stat_ssl`)' \
| DATABASE_URL="postgresql://frodo:friend@localhost:5432/frodo?sslmode=require" bun run -
PostgresError: Connection closed
 code: "ERR_POSTGRES_CONNECTION_CLOSED"

And in Postgres logs I see:

2025-01-24 15:51:08.414 UTC [88] LOG:  could not accept SSL connection: no suitable signature algorithm

@cirospaciari
Copy link
Member

@jawj thank you for the PR, looks like CA is not being load, will investigate

@jawj
Copy link
Author

jawj commented Jan 28, 2025

Thanks @cirospaciari. Once this is addressed I can continue to work on channel binding support.

@cirospaciari
Copy link
Member

cirospaciari commented Jan 30, 2025

Thanks @cirospaciari. Once this is addressed I can continue to work on channel binding support.

I already have some fix in-place but is not merged yet, I'm investigating and fixing another issue in postgres and after this probably tomorrow a PR will be made for it.

Update: Still investigating the Issue

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