Keep role re-assertion within a non-superuser provisioner's rights, and test the second boot

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<Pl
     // The folder name becomes a Postgres identifier, which truncates past 63 bytes — two long ids
     // would then share one database. Only checked for a plugin that asked for storage.
     if (manifest.storage === true && !isValidStoragePluginId(id)) {
-      fail(`declares storage, so its folder name must be at most ${MAX_STORAGE_PLUGIN_ID} characters`);
+      fail(`declares storage, so its folder name must be at most ${MAX_STORAGE_PLUGIN_ID_LENGTH} characters`);
       continue;
     }
 

src/plugin-host/storage-provisioning.ts

@@ -2,25 +2,20 @@
 // alone — the only process holding superuser credentials, which is why the driver stops here.
 
 import postgres from "postgres";
-import { derivePassword, NAME_PREFIX, orphanNames, provisionSql, quoteIdentifier, storageName } from "./storage.ts";
+import { derivePassword, orphanNames, provisionSql, 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;
 }
 
 export interface ProvisionResult {
-  orphans: string[]; // provisioned once, but no installed plugin claims them any more
+  orphans: string[]; // a plugin_ database no installed plugin claims; reported, never dropped
   provisioned: string[];
 }
 
-// Idempotent, and it drops nothing: an uninstalled plugin keeps its data until an operator removes
-// it deliberately. Orphans are reported rather than removed, so nobody has to guess they exist.
 export async function provisionStorage(options: ProvisionOptions): Promise<ProvisionResult> {
   const sql = postgres(options.adminUrl, { connect_timeout: 10, max: 1, onnotice: () => {} });
   try {
@@ -39,12 +34,9 @@ export async function provisionStorage(options: ProvisionOptions): Promise<Provi
       for (const statement of plan) await sql.unsafe(statement); // provisionSql quotes what it interpolates
       provisioned.push(name);
     }
-    for (const database of options.lockdownDatabases ?? []) {
-      await sql.unsafe(`REVOKE CONNECT ON DATABASE ${quoteIdentifier(database)} FROM PUBLIC`);
-    }
-    const existing = await sql<{ datname: string }[]>`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
   }
 }

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<unknown> {
   }
 }
 
+// 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<void> {
+  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 });
+    }
   }
 });

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