docs(decisions): handoff for the passkey reconciler review fixes

Also states in bind_browser_flow()'s docstring that it is reached only when git's bind-browser-flow is "true". Co-Authored-By: Claude Opus 5.5 <[email protected]>
2026-10-29 21:45:40 -05:00
1 parent 7c4eb439b2
commit 6bb8af1d70
@@ -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-<hash>`, 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 <alias>`, 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.
@@ -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.