From c5c9cce2b67f628793c1b2d3893b28ee8a8689e8 Mon Sep 17 00:00:00 2001 From: lilleman Date: Wed, 19 Aug 2026 00:44:49 +0200 Subject: [PATCH] Keep role re-assertion within a non-superuser provisioner's rights, and test the second boot --- AGENTS.md | 7 +++ README.md | 4 +- ci.sh | 18 +++++-- compose.override.yml | 1 + compose.yml | 1 + ory/postgres/init/init.sql | 6 +-- src/auth/bootstrap.test.ts | 64 ++++++++++++++++++++++++- src/auth/bootstrap.ts | 31 +++++++++--- src/plugin-host/discovery.ts | 4 +- src/plugin-host/storage-provisioning.ts | 16 ++----- src/plugin-host/storage.test.ts | 62 ++++++++++++++++-------- src/plugin-host/storage.ts | 22 ++++----- 12 files changed, 173 insertions(+), 63 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 919bb56..78943d1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -101,6 +101,13 @@ 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.** - **`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 2ae80e8..33fcac1 100644 --- a/README.md +++ b/README.md @@ -798,7 +798,9 @@ creates any role, so no database is ever given a password derivable from a const **Only `bootstrap` holds provisioning credentials.** It alone gets `PLUGIN_DB_ADMIN_URL`, an account with `CREATEDB` and `CREATEROLE` (superuser works but is more than it needs; the dev stack simply -reuses Ory's). `web` gets `PLUGIN_DB_URL`, which names the server and must carry no credentials — +reuses Ory's). Keep using the same account: Postgres gives a `CREATEROLE` account admin rights only +over the roles it created itself, so if you swap it for a fresh one, grant that one `ADMIN OPTION` on +the existing `plugin_*` roles first, or the next boot cannot re-apply their passwords. `web` gets `PLUGIN_DB_URL`, which names the server and must carry no credentials — supply one with a username or password and boot fails, rather than leaving a privileged password in the process that runs plugin code. Set both, plus `PLUGIN_DB_SECRET` ([Configuration](#configuration)); the dev stack sets them for you. diff --git a/ci.sh b/ci.sh index cdcabad..6bbbc33 100755 --- a/ci.sh +++ b/ci.sh @@ -69,11 +69,19 @@ step "Plugin storage (real Postgres)" # --wait, because initdb on a cold volume outlasts the suite's connect timeout. storage_rc=0 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 2>&1) || storage_rc=$? -docker compose -p "$storage_proj" down -v >/dev/null 2>&1 || true +storage_files=(-p "$storage_proj" -f compose.yml) # no override merge, like the e2e suites below +storage_dsn="postgres://${POSTGRES_USER:-ory}:${POSTGRES_PASSWORD:-ory}@postgres:5432/ory" +storage_out="" +docker compose "${storage_files[@]}" up -d --wait postgres >/dev/null || storage_rc=$? +# `if`, not `&&`: a false `&&` returns non-zero, which under `set -e` would exit before teardown. +if [ "$storage_rc" -eq 0 ]; then + # --build like the e2e suites: this stack mounts no source, so without it the step would test + # whatever `web` image that project last baked. + storage_out=$(docker compose "${storage_files[@]}" run --build --rm --no-deps \ + -e "PLUGIN_DB_ADMIN_URL=$storage_dsn" \ + web node --test src/plugin-host/storage.test.ts 2>&1) || storage_rc=$? +fi +docker compose "${storage_files[@]}" down -v >/dev/null 2>&1 || true # also covers a failed `up` 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. diff --git a/compose.override.yml b/compose.override.yml index 9cbccfc..4f8202a 100644 --- a/compose.override.yml +++ b/compose.override.yml @@ -35,6 +35,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 REQUIRE_SECURE_SECRETS: "false" # dev derives from the throwaway, as web does volumes: - .:/app diff --git a/compose.yml b/compose.yml index 6700bda..6e252bc 100644 --- a/compose.yml +++ b/compose.yml @@ -153,6 +153,7 @@ services: # `storage` fails the seed loudly. The secret must match web's; both derive the same passwords. PLUGIN_DB_ADMIN_URL: ${PLUGIN_DB_ADMIN_URL:-} PLUGIN_DB_SECRET: ${PLUGIN_DB_SECRET:-} + PLUGIN_DB_URL: ${PLUGIN_DB_URL:-} # only to refuse a mismatch: what bootstrap creates, web connects to REQUIRE_SECURE_SECRETS: "true" # refuse the throwaway secret here too, before any role is created volumes: - ./ory/kratos/tokenizer:/etc/config/kratos/tokenizer diff --git a/ory/postgres/init/init.sql b/ory/postgres/init/init.sql index cbf7114..dad615f 100644 --- a/ory/postgres/init/init.sql +++ b/ory/postgres/init/init.sql @@ -6,9 +6,9 @@ CREATE DATABASE kratos; CREATE DATABASE keto; CREATE DATABASE hydra; --- Postgres grants CONNECT to PUBLIC by default, so every plugin role would otherwise reach the auth --- plane: table data stays protected, but pg_catalog and the connection slots do not. Ory connects as --- the POSTGRES_USER, which owns these and keeps its access. +-- Postgres grants CONNECT to PUBLIC by default, so every plugin role could otherwise open the auth +-- plane's databases and read pg_catalog; table data stays protected either way. Ory connects as the +-- POSTGRES_USER, which owns these and keeps its access. REVOKE CONNECT ON DATABASE kratos FROM PUBLIC; REVOKE CONNECT ON DATABASE keto FROM PUBLIC; REVOKE CONNECT ON DATABASE hydra FROM PUBLIC; diff --git a/src/auth/bootstrap.test.ts b/src/auth/bootstrap.test.ts index 194c207..2a0c769 100644 --- a/src/auth/bootstrap.test.ts +++ b/src/auth/bootstrap.test.ts @@ -5,7 +5,10 @@ import { test } from "node:test"; import assert from "node:assert/strict"; import { randomUUID } from "node:crypto"; -import { ensureJwks, firstRunBanner, identityPayload, permissionTuple, seedAdmin, seedPermissions } from "./bootstrap.ts"; +import { differentServer, ensureJwks, firstRunBanner, identityPayload, permissionTuple, provisionPluginStorage, seedAdmin, seedPermissions } 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"; const json = (status: number, body?: unknown) => new Response(body === undefined ? null : JSON.stringify(body), { @@ -151,3 +154,62 @@ test("ensureJwks generates a key only when the file is absent", () => { assert.equal(ensureJwks(path, { exists: () => true, write }), false); assert.equal(writes.length, 1); // present → nothing written }); + +// --- Plugin storage provisioning ----------------------------------------------------- +// The provisioner is injected, so the branch decisions are testable without a Postgres. + +const SILENT = createLogger({ level: "none" }); +const storagePlugin = (id: string): Plugin => ({ apiVersion: "1.0.0", id, storage: true }); +const EMPTY: ProvisionResult = { orphans: [], provisioned: [] }; + +function recordingProvisioner(result: ProvisionResult = EMPTY) { + const calls: ProvisionOptions[] = []; + return { calls, provision: async (options: ProvisionOptions) => { calls.push(options); return result; } }; +} + +test("provisioning is skipped entirely when nothing declares storage and none is configured", async () => { + const { calls, provision } = recordingProvisioner(); + await provisionPluginStorage({}, [{ apiVersion: "1.0.0", id: "plain" }], SILENT, provision); + assert.deepEqual(calls, []); // no connection attempted, so an unconfigured stack still boots +}); + +// Uninstalling the last storage plugin is exactly when a left-behind database needs naming. +test("provisioning still runs with nothing to provision, so orphans are reported", async () => { + const { calls, provision } = recordingProvisioner({ orphans: ["plugin_gone"], provisioned: [] }); + await provisionPluginStorage({ PLUGIN_DB_ADMIN_URL: "postgres://ory:ory@db:5432/ory" }, [], SILENT, provision); + assert.equal(calls.length, 1); + assert.deepEqual(calls[0]?.pluginIds, []); +}); + +test("a plugin declaring storage without a provisioning DSN fails loud, naming the plugin", async () => { + const { calls, provision } = recordingProvisioner(); + await assert.rejects( + provisionPluginStorage({}, [storagePlugin("things")], SILENT, provision), + /PLUGIN_DB_ADMIN_URL.*things/s, + ); + assert.deepEqual(calls, []); +}); + +test("the connection limit and derived secret reach the provisioner", async () => { + const { calls, provision } = recordingProvisioner(); + const env = { PLUGIN_DB_ADMIN_URL: "postgres://ory:ory@db:5432/ory", PLUGIN_DB_CONNECTION_LIMIT: "25", PLUGIN_DB_SECRET: "real" }; + await provisionPluginStorage(env, [storagePlugin("things")], SILENT, provision); + assert.equal(calls[0]?.connectionLimit, 25); + assert.equal(calls[0]?.secret, "real"); + assert.deepEqual(calls[0]?.pluginIds, ["things"]); +}); + +// 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 () => { + 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, []); +}); + +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"); +}); diff --git a/src/auth/bootstrap.ts b/src/auth/bootstrap.ts index b18dd43..d693f01 100644 --- a/src/auth/bootstrap.ts +++ b/src/auth/bootstrap.ts @@ -18,10 +18,6 @@ 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) { @@ -169,17 +165,20 @@ async function main() { // A database and login role for each plugin that asked for one. It happens here because bootstrap // holds the stack's only provisioning credentials — web derives the same password and connects as // the plugin's own role. -async function provisionPluginStorage(env: Env, plugins: Plugin[], log: Log): Promise { +export async function provisionPluginStorage(env: Env, plugins: Plugin[], log: Log, provision = provisionStorage): Promise { const ids = storagePluginIds(plugins); const adminUrl = env["PLUGIN_DB_ADMIN_URL"]; // Still connect with nothing to provision, as long as storage is configured: uninstalling the // 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(", ")}`); - const result = await provisionStorage({ + // 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})`); + const result = await provision({ adminUrl, connectionLimit: resolvePluginDbConnectionLimit(env), - lockdownDatabases: ORY_DATABASES, pluginIds: ids, secret: resolvePluginDbSecret(env), }); @@ -190,6 +189,24 @@ async function provisionPluginStorage(env: Env, plugins: Plugin[], log: Log): Pr } } +// 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 { + 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 + return `${admin} vs ${web}`; +} + +function safeHostPort(url: string): string | null { + try { + const parsed = new URL(url); + return `${parsed.hostname}:${parsed.port || "5432"}`; + } catch { + return null; + } +} + // Seed every discovered plugin's declared permission names (plus any ADMIN_PERMISSIONS), so the // shipped example — and any dropped-in plugin — works for the demo admin without a host edit. async function seedAdminAndPermissions(env: Env, plugins: Plugin[], log: Log): Promise { diff --git a/src/plugin-host/discovery.ts b/src/plugin-host/discovery.ts index ba32562..d4e19c9 100644 --- a/src/plugin-host/discovery.ts +++ b/src/plugin-host/discovery.ts @@ -8,7 +8,7 @@ import { existsSync, readdirSync, readFileSync } from "node:fs"; import { dirname, join } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { checkApiVersion, findConflicts, isValidPermissionName, isValidPluginId, RESERVED_PLUGIN_IDS, type Plugin, type PluginManifest } from "./plugin.ts"; -import { isValidStoragePluginId, MAX_STORAGE_PLUGIN_ID } from "./storage.ts"; +import { isValidStoragePluginId, MAX_STORAGE_PLUGIN_ID_LENGTH } from "./storage.ts"; const rootDir = join(dirname(fileURLToPath(import.meta.url)), "..", ".."); @@ -70,7 +70,7 @@ export async function discoverPlugins(options: DiscoverOptions = {}): Promise { const sql = postgres(options.adminUrl, { connect_timeout: 10, max: 1, onnotice: () => {} }); try { @@ -39,12 +34,9 @@ export async function provisionStorage(options: ProvisionOptions): Promise`SELECT datname FROM pg_database WHERE starts_with(datname, ${NAME_PREFIX})`; + const existing = await sql<{ datname: string }[]>`SELECT datname FROM pg_database`; return { orphans: orphanNames(existing.map((row) => row.datname), provisioned), provisioned }; } finally { - await sql.end(); + await sql.end({ timeout: 5 }); // a wedged connection would otherwise hang the boot web waits on } } diff --git a/src/plugin-host/storage.test.ts b/src/plugin-host/storage.test.ts index b9eded9..f04f03b 100644 --- a/src/plugin-host/storage.test.ts +++ b/src/plugin-host/storage.test.ts @@ -9,7 +9,7 @@ import { buildCredentials, derivePassword, isValidStoragePluginId, - MAX_STORAGE_PLUGIN_ID, + MAX_STORAGE_PLUGIN_ID_LENGTH, orphanNames, provisionSql, quoteIdentifier, @@ -26,9 +26,9 @@ test("the database and the role share one plugin_-prefixed name", () => { }); test("a storage plugin's id must leave the identifier under Postgres' 63 bytes", () => { - assert.equal(MAX_STORAGE_PLUGIN_ID, 56); // 63 - "plugin_" - assert.ok(isValidStoragePluginId("a".repeat(MAX_STORAGE_PLUGIN_ID))); - assert.ok(!isValidStoragePluginId("a".repeat(MAX_STORAGE_PLUGIN_ID + 1))); + assert.equal(MAX_STORAGE_PLUGIN_ID_LENGTH, 56); // 63 - "plugin_" + assert.ok(isValidStoragePluginId("a".repeat(MAX_STORAGE_PLUGIN_ID_LENGTH))); + assert.ok(!isValidStoragePluginId("a".repeat(MAX_STORAGE_PLUGIN_ID_LENGTH + 1))); }); test("the password is derived, so the same one is reachable without storing it", () => { @@ -63,7 +63,9 @@ test("quoting doubles an embedded quote", () => { assert.equal(quoteLiteral("we'ird"), "'we''ird'"); }); -const ATTRIBUTES = "LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE CONNECTION LIMIT 10"; +// 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", () => { assert.deepEqual( @@ -100,10 +102,12 @@ test("re-provisioning re-asserts every attribute and creates nothing twice", () ]); }); -// The limit is interpolated unquoted, so a non-integer would corrupt the statement text. -test("a non-integer connection limit is refused, not interpolated", () => { - const plan = { connectionLimit: 1.5, databaseExists: false, name: "plugin_things", password: "pw", roleExists: false }; - assert.throws(() => provisionSql(plan), /connectionLimit must be an integer/); +// The limit is interpolated unquoted, and Postgres reads a negative one as "unlimited". +test("a connection limit that is not a positive integer is refused, not interpolated", () => { + const plan = { databaseExists: false, name: "plugin_things", password: "pw", roleExists: false }; + for (const connectionLimit of [1.5, 0, -1, Number.NaN]) { + assert.throws(() => provisionSql({ ...plan, connectionLimit }), /positive integer/, `for ${connectionLimit}`); + } }); // --- Integration: the statements above, against a real Postgres ----------------------- @@ -130,11 +134,22 @@ async function queryAs(url: string, statement: string): Promise { } } +// Drops what a previous run may have left behind: `finally` does not survive a SIGKILL or a +// cancelled CI job, and the leftovers would otherwise fail every later run on the same server. +async function dropStorage(admin: postgres.Sql, ids: string[]): Promise { + for (const id of ids) { + const name = quoteIdentifier(storageName(id)); + await admin.unsafe(`DROP DATABASE IF EXISTS ${name} WITH (FORCE)`); + await admin.unsafe(`DROP ROLE IF EXISTS ${name}`); + } +} + test("provisions a database its plugin can use and a peer plugin cannot reach", integration, async () => { const ids = ["storage-itest-a", "storage-itest-b"]; const base = baseUrlOf(ADMIN_URL); const admin = postgres(ADMIN_URL, { connect_timeout: 10, max: 1, onnotice: () => {} }); try { + await dropStorage(admin, ids); await provisionStorage({ adminUrl: ADMIN_URL, connectionLimit: 10, pluginIds: ids, secret: SECRET }); const owner = buildCredentials(base, "storage-itest-a", SECRET); @@ -163,12 +178,11 @@ test("provisions a database its plugin can use and a peer plugin cannot reach", const uninstalled = await provisionStorage({ adminUrl: ADMIN_URL, connectionLimit: 10, pluginIds: [], secret: "a-rotated-secret" }); for (const id of ids) assert.ok(uninstalled.orphans.includes(storageName(id)), `${id}'s database is reported`); } finally { - for (const id of ids) { - const name = quoteIdentifier(storageName(id)); - await admin.unsafe(`DROP DATABASE IF EXISTS ${name} WITH (FORCE)`); - await admin.unsafe(`DROP ROLE IF EXISTS ${name}`); + try { + await dropStorage(admin, ids); + } finally { + await admin.end({ timeout: 5 }); // its own finally, or a failed DROP leaks the connection } - await admin.end(); } }); @@ -179,22 +193,30 @@ test("provisions through a CREATEDB + CREATEROLE account, without superuser", in const provisioner = "storage-itest-provisioner"; const admin = postgres(ADMIN_URL, { connect_timeout: 10, max: 1, onnotice: () => {} }); try { + // The fresh provisioner below holds no ADMIN option on a role an earlier run left behind, so a + // leftover would fail the ALTER branch rather than the code being wrong. + await dropStorage(admin, [pluginId]); 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 provision = () => provisionStorage({ adminUrl: asProvisioner.href, connectionLimit: 10, pluginIds: [pluginId], secret: SECRET }); + await provision(); + // Twice: the second run takes the ALTER branch, where naming a superuser-only attribute would + // fail — i.e. every redeploy after the one that worked. + await provision(); 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(); + try { + await dropStorage(admin, [pluginId]); + await admin.unsafe(`DROP ROLE IF EXISTS ${quoteIdentifier(provisioner)}`); + } finally { + await admin.end({ timeout: 5 }); + } } }); diff --git a/src/plugin-host/storage.ts b/src/plugin-host/storage.ts index b69bbab..361ed8e 100644 --- a/src/plugin-host/storage.ts +++ b/src/plugin-host/storage.ts @@ -9,9 +9,8 @@ import type { Plugin } from "./plugin.ts"; export const NAME_PREFIX = "plugin_"; // Postgres truncates an identifier at 63 bytes, which would silently collide two long ids. -export const MAX_STORAGE_PLUGIN_ID = 63 - NAME_PREFIX.length; +export const MAX_STORAGE_PLUGIN_ID_LENGTH = 63 - NAME_PREFIX.length; -// `url` pre-assembles the other fields as a DSN, which most drivers take directly. export interface StorageCredentials { database: string; host: string; @@ -26,7 +25,7 @@ export function storageName(pluginId: string): string { } export function isValidStoragePluginId(pluginId: string): boolean { - return pluginId.length <= MAX_STORAGE_PLUGIN_ID; + return Buffer.byteLength(pluginId) <= MAX_STORAGE_PLUGIN_ID_LENGTH; // Postgres counts bytes, not characters } export function storagePluginIds(plugins: Plugin[]): string[] { @@ -34,6 +33,7 @@ export function storagePluginIds(plugins: Plugin[]): string[] { } // Derived, never stored — which is what keeps the host free of state it would have to persist. +// Whoever holds the secret holds every plugin's database. export function derivePassword(secret: string, pluginId: string): string { return createHmac("sha256", secret).update(pluginId).digest("base64url"); } @@ -58,8 +58,6 @@ 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(); } @@ -72,16 +70,16 @@ export interface ProvisionPlan { roleExists: boolean; } -// One plugin's full plan, in order. Both role branches state the same attributes, so every boot -// re-asserts them: a privilege granted by hand out of band does not survive silently. export function provisionSql(plan: ProvisionPlan): string[] { - if (!Number.isSafeInteger(plan.connectionLimit)) { - throw new Error(`storage: connectionLimit must be an integer, got ${plan.connectionLimit}`); // interpolated unquoted + // Interpolated unquoted, and Postgres reads a negative limit as "unlimited" — the opposite of the point. + if (!Number.isSafeInteger(plan.connectionLimit) || plan.connectionLimit < 1) { + throw new Error(`storage: connectionLimit must be a positive integer, got ${plan.connectionLimit}`); } const identifier = quoteIdentifier(plan.name); - // NOSUPERUSER/NOCREATEDB/NOCREATEROLE: a plugin owns its own database and nothing beyond it. - // The limit bounds one plugin's pools so they cannot starve Ory, which shares this server. - const attributes = `LOGIN NOSUPERUSER NOCREATEDB NOCREATEROLE CONNECTION LIMIT ${plan.connectionLimit} PASSWORD ${quoteLiteral(plan.password)}`; + // 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. + 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}`, // CREATE DATABASE ... OWNER needs SET ROLE on the owner, and PG16+ gives a CREATEROLE account