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