From 8d1cfaacd22d336b298207f8073cd65bc744b82c Mon Sep 17 00:00:00 2001 From: FlorianJeandenans Date: Fri, 7 Aug 2026 14:50:14 +0200 Subject: [PATCH] docs: propose dashboard RBAC, tenant isolation, and operator views (issue #76) A design proposal, not an accepted ADR -- deliberately left for human review before implementation starts, unlike ADR-013/014/015 this session: this one decides who can see which tenant's data, a real security boundary rather than a narrower technical decision. #76 itself says this needs its own design pass before implementation, given its size; this is that pass. Grounds the proposal in what already exists rather than inventing new mechanism: - Authentication is already done (ADR-014): session API keys via wallet-signature login. - Ownership is already done (#12): workloads.owner_id, already scoped correctly by internal/workloadapi's gRPC surface -- just never wired into the dashboard's HTTP surface, which reads everything unauthenticated today. - Operator-view data (queue depth, worker claims, retry counts) already exists as plain columns on `workloads` (attempt_count, next_attempt_at, worker_id, worker_lease_until) -- no new schema needed for that slice. Proposes: a single `users.role` column (tenant/operator, not a many-to-many table -- deliberately avoids speculative complexity), a three-tier endpoint classification (public/tenant/operator) covering every existing and planned dashboard endpoint, an explicit correction that "validator views" are public data (not a fourth role -- validators authenticate to the Agent/chain, never to the dashboard), `controlplane-admin grant-role` mirroring the existing break-glass operator-tool pattern, a `requireRole` middleware wrapping routes at registration (auditable in one place), a first-pass secret-redaction table that flags workload `definition`'s env vars as the one real leakage risk needing an explicit redaction decision before tenant workload views can ship, three open questions for the accepting reviewer instead of silently guessing at them, and a 6-slice implementation sequence mirroring ADR-013's slicing discipline. Leaves #76 open: RBAC implementation itself, user/operator views, the secret-redaction decision, and E2E tests are all still outstanding -- this document unblocks them, it doesn't build them. Co-Authored-By: Claude Sonnet 5 --- ...oard-rbac-and-tenant-isolation-proposal.md | 210 ++++++++++++++++++ 1 file changed, 210 insertions(+) create mode 100644 docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md diff --git a/docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md b/docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md new file mode 100644 index 0000000..d1df53c --- /dev/null +++ b/docs/control-plane/dashboard-rbac-and-tenant-isolation-proposal.md @@ -0,0 +1,210 @@ +# Proposal: dashboard RBAC, tenant isolation, and operator views + +## Status + +**Proposed — not accepted.** Unlike ADR-013/014/015 this session, this one is deliberately left +for explicit human review before implementation starts: it changes who can see which tenant's +data, which is a real security boundary, not a narrower technical decision. If accepted, it +becomes an ADR under `docs/adr/` at whatever number is next free at acceptance time (see +ADR-012's "Consequences" section on why this repository assigns ADR numbers at acceptance, not +in advance). Written to unblock issue #76's largest remaining item: "RBAC and tenant isolation on +the dashboard itself (today: no auth at all)," plus the user- and operator-view items that depend +on it. + +## Context + +Three things already exist, independently, and this proposal's whole job is wiring them together +rather than inventing anything new: + +1. **Authentication** (ADR-014, accepted and implemented): a browser can log in by signing a + challenge, getting back a session API key. `internal/userauth` already has `users`, + `api_keys`, `Authenticate`, and an interceptor pattern (`internal/userauth/interceptor.go`, + reused by the dashboard's own `authenticatedUserID` in `internal/dashboard/auth.go`). +2. **Ownership** (issue #12, accepted and implemented): `workloads.owner_id` (migration + `000009_users_and_api_keys.sql`) already ties a workload to a `users.user_id`, and + `internal/workloadapi`'s gRPC surface already scopes every query by it + (`internal/workloadapi/postgres.go`: `WHERE workload_id=$1 AND owner_id=$2` throughout). A + real per-tenant boundary already exists — just not on the dashboard's HTTP surface. +3. **A dashboard with no authorization at all.** `internal/dashboard`'s `loadOverview` reads + every provider and the newest 500 workloads system-wide + (`SELECT ... FROM workloads ORDER BY created_at DESC LIMIT $1`, no `owner_id` filter) and + serves it to anyone who can reach the listener. `/api/v1/validator-scores/*` and + `/api/v1/agent-endpoint/*` are the same: unauthenticated by design (documented in each as "not + a new trust boundary" — correct today, because nothing tenant-private is exposed yet). Once + this proposal adds tenant-scoped workload views (cost, logs, env-derived metadata), that + "not a new trust boundary" reasoning stops applying to the *new* endpoints and must be revisited + per-endpoint, not assumed to extend automatically. + +What's missing is purely the authorization layer in between: **what does an authenticated +session's `users.user_id` get to see**, and **what does an unauthenticated caller still get to +see** (today: everything; after this proposal: only network-wide aggregate data, not per-tenant +detail). + +## Decision + +### 1. Three roles, one column + +```sql +ALTER TABLE users ADD COLUMN role text NOT NULL DEFAULT 'tenant' + CHECK (role IN ('tenant', 'operator')); +``` + +Deliberately **not** a `user_roles` many-to-many table. Every real actor in this system today +(tenant submitting workloads, operator running the Control Plane) has exactly one job; multi-role +users are a real future need (an operator who is also a tenant of their own network) but adding +that flexibility now is speculative complexity this proposal doesn't need to carry. If that need +becomes real, migrating `role text` to a join table is a normal, additive schema change — nothing +built on top of this proposal has to be designed around it in advance. + +Three effective access levels, not two, because "operator" and "authenticated tenant" alone don't +cover what's public today: + +| Level | Who | Determined by | +|---|---|---| +| **Public** | Anyone who can reach the listener, no session | No `Authorization` header | +| **Tenant** | A logged-in user, `role = 'tenant'` (the default) | Valid session/API key, `users.role` | +| **Operator** | A logged-in user, `role = 'operator'` | Valid session/API key, `users.role`, explicitly granted (see §4) | + +**Network Validators are not a fourth dashboard role.** A validator authenticates to the *Agent* +over mTLS (ADR-013 §3) and to the *chain* by signing extrinsics directly — it has no reason to +hold a dashboard session, and nothing in ADR-013/§85's challenge loop calls a dashboard endpoint +that needs one (`GET /api/v1/agent-endpoint/{provider_id}` is deliberately public, per its own +doc comment, precisely so a validator's daemon needs no dashboard credential at all). "Validator +views" in #76's original scope (challenge queue, evidence, quorum, score history) are **public, +network-wide data** — #87 already shipped the score-history piece as a public endpoint, correctly, +and every other validator view belongs in the same Public tier, not a new role. + +### 2. Endpoint classification + +Every endpoint this proposal knows about, existing or planned, gets exactly one tier. This table +is the actual authorization contract; implementation is "make the code match this table," not a +separate design exercise per endpoint. + +| Endpoint | Tier | Notes | +|---|---|---| +| `GET /api/v1/overview` | Public, **but stops including per-workload detail** | Provider/validator/chain-health data stays public (unchanged). Workload rows must drop to counts-by-state only for public callers — see §3. | +| `GET /api/v1/validator-scores/{provider_id}` | Public | Unchanged (#87). | +| `GET /api/v1/agent-endpoint/{provider_id}` | Public | Unchanged (#85/#13) — a validator's own credential-free discovery path. | +| `POST /api/v1/auth/*` | Public | Unchanged (ADR-014) — has to be, it's how you stop being anonymous. | +| `GET /api/v1/my/workloads` *(new)* | Tenant | Own workloads only, `WHERE owner_id = $session_user_id`. Replaces reading workload detail out of `/api/v1/overview`. | +| `GET /api/v1/my/workloads/{workload_id}` *(new)* | Tenant | 404 (not 403) for a workload that exists but isn't the caller's — matches `internal/workloadapi`'s existing "ownership check via the query itself" pattern, not a separate authorization branch that could be gotten wrong independently. | +| `POST /api/v1/my/workloads/{workload_id}/stop` *(new)* | Tenant | Thin HTTP wrapper over the same `StopWorkload` path `internal/workloadapi` already exposes over gRPC — no new business logic, just a browser-reachable entry point with the same ownership check. | +| `GET /api/v1/operator/queue` *(new)* | Operator | Counts of workloads by `state`, oldest `next_attempt_at` per state, `attempt_count` distribution — all already-existing columns (`migrations/000004_workloads.sql`, `000006`, `000007`), no new schema needed. | +| `GET /api/v1/operator/workers` *(new)* | Operator | Distinct `worker_id`/`worker_lease_until` currently holding a claim (`workloads.worker_id`) cross-referenced with `internal/agentmanager`'s live connection state. | +| `GET /api/v1/operator/audit` *(new, later slice)* | Operator | No audit log exists yet — this is new work, not a read of existing data. Flagged as its own slice in §5, not assumed free. | +| Dashboard static assets (`/dashboard/*`) | Public | Unchanged — the HTML/JS shell itself carries no data; per-role content is fetched by the JS after login, same SPA-shell pattern already in place for the auth panel. | + +### 3. `/api/v1/overview`'s workload list must shrink for public callers + +This is the one *behavior change* to an already-public, already-shipped endpoint, so it gets its +own paragraph rather than hiding in the table above. Today `Overview.Workloads` returns up to 100 +workload rows (`workload_id`, `state`, `provider_id`, `lease_id`, `created_at`) to anyone. None of +those fields are secret today, but `workload_id`/`lease_id`/`created_at` are exactly the shape of +"which tenants are using this network and when" — not something this proposal wants to keep +broadcasting once a real multi-tenant answer (`GET /api/v1/my/workloads`) exists. Proposed +replacement: `Overview` keeps `WorkloadsTotal` and a **count-by-state** breakdown +(`{"REQUESTED": 3, "RUNNING": 12, ...}`), drops the per-row `Providers`... no — drops the per-row +`Workloads` list entirely. This is a breaking change to `Overview`'s JSON shape and needs its own +PR description calling that out explicitly, the same care given to every other behavior change to +shipped code this session (e.g. #90's Network-dimension evidence change). + +### 4. Granting the operator role + +Mirrors the existing `controlplane-admin` break-glass pattern (`cmd/controlplane-admin`'s +`create-user`/`issue-key`/`revoke-key`) rather than inventing a self-service path — becoming an +operator is not something a user should be able to grant themselves, unlike ADR-014 §6's +self-service API keys. + +``` +controlplane-admin grant-role operator +controlplane-admin grant-role tenant # revoke back to default +``` + +Requires the same `DATABASE_URL` connection every other `controlplane-admin` user command already +needs — no new credential type. + +### 5. Authorization enforcement point + +One new, small piece of shared code: `internal/dashboard` gains a `requireRole(next +http.HandlerFunc, role string) http.HandlerFunc` wrapper, built directly on the existing +`authenticatedUserID` (`internal/dashboard/auth.go`) plus one `users.role` lookup. Every Tenant/ +Operator-tier endpoint in §2's table is wrapped once at route registration +(`Server.Handler()`in `dashboard.go`) — the same place every route is already declared, so a +reviewer can audit the entire authorization surface by reading one function, not by finding every +handler's own ad hoc check. Public-tier endpoints are simply not wrapped, same as today. + +### 6. Secret redaction: first-pass audit + +#76 asks for this "as a stated requirement," not because a known leak exists. Walking every field +currently or newly proposed to cross the dashboard boundary: + +| Data | Currently exposed? | Contains a secret? | +|---|---|---| +| Provider public keys, endpoints, capabilities | Yes, public | No — these are the provider's own on-chain-public identity/advertisement. | +| Workload `image`, `state`, timestamps | Yes, public today; moving to Tenant-only detail (§3) | No secrets in these fields themselves. | +| Workload `definition` (raw bytes, includes env vars per `WorkloadDefinition` in `shared.proto`) | **Not currently exposed anywhere in the dashboard** — `loadOverview` never selects the `definition` column. | **Yes, potentially** — a tenant's `env` map is exactly where a workload's secrets live (API keys, DB passwords for the deployed app). This must **never** be returned verbatim by `GET /api/v1/my/workloads/{workload_id}` — needs an explicit redaction pass (e.g. key names only, values withheld) before that endpoint ships, not an oversight to catch later. | +| Session API keys / raw keys | Never re-exposed after creation (ADR-014 §5/§6, unchanged) | N/A, already correctly handled | +| Container `last_error`/`error_code` | Not currently exposed; proposed for operator queue view | Possible secret leakage if an application error message embeds a credential (e.g. a failed DB connection string in a stack trace) — operator-tier only (not public), which bounds but does not eliminate this; flagged as a real, open question in §7, not resolved here. | + +### 7. Open questions for the accepting reviewer + +Not resolved by this proposal — need an explicit answer before implementation, listed rather than +guessed at: + +1. Does `workload.definition`'s env redaction need to be configurable per-tenant (some tenants + may want their own values visible to themselves, just not to operators), or withheld from + *everyone including the owning tenant* on principle? This proposal's table above assumes the + latter (never verbatim, to anyone, including the owner) is the safer default, but that's a + product decision, not a purely technical one. +2. Should `last_error`/`error_code` be shown to the *owning tenant* (Tenant tier, their own + workload) even though they're withheld from Operator-tier's cross-tenant queue view for the + secret-leakage reason above? Plausibly yes (a tenant needs to know why their own workload + failed) — but that means the redaction rule is role-*and*-ownership-dependent, not just + role-dependent, which is more logic than §5's `requireRole` wrapper alone covers. +3. Is a single global `operator` role sufficient, or does this need read-only-operator vs. + operator-with-admin-actions (stop-any-workload, revoke-any-key) as separate levels? This + proposal assumes one level is enough for the MVP's likely-single-operator deployment reality, + but that assumption should be stated and confirmed, not silently baked in. + +## Sequencing + +Mirrors ADR-013's slicing discipline: each slice is independently mergeable, independently +testable, and does not block on a later slice existing. + +1. **Schema + grant path**: `role` column, `controlplane-admin grant-role`, `requireRole` + middleware with no routes wrapped yet (dead code, but testable in isolation — matches how + `CreateAPIKeyWithExpiry` landed ahead of wallet login using it). +2. **Tenant workload views**: `GET/POST /api/v1/my/workloads*`, wrapped in `requireRole(..., + "tenant")` — note every role, including `operator`, must pass a `"tenant"` check too, i.e. + `requireRole` checks "authenticated at all," and a stricter `"operator"` check is additive, not + a separate hierarchy to design. §6/§7's redaction questions must be answered before this slice + ships, not deferred past it, since it's this slice that first exposes `definition`. +3. **`/api/v1/overview` breaking change**: §3's workload-list removal, its own PR with an explicit + "behavior change to shipped code" callout and updated dashboard JS/HTML. +4. **Operator queue/worker views**: read-only, all from already-existing columns — the + lowest-risk slice, could plausibly land before or in parallel with slice 2 if the accepting + reviewer wants operator visibility sooner. +5. **Operator audit log**: new schema (an append-only `audit_events` table logging every + Tenant/Operator-tier write action), its own slice — genuinely new work, not a read of existing + data, sized separately. +6. **E2E tests** (#76's own separate item): once slices 1-4 exist, a real end-to-end test + (login as tenant A, submit a workload, confirm tenant B's session cannot see it; login as + operator, confirm queue view works; confirm an unauthenticated caller gets exactly Public-tier + data) becomes possible to write meaningfully — attempting it earlier would just be testing + individual pieces already covered by their own slice's unit tests. + +## Consequences + +- `Overview`'s JSON shape loses its `Workloads` field — every existing consumer (today: only the + dashboard's own `app.js`) must be updated in the same PR as §3's slice. +- A user's role becomes a real, persistent piece of authorization state for the first time in this + codebase — `controlplane-admin grant-role` needs the same operational care (who has access to + run it, is it logged) as `issue-key` already gets, since granting `operator` is granting + cross-tenant visibility. +- Slice 2 cannot ship honestly without §7 question 1 and 2 being answered first — this is called + out explicitly in §5's sequencing so "we'll figure out redaction later" cannot quietly become + the default via merge-order accident. +- This proposal does not attempt multi-role users, fine-grained per-resource ACLs beyond + ownership, or SSO/external-IdP integration — all plausible future needs, none required by #76's + actual acceptance criteria, and adding any of them now would be exactly the kind of speculative + complexity this proposal's role model (§1) deliberately avoids.