From f7d4648851a6045f15588c1892728c392c5cb4fb Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Sat, 12 Sep 2026 21:17:15 -0500 Subject: [PATCH] docs(keycloak): record the URL-reconciliation decisions Records K1-K9: extend-vs-sibling and why, the generated+gated desired state, verbatim token pass-through, per-pass fail-closed, order-insensitivity, unmanaged-means-untouched, public clients' URLs managed, one alert pair not two, and the mutation harness. Also records a process note: my first pass reported three false findings (reconciler absent from main, import not templated, BASE_DOMAIN dead) because I diffed against origin/main -- origin is dev's Gitea, stale at 4809199, while the live remote is prod-gitea at fa4deac. All three were wrong. The rule for this repo is written down so the next agent does not repeat it: origin is not the source of truth. Co-Authored-By: Claude Opus 5 --- DECISIONS-LOG.md | 92 +++++++++++++++++++++++++++++++++++++ platform/keycloak/README.md | 64 +++++++++++++++++++++++--- 2 files changed, 149 insertions(+), 7 deletions(-) diff --git a/DECISIONS-LOG.md b/DECISIONS-LOG.md index b20b43f..5f8b345 100644 --- a/DECISIONS-LOG.md +++ b/DECISIONS-LOG.md @@ -2331,3 +2331,95 @@ vpn-class egress dead until someone noticed. > gateway whose tunnel interface is dynamic requires an explicit re-resolution trigger on every > interface change. This is a standing operational requirement of the ADR-0015 design, not an > implementation detail.* + +## Keycloak client REDIRECT URIs reconcile too — the same one-shot, one field over (2026-09-12) + +Follow-up to the client-secret reconciler. That CronJob heals the one field the one-shot realm +import got wrong in the 2026-09-06 outage. This adds the *other* field the same one-shot cannot +carry: the environment-derived client URLs. + +**The trigger.** prod is moving from `prod.home.keithjackson.dev` to the apex `home.keithjackson.dev`. +`BASE_DOMAIN` is live — the realm import carries **26** `${BASE_DOMAIN}` references and 39 files under +`platform/` + `apps/` reference it — so flipping the knob re-renders every declared redirect URI in +git. It reaches Keycloak's database **never**: the `KeycloakRealmImport` rests at +`Done=True / Started=False` and does not re-run. Measured on prod's live CR: +`https://grafana.prod.home.keithjackson.dev/login/generic_oauth`, i.e. substitution demonstrably +happened at the *current* value and then froze. After the flip, every OIDC login fails with a +`redirect_uri` mismatch while the CR still reads `Done=True` and nothing alerts. Identical shape to +the literal-`(env:...)` placeholder outage: **declared correctly in git, not in effect, behind a +green signal.** + +### Decisions + +| # | Decision | Why | +|---|---|---| +| K1 | **EXTEND** the existing reconciler with a second pass; do NOT add a sibling CronJob | Decisive reason: a URL write needs **no new endpoint**. It is the same minimal `PUT /admin/realms//clients/`, and the current URLs already come back in the allow-listed client lookup. So the hard allow-list that makes `/users`, `/groups`, `/roles`, `/authentication` unreachable is widened by *nothing* and is preserved rather than re-declared in a second copy. The ConfigMap also carries exactly one script key by L0 contract, so a sibling would have had to fork ~300 lines of safety-critical code the file header explicitly forbids forking. Blast radii were already identical (client representations only), so separating them bought nothing real. | +| K2 | The desired state is **GENERATED** from the realm import (`ci/render-keycloak-client-urls.py`), gated by L0 `--check` | The pod has `automountServiceAccountToken: false` and cannot read the `KeycloakRealmImport` CR, so its desired state must be mounted. A hand-authored ConfigMap would be a second declaration of every public hostname, free to disagree with the import — the same shape as the bug. Generated + gated makes them provably identical. Same `render==golden` idiom as `catalog/render.py`. | +| K3 | URL strings are copied **verbatim**, tokens included; the renderer never parses a hostname and never reads `cluster-vars` | `client-urls.yaml` therefore carries `https://grafana.${BASE_DOMAIN}/...` unchanged, and L0 asserts it is rendered by the **same** Flux Kustomization as the import. `postBuild.substituteFrom` is per-Kustomization, so a split would let one token resolve to two values and the realm would converge on a hostname the HTTPRoutes do not serve. There is exactly one place the domain is decided, and **no new substitution variable** was introduced. | +| K4 | The two passes **fail closed independently** | If a malformed URL ConfigMap could abort the run, a hostname typo would stop client-*secret* repair — making credential healing depend on a file that has nothing to do with credentials, a strictly worse posture than before pass 2 existed. Each pass reads its own input, fails closed for its own fields, and the exit code is the OR. | +| K5 | List fields compare **order-insensitively** (`sorted`, not `set`) | Keycloak does not preserve the authored order of `redirectUris`. An order-sensitive compare would rewrite every client on every run forever while reporting success. `sorted` keeps duplicates as drift. | +| K6 | A field the desired state omits is **UNMANAGED**, never forced to empty | Only `redirectUris`/`webOrigins` are declared today. Reading absence as "must be empty" would wipe an operator-set `adminUrl`, or any field a future Keycloak version populates itself. | +| K7 | Public clients' **URLs are managed** even though their secrets are not | `kubernetes` is PKCE-public and its localhost callbacks are what `kubectl` logins land on; a stale URI on any client widens what the realm will redirect to. | +| K8 | **Extend** the existing `KeycloakClientSecretReconcilerFailing`/`…Stale` pair rather than adding a second | One CronJob ⇒ one Failing/Stale pair. A second pair keyed to the same `job_name` regex would fire twice for a single cause and train the operator to ignore both. The CronJob exports no metrics of its own, so there is no *new* series to alert on — inventing a rule would have meant inventing a signal. No new critical, nothing permanently firing. | +| K9 | Commit a **mutation harness** and run it in L0 (~20s) | A passing selftest proves nothing if it cannot fail, and "reported success while the realm was broken" is the exact failure this component exists to end. `mutations.py` injects 12 plausible regressions into the *extracted* script and requires `--selftest` to catch each. Two **survived on the first run** — order-insensitivity and readback verification were untested — and the suite was strengthened until all 12 were killed. That is the harness earning its place, not decoration. | + +### Evidence + +- reconciler `--selftest`: **84 assertions** pass (up from 39). Covers the domain flip, a client + *gaining* a URI, a client *losing* a stale one, a public client's URLs managed while its secret is + not, an unmanaged field surviving untouched, per-pass fail-closed, order-insensitivity, and a + server that accepts a `PUT` with 204 and does not apply it. +- `mutations.py`: **12/12 injected regressions killed.** +- L0 `keycloak-client-secret-reconciler`: **212 assertions.** Verified to FAIL when + `client-urls.yaml` is hand-edited away from the realm import. +- The renderer's own `--check` verified to FAIL and print a diff on a drifted redirect URI. + +**Verified against the LIVE prod realm, read-only** (`CLUSTER=prod ... --context k3s-prod`, admin API +via port-forward, `--audit` mode which provably never writes): + +- prod's `KeycloakRealmImport` reads `Done=True / Started=False / HasErrors=False` — the one-shot + state, confirmed. +- Its `spec` carries the SUBSTITUTED value `https://grafana.prod.home.keithjackson.dev/...`, and all + 13 clients' stored URLs sit on `prod.home.keithjackson.dev`. Substitution demonstrably happens and + then freezes. +- Running the shipped script's `--audit` at prod's CURRENT `BASE_DOMAIN`: **all 13 clients report + `urls ok (already correct, no write)`** and it emits `KEYCLOAK_CLIENT_URLS_AUDIT_OK`. The realm is + converged today, so the reconciler would be a no-op right now — which is the correct baseline. +- Re-running with the desired state rendered at the POST-FLIP apex value: **24 drifted fields across + 12 clients**, each naming the exact before/after, while `kubernetes` correctly reports `ok` (its + localhost callbacks are not domain-derived, so it must not drift). That is the precise damage the + apex flip will do to SSO, detected, with zero false positives. +- The same run had pass 1 fail closed on an empty secrets dir while pass 2 completed normally — + K4's independence, demonstrated live rather than only in the selftest. + +**K5 (order-insensitivity) turned out to be measured, not defensive.** The live realm stores +`kubernetes`' 8 redirect URIs and `oauth2-proxy`' 2 in a DIFFERENT ORDER than git declares them — +Keycloak genuinely reorders them. An order-sensitive comparison would therefore have rewritten those +two clients on **every run, every 15 minutes, forever**, reporting success each time. Both report +converged under `sorted()`. + +### Process note — I initially reported three false findings, from reading the wrong ref + +Worth recording because the mistake is cheap to repeat. My first pass concluded that the reconciler +was absent from `main`, that the realm import was not templated, and that `BASE_DOMAIN` was a dead +knob. **All three were wrong.** I had diffed against `origin/main`, and `origin` is *dev's* Gitea, +stale at `4809199`; the live remote is `prod-gitea`, with `main` at `fa4deac` — 68 commits ahead. On +the real `main` the reconciler is present (delivered file-level, not via a merge, so it is not an +ancestor of a branch cut from the old ref), the import carries 26 `${BASE_DOMAIN}` references, and 39 +files reference the knob. + +Consequences avoided: the branch was rebased off the stale base before merging. As-is it was 221 +files / 20,328 deletions against `main` and would have reverted velero's R2 enablement, the VPN egress +gateway and the arr config-PVC fix. Rebasing also picked up a reconciler fix that only exists on real +`main` — `fsGroup: 65532` with the client-secrets volume at `defaultMode: 0440`, without which the +container dies with `PermissionError` on every run; my older base had `0400` and no `fsGroup`. + +**Rule for future agents in this repo: `origin` is not the source of truth. Diff against +`prod-gitea/main`, or local `main` after confirming it matches.** + +### Still open + +- `https://${BASE_DOMAIN}/runbooks/keycloak-client-secrets` is referenced by both alerts and still + does not exist. +- The apex flip itself is `migration/dns-apex-inversion`'s job. This change only guarantees the realm + follows git once the knob moves; it does not move it. diff --git a/platform/keycloak/README.md b/platform/keycloak/README.md index 7fe518e..53cd00e 100644 --- a/platform/keycloak/README.md +++ b/platform/keycloak/README.md @@ -29,7 +29,8 @@ files pulled as remote kustomize bases (the kubevirt-stub pattern); Renovate mai | `keycloak-instance.yaml` (`Keycloak` CR, `${KEYCLOAK_REPLICAS}`) | **platform-config** | config | | `postgres-cluster.yaml` (CNPG `Cluster` on longhorn) | **platform-config** | config | | `keycloak-realm-import.yaml` (`KeycloakRealmImport`: realm/groups/clients/MFA) | **platform-config** | config | -| `client-secret-reconciler.yaml` (CronJob: client secrets ← git, every 15m) | **platform-config** | config | +| `client-secret-reconciler.yaml` (CronJob: client secrets **and** client URLs ← git, every 15m) | **platform-config** | config | +| `client-urls.yaml` (**GENERATED** ConfigMap: the client-URL desired state) | **platform-config** | config | | `httproute.yaml` (keycloak.home.keithjackson.dev) | **platform-config** | config | | `servicemonitor.yaml` (Prometheus scrape) | **platform-config** | config | @@ -85,8 +86,8 @@ Three layers now cover it: | Layer | Where | What it does | |---|---|---| | **Prevent** | `conformance/tests/l0-static/keycloak-client-secret-reconciler` (runs on every merge, unconditionally, from `ci/l0.sh`) | Every placeholder is defined, reads `keycloak-realm-clients`, and names an **existing** key called `-client-secret`; public clients declare no secret; and the Flux Kustomization applying the Secret is a real transitive `dependsOn` ancestor (`wait: true`) of the ones applying the import and the reconciler. A missing SOPS key is now unmergeable. | -| **Heal** | `client-secret-reconciler.yaml` — CronJob, every 15m | Compares each client's **stored** secret with the mounted `keycloak-realm-clients` value and PUTs the **minimal** `{clientId, secret}` representation only where they differ. Skips correct clients, never logs a secret, holds a hard endpoint allow-list so users/groups/roles/flows are unreachable, and needs no Kubernetes API access (`automountServiceAccountToken: false`). | -| **Detect** | `conformance/tests/l2-behavioral/keycloak-client-secrets-match-git` + alerts `KeycloakClientSecretReconcilerFailing` / `…Stale` | Runs the same script in read-only `--audit` mode against the live realm and fails on any mismatch or literal placeholder; the alerts make the reconciler's silence observable in prod. | +| **Heal** | `client-secret-reconciler.yaml` — CronJob, every 15m, two passes | Compares each client's **stored** secret with the mounted `keycloak-realm-clients` value and PUTs the **minimal** `{clientId, secret}` representation only where they differ; a second, independent pass does the same for the client URLs against the generated `client-urls.yaml` (see below). Skips correct clients, never logs a secret, holds a hard endpoint allow-list so users/groups/roles/flows are unreachable, and needs no Kubernetes API access (`automountServiceAccountToken: false`). | +| **Detect** | `conformance/tests/l2-behavioral/keycloak-client-secrets-match-git` + alerts `KeycloakClientSecretReconcilerFailing` / `…Stale` | Runs the same script in read-only `--audit` mode against the live realm and fails on any mismatch, literal placeholder **or drifted client URL**; the alerts make the reconciler's silence observable in prod. | The reconciler's own `--selftest` (run by the L0 gate, which extracts the script from the shipped ConfigMap) proves compare-then-write, **idempotence** (zero writes on a converged realm), @@ -99,10 +100,59 @@ kubectl -n keycloak create job --from=cronjob/keycloak-client-secret-reconciler kubectl -n keycloak logs -l app.kubernetes.io/name=keycloak-client-secret-reconciler --tail=-1 ``` -**What the reconciler does NOT fix:** anything other than client secrets. A NEW client, a changed -redirect URI or a new protocol mapper still needs the realm import to actually re-run (it will not on -its own) or a deliberate admin-API/`kcadm` change. `Done=True` is never evidence that a realm edit -landed. +## Client URLs: the same one-shot failure, one field over + +Moving an environment's `BASE_DOMAIN` re-renders every declared `redirectUris` / `webOrigins` / +`rootUrl` / `baseUrl` / `adminUrl` in git — and reaches the realm **never**, because a completed +import does not re-run. Measured on prod: stored redirect URIs still on the pre-migration hostname +while git declared the new one. Every OIDC login then fails with a `redirect_uri` mismatch, the +`KeycloakRealmImport` still reads `Done=True`, and nothing alerts. + +**Pass 2 of the same CronJob** closes it. It was made a second pass of the existing reconciler rather +than a sibling CronJob for one decisive reason: **a URL write needs no new endpoint.** It is the same +minimal `PUT /admin/realms//clients/`, and the current URLs already come back in the +allow-listed client lookup — so the hard allow-list that makes `/users`, `/groups`, `/roles` and +`/authentication` unreachable is widened by *nothing*, and is preserved rather than re-declared in a +second copy. (The ConfigMap also carries exactly one script key by L0 contract, so a sibling would +have had to fork ~300 lines of safety-critical code the header explicitly forbids forking.) + +**The desired state is generated, not authored.** `client-urls.yaml` is rendered from +`keycloak-realm-import.yaml` by `ci/render-keycloak-client-urls.py`, and the L0 gate runs that +renderer with `--check` on every merge, so the committed file cannot disagree with the import. URL +strings are copied verbatim — a `${BASE_DOMAIN}` token passes through unchanged, and both files are +rendered by the **same** Flux Kustomization, so both are substituted in one pass with one value. The +renderer never parses a hostname and never reads `cluster-vars`: there is no second place the domain +can be decided. + +```sh +# regenerate after editing a client URL in the realm import (the gate will demand it) +python3 ci/render-keycloak-client-urls.py --write +``` + +Properties, each proved by `--selftest` and each shown to be *falsifiable* by +`conformance/tests/l0-static/keycloak-client-secret-reconciler/mutations.py`: + +* **Converged ⇒ zero writes.** List fields compare order-insensitively (`sorted`, not `set` — + duplicates are still drift), because Keycloak does not preserve the authored order of + `redirectUris`; an order-sensitive compare would rewrite every client forever while reporting + success. +* **Gaining and losing a URI both work.** The declared list is exact, so a URI removed from git is + removed from the realm — a stale wildcard left behind quietly widens what the realm will redirect + to. +* **Undeclared ⇒ unmanaged.** A field the desired state omits is left exactly as it is. Absence never + means "force to empty". +* **Public clients' URLs *are* managed** (their secrets are not): `kubernetes` is PKCE-public and its + localhost callbacks are what `kubectl` logins land on. +* **The two passes are independent.** Each reads its own desired state and fails closed for its own + fields only. A malformed URL ConfigMap cannot stop client-*secret* repair, and an unmounted Secret + cannot stop URL repair. +* URLs are logged in full (they are public hostnames, and the before/after is what makes a migration + auditable); secrets are still never logged. + +**What the reconciler does NOT fix:** anything outside client secrets and those five URL fields. A +NEW client, a new protocol mapper or a changed scope still needs the realm import to actually re-run +(it will not on its own) or a deliberate admin-API/`kcadm` change. `Done=True` is never evidence that +a realm edit landed. ## Exceptions that keep native auth (do not force SSO) — ADR-0014