Fix contradictory upstream ID token exp contract

docs/operator/virtualmcpserver-kubernetes-guide.md

@@ -664,6 +664,12 @@ WARN upstream access token lacks profile claims; using the upstream ID token's
      values for Cedar evaluation provider=okta claims="[email]"
 ```
 
+That fallback uses the stored `id_token` even after it has expired, by design. It
+is read as a record of what the upstream asserted when the user logged in, not
+presented to anyone as a credential. Since a session can outlive an `id_token` by
+days, expiring it out of policy evaluation would let the same user be permitted
+early in a session and denied later, with no configuration change.
+
 The token the client presented — the one ToolHive's auth server issued — is
 never a claim source here, even though it mirrors a `name` and `email`. In a
 multi-upstream chain those mirrored values come from the **first** configured

pkg/auth/identity.go

@@ -121,16 +121,37 @@ type Identity struct {
 	// is active and the JWT contains a token session ID (tsid claim).
 	// Each value is the rotated ID token when a refresh produced one
 	// (OIDC Core 1.0 §12.2), otherwise the original JWT captured at the initial
-	// OIDC login; it is not independently validated for freshness.
+	// OIDC login.
 	//
 	// State semantics:
 	//   - nil            — no tsid claim was present on the incoming JWT
 	//                      (middleware did not attempt to load credentials).
 	//   - empty map      — tsid claim was valid but no providers had an
 	//                      ID token stored for the session.
 	//   - populated map  — keys are upstream provider names; values are the
-	//                      stored ID token JWTs (may be expired; callers MUST
-	//                      validate the `exp` claim before use).
+	//                      stored ID token JWTs, which may be expired.
+	//
+	// Whether `exp` matters depends on what the caller does with the token. Nothing
+	// in this package enforces it; the two current consumers use the token
+	// differently and neither validates it locally:
+	//
+	//   - PRESENTING it as a credential — e.g. as the subject_token of an RFC 8693
+	//     exchange (pkg/vmcp/auth/strategies) — is subject to expiry, because the
+	//     party receiving it will reject an expired assertion. That consumer
+	//     deliberately does not pre-check `exp` either: it lets the IdP reject the
+	//     exchange and surfaces the resulting `invalid_grant`. So such a caller must
+	//     either check `exp` first or be prepared to handle that failure.
+	//   - READING identity claims out of it — as the Cedar authorizer does for the
+	//     profile claims an upstream access token omits — is not subject to expiry at
+	//     all. The token is evidence that the upstream asserted something at login,
+	//     and expiry says nothing about whether that assertion happened. Rejecting it
+	//     once expired would flip an authorization decision mid-session, since
+	//     sessions outlive ID tokens by days; and the claims are no staler than the
+	//     alternative, since the profile claims mirrored into the ToolHive-issued
+	//     token are likewise captured once at login and never refreshed.
+	//
+	// Either way the token is NOT re-validated for signature or freshness here, so
+	// treat its claims as login-time facts rather than current state.
 	//
 	// Redacted in MarshalJSON() to prevent token leakage.
 	// MUST NOT be mutated after the Identity is placed in the request context.

pkg/auth/upstreamtoken/service.go

@@ -82,7 +82,9 @@ func (s *InProcessService) GetValidTokens(ctx context.Context, sessionID, provid
 // the rotated ID token when a refresh produced one (OIDC Core 1.0 §12.2),
 // otherwise the original JWT captured at the initial OIDC login; it is not
 // independently validated for freshness and may be empty if the upstream login
-// never yielded one. Callers MUST check its exp claim before use.
+// never yielded one. Callers that PRESENT it as a credential must expect expiry
+// to be rejected; callers that only READ ITS CLAIMS are unaffected by expiry. See
+// Identity.UpstreamIDTokens for both consumers.
 //
 // Returns an empty map and nil failed slice (not error) for unknown sessions.
 func (s *InProcessService) GetAllUpstreamCredentials(
@@ -131,9 +133,15 @@ func (s *InProcessService) GetAllUpstreamCredentials(
 		result[providerName] = *refreshed
 	}
 
-	// TODO(auth): the "check exp" contract on UpstreamCredential.IDToken is
-	// documented but not enforced here. Enforcement belongs at the RFC 8693
-	// token-exchange consumer when it receives the credential.
+	// The returned ID tokens are deliberately not checked for expiry here. Neither
+	// of today's two consumers wants that pre-check: one presents the token as an
+	// RFC 8693 subject_token and relies on the IdP to reject an expired assertion
+	// (pkg/vmcp/auth/strategies surfaces the resulting invalid_grant), and the other
+	// only reads its claims, for which expiry is irrelevant. Enforcing it here would
+	// break the second while duplicating what the IdP already does for the first.
+	// A future consumer that presents the token to a sink which does NOT validate
+	// `exp` would change that calculus — such a caller must check it itself.
+	// See Identity.UpstreamIDTokens for both.
 	return result, failed, nil
 }
 

pkg/auth/upstreamtoken/types.go

@@ -23,13 +23,21 @@ const TokenSessionIDClaimKey = "tsid"
 // the original JWT captured at the initial OIDC login (OIDC Core 1.0 §3.1.3.7).
 // It is not independently validated for freshness.
 //
-// Callers MUST check its `exp` claim before using it (e.g. as the subject_token
-// of an RFC 8693 token exchange), as it may be expired. Note also that the ID
+// Callers that PRESENT it as a credential — e.g. as the subject_token of an
+// RFC 8693 token exchange — are subject to expiry and must either check its `exp`
+// claim first or handle the resulting rejection. Today's such consumer
+// (pkg/vmcp/auth/strategies) deliberately takes the second option: it lets the IdP
+// reject the exchange and surfaces the `invalid_grant`. Note also that the ID
 // token's `aud` is this auth server's client registration with the issuing
 // upstream, not the token-exchange endpoint — whether a target authorization
 // server accepts it as a subject token is governed by that server's policy and
 // is typically limited to the same issuer/audience.
 //
+// Callers that only READ ITS CLAIMS are a different case and deliberately do not
+// check `exp`: the token is then evidence of what the upstream asserted at login,
+// and expiry says nothing about whether that assertion happened. See
+// Identity.UpstreamIDTokens for the two consumers and why they differ.
+//
 // IDToken may be empty when the upstream login did not return an id_token
 // (e.g. the provider was not asked for the openid scope). An empty IDToken
 // is a legitimate state, not an error. Note that Identity.UpstreamTokens and
@@ -57,9 +65,10 @@ type TokenReader interface {
 	//
 	// Each returned IDToken is the rotated ID token when a refresh produced
 	// one (OIDC Core 1.0 §12.2), otherwise the original JWT captured at OIDC
-	// login (OIDC Core 1.0 §3.1.3.7). Callers MUST check each ID token's `exp`
-	// claim before using it for e.g. RFC 8693 subject-token exchange, as it may
-	// be expired.
+	// login (OIDC Core 1.0 §3.1.3.7), and it may be expired. Callers that PRESENT
+	// it as a credential (e.g. RFC 8693 subject-token exchange) must check `exp`
+	// first or handle the resulting rejection; callers that only READ ITS CLAIMS
+	// are unaffected by expiry. See UpstreamCredential for both consumers.
 	//
 	// Returns an empty map and nil failed slice (not error) for unknown sessions.
 	GetAllUpstreamCredentials(ctx context.Context, sessionID string) (
@@ -78,8 +87,10 @@ type Service interface {
 	//   - ErrNoRefreshToken if the access token is expired and no refresh token is available
 	//   - ErrRefreshFailed if the refresh attempt fails (e.g., revoked refresh token)
 	//
-	// The returned UpstreamCredential.IDToken may be empty or expired; callers
-	// MUST check its `exp` claim before using it (e.g. as the subject_token of
-	// an RFC 8693 token exchange). See UpstreamCredential for details.
+	// The returned UpstreamCredential.IDToken may be empty or expired. Callers that
+	// PRESENT it as a credential (e.g. as the subject_token of an RFC 8693 token
+	// exchange) must check its `exp` claim first or handle the resulting rejection;
+	// callers that only READ ITS CLAIMS are unaffected by expiry. See
+	// UpstreamCredential for details.
 	GetValidTokens(ctx context.Context, sessionID, providerName string) (*UpstreamCredential, error)
 }

pkg/authz/authorizers/cedar/core.go

@@ -19,7 +19,6 @@ import (
 	"github.com/golang-jwt/jwt/v5"
 
 	"github.com/stacklok/toolhive/pkg/auth"
-	"github.com/stacklok/toolhive/pkg/authserver/server/session"
 	"github.com/stacklok/toolhive/pkg/authz/authorizers"
 	"github.com/stacklok/toolhive/pkg/syncutil"
 )
@@ -563,25 +562,26 @@ func (a *Authorizer) IsAuthorized(
 // source is that provider's upstream credentials — and only that provider's. Its
 // access token is primary; the profile claims listed in profileClaimsFromIDToken
 // fall back to the same provider's id_token when the access token does not carry
-// them. The access token alone was a deny-all trap, because many OIDC providers
-// put identity claims such as `email` in the id_token only and omit them from the
-// access token, so every policy referencing `claim_email` silently denied every
-// request (#5916).
+// them, because many OIDC providers assert `email` only in the id_token and the
+// access token alone was therefore a deny-all trap (#5916).
 //
-// Both tokens come from the same upstream login for the same provider name
-// (projected side by side from one credential bundle in TokenValidator.Middleware),
-// so a supplemented claim carries the same provenance as one read directly from
-// the access token: `principal has claim_email` means "this upstream asserted an
-// email", never "some other trusted party did". That is why the ToolHive-issued
-// token the client presented is NOT a claim source on this path, even though it
-// mirrors the upstream name/email: in a multi-upstream chain those mirrored values
-// come from the FIRST configured upstream (the identity provider — see
-// handlers/callback.go, and validateChain, which requires chain[0] to be
-// upstreams[0]), which need not be the pinned provider. Using them would
-// attribute the identity provider's email to a different IdP.
+// Both tokens belong to the same upstream provider and session: the auth
+// middleware splits a single credential bundle into Identity.UpstreamTokens and
+// Identity.UpstreamIDTokens (see TokenValidator.Middleware in pkg/auth/token.go),
+// so a given key denotes the same provider and session in both maps. That is what
+// makes a supplemented claim carry the same provenance as one read from the access
+// token: `principal has claim_email` means "this upstream asserted an email". (The
+// two maps' key SETS may differ — a provider can have an access token but no
+// id_token, which is why the fallback is conditional. That is not a case of one key
+// meaning two different identities.)
 //
-// The supplement is deliberately restricted to profileClaimsFromIDToken rather
-// than being a general merge of the two token bodies. Read this before widening it:
+// The ToolHive-issued token the client presented is deliberately NOT a claim
+// source here, even though it mirrors a name/email — in a multi-upstream chain
+// those belong to the first configured upstream, which need not be the pinned
+// provider, so using them could attribute one IdP's email to another.
+//
+// The supplement is restricted to profileClaimsFromIDToken rather than merging
+// the two token bodies. Read this before widening it:
 //
 //   - An id_token's registered claims (`iss`, `aud`, `exp`, `nonce`, `at_hash`,
 //     `azp`) describe that token, not the user. Merging them would overwrite the
@@ -600,16 +600,10 @@ func (a *Authorizer) IsAuthorized(
 // never be shadowed. A claim absent from both tokens stays absent, so `has`-guarded
 // policies still fail closed.
 //
-// The stored id_token is used without checking `exp`, deliberately. It is read
-// here as a record of what the upstream asserted at login, not presented as a
-// credential — the "callers MUST check exp" contract on UpstreamCredential.IDToken
-// exists for RFC 8693 subject-token use, and the service that owns the field says
-// so (see the TODO in pkg/auth/upstreamtoken/service.go). Enforcing it here would
-// be actively harmful: sessions live for the refresh-token lifespan (7 days by
-// default) while id_tokens expire in minutes, so a policy would permit early in a
-// session and silently deny later. The claims are no staler than the alternative —
-// the ToolHive-issued token's mirrored profile claims are also captured once at
-// login and never refreshed.
+// The id_token is used without checking `exp`, deliberately: it is read as a record
+// of what the upstream asserted at login, not presented as a credential, and
+// rejecting it once expired would flip a policy from permit to deny mid-session.
+// See Identity.UpstreamIDTokens for that contract and its two consumers.
 func (a *Authorizer) resolveClaims(identity *auth.Identity) (jwt.MapClaims, error) {
 	requestClaims := jwt.MapClaims(identity.Claims)
 
@@ -710,15 +704,29 @@ func (a *Authorizer) supplementFromIDToken(identity *auth.Identity, upstreamClai
 	return merged
 }
 
+// OIDC Core 1.0 §5.1 standard claim names, declared here rather than borrowed
+// from another package. What this code reads is an upstream provider's id_token,
+// so these are wire names fixed by the OIDC spec — not names ToolHive chooses.
+// Anchoring them to a ToolHive constant would invert the dependency: renaming that
+// constant would silently change which claim Cedar reads out of a third party's
+// token. Two literals also do not justify pulling an authorization-server
+// dependency tree into the authorizer.
+const (
+	oidcNameClaim  = "name"
+	oidcEmailClaim = "email"
+)
+
 // profileClaimsFromIDToken are the OIDC Core §5.1 profile claims that may be read
-// from the primary provider's id_token when its access token omits them. They are
-// spelled with the auth server's own constants — the same two claims it extracts
-// from that id_token into the token it issues (upstream/oidc.go into session.New) —
-// so a rename on either side is a compile error. That does not catch a claim being
-// ADDED there, which still needs a deliberate decision here.
+// from the primary provider's id_token when its access token omits them. They
+// coincide with the two claims the embedded auth server extracts from that same
+// id_token: pkg/authserver/upstream/oidc.go reads them by their OIDC names, and the
+// callback handler then passes them to session.New
+// (pkg/authserver/server/handlers/callback.go). Both sides key off the wire names
+// independently — a shared reliance on the spec, not a coupling. If the auth server
+// ever mirrors more, adding it here is a separate, deliberate decision.
 //
 // Widening this list widens what can satisfy a policy — see resolveClaims first.
-var profileClaimsFromIDToken = []string{session.NameClaimKey, session.EmailClaimKey}
+var profileClaimsFromIDToken = []string{oidcNameClaim, oidcEmailClaim}
 
 // supplementProfileClaims returns a fresh claim set holding every access-token
 // claim plus, for each name in profileClaimsFromIDToken that the access token does