diff --git a/docs/operator/virtualmcpserver-kubernetes-guide.md b/docs/operator/virtualmcpserver-kubernetes-guide.md index b186cfcdc5..93e7a7b010 100644 --- a/docs/operator/virtualmcpserver-kubernetes-guide.md +++ b/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 diff --git a/pkg/auth/identity.go b/pkg/auth/identity.go index 66bb9bccbd..80f795e572 100644 --- a/pkg/auth/identity.go +++ b/pkg/auth/identity.go @@ -121,7 +121,7 @@ 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 @@ -129,8 +129,29 @@ type Identity struct { // - 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. diff --git a/pkg/auth/upstreamtoken/service.go b/pkg/auth/upstreamtoken/service.go index 503e15ab6d..3c948b590e 100644 --- a/pkg/auth/upstreamtoken/service.go +++ b/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 } diff --git a/pkg/auth/upstreamtoken/types.go b/pkg/auth/upstreamtoken/types.go index df26f3fc2d..90beaaac04 100644 --- a/pkg/auth/upstreamtoken/types.go +++ b/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) } diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index aa03265344..d6198e71fe 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/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