[codex] Allow project admin to manage extensions#4
Conversation
tonychang04
left a comment
There was a problem hiding this comment.
Review — [codex] Allow project admin to manage extensions
Code quality: 👍 The refactor into the InsforgeUtilityCall struct + handle_policy_utility / handle_extension_utility dispatchers is clean and removes the long parameter lists. The privileged user-context switch in run_as_user is the correct Postgres idiom — GetUserIdAndSecContext → SetUserIdAndSecContext(target, ... | SECURITY_LOCAL_USERID_CHANGE) → PG_TRY, with the role restored on both the PG_CATCH (re-throw) and success (after PG_END_TRY) paths. No leak of superuser context, and no recursion risk (re-entry runs as BOOTSTRAP_SUPERUSERID so !superuser() short-circuits the hook). The statement gate (is_extension_utility_statement) is correctly scoped to CREATE/ALTER/AlterContents/DROP-EXTENSION node tags.
⚠️ One security consideration to confirm before merge: no extension allowlist
handle_extension_utility elevates any CREATE EXTENSION <x> from project_admin to run as BOOTSTRAP_SUPERUSERID, with no restriction on which extension. The Postgres image (postgres/Dockerfile) installs postgresql-contrib-15, which ships several extensions whose install scripts/functions are sensitive when created by superuser:
file_fdw(read server files),adminpack(pg_file_write/pg_file_unlink),dblink/postgres_fdw(outbound connections → SSRF to internal services), pluspg_net(HTTP).
Good news: the image does not ship untrusted PLs (plpython3u / plperlu), so there's no direct code-exec via untrusted language. And many of those contrib functions stay superuser-gated at EXECUTE time independent of who owns them — so this isn't necessarily a clean RCE. But it does meaningfully widen the attack surface for a non-superuser role.
Recommendation: add an allowlist of permitted extension names (e.g. a insforge.extension_allow_list GUC checked against stmt->extname in handle_extension_utility), defaulting to the intended set (vector, pg_graphql, pg_net, postgis, pg_trgm, uuid-ossp, ...). That keeps the feature's value while preventing project_admin from installing file_fdw/dblink/adminpack/postgres_fdw. At minimum, please confirm those contrib extensions' privileged functions remain superuser-gated post-install so installation alone isn't exploitable.
(Also nice-to-have: audit-log each elevated extension statement — who ran what — for traceability.)
Not blocking on the refactor itself, which is solid; flagging the allowlist as the one thing worth deciding explicitly before this ships.
tonychang04
left a comment
There was a problem hiding this comment.
Approved per fermioniclyu — proceeding without the extension allowlist for now (security note left in prior review comment for the record: no untrusted PLs shipped, and contrib functions stay superuser-gated at EXECUTE).
Summary
insforge.extension_grant_roleGUC, defaulting toproject_admin.CREATE/ALTER/DROP EXTENSIONutility statements from that role through the existinginsforge_pg_utilspreload hook as the bootstrap superuser.ProcessUtilitycall context and shared user-context switching.Why
Raw SQL now runs as
project_admin, which cannot install extensions that require superuser privileges. This keeps database-levelCREATEungranted, avoids changing extension control files, and scopes the bypass to extension utility statements.The hook now has more than one responsibility, so the dispatcher shape keeps policy and extension handling separate while reusing the low-level execution plumbing.
Validation
git diff --checkdocker build -t insforge-postgres-hook-test ./postgrespostgresql.conf:project_admincanCREATE EXTENSION vectorandDROP EXTENSION vector;project_adminstill cannotCREATE SCHEMAorCREATE PUBLICATION; a regularapp_userstill cannotCREATE EXTENSION vector.