From a40ca4f303ab208368981feb8773751475a7f204 Mon Sep 17 00:00:00 2001 From: Smana Date: Thu, 1 Oct 2026 06:05:22 +0200 Subject: [PATCH] fix(zitadel): restart env-at-start OIDC consumers after a client rotation After an OpenBao rebuild the sync rotates the ZITADEL OIDC client (#2078), but a Deployment that read its client id from env at start keeps the dead one: headlamp-oauth2-proxy answered "App not found" until restarted by hand. cmd_sync now records each store key whose stored client id it replaced and, with kubectl on --cluster, force-syncs the ExternalSecrets reading it, waits until the Secret carries the new id, then rollout-restarts the Deployments referencing that Secret through env or envFrom. A first bootstrap or a re-run with nothing rotated restarts nothing. --- .../test-zitadel-oidc-clients-convergence.sh | 12 ++ .../test-zitadel-oidc-clients-openbao.sh | 3 +- .../test-zitadel-oidc-clients-restart.sh | 111 ++++++++++++++++++ scripts/provision/zitadel-oidc-clients.sh | 108 ++++++++++++++++- website/content/docs/get-started/sso.md | 3 + 5 files changed, 235 insertions(+), 2 deletions(-) create mode 100755 scripts/ci/tests/test-zitadel-oidc-clients-restart.sh diff --git a/scripts/ci/tests/test-zitadel-oidc-clients-convergence.sh b/scripts/ci/tests/test-zitadel-oidc-clients-convergence.sh index bc9c6ab7c..dc786f8c9 100755 --- a/scripts/ci/tests/test-zitadel-oidc-clients-convergence.sh +++ b/scripts/ci/tests/test-zitadel-oidc-clients-convergence.sh @@ -121,6 +121,9 @@ load_function store_write_and_mirror "$SRC" load_function publish_project_id "$SRC" # a no-op here: CLOUD=aws mirror_to_openbao() { cat >/dev/null; } force_sync_mirrored() { :; } # its own suite; no kubectl here +load_function stored_client_id "$SRC" +# Its kubectl side has its own suite; here only WHICH keys reach it matters. +restart_rotated_consumers() { printf '%s\n' "$@" > "$STORE_DIR/.rotated"; } # ── globals cmd_sync / converge_secret read ───────────────────────────────── CLUSTER="aws-0" @@ -166,6 +169,7 @@ after="$(store_read headlamp-envvars)" check "converge: OIDC_SCOPES gains groups" "profile,email,groups" "$(jq -r '.OIDC_SCOPES' <<< "$after")" check "converge: client secret untouched" "do-not-touch-me" "$(jq -r '.OIDC_CLIENT_SECRET' <<< "$after")" check "converge: client id still correct" "existing-client-id" "$(jq -r '.OIDC_CLIENT_ID' <<< "$after")" +check "converge: same client id, nothing restarted" "" "$(cat "$STORE_DIR/.rotated" 2>/dev/null)" case "$out" in *"[converged] headlamp"*) printf ' ok converge: reported in the run output\n' ;; @@ -216,4 +220,12 @@ case "$(cat "$OUT_FILE")" in *) printf ' FAIL secret truly absent: the restore-or-recreate advice is gone\n'; fail=1 ;; esac +# ── the store holds a client ZITADEL no longer knows: a rotation ────────── +jq -c '.OIDC_CLIENT_ID = "dead-client-id"' <<< "$after" > "$STORE_DIR/headlamp-envvars" +rm -f "$STORE_DIR/.rotated" +( set -o errexit -o nounset -o pipefail; cmd_sync ) > "$OUT_FILE" 2>&1 +check "rotation: cmd_sync exits 0 under errexit" "0" "$?" +check "rotation: the key and its new client id reach the restart" \ + "headlamp-envvars=existing-client-id" "$(cat "$STORE_DIR/.rotated" 2>/dev/null)" + exit "$fail" diff --git a/scripts/ci/tests/test-zitadel-oidc-clients-openbao.sh b/scripts/ci/tests/test-zitadel-oidc-clients-openbao.sh index 39d3fac67..6c52cb380 100755 --- a/scripts/ci/tests/test-zitadel-oidc-clients-openbao.sh +++ b/scripts/ci/tests/test-zitadel-oidc-clients-openbao.sh @@ -1049,13 +1049,14 @@ contains "$out" "[FAILED ]" "writes that do not stick: says [FAILED echo echo "== cmd_sync wiring: the reconcile runs once, after the loop (Task 3) ==" -for f in cmd_sync oidc_config_payload store_write_and_mirror publish_project_id; do +for f in cmd_sync oidc_config_payload store_write_and_mirror publish_project_id stored_client_id; do body="$(sed -n "/^${f}() {/,/^}/p" "$CONSUMERS_SRC")" [ -n "$body" ] || { echo " FAIL could not extract ${f}() from $CONSUMERS_SRC" >&2; fail=1; } eval "$body" done # Tested in test-zitadel-oidc-clients-mirror.sh; stubbed so no kubectl runs. force_sync_mirrored() { :; } +restart_rotated_consumers() { :; } # test-zitadel-oidc-clients-restart.sh # Every OTHER thing cmd_sync calls, stubbed: this section is about the ONE new # call, not a restatement of the redirect/convergence suites' own coverage. diff --git a/scripts/ci/tests/test-zitadel-oidc-clients-restart.sh b/scripts/ci/tests/test-zitadel-oidc-clients-restart.sh new file mode 100755 index 000000000..530d3dc0e --- /dev/null +++ b/scripts/ci/tests/test-zitadel-oidc-clients-restart.sh @@ -0,0 +1,111 @@ +#!/usr/bin/env bash +# shellcheck disable=SC2034 +# (APPLY, CLUSTER and the OIDC_CONSUMER_* knobs are read by function bodies +# eval'd out of the script, which static analysis cannot see.) +# +# After a rotation, the Deployments that read a client id from env at start must +# restart, or they keep the dead directory's client ("App not found" from +# headlamp-oauth2-proxy). Restarts only the readers of a rotated key's Secret, +# only once that Secret carries the new id, and only with kubectl on $CLUSTER. +# The functions are lifted out of the script, so this tests the code that ships. +set -uo pipefail +REPO_ROOT="$(cd "$(dirname "$0")/../../.." && pwd)" +cd "$REPO_ROOT" || exit 1 +S="${ZITADEL_OIDC_CLIENTS_SCRIPT:-scripts/provision/zitadel-oidc-clients.sh}" +fail=0 +ok() { printf ' ok %s\n' "$1"; } +bad() { printf ' FAIL %s\n' "$1"; fail=1; } + +for f in stored_client_id restart_rotated_consumers; do + body="$(sed -n "/^${f}() {/,/^}/p" "$S")" + [ -n "$body" ] || { echo "could not extract ${f}() from $S" >&2; exit 1; } + eval "$body" +done +# shellcheck source=scripts/lib/bao-map.sh +. "$REPO_ROOT/scripts/lib/bao-map.sh" + +T="$(mktemp -d)"; trap 'rm -rf "$T"' EXIT +sleep() { :; } + +# Two ExternalSecrets: one from the managed store under the bare key (unmapped), +# one from OpenBao at the key's mapped path. +cat >"$T/es.json" <<'EOF' +{"items":[ + {"metadata":{"namespace":"tooling","name":"headlamp-oauth2-proxy"}, + "spec":{"secretStoreRef":{"name":"clustersecretstore"}, + "dataFrom":[{"extract":{"key":"headlamp-oauth2-proxy"}}], + "target":{"name":"headlamp-oauth2-proxy"}}}, + {"metadata":{"namespace":"observability","name":"grafana-es"}, + "spec":{"secretStoreRef":{"name":"openbao-platform"}, + "dataFrom":[{"extract":{"key":"victoria-metrics/grafana-envvars"}}], + "target":{"name":"grafana-envvars"}}} +]} +EOF +# Readers by env and by envFrom, a bystander, and a volume mount (not env). +cat >"$T/deploy.json" <<'EOF' +{"items":[ + {"metadata":{"name":"headlamp-oauth2-proxy"},"spec":{"template":{"spec":{"containers":[ + {"env":[{"name":"OAUTH2_PROXY_CLIENT_ID","valueFrom":{"secretKeyRef":{"name":"headlamp-oauth2-proxy","key":"client-id"}}}]}]}}}}, + {"metadata":{"name":"grafana"},"spec":{"template":{"spec":{"containers":[ + {"envFrom":[{"secretRef":{"name":"grafana-envvars"}}]}]}}}}, + {"metadata":{"name":"headlamp"},"spec":{"template":{"spec":{"containers":[ + {"envFrom":[{"secretRef":{"name":"headlamp-envvars"}}]}]}}}}, + {"metadata":{"name":"mounts-it"},"spec":{"template":{"spec":{"containers":[{}]}}}} +]} +EOF +jq '.items[3].spec.template.spec.volumes = [{secret: {secretName: "headlamp-oauth2-proxy"}}]' "$T/deploy.json" > "$T/d.json" && mv "$T/d.json" "$T/deploy.json" # pragma: allowlist secret + +CM="gke-gcp-0-vars" +SECRET_ID="new-id" +kubectl() { + printf '%s\n' "$*" >>"$T/calls" + case "$1 $2" in + "get configmap") printf 'configmap/%s\n' "$CM" ;; + "get externalsecrets") cat "$T/es.json" ;; + "get secret") jq -n --arg v "$(printf '%s' "$SECRET_ID" | base64)" '{data:{"client-id":$v,GF_AUTH_GENERIC_OAUTH_CLIENT_ID:$v}}' ;; + "get deployments") cat "$T/deploy.json" ;; + *) : ;; + esac +} +run() { : >"$T/calls"; restart_rotated_consumers "$@" >"$T/out" 2>&1; } +restarts() { grep '^rollout restart' "$T/calls" | sort | tr '\n' ';'; } + +APPLY=true CLUSTER=gcp-0 +OIDC_CONSUMER_WAIT_SECONDS=60 + +# stored_client_id reads every consumer's field name. +for blob in '{"GF_AUTH_GENERIC_OAUTH_CLIENT_ID":"a"}' '{"OIDC_CLIENT_ID":"a"}' '{"clientID":"a"}' \ + '{"client_id":"a"}' '{"client-id":"a","client-secret":"s"}'; do + [ "$(stored_client_id <<< "$blob")" = a ] && ok "stored_client_id: ${blob%%:*}}" \ + || bad "stored_client_id on $blob" +done +[ -z "$(stored_client_id <<< '{"other":"x"}')" ] && ok "stored_client_id: empty when none" || bad "stored_client_id invented an id" + +run 'headlamp-oauth2-proxy=new-id' +[ "$(restarts)" = "rollout restart deployment headlamp-oauth2-proxy -n tooling;" ] \ + && ok "unmapped key: only its env reader restarts" || bad "unmapped key restarts: $(restarts)" +grep -q '^annotate externalsecret headlamp-oauth2-proxy -n tooling force-sync=' "$T/calls" \ + && ok "its ExternalSecret is force-synced first" || bad "no force-sync: $(cat "$T/calls")" + +run 'observability-victoria-metrics-k8s-stack-grafana-envvars=new-id' +grep -q '^rollout restart deployment grafana -n observability$' "$T/calls" \ + && ok "mapped key: the envFrom reader restarts" || bad "mapped key restarts: $(restarts)" + +run +[ ! -s "$T/calls" ] && ok "nothing rotated: no kubectl call" || bad "called kubectl with nothing rotated: $(cat "$T/calls")" + +APPLY=false run 'headlamp-oauth2-proxy=new-id' +[ ! -s "$T/calls" ] && ok "dry run: no kubectl call" || bad "dry run called kubectl: $(cat "$T/calls")" + +CM="eks-aws-0-vars" run 'headlamp-oauth2-proxy=new-id' +[ -z "$(restarts)" ] && grep -q 'does not point at gcp-0' "$T/out" \ + && ok "kubectl on another cluster: warns, restarts nothing" || bad "wrong context: $(restarts) $(cat "$T/out")" + +SECRET_ID="old-id" OIDC_CONSUMER_WAIT_SECONDS=0 run 'headlamp-oauth2-proxy=new-id' +[ -z "$(restarts)" ] && grep -q 'does not carry client new-id' "$T/out" \ + && ok "Secret not refreshed: no restart onto the old id" || bad "stale Secret: $(restarts) $(cat "$T/out")" + +grep -A1 '^ force_sync_mirrored "' "$S" | grep -q 'restart_rotated_consumers' \ + && ok "cmd_sync restarts right after the force-sync" || bad "restart_rotated_consumers not wired after force_sync_mirrored" + +exit "$fail" diff --git a/scripts/provision/zitadel-oidc-clients.sh b/scripts/provision/zitadel-oidc-clients.sh index fe3ce7a78..c3217098d 100755 --- a/scripts/provision/zitadel-oidc-clients.sh +++ b/scripts/provision/zitadel-oidc-clients.sh @@ -30,6 +30,8 @@ # a seed that predates the app, so its id is new every time and # Terraform, which created the mount, ignores this field afterwards. # No-op when --openbao-url is empty, which is every consumer call. +# 6. Restarts the Deployments that read a REPLACED client id from env, once +# their Secret carries the new one (restart_rotated_consumers). # # Step 4 MERGES rather than overwrites where a secret holds more than OIDC: # grafana-envvars also carries the generated admin credentials, and clobbering @@ -992,6 +994,95 @@ force_sync_mirrored() { done || echo "WARN: could not match ExternalSecrets to the mirrored paths" >&2 } +# The client id a stored consumer payload carries, whichever field its consumer +# names it by (merge_secret). The payload arrives on stdin: it holds the secret. +stored_client_id() { + jq -r 'first(.GF_AUTH_GENERIC_OAUTH_CLIENT_ID, .OIDC_CLIENT_ID, .clientID, + .client_id, ."client-id" | strings) // empty' 2>/dev/null || true +} + +# Restart the Deployments that read a ROTATED client from env. They resolve it +# once, at start, so a refreshed Secret is not enough: after an OpenBao rebuild +# headlamp-oauth2-proxy kept the dead directory's client id and answered "App +# not found" until restarted by hand, while every resource reported healthy. +# +# Arguments are `=`, only for keys whose stored client +# id this run changed, so a re-run with nothing rotated restarts nothing. Warn-only +# like force_sync_mirrored: the clients are already written. +# +# kubectl must point at $CLUSTER. aws/eks/init's consuming sync for gcp-0 runs +# with kubectl on aws-0, and restarting there would restart the wrong cluster. +# +# The Secret has to carry the new id before the restart, or the new pods read +# the old one again: force-sync each ExternalSecret reading the key, then wait +# (one deadline for all of them) until its Secret does. +restart_rotated_consumers() { + [ "$APPLY" = "true" ] && [ $# -gt 0 ] || return 0 + local pair store_name id target es_json ns es obj deploys d deadline now + local matches=() restarted=() + if ! kubectl get configmap -n flux-system -o name 2>/dev/null | grep -Eq -- "-${CLUSTER}-vars\$"; then + echo "WARN: kubectl does not point at ${CLUSTER}. Rotated: ${*%%=*}." >&2 + echo " Restart the Deployments reading those keys there, once their Secrets refresh." >&2 + return 0 + fi + if ! es_json="$(kubectl get externalsecrets -A -o json 2>/dev/null)"; then + echo "WARN: could not list ExternalSecrets; restart the consumers of ${*%%=*} by hand" >&2 + return 0 + fi + now="$(date +%s)" + for pair in "$@"; do + store_name="${pair%%=*}"; id="${pair#*=}" + target="$(bao_target_for "$store_name")" || target="" + # Read from OpenBao at the key's mapped path, or from the managed store + # under the key itself (unmapped keys such as headlamp-oauth2-proxy). + while read -r ns es obj; do + [ -n "$obj" ] || continue + kubectl annotate externalsecret "$es" -n "$ns" force-sync="$now" --overwrite >/dev/null 2>&1 \ + || echo "WARN: could not force-sync externalsecret ${ns}/${es}" >&2 + matches+=("${ns} ${obj} ${id}") + done < <(jq -r --arg name "$store_name" --arg target "$target" '.items[] + | (.spec.secretStoreRef.name // "") as $store + | ([(.spec.data // [])[].remoteRef.key?, (.spec.dataFrom // [])[].extract.key?] + | map(select(. != null))) as $keys + | select((($store | startswith("openbao-")) and $target != "" + and ($keys | map(($store | ltrimstr("openbao-")) + "/" + .) | index($target)) != null) + or ($store == "clustersecretstore" and ($keys | index($name)) != null)) + | "\(.metadata.namespace) \(.metadata.name) \(.spec.target.name // .metadata.name)"' <<< "$es_json") + done + [ "${#matches[@]}" -gt 0 ] || { echo "[ok ] no ExternalSecret reads a rotated key; nothing to restart"; return 0; } + + deadline=$(( $(date +%s) + ${OIDC_CONSUMER_WAIT_SECONDS:-180} )) + for pair in "${matches[@]}"; do + read -r ns obj id <<< "$pair" + until kubectl get secret "$obj" -n "$ns" -o json 2>/dev/null \ + | jq -e --arg id "$id" '.data // {} | any(.[]; @base64d == $id)' >/dev/null 2>&1; do + if [ "$(date +%s)" -ge "$deadline" ]; then + echo "WARN: ${ns}/${obj} does not carry client ${id} yet; its consumers keep the old one." >&2 + echo " Once it does: kubectl rollout restart deployment -n ${ns} " >&2 + continue 2 + fi + sleep "${OIDC_CONSUMER_POLL_SECONDS:-5}" + done + if ! deploys="$(kubectl get deployments -n "$ns" -o json 2>/dev/null | jq -r --arg s "$obj" '.items[] + | select([.spec.template.spec | (.containers // []) + (.initContainers // []) | .[] + | (.envFrom // [])[].secretRef.name?, (.env // [])[].valueFrom.secretKeyRef.name?] + | index($s) != null) + | .metadata.name')"; then + echo "WARN: could not list Deployments in ${ns}; restart the readers of ${obj} by hand" >&2 + continue + fi + for d in $deploys; do + [[ " ${restarted[*]} " == *" ${ns}/${d} "* ]] && continue + if kubectl rollout restart deployment "$d" -n "$ns" >/dev/null 2>&1; then + echo "[restart] deployment ${ns}/${d} -- ${obj} carries rotated client ${id}" + restarted+=("${ns}/${d}") + else + echo "WARN: could not restart deployment ${ns}/${d}; it keeps the old client id" >&2 + fi + done + done +} + # GCP hosting: publish this directory's project id for gke/configure, which # reads it at plan time (GCP parity GP-4). A fresh directory gets a new id # every build, and the committed one would otherwise come back on the next @@ -1213,6 +1304,9 @@ cmd_sync() { local failed_mirrors="" wrc=0 # Keys mirrored without error, force-synced after the loop. local mirrored_keys=() + # `=` for each key whose stored client id this run replaced: + # the only consumers restart_rotated_consumers restarts. + local rotated=() prev_id # Fed to reconcile_openbao_oidc after the loop -- see the consumer's own # branches below for where each is set. Empty stays empty on a dry run # (reconcile_openbao_oidc treats that as its own skip) and on any topology @@ -1378,6 +1472,7 @@ cmd_sync() { echo "[dry-run] ${name} -- would converge non-secret fields in ${key}" converged=$((converged + 1)) else + prev_id="$(stored_client_id <<< "$existing_secret")" wrc=0 printf '%s' "$desired" | store_write_and_mirror "$key" || wrc=$? case "$wrc" in @@ -1385,6 +1480,7 @@ cmd_sync() { 2) failed_mirrors="${failed_mirrors}${failed_mirrors:+ }${key}" ;; *) exit 1 ;; esac + [ -z "$prev_id" ] || [ "$prev_id" = "$client_id" ] || rotated+=("${key}=${client_id}") echo "[converged] ${name} -> ${key} (client id ${client_id}, secret untouched)" converged=$((converged + 1)) fi @@ -1418,6 +1514,14 @@ cmd_sync() { # store an empty payload. local merged merged="$(merge_secret "$key" "$consumer" "$client_id" "$client_secret")" || exit 1 + # A key that held a client before is a rotation (a fresh directory, or a + # restore older than the app); one that held none is a first bootstrap, + # whose consumers are still waiting for the Secret and start on their own. + # Never fatal: ZITADEL has issued the secret, and only the write below keeps it. + prev_id="" + if store_exists "$key"; then + prev_id="$(store_read "$key" | stored_client_id)" || prev_id="" + fi wrc=0 printf '%s' "$merged" | store_write_and_mirror "$key" || wrc=$? case "$wrc" in @@ -1425,6 +1529,7 @@ cmd_sync() { 2) failed_mirrors="${failed_mirrors}${failed_mirrors:+ }${key}" ;; *) exit 1 ;; esac + [ -z "$prev_id" ] || [ "$prev_id" = "$client_id" ] || rotated+=("${key}=${client_id}") echo "[created] ${name} -> ${key} (client ${client_id})" created=$((created + 1)) done @@ -1447,6 +1552,7 @@ cmd_sync() { local openbao_failed=0 reconcile_openbao_oidc "$openbao_key" "$openbao_client_id" || openbao_failed=1 force_sync_mirrored "${mirrored_keys[@]}" + restart_rotated_consumers ${rotated[@]+"${rotated[@]}"} echo echo "created: ${created}, updated: ${updated}, unchanged: ${skipped}, converged: ${converged}" @@ -1469,7 +1575,7 @@ cmd_sync() { case "$COMMAND" in sync) cmd_sync ;; *) - sed -n '2,40p' "$0" | sed 's/^# \{0,1\}//' + sed -n '2,44p' "$0" | sed 's/^# \{0,1\}//' exit 2 ;; esac diff --git a/website/content/docs/get-started/sso.md b/website/content/docs/get-started/sso.md index 3b621d543..fea6d9145 100644 --- a/website/content/docs/get-started/sso.md +++ b/website/content/docs/get-started/sso.md @@ -74,6 +74,9 @@ Every step is idempotent — re-running prints `[skip …]` and changes nothing. # groups action, so every consumer authenticates and then has no groups. # On a hosting cluster, $OPENBAO points OpenBao at whatever client ZITADEL # now holds; leaving it out strands OpenBao on a client ZITADEL forgot. +# A client id that REPLACES one the cluster held also restarts the +# Deployments reading it from env (oauth2-proxy, Headlamp, Grafana), once +# their Secret carries it. Only with kubectl on --cluster; otherwise it warns. ./scripts/provision/zitadel-oidc-clients.sh sync $CL $OPENBAO --apply ```