-- +goose Up -- Two audit findings that share a theme: something the schema depends on was provisioned by hand and -- therefore is not reproducible, and a SECURITY DEFINER function that reaches past row level security. -- --------------------------------------------------------------------------------------------------- -- 1. missionmatch_aioe must exist for the migration set to replay. -- -- Migration 0043 grants to missionmatch_aioe and creates a policy for it, but db/bootstrap/00_roles.sql -- creates only migrator, app, and compliance. 0043's own comment admits the role is "PROVISIONED OUT OF -- BAND", which was survivable while there was one database provisioned by hand. -- -- It stopped being survivable when migration 0061 introduced the database per tenant model, because a -- tenant database is now BUILT BY REPLAYING THIS SET. Provisioning a new tenant aborts at 0043 with -- role "missionmatch_aioe" does not exist. This is the same reproducibility failure 0060 was written to -- fix for a table, reintroduced seventeen migrations earlier for a role. -- -- Created with NOLOGIN and no password: the role exists so grants and policies can name it, and the -- deployment attaches a credential when it actually runs the optimization engine. A role that cannot log -- in is not a new authentication surface. -- +goose StatementBegin DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_roles WHERE rolname = 'missionmatch_aioe') THEN CREATE ROLE missionmatch_aioe NOLOGIN NOBYPASSRLS; END IF; END $$; -- +goose StatementEnd -- --------------------------------------------------------------------------------------------------- -- 2. auth_touch_last_login must not be a cross tenant write primitive. -- -- The function is SECURITY DEFINER, owned by the BYPASSRLS migrator, and granted to the application role, -- and it updated users by id with NO organization predicate. users carries FORCE ROW LEVEL SECURITY -- precisely so that is impossible, and this function stepped around it: any caller reaching it with an -- arbitrary uuid writes a row in another tenant, and the difference between "row updated" and "no row" -- is an oracle for whether a given user id exists anywhere on the platform. -- -- The columns written are low value, which is why this is a hardening fix rather than an incident, but a -- confirmed RLS bypass write is worth closing on its own terms. -- -- The fix keeps the SECURITY DEFINER (the login path legitimately needs to write before a tenant -- transaction exists) and adds the organization as a REQUIRED argument, so the caller must state which -- tenant it believes the user belongs to and the update refuses when that is wrong. DROP FUNCTION IF EXISTS auth_touch_last_login(uuid); -- +goose StatementBegin CREATE OR REPLACE FUNCTION auth_touch_last_login(p_user_id uuid, p_org_id uuid) RETURNS void AS $$ BEGIN IF p_user_id IS NULL OR p_org_id IS NULL THEN RETURN; END IF; UPDATE users SET last_login_at = now(), updated_at = now() WHERE id = p_user_id AND organization_id = p_org_id; END; $$ LANGUAGE plpgsql SECURITY DEFINER SET search_path = public; -- +goose StatementEnd REVOKE ALL ON FUNCTION auth_touch_last_login(uuid, uuid) FROM PUBLIC; GRANT EXECUTE ON FUNCTION auth_touch_last_login(uuid, uuid) TO missionmatch_app; -- --------------------------------------------------------------------------------------------------- -- 3. audit_logs gets a read policy. -- -- It is tenant scoped (it has organization_id), the application role holds SELECT, and it had no row -- level security at all: the comment in 0001 says "reads MUST filter by org", which makes the largest -- cross tenant surface in the schema depend on every future query remembering a predicate. old_values and -- new_values are jsonb snapshots of candidate rows, so a single forgotten WHERE leaks another tenant's -- PII rather than metadata. -- -- SELECT only. INSERT genuinely needs the NULL org path for platform events (a failed login before any -- tenant is known), so writes stay unpoliced and the append only triggers keep guarding them. ALTER TABLE audit_logs ENABLE ROW LEVEL SECURITY; ALTER TABLE audit_logs FORCE ROW LEVEL SECURITY; -- +goose StatementBegin DO $$ BEGIN IF NOT EXISTS (SELECT 1 FROM pg_policy WHERE polname = 'audit_logs_tenant_read') THEN EXECUTE $p$ CREATE POLICY audit_logs_tenant_read ON audit_logs FOR SELECT USING (organization_id = COALESCE(NULLIF(current_setting('app.current_org_id', true), ''), '00000000-0000-0000-0000-000000000000')::uuid) $p$; END IF; -- Writes stay open to the app role, including the NULL org platform events it must be able to record. IF NOT EXISTS (SELECT 1 FROM pg_policy WHERE polname = 'audit_logs_insert_any') THEN EXECUTE 'CREATE POLICY audit_logs_insert_any ON audit_logs FOR INSERT WITH CHECK (true)'; END IF; END $$; -- +goose StatementEnd -- +goose Down DROP POLICY IF EXISTS audit_logs_tenant_read ON audit_logs; DROP POLICY IF EXISTS audit_logs_insert_any ON audit_logs; ALTER TABLE audit_logs NO FORCE ROW LEVEL SECURITY; ALTER TABLE audit_logs DISABLE ROW LEVEL SECURITY; DROP FUNCTION IF EXISTS auth_touch_last_login(uuid, uuid); -- +goose StatementBegin CREATE OR REPLACE FUNCTION auth_touch_last_login(p_user_id uuid) RETURNS void AS $$ BEGIN UPDATE users SET last_login_at = now(), updated_at = now() WHERE id = p_user_id; END; $$ LANGUAGE plpgsql SECURITY DEFINER SET search_path = public; -- +goose StatementEnd REVOKE ALL ON FUNCTION auth_touch_last_login(uuid) FROM PUBLIC; GRANT EXECUTE ON FUNCTION auth_touch_last_login(uuid) TO missionmatch_app; -- The role is deliberately NOT dropped: other databases in the cluster may depend on it, and dropping a -- role that still owns grants elsewhere fails in a way that is confusing to diagnose during a rollback.