fix(auth): five oauth2-proxy sidecars shared one cookie, died at 5 minutes, and served no metrics #28
@@ -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,
|
||||
|
||||
Reference in new issue
Block a user