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) <[email protected]>
189 lines
14 KiB
Markdown
189 lines
14 KiB
Markdown
---
|
|
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 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
|
|
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
|
|
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.
|
|
- **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.
|