Skip to content

Harden act/may_act handling on the external-issuer token exchange path #6113

Description

@jhrozek

Found while fixing #6093, which closed the emitting half of the act-claim
shape problem. Three adjacent gaps stayed open. All three are gated behind the
same dormant path — NewMultiIssuerTokenValidator has no non-callers today
(factory.go builds NewSelfIssuedTokenValidator only), so none is exploitable
now. All three become reachable the moment the multi-issuer work in #5989 lands,
and the third one cannot be retrofitted afterwards.

1. may_act shape validation is missing on the external-issuer path

This is the direct sibling of #6093, in the opposite direction: #6093 was
ToolHive emitting a non-conformant claim, this is ToolHive accepting one.

SelfIssuedTokenValidator.Validate strictly validates may_act — it must be a
JSON object with a string sub, and a present-but-malformed claim makes the
whole token invalid:

// pkg/authserver/server/tokenexchange/validator.go:157-168
if rawMayAct, ok := extraClaims["may_act"]; ok && rawMayAct != nil {
    m, ok := rawMayAct.(map[string]any)
    if !ok {
        return nil, fmt.Errorf("subject token has malformed 'may_act' claim: expected a JSON object")
    }
    ...

MultiIssuerTokenValidator.validateExternalToken skips it. It calls
buildValidatedClaims directly, and that function has no may_act case:

// pkg/authserver/server/tokenexchange/multi_issuer_validator.go:232
return buildValidatedClaims(standardClaims, extraClaims), nil

So the shape check is per-validator rather than at the shared choke point, and
the path where a foreign may_act can actually arrive is the one without it.

Currently masked, but only incidentally: external tokens carry no client_id,
so they fail closed at checkDelegationConsent before may_act matters. That
is a consequence of an unrelated check, not a guard on this one.

Fix direction: move the may_act shape check into buildValidatedClaims
so both validators inherit it, rather than adding a second copy. Worth checking
at the same time whether any other claim is validated on one path only.

2. ToolHive's own minted act hop carries no iss

// pkg/authserver/server/tokenexchange/handler.go:156
act := map[string]any{"sub": actorID}   // actorID = client.GetID()

RFC 8693 §4.1 notes that "the combination of the two claims iss and sub
might be necessary to uniquely identify an actor"
, and RFC 7519 §4.1.2
requires sub be scoped to its issuer or globally unique. A bare OAuth
client_id is neither on its own.

Today every hop in any chain we mint is ours, so a bare sub is unambiguous.
Once #5989 lets a foreign chain nest under our hop, the result is a chain of
bare sub strings drawn from mixed issuer namespaces with nothing recording
which issuer scoped which hop — two different agents named coding-agent at
two different IdPs become indistinguishable in the audit trail.

Fix direction: stamp our own issuer on the hop we mint —
act: {"iss": <as issuer>, "sub": actorID}. Cheap now. Prior hops minted
without iss can never be repaired retroactively, so this is worth doing
before foreign chains start arriving. Note checkDelegationConsent compares
may_act.sub against actorID, so the consent path is unaffected.

3. The act subtree is depth-capped but not size-capped

maxDelegationDepth bounds the chain at 10 hops, but each hop's extra claims
are re-signed verbatim:

// pkg/authserver/server/tokenexchange/handler.go
act["act"] = priorAct

Verbatim nesting is deliberate and should stay — rebuilding from the parsed
chain would silently drop the history trail RFC 8693 §4.1 asks us to preserve,
because toolhive-core keeps each hop's extra claims in an unexported map. The
consequence is that hop count is bounded while hop content is not, so a
complicit or buggy issuer can inflate every token we mint downstream of it.

There is also a data-minimization angle: RFC 8693 §6 asks deployments to
"determine the minimally necessary amount of data and only include such
information in issued tokens"
, and §4.1 notes non-identity claims such as
exp, nbf, and aud are "not meaningful when used within an act claim".
We currently re-sign whatever a prior hop carried, including those. Worth
noting the asymmetry: toolhive-core deliberately keeps hop extras out of logs
and audit records as a PII guard, while we re-sign them into the token — we log
less than we sign.

Fix direction: bound the serialized size of the prior act subtree, and/or
strip the claims §4.1 calls not meaningful. Belongs with the external-token
delegation-consent policy in #5989, which is where the limit would be
configured, since a self-issued chain cannot reach a pathological size.

Why one issue

All three live in the same two files, share the same trigger (a subject token
from a foreign issuer), and would be reviewed together. Splitting them would
mean three PRs touching handler.go and validator.go in sequence.

Priority order if they get split later: (1) is a real validation gap, (2) is
time-sensitive because it cannot be retrofitted, (3) is a hardening limit that
naturally lands with #5989.

Metadata

Metadata

Assignees

No one assigned

    Labels

    authenticationgoPull requests that update go codeneeds-triageIssue needs initial triage by a maintainer

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions