Repository navigation
fix(deploy): no install can connect to another install's database - #183
Merged
Merged
Conversation
Several installs share one PostgreSQL cluster, one role and one database (plus <database>_drill) each. PostgreSQL gives CONNECT on every new database to PUBLIC, and 05 never took it back, so install A's role could open install B's databases; only the schema permissions stopped it from reading. deploy/05-connect-isolation.sql holds the two statements (REVOKE CONNECT ... FROM PUBLIC; GRANT CONNECT ... TO <role>) with psql variables. 05 runs it for the database and the drill database of every install, as the postgres superuser, with the file on stdin (read by root, so the postgres user needs no access to the checkout). It is safe to repeat. No new role, no pg_read_all_data (RESEARCH.md 5.8, ADR-028 item 7). Tests (tests/postgres): - test_deploy_05_isolation.py runs the REAL 05 as root in a throwaway postgres:16 container for two installs, twice. Deny: neither role opens the other's database or drill database. Allow: each opens its own two and still writes and reads in its own app and observability schemas. ACLs give PUBLIC no CONNECT. No role besides the two, no predefined role. The SQL file closes an install made by the old 05. Skips without Docker. - test_connect_isolation.py applies the same SQL file with psql on the DATABASE_URL server to two installs set up the 05 way; same deny/allow, and a second run changes no ACL. Skips without a superuser or psql. Both were red at 01183a6 (the old 05 let alpha into beta's databases). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nstall SECURITY.md gets a "Database Isolation Between Installs" section: only the install's own role may connect to its database and its drill database; never pg_read_all_data; the tests that prove it. It says plainly that the rule is not applied on the production server yet. For an existing install, the safe path is the two SQL lines per database (deploy/05-connect-isolation.sql, or by hand): no new password, no data change, and an open session keeps working (checked on postgres:16: the open session still answered, a new one got "User does not have CONNECT privilege"). It also says that re-running 05 resets the role's password, so DATABASE_URL in .env must be updated and the app restarted (SPEC-F Amendment 1). One line in the runbook's "Existing installs". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Human review: pending. Not applied on the production server. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Every 05 test started from databases the new 05 created. So a 05 that isolated only the databases it creates would pass them all, and the real upgrade path (an install made before this unit) was never tested. The new case makes two installs with the old script (a byte-identical copy of 01183a6's deploy/05-create-databases.sh, kept next to the test because CI checks out one commit), checks the hole is open, runs the new 05 over them, and asserts neither role can open the other's database or drill database. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The REVOKE stops new connections only, so a session opened before it stays open. SECURITY.md now says to run the pg_stat_activity check again after the two lines, and if a row is left, to restart the install that owns that role or end the sessions with pg_terminate_backend. The check leaves out the postgres superuser, which may always connect, so a row means a role the rule locks out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The postgres:16 entrypoint first starts a short init server that listens on the unix socket only, then stops it and starts the real one. The old poll (psql over the socket) could pass on that init server, so 05 could start while no server was up, and the test went red for no reason. pg_isready -h 127.0.0.1 only passes on the real server. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main moved to f83e7f0 (#180 off-site SFTP panel, #181 backup retention pools, #182 recovery objectives). One conflict, in docs/engineering/AI_ASSISTANCE_LOG.md: both sides added entries at the top. Kept both: the F entry first, unchanged, then main's H2 and H1 entries, each with its own review line. deploy/README.md and docs/engineering/DEPLOYMENT_RUNBOOK.md merged on their own; each is main plus the one F line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On the manual path the operator types role= by hand. With a typo the GRANT failed but the REVOKE before it was already committed, so PUBLIC lost CONNECT and the named role never got it. The two statements now run in one transaction, so a failed GRANT takes the REVOKE back too. The new test applies the file with a role that does not exist and checks the database ACL is the same before and after. It was red without the transaction. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ill db The four by-hand lines now run between BEGIN and COMMIT, like the SQL file. If psql answers ROLLBACK, a name was wrong and nothing changed. An install made before the restore drill has no _drill database: leave it out, because the next 05 run creates it with the rule already on. Inside one transaction a missing drill database would roll back the lines for the main database too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root cause
deploy/05-create-databases.shcreates one role and one database per install on ONE shared PostgreSQL cluster, butnever revoked
CONNECTfromPUBLIC. So install A's role could connect to install B's database (and B's drilldatabase); only schema permissions stopped it from reading. The db-maturity spike reproduced that a broader read role
would read the other install's visitor phone numbers. One layer of defense between installs was missing.
Part of Daneshbonyan re-evaluation track 7 (evaluator reason 4: database maturity).
Design doc
docs/features/db-maturity/RESEARCH.md sections 5.8 and 6 item 8 (work row F), ADR-028 in docs/engineering/DECISIONS.md.
What changed:
deploy/05-connect-isolation.sql:REVOKE CONNECT ON DATABASE ... FROM PUBLICandGRANT CONNECT ... TO <that install's role>, inside one transaction, with psql:"var"quoted identifiers. It fails closed if a variable ismissing.
deploy/05-create-databases.shchecks the file exists before creating anything and applies it, as the superuserwith
ON_ERROR_STOP, topadyar_<slug>andpadyar_<slug>_drillon every run. Old installs are covered on the nextrun; re-runs change nothing. No new role, no
pg_read_all_data, no membership.docs/engineering/SECURITY.md: the rule; how to apply it on an existing server with the two SQL lines per database(the safe path, because re-running 05 also resets the app password); open sessions stay connected until they end
(what to do); installs without a drill database yet; and that it is NOT applied on the production server yet.
deploy/README.mdand the runbook.Tests
(
test_connect_isolation_oracle.py, not committed; runs the REAL 05 as root in a container): 28 failed / 26 passedon the base; 54/54 on the first patch.
tests/postgres/test_deploy_05_isolation.pyruns the real 05 in a throwaway postgres:16 container for twoinstalls, twice, and also runs the OLD 05 (an exact copy of the script at 01183a6) followed by the new one: role A
cannot connect to B or B_drill, every install keeps its own access.
tests/postgres/test_connect_isolation.pyapplies the same SQL file over DATABASE_URL. Both skip cleanly without Docker / psql / a superuser.
Security
Removes a cross-install connect path. The app and the metrics exporter need no CONNECT on another install's database
(checked by the final reviewer). Identifiers in the SQL are quoted psql variables, so a slug cannot inject SQL. The
manual path runs in one transaction, so a mistyped role cannot leave a database without its own role's CONNECT.
AI assistance
Written by Claude agents in a Herdr team: implementer
ops-t1-impl(Opus 5.5) wrote the SQL, script change, tests anddocs; reviewer
ops-t1-rev(Opus 5.5) wrote the oracle first and reviewed (accepted; its low, a missing old-installtest, was fixed and shown red against a mutation); final reviewer
drill-t1-final(Opus 5.5, xhigh) reviewed the wholediff (accepted; its low, the missing transaction, was fixed); lead
foreman7(Opus 5.5) re-ran the oracle on the baseand the patch in its own container. Log entry in
docs/engineering/AI_ASSISTANCE_LOG.md.Human review