Skip to content

Fix review findings in the act claim audit capture - #6096

Merged
jhrozek merged 9 commits into
mainfrom
audit-delegation-review-fixes
Jul 28, 2026
Merged

Fix review findings in the act claim audit capture#6096
jhrozek merged 9 commits into
mainfrom
audit-delegation-review-fixes

Conversation

@jhrozek

@jhrozek jhrozek commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

#6046 landed RFC 8693 act claim capture in audit events. A review of it afterwards surfaced eleven findings, of which the substantive ones were: a config knob that could not do what it advertised, exported test-only API on a production type, a repeated call that six of eight sites could have dropped silently, a published wire contract asserted only as Go struct fields, and three ways the parser lost forensic evidence on malformed input. This fixes all of them.

Closes nothing on its own — this is follow-up on #6035 / #6046.

  • maxDelegationDepth above 10 was silently ignored. claimsToIdentity parses at the hardcoded DefaultMaxDelegationDepth and the audit layer could only shrink that result, so an operator setting 25 got 10 actors and no warning while the full chain sat unread in Claims["act"]. Re-parse the raw claim at the configured depth, guarded so it can only widen, never narrow.
  • SetLogWriterForTest was exported, immediately deprecated, and needed two //nolint:staticcheck. Config.LogFile already does the job; the integration tests now point it at a t.TempDir() file.
  • attachDelegation is hand-repeated in all eight WorkflowAuditor.Log* methods, but only two were tested — the six uncovered ones included every terminal failed/timed-out event. Table-driven over all eight now.
  • The emitted wire contract was unpinned. docs/middleware.md publishes delegation_chain.{actors[].sub, actors[].act_claims, truncated, dropped_count}, but most of it was asserted only as Go struct fields, so a struct-tag rename would break every log consumer with a green suite.
  • Every nested act chain in the suite was 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. A real exchange now drives it via a may_act-bearing subject token (RFC 8693 §4.4, which this server honors but cannot yet issue).
  • Parser hardening: a non-string act.sub vanished entirely; a malformed act was indistinguishable from an absent one; act_claims bypassed the internalClaims filter that keeps tsid out of external sinks — which matters because Identity.MarshalJSON redacts tsid from top-level claims but marshals the delegation chain verbatim to webhook receivers.
  • maxDelegationDepth had no lower bound while its sibling maxDataSize rejects negatives.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Changes

File Change
pkg/audit/auditor.go Re-parse the raw act claim at the configured depth; delete SetLogWriterForTest
pkg/auth/delegation.go Preserve a non-string sub; add Malformed; filter internalClaims; never emit "actors": null
pkg/auth/context.go Admit an actorless-but-malformed chain; scope the authz comment to RFC 8693 §4.1
pkg/audit/config.go Reject a non-positive maxDelegationDepth (Minimum=1 + Validate)
pkg/authserver/server/tokenexchange/{handler,validator}.go may_act is §4.4, not §4.1 (comments only)
pkg/authserver/integration_delegation_audit_test.go New: delegation audit tests + helpers, incl. the real multi-actor exchange
docs/middleware.md, docs/arch/02-core-concepts.md Document malformed, the webhook payload change, and the correct log key
deploy/charts/operator-crds/**, docs/operator/crd-api.md, docs/server/** Regenerated for Minimum=1

Test plan

  • Unit tests
  • Manual verification: each fix was mutation-tested — the production behavior was broken, the new test confirmed failing, then restored. Includes dropping attachDelegation from each of the eight Log* methods individually (clean 1:1 failure mapping) and inverting act["act"] = priorAct to confirm the multi-hop ordering assertion bites.

Does this introduce a user-facing change?

Yes, three:

  1. delegation_chain gains a malformed member for an act claim that is present but not a JSON object (RFC 8693 §4.1 requires an object).
  2. delegation_chain.actors is now always an array — an actorless malformed chain would otherwise have emitted "actors": null, a shape no consumer could see before.
  3. maxDelegationDepth above 10 now takes effect; values below 1 are rejected at admission rather than silently becoming 10.

Also worth calling out: PrincipalInfo gained delegation_chain in #6046, so admission webhook receivers already see it. That was undocumented; this PR documents it.

Special notes for reviewers

Overlap with toolhive-core#196. That PR implements the same feature for the shared library and diverges on every JSON key (delegation vs delegationChain, chain vs actors, omitted vs dropped_count), on the default depth (16 vs 10), and on whether per-hop claims are serialized at all. That divergence predates this PR — the field names all landed on main with #6046. Since core#81 graduated the audit package out of toolhive, whichever schema wins there eventually replaces ours, so it needs settling; I have commented on core#196.

The one divergence this PR adds is malformed: core#196 handles the same input by hard-erroring (ErrNotJSONObject) where we record a flag and keep the partial chain. Worth deciding which behavior we want before either is depended on.

Follow-ups filed rather than folded in: #6093 (the auth server can itself mint a §4.1-violating nested act, which is how malformed becomes reachable from our own issuance path), #6094 (WorkflowAuditor leaks the audit log fd), #6095 (extract the eight-way repeated event prologue — the root cause the new test table only guards against).

Generated with Claude Code

jhrozek and others added 8 commits July 28, 2026 13:21
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
JAORMX
JAORMX previously approved these changes Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.35%. Comparing base (92c70db) to head (fdaa9a8).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6096      +/-   ##
==========================================
+ Coverage   72.31%   72.35%   +0.04%     
==========================================
  Files         730      731       +1     
  Lines       75630    75759     +129     
==========================================
+ Hits        54689    54813     +124     
- Misses      17035    17045      +10     
+ Partials     3906     3901       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Jul 28, 2026
@jhrozek
jhrozek merged commit cd373ec into main Jul 28, 2026
49 checks passed
@jhrozek
jhrozek deleted the audit-delegation-review-fixes branch July 28, 2026 12:23
JAORMX added a commit that referenced this pull request Jul 28, 2026
Replaces toolhive's local delegation-chain types with the canonical
schema from toolhive-core v0.0.35 (stacklok/toolhive-core#196):
pkg/auth/delegation.go deleted in favor of toolhive-core/audit,
unified `delegation` wire field across audit events, Identity, and
PrincipalInfo (was delegationChain/delegation_chain/actors), typed
{iss,sub} hops with act_claims serialization removed, record-and-flag
malformed handling with malformedReason, and slog output mirroring the
JSON shape. Keeps the toolhive mint cap at 10 (core's 16 is a parse
ceiling). Builds on the hardening from #6096; non-schema parts kept.

Closes the toolhive side of #6035.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants