Skip to content

Commit 092de78

Browse files
valvesssclaude
andauthored
HAULDR-4: desarmar o trigger de auditoria e ensinar o cutover a atravessá-lo (#32)
O trigger de DDL da 0004 é SECURITY INVOKER: grava o ddl_log como quem emitiu a DDL — o que é o ponto (o `actor` tem de ser o autor real, e por isso SECURITY DEFINER não serve). O custo é a escrita ficar sujeita ao privilégio do invocante, e a 0004 não concede nenhum. No pg16 isso ficou dormente porque o autor da DDL é `postgres`, superuser lá, que passa por cima da ACL. Duas situações não têm essa cobertura: um app que é DONO das suas tabelas e migra como `<app>_authenticator` (heimdall, brokk — os dois que caíram), e QUALQUER projeto no supabase/postgres 17, onde `postgres` é deprivilegiado. Nesses, DDL no boot morre com 42501 e o container crashloopa. - migrations/0005: os três grants (schema + tabela + sequence — `id` é bigserial, faltar um dá um 42501 DIFERENTE de cada vez). Migration nova, não edição da 0004: as aplicadas nunca re-rodam, e o applyMigrations do provision e do sampler propaga esta pela frota. - cutover.ts: o stageA abortava em TODO app com auditoria. pg_dump sempre emite os event triggers (são globais do banco — `-N hauldr_audit` NÃO os exclui, e usá-lo sozinho piora: sobrariam a apontar para funções ausentes), e no v17 um event trigger superuser-owned não pode executar função de não-superuser. O pg_restore falhava essas 4 entradas, restaurava todo o resto e ainda assim saía != 0 — abortando um stageA cujos DADOS tinham chegado intactos. dumpRestore passa a filtrar o TOC (-L, por isso ficheiro em vez de pipe) e repairAudit reconstrói os triggers no destino, re-owned para o superuser do cluster e com os grants. Provado ao vivo: `cli cutover zztest --stage a` de origem ARMADA termina exit 0 (antes abortava no passo 4); destino fica com paridade de linhas, triggers owned by supabase_admin e os três grants. O vetor exato do Heimdall — ALTER TABLE ADD COLUMN IF NOT EXISTS de coluna existente, que o PG pula mas o trigger regista — passa no v17 como `postgres` não-superuser, auditado com o ator real. Os 29 bancos do pg16 + 3 do v17 já foram desarmados à mão (GRANT é idempotente; a 0005 reconcilia). Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 7eaf05b commit 092de78

2 files changed

Lines changed: 140 additions & 40 deletions

File tree

control-plane/src/cutover.ts

Lines changed: 110 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
11
import { spawn } from "node:child_process";
2+
import { mkdtemp, rm, writeFile } from "node:fs/promises";
3+
import { tmpdir } from "node:os";
4+
import { join } from "node:path";
25
import { controlPool } from "./db";
36
import { config } from "./config";
47
import {
@@ -52,61 +55,122 @@ function connOf(url: string): Conn {
5255
};
5356
}
5457

55-
/** Pipe `pg_dump` (source) → `pg_restore` (target), resolving on clean exit. */
56-
function dumpRestore(src: Conn, dst: Conn, database: string): Promise<void> {
58+
/** Run a pg CLI to completion; resolves with stdout, rejects on non-zero exit. */
59+
function runPg(cmd: string, args: string[], password: string): Promise<string> {
5760
return new Promise((resolve, reject) => {
61+
const p = spawn(cmd, args, { env: { ...process.env, PGPASSWORD: password } });
62+
let out = "";
63+
let err = "";
64+
p.stdout.on("data", (b) => (out += b.toString()));
65+
p.stderr.on("data", (b) => (err += b.toString()));
66+
p.on("error", (e) => reject(new Error(`${cmd} spawn failed: ${e.message}`)));
67+
p.on("close", (code) =>
68+
code === 0
69+
? resolve(out)
70+
: reject(new Error(`${cmd} exited ${code}: ${err.slice(-2000)}`)),
71+
);
72+
});
73+
}
74+
75+
/**
76+
* `pg_dump` (source) → `pg_restore` (target), through a temp file.
77+
*
78+
* Deliberately NOT a pipe: the restore runs from a filtered TOC (`-L`), and
79+
* pg_restore cannot seek a stream. The filter drops the DDL-audit event triggers
80+
* (migrations/0004), which cannot survive the crossing as dumped:
81+
* - pg_dump ALWAYS emits them. Event triggers are database-global, so
82+
* `-N hauldr_audit` excludes the schema's functions but not the triggers
83+
* that call them — excluding the schema alone makes it worse, not better.
84+
* - their functions arrive owned by `postgres`, and on the supabase image
85+
* `postgres` is not a superuser. A superuser-owned event trigger may not
86+
* execute a non-superuser-owned function, so the CREATE fails.
87+
* pg_restore is best-effort: it fails those four TOC entries, restores
88+
* everything else, and still exits non-zero — which used to abort a stageA whose
89+
* DATA had in fact landed intact. Skipping them here and rebuilding them in
90+
* `repairAudit` keeps the exit code meaningful: it now means a real failure.
91+
*/
92+
async function dumpRestore(src: Conn, dst: Conn, database: string): Promise<void> {
93+
const dir = await mkdtemp(join(tmpdir(), `cutover-${database}-`));
94+
const archive = join(dir, "archive.dump");
95+
const toc = join(dir, "restore.toc");
96+
try {
5897
// -Fc custom format; -N realtime EXCLUDES the service-managed realtime schema
5998
// (Realtime rebuilds it on tenant register; carrying it over causes
6099
// `SET log_min_messages` permission errors + supabase_realtime_admin grant
61-
// failures on the v17 restore). Plain restore into the v17 db owned by postgres.
62-
const dump = spawn(
100+
// failures on the v17 restore).
101+
await runPg(
63102
"pg_dump",
64103
[
65104
"-h", src.host, "-p", src.port, "-U", src.user,
66-
"-Fc", "-N", "realtime", database,
105+
"-Fc", "-N", "realtime", "-f", archive, database,
67106
],
68-
{ env: { ...process.env, PGPASSWORD: src.password } },
107+
src.password,
69108
);
70-
const restore = spawn(
109+
110+
// TOC line shape: `<dumpId>; <tableoid> <oid> <desc> <schema> <name> <owner>`.
111+
// Anchor on the `desc` field so a table whose NAME merely contains the words
112+
// is not silently dropped from the restore.
113+
const listed = await runPg("pg_restore", ["-l", archive], dst.password);
114+
const kept = listed
115+
.split("\n")
116+
.filter((l) => !/^\s*\d+;\s+\d+\s+\d+\s+EVENT TRIGGER\b/.test(l));
117+
await writeFile(toc, kept.join("\n"));
118+
119+
await runPg(
71120
"pg_restore",
72121
[
73122
"-h", dst.host, "-p", dst.port, "-U", dst.user,
74-
"-d", database,
123+
"-d", database, "-L", toc, archive,
75124
],
76-
{ env: { ...process.env, PGPASSWORD: dst.password } },
125+
dst.password,
77126
);
127+
} finally {
128+
await rm(dir, { recursive: true, force: true });
129+
}
130+
}
78131

79-
let dumpErr = "";
80-
let restoreErr = "";
81-
dump.stderr.on("data", (b) => (dumpErr += b.toString()));
82-
restore.stderr.on("data", (b) => (restoreErr += b.toString()));
83-
84-
dump.stdout.pipe(restore.stdin);
85-
86-
let dumpDone = false;
87-
let restoreDone = false;
88-
let failed = false;
89-
const fail = (msg: string) => {
90-
if (failed) return;
91-
failed = true;
92-
reject(new Error(msg));
93-
};
132+
/**
133+
* Rebuild the DDL-audit event triggers on the target cluster (see dumpRestore for
134+
* why they are not restored), and leave the ledger writable by the app roles.
135+
*
136+
* Re-owning the functions is what legalises the triggers: created by this
137+
* cluster's admin (a superuser), they may only execute a superuser-owned
138+
* function, and the restore left them owned by the deprivileged `postgres`.
139+
*
140+
* The grants mirror migrations/0005 and are re-applied because they are a
141+
* PRECONDITION of the move, not a nicety: the trigger writes `ddl_log` as
142+
* whoever emitted the DDL, and on pg17 that is a `postgres` which no longer
143+
* bypasses the ACL — the first DDL-on-boot (GoTrue, storage-api) would die 42501
144+
* and crashloop the sidecar. Applied here too so a project whose db predates
145+
* 0005 still lands on v17 disarmed.
146+
*/
147+
async function repairAudit(cluster: Cluster, database: string): Promise<void> {
148+
const c = dbClientForCluster(cluster, database);
149+
await c.connect();
150+
try {
151+
const { rowCount } = await c.query(
152+
"select 1 from pg_namespace where nspname = 'hauldr_audit'",
153+
);
154+
if (!rowCount) return; // db predates migrations/0004 — nothing to rebuild
94155

95-
dump.on("error", (e) => fail(`pg_dump spawn failed: ${e.message}`));
96-
restore.on("error", (e) => fail(`pg_restore spawn failed: ${e.message}`));
97-
dump.on("close", (code) => {
98-
dumpDone = true;
99-
if (code !== 0) return fail(`pg_dump exited ${code}: ${dumpErr.slice(-2000)}`);
100-
if (restoreDone && !failed) resolve();
101-
});
102-
restore.on("close", (code) => {
103-
restoreDone = true;
104-
// pg_restore returns non-zero on ANY error; surface its stderr. (It emits
105-
// warnings on stderr too, but a non-zero exit is a real failure here.)
106-
if (code !== 0) return fail(`pg_restore exited ${code}: ${restoreErr.slice(-2000)}`);
107-
if (dumpDone && !failed) resolve();
108-
});
109-
});
156+
await c.query("alter function hauldr_audit.on_ddl_end() owner to current_user");
157+
await c.query("alter function hauldr_audit.on_ddl_drop() owner to current_user");
158+
await c.query("drop event trigger if exists hauldr_audit_ddl_end");
159+
await c.query(
160+
`create event trigger hauldr_audit_ddl_end on ddl_command_end
161+
execute function hauldr_audit.on_ddl_end()`,
162+
);
163+
await c.query("drop event trigger if exists hauldr_audit_ddl_drop");
164+
await c.query(
165+
`create event trigger hauldr_audit_ddl_drop on sql_drop
166+
execute function hauldr_audit.on_ddl_drop()`,
167+
);
168+
await c.query("grant usage on schema hauldr_audit to public");
169+
await c.query("grant insert on hauldr_audit.ddl_log to public");
170+
await c.query("grant usage on sequence hauldr_audit.ddl_log_id_seq to public");
171+
} finally {
172+
await c.end();
173+
}
110174
}
111175

112176
/** Per-table row counts for schemas public + auth, keyed `schema.table`. */
@@ -211,10 +275,16 @@ export async function stageA(name: string): Promise<void> {
211275
await v17admin.end();
212276
}
213277

214-
// 4. Data move: pg_dump (pg16) → pg_restore (pg17), excluding `realtime`.
278+
// 4. Data move: pg_dump (pg16) → pg_restore (pg17), excluding `realtime` and
279+
// the DDL-audit event triggers (rebuilt in 5 — see dumpRestore).
215280
console.log(`[cutover ${name}] stageA 4/7 pg_dump → pg_restore (excluding realtime)`);
216281
await dumpRestore(src, dst, database);
217282

283+
// 4b. Rebuild the audit event triggers the restore had to skip, owned by this
284+
// cluster's superuser + granted, so DDL-on-boot does not 42501 on v17.
285+
console.log(`[cutover ${name}] stageA 4b/7 rebuild DDL-audit event triggers on pg17`);
286+
await repairAudit("pg17", database);
287+
218288
// 5. Apply the v17 compat baseline as supabase_admin: default extensions, the
219289
// Supabase-dialect auth helpers (reused verbatim from gotrue.ts), and the
220290
// GoTrue search_path crash-loop fix.

migrations/0005_audit_grants.sql

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
-- Let the DDL audit (0004) record DDL emitted by non-superusers.
2+
--
3+
-- 0004's event triggers are SECURITY INVOKER: the ledger row is written as
4+
-- whoever emitted the DDL. That is the point — `actor` must be the real author,
5+
-- so SECURITY DEFINER is NOT the alternative (it would log the function owner
6+
-- every time and defeat the audit). The cost is that the write is subject to the
7+
-- invoker's privileges, and the ledger ships with none granted.
8+
--
9+
-- On the pg16 cluster the DDL author is `postgres`, a superuser there, which
10+
-- bypasses the ACL — which is why the ungranted ledger has worked so far. Two
11+
-- cases do not have that cover:
12+
-- - an app that OWNS its tables and migrates as `<app>_authenticator`
13+
-- (heimdall, brokk) — never a superuser, even on pg16;
14+
-- - ANY project on supabase/postgres 17, where `postgres` is deprivileged.
15+
-- In both, DDL-on-boot (GoTrue, storage-api, ensureSchema) dies with `42501
16+
-- permission denied` and the container crashloops. This is a prerequisite of the
17+
-- pg16 → pg17 cutover, not a nicety.
18+
--
19+
-- All three grants are required, not just the INSERT: `ddl_log.id` is a
20+
-- bigserial, so the insert touches the schema, the table AND the sequence. A
21+
-- missing one fails with a DIFFERENT 42501 each time (schema → table →
22+
-- sequence), which reads like progress but is only the next lock.
23+
--
24+
-- PUBLIC is the correct scope: the trigger fires for every role that can emit
25+
-- DDL, so every such role must be able to append. It stays append-only for them
26+
-- — no select, no update, no delete.
27+
28+
grant usage on schema hauldr_audit to public;
29+
grant insert on hauldr_audit.ddl_log to public;
30+
grant usage on sequence hauldr_audit.ddl_log_id_seq to public;

0 commit comments

Comments
 (0)