fix(sso): gitea + nextcloud OIDC bootstrap was DEAD CODE — envsubst ate the guard
Both ServiceDefinitions guarded their Keycloak OIDC registration with an UNESCAPED
braced expansion:
if [ -z "${GITEA_OIDC_CLIENT_SECRET:-}" ]; then # gitea
if [ -z "${NEXTCLOUD_OIDC_CLIENT_SECRET:-}" ]; then # nextcloud, then exit 0
apps-wave-0/1 both declare postBuild.substituteFrom: cluster-vars, and neither name is a
cluster-vars key — so Flux's envsubst substituted them away AT RECONCILE TIME, long
before the container ever ran. Verified in the LIVE cluster: the deployed Deployments
literally contain
if [ -z "" ]; then
which is always true. The entire OIDC registration block has been skipped on EVERY boot
of both apps.
WHY NOBODY NOTICED: both apps DO have the provider registered — `gitea admin auth list`
shows keycloak/OAuth2/true and `occ user_oidc:provider` shows the keycloak provider.
Those live in each app's own database, persisted from an earlier successful registration.
SSO works today and DECISIONS-LOG is not wrong. But the idempotent re-registration is
dead, so any PVC rebuild, DB restore, or fresh cluster build comes up with NO OIDC and
only the break-glass local admin. A PROD BUILD FROM THIS REPO WOULD SILENTLY HAVE NO SSO
on gitea or nextcloud.
The irony: the surrounding code was hardened twice — see its own comments about a "silent
first-boot RACE", the verify-then-retry loops, the marker files. All of that hardening was
unreachable, because the guard above it always evaluated true.
Not a regression from the recent R-ENV-02 tokenization — introduced 2026-08-27 (8738871).
FIXED AT THE RIGHT SEAM: apps/wave-0/gitea.yaml and apps/wave-1/nextcloud.yaml are
RENDER-GENERATED, so the fix is in catalog/*.yaml and the goldens are regenerated.
Escaped to $${...}, the convention documented in clusters/base/apps-vms.yaml. Post-envsubst
the container now receives the intended `${VAR:-}` instead of `""`.
STRICT MODE WOULD NEVER HAVE CAUGHT THIS. Flux deliberately exempts default-providing
operators (-, :-, +, :+, =, :=, ?, :?) from StrictPostBuildSubstitutions, because they
"intentionally handle the unset case". So a targeted check was required.
MADE UN-REPEATABLE IN THE RENDERER: catalog/render.py gains a substitution-safety section
wired into emit(), so every render path refuses to emit a manifest containing a braced
expansion that is not exactly ${KNOWN_KEY} for a key read LIVE from cluster-vars (no
hardcoded copy to drift; unreadable => empty set => fail-closed). Dollar-runs are counted
properly ($$ is a literal $ to envsubst, so odd runs are flagged and even runs are not),
and indexed forms get their own hard-error message. Nine new --selftest assertions,
including the exact regression. Proven to bite: rendering the pre-fix catalog files from
HEAD now exits 1 with the precise diagnosis.
Sweep: the complete substituted surface was enumerated from every Kustomization's path.
catalog/ contains exactly TWO braced expansions and both were these bugs; every other
braced expansion on a substituted path is a declared cluster-vars key (zero typos, zero
undeclared names); existing $${UOS_URL} escapes verified intact.
Verified: --selftest PASS; canonical render is byte-identical to the committed apps/ for
all 10 onboarded services; the three L1 render checks pass.
NEXT RECONCILE: both pod templates change, so gitea (wave 0) and nextcloud (wave 1) roll —
strategy: Recreate on RWO Longhorn PVCs, so expect a brief single-pod outage each. The
OIDC block then executes for the first time in these apps' deployed lifetime. Both paths
are idempotent (gitea takes auth update-oauth on the existing id; nextcloud's
user_oidc:provider is create-or-update), so the expected result is a no-op re-write plus
the verify loop logging success. If the SOPS client secret ever diverged from Keycloak's,
this is when it gets corrected instead of silently left stale. Tail both pods after merge.
Serves: R-IDP-02, R-IDP-06, R-ENV-02, R-GITOPS-02.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
jakman85andClaude Opus 5 committed
1 parent
b8602c939c
commit
3484955903
6 files changed
+258
-10
No files matched your search
+217
-4
@@ -29,6 +29,7 @@ conformance harness stays stdlib-only and only shells out to this script + diffs
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import re
|
||||
import sys
|
||||
from pathlib import Path
|
||||
|
||||
@@ -936,6 +937,124 @@ def r_virtualmachine(s: dict) -> dict:
|
||||
}
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Substitution safety (R-ENV-02 / R-GITOPS-02) — refuse to emit a manifest that
|
||||
# Flux's envsubst would silently eat before the workload ever runs.
|
||||
# =============================================================================
|
||||
# Every manifest this renderer writes lands under apps/wave-<N>/, and EVERY apps-wave-<N>
|
||||
# Flux Kustomization declares `postBuild.substituteFrom: cluster-vars` (see
|
||||
# flux_wave_kustomization_text below). Flux therefore runs envsubst over the WHOLE rendered
|
||||
# document — including any shell script carried verbatim in a `lifecycle` hook or an env
|
||||
# value — at reconcile time, long before a container sees it. The escape contract is stated
|
||||
# in clusters/base/apps-vms.yaml and proven with `flux envsubst`:
|
||||
#
|
||||
# `${VAR}` -> SUBSTITUTED (empty when unset) => a shell brace must be written `$${VAR}`,
|
||||
# which envsubst emits back as `${VAR}`.
|
||||
# `$VAR` -> left alone => bare shell vars need NO escaping.
|
||||
# `$(cmd)` -> left alone => command substitution needs NO escaping.
|
||||
# `${A[0]}` -> HARD ERROR ("missing closing brace"), failing the whole Kustomization.
|
||||
#
|
||||
# This bit us for real (fixed 2026-09-01): gitea/nextcloud guarded their whole Keycloak OIDC
|
||||
# registration with `if [ -z "${..._OIDC_CLIENT_SECRET:-}" ]`. Neither name is a cluster-var,
|
||||
# so envsubst rendered the guard as `if [ -z "" ]` — ALWAYS TRUE — and every boot skipped the
|
||||
# registration entirely. Flux's STRICT substitution mode cannot catch this class: it
|
||||
# deliberately EXEMPTS the default-operator forms (`${V:-}`, `${V:=x}`, `${V:?}`), which is
|
||||
# exactly the form a defensive shell script uses. So the renderer checks it itself.
|
||||
CLUSTER_VARS_PATH = (
|
||||
Path(__file__).resolve().parent.parent / "clusters" / "base" / "cluster-vars.yaml"
|
||||
)
|
||||
|
||||
# A run of `$` followed by `{...}`. envsubst consumes `$$` as a literal `$`, so a run of N
|
||||
# dollars leaves the brace SUBSTITUTED when N is odd (`${X}`, `$$${X}`) and LITERAL when N is
|
||||
# even (`$${X}` — the escape). Deliberately greedy about the body so malformed/indexed forms
|
||||
# (`${A[0]}`) are caught too rather than skipped.
|
||||
_BRACED_EXPANSION_RE = re.compile(r"(\$+)\{([^{}]*)\}")
|
||||
_PLAIN_IDENT_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*\Z")
|
||||
|
||||
_CLUSTER_VARS_CACHE: dict = {}
|
||||
|
||||
|
||||
def cluster_vars_keys(path: Path | None = None) -> set:
|
||||
"""The declared cluster-vars keys — the ONLY names envsubst may legitimately expand.
|
||||
|
||||
Read from clusters/base/cluster-vars.yaml (the single knob surface, R-ENV-02) so the
|
||||
renderer never carries a stale hardcoded copy. Unreadable/absent => empty set, i.e.
|
||||
FAIL-CLOSED: with no known knobs, every braced expansion must be escaped for the shell.
|
||||
"""
|
||||
key = str(path or CLUSTER_VARS_PATH)
|
||||
if key not in _CLUSTER_VARS_CACHE:
|
||||
try:
|
||||
doc = yaml.safe_load(Path(key).read_text())
|
||||
data = doc.get("data") if isinstance(doc, dict) else None
|
||||
_CLUSTER_VARS_CACHE[key] = set(data) if isinstance(data, dict) else set()
|
||||
except Exception: # pragma: no cover - absent/malformed => fail closed
|
||||
_CLUSTER_VARS_CACHE[key] = set()
|
||||
return _CLUSTER_VARS_CACHE[key]
|
||||
|
||||
|
||||
def substitution_violations(text: str, known: set | None = None) -> list:
|
||||
"""Every unescaped `${...}` in `text` that is NOT a bare, declared cluster-vars key.
|
||||
|
||||
Returns `(line_number, token, reason)` triples in line order. An expansion is ALLOWED only
|
||||
when it is exactly `${KNOWN_KEY}` — a deliberate R-ENV-02 knob. Anything else is a bug:
|
||||
a shell expansion that envsubst will eat (`${V:-}`), a typo'd/undeclared knob that
|
||||
envsubst will blank, or an indexed form that hard-fails the whole Kustomization.
|
||||
"""
|
||||
keys = cluster_vars_keys() if known is None else known
|
||||
out = []
|
||||
for lineno, line in enumerate(text.splitlines(), start=1):
|
||||
for m in _BRACED_EXPANSION_RE.finditer(line):
|
||||
if len(m.group(1)) % 2 == 0:
|
||||
continue # even run of `$` => escaped; envsubst emits it back literally
|
||||
expr = m.group(2)
|
||||
if _PLAIN_IDENT_RE.match(expr):
|
||||
if expr in keys:
|
||||
continue # a declared cluster-vars knob, intentionally substituted
|
||||
reason = (
|
||||
f"'{expr}' is not a declared cluster-vars key — envsubst will replace "
|
||||
"it with the EMPTY STRING"
|
||||
)
|
||||
elif "[" in expr:
|
||||
reason = (
|
||||
"indexed expansion — envsubst hard-fails with 'missing closing brace' "
|
||||
"and the whole Kustomization stops reconciling"
|
||||
)
|
||||
else:
|
||||
reason = (
|
||||
"shell expansion (default-operator/parameter form) — envsubst eats it "
|
||||
"at reconcile time and STRICT mode exempts this form, so nothing else "
|
||||
"will catch it"
|
||||
)
|
||||
out.append((lineno, m.group(0), reason))
|
||||
return out
|
||||
|
||||
|
||||
def assert_substitution_safe(text: str, origin: str = "<render>") -> None:
|
||||
"""Refuse to emit a manifest carrying an unescaped braced expansion (see above).
|
||||
|
||||
Raises ValueError listing every offending token. The fix is almost always to escape the
|
||||
shell brace as `$${...}` (envsubst emits a literal `${...}` for the shell), or — if the
|
||||
value really is an environment knob — to declare it in clusters/base/cluster-vars.yaml
|
||||
and write it as a bare `${KNOB}`.
|
||||
"""
|
||||
bad = substitution_violations(text)
|
||||
if not bad:
|
||||
return
|
||||
lines = [
|
||||
f"{origin}: refusing to emit — unescaped `${{...}}` in a manifest that Flux's "
|
||||
"postBuild.substituteFrom (cluster-vars) will substitute at reconcile time:"
|
||||
]
|
||||
for lineno, token, reason in bad:
|
||||
lines.append(f" line {lineno}: {token} -> {reason}")
|
||||
lines.append(
|
||||
" FIX: a SHELL expansion must be escaped `$${...}` (envsubst emits a literal "
|
||||
"`${...}`); bare `$VAR` / `$(cmd)` need no escaping. A genuine environment knob "
|
||||
"must be declared in clusters/base/cluster-vars.yaml and written bare `${KNOB}`. "
|
||||
"Contract: clusters/base/apps-vms.yaml."
|
||||
)
|
||||
raise ValueError("\n".join(lines))
|
||||
|
||||
|
||||
# =============================================================================
|
||||
# Top-level render + emit
|
||||
# =============================================================================
|
||||
@@ -1026,8 +1145,14 @@ def render(sd: dict, image_automation: bool = True, inbound: list | None = None)
|
||||
return docs
|
||||
|
||||
|
||||
def emit(docs: list) -> str:
|
||||
"""Deterministic multi-document YAML text (golden-diffable)."""
|
||||
def emit(docs: list, origin: str = "<render>") -> str:
|
||||
"""Deterministic multi-document YAML text (golden-diffable).
|
||||
|
||||
Every emitted document is checked against the substitution contract before it is handed
|
||||
back (assert_substitution_safe): the rendered manifest lands on a Flux path that runs
|
||||
envsubst over it, so an unescaped `${...}` meant for a SHELL is silently blanked. Raises
|
||||
ValueError rather than emitting such a manifest.
|
||||
"""
|
||||
parts = []
|
||||
for comment, resource in docs:
|
||||
body = yaml.dump(
|
||||
@@ -1037,7 +1162,9 @@ def emit(docs: list) -> str:
|
||||
width=4096,
|
||||
)
|
||||
parts.append(f"---\n# {comment}\n{body}".rstrip() + "\n")
|
||||
return "".join(parts)
|
||||
text = "".join(parts)
|
||||
assert_substitution_safe(text, origin)
|
||||
return text
|
||||
|
||||
|
||||
def load_sd(path: Path) -> dict:
|
||||
@@ -1076,7 +1203,9 @@ def render_file(path: Path, image_automation: bool = True, catalog_dir: Path | N
|
||||
src_dir = catalog_dir if catalog_dir is not None else path.parent
|
||||
rev = build_reverse_map(_load_catalog(src_dir))
|
||||
inbound = rev.get(sd["name"], [])
|
||||
return emit(render(sd, image_automation=image_automation, inbound=inbound))
|
||||
return emit(
|
||||
render(sd, image_automation=image_automation, inbound=inbound), origin=str(path)
|
||||
)
|
||||
|
||||
|
||||
# =============================================================================
|
||||
@@ -1729,6 +1858,90 @@ def _selftest() -> int:
|
||||
check(kt["kind"] == "Kustomization" and kt["resources"] == ["svc.yaml", "sonarr.yaml"],
|
||||
"wave kustomization must list exactly the wave's resources")
|
||||
|
||||
# 8) substitution safety (R-ENV-02): the renderer must REFUSE to emit a manifest whose
|
||||
# shell script carries an unescaped `${...}`. Every rendered manifest lands under a
|
||||
# Flux path with postBuild.substituteFrom cluster-vars, so envsubst rewrites it before
|
||||
# the container runs — this is the gitea/nextcloud OIDC-guard bug (`[ -z "${X:-}" ]`
|
||||
# -> `[ -z "" ]`, always true, whole registration block skipped on every boot).
|
||||
known = {"BASE_DOMAIN", "GATEWAY_VIP"}
|
||||
# allowed: a bare, DECLARED cluster-vars knob is a deliberate R-ENV-02 substitution.
|
||||
check(
|
||||
substitution_violations("host: app.${BASE_DOMAIN}", known) == [],
|
||||
"a bare declared cluster-vars key must be allowed (that is what substitution is FOR)",
|
||||
)
|
||||
# allowed: escaped shell braces, and the forms envsubst leaves alone.
|
||||
check(
|
||||
substitution_violations('if [ -z "$${X:-}" ]; then', known) == [],
|
||||
"an ESCAPED `$${...}` must be allowed (envsubst emits it back as a literal `${...}`)",
|
||||
)
|
||||
check(
|
||||
substitution_violations('t="$(mktemp)"; echo "$HOME $1"', known) == [],
|
||||
"bare `$VAR` / `$(cmd)` are left alone by envsubst and must NOT be flagged",
|
||||
)
|
||||
# `$$` is a literal `$` to envsubst, so an ODD run of dollars still leaves the brace live.
|
||||
check(
|
||||
len(substitution_violations("$$${X:-}", known)) == 1,
|
||||
"an ODD run of `$` still substitutes ($$ -> literal $, then `${X:-}` is eaten)",
|
||||
)
|
||||
# flagged: the exact regression — a default-operator form STRICT mode deliberately exempts.
|
||||
guard_bug = substitution_violations('if [ -z "${GITEA_OIDC_CLIENT_SECRET:-}" ]; then', known)
|
||||
check(
|
||||
len(guard_bug) == 1 and guard_bug[0][1] == "${GITEA_OIDC_CLIENT_SECRET:-}",
|
||||
"an unescaped `${VAR:-}` shell guard must be flagged (strict mode exempts this form)",
|
||||
)
|
||||
# flagged: an undeclared bare knob (envsubst blanks it), and an indexed form (hard error).
|
||||
check(
|
||||
len(substitution_violations("value: ${NOT_A_KNOB}", known)) == 1,
|
||||
"an undeclared bare `${VAR}` must be flagged (envsubst blanks it silently)",
|
||||
)
|
||||
idx = substitution_violations("echo ${PIPESTATUS[0]}", known)
|
||||
check(
|
||||
len(idx) == 1 and "missing closing brace" in idx[0][2],
|
||||
"an indexed `${A[0]}` must be flagged (envsubst hard-fails the whole Kustomization)",
|
||||
)
|
||||
# the checker must be wired into emit(): rendering an SD whose lifecycle hook carries the
|
||||
# bug RAISES rather than writing a manifest whose guard is dead on arrival.
|
||||
bad_sd = {
|
||||
"name": "subbad",
|
||||
"source": {"image": "a:1"},
|
||||
"lifecycle": {
|
||||
"postStart": {
|
||||
"exec": {
|
||||
"command": [
|
||||
"/bin/sh",
|
||||
"-c",
|
||||
'if [ -z "${SUBBAD_SECRET:-}" ]; then exit 0; fi',
|
||||
]
|
||||
}
|
||||
}
|
||||
},
|
||||
}
|
||||
try:
|
||||
emit(render(bad_sd), origin="<selftest>")
|
||||
check(False, "emit must REFUSE a manifest with an unescaped `${...}` shell expansion")
|
||||
except ValueError as exc:
|
||||
check(
|
||||
"SUBBAD_SECRET" in str(exc) and "$${" in str(exc),
|
||||
"the refusal must name the offending token AND state the `$${...}` fix",
|
||||
)
|
||||
# the ESCAPED form of the same hook renders normally (the fix must be emittable).
|
||||
good_sd = dict(bad_sd, name="subgood")
|
||||
good_sd["lifecycle"] = {
|
||||
"postStart": {
|
||||
"exec": {
|
||||
"command": [
|
||||
"/bin/sh",
|
||||
"-c",
|
||||
'if [ -z "$${SUBBAD_SECRET:-}" ]; then exit 0; fi',
|
||||
]
|
||||
}
|
||||
}
|
||||
}
|
||||
check(
|
||||
"$${SUBBAD_SECRET:-}" in emit(render(good_sd), origin="<selftest>"),
|
||||
"the escaped `$${...}` form must render (and reach the shell as `${...}`)",
|
||||
)
|
||||
|
||||
if failures:
|
||||
for f in failures:
|
||||
sys.stderr.write(f"SELFTEST FAIL: {f}\n")
|
||||
|
||||
Reference in new issue
Block a user