[UniversalBasicInc]

Fixes #28.

Problem

values.yaml copies auth.oidc.clientSecret / cookieSecret into the Dex and oauth2-proxy values (and pipelines.seaweedfs.* into user-namespace) using YAML anchors. Anchors are resolved when values.yaml itself is parsed, so values supplied with -f or --set update only the source keys, not the copies. When Dex's static client secret is set explicitly but oauth2-proxy's copy isn't updated, the two no longer match and login fails after Dex with HTTP 500 invalid_client. Editing values.yaml directly is not affected.

Change

• One chart-managed Secret, kubeflow-oidc-client, built from auth.oidc.*. oauth2-proxy reads it via config.existingSecret, Dex via envVars + staticClients[].secretEnv, so values set either way reach both. auth.oidc.existingSecret: true lets operators provide the Secret themselves.
• validations.yaml: the render fails if a remaining copy (Dex, oauth2-proxy, user-namespace SeaweedFS keys) differs from its source, or if auth.oidc.cookieSecret is a length oauth2-proxy would reject.
• CI: a render step checks that auth.oidc.* overrides reach Dex and oauth2-proxy, and that mismatched SeaweedFS copies fail the render.
• Docs: guides use secretEnv and URL-safe base64 cookie secrets; a note on rotating the OIDC secrets; the values reference covers auth.oidc.existingSecret (the client id is fixed to kubeflow-oidc-authservice, the id Dex registers) and notes that dex.oidcClientId and dex.existingOidcClientSecret are not read by this chart; an upgrade note in docs/upgrade-notes.md.

Compatibility

Existing values files keep working, including ones that set Dex's staticClients[].secret explicitly.

Testing

• helm lint passes; the new CI step passes on this branch and fails on main.
• On a kind cluster with upstream Dex 0.24.1 and oauth2-proxy 10.7.0: on main, a values file that sets the client secret reproduces the HTTP 500; on this branch, login succeeds.
• The new CI step is gated on APPCO_TOKEN, like the existing render steps.

This change was developed with assistance from Claude (Anthropic); I reviewed and tested every part of it.

🤖 Generated with Claude Code

[mbelur]

@UniversalBasicInc Thanks for the PR! The changes look good. We can approve and merge it once the conflicts are resolved.

[UniversalBasicInc]

Thanks! Rebased onto current main and resolved the conflicts with #41 (kept both guards in validations.yaml and the new oauth2-proxy note in values.yaml). The render checks still pass, including with the new native ext_authz values.

[mbelur]

Findings from AI assisted code-review:

1. [Blocking] auth.oidc.clientId is a new footgun — it reaches oauth2-proxy but not Dex

The PR newly introduces auth.oidc.clientId and documents it as authoritative ("OIDC client ID written to the shared Secret"). But the client id lives as a literal in three unconnected places, and only one is wired to the new knob:

┌──────────────────────────────────────────────────────────────┬───────────────────────────────┐
│ Location │ Driven by auth.oidc.clientId? │
├──────────────────────────────────────────────────────────────┼───────────────────────────────┤
│ secret client-id → oauth2-proxy OAUTH2_PROXY_CLIENT_ID │ ✅ yes │
├──────────────────────────────────────────────────────────────┼───────────────────────────────┤
│ Dex dex.config.staticClients[0].id (hardcoded literal) │ ❌ no │
├──────────────────────────────────────────────────────────────┼───────────────────────────────┤
│ oauth2-proxy config.clientID (literal; env from secret wins) │ ❌ no │
└──────────────────────────────────────────────────────────────┴───────────────────────────────┘

Dex consumes the shared secret only for the client-secret (secretEnv), never for the id. So overriding auth.oidc.clientId makes oauth2-proxy present the new id while Dex still only knows kubeflow-oidc-authservice → invalid_client → login fails. This is the same class of bug the PR sets out to eliminate, re-introduced for clientId, and unlike clientSecret/cookieSecret there is no validation guarding it. Confirmed by render: --set auth.oidc.clientId=my-custom-client yields secret/oauth2-proxy = my-custom-client, Dex still kubeflow-oidc-authservice.

Defaults match, so out-of-the-box installs are fine — it only bites an operator who trusts the docs and overrides it.

Three ways to fix (pick one):

• B — recommended: drop auth.oidc.clientId and emit a fixed client-id: "kubeflow-oidc-authservice" in the secret template. The id isn't sensitive and realistically never needs per-install override; this removes the divergence point entirely. Upgrade-safe: every released chart already uses kubeflow-oidc-authservice everywhere, and the knob is unreleased, so no existing deployment can be relying on a custom id. A leftover auth.oidc.clientId in an operator's values file is silently ignored (no additionalProperties: false).
• idEnv — keep it configurable and actually wire it: Dex supports idEnv (the id counterpart of secretEnv). Add a KUBEFLOW_OIDC_CLIENT_ID env from the secret's client-id key and set idEnv: KUBEFLOW_OIDC_CLIENT_ID on the static client. Then all three consumers track the secret and clientId becomes a genuine single source of truth.
• A — lowest effort, matches the PR's own style: add a $credErrs check that fails the render if auth.oidc.clientId diverges from the Dex static-client id and oauth2-proxy config.clientID (select the Dex client by .secretEnv being set, not by .id == "kubeflow-oidc-authservice" — that same brittleness exists in the current clientSecret check and is worth fixing in the same pass).

2. [Low] Dead values left behind

dex.oidcClientId is now read by nothing (the dex-idp subchart doesn't consume it; it was already dead pre-PR). dex.oidcClientSecret survives only so the new validation can detect misuse — that's fine — but consider removing dex.oidcClientId or commenting it as unused.

3. [Low] values.schema.json not updated for the new keys

auth.oidc.* (including the new clientId/existingSecret) isn't in the schema — pre-existing gap, and auth has no additionalProperties: false, so nothing fails. Adding existingSecret as type: boolean would catch the classic existingSecret: "false" string footgun (string → truthy → secret silently skipped → pods reference a missing secret). Optional.

4. [Low / release-note] SeaweedFS handled asymmetrically

OIDC gets a real fix (shared secret); SeaweedFS only gets a fail-fast guard. This changes behavior: --set pipelines.seaweedfs.accessKey=x alone now hard-fails the render where it previously rendered (silently broken). Fail-fast is the better behavior, but it's a visible change worth a release note. Scoping it this way is defensible (OIDC was the filed bug).

5. [Info] Minor convention drift (not a problem)

The new secrets/oidc-client-Secret.yaml uses namespace: {{ .Release.Namespace }} + app.kubernetes.io/* labels, while its sibling cloudflare-api-token-Secret.yaml hardcodes namespace: kubeflow with no labels. The new template is actually the better pattern — just inconsistent with its neighbor.

---

Testing note

Full-chart helm template fails in this workspace on a pre-existing migration guard (validations.yaml:126, removed ingress-gateway keys) that trips on the locally-vendored subchart defaults — unrelated to this PR (the keys are untouched, and it fails identically on main). I validated the OIDC wiring on an isolated copy with only that guard neutralized. The PR's own new CI render step looks correct and genuinely exercises the fix.

---

Verdict

Solid, well-tested fix for #28 with good fail-fast guards and a real security tidy-up. Finding #1 should block merge — auth.oidc.clientId reproduces the exact footgun the PR eliminates, with no guard, because Dex consumes the shared secret for the credential alone and not the id. Everything else is low-severity or optional. Recommend fix option B (or idEnv if clientId must stay configurable), then merge.

[UniversalBasicInc]

Thanks for the thorough review! Pushed an update:

1. clientId: went with option B. auth.oidc.clientId is gone and the Secret writes a fixed client-id: kubeflow-oidc-authservice, so the id can't diverge between Dex and oauth2-proxy any more. The values reference and the existingSecret comment now say a self-supplied Secret must use that id. Rendered with --set auth.oidc.clientId=my-custom-client: the value is ignored, and the Secret, Dex's static client and oauth2-proxy all use kubeflow-oidc-authservice.
2. Dead values: dex.oidcClientId is now listed alongside oidcClientSecret / existingOidcClientSecret as not read by the chart. I left the keys in place so existing values files don't change meaning.
3. Schema: left out of this PR since auth.* isn't in the schema yet; happy to send that as a separate PR (including existingSecret as a boolean) if you'd like.
4. Release note: added a section to docs/upgrade-notes.md (0.4.2) covering the move to the kubeflow-oidc-client Secret and the new render-time failures, including the SeaweedFS one.
5. Kept the new Secret's labels/namespace as they are.

The repro checks still pass on the updated branch, including the CI render step.

[mbelur]

LGTM