From 5b6b772c5cab41cd368c762cd94d8cfefe966cde Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 08:55:26 +0200 Subject: [PATCH 1/9] Honor maxDelegationDepth above the auth default The audit config knob was silently ineffective for any value above 10. claimsToIdentity parses the RFC 8693 "act" claim at the hardcoded DefaultMaxDelegationDepth, and rebindDelegationChain can only shrink an already-parsed actor slice, so it could never recover actors the identity layer dropped. An operator setting maxDelegationDepth to 25 still got 10 actors and no warning, even though the full chain was still present in the raw claim. Re-parse the raw "act" claim at the configured depth when the identity-layer chain was truncated and audit asks for more actors than it kept. The re-parse is trusted only when it yields at least as many actors as are already parsed, so it can widen the chain but never narrow it. Co-Authored-By: Claude Opus 5 --- pkg/audit/auditor.go | 29 ++++++---- pkg/audit/auditor_test.go | 112 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 11 deletions(-) diff --git a/pkg/audit/auditor.go b/pkg/audit/auditor.go index f321edc988..1062ce7c55 100644 --- a/pkg/audit/auditor.go +++ b/pkg/audit/auditor.go @@ -558,24 +558,31 @@ func extractSubjectsFromIdentity(identity *auth.Identity) map[string]string { } // extractDelegationChainFromIdentity returns the RFC 8693 delegation chain -// carried by the identity, re-bounded to maxDepth, or nil when the identity -// has no chain. An already-parsed chain is re-bounded by applying maxDepth to -// its actor list directly; an identity carrying only the raw "act" claim is -// parsed once via auth.ParseDelegationChain. In both cases the configured cap -// is enforced — there is no uncapped "honor as-is" fallback. +// carried by the identity, bound to maxDepth, or nil when there is none. +// The identity layer (auth.claimsToIdentity) always parses at +// auth.DefaultMaxDelegationDepth, so a larger maxDepth requires re-parsing the +// raw "act" claim — rebinding an already-parsed chain can only shrink it. The +// re-parse is only trusted when it yields at least as many actors as are +// already parsed, so it can widen the chain but never narrow it. func extractDelegationChainFromIdentity(identity *auth.Identity, maxDepth int) *auth.DelegationChain { if identity == nil { return nil } - if chain := identity.DelegationChain; chain != nil && len(chain.Actors) > 0 { - return rebindDelegationChain(chain, maxDepth) + rawAct := identity.Claims["act"] + chain := identity.DelegationChain + existingActors := 0 + if chain != nil { + existingActors = len(chain.Actors) } - if act, ok := identity.Claims["act"]; ok && act != nil { - chain := auth.ParseDelegationChain(act, maxDepth) - if len(chain.Actors) > 0 { - return &chain + if rawAct != nil && (chain == nil || len(chain.Actors) == 0 || + (chain.Truncated && maxDepth > len(chain.Actors))) { + if parsed := auth.ParseDelegationChain(rawAct, maxDepth); len(parsed.Actors) > 0 && len(parsed.Actors) >= existingActors { + return &parsed } } + if chain != nil && len(chain.Actors) > 0 { + return rebindDelegationChain(chain, maxDepth) + } return nil } diff --git a/pkg/audit/auditor_test.go b/pkg/audit/auditor_test.go index 5d2e8554b2..44f26cf33d 100644 --- a/pkg/audit/auditor_test.go +++ b/pkg/audit/auditor_test.go @@ -628,6 +628,42 @@ func TestExtractSubjects(t *testing.T) { assert.Equal(t, 1, chain.DroppedCount, "one dropped actor must be reported") }) + t.Run("delegation chain widens through configured max depth", func(t *testing.T) { + t.Parallel() + maxDepth := 25 + depthAuditor, err := NewAuditorWithTransport(&Config{MaxDelegationDepth: &maxDepth}, "sse") + require.NoError(t, err) + + subs := []string{ + "agent-1", "agent-2", "agent-3", "agent-4", "agent-5", + "agent-6", "agent-7", "agent-8", "agent-9", "agent-10", + "agent-11", "agent-12", "agent-13", "agent-14", + } + claims := jwt.MapClaims{ + "sub": "user123", + "act": nestedActClaim(subs...), + } + + req := httptest.NewRequest("GET", "/test", nil) + // Shaped like what auth.claimsToIdentity produces for a 14-hop token: + // the identity-layer parse caps at auth.DefaultMaxDelegationDepth (10), + // truncating the chain even though the raw "act" claim has all 14 hops. + parsed := auth.ParseDelegationChain(claims["act"], auth.DefaultMaxDelegationDepth) + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "user123", + Claims: claims, + DelegationChain: &parsed, + }, + } + req = req.WithContext(auth.WithIdentity(req.Context(), identity)) + + chain := depthAuditor.extractDelegationChain(req) + require.NotNil(t, chain) + require.Len(t, chain.Actors, len(subs), "configured depth must recover actors dropped by the identity-layer parse") + assert.False(t, chain.Truncated) + }) + t.Run("no identity yields nil delegation chain", func(t *testing.T) { t.Parallel() req := httptest.NewRequest("GET", "/test", nil) @@ -702,6 +738,82 @@ func TestExtractDelegationChainFromIdentity(t *testing.T) { } assert.Nil(t, extractDelegationChainFromIdentity(identity, auth.DefaultMaxDelegationDepth)) }) + + // The following two cases share an identity shaped exactly like what + // auth.claimsToIdentity produces for a 14-hop token: the identity-layer + // parse always caps at auth.DefaultMaxDelegationDepth (10), so + // DelegationChain holds only the first 10 actors, truncated, with the + // full 14-hop chain still available in the raw "act" claim. + subs := []string{ + "agent-1", "agent-2", "agent-3", "agent-4", "agent-5", + "agent-6", "agent-7", "agent-8", "agent-9", "agent-10", + "agent-11", "agent-12", "agent-13", "agent-14", + } + truncatedIdentity := func() *auth.Identity { + actors := make([]auth.DelegatedActor, auth.DefaultMaxDelegationDepth) + for i := range actors { + actors[i] = auth.DelegatedActor{Subject: subs[i]} + } + return &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "user123", + DelegationChain: &auth.DelegationChain{ + Actors: actors, + Truncated: true, + DroppedCount: len(subs) - auth.DefaultMaxDelegationDepth, + }, + Claims: map[string]any{"act": nestedActClaim(subs...)}, + }, + } + } + + t.Run("configured depth wider than identity-layer parse recovers dropped actors", func(t *testing.T) { + t.Parallel() + chain := extractDelegationChainFromIdentity(truncatedIdentity(), 25) + require.NotNil(t, chain) + require.Len(t, chain.Actors, len(subs)) + assert.False(t, chain.Truncated) + assert.Zero(t, chain.DroppedCount) + assert.Equal(t, subs[0], chain.Actors[0].Subject) + assert.Equal(t, subs[len(subs)-1], chain.Actors[len(subs)-1].Subject) + }) + + t.Run("configured depth narrower than identity-layer parse still rebinds", func(t *testing.T) { + t.Parallel() + chain := extractDelegationChainFromIdentity(truncatedIdentity(), 3) + require.NotNil(t, chain) + require.Len(t, chain.Actors, 3) + assert.True(t, chain.Truncated) + // 4 actors already dropped by the identity-layer parse (14 - 10), + // plus 7 more dropped by the narrower rebind (10 - 3). + assert.Equal(t, 11, chain.DroppedCount) + }) + + t.Run("truncated chain without raw act claim falls back to rebind", func(t *testing.T) { + t.Parallel() + identity := truncatedIdentity() + identity.Claims = nil // widening requested, but no raw claim to re-parse + + chain := extractDelegationChainFromIdentity(identity, 25) + require.NotNil(t, chain) + require.Len(t, chain.Actors, auth.DefaultMaxDelegationDepth) + assert.True(t, chain.Truncated, "without the raw claim, the identity-layer truncation cannot be recovered") + assert.Equal(t, 4, chain.DroppedCount) + }) +} + +// nestedActClaim builds a raw RFC 8693 "act" claim nesting subs in order, +// outermost first, matching the shape auth.ParseDelegationChain expects. +func nestedActClaim(subs ...string) map[string]any { + var current map[string]any + for i := len(subs) - 1; i >= 0; i-- { + m := map[string]any{"sub": subs[i]} + if current != nil { + m["act"] = current + } + current = m + } + return current } func TestDetermineComponent(t *testing.T) { From 066bf821f74447598e116aa410fd5235b6d4846b Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 09:45:03 +0200 Subject: [PATCH 2/9] Drop the test-only log-writer hook from Auditor SetLogWriterForTest added exported, immediately-deprecated test surface to a production type, and both call sites needed //nolint:staticcheck to mute the deprecation the same change introduced. It also wrote auditLogger with no synchronization on a type whose Middleware reads it from concurrent request goroutines, and never closed the writer it replaced. None of it was necessary: NewAuditorWithTransport already selects its writer from Config.LogFile, so the integration tests can point it at a t.TempDir() file and read the events back. That also converges these tests on the capture pattern the vMCP authz integration tests already use. Co-Authored-By: Claude Opus 5 --- pkg/audit/auditor.go | 12 --------- pkg/authserver/integration_test.go | 43 +++++++++++++++++++----------- 2 files changed, 27 insertions(+), 28 deletions(-) diff --git a/pkg/audit/auditor.go b/pkg/audit/auditor.go index 1062ce7c55..3528691bad 100644 --- a/pkg/audit/auditor.go +++ b/pkg/audit/auditor.go @@ -113,18 +113,6 @@ func (a *Auditor) Close() error { return nil } -// SetLogWriterForTest redirects the auditor's log output to w. It is intended -// for tests in other packages (e.g. integration tests) that need to capture -// emitted audit events without reimplementing the auditor. -// -// Deprecated: this is test-only surface on a production type. A future -// NewAuditorWithWriter constructor accepting an explicit io.Writer should -// replace it; keep usage confined to tests. -func (a *Auditor) SetLogWriterForTest(w io.Writer) { - a.auditLogger = NewAuditLogger(w) - a.logWriter = w -} - // isSSETransport checks if the current transport is SSE func (a *Auditor) isSSETransport() bool { return a.transportType == types.TransportTypeSSE.String() diff --git a/pkg/authserver/integration_test.go b/pkg/authserver/integration_test.go index f83e406571..266dff6ab0 100644 --- a/pkg/authserver/integration_test.go +++ b/pkg/authserver/integration_test.go @@ -15,6 +15,8 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" + "path/filepath" "strings" "sync/atomic" "testing" @@ -790,6 +792,23 @@ func TestIntegration_TokenExchange_ConfidentialClientHappyPath(t *testing.T) { "delegated token exp must be capped at the 15m delegation lifespan, not the subject token's 30m") } +// readSingleAuditEvent reads the audit log file at path and decodes exactly +// one newline-delimited JSON event, failing the test if there isn't exactly one. +func readSingleAuditEvent(t *testing.T, path string) map[string]any { + t.Helper() + + data, err := os.ReadFile(path) + require.NoError(t, err) + require.NotEmpty(t, strings.TrimSpace(string(data)), "audit log is empty; expected one event") + + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + require.Len(t, lines, 1, "expected exactly one audit event, got: %q", string(data)) + + var event map[string]any + require.NoError(t, json.Unmarshal([]byte(lines[0]), &event)) + return event +} + // TestIntegration_TokenExchange_AuditDelegationChain proves the end-to-end // audit story for RFC 8693 delegation: a delegated token minted by the real // token-exchange handler is validated by the auth middleware, and the audit @@ -866,10 +885,10 @@ func TestIntegration_TokenExchange_AuditDelegationChain(t *testing.T) { // Chain: audit (outer) -> auth -> bare stub handler. The stub does NOT // re-publish the identity: this test pins that TokenValidator.Middleware // alone publishes the validated identity to the audit holder. - var auditBuf bytes.Buffer - auditor, err := audit.NewAuditorWithTransport(&audit.Config{}, "streamable-http") + auditLog := filepath.Join(t.TempDir(), "audit.log") + auditor, err := audit.NewAuditorWithTransport(&audit.Config{LogFile: auditLog}, "streamable-http") require.NoError(t, err) - auditor.SetLogWriterForTest(&auditBuf) //nolint:staticcheck // test-only writer capture until NewAuditorWithWriter exists + defer auditor.Close() stub := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) @@ -884,11 +903,7 @@ func TestIntegration_TokenExchange_AuditDelegationChain(t *testing.T) { "delegated token must validate through the auth middleware") // Exactly one audit event, carrying the delegation chain. - lines := strings.Split(strings.TrimSpace(auditBuf.String()), "\n") - require.Len(t, lines, 1, "expected exactly one audit event, got: %q", auditBuf.String()) - - var event map[string]any - require.NoError(t, json.Unmarshal([]byte(lines[0]), &event)) + event := readSingleAuditEvent(t, auditLog) subjects, ok := event["subjects"].(map[string]any) require.True(t, ok, "subjects should be a map") @@ -944,10 +959,10 @@ func TestIntegration_AuditMiddleware_NonDelegatedTokenOmitsDelegationChain(t *te }, auth.WithKeyProvider(&testKeyProvider{key: ts.PrivateKey})) require.NoError(t, err) - var auditBuf bytes.Buffer - auditor, err := audit.NewAuditorWithTransport(&audit.Config{}, "streamable-http") + auditLog := filepath.Join(t.TempDir(), "audit.log") + auditor, err := audit.NewAuditorWithTransport(&audit.Config{LogFile: auditLog}, "streamable-http") require.NoError(t, err) - auditor.SetLogWriterForTest(&auditBuf) //nolint:staticcheck // test-only writer capture until NewAuditorWithWriter exists + defer auditor.Close() stub := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) @@ -961,11 +976,7 @@ func TestIntegration_AuditMiddleware_NonDelegatedTokenOmitsDelegationChain(t *te require.Equal(t, http.StatusOK, rr.Code, "plain token must validate through the auth middleware") - lines := strings.Split(strings.TrimSpace(auditBuf.String()), "\n") - require.Len(t, lines, 1, "expected exactly one audit event, got: %q", auditBuf.String()) - - var event map[string]any - require.NoError(t, json.Unmarshal([]byte(lines[0]), &event)) + event := readSingleAuditEvent(t, auditLog) subjects, ok := event["subjects"].(map[string]any) require.True(t, ok, "subjects should be a map") From d87836ff911349f398d02af96d377bae9b484ffb Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 10:05:18 +0200 Subject: [PATCH 3/9] Assert delegation capture across all Log* methods attachDelegation is hand-repeated in all eight WorkflowAuditor Log* methods, but only LogWorkflowStarted and LogStepStarted were covered, so dropping the call from any of the other six -- including every terminal failed/timed-out event, which is what an investigator reads -- would ship silently. Table-drive the assertion over every method, and pin dropped_count and the surviving actor under a configured depth cap. Drop the identity_with_delegation_chain case from ExtractSubjects: extractSubjects never reads DelegationChain, so the case only restated what complete_identity already asserts and passed with the whole feature removed. The behaviour it appeared to cover is now pinned by TestWorkflowAuditor_DelegationChain. Co-Authored-By: Claude Opus 5 --- pkg/audit/workflow_auditor_test.go | 143 ++++++++++++++++------------- 1 file changed, 81 insertions(+), 62 deletions(-) diff --git a/pkg/audit/workflow_auditor_test.go b/pkg/audit/workflow_auditor_test.go index e2f71c6113..83da6b66c7 100644 --- a/pkg/audit/workflow_auditor_test.go +++ b/pkg/audit/workflow_auditor_test.go @@ -565,31 +565,6 @@ func TestWorkflowAuditor_ExtractSubjects(t *testing.T) { SubjectKeyUser: "johndoe", }, }, - { - name: "identity_with_delegation_chain", - identity: &auth.Identity{ - PrincipalInfo: auth.PrincipalInfo{ - Subject: "user-delegated", - Name: "Delegated User", - Claims: map[string]any{ - "act": map[string]any{ - "sub": "agent-1", - "act": map[string]any{"sub": "agent-2"}, - }, - }, - DelegationChain: &auth.DelegationChain{ - Actors: []auth.DelegatedActor{ - {Subject: "agent-1"}, - {Subject: "agent-2"}, - }, - }, - }, - }, - wantSubjects: map[string]string{ - SubjectKeyUserID: "user-delegated", - SubjectKeyUser: "Delegated User", - }, - }, { name: "anonymous_user", identity: nil, @@ -641,46 +616,86 @@ func TestWorkflowAuditor_DelegationChain(t *testing.T) { }, } - t.Run("workflow event carries delegation chain", func(t *testing.T) { - t.Parallel() - auditor, writer := createTestAuditor(t, DefaultConfig()) - - ctx := auth.WithIdentity(context.Background(), delegatedIdentity) - auditor.LogWorkflowStarted(ctx, "wf-1", "wf", nil, time.Second) - - require.NotEmpty(t, writer.logs, "expected log entry") - entry := parseLogEntry(t, writer.getLastLog()) - - chain, ok := entry["delegation_chain"].(map[string]any) - require.True(t, ok, "delegation_chain should be present in the log output") - assert.Equal(t, false, chain["truncated"]) - actors, ok := chain["actors"].([]any) - require.True(t, ok) - require.Len(t, actors, 2) - first, ok := actors[0].(map[string]any) - require.True(t, ok) - assert.Equal(t, "agent-1", first["sub"]) - second, ok := actors[1].(map[string]any) - require.True(t, ok) - assert.Equal(t, "agent-2", second["sub"]) - }) + // attachDelegation is hand-repeated in every Log* method, so each call site is pinned individually. + logMethods := []struct { + name string + logFunc func(a *WorkflowAuditor, ctx context.Context) + }{ + { + name: "LogWorkflowStarted", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogWorkflowStarted(ctx, "wf-1", "wf", nil, time.Second) + }, + }, + { + name: "LogWorkflowCompleted", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogWorkflowCompleted(ctx, "wf-1", "wf", time.Second, 1, nil) + }, + }, + { + name: "LogWorkflowFailed", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogWorkflowFailed(ctx, "wf-1", "wf", time.Second, 1, errors.New("failed")) + }, + }, + { + name: "LogWorkflowTimedOut", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogWorkflowTimedOut(ctx, "wf-1", "wf", time.Second, 1) + }, + }, + { + name: "LogStepStarted", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogStepStarted(ctx, "wf-1", "step-1", "tool", "some-tool") + }, + }, + { + name: "LogStepCompleted", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogStepCompleted(ctx, "wf-1", "step-1", time.Second, 0) + }, + }, + { + name: "LogStepFailed", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogStepFailed(ctx, "wf-1", "step-1", time.Second, 0, errors.New("failed")) + }, + }, + { + name: "LogStepSkipped", + logFunc: func(a *WorkflowAuditor, ctx context.Context) { + a.LogStepSkipped(ctx, "wf-1", "step-1", "condition") + }, + }, + } - t.Run("step event carries delegation chain", func(t *testing.T) { - t.Parallel() - auditor, writer := createTestAuditor(t, DefaultConfig()) + for _, tt := range logMethods { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + auditor, writer := createTestAuditor(t, DefaultConfig()) - ctx := auth.WithIdentity(context.Background(), delegatedIdentity) - auditor.LogStepStarted(ctx, "wf-1", "step-1", "tool", "some-tool") + ctx := auth.WithIdentity(context.Background(), delegatedIdentity) + tt.logFunc(auditor, ctx) - require.NotEmpty(t, writer.logs, "expected log entry") - entry := parseLogEntry(t, writer.getLastLog()) + require.NotEmpty(t, writer.logs, "expected log entry") + entry := parseLogEntry(t, writer.getLastLog()) - chain, ok := entry["delegation_chain"].(map[string]any) - require.True(t, ok, "delegation_chain should be present in the log output") - actors, ok := chain["actors"].([]any) - require.True(t, ok) - assert.Len(t, actors, 2) - }) + chain, ok := entry["delegation_chain"].(map[string]any) + require.True(t, ok, "delegation_chain should be present in the log output") + assert.Equal(t, false, chain["truncated"]) + actors, ok := chain["actors"].([]any) + require.True(t, ok) + require.Len(t, actors, 2) + first, ok := actors[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, "agent-1", first["sub"]) + second, ok := actors[1].(map[string]any) + require.True(t, ok) + assert.Equal(t, "agent-2", second["sub"]) + }) + } t.Run("no identity omits delegation chain", func(t *testing.T) { t.Parallel() @@ -711,7 +726,11 @@ func TestWorkflowAuditor_DelegationChain(t *testing.T) { assert.Equal(t, true, chain["truncated"]) actors, ok := chain["actors"].([]any) require.True(t, ok) - assert.Len(t, actors, 1) + require.Len(t, actors, 1) + first, ok := actors[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, "agent-1", first["sub"]) + assert.Equal(t, float64(1), chain["dropped_count"]) }) } From 4ca8545561d8a63b69b75291fb823fc5ad02ccb3 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 10:33:36 +0200 Subject: [PATCH 4/9] Extract the delegation audit test helpers The two delegation audit tests were ~90-line near-clones differing only in the token minted and the final assertion, so every new case would have been another copy. Move them to their own file and factor the shared setup into signTestJWT, exchangeForDelegatedToken and auditEventForToken. Mechanical: the assertion sets are unchanged, including failure messages, which the helpers carry through as a parameter. The only addition is asserting the auditor's Close error, and the switch from defer to t.Cleanup that the parallel-test rule asks for. Co-Authored-By: Claude Opus 5 --- .../integration_delegation_audit_test.go | 218 ++++++++++++++++++ pkg/authserver/integration_test.go | 199 ---------------- 2 files changed, 218 insertions(+), 199 deletions(-) create mode 100644 pkg/authserver/integration_delegation_audit_test.go diff --git a/pkg/authserver/integration_delegation_audit_test.go b/pkg/authserver/integration_delegation_audit_test.go new file mode 100644 index 0000000000..efb5af6fd6 --- /dev/null +++ b/pkg/authserver/integration_delegation_audit_test.go @@ -0,0 +1,218 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package authserver + +import ( + "context" + "crypto/rsa" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/go-jose/go-jose/v4" + "github.com/go-jose/go-jose/v4/jwt" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/audit" + "github.com/stacklok/toolhive/pkg/auth" + "github.com/stacklok/toolhive/pkg/authserver/server/registration" + "github.com/stacklok/toolhive/pkg/oauthproto" +) + +// readSingleAuditEvent reads the audit log file at path and decodes exactly +// one newline-delimited JSON event, failing the test if there isn't exactly one. +func readSingleAuditEvent(t *testing.T, path string) map[string]any { + t.Helper() + + data, err := os.ReadFile(path) + require.NoError(t, err) + require.NotEmpty(t, strings.TrimSpace(string(data)), "audit log is empty; expected one event") + + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + require.Len(t, lines, 1, "expected exactly one audit event, got: %q", string(data)) + + var event map[string]any + require.NoError(t, json.Unmarshal([]byte(lines[0]), &event)) + return event +} + +// signTestJWT signs a JWT with key using the standard test issuer/audience and +// a 30-minute expiry, embedding subject and any extraClaims (e.g. "client_id" +// for a delegation act claim). extraClaims may be nil. +func signTestJWT(t *testing.T, key *rsa.PrivateKey, subject string, extraClaims map[string]any) string { + t.Helper() + + signer, err := jose.NewSigner( + jose.SigningKey{Algorithm: jose.RS256, Key: key}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "test-key"), + ) + require.NoError(t, err) + + now := time.Now() + builder := jwt.Signed(signer). + Claims(jwt.Claims{ + Issuer: testIssuer, + Subject: subject, + Audience: jwt.Audience{testAudience}, + Expiry: jwt.NewNumericDate(now.Add(30 * time.Minute)), + IssuedAt: jwt.NewNumericDate(now), + }) + if extraClaims != nil { + builder = builder.Claims(extraClaims) + } + token, err := builder.Serialize() + require.NoError(t, err) + return token +} + +// exchangeForDelegatedToken performs an RFC 8693 token exchange against +// serverURL using the given confidential client credentials and subject +// token, and returns the resulting delegated access token. +func exchangeForDelegatedToken(t *testing.T, serverURL, clientID, clientSecret, subjectToken string) string { + t.Helper() + + resp := makeTokenRequest(t, serverURL, url.Values{ + "grant_type": {oauthproto.GrantTypeTokenExchange}, + "subject_token": {subjectToken}, + "subject_token_type": {oauthproto.TokenTypeAccessToken}, + "client_id": {clientID}, + "client_secret": {clientSecret}, + }) + defer resp.Body.Close() + body := parseTokenResponse(t, resp) + require.Equal(t, http.StatusOK, resp.StatusCode, + "token exchange should succeed, got %d (body: %v)", resp.StatusCode, body) + delegated, ok := body["access_token"].(string) + require.True(t, ok, "access_token should be a string") + require.NotEmpty(t, delegated) + return delegated +} + +// auditEventForToken runs token through the audit(auth(stub)) middleware +// chain — audit outermost, auth innermost — and returns the single resulting +// audit event. The stub handler does not re-publish the identity itself: this +// pins that TokenValidator.Middleware alone publishes the validated identity +// to the audit holder via the IdentityHolder read-back. +func auditEventForToken(t *testing.T, key *rsa.PrivateKey, token, validationMsg string) map[string]any { + t.Helper() + + // Build a validator that trusts the server's issuer and keys, exactly like + // the middleware in front of a protected resource server. The in-process + // key provider avoids self-referential HTTP JWKS fetches. + validator, err := auth.NewTokenValidator(context.Background(), auth.TokenValidatorConfig{ + Issuer: testIssuer, + Audience: testAudience, + }, auth.WithKeyProvider(&testKeyProvider{key: key})) + require.NoError(t, err) + + auditLog := filepath.Join(t.TempDir(), "audit.log") + auditor, err := audit.NewAuditorWithTransport(&audit.Config{LogFile: auditLog}, "streamable-http") + require.NoError(t, err) + t.Cleanup(func() { + assert.NoError(t, auditor.Close()) + }) + + stub := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }) + + req := httptest.NewRequest(http.MethodPost, "/messages", strings.NewReader("{}")) + req.Header.Set("Authorization", "Bearer "+token) + rr := httptest.NewRecorder() + auditor.Middleware(validator.Middleware(stub)).ServeHTTP(rr, req) + + require.Equal(t, http.StatusOK, rr.Code, validationMsg) + + return readSingleAuditEvent(t, auditLog) +} + +// TestIntegration_TokenExchange_AuditDelegationChain proves the end-to-end +// audit story for RFC 8693 delegation: a delegated token minted by the real +// token-exchange handler is validated by the auth middleware, and the audit +// middleware emits an event whose delegationChain records the acting agent. +func TestIntegration_TokenExchange_AuditDelegationChain(t *testing.T) { + t.Parallel() + + const ( + agentClientID = "test-audit-agent-client" + agentClientSecret = "test-audit-agent-secret" + delegatedUserSub = "audit-delegated-user-sub" + ) + + agentClient, err := registration.New(registration.Config{ + ID: agentClientID, + Secret: agentClientSecret, + Public: false, + GrantTypes: []string{oauthproto.GrantTypeTokenExchange}, + Scopes: registration.DefaultScopes, + Audience: []string{testAudience}, + }) + require.NoError(t, err) + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m, withExtraClient(agentClient)) + + // Mint the subject token (user) signed by the server's key. + subjectToken := signTestJWT(t, ts.PrivateKey, delegatedUserSub, map[string]any{ + "client_id": agentClientID, + }) + + // Exchange it for a delegated token carrying act.sub = agent. + delegated := exchangeForDelegatedToken(t, ts.Server.URL, agentClientID, agentClientSecret, subjectToken) + + // Chain: audit (outer) -> auth -> bare stub handler. + event := auditEventForToken(t, ts.PrivateKey, delegated, + "delegated token must validate through the auth middleware") + + subjects, ok := event["subjects"].(map[string]any) + require.True(t, ok, "subjects should be a map") + assert.Equal(t, delegatedUserSub, subjects["user_id"], + "audit subject must be the delegated user, not the agent") + + chain, ok := event["delegation_chain"].(map[string]any) + require.True(t, ok, "delegation_chain must be present in the audit event") + assert.Equal(t, false, chain["truncated"]) + actors, ok := chain["actors"].([]any) + require.True(t, ok, "actors should be an array") + require.Len(t, actors, 1, "a single exchange yields a single actor") + actor, ok := actors[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, agentClientID, actor["sub"], + "the delegation chain must record the acting agent client") +} + +// TestIntegration_AuditMiddleware_NonDelegatedTokenOmitsDelegationChain is the +// negative companion to TestIntegration_TokenExchange_AuditDelegationChain: a +// plain (non-delegated) token through the same audit(auth(stub)) chain must +// produce an audit event with NO delegation_chain member. +func TestIntegration_AuditMiddleware_NonDelegatedTokenOmitsDelegationChain(t *testing.T) { + t.Parallel() + + const plainUserSub = "audit-plain-user-sub" + + m := startMockOIDC(t) + ts := setupTestServerWithMockOIDC(t, m) + + // Mint a plain subject token signed by the server's key (no act claim). + plainToken := signTestJWT(t, ts.PrivateKey, plainUserSub, nil) + + event := auditEventForToken(t, ts.PrivateKey, plainToken, + "plain token must validate through the auth middleware") + + subjects, ok := event["subjects"].(map[string]any) + require.True(t, ok, "subjects should be a map") + assert.Equal(t, plainUserSub, subjects["user_id"], + "audit subject must be the authenticated user") + + _, exists := event["delegation_chain"] + assert.False(t, exists, + "a non-delegated token must not produce a delegation_chain member") +} diff --git a/pkg/authserver/integration_test.go b/pkg/authserver/integration_test.go index 266dff6ab0..0c5f20e214 100644 --- a/pkg/authserver/integration_test.go +++ b/pkg/authserver/integration_test.go @@ -15,8 +15,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "os" - "path/filepath" "strings" "sync/atomic" "testing" @@ -31,7 +29,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/stacklok/toolhive/pkg/audit" "github.com/stacklok/toolhive/pkg/auth" "github.com/stacklok/toolhive/pkg/auth/upstreamtoken" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" @@ -792,202 +789,6 @@ func TestIntegration_TokenExchange_ConfidentialClientHappyPath(t *testing.T) { "delegated token exp must be capped at the 15m delegation lifespan, not the subject token's 30m") } -// readSingleAuditEvent reads the audit log file at path and decodes exactly -// one newline-delimited JSON event, failing the test if there isn't exactly one. -func readSingleAuditEvent(t *testing.T, path string) map[string]any { - t.Helper() - - data, err := os.ReadFile(path) - require.NoError(t, err) - require.NotEmpty(t, strings.TrimSpace(string(data)), "audit log is empty; expected one event") - - lines := strings.Split(strings.TrimSpace(string(data)), "\n") - require.Len(t, lines, 1, "expected exactly one audit event, got: %q", string(data)) - - var event map[string]any - require.NoError(t, json.Unmarshal([]byte(lines[0]), &event)) - return event -} - -// TestIntegration_TokenExchange_AuditDelegationChain proves the end-to-end -// audit story for RFC 8693 delegation: a delegated token minted by the real -// token-exchange handler is validated by the auth middleware, and the audit -// middleware emits an event whose delegationChain records the acting agent. -func TestIntegration_TokenExchange_AuditDelegationChain(t *testing.T) { - t.Parallel() - - const ( - agentClientID = "test-audit-agent-client" - agentClientSecret = "test-audit-agent-secret" - delegatedUserSub = "audit-delegated-user-sub" - ) - - agentClient, err := registration.New(registration.Config{ - ID: agentClientID, - Secret: agentClientSecret, - Public: false, - GrantTypes: []string{oauthproto.GrantTypeTokenExchange}, - Scopes: registration.DefaultScopes, - Audience: []string{testAudience}, - }) - require.NoError(t, err) - - m := startMockOIDC(t) - ts := setupTestServerWithMockOIDC(t, m, withExtraClient(agentClient)) - - // Mint the subject token (user) signed by the server's key. - signer, err := jose.NewSigner( - jose.SigningKey{Algorithm: jose.RS256, Key: ts.PrivateKey}, - (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "test-key"), - ) - require.NoError(t, err) - - now := time.Now() - subjectToken, err := jwt.Signed(signer). - Claims(jwt.Claims{ - Issuer: testIssuer, - Subject: delegatedUserSub, - Audience: jwt.Audience{testAudience}, - Expiry: jwt.NewNumericDate(now.Add(30 * time.Minute)), - IssuedAt: jwt.NewNumericDate(now), - }). - Claims(map[string]any{ - "client_id": agentClientID, - }). - Serialize() - require.NoError(t, err) - - // Exchange it for a delegated token carrying act.sub = agent. - resp := makeTokenRequest(t, ts.Server.URL, url.Values{ - "grant_type": {oauthproto.GrantTypeTokenExchange}, - "subject_token": {subjectToken}, - "subject_token_type": {oauthproto.TokenTypeAccessToken}, - "client_id": {agentClientID}, - "client_secret": {agentClientSecret}, - }) - defer resp.Body.Close() - body := parseTokenResponse(t, resp) - require.Equal(t, http.StatusOK, resp.StatusCode, - "token exchange should succeed, got %d (body: %v)", resp.StatusCode, body) - delegated, ok := body["access_token"].(string) - require.True(t, ok, "access_token should be a string") - require.NotEmpty(t, delegated) - - // Build a validator that trusts the server's issuer and keys, exactly like - // the middleware in front of a protected resource server. The in-process - // key provider avoids self-referential HTTP JWKS fetches. - validator, err := auth.NewTokenValidator(context.Background(), auth.TokenValidatorConfig{ - Issuer: testIssuer, - Audience: testAudience, - }, auth.WithKeyProvider(&testKeyProvider{key: ts.PrivateKey})) - require.NoError(t, err) - - // Chain: audit (outer) -> auth -> bare stub handler. The stub does NOT - // re-publish the identity: this test pins that TokenValidator.Middleware - // alone publishes the validated identity to the audit holder. - auditLog := filepath.Join(t.TempDir(), "audit.log") - auditor, err := audit.NewAuditorWithTransport(&audit.Config{LogFile: auditLog}, "streamable-http") - require.NoError(t, err) - defer auditor.Close() - - stub := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - }) - - req := httptest.NewRequest(http.MethodPost, "/messages", strings.NewReader("{}")) - req.Header.Set("Authorization", "Bearer "+delegated) - rr := httptest.NewRecorder() - auditor.Middleware(validator.Middleware(stub)).ServeHTTP(rr, req) - - require.Equal(t, http.StatusOK, rr.Code, - "delegated token must validate through the auth middleware") - - // Exactly one audit event, carrying the delegation chain. - event := readSingleAuditEvent(t, auditLog) - - subjects, ok := event["subjects"].(map[string]any) - require.True(t, ok, "subjects should be a map") - assert.Equal(t, delegatedUserSub, subjects["user_id"], - "audit subject must be the delegated user, not the agent") - - chain, ok := event["delegation_chain"].(map[string]any) - require.True(t, ok, "delegation_chain must be present in the audit event") - assert.Equal(t, false, chain["truncated"]) - actors, ok := chain["actors"].([]any) - require.True(t, ok, "actors should be an array") - require.Len(t, actors, 1, "a single exchange yields a single actor") - actor, ok := actors[0].(map[string]any) - require.True(t, ok) - assert.Equal(t, agentClientID, actor["sub"], - "the delegation chain must record the acting agent client") -} - -// TestIntegration_AuditMiddleware_NonDelegatedTokenOmitsDelegationChain is the -// negative companion to TestIntegration_TokenExchange_AuditDelegationChain: a -// plain (non-delegated) token through the same audit(auth(stub)) chain must -// produce an audit event with NO delegation_chain member. -func TestIntegration_AuditMiddleware_NonDelegatedTokenOmitsDelegationChain(t *testing.T) { - t.Parallel() - - const plainUserSub = "audit-plain-user-sub" - - m := startMockOIDC(t) - ts := setupTestServerWithMockOIDC(t, m) - - // Mint a plain subject token signed by the server's key (no act claim). - signer, err := jose.NewSigner( - jose.SigningKey{Algorithm: jose.RS256, Key: ts.PrivateKey}, - (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "test-key"), - ) - require.NoError(t, err) - - now := time.Now() - plainToken, err := jwt.Signed(signer). - Claims(jwt.Claims{ - Issuer: testIssuer, - Subject: plainUserSub, - Audience: jwt.Audience{testAudience}, - Expiry: jwt.NewNumericDate(now.Add(30 * time.Minute)), - IssuedAt: jwt.NewNumericDate(now), - }). - Serialize() - require.NoError(t, err) - - validator, err := auth.NewTokenValidator(context.Background(), auth.TokenValidatorConfig{ - Issuer: testIssuer, - Audience: testAudience, - }, auth.WithKeyProvider(&testKeyProvider{key: ts.PrivateKey})) - require.NoError(t, err) - - auditLog := filepath.Join(t.TempDir(), "audit.log") - auditor, err := audit.NewAuditorWithTransport(&audit.Config{LogFile: auditLog}, "streamable-http") - require.NoError(t, err) - defer auditor.Close() - - stub := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - }) - - req := httptest.NewRequest(http.MethodPost, "/messages", strings.NewReader("{}")) - req.Header.Set("Authorization", "Bearer "+plainToken) - rr := httptest.NewRecorder() - auditor.Middleware(validator.Middleware(stub)).ServeHTTP(rr, req) - - require.Equal(t, http.StatusOK, rr.Code, - "plain token must validate through the auth middleware") - - event := readSingleAuditEvent(t, auditLog) - - subjects, ok := event["subjects"].(map[string]any) - require.True(t, ok, "subjects should be a map") - assert.Equal(t, plainUserSub, subjects["user_id"], - "audit subject must be the authenticated user") - - _, exists := event["delegation_chain"] - assert.False(t, exists, - "a non-delegated token must not produce a delegation_chain member") -} - // ============================================================================ // Full PKCE Flow Integration Tests with Mock Upstream IDP (using mockoidc) // ============================================================================ From 9888d01111c6c3cd0ab5120da1cd476cd27d4fec Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 10:45:55 +0200 Subject: [PATCH 5/9] Pin the emitted delegation chain wire contract docs/middleware.md publishes delegation_chain.{actors[].sub, actors[].act_claims, truncated, dropped_count} as the audit log contract, but most of it was asserted only as Go struct fields, so a struct-tag rename would break every downstream consumer with a green suite. Assert the emitted JSON on the logAuditEvent path, including that dropped_count is omitted when nothing was dropped. Every nested act chain in the suite was also hand-built, so handler.go's act["act"] = priorAct was never executed by a test and the outermost-first ordering the audit record depends on rested on fixtures the tests wrote themselves. Drive a real exchange with a may_act-bearing subject token (RFC 8693 4.4, which this server honors but cannot yet issue) so the handler produces the outer hop itself, and assert the ordering end to end. Also add the missing negative cases on both the POST and stream-open paths, and a direct test for MaxDelegationDepthOrDefault. Co-Authored-By: Claude Opus 5 --- pkg/audit/auditor_test.go | 159 +++++++++++++++++- pkg/audit/config_test.go | 27 +++ .../integration_delegation_audit_test.go | 127 +++++++++++++- 3 files changed, 308 insertions(+), 5 deletions(-) diff --git a/pkg/audit/auditor_test.go b/pkg/audit/auditor_test.go index 44f26cf33d..a5318d32aa 100644 --- a/pkg/audit/auditor_test.go +++ b/pkg/audit/auditor_test.go @@ -1303,7 +1303,14 @@ func TestAuditLoggerLevelFormat(t *testing.T) { // buffer, for tests asserting on emitted events. func newBufferAuditor(t *testing.T) (*Auditor, *bytes.Buffer) { t.Helper() - auditor, err := NewAuditorWithTransport(&Config{Component: "test"}, "streamable-http") + return newBufferAuditorWithConfig(t, &Config{Component: "test"}) +} + +// newBufferAuditorWithConfig is newBufferAuditor for tests that need a +// non-default Config (e.g. MaxDelegationDepth). +func newBufferAuditorWithConfig(t *testing.T, cfg *Config) (*Auditor, *bytes.Buffer) { + t.Helper() + auditor, err := NewAuditorWithTransport(cfg, "streamable-http") require.NoError(t, err) var logBuf bytes.Buffer auditor.auditLogger = NewAuditLogger(&logBuf) @@ -1413,6 +1420,122 @@ func TestMiddlewareAuditsInnerChainOutcomes(t *testing.T) { }) } +// TestLogAuditEventDelegationChain pins the emitted delegation_chain shape on +// the primary logAuditEvent path (every ordinary POST). The SSE path already +// has an equivalent pin (see "delegated token stream open carries the +// delegation chain" in TestStreamOpenAuditEvents) and workflow auditing has +// its own (workflow_auditor_test.go), but logAuditEvent had none, so a +// pkg/audit regression here was only ever caught by running pkg/authserver's +// suite. What this test uniquely pins beyond those two siblings is that the +// POST path honours Config.MaxDelegationDepth via +// Config.MaxDelegationDepthOrDefault at this specific call site. +func TestLogAuditEventDelegationChain(t *testing.T) { + t.Parallel() + + // twoHopIdentity builds an auth.Identity carrying a hand-built two-actor + // RFC 8693 chain, outermost (most recent) first: agent-2 delegated from + // agent-1, with agent-1's act claim also carrying an extra "iss" member to + // pin the documented act_claims shape. + twoHopIdentity := func() *auth.Identity { + act := map[string]any{ + "sub": "agent-2", + "act": map[string]any{"sub": "agent-1", "iss": "https://issuer.example"}, + } + chain := auth.ParseDelegationChain(act, auth.DefaultMaxDelegationDepth) + return &auth.Identity{PrincipalInfo: auth.PrincipalInfo{ + Subject: "user-123", + Claims: map[string]any{"act": act}, + DelegationChain: &chain, + }} + } + + t.Run("full documented shape: two actors, one carrying act_claims", func(t *testing.T) { + t.Parallel() + auditor, logBuf := newBufferAuditor(t) + identity := twoHopIdentity() + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = auth.WithIdentity(r.Context(), identity) + w.WriteHeader(http.StatusOK) + }) + auditor.Middleware(handler).ServeHTTP(httptest.NewRecorder(), newToolsCallRequest()) + + events := decodeAuditEvents(t, logBuf) + require.Len(t, events, 1) + + chain, ok := events[0]["delegation_chain"].(map[string]any) + require.True(t, ok, "the POST/logAuditEvent path must carry the delegation chain") + assert.Equal(t, false, chain["truncated"]) + _, hasDropped := chain["dropped_count"] + assert.False(t, hasDropped, "dropped_count must be omitted when nothing was dropped") + + actors, ok := chain["actors"].([]any) + require.True(t, ok, "actors should be an array") + require.Len(t, actors, 2) + + outer, ok := actors[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, "agent-2", outer["sub"], "actors[0] must be the outermost/most recent actor") + _, hasClaims := outer["act_claims"] + assert.False(t, hasClaims, "the outer actor here carries no extra act members") + + inner, ok := actors[1].(map[string]any) + require.True(t, ok) + assert.Equal(t, "agent-1", inner["sub"]) + actClaims, ok := inner["act_claims"].(map[string]any) + require.True(t, ok, "the documented act_claims member must surface extra act-claim data") + assert.Equal(t, "https://issuer.example", actClaims["iss"]) + }) + + t.Run("MaxDelegationDepth truncates the chain and keeps the outermost actor", func(t *testing.T) { + t.Parallel() + maxDepth := 1 + auditor, logBuf := newBufferAuditorWithConfig(t, &Config{Component: "test", MaxDelegationDepth: &maxDepth}) + identity := twoHopIdentity() + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = auth.WithIdentity(r.Context(), identity) + w.WriteHeader(http.StatusOK) + }) + auditor.Middleware(handler).ServeHTTP(httptest.NewRecorder(), newToolsCallRequest()) + + events := decodeAuditEvents(t, logBuf) + require.Len(t, events, 1) + + chain, ok := events[0]["delegation_chain"].(map[string]any) + require.True(t, ok) + assert.Equal(t, true, chain["truncated"]) + assert.Equal(t, float64(1), chain["dropped_count"], + "one of the two actors must be reported dropped") + + actors, ok := chain["actors"].([]any) + require.True(t, ok) + require.Len(t, actors, 1, "MaxDelegationDepth=1 must leave exactly one surviving actor") + surviving, ok := actors[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, "agent-2", surviving["sub"], "the surviving actor must be the outermost one") + }) + + t.Run("plain identity omits the delegation chain", func(t *testing.T) { + t.Parallel() + auditor, logBuf := newBufferAuditor(t) + identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{Subject: "user-123"}} + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = auth.WithIdentity(r.Context(), identity) + w.WriteHeader(http.StatusOK) + }) + auditor.Middleware(handler).ServeHTTP(httptest.NewRecorder(), newToolsCallRequest()) + + events := decodeAuditEvents(t, logBuf) + require.Len(t, events, 1) + + _, exists := events[0]["delegation_chain"] + assert.False(t, exists, + "a non-delegated identity must not produce a delegation_chain member on the POST path") + }) +} + // TestStreamOpenAuditEvents pins the deferred stream-open logging: the // connection event for SSE / streamable GET requests is logged on the FIRST // response write, so it reflects the real outcome and the identity attached by @@ -1538,7 +1661,9 @@ func TestStreamOpenAuditEvents(t *testing.T) { } handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Emulate inner auth attaching a delegated identity before the stream starts. + // Emulate inner auth attaching the identity before the stream starts. + // The returned context is intentionally discarded: only WithIdentity's + // side effect of filling the audit-injected IdentityHolder matters here. _ = auth.WithIdentity(r.Context(), delegated) w.Header().Set("Content-Type", "text/event-stream") w.WriteHeader(http.StatusOK) @@ -1563,4 +1688,34 @@ func TestStreamOpenAuditEvents(t *testing.T) { require.True(t, ok) assert.Equal(t, "agent-2", second["sub"]) }) + + t.Run("non-delegated identity stream open omits the delegation chain", func(t *testing.T) { + t.Parallel() + auditor, logBuf := newBufferAuditor(t) + + plain := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{Subject: "user-123"}} + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Emulate inner auth attaching the identity before the stream starts. + // The returned context is intentionally discarded: only WithIdentity's + // side effect of filling the audit-injected IdentityHolder matters here. + _ = auth.WithIdentity(r.Context(), plain) + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("event: message\ndata: {}\n\n")) + }) + auditor.Middleware(handler).ServeHTTP(httptest.NewRecorder(), newStreamRequest()) + + events := decodeAuditEvents(t, logBuf) + require.Len(t, events, 1) + assert.Equal(t, EventTypeSSEConnection, events[0]["type"]) + + subjects, ok := events[0]["subjects"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "user-123", subjects[SubjectKeyUserID], "the identity must have landed") + + _, exists := events[0]["delegation_chain"] + assert.False(t, exists, + "an authenticated non-delegated identity must not produce a delegation_chain member on the stream-open path") + }) } diff --git a/pkg/audit/config_test.go b/pkg/audit/config_test.go index 2ec0a477d4..5a072544f9 100644 --- a/pkg/audit/config_test.go +++ b/pkg/audit/config_test.go @@ -16,6 +16,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/auth" ) func TestDefaultConfig(t *testing.T) { @@ -447,6 +449,31 @@ func TestGetLogWriter_WithActualFile(t *testing.T) { }) } +func TestMaxDelegationDepthOrDefault(t *testing.T) { + t.Parallel() + + depth := func(d int) *int { return &d } + + tests := []struct { + name string + cfg *Config + want int + }{ + {name: "nil receiver", cfg: nil, want: auth.DefaultMaxDelegationDepth}, + {name: "nil MaxDelegationDepth field", cfg: &Config{}, want: auth.DefaultMaxDelegationDepth}, + {name: "zero MaxDelegationDepth", cfg: &Config{MaxDelegationDepth: depth(0)}, want: auth.DefaultMaxDelegationDepth}, + {name: "negative MaxDelegationDepth", cfg: &Config{MaxDelegationDepth: depth(-3)}, want: auth.DefaultMaxDelegationDepth}, + {name: "positive MaxDelegationDepth is honored", cfg: &Config{MaxDelegationDepth: depth(5)}, want: 5}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, tt.cfg.MaxDelegationDepthOrDefault()) + }) + } +} + // waitForAuditLog polls the audit log file until content is available or timeout is reached. // This is more reliable than a fixed sleep for async log writes. func waitForAuditLog(t *testing.T, logFilePath string, timeout time.Duration) []byte { diff --git a/pkg/authserver/integration_delegation_audit_test.go b/pkg/authserver/integration_delegation_audit_test.go index efb5af6fd6..abad93fcaf 100644 --- a/pkg/authserver/integration_delegation_audit_test.go +++ b/pkg/authserver/integration_delegation_audit_test.go @@ -103,6 +103,17 @@ func exchangeForDelegatedToken(t *testing.T, serverURL, clientID, clientSecret, // to the audit holder via the IdentityHolder read-back. func auditEventForToken(t *testing.T, key *rsa.PrivateKey, token, validationMsg string) map[string]any { t.Helper() + return auditEventForTokenWithConfig(t, key, token, validationMsg, audit.Config{}) +} + +// auditEventForTokenWithConfig is auditEventForToken for tests that need a +// non-default audit.Config (e.g. MaxDelegationDepth). cfg is taken by value +// and its LogFile field is set on the local copy, so the caller's config is +// never mutated. +func auditEventForTokenWithConfig( + t *testing.T, key *rsa.PrivateKey, token, validationMsg string, cfg audit.Config, +) map[string]any { + t.Helper() // Build a validator that trusts the server's issuer and keys, exactly like // the middleware in front of a protected resource server. The in-process @@ -113,8 +124,8 @@ func auditEventForToken(t *testing.T, key *rsa.PrivateKey, token, validationMsg }, auth.WithKeyProvider(&testKeyProvider{key: key})) require.NoError(t, err) - auditLog := filepath.Join(t.TempDir(), "audit.log") - auditor, err := audit.NewAuditorWithTransport(&audit.Config{LogFile: auditLog}, "streamable-http") + cfg.LogFile = filepath.Join(t.TempDir(), "audit.log") + auditor, err := audit.NewAuditorWithTransport(&cfg, "streamable-http") require.NoError(t, err) t.Cleanup(func() { assert.NoError(t, auditor.Close()) @@ -131,7 +142,7 @@ func auditEventForToken(t *testing.T, key *rsa.PrivateKey, token, validationMsg require.Equal(t, http.StatusOK, rr.Code, validationMsg) - return readSingleAuditEvent(t, auditLog) + return readSingleAuditEvent(t, cfg.LogFile) } // TestIntegration_TokenExchange_AuditDelegationChain proves the end-to-end @@ -216,3 +227,113 @@ func TestIntegration_AuditMiddleware_NonDelegatedTokenOmitsDelegationChain(t *te assert.False(t, exists, "a non-delegated token must not produce a delegation_chain member") } + +// mintTwoHopDelegatedToken produces a token whose second hop is genuine +// production output of a real RFC 8693 token-exchange call — firstAgentClientID +// "delegates" to secondAgentClientID — while the first hop is a hand-signed +// stand-in for an earlier exchange this test never performs. What this pins +// is the handler's nesting direction: the second (real) actor must land at +// actors[0], outermost, with the hand-signed first actor at actors[1]. +// +// This AS can honour an RFC 8693 §4.4 may_act claim — checkDelegationConsent +// (handler.go) treats it as the authoritative consent signal and skips the +// client_id binding entirely when present — but no code path anywhere writes +// may_act, and the exchange does not propagate an inbound one: +// pkg/authserver/server/tokenexchange/validator.go:276-281 routes may_act +// into the structured MayAct field so it never lands in Extra, and +// handler.go:160 only ever copies Extra["act"]. So the subject token here +// hand-signs the one claim a may_act-emitting issuer would have written +// (plus a prior "act" entry standing in for that earlier hop); everything +// downstream of it — the consent check, the act-claim nesting, the issued +// token — is genuine production output of a single real exchange. +func mintTwoHopDelegatedToken(t *testing.T) (ts *testServerWithUpstream, delegated, firstAgentClientID, secondAgentClientID string) { + t.Helper() + + const ( + firstAgent = "test-first-agent-client" + secondAgent = "test-second-agent-client" + secondAgentSecret = "test-second-agent-secret" //nolint:gosec // test fixture, not a real credential + delegatedUserSub = "audit-multi-actor-user-sub" + ) + + secondAgentClient, err := registration.New(registration.Config{ + ID: secondAgent, + Secret: secondAgentSecret, + Public: false, + GrantTypes: []string{oauthproto.GrantTypeTokenExchange}, + Scopes: registration.DefaultScopes, + Audience: []string{testAudience}, + }) + require.NoError(t, err) + + m := startMockOIDC(t) + ts = setupTestServerWithMockOIDC(t, m, withExtraClient(secondAgentClient)) + + subjectToken := signTestJWT(t, ts.PrivateKey, delegatedUserSub, map[string]any{ + "client_id": firstAgent, // ignored: may_act wins + "act": map[string]any{"sub": firstAgent}, // prior hop + "may_act": map[string]any{"sub": secondAgent}, // RFC 8693 §4.4 consent + }) + + delegated = exchangeForDelegatedToken(t, ts.Server.URL, secondAgent, secondAgentSecret, subjectToken) + return ts, delegated, firstAgent, secondAgent +} + +// TestIntegration_TokenExchange_MultiActorDelegationChain proves the +// handler's nesting direction end to end: hop 2 (secondAgentClientID) is +// real handler output from an actual token-exchange call, hop 1 +// (firstAgentClientID) is the hand-signed stand-in described on +// mintTwoHopDelegatedToken, and the audit event for the resulting token +// orders the two actors outermost (most recent) first. +func TestIntegration_TokenExchange_MultiActorDelegationChain(t *testing.T) { + t.Parallel() + + ts, delegated, firstAgentClientID, secondAgentClientID := mintTwoHopDelegatedToken(t) + + event := auditEventForToken(t, ts.PrivateKey, delegated, + "re-delegated token must validate through the auth middleware") + + chain, ok := event["delegation_chain"].(map[string]any) + require.True(t, ok, "delegation_chain must be present in the audit event") + assert.Equal(t, false, chain["truncated"]) + actors, ok := chain["actors"].([]any) + require.True(t, ok, "actors should be an array") + require.Len(t, actors, 2, "a re-delegated token yields two actors") + + actor0, ok := actors[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, secondAgentClientID, actor0["sub"], + "actors[0] (outermost) must be the client that performed the second exchange") + actor1, ok := actors[1].(map[string]any) + require.True(t, ok) + assert.Equal(t, firstAgentClientID, actor1["sub"], + "actors[1] (innermost) must be the earlier, first-hop actor") +} + +// TestIntegration_TokenExchange_TruncatedDelegationChainAuditEvent proves +// that an auditor configured with MaxDelegationDepth: 1 truncates a genuine +// 2-actor delegation chain down to its outermost (most recent) actor. +func TestIntegration_TokenExchange_TruncatedDelegationChainAuditEvent(t *testing.T) { + t.Parallel() + + ts, delegated, _, secondAgentClientID := mintTwoHopDelegatedToken(t) + + maxDepth := 1 + event := auditEventForTokenWithConfig(t, ts.PrivateKey, delegated, + "re-delegated token must validate through the auth middleware", + audit.Config{MaxDelegationDepth: &maxDepth}) + + chain, ok := event["delegation_chain"].(map[string]any) + require.True(t, ok, "delegation_chain must be present in the audit event") + assert.Equal(t, true, chain["truncated"]) + assert.Equal(t, float64(1), chain["dropped_count"], + "one of the two actors must be reported dropped") + + actors, ok := chain["actors"].([]any) + require.True(t, ok, "actors should be an array") + require.Len(t, actors, 1, "MaxDelegationDepth=1 must leave exactly one surviving actor") + actor0, ok := actors[0].(map[string]any) + require.True(t, ok) + assert.Equal(t, secondAgentClientID, actor0["sub"], + "the surviving actor must be the outermost one") +} From b4abcaf0e74de388e7ad47469c55f619ac570590 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 11:13:50 +0200 Subject: [PATCH 6/9] Harden act claim parsing against malformed input Three ways the parser lost forensic evidence on a non-conformant act claim, all reachable only from a trusted issuer since validation runs first, but all silent: A non-string "sub" vanished entirely -- it was captured only when it type-asserted to string, yet dropped from the claims copy regardless, so {"sub": 123} recorded an anonymous actor with no trace an identifier had been present. Keep it under act_claims instead. RFC 8693 4.1 requires act to be a JSON object. When it was not, the record was indistinguishable from a token with no delegation at all: a non-object at the top produced no delegation_chain member, and a non-object nested deeper reported truncated=false, asserting completeness while discarding an unreadable remainder. Report it as malformed, and admit an actorless-but-malformed chain through the guards that previously required at least one actor. act_claims also bypassed internalClaims, which keeps tsid out of external sinks -- Identity.MarshalJSON redacts it from top-level claims but marshals the delegation chain verbatim. Reuse the same list so the filter cannot drift. Actors is now always a non-nil slice: emitting an actorless chain would otherwise have marshalled "actors": null, a shape no consumer could see before this change. Co-Authored-By: Claude Opus 5 --- docs/middleware.md | 16 ++++- pkg/audit/auditor.go | 5 +- pkg/audit/auditor_test.go | 68 +++++++++++++++++++++ pkg/auth/context.go | 2 +- pkg/auth/context_test.go | 6 +- pkg/auth/delegation.go | 41 ++++++++++--- pkg/auth/delegation_test.go | 115 ++++++++++++++++++++++++++++++++++++ 7 files changed, 238 insertions(+), 15 deletions(-) diff --git a/docs/middleware.md b/docs/middleware.md index 573bc7808b..08314f3000 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -527,14 +527,26 @@ Audit events are logged as structured JSON objects: - `delegation_chain`: RFC 8693 delegation chain, present only when the caller authenticated with a delegated token (i.e. the JWT carries an `act` claim) - `actors`: Acting parties, outermost (most recent) first — `actors[0]` is - the direct delegate that presented the token + the direct delegate that presented the token. Empty array (not omitted) + when `malformed` is `true` and no actor could be parsed at all - `sub`: Acting party identifier (from the `act` claim's `sub` member) - - `act_claims`: Additional `act` claim members (e.g. `iss`), when present + - `act_claims`: Additional `act` claim members (e.g. `iss`), when present. + May also carry `sub` when the token's `act.sub` was present but not a + string (RFC 7519 §4.1.2 requires a string); such a value is + deliberately not reported as the actor's `sub` and must not be treated + as an actor identifier - `truncated`: `true` when the chain exceeded the configured maximum depth (`maxDelegationDepth` in the audit config, default 10) and trailing actors were dropped - `dropped_count`: Number of actors dropped due to the depth cap, present only when `truncated` is `true` + - `malformed`: `true` when the token's `act` claim (at the top level or at + some nesting depth) was present and non-null but not a JSON object, per + RFC 8693 §4.1; present only when `true`. Actors already parsed from + shallower levels of the chain (if any) are still reported alongside it. + When the malformed value sits immediately past the depth cap, + `dropped_count` is deliberately 0 and `truncated` is `false` — unreadable + data past the cap is not a countable actor - `target`: Information about the operation target - `endpoint`: HTTP endpoint path - `method`: HTTP method diff --git a/pkg/audit/auditor.go b/pkg/audit/auditor.go index 3528691bad..330fb42e60 100644 --- a/pkg/audit/auditor.go +++ b/pkg/audit/auditor.go @@ -564,11 +564,12 @@ func extractDelegationChainFromIdentity(identity *auth.Identity, maxDepth int) * } if rawAct != nil && (chain == nil || len(chain.Actors) == 0 || (chain.Truncated && maxDepth > len(chain.Actors))) { - if parsed := auth.ParseDelegationChain(rawAct, maxDepth); len(parsed.Actors) > 0 && len(parsed.Actors) >= existingActors { + if parsed := auth.ParseDelegationChain(rawAct, maxDepth); (len(parsed.Actors) > 0 || parsed.Malformed) && + len(parsed.Actors) >= existingActors { return &parsed } } - if chain != nil && len(chain.Actors) > 0 { + if chain != nil && (len(chain.Actors) > 0 || chain.Malformed) { return rebindDelegationChain(chain, maxDepth) } return nil diff --git a/pkg/audit/auditor_test.go b/pkg/audit/auditor_test.go index a5318d32aa..d1a883a7b4 100644 --- a/pkg/audit/auditor_test.go +++ b/pkg/audit/auditor_test.go @@ -739,6 +739,46 @@ func TestExtractDelegationChainFromIdentity(t *testing.T) { assert.Nil(t, extractDelegationChainFromIdentity(identity, auth.DefaultMaxDelegationDepth)) }) + t.Run("malformed act with zero actors is not discarded (re-parse guard)", func(t *testing.T) { + t.Parallel() + // No pre-parsed DelegationChain, so the ONLY path that can produce a + // result is the raw-act re-parse guard + // (auth.ParseDelegationChain(rawAct, maxDepth)); the trailing + // "chain != nil" branch is unreachable here (chain is nil), so this + // isolates the re-parse guard's own Malformed handling. + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "user123", + Claims: map[string]any{"act": "not-an-object"}, + }, + } + chain := extractDelegationChainFromIdentity(identity, auth.DefaultMaxDelegationDepth) + require.NotNil(t, chain, "a malformed-but-actorless chain must still surface, not be swallowed by the len(Actors) > 0 guards") + assert.True(t, chain.Malformed) + assert.Empty(t, chain.Actors) + }) + + t.Run("malformed act with zero actors is not discarded (trailing raw-claim branch)", func(t *testing.T) { + t.Parallel() + // No raw "act" claim is present (e.g. Claims not carrying the raw + // value, or already stripped), so the re-parse guard's rawAct != nil + // check is skipped entirely: this exercises the OTHER guard, the + // trailing "chain != nil && ..." branch that rebinds an + // already-parsed chain. + identity := &auth.Identity{ + PrincipalInfo: auth.PrincipalInfo{ + Subject: "user123", + DelegationChain: &auth.DelegationChain{ + Malformed: true, + }, + }, + } + chain := extractDelegationChainFromIdentity(identity, auth.DefaultMaxDelegationDepth) + require.NotNil(t, chain, "a malformed-but-actorless chain must still surface via the trailing rebind branch") + assert.True(t, chain.Malformed) + assert.Empty(t, chain.Actors) + }) + // The following two cases share an identity shaped exactly like what // auth.claimsToIdentity produces for a 14-hop token: the identity-layer // parse always caps at auth.DefaultMaxDelegationDepth (10), so @@ -1534,6 +1574,34 @@ func TestLogAuditEventDelegationChain(t *testing.T) { assert.False(t, exists, "a non-delegated identity must not produce a delegation_chain member on the POST path") }) + + t.Run("malformed act claim is pinned as malformed:true with zero actors", func(t *testing.T) { + t.Parallel() + auditor, logBuf := newBufferAuditor(t) + act := "not-an-object" + chain := auth.ParseDelegationChain(act, auth.DefaultMaxDelegationDepth) + identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{ + Subject: "user-123", + Claims: map[string]any{"act": act}, + DelegationChain: &chain, + }} + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = auth.WithIdentity(r.Context(), identity) + w.WriteHeader(http.StatusOK) + }) + auditor.Middleware(handler).ServeHTTP(httptest.NewRecorder(), newToolsCallRequest()) + + events := decodeAuditEvents(t, logBuf) + require.Len(t, events, 1) + + logged, ok := events[0]["delegation_chain"].(map[string]any) + require.True(t, ok, "a malformed act claim must still produce a delegation_chain member") + assert.Equal(t, true, logged["malformed"]) + actors, ok := logged["actors"].([]any) + require.True(t, ok, "actors must marshal as a JSON array, not null") + assert.Len(t, actors, 0) + }) } // TestStreamOpenAuditEvents pins the deferred stream-open logging: the diff --git a/pkg/auth/context.go b/pkg/auth/context.go index 6c3bddf8ed..927ec15176 100644 --- a/pkg/auth/context.go +++ b/pkg/auth/context.go @@ -208,7 +208,7 @@ func claimsToIdentity(claims jwt.MapClaims, token string) (*Identity, error) { // parties acting on behalf of the subject. if act, ok := claims["act"]; ok && act != nil { chain := ParseDelegationChain(act, DefaultMaxDelegationDepth) - if len(chain.Actors) > 0 { + if len(chain.Actors) > 0 || chain.Malformed { identity.DelegationChain = &chain } } diff --git a/pkg/auth/context_test.go b/pkg/auth/context_test.go index 8f920b406d..6fabf5b435 100644 --- a/pkg/auth/context_test.go +++ b/pkg/auth/context_test.go @@ -168,11 +168,13 @@ func TestClaimsToIdentity_ParsesActClaim(t *testing.T) { assert.Nil(t, id.DelegationChain) }) - t.Run("non-map act claim", func(t *testing.T) { + t.Run("non-map act claim is surfaced as malformed, not silently dropped", func(t *testing.T) { t.Parallel() id, err := claimsToIdentity(jwt.MapClaims{"sub": "user123", "act": "agent-1"}, "tok") require.NoError(t, err) - assert.Nil(t, id.DelegationChain) + require.NotNil(t, id.DelegationChain, "a malformed act must still produce a chain so the issue is visible") + assert.True(t, id.DelegationChain.Malformed) + assert.Empty(t, id.DelegationChain.Actors) }) } diff --git a/pkg/auth/delegation.go b/pkg/auth/delegation.go index 6e97b9d920..63f367b1f0 100644 --- a/pkg/auth/delegation.go +++ b/pkg/auth/delegation.go @@ -3,6 +3,8 @@ package auth +import "slices" + // DefaultMaxDelegationDepth is the default cap on how many nested RFC 8693 // "act" entries ParseDelegationChain will follow. It is aligned with the // issuance-side cap in pkg/authserver/server/tokenexchange/handler.go @@ -14,13 +16,18 @@ const DefaultMaxDelegationDepth = 10 // chain (one "act" claim value). type DelegatedActor struct { // Subject identifies the acting party (the "sub" member of the act claim). - // Empty when the act claim carries no "sub". + // Empty both when the act claim carries no "sub" and when "sub" is present + // but not a string; the latter case is distinguishable from the former by + // its raw value surfacing under Claims instead of vanishing. Subject string `json:"sub,omitempty"` // Claims holds the remaining members of the act claim (e.g. "iss", // "client_id"), preserving forward-compatibility with act claims that // carry more than a subject. The "act_claims" tag disambiguates these - // per-actor members from the identity's top-level claims. + // per-actor members from the identity's top-level claims. A non-string + // "sub" is included here rather than dropped, so a malformed subject is + // still visible. Internal claims (see internalClaims in context.go) are + // filtered out the same as they are from top-level Identity.Claims. Claims map[string]any `json:"act_claims,omitempty"` } @@ -39,33 +46,47 @@ type DelegationChain struct { // DroppedCount reports how many actors were dropped due to the depth cap. // Zero (and omitted from JSON) when nothing was dropped. DroppedCount int `json:"dropped_count,omitempty"` + + // Malformed is true when a present, non-nil "act" value (top-level or + // nested) was not a JSON object, per RFC 8693 §4.1. This is distinct from + // a legitimately absent/null "act": that case leaves Malformed false, with + // or without actors already parsed from shallower levels of the chain. + Malformed bool `json:"malformed,omitempty"` } // ParseDelegationChain parses a JWT "act" claim value into a DelegationChain. // The act value is expected to be a map (as produced by JSON decoding); any -// other shape yields an empty, non-truncated chain. Nested "act" members are -// followed up to maxDepth actors; maxDepth <= 0 uses DefaultMaxDelegationDepth. -// A nil/absent act value yields the zero chain (no actors, not truncated). +// other non-nil shape yields Malformed=true, with whatever actors were parsed +// from shallower levels preserved. Nested "act" members are followed up to +// maxDepth actors; maxDepth <= 0 uses DefaultMaxDelegationDepth. A nil/absent +// act value yields the zero chain (no actors, not truncated, not malformed). func ParseDelegationChain(act any, maxDepth int) DelegationChain { if maxDepth <= 0 { maxDepth = DefaultMaxDelegationDepth } - chain := DelegationChain{} + chain := DelegationChain{Actors: []DelegatedActor{}} current := act for len(chain.Actors) < maxDepth { m, ok := current.(map[string]any) if !ok { + // A nil current here means act was absent/null (either at the + // top level, or the legitimate end of a chain checked below) — + // not malformed. Anything else present but not an object is. + if current != nil { + chain.Malformed = true + } return chain } actor := DelegatedActor{} - if sub, ok := m["sub"].(string); ok { + sub, subIsString := m["sub"].(string) + if subIsString { actor.Subject = sub } claims := make(map[string]any, len(m)) for k, v := range m { - if k == "sub" || k == "act" { + if k == "act" || (k == "sub" && subIsString) || slices.Contains(internalClaims, k) { continue } claims[k] = v @@ -88,6 +109,10 @@ func ParseDelegationChain(act any, maxDepth int) DelegationChain { for { m, ok := current.(map[string]any) if !ok { + // current is guaranteed non-nil here (the loop above only + // advances current to a checked, present, non-nil "act" value), + // so a failed assertion means unparseable data beyond the cap. + chain.Malformed = true break } chain.Truncated = true diff --git a/pkg/auth/delegation_test.go b/pkg/auth/delegation_test.go index a1d303ded1..7e458ddd4f 100644 --- a/pkg/auth/delegation_test.go +++ b/pkg/auth/delegation_test.go @@ -132,6 +132,121 @@ func TestParseDelegationChain(t *testing.T) { } } +// TestParseDelegationChain_Malformed pins RFC 8693 §4.1 non-object "act" +// handling: a malformed act must be distinguishable from a legitimately +// absent/ended one, and must never be conflated with it. +func TestParseDelegationChain_Malformed(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + act any + maxDepth int + wantSubjects []string + wantMalformed bool + }{ + { + name: "absent act is not malformed", + act: nil, + maxDepth: DefaultMaxDelegationDepth, + wantSubjects: nil, + wantMalformed: false, + }, + { + name: "top-level non-object act is malformed with zero actors", + act: "agent-1", + maxDepth: DefaultMaxDelegationDepth, + wantSubjects: nil, + wantMalformed: true, + }, + { + name: "top-level array act is malformed with zero actors", + act: []any{"agent-1"}, + maxDepth: DefaultMaxDelegationDepth, + wantSubjects: nil, + wantMalformed: true, + }, + { + name: "nested non-object act is malformed with actors parsed so far", + act: map[string]any{"sub": "agent-1", "act": "junk"}, + maxDepth: DefaultMaxDelegationDepth, + wantSubjects: []string{"agent-1"}, + wantMalformed: true, + }, + { + name: "non-object act exactly at the depth cap is malformed", + act: map[string]any{ + "sub": "agent-1", + "act": map[string]any{"sub": "agent-2", "act": "junk"}, + }, + maxDepth: 2, + wantSubjects: []string{"agent-1", "agent-2"}, + wantMalformed: true, + }, + { + name: "legitimately ended chain (no act member) is not malformed", + act: map[string]any{"sub": "agent-1"}, + maxDepth: DefaultMaxDelegationDepth, + wantSubjects: []string{"agent-1"}, + wantMalformed: false, + }, + { + name: "legitimately ended chain (explicit null act) is not malformed", + act: map[string]any{"sub": "agent-1", "act": nil}, + maxDepth: DefaultMaxDelegationDepth, + wantSubjects: []string{"agent-1"}, + wantMalformed: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + chain := ParseDelegationChain(tt.act, tt.maxDepth) + + assert.Equal(t, tt.wantSubjects, actorSubjects(chain)) + assert.Equal(t, tt.wantMalformed, chain.Malformed) + }) + } +} + +// TestParseDelegationChain_NonStringSub pins that a non-string "sub" is +// preserved under act_claims instead of silently vanishing: an actor whose +// identifier was the wrong JSON type must stay distinguishable from one with +// no identifier at all. +func TestParseDelegationChain_NonStringSub(t *testing.T) { + t.Parallel() + + chain := ParseDelegationChain(map[string]any{"sub": float64(123)}, DefaultMaxDelegationDepth) + + require.Len(t, chain.Actors, 1) + assert.Empty(t, chain.Actors[0].Subject, "a non-string sub must not populate Subject") + assert.Equal(t, map[string]any{"sub": float64(123)}, chain.Actors[0].Claims, + "a non-string sub must be preserved under act_claims, not dropped") +} + +// TestParseDelegationChain_FiltersInternalClaims pins that act_claims goes +// through the same internal-claim filter as the identity's top-level claims, +// so credential-adjacent claims (e.g. "tsid") never reach an external sink +// through this path even if act construction grows to mirror subject-token +// claims in the future. +func TestParseDelegationChain_FiltersInternalClaims(t *testing.T) { + t.Parallel() + + act := map[string]any{ + "sub": "agent-1", + "tsid": "session-secret", + "iss": "https://issuer.example", + } + + chain := ParseDelegationChain(act, DefaultMaxDelegationDepth) + + require.Len(t, chain.Actors, 1) + assert.Equal(t, map[string]any{"iss": "https://issuer.example"}, chain.Actors[0].Claims, + "tsid must be filtered out of act_claims the same as top-level Claims") +} + func TestParseDelegationChain_ActorClaims(t *testing.T) { t.Parallel() From 947b4463cc8e86a2fb62efc37417251347c3863f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 13:13:37 +0200 Subject: [PATCH 7/9] Correct delegation docs and RFC citations The delegation chain also reaches admission webhook receivers, because GetPrincipalInfo copies the embedded PrincipalInfo that now carries it -- a user-facing payload change documented nowhere. Note it where the webhook middleware is described. The comment introducing the parse told authorizers to consume the full chain, which RFC 8693 4.1 forbids: an authorizer must consider only the current actor, and prior actors are informational. Nothing reads the chain for authz today, so this only removes a future trap. The architecture doc named the Go struct tag rather than the emitted log key operators actually see. may_act is RFC 8693 4.4, not 4.1; the delegation-consent code cited the act claim's section in four places, and a test added earlier on this branch already cited 4.4 correctly. Co-Authored-By: Claude Opus 5 --- docs/arch/02-core-concepts.md | 2 +- docs/middleware.md | 2 ++ pkg/auth/context.go | 7 ++++--- pkg/authserver/integration_test.go | 2 +- pkg/authserver/server/tokenexchange/handler.go | 2 +- pkg/authserver/server/tokenexchange/validator.go | 4 ++-- 6 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/arch/02-core-concepts.md b/docs/arch/02-core-concepts.md index 8ec9677ddc..6d3f1d564c 100644 --- a/docs/arch/02-core-concepts.md +++ b/docs/arch/02-core-concepts.md @@ -659,7 +659,7 @@ See `pkg/audit/mcp_events.go` for complete list of event types. **Event data:** - Timestamp, source, outcome - Subjects (user, session) -- Delegation chain (`delegationChain`) — RFC 8693 `act` claim actors, outermost +- Delegation chain (`delegation_chain`) — RFC 8693 `act` claim actors, outermost first, when the caller used a delegated token - Target (endpoint, method, resource) - Request/response data (configurable) diff --git a/docs/middleware.md b/docs/middleware.md index 08314f3000..523008ec48 100644 --- a/docs/middleware.md +++ b/docs/middleware.md @@ -45,6 +45,8 @@ Multiple webhook definitions of the same type run in configuration order. When m Configuration files may be written in YAML or JSON. Duration values such as `timeout` accept strings like `5s`, and omitted timeouts default to `10s`. +When the caller authenticated with an RFC 8693 delegated token, the request payload sent to webhook receivers includes a `delegation_chain` field on the principal object, with the same shape as the `delegation_chain` field documented under [Audit Middleware](#9-audit-middleware) below. + Example: ```bash diff --git a/pkg/auth/context.go b/pkg/auth/context.go index 927ec15176..85e1546652 100644 --- a/pkg/auth/context.go +++ b/pkg/auth/context.go @@ -203,9 +203,10 @@ func claimsToIdentity(claims jwt.MapClaims, token string) (*Identity, error) { identity.Email = email } - // Parse the RFC 8693 "act" claim (if present) into a delegation chain so - // downstream consumers (audit logs, authorizers) see the full chain of - // parties acting on behalf of the subject. + // Parse the RFC 8693 "act" claim (if present) into a delegation chain. + // This is primarily for audit: per RFC 8693 §4.1, an authorizer applying + // access control MUST consider only the current actor (chain.Actors[0]); + // any nested actors in chain.Actors[1:] are informational only. if act, ok := claims["act"]; ok && act != nil { chain := ParseDelegationChain(act, DefaultMaxDelegationDepth) if len(chain.Actors) > 0 || chain.Malformed { diff --git a/pkg/authserver/integration_test.go b/pkg/authserver/integration_test.go index 0c5f20e214..014954ac2a 100644 --- a/pkg/authserver/integration_test.go +++ b/pkg/authserver/integration_test.go @@ -711,7 +711,7 @@ func TestIntegration_TokenExchange_ConfidentialClientHappyPath(t *testing.T) { // Mint a subject token signed by the server's own key (the validator verifies // against the server's JWKS). client_id must equal the acting client so the - // RFC 8693 §4.1 delegation-consent check (client_id binding) passes. + // RFC 8693 §4.4 delegation-consent check (client_id binding) passes. signer, err := jose.NewSigner( jose.SigningKey{Algorithm: jose.RS256, Key: ts.PrivateKey}, (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "test-key"), diff --git a/pkg/authserver/server/tokenexchange/handler.go b/pkg/authserver/server/tokenexchange/handler.go index a7c0b614ce..35678284f6 100644 --- a/pkg/authserver/server/tokenexchange/handler.go +++ b/pkg/authserver/server/tokenexchange/handler.go @@ -284,7 +284,7 @@ func validateExchangeParams(form url.Values) (string, error) { return subjectToken, nil } -// checkDelegationConsent enforces the RFC 8693 §4.1 delegation consent check. +// checkDelegationConsent enforces the RFC 8693 §4.4 delegation consent check. // // If the subject token carries a may_act claim, it is the authoritative // consent signal: only the party named in may_act.sub may delegate. The diff --git a/pkg/authserver/server/tokenexchange/validator.go b/pkg/authserver/server/tokenexchange/validator.go index 07a0822043..a1a3d6bd76 100644 --- a/pkg/authserver/server/tokenexchange/validator.go +++ b/pkg/authserver/server/tokenexchange/validator.go @@ -53,14 +53,14 @@ type ValidatedClaims struct { // Scopes is the space-delimited scope string from the "scope" claim. // Empty if the subject token carries no scope claim. Scopes string - // MayAct holds the authorized actor from the "may_act" claim (RFC 8693 §4.1). + // MayAct holds the authorized actor from the "may_act" claim (RFC 8693 §4.4). // Nil when the subject token does not carry a may_act claim. MayAct *MayActClaim // Extra contains all non-standard claims not captured by other fields. Extra map[string]any } -// MayActClaim represents the RFC 8693 §4.1 may_act claim from a subject token. +// MayActClaim represents the RFC 8693 §4.4 may_act claim from a subject token. // It identifies the party authorized to act on behalf of the subject. type MayActClaim struct { Sub string `json:"sub"` From cf158449ce48cb542dcec018a0a08337672a25b5 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 13:19:53 +0200 Subject: [PATCH 8/9] Reject a non-positive maxDelegationDepth maxDataSize is rejected when negative, but maxDelegationDepth was accepted at any value and silently became 10 via the defaulting accessor, so a typo degraded audit coverage instead of failing. The kubebuilder Minimum is what rejects it at admission for VirtualMCPServer; the Validate check catches the CLI and config-file paths, which have no admission layer, and fires at vMCP startup as defense in depth. A nil pointer stays valid -- that is the unset case the accessor and the CRD default already cover -- so this rejects rather than repairs. Co-Authored-By: Claude Opus 5 --- ...olhive.stacklok.dev_virtualmcpservers.yaml | 2 ++ ...olhive.stacklok.dev_virtualmcpservers.yaml | 2 ++ docs/operator/crd-api.md | 2 +- docs/server/docs.go | 2 +- docs/server/swagger.json | 2 +- docs/server/swagger.yaml | 1 + pkg/audit/config.go | 5 +++ pkg/audit/config_test.go | 31 +++++++++++++++++++ 8 files changed, 44 insertions(+), 3 deletions(-) diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index 4dade447ac..8ab14082ab 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1190,6 +1190,7 @@ spec: MaxDelegationDepth caps how many nested RFC 8693 "act" entries are recorded in an audit event's delegationChain. Deeper chains are truncated (marked with truncated=true). Defaults to 10 when unset. + minimum: 1 type: integer type: object backends: @@ -4644,6 +4645,7 @@ spec: MaxDelegationDepth caps how many nested RFC 8693 "act" entries are recorded in an audit event's delegationChain. Deeper chains are truncated (marked with truncated=true). Defaults to 10 when unset. + minimum: 1 type: integer type: object backends: diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index e708851674..5cdbfe4061 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1193,6 +1193,7 @@ spec: MaxDelegationDepth caps how many nested RFC 8693 "act" entries are recorded in an audit event's delegationChain. Deeper chains are truncated (marked with truncated=true). Defaults to 10 when unset. + minimum: 1 type: integer type: object backends: @@ -4647,6 +4648,7 @@ spec: MaxDelegationDepth caps how many nested RFC 8693 "act" entries are recorded in an audit event's delegationChain. Deeper chains are truncated (marked with truncated=true). Defaults to 10 when unset. + minimum: 1 type: integer type: object backends: diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 7103df642d..49e7f5f52e 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -42,7 +42,7 @@ _Appears in:_ | `includeResponseData` _boolean_ | IncludeResponseData determines whether to include response data in audit logs. | false | Optional: \{\}
| | `detectApplicationErrors` _boolean_ | DetectApplicationErrors controls whether the audit middleware inspects
JSON-RPC response bodies for application-level errors when the HTTP
status code indicates success (2xx). When enabled, a small prefix of
the response body is buffered to detect JSON-RPC error fields,
independent of the IncludeResponseData setting. | true | Optional: \{\}
| | `maxDataSize` _integer_ | MaxDataSize limits the size of request/response data included in audit logs (in bytes). | 1024 | Optional: \{\}
| -| `maxDelegationDepth` _integer_ | MaxDelegationDepth caps how many nested RFC 8693 "act" entries are
recorded in an audit event's delegationChain. Deeper chains are
truncated (marked with truncated=true). Defaults to 10 when unset. | 10 | Optional: \{\}
| +| `maxDelegationDepth` _integer_ | MaxDelegationDepth caps how many nested RFC 8693 "act" entries are
recorded in an audit event's delegationChain. Deeper chains are
truncated (marked with truncated=true). Defaults to 10 when unset. | 10 | Minimum: 1
Optional: \{\}
| | `logFile` _string_ | LogFile specifies the file path for audit logs. If empty, logs to stdout. | | Optional: \{\}
| diff --git a/docs/server/docs.go b/docs/server/docs.go index f1a127a2b1..7158b77090 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -220,7 +220,7 @@ const docTemplate = `{ "type": "integer" }, "maxDelegationDepth": { - "description": "MaxDelegationDepth caps how many nested RFC 8693 \"act\" entries are\nrecorded in an audit event's delegationChain. Deeper chains are\ntruncated (marked with truncated=true). Defaults to 10 when unset.\n+kubebuilder:default=10\n+optional", + "description": "MaxDelegationDepth caps how many nested RFC 8693 \"act\" entries are\nrecorded in an audit event's delegationChain. Deeper chains are\ntruncated (marked with truncated=true). Defaults to 10 when unset.\n+kubebuilder:validation:Minimum=1\n+kubebuilder:default=10\n+optional", "type": "integer" } }, diff --git a/docs/server/swagger.json b/docs/server/swagger.json index b16b3af016..acfe5ba218 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -213,7 +213,7 @@ "type": "integer" }, "maxDelegationDepth": { - "description": "MaxDelegationDepth caps how many nested RFC 8693 \"act\" entries are\nrecorded in an audit event's delegationChain. Deeper chains are\ntruncated (marked with truncated=true). Defaults to 10 when unset.\n+kubebuilder:default=10\n+optional", + "description": "MaxDelegationDepth caps how many nested RFC 8693 \"act\" entries are\nrecorded in an audit event's delegationChain. Deeper chains are\ntruncated (marked with truncated=true). Defaults to 10 when unset.\n+kubebuilder:validation:Minimum=1\n+kubebuilder:default=10\n+optional", "type": "integer" } }, diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 35b73f19c2..42103ee47d 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -227,6 +227,7 @@ components: MaxDelegationDepth caps how many nested RFC 8693 "act" entries are recorded in an audit event's delegationChain. Deeper chains are truncated (marked with truncated=true). Defaults to 10 when unset. + +kubebuilder:validation:Minimum=1 +kubebuilder:default=10 +optional type: integer diff --git a/pkg/audit/config.go b/pkg/audit/config.go index 7520e406f1..deea901066 100644 --- a/pkg/audit/config.go +++ b/pkg/audit/config.go @@ -57,6 +57,7 @@ type Config struct { // MaxDelegationDepth caps how many nested RFC 8693 "act" entries are // recorded in an audit event's delegationChain. Deeper chains are // truncated (marked with truncated=true). Defaults to 10 when unset. + // +kubebuilder:validation:Minimum=1 // +kubebuilder:default=10 // +optional MaxDelegationDepth *int `json:"maxDelegationDepth,omitempty" yaml:"maxDelegationDepth,omitempty"` @@ -176,6 +177,10 @@ func (c *Config) Validate() error { return fmt.Errorf("maxDataSize cannot be negative") } + if c.MaxDelegationDepth != nil && *c.MaxDelegationDepth <= 0 { + return fmt.Errorf("maxDelegationDepth must be positive") + } + // Validate event types (basic validation - could be extended) validEventTypes := map[string]bool{ EventTypeMCPInitialize: true, diff --git a/pkg/audit/config_test.go b/pkg/audit/config_test.go index 5a072544f9..cdd9064666 100644 --- a/pkg/audit/config_test.go +++ b/pkg/audit/config_test.go @@ -449,6 +449,37 @@ func TestGetLogWriter_WithActualFile(t *testing.T) { }) } +func TestValidateMaxDelegationDepth(t *testing.T) { + t.Parallel() + + depth := func(d int) *int { return &d } + + tests := []struct { + name string + value *int + wantErr bool + }{ + {name: "nil is valid", value: nil, wantErr: false}, + {name: "positive is valid", value: depth(5), wantErr: false}, + {name: "zero is rejected", value: depth(0), wantErr: true}, + {name: "negative is rejected", value: depth(-3), wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + config := &Config{MaxDelegationDepth: tt.value} + err := config.Validate() + if tt.wantErr { + assert.Error(t, err) + assert.Contains(t, err.Error(), "maxDelegationDepth must be positive") + } else { + assert.NoError(t, err) + } + }) + } +} + func TestMaxDelegationDepthOrDefault(t *testing.T) { t.Parallel() From fdaa9a8a7d302504d36c428c1b472aec8d561b96 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 28 Jul 2026 13:59:33 +0200 Subject: [PATCH 9/9] Extract parseActor from ParseDelegationChain The malformed, non-string-sub and internal-claim guards took the function to cyclomatic complexity 18, over the configured limit of 15. Moving the per-actor construction out verbatim brings it to 11 and leaves the parser as its two loops plus the malformed bookkeeping. No behavior change; also fixes a codespell hit on a comment. Co-Authored-By: Claude Opus 5 --- pkg/auth/delegation.go | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/pkg/auth/delegation.go b/pkg/auth/delegation.go index 63f367b1f0..49cbf0472a 100644 --- a/pkg/auth/delegation.go +++ b/pkg/auth/delegation.go @@ -79,22 +79,7 @@ func ParseDelegationChain(act any, maxDepth int) DelegationChain { return chain } - actor := DelegatedActor{} - sub, subIsString := m["sub"].(string) - if subIsString { - actor.Subject = sub - } - claims := make(map[string]any, len(m)) - for k, v := range m { - if k == "act" || (k == "sub" && subIsString) || slices.Contains(internalClaims, k) { - continue - } - claims[k] = v - } - if len(claims) > 0 { - actor.Claims = claims - } - chain.Actors = append(chain.Actors, actor) + chain.Actors = append(chain.Actors, parseActor(m)) next, ok := m["act"] if !ok || next == nil { @@ -111,7 +96,7 @@ func ParseDelegationChain(act any, maxDepth int) DelegationChain { if !ok { // current is guaranteed non-nil here (the loop above only // advances current to a checked, present, non-nil "act" value), - // so a failed assertion means unparseable data beyond the cap. + // so a failed assertion means unparsable data beyond the cap. chain.Malformed = true break } @@ -126,3 +111,26 @@ func ParseDelegationChain(act any, maxDepth int) DelegationChain { return chain } + +// parseActor builds one DelegatedActor from a decoded "act" claim object: the +// "sub" member (if a string) becomes Subject, and every other member except +// "act" and internalClaims (see context.go) is copied into Claims. A +// non-string "sub" is preserved under Claims rather than dropped. +func parseActor(m map[string]any) DelegatedActor { + actor := DelegatedActor{} + sub, subIsString := m["sub"].(string) + if subIsString { + actor.Subject = sub + } + claims := make(map[string]any, len(m)) + for k, v := range m { + if k == "act" || (k == "sub" && subIsString) || slices.Contains(internalClaims, k) { + continue + } + claims[k] = v + } + if len(claims) > 0 { + actor.Claims = claims + } + return actor +}