From f4a155a807ef14326bc70a4ce88b2e437d69bcae Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Wed, 30 Sep 2026 12:56:33 -0500 Subject: [PATCH] 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,