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]>
11 KiB
date, branch, title
| date | branch | title |
|---|---|---|
| 2026-10-29 | feat/keycloak-passkey-reconcile | 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-flowinplatform/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 unquotedtrueis a YAML boolean, which a ConfigMap rejects at apply time and which would have failed the wholeplatform-configKustomization, realm import included. "false": pass 3 builds and verifies the candidate (or reuses a verified one), logsWOULD BIND <alias>, exits 0 withKEYCLOAK_PASSKEY_RECONCILED_OK_PENDING_BIND, and never PUTs a changedbrowserFlow. 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-r2is 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-bindstill 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-clientaccount-consolebrowser-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()readsdisplayNameand cites the source. I did not resolve byflowId:getFlows()(same file) filters toisTopLevel(), so a nested sub-flow's id resolves to nothing through the flows list. A sub-flow row with nodisplayNamemakes 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,flowIdand noalias/providerId. Authenticator rows carry a display type asdisplayName(not the provider id) and the config alias asalias. Nulls are omitted. - New assertions: after a real run,
browser-mfa-formsandbrowser-mfa-conditional-otpare in the protected set, and an execution add or requirement write into them is refused. A missingdisplayNamefails 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 insidebuild_flow. - The config
POST /executions/{id}/configis held to the same build scope. - The body may carry only
id,requirementandpriority. - 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
KeycloakPasskeyReconcilerFailinginplatform/monitoring/alerts.yaml(warning,for: 15m,kube_job_status_failedonjob_name=~"keycloak-passkey-reconciler-.*"), the same shape as the sibling's. It has promtool tests inplatform/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 inalert-rules-unit-testedwould 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.json5enables thekubernetesmanager overplatform/**. The image is pinned topython: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 reportsimageID ...@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-immutabilitypass (the spec repo resolved as a sibling),decisions-logpass,lintpass,l0pass (includingkeycloak-passkey-reconciler,alert-rules-unit-testedandalert-runbooks-resolvable),render-goldenpass,l1pass,approval-policypass (N/A offline; it is guaranteed to fail in CI).gitea-settingsdid not run (noSETTINGS_API_TOKENlocally) and is the only stage that did not run. L2/L3conformanceis skipped by design (no cluster). rc 3 is not green. -
platform/monitoring/tests/run.sh platform/monitoring/tests/keycloak-passkey-reconciler.test.yamlin the toolbox:promtool check rulesSUCCESS (54 rules),promtool test rulesSUCCESS, 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
aliasagain; - 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;
-r2built beside a hand-edited bound flow. "Sub-flows byalias" is caught less cleanly (the selftest aborts on an unreadable listing rather than failing an assertion). It is still caught.
- sub-flows read by
-
The renderer accepts
"true"and"false"and refusestrue(unquoted) and"yes". I checked this by hand-editing the committed file and running--check. -
The 26.0.6 source for
recurseExecutions,updateExecutionsandgetFlowswas read directly (lines ~225-231, 658-724 and 747-806 of the file at that tag). -
Read-only
kubectl --context k3s-prod: Keycloak pods runquay.io/keycloak/keycloak:26.0.6(2 replicas). The only CronJob inkeycloakiskeycloak-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
displayNameon 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-consoleoverride 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
browserFlowback 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@digestpin under thekubernetesmanager is from its documentation, not from a Renovate run here. - CI's own result is in the PR, not here.
approval-policyis 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
keycloaknamespace. - Whether a passkey login should skip TOTP for platform-admins. This is the PR #72 design decision, currently "no TOTP after a passkey".
- A
KeycloakPasskeyReconcilerStalerule (no successful run in N hours), like the sibling's. Only the failure alert was in scope.