Skip to content

Adopt toolhive-core canonical delegation-chain schema - #6107

Merged
JAORMX merged 1 commit into
mainfrom
adopt-core-delegation-schema
Jul 28, 2026
Merged

Adopt toolhive-core canonical delegation-chain schema#6107
JAORMX merged 1 commit into
mainfrom
adopt-core-delegation-schema

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adopts the canonical RFC 8693 delegation-chain schema that landed in toolhive-core (toolhive-core#196) as the single wire contract for delegation capture, replacing the schema toolhive grew in #6035 / #6046 and hardened in #6096. The reviewer of both efforts agreed the core schema — decided on the merits of a standards/production survey — is the one to converge on, so this PR deletes toolhive's local parser and types (pkg/auth/delegation.go) and imports github.com/stacklok/toolhive-core/audit everywhere instead.

Wire contract change (old → new)

Old (toolhive #6046/#6096) New (toolhive-core canonical) Notes
delegationChain (audit event) / delegation_chain (slog, webhook PrincipalInfo) — three spellings delegation everywhere JSON, slog, and webhook payloads are now shape-identical under one key
actors chain Always a JSON array, never null (pinned by test); outermost-first ordering unchanged — chain[0] is the current actor and is stable under truncation
per-actor sub per-hop sub unchanged semantics; omitted when absent/non-string
per-actor act_claims (filtered map of extra members) removed Core never serializes per-hop extras (PII guard: unexported, Extra() copy accessor, LogValuer + Stringer sealed). The per-hop tsid filtering from #6096 is moot and deleted; top-level claim filtering is untouched
iss buried in act_claims per-hop iss (typed, promoted) (iss, sub) is the stable actor identifier per OIDC Core §5.7
dropped_count (omitempty) omitted (always present) counts well-formed hops dropped by the depth cap
truncated truncated (always present) truncation now keeps the outermost hops in core too (same as before)
malformed (omitempty, bool only) malformed (always present) + malformedReason enum act_not_object, nested_act_not_object, iss_not_string, sub_not_string — low-cardinality, safe as a metric dimension

truncated / omitted / malformed deliberately have no omitempty: an incomplete or non-conformant chain is never silently indistinguishable from a complete, well-formed one.

Breaking change: admission-webhook payload

PrincipalInfo (sent to admission-webhook receivers) and Identity.MarshalJSON both carried the chain since #6046 — under delegation_chain and delegationChain respectively. Both are now delegation with the core shape above. Webhook receivers that read the old keys must migrate.

Behavioral notes

  • auth.ParseDelegationChain is gone; claimsToIdentity parses via coreaudit.ParseDelegationChain, which never fails — a malformed act claim can never fail authentication, only be recorded (malformed + malformedReason). Hops parsed before the violation are preserved.
  • auth.DefaultMaxDelegationDepth stays 10 (toolhive's minting cap, aligned with the token-exchange handler and Cedar nesting caps), deliberately below core's parse ceiling of 16: parsing at the mint cap preserves the signal that a truncated:true chain cannot have come from toolhive's own issuance path. Config.MaxDelegationDepthOrDefault keeps defaulting to 10 and the kubebuilder default=10 annotation is unchanged; the distinction is documented in code.
  • Fix review findings in the act claim audit capture #6096's widen-only re-parse guard (re-parse the raw act at the configured depth when it could widen an identity-layer-truncated chain, never narrow it) is preserved, simplified around the no-error parse.
  • All ten emitter call sites (2 in auditor.go, 8 WorkflowAuditor.Log* methods) keep working; Fix review findings in the act claim audit capture #6096's table-driven eight-way coverage is retained and asserts the new keys from emitted JSON, as does the end-to-end token-exchange → middleware → audit test driving a real nested act via may_act.

Relationship to #6096

#6096 merged earlier today; this PR is based on main including it. Its non-schema hardening (widen-only depth guard, SetLogWriterForTest removal, maxDelegationDepth validation, table-driven workflow-emitter tests, real multi-hop exchange test) is kept; its schema-level surface (actors/act_claims/dropped_count/bool-only malformed) is superseded by the core contract. The one divergence #6096 flagged for settling — record-and-flag vs hard-error on malformed input — was settled in core in favor of record-and-flag, which is what ships here.

Dependency pin

  • github.com/stacklok/toolhive-core is pinned to the released v0.0.35 (includes the canonical schema from core#196 and core's go-sdk v1.7.0 bump from core#200). No re-pin step is pending.
  • The bump transitively raises github.com/google/cel-go v0.29.2 → v0.30.0 (required by core; core made that move in Update module github.com/google/cel-go to v0.30.0 toolhive-core#191) and github.com/modelcontextprotocol/go-sdk v1.7.0-pre.3 → v1.7.0 final (indirect in this module; propagated via MVS from core).

Docs

  • docs/middleware.md: delegation example and field descriptions rewritten to the new wire shape (act_claims is gone entirely; iss is now a typed per-hop field; malformedReason documented).
  • docs/arch/02-core-concepts.md: log key updated.
  • Regenerated: swagger (docs/server/*), CRDs (deploy/charts/operator-crds/**), docs/operator/crd-api.md (comment-text only; no schema change to the CRD).

Recommended companion follow-up (not in this PR)

#6093 — the auth server can itself mint a §4.1-violating nested act (its actChainDepth returns 0 for a non-map prior act, so a malformed inbound act is copied through instead of rejected). With this PR that token is recorded as malformed; the fix belongs at mint time (reject with invalid_grant). Kept separate deliberately: issuance-path enforcement vs audit-side capture.

Type of change

  • Breaking change (wire-contract rename for audit/webhook consumers)

Test plan

  • golangci-lint run ./... + go vet ./... clean
  • go test -race across the full unit suite (incl. pkg/auth, pkg/audit, pkg/authserver integration delegation tests)
  • Wire shape asserted from marshaled JSON bytes (not Go struct fields): delegation.{chain[].iss,chain[].sub,truncated,omitted,malformed,malformedReason}, "chain":[] pinned never-null
  • task license-check

🤖 Generated with Claude Code

Replace toolhive's local RFC 8693 delegation capture (pkg/auth/delegation.go,
landed in #6046 and hardened in #6096) with the canonical schema from
toolhive-core v0.0.35 (stacklok/toolhive-core#196).

- Delete pkg/auth/delegation.go; parse via toolhive-core/audit's
  ParseDelegationChain, which never fails: malformed act claims are recorded
  (malformed + malformedReason) instead of dropped or erroring, and can never
  fail authentication.
- Unify the wire key to `delegation` across audit events, slog output,
  Identity.MarshalJSON, and the admission-webhook PrincipalInfo payload
  (previously delegationChain / delegation_chain). Hops live under `chain`
  (always an array), with promoted per-hop iss/sub; act_claims is gone (core
  never serializes per-hop extras — PII guard), dropped_count becomes
  `omitted`, and truncated/omitted/malformed are always present.
- Keep auth.DefaultMaxDelegationDepth = 10 (toolhive's minting cap) rather
  than core's 16 parse ceiling, preserving the truncated=true ⇒ foreign-token
  signal; Config.MaxDelegationDepthOrDefault documents the distinction.
- Preserve #6096's widen-only re-parse guard, simplified around the no-error
  parse; all ten emitter call sites unchanged.
- Update all delegation tests to assert the emitted JSON keys and rewrite the
  docs (middleware.md, 02-core-concepts.md); regenerate swagger, CRDs, and
  crd-api.md for the comment-text change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JAORMX
JAORMX marked this pull request as ready for review July 28, 2026 14:14
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label 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.36%. Comparing base (6d6e9f9) to head (c7f60a1).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6107      +/-   ##
==========================================
- Coverage   72.38%   72.36%   -0.02%     
==========================================
  Files         733      733              
  Lines       75804    75784      -20     
==========================================
- Hits        54872    54844      -28     
- Misses      17033    17038       +5     
- Partials     3899     3902       +3     

☔ 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.

@JAORMX
JAORMX merged commit 2858ac8 into main Jul 28, 2026
81 of 84 checks passed
@JAORMX
JAORMX deleted the adopt-core-delegation-schema branch July 28, 2026 15:02
JAORMX pushed a commit that referenced this pull request Jul 28, 2026
* Reject malformed prior act claims at token exchange

RFC 8693 §4.1 requires the act claim to be a JSON object, but the token
exchange handler copied a subject token's prior act into the newly minted
token without checking its shape. The depth gate did not catch it either:
actChainDepth returned 0 for any non-map, making "absent" and "malformed"
indistinguishable, so a subject token carrying act: "some-agent" produced
a minted token asserting a delegation chain we ourselves would flag as
non-conformant on the consuming side.

Reuse the shared audit-side parser adopted in #6107 rather than teaching
the bespoke walker about shapes: it reports the depth and the conformance
verdict in one pass, so the two cannot disagree. The exchange is now
rejected with invalid_grant, consistent with the adjacent depth, consent,
and expiry gates.

The new gate is monotonically stricter than the old one: a well-formed
chain yields len(Chain) == min(depth, cap), so the depth boundary is
unchanged and nothing previously rejected is newly accepted.

Closes #6093

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

* Pin the iss_not_string rejection reason

The gate rejects on every reason ParseDelegationChain reports and surfaces
the reason in the client-visible hint, but only three of the four were
covered. Add the non-string iss case so the fourth reason and its hint are
pinned too.

The hop keeps a valid sub: the parser reports the first violation it finds
and checks iss before sub, so a hop with both non-string would report only
iss_not_string and leave this branch unpinned.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 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.

3 participants