diff --git a/DESIGN.md b/DESIGN.md index c245ef3..eced8ab 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -382,18 +382,25 @@ a one-shot Job that calls `POST /api/bootstrap`, gets a one-time admin key back, and stores it in a Secret (`bootstrap-job.yaml`). The operator absorbs this rather than shelling out to curl: -1. On a `TerdutServer`'s first reconcile after its Deployment reports Ready - (`/healthz` reachable through the Service), the controller calls - `POST /api/bootstrap` itself with a fixed, recognizable identity — - e.g. username `terdut-operator`, email `terdut-operator@` — - distinct from `spec.bootstrap.username/email` used for the *human* first - admin the chart bootstraps today. Two separate bootstrap identities are - needed only if terdut-server's `/api/bootstrap` is single-shot per - install; if it already returns 403 after the first caller regardless of - identity, the operator instead **creates its bot user via - `POST /api/users`** once a human admin exists — this needs confirming - against the endpoint's actual behavior before implementation and is - flagged as a first task, not assumed here. +1. **Confirmed against source** (`internal/api/users.go`'s `handleBootstrap`): + `/api/bootstrap` is single-shot *per install*, not per identity — it + gates on `SELECT COUNT(*) FROM users`, so any call once one user exists + 403s regardless of who's asking, exactly as `charts/terdut-server`'s own + `bootstrap-job.yaml` already assumes (403 → "already bootstrapped, + nothing to do", exit 0). This rules out the two-identity plan this + section originally described: there is no way for the operator to get + its *own* bootstrap identity once the chart (or a human) has already + bootstrapped the server. The operator's real first-reconcile flow has to + be: call `/api/bootstrap` **only if nothing has bootstrapped yet** + (an empty-DB fresh install with no chart bootstrap job enabled), and + otherwise obtain its credential through a dedicated, repeatable + service-account endpoint — see the note at the end of this section. + This also surfaces an unhandled race worth designing around explicitly + once §10 is settled: if both the chart's bootstrap Job and this + controller call `/api/bootstrap` against the same fresh install, + exactly one gets the 201 and the other must treat 403 as "someone else + already bootstrapped, go get my own credential the other way" rather + than as an error. 2. The returned API key is written to a generated Secret (`-operator-credentials`), owner-referenced to the `TerdutServer`, referenced back from `status.operatorCredentialsSecretRef`. @@ -422,16 +429,40 @@ this rather than shelling out to curl: - This is one shared mirrored Secret per (server, consuming namespace) pair, not one per `TerdutTeam` — terdut-server's own API key isn't team-scoped (§13), so there is nothing finer to hand out; "scoped" here - means scoped by *namespace boundary*, not by team permission. + means scoped by *namespace boundary*, not by team permission. **This is + the design's real weak point, not the mirroring mechanism itself:** the + RBAC argument above (mirror rather than grant broad cross-namespace + Secret-read) is sound on its own terms, but every mirrored copy is + still server-admin-equivalent regardless of which team's namespace it + lands in — `allowedTeams` gates whether a namespace may attach a + `TerdutTeam` at all, it does nothing to bound what that namespace's + copy of the credential can then do to every *other* team on the same + server. This goes away, mirroring included, once team-scoped service- + account tokens exist (see the note below): mint one key per + `TerdutTeam`, directly into its own namespace, owner-referenced to the + CR. No mirror, no shared-per-namespace blast radius — a leaked Secret + compromises exactly one team. 5. **Rotation**: the key is a bearer credential with no expiry modeled - server-side today. Rotation is manual (delete the Secret + the - `api_keys` row via `DELETE /api/users/{id}/api-keys/{keyID}`, let the - controller re-bootstrap) until/unless terdut-server grows key expiry; a - rotation invalidates every mirror too, which the `TerdutServer` controller - re-copies on its own next reconcile. - Documented as an operational runbook note, not automated in v1. -6. This is explicitly a stand-in for a real scoped service-account token - type; see §13. + server-side today. This section's original plan — delete the Secret + + the `api_keys` row, let the controller re-bootstrap — **does not work**: + deleting an `api_keys` row doesn't reduce `users` to zero, so the next + `/api/bootstrap` call still 403s (see point 1 above). Until terdut-server + grows a real credential-issuance endpoint, there is no working rotation + story here at all; do not implement this as written. +6. **This entire section is a stand-in for a real scoped service-account + token type, and more than a nice-to-have**: it's the dependency that + makes points 1 and 5 above actually resolvable. Recommended shape (raised + as a terdut-server feature request, tracked in §13): a + `POST /api/service-accounts` (instance-scoped, admin-only, safely + callable repeatedly — unlike `/api/bootstrap`) to create the operator's + own identity and mint its first key, `POST /api/service-accounts/{id}/keys` + to rotate without recreating the account, and `GET /api/service-accounts?name=` + so a 403 from a stale lookup resolves to "fetch my existing account" instead + of an unhandled error. Team-scoped accounts (rather than the one + instance-scoped operator identity) are what let point 4 above mint a key + per `TerdutTeam` instead of mirroring. Until this lands server-side, + treat this section's bootstrap flow as v1-blocking, not v1-shippable — + see §10's note on sequencing. ## 7. Ownership, status, garbage collection @@ -494,7 +525,7 @@ documented and tested operationally: cross-namespace `Secret` write.** It is a single Deployment/binary already watching every namespace it's granted (the normal Kubebuilder shape), so mirroring a credentials Secret into a consenting namespace (§6) is not a - new privilege *boundary* — it's the same ServiceAccount that already + new Kubernetes RBAC *boundary* — it's the same ServiceAccount that already reconciles objects there — but it is new *scope* (`create`/`update` on `Secrets` cluster-wide rather than only within each object's own namespace), and should be called out explicitly in the operator's @@ -502,6 +533,15 @@ documented and tested operationally: RBAC is ever granted cross-namespace Secret access by this design — `TerdutServer.spec.allowedTeams` only ever authorizes the operator to act on a `TerdutTeam`'s behalf, never a person or a workload directly. + **This is a statement about Kubernetes RBAC only, though — it says + nothing about what the mirrored terdut-server credential itself can do + once it's there.** Today that credential is the server-admin-equivalent + bot key (§6), so a compromised or over-read namespace can reach every + team on the server, not just its own; `allowedTeams` bounds who may + *attach*, not what an attached namespace's copy of the credential can + then *do*. That's a real privilege-boundary gap, and it closes once §6's + team-scoped service-account keys exist and mirroring is dropped in favor + of a key minted directly per `TerdutTeam`. - terdut-server's own RBAC is unaffected — the operator talks to it purely over HTTP with the bot user's API key, never via the Kubernetes API for app-level state. @@ -527,6 +567,14 @@ conflict Kubernetes operators exist to avoid. Proposed path: migration guide and probably a major chart version bump — out of scope for this design doc beyond flagging it; do not start that migration work without separately confirming this recommendation. +- **This decision isn't only about the migration — it also decides who + owns bootstrap.** §6 assumed the operator could always get its own + bootstrap identity separately from the chart's; §6 point 1 shows that's + false, so whichever of {chart's Job, operator controller} is expected to + call `/api/bootstrap` first has to be settled explicitly (a one-paragraph + call, not the full migration plan) *before* writing any operator + bootstrap/credential code, not deferred alongside the rest of this + section. ## 11. Testing strategy @@ -554,9 +602,20 @@ conflict Kubernetes operators exist to avoid. Proposed path: ## 13. Deferred / explicitly out of scope for this design -- A real scoped service-account/token type in terdut-server (least-privilege - operator identity instead of a bot admin user) — worth raising as a - terdut-server feature request, not designed here. +- **A real scoped service-account/token type in terdut-server — not merely + deferred, this is v1-blocking for §6 as written** (verified: without it, + §6's bootstrap flow has no working credential-rotation path and no clean + answer to the chart-vs-operator bootstrap race; see §6 points 1, 5, 6 and + §10). Sequence this server-side change *before* implementing the + `TerdutServer` controller's bootstrap logic, not after. +- **A version-discovery endpoint on terdut-server** (e.g. `GET /api/version`). + Neither this operator nor terdut-tui has one today — both independently + detect capability by probing specific routes (terdut-tui via `GET + /api/teams` 404-checking; this operator would otherwise need to invent + its own equivalent probe). An unattended reconciler is more exposed to a + silent breaking API change than an interactive TUI a human is watching; + raising this alongside the service-account request rather than inventing + another route-probe here. - CloudNativePG support — same `spec.database` shape as Zalando should extend to it, but the concrete field/Secret-naming conventions need their own look.