maint: enable SCRAM support #193
Conversation
In order to support SCRAM support for the new Heroku Postgres "Essential" plans, we need to shift from MD5 hashed passwords in `auth_file` to plain text. This does not materially change the threat model, as anyone with dyno access can read the passwords from the environment just as well as the file. See: https://www.pgbouncer.org/config.html#authentication-file-format for more. This commit switches the `auth_type` to `scram-sha-256` and also pushes `server_tls_sslmode` up to `require` over `prefer`. Why not use a method like `auth_query`? Exposing something like `pg_authid` or `pg_shadow` in a safe way via a `SECURITY DEFINER` function is extremely challenging in a multi-tenant environment. This may change in the future. Fixes #155. Ref: https://gus.my.salesforce.com/a07EE00001rjvVBYAY
mble-sfdc
requested review from
subakva,
binarycleric,
edmorley,
jdowning and
troycoll
| auth_file = $CONFIG_DIR/users.txt | ||
| server_tls_sslmode = prefer |
There was a problem hiding this comment.
Do we expect this change to break anything? I suspect not now that we support TLS fleet-wide.
There was a problem hiding this comment.
I do not expect this to break anything - frankly if it does, consumers of the buildpack have bigger problems to worry about. We've been enforcing TLS for some time now.
There was a problem hiding this comment.
We've seen a few GitHub Issues in the past where customers are using the buildpack to connect to RDS/Crunchy/etc. I hope that this change doesn't impact that as well but I would assume they are also supporting TLS and agree customers have bigger problems if they are not for some reason.
There was a problem hiding this comment.
This change looks straight-forward enough to me and I'm glad that we're moving away from MD5ed passwords. I spoke to Troy on Slack and he has verified that the buildpack works against essential-0, mini, and standard-0 addons so we can be confident it'll work against the fleet.
There was a problem hiding this comment.
Guessing this will never be used on a multiuser system, but if it's possible, setting a umask up at the top to ensure the users.txt file is ONLY readable by the intended user/group would be a good practice. But this is likely going in a single-user container so prob not necessary.
|
So double checked that SCRAM has been supported since pg10, so that should not be an issue. But we'll need to be quite careful in our release notes (significant version bump?) to indicate the database must be configured to support scram auth in pg_hba.conf (which will be true for all heroku dbs, but as indicated above sometimes this buildpack might be used for other db targets). [edit] wait, this is cool: https://www.postgresql.org/docs/14/auth-password.html
|
There was a problem hiding this comment.
This looks great from a buildpack implementation point of view (I'll leave the Postgres side of the review to others). When this is released, do you think there is a need for a public changelog post on https://devcenter.heroku.com/changelog ?
|
@edmorley we've got a changelog drafted and approved, will publish as soon as this version is released. |
|
@troycoll Are you ready for me to perform the release? |
|
@edmorley yep fire away! I'll publish the Changelog now |
|
Released via #194. |
In order to support SCRAM support for the new Heroku Postgres "Essential" plans, we need to shift from MD5 hashed passwords in
auth_fileto plain text. This does not materially change the threat model, as anyone with dyno access can read the passwords from the environment just as well as the file.See: https://www.pgbouncer.org/config.html#authentication-file-format for more.
This commit switches the
auth_typetoscram-sha-256and also pushesserver_tls_sslmodeup torequireoverprefer.Why not use a method like
auth_query? Exposing something likepg_authidorpg_shadowin a safe way via aSECURITY DEFINERfunction is extremely challenging in a multi-tenant environment. This may change in the future.Fixes #155.
Ref: https://gus.my.salesforce.com/a07EE00001rjvVBYAY
Testing Notes:
heroku buildpacks:add https://github.com/heroku/heroku-buildpack-pgbouncer.git#17cb11aa1ebabf5d467cb6341c8dfc155533a326