Normalize canonical inbound grants

[jhrozek]

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.

• Adds pkg/authserver/inbound_grants.go with NormalizeInboundGrants, which reconciles a new canonical RunConfig.InboundGrants surface (per-family token_exchange/jwt_bearer sub-configs whose issuer_policies reference a trusted_issuers entry by name) against the legacy top-level delegate_clients and the RFC 8693/7523 fields embedded directly on trusted_issuers.
• Legacy and canonical configuration for the same grant family are mutually exclusive and rejected at validation time; the two families are otherwise independent, and omitting inbound_grants entirely preserves released behavior.
• Threads the normalized result through RunConfig.Validate, the embedded-auth-server runner, and buildProvider/discovery — this is also where spiffe_trust_domains (added in Define SPIFFE trust configuration #6467) finally gets validated and wired into Config.SPIFFETrust, closing the gap flagged in that PR's review.
• Adds a DisableTokenExchange capability so RFC 8693 registration and discovery advertisement turn off together and can't drift out of sync.
• Adds TrustedIssuer.Name so canonical issuer_policies can reference an issuer without duplicating its fields.
• Updates docs/arch/17-token-exchange-delegation.md for the new inbound_grants shape and the now-conditional token-exchange discovery advertisement.
• Adds a runner-level test proving the canonical delegate-client, SPIFFE-client, and jwt_bearer paths reach a running server (the existing tests only covered normalization in isolation).

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)

pkg/authserver/inbound_grants_test.go covers normalization: legacy/canonical mutual exclusion per grant family, issuer-policy resolution by name (unknown/duplicate refs), and capability derivation. pkg/authserver/runner/embeddedauthserver_test.go adds coverage proving the canonical delegate-client, SPIFFE-client, and jwt_bearer paths actually reach a running embedded auth server, not just the normalization step in isolation.

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

This closes the "spiffe_trust_domains is a silent no-op" gap flagged in review on #6467 — RunConfig.Validate() now calls ValidateSPIFFETrust via NormalizeInboundGrants, and Config.SPIFFETrust is built and threaded into the embedded auth server in embeddedauthserver.go. Live SVID/bundle verification is still out of scope here; this is config normalization and wiring only.

[codecov]

Codecov Report

❌ Patch coverage is 96.03960% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.23%. Comparing base (031675e) to head (61c427c).

Files with missing lines	Patch %	Lines
pkg/authserver/runner/embeddedauthserver.go	79.16%	5 Missing ⚠️
pkg/authserver/server_impl.go	85.71%	2 Missing ⚠️
pkg/authserver/server/tokenexchange/handler.go	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6473      +/-   ##
==========================================
+ Coverage   78.19%   78.23%   +0.03%     
==========================================
  Files         769      770       +1     
  Lines       75078    75232     +154     
==========================================
+ Hits        58707    58856     +149     
- Misses      16366    16371       +5     
  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 #6467 and #6200. CI is green, but I found two blocking model/correctness issues:

1. SPIFFEClients is nested under inbound_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 for spiffeClientAuth under inboundGrants with grants and token-exchange permission configured per association. Authentication method and grant authorization need to remain independent in the canonical schema.

2. SPIFFE validation reports paths under inbound_grants.spiffe_client_auth[...], but this PR's actual serialized path is inbound_grants.token_exchange.spiffe_clients[...] (pkg/authserver/spiffe_trust.go, reached from RunConfig.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.

[JAORMX]

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_grants can disable token exchange, and this PR correctly removes the Fosite handler and discovery advertisement via TokenExchangeEnabled. However, /oauth/register still calls ValidateDCRRequest without that capability (pkg/authserver/server/handlers/dcr.go:243-257). It can therefore return 201 Created for a token-exchange-only client even though the server has no token-exchange handler. This is especially direct for private_key_jwt, whose token-only path explicitly accepts a token-exchange registration. Please thread TokenExchangeEnabled into DCR validation (or reject allow_private_key_jwt_registration when 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.

[jhrozek]

Pushed a commit that resolves both blocking issues — thanks for catching these.

1. SPIFFE subordinate to token exchange: SPIFFEClientAuth is no longer nested under inbound_grants.token_exchange. Following Define SPIFFE trust configuration #6467's schema fix, InboundGrantsRunConfig now has SPIFFEClientAuth as a sibling of TokenExchange/JWTBearer, and NormalizeInboundGrants no longer touches SPIFFE at all — ValidateSPIFFETrust/NewSPIFFETrustConfig read cfg.InboundGrants directly in RunConfig.Validate()/embeddedauthserver.go, independent of the legacy/canonical projection this file does. Disabling or omitting token_exchange no longer drops SPIFFE associations.

   This surfaced a real bug while fixing it: since SPIFFE client auth is now independent of NormalizedInboundGrants.Capabilities.TokenExchange, a config with only SPIFFE associations (no inbound_grants.token_exchange) was computing DisableTokenExchange: true and silently disabling the token-exchange grant server-wide — even though SPIFFE clients exclusively use that grant. Fixed by having prepareInboundGrantConfiguration OR in whether any SPIFFE client-auth association is configured. Caught by re-running the existing TestEmbeddedAuthServer_SPIFFEAssociationDoesNotAuthenticateClient test, which started failing with invalid_request instead of the expected invalid_client once the schema was decoupled.

2. Validation paths didn't match the schema: fixed as part of Define SPIFFE trust configuration #6467 — error paths already say inbound_grants.spiffe_client_auth[...], matching the real field now that it's flat.

On the normalization-direction note (projecting canonical types back into the legacy TrustedIssuer representation): left as-is for now — agreed it's worth revisiting, but a larger refactor of the runtime model felt like more risk than this PR should carry given the SPIFFE-specific fixes above. Happy to open a follow-up if you'd like it tracked separately.

Signed-off-by trailer fixed. CI is green on the amended commit.

[JAORMX]

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.

[jhrozek]

Good catch, and you're right on a closer look — an issuer alone doesn't identify an actor, full stop. I checked how our audit code actually consumes this chain (pkg/audit's delegation chain extraction treats every hop as an acting party), and nesting a bare {"iss": ...} there really was presenting a non-actor as one.

Fixed in 9382f6e: the external issuer is now recorded as its own top-level external_issuer claim, set whenever the subject token came from a trusted external issuer, regardless of whether an actor was also resolved. Nesting under act.act now only happens when there's a genuine actor to nest. Cedar picks up external_issuer the same way it picks up act (generic claim_ prefixing, no special-casing needed).

Also added a test that pins the depth accounting for the no-actor case, since removing that phantom nesting level changes how deep a prior delegation chain can go before hitting the max-depth cap.

[JAORMX]

Fresh review of 64484b91 found one remaining issue; the prior act and Cedar-documentation blockers are resolved, and current CI is green.

Medium — JWKS cache lifecycle leak: newExternalIssuerConfig creates a jwk.Cache with context.Background() but exposes no shutdown path (pkg/authserver/server/tokenexchange/multi_issuer_validator.go:547-557). jwk.Cache starts background workers, so constructing and tearing down auth servers in-process leaks goroutines. Please give the validator/owning server a close lifecycle that calls jwksCache.Shutdown and root the cache lifetime in that owner rather than a global context.

I found no material regression in the focused act/Cedar fixes.

[JAORMX]

Status correction: I verified the JWKS cache shutdown leak is real but predates this PR and is already on main; #6473 does not modify or worsen the cache construction. It is not a #6473 regression/blocker. The previous act and Cedar-documentation findings remain resolved on 64484b91.

[JAORMX]

Fresh full re-review of the current head found one release blocker, so I am not approving.

High — re-exchanging an externally sourced delegated token launders external_issuer provenance. The first exchange records external provenance, but a subsequent exchange follows the self-issued validation path and only preserves the prior act chain. It does not carry external_issuer forward (pkg/authserver/server/tokenexchange/handler.go:208-220, 503-563). This is especially visible for may_act and ActorMatcher-only authorization, where the external source is intentionally absent from act: one permitted re-exchange becomes indistinguishable from a native/self-issued token to Cedar or downstream policy using context.claim_external_issuer.

Please make authenticated external provenance transitive across re-exchanges (or define an equivalent non-removable provenance structure) and add regression coverage for external token → delegated token → re-exchanged token on both authorization paths. CI is green and the prior actor-shape/Cedar-doc findings are resolved.

[jhrozek]

Pushed a fix for the re-exchange laundering finding.

You're right, and I confirmed the exact mechanism: a re-exchanged token routes through the self-issued validator (its own iss is now this server's), which had no path reading back a prior external_issuer claim -- only the act chain got carried forward. Fixed the same way act itself is already carried forward: buildActClaim now reads external_issuer out of the subject token's own Extra map when this exchange didn't establish a fresh one of its own, falling back to it rather than dropping it. A fresh external issuer resolved this hop always takes precedence over a prior one, since the two states are mutually exclusive under the current handler structure (one validated subject token per exchange) -- an external token can never also be presenting a prior ToolHive-recorded external hop.

Added coverage for both authorization paths you called out: a may_act-bearing external token and an ActorMatcher-only external token, each carried through a first exchange then a second self-issued re-exchange, asserting external_issuer survives.

CI is green.

[JAORMX]

Focused recheck of 3537835c confirms the external-provenance blocker is resolved: self-issued re-exchange carries the verified string external_issuer forward, and the may_act and ActorMatcher paths are covered. The tests reconstruct the second-hop claims rather than exchange the first issued JWT, so an issuance/serialization end-to-end regression test would still strengthen this, but I found no functional provenance regression.

CI is not green: the current Go Vulnerability Check failed for GO-2026-6354 and GO-2026-6355. Please triage that failure before approval.

[JAORMX]

Re-review complete at 3537835c7ad6db13207552f0a08681629ed82626.

The DCR/token-exchange capability mismatch is fixed and covered for explicit and implicit private_key_jwt token-exchange registrations. The architecture documentation now uses inbound_grants.spiffe_client_auth and accurately describes family-wide legacy/canonical conflicts. No new release-blocking 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.

[JAORMX]

Fresh review found one additional material capability mismatch. When allow_private_key_jwt_registration is true but canonical configuration disables token exchange, discovery still advertises private_key_jwt (and the token-only registration path) (pkg/authserver/server/handlers/discovery.go:131-153), while DCR correctly rejects every private-key-JWT registration because that flow is token-exchange-only (pkg/authserver/server/handlers/dcr.go:252-257). A JWT-bearer-only deployment therefore advertises an unusable registration capability. Please gate the discovery advertisement on both capabilities, or reject the contradictory startup configuration. I cannot approve while this and the failing vulnerability check remain.

[JAORMX]

CI update: E2E Test Lifecycle (kindest/node:v1.33.7) is also failing on the current head. The VirtualMCP suite timed out after the cross-pod Redis session-restore spec failed; subsequent specs were interrupted. This is outside the inbound-grant change, but it needs retry or triage before approval. The Go Vulnerability Check remains failing as previously noted.

[jhrozek]

Pushed a fix for the discovery/DCR capability mismatch.

Traced it further and it's actually simpler than "gate discovery too": a private_key_jwt registration can never request any grant except token-exchange (validateGrantTypes/validateResponseTypes in pkg/authserver/server/registration/dcr.go force this unconditionally), so there's no legitimate use of allow_private_key_jwt_registration with token exchange disabled at all -- every such registration would fail 100% of the time, not just in some narrower case. Rather than gate three separate discovery call sites (and leave the door open for a fourth to reintroduce the same gap), I added a startup-time rejection of the combination in Config.Validate(), matching the existing pattern for other capability conflicts in that file.

CI is green.

[JAORMX]

Focused recheck of 8f31427d confirms both latest functional fixes: external_issuer is carried through the self-issued re-exchange path (pkg/authserver/server/tokenexchange/handler.go:208-219, 577-580), and the contradictory private_key_jwt/token-exchange-disabled configuration now fails at startup (pkg/authserver/config.go:1095, 1206-1222). No new focused regression found.

I cannot approve yet: Go Vulnerability Check still fails on GO-2026-6354 and GO-2026-6355; the lifecycle E2E jobs, including v1.33.7, are still in progress. All review threads are resolved.

[JAORMX]

CI update for 61c427c5: Helm chart lint/test failed before project tests because arduino/setup-task timed out requesting go-task/task tags. This appears to be external/transient, but the current checks are not green; Docs, Go tests, and lifecycle E2E are still in progress. The external_issuer re-exchange fix is otherwise present and covered by the focused tests.

[JAORMX]

CI update for 61c427c5: all project checks are green, including the lifecycle E2E jobs. The only non-green check is Helm chart lint/test, which failed before running project tests because arduino/setup-task timed out requesting go-task/task tags (Request timeout: /repos/go-task/task/git/refs/tags). This looks external/transient, but CI is not green, so I cannot approve until it is retried or otherwise triaged.

[JAORMX]

Re-review of the updated head 61c427c53f3dec880165a73d11b386714f0e49a1: approval remains valid. The follow-up resolves the remaining private_key_jwt configuration-validation inconsistency; no new correctness or security regressions found.

CI is otherwise green. Helm Charts / Lint and Test Helm Charts failed during the external Install Task setup before project chart tests ran, so please rerun or triage that job before merging. No local tests were run.