From 38a1f1049b979de968e8a15a10c5710b3a39cef0 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sun, 12 Apr 2026 22:40:12 +0100 Subject: [PATCH 1/9] Support actor_token and id_token in RFC 8693 token exchange MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An agent calling the token-exchange grant could previously only be identified by its own OAuth client credentials at the endpoint. RFC 8693 also defines an explicit actor_token: a second, self-issued JWT the agent presents alongside the user's subject_token, giving the exchange a request-level proof of possession distinct from client authentication. The handler unconditionally rejected both actor_token and actor_token_type before this change ("not yet supported"), and only accepted urn:...:access_token/jwt as subject_token_type, so a subject token minted as an OIDC id_token (a legitimate shape from many IdPs) had no way to be exchanged. What changed: - Accept actor_token + actor_token_type instead of rejecting them outright. resolveActorIdentity validates actor_token against the server's own JWKS (self-issued only — an actor_token is never accepted from an external trusted issuer) and requires its "sub" to equal the authenticated client's ID before the exchange proceeds. - Accept id_token as a valid subject_token_type value. - Removed the now-superseded validateExchangeParams helper (the old, actor_token-rejecting parameter validator) in favor of the new validateFormParams/resolveActorIdentity split. - Corrected docs/arch/token-delegation-act-chain.md, which still described RFC 8693's chained-act-claim nesting as "not implemented" — that landed separately in #6149 before this branch was rebased; the doc had gone stale, not the behavior. - Added an integration test proving actor_token composes correctly with the configured-delegate-client relaxation (a client granted blanket self-issued-token trust): a mismatched actor_token must still be rejected during actor-identity resolution before delegation consent is ever reached, so that blanket trust can never be misread as also loosening the actor_token binding check. What this enables: a client can additionally prove it holds a second, independently-issued token bound to its own client_id at exchange time, and subject tokens minted as id_tokens by IdPs that issue that shape become exchangeable. What this deliberately does NOT do, by design: actor_token's own claims never flow into the delegated token's "act" claim. Because "sub" must equal client.GetID(), the resulting actor identity is identical whether or not actor_token is supplied — this is actor-token *confirmation* (proof of possession), not RFC 8693's general actor-delegation use case of asserting a distinct sub-client-granularity actor. That's a scope boundary recorded in resolveActorIdentity's doc comment, not an oversight. No new server configuration is introduced — actor_token/actor_token_type are request-time form parameters at /oauth/token, not RunConfig/CRD fields. Example request, assuming a confidential client already registered for the token-exchange grant: POST /oauth/token Content-Type: application/x-www-form-urlencoded grant_type=urn:ietf:params:oauth:grant-type:token-exchange &subject_token= &subject_token_type=urn:ietf:params:oauth:token-type:id_token &actor_token= &actor_token_type=urn:ietf:params:oauth:token-type:jwt &client_id=agent-client-id &client_secret=... The delegated access token's "act" claim is unaffected by actor_token's presence — it always names the authenticated client: "act": { "sub": "agent-client-id" } Closes #5815 --- docs/arch/token-delegation-act-chain.md | 35 ++++ docs/arch/token-delegation-actor-id.md | 103 +++++++++++ ...delegate_client_runner_integration_test.go | 151 +++++++++++++++- .../server/tokenexchange/factory.go | 1 + .../server/tokenexchange/handler.go | 167 +++++++++++++----- .../server/tokenexchange/handler_test.go | 148 ++++++++++++++++ 6 files changed, 561 insertions(+), 44 deletions(-) create mode 100644 docs/arch/token-delegation-act-chain.md create mode 100644 docs/arch/token-delegation-actor-id.md diff --git a/docs/arch/token-delegation-act-chain.md b/docs/arch/token-delegation-act-chain.md new file mode 100644 index 0000000000..6b68131f13 --- /dev/null +++ b/docs/arch/token-delegation-act-chain.md @@ -0,0 +1,35 @@ +# Delegation chain nesting (`act` claim) + +## Status + +Implemented. Token exchange preserves a prior RFC 8693 `act` claim by nesting +it under the newly resolved actor. The handler rejects malformed chains and +limits the resulting chain to ten levels. + +## Behavior + +When a delegated token is re-exchanged, the new actor is prepended to the +existing chain: + +```json +{ + "act": { + "sub": "new-actor", + "act": { + "sub": "prior-actor" + } + } +} +``` + +For an externally issued subject token, the trusted issuer provenance is also +nested before any existing chain. The handler parses the prior chain using the +shared audit parser, rejects malformed content, and caps the final depth to +avoid issuing unbounded tokens. + +## References + +- `pkg/authserver/server/tokenexchange/handler.go` — `buildActClaim` +- `pkg/authserver/server/tokenexchange/handler_test.go` — re-exchange and depth + limit coverage +- RFC 8693 section 4.1 — `act` claim semantics diff --git a/docs/arch/token-delegation-actor-id.md b/docs/arch/token-delegation-actor-id.md new file mode 100644 index 0000000000..0a9294fe12 --- /dev/null +++ b/docs/arch/token-delegation-actor-id.md @@ -0,0 +1,103 @@ +# Actor identity shape in the `act` claim + +## Status + +Not implemented. Deferred — this is a cross-cutting identity-model +decision, not a handler-level fix. + +## Problem + +The token exchange handler sets `act.sub` to the raw OAuth client ID +via `client.GetID()` (`pkg/authserver/server/tokenexchange/handler.go`): + +```go +actorID := client.GetID() +... +delegatedSession.JWTClaims.Extra["act"] = map[string]interface{}{ + "sub": actorID, +} +``` + +This produces values like `"devops-agent"` — a plain string client ID. + +However, the Cedar authorization test policies match SPIFFE-style +identities (`pkg/authz/authorizers/cedar/core_test.go`): + +``` +context.claim_act.sub like "spiffe://toolhive.dev/ns/agents/sa/*" +``` + +with test fixtures using: + +```go +"act": map[string]interface{}{ + "sub": "spiffe://toolhive.dev/ns/agents/sa/devops-agent", +}, +``` + +A token issued by this handler would **not match** a Cedar policy +written against the SPIFFE pattern, because `act.sub` is +`"devops-agent"`, not `"spiffe://toolhive.dev/ns/agents/sa/devops-agent"`. + +## Is this a bug? + +No. The Cedar tests construct their own claim fixtures with SPIFFE +URIs directly — they are illustrative of *what policies could look +like* with SPIFFE-style identities, not testing this handler's output. +The handler produces a raw `client_id`, which is consistent with what +fosite uses for client identity throughout. + +## The design question + +Should the handler transform `client.GetID()` into a SPIFFE URI before +placing it in the `act` claim? This depends on: + +1. **Agent identity model**: does toolhive's agent identity use SPIFFE + URIs? The workload identity system does (`pkg/auth/identity.go` + references SPIFFE), but the OAuth client registry stores plain + string client IDs. + +2. **Downstream consumers**: do Cedar policies, audit logs, and other + consumers expect SPIFFE URIs or raw client IDs? The Cedar test + policies suggest SPIFFE; the handler produces raw client IDs. + +3. **Cross-cutting concern**: if SPIFFE URIs are the right identifier + space, the transformation should happen at a shared layer (e.g., a + client-to-SPIFFE resolver), not hardcoded in the token exchange + handler. Other handlers that emit client identity (e.g., the + standard token handler's `client_id` claim) would need the same + transformation. + +## Options + +- **Option A (keep raw client_id)**: the handler emits `client.GetID()` + as-is. Cedar policies must match against plain client IDs. Simplest, + consistent with fosite, but doesn't align with the SPIFFE-based + workload identity model. + +- **Option B (transform to SPIFFE)**: the handler resolves the client + ID to a SPIFFE URI before placing it in `act.sub`. Requires a + client-to-SPIFFE resolver or a convention (e.g., + `spiffe://toolhive.dev/ns/agents/sa/`). Aligns with the + Cedar test policies and the workload identity model, but adds a + transformation step that other handlers would also need. + +- **Option C (store SPIFFE URI in client registry)**: the OAuth client + registration carries a SPIFFE URI alongside the client ID. The + handler uses the SPIFFE URI if present, falling back to `client_id`. + Most flexible, but requires changes to client registration. + +## Recommendation + +Defer until the agent identity model is finalized. The current +behavior (raw `client_id`) is correct for the OAuth layer and doesn't +block any functionality — it's a mismatch between test fixtures and +handler output, not a runtime bug. When the identity model decision is +made, apply it as a cross-cutting concern, not a handler-specific fix. + +## References + +- `pkg/authserver/server/tokenexchange/handler.go` — `actorID` assignment and `act` claim +- `pkg/authz/authorizers/cedar/core_test.go` — Cedar policies with SPIFFE URIs +- `pkg/authz/authorizers/cedar/entity.go` — Cedar value conversion for `act` claim +- `pkg/auth/identity.go` — SPIFFE references in workload identity diff --git a/pkg/authserver/delegate_client_runner_integration_test.go b/pkg/authserver/delegate_client_runner_integration_test.go index 112e7cab83..20b88dec0b 100644 --- a/pkg/authserver/delegate_client_runner_integration_test.go +++ b/pkg/authserver/delegate_client_runner_integration_test.go @@ -161,6 +161,111 @@ func TestConfiguredDelegateClientTokenExchange(t *testing.T) { } } +// TestConfiguredDelegateClientTokenExchange_WithActorToken proves that an +// explicit RFC 8693 actor_token composes correctly with the configured +// delegate-client relaxation instead of being tested in isolation from it. +// +// The discriminating case is the mismatched-actor_token one: a configured +// delegate client's blanket trust to exchange ANY self-issued subject token +// (see docs/arch/17-token-exchange-delegation.md, "Delegate clients and +// self-issued token exchange") must never be interpretable as also relaxing +// the actor_token binding check in resolveActorIdentity. The subject token +// deliberately uses a client_id the delegate relaxation WOULD otherwise +// excuse, so a wrongly-successful response here proves actor identity +// resolution was skipped, not merely that delegation consent was lenient. +func TestConfiguredDelegateClientTokenExchange_WithActorToken(t *testing.T) { + t.Parallel() + + const originalClientID = "original-non-delegate-client" + + tests := []struct { + name string + subjectClientID string // client_id claim baked into the subject token + actorTokenSub string // sub claim of the actor_token + wantStatus int + wantError string + }{ + { + // resolveActorIdentity's sub-mismatch branch deliberately returns + // invalid_grant, not invalid_request — a distinct "wrong party" + // error class, the same convention checkDelegationConsent uses + // for its own client_id-mismatch case elsewhere in this file. + // invalid_request is reserved for a malformed/unverifiable token. + name: "mismatched actor_token rejected before delegation consent is reached", + subjectClientID: originalClientID, + actorTokenSub: "someone-else", + wantStatus: http.StatusBadRequest, + wantError: "invalid_grant", + }, + { + name: "matching actor_token still succeeds via the delegate-client relaxation", + subjectClientID: originalClientID, + actorTokenSub: delegateClientID, + wantStatus: http.StatusOK, + }, + { + // Delegate status must not change behavior when the relaxation isn't + // needed: the subject token's client_id already matches the + // authenticated client, so this succeeds on ordinary client_id + // binding, and verifyDelegatedToken's act.sub assertion below proves + // it produces the identical act shape either way. + name: "actor_token present but relaxation unneeded still succeeds", + subjectClientID: delegateClientID, + actorTokenSub: delegateClientID, + wantStatus: http.StatusOK, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + server, issuer, embedded := startConfiguredDelegateAuthServer(t) + subjectToken := signedSubjectTokenForClient(t, embedded, issuer, tt.subjectClientID) + actorToken := signedActorToken(t, embedded, issuer, tt.actorTokenSub) + + values := url.Values{ + "grant_type": {tokenExchangeGrantType}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "actor_token": {actorToken}, + "actor_token_type": {oauthproto.TokenTypeAccessToken}, + "scope": {"openid profile"}, + "audience": {delegateAudience}, + "client_id": {delegateClientID}, + "client_secret": {delegateClientSecret}, + } + + request, err := http.NewRequest(http.MethodPost, server.URL+"/oauth/token", strings.NewReader(values.Encode())) + require.NoError(t, err) + request.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + response, err := (&http.Client{Timeout: 10 * time.Second}).Do(request) + require.NoError(t, err) + t.Cleanup(func() { + _, _ = io.Copy(io.Discard, response.Body) + require.NoError(t, response.Body.Close()) + }) + + var body map[string]any + require.NoError(t, json.NewDecoder(response.Body).Decode(&body)) + require.Equal(t, tt.wantStatus, response.StatusCode, "token response: %v", body) + if tt.wantError != "" { + assert.Equal(t, tt.wantError, body["error"]) + return + } + + accessToken, ok := body["access_token"].(string) + require.True(t, ok) + // verifyDelegatedToken asserts act.sub == delegateClientID, which + // here also confirms actor_token's own claims never leak into act: + // RFC 8693 §4.1's act.sub must name the current actor precisely + // because downstream access-control decisions key off it. + verifyDelegatedToken(t, embedded, accessToken, issuer) + }) + } +} + func assertConfiguredDelegateDiscovery(t *testing.T, serverURL string) { t.Helper() @@ -233,6 +338,19 @@ func signedDelegateSubjectToken( issuer string, ) string { t.Helper() + return signedSubjectTokenForClient(t, embedded, issuer, delegateClientID) +} + +// signedSubjectTokenForClient mints a self-issued subject token whose +// "client_id" claim is the given client — used to exercise the configured +// delegate-client relaxation, which requires a subject token originally +// obtained by a DIFFERENT client than the one performing the exchange. +func signedSubjectTokenForClient( + t *testing.T, + embedded *authserverrunner.EmbeddedAuthServer, + issuer, clientID string, +) string { + t.Helper() signingKey, err := embedded.KeyProvider().SigningKey(context.Background()) require.NoError(t, err) @@ -250,13 +368,44 @@ func signedDelegateSubjectToken( Expiry: jwt.NewNumericDate(now.Add(30 * time.Minute)), IssuedAt: jwt.NewNumericDate(now), }).Claims(map[string]any{ - "client_id": delegateClientID, + "client_id": clientID, "scope": "openid profile", }).Serialize() require.NoError(t, err) return token } +// signedActorToken mints a self-issued RFC 8693 actor_token with the given +// "sub" claim. resolveActorIdentity requires this to equal the authenticated +// client's ID or the exchange is rejected before delegation consent is ever +// consulted — see TestConfiguredDelegateClientTokenExchange_WithActorToken. +func signedActorToken( + t *testing.T, + embedded *authserverrunner.EmbeddedAuthServer, + issuer, sub string, +) string { + t.Helper() + + signingKey, err := embedded.KeyProvider().SigningKey(context.Background()) + require.NoError(t, err) + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: jose.SignatureAlgorithm(signingKey.Algorithm), Key: signingKey.Key}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", signingKey.KeyID), + ) + require.NoError(t, err) + + now := time.Now() + token, err := jwt.Signed(signer).Claims(jwt.Claims{ + Issuer: issuer, + Subject: sub, + Audience: jwt.Audience{delegateAudience}, + Expiry: jwt.NewNumericDate(now.Add(time.Hour)), + IssuedAt: jwt.NewNumericDate(now), + }).Serialize() + require.NoError(t, err) + return token +} + func verifyDelegatedToken(t *testing.T, embedded *authserverrunner.EmbeddedAuthServer, token, issuer string) { t.Helper() diff --git a/pkg/authserver/server/tokenexchange/factory.go b/pkg/authserver/server/tokenexchange/factory.go index 9fe5817e3c..f90bc21c87 100644 --- a/pkg/authserver/server/tokenexchange/factory.go +++ b/pkg/authserver/server/tokenexchange/factory.go @@ -85,6 +85,7 @@ func Factory( Config: config.Config, }, validator: validator, + selfValidator: selfValidator, delegationLifespan: delegationLifespan, config: config.Config, allowedAudiences: config.AllowedAudiences, diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index ac9c063a2e..a8657aaa0e 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -57,7 +57,8 @@ const anyDelegateClient = "*" // token effort. type Handler struct { *oauth2.HandleHelper - validator SubjectTokenValidator + validator SubjectTokenValidator // for subject tokens (multi-issuer) + selfValidator SubjectTokenValidator // for actor tokens (self-issued only) delegationLifespan time.Duration config tokenExchangeConfig allowedAudiences []string @@ -66,6 +67,13 @@ type Handler struct { configuredDelegateClients []string } +// formParams holds the validated RFC 8693 form parameters extracted from a +// token exchange request. +type formParams struct { + subjectToken string + actorToken string // empty if not provided +} + // tokenExchangeConfig defines the configuration interface needed by the handler. type tokenExchangeConfig interface { fosite.ScopeStrategyProvider @@ -116,14 +124,25 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi // already authenticated by fosite's client authentication strategy before // this handler runs. actorID := client.GetID() + form := requester.GetRequestForm() - subjectToken, err := validateExchangeParams(requester.GetRequestForm()) + // Validate required RFC 8693 form parameters. + params, err := validateFormParams(form) if err != nil { return err } - // Validate the subject token against the server's own JWKS. - validatedClaims, err := h.validator.Validate(ctx, subjectToken) + // Validate requested_token_type per RFC 8693 Section 2.1: if the client + // requests a token type the server does not support, the request must fail. + requestedTokenType := form.Get("requested_token_type") + if requestedTokenType != "" && requestedTokenType != oauthproto.TokenTypeAccessToken { + return errorsx.WithStack(fosite.ErrInvalidRequest.WithHintf( + "The 'requested_token_type' value %q is not supported. This server only issues %q.", + requestedTokenType, oauthproto.TokenTypeAccessToken)) + } + + // Validate the subject token against the configured token validator. + validatedClaims, err := h.validator.Validate(ctx, params.subjectToken) if err != nil { slog.Debug("Subject token validation failed", "error", err, @@ -142,8 +161,14 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi "The subject token is invalid or could not be verified.")) } - configuredDelegate := slices.Contains(h.configuredDelegateClients, actorID) - if err := checkDelegationConsent(validatedClaims, actorID, configuredDelegate); err != nil { + // Resolve actor identity: explicit actor_token or authenticated client. + actorSub, err := h.resolveActorIdentity(ctx, params, client) + if err != nil { + return err + } + + configuredDelegate := slices.Contains(h.configuredDelegateClients, actorSub) + if err := checkDelegationConsent(validatedClaims, actorSub, configuredDelegate); err != nil { return err } @@ -159,14 +184,14 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi delegatedSession := session.New( delegatedSubject(validatedClaims), "", // No IDP session link for delegated tokens. - actorID, + actorSub, session.UserClaims{ Name: validatedClaims.Name, Email: validatedClaims.Email, }, ) - act, err := buildActClaim(validatedClaims, actorID) + act, err := buildActClaim(validatedClaims, actorSub) if err != nil { return err } @@ -185,7 +210,7 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi slog.Debug("Token exchange request validated", "subject", validatedClaims.Subject, - "actor", actorID, + "actor", actorSub, "issuer", validatedClaims.Issuer, "subject_token_client", validatedClaims.ExternalActor, "subject_token_client_id", validatedClaims.ClientID, @@ -241,60 +266,116 @@ func (h *Handler) PopulateTokenEndpointResponse( return nil } -// computeLifetime returns the minimum of the subject token's remaining lifetime -// and the configured delegation lifespan. Returns an error if the subject token -// has already expired. -func (h *Handler) computeLifetime(subjectExpiry time.Time) (time.Duration, error) { - remaining := time.Until(subjectExpiry) - if remaining <= 0 { - return 0, fmt.Errorf("subject token expired %v ago", -remaining) +// resolveActorIdentity determines the acting party identity: always the +// authenticated OAuth client ID. +// +// This is actor-token *confirmation* (proof-of-possession hardening), not +// RFC 8693's general actor-delegation use case. When actor_token is present, +// it is validated against the AS's own JWKS and its "sub" is required to +// equal client.GetID() — so the resulting identity is identical whether or +// not actor_token was supplied at all. Presenting actor_token only proves the +// caller additionally holds a self-issued JWT for its own client_id; it never +// lets a distinct actor identity flow into the act claim. Do not repurpose +// this equality check to record a different actor identity without +// revisiting the callers that assume act.sub == the authenticated client ID. +func (h *Handler) resolveActorIdentity( + ctx context.Context, params *formParams, client fosite.Client, +) (string, error) { + if params.actorToken != "" { + // Validate actor_token against the AS's own JWKS (must be self-issued). + actorClaims, err := h.selfValidator.Validate(ctx, params.actorToken) + if err != nil { + slog.Debug("Actor token validation failed", "error", fmt.Errorf("actor token: %w", err)) + return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( + "The actor token is invalid or could not be verified.")) + } + // Binding check: actor_token.sub MUST match the authenticated client ID. + // This prevents replay attacks where a leaked actor token is presented + // by a different client. The client ID is always verified by fosite's + // client authentication before reaching here. + if actorClaims.Subject != client.GetID() { + return "", errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( + "The actor token subject does not match the authenticated client identity.")) + } + return actorClaims.Subject, nil } - if remaining < h.delegationLifespan { - return remaining, nil - } - return h.delegationLifespan, nil + // No actor_token: the authenticated client is the acting party. + return client.GetID(), nil } -// validateExchangeParams validates the required RFC 8693 form parameters and -// returns the subject token on success. -func validateExchangeParams(form url.Values) (string, error) { +// validateFormParams validates the required RFC 8693 form parameters and returns +// the parsed parameters on success. +func validateFormParams(form url.Values) (*formParams, error) { subjectToken := form.Get("subject_token") if subjectToken == "" { - return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( + return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( "The 'subject_token' parameter is required for token exchange.")) } subjectTokenType := form.Get("subject_token_type") if subjectTokenType == "" { - return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( + return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( "The 'subject_token_type' parameter is required for token exchange.")) } - if subjectTokenType != oauthproto.TokenTypeAccessToken && subjectTokenType != oauthproto.TokenTypeJWT { - return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHintf( - "The 'subject_token_type' value %q is not supported. Use %q or %q.", - subjectTokenType, oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT)) + switch subjectTokenType { + case oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT, oauthproto.TokenTypeIDToken: + // Valid subject token types. + default: + return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithHintf( + "The 'subject_token_type' value %q is not supported. Use %q, %q, or %q.", + subjectTokenType, oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT, oauthproto.TokenTypeIDToken)) } - // Reject actor_token parameters for now — the acting party identity is - // derived from the authenticated OAuth client. A later commit adds - // actor_token support for asserting a distinct actor. - if form.Get("actor_token") != "" || form.Get("actor_token_type") != "" { - return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( - "The 'actor_token' and 'actor_token_type' parameters are not yet supported.")) + actorToken := form.Get("actor_token") + actorTokenType := form.Get("actor_token_type") + + // actor_token_type without actor_token is invalid. + if actorTokenType != "" && actorToken == "" { + return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( + "The 'actor_token_type' parameter requires 'actor_token' to be present.")) } - // Validate requested_token_type per RFC 8693 Section 2.1: if the client - // requests a token type the server does not support, the request must fail. - requestedTokenType := form.Get("requested_token_type") - if requestedTokenType != "" && requestedTokenType != oauthproto.TokenTypeAccessToken { - return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHintf( - "The 'requested_token_type' value %q is not supported. This server only issues %q.", - requestedTokenType, oauthproto.TokenTypeAccessToken)) + // actor_token requires actor_token_type. + if actorToken != "" && actorTokenType == "" { + return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( + "The 'actor_token_type' parameter is required when 'actor_token' is present.")) } - return subjectToken, nil + // Validate actor_token_type if present. + // Note: id_token is intentionally excluded for actor tokens. An actor presents + // a bearer credential (access_token/jwt), not an identity assertion (id_token). + if actorTokenType != "" { + switch actorTokenType { + case oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT: + // Valid actor token types. + default: + return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithHintf( + "The 'actor_token_type' value %q is not supported. Use %q or %q.", + actorTokenType, oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT)) + } + } + + return &formParams{ + subjectToken: subjectToken, + actorToken: actorToken, + }, nil +} + +// computeLifetime returns the minimum of the subject token's remaining lifetime +// and the configured delegation lifespan. Returns an error if the subject token +// has already expired. +func (h *Handler) computeLifetime(subjectExpiry time.Time) (time.Duration, error) { + remaining := time.Until(subjectExpiry) + if remaining <= 0 { + return 0, fmt.Errorf("subject token expired %v ago", -remaining) + } + + if remaining < h.delegationLifespan { + return remaining, nil + } + return h.delegationLifespan, nil } // delegatedSubject returns the "sub" to embed in the delegated token. diff --git a/pkg/authserver/server/tokenexchange/handler_test.go b/pkg/authserver/server/tokenexchange/handler_test.go index b52bbd0616..699320ef4d 100644 --- a/pkg/authserver/server/tokenexchange/handler_test.go +++ b/pkg/authserver/server/tokenexchange/handler_test.go @@ -38,6 +38,7 @@ func newTestHandler(t *testing.T, tj *testJWKS, delegationLifespan time.Duration // PopulateTokenEndpointResponse, so IssueAccessToken is never called. HandleHelper: nil, validator: validator, + selfValidator: validator, delegationLifespan: delegationLifespan, allowedAudiences: []string{testIssuer}, config: &mockConfig{ @@ -104,6 +105,24 @@ func nestedActChain(depth int) map[string]any { return chain } +// signActorToken creates a self-issued JWT suitable for use as an actor_token. +// The sub claim is set to the given subject (typically the client_id). +func signActorToken(t *testing.T, tj *testJWKS, subject string) string { + t.Helper() + + now := time.Now() + claims := jwt.Claims{ + Subject: subject, + Issuer: testIssuer, + Audience: jwt.Audience{testIssuer}, + Expiry: jwt.NewNumericDate(now.Add(time.Hour)), + IssuedAt: jwt.NewNumericDate(now), + } + return tj.signToken(t, claims, map[string]any{ + "client_id": testAgentClientID, + }) +} + func TestTokenExchangeHandler_CanHandleTokenEndpointRequest(t *testing.T) { t.Parallel() @@ -257,6 +276,135 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { wantFositeIs: fosite.ErrInvalidGrant, hintContains: "different client", }, + { + name: "valid exchange with actor_token", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + lifespan: 15 * time.Minute, + form: func(t *testing.T) url.Values { + t.Helper() + f := defaultFormValues(t, tj) + f.Set("actor_token", signActorToken(t, tj, testAgentClientID)) + f.Set("actor_token_type", oauthproto.TokenTypeJWT) + return f + }, + check: func(t *testing.T, req *fosite.AccessRequest) { + t.Helper() + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + + // Verify subject is the user from the subject token. + assert.Equal(t, "user-123", sess.JWTClaims.Subject) + + // Verify the act claim uses the actor token's sub. + actClaim, exists := sess.JWTClaims.Extra["act"] + require.True(t, exists, "act claim must be present") + actMap, ok := actClaim.(map[string]interface{}) + require.True(t, ok, "act claim must be a map") + assert.Equal(t, testAgentClientID, actMap["sub"]) + }, + }, + { + name: "actor_token sub mismatch with client ID", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + lifespan: 15 * time.Minute, + form: func(t *testing.T) url.Values { + t.Helper() + f := defaultFormValues(t, tj) + f.Set("actor_token", signActorToken(t, tj, "other-agent")) + f.Set("actor_token_type", oauthproto.TokenTypeJWT) + return f + }, + wantErr: true, + wantFositeIs: fosite.ErrInvalidGrant, + hintContains: "does not match the authenticated client identity", + }, + { + name: "invalid actor_token returns invalid_request", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + lifespan: 15 * time.Minute, + form: func(t *testing.T) url.Values { + t.Helper() + f := defaultFormValues(t, tj) + f.Set("actor_token", "not-a-valid-jwt") + f.Set("actor_token_type", oauthproto.TokenTypeJWT) + return f + }, + wantErr: true, + wantFositeIs: fosite.ErrInvalidRequest, + hintContains: "actor token is invalid", + }, + { + name: "actor_token without actor_token_type", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + f := defaultFormValues(t, tj) + f.Set("actor_token", signActorToken(t, tj, testAgentClientID)) + return f + }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidRequest, + hintContains: "actor_token_type", + }, + { + name: "actor_token_type without actor_token", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + f := defaultFormValues(t, tj) + f.Set("actor_token_type", oauthproto.TokenTypeJWT) + return f + }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidRequest, + hintContains: "actor_token_type", + }, + { + name: "id_token actor_token_type rejected", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + f := defaultFormValues(t, tj) + f.Set("actor_token", signActorToken(t, tj, testAgentClientID)) + f.Set("actor_token_type", oauthproto.TokenTypeIDToken) + return f + }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidRequest, + hintContains: "access_token", + }, + { + name: "id_token subject_token_type accepted", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + token := tj.signToken(t, validClaims(), validExtraClaims()) + return url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {token}, + "subject_token_type": {oauthproto.TokenTypeIDToken}, + } + }, + lifespan: 15 * time.Minute, + check: func(t *testing.T, req *fosite.AccessRequest) { + t.Helper() + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + assert.Equal(t, "user-123", sess.JWTClaims.Subject) + }, + }, { name: "invalid subject_token — bad JWT", ctx: func(_ *testing.T) context.Context { return context.Background() }, From 8afef06235d89bd4fb1ca3994defe8954183abd9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 14 Aug 2026 18:34:41 +0200 Subject: [PATCH 2/9] Remove orphaned token-delegation design-note docs Neither followed docs/arch's numbered-and-indexed convention, neither was linked from docs/arch/README.md or referenced anywhere else. token-delegation-act-chain.md duplicated content already in docs/arch/17-token-exchange-delegation.md (nested provenance, depth cap). token-delegation-actor-id.md was an open design question (should act.sub be a SPIFFE URI) written as a doc file instead of a tracked issue - it belongs in the epic's issue tracker, not shipped as architecture documentation. --- docs/arch/token-delegation-act-chain.md | 35 -------- docs/arch/token-delegation-actor-id.md | 103 ------------------------ 2 files changed, 138 deletions(-) delete mode 100644 docs/arch/token-delegation-act-chain.md delete mode 100644 docs/arch/token-delegation-actor-id.md diff --git a/docs/arch/token-delegation-act-chain.md b/docs/arch/token-delegation-act-chain.md deleted file mode 100644 index 6b68131f13..0000000000 --- a/docs/arch/token-delegation-act-chain.md +++ /dev/null @@ -1,35 +0,0 @@ -# Delegation chain nesting (`act` claim) - -## Status - -Implemented. Token exchange preserves a prior RFC 8693 `act` claim by nesting -it under the newly resolved actor. The handler rejects malformed chains and -limits the resulting chain to ten levels. - -## Behavior - -When a delegated token is re-exchanged, the new actor is prepended to the -existing chain: - -```json -{ - "act": { - "sub": "new-actor", - "act": { - "sub": "prior-actor" - } - } -} -``` - -For an externally issued subject token, the trusted issuer provenance is also -nested before any existing chain. The handler parses the prior chain using the -shared audit parser, rejects malformed content, and caps the final depth to -avoid issuing unbounded tokens. - -## References - -- `pkg/authserver/server/tokenexchange/handler.go` — `buildActClaim` -- `pkg/authserver/server/tokenexchange/handler_test.go` — re-exchange and depth - limit coverage -- RFC 8693 section 4.1 — `act` claim semantics diff --git a/docs/arch/token-delegation-actor-id.md b/docs/arch/token-delegation-actor-id.md deleted file mode 100644 index 0a9294fe12..0000000000 --- a/docs/arch/token-delegation-actor-id.md +++ /dev/null @@ -1,103 +0,0 @@ -# Actor identity shape in the `act` claim - -## Status - -Not implemented. Deferred — this is a cross-cutting identity-model -decision, not a handler-level fix. - -## Problem - -The token exchange handler sets `act.sub` to the raw OAuth client ID -via `client.GetID()` (`pkg/authserver/server/tokenexchange/handler.go`): - -```go -actorID := client.GetID() -... -delegatedSession.JWTClaims.Extra["act"] = map[string]interface{}{ - "sub": actorID, -} -``` - -This produces values like `"devops-agent"` — a plain string client ID. - -However, the Cedar authorization test policies match SPIFFE-style -identities (`pkg/authz/authorizers/cedar/core_test.go`): - -``` -context.claim_act.sub like "spiffe://toolhive.dev/ns/agents/sa/*" -``` - -with test fixtures using: - -```go -"act": map[string]interface{}{ - "sub": "spiffe://toolhive.dev/ns/agents/sa/devops-agent", -}, -``` - -A token issued by this handler would **not match** a Cedar policy -written against the SPIFFE pattern, because `act.sub` is -`"devops-agent"`, not `"spiffe://toolhive.dev/ns/agents/sa/devops-agent"`. - -## Is this a bug? - -No. The Cedar tests construct their own claim fixtures with SPIFFE -URIs directly — they are illustrative of *what policies could look -like* with SPIFFE-style identities, not testing this handler's output. -The handler produces a raw `client_id`, which is consistent with what -fosite uses for client identity throughout. - -## The design question - -Should the handler transform `client.GetID()` into a SPIFFE URI before -placing it in the `act` claim? This depends on: - -1. **Agent identity model**: does toolhive's agent identity use SPIFFE - URIs? The workload identity system does (`pkg/auth/identity.go` - references SPIFFE), but the OAuth client registry stores plain - string client IDs. - -2. **Downstream consumers**: do Cedar policies, audit logs, and other - consumers expect SPIFFE URIs or raw client IDs? The Cedar test - policies suggest SPIFFE; the handler produces raw client IDs. - -3. **Cross-cutting concern**: if SPIFFE URIs are the right identifier - space, the transformation should happen at a shared layer (e.g., a - client-to-SPIFFE resolver), not hardcoded in the token exchange - handler. Other handlers that emit client identity (e.g., the - standard token handler's `client_id` claim) would need the same - transformation. - -## Options - -- **Option A (keep raw client_id)**: the handler emits `client.GetID()` - as-is. Cedar policies must match against plain client IDs. Simplest, - consistent with fosite, but doesn't align with the SPIFFE-based - workload identity model. - -- **Option B (transform to SPIFFE)**: the handler resolves the client - ID to a SPIFFE URI before placing it in `act.sub`. Requires a - client-to-SPIFFE resolver or a convention (e.g., - `spiffe://toolhive.dev/ns/agents/sa/`). Aligns with the - Cedar test policies and the workload identity model, but adds a - transformation step that other handlers would also need. - -- **Option C (store SPIFFE URI in client registry)**: the OAuth client - registration carries a SPIFFE URI alongside the client ID. The - handler uses the SPIFFE URI if present, falling back to `client_id`. - Most flexible, but requires changes to client registration. - -## Recommendation - -Defer until the agent identity model is finalized. The current -behavior (raw `client_id`) is correct for the OAuth layer and doesn't -block any functionality — it's a mismatch between test fixtures and -handler output, not a runtime bug. When the identity model decision is -made, apply it as a cross-cutting concern, not a handler-specific fix. - -## References - -- `pkg/authserver/server/tokenexchange/handler.go` — `actorID` assignment and `act` claim -- `pkg/authz/authorizers/cedar/core_test.go` — Cedar policies with SPIFFE URIs -- `pkg/authz/authorizers/cedar/entity.go` — Cedar value conversion for `act` claim -- `pkg/auth/identity.go` — SPIFFE references in workload identity From 741532079ffa1a5dad117fef944cfee6cb6f10bc Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 14 Aug 2026 22:07:19 +0200 Subject: [PATCH 3/9] Add real kind e2e coverage for actor_token exchange Extends the delegate-client e2e suite with a live HTTP round trip against the deployed pod: a matching self-issued actor_token still resolves to the delegate client as the recorded actor, and a mismatched actor_token is rejected with a real 400 from the running server, not just in unit tests. --- .../virtualmcp_delegate_clients_test.go | 102 ++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go index 17d88ebaad..2bfafebb2e 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go @@ -50,6 +50,7 @@ var _ = ginkgo.Describe("VirtualMCPServer delegate clients", ginkgo.Ordered, fun dexCleanup func() issuer string signingPublicKey *rsa.PublicKey + signingPrivateKey *rsa.PrivateKey ) ginkgo.BeforeAll(func() { @@ -73,6 +74,7 @@ var _ = ginkgo.Describe("VirtualMCPServer delegate clients", ginkgo.Ordered, fun privateKey, err := rsa.GenerateKey(rand.Reader, 2048) gomega.Expect(err).NotTo(gomega.HaveOccurred()) signingPublicKey = &privateKey.PublicKey + signingPrivateKey = privateKey gomega.Expect(k8sClient.Create(ctx, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{Name: signingKeySecretName, Namespace: defaultNamespace}, Data: map[string][]byte{"private-key": pem.EncodeToMemory(&pem.Block{ @@ -215,6 +217,51 @@ var _ = ginkgo.Describe("VirtualMCPServer delegate clients", ginkgo.Ordered, fun defer response.Body.Close() gomega.Expect(response.StatusCode).To(gomega.Equal(http.StatusUnauthorized)) }) + + ginkgo.It("resolves actor identity from a self-issued actor_token against the real pod", func() { + port, cleanup, err := startRateLimitServicePortForward("vmcp-"+vmcpName, 4483) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + defer cleanup() + localURL := fmt.Sprintf("http://localhost:%d", port) + + subjectToken, err := getEmbeddedASToken( + localURL, + dexInfo.LocalURL, + fmt.Sprintf("%s.%s.svc.cluster.local:5556", dexName, defaultNamespace), + vmcpHost, + issuer, + ) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + ginkgo.By("exchanging with an actor_token whose sub matches the delegate client") + actorToken := signSelfIssuedActorToken(signingPrivateKey, issuer, clientID) + exchanged := exchangeDelegateTokenWithActor(localURL, subjectToken, actorToken, issuer, clientSecret) + claims := verifiedJWTClaims(exchanged, signingPublicKey) + act, ok := claims["act"].(map[string]any) + gomega.Expect(ok).To(gomega.BeTrue()) + // actor_token only proves possession; it must not change the recorded + // actor identity, which stays the authenticated delegate client. + gomega.Expect(act["sub"]).To(gomega.Equal(clientID)) + + ginkgo.By("rejecting an actor_token whose sub does not match the authenticated delegate client") + mismatchedActorToken := signSelfIssuedActorToken(signingPrivateKey, issuer, "someone-else") + form := url.Values{ + "grant_type": {"urn:ietf:params:oauth:grant-type:token-exchange"}, + "subject_token": {subjectToken}, + "subject_token_type": {"urn:ietf:params:oauth:token-type:jwt"}, + "actor_token": {mismatchedActorToken}, + "actor_token_type": {"urn:ietf:params:oauth:token-type:jwt"}, + "audience": {issuer}, + } + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, localURL+"/oauth/token", strings.NewReader(form.Encode())) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth(clientID, clientSecret) + response, err := http.DefaultClient.Do(req) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + defer response.Body.Close() + gomega.Expect(response.StatusCode).To(gomega.Equal(http.StatusBadRequest)) + }) }) func exchangeDelegateToken(endpoint, subjectToken, audience, secret string) string { @@ -239,6 +286,61 @@ func exchangeDelegateToken(endpoint, subjectToken, audience, secret string) stri return token.AccessToken } +// exchangeDelegateTokenWithActor is exchangeDelegateToken extended with an +// RFC 8693 actor_token/actor_token_type pair. +func exchangeDelegateTokenWithActor(endpoint, subjectToken, actorToken, audience, secret string) string { + form := url.Values{ + "grant_type": {"urn:ietf:params:oauth:grant-type:token-exchange"}, + "subject_token": {subjectToken}, + "subject_token_type": {"urn:ietf:params:oauth:token-type:jwt"}, + "actor_token": {actorToken}, + "actor_token_type": {"urn:ietf:params:oauth:token-type:jwt"}, + "audience": {audience}, + } + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, endpoint+"/oauth/token", strings.NewReader(form.Encode())) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth("e2e-delegate-client", secret) + response, err := http.DefaultClient.Do(req) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + defer response.Body.Close() + if response.StatusCode != http.StatusOK { + body, readErr := io.ReadAll(response.Body) + gomega.Expect(readErr).NotTo(gomega.HaveOccurred()) + gomega.Expect(response.StatusCode).To(gomega.Equal(http.StatusOK), string(body)) + } + var token struct { + AccessToken string `json:"access_token"` + } + gomega.Expect(json.NewDecoder(response.Body).Decode(&token)).To(gomega.Succeed()) + gomega.Expect(token.AccessToken).NotTo(gomega.BeEmpty()) + return token.AccessToken +} + +// signSelfIssuedActorToken mints a self-issued RFC 8693 actor_token signed +// with the embedded auth server's own signing key, so it validates against +// the server's own JWKS — resolveActorIdentity +// (pkg/authserver/server/tokenexchange/handler.go) only accepts a +// self-issued actor_token, never an externally-issued one. +func signSelfIssuedActorToken(privateKey *rsa.PrivateKey, issuer, sub string) string { + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: jose.RS256, Key: privateKey}, + (&jose.SignerOptions{}).WithType("JWT"), + ) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + now := time.Now() + token, err := jwt.Signed(signer).Claims(jwt.Claims{ + Issuer: issuer, + Subject: sub, + Audience: jwt.Audience{issuer}, + Expiry: jwt.NewNumericDate(now.Add(time.Hour)), + IssuedAt: jwt.NewNumericDate(now), + }).Serialize() + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + return token +} + func verifiedJWTClaims(token string, signingKey *rsa.PublicKey) map[string]any { parsed, err := jwt.ParseSigned(token, []jose.SignatureAlgorithm{jose.RS256}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) From 6b68f9ec418dc125c0ef65fe43d34dd8db1d8a63 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 17 Aug 2026 10:25:36 +0200 Subject: [PATCH 4/9] Bind actor_token to client_id, not sub, in token exchange resolveActorIdentity required an actor_token's "sub" to equal the authenticated client ID, making the token's own client_id claim useless and collapsing actor_token to a no-op self-check that could never assert an actor distinct from the OAuth client. Bind on the actor_token's "client_id" claim instead (the same proof-of-possession role a normal token's client_id plays) and let "sub" flow through as the actor identity, so actor_token can name a delegate persona distinct from the authenticated client while still proving it was minted for that client. --- ...delegate_client_runner_integration_test.go | 63 +++++++++++-------- .../server/tokenexchange/handler.go | 41 +++++++----- .../server/tokenexchange/handler_test.go | 60 ++++++++++++++++-- .../virtualmcp_delegate_clients_test.go | 16 +++-- 4 files changed, 126 insertions(+), 54 deletions(-) diff --git a/pkg/authserver/delegate_client_runner_integration_test.go b/pkg/authserver/delegate_client_runner_integration_test.go index 20b88dec0b..213c543215 100644 --- a/pkg/authserver/delegate_client_runner_integration_test.go +++ b/pkg/authserver/delegate_client_runner_integration_test.go @@ -179,29 +179,32 @@ func TestConfiguredDelegateClientTokenExchange_WithActorToken(t *testing.T) { const originalClientID = "original-non-delegate-client" tests := []struct { - name string - subjectClientID string // client_id claim baked into the subject token - actorTokenSub string // sub claim of the actor_token - wantStatus int - wantError string + name string + subjectClientID string // client_id claim baked into the subject token + actorTokenClient string // client_id claim of the actor_token (binding check) + actorTokenSub string // sub claim of the actor_token (asserted actor identity) + wantStatus int + wantError string }{ { - // resolveActorIdentity's sub-mismatch branch deliberately returns - // invalid_grant, not invalid_request — a distinct "wrong party" - // error class, the same convention checkDelegationConsent uses - // for its own client_id-mismatch case elsewhere in this file. + // resolveActorIdentity's client_id-mismatch branch deliberately + // returns invalid_grant, not invalid_request — a distinct "wrong + // party" error class, the same convention checkDelegationConsent + // uses for its own client_id-mismatch case elsewhere in this file. // invalid_request is reserved for a malformed/unverifiable token. - name: "mismatched actor_token rejected before delegation consent is reached", - subjectClientID: originalClientID, - actorTokenSub: "someone-else", - wantStatus: http.StatusBadRequest, - wantError: "invalid_grant", + name: "mismatched actor_token rejected before delegation consent is reached", + subjectClientID: originalClientID, + actorTokenClient: "someone-elses-client", + actorTokenSub: "someone-else", + wantStatus: http.StatusBadRequest, + wantError: "invalid_grant", }, { - name: "matching actor_token still succeeds via the delegate-client relaxation", - subjectClientID: originalClientID, - actorTokenSub: delegateClientID, - wantStatus: http.StatusOK, + name: "matching actor_token still succeeds via the delegate-client relaxation", + subjectClientID: originalClientID, + actorTokenClient: delegateClientID, + actorTokenSub: delegateClientID, + wantStatus: http.StatusOK, }, { // Delegate status must not change behavior when the relaxation isn't @@ -209,10 +212,11 @@ func TestConfiguredDelegateClientTokenExchange_WithActorToken(t *testing.T) { // authenticated client, so this succeeds on ordinary client_id // binding, and verifyDelegatedToken's act.sub assertion below proves // it produces the identical act shape either way. - name: "actor_token present but relaxation unneeded still succeeds", - subjectClientID: delegateClientID, - actorTokenSub: delegateClientID, - wantStatus: http.StatusOK, + name: "actor_token present but relaxation unneeded still succeeds", + subjectClientID: delegateClientID, + actorTokenClient: delegateClientID, + actorTokenSub: delegateClientID, + wantStatus: http.StatusOK, }, } @@ -222,7 +226,7 @@ func TestConfiguredDelegateClientTokenExchange_WithActorToken(t *testing.T) { server, issuer, embedded := startConfiguredDelegateAuthServer(t) subjectToken := signedSubjectTokenForClient(t, embedded, issuer, tt.subjectClientID) - actorToken := signedActorToken(t, embedded, issuer, tt.actorTokenSub) + actorToken := signedActorToken(t, embedded, issuer, tt.actorTokenClient, tt.actorTokenSub) values := url.Values{ "grant_type": {tokenExchangeGrantType}, @@ -376,13 +380,16 @@ func signedSubjectTokenForClient( } // signedActorToken mints a self-issued RFC 8693 actor_token with the given -// "sub" claim. resolveActorIdentity requires this to equal the authenticated -// client's ID or the exchange is rejected before delegation consent is ever -// consulted — see TestConfiguredDelegateClientTokenExchange_WithActorToken. +// "client_id" and "sub" claims. resolveActorIdentity requires "client_id" to +// equal the authenticated client's ID (the binding/proof-of-possession +// check) or the exchange is rejected before delegation consent is ever +// consulted; "sub" is the asserted actor identity, which flows into the +// delegated token's act.sub and may legitimately differ from "client_id" — +// see TestConfiguredDelegateClientTokenExchange_WithActorToken. func signedActorToken( t *testing.T, embedded *authserverrunner.EmbeddedAuthServer, - issuer, sub string, + issuer, tokenClientID, sub string, ) string { t.Helper() @@ -401,6 +408,8 @@ func signedActorToken( Audience: jwt.Audience{delegateAudience}, Expiry: jwt.NewNumericDate(now.Add(time.Hour)), IssuedAt: jwt.NewNumericDate(now), + }).Claims(map[string]any{ + "client_id": tokenClientID, }).Serialize() require.NoError(t, err) return token diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index a8657aaa0e..09faa0d14b 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -266,18 +266,25 @@ func (h *Handler) PopulateTokenEndpointResponse( return nil } -// resolveActorIdentity determines the acting party identity: always the -// authenticated OAuth client ID. +// resolveActorIdentity determines the acting party identity: either the +// authenticated OAuth client ID, or — when actor_token is present — the +// distinct actor identity it asserts. // -// This is actor-token *confirmation* (proof-of-possession hardening), not -// RFC 8693's general actor-delegation use case. When actor_token is present, -// it is validated against the AS's own JWKS and its "sub" is required to -// equal client.GetID() — so the resulting identity is identical whether or -// not actor_token was supplied at all. Presenting actor_token only proves the -// caller additionally holds a self-issued JWT for its own client_id; it never -// lets a distinct actor identity flow into the act claim. Do not repurpose -// this equality check to record a different actor identity without -// revisiting the callers that assume act.sub == the authenticated client ID. +// actor_token lets the authenticated client name a more specific actor than +// its own client_id (e.g. a particular agent instance or delegate persona) +// the same way a normal issued token records its client identity: via a +// "client_id" claim. The actor_token's own "client_id" claim MUST match the +// authenticated client ID — this is the binding/proof-of-possession check, +// proving the token was minted for this very client — while its "sub" claim +// is the actor identity that is returned here and flows into the delegated +// token's act.sub the same place a normal client's identity would go. "sub" +// is deliberately not compared to client.GetID(): requiring equality there +// would make actor_token unable to ever assert an identity different from +// the OAuth client, collapsing it to a no-op self-check. Note that this does +// not by itself grant the asserted actor any extra privilege: the resulting +// actor identity still has to satisfy checkDelegationConsent (may_act, +// ExternalActor, client_id binding, or the configured-delegate exemption) +// like any other actor identity would. func (h *Handler) resolveActorIdentity( ctx context.Context, params *formParams, client fosite.Client, ) (string, error) { @@ -289,13 +296,13 @@ func (h *Handler) resolveActorIdentity( return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( "The actor token is invalid or could not be verified.")) } - // Binding check: actor_token.sub MUST match the authenticated client ID. - // This prevents replay attacks where a leaked actor token is presented - // by a different client. The client ID is always verified by fosite's - // client authentication before reaching here. - if actorClaims.Subject != client.GetID() { + // Binding check: actor_token's client_id claim MUST match the + // authenticated client ID. This prevents replay attacks where a leaked + // actor token is presented by a different client. The client ID is + // always verified by fosite's client authentication before reaching here. + if actorClaims.ClientID != client.GetID() { return "", errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( - "The actor token subject does not match the authenticated client identity.")) + "The actor token's client_id claim does not match the authenticated client identity.")) } return actorClaims.Subject, nil } diff --git a/pkg/authserver/server/tokenexchange/handler_test.go b/pkg/authserver/server/tokenexchange/handler_test.go index 699320ef4d..b4554975fa 100644 --- a/pkg/authserver/server/tokenexchange/handler_test.go +++ b/pkg/authserver/server/tokenexchange/handler_test.go @@ -106,9 +106,19 @@ func nestedActChain(depth int) map[string]any { } // signActorToken creates a self-issued JWT suitable for use as an actor_token. -// The sub claim is set to the given subject (typically the client_id). +// The sub claim is set to the given subject (the asserted actor identity), +// and the "client_id" claim is set to testAgentClientID (the authenticated +// client the token is bound to). func signActorToken(t *testing.T, tj *testJWKS, subject string) string { t.Helper() + return signActorTokenForClient(t, tj, subject, testAgentClientID) +} + +// signActorTokenForClient is signActorToken with an explicit "client_id" +// claim, for exercising the binding check against a client other than +// testAgentClientID. +func signActorTokenForClient(t *testing.T, tj *testJWKS, subject, clientID string) string { + t.Helper() now := time.Now() claims := jwt.Claims{ @@ -119,7 +129,7 @@ func signActorToken(t *testing.T, tj *testJWKS, subject string) string { IssuedAt: jwt.NewNumericDate(now), } return tj.signToken(t, claims, map[string]any{ - "client_id": testAgentClientID, + "client_id": clientID, }) } @@ -306,17 +316,59 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { }, }, { - name: "actor_token sub mismatch with client ID", + name: "actor_token asserts a distinct actor identity", ctx: func(_ *testing.T) context.Context { return context.Background() }, client: defaultClient, lifespan: 15 * time.Minute, form: func(t *testing.T) url.Values { t.Helper() - f := defaultFormValues(t, tj) + // The subject token's own client_id claim names "other-agent" + // so checkDelegationConsent's client_id fallback binds it to + // that actor (the same way it would bind to testAgentClientID + // in the no-actor_token case). + extra := validExtraClaims() + extra["client_id"] = "other-agent" + subjectToken := tj.signToken(t, validClaims(), extra) + f := url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + } + // client_id claim still matches the authenticated client, but + // sub names a distinct actor identity — this must be honored, + // not rejected, since the client_id claim is the binding check. f.Set("actor_token", signActorToken(t, tj, "other-agent")) f.Set("actor_token_type", oauthproto.TokenTypeJWT) return f }, + check: func(t *testing.T, req *fosite.AccessRequest) { + t.Helper() + + sess, ok := req.GetSession().(*session.Session) + require.True(t, ok, "session should be *session.Session") + + actClaim, exists := sess.JWTClaims.Extra["act"] + require.True(t, exists, "act claim must be present") + actMap, ok := actClaim.(map[string]interface{}) + require.True(t, ok, "act claim must be a map") + assert.Equal(t, "other-agent", actMap["sub"]) + }, + }, + { + name: "actor_token client_id mismatch with authenticated client", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + lifespan: 15 * time.Minute, + form: func(t *testing.T) url.Values { + t.Helper() + f := defaultFormValues(t, tj) + // The actor_token's client_id claim names a different client + // than the one authenticating this request, so the binding + // check must reject it regardless of the asserted actor sub. + f.Set("actor_token", signActorTokenForClient(t, tj, "some-actor", "a-different-client")) + f.Set("actor_token_type", oauthproto.TokenTypeJWT) + return f + }, wantErr: true, wantFositeIs: fosite.ErrInvalidGrant, hintContains: "does not match the authenticated client identity", diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go index 2bfafebb2e..c4ad1669b8 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go @@ -234,17 +234,15 @@ var _ = ginkgo.Describe("VirtualMCPServer delegate clients", ginkgo.Ordered, fun gomega.Expect(err).NotTo(gomega.HaveOccurred()) ginkgo.By("exchanging with an actor_token whose sub matches the delegate client") - actorToken := signSelfIssuedActorToken(signingPrivateKey, issuer, clientID) + actorToken := signSelfIssuedActorToken(signingPrivateKey, issuer, clientID, clientID) exchanged := exchangeDelegateTokenWithActor(localURL, subjectToken, actorToken, issuer, clientSecret) claims := verifiedJWTClaims(exchanged, signingPublicKey) act, ok := claims["act"].(map[string]any) gomega.Expect(ok).To(gomega.BeTrue()) - // actor_token only proves possession; it must not change the recorded - // actor identity, which stays the authenticated delegate client. gomega.Expect(act["sub"]).To(gomega.Equal(clientID)) - ginkgo.By("rejecting an actor_token whose sub does not match the authenticated delegate client") - mismatchedActorToken := signSelfIssuedActorToken(signingPrivateKey, issuer, "someone-else") + ginkgo.By("rejecting an actor_token whose client_id claim does not match the authenticated delegate client") + mismatchedActorToken := signSelfIssuedActorToken(signingPrivateKey, issuer, "someone-elses-client", "someone-else") form := url.Values{ "grant_type": {"urn:ietf:params:oauth:grant-type:token-exchange"}, "subject_token": {subjectToken}, @@ -322,7 +320,11 @@ func exchangeDelegateTokenWithActor(endpoint, subjectToken, actorToken, audience // the server's own JWKS — resolveActorIdentity // (pkg/authserver/server/tokenexchange/handler.go) only accepts a // self-issued actor_token, never an externally-issued one. -func signSelfIssuedActorToken(privateKey *rsa.PrivateKey, issuer, sub string) string { +// +// tokenClientID is the "client_id" claim, which resolveActorIdentity binds +// against the authenticated OAuth client ID; sub is the asserted actor +// identity, which may legitimately differ from tokenClientID. +func signSelfIssuedActorToken(privateKey *rsa.PrivateKey, issuer, tokenClientID, sub string) string { signer, err := jose.NewSigner( jose.SigningKey{Algorithm: jose.RS256, Key: privateKey}, (&jose.SignerOptions{}).WithType("JWT"), @@ -336,6 +338,8 @@ func signSelfIssuedActorToken(privateKey *rsa.PrivateKey, issuer, sub string) st Audience: jwt.Audience{issuer}, Expiry: jwt.NewNumericDate(now.Add(time.Hour)), IssuedAt: jwt.NewNumericDate(now), + }).Claims(map[string]any{ + "client_id": tokenClientID, }).Serialize() gomega.Expect(err).NotTo(gomega.HaveOccurred()) return token From 458be5a7ef402f03ba42719c2ca9d05c07a7862d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 17 Aug 2026 10:26:39 +0200 Subject: [PATCH 5/9] Reject id_token as a token-exchange subject_token_type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit subject_token_type=id_token was accepted but validated with the exact same access-token profile as any other subject token, silently ignoring the semantic differences RFC 8693 assigns to id_token (e.g. an ID token's aud names the relying-party client, not a resource). Rather than validate an ID token as if it were an access token, decline the type until a real ID-token validation profile exists — matching the existing actor_token_type restriction. --- pkg/authserver/server/tokenexchange/handler.go | 13 ++++++++++--- .../server/tokenexchange/handler_test.go | 14 +++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index 09faa0d14b..bf001e1bcb 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -326,13 +326,20 @@ func validateFormParams(form url.Values) (*formParams, error) { "The 'subject_token_type' parameter is required for token exchange.")) } + // id_token is intentionally excluded here, the same as for actor_token + // tokens below: an ID token's claim conventions differ from an access + // token's (e.g. "aud" names the relying-party client, not a resource) and + // this validator applies neither a distinct validation profile nor an + // id_token-specific claim mapping — accepting the type would let a client + // declare "id_token" while the token is validated exactly like an access + // token, silently ignoring the declared type's semantics. switch subjectTokenType { - case oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT, oauthproto.TokenTypeIDToken: + case oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT: // Valid subject token types. default: return nil, errorsx.WithStack(fosite.ErrInvalidRequest.WithHintf( - "The 'subject_token_type' value %q is not supported. Use %q, %q, or %q.", - subjectTokenType, oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT, oauthproto.TokenTypeIDToken)) + "The 'subject_token_type' value %q is not supported. Use %q or %q.", + subjectTokenType, oauthproto.TokenTypeAccessToken, oauthproto.TokenTypeJWT)) } actorToken := form.Get("actor_token") diff --git a/pkg/authserver/server/tokenexchange/handler_test.go b/pkg/authserver/server/tokenexchange/handler_test.go index b4554975fa..eeba9ca218 100644 --- a/pkg/authserver/server/tokenexchange/handler_test.go +++ b/pkg/authserver/server/tokenexchange/handler_test.go @@ -436,7 +436,7 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { hintContains: "access_token", }, { - name: "id_token subject_token_type accepted", + name: "id_token subject_token_type rejected", ctx: func(_ *testing.T) context.Context { return context.Background() }, client: defaultClient, form: func(t *testing.T) url.Values { @@ -448,14 +448,10 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { "subject_token_type": {oauthproto.TokenTypeIDToken}, } }, - lifespan: 15 * time.Minute, - check: func(t *testing.T, req *fosite.AccessRequest) { - t.Helper() - - sess, ok := req.GetSession().(*session.Session) - require.True(t, ok, "session should be *session.Session") - assert.Equal(t, "user-123", sess.JWTClaims.Subject) - }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidRequest, + hintContains: "subject_token_type", }, { name: "invalid subject_token — bad JWT", From 55a94c3e44b983885ccf86411df3f04640c70930 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 17 Aug 2026 10:26:54 +0200 Subject: [PATCH 6/9] Document actor_token/id_token behavior in token exchange arch doc The token-exchange delegation doc never covered actor_token at all, and didn't explain why id_token is rejected as a subject/actor token type. Document the client_id/sub split resolveActorIdentity now enforces and the rationale for declining id_token until a real validation profile exists. --- docs/arch/17-token-exchange-delegation.md | 36 +++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/docs/arch/17-token-exchange-delegation.md b/docs/arch/17-token-exchange-delegation.md index d3386aeb6d..e9fa6e12b9 100644 --- a/docs/arch/17-token-exchange-delegation.md +++ b/docs/arch/17-token-exchange-delegation.md @@ -200,6 +200,42 @@ whether to grant the exchange, in this order: 4. **No binding.** If none of the above apply, the token carries no verifiable client binding and the exchange is rejected. +## `actor_token` and `id_token` + +RFC 8693 also defines an optional `actor_token`/`actor_token_type` pair, +which `resolveActorIdentity` (`handler.go`) supports for self-issued actor +tokens only — an `actor_token` validated against any other issuer is +rejected. Two claims on the `actor_token` matter, and they play different +roles: + +- **`client_id`** is the binding/proof-of-possession check: it must equal the + authenticated OAuth client ID (the client fosite already authenticated for + this request). This is the same role a `client_id` claim plays on a normal + issued token — it says which client the token was minted for. +- **`sub`** is the actor identity that flows into the delegated token's + `act.sub`, in the same slot the authenticated client ID would otherwise + occupy. It is not required to equal the client ID: this is what lets a + single OAuth client present a more specific actor identity (e.g. a + particular agent instance or delegate persona) than its own `client_id`. + +Presenting a distinct `sub` this way does not itself grant any extra +privilege — the resulting actor identity still has to clear +`checkDelegationConsent` above like any other actor identity would (via +`may_act`, `ExternalActor`, the `client_id` binding, or the configured- +delegate exemption). + +`actor_token_type` only accepts `access_token` or `jwt`; `id_token` is +rejected outright. Similarly, `subject_token_type` accepts `access_token` or +`jwt`, but not `id_token`: this server applies exactly one validation profile +to a subject or actor token (the self-issued/access-token one — see +`rejectIDTokenClaims`'s doc comment in `validator.go`), so accepting a +declared `subject_token_type`/`actor_token_type` of `id_token` without a +distinct validation profile behind it would silently ignore the semantic +differences between an ID token and an access token (e.g. an ID token's +`aud` names the relying-party client, not a resource). Rather than validate +an ID token as if it were an access token, the server declines the type +until a real ID-token validation profile is implemented. + ### Delegate clients and self-issued token exchange A configured delegate client can convert *any* self-issued ToolHive access From e5386646713ef80cc14200b55a0e73b992d11c75 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 17 Aug 2026 15:30:19 +0200 Subject: [PATCH 7/9] Address token exchange review feedback --- docs/arch/17-token-exchange-delegation.md | 5 +- ...delegate_client_runner_integration_test.go | 24 +++++--- .../server/tokenexchange/handler.go | 18 +++--- .../server/tokenexchange/handler_test.go | 2 +- pkg/oauthproto/grants.go | 23 +++----- pkg/oauthproto/grants_test.go | 59 +++++++------------ 6 files changed, 59 insertions(+), 72 deletions(-) diff --git a/docs/arch/17-token-exchange-delegation.md b/docs/arch/17-token-exchange-delegation.md index e9fa6e12b9..05802a998f 100644 --- a/docs/arch/17-token-exchange-delegation.md +++ b/docs/arch/17-token-exchange-delegation.md @@ -208,7 +208,7 @@ tokens only — an `actor_token` validated against any other issuer is rejected. Two claims on the `actor_token` matter, and they play different roles: -- **`client_id`** is the binding/proof-of-possession check: it must equal the +- **`client_id`** is the authenticated-client binding: it must equal the authenticated OAuth client ID (the client fosite already authenticated for this request). This is the same role a `client_id` claim plays on a normal issued token — it says which client the token was minted for. @@ -226,7 +226,8 @@ delegate exemption). `actor_token_type` only accepts `access_token` or `jwt`; `id_token` is rejected outright. Similarly, `subject_token_type` accepts `access_token` or -`jwt`, but not `id_token`: this server applies exactly one validation profile +`jwt`, but not `id_token`: this embedded authorization-server endpoint does +not implement the XAA/ID-JAG profile and applies exactly one validation profile to a subject or actor token (the self-issued/access-token one — see `rejectIDTokenClaims`'s doc comment in `validator.go`), so accepting a declared `subject_token_type`/`actor_token_type` of `id_token` without a diff --git a/pkg/authserver/delegate_client_runner_integration_test.go b/pkg/authserver/delegate_client_runner_integration_test.go index 213c543215..8b73d88278 100644 --- a/pkg/authserver/delegate_client_runner_integration_test.go +++ b/pkg/authserver/delegate_client_runner_integration_test.go @@ -103,6 +103,15 @@ func TestConfiguredDelegateClientTokenExchange(t *testing.T) { wantStatus: http.StatusBadRequest, wantError: "invalid_request", }, + { + name: "id_token_subject_token_type_rejected", + authenticate: func(_ *http.Request) {}, + mutateRequest: func(values url.Values) { + values.Set("subject_token_type", oauthproto.TokenTypeIDToken) + }, + wantStatus: http.StatusBadRequest, + wantError: "invalid_request", + }, } t.Run("discovery advertises token exchange and client secret methods", func(t *testing.T) { @@ -187,17 +196,14 @@ func TestConfiguredDelegateClientTokenExchange_WithActorToken(t *testing.T) { wantError string }{ { - // resolveActorIdentity's client_id-mismatch branch deliberately - // returns invalid_grant, not invalid_request — a distinct "wrong - // party" error class, the same convention checkDelegationConsent - // uses for its own client_id-mismatch case elsewhere in this file. - // invalid_request is reserved for a malformed/unverifiable token. + // RFC 8693 §2.2.2 requires invalid_request when an actor token is + // unacceptable based on policy, including a client-ID binding mismatch. name: "mismatched actor_token rejected before delegation consent is reached", subjectClientID: originalClientID, actorTokenClient: "someone-elses-client", actorTokenSub: "someone-else", wantStatus: http.StatusBadRequest, - wantError: "invalid_grant", + wantError: "invalid_request", }, { name: "matching actor_token still succeeds via the delegate-client relaxation", @@ -381,9 +387,9 @@ func signedSubjectTokenForClient( // signedActorToken mints a self-issued RFC 8693 actor_token with the given // "client_id" and "sub" claims. resolveActorIdentity requires "client_id" to -// equal the authenticated client's ID (the binding/proof-of-possession -// check) or the exchange is rejected before delegation consent is ever -// consulted; "sub" is the asserted actor identity, which flows into the +// equal the authenticated client's ID (the client-ID binding check) or the +// exchange is rejected before delegation consent is ever consulted; "sub" is +// the asserted actor identity, which flows into the // delegated token's act.sub and may legitimately differ from "client_id" — // see TestConfiguredDelegateClientTokenExchange_WithActorToken. func signedActorToken( diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index bf001e1bcb..2c44f538e9 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -274,12 +274,12 @@ func (h *Handler) PopulateTokenEndpointResponse( // its own client_id (e.g. a particular agent instance or delegate persona) // the same way a normal issued token records its client identity: via a // "client_id" claim. The actor_token's own "client_id" claim MUST match the -// authenticated client ID — this is the binding/proof-of-possession check, -// proving the token was minted for this very client — while its "sub" claim -// is the actor identity that is returned here and flows into the delegated -// token's act.sub the same place a normal client's identity would go. "sub" -// is deliberately not compared to client.GetID(): requiring equality there -// would make actor_token unable to ever assert an identity different from +// authenticated client ID — this is the client-ID binding check, proving the +// token was minted for this client — while its "sub" claim is the actor +// identity that is returned here and flows into the delegated token's act.sub +// the same place a normal client's identity would go. "sub" is deliberately +// not compared to client.GetID(): requiring equality there would make +// actor_token unable to ever assert an identity different from // the OAuth client, collapsing it to a no-op self-check. Note that this does // not by itself grant the asserted actor any extra privilege: the resulting // actor identity still has to satisfy checkDelegationConsent (may_act, @@ -296,12 +296,14 @@ func (h *Handler) resolveActorIdentity( return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( "The actor token is invalid or could not be verified.")) } - // Binding check: actor_token's client_id claim MUST match the + // Client-ID binding: actor_token's client_id claim MUST match the // authenticated client ID. This prevents replay attacks where a leaked // actor token is presented by a different client. The client ID is // always verified by fosite's client authentication before reaching here. + // RFC 8693 §2.2.2 requires invalid_request when an actor token is + // unacceptable based on policy. if actorClaims.ClientID != client.GetID() { - return "", errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( + return "", errorsx.WithStack(fosite.ErrInvalidRequest.WithHint( "The actor token's client_id claim does not match the authenticated client identity.")) } return actorClaims.Subject, nil diff --git a/pkg/authserver/server/tokenexchange/handler_test.go b/pkg/authserver/server/tokenexchange/handler_test.go index eeba9ca218..bdb0d95204 100644 --- a/pkg/authserver/server/tokenexchange/handler_test.go +++ b/pkg/authserver/server/tokenexchange/handler_test.go @@ -370,7 +370,7 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { return f }, wantErr: true, - wantFositeIs: fosite.ErrInvalidGrant, + wantFositeIs: fosite.ErrInvalidRequest, hintContains: "does not match the authenticated client identity", }, { diff --git a/pkg/oauthproto/grants.go b/pkg/oauthproto/grants.go index 376baabb38..d650c9f2a3 100644 --- a/pkg/oauthproto/grants.go +++ b/pkg/oauthproto/grants.go @@ -165,15 +165,13 @@ func NewFormRequest( // // - If client is nil, DefaultHTTPClient is used so callers automatically // get the shared transport (connection reuse, consistent timeouts). -// - The response body is read with io.LimitReader capped at +// - The response body is initially read with io.LimitReader capped at // maxResponseBodySize (1 MiB, matching x/oauth2) before any parsing, so -// a pathological server cannot exhaust memory. +// a pathological server cannot exhaust memory. Before closing, at most one +// additional maxResponseBodySize is drained to enable connection reuse +// without unbounded reads from oversized or never-ending bodies. // - On every exit path — success, JSON decode failure, and RetrieveError -// — the body is closed. The body is deliberately NOT drained: -// io.Copy(io.Discard, resp.Body) would be unbounded on oversized or -// never-terminating bodies and would defeat the 1 MiB cap above. When -// the body exceeds the cap, net/http cannot reuse the connection; that -// is the intended tradeoff and matches x/oauth2/internal/token.go. +// — the body is bounded-drained and closed. // - RFC 6749 Section 5.2 routing (a 2xx body with an "error" field) is // handled inside ParseTokenResponse; DoTokenRequest surfaces the // resulting *oauth2.RetrieveError unchanged. @@ -192,14 +190,9 @@ func DoTokenRequest(client *http.Client, req *http.Request) (*TokenResponse, err return nil, fmt.Errorf("oauth: token request failed: %w", err) } defer func() { - // Close without draining. Matching x/oauth2/internal/token.go — the - // LimitReader below caps how much we read, and draining the remainder - // via io.Copy(io.Discard, resp.Body) would be unbounded on oversized - // or never-terminating bodies, which defeats the 1 MiB memory cap. - // The tradeoff: when the body exceeds maxResponseBodySize, net/http - // cannot reuse the underlying connection. That is acceptable — the - // response is already pathological and connection reuse is not worth - // unbounded reads. + // A bounded drain permits connection reuse for ordinary response bodies + // without indefinitely reading oversized or never-ending IdP responses. + _, _ = io.CopyN(io.Discard, resp.Body, maxResponseBodySize) if closeErr := resp.Body.Close(); closeErr != nil { slog.Debug("oauth: close token response body", "error", closeErr) } diff --git a/pkg/oauthproto/grants_test.go b/pkg/oauthproto/grants_test.go index b44c8e7447..5e8b036619 100644 --- a/pkg/oauthproto/grants_test.go +++ b/pkg/oauthproto/grants_test.go @@ -586,9 +586,8 @@ func TestDoTokenRequest_ContextCancellation(t *testing.T) { // trackingBody wraps an io.Reader and records whether the body was read // and closed, plus the total number of bytes Read was allowed to consume. -// DoTokenRequest reads through a LimitReader capped at maxResponseBodySize -// and then closes without draining; bytesRead lets tests assert the cap -// is honored even when the underlying body is much larger. +// DoTokenRequest initially reads through a LimitReader capped at +// maxResponseBodySize, then drains at most one additional cap before closing. type trackingBody struct { reader io.Reader readHit atomic.Bool @@ -631,9 +630,7 @@ func (t *trackingTransport) RoundTrip(_ *http.Request) (*http.Response, error) { } // TestDoTokenRequest_ClosesBody verifies that both the success and error -// paths close the response body. The body is intentionally NOT drained past -// the LimitReader cap (see TestDoTokenRequest_DoesNotDrainOversizedBody for -// the regression test on that property). +// paths bounded-drain and close the response body. func TestDoTokenRequest_ClosesBody(t *testing.T) { t.Parallel() @@ -694,29 +691,19 @@ func TestDoTokenRequest_ClosesBody(t *testing.T) { } } -// TestDoTokenRequest_DoesNotDrainOversizedBody pins the behavior that the -// response body is closed without an unbounded drain. A malicious or -// misbehaving IdP could return an arbitrarily large body; the earlier -// io.Copy(io.Discard, resp.Body) drain in the defer would read all of it, -// defeating the maxResponseBodySize cap and (with a caller-supplied -// no-timeout client) blocking the goroutine indefinitely. -// -// This test wires a response body ten times larger than the cap and asserts -// the number of bytes read from the underlying body stays within one Read -// buffer of maxResponseBodySize — i.e., only the LimitReader's quota is -// consumed, not the full body. -func TestDoTokenRequest_DoesNotDrainOversizedBody(t *testing.T) { +// TestDoTokenRequest_BoundedDrainAfterRFC6749Error verifies that a non-2xx +// RFC 6749 error response is drained before closing, while consumption remains +// bounded. The initial read and the drain each consume at most +// maxResponseBodySize, preventing oversized or never-ending IdP responses from +// causing unbounded reads. +func TestDoTokenRequest_BoundedDrainAfterRFC6749Error(t *testing.T) { t.Parallel() - // 10 MiB body — well over the 1 MiB cap. - oversized := make([]byte, 10*maxResponseBodySize) - for i := range oversized { - oversized[i] = 'A' - } - + const errorBody = `{"error":"invalid_grant"}` + body := []byte(errorBody + strings.Repeat(" ", 3*maxResponseBodySize-len(errorBody))) tr := &trackingTransport{ - status: http.StatusOK, - bodyBytes: oversized, + status: http.StatusBadRequest, + bodyBytes: body, contentTyp: "application/json", } client := &http.Client{Transport: tr} @@ -724,22 +711,20 @@ func TestDoTokenRequest_DoesNotDrainOversizedBody(t *testing.T) { req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "http://example/token", strings.NewReader("")) require.NoError(t, err) - // Expected: ParseTokenResponse fails to unmarshal 'AAAA…' as JSON on a - // 2xx status, returning a wrapped parse error. The key property under - // test is bytesRead, not the error surface. - _, _ = DoTokenRequest(client, req) + tokenResp, err := DoTokenRequest(client, req) + assert.Nil(t, tokenResp) + var retrieveErr *oauth2.RetrieveError + require.True(t, errors.As(err, &retrieveErr)) + assert.Equal(t, "invalid_grant", retrieveErr.ErrorCode) require.NotNil(t, tr.lastBody) assert.True(t, tr.lastBody.closed.Load(), "body must be closed") - // io.LimitReader stops exactly at N bytes; the underlying Read is not - // called again after the limit is hit. Allow a small slop (one typical - // Read buffer, 32 KiB) for implementations that may over-fill on the - // final Read. - const slop = 32 << 10 bytesRead := tr.lastBody.bytesRead.Load() - assert.LessOrEqual(t, bytesRead, int64(maxResponseBodySize)+int64(slop), - "DoTokenRequest must not drain the response body past the LimitReader cap") + assert.Equal(t, int64(2*maxResponseBodySize), bytesRead, + "DoTokenRequest must drain one bounded chunk after reading the response") + assert.Less(t, bytesRead, int64(len(body)), + "DoTokenRequest must not drain the entire oversized response body") } // TestDoTokenRequest_ClientDoError surfaces transport-level errors via %w. From cfbe482c4ec5edc7f0f06e6949aab175775f1593 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 17 Aug 2026 15:32:03 +0200 Subject: [PATCH 8/9] Drain actor token error response --- .../virtualmcp/virtualmcp_delegate_clients_test.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go index c4ad1669b8..68ebe50cc3 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_delegate_clients_test.go @@ -257,7 +257,11 @@ var _ = ginkgo.Describe("VirtualMCPServer delegate clients", ginkgo.Ordered, fun req.SetBasicAuth(clientID, clientSecret) response, err := http.DefaultClient.Do(req) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - defer response.Body.Close() + defer func() { + // Drain the expected error response so the HTTP transport can reuse the connection. + _, _ = io.Copy(io.Discard, response.Body) + _ = response.Body.Close() + }() gomega.Expect(response.StatusCode).To(gomega.Equal(http.StatusBadRequest)) }) }) From aec1464745583d268aa8d58ad6801438c1b9e8d7 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 17 Aug 2026 17:26:43 +0200 Subject: [PATCH 9/9] Separate authenticated client identity from asserted actor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit resolveActorIdentity's actorSub (the actor_token's asserted identity, which the prior fix let differ from the authenticated client) was being reused as the authenticated client identity for delegate-client policy, AllowedDelegateClients checks, subject-token client_id binding, and the issued token's own client_id. That let a client exchange a subject token issued to a completely different client, simply by presenting an actor_token whose sub happened to equal that subject token's client_id claim — a full bypass of the "subject token was issued to a different client" binding. Route client.GetID() to every policy/binding decision, and reserve actorSub for what it is actually meant to represent: may_act.sub and the emitted act.sub claim. --- .../server/tokenexchange/handler.go | 42 ++++-- .../server/tokenexchange/handler_test.go | 126 +++++++++++++++--- 2 files changed, 137 insertions(+), 31 deletions(-) diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index 2c44f538e9..926406eb5f 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -167,8 +167,8 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi return err } - configuredDelegate := slices.Contains(h.configuredDelegateClients, actorSub) - if err := checkDelegationConsent(validatedClaims, actorSub, configuredDelegate); err != nil { + configuredDelegate := slices.Contains(h.configuredDelegateClients, client.GetID()) + if err := checkDelegationConsent(validatedClaims, client.GetID(), actorSub, configuredDelegate); err != nil { return err } @@ -180,11 +180,16 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi return err } - // Build the delegated session with the user's identity and the agent's act claim. + // Build the delegated session with the user's identity and the + // authenticated client's identity. The third argument becomes the + // issued token's RFC 9068 client_id, so it must identify the client the + // token was actually issued to (client.GetID()) — not actorSub, which + // may name a distinct actor asserted via actor_token and belongs only + // in the act claim below. delegatedSession := session.New( delegatedSubject(validatedClaims), "", // No IDP session link for delegated tokens. - actorSub, + client.GetID(), session.UserClaims{ Name: validatedClaims.Name, Email: validatedClaims.Email, @@ -605,7 +610,7 @@ func delegateClientAllowed(allowedDelegateClients []string, actorID string) bool // 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, configuredDelegate bool) error { +func checkDelegationConsent(validatedClaims *ValidatedClaims, clientID, actorSub string, configuredDelegate bool) error { // selfIssuedDelegate is true only when a configured delegate client is // presenting a self-issued token (ExternalIssuer == "" rules out the // external-issuer path explicitly, rather than relying on ExternalActor @@ -617,11 +622,18 @@ func checkDelegationConsent(validatedClaims *ValidatedClaims, actorID string, co switch { case validatedClaims.MayAct != nil: - if validatedClaims.MayAct.Sub != actorID { + // may_act.sub is compared against actorSub, not clientID: this is + // the one binding that is meant to key off the asserted actor — + // may_act's whole purpose is authorizing a specific actor, which + // actor_token lets the authenticated client name distinctly from + // itself. + if validatedClaims.MayAct.Sub != actorSub { 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) { + // AllowedDelegateClients binds to the authenticated ToolHive client + // (clientID), not the asserted actor — see the doc comment above. + if validatedClaims.ExternalIssuer != "" && !delegateClientAllowed(validatedClaims.AllowedDelegateClients, clientID) { return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( "This client is not authorized to exchange subject tokens from the external actor's issuer.")) } @@ -631,16 +643,20 @@ func checkDelegationConsent(validatedClaims *ValidatedClaims, actorID string, co // 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. + // clientID or actorSub. 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) { + // AllowedDelegateClients binds this allowlisted external actor to a + // specific set of ToolHive clients — see delegateClientAllowed. This + // must be the authenticated client (clientID), not the asserted + // actor (actorSub): the allowlist is "this external actor's tokens + // may be exchanged by this ToolHive client", independent of whatever + // actor identity that client asserts via actor_token. + if !delegateClientAllowed(validatedClaims.AllowedDelegateClients, clientID) { 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 && !selfIssuedDelegate: + case validatedClaims.ClientID != "" && validatedClaims.ClientID != clientID && !selfIssuedDelegate: return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( "The subject token was issued to a different client.")) case validatedClaims.ClientID == "": diff --git a/pkg/authserver/server/tokenexchange/handler_test.go b/pkg/authserver/server/tokenexchange/handler_test.go index bdb0d95204..3aa6639629 100644 --- a/pkg/authserver/server/tokenexchange/handler_test.go +++ b/pkg/authserver/server/tokenexchange/handler_test.go @@ -322,21 +322,15 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { lifespan: 15 * time.Minute, form: func(t *testing.T) url.Values { t.Helper() - // The subject token's own client_id claim names "other-agent" - // so checkDelegationConsent's client_id fallback binds it to - // that actor (the same way it would bind to testAgentClientID - // in the no-actor_token case). - extra := validExtraClaims() - extra["client_id"] = "other-agent" - subjectToken := tj.signToken(t, validClaims(), extra) - f := url.Values{ - "grant_type": {oauthproto.GrantTypeTokenExchange}, - "subject_token": {subjectToken}, - "subject_token_type": {oauthproto.TokenTypeAccessToken}, - } - // client_id claim still matches the authenticated client, but - // sub names a distinct actor identity — this must be honored, - // not rejected, since the client_id claim is the binding check. + // The subject token's client_id claim matches the + // authenticated client (testAgentClientID) — the normal + // binding case, unaffected by actor_token. actor_token + // separately asserts a distinct actor ("other-agent"); that + // identity flows into act.sub only, never into client-binding + // checks or the issued token's own client_id (see the + // "cannot bypass" test below for the case that used to be + // confused with this one). + f := defaultFormValues(t, tj) f.Set("actor_token", signActorToken(t, tj, "other-agent")) f.Set("actor_token_type", oauthproto.TokenTypeJWT) return f @@ -352,7 +346,49 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { actMap, ok := actClaim.(map[string]interface{}) require.True(t, ok, "act claim must be a map") assert.Equal(t, "other-agent", actMap["sub"]) + + // The issued token's client_id (RFC 9068) must identify the + // authenticated client, not the asserted actor. + assert.Equal(t, testAgentClientID, sess.JWTClaims.Extra["client_id"]) + }, + }, + { + // Regression guard for the exact bug the actor_token/client_id + // binding split fixes: previously, checkDelegationConsent's + // client_id fallback compared the subject token's client_id + // against the *asserted actor* (actorSub), not the authenticated + // client. That let a client authenticated as testAgentClientID + // exchange a subject token issued to ANY other client, simply by + // presenting an actor_token whose sub happened to equal that + // subject token's client_id — a complete bypass of "the subject + // token was issued to a different client". All four identities + // here are distinct on purpose: authenticated client + // (testAgentClientID), actor_token's client_id claim + // (testAgentClientID, so the actor_token binding itself passes), + // actor_token's sub ("other-agent"), and the subject token's own + // client_id claim ("other-agent", chosen to equal the actor sub — + // exactly the value that used to unlock the bypass). + name: "actor_token cannot bypass subject-token client_id binding", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + lifespan: 15 * time.Minute, + form: func(t *testing.T) url.Values { + t.Helper() + extra := validExtraClaims() + extra["client_id"] = "other-agent" + subjectToken := tj.signToken(t, validClaims(), extra) + f := url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + } + f.Set("actor_token", signActorToken(t, tj, "other-agent")) + f.Set("actor_token_type", oauthproto.TokenTypeJWT) + return f }, + wantErr: true, + wantFositeIs: fosite.ErrInvalidGrant, + hintContains: "different client", }, { name: "actor_token client_id mismatch with authenticated client", @@ -1234,8 +1270,14 @@ func TestCheckDelegationConsent(t *testing.T) { const actorID = testAgentClientID tests := []struct { - name string - claims *ValidatedClaims + name string + claims *ValidatedClaims + // clientID/actorSub default to actorID when unset, reproducing the + // pre-split behavior for every case that doesn't care about the + // distinction. Set both explicitly, to different values, for cases + // that test the clientID/actorSub split itself. + clientID string + actorSub string configuredDelegate bool // defaults to false, reproducing pre-delegate-exception behavior wantErr bool errContains string @@ -1416,12 +1458,60 @@ func TestCheckDelegationConsent(t *testing.T) { wantErr: true, errContains: "different client", }, + { + // The clientID/actorSub split: may_act.sub binds to the asserted + // actor (actorSub), not the authenticated client (clientID) — + // this is may_act's whole purpose, letting a client authenticate + // as itself while acting as a distinct actor. + name: "may_act matching actorSub, differing from clientID, accepted", + claims: &ValidatedClaims{MayAct: &MayActClaim{Sub: "distinct-actor"}}, + clientID: "the-authenticated-client", + actorSub: "distinct-actor", + }, + { + // The clientID/actorSub split, external-issuer path: + // AllowedDelegateClients binds to the authenticated client + // (clientID), not the asserted actor — an actor identity that + // happens to collide with an allowlisted client must not grant + // access on its own. + name: "ExternalActor set, actorSub in AllowedDelegateClients but clientID is not rejected", + claims: &ValidatedClaims{ + ExternalActor: "ext-agent", + AllowedDelegateClients: []string{"distinct-actor"}, + }, + clientID: "the-authenticated-client", + actorSub: "distinct-actor", + wantErr: true, + errContains: "not authorized to exchange subject tokens", + }, + { + // The clientID/actorSub split, client_id-binding fallback: the + // subject token's client_id must match the authenticated client + // (clientID), not the asserted actor (actorSub) — this is the + // exact bypass TestTokenExchangeHandler_HandleTokenEndpointRequest's + // "actor_token cannot bypass subject-token client_id binding" + // covers end-to-end; this pins the same invariant at the + // checkDelegationConsent unit level. + name: "client_id matching actorSub but not clientID rejected", + claims: &ValidatedClaims{ClientID: "distinct-actor"}, + clientID: "the-authenticated-client", + actorSub: "distinct-actor", + wantErr: true, + errContains: "different client", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := checkDelegationConsent(tt.claims, actorID, tt.configuredDelegate) + clientID, actorSub := tt.clientID, tt.actorSub + if clientID == "" { + clientID = actorID + } + if actorSub == "" { + actorSub = actorID + } + err := checkDelegationConsent(tt.claims, clientID, actorSub, tt.configuredDelegate) if tt.wantErr { require.Error(t, err) var rfcErr *fosite.RFC6749Error