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,