feat: use go-spiffe SDK directly instead of spiffe-helper sidecar

[Alan-Cha]

Summary

Remove spiffe-helper sidecar from operator pod and use go-spiffe SDK directly to fetch JWT-SVIDs from the SPIRE Workload API.

Changes

• operator/cmd/main.go: Remove --jwt-svid-path flag, pass SpiffeSocket to controller
• operator/internal/controller/clientregistration_controller.go:
  • Replace JWTSVIDPath field with SpiffeSocket
  • Add fetchJWTSVID() helper using workloadapi.Client.FetchJWTSVID()
• charts/operator/templates/manager/manager.yaml: Remove spiffe-helper sidecar container, jwt-svid volume, and --jwt-svid-path CLI arg
• charts/operator/templates/manager/configmap-spiffe-helper.yaml: Delete (no longer needed)

Motivation

This matches the architecture already used by authbridge-proxy and eliminates the need for maintaining a separate spiffe-helper sidecar. The go-spiffe SDK provides direct Workload API access, making the file-based JWT-SVID exchange unnecessary.

Testing

• ✅ Operator builds successfully with go-spiffe SDK
• ✅ Operator pod runs with single container (no spiffe-helper sidecar)
• ✅ Client registration controller compiles with new fetchJWTSVID() method
• ✅ E2E test verifies weather agents deploy and register correctly

Closes

Replaces #478 with a clean implementation.

Assisted-By: Claude Code

[Alan-Cha]

Update: Fixed JWT-SVID Audience Issue

Found and fixed a critical bug during E2E testing:

Problem: JWT-SVID was being fetched with audience=realm_name ("rossoctl"), but Keycloak's JWT-SPIFFE authentication expects the audience to be the full token endpoint URL.

Fix: Changed fetchJWTSVID() call to use the token endpoint URL as audience:

tokenEndpoint := strings.TrimSuffix(ab.KeycloakURL, "/") + "/realms/" + ab.KeycloakRealm + "/protocol/openid-connect/token"
jwtSVID, err := r.fetchJWTSVID(ctx, tokenEndpoint)

Verification in progress:

• ✅ fetchJWTSVID() is being called
• ✅ JWT-SVID is successfully fetched from SPIRE
• ⏳ Testing Keycloak accepts JWT-SVID with correct audience
• ⏳ Verifying client credentials Secret creation
• ⏳ End-to-end token exchange testing

Committed in 952a53d

[Alan-Cha]

Critical Fix: JWT-SVID Audience Must Match Keycloak Issuer URL

Found root cause of "Invalid token audience" error:

Problem: JWT-SVID audience was using in-cluster service URL, but Keycloak validates against its public issuer URL.

From authentication.md:

When presenting this to Keycloak as a client assertion (RFC 7523), the JWT's aud claim must equal Keycloak's realm issuer URL.

This URL is always keycloak.publicUrl/realms/<realm> — derived automatically from your Helm values. It must be the external/public URL, not the in-cluster service address.

Solution: Query Keycloak's OIDC discovery endpoint to get authoritative issuer URL:

func (r *ClientRegistrationReconciler) getKeycloakIssuer(ctx context.Context, keycloakURL, realm string) (string, error) {
    discoveryURL := keycloakURL + "/realms/" + realm + "/.well-known/openid-configuration"
    // Query and extract issuer field
    return config.Issuer, nil  // e.g., "http://keycloak.localtest.me:8080/realms/rossoctl"
}

Testing with correct issuer URL now...

Committed in afce861

[clawgenti]

Clean refactor that eliminates the spiffe-helper sidecar in favor of the go-spiffe SDK — good alignment with the authbridge-proxy pattern. A few items below.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

nit: This {{- if and .Values.spiffe ... }}{{- end }} block is empty after removing the jwt-svid volume mount from inside it — it can be deleted.

[clawgenti]

suggestion: getKeycloakIssuer is called on every reconcile when UseSpiffeAuth=true, creating a new http.Client and issuing an OIDC discovery request each time. The issuer URL is stable — consider caching it on the reconciler (e.g. a keycloakIssuer string field populated once on first successful fetch) to avoid per-reconcile HTTP overhead.

[clawgenti]

suggestion: fetchJWTSVID opens a new gRPC connection to the SPIRE Workload API on every call. Consider reusing a long-lived workloadapi.Client (stored on the reconciler and lazily initialized) to reduce connection churn on frequent reconciles, similar to how authbridge-proxy manages its SPIFFE client.

[cwiklik]

Solid, well-motivated change: dropping the spiffe-helper sidecar for the go-spiffe SDK (matches authbridge-proxy), and deriving the JWT-SVID audience from Keycloak's OIDC-discovery issuer is the correct, robust approach (Keycloak validates aud by string-equality against its own issuer). The token is never logged, all failure paths emit events + requeue, and Close() errors are checked.

One must-fix — a Helm gating regression (inline). The Workload API socket volume is declared for verifiedFetch.enabled OR operatorAuth.enabled (manager.yaml ~213), but the volumeMount into the manager container is gated on verifiedFetch.enabled only (~183), and the operatorAuth.enabled mount block (~188) is now empty. So enabling spiffe.operatorAuth.enabled without verifiedFetch.enabled creates the socket volume but never mounts it → the SDK's fetchJWTSVID dials a missing socket and client registration fails permanently. Pre-PR this worked because the spiffe-helper sidecar (gated on operatorAuth.enabled) carried the mount. CI/E2E is green because it enables both toggles, masking the gap. Fix: mount spiffe-workload-api under operatorAuth.enabled too (or use the same or condition on the mount, guarding a double-mount when both are on).

Nits: spiffe.operatorAuth.jwtSVIDPath (values.yaml:209) is now orphaned and its comment still says "written by spiffe-helper sidecar" — remove it; and two empty {{- if …operatorAuth.enabled }}{{- end }} template blocks remain in manager.yaml.

Suggestion (inline, non-blocking): getKeycloakIssuer does an OIDC-discovery HTTP round-trip and fetchJWTSVID opens a fresh Workload API client on every reconcile; the issuer is stable, so caching (and optionally reusing the client) would cut per-registration overhead.

Author: Alan-Cha (MEMBER — maintainer) · Areas: Go, Helm/K8s · .claude/.vscode: none · Commits: 7, signed-off · CI: passing.

Assisted-By: Claude Code

[cwiklik]

suggestion (non-blocking) — getKeycloakIssuer runs an OIDC-discovery HTTP round-trip on every reconcileOne, and fetchJWTSVID opens+closes a fresh workloadapi.Client per call. Both are correct, but the issuer is stable — caching it (and optionally reusing the Workload API client) would remove a per-registration HTTP hop and connection churn.

[clawgenti]

Clean removal of the spiffe-helper sidecar in favour of direct go-spiffe SDK integration — the approach is sound and matches the authbridge-proxy pattern. Two related issues in fetchJWTSVID are worth addressing before merge.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

Mutex held during slow gRPC dial. workloadapi.New() makes a gRPC connection attempt which can block for the full context deadline. All concurrent reconciles will queue on this mutex during that window. Consider using sync.Once or releasing the lock, dialling outside the critical section, then re-acquiring to store — a standard double-checked lock:

r.workloadAPIClientMu.Lock()
if r.workloadAPIClient == nil {
    r.workloadAPIClientMu.Unlock()
    client, err := workloadapi.New(ctx, workloadapi.WithAddr(r.SpiffeSocket))
    if err != nil {
        return "", fmt.Errorf("failed to create SPIFFE Workload API client: %w", err)
    }
    r.workloadAPIClientMu.Lock()
    if r.workloadAPIClient == nil {
        r.workloadAPIClient = client
    } else {
        _ = client.Close()
    }
}
client := r.workloadAPIClient
r.workloadAPIClientMu.Unlock()

[clawgenti]

Stale client not retried on disconnect. If FetchJWTSVID returns an error due to a broken gRPC connection (SPIRE agent restarted, socket recycled), the cached workloadAPIClient is never replaced — every subsequent reconcile will fail with the same dead client until the operator pod restarts. Consider clearing r.workloadAPIClient = nil (under the mutex) when a fetch error is detected, so the next reconcile re-dials:

if err != nil {
    r.workloadAPIClientMu.Lock()
    if r.workloadAPIClient == client {
        _ = r.workloadAPIClient.Close()
        r.workloadAPIClient = nil
    }
    r.workloadAPIClientMu.Unlock()
    return "", fmt.Errorf("failed to fetch JWT-SVID: %w", err)
}

[clawgenti]

The latest commit correctly addresses both items from the previous review: double-checked locking prevents holding the mutex during the slow gRPC dial, and the stale-client clear-on-error means a dead connection is evicted rather than reused. The Helm spiffe-workload-api volumeMount is now gated on verifiedFetch.enabled OR operatorAuth.enabled (manager.yaml:183), which closes the socket-created-but-not-mounted regression cwiklik flagged. One minor nit below.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

nit: getKeycloakIssuer allocates a fresh http.Client on every invocation. In practice this runs at most once per (keycloakURL, realm) pair thanks to the sync.Map cache, so it is not a hot path — but a struct-level httpClient *http.Client initialized in SetupWithManager (or a package-level var) would make the intent clearer and shave a tiny allocation. Non-blocking.

[mrsabath]

Clean, well-motivated refactor: dropping the spiffe-helper sidecar for the go-spiffe SDK matches the authbridge-proxy pattern, and deriving the JWT-SVID audience from Keycloak's OIDC-discovery issuer is the correct approach (Keycloak validates aud by string-equality against its own advertised issuer). The token is never logged, all failure paths emit events and requeue, and Close() errors are checked.

The Helm gating regression and orphaned jwtSVIDPath value flagged on the earlier commit are both already fixed on HEAD — the socket volumeMount and volume now share the same or condition, and the value is removed. Thanks for that.

One must-fix remains around the cached Workload API client (inline). clawgenti already surfaced the same client-lifecycle points; flagging them here too since the stale-client case is a permanent failure mode worth blocking on.

Areas reviewed: Go, Helm/K8s, values.yaml, tests. Commits: 8, all signed-off. CI: all green.

[mrsabath]

must-fix — Stale client never recovered on fetch error. If FetchJWTSVID fails because the gRPC connection is broken (SPIRE agent restarted, socket recycled), the cached workloadAPIClient is never replaced, so every subsequent reconcile fails with the same dead client until the operator pod restarts — a permanent failure mode. Clear the cached client (under the mutex, guarding == client) before returning:

if err != nil {
    r.workloadAPIClientMu.Lock()
    if r.workloadAPIClient == client {
        _ = r.workloadAPIClient.Close()
        r.workloadAPIClient = nil
    }
    r.workloadAPIClientMu.Unlock()
    return "", fmt.Errorf("failed to fetch JWT-SVID: %w", err)
}

[mrsabath]

suggestion — workloadapi.New() performs a gRPC dial while workloadAPIClientMu is held, so all concurrent reconciles serialize behind the full dial window (up to the context deadline on a slow/unavailable agent). Consider a double-checked lock: dial outside the critical section, then re-acquire to store, Close()-ing the loser on a race.

[mrsabath]

suggestion — the client is created with the reconcile-scoped ctx. When that reconcile's context is cancelled, a long-lived client tied to it can be torn down out from under later reconciles. Consider dialing with a manager-scoped / background context so the cached client outlives any single reconcile.

[mrsabath]

suggestion (related to the arg block above) — the volume/mount are now correctly gated on verifiedFetch.enabled OR operatorAuth.enabled, but the --verified-fetch-spiffe-socket arg (line ~49) is still rendered only when verifiedFetch.enabled. The controller wires SpiffeSocket from that flag, so with spiffe.operatorAuth.enabled alone the flag is absent and it falls back to the Go default (unix:///spiffe-workload-api/spire-agent.sock). It works today only because that default matches verifiedFetch.spiffeEndpointSocket; a user who customizes that socket path would silently diverge. Consider rendering the socket arg under the same or condition.

[Alan-Cha]

Addressed review feedback

Commit 1703081 (response to first round):

• ✅ Fixed double-checked locking (line 648)
• ✅ Fixed stale client recovery on error (line 663)

Commit cde78f3 (response to latest comments):

• ✅ Use context.Background() for workloadapi dial so client outlives individual reconciles (line 650)
• ✅ Reuse http.Client initialized in SetupWithManager() instead of allocating fresh (line 704)
• ✅ Render --verified-fetch-spiffe-socket flag under same OR condition as volume mount (line 183)

All suggestions implemented. Ready for re-review.

[mrsabath]

Summary

Clean, well-motivated change that removes the spiffe-helper sidecar and fetches JWT-SVIDs directly via the go-spiffe SDK, matching the authbridge-proxy architecture. I re-checked each prior-round concern against the current PR head (cde78f3):

• socket mount (cwiklik must-fix): resolved. The spiffe-workload-api volumeMount now uses or verifiedFetch.enabled (and ...operatorAuth.enabled), consistent with the volume declaration.
• mutex held during slow gRPC dial (clawgenti): resolved. fetchJWTSVID dials outside the critical section with correct double-checked locking, and dials with context.Background() so the cached client outlives any single reconcile; the losing goroutine closes its client.
• stale client not retried (clawgenti): resolved. On fetch error the cached client is closed and nilled under an identity check (r.workloadAPIClient == client), so the next reconcile re-dials.
• OIDC discovery every reconcile: resolved. getKeycloakIssuer caches by (keycloakURL, realm) in a sync.Map.

The JWT-SVID is properly treated as a bearer secret (never logged). No spiffe-helper references linger in the tree. My only substantive point is the implicit socket-path coupling between operator-auth and verified-fetch (inline comment on main.go); it works today because the Go default matches the CSI mountPath, but the coupling is fragile. The other two comments are non-blocking.

Areas reviewed: Go (controller, main wiring, go-spiffe SDK), Helm/K8s (manager.yaml, values.yaml), Tests, Security (bearer-token handling), commit/PR conventions.
Commits: 9, all signed-off. CI: all green (Build, Lint, Unit, Integration, CodeQL, Trivy, Helm lint); E2E pending maintainer trigger.

Verdict: APPROVE — the prior CHANGES_REQUESTED items are resolved; remaining comments are a hardening suggestion and a nit.

Note: this approval does not dismiss @cwiklik's standing CHANGES_REQUESTED — that would need their re-review to clear.

[mrsabath]

suggestion: SpiffeSocket for operator JWT-SVID auth is wired to verifiedFetchSpiffeSocket, and the --verified-fetch-spiffe-socket arg is emitted only under verifiedFetch.enabled (manager.yaml:49-51), while JWT-SVID auth is gated on the separate operatorAuth.enabled. When operatorAuth.enabled=true + verifiedFetch.enabled=false, the flag isn't passed and the code relies on the Go default unix:///spiffe-workload-api/spire-agent.sock matching the CSI mountPath. It does today, so this works, but the coupling is implicit and fragile: change verifiedFetch.spiffeEndpointSocket or the default and the two features silently diverge. Consider a dedicated --operator-spiffe-socket (or emit the socket arg under the combined condition) and rename the reused variable so the operator-auth path doesn't depend on a verified-fetch-named default.

[mrsabath]

suggestion (non-blocking): the cached workloadAPIClient is only closed on fetch error, never on operator shutdown. A single leaked gRPC conn for process lifetime is minor (freed on exit), but if the manager supports graceful stop, closing it in a cleanup hook would be tidy. The dial-with-context.Background() and double-checked locking here look correct.

[mrsabath]

nit: the comment says "client creation failure," but workloadapi.New uses non-blocking grpc.DialContext (no WithBlock), so New succeeds and the error actually surfaces from FetchJWTSVID blocking to the 2s ctx timeout. The test passes, but for a different reason than documented. Consider correcting the comment, or asserting the error is a deadline/connection error to make the intent precise.

[mrsabath]

Re-reviewed against cde78f3. All items from my previous review are addressed cleanly:

• must-fix (stale client) — resolved: fetchJWTSVID now clears the cached client under the mutex (guarded on == client) on fetch error, so the next reconcile re-dials after a SPIRE agent restart.
• double-checked locking — resolved: the gRPC dial happens outside the mutex, with a re-acquire to store and Close() on the losing race.
• reconcile-scoped ctx — resolved: dials with context.Background() so the cached client outlives a single reconcile.
• socket arg gating — resolved: --verified-fetch-spiffe-socket is now rendered under the same verifiedFetch.enabled OR operatorAuth.enabled condition as the volume/mount.

Bonus: the OIDC-discovery http.Client is now hoisted to a reusable field initialized in SetupWithManager. Lock discipline checks out (no double-unlock, no leaked lock on the dial-error path), and r.httpClient is always initialized before any reconcile.

No new issues. Areas re-reviewed: Go, Helm. CI green except E2E still pending (passed on the prior equivalent commit).

[clawgenti]

Good progress — all the must-fix items from previous rounds (socket mount gating, mutex-during-dial, stale-client recovery, OIDC discovery caching) are cleanly resolved on HEAD. One latent bug remains around SpiffeSocket argument wiring, plus two nits.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

bug: SpiffeSocket is always wired from verifiedFetchSpiffeSocket, but --verified-fetch-spiffe-socket is only emitted in manager.yaml when verifiedFetch.enabled=true (line ~51). When spiffe.operatorAuth.enabled=true and verifiedFetch.enabled=false, the socket arg is never passed and SpiffeSocket is an empty string — fetchJWTSVID will fail on every reconcile with "SpiffeSocket is required". The volume and mount were correctly extended to cover operatorAuth.enabled, but the CLI arg block was not. Fix: emit --verified-fetch-spiffe-socket also under the operatorAuth.enabled branch (or share the same or condition as the volume).

[clawgenti]

nit: the comment on line 10 says "expected error when connecting to nonexistent socket" and implies workloadapi.New is the failure point, but workloadapi.New uses a non-blocking grpc.DialContext (no WithBlock) so New succeeds immediately. The error is actually returned by the subsequent FetchJWTSVID call once the 2-second context deadline expires. The test is correct; the comment (and the check on line 23–26) should reference the fetch failing, not client creation.

[clawgenti]

suggestion (non-blocking): the cached workloadAPIClient is closed on fetch error but never on operator shutdown. A single leaked gRPC connection for the process lifetime is harmless (freed on exit), but adding a cleanup hook (e.g., mgr.Add(manager.RunnableFunc(...)) in SetupWithManager) would be tidy and matches the pattern expected by graceful-stop tests.