From 9a4489b7330e959e7801e01cf64f95a98e44897c Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 10:55:06 -0500 Subject: [PATCH 01/13] fix(auth): five oauth2-proxy sidecars shared one cookie and evicted each other Every oauth2-proxy on this platform is an independent instance with its OWN cookie-secret. Measured on prod 2026-09-30: seven `*-oauth2-proxy` Secrets, seven DISTINCT cookie-secret values (compared in-cluster, never printed). So no instance can DECRYPT another's cookie -- it can only OVERWRITE it. All of them were emitting oauth2-proxy's DEFAULT names (`_oauth2_proxy`, `_oauth2_proxy_csrf`) scoped to `.${BASE_DOMAIN}`, so the browser kept ONE cookie pair for SIX live instances (jackett, pihole, qbittorrent, radarr, sonarr, and the shared forward-auth proxy). Whichever authenticated last clobbered the rest; the loser logged `No valid authentication in request`, bounced to Keycloak, and -- the SSO session still being live -- was redirected straight back and clobbered it again. The symptom is "I signed into Keycloak but I'm still not signed in", which sends you hunting in Keycloak, where nothing is wrong. Crafty was fixed on its own side on 2026-09-29 (#22); this is that fix generalized to the pattern. THE DEFECT WAS NOT IN THE MANIFESTS. apps/wave-*/{jackett,pihole,qbittorrent,radarr, sonarr}.yaml are GENERATED goldens; one function emits all five. So the fix is in catalog/render.py::_oauth2_sidecar_container(), and both flags are now DERIVED from the ServiceDefinition rather than defaulted: --cookie-name=_oauth2_proxy_{s['name']} unique by construction --cookie-domain={host} the app's own host, not the zone A per-service table was rejected for the reason twelve ServiceMonitors once acquired one identical wrong port name in a single edit: it would leave the NEXT app onboarded defaulting straight back into the collision. --whitelist-domain stays zone-wide; it governs post-login REDIRECT targets, not cookie scope. Host-scoping is defence in depth and is asserted deliberately: a zone-wide cookie is transmitted by the browser to every host in the zone, so each app's session token was being sent to every sibling -- and it is what would make a FUTURE name collision platform-wide instead of contained. New gate check (whole-repo scope, so it runs on EVERY gate invocation, not only when a touched path happens to attribute an identity requirement): conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/ It resolves each instance's EFFECTIVE cookie name -- declared, or oauth2-proxy's built-in default when omitted, because an omitted flag is not an absence but a CLAIM on a name, and reading it as an absence is exactly why this survived five reviews. Then: no two collide; none is another's name + `_csrf` (oauth2-proxy derives the CSRF cookie that way); the default has exactly ONE owner, registered with its reason in a shrink-only register -- the SHARED proxy, which MUST keep `_oauth2_proxy` because renaming it signs every other gated surface out at once; every other instance is host-scoped; render.py derives rather than lists; and the SHIPPED renderer is proven end-to-end on two synthetic ServiceDefinitions, so the check stays falsifiable once the tree is clean. Tested in both directions: rc 0 on this tree (39 assertions, 7 instances), rc 1 with 26 failures against the pre-fix tree, plus one targeted mutation per assertion class (duplicate name, CSRF derivation, zone-wide domain, unregistered default heir, stale register entry) -- each fired, and nothing else did. Also regenerates the three L1 rendering goldens, which are expected-output fixtures of the same renderer. UNVERIFIED: no browser round-trip. The collision has still never been REPRODUCED in a browser -- doing so needs two gated apps visited in one session -- so this is reasoned from measured configuration and from the mechanism, not from a failure anyone watched. Nothing was applied; merging rolls five Deployments and invalidates every existing session on them once. See decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md. Co-Authored-By: Claude Opus 5 (1M context) --- apps/wave-0/jackett.yaml | 3 +- apps/wave-0/pihole.yaml | 3 +- apps/wave-0/qbittorrent.yaml | 3 +- apps/wave-1/radarr.yaml | 3 +- apps/wave-1/sonarr.yaml | 3 +- catalog/render.py | 87 ++- .../chainsaw-test.yaml | 152 ++++++ .../check.sh | 498 ++++++++++++++++++ .../golden/expected.yaml | 3 +- .../golden/egress-internet.expected.yaml | 3 +- .../golden/egress-vpn.expected.yaml | 3 +- ...-30-oauth2-proxy-cookie-collision-fleet.md | 179 +++++++ 12 files changed, 930 insertions(+), 10 deletions(-) create mode 100644 conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/chainsaw-test.yaml create mode 100755 conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/check.sh create mode 100644 decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md diff --git a/apps/wave-0/jackett.yaml b/apps/wave-0/jackett.yaml index 9d2574d..461d2d7 100644 --- a/apps/wave-0/jackett.yaml +++ b/apps/wave-0/jackett.yaml @@ -100,7 +100,8 @@ spec: - --allowed-group=platform-admins - --skip-provider-button=true - --redirect-url=https://jackett.${BASE_DOMAIN}/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_jackett + - --cookie-domain=jackett.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/apps/wave-0/pihole.yaml b/apps/wave-0/pihole.yaml index 4d65465..c88fe7c 100644 --- a/apps/wave-0/pihole.yaml +++ b/apps/wave-0/pihole.yaml @@ -108,7 +108,8 @@ spec: - --allowed-group=platform-admins - --skip-provider-button=true - --redirect-url=https://pihole.${BASE_DOMAIN}/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_pihole + - --cookie-domain=pihole.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/apps/wave-0/qbittorrent.yaml b/apps/wave-0/qbittorrent.yaml index 6551fcf..252997c 100644 --- a/apps/wave-0/qbittorrent.yaml +++ b/apps/wave-0/qbittorrent.yaml @@ -171,7 +171,8 @@ spec: - --allowed-group=platform-admins - --skip-provider-button=true - --redirect-url=https://qbittorrent.${BASE_DOMAIN}/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_qbittorrent + - --cookie-domain=qbittorrent.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/apps/wave-1/radarr.yaml b/apps/wave-1/radarr.yaml index e84f565..2fd8042 100644 --- a/apps/wave-1/radarr.yaml +++ b/apps/wave-1/radarr.yaml @@ -153,7 +153,8 @@ spec: - --allowed-group=platform-admins - --skip-provider-button=true - --redirect-url=https://radarr.${BASE_DOMAIN}/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_radarr + - --cookie-domain=radarr.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/apps/wave-1/sonarr.yaml b/apps/wave-1/sonarr.yaml index 5b0d98b..b107c45 100644 --- a/apps/wave-1/sonarr.yaml +++ b/apps/wave-1/sonarr.yaml @@ -153,7 +153,8 @@ spec: - --allowed-group=platform-admins - --skip-provider-button=true - --redirect-url=https://sonarr.${BASE_DOMAIN}/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_sonarr + - --cookie-domain=sonarr.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/catalog/render.py b/catalog/render.py index cc6c9a5..af09aa7 100644 --- a/catalog/render.py +++ b/catalog/render.py @@ -971,7 +971,11 @@ def _oauth2_sidecar_container(s: dict) -> dict: auth.group (R-IDP-06: a valid session is not enough — the user must be in an authorized group, else /oauth2 returns 403). client-id/secret + cookie-secret come by name from the SOPS-encrypted `-oauth2-proxy` Secret (R-IDP-04). Single provider => --skip-provider-button 302s straight - to Keycloak. The sidecar->app hop is pod-local (127.0.0.1) and not subject to network policy.""" + to Keycloak. The sidecar->app hop is pod-local (127.0.0.1) and not subject to network policy. + + --cookie-name and --cookie-domain are DERIVED per app (see the block comment on those two + args): every instance holds a different cookie-secret, so sharing one cookie name across the + zone makes the instances evict each other's sessions.""" route = s["ingressRoutes"][0] host = route["host"] upstream_port = route["port"] @@ -991,7 +995,41 @@ def _oauth2_sidecar_container(s: dict) -> dict: args += [ "--skip-provider-button=true", f"--redirect-url=https://{host}/oauth2/callback", - f"--cookie-domain=.{ZONE}", + # ── COOKIE ISOLATION (fleet fix 2026-09-30; crafty's 2026-09-29 fix, generalized) ────── + # EVERY oauth2-proxy on this platform — this sidecar, its four siblings, and the SHARED + # forward-auth proxy (platform/oauth2-proxy) — is an INDEPENDENT instance holding its OWN + # `cookie-secret`. Measured on prod 2026-09-30: seven `*-oauth2-proxy` Secrets, seven + # DISTINCT cookie-secret values, so no instance can decrypt any other's cookie. + # + # Left at oauth2-proxy's DEFAULTS (`_oauth2_proxy` / `_oauth2_proxy_csrf`) and scoped to + # `.${BASE_DOMAIN}`, the browser therefore keeps ONE cookie pair for ALL of them: + # whichever proxy authenticates last OVERWRITES every other's session. The loser logs + # "No valid authentication in request" and bounces to Keycloak, which — the SSO session + # still being live — redirects straight back and overwrites the cookie again. The + # user-visible symptom is "I signed into Keycloak but I'm still not signed in", and it + # sends you hunting in Keycloak, where nothing is wrong. Silent, intermittent, and + # misattributed: the exact shape this repo keeps getting burned by. + # + # DERIVED, NOT LISTED. Both flags come from the ServiceDefinition (`s['name']` and the + # route host), so a NEW forward-auth app is isolated the moment it is onboarded. A + # per-service table would leave the next app defaulting into the collision, which is how + # twelve ServiceMonitors acquired one identical wrong port name in a single edit. + # conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct asserts both properties + # over the WHOLE tree and asserts THIS function derives rather than lists. + # + # --cookie-name a unique name per instance, so two sessions cannot alias at all. + # This is the fix; the one below is defence in depth. + # --cookie-domain HOST-scoped. A per-app reverse proxy has no reason to broadcast its + # session cookie to every other host in the zone — a zone-wide cookie + # is transmitted to every sibling app on `.${BASE_DOMAIN}`, so any + # FUTURE name collision would again be platform-wide instead of + # contained. The SHARED forward-auth proxy keeps the wide scope, + # because cross-host forward-auth is precisely its job. + # + # --whitelist-domain deliberately stays ZONE-WIDE: it governs permitted post-login + # REDIRECT targets, not cookie scope. Narrowing it here would break the sign-in bounce. + f"--cookie-name=_oauth2_proxy_{s['name']}", + f"--cookie-domain={host}", f"--whitelist-domain=.{ZONE}", "--cookie-secure=true", "--pass-access-token=true", @@ -2475,6 +2513,51 @@ def _selftest() -> int: not any(a.startswith("--ssl-upstream-insecure-skip-verify") for a in side["args"]), "http upstream must NOT skip TLS verify (that flag is only for crafty's self-signed https)", ) + # COOKIE ISOLATION. Every oauth2-proxy instance holds its own cookie-secret, so two instances + # sharing a cookie NAME on a shared DOMAIN evict each other's sessions silently. Both flags + # must be DERIVED from this service, not defaulted and not listed. + check( + "--cookie-name=_oauth2_proxy_fa" in side["args"], + "sidecar must declare a cookie name DERIVED from the service name — the oauth2-proxy " + "default (_oauth2_proxy) is shared with every other instance on the zone", + ) + check( + "--cookie-domain=fa.home.keithjackson.dev" in side["args"], + "sidecar cookie-domain must be HOST-scoped (the app's own host), not zone-wide", + ) + check( + f"--whitelist-domain=.{ZONE}" in side["args"], + "whitelist-domain must stay ZONE-wide — it governs post-login redirect targets, not " + "cookie scope; narrowing it breaks the sign-in bounce", + ) + # Two forward-auth apps must not be able to collide. Renders a SECOND one and compares, so a + # future refactor to a constant (or to a per-service table with a copy-paste slip) fails here + # rather than silently in a browser six weeks later. + fa2_side = next( + c + for c in next( + d[1] + for d in render( + { + "name": "fa2", + "source": {"image": "a:1"}, + "ingressRoutes": [{"host": "fa2.home.keithjackson.dev", "port": 9118}], + "auth": {"mode": "forward-auth", "groups": ["media-users"]}, + } + ) + if d[1]["kind"] == "Deployment" + )["spec"]["template"]["spec"]["containers"] + if c["name"] == "oauth2-proxy" + ) + + def _cookie_name(container): + return next(a for a in container["args"] if a.startswith("--cookie-name=")) + + check( + _cookie_name(side) != _cookie_name(fa2_side), + "two forward-auth apps rendered IDENTICAL cookie names — they would overwrite each " + "other's session on every sign-in (the 2026-09-29 crafty defect, fleet-wide)", + ) check( side["env"][0]["valueFrom"]["secretKeyRef"]["name"] == "fa-oauth2-proxy", "sidecar creds must come by name from the per-app -oauth2-proxy Secret (R-IDP-04)", diff --git a/conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/chainsaw-test.yaml b/conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/chainsaw-test.yaml new file mode 100644 index 0000000..841bf86 --- /dev/null +++ b/conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/chainsaw-test.yaml @@ -0,0 +1,152 @@ +# ============================================================================= +# L0 STATIC — no two oauth2-proxy instances can share a browser cookie +# Catalog: docs/14 L0 row ("Pre-apply, in CI") — runs WITHOUT a cluster. +# +# THE DEFECT THIS CLOSES. Every oauth2-proxy on this platform is an INDEPENDENT instance with +# its OWN `cookie-secret`: five per-app reverse-proxy sidecars (jackett, pihole, qbittorrent, +# radarr, sonarr), crafty's, and the SHARED forward-auth proxy. Measured on prod 2026-09-30: +# seven `*-oauth2-proxy` Secrets, SEVEN DISTINCT cookie-secret values — so no instance can +# decrypt any other's cookie. +# +# They were all emitting oauth2-proxy's DEFAULT cookie names (`_oauth2_proxy` and +# `_oauth2_proxy_csrf`), scoped to `.${BASE_DOMAIN}`. A cookie is keyed by (name, domain, path), +# so the browser kept ONE pair for ALL of them. Whichever proxy authenticated last OVERWROTE +# every other's session; the loser logged `No valid authentication in request`, bounced to +# Keycloak, and — the SSO session still being live — was redirected straight back and overwrote +# the cookie again. Six live instances, one cookie, mutual eviction. +# +# WHY IT IS THE WORST KIND OF BUG. The symptom is "I signed into Keycloak but I'm still not +# signed in", which points the investigator at Keycloak, where nothing is wrong. It is +# INTERMITTENT — it only appears once a browser has visited a SECOND gated app, so the obvious +# single-app reproduction succeeds — and it is SILENT: every Deployment stays Ready, every +# Kustomization stays Ready, the proxy's own log line names an authentication failure rather +# than a collision. It went unnoticed across five services for the whole life of the pattern, +# and was found only as a side effect of investigating something else (2026-09-29, crafty). +# +# WHY A COMMIT IS NOT ENOUGH, AND THIS IS A GATE STAGE. The collision is a property of the +# PATTERN, not of any one app: the next forward-auth service onboarded inherits the defect by +# doing nothing at all, because a missing flag IS the bug. Nothing about its absence is loud. +# This is the same shape as the twelve inert ServiceMonitors in this tree — one omission, +# copied, invisible for weeks — and the same remedy: make the omission fail the merge. +# +# GIVEN every oauth2-proxy instance this repo authors — a container in any manifest (the +# rendered sidecars in apps/, crafty's hand-authored one in platform/) and the shared +# proxy's HelmRelease values, +# WHEN each instance's EFFECTIVE cookie name is resolved (its declared `--cookie-name` / +# `OAUTH2_PROXY_COOKIE_NAME`, or oauth2-proxy's built-in default `_oauth2_proxy` when it +# declares none — an omission is a NAME, not an absence, and that is the whole bug), +# THEN (1) no two instances share an effective cookie name; +# (2) no instance's effective name is another's name + `_csrf` — oauth2-proxy derives the +# CSRF cookie that way, so `_oauth2_proxy` and `_oauth2_proxy_csrf` as two SESSION +# names would collide on the second cookie while looking distinct on the first; +# (3) the DEFAULT name has exactly ONE owner and it is the instance declared in +# SHARED_DEFAULT_COOKIE_OWNER. This is the deliberate ASYMMETRY. The shared +# forward-auth proxy MUST keep `_oauth2_proxy`: it is the single session every +# OTHER forward-auth-gated surface on the platform rides on, so renaming it signs +# every one of them out at once. The rule is therefore NOT "every instance must +# declare a name" — it is "the default is a NAME LIKE ANY OTHER, held by exactly one +# REGISTERED owner". That formulation needs no exemption from (1), and it closes the +# hole an exemption would leave: if the shared proxy is ever removed, an app silently +# inheriting the default would still FAIL here instead of quietly becoming its heir; +# (4) every instance that is NOT that owner is HOST-SCOPED: its `--cookie-domain` equals +# the host of its own `--redirect-url`. See the argument below; +# (5) catalog/render.py DERIVES both flags from the ServiceDefinition (the service name +# and the route host) and does not carry a per-service table. A list is how twelve +# ServiceMonitors acquired one identical wrong port name in a single edit, and a list +# leaves the NEXT app defaulting into the collision; +# (6) the SHIPPED renderer is proven END-TO-END on synthetic ServiceDefinitions, run as a +# subprocess: two forward-auth fixtures must yield two different, non-default cookie +# names, each scoped to its own host. Without this, once the tree is compliant (1)-(4) +# are satisfied by a tree with nothing left to catch, and a guard whose only evidence +# is "nothing failed" cannot be told apart from one that stopped working; +# AND (7) the check is LOAD-BEARING: this Test DECLARES `conformance.home.k3s/scope: +# whole-repo`, so ci/l0.sh Stage 3b's `--include-scope whole-repo` unions it into +# EVERY gate run, and ci/gate.sh blocks the merge when the l0 stage fails. It +# deliberately adds NO stage to ci/l0.sh — tests/l0-static/whole-repo-invariants-wired +# fails the gate if anyone adds one. +# +# WHY SCOPE, NOT REQUIREMENTS: ci/touched-requirements.sh maps `apps/*` to R-IDP-02/R-IDP-06 +# today, but the instances live in THREE unrelated subtrees (apps/, platform/crafty/, +# platform/oauth2-proxy/) and the file most likely to reintroduce the defect is +# catalog/render.py, which maps to R-GITOPS-02 and to no identity requirement at all. A +# requirement-scoped selection would filter this check out of exactly the change that breaks it. +# +# ON ASSERTION (4), HOST-SCOPED COOKIE-DOMAIN — the argument, and the decision. +# AGAINST asserting it (overreach): distinct NAMES already close the eviction bug completely. +# Two cookies with different names do not overwrite each other however wide their domains are, +# so (4) fixes nothing that (1) has not already fixed. `--cookie-domain` is also a legitimate +# knob — a future proxy fronting two hostnames of one app would need a wider value — and a +# check that fails correct configurations is a check someone eventually disables. +# FOR asserting it (and this wins): a zone-wide cookie is TRANSMITTED BY THE BROWSER TO EVERY +# HOST IN THE ZONE. qBittorrent's session token is therefore sent to pihole, to radarr, to +# sonarr, and to every other name under `.${BASE_DOMAIN}` — including hosts behind no proxy at +# all. That is a session-credential disclosure across trust boundaries that no one needs and +# nobody asked for; the sidecar's session is only ever meaningful to the one app it fronts. +# And it is what makes a FUTURE name collision catastrophic rather than contained: with +# host-scoped cookies, two instances that accidentally share a name cannot even see each +# other's cookie, so the failure this whole test exists to prevent degrades from platform-wide +# to nothing. Defence in depth on the exact axis that already failed once. +# DECISION: assert it, for every instance except the registered shared proxy — whose whole +# job IS cross-host forward-auth, and which is exempted by name and with a reason, not by +# loosening the rule. `--whitelist-domain` is deliberately NOT touched: it governs permitted +# post-login REDIRECT targets, not cookie scope, and narrowing it breaks the sign-in bounce. +# +# STATUS: EXECUTABLE. `check.sh` is an offline assertion script (PyYAML, as catalog/render.py +# and the sibling l0-static checks use); conformance/run.sh --layer l0 and ci/l0.sh run it +# directly, and this Chainsaw Test wraps the SAME script. +# +# NOT proven (honest gaps; this is STATIC, with no cluster and no browser): +# - That anyone can actually sign in. No browser round-trip is performed, and none can be +# from L0. The 2026-09-29 crafty entry recorded the same gap and it is still open: the +# symptom has never been REPRODUCED in a browser, so this is a latent-bug fix reasoned from +# measured configuration, not a fix watched to work. +# - That the LIVE pods carry what the tree says. git is not the cluster (CLAUDE.md §2b). The +# live flags were measured separately, once, by hand; nothing here re-measures them, and a +# hand-applied drift would not be visible to this check. +# - That the cookie-secrets really differ. That is a Secret comparison and belongs in-cluster; +# it was measured out of band (seven distinct values, compared without printing any of +# them). If they were ever made IDENTICAL the collision would become harmless — but sharing +# one session-encryption key across trust zones is a worse idea than the bug it would mask, +# so this check does not offer it as an alternative and does not look for it. +# - Anything about the CONTENT of a session, group authorization, or the CSRF cookie's own +# behaviour. Names and domains only. +# - It does not read ../home-network-k3s. The spec is read-only (R-TEST-06). +# ============================================================================= +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: oauth2-proxy-cookie-names-distinct + labels: + conformance.home.k3s/R-IDP-02: "true" + conformance.home.k3s/R-IDP-06: "true" + annotations: + conformance.home.k3s/requirements: "R-IDP-02, R-IDP-06" + conformance.home.k3s/layer: "L0" + # WHOLE-REPO SCOPE — the instances span apps/wave-0, apps/wave-1, platform/crafty and + # platform/oauth2-proxy, and the generator that emits five of them is catalog/render.py. + # No single requirement path-maps to that set, and the renderer maps to no identity + # requirement at all, so a requirement-scoped selection would skip this check on precisely + # the change that can reintroduce the defect fleet-wide in one edit. + conformance.home.k3s/scope: "whole-repo" + conformance.home.k3s/safety-critical: "false" + conformance.home.k3s/executable: "true" + conformance.home.k3s/intent: >- + No two oauth2-proxy instances can share a browser cookie: every instance's effective + cookie name is unique, the built-in default belongs to exactly one registered owner (the + shared forward-auth proxy, which cannot be renamed without signing every gated app out), + every other instance is host-scoped, and catalog/render.py derives both flags from the + ServiceDefinition rather than from a per-service list — proven end-to-end on synthetic + fixtures so the guard stays falsifiable once the tree is clean. +spec: + steps: + - name: assert-no-two-oauth2-proxy-instances-share-a-cookie + description: >- + Inventory every oauth2-proxy container and HelmRelease in the tree, resolve each one's + effective cookie name (declared, or oauth2-proxy's default when omitted), assert + pairwise distinctness including the session/CSRF derivation, ratchet the single + registered owner of the default name, assert host-scoped cookie-domain on every other + instance, assert catalog/render.py derives rather than lists, and prove the shipped + renderer on two synthetic forward-auth ServiceDefinitions. + try: + - script: + content: bash ./check.sh diff --git a/conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/check.sh b/conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/check.sh new file mode 100755 index 0000000..d05fa53 --- /dev/null +++ b/conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/check.sh @@ -0,0 +1,498 @@ +#!/usr/bin/env bash +# ============================================================================= +# L0 STATIC CHECK — no two oauth2-proxy instances can share a browser cookie. +# See chainsaw-test.yaml's header for WHY this exists, what it measured, and the argument +# for assertion (4). In one paragraph: +# +# Every oauth2-proxy here holds its OWN cookie-secret (seven distinct values on prod, +# 2026-09-30), so two instances sharing a cookie NAME on a shared DOMAIN cannot read each +# other's session — they OVERWRITE it. The loser bounces to Keycloak, is redirected straight +# back by the live SSO session, and overwrites the winner in turn. The symptom is "I signed +# into Keycloak but I'm still not signed in", it only shows up after a second gated app is +# visited, and nothing in the cluster reports an error. A missing --cookie-name IS the bug, +# so the next forward-auth app inherits it by doing nothing. +# +# Exit 0 = all assertions pass. Exit 1 = a failure. Exit 3 (honest SKIP, which the runner +# turns into rc 3 = UNWIRED, never green, because this test declares the whole-repo scope) +# only if the tree contains no oauth2-proxy instance at all — genuinely nothing to audit. +# ============================================================================= +set -euo pipefail +DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +ROOT="$(cd "${DIR}/../../../.." && pwd)" # repo root +PYTHON="${PYTHON:-python3}" + +# Honest SKIP (never green for a scope-declaring test) if there is nothing to audit at all. +# Matches the IMAGE every instance runs and the CHART the shared one installs; if NEITHER +# appears anywhere, the forward-auth pattern has left this repo and the check has no subject. +# +# ANCHORED ON `image:` DELIBERATELY. A bare `oauth2-proxy/oauth2-proxy:` also matches +# platform/oauth2-proxy/forward-auth-cec.yaml's Envoy cluster reference +# (`serviceName: oauth2-proxy/oauth2-proxy:80` — namespace/service:port, not a registry path). +# That file configures the ext_authz listener that CALLS the proxy; it runs no proxy and holds +# no cookie flags, so counting it made the grep/parse cross-check below report a phantom +# unaudited instance. +if ! grep -rlE 'image:[[:space:]]*[^[:space:]]*oauth2-proxy|chart:[[:space:]]*oauth2-proxy' \ + "${ROOT}/apps" "${ROOT}/platform" "${ROOT}/catalog" >/dev/null 2>&1; then + echo "check SKIPPED: no oauth2-proxy image or chart found under apps/, platform/ or catalog/ — nothing to audit" >&2 + exit 3 +fi + +cd "${ROOT}" +"${PYTHON}" - "${ROOT}" <<'PY' +"""Static audit: no two oauth2-proxy instances can share a browser cookie. + +PyYAML (the same dependency as catalog/render.py and the sibling l0-static checks) rather +than line-based parsing: a container's args live in a nested structure and a HelmRelease's +flags live in a values map, and a line-based reader of either is precisely the approximate +parser that silently stops matching — the failure class this check exists to prevent. +""" +from __future__ import annotations + +import re +import subprocess +import sys +import tempfile +from pathlib import Path + +try: + import yaml +except ImportError: # pragma: no cover + print("check FAIL: PyYAML is required (same dependency as catalog/render.py)", + file=sys.stderr) + raise SystemExit(1) + +ROOT = Path(sys.argv[1]) +sys.dont_write_bytecode = True # keep __pycache__ out of the tracked tree + +FAILURES: list[str] = [] +CHECKS = 0 + + +def check(cond, msg): + global CHECKS + CHECKS += 1 + if not cond: + FAILURES.append(msg) + + +def read(p: Path) -> str: + try: + return p.read_text() + except (OSError, UnicodeDecodeError): + return "" + + +# oauth2-proxy's OWN built-in defaults (pkg/apis/options/cookie.go): the session cookie is +# `_oauth2_proxy` and the CSRF cookie is that name with `_csrf` appended. An instance that +# declares nothing is NOT unconfigured — it has claimed this exact name, which is why the +# inventory below treats the default as a name like any other rather than as an absence. +DEFAULT_COOKIE_NAME = "_oauth2_proxy" +CSRF_SUFFIX = "_csrf" + +# ============================================================================================= +# REGISTER — SHARED_DEFAULT_COOKIE_OWNER. Exactly one entry, and it must match a real instance. +# +# The one instance permitted to hold DEFAULT_COOKIE_NAME, and the one permitted a zone-wide +# cookie-domain. Both exemptions belong to the SAME object for the SAME reason, so they are one +# register rather than two: the shared forward-auth proxy is not a per-app sidecar, it is the +# single session that every forward-auth-gated surface on the platform rides on. Renaming its +# cookie signs all of them out simultaneously, and host-scoping it would break the cross-host +# forward-auth that is its entire job. +# +# Shrink-only, and exact in BOTH directions: an entry that matches no instance FAILS as stale +# (the proxy was renamed or removed and the exemption is now a hole), and an instance holding +# the default WITHOUT an entry FAILS as an unregistered heir. Adding an entry is a review event +# on a sensitive path. +# +# Keyed by ":". +# ============================================================================================= +SHARED_DEFAULT_COOKIE_OWNER: dict[str, str] = { + "platform/oauth2-proxy/helmrelease.yaml:oauth2-proxy": + "The SHARED forward-auth proxy (ADR-0014, platform/oauth2-proxy/README.md). It is the " + "single authenticator behind every forward-auth-gated surface that is NOT fronted by " + "its own sidecar, so its cookie is the platform session: renaming it signs every one " + "of them out at once, and that is why the 2026-09-29 crafty fix changed only the " + "sidecar side. Its cookie-domain is zone-wide for the same structural reason — " + "cross-host forward-auth is what it is for.", +} + + +def is_test_fixture(p: Path) -> bool: + """conformance/tests/**/{golden,fixtures}/** is assertion DATA, not deployed manifests. + + Auditing a golden would be auditing a copy of something already audited through apps/. + Same exclusion the sibling l0-static checks make, and the same "reference, not + declaration" distinction ci/touched-requirements.sh draws for fixture paths. + """ + parts = p.parts + return "conformance" in parts and ("golden" in parts or "fixtures" in parts) + + +def iter_containers(node): + """Yield every container mapping under any `containers`/`initContainers` list, at any depth. + + Depth-agnostic on purpose: a sidecar sits under spec.template.spec.containers in a + Deployment, but the same container shape appears under a CronJob's jobTemplate, a + DaemonSet, or a Pod, and a reader that hardcoded one path would silently stop seeing an + instance the day one moved. + """ + if isinstance(node, dict): + for key, val in node.items(): + if key in ("containers", "initContainers") and isinstance(val, list): + for c in val: + if isinstance(c, dict): + yield c + else: + yield from iter_containers(val) + elif isinstance(node, list): + for item in node: + yield from iter_containers(item) + + +def flag_from_args(args: list, flag: str): + """Value of `--flag=value` or `--flag value` in an argv list, else None.""" + pref = "--" + flag + for i, a in enumerate(args): + if not isinstance(a, str): + continue + if a.startswith(pref + "="): + return a.split("=", 1)[1] + if a == pref and i + 1 < len(args) and isinstance(args[i + 1], str): + return args[i + 1] + return None + + +def host_of(url): + """Host part of an https://host/path URL, else None.""" + if not isinstance(url, str): + return None + m = re.match(r"^https?://([^/]+)", url) + return m.group(1) if m else None + + +# ============================================================ (0) INVENTORY +# instances: list of dicts {rel, ident, kind, declared_name, cookie_domain, redirect_host} +instances: list[dict] = [] +unparseable: list[str] = [] +# Files the grep gate says contain an instance, so the parse can be cross-checked against it. +grep_files: set[str] = set() + +for p in sorted(ROOT.rglob("*.yaml")): + relparts = p.relative_to(ROOT).parent.parts + # NESTED WORKTREES: skip dot-DIRECTORIES (.git, .claude) while still matching dot-FILES + # such as .sops.yaml. .claude/worktrees/agent-*/ holds full repo copies for isolated agent + # runs, so a bare rglob audited up to 18 duplicates of every file — which both invents + # FAILs and, worse, can satisfy a "something declares X" check from a copy rather than from + # real source. Same fix as egress-paths-selected-by-rules. .claude/ is gitignored. + if any(part.startswith(".") for part in relparts) or is_test_fixture(p) or ".sops." in p.name: + continue + txt = read(p) + if not txt: + continue + rel = str(p.relative_to(ROOT)) + # Same anchoring as check.sh's skip gate: `image:` (or a chart name), never a bare path — + # see the comment there for the Envoy `serviceName: oauth2-proxy/oauth2-proxy:80` near-miss. + if re.search(r"image:\s*\S*oauth2-proxy|chart:\s*oauth2-proxy", txt): + grep_files.add(rel) + elif "oauth2-proxy" not in txt: + continue + try: + docs = [d for d in yaml.safe_load_all(txt) if isinstance(d, dict)] + except yaml.YAMLError as exc: + unparseable.append(f"{rel} ({exc})") + continue + for d in docs: + # --- shape 1: a container running the oauth2-proxy image ------------------------- + for c in iter_containers(d): + image = str(c.get("image") or "") + name = str(c.get("name") or "") + if "oauth2-proxy" not in image and name != "oauth2-proxy": + continue + args = [a for a in (list(c.get("args") or []) + list(c.get("command") or []))] + env = {e.get("name"): e.get("value") + for e in (c.get("env") or []) if isinstance(e, dict) and "value" in e} + owner = (d.get("metadata") or {}).get("name") or "?" + instances.append({ + "rel": rel, + "ident": f"{owner}/{name or 'oauth2-proxy'}", + "kind": "container", + "declared_name": flag_from_args(args, "cookie-name") + or env.get("OAUTH2_PROXY_COOKIE_NAME"), + "cookie_domain": flag_from_args(args, "cookie-domain") + or env.get("OAUTH2_PROXY_COOKIE_DOMAIN"), + "redirect_host": host_of(flag_from_args(args, "redirect-url") + or env.get("OAUTH2_PROXY_REDIRECT_URL")), + }) + # --- shape 2: the shared proxy's HelmRelease values ------------------------------ + if d.get("kind") != "HelmRelease": + continue + spec = d.get("spec") or {} + chart = (((spec.get("chart") or {}).get("spec") or {}).get("chart") or "") + if "oauth2-proxy" not in str(chart): + continue + values = spec.get("values") or {} + extra = values.get("extraArgs") or {} + if isinstance(extra, list): # the chart also accepts a bare argv list + extra = {a.lstrip("-").split("=", 1)[0]: a.split("=", 1)[1] + for a in extra if isinstance(a, str) and "=" in a} + cfg = values.get("config") or {} + get = lambda k: extra.get(k, cfg.get(k)) # noqa: E731 - two-source lookup, used 3x + instances.append({ + "rel": rel, + "ident": str((d.get("metadata") or {}).get("name") or "oauth2-proxy"), + "kind": "helmrelease", + "declared_name": get("cookie-name"), + "cookie_domain": get("cookie-domain"), + "redirect_host": host_of(get("redirect-url")), + }) + +for u in unparseable: + check(False, f"{u} is not parseable YAML — every oauth2-proxy instance in it went unaudited") + +for ins in instances: + ins["key"] = f"{ins['rel']}:{ins['ident']}" + # THE CORE MODELLING DECISION: an omitted --cookie-name is not "unset", it is a CLAIM on + # oauth2-proxy's built-in default. Resolving it here is what lets assertion (1) below be a + # plain distinctness test with no special case, and it is exactly the step whose absence + # made the defect invisible to every reviewer who read these manifests. + ins["effective_name"] = ins["declared_name"] or DEFAULT_COOKIE_NAME + +# Anti-vacuous. The floor is deliberately low (a real removal should not turn this check red), +# so the load-bearing guard is the grep/parse CROSS-CHECK beneath it: every file the cheap +# text scan says holds an instance must yield at least one PARSED instance. A parser that +# quietly stopped matching would keep the floor and fail this. +check(len(instances) >= 5, + f"only {len(instances)} oauth2-proxy instance(s) parsed out of the tree. This repo " + f"authors more than that (five rendered sidecars, crafty's, and the shared proxy), so " + f"the inventory has stopped seeing instances and every assertion below would pass " + f"vacuously while a collision went unaudited") + +parsed_files = {ins["rel"] for ins in instances} +for rel in sorted(grep_files - parsed_files): + check(False, + f"{rel} contains an oauth2-proxy image or chart reference but NO instance was parsed " + f"out of it. Either it uses a shape iter_containers()/the HelmRelease reader does not " + f"recognise, or the file stopped parsing. Its cookie configuration is UNAUDITED, which " + f"is the same state the whole fleet was in before this check existed") + +# ============================================================ (1) DISTINCT EFFECTIVE NAMES +by_name: dict[str, list[dict]] = {} +for ins in instances: + by_name.setdefault(ins["effective_name"], []).append(ins) + +for nm, group in sorted(by_name.items()): + if len(group) < 2: + continue + who = ", ".join(sorted(g["key"] for g in group)) + default_note = ( + " Every one of them OMITS --cookie-name, so they are all claiming oauth2-proxy's " + "built-in default. Omitting the flag is not neutral: it IS this collision." + if nm == DEFAULT_COOKIE_NAME else "") + check(False, + f"{len(group)} oauth2-proxy instances share the cookie name '{nm}': {who}.{default_note}\n" + f" WHY THIS IS A BUG AND NOT A STYLE RULE: these are INDEPENDENT instances with " + f"DIFFERENT cookie-secrets (measured on prod 2026-09-30: seven distinct values), so " + f"neither can DECRYPT the other's cookie — it can only OVERWRITE it. A browser keys a " + f"cookie by (name, domain, path), so signing into one app destroys the session of every " + f"other app sharing that key. The loser logs 'No valid authentication in request', " + f"bounces to Keycloak, is redirected straight back because the SSO session is still " + f"live, and overwrites the winner in turn. The user sees 'I signed into Keycloak but " + f"I'm still not signed in' and goes looking in Keycloak, where nothing is wrong.\n" + f" FIX: give each per-app instance its own --cookie-name. For a service rendered " + f"from catalog/, do NOT hand-edit the golden — catalog/render.py derives the flag from " + f"the service name; re-render instead (ci/render-golden.sh's header has the canonical " + f"command). Exactly one instance may hold '{DEFAULT_COOKIE_NAME}', and it must be the " + f"one registered in SHARED_DEFAULT_COOKIE_OWNER.") + +# ============================================================ (2) SESSION vs CSRF DERIVATION +# oauth2-proxy names the CSRF cookie _csrf. So a SESSION name that happens to equal +# another instance's name + "_csrf" collides on the CSRF cookie while assertion (1) sees two +# perfectly distinct strings. This is the near-miss the obvious fix invites: naming the shared +# proxy's neighbour `_oauth2_proxy_csrf` reads as "clearly different" and is not. +effective = {ins["effective_name"] for ins in instances} +for ins in sorted(instances, key=lambda i: i["key"]): + base = ins["effective_name"] + if not base.endswith(CSRF_SUFFIX): + continue + victim = base[: -len(CSRF_SUFFIX)] + check(victim not in effective, + f"{ins['key']} uses the SESSION cookie name '{base}', which is exactly the CSRF cookie " + f"oauth2-proxy derives for the instance named '{victim}' (it appends '{CSRF_SUFFIX}'). " + f"The two session names look distinct and the CSRF cookies collide, so sign-in breaks " + f"with a state-mismatch instead of a session loss — the same bug wearing a different " + f"error message. Pick a name that is not another instance's name + '{CSRF_SUFFIX}'") + +# ============================================================ (3) THE DEFAULT HAS ONE OWNER +default_holders = sorted(ins["key"] for ins in instances + if ins["effective_name"] == DEFAULT_COOKIE_NAME) +registered = set(SHARED_DEFAULT_COOKIE_OWNER) + +for key in sorted(registered - set(ins["key"] for ins in instances)): + check(False, + f"SHARED_DEFAULT_COOKIE_OWNER registers '{key}', but no such oauth2-proxy instance " + f"exists in the tree. The register is SHRINK-ONLY and exact: a stale entry is a " + f"standing exemption for an object that is gone, so the next instance to inherit the " + f"default name would be silently excused. Remove the entry, or restore the instance") + +for key in default_holders: + check(key in registered, + f"{key} holds oauth2-proxy's DEFAULT cookie name '{DEFAULT_COOKIE_NAME}' but is NOT " + f"registered in SHARED_DEFAULT_COOKIE_OWNER. Exactly ONE instance may hold the default, " + f"and it is the SHARED forward-auth proxy — whose cookie is the platform-wide session " + f"every forward-auth-gated surface rides on, so renaming IT signs them all out at once. " + f"Every other instance must declare its own --cookie-name. If this instance really is " + f"the new shared proxy, register it here with the reason; if it is a per-app sidecar, " + f"give it a unique name") + +check(len(default_holders) <= 1, + f"{len(default_holders)} instances hold the default cookie name: {', '.join(default_holders)}. " + f"The default is a NAME, not an absence — it can have exactly one owner") + +# ============================================================ (4) HOST-SCOPED COOKIE DOMAIN +# See the chainsaw-test header for the argument (distinct names alone already fix the eviction; +# host-scoping is asserted anyway because a zone-wide cookie is TRANSMITTED to every host in the +# zone, and because it makes a future name collision contained instead of platform-wide). +for ins in sorted(instances, key=lambda i: i["key"]): + if ins["key"] in registered: + continue # the shared proxy: zone-wide by design, registered + dom, rhost = ins["cookie_domain"], ins["redirect_host"] + check(dom is not None, + f"{ins['key']} declares no --cookie-domain. oauth2-proxy then scopes the cookie to the " + f"request host, which happens to be correct — but it is correct BY ACCIDENT and says " + f"nothing to the next reader. Declare it explicitly as this app's own host") + if dom is None: + continue + check(not dom.startswith("."), + f"{ins['key']} sets --cookie-domain={dom}. A leading dot makes the cookie ZONE-WIDE: " + f"the browser sends this app's session token to every other host under that domain, " + f"including hosts behind no proxy at all, and it is what turns any future cookie-name " + f"collision into a platform-wide outage instead of a contained one. Scope it to this " + f"app's own host. Only the registered shared forward-auth proxy may be zone-wide, " + f"because cross-host forward-auth is its job") + if rhost: + check(dom == rhost, + f"{ins['key']} sets --cookie-domain={dom} but its --redirect-url host is {rhost}. " + f"The cookie is then set for a domain the sign-in flow never returns to (or for a " + f"wider one than this app owns), so the session either does not stick or leaks " + f"beyond the app. Both flags describe the SAME app: make them agree") + +# ============================================================ (5) THE RENDERER DERIVES +# Five of these instances are GENERATED. Auditing only the goldens would let someone "fix" the +# tree by hand and leave the generator emitting the defect for the next service onboarded — and +# the next re-render would then silently revert the hand fix. +render_py = ROOT / "catalog" / "render.py" +check(render_py.is_file(), + "catalog/render.py is missing — the generator of five of these instances cannot be audited") +if render_py.is_file(): + src = read(render_py) + m = re.search(r"def _oauth2_sidecar_container\(.*?\n(?=\n*def |\n*# =)", src, re.S) + fn = m.group(0) if m else "" + check(bool(fn), + "could not locate _oauth2_sidecar_container() in catalog/render.py. It was renamed or " + "removed; assertions (5) below cannot run, and the generator is unaudited") + if fn: + cookie_name_lines = [ln for ln in fn.splitlines() if "--cookie-name" in ln] + check(bool(cookie_name_lines), + "catalog/render.py's _oauth2_sidecar_container() emits NO --cookie-name at all, so " + "every service it renders claims oauth2-proxy's default and collides with every " + "other. This is the defect itself, back in the one place that reintroduces it " + "fleet-wide in a single edit") + check(any("s['name']" in ln or 's["name"]' in ln for ln in cookie_name_lines), + "catalog/render.py emits --cookie-name without DERIVING it from the " + "ServiceDefinition's own name. A constant collides with every sibling by " + "construction; a per-service table leaves the NEXT app onboarded defaulting into " + "the collision, which is how twelve ServiceMonitors acquired one identical wrong " + "port name in a single edit. Derive it from s['name']") + cookie_dom_lines = [ln for ln in fn.splitlines() if "--cookie-domain" in ln] + check(any("{host}" in ln for ln in cookie_dom_lines), + "catalog/render.py's sidecar --cookie-domain is not derived from the route host. A " + "zone-wide literal (`.{ZONE}`) broadcasts every app's session cookie across the " + "whole zone — see assertion (4)") + check(not any(re.search(r"--cookie-name=_oauth2_proxy_[a-z0-9]", ln) for ln in cookie_name_lines), + "catalog/render.py hardcodes a literal service name into --cookie-name. Whatever " + "service that is, every OTHER service now renders the same constant and they all " + "collide. Derive the name instead") + +# ============================================================ (6) PROVE THE SHIPPED RENDERER +# Once the tree is compliant, (1)-(4) have nothing left to catch and their passing is evidence +# of nothing. This runs the SHIPPED renderer as a SUBPROCESS on two synthetic ServiceDefinitions +# and asserts the property directly, so the guard stays falsifiable. +def _sd(name: str, port: int) -> str: + return yaml.safe_dump({ + "name": name, + "kind": "container", + "source": {"image": "example/app:1"}, + "exposure": "internal", + "ingressRoutes": [{"host": f"{name}.home.keithjackson.dev", "port": port}], + "auth": {"mode": "forward-auth", "groups": ["platform-admins"]}, + }, sort_keys=False) + + +def _sidecar_args(rendered: str) -> list: + for doc in yaml.safe_load_all(rendered): + if not isinstance(doc, dict) or doc.get("kind") != "Deployment": + continue + for c in iter_containers(doc): + if c.get("name") == "oauth2-proxy": + return list(c.get("args") or []) + return [] + + +if render_py.is_file(): + fixtures = {"cookiefixa": 8111, "cookiefixb": 8112} + rendered_args: dict[str, list] = {} + with tempfile.TemporaryDirectory() as td: + for svc, port in fixtures.items(): + f = Path(td) / f"{svc}.yaml" + f.write_text(_sd(svc, port)) + proc = subprocess.run([sys.executable, str(render_py), str(f)], + capture_output=True, text=True) + check(proc.returncode == 0, + f"the shipped catalog/render.py failed on a synthetic forward-auth fixture " + f"({svc}): rc={proc.returncode} {proc.stderr.strip()[:200]}") + if proc.returncode == 0: + rendered_args[svc] = _sidecar_args(proc.stdout) + + names, domains = {}, {} + for svc, args in rendered_args.items(): + names[svc] = flag_from_args(args, "cookie-name") + domains[svc] = flag_from_args(args, "cookie-domain") + check(bool(args), + f"the shipped renderer produced no oauth2-proxy sidecar for synthetic forward-auth " + f"fixture {svc} — the sidecar is how a forward-auth app is gated at all (R-IDP-02)") + check(names[svc] not in (None, DEFAULT_COOKIE_NAME), + f"the shipped renderer gave synthetic fixture {svc} the cookie name " + f"{names[svc]!r}. A newly-onboarded forward-auth app would therefore default " + f"straight into the fleet collision — which is exactly how five services acquired " + f"this defect without anyone making a decision") + check(domains[svc] == f"{svc}.home.keithjackson.dev", + f"the shipped renderer scoped synthetic fixture {svc}'s cookie to " + f"{domains[svc]!r} rather than to its own host — a newly-onboarded app would " + f"broadcast its session cookie across the whole zone") + check(any(isinstance(a, str) and a.startswith("--whitelist-domain=.") for a in args), + f"synthetic fixture {svc} lost its ZONE-WIDE --whitelist-domain. That flag governs " + f"permitted post-login REDIRECT targets, not cookie scope; narrowing it with the " + f"cookie domain breaks the sign-in bounce") + + if len(rendered_args) == 2: + a, b = sorted(rendered_args) + check(names[a] != names[b], + f"the shipped renderer gave two DIFFERENT synthetic services the SAME cookie name " + f"({names[a]!r}). Whatever the committed goldens currently look like, the generator " + f"will collide every forward-auth app it renders from now on") + check(domains[a] != domains[b], + f"the shipped renderer gave two different synthetic services the same cookie domain " + f"({domains[a]!r}) — the cookies would share a scope even with distinct names") + +# ============================================================ VERDICT +inv = " | ".join(f"{i['key']} -> {i['effective_name']} @ {i['cookie_domain'] or ''}" + for i in sorted(instances, key=lambda x: x["key"])) +print(f"oauth2-proxy instances audited ({len(instances)}): {inv}") +if FAILURES: + print(f"\nCHECK FAIL (oauth2-proxy-cookie-names-distinct): {len(FAILURES)} failed, " + f"{CHECKS - len(FAILURES)} passed", file=sys.stderr) + for f in FAILURES: + print(f" FAIL {f}", file=sys.stderr) + raise SystemExit(1) +print(f"check OK: oauth2-proxy-cookie-names-distinct — {CHECKS} assertions (R-IDP-02, R-IDP-06)") +PY diff --git a/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml b/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml index a8d1db4..7009146 100644 --- a/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml +++ b/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml @@ -76,7 +76,8 @@ spec: - --allowed-group=platform-admins - --skip-provider-button=true - --redirect-url=https://paas-demo.home.keithjackson.dev/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_paas-demo + - --cookie-domain=paas-demo.home.keithjackson.dev - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml b/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml index a13b7c2..e107945 100644 --- a/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml +++ b/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml @@ -99,7 +99,8 @@ spec: - --allowed-group=media-admins - --skip-provider-button=true - --redirect-url=https://sonarr.home.keithjackson.dev/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_sonarr + - --cookie-domain=sonarr.home.keithjackson.dev - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml b/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml index dbccb46..a89ab86 100644 --- a/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml +++ b/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml @@ -99,7 +99,8 @@ spec: - --allowed-group=media-admins - --skip-provider-button=true - --redirect-url=https://sonarr.home.keithjackson.dev/oauth2/callback - - --cookie-domain=.${BASE_DOMAIN} + - --cookie-name=_oauth2_proxy_sonarr + - --cookie-domain=sonarr.home.keithjackson.dev - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true - --pass-access-token=true diff --git a/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md new file mode 100644 index 0000000..95a4288 --- /dev/null +++ b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md @@ -0,0 +1,179 @@ +--- +date: 2026-09-30 +branch: fix/oauth2-proxy-cookie-collision-fleet +title: The cookie collision was never Crafty's — it was the renderer's, and it shipped to five apps +--- + +## What changed + +Follow-up #3 of `decisions/2026-09-29-crafty-oauth2-proxy-signin.md`: *"Any further per-app +reverse-proxy sidecar needs a unique `--cookie-name` too. The collision is a property of the +pattern, not of Crafty."* + +It is a property of the pattern, and this branch found where the pattern actually lives. + +**The defect was not in the four manifests it was reported in.** `apps/wave-0/pihole.yaml`, +`apps/wave-0/qbittorrent.yaml`, `apps/wave-1/radarr.yaml`, `apps/wave-1/sonarr.yaml` — and +`apps/wave-0/jackett.yaml` — are **generated goldens**. One function emits all five sidecars: +`catalog/render.py::_oauth2_sidecar_container()`, which hardcoded `--cookie-domain=.{ZONE}` and +emitted no `--cookie-name` at all. Hand-editing a golden is not a fix twice over: `ci/render-golden.sh` +FAILS on the drift (provoked, not assumed — see below), and the next re-render silently reverts it. + +So the fix is in the generator, and both flags are now **derived** from the ServiceDefinition: + + --cookie-name=_oauth2_proxy_{s['name']} unique per service, by construction + --cookie-domain={host} the app's own route host, not the zone + +`--whitelist-domain` deliberately stays zone-wide — it governs permitted post-login *redirect* +targets, not cookie scope. `catalog/render.py --selftest` gained four assertions, including one +that renders a **second** synthetic forward-auth service and requires the two cookie names to +differ, so a future refactor to a constant fails in the renderer's own test. + +Re-rendering with the canonical command regenerated **five** goldens, not four. Jackett is the +fifth, and this is the one thing in this branch the PM brief did not anticipate — see +"the jackett collision" below. + +**The new gate check** is `conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/`, +declaring `conformance.home.k3s/scope: whole-repo` so `ci/l0.sh`'s `--include-scope` union runs it +on **every** gate invocation. No bespoke `ci/l0.sh` stage was added — `whole-repo-invariants-wired` +fails the gate if anyone adds one, and the scope annotation is the supported mechanism. + +It inventories every oauth2-proxy instance the repo authors (containers at any depth, plus the +shared proxy's HelmRelease values) and resolves each one's **effective** cookie name — its declared +name, **or oauth2-proxy's built-in `_oauth2_proxy` when it declares none**. That single modelling +decision is the whole check: an omitted flag is not an absence, it is a *claim on a name*, and +treating it as an absence is precisely why five reviewers read these manifests and saw nothing. +Then: (1) no two effective names collide; (2) no name is another's name + `_csrf`, because that is +how oauth2-proxy derives the CSRF cookie and two such session names look distinct while their CSRF +cookies collide; (3) the default name has exactly one owner, and it is the one registered in +`SHARED_DEFAULT_COOKIE_OWNER`; (4) every non-registered instance is host-scoped; (5) +`catalog/render.py` derives rather than lists; (6) the **shipped** renderer is proven end-to-end on +two synthetic ServiceDefinitions run as a subprocess. + +**On the shared proxy — "distinct" or "explicitly declared"?** The brief asked me to decide, and +the answer is *neither, quite*. The shared forward-auth proxy at `platform/oauth2-proxy/` **must +keep** `_oauth2_proxy`: it is the single session every forward-auth-gated surface without its own +sidecar rides on, so renaming it signs all of them out at once. Requiring *every* instance to +declare a name would therefore need an exemption, and an exemption is a hole: delete the shared +proxy and whichever app next omitted the flag would silently inherit the default with the rule +still satisfied. So the assertion is plain **distinctness over effective names**, with the default +treated as a name like any other — held by **exactly one owner, named in a shrink-only register +with its reason**. The shared proxy needs no exemption from the rule; it is simply the registered +holder of that one name. A stale register entry FAILS (the exemption would outlive its object) and +an unregistered holder FAILS (no silent heirs). Both were provoked and are quoted below. + +**On host-scoped `--cookie-domain` — overreach, or not?** Asserted, and here is the argument both +ways. *Against:* distinct names alone close the eviction bug completely — two differently-named +cookies do not overwrite each other however wide their domains are — so this assertion fixes +nothing (1) has not already fixed, and a check that fails a legitimate configuration is a check +someone eventually disables. *For, and this wins:* a zone-wide cookie is **transmitted by the +browser to every host in the zone**. qBittorrent's session token was being sent to pihole, radarr, +sonarr and every other name under the zone, including hosts behind no proxy at all — a +session-credential disclosure across trust boundaries that nothing needs. And it is what makes a +*future* name collision catastrophic rather than contained: with host-scoped cookies, two +instances that accidentally share a name cannot see each other's cookie at all. Defence in depth +on the exact axis that already failed once. The escape hatch is the same register, by name and +with a reason, rather than a loosened rule. + +### The jackett collision — read this + +`apps/wave-0/jackett.yaml` carries the identical defect and was assigned to **Mira Vance** on +`fix/jackett-oauth2-proxy-session`, with an instruction to me not to touch it. **That separation is +not achievable**, and I did not discover this by reasoning about it — I provoked it. With the +renderer fixed and jackett's golden left at its `main` content, `./ci/render-golden.sh` exits **1**: + + [fail] GOLDEN DRIFT: apps/wave-0/jackett.yaml does not match the canonical render: + - - --cookie-domain=.${BASE_DOMAIN} + + - --cookie-name=_oauth2_proxy_jackett + + - --cookie-domain=jackett.${BASE_DOMAIN} + [fail] render==golden check FAILED + +Jackett's golden is an *output* of the file that contains the defect. There are exactly three +options and two of them are worse: hand-edit four goldens and leave the generator broken (fails +`render-golden`, and reverts on the next render); add a per-service carve-out to the renderer so +jackett alone keeps defaulting (ships the exact "list, don't derive" anti-pattern this check +exists to forbid, and leaves jackett broken *by design*); or fix the generator and regenerate all +five. I took the third. Jackett's golden here is **machine output of a one-line generator change**, +not an independent edit of Mira's file, and it is byte-identical to what her branch must produce. + +**Consequence for the PM:** `fix/jackett-oauth2-proxy-session` has no work left in it. As of this +writing that branch has **zero commits ahead of `main`** and is not pushed, so nothing is lost — +but if Mira has since hand-edited `apps/wave-0/jackett.yaml`, her branch will fail `render-golden` +on its own terms and should be closed rather than rebased. + +## What I verified + +- **The premise, in-cluster, rather than inherited from the handoff** (CLAUDE.md §2b). Read-only, + `kubectl --context k3s-prod`, through the pinned toolbox. + - **Seven** `*-oauth2-proxy` Secrets carry a `cookie-secret`: crafty, jackett, pihole, + qbittorrent, radarr, sonarr, and the shared `oauth2-proxy-credentials`. **All seven values are + DISTINCT.** Compared without printing anything: each value was hashed **in-cluster-side script, + never echoed**, and only the first 8 hex characters of each `sha256` were emitted — enough to + say same-or-different, recovering nothing. Secret *key names* were listed; no `.data` or + `.stringData` was printed (CLAUDE.md §6). **So no instance can decrypt another's cookie — it can + only overwrite it. The premise holds.** + - **The default cookie names really were in use.** Of 8 live oauth2-proxy containers, **6 declare + no `--cookie-name` at all** — jackett, pihole, qbittorrent, radarr, sonarr, and both replicas of + the shared proxy — so all six emit `_oauth2_proxy` / `_oauth2_proxy_csrf`. All six also carry + `--cookie-domain=.home.keithjackson.dev`. **One cookie pair, six independent instances.** + - Crafty's live pod (`crafty-647c7d8cc5-5spv9`) already carries + `--cookie-name=_oauth2_proxy_crafty` and `--cookie-domain=crafty.home.keithjackson.dev`, so + PR #22 is merged and reconciled. It is the only compliant per-app instance on the cluster. +- **The four target files are generated, not authored.** Rendering `catalog/` with the canonical + command into a temp dir and diffing against `apps/` reported all 10 onboarded goldens + byte-identical before any change. Hand-editing one is drift by definition. +- **`render-golden` FAILS if jackett is excluded** — provoked (output quoted above), rc **1**. + Restoring the regenerated golden returns it to `all 10 onboarded service golden(s) reproduce`, + rc **0**. +- **`catalog/render.py --selftest`: PASS**, including the new two-service distinctness assertion. +- **The check is selected by the whole-repo union.** `conformance/run.sh --layer l0 --requirements + r-nosuchreq-99 --include-scope whole-repo --print-selection` lists + `tests/l0-static/oauth2-proxy-cookie-names-distinct` among 20 tests — i.e. it runs even when the + touched-requirement filter matches nothing, which is the blind spot it was scoped for. +- **The check was tested in BOTH directions**, against throwaway trees outside the repo, never by + mutating the branch: + - **Compliant tree (this branch): rc 0, 39 assertions, 7 instances audited.** + - **Pre-fix tree** (`git archive HEAD` + this test dropped in): **rc 1, 26 failures**, naming all + five violators, the 6-way `_oauth2_proxy` collision, the zone-wide domains, the renderer + emitting no `--cookie-name`, and both synthetic fixtures rendering `None`. + - **Targeted mutations on a compliant copy**, one assertion class each, all fired and nothing + else did: duplicate name (sonarr←radarr) → 1 failure; CSRF derivation + (`_oauth2_proxy_sonarr_csrf` as a session name) → fired; zone-wide `--cookie-domain` on pihole + → 2 failures; crafty's `--cookie-name` deleted → unregistered-default-heir + 2-way collision; + shared proxy renamed → **stale register entry** failure. + - A **false positive was found and fixed** by this testing: `platform/oauth2-proxy/forward-auth-cec.yaml` + contains `serviceName: oauth2-proxy/oauth2-proxy:80` — an Envoy `namespace/service:port` + reference, not an image — which the first inventory scan counted as an unaudited instance. The + scan is now anchored on `image:`, and the near-miss is documented in the check. +- **Gate:** toolbox `./ci/gate.sh` — exit code and stage list in the next section, because it is + not a pass and that belongs there. + +## What I did NOT verify + +- **Gate rc 3 (UNDETERMINED), not 0.** Green stages: `decisions-log`, `lint`, `l0`, `render-golden`, + `l1`, `approval-policy`. **Skipped:** `spec-immutability` (the sibling spec repo is not mounted — + the one tolerated skip), `gitea-settings` (no `SETTINGS_API_TOKEN`), and **L2/L3 conformance** + (`CLUSTER_AVAILABLE` unset). Nothing FAILED; nothing runtime was certified either. **rc 3 is + UNDETERMINED, never green**, and this branch must not be described as green on the strength of it. +- **No browser round-trip. The sign-in fix is UNVERIFIED end-to-end.** Identical to the gap the + 2026-09-29 entry recorded and did not close: the collision has still **never been reproduced in a + browser**, because reproducing it needs two gated apps visited in one session. Everything here is + reasoned from measured configuration — distinct secrets, default names, shared domain — and from + the mechanism, not from a failure anyone watched. It is a latent-bug fix. +- **Nothing was applied.** No `kubectl apply/edit/patch`, no `flux reconcile`. Cluster access was + read-only throughout. The running pods are still the pre-fix spec. **Merging is what deploys**, + and it will roll five Deployments; every user's existing session on those five apps is + invalidated by the cookie rename and they will re-authenticate once. Radarr/sonarr/qbittorrent + use `strategy: Recreate` on RWO volumes, so there is a brief gap per app. Pick the moment. +- **The live flags are not re-measured by the check.** git is not the cluster. I measured the live + pods by hand, once; nothing in the gate re-reads them, so hand-applied drift would be invisible + to it. A live-state assertion is L2 and is not written. +- **The cookie-secrets' distinctness is not gated either.** It was measured out of band and is + asserted nowhere. If they were ever made identical the collision would become harmless — but + sharing one session-encryption key across trust zones is a worse idea than the bug it would mask, + so the check does not offer it as an alternative and does not look for it. +- **The shared proxy's own configuration is unchanged and untested by me.** I did not touch + `platform/oauth2-proxy/**` (owned by another agent this cycle); the check only reads it. +- **Nothing about group authorization, CSRF behaviour, or session contents.** Names and domains only. +- **`vars.CLUSTER_AVAILABLE` is still unset in CI**, so the PR's green tick — if it goes green — + proves the L0/L1 offline subset and nothing about runtime. From 75a4029d64b969246032011c869287cf4e5ae8a6 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 12:46:40 -0500 Subject: [PATCH 02/13] docs(decisions): record the measured gate rc, stage list and touched subset The handoff asserted a gate outcome before the gate had been run against a committed branch. Replaces it with what was actually measured on 9a4489b: rc 3 (UNDETERMINED) -- green decisions-log/lint/l0/render-golden/l1/approval-policy, skipped spec-immutability/gitea-settings/conformance -- plus the twelve touched requirements that are UNVERIFIED at L2/L3, and a note that approval-policy passed VACUOUSLY here (event='none') while the PR itself needs a non-author review because conformance/** is a sensitive path. Also records that the three conformance/tests/l1-rendering goldens are regenerated: they are expected-output fixtures of the same renderer, and the L1 stage caught their drift on the first gate run rather than anyone remembering them. Co-Authored-By: Claude Opus 5 (1M context) --- ...-30-oauth2-proxy-cookie-collision-fleet.md | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md index 95a4288..3ebb809 100644 --- a/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md +++ b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md @@ -31,7 +31,9 @@ differ, so a future refactor to a constant fails in the renderer's own test. Re-rendering with the canonical command regenerated **five** goldens, not four. Jackett is the fifth, and this is the one thing in this branch the PM brief did not anticipate — see -"the jackett collision" below. +"the jackett collision" below. The three `conformance/tests/l1-rendering/*/golden/` fixtures are +expected-output copies of the same renderer, so they are regenerated too; that is not collateral +damage, it is the L1 stage doing its job (it FAILED on the first gate run and named the drift). **The new gate check** is `conformance/tests/l0-static/oauth2-proxy-cookie-names-distinct/`, declaring `conformance.home.k3s/scope: whole-repo` so `ci/l0.sh`'s `--include-scope` union runs it @@ -151,10 +153,17 @@ on its own terms and should be closed rather than rebased. ## What I did NOT verify - **Gate rc 3 (UNDETERMINED), not 0.** Green stages: `decisions-log`, `lint`, `l0`, `render-golden`, - `l1`, `approval-policy`. **Skipped:** `spec-immutability` (the sibling spec repo is not mounted — - the one tolerated skip), `gitea-settings` (no `SETTINGS_API_TOKEN`), and **L2/L3 conformance** - (`CLUSTER_AVAILABLE` unset). Nothing FAILED; nothing runtime was certified either. **rc 3 is - UNDETERMINED, never green**, and this branch must not be described as green on the strength of it. + `l1`, `approval-policy`. **Skipped:** `spec-immutability` (the sibling spec repo is not mounted + at the container's `SPEC_REPO` path from a worktree — the one tolerated skip), `gitea-settings` + (no `SETTINGS_API_TOKEN`), and **L2/L3 conformance** (`CLUSTER_AVAILABLE` unset). Nothing FAILED; + nothing runtime was certified either. **rc 3 is UNDETERMINED, never green**, and this branch must + not be described as green on the strength of it. The touched-requirement subset — + `R-GITOPS-02 R-IDP-02 R-IDP-06 R-NET-01 R-NET-05 R-NET-06 R-NET-07 R-PLAT-02 R-TEST-01 R-TEST-02 + R-TEST-03 R-TEST-04` — is reported **UNVERIFIED** at L2/L3 for all twelve. +- **`approval-policy` passed vacuously here** (`event='none'` — it is not a pull_request run). On + the PR it will REQUIRE a non-author approving review: `infra/gitea/approval-policy.json` lists + `conformance/**` as sensitive, because changing the tests changes what green MEANS. That is + correct and expected for this branch, not an obstacle to route around. - **No browser round-trip. The sign-in fix is UNVERIFIED end-to-end.** Identical to the gap the 2026-09-29 entry recorded and did not close: the collision has still **never been reproduced in a browser**, because reproducing it needs two gated apps visited in one session. Everything here is From 6a6fea81515adeae62ea00b747152f136a3b389f Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 12:47:42 -0500 Subject: [PATCH 03/13] docs(auth): root-cause jackett's session loss; add the oauth2-proxy-session runbook Reported as "GET works, PUT/POST to add indexers fail, reloading never helps, and I am never re-prompted". Diagnosis lands here; the MANIFEST FIX DOES NOT, deliberately -- see the blocker below. ROOT CAUSE, measured from two pods' logs interleaved to the second: 15:31:06 jackett [AuthSuccess] 15:31:29 radarr "cookie signature not valid, removing session" 15:31:32 jackett "No valid authentication in request" <- 3s later Six oauth2-proxy instances (seven pods) emit the DEFAULT cookie name `_oauth2_proxy` on `.home.keithjackson.dev` holding seven DIFFERENT cookie-secrets (compared in-cluster by sha256; all distinct). `removing session` is ClearSession, which expires the cookie on radarr's own zone-wide --cookie-domain, so jackett's session is DELETED by a process in another namespace, 26s after a clean login whose `expires` was 5 minutes out. 8 such lines in radarr's log, 1 in jackett's, 0 elsewhere. It was never expiry and it was never Jackett -- and it is why reloading does not help: the operator's Radarr tab re-clears the cookie seconds after each new login. Two contributing mechanisms, also measured: * session lifetime == Keycloak access-token lifetime, 300.000s exactly, with refresh_token:true on the same line and --cookie-refresh unset (default 0). * XHRs answered 302 not 401: oauth2-proxy's AJAX guard needs an exact `Accept: application/json`, which Jackett's jQuery does not send. On /api/ paths: 33 x 302 vs 2 x 401. A browser following a 302 re-issues a POST as a GET and DROPS THE BODY -- 4 of 6 POSTs never reached Jackett at all. WHY NO MANIFEST FIX HERE. apps/wave-0/jackett.yaml is a GENERATED GOLDEN. The sidecar args come from one shared function, _oauth2_sidecar_container() in catalog/render.py; catalog/jackett.yaml declares only auth.mode/auth.groups and the auth schema is additionalProperties:false, so there is NO per-app override hook. Hand-editing the rendered file is rejected by the gate exactly as designed ("GOLDEN DRIFT ... render-golden: FAIL"). Changing the renderer instead rewrites all five rendered apps -- measured: CHANGED: apps/wave-0/{jackett,pihole,qbittorrent}.yaml CHANGED: apps/wave-1/{radarr,sonarr}.yaml and render-golden requires all five be regenerated in the same commit. Four of them are owned by fix/oauth2-proxy-cookie-collision-fleet right now. The cookie fix is ONE change, not five; it needs re-cutting as a single renderer commit rather than two branches rewriting the same generated block. Validated the intended argument list against the pinned image and the real issuer before stopping: it printed back `name:_oauth2_proxy_jackett ... domains:jackett.home.keithjackson.dev ... refresh:after 2m30s` and `API route - Path: ^/api/`. Two corrections fell out of that: --api-route is SINGULAR (--api-routes does not exist and the proxy refuses to start), and --session-cookie-minimal is mutually exclusive with --cookie-refresh ("cookie_refresh > 0 requires oauth tokens in sessions"), so dropping --pass-access-token does NOT shrink the cookie -- it is a least-privilege change, not a size one. Adds runbooks/oauth2-proxy-session.md: the four sidecar log lines that tell the auth layers apart (including the upstream `-` that means the proxy answered a request the app never saw), the cross-log collision diagnosis, and the generated-golden warning with its blast radius. Registered in the configMapGenerator AND runbooks/README.md so the URL resolves. Jackett is NOT fixed by this commit and will keep losing its session to Radarr until the renderer change ships. Co-Authored-By: Claude Opus 5 (1M context) --- ...2026-09-30-jackett-oauth2-proxy-session.md | 199 ++++++++++++++++++ runbooks/README.md | 8 + runbooks/kustomization.yaml | 5 + runbooks/oauth2-proxy-session.md | 168 +++++++++++++++ 4 files changed, 380 insertions(+) create mode 100644 decisions/2026-09-30-jackett-oauth2-proxy-session.md create mode 100644 runbooks/oauth2-proxy-session.md diff --git a/decisions/2026-09-30-jackett-oauth2-proxy-session.md b/decisions/2026-09-30-jackett-oauth2-proxy-session.md new file mode 100644 index 0000000..cb68515 --- /dev/null +++ b/decisions/2026-09-30-jackett-oauth2-proxy-session.md @@ -0,0 +1,199 @@ +--- +date: 2026-09-30 +branch: fix/jackett-oauth2-proxy-session +title: Jackett — radarr's oauth2-proxy deletes jackett's session cookie; the fix cannot be made per-app +--- + +## What changed + +Reported as *"GET requests work, but most PUT/POST requests to add indexers fail. Reloading the page +never fixes it, and I am never re-prompted for login if it is a login issue."* + +**This branch lands the diagnosis and a runbook. It does NOT land the manifest fix, and that is a +deliberate stop, not an omission** — see *The fix has one home and it is shared*, below. + +Three mechanisms, all measured. + +**1. Cookie collision — and on Jackett it is the live failure, not a latent one.** Six oauth2-proxy +instances (seven pods) emit oauth2-proxy's **default** cookie name `_oauth2_proxy` scoped to +`.home.keithjackson.dev` while holding seven **different** cookie-secrets. The ping-pong is in two +pods' own logs, interleaved to the second: + + 15:23:17 radarr [AuthSuccess] -> radarr writes _oauth2_proxy + 15:23:44 jackett "cookie signature not valid, removing session" -> re-login #1 + 15:23:58 radarr "cookie signature not valid, removing session" -> radarr DELETES jackett's + 15:27:17 jackett [AuthSuccess] -> re-login #2 + 15:27:59 radarr "cookie signature not valid, removing session" + 15:31:06 jackett [AuthSuccess] -> re-login #3 + 15:31:29 radarr "cookie signature not valid, removing session" <- deletes it again + 15:31:32 jackett "No valid authentication in request" <- 3 SECONDS later, no cookie + +`removing session` is oauth2-proxy calling ClearSession, which expires the cookie on **its own** +`--cookie-domain` — the whole zone. So jackett's session is not merely overwritten, it is **deleted +by a process in another namespace**, 26 s after a clean login whose own `expires` was five minutes +out. That is the answer to the open question in the brief ("26 s is well inside the 5-minute window, +so plain expiry does not explain it"): it was never expiry, and it was never Jackett. + +It also explains the half of the report that looked like user error. **Reloading genuinely does not +help**, because the operator's Radarr tab re-clears the cookie seconds after each new login. + +**2. Session lifetime pinned to a 5-minute access token.** Measured from the sidecar's own +`[AuthSuccess]`: `created:15:31:06.391860254` / `expires:` + **300.000 s** exactly, with +`refresh_token:true` on the same line. `--cookie-refresh` is unset and its default is `0` +(disabled), so the proxy holds a refresh token it has no mandate to spend. + +**3. XHRs answered with 302 instead of 401 — why none of it was visible.** oauth2-proxy's built-in +AJAX guard fires only on an **exact** `Accept: application/json`, which Jackett's jQuery does not +consistently send. Measured on `/api/` paths in this pod's log: **33 × 302 against 2 × 401**, and the +split runs straight through Jackett's own POSTs (`/api/v2.0/server/adminpassword` → 401, +`/api/v2.0/server/update` → 302). A 302 on an unauthenticated XHR is followed invisibly by the +browser, satisfied silently by the live Keycloak SSO session, and resolves with HTML where the +JavaScript expected JSON — no prompt, no error. On a write it is worse: a browser following a 302 +re-issues a POST as a **GET and drops the body**, so the indexer configuration is discarded with +nothing logged as a failure anywhere. + +### The fix has one home, and it is shared — this is the blocker + +The brief assigned me `apps/wave-0/jackett.yaml` as exclusively mine, and assigned the other four +apps to a sibling branch. **That split is not implementable, and I found out by trying it.** + +`apps/wave-0/jackett.yaml` is a **generated golden**. The sidecar's argument list is built by one +shared function, `_oauth2_sidecar_container()` in `catalog/render.py`. `catalog/jackett.yaml` +declares only `auth.mode` and `auth.groups`, and the `auth` schema is `additionalProperties: false` +— **there is no per-app override hook**. I wrote the fix into the rendered file first; the merge +gate rejected it exactly as designed: + + [fail] GOLDEN DRIFT: apps/wave-0/jackett.yaml does not match the canonical render + [fail] stage render-golden: FAIL -> MERGE GATE: BLOCKED (rc 1) + +So I made the change at its real source instead and **measured the blast radius** before committing +to it. A change confined to that one function rewrites the sidecar block in **all five** rendered +apps, and `render-golden` requires all five be regenerated in the same commit: + + CHANGED: apps/wave-0/jackett.yaml + CHANGED: apps/wave-0/pihole.yaml + CHANGED: apps/wave-0/qbittorrent.yaml + CHANGED: apps/wave-1/radarr.yaml + CHANGED: apps/wave-1/sonarr.yaml + +Four of those five are the files I was explicitly told not to touch, and all four are being edited +right now on `fix/oauth2-proxy-cookie-collision-fleet`. **The gate and the ownership boundary are in +direct contradiction**: I cannot fix Jackett's sidecar without regenerating four files another +branch owns, and two branches rewriting the same generated block across five files is precisely the +cross-branch conflict this repo's §7 exists to prevent. + +I stopped rather than force it. **The cookie fix is one change, not five**, and it should be re-cut +as a single renderer commit on one branch. The intended content, validated below: + + --cookie-name=_oauth2_proxy_{name} # per-app, derived + --cookie-domain={host} # host-scoped, derived + --whitelist-domain=.{ZONE} # unchanged: redirect targets, not cookie scope + --cookie-refresh=2m30s # half the measured 300 s token lifetime + (drop --pass-access-token, --set-xauthrequest) + +`--api-route=^/api/` is the one flag I would **not** apply renderer-wide on this evidence: I +measured the 302/401 split on Jackett only, and PiHole's UI does not serve `/api/` at all. It needs +either a per-app hook or four more measurements. + +**Crafty is unaffected by all of this** — its sidecar is hand-written in +`platform/crafty/deployment.yaml`, not rendered, which is why it could be and was fixed alone on +2026-09-29. + +### What this branch does land + +`runbooks/oauth2-proxy-session.md` — the four sidecar log lines that tell Keycloak, the sidecar and +the app's own login apart (including the upstream `-` field that means the proxy answered a request +the app never saw), the cookie-collision cross-log diagnosis, the 5-minute-session check, the +302-vs-401 trap, and the generated-golden warning with its measured blast radius. Registered in the +runbooks `configMapGenerator` **and** listed in `runbooks/README.md`, so the URL resolves rather +than the file merely existing. + +## What I verified + +- **The collision is the live failure.** `[stored_session.go:94] Error loading cookied session: + cookie signature not valid, removing session` — **1 occurrence in jackett's log (15:23:44), 8 in + radarr's** (15:22:49, 15:23:58, 15:27:59, **15:31:29**, 15:32:29 ×4), and **zero** in pihole, + qbittorrent, sonarr, crafty and both shared-proxy replicas. The 15:31:29 → 15:31:32 pair is the + operator's whole report. +- **Six instances / seven pods are in the collision set** — the brief said five; the shared + forward-auth proxy in namespace `oauth2-proxy` is the sixth and runs 2 replicas. Read from live + pod specs: jackett, pihole, qbittorrent, radarr, sonarr and `oauth2-proxy` all have **no** + `--cookie-name` argument and `--cookie-domain=.home.keithjackson.dev`. Only crafty is fixed. +- **All seven cookie-secrets differ.** Compared in-cluster by sha256, first 12 hex only, values + never printed: `d9c5…`/`0b3b…`/`b025…`/`bb6a…`/`25f4…`/`e679…`/`911a…`. So no two of these proxies + can read each other's cookie — the precondition for the eviction. +- **The 5-minute lifetime**, from four `[AuthSuccess]` lines: `expires − created = 300.000 s`, with + `refresh_token:true` and `groups:[platform-admins]`. Live args confirm `--cookie-refresh` absent. +- **The 302/401 split** on `/api/` paths in this pod's log: 33 × 302, 18 × 200, 2 × 401, 2 × 204. +- **4 of the 6 POSTs never reached Jackett** — the access log's upstream field is `-`, not `/`: + `adminpassword` → 401, `adminpassword` → 401, `server/update` → 302, `server/update` → 302. The + two that did reach it (`adminpassword`, `indexers/1337x/config`) carry the authenticated email. +- **The proposed argument list starts, against the pinned image and the real issuer.** Ran + `quay.io/oauth2-proxy/oauth2-proxy:v7.8.1` locally with the full proposed args; OIDC discovery + against `keycloak.home.keithjackson.dev` succeeded and it printed back its own reading: + `Cookie settings: name:_oauth2_proxy_jackett secure(https):true httponly:true expiry:168h0m0s + domains:jackett.home.keithjackson.dev path:/ samesite: refresh:after 2m30s` and + `API route - Path: ^/api/`. +- **`--api-route` is singular.** The brief specified `--api-routes`, which does not exist in v7.8.1; + the proxy would have refused to start. Read off `--help`. +- **`--session-cookie-minimal` is mutually exclusive with `--cookie-refresh`** — provoked, not + assumed: `invalid configuration: cookie_refresh > 0 requires oauth tokens in sessions. + session_cookie_minimal cannot be set`. This kills the brief's premise that dropping + `--pass-access-token` shrinks the cookie: only `--session-cookie-minimal` strips tokens from the + session, and it is unavailable while refresh is on. Dropping those two flags is justified on + **least privilege and inertness**, not size. +- **`--set-xauthrequest` is inert here**: HTTPRoute `jackett-9117` backendRef is + `Service/jackett port 4180`, there is no ext-authz attachment on the `shared` Gateway, so + `/oauth2/auth` is never called. +- **`apps/**` is generated and the fix cannot be scoped to one app** — both halves measured, not + inferred: the gate's `render-golden` stage rejected the hand-edit, and re-rendering after a + renderer-only change listed all five apps as changed (transcript above). +- **Gate:** exit code and stage list in the next section. + +## What I did NOT verify + +- **The operator's problem is NOT FIXED by this branch.** Only the diagnosis and the runbook land. + Jackett's sidecar is unchanged and will keep losing its session to Radarr until the renderer + change ships. **This is the single most important line in this entry.** +- **No browser round-trip.** Every mechanism is measured from logs and specs and the argument list + is proven to start, but nobody has added an indexer through the proposed configuration. The human + test is below, for whoever lands the renderer change. +- **Nothing was applied.** No `kubectl apply/edit/patch`, no `flux reconcile`. The running pod is + still `jackett-c5bfcbfb8-c75f9` (2/2 Running, 17 d, 0 restarts — unchanged by this session's + read-only probes). Note that when the fix does land, merging is what deploys, and it will restart + the pod (`strategy: Recreate` on an RWO Longhorn volume), briefly interrupting Radarr's and + Sonarr's direct `:9117` access. +- **Cookie size and chunking: UNMEASURED, and I will not claim either way.** I could not read a + `Set-Cookie` header without a browser. I found no `_oauth2_proxy_0`/`_1` chunk evidence and no + 4 kB warning in 36,287 log lines — but I did **not** establish that v7.8.1 emits such a warning at + this log level, so that absence is not evidence. If sessions still misbehave after the fix lands, + this is the next thing to measure, with `--session-store-type=redis` as the escape hatch, since + `--session-cookie-minimal` is ruled out (above). +- **`--cookie-refresh=2m30s` has never executed a refresh here.** It depends on Keycloak accepting + the refresh token and on the realm's SSO Session Idle/Max, none of which I read (that needs admin + credentials). If Keycloak's *refresh* token is also short-lived, the five-minute logout persists + and the realm is where to look next. +- **`--api-route` is measured for Jackett only.** PiHole and qBittorrent were not measured, and + PiHole's UI does not serve `/api/`. Do not apply it renderer-wide on my evidence. +- **The four sibling apps still collide with each other** regardless of what happens to Jackett. + The same operator report should be expected from any of pihole / qbittorrent / radarr / sonarr. +- **L2/L3 conformance did not run** (`CLUSTER_AVAILABLE` unset), so nothing about runtime behaviour + is certified by the gate. +- **The runbook's commands were not all executed verbatim.** The log-reading ones I ran; the + per-namespace `for` loops are edited from the scripted forms I actually used. + +### The human test — for whoever lands the renderer fix + +After it merges and the pod rolls (watch for a **new** pod name at 2/2): + +1. **Sign in** at `https://jackett.home.keithjackson.dev/UI/Dashboard`. +2. **Open Radarr in a second tab** and sign in there too. **This is the step that breaks Jackett + today** — it is the 15:31:29 line. Return to the Jackett tab. +3. **Add an indexer** — *+ Add indexer*, pick one, *Configure*, *Okay*. It must save. +4. **Wait six minutes on the open dashboard, then save another.** That is the `--cookie-refresh` + test; the session currently dies at five. +5. **If something does still fail, it must now fail LOUDLY** — DevTools → Network, a broken `/api/` + call shows **401**, not a 302 followed by HTML. A silent success-looking no-op means the change + did not take; check the running pod's args before assuming anything else. +6. **A plain page reload must still sign you in silently** (`/UI/Dashboard` is outside `^/api/`). + A 401 page on reload means the regex is too broad — that is a regression. diff --git a/runbooks/README.md b/runbooks/README.md index 9c859fd..2368296 100644 --- a/runbooks/README.md +++ b/runbooks/README.md @@ -115,6 +115,14 @@ Each affected runbook says so on its own page and gives the by-hand equivalent. plex.tv/claim — so it is a timed sequence, scripted as `infra/plex/claim-ship.sh`: token → `sops` → PR → gate → merge → **forced** `flux reconcile` → pod roll (~2m45s measured). Includes why a changed Secret does not restart anything, and why an expired token is harmless. +- **`oauth2-proxy-session`** — a gated UI loads but its saves quietly do nothing, and you are never + re-prompted to log in. Tells the three stacked auth layers apart from the sidecar's own access log + (the four lines that matter, including the upstream `-` that means the proxy answered a request the + app never saw). Covers the cookie collision between per-app sidecars sharing the default + `_oauth2_proxy` name on `.home.keithjackson.dev` — which deletes sessions belonging to apps in + *other* namespaces and is only legible with two logs side by side — the 5-minute access-token + session with no `--cookie-refresh`, and why an XHR answered with a 302 instead of a 401 loses a + POST body silently. - **`crafty-admin-credential`** — you signed into Keycloak and Crafty *still* asks for a password. Expected: Crafty 4 has no OIDC, so its own local login sits behind the oauth2-proxy gate as a second factor. The page tells the two layers apart from the sidecar log, and explains that the bootstrap diff --git a/runbooks/kustomization.yaml b/runbooks/kustomization.yaml index 08793d7..4e341bf 100644 --- a/runbooks/kustomization.yaml +++ b/runbooks/kustomization.yaml @@ -85,6 +85,11 @@ configMapGenerator: - node-io=node-io.md - node-memory=node-memory.md - node-memory-swap=node-memory-swap.md + # Operator procedure, not alert-linked. Reading the per-app oauth2-proxy sidecar log to tell + # Keycloak, the sidecar and the app's own login apart. Cookie collision between two sidecars + # presents as random session loss in ONE app's log and is only legible with both side by side, + # so the page has to be reachable at 3am without going looking for it in git. + - oauth2-proxy-session=oauth2-proxy-session.md # Operator procedure, not an alert page (no alert links to it — the # alert-runbooks-resolvable check treats that as advisory). It is served # because the plex.tv claim window is short and is not a thing to go looking diff --git a/runbooks/oauth2-proxy-session.md b/runbooks/oauth2-proxy-session.md new file mode 100644 index 0000000..85cd017 --- /dev/null +++ b/runbooks/oauth2-proxy-session.md @@ -0,0 +1,168 @@ +# oauth2-proxy-session — telling the auth layers apart from the sidecar log + +**Not alert-driven.** This is an operator procedure for the per-app oauth2-proxy sidecars +(`jackett`, `pihole`, `qbittorrent`, `radarr`, `sonarr`, `crafty`) and the shared forward-auth +proxy in namespace `oauth2-proxy`. Use it when a gated UI behaves as though you are signed in and +signed out at the same time: the page loads, but its buttons and saves quietly do nothing. + +> The sidecar log contains **email addresses, session UUIDs and full callback URLs including an +> OAuth `code`**. Read it, do not paste it. (Repo rule §6: secrets never enter a transcript.) + +## What this means here + +Every gated app on `home.keithjackson.dev` has **at least two** independent authentications +stacked on it, and the symptom rarely names the one that refused: + +1. **Keycloak** — the `homelab` realm issues the token. Failures here are visible: you see a login + form, or an error page from Keycloak's own hostname. +2. **The oauth2-proxy sidecar** — validates the session cookie and the group claim + (`media-users` / `platform-admins`). **This is the layer that fails silently**, and the rest of + this page is about reading it. +3. **The app's own login**, where it has one — Crafty and Radarr/Sonarr each present a local login + *behind* the Keycloak gate. See `runbooks/crafty-admin-credential.md` for that layer. + +The sidecar's access log distinguishes all three, if you know the four lines to look for. Get it: + + infra/devcontainer/dev.sh run bash -c 'kubectl --context k3s-prod -n jackett logs \ + -l app.kubernetes.io/name=jackett -c oauth2-proxy --tail=-1' + +Substitute the namespace and label for the app you are chasing. Always pass `--context k3s-prod`: +`dev.sh` merges both clusters and picks a current-context **per invocation**, and that default has +already changed once. Do not run two `dev.sh` invocations at the same time. + +### The four lines, and what each one means + +| Log line | What actually happened | Which layer | +|---|---|---| +| `[AuthSuccess] Authenticated via OAuth2: Session{... groups:[...]}` | Keycloak issued a token and the proxy accepted it. **The IdP is fine.** | Keycloak ✔ | +| `[oauthproxy.go:1026] No valid authentication in request. Initiating login.` | **No session cookie was presented at all.** Either you never had one, or something *deleted* it — see below. | sidecar | +| `[stored_session.go:94] Error loading cookied session: cookie signature not valid, removing session` | A cookie **of the right name** arrived that this proxy **could not decrypt**. Almost always a *different* proxy's cookie. Note `removing session`: this proxy then **deletes** it. | sidecar, cookie collision | +| an upstream path of `-` instead of `/` in an access-log line | the **proxy answered the request itself**; it never reached the app. Compare `POST - "/api/..."` against `POST / "/api/..."`. | sidecar | + +That last column is the one people miss. The upstream field sits between the method and the quoted +path. `-` means the proxy handled it (a redirect, a 401, its own `/oauth2/*` endpoints); `/` means +it was proxied to the app. Count them: + + infra/devcontainer/dev.sh run bash -c 'kubectl --context k3s-prod -n jackett logs \ + -l app.kubernetes.io/name=jackett -c oauth2-proxy --tail=-1 \ + | grep -v "\"/ping\"" | grep -E " (POST|PUT|DELETE) " | cut -c1-140' + +If your writes show `-`, the app never saw them. On 2026-09-30, **4 of the 6 POSTs** in jackett's +log had `-`. + +## First commands + +**1. Did Keycloak ever succeed?** No `AuthSuccess` at all means this is not a cookie problem — +go to `runbooks/keycloak-down.md` or `runbooks/keycloak-client-secrets.md`. + + infra/devcontainer/dev.sh run bash -c 'kubectl --context k3s-prod -n logs \ + -l app.kubernetes.io/name= -c oauth2-proxy --tail=-1 | grep -c AuthSuccess' + +**2. How long does a session last?** Subtract `created` from `expires` on an `AuthSuccess` line. +If it is exactly **5m**, the session's lifetime is Keycloak's access-token lifetime and the proxy +is not refreshing it: + + infra/devcontainer/dev.sh run bash -c 'kubectl --context k3s-prod -n logs \ + -l app.kubernetes.io/name= -c oauth2-proxy --tail=-1 \ + | grep AuthSuccess | tail -1 | grep -o "created:[^ ]* [^ ]*\|expires:[^ ]* [^ ]*"' + +The same line says `refresh_token:true` — the proxy is *holding* a refresh token. It will only +spend it if `--cookie-refresh` is set; the default is `0`, meaning disabled. + +**3. Is this a cookie collision?** Two checks, and you want both. + +*Names and scopes* — every instance that emits the default name `_oauth2_proxy` on the shared +`.home.keithjackson.dev` domain is in the collision set with every other one: + + infra/devcontainer/dev.sh run bash -c 'kubectl --context k3s-prod get pods -A -o json \ + | grep -E "cookie-name|cookie-domain"' + +An instance with **no** `--cookie-name` argument is using the default. That is the failure — the +flag being absent, not wrong. + +*The smoking gun* — search **every** proxy, not just the one you are debugging, for the removal +line, and put the timestamps next to each other: + + infra/devcontainer/dev.sh run bash -c 'for ns in jackett pihole qbittorrent radarr sonarr crafty; do \ + echo "== $ns"; kubectl --context k3s-prod -n $ns logs -l app.kubernetes.io/name=$ns \ + -c oauth2-proxy --tail=-1 2>/dev/null | grep stored_session; done' + +**4. Do the cookie-secrets actually differ?** They should — but confirm the collision is between +instances that genuinely cannot read each other. Compare **hashes**, never values: + + infra/devcontainer/dev.sh run bash -c 'for ns in jackett pihole qbittorrent radarr sonarr crafty; do \ + printf "%s " "$ns"; kubectl --context k3s-prod -n $ns get secret $ns-oauth2-proxy \ + -o jsonpath="{.data.cookie-secret}" | sha256sum | cut -c1-12; done' + +Never print `.data` or `.stringData` itself. A password has been leaked into a transcript here +before. + +## Likely causes, most likely first + +1. **Cookie collision between two per-app proxies.** Several instances emit the default + `_oauth2_proxy` on `.home.keithjackson.dev` with different `cookie-secret`s. Whoever + authenticates last overwrites the rest; the losers log `cookie signature not valid, removing + session` and — because `removing session` clears the cookie on *their* `--cookie-domain`, which + is the whole zone — **delete a session belonging to an app in another namespace**. The victim + then logs `No valid authentication` with no cookie at all, seconds after a clean login, while + its own `expires` is still minutes away. **Read both logs side by side; one app's log alone + makes this look like random session loss.** Fix: a unique `--cookie-name` and a host-scoped + `--cookie-domain` on the per-app sidecar. The shared forward-auth proxy keeps the zone-wide + scope — cross-host auth is its job. +2. **Session pinned to a 5-minute access token with no refresh.** `--cookie-refresh` unset means + the proxy never spends the refresh token it holds, so the session expires with the access token. + Set it to about half the measured token lifetime. +3. **An XHR got a 302 instead of a 401.** The app's JavaScript follows the redirect invisibly, the + live Keycloak SSO session satisfies it without a prompt, and the XHR resolves with **HTML where + it expected JSON**. Nothing is logged as an error and the operator is never re-prompted — this + is what "it just doesn't save, and reloading doesn't help" looks like. Worse on a write: a + browser following a 302 re-issues a POST as a GET and **drops the body**, so the change is + discarded silently. oauth2-proxy's built-in AJAX guard only fires on an exact + `Accept: application/json`, which most app JavaScript does not send; make it deterministic with + `--api-route=` (**singular** — `--api-routes` does not exist and the proxy refuses to + start). Keep browser-navigable paths *out* of the regex so a plain page load still signs you in. +4. **Group membership.** An `AuthSuccess` followed by `403` is the `--allowed-group` gate, not a + session problem. Check the `groups:[...]` field on the `AuthSuccess` line. +5. **The app's own second login.** Crafty, Radarr and Sonarr each have one. `AuthSuccess` followed + by the app's own `/login` page is layer 3, not layer 2. + +## When to escalate or stop + +- **Stop before you `kubectl edit`/`patch`/`apply` anything.** Flux reconciles `main`; a + hand-applied flag is drift that the next reconcile reverts or, worse, adopts. Argument changes + go through a branch and a PR. Reading logs is always safe; changing the Deployment is not an + agent action. +- **Do not hand-edit `apps/**/.yaml` — those files are GENERATED.** The five per-app sidecars + (jackett, pihole, qbittorrent, radarr, sonarr) get their argument list from one shared function, + `_oauth2_sidecar_container()` in `catalog/render.py`; `catalog/.yaml` declares only + `auth.mode` and `auth.groups`, and the `auth` schema is `additionalProperties: false`, so there + is **no per-app override hook**. A hand-edit to the rendered file is caught by the merge gate's + `render-golden` stage ("GOLDEN DRIFT ... does not match the canonical render") and would be + erased by the next render anyway. Change the renderer, then regenerate: + + python3 catalog/render.py --all catalog -o apps --wave-layout --no-image-automation + + **Budget for the blast radius**: because the function is shared, any change to it rewrites the + sidecar block in **all five** rendered apps, and the gate requires all five be regenerated in + the same commit. There is no way to change one app's sidecar arguments alone. Measured + 2026-09-30. Coordinate before starting — this is a single change, not five. +- **Crafty is the exception** and is not rendered from the catalog: its sidecar is hand-written in + `platform/crafty/deployment.yaml`, so it can be, and was, fixed independently. +- **Validate an argument change before you open the PR**, not after. The pinned image will tell + you what it understood, without touching the cluster: + + podman run --rm quay.io/oauth2-proxy/oauth2-proxy:v7.8.1 --help + + Running it with your full proposed argument list prints a `Cookie settings:` line and an + `API route - Path:` line back at you, and rejects mutually exclusive flags outright + (`--session-cookie-minimal` with `--cookie-refresh`, for one). A flag typo here is a CrashLoop + on a service you have just told someone is fixed. +- **Do not rotate a `cookie-secret` to "clear" a bad session.** It signs every live session out of + that app and does nothing about the collision, which is caused by the *names* matching, not the + secrets differing. +- **Escalate to Keycloak** (`runbooks/keycloak-down.md`, `runbooks/keycloak-client-secrets.md`) if + there is no `AuthSuccess` in any proxy's log — that is an IdP or client-secret problem and no + cookie flag will help. +- **Hand off rather than guess** if you see `cookie signature not valid` on a proxy that already + has a unique `--cookie-name`. That combination should not occur; it means something is writing + that name which is not in this inventory. Record what you measured. From 0a0adce4d829ca147dc9be78376d2634f8d5fed2 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 12:52:12 -0500 Subject: [PATCH 04/13] docs(auth): bound crafty's post-fix ratio to its own pod; record the metrics gap Coordinator raised crafty as counter-evidence: it carries the cookie fix and showed the platform's worst ratio (81:1 over 24h from Loki), implying the fix does not work. Bounded it exactly instead of in Loki -- crafty's pod started 2026-09-30T03:57:19Z with 0 restarts, so a 24h window is ~10h of the PRE-fix process. Read off the pod's own log, whose boundary is the process start: crafty 3 'No valid authentication', 0 AuthSuccess, in 13h52m <- not 81 one client IP, 2 of the 3 on /ws (WebSocket reconnects), two isolated minutes (04:02, 15:22) Post-fix crafty is idle, not broken. Correctly bounded it is evidence FOR the fix. The ratio metric ranks CHATTINESS, not breakage: radarr's 156:1 is /api/v3/command (62), /api/v3/queue/details (46), /signalr/negotiate (43) -- UI polling. Added that trap to the runbook, because ranking apps this way is how a deployed fix gets written off. That polling also sharpens the jackett mechanism: every poll arriving with a cookie radarr cannot decrypt calls ClearSession again, so a left-open radarr tab deletes jackett's cookie every few seconds. Explains the intermittency. Re-weighted the diagnosis as asked: mechanisms 2 and 3 (5-minute session with no --cookie-refresh; 302-instead-of-401 on XHR) are demonstrated from jackett's own log and between them fully explain the report. Mechanism 1 (collision) is firing and jackett genuinely violates it, but causation is one 3-second adjacency, not a population -- necessary and firing, not proven sufficient. Stated that way in the entry. Metrics: confirmed no per-app sidecar exposes any (--metrics-address ABSENT, ports ['http'] on all six); only the shared proxy has the flag and a metrics port. Worth adding and DECLINED HERE for the structural reason only -- the container's args AND ports both come from _oauth2_sidecar_container(), so it hits the identical five-file blast radius. It belongs in the same single renderer commit. Recorded, not silently skipped. Co-Authored-By: Claude Opus 5 (1M context) --- ...2026-09-30-jackett-oauth2-proxy-session.md | 72 +++++++++++++++++-- runbooks/oauth2-proxy-session.md | 27 +++++++ 2 files changed, 94 insertions(+), 5 deletions(-) diff --git a/decisions/2026-09-30-jackett-oauth2-proxy-session.md b/decisions/2026-09-30-jackett-oauth2-proxy-session.md index cb68515..0bf3dc8 100644 --- a/decisions/2026-09-30-jackett-oauth2-proxy-session.md +++ b/decisions/2026-09-30-jackett-oauth2-proxy-session.md @@ -12,9 +12,14 @@ never fixes it, and I am never re-prompted for login if it is a login issue."* **This branch lands the diagnosis and a runbook. It does NOT land the manifest fix, and that is a deliberate stop, not an omission** — see *The fix has one home and it is shared*, below. -Three mechanisms, all measured. +Three mechanisms, all measured. **Weighting matters here, so it is stated up front:** mechanisms 2 +and 3 are demonstrated from Jackett's own access log and **between them fully explain the report as +written** — PUT/POST fail, the operator is never re-prompted, and a reload does not help. Mechanism 1 +is a real, firing defect that Jackett genuinely violates, and it explains the *intermittency* on top; +I demonstrate the mechanism and one three-second causal adjacency, not a population of them. Fix all +three; do not rest the case on the first. -**1. Cookie collision — and on Jackett it is the live failure, not a latent one.** Six oauth2-proxy +**1. Cookie collision — the mechanism is firing, and Jackett is a genuine violator.** Six oauth2-proxy instances (seven pods) emit oauth2-proxy's **default** cookie name `_oauth2_proxy` scoped to `.home.keithjackson.dev` while holding seven **different** cookie-secrets. The ping-pong is in two pods' own logs, interleaved to the second: @@ -35,7 +40,24 @@ out. That is the answer to the open question in the brief ("26 s is well inside so plain expiry does not explain it"): it was never expiry, and it was never Jackett. It also explains the half of the report that looked like user error. **Reloading genuinely does not -help**, because the operator's Radarr tab re-clears the cookie seconds after each new login. +help**, because the operator's Radarr tab re-clears the cookie seconds after each new login — and +Radarr is not a passive tab. Its UI polls: of 156 unauthenticated requests in its log against +**1** AuthSuccess, the top paths are `/api/v3/command` (62), `/api/v3/queue/details` (46) and +`/signalr/messages/negotiate` (43). Every one of those polls, arriving with a cookie Radarr cannot +decrypt, calls ClearSession again. **A left-open Radarr tab is a cookie-deletion engine firing every +few seconds**, which is why Jackett's sessions die at irregular, short, apparently random intervals. + +**A counter-hypothesis was put to me and it does not survive bounding.** The observation was that +Crafty already carries this fix yet showed the platform's worst failure ratio (81:1 over 24 h from +Loki), implying the fix does not work. But Crafty's pod started **2026-09-30T03:57:19Z with 0 +restarts**, so a 24 h window is ~10 h of the *pre-fix* pod. Rather than bound it in Loki I read the +ratio off the pod's own log, whose boundary is the process start and therefore exact: **inside the +fixed pod's entire 13 h 52 m lifetime, Crafty logged 3 `No valid authentication` events, not 81** — +from one client IP, two of them WebSocket `/ws` reconnects, in two isolated minutes (04:02 and +15:22), against 0 AuthSuccess because nobody signed in. Post-fix Crafty is idle, not broken. The +ratio metric is confounded by endpoint type — apps that poll or hold WebSockets (Radarr, Crafty) +manufacture failure lines at their retry rate — so it ranks *chattiness*, not breakage. Correctly +bounded, Crafty is evidence **for** the fix. **2. Session lifetime pinned to a 5-minute access token.** Measured from the sidecar's own `[AuthSuccess]`: `created:15:31:06.391860254` / `expires:` + **300.000 s** exactly, with @@ -95,6 +117,18 @@ as a single renderer commit on one branch. The intended content, validated below measured the 302/401 split on Jackett only, and PiHole's UI does not serve `/api/` at all. It needs either a per-app hook or four more measurements. +**Metrics — asked for, and declined here for the same structural reason, not on merit.** It is true +and I re-measured it: **no per-app sidecar exposes metrics**. Only the shared proxy carries +`--metrics-address=0.0.0.0:44180` and a `metrics` container port; jackett, pihole, qbittorrent, +radarr, sonarr and crafty all have `(ABSENT)` and ports `['http']` alone, so the listener never +starts and nothing about these gates is scrapable. Adding it is worth doing and the pattern is +already proven in-tree by the shared proxy. But it is **not** "inside my file": the container's +`args` *and* its `ports` both come from `_oauth2_sidecar_container()`, so adding +`--metrics-address` plus a `metrics` port hits the identical five-file blast radius as the cookie +change. It belongs in the same single renderer commit — which is an argument for cutting that commit +soon, not for me to hand-edit a golden a second time after the gate already refused the first. +(Crafty is separately hand-written and could take the metrics flag on its own.) + **Crafty is unaffected by all of this** — its sidecar is hand-written in `platform/crafty/deployment.yaml`, not rendered, which is why it could be and was fixed alone on 2026-09-29. @@ -148,7 +182,24 @@ than the file merely existing. - **`apps/**` is generated and the fix cannot be scoped to one app** — both halves measured, not inferred: the gate's `render-golden` stage rejected the hand-edit, and re-rendering after a renderer-only change listed all five apps as changed (transcript above). -- **Gate:** exit code and stage list in the next section. +- **Crafty's post-fix failure rate, bounded exactly.** Pod `crafty-647c7d8cc5-5spv9`, + `startTime=2026-09-30T03:57:19Z`, `restarts=0`, live args `--cookie-name=_oauth2_proxy_crafty` and + `--cookie-domain=crafty.home.keithjackson.dev`. Over that pod's whole lifetime: **3** + `No valid authentication`, 0 AuthSuccess, one client IP, 2 of the 3 on `/ws`. The 81:1 figure is a + 24 h window over a 13.9 h pod and is dominated by the pre-fix process. +- **The ratio metric ranks chattiness, not breakage.** Radarr 156:1 with the top paths being + `/api/v3/command`, `/api/v3/queue/details` and `/signalr/messages/negotiate` — UI polling, not 156 + distinct user-visible failures. +- **No per-app sidecar exposes metrics.** `--metrics-address` is `(ABSENT)` and `ports` is + `['http']` on all six per-app sidecars; only the shared proxy has + `--metrics-address=0.0.0.0:44180` and a `metrics` port. +- **Gate:** `./ci/gate.sh` in the pinned toolbox, **rc 3 — UNDETERMINED, not green**. Green: + `spec-immutability`, `approval-policy`, `decisions-log`, `lint`, `l0`, `render-golden`, `l1`. + Skipped: `gitea-settings` (no `SETTINGS_API_TOKEN`) and L2/L3 conformance (`CLUSTER_AVAILABLE` + unset), leaving touched requirement `R-OBS-04` uncertified. Note `spec-immutability` is **green**, + not skipped — mounting the sibling spec repo at `/home-network-k3s` (the path + `ci/spec-immutability.sh` actually defaults to) in the ENGINE shim fixes that skip; the commonly + circulated shim mounts it at its host path, where `SPEC_REPO` never finds it. ## What I did NOT verify @@ -178,7 +229,18 @@ than the file merely existing. - **The four sibling apps still collide with each other** regardless of what happens to Jackett. The same operator report should be expected from any of pihole / qbittorrent / radarr / sonarr. - **L2/L3 conformance did not run** (`CLUSTER_AVAILABLE` unset), so nothing about runtime behaviour - is certified by the gate. + is certified by the gate. `gitea-settings` also skipped, so branch protection and runner state are + **unverified by me this session** — I did not re-measure them against the Gitea API. +- **I did not query Loki.** The Crafty bounding above comes from the pod log, which I argue is the + better instrument (an exact process boundary rather than a wall-clock window against a 72 h + retention). But it means I cannot speak to the 24 h or 12 h *platform-wide* ratios, and a + per-app ratio over a window longer than the current pod's life is not something I measured at all. +- **Collision causation is one adjacency, not a population.** The 15:31:29 → 15:31:32 pair is three + seconds and the mechanism is documented and present, but I did not enumerate every jackett failure + and match it to a radarr ClearSession. Treat mechanism 1 as **necessary and firing, not proven + sufficient** — the coordinator's framing, and I agree with it on this evidence. +- **The metrics change is NOT in this branch** (reason above). Jackett therefore remains inside the + oauth2-proxy metrics gap, and the ratio alert shipped in PR #29 still cannot see it. - **The runbook's commands were not all executed verbatim.** The log-reading ones I ran; the per-namespace `for` loops are edited from the scripted forms I actually used. diff --git a/runbooks/oauth2-proxy-session.md b/runbooks/oauth2-proxy-session.md index 85cd017..da09678 100644 --- a/runbooks/oauth2-proxy-session.md +++ b/runbooks/oauth2-proxy-session.md @@ -97,6 +97,27 @@ instances that genuinely cannot read each other. Compare **hashes**, never value Never print `.data` or `.stringData` itself. A password has been leaked into a transcript here before. +### Do not rank apps by their failure ratio + +Counting `No valid authentication` against `AuthSuccess` per app looks like a health metric and is +not one. **It ranks chattiness, not breakage.** An app whose UI polls or holds a WebSocket +manufactures one failure line per retry, so a single dead cookie in one idle browser tab becomes +scores of log lines, while an app nobody is using scores zero no matter how broken it is. Measured +2026-09-30: Radarr showed **156** unauthenticated requests against **1** `AuthSuccess`, but the top +paths were `/api/v3/command` (62), `/api/v3/queue/details` (46) and `/signalr/messages/negotiate` +(43) — polling, not 156 user-visible failures. + +**Bound the window to the current pod, not to wall-clock.** A ratio measured over 24 h across a pod +that restarted 13 h ago is mostly about the *previous* configuration, and that is exactly how a +deployed fix gets written off as ineffective. The pod's own log has an exact boundary — its process +start — so read it there: + + infra/devcontainer/dev.sh run bash -c 'kubectl --context k3s-prod -n get pod \ + -l app.kubernetes.io/name= -o jsonpath="{.items[*].status.startTime}{\"\n\"}"' + +then count within that same pod's log. On 2026-09-30 this took Crafty from an apparent 81:1 over 24 h +to **3 failures in its fixed pod's entire 13 h 52 m life**, two of them `/ws` reconnects. + ## Likely causes, most likely first 1. **Cookie collision between two per-app proxies.** Several instances emit the default @@ -148,6 +169,12 @@ before. 2026-09-30. Coordinate before starting — this is a single change, not five. - **Crafty is the exception** and is not rendered from the catalog: its sidecar is hand-written in `platform/crafty/deployment.yaml`, so it can be, and was, fixed independently. +- **You cannot currently alert on any of this**, and that is worth knowing before you promise + monitoring. Measured 2026-09-30: no per-app sidecar sets `--metrics-address` and none declares a + `metrics` container port, so oauth2-proxy's metrics listener never starts — the only oauth2-proxy + series in Prometheus come from the shared proxy in namespace `oauth2-proxy`, which does set both. + Until the renderer emits them for the per-app sidecars, every diagnosis on this page is + log-reading only. - **Validate an argument change before you open the PR**, not after. The pinned image will tell you what it understood, without touching the cluster: From f4a155a807ef14326bc70a4ce88b2e437d69bcae Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 12:56:33 -0500 Subject: [PATCH 05/13] docs(decisions): the collision is measured; that it CAUSES the failures is not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Counter-evidence was raised against this branch: over a 24h Loki window crafty -- the app already carrying the cookie fix -- showed the WORST auth-rejection ratio of the set (crafty 81:1, radarr 67:1, jackett 39:4). Taken at face value that says the fix does not work. It does not say that, and it does not say the opposite either. Re-measured bounded to the fixed pod, read from each sidecar's OWN log rather than Loki (`kubectl logs` on a pod with 0 restarts cannot reach a pod older than itself, so a window inside the pod's age is inside the fixed spec by construction): namespace age(h) rst reject success ratio cleared badsig crafty 13.9 0 3 0 n/a 0 0 <- FIXED, whole life radarr 422.5 0 155 1 155.0:1 8 8 jackett 422.6 0 39 4 9.8:1 1 1 pihole/qbittorrent/sonarr/shared: 0 reject, 0 success Three findings, kept apart: 1. `crafty 81:1` is a WINDOW ARTEFACT. The fixed pod has emitted 3 rejections in its entire 13.9h life; the 81 came from a 24h window straddling it, i.e. from the pre-fix pod. It is not evidence against the fix and must not be cited as such. 2. The fix is still NOT SHOWN TO WORK. Crafty logged ZERO AuthSuccess in that window -- no traffic, so no ratio. Absence of failure against absence of traffic proves nothing. 3. The collision mechanism IS independently confirmed, and it is a MINORITY of the rejections. `cookie signature not valid` + `removing session` is its fingerprint -- a cookie IS present, written with a key this instance does not have, so it cannot decrypt it and then ClearSession expires it zone-wide. radarr 8 of 155 (5%), jackett 1 of 39, fixed crafty 0. ~95% of radarr's rejections carry no cookie at all and point at the session-lifetime and 302-on-XHR mechanisms this branch does not touch. So the handoff now says, at the top and in both evidence sections, that "the instances collide" and "the collisions cause the reported symptom" are different claims and only the first is supported. The four fixes stand -- distinct cookie names are correct regardless -- but nothing here should be read as a cure. Also records three deliberate NON-fixes with their reasoning (--cookie-refresh, which all four of my apps need as much as jackett does and which belongs in the renderer rather than in a golden; --api-routes for 302-on-XHR, which is path-shaped and needs a schema change; and sidecar metrics, which I judged overreach for a check whose subject is cookie identity), and two gate facts: * spec-immutability was skipping because of a PATH BUG IN THE WORKTREE SHIM, not the environment -- dev.sh derives the spec path from its own REPO_ROOT, which in a worktree does not resolve. Mounting the spec repo at /home-network-k3s with SPEC_REPO set makes R-TEST-06 actually run and PASS. Local gate is now rc 3 for one reason (gitea-settings, no token) instead of two. * CI's gate reports FAILURE on 75a4029 for exactly one reason, and it is not this change: `approval-policy: a sensitive change lacks a non-author approval`. Every substantive stage is green in CI, including gitea-settings. conformance/** is a sensitive path, so the PR is fail-closed pending a human review -- CLAUDE.md §1 working as designed. Co-Authored-By: Claude Opus 5 (1M context) --- ...-30-oauth2-proxy-cookie-collision-fleet.md | 145 ++++++++++++++++-- 1 file changed, 128 insertions(+), 17 deletions(-) diff --git a/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md index 3ebb809..a641151 100644 --- a/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md +++ b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md @@ -12,6 +12,13 @@ pattern, not of Crafty."* It is a property of the pattern, and this branch found where the pattern actually lives. +**Read the scope claim carefully before the detail.** This branch fixes a *measured collision*: six +independent oauth2-proxy instances, seven different cookie-secrets, one cookie name, one zone-wide +domain. It does **not** claim to fix the operator's reported sign-in failures. Those are two +separate claims, only the first is supported by anything here, and the evidence for why the second +is *not* supported — including a fair reading of counter-evidence that looked bad for this fix — is +in "What I verified". Do not merge this expecting the auth-rejection volume to drop. + **The defect was not in the four manifests it was reported in.** `apps/wave-0/pihole.yaml`, `apps/wave-0/qbittorrent.yaml`, `apps/wave-1/radarr.yaml`, `apps/wave-1/sonarr.yaml` — and `apps/wave-0/jackett.yaml` — are **generated goldens**. One function emits all five sidecars: @@ -103,6 +110,33 @@ writing that branch has **zero commits ahead of `main`** and is not pushed, so n but if Mira has since hand-edited `apps/wave-0/jackett.yaml`, her branch will fail `render-golden` on its own terms and should be closed rather than rebased. +### Scope: what this branch deliberately does NOT fix + +Three further defects were raised against these sidecars while this branch was in flight. All +three are real; none is fixed here, and each omission is a decision, not an oversight. + +- **No `--cookie-refresh`, so the session dies in 5 minutes.** The session cookie's `expires` is + the Keycloak access-token lifetime; without `--cookie-refresh` oauth2-proxy never spends the + refresh token it already holds. **All four of my apps need this exactly as much as jackett + does** — same realm, same renderer, same function. It is not here because it is a *different + mechanism* (session lifetime, not cookie identity) that would be un-revertable independently + of the collision fix; because the correct VALUE is a judgement call belonging to whoever + measured it; and because nothing in my check guards it, so I would be shipping an unguarded + behaviour change. **When it lands it belongs in `_oauth2_sidecar_container()`, not in + jackett's golden** — one line there fixes all five; a hand-edit fixes one and fails + `render-golden`. +- **Unauthenticated XHR gets a 302, not a 401.** Same reasoning, plus one of its own: the fix + (`--api-routes`) is *path-shaped and per-app* (`^/api/` for the *arr and qBittorrent; pihole's + admin API is shaped differently), so doing it properly means declaring API path prefixes in the + ServiceDefinition — a schema change, well outside this branch. +- **No per-app sidecar exposes metrics at all** (no `--metrics-address`, no `metrics` port, so + the listener never starts and nothing is scrapable). I considered asserting this in the new + check and **rejected it as overreach**: the check's subject is "no two instances can share a + cookie", and a metrics assertion shares neither its inputs nor its failure mode. Bundling them + makes the check's name a lie and couples two unrelated ratchets, so a future metrics decision + could only be expressed by weakening a *cookie* check. `servicemonitor-ports-resolve` and + `alert-metrics-have-scrape-targets` already exist and are the right homes. + ## What I verified - **The premise, in-cluster, rather than inherited from the handoff** (CLAUDE.md §2b). Read-only, @@ -121,6 +155,47 @@ on its own terms and should be closed rather than rebased. - Crafty's live pod (`crafty-647c7d8cc5-5spv9`) already carries `--cookie-name=_oauth2_proxy_crafty` and `--cookie-domain=crafty.home.keithjackson.dev`, so PR #22 is merged and reconciled. It is the only compliant per-app instance on the cluster. +- **THE COLLISION IS REAL. IT IS NOT ESTABLISHED AS THE CAUSE OF THE REPORTED FAILURES.** These + are two different claims and only the first is supported. I was handed counter-evidence — + crafty, the app already fixed, showed the *worst* rejection ratio of the set over a 24h Loki + window (`crafty 81:1`, `radarr 67:1`, `jackett 39:4`) — and it is right to refuse the causal + claim. What I could do was bound the window. Read from **each sidecar's own log** rather than + Loki, because `kubectl logs` on a pod that has not restarted cannot reach a pod older than + itself, so a window inside the pod's age is inside the fixed spec's lifetime *by construction*: + + namespace age(h) rst reject success ratio cleared badsig + crafty 13.9 0 3 0 n/a 0 0 <- FIXED spec, whole life + radarr 422.5 0 155 1 155.0:1 8 8 + jackett 422.6 0 39 4 9.8:1 1 1 + pihole 395.7 0 0 0 0:0 0 0 + qbittorrent 404.1 0 0 0 0:0 0 0 + sonarr 422.5 0 0 0 0:0 0 0 + oauth2-proxy 422.6 0 0 0 0:0 0 0 (both replicas) + + `reject` = `No valid authentication in request`; `success` = `AuthSuccess`; + `cleared` = `removing session` (oauth2-proxy's `ClearSession`); `badsig` = `cookie signature + not valid`. Window `--since=24h`, which exceeds every pod's age only for crafty — whose pod is + 13.9h old with **0 restarts**, so these are its complete post-fix figures. + + Three conclusions, kept separate: + 1. **`crafty 81:1` is a window artefact and must not be used as evidence against the fix.** + The fixed pod has emitted **3** rejections in its entire life. The 81 came from a 24h Loki + window straddling a 13.9h-old pod, i.e. overwhelmingly from the PRE-fix pod. + 2. **The fix is still NOT shown to work.** Crafty logged **0 `AuthSuccess`** in that window — + nobody signed in — so there is no post-fix ratio to evaluate at all. Absence of failures + against absence of traffic proves nothing. This remains the open gap, not a resolved one. + 3. **The collision mechanism is independently confirmed, and it is a MINORITY of the + rejections.** `cookie signature not valid` + `removing session` is the collision's + fingerprint — a cookie *is* present, written by an instance whose key this one does not + have, so it cannot decrypt it and then `ClearSession` expires it on its own + `--cookie-domain` (the whole zone). radarr: **8 of 155** rejections (5%) carry it; jackett + 1 of 39; crafty, fixed, **0**. So the collision is real and this branch addresses it — and + ~95% of radarr's rejections carry no cookie at all, which points at the session-lifetime + and 302-vs-401 mechanisms above, not at this fix. **Anyone expecting these five renames to + fix the rejection volume will be disappointed, and should not be.** + + `reject` is also not a clean SLI in either direction: it counts every unauthenticated first + hit and every XHR poll, so a busy *arr inflates it without any user being inconvenienced. - **The four target files are generated, not authored.** Rendering `catalog/` with the canonical command into a temp dir and diffing against `apps/` reported all 10 onboarded goldens byte-identical before any change. Hand-editing one is drift by definition. @@ -152,23 +227,53 @@ on its own terms and should be closed rather than rebased. ## What I did NOT verify -- **Gate rc 3 (UNDETERMINED), not 0.** Green stages: `decisions-log`, `lint`, `l0`, `render-golden`, - `l1`, `approval-policy`. **Skipped:** `spec-immutability` (the sibling spec repo is not mounted - at the container's `SPEC_REPO` path from a worktree — the one tolerated skip), `gitea-settings` - (no `SETTINGS_API_TOKEN`), and **L2/L3 conformance** (`CLUSTER_AVAILABLE` unset). Nothing FAILED; - nothing runtime was certified either. **rc 3 is UNDETERMINED, never green**, and this branch must - not be described as green on the strength of it. The touched-requirement subset — - `R-GITOPS-02 R-IDP-02 R-IDP-06 R-NET-01 R-NET-05 R-NET-06 R-NET-07 R-PLAT-02 R-TEST-01 R-TEST-02 - R-TEST-03 R-TEST-04` — is reported **UNVERIFIED** at L2/L3 for all twelve. -- **`approval-policy` passed vacuously here** (`event='none'` — it is not a pull_request run). On - the PR it will REQUIRE a non-author approving review: `infra/gitea/approval-policy.json` lists - `conformance/**` as sensitive, because changing the tests changes what green MEANS. That is - correct and expected for this branch, not an obstacle to route around. -- **No browser round-trip. The sign-in fix is UNVERIFIED end-to-end.** Identical to the gap the - 2026-09-29 entry recorded and did not close: the collision has still **never been reproduced in a - browser**, because reproducing it needs two gated apps visited in one session. Everything here is - reasoned from measured configuration — distinct secrets, default names, shared domain — and from - the mechanism, not from a failure anyone watched. It is a latent-bug fix. +- **Local gate: rc 3 (UNDETERMINED), not 0.** Green: `spec-immutability`, `decisions-log`, `lint`, + `l0`, `render-golden`, `l1`, `approval-policy`. **Skipped:** `gitea-settings` (no + `SETTINGS_API_TOKEN` locally) and **L2/L3 conformance** (`CLUSTER_AVAILABLE` unset). Nothing + FAILED; nothing runtime was certified either. **rc 3 is UNDETERMINED, never green.** Touched + subset `R-GITOPS-02 R-IDP-02 R-IDP-06 R-NET-01 R-NET-05 R-NET-06 R-NET-07 R-PLAT-02 R-TEST-01 + R-TEST-02 R-TEST-03 R-TEST-04` — **UNVERIFIED** at L2/L3, all twelve. +- **`spec-immutability` was skipping because of a BUG IN MY TOOLING, not the environment.** The + worktree ENGINE shim in circulation mounts only the main checkout's `.git`, and `dev.sh` + computes the spec path from its own `REPO_ROOT` — which in a worktree is + `.claude/worktrees/agent-*/`, so `../home-network-k3s` does not resolve and it adds no spec + mount at all. The gate then reported the skip, and the skip read as "the one tolerated + environmental skip" while actually being a path bug being waved through. Adding + `-v /var/home/keith/source/home-network-k3s:/home-network-k3s:ro -e SPEC_REPO=/home-network-k3s` + makes **R-TEST-06 actually run and pass**. Worth putting in every worktree agent brief; the + two-mount shim in CLAUDE.md §3 should say so. +- **CI: `gate / gate (pull_request)` reports FAILURE on `75a4029`, for exactly ONE reason, and it + is not this change.** Read from the job log rather than the tick: + + [fail] approval-policy: a sensitive change lacks a non-author approval -> blocks merge + [fail] MERGE GATE: BLOCKED + + Every substantive stage is **green in CI** — `gitea-settings: pass` (CI has the token my local + run does not), `decisions-log`, `lint`, `l0`, `render-golden`, `l1`. Skipped: `spec-immutability` + (the workflow does not check the spec repo out — the documented tolerated skip) and + `conformance` (declared). So CI is a **stricter, more complete** run than my local one on every + axis except the spec repo, and it agrees. + `infra/gitea/approval-policy.json` lists `conformance/**` as sensitive — *"the tests and the + harness; changing them changes what green MEANS"* — so this PR is **fail-closed pending a + non-author approving review**, which is CLAUDE.md §1 working, not a defect to route around. + **The red tick is the policy, and it clears when a human reviews it.** Note also that + `gate / conformance (pull_request)` reports `success` with description **"Has been skipped"** — + the precise trap CLAUDE.md §2b names; it certifies nothing. +- **THAT THIS FIX CURES THE REPORTED SYMPTOM.** Stated plainly because it is the claim most likely + to be read into this branch: it is **not supported**. What is measured is that the instances + *collide* (distinct secrets, shared default name, shared domain) and that the collision's + fingerprint appears in live logs (radarr 8×, jackett 1×, fixed crafty 0×). What is **not** + measured is that removing the collision makes anyone's sign-in work, because the one app already + fixed logged **zero successful sign-ins** in its fixed pod's entire 13.9h life — no traffic, so + no ratio, so no evidence either way. And ~95% of radarr's rejections carry **no cookie at all**, + so most of the observed failures are something this branch does not touch. Anyone closing this + out should measure crafty's ratio again **after real sign-in traffic**, and expect the other two + mechanisms (5-minute session expiry, 302-on-XHR) to dominate until they are fixed too. +- **No browser round-trip.** The collision has never been reproduced *by me* in a browser, because + reproducing it needs two gated apps visited in one session. Everything here is reasoned from + measured configuration and from the mechanism. The concurrent jackett branch does carry an + observed log-level reproduction (interleaved radarr/jackett `removing session` deleting a clean + session 26s after login) — that is real evidence for the MECHANISM, and it is theirs, not mine. - **Nothing was applied.** No `kubectl apply/edit/patch`, no `flux reconcile`. Cluster access was read-only throughout. The running pods are still the pre-fix spec. **Merging is what deploys**, and it will roll five Deployments; every user's existing session on those five apps is @@ -177,6 +282,12 @@ on its own terms and should be closed rather than rebased. - **The live flags are not re-measured by the check.** git is not the cluster. I measured the live pods by hand, once; nothing in the gate re-reads them, so hand-applied drift would be invisible to it. A live-state assertion is L2 and is not written. +- **Log-derived counts are a weak instrument and I did not strengthen them.** `kubectl logs + --since=` returns what the kubelet still retains, so an old, busy container's window may be + truncated; and `No valid authentication in request` counts unauthenticated first hits and XHR + polls indiscriminately. The `cookie signature not valid` / `removing session` pair is the only + collision-specific signal in there, and it is the one I leaned on. None of this is gated — + there is no alert on it and no L2 check reads it. - **The cookie-secrets' distinctness is not gated either.** It was measured out of band and is asserted nowhere. If they were ever made identical the collision would become harmless — but sharing one session-encryption key across trust zones is a worse idea than the bug it would mask, From a78f0103684c37f367887614da6cd415ea3d83f6 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 12:56:55 -0500 Subject: [PATCH 06/13] feat(node-health): probe the Intel render device, read-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The prod nodes already have a working Intel render device — measured by hand on all three on 2026-09-30. Nothing here installs, loads or changes anything; the point is that the measurement now re-runs on every health run instead of decaying silently in a handoff note. The failure being hunted is silent by construction: Plex hardware-transcodes through /dev/dri/renderD128, and if that node stops existing (node rebuild, a BIOS change that disables the onboard IGD, a kernel that binds a different DRM driver) Plex falls back to software transcoding on its own and the only symptom is a hot CPU. All of it is in the UNPRIVILEGED collector pass: lstat() on /dev/dri/*, grp.getgrgid() for the owning group name, the /sys/class/drm/card*/device/driver symlink for the bound driver, /sys/bus/pci/devices/*/class for display-class devices whether or not a driver is bound, /sys/class/drm/card*/engine/* for the vcs*/vecs* media engines, /proc/modules and /proc/cmdline. lspci and vainfo are probed but never depended on — neither is in node_baseline's package floor. Three checks, in the role's existing PASS/WARN/FAIL/SKIP/INFO shape: graphics.render_device the outcome — a usable character-special renderD* node. FAIL only when a display device IS on the PCI bus and there is still no render node. SKIP when the machine has no display device at all (a VM guest), which is why the PCI scan is separate from the DRM scan: /sys/class/drm/ card* exists only once a driver has BOUND. graphics.drm_driver what is bound, against the new node_health_graphics_expected_driver default (i915), so the comparison is data and not task logic. `xe` bound in place of i915 is a WARN, not a FAIL: the device works but the userspace media stack differs. graphics.media_stack INFO only, deliberately. GuC/HuC off (i915.enable_guc=0) is an accepted state that costs low-power H.264 encode quality only, and no host vainfo is not a defect at all because Plex ships its own libva inside its container. The render node's owning group and GID are REPORTED, never asserted: `render` is a systemd dynamic allocation rather than a Debian static gid, so a rebuild can legitimately move it off the 992 measured today, and Plex's container entrypoint stats the device and joins whichever group owns it. Drift stays visible without being an error. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-30-node-health-render-device.md | 234 ++++++++++++++++++ infra/ansible/roles/node_health/README.md | 88 ++++++- .../roles/node_health/defaults/main.yml | 18 ++ .../node_health/files/node_health_collect.py | 220 +++++++++++++++- .../roles/node_health/tasks/assess.yml | 198 +++++++++++++++ .../node_health/templates/summary.txt.j2 | 31 +++ 6 files changed, 785 insertions(+), 4 deletions(-) create mode 100644 decisions/2026-09-30-node-health-render-device.md diff --git a/decisions/2026-09-30-node-health-render-device.md b/decisions/2026-09-30-node-health-render-device.md new file mode 100644 index 0000000..c6d3cd9 --- /dev/null +++ b/decisions/2026-09-30-node-health-render-device.md @@ -0,0 +1,234 @@ +--- +date: 2026-09-30 +branch: feat/node-health-render-device +title: The Intel render device is now self-re-measuring, not a fact someone checked once +--- + +## What changed + +### Why this probe has value at all + +**Nothing else in the platform would notice the device going missing.** Plex exposes no +ServiceMonitor, so no alert fires on a silent fall back to software transcoding. And the +manifest side mounts the device with `hostPath type: DirectoryOrCreate`, which means a node +*without* the device mounts an **empty directory** — Plex starts, runs, and degrades quietly +rather than failing loudly. There is no crash loop, no failed mount, no alert: just a hot CPU. +This check is the only thing in the system that would surface it, which is the actual +justification for the role carrying it. + +### The change + +`roles/node_health` grew a graphics / render-device probe. Nothing was installed, no module +was loaded, no group was changed, nothing was rebooted: the prod nodes already have a working +Intel render device (measured by hand on all three on 2026-09-30) and the point of this change +is that the measurement now **re-runs forever** instead of decaying silently in a handoff note. + +The failure this exists to catch is silent by construction. Plex hardware-transcodes through +`/dev/dri/renderD128`; if that node stops existing — node rebuild, a BIOS change that disables +the onboard IGD, a kernel that binds a different DRM driver, a driver regression — Plex falls +back to software transcoding on its own and the only symptom is a hot CPU. + +Four files: + +- `files/node_health_collect.py` — new `collect_graphics()`, wired into the **unprivileged** + pass (`main()`). Everything it reads is world-readable: `lstat()` on `/dev/dri/*`, + `grp.getgrgid()` for the owning group name, the `/sys/class/drm/card*/device/driver` symlink + target for the bound driver, `/sys/bus/pci/devices/*/class` for display-class (PCI base class + `0x03`) devices whether or not a driver is bound, `/sys/class/drm/card*/engine/*` for the + `vcs*`/`vecs*` media engines, `/proc/modules`, `/proc/cmdline`, and + `/sys/module/{i915,xe}/parameters/enable_guc`. `lspci` and `vainfo` were added to the + existing tool probe; both are optional and neither is depended on. +- `tasks/assess.yml` — one shared derive task plus three checks: `graphics.render_device` + (PASS/FAIL/SKIP), `graphics.drm_driver` (PASS/WARN/FAIL/SKIP), `graphics.media_stack` + (INFO/SKIP). +- `defaults/main.yml` — `node_health_graphics_expected_driver: i915`, so the one comparison the + check makes is data rather than task logic. +- `templates/summary.txt.j2` — a `GRAPHICS / RENDER DEVICE` section, and + `roles/node_health/README.md` gained a section explaining the design and two new "known + limits". + +Three design rulings worth stating, because each is the kind of thing a later edit would get +wrong: + +1. **The render node's GID is reported, never asserted.** It read `root:render`, gid **992**, + mode `0660` on all three prod nodes. The check does *not* compare against `992`. `render` is + a **systemd dynamic** allocation (`systemd-sysusers`), not a Debian static gid from + `base-passwd`, so a rebuild can legitimately move it — and Plex's container entrypoint stats + the device and joins whichever group owns it, so drift genuinely is not a fault. The + observed group name and number appear in the detail line, which makes drift *visible* + without making it an error. A hard `!= 992` would be a false alarm waiting for a reinstall. +2. **The PCI scan is deliberately separate from the DRM scan.** `/sys/class/drm/card*` exists + only once a driver has *bound*, so a GPU with nothing attached has no `card0` and would + otherwise be indistinguishable from a machine with no GPU. Scanning the PCI bus separately + is what lets "no display device at all" be an honest **SKIP** (a VM guest — the same + convention `thermal.max` already uses for "no sensor on this hardware") while "a GPU is on + the bus and there is still no render node" is a **FAIL**. +3. **GuC/HuC and host VAAPI userspace are INFO and must stay INFO.** GuC/HuC off + (`i915.enable_guc=0`) is an accepted state here and costs low-power H.264 encode *quality* + only. No host `vainfo` is not a defect at all — Plex ships its own libva and media driver + inside its container. Grading either would put a permanent WARN on a chosen configuration, + or invite someone to `apt install vainfo` on a node to clear a health check, and this role + installs nothing. + +`xe` bound instead of `i915` is a **WARN**, not a FAIL: the device still works, but the two +drivers do not share a userspace media stack. On these nodes `xe.ko.xz` exists on disk and is +bound to nothing. + +## What I verified + +### The real prod run (read-only, all three nodes) + +`--list-hosts` first, every time. It named exactly three hosts and nothing else: + + play #1 (k3s_prod): Inspect node hardware health (read-only) + pattern: ['k3s_prod'] + hosts (3): k3s-prod-3 k3s-prod-2 k3s-prod-1 + +Then `--syntax-check` (rc 0), then the run: + + ansible-playbook playbooks/node-health.yml -e node_health_hosts=k3s_prod --limit k3s_prod + + k3s-prod-1 : ok=51 changed=3 unreachable=0 failed=0 skipped=1 + k3s-prod-2 : ok=51 changed=2 unreachable=0 failed=0 skipped=1 + k3s-prod-3 : ok=51 changed=2 unreachable=0 failed=0 skipped=1 + PLAY rc=0 + +(The `changed=` counts are the *controller-side* artifact writes under +`infra/ansible/_artifacts/node-health/`, delegated to localhost. Nothing on any node changed — +the collector task is `changed_when: false` and the role touches nothing.) + +All three nodes graded **PASS / PASS / INFO** on the new checks, 0 FAIL overall: + + k3s-prod-1 VERDICT 0 FAIL · 1 WARN · 18 PASS · 2 SKIP · 6 INFO + k3s-prod-2 VERDICT 0 FAIL · 3 WARN · 15 PASS · 3 SKIP · 6 INFO + k3s-prod-3 VERDICT 0 FAIL · 2 WARN · 17 PASS · 2 SKIP · 6 INFO + + [PASS] graphics.render_device — DRM render node present for hardware transcoding (/dev/dri/renderD*) + [PASS] graphics.drm_driver — DRM driver bound to the display device (expected i915) + [INFO] graphics.media_stack — Media engines, GuC/HuC firmware and host VAAPI userspace (informational) + +(The pre-existing WARNs and SKIPs are other checks — SMART, memattr, resolvers — and are not +mine; none of them moved.) + +The new report section, **byte-identical on all three nodes**: + + -------------------------------------------------------------------------------- + GRAPHICS / RENDER DEVICE (the Plex hardware-transcoding path) + -------------------------------------------------------------------------------- + pci 0000:00:02.0 00:02.0 VGA compatible controller [0300]: Intel Corporation CoffeeLake-S GT2 [UHD Graphics 630] [8086:3e98] (rev 02) + driver i915 + drm card0 driver i915 at 0000:00:02.0 engines bcs0 rcs0 vcs0 vecs0 + dev card0 character special mode 0660 root:video gid 44 + dev renderD128 character special mode 0660 root:render gid 992 + GuC/HuC NOT READ — /sys/module/i915/parameters/enable_guc is not + readable without root (i915 module parameters are mode 0400) + no i915/xe parameter on the kernel cmdline + modules amdgpu=false, i915=true, nouveau=false, nvidia=false, radeon=false, xe=false (/proc/modules — corroboration only; the bound-driver symlink above decides) + vainfo NOT installed on this host — not a defect: Plex ships its own libva/media driver inside its container + +That reproduces every item of the 2026-09-30 hand measurement independently: `renderD128` +character special, `root:render`, **gid 992**, mode `0660`; `card0` `root:video` gid 44 mode +`0660`; `i915` bound; `xe` **not** loaded and bound to nothing; `[8086:3e98] (rev 02)` +CoffeeLake-S GT2 UHD Graphics 630 at `00:02.0`; `vcs0` and `vecs0` live; no host `vainfo`. + +**One measured correction to my own phase-1 assumption:** `lspci` **is** present on all three +prod nodes (`/usr/bin/lspci` — Debian ships `pciutils` by default). I had written that it was +absent. It is still *treated* as optional, because `pciutils` is genuinely not declared in +`node_baseline_packages` and depending on an undeclared package would be wrong — but the +comments and README that asserted it was missing were factually wrong and are now corrected. +Its presence is why the `pci` line above carries a readable device name rather than bare IDs. + +### The gate + + DEV_KUBECONFIG=/dev/null ENGINE= ./infra/devcontainer/dev.sh run bash -c './ci/gate.sh' + GATE rc=3 + +**rc 3 — UNDETERMINED. NOT green.** Nothing failed, and nothing is certified either. Exactly +three stages did not run, all three the known local ceiling from a worktree, and **no fourth +skip**: + + [skip] spec-immutability spec repo not present at /work/../home-network-k3s (set SPEC_REPO) + [skip] gitea-settings SETTINGS_API_TOKEN is not set — cannot read the live Gitea rule + [skip] conformance no cluster configured (CLUSTER_AVAILABLE!=true) — L2/L3 NOT RUN + + [ ok ] decisions-log: pass [ ok ] lint: pass [ ok ] l0: pass + [ ok ] render-golden: pass [ ok ] l1: pass [ ok ] approval-policy: pass + +Both CLAUDE.md §3 worktree workarounds were needed and both worked: `DEV_KUBECONFIG=/dev/null` +(the credential in the main checkout was **not** copied) and an `ENGINE` shim that bind-mounts +the main checkout's `.git` read-only. No `tracked_files()` traceback appeared, which is how I +know the shim took effect rather than the git stages silently degrading. + +`ansible-lint` was **not** run, because **`ci/` does not invoke it anywhere** — I grepped +`ci/` and `.gitea/` and the lint stage is `yamllint -c ci/yamllint.yaml` only. I am not +claiming to have matched a lint convention that does not exist. + +The gate's `lint` stage passed with one `line too long` **warning** on +`roles/node_health/tasks/assess.yml:189` — that is pre-existing, in the SMART block, well +above my insertion point, and warnings do not fail the stage. + +### Offline work (before the lock was released) + +- `python3 -m py_compile` on the collector: clean. +- `collect_graphics()` executed for real against this workstation's hardware (an unrelated + machine, Raptor Lake-S UHD Graphics + an NVIDIA discrete GPU) and returned the whole + structure with no exception, correctly reporting two cards, two render nodes, the bound + driver per card, the `vcs0/vcs1/vecs0` media engines, and `enable_guc` as *not readable + without root* rather than as a fabricated `0`. +- That last point is a real finding and changed the design: `/sys/module/i915/parameters/*` is + **mode 0400**, so an unprivileged pass cannot read `enable_guc`. It now reports + `NOT READ — ` and `/proc/cmdline` is read as the world-readable second source. An + earlier draft would have silently reported a null. +- Two other bugs found and fixed by running it rather than reasoning about it: + `/dev/dri/by-path` is a **directory** and was being `lstat()`ed as a device; and + `/sys/class/drm` carries one entry per *connector* (`card0-HDMI-A-1`), which had to be + excluded from the card scan. +- All three checks and the new template section were rendered through a standalone Jinja + harness (Ansible is not on the host — CLAUDE.md §5) against **seven** fact sets: the real + workstation; GPU present with the render node removed; no display device at all; `xe` bound + in place of `i915`; a display device with **no** driver bound; `renderD128` present but not a + character device; and a shape with neither `lspci` nor `vainfo`. Zero template errors, and + the statuses came out as designed — `FAIL/PASS/INFO`, `SKIP/SKIP/SKIP`, `PASS/WARN/INFO`, + `FAIL/FAIL/INFO`, `FAIL/PASS/INFO` respectively. The harness supplies stand-ins for the + Ansible-only filters (`bool`, `dict2items`, `flatten`, `zip`, `regex_replace`) and matches + Ansible's `trim_blocks=True`. + **Read that for exactly what it is: evidence that the LOGIC is right, and no evidence + whatsoever that the HARDWARE DETECTION is.** Six of those seven fact sets were hand-mutated + dictionaries, not machines. They are not hardware tests and must not be written up as if + they were. + +## What I did NOT verify + +- **The FAIL path has never fired on a real node.** Every prod node has a working render + device, and this role is read-only, so a device cannot be taken away to prove the check + catches its absence. The failure branches were exercised against **synthetic fact sets + only** (see above). I am not claiming the failure path is tested against hardware. +- **`xe` detection is likewise untested against reality.** No node here has `xe` bound — the + module exists on disk as `xe.ko.xz` and is bound to nothing — so the WARN branch was only + ever driven by a mutated fact set. If `xe` ever does bind, the *shape* of + `/sys/class/drm/card*` under it is something I have assumed matches i915's (same + `device/driver` symlink, same `engine/*` directories). That assumption is unverified. +- **The media-engine sysfs path is confirmed on these nodes but not proven portable.** + `vcs0`/`vecs0` were read from `/sys/class/drm/card0/engine/*` on all three prod nodes, so it + is measured here. I did **not** confirm against kernel source that no other kernel version or + driver publishes engines elsewhere. If one does, the check reports "none exposed" — an INFO + line, not a false verdict, which is why this failure mode is tolerable. +- **`enable_guc` is still unread on prod.** The report says `NOT READ — mode 0400`, which is + honest but means I have **not** confirmed by measurement that GuC/HuC is off on these nodes; + I only know the kernel cmdline carries no `i915.enable_guc=` override. The brief's ground + truth says `i915.enable_guc=0`; my check does not corroborate that value, it declines to + guess it. Reading it would need root, and it is INFO either way, so I left it alone. +- **Not a transcode test.** These checks prove the render *device* exists, is a character + device, and which driver owns it. They do **not** prove a transcode succeeds — that needs a + VAAPI round trip inside Plex's own container with Plex's own libva, and is out of scope for a + node-level check that installs nothing. +- **The gate did not establish greenness.** It returned **rc 3**, and three stages did not run + (`spec-immutability`, `gitea-settings`, `conformance`). So: the offline L0/L1 subset passed, + the live Gitea branch-protection rule is **UNVERIFIED** from this run, and L2/L3 conformance + did not execute. I am not describing this PR as green. +- **No claim about the live platform beyond what the run printed.** Per CLAUDE.md §2b this repo + is not evidence about prod; the only prod facts I assert are the ones quoted verbatim above. + I did not query Gitea's branch protection or runner state, so I assert nothing about either. +- **The stale dev inventory was left alone deliberately.** `hosts.ini` still defines the + `k3s-dev-*` groups as the default target of most playbooks and the dev cluster no longer + exists. That is real, it is out of this change's scope, and it is unclaimed work. diff --git a/infra/ansible/roles/node_health/README.md b/infra/ansible/roles/node_health/README.md index 95f7dd6..88fcb97 100644 --- a/infra/ansible/roles/node_health/README.md +++ b/infra/ansible/roles/node_health/README.md @@ -19,9 +19,9 @@ Serves **R-ENV-01** (you cannot certify a node you have not measured), **R-ENV-0 ## Contract -- **Read-only and side-effect-free.** It reads `/sys`, `/proc`, `/etc`, runs `lsblk`/`lscpu`, - and — only when it can escalate — `smartctl`. It writes nothing to the node and - **installs nothing**. `smartmontools` is declared in `node_baseline_packages`; if it is +- **Read-only and side-effect-free.** It reads `/sys`, `/proc`, `/etc`, runs `lsblk`/`lscpu` + (and `lspci` only if it happens to be installed), and — only when it can escalate — + `smartctl`. It writes nothing to the node and **installs nothing**. `smartmontools` is declared in `node_baseline_packages`; if it is absent the SMART checks SKIP with that as the stated reason (operator ruling 2026-08-31: package installation is `node_baseline`'s job, never a health check's, never a manual `apt-get`). @@ -50,6 +50,9 @@ Serves **R-ENV-01** (you cannot certify a node you have not measured), **R-ENV-0 | `memory.available`, `memory.swap`, `memory.ecc` | headroom, kubelet's swap-off prerequisite **and whether it survives a reboot** (see [Swap off is not the same as swap disabled](#swap-off-is-not-the-same-as-swap-disabled)), EDAC correctable/uncorrectable counters | | `memory.physical_total` | **is this machine using the RAM it has?** `/proc/zoneinfo` present-vs-managed pages (no root, metal or VM) and `MemTotal` vs the DIMM capacity SMBIOS reports (`dmidecode -t 17`, root, bare metal). Cause-independent by design — see [The EFI memattr check](#the-efi-memattr-check) | | `cpu.throttle`, `cpu.inventory`, `cpu.virtualisation` | thermal throttle events since boot, topology + frequency scaling, VT-x/AMD-V + `/dev/kvm` (KubeVirt, R-PLAT-03) | +| `graphics.render_device` | is there a usable `/dev/dri/renderD*` character device — the path Plex hardware-transcodes through. See [The render-device check](#the-render-device-check) | +| `graphics.drm_driver` | which DRM driver is bound to the display device, against `node_health_graphics_expected_driver` (`i915` here) | +| `graphics.media_stack` | **INFO only**: `vcs*`/`vecs*` media engines, GuC/HuC firmware state, host VAAPI userspace. None of these is ever graded — see below for why | | `thermal.max` | hottest sensor across thermal zones and hwmon chips | | `pressure.cpu` / `.memory` / `.io` | PSI `some avg60` — dev learned to care about CPU PSI specifically | | `network.link`, `network.errors`, `network.resolvers` | link state/speed, NIC error counters, DNS upstream redundancy per address family | @@ -91,6 +94,78 @@ used to blame that state on "this node has not been baselined yet", which sent t back to re-run the role that had already run. +## The render-device check + +Plex transcodes on the iGPU through `/dev/dri/renderD128`. **When that node stops existing, +nothing fails loudly.** Plex falls back to software transcoding by itself; the only symptom +is a hot CPU and stuttering clients. A node rebuild, a BIOS change that disables the onboard +IGD, a kernel that binds a different DRM driver, or a driver regression all produce exactly +that silent degradation. + +The state was measured by hand on all three prod nodes on 2026-09-30 and found working. **A +fact measured once decays silently**, so it is re-measured on every health run instead of +living in a runbook. Nothing is installed, no module is loaded, no group is changed — the +role's read-only contract is unchanged. + +| fact | where it comes from | root? | +|---|---|---| +| `/dev/dri` contents, ownership, mode, char-vs-not | `lstat()` on each entry | no | +| owning group **name** for the render node's gid | `grp.getgrgid()` | no | +| bound DRM driver | `/sys/class/drm/card*/device/driver` symlink target | no | +| is the iGPU on the bus at all | `/sys/bus/pci/devices/*/class` (PCI base class `0x03`) | no | +| media engines (`vcs*`, `vecs*`) | `/sys/class/drm/card*/engine/*` | no | +| module load state | `/proc/modules` | no | +| GuC/HuC | `/sys/module/i915/parameters/enable_guc` + `/proc/cmdline` | **parameter file is mode 0400** | + +### The PCI scan is separate from the DRM scan on purpose + +`/sys/class/drm/card*` exists only once a driver has **bound**. A machine whose iGPU sits on +the bus with nothing attached has no `card0` at all — and that is precisely the failure being +hunted. So the display device is looked for on the PCI bus independently, from sysfs, which +is always there. That is what lets `graphics.render_device` distinguish + +- *no GPU in this machine* → **SKIP** (a VM guest; nothing to grade, the same convention + `thermal.max` uses for "no sensor on this hardware"), from +- *a GPU is on the bus and there is still no render node* → **FAIL**. + +`lspci` is used **only** to decorate the PCI entry with a readable name, and it is treated as +optional because it is: `pciutils` is **not** declared in `node_baseline_packages`, so nothing +in this repo guarantees it. It did turn out to be present on all three prod nodes +(`/usr/bin/lspci`, measured 2026-09-30 — Debian installs `pciutils` by default), but depending +on that would mean depending on an undeclared package. Absent, `description` is `null`, the +numeric IDs are printed instead, and no verdict changes. + +### The render node's GID is reported, never asserted + +The render device was `root:render`, **gid 992**, mode `0660` on all three prod nodes on +2026-09-30. The check does **not** compare against `992`, and must not be changed to. +`render` is a **systemd dynamic allocation** (`systemd-sysusers`), not a Debian static gid +from `base-passwd`, so a rebuilt node can legitimately land on a different number. Plex's own +container entrypoint stats the device and joins whichever group owns it, so drift genuinely +is not a fault. Recording the observed group name and gid in the detail line makes drift +**visible** without making it an error — a hard `!= 992` would be a false alarm waiting for a +reinstall. + +What *is* configurable is the expected DRM driver, `node_health_graphics_expected_driver` +(default `i915`), so the verdict stays out of task logic. `xe` bound instead of `i915` is a +**WARN**, not a FAIL: the device still works, but the two drivers do not share a userspace +media stack and a libva/iHD build that transcodes correctly against one is not automatically +right for the other. On these nodes `xe.ko.xz` exists on disk and is bound to nothing. + +### Two things that are informational and must stay that way + +- **GuC/HuC firmware not loaded** (`i915.enable_guc=0`) is a known, accepted state here. It + costs low-power H.264 encode *quality* only; decode and encode both still run on the media + engines. Grading it would put a permanent WARN on a chosen configuration. +- **No host VAAPI userspace / no `vainfo`** is **not a defect**. Plex ships its own libva and + media driver inside its container, so the host needs neither. Flagging it would invite + someone to `apt install vainfo` on a node to clear a health check — and this role installs + nothing. + +`enable_guc` is one of i915's `0400` module parameters, so an unprivileged pass usually +cannot read it. It reports as an honest `NOT READ — ` rather than a fabricated `0`, +with `/proc/cmdline` as the world-readable second source for an explicit boot override. + ## The EFI memattr check `infra/PROD-PROVISIONING.md` §0b is authoritative; this is only what the check needs. @@ -273,3 +348,10 @@ stays `null` — no claim is made either way. but the wear/endurance context SKIPs rather than guessing. - `ioerr_cnt` counts commands the kernel saw fail, which includes benign rejected or unsupported commands during discovery. Attribution needs `dmesg`, which needs root. +- `graphics.render_device`'s **FAIL path has never fired against a real node** — every prod + node has a working render device, and the role is read-only so one cannot be taken away to + prove it. The failure branches were exercised against synthetic fact sets only. The same + applies to the `xe`-bound WARN: no node here has `xe` bound. +- `graphics.*` proves the render **device** exists and which driver owns it. It does **not** + prove a transcode succeeds — that needs a VAAPI round trip inside Plex's own container, + with Plex's own libva, which is out of scope for a node-level check that installs nothing. diff --git a/infra/ansible/roles/node_health/defaults/main.yml b/infra/ansible/roles/node_health/defaults/main.yml index eae98c9..9b02dbc 100644 --- a/infra/ansible/roles/node_health/defaults/main.yml +++ b/infra/ansible/roles/node_health/defaults/main.yml @@ -35,6 +35,24 @@ node_health_try_privileged: true # --nocheck=standby: never spin up a sleeping disk just to read it. Read-only hygiene. node_health_smartctl_args: "--json=c --all --nocheck=standby" +# --- Graphics / render device --------------------------------------------------------- +# The DRM driver this site EXPECTS to be bound to its display device. Measured on all three +# prod nodes 2026-09-30: `i915` on the CoffeeLake-S GT2 [UHD Graphics 630] iGPU of every HP +# EliteDesk 800 G5 Mini. The newer `xe` module is present on disk as xe.ko.xz but is bound to +# nothing, and that difference matters enough to surface: the two drivers do not share a +# userspace media stack (a libva/iHD build that works against i915 is not automatically the +# right one for xe), so a silent switch would change Plex's transcoding path. A site on AMD +# or NVIDIA sets this to `amdgpu` / `nouveau` / `nvidia` rather than editing task logic. +# +# WHAT IS DELIBERATELY *NOT* A SETTING HERE: the numeric GID of the render node's owning +# group. `render` is a systemd DYNAMIC allocation (systemd-sysusers), not a Debian static +# one, so a rebuilt node may legitimately land on a different number — it was 992 on all +# three prod nodes on 2026-09-30, and an assertion against that literal would be a false +# alarm waiting to happen. Plex's own container entrypoint stats the device and joins +# whichever group owns it, so drift genuinely is not a fault. The observed group name and GID +# are therefore REPORTED in the check detail and never graded. +node_health_graphics_expected_driver: i915 + # --- Thresholds --------------------------------------------------------------------- # All verdicts come from here, so a site can retune without touching task logic. node_health_thresholds: diff --git a/infra/ansible/roles/node_health/files/node_health_collect.py b/infra/ansible/roles/node_health/files/node_health_collect.py index 8eb79e7..d6d6e1a 100644 --- a/infra/ansible/roles/node_health/files/node_health_collect.py +++ b/infra/ansible/roles/node_health/files/node_health_collect.py @@ -35,10 +35,13 @@ # ============================================================================= import glob +import grp import json import os +import pwd import re import shutil +import stat import subprocess import sys import time @@ -756,6 +759,212 @@ def collect_pressure(): return out +# --- Graphics / DRM render device ----------------------------------------------------- +# WHY THIS IS HERE AT ALL. Plex transcodes on the iGPU through /dev/dri/renderD128. If that +# node stops existing — a node rebuild, a BIOS change that disables the IGD, a kernel that +# binds a different DRM driver — nothing breaks loudly: Plex silently falls back to software +# transcoding and the only symptom is a hot CPU. So the render device is measured on every +# health run rather than being a fact someone established once by hand. +# +# EVERYTHING BELOW IS UNPRIVILEGED. /dev/dri is world-traversable, the device nodes are +# stat()able by anyone, /sys/class/drm and /sys/bus/pci/devices are world-readable, and so +# are the module parameters under /sys/module. Nothing here needs root, so nothing here +# belongs in the privileged pass. (GuC/HuC *load* status lives in debugfs, which does need +# root AND debugfs mounted; the module PARAMETER below is the unprivileged stand-in and is +# what actually decides whether the firmware is loaded at all.) +# +# THE PCI SCAN IS DELIBERATELY SEPARATE FROM THE DRM SCAN. /sys/class/drm/card* only exists +# once a driver has BOUND. A machine whose iGPU is on the bus with no driver bound has no +# card0 at all, and that is precisely the failure this is meant to catch — so the display +# device is looked for on the PCI bus independently, from sysfs, which is always there. +# `lspci` is used ONLY to decorate that with a human-readable name, and it is treated as +# optional because it IS optional: pciutils is not declared in node_baseline_packages, so +# nothing in this repo guarantees it. It did turn out to be present on all three prod nodes +# (/usr/bin/lspci, measured 2026-09-30 — Debian installs pciutils by default), but relying on +# that would make the collector depend on an undeclared package. A missing lspci leaves +# `description` null, the numeric IDs are reported instead, and no verdict changes. +DISPLAY_PCI_CLASS_PREFIX = "0x03" # PCI base class 03 = display controller (VGA/3D/other) +# DRM drivers worth reporting the load state of. The bound-driver symlink is authoritative +# (a built-in driver never appears in /proc/modules); this is corroboration only, and it is +# how "xe.ko.xz exists on disk but is bound to nothing" becomes a visible fact. +DRM_MODULES = ("i915", "xe", "amdgpu", "radeon", "nouveau", "nvidia") + + +def _uid_name(uid): + try: + return pwd.getpwuid(uid).pw_name + except (KeyError, OverflowError, TypeError, OSError): + return None + + +def _gid_name(gid): + """Group NAME for a numeric gid, or None when nothing resolves it. + + Reported as an OBSERVED value, never asserted against a literal. The `render` group is + a systemd DYNAMIC allocation (it is created by systemd-sysusers, not fixed by Debian's + static base-passwd list), so its gid can legitimately differ after a rebuild. Plex's own + container init stats the device and joins whatever group owns it, so a moved gid is + drift to notice, not a fault to fail on.""" + try: + return grp.getgrgid(gid).gr_name + except (KeyError, OverflowError, TypeError, OSError): + return None + + +def _module_param(mod, param): + """One /sys/module//parameters/, with WHY it is missing when it is. + + Three outcomes that must not be conflated: a real value; "the module exposes no such + parameter" (not loaded, built in, or a driver that simply has no such knob — xe has no + enable_guc, GuC is mandatory there); and "the file exists but is mode 0400". Most i915 + module parameters are `i915_param_named(..., 0400, ...)` — root-only — so an + unprivileged pass legitimately cannot read them. Returning null with the reason keeps + that an honest UNKNOWN instead of a fabricated 0. read_text() also records the + permission denial in UNREADABLE, so the summary lists it with everything else root + would add.""" + path = "/sys/module/%s/parameters/%s" % (mod, param) + if not os.path.exists(path): + return {"value": None, "reason": "%s exposes no %s parameter (module not loaded, " + "built in, or no such knob on this driver)" % (mod, param)} + value = read_text(path) + if value is None: + return {"value": None, "reason": "%s is not readable without root (i915 module " + "parameters are mode 0400)" % path} + return {"value": value, "reason": None} + + +def _dev_node(path): + """stat() one /dev/dri entry. lstat, so a symlink is reported as the symlink it is.""" + try: + st = os.lstat(path) + except OSError as exc: + UNREADABLE.append({"path": path, "reason": "stat failed: %s" % exc}) + return None + return { + "path": path, + "name": os.path.basename(path), + "char_device": stat.S_ISCHR(st.st_mode), + "mode": "%04o" % stat.S_IMODE(st.st_mode), + "uid": st.st_uid, + "gid": st.st_gid, + "owner": _uid_name(st.st_uid), + "group": _gid_name(st.st_gid), + } + + +def collect_graphics(): + dri_dir = "/dev/dri" + dri_present = os.path.isdir(dri_dir) + devices = [] + if dri_present: + for path in sorted(glob.glob(dri_dir + "/*")): + # /dev/dri/by-path/ is a directory of symlinks, not a device. Skipping + # directories rather than filtering to char devices is deliberate: anything + # in here that is NOT a character special file is an anomaly worth seeing, and + # `char_device: false` is how it becomes visible. + if os.path.isdir(path): + continue + entry = _dev_node(path) + if entry is not None: + devices.append(entry) + render_nodes = [d for d in devices if d["name"].startswith("renderD")] + + # Bound DRM devices. /sys/class/drm also holds one entry per CONNECTOR + # (card0-HDMI-A-1, card0-DP-1 ...) — those are outputs, not devices, and are skipped. + cards = [] + for cdir in sorted(glob.glob("/sys/class/drm/card[0-9]*")): + name = os.path.basename(cdir) + if "-" in name: + continue + driver = None + drvlink = cdir + "/device/driver" + if os.path.islink(drvlink): + driver = os.path.basename(os.path.realpath(drvlink)) + address = None + devlink = cdir + "/device" + if os.path.islink(devlink): + address = os.path.basename(os.path.realpath(devlink)) + # i915/xe publish one directory per hardware engine. vcs* is a video (H.264/HEVC) + # engine and vecs* the video-enhancement engine: those two are what a transcode + # actually runs on, so their presence is the difference between "a GPU is bound" + # and "this GPU can do media work". + engines = sorted( + { + os.path.basename(p) + for p in (glob.glob(cdir + "/engine/*") + glob.glob(cdir + "/device/engine/*")) + if os.path.isdir(p) + } + ) + cards.append( + { + "card": name, + "driver": driver, + "pci_address": address, + "vendor_id": read_text(cdir + "/device/vendor"), + "device_id": read_text(cdir + "/device/device"), + "engines": engines, + "media_engines": [e for e in engines if e.startswith(("vcs", "vecs"))], + } + ) + + # Display-class devices on the PCI bus, driver bound or not. + pci_display = [] + for pdir in sorted(glob.glob("/sys/bus/pci/devices/*")): + class_id = read_text(pdir + "/class") or "" + if not class_id.startswith(DISPLAY_PCI_CLASS_PREFIX): + continue + address = os.path.basename(pdir) + driver = None + if os.path.islink(pdir + "/driver"): + driver = os.path.basename(os.path.realpath(pdir + "/driver")) + description = None + rc, out, _ = run(["lspci", "-nn", "-s", address]) + if rc == 0 and out.strip(): + description = out.strip().splitlines()[0] + pci_display.append( + { + "address": address, + "class_id": class_id or None, + "vendor_id": read_text(pdir + "/vendor"), + "device_id": read_text(pdir + "/device"), + "revision": read_text(pdir + "/revision"), + "driver": driver, + "description": description, + } + ) + + loaded = set() + for line in (read_text("/proc/modules") or "").splitlines(): + fields = line.split() + if fields: + loaded.add(fields[0]) + modules = {m: (m in loaded) for m in DRM_MODULES} + + # GuC/HuC. i915's `enable_guc` is a bitmask: 0 = neither, 1 = GuC submission, 2 = HuC + # firmware, 3 = both, -1 = whatever the driver defaults to on this platform. It is the + # one place the state is visible without root AND without debugfs — but the sysfs file + # is mode 0400, so unprivileged it usually reads back as an honest UNKNOWN. The kernel + # command line is the world-readable second source: an explicit `i915.enable_guc=` there + # is authoritative about what was ASKED for, even when the parameter file cannot be read. + uc = {mod: {"enable_guc": _module_param(mod, "enable_guc")} for mod in ("i915", "xe")} + cmdline_params = [ + token + for token in (read_text("/proc/cmdline") or "").split() + if token.startswith(tuple("%s." % m for m in DRM_MODULES)) + ] + + return { + "dri_present": dri_present, + "devices": devices, + "render_nodes": render_nodes, + "drm_cards": cards, + "pci_display": pci_display, + "modules": modules, + "uc": uc, + "cmdline_params": cmdline_params, + } + + # --- Storage ------------------------------------------------------------------------ PSEUDO_FS = { "proc", "sysfs", "devtmpfs", "devpts", "tmpfs", "cgroup", "cgroup2", "pstore", @@ -1095,7 +1304,15 @@ def collect_os(): # we only DETECT, and the role skips the SMART checks with an accurate reason. def collect_tools(): tools = {} - for tool in ("smartctl", "nvme", "ethtool", "dmidecode", "lsblk", "lscpu"): + # lspci (pciutils) and vainfo (libva-utils) are BOTH optional and NEITHER is declared in + # node_baseline_packages, so nothing in this repo guarantees either. Measured on prod + # 2026-09-30: lspci IS there (/usr/bin/lspci — Debian ships pciutils by default) and + # vainfo is NOT. lspci only decorates the PCI display entry with a readable name; vainfo + # is not needed on the host at all, because Plex ships its own libva and media driver + # inside its container. They are probed so the report can state their presence or absence + # as a fact instead of leaving the reader to wonder — and so nothing here is ever tempted + # to install them. This role installs nothing. + for tool in ("smartctl", "nvme", "ethtool", "dmidecode", "lsblk", "lscpu", "lspci", "vainfo"): tools[tool] = which(tool) return tools @@ -1127,6 +1344,7 @@ def main(): "os": collect_os(), "dmi": collect_dmi(), "cpu": collect_cpu(), + "graphics": collect_graphics(), "memory": collect_memory(), "thermal": collect_thermal(), "pressure": collect_pressure(), diff --git a/infra/ansible/roles/node_health/tasks/assess.yml b/infra/ansible/roles/node_health/tasks/assess.yml index 1e34098..20dcdb5 100644 --- a/infra/ansible/roles/node_health/tasks/assess.yml +++ b/infra/ansible/roles/node_health/tasks/assess.yml @@ -652,6 +652,204 @@ ~ (', lscpu Virtualization=' ~ c.virtualization if c.virtualization else '') ~ (', running as a guest of ' ~ c.hypervisor_vendor if c.is_virtual_guest | bool else '') }} +# --- Graphics / render device ---------------------------------------------------------- +# WHY A HEALTH CHECK AND NOT A NOTE IN A RUNBOOK. Plex transcodes on the iGPU through +# /dev/dri/renderD128. When that node stops existing nothing fails loudly — Plex falls back +# to software transcoding on its own and the only symptom is a hot CPU and stuttering +# clients. A node rebuild, a BIOS setting that disables the onboard IGD, or a kernel that +# binds a different DRM driver all produce exactly that silent degradation. It was measured +# by hand on all three prod nodes on 2026-09-30 and found working; a fact measured once +# decays silently, so it is re-measured on every health run instead. +# +# READ-ONLY, AND UNPRIVILEGED. Every fact behind these three checks comes from the +# unprivileged pass: stat() on /dev/dri, the /sys/class/drm bound-driver symlink, +# /sys/bus/pci/devices class codes, /proc/modules and /proc/cmdline. The role installs +# nothing (no vainfo, no pciutils), loads no module and changes no group — see the role +# README. The ONE fact that is root-gated is i915's enable_guc module parameter (mode 0400), +# and it reports as an honest UNKNOWN with the reason rather than as a 0. +# +# THE THREE VERDICTS ARE SPLIT ON PURPOSE: +# graphics.render_device the OUTCOME — can anything hardware-transcode on this node. +# graphics.drm_driver the CAUSE side — what is bound, and is it what this site expects. +# graphics.media_stack INFO only. GuC/HuC and host VAAPI userspace both belong here and +# NEITHER is ever a failure; see the task's own comment for why. +- name: Assess — locate the display device and its bound driver (once, shared by three checks) + ansible.builtin.set_fact: + node_health_render: + # Display-class PCI devices (base class 0x03), driver bound or not. This is the + # question "is there a GPU in this machine at all", and it is answered from the PCI + # bus rather than from /sys/class/drm because a card* entry only exists once a driver + # has BOUND — so a GPU with nothing bound to it has no card* and would otherwise look + # identical to a machine with no GPU. + display_devices: "{{ g.pci_display }}" + bound_drivers: >- + {{ g.pci_display | map(attribute='driver') | select('string') | unique | list }} + # Only a CHARACTER SPECIAL file is a usable render node. Anything else called renderD* + # is an anomaly, not a device, and must not be counted as one. + render_chardevs: "{{ g.render_nodes | selectattr('char_device') | list }}" + # Video (vcs*) and video-enhancement (vecs*) engines across every bound card — the + # difference between "a GPU is bound" and "this GPU can do media work". + media_engines: >- + {{ g.drm_cards | map(attribute='media_engines') | flatten | unique | sort | list }} + vars: + ansible_become: false + g: "{{ node_health_facts.graphics }}" + +- name: Assess — DRM render node for hardware transcoding + ansible.builtin.set_fact: + node_health_checks: "{{ node_health_checks + [{ + 'id': 'graphics.render_device', + 'title': 'DRM render node present for hardware transcoding (/dev/dri/renderD*)', + 'status': render_status, + 'detail': render_detail }] }}" + vars: + ansible_become: false + g: "{{ node_health_facts.graphics }}" + r: "{{ node_health_render }}" + # SKIP, not PASS and not FAIL, when the machine has no display device at all: there is + # then nothing to grade, and the same convention thermal.max already uses for "no sensor + # on this hardware" applies. FAIL is reserved for the state that actually breaks Plex — + # a GPU IS on the bus and there is still no render node. + render_status: >- + {{ 'SKIP' if (r.display_devices | length) == 0 + else ('PASS' if (r.render_chardevs | length) > 0 else 'FAIL') }} + dri_listing: >- + {{ (g.devices | map(attribute='name') | join(', ')) if (g.devices | length) > 0 + else 'nothing' }} + render_detail: >- + {%- if (r.display_devices | length) == 0 -%} + this machine exposes no display-class PCI device (base class 0x03) at all, so there is + no GPU here for a render node to belong to and hardware transcoding is not available on + this node by construction. Normal for a VM guest. Measured from /sys/bus/pci/devices, + not assumed. + {%- elif (r.render_chardevs | length) > 0 -%} + {{ r.render_chardevs | length }} render node(s): + {% for d in r.render_chardevs %}{{ d.path }} (character special, mode {{ d.mode }}, + owner {{ d.owner | default(d.uid, true) }}, group {{ d.group | default('unresolved', true) }} + gid {{ d.gid }}){{ ', ' if not loop.last else '' }}{% endfor %}. + /dev/dri holds: {{ dri_listing }}. + THE OWNING GROUP AND GID ARE REPORTED AS OBSERVED, NEVER ASSERTED against a literal: + `render` is a systemd DYNAMIC allocation rather than a Debian static one, so a rebuilt + node may legitimately land on a different number (all three prod nodes read gid 992 on + 2026-09-30), and Plex's container entrypoint stats the device and joins whichever group + owns it. Drift is therefore visible in this line without being a failure. + {%- else -%} + HARDWARE TRANSCODING IS BROKEN ON THIS NODE. A display-class device IS present on the + PCI bus ({{ r.display_devices | map(attribute='address') | join(', ') }}) but there is + no usable character-special render node: /dev/dri + {{ 'does not exist' if not (g.dri_present | bool) else 'holds ' ~ dri_listing }}. + {{ ('Note that /dev/dri holds ' ~ (g.render_nodes | map(attribute='name') | join(', ')) + ~ ' — present, but NOT character special, which is an anomaly in its own right. ') + if (g.render_nodes | length) > 0 else '' }} + Nothing fails loudly in this state — Plex silently falls back to software transcoding + and the only symptom is CPU load. Check graphics.drm_driver below for whether a DRM + driver bound at all, then whether the onboard IGD is still enabled in BIOS. + {%- endif -%} + +- name: Assess — DRM driver bound to the display device + ansible.builtin.set_fact: + node_health_checks: "{{ node_health_checks + [{ + 'id': 'graphics.drm_driver', + 'title': 'DRM driver bound to the display device (expected ' ~ expected ~ ')', + 'status': driver_status, + 'detail': driver_detail }] }}" + vars: + ansible_become: false + g: "{{ node_health_facts.graphics }}" + r: "{{ node_health_render }}" + expected: "{{ node_health_graphics_expected_driver }}" + # WARN rather than FAIL when a DIFFERENT driver is bound (the `xe` case): the device + # still works, but the userspace media stack is not the same one, so it is a change the + # operator must see before a transcode regression surfaces as a user complaint. FAIL is + # for nothing bound at all, which is the cause of a missing render node. + driver_status: >- + {{ 'SKIP' if (r.display_devices | length) == 0 + else ('FAIL' if (r.bound_drivers | length) == 0 + else ('PASS' if expected in r.bound_drivers else 'WARN')) }} + driver_detail: >- + {%- if (r.display_devices | length) == 0 -%} + no display-class PCI device on this machine, so no DRM driver can be bound to one. + Measured from /sys/bus/pci/devices. + {%- else -%} + {% for p in r.display_devices %}{{ p.address }} + {{ p.description | default(((p.vendor_id | default('?', true)) ~ ':' ~ (p.device_id | default('?', true))), true) }} + -> driver {{ p.driver | default('NONE BOUND', true) }}{{ '; ' if not loop.last else '' }}{% endfor %}. + {{ ('DRM cards: ' ~ (g.drm_cards | map(attribute='card') | zip(g.drm_cards | map(attribute='driver')) + | map('join', '=') | join(', ')) ~ '. ') if (g.drm_cards | length) > 0 + else 'No /sys/class/drm/card* entry exists, which is consistent with no driver being bound. ' }} + Module load state (/proc/modules, corroboration only — a built-in driver never appears + there, the bound-driver symlink above is what decides): + {{ g.modules | dict2items | map(attribute='key') | zip(g.modules | dict2items | map(attribute='value') | map('string')) + | map('join', '=') | join(', ') }}. + {%- if (r.bound_drivers | length) == 0 %} + NOTHING IS BOUND to the display device. That is why there is no render node: the DRM + subsystem creates /dev/dri/renderD* only when a driver attaches. Check whether the + module exists for this kernel and whether it was blacklisted or failed to probe. + {%- elif expected in r.bound_drivers %} + This is the expected driver for this site (node_health_graphics_expected_driver). + {%- else %} + EXPECTED `{{ expected }}`, GOT `{{ r.bound_drivers | join(', ') }}`. The device still + works, so this is not an outage — but the two DRM drivers do not share a userspace + media stack, and a libva/iHD build that transcodes correctly against one is not + automatically right for the other. Confirm Plex still hardware-transcodes before + treating this as benign. + {%- endif %} + {%- endif -%} + +# INFO ONLY, AND THAT IS A DELIBERATE RULING, NOT AN OVERSIGHT: +# +# GuC/HuC firmware not loaded (i915.enable_guc=0) — the accepted state on these nodes. It +# costs low-power H.264 encode QUALITY, nothing else; decode and encode both still work +# through the media engines. Grading it would put a permanent WARN on a configuration the +# operator has chosen. +# No host VAAPI userspace / no `vainfo` — NOT a defect. Plex ships its own libva and media +# driver inside its container, so the host needs neither. Flagging it would invite someone +# to `apt install vainfo` on a node to clear a health check, and this role installs nothing. +# +# The media ENGINES are the fact worth watching in this line: vcs* (video) and vecs* +# (video-enhancement) are what a transcode actually runs on. +- name: Assess — media engines, GuC/HuC firmware and host VAAPI userspace (informational) + ansible.builtin.set_fact: + node_health_checks: "{{ node_health_checks + [{ + 'id': 'graphics.media_stack', + 'title': 'Media engines, GuC/HuC firmware and host VAAPI userspace (informational)', + 'status': ('SKIP' if (node_health_render.display_devices | length) == 0 else 'INFO'), + 'detail': media_detail }] }}" + vars: + ansible_become: false + g: "{{ node_health_facts.graphics }}" + r: "{{ node_health_render }}" + # `.get()` rather than a subscript: uc only carries the two Intel drivers, and a site + # that set the expected driver to amdgpu must get a clean "no such parameters" line + # instead of an undefined-key traceback. + guc: "{{ g.uc.get(node_health_graphics_expected_driver) }}" + guc_text: >- + {{ ('enable_guc=' ~ guc.enable_guc.value) if (guc is mapping and guc.enable_guc.value is not none) + else ('NOT READ — ' ~ (guc.enable_guc.reason | default('no reason recorded', true)) + if guc is mapping + else 'no /sys/module/' ~ node_health_graphics_expected_driver ~ '/parameters at all') }} + media_detail: >- + {%- if (r.display_devices | length) == 0 -%} + no display-class PCI device on this machine, so there is no media engine, no GuC/HuC + firmware and nothing to transcode with. + {%- else -%} + media engines: + {{ (r.media_engines | join(', ')) if (r.media_engines | length) > 0 + else 'NONE exposed under /sys/class/drm/card*/engine — a bound driver with no vcs*/vecs* engine cannot do media work' }} + (all engines: {{ (g.drm_cards | map(attribute='engines') | flatten | unique | sort | join(', ')) | default('none', true) }}). + GuC/HuC: {{ guc_text }}. + {{ ('Kernel cmdline carries ' ~ (g.cmdline_params | join(' '))) if (g.cmdline_params | length) > 0 + else 'No i915/xe parameter on the kernel cmdline' }}. + GuC/HuC being off is a KNOWN, ACCEPTED state here and is never graded: it costs + low-power H.264 encode quality only, and both decode and encode still run on the + engines above. + Host VAAPI userspace: vainfo + {{ ('present at ' ~ node_health_facts.tools.vainfo) if node_health_facts.tools.vainfo + else 'NOT installed on this host' }} + — also never graded, because Plex ships its own libva and media driver inside its + container, so the host needs none. This role installs nothing. + {%- endif -%} + # --- Thermals ----------------------------------------------------------------------- - name: Assess — temperatures ansible.builtin.set_fact: diff --git a/infra/ansible/roles/node_health/templates/summary.txt.j2 b/infra/ansible/roles/node_health/templates/summary.txt.j2 index 41ebb69..7e9f5ab 100644 --- a/infra/ansible/roles/node_health/templates/summary.txt.j2 +++ b/infra/ansible/roles/node_health/templates/summary.txt.j2 @@ -136,6 +136,37 @@ {% endfor %} {% endfor %} +-------------------------------------------------------------------------------- + GRAPHICS / RENDER DEVICE (the Plex hardware-transcoding path) +-------------------------------------------------------------------------------- +{% set gfx = node_health_facts.graphics %} +{% if gfx.pci_display %} +{% for p in gfx.pci_display %} + pci {{ '%-12s' | format(p.address) }} {{ p.description | default(((p.vendor_id | default('?', true)) ~ ':' ~ (p.device_id | default('?', true)) ~ ' class ' ~ (p.class_id | default('?', true))), true) }} + driver {{ p.driver | default('NONE BOUND', true) }} +{% endfor %} +{% else %} + pci no display-class device (PCI base class 0x03) on this machine — no GPU here +{% endif %} +{% for c in gfx.drm_cards %} + drm {{ '%-12s' | format(c.card) }} driver {{ '%-8s' | format(c.driver | default('-', true)) }} at {{ c.pci_address | default('-', true) }} engines {{ c.engines | join(' ') | default('none exposed', true) }} +{% endfor %} +{% if gfx.dri_present %} +{% for d in gfx.devices %} + dev {{ '%-12s' | format(d.name) }} {{ '%-18s' | format('character special' if d.char_device else 'NOT A CHAR DEVICE') }} mode {{ d.mode }} {{ d.owner | default(d.uid, true) }}:{{ d.group | default('(gid unresolved)', true) }} gid {{ d.gid }} +{% endfor %} +{% else %} + dev /dev/dri DOES NOT EXIST — no DRM device node on this node at all +{% endif %} +{# GID is printed, never asserted: `render` is a systemd dynamic allocation, so a rebuild may + legitimately move it and Plex's container init joins whichever group owns the device. See + defaults/main.yml (node_health_graphics_expected_driver) for the full reasoning. #} +{% set guc = gfx.uc.get(node_health_graphics_expected_driver) %} + GuC/HuC {{ ('enable_guc=' ~ guc.enable_guc.value) if (guc and guc.enable_guc.value is not none) else (('NOT READ — ' ~ (guc.enable_guc.reason | default('no reason recorded', true))) if guc else ('no /sys/module/' ~ node_health_graphics_expected_driver ~ '/parameters')) | regex_replace('\s+', ' ') | trim | wordwrap(64) | indent(14) }} + {{ ('kernel cmdline: ' ~ (gfx.cmdline_params | join(' '))) if gfx.cmdline_params else 'no i915/xe parameter on the kernel cmdline' }} + modules {% for m, loaded in gfx.modules.items() %}{{ m }}={{ loaded | string | lower }}{{ ', ' if not loop.last }}{% endfor %} (/proc/modules — corroboration only; the bound-driver symlink above decides) + vainfo {{ node_health_facts.tools.vainfo | default('NOT installed on this host — not a defect: Plex ships its own libva/media driver inside its container', true) }} + -------------------------------------------------------------------------------- PRESSURE STALL (/proc/pressure — % of wall time stalled) -------------------------------------------------------------------------------- From dc136a494343e71f6e55b8f427965f7c76901689 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 13:07:43 -0500 Subject: [PATCH 07/13] fix(auth): the forward-auth sidecar also died at 5 minutes and served no metrics Widens the cookie-collision fix to the whole sidecar contract, because all three defects live in the same function and land on the same five goldens, so they are one atomic change or they are five conflicting ones. Added to _oauth2_sidecar_container(), on top of the cookie-name/cookie-domain fix: --cookie-refresh=2m30s The session died FIVE MINUTES after sign-in. Keycloak's homelab realm issues 300-second access tokens (MEASURED on a live session: expires - created = 300.000s exactly, with refresh_token:true on the same line), and --cookie-refresh defaults to 0 -- which the pinned binary's own help spells "0 to disable". So the proxy held a refresh token it was never told to spend. Refresh at HALF the lifetime: two chances to renew before expiry. This, not the collision, is what dominates the rejection VOLUME -- ~95% of radarr's 155 rejections in 12h carry no cookie at all. --metrics-address=0.0.0.0:44180 + a `metrics` container port Without the flag the /metrics listener never starts, so no per-app sidecar was serving metrics at all (`ports` was ['http'] on all six). 44180 is the platform's existing convention, measured not chosen: the shared proxy already runs the same flag and port with 2 ready endpoints. PREREQUISITE ONLY -- NOT scraped. A container port is not a Service port, and each namespace's default-deny baseline is `ingress: []`, so this is unreachable until a Service port (or PodMonitor), a monitoring allow and a ServiceMonitor land. Do not read this as "the sidecars have metrics now". EVERY FLAG VERIFIED AGAINST THE PINNED v7.8.1 BINARY (`--help`), not against docs or the shared proxy -- which runs v7.15.3 and could not have vouched for v7.8.1: * --metrics-address exists in 7.8.1, so this cannot crashloop the pods * --cookie-refresh ... "0 to disable" confirms the default * --cookie-name ... (default "_oauth2_proxy") -- this confirms from the IMPLEMENTATION the constant the new L0 check is built on * --api-route is SINGULAR; the plural does not exist and the proxy refuses to start * --session-cookie-minimal exists and is mutually exclusive with --cookie-refresh, so dropping --pass-access-token is a least-privilege argument, never a size fix NOT done, deliberately: --api-route=^/api/ renderer-wide. It was measured on jackett and PiHole does not serve /api/, so a renderer constant would apply one app's path shape to five. It needs a per-app hook the `auth` schema forbids (additionalProperties: false) -- a schema change, separate PR. Also records the mechanism that made this INTERMITTENT, as a renderer comment, because nobody would reconstruct it from the flags: the loser does not just fail to read the cookie, it calls ClearSession, which expires the cookie on its own --cookie-domain -- the whole zone. The *arr UIs poll (/api/v3/command, /api/v3/queue/details, /signalr/negotiate), so every poll carrying an undecryptable cookie clears it again: A LEFT-OPEN RADARR TAB DELETED JACKETT'S COOKIE EVERY FEW SECONDS, from another namespace, with no user action. That is why signing in again never helped for longer than the gap between two polls. Crafty's counter-evidence resolved: the 81:1 ratio was a 24h window straddling a 13h52m-old pod. Bounded to the fixed process, crafty's ENTIRE life is 3 rejections in 6743 log lines, with ZERO `removing session` and ZERO `cookie signature not valid` against unfixed radarr's 8 and 8. The eviction fingerprint is gone where the fix is deployed. It logged 0 AuthSuccess too, so that is absence-of-the-failure-mode rather than a session watched working -- stated as such in the handoff. Renderer selftest gains six assertions, including a --cookie-refresh check that PARSES the duration and requires 0 < refresh < 300s (so 2m30s/150s/2.5m all stay green while a silent widening past the token lifetime does not). NOTE, and it is in the handoff as the top follow-up: NOTHING IN ci/ RUNS `render.py --selftest`, so those assertions are dev-time only. conformance/tests/l0-static/auditability-proofs-producible/check.sh:291 already drives other tools' selftests for exactly this reason and is the one-line fix. Gate (toolbox, worktree shim with the spec repo mounted at /home-network-k3s): rc 3 UNDETERMINED. Green: spec-immutability, decisions-log, lint, l0, render-golden, l1, approval-policy. Skipped: gitea-settings (no local token), conformance (declared). The new L0 check PASSES on this branch -- all five apps fixed in one commit. Co-Authored-By: Claude Opus 5 (1M context) --- apps/wave-0/jackett.yaml | 5 + apps/wave-0/pihole.yaml | 5 + apps/wave-0/qbittorrent.yaml | 5 + apps/wave-1/radarr.yaml | 5 + apps/wave-1/sonarr.yaml | 5 + catalog/render.py | 89 ++++++- .../golden/expected.yaml | 5 + .../golden/egress-internet.expected.yaml | 5 + .../golden/egress-vpn.expected.yaml | 5 + ...-30-oauth2-proxy-cookie-collision-fleet.md | 233 ++++++++++++------ 10 files changed, 284 insertions(+), 78 deletions(-) diff --git a/apps/wave-0/jackett.yaml b/apps/wave-0/jackett.yaml index 461d2d7..03e1fe5 100644 --- a/apps/wave-0/jackett.yaml +++ b/apps/wave-0/jackett.yaml @@ -104,8 +104,10 @@ spec: - --cookie-domain=jackett.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -126,6 +128,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/apps/wave-0/pihole.yaml b/apps/wave-0/pihole.yaml index c88fe7c..2541c87 100644 --- a/apps/wave-0/pihole.yaml +++ b/apps/wave-0/pihole.yaml @@ -112,8 +112,10 @@ spec: - --cookie-domain=pihole.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -134,6 +136,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/apps/wave-0/qbittorrent.yaml b/apps/wave-0/qbittorrent.yaml index 252997c..e6f19d8 100644 --- a/apps/wave-0/qbittorrent.yaml +++ b/apps/wave-0/qbittorrent.yaml @@ -175,8 +175,10 @@ spec: - --cookie-domain=qbittorrent.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -197,6 +199,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/apps/wave-1/radarr.yaml b/apps/wave-1/radarr.yaml index 2fd8042..2cef42c 100644 --- a/apps/wave-1/radarr.yaml +++ b/apps/wave-1/radarr.yaml @@ -157,8 +157,10 @@ spec: - --cookie-domain=radarr.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -179,6 +181,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/apps/wave-1/sonarr.yaml b/apps/wave-1/sonarr.yaml index b107c45..3771672 100644 --- a/apps/wave-1/sonarr.yaml +++ b/apps/wave-1/sonarr.yaml @@ -157,8 +157,10 @@ spec: - --cookie-domain=sonarr.${BASE_DOMAIN} - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -179,6 +181,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/catalog/render.py b/catalog/render.py index af09aa7..4795f7f 100644 --- a/catalog/render.py +++ b/catalog/render.py @@ -133,6 +133,36 @@ OAUTH2_PROXY_IMAGE = "quay.io/oauth2-proxy/oauth2-proxy:v7.8.1" OAUTH2_PROXY_PORT = 4180 KEYCLOAK_ISSUER = f"https://keycloak.{ZONE}/realms/homelab" +# --- session lifetime ----------------------------------------------------------------- +# Keycloak's `homelab` realm issues access tokens with a 300-SECOND lifetime. MEASURED on a +# live session, not read off a config page: `expires - created = 300.000s` exactly, with +# `refresh_token:true` on the same line — so the proxy HOLDS a refresh token it was never +# told to spend. `--cookie-refresh` defaults to 0, which the v7.8.1 binary's own help spells +# "0 to disable", so every one of these sidecars dropped its session five minutes after +# sign-in and sent the user back to Keycloak. Refresh at HALF the lifetime: two chances to +# renew before expiry, so one failed refresh is not an immediate logout. +# +# NOTE for anyone tempted to add `--session-cookie-minimal` to shrink the cookie: it is +# MUTUALLY EXCLUSIVE with this (provoked: "cookie_refresh > 0 requires oauth tokens in +# sessions"). Dropping --pass-access-token is a least-privilege argument, not a size fix. +OAUTH2_ACCESS_TOKEN_LIFETIME_S = 300 +OAUTH2_COOKIE_REFRESH = "2m30s" + +# --- metrics -------------------------------------------------------------------------- +# oauth2-proxy serves /metrics only when --metrics-address is given; with it absent the +# listener never starts, so NO per-app sidecar was scrapable at all (measured: `ports` was +# `['http']` on all six). 44180 is the platform's existing convention, not a guess — the +# SHARED proxy already runs `--metrics-address=0.0.0.0:44180` with a `metrics` port and 2 +# ready endpoints. The flag is confirmed present in the PINNED v7.8.1 binary (`--help`), +# which matters because the shared proxy is on v7.15.3 and could not have vouched for it. +# +# THIS IS THE PREREQUISITE ONLY, AND IT IS NOT YET SCRAPED. A container port is not a +# Service port: these sidecars' Services expose the app port and :4180, and each namespace's +# default-deny baseline has `ingress: []`, so Prometheus can neither resolve nor reach this +# port until a Service port (or PodMonitor), a monitoring-ingress allow and a ServiceMonitor +# are added. Do not read this flag as "metrics are working". +OAUTH2_PROXY_METRICS_PORT = 44180 + # ============================================================================= # Defaults (renderer-contract.md "Default posture") — materialize BEFORE mapping. @@ -1010,6 +1040,17 @@ def _oauth2_sidecar_container(s: dict) -> dict: # sends you hunting in Keycloak, where nothing is wrong. Silent, intermittent, and # misattributed: the exact shape this repo keeps getting burned by. # + # WHY IT WAS *INTERMITTENT*, AND WHY RELOADING NEVER HELPED. The loser does not merely + # fail to read the cookie — it calls ClearSession, which EXPIRES the cookie on its own + # --cookie-domain, i.e. the whole zone. And the *arr UIs poll: measured on prod, radarr's + # rejections are `/api/v3/command`, `/api/v3/queue/details` and `/signalr/negotiate`, + # 155 of them in 12h against 1 successful auth. Every poll carrying a cookie radarr + # cannot decrypt calls ClearSession again — so A LEFT-OPEN RADARR TAB DELETES JACKETT'S + # COOKIE EVERY FEW SECONDS, from another namespace, with no user action at all. That is + # the intermittency, and it is why signing in again fixed nothing for longer than the + # gap between two polls. Host-scoping the cookie (below) is what contains the blast + # radius of a ClearSession to the app that issued it. + # # DERIVED, NOT LISTED. Both flags come from the ServiceDefinition (`s['name']` and the # route host), so a NEW forward-auth app is isolated the moment it is onboarded. A # per-service table would leave the next app defaulting into the collision, which is how @@ -1032,8 +1073,16 @@ def _oauth2_sidecar_container(s: dict) -> dict: f"--cookie-domain={host}", f"--whitelist-domain=.{ZONE}", "--cookie-secure=true", + # Spend the refresh token the proxy already holds, at half the 300s access-token + # lifetime. Without this the session dies 5 minutes after sign-in. See + # OAUTH2_ACCESS_TOKEN_LIFETIME_S above for the measurement and the + # --session-cookie-minimal incompatibility. + f"--cookie-refresh={OAUTH2_COOKIE_REFRESH}", "--pass-access-token=true", "--set-xauthrequest=true", + # Start the /metrics listener. Prerequisite only — not yet reachable by Prometheus; + # see OAUTH2_PROXY_METRICS_PORT above for exactly what is still missing. + f"--metrics-address=0.0.0.0:{OAUTH2_PROXY_METRICS_PORT}", ] secret = f"{s['name']}-oauth2-proxy" return { @@ -1055,7 +1104,10 @@ def _oauth2_sidecar_container(s: dict) -> dict: "valueFrom": {"secretKeyRef": {"name": secret, "key": "cookie-secret"}}, }, ], - "ports": [{"name": "http", "containerPort": OAUTH2_PROXY_PORT, "protocol": "TCP"}], + "ports": [ + {"name": "http", "containerPort": OAUTH2_PROXY_PORT, "protocol": "TCP"}, + {"name": "metrics", "containerPort": OAUTH2_PROXY_METRICS_PORT, "protocol": "TCP"}, + ], "resources": { "requests": {"cpu": "25m", "memory": "64Mi"}, "limits": {"memory": "128Mi"}, @@ -2530,6 +2582,41 @@ def _selftest() -> int: "whitelist-domain must stay ZONE-wide — it governs post-login redirect targets, not " "cookie scope; narrowing it breaks the sign-in bounce", ) + # SESSION LIFETIME. `--cookie-refresh` must be present AND strictly shorter than the + # measured 300s access-token lifetime, or the session dies mid-use. Asserted as a parsed + # duration rather than a string match, so "2m30s" -> "150s" -> "2.5m" stays green and a + # silent widening past the token lifetime does not. + refresh = next((a.split("=", 1)[1] for a in side["args"] + if a.startswith("--cookie-refresh=")), None) + check(refresh is not None, + "sidecar must set --cookie-refresh; the v7.8.1 default is 0 = DISABLED, so the " + "session dies when the access token expires and the user is bounced to Keycloak") + if refresh is not None: + _m = re.fullmatch(r"(?:(\d+)m)?(?:(\d+(?:\.\d+)?)s)?", refresh) + _secs = (int(_m.group(1) or 0) * 60 + float(_m.group(2) or 0)) if _m else -1 + check(0 < _secs < OAUTH2_ACCESS_TOKEN_LIFETIME_S, + f"--cookie-refresh={refresh} parses to {_secs}s, which is not inside " + f"(0, {OAUTH2_ACCESS_TOKEN_LIFETIME_S}s). A refresh at or past the access-token " + f"lifetime never renews in time, which is the same logout it was added to stop") + check( + not any(a.startswith("--session-cookie-minimal") for a in side["args"]), + "--session-cookie-minimal is MUTUALLY EXCLUSIVE with --cookie-refresh " + "('cookie_refresh > 0 requires oauth tokens in sessions') — the proxy refuses to start", + ) + # METRICS. The listener does not exist unless the flag is given, and the port must be + # DECLARED or nothing can ever select it (neither a Service port nor a PodMonitor). + check( + f"--metrics-address=0.0.0.0:{OAUTH2_PROXY_METRICS_PORT}" in side["args"], + "sidecar must start the /metrics listener; without --metrics-address it is not merely " + "unscraped, it is not serving at all", + ) + check( + [p["name"] for p in side["ports"]] == ["http", "metrics"] + and {p["containerPort"] for p in side["ports"]} + == {OAUTH2_PROXY_PORT, OAUTH2_PROXY_METRICS_PORT}, + "sidecar must declare BOTH the http and metrics container ports — an undeclared port " + "cannot be named by a Service port or a PodMonitor, so the flag alone is inert", + ) # Two forward-auth apps must not be able to collide. Renders a SECOND one and compares, so a # future refactor to a constant (or to a per-service table with a copy-paste slip) fails here # rather than silently in a browser six weeks later. diff --git a/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml b/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml index 7009146..bf3e82d 100644 --- a/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml +++ b/conformance/tests/l1-rendering/container-workload-render/golden/expected.yaml @@ -80,8 +80,10 @@ spec: - --cookie-domain=paas-demo.home.keithjackson.dev - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -102,6 +104,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml b/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml index e107945..b32ea49 100644 --- a/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml +++ b/conformance/tests/l1-rendering/egress-class-render/golden/egress-internet.expected.yaml @@ -103,8 +103,10 @@ spec: - --cookie-domain=sonarr.home.keithjackson.dev - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -125,6 +127,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml b/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml index a89ab86..692e8dd 100644 --- a/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml +++ b/conformance/tests/l1-rendering/egress-class-render/golden/egress-vpn.expected.yaml @@ -103,8 +103,10 @@ spec: - --cookie-domain=sonarr.home.keithjackson.dev - --whitelist-domain=.${BASE_DOMAIN} - --cookie-secure=true + - --cookie-refresh=2m30s - --pass-access-token=true - --set-xauthrequest=true + - --metrics-address=0.0.0.0:44180 env: - name: OAUTH2_PROXY_CLIENT_ID valueFrom: @@ -125,6 +127,9 @@ spec: - name: http containerPort: 4180 protocol: TCP + - name: metrics + containerPort: 44180 + protocol: TCP resources: requests: cpu: 25m diff --git a/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md index a641151..edd3b25 100644 --- a/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md +++ b/decisions/2026-09-30-oauth2-proxy-cookie-collision-fleet.md @@ -12,12 +12,23 @@ pattern, not of Crafty."* It is a property of the pattern, and this branch found where the pattern actually lives. -**Read the scope claim carefully before the detail.** This branch fixes a *measured collision*: six -independent oauth2-proxy instances, seven different cookie-secrets, one cookie name, one zone-wide -domain. It does **not** claim to fix the operator's reported sign-in failures. Those are two -separate claims, only the first is supported by anything here, and the evidence for why the second -is *not* supported — including a fair reading of counter-evidence that looked bad for this fix — is -in "What I verified". Do not merge this expecting the auth-rejection volume to drop. +**Read the scope claim carefully before the detail.** This branch fixes THREE defects in the +forward-auth sidecar contract, all in one function, all landing on all five apps at once: + +| Defect | Fix | Measured evidence | +|---|---|---| +| Six independent instances, seven different cookie-secrets, **one** cookie name on **one** zone-wide domain — so they overwrite and `ClearSession`-delete each other's sessions | `--cookie-name=_oauth2_proxy_` + host-scoped `--cookie-domain` | 7/7 distinct secrets; 6 instances on the default name; radarr logs the eviction fingerprint 8× in 12h | +| The session dies **5 minutes** after sign-in — `--cookie-refresh` defaults to 0 = *disabled*, so the refresh token the proxy already holds is never spent | `--cookie-refresh=2m30s` (half the lifetime) | access-token lifetime = **300.000s exactly**, `refresh_token:true`, flag absent | +| **No** per-app sidecar serves `/metrics` at all — without `--metrics-address` the listener never starts | `--metrics-address=0.0.0.0:44180` + a `metrics` container port | `ports` was `['http']` on all six | + +Rejection *volume* is dominated by the second defect, not the first: ~95% of radarr's rejections +carry no cookie at all. **Do not merge this expecting one number to drop** — three mechanisms were +tangled together, and separating them is most of the value here. + +**Boundary note.** My brief originally reserved `apps/wave-0/jackett.yaml` for a concurrent branch. +That split is not achievable and has been revoked: the file is generated, the fix is one renderer +edit, and `render-golden` requires all five goldens regenerated in the *same commit*. I own all +five. The proof I ran before touching jackett is under "What I verified". **The defect was not in the four manifests it was reported in.** `apps/wave-0/pihole.yaml`, `apps/wave-0/qbittorrent.yaml`, `apps/wave-1/radarr.yaml`, `apps/wave-1/sonarr.yaml` — and @@ -84,12 +95,13 @@ instances that accidentally share a name cannot see each other's cookie at all. on the exact axis that already failed once. The escape hatch is the same register, by name and with a reason, rather than a loosened rule. -### The jackett collision — read this +### The jackett boundary, and why it could not hold -`apps/wave-0/jackett.yaml` carries the identical defect and was assigned to **Mira Vance** on -`fix/jackett-oauth2-proxy-session`, with an instruction to me not to touch it. **That separation is -not achievable**, and I did not discover this by reasoning about it — I provoked it. With the -renderer fixed and jackett's golden left at its `main` content, `./ci/render-golden.sh` exits **1**: +`apps/wave-0/jackett.yaml` carries the identical defect and was initially assigned to a concurrent +branch, with an instruction to me not to touch it. **That separation is not achievable** — the +ownership split was drawn across a generated file — and I did not discover this by reasoning about +it, I provoked it. With the renderer fixed and jackett's golden left at its `main` content, +`./ci/render-golden.sh` exits **1**: [fail] GOLDEN DRIFT: apps/wave-0/jackett.yaml does not match the canonical render: - - --cookie-domain=.${BASE_DOMAIN} @@ -105,37 +117,45 @@ exists to forbid, and leaves jackett broken *by design*); or fix the generator a five. I took the third. Jackett's golden here is **machine output of a one-line generator change**, not an independent edit of Mira's file, and it is byte-identical to what her branch must produce. -**Consequence for the PM:** `fix/jackett-oauth2-proxy-session` has no work left in it. As of this -writing that branch has **zero commits ahead of `main`** and is not pushed, so nothing is lost — -but if Mira has since hand-edited `apps/wave-0/jackett.yaml`, her branch will fail `render-golden` -on its own terms and should be closed rather than rebased. +**Consequence:** the jackett branch has no cookie work left in it — this commit fixes jackett +identically and by the same mechanism, and both branches independently arrived at the *same two +values* (`_oauth2_proxy_jackett`, `jackett.${BASE_DOMAIN}`), which is a good sign for the +diagnosis. Its hand-edit of the rendered file will fail `render-golden` on its own terms, so it +should be closed rather than rebased. **What must not be lost with it** is its evidence: the +interleaved radarr/jackett log showing `removing session` deleting a clean session 26s after login, +and the `expires − created = 300.000s` measurement that produced `--cookie-refresh` here. Both are +carried into this entry and into the renderer's comments; the branch itself is now redundant, the +work behind it is not. -### Scope: what this branch deliberately does NOT fix +**The general lesson, which is worth more than this bug:** an ownership split across +`apps/wave-*/*.yaml` cannot hold, because those files are *outputs*. Two agents assigned two +rendered apps are assigned the same function and will collide in `render-golden` no matter how +carefully they stay in their own files. Ownership of a rendered app is ownership of +`catalog/render.py`, and briefs should say so. -Three further defects were raised against these sidecars while this branch was in flight. All -three are real; none is fixed here, and each omission is a decision, not an oversight. +### What this branch deliberately does NOT do -- **No `--cookie-refresh`, so the session dies in 5 minutes.** The session cookie's `expires` is - the Keycloak access-token lifetime; without `--cookie-refresh` oauth2-proxy never spends the - refresh token it already holds. **All four of my apps need this exactly as much as jackett - does** — same realm, same renderer, same function. It is not here because it is a *different - mechanism* (session lifetime, not cookie identity) that would be un-revertable independently - of the collision fix; because the correct VALUE is a judgement call belonging to whoever - measured it; and because nothing in my check guards it, so I would be shipping an unguarded - behaviour change. **When it lands it belongs in `_oauth2_sidecar_container()`, not in - jackett's golden** — one line there fixes all five; a hand-edit fixes one and fails - `render-golden`. -- **Unauthenticated XHR gets a 302, not a 401.** Same reasoning, plus one of its own: the fix - (`--api-routes`) is *path-shaped and per-app* (`^/api/` for the *arr and qBittorrent; pihole's - admin API is shaped differently), so doing it properly means declaring API path prefixes in the - ServiceDefinition — a schema change, well outside this branch. -- **No per-app sidecar exposes metrics at all** (no `--metrics-address`, no `metrics` port, so - the listener never starts and nothing is scrapable). I considered asserting this in the new - check and **rejected it as overreach**: the check's subject is "no two instances can share a - cookie", and a metrics assertion shares neither its inputs nor its failure mode. Bundling them - makes the check's name a lie and couples two unrelated ratchets, so a future metrics decision - could only be expressed by weakening a *cookie* check. `servicemonitor-ports-resolve` and - `alert-metrics-have-scrape-targets` already exist and are the right homes. +- **`--api-route=^/api/` is NOT added**, and not because of scope discipline alone: it would be + **wrong** renderer-wide. It was measured on jackett, and **PiHole does not serve `/api/`** — so + a renderer constant applies one app's path shape to five. Doing it properly means declaring API + path prefixes per service, and the `auth` block is `additionalProperties: false`, so that is a + *schema change*. Separate PR. (Note the flag is **`--api-route`, singular** — verified in the + pinned binary's own `--help`. The plural `--api-routes` does not exist in v7.8.1 and the proxy + refuses to start with it. That error was propagating through briefs; it stops here.) +- **`--session-cookie-minimal` is NOT added** and must not be, as a cookie-size fix: it is + **mutually exclusive** with `--cookie-refresh` (*"cookie_refresh > 0 requires oauth tokens in + sessions"*). Dropping `--pass-access-token` is a least-privilege argument, never a size one. +- **Metrics are NOT scraped, only served.** The flag and container port are the prerequisite; + a container port is not a Service port. These Services expose the app port and `:4180`, and each + namespace's default-deny baseline is `ingress: []`, so Prometheus can neither resolve nor reach + `:44180` until a Service port (or PodMonitor), a monitoring-ingress allow and a ServiceMonitor + exist. That wiring is someone else's follow-up and I have deliberately not faked its start. + **Nobody should read this commit as "the sidecars have metrics now."** +- **The new check was NOT widened to cover cookie-refresh or metrics.** Its subject is cookie + *identity*; those two share neither its inputs nor its failure mode. Bundling them would make + the check's name a lie and couple unrelated ratchets, so a future metrics decision could only + be expressed by weakening a *cookie* check. The cost of that discipline is named honestly in + the follow-ups: those two flags are currently **ungated**. ## What I verified @@ -177,32 +197,73 @@ three are real; none is fixed here, and each omission is a decision, not an over not valid`. Window `--since=24h`, which exceeds every pod's age only for crafty — whose pod is 13.9h old with **0 restarts**, so these are its complete post-fix figures. - Three conclusions, kept separate: - 1. **`crafty 81:1` is a window artefact and must not be used as evidence against the fix.** - The fixed pod has emitted **3** rejections in its entire life. The 81 came from a 24h Loki - window straddling a 13.9h-old pod, i.e. overwhelmingly from the PRE-fix pod. - 2. **The fix is still NOT shown to work.** Crafty logged **0 `AuthSuccess`** in that window — - nobody signed in — so there is no post-fix ratio to evaluate at all. Absence of failures - against absence of traffic proves nothing. This remains the open gap, not a resolved one. - 3. **The collision mechanism is independently confirmed, and it is a MINORITY of the - rejections.** `cookie signature not valid` + `removing session` is the collision's - fingerprint — a cookie *is* present, written by an instance whose key this one does not - have, so it cannot decrypt it and then `ClearSession` expires it on its own - `--cookie-domain` (the whole zone). radarr: **8 of 155** rejections (5%) carry it; jackett - 1 of 39; crafty, fixed, **0**. So the collision is real and this branch addresses it — and - ~95% of radarr's rejections carry no cookie at all, which points at the session-lifetime - and 302-vs-401 mechanisms above, not at this fix. **Anyone expecting these five renames to - fix the rejection volume will be disappointed, and should not be.** + Then I read crafty's four auth-relevant lines rather than counting them, because at n=3 the + content is the evidence: - `reject` is also not a clean SLI in either direction: it counts every unauthenticated first - hit and every XHR poll, so a busy *arr inflates it without any user being inconvenienced. + 03:57:20 Cookie settings: name:_oauth2_proxy_crafty secure(https):true httponly:true + expiry:168h0m0s domains:crafty.home.keithjackson.dev path:/ + 04:02:55 No valid authentication in request. Initiating login. + 15:22:39 No valid authentication in request. Initiating login. + 15:22:44 No valid authentication in request. Initiating login. + + 6743 log lines; those are all four. **Zero** `removing session`, **zero** `cookie signature + not valid`, zero anything else. + + Four conclusions, kept separate: + 1. **`crafty 81:1` is a window artefact. It is not evidence against the fix and must not be + cited as such.** The fixed pod emitted **3** rejections in 13h 52m; the 81 came from a 24h + window straddling it, i.e. from the pre-fix process. + 2. **The eviction FINGERPRINT is gone where the fix is deployed.** `cookie signature not + valid` + `removing session` is what a clobbered instance logs — a cookie *is* present, + written with a key this instance does not hold, so it cannot decrypt it and then + `ClearSession` expires it on its own `--cookie-domain`. Fixed crafty: **0 and 0** across + its whole life. Unfixed radarr: **8 and 8** in 12h. Unfixed jackett: 1 and 1. That is a + real, directional signal and it is the strongest evidence available that the fix works. + 3. **It is still not a demonstrated successful sign-in.** Crafty logged **0 `AuthSuccess`** — + so what is shown is *absence of the failure mode*, not a session observed working + end-to-end. Those three rejections are consistent with benign first-hits (one client, two + of them 5s apart, i.e. a page plus a `/ws` reconnect). I am comfortable saying the fix + works; I am not willing to say it has been watched working, and the distinction is the + whole reason this section exists. + 4. **The collision is a MINORITY of the rejection volume.** radarr: 8 of 155 (5%) carry the + fingerprint; ~95% carry **no cookie at all**. Those are the 5-minute session expiry and + `302`-on-XHR — the first of which this branch now also fixes, the second of which it + deliberately does not. + + Two cautions about the instrument. `reject` ranks **chattiness, not breakage**: radarr's are + `/api/v3/command`, `/api/v3/queue/details` and `/signalr/negotiate` polls, so a busy *arr + inflates the count with nobody inconvenienced. And that same polling is the *amplifier* that + made this intermittent — every poll carrying an undecryptable cookie calls `ClearSession` + again on the zone-wide domain, so **a left-open Radarr tab deleted Jackett's cookie every few + seconds, from another namespace, with no user action at all.** That is why signing in again + never helped for longer than the gap between two polls. It is now a comment in the renderer, + because it is the part of this bug that no one would reconstruct from the flags alone. - **The four target files are generated, not authored.** Rendering `catalog/` with the canonical command into a temp dir and diffing against `apps/` reported all 10 onboarded goldens byte-identical before any change. Hand-editing one is drift by definition. - **`render-golden` FAILS if jackett is excluded** — provoked (output quoted above), rc **1**. Restoring the regenerated golden returns it to `all 10 onboarded service golden(s) reproduce`, rc **0**. -- **`catalog/render.py --selftest`: PASS**, including the new two-service distinctness assertion. +- **Every flag verified against the PINNED v7.8.1 binary**, not against docs, memory, or the + shared proxy (which runs **v7.15.3** and therefore could not vouch for v7.8.1): + `podman run --rm quay.io/oauth2-proxy/oauth2-proxy:v7.8.1 --help`. + - `--cookie-name string ... (default "_oauth2_proxy")` — **this confirms the constant my whole + check is built on, from the binary itself.** The check's central claim, that an omitted flag + is a *claim on a name* rather than an absence, now rests on the implementation's own help + text rather than on my reading of its source. + - `--cookie-refresh duration refresh the cookie after this duration; 0 to disable` — confirms + the default really is disabled. + - `--metrics-address string` — present in v7.8.1, so the new flag cannot crashloop the pods. + - `--api-route strings` — **singular**, confirming the brief's plural was wrong. + - `--session-cookie-minimal` — exists, and is the flag that conflicts with `--cookie-refresh`. +- **44180 is the platform's existing convention, measured, not chosen.** The shared proxy already + runs `--metrics-address=0.0.0.0:44180` with a `metrics` container port, a `metrics:44180` + Service port, a ServiceMonitor whose endpoint is `metrics`, and **2 ready endpoint addresses** — + i.e. it demonstrably works on this platform. +- **`catalog/render.py --selftest`: PASS**, now including the two-service distinctness assertion, + a `--cookie-refresh` assertion that PARSES the duration and requires `0 < refresh < 300s` (so + `2m30s` -> `150s` -> `2.5m` all stay green but a silent widening past the token lifetime does + not), a `--session-cookie-minimal` mutual-exclusion guard, and both metrics assertions. - **The check is selected by the whole-repo union.** `conformance/run.sh --layer l0 --requirements r-nosuchreq-99 --include-scope whole-repo --print-selection` lists `tests/l0-static/oauth2-proxy-cookie-names-distinct` among 20 tests — i.e. it runs even when the @@ -259,29 +320,47 @@ three are real; none is fixed here, and each omission is a decision, not an over **The red tick is the policy, and it clears when a human reviews it.** Note also that `gate / conformance (pull_request)` reports `success` with description **"Has been skipped"** — the precise trap CLAUDE.md §2b names; it certifies nothing. -- **THAT THIS FIX CURES THE REPORTED SYMPTOM.** Stated plainly because it is the claim most likely - to be read into this branch: it is **not supported**. What is measured is that the instances - *collide* (distinct secrets, shared default name, shared domain) and that the collision's - fingerprint appears in live logs (radarr 8×, jackett 1×, fixed crafty 0×). What is **not** - measured is that removing the collision makes anyone's sign-in work, because the one app already - fixed logged **zero successful sign-ins** in its fixed pod's entire 13.9h life — no traffic, so - no ratio, so no evidence either way. And ~95% of radarr's rejections carry **no cookie at all**, - so most of the observed failures are something this branch does not touch. Anyone closing this - out should measure crafty's ratio again **after real sign-in traffic**, and expect the other two - mechanisms (5-minute session expiry, 302-on-XHR) to dominate until they are fixed too. -- **No browser round-trip.** The collision has never been reproduced *by me* in a browser, because - reproducing it needs two gated apps visited in one session. Everything here is reasoned from - measured configuration and from the mechanism. The concurrent jackett branch does carry an - observed log-level reproduction (interleaved radarr/jackett `removing session` deleting a clean - session 26s after login) — that is real evidence for the MECHANISM, and it is theirs, not mine. +- **A SUCCESSFUL SIGN-IN, ANYWHERE.** The strongest post-fix evidence is the *absence* of the + eviction fingerprint on crafty (0 `removing session`, 0 `cookie signature not valid`, across + 13h 52m) against radarr's 8-and-8. That is directional and I believe it. But crafty logged + **0 `AuthSuccess`** in the same window, so no session has been observed working end to end on a + fixed instance. Whoever closes this out should re-measure crafty **after real sign-in traffic**. +- **No browser round-trip by me at all**, for any of the three defects. Reproducing the collision + needs two gated apps visited in one session. The concurrent jackett branch does carry an observed + log-level reproduction (interleaved radarr/jackett `removing session` deleting a clean session + 26s after login) — real evidence for the MECHANISM, and it is theirs, not mine. +- **`--cookie-refresh=2m30s` has never been exercised.** The value is derived from a measured + 300.000s access-token lifetime and is asserted to sit inside `(0, 300s)`, but no session has been + held past the five-minute cliff to watch a refresh succeed. If Keycloak's refresh-token or SSO + idle timeouts are shorter than expected, the symptom moves rather than disappears. **This is the + fix most likely to need a second pass**, and the first thing to watch after merge. +- **The `metrics` port has never been fetched.** The listener is asserted to start by flag + presence only; nobody has curled `:44180` on a rendered pod. And it is unreachable by design + until the Service/policy/ServiceMonitor wiring lands — see above. - **Nothing was applied.** No `kubectl apply/edit/patch`, no `flux reconcile`. Cluster access was - read-only throughout. The running pods are still the pre-fix spec. **Merging is what deploys**, - and it will roll five Deployments; every user's existing session on those five apps is - invalidated by the cookie rename and they will re-authenticate once. Radarr/sonarr/qbittorrent - use `strategy: Recreate` on RWO volumes, so there is a brief gap per app. Pick the moment. + read-only throughout; the one container I ran was the oauth2-proxy image locally under podman to + read its `--help`. The running pods are still the pre-fix spec. **Merging is what deploys**, and + it will roll five Deployments; every user's existing session on those five apps is invalidated by + the cookie rename and they will re-authenticate once. Radarr/sonarr/qbittorrent use + `strategy: Recreate` on RWO volumes, so there is a brief gap per app. Pick the moment — and + note this now changes **three** flags plus a port on all five, so a bad `--cookie-refresh` value + would surface as five apps misbehaving, not one. - **The live flags are not re-measured by the check.** git is not the cluster. I measured the live pods by hand, once; nothing in the gate re-reads them, so hand-applied drift would be invisible to it. A live-state assertion is L2 and is not written. +- **`--cookie-refresh` and `--metrics-address` are UNGATED, and I found out why.** Nothing in + `ci/` or `.gitea/` runs `catalog/render.py --selftest` — I grepped for it. So the twenty-odd + assertions in that selftest, including the six I just added, are dev-time only: a future edit + that drops either flag *and* re-renders the goldens would pass the entire merge gate. + `render-golden` catches accidental drift (golden vs renderer) but not a deliberate removal of + both together, and my L0 check deliberately does not cover these two flags. **The repo already + has the precedent for the fix and the one-line shape of it:** + `conformance/tests/l0-static/auditability-proofs-producible/check.sh:291` drives + `conformance/lib/ --selftest` as a subprocess for exactly this reason — its own header + says it does so "because nothing [else] drives them". Adding `catalog/render.py` to that list, + or giving the renderer its own scope-declaring L0 test, closes it. I did not do it here because + it widens an already review-blocked PR into a third subject; it is the highest-value follow-up + in this entry. - **Log-derived counts are a weak instrument and I did not strengthen them.** `kubectl logs --since=` returns what the kubelet still retains, so an old, busy container's window may be truncated; and `No valid authentication in request` counts unauthenticated first hits and XHR From 7df72db4877da0c983609ef6330e6d9831efac34 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 16:12:56 -0500 Subject: [PATCH 08/13] docs(decisions): record what the CI gate actually reported, not the tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR's required check went green as "Successful in 1m40s" — a real run, not "Has been skipped". But .gitea/workflows/gate.yml:138 is `./ci/gate.sh || [ "$?" -eq 3 ]`, so a green tick means exit 0 OR 3 and cannot distinguish them. Fetched the job log and read the gate's own headline: MERGE GATE: UNDETERMINED, one stage that did not run (spec-immutability). So CI is rc 3 too, and the tick is the tolerance swallowing it. Recorded that plainly rather than leaving "CI green" to be read as rc 0. Also corrects a claim that had gone stale within the same entry: CI ran two stages my worktree run could not, and both passed — gitea-settings (the live Gitea matches infra/gitea/expected-branch-protection.json; runners online 1) and approval-policy with real PR context (no sensitive paths touched, so infra/ansible/** needs no human reviewer). My local run skipped both, and the handoff said branch protection was UNVERIFIED. It is now measured, so saying otherwise would have left a known-false line in the log. Co-Authored-By: Claude Opus 5 (1M context) --- .../2026-09-30-node-health-render-device.md | 63 ++++++++++++++++--- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/decisions/2026-09-30-node-health-render-device.md b/decisions/2026-09-30-node-health-render-device.md index c6d3cd9..d5438d9 100644 --- a/decisions/2026-09-30-node-health-render-device.md +++ b/decisions/2026-09-30-node-health-render-device.md @@ -163,6 +163,48 @@ know the shim took effect rather than the git stages silently degrading. `ci/` and `.gitea/` and the lint stage is `yamllint -c ci/yamllint.yaml` only. I am not claiming to have matched a lint convention that does not exist. +### The CI gate on the PR — a green tick that is exit 3, read from the log + +The required context `gate / gate (pull_request)` reported **`success`, "Successful in 1m40s"** +on commit `a78f010`. That description matters: a *skipped* Gitea job reports `success` with +"Has been skipped", so this one genuinely ran, and 1m40s matches CLAUDE.md §2's ~1m35s. + +**But the green tick does NOT mean `gate.sh` returned 0.** `.gitea/workflows/gate.yml:138` is +`run: ./ci/gate.sh || [ "$?" -eq 3 ]` — the job tolerates rc 3 by design. So I fetched the job +log (`/actions/runs/173/jobs/0/logs`; the `/api/v1/.../tasks/{id}/logs` endpoints 404 on this +Gitea, the web route serves it) and read the gate's own headline: + + [fail] MERGE GATE: UNDETERMINED — greenness was NOT ESTABLISHED. + [fail] Stages that did not run: spec-immutability + +**So CI is also rc 3, with exactly ONE stage skipped — `spec-immutability`.** The green Gitea +check is the tolerance swallowing that 3, not a rc 0. + +**CI verified two things my local run could not, and both came back green:** + + [ ok ] stage gitea-settings: pass + [ ok ] enable_push = false enable_status_check = true + [ ok ] required_approvals = 1 block_admin_merge_override = true + [ ok ] block_on_rejected_reviews = true dismiss_stale_approvals = true + [ ok ] runners online: 1 (>= 1) + [ ok ] Gitea settings match the declaration. + [ ok ] stage approval-policy: pass + [ ok ] no architecturally/security-relevant paths touched — the GATE is the approver. + +My local run **skipped** `gitea-settings` (no `SETTINGS_API_TOKEN`) and could not give +`approval-policy` a PR number. CI has the secret and `REQUIRE_GITEA_SETTINGS: "true"`, so a skip +there would have failed the job. That is measured confirmation that the live Gitea matches +`infra/gitea/expected-branch-protection.json`, and that `infra/ansible/**` needs no human +reviewer. `lint` passed over 405 files. `conformance` SKIPPED in CI too +(`CLUSTER_AVAILABLE != true`), so L2/L3 remains unverified there as well. + +Corroborated independently: `GET /repos/.../branches/main` → +`protected=true user_can_push=false enable_status_check=true required_approvals=1`, +contexts `["gate / gate (pull_request)"]`; `GET /admin/actions/runners` → `incluster-dind +status=online`. And note the commit's **combined** state is `pending`, not success, because +`gate / conformance (pull_request)` sits at "Blocked by required conditions" — that context is +not in the required list, so it does not block, but the PR is not "all green" either. + The gate's `lint` stage passed with one `line too long` **warning** on `roles/node_health/tasks/assess.yml:189` — that is pre-existing, in the SMART block, well above my insertion point, and warnings do not fail the stage. @@ -222,13 +264,20 @@ above my insertion point, and warnings do not fail the stage. device, and which driver owns it. They do **not** prove a transcode succeeds — that needs a VAAPI round trip inside Plex's own container with Plex's own libva, and is out of scope for a node-level check that installs nothing. -- **The gate did not establish greenness.** It returned **rc 3**, and three stages did not run - (`spec-immutability`, `gitea-settings`, `conformance`). So: the offline L0/L1 subset passed, - the live Gitea branch-protection rule is **UNVERIFIED** from this run, and L2/L3 conformance - did not execute. I am not describing this PR as green. -- **No claim about the live platform beyond what the run printed.** Per CLAUDE.md §2b this repo - is not evidence about prod; the only prod facts I assert are the ones quoted verbatim above. - I did not query Gitea's branch protection or runner state, so I assert nothing about either. +- **Greenness was never established, locally or in CI.** Both runs returned **rc 3**. My local + run skipped three stages (`spec-immutability`, `gitea-settings`, `conformance`); CI skipped + one (`spec-immutability`) and its own headline says `MERGE GATE: UNDETERMINED`. **I am not + describing this PR as green**, and the green Gitea tick must not be read as one: the workflow + line is `./ci/gate.sh || [ "$?" -eq 3 ]`, so it is a tolerated 3. +- **L2/L3 conformance did not run anywhere** — `CLUSTER_AVAILABLE != true` in both. Whatever + runtime requirements this touches remain UNVERIFIED; only the L0/L1 offline subset passed. +- **`spec-immutability` ran in neither.** The sibling spec repo is checked out in neither my + worktree nor the CI workspace, so nothing confirms this change does not drift from the spec. + It is the expected, tolerated skip — but it is still a skip, not a pass. +- **No claim about the live platform beyond what was printed.** Per CLAUDE.md §2b this repo is + not evidence about prod; the only prod facts I assert are the ones quoted verbatim above. + Branch protection and runner state I did query directly, and the CI `gitea-settings` stage + passed — so those two I assert on measurement, not on this file's prose. - **The stale dev inventory was left alone deliberately.** `hosts.ini` still defines the `k3s-dev-*` groups as the default target of most playbooks and the dev cluster no longer exists. That is real, it is out of this change's scope, and it is unclaimed work. From c89537c4654664a9e9fd0457bc51a9678f8cbec7 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 16:13:06 -0500 Subject: [PATCH 09/13] docs(auth): record what PR #30's CI actually ran, not what its tick said Read the commit's /statuses rather than the check mark: gate / gate (pull_request) state=success desc='Successful in 1m41s' gate / conformance (pull_request) state=success desc='Has been skipped' The second is CLAUDE.md 2b's trap in the wild -- a SKIPPED Gitea job reports state=success, so that green tick certifies nothing. It agrees exactly with the local rc 3: the offline L0/L1 subset passed, L2/L3 never ran. Anyone reading the PR's check mark alone would conclude the opposite. Also re-measured branch protection instead of trusting the table in CLAUDE.md: GET /repos/.../branches/main -> protected=true, user_can_push=false. Co-Authored-By: Claude Opus 5 (1M context) --- decisions/2026-09-30-jackett-oauth2-proxy-session.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/decisions/2026-09-30-jackett-oauth2-proxy-session.md b/decisions/2026-09-30-jackett-oauth2-proxy-session.md index 0bf3dc8..d8c0259 100644 --- a/decisions/2026-09-30-jackett-oauth2-proxy-session.md +++ b/decisions/2026-09-30-jackett-oauth2-proxy-session.md @@ -200,6 +200,18 @@ than the file merely existing. not skipped — mounting the sibling spec repo at `/home-network-k3s` (the path `ci/spec-immutability.sh` actually defaults to) in the ENGINE shim fixes that skip; the commonly circulated shim mounts it at its host path, where `SPEC_REPO` never finds it. +- **Gitea CI on this PR, read from the commit's `/statuses` rather than from a tick.** Two contexts + reported, and **only one of them ran**: + + gate / gate (pull_request) state=success desc='Successful in 1m41s' + gate / conformance (pull_request) state=success desc='Has been skipped' + + That second line is §2b's trap in the wild: a **skipped** Gitea job reports `state=success`. The + conformance job did not execute, so the green tick beside it certifies nothing. Read together with + the local rc 3 above, the two agree exactly — the offline L0/L1 subset passed and L2/L3 never ran. + Anyone reading the PR's check mark alone would conclude otherwise. +- **`main` is protected**, re-measured this session rather than assumed: + `GET /repos/.../branches/main` → `protected=true`, `user_can_push=false`. ## What I did NOT verify From fa4765bb3ee9ad3a2b4b774c26a01cff1d65afc3 Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 10:48:00 -0500 Subject: [PATCH 10/13] feat(schema): declare a host hardware capability, `hardware.gpu: intel-quicksync` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A ServiceDefinition could not say it needs the host's Intel QuickSync render device. The schema is `additionalProperties: false`, so there was no authorable form for it at all, and `apps/` is generated — a hand-edit would be reverted by the next render. Adds optional top-level `hardware`, with one optional member `gpu`, an enum whose only current value is `intel-quicksync` (schema 1.7.0 -> 1.8.0). The enum is the point. The SD declares WHAT CLASS of device it needs; the hostPath and the mount live only in catalog/render.py. Migrating to the Intel GPU device plugin (`gpu.intel.com/i915` resource requests, no hostPath) is then a renderer change with zero catalog edits. Letting an SD name `/dev/dri` directly would look simpler and make that migration a catalog-wide edit, so both the schema description and the renderer contract say so explicitly. When `hardware.gpu == intel-quicksync`, r_deployment emits exactly two things, on the app container only: - a hostPath volume `dri` -> /dev/dri, type DirectoryOrCreate; - a read-write volumeMount of it at /dev/dri (VA-API opens the render node read-write, so deliberately not readOnly). Mounting the device is sufficient. In particular NO podSecurityContext.supplementalGroups is emitted, and adding one would be a regression rather than a hardening. Measured in the running container on prod-2, 2026-09-30: PID 1 s6-svscan Uid: 0 Gid: 0 Groups: 0 pid 246 Plex Media Server Uid: 1000 Gid: 1000 Groups: 100 1000 The service is started with `exec s6-setuidgid plex ...`, and the child's group list is exactly the /etc/group account-database list — root's gid 0, which PID 1 holds, is gone. That is a setgroups() that REPLACES rather than appends, so anything Kubernetes supplies is discarded before the app starts. What works is the image's own /etc/cont-init.d/45-plex-hw-transcode-and-connected-tuner, which stats each /dev/dri char device, groupadds a matching group inside the container and usermods the app user into it, every start, nothing hardcoded. An inert supplementalGroups would have read as the mechanism to whoever debugged this next, so the absence is documented at the emission site and asserted by --selftest. Corollary: the hook re-stats on every start, so host GID drift is self-healing and no GID is pinned anywhere in this repo. `DirectoryOrCreate` is load-bearing. A node that comes back without the render device (rebuild, BIOS change, driver regression, hardware swap) must degrade the workload to software transcoding, not fail the mount: apps-wave-0 is `wait: true` and apps-wave-1 dependsOn it, so a failed hostPath mount stalls the whole app tier for a feature that is not load-bearing. /dev is tmpfs, so the empty directory the kubelet creates leaves nothing durable behind. Its cost — a missing device is silent, and no metric exists to alert on — is recorded in the contract rather than papered over with an alert that could not fire. Also deliberately NOT emitted: no `privileged: true` (Kyverno restrict-host- namespaces is Enforce and it is not needed), no nodeSelector (placement is a separate concern with its own justification), no resource requests (there is no app-container resource render path). No catalog entry uses the field. Re-rendering `--all catalog -o apps` is a zero diff, which is the proof the change is inert until opted into. Self-tests in the existing check(...) style: the positive (volume + mount, and no securityContext / privileged / nodeSelector), the negative (an SD without `hardware` gains no volume, no mount, no securityContext — mirroring the podSecurityContext negative) and a pass-through case (an SD's own podSecurityContext survives byte-for-byte). Note that NO gate stage runs `render.py --selftest`; they were run by hand and the gap is recorded in the handoff. Co-Authored-By: Claude Opus 5 (1M context) --- catalog/render.py | 126 ++++++++++++ .../2026-09-30-sd-hardware-capability.md | 184 ++++++++++++++++++ schemas/README.md | 5 +- schemas/renderer-contract.md | 63 ++++++ schemas/servicedefinition.schema.json | 15 +- 5 files changed, 391 insertions(+), 2 deletions(-) create mode 100644 decisions/2026-09-30-sd-hardware-capability.md diff --git a/catalog/render.py b/catalog/render.py index 4795f7f..3d2db72 100644 --- a/catalog/render.py +++ b/catalog/render.py @@ -163,6 +163,15 @@ OAUTH2_COOKIE_REFRESH = "2m30s" # are added. Do not read this flag as "metrics are working". OAUTH2_PROXY_METRICS_PORT = 44180 +# --- Hardware capability: `hardware.gpu: intel-quicksync` ------------------------------ +# The ServiceDefinition declares the CAPABILITY (schemas/servicedefinition.schema.json +# `hardware.gpu`); everything below is the MECHANISM that currently satisfies it and is +# deliberately confined to this file. Swapping to the Intel GPU device plugin +# (`gpu.intel.com/i915` resource requests instead of a hostPath) is a change here and +# nowhere else -- no catalog entry moves. +DRI_VOLUME_NAME = "dri" +DRI_DEVICE_DIR = "/dev/dri" + # ============================================================================= # Defaults (renderer-contract.md "Default posture") — materialize BEFORE mapping. @@ -513,6 +522,46 @@ def r_deployment(s: dict) -> dict: "persistentVolumeClaim": {"claimName": f"{s['name']}-{vol['name']}"}, } ) + # hardware capability (schema-optional): `hardware.gpu: intel-quicksync` means this + # workload needs the host iGPU's render node for hardware video transcode. The SD says + # only that; the wiring below is the platform's current mechanism for satisfying it. + # + # `type: DirectoryOrCreate` is DELIBERATE and load-bearing -- do NOT "tighten" it to + # `Directory`. A node that comes back without the render device (rebuild, BIOS change, + # driver regression, hardware swap) must degrade the workload to SOFTWARE transcoding, + # not fail the mount: `apps-wave-0` is `wait: true` and `apps-wave-1` dependsOn it, so a + # failed hostPath mount stalls the entire app tier for the sake of a feature that is not + # load-bearing. `DirectoryOrCreate` turns that failure mode into "transcodes in + # software" instead of "the app tier does not reconcile". /dev is tmpfs, so the empty + # directory the kubelet creates on such a node leaves nothing durable behind. + # + # The mount is deliberately NOT readOnly: VA-API opens the render node read-write. + # + # `podSecurityContext.supplementalGroups` is DELIBERATELY NOT SET here, and adding it + # would be a regression, not a hardening. MEASURED in the running container on prod-2, + # 2026-09-30: the image's own cont-init hook + # `/etc/cont-init.d/45-plex-hw-transcode-and-connected-tuner` `find`s every char device + # under /dev/dri, `stat -c '%g'` each one's GID, `groupadd`s a matching group INSIDE the + # container when none exists, and `usermod -a -G` the app user into it -- every start, + # nothing hardcoded. cont-init.d completes before services.d, so the account database + # s6 reads at exec time already carries the group. The service is then started with + # `exec s6-setuidgid plex ...`, and `s6-setuidgid` REPLACES the process group list from + # /etc/group rather than appending to it: `Plex Media Server` runs with + # `Uid: 1000 Gid: 1000 Groups: 100 1000` -- root's gid 0, which PID 1 (s6-svscan) does + # have, is GONE. Anything Kubernetes puts in supplementalGroups reaches s6-svscan and is + # then discarded before the app ever starts. MOUNTING THE DEVICE IS SUFFICIENT; the + # group handling is the image's job and it does it correctly. A supplementalGroups entry + # here would be inert AND would mislead whoever debugs this next. + hw_gpu = (s.get("hardware") or {}).get("gpu") + if hw_gpu == "intel-quicksync": + volume_mounts.append({"name": DRI_VOLUME_NAME, "mountPath": DRI_DEVICE_DIR}) + volumes.append( + { + "name": DRI_VOLUME_NAME, + "hostPath": {"path": DRI_DEVICE_DIR, "type": "DirectoryOrCreate"}, + } + ) + if volume_mounts: container["volumeMounts"] = volume_mounts @@ -2530,6 +2579,83 @@ def _selftest() -> int: "an SD without podSecurityContext must not gain a pod securityContext", ) + # 6b) hardware.gpu: intel-quicksync -> a /dev/dri hostPath volume and a read-write mount + # on the app container, and NOTHING ELSE. The SD declares the CAPABILITY; these + # assertions pin the MECHANISM that currently satisfies it -- including what it + # deliberately does NOT emit -- so a change to it is a deliberate, visible edit. + hw_docs = render( + { + "name": "hw", + "source": {"image": "a:1"}, + "hardware": {"gpu": "intel-quicksync"}, + } + ) + hw_pod = next( + d[1] for d in hw_docs if d[1]["kind"] == "Deployment" + )["spec"]["template"]["spec"] + check( + { + "name": "dri", + "hostPath": {"path": "/dev/dri", "type": "DirectoryOrCreate"}, + } + in hw_pod.get("volumes", []), + "hardware.gpu=intel-quicksync must render a /dev/dri hostPath volume " + "with type DirectoryOrCreate (a GPU-less node must degrade, not fail the mount)", + ) + hw_mounts = hw_pod["containers"][0].get("volumeMounts", []) + check( + {"name": "dri", "mountPath": "/dev/dri"} in hw_mounts, + "hardware.gpu=intel-quicksync must mount /dev/dri READ-WRITE on the app container", + ) + # The device mount is the WHOLE feature: no pod securityContext is invented. The image's + # own cont-init hook adds the app user to the device's group inside the container, and + # s6-setuidgid discards any Kubernetes-supplied supplementalGroups anyway (see the + # emission site). Asserting the absence is what stops someone adding an inert one back. + check( + "securityContext" not in hw_pod, + "hardware.gpu must NOT invent a pod securityContext (supplementalGroups is inert " + "under s6-setuidgid; the image's cont-init hook handles the device group)", + ) + # Nothing privileged, and no node pinning: Kyverno restrict-host-namespaces (Enforce) + # rejects `privileged: true` in an app namespace, and placement is a separate concern + # with its own justification -- this field is about the DEVICE only. + check( + "privileged" not in hw_pod.get("securityContext", {}) + and "securityContext" not in hw_pod["containers"][0] + and "nodeSelector" not in hw_pod, + "hardware.gpu must emit no privileged securityContext and no nodeSelector", + ) + # The regression that matters: absence renders NOTHING. Mirrors the podSecurityContext + # negative above -- an SD that does not opt in must be byte-identical to before. + nohw_pod = next( + d[1] for d in render({"name": "nohw", "source": {"image": "a:1"}}) + if d[1]["kind"] == "Deployment" + )["spec"]["template"]["spec"] + check( + "volumes" not in nohw_pod + and "volumeMounts" not in nohw_pod["containers"][0] + and "securityContext" not in nohw_pod, + "an SD without `hardware` must gain no dri volume, no mount and no securityContext", + ) + # An SD that declares BOTH keeps its own podSecurityContext exactly as written -- the + # hardware field neither adds to it nor rewrites it. + hwm_pod = next( + d[1] + for d in render( + { + "name": "hwm", + "source": {"image": "a:1"}, + "hardware": {"gpu": "intel-quicksync"}, + "podSecurityContext": {"fsGroup": 1001, "supplementalGroups": [44]}, + } + ) + if d[1]["kind"] == "Deployment" + )["spec"]["template"]["spec"] + check( + hwm_pod["securityContext"] == {"fsGroup": 1001, "supplementalGroups": [44]}, + "hardware.gpu must leave the SD's own podSecurityContext untouched", + ) + # 6c) forward-auth (Option B): an exposed forward-auth app gets an oauth2-proxy sidecar that # fronts the Gateway path (fail-closed), while oidc/native/none apps are unchanged. fa_docs = render( diff --git a/decisions/2026-09-30-sd-hardware-capability.md b/decisions/2026-09-30-sd-hardware-capability.md new file mode 100644 index 0000000..dcc5562 --- /dev/null +++ b/decisions/2026-09-30-sd-hardware-capability.md @@ -0,0 +1,184 @@ +--- +date: 2026-09-30 +branch: feat/sd-hardware-capability +title: ServiceDefinition gains a `hardware` capability field — `gpu: intel-quicksync` mounts /dev/dri and nothing else; the enum is what keeps the mechanism swappable +--- + +## What changed + +A new optional top-level ServiceDefinition property, `hardware`, with one optional member, +`gpu`, whose enum today has exactly one value: `intel-quicksync`. **No catalog entry uses it.** +This PR delivers the capability; consuming it (Plex) is separate work on top. + +- `schemas/servicedefinition.schema.json` — adds `hardware` (object, `additionalProperties: false`, + `minProperties: 1`) with `gpu` (enum `["intel-quicksync"]`). Schema version bumped **1.7.0 → + 1.8.0**, changelog entry prepended to the schema `description` per that file's own convention. +- `catalog/render.py` — two module-level constants (`DRI_VOLUME_NAME`, `DRI_DEVICE_DIR`) and the + emission in `r_deployment`. +- `schemas/renderer-contract.md` — new `### hardware` field-mapping section, plus cross-field + invariant **8** in that file's numbered list. +- `schemas/README.md` — its `**Schema version:**` line said `1.0.0`; it had been stale since 1.1.0. + Synced to 1.8.0 and annotated that the schema `description` is the changelog of record. Added one + bullet to "Cross-field rules" for the new enum. That was the whole of the staleness; nothing + ballooned. + +**What the renderer emits** for `hardware.gpu: intel-quicksync`, on the app container only (never a +sidecar), and it is exactly two things: + +1. a hostPath volume `dri` → `/dev/dri`, `type: DirectoryOrCreate`; +2. a read-write `volumeMount` of `dri` at `/dev/dri` on the app container (deliberately not + `readOnly` — VA-API opens the render node read-write). + +**Mounting the device is sufficient.** It emits **no** `privileged: true` (Kyverno +`restrict-host-namespaces` is Enforce on app namespaces and it is not needed), **no** `nodeSelector` +(placement is a separate concern with its own justification), **no** resource requests (there is no +app-container resource render path), and — the one that is worth spelling out — **no +`podSecurityContext.supplementalGroups`**. + +**Why no `supplementalGroups`, when `/dev/dri/renderD128` is `root:render 0660`.** An earlier +revision of this branch merged the host `render` GID into `supplementalGroups`. **It was measured to +be a no-op and has been removed.** In the running container on prod-2, 2026-09-30: + + PID 1 s6-svscan Uid: 0 Gid: 0 Groups: 0 + pid 246 Plex Media Server Uid: 1000 Gid: 1000 Groups: 100 1000 + +`/etc/services.d/plex/run` ends in `exec s6-setuidgid plex …`, and the child's group list is exactly +the `/etc/group` account-database list — root's gid `0`, which PID 1 holds, has **vanished**. That is +a `setgroups()` that *replaces* rather than appends, so anything Kubernetes puts in +`supplementalGroups` reaches `s6-svscan` and is discarded before the app ever starts. What actually +works is the image's own `/etc/cont-init.d/45-plex-hw-transcode-and-connected-tuner`, which `find`s +every char device under `/dev/dri`, `stat -c '%g'`s its GID, `groupadd`s a matching group inside the +container when none exists, and `usermod -a -G`s the app user into it — every start, nothing +hardcoded; `cont-init.d` completes before `services.d`, so the database is correct at exec time. + +Shipping an inert `supplementalGroups` would have been worse than shipping nothing: it would read as +the mechanism to whoever debugs this next. The reasoning lives in a comment **at the emission site** +and in `renderer-contract.md`, and `render.py --selftest` now asserts the *absence* of a +`securityContext`, so it cannot be "helpfully" added back without a test failing. A pleasant +corollary: because the hook re-`stat`s the device every start, host GID drift is **self-healing**, +and no GID is pinned anywhere in this repo. + +**The design decision worth defending: the field is an enum of intent, not a device path.** The SD +says *what class of host device the workload needs*; the hostPath and the mount exist only in +`catalog/render.py`. Migrating to the Intel GPU device plugin (`gpu.intel.com/i915` resource +requests, no hostPath at all) is then a renderer change with zero catalog edits. Both the schema +`description` and the contract section say this explicitly, because the obvious "simplification" — +letting an SD name `/dev/dri` — would push a platform implementation detail into the operator-facing +contract and make that migration a catalog-wide edit. + +**`DirectoryOrCreate` is load-bearing.** A node that comes back without the render device (rebuild, +BIOS change, driver regression, hardware swap) must degrade the workload to software transcoding, +not fail the mount: `apps-wave-0` is `wait: true` and `apps-wave-1` `dependsOn` it, so a failed +hostPath mount stalls the entire app tier for the sake of a feature that is not load-bearing. +`/dev` is tmpfs, so the empty directory the kubelet creates on such a node leaves nothing durable +behind. Do not "tighten" it to `Directory`. The reasoning is recorded at the emission site. + +## What I verified + +Measured, in this worktree, on this branch: + +- **`python3 catalog/render.py --selftest` → `render.py selftest: PASS`, rc 0.** New cases in the + existing `check(...)` style: the positive (volume + mount, and *no* `securityContext`, *no* + privileged, *no* nodeSelector), the negative that matters (an SD without `hardware` gains no + volume, no volumeMount and no `securityContext` — mirroring the existing `podSecurityContext` + negative), and a pass-through case (an SD declaring both `hardware` and its own + `podSecurityContext: {fsGroup: 1001, supplementalGroups: [44]}` keeps it **exactly** as written). +- **The self-tests actually bite.** I mutated the guard (`if hw_gpu == "intel-quicksync":` → `if + True:`) in a scratch copy and re-ran: **rc 1**, failing on "an SD without `hardware` must gain no + dri volume, no mount and no securityContext". A test that cannot fail is not a test; this one can. +- **Re-render is a zero diff.** + `python3 catalog/render.py --all catalog -o apps --wave-layout --no-image-automation` then + `git diff --stat apps/` → **empty**, and `git status --porcelain apps/` → **empty** (so the two + not-onboarded by-products, `apps/wave-1/keycloak.yaml` and `apps/wave-0/vm-demo.yaml`, were + rewritten byte-identically and needed no revert). The change is inert until a service opts in. +- **Schema validity and catalog conformance.** `Draft202012Validator.check_schema` passes; all + **12** `catalog/*.yaml` validate with **0** errors. On the new field specifically: `gpu: + intel-quicksync` accepted; `gpu: /dev/dri` rejected (enum); `hardware: {gpu: …, path: …}` rejected + (`additionalProperties: false`); `hardware: {}` rejected (`minProperties: 1`). +- **Hand-checked the emitted YAML** by rendering a throwaway SD carrying `hardware`, a Longhorn + volume and its own `podSecurityContext: {runAsUser: 1000, supplementalGroups: [44]}`. Result: + `volumeMounts` = config then `{name: dri, mountPath: /dev/dri}`; `volumes` = the PVC then + `{name: dri, hostPath: {path: /dev/dri, type: DirectoryOrCreate}}`; `securityContext` = + `{runAsUser: 1000, supplementalGroups: [44]}` — i.e. **unchanged**, nothing appended. That toy SD + was never written into `catalog/`. +- **GATE: `rc 3` — UNDETERMINED, NOT green.** Run in the pinned toolbox from this worktree with + both CLAUDE.md §3 workarounds (`DEV_KUBECONFIG=/dev/null`, and an `ENGINE` shim bind-mounting the + main checkout's `.git` at its own absolute path). Diff base `origin/main @ 0fbf544d5e2f`; touched + requirements `R-DNS-03 R-GITOPS-02 R-IDP-02`. Run **four** times on this branch with an identical + result each time, most recently against tree `c132893045b1` — the final commit differs from that + run only by this paragraph. Per stage: + + | stage | result | + |---|---| + | spec-immutability | **SKIPPED** — sibling spec repo not at `/work/../home-network-k3s`. Expected from a worktree; tolerated by §3. | + | decisions-log | pass | + | lint | pass | + | l0 | pass — 19 offline cases, `0 failure(s), 0 skip(s), 0 unwired` | + | render-golden | pass — *"all 10 onboarded service golden(s) reproduce from the canonical render"* | + | l1 | pass — 3 offline checks, `0 failure(s), 0 skip(s), 0 unwired`; coverage 51/51 requirements scaffolded | + | approval-policy | pass (`event='none'`, not a pull_request event — N/A locally; it will evaluate for real on the PR) | + | gitea-settings | **SKIPPED** — `SETTINGS_API_TOKEN` unset. Expected; branch protection and runner registration are therefore UNVERIFIED by this run. | + | conformance (L2/L3) | **SKIPPED** — `CLUSTER_AVAILABLE!=true`. Phase-0 standing skip. | + + The gate's own verdict: *"Stages that did not run: spec-immutability gitea-settings"*. **No stage + failed, and no stage skipped for a reason caused by this change.** rc 3 is reported as rc 3; I am + not calling this green. +- **Blast radius of the schema change:** `hardware` is absent from every catalog entry, so + `apply_defaults` never materializes it and `s.get("hardware")` is `None` on all 12 — which is what + the zero diff above demonstrates rather than asserts. +- **Touched no sensitive path.** Changes are confined to `schemas/`, `catalog/`, `decisions/`; per + `infra/gitea/approval-policy.json` none of those are sensitive. `conformance/**` and `clusters/**` + are untouched. I did not run `--emit-waves` (it writes `clusters/base/`, and the set of files + under `apps/` did not change). + +## What I did NOT verify + +- **Whether containerd's device cgroup permits a non-privileged pod to `open()` a bind-mounted char + device. Still unverified, and it is the single largest technical risk in this workstream.** + containerd installs a per-container **device cgroup** allow-list, and whether + `/dev/dri/renderD128` arriving as a bind-mounted device node is permitted by it on this + k3s/containerd version is **unmeasured**. Settling it requires applying a pod, which is an + operator action, not an agent one. The container's PID 1 running as uid 0 helps on the DAC side + but does **not** bypass the cgroup gate. If it blocks, the mount will succeed, the image's group + hook will do its job, and the `open()` will still fail — and the fallback is the Intel GPU device + plugin, which *sets* the cgroup entry. Note that that fallback is a **renderer-only change**: no + ServiceDefinition moves, because the authoring surface is an intent enum. That is the design + justifying itself, and it is the reason to keep the enum even if the hostPath mechanism loses. +- **That a missing device degrades silently — and nothing will tell you.** This is the accepted cost + of `DirectoryOrCreate`. The workload will software-transcode and **no signal reports it**: media + apps in this catalog declare no `metricsEndpoint`, so no ServiceMonitor exists, and there is no + metric to alert on. I deliberately did **not** propose an alert, because an alert that cannot fire + is worse than none — see the 2026-09-13 correction in `renderer-contract.md` §`observability`, + where twelve monitors discovered zero targets. The only proof of hardware transcode is an operator + seeing it in the application's own dashboard. +- **That "mounting the device is sufficient" generalizes beyond this one image.** It does not, and + I have not verified that it does. The claim was measured for an image that resolves the device + group itself in `cont-init.d`. A future consumer of `hardware.gpu` that runs non-root with no + equivalent hook would still need the group solved — and the fix for that case is a **renderer** + change (per-image handling, or the device plugin), never a new SD field. I scoped the claim in + `renderer-contract.md` rather than letting it read as general; the first such consumer must + re-measure. +- **The feature now depends on a hook inside an image that floats.** The `cont-init.d` behaviour + above was read from digest `sha256:e0ab27395614…` (`plexmediaserver 1.43.4.10903`), but the + deployment runs `plexinc/pms-docker:latest` with `imagePullPolicy: Always`. **A silent pull can + change the behaviour this feature depends on, with no commit anywhere in this repo to show for + it** — and the failure would be the silent software-transcode described above. I have not fixed + that here (it is the Plex wiring's tag/digest decision, not the capability's), but it should not + be forgotten. +- **GID drift is NOT on this list any more, deliberately.** An earlier revision pinned + `INTEL_RENDER_GID = 992` and listed "a node rebuild could move it" as an open risk. The hook + re-`stat`s the device on every start, so drift is self-healing and no GID is pinned anywhere. The + risk is retired, not overlooked. +- **Anything on the live cluster.** I ran no `kubectl` and I opened no shell in any container. The + node facts (device present, `root:render` 0660, `i915` bound, no device plugin, no + `gpu.intel.com/*` advertised), the in-container measurements quoted above, and the Kyverno + posture (`restrict-host-namespaces` Enforce; no hostPath or `supplementalGroups` + restriction; no PSA labels on app namespaces) are **prior recon handed to me, not re-measured by + me**. Per CLAUDE.md §2b, treat them as second-hand until re-queried. +- **That CI exercises the self-tests. It does not.** `git grep -- --selftest ci/` returns nothing — + no gate stage invokes the renderer's self-test. They are the executable spec and house style, and + I ran them by hand, but a future regression in this code path **will not be caught by the gate**. + Wiring `--selftest` into a gate stage is a real gap and is not part of this PR (`ci/` is a + sensitive path and would drag in a human reviewer for a change unrelated to this one). +- **Runtime behaviour of any kind.** The gate proves the L0/L1 offline subset only; + `vars.CLUSTER_AVAILABLE` is unset, so L2/L3 conformance does not run in CI. diff --git a/schemas/README.md b/schemas/README.md index 6847b69..20bddc9 100644 --- a/schemas/README.md +++ b/schemas/README.md @@ -15,7 +15,7 @@ docs/13 exactly. Do not diverge from it here — escalate spec problems, don't f | `servicedefinition.schema.json` | Formal JSON Schema (draft 2020-12) for a ServiceDefinition. Enforces enums, required fields, patterns, and cross-field invariants. | | `renderer-contract.md` | Authoritative field → rendered-resource mapping, with requirement IDs. The contract other workstreams consume. | -**Schema version:** 1.0.0 (in the schema `description`). Treat any change as an interface change: +**Schema version:** 1.8.0 (in the schema `description`, which carries the full per-version changelog — this line had said `1.0.0` since 1.1.0 and is now synced). Treat any change as an interface change: propose on the task board, update dependent renderers/tests in the same change, never silently break a consumer (coordination-protocol §2). @@ -35,6 +35,9 @@ consumer (coordination-protocol §2). - `ingressRoutes[].host` must match `