diff --git a/docs/arch/17-token-exchange-delegation.md b/docs/arch/17-token-exchange-delegation.md new file mode 100644 index 0000000000..b34d412d34 --- /dev/null +++ b/docs/arch/17-token-exchange-delegation.md @@ -0,0 +1,257 @@ +# External Subject-Token Exchange (Delegation) + +This document covers the embedded authorization server's RFC 8693 token-exchange +grant when the `subject_token` comes from an external, trusted OIDC issuer +(`trustedIssuers`). It is unrelated to the *outgoing* token exchange described in +docs [02](02-core-concepts.md), [04](04-secrets-management.md), +[05](05-runconfig-and-permissions.md), [09](09-operator-architecture.md), and +[10](10-virtual-mcp-architecture.md), where `MCPExternalAuthConfig` middleware +exchanges a client's token for an upstream IdP token. Here the server is the +token *issuer*, accepting an externally-minted token as proof of identity and +re-issuing a ToolHive-scoped delegated token. + +Normally the grant accepts only the server's own self-issued tokens as +`subject_token`. `trustedIssuers` extends that to tokens minted by an external +IdP (e.g. a corporate IdP), so a client already holding a token from that IdP +can exchange it for a ToolHive delegated token without a separate ToolHive +login. + +## Prerequisite: not yet usable end to end + +- Using this grant requires a confidential client holding the token-exchange + grant. No supported deployment path provisions one today: DCR always + registers public clients restricted to `authorization_code`/`refresh_token`, + CIMD clients are public-only, and `RunConfig` has no client-seeding field. +- Discovery metadata does not yet advertise the token-exchange grant or any + secret-based client auth method, so a metadata-driven client will conclude + the grant is unsupported and never attempt it. +- This applies to the self-issued subject-token path too, not only + `trustedIssuers`. +- Tracked in [#6082](https://github.com/stacklok/toolhive/issues/6082). + +## Trust model + +A trusted issuer plus a matching audience and a valid signature only proves +the token was minted for ToolHive to consume — it authorizes ToolHive as a +*resource*, not any particular *client* as a delegate. Accepting the token on +that basis alone would let any client presenting a validly-signed token +exchange it: a confused-deputy risk (CWE-863). The handler therefore requires +an explicit consent signal before treating the request as an authorized +delegation. + +## Consent signals + +Two functions cooperate here. `resolveAllowedActor` +(`pkg/authserver/server/tokenexchange/multi_issuer_validator.go`), called from +`validateExternalToken` during signature/claim validation, checks the +external-issuer allowlist and — when it matches — sets +`ValidatedClaims.ExternalActor`. `checkDelegationConsent` +(`pkg/authserver/server/tokenexchange/handler.go`) runs afterwards and decides +whether to grant the exchange, in this order: + +1. **`may_act` (RFC 8693 §4.4).** If the subject token carries a well-formed + `may_act` claim, it is authoritative: the allowlist step above is skipped + entirely (`validateExternalToken` never calls `resolveAllowedActor` when + `may_act` is present), and `checkDelegationConsent` enforces `may_act.sub` + against the authenticated ToolHive client. A malformed `may_act` is + rejected outright by `validateMayActShape`, not silently ignored or + fallen through to the allowlist. On the external path, `may_act.iss` is + mandatory, not merely constrained when present as on the self-issued path + — `validateMayActShape`'s `requireIss` parameter is `true` there. Without + this, an external issuer omitting `iss` could authorize any ToolHive + client by naming its ID in a bare `may_act.sub`, bypassing + `allowedActors`/`allowedDelegateClients` entirely — the one consent path + that does. +2. **`ExternalActor`.** Otherwise, consent was already established by + `resolveAllowedActor`: the claim named by that issuer's `actorClaim` + (default `azp`; `appid` for Microsoft Entra v1, `cid` for Okta) matched an + entry in that issuer's `allowedActors`. `allowedActors` holds client IDs in + the *external* IdP's namespace, not ToolHive client IDs — the likeliest + misconfiguration. An empty `allowedActors` accepts only `may_act`-bearing + tokens from that issuer. `checkDelegationConsent` then additionally checks + the issuer's `allowedDelegateClients` against the authenticated ToolHive + client — this field is required (see below), so the check always applies + on this path. +3. **`client_id` binding.** For a self-issued subject token (not part of the + external path above), the token's `client_id` must match the authenticated + client. +4. **No binding.** If none of the above apply, the token carries no + verifiable client binding and the exchange is rejected. + +## Accepted limitations + +1. **`allowedDelegateClients` binds an allowlisted actor to specific ToolHive + clients, and is mandatory, not opt-in.** `allowedActors` by itself + authorizes "this external client's tokens may be exchanged here," not + "…by this particular ToolHive client" — the external actor claim is never + compared against the authenticated ToolHive client ID. Without a separate + binding, every ToolHive confidential client holding the token-exchange + grant would be delegation-equivalent with respect to an allowlisted + external actor, so compromise of the weakest such client would be as good + as compromise of all of them. + + `TrustedIssuer.AllowedDelegateClients` (`allowed_delegate_clients` on a + hand-written `authserver.RunConfig`) closes this per issuer: a list of + ToolHive client IDs permitted to use that issuer's allowlisted actors, or + the wildcard `"*"` to explicitly permit any ToolHive client holding the + token-exchange grant. `validateTrustedIssuer` rejects the field when it is + empty or absent — permissiveness must be *declared* with the wildcard, not + obtained by leaving the field out — and rejects the wildcard combined with + specific client IDs, since silently ignoring the specific IDs alongside it + would be worse than rejecting the config outright. `checkDelegationConsent` + checks the authenticated client against this list on both consent paths: + `may_act` bypasses `allowedActors` (the external-issuer allowlist) but NOT + `allowedDelegateClients`, since the validator sets `AllowedDelegateClients` + for every external token regardless of which path authorized it (see + limitation 4 below). A self-issued `may_act` (no external issuer involved) + has no `allowedDelegateClients` equivalent and is unaffected — it remains + bound by `may_act.sub` alone. + + Nothing can reach this code path in production today: the token-exchange + grant requires a confidential client, and no supported deployment path + provisions one (see issue #6082). That makes the fail-closed default free + right now — it would be a breaking change once a client can actually reach + this grant. +2. **Subject namespace collisions are closed, not accepted.** A trusted + issuer is trusted to assert *any* subject this server accepts for + delegation, and downstream authorization decisions (Cedar) key on `sub` + alone — not `iss`. Rather than requiring operators to keep every issuer's + subject namespace disjoint, the delegated token's `sub` is qualified as + `#` for every trusted-issuer exchange + (`tokenexchange.delegatedSubject`), never the external `sub` copied + verbatim. ToolHive's own native subjects are UUIDs minted by + `UserResolver.ResolveUser`, which cannot contain `#`, so a qualified + external subject can never collide with one. Scope names are not + qualified this way and remain the operator's responsibility to keep + disjoint across issuers. +3. **Provenance is recorded for every external token.** The RFC 8693 §4.1 + `act` claim records who acted: `act.sub` is always the ToolHive client, + with the external issuer nested one level in — `ValidatedClaims.ExternalIssuer` + is set for every token validated by the external-issuer path, whether or + not it also carries `may_act`. The nested entry additionally carries `sub` + (the allowlisted actor claim) when the allowlist path resolved one; + a `may_act`-bearing external token yields `act = {sub: , + act: {iss: }}` — no client-namespace actor to report, but + the issuer is still recorded. Either way, Cedar authorizers key on `sub` + and do not read `act` — it is an audit trail, not an access control. (AWS + STS role mapping can read arbitrary claims including `act` via its CEL + matcher, so "authorizers" here means Cedar specifically, not every + consumer.) +4. **A `may_act`-emitting issuer bypasses the allowlist entirely.** Since it + takes priority and can name any ToolHive client, that claim must be drawn + from ToolHive's own client namespace and must not be influenceable by an + untrusted party. +5. **ID/access-token discrimination on the external path rests on two + partial layers, neither of which is exhaustive alone.** Nothing inspects + the JWT header's `typ` claim: RFC 8725 §3.12 recommends `typ: at+jwt` for + access tokens, but Entra v1/v2, Okta, and Google all emit a bare `JWT` + `typ` for access tokens too, so requiring `at+jwt` would reject those + providers' genuine access tokens. + + Layer one: `rejectIDTokenClaims` (`validator.go`) rejects a subject token + carrying `at_hash` or `c_hash` — OIDC Core §3.3.2.11/§3.3.2.10 define both + for ID tokens only. This is a sound *positive* detector but not + exhaustive: both claims are OPTIONAL even on a valid + authorization-code-flow ID token, so an ID token that omits them is not + caught here. + + Layer two: `TrustedIssuer.ExpectedAudience` is documented as MUST be a + resource/API identifier and MUST NOT be a client ID, since an ID token's + `aud` is the requesting client's own ID while an access token's `aud` + names the resource. This check is operator-dependent, not + self-enforcing — nothing validates that the operator actually set a + resource identifier rather than a client ID. + `NewMultiIssuerTokenValidator` emits a `slog.Warn` at startup + (`looksLikeResourceIdentifier`) when `expected_audience` has no URI + scheme (`https://`, `api://`), naming the issuer and the consequence, but + does not hard-reject it: Entra v1 legitimately uses a bare app-ID GUID + (no scheme) as an access token's `aud`, so rejecting every non-URI + audience would break that real, supported provider. + + A third discriminator — rejecting when a subject token's `aud` equals the + issuer's configured actor claim, since OIDC Core requires an ID token's + `aud` to equal its own `azp` — was evaluated and not shipped: an app whose + API and client share one registration (the same ambiguous + bare-GUID-audience configuration layer two warns about) legitimately + issues *access* tokens where `aud` and `azp`/`appid` are the same value + too, so this would reject genuine access tokens from that setup. + + Taken together: an operator who sets `expected_audience` to a client ID + (despite the startup warning), talking to a real IdP whose ID tokens omit + `at_hash`/`c_hash` (both legal per OIDC Core), has no remaining + discriminator here. The blast radius is bounded: an admitted ID token + carries no `scope`/`scp` claim, so `grantScopes` (`handler.go`) either + rejects the exchange with `invalid_scope` or grants a zero-scope + delegated token — it must also be signed by a configured trusted issuer + and satisfy one of the consent signals above. + +## Operational notes + +- **Audience.** The requested audience is bounded by the subject token's own + `aud` claim (`ensureAudienceSubsetOfSubject`); a request for an audience the + subject token doesn't carry fails with `invalid_target`. An external IdP's + `aud` is typically an app-ID GUID or `api://`. For an exchange to + succeed, the external IdP's API identifier must be set as the per-issuer + `expectedAudience` *and* appear in the server's own `allowedAudiences` (plus + the calling client's registered audiences) — `expectedAudience` alone is not + enough, since it only governs whether the subject token validates, not what + the delegated token is granted. A startup WARN fires when `expectedAudience` + isn't in `allowedAudiences`. +- **Scopes.** `grantScopes` rejects — it does not intersect — any requested + scope absent from the subject token's granted scopes, with `invalid_scope`. + Both the RFC 9068 `"scope"` string claim and the `"scp"` array claim are + read (`"scope"` wins if both are present) — `"scp"` is not an Entra + quirk, it is what fosite's own default JWT claims strategy writes for a + self-issued ToolHive access token, so a genuine subject token minted by + this server's own token endpoint carries scopes this way already. +- **Delegated token lifetime.** `min(subject token's remaining lifetime, + configured delegationLifespan)`. A short-lived external token silently + yields a short delegated token. +- **Delegation chain depth.** Re-exchanging an already-delegated token nests + `act` further; `maxDelegationDepth` (10) bounds it, past which the exchange + fails with `invalid_grant` ("delegation chain is too deep"). +- **Discovery redirects.** The per-issuer HTTP client only follows same-host + redirects (`networking.SameHostRedirectPolicy()`), so an issuer whose + `/.well-known/openid-configuration` redirects cross-host cannot be + onboarded via discovery — set `jwksUrl` explicitly to skip discovery. +- **JWKS caching.** A successful fetch is cached and refreshed by jwx's + `jwk.Cache` on a schedule driven by the response's `Cache-Control`/`Expires` + headers (no fixed TTL). The 30-second figure is `jwksFetchFailureBackoff`, + which gates only how often a persistently-failing issuer is retried before + its first successful fetch — so a just-corrected `jwksUrl` can keep failing + for up to 30s before the next retry; once a fetch has ever succeeded, this + backoff no longer applies. +- **Diagnostics.** A non-allowlisted actor and an untrusted issuer both + surface as the same generic `invalid_request` + ("The subject token is invalid or could not be verified."), per RFC 8693 + §2.2.2. The only discriminator is a `slog.Debug` log line, so diagnosing a + failed exchange needs debug logging enabled. The two exceptions are startup + `slog.Warn` lines — an issuer with empty `allowedActors`, and an + `expectedAudience` missing from `allowedAudiences` — the only non-DEBUG + diagnostics this feature emits. +- **HTTPS enforcement for a trusted issuer.** `validateTrustedIssuerURL` + (`pkg/authserver/config.go`) validates `issuer_url` at config time and, + unlike the server's own issuer, grants no localhost exemption: an + `http://localhost` trusted issuer is rejected unless that issuer's own + `insecure_allow_http` is set. There is no separate runtime scheme check for + `issuer_url` — only `jwks_url` gets one (`ValidateJWKSURL`, on every fetch; + shared verbatim between the runtime choke point and the config-time check + so the two can't drift). There is no operator (CRD) surface for this + feature yet — see [#6082](https://github.com/stacklok/toolhive/issues/6082) + — so today `trustedIssuers` is reachable only via a hand-written + `authserver.RunConfig`. +- **`allowPrivateIPs` requires `jwksUrl`.** Enforced at config time by + `validateTrustedIssuers` (`pkg/authserver/config.go`) for every RunConfig. + Without a hand-configured `jwksUrl`, OIDC discovery — a document fetched + from, and thus influenceable by, the external issuer itself — would choose + the private JWKS dial target, which is exactly what pinning it to + operator-supplied config prevents. +- **Misconfiguration surfaces as a pod crash**, not an operator condition — + check pod logs, not `kubectl describe`. + +## Implementation + +- `pkg/authserver/server/tokenexchange/handler.go` — `checkDelegationConsent`, `grantScopes`, `grantAndBoundAudiences` +- `pkg/authserver/server/tokenexchange/multi_issuer_validator.go` — `MultiIssuerTokenValidator`, `TrustedIssuer`, `resolveAllowedActor`, `ValidateJWKSURL` +- `pkg/authserver/server/tokenexchange/validator.go` — `assignClaim`, `buildValidatedClaims`, `validateMayActShape`, `scpToScopeString` +- `pkg/authserver/config.go` — `validateTrustedIssuerURL`, `validateJWKSEndpointURL`, `validateTrustedIssuers`, `warnTrustedIssuerAudiences` diff --git a/docs/arch/README.md b/docs/arch/README.md index 898ce7bc77..13df8eeb99 100644 --- a/docs/arch/README.md +++ b/docs/arch/README.md @@ -134,6 +134,12 @@ Welcome to the ToolHive architecture documentation. This directory contains comp - Composite-workflow suspend/resume as the sole durable-state trigger (RFC-0083 D5 handles, owner binding, TTL) - Sampling's deprecated-pass-through-only disposition (SEP-2577) and the go-sdk/mcpcompat gap list +17. **[External Subject-Token Exchange (Delegation)](17-token-exchange-delegation.md)** + - `trustedIssuers`: accepting subject tokens from an external OIDC issuer during RFC 8693 token exchange + - Confused-deputy trust model and the `may_act` / allowlist consent signals + - Accepted limitations: client-set equivalence, disjoint subject namespaces, partial provenance + - Operational gotchas: audience/scope binding, discovery redirects, JWKS caching, diagnostics + ### Existing Documentation For middleware architecture, see: **[docs/middleware.md](../middleware.md)** diff --git a/docs/server/docs.go b/docs/server/docs.go index dfa6533e6d..4e9f59ac11 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -566,6 +566,14 @@ const docTemplate = `{ "token_lifespans": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_authserver.TokenLifespanRunConfig" }, + "trusted_issuers": { + "description": "TrustedIssuers lists external OIDC issuers whose tokens are accepted as\nsubject tokens during RFC 8693 token exchange. Empty (the default) means\nonly self-issued subject tokens are accepted.\n\nPrerequisite: the token-exchange grant requires a confidential client,\nand no supported deployment path provisions one today (DCR and CIMD\nclients are both public-only, and there is no client-seeding field on\nthis RunConfig), so this grant is not yet usable end to end for\nself-issued or external subject tokens alike. Tracked in\nhttps://github.com/stacklok/toolhive/issues/6082.\n\nSee tokenexchange.TrustedIssuer for the per-issuer field reference, and\ndocs/arch/17-token-exchange-delegation.md for the trust model, consent\nsignals, and operator-facing constraints (audience/scope bounding,\nsubject namespace qualification, required client binding) that aren't\nvisible from the config shape alone.", + "items": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_authserver_server_tokenexchange.TrustedIssuer" + }, + "type": "array", + "uniqueItems": false + }, "upstreams": { "description": "Upstreams configures connections to upstream Identity Providers.\nAt least one upstream is required - the server delegates authentication to these providers.\nMultiple upstreams are supported for sequential authorization chains.", "items": { @@ -723,6 +731,51 @@ const docTemplate = `{ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_authserver_server_tokenexchange.TrustedIssuer": { + "properties": { + "actor_claim": { + "description": "ActorClaim names the claim identifying the client that requested the\nsubject token from THIS EXTERNAL ISSUER (used by AllowedActors below).\nValues are in the external issuer's namespace, NOT ToolHive client\nIDs. Defaults to \"azp\"; use \"appid\" for Microsoft Entra v1, \"cid\" for\nOkta. The special value \"client_id\" reads ValidatedClaims.ClientID\ninstead of Extra (assignClaim routes it to that field) — it is still\nthe external token's client_id claim, not a ToolHive one.", + "type": "string" + }, + "allow_private_ips": { + "description": "AllowPrivateIPs permits OIDC discovery and JWKS fetches for THIS\nissuer to resolve to a private or loopback address. Use only when the\nissuer is hosted inside the same cluster and has no public endpoint.", + "type": "boolean" + }, + "allowed_actors": { + "description": "AllowedActors is the allowlist of ActorClaim values authorized to\nexchange a subject token from this issuer when it carries no\n\"may_act\" claim; empty means only may_act-bearing tokens are\naccepted. By itself names no ToolHive client — see\nAllowedDelegateClients and docs/arch/17-token-exchange-delegation.md\n(\"Accepted limitations\" #1).", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "allowed_delegate_clients": { + "description": "AllowedDelegateClients restricts which ToolHive client IDs may\nexchange a subject token from this issuer, for BOTH consent paths.\nRequired (validateTrustedIssuer rejects empty/absent); \"*\" permits\nany confidential client holding the grant. See\ndocs/arch/17-token-exchange-delegation.md (\"Accepted limitations\" #1).", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "expected_audience": { + "description": "ExpectedAudience is the expected \"aud\" claim value that must appear\nin the token's audience list (a resource/API identifier, not a\nclient ID — required, but not enforced; see looksLikeResourceIdentifier).\nSee docs/arch/17-token-exchange-delegation.md (\"ID/access-token\ndiscrimination\") for why and its limits.", + "type": "string" + }, + "insecure_allow_http": { + "description": "InsecureAllowHTTP permits plain-HTTP OIDC discovery and JWKS fetches\nfor THIS issuer only. Development and testing only — never set in\nproduction. Does not relax the private-IP guard; see AllowPrivateIPs.\nDeliberately per-issuer: this server's own InsecureAllowHTTP must not\nsilently permit plaintext discovery for every trusted external issuer\ntoo — a network attacker who can intercept that traffic could\nsubstitute a JWKS and forge subject tokens for that issuer's\nnamespace.", + "type": "boolean" + }, + "issuer_url": { + "description": "IssuerURL is the expected \"iss\" claim value (exact match).", + "type": "string" + }, + "jwks_url": { + "description": "JWKSURL is the URL to fetch the issuer's JSON Web Key Set from.\nIf empty, it is resolved via OIDC discovery at {IssuerURL}/.well-known/openid-configuration.", + "type": "string" + } + }, + "type": "object" + }, "github_com_stacklok_toolhive_pkg_authz.Config": { "description": "DEPRECATED: Middleware configuration.\nAuthzConfig contains the authorization configuration", "properties": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index a9625fbec9..ef5bbd39ce 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -559,6 +559,14 @@ "token_lifespans": { "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_authserver.TokenLifespanRunConfig" }, + "trusted_issuers": { + "description": "TrustedIssuers lists external OIDC issuers whose tokens are accepted as\nsubject tokens during RFC 8693 token exchange. Empty (the default) means\nonly self-issued subject tokens are accepted.\n\nPrerequisite: the token-exchange grant requires a confidential client,\nand no supported deployment path provisions one today (DCR and CIMD\nclients are both public-only, and there is no client-seeding field on\nthis RunConfig), so this grant is not yet usable end to end for\nself-issued or external subject tokens alike. Tracked in\nhttps://github.com/stacklok/toolhive/issues/6082.\n\nSee tokenexchange.TrustedIssuer for the per-issuer field reference, and\ndocs/arch/17-token-exchange-delegation.md for the trust model, consent\nsignals, and operator-facing constraints (audience/scope bounding,\nsubject namespace qualification, required client binding) that aren't\nvisible from the config shape alone.", + "items": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_authserver_server_tokenexchange.TrustedIssuer" + }, + "type": "array", + "uniqueItems": false + }, "upstreams": { "description": "Upstreams configures connections to upstream Identity Providers.\nAt least one upstream is required - the server delegates authentication to these providers.\nMultiple upstreams are supported for sequential authorization chains.", "items": { @@ -716,6 +724,51 @@ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_authserver_server_tokenexchange.TrustedIssuer": { + "properties": { + "actor_claim": { + "description": "ActorClaim names the claim identifying the client that requested the\nsubject token from THIS EXTERNAL ISSUER (used by AllowedActors below).\nValues are in the external issuer's namespace, NOT ToolHive client\nIDs. Defaults to \"azp\"; use \"appid\" for Microsoft Entra v1, \"cid\" for\nOkta. The special value \"client_id\" reads ValidatedClaims.ClientID\ninstead of Extra (assignClaim routes it to that field) — it is still\nthe external token's client_id claim, not a ToolHive one.", + "type": "string" + }, + "allow_private_ips": { + "description": "AllowPrivateIPs permits OIDC discovery and JWKS fetches for THIS\nissuer to resolve to a private or loopback address. Use only when the\nissuer is hosted inside the same cluster and has no public endpoint.", + "type": "boolean" + }, + "allowed_actors": { + "description": "AllowedActors is the allowlist of ActorClaim values authorized to\nexchange a subject token from this issuer when it carries no\n\"may_act\" claim; empty means only may_act-bearing tokens are\naccepted. By itself names no ToolHive client — see\nAllowedDelegateClients and docs/arch/17-token-exchange-delegation.md\n(\"Accepted limitations\" #1).", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "allowed_delegate_clients": { + "description": "AllowedDelegateClients restricts which ToolHive client IDs may\nexchange a subject token from this issuer, for BOTH consent paths.\nRequired (validateTrustedIssuer rejects empty/absent); \"*\" permits\nany confidential client holding the grant. See\ndocs/arch/17-token-exchange-delegation.md (\"Accepted limitations\" #1).", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "expected_audience": { + "description": "ExpectedAudience is the expected \"aud\" claim value that must appear\nin the token's audience list (a resource/API identifier, not a\nclient ID — required, but not enforced; see looksLikeResourceIdentifier).\nSee docs/arch/17-token-exchange-delegation.md (\"ID/access-token\ndiscrimination\") for why and its limits.", + "type": "string" + }, + "insecure_allow_http": { + "description": "InsecureAllowHTTP permits plain-HTTP OIDC discovery and JWKS fetches\nfor THIS issuer only. Development and testing only — never set in\nproduction. Does not relax the private-IP guard; see AllowPrivateIPs.\nDeliberately per-issuer: this server's own InsecureAllowHTTP must not\nsilently permit plaintext discovery for every trusted external issuer\ntoo — a network attacker who can intercept that traffic could\nsubstitute a JWKS and forge subject tokens for that issuer's\nnamespace.", + "type": "boolean" + }, + "issuer_url": { + "description": "IssuerURL is the expected \"iss\" claim value (exact match).", + "type": "string" + }, + "jwks_url": { + "description": "JWKSURL is the URL to fetch the issuer's JSON Web Key Set from.\nIf empty, it is resolved via OIDC discovery at {IssuerURL}/.well-known/openid-configuration.", + "type": "string" + } + }, + "type": "object" + }, "github_com_stacklok_toolhive_pkg_authz.Config": { "description": "DEPRECATED: Middleware configuration.\nAuthzConfig contains the authorization configuration", "properties": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 0969382edb..5a8d3ad938 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -667,6 +667,28 @@ components: $ref: '#/components/schemas/storage.RunConfig' token_lifespans: $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_authserver.TokenLifespanRunConfig' + trusted_issuers: + description: |- + TrustedIssuers lists external OIDC issuers whose tokens are accepted as + subject tokens during RFC 8693 token exchange. Empty (the default) means + only self-issued subject tokens are accepted. + + Prerequisite: the token-exchange grant requires a confidential client, + and no supported deployment path provisions one today (DCR and CIMD + clients are both public-only, and there is no client-seeding field on + this RunConfig), so this grant is not yet usable end to end for + self-issued or external subject tokens alike. Tracked in + https://github.com/stacklok/toolhive/issues/6082. + + See tokenexchange.TrustedIssuer for the per-issuer field reference, and + docs/arch/17-token-exchange-delegation.md for the trust model, consent + signals, and operator-facing constraints (audience/scope bounding, + subject namespace qualification, required client binding) that aren't + visible from the config shape alone. + items: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_authserver_server_tokenexchange.TrustedIssuer' + type: array + uniqueItems: false upstreams: description: |- Upstreams configures connections to upstream Identity Providers. @@ -830,6 +852,75 @@ components: If not specified, defaults to GET. type: string type: object + github_com_stacklok_toolhive_pkg_authserver_server_tokenexchange.TrustedIssuer: + properties: + actor_claim: + description: |- + ActorClaim names the claim identifying the client that requested the + subject token from THIS EXTERNAL ISSUER (used by AllowedActors below). + Values are in the external issuer's namespace, NOT ToolHive client + IDs. Defaults to "azp"; use "appid" for Microsoft Entra v1, "cid" for + Okta. The special value "client_id" reads ValidatedClaims.ClientID + instead of Extra (assignClaim routes it to that field) — it is still + the external token's client_id claim, not a ToolHive one. + type: string + allow_private_ips: + description: |- + AllowPrivateIPs permits OIDC discovery and JWKS fetches for THIS + issuer to resolve to a private or loopback address. Use only when the + issuer is hosted inside the same cluster and has no public endpoint. + type: boolean + allowed_actors: + description: |- + AllowedActors is the allowlist of ActorClaim values authorized to + exchange a subject token from this issuer when it carries no + "may_act" claim; empty means only may_act-bearing tokens are + accepted. By itself names no ToolHive client — see + AllowedDelegateClients and docs/arch/17-token-exchange-delegation.md + ("Accepted limitations" #1). + items: + type: string + type: array + uniqueItems: false + allowed_delegate_clients: + description: |- + AllowedDelegateClients restricts which ToolHive client IDs may + exchange a subject token from this issuer, for BOTH consent paths. + Required (validateTrustedIssuer rejects empty/absent); "*" permits + any confidential client holding the grant. See + docs/arch/17-token-exchange-delegation.md ("Accepted limitations" #1). + items: + type: string + type: array + uniqueItems: false + expected_audience: + description: |- + ExpectedAudience is the expected "aud" claim value that must appear + in the token's audience list (a resource/API identifier, not a + client ID — required, but not enforced; see looksLikeResourceIdentifier). + See docs/arch/17-token-exchange-delegation.md ("ID/access-token + discrimination") for why and its limits. + type: string + insecure_allow_http: + description: |- + InsecureAllowHTTP permits plain-HTTP OIDC discovery and JWKS fetches + for THIS issuer only. Development and testing only — never set in + production. Does not relax the private-IP guard; see AllowPrivateIPs. + Deliberately per-issuer: this server's own InsecureAllowHTTP must not + silently permit plaintext discovery for every trusted external issuer + too — a network attacker who can intercept that traffic could + substitute a JWKS and forge subject tokens for that issuer's + namespace. + type: boolean + issuer_url: + description: IssuerURL is the expected "iss" claim value (exact match). + type: string + jwks_url: + description: |- + JWKSURL is the URL to fetch the issuer's JSON Web Key Set from. + If empty, it is resolved via OIDC discovery at {IssuerURL}/.well-known/openid-configuration. + type: string + type: object github_com_stacklok_toolhive_pkg_authz.Config: description: |- DEPRECATED: Middleware configuration. diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index 72d9cdb67b..2ca7e54249 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -10,6 +10,7 @@ import ( "log/slog" "net/url" "regexp" + "slices" "strings" "time" @@ -18,6 +19,7 @@ import ( "github.com/stacklok/toolhive/pkg/authserver/server/handlers" "github.com/stacklok/toolhive/pkg/authserver/server/keys" "github.com/stacklok/toolhive/pkg/authserver/server/registration" + "github.com/stacklok/toolhive/pkg/authserver/server/tokenexchange" "github.com/stacklok/toolhive/pkg/authserver/storage" "github.com/stacklok/toolhive/pkg/authserver/upstream" "github.com/stacklok/toolhive/pkg/networking" @@ -117,6 +119,25 @@ type RunConfig struct { // Production deployments reachable outside the cluster MUST use https://. //nolint:lll // field tags require full JSON+YAML names InsecureAllowHTTP bool `json:"insecure_allow_http,omitempty" yaml:"insecure_allow_http,omitempty"` + + // TrustedIssuers lists external OIDC issuers whose tokens are accepted as + // subject tokens during RFC 8693 token exchange. Empty (the default) means + // only self-issued subject tokens are accepted. + // + // Prerequisite: the token-exchange grant requires a confidential client, + // and no supported deployment path provisions one today (DCR and CIMD + // clients are both public-only, and there is no client-seeding field on + // this RunConfig), so this grant is not yet usable end to end for + // self-issued or external subject tokens alike. Tracked in + // https://github.com/stacklok/toolhive/issues/6082. + // + // See tokenexchange.TrustedIssuer for the per-issuer field reference, and + // docs/arch/17-token-exchange-delegation.md for the trust model, consent + // signals, and operator-facing constraints (audience/scope bounding, + // subject namespace qualification, required client binding) that aren't + // visible from the config shape alone. + //nolint:lll // field tags require full JSON+YAML names + TrustedIssuers []tokenexchange.TrustedIssuer `json:"trusted_issuers,omitempty" yaml:"trusted_issuers,omitempty"` } // Validate checks that the on-disk RunConfig is internally consistent. Called @@ -129,6 +150,16 @@ func (c *RunConfig) Validate() error { return fmt.Errorf("cimd: %w", err) } } + // Also checked by Config.Validate() (same reasoning as + // validateBaselineClientScopes below): buildUpstreamConfigs performs live + // RFC 7591 registration against upstream IdPs before authserver.New ever + // reaches Config.Validate(), so a malformed trusted-issuer config must + // fail here, before that side-effecting work runs — not on a crash loop + // after it, which would also orphan an upstream registration on every + // restart with the default in-memory DCR store. + if err := validateTrustedIssuers(c.TrustedIssuers, c.Issuer); err != nil { + return err + } return c.validateBaselineClientScopes() } @@ -685,6 +716,13 @@ type Config struct { // Only set this for in-cluster Kubernetes deployments on a trusted network. // Production deployments reachable outside the cluster MUST use https://. InsecureAllowHTTP bool + + // TrustedIssuers lists external OIDC issuers whose tokens are accepted as + // subject tokens during RFC 8693 token exchange. See the identically + // named field on RunConfig for the full doc comment, including the + // fail-closed AllowedActors semantics and the audience/scope constraints + // operators must account for. + TrustedIssuers []tokenexchange.TrustedIssuer } // Validate checks that the Config is valid. @@ -749,6 +787,15 @@ func (c *Config) Validate() error { return err } + // RunConfig.Validate() also runs this (see the comment there for why: + // buildUpstreamConfigs's live DCR registration happens before this method + // is reached), but a caller that constructs Config directly bypasses that, + // same as the BaselineClientScopes check above. + if err := validateTrustedIssuers(c.TrustedIssuers, c.Issuer); err != nil { + return err + } + c.warnTrustedIssuerAudiences() + slog.Debug("authserver config validation passed", "issuer", c.Issuer, "upstream_count", len(c.Upstreams), @@ -786,6 +833,102 @@ func (c *Config) validateDelegationTokenLifespan() error { return nil } +// validateTrustedIssuers checks every configured TrustedIssuer as early as +// RunConfig.Validate can catch it, so a bad actor_claim or duplicate +// issuer_url fails before buildUpstreamConfigs performs live RFC 7591 +// registration against upstream IdPs — not after it, on a crash loop that +// orphans an upstream registration on every restart. Shared by +// RunConfig.Validate() and Config.Validate() (mirrors +// validateBaselineClientScopes); NewMultiIssuerTokenValidator repeats these +// checks again at server startup as defence in depth. +// +// issuer_url is checked by validateTrustedIssuerURL, jwks_url (when set) by +// validateJWKSEndpointURL — see their doc comments for the URL rules each +// enforces. The remaining structural checks (required fields, self-issuer +// collision, duplicate issuers, ActorClaim reachability) run via +// tokenexchange.ValidateTrustedIssuers. +func validateTrustedIssuers(issuers []tokenexchange.TrustedIssuer, selfIssuer string) error { + for _, ti := range issuers { + if err := validateTrustedIssuerURL(ti.IssuerURL, ti.InsecureAllowHTTP); err != nil { + return fmt.Errorf("trusted_issuers: issuer_url %q: %w", ti.IssuerURL, err) + } + // AllowPrivateIPs without a hand-configured jwks_url would let OIDC + // discovery — a document fetched from, and thus influenceable by, + // the external issuer itself — choose the private target the dial + // is allowed to reach. Requiring jwks_url pins that target to + // operator-supplied config instead. + // + // This is the fail-fast layer, not the only one: validateTrustedIssuer + // (multi_issuer_validator.go) enforces the same invariant inside + // NewMultiIssuerTokenValidator, so a caller constructing a validator + // without routing through Config.Validate is still covered. Note that + // ensureRegistered's ValidateJWKSURL does NOT cover it — that check is + // gated on net.ParseIP, so it only rejects private IP *literals*, and a + // discovery document advertising a private *hostname* passes it + // cleanly. Checking here and in the constructor is deliberate + // duplication, not redundancy. + if ti.AllowPrivateIPs && ti.JWKSURL == "" { + return fmt.Errorf( + "trusted_issuers: issuer_url %q: allow_private_ips requires jwks_url to be set explicitly; "+ + "otherwise OIDC discovery — fetched from the external issuer — would choose the private target", + ti.IssuerURL) + } + if ti.JWKSURL != "" { + if err := validateJWKSEndpointURL(ti.JWKSURL, ti.InsecureAllowHTTP, ti.AllowPrivateIPs); err != nil { + return fmt.Errorf("trusted_issuers: jwks_url %q: %w", ti.JWKSURL, err) + } + } + } + if err := tokenexchange.ValidateTrustedIssuers(issuers, selfIssuer); err != nil { + return fmt.Errorf("trusted_issuers: %w", err) + } + return nil +} + +// validateJWKSEndpointURL checks that rawURL parses, has a host, uses the +// "https" scheme (or "http" when insecureAllowHTTP is set), and — when the +// host is an IP literal — is not a private or loopback address unless +// allowPrivateIPs permits it. Unlike validateIssuerURL, it does not enforce +// OIDC issuer-identifier rules (no query/fragment/trailing-slash) since a +// JWKS endpoint legitimately carries those. +// +// Delegates to tokenexchange.ValidateJWKSURL, the same predicate the runtime +// choke point (ensureRegistered, called on every JWKS fetch) enforces — the two +// were previously separate implementations that had drifted apart (a +// runtime check laxer than this one would silently defeat this config-time +// guard), so this is now the single source of truth for both. +// +// Deliberately not networking.ValidateEndpointURL / +// ValidateEndpointURLWithInsecure: both also honor the +// INSECURE_DISABLE_URL_VALIDATION environment variable, which would let an +// unrelated env var silently disable this SSRF-relevant scheme check; the +// insecure variant also skips the parse/host check entirely rather than +// only relaxing the scheme. This helper takes its "insecure" bits solely +// from the issuer's own explicit InsecureAllowHTTP/AllowPrivateIPs fields. +func validateJWKSEndpointURL(rawURL string, insecureAllowHTTP, allowPrivateIPs bool) error { + return tokenexchange.ValidateJWKSURL(rawURL, insecureAllowHTTP, allowPrivateIPs) +} + +// warnTrustedIssuerAudiences logs a warning for each TrustedIssuer whose +// ExpectedAudience is absent from AllowedAudiences. This is not a hard +// error: a subject token may carry additional audiences beyond +// ExpectedAudience, so the mismatch is sufficient-but-not-necessary for +// every exchange from that issuer to fail — an operator may know a specific +// subject token will present a matching aud even though ExpectedAudience +// itself is not in AllowedAudiences. But when it's not intentional, this is +// the invalid_target footgun documented on the TrustedIssuers field: warn so +// it surfaces at startup instead of at the first exchange attempt. +func (c *Config) warnTrustedIssuerAudiences() { + for _, ti := range c.TrustedIssuers { + if !slices.Contains(c.AllowedAudiences, ti.ExpectedAudience) { + slog.Warn("trusted issuer's expected_audience is not in allowed_audiences; "+ + "token exchange will fail with invalid_target unless subject tokens from it "+ + "carry an additional audience matching one", + "issuer", ti.IssuerURL, "expected_audience", ti.ExpectedAudience) + } + } +} + // Validate checks that the OAuth2UpstreamRunConfig is internally consistent. // It enforces the mutual exclusivity of ClientID and DCRConfig: exactly one must // be set. A ClientID is required for pre-provisioned clients; a DCRConfig is @@ -1041,7 +1184,45 @@ func (c *Config) applyDefaults() error { // MUST use the "https" scheme, except for localhost during development. // When insecureAllowHTTP is true, http:// is also permitted for non-localhost // hosts (for in-cluster Kubernetes deployments on trusted networks). +// +// This server's own issuer is additionally held to a no-trailing-slash rule +// that OIDC itself does not require (see validateIssuerURLCore's +// allowTrailingSlash parameter) — defensible here only because we control +// this value, unlike a trusted external issuer (validateTrustedIssuerURL). func validateIssuerURL(issuer string, insecureAllowHTTP bool) error { + return validateIssuerURLCore(issuer, insecureAllowHTTP, true, false) +} + +// validateTrustedIssuerURL is like validateIssuerURL but never exempts +// localhost from the HTTPS requirement: a trusted external issuer is not +// this server's own issuer, so it must not inherit the same-host +// development convenience validateIssuerURL grants the server's own issuer +// and AuthorizationEndpointBaseURL. Without this, "issuer_url: +// http://localhost:9000" with insecure_allow_http: false would pass config +// validation here yet fail at runtime, since the per-issuer HTTP client is +// still built with InsecureAllowHTTP=false (see NewMultiIssuerTokenValidator) +// — jwks_url has no such exemption, so the two would otherwise disagree. +// +// Unlike validateIssuerURL, a trailing slash is accepted: OIDC Discovery §3 +// forbids query and fragment components on an issuer identifier, but not a +// trailing slash — §4.1 only requires one be trimmed before the well-known +// discovery path is appended, which presupposes a trailing-slash issuer is +// legal in the first place, and §4.3 requires the discovery document's +// "issuer" to match the token's "iss" verbatim. Microsoft Entra ID v1 — the +// default for a newly registered API — issues +// "iss": "https://sts.windows.net/{tenant}/" with a trailing slash, so +// rejecting it here would make v1 tokens impossible to configure at all. +func validateTrustedIssuerURL(issuer string, insecureAllowHTTP bool) error { + return validateIssuerURLCore(issuer, insecureAllowHTTP, false, true) +} + +// validateIssuerURLCore is the shared implementation behind validateIssuerURL +// and validateTrustedIssuerURL. localhostExempt controls whether a loopback +// host is treated as HTTPS-exempt regardless of insecureAllowHTTP. +// allowTrailingSlash controls whether a trailing slash on the issuer is +// accepted — see validateTrustedIssuerURL's doc comment for why the trusted- +// issuer path must allow it while this server's own issuer does not. +func validateIssuerURLCore(issuer string, insecureAllowHTTP, localhostExempt, allowTrailingSlash bool) error { if issuer == "" { return fmt.Errorf("issuer is required") } @@ -1066,20 +1247,34 @@ func validateIssuerURL(issuer string, insecureAllowHTTP bool) error { if parsed.Fragment != "" { return fmt.Errorf("must not contain fragment component") } + // Userinfo is rejected on two independent grounds. It cannot work: OIDC + // Discovery 1.0 Section 4.3 compares the discovery document's "issuer" + // against this value by exact string match, and no provider echoes back + // embedded credentials, so such an issuer always fails discovery. And it + // must not be stored: a password here would sit in the RunConfig and be + // echoed by the validation errors and startup warnings that quote the + // issuer URL. Rejecting it outright beats redacting it at every use. + // Note that parsed.User is non-nil even for "https://user@host" with no + // password, which is equally unusable as an issuer identifier. + if parsed.User != nil { + return fmt.Errorf("must not contain userinfo (credentials in the URL)") + } - // HTTPS is required unless it's a loopback address (for development) or - // insecureAllowHTTP is explicitly set for trusted in-cluster deployments. + // HTTPS is required unless it's a loopback address (for development, and + // only when localhostExempt) or insecureAllowHTTP is explicitly set. if parsed.Scheme != "https" { if parsed.Scheme != "http" { return fmt.Errorf("scheme must be https (or http for localhost)") } - if !networking.IsLocalhost(parsed.Host) && !insecureAllowHTTP { + if !insecureAllowHTTP && (!localhostExempt || !networking.IsLocalhost(parsed.Host)) { return fmt.Errorf("http scheme is only allowed for localhost, use https for %s", parsed.Hostname()) } } - // Issuer must not have trailing slash per OIDC spec - if strings.HasSuffix(issuer, "/") { + // Not an OIDC requirement — see validateTrustedIssuerURL's doc comment. + // ToolHive's own issuer is held to this stricter, self-imposed rule + // since we control the value; a trusted external issuer is not. + if !allowTrailingSlash && strings.HasSuffix(issuer, "/") { return fmt.Errorf("must not have trailing slash") } diff --git a/pkg/authserver/config_test.go b/pkg/authserver/config_test.go index 7487b759fb..b067163f71 100644 --- a/pkg/authserver/config_test.go +++ b/pkg/authserver/config_test.go @@ -5,6 +5,7 @@ package authserver import ( "bytes" + "log/slog" "strings" "testing" "time" @@ -14,6 +15,7 @@ import ( servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/keys" "github.com/stacklok/toolhive/pkg/authserver/server/registration" + "github.com/stacklok/toolhive/pkg/authserver/server/tokenexchange" "github.com/stacklok/toolhive/pkg/authserver/upstream" ) @@ -47,6 +49,15 @@ func TestValidateIssuerURL(t *testing.T) { {name: "missing host", issuer: "https://", wantErr: true, errMsg: "host is required"}, {name: "query component", issuer: "https://example.com?foo=bar", wantErr: true, errMsg: "must not contain query"}, {name: "fragment component", issuer: "https://example.com#section", wantErr: true, errMsg: "must not contain fragment"}, + { + name: "userinfo with password", issuer: "https://user:hunter2@example.com", + wantErr: true, errMsg: "must not contain userinfo", + }, + { + // url.Parse populates User for a bare username too. + name: "userinfo without password", issuer: "https://user@example.com", + wantErr: true, errMsg: "must not contain userinfo", + }, {name: "http non-localhost", issuer: "http://example.com", wantErr: true, errMsg: "http scheme is only allowed for localhost"}, {name: "ftp scheme", issuer: "ftp://example.com", wantErr: true, errMsg: "scheme must be https"}, {name: "trailing slash", issuer: "https://example.com/", wantErr: true, errMsg: "must not have trailing slash"}, @@ -264,6 +275,9 @@ func TestConfigApplyDefaults(t *testing.T) { func assertError(t *testing.T, err error, wantErr bool, errMsg string) { t.Helper() if wantErr { + if errMsg == "" { + t.Fatal("wantErr is true but errMsg is empty: strings.Contains(x, \"\") is always true, so this case would pass unconditionally") + } if err == nil { t.Errorf("expected error containing %q, got nil", errMsg) } else if !strings.Contains(err.Error(), errMsg) { @@ -745,3 +759,419 @@ func TestConfigApplyDefaults_DelegationTokenLifespan(t *testing.T) { }) } } + +// TestConfigValidate_TrustedIssuers covers validateTrustedIssuers as reached +// from Config.Validate: the URL-shape checks (validateTrustedIssuerURL on +// issuer_url, validateJWKSEndpointURL on jwks_url) and the structural checks +// delegated to tokenexchange.ValidateTrustedIssuers. +func TestConfigValidate_TrustedIssuers(t *testing.T) { + t.Parallel() + + // base returns a minimally-valid Config (Issuer "https://example.com", + // AllowedAudiences ["https://mcp.example.com"]) so each case isolates the + // TrustedIssuers check from unrelated validation failures. + base := func() Config { + return Config{ + Issuer: "https://example.com", + KeyProvider: keys.NewGeneratingProvider(keys.DefaultAlgorithm), + HMACSecrets: &servercrypto.HMACSecrets{Current: make([]byte, 32)}, + Upstreams: []UpstreamConfig{{ + Name: "default", + Type: UpstreamProviderTypeOAuth2, + OAuth2Config: &upstream.OAuth2Config{ + CommonOAuthConfig: upstream.CommonOAuthConfig{ClientID: "c", RedirectURI: "https://example.com/cb"}, + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + }, + }}, + AllowedAudiences: []string{"https://mcp.example.com"}, + } + } + + tests := []struct { + name string + issuers []tokenexchange.TrustedIssuer + wantErr bool + errMsg string + }{ + { + name: "no trusted issuers is byte-identical to before", + issuers: nil, + }, + { + name: "issuer_url bad scheme rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "htps://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "issuer_url", + }, + { + name: "issuer_url empty rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "issuer is required", + }, + { + name: "issuer_url http without per-issuer insecure_allow_http rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "http://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "http scheme is only allowed for localhost", + }, + { + name: "issuer_url http with per-issuer insecure_allow_http accepted", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "http://idp.example.com", ExpectedAudience: "https://mcp.example.com", InsecureAllowHTTP: true, AllowedDelegateClients: []string{"*"}}, + }, + }, + { + // Unlike Config.Issuer, a trusted issuer gets no localhost + // exemption: it isn't this server's own issuer, so the same + // same-host development convenience doesn't apply — see + // validateTrustedIssuerURL's doc comment. Without + // insecure_allow_http, http://localhost must be rejected here + // the same as any other http issuer_url. + name: "issuer_url http localhost rejected without per-issuer insecure_allow_http", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "http://localhost:8080", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "http scheme is only allowed for localhost", + }, + { + name: "issuer_url http localhost accepted with per-issuer insecure_allow_http", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "http://localhost:8080", ExpectedAudience: "https://mcp.example.com", InsecureAllowHTTP: true, AllowedDelegateClients: []string{"*"}}, + }, + }, + { + // Azure AD B2C's real jwks_uri carries a query string + // (?p=B2C_1_...); jwks_url must be validated as an ordinary + // endpoint URL, not an OIDC issuer identifier, or a legitimate + // production IdP would be rejected. + name: "jwks_url with query string accepted", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "https://idp.example.com/keys?p=B2C_1_signin", + AllowedDelegateClients: []string{"*"}, + }, + }, + }, + { + name: "jwks_url with trailing slash accepted", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "https://idp.example.com/keys/", + AllowedDelegateClients: []string{"*"}, + }, + }, + }, + { + name: "jwks_url bad scheme rejected", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "ftp://idp.example.com/keys", + AllowedDelegateClients: []string{"*"}, + }, + }, + wantErr: true, + errMsg: "jwks_url", + }, + { + name: "jwks_url http rejected without per-issuer insecure_allow_http", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "http://idp.example.com/keys", + AllowedDelegateClients: []string{"*"}, + }, + }, + wantErr: true, + errMsg: "jwks_url", + }, + { + name: "jwks_url http accepted with per-issuer insecure_allow_http", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "http://idp.example.com/keys", + InsecureAllowHTTP: true, + AllowedDelegateClients: []string{"*"}, + }, + }, + }, + { + name: "jwks_url private IP literal rejected without allow_private_ips", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "https://10.0.0.5/keys", + AllowedDelegateClients: []string{"*"}, + }, + }, + wantErr: true, + errMsg: "private or loopback", + }, + { + name: "jwks_url private IP literal accepted with allow_private_ips", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "https://10.0.0.5/keys", + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }, + }, + }, + { + // Mirrors the CRD's Kubebuilder CEL rule requiring jwksUrl + // whenever allowPrivateIPs is set (mcpexternalauthconfig_types.go): + // without a hand-configured jwks_url, OIDC discovery — a document + // fetched from, and thus influenceable by, the external issuer + // itself — would choose the private JWKS dial target. A + // hand-written RunConfig must not be able to bypass what the CRD + // path already guarantees. + name: "allow_private_ips without jwks_url rejected", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }, + }, + wantErr: true, + errMsg: "allow_private_ips requires jwks_url", + }, + { + name: "missing expected_audience rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "expected_audience is required", + }, + { + name: "issuer_url equal to Config.Issuer rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "must not equal the authorization server's own issuer", + }, + { + name: "duplicate issuer_url rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "configured more than once", + }, + { + name: "actor_claim sub rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", ActorClaim: "sub", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "actor_claim", + }, + { + // Unlike Config.Issuer, a trusted issuer_url permits a trailing + // slash: Microsoft Entra ID v1 (the default for a newly + // registered API) issues "iss": "https://sts.windows.net/{tenant}/" + // with one, and OIDC Discovery has no rule against it — see + // validateTrustedIssuerURL's doc comment. + name: "issuer_url with trailing slash accepted", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://sts.windows.net/11111111-2222-3333-4444-555555555555/", + ExpectedAudience: "https://mcp.example.com", + AllowedDelegateClients: []string{"*"}, + }, + }, + }, + { + // A may_act-only issuer is legitimate: an empty AllowedActors + // means every non-may_act token from it is rejected at + // validation time, not that the config itself is invalid. + name: "empty allowed_actors accepted", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedActors: nil, AllowedDelegateClients: []string{"*"}}, + }, + }, + { + // #5989 hardening reaches Config.Validate through the same + // shared validateTrustedIssuers -> tokenexchange.ValidateTrustedIssuers + // path as the constructor-level check. + name: "absent allowed_delegate_clients rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com"}, + }, + wantErr: true, + errMsg: "allowed_delegate_clients is required", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cfg := base() + cfg.TrustedIssuers = tt.issuers + assertError(t, cfg.Validate(), tt.wantErr, tt.errMsg) + }) + } +} + +// TestRunConfigValidate_TrustedIssuers asserts that RunConfig.Validate +// itself catches every TrustedIssuers failure mode reachable through +// validateTrustedIssuers — the four structural checks, the issuer_url shape +// check, and (mirroring TestConfigValidate_TrustedIssuers) the jwks_url +// private-IP guard — not only Config.Validate, because it's the same +// shared function both call. This matters because buildUpstreamConfigs +// performs live RFC 7591 registration against upstream IdPs before +// Config.Validate is ever reached (see the comment on RunConfig.Validate). +// A bad trusted issuer caught only at the Config layer would orphan an +// upstream client registration on every restart of the resulting crash loop. +func TestRunConfigValidate_TrustedIssuers(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + issuers []tokenexchange.TrustedIssuer + wantErr bool + errMsg string + }{ + { + name: "no trusted issuers passes", + issuers: nil, + }, + { + name: "malformed issuer_url rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "htps://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "issuer_url", + }, + { + name: "missing expected_audience rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "expected_audience is required", + }, + { + name: "issuer_url equal to RunConfig.Issuer rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "must not equal the authorization server's own issuer", + }, + { + name: "duplicate issuer_url rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "configured more than once", + }, + { + name: "actor_claim sub rejected", + issuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", ActorClaim: "sub", AllowedDelegateClients: []string{"*"}}, + }, + wantErr: true, + errMsg: "actor_claim", + }, + { + name: "jwks_url private IP literal rejected without allow_private_ips", + issuers: []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + JWKSURL: "https://10.0.0.5/keys", + AllowedDelegateClients: []string{"*"}, + }, + }, + wantErr: true, + errMsg: "private or loopback", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cfg := RunConfig{Issuer: "https://example.com", TrustedIssuers: tt.issuers} + assertError(t, cfg.Validate(), tt.wantErr, tt.errMsg) + }) + } +} + +// TestConfig_WarnTrustedIssuerAudiences pins the invalid_target footgun +// warning: it must fire at startup for an ExpectedAudience absent from +// AllowedAudiences, and stay silent when the audience is present. +// +// See TestNewEmbeddedAuthServer_TrustedIssuers in +// runner/embeddedauthserver_test.go for the same pattern with the rationale +// spelled out. +// +//nolint:paralleltest // captures the package-global slog.Default() +func TestConfig_WarnTrustedIssuerAudiences(t *testing.T) { + tests := []struct { + name string + audiences []string + wantWarn bool + }{ + { + name: "expected_audience absent from allowed_audiences warns", + audiences: []string{"https://other.example.com"}, + wantWarn: true, + }, + { + name: "expected_audience present in allowed_audiences is silent", + audiences: []string{"https://mcp.example.com"}, + wantWarn: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + cfg := Config{ + AllowedAudiences: tt.audiences, + TrustedIssuers: []tokenexchange.TrustedIssuer{ + {IssuerURL: "https://idp.example.com", ExpectedAudience: "https://mcp.example.com", AllowedDelegateClients: []string{"*"}}, + }, + } + cfg.warnTrustedIssuerAudiences() + + if tt.wantWarn { + require.Contains(t, buf.String(), "trusted issuer's expected_audience is not in allowed_audiences") + } else { + require.Empty(t, buf.String()) + } + }) + } +} diff --git a/pkg/authserver/integration_test.go b/pkg/authserver/integration_test.go index 014954ac2a..84bf0de51a 100644 --- a/pkg/authserver/integration_test.go +++ b/pkg/authserver/integration_test.go @@ -36,6 +36,7 @@ import ( "github.com/stacklok/toolhive/pkg/authserver/server/keys" "github.com/stacklok/toolhive/pkg/authserver/server/registration" "github.com/stacklok/toolhive/pkg/authserver/server/session" + "github.com/stacklok/toolhive/pkg/authserver/server/tokenexchange" "github.com/stacklok/toolhive/pkg/authserver/storage" "github.com/stacklok/toolhive/pkg/authserver/upstream" "github.com/stacklok/toolhive/pkg/oauthproto" @@ -101,6 +102,10 @@ type testServerOptions struct { // flows the default public client cannot exercise (e.g. RFC 8693 token // exchange, which requires a confidential acting client). extraClients []fosite.Client + // trustedIssuers, when non-empty, is passed through to Config.TrustedIssuers, + // enabling RFC 8693 token exchange with subject tokens from external OIDC + // issuers in addition to self-issued ones. + trustedIssuers []tokenexchange.TrustedIssuer } // testServerOption is a functional option for test server setup. @@ -144,6 +149,15 @@ func withExtraClient(c fosite.Client) testServerOption { } } +// withTrustedIssuers configures Config.TrustedIssuers, enabling token +// exchange with subject tokens from the given external OIDC issuers in +// addition to self-issued ones. +func withTrustedIssuers(issuers []tokenexchange.TrustedIssuer) testServerOption { + return func(opts *testServerOptions) { + opts.trustedIssuers = issuers + } +} + // withRedisBackedStorage swaps the default in-memory storage for a // miniredis-backed *RedisStorage. This exercises the same Lua scripts and // Redis-shape key layout used in production, while remaining hermetic and @@ -274,6 +288,7 @@ func setupTestServer(t *testing.T, opts ...testServerOption) *testServer { Upstreams: []UpstreamConfig{{Name: "default", Type: UpstreamProviderTypeOAuth2, OAuth2Config: upstreamCfg}}, UpstreamFilter: options.upstreamFilter, AllowedAudiences: []string{"https://mcp.example.com"}, + TrustedIssuers: options.trustedIssuers, } // 7. Create server using newServer with test options @@ -789,6 +804,786 @@ func TestIntegration_TokenExchange_ConfidentialClientHappyPath(t *testing.T) { "delegated token exp must be capped at the 15m delegation lifespan, not the subject token's 30m") } +// TestIntegration_TokenExchange_SelfIssuedSubjectTokenScopeFromScp proves +// that a token-exchange request carrying a requested scope succeeds against +// a subject token minted by the server's own authorization_code grant, not +// just a hand-built JWT. +// +// ToolHive's own issued access tokens carry granted scopes as a "scp" array +// claim, not the RFC 9068 "scope" string claim — fosite's default JWT claims +// strategy writes "scp" unless ScopeField is explicitly String or Both, which +// this server does not set (see TestIntegration_FullPKCEFlow's own "scp" +// assertion). Every other token-exchange test in this file hand-mints its +// subject token with an explicit "scope" claim, which masks this: assignClaim +// previously only read "scope", so a genuine self-issued access token used as +// subject_token always resolved to Scopes == "", and grantScopes rejected any +// requested scope with invalid_scope. +func TestIntegration_TokenExchange_SelfIssuedSubjectTokenScopeFromScp(t *testing.T) { + t.Parallel() + + const ( + agentClientID = "test-agent-client-scp" + agentClientSecret = "test-agent-secret-scp" + ) + + // The acting agent is also the client that logs in and obtains the + // subject token: it must be confidential (token exchange requires it) and + // registered for both authorization_code (to mint a genuine access token) + // and token-exchange (to perform the exchange as itself). + agentClient, err := registration.New(registration.Config{ + ID: agentClientID, + Secret: agentClientSecret, + Public: false, + RedirectURIs: []string{testRedirectURI}, + GrantTypes: []string{"authorization_code", oauthproto.GrantTypeTokenExchange}, + Scopes: registration.DefaultScopes, + Audience: []string{testAudience}, + }) + require.NoError(t, err) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, withExtraClient(agentClient)) + + verifier := servercrypto.GeneratePKCEVerifier() + challenge := servercrypto.ComputePKCEChallenge(verifier) + + authCode, _ := completeAuthorizationFlow(t, ts.Server.URL, authorizationParams{ + ClientID: agentClientID, + RedirectURI: testRedirectURI, + State: "scp-scope-state", + Challenge: challenge, + Scope: "openid profile", + ResponseType: "code", + }) + + // Mint the subject token via the real authorization_code grant (a + // confidential client, so client_secret is required), rather than + // hand-building a JWT — this is what makes the resulting token carry + // "scp", not "scope". + tokenResp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {"authorization_code"}, + "code": {authCode}, + "redirect_uri": {testRedirectURI}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + "code_verifier": {verifier}, + }) + defer tokenResp.Body.Close() + tokenBody := parseTokenResponse(t, tokenResp) + require.Equal(t, http.StatusOK, tokenResp.StatusCode, + "authorization_code exchange should succeed, got %d (body: %v)", tokenResp.StatusCode, tokenBody) + subjectToken, ok := tokenBody["access_token"].(string) + require.True(t, ok, "access_token should be a string") + require.NotEmpty(t, subjectToken) + + // Exchange the genuine access token for a delegated token, requesting a + // scope that was granted to it ("profile"). Before the assignClaim fix, + // this fails with invalid_scope because Scopes never picks up "scp". + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + "scope": {"profile"}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, + "token exchange requesting a scope carried by the subject token's 'scp' claim should succeed, "+ + "got %d (body: %v)", resp.StatusCode, body) + assert.Equal(t, "profile", body["scope"], "delegated token should be granted the requested scope") +} + +// ============================================================================ +// RFC 8693 Token Exchange: Trusted External Issuer Integration Tests +// ============================================================================ + +// externalIdPKeyID is the "kid" advertised in the test-local external IdP's JWKS. +const externalIdPKeyID = "external-idp-key" + +// startExternalIdPServer starts a test-local external OIDC issuer serving +// both a discovery document and a JWKS endpoint, signed by key — deliberately +// NOT the authorization server's own key. The discovery document echoes +// r.Host as its own issuer so it stays self-consistent regardless of which +// random port httptest.NewServer binds to. +// +// The returned counter increments on every discovery-document hit, so a +// subtest configuring an explicit jwks_url (which should skip discovery +// entirely) can assert it stayed at zero, and a subtest relying on discovery +// can assert it didn't. +func startExternalIdPServer(t *testing.T, key *rsa.PrivateKey) (*httptest.Server, *atomic.Int64) { + t.Helper() + + jwks := jose.JSONWebKeySet{Keys: []jose.JSONWebKey{{ + Key: key.Public(), + KeyID: externalIdPKeyID, + Algorithm: string(jose.RS256), + Use: "sig", + }}} + + var discoveryHits atomic.Int64 + + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + discoveryHits.Add(1) + base := "http://" + r.Host + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": base, + "jwks_uri": base + "/jwks", + }) + }) + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jwks) + }) + + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv, &discoveryHits +} + +// signExternalToken signs a JWT with key — the external IdP's key, distinct +// from the authorization server's own signing key — for use as a subject +// token presented during token exchange. +func signExternalToken(t *testing.T, key *rsa.PrivateKey, claims jwt.Claims, extra map[string]any) string { + t.Helper() + + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: jose.RS256, Key: key}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", externalIdPKeyID), + ) + require.NoError(t, err) + + builder := jwt.Signed(signer).Claims(claims) + if extra != nil { + builder = builder.Claims(extra) + } + raw, err := builder.Serialize() + require.NoError(t, err) + return raw +} + +// TestIntegration_TokenExchange_TrustedExternalIssuer drives RFC 8693 token +// exchange over HTTP with subject tokens from a trusted external OIDC +// issuer — proving the fail-closed consent policy (an allowlisted actor +// claim, or an authoritative may_act, or rejection) at the HTTP level, not +// just in the tokenexchange package's own unit tests. +func TestIntegration_TokenExchange_TrustedExternalIssuer(t *testing.T) { + t.Parallel() + + const ( + agentClientID = "external-agent-client" + agentClientSecret = "external-agent-secret" + allowedActor = "external-agent-azp" + externalUserSub = "external-user-sub" + ) + + // The acting agent is a confidential ToolHive client registered for the + // token-exchange grant. Its value is safe to reuse across parallel + // subtests: each subtest registers it into its own storage instance. + newAgentClient := func(t *testing.T) fosite.Client { + t.Helper() + c, err := registration.New(registration.Config{ + ID: agentClientID, + Secret: agentClientSecret, + Public: false, + GrantTypes: []string{oauthproto.GrantTypeTokenExchange}, + Scopes: registration.DefaultScopes, + Audience: []string{testAudience}, + }) + require.NoError(t, err) + return c + } + + // externalClaims returns standard claims for a subject token from the + // external IdP. The audience must equal testAudience — the server's sole + // AllowedAudience — because ensureAudienceSubsetOfSubject bounds the + // granted (default) audience by the subject token's own "aud". + externalClaims := func(issuer string) jwt.Claims { + now := time.Now() + return jwt.Claims{ + Subject: externalUserSub, + Issuer: issuer, + Audience: jwt.Audience{testAudience}, + Expiry: jwt.NewNumericDate(now.Add(30 * time.Minute)), + IssuedAt: jwt.NewNumericDate(now), + } + } + + t.Run("allowlisted actor happy path", func(t *testing.T) { + t.Parallel() + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, _ := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: testAudience, + // JWKSURL is required whenever AllowPrivateIPs is set (see + // validateTrustedIssuers in pkg/authserver/config.go): the + // private dial target must come from operator config, not an + // OIDC discovery document. idpServer is loopback, so + // AllowPrivateIPs is unavoidable here; "explicit jwks_url + // resolution path" below is the dedicated test for the + // discovery-vs-explicit distinction this used to also cover. + JWKSURL: idpServer.URL + "/jwks", + AllowedActors: []string{allowedActor}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }}), + ) + + subjectToken := signExternalToken(t, externalKey, externalClaims(idpServer.URL), map[string]any{ + "azp": allowedActor, + }) + + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, + "token exchange should succeed, got %d (body: %v)", resp.StatusCode, body) + assert.Equal(t, oauthproto.TokenTypeAccessToken, body["issued_token_type"]) + + tokenType, ok := body["token_type"].(string) + require.True(t, ok, "token_type should be a string") + assert.Equal(t, "bearer", strings.ToLower(tokenType)) + + delegated, ok := body["access_token"].(string) + require.True(t, ok, "access_token should be a string") + require.NotEmpty(t, delegated) + + parsed, err := jwt.ParseSigned(delegated, []jose.SignatureAlgorithm{jose.RS256}) + require.NoError(t, err) + var claims map[string]any + require.NoError(t, parsed.Claims(ts.PrivateKey.Public(), &claims)) + + assert.Equal(t, idpServer.URL+"#"+externalUserSub, claims["sub"], + "delegated token subject must be the external issuer's URL, qualifying the external user's "+ + "subject so it can never collide with a native ToolHive user's UUID") + assert.Equal(t, testIssuer, claims["iss"], + "delegated token must carry ToolHive's own issuer, never the external one") + + aud, ok := claims["aud"].([]interface{}) + require.True(t, ok, "aud claim should be an array") + require.Len(t, aud, 1) + assert.Equal(t, testAudience, aud[0]) + + act, ok := claims["act"].(map[string]any) + require.True(t, ok, "delegated token must carry an 'act' claim") + assert.Equal(t, agentClientID, act["sub"], "outermost act.sub must be the ToolHive acting client") + + nested, ok := act["act"].(map[string]any) + require.True(t, ok, "external delegation must nest the issuer/actor provenance record") + assert.Equal(t, allowedActor, nested["sub"], "nested act.sub is the external actor claim value") + assert.Equal(t, idpServer.URL, nested["iss"], "nested act.iss is the external issuer") + }) + + t.Run("non-allowlisted actor rejected", func(t *testing.T) { + t.Parallel() + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, _ := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: testAudience, + JWKSURL: idpServer.URL + "/jwks", + AllowedActors: []string{allowedActor}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }}), + ) + + const rejectedActor = "some-other-client" + subjectToken := signExternalToken(t, externalKey, externalClaims(idpServer.URL), map[string]any{ + "azp": rejectedActor, + }) + + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusBadRequest, resp.StatusCode, + "token exchange with a non-allowlisted actor should be a 400, got %d (body: %v)", resp.StatusCode, body) + // The validator rejects the token before checkDelegationConsent runs, so + // RFC 8693 §2.2.2's invalid_request applies — not invalid_grant. + assert.Equal(t, "invalid_request", body["error"]) + + errDesc, _ := body["error_description"].(string) + assert.Contains(t, errDesc, "invalid or could not be verified", + "the handler's fixed hint must not be replaced by a more specific — and leakier — message") + assert.NotContains(t, errDesc, rejectedActor, + "the error must not leak the rejected actor claim value") + }) + + t.Run("may_act path skips the allowlist", func(t *testing.T) { + t.Parallel() + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, _ := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: testAudience, + JWKSURL: idpServer.URL + "/jwks", + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + // AllowedActors deliberately empty: may_act must be honored + // without any actor being allowlisted. + AllowedDelegateClients: []string{"*"}, + }}), + ) + + // No "azp" claim at all — only may_act, naming the ToolHive agent + // client directly as the party authorized to act. + subjectToken := signExternalToken(t, externalKey, externalClaims(idpServer.URL), map[string]any{ + "may_act": map[string]any{"sub": agentClientID, "iss": testIssuer}, + }) + + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, + "token exchange via may_act should succeed, got %d (body: %v)", resp.StatusCode, body) + + delegated, ok := body["access_token"].(string) + require.True(t, ok) + require.NotEmpty(t, delegated) + + parsed, err := jwt.ParseSigned(delegated, []jose.SignatureAlgorithm{jose.RS256}) + require.NoError(t, err) + var claims map[string]any + require.NoError(t, parsed.Claims(ts.PrivateKey.Public(), &claims)) + + act, ok := claims["act"].(map[string]any) + require.True(t, ok) + assert.Equal(t, agentClientID, act["sub"]) + + // may_act carries no ExternalActor (see ValidatedClaims.ExternalActor's + // doc comment), but the external issuer must still be recorded — this + // is the path that bypasses the allowlist entirely, so it needs the + // audit trail at least as much as the allowlist path does. + nested, ok := act["act"].(map[string]any) + require.True(t, ok, "external issuer must still be nested even without an allowlisted actor") + assert.Equal(t, idpServer.URL, nested["iss"]) + _, hasSub := nested["sub"] + assert.False(t, hasSub, "no client-namespace actor claim exists to report on the may_act path") + }) + + t.Run("allowed delegate clients binds the allowlisted actor to a specific ToolHive client", func(t *testing.T) { + t.Parallel() + + const ( + otherAgentClientID = "other-external-agent-client" + otherAgentClientSecret = "other-external-agent-secret" + ) + + // A second confidential client, also registered for the token-exchange + // grant and otherwise identical to the primary agent client — the only + // difference the test exercises is that it is NOT in this issuer's + // AllowedDelegateClients. Without that field (or if the binding check + // were removed), this client would succeed exactly like the primary + // one, since both hold the grant and both present the same + // allowlisted external actor claim. + otherAgentClient, err := registration.New(registration.Config{ + ID: otherAgentClientID, + Secret: otherAgentClientSecret, + Public: false, + GrantTypes: []string{oauthproto.GrantTypeTokenExchange}, + Scopes: registration.DefaultScopes, + Audience: []string{testAudience}, + }) + require.NoError(t, err) + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, _ := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withExtraClient(otherAgentClient), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: testAudience, + JWKSURL: idpServer.URL + "/jwks", + AllowedActors: []string{allowedActor}, + AllowedDelegateClients: []string{agentClientID}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + }}), + ) + + subjectToken := signExternalToken(t, externalKey, externalClaims(idpServer.URL), map[string]any{ + "azp": allowedActor, + }) + + exchange := func(clientID, clientSecret string) *http.Response { + return makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {clientID}, + "client_secret": {clientSecret}, + }) + } + + permittedResp := exchange(agentClientID, agentClientSecret) + defer permittedResp.Body.Close() + permittedBody := parseTokenResponse(t, permittedResp) + require.Equal(t, http.StatusOK, permittedResp.StatusCode, + "the allowlisted delegate client should succeed, got %d (body: %v)", + permittedResp.StatusCode, permittedBody) + + rejectedResp := exchange(otherAgentClientID, otherAgentClientSecret) + defer rejectedResp.Body.Close() + rejectedBody := parseTokenResponse(t, rejectedResp) + require.Equal(t, http.StatusBadRequest, rejectedResp.StatusCode, + "a client absent from AllowedDelegateClients should be a 400, got %d (body: %v)", + rejectedResp.StatusCode, rejectedBody) + assert.Equal(t, "invalid_grant", rejectedBody["error"]) + errDesc, _ := rejectedBody["error_description"].(string) + assert.Contains(t, errDesc, "not authorized to exchange subject tokens") + }) + + t.Run("untrusted issuer rejected before any JWKS fetch", func(t *testing.T) { + t.Parallel() + + const untrustedIssuer = "https://untrusted-issuer.example.com" + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, discoveryHits := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + // The sole registered trusted issuer is idpServer.URL — a live, + // fetchable JWKS signed with the SAME key the subject token below + // uses. This makes the rejection discriminating rather than + // coincidental: the subject token's signature WOULD verify + // successfully against this real JWKS if the issuer-map lookup were + // ever bypassed (a fallback to the sole configured issuer, a wildcard + // match, or deleted "iss"-based routing) — so a 400 here can only be + // explained by the "iss" string itself failing to match idpServer.URL, + // not by an unverifiable signature or a validator that was never + // constructed in the first place. + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: testAudience, + JWKSURL: idpServer.URL + "/jwks", + AllowedActors: []string{allowedActor}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }}), + ) + + // Signed with idpServer's real key, but claims an issuer that was + // never registered in TrustedIssuers. + subjectToken := signExternalToken(t, externalKey, externalClaims(untrustedIssuer), map[string]any{ + "azp": allowedActor, + }) + + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusBadRequest, resp.StatusCode, + "token exchange from an untrusted issuer should be a 400, got %d (body: %v)", resp.StatusCode, body) + assert.Equal(t, "invalid_request", body["error"]) + + errDesc, _ := body["error_description"].(string) + assert.Contains(t, errDesc, "invalid or could not be verified", + "the handler's fixed hint must not be replaced by a more specific — and leakier — message") + assert.NotContains(t, errDesc, untrustedIssuer, + "the error must not leak the untrusted issuer URL") + + // The issuer-map miss must short-circuit before any JWKS fetch. + // Note: JWKSURL is now preconfigured above (see comment on the + // "allowlisted actor happy path" subtest), which already skips + // discovery on its own — so this assertion holding is necessary but + // not on its own sufficient proof of the short-circuit; the 400 + // response and error content below are the discriminating checks. + assert.Zero(t, discoveryHits.Load(), "issuer-map miss must precede any JWKS discovery fetch") + }) + + t.Run("self-issued subject token still works alongside trusted issuers", func(t *testing.T) { + t.Parallel() + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, _ := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: testAudience, + JWKSURL: idpServer.URL + "/jwks", + AllowedActors: []string{allowedActor}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }}), + ) + + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: jose.RS256, Key: ts.PrivateKey}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "test-key"), + ) + require.NoError(t, err) + + now := time.Now() + subjectToken, err := jwt.Signed(signer). + Claims(jwt.Claims{ + Issuer: testIssuer, + Subject: "self-issued-delegated-user", + Audience: jwt.Audience{testAudience}, + Expiry: jwt.NewNumericDate(now.Add(30 * time.Minute)), + IssuedAt: jwt.NewNumericDate(now), + }). + Claims(map[string]any{"client_id": agentClientID}). + Serialize() + require.NoError(t, err) + + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, + "self-issued token exchange should still succeed with trusted issuers configured, "+ + "got %d (body: %v)", resp.StatusCode, body) + assert.Equal(t, oauthproto.TokenTypeAccessToken, body["issued_token_type"]) + }) + + t.Run("explicit jwks_url resolution path", func(t *testing.T) { + t.Parallel() + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, discoveryHits := startExternalIdPServer(t, externalKey) + + // Prove the counter actually increments before relying on its + // zero-ness below — otherwise a discovery handler that silently + // stopped counting would make the "must skip discovery" assertion + // vacuously true. + discResp, err := http.Get(idpServer.URL + "/.well-known/openid-configuration") //nolint:noctx + require.NoError(t, err) + discResp.Body.Close() + require.Equal(t, int64(1), discoveryHits.Load(), "discovery counter must be live") + discoveryHits.Store(0) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + // Set directly rather than relying on discovery: this exercises + // ensureRegistered's pre-configured-URL branch instead of + // discoverJWKSURL. + JWKSURL: idpServer.URL + "/jwks", + ExpectedAudience: testAudience, + AllowedActors: []string{allowedActor}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }}), + ) + + subjectToken := signExternalToken(t, externalKey, externalClaims(idpServer.URL), map[string]any{ + "azp": allowedActor, + }) + + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, + "token exchange with an explicit jwks_url should succeed, got %d (body: %v)", resp.StatusCode, body) + assert.Equal(t, oauthproto.TokenTypeAccessToken, body["issued_token_type"]) + + assert.Zero(t, discoveryHits.Load(), + "a preconfigured jwks_url must skip OIDC discovery entirely") + }) + + t.Run("external token's exp bounds the delegated token's lifetime", func(t *testing.T) { + t.Parallel() + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, _ := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: testAudience, + JWKSURL: idpServer.URL + "/jwks", + AllowedActors: []string{allowedActor}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }}), + ) + + // 5m remaining lifetime, well under the 15m default delegation + // lifespan — so the delegated token's exp must track the subject + // token, not the (longer) configured cap. + now := time.Now() + subjClaims := jwt.Claims{ + Subject: externalUserSub, + Issuer: idpServer.URL, + Audience: jwt.Audience{testAudience}, + Expiry: jwt.NewNumericDate(now.Add(5 * time.Minute)), + IssuedAt: jwt.NewNumericDate(now), + } + subjectToken := signExternalToken(t, externalKey, subjClaims, map[string]any{"azp": allowedActor}) + + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, + "token exchange should succeed, got %d (body: %v)", resp.StatusCode, body) + + delegated, ok := body["access_token"].(string) + require.True(t, ok) + require.NotEmpty(t, delegated) + + parsed, err := jwt.ParseSigned(delegated, []jose.SignatureAlgorithm{jose.RS256}) + require.NoError(t, err) + var claims map[string]any + require.NoError(t, parsed.Claims(ts.PrivateKey.Public(), &claims)) + + exp, ok := claims["exp"].(float64) + require.True(t, ok, "exp claim should be a number") + assert.WithinDuration(t, now.Add(5*time.Minute), time.Unix(int64(exp), 0), 2*time.Minute, + "delegated token exp must track the external subject token's 5m remaining lifetime, "+ + "not the 15m default delegation lifespan") + }) + + t.Run("invalid_target when the external aud isn't a ToolHive-allowed audience", func(t *testing.T) { + t.Parallel() + + // A realistic Entra-style app-ID audience: legitimate as the trusted + // issuer's own ExpectedAudience, but not one of ToolHive's + // AllowedAudiences — the footgun documented on RunConfig.TrustedIssuers. + const foreignAudience = "api://some-app-id" + + externalKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + idpServer, _ := startExternalIdPServer(t, externalKey) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, + withExtraClient(newAgentClient(t)), + withTrustedIssuers([]tokenexchange.TrustedIssuer{{ + IssuerURL: idpServer.URL, + ExpectedAudience: foreignAudience, + JWKSURL: idpServer.URL + "/jwks", + AllowedActors: []string{allowedActor}, + InsecureAllowHTTP: true, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{"*"}, + }}), + ) + + now := time.Now() + subjClaims := jwt.Claims{ + Subject: externalUserSub, + Issuer: idpServer.URL, + Audience: jwt.Audience{foreignAudience}, + Expiry: jwt.NewNumericDate(now.Add(30 * time.Minute)), + IssuedAt: jwt.NewNumericDate(now), + } + subjectToken := signExternalToken(t, externalKey, subjClaims, map[string]any{"azp": allowedActor}) + + // No resource/audience requested: the handler defaults to ToolHive's + // sole AllowedAudience (testAudience), which this subject token's aud + // does not cover. + resp := makeTokenRequest(t, ts.Server.URL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {agentClientID}, + "client_secret": {agentClientSecret}, + }) + defer resp.Body.Close() + + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusBadRequest, resp.StatusCode, + "an external aud outside ToolHive's AllowedAudiences must be rejected, got %d (body: %v)", + resp.StatusCode, body) + assert.Equal(t, "invalid_target", body["error"]) + errDesc, _ := body["error_description"].(string) + assert.Contains(t, errDesc, "not covered by the subject token") + }) +} + // ============================================================================ // Full PKCE Flow Integration Tests with Mock Upstream IDP (using mockoidc) // ============================================================================ diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 680fbde42c..f765b5a936 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -241,6 +241,11 @@ func NewEmbeddedAuthServerWithStorage( CIMDCacheMaxSize: cimdCacheMaxSize, CIMDCacheFallbackTTL: cimdCacheFallbackTTL, InsecureAllowHTTP: cfg.InsecureAllowHTTP, + // slices.Clone is shallow: each TrustedIssuer's own AllowedActors + // slice is still shared with cfg. NewMultiIssuerTokenValidator's + // constructor clones AllowedActors per issuer before use, so the + // authorization-critical data is protected without a deep copy here. + TrustedIssuers: slices.Clone(cfg.TrustedIssuers), } // 8. Create the auth server. authserver.New also asserts the DCR diff --git a/pkg/authserver/runner/embeddedauthserver_test.go b/pkg/authserver/runner/embeddedauthserver_test.go index 9b453505ea..ba2395c499 100644 --- a/pkg/authserver/runner/embeddedauthserver_test.go +++ b/pkg/authserver/runner/embeddedauthserver_test.go @@ -19,6 +19,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "sync" "sync/atomic" "testing" "time" @@ -29,6 +30,7 @@ import ( "github.com/stacklok/toolhive/pkg/authserver" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/keys" + "github.com/stacklok/toolhive/pkg/authserver/server/tokenexchange" "github.com/stacklok/toolhive/pkg/authserver/storage" "github.com/stacklok/toolhive/pkg/oauthproto" ) @@ -2083,6 +2085,32 @@ func (s *urlErrorOnCloseStorage) Close() error { return s.closeErr } +// syncBuffer is a concurrency-safe io.Writer over a bytes.Buffer, used by the +// slog-capturing tests below. slog.SetDefault is process-global, so while a +// capturing handler is installed, any goroutine in the test binary — not just +// the capturing test's own subtests — can write a record into it; a plain +// bytes.Buffer is a data race waiting for some concurrently running test to +// log, which is exactly what it became once a parallel parent was added. +// Mirrors the identically-named helper in pkg/authz/authorizers/cedar/ +// core_test.go; kept as a separate copy since it is an unexported test type +// and not worth exporting across packages. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + // TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog pins the post-#5196 // invariant that the deferred-cleanup slog.Warn at the top of // NewEmbeddedAuthServerWithStorage routes both closeErr and retErr through @@ -2098,11 +2126,11 @@ func (s *urlErrorOnCloseStorage) Close() error { // - the captured log record DOES contain the host components, so // operators retain enough context to correlate the failure. // -// NOT t.Parallel(): the test swaps slog.Default() to capture output and -// restores it via t.Cleanup. Running in parallel would race with any other -// test in this package that emits a log record. Confirmed against the -// paralleltest rule on a sample run — every other test failed with a -// data-race report on slog's internal default-logger handle. +// NOT t.Parallel(): the test swaps slog.Default() and restores it via +// t.Cleanup. The capturing buffer is a syncBuffer, so a concurrent writer is +// not itself a data race — the reason to stay sequential is the swap/restore +// pair, which two overlapping capturing tests would interleave, leaving +// whichever restored last as the default and clobbering the other's capture. // //nolint:paralleltest // see comment above; mutates the package-global slog.Default() func TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog(t *testing.T) { @@ -2155,7 +2183,7 @@ func TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog(t *testing.T) { // Capture slog output by swapping the default logger for the duration // of this test. Restore on cleanup so parallel tests are unaffected. - var buf bytes.Buffer + var buf syncBuffer prev := slog.Default() slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) t.Cleanup(func() { slog.SetDefault(prev) }) @@ -2181,6 +2209,104 @@ func TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog(t *testing.T) { "closeErr host must remain in the Warn record after sanitisation") } +// TestNewEmbeddedAuthServer_TrustedIssuers pins the RunConfig.TrustedIssuers +// -> Config.TrustedIssuers conversion (embeddedauthserver.go's resolvedCfg, +// ~line 244). "Construction succeeds" alone doesn't discriminate: deleting +// the resolvedCfg.TrustedIssuers assignment entirely still builds a server +// (it degrades to the empty-TrustedIssuers case, which is itself valid), so +// the no-trusted-issuers control subtest can't disagree with a broken one. +// +// Instead this uses an observable side effect that only fires if the field +// actually reached the Factory closure and NewMultiIssuerTokenValidator's +// constructor (not merely RunConfig.Validate/Config.Validate, both of which +// run before resolvedCfg is built): NewMultiIssuerTokenValidator logs a +// slog.Warn ("Trusted issuer has no allowed actors configured") for each +// TrustedIssuer with an empty AllowedActors. Confirmed by mutation: deleting +// `TrustedIssuers: slices.Clone(cfg.TrustedIssuers)` from resolvedCfg's +// construction makes this test fail (the warning never fires because +// MultiIssuerTokenValidator is never even built — Factory falls back to the +// self-issued validator). +// +// What this does NOT cover: resolvedCfg has no exported accessor, so the +// mutate-the-caller's-slice-after-construction claim (slices.Clone protects +// the outer []TrustedIssuer from a later append/removal on cfg, but is +// shallow — a mutation reaching through a still-shared AllowedActors slice +// would not be caught by this outer clone) is not observable through this +// package's public API. Proving it would require a live token-exchange HTTP +// round trip against an external issuer (Step 7's HTTP-level integration +// tests), or a production-code accessor this task's scope excludes. +// NewMultiIssuerTokenValidator's own AllowedActors clone (see +// multi_issuer_validator.go) closes that gap at the layer where it actually +// matters for concurrent request handling. +func TestNewEmbeddedAuthServer_TrustedIssuers(t *testing.T) { + t.Parallel() + + base := func() *authserver.RunConfig { + return &authserver.RunConfig{ + SchemaVersion: authserver.CurrentSchemaVersion, + Issuer: "https://auth.example.com", + Upstreams: []authserver.UpstreamRunConfig{ + { + Name: "test-upstream", + Type: authserver.UpstreamProviderTypeOAuth2, + OAuth2Config: &authserver.OAuth2UpstreamRunConfig{ + AuthorizationEndpoint: "https://example.com/authorize", + TokenEndpoint: "https://example.com/token", + ClientID: "test-client-id", + RedirectURI: "https://auth.example.com/oauth/callback", + }, + }, + }, + AllowedAudiences: []string{"https://mcp.example.com"}, + } + } + + t.Run("no trusted issuers builds server (control case)", func(t *testing.T) { + t.Parallel() + + cfg := base() + + srv, err := NewEmbeddedAuthServer(context.Background(), cfg) + require.NoError(t, err) + require.NotNil(t, srv) + require.NoError(t, srv.Close()) + }) + + // NOT t.Parallel(): captures the package-global slog.Default() via + // slog.SetDefault, which is atomic, and restores it via t.Cleanup. + // The buffer itself is the only unsynchronized object in that swap, and + // it's mutex-guarded (syncBuffer), so records emitted by any other test + // running concurrently in this parallel phase land harmlessly. That's + // fine here because the assertion below is Contains-only, which + // tolerates foreign records mixed into the buffer. + //nolint:paralleltest // see comment above + t.Run("valid trusted issuers reach the Factory closure and validator constructor", func(t *testing.T) { + cfg := base() + cfg.TrustedIssuers = []tokenexchange.TrustedIssuer{ + { + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + // AllowedActors intentionally empty: this is what makes + // NewMultiIssuerTokenValidator log its warning, the + // observable proof this test relies on. + AllowedDelegateClients: []string{"*"}, + }, + } + + var buf syncBuffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + srv, err := NewEmbeddedAuthServer(context.Background(), cfg) + require.NoError(t, err) + require.NotNil(t, srv) + t.Cleanup(func() { _ = srv.Close() }) + + assert.Contains(t, buf.String(), "Trusted issuer has no allowed actors configured") + }) +} + func TestResolveCIMDConfig(t *testing.T) { t.Parallel() diff --git a/pkg/authserver/server/tokenexchange/factory.go b/pkg/authserver/server/tokenexchange/factory.go index 30cccbd6dc..99c3f58cac 100644 --- a/pkg/authserver/server/tokenexchange/factory.go +++ b/pkg/authserver/server/tokenexchange/factory.go @@ -19,17 +19,39 @@ import ( // Returns an error if delegationLifespan is not in (0, server.MaxAccessTokenLifespan]: a zero // or negative value would produce delegated tokens with an expiry already in the past, and a // value above the access token ceiling would only be caught at request time by the per-request cap. -func Factory(delegationLifespan time.Duration) (server.Factory, error) { +// +// When trustedIssuers is non-empty, subject tokens are validated by a +// MultiIssuerTokenValidator wrapping the self-issued validator; otherwise the +// self-issued validator is used directly, preserving prior behavior exactly. +// Each TrustedIssuer carries its own InsecureAllowHTTP/AllowPrivateIPs (see +// NewMultiIssuerTokenValidator) — this Factory takes no validator-wide +// equivalent, so a self-issuer setting can never reach the external path +// through here. +func Factory(delegationLifespan time.Duration, trustedIssuers []TrustedIssuer) (server.Factory, error) { if delegationLifespan <= 0 || delegationLifespan > server.MaxAccessTokenLifespan { return nil, fmt.Errorf("tokenexchange: delegationLifespan must be between %v and %v, got %v", time.Duration(0), server.MaxAccessTokenLifespan, delegationLifespan) } return func(config *server.AuthorizationServerConfig, storage fosite.Storage, strategy any) (any, error) { - validator, err := NewSelfIssuedTokenValidator(config.PublicJWKS(), config.GetAccessTokenIssuer(), config.AllowedAudiences) + selfValidator, err := NewSelfIssuedTokenValidator(config.PublicJWKS(), config.GetAccessTokenIssuer(), config.AllowedAudiences) if err != nil { return nil, fmt.Errorf("tokenexchange: failed to create subject token validator: %w", err) } + // IIFE keeps validator a single immutable assignment rather than a + // mutable var reassigned across branches (go-style): reassigning it + // in place risked ending up with a non-nil SubjectTokenValidator + // wrapping a nil *MultiIssuerTokenValidator on the error path. + validator, err := func() (SubjectTokenValidator, error) { + if len(trustedIssuers) == 0 { + return selfValidator, nil + } + return NewMultiIssuerTokenValidator(selfValidator, config.GetAccessTokenIssuer(), trustedIssuers) + }() + if err != nil { + return nil, fmt.Errorf("tokenexchange: trusted_issuers: %w", err) + } + // Use the embedded *fosite.Config for HandleHelper and handlerConfig // because AuthorizationServerConfig shadows GetAccessTokenLifespan() without // a context parameter, which doesn't satisfy fosite's provider interfaces. diff --git a/pkg/authserver/server/tokenexchange/factory_test.go b/pkg/authserver/server/tokenexchange/factory_test.go index 41d71d60b0..bd3c75f28d 100644 --- a/pkg/authserver/server/tokenexchange/factory_test.go +++ b/pkg/authserver/server/tokenexchange/factory_test.go @@ -4,13 +4,18 @@ package tokenexchange import ( + "context" + "crypto/rand" + "crypto/rsa" "testing" "time" + "github.com/ory/fosite" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stacklok/toolhive/pkg/authserver/server" + servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" ) func TestFactory(t *testing.T) { @@ -55,7 +60,7 @@ func TestFactory(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - f, err := Factory(tt.delegationLifespan) + f, err := Factory(tt.delegationLifespan, nil) if tt.wantErr { require.Error(t, err) assert.Contains(t, err.Error(), "delegationLifespan must be between") @@ -67,3 +72,121 @@ func TestFactory(t *testing.T) { }) } } + +// buildTestAuthServerConfig returns a minimally-valid +// *server.AuthorizationServerConfig for exercising the closure Factory +// returns. AllowedAudiences is non-empty so it doubles as a valid +// ExpectedAudience target for a TrustedIssuer in the tests below. +func buildTestAuthServerConfig(t *testing.T) *server.AuthorizationServerConfig { + t.Helper() + + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + cfg, err := server.NewAuthorizationServerConfig(&server.AuthorizationServerParams{ + Issuer: "https://auth.example.com", + AccessTokenLifespan: time.Hour, + RefreshTokenLifespan: time.Hour * 24, + AuthCodeLifespan: time.Minute * 10, + HMACSecrets: servercrypto.NewHMACSecrets([]byte("test-secret-with-32-bytes-long!!")), + SigningKeyID: "key-1", + SigningKeyAlgorithm: "RS256", + SigningKey: rsaKey, + AllowedAudiences: []string{"https://mcp.example.com"}, + }) + require.NoError(t, err) + return cfg +} + +// fakeClientManager is a no-op fosite.ClientManager (the entirety of +// fosite.Storage) so fakeFactoryStorage satisfies the Factory closure's +// storage fosite.Storage parameter without needing a real client store — +// the closure only type-asserts storage to oauth2.AccessTokenStorage, it +// never calls a ClientManager method. +type fakeClientManager struct{} + +func (fakeClientManager) GetClient(context.Context, string) (fosite.Client, error) { + return nil, fosite.ErrNotFound +} +func (fakeClientManager) ClientAssertionJWTValid(context.Context, string) error { return nil } +func (fakeClientManager) SetClientAssertionJWT(context.Context, string, time.Time) error { + return nil +} + +// fakeFactoryStorage combines the no-op ClientManager with the package's +// existing mockAccessTokenStorage so it satisfies both fosite.Storage (the +// closure's declared parameter type) and oauth2.AccessTokenStorage (what the +// closure actually type-asserts against). +type fakeFactoryStorage struct { + fakeClientManager + *mockAccessTokenStorage +} + +// TestFactory_ValidatorSelection asserts which SubjectTokenValidator the +// closure returned by Factory builds into the Handler: the self-issued +// validator when trustedIssuers is empty, the multi-issuer validator when +// it isn't, and a hard error — not a silent downgrade to the self-issued +// validator — when a configured TrustedIssuer is itself invalid. +func TestFactory_ValidatorSelection(t *testing.T) { + t.Parallel() + + validIssuer := TrustedIssuer{ + IssuerURL: "https://idp.example.com", + ExpectedAudience: "https://mcp.example.com", + AllowedDelegateClients: []string{anyDelegateClient}, + } + invalidIssuer := TrustedIssuer{ + IssuerURL: "https://idp.example.com", + // ExpectedAudience deliberately empty: invalid per validateTrustedIssuer. + AllowedDelegateClients: []string{anyDelegateClient}, + } + + tests := []struct { + name string + trustedIssuers []TrustedIssuer + wantErr string + wantValidator any // nil when wantErr is set + }{ + { + name: "no trusted issuers builds self-issued validator", + trustedIssuers: nil, + wantValidator: &SelfIssuedTokenValidator{}, + }, + { + name: "valid trusted issuer builds multi-issuer validator", + trustedIssuers: []TrustedIssuer{validIssuer}, + wantValidator: &MultiIssuerTokenValidator{}, + }, + { + name: "invalid trusted issuer fails closed, not silently downgraded", + trustedIssuers: []TrustedIssuer{invalidIssuer}, + wantErr: "trusted_issuers", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + f, err := Factory(15*time.Minute, tt.trustedIssuers) + require.NoError(t, err) + + cfg := buildTestAuthServerConfig(t) + storage := &fakeFactoryStorage{mockAccessTokenStorage: &mockAccessTokenStorage{}} + strategy := &mockAccessTokenStrategy{} + + result, err := f(cfg, storage, strategy) + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Nil(t, result) + return + } + require.NoError(t, err) + + handler, ok := result.(*Handler) + require.True(t, ok, "Factory closure must return *Handler, got %T", result) + assert.IsType(t, tt.wantValidator, handler.validator) + }) + } +} diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index 951f319f4d..304fa9eea7 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -8,6 +8,7 @@ import ( "fmt" "log/slog" "net/url" + "slices" "strings" "time" @@ -31,6 +32,15 @@ var _ fosite.TokenEndpointHandler = (*Handler)(nil) // parse it. const maxDelegationDepth = 10 +// anyDelegateClient is the TrustedIssuer.AllowedDelegateClients entry that +// explicitly opts an issuer into "any ToolHive confidential client holding +// the token-exchange grant may act as its delegate" — the behavior that used +// to be the implicit default whenever the field was left empty. See +// checkDelegationConsent's doc comment for why that default was removed: +// permissiveness must now be declared with this wildcard, not left to an +// absent field. +const anyDelegateClient = "*" + // Handler implements RFC 8693 token exchange for user-to-agent delegation. // // When an authenticated OAuth client (the acting agent) presents a user's JWT @@ -116,12 +126,15 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi "error", err, "actor", actorID, ) - // TODO(#5989): this maps every validation failure to invalid_request, - // which is correct for a malformed/unverifiable subject token. Once the - // multi-issuer validator is wired in, grant-level failures reachable only - // on the external path (untrusted issuer, wrong audience, expired) should - // map to invalid_grant per RFC 6749 §5.2; that needs typed validator - // errors the handler can distinguish. + // RFC 8693 §2.2.2 mandates invalid_request here: "if either the + // subject_token or actor_token are invalid for any reason, or are + // unacceptable based on policy ... The value of the error parameter + // MUST be the invalid_request error code." checkDelegationConsent + // below deliberately returns invalid_grant instead for its own + // failures — a documented deviation: RFC 6749 §5.2's invalid_grant + // covers "was issued to another client", §2.2.2 permits other error + // codes for other failures, and both Keycloak and Hydra follow this + // same split. return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( "The subject token is invalid or could not be verified.")) } @@ -140,7 +153,7 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi // Build the delegated session with the user's identity and the agent's act claim. delegatedSession := session.New( - validatedClaims.Subject, + delegatedSubject(validatedClaims), "", // No IDP session link for delegated tokens. actorID, session.UserClaims{ @@ -149,45 +162,9 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi }, ) - // Add the RFC 8693 Section 4.1 "act" claim identifying the acting party. - // If the subject token itself carries an "act" claim (i.e. it is a - // previously-delegated token being re-exchanged), nest it so the full - // delegation chain remains auditable rather than being discarded. - act := map[string]any{"sub": actorID} - if priorAct, ok := validatedClaims.Extra["act"]; ok && priorAct != nil { - // Parse with the shared audit-side parser rather than a bespoke walker: it - // reports both the chain depth and any RFC 8693 Section 4.1 conformance - // violation, so a non-object act cannot slip past the depth gate and be - // re-minted. A JSON-null act is filtered by the guard above: it asserts no - // delegation and must not read as malformed. - chain := coreaudit.ParseDelegationChain(priorAct, maxDelegationDepth) - if chain.Malformed { - // RFC 8693 Section 2.2.2 nominally calls for invalid_request on a bad - // subject_token, but also allows that "other error codes may also be - // used, as appropriate": invalid_grant keeps this consistent with the - // depth, consent, and expiry gates in this same handler, all of which - // reject a structurally unacceptable subject token that way. - // - // MalformedReason is a closed, value-free enum; it is surfaced to the - // client and MUST stay that way — never interpolate claim contents here. - return errorsx.WithStack(fosite.ErrInvalidGrant.WithHintf( - "The subject token's delegation chain is malformed (%s).", chain.MalformedReason)) - } - // Equivalent to the depth of the prior chain: the parser appends exactly one - // hop per level and stops at maxDelegationDepth, so len(Chain) is - // min(depth, maxDelegationDepth). - if len(chain.Chain) >= maxDelegationDepth { - return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( - "The subject token's delegation chain is too deep.")) - } - // Nest priorAct verbatim rather than re-serializing from chain: core keeps - // each hop's extra claims in an unexported map, so rebuilding would silently - // drop the history trail that Section 4.1 asks us to preserve. The cost is - // that unknown hop claims — and the non-identity claims Section 4.1 calls - // "not meaningful" inside act — pass through re-signed, which sits in - // tension with Section 6 data minimization. Accepted here; bounding the - // subtree belongs with the external-issuer work in #5989. - act["act"] = priorAct + act, err := buildActClaim(validatedClaims, actorID) + if err != nil { + return err } delegatedSession.JWTClaims.Extra["act"] = act @@ -205,6 +182,8 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi slog.Debug("Token exchange request validated", "subject", validatedClaims.Subject, "actor", actorID, + "issuer", validatedClaims.Issuer, + "subject_token_client", validatedClaims.ExternalActor, "lifetime", lifetime.String(), ) @@ -313,19 +292,202 @@ func validateExchangeParams(form url.Values) (string, error) { return subjectToken, nil } +// delegatedSubject returns the "sub" to embed in the delegated token. +// +// Self-issued tokens pass through unchanged: their subject is already a +// UUID from UserResolver.ResolveUser's (providerID, providerSubject) +// mapping (pkg/authserver/server/handlers/user.go), ToolHive's own native +// subject namespace. +// +// A subject token from a trusted external issuer is qualified as +// "#" instead, because the issued token's "sub" is the only +// signal Cedar's extractClientIDFromClaims (pkg/authz/authorizers/cedar/core.go) +// keys on — it reads "sub" alone, not "iss" — and the external issuer +// chooses that value, not ToolHive. Without qualification, an external +// issuer could pick a "sub" equal to a native user's UUID and mint a +// delegated token indistinguishable from that user's, entirely without +// malicious intent on either issuer's part: two trusted issuers merely +// choosing overlapping subject values collide with no attacker involved. +// +// "#" is an unambiguous separator: an issuer URL cannot itself contain a +// fragment (the OIDC/OAuth "iss" value is a URL without a fragment +// component, and this validator's own issuer comparison is an exact string +// match against that value), so strings.Cut(sub, "#") on the qualified +// subject always recovers (issuerURL, externalSub) uniquely. A native UUID +// can never collide with a qualified value, since a UUID contains no "#". +// +// Re-exchanging a previously-issued delegated token does not double-qualify: +// such a token's "iss" is always this server's own issuer (fosite stamps +// "iss" from the server's config at issuance — see session.New's doc +// comment), so Validate routes it to the self-issued path, which never +// calls this branch. +func delegatedSubject(validatedClaims *ValidatedClaims) string { + if validatedClaims.ExternalIssuer == "" { + return validatedClaims.Subject + } + return validatedClaims.ExternalIssuer + "#" + validatedClaims.Subject +} + +// buildActClaim assembles the RFC 8693 Section 4.1 "act" claim for the +// delegated token. The outermost act.sub is always actorID (the ToolHive +// client) — every downstream consumer reads that as "who is acting", and it +// must not change regardless of how the subject token was obtained. +// +// Extracted from HandleTokenEndpointRequest rather than inlined: the external +// provenance nesting and the prior-chain depth gate below add five branches +// that have nothing to do with the surrounding request plumbing, and two of +// them reject the request outright. +func buildActClaim(validatedClaims *ValidatedClaims, actorID string) (map[string]any, error) { + act := map[string]any{"sub": actorID} + nestUnder := act + newLevels := 1 + + // When the subject token came from a trusted external issuer + // (ValidatedClaims.ExternalIssuer is set only there — see + // multi_issuer_validator.go), nest that issuer one level in, together + // with the allowlisted actor when one was resolved. RFC 8693 §4.1 + // anticipates exactly this: "the combination of the two claims 'iss' and + // 'sub' might be necessary to uniquely identify an actor." Without this, + // the issued token would carry no record that the delegation originated + // externally at all — including for a may_act-bearing external token, + // which leaves ExternalActor unset because may_act.sub already names the + // delegate directly via the actorID binding above. That token still has + // an external issuer worth recording, so nesting here is keyed on + // ExternalIssuer, not ExternalActor: the allowlist path's accepted + // any-ToolHive-client scope limitation (see checkDelegationConsent) + // depends on this provenance being auditable after the fact, and a + // may_act-bearing external token — the path that bypasses the allowlist + // entirely — needs it at least as much. + if validatedClaims.ExternalIssuer != "" { + external := map[string]any{"iss": validatedClaims.ExternalIssuer} + // ExternalActor is only present on the allowlist path (see its doc + // comment) — a may_act-bearing external token has no client-namespace + // actor claim to report, so the nested entry there carries "iss" only. + if validatedClaims.ExternalActor != "" { + external["sub"] = validatedClaims.ExternalActor + } + act["act"] = external + nestUnder = external + newLevels = 2 + } + + // If the subject token itself carries an "act" claim (i.e. it is a + // previously-delegated token being re-exchanged), nest it so the full + // delegation chain remains auditable rather than being discarded. + // newLevels accounts for the external wrapper above, if any, so the + // resulting chain never exceeds maxDelegationDepth regardless of how + // many levels this exchange itself adds. + if priorAct, ok := validatedClaims.Extra["act"]; ok && priorAct != nil { + // Parse with the shared audit-side parser rather than a bespoke walker: it + // reports both the chain depth and any RFC 8693 Section 4.1 conformance + // violation, so a non-object act cannot slip past the depth gate and be + // re-minted. A JSON-null act is filtered by the guard above: it asserts no + // delegation and must not read as malformed. + chain := coreaudit.ParseDelegationChain(priorAct, maxDelegationDepth) + if chain.Malformed { + // RFC 8693 Section 2.2.2 nominally calls for invalid_request on a bad + // subject_token, but also allows that "other error codes may also be + // used, as appropriate": invalid_grant keeps this consistent with the + // depth, consent, and expiry gates in this same handler, all of which + // reject a structurally unacceptable subject token that way. + // + // MalformedReason is a closed, value-free enum; it is surfaced to the + // client and MUST stay that way — never interpolate claim contents here. + return nil, errorsx.WithStack(fosite.ErrInvalidGrant.WithHintf( + "The subject token's delegation chain is malformed (%s).", chain.MalformedReason)) + } + // len(Chain) is the prior chain's depth: the parser appends exactly one + // hop per level and stops at maxDelegationDepth, so it is + // min(depth, maxDelegationDepth). newLevels is what this exchange adds + // on top — one for the acting client, two when an external issuer is + // also nested — so the resulting chain never exceeds the cap. + if len(chain.Chain)+newLevels > maxDelegationDepth { + return nil, errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( + "The subject token's delegation chain is too deep.")) + } + // Nest priorAct verbatim rather than re-serializing from chain: core keeps + // each hop's extra claims in an unexported map, so rebuilding would silently + // drop the history trail that Section 4.1 asks us to preserve. The cost is + // that unknown hop claims — and the non-identity claims Section 4.1 calls + // "not meaningful" inside act — pass through re-signed, which sits in + // tension with Section 6 data minimization. Accepted here; bounding the + // subtree is tracked separately. + // + // nestUnder is the innermost entry this exchange added: the acting + // client normally, or the external-issuer entry when one was nested + // above, so the prior chain hangs below the provenance rather than + // displacing it. + nestUnder["act"] = priorAct + } + return act, nil +} + +// delegateClientAllowed reports whether actorID may use an issuer's +// AllowedDelegateClients-bound consent grant (a may_act.sub match or an +// AllowedActors match): either the list contains the wildcard +// anyDelegateClient, or it contains actorID directly. An empty list denies +// every client — validateTrustedIssuer rejects that configuration outright +// for any TrustedIssuer, so the empty case here is an extra fail-closed +// backstop, not the path an operator is expected to reach. Callers must only +// invoke this for a claim that actually went through that per-issuer +// validation (ExternalIssuer != ""); a self-issued token's +// AllowedDelegateClients is always nil, since that field has no equivalent +// concept on the self-issued path. +func delegateClientAllowed(allowedDelegateClients []string, actorID string) bool { + return slices.Contains(allowedDelegateClients, anyDelegateClient) || + slices.Contains(allowedDelegateClients, actorID) +} + // checkDelegationConsent enforces the RFC 8693 §4.4 delegation consent check. // -// If the subject token carries a may_act claim, it is the authoritative -// consent signal: only the party named in may_act.sub may delegate. The -// client_id binding is skipped in this case because may_act enables -// cross-client delegation (the token was issued to client A but authorizes -// client B to act). +// There are three consent sources, checked in order: +// +// 1. may_act: if present, it is the authoritative consent signal — only the +// party named in may_act.sub may delegate. The client_id binding is +// skipped in this case because may_act enables cross-client delegation +// (the token was issued to client A but authorizes client B to act). +// ValidatedClaims.AllowedDelegateClients is additionally enforced here, +// but ONLY when ExternalIssuer is set: that field is a TrustedIssuer +// concept with no self-issued equivalent, so a self-issued may_act (a +// ToolHive user token delegating to a ToolHive client directly) is +// bound by may_act.sub alone, the same as before this check existed. +// On the external path, may_act bypasses AllowedActors, not per-client +// containment — delegateClientAllowed still applies there. +// +// 2. ExternalActor: if may_act is absent but the multi-issuer validator has +// already established consent for this token (multi_issuer_validator.go), +// it did so by matching the subject token's actor claim against that +// issuer's operator-configured AllowedActors. That actor claim — even +// when configured as "client_id" — names a client in the EXTERNAL +// issuer's namespace, not ToolHive's, so it must never be compared +// against actorID the way case 3 below compares ValidatedClaims.ClientID. +// This case MUST be checked before case 3: it is not a fallback for an +// empty ClientID, it is a distinct, already-verified consent signal that +// takes priority whenever it is set, even if ClientID also happens to be +// populated. // -// If may_act is absent, fall back to client_id binding: the subject token's -// client_id must match the authenticated client. This prevents a stolen -// subject token from being exchanged by a different client. +// ValidatedClaims.AllowedDelegateClients binds this to specific ToolHive +// clients: actorID must appear in it (or it must contain the wildcard +// anyDelegateClient), checked below by delegateClientAllowed. Without +// this, the allowlist would authorize "this external client's tokens may +// be exchanged", not "...by this particular ToolHive client" — every +// ToolHive confidential client holding the token-exchange grant would be +// delegation-equivalent with respect to an allowlisted external actor, so +// compromise of the weakest such client would be as good as compromise +// of all of them (see #5989). validateTrustedIssuer rejects an empty +// AllowedDelegateClients at construction for exactly this reason: an +// operator must declare permissiveness with the wildcard, not get it by +// omission. Either way this remains bounded by: the calling client must +// already possess a valid subject token (it cannot forge one), and +// grantScopes/grantAndBoundAudiences still narrow the result to what +// both the client and the subject token are authorized for. // -// If neither may_act nor client_id is present, the subject token carries no +// 3. client_id: if neither of the above applies, fall back to client_id +// binding — the subject token's client_id must match the authenticated +// client. This prevents a stolen subject token from being exchanged by a +// different client. +// +// If none of the three consent sources apply, the subject token carries no // verifiable binding to any client at all — this fails closed (CWE-863) // rather than allowing an unbound token through. func checkDelegationConsent(validatedClaims *ValidatedClaims, actorID string) error { @@ -335,6 +497,25 @@ func checkDelegationConsent(validatedClaims *ValidatedClaims, actorID string) er return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( "The subject token does not authorize this client to act on behalf of the subject.")) } + if validatedClaims.ExternalIssuer != "" && !delegateClientAllowed(validatedClaims.AllowedDelegateClients, actorID) { + return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( + "This client is not authorized to exchange subject tokens from the external actor's issuer.")) + } + case validatedClaims.ExternalActor != "": + // Consent was already established by the external-issuer validator: the + // subject token's actor claim matched this issuer's operator-configured + // AllowedActors. That claim lives in the external issuer's client + // namespace, not ToolHive's, so — even when ClientID is also populated + // (ActorClaim: "client_id") — it must never be compared against + // actorID. This case must be checked before the client_id cases below, + // not merged with them. + // + // AllowedDelegateClients binds this allowlisted actor to a specific set + // of ToolHive clients — see delegateClientAllowed. + if !delegateClientAllowed(validatedClaims.AllowedDelegateClients, actorID) { + return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( + "This client is not authorized to exchange subject tokens from the external actor's issuer.")) + } case validatedClaims.ClientID != "" && validatedClaims.ClientID != actorID: return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( "The subject token was issued to a different client.")) @@ -481,9 +662,14 @@ func (h *Handler) grantDefaultAudience(ctx context.Context, requester fosite.Acc // ensureAudienceSubsetOfSubject verifies that every audience granted to the // delegated token is covered by the subject token's own audience. A subject -// token always carries at least one audience (the validator rejects tokens -// whose aud does not intersect the server's allowed audiences), so an empty -// subject audience here can only reject. +// token always carries at least one audience, so an empty subject audience +// here can only reject — but what guarantees that non-empty audience differs +// by path. On the self-issued path, SelfIssuedTokenValidator rejects tokens +// whose aud does not intersect this server's AllowedAudiences. On the +// external path, validateExternalToken instead requires aud to contain the +// issuer's own configured ExpectedAudience, which has no required +// relationship to AllowedAudiences — see the TrustedIssuers field doc +// comment in pkg/authserver/config.go for the operator-facing consequence. func ensureAudienceSubsetOfSubject(granted, subjectAud []string) error { subj := make(map[string]bool, len(subjectAud)) for _, a := range subjectAud { diff --git a/pkg/authserver/server/tokenexchange/handler_test.go b/pkg/authserver/server/tokenexchange/handler_test.go index 6d6b042512..a5f1f9a389 100644 --- a/pkg/authserver/server/tokenexchange/handler_test.go +++ b/pkg/authserver/server/tokenexchange/handler_test.go @@ -959,6 +959,368 @@ func TestTokenExchangeHandler_DefaultAudience(t *testing.T) { } } +func TestCheckDelegationConsent(t *testing.T) { + t.Parallel() + + const actorID = testAgentClientID + + tests := []struct { + name string + claims *ValidatedClaims + wantErr bool + errContains string + }{ + { + name: "may_act matching actorID accepted", + claims: &ValidatedClaims{MayAct: &MayActClaim{Sub: actorID}}, + }, + { + name: "may_act not matching actorID rejected", + claims: &ValidatedClaims{MayAct: &MayActClaim{Sub: "different-agent"}}, + wantErr: true, + errContains: "does not authorize", + }, + { + // #5989 fix: AllowedDelegateClients must be enforced on the + // may_act path too, not only ExternalActor's — an issuer that + // can emit may_act but has no AllowedActors-equivalent consent + // path (Entra, Okta) would otherwise have no per-client + // containment at all. ExternalIssuer must be set for this check + // to apply at all — see checkDelegationConsent's doc comment for + // why a self-issued may_act (ExternalIssuer == "") skips it. + name: "may_act matching actorID, actorID in AllowedDelegateClients accepted", + claims: &ValidatedClaims{ + MayAct: &MayActClaim{Sub: actorID}, + ExternalIssuer: "https://idp.example.com", + AllowedDelegateClients: []string{"some-other-client", actorID}, + }, + }, + { + name: "may_act matching actorID, actorID not in AllowedDelegateClients rejected", + claims: &ValidatedClaims{ + MayAct: &MayActClaim{Sub: actorID}, + ExternalIssuer: "https://idp.example.com", + AllowedDelegateClients: []string{"some-other-client"}, + }, + wantErr: true, + errContains: "not authorized to exchange subject tokens", + }, + { + // A self-issued may_act (ExternalIssuer unset) has no + // AllowedDelegateClients equivalent, so an empty/absent value + // here must not reject it — unlike the external-issuer case + // below, where the same emptiness is a hard rejection. + name: "may_act matching actorID, ExternalIssuer unset, AllowedDelegateClients not enforced", + claims: &ValidatedClaims{ + MayAct: &MayActClaim{Sub: actorID}, + }, + }, + { + name: "ExternalActor set, actorID in wildcard AllowedDelegateClients accepted", + claims: &ValidatedClaims{ + ExternalActor: "ext-agent", + AllowedDelegateClients: []string{anyDelegateClient}, + }, + }, + { + // Guards the switch ordering in checkDelegationConsent: the + // ExternalActor case must be checked before the client_id cases, + // so a populated (and mismatched) ClientID must not cause + // rejection when ExternalActor is already set. + name: "ExternalActor set with a differing ClientID still accepted", + claims: &ValidatedClaims{ + ExternalActor: "ext-agent", + ClientID: "some-other-client", + AllowedDelegateClients: []string{anyDelegateClient}, + }, + }, + { + // may_act must keep winning even when ExternalActor is also set + // (the external validator only sets ExternalActor when MayAct is + // nil, but checkDelegationConsent's own ordering must not depend + // on that invariant holding). + name: "ExternalActor set but MayAct mismatched is rejected", + claims: &ValidatedClaims{ + ExternalActor: "ext-agent", + MayAct: &MayActClaim{Sub: "different-agent"}, + }, + wantErr: true, + errContains: "does not authorize", + }, + { + // #5989 hardening: AllowedDelegateClients is now required at + // construction (validateTrustedIssuer), so nil/empty must be + // rejected at runtime too, not treated as "any client" — this + // pins the fail-closed default this PR introduces. + name: "ExternalActor set, nil AllowedDelegateClients rejected", + claims: &ValidatedClaims{ + ExternalActor: "ext-agent", + // AllowedDelegateClients intentionally nil. + }, + wantErr: true, + errContains: "not authorized to exchange subject tokens", + }, + { + name: "ExternalActor set, actorID in AllowedDelegateClients accepted", + claims: &ValidatedClaims{ + ExternalActor: "ext-agent", + AllowedDelegateClients: []string{"some-other-client", actorID}, + }, + }, + { + name: "ExternalActor set, actorID not in AllowedDelegateClients rejected", + claims: &ValidatedClaims{ + ExternalActor: "ext-agent", + AllowedDelegateClients: []string{"some-other-client"}, + }, + wantErr: true, + errContains: "not authorized to exchange subject tokens", + }, + { + name: "client_id matching actorID accepted", + claims: &ValidatedClaims{ClientID: actorID}, + }, + { + name: "client_id not matching actorID rejected", + claims: &ValidatedClaims{ClientID: "different-client"}, + wantErr: true, + errContains: "different client", + }, + { + name: "no may_act, no ExternalActor, no client_id rejected", + claims: &ValidatedClaims{}, + wantErr: true, + errContains: "no verifiable client binding", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := checkDelegationConsent(tt.claims, actorID) + if tt.wantErr { + require.Error(t, err) + var rfcErr *fosite.RFC6749Error + require.True(t, errors.As(err, &rfcErr), "expected fosite RFC6749Error") + assert.Contains(t, rfcErr.Reason(), tt.errContains) + return + } + require.NoError(t, err) + }) + } +} + +// newTestHandlerWithValidator creates a Handler wired to a caller-provided +// validator, for tests that need external-issuer delegation coverage that +// newTestHandler's self-issued-only validator cannot produce. +func newTestHandlerWithValidator(validator SubjectTokenValidator) *Handler { + return &Handler{ + validator: validator, + delegationLifespan: 15 * time.Minute, + allowedAudiences: []string{testIssuer}, + config: &mockConfig{ + scopeStrategy: fosite.ExactScopeStrategy, + audienceStrategy: fosite.DefaultAudienceMatchingStrategy, + }, + } +} + +// TestTokenExchangeHandler_ActChainProvenance covers the act-chain shape that +// HandleTokenEndpointRequest builds, for the case a token exchange terminates +// on (ValidatedClaims.ExternalActor set) that TestTokenExchangeHandler_ +// HandleTokenEndpointRequest's self-issued-only table cannot exercise. +func TestTokenExchangeHandler_ActChainProvenance(t *testing.T) { + t.Parallel() + + tj := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + jwksServer := startJWKSServer(t, externalJWKS) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + multiValidator := newMultiValidator(t, tj, trustedIssuers) + + // externalToken signs a subject token from the trusted external issuer + // with an allowlisted actor (so ExternalActor is set) and an optional + // prior act chain. The audience includes testIssuer alongside the + // issuer's required ExpectedAudience so the delegated token's + // default-audience grant (testIssuer) stays covered by the subject + // token's own audience (ensureAudienceSubsetOfSubject). + externalToken := func(t *testing.T, priorAct any) string { + t.Helper() + claims := externalClaims() + claims.Audience = jwt.Audience{testExternalAudience, testIssuer} + extra := map[string]any{"azp": "ext-agent"} + if priorAct != nil { + extra["act"] = priorAct + } + return externalJWKS.signToken(t, claims, extra) + } + + requestWith := func(t *testing.T, h *Handler, token string) (*fosite.AccessRequest, error) { + t.Helper() + form := url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {token}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + } + req := newAccessRequest(t, defaultClient(), form) + return req, h.HandleTokenEndpointRequest(context.Background(), req) + } + + t.Run("external delegation nests actor and issuer under the client's act entry", func(t *testing.T) { + t.Parallel() + h := newTestHandlerWithValidator(multiValidator) + + req, err := requestWith(t, h, externalToken(t, nil)) + require.NoError(t, err) + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + assert.Equal(t, testExternalIssuer+"#"+"ext-user-456", sess.JWTClaims.Subject, + "external subject must be qualified with the issuer URL, not copied verbatim") + act, ok := sess.JWTClaims.Extra["act"].(map[string]any) + require.True(t, ok, "act claim must be a map") + assert.Equal(t, testAgentClientID, act["sub"], "outermost act.sub is always the ToolHive client") + + nested, ok := act["act"].(map[string]any) + require.True(t, ok, "external actor/issuer must be nested under act.act") + assert.Equal(t, "ext-agent", nested["sub"]) + assert.Equal(t, testExternalIssuer, nested["iss"]) + assert.Nil(t, nested["act"], "no prior chain to nest further") + }) + + t.Run("self-issued delegation stays a single level with no external nesting", func(t *testing.T) { + t.Parallel() + h := newTestHandler(t, tj, 15*time.Minute) + + req, err := requestWith(t, h, tj.signToken(t, validClaims(), validExtraClaims())) + require.NoError(t, err) + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + assert.Equal(t, "user-123", sess.JWTClaims.Subject, + "self-issued subject must pass through unchanged, never qualified") + act, ok := sess.JWTClaims.Extra["act"].(map[string]any) + require.True(t, ok, "act claim must be a map") + assert.Equal(t, testAgentClientID, act["sub"]) + assert.Nil(t, act["act"], "self-issued delegation must not nest an external actor") + }) + + // #5989 fix: an external issuer choosing a "sub" equal to a native + // ToolHive user's UUID must never produce a delegated token whose "sub" + // collides with that UUID — Cedar's extractClientIDFromClaims + // (pkg/authz/authorizers/cedar/core.go) keys on "sub" alone, and native + // ToolHive subjects are UUIDs from UserResolver.ResolveUser, never an + // upstream provider's raw subject value. + t.Run("external subject equal to a plausible native UUID is qualified, not collided", func(t *testing.T) { + t.Parallel() + h := newTestHandlerWithValidator(multiValidator) + + const nativeLikeUUID = "8a34c9e0-6b1a-4e5f-9c2d-1f0a7b3d5e6c" + claims := externalClaims() + claims.Subject = nativeLikeUUID + claims.Audience = jwt.Audience{testExternalAudience, testIssuer} + token := externalJWKS.signToken(t, claims, map[string]any{"azp": "ext-agent"}) + + req, err := requestWith(t, h, token) + require.NoError(t, err) + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + assert.NotEqual(t, nativeLikeUUID, sess.JWTClaims.Subject, + "the qualified subject must never equal the bare external sub, "+ + "or it could collide with a native user sharing that UUID") + assert.Equal(t, testExternalIssuer+"#"+nativeLikeUUID, sess.JWTClaims.Subject) + }) + + // The documented dangerous config (see checkDelegationConsent and + // resolveAllowedActor's doc comments): a trusted external issuer emitting + // a well-formed may_act naming this client bypasses the allowlist + // entirely. Only covered at the checkDelegationConsent unit level until + // now — this exercises it through the full handler. + t.Run("external token carrying may_act still nests the issuer, with no actor to report", func(t *testing.T) { + t.Parallel() + h := newTestHandlerWithValidator(multiValidator) + + claims := externalClaims() + claims.Audience = jwt.Audience{testExternalAudience, testIssuer} + token := externalJWKS.signToken(t, claims, map[string]any{ + "may_act": map[string]any{"sub": testAgentClientID, "iss": testIssuer}, + }) + + req, err := requestWith(t, h, token) + require.NoError(t, err) + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + act, ok := sess.JWTClaims.Extra["act"].(map[string]any) + require.True(t, ok, "act claim must be a map") + assert.Equal(t, testAgentClientID, act["sub"]) + + // The path that bypasses the allowlist entirely still records where + // the delegation came from (ValidatedClaims.ExternalIssuer is set + // unconditionally by validateExternalToken) — it just has no + // client-namespace actor claim to report, since may_act.sub already + // named the delegate directly via the outermost act.sub above. + nested, ok := act["act"].(map[string]any) + require.True(t, ok, "external issuer must be nested even on the may_act path") + assert.Equal(t, testExternalIssuer, nested["iss"]) + _, hasSub := nested["sub"] + assert.False(t, hasSub, "no ExternalActor exists on the may_act path") + }) + + // maxDelegationDepth (10) bounds the prior chain depth + newLevels. The + // external wrapper adds a level of its own (newLevels=2) versus the + // self-issued path (newLevels=1), so the external path's prior chain must + // be one level shallower to fit: + // self-issued: depth 9 accepted (9+1=10); depth 10 rejected (10+1=11>10) + // — see "delegation chain under max depth is nested" / + // "delegation chain at max depth rejected" in + // TestTokenExchangeHandler_HandleTokenEndpointRequest. + // external: depth 8 accepted (8+2=10); depth 9 rejected (9+2=11>10) + // Split into two subtests (rather than one asserting both sides) so a + // failure on the accept half can't hide a regression on the reject half. + t.Run("external delegation at depth 8 fits exactly and preserves the full nested chain", func(t *testing.T) { + t.Parallel() + h := newTestHandlerWithValidator(multiValidator) + + req, err := requestWith(t, h, externalToken(t, nestedActChain(8))) + require.NoError(t, err, "depth 8 plus the external wrapper must fit exactly at maxDelegationDepth") + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + act, ok := sess.JWTClaims.Extra["act"].(map[string]any) + require.True(t, ok, "act claim must be a map") + external, ok := act["act"].(map[string]any) + require.True(t, ok, "external actor/issuer must be nested under act.act") + assert.Equal(t, "ext-agent", external["sub"]) + assert.Equal(t, testExternalIssuer, external["iss"]) + // The prior chain must be preserved under the external wrapper, not + // dropped or overwritten by it — this is the provenance record + // checkDelegationConsent's doc comment relies on being auditable. + assert.Equal(t, nestedActChain(8), external["act"], + "the prior act chain must be nested under the external wrapper, unchanged") + }) + + t.Run("external delegation at depth 9 is rejected as too deep", func(t *testing.T) { + t.Parallel() + h := newTestHandlerWithValidator(multiValidator) + + _, err := requestWith(t, h, externalToken(t, nestedActChain(9))) + require.Error(t, err) + assert.True(t, errors.Is(err, fosite.ErrInvalidGrant)) + var rfcErr *fosite.RFC6749Error + require.True(t, errors.As(err, &rfcErr), "expected fosite RFC6749Error") + assert.Contains(t, rfcErr.Reason(), "too deep") + }) +} + // mockConfig implements the tokenExchangeConfig interface for testing. type mockConfig struct { scopeStrategy fosite.ScopeStrategy diff --git a/pkg/authserver/server/tokenexchange/multi_issuer_validator.go b/pkg/authserver/server/tokenexchange/multi_issuer_validator.go index da970861fd..62cb4ede03 100644 --- a/pkg/authserver/server/tokenexchange/multi_issuer_validator.go +++ b/pkg/authserver/server/tokenexchange/multi_issuer_validator.go @@ -9,156 +9,423 @@ import ( "errors" "fmt" "io" + "log/slog" "net" "net/http" "net/url" + "slices" + "strings" "sync" "time" "github.com/go-jose/go-jose/v4" "github.com/go-jose/go-jose/v4/jwt" + "github.com/lestrrat-go/httprc/v3" + "github.com/lestrrat-go/jwx/v3/jwk" + "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" ) const ( - // jwksCacheTTL is the time-to-live for cached JWKS fetched from external issuers. - jwksCacheTTL = 5 * time.Minute - // httpTimeout is the timeout for HTTP requests to external OIDC endpoints. httpTimeout = 10 * time.Second - // maxResponseBodySize is the maximum size of HTTP response bodies read from - // external OIDC endpoints (1 MiB). This prevents resource exhaustion from - // unexpectedly large responses. + // maxResponseBodySize is the maximum size of HTTP response bodies read + // from external OIDC discovery documents AND JWKS fetches (1 MiB). This + // prevents resource exhaustion from unexpectedly large responses. The + // discovery read enforces it directly via io.LimitReader; the JWKS read + // goes through jwx instead, so it is enforced by wrapping every issuer's + // *http.Client transport with limitedBodyTransport — see where that + // client is built in NewMultiIssuerTokenValidator. maxResponseBodySize = 1 << 20 // maxJWKSKeys caps the number of keys accepted from an external JWKS to // prevent CPU amplification from a hostile endpoint serving many keys. maxJWKSKeys = 100 - // maxRedirects caps redirects followed when fetching external OIDC metadata. - maxRedirects = 5 + // minKidRefreshInterval bounds how often refreshOnUnknownKid forces an + // issuer's jwk.Cache to fetch its JWKS ahead of jwx's own background + // refresh schedule. verifySignature's kidMatched check runs before the + // subject token's signature is trusted, so without this floor a client + // presenting a syntactically valid JWT that merely names a made-up kid + // could force a fresh fetch to the external IdP on every attempt. + minKidRefreshInterval = 30 * time.Second + + // jwksFetchFailureBackoff bounds how often ensureRegistered retries a + // JWKS fetch for an issuer that has never yet succeeded. Key resolution + // runs before the subject token's signature is checked, so without this + // an authenticated client holding the token-exchange grant could force a + // real outbound fetch to a broken external IdP on every single request. + jwksFetchFailureBackoff = 30 * time.Second + + // jwksRefreshInterval is the fixed interval at which each issuer's + // jwk.Cache re-fetches its JWKS in the background. It is passed to + // Register via jwk.WithConstantInterval, which makes the resource + // ignore the response's Cache-Control/Expires headers entirely rather + // than merely bounding them with WithMaxInterval — deliberately: + // absent this override, httprc derives the interval from those + // headers, clamped to [DefaultMinInterval, DefaultMaxInterval] = + // [15m, 30 days], so a hostile or misconfigured issuer could otherwise + // extend our own key-retention window up to a month simply by setting + // a long max-age. + jwksRefreshInterval = 5 * time.Minute + + // defaultActorClaim is the claim read to identify the client that + // requested an external subject token, when a TrustedIssuer does not + // configure ActorClaim. This matches Microsoft Entra v2 and many other + // OIDC providers' convention for the authorized-party claim. + // + // "client_id" is the normative claim: RFC 8693 §4.3 names it as the + // party a token was issued to, and RFC 9068 §2.2 makes it a REQUIRED + // access-token claim. "azp" is the default here only because Entra, + // Okta, and other major providers omit "client_id" from their access + // tokens in practice — "azp" itself appears nowhere in RFC 9068; it is + // an OIDC Core claim defined for ID tokens, not access tokens. + defaultActorClaim = "azp" + + // externalClockSkewLeeway widens "nbf"/"iat"/"exp" acceptance for external + // subject tokens to tolerate clock skew between the external IdP and + // ToolHive. validateExternalToken independently rejects an expired + // subject token by comparing its exp against time.Now() with zero + // tolerance, so this leeway only protects against spurious + // not-yet-valid/issued-in-the-future rejections — it never causes an + // expired token to be accepted. + externalClockSkewLeeway = 60 * time.Second ) +// actorClaimsNotInExtra are claims assignClaim drops or reroutes, so they +// never reach ValidatedClaims.Extra. Only "client_id" has an explicit +// fallback in resolveAllowedActor; configuring ActorClaim to any of these +// would make every external token look like it's missing the claim. +var actorClaimsNotInExtra = []string{ + "sub", "iss", "aud", "exp", "iat", "nbf", "jti", "name", "email", "scope", "scp", "may_act", +} + // Compile-time check that MultiIssuerTokenValidator implements SubjectTokenValidator. var _ SubjectTokenValidator = (*MultiIssuerTokenValidator)(nil) // TrustedIssuer configures an external OIDC issuer whose tokens are // accepted as subject tokens during token exchange. +// +// This type is reused verbatim as the wire schema for +// authserver.RunConfig.TrustedIssuers (deliberately, to avoid a parallel +// type that drifts — see the go-style rule against that). Its JSON/YAML +// tags are therefore part of the serialized RunConfig, which is reflected +// into docs/server/swagger.*; adding, renaming, or retagging a field here is +// a schema change, not a purely internal one. type TrustedIssuer struct { // IssuerURL is the expected "iss" claim value (exact match). - IssuerURL string + IssuerURL string `json:"issuer_url" yaml:"issuer_url"` // ExpectedAudience is the expected "aud" claim value that must appear - // in the token's audience list. Required; NewMultiIssuerTokenValidator - // rejects any TrustedIssuer with an empty ExpectedAudience. - ExpectedAudience string + // in the token's audience list (a resource/API identifier, not a + // client ID — required, but not enforced; see looksLikeResourceIdentifier). + // See docs/arch/17-token-exchange-delegation.md ("ID/access-token + // discrimination") for why and its limits. + ExpectedAudience string `json:"expected_audience" yaml:"expected_audience"` // JWKSURL is the URL to fetch the issuer's JSON Web Key Set from. // If empty, it is resolved via OIDC discovery at {IssuerURL}/.well-known/openid-configuration. - JWKSURL string + JWKSURL string `json:"jwks_url,omitempty" yaml:"jwks_url,omitempty"` + // InsecureAllowHTTP permits plain-HTTP OIDC discovery and JWKS fetches + // for THIS issuer only. Development and testing only — never set in + // production. Does not relax the private-IP guard; see AllowPrivateIPs. + // Deliberately per-issuer: this server's own InsecureAllowHTTP must not + // silently permit plaintext discovery for every trusted external issuer + // too — a network attacker who can intercept that traffic could + // substitute a JWKS and forge subject tokens for that issuer's + // namespace. + InsecureAllowHTTP bool `json:"insecure_allow_http,omitempty" yaml:"insecure_allow_http,omitempty"` + // AllowPrivateIPs permits OIDC discovery and JWKS fetches for THIS + // issuer to resolve to a private or loopback address. Use only when the + // issuer is hosted inside the same cluster and has no public endpoint. + AllowPrivateIPs bool `json:"allow_private_ips,omitempty" yaml:"allow_private_ips,omitempty"` + // ActorClaim names the claim identifying the client that requested the + // subject token from THIS EXTERNAL ISSUER (used by AllowedActors below). + // Values are in the external issuer's namespace, NOT ToolHive client + // IDs. Defaults to "azp"; use "appid" for Microsoft Entra v1, "cid" for + // Okta. The special value "client_id" reads ValidatedClaims.ClientID + // instead of Extra (assignClaim routes it to that field) — it is still + // the external token's client_id claim, not a ToolHive one. + ActorClaim string `json:"actor_claim,omitempty" yaml:"actor_claim,omitempty"` + // AllowedActors is the allowlist of ActorClaim values authorized to + // exchange a subject token from this issuer when it carries no + // "may_act" claim; empty means only may_act-bearing tokens are + // accepted. By itself names no ToolHive client — see + // AllowedDelegateClients and docs/arch/17-token-exchange-delegation.md + // ("Accepted limitations" #1). + AllowedActors []string `json:"allowed_actors,omitempty" yaml:"allowed_actors,omitempty"` + // AllowedDelegateClients restricts which ToolHive client IDs may + // exchange a subject token from this issuer, for BOTH consent paths. + // Required (validateTrustedIssuer rejects empty/absent); "*" permits + // any confidential client holding the grant. See + // docs/arch/17-token-exchange-delegation.md ("Accepted limitations" #1). + //nolint:lll // field tags require full JSON+YAML names + AllowedDelegateClients []string `json:"allowed_delegate_clients,omitempty" yaml:"allowed_delegate_clients,omitempty"` } // MultiIssuerTokenValidator validates subject tokens from the authorization -// server itself or from configured external OIDC issuers. +// server itself or from configured external OIDC issuers, delegating +// self-issued tokens to SelfIssuedTokenValidator and resolving external +// issuers' JWKS (via OIDC discovery if needed) to verify signature and +// claims. // -// For self-issued tokens (where the "iss" claim matches selfIssuer), validation -// is delegated to the SelfIssuedTokenValidator. For tokens from trusted external -// issuers, the validator resolves the issuer's JWKS (via OIDC discovery if needed), -// verifies the JWT signature, and validates standard claims. -// -// TODO(#5989): this validator is not yet wired into Factory (which still -// constructs a SelfIssuedTokenValidator), so external subject tokens are not -// reachable in production. External tokens carry no client_id claim, so the -// handler's checkDelegationConsent fails them closed. The external-token -// delegation-consent policy MUST land in the same change that wires this -// validator into Factory — do not enable external issuers without it. +// A valid signature and audience alone would authorize ToolHive as a +// resource, not any particular client, as a delegate — a confused-deputy +// risk (CWE-863). validateExternalToken therefore requires a "may_act" +// claim or a matching AllowedActors entry (surfaced as +// ValidatedClaims.ExternalActor) before returning successfully. See +// docs/arch/17-token-exchange-delegation.md for the full consent-signal +// precedence and trust model. type MultiIssuerTokenValidator struct { selfIssuer string selfValidator *SelfIssuedTokenValidator issuers map[string]*externalIssuerConfig - httpClient *http.Client - - // insecureSkipJWKSURLValidation disables HTTPS enforcement on discovered - // JWKS URLs and relaxes the dial-time IP/scheme checks (so httptest servers - // on loopback over HTTP are reachable). This MUST only be set for testing - // with httptest servers. - insecureSkipJWKSURLValidation bool } // externalIssuerConfig holds the configuration and cached state for an external -// OIDC issuer. The mutex protects lazy JWKS URL discovery and JWKS caching. +// OIDC issuer. The embedded TrustedIssuer is treated as immutable after +// construction: resolveAllowedActor reads TrustedIssuer.AllowedActors (and +// discoverJWKSURL/fetchJWKS read httpClient) on every validation without +// holding mu, since mu protects JWKS state only. Mutating TrustedIssuer +// fields in place after NewMultiIssuerTokenValidator returns is a data race. type externalIssuerConfig struct { TrustedIssuer - mu sync.Mutex - jwksURL string // resolved from OIDC discovery or preconfigured - jwks *jose.JSONWebKeySet // cached JWKS - jwksExp time.Time // when the cached JWKS expires + // httpClient is dedicated to this issuer, built once at construction + // time from its own InsecureAllowHTTP/AllowPrivateIPs. A single + // validator-wide client couldn't enforce per-issuer SSRF/transport + // policy: http.Client.CheckRedirect and Transport.DialContext have no + // way to know which issuer's fetch they are guarding. + httpClient *http.Client + + // jwksCache is this issuer's own jwk.Cache, registered with httpClient + // above via jwk.WithHTTPClient (see registerOrRefresh). A cache per + // issuer, rather than one shared across every configured issuer, is what + // makes two issuers resolving to the same jwks_url (e.g. two Microsoft + // Entra v1 tenants, which share one tenant-independent JWKS endpoint) a + // non-event: httprc keys a cached resource by URL alone and only honors + // jwk.WithHTTPClient on a URL's first Register call, so a shared cache + // would have the second such issuer silently inherit the first one's + // *http.Client — defeating InsecureAllowHTTP/AllowPrivateIPs's per-issuer + // guarantee for it. Splitting the cache per issuer makes that collision + // unrepresentable instead of guarding against it. + jwksCache *jwk.Cache + + mu sync.Mutex + // jwksURL is resolved from OIDC discovery, or copied from + // TrustedIssuer.JWKSURL when hand-configured. Once set, it is never + // cleared: unlike the JWKS document itself (which jwksCache keeps fresh + // on its own schedule — see ensureRegistered), a change to + // the *endpoint URL* an issuer serves its keys from requires a process + // restart to pick up. Neither Microsoft Entra nor Okta documents rotating + // that URL, only the keys served at it, and every other TrustedIssuer + // field already requires a restart to take effect, so this is + // consistent with the rest of this config's lifecycle. + jwksURL string + // fetched is true once at least one JWKS fetch has succeeded for this + // issuer since process start. Until then, ensureRegistered forces a + // synchronous fetch on every call, since there is no cached value yet to + // fall back on and jwx's own background schedule offers no way to wait + // for its result. + fetched bool + // lastKidRefresh is the last time refreshOnUnknownKid forced a fetch; + // see minKidRefreshInterval. + lastKidRefresh time.Time + // fetchFailedAt and fetchErr gate retries once registered but never fetched: + // key resolution runs before signature verification, so without this an + // authenticated client can otherwise drive one real outbound fetch per + // token-exchange request just by naming an issuer whose endpoint is + // down. fetchErr is served directly while the gate is closed, so the + // caller still gets a specific error instead of a generic "try later". + // Not consulted once fetched is true: a healthy issuer, or one serving + // stale-but-valid keys through a later background-refresh failure, must + // never be gated. + fetchFailedAt time.Time + fetchErr error +} + +// limitedBodyTransport wraps an http.RoundTripper to cap every response body +// at max bytes, via http.MaxBytesReader rather than io.LimitReader: the +// latter truncates silently, which would let a caller parse a cut-off JWKS +// document as if it were complete, where the former surfaces a +// *http.MaxBytesError instead. Its error text ("http: request body too +// large") is written for the request-body case MaxBytesReader was designed +// for, so it reads oddly for a capped response — an accepted rough edge +// rather than justifying a custom ReadCloser. +// +// The nil first argument (http.ResponseWriter) is safe: MaxBytesReader only +// reaches it through a type assertion (`l.w.(requestTooLarger)`) used to tell +// a real server connection to close early, which — on a nil interface value +// — safely evaluates to false rather than panicking (net/http/request.go). +type limitedBodyTransport struct { + base http.RoundTripper + max int64 +} + +// RoundTrip delegates to base and then caps the returned body. The cap +// applies to every response, including a non-2xx one whose body the caller +// doesn't intend to parse — draining it is only ever io.Discard-ed, not +// unbounded, so this errs on the safe side rather than special-casing status +// codes. +func (t *limitedBodyTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.base.RoundTrip(req) + if err != nil { + return nil, err + } + resp.Body = http.MaxBytesReader(nil, resp.Body, t.max) + return resp, nil } -// NewMultiIssuerTokenValidator creates a validator that accepts tokens from the -// authorization server itself and from the provided list of trusted external issuers. -// Returns an error if any TrustedIssuer has an empty ExpectedAudience. +// NewMultiIssuerTokenValidator creates a validator that accepts tokens from +// the authorization server itself and from the provided trusted external +// issuers. Returns an error if selfValidator is nil, selfIssuer is empty, +// any TrustedIssuer is invalid (see validateTrustedIssuer), or an issuer's +// dedicated HTTP client cannot be built (see newExternalIssuerConfig for +// why each issuer gets its own). func NewMultiIssuerTokenValidator( selfValidator *SelfIssuedTokenValidator, selfIssuer string, trustedIssuers []TrustedIssuer, ) (*MultiIssuerTokenValidator, error) { - for _, issuer := range trustedIssuers { - if issuer.ExpectedAudience == "" { - return nil, fmt.Errorf("trusted issuer %q: ExpectedAudience is required", issuer.IssuerURL) - } + if selfValidator == nil { + return nil, errors.New("selfValidator must not be nil") + } + if selfIssuer == "" { + return nil, errors.New("selfIssuer must not be empty") } issuers := make(map[string]*externalIssuerConfig, len(trustedIssuers)) for _, ti := range trustedIssuers { - issuers[ti.IssuerURL] = &externalIssuerConfig{ - TrustedIssuer: ti, - jwksURL: ti.JWKSURL, + if err := validateTrustedIssuer(ti, selfIssuer, issuers); err != nil { + return nil, err } + if len(ti.AllowedActors) == 0 { + slog.Warn("Trusted issuer has no allowed actors configured; "+ + "only may_act-bearing subject tokens from it will be accepted", + "issuer", ti.IssuerURL, + ) + } + if !looksLikeResourceIdentifier(ti.ExpectedAudience) { + slog.Warn("Trusted issuer's expected_audience does not look like a resource identifier "+ + "(no URI scheme, e.g. \"https://\" or \"api://\"); it may actually be a client ID. "+ + "rejectIDTokenClaims only catches an ID token that carries at_hash/c_hash, which OIDC "+ + "does not require, so this issuer's audience check may not distinguish an ID token "+ + "from an access token", + "issuer", ti.IssuerURL, "expected_audience", ti.ExpectedAudience, + ) + } + + issuerConfig, err := newExternalIssuerConfig(ti) + if err != nil { + return nil, err + } + issuers[ti.IssuerURL] = issuerConfig } - v := &MultiIssuerTokenValidator{ + return &MultiIssuerTokenValidator{ selfIssuer: selfIssuer, selfValidator: selfValidator, issuers: issuers, + }, nil +} + +// newExternalIssuerConfig builds the *externalIssuerConfig for a single +// already-validated TrustedIssuer: a dedicated HTTP client (scoped to that +// issuer's own InsecureAllowHTTP/AllowPrivateIPs), its body-size-capped +// transport, and its own jwk.Cache. Called once per issuer from +// NewMultiIssuerTokenValidator's constructor loop, after validateTrustedIssuer +// and the startup warnings have already run for ti. +func newExternalIssuerConfig(ti TrustedIssuer) (*externalIssuerConfig, error) { + // Clone AllowedActors and AllowedDelegateClients so a caller mutating + // their original slices in place (e.g. a future config reload) cannot + // race with the unsynchronized reads in resolveAllowedActor and + // validateExternalToken, which run on every validation without + // holding externalIssuerConfig.mu (that mutex guards JWKS state only). + ti.AllowedActors = slices.Clone(ti.AllowedActors) + ti.AllowedDelegateClients = slices.Clone(ti.AllowedDelegateClients) + + // Deliberately networking.NewHttpClientBuilder(), not + // NewHostScopedClientBuilder: that helper ORs + // INSECURE_DISABLE_URL_VALIDATION and an auto-localhost exemption into + // BOTH the HTTP-scheme and private-IP gates, so an unrelated env var — + // or a trusted issuer that merely happens to be on localhost — would + // silently widen AllowPrivateIPs regardless of what the operator set, + // defeating the point of splitting the two flags per issuer. + // + // Keep-alive connections are disabled: this client dials jwks_uri, a host taken + // from an untrusted discovery document, only on the fixed + // jwksRefreshInterval schedule plus occasional on-demand refreshes — + // no hot path here to trade the per-dial SSRF check away for. + httpClient, err := networking.NewHttpClientBuilder(). + WithInsecureAllowHTTP(ti.InsecureAllowHTTP). + WithPrivateIPs(ti.AllowPrivateIPs). + WithTimeout(httpTimeout). + WithDisableKeepAlives(true). + Build() + if err != nil { + return nil, fmt.Errorf("issuer_url %q: failed to build HTTP client: %w", ti.IssuerURL, err) + } + // Guard against a discovery/JWKS redirect hop landing on a + // different, unvetted host — the same policy the transparent proxy + // data path applies to a response derived from an untrusted remote + // server (see SameHostRedirectPolicy's doc comment). + httpClient.CheckRedirect = networking.SameHostRedirectPolicy() + + // Cap every response body this client reads at maxResponseBodySize — + // discovery already enforces this itself via io.LimitReader + // (discoverJWKSURL), but the JWKS fetch is handed to jwx's jwk.Cache + // below, which has no equivalent cap of its own (httprc.MaxBufferSize + // is ~1000 MiB, and its transformer does an unbounded io.ReadAll under + // that ceiling before parsing). Wrapped OUTSIDE httpClient.Transport + // (which Build() always sets — see networking's builder) so the + // private-IP dial guard and ValidatingTransport's scheme check still + // run first, on the inner, unwrapped transport. + httpClient.Transport = &limitedBodyTransport{ + base: httpClient.Transport, + max: maxResponseBodySize, + } + + // One jwk.Cache per issuer (see externalIssuerConfig.jwksCache's doc + // comment for why), each running its own background worker pool + // (jwk.NewCache -> httprc.Client.Start) for the life of the process. + // WithWorkers(1) caps that pool to one worker per issuer instead of + // httprc's default five — budget roughly three goroutines per issuer + // including its controller loop and wait-group waiter. + // context.Background() is deliberate: there's no per-call context to + // root this in, and the loop is meant to outlive any single call, + // stopped only via jwk.Cache.Shutdown — which nothing here calls, + // matching pkg/auth/token.go's TokenValidator. + jwksCache, err := jwk.NewCache(context.Background(), httprc.NewClient(httprc.WithWorkers(1))) + if err != nil { + return nil, fmt.Errorf("issuer_url %q: failed to create JWKS cache: %w", ti.IssuerURL, err) } - v.httpClient = &http.Client{ - Timeout: httpTimeout, - CheckRedirect: func(req *http.Request, via []*http.Request) error { - if len(via) >= maxRedirects { - return errors.New("too many redirects") - } - // Re-validate the scheme of each redirect hop; the resolved IP - // is checked in DialContext below. - if !v.insecureSkipJWKSURLValidation && req.URL.Scheme != "https" { - return fmt.Errorf("redirect to non-HTTPS URL: %q", req.URL.Scheme) - } - return nil - }, - Transport: &http.Transport{ - DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { - host, port, err := net.SplitHostPort(addr) - if err != nil { - return nil, err - } - ips, err := net.DefaultResolver.LookupIPAddr(ctx, host) - if err != nil { - return nil, err - } - for _, ipa := range ips { - if !v.insecureSkipJWKSURLValidation && isDisallowedIP(ipa.IP) { - return nil, fmt.Errorf("refusing to connect to disallowed address %s", ipa.IP) - } - } - return (&net.Dialer{Timeout: 5 * time.Second}).DialContext( - ctx, network, net.JoinHostPort(ips[0].String(), port)) - }, - }, - } - - return v, nil + return &externalIssuerConfig{ + TrustedIssuer: ti, + jwksURL: ti.JWKSURL, + httpClient: httpClient, + jwksCache: jwksCache, + }, nil +} + +// ValidateTrustedIssuers runs every structural check +// NewMultiIssuerTokenValidator performs on trustedIssuers — required fields, +// self-issuer collision, duplicate issuers, and ActorClaim reachability — +// without constructing a validator or any per-issuer HTTP client. Config +// validation calls this to fail before the live upstream DCR registration +// and storage creation that run between RunConfig.Validate and server +// construction; NewMultiIssuerTokenValidator repeats the same checks at +// server startup as defence in depth. Both route through validateTrustedIssuer, +// so the two can't drift out of sync. +func ValidateTrustedIssuers(trustedIssuers []TrustedIssuer, selfIssuer string) error { + issuers := make(map[string]*externalIssuerConfig, len(trustedIssuers)) + for _, ti := range trustedIssuers { + if err := validateTrustedIssuer(ti, selfIssuer, issuers); err != nil { + return err + } + issuers[ti.IssuerURL] = &externalIssuerConfig{TrustedIssuer: ti} + } + return nil } // Validate parses the raw JWT to extract the issuer claim, then routes validation @@ -172,12 +439,10 @@ func (v *MultiIssuerTokenValidator) Validate(ctx context.Context, rawToken strin return nil, fmt.Errorf("failed to determine token issuer: %w", err) } - // Self-issued tokens are delegated to the existing validator. if issuer == v.selfIssuer { return v.selfValidator.Validate(ctx, rawToken) } - // Look up the external issuer configuration. issuerConfig, ok := v.issuers[issuer] if !ok { return nil, fmt.Errorf("untrusted issuer: %q", issuer) @@ -198,23 +463,36 @@ func (v *MultiIssuerTokenValidator) validateExternalToken( return nil, fmt.Errorf("subject token is not a valid JWT: %w", err) } - jwks, err := v.resolveJWKS(ctx, issuerConfig) + jwks, err := v.lookupJWKS(ctx, issuerConfig) if err != nil { return nil, fmt.Errorf("failed to fetch JWKS for issuer %s: %w", issuerConfig.IssuerURL, err) } - standardClaims, extraClaims, err := verifySignature(parsedToken, jwks) + standardClaims, extraClaims, kidMatched, err := verifySignature(parsedToken, jwks) + if err != nil && !kidMatched { + // The token's kid isn't among the keys we have cached — possibly a + // legitimate rotation the issuer's cache hasn't caught up with yet + // (jwx's own background refresh floor is 15 minutes by default). + // Force an immediate re-fetch and retry once before giving up; a + // spoofed-kid attempt (kidMatched true) never reaches this branch, + // since a refresh can't change what signature the token was made + // with. + v.refreshOnUnknownKid(ctx, issuerConfig) + if refreshedJWKS, lookupErr := v.lookupJWKS(ctx, issuerConfig); lookupErr == nil { + standardClaims, extraClaims, _, err = verifySignature(parsedToken, refreshedJWKS) + } + } if err != nil { return nil, err } - // Validate standard claims: issuer must match, audience must contain the expected value, - // and the token must not be expired. + // Validate issuer and audience, tolerating externalClockSkewLeeway on + // nbf/iat/exp. Expiry is enforced strictly (no leeway) below. expected := jwt.Expected{ Issuer: issuerConfig.IssuerURL, AnyAudience: jwt.Audience{issuerConfig.ExpectedAudience}, } - if err := standardClaims.ValidateWithLeeway(expected, 0); err != nil { + if err := standardClaims.ValidateWithLeeway(expected, externalClockSkewLeeway); err != nil { return nil, fmt.Errorf("subject token claims validation failed: %w", err) } @@ -229,54 +507,269 @@ func (v *MultiIssuerTokenValidator) validateExternalToken( return nil, errors.New("subject token is missing required 'exp' claim") } - return buildValidatedClaims(standardClaims, extraClaims), nil + // An expired subject token is never acceptable, despite the leeway + // above: it cannot bound the delegated token's lifetime. + if standardClaims.Expiry.Time().Before(time.Now()) { + return nil, errors.New("subject token has expired") + } + + // Reject a subject token that is actually an ID token — see + // rejectIDTokenClaims's doc comment. + if err := rejectIDTokenClaims(extraClaims); err != nil { + return nil, err + } + + // selfIssuer (not issuerConfig.IssuerURL) is the correct comparison for + // may_act's optional "iss": that member identifies the namespace of + // may_act.sub, always compared against a ToolHive client ID regardless + // of which issuer validated the surrounding token. requireIss is true + // here — unlike the self-issued path, the external path cannot leave + // "iss" optional; see validateMayActShape's doc comment. + if err := validateMayActShape(extraClaims, v.selfIssuer, true); err != nil { + return nil, err + } + + claims := buildValidatedClaims(standardClaims, extraClaims) + + // Recorded for every external token, even a may_act-bearing one that + // leaves ExternalActor unset — without this, a may_act-authorized + // external token would be indistinguishable from a self-issued exchange + // downstream, which is backwards: this is the path that most needs an + // audit trail. Set from issuerConfig, already matched against the + // validated "iss" claim above — never from token content. + claims.ExternalIssuer = issuerConfig.IssuerURL + + // Set unconditionally, including for a may_act-bearing token: an issuer + // that can emit may_act but has no way to populate AllowedActors (e.g. + // Entra, Okta) would otherwise have no per-client containment on that + // path (see #5989). checkDelegationConsent enforces it against the + // authenticated client; this validator never sees that client ID. + claims.AllowedDelegateClients = issuerConfig.AllowedDelegateClients + + // A may_act claim is authoritative and enforced by checkDelegationConsent + // against the authenticated client — validateMayActShape above has + // already confirmed its own "iss" (if any) names this server, so + // may_act.sub is in ToolHive's own client namespace. AllowedActors below + // is skipped entirely when MayAct is set. Otherwise, the resolved actor + // claim (even "client_id") is in the EXTERNAL issuer's namespace and + // must match AllowedActors — it can never be compared against the + // authenticated ToolHive client directly. + if claims.MayAct == nil { + actor, err := resolveAllowedActor(issuerConfig, claims) + if err != nil { + return nil, err + } + claims.ExternalActor = actor + } + + return claims, nil } -// resolveJWKS returns the cached JWKS for an external issuer, fetching it if -// the cache is empty or expired. If the JWKS URL is not configured, it is first -// resolved via OIDC discovery. -func (v *MultiIssuerTokenValidator) resolveJWKS( - ctx context.Context, - issuerConfig *externalIssuerConfig, -) (*jose.JSONWebKeySet, error) { +// ensureRegistered resolves issuerConfig.jwksURL — via OIDC discovery on +// first use — and registers it with issuerConfig's own jwk.Cache (see +// registerOrRefresh for the Register-vs-Refresh decision). Discovery +// happens at most once per issuer for the life of the process; see +// externalIssuerConfig's jwksURL doc comment. +// +// The JWKS fetch is retried until one succeeds (fetched stays false +// otherwise), gated by jwksFetchFailureBackoff: key resolution runs before +// the subject token's signature is checked, so without this gate an +// authenticated client could force a real outbound attempt on every +// request to an issuer whose endpoint is down. The gate only applies +// before the first successful fetch — once fetched is true, a healthy +// issuer or one serving stale-but-valid keys through a later refresh +// failure is unaffected. While closed, the last error is replayed +// directly rather than retried. +// +// Every background refresh, not just the first request-triggered one, +// goes through the maxResponseBodySize-capped transport (see +// limitedBodyTransport) despite having no request or client authentication +// of its own — without that cap a compromised issuer could force an +// oversized allocation on every autonomous refresh, indefinitely. +func (v *MultiIssuerTokenValidator) ensureRegistered(ctx context.Context, issuerConfig *externalIssuerConfig) error { issuerConfig.mu.Lock() defer issuerConfig.mu.Unlock() - // The lock is intentionally held across the network fetch so concurrent - // validations of the same issuer don't trigger duplicate JWKS fetches. - // Return cached JWKS if still valid. - if issuerConfig.jwks != nil && time.Now().Before(issuerConfig.jwksExp) { - return issuerConfig.jwks, nil + if issuerConfig.fetched { + return nil + } + if time.Since(issuerConfig.fetchFailedAt) < jwksFetchFailureBackoff { + return issuerConfig.fetchErr } - // Cache expired — clear the discovered URL so we re-discover on next fetch. - // This handles the (rare) case where an issuer rotates its JWKS endpoint URL. - // The explicitly configured JWKSURL (from TrustedIssuer) is preserved. - if issuerConfig.JWKSURL == "" { - issuerConfig.jwksURL = "" + if err := v.registerOrRefresh(ctx, issuerConfig); err != nil { + issuerConfig.fetchErr = err + issuerConfig.fetchFailedAt = time.Now() + return err } + issuerConfig.fetched = true + issuerConfig.fetchErr = nil + return nil +} + +// registerOrRefresh performs the actual discovery/registration/fetch +// attempt for issuerConfig; ensureRegistered holds issuerConfig.mu across +// this call, single-flighting it per issuer so concurrent validations +// don't pile up N redundant fetches. +// +// Whether jwksURL is already registered is asked of +// issuerConfig.jwksCache.IsRegistered directly, never remembered in a +// field: Register's own registration step is a channel send to the +// cache's backend goroutine and can fail after enqueue but before receipt +// (context deadline), in which case nothing was actually registered. A +// locally remembered "we called Register" flag can't distinguish that +// from "registered, only the fetch failed", and would wrongly keep +// retrying via Refresh — which errors on a URL the cache never heard of — +// forever after. Asking the cache directly is authoritative either way. +// +// IsRegistered makes no network call but isn't free: it's a round-trip +// over that same channel, so it blocks if the backend is busy and returns +// false (not an error) on context expiry. Its own timeout below matters +// for that reason — a false from an expired context just routes to +// Register, whose "already registered" error is transient and absorbed by +// ensureRegistered's backoff gate. +func (v *MultiIssuerTokenValidator) registerOrRefresh(ctx context.Context, issuerConfig *externalIssuerConfig) error { + // Detach from the caller's request context throughout this function: net/http + // cancels ctx when the client disconnects, and this runs before the subject + // token's signature is even checked, so an aborted connection must not cut off + // work other in-flight validations of this issuer are waiting on (mu, held by + // the caller), nor let repeating the abort drive unbounded outbound requests + // to the external IdP. + detached := context.WithoutCancel(ctx) - // Discover the JWKS URL if not yet resolved. if issuerConfig.jwksURL == "" { - jwksURL, err := v.discoverJWKSURL(ctx, issuerConfig.IssuerURL) + discoverCtx, cancel := context.WithTimeout(detached, httpTimeout) + jwksURL, err := v.discoverJWKSURL(discoverCtx, issuerConfig) + cancel() if err != nil { - return nil, fmt.Errorf("OIDC discovery failed for %s: %w", issuerConfig.IssuerURL, err) + return fmt.Errorf("OIDC discovery failed for %s: %w", issuerConfig.IssuerURL, err) } issuerConfig.jwksURL = jwksURL } - // Fetch and cache the JWKS. - jwks, err := v.fetchJWKS(ctx, issuerConfig.jwksURL) + // Validate the JWKS URL here, in the single choke point every + // registration passes through — whether it was hand-configured on + // TrustedIssuer or just discovered above. A configured JWKSURL never + // reaches discoverJWKSURL, so checking only there would leave + // hand-configured URLs unvalidated. + if err := ValidateJWKSURL(issuerConfig.jwksURL, issuerConfig.InsecureAllowHTTP, issuerConfig.AllowPrivateIPs); err != nil { + return fmt.Errorf("jwks_url for issuer %s is invalid: %w", issuerConfig.IssuerURL, err) + } + + registeredCtx, cancel := context.WithTimeout(detached, httpTimeout) + registered := issuerConfig.jwksCache.IsRegistered(registeredCtx, issuerConfig.jwksURL) + cancel() + + if registered { + // Already registered with this issuer's own cache, on a prior call + // whose own fetch never completed successfully — the only way this + // can be true, now that each issuer has its own cache. Register + // would error on an already-tracked URL, so retry via Refresh + // instead. + fetchCtx, cancel := context.WithTimeout(detached, httpTimeout) + defer cancel() + if _, err := issuerConfig.jwksCache.Refresh(fetchCtx, issuerConfig.jwksURL); err != nil { + return fmt.Errorf("failed to fetch JWKS for issuer %s: %w", issuerConfig.IssuerURL, err) + } + return nil + } + + // A newly created httprc.Resource is always scheduled to fetch + // immediately, so Register's own default WithWaitReady(true) blocks on + // that single automatic fetch. An explicit Refresh call right here would + // race it and issue a genuine second outbound request — that's why the + // registered branch above, not this one, is where Refresh is used. + fetchCtx, cancel := context.WithTimeout(detached, httpTimeout) + defer cancel() + if err := issuerConfig.jwksCache.Register(fetchCtx, issuerConfig.jwksURL, + jwk.WithHTTPClient(issuerConfig.httpClient), jwk.WithConstantInterval(jwksRefreshInterval)); err != nil { + return fmt.Errorf("failed to register JWKS for issuer %s: %w", issuerConfig.IssuerURL, err) + } + return nil +} + +// lookupJWKS returns issuerConfig's current JWKS, registering and fetching it +// first if this is the first use (see ensureRegistered). jwk.Cache serves the +// last successfully fetched Set even while a later background refresh is +// failing (httprc only stores a value after a successful fetch), so a +// transient outage at an issuer that has already been reached once no longer +// surfaces as a validation failure. +// Each call converts the cached Set into a fresh *jose.JSONWebKeySet — the +// two libraries' key representations aren't shared, so nothing here is +// visible to, or mutable by, any other concurrent caller. +func (v *MultiIssuerTokenValidator) lookupJWKS( + ctx context.Context, + issuerConfig *externalIssuerConfig, +) (*jose.JSONWebKeySet, error) { + if err := v.ensureRegistered(ctx, issuerConfig); err != nil { + return nil, err + } + + set, err := issuerConfig.jwksCache.Lookup(ctx, issuerConfig.jwksURL) + if err != nil { + return nil, fmt.Errorf("failed to lookup JWKS: %w", err) + } + + jwks, err := bridgeJWKSet(set) if err != nil { return nil, err } - issuerConfig.jwks = jwks - issuerConfig.jwksExp = time.Now().Add(jwksCacheTTL) + if len(jwks.Keys) == 0 { + return nil, errors.New("JWKS contains no keys") + } + if len(jwks.Keys) > maxJWKSKeys { + return nil, fmt.Errorf("JWKS contains too many keys: %d (max %d)", len(jwks.Keys), maxJWKSKeys) + } return jwks, nil } +// bridgeJWKSet converts a jwx jwk.Set into the go-jose jose.JSONWebKeySet +// shape verifySignature works with, by round-tripping through the standard +// JWKS JSON both types serialize to and from. This is the simplest correct +// conversion between the two libraries' key representations — it doesn't +// require hand-mapping every key type's fields (RSA, EC, OKP, ...) between +// jwk.Key and jose.JSONWebKey. +func bridgeJWKSet(set jwk.Set) (*jose.JSONWebKeySet, error) { + raw, err := json.Marshal(set) + if err != nil { + return nil, fmt.Errorf("failed to marshal JWKS: %w", err) + } + var jwks jose.JSONWebKeySet + if err := json.Unmarshal(raw, &jwks); err != nil { + return nil, fmt.Errorf("failed to parse JWKS: %w", err) + } + return &jwks, nil +} + +// refreshOnUnknownKid forces issuerConfig's own jwk.Cache to re-fetch its +// JWKS immediately, ahead of jwx's own background refresh schedule, when a +// subject token names a kid the last cached JWKS doesn't have — the +// situation a legitimate key rotation produces. Gated by minKidRefreshInterval +// and single-flighted via issuerConfig.mu, the same mutex ensureRegistered +// uses for its own initial fetch. +// +// Errors are logged, not returned: the caller (validateExternalToken) has +// already failed signature verification once and will simply fail again if +// the refresh didn't produce a usable key, which is the correct outcome for +// a genuinely invalid token. +func (*MultiIssuerTokenValidator) refreshOnUnknownKid(ctx context.Context, issuerConfig *externalIssuerConfig) { + issuerConfig.mu.Lock() + defer issuerConfig.mu.Unlock() + + if time.Since(issuerConfig.lastKidRefresh) < minKidRefreshInterval { + return + } + issuerConfig.lastKidRefresh = time.Now() + + fetchCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*httpTimeout) + defer cancel() + if _, err := issuerConfig.jwksCache.Refresh(fetchCtx, issuerConfig.jwksURL); err != nil { + slog.Debug("JWKS refresh on unknown kid failed", "issuer", issuerConfig.IssuerURL, "error", err) + } +} + // peekIssuer parses a JWT without signature verification to extract the "iss" claim. func peekIssuer(rawToken string) (string, error) { token, err := jwt.ParseSigned(rawToken, allowedSignatureAlgorithms) @@ -298,16 +791,24 @@ func peekIssuer(rawToken string) (string, error) { // discoverJWKSURL performs OIDC discovery to resolve the JWKS URL for an issuer. // It fetches the OpenID Connect discovery document at {issuerURL}/.well-known/openid-configuration -// and extracts the jwks_uri field. -func (v *MultiIssuerTokenValidator) discoverJWKSURL(ctx context.Context, issuerURL string) (string, error) { - discoveryURL := issuerURL + "/.well-known/openid-configuration" +// and extracts the jwks_uri field, using issuerConfig's own dedicated HTTP client. +// +// IssuerURL itself may legitimately carry a trailing slash (see +// validateTrustedIssuerURL in pkg/authserver/config.go — Microsoft Entra ID +// v1 is one real-world example), so it is trimmed before the well-known path +// is appended here, per OIDC Discovery §4.1. The comparison against the +// discovery document's own "issuer" below intentionally uses the untrimmed +// IssuerURL: per §4.3 that comparison must be exact, since it stands in for +// the eventual match against the token's "iss". +func (*MultiIssuerTokenValidator) discoverJWKSURL(ctx context.Context, issuerConfig *externalIssuerConfig) (string, error) { + discoveryURL := strings.TrimSuffix(issuerConfig.IssuerURL, "/") + "/.well-known/openid-configuration" req, err := http.NewRequestWithContext(ctx, http.MethodGet, discoveryURL, nil) if err != nil { return "", fmt.Errorf("failed to create discovery request: %w", err) } - resp, err := v.httpClient.Do(req) + resp, err := issuerConfig.httpClient.Do(req) if err != nil { return "", fmt.Errorf("discovery request failed: %w", err) } @@ -328,85 +829,176 @@ func (v *MultiIssuerTokenValidator) discoverJWKSURL(ctx context.Context, issuerU return "", fmt.Errorf("failed to parse discovery document: %w", err) } - if doc.Issuer != issuerURL { - return "", fmt.Errorf("discovery document issuer %q does not match expected issuer %q", doc.Issuer, issuerURL) + if doc.Issuer != issuerConfig.IssuerURL { + return "", fmt.Errorf("discovery document issuer %q does not match expected issuer %q", doc.Issuer, issuerConfig.IssuerURL) } if doc.JWKSURI == "" { return "", fmt.Errorf("discovery document missing 'jwks_uri'") } - if !v.insecureSkipJWKSURLValidation { - if err := validateJWKSURL(doc.JWKSURI); err != nil { - return "", fmt.Errorf("discovered jwks_uri is invalid: %w", err) - } - } - + // The returned URL is validated by the caller (ensureRegistered), which is + // the single choke point covering both discovered and pre-configured + // JWKS URLs. return doc.JWKSURI, nil } -// validateJWKSURL checks that the JWKS URL uses HTTPS and is not a private/loopback address. -// This prevents SSRF attacks where a compromised discovery document points to internal services. -func validateJWKSURL(jwksURL string) error { +// ValidateJWKSURL checks that jwksURL parses, has a host, uses HTTPS unless +// insecureAllowHTTP permits plain HTTP — and only exactly the "http" scheme, +// not any other non-https scheme such as "file" or "ftp" — and, when the +// host is an IP literal, is not a private or loopback address unless +// allowPrivateIPs permits that. Both flags come from the specific +// TrustedIssuer being fetched (see ensureRegistered), never from a validator-wide +// or self-issuer setting. This prevents SSRF attacks where a compromised +// discovery document — or a hand-configured jwks_url — points to internal +// services. +// +// This is the single implementation shared by the runtime choke point above +// (ensureRegistered, on every fetch) and pkg/authserver/config.go's config-time +// check (validateJWKSEndpointURL): the two must not drift out of sync, or a +// laxer runtime check would silently defeat the config-time guard. +func ValidateJWKSURL(jwksURL string, insecureAllowHTTP, allowPrivateIPs bool) error { u, err := url.Parse(jwksURL) if err != nil { return fmt.Errorf("invalid URL: %w", err) } - if u.Scheme != "https" { + if u.Host == "" { + return errors.New("host is required") + } + + // Unlike issuer_url, a jwks_url carrying userinfo would actually work — + // net/http turns it into a Basic auth header on every JWKS fetch — which + // is precisely why it is rejected rather than tolerated: it would put a + // live credential in the RunConfig, in this function's error strings, and + // in any log that quotes the URL. A JWKS endpoint is public by + // definition (it serves verification keys), so there is no legitimate + // reason to authenticate to one. + if u.User != nil { + return errors.New("must not contain userinfo (credentials in the URL)") + } + + if u.Scheme != "https" && (u.Scheme != "http" || !insecureAllowHTTP) { return fmt.Errorf("must use HTTPS, got %q", u.Scheme) } host := u.Hostname() ip := net.ParseIP(host) - if ip != nil && isDisallowedIP(ip) { + if ip != nil && !allowPrivateIPs && networking.IsPrivateIP(ip) { return errors.New("must not point to a private or loopback address") } return nil } -// isDisallowedIP reports whether an IP must not be dialed when fetching -// external OIDC metadata, blocking SSRF to internal/metadata addresses. -func isDisallowedIP(ip net.IP) bool { - return ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || - ip.IsLinkLocalMulticast() || ip.IsUnspecified() -} - -// fetchJWKS fetches a JSON Web Key Set from the given URL. -func (v *MultiIssuerTokenValidator) fetchJWKS(ctx context.Context, jwksURL string) (*jose.JSONWebKeySet, error) { - req, err := http.NewRequestWithContext(ctx, http.MethodGet, jwksURL, nil) - if err != nil { - return nil, fmt.Errorf("failed to create JWKS request: %w", err) +// validateTrustedIssuer checks a single TrustedIssuer for structural validity +// before it is admitted into issuers: required fields, no collision with +// selfIssuer or an already-registered issuer, an ActorClaim that +// resolveAllowedActor can actually read from Extra (or ClientID), and a +// well-formed AllowedDelegateClients (non-empty, no empty entries, and the +// wildcard anyDelegateClient never combined with specific client IDs). +// +// Error messages name TrustedIssuer's wire keys (issuer_url, +// expected_audience, actor_claim), not its Go field names: TrustedIssuer is +// the serialized RunConfig.TrustedIssuers schema (see its doc comment), so an +// operator's YAML uses these keys and should see them echoed back, not Go +// identifiers they never wrote. +func validateTrustedIssuer(ti TrustedIssuer, selfIssuer string, issuers map[string]*externalIssuerConfig) error { + if ti.IssuerURL == "" { + return errors.New("issuer_url is required") + } + if ti.ExpectedAudience == "" { + return fmt.Errorf("issuer_url %q: expected_audience is required", ti.IssuerURL) + } + if ti.IssuerURL == selfIssuer { + return fmt.Errorf("issuer_url %q: must not equal the authorization server's own issuer; "+ + "self-issued tokens are already handled separately", ti.IssuerURL) + } + if _, dup := issuers[ti.IssuerURL]; dup { + return fmt.Errorf("issuer_url %q: configured more than once", ti.IssuerURL) + } + if ti.ActorClaim != "" && slices.Contains(actorClaimsNotInExtra, ti.ActorClaim) { + return fmt.Errorf( + "issuer_url %q: actor_claim %q is not supported "+ + `(use "client_id" or a non-registered claim such as "azp", "appid", "cid")`, + ti.IssuerURL, ti.ActorClaim) + } + if len(ti.AllowedDelegateClients) == 0 { + return fmt.Errorf( + "issuer_url %q: allowed_delegate_clients is required; set it to [%q] to permit any ToolHive "+ + "confidential client holding the token-exchange grant, or list specific client IDs to bind "+ + "delegation to them — an issuer with no entries can never authorize a delegated exchange", + ti.IssuerURL, anyDelegateClient) + } + if slices.Contains(ti.AllowedDelegateClients, "") { + return fmt.Errorf( + "issuer_url %q: allowed_delegate_clients must not contain an empty client ID", ti.IssuerURL) + } + if slices.Contains(ti.AllowedDelegateClients, anyDelegateClient) && len(ti.AllowedDelegateClients) > 1 { + return fmt.Errorf( + "issuer_url %q: allowed_delegate_clients must not combine the wildcard %q with specific client IDs", + ti.IssuerURL, anyDelegateClient) + } + // AllowPrivateIPs without a hand-configured jwks_url would let OIDC + // discovery — a document fetched from, and thus influenceable by, the + // external issuer itself — choose the private target the dial is + // allowed to reach. Requiring jwks_url pins that target to + // operator-supplied config. Mirrors the config-time check in + // pkg/authserver/config.go's validateTrustedIssuers; duplicated here so + // a caller that builds the validator directly (factory, tests) without + // running Config.Validate cannot bypass it. + if ti.AllowPrivateIPs && ti.JWKSURL == "" { + return fmt.Errorf( + "issuer_url %q: allow_private_ips requires jwks_url to be set explicitly; "+ + "otherwise OIDC discovery — fetched from the external issuer — would choose the private target", + ti.IssuerURL) } + return nil +} - resp, err := v.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("JWKS request failed: %w", err) - } - defer func() { _ = resp.Body.Close() }() +// looksLikeResourceIdentifier reports whether aud is shaped like a resource/API +// identifier (an absolute URI, e.g. "https://api.example.com" or +// "api://") rather than a bare opaque string or GUID, which is more +// likely a client ID. This is a heuristic warning signal only — Entra v1 +// legitimately uses a bare app-ID GUID as an access token's "aud" (see +// TrustedIssuer.ExpectedAudience's doc comment), so a bare value is not +// rejected, only flagged for the operator to double-check. +func looksLikeResourceIdentifier(aud string) bool { + return strings.Contains(aud, "://") +} - if resp.StatusCode != http.StatusOK { - _, _ = io.Copy(io.Discard, resp.Body) - return nil, fmt.Errorf("JWKS endpoint returned status %d", resp.StatusCode) +// resolveAllowedActor resolves the issuer's configured actor claim from +// claims and checks it against issuerConfig.AllowedActors, returning the +// matched value on success. Called only when the subject token carries no +// may_act claim — see validateExternalToken. +func resolveAllowedActor(issuerConfig *externalIssuerConfig, claims *ValidatedClaims) (string, error) { + claimName := issuerConfig.ActorClaim + if claimName == "" { + claimName = defaultActorClaim } - body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodySize)) - if err != nil { - return nil, fmt.Errorf("failed to read JWKS response: %w", err) + // "client_id" is routed to ValidatedClaims.ClientID by assignClaim, so it + // never appears in Extra; without this fallback a plausible operator + // config (ActorClaim: "client_id") would silently reject all traffic. + var raw any + if claimName == "client_id" { + raw = claims.ClientID + } else { + raw = claims.Extra[claimName] } - var jwks jose.JSONWebKeySet - if err := json.Unmarshal(body, &jwks); err != nil { - return nil, fmt.Errorf("failed to parse JWKS: %w", err) + actor, ok := raw.(string) + if !ok || actor == "" { + return "", fmt.Errorf( + "subject token from issuer %q is missing or has an invalid %q claim required for delegation consent", + issuerConfig.IssuerURL, claimName) } - if len(jwks.Keys) == 0 { - return nil, errors.New("JWKS contains no keys") - } - if len(jwks.Keys) > maxJWKSKeys { - return nil, fmt.Errorf("JWKS contains too many keys: %d (max %d)", len(jwks.Keys), maxJWKSKeys) + if !slices.Contains(issuerConfig.AllowedActors, actor) { + return "", fmt.Errorf( + "subject token from issuer %q names actor %q in claim %q, which is not in the allowed actors list", + issuerConfig.IssuerURL, actor, claimName) } - return &jwks, nil + return actor, nil } diff --git a/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go b/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go index c357de4105..62b7c7726e 100644 --- a/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go +++ b/pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go @@ -4,11 +4,16 @@ package tokenexchange import ( + "bytes" "context" "encoding/json" "fmt" + "io" + "log/slog" "net/http" "net/http/httptest" + "strings" + "sync" "sync/atomic" "testing" "time" @@ -17,6 +22,8 @@ import ( "github.com/go-jose/go-jose/v4/jwt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/networking" ) const ( @@ -41,34 +48,6 @@ func startJWKSServer(t *testing.T, tj *testJWKS) *httptest.Server { return srv } -// startDiscoveryServer creates a test HTTP server that serves both OIDC discovery -// and JWKS endpoints, simulating an external OIDC provider. -func startDiscoveryServer(t *testing.T, tj *testJWKS) *httptest.Server { - t.Helper() - - mux := http.NewServeMux() - mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { - // The issuer and jwks_uri must use the test server's own base URL, - // which we don't know until the server starts. We use the Host header - // to construct them. The issuer must match the requested issuer URL - // (which the test configures as the discovery server's own URL). - base := "http://" + r.Host - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]string{ - "issuer": base, - "jwks_uri": base + "/jwks", - }) - }) - mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(tj.publicJWKS()) - }) - - srv := httptest.NewServer(mux) - t.Cleanup(srv.Close) - return srv -} - // newMultiValidator creates a MultiIssuerTokenValidator configured for testing. // The external JWKS URL is pre-resolved (no discovery needed) unless jwksURL is empty. func newMultiValidator( @@ -81,9 +60,18 @@ func newMultiValidator( selfValidator, err := NewSelfIssuedTokenValidator(selfJWKS.publicJWKS(), testIssuer, []string{testIssuer}) require.NoError(t, err) - v, err := NewMultiIssuerTokenValidator(selfValidator, testIssuer, trustedIssuers) + // Copy rather than mutate the caller's slice: give every test issuer + // permissive flags so its httptest discovery/JWKS server, which runs on + // loopback over plain HTTP, is reachable. + issuers := make([]TrustedIssuer, len(trustedIssuers)) + for i, ti := range trustedIssuers { + ti.InsecureAllowHTTP = true + ti.AllowPrivateIPs = true + issuers[i] = ti + } + + v, err := NewMultiIssuerTokenValidator(selfValidator, testIssuer, issuers) require.NoError(t, err) - v.insecureSkipJWKSURLValidation = true // Allow HTTP test servers return v } @@ -119,9 +107,10 @@ func TestMultiIssuerTokenValidator_Validate(t *testing.T) { { name: "self-issued token routes to self validator", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, }}, token: func(t *testing.T) string { t.Helper() @@ -134,20 +123,24 @@ func TestMultiIssuerTokenValidator_Validate(t *testing.T) { assert.Equal(t, []string{testIssuer}, vc.Audience) assert.Equal(t, "Test User", vc.Name) assert.Equal(t, "test@example.com", vc.Email) + assert.Empty(t, vc.ExternalIssuer, "self-issued path must never set ExternalIssuer") }, }, { name: "external token accepted", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, }}, token: func(t *testing.T) string { t.Helper() return externalJWKS.signToken(t, externalClaims(), map[string]interface{}{ "name": "External User", "email": "ext@keycloak.example.com", + "azp": "ext-agent", }) }, check: func(t *testing.T, vc *ValidatedClaims) { @@ -160,292 +153,1520 @@ func TestMultiIssuerTokenValidator_Validate(t *testing.T) { assert.Equal(t, "ext@keycloak.example.com", vc.Email) assert.False(t, vc.Expiry.IsZero()) assert.False(t, vc.IssuedAt.IsZero()) + assert.Equal(t, "ext-agent", vc.ExternalActor, "actor claim matched via default azp") + assert.Equal(t, testExternalIssuer, vc.ExternalIssuer) }, }, { - name: "external token wrong audience", + name: "external token surfaces configured AllowedDelegateClients", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{"toolhive-agent-a", "toolhive-agent-b"}, }}, token: func(t *testing.T) string { t.Helper() - claims := externalClaims() - claims.Audience = jwt.Audience{"wrong-audience"} - return externalJWKS.signToken(t, claims, nil) + return externalJWKS.signToken(t, externalClaims(), map[string]interface{}{"azp": "ext-agent"}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + assert.Equal(t, "ext-agent", vc.ExternalActor) + assert.Equal(t, []string{"toolhive-agent-a", "toolhive-agent-b"}, vc.AllowedDelegateClients, + "the validator surfaces the issuer's configured allowlist for the handler to enforce") }, - wantErr: true, - errContains: "claims validation failed", }, { - name: "unknown issuer rejected", + // #5989 hardening: AllowedDelegateClients is required (validateTrustedIssuer + // rejects an empty one), so the permissive case is now the wildcard, + // declared explicitly, rather than an absent field. + name: "external token surfaces the wildcard AllowedDelegateClients when the issuer opts into it", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, }}, token: func(t *testing.T) string { t.Helper() - claims := externalClaims() - claims.Issuer = "https://evil.example.com" - return externalJWKS.signToken(t, claims, nil) + return externalJWKS.signToken(t, externalClaims(), map[string]interface{}{"azp": "ext-agent"}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + assert.Equal(t, []string{anyDelegateClient}, vc.AllowedDelegateClients) }, - wantErr: true, - errContains: "untrusted issuer", }, { - name: "external token bad signature", + // #5989 fix: an issuer emitting may_act with no way to populate + // AllowedActors (e.g. Entra, Okta) previously had no per-client + // containment at all. AllowedDelegateClients must now surface on + // the may_act path too, for checkDelegationConsent to enforce. + name: "external token with may_act still surfaces configured AllowedDelegateClients", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{"toolhive-agent-a"}, }}, token: func(t *testing.T) string { t.Helper() - // Sign with a different key than the one the JWKS server serves. - wrongJWKS := newTestJWKS(t) - return wrongJWKS.signToken(t, externalClaims(), nil) + return externalJWKS.signToken(t, externalClaims(), + map[string]any{"may_act": map[string]any{"sub": "some-toolhive-client", "iss": testIssuer}}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + require.NotNil(t, vc.MayAct) + assert.Equal(t, []string{"toolhive-agent-a"}, vc.AllowedDelegateClients, + "may_act bypasses the AllowedActors allowlist, but not per-client containment") }, - wantErr: true, - errContains: "signature verification failed", }, { - name: "self-issued token signed by external key fails", + name: "external token custom actor claim appid accepted", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + ActorClaim: "appid", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, }}, token: func(t *testing.T) string { t.Helper() - // Token claims say iss=self, but signed by the external key. - // Routes to self validator, which rejects the signature. - return externalJWKS.signToken(t, validClaims(), nil) + return externalJWKS.signToken(t, externalClaims(), map[string]any{"appid": "ext-agent"}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + assert.Equal(t, "ext-agent", vc.ExternalActor) + }, + }, + { + name: "external token custom actor claim cid accepted", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + ActorClaim: "cid", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{"cid": "ext-agent"}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + assert.Equal(t, "ext-agent", vc.ExternalActor) + }, + }, + { + name: "external token actor claim client_id resolves from ClientID field", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + ActorClaim: "client_id", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + // "client_id" is routed to ValidatedClaims.ClientID by assignClaim, so + // it never lands in Extra — resolveAllowedActor must fall back to + // reading the structured field instead. + return externalJWKS.signToken(t, externalClaims(), map[string]any{"client_id": "ext-agent"}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + assert.Equal(t, "ext-agent", vc.ClientID) + assert.Equal(t, "ext-agent", vc.ExternalActor) + }, + }, + { + name: "external token actor not in allowed actors rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": "some-other-client"}) }, wantErr: true, - errContains: "signature verification failed", + errContains: "not in the allowed actors list", }, { - name: "external token missing subject", + name: "external token missing actor claim rejected", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, }}, token: func(t *testing.T) string { t.Helper() - claims := externalClaims() - claims.Subject = "" - return externalJWKS.signToken(t, claims, nil) + return externalJWKS.signToken(t, externalClaims(), nil) }, wantErr: true, - errContains: "missing required 'sub' claim", + errContains: "required for delegation consent", }, { - name: "external token expired", + name: "external token actor claim is a number rejected", trustedIssuers: []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, }}, token: func(t *testing.T) string { t.Helper() - claims := externalClaims() - claims.Expiry = jwt.NewNumericDate(time.Now().Add(-time.Hour)) - claims.IssuedAt = jwt.NewNumericDate(time.Now().Add(-2 * time.Hour)) - return externalJWKS.signToken(t, claims, nil) + return externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": 123}) }, wantErr: true, - errContains: "claims validation failed", + errContains: "required for delegation consent", }, { - name: "malformed token", - trustedIssuers: nil, - token: func(_ *testing.T) string { - return "not-a-jwt" + name: "external token actor claim is a bool rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": true}) }, wantErr: true, - errContains: "failed to determine token issuer", + errContains: "required for delegation consent", }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - validator := newMultiValidator(t, selfJWKS, tt.trustedIssuers) - rawToken := tt.token(t) - - result, err := validator.Validate(context.Background(), rawToken) - - if tt.wantErr { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.errContains) - assert.Nil(t, result) - return - } - - require.NoError(t, err) - require.NotNil(t, result) - if tt.check != nil { - tt.check(t, result) - } - }) - } -} - -func TestMultiIssuerTokenValidator_OIDCDiscovery(t *testing.T) { - t.Parallel() - - selfJWKS := newTestJWKS(t) - externalJWKS := newTestJWKS(t) - discoveryServer := startDiscoveryServer(t, externalJWKS) - - // Configure the trusted issuer WITHOUT a JWKS URL, forcing OIDC discovery. - // The discovery server's URL is used as the issuer URL so that the - // /.well-known/openid-configuration endpoint is reachable. - trustedIssuers := []TrustedIssuer{{ - IssuerURL: discoveryServer.URL, - ExpectedAudience: testExternalAudience, - // JWKSURL intentionally left empty to trigger discovery. - }} - - validator := newMultiValidator(t, selfJWKS, trustedIssuers) - - // Sign a token with the external key, using the discovery server's URL as issuer. - claims := jwt.Claims{ - Subject: "discovered-user", - Issuer: discoveryServer.URL, - Audience: jwt.Audience{testExternalAudience}, - Expiry: jwt.NewNumericDate(time.Now().Add(time.Hour)), - IssuedAt: jwt.NewNumericDate(time.Now()), - NotBefore: jwt.NewNumericDate(time.Now().Add(-time.Minute)), - ID: "jti-disc-001", - } - rawToken := externalJWKS.signToken(t, claims, nil) - - result, err := validator.Validate(context.Background(), rawToken) - require.NoError(t, err) - require.NotNil(t, result) - assert.Equal(t, "discovered-user", result.Subject) - assert.Equal(t, discoveryServer.URL, result.Issuer) -} - -func TestMultiIssuerTokenValidator_JWKSCaching(t *testing.T) { - t.Parallel() - - selfJWKS := newTestJWKS(t) - externalJWKS := newTestJWKS(t) - - // Track how many times the JWKS endpoint is hit. - var fetchCount atomic.Int32 - mux := http.NewServeMux() - mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { - fetchCount.Add(1) - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(externalJWKS.publicJWKS()) - }) - jwksServer := httptest.NewServer(mux) - t.Cleanup(jwksServer.Close) - - trustedIssuers := []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", - }} - - validator := newMultiValidator(t, selfJWKS, trustedIssuers) - - // Validate two tokens — the JWKS should be fetched only once (cached). - for i := range 2 { - claims := externalClaims() - claims.ID = fmt.Sprintf("jti-cache-%d", i) - rawToken := externalJWKS.signToken(t, claims, nil) - - result, err := validator.Validate(context.Background(), rawToken) - require.NoError(t, err) - require.NotNil(t, result) - assert.Equal(t, "ext-user-456", result.Subject) - } - - assert.Equal(t, int32(1), fetchCount.Load(), "JWKS should be fetched only once due to caching") -} - -func TestNewMultiIssuerTokenValidator_EmptyAudience(t *testing.T) { - t.Parallel() - selfJWKS := newTestJWKS(t) - selfValidator, err := NewSelfIssuedTokenValidator(selfJWKS.publicJWKS(), testIssuer, []string{testIssuer}) - require.NoError(t, err) - _, err = NewMultiIssuerTokenValidator(selfValidator, testIssuer, []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: "", - }}) - require.Error(t, err) - assert.Contains(t, err.Error(), "ExpectedAudience is required") -} - -func TestMultiIssuerTokenValidator_DiscoveryFailures(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - handler func(w http.ResponseWriter, r *http.Request) - }{ { - name: "non-200", - handler: func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusInternalServerError) + name: "external token actor claim is an array rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": []string{"ext-agent"}}) }, + wantErr: true, + errContains: "required for delegation consent", }, { - name: "malformed doc", - handler: func(w http.ResponseWriter, _ *http.Request) { - w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte("{not-json")) + name: "external token actor claim is a nested object rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), + map[string]any{"azp": map[string]any{"id": "ext-agent"}}) }, + wantErr: true, + errContains: "required for delegation consent", }, { - name: "missing jwks_uri", - handler: func(w http.ResponseWriter, r *http.Request) { - // Issuer must match so discovery reaches the jwks_uri check. - w.Header().Set("Content-Type", "application/json") - _ = json.NewEncoder(w).Encode(map[string]string{ - "issuer": "http://" + r.Host, - }) + name: "external token actor claim empty string rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": ""}) }, + wantErr: true, + errContains: "required for delegation consent", }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() + { + name: "external token no may_act and empty allowed actors rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + // AllowedActors intentionally empty. + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": "ext-agent"}) + }, + wantErr: true, + errContains: "not in the allowed actors list", + }, + { + name: "external token with may_act and empty allowed actors accepted", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + // AllowedActors intentionally empty: may_act is authoritative and + // skips the allowlist entirely. + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), + map[string]any{"may_act": map[string]any{"sub": "some-toolhive-client", "iss": testIssuer}}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + require.NotNil(t, vc.MayAct) + assert.Equal(t, "some-toolhive-client", vc.MayAct.Sub) + assert.Empty(t, vc.ExternalActor, "allowlist is skipped whenever may_act is present") + assert.Equal(t, testExternalIssuer, vc.ExternalIssuer, + "provenance must still be recorded on the may_act path, which bypasses the allowlist") + }, + }, + { + // #5989 fix: the external path cannot leave may_act.iss optional + // the way the self-issued path does (see + // TestSelfIssuedTokenValidator_Validate's "token with may_act + // claim extracts MayAct" for the self-issued equivalent, which + // omits iss and is accepted). Without this, an external issuer + // could authorize ANY ToolHive client via a bare may_act.sub, + // bypassing AllowedActors/AllowedDelegateClients entirely. + name: "external token may_act missing iss rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), + map[string]any{"may_act": map[string]any{"sub": "some-toolhive-client"}}) + }, + wantErr: true, + errContains: "missing required 'iss'", + }, + { + name: "external token may_act wins even when azp is not allowlisted", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"someone-else"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "not-in-the-allowlist", + "may_act": map[string]any{"sub": "some-toolhive-client", "iss": testIssuer}, + }) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + require.NotNil(t, vc.MayAct) + assert.Empty(t, vc.ExternalActor) + assert.Equal(t, testExternalIssuer, vc.ExternalIssuer) + }, + }, + { + name: "external token malformed may_act — JSON string, not object — rejected outright", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + // azp is allowlisted, so a regression that fell through to the + // allowlist instead of rejecting would wrongly accept this. + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "ext-agent", + "may_act": "agent-a", + }) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + name: "external token malformed may_act — non-string sub — rejected outright", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "ext-agent", + "may_act": map[string]any{"sub": 123}, + }) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + name: "external token malformed may_act — empty sub — rejected outright", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "ext-agent", + "may_act": map[string]any{"sub": ""}, + }) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + name: "external token malformed may_act — array sub — rejected outright", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "ext-agent", + "may_act": map[string]any{"sub": []string{"agent-a"}}, + }) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + name: "external token malformed may_act — object with no sub key — rejected outright", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "ext-agent", + "may_act": map[string]any{"subject": "agent-a"}, + }) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + // Proves validateMayActShape is passed v.selfIssuer, not + // issuerConfig.IssuerURL: may_act.sub is always compared against + // a ToolHive client ID by checkDelegationConsent, regardless of + // which external issuer validated the surrounding token, so its + // optional iss must be checked against THIS server's own + // issuer. + name: "external token may_act.iss matching this server's own issuer accepted", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), + map[string]any{"may_act": map[string]any{"sub": "some-toolhive-client", "iss": testIssuer}}) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + require.NotNil(t, vc.MayAct) + assert.Equal(t, testIssuer, vc.MayAct.Iss) + }, + }, + { + // The external issuer's own URL is never a valid may_act.iss + // value: a regression that compared against issuerConfig.IssuerURL + // instead of v.selfIssuer would wrongly accept this. + name: "external token may_act.iss matching the external issuer's own URL rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), + map[string]any{"may_act": map[string]any{"sub": "some-toolhive-client", "iss": testExternalIssuer}}) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + name: "external token carrying c_hash (an ID token) rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "ext-agent", + "c_hash": "some-hash-value", + }) + }, + wantErr: true, + errContains: "'c_hash'", + }, + { + name: "external token wrong audience", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + claims := externalClaims() + claims.Audience = jwt.Audience{"wrong-audience"} + return externalJWKS.signToken(t, claims, nil) + }, + wantErr: true, + errContains: "claims validation failed", + }, + { + name: "unknown issuer rejected", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + claims := externalClaims() + claims.Issuer = "https://evil.example.com" + return externalJWKS.signToken(t, claims, nil) + }, + wantErr: true, + errContains: "untrusted issuer", + }, + { + name: "external token bad signature", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + // Sign with a different key than the one the JWKS server serves. + wrongJWKS := newTestJWKS(t) + return wrongJWKS.signToken(t, externalClaims(), nil) + }, + wantErr: true, + errContains: "signature verification failed", + }, + { + name: "self-issued token signed by external key fails", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + // Token claims say iss=self, but signed by the external key. + // Routes to self validator, which rejects the signature. + return externalJWKS.signToken(t, validClaims(), nil) + }, + wantErr: true, + errContains: "signature verification failed", + }, + { + name: "external token missing subject", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + claims := externalClaims() + claims.Subject = "" + return externalJWKS.signToken(t, claims, nil) + }, + wantErr: true, + errContains: "missing required 'sub' claim", + }, + { + name: "external token missing exp claim", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + claims := externalClaims() + claims.Expiry = nil + return externalJWKS.signToken(t, claims, map[string]any{"azp": "ext-agent"}) + }, + wantErr: true, + errContains: "missing required 'exp' claim", + }, + { + // Distinct from "malformed token" below: that hits the JWT parse + // failure inside peekIssuer, this hits its separate empty-issuer + // check on an otherwise well-formed token. Both wrap to the same + // outer "failed to determine token issuer" message, so assert on + // the distinct inner text instead. + name: "token missing iss claim", + token: func(t *testing.T) string { + t.Helper() + claims := externalClaims() + claims.Issuer = "" + return externalJWKS.signToken(t, claims, nil) + }, + wantErr: true, + errContains: "missing 'iss' claim", + }, + { + // Proves the allowlist is skipped (not just "would have failed + // anyway") whenever may_act is present: both azp and + // AllowedActors would satisfy resolveAllowedActor if it ran, so + // a regression that calls it unconditionally and only gates the + // error on MayAct==nil would still populate ExternalActor here. + name: "external token may_act present alongside an allowlisted azp still yields empty ExternalActor", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + return externalJWKS.signToken(t, externalClaims(), map[string]any{ + "azp": "ext-agent", + "may_act": map[string]any{"sub": "some-toolhive-client", "iss": testIssuer}, + }) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + require.NotNil(t, vc.MayAct) + assert.Empty(t, vc.ExternalActor, "allowlist must be skipped entirely when may_act is present") + }, + }, + { + name: "external token expired", + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + token: func(t *testing.T) string { + t.Helper() + claims := externalClaims() + claims.Expiry = jwt.NewNumericDate(time.Now().Add(-time.Hour)) + claims.IssuedAt = jwt.NewNumericDate(time.Now().Add(-2 * time.Hour)) + return externalJWKS.signToken(t, claims, nil) + }, + wantErr: true, + errContains: "claims validation failed", + }, + { + name: "malformed token", + trustedIssuers: nil, + token: func(_ *testing.T) string { + return "not-a-jwt" + }, + wantErr: true, + errContains: "failed to determine token issuer", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + validator := newMultiValidator(t, selfJWKS, tt.trustedIssuers) + rawToken := tt.token(t) + + result, err := validator.Validate(context.Background(), rawToken) + + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errContains) + assert.Nil(t, result) + return + } + + require.NoError(t, err) + require.NotNil(t, result) + if tt.check != nil { + tt.check(t, result) + } + }) + } +} + +// TestMultiIssuerTokenValidator_TrailingSlashIssuerExactMatch covers a +// trailing-slash issuer_url (e.g. Microsoft Entra ID v1's +// "https://sts.windows.net/{tenant}/") being accepted as an exact "iss" +// match. JWKSURL is set explicitly rather than left for discovery to resolve: +// validateTrustedIssuer rejects {AllowPrivateIPs: true, JWKSURL: ""} (the +// combination newMultiValidator forces on every test issuer to reach its +// loopback server), so an empty JWKSURL would fail construction here. +// discoverJWKSURL's own trailing-slash trim is covered directly by +// TestMultiIssuerTokenValidator_DiscoverJWKSURL instead; this test is only +// about the trailing-slash IssuerURL matching the token's "iss" claim exactly +// (per OIDC Core §3.1.3.3), which happens regardless of how JWKSURL was +// resolved. +func TestMultiIssuerTokenValidator_TrailingSlashIssuerExactMatch(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + jwksServer := startJWKSServer(t, externalJWKS) + + trailingSlashIssuer := "https://tenant.example.com/realm/" + trustedIssuers := []TrustedIssuer{{ + IssuerURL: trailingSlashIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + claims := jwt.Claims{ + Subject: "trailing-slash-user", + Issuer: trailingSlashIssuer, + Audience: jwt.Audience{testExternalAudience}, + Expiry: jwt.NewNumericDate(time.Now().Add(time.Hour)), + IssuedAt: jwt.NewNumericDate(time.Now()), + NotBefore: jwt.NewNumericDate(time.Now().Add(-time.Minute)), + ID: "jti-trailing-slash-001", + } + rawToken := externalJWKS.signToken(t, claims, map[string]any{"azp": "ext-agent"}) + + result, err := validator.Validate(context.Background(), rawToken) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, "trailing-slash-user", result.Subject) + assert.Equal(t, trailingSlashIssuer, result.Issuer) +} + +func TestMultiIssuerTokenValidator_JWKSCaching(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + + // Track how many times the JWKS endpoint is hit. + var fetchCount atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + fetchCount.Add(1) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(externalJWKS.publicJWKS()) + }) + jwksServer := httptest.NewServer(mux) + t.Cleanup(jwksServer.Close) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + // Validate two tokens — the JWKS should be fetched only once (cached). + for i := range 2 { + claims := externalClaims() + claims.ID = fmt.Sprintf("jti-cache-%d", i) + rawToken := externalJWKS.signToken(t, claims, map[string]any{"azp": "ext-agent"}) + + result, err := validator.Validate(context.Background(), rawToken) + require.NoError(t, err) + require.NotNil(t, result) + assert.Equal(t, "ext-user-456", result.Subject) + } + + assert.Equal(t, int32(1), fetchCount.Load(), "JWKS should be fetched only once due to caching") +} + +// TestMultiIssuerTokenValidator_JWKSRefreshIntervalIsPinned confirms +// registerOrRefresh actually threads jwk.WithConstantInterval(jwksRefreshInterval) +// through to the underlying httprc.Resource, even though the JWKS endpoint +// advertises a much longer Cache-Control max-age. Without the constant +// interval, httprc would derive the refresh schedule from that header +// instead — see jwksRefreshInterval's doc comment for why an external +// issuer must not get to choose how long we keep its keys cached. +// +// This can't be observed by waiting for a second fetch without a wall-clock +// sleep (forbidden by this repo's testing rules), so it asserts directly on +// the registered resource's ConstantInterval() instead of on fetch timing. +func TestMultiIssuerTokenValidator_JWKSRefreshIntervalIsPinned(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + + mux := http.NewServeMux() + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + // A long max-age that would push httprc's derived interval far past + // jwksRefreshInterval if the constant interval were not applied. + w.Header().Set("Cache-Control", "max-age=2592000") // 30 days + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(externalJWKS.publicJWKS()) + }) + jwksServer := httptest.NewServer(mux) + t.Cleanup(jwksServer.Close) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + rawToken := externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": "ext-agent"}) + _, err := validator.Validate(context.Background(), rawToken) + require.NoError(t, err) + + issuerConfig := validator.issuers[testExternalIssuer] + resource, err := issuerConfig.jwksCache.LookupResource(context.Background(), issuerConfig.jwksURL) + require.NoError(t, err) + assert.Equal(t, jwksRefreshInterval, resource.ConstantInterval(), + "registered resource must ignore the endpoint's own Cache-Control max-age") +} + +func TestNewMultiIssuerTokenValidator_Validation(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + selfValidator, err := NewSelfIssuedTokenValidator(selfJWKS.publicJWKS(), testIssuer, []string{testIssuer}) + require.NoError(t, err) + + tests := []struct { + name string + selfValidator *SelfIssuedTokenValidator + selfIssuer string + trustedIssuers []TrustedIssuer + errContains string + }{ + { + name: "nil selfValidator rejected", + selfIssuer: testIssuer, + errContains: "selfValidator must not be nil", + }, + { + name: "empty selfIssuer rejected", + selfValidator: selfValidator, + selfIssuer: "", + errContains: "selfIssuer must not be empty", + }, + { + name: "empty IssuerURL rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: "", + ExpectedAudience: testExternalAudience, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + errContains: "issuer_url is required", + }, + { + name: "empty ExpectedAudience rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: "", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + errContains: "expected_audience is required", + }, + { + name: "IssuerURL equal to selfIssuer rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testIssuer, + ExpectedAudience: testExternalAudience, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + errContains: "must not equal the authorization server's own issuer", + }, + { + name: "duplicate IssuerURL rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{ + {IssuerURL: testExternalIssuer, ExpectedAudience: testExternalAudience, AllowedDelegateClients: []string{anyDelegateClient}}, + {IssuerURL: testExternalIssuer, ExpectedAudience: testExternalAudience, AllowedDelegateClients: []string{anyDelegateClient}}, + }, + errContains: "configured more than once", + }, + { + name: `ActorClaim "sub" rejected — assignClaim never leaves it in Extra`, + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + ActorClaim: "sub", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + errContains: "actor_claim", + }, + { + name: `ActorClaim "scope" rejected — rerouted to a structured field`, + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + ActorClaim: "scope", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + errContains: "actor_claim", + }, + { + name: `ActorClaim "may_act" rejected — rerouted to a structured field`, + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + ActorClaim: "may_act", + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + errContains: "actor_claim", + }, + { + name: "AllowedDelegateClients with an empty entry rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + AllowedDelegateClients: []string{"toolhive-agent-a", ""}, + }}, + errContains: "allowed_delegate_clients must not contain an empty client ID", + }, + { + // #5989 hardening: permissiveness must be declared with the + // wildcard, not obtained by omission — pins the fail-closed + // default this PR introduces. + name: "AllowedDelegateClients absent rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + // AllowedDelegateClients intentionally unset. + }}, + errContains: "allowed_delegate_clients is required", + }, + { + name: "AllowedDelegateClients empty (non-nil) slice rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + AllowedDelegateClients: []string{}, + }}, + errContains: "allowed_delegate_clients is required", + }, + { + // The wildcard authorizes every ToolHive client explicitly; mixing + // it with specific IDs would either be redundant or, worse, give + // the false impression that only the listed IDs are bound. + // Rejecting outright avoids silently ignoring the specific IDs. + name: "AllowedDelegateClients wildcard mixed with a specific client ID rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + AllowedDelegateClients: []string{anyDelegateClient, "toolhive-agent-a"}, + }}, + errContains: "must not combine the wildcard", + }, + { + // AllowPrivateIPs without a hand-configured jwks_url would let + // OIDC discovery choose the private target the dial is allowed to + // reach; validateTrustedIssuer must reject it so a caller that + // skips Config.Validate (factory, tests) cannot bypass the + // config-time check in pkg/authserver/config.go. + name: "AllowPrivateIPs without JWKSURL rejected", + selfValidator: selfValidator, + selfIssuer: testIssuer, + trustedIssuers: []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + AllowPrivateIPs: true, + AllowedDelegateClients: []string{anyDelegateClient}, + }}, + errContains: "allow_private_ips requires jwks_url", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + v, err := NewMultiIssuerTokenValidator(tt.selfValidator, tt.selfIssuer, tt.trustedIssuers) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errContains) + assert.Nil(t, v) + }) + } +} + +func TestNewMultiIssuerTokenValidator_EmptyAllowedActorsAccepted(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + selfValidator, err := NewSelfIssuedTokenValidator(selfJWKS.publicJWKS(), testIssuer, []string{testIssuer}) + require.NoError(t, err) + + // Empty AllowedActors must be accepted by the constructor: a may_act-only + // issuer (no allowlisted actors at all) is a legitimate configuration. + v, err := NewMultiIssuerTokenValidator(selfValidator, testIssuer, []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + AllowedDelegateClients: []string{anyDelegateClient}, + }}) + require.NoError(t, err) + assert.NotNil(t, v) +} + +// syncBuffer is a concurrency-safe io.Writer over a bytes.Buffer, used by +// TestNewMultiIssuerTokenValidator_AudienceShapeWarning to capture +// slog.Default() output. slog.SetDefault is process-global, so a plain +// bytes.Buffer would race against any other goroutine that logs while a +// capturing handler is installed. Mirrors the identically-named helper in +// pkg/authserver/runner/embeddedauthserver_test.go; kept as a separate copy +// since it is an unexported test type not worth exporting across packages. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (b *syncBuffer) Write(p []byte) (int, error) { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.Write(p) +} + +func (b *syncBuffer) String() string { + b.mu.Lock() + defer b.mu.Unlock() + return b.buf.String() +} + +// TestNewMultiIssuerTokenValidator_AudienceShapeWarning pins +// looksLikeResourceIdentifier's startup warning: a TrustedIssuer whose +// ExpectedAudience has no URI scheme (e.g. a bare client-ID-shaped value) +// logs a slog.Warn naming the issuer, since that audience shape cannot +// distinguish an ID token from an access token for that issuer (the token +// itself may carry neither "at_hash" nor "c_hash", which OIDC permits — see +// rejectIDTokenClaims's doc comment). A URI-shaped audience (has a "://" +// scheme) must NOT trigger the warning. +// +// NOT t.Parallel(): swaps the package-global slog.Default() and restores it +// via t.Cleanup; see syncBuffer's doc comment for why a concurrent writer +// alone would be safe but an overlapping swap/restore pair would not be. +// +//nolint:paralleltest // mutates the package-global slog.Default() +func TestNewMultiIssuerTokenValidator_AudienceShapeWarning(t *testing.T) { + tests := []struct { + name string + expectedAudience string + wantWarning bool + }{ + { + name: "bare client-ID-shaped audience warns", + expectedAudience: "toolhive-authserver", + wantWarning: true, + }, + { + name: "bare GUID audience warns", + expectedAudience: "3f2504e0-4f89-11d3-9a0c-0305e82c3301", + wantWarning: true, + }, + { + name: "https URI audience does not warn", + expectedAudience: "https://api.example.com", + wantWarning: false, + }, + { + name: "api URI audience does not warn", + expectedAudience: "api://3f2504e0-4f89-11d3-9a0c-0305e82c3301", + wantWarning: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { selfJWKS := newTestJWKS(t) - externalJWKS := newTestJWKS(t) + selfValidator, err := NewSelfIssuedTokenValidator(selfJWKS.publicJWKS(), testIssuer, []string{testIssuer}) + require.NoError(t, err) - mux := http.NewServeMux() - mux.HandleFunc("/.well-known/openid-configuration", tt.handler) - server := httptest.NewServer(mux) - t.Cleanup(server.Close) + var buf syncBuffer + prev := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelDebug}))) + t.Cleanup(func() { slog.SetDefault(prev) }) + + v, err := NewMultiIssuerTokenValidator(selfValidator, testIssuer, []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: tt.expectedAudience, + AllowedActors: []string{"ext-agent"}, // avoid the unrelated AllowedActors warning + AllowedDelegateClients: []string{anyDelegateClient}, + }}) + require.NoError(t, err) + require.NotNil(t, v) + + logged := buf.String() + const wantSubstring = "does not look like a resource identifier" + if tt.wantWarning { + assert.Contains(t, logged, wantSubstring) + assert.Contains(t, logged, testExternalIssuer) + } else { + assert.NotContains(t, logged, wantSubstring) + } + }) + } +} + +func TestNewMultiIssuerTokenValidator_ClonesAllowedActors(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + jwksServer := startJWKSServer(t, externalJWKS) + + allowedActors := []string{"ext-agent"} + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: allowedActors, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + // Mutate the caller's slice after construction. If the constructor didn't + // clone it, this would change what the validator accepts. + allowedActors[0] = "someone-else" + + rawToken := externalJWKS.signToken(t, externalClaims(), map[string]any{"azp": "ext-agent"}) + result, err := validator.Validate(context.Background(), rawToken) + require.NoError(t, err, "post-construction mutation of the caller's slice must not affect validation") + require.NotNil(t, result) + assert.Equal(t, "ext-agent", result.ExternalActor) +} + +// TestMultiIssuerTokenValidator_ClockSkewLeeway exercises externalClockSkewLeeway +// (60s): nbf/iat tolerate it, but exp is enforced strictly by an independent +// check in validateExternalToken, since go-jose's leeway would otherwise widen +// exp too and let a genuinely expired subject token through. +func TestMultiIssuerTokenValidator_ClockSkewLeeway(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + jwksServer := startJWKSServer(t, externalJWKS) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + tests := []struct { + name string + claims func() jwt.Claims + wantErr bool + errContains string + }{ + { + name: "nbf ~30s in the future is within leeway", + claims: func() jwt.Claims { + c := externalClaims() + c.NotBefore = jwt.NewNumericDate(time.Now().Add(30 * time.Second)) + return c + }, + }, + { + name: "nbf ~5m in the future exceeds leeway", + claims: func() jwt.Claims { + c := externalClaims() + c.NotBefore = jwt.NewNumericDate(time.Now().Add(5 * time.Minute)) + return c + }, + wantErr: true, + errContains: "claims validation failed", + }, + { + // go-jose's leeway also widens exp, so a naive implementation would + // accept this. validateExternalToken's independent strict check + // must reject it instead. + name: "expired ~30s ago is rejected by the strict expiry check, not the leeway", + claims: func() jwt.Claims { + c := externalClaims() + c.Expiry = jwt.NewNumericDate(time.Now().Add(-30 * time.Second)) + return c + }, + wantErr: true, + errContains: "subject token has expired", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rawToken := externalJWKS.signToken(t, tt.claims(), map[string]any{"azp": "ext-agent"}) + result, err := validator.Validate(context.Background(), rawToken) + + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errContains) + assert.Nil(t, result) + return + } + require.NoError(t, err) + require.NotNil(t, result) + }) + } +} + +// TestMultiIssuerTokenValidator_DiscoverJWKSURL exercises discoverJWKSURL +// directly via the externalIssuerConfig seam, bypassing constructor +// validation so an empty JWKSURL can be tested at all. +// NewMultiIssuerTokenValidator can no longer exercise OIDC discovery through +// its own constructor: validateTrustedIssuer rejects {AllowPrivateIPs: true, +// JWKSURL: ""}, and every test issuer built through it runs on a loopback +// httptest server, which requires AllowPrivateIPs: true to be reachable at +// all. discoverJWKSURL itself only reads issuerConfig.IssuerURL and +// .httpClient, so it needs neither a real validator nor a registered issuer. +func TestMultiIssuerTokenValidator_DiscoverJWKSURL(t *testing.T) { + t.Parallel() + + newDiscoveryServer := func(t *testing.T, handler http.HandlerFunc) *httptest.Server { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", handler) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + return srv + } + + tests := []struct { + name string + // build returns the issuerConfig to call discoverJWKSURL with, and + // (only for the no-error cases) the jwks_uri it must return. + build func(t *testing.T) (issuerConfig *externalIssuerConfig, wantJWKSURI string) + errContains string + }{ + { + name: "happy path returns the advertised jwks_uri", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := newDiscoveryServer(t, func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": base, + "jwks_uri": base + "/jwks", + }) + }) + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: srv.Client(), + }, srv.URL + "/jwks" + }, + }, + { + // Proves two things at once: the well-known path is built by + // trimming the trailing slash (a client that refuses to follow + // redirects would see a 301 from http.ServeMux's path-cleaning if + // the trim were dropped, since the request path would then + // contain "//"), and the issuer-match comparison below that use + // the *untrimmed* IssuerURL (the discovery document's own + // "issuer" here still carries the trailing slash). + name: "trailing-slash issuer_url is trimmed before the well-known path is appended", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := newDiscoveryServer(t, func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + "/" + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": base, + "jwks_uri": base + "jwks", + }) + }) + client := srv.Client() + client.CheckRedirect = func(*http.Request, []*http.Request) error { + return fmt.Errorf("unexpected redirect: issuer_url trailing slash was not trimmed") + } + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL + "/", AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: client, + }, srv.URL + "/jwks" + }, + }, + { + name: "invalid issuer_url fails to build the discovery request", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + return &externalIssuerConfig{ + // A raw control character is rejected by url.Parse inside + // http.NewRequestWithContext, before any network I/O. + TrustedIssuer: TrustedIssuer{IssuerURL: "http://example.com/\x00", AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: &http.Client{}, + }, "" + }, + errContains: "failed to create discovery request", + }, + { + name: "unreachable issuer fails the discovery request", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := httptest.NewServer(http.NotFoundHandler()) + srv.Close() // closed before use: the port is now guaranteed unreachable. + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: srv.Client(), + }, "" + }, + errContains: "discovery request failed", + }, + { + name: "non-200 response", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := newDiscoveryServer(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: srv.Client(), + }, "" + }, + errContains: "discovery endpoint returned status 500", + }, + { + name: "malformed JSON body fails to parse", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := newDiscoveryServer(t, func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("{not-json")) + }) + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: srv.Client(), + }, "" + }, + // The distinguishing substring from json.Unmarshal itself, not + // just the wrapping "failed to parse discovery document": the + // oversized-body case below wraps the same outer message but + // fails for a different underlying reason. + errContains: "invalid character", + }, + { + // A body over maxResponseBodySize (1 MiB) is not its own error + // branch: io.LimitReader silently truncates it, and the + // truncated bytes then fail to parse as JSON — so this exercises + // the same "failed to parse discovery document" wrapper as the + // malformed-JSON case above, but for a distinct underlying + // reason (an unterminated value, not a syntax error), which is + // what's asserted on below. + name: "oversized discovery document is truncated and fails JSON parsing", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := newDiscoveryServer(t, func(w http.ResponseWriter, r *http.Request) { + issuer := "http://" + r.Host + w.Header().Set("Content-Type", "application/json") + // Deliberately WELL-FORMED and complete: issuer matches and + // jwks_uri is present, so if the whole body were read this + // document would parse and discovery would succeed. Only the + // maxResponseBodySize cap cutting inside the padding makes it + // fail. That is what pins the cap — an unterminated body + // would fail the same way with the cap deleted. + _, _ = w.Write([]byte(`{"issuer":"` + issuer + `","jwks_uri":"` + issuer + `/jwks","padding":"`)) + _, _ = w.Write([]byte(strings.Repeat("a", 2*maxResponseBodySize))) + _, _ = w.Write([]byte(`"}`)) + }) + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: srv.Client(), + }, "" + }, + errContains: "unexpected end of JSON input", + }, + { + name: "issuer mismatch is rejected", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := newDiscoveryServer(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": "https://different-issuer.example.com", + "jwks_uri": "http://" + r.Host + "/jwks", + }) + }) + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: srv.Client(), + }, "" + }, + errContains: "does not match expected issuer", + }, + { + name: "missing jwks_uri is rejected", + build: func(t *testing.T) (*externalIssuerConfig, string) { + t.Helper() + srv := newDiscoveryServer(t, func(w http.ResponseWriter, r *http.Request) { + // Issuer must match so discovery reaches the jwks_uri check. + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": "http://" + r.Host, + }) + }) + return &externalIssuerConfig{ + TrustedIssuer: TrustedIssuer{IssuerURL: srv.URL, AllowedDelegateClients: []string{anyDelegateClient}}, + httpClient: srv.Client(), + }, "" + }, + errContains: "missing 'jwks_uri'", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + issuerConfig, wantJWKSURI := tt.build(t) + gotJWKSURI, err := (&MultiIssuerTokenValidator{}).discoverJWKSURL(context.Background(), issuerConfig) + + if tt.errContains != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errContains) + assert.Empty(t, gotJWKSURI) + return + } + require.NoError(t, err) + assert.Equal(t, wantJWKSURI, gotJWKSURI) + }) + } +} + +// TestMultiIssuerTokenValidator_DiscoveryRefusesPrivateAddress proves the +// dial-time private-IP guard on the strict (AllowPrivateIPs: false) path: an +// issuer with an empty JWKSURL forces OIDC discovery, and every test server +// in this file runs on loopback — so with AllowPrivateIPs left false, +// discovery must be refused before any response is even read, regardless of +// what the discovery endpoint would have returned. The content-level failure +// modes of discoverJWKSURL itself (non-200, malformed doc, issuer mismatch, +// missing jwks_uri) are exercised directly by +// TestMultiIssuerTokenValidator_DiscoverJWKSURL instead, since none of them +// can be reached through this constructor path anymore. +func TestMultiIssuerTokenValidator_DiscoveryRefusesPrivateAddress(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + + // The handler would, if reached, serve a valid discovery document — this + // confirms the failure happens at the dial, not because the endpoint + // itself is broken. + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + base := "http://" + r.Host + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]string{ + "issuer": base, + "jwks_uri": base + "/jwks", + }) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) - trustedIssuers := []TrustedIssuer{{ - IssuerURL: server.URL, - ExpectedAudience: testExternalAudience, - // JWKSURL left empty to force discovery. - }} - validator := newMultiValidator(t, selfJWKS, trustedIssuers) + // JWKSURL is left empty to force discovery. AllowPrivateIPs is + // deliberately false: newMultiValidator would force it true to reach the + // loopback server, defeating the point of this test, so the constructor + // is called directly here instead. + trustedIssuers := []TrustedIssuer{{ + IssuerURL: server.URL, + ExpectedAudience: testExternalAudience, + InsecureAllowHTTP: true, + AllowPrivateIPs: false, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + selfValidator, err := NewSelfIssuedTokenValidator(selfJWKS.publicJWKS(), testIssuer, []string{testIssuer}) + require.NoError(t, err) + validator, err := NewMultiIssuerTokenValidator(selfValidator, testIssuer, trustedIssuers) + require.NoError(t, err) - claims := externalClaims() - claims.Issuer = server.URL - rawToken := externalJWKS.signToken(t, claims, nil) + claims := externalClaims() + claims.Issuer = server.URL + rawToken := externalJWKS.signToken(t, claims, nil) - result, err := validator.Validate(context.Background(), rawToken) - require.Error(t, err) - assert.Contains(t, err.Error(), "OIDC discovery failed") - assert.Nil(t, result) - }) - } + result, err := validator.Validate(context.Background(), rawToken) + require.Error(t, err) + assert.Contains(t, err.Error(), "OIDC discovery failed") + assert.Contains(t, err.Error(), networking.ErrPrivateIpAddress, + "the failure must be the dial-time private-IP guard specifically, not just any discovery error") + assert.Nil(t, result) } func TestMultiIssuerTokenValidator_KidMismatch(t *testing.T) { @@ -456,9 +1677,10 @@ func TestMultiIssuerTokenValidator_KidMismatch(t *testing.T) { jwksServer := startJWKSServer(t, externalJWKS) trustedIssuers := []TrustedIssuer{{ - IssuerURL: testExternalIssuer, - ExpectedAudience: testExternalAudience, - JWKSURL: jwksServer.URL + "/jwks", + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: jwksServer.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, }} validator := newMultiValidator(t, selfJWKS, trustedIssuers) @@ -474,3 +1696,649 @@ func TestMultiIssuerTokenValidator_KidMismatch(t *testing.T) { assert.Contains(t, err.Error(), "signature verification failed") assert.Nil(t, result) } + +// TestValidateJWKSURL exercises ValidateJWKSURL directly: this is the SSRF +// guard applied in ensureRegistered to every JWKS URL for a given issuer, whether +// hand-configured on TrustedIssuer or resolved via discovery, and shared +// verbatim with pkg/authserver/config.go's config-time check +// (validateJWKSEndpointURL) so the two can't drift out of sync. The +// equivalent check on redirect hops (networking.SameHostRedirectPolicy) and +// the dial-time IP guard (networking.NewHostScopedClientBuilder) are +// exercised via the networking package's own tests, not here. These cases +// all pass insecureAllowHTTP=false, allowPrivateIPs=false — the strict +// defaults — since every other test in this file goes through +// newMultiValidator, which sets both permissive flags on its test issuers to +// reach their httptest servers over plain HTTP on loopback; the +// insecureAllowHTTP=true cases below are the exception, covering the laxity +// that must not extend beyond "http" to every other non-https scheme. +func TestValidateJWKSURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + url string + insecureAllowHTTP bool + wantErr string + }{ + {name: "https accepted", url: "https://issuer.example.com/jwks"}, + {name: "http rejected", url: "http://issuer.example.com/jwks", wantErr: "must use HTTPS"}, + { + name: "userinfo with password rejected", + url: "https://user:hunter2@issuer.example.com/jwks", + wantErr: "must not contain userinfo", + }, + { + // url.Parse sets User for a bare username too, and net/http would + // still send it as a Basic auth header. + name: "userinfo without password rejected", + url: "https://user@issuer.example.com/jwks", + wantErr: "must not contain userinfo", + }, + { + name: "userinfo rejected even with insecureAllowHTTP", + url: "http://user:hunter2@issuer.example.com/jwks", + insecureAllowHTTP: true, + wantErr: "must not contain userinfo", + }, + { + name: "http accepted with insecureAllowHTTP", + url: "http://issuer.example.com/jwks", + insecureAllowHTTP: true, + }, + { + name: "ftp rejected even with insecureAllowHTTP", + url: "ftp://issuer.example.com/jwks", + insecureAllowHTTP: true, + wantErr: "must use HTTPS", + }, + { + name: "no scheme rejected even with insecureAllowHTTP", + url: "//issuer.example.com/jwks", + insecureAllowHTTP: true, + wantErr: "must use HTTPS", + }, + {name: "loopback IP literal rejected", url: "https://127.0.0.1/jwks", wantErr: "private or loopback"}, + {name: "private IP literal rejected", url: "https://10.1.2.3/jwks", wantErr: "private or loopback"}, + {name: "malformed URL rejected", url: "://not-a-url", wantErr: "invalid URL"}, + {name: "missing host rejected", url: "https:///jwks", wantErr: "host is required"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + err := ValidateJWKSURL(tt.url, tt.insecureAllowHTTP, false) + if tt.wantErr == "" { + assert.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +// TestMultiIssuerTokenValidator_FetchJWKS exercises ensureRegistered's and +// lookupJWKS's error paths through the full Validate path, bypassing OIDC +// discovery via a preconfigured JWKSURL. Registration and the JWKS's own +// zero-keys/too-many-keys checks now go through the issuer's own jwk.Cache and +// lookupJWKS respectively rather than a private HTTP fetch. +// +// For "non-200 response" and "malformed JSON body", verified empirically: +// httprc.Resource's `ready` channel only ever closes on a *successful* +// fetch, so Register's WithWaitReady(true) wait (ensureRegistered's +// first-ever-registration path) can't distinguish "still fetching" from +// "fetch failed" — it just blocks until fetchCtx's own deadline and returns +// a generic timeout, discarding the real HTTP/parse error, which is why +// these two cases assert on "context deadline exceeded" rather than on +// jwx's own status-code or parse-error text (that text only ever surfaces +// on a *later* validation of the same never-yet-succeeded issuer, via +// ensureRegistered's Refresh-based retry branch — not exercised by a single +// Validate call here). "zero keys" and "too many keys" are unaffected: a +// JWKS that parses but fails this file's own key-count checks still +// registers successfully, so lookupJWKS's checks run immediately. +func TestMultiIssuerTokenValidator_FetchJWKS(t *testing.T) { + t.Parallel() + + // Built once: maxJWKSKeys+1 distinct public keys for the "too many keys" case. + tooManyKeys := make([]jose.JSONWebKey, maxJWKSKeys+1) + for i := range tooManyKeys { + key := newECDSAJWK(t, fmt.Sprintf("k%d", i)) + tooManyKeys[i] = key.Public() + } + + // A complete, otherwise-valid JWKS document padded past + // maxResponseBodySize with an unrelated field — see its use below for why + // that matters. + oversizedJWKSDoc := func() []byte { + paddingKey := newECDSAJWK(t, "padding-kid") + raw, err := json.Marshal(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{paddingKey.Public()}}) + require.NoError(t, err) + var doc map[string]any + require.NoError(t, json.Unmarshal(raw, &doc)) + doc["padding"] = strings.Repeat("a", 4*1024*1024) + padded, err := json.Marshal(doc) + require.NoError(t, err) + return padded + }() + + tests := []struct { + name string + handler http.HandlerFunc + wantErr string + }{ + { + name: "non-200 response", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }, + wantErr: "context deadline exceeded", + }, + { + name: "malformed JSON body", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("{not-json")) + }, + wantErr: "context deadline exceeded", + }, + { + name: "zero keys", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: []jose.JSONWebKey{}}) + }, + wantErr: "contains no keys", + }, + { + name: "too many keys", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(jose.JSONWebKeySet{Keys: tooManyKeys}) + }, + wantErr: "too many keys", + }, + { + // Pins limitedBodyTransport's cap on the JWKS fetch path (jwx's + // own httprc.MaxBufferSize ceiling is ~1000 MiB, far too high to + // bound anything here on its own). Deliberately WELL-FORMED and + // complete, unlike the other failure cases above: the padding + // field is oversized but the document would parse successfully + // if read in full, so only limitedBodyTransport cutting the read + // short makes this fail — the same technique + // TestMultiIssuerTokenValidator_DiscoverJWKSURL's oversized-body + // case uses for the discovery path. The 4 MiB padding size is a + // fixed literal independent of maxResponseBodySize (1 MiB): sizing + // it as a multiple of that constant would make a broken cap and a + // shrunken constant fail identically, hiding a regression in the + // cap itself. + name: "oversized JWKS response is rejected rather than parsed", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(oversizedJWKSDoc) + }, + wantErr: "context deadline exceeded", + }, + } + + selfJWKS := newTestJWKS(t) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + mux := http.NewServeMux() + mux.HandleFunc("/jwks", tt.handler) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: srv.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }} + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + rawToken := signExternalToken(t, newECDSAJWK(t, "any-kid"), externalClaims()) + _, err := validator.Validate(context.Background(), rawToken) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } +} + +// signExternalToken signs claims with the given key for the external-issuer +// path, adding a may_act claim so the token satisfies delegation consent +// without needing an AllowedActors allowlist match — signWithJWK doesn't +// support extra claims like azp. +func signExternalToken(t *testing.T, key jose.JSONWebKey, claims jwt.Claims) string { + t.Helper() + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: jose.ES256, Key: key}, + (&jose.SignerOptions{}).WithType("JWT"), + ) + require.NoError(t, err) + raw, err := jwt.Signed(signer). + Claims(claims). + Claims(map[string]any{"may_act": map[string]any{"sub": "some-toolhive-client", "iss": testIssuer}}). + Serialize() + require.NoError(t, err) + return raw +} + +// TestMultiIssuerTokenValidator_KeyRotationRefreshesImmediately is the test +// that demonstrates the point of the cache rewrite: a subject token signed +// with a newly rotated key, carrying a kid the validator has never seen, must +// validate successfully on the very first attempt — not after jwx's own +// 15-minute-minimum background refresh, and not only on a second, separate +// request. +func TestMultiIssuerTokenValidator_KeyRotationRefreshesImmediately(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + keyV1 := newECDSAJWK(t, "v1") + keyV2 := newECDSAJWK(t, "v2") + + var mu sync.Mutex + currentKey := keyV1 + mux := http.NewServeMux() + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + mu.Lock() + key := currentKey + mu.Unlock() + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(publicJWKSOf(key)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: srv.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }} + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + // Prime the cache: validate successfully against v1. + _, err := validator.Validate(context.Background(), signExternalToken(t, keyV1, externalClaims())) + require.NoError(t, err) + + // Rotate: the server now serves only v2's public key, under a new kid. + mu.Lock() + currentKey = keyV2 + mu.Unlock() + + claims := externalClaims() + claims.ID = "jti-post-rotation" + tokenV2 := signExternalToken(t, keyV2, claims) + + done := make(chan struct { + result *ValidatedClaims + err error + }, 1) + go func() { + result, err := validator.Validate(context.Background(), tokenV2) + done <- struct { + result *ValidatedClaims + err error + }{result, err} + }() + + select { + case out := <-done: + require.NoError(t, out.err) + require.NotNil(t, out.result) + assert.Equal(t, "ext-user-456", out.result.Subject) + case <-time.After(5 * time.Second): + t.Fatal("timeout waiting for validation after key rotation") + } +} + +// TestMultiIssuerTokenValidator_UnknownKidRefreshIsRateLimited proves +// refreshOnUnknownKid's minKidRefreshInterval gate: repeated subject tokens +// naming a kid absent from the cached JWKS must not force a fetch per +// request — only the first ever unknown-kid attempt (within the interval) +// may do so. +func TestMultiIssuerTokenValidator_UnknownKidRefreshIsRateLimited(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + keyV1 := newECDSAJWK(t, "v1") + + var fetchCount atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + fetchCount.Add(1) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(publicJWKSOf(keyV1)) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: srv.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }} + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + // Prime the cache with a first, successful validation. + _, err := validator.Validate(context.Background(), signExternalToken(t, keyV1, externalClaims())) + require.NoError(t, err) + primedFetches := fetchCount.Load() + + // Repeatedly present a token naming a kid absent from the served JWKS. + unknownKey := newECDSAJWK(t, "unknown-kid") + for i := range 5 { + claims := externalClaims() + claims.ID = fmt.Sprintf("jti-unknown-%d", i) + rawToken := signExternalToken(t, unknownKey, claims) + _, err := validator.Validate(context.Background(), rawToken) + require.Error(t, err) + } + + // Only the first unknown-kid attempt should have forced a refresh; the + // gate must hold for the remaining four within minKidRefreshInterval. + assert.Equal(t, primedFetches+1, fetchCount.Load(), + "repeated unknown-kid tokens within the window must not each force a fetch") +} + +// TestMultiIssuerTokenValidator_NeverFetchedRetryIsRateLimited proves +// ensureRegistered's jwksFetchFailureBackoff gate: an issuer whose endpoint +// has never once succeeded must not be re-fetched on every request — key +// resolution runs before signature verification, so without this gate any +// client holding a subject token naming this issuer could drive one real +// outbound fetch per request. +func TestMultiIssuerTokenValidator_NeverFetchedRetryIsRateLimited(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + + var fetchCount atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + fetchCount.Add(1) + w.WriteHeader(http.StatusInternalServerError) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: srv.URL + "/jwks", + AllowedDelegateClients: []string{anyDelegateClient}, + }} + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + rawToken := signExternalToken(t, newECDSAJWK(t, "any-kid"), externalClaims()) + + // First attempt: no cached value yet, so this one genuinely fetches (and + // blocks on Register's own wait — see ensureRegistered's doc comment). + _, err := validator.Validate(context.Background(), rawToken) + require.Error(t, err) + require.Equal(t, int32(1), fetchCount.Load()) + + // Further attempts within jwksFetchFailureBackoff must replay the stored + // error instead of fetching again. + for range 3 { + _, err := validator.Validate(context.Background(), rawToken) + require.Error(t, err) + } + assert.Equal(t, int32(1), fetchCount.Load(), + "repeated requests against a never-successfully-fetched issuer must not each force a fetch") +} + +// TestMultiIssuerTokenValidator_SharedJWKSURL_SamePolicy proves that two +// issuers resolving to the identical jwksURL under the identical HTTP +// transport policy both validate successfully — each through its own +// jwk.Cache and *http.Client (see externalIssuerConfig.jwksCache). This is +// the common real-world case: Microsoft Entra v1 tenants share one +// tenant-independent JWKS endpoint, so two Entra tenants configured as +// separate trusted issuers collide on the same jwks_url by construction. +func TestMultiIssuerTokenValidator_SharedJWKSURL_SamePolicy(t *testing.T) { + t.Parallel() + + const ( + issuerAURL = "https://issuer-a.example.com" + issuerBURL = "https://issuer-b.example.com" + audienceA = "aud-a" + audienceB = "aud-b" + ) + + selfJWKS := newTestJWKS(t) + sharedJWKS := newTestJWKS(t) + jwksServer := startJWKSServer(t, sharedJWKS) + + trustedIssuers := []TrustedIssuer{ + { + IssuerURL: issuerAURL, + ExpectedAudience: audienceA, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"agent-a"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }, + { + IssuerURL: issuerBURL, + ExpectedAudience: audienceB, + JWKSURL: jwksServer.URL + "/jwks", + AllowedActors: []string{"agent-b"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }, + } + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + tokenFor := func(issuer, audience, actor, jti string) string { + now := time.Now() + claims := jwt.Claims{ + Subject: "shared-user", + Issuer: issuer, + Audience: jwt.Audience{audience}, + Expiry: jwt.NewNumericDate(now.Add(time.Hour)), + IssuedAt: jwt.NewNumericDate(now), + NotBefore: jwt.NewNumericDate(now.Add(-time.Minute)), + ID: jti, + } + return sharedJWKS.signToken(t, claims, map[string]any{"azp": actor}) + } + + resultA, err := validator.Validate(context.Background(), tokenFor(issuerAURL, audienceA, "agent-a", "jti-a")) + require.NoError(t, err, "first issuer to register the shared jwks_url must validate") + assert.Equal(t, issuerAURL, resultA.ExternalIssuer) + + resultB, err := validator.Validate(context.Background(), tokenFor(issuerBURL, audienceB, "agent-b", "jti-b")) + require.NoError(t, err, "second issuer sharing the same jwks_url under the same policy must also validate") + assert.Equal(t, issuerBURL, resultB.ExternalIssuer) +} + +// TestMultiIssuerTokenValidator_SharedJWKSURL_DifferingPolicy proves the +// property a per-issuer jwk.Cache adds over a shared one: two issuers +// resolving to the same jwks_url but configuring DIFFERENT +// insecure_allow_http/allow_private_ips settings both validate +// independently, each fetching through its own dedicated *http.Client. A +// shared cache could not do this — httprc keys a cached resource by URL +// alone and only honors jwk.WithHTTPClient on a URL's first Register call, +// so the second issuer would have silently inherited the first one's client +// and transport policy. Splitting the cache per issuer removes that +// collision instead of merely guarding against it. +func TestMultiIssuerTokenValidator_SharedJWKSURL_DifferingPolicy(t *testing.T) { + t.Parallel() + + const ( + issuerAURL = "https://issuer-a.example.com" + issuerBURL = "https://issuer-b.example.com" + audienceA = "aud-a" + audienceB = "aud-b" + ) + + selfJWKS := newTestJWKS(t) + sharedJWKS := newTestJWKS(t) + jwksServer := startJWKSServer(t, sharedJWKS) + sharedJWKSURL := jwksServer.URL + "/jwks" + + // Deliberately NOT newMultiValidator: that helper forces + // InsecureAllowHTTP and AllowPrivateIPs to true on every issuer so its + // loopback httptest servers are reachable, which would erase the very + // difference this test exists to exercise. Both issuers share one + // plain-HTTP loopback jwks_url and allow private IPs, and differ ONLY in + // InsecureAllowHTTP — so each is judged against its own transport policy: + // A is refused for its own reason (no HTTP permitted), B succeeds. + // + // Under a shared cache B could not succeed here: the policy-claim guard + // rejected any second issuer whose policy differed from the URL's first + // claimant, and without that guard B would have silently inherited A's + // client. Per-issuer caches make both outcomes independent. + selfValidator, err := NewSelfIssuedTokenValidator(selfJWKS.publicJWKS(), testIssuer, []string{testIssuer}) + require.NoError(t, err) + validator, err := NewMultiIssuerTokenValidator(selfValidator, testIssuer, []TrustedIssuer{ + { + IssuerURL: issuerAURL, + ExpectedAudience: audienceA, + JWKSURL: sharedJWKSURL, + AllowedActors: []string{"agent-a"}, + AllowPrivateIPs: true, + InsecureAllowHTTP: false, + AllowedDelegateClients: []string{anyDelegateClient}, + }, + { + IssuerURL: issuerBURL, + ExpectedAudience: audienceB, + JWKSURL: sharedJWKSURL, + AllowedActors: []string{"agent-b"}, + AllowPrivateIPs: true, + InsecureAllowHTTP: true, + AllowedDelegateClients: []string{anyDelegateClient}, + }, + }) + require.NoError(t, err) + + tokenFor := func(issuer, audience, actor, jti string) string { + now := time.Now() + claims := jwt.Claims{ + Subject: "shared-user", + Issuer: issuer, + Audience: jwt.Audience{audience}, + Expiry: jwt.NewNumericDate(now.Add(time.Hour)), + IssuedAt: jwt.NewNumericDate(now), + NotBefore: jwt.NewNumericDate(now.Add(-time.Minute)), + ID: jti, + } + return sharedJWKS.signToken(t, claims, map[string]any{"azp": actor}) + } + + // A is judged against its OWN policy: it forbids plain HTTP, so its fetch + // of the shared http:// jwks_url is refused. Not a policy-conflict error — + // A is simply misconfigured for this URL. + _, err = validator.Validate(context.Background(), tokenFor(issuerAURL, audienceA, "agent-a", "jti-a")) + require.Error(t, err, "the issuer forbidding plain HTTP must be refused for its own jwks_url") + assert.Contains(t, err.Error(), "must use HTTPS", + "the refusal must come from issuer A's own transport policy") + + // B shares that exact URL but permits HTTP, and succeeds — the outcome a + // shared cache could not produce, since A reached the URL first. + resultB, err := validator.Validate(context.Background(), tokenFor(issuerBURL, audienceB, "agent-b", "jti-b")) + require.NoError(t, err, "the issuer permitting HTTP must validate independently, "+ + "neither blocked by nor inheriting issuer A's stricter policy") + assert.Equal(t, issuerBURL, resultB.ExternalIssuer) +} + +// TestMultiIssuerTokenValidator_RetryAfterFetchFailureRefreshes proves the +// regression-safety half of the ensureRegistered rewrite (removing +// externalIssuerConfig.added in favor of asking issuerConfig.jwksCache.IsRegistered +// directly): once a JWKS fetch has failed but the resource was genuinely +// registered with the issuer's own cache, a later retry — once +// jwksFetchFailureBackoff has elapsed — must refresh the existing +// registration rather than attempt to register it again, which would fail +// with "already registered". +func TestMultiIssuerTokenValidator_RetryAfterFetchFailureRefreshes(t *testing.T) { + t.Parallel() + + selfJWKS := newTestJWKS(t) + externalJWKS := newTestJWKS(t) + + var succeed atomic.Bool + var fetchCount atomic.Int32 + mux := http.NewServeMux() + mux.HandleFunc("/jwks", func(w http.ResponseWriter, _ *http.Request) { + fetchCount.Add(1) + if !succeed.Load() { + w.WriteHeader(http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(externalJWKS.publicJWKS()) + }) + srv := httptest.NewServer(mux) + t.Cleanup(srv.Close) + + trustedIssuers := []TrustedIssuer{{ + IssuerURL: testExternalIssuer, + ExpectedAudience: testExternalAudience, + JWKSURL: srv.URL + "/jwks", + AllowedActors: []string{"ext-agent"}, + AllowedDelegateClients: []string{anyDelegateClient}, + }} + validator := newMultiValidator(t, selfJWKS, trustedIssuers) + + tokenWithID := func(jti string) string { + claims := externalClaims() + claims.ID = jti + return externalJWKS.signToken(t, claims, map[string]any{"azp": "ext-agent"}) + } + + // First attempt: the endpoint is broken. This genuinely registers the + // resource with the issuer's own cache (per httprc.Controller.Add's own + // ordering), but the fetch itself fails. + _, err := validator.Validate(context.Background(), tokenWithID("jti-1")) + require.Error(t, err) + require.Equal(t, int32(1), fetchCount.Load()) + + // Force the backoff gate open, as if jwksFetchFailureBackoff had elapsed, + // without waiting the real 30s out. + issuerConfig := validator.issuers[testExternalIssuer] + issuerConfig.mu.Lock() + issuerConfig.fetchFailedAt = time.Now().Add(-jwksFetchFailureBackoff - time.Second) + issuerConfig.mu.Unlock() + + // The endpoint now recovers. The retry must go through Refresh — a + // second Register call against the same URL would fail with "already + // registered". + succeed.Store(true) + result, err := validator.Validate(context.Background(), tokenWithID("jti-2")) + require.NoError(t, err, "retry after a registered-but-failed fetch must refresh, not re-register") + require.NotNil(t, result) +} + +// TestLimitedBodyTransport asserts directly on the body cap that protects the +// JWKS fetch path. A direct test is necessary rather than sufficient coverage +// via Validate: jwx surfaces every fetch failure as its own WaitReady timeout, +// so the cap's error never reaches a caller and cannot be distinguished there +// from a 500, a parse failure, or a kid mismatch. Asserting on the cap itself +// is the only way to pin it — the oversized case in +// TestMultiIssuerTokenValidator_FetchJWKS proves the fetch fails, not why. +func TestLimitedBodyTransport(t *testing.T) { + t.Parallel() + + const bodyCap = 1024 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(strings.Repeat("a", 8*1024))) + })) + t.Cleanup(srv.Close) + + client := srv.Client() + client.Transport = &limitedBodyTransport{base: client.Transport, max: bodyCap} + + resp, err := client.Get(srv.URL) + require.NoError(t, err, "the cap applies to reading the body, not to the round trip") + t.Cleanup(func() { _ = resp.Body.Close() }) + + body, err := io.ReadAll(resp.Body) + require.Error(t, err, "reading past the cap must fail rather than truncate silently: "+ + "a truncated JWKS would be parsed as though it were the whole document") + assert.LessOrEqual(t, int64(len(body)), int64(bodyCap), + "no more than the cap may be delivered before the error") +} diff --git a/pkg/authserver/server/tokenexchange/validator.go b/pkg/authserver/server/tokenexchange/validator.go index a1a3d6bd76..665dba033a 100644 --- a/pkg/authserver/server/tokenexchange/validator.go +++ b/pkg/authserver/server/tokenexchange/validator.go @@ -8,8 +8,10 @@ package tokenexchange import ( "context" + "errors" "fmt" "slices" + "strings" "time" "github.com/go-jose/go-jose/v4" @@ -50,12 +52,55 @@ type ValidatedClaims struct { Email string // ClientID is the OAuth client ID from the custom "client_id" claim. ClientID string - // Scopes is the space-delimited scope string from the "scope" claim. - // Empty if the subject token carries no scope claim. + // Scopes is the space-delimited scope string assembled from the "scope" + // or "scp" claim. RFC 9068 §2.2.1 spells it "scope" as a JSON string, but + // fosite's default JWT claims strategy (token/jwt/claims_jwt.go) writes + // scopes as a JSON array under "scp" instead, unless ScopeField is + // explicitly set to String or Both — this server does not set it, so a + // genuine ToolHive-issued access token used as a subject token carries + // "scp", not "scope". When both are present, "scope" wins. Empty if the + // subject token carries neither claim. Scopes string // MayAct holds the authorized actor from the "may_act" claim (RFC 8693 §4.4). // Nil when the subject token does not carry a may_act claim. MayAct *MayActClaim + // ExternalActor is the client identity that the external-issuer validation + // path has already authorized for delegation, via a per-issuer actor-claim + // allowlist match. It is set ONLY by that path (multi_issuer_validator.go), + // after the resolved actor claim is confirmed present in the issuer's + // AllowedActors — never populated from token claims by buildValidatedClaims + // or assignClaim. It is empty for self-issued tokens and for external + // tokens that carry a may_act claim (may_act is authoritative there + // instead). A non-empty value means the validator has already authorized + // this token's actor for delegation; it is not itself a raw claim. + ExternalActor string + // ExternalIssuer is set by the external-issuer validation path + // (validateExternalToken in multi_issuer_validator.go) to that issuer's + // IssuerURL, for EVERY external token it validates — unlike + // ExternalActor, this is set regardless of whether the token carries a + // may_act claim. It exists so the handler can record provenance (which + // issuer a delegation actually originated from) even for a + // may_act-bearing external token, which leaves ExternalActor unset. + // Empty for self-issued tokens. Like ExternalActor, it is never + // populated from token claims by buildValidatedClaims or assignClaim — + // it comes from the already-validated issuer config the token matched, + // not from anything token-supplied, so it cannot be spoofed via claims. + ExternalIssuer string + // AllowedDelegateClients is set for EVERY external token — like + // ExternalIssuer and unlike ExternalActor, it does not depend on whether + // the token carries a may_act claim (see validateExternalToken). That + // matters: the may_act path bypasses the AllowedActors allowlist + // entirely, so it is the path that most needs this restriction to still + // apply. It is set to that issuer's configured + // TrustedIssuer.AllowedDelegateClients, and is never populated + // from token claims by buildValidatedClaims or assignClaim, and the + // validator never compares it against anything: the validator does not + // know the authenticated ToolHive client, so checkDelegationConsent + // (handler.go) is the one that checks actorID against this list. Nil + // means the issuer did not configure AllowedDelegateClients, which is + // permissive — any ToolHive client may use the allowlisted external + // actor, same as before this field existed. + AllowedDelegateClients []string // Extra contains all non-standard claims not captured by other fields. Extra map[string]any } @@ -64,6 +109,16 @@ type ValidatedClaims struct { // It identifies the party authorized to act on behalf of the subject. type MayActClaim struct { Sub string `json:"sub"` + // Iss, when present, qualifies Sub's namespace. RFC 8693 §4.4: "the + // combination of the two claims 'iss' and 'sub' are sometimes necessary + // to uniquely identify an authorized actor." Without it, Sub alone could + // name a party in some other issuer's namespace while still being + // compared against a ToolHive client ID by checkDelegationConsent — a + // namespace-confusion bug, not just a missing feature. validateMayActShape + // requires Iss, when present, to equal this authorization server's own + // issuer, so by the time checkDelegationConsent reads Sub it is + // guaranteed to be in ToolHive's own client namespace. + Iss string `json:"iss,omitempty"` } // Compile-time check that SelfIssuedTokenValidator implements SubjectTokenValidator. @@ -122,7 +177,12 @@ func (v *SelfIssuedTokenValidator) Validate(_ context.Context, rawToken string) return nil, fmt.Errorf("subject token is not a valid JWT: %w", err) } - standardClaims, extraClaims, err := verifySignature(parsedToken, v.publicJWKS) + // kidMatched is discarded here: it exists to let the external-issuer path + // (validateExternalToken in multi_issuer_validator.go) decide whether an + // unknown kid is worth an on-demand JWKS refresh. This validator's JWKS + // never comes over HTTP — it's this authorization server's own + // PublicJWKS() — so there is nothing to refresh. + standardClaims, extraClaims, _, err := verifySignature(parsedToken, v.publicJWKS) if err != nil { return nil, err } @@ -154,18 +214,19 @@ func (v *SelfIssuedTokenValidator) Validate(_ context.Context, rawToken string) return nil, fmt.Errorf("subject token is missing required 'sub' claim") } - // If may_act is present, it must be a well-formed object with a string sub. - // A present-but-malformed may_act is treated as an invalid token, not - // an absent one — fail closed to prevent silent downgrade to client_id. - if rawMayAct, ok := extraClaims["may_act"]; ok && rawMayAct != nil { - m, ok := rawMayAct.(map[string]any) - if !ok { - return nil, fmt.Errorf("subject token has malformed 'may_act' claim: expected a JSON object") - } - sub, ok := m["sub"].(string) - if !ok || sub == "" { - return nil, fmt.Errorf("subject token has malformed 'may_act' claim: missing or invalid 'sub'") - } + // Reject a subject token that is actually an ID token — see + // rejectIDTokenClaims's doc comment. + if err := rejectIDTokenClaims(extraClaims); err != nil { + return nil, err + } + + // If may_act is present, it must be well-formed. A present-but-malformed + // may_act is treated as an invalid token, not an absent one — fail closed + // to prevent silent downgrade to client_id. requireIss is false here: see + // validateMayActShape's doc comment for why the self-issued path doesn't + // need it. + if err := validateMayActShape(extraClaims, v.issuer, false); err != nil { + return nil, err } return buildValidatedClaims(standardClaims, extraClaims), nil @@ -175,34 +236,66 @@ func (v *SelfIssuedTokenValidator) Validate(_ context.Context, rawToken string) // If the JWT header names a key ID (kid) that matches a key in the JWKS, only that // key is tried: a token naming a kid but signed by a different key is a spoofed-kid // attempt and must fail, not fall back to verifying against the rest of the keyset. -// When there is no kid, or the kid doesn't match any key in the JWKS, every key is -// tried in turn. Returns on the first successful verification. On failure, the last -// verification error is wrapped. +// When there is no kid, or the kid doesn't match any key in the JWKS, every key whose +// declared "use"/"alg" (RFC 7517 §4.2/§4.4) is compatible with this signature is +// tried in turn — a key marked for a different use (e.g. "enc") or a different +// algorithm is skipped rather than tried, per RFC 8725 §3.12's recommendation to +// scope keys to a single purpose as a substitution defense. Returns on the first +// successful verification. On failure, the last verification error is wrapped. +// +// The returned kidMatched is true only when the token's kid header was found among +// publicJWKS's keys — regardless of whether the signature itself then verified. The +// external-issuer path (multi_issuer_validator.go) uses this to tell "this key +// genuinely isn't in our cached JWKS yet, possibly a rotation" (kidMatched false) +// apart from "this kid exists here but the signature is wrong" (kidMatched true, +// which a refresh can never fix and must not retry). func verifySignature( token *jwt.JSONWebToken, publicJWKS *jose.JSONWebKeySet, -) (jwt.Claims, map[string]any, error) { +) (jwt.Claims, map[string]any, bool, error) { candidates := publicJWKS.Keys + var kidMatched bool if len(token.Headers) > 0 && token.Headers[0].KeyID != "" { if matched := publicJWKS.Key(token.Headers[0].KeyID); len(matched) > 0 { candidates = matched + kidMatched = true } } + var headerAlg string + if len(token.Headers) > 0 { + headerAlg = token.Headers[0].Algorithm + } + var lastErr error + var filtered int for _, key := range candidates { + if key.Use != "" && key.Use != "sig" { + filtered++ + continue + } + if key.Algorithm != "" && key.Algorithm != headerAlg { + filtered++ + continue + } var claims jwt.Claims extra := map[string]any{} err := token.Claims(key, &claims, &extra) if err == nil { - return claims, extra, nil + return claims, extra, kidMatched, nil } lastErr = err } if lastErr != nil { - return jwt.Claims{}, nil, fmt.Errorf("subject token signature verification failed: %w", lastErr) + return jwt.Claims{}, nil, kidMatched, fmt.Errorf("subject token signature verification failed: %w", lastErr) + } + if filtered > 0 { + return jwt.Claims{}, nil, kidMatched, fmt.Errorf( + "subject token signature verification failed: no compatible keys in JWKS "+ + "(%d key(s) present but all skipped by use/alg filter)", + len(candidates)) } - return jwt.Claims{}, nil, fmt.Errorf("subject token signature verification failed: no keys in JWKS") + return jwt.Claims{}, nil, kidMatched, fmt.Errorf("subject token signature verification failed: no keys in JWKS") } // validateAudience checks that tokenAudience intersects allowedAudiences. @@ -248,13 +341,57 @@ func buildValidatedClaims( assignClaim(vc, k, val) } + // Fall back to "scp" only if "scope" (handled above) didn't already set + // Scopes. This must run after the loop, not inside assignClaim's per-key + // switch, because map iteration order is random — assignClaim can't tell + // whether a not-yet-seen "scope" claim will still show up. + // + // Without this fallback, a genuine self-issued subject token (which + // carries "scp", not "scope") reads as scope-less and any scoped + // exchange fails with invalid_scope — scoped delegation against a + // self-issued token was effectively unusable before this fallback. + if vc.Scopes == "" { + if scp, ok := extra["scp"]; ok { + vc.Scopes = scpToScopeString(scp) + } + } + return vc } +// scpToScopeString normalizes an "scp" claim value into a space-delimited +// scope string. fosite's default JWT claims strategy writes "scp" as a JSON +// array (see the Scopes field doc comment), which json.Unmarshal decodes as +// []any — but a plain space-delimited string is accepted too, in case an +// issuer writes it that way instead. Non-string array elements are skipped +// rather than rejected: a malformed entry degrades to a smaller scope set +// instead of failing subject-token validation outright. +func scpToScopeString(val any) string { + switch v := val.(type) { + case string: + return v + case []any: + scopes := make([]string, 0, len(v)) + for _, item := range v { + if s, ok := item.(string); ok { + scopes = append(scopes, s) + } + } + return strings.Join(scopes, " ") + default: + return "" + } +} + // assignClaim routes one non-standard JWT claim onto its structured // ValidatedClaims field, or into Extra if it isn't a well-known claim. // Registered JWT claims (sub, iss, aud, exp, iat, nbf, jti) are dropped — // they're already captured in buildValidatedClaims's structured fields. +// "scp" is likewise dropped here, though its value is read separately (see +// buildValidatedClaims's post-loop fallback) since its precedence relative +// to "scope" can't be decided from a single, order-independent key/value +// pair. Keep actorClaimsNotInExtra (multi_issuer_validator.go) in sync with +// the cases here. func assignClaim(vc *ValidatedClaims, key string, val any) { switch key { case "name": @@ -273,10 +410,19 @@ func assignClaim(vc *ValidatedClaims, key string, val any) { if s, ok := val.(string); ok { vc.Scopes = s } + case "scp": + // Handled by buildValidatedClaims after its call to assignClaim for + // every key completes, so "scope" (if present) wins regardless of + // map iteration order. Case exists here only to keep "scp" out of + // Extra, like every other well-known claim below. case "may_act": if m, ok := val.(map[string]any); ok { if s, ok := m["sub"].(string); ok { - vc.MayAct = &MayActClaim{Sub: s} + mayAct := &MayActClaim{Sub: s} + if iss, ok := m["iss"].(string); ok { + mayAct.Iss = iss + } + vc.MayAct = mayAct } } case "sub", "iss", "aud", "exp", "iat", "nbf", "jti": @@ -285,3 +431,115 @@ func assignClaim(vc *ValidatedClaims, key string, val any) { vc.Extra[key] = val } } + +// validateMayActShape rejects a present-but-malformed may_act claim. Both +// validation paths (self-issued here, external in multi_issuer_validator.go) +// call this before buildValidatedClaims: assignClaim silently drops a +// malformed may_act rather than surfacing it, and each path's fallback +// consent signal when may_act is absent (client_id here, the actor allowlist +// externally) is weaker than what a well-formed may_act would have granted. +// Letting a malformed claim fall through would silently widen consent +// instead of rejecting the token. +// +// selfIssuer is this authorization server's own issuer identifier. When +// may_act carries an "iss" member, RFC 8693 §4.4 uses it together with "sub" +// to identify the actor's namespace; this server only ever grants delegation +// to its own clients (checkDelegationConsent compares may_act.sub against a +// ToolHive client ID), so an "iss" naming any other issuer would mean sub is +// being read out of the wrong namespace. Requiring iss == selfIssuer when +// present — same fail-closed treatment as a malformed sub — prevents that +// namespace confusion. +// +// requireIss makes "iss" mandatory rather than merely constrained when +// present. The self-issued path passes false: the subject token's own "iss" +// already equals selfIssuer (Validate routed it here on exactly that basis), +// so may_act.iss would be redundant to require. The external path +// (multi_issuer_validator.go) passes true: there, an external issuer that +// emits no "iss" on may_act could otherwise authorize ANY ToolHive client by +// naming its ID in a bare may_act.sub, entirely bypassing this issuer's +// AllowedActors/AllowedDelegateClients allowlists — the one consent path +// that does. No major IdP is known to emit may_act at all (Microsoft Entra, +// Okta, and Google do not; Keycloak emits "act", a different claim), so +// reaching this path already requires an operator configuring a custom +// claim mapper for a nonstandard claim — an operator capable of adding "sub" +// to that mapper is assumed capable of adding "iss" too. That assumption is +// not verified against any real IdP's behavior in this repo; it is the +// rationale for requireIss, not a tested fact. +func validateMayActShape(extra map[string]any, selfIssuer string, requireIss bool) error { + raw, ok := extra["may_act"] + if !ok || raw == nil { + return nil + } + m, ok := raw.(map[string]any) + if !ok { + return errors.New("subject token has malformed 'may_act' claim: expected a JSON object") + } + if sub, ok := m["sub"].(string); !ok || sub == "" { + return errors.New("subject token has malformed 'may_act' claim: missing or invalid 'sub'") + } + rawIss, present := m["iss"] + if !present { + if requireIss { + return errors.New("subject token has malformed 'may_act' claim: missing required 'iss'") + } + return nil + } + iss, ok := rawIss.(string) + if !ok || iss == "" { + return errors.New("subject token has malformed 'may_act' claim: invalid 'iss'") + } + if iss != selfIssuer { + return fmt.Errorf( + "subject token has malformed 'may_act' claim: 'iss' %q does not match this authorization server's issuer", + iss) + } + return nil +} + +// rejectIDTokenClaims rejects a subject token that carries "at_hash" or +// "c_hash" — OIDC Core §3.3.2.11 / §3.3.2.10 define both for ID tokens only, +// never present on an access token. This is a sound POSITIVE detector (their +// presence proves an ID token) but is NOT EXHAUSTIVE: both claims are +// OPTIONAL even on a valid authorization-code-flow ID token, so an ID token +// that omits them passes this check undetected. A "typ: at+jwt" header check +// is not viable as a replacement: Entra v1/v2, Okta, and Google all emit bare +// "JWT" for access tokens too. Deliberately does NOT check "nonce": Microsoft +// Graph access tokens carry one, so rejecting on its presence would reject +// legitimate Entra access tokens. +// +// The other, complementary layer is the audience-shape check on the external +// path: TrustedIssuer.ExpectedAudience (multi_issuer_validator.go) is +// documented as MUST be a resource identifier, not a client ID, since an ID +// token's "aud" is the requesting client's own ID. That check is +// operator-dependent, not self-enforcing — see ExpectedAudience's doc +// comment and looksLikeResourceIdentifier's startup warning. Between the two, +// ID/access-token discrimination for an external issuer is partial: an +// operator who mis-sets ExpectedAudience to a client ID, presented with a +// real IdP whose ID tokens omit at_hash/c_hash (both legal per OIDC Core), +// has no remaining defense here. The blast radius is bounded elsewhere: an +// admitted ID token carries no "scope"/"scp" claim, so grantScopes +// (handler.go) either rejects the exchange with invalid_scope or grants a +// zero-scope delegated token. +// +// A third discriminator was considered and rejected: an ID token's "aud" +// MUST equal its own "azp"/authorized-party claim (OIDC Core §2, §3.1.3.7 +// step 9), so rejecting a token whose "aud" equals the issuer's configured +// actor claim looked like a candidate. It is not shipped, because it also +// fires on a legitimate access token: an app whose API and client share one +// registration — e.g. an Entra v1 app that calls its own exposed API, or any +// setup where ExpectedAudience is misconfigured to the app's bare +// client/resource GUID, which is exactly the ambiguous case this file's +// audience-shape check already warns about — issues access tokens whose +// "aud" is that shared ID and whose azp/appid is the very same ID. Rejecting +// on aud == azp would then reject that provider's genuine access tokens, not +// just its ID tokens. A false-positive risk of that shape is worse than the +// gap it would close. +func rejectIDTokenClaims(extra map[string]any) error { + if _, ok := extra["at_hash"]; ok { + return errors.New("subject token carries an 'at_hash' claim: ID tokens are not accepted as subject tokens") + } + if _, ok := extra["c_hash"]; ok { + return errors.New("subject token carries a 'c_hash' claim: ID tokens are not accepted as subject tokens") + } + return nil +} diff --git a/pkg/authserver/server/tokenexchange/validator_test.go b/pkg/authserver/server/tokenexchange/validator_test.go index 783203b0a2..56877792d4 100644 --- a/pkg/authserver/server/tokenexchange/validator_test.go +++ b/pkg/authserver/server/tokenexchange/validator_test.go @@ -296,6 +296,69 @@ func TestSelfIssuedTokenValidator_Validate(t *testing.T) { wantErr: true, errContains: "malformed 'may_act' claim", }, + { + name: "may_act with iss matching this server's own issuer accepted", + token: func(t *testing.T) string { + t.Helper() + extra := validExtraClaims() + extra["may_act"] = map[string]any{"sub": "authorized-agent", "iss": testIssuer} + return tj.signToken(t, validClaims(), extra) + }, + check: func(t *testing.T, vc *ValidatedClaims) { + t.Helper() + require.NotNil(t, vc.MayAct) + assert.Equal(t, "authorized-agent", vc.MayAct.Sub) + assert.Equal(t, testIssuer, vc.MayAct.Iss) + }, + }, + { + // RFC 8693 §4.4: sub is only meaningful together with iss. This + // server only ever grants delegation to its own clients, so an + // iss naming a different issuer is namespace confusion, not a + // legitimate qualifier — reject rather than silently ignore it. + name: "malformed may_act — iss not matching this server's own issuer", + token: func(t *testing.T) string { + t.Helper() + extra := validExtraClaims() + extra["may_act"] = map[string]any{"sub": "authorized-agent", "iss": "https://evil.example.com"} + return tj.signToken(t, validClaims(), extra) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + name: "malformed may_act — empty iss", + token: func(t *testing.T) string { + t.Helper() + extra := validExtraClaims() + extra["may_act"] = map[string]any{"sub": "authorized-agent", "iss": ""} + return tj.signToken(t, validClaims(), extra) + }, + wantErr: true, + errContains: "malformed 'may_act' claim", + }, + { + name: "ID token masquerading as subject token rejected — at_hash present", + token: func(t *testing.T) string { + t.Helper() + extra := validExtraClaims() + extra["at_hash"] = "some-hash-value" + return tj.signToken(t, validClaims(), extra) + }, + wantErr: true, + errContains: "'at_hash'", + }, + { + name: "ID token masquerading as subject token rejected — c_hash present", + token: func(t *testing.T) string { + t.Helper() + extra := validExtraClaims() + extra["c_hash"] = "some-hash-value" + return tj.signToken(t, validClaims(), extra) + }, + wantErr: true, + errContains: "'c_hash'", + }, } for _, tt := range tests { @@ -527,4 +590,55 @@ func TestSelfIssuedTokenValidator_MultiKeyJWKS(t *testing.T) { require.NotNil(t, result) assert.Equal(t, "user-123", result.Subject) }) + + t.Run("key marked for a non-signing use is never tried", func(t *testing.T) { + t.Parallel() + + // Same key material a signature could actually verify against, but + // declared "enc" in the JWKS — RFC 7517 §4.2 reserves it for a + // different purpose. Without the use filter, this key would still + // verify the signature below and wrongly accept the token. + jwk := newECDSAJWK(t, "enc-key") + jwk.Use = "enc" + jwks := publicJWKSOf(jwk) + + validator, err := NewSelfIssuedTokenValidator(jwks, testIssuer, []string{testIssuer}) + require.NoError(t, err) + + // No kid on the token, so verifySignature must fall back to trying + // every key in the JWKS — and skip this one. + signingKeyNoKID := jwk + signingKeyNoKID.KeyID = "" + rawToken := signWithJWK(t, signingKeyNoKID, jose.ES256, validClaims()) + + result, err := validator.Validate(context.Background(), rawToken) + require.Error(t, err) + assert.Contains(t, err.Error(), "signature verification failed") + assert.Nil(t, result) + }) + + t.Run("key with a mismatched declared algorithm is never tried", func(t *testing.T) { + t.Parallel() + + // The JWKS entry claims RS256, but the token header (and the actual + // signing) uses ES256 — a misconfigured or spoofed JWKS entry. + // Without the alg filter, go-jose would still attempt (and, for an + // EC key, fail) this candidate; the point here is that it must be + // skipped rather than tried at all. + jwk := newECDSAJWK(t, "mismatched-alg-key") + jwk.Algorithm = string(jose.RS256) + jwks := publicJWKSOf(jwk) + + validator, err := NewSelfIssuedTokenValidator(jwks, testIssuer, []string{testIssuer}) + require.NoError(t, err) + + signingKeyNoKID := jwk + signingKeyNoKID.KeyID = "" + rawToken := signWithJWK(t, signingKeyNoKID, jose.ES256, validClaims()) + + result, err := validator.Validate(context.Background(), rawToken) + require.Error(t, err) + assert.Contains(t, err.Error(), "signature verification failed") + assert.Nil(t, result) + }) } diff --git a/pkg/authserver/server_impl.go b/pkg/authserver/server_impl.go index c218486fde..458ac1b273 100644 --- a/pkg/authserver/server_impl.go +++ b/pkg/authserver/server_impl.go @@ -249,7 +249,7 @@ func decorateStorageForCIMD(cfg Config, stor storage.Storage) (storage.Storage, func buildProvider( cfg Config, authServerConfig *oauthserver.AuthorizationServerConfig, stor storage.Storage, ) (fosite.OAuth2Provider, error) { - tokenExchangeFactory, err := tokenexchange.Factory(cfg.DelegationTokenLifespan) + tokenExchangeFactory, err := tokenexchange.Factory(cfg.DelegationTokenLifespan, cfg.TrustedIssuers) if err != nil { return nil, fmt.Errorf("failed to create token exchange factory: %w", err) }