diff --git a/AGENTS.md b/AGENTS.md index 78943d1..f754cf9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,8 +84,8 @@ Revisit only if the stated reason stops holding. - **Plugin storage hands over credentials, not a client** (README → Plugin storage). The host takes `postgres` to run the provisioning DDL, and `storage-provisioning.ts` is the only module importing it — `storage.ts` beside it stays pure so `web` never loads a driver (`src/postgres.test.ts` guards - both halves; the claim silently went false once already). It is never re-exported through the - barrel, so no driver shape enters the contract. Three properties hold the design together, so + both halves, because one value imported from the wrong module breaks it invisibly). It is never + re-exported through the barrel, so no driver shape enters the contract. Three properties hold the design together, so don't trade one away in isolation: passwords are `HMAC-SHA256(PLUGIN_DB_SECRET, id)` rather than stored, which is what keeps the host stateless — whoever holds that secret holds every plugin database, so it ranks with the DB password itself; the provisioning DSN reaches `bootstrap` only @@ -101,13 +101,12 @@ Revisit only if the stated reason stops holding. 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.** -- **`ory/postgres/init/init.sql` is the only home for the Ory databases' ACL.** Re-asserting the - `REVOKE CONNECT` from `bootstrap` each boot was tried and removed: `REVOKE` only *warns* when the - caller doesn't own the database, so under the least-privilege provisioning account the README - recommends it reported success while changing nothing — and it hard-failed whenever - `PLUGIN_DB_ADMIN_URL` named a server with no `kratos`. A volume created before that file gained the - revokes keeps the default grant; `docker compose down -v` is the dev remedy. **Valid while - pre-release, with no deployed volumes to migrate.** +- **`ory/postgres/init/init.sql` is the only home for the Ory databases' ACL** — don't re-assert the + `REVOKE CONNECT` from `bootstrap`. `REVOKE` only *warns* when the caller doesn't own the database, + so under the least-privilege provisioning account the README recommends it would report success + while changing nothing, and it hard-fails whenever `PLUGIN_DB_ADMIN_URL` names a server with no + `kratos`. A volume created before that file gained the revokes keeps the default grant; + `docker compose down -v` is the dev remedy. **Valid while pre-release, with no deployed volumes.** - **`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 diff --git a/README.md b/README.md index 33fcac1..e104215 100644 --- a/README.md +++ b/README.md @@ -781,11 +781,12 @@ What the host does guarantee: boundary: plugins share the `web` process, so a plugin that goes looking can reach another's credentials. Install plugins you trust ([Security model](#security-model)). - **Provisioning is idempotent and runs every boot**, so a plugin dropped in later is picked up by - the next `docker compose up -d` — the same rule as permission seeding. Role attributes are - re-applied each time, so a privilege granted by hand out of band does not quietly persist. + the next `docker compose up -d` — the same rule as permission seeding. Each boot re-applies the + role's password, connection limit, and `NOCREATEDB`/`NOCREATEROLE`. - **Your data is never dropped.** Removing a plugin folder leaves its database untouched; deleting it is a deliberate act by an operator. Each boot logs any `plugin_*` database no installed plugin - claims, so what you left behind stays findable. + claims, so what you left behind stays findable — read that list before dropping anything, since a + second Plainpages stack sharing this server will have its databases named there too. **Passwords are derived, never stored** — each is `HMAC-SHA256(PLUGIN_DB_SECRET, )`, so `bootstrap` and `web` compute the same value independently and nothing has to be written down. diff --git a/compose.override.yml b/compose.override.yml index 4f8202a..2b9de80 100644 --- a/compose.override.yml +++ b/compose.override.yml @@ -1,5 +1,9 @@ # Development overrides, merged automatically by `docker compose up`. # Mounts the source for live editing and restarts on change via `node --watch`. + +# web connects with it and bootstrap provisions against it, so the two must agree — one home. +x-plugin-db-url: &plugin-db-url postgres://postgres:5432 + services: web: command: node --watch src/server.ts @@ -15,7 +19,7 @@ services: LOG_LEVEL: "debug" # verbose by default while developing (base defaults to info) # Point plugin storage at the bundled Postgres, so a dropped-in plugin declaring `storage` # works with no further config; the secret falls back to the dev throwaway (config.ts). - PLUGIN_DB_URL: postgres://postgres:5432 + PLUGIN_DB_URL: *plugin-db-url REQUIRE_SECURE_SECRETS: "false" SECURE_COOKIES: "false" # dev serves http — Secure cookies wouldn't be sent SCHEDULING_UPSTREAM: "http://shifts-upstream:4000" # backs the reference plugin once you copy it into plugins/ @@ -35,7 +39,7 @@ services: # Provisions the plugin databases web connects to above, as the dev superuser. environment: PLUGIN_DB_ADMIN_URL: postgres://${POSTGRES_USER:-ory}:${POSTGRES_PASSWORD:-ory}@postgres:5432/ory - PLUGIN_DB_URL: postgres://postgres:5432 # must match web's above + PLUGIN_DB_URL: *plugin-db-url REQUIRE_SECURE_SECRETS: "false" # dev derives from the throwaway, as web does volumes: - .:/app diff --git a/src/auth/bootstrap.test.ts b/src/auth/bootstrap.test.ts index 2a0c769..f57655b 100644 --- a/src/auth/bootstrap.test.ts +++ b/src/auth/bootstrap.test.ts @@ -5,7 +5,7 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { randomUUID } from "node:crypto"; -import { differentServer, ensureJwks, firstRunBanner, identityPayload, permissionTuple, provisionPluginStorage, seedAdmin, seedPermissions } from "./bootstrap.ts"; +import { ensureJwks, firstRunBanner, identityPayload, permissionTuple, provisionPluginStorage, seedAdmin, seedPermissions, serverMismatch } from "./bootstrap.ts"; import { createLogger } from "../logger.ts"; import type { Plugin } from "../plugin-host/plugin.ts"; import type { ProvisionOptions, ProvisionResult } from "../plugin-host/storage-provisioning.ts"; @@ -200,16 +200,17 @@ test("the connection limit and derived secret reach the provisioner", async () = }); // bootstrap creates the role on one server; web tells the plugin to connect to another. Left -// unchecked it surfaces inside a plugin as "password authentication failed", naming neither. -test("provisioning refuses when the two storage URLs name different servers", async () => { +// unsaid it surfaces inside a plugin as "password authentication failed", naming neither. Warned +// rather than refused: web reaching a pooler bootstrap cannot provision through is legitimate. +test("a storage URL mismatch is reported, and provisioning still runs", async () => { const { calls, provision } = recordingProvisioner(); const env = { PLUGIN_DB_ADMIN_URL: "postgres://ory:ory@db-a:5432/ory", PLUGIN_DB_URL: "postgres://db-b:5432" }; - await assert.rejects(provisionPluginStorage(env, [storagePlugin("things")], SILENT, provision), /one server.*db-a:5432 vs db-b:5432/s); - assert.deepEqual(calls, []); + await provisionPluginStorage(env, [storagePlugin("things")], SILENT, provision); + assert.equal(calls.length, 1); }); test("the same server spelled with an implicit port still agrees", () => { - assert.equal(differentServer("postgres://ory:ory@db:5432/ory", "postgres://db"), null); // 5432 is the default - assert.equal(differentServer("postgres://ory:ory@db:5432/ory", undefined), null); // web's own boot error to raise - assert.equal(differentServer("postgres://ory:ory@db:5432/ory", "postgres://db:6543"), "db:5432 vs db:6543"); + assert.equal(serverMismatch("postgres://ory:ory@db:5432/ory", "postgres://db"), null); // 5432 is the default + assert.equal(serverMismatch("postgres://ory:ory@db:5432/ory", undefined), null); // web's own boot error to raise + assert.equal(serverMismatch("postgres://ory:ory@db:5432/ory", "postgres://db:6543"), "db:5432 vs db:6543"); }); diff --git a/src/auth/bootstrap.ts b/src/auth/bootstrap.ts index d693f01..137ef59 100644 --- a/src/auth/bootstrap.ts +++ b/src/auth/bootstrap.ts @@ -146,7 +146,7 @@ export function firstRunBanner(opts: { appUrl: string; email: string; password: // --- CLI (the bootstrap container entrypoint) ---------------------------------------- async function main() { - const env = process.env; + const env = { ...process.env }; // snapshot: the storage credentials leave process.env before discovery // Structured like the web app so prod logs stay uniform; honour LOG_FORMAT/SERVICE_NAME. const log = createLogger({ format: env["LOG_FORMAT"] === "json" ? "json" : "text", @@ -155,6 +155,10 @@ async function main() { // runWithLog makes `log` ambient so seedAdmin's tracedFetch traces the Kratos/Keto seed calls. await runWithLog(log, async () => { if (ensureJwks(env["JWKS_FILE"] ?? "/etc/config/kratos/tokenizer/jwks.json")) log.info("generated a JWKS signing key"); + // Discovery imports every plugin module — and its dependencies — into *this* process, which holds + // the credential that may CREATE DATABASE/ROLE. Same move as server.ts, on the stronger secret. + delete process.env["PLUGIN_DB_ADMIN_URL"]; + delete process.env["PLUGIN_DB_SECRET"]; const plugins = await discoverPlugins(); await provisionPluginStorage(env, plugins, log); await seedAdminAndPermissions(env, plugins, log); @@ -172,10 +176,11 @@ export async function provisionPluginStorage(env: Env, plugins: Plugin[], log: L // last storage plugin is exactly when an orphaned database needs naming. if (ids.length === 0 && !adminUrl) return; if (!adminUrl) throw new Error(`bootstrap: PLUGIN_DB_ADMIN_URL must be set — these plugins declare storage: ${ids.join(", ")}`); - // Provisioned here, connected to from web: a different server means the role exists in one place - // and is looked for in another, surfacing as "password authentication failed" inside a plugin. - const serverMismatch = differentServer(adminUrl, env["PLUGIN_DB_URL"]); - if (serverMismatch) throw new Error(`bootstrap: PLUGIN_DB_ADMIN_URL and PLUGIN_DB_URL must name one server (${serverMismatch})`); + // Provisioned here, connected to from web: a different server means the role is created in one + // place and looked for in another, surfacing inside a plugin as "password authentication failed". + // Warned, not refused — web reaching a pooler that cannot run CREATE DATABASE is a legitimate split. + const mismatch = serverMismatch(adminUrl, env["PLUGIN_DB_URL"]); + if (mismatch) log.warn("PLUGIN_DB_ADMIN_URL and PLUGIN_DB_URL name different servers", { servers: mismatch }); const result = await provision({ adminUrl, connectionLimit: resolvePluginDbConnectionLimit(env), @@ -191,7 +196,7 @@ export async function provisionPluginStorage(env: Env, plugins: Plugin[], log: L // Describes the disagreement, or null when they agree (or when web's URL is unset — that is web's // own boot error to raise, naming the plugin that wanted storage). -export function differentServer(adminUrl: string, webUrl: string | undefined): string | null { +export function serverMismatch(adminUrl: string, webUrl: string | undefined): string | null { if (!webUrl) return null; const [admin, web] = [safeHostPort(adminUrl), safeHostPort(webUrl)]; if (admin === null || web === null || admin === web) return null; // a malformed URL fails in config.ts diff --git a/src/compose.test.ts b/src/compose.test.ts index 6542ce5..fc930ed 100644 --- a/src/compose.test.ts +++ b/src/compose.test.ts @@ -86,6 +86,9 @@ test("the provisioning superuser DSN reaches bootstrap only, never web", () => { const boot = compose.slice(compose.indexOf("\n bootstrap:")); const overrideWeb = override.slice(override.indexOf("\n web:"), override.indexOf("\n bootstrap:")); assert.match(boot, /PLUGIN_DB_ADMIN_URL:/, "bootstrap is given the superuser DSN"); + // Reordering the override's services would empty this slice, and every doesNotMatch below would + // then pass against "". + assert.ok(overrideWeb.includes("PLUGIN_DB_URL"), "sliced the dev override's web block"); for (const [name, block] of [["base", webBlock], ["dev override", overrideWeb]] as const) assert.doesNotMatch(block, /PLUGIN_DB_ADMIN_URL/, `${name} web never sees it`); assert.match(webBlock, /PLUGIN_DB_URL:\s*\$\{PLUGIN_DB_URL/, "base wires web's base URL from env"); diff --git a/src/plugin-host/storage-provisioning.ts b/src/plugin-host/storage-provisioning.ts index e228e1f..c58d422 100644 --- a/src/plugin-host/storage-provisioning.ts +++ b/src/plugin-host/storage-provisioning.ts @@ -17,7 +17,9 @@ export interface ProvisionResult { } export async function provisionStorage(options: ProvisionOptions): Promise { - const sql = postgres(options.adminUrl, { connect_timeout: 10, max: 1, onnotice: () => {} }); + // Notices are left to surface: a REVOKE the account cannot perform only *warns*, and silencing + // that would mean reporting a locked-down database that is still open to PUBLIC. + const sql = postgres(options.adminUrl, { connect_timeout: 10, max: 1 }); try { const provisioned: string[] = []; for (const pluginId of options.pluginIds) { diff --git a/src/plugin-host/storage.test.ts b/src/plugin-host/storage.test.ts index f04f03b..3bc49c1 100644 --- a/src/plugin-host/storage.test.ts +++ b/src/plugin-host/storage.test.ts @@ -63,8 +63,6 @@ test("quoting doubles an embedded quote", () => { assert.equal(quoteLiteral("we'ird"), "'we''ird'"); }); -// No NOSUPERUSER: naming it in an ALTER is superuser-only, so re-asserting it would break every -// boot after the first under the least-privilege account the README recommends. const ATTRIBUTES = "LOGIN NOCREATEDB NOCREATEROLE CONNECTION LIMIT 10"; test("only the plugins that asked for storage are provisioned", () => { diff --git a/src/plugin-host/storage.ts b/src/plugin-host/storage.ts index 361ed8e..3516a68 100644 --- a/src/plugin-host/storage.ts +++ b/src/plugin-host/storage.ts @@ -76,9 +76,7 @@ export function provisionSql(plan: ProvisionPlan): string[] { throw new Error(`storage: connectionLimit must be a positive integer, got ${plan.connectionLimit}`); } const identifier = quoteIdentifier(plan.name); - // No NOSUPERUSER: only a superuser may name SUPERUSER in an ALTER, so re-asserting it would fail - // every boot after the first under the CREATEDB+CREATEROLE account the README recommends. CREATE - // defaults to NOSUPERUSER and a non-superuser cannot grant it, so nothing is given up. + // No NOSUPERUSER: naming SUPERUSER in an ALTER is superuser-only, and CREATE defaults to it anyway. const attributes = `LOGIN NOCREATEDB NOCREATEROLE CONNECTION LIMIT ${plan.connectionLimit} PASSWORD ${quoteLiteral(plan.password)}`; return [ plan.roleExists ? `ALTER ROLE ${identifier} WITH ${attributes}` : `CREATE ROLE ${identifier} ${attributes}`, diff --git a/src/postgres.test.ts b/src/postgres.test.ts index ffa2c23..e636e92 100644 --- a/src/postgres.test.ts +++ b/src/postgres.test.ts @@ -31,9 +31,9 @@ test("init SQL gives each Ory service its own database, and leaves plugin databa } }); -// AGENTS.md records that the driver runs the provisioning DDL in bootstrap "and nothing else". -// discovery.ts once imported a validator from the module that held it, quietly putting the driver in -// web's graph — the claim outlived the fact, which is what this catches. +// AGENTS.md records that the driver runs the provisioning DDL in bootstrap and nothing else. A +// single value imported from the wrong module puts it in web's graph without changing behaviour, +// so nothing but this would notice. test("the Postgres driver reaches bootstrap only, never web's import graph", () => { const files = sourceFiles(); assert.ok(files.length > 40, "walks the source tree"); diff --git a/src/server.ts b/src/server.ts index e693de0..b489926 100644 --- a/src/server.ts +++ b/src/server.ts @@ -61,6 +61,10 @@ const storageCredentials = new Map(); if (pluginDbUrl !== undefined) { for (const id of declaresStorage) storageCredentials.set(id, buildCredentials(pluginDbUrl, id, config.pluginDbSecret)); } +// onBoot is the only way credentials are handed over, so without one the database is provisioned +// and unreachable. A warning, not a refusal — the plugin still works, it just cannot store anything. +const unreachable = plugins.filter((plugin) => plugin.storage && !plugin.hooks?.onBoot).map((plugin) => plugin.id); +if (unreachable.length > 0) log.warn("plugins declare storage but have no onBoot to receive it", { plugins: unreachable.join(", ") }); // plugin onBoot — after discovery, before listen; a throw aborts boot. await runBootHooks(plugins, (plugin) => {