From 298337849671d42c260b949f31725fed60f5a15c Mon Sep 17 00:00:00 2001 From: Keith Jackson Date: Thu, 1 Oct 2026 22:25:23 -0500 Subject: [PATCH] chore(decisions): record the CI reading, and why the metrics policy is NOT here Follow-up to the branch's one substantive commit; no code change. CI on 99560c2d, read from the commit's /statuses rather than from a tick: gate / gate (pull_request) state=failure desc='Failing after 1m44s' gate / conformance (pull_request) state=pending desc='Blocked by required conditions' The second line is PENDING, not the `success` + "Has been skipped" trap -- the conformance job is gated on the gate job, which failed, so it never ran. MY LOCAL `approval-policy: green` WAS MEANINGLESS AND I NEARLY REPORTED IT AS EVIDENCE. ci/approval-policy.sh:47 reads GITHUB_EVENT_NAME, unset outside CI, so it short-circuits to "N/A": a clean local gate STRUCTURALLY cannot catch a sensitive-path problem. Verified by re-running with GITHUB_EVENT_NAME=pull_request PR_NUMBER=34, where the stage evaluates and fails CLOSED on the missing token while every other stage stays green -- which bounds the CI failure to that one stage. I could NOT read CI's job log to confirm it from CI's own output: this Gitea answers 404 on /actions/runs and on /actions/jobs/{task}/logs, so only /actions/tasks is available. Attribution therefore rests on the local reproduction plus the sensitive-path rule, not on CI's log -- weaker evidence than PR #28 had, named rather than papered over. Branch protection and the runner re-measured against the live API (CLAUDE.md 2b), not inherited from the CLAUDE.md table: protected=True, user_can_push=False, enable_status_check=True, contexts=['gate / gate (pull_request)'], required_approvals=1; runner id=1 incluster-dind online. So a human is required on every PR here regardless of path. I did NOT provoke a rejected push to main, so user_can_push is the API's word only. DECLINED, with reasons: bundling Ines Dubois's metrics-ingress NetworkPolicy. The coordinator asked for my judgement and the answer is to queue it as the next PR, not to drop it. Principally because it is a MODEL question, not a policy to bolt on: schema 1.6.0's observability.metricsEndpoint already renders the monitoring-namespace ingress allow, and what the model cannot express is a SECOND metrics endpoint belonging to a sidecar the RENDERER injected. Keying a bespoke policy off the pod label auth-mode=forward-auth hardcodes "forward-auth implies 44180" instead of deriving it -- the "list, don't derive" anti-pattern oauth2-proxy-cookie-names-distinct assertion (5) forbids, one directory from the check that forbids it. Also: it is a trust-boundary ingress allow and a different requirement set (R-NET-01/02, R-OBS-01 vs R-IDP-02), so it should not be reviewed in the shadow of an --api-route headline; the conflict argument does not apply to my diff, which regenerates TWO goldens not five, and a CiliumNetworkPolicy is a different document in files git merges independently; every fact in the finding is hers and I have measured none of it; and the PodMonitor it serves is in HER PR #29, so shipping the policy alone would open an ingress allow for a scraper that selects nothing. Also records that the sidecars' metrics remain UNSCRAPABLE and that this branch does not change that. Co-Authored-By: Claude Opus 5 (1M context) --- ...26-10-01-oauth2-proxy-api-route-per-app.md | 148 +++++++++++++++++- 1 file changed, 145 insertions(+), 3 deletions(-) diff --git a/decisions/2026-10-01-oauth2-proxy-api-route-per-app.md b/decisions/2026-10-01-oauth2-proxy-api-route-per-app.md index 41d7d71..886fe5f 100644 --- a/decisions/2026-10-01-oauth2-proxy-api-route-per-app.md +++ b/decisions/2026-10-01-oauth2-proxy-api-route-per-app.md @@ -83,6 +83,57 @@ Note the consequence of his stage change for anyone reading this later: once PR in CI the way rc 3 is** — a selftest failure will block. Which is the right outcome, and it is why a selftest that prints nothing was worth fixing before that lands rather than after. +### Declined, with reasons: the metrics-ingress NetworkPolicy + +The coordinator asked for my judgement on bundling Ines Dubois's finding — that #28's metrics +listener genuinely serves (she measured `http=200`, 10,208 bytes, +`oauth2_proxy_requests_total{code="302"} 2` via port-forward) but the scrape will be **dropped**, +because namespace `jackett`'s default-deny has **no `ingress` key at all** and +`jackett-allow-gateway-ingress` admits `fromEntities: [ingress]` only. The fix belongs in each +app's own rendered policy, i.e. in my file and my goldens. **I declined to bundle it**, and asked +for it as the immediate next PR. Five reasons, in the order that decided it: + +1. **The real reason is that it is a MODEL question, not a policy to bolt on.** Schema 1.6.0 + already owns this: `observability.metricsEndpoint` renders the Service port **and** the + monitoring-namespace ingress allow. What the model cannot currently express is a **second** + metrics endpoint on the same pod belonging to a *sidecar the renderer injected*, rather than to + the workload the operator declared. Keying a bespoke policy off the pod label + `homelab.keithjackson.dev/auth-mode: forward-auth` works today, but it hardcodes "forward-auth + implies a metrics port on 44180" into a policy instead of **deriving** it from the sidecar + declaration that is already the single source of that port — the exact "list, don't derive" + anti-pattern `oauth2-proxy-cookie-names-distinct` assertion (5) exists to forbid, and which I + would be introducing one directory away from the check that forbids it. That deserves its own + review, and it is squarely the kind of decision I was brought in to make deliberately. +2. **Different subject, and it is a trust-boundary change.** Mine is the auth *response code* on + API paths (R-IDP-02). That is an ingress allow (R-NET-01/02, R-OBS-01) admitting a new + namespace to a new port. Bundling a security-relevant ingress rule into a PR whose headline is + `--api-route` is how such a rule gets waved through on the strength of the other half — and this + PR already needs a human for a *different* sensitive-path reason, so the second subject would be + reviewed in the shadow of the first. +3. **The conflict argument does not actually apply to my diff.** It assumes I am regenerating five + goldens; **I am regenerating two** (jackett, radarr), because the flag renders only where + declared. A metrics-ingress policy renders a *different document* — a CiliumNetworkPolicy — in + all five. Different hunks, in files git merges independently; and goldens regenerate + deterministically from the renderer, so the follow-up rebases onto `main` cleanly. The N-1 + rebase problem `decisions/README.md` records was two branches appending to **the same hunk**, + which this is not. I checked my own diff before answering rather than accepting the premise. +4. **Every fact in the finding is hers and I have measured none of it** — not the 200/10,208 bytes, + not the `wget: download timed out` on `:44180` *and* `:4180`, not the Flux sibling ordering that + makes `oauth2-proxy` and `apps-wave-0` unordered. Shipping a security-adjacent rule on wholly + inherited evidence, as a footnote in someone else's PR, is precisely what CLAUDE.md §2b exists + to stop. In its own PR I would measure it first. +5. **Sequencing: the PodMonitor is in HER PR #29, not mine.** If I ship the policy and her + PodMonitor is reworked or delayed, I have opened an ingress allow for a scraper that selects + nothing — a hole with no consumer. The policy is only useful paired with the PodMonitor, which + argues for it landing adjacent to that, not here. It also drags in a + `KNOWN_EXTERNAL_MONITORS` entry in `conformance/tests/l0-static/servicemonitor-ports-resolve/`, + a second conformance interaction on top of the new check this PR already adds. + +Diff size is the tiebreaker rather than the argument: **1,245 insertions across 13 files**, +including a ~400-line new conformance check. I would rather hand a reviewer one subject they can +actually falsify. **Queued, not dropped** — and if the next person takes it, the model note in (1) +is the part worth arguing about, not the YAML. + ### Which apps got which routes, and the measurement behind each | App | `auth.apiRoutes` | Evidence | @@ -224,6 +275,33 @@ rendered sidecar against its **own ServiceDefinition**, which is the only readin r-nosuchreq-99 --include-scope whole-repo --print-selection` lists `tests/l0-static/oauth2-proxy-api-routes-per-app` among **21** tests (it was 20), i.e. it runs even when the touched-requirement filter matches nothing. +- **THE SCHEMA GUARD IS PROVEN WITH THE PINNED VALIDATOR `ci/l0.sh` ACTUALLY DRIVES**, not by + reading the pattern. `./schemas/validate.sh` (`check-jsonschema, version 0.29.4`) against seven + throwaway ServiceDefinitions written outside the repo: + + good ["^/api/", "^/signalr/"] rc 0 + bad-root ["^/"] rc 1 apiRoutes ... does not match + bad-dotstar ["^/.*"] rc 1 apiRoutes ... does not match + bad-unanchored ["/api/"] rc 1 apiRoutes ... does not match + bad-alt ["^/api/|^/"] rc 1 apiRoutes ... does not match + bad-everything [".*"] rc 1 apiRoutes ... does not match + bad-mode ["^/api/"] on mode: oidc rc 1 const 'forward-auth' + + So a too-broad regex is rejected **where a human types it**, on every gate run, by the real tool + — and the shape invariant fires too. +- **THE RENDERER'S REFUSAL REACHES `render-golden` AS A READABLE FAILURE, NOT A TRACEBACK.** With + `^/signalr/` swapped for `^/` in a *copy* of the catalog (outside the repo), both invocation + shapes exit **rc 1 with 0 tracebacks** — the single-SD form and `--all -o + --wave-layout --no-image-automation`, which is the one `ci/render-golden.sh` drives: + + render.py: radarr: auth.apiRoutes entry '^/' is not an anchored path regex. Each entry + must start with '^/' and continue with literal path characters (e.g. '^/api/', + '^/signalr/', '^/api/v3/.*'). ... A too-broad --api-route answers a PAGE LOAD with 401, so + a logged-out operator sees a bare error page and is never offered a sign-in: the service + looks permanently broken rather than merely unauthenticated. + + So the breadth guard is gated by `render-golden` even on a run where the schema stage skipped, + and the operator is told *why* rather than handed a stack trace. - **The schema's new shape invariant.** A non-empty `auth.apiRoutes` requires `auth.mode: forward-auth`, because the flag renders onto the per-app sidecar and no sidecar exists for `oidc`/`native`/`none` — declaring it elsewhere would be a **silent no-op**, which is the @@ -245,9 +323,67 @@ rendered sidecar against its **own ServiceDefinition**, which is the only readin watched. Nobody has added a Jackett indexer or recovered a Radarr tab through this configuration. The human test below is the first real evidence this change will ever have. - **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.** + `l0`, `render-golden`, `l1`. **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.** +- **MY LOCAL `approval-policy: green` WAS MEANINGLESS, AND I ALMOST REPORTED IT AS EVIDENCE.** + `ci/approval-policy.sh:47` reads `GITHUB_EVENT_NAME`, which is unset outside CI, so the stage + short-circuits at line 50: `ok "not a pull_request event (event='none') — approval policy N/A"`. + **A clean local gate therefore CANNOT catch a sensitive-path problem — structurally, not by + accident.** Flagged to me by the coordinator from Ines Dubois's round, and I verified it by + re-running the gate with `GITHUB_EVENT_NAME=pull_request PR_NUMBER=34`: the stage then + **evaluates** and fails closed on the missing token — + + [fail] no SETTINGS_API_TOKEN — cannot read this PR's changed files or its reviews. + [fail] stage approval-policy: FAIL + [fail] approval-policy: a sensitive change lacks a non-author approval -> blocks merge + [fail] MERGE GATE: BLOCKED (rc 1, not 3) + + — while **every other stage stays green** (`spec-immutability`, `decisions-log`, `lint`, `l0`, + `render-golden`, `l1`). That is the useful result: it bounds the CI failure to this one stage + rather than leaving "something is red" as the whole report. Fail-closed is correct here and + deliberate: unlike `gitea-settings`, where a missing credential is a lock-out, a skip of this + stage would be a bypass (CLAUDE.md §1). +- **CI, read from each commit's `/statuses` rather than from a tick.** Both contexts report, and + **only one of them runs**. Identical on `99560c2d`, `df1b25b6` and `f1d16f44`: + + gate / gate (pull_request) state=failure desc='Failing after ~1m43-1m45s' + gate / conformance (pull_request) state=success desc='Has been skipped' + + **That second line is §2b's trap in the wild**: a *skipped* Gitea job reports + `state=success`. The conformance job did not execute, so its green tick certifies nothing — + read the description, not the colour. The duration is given as a range on purpose: it differs + by a second or two per run, and pinning one number inside the file whose commit triggers the + next run is a self-reference, not a measurement. The load-bearing fields are the **state** and + the **description**, and those are stable across all three. + + Worth recording that the **same context changed its answer over time**: on the first push it + briefly read `state=pending desc='Blocked by required conditions'` — still waiting on the gate + job it depends on — and resolved to `success` only once that job settled. So sampling a status + once can show either a pending **or** a green tick for a job that never executed. + **I could NOT read the CI job log to confirm the failing stage from CI's + own output**: this Gitea's `/api/v1/repos/{owner}/{repo}/actions/runs` endpoint answers + **404** (only `/actions/tasks` exists, which lists tasks without logs), and + `/actions/jobs/{task_id}/logs` answers `404 Not found` for the task id. So my attribution of the + CI failure to `approval-policy` rests on the local event-set reproduction above plus the + sensitive-path rule, **not** on CI's own log. That is weaker evidence than PR #28 had and I am + naming it rather than borrowing its wording. +- **Branch protection and the runner, RE-MEASURED this session against the live API** (§2b), + not inherited from CLAUDE.md §2's table: + + GET /repos/Home-Network/home-network-k3s-impl/branches/main + protected True + user_can_push False + enable_status_check True + status_check_contexts ['gate / gate (pull_request)'] + required_approvals 1 + GET /admin/actions/runners + id=1 name=incluster-dind status=online + + So `required_approvals=1` applies to **every** PR here regardless of path — this needs a human + even without the sensitive-path rule. I did **not** provoke a rejected push to `main` this + session; `user_can_push: False` is the API's word, which is exactly the weaker form of evidence + CLAUDE.md §2 warns about, and I did not strengthen it. - **`^/api/` ON RADARR IS NOT MY MEASUREMENT.** It is PR #30's, taken on the **pre-fix** pod. The current pod has logged **zero** `/api/` traffic in 23 h, so I could not reproduce it. I judge the breadth risk nil (`/api/v3/` is pure JSON; the UI is at `/`), but if someone wants a strictly @@ -301,6 +437,12 @@ rendered sidecar against its **own ServiceDefinition**, which is the only readin (R-TEST-06, read-only). `schemas/renderer-contract.md` is updated in **this** repo only; the canonical spec `docs/13-service-catalog.md` is **not** updated, because it is immutable from here. Someone with spec write access should mirror `auth.apiRoutes` into docs/13 and docs/15. +- **THE OAUTH2-PROXY SIDECARS' METRICS ARE STILL UNSCRAPABLE, AND THIS PR DOES NOT FIX IT.** See + the declined-bundle section above. #28 shipped `--metrics-address` as a prerequisite and said + plainly that it was not scraped; Ines has now measured that the listener **serves** and that the + scrape is **dropped by each app namespace's default-deny**. Nothing in my change touches that. + Anyone reading this entry as "the fleet gates are observable now" is wrong, and the next PR is + where that changes. - **Alternation is refused, and that is a judgement I may have to revisit.** No app in this catalog needs it, and the flag is repeatable — but if some future app genuinely needs a regex the ratchet rejects, the fix is to widen the pattern **with a reason** on all three surfaces at once, not to