diff --git a/src/servers/os-user-postgres.ts b/src/servers/os-user-postgres.ts index f1007211..05b7ffaa 100644 --- a/src/servers/os-user-postgres.ts +++ b/src/servers/os-user-postgres.ts @@ -2,6 +2,7 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { sql } from 'drizzle-orm'; +import postgres from 'postgres'; import { db } from 'officerdb/db'; import { osUserHome, runAs } from './os-user'; @@ -33,16 +34,32 @@ import { osUserHome, runAs } from './os-user'; // - Revoking from the ROLE does not help. Postgres privileges are additive and there is no DENY, so a // PUBLIC grant is not overridden by a role-level revoke. Revoking from PUBLIC is the only lock. // -// ── The residue, stated rather than implied ── +// ── Keeping members out of EACH OTHER'S databases ── // -// A database one member creates is readable-as-metadata by another: `datacl` is NOT inherited from the -// template (measured — closing `template1` and creating from it still produced a NULL `datacl`), and -// `CREATE DATABASE` fires no event trigger, so nothing can close a member's database at the moment they -// create it. A second member can connect and read table and column NAMES from the catalogue. They cannot -// read a row, and they cannot create anything (PG15+ removed PUBLIC's CREATE on `public`). +// Harder than it sounds, and the two obvious routes are both dead ends. `datacl` is NOT inherited from the +// template — measured: closing `template1` and creating a database from it still produced a NULL `datacl`, +// so PUBLIC gets CONNECT on every database a member makes. And `CREATE DATABASE` fires no event trigger, +// because it is a global object, so nothing can react at the moment of creation. // -// Closing that needs either a sweep or a `pg_hba.conf` rule per member, and both are decisions rather -// than details. Not built. Do not let this comment be read as "handled". +// What does work is a `login` event trigger (PG17+) installed in `template1`. Event triggers live in a +// per-database catalogue, and `CREATE DATABASE` copies the template's catalogues — so every database a +// member creates carries it, automatically, with no naming convention, no sweep and no window. +// +// ── Why the function lives in its own schema ── +// +// The first version put it in `public` and a member DEFEATED IT IN ONE STATEMENT: +// +// DROP FUNCTION public.officer_owner_only() CASCADE; -- takes the event trigger with it +// +// They could not drop or disable the trigger — `must be owner of event trigger` — but in PG15+ the +// `public` schema is owned by `pg_database_owner`, which resolves to THEM inside their own database, and a +// schema owner may drop objects in it that they do not own. Both objects were owned by `postgres` and it +// made no difference. +// +// So the function goes in a schema owned by the platform with PUBLIC revoked. Measured after the move — +// every route refused: DROP EVENT TRIGGER, ALTER … DISABLE, DROP FUNCTION, DROP SCHEMA, ALTER SCHEMA … +// OWNER TO, CREATE OR REPLACE over the top, and `PGOPTIONS=-c event_triggers=off` (that GUC is superuser +// only). A superuser can still use it, which is the recovery path if this ever locks something out. /** Legal Linux/Postgres account name. Identical to `validateUsername`, restated because this one reaches SQL. */ const ROLE_NAME_RE = /^[a-zA-Z0-9._-]{2,32}$/; @@ -150,6 +167,65 @@ export async function ensureAppDatabaseClosed(): Promise (ex instanceof Error ? ex.message : String(ex)); +/** The isolation schema, function and login trigger. Ordered; each is idempotent on its own. */ +const TEMPLATE_ISOLATION_SQL = [ + `CREATE SCHEMA IF NOT EXISTS officer AUTHORIZATION CURRENT_USER`, + `REVOKE ALL ON SCHEMA officer FROM PUBLIC`, + // SECURITY INVOKER (the default) with a pinned search_path. DEFINER is not needed — every catalogue this + // reads is world-readable — and not using it removes the search_path-shadowing class of problem outright. + `CREATE OR REPLACE FUNCTION officer.owner_only() RETURNS event_trigger + LANGUAGE plpgsql SET search_path = pg_catalog AS $fn$ + BEGIN + IF (SELECT rolsuper FROM pg_roles WHERE rolname = current_user) THEN RETURN; END IF; + IF pg_has_role(current_user, (SELECT datdba FROM pg_database WHERE datname = current_database()), 'USAGE') + THEN RETURN; END IF; + RAISE EXCEPTION 'permission denied for database "%"', current_database() + USING HINT = 'Officer: a database is private to the account that created it.'; + END $fn$`, + // No IF NOT EXISTS for event triggers, so drop and recreate. The gap is inside template1 and only matters + // to a CREATE DATABASE landing in the same instant, which cannot happen — see the connection note below. + `DROP EVENT TRIGGER IF EXISTS officer_owner_only_login`, + `CREATE EVENT TRIGGER officer_owner_only_login ON login EXECUTE FUNCTION officer.owner_only()`, +]; + +/** + * Install the isolation trigger into `template1`, so every database created afterwards inherits it. + * + * ── Why this opens its own connection, and closes it immediately ── + * + * The platform's pool is connected to `officer`; a connection cannot change database. And `CREATE DATABASE` + * requires that NO other session is connected to the template it copies — so a pooled or long-lived + * connection to `template1` would make every member's `createdb` fail with "source database is being + * accessed by other users". Hence one connection, `max: 1`, ended in a `finally`. + * + * Only ever runs at provisioning time, which is rare enough that the race with a member running `createdb` + * in the same second is worth naming and not worth engineering around: it surfaces as a retryable error on + * their side. + */ +export async function ensureTemplateIsolation(): Promise<{ ok: true } | { ok: false; error: string }> { + const url = process.env.POSTGRES_URL; + if (!url) return { ok: false, error: 'POSTGRES_URL is not set' }; + + let target: string; + try { + const parsed = new URL(url); + parsed.pathname = '/template1'; + target = parsed.toString(); + } catch { + return { ok: false, error: 'POSTGRES_URL could not be parsed' }; + } + + const client = postgres(target, { max: 1, idle_timeout: 5, connection: { application_name: 'officer-provision' } }); + try { + for (const statement of TEMPLATE_ISOLATION_SQL) await client.unsafe(statement); + return { ok: true }; + } catch (ex) { + return { ok: false, error: `could not install database isolation into template1: ${asMessage(ex)}` }; + } finally { + await client.end({ timeout: 5 }); + } +} + export type PostgresRoleResult = | { ok: true; role: string; created: boolean; passwordSet: boolean } | { ok: false; error: string }; @@ -172,10 +248,13 @@ export async function provisionPostgresRole(params: { uid: number; gid: number; }): Promise { - // Before anything else, and before the role can exist to take advantage of it. + // Both before anything else, and before the role can exist to take advantage of either being absent. const closed = await ensureAppDatabaseClosed(); if (!closed.ok) return { ok: false, error: closed.error }; + const isolated = await ensureTemplateIsolation(); + if (!isolated.ok) return { ok: false, error: isolated.error }; + // Re-asserted here even though `validateUsername` already ran at the route. This string reaches SQL as an // identifier, and the distance between that check and this one is a whole call chain — the kind of gap // where a future caller arrives without having passed the first one.