From 6db14a2205f760075587e1f3333c64418c138853 Mon Sep 17 00:00:00 2001 From: lilleman Date: Wed, 19 Aug 2026 00:20:17 +0200 Subject: [PATCH] Isolate the storage CI stack, prove least-privilege provisioning, drop the secret before discovery --- AGENTS.md | 11 +++++--- ci.sh | 17 ++++++++---- src/auth/bootstrap.ts | 5 ++++ src/config.test.ts | 18 ++++++++++++- src/plugin-host/storage-provisioning.ts | 11 +++++--- src/plugin-host/storage.test.ts | 35 +++++++++++++++++++++++++ src/plugin-host/storage.ts | 11 +++++++- src/server.ts | 8 +++--- 8 files changed, 99 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a244c2c..919bb56 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -97,9 +97,14 @@ Revisit only if the stated reason stops holding. *accidents*, not hostile plugins: `PLUGIN_DB_SECRET` is in `web`'s environment during `onBoot`, and a plugin already holds `ctx.system`'s Ory admin clients — so cross-plugin DB isolation is containment, and README says so rather than implying a sandbox. Consistent with priority #7 - (crash-isolation is a non-goal). `server.ts` still derives every credential *before* the boot hooks - and then deletes the secret from `process.env` — defence in depth, and the ordering is the whole - point, so don't move it. **Valid while plugins are operator-installed code, not third-party uploads.** + (crash-isolation is a non-goal). `server.ts` still deletes the secret from `process.env` right + after `loadConfig`, which is before discovery imports any plugin module — the ordering is the whole + point, so move it earlier if anything, **never later**. **Valid while plugins are + operator-installed code, not third-party uploads.** +- **`bootstrap.ts` stays under `src/auth/`** even though it now provisions plugin databases as well + as seeding Ory. It is the one-shot service's entrypoint, not an auth module; moving it to + `src/bootstrap.ts` would edit `compose.yml`, five e2e compose files and `src/compose.test.ts` for a + rename. Reconsider when a third seeding concern lands. - **`BootContext.storage` keeps all six credential fields, and there is no `onShutdown` hook.** While `HOST_API_VERSION` is frozen both are free to revisit; after the freeze, adding is compatible and removing is not, so the shape errs small elsewhere. Pools handed to a plugin are reaped on process diff --git a/ci.sh b/ci.sh index 41ab585..cdcabad 100755 --- a/ci.sh +++ b/ci.sh @@ -64,13 +64,20 @@ count=$(echo "$units" | grep -oE 'tests [0-9]+' | grep -oE '[0-9]+' | head -1 || # test skips there — and it is the only thing proving the DDL actually grants what it claims, rather # than that the SQL text is the text we wrote. `node --test` counts a skip, so the floor won't catch it. step "Plugin storage (real Postgres)" +# Own project name, like every E2E suite below: the default project is the DEV stack, so a bare +# `down -v` here would delete the operator's pgdata — Ory identities and every plugin database. +# --wait, because initdb on a cold volume outlasts the suite's connect timeout. storage_rc=0 -docker compose up -d postgres >/dev/null -docker compose run --rm --no-deps \ +storage_proj=plainpages-storage +docker compose -p "$storage_proj" up -d --wait postgres >/dev/null +storage_out=$(docker compose -p "$storage_proj" run --rm --no-deps \ -e PLUGIN_DB_ADMIN_URL=postgres://${POSTGRES_USER:-ory}:${POSTGRES_PASSWORD:-ory}@postgres:5432/ory \ - web node --test src/plugin-host/storage.test.ts || storage_rc=$? -docker compose down -v >/dev/null 2>&1 || true -[ "$storage_rc" -eq 0 ] || { echo "plugin storage integration tests failed (exit $storage_rc)"; exit "$storage_rc"; } + web node --test src/plugin-host/storage.test.ts 2>&1) || storage_rc=$? +docker compose -p "$storage_proj" down -v >/dev/null 2>&1 || true +echo "$storage_out" | grep -E '^. (tests|pass|fail|skipped) ' || true +[ "$storage_rc" -eq 0 ] || { echo "$storage_out"; echo "plugin storage integration tests failed (exit $storage_rc)"; exit "$storage_rc"; } +# A skip here exits 0 and proves nothing — the same trap the unit floor above guards against. +echo "$storage_out" | grep -qE '^. skipped 0$' || { echo "storage integration test skipped — PLUGIN_DB_ADMIN_URL not wired through"; exit 1; } # Run one E2E suite against its OWN named stack, then always tear it down (even on failure). The # per-suite project name keeps a flaky teardown from leaking containers/volumes into the next suite. diff --git a/src/auth/bootstrap.ts b/src/auth/bootstrap.ts index 0a96481..b18dd43 100644 --- a/src/auth/bootstrap.ts +++ b/src/auth/bootstrap.ts @@ -18,6 +18,10 @@ import { createLogger, runWithLog, tracedFetch, type Log } from "../logger.ts"; type Env = Record; +// Kept closed to PUBLIC on every boot, not just on a fresh volume — a plugin role would otherwise +// reach the auth plane's catalogs and connection slots (ory/postgres/init/init.sql seeds the same). +const ORY_DATABASES = ["hydra", "keto", "kratos"]; + // --- Pure payload builders (the Kratos/Keto request contracts) ----------------------- export function identityPayload(email: string, password: string) { @@ -175,6 +179,7 @@ async function provisionPluginStorage(env: Env, plugins: Plugin[], log: Log): Pr const result = await provisionStorage({ adminUrl, connectionLimit: resolvePluginDbConnectionLimit(env), + lockdownDatabases: ORY_DATABASES, pluginIds: ids, secret: resolvePluginDbSecret(env), }); diff --git a/src/config.test.ts b/src/config.test.ts index 74cf0df..62a00d5 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -1,6 +1,6 @@ import assert from "node:assert/strict"; import { test } from "node:test"; -import { loadConfig, resolvePluginDbSecret } from "./config.ts"; +import { loadConfig, resolvePluginDbConnectionLimit, resolvePluginDbSecret } from "./config.ts"; // Explicit secure-secret enforcement (no environment sniffing): secrets are the only // thing a hardened deploy must supply. @@ -30,6 +30,22 @@ test("bootstrap refuses a missing, empty or throwaway plugin storage secret when assert.equal(resolvePluginDbSecret({ ...hardened, PLUGIN_DB_SECRET: "a-real-secret" }), "a-real-secret"); }); +// buildCredentials overwrites the userinfo, so a pasted admin DSN would *work* — and leave a +// privileged password in the process that runs plugin code. Refusing it is the whole guard. +test("PLUGIN_DB_URL carrying credentials is refused, not silently overwritten", () => { + assert.throws(() => loadConfig({ PLUGIN_DB_URL: "postgres://root:hunter2@db:5432/ory" }), /no username or password/); + assert.throws(() => loadConfig({ PLUGIN_DB_URL: "postgres://root@db:5432" }), /no username or password/); + assert.equal(loadConfig({ PLUGIN_DB_URL: "postgres://db:5432" }).pluginDbUrl, "postgres://db:5432"); + assert.equal(loadConfig({}).pluginDbUrl, undefined); // unset ⇒ storage off +}); + +test("the per-role connection ceiling defaults to 10 and rejects nonsense", () => { + assert.equal(resolvePluginDbConnectionLimit({}), 10); + assert.equal(resolvePluginDbConnectionLimit({ PLUGIN_DB_CONNECTION_LIMIT: "25" }), 25); + assert.throws(() => resolvePluginDbConnectionLimit({ PLUGIN_DB_CONNECTION_LIMIT: "0" }), /positive integer/); + assert.throws(() => resolvePluginDbConnectionLimit({ PLUGIN_DB_CONNECTION_LIMIT: "ten" }), /positive integer/); +}); + test("loads dev defaults when the environment is empty", () => { const c = loadConfig({}); assert.equal(c.port, 3000); diff --git a/src/plugin-host/storage-provisioning.ts b/src/plugin-host/storage-provisioning.ts index 8c39f6e..ccf845a 100644 --- a/src/plugin-host/storage-provisioning.ts +++ b/src/plugin-host/storage-provisioning.ts @@ -2,11 +2,14 @@ // alone — the only process holding superuser credentials, which is why the driver stops here. import postgres from "postgres"; -import { derivePassword, NAME_PREFIX, provisionSql, storageName } from "./storage.ts"; +import { derivePassword, NAME_PREFIX, orphanNames, provisionSql, quoteIdentifier, storageName } from "./storage.ts"; export interface ProvisionOptions { adminUrl: string; // needs CREATEDB + CREATEROLE, not superuser connectionLimit: number; + // Databases to keep closed to PUBLIC on every run. init.sql seeds this for the Ory databases, but + // it runs once on an empty data dir — an existing volume would keep the default grant forever. + lockdownDatabases?: string[]; pluginIds: string[]; secret: string; } @@ -36,9 +39,11 @@ export async function provisionStorage(options: ProvisionOptions): Promise`SELECT datname FROM pg_database WHERE starts_with(datname, ${NAME_PREFIX})`; - const orphans = existing.map((row) => row.datname).filter((name) => !provisioned.includes(name)).sort(); - return { orphans, provisioned }; + return { orphans: orphanNames(existing.map((row) => row.datname), provisioned), provisioned }; } finally { await sql.end(); } diff --git a/src/plugin-host/storage.test.ts b/src/plugin-host/storage.test.ts index f0c78e5..b9eded9 100644 --- a/src/plugin-host/storage.test.ts +++ b/src/plugin-host/storage.test.ts @@ -10,6 +10,7 @@ import { derivePassword, isValidStoragePluginId, MAX_STORAGE_PLUGIN_ID, + orphanNames, provisionSql, quoteIdentifier, quoteLiteral, @@ -71,10 +72,17 @@ test("only the plugins that asked for storage are provisioned", () => { ); }); +test("an orphan is a plugin_ database no installed plugin claims", () => { + const existing = ["plugin_gone", "plugin_here", "kratos", "ory"]; + assert.deepEqual(orphanNames(existing, ["plugin_here"]), ["plugin_gone"]); // Ory's are not ours to report + assert.deepEqual(orphanNames(existing, ["plugin_here", "plugin_gone"]), []); +}); + test("provisioning creates the role and the database when neither exists", () => { const plan = { connectionLimit: 10, databaseExists: false, name: "plugin_things", password: "pw", roleExists: false }; assert.deepEqual(provisionSql(plan), [ `CREATE ROLE "plugin_things" ${ATTRIBUTES} PASSWORD 'pw'`, + `GRANT "plugin_things" TO CURRENT_USER`, // else a CREATEROLE (non-superuser) account cannot own it `CREATE DATABASE "plugin_things" OWNER "plugin_things"`, `REVOKE ALL ON DATABASE "plugin_things" FROM PUBLIC`, `GRANT ALL PRIVILEGES ON DATABASE "plugin_things" TO "plugin_things"`, @@ -163,3 +171,30 @@ test("provisions a database its plugin can use and a peer plugin cannot reach", await admin.end(); } }); + +// README tells an operator CREATEDB + CREATEROLE is enough and superuser is more than it needs. +// That is a promise about their production credentials, so prove it rather than assert it. +test("provisions through a CREATEDB + CREATEROLE account, without superuser", integration, async () => { + const pluginId = "storage-itest-lowpriv"; + const provisioner = "storage-itest-provisioner"; + const admin = postgres(ADMIN_URL, { connect_timeout: 10, max: 1, onnotice: () => {} }); + try { + await admin.unsafe(`DROP ROLE IF EXISTS ${quoteIdentifier(provisioner)}`); + await admin.unsafe(`CREATE ROLE ${quoteIdentifier(provisioner)} LOGIN CREATEDB CREATEROLE PASSWORD 'itest-provisioner'`); + const asProvisioner = new URL(ADMIN_URL); + asProvisioner.username = provisioner; + asProvisioner.password = "itest-provisioner"; + await provisionStorage({ adminUrl: asProvisioner.href, connectionLimit: 10, pluginIds: [pluginId], secret: SECRET }); + + const owner = buildCredentials(baseUrlOf(ADMIN_URL), pluginId, SECRET); + await queryAs(owner.url, "CREATE TABLE IF NOT EXISTS notes (body text)"); + const rows = (await queryAs(owner.url, "SELECT 1 AS ok")) as { ok: number }[]; + assert.deepEqual(rows.map((row) => row.ok), [1]); // the plugin owns and can use what it was given + } finally { + const name = quoteIdentifier(storageName(pluginId)); + await admin.unsafe(`DROP DATABASE IF EXISTS ${name} WITH (FORCE)`); + await admin.unsafe(`DROP ROLE IF EXISTS ${name}`); + await admin.unsafe(`DROP ROLE IF EXISTS ${quoteIdentifier(provisioner)}`); + await admin.end(); + } +}); diff --git a/src/plugin-host/storage.ts b/src/plugin-host/storage.ts index 485d0fe..b69bbab 100644 --- a/src/plugin-host/storage.ts +++ b/src/plugin-host/storage.ts @@ -58,6 +58,12 @@ export function quoteLiteral(value: string): string { return `'${value.replaceAll("'", "''")}'`; } +// A database this host provisioned once and no installed plugin claims any more. Nothing drops it, +// so naming it is the only way an operator finds it again. +export function orphanNames(existing: string[], provisioned: string[]): string[] { + return existing.filter((name) => name.startsWith(NAME_PREFIX) && !provisioned.includes(name)).sort(); +} + export interface ProvisionPlan { connectionLimit: number; databaseExists: boolean; @@ -78,7 +84,10 @@ export function provisionSql(plan: ProvisionPlan): string[] { const attributes = `LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE CONNECTION LIMIT ${plan.connectionLimit} PASSWORD ${quoteLiteral(plan.password)}`; return [ plan.roleExists ? `ALTER ROLE ${identifier} WITH ${attributes}` : `CREATE ROLE ${identifier} ${attributes}`, - ...(plan.databaseExists ? [] : [`CREATE DATABASE ${identifier} OWNER ${identifier}`]), + // CREATE DATABASE ... OWNER needs SET ROLE on the owner, and PG16+ gives a CREATEROLE account + // ADMIN but *not* SET on the roles it creates — so it grants itself membership first. A + // superuser could skip this; issuing it anyway is what keeps a least-privilege account working. + ...(plan.databaseExists ? [] : [`GRANT ${identifier} TO CURRENT_USER`, `CREATE DATABASE ${identifier} OWNER ${identifier}`]), `REVOKE ALL ON DATABASE ${identifier} FROM PUBLIC`, `GRANT ALL PRIVILEGES ON DATABASE ${identifier} TO ${identifier}`, ]; diff --git a/src/server.ts b/src/server.ts index ea73d98..e693de0 100644 --- a/src/server.ts +++ b/src/server.ts @@ -16,6 +16,10 @@ import { loadMenuConfig } from "./ui/menu-config.ts"; import { buildCredentials, storagePluginIds, type StorageCredentials } from "./plugin-host/storage.ts"; const config = loadConfig(); // validates the env (incl. enforced secrets) — fails loud at boot +// The storage secret is in `config` now, so drop it from the environment before ANY plugin code +// runs: a plugin module's top level evaluates during discovery, long before onBoot. Defence in +// depth, not a boundary (AGENTS.md) — and only ever move this line earlier, never later. +delete process.env["PLUGIN_DB_SECRET"]; // App-level logger: structured, OTLP-capable when OTLP_ENDPOINT is set. The hot path clones it // per request for access logging + a trace span (src/http/app.ts); console-only otherwise. const log = createLogger({ format: config.logFormat, level: config.logLevel, otlpEndpoint: config.otlpEndpoint, otlpProtocol: config.otlpProtocol, serviceName: config.serviceName }); @@ -53,14 +57,10 @@ if (declaresStorage.length > 0 && pluginDbUrl === undefined) { throw new Error(`config: PLUGIN_DB_URL must be set — these plugins declare storage: ${declaresStorage.join(", ")}`); } -// Derive every plugin's credentials first, then drop the secret: onBoot is the window in which -// plugin code could read it out of the environment and derive a *peer's* password. Order matters — -// deleting after the hooks would protect nothing. Defence in depth, not a boundary (AGENTS.md). const storageCredentials = new Map(); if (pluginDbUrl !== undefined) { for (const id of declaresStorage) storageCredentials.set(id, buildCredentials(pluginDbUrl, id, config.pluginDbSecret)); } -delete process.env["PLUGIN_DB_SECRET"]; // plugin onBoot — after discovery, before listen; a throw aborts boot. await runBootHooks(plugins, (plugin) => {