feat(sudoers): let supabase-admin-agent invoke pgbackrest reconcile as root - #2291
Draft
hunleyd wants to merge 13 commits into
Draft
feat(sudoers): let supabase-admin-agent invoke pgbackrest reconcile as root#2291hunleyd wants to merge 13 commits into
hunleyd wants to merge 13 commits into
Conversation
…s root The salt subcommand's systemd service runs as the unprivileged supabase-admin-agent OS user, but /etc/pgbackrest/conf.d is pgbackrest:postgres 02770 — only root/adminapi can write there. Adds a NOPASSWD sudoers entry for `pgbackrest reconcile`, mirroring the existing salt-call entry, so the salt cycle can elevate just this one command (via sudo) to keep async-archiving config in sync with billing tier (INDATA-996) without granting the supabase-admin-agent user any broader group membership.
This comment has been minimized.
This comment has been minimized.
…priv agent The new sudoers rule in this PR grants the supabase-admin-agent user NOPASSWD root exec of /opt/supabase-admin-agent/supabase-admin-agent, but the config dir, extracted archive, and symlink were all owned by that same low-priv user/group. Since sudo matches on path only (not content/hash), that account could replace the binary and use the new sudo grant to run arbitrary code as root. Own the dir/binary/symlink as root:root instead, matching the existing pgdata-chown/pgdata-signal pattern in this file — the agent process only needs to execute it, never write to it.
This comment has been minimized.
This comment has been minimized.
hunleyd
marked this pull request as ready for review
July 22, 2026 18:04
hunleyd
enabled auto-merge
July 22, 2026 18:05
hunleyd
marked this pull request as draft
July 23, 2026 01:52
auto-merge was automatically disabled
July 23, 2026 01:52
Pull request was converted to draft
spool-path was never set explicitly, so pgBackRest defaulted to /var/spool/pgbackrest, which lands on the AMI's root volume (10GB, shared with the OS/systemd/journal) rather than the /data EBS volume PGDATA lives on. archive-get's async replica-catch-up queue can hold several 16MB+ WAL segments at once; once archive-async is enabled fleet-wide (INDATA-996) that risks filling the root volume and taking the instance down. Points spool-path at /data/pgbackrest_spool instead, and updates the directory-creation task to match. Global option, set once at provisioning — no supabase-admin-agent changes needed.
2 tasks
…lume" This reverts commit 63a434a.
…rationale Single-line comments only per updated style rules: collapses the 4-line sudoers rationale into one line, and adds a why-comment on the new root:root ownership for /opt/supabase-admin-agent so a future edit doesn't revert it back to the unprivileged service account.
The comment described /etc/pgbackrest/conf.d as root/adminapi-only, but setup-pgbackrest.yml creates it pgbackrest:postgres 02770 -- the agent just isn't in either group. Contradicted the PR's own accurate description of the same path.
…chiving-on-customer-tier * origin/develop: (36 commits) Lots of bootstrap script clean ups (#2326) ci: Fix nix-build PUSH_TO_CACHE (#2345) ci/nix-install-ephemeral: Drop sticky disk config (#2346) chore: bump postgres_release to cut fresh AMIs (includes #2334) (#2349) fix(multigres): stop base config data_directory from overriding pooler data dir (#2344) fix(cron): remove unnecessary TRIGGER grant on cron.job_run_details from postgres (#2334) chore: enable extension version restriction (warn) and cut AMIs (#2315) feat(nix): add site-env packages for rolling instance updates (#2283) ci: resolve package install sha from a flake input override (#2327) fix(multigres): remove dangling wal-g include from postgresql.conf (#2338) fix: Allow test CI pipelines to use old Int CA access. For staging only (#2330) chore(nix): remove maintainers field from package definitions (#2280) chore: bump multigres to b713432 (#2323) ci: Use arm-native-runner for kvm builds on aarch64-linux (#2319) Whole lot of ansible clean up (#2272) fix(ansible): drop no-op zpool loop item from zswap task (#2322) ci: Fix dockerhub-release-matrix matrix generation (#2320) chore: bump pgctld (#2318) fix(ansible): skip zswap params the kernel does not expose (#2321) feat(docker): add Dockerfile-supabase base image and rewrite Dockerfile-multigres as layered image (#2160) ...
…d unpack tasks Owner/group alone left permissions umask- and tarball-derived instead of deterministic; a UMASK 027 hardening pass would yield 0750 and break the salt service, postgres archive_command, and adminapi execution paths.
A malformed /etc/sudoers.d/supabase-admin-agent breaks sudo fleet-wide, including the adminapi entries pgBackRest restore depends on. visudo -csf rejects a bad file before it's ever installed.
The comment implied this change alone stops the agent from ever gaining root; the salt-call NOPASSWD entry above already grants that group effectively unrestricted root. Reworded to describe what this change actually closes: binary tampering ahead of the new sudo grant.
Per convention, comments only cite a Linear issue when tagged as a TODO for follow-up work, not as a general reference to the ticket a change is part of.
state: link with the file module's default follow: yes applies owner/group to the link target, not the symlink itself, and the kernel ignores symlink ownership for access control regardless. The target is already root-owned by the unpack task above; these two lines re-asserted nothing.
Previous wording's trailing clause read as describing the entry it was attached to; reworded so 'the salt subcommand above' unambiguously refers to the entry on the preceding line.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
saltsubcommand's systemd service runs as the unprivilegedsupabase-admin-agentOS user (groupsupabase-admin-agentonly), but/etc/pgbackrest/conf.dispgbackrest:postgres 02770— onlyroot/adminapi(in thepostgresgroup) can write there.NOPASSWDsudoers entry so the salt cycle can elevate justpgbackrest reconcileviasudo, mirroring the existingsalt-callentry in the same file — no broader group membership needed./opt/supabase-admin-agent(directory, unpacked binary) from ownersupabase-admin-agentto ownerroot:root, so the sudoers grant above cannot be used to elevate a binary the unprivileged agent user could replace first. Pins explicitmode: "0755"on both so this doesn't depend on umask or the release tarball's stored permission bits. (The symlink itself carries no separateowner/group—state: linkwith the file module's defaultfollow: yeswould only re-apply those to the link target, already covered by the unpack task above; the symlink still getsmode: "0755"explicitly.)Why this is safe
/opt/supabase-admin-agent/supabase-admin-agent pgbackrest reconcile— it can't be used to run anything else as root. Confirmed against the actual call site insupabase-admin-agentPR Generate optimized config for PG #112:reconcileBinis a hardcoded constant equal to this exact path, with no--configflag and noos.Executable()resolution that could drift from it.platformrepo's provisioning scripts (init-scripts/project/00-init.shandinit-scripts/suspend-and-wake/00-init.sh) recursivelychownthe whole directory back to the unprivileged agent user after every install. That's tracked as a hard blocker in INDATA-1151 and must ship before this sudoers rule is actually safe on real instances.platformprovisioning flow is also what writes/opt/supabase-admin-agent/config.yaml(root templates it, thenchown supabase-admin-agent:supabase-admin-agent+chmod 0640—init-scripts/{project,suspend-and-wake}/00-init.sh), and a periodic salt state (salt/state/formula/supabase-admin-agent/init.sls) re-templates it at0644running as the agent user.reconcilereads this file (--config, default/opt/supabase-admin-agent/config.yaml) and derivesEffectiveAdminAPIUser()from it — so even with the binary root-owned, root-runreconcilestill trusts a config the agent user can write. INDATA-1151's fix needs to explicitly decideconfig.yaml's final ownership (root-own it too, vs. carve it out as agent-writable for the salt refresh path) — it isn't automatically solved by scoping down the recursive chown on the binary/dir alone.audit-specs/baselines/prod-deployed/files-opt-deployed.ymlandbaseline.ymlstill record/opt/supabase-admin-agent/README.mdand the binary as owner993/group987(the agent uid/gid). Because theplatformboot scripts abovechown -Rreal instances back to that uid/gid on every project creation and wake, those baselines stay accurate on live instances even after this PR merges — they only need updating once INDATA-1151 actually removes that revert. Left untouched here; update them as part of INDATA-1151 instead.platformrepo also ships its own copy of this sudoers file atinit-scripts/suspend-and-wake/supabase-admin-agent.sudoers.conf, containing only the pre-existingsalt-call/gpgentries.setup_supabase_admin_agent()ininit-scripts/suspend-and-wake/00-init.shunconditionally reinstalls that copy to/etc/sudoers.d/supabase-admin-agenton every project wake, silently stripping thepgbackrest reconcileline this PR adds. The agent only logs the resultingsudodenial atWarn, so this fails silently. INDATA-1151 does not cover this — theplatformrepo's wake-path sudoers copy needs its own update. Tracked in INDATA-1171.supabase-admin-agentalready elevatessalt-callandgpgvia this exact mechanism (pkg/salt/exec.go); this isn't a new privilege model, just one more entry.visudo -cparses the file clean.Alternatives considered
/etc/pgbackrest/conf.dispgbackrest:postgres 02770— the writable group ispostgres, not apgbackrest-specific group.setup-postgres.ymlrunsinitdbwith--allow-group-access, so thepostgresgroup has read access to the entire live PGDATA (every table file, every WAL segment) on every instance — addingsupabase-admin-agentto that group to reachconf.dwould grant a much larger blast radius than the single-argv-path sudo grant this PR actually adds. Rejected in favor of keeping the narrowly-scoped sudo rule.Related
pgbackrest reconcileneeds, not the tier-gating logic itself.platformrepo'sinit-scripts/suspend-and-wake/supabase-admin-agent.sudoers.confalso needs thepgbackrest reconcileentry added, or the wake path strips it every time — see "Why this is safe" above.Depends on
pgbackrest reconcilecommand this grants access to)platformrepo update toinit-scripts/suspend-and-wake/supabase-admin-agent.sudoers.conf(adds thepgbackrest reconcileNOPASSWD entry so the wake path stops stripping it)Test plan
visudo -c -f ansible/files/supabase_admin_agent_config/supabase-admin-agent.sudoers.conf— parsed OKsudo-invokedpgbackrest reconcilesucceeds on a test instance without a password prompt, and that/opt/supabase-admin-agentstays root-owned after a fresh project creation and a pause/wake cycleSmoke Test Results
PASS — test-postgres-pitr.sh (engine 17)
Command:
./scripts/test-postgres-pitr.sh 'https://github.com/supabase/postgres/pull/2291' --engine 17The script itself reported a failure at the final health-check step because the invocation was initially missing an unrelated CLI credential (
SUPADEV_SYSTEM_API_KEY) — backup creation and PITR restore both succeeded before that point. Re-running just the health check (supadev project-health <restored-project>) once the credential was set confirmed all componentsACTIVE_HEALTHY(Storage, Realtime, Postgres (Admin), Postgres (User), Supavisor (TXN), PgBouncer, and others). Net result: PITR backup/restore works correctly with this PR's ansible ownership change.SKIPPED — test-postgres-pause-restore.sh (engine 17)
Command:
./scripts/test-postgres-pause-restore.sh 'https://github.com/supabase/postgres/pull/2291' --engine 17Blocked by a pre-existing local dev-stack defect ("IPv6 address is not defined for this project", 500 from mgmt-api) unrelated to this PR's diff (ansible sudoers/ownership only) — OrbStack's
ipv6config was already correct, so the fix requires an engine restart that drops every container on the host. Skipped rather than disrupt other work; PITR is the test most directly relevant to this diff's pgbackrest/archiving change and passed cleanly.