From 51d8676f900e251493cf739fd944cb895e9491fd Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Sat, 12 Sep 2026 20:57:39 -0500 Subject: [PATCH 1/4] feat(keycloak): render the client-URL desired state from the realm import The realm import is a one-shot: once Done=True it never re-runs, so a redirect URI changed in git never reaches the realm. Healing that needs a DESIRED STATE the reconciler can read -- and the reconciler pod has no Kubernetes API access at all (automountServiceAccountToken: false), so it cannot read the KeycloakRealmImport CR. Its desired state must be mounted, hence a ConfigMap. A hand-authored ConfigMap would be a second source of truth for every public hostname in the realm, free to disagree with the import -- the same shape as the bug being closed. So it is GENERATED from the import and the render is gated, the same render==golden idiom catalog/render.py already uses for apps/. URL strings are copied VERBATIM: a ${BASE_DOMAIN} token in the source appears unchanged here, and both files are rendered by the SAME Flux Kustomization (./platform-config), so both are substituted in one pass with one value. The renderer never parses a hostname and never reads cluster-vars, so there is no second place the domain can be decided. Co-Authored-By: Claude Opus 5 --- ci/render-keycloak-client-urls.py | 214 +++++++++++++++++++++++++++++ platform-config/kustomization.yaml | 11 ++ platform/keycloak/client-urls.yaml | 157 +++++++++++++++++++++ 3 files changed, 382 insertions(+) create mode 100755 ci/render-keycloak-client-urls.py create mode 100644 platform/keycloak/client-urls.yaml diff --git a/ci/render-keycloak-client-urls.py b/ci/render-keycloak-client-urls.py new file mode 100755 index 0000000..266ca82 --- /dev/null +++ b/ci/render-keycloak-client-urls.py @@ -0,0 +1,214 @@ +#!/usr/bin/env python3 +# ============================================================================= +# ci/render-keycloak-client-urls.py +# +# Render platform/keycloak/client-urls.yaml FROM platform/keycloak/keycloak-realm-import.yaml. +# +# WHY THIS EXISTS +# --------------- +# The Keycloak realm import is a ONE-SHOT that fails OPEN: once it rests at +# Done=True / Started=False it never re-runs, so a redirect URI changed in git is +# never applied to the realm. The client-secret half of that failure class is +# already healed by a level-triggered CronJob (platform/keycloak/client-secret- +# reconciler.yaml). This renderer supplies the DESIRED STATE that the same CronJob +# needs to heal the URL half. +# +# THE CONSTRAINT THAT SHAPES IT: the reconciler pod runs with +# automountServiceAccountToken: false. It has NO Kubernetes API access at all, by +# design (R-SEC-02) — so it cannot read the KeycloakRealmImport CR to learn what +# the realm is supposed to look like. Its inputs must be MOUNTED. Hence a +# ConfigMap. +# +# A hand-authored ConfigMap would be a SECOND SOURCE OF TRUTH for every public +# hostname in the realm, free to disagree with the realm import — and "two +# declarations that can disagree" is the same shape as the bug this whole effort +# is closing. So the ConfigMap is GENERATED from the realm import and the +# generation is GATED: conformance/tests/l0-static/keycloak-client-secret- +# reconciler/check.sh runs this script with --check on every merge, and the merge +# fails if the committed file is not what this renderer produces. That is the same +# render==golden idiom catalog/render.py + ci/render-golden.sh already use for +# apps/. +# +# STRINGS ARE COPIED VERBATIM, NEVER RE-DERIVED. If a URL in the realm import is +# written as a Flux substitution token (https://grafana.${BASE_DOMAIN}/...), the +# token is carried through into the ConfigMap CHARACTER FOR CHARACTER. Both files +# are rendered by the SAME Flux Kustomization (./platform-config, whose +# postBuild.substituteFrom reads cluster-vars), so both are substituted in the +# same pass with the same values and cannot disagree about what BASE_DOMAIN is. +# This renderer never parses, splits, or reconstructs a hostname, and never reads +# cluster-vars — so there is no second place for the domain to be decided. +# +# USAGE +# python3 ci/render-keycloak-client-urls.py # print the rendered file +# python3 ci/render-keycloak-client-urls.py --write # write it (regenerate) +# python3 ci/render-keycloak-client-urls.py --check # exit 1 if it has drifted +# ============================================================================= +from __future__ import annotations + +import argparse +import difflib +import json +import sys +from pathlib import Path + +try: + import yaml +except ImportError: # pragma: no cover + print("FATAL: PyYAML is required (same dependency as catalog/render.py)", + file=sys.stderr) + raise SystemExit(1) + +ROOT = Path(__file__).resolve().parent.parent +IMPORT_FILE = "platform/keycloak/keycloak-realm-import.yaml" +OUT_FILE = "platform/keycloak/client-urls.yaml" +DATA_KEY = "client-urls.json" +CONFIGMAP_NAME = "keycloak-client-urls" +NAMESPACE = "keycloak" + +# The client fields this renderer exports, and therefore the ONLY fields the +# reconciler may write. Every one of them is an environment-derived URL: it changes +# when BASE_DOMAIN changes, and a stale value breaks OIDC login (a wrong +# redirectUri) or silently widens trust (a stale wildcard left behind). +# +# DELIBERATELY ABSENT: `secret` (healed by the secret pass from the SOPS Secret, +# and it must never appear in a ConfigMap), and every non-URL field — +# protocolMappers, defaultClientScopes, attributes, flags. Those are not +# environment-derived, so nothing re-renders them on a domain flip; keeping them +# out keeps the reconciler's write surface as narrow as the problem. +LIST_FIELDS = ("redirectUris", "webOrigins") +STR_FIELDS = ("rootUrl", "baseUrl", "adminUrl") + + +def render(root: Path) -> str: + """Return the full text of client-urls.yaml as it should be committed.""" + docs = list(yaml.safe_load_all((root / IMPORT_FILE).read_text())) + imp = next((d for d in docs if d and d.get("kind") == "KeycloakRealmImport"), None) + if imp is None: + raise SystemExit("FATAL: no KeycloakRealmImport document in %s" % IMPORT_FILE) + clients = (imp.get("spec") or {}).get("realm", {}).get("clients") or [] + if not clients: + # Fail closed. Emitting an empty desired state would make the reconciler + # believe every client is unmanaged and silently stop reconciling URLs. + raise SystemExit("FATAL: %s declares zero clients — refusing to render an " + "empty desired state" % IMPORT_FILE) + + want: dict[str, dict] = {} + for c in clients: + cid = c.get("clientId") + if not isinstance(cid, str) or not cid: + raise SystemExit("FATAL: %s has a client with no clientId" % IMPORT_FILE) + entry: dict[str, object] = {} + for f in LIST_FIELDS: + if f not in c: + continue # not declared => NOT MANAGED (never forced empty) + v = c[f] + if not isinstance(v, list) or not all(isinstance(x, str) for x in v): + raise SystemExit("FATAL: %s client %r field %s is not a list of strings" + % (IMPORT_FILE, cid, f)) + entry[f] = list(v) # verbatim, order as authored + for f in STR_FIELDS: + if f not in c: + continue + v = c[f] + if not isinstance(v, str): + raise SystemExit("FATAL: %s client %r field %s is not a string" + % (IMPORT_FILE, cid, f)) + entry[f] = v + if entry: + want[cid] = entry + if not want: + raise SystemExit("FATAL: no client in %s declares any of %s — refusing to render " + "an empty desired state" + % (IMPORT_FILE, ", ".join(LIST_FIELDS + STR_FIELDS))) + + body = json.dumps(want, indent=2, sort_keys=True, ensure_ascii=False) + indented = "\n".join((" " + ln) if ln else "" for ln in body.splitlines()) + return HEADER + indented + "\n" + + +HEADER = """--- +# ============================================================================= +# GENERATED FILE — DO NOT EDIT BY HAND. +# +# source: platform/keycloak/keycloak-realm-import.yaml (spec.realm.clients) +# regenerate: python3 ci/render-keycloak-client-urls.py --write +# gated by: conformance/tests/l0-static/keycloak-client-secret-reconciler +# (runs this renderer with --check on every merge) +# +# WHAT THIS IS: the DESIRED STATE for every environment-derived client URL in the +# `homelab` realm — redirectUris, webOrigins, rootUrl, baseUrl, adminUrl. The +# client-secret reconciler CronJob mounts it and makes the live realm match it. +# +# WHY IT IS A SEPARATE OBJECT: the realm import is a ONE-SHOT that never re-runs +# once Done=True, so a redirect URI edited in git never reaches the realm. Prod's +# stored redirect URIs were measured still pointing at the pre-migration hostname +# while git declared the new one, and every OIDC login would have failed with a +# redirect_uri mismatch. The reconciler is the level-triggered loop that closes +# that; this file is what it reconciles TOWARDS. +# +# WHY IT IS GENERATED AND NOT AUTHORED: a hand-written copy of every public +# hostname would be a second source of truth, free to disagree with the realm +# import. It is rendered from the import and the render is gated, so the two +# cannot drift. URL strings are copied VERBATIM — if the source uses a +# ${BASE_DOMAIN} token it appears here unchanged, and both files are substituted +# by the SAME Flux Kustomization (./platform-config), so the domain is decided in +# exactly one place. +# +# A field that is NOT declared on a client in the realm import is simply ABSENT +# here, and the reconciler leaves it alone. Absence means UNMANAGED, never +# "force to empty". +# ============================================================================= +apiVersion: v1 +kind: ConfigMap +metadata: + name: %s + namespace: %s + labels: + app.kubernetes.io/name: %s + app.kubernetes.io/part-of: keycloak +data: + %s: | +""" % (CONFIGMAP_NAME, NAMESPACE, CONFIGMAP_NAME, DATA_KEY) + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__) + g = ap.add_mutually_exclusive_group() + g.add_argument("--write", action="store_true", help="write %s" % OUT_FILE) + g.add_argument("--check", action="store_true", + help="exit 1 if the committed file has drifted from the source") + args = ap.parse_args() + + want = render(ROOT) + target = ROOT / OUT_FILE + + if args.write: + target.write_text(want, encoding="utf-8") + print("wrote %s" % OUT_FILE) + return 0 + + if args.check: + if not target.is_file(): + print("CHECK FAIL: %s does not exist. Run: python3 " + "ci/render-keycloak-client-urls.py --write" % OUT_FILE, file=sys.stderr) + return 1 + have = target.read_text(encoding="utf-8") + if have == want: + print("client-urls.yaml is in sync with %s" % IMPORT_FILE) + return 0 + print("CHECK FAIL: %s has DRIFTED from %s — the reconciler would converge the " + "realm on a desired state the realm import does not declare.\n" + "Regenerate with: python3 ci/render-keycloak-client-urls.py --write\n" + % (OUT_FILE, IMPORT_FILE), file=sys.stderr) + for line in difflib.unified_diff(have.splitlines(True), want.splitlines(True), + fromfile="committed " + OUT_FILE, + tofile="rendered from " + IMPORT_FILE): + sys.stderr.write(line) + return 1 + + sys.stdout.write(want) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/platform-config/kustomization.yaml b/platform-config/kustomization.yaml index 6726e29..6d5cfed 100644 --- a/platform-config/kustomization.yaml +++ b/platform-config/kustomization.yaml @@ -93,6 +93,17 @@ resources: # so it applies after the keycloak-realm-clients Secret (rendered by ./platform) and # alongside the import whose failure mode it covers. - ../platform/keycloak/client-secret-reconciler.yaml + # The DESIRED STATE for every environment-derived client URL (redirectUris, webOrigins, + # rootUrl, baseUrl, adminUrl), GENERATED from keycloak-realm-import.yaml by + # ci/render-keycloak-client-urls.py and gated by the L0 check. The reconciler pod has no + # Kubernetes API access (automountServiceAccountToken: false), so it cannot read the + # KeycloakRealmImport CR — its desired state must be MOUNTED, hence this ConfigMap. + # + # IT MUST BE RENDERED BY THIS SAME KUSTOMIZATION as the realm import above: both are then + # substituted in one pass by this Kustomization's postBuild.substituteFrom (cluster-vars), + # so a ${BASE_DOMAIN} token in either resolves to the same value and the two declarations + # cannot disagree about the domain. + - ../platform/keycloak/client-urls.yaml - ../platform/keycloak/k8s-oidc-rbac.yaml # maps OIDC groups -> cluster roles (kubectl SSO RBAC) - ../platform/keycloak/httproute.yaml - ../platform/keycloak/servicemonitor.yaml diff --git a/platform/keycloak/client-urls.yaml b/platform/keycloak/client-urls.yaml new file mode 100644 index 0000000..8fe5123 --- /dev/null +++ b/platform/keycloak/client-urls.yaml @@ -0,0 +1,157 @@ +--- +# ============================================================================= +# GENERATED FILE — DO NOT EDIT BY HAND. +# +# source: platform/keycloak/keycloak-realm-import.yaml (spec.realm.clients) +# regenerate: python3 ci/render-keycloak-client-urls.py --write +# gated by: conformance/tests/l0-static/keycloak-client-secret-reconciler +# (runs this renderer with --check on every merge) +# +# WHAT THIS IS: the DESIRED STATE for every environment-derived client URL in the +# `homelab` realm — redirectUris, webOrigins, rootUrl, baseUrl, adminUrl. The +# client-secret reconciler CronJob mounts it and makes the live realm match it. +# +# WHY IT IS A SEPARATE OBJECT: the realm import is a ONE-SHOT that never re-runs +# once Done=True, so a redirect URI edited in git never reaches the realm. Prod's +# stored redirect URIs were measured still pointing at the pre-migration hostname +# while git declared the new one, and every OIDC login would have failed with a +# redirect_uri mismatch. The reconciler is the level-triggered loop that closes +# that; this file is what it reconciles TOWARDS. +# +# WHY IT IS GENERATED AND NOT AUTHORED: a hand-written copy of every public +# hostname would be a second source of truth, free to disagree with the realm +# import. It is rendered from the import and the render is gated, so the two +# cannot drift. URL strings are copied VERBATIM — if the source uses a +# ${BASE_DOMAIN} token it appears here unchanged, and both files are substituted +# by the SAME Flux Kustomization (./platform-config), so the domain is decided in +# exactly one place. +# +# A field that is NOT declared on a client in the realm import is simply ABSENT +# here, and the reconciler leaves it alone. Absence means UNMANAGED, never +# "force to empty". +# ============================================================================= +apiVersion: v1 +kind: ConfigMap +metadata: + name: keycloak-client-urls + namespace: keycloak + labels: + app.kubernetes.io/name: keycloak-client-urls + app.kubernetes.io/part-of: keycloak +data: + client-urls.json: | + { + "crafty": { + "redirectUris": [ + "https://crafty.${BASE_DOMAIN}/oauth2/callback" + ], + "webOrigins": [ + "https://crafty.${BASE_DOMAIN}" + ] + }, + "gitea": { + "redirectUris": [ + "https://gitea.${BASE_DOMAIN}/user/oauth2/keycloak/callback" + ], + "webOrigins": [ + "https://gitea.${BASE_DOMAIN}" + ] + }, + "grafana": { + "redirectUris": [ + "https://grafana.${BASE_DOMAIN}/login/generic_oauth" + ], + "webOrigins": [ + "https://grafana.${BASE_DOMAIN}" + ] + }, + "headlamp": { + "redirectUris": [ + "https://headlamp.${BASE_DOMAIN}/oidc-callback" + ], + "webOrigins": [ + "https://headlamp.${BASE_DOMAIN}" + ] + }, + "jackett": { + "redirectUris": [ + "https://jackett.${BASE_DOMAIN}/oauth2/callback" + ], + "webOrigins": [ + "https://jackett.${BASE_DOMAIN}" + ] + }, + "kubernetes": { + "redirectUris": [ + "http://localhost:8000", + "http://localhost:8000/", + "http://localhost:18000", + "http://localhost:18000/", + "http://127.0.0.1:8000", + "http://127.0.0.1:8000/", + "http://127.0.0.1:18000", + "http://127.0.0.1:18000/" + ], + "webOrigins": [ + "http://localhost", + "http://127.0.0.1" + ] + }, + "nextcloud": { + "redirectUris": [ + "https://nextcloud.${BASE_DOMAIN}/apps/user_oidc/code" + ], + "webOrigins": [ + "https://nextcloud.${BASE_DOMAIN}" + ] + }, + "oauth2-proxy": { + "redirectUris": [ + "https://*.${BASE_DOMAIN}/oauth2/callback", + "https://auth.${BASE_DOMAIN}/oauth2/callback" + ], + "webOrigins": [ + "https://*.${BASE_DOMAIN}" + ] + }, + "pihole": { + "redirectUris": [ + "https://pihole.${BASE_DOMAIN}/oauth2/callback" + ], + "webOrigins": [ + "https://pihole.${BASE_DOMAIN}" + ] + }, + "qbittorrent": { + "redirectUris": [ + "https://qbittorrent.${BASE_DOMAIN}/oauth2/callback" + ], + "webOrigins": [ + "https://qbittorrent.${BASE_DOMAIN}" + ] + }, + "radarr": { + "redirectUris": [ + "https://radarr.${BASE_DOMAIN}/oauth2/callback" + ], + "webOrigins": [ + "https://radarr.${BASE_DOMAIN}" + ] + }, + "semaphore": { + "redirectUris": [ + "https://semaphore.${BASE_DOMAIN}/api/auth/oidc/keycloak/redirect" + ], + "webOrigins": [ + "https://semaphore.${BASE_DOMAIN}" + ] + }, + "sonarr": { + "redirectUris": [ + "https://sonarr.${BASE_DOMAIN}/oauth2/callback" + ], + "webOrigins": [ + "https://sonarr.${BASE_DOMAIN}" + ] + } + } From 23ea72d01e8a565122f635d3f92986c9ac871857 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Sat, 12 Sep 2026 21:09:04 -0500 Subject: [PATCH 2/4] feat(keycloak): reconcile client redirect URIs, not just client secrets The realm import is a one-shot that never re-runs, so a redirect URI changed in git reaches the realm never. Prod's stored URIs were measured still on the pre-migration hostname while git declared the new one -- after a BASE_DOMAIN flip every OIDC login fails with a redirect_uri mismatch, with the CR still reading Done=True and nothing alerting. Same failure class as the literal-placeholder secrets, same cure. EXTENDS the existing reconciler rather than adding a sibling. Decisive reason: a URL write needs NO new endpoint. It is the same minimal client PUT, and the current URLs already come back in the allow-listed client lookup -- so the allow-list is widened by nothing and the users/groups/roles/authentication guarantee is preserved unchanged 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 that the header explicitly forbids forking. Reconciles redirectUris, webOrigins, rootUrl, baseUrl, adminUrl -- only the fields a client actually declares, and on a write only the fields that actually drifted. A field the desired state omits is UNMANAGED and left untouched; absence never means force-to-empty. 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, which would otherwise make credential healing depend on a hostname file. List fields compare order-insensitively (sorted, not set -- duplicates are still drift), because Keycloak does not preserve the authored order of redirectUris and an order-sensitive compare would rewrite every client forever while reporting success. --selftest grows to 84 assertions covering the domain flip, a client gaining a URI, a client losing a stale one, a public client's URLs being managed while its secret is not, per-pass fail-closed, order-insensitivity, and a server that accepts a PUT without applying it. Co-Authored-By: Claude Opus 5 --- .../chainsaw-test.yaml | 2 +- .../keycloak/client-secret-reconciler.yaml | 707 ++++++++++++++++-- 2 files changed, 642 insertions(+), 67 deletions(-) diff --git a/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml b/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml index fd85f39..c384ca2 100644 --- a/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml +++ b/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml @@ -119,7 +119,7 @@ spec: image: python:3.13.15-alpine3.23 command: - python3 - - /opt/reconciler/reconcile-client-secrets.py + - /opt/reconciler/reconcile-keycloak-clients.py # READ-ONLY. --audit never issues a write on any path; the # script's --selftest proves that (L0 gate). - --audit diff --git a/platform/keycloak/client-secret-reconciler.yaml b/platform/keycloak/client-secret-reconciler.yaml index 0d231d7..9e9f9f7 100644 --- a/platform/keycloak/client-secret-reconciler.yaml +++ b/platform/keycloak/client-secret-reconciler.yaml @@ -1,6 +1,27 @@ --- # ============================================================================= -# Keycloak CLIENT-SECRET RECONCILER (R-IDP-03, R-IDP-04) +# Keycloak CLIENT RECONCILER (R-IDP-03, R-IDP-04) +# +# TWO PASSES over the realm's OIDC clients, both closing the SAME failure class -- +# the realm import is a one-shot that never re-runs, so anything it got wrong (or +# anything git changed afterwards) stays wrong forever: +# +# pass 1 CLIENT SECRETS, against the mounted SOPS keycloak-realm-clients Secret. +# The 2026-09-06 incident: all 12 clients stored the literal +# `(env:...)` placeholder and every OIDC login failed. +# pass 2 ENVIRONMENT-DERIVED CLIENT URLS (redirectUris, webOrigins, rootUrl, +# baseUrl, adminUrl), against the mounted keycloak-client-urls ConfigMap +# that ci/render-keycloak-client-urls.py generates from the realm import. +# The failure: prod's stored redirect URIs still named the PRE-migration +# hostname while git declared the new one, so after a BASE_DOMAIN flip +# every OIDC login fails with a redirect_uri mismatch. Nothing re-ran the +# import to fix it, and nothing alerted. +# +# THE TWO PASSES ARE INDEPENDENT. Each reads its OWN desired state and fails closed +# for its OWN fields only; neither can disable the other. That matters: if a +# malformed URL ConfigMap could abort the run, a typo in a hostname would stop +# client-SECRET repair, making credential healing depend on a file that has nothing +# to do with credentials. Proved by --selftest cases 6a/6b. # # CARRIED BY platform-config (dependsOn platform, wait:true), so it applies only # after the `platform` tier has applied the SOPS-encrypted keycloak-realm-clients @@ -48,6 +69,18 @@ # platform-admins / viewers, and they must survive every run. It never logs a # secret, and it writes only where the stored value differs from git. # +# ADDING PASS 2 WIDENED THE ALLOW-LIST BY NOTHING AT ALL. A URL write is the SAME +# minimal `PUT /admin/realms//clients/` the secret pass already used, +# and the current URLs come back in the client LOOKUP that was already allow-listed +# -- so pass 2 needed no new endpoint and the users/groups/roles guarantee is +# unchanged, not merely re-argued. --selftest asserts the refusal of /users, +# /groups, /roles, /authentication and the whole-realm endpoint directly. +# +# WHAT PASS 2 MAY WRITE is narrower still: only the five URL fields named above, +# and on any given client only the ones that actually drifted. A field the desired +# state does not declare is UNMANAGED and left exactly as it is -- absence never +# means "force to empty". +# # PROOF: the script's own `--selftest` drives these real code paths against an # in-process stub Keycloak and asserts compare-then-write, idempotence (a second # run over a converged realm issues ZERO writes), no-secret-logging, the endpoint @@ -70,12 +103,12 @@ data: # rendered by a Flux Kustomization with postBuild.substituteFrom, so an # unescaped dollar-brace expansion here would be rewritten before any container # ran (the trap documented at length in apps/wave-1/nextcloud.yaml). - reconcile-client-secrets.py: | + reconcile-keycloak-clients.py: | #!/usr/bin/env python3 # ============================================================================= - # reconcile-client-secrets.py -- make every Keycloak OIDC client's STORED secret - # equal the value git declares for it, idempotently, without touching anything - # else in the realm. + # reconcile-keycloak-clients.py -- make every Keycloak OIDC client's STORED secret + # AND its environment-derived URLs equal what git declares, idempotently, without + # touching anything else in the realm. # # WHY THIS EXISTS (the incident it prevents) # ------------------------------------------ @@ -94,8 +127,8 @@ data: # So the import is a ONE-SHOT that fails OPEN. This is the level-triggered # reconciler that closes it. # - # CONTRACT - # -------- + # CONTRACT -- PASS 1, CLIENT SECRETS + # --------------------------------- # Desired state files in SECRETS_DIR named `-client-secret`, each # holding exactly the secret for that clientId (this is the # `keycloak-realm-clients` Secret, mounted). @@ -103,6 +136,31 @@ data: # Action only where they differ: PUT /admin/realms//clients/ # with the MINIMAL representation {"clientId":..,"secret":..}. # + # CONTRACT -- PASS 2, ENVIRONMENT-DERIVED CLIENT URLS + # -------------------------------------------------- + # Desired state URLS_FILE, a JSON object {clientId: {field: value}} covering only + # redirectUris / webOrigins / rootUrl / baseUrl / adminUrl. It is the + # mounted `keycloak-client-urls` ConfigMap, GENERATED from + # keycloak-realm-import.yaml by ci/render-keycloak-client-urls.py and + # gated by the L0 check, so the realm import stays the single source + # of truth for every hostname. A field absent from it is UNMANAGED. + # Actual state the client LOOKUP response (which already carries these fields -- + # no extra endpoint, no extra request). + # Action only where they differ: the same minimal client PUT, carrying + # clientId plus ONLY the fields that drifted. + # + # WHY THIS PASS EXISTS: the realm import is templated per environment, so a + # BASE_DOMAIN change re-renders every declared redirect URI in git -- and reaches + # the realm never, because the import will not re-run. Measured on prod: stored + # redirect URIs on the PRE-migration hostname while git declared the new one. Every + # OIDC login would fail with a redirect_uri mismatch, with nothing alerting and the + # CR still reading Done=True. + # + # LIST FIELDS ARE COMPARED ORDER-INSENSITIVELY (sorted, not set: duplicates are + # still drift). Keycloak does not preserve the authored order of redirectUris, so an + # order-sensitive comparison would rewrite every client on every run forever while + # reporting success. + # # WHY MINIMAL: Keycloak merges non-null fields of a client representation, so a # two-field PUT rewrites the secret and nothing else. PUTting the full # round-tripped representation back has a far larger blast radius and is in fact @@ -121,7 +179,18 @@ data: # structurally unreachable from here -- prod's `homelab` realm carries a # real user and the groups media-users / platform-admins / viewers, and they # MUST survive. - # 5. public clients (e.g. `kubernetes`: PKCE, no secret) are skipped. + # 5. public clients (e.g. `kubernetes`: PKCE, no secret) are skipped BY PASS 1. + # Pass 2 does NOT skip them: a public client's redirect URIs are exactly what + # kubectl logins land on, and a stale one widens what the realm will redirect to. + # 6. the two passes are INDEPENDENT. Each reads its own desired state and fails + # closed for its own fields; an unreadable URL ConfigMap cannot stop secret + # repair, and an unmounted Secret cannot stop URL repair. + # 7. pass 2 writes ONLY the five URL fields, and on a given client only the ones + # that drifted. An undeclared field is never written at all. + # + # URLs ARE LOGGED IN FULL; SECRETS ARE NEVER LOGGED. Redirect URIs are public + # hostnames, and naming the exact before/after is what makes a domain migration + # auditable from the Job log. Property 3 is unchanged for secret values. # # MODES # (default) reconcile: compare, and write only what differs. @@ -161,6 +230,20 @@ data: READY_ATTEMPTS = int(os.environ.get("READY_ATTEMPTS", "24")) READY_SLEEP = float(os.environ.get("READY_SLEEP", "5")) + # ---- pass 2 input: the environment-derived client URLs ------------------------- + # A GENERATED ConfigMap, rendered from keycloak-realm-import.yaml by + # ci/render-keycloak-client-urls.py and gated by the L0 check, mounted read-only. + # See the header for why this is a mounted file and not a read of the CR. + URLS_FILE = os.environ.get("CLIENT_URLS_FILE", + "/etc/keycloak-client-urls/client-urls.json") + # The ONLY client fields this program may write, besides the secret. Each is an + # environment-derived URL that changes when the base domain changes. Anything not + # named here is not writable by this program at all: an unknown field in the + # desired-state file is a hard error, not something to pass through to Keycloak. + URL_LIST_FIELDS = ("redirectUris", "webOrigins") + URL_STR_FIELDS = ("rootUrl", "baseUrl", "adminUrl") + URL_FIELDS = URL_LIST_FIELDS + URL_STR_FIELDS + # Endpoint allow-list (safety property 4). fullmatch-ed against the path of every # URL _http() is asked to open; anything else raises before a socket exists. _ALLOWED = ( @@ -223,6 +306,80 @@ data: return out + def declared_urls(path): + """{clientId: {field: value}} from the mounted, generated client-URL ConfigMap. + + Validates the shape STRICTLY and raises on anything unexpected. This file decides + what gets written into the realm, so a malformed one must stop the URL pass dead + rather than be half-understood: an unknown field name here would otherwise be + forwarded to Keycloak as a client-representation field, which is exactly the + wide-blast-radius write the minimal-PUT rule exists to prevent. + """ + with open(path, "r", encoding="utf-8") as fh: + raw = json.load(fh) + if not isinstance(raw, dict) or not raw: + raise ValueError("%s does not hold a non-empty JSON object of clients" % path) + out = {} + for cid, entry in raw.items(): + if not isinstance(cid, str) or not cid: + raise ValueError("%s has a client with an empty clientId" % path) + if not isinstance(entry, dict) or not entry: + raise ValueError("%s: client %r has no declared URL fields" % (path, cid)) + clean = {} + for field, value in entry.items(): + if field not in URL_FIELDS: + raise ValueError( + "%s: client %r declares field %r, which this reconciler is not " + "allowed to write. Writable fields are: %s" + % (path, cid, field, ", ".join(URL_FIELDS))) + if field in URL_LIST_FIELDS: + if (not isinstance(value, list) + or not all(isinstance(x, str) and x for x in value)): + raise ValueError("%s: client %r field %s must be a list of " + "non-empty strings" % (path, cid, field)) + clean[field] = list(value) + else: + if not isinstance(value, str): + raise ValueError("%s: client %r field %s must be a string" + % (path, cid, field)) + clean[field] = value + out[cid] = clean + return out + + + def url_drift(client, wanted): + """{field: (stored, declared)} for every declared URL field that differs. + + LIST FIELDS ARE COMPARED AS SORTED SEQUENCES, not in the order Keycloak happens to + return them. Keycloak does not preserve the authored order of redirectUris, so an + order-sensitive comparison would report drift on every run and rewrite the client + forever -- the exact opposite of "a converged realm produces zero writes". Sorting + rather than set()-ing keeps it duplicate-sensitive, so a realm holding the same URI + twice is still drift. + + A field ABSENT from `wanted` is UNMANAGED and never inspected: the desired-state + file only carries what the realm import actually declares, and absence must mean + "leave it alone", never "force it empty". Keycloak reports an unset string field as + null or absent; both read as "" so an unset rootUrl matching a declared "" is not + perpetual drift. + """ + drift = {} + for field, declared_value in sorted(wanted.items()): + if field in URL_LIST_FIELDS: + stored = client.get(field) or [] + if not isinstance(stored, list): + stored = [] + if sorted(stored) != sorted(declared_value): + drift[field] = (sorted(stored), sorted(declared_value)) + else: + stored = client.get(field) or "" + if not isinstance(stored, str): + stored = "" + if stored != declared_value: + drift[field] = (stored, declared_value) + return drift + + def admin_token(base, user, password, log): """Bearer token for admin-cli in the master realm, with a bounded readiness wait.""" last = "" @@ -315,6 +472,86 @@ data: return changed, problems + def reconcile_url_fields(base, realm, token, want, audit, log): + """PASS 2 -- make every declared client URL match git. Return (changed, problems). + + Independent of the secret pass by construction: it shares only the bearer token and + the endpoint allow-list, reads a different input file, and writes a disjoint set of + client fields. Neither pass can disable the other (see run()). + + Unlike the secret pass this does NOT skip public clients. A public client's redirect + URIs matter just as much -- `kubernetes` is PKCE-public and its localhost callbacks + are what kubectl logins land on -- and a stale URI left behind on any client widens + what the realm will redirect to. + + URL VALUES ARE LOGGED IN FULL, deliberately. They are public hostnames, not + secrets, and naming the exact before/after is what makes a domain migration + auditable from the Job log. The secret pass still never prints a value. + """ + hdrs = {"Authorization": "Bearer " + token} + changed = 0 + problems = [] + for cid in sorted(want): + st, text = _http("GET", base, "/admin/realms/%s/clients" % realm, + query={"clientId": cid}, headers=hdrs) + if st != 200: + problems.append("%s: client lookup failed (http %s)" % (cid, st)) + continue + found = json.loads(text) + if not isinstance(found, list) or not found: + problems.append("%s: NOT FOUND in realm %s -- git declares URLs for it but " + "the realm has no such client (the operator runs the realm " + "import exactly once)" % (cid, realm)) + continue + client = found[0] + uuid = client.get("id") + drift = url_drift(client, want[cid]) + if not drift: + log("%-14s urls ok (already correct, no write)" % cid) + continue + for field in sorted(drift): + stored, declared_value = drift[field] + if audit: + problems.append("%s: %s DRIFT -- realm has %r, git declares %r" + % (cid, field, stored, declared_value)) + else: + log("%-14s %s DRIFT -- realm has %r, git declares %r" + % (cid, field, stored, declared_value)) + if audit: + continue + # MINIMAL representation, same rule as the secret pass: clientId plus ONLY the + # fields that actually drifted. Keycloak merges non-null fields, so the client's + # secret, protocol mappers, scopes, flags and every realm-level object are + # untouched by this write. A full round-tripped representation is rejected + # outright (400 "Cannot parse the JSON") and is modelled as such in --selftest. + rep = {"clientId": client.get("clientId")} + for field in sorted(drift): + rep[field] = want[cid][field] + st, text = _http("PUT", base, "/admin/realms/%s/clients/%s" % (realm, uuid), + headers=hdrs, body=rep) + if not 200 <= st < 300: + problems.append("%s: url PUT failed (http %s) %s" % (cid, st, text[:160])) + continue + # Readback through the same allow-listed lookup: a 204 only says the request + # parsed, not that the realm now holds what git declares. + st, text = _http("GET", base, "/admin/realms/%s/clients" % realm, + query={"clientId": cid}, headers=hdrs) + after = json.loads(text) if st == 200 else [] + if not isinstance(after, list) or not after: + problems.append("%s: url PUT was accepted but the client could not be " + "re-read to verify it" % cid) + continue + still = url_drift(after[0], want[cid]) + if still: + problems.append("%s: url PUT was accepted but %s still does not match git" + % (cid, ", ".join(sorted(still)))) + continue + changed += 1 + log("%-14s urls REPAIRED (%s) and verified by readback" + % (cid, ", ".join(sorted(drift)))) + return changed, problems + + def run(argv, log): audit = "--audit" in argv base = os.environ.get("KEYCLOAK_BASE_URL", "").strip() @@ -329,45 +566,129 @@ data: return 1 log("mode=%s realm=%s base=%s" % ("audit (read-only)" if audit else "reconcile", realm, base)) + + # ---- READ BOTH DESIRED STATES FIRST, AND INDEPENDENTLY --------------------- + # Each pass fails closed for ITS OWN fields and MUST NOT be able to disable the + # other. If the URL ConfigMap were allowed to gate the whole run, an unapplied or + # malformed ConfigMap would stop client-SECRET repair -- making SSO credential + # healing depend on a file that has nothing to do with credentials. That would be + # a strictly worse posture than before this pass existed, so the two inputs are + # read separately and a failure in one is recorded, not propagated. + want_secrets = None + secret_input_fail = "" try: - want = declared(SECRETS_DIR) + want_secrets = declared(SECRETS_DIR) except OSError as e: - log("FATAL: cannot read %s (%s)" % (SECRETS_DIR, e.__class__.__name__)) + secret_input_fail = ("cannot read %s (%s)" + % (SECRETS_DIR, e.__class__.__name__)) + if want_secrets is not None and not want_secrets: + want_secrets = None + secret_input_fail = ( + "no `%s` keys found in %s -- the keycloak-realm-clients Secret " + "is missing or empty, and reporting success here would repeat the very " + "fail-open the realm import already has" % (KEY_SUFFIX, SECRETS_DIR)) + + want_urls = None + url_input_fail = "" + try: + want_urls = declared_urls(URLS_FILE) + except OSError as e: + url_input_fail = ("cannot read %s (%s) -- the keycloak-client-urls ConfigMap " + "is not mounted" % (URLS_FILE, e.__class__.__name__)) + except (ValueError, TypeError) as e: + url_input_fail = "%s is unusable: %s" % (URLS_FILE, e) + + if want_secrets is None and want_urls is None: + log("FATAL: neither desired state could be read, so nothing can be " + "reconciled and NOTHING WAS WRITTEN.") + log("FATAL: secrets: %s" % secret_input_fail) + log("FATAL: urls: %s" % url_input_fail) return 1 - if not want: - log("FATAL: no `%s` keys found in %s -- the keycloak-realm-clients " - "Secret is missing or empty, and reporting success here would repeat the " - "very fail-open the realm import already has" - % (KEY_SUFFIX, SECRETS_DIR)) - return 1 - log("declared clients (%d): %s" % (len(want), ", ".join(sorted(want)))) + if want_secrets is not None: + log("declared client secrets (%d): %s" + % (len(want_secrets), ", ".join(sorted(want_secrets)))) + if want_urls is not None: + log("declared client urls (%d): %s" + % (len(want_urls), ", ".join(sorted(want_urls)))) + try: token = admin_token(base, user, password, log) - changed, problems = reconcile(base, realm, token, want, audit, log) except Refused as e: log("FATAL: %s" % e) return 1 except (RuntimeError, OSError, ValueError) as e: log("FATAL: %s: %s" % (e.__class__.__name__, e)) return 1 + + rc = 0 + problems = [] + + # ---- PASS 1: client secrets ------------------------------------------------ + if want_secrets is None: + log("") + log("SECRETS PASS SKIPPED -- FAILED CLOSED, no secret was written: %s" + % secret_input_fail) + rc = 1 + else: + log("") + log("-- pass 1: client secrets --") + try: + changed, found = reconcile(base, realm, token, want_secrets, audit, log) + except Refused as e: + log("FATAL: %s" % e) + return 1 + except (RuntimeError, OSError, ValueError) as e: + log("FATAL: %s: %s" % (e.__class__.__name__, e)) + return 1 + problems.extend(found) + if found: + rc = 1 + elif audit: + log("KEYCLOAK_CLIENT_SECRETS_AUDIT_OK: all %d declared client secret(s) " + "match git" % len(want_secrets)) + else: + log("KEYCLOAK_CLIENT_SECRETS_RECONCILED_OK: %d client(s) checked, %d " + "repaired" % (len(want_secrets), changed)) + + # ---- PASS 2: environment-derived client URLs ------------------------------- + if want_urls is None: + log("") + log("URL PASS SKIPPED -- FAILED CLOSED, no URL was written: %s" + % url_input_fail) + rc = 1 + else: + log("") + log("-- pass 2: client urls --") + try: + changed, found = reconcile_url_fields(base, realm, token, want_urls, + audit, log) + except Refused as e: + log("FATAL: %s" % e) + return 1 + except (RuntimeError, OSError, ValueError) as e: + log("FATAL: %s: %s" % (e.__class__.__name__, e)) + return 1 + problems.extend(found) + if found: + rc = 1 + elif audit: + log("KEYCLOAK_CLIENT_URLS_AUDIT_OK: all %d declared client URL set(s) " + "match git" % len(want_urls)) + else: + log("KEYCLOAK_CLIENT_URLS_RECONCILED_OK: %d client(s) checked, %d " + "repaired" % (len(want_urls), changed)) + if problems: log("") for p in problems: log("PROBLEM %s" % p) log("") if audit: - log("AUDIT FAIL: %d client secret(s) do not match git" % len(problems)) + log("AUDIT FAIL: %d client field(s) do not match git" % len(problems)) else: - log("RECONCILE FAIL: %d client secret(s) could not be reconciled" + log("RECONCILE FAIL: %d client field(s) could not be reconciled" % len(problems)) - return 1 - if audit: - log("KEYCLOAK_CLIENT_SECRETS_AUDIT_OK: all %d declared client secret(s) match " - "git" % len(want)) - else: - log("KEYCLOAK_CLIENT_SECRETS_RECONCILED_OK: %d client(s) checked, %d repaired" - % (len(want), changed)) - return 0 + return rc # ============================================================================= @@ -392,7 +713,7 @@ data: import tempfile import threading - global SECRETS_DIR, READY_ATTEMPTS, READY_SLEEP + global SECRETS_DIR, URLS_FILE, READY_ATTEMPTS, READY_SLEEP SECRET_A = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" SECRET_B = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" @@ -403,7 +724,11 @@ data: # spelled this way so the file stays free of the character envsubst reacts to. LITERAL = chr(36) + "(env:PIHOLE_CLIENT_SECRET)" - state = {"clients": {}, "writes": [], "touched": []} + # "lying" holds clientIds for which the stub ACCEPTS a URL PUT (204) and then does + # NOT apply it -- a server that reports success without converging. Only the + # readback catches that, so it is what proves the readback is load-bearing. + state = {"clients": {}, "secret_writes": [], "url_writes": [], + "touched": [], "lying": set()} class Stub(http.server.BaseHTTPRequestHandler): def log_message(self, *a): @@ -438,15 +763,32 @@ data: if m and method == "PUT": raw = self.rfile.read(int(self.headers.get("Content-Length") or 0)) rep = json.loads(raw or b"{}") + keys = set(rep) # The live server rejects a full round-tripped representation with # 400 "Cannot parse the JSON"; modelled so a regression to the fat - # PUT fails here rather than against a real realm. - if set(rep) != {"clientId", "secret"}: + # PUT fails here rather than against a real realm. Exactly two minimal + # shapes are accepted: the secret write, and a URL write carrying only + # clientId plus writable URL fields. + is_secret_put = keys == {"clientId", "secret"} + rest = keys - {"clientId"} + is_url_put = ("clientId" in keys and rest + and rest.issubset(set(URL_FIELDS))) + if not (is_secret_put or is_url_put): return self._send(400, { "error": "unknown_error", "error_description": "Cannot parse the JSON"}) - state["writes"].append(m.group(1)) - state["clients"][m.group(1)]["secret"] = rep["secret"] + target = state["clients"][m.group(1)] + if is_secret_put: + state["secret_writes"].append(m.group(1)) + target["secret"] = rep["secret"] + else: + # Record WHICH fields were written: the proof that a URL write + # never carries a secret and never widens past the drifted field. + state["url_writes"].append( + (m.group(1), tuple(sorted(rest)))) + if m.group(1) not in state["lying"]: + for f in rest: + target[f] = rep[f] return self._send(204, {}) return self._send(500, {"error": "an endpoint the reconciler must not touch"}) @@ -472,6 +814,41 @@ data: os.mkdir(os.path.join(tmp, "..data")) SECRETS_DIR = tmp + + # ---- pass-2 fixture: THE DOMAIN MIGRATION, modelled ------------------------- + # git declares the NEW apex hostnames; the realm below still holds the OLD + # pre-migration ones, which is exactly what was measured on prod (stored + # grafana redirectUri on `grafana.prod.home.` while git declared `grafana.home.`). + NEW = "home.keithjackson.dev" + OLD = "prod.home.keithjackson.dev" + WANT_URLS = { + # drifted: the domain flip. Also proves a MULTI-FIELD write. + "grafana": { + "redirectUris": ["https://grafana." + NEW + "/login/generic_oauth"], + "webOrigins": ["https://grafana." + NEW], + }, + # already converged: must take ZERO writes even though its secret drifts. + "pihole": { + "redirectUris": ["https://pihole." + NEW + "/oauth2/callback"], + }, + # a PUBLIC client still has its URLs managed -- kubectl logins land on these. + # Declares TWO URIs where the realm holds one: proves GAINING a URI. + "kubernetes": { + "redirectUris": ["http://localhost:8000", "http://localhost:18000"], + }, + # declares ONE URI where the realm holds two: proves LOSING a URI. A stale + # wildcard left behind is a real widening of what the realm will redirect to. + "gitea": { + "redirectUris": ["https://gitea." + NEW + "/user/oauth2/keycloak/callback"], + "rootUrl": "https://gitea." + NEW, + }, + } + urls_dir = tempfile.mkdtemp() + URLS_GOOD = os.path.join(urls_dir, "client-urls.json") + with open(URLS_GOOD, "w", encoding="utf-8") as fh: + json.dump(WANT_URLS, fh) + URLS_FILE = URLS_GOOD + READY_ATTEMPTS, READY_SLEEP = 2, 0.01 os.environ.update({"KEYCLOAK_BASE_URL": base, "KEYCLOAK_REALM": "homelab", "KEYCLOAK_ADMIN_USER": "admin", @@ -486,73 +863,187 @@ data: failures.append(what) def invoke(*argv): - state["writes"] = [] + state["secret_writes"] = [] + state["url_writes"] = [] state["touched"] = [] buf = io.StringIO() with contextlib.redirect_stdout(buf): rc = run(list(argv), print) return rc, buf.getvalue() + def url_writes(): + """[(clientId, (fields written, ...))] sorted -- the URL blast radius.""" + return sorted(state["url_writes"]) + def reset(): + state["lying"] = set() state["clients"] = { - # already correct + # secret already correct; URLs on the OLD domain (the migration drift) "grafana": {"clientId": "grafana", "publicClient": False, - "secret": SECRET_A}, - # THE INCIDENT: the literal, unresolved import placeholder - "pihole": {"clientId": "pihole", "publicClient": False, "secret": LITERAL}, - # public client: no secret, must be skipped - "kubernetes": {"clientId": "kubernetes", "publicClient": True}, - # ordinary drift (rotated in git, stale in the realm) - "gitea": {"clientId": "gitea", "publicClient": False, "secret": STALE}, + "secret": SECRET_A, + "redirectUris": ["https://grafana." + OLD + + "/login/generic_oauth"], + "webOrigins": ["https://grafana." + OLD]}, + # THE INCIDENT: the literal, unresolved import placeholder. + # URLs are ALREADY CONVERGED -- proves the two passes are independent: + # a secret repair here must not drag a URL write along with it. + # adminUrl is set in the realm and NOT declared in git: an UNMANAGED field. + # It must survive untouched. "Not declared" must mean "leave it alone", + # never "force it empty" -- an operator-set adminUrl or a field a future + # Keycloak version populates itself must not be wiped by this reconciler. + "pihole": {"clientId": "pihole", "publicClient": False, "secret": LITERAL, + "redirectUris": ["https://pihole." + NEW + "/oauth2/callback"], + "adminUrl": "https://pihole." + NEW + "/admin"}, + # public client: no secret (skipped by pass 1) but URLs ARE managed. + # Holds one URI where git declares two => must GAIN one. + "kubernetes": {"clientId": "kubernetes", "publicClient": True, + "redirectUris": ["http://localhost:8000"]}, + # ordinary secret drift, plus a STALE EXTRA redirect URI that must be + # REMOVED, plus an unset rootUrl that must be set. + "gitea": {"clientId": "gitea", "publicClient": False, "secret": STALE, + "redirectUris": [ + "https://gitea." + NEW + "/user/oauth2/keycloak/callback", + "https://gitea." + OLD + "/user/oauth2/keycloak/callback"]}, } def leaked(text): return [s for s in (SECRET_A, SECRET_B, SECRET_C, STALE, ADMIN_PW) if s in text] - # -- 1. audit of a drifted realm: fails, names the placeholder, writes NOTHING + # -- 1. audit of a drifted realm: fails, names the drift, writes NOTHING reset() rc, out = invoke("--audit") - expect(rc == 1, "audit must exit 1 when a stored secret differs from git") - expect(state["writes"] == [], - "audit mode must never write (writes=%r)" % state["writes"]) + expect(rc == 1, "audit must exit 1 when a stored value differs from git") + expect(state["secret_writes"] == [] and state["url_writes"] == [], + "audit mode must never write (secrets=%r urls=%r)" + % (state["secret_writes"], state["url_writes"])) expect("UNRESOLVED IMPORT PLACEHOLDER" in out, "audit must name the unresolved-placeholder case explicitly") expect("pihole: DRIFT" in out and "gitea: DRIFT" in out, - "audit must report BOTH drifted clients, not stop at the first") + "audit must report BOTH drifted client secrets, not stop at the first") expect(re.search(r"grafana\s+ok \(already correct", out), - "an already-correct client must be reported as such") + "an already-correct client secret must be reported as such") expect(re.search(r"kubernetes\s+skipped \(public client", out), - "a public client must be skipped, not treated as drift") + "a public client must be skipped BY THE SECRET PASS, not treated as drift") expect(not leaked(out), "audit output leaked a secret: %r" % leaked(out)) + # the URL half of the same audit + expect("grafana: redirectUris DRIFT" in out and "grafana: webOrigins DRIFT" in out, + "audit must report EVERY drifted URL field of a client, not just the first") + expect("kubernetes: redirectUris DRIFT" in out, + "audit must report URL drift on a PUBLIC client -- its URLs are managed " + "even though its secret is not") + expect(re.search(r"pihole\s+urls ok \(already correct", out), + "a client whose URLs are already converged must be reported as such") + expect(OLD in out, + "a URL audit must print the actual stored value -- URLs are public " + "hostnames, and naming the before/after is what makes a migration auditable") - # -- 2. reconcile: repairs exactly the drifted clients, verified by readback + # -- 2. reconcile: repairs exactly what drifted, in both passes, readback-verified reset() rc, out = invoke() - expect(rc == 0, "reconcile must exit 0 after repairing (tail=%r)" % out[-300:]) - expect(sorted(state["writes"]) == ["gitea", "pihole"], - "reconcile must write exactly the drifted clients, got %r" % state["writes"]) - expect("grafana" not in state["writes"], - "compare-then-write: an already-correct client must not be written") - expect("kubernetes" not in state["writes"], "a public client must not be written") + expect(rc == 0, "reconcile must exit 0 after repairing (tail=%r)" % out[-400:]) + expect(sorted(state["secret_writes"]) == ["gitea", "pihole"], + "the secret pass must write exactly the secret-drifted clients, got %r" + % state["secret_writes"]) + expect("grafana" not in state["secret_writes"], + "compare-then-write: an already-correct secret must not be written") + expect("kubernetes" not in state["secret_writes"], + "a public client must not be written by the secret pass") expect("REPAIRED and verified by readback" in out, "every repair must be readback-verified") - expect("2 repaired" in out, "the summary must count the repairs") expect(not leaked(out), "reconcile output leaked a secret: %r" % leaked(out)) + # -- 2a. THE URL BLAST RADIUS, field by field. This is the whole point: a write + # carries ONLY the fields that drifted, for ONLY the clients that drifted. + expect(url_writes() == [ + ("gitea", ("redirectUris", "rootUrl")), + ("grafana", ("redirectUris", "webOrigins")), + ("kubernetes", ("redirectUris",)), + ], + "the URL pass must write exactly the drifted FIELDS of exactly the drifted " + "clients, got %r" % (url_writes(),)) + expect(not [w for w in state["url_writes"] if w[0] == "pihole"], + "INDEPENDENCE: pihole's secret was repaired but its URLs were already " + "converged, so it must take NO url write") + expect(not [w for w in state["url_writes"] if "secret" in w[1]], + "a URL write must never carry the `secret` field") + # the corrections actually landed, including the ones that are not simple swaps + expect(state["clients"]["grafana"]["redirectUris"] + == ["https://grafana." + NEW + "/login/generic_oauth"], + "the drifted redirectUri must be CORRECTED to the declared value, got %r" + % (state["clients"]["grafana"]["redirectUris"],)) + expect(sorted(state["clients"]["kubernetes"]["redirectUris"]) + == sorted(["http://localhost:8000", "http://localhost:18000"]), + "a client GAINING a redirect URI must end up with both, got %r" + % (state["clients"]["kubernetes"]["redirectUris"],)) + expect(state["clients"]["gitea"]["redirectUris"] + == ["https://gitea." + NEW + "/user/oauth2/keycloak/callback"], + "a client LOSING a redirect URI must have the stale one REMOVED (a stale " + "URI left behind widens what the realm will redirect to), got %r" + % (state["clients"]["gitea"]["redirectUris"],)) + expect(state["clients"]["gitea"].get("rootUrl") == "https://gitea." + NEW, + "a previously-unset rootUrl must be set to the declared value") + expect("urls REPAIRED" in out, "a URL repair must be logged as such") + # UNMANAGED MEANS UNTOUCHED. pihole holds an adminUrl that git does not declare. + # Absence from the desired state must never be read as "force this to empty". + expect(state["clients"]["pihole"].get("adminUrl") + == "https://pihole." + NEW + "/admin", + "a field the desired state does NOT declare must be left exactly as it " + "was, not wiped, got %r" % (state["clients"]["pihole"].get("adminUrl"),)) + + # -- 2b. READBACK IS LOAD-BEARING. A server that accepts the PUT with 204 and does + # not apply it must be reported as a FAILURE, not as a repair. Without the + # readback this is indistinguishable from success -- and "reported success + # while the realm never converged" is the entire failure class being closed. + reset() + state["lying"] = {"grafana"} + rc, out = invoke() + expect(rc == 1, "a PUT accepted but not applied must fail the run, not pass it") + expect("still does not match git" in out, + "the readback must name the client whose write did not take effect") + expect(not [ln for ln in out.splitlines() + if "urls REPAIRED" in ln and "grafana" in ln], + "a client whose write did not take effect must NOT be logged as repaired") + # Re-converge before the idempotence check below, which requires a converged realm. + state["lying"] = set() + invoke() # -- 3. IDEMPOTENCE: re-running against the converged realm writes nothing rc, out = invoke() expect(rc == 0, "the second reconcile must exit 0") - expect(state["writes"] == [], - "IDEMPOTENCE: a converged realm must take ZERO writes, got %r" - % state["writes"]) + expect(state["secret_writes"] == [] and state["url_writes"] == [], + "IDEMPOTENCE: a converged realm must take ZERO writes, got secrets=%r " + "urls=%r" % (state["secret_writes"], state["url_writes"])) expect("0 repaired" in out, "the second run must report 0 repaired") rc, out = invoke("--audit") expect(rc == 0, "audit must pass once the realm has converged") expect("KEYCLOAK_CLIENT_SECRETS_AUDIT_OK" in out, - "a clean audit must emit the sentinel the L2 conformance test greps for") + "a clean audit must emit the secret sentinel the L2 test greps for") + expect("KEYCLOAK_CLIENT_URLS_AUDIT_OK" in out, + "a clean audit must emit the URL sentinel the L2 test greps for") - # -- 4. BLAST RADIUS: only the allow-listed endpoint shapes are ever touched + # -- 3a. ORDER-INSENSITIVITY. Keycloak does not preserve the authored order of + # redirectUris. If the comparison were order-sensitive, a converged realm + # would be rewritten on EVERY run -- a permanent write loop against the + # IdP database that still reports success. + state["clients"]["kubernetes"]["redirectUris"] = [ + "http://localhost:18000", "http://localhost:8000"] + rc, out = invoke() + expect(rc == 0, "a reordered-but-equal URI list must still exit 0") + expect(state["url_writes"] == [], + "ORDER-INSENSITIVITY: a realm holding the declared URIs in a different " + "order is CONVERGED and must take zero writes, got %r" + % (state["url_writes"],)) + # ... but a DUPLICATE is still drift: sorted(), not set(). + state["clients"]["kubernetes"]["redirectUris"] = [ + "http://localhost:8000", "http://localhost:8000", "http://localhost:18000"] + rc, out = invoke() + expect(url_writes() == [("kubernetes", ("redirectUris",))], + "a duplicated redirect URI is still drift and must be corrected, got %r" + % (url_writes(),)) + + # -- 4. BLAST RADIUS: only the allow-listed endpoint shapes are ever touched. + # The URL pass added NO endpoint: it reuses the client lookup and the same + # minimal client PUT, so users/groups/roles stay structurally unreachable. reset() invoke() for touched in state["touched"]: @@ -569,6 +1060,10 @@ data: "the allow-list must refuse a users endpoint before opening a socket") expect(_refused("/admin/realms/homelab/groups"), "the allow-list must refuse a groups endpoint before opening a socket") + expect(_refused("/admin/realms/homelab/roles"), + "the allow-list must refuse a roles endpoint before opening a socket") + expect(_refused("/admin/realms/homelab/authentication/flows"), + "the allow-list must refuse an authentication-flow endpoint") expect(_refused("/admin/realms/homelab"), "the allow-list must refuse a whole-realm update endpoint") @@ -579,13 +1074,77 @@ data: expect(rc == 1, "a declared client missing from the realm must fail the audit") expect("NOT FOUND" in out, "the missing client must be named in the output") - # -- 6. an empty / unmounted Secret must fail closed, never report success + # -- 6. FAIL CLOSED, AND ONLY FOR THE AFFECTED PASS. + # 6a. an empty / unmounted Secret: no secret is written, and the URL pass still + # runs. Before pass 2 existed a missing Secret aborted the run; it must not + # now be able to abort URL reconciliation either, nor vice versa (6b). + reset() SECRETS_DIR = tempfile.mkdtemp() rc, out = invoke() expect(rc == 1, "an empty secrets dir must fail closed") expect("keys found in" in out, "the empty-secret failure must say what is missing") + expect(state["secret_writes"] == [], + "FAIL CLOSED: an unreadable secret desired-state must write NO secret, " + "got %r" % (state["secret_writes"],)) + expect(state["url_writes"] != [], + "INDEPENDENCE: an unreadable SECRET input must not disable the URL pass -- " + "SSO URL healing cannot depend on the credentials file") SECRETS_DIR = tmp + # 6b. the mirror image: an unmounted / malformed URL ConfigMap must write no URL + # and must NOT stop client-secret repair, which is the more critical half. + reset() + URLS_FILE = os.path.join(urls_dir, "does-not-exist.json") + rc, out = invoke() + expect(rc == 1, "a missing URL desired-state file must fail closed") + expect(state["url_writes"] == [], + "FAIL CLOSED: an unreadable URL desired-state must write NO url, got %r" + % (state["url_writes"],)) + expect(sorted(state["secret_writes"]) == ["gitea", "pihole"], + "INDEPENDENCE: a missing URL ConfigMap must not disable client-SECRET " + "repair, got %r" % (state["secret_writes"],)) + expect("FAILED CLOSED" in out, "the fail-closed path must say so explicitly") + + # 6c. a desired-state file naming a field this program may not write is REFUSED + # wholesale -- it must never be forwarded to Keycloak as a client field. + reset() + bad = os.path.join(urls_dir, "bad-field.json") + with open(bad, "w", encoding="utf-8") as fh: + json.dump({"grafana": {"serviceAccountsEnabled": True}}, fh) + URLS_FILE = bad + rc, out = invoke() + expect(rc == 1, "a desired-state file with a non-writable field must fail closed") + expect(state["url_writes"] == [], + "a non-writable field must produce NO write at all, got %r" + % (state["url_writes"],)) + expect("not allowed to write" in out, + "the refusal must name the problem as a non-writable field") + + # 6d. an empty desired state is fail-closed, not "everything is unmanaged" + reset() + empty = os.path.join(urls_dir, "empty.json") + with open(empty, "w", encoding="utf-8") as fh: + fh.write("{}") + URLS_FILE = empty + rc, out = invoke() + expect(rc == 1, "an empty URL desired state must fail closed, not silently pass") + expect(state["url_writes"] == [], "an empty desired state must write nothing") + URLS_FILE = URLS_GOOD + + # -- 7. BOTH desired states unreadable: nothing is written at all. + reset() + SECRETS_DIR = tempfile.mkdtemp() + URLS_FILE = os.path.join(urls_dir, "also-missing.json") + rc, out = invoke() + expect(rc == 1, "with neither input readable the run must fail") + expect(state["secret_writes"] == [] and state["url_writes"] == [], + "with neither input readable NOTHING may be written, got secrets=%r urls=%r" + % (state["secret_writes"], state["url_writes"])) + expect("NOTHING WAS WRITTEN" in out, + "the total-failure path must state plainly that nothing was written") + SECRETS_DIR = tmp + URLS_FILE = URLS_GOOD + srv.shutdown() print("") if failures: @@ -594,7 +1153,8 @@ data: print(" FAIL %s" % f) return 1 print("SELFTEST OK: %d assertions (compare-then-write, idempotence, " - "no-secret-logging, endpoint allow-list, fail-closed)" % checks[0]) + "order-insensitivity, per-pass fail-closed, minimal-write blast radius, " + "no-secret-logging, endpoint allow-list)" % checks[0]) return 0 @@ -668,7 +1228,7 @@ spec: imagePullPolicy: IfNotPresent command: - python3 - - /opt/reconciler/reconcile-client-secrets.py + - /opt/reconciler/reconcile-keycloak-clients.py env: - name: KEYCLOAK_BASE_URL value: http://keycloak-service.keycloak.svc.cluster.local:8080 @@ -703,6 +1263,14 @@ spec: - name: client-secrets mountPath: /etc/keycloak-realm-clients readOnly: true + # The DESIRED state for pass 2: every environment-derived client URL, + # GENERATED from keycloak-realm-import.yaml (ci/render-keycloak-client- + # urls.py) so there is one source of truth for the hostnames. Mounted + # for the same reason as the Secret above: this pod has no Kubernetes + # API access, so it cannot read the KeycloakRealmImport CR itself. + - name: client-urls + mountPath: /etc/keycloak-client-urls + readOnly: true securityContext: allowPrivilegeEscalation: false readOnlyRootFilesystem: true @@ -727,3 +1295,10 @@ spec: # sets. 0400 is owner-only and the owner is root, not the 65532 the container # runs as. See the fsGroup comment above. defaultMode: 0440 + # The URL desired state. 0444 is fine where the Secret needs 0440: these are + # public hostnames, not credentials. fsGroup 65532 (set above) makes it readable + # either way. + - name: client-urls + configMap: + name: keycloak-client-urls + defaultMode: 0444 From 6202d92c27845540d961dca9ce0bd5c15cfd9fb3 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Sat, 12 Sep 2026 21:15:25 -0500 Subject: [PATCH 3/4] test(conformance): gate the URL desired state, and prove the selftest can fail L0 (212 assertions, was 175): * runs ci/render-keycloak-client-urls.py --check, so client-urls.yaml must be exactly what the realm import renders. A hand edit that makes the two disagree is unmergeable -- this is what keeps the realm import the single source of truth for hostnames rather than just the place they were first typed. * asserts client-urls.yaml and the realm import are rendered by the SAME Flux Kustomization. postBuild.substituteFrom is per-Kustomization, so a split would let one ${BASE_DOMAIN} resolve to two different values and the realm would converge on a hostname the HTTPRoutes do not serve. * asserts the desired state uses no substitution token the realm definition lacks, names only clients the import declares, and declares no field outside the five-field write surface; that the CronJob mounts it readOnly at the path the script defaults to; and that the script, renderer and check agree on which fields are writable. * NEW mutations.py: injects 12 plausible regressions into the EXTRACTED script and requires --selftest to catch every one. A passing suite proves nothing if it cannot fail, and "reported success while the realm was broken" is precisely what this component exists to end -- so the same standard applies to its own test. ~20s. L2: the audit Job now mounts keycloak-client-urls and asserts KEYCLOAK_CLIENT_URLS_AUDIT_OK. Without that it would pass on a realm still holding pre-migration redirect URIs -- the exact state prod was measured in. Alerts: the existing Failing/Stale pair is EXTENDED to name redirect-URI drift, not duplicated. One CronJob gets one pair; a second pair on the same job_name regex would fire twice for one cause. No new critical, no new permanently-firing rule. Co-Authored-By: Claude Opus 5 --- .../check.sh | 225 +++++++++++++++++- .../mutations.py | 190 +++++++++++++++ .../chainsaw-test.yaml | 40 +++- platform/monitoring/alerts.yaml | 30 ++- 4 files changed, 465 insertions(+), 20 deletions(-) create mode 100755 conformance/tests/l0-static/keycloak-client-secret-reconciler/mutations.py diff --git a/conformance/tests/l0-static/keycloak-client-secret-reconciler/check.sh b/conformance/tests/l0-static/keycloak-client-secret-reconciler/check.sh index 185e9d5..6f672ee 100755 --- a/conformance/tests/l0-static/keycloak-client-secret-reconciler/check.sh +++ b/conformance/tests/l0-static/keycloak-client-secret-reconciler/check.sh @@ -56,6 +56,23 @@ # no-secret-logging, the endpoint allow-list (users/groups/roles are # unreachable) and fail-closed behaviour. What CI proves is byte-for-byte # what ships. +# (6b) AND THAT PROOF IS FALSIFIABLE: mutations.py injects plausible regressions +# into the extracted script one at a time and requires --selftest to CATCH +# every one. A passing suite proves nothing if it cannot fail, and "reported +# success while the realm was broken" is the exact failure this component +# exists to end — so the same standard is applied to its own test. +# (8) THE URL DESIRED STATE IS DERIVED, NOT DECLARED TWICE: the reconciler's +# second pass converges the realm's environment-derived client URLs +# (redirectUris, webOrigins, rootUrl, baseUrl, adminUrl) on +# platform/keycloak/client-urls.yaml. That file is GENERATED from the realm +# import by ci/render-keycloak-client-urls.py, and this check RUNS the +# renderer with --check, so a hand edit that makes the two disagree is +# unmergeable. It further asserts the two files are rendered by the SAME Flux +# Kustomization (postBuild.substituteFrom is per-Kustomization, so a split +# would let one ${BASE_DOMAIN} resolve to two different values), that the +# desired state uses no substitution token the realm definition does not, +# that every client it names exists in the import, and that it declares no +# field outside the reconciler's five-field write surface. # AND (7) THIS CHECK IS LOAD-BEARING: ci/l0.sh runs it unconditionally, ci/gate.sh # blocks the merge when the l0 stage fails, ci/touched-requirements.sh maps # platform/keycloak/ to these requirement IDs, and an L2 test exists that @@ -63,8 +80,13 @@ # and isn't" failure this repo keeps hitting. # # NOT proven here (named honestly, in the style of the sibling L0 checks): -# - that the LIVE realm's stored secrets match git. That is cluster state; it is -# conformance/tests/l2-behavioral/keycloak-client-secrets-match-git. +# - that the LIVE realm's stored secrets OR redirect URIs match git. That is cluster +# state; it is conformance/tests/l2-behavioral/keycloak-client-secrets-match-git, +# which runs the reconciler's own --audit against the live realm. +# - that the hostnames the realm import declares are the ones the Gateway/HTTPRoutes +# actually serve. This check proves the realm converges on what GIT says; it cannot +# prove git is right about the domain. A BASE_DOMAIN flip that misses an HTTPRoute is +# a different invariant. # - that the SOPS ciphertext decrypts to a usable secret. Key PRESENCE is checkable # offline; the plaintext is not, and must not be. # - that the CronJob's image can be pulled or its schedule fires. @@ -77,9 +99,10 @@ PYTHON="${PYTHON:-python3}" IMPORT_FILE="platform/keycloak/keycloak-realm-import.yaml" SOPS_FILE="platform/keycloak/keycloak-realm-clients.sops.yaml" RECON_FILE="platform/keycloak/client-secret-reconciler.yaml" +URLS_FILE="platform/keycloak/client-urls.yaml" # Honest SKIP (never green) if the thing under test is absent entirely. -for f in "$IMPORT_FILE" "$SOPS_FILE" "$RECON_FILE"; do +for f in "$IMPORT_FILE" "$SOPS_FILE" "$RECON_FILE" "$URLS_FILE"; do if [ ! -f "${ROOT}/${f}" ]; then echo "check SKIPPED: ${f} absent — nothing to audit" >&2 exit 3 @@ -95,6 +118,7 @@ the Flux graph walk rather than forked, so there is one definition of "what a Fl renders" in the harness.""" from __future__ import annotations +import json import re import subprocess import sys @@ -116,10 +140,21 @@ import status as st # noqa: E402 — reuse the walk, don't f IMPORT_FILE = "platform/keycloak/keycloak-realm-import.yaml" SOPS_FILE = "platform/keycloak/keycloak-realm-clients.sops.yaml" RECON_FILE = "platform/keycloak/client-secret-reconciler.yaml" +URLS_FILE = "platform/keycloak/client-urls.yaml" SECRET_NAME = "keycloak-realm-clients" ADMIN_SECRET = "keycloak-admin-credentials" +URLS_CM_NAME = "keycloak-client-urls" KEY_SUFFIX = "-client-secret" SECRETS_MOUNT = "/etc/keycloak-realm-clients" +URLS_MOUNT = "/etc/keycloak-client-urls" +URLS_DATA_KEY = "client-urls.json" +RENDERER = "ci/render-keycloak-client-urls.py" +DIR_PATH = ROOT / "conformance" / "tests" / "l0-static" / "keycloak-client-secret-reconciler" +# The ONLY client fields the reconciler may write besides the secret. Kept in lockstep +# with the renderer and the script; asserted below to be exactly what they agree on. +URL_LIST_FIELDS = ("redirectUris", "webOrigins") +URL_STR_FIELDS = ("rootUrl", "baseUrl", "adminUrl") +URL_FIELDS = URL_LIST_FIELDS + URL_STR_FIELDS DOLLAR = chr(36) FAILURES: list[str] = [] @@ -446,6 +481,24 @@ if cj is not None: check(len(cm_vols) == 1, f"{RECON_FILE}: the CronJob does not mount the reconciler ConfigMap, so it " f"would run nothing") + # The URL desired state must be MOUNTED at the path the script defaults to, for the + # same reason as the Secret: the pod holds no ServiceAccount token and cannot read + # the KeycloakRealmImport CR, so an unmounted ConfigMap means pass 2 fails closed on + # every run forever -- correct behaviour, but a permanently red reconciler. + url_vols = [v for v in vols.values() + if (v.get("configMap") or {}).get("name") == URLS_CM_NAME] + check(len(url_vols) == 1, + f"{RECON_FILE}: the CronJob must mount the {URLS_CM_NAME} ConfigMap — that " + f"mount IS the desired state the URL pass reconciles towards, and the pod has " + f"no Kubernetes API access to read the realm import instead") + if url_vols: + mp = mounts.get(url_vols[0]["name"], {}).get("mountPath") + check(mp == URLS_MOUNT, + f"{RECON_FILE}: {URLS_CM_NAME} is mounted at {mp}, but the script defaults " + f"to {URLS_MOUNT}/{URLS_DATA_KEY} and no env var overrides it") + check(mounts.get(url_vols[0]["name"], {}).get("readOnly") is True, + f"{RECON_FILE}: the {URLS_CM_NAME} mount must be readOnly — the reconciler " + f"reads its desired state, it never writes it") env_srcs = {e["name"]: e for e in ctr.get("env", [])} for var in ("KEYCLOAK_ADMIN_USER", "KEYCLOAK_ADMIN_PASSWORD"): ref = (env_srcs.get(var, {}).get("valueFrom") or {}).get("secretKeyRef") or {} @@ -467,6 +520,110 @@ if cj is not None: check(re.search(r":[0-9]", img), f"{RECON_FILE}: container image {img!r} is not pinned to an explicit version") +# ================================================ (8) the URL desired state is DERIVED +# The reconciler's second pass makes the realm's environment-derived client URLs match +# platform/keycloak/client-urls.yaml. That file must not be an independent declaration of +# every public hostname: two declarations that can disagree is the same shape as the bug +# this whole change closes. It is GENERATED from the realm import, and the generation is +# gated HERE -- a hand edit to either file that makes them disagree is unmergeable. +urls_docs = [d for d in load(URLS_FILE) if d] +urls_cm = next((d for d in urls_docs if d.get("kind") == "ConfigMap"), None) +check(urls_cm is not None, f"{URLS_FILE}: no ConfigMap document found") +check((urls_cm or {}).get("metadata", {}).get("name") == URLS_CM_NAME, + f"{URLS_FILE}: the ConfigMap must be named {URLS_CM_NAME} (the CronJob mounts it " + f"by that name)") + +urls_want: dict[str, dict] = {} +if urls_cm is not None: + data = urls_cm.get("data") or {} + check(set(data) == {URLS_DATA_KEY}, + f"{URLS_FILE}: the ConfigMap must carry exactly the key {URLS_DATA_KEY} " + f"(found {sorted(data)}) — the script reads that filename from its mount") + raw = data.get(URLS_DATA_KEY, "") + try: + urls_want = json.loads(raw) + except ValueError as e: + check(False, f"{URLS_FILE}: {URLS_DATA_KEY} is not valid JSON ({e}) — the " + f"reconciler would fail closed on every run") + check(isinstance(urls_want, dict) and bool(urls_want), + f"{URLS_FILE}: {URLS_DATA_KEY} must be a non-empty JSON object of clients") + +# THE GATE: the committed file must be exactly what the renderer produces from the realm +# import. This is what makes the realm import the single source of truth for hostnames +# rather than merely the place they were first typed. +if (ROOT / RENDERER).is_file(): + proc = subprocess.run([sys.executable, str(ROOT / RENDERER), "--check"], + capture_output=True, text=True, timeout=120, cwd=str(ROOT)) + check(proc.returncode == 0, + f"{URLS_FILE} is NOT what {RENDERER} renders from {IMPORT_FILE}. The URL " + f"reconciler would converge the live realm on a desired state the realm import " + f"does not declare — two sources of truth for every public hostname, which is " + f"the failure class this change exists to close. Regenerate with: " + f"python3 {RENDERER} --write\n " + + "\n ".join((proc.stdout + proc.stderr).strip().splitlines()[:12])) +else: + check(False, f"{RENDERER} is missing — {URLS_FILE} would become a hand-maintained " + f"second declaration of every hostname with nothing keeping it honest") + +# Every client named in the desired state must exist in the realm import (otherwise every +# reconciler run reports NOT FOUND forever), and every field must be one the reconciler is +# actually allowed to write. +import_client_ids = {c.get("clientId") for c in clients} +for cid in sorted(urls_want): + check(cid in import_client_ids, + f"{URLS_FILE}: declares URLs for client '{cid}', which {IMPORT_FILE} does not " + f"declare. The reconciler would look it up, not find it, and fail every run") + entry = urls_want[cid] if isinstance(urls_want.get(cid), dict) else {} + check(bool(entry), f"{URLS_FILE}: client '{cid}' declares no URL fields") + for field in sorted(entry): + check(field in URL_FIELDS, + f"{URLS_FILE}: client '{cid}' declares field '{field}', which is not one of " + f"the writable URL fields ({', '.join(URL_FIELDS)}). Anything else would be " + f"a client-representation field written into the realm by a file edit") + +# The three places that name the writable field set must agree, or the gate is checking a +# different contract than the one that ships. +for field in URL_FIELDS: + check(field in script, + f"{RECON_FILE}: the shipped script never mentions the writable URL field " + f"'{field}', but this check and {RENDERER} both treat it as writable — the " + f"three definitions of the write surface have diverged") + +# A redirect URI is only as good as the hostname it points at. If the realm import uses a +# Flux substitution token, the desired-state ConfigMap must carry the SAME token (not a +# resolved value) and must be rendered by the SAME Kustomization, or the two are +# substituted differently and disagree about the domain at reconcile time. +# Scan the DATA PAYLOAD, not the whole file: both files carry header prose that discusses +# substitution tokens by name, and matching those would be a false positive. +TOKEN_RE = re.compile(re.escape(DOLLAR) + r"\{([A-Z_][A-Z0-9_]*)\}") +import_tokens = set(TOKEN_RE.findall( + yaml.safe_dump((realm_import or {}).get("spec", {}).get("realm", {})))) +urls_tokens = set(TOKEN_RE.findall( + ((urls_cm or {}).get("data") or {}).get(URLS_DATA_KEY, ""))) +check(urls_tokens <= import_tokens, + f"{URLS_FILE} uses substitution token(s) {sorted(urls_tokens - import_tokens)} that " + f"the realm definition in {IMPORT_FILE} does not — the desired state would resolve " + f"to a hostname the realm import never declared") + +urls_owners = owners(URLS_FILE) +check(bool(urls_owners), + f"{URLS_FILE} is rendered by NO Flux Kustomization — the reconciler would mount a " + f"ConfigMap that does not exist and fail closed on every run") +check(set(urls_owners) == set(import_owners), + f"{URLS_FILE} is rendered by {urls_owners} but {IMPORT_FILE} by {import_owners}. " + f"They MUST be rendered by the same Flux Kustomization: postBuild.substituteFrom is " + f"per-Kustomization, so a split would let the same " + f"{DOLLAR}{{BASE_DOMAIN}} resolve to two different values — the realm converging on " + f"a hostname the HTTPRoutes do not serve") +for env in ("dev", "prod"): + entry = f"clusters/{env}" + if not (ROOT / entry).is_dir(): + continue + applied, _ = st.reconciled_files(ROOT, entry) + check(URLS_FILE in applied, + f"{URLS_FILE} is not reachable from the {entry} entrypoint — the URL pass would " + f"fail closed on that cluster forever") + # ============================== (6) the reconciler's BEHAVIOUR, proven by its own selftest if script: with tempfile.TemporaryDirectory() as td: @@ -482,6 +639,35 @@ if script: if proc.returncode == 0: print(" reconciler --selftest: " + (tail[-1] if tail else "(no output)")) + # ---- (6b) THE SELFTEST IS FALSIFIABLE, not decorative ----------------------- + # A passing suite proves nothing unless it can be shown to FAIL. mutations.py + # injects plausible regressions one at a time -- order-sensitive comparison, a + # fat round-tripped PUT, additive-only URI sets, a widened allow-list, coupled + # passes, fail-open on an unreadable input -- and requires --selftest to catch + # every one. A mutation that SURVIVES means the suite does not check that + # property, and this check fails naming it. + # + # This matters more than usual here: the whole incident was a component that + # REPORTED SUCCESS while the realm was broken. A green test that cannot fail is + # the same lie one level up. ~20s, no network, no cluster. + mut = DIR_PATH / "mutations.py" + if mut.is_file(): + mproc = subprocess.run([sys.executable, str(mut), str(p)], + capture_output=True, text=True, timeout=600) + mtail = (mproc.stdout + mproc.stderr).strip().splitlines() + check(mproc.returncode == 0, + "the reconciler's --selftest is NOT FALSIFIABLE — at least one injected " + "regression survived it, so the suite does not prove what it claims:\n" + " " + "\n ".join(mtail[-14:])) + if mproc.returncode == 0: + print(" reconciler mutations: " + + (mtail[-1] if mtail else "(no output)")) + else: + check(False, + "conformance/tests/l0-static/keycloak-client-secret-reconciler/" + "mutations.py is missing — nothing proves the --selftest can fail, and " + "an unfalsifiable proof is the failure mode this component exists to end") + # ======================================================= (7) this check is load-bearing L0 = (ROOT / "ci" / "l0.sh").read_text() if (ROOT / "ci" / "l0.sh").is_file() else "" GATE = (ROOT / "ci" / "gate.sh").read_text() if (ROOT / "ci" / "gate.sh").is_file() else "" @@ -504,9 +690,36 @@ check(l2.is_file(), "keycloak-client-secrets-match-git) is missing — this static check cannot see a " "realm that drifted after apply, which is the whole failure mode") if l2.is_file(): - check("KEYCLOAK_CLIENT_SECRETS_AUDIT_OK" in l2.read_text(), - "the L2 test does not assert the reconciler's audit sentinel, so it could pass " - "on a Job that never ran the audit") + l2_text = l2.read_text() + check("KEYCLOAK_CLIENT_SECRETS_AUDIT_OK" in l2_text, + "the L2 test does not assert the reconciler's secret audit sentinel, so it could " + "pass on a Job that never ran the audit") + check("KEYCLOAK_CLIENT_URLS_AUDIT_OK" in l2_text, + "the L2 test does not assert the reconciler's URL audit sentinel. The live realm " + "could hold pre-migration redirect URIs and the test would still pass — which is " + "exactly the state prod was measured in") + check(URLS_CM_NAME in l2_text, + f"the L2 audit Job does not mount the {URLS_CM_NAME} ConfigMap, so its URL pass " + f"would fail closed and the test would report a false failure") + +# The alert pair must cover the reconciler's silence. Extended, not duplicated: one CronJob +# gets one Failing/Stale pair, because two pairs keyed to the same job_name regex would +# both fire for a single cause. +ALERTS = "platform/monitoring/alerts.yaml" +alerts_text = (ROOT / ALERTS).read_text() if (ROOT / ALERTS).is_file() else "" +for rule in ("KeycloakClientSecretReconcilerFailing", "KeycloakClientSecretReconcilerStale"): + check(rule in alerts_text, + f"{ALERTS} has no {rule} rule — the reconciler's silence would be unobservable, " + f"and a healer nobody notices is absent is the 'looks applied and isn't' failure") +check("redirect" in alerts_text.lower(), + f"{ALERTS}: the reconciler alert descriptions never mention redirect URIs, so an " + f"operator paged at 3am would look only for a client-secret problem while the actual " + f"drift was a hostname") + +# The renderer must be a real, runnable, tracked file — not a documented intention. +check((ROOT / RENDERER).is_file(), + f"{RENDERER} is missing; without it {URLS_FILE} cannot be regenerated and the " + f"single-source-of-truth gate above has nothing to compare against") # ------------------------------------------------------------------------------ report print(f"realm import: {len(clients)} client(s), {len(declared_pairs)} confidential with a " diff --git a/conformance/tests/l0-static/keycloak-client-secret-reconciler/mutations.py b/conformance/tests/l0-static/keycloak-client-secret-reconciler/mutations.py new file mode 100755 index 0000000..cbc99f3 --- /dev/null +++ b/conformance/tests/l0-static/keycloak-client-secret-reconciler/mutations.py @@ -0,0 +1,190 @@ +#!/usr/bin/env python3 +# ============================================================================= +# mutations.py -- prove the reconciler's --selftest IS FALSIFIABLE. +# +# WHY THIS EXISTS +# --------------- +# A selftest that passes proves nothing on its own: a selftest asserting `True == +# True` also passes. The reconciler exists because a one-shot import REPORTED +# SUCCESS while the realm was broken, and the same trap applies one level up -- a +# green test suite that cannot fail is the same lie in a different place. +# +# So this injects a series of PLAUSIBLE regressions into the shipped script, one at +# a time, and requires that --selftest FAILS for every one. A mutation that +# SURVIVES means the suite does not actually check that property, and the check +# fails, naming it. +# +# Each mutation is a mistake a reasonable person could make while editing this +# code -- not a nonsense edit. Several were live design questions while it was +# written (order-sensitivity, additive-vs-exact URI sets, whether the two passes +# should share a failure path); the mutation is the record that the answer is +# enforced and not merely commented. +# +# Invoked by check.sh with the script EXTRACTED from the shipped ConfigMap, so what +# is proven falsifiable is byte-for-byte what runs on the cluster. +# +# python3 mutations.py +# +# Runtime is ~20s (one subprocess per mutation, each running the full selftest +# against an in-process stub Keycloak; no network, no cluster). +# ============================================================================= +from __future__ import annotations + +import re +import subprocess +import sys +import tempfile +from pathlib import Path + + +def dedent4(text: str) -> str: + """Anchors are written with the YAML file's indentation. The script extracted from + the ConfigMap block scalar has 4 fewer leading spaces on every line, so anchors are + authored to match the YAML (easy to diff against the manifest) and dedented here.""" + return "\n".join(ln[4:] if ln.startswith(" ") else ln for ln in text.split("\n")) + + +# (name, the mistake being modelled, anchor, replacement) +MUTATIONS: list[tuple[str, str, str, str]] = [ + ("order-sensitive URL comparison", + "compare redirectUris in the order Keycloak returns them -- Keycloak does not " + "preserve the authored order, so a converged realm is rewritten on every run " + "forever while every run still reports success", + "if sorted(stored) != sorted(declared_value):", + "if stored != declared_value:"), + + ("fat round-tripped URL PUT", + "PUT the whole client representation back instead of only the drifted fields -- " + "the live server rejects it with 400 'Cannot parse the JSON', and even if it did " + "not, the blast radius would cover every field of the client", + " rep = {\"clientId\": client.get(\"clientId\")}\n" + " for field in sorted(drift):\n" + " rep[field] = want[cid][field]", + " rep = dict(client)\n" + " for field in sorted(drift):\n" + " rep[field] = want[cid][field]"), + + ("write unconditionally (compare-then-write removed)", + "drop the converged short-circuit -- every client is rewritten on every run, so " + "the CronJob becomes a permanent write load against the IdP database", + " drift = url_drift(client, want[cid])\n if not drift:", + " drift = url_drift(client, want[cid]) or dict(\n" + " (f, (\"\", want[cid][f])) for f in want[cid])\n if False:"), + + ("additive-only URI reconciliation", + "union the declared URIs with the stored ones -- a redirect URI REMOVED from git " + "is never removed from the realm, so a stale wildcard keeps working and quietly " + "widens what the realm will redirect to", + " rep[field] = want[cid][field]", + " rep[field] = (sorted(set(want[cid][field]) | set(client.get(field) or []))\n" + " if field in URL_LIST_FIELDS else want[cid][field])"), + + ("URL pass fails OPEN on an unreadable input", + "treat a missing ConfigMap as 'nothing declared' -- the run reports success while " + "reconciling nothing, which is precisely the fail-open of the realm import", + " except OSError as e:\n" + " url_input_fail = (\"cannot read %s (%s) -- the keycloak-client-urls ConfigMap \"\n" + " \"is not mounted\" % (URLS_FILE, e.__class__.__name__))", + " except OSError:\n want_urls = {}"), + + ("URL pass skips public clients", + "copy the secret pass's publicClient skip into the URL pass -- the `kubernetes` " + "client is PKCE-public, so kubectl's redirect URIs would never converge", + " drift = url_drift(client, want[cid])", + " if client.get(\"publicClient\"):\n continue\n" + " drift = url_drift(client, want[cid])"), + + ("the two passes are coupled", + "abort the whole run when either input is bad -- a typo in a hostname ConfigMap " + "would stop client-SECRET repair, making credential healing depend on a file that " + "has nothing to do with credentials", + " if want_secrets is None and want_urls is None:", + " if want_secrets is None or want_urls is None:"), + + ("unmanaged fields forced to empty", + "iterate every writable field instead of only the declared ones -- a rootUrl or " + "adminUrl the realm holds and git does not declare would be WIPED, turning " + "'not declared' into 'must be empty'", + " for field, declared_value in sorted(wanted.items()):", + " for field in URL_FIELDS:\n" + " declared_value = wanted.get(\n" + " field, [] if field in URL_LIST_FIELDS else \"\")"), + + ("no readback verification", + "trust the 204 -- a server that accepts the PUT and does not apply it is reported " + "as REPAIRED, which is 'reports success while the realm never converged' again", + " still = url_drift(after[0], want[cid])", + " still = {}"), + + ("allow-list widened to the whole realm", + "add a realm-level endpoint 'just in case' -- /users, /groups, /roles and " + "/authentication stop being structurally unreachable", + " r\"/admin/realms/[^/]+/clients\",", + " r\"/admin/realms/[^/]+\",\n r\"/admin/realms/[^/]+/clients\","), + + ("desired-state field validation removed", + "accept any field name from the ConfigMap -- an arbitrary client field becomes " + "writable through a file edit, bypassing the five-field write surface entirely", + " if field not in URL_FIELDS:", + " if False:"), + + ("URL drift detected but never repaired", + "leave the audit-mode early-continue in place on the write path -- drift is " + "reported in the log and silently not fixed", + " if audit:\n continue\n # MINIMAL representation", + " if True:\n continue\n # MINIMAL representation"), +] + + +def main() -> int: + if len(sys.argv) != 2: + print("usage: mutations.py ", file=sys.stderr) + return 2 + src = Path(sys.argv[1]).read_text() + + problems: list[str] = [] + killed = 0 + for name, why, old, new in MUTATIONS: + old, new = dedent4(old), dedent4(new) + # An anchor that no longer matches is a FAILURE, not a skip: it means the code + # moved and this mutation silently stopped proving anything. + n = src.count(old) + if n != 1: + problems.append("%s: anchor matches %d times (expected exactly 1) -- the " + "code moved and this mutation no longer proves anything. " + "Re-anchor it against the current source." % (name, n)) + continue + with tempfile.NamedTemporaryFile("w", suffix=".py", delete=False) as fh: + fh.write(src.replace(old, new, 1)) + path = fh.name + try: + proc = subprocess.run([sys.executable, path, "--selftest"], + capture_output=True, text=True, timeout=240) + finally: + Path(path).unlink(missing_ok=True) + if proc.returncode == 0: + problems.append("%s SURVIVED -- --selftest still passed with this regression " + "applied, so nothing in the suite actually checks it. The " + "mistake it models: %s" % (name, why)) + continue + killed += 1 + out = proc.stdout + proc.stderr + m = re.search(r"SELFTEST FAIL: (\d+) of \d+", out) + how = ("%s assertion(s) caught it" % m.group(1) if m + else "the selftest aborted (caught, less cleanly)") + print(" killed %-42s %s" % (name, how)) + + print() + if problems: + print("MUTATION CHECK FAIL: %d of %d mutation(s) not proven" + % (len(problems), len(MUTATIONS)), file=sys.stderr) + for p in problems: + print(" FAIL %s" % p, file=sys.stderr) + return 1 + print("MUTATION CHECK OK: all %d injected regressions were caught by --selftest " + "(the suite is falsifiable, not decorative)" % killed) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml b/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml index c384ca2..81656ca 100644 --- a/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml +++ b/conformance/tests/l2-behavioral/keycloak-client-secrets-match-git/chainsaw-test.yaml @@ -1,6 +1,6 @@ # ============================================================================= -# L2 BEHAVIORAL — every OIDC client's STORED secret equals the one git declares -# (R-IDP-02, R-IDP-04) [RUNNABLE] +# L2 BEHAVIORAL — every OIDC client's STORED secret AND redirect URIs equal what git +# declares (R-IDP-02, R-IDP-04) [RUNNABLE] # Spec: docs/15 identity-and-access; ADR-0014. # Impl: platform/keycloak/client-secret-reconciler.yaml (the CronJob + its script). # @@ -27,14 +27,26 @@ # its endpoint allow-list makes users/groups/roles unreachable), # -> THEN every declared client's stored secret matches git, and none holds an # unresolved import placeholder. Emits KEYCLOAK_CLIENT_SECRETS_AUDIT_OK. +# -> AND every declared client URL — redirectUris, webOrigins, rootUrl, baseUrl, +# adminUrl — matches git. Emits KEYCLOAK_CLIENT_URLS_AUDIT_OK. +# +# THE URL HALF EXISTS FOR THE SAME REASON, ONE FIELD OVER. The realm import is templated +# per environment, so moving prod's BASE_DOMAIN re-renders every declared redirect URI 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 would fail with a redirect_uri mismatch, and once again every +# signal would be green: the CR reads Done=True, the pods are healthy, and the +# secret-only version of this very test would have PASSED. Asserting only the secret +# sentinel is what would have let that ship. # # WHY IT RUNS THE SHIPPED SCRIPT rather than re-implementing the comparison: a detector # that duplicates the healer's logic drifts from it. This audits with the same code, in # the same mode the L0 gate selftests, so a passing audit is evidence about the healer # too. It runs as a Job IN the keycloak namespace (not in the ephemeral chainsaw -# namespace) because the desired state is the mounted keycloak-realm-clients Secret and -# the admin credential is keycloak-admin-credentials — both namespace-local, which is -# also why neither this Job nor the CronJob needs any Kubernetes API access. +# namespace) because the desired state is the mounted keycloak-realm-clients Secret plus +# the keycloak-client-urls ConfigMap, and the admin credential is +# keycloak-admin-credentials — all namespace-local, which is also why neither this Job nor +# the CronJob needs any Kubernetes API access. # # chainsaw v0.2.12: Job + status assert; the leftover-Job delete keeps a re-run from # colliding with an immutable Job left by a previous failure. @@ -149,6 +161,12 @@ spec: - name: client-secrets mountPath: /etc/keycloak-realm-clients readOnly: true + # The URL pass's desired state. Without this mount the audit + # fails closed on pass 2 and this test would report a false + # failure rather than auditing the realm's redirect URIs. + - name: client-urls + mountPath: /etc/keycloak-client-urls + readOnly: true securityContext: allowPrivilegeEscalation: false readOnlyRootFilesystem: true @@ -170,6 +188,10 @@ spec: secret: secretName: keycloak-realm-clients defaultMode: 0400 + - name: client-urls + configMap: + name: keycloak-client-urls + defaultMode: 0444 - assert: resource: apiVersion: batch/v1 @@ -192,6 +214,14 @@ spec: || { echo "FAIL: the audit did not report every client secret matching git"; exit 1; } echo "$out" | grep -q 'UNRESOLVED IMPORT PLACEHOLDER' \ && { echo "FAIL: a client still holds the literal import placeholder"; exit 1; } + # The URL half. A realm still holding PRE-migration redirect URIs is the + # state prod was measured in: every OIDC login fails with a redirect_uri + # mismatch while the KeycloakRealmImport CR still reads Done=True. Asserting + # only the secret sentinel would pass on exactly that realm. + echo "$out" | grep -q 'KEYCLOAK_CLIENT_URLS_AUDIT_OK' \ + || { echo "FAIL: the audit did not report every client URL matching git"; exit 1; } + echo "$out" | grep -q 'DRIFT -- realm has' \ + && { echo "FAIL: a client URL still differs from the declared one"; exit 1; } echo KEYCLOAK_CLIENT_SECRET_DRIFT_NONE_OK check: (contains($stdout, 'KEYCLOAK_CLIENT_SECRET_DRIFT_NONE_OK')): true diff --git a/platform/monitoring/alerts.yaml b/platform/monitoring/alerts.yaml index 5260b8e..daa1eb5 100644 --- a/platform/monitoring/alerts.yaml +++ b/platform/monitoring/alerts.yaml @@ -431,7 +431,7 @@ spec: summary: "Keycloak database unavailable" description: "Keycloak's PostgreSQL is not ready for 5m — the IdP will fail (docs/15)." runbook_url: "https://${BASE_DOMAIN}/runbooks/keycloak-db" - # --- The client-secret reconciler (platform/keycloak/client-secret-reconciler.yaml) --- + # --- The client reconciler (platform/keycloak/client-secret-reconciler.yaml) --- # WHY THESE EXIST: on 2026-09-06 every OIDC login on prod failed because the realm import # had stored the LITERAL `(env:...)` placeholder as all 12 client secrets — and every # signal the cluster offered was GREEN (the import Job succeeded, the CR read Done=True, @@ -440,6 +440,14 @@ spec: # the healer; these two rules make its SILENCE observable, so the class cannot sit # undetected again. They are the runtime counterpart of the L2 audit test. # + # THE CronJob NOW RECONCILES TWO THINGS — client secrets, and the environment-derived + # client URLs (redirectUris, webOrigins, rootUrl, baseUrl, adminUrl) that a BASE_DOMAIN + # migration re-renders in git and the one-shot import never carries to the realm. The + # pair below is EXTENDED to say so rather than DUPLICATED: one CronJob gets one + # Failing/Stale pair, because 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 Job log names which + # pass failed, which client, and for a URL the exact before/after. + # # UNVERIFIED-ON-CLUSTER at authoring time: no CronJob existed on either cluster yet, so # these series had no samples to check against. `kube_job_status_failed` / # `kube_job_status_succeeded` and the `job_name` label WERE confirmed present (the realm @@ -457,12 +465,15 @@ spec: labels: severity: warning annotations: - summary: "Keycloak client-secret reconciler failing" + summary: "Keycloak client reconciler failing" description: >- - A keycloak-client-secret-reconciler Job has been failing for 15m — an OIDC client - secret may differ from git (unresolved import placeholder, or an unrepairable - drift) and SSO logins for that client will fail. Read the Job log: it names the - client and never prints a secret. + A keycloak-client-secret-reconciler Job has been failing for 15m — an OIDC client's + stored SECRET or its REDIRECT URIs may differ from git (an unresolved import + placeholder, a pre-migration hostname the one-shot realm import never updated, or + an unrepairable drift), and SSO logins for that client will fail — a bad + redirectUri fails with a redirect_uri mismatch at the IdP. Read the Job log: it + names the pass, the client and the exact URL before/after, and never prints a + secret. runbook_url: "https://${BASE_DOMAIN}/runbooks/keycloak-client-secrets" - alert: KeycloakClientSecretReconcilerStale # No SUCCESSFUL run in scope. Catches the failure modes the rule above cannot see: the @@ -475,11 +486,12 @@ spec: labels: severity: warning annotations: - summary: "Keycloak client-secret reconciler has not succeeded" + summary: "Keycloak client reconciler has not succeeded" description: >- No successful keycloak-client-secret-reconciler run in 2h (schedule is every 15m). - The CronJob may be suspended, unapplied, or unable to schedule — client-secret - drift would go uncorrected and unnoticed (docs/15). + The CronJob may be suspended, unapplied, or unable to schedule — client-secret AND + redirect-URI drift would go uncorrected and unnoticed (docs/15). This is the rule + that would catch the healer being quietly absent during a domain migration. runbook_url: "https://${BASE_DOMAIN}/runbooks/keycloak-client-secrets" # ===================================================================================== From f7d4648851a6045f15588c1892728c392c5cb4fb Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Sat, 12 Sep 2026 21:17:15 -0500 Subject: [PATCH 4/4] 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