fix(keycloak-passkey): bind only on git's say-so; protect real sub-flows; screen the requirement PUT by body id
Review findings on the passkey reconciler, fixed:
1. HIGH - the bind was automatic and rollback did not stick. A git-declared switch,
`bind-browser-flow` in passkey-desired-state.yaml, now decides whether pass 3 may
PUT browserFlow. Default "false": build + verify the candidate (or reuse a verified
one), log WOULD BIND <alias>, never touch the binding - including after an earlier
bind, so an operator's rollback sticks, and a hand-edited bound candidate is left
alone rather than shadowed by an -r2 build. "true" binds as before. The renderer
carries the one hand-edited line through and --check refuses anything but the
quoted strings "true"/"false". --no-bind can only narrow it.
2. HIGH - the protected-subtree guard read `alias` on sub-flow rows. Keycloak 26.0.6
AuthenticationManagementResource.recurseExecutions sets displayName =
flowRef.getAlias() and flowId on a sub-flow row and never sets alias (alias is the
CONFIG alias on authenticator rows), so the protected set was only the top-level
alias. Sub-flows are now named by displayName (getFlows() lists top-level flows
only, so flowId cannot be resolved through it); a sub-flow row without one leaves
the subtree undetermined and the flow pass fail-closed. The selftest stub now
returns 26.0.6-shaped rows, and a real run is asserted to refuse writes into
browser-mfa-forms and browser-mfa-conditional-otp.
3. MEDIUM - PUT /flows/{alias}/executions was screened on the path alias only, but
updateExecutions resolves the execution realm-wide by body id. body.id is now
screened against the protected ids AND required to be in the tree this run is
building (recorded from the build's own readback); the config POST is held to the
same build scope; and the body may carry only id/requirement/priority (the same
handler renames any flow named by flowId).
4. Stale "audit-only" docs rewritten (platform-config kustomization, chainsaw test,
desired-state header and its renderer template, README) to describe what ships.
5. backoffLimit 0 (a retried deterministic build failure burns the candidate-alias
budget); the stale "Renovate does not manage image: tags" comment corrected and the
image pinned by index digest.
6. The ~160-line header duplicated in the YAML and the script is now kept once, in the
script, with a short pointer above it.
Selftest: 259 assertions (was 216). mutations.py: 40 injected regressions (was 30),
including one per finding above, all caught.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
This commit is contained in:
jakman85andClaude Opus 5.5 committed
1 parent
f5931944fa
commit
17eed6e8ae
8 files changed
+893
-310
No files matched your search
@@ -1,6 +1,7 @@
|
||||
# =============================================================================
|
||||
# L0 STATIC — the passkey configuration is derived from one declaration, continuously
|
||||
# reconciled, and the browser login flow is auditable but UNWRITABLE
|
||||
# reconciled, and the browser login flow converges BLUE/GREEN: the BOUND flow is
|
||||
# unwritable, and binding a new one is git's explicit decision (bind-browser-flow)
|
||||
# Catalog: docs/14 L0 row ("Pre-apply, in CI") — runs WITHOUT a cluster.
|
||||
#
|
||||
# R-IDP-03 (identity config is declarative) and R-IDP-04 (MFA available) are both
|
||||
@@ -25,25 +26,30 @@
|
||||
# import declares NO requiredActions block (26.0.6 treats it as REPLACE, which would
|
||||
# delete CONFIGURE_TOTP and lock out every platform-admin); the enrolment action is
|
||||
# enabled but NOT a defaultAction, so enrolment stays opt-in; the reconciler carries
|
||||
# no ServiceAccount token, mounts only what it needs, and contains no dollar sign;
|
||||
# the endpoint allow-list contains NO write for any authentication flow, execution
|
||||
# or authenticator config, and NO required-action write for any alias other than the
|
||||
# one named literally; the SIBLING client-secret reconciler's allow-list is NOT
|
||||
# widened; and the reconciler's own `--selftest` (extracted from the shipped
|
||||
# ConfigMap and executed) proves compare-then-write, idempotence, strict
|
||||
# read-modify-write, the RpEntityName gate, flow-is-unwritable, the conditional-OTP
|
||||
# regression and fail-closed — with mutations.py proving that selftest can FAIL.
|
||||
# no ServiceAccount token, mounts only what it needs, passes no mode flag, and
|
||||
# contains no dollar sign; the endpoint allow-list contains NO delete, NO
|
||||
# single-execution or config-edit write, and NO required-action write for any alias
|
||||
# other than the one named literally; the protected-subtree guard exists, is wired
|
||||
# into the single request choke point and is fail-closed; the SIBLING client-secret
|
||||
# reconciler's allow-list is NOT widened; and the reconciler's own `--selftest`
|
||||
# (extracted from the shipped ConfigMap and executed) proves compare-then-write,
|
||||
# idempotence, strict read-modify-write, the RpEntityName gate, the bound flow and
|
||||
# its sub-flows unwritable, the git bind switch and a rollback that sticks, the
|
||||
# conditional-OTP regression and fail-closed — with mutations.py proving that
|
||||
# selftest can FAIL.
|
||||
#
|
||||
# WHY THE FLOW IS AUDITED AND NOT CONVERGED, asserted here rather than only argued:
|
||||
# converging a five-flow, ten-execution restructure needs create-subflow,
|
||||
# create-execution, set-requirement, set-priority and bind-config as SEPARATE admin API
|
||||
# calls with no transaction. A run that dies between them leaves a HALF-BUILT FLOW BOUND
|
||||
# AS THE LOGIN PATH for Grafana, Gitea, Nextcloud, Headlamp, Semaphore, PiHole, Crafty,
|
||||
# the *arr stack AND kubectl. This repo has already had a total SSO outage from a realm
|
||||
# change (2026-09-06), and the conditional-OTP sub-flow — which the declared structure
|
||||
# moves one level deeper than it sits today — once 403'd every platform-admin. A
|
||||
# reconciler that half-rewrites the bound flow every 15 minutes is a worse failure than
|
||||
# the one it replaces, because it is a loop rather than a single bad change.
|
||||
# WHY THE FLOW IS BUILT BLUE/GREEN AND BOUND ONLY ON GIT'S SAY-SO, asserted here rather
|
||||
# than only argued: converging a five-flow, ten-execution restructure needs
|
||||
# create-subflow, create-execution, set-requirement and bind-config as SEPARATE admin API
|
||||
# calls with no transaction. Doing that to the BOUND flow means a run that dies between
|
||||
# them leaves a HALF-BUILT FLOW AS THE LOGIN PATH for Grafana, Gitea, Nextcloud, Headlamp,
|
||||
# Semaphore, PiHole, Crafty, the *arr stack AND kubectl — this repo has had a total SSO
|
||||
# outage from a realm change (2026-09-06), and the conditional-OTP sub-flow once 403'd
|
||||
# every platform-admin. So every multi-call write lands on a NEW, unused candidate flow,
|
||||
# which is verified by readback before anything is bound. Binding it is then ONE atomic
|
||||
# PUT of browserFlow, made only when passkey-desired-state.yaml sets
|
||||
# bind-browser-flow: "true" (default "false"): an untested login flow never goes live on
|
||||
# a schedule tick, and with the switch off an operator's rollback is never undone.
|
||||
#
|
||||
# STATUS: EXECUTABLE. `check.sh` is a pure offline assertion script (git + PyYAML +
|
||||
# stdlib); conformance/run.sh --layer l0 and ci/l0.sh run it directly, and this Chainsaw
|
||||
@@ -93,9 +99,10 @@ spec:
|
||||
- name: audit-passkey-declaration-renderer-and-reconciler
|
||||
description: >-
|
||||
Assert the desired state is derived from the realm import and shares its
|
||||
substitution pass, that the reconciler's write surface excludes every flow
|
||||
mutation, that the sibling reconciler is unwidened, and run the reconciler's own
|
||||
selftest plus the mutation suite that proves it can fail.
|
||||
substitution pass, that the reconciler cannot write the bound flow or delete
|
||||
anything and binds only on git's bind-browser-flow, that the sibling reconciler
|
||||
is unwidened, and run the reconciler's own selftest plus the mutation suite that
|
||||
proves it can fail.
|
||||
try:
|
||||
- script:
|
||||
content: bash ./check.sh
|
||||
@@ -62,6 +62,11 @@
|
||||
# been read. That last one is what the whole design rests on: a reconciler that
|
||||
# half-rewrites the BOUND browser flow is a total SSO outage on a loop, and
|
||||
# this repo has had one (2026-09-06).
|
||||
# (6b) THE BIND IS GIT'S DECISION: the desired-state ConfigMap carries
|
||||
# bind-browser-flow as exactly "true" or "false", the script reads it from the
|
||||
# SAME mount, and the CronJob passes no flag that could widen it. With "false"
|
||||
# (the default) the reconciler builds and verifies but never PUTs browserFlow,
|
||||
# so an operator's rollback sticks; the selftest proves that end to end.
|
||||
# (7) THE SIBLING RECONCILER IS UNTOUCHED: client-secret-reconciler.yaml's
|
||||
# allow-list is NOT widened by this work. Its --selftest asserts it refuses
|
||||
# /users, /groups, /roles, /authentication and the whole-realm endpoint, and
|
||||
@@ -152,6 +157,7 @@ RENDERER = "ci/render-keycloak-passkey-policy.py"
|
||||
SIBLING_FILE = "platform/keycloak/client-secret-reconciler.yaml"
|
||||
STATE_CM_NAME = "keycloak-passkey-desired-state"
|
||||
STATE_DATA_KEY = "passkey-desired-state.json"
|
||||
BIND_KEY = "bind-browser-flow"
|
||||
SCRIPT_KEY = "reconcile-keycloak-passkeys.py"
|
||||
ENROL_ACTION = "webauthn-register-passwordless"
|
||||
DOLLAR = chr(36)
|
||||
@@ -225,10 +231,16 @@ if state_cm is not None:
|
||||
f"{STATE_FILE}: the ConfigMap is named "
|
||||
f"{state_cm['metadata']['name']!r}, but the CronJob mounts {STATE_CM_NAME!r}")
|
||||
data = state_cm.get("data") or {}
|
||||
check(list(data) == [STATE_DATA_KEY],
|
||||
f"{STATE_FILE}: expected exactly the data key {STATE_DATA_KEY!r}, got "
|
||||
f"{sorted(data)} — the reconciler's PASSKEY_STATE_FILE default would not "
|
||||
f"resolve")
|
||||
check(sorted(data) == sorted([STATE_DATA_KEY, BIND_KEY]),
|
||||
f"{STATE_FILE}: expected exactly the data keys {STATE_DATA_KEY!r} and "
|
||||
f"{BIND_KEY!r}, got {sorted(data)} — the reconciler's PASSKEY_STATE_FILE / "
|
||||
f"PASSKEY_BIND_FILE defaults would not resolve")
|
||||
# THE BIND SWITCH. An unquoted true/false is a YAML BOOLEAN, which a ConfigMap rejects
|
||||
# at apply time -- failing the whole platform-config Kustomization, realm import
|
||||
# included. The renderer refuses it too; asserted here so the reason is on record.
|
||||
check(data.get(BIND_KEY) in ("true", "false"),
|
||||
f"{STATE_FILE}: data.{BIND_KEY} is {data.get(BIND_KEY)!r}; it must be the QUOTED "
|
||||
f"string \"true\" or \"false\"")
|
||||
raw = data.get(STATE_DATA_KEY, "")
|
||||
try:
|
||||
desired = json.loads(raw)
|
||||
@@ -457,6 +469,25 @@ if cj is not None:
|
||||
f"the mount path — they are a pair and must move together")
|
||||
check((mounts.get(state_vol) or {}).get("readOnly") is True,
|
||||
f"{RECON_FILE}: the desired-state mount must be readOnly")
|
||||
check(f'"/etc/keycloak-passkey/{BIND_KEY}"' in script,
|
||||
f"{RECON_FILE}: the script's PASSKEY_BIND_FILE default is not "
|
||||
f"/etc/keycloak-passkey/{BIND_KEY}. An unresolved bind file reads as \"false\" "
|
||||
f"(safe, but git's \"true\" would then silently never take effect)")
|
||||
check(not (state_vol and (cm_vols[state_vol]["configMap"] or {}).get("items")),
|
||||
f"{RECON_FILE}: the desired-state volume selects `items`, so one of its two "
|
||||
f"keys may not be projected")
|
||||
|
||||
# The bind decision belongs to git. A container arg could only NARROW it today
|
||||
# (--no-bind), but nothing in the CronJob may carry a mode flag at all: the shipped
|
||||
# behaviour must be readable from passkey-desired-state.yaml alone.
|
||||
cmd = [str(x) for x in (ctr.get("command") or []) + (ctr.get("args") or [])]
|
||||
check(not [x for x in cmd if x.startswith("--")],
|
||||
f"{RECON_FILE}: the CronJob passes {[x for x in cmd if x.startswith('--')]} to "
|
||||
f"the reconciler. Its mode is decided by git (bind-browser-flow), not by flags")
|
||||
# Retries burn the bounded candidate-alias budget on a deterministic failure.
|
||||
check(cj["spec"]["jobTemplate"]["spec"].get("backoffLimit") == 0,
|
||||
f"{RECON_FILE}: backoffLimit must be 0 — every failure here is deterministic "
|
||||
f"within a tick, and each retried build consumes another candidate alias")
|
||||
|
||||
# The admin credential, referenced BY NAME. Never decrypted, never a literal.
|
||||
for key in ("KEYCLOAK_ADMIN_USER", "KEYCLOAK_ADMIN_PASSWORD"):
|
||||
@@ -591,6 +622,21 @@ check("_FLOW_CREATE_PATH" in script,
|
||||
f"altogether")
|
||||
# And the one function allowed to change the live binding must be distinct from the
|
||||
# policy write, which must refuse to.
|
||||
# FINDING 2026-10-29: sub-flows are named by displayName in 26.0.6 (recurseExecutions
|
||||
# never sets `alias` on a sub-flow row). Reading `alias` shrank the protected subtree to
|
||||
# the top-level flow alone while every selftest passed against a stub that returned it.
|
||||
check(re.search(r'^def subflow_alias\(ex\):\n(?:.*\n)*? name = ex\.get\("displayName"\)',
|
||||
script, re.M) is not None,
|
||||
f"{RECON_FILE}: subflow_alias() no longer reads displayName. Keycloak 26.0.6's "
|
||||
f"executions listing names a sub-flow ONLY by displayName; reading `alias` yields "
|
||||
f"None and the protected subtree silently shrinks to the top-level flow")
|
||||
# FINDING 2026-10-29: the requirement PUT's real target is body["id"].
|
||||
check("_EXEC_UPDATE_PATH" in script and "_BUILD_EXECUTION_IDS" in script,
|
||||
f"{RECON_FILE}: the requirement PUT (PUT /flows/<alias>/executions) is no longer "
|
||||
f"screened on its body id. updateExecutions resolves the execution realm-wide by id "
|
||||
f"and ignores the path alias, so a path-only screen lets it write the bound flow")
|
||||
check("def declared_bind(" in script and "BIND_FILE" in script,
|
||||
f"{RECON_FILE}: the reconciler no longer reads git's bind-browser-flow switch")
|
||||
check("def bind_browser_flow(" in script,
|
||||
f"{RECON_FILE}: there is no bind_browser_flow() function. The atomic flip must be "
|
||||
f"one named, auditable operation, not an incidental field in some other write")
|
||||
|
||||
@@ -78,10 +78,10 @@ MUTATIONS: list[tuple[str, str, str, str]] = [
|
||||
"module-level initial value is deliberately NOT the anchor here: run() resets "
|
||||
"these at the start of every run, so the initialiser is unreachable and mutating "
|
||||
"it would prove nothing.)",
|
||||
" global _PROTECTED_FLOW_ALIASES, _PROTECTED_EXECUTION_IDS\n"
|
||||
" global _PROTECTED_FLOW_ALIASES, _PROTECTED_EXECUTION_IDS, _BUILD_EXECUTION_IDS\n"
|
||||
" _PROTECTED_FLOW_ALIASES = None\n"
|
||||
" _PROTECTED_EXECUTION_IDS = None",
|
||||
" global _PROTECTED_FLOW_ALIASES, _PROTECTED_EXECUTION_IDS\n"
|
||||
" global _PROTECTED_FLOW_ALIASES, _PROTECTED_EXECUTION_IDS, _BUILD_EXECUTION_IDS\n"
|
||||
" _PROTECTED_FLOW_ALIASES = frozenset()\n"
|
||||
" _PROTECTED_EXECUTION_IDS = frozenset()"),
|
||||
|
||||
@@ -117,6 +117,84 @@ MUTATIONS: list[tuple[str, str, str, str]] = [
|
||||
" \"the existing flow %r\" % candidate):",
|
||||
" if cand_shape is not None:"),
|
||||
|
||||
# ------------------------------------- 2026-10-29 review findings, each made falsifiable
|
||||
("sub-flows identified by `alias` again",
|
||||
"read `alias` off a sub-flow row, as the first version did -- Keycloak 26.0.6's "
|
||||
"recurseExecutions sets only displayName (and flowId) on a sub-flow row, so every "
|
||||
"sub-flow resolves to nothing and the protected subtree is the top-level flow alone: "
|
||||
"a write into browser-mfa-forms would NOT be refused",
|
||||
" name = ex.get(\"displayName\")\n"
|
||||
" return name if isinstance(name, str) and name else None",
|
||||
" name = ex.get(\"alias\")\n"
|
||||
" return name if isinstance(name, str) and name else None"),
|
||||
|
||||
("an unnameable sub-flow is tolerated",
|
||||
"carry on when a sub-flow row has no displayName -- the protected subtree is then "
|
||||
"SMALLER than the live tree, which is the fail-open the guard exists to prevent",
|
||||
" if name is None:\n"
|
||||
" return None, None",
|
||||
" if name is None:\n"
|
||||
" name = \"\""),
|
||||
|
||||
("the requirement PUT is screened on its path only",
|
||||
"trust the alias in PUT /flows/<alias>/executions -- updateExecutions resolves "
|
||||
"body.id realm-wide and never checks it belongs to <alias>, so a candidate alias in "
|
||||
"the path can carry a BOUND execution's id in the body",
|
||||
" targets.append((\"execution\", (body or {}).get(\"id\")))",
|
||||
" pass"),
|
||||
|
||||
("execution-addressed writes are not held to the build tree",
|
||||
"'outside the bound subtree is enough' -- it is not: every OTHER flow in the realm is "
|
||||
"outside it too, so a stray id would edit an unrelated flow",
|
||||
" if what == \"execution\" and (_BUILD_EXECUTION_IDS is None\n"
|
||||
" or target not in _BUILD_EXECUTION_IDS):",
|
||||
" if False:"),
|
||||
|
||||
("the requirement PUT may carry any key",
|
||||
"let the body carry flowId / displayName -- the same handler then RENAMES whatever "
|
||||
"flow flowId names, bound or not",
|
||||
" if extra:\n"
|
||||
" raise Refused(",
|
||||
" if False:\n"
|
||||
" raise Refused("),
|
||||
|
||||
("git's bind switch is ignored",
|
||||
"bind whenever verification passes, as the first version did -- the first live use of "
|
||||
"an untested login flow becomes a schedule tick, and an operator's rollback is "
|
||||
"undone within 15 minutes, forever",
|
||||
" bind = git_bind and not no_bind",
|
||||
" bind = not no_bind"),
|
||||
|
||||
("bind=false still binds inside the flow pass",
|
||||
"keep the flag but forget to honour it where the bind happens",
|
||||
" if not bind:\n"
|
||||
" # THE BIND SWITCH IS OFF.",
|
||||
" if False:\n"
|
||||
" # THE BIND SWITCH IS OFF."),
|
||||
|
||||
("an absent bind switch means bind",
|
||||
"default to true when the key is missing -- a ConfigMap predating the switch, or a "
|
||||
"deleted line, would silently put an untested flow live",
|
||||
" except FileNotFoundError:\n"
|
||||
" return False",
|
||||
" except FileNotFoundError:\n"
|
||||
" return True"),
|
||||
|
||||
("an invalid bind switch is guessed at",
|
||||
"read anything that is not \"true\" as false -- a typo then passes silently instead of "
|
||||
"stopping the run, and the operator believes the flow is live",
|
||||
" if raw == \"false\":\n"
|
||||
" return False\n"
|
||||
" raise ValueError(",
|
||||
" return False\n"
|
||||
" raise ValueError("),
|
||||
|
||||
("bind=false rebuilds beside a hand-edited bound flow",
|
||||
"treat the hand-edited bound candidate as a 'half-built leftover' -- with the switch "
|
||||
"off this builds an -r2 next to the operator's hotfix on every tick",
|
||||
" if candidate in have and candidate == bound and not bind:",
|
||||
" if False:"),
|
||||
|
||||
# ------------------------------------------------------------------- idempotence
|
||||
("sub-flow aliases are compared as names",
|
||||
"compare the sub-flow alias like any other field -- the flows this program builds "
|
||||
|
||||
Reference in new issue
Block a user