Rework §6 bootstrap/credentials against confirmed server behavior
/api/bootstrap is single-shot per install (gated on COUNT(*) FROM users, confirmed against internal/api/users.go and the chart's bootstrap-job.yaml), not per identity — the two-identity bootstrap plan and the delete-Secret-to-rotate runbook this section described don't work against that. Rewrites §6 points 1/5/6 around a dedicated, repeatable service-account credential instead (proposed server-side in terdut-server's new SERVICE-ACCOUNTS.md), notes in §9 that Secret mirroring is RBAC-sound but still hands out a server-admin-equivalent credential per consenting namespace, and flags in §10 that chart-vs- operator bootstrap ownership blocks §6 and needs deciding first. Updates §13 to mark the service-account type as v1-blocking rather than a someday improvement, and adds a version-discovery endpoint to the same list (both this operator and terdut-tui currently detect server capability by route-probing).
This commit is contained in:
@@ -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
|
back, and stores it in a Secret (`bootstrap-job.yaml`). The operator absorbs
|
||||||
this rather than shelling out to curl:
|
this rather than shelling out to curl:
|
||||||
|
|
||||||
1. On a `TerdutServer`'s first reconcile after its Deployment reports Ready
|
1. **Confirmed against source** (`internal/api/users.go`'s `handleBootstrap`):
|
||||||
(`/healthz` reachable through the Service), the controller calls
|
`/api/bootstrap` is single-shot *per install*, not per identity — it
|
||||||
`POST /api/bootstrap` itself with a fixed, recognizable identity —
|
gates on `SELECT COUNT(*) FROM users`, so any call once one user exists
|
||||||
e.g. username `terdut-operator`, email `terdut-operator@<serverRef>` —
|
403s regardless of who's asking, exactly as `charts/terdut-server`'s own
|
||||||
distinct from `spec.bootstrap.username/email` used for the *human* first
|
`bootstrap-job.yaml` already assumes (403 → "already bootstrapped,
|
||||||
admin the chart bootstraps today. Two separate bootstrap identities are
|
nothing to do", exit 0). This rules out the two-identity plan this
|
||||||
needed only if terdut-server's `/api/bootstrap` is single-shot per
|
section originally described: there is no way for the operator to get
|
||||||
install; if it already returns 403 after the first caller regardless of
|
its *own* bootstrap identity once the chart (or a human) has already
|
||||||
identity, the operator instead **creates its bot user via
|
bootstrapped the server. The operator's real first-reconcile flow has to
|
||||||
`POST /api/users`** once a human admin exists — this needs confirming
|
be: call `/api/bootstrap` **only if nothing has bootstrapped yet**
|
||||||
against the endpoint's actual behavior before implementation and is
|
(an empty-DB fresh install with no chart bootstrap job enabled), and
|
||||||
flagged as a first task, not assumed here.
|
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
|
2. The returned API key is written to a generated Secret
|
||||||
(`<name>-operator-credentials`), owner-referenced to the `TerdutServer`,
|
(`<name>-operator-credentials`), owner-referenced to the `TerdutServer`,
|
||||||
referenced back from `status.operatorCredentialsSecretRef`.
|
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)
|
- This is one shared mirrored Secret per (server, consuming namespace)
|
||||||
pair, not one per `TerdutTeam` — terdut-server's own API key isn't
|
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
|
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
|
5. **Rotation**: the key is a bearer credential with no expiry modeled
|
||||||
server-side today. Rotation is manual (delete the Secret + the
|
server-side today. This section's original plan — delete the Secret +
|
||||||
`api_keys` row via `DELETE /api/users/{id}/api-keys/{keyID}`, let the
|
the `api_keys` row, let the controller re-bootstrap — **does not work**:
|
||||||
controller re-bootstrap) until/unless terdut-server grows key expiry; a
|
deleting an `api_keys` row doesn't reduce `users` to zero, so the next
|
||||||
rotation invalidates every mirror too, which the `TerdutServer` controller
|
`/api/bootstrap` call still 403s (see point 1 above). Until terdut-server
|
||||||
re-copies on its own next reconcile.
|
grows a real credential-issuance endpoint, there is no working rotation
|
||||||
Documented as an operational runbook note, not automated in v1.
|
story here at all; do not implement this as written.
|
||||||
6. This is explicitly a stand-in for a real scoped service-account token
|
6. **This entire section is a stand-in for a real scoped service-account
|
||||||
type; see §13.
|
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
|
## 7. Ownership, status, garbage collection
|
||||||
|
|
||||||
@@ -494,7 +525,7 @@ documented and tested operationally:
|
|||||||
cross-namespace `Secret` write.** It is a single Deployment/binary already
|
cross-namespace `Secret` write.** It is a single Deployment/binary already
|
||||||
watching every namespace it's granted (the normal Kubebuilder shape), so
|
watching every namespace it's granted (the normal Kubebuilder shape), so
|
||||||
mirroring a credentials Secret into a consenting namespace (§6) is not a
|
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
|
reconciles objects there — but it is new *scope* (`create`/`update` on
|
||||||
`Secrets` cluster-wide rather than only within each object's own
|
`Secrets` cluster-wide rather than only within each object's own
|
||||||
namespace), and should be called out explicitly in the operator's
|
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 —
|
RBAC is ever granted cross-namespace Secret access by this design —
|
||||||
`TerdutServer.spec.allowedTeams` only ever authorizes the operator to act
|
`TerdutServer.spec.allowedTeams` only ever authorizes the operator to act
|
||||||
on a `TerdutTeam`'s behalf, never a person or a workload directly.
|
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
|
- 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
|
over HTTP with the bot user's API key, never via the Kubernetes API for
|
||||||
app-level state.
|
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
|
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
|
for this design doc beyond flagging it; do not start that migration work
|
||||||
without separately confirming this recommendation.
|
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
|
## 11. Testing strategy
|
||||||
|
|
||||||
@@ -554,9 +602,20 @@ conflict Kubernetes operators exist to avoid. Proposed path:
|
|||||||
|
|
||||||
## 13. Deferred / explicitly out of scope for this design
|
## 13. Deferred / explicitly out of scope for this design
|
||||||
|
|
||||||
- A real scoped service-account/token type in terdut-server (least-privilege
|
- **A real scoped service-account/token type in terdut-server — not merely
|
||||||
operator identity instead of a bot admin user) — worth raising as a
|
deferred, this is v1-blocking for §6 as written** (verified: without it,
|
||||||
terdut-server feature request, not designed here.
|
§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
|
- CloudNativePG support — same `spec.database` shape as Zalando should
|
||||||
extend to it, but the concrete field/Secret-naming conventions need their
|
extend to it, but the concrete field/Secret-naming conventions need their
|
||||||
own look.
|
own look.
|
||||||
|
|||||||
Reference in New Issue
Block a user