Fix(charts+controller): SPIFFE auth follow-up — jwtAudience cleanup, credential secret skip, tag fix#476
Conversation
|
Note on removing This was mistakenly introduced in #473. The This PR removes the |
…jwtAudience spiffe.enabled — master switch indicating SPIRE is present cluster-wide. spiffe.operatorAuth.enabled — narrower flag for operator-to-Keycloak auth. Both must be true to enable the spiffe-helper sidecar. jwtAudience was always going to equal keycloak.publicUrl/realms/<realm>. Remove the user-facing field; the template now derives it and fails at helm install time if keycloak.publicUrl is not set. Signed-off-by: Alan Cha <alan.cha@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
…:latest The spiffeHelper.image.* values block was introduced to pin the image tag but the standalone spiffe-helper image is not published by the kagenti-extensions automated pipeline. Only :latest exists from a historical publish. Remove the values block and hardcode :latest. Issue #477 tracks replacing the sidecar with a direct go-spiffe SDK call. Signed-off-by: Alan Cha <alan.cha@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
…derated-jwt mode In federated-jwt mode, AuthBridge reads JWT-SVIDs from the SPIFFE workload API socket directly — no credential Secret is created or needed. Two fixes: 1. Skip ensureClientCredentialsSecret() when CLIENT_AUTH_TYPE=federated-jwt. The Secret would contain a valid Keycloak client secret that Keycloak correctly rejects (it enforces federated-jwt auth type), but creating it is a hygiene issue and a minor defense-in-depth gap. 2. Skip patchTemplate() in the same case. patchTemplate injects the AnnotationKeycloakClientSecretName onto the Deployment pod template, which the webhook uses to mount the credential Secret. Without the Secret, pods get stuck in Init. Signed-off-by: Alan Cha <alan.cha@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
…jwt mode The webhook pre-populates AnnotationKeycloakClientSecretName so pods can mount the credential Secret before the controller reconciles. In federated-jwt mode no Secret is created, so injecting this annotation leaves pods stuck in Init waiting for a Secret that never appears. Fix: read CLIENT_AUTH_TYPE from authbridge-config before pre-populating the annotation and skip it when federated-jwt is configured. Signed-off-by: Alan Cha <alan.cha@ibm.com> Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
0532b06 to
0d25ce8
Compare
mrsabath
left a comment
There was a problem hiding this comment.
Review
Clean, well-scoped follow-up to #473. Verified every claim against the PR head.
This is a textbook dual-mode PR, so I focused on source/env/fallback parity across the three code paths that must agree in federated-jwt mode:
| Path | File | federated-jwt behavior |
Source |
|---|---|---|---|
| Controller | clientregistration_controller.go:378 |
skips ensureClientCredentialsSecret + patchTemplate |
CLIENT_AUTH_TYPE in authbridge-config |
| Webhook | authbridge_webhook.go:106 |
skips annotation pre-population | same CM via ReadNamespaceConfig |
| Mutator | pod_mutator.go:857 |
emits SPIFFE identity, no secret mount | same CM / constant |
All three read CLIENT_AUTH_TYPE from the same ConfigMap and compare against the same value — the controller uses the string literal "federated-jwt", webhook/mutator use injector.ClientAuthTypeFederatedJWT (defined as that literal in constants.go:37). No parity gap.
Other checks:
- Default/fallback parity ✓ — empty/absent
CLIENT_AUTH_TYPE→ controller defaults toclient-secret; webhook's"" != federated-jwt→ pre-populates the annotation. Consistent safe default. - Helm
failguard ✓ — the hardfailon emptykeycloak.publicUrlis inside thespiffe.operatorAuth.enabledblock, so it never breaks default (non-SPIFFE) installs.keycloak.{publicUrl,realm}always exist invalues.yaml, so no template nil-deref. - No dangling refs ✓ — removed
spiffe.operatorAuth.jwtAudienceandspiffeHelper.imagehave zero remaining references in the chart. - CI ✓ — all 16 checks pass, including E2E (32m) and DCO.
The :latest hardcode (change #3) is a documented trade-off tracked in #477, not a defect.
Verdict: APPROVE — no blocking issues. The dual-mode parity, usually where these PRs hide bugs, is correct across all three paths.
Areas reviewed: Helm, Go controller, Go webhook, Go mutator (cross-referenced)
Commits: 4, all signed-off
CI status: passing (16/16)
…r inbound JWT validation In federated-jwt mode the inbound jwt-validation plugin in AuthBridge reads /shared/client-id.txt to determine the expected audience for incoming requests. PR rossoctl#476 skipped the entire ensureClientCredentialsSecret call in this mode, which meant client-id.txt was never written and every inbound request to the agent was rejected with HTTP 503 indefinitely. Controller: - Pass empty clientSecret (not clientID) to ensureClientCredentialsSecret in federated-jwt mode so the Secret is still created with client-id.txt only. - Remove the authType guard so patchTemplate still runs in federated-jwt mode, keeping the pod template annotation that causes the webhook to mount the Secret. - In ensureClientCredentialsSecret, omit client-secret.txt when clientSecret is empty rather than always writing it. Webhook: - Remove the federated-jwt guard that skipped annotation pre-population. The Secret still exists in federated-jwt mode (with client-id.txt only), so the volume mount annotation is needed for the first pod to come up correctly. Fixes rossoctl#482 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Alan Cha <Alan.cha1@ibm.com>
Summary
Follow-up fixes to #473 based on E2E testing of the full SPIFFE auth stack.
Changes
1. Remove
jwtAudienceHelm value — always derive fromkeycloak.publicUrlspiffe.operatorAuth.jwtAudiencewas the audience embedded in the JWT-SVID that SPIRE mints for the operator. This value must exactly equalkeycloak.publicUrl/realms/<realm>— the issuer URL Keycloak'sFederatedJWTClientValidatorchecks against. It cannot meaningfully be set to anything else.Removed the user-facing field. The template now derives the value directly from
keycloak.publicUrlandkeycloak.realm, and fails athelm installtime with a clear message ifkeycloak.publicUrlis not set. Eliminates a footgun where using the internal k8s service URL instead of the external URL causes a silent auth failure.2. Skip credential Secret creation in
federated-jwtmodeIn
federated-jwtmode, AuthBridge reads JWT-SVIDs from the SPIFFE workload API socket directly — it does not read thekagenti-keycloak-client-credentials-*Secret. The operator was still creating it unconditionally.Keycloak correctly rejects
client_secretgrants forfederated-jwtclients (HTTP 401), so the Secret is not exploitable. But it is unnecessary and adds noise in agent namespaces.Fix: skip
ensureClientCredentialsSecret()whenCLIENT_AUTH_TYPE == "federated-jwt".3. Remove
spiffeHelper.imagevalues block — revert to:latestThe
spiffeHelper.image.*values block was mistakenly introduced in #473. Theghcr.io/kagenti/kagenti-extensions/spiffe-helperimage is not published by the kagenti-extensions build pipeline (which only buildsauthbridge,authbridge-envoy,authbridge-lite,proxy-init). Pinning to a specific tag has no meaning — only:latestexists from a historical publish.This commit removes the values block and hardcodes
:latestin the template. Issue #477 tracks replacing the sidecar approach entirely with a direct go-spiffe SDK call, which would eliminate the external image dependency.Testing
Validated on a fresh Kind cluster with both operator and agent SPIFFE auth enabled:
STATUS: deployedclient_secretgrant forfederated-jwtclients: HTTP 401federated-jwtagentsRelated
Checklist
Assisted-By: Claude Code