Fix Cedar deny-all when upstream JWT lacks identity claims

[JAORMX]

Summary

Cedar authorization denied every request, for every user, on any workload whose
embedded auth server sits in front of an upstream IdP that issues JWT-shaped
access tokens without the identity claims policies reference — most commonly
email, which many OIDC providers assert only in the id_token.

The chain: when the embedded auth server is active, injectUpstreamProviderIfNeeded
force-injects the first upstream as Cedar's PrimaryUpstreamProvider (no opt-out),
so resolveClaims read the principal's claims from the upstream access token
only. The opaque-token fallback added in #5147 is gated on !looksLikeJWT, so a
well-formed upstream JWT that simply lacks email never fell back.
principal has claim_email was false for everyone, so a forbid ... unless domain
gate applied cleanly — no Cedar diagnostic, no warning logged, tools/list empty
and tools/call 403. Audit logging reads the AS-issued token, so it kept reporting
the correct user and pointed investigations away from authorization. Upstreams with
opaque access tokens (Google, GitHub) hit the #5147 fallback and worked, which made
the breakage look provider-specific.

The provider does assert the claim — in its id_token, which the auth server
already stores alongside that provider's access token.

• resolveClaims now supplements from the same provider's id_token: name
  and email fall back to it when the access token omits them. Access-token values
  always win. Provenance is preserved, so principal has claim_email keeps meaning
  "this upstream asserted an email".
• The ToolHive-issued token is not a claim source on this path. In a
  multi-upstream chain its mirrored name/email belong to the first configured
  upstream, which need not be the pinned provider — see "Special notes".
• sub is never supplemented: it becomes the Cedar principal entity ID, where
  Cedar has no has-style guard, so a missing access-token sub fails closed with
  ErrMissingPrincipal instead of having a principal chosen for it.
• Nothing is fabricated, so has-guarded policies still fail closed on claims
  neither token carries — with two rate-limited WARNs distinguishing "access token
  lacked profile claims, used the id_token" from "no id_token stored, will deny",
  so the condition is diagnosable from logs alone (requested in the issue's comment).

One fix covers the CLI runner, operator and vMCP paths, since all three funnel
through resolveClaims. The HTTP authorizer reads identity.Claims directly and
has no equivalent bug.

Fixes #5916

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)

Tests added:

• TestSupplementProfileClaims — precedence, and the never-filled sets: the
  id_token's registered claims (aud/exp/nonce/at_hash/azp),
  authorization-bearing claims, and sub. Plus the no-mutation/no-alias contract.
• TestResolveClaimsUpstreamSupplement — the claim-source contract end to end,
  including that the ToolHive-issued token's mirrored email never leaks in (the
  fixture uses a leaked-from-as-token@… sentinel), that an expired id_token
  is still used, and that an unparsable one degrades to no-supplement rather than
  failing the request.
• TestAuthorizeWithJWTClaims_UpstreamJWTMissingIdentityClaim — the reporter's
  exact policy pair against a JWT access token with no email.
• TestAuthorizeWithJWTClaims_PrincipalStaysUpstreamSourced — a forbid keyed on
  an upstream subject keeps matching, and a missing access-token sub fails closed
  even though the id_token carries one.
• TestIntegrationUpstreamJWTWithoutEmailClaim — the reported scenario through the
  full authorization middleware, plus regression guards for the Fall back to request-token claims for opaque upstream tokens #5147 opaque-token
  fallback and the tampered-JWT hard deny.

Both guarantees were verified by negative control, not just asserted: swapping the
supplement source back to the issued token fails 5 cases across the two packages
(including the integration reproducer), and adding sub back to the allowlist
fails the principal guard.

task test is otherwise green — the three failures in my run (pkg/api ×2,
pkg/secrets/keyring ×1) are pre-existing environment limitations in my sandbox
(no container runtime, no keyring), confirmed by stashing the change and re-running.
Also clean over repeated -race runs (see the captureSlogWarn note below).

Changes

File	Change
pkg/authz/authorizers/cedar/core.go	resolveClaims supplements profile claims from the provider's id_token via new supplementFromIDToken/supplementProfileClaims + profileClaimsFromIDToken; two dedicated rate limiters for the new diagnostics
pkg/authz/authorizers/cedar/core_test.go	New contract/behavioral tests; makes captureSlogWarn concurrency-safe
pkg/authz/integration_test.go	Middleware-level reproducer; pins the JWT-vs-opaque branch difference explicitly
docs/operator/virtualmcpserver-kubernetes-guide.md	Documents which token's claims a policy sees, both new WARNs, and that group/role/scope claims and the principal are access-token-only

Does this introduce a user-facing change?

Yes. Cedar policies referencing email or name now work when the upstream IdP
asserts them in its id_token rather than its access token, instead of denying
every request. Policies that already worked are unaffected: any claim the access
token asserts still wins, and group/role/scope claims plus the principal's sub
are never substituted. principal has claim_email continues to mean "the pinned
upstream asserted an email".

Special notes for reviewers

Why the supplement source is the id_token and not the ToolHive-issued token.
The first version of this PR supplemented from the issued token, on the premise
that its mirrored values are copies of the upstream's. Research for @jhrozek's
review found that premise fails in a configuration we document:

• validateChain (handler.go:346-353) requires an upstream chain to be "a
  non-empty, in-order, duplicate-free subsequence of the configured upstreams led
  by the required first upstream".
• Only the first leg resolves identity from its provider (userEmail = result.Email,
  callback.go:132-133); later legs carry those values forward
  (userEmail = pending.ResolvedUserEmail, callback.go:137). So the issued token's
  name/email always belong to the first configured upstream.
• primaryUpstreamProvider may name any upstream — the converter validates
  membership, not position, and the vMCP guide documents pinning explicitly.

With [okta, github] and primaryUpstreamProvider: github, the old version
supplemented okta's email into a claim set the policy believes is github's, silently.
verifyChainIdentity keeps it the same person, but a gate distinguishing a
corporate from a personal IdP is exactly what gets subverted. UpstreamIDTokens is
keyed by provider name and projected from the same credential bundle
(token.go:1265-1279), so provenance holds by construction.

No coverage lost, and no third tier. An OIDC upstream cannot log in without an
id_token (upstream/oidc.go:313 rejects it; openid is force-added at oidc.go:229),
and name/email are extracted from that id_token into the issued token
(oidc.go:330) — so the reporter's provider supplies the identical value with better
attribution. It also can't vanish mid-session: a refresh omitting a rotated id_token
carries the original forward (service.go:181-183). Only an OAuth 2.0 upstream never
asked for openid has none, and there the claim genuinely isn't available from that
provider, so absent-and-deny is honest — with a dedicated WARN. Dropping the issued
token entirely makes this less code than the version first reviewed.

exp is deliberately not enforced on the id_token. It's read as a record of
what the upstream asserted at login, not presented as a credential. The "callers
MUST check exp" contract exists for RFC 8693 subject-token use, and the owning
service says so: "Enforcement belongs at the RFC 8693 token-exchange consumer"
(upstreamtoken/service.go:134); xaa.go, the only current consumer, doesn't check
it either. Enforcing it here would be harmful — RefreshTokenLifespan defaults to
7 days (config.go:999) while id_tokens expire in minutes, so a policy would permit
early in a session and silently deny later. Pinned by
expired_id_token_is_still_used.

Unrelated latent race fixed here out of necessity. captureSlogWarn installs a
capturing handler through the process-global slog.SetDefault over a plain
bytes.Buffer, so any parallel test can race the read — keeping the capturing test
non-parallel never prevented that. Adding a second slog.Warn on a hot path made it
reproducible under -race (it does not reproduce on main). The buffer is now
mutex-guarded, and TestStaleTHVGroupWarning keys on its specific message rather
than on the buffer being empty, since foreign records can legitimately land there.

Known inconsistency left for a follow-up, deliberately. The #5147 opaque-token
branch still falls back to the issued token's claims, so it retains the provenance
flaw fixed here — and its principal is ToolHive's internal user ID rather than the
upstream subject, so a rule keyed on Client::"okta|alice" works with JWT access
tokens and silently doesn't with opaque ones. Unifying that branch on the id_token
would fix both but changes behaviour on a shipped path (Google/GitHub deployments),
so it wants its own PR; the current behaviour is pinned with a comment in
opaque_upstream_token_still_falls_back. Likewise: taking groups/roles from the
id_token is provenance-legitimate now that the source is the same provider, but it
would newly grant requests that deny today.

Also out of scope: honoring primaryUpstreamProvider on
MCPServer/MCPRemoteProxy with an explicit opt-out (parity with #5199 for
VirtualMCPServer), and the missing-upstream-token hard deny the issue flags
parenthetically, left fail-closed.

Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.21%. Comparing base (8a860d9) to head (c943195).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6022      +/-   ##
==========================================
+ Coverage   72.18%   72.21%   +0.02%     
==========================================
  Files         721      721              
  Lines       75025    75069      +44     
==========================================
+ Hits        54158    54211      +53     
+ Misses      17005    16979      -26     
- Partials     3862     3879      +17     

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

[jhrozek]

Went through this fairly carefully since it's an authz trust-boundary change. The test suite is genuinely good — authorization_bearing_claims_are_never_filled and as_token_protocol_claims_are_never_filled enumerate exactly the claims that would be dangerous to merge, and the integration test is a real #5916 regression test (I traced it against the pre-fix resolveClaims and it does fail on old code). Also confirmed the signature-verification premise holds: UpstreamTokens is only ever populated inside the validated-token middleware from server-side state (token.go:1277), never client-supplied.

Two things I'd like to sort out before this goes in, both on the same design decision:

The sub entry in mirroredIdentityClaims doesn't hold up. The comment justifies the allowlist on the grounds that the AS-issued values are copies of the upstream ones, but the AS sub is a random UUID from UserResolver, not the upstream subject — and unlike name/email, sub becomes the Cedar principal entity ID. Details inline; the failure mode I care about is a forbid keyed on an upstream sub quietly stopping matching.

And separately: was identity.UpstreamIDTokens considered? The upstream id_token is already on the Identity, and sourcing the supplement from it rather than the AS token would keep provenance intact and make the first point moot.

Rest is minor — a few comment/doc things and one small test gap.

[jhrozek]

Re-read the whole thing against my earlier comments. All eight are addressed, and the id_token pivot turned out better than what I was asking for — I only suggested it as a provenance improvement, and the multi-upstream misattribution you found (primaryUpstreamProvider: github with [okta, github] supplementing okta's email into a claim set the policy reads as github's) makes it a correctness fix, not an aesthetic one. Traced that myself: validateChain does pin chain[0] to upstreams[0], only the first leg calls the provider for identity, and the converter validates membership rather than position. So the config is reachable.

Also confirmed the three factual claims the new version rests on, since they're what makes "no third tier" hold rather than being a coverage regression: openid is force-added at oidc.go:230 and a login with no ID token is rejected at oidc.go:312, so an OIDC upstream can't reach this code without one; and upstreamtoken/service.go:135-137 really does say enforcement belongs at the RFC 8693 consumer.

Agreed on not enforcing exp — the permit-early-deny-later flip is worse than either alternative, and the claims are no staler than the mirrored ones you removed. expired_id_token_is_still_used is the right place to pin that.

Rest of it: sub is out and the principal test's third case is the one that matters; the rate limiters are split; the fixtures no longer encode AS-sub == upstream-sub; name now conflicts in both directions. Good call on skipping the set-equality test.

Three non-blocking things, all new to this version rather than leftovers:

The session.NameClaimKey/EmailClaimKey coupling was my suggestion, but it fits less well now that the read target changed. Those constants are the claim names the auth server uses when mirroring into the token it issues; what this code reads is the upstream provider's id_token, where name/email are OIDC Core §5.1 wire names that happen to coincide. So the rename-safety runs against the wrong side: someone changing session.NameClaimKey's value would silently change which claim Cedar reads out of a third party's id_token. Concretely it also means pkg/authz/authorizers/cedar now pulls ory/fosite into its dependency tree (13 packages by go list -deps) to spell two string literals. Two consts local to this file, or in the upstream OIDC package if it has them, would be a better anchor. Your call — the coupling is harmless in practice since these two names aren't going to be renamed.

identity.go:132-133 still says callers MUST validate exp before use, and this is now a consumer that deliberately doesn't. The reasoning is right and it's in the resolveClaims comment, but the two read as contradictory to whoever finds the field first. A clause on the field pointing at the two consumers and their different obligations would settle it.

Comment density is still high — 140 of the 222 added lines in core.go are comment or blank. The duplication complaint is fully resolved, and the widening-hazard bullets are load-bearing, so this isn't the same objection. Just noting the resolveClaims doc comment is now around 60 lines and might read better with the multi-upstream and exp paragraphs trimmed to their conclusions, since the full arguments are in the commit message and this thread.

Haven't run the suite myself; taking your reported results and the negative-control checks at face value.