Add Project DB per-domain role provisioning functions - #6962
Conversation
| REVOKE EXECUTE ON FUNCTION projectdb_provision_role(text, text) FROM PUBLIC; | ||
| REVOKE EXECUTE ON FUNCTION projectdb_drop_role(text) FROM PUBLIC; | ||
|
|
||
| GRANT EXECUTE ON FUNCTION projectdb_provision_role(text, text) TO {{ postgres_users.commcare.username }}; |
There was a problem hiding this comment.
so {{ postgres_users.commcare.username }} is a superuser?
There was a problem hiding this comment.
No, the commcare user doesn't normally have permission to modify roles. Its permissions are limited, and granting CREATEROLE directly would open up potential privilege escalation paths:
Because the
CREATEROLEprivilege allows a user to grant or revoke membership even in roles to which it does not (yet) have any access, aCREATEROLEuser can obtain access to the capabilities of every predefined role in the system, including highly privileged roles such aspg_execute_server_programandpg_write_server_files.
This was actually addressed in PostgreSQL 16, but we're still on 14. So instead, projectdb_provision_role is a SECURITY DEFINER function (meaning it runs with the permissions of the role which created it). By granting just that function to the commcare user, its ability to fiddle with roles is limited to creating and deleting roles starting with projectdb_. Further, roles are cluster wide, and not limited to just the projectdb logical database.
millerdev
left a comment
There was a problem hiding this comment.
Offlined with Ethan about concerns around connection pooling per-domain and how that could affect PG resource usage. It was already on his radar.
This looks like a solid start. However, there is a small part of me that wonders if we actually need it? On the con side, it may add significant inconvenience to debugging projectdb data. On the other hand, I can understand the rationale given that the plan is to execute (pre-parsed and re-rendered) user-supplied SQL against these tables. It's a belt-and-suspenders approach.
| ELSE | ||
| EXECUTE format('CREATE ROLE %I WITH LOGIN PASSWORD %L', role_name, role_password); | ||
| END IF; | ||
| EXECUTE format('CREATE ROLE %I WITH LOGIN PASSWORD %L', role_name, role_password); |
There was a problem hiding this comment.
With this change, projectdb_provision_role no longer reads pg_roles, so the pg_temp-shadowing concern no longer applies. Seems like pg_temp could be removed from the search path, although maybe it's safer to leave it in case this function changes in a way that needs it later?
Absolutely. When I pitched the idea, it seemed like a pretty simple way to get a too-simple-to-fail security layer that makes this unassailable, whereas the other mechanisms are probably going to have some degree of brittleness. Not that I don't trust them - it's just like providing DB level schema guarantees vs application layer wrappers.
How so? There will still be a CommCareHQ application user with full read and write access to all tables. You and I already spoke about this offline, but recording here for others (and for posterity) - I think we might want to route any kind of complex querying through this user, but we can also use the regular CommCareHQ user for simple access patterns. One benefit of that is that if we GA a version of this, we don't necessarily need a user (and pgbouncer pool) for every domain - if this is accessed only through some Case List Explorer type report or something similarly controlled, we can say that the normal mechanisms of tenant separation are sufficient. |
That makes sense. I had missed/misunderstood it in our original discussion. Thank for reiterating. |
a012e99 to
9d32210
Compare
There is more of it coming, and a growing plpgsql body embedded in YAML is hard to read. No change to the SQL itself. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Project DB will give each domain its own read-only Postgres login role, so that a query issued for one domain cannot read another domain's schema even if the layer above it is buggy or handed hostile SQL. Creating a role requires CREATEROLE, which the commcare user does not have and should not be given: the unrestricted pre-PG16 version of that attribute would let the web process alter, drop, or take over any non-superuser role in the cluster. These functions run as their superuser owner instead and refuse to touch any role outside the projectdb_ namespace. Nothing calls them yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
On PG14 the public schema still grants CREATE to PUBLIC, so a per-domain role would be able to create objects in it. PG15 changed that default, and this is a no-op there. USAGE is left alone on purpose. The Project DB extensions are installed into public, and domain roles need to reach similarity(), dmetaphone() and the earthdistance functions.
Unless pg_temp is listed explicitly, Postgres searches it first for relation and type names, ahead of even an explicitly-listed pg_catalog. Both function bodies read pg_roles, so a caller holding TEMPORARY on the database could shadow it with a temp view and choose which branch the function takes. https://www.postgresql.org/docs/14/sql-createfunction.html#SQL-CREATEFUNCTION-SECURITY https://www.postgresql.org/docs/14/runtime-config-client.html#GUC-SEARCH-PATH
This is simpler and safer than altering existing roles
9d32210 to
99f0b46
Compare
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 (This PR)
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 (#6963)
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.