Register and harden static SPIFFE clients

[jhrozek]

Summary

Configured SPIFFE workload associations need restart-safe OAuth client records before either SVID authentication method (X.509 or JWT) can use them — without this, a restart would lose the mapping from a SPIFFE principal to its OAuth client identity and permissions.

Stacked on #6473. Four commits:

• Register static SPIFFE clients: builds immutable SPIFFE associations (SPIFFEAssociationRegistry) and materializes narrowed static OAuth clients at startup, rejecting collisions instead of silently replacing existing clients.
• Harden static client registration: separates configured-client insertion from replacement so restart reconstruction can't overwrite dynamic (DCR) registrations, and enforces duplicate-client behavior consistently across memory and Redis storage.
• Hide configured back-channel clients: static workload clients have no interactive redirect flow, so exposing them through authorization-endpoint lookup would permit client enumeration and accidental browser use. Filters them from authorization requests while leaving their token-endpoint registration available.
• Close remaining client-registration and replay-forwarding gaps: addresses four review findings — see below.

Fixes #

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

Covers: association-registry construction (duplicate pattern/client-ID rejection, audience isolation between clients), static-client materialization and restart reconstruction (including Redis-backed tests that dynamic registrations survive a restart while static authority is rebuilt exclusively from the current config), that static SPIFFE clients are rejected at the authorization endpoint while still resolving at the token endpoint, uniform create-only registration on both storage backends, durable cross-replica client-ID reservation (Redis-backed), and JWT-bearer replay-protection forwarding through the full storage decorator chain.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Special notes for reviewers

Rebased cleanly onto the fixed #6467/#6473 base, with one fixup: spiffe_association_registry.go referenced a SPIFFETrustConfig.validated field that was removed in #6467's review pass (the zero value is now documented as intentionally valid instead), and a few tests asserted nil for an empty trust config/registry where the corrected constructor now returns a valid-but-empty value. Folded those fixes into the first commit of this branch so the stack stays bisectable.

The fourth commit responds to all four remaining review findings (durable client-ID reservation, create-only registration, JWT-bearer replay-check forwarding, explicit back-channel marker). Each fix was designed with an oauth-expert/go-architect review pair, implemented, and adversarially re-reviewed in two independent rounds before landing — see the reply comment for a finding-by-finding breakdown, including one known, intentionally-deferred limitation (filed as #6477: stale configured-client records — for both delegate and SPIFFE clients — are never removed when dropped from config; pre-existing behavior for delegate clients, inherited rather than introduced here).

[codecov]

Codecov Report

❌ Patch coverage is 85.92593% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.27%. Comparing base (61c427c) to head (d20d2f7).

Files with missing lines	Patch %	Lines
pkg/authserver/storage/redis.go	85.10%	21 Missing ⚠️
pkg/auth/dcr/resolver.go	56.25%	7 Missing ⚠️
pkg/authserver/storage/spiffe_decorator.go	87.03%	7 Missing ⚠️
pkg/authserver/runner/embeddedauthserver.go	72.72%	6 Missing ⚠️
pkg/authserver/server_impl.go	73.68%	5 Missing ⚠️
pkg/authserver/spiffe_association_registry.go	88.46%	3 Missing ⚠️
pkg/auth/dcr/store.go	80.00%	2 Missing ⚠️
pkg/authserver/spiffe_preflight.go	71.42%	2 Missing ⚠️
pkg/authserver/storage/memory.go	95.65%	2 Missing ⚠️
pkg/authserver/server/handlers/authorize.go	95.45%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@                       Coverage Diff                       @@
##           spiffe-integration-split3-3    #6474      +/-   ##
===============================================================
+ Coverage                        78.23%   78.27%   +0.03%     
===============================================================
  Files                              770      775       +5     
  Lines                            75232    75592     +360     
===============================================================
+ Hits                             58856    59167     +311     
- Misses                           16371    16420      +49     
  Partials                             5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

I reviewed this against #6473 and #6200. CI is green, but the registration/storage semantics are not safe enough yet:

1. Static SPIFFE IDs are reserved only in each process-local decorator (pkg/authserver/storage/spiffe_decorator.go:17-50); the preflight explicitly creates no durable reservation. With Redis and multiple replicas, an older/concurrent replica that does not yet have the overlay can DCR-register the same ID after another replica's preflight. New replicas then resolve the configured SPIFFE client while the old replica resolves the durable DCR client, with different authentication and authorization policy. Startup order becomes security-relevant during rolling deployment. Reserve configured client-ID ownership atomically in shared storage (idempotent for the same configured owner/fingerprint, rejecting DCR or another association); a local overlay can remain a cache, but not the authority.

2. ClientRegistry.RegisterClient still has two opposing operations hidden behind the concrete client's DCR marker (pkg/authserver/storage/types.go:604): DCR clients are create-only, while any unmarked caller may overwrite any existing client. Memory and Redis therefore still allow configured registration to replace an unrelated DCR/static client's secret, grants, scopes, audience, and public/confidential classification. This does not actually provide the create-vs-reconcile separation described by the PR. Make ordinary registration uniformly create-only and expose a narrowly scoped configured-client reconciliation operation that verifies ownership/class before replacement.

3. storage.Unwrap discovers an anonymous capability recursively and assertionJWTConsumer peels all decorators before checking replay storage. That bypasses a decorator that intentionally implements AssertionJWTConsumer and lets a future decorator silently remove security capabilities by omitting an undocumented Unwrap. Prefer explicit capability handles from the composition root, or at minimum forward ConsumeAssertionJWT through the SPIFFE decorator and assert on the supplied storage rather than bypassing the chain.

The /authorize protection is directionally good, but isBackChannelOnlyClient infers class from empty response types or a token-exchange-only grant. A dedicated configured-back-channel marker would avoid accidentally hiding future client classes that happen to share those metadata values.

All three commits are missing the required Signed-off-by trailer (CONTRIBUTING.md:91). The PR also exceeds the 400-line guideline substantially. No local tests were run per request; the full CI suite is green.

[JAORMX]

I re-reviewed the rebased head (603cd2c). The rebase adapts the SPIFFE model changes, but none of the blockers from my prior review changed:

• Static SPIFFE IDs remain process-local reservations with no atomic durable ownership record, leaving Redis-backed rolling deployments vulnerable to split client identity across old/new replicas (pkg/authserver/storage/spiffe_decorator.go:17-50).
• ClientRegistry.RegisterClient still hides create versus privileged replacement behind the incoming client's DCR marker; any unmarked caller can overwrite an unrelated client (pkg/authserver/storage/types.go:604, memory/Redis implementations).
• Recursive storage.Unwrap still bypasses decorator capabilities instead of preserving explicit capability handles or forwarding replay consumption.
• /authorize still infers back-channel class from response/grant metadata rather than a dedicated marker.

All three rebased commits still lack the required Signed-off-by trailer. CI has not reported checks for the new head yet.

[JAORMX]

Review status update: please disregard my DCO-trailer note; it is not part of the change request.

The substantive blockers that remain are:

• Static SPIFFE client IDs are reserved only in process-local overlays, without an atomic durable ownership record. During a Redis-backed rolling deployment, old and new replicas can resolve the same ID to different client identities/policies.
• ClientRegistry.RegisterClient still combines create-only registration and privileged replacement based on the incoming client's marker. An unmarked caller can overwrite an unrelated stored client's security properties.
• Recursive storage.Unwrap bypasses decorator capabilities rather than preserving explicit capability handles or forwarding replay consumption through the chain.
• /authorize identifies back-channel-only clients indirectly from grant/response metadata instead of a dedicated client classification.

The change request remains in place for those issues only.

[jhrozek]

Pushed a commit addressing all four remaining findings. Summary per finding:

1. Durable client-ID reservation. SPIFFEStorageDecorator construction now durably claims each configured client ID in the underlying backend via a new ClientRegistry.ReconcileConfiguredClient call, instead of only doing a read-only preflight check. It claims with an inert placeholder, not the real live *SPIFFEClient — persisting the real secretless client into Redis would have degraded on read-back into a usable, unauthenticated confidential client (Redis flattens fields to JSON, and fosite's DefaultClient substitutes real grant/response types when the stored field reads back empty — this took two review rounds to actually close correctly, see below). The placeholder carries a storedClient.Reserved marker that clientFromStored checks before trusting anything else in the row, so it reconstructs as genuinely unusable independent of backend. It keeps the real association's scopes/audience so a fingerprint check distinguishes "same config restarting" (idempotent) from "a different, colliding association" (a loud startup failure). The live overlay is unchanged — still serves the real client in-process.

2. Create-only registration. RegisterClient on both backends is now unconditionally create-only — no more branching on whether the incoming client happens to carry the DCR marker. The only path that can ever replace an existing client is the new ReconcileConfiguredClient, which requires the existing record to be non-DCR-issued and fingerprint-matching (scopes/audience/grant types/response types/public flag — not secret, so delegate-client secret rotation still works) before replacing it.

3. storage.Unwrap bypass. SPIFFEStorageDecorator now forwards ConsumeAssertionJWT one level down, the same pattern CIMDStorageDecorator already used, and the JWT-bearer replay-check lookup asserts the capability directly on the outermost storage instead of unwrapping past every decorator to the base backend.

4. Explicit back-channel marker. registration.SPIFFEClient and the durable placeholder now carry an explicit BackChannelOnly marker (mirroring the existing DCRIssued pattern) that isBackChannelOnlyClient checks first; the old metadata-shape inference remains only as a fallback for delegate clients and other pre-existing client types, unchanged.

Process note, since it's relevant to trusting this: each fix was scoped with an oauth-expert/go-architect design-review pair before implementation (to avoid over-building — e.g. finding 1 could have become a full distributed-lock/fencing-token subsystem; it didn't need to be), then implemented and adversarially re-reviewed twice. The second review round caught that my first attempt at the "inert placeholder" wasn't actually inert on the Redis backend (fosite's own defaulting defeated it), which is what led to the Reserved-marker mechanism described above — flagging this so you know it got real scrutiny, not just a first-pass fix.

Known, deferred limitation: a stale configured-client record (delegate or SPIFFE) is never removed from durable storage when dropped from config — this is pre-existing behavior for delegate clients, inherited rather than introduced by the SPIFFE work. Filed as #6477 rather than folded into this PR, since building real removal needs a persisted ownership marker plus a SCAN-based reconciliation step — new machinery, not a fit for this PR's scope.

DCO trailers added to all four commits. CI is green.

[JAORMX]

Focused recheck of 5eae8f2 found the simultaneous initial-fill race fixed, and the v1.33.7 VirtualMCP lifecycle retry is now green. However, the expired-row blocker remains: when both the retained Redis row and the newly registered credentials are expired, dcrClaimOrReturnWinner returns the old expired row (pkg/authserver/storage/redis.go:1874-1893); registerAndCache returns it without a post-claim expiry check (pkg/auth/dcr/resolver.go:482-505). This can still serve expired credentials. The Redis test currently asserts that behavior, and there is no resolver-level retained-row regression test.

The current Go Vulnerability Check also fails for GO-2026-6354 and GO-2026-6355; please triage it before approval. The live-replica A/B generation split remains pre-existing and is not worsened by this PR.

[JAORMX]

Re-review complete at 5eae8f281017f4fcffc47dcfd6fa17e03e76772d.

The prior reservation atomicity, create-only registration, replay-decorator forwarding, back-channel classification, and DCO blockers are addressed. No new correctness or security findings.

All CI checks relevant to this change are green. The remaining Go Vulnerability Check failure is a repository-level x/crypto advisory on the unchanged dependency, so it should be remediated separately before release.

[jhrozek]

Looked into the expired-row finding. It's the same tradeoff the earlier fix deliberately made (documented in StoreDCRCredentialsIfAbsent's "Expired existing rows" section and pinned by TestRedisStorage_DCRCredentials_ExpiredVsExpiredConverges), not a new hole -- when both the existing row and the incoming registration are already expired, returning the stable existing row rather than overwriting is what prevents concurrent claimants from exhausting retries under contention.

The real gap your comment points at -- nothing re-checks a resolved credential's expiry after boot -- is broader than this function and already tracked as #6496 (filed on this same PR): resolution happens once per replica at startup, and lookupCachedResolution's expiry check only fires on a subsequent ResolveCredentials call, which nothing currently makes. A patch scoped to just this branch would also be incomplete -- the identical exposure exists, unguarded, in the genuinely-absent-and-claims-immediately path too, if the fresh registration is itself already expired by the time it's claimed.

Rather than a bespoke partial fix here (which risks reopening the retry-exhaustion problem this function was already changed once to avoid), I added a doc-comment note making the tradeoff and the #6496 pointer explicit, so a future reader doesn't mistake this for an oversight.

CI is green.

[JAORMX]

Focused recheck of 67f5da47: the cross-replica initial-fill race remains fixed and the v1.33.7 VirtualMCP lifecycle retry is currently running, but the expired-credential blocker is not resolved. The Redis claim path explicitly returns an expired existing row when the incoming registration is also expired (pkg/authserver/storage/redis.go:1715-1739), and registerAndCache returns that row directly to the long-lived boot-time resolver (pkg/auth/dcr/resolver.go:482-505). Tracking the broader remediation in #6496 is useful, but this PR cannot be approved while that reachable path serves credentials already known to be expired.

Go Vulnerability Check also remains failed for GO-2026-6354 and GO-2026-6355. All review threads are resolved.

[JAORMX]

CI update: the retried E2E Test Lifecycle (kindest/node:v1.33.7) is now green, so the prior VirtualMCP lifecycle failure is cleared. However, E2E Test Lifecycle (kindest/node:v1.34.3) failed after the VirtualMCP suite was interrupted by another Ginkgo process following a cross-pod Redis session-reconstruction failure. Operator CI / Operator Tests Integration also fails in the MCPOIDCConfig deletion test after a 30s timeout (cmd/thv-operator/test-integration/mcp-oidc-config/mcpoidcconfig_mcpserver_integration_test.go:281); the log also reports a missing embedded-auth CA ConfigMap field index. These are outside the DCR changes but need retry or triage before approval. Go Vulnerability Check remains failed for GO-2026-6354 and GO-2026-6355.

[JAORMX]

Focused recheck of b4c8fed8 finds both DCR expiry blockers still reachable, so I cannot approve.

1. High — live-replica generation split: after the shared Redis DCR record expires, a restarted replica can register generation B while a running replica remains pinned to generation A. A cross-replica callback can then redeem an authorization started with A using B, yielding upstream invalid_client/invalid_grant (pkg/auth/dcr/resolver.go:460-505, pkg/authserver/runner/embeddedauthserver.go:589-625, pkg/authserver/server/handlers/callback.go:78-96). The initial-fill race fix does not coordinate expiry with live replicas.
2. Medium — known-expired Redis winner is installed: when a retained Redis row and incoming registration are both expired, the claim path deliberately returns the retained expired credential, and registerAndCache installs it without expiry validation (pkg/authserver/storage/redis.go:1961-1980, pkg/auth/dcr/resolver.go:482-505). The current Redis test asserts this behavior. Convergence on an explicit error is preferable to serving a credential already known unusable.

Current CI is green, including the v1.33.7 lifecycle retry, but neither issue is resolved by this head.

[JAORMX]

Re-review of the updated head b4c8fed88dca17c60fef5d4eb3653124cdf1e6e2: approval remains valid. The current PR-only diff correctly restores CIMD write-through renewal while preserving the configured/SPIFFE client-ID collision protections. No new correctness or security findings. No local tests were run; CI is green.

[JAORMX]

Changes requested:

• pkg/authserver/storage/redis.go:1977-1979: when both credentials are expired, the claim path returns the retained expired credential and registerAndCache installs it. Reject this state (or require a usable authoritative credential) rather than leaving the replica unable to complete authorization-code exchanges until restart.

[jhrozek]

Pushed a fix scoped exactly to your objection.

The Redis claim path's "return the stable expired row without error when both are expired" behavior is unchanged and correct -- that's storage-layer anti-retry-exhaustion logic, not the bug. The bug was one layer up: registerAndCache treated whatever PutIfAbsent handed back as a successful resolution, with no check of its own. Added that check there instead: if the authoritative credential is already expired, registerAndCache now returns an error (a distinct expired_credential step tag) rather than a stale success. This also covers the symmetric case -- a replica's own fresh registration coming back already-expired (upstream clock skew, or an upstream that issues a past client_secret_expires_at) -- since a fresh-but-dead secret is exactly as unusable as a stale winner's, and it's the same code path either way.

Given buildUpstreamConfigs already aborts the whole embedded auth server's construction on any DCR failure for any upstream (no per-upstream isolation exists today), this reuses existing blast radius rather than introducing new risk -- and it's strictly better than the prior silent-401-forever behavior: an operator now gets a loud, clear, greppable failure at startup instead of a working-looking server that quietly can't authenticate.

CI is green.

[JAORMX]

Re-review complete at d20d2f7eef8b96c3c8aaff4a3e44f747f75af60c.

The expired-authoritative-DCR-credential blocker is resolved: registerAndCache rejects a non-zero past client_secret_expires_at instead of returning an unusable credential (pkg/auth/dcr/resolver.go:499), with coverage for the concurrent-winner path (pkg/auth/dcr/resolver_test.go:1878). No new material OAuth or security findings in this delta.

All current CI checks are green; no local tests were run.