diff --git a/decisions/2026-10-29-keycloak-passkey-reconcile.md b/decisions/2026-10-29-keycloak-passkey-reconcile.md new file mode 100644 index 0000000..eb85e26 --- /dev/null +++ b/decisions/2026-10-29-keycloak-passkey-reconcile.md @@ -0,0 +1,172 @@ +--- +date: 2026-10-29 +branch: feat/keycloak-passkey-reconcile +title: "Passkey reconciler review fixes: bind only on git's say-so (rollback sticks), protect the real 26.0.6 sub-flows, screen the requirement PUT by body id, alert on failure" +--- + +## What changed + +This branch carries the unmerged passkey reconciler (commit `36fcb7b`, cherry-picked with its +authorship, recorded in `decisions/2026-10-18-keycloak-passkey-reconciler.md`) plus the fixes for a +review of it. Read that entry for the design; this one records what the review found and what +changed. + +**1. HIGH: the bind was automatic, and rollback did not stick.** Within one tick of merge the +reconciler would build and BIND `browser-mfa-`, a login flow no human had tried. Worse, the +README's rollback (rebind `browser-mfa`) was undone every 15 minutes: the next run found the +verified candidate again and rebound it, and a hotfix inside the bound flow was "corrected" by +building and binding an `-r2`. + +- New git-declared switch: `bind-browser-flow` in `platform/keycloak/passkey-desired-state.yaml`, + a second key of the same ConfigMap, so it reaches the pod through the existing mount. **Default + `"false"`.** It is the one hand-edited line in a generated file: the renderer carries it through + on `--write`, and `--check` (run by the L0 gate) refuses anything but the quoted strings `"true"` + / `"false"`. An unquoted `true` is a YAML boolean, which a ConfigMap rejects at apply time and + which would have failed the whole `platform-config` Kustomization, realm import included. +- `"false"`: pass 3 builds and verifies the candidate (or reuses a verified one), logs + `WOULD BIND `, exits 0 with `KEYCLOAK_PASSKEY_RECONCILED_OK_PENDING_BIND`, and never PUTs + a changed `browserFlow`. That holds after an earlier bind too, so the operator's rebind sticks. + If the bound flow *is* the candidate alias and has been hand-edited, it is left alone (no `-r2` + is built beside it). +- `"true"`: binds as before, and keeps asserting it. That is GitOps working; the README says so, + and says not to hotfix the flow while it is `"true"`. +- The CronJob passes no flag. `--no-bind` still exists for a hand-run Job and can only narrow + git's value. The L0 check asserts the CronJob carries no `--` argument. +- Rollback is now: a PR setting `"false"` (first), then one rebind write. README documents the + order and why it matters, plus how to test the candidate on one client (a per-client + `account-console` browser-flow override) before the go-live PR. + +**2. HIGH: the protected-subtree guard read the wrong field for sub-flows.** Read from source at tag +26.0.6 (`services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java`, +fetched from GitHub this session): `recurseExecutions` sets `displayName = flowRef.getAlias()` and +`flowId` on a sub-flow row and never calls `setAlias`. `alias` is set only on authenticator rows, +where it holds the bound **config's** alias. So the protected set was `{bound}` alone, and a write +into `browser-mfa-forms` would not have been refused. The old stub returned `alias` on sub-flow +rows, which is why all 216 assertions passed anyway. + +- New `subflow_alias()` reads `displayName` and cites the source. I did not resolve by `flowId`: + `getFlows()` (same file) filters to `isTopLevel()`, so a nested sub-flow's id resolves to nothing + through the flows list. A sub-flow row with no `displayName` makes the listing unreadable, which + leaves the flow pass fail-closed. +- The stub now returns rows shaped like 26.0.6's: sub-flow rows carry `displayName`, `flowId` and no + `alias`/`providerId`. Authenticator rows carry a display *type* as `displayName` (not the + provider id) and the config alias as `alias`. Nulls are omitted. +- New assertions: after a real run, `browser-mfa-forms` and `browser-mfa-conditional-otp` are in + the protected set, and an execution add or requirement write into them is refused. A missing + `displayName` fails closed. + +**3. MEDIUM: `PUT /flows/{alias}/executions` was screened on the wrong field.** `updateExecutions` +resolves `realm.getAuthenticationExecutionById(rep.getId())` without checking membership in +`{alias}`. The same handler also renames the flow named by `rep.getFlowId()` when the body carries +`authenticationFlow` + `displayName`. Now: + +- `body["id"]` is screened against the protected execution ids. +- It must also be in `_BUILD_EXECUTION_IDS`: the ids read back from the flow this call just + created, open only inside `build_flow`. +- The config `POST /executions/{id}/config` is held to the same build scope. +- The body may carry only `id`, `requirement` and `priority`. +- Selftest cases cover each of these. + +**4. Stale docs on sensitive paths** now describe what ships, including the switch: +`platform-config/kustomization.yaml`, the chainsaw test header, the desired-state header and its +renderer template, the renderer's own header and `--check` message, and the README. + +**5.** +- `backoffLimit: 0`: every failure is deterministic within a tick, and a retried build burns + another of the 5 candidate aliases. +- New alert `KeycloakPasskeyReconcilerFailing` in `platform/monitoring/alerts.yaml` (warning, + `for: 15m`, `kube_job_status_failed` on `job_name=~"keycloak-passkey-reconciler-.*"`), the same + shape as the sibling's. It has promtool tests in + `platform/monitoring/tests/keycloak-passkey-reconciler.test.yaml`, in both directions plus + "the sibling's failure is not this alert" and "one transient failure stays quiet". The alert + ratchet in `alert-rules-unit-tested` would have refused it untested. A new runbook, + `runbooks/keycloak-passkey-reconciler.md`, is projected by the runbooks generator. +- The stale "Renovate does not manage plain `image:` tags" comment is corrected: `renovate.json5` + enables the `kubernetes` manager over `platform/**`. The image is pinned to + `python:3.13.15-alpine3.23@sha256:6438599575cca0d1df94aeee0d2ae088d4d8846eab554b2ee7784a3a6df0d516`, + the multi-arch index digest read from Docker Hub today. The tag is mutable: the sibling + reconciler's pod on prod reports `imageID ...@sha256:75f27d68...` under the same tag, which is + neither today's index nor today's amd64 manifest. + +**6. Hygiene.** The ~160-line header that appeared in both the YAML and the script is now kept once, +in the script, where it ships next to the code and its selftest. A short header above the +ConfigMap points to it. Nothing else was restructured, apart from the build's second half moving +into `_finish_build()` so the build scope can be opened and closed around it. + +Sensitive paths touched: `ci/**` (renderer), `conformance/**` (L0 check, mutations, chainsaw), +`platform-config/kustomization.yaml` (comment only). This needs the operator's +`/approve-override` or a non-author review. I did not post one. + +## What I verified + +- **Local toolbox gate, rc 3** (pinned toolbox via `dev.sh`, `DEV_KUBECONFIG=/dev/null`, ENGINE + shim bind-mounting the main `.git`, run on the committed branch). Stages: `spec-immutability` + pass (the spec repo resolved as a sibling), `decisions-log` pass, `lint` pass, `l0` pass + (including `keycloak-passkey-reconciler`, `alert-rules-unit-tested` and + `alert-runbooks-resolvable`), `render-golden` pass, `l1` pass, `approval-policy` pass (N/A + offline; it is guaranteed to fail in CI). **`gitea-settings` did not run** (no + `SETTINGS_API_TOKEN` locally) and is the only stage that did not run. L2/L3 `conformance` is + skipped by design (no cluster). rc 3 is **not** green. +- `platform/monitoring/tests/run.sh platform/monitoring/tests/keycloak-passkey-reconciler.test.yaml` + in the toolbox: `promtool check rules` SUCCESS (54 rules), `promtool test rules` SUCCESS, + promtool 3.14.0. + +- **Selftest: 259 assertions pass** (was 216), extracted from the shipped ConfigMap by the L0 + check. +- **`mutations.py`: 40 of 40 injected regressions caught** (was 30). The ten new ones each undo + one fix: + - sub-flows read by `alias` again; + - an unnameable sub-flow tolerated; + - the requirement PUT screened on its path only; + - execution writes not held to the build tree; + - extra body keys allowed; + - the bind switch ignored in `run()`; + - the bind switch ignored in `reconcile_flow()`; + - an absent switch reading as true; + - an invalid switch guessed at; + - `-r2` built beside a hand-edited bound flow. + "Sub-flows by `alias`" is caught less cleanly (the selftest aborts on an unreadable listing + rather than failing an assertion). It is still caught. +- The renderer accepts `"true"` and `"false"` and refuses `true` (unquoted) and `"yes"`. I + checked this by hand-editing the committed file and running `--check`. +- The 26.0.6 source for `recurseExecutions`, `updateExecutions` and `getFlows` was read directly + (lines ~225-231, 658-724 and 747-806 of the file at that tag). +- Read-only `kubectl --context k3s-prod`: Keycloak pods run `quay.io/keycloak/keycloak:26.0.6` + (2 replicas). The only CronJob in `keycloak` is `keycloak-client-secret-reconciler`, so the + passkey reconciler is **not** live. + +## What I did NOT verify + +- **Nothing has run against a real Keycloak.** The stub's row shapes now follow the 26.0.6 + source, but they are still my reading of that source, not a captured response. In particular, + I have never seen a live executions listing to confirm `displayName` on a sub-flow row. If it + differs, the failure is fail-closed (no build), not fail-open. +- **The candidate flow has never been tested in a browser.** That is now a precondition of the + go-live PR (README, "Testing the candidate flow before binding it"). Nobody has run those steps. + I also have not checked that the per-client `account-console` override behaves as described on + 26.0.6. +- **"Merging changes nothing users see" is true of login, but passes 1 and 2 still write the + realm.** They set the passwordless policy and enable the enrolment action. That action is + enabled by default in 26.0.6, but I did not read the live realm, because no admin API calls were + allowed. Pass 3 also creates the unbound candidate flow (realm DB writes, invisible to login). +- **Race in pass 1, not fixed:** the policy PUT is a read-modify-write that carries `browserFlow` + back from its own GET. An operator rebind landing between that GET and PUT would be reverted. + The window is one HTTP round trip and only exists when the policy has drifted. +- **The alert has never seen a real series.** The CronJob has never run, so + `kube_job_status_failed{job_name=~"keycloak-passkey-reconciler-.*"}` does not exist yet. Its + shape is copied from the sibling's rule, which does have live series. +- **The digest is of today's index.** I did not pull it or run the image. Renovate's handling of a + `tag@digest` pin under the `kubernetes` manager is from its documentation, not from a Renovate + run here. +- **CI's own result** is in the PR, not here. `approval-policy` is expected to fail until the + operator overrides. + +### Follow-ups (out of scope here, deliberately not done) + +- A dedicated least-privilege service-account client for the reconciler, instead of the + master-realm break-glass admin it shares with the client-secret reconciler. +- A NetworkPolicy for the `keycloak` namespace. +- Whether a passkey login should skip TOTP for platform-admins. This is the PR #72 design + decision, currently "no TOTP after a passkey". +- A `KeycloakPasskeyReconcilerStale` rule (no successful run in N hours), like the sibling's. + Only the failure alert was in scope. diff --git a/platform/keycloak/passkey-reconciler.yaml b/platform/keycloak/passkey-reconciler.yaml index dfa64a5..6b346f1 100644 --- a/platform/keycloak/passkey-reconciler.yaml +++ b/platform/keycloak/passkey-reconciler.yaml @@ -1332,7 +1332,7 @@ data: A single read-modify-write PUT of the realm, changing exactly one field. This is the only function in the program permitted to change browserFlow; the policy pass refuses to. It is called ONLY after the new flow has been built and a readback has - proved it matches the desired shape. + proved it matches the desired shape, AND only when git's bind-browser-flow is "true". The previous alias is returned so it can be logged. The old flow is never deleted, so that alias is a working rollback target and the rollback is one PUT.