feat(juicefs): scoped juicefs_meta role — stop authenticating as a superuser - #2614
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3a42f0b8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Reviewed both findings against source. Both confirmed, and the first is broader than reported. 1. Ledger poisoning — confirmed, and it applies to BOTH guards
The part worth adding: the
Both So the guard that makes this file safe to land in a shared migrations directory is the same thing that guarantees it never takes effect on the one node it exists for. The tension is real and it is not your bug alone: the ledger has two states (applied / not applied) and this migration needs a third (not applicable here). A clean The narrowest fix that resolves it: move the role DDL into 2. PUBLIC grants — confirmed, and the comment is the load-bearing part
PostgreSQL grants The role is That is accurate about what this migration grants and inaccurate about what the role has. The distinction matters because the comment is what a future operator will trust. Either narrow it to "grants no privileges on ..." and note the inherited VerdictThe direction is right and the migration is careful — the node-conditional guard and the non-superuser detection are both good instincts, and the comment explaining why a DO block aborts the whole run under Holding rather than merging: fix 1 is a placement decision on your lane, and fix 2 touches database-wide |
Root cause + remediation, as requestedRead the whole apply path rather than reasoning from the guard. The finding stands, but the shape is more specific — and more alarming — than "the ledger is wrong". The ledger contract
if admin_psql < "$f" >/tmp/sb_apply.log 2>&1; then
admin_psql -c "INSERT INTO public.pmoves_bootstrap_history(kind, filename) ..."; ok++
else
fail++; echo " ⚠️ $akind FAILED (skipped): $name ..."
fiIt records on psql exit 0. That conflates transport succeeded with intent achieved. For an unconditional migration those are the same statement. For a conditional one they are not: a The table is The ordering that makes it fireMigrations run first. And So on a fresh database the schema this migration guards on is guaranteed absent at the moment the migration runs. The precise failure windowThis is the part worth pinning down, because it is not "the migration never works":
So it works on B850 — a database bootstrapped before this migration existed — and silently does nothing on any node rebuilt from scratch afterwards. That is the "works on my machine, absent after DR" shape. For a security control it is the worst one available: it validates in testing and is missing exactly when someone rebuilds. A second finding: the node-conditional premise is voidThe PR guards on schema existence to stay a no-op on gateway nodes, citing But So guard 1 does not distinguish metadata home from gateway. It distinguishes bootstrapped from not-yet-bootstrapped — and on the fresh path it fires on the metadata home as well. RemediationRecommended — move the role DDL into a seed. New file Why this is the right shape rather than a workaround:
Not recommended, and why:
Scope noteI checked whether this is systemic before proposing anything: no other migration in Happy to implement the seed move if you want it off your plate — say the word and I'll do it against this branch. Leaving it with you since it is your lane and the placement is a design call. |
…peruser
L6 step 1. APPLIED AND VERIFIED on B850 (the metadata home).
JuiceFS's metadata DSN authenticates as `supabase_admin` (rolsuper=t,
rolcreaterole=t) — a full superuser — using the credential exposed for ~11 days
and still un-rotated. Cross-node mounts require every node to reach the metadata
engine ("ensure that all nodes has access to the Metadata Engine"), so once that
port is tailnet-reachable, whatever role the DSN carries becomes a network-exposed
auth surface. Least privilege BEFORE reachability.
Verified on B850 after apply:
role attrs : super=f createrole=f createdb=f login=f bypassrls=f repl=f
tables : SELECT/INSERT on 18/18
sequences : USAGE on 7/7 (JuiceFS allocates inodes from these)
CREATE on schema: f
NOLOGIN until the operator delivers a password via the CHIT pipeline — a
login-capable role with no password is a worse default.
Two guards, both found by RUNNING it rather than reasoning about it:
1. NODE-CONDITIONAL. supa-migrate applies every migration on whatever node it
runs, but the metadata engine lives on one host (B850: schema + 18 tables;
z890: schema ABSENT). An unconditional GRANT would abort the whole migration
run on every non-metadata node. Verified: clean no-op on z890, exit 0.
2. PRIVILEGE PRECONDITION. supa-migrate connects as `-U postgres`, but on B850 the
hardened Supabase image leaves postgres NON-superuser (rolsuper=f) while every
juicefs_meta object is owned by supabase_admin. The first apply failed with
"permission denied to change default privileges" — and since a DO block is one
transaction under ON_ERROR_STOP, that aborts the ENTIRE supa-migrate run, not
just this file. It now detects and skips with an actionable NOTICE. Verified
both ways: as postgres it skips cleanly (exit 0); as supabase_admin it applies
and is idempotent on re-run.
Point 2 is a standing operational finding worth its own follow-up: the sanctioned
migration Known Road cannot apply owner-scoped DDL on the metadata home.
Does NOT repoint the live mount's DSN — separate reviewable step, so this lands
without touching a live filesystem.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…never ran
The role was never created on any database bootstrapped from scratch, and the
bootstrap reported success while it happened.
ROOT CAUSE. supabase-bootstrap records a file as applied when psql exits 0, not
when it achieved anything:
if admin_psql < "$f"; then INSERT INTO public.pmoves_bootstrap_history ...
The migration guarded on schema juicefs_meta existing and RETURNed cleanly when
it did not. Clean return = exit 0 = recorded = skipped by filename forever.
And the schema is created by a SEED (initdb/00_2_juicefs_meta_schema.sql) while
migrations run BEFORE seeds, so on a fresh database the guard always fired.
It worked on databases bootstrapped before the migration landed, because the
schema was already there. So it passed where it was developed and was absent on
any node rebuilt from scratch -- a security control that validates in testing
and is missing during a rebuild.
REPRODUCED, not argued, on supabase/postgres:17.6.1.108 with a replica of
apply_dir (ledger table, record-on-exit-0, migrations-then-seeds):
fresh DB role=ABSENT schema=ABSENT
run 1 migration: applied=1 seed: applied=1
-> role=ABSENT schema=PRESENT <- "applied", created nothing
run 2 migration: skipped=1 seed: skipped=1
-> role=ABSENT schema=PRESENT <- skipped by ledger
FIX. Same DDL, moved to supabase/initdb/00_3_juicefs_meta_role.sql. Seeds run
after migrations, so the schema is present by construction and the
schema-existence guard is DELETED rather than fixed. Sorts immediately after
00_2_ under LC_ALL=C. Matches the house pattern documented in 00_1 and 00_2: a
dedicated new seed filename applies on both fresh and already-bootstrapped
databases. Idempotent, so re-running is a no-op and the ledger entry is honest.
Verified on a fresh DB, same harness:
run 1 seed: applied=2 -> role=PRESENT schema=PRESENT
run 2 seed: skipped=2 -> role=PRESENT schema=PRESENT
And the privilege shape is what the comments claim:
rolcanlogin=false rolsuper=false bypassrls=false createdb=false createrole=false
USAGE on juicefs_meta = true CREATE on juicefs_meta = false
default-ACL rows in juicefs_meta = 2
SECOND FINDING, also measured. The comment claimed "No privileges on public /
auth / storage / any other schema". Measured on the created role:
USAGE on schema public = TRUE
inherited from PUBLIC, which this migration never granted. Confirmed the
executable half with a SECURITY DEFINER probe in public, the shape this repo
uses for public.complete_studio_board_publish:
juicefs_meta can EXECUTE a public SECURITY DEFINER fn : true
after REVOKE EXECUTE ... FROM PUBLIC : false
Inert while the role is NOLOGIN. The comment now says what the seed does not
grant versus what the role does not have, and names the REVOKE as the
prerequisite before LOGIN is enabled. That REVOKE is a database-wide policy
decision and is deliberately not made here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e3a42f0 to
7b95300
Compare
…e rotation Two review findings, both confirmed against source and now fixed. 1. TARGET. Step 1 named `make -C pmoves supa-migrate`, which cannot do the work: it connects as -U postgres, and postgres is NOT superuser in the hardened Supabase image (verified on supabase/postgres:17.6.1.108 -- postgres rolsuper=false, supabase_admin rolsuper=true), so it cannot grant on supabase_admin-owned objects. It also applies no seeds and keeps no ledger. The target is `supabase-bootstrap`, which connects as supabase_admin. The target FILE changed too: the DDL is now a seed (supabase/initdb/00_3_juicefs_meta_role.sql), not a migration. As a migration it ran before its own schema existed on a fresh database, guarded, returned cleanly -- and a clean return is exit 0, which apply_dir records as applied. Reproduced and fixed in #2614; ledger semantics written up in pmoves/docs/services/supabase/MIGRATION_WORKFLOW.md. A third mention further down still said supa-migrate after the header was fixed. Caught on a re-grep -- fixing the prominent instance and leaving the buried one is how a doc keeps saying the wrong thing. 2. SEQUENCING. The order read role -> rotate -> expose, framed as though creating the role shrinks the rotation's blast radius. It does not. The role is NOLOGIN and juicefs-cross-node-setup.sh:58 still builds postgres://supabase_admin@... unconditionally, with no branch -- so until the mount is repointed the rotation touches it exactly as before. Cutover is now its own numbered step between the two: grant LOGIN, repoint the script and the in-stack defaults, verify a real read through the new credential. Rotation follows a VERIFIED cutover, never precedes it. Otherwise the steps rotate and expose a credential that is still the only one in use, while the document reads as though the exposure had been reduced. Also added to Verification: presence of the role is not the assertion. The privilege shape is, including that has_schema_privilege(...,'public','USAGE') is true by PUBLIC inheritance rather than by anything this seed grants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n was wrong here (#2613) * docs(juicefs): L6 design — scoped meta role, and why the pooler option was wrong here Records the L6 cross-node design plus the documentation review that overturned my own first recommendation. The reasoning is the durable part. Blocker mechanism: supabase-db sits on pmoves_api + pmoves_data, both internal:true. Docker installs no DNAT for internal-only networks, so published ports are recorded but never plumbed — b850:5432 measured unreachable from BOTH z890 and nano-1. The in-repo NATS precedent (docker-compose.yml:2906) already solves this by multi-homing onto pmoves_external. CORRECTION — the pooler recommendation was wrong for this deployment: * Two official sources conflict. Supabase PLATFORM docs say 5432=session / 6543=transaction and that transaction mode does not support prepared statements. Supavisor PROJECT docs say mode is set by `mode_type` ON THE USER, not by port. The port convention is hosted-platform behaviour; self-hosted resolves mode from the user row, and this stack sets POOLER_POOL_MODE=transaction. So "use 5432 for session mode" does not hold here. * JuiceFS docs never mention poolers and point to the lib/pq driver, which uses the extended query protocol (prepared statements) — transaction mode would break it. * Decisive against the actual deployment: _supavisor.tenants and _supavisor.users are BOTH 0 rows, on z890 AND B850. The pooler is Up (healthy) answering /api/health 204s, but has never been provisioned to pool anything — its health check probes the API, not tenant existence. Exposing it would publish a port that cannot serve a connection. (Same shape as Archon reporting container-healthy while ready:false for three days.) Decision: expose supabase-db multi-homed onto pmoves_external, bound to the TAILNET interface only — not 0.0.0.0. The NATS precedent defaults to 0.0.0.0 on the grounds that NATS is credential-guarded; that is weaker for a database, and weaker still while the admin credential is un-rotated. Supavisor stays a future lane (provision tenant + session-mode user + verify lib/pq), not a step in this one. Ordering refinement: scoped role FIRST, then rotate, then expose. Once JuiceFS authenticates as a single-schema role, the pending admin rotation no longer touches the mount at all — it shrinks the rotation's blast radius instead of widening it. The migration SQL is included inline, reviewed and ready. It is not yet committed to pmoves/supabase/migrations/ because that path is protected and needs an operator-set KNOWN_ROAD=migrations:handoff:<this file>; an agent must not self-grant a protected-path bypass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(juicefs): correct the apply target and put the cutover before the rotation Two review findings, both confirmed against source and now fixed. 1. TARGET. Step 1 named `make -C pmoves supa-migrate`, which cannot do the work: it connects as -U postgres, and postgres is NOT superuser in the hardened Supabase image (verified on supabase/postgres:17.6.1.108 -- postgres rolsuper=false, supabase_admin rolsuper=true), so it cannot grant on supabase_admin-owned objects. It also applies no seeds and keeps no ledger. The target is `supabase-bootstrap`, which connects as supabase_admin. The target FILE changed too: the DDL is now a seed (supabase/initdb/00_3_juicefs_meta_role.sql), not a migration. As a migration it ran before its own schema existed on a fresh database, guarded, returned cleanly -- and a clean return is exit 0, which apply_dir records as applied. Reproduced and fixed in #2614; ledger semantics written up in pmoves/docs/services/supabase/MIGRATION_WORKFLOW.md. A third mention further down still said supa-migrate after the header was fixed. Caught on a re-grep -- fixing the prominent instance and leaving the buried one is how a doc keeps saying the wrong thing. 2. SEQUENCING. The order read role -> rotate -> expose, framed as though creating the role shrinks the rotation's blast radius. It does not. The role is NOLOGIN and juicefs-cross-node-setup.sh:58 still builds postgres://supabase_admin@... unconditionally, with no branch -- so until the mount is repointed the rotation touches it exactly as before. Cutover is now its own numbered step between the two: grant LOGIN, repoint the script and the in-stack defaults, verify a real read through the new credential. Rotation follows a VERIFIED cutover, never precedes it. Otherwise the steps rotate and expose a credential that is still the only one in use, while the document reads as though the exposure had been reduced. Also added to Verification: presence of the role is not the assertion. The privilege shape is, including that has_schema_privilege(...,'public','USAGE') is true by PUBLIC inheritance rather than by anything this seed grants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
L6 step 1. Applied and verified on B850 (the metadata home). Design rationale is in #2613.
Why
JuiceFS's metadata DSN authenticates as
supabase_admin—rolsuper=t, rolcreaterole=t, a full superuser — using the credential that was exposed for ~11 days and is still un-rotated.Cross-node mounts require every node to reach the metadata engine ("ensure that all nodes has access to the Metadata Engine"). So the moment that port is tailnet-reachable, whatever role the DSN carries becomes a network-exposed auth surface. Least privilege before reachability, not after.
Verified on B850 after apply
super=f createrole=f createdb=f login=f bypassrls=f repl=fNOLOGINuntil the operator delivers a password via CHIT — a login-capable role with no password is a worse default.Two guards — both found by running it, not reasoning about it
1. Node-conditional.
supa-migrateapplies every migration on whatever node it runs, but the metadata engine lives on one host (B850: schema + 18 tables; z890: schema absent). An unconditionalGRANTwould abort the whole migration run on every non-metadata node. ✅ Verified clean no-op on z890, exit 0.2. Privilege precondition.
supa-migrateconnects as-U postgres, but on B850 the hardened Supabase image leavespostgresNON-superuser (rolsuper=f) while everyjuicefs_metaobject is owned bysupabase_admin. My first apply failed withpermission denied to change default privileges— and since aDOblock is one transaction underON_ERROR_STOP, that aborts the entiresupa-migraterun, not just this file. It now detects and skips with an actionable NOTICE.✅ Verified both ways: as
postgresit skips cleanly (exit 0); assupabase_adminit applies and is idempotent on re-run.Scope
Does not repoint the live mount's DSN — that's a separate reviewable step, so this lands without touching a live filesystem.
Applied via the
migrations:Known Road with the handoff on disk; the bypass is recorded inknown-roads.jsonl.Next in L6
rotate supabase_admin(operator — ~27 consumers, one window) → repoint the mount DSN tojuicefs_meta→ exposesupabase-dbmulti-homed, tailnet-bound only → verify a real file read from 4090/5090/jetson (not just a directory listing — that's the failure mode this lane exists for).🤖 Generated with Claude Code