Normalize canonical inbound grants - #6473
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6473 +/- ##
==========================================
+ Coverage 78.25% 78.38% +0.12%
==========================================
Files 770 776 +6
Lines 75436 76094 +658
==========================================
+ Hits 59035 59647 +612
- Misses 16396 16442 +46
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a93bd9b to
047b301
Compare
JAORMX
left a comment
There was a problem hiding this comment.
I reviewed this against #6467 and #6200. CI is green, but I found two blocking model/correctness issues:
-
SPIFFEClientsis nested underinbound_grants.token_exchange(pkg/authserver/inbound_grants.go:19-24). That makes SPIFFE client authentication subordinate to RFC 8693: disabling or omitting token exchange removes the association, and every SPIFFE-authenticated client is necessarily token-exchange-enabled. #6200 calls forspiffeClientAuthunderinboundGrantswith grants and token-exchange permission configured per association. Authentication method and grant authorization need to remain independent in the canonical schema. -
SPIFFE validation reports paths under
inbound_grants.spiffe_client_auth[...], but this PR's actual serialized path isinbound_grants.token_exchange.spiffe_clients[...](pkg/authserver/spiffe_trust.go, reached fromRunConfig.Validate). Operators therefore get errors naming a field that does not exist. Please make the validation paths match the final schema.
The normalization direction is useful, but it currently projects the canonical wire types back into the legacy combined TrustedIssuer runtime representation. That leaves parallel policy shapes and several field-by-field copy/conflict-detection sites that must evolve in lockstep. I would prefer one grant-oriented normalized runtime model consumed by validation/provider construction, with legacy and canonical wire inputs translated into it once.
Standards notes: the commit is missing the required Signed-off-by trailer (CONTRIBUTING.md:91), and the PR is well beyond the repository's 400-line guideline. The signoff must be fixed; please also consider whether the compatibility normalization can be split from the SPIFFE wiring to reduce review risk.
8abeeaa to
108bc9c
Compare
047b301 to
7c5e172
Compare
JAORMX
left a comment
There was a problem hiding this comment.
I re-reviewed the rebased head (7c5e172). The rebase fixes the two model issues from my previous review: spiffe_client_auth remains a sibling of the grant families, validation paths now match the schema, and the commit has its DCO trailer.
One new enablement inconsistency blocks approval:
- Canonical
inbound_grantscan disable token exchange, and this PR correctly removes the Fosite handler and discovery advertisement viaTokenExchangeEnabled. However,/oauth/registerstill callsValidateDCRRequestwithout that capability (pkg/authserver/server/handlers/dcr.go:243-257). It can therefore return201 Createdfor a token-exchange-only client even though the server has no token-exchange handler. This is especially direct forprivate_key_jwt, whose token-only path explicitly accepts a token-exchange registration. Please threadTokenExchangeEnabledinto DCR validation (or rejectallow_private_key_jwt_registrationwhen canonical configuration disables token exchange) and add a regression test proving a disabled grant cannot be registered.
The architecture doc also still places SPIFFE clients at inbound_grants.token_exchange.spiffe_clients; the implemented path is the sibling inbound_grants.spiffe_client_auth. Please update those references and make the documented legacy/canonical exclusion scope match the family-wide behavior in NormalizeInboundGrants.
CI currently has two failures. Both look unrelated to this authserver diff: one VirtualMCP Redis/session E2E timeout interrupted the remaining specs, and one MCPOIDCConfig integration test timed out amid resource-version conflicts and a missing test field index. They still need a clean rerun before approval.
|
Pushed a commit that resolves both blocking issues — thanks for catching these.
On the normalization-direction note (projecting canonical types back into the legacy Signed-off-by trailer fixed. CI is green on the amended commit. |
108bc9c to
e497cae
Compare
7c5e172 to
a1a21ea
Compare
JAORMX
left a comment
There was a problem hiding this comment.
I re-reviewed the current rebased head (a1a21ea). The earlier SPIFFE placement and validation-path fixes remain good, but the latest blocker is still present: validateDCRRequest does not receive TokenExchangeEnabled, so /oauth/register can create a token-exchange-only client after canonical configuration has removed the token-exchange handler and discovery advertisement (pkg/authserver/server/handlers/dcr.go:243-257). Please gate DCR validation on the effective capability and add the disabled-grant regression test.
The architecture documentation still names inbound_grants.token_exchange.spiffe_clients at docs/arch/17-token-exchange-delegation.md:127, while the implemented path is inbound_grants.spiffe_client_auth. Its “mutually exclusive per issuer” wording around line 703 also does not match the family-wide conflict implemented by NormalizeInboundGrants; please align one side.
The amended commit has its DCO trailer. CI is still running on this head.
e497cae to
3413cc2
Compare
a1a21ea to
f1fc2d8
Compare
Restart reconstruction must not overwrite dynamic registrations or weaken the existing DCR replacement contract. Separate configured-client insertion from replacement and enforce duplicate behavior consistently in memory and Redis storage. Refs #6200 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
Static workload clients have no interactive redirect flow, and exposing them through authorization lookup would permit enumeration and accidental browser use. Filter configured back-channel clients from authorization requests while leaving their token-endpoint registration available. Refs #6200 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
A reviewer (JAORMX) found four remaining issues in how this branch registers and reserves static SPIFFE clients: RegisterClient let any caller overwrite any existing client just by omitting a marker, the SPIFFE overlay never durably reserved its client IDs so a rolling deployment could let an old replica hand the same ID to a DCR registration, JWT-bearer replay protection bypassed every storage decorator by unwrapping straight to the base backend, and the /authorize back-channel guard inferred client class from metadata shape instead of an explicit marker. This commit closes all four, designed with an oauth-expert/go-architect review pair per finding and implemented and adversarially re-reviewed in two rounds before landing. Registration is now uniformly create-only. `RegisterClient` on both storage backends no longer branches on whether the incoming client carries the DCR-issued marker — it always fails if a client with that ID exists, full stop, so no caller (present or future) can silently overwrite an existing registration by simply forgetting to mark it. The only path that can ever replace a client is the new `ClientRegistry.ReconcileConfiguredClient`, which creates on first use and otherwise requires the existing record to be non-DCR-issued and have a matching fingerprint (scopes, audience, grant/response types, public flag — never the secret, so delegate-client secret rotation still reconciles) before replacing it. Delegate-client startup registration now goes through this method instead of `RegisterClient`. Static SPIFFE client IDs are now durably reserved, not just preflight-checked. The overlay previously only read durable storage to detect a collision before serving clients in-process; nothing was ever written, so an older replica mid-rollout could still win a race and DCR-register the same ID with a different client shape. Construction now calls `ReconcileConfiguredClient` against the underlying backend with an inert placeholder for each configured ID — never the real `*SPIFFEClient` object, since persisting that directly into Redis would have degraded on read-back into a usable, unauthenticated confidential client (Redis flattens a client's fields to JSON, and `fosite.DefaultClient` substitutes real grant/response types when the stored field reads back empty). The placeholder is instead marked with a `storedClient.Reserved` bit that `clientFromStored` checks before trusting anything else in the row, so it reconstructs as genuinely unusable — no grant type, no response type, no secret — independent of backend. It keeps the real association's scopes/audience so the fingerprint check can tell "same config restarting" (idempotent) from "a different, colliding association" (a loud startup failure instead of silent divergence). The live overlay is unchanged: it still serves the real client in-process, exactly as before. The reconcile call against Redis uses a bounded WATCH/MULTI retry loop, since go-redis does not itself retry a concurrent write. JWT-bearer replay protection no longer bypasses the storage decorator chain. It used to call `storage.Unwrap`, peeling every decorator down to the base backend before checking for replay-consumption support — so a decorator sitting in between could never intercept or audit that call, and a future one could silently lose the capability by omitting an undocumented `Unwrap` method. `SPIFFEStorageDecorator` now forwards `ConsumeAssertionJWT` one level down, the same way `CIMDStorageDecorator` already did, and the lookup asserts the capability directly on the outermost storage instead of unwrapping past the chain. The /authorize back-channel guard is now marker-driven for the client types this stack introduces. `isBackChannelOnlyClient` inferred "no interactive flow" from metadata shape alone (empty response types, or an exact token-exchange grant) — a future client class sharing that shape by coincidence would be silently and incorrectly hidden. `registration.SPIFFEClient` and the durable placeholder now carry an explicit `BackChannelOnly` marker (mirroring the existing `DCRIssued` marker pattern) that the guard checks first; the metadata-shape inference remains as a fallback for delegate clients and any other existing client type, unchanged. Refs #6200 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
A reviewer found that two replicas racing on the same outbound DCR (RFC 7591) cache-miss could each independently register a different OAuth client with the upstream IdP — dynamic registration always mints a fresh client_id/secret — then whichever replica's write landed last in the shared Redis cache silently won. The losing replica keeps the client it registered baked into its own config for the rest of its process lifetime (DCR resolution runs once per upstream at startup, never re-resolved), so it no longer agrees with the durable cache about which client it holds credentials for. dcrFlight (a singleflight.Group) only coalesces concurrent callers within one process; it has no cross-replica reach. Change the cache-population contract from upsert to create-if-absent, returning the authoritative durable value either way: the caller's own resolution on a successful claim, or the concurrent winner's otherwise. CredentialStore.Put becomes PutIfAbsent, and DCRCredentialStore.StoreDCRCredentials becomes StoreDCRCredentialsIfAbsent; registerAndCache now returns whichever resolution the store says is authoritative instead of trusting its own local registration, and logs (at Debug, without ever including a secret) when this replica lost the race. Callers MUST use the returned value — RFC 7591 guarantees nothing about the two registrations converging. Redis claims the key with SET...NX (the same reservation-lock shape already used twice in this file for ClientAssertionJWTValid and ConsumeAssertionJWT), not WATCH/MULTI: unlike ReconcileConfiguredClient, this write has no read-then-decide step to protect, so a plain atomic NX claim is sufficient. On a lost claim it reads back the winner through the existing GetDCRCredentials path rather than a second, hand-rolled unmarshal, and retries the whole claim-or-read cycle (bounded) if the winner's row evicts between the failed NX and the read — its TTL can be as short as one second when the caller's ClientSecretExpiresAt was already in the past, so this is a real, reachable window, not a hypothetical one, and the alternative (a hard error) would turn a retryable race into a permanent startup failure. MemoryStorage's implementation treats an existing entry as absent only when its ClientSecretExpiresAt is non-zero and already past — otherwise it returns the existing entry unchanged rather than overwriting it. A single process's dcrFlight already prevents a live race there; this is contract symmetry with Redis, plus the correctness case Redis gets from TTL eviction: without the expiry check, a never-expiring entry can never be reclaimed, but a naive "any existing entry blocks re-registration" check would also permanently pin an already-expired one that should be re-registered. Refs #6200 Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
Rebasing this stack onto main picked up CIMD's write-through client persistence (main), which relies on RegisterClient acting as an upsert to renew a resolved client's row on every document re-fetch. This stack's own registration hardening made RegisterClient create-only for DCR-issued clients, so every renewal after the first fetch for a given CIMD client_id would silently fail, leaving stale client data (and, absent the token-exchange-triggered RenewClientTTL path, a stale TTL) in storage. Add UpsertDCRIssuedClient, a narrow fourth ClientRegistry operation distinct from both RegisterClient (unauthenticated DCR, stays create-only) and ReconcileConfiguredClient (fingerprint-locked, the wrong shape since a CIMD document can legitimately change between fetches). It creates the row if absent, replaces and renews it only when the existing row is itself DCR-issued, and refuses with ErrAlreadyExists otherwise -- protecting a configured or SPIFFE client from being clobbered. Wire CIMDStorageDecorator.fetch to call it instead of RegisterClient, and give SPIFFEStorageDecorator the same reserved-ID guard its other overrides already enforce. Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
StoreDCRCredentialsIfAbsent deliberately returns a stable-but-expired existing row without error when both it and a fresh registration attempt are already expired, to avoid every concurrent claimant re-entering the write path and exhausting retries. That's the right call for the storage layer, but registerAndCache was treating whatever it got back as a successful resolution regardless -- handing callers a client_secret the upstream has already invalidated. Reject an already-expired authoritative credential in registerAndCache instead, where "expired means unusable" is actually DCR policy, not storage policy. This also covers a replica's own fresh registration turning out already-expired (upstream clock skew, or an upstream that issues a past client_secret_expires_at) -- the same guard applies either way, since a fresh-but-dead secret is exactly as unusable as a stale winner's. Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
95b1339 to
319de92
Compare
JAORMX
left a comment
There was a problem hiding this comment.
Deep re-review at 319de92a14c711a5c2f6209a23409662c905272d found two additional implementation defects:
- P1 — SPIFFE
resourcesare discarded.pkg/authserver/spiffe_association_registry.go:55-58andpkg/authserver/server/registration/spiffe_client.go:31-44retain scopes/audiences only, whilepkg/authserver/spiffe_trust.go:138-150independently validatesResources. The token-exchange handler checks RFC 8707resourcethroughclient.GetAudience()(pkg/authserver/server/tokenexchange/handler.go:828-851), soresources: [R]cannot authorizeresource=R, whileresource=Ais incorrectly allowed when onlyaudiences: [A]is configured. Preserve resource authorization separately and test disjoint audience/resource permissions. docs/arch/17-token-exchange-delegation.md:643-645says an external issuer is not retained across delegated-token re-exchange, butpkg/authserver/server/tokenexchange/handler.go:575-579copies it. Align the documentation or behavior and test the intended provenance contract.
The existing canonical-omission documentation blocker remains as previously reviewed.
RFC 8707 resources and RFC 8693 audiences are independent request dimensions for SPIFFE-authenticated token exchange, but the resource allowlist was validated at config time and then silently discarded when building the runtime client: staticClients() only passed scopes and audiences to NewSPIFFEClient, and grantResourceAudience checked every "resource" request parameter against GetAudience() regardless. A client configured with disjoint audiences and resources could get a token for a resource that was never in its resources allowlist (as long as it happened to match an audience), while a legitimately configured resource was wrongly denied. Give SPIFFEClient its own Resources() accessor alongside Audiences(), thread policy.Resources() through the association registry, and have grantResourceAudience check a resourceScopedClient's Resources() instead of GetAudience() when the client implements it. Other client types (DelegateClient, DCR clients) are unaffected since they don't implement the new interface and keep using GetAudience() exactly as before. An earlier iteration of this client (before the current three-arg constructor) modeled resources correctly and had a test asserting GetAudience() stayed empty absent an explicit audience -- fail closed. That assertion was removed rather than adapted when the field was dropped, and its replacement asserted the new fail-open behavior as intended. Restore disjoint-resources/audiences coverage at the client, registry, and handler layers so the same regression can't hide again. Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at 7ff760b2c9c831c5172ae1779c58f6fde3911ac2: the new resource-vs-audience authorization behavior is correct and well covered, but its durable configuration fingerprint still omits the new resource policy.
- High:
pkg/authserver/storage/spiffe_decorator.go:143-149constructs the durable static-client placeholder with scopes and RFC 8693 audiences only. Two replicas using the same SPIFFE client ID/scopes/audiences but different RFC 8707resourcestherefore reconcile as identical during a rolling policy change, while their in-process clients authorize different resource sets. Include the resource allowlist in the placeholder fingerprint/reconciliation contract for memory and Redis, and add a conflicting-resources regression test.
The resource authorization implementation itself now correctly keeps Resources() separate from GetAudience() (pkg/authserver/server/tokenexchange/handler.go:810-865). Local task test did not complete before the command deadline; CI should provide the authoritative full-suite result.
memory.go and redis.go each hand-rolled their own fingerprint comparison to decide whether a reconciling client is the same logical client or a colliding one, kept in sync only by a doc comment. Replace both with one clientFingerprint value type and two adapters so the comparison exists once and cannot drift between backends.
staticClientPlaceholder durably fingerprinted a SPIFFE association on scopes and audiences only, so two associations at the same client ID differing solely in their RFC 8707 resource allowlist would reconcile as the same client instead of failing loudly. Thread resources through inertPlaceholderClient, clientFingerprint, and storedClient so the durable identity matches what the client actually authorizes.
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at bcb58e0ba9a304412b4a80c79b1b3c1599d00aba: the RFC 8707 resource allowlist is now included in both memory and Redis placeholder fingerprints, with conflict coverage. That resolves the last-head blocker.
The deeper pass found remaining issues:
- High: the durable placeholder still fingerprints only OAuth policy (
pkg/authserver/storage/spiffe_decorator.go:143-176;pkg/authserver/storage/types.go:613-652), not the SPIFFE association identity: trust domain, principal pattern, or enabled authentication methods. During a rolling change, two replicas can therefore accept different SPIFFE principals for the same client ID while durable reconciliation considers them identical whenever scopes/audiences/resources match. Include association identity in the durable ownership contract and cover a same-policy/different-principal conflict. - Medium:
preflightDurableCollisionsonly reconciles desired IDs (spiffe_decorator.go:76-84) and has no removal path. Removing or renaming an association leaves its durable placeholder reserved indefinitely, so that client ID cannot later be registered. Reconcile stale placeholders owned by this auth-server configuration without deleting unrelated configured/DCR clients. - The prior documentation blockers remain: canonical JWT-bearer configuration does not disable legacy RFC 8693 when
token_exchangeis omitted (docs/arch/17-token-exchange-delegation.md:269-275);external_issueris carried across re-exchange despite lines 643-645 saying it is single-hop; and lines 259-265 say static configuration replaces a DCR collision althoughReconcileConfiguredClientrejects it.
All checks are green.
Signed-off-by: Jakub Hrozek <jakub@stacklok.com>
|
Pushed a fix for the durable placeholder fingerprint gap — it now includes the SPIFFE association identity (trust domain, principal pattern, authentication methods), not just OAuth policy, so two replicas can no longer durably reconcile associations that differ only in which principal is allowed to authenticate as a given client ID. Went through two rounds of adversarial review before landing: the first pass caught a real regression in an early version of the fix (an interface-embedding mistake that would have silently dropped the RFC 8707 resources enforcement we'd just added), which got fixed and independently re-verified. On the placeholder-removal finding (stale reservations never getting cleaned up when an association disappears or is renamed): that's the same gap tracked in #6477, which is a separate, more involved piece of work (lease-based reconciliation across replicas, not a small fix). I'd like to keep it out of this PR and track it there rather than block on it here — let me know if you'd rather see it addressed inline first. |
JAORMX
left a comment
There was a problem hiding this comment.
Re-review at d2d311d:
Resolved: durable SPIFFE fingerprints now include association identity (trust-domain reference, normalized principal, methods) as well as OAuth policy. External-issuer propagation is implemented and tested. The remaining blockers are:
- pkg/authserver/storage/spiffe_decorator.go:29-48,61-84: SPIFFE placeholders have no stale-association reconciliation/removal path. Removing or renaming an association leaves its durable client ID reserved indefinitely. Reconcile stale placeholders owned by this configuration without deleting configured/DCR clients.
- docs/arch/17-token-exchange-delegation.md:260-275,638-645: documentation still says canonical omission disables RFC 8693, that configured clients replace DCR clients, and that external_issuer is single-hop. Those claims disagree with NormalizeInboundGrants, storage collision behavior, and re-exchange propagation. Align the document and add/retain behavior coverage.
Exact-head CI is green.
Summary
The SPIFFE client-auth epic needs a place to configure SPIFFE association policy without inventing a parallel trust/grant path next to the existing delegate-client and trusted-issuer configuration. As more inbound grant families (RFC 8693 token exchange, RFC 7523 JWT-bearer, SPIFFE) accumulate, they need one canonical surface to configure and reason about instead of three independent ones, without breaking deployments that already rely on the legacy fields.
Stacked on #6467.
pkg/authserver/inbound_grants.gowithNormalizeInboundGrants, which reconciles a new canonicalRunConfig.InboundGrantssurface (per-familytoken_exchange/jwt_bearersub-configs whoseissuer_policiesreference atrusted_issuersentry by name) against the legacy top-leveldelegate_clientsand the RFC 8693/7523 fields embedded directly ontrusted_issuers.inbound_grantsentirely preserves released behavior.RunConfig.Validate, the embedded-auth-server runner, andbuildProvider/discovery — this is also wherespiffe_trust_domains(added in Define SPIFFE trust configuration #6467) finally gets validated and wired intoConfig.SPIFFETrust, closing the gap flagged in that PR's review.DisableTokenExchangecapability so RFC 8693 registration and discovery advertisement turn off together and can't drift out of sync.TrustedIssuer.Nameso canonicalissuer_policiescan reference an issuer without duplicating its fields.docs/arch/17-token-exchange-delegation.mdfor the newinbound_grantsshape and the now-conditional token-exchange discovery advertisement.jwt_bearerpaths reach a running server (the existing tests only covered normalization in isolation).Fixes #
Type of change
Test plan
task test)task test-e2e)task lint-fix)pkg/authserver/inbound_grants_test.gocovers normalization: legacy/canonical mutual exclusion per grant family, issuer-policy resolution by name (unknown/duplicate refs), and capability derivation.pkg/authserver/runner/embeddedauthserver_test.goadds coverage proving the canonical delegate-client, SPIFFE-client, andjwt_bearerpaths actually reach a running embedded auth server, not just the normalization step in isolation.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Special notes for reviewers
This closes the "
spiffe_trust_domainsis a silent no-op" gap flagged in review on #6467 —RunConfig.Validate()now callsValidateSPIFFETrustviaNormalizeInboundGrants, andConfig.SPIFFETrustis built and threaded into the embedded auth server inembeddedauthserver.go. Live SVID/bundle verification is still out of scope here; this is config normalization and wiring only.