diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index 35678284f6..951f319f4d 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -15,6 +15,7 @@ import ( "github.com/ory/fosite/handler/oauth2" "github.com/ory/x/errorsx" + coreaudit "github.com/stacklok/toolhive-core/audit" "github.com/stacklok/toolhive/pkg/authserver/server" "github.com/stacklok/toolhive/pkg/authserver/server/session" "github.com/stacklok/toolhive/pkg/oauthproto" @@ -154,10 +155,38 @@ func (h *Handler) HandleTokenEndpointRequest(ctx context.Context, requester fosi // delegation chain remains auditable rather than being discarded. act := map[string]any{"sub": actorID} if priorAct, ok := validatedClaims.Extra["act"]; ok && priorAct != nil { - if actChainDepth(priorAct) >= maxDelegationDepth { + // Parse with the shared audit-side parser rather than a bespoke walker: it + // reports both the chain depth and any RFC 8693 Section 4.1 conformance + // violation, so a non-object act cannot slip past the depth gate and be + // re-minted. A JSON-null act is filtered by the guard above: it asserts no + // delegation and must not read as malformed. + chain := coreaudit.ParseDelegationChain(priorAct, maxDelegationDepth) + if chain.Malformed { + // RFC 8693 Section 2.2.2 nominally calls for invalid_request on a bad + // subject_token, but also allows that "other error codes may also be + // used, as appropriate": invalid_grant keeps this consistent with the + // depth, consent, and expiry gates in this same handler, all of which + // reject a structurally unacceptable subject token that way. + // + // MalformedReason is a closed, value-free enum; it is surfaced to the + // client and MUST stay that way — never interpolate claim contents here. + return errorsx.WithStack(fosite.ErrInvalidGrant.WithHintf( + "The subject token's delegation chain is malformed (%s).", chain.MalformedReason)) + } + // Equivalent to the depth of the prior chain: the parser appends exactly one + // hop per level and stops at maxDelegationDepth, so len(Chain) is + // min(depth, maxDelegationDepth). + if len(chain.Chain) >= maxDelegationDepth { return errorsx.WithStack(fosite.ErrInvalidGrant.WithHint( "The subject token's delegation chain is too deep.")) } + // Nest priorAct verbatim rather than re-serializing from chain: core keeps + // each hop's extra claims in an unexported map, so rebuilding would silently + // drop the history trail that Section 4.1 asks us to preserve. The cost is + // that unknown hop claims — and the non-identity claims Section 4.1 calls + // "not meaningful" inside act — pass through re-signed, which sits in + // tension with Section 6 data minimization. Accepted here; bounding the + // subtree belongs with the external-issuer work in #5989. act["act"] = priorAct } delegatedSession.JWTClaims.Extra["act"] = act @@ -468,22 +497,3 @@ func ensureAudienceSubsetOfSubject(granted, subjectAud []string) error { } return nil } - -// actChainDepth returns the number of nested "act" entries in a subject -// token's delegation chain, starting from its outermost act claim. A -// non-map act (or nil) has depth 0. -func actChainDepth(act any) int { - depth := 0 - for { - m, ok := act.(map[string]any) - if !ok { - return depth - } - depth++ - next, ok := m["act"] - if !ok || next == nil { - return depth - } - act = next - } -} diff --git a/pkg/authserver/server/tokenexchange/handler_test.go b/pkg/authserver/server/tokenexchange/handler_test.go index d5bfb45e03..6d6b042512 100644 --- a/pkg/authserver/server/tokenexchange/handler_test.go +++ b/pkg/authserver/server/tokenexchange/handler_test.go @@ -81,9 +81,21 @@ func defaultFormValues(t *testing.T, tj *testJWKS) url.Values { } } +// actFormValues returns valid token exchange form values whose subject token +// carries the given "act" claim. +func actFormValues(t *testing.T, tj *testJWKS, act any) url.Values { + t.Helper() + + extra := validExtraClaims() + extra["act"] = act + form := defaultFormValues(t, tj) + form.Set("subject_token", tj.signToken(t, validClaims(), extra)) + return form +} + // nestedActChain builds a chain of depth nested "act" maps, each holding a -// distinct "sub", for exercising actChainDepth boundary behavior. depth must -// be at least 1. +// distinct "sub", for exercising the delegation-depth cap. depth must be at +// least 1. func nestedActChain(depth int) map[string]any { chain := map[string]any{"sub": fmt.Sprintf("agent-%d", depth-1)} for i := depth - 2; i >= 0; i-- { @@ -709,6 +721,112 @@ func TestTokenExchangeHandler_HandleTokenEndpointRequest(t *testing.T) { "the prior act chain should be nested unchanged under the new act claim") }, }, + { + // RFC 8693 Section 4.1 requires act to be a JSON object. Rejecting keeps + // us from re-minting the violation. A non-object array or scalar reaches + // the same parser branch, so the string case covers both. + name: "non-object act rejected", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + return actFormValues(t, tj, "some-agent") + }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidGrant, + hintContains: "act_not_object", + }, + { + name: "non-object nested act rejected", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + return actFormValues(t, tj, map[string]any{"sub": "agent-0", "act": "agent-1"}) + }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidGrant, + hintContains: "nested_act_not_object", + }, + { + // RFC 7519 Section 4.1.2 fixes sub as a string, so a hop carrying a + // non-string sub is non-conformant even though the chain is an object. + name: "non-string sub in act hop rejected", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + return actFormValues(t, tj, map[string]any{"sub": 42}) + }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidGrant, + hintContains: "sub_not_string", + }, + { + // RFC 7519 Section 4.1.1 fixes iss as a string too. sub is kept valid + // here to isolate the reason: the parser reports the first violation it + // finds and checks iss before sub, so a hop with both would report only + // iss_not_string and leave this branch unpinned. + name: "non-string iss in act hop rejected", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + return actFormValues(t, tj, map[string]any{"iss": 42, "sub": "agent-0"}) + }, + lifespan: 15 * time.Minute, + wantErr: true, + wantFositeIs: fosite.ErrInvalidGrant, + hintContains: "iss_not_string", + }, + { + // A JSON-null nested act ends the chain; it asserts no further + // delegation and must not read as malformed. + name: "null nested act is accepted and nested unchanged", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + return actFormValues(t, tj, map[string]any{"sub": "agent-0", "act": nil}) + }, + 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") + actMap, ok := sess.JWTClaims.Extra["act"].(map[string]any) + require.True(t, ok, "act claim must be a map") + assert.Equal(t, testAgentClientID, actMap["sub"]) + assert.Equal(t, map[string]any{"sub": "agent-0", "act": nil}, actMap["act"], + "the prior act claim should be nested verbatim, null member included") + }, + }, + { + // Tolerated by design: RFC 8693 Section 4.1 only requires an object, and + // sub is conventional rather than mandatory, so an empty hop is + // conformant. Pinned so a future tightening of the shared parser breaks + // a test rather than a caller. + name: "empty act object is accepted", + ctx: func(_ *testing.T) context.Context { return context.Background() }, + client: defaultClient, + form: func(t *testing.T) url.Values { + t.Helper() + return actFormValues(t, tj, map[string]any{}) + }, + 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") + actMap, ok := sess.JWTClaims.Extra["act"].(map[string]any) + require.True(t, ok, "act claim must be a map") + assert.Equal(t, testAgentClientID, actMap["sub"]) + assert.Equal(t, map[string]any{}, actMap["act"]) + }, + }, } for _, tt := range tests { @@ -841,29 +959,6 @@ func TestTokenExchangeHandler_DefaultAudience(t *testing.T) { } } -func TestActChainDepth(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - act any - want int - }{ - {name: "nil is depth 0", act: nil, want: 0}, - {name: "non-map is depth 0", act: "not-a-map", want: 0}, - {name: "single act entry is depth 1", act: nestedActChain(1), want: 1}, - {name: "nested chain reports its full depth", act: nestedActChain(5), want: 5}, - {name: "map without a nested act claim stops at depth 1", act: map[string]any{"sub": "agent"}, want: 1}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - assert.Equal(t, tt.want, actChainDepth(tt.act)) - }) - } -} - // mockConfig implements the tokenExchangeConfig interface for testing. type mockConfig struct { scopeStrategy fosite.ScopeStrategy