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 84.54259% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.24%. Comparing base (64484b9) to head (718c4e9).

Files with missing lines	Patch %	Lines
pkg/authserver/storage/redis.go	79.74%	16 Missing ⚠️
pkg/auth/dcr/resolver.go	41.66%	7 Missing ⚠️
pkg/authserver/runner/embeddedauthserver.go	72.72%	6 Missing ⚠️
pkg/authserver/server_impl.go	73.68%	5 Missing ⚠️
pkg/authserver/storage/spiffe_decorator.go	89.79%	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/server/handlers/authorize.go	95.45%	1 Missing ⚠️
pkg/authserver/server/registration/client.go	66.66%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@                       Coverage Diff                       @@
##           spiffe-integration-split3-3    #6474      +/-   ##
===============================================================
+ Coverage                        78.21%   78.24%   +0.03%     
===============================================================
  Files                              770      775       +5     
  Lines                            75127    75400     +273     
===============================================================
+ Hits                             58758    58995     +237     
- Misses                           16364    16400      +36     
  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]

Re-review complete. All four prior blockers are addressed: static SPIFFE IDs now have durable Redis-safe claims; normal registration is create-only with fingerprint-gated configured reconciliation; assertion-JWT replay consumption is forwarded through the decorator chain; and back-channel status uses an explicit marker. Current CI is green.

One new medium-severity multi-replica correctness gap remains in DCR cache fill. dcrFlight coalesces only within one process; two replicas can independently register different upstream OAuth clients on a shared-cache miss, both unconditionally store the same cache key, and continue with different in-memory credentials (pkg/auth/dcr/resolver.go:368-405, 421-476; pkg/authserver/storage/redis.go:1690-1761). A callback landing on the other replica then redeems with a different client ID/secret and fails. Please make initial cache population a shared atomic claim/CAS (or read the durable winner before returning) unless strict authorization-flow session affinity is an explicit requirement.

[JAORMX]

Rechecked at 0fa16aa: the multi-replica DCR cache-fill race remains. ResolveCredentials only coalesces within a process (pkg/auth/dcr/resolver.go:368-401); separate replicas can independently register upstream clients and each return its own credentials after an unconditional shared-store write (resolver.go:460-485, pkg/authserver/storage/redis.go:1758). The static-client Redis WATCH protection does not cover DCRCredentialStore.

Current unit/lint/docs/security checks are green; the E2E and Operator portions of the latest CI run are still in progress.

[jhrozek]

Pushed a fix for the DCR cache-fill race, as a new commit.

Scoped it with an oauth-expert/go-architect/redis-valkey-advisor design pass first (per your note about not requiring a newer Redis than we already support), then implemented and adversarially re-reviewed.

CredentialStore.Put → PutIfAbsent, DCRCredentialStore.StoreDCRCredentials → StoreDCRCredentialsIfAbsent: the contract changes from upsert to create-if-absent, always returning the authoritative durable value (the caller's own on a successful claim, the concurrent winner's otherwise) — registerAndCache now uses whatever the store says is authoritative instead of trusting its own local registration. Redis claims via SET...NX (the same reservation-lock shape already used twice in that file, no WATCH/MULTI needed — there's no read-then-decide step here, just first-writer-wins). MemoryStorage treats an entry as absent only when its ClientSecretExpiresAt is non-zero and already past, for contract symmetry plus its own correctness case.

The re-review caught one real gap in the first pass: on a lost claim, the follow-up read could hit a genuinely-evicted key (the winner's TTL can be as short as 1 second when its ClientSecretExpiresAt was already past) and turned that into a hard, permanent error rather than retrying the claim — worth calling out since DCR resolution runs once at startup, so an unhandled miss there fails the whole process, not just one request. Fixed with a small bounded retry around the claim-or-read cycle.

CI is green.

[JAORMX]

Rechecked the new create-if-absent implementation. It fixes the original concurrent-startup cache-fill race: a Redis NX claim yields one authoritative registration and losers use the durable winner.

One medium lifecycle gap remains for finite client-secret expiry. A still-running replica retains its resolved upstream credentials after the Redis entry expires; a later-starting replica then sees a miss, registers a new client, and becomes authoritative. The old replica can still initiate authorization with the prior client while another replica handles the callback with the new credentials, producing replica-dependent code-exchange failure (pkg/authserver/storage/redis.go:1765-1794; pkg/authserver/runner/embeddedauthserver.go:593-618). Please coordinate rotation/retirement across live replicas, or keep the initiating credential generation available until all in-flight authorization flows complete. A two-replica expiration/restart regression test would cover this.

[JAORMX]

CI update: E2E Test Lifecycle (kindest/node:v1.33.7) failed on this head. The log shows the VirtualMCP suite timing out/interrupted after a cross-pod Redis session-reconstruction failure; the remaining suites were interrupted. This is outside the DCR path but needs retry or triage before approval.

[JAORMX]

Focused recheck of 718c4e9c:

• High — live-replica expiry split remains. After Redis expires the cached DCR credential, a restarted replica creates generation B while a running replica keeps generation A in its immutable upstream runtime config (pkg/authserver/runner/embeddedauthserver.go:583-618; pkg/authserver/storage/redis.go:1712-1716,1765-1772). Cross-replica callbacks can therefore redeem with different client registrations. PutIfAbsent resolves only simultaneous initial fill.
• Medium — stale Redis row can win an expired-cache refresh. The resolver treats expired credentials as a miss and registers a replacement, but Redis SET NX can return a still-retained expired row; registerAndCache returns it without an expiry recheck (pkg/auth/dcr/resolver.go:482-505,711-720; pkg/authserver/storage/redis.go:1791-1813). This is asymmetric with memory storage, which replaces expired entries.

The initial concurrent-fill race is fixed, but these expiry paths remain blockers. CI also has the existing failed v1.33.7 lifecycle job.

[jhrozek]

Traced the High finding (live-replica expiry split) back through git history — it's a pre-existing architectural gap, not something this PR introduces or worsens. The single-shot-resolve-at-startup architecture (no refresh path at all) goes back to #3540/#5044, and the TTL/expiry-tied Redis caching goes back to #5195 — both well before this PR touched anything. This PR's only change in this area (718c4e9c) is the create-if-absent fix for the simultaneous first-fill race between replicas starting at the same time; it doesn't affect a long-running replica's never-refreshed in-memory config one way or the other.

Filed #6496 to track it separately, since a real fix needs new infrastructure (scheduled refresh or push invalidation) rather than a bounded change to the code this PR touches.

Still working the Medium finding (stale Redis row winning an expired-cache readback) — that one is in scope here and I'll push a fix shortly.