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) <[email protected]>
This commit is contained in:
jakman85andClaude Opus 5 committed
1 parent
f4a155a807
commit
dc136a4943
10 files changed
+284
-78
No files matched your search
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
+88
-1
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
+5
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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_<app>` + 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/<tool> --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
|
||||
|
||||
Reference in new issue
Block a user