Conversation
e03f14d to
3a71eb6
Compare
| {% if postgresql_dbs.project_db %} | ||
| ; Support for authenticating as dynamically created users for ProjectDB | ||
| auth_user = {{ pgbouncer_auth_username }} | ||
| auth_query = SELECT usename, passwd FROM pgbouncer.user_lookup($1) |
There was a problem hiding this comment.
Setting this does mean that any attempts to login with a bad user will error in dbs that don't have user_lookup defined
MartinRiese
left a comment
There was a problem hiding this comment.
Makes sense to me. But I don't feel comfortable approving.
3a71eb6 to
238c809
Compare
| SET search_path = pg_catalog, pg_temp | ||
| AS $$ | ||
| SELECT rolname, CASE WHEN rolvaliduntil < now() THEN NULL ELSE rolpassword END | ||
| FROM pg_authid WHERE rolname = username AND rolcanlogin; |
There was a problem hiding this comment.
I see that this is a general function, but for our purposes should it have a LIKE 'projectdb\_%' namespace check? As is it also works for any role.
There was a problem hiding this comment.
It could - that's something I considered when implementing this, though I think I was hedging towards keeping this generic in case we needed to adopt this pattern more broadly in the future. However it does seem better to be stricter about the scope if there's no big disadvantage to doing so, as this could conceivably be part of a privilege escalation avenue
| REVOKE EXECUTE ON FUNCTION pgbouncer.user_lookup(text) FROM PUBLIC; | ||
| GRANT USAGE ON SCHEMA pgbouncer TO {{ pgbouncer_auth_username }}; | ||
| GRANT EXECUTE ON FUNCTION pgbouncer.user_lookup(text) TO {{ pgbouncer_auth_username }}; | ||
| GRANT CONNECT ON DATABASE {{ postgresql_dbs.project_db.name }} TO {{ pgbouncer_auth_username }}; |
There was a problem hiding this comment.
d969db4 commit message is "pgbouncer" - would have been nice to elaborate on what that means? No need to rebase/amend the commit message now, but maybe leave a comment here with a bit more detail about why this was removed?
There was a problem hiding this comment.
Oh, sorry about that - I'd meant to rebase and fixup that commit into the commit that introduced that line, so it never showed up in the PR, but must've forgotten. That came up in my own code review of this with Claude, that it shouldn't be necessary to GRANT CONNECT, as that's already the default.
| login_host: "{{ db_is_remote and postgresql_host or omit }}" | ||
| login_user: "{{ db_is_remote and postgres_users.root.username or omit }}" | ||
| login_password: "{{ db_is_remote and postgres_users.root.password or omit }}" |
There was a problem hiding this comment.
The second part of my earlier comment was easy to miss. Do you agree that these should also be changed to <true-value> if <condition> else <false-value> syntax?
There was a problem hiding this comment.
Oh thanks yeah I did miss that. I'm guessing Claude did it that way because that's apparently the pattern in this file. Not sure whether the existing references intend to fallback to omit when the value is Falsy, but I can at least deal with that here
0028811 to
7af8842
Compare
c07b503 to
4b62d8d
Compare
|
@millerdev @gherceg @esoergel Could I get another review? The force pushes that involved Ethans commits were all rebasing using github's stacks. So this is only for the four I added. With the last one being the only controversial one. AWS RDS does not expose pg_authid to anybody. So added a separate table that we manage would be one option. That is what (A)I implemented. The password is already salted and hashed at that point so storing them in this way should be fine. An alternative that Ethan pointed out would be to use a common password between the users that would be provided at deploy time like for the other db users. Claude complained about that because the whole point is to separate access to different domains. Since we menage the passwords anyways and there would still be a separate user I don't think that would be a problem. I would like to hear your take on that. p.s. the run book now passes. |
gherceg
left a comment
There was a problem hiding this comment.
I did a pass over those recent commits but feel like I would need to do more of my own research to give feedback on your approach. If @millerdev hasn't looked yet I'd be interested in pairing on this as well.
| self.dbs.project_db.pgbouncer_hosts = list(ucr.pgbouncer_hosts) | ||
| if self.dbs.project_db.pgbouncer_endpoint is None: | ||
| self.dbs.project_db.pgbouncer_endpoint = ucr.pgbouncer_endpoint | ||
| self.pgbouncer_override.pgbouncer_auth_type = 'scram-sha-256' |
There was a problem hiding this comment.
nit: would put this line before the if blocks just for easier readability, but I know this is very nitpicky.
| CREATE EXTENSION IF NOT EXISTS fuzzystrmatch; | ||
|
|
||
| -- RDS doesn't expose pg_authid, create our own stand-in | ||
| CREATE SCHEMA IF NOT EXISTS pgbouncer; |
There was a problem hiding this comment.
Perhaps I'm misunderstanding, but if we are adding our own schema, should we name it something more custom/specific since it seems possible for this schema name to conflict if pgbouncer decided it wanted a schema in dbs it connects to?
In transaction pooling mode pgbouncer verifies passwords itself, so every per-domain role would otherwise need an entry in userlist.txt auth_query lets pgbouncer ask Postgres instead This is the database half of that: - a pgbouncer_auth login role, created only where project_db lives - pgbouncer.user_lookup(), which returns one row for one username. Password hashes live in pg_authid, which is superuser-only, so the lookup goes through a SECURITY DEFINER wrapper rather than granting the auth account broad catalog access. EXECUTE is revoked from PUBLIC and granted only to pgbouncer_auth. The user_lookup function is pulled from pgbouncer's documentation (https://www.pgbouncer.org/config.html) PGBOUNCER_AUTH_PASSWORD defaults to empty since only environments with a project_db need it. The pgbouncer configuration follows separately.
This is just a formality - Changelog 0071 from early 2023 told everyone to get on Ubuntu 22.04, which shipped with pgbouncer 1.16. 1.14 is the min version that contains SCRAM support, which is needed for dynamically querying password hashes
Completes the auth_query wiring: pgbouncer now resolves unknown usernames by asking Postgres Both ini templates are updated, though I'm pretty sure the "classic" one is unused
…dbs.yml Co-authored-by: Daniel Miller <dmiller@dimagi.com>
"Set up the project_db database" now references pgbouncer_auth which in check mode is created in a transaction and not commited. So skip both in check mode.
93d07d3 moved postgres to version 14 for scram support. This actually uses it.
It rejects any changes that contain SUPERUSER. Even the negative in this case. NOSUPERUSER is the default if not specified. There is no need to explicitly list it (https://www.postgresql.org/docs/current/sql-createrole.html).
RDS doesn't expose pg_authid, create our own stand-in. Grant access to provisioning user but remove from public.
4b62d8d to
b99a82c
Compare
|
@gherceg both fair points. I'll wait for more feedback before renaming anything. |
These three stacked PRs introduce a database-level safeguard that keeps a Project DB query from reading another domain's data, even if the query layer above it has a bug or is handed hostile SQL.
Part I: HQ Query Users (#6962)
Each domain gets its own postgres role, which has read-only access to only that domain's tables. Queries on behalf of that domain are authenticated as that user, so postgres enforces separation between tenants.
HQ provisions these query users using a constrained
SECURITY DEFINERfunction calledprojectdb_provision_rolewhich narrowly defines the action. This function is created during database provisioning. Passwords are derived from the domain name viadjango.utils.crypto.salted_hmac, never stored.Part II: PgBouncer
PgBouncer connection pooling complicates things in two ways
Part IIa: PGBouncer Authentication (This PR)
PgBouncer authenticates queries itself, so it has to know user passwords. Currently, it validates against a file called
userlist.txtwhich is managed by commcare-cloud. Adding a new user requires amending that file, which just isn't doable in this context. Instead, we give pgbouncer anauth_queryit can run to get the username and password to authenticate a user that's not inuserlist.txt.To do this, we need a few things in place:
user_lookupfunction as described in the pgbouncer docs (https://www.pgbouncer.org/config.html), supporting theauth_queryauth_userpgbouncer can authenticate as when running theauth_query, and a password for that useruser_lookupfunctionPart IIb: (#6965)
PgBouncer keys its pools by
(user, database)pair, meaning that each domain will have it's own pool in pgbouncer. Right now, we allow up to 490 open connections per pool (default_pool_size). Since ProjectDB uses many pools, we can instead set amax_db_connectionsas a limit across all ProjectDB pools, and set a much lowerpool_sizeto prevent one domain from claiming all that for itself.Environments Affected
Staging, since it has
project_dbconfigured already. Should only affect other environments when that's enabled.Announce New Release
No. Shouldn't require action by anyone but me.