Repository navigation
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Pull request was converted to draft
|
:robot_face: Minor comment-wording issue found while reviewing the platform-repo sudoers sync (supabase/platform#37381), which copies this PR's sudoers lines byte-for-byte. The added comment's contrast clause is imprecise: "the salt subcommand above stays unprivileged" reads as if it's describing the entry directly above it ( Since supabase/platform#37381 copies this comment verbatim to keep the two copies byte-identical, a wording fix here would need to land in both places together. Suggested rewording: "the agent's own salt subcommand keeps running as the unprivileged supabase-admin-agent user." Not blocking — just flagging so it doesn't propagate further as more copies of this file get created (three platform-repo copies already exist). |
|
@claude review |
…s root The salt subcommand's systemd service runs as the unprivileged supabase-admin-agent OS user, which can't write to /etc/pgbackrest/conf.d (owned pgbackrest:postgres 02770). Adds a NOPASSWD sudoers entry scoped to exactly `pgbackrest reconcile`, and changes /opt/supabase-admin-agent ownership to root:root so that sudo grant can't be used to elevate a binary the unprivileged agent user could otherwise replace first. Dependencies: requires supabase/supabase-admin-agent#112 merged (adds the pgbackrest reconcile command this sudoers entry grants access to) and INDATA-1151 resolved via supabase/platform#37380 (the platform repo's provisioning scripts currently revert this root ownership on every new project and wake).
9033628 to
45abbe3
Compare
Summary
The
saltsubcommand's systemd service runs as the unprivilegedsupabase-admin-agentOS user, which can't write to/etc/pgbackrest/conf.d(ownedpgbackrest:postgres 02770). This PR adds aNOPASSWDsudoers entry scoped to exactlypgbackrest reconcile, and changes/opt/supabase-admin-agentownership toroot:rootso that sudo grant can't be used to elevate a binary the unprivileged agent user could otherwise replace first.(bot-generated information collapsed below)
Warning
Dependencies
pgbackrest reconcilecommand this sudoers entry grants access to)platformrepo's provisioning scripts currently revert the root-ownership change on every instance boot, so this must land before the sudoers rule is safe on real instances)platformrepo's own sudoers copies and the AMI-shipped one; per #37381 itself, this closes the boot-to-first-converge window but is not what makesreconcile's sudo check succeed in steady state. See #5 below.)supabase/salt's owninit.slsreassertssupabase-admin-agent:supabase-admin-agentownership on every converge (~10 min cadence, fleet-wide) and grants the binarycap_fowner/cap_chowncapabilities that bypass ownership restrictions entirely regardless of chown. Merging Create LICENSE #1-Automate our build with Github Actions #3 above establishes root ownership only in the window between an instance's own boot and its first Salt converge; this dependency is what makes it durable. Also flagged directly on supabase/salt#826, an in-review PR that currently reinforces agent ownership rather than fixing it.reconcileonly ever runs immediately after a Salt state apply in the same process, and Salt's ownfile.managedstate for/etc/sudoers.d/supabase-admin-agent(replace: True, sourced from a copy tracked in thesaltrepo, separate from both Automate our build with Github Actions #3's platform copies and this PR's AMI copy) always rewrites the file on that same converge beforereconcileever checks sudo. This is the dependency that actually determines whetherreconcile's sudo check succeeds in production; Create LICENSE #1-Support for more images #4 above are necessary but not sufficient without it.Details
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 (see INDATA-1151 in Dependencies above). Even once that's fixed,supabase/saltreasserts agent ownership on its own converge cadence and separately grants the binarycap_fowner/cap_chowncapabilities that bypass ownership restrictions entirely; see INDATA-1312 in Dependencies above. The root-ownership property this PR relies on for safety is not durable in production until INDATA-1312 also lands.platformprovisioning flow is also what writes/opt/supabase-admin-agent/config.yaml(root templates it, thenchown supabase-admin-agent:supabase-admin-agent+chmod 0640, perinit-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. See INDATA-1171 in Dependencies above.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 adds. Rejected in favor of keeping the narrowly-scoped sudo rule.Testing
visudo -c -f ansible/files/supabase_admin_agent_config/supabase-admin-agent.sudoers.conf: parsed OKvisudo -c -f ansible/files/adminapi.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.Misc
Related issues: INDATA-996 (this PR is the sudoers/ownership plumbing
pgbackrest reconcileneeds, not the tier-gating logic itself)Also carries an unrelated, stacked fix for INDATA-1153: grants
adminapisudo for a newpgbackrest ensure-spool-dirsubcommand (ansible/files/adminapi.sudoers.conf), the companion commit for which lives in supabase-admin-agent#120 (which this branch's own dependency, INDATA-996, is stacked under).enable,setup-replica, andstanza-upgradeall need this grant to backfill/data/pgbackrest_spoolon instances whose data volume predates postgres#2312's AMI directory-creation task. Riding this PR purely because #120 depends on this branch; unrelated to thepgbackrest reconcilesudoers work this PR is otherwise about.