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 95.81152% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.21%. Comparing base (7888933) to head (64484b9).

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	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6473      +/-   ##
==========================================
+ Coverage   78.17%   78.21%   +0.03%     
==========================================
  Files         769      770       +1     
  Lines       74984    75127     +143     
==========================================
+ Hits        58621    58758     +137     
- Misses      16358    16364       +6     
  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]

Pushed a commit addressing both findings.

DCR/token-exchange capability gating. /oauth/register now rejects a registration whose effective grant types include token-exchange when it's disabled server-wide, instead of accepting the client and only failing later, confusingly, at /oauth/token. The check runs on the post-defaulting grant types validateGrantTypes already computes (so it catches both an explicit grant_types field and the implicit private_key_jwt default), using the same h.config.TokenExchangeEnabled runtime-capability pattern the existing scope validation already uses. Added a regression test covering the explicit-grant, implicit-default, and enabled-success cases.

Docs drift. Fixed the stale inbound_grants.token_exchange.spiffe_clients field path (now inbound_grants.spiffe_client_auth) and corrected the "mutually exclusive per issuer" wording — the actual conflict check in NormalizeInboundGrants is family-wide across all issuers, not per-issuer.

Also worth flagging: rebasing onto #6467's fix (the hard-reject on non-empty spiffe_trust_domains, per your review there) surfaced that the SPIFFE-forces-token-exchange-capability regression fix from earlier in this PR's development had been silently lost during a rebase — NormalizeInboundGrants no longer set Capabilities.TokenExchange = true for a SPIFFE-only config. Caught it via a test failure while adapting to the hard-reject, re-applied it (now living directly in NormalizeInboundGrants instead of a separate helper), and added a same-package regression test this time specifically so a future rebase can't silently drop it again without a local test catching it.

CI is green.

[JAORMX]

Re-review complete. The prior functional blocker is fixed: effective token-exchange enablement is consistently applied to provider registration, discovery, and DCR validation, including the defaulted-grant path. The SPIFFE placement and DCO issues are also resolved; current CI is green.

Two remaining findings:

1. Medium: buildActClaim emits an RFC 8693-invalid nested act object for external may_act tokens: the nested actor has only iss and no required sub (pkg/authserver/server/tokenexchange/handler.go:316-325). Please preserve issuer provenance outside act, or supply a valid actor subject.
2. Low: docs/arch/17-token-exchange-delegation.md:82-85 still documents SPIFFE clients beneath inbound_grants.token_exchange, contradicting both the implemented sibling inbound_grants.spiffe_client_auth path and the same document at lines 126-129. Following the earlier path can silently omit the association during decoding. Please align the early overview.

[JAORMX]

Rechecked at db12a8a. The two findings remain; the latest head does not address them.

• Medium: External may_act exchanges still build an issuer-only nested act object (pkg/authserver/server/tokenexchange/handler.go:477-485; asserted in handler_test.go:1828-1850). RFC 8693 models each act object as an acting subject, so this is a semantic/interoperability defect in the delegation chain (clarifying my earlier wording: not a capitalized RFC MUST).
• Low: The early overview still says SPIFFE clients live in inbound_grants.token_exchange (docs/arch/17-token-exchange-delegation.md:81-83), contradicting the real sibling inbound_grants.spiffe_client_auth schema and lines 126-129 of the same document.

CI is green.

[jhrozek]

Thanks for catching the docs drift — fixed the remaining stale inbound_grants.token_exchange reference in the early overview section (lines 82-85), which I'd missed when I fixed the other two spots earlier.

On the buildActClaim finding: I looked into it and I don't think it's correct, so I want to push back rather than implement it silently.

RFC 8693 §4.1 doesn't actually require sub on the act object. The only normative language in that section governs (1) which claim types must be excluded from act (exp, nbf, aud — "are therefore not used") and (2) how a consumer must process nested act claims ("MUST only consider the token's top-level claims and the party identified as the current actor"). The iss+sub combination is introduced only as an example — "the combination of the two claims iss and sub might be necessary to uniquely identify an actor" — not a requirement. buildActClaim's own pre-existing comment already quotes this correctly, three lines above where I'd have added the "requires sub" claim.

Beyond not being required, actually adding a fallback sub there would have made things worse: on the may_act-bearing and ActorMatcher-only paths, there's no actor to report at that nesting level — the external subject never acted, they're the party being acted for. Backfilling their own subject into that slot would misrepresent them as a prior actor in the delegation chain, and it reaches real consumers: pkg/audit/auditor.go's extractDelegationChainFromIdentity parses this exact claim into AuditEvent.DelegationChain, documented as "the full chain of acting parties." It would also reintroduce the external subject's bare, unqualified value into the token — delegatedSubject's own doc comment explains at length why that's specifically avoided (the qualified issuer#subject form exists because Cedar's claim extraction reads sub alone). And it would contradict chainToAct fifty lines below in the same file, which deliberately allows an iss-only hop when re-nesting a prior delegation chain.

I added a direct unit test for buildActClaim (TestBuildActClaim_NestedActOnlyCarriesSubWhenAnActorWasResolved) that locks in the actual correct invariant instead — sub present when an actor was resolved, absent otherwise — so this doesn't need re-litigating from scratch if it comes up again. Happy to be shown a part of the RFC or a downstream consumer I'm missing if you still think this is wrong.

CI is green.

[JAORMX]

Thanks for fixing the stale SPIFFE-path documentation; that finding is resolved.

I rechecked the act rationale and am leaving the medium finding in place. An act object represents an acting party, and the issuer-only nested hop still identifies none (pkg/authserver/server/tokenexchange/handler.go:477-485); the new test enshrines the problematic shape rather than demonstrating its correctness. The external issuer did not act, so it should not be added to the delegation chain as a prior act hop.

One additional docs correction: the updated section says Cedar authorizers do not read act (docs/arch/17-token-exchange-delegation.md:571-574), but ToolHive Cedar policies consume context.claim_act.sub (pkg/authz/authorizers/cedar/core_test.go:422-445, :448-471). Please correct that claim; it is material for operators authoring delegation policy.

[JAORMX]

Re-reviewed at 9382f6e0. Both prior findings are resolved: external may_act/matcher-only exchanges no longer produce an issuer-only nested act object, and the delegation documentation now accurately states that Cedar exposes act and external_issuer through generic claim_ context attributes. No focused regression found.

CI is not yet green: E2E Test Lifecycle (kindest/node:v1.35.1) failed at Run VirtualMCP Lifecycle E2E tests; the other completed checks are green. This is outside the reviewed token-exchange path, but needs a retry or triage before approval.

[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.