t11 execute store: own database + own role per domain, rbac/approvals/replay-store schema - #6
Merged
Merged
Conversation
…/replay-store schema, second alembic chain the boundary now holds at the data layer, not just in import-linter: a compromised recommend domain holds no credential that can open a connection to the execute database where tokens, approvals and grants live (postgres has no cross-database queries, so that ends it). - docker/postgres-init/10-domains.sql: one database per domain, each owned by its own non-superuser role, CONNECT revoked from PUBLIC. mounted by compose AND read by the isolation test, so the test asserts the deployed config - recommend's compose DATABASE_URL moves off the superuser to hodlin_recommend (the isolation is only real if the app isn't admin) - execute store: 9 tables in three groups - rbac (operators/roles/permissions + joins), authorization (proposals with the hash WE compute, approvals, auth_tokens as the replay store), and tx_attempts written before broadcast. wei as numeric(78,0) - integer, exact, never float - partial unique index on auth_tokens(proposal_hash) where consumed_at is null: "approved twice, got two spendable tokens" is unrepresentable in the schema - own alembic chain with an explicit alembic_version_execute, so both chains coexist if ever pointed at one database - EXECUTE_ env prefix as a boundary: leaking the recommend domain's bare DATABASE_URL into this process configures nothing (asserted) verified against real postgres in the deployed shape: init scripts create both databases/roles, recommend migrates as its non-superuser role, execute migrates as its own, and both cross-domain connects fail with "does not have CONNECT privilege". gate green, 127 passed.
…chain_id, unambiguous fk names the two mediums both said the same thing: the postgres-level isolation was real, but the layers around it gave the credential back. - .env split per domain (.env.execute for the execute side). compose injects EVERY key of an env_file, so a shared .env put EXECUTE_DATABASE_URL straight into the recommend container - a compromised recommend process just reads it out of os.environ, no sql injection needed. verified with `docker compose config`: the app service now resolves 16 keys, none of them EXECUTE_*, and re-adding the variable to .env shows it would still be injected (so the file split is the mechanism, not the prefix) - 10-domains.sql is genuinely idempotent: CREATE DATABASE (no IF NOT EXISTS, can't sit in a DO block) is generated only for missing databases and \gexec'd. the old file aborted under the entrypoint's ON_ERROR_STOP=1 *before* the REVOKE CONNECT ... FROM PUBLIC, i.e. it failed OPEN on any re-run or pre-created database. verified in the real image: second `psql -f` under ON_ERROR_STOP=1 exits 0, PUBLIC still has no CONNECT on either database - the isolation test no longer swallows DuplicateDatabaseError - it applies the file twice and asserts the revoke stuck, which is what that claim was worth - its statement splitter now reads the file the way psql does (char-level quote and dollar-quote tracking, inline -- comments stripped, \gexec honoured, unterminated text raises). before, a trailing comment silently dropped or merged a statement - poor property for the one test whose value is "asserts the deployed config". pinned with a unit test - alembic env (both domains): the url goes to create_async_engine directly, not through config.set_main_option, where ConfigParser treats % as interpolation. verified against real postgres with password p%ss: old code raised "invalid interpolation syntax", new code migrates - chain_id int32 -> int64: EIP-155 ids exceed 2**31 (palm is 11297108109) and the overflow would land on the pre-broadcast intent row, the one write that must not fail - approvals/tx_attempts.proposal_id -> proposal_row_id: it holds the surrogate proposals.id while proposals.proposal_id is the contract uuid. one name for two things type-checks and fails at INSERT, mid-approval - new drift test: alembic's own compare_metadata against ExecuteBase.metadata must be empty, so a rename landing on one side only fails the suite instead of passing a table-names check - docstrings that overstated the EXECUTE_ prefix now say what it actually buys (no accidental binding) vs what the file split buys (no co-located secrets), and compose's documented test url points at the admin database - aimed at hodlin_recommend it would have deposited auth_tokens and approvals there gate green, 131 passed.
…rap connect, dollar-tag-aware splitter the interesting one is a bug in the previous round's fix: guarding CREATE DATABASE with \gexec means a database that already exists never gets its OWNER set, and in pg15+ `public` belongs to pg_database_owner - so the role could CONNECT and still fail every migration with "permission denied for schema public". provisioning that looks complete and isn't, in the exact scenario the file's own comment claimed to cover (databases pre-created by IaC). - ALTER DATABASE ... OWNER TO, unconditional and idempotent, after the \gexec. verified in the real image: hand hodlin_execute to the admin, reapply, owner is back to hodlin_execute and CREATE TABLE as that role works - new test for it, since the suite structurally couldn't see this: the fixture's own first application creates the databases correctly, so the test now hands the database to the admin role and reapplies - REVOKE CONNECT on the compose bootstrap database `hodlin` too - both domain roles could connect there, and the integration suite creates tables in it. generated conditionally (testcontainers names its database `test`, and a REVOKE on a missing database would abort the file). postgres/template1 left alone on purpose: template acls are copied into every future database - splitter matches the dollar-quote TAG, not just `$$`: `DO $do$ ... $do$` is what psql needs once a block nests, and a `$$` inside such a body must not end it. also stops a `$$` inside a string literal from toggling. unterminated block now asserts instead of vanishing - auth_tokens docstring says what the partial unique index does NOT cover, both landing on T13/T14's mint path: "live" means unconsumed, not unexpired (the predicate can't reference now(), so minting must supersede an expired row or the proposal becomes permanently unapprovable), and the key is the hash, so two byte-identical proposals share a slot and the violation has to become a domain refusal rather than an escaping IntegrityError - approvals.proposal_row_id CASCADE -> RESTRICT, matching tx_attempts: those rows are the human-decision audit trail, refusals included, so deleting a proposal must fail loudly rather than quietly take the record with it - .env.execute.example no longer claims alembic reads it - the migration env reads the environment only, so the export step is now spelled out - the one migration test without cleanup now drops this chain's tables on failure (not a retry of the failing downgrade, which would bury the original traceback) gate green, 133 passed.
…ed name the previous commit revoked PUBLIC's connect on the bootstrap database behind a `WHERE EXISTS (... datname = 'hodlin')` guard - which means renaming POSTGRES_DB (or running the file against a differently-named cluster, e.g. testcontainers' `test`) silently skips the revoke. failing open, which is the direction this file exists to avoid. current_database() is the database the script is connected to by definition, so it needs no guard and can't be missed. verified with POSTGRES_DB=some_other_name: the renamed database ends with public_connect=false and hodlin_recommend gets "does not have CONNECT privilege" trying to reach it. the isolation test now asserts the property on that database too, not just the two domain ones. gate green, 133 passed.
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
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.
First M2 task. The execute domain gets its own database, its own non-superuser role, and its own alembic chain — so the boundary that import-linter enforces at build time also holds at the data layer.
why a second database, not a second schema
A compromised recommend domain holds no credential that can open a connection to the execute database where tokens, approvals and grants live. Postgres has no cross-database queries, so that ends it — there is no
SET search_pathor forgottenGRANTthat walks back across. One SQL injection in recommend must not read every live token.what
docker/postgres-init/10-domains.sql— one database per domain, each owned by its own non-superuser role,CONNECTrevoked fromPUBLIC. Mounted by compose and read by the isolation test, so the test asserts the config that actually deploys rather than a copy of it.docker-compose.yml— recommend'sDATABASE_URLmoves off the superuser ontohodlin_recommend. The isolation is only real if the app isn't admin.store/tables.py— 9 tables in three groups: rbac (operators/roles/permissions + joins), authorization (proposals carrying the hash we compute, approvals,auth_tokensas the replay store), andtx_attempts, written before broadcast. Wei isnumeric(78,0)— integer, exact, never float.auth_tokens(proposal_hash) WHERE consumed_at IS NULL— "approved twice, got two spendable tokens" is unrepresentable in the schema, not merely rejected by application code.store/migrations/— own chain with an explicitalembic_version_execute, so both chains coexist if ever pointed at one database.config.py—EXECUTE_env prefix as a boundary: leaking the recommend domain's bareDATABASE_URLinto this process configures nothing (asserted).verified against real postgres, in the deployed shape
Init scripts create both databases and roles; recommend migrates as its own non-superuser role; execute migrates as its own; both cross-domain connects fail with
does not have CONNECT privilege. Gate green — ruff, mypy strict, import-linter, pytest (127 passed).review round (7288e61)
A high-effort subagent review confirmed the good parts independently — it ran the new chain against a real
postgres:18-alpineand diffed the result againstExecuteBase.metadatawith Alembic'scompare_metadata(clean, and the hand-written0001reverses), and confirmedPUBLICends with noCONNECTon either domain database. It also found that the layers around the per-role isolation were handing the credential back. Both mediums and all six should-fixes are now applied:appservice usesenv_file: - .env, and Compose injects every key an env file contains — soEXECUTE_DATABASE_URLliving in that shared file put the execute domain's database credential straight intoos.environof the process that is assumed compromised. No SQL injection required. Env files are now per domain (.envrecommend,.env.executeexecute, each with its own committed template);config.pyreads.env.execute. Verified withdocker compose config: the app service resolves 16 keys, noneEXECUTE_*— and re-adding the variable to.envshows it would still be injected, which is the proof that the file split is the mechanism and the prefix is not.10-domains.sqlfailed open on reapplication.CREATE DATABASEhas noIF NOT EXISTSand cannot sit in aDOblock, so it aborted the script under the entrypoint'sON_ERROR_STOP=1— before theREVOKE CONNECT … FROM PUBLIC, leaving PUBLIC with default CONNECT on any re-run or IaC-precreated cluster. The statement is now generated only for missing databases and\gexec'd, so every application reaches the grants. Verified in the real image: a secondpsql -f … -v ON_ERROR_STOP=1exits 0 (DO / REVOKE / REVOKE / GRANT / GRANT) andhas_database_privilege('public', …)is stillffor both.DuplicateDatabaseError; it applies the file twice and then asserts the revoke stuck — the assertion that claim was always supposed to carry.--comments stripped,\gexechonoured, anything unterminated raising instead of vanishing. Previously a trailing comment silently dropped or merged a statement, a poor property for the one test whose value is "asserts the deployed configuration". Pinned with a unit test.create_async_enginedirectly instead of throughconfig.set_main_option, where ConfigParser reads%as interpolation syntax. Verified against real Postgres with the passwordp%ss: the old code raisedinvalid interpolation syntax, the new code migrates. (Pre-existing in recommend'senv.py, fixed there too rather than left as a known crash in the sibling.)chain_idint32 → int64. EIP-155 ids exceed 2^31 (Palm is 11297108109), and the overflow would land on the pre-broadcast intent row — the one write that must not fail.approvals/tx_attempts.proposal_id→proposal_row_id. It holds the surrogateproposals.idwhileproposals.proposal_idis the contract's UUID; one name for two things type-checks and fails at INSERT, mid-approval.compare_metadataagainstExecuteBase.metadatamust come back empty, so a rename landing on one side only fails the suite instead of passing.EXECUTE_prefix now separate what it buys (no accidental binding of the other domain'sDATABASE_URL) from what the file split buys (no co-located secrets). Compose's documented test URL points at the admin database — aimed athodlin_recommend, the execute chain would have depositedauth_tokensandapprovalsinside the recommend domain's database.Cleared as a non-bug:
except A, B:in the isolation test is valid PEP 758 on 3.14.Gate green after the fixes: ruff, mypy strict, import-linter, pytest — 131 passed (127 + the splitter unit test, the drift test, and two env-separation tests).
second review round (6d609cb)
A second high-effort review over the fixed branch found eight more, including a bug in the previous round's own fix:
\gexecguard skippedOWNERfor a database that already exists — and in PG15+publicbelongs topg_database_owner, so the role couldCONNECTand still fail every migration withpermission denied for schema public. Provisioning that looks complete and isn't, in exactly the IaC-precreated scenario the file's comment claimed to cover. Fixed with an unconditional, idempotentALTER DATABASE … OWNER TO. Verified in the real image: handhodlin_executeto the admin role, reapply, ownership is restored andCREATE TABLEas that role succeeds. The suite structurally couldn't catch this — its own first application creates the databases correctly — so the new test hands the database away and reapplies.PUBLICkeptCONNECTon the compose bootstrap databasehodlin. Both domain roles could connect there, and the integration suite creates tables in it — catalog exposure, not a data leak, but on the exact property this file asserts. Now revoked, generated conditionally so a cluster that names its database something else (testcontainers usestest) doesn't abort the file.postgres/template1deliberately untouched: template ACLs are copied into every future database.DO $do$ … $do$is what psql needs as soon as a block nests, and a$$inside such a body must not end it; a$$inside a string literal no longer toggles either. An unterminated block asserts instead of vanishing.AuthTokenbecause both land on T13/T14's mint path: "live" means unconsumed, not unexpired — the predicate can't referencenow(), so minting has to supersede an expired row or a proposal whose token expired unused becomes permanently unapprovable; and the key is the hash, so two byte-identical proposals share a slot and that violation must become a domain-level refusal rather than anIntegrityErrorescaping the store.approvals.proposal_row_idCASCADE → RESTRICT, matchingtx_attempts. Those rows are the human-decision audit trail, refusals included; deleting a proposal should fail loudly rather than quietly take the record of who decided what..env.execute.exampleno longer claims Alembic reads it — the migration env reads the environment only (by design, so migrations need none of the domain's other secrets), so the export step is spelled out instead of implied.DuplicateTablein the next test. Not a retry of the failing command, which would bury the original traceback.Gate green: 133 passed (+2 for the ownership and dollar-tag tests).