Add a consent model for external OIDC subject tokens - #6149
Open
jhrozek wants to merge 10 commits into
Open
Conversation
Read scopes from the scp array claim as well as the scope string. This server's own tokens carry scp, because that is fosite's default output and nothing overrides it, so a genuine self-issued token arrived with no scopes at all and any scoped exchange failed. Every test missed it by minting subject tokens by hand with a scope claim. Reject a present-but-malformed may_act rather than dropping it. Dropping it let a token meaning "only this party may delegate" fall through to a weaker consent signal, widening consent instead of refusing the token. Where may_act names an issuer, require it to be this server's own: RFC 8693 4.4 notes issuer and subject together may be needed to identify an actor, and without the qualifier a foreign namespace's identifier was compared against a local client id inside a consent decision. Reject a subject token carrying at_hash or c_hash. Those appear only on ID tokens, which are more widely exposed than access tokens and must not serve as a delegation subject. Checking the token type header instead would be impractical, since the providers this targets all emit a bare JWT type. Restrict signature verification to keys whose declared use and algorithm match. On an unknown key id every key was tried, so a key set containing one published for encryption could verify a signature. Carry three fields the external-issuer path sets: the issuer a token came from, the actor whose consent was established, and the clients that issuer permits to delegate. All three are set only by the validator and never populated from token content, so they cannot be spoofed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A trusted issuer plus a matching audience and a valid signature only proves the token was minted for this server to consume. It authorizes the server as a resource, not any particular client as a delegate, so accepting on that basis alone is a confused-deputy risk. The external path now requires an explicit consent signal and fails closed without one. A well-formed may_act is authoritative and the caller enforces it against the authenticated client. Otherwise the issuer's configured actor claim -- azp by default, appid for Entra v1 or cid for Okta -- must match that issuer's allowlist. An empty allowlist therefore accepts only may_act-bearing tokens. An issuer may also name which clients its allowlist permits, and that binding now applies on both consent paths: neither Entra nor Okta can emit may_act, so for those deployments it is the only per-client containment there is. Scheme and network relaxation are configured per issuer. The server-wide insecure-HTTP flag is documented as an in-cluster concern for the server's own issuer, but here it also disabled the key-set HTTPS check, the redirect check and the dial-time private-address guard -- so setting it for the documented reason silently permitted plaintext discovery, letting anyone on-path substitute the key-set URL and thereafter forge subject tokens. Each issuer now carries its own flags, matching how upstream OIDC config already separates them. Each issuer gets its own HTTP client from the shared networking builder, replacing a hand-rolled dialer and redirect check, which gains guards the local version lacked including NAT64 and IPv4-mapped private addresses. The key-set URL check moves to the single point every fetch passes through, so it now also covers hand-configured URLs that previously reached the fetch unchecked. Key sets are cached by the shared JWX cache rather than a hand-rolled TTL, and an unknown key id triggers one rate-limited refresh before giving up. Neither provider guarantees publishing a signing key before using it, and both document immediate rotation, so a fixed expiry meant every token signed with a new key failed until it lapsed. The library also keeps serving the last good key set when a refresh fails, where the previous design made a valid cached set unreachable after any single failure. Retries before a first success are gated, since key resolution precedes signature verification and would otherwise let any client holding the grant drive one outbound fetch per request. Discovery now runs once per issuer instead of on every cache expiry, which was costing dozens of requests an hour against one recommended. Changing an issuer's key-set URL consequently needs a restart, which every other trusted-issuer change already does. A trailing slash is trimmed before appending the well-known path, as OIDC Discovery requires, while the exact match against the configured value keeps comparing it untrimmed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The consent check ended in a case rejecting any subject token with no client id. External tokens have none, so every external token was refused there -- including one that had just satisfied the validator's allowlist, which left the whole policy unreachable. Add a third consent source between may_act and the client-id binding. Its position matters and it is not a fallback for an absent client id: an issuer configured to read the client-id claim as its actor claim populates both, and that value names a client in the external issuer's namespace, so it must never be compared against a local client id the way the self-issued path compares it. Where the issuer names permitted delegate clients, the authenticated client must be among them, on this path and on the may_act path alike. The issued token also records where an external delegation came from. The act chain nests the external issuer, and the actor when the allowlist established consent, which RFC 8693 4.1 anticipates in noting that issuer and subject together may be needed to identify an actor. Nested entries are informational, so this adds provenance without affecting authorization. It is recorded for every external token rather than only the allowlisted one, because the may_act path bypasses the allowlist and so relies on that trail more, not less. The outermost actor stays the local client, and the depth cap accounts for the extra level on top of the prior chain's own depth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Carry the trusted-issuer list from the serializable run config to the resolved one. The validator's own type is reused as the wire type rather than mirrored: every field is operator-facing, all validator-internal state already lives on a private struct that embeds it, and a parallel type would only duplicate the tags it already carries. URL validation runs at both layers, following the precedent the baseline scopes already set. The resolved layer is reached only after storage setup and after live client registration against the upstream, so a malformed issuer caught only there would trigger a network side effect and then crash-loop, orphaning a registration on every restart. The serializable layer runs before any of that, which is where the contract says operator misconfiguration must fail. Validation is split rather than duplicated. This layer owns URL shape, which needs the insecure-HTTP flag; the validator's constructor owns required fields, self-issuer collisions, duplicates and actor-claim reachability, which need the server's own issuer and a cross-entry view. A trailing slash is permitted on a trusted issuer, and the claim that OIDC forbids one was wrong -- the spec instead requires trimming it before appending the well-known path, which only makes sense because such issuers are legal. Rejecting them made Microsoft Entra v1 impossible to configure, since its issuer carries one and the discovery document must match it exactly. The server's own issuer keeps the stricter rule, since that value is ours to choose. A key-set URL is checked as an ordinary endpoint rather than an issuer identifier, since real values carry query strings. The shared networking helpers are unsuitable: they skip the parse and host checks entirely when insecure, and honour an environment variable that would quietly disable an SSRF-relevant scheme check. Requiring an explicit key-set URL alongside private addresses keeps the private target in operator config rather than a discovery document an attacker could influence. A startup warning fires when an issuer's expected audience is absent from the allowed audiences, since that usually means every exchange fails on the requested target -- but a subject token may carry additional audiences, so it cannot be an error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This is the change that makes external subject tokens reachable. Until now the factory built only the self-issued validator, so everything above was inert -- which is why the consent policy lands before it rather than after. When trusted issuers are configured the self-issued validator is wrapped rather than replaced, so self-issued tokens keep taking their own path; with none configured the behaviour and error strings are unchanged, and no background key-set refresh is started. A constructor failure aborts server startup rather than quietly falling back to the self-issued validator, which would silently disable every trusted issuer. The server-wide insecure-HTTP flag is deliberately not forwarded. It reaches this path through no route now, so setting it for its documented in-cluster purpose has no effect on trusted-issuer transport or SSRF posture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cover every branch of the consent policy, the constructor validation that rejects a misconfigured trusted issuer, both configuration layers, the factory's validator selection, and the whole path through a real token endpoint. Each malformed-may_act case carries an allowlisted actor claim, so removing the shape guard turns them into false accepts rather than leaving them green -- the regression these tests exist to catch. The same technique pins the rest: the external-consent case is asserted with a conflicting client id so reordering the switch fails it, the expired case asserts the validator's strict check rather than the leeway- tolerant one, and the allowlist-clone case mutates the caller's slice in place so dropping the copy is observable. Key rotation is exercised end to end: a token signed with a freshly rotated key succeeds immediately rather than after a cache expiry, which is the behaviour the caching rewrite exists to provide and which nothing previously demonstrated. A companion test pins the rate limits, so a burst of unknown key ids triggers one refresh rather than one per request, and repeated failures before any success fetch once rather than per request. The end-to-end cases decode the issued token rather than counting a 200: its issuer is this server's own even though the subject came from elsewhere, and its act claim nests the external issuer beneath the local client. A discovery-hit counter distinguishes the two key-resolution paths, which are otherwise indistinguishable because the test issuer serves the same URL either way, and a direct probe proves the counter is live so the zero-assertions cannot pass vacuously. One case pins the audience constraint an operator meets first: an Entra-style identifier is rejected, because the requested audience is bounded by the subject token's own. The scope case mints its subject token through the real authorization-code grant rather than by hand, since hand-built tokens carry a scope claim this server never issues -- which is what hid the missing scp handling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Describe the trust model, both consent signals, the accepted limitations and the operational behaviour an operator needs before configuring a trusted issuer. It opens by distinguishing this from the outgoing token exchange described in five other architecture documents, since both are called token exchange and readers will otherwise conflate them. It also says plainly that the grant is not yet usable end to end: it needs a confidential client, no supported deployment path creates one, and discovery advertises neither the grant nor secret-based client authentication. That gap predates this work and applies to self-issued subject tokens too, but a reader of an operator-facing document should not have to find it out by trying. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The delegation-consent and provenance work pushed HandleTokenEndpointRequest to cyclomatic complexity 16 (limit 15) by adding five branches: external issuer nesting, the allowlisted-actor sub-check, the prior-act guard, the malformed-chain check, and the depth gate. Extract the whole act-claim construction into buildActClaim rather than raise the threshold — the block is cohesive, independent of the surrounding request plumbing, and two of its branches reject the request outright. Wrap lookupJWKS's signature, which exceeded the 130-column limit. No behaviour change: the extracted body is identical apart from returning (map, error) instead of assigning into the session directly.
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
July 30, 2026 17:29
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6149 +/- ##
==========================================
+ Coverage 72.61% 72.69% +0.08%
==========================================
Files 736 736
Lines 76340 76593 +253
==========================================
+ Hits 55431 55676 +245
- Misses 16973 16998 +25
+ Partials 3936 3919 -17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Codespell rejects "keep-alives", "unparseable" and "disjointness". The last two corrections it proposes are wrong in context — "disjointedness" means incoherence, not the set property — so reword the prose rather than widen the repo-wide ignore list for one PR.
The RunConfig and TrustedIssuer doc comments picked up the #6082 prerequisite note and the AllowedDelegateClients rationale after the last generation, leaving the committed swagger stale.
github-actions
Bot
added
size/XL
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The embedded auth server's RFC 8693 token exchange only accepted subject tokens it had minted itself.
MultiIssuerTokenValidatorcould already verify tokens from trusted external OIDC issuers, but it was dead code —factory.gobuilt onlyNewSelfIssuedTokenValidator. And even wired in, it would not have worked: external tokens carry noclient_idclaim, socheckDelegationConsentrejected every one of them at its terminalClientID == ""branch.The gap was not a bug but a missing authorization decision. "Trusted issuer + valid signature +
aud== us" authorizes ToolHive as a resource; it does not authorize any particular client to act on the subject's behalf. Accepting such a token wholesale is a confused-deputy risk (CWE-863). This PR ships a fail-closed consent policy in the same change that first makes external subject tokens reachable.may_act(RFC 8693 §4.4) stays authoritative when present. Otherwise the per-issuer actor claim (defaultazp;appidfor Entra v1,cidfor Okta) must resolve to a non-empty string present in that issuer'sallowed_actors. An issuer with an emptyallowed_actorsaccepts onlymay_act-bearing tokens. Anything else is rejected.allowed_delegate_clientsnarrows an allowlisted external actor to specific ToolHive clients, applied on both themay_actand allowlist paths — themay_actpath needs it most, since that path bypassesallowed_actorsentirely.ExternalActor,ExternalIssuerandAllowedDelegateClientsonValidatedClaimsare populated only from already-validated issuer config, never from token content, and theactchain now records the originating external issuer so a delegation's origin stays auditable.trusted_issuersis plumbedRunConfig→Config→ factory, validated at both layers so a typo fails before live DCR registration rather than crash-looping afterwards.ValidateJWKSURLchoke point covering configured and discovered URLs, and ajwk.Cache-backed JWKS path with fetch-failure backoff.Closes #5989
Type of change
Test plan
task test)task lint-fix) —0 issuesTwo caveats, stated plainly:
task testcould not report a result on my machine: it pipesgo test -jsonintogotestfmt, and the formatter panicked withBUG: Empty package name encountered, which made the whole pipeline exit 0 while running nothing. I re-ran with the Taskfile's exact flags (-ldflags=-extldflags=-Wl,-w -v -json -race, same e2e exclusions) capturing raw JSON instead. Result: 198 packages pass, 60 skip, 1 fail.pkg/vmcp/server :: TestIntegration_SSEGetConnectionSurvivesWriteTimeout(Post .../mcp: EOFafter 30.07s). This PR touches zero files underpkg/vmcp, and the test passes in isolation (34.8s — longer than the 30s deadline it missed under full parallel-raceload, consistent with a load-sensitive timeout rather than an assertion problem). All 12pkg/authserver/...packages pass, includingtokenexchangeandtest/integration/authserver.New coverage added here: consent-policy branches (positive and negative per branch),
scope/scpprecedence, clock-skew leeway, config validation, factory wiring, shared-JWKS-URL policy conflicts, and HTTP-level end-to-end exchange against anhttptestexternal IdP — all under plaintask test, no Docker, no network egress, no wall-clock sleeps.Changes
server/tokenexchange/validator.goExternalActor/ExternalIssuer/AllowedDelegateClientsonValidatedClaims;may_actshape validation; ID-token claim rejection;kid/use/algfiltering;scpfallbackserver/tokenexchange/multi_issuer_validator.goTrustedIssuerconfig surface; actor-claim resolution and allowlist check; per-issuer HTTP client;jwk.CacheJWKS with failure backoff;ValidateJWKSURLchoke pointserver/tokenexchange/handler.goExternalActorconsent branch andAllowedDelegateClientsguard; external-issueractprovenance;buildActClaimextractionserver/tokenexchange/factory.go,server_impl.goauthserver/config.goTrustedIssuersonRunConfigandConfig; validation at both layers; userinfo rejection in URL validationauthserver/runner/embeddedauthserver.godocs/arch/17-token-exchange-delegation.mddocs/server/swagger.*,docs.goRunConfigfieldDoes this introduce a user-facing change?
Yes. Operators can declare
trusted_issuerson the embedded auth server so agents can exchange subject tokens minted by an external OIDC issuer (Entra, Okta, Keycloak) for ToolHive-scoped delegated tokens. Every entry requiresissuer_urlandexpected_audience; consent is fail-closed, so an issuer with noallowed_actorsaccepts onlymay_act-bearing tokens.There is no CRD surface in this PR — the feature is reachable only from a hand-written
RunConfigfor now. See the note below.Special notes for reviewers
This is not yet usable end to end, by design. Nothing in production can currently provision a confidential client holding the token-exchange grant: DCR hardcodes
Public: trueand restricts grant types, CIMD is public-only, and there is noRunConfigclient-seeding path. Discovery also advertises neither the grant nor secret-based client auth. Both blockers are tracked in #6082 (against epic #5194) and predate this PR — they affect the self-issued path too. This PR is the consent model; #6082 is what makes either path reachable. CRD fields were deliberately dropped from this branch until then, so we do not freeze an API shape around a feature nobody can exercise.Review size. ~1,700 production lines over 7 files, above the 400-line guideline. Much of
multi_issuer_validator.go's bulk is doc comments on security-relevant invariants rather than logic, and the file's core was pre-existing dead code being hardened. Commits are ordered to be read in sequence and each is scoped to one concern. Happy to split if reviewers prefer — the natural seam is after "Wire the multi-issuer validator into the factory", though steps 1, 2 and 5 must not be separated, since that commit is what makes external tokens reachable at all.Note on per-commit review. Commits group by file (production first, tests last), so intermediate commits do not individually compile their tests, and a mid-branch commit can show config plumbed but not yet consumed. Please review the branch tip.
Where I'd like scrutiny:
checkDelegationConsentcase ordering —MayAct→ExternalActor→ClientIDmismatch →ClientIDempty. An external token's ownclient_idmust only ever be able to reject, never authorize, since it lives in the external IdP's namespace.allowed_delegate_clientsis set; a trusted issuer's subject namespace must be disjoint from every other issuer's, or it can mint a delegated token indistinguishable from a local user's; and amay_act-emitting issuer controls consent directly, so that claim must be drawn from ToolHive's client namespace.may_actwithoutissis compared directly against a ToolHive client ID. That is a deliberate trust-policy choice, not a standards guarantee — RFC 8693 saysiss+submay be needed to identify an actor. Flagged by review; happy to tighten to requireissif reviewers prefer.audconstraint:ensureAudienceSubsetOfSubjectbounds the granted audience by the subject token'saud, so the external API identifier must equal one of ToolHive's allowed-audience URIs or every request yieldsinvalid_target. Documented, but it will look broken on first use.Prior review. Reviewed by OAuth/RFC-8693, Entra, Okta, Kubernetes and Go security specialist agents plus three external review passes. Fixes landed from those include the
scp-vs-scopeprecedence bug, a JWKS negative-caching stale-discard bug, a lost retry bound, per-issuer HTTP policy collapse on a sharedjwks_uri(real for multi-tenant Entra, whose v1 JWKS endpoint is tenant-independent), a permanently-broken-issuer bug whenRegisterfailed before registering, and userinfo-in-URL rejection for bothissuer_urlandjwks_url.Implementation plan
Approved implementation plan
Issue #5989 — Consent model for external OIDC subject tokens
Context
MultiIssuerTokenValidator(pkg/authserver/server/tokenexchange/multi_issuer_validator.go) can alreadyverify subject tokens from trusted external OIDC issuers, but it is dead code:
factory.gostill buildsNewSelfIssuedTokenValidator, so external tokens are unreachable. Even if it were wired in, external tokenscarry no
client_idclaim, socheckDelegationConsent(handler.go:302) rejects them at theClientID == ""branch.The gap is not a bug — it is a missing authorization decision. "Trusted issuer +
aud== us + validsignature" authorizes ToolHive as a resource, not any specific client. Accepting such a token wholesale
is a confused-deputy risk (CWE-863). The outcome of this work: a fail-closed consent policy for external
subject tokens, shipped in the same change that first makes external tokens reachable, plus the operator
config surface to declare trusted issuers.
Reference:
gh issue view 5989.Design
Consent policy (fail-closed, evaluated on the external path only)
may_act(RFC 8693 §4.4) → authoritative. The validator does not check the allowlist;the existing
checkDelegationConsentenforcesmay_act.sub == actorID. Unchanged behaviour.may_act→ resolve the per-issuer actor claim (defaultazp; operators may setappidfor Entra v1or
cidfor Okta) and require its value to be a non-empty string present in that issuer'sAllowedActors. EmptyAllowedActors⇒ reject every non-may_acttoken (mirrors the existing emptyAllowedAudiencesconvention).The self-issued path (
client_idbinding) is untouched.Correction to the issue's proposed split
The issue states the recommended split "needs no new field" on
ValidatedClaims. That is wrong:checkDelegationConsenthas a terminalcase validatedClaims.ClientID == ""that rejects everyexternal token, including one that just passed the validator's allowlist. The handler must be able to tell
"already consented by the validator" from "unbound token".
So: add
ExternalActor stringtoValidatedClaims(the escape hatch the issue itself offers). Thevalidator sets it only after a successful allowlist match;
buildValidatedClaimsnever populates it fromclaims, and the self-issued path never sets it — so it cannot be spoofed from token content. The handler
gains one branch that treats a non-empty
ExternalActoras consent already granted. This also gives thehandler an auditable value to log.
We deliberately do not populate
ValidatedClaims.ClientIDfrom the external actor claim (namespacecollision between the external IdP's client IDs and ToolHive's own).
Actor-claim resolution
assignClaim(validator.go:258) routesname/email/client_id/scope/may_actto structured fieldsand drops the registered claims (
sub, iss, aud, exp, iat, nbf, jti); everything else lands inExtra.Consequences the implementation must handle:
Extra[ActorClaim]—azp/appid/cidare not well-known fields.ActorClaim == "client_id"would never be found inExtra. Fall back toValidatedClaims.ClientIDforthat one name rather than failing closed on a plausible operator config.
ActorClaimset to a registered claim (sub,iss, …) is guaranteed to fail closed forever. Reject thatat config-validation time with a clear error rather than silently rejecting all traffic (go-style rule:
fail loudly on config that silently disables).
Config path (chosen: reuse one type)
tokenexchange.TrustedIssuergains JSON/YAML tags and is used verbatim in both the serializableRunConfigand the resolved
Config— same asAllowedAudiences([]stringin both layers), and it avoids the"parallel types that drift" go-style rule. No mapping code.
Trusted issuers reach the factory as a closure argument, mirroring
DelegationTokenLifespan:Factory(delegationLifespan, trustedIssuers, insecureAllowHTTP). The issue suggestedserver.AuthorizationServerConfiginstead, but that is impossible:tokenexchangeimportspkg/authserver/server, soservercannot referencetokenexchange.TrustedIssuerwithout an import cycle.Reaching an external issuer in tests and local dev
insecureSkipJWKSURLValidation(multi_issuer_validator.go:81) is unexported, so anhttptestJWKS serveris unreachable from any package other than
tokenexchange— includingpackage authserver, where the onlyexisting full-HTTP token-exchange test lives.
httptest.NewTLSServerdoes not help: that transport sets noRootCAs, and the dial-time private-IP guard fires regardless of scheme.Fix by reusing the knob that already exists:
RunConfig.InsecureAllowHTTP(config.go:119), whoseestablished meaning across the repo is "permit http:// OIDC issuers and HTTP discovery for
development/testing". Thread it to
NewMultiIssuerTokenValidatorand rename the field toinsecureAllowHTTP. This also unblocks local dev against a plain-HTTP Keycloak in kind.The field relaxes the HTTPS checks and the loopback/private-IP SSRF guard — the same blast radius it has
today. Its doc comment must say so explicitly, on both the struct field and the constructor parameter.
Clock-skew leeway
External path only:
ValidateWithLeeway(expected, externalClockSkewLeeway)withexternalClockSkewLeeway = 60 * time.Second. Self-issued stays at0(shares ToolHive's clock).Honest limitation to record in the code comment: leeway widens
nbf/iatacceptance, but a token expiredby less than the leeway still fails later at
computeLifetime(handler.go:234,remaining <= 0→invalid_grant"The subject token has expired"). That is fail-closed and out of scope to change here.Accepted limitation (document, do not fix here)
An allowlisted external actor satisfies consent for any ToolHive confidential client holding the
token-exchange grant — the allowlist authorizes "this external client's tokens may be exchanged here", not
"…by this particular ToolHive client". This is the decision recorded in the issue. Document it in the
TrustedIssuer.AllowedActorsdoc comment.Out of scope
Error-code taxonomy (
invalid_request→invalid_grantfor grant-level external failures,handler.go:118). Leave theTODOin place, retargeted to a new follow-up issue.Corrected after the Step 2 OAuth review. The issue's proposed follow-up is backwards.
RFC 8693 §2.2.2 says verbatim: "if either the
subject_tokenoractor_tokenare invalid for anyreason, or are unacceptable based on policy … The value of the
errorparameter MUST be theinvalid_requesterror code." So the currentinvalid_requestfor validator failures is what the specmandates, and retargeting it to
invalid_grantwould move away from conformance. The consent brancheskeep
invalid_grantas a documented deviation (RFC 6749 §5.2 covers "issued to another client", §2.2.2permits other codes, Keycloak and Hydra do the same). No follow-up issue is filed; the TODO is deleted
and replaced with a comment recording the reasoning. Step 9's "file the follow-up issue" deliverable is
dropped.
Execution
Single branch (
5989-delegation-hardening), granular commits. PR split decided after the code lands;the natural seam is after Step 7 (
pkg/authservercomplete, self-consistent, and covered end to end) — butnote the sequencing constraint: Steps 1, 2 and 5 must never be separated across PRs, since Step 5 is
what makes external tokens reachable.
Per-step workflow (per
MEMORY.mdMoE convention): worker implements → opus reviewer reviews → user decidescommit or rework. Do not start step N+1 before step N's review returns.
Step 1 — Validator-side consent policy + leeway
Worker:
go-expert-developer(sonnet) · Reviewer:go-security-reviewer(opus)Deliverables
pkg/authserver/server/tokenexchange/multi_issuer_validator.goTrustedIssuer: addActorClaim stringandAllowedActors []string, with JSON/YAML tags on all fivefields (
issuer_url,expected_audience,jwks_url,actor_claim,allowed_actors).const defaultActorClaim = "azp",const externalClockSkewLeeway = 60 * time.Second.validateExternalToken: swap leeway0→externalClockSkewLeeway; afterbuildValidatedClaims,when
claims.MayAct == nil, run the allowlist check and setclaims.ExternalActoron success.Extra[ActorClaim]with theclient_idfallback, and checking membership viaslices.Contains.NewMultiIssuerTokenValidator:slog.Warnonce per issuer whenAllowedActorsis empty — onlymay_act-bearing tokens from that issuer will be accepted.TODO(#5989)block from the type doc comment; replace it with a short description of theconsent policy.
pkg/authserver/server/tokenexchange/validator.go: addExternalActor stringtoValidatedClaimswith adoc comment stating it is set only by the external path after an allowlist match and is never read
from token claims.
Acceptance criteria
may_actexternal token whose actor claim value is inAllowedActors→ validated,ExternalActorset to that value.may_actexternal token with: missing actor claim / non-string actor claim / empty-string value /value absent from
AllowedActors/ issuer with emptyAllowedActors→ error,ExternalActorempty.may_act-bearing external token → validated regardless ofAllowedActors,ExternalActorempty.0,ExternalActornever set.ExternalActor.task lint-fixclean; package builds. (Do not runtask testhere — Step 3 owns tests.)Step 2 — Handler consent branch
Worker:
go-expert-developer(sonnet) · Reviewer:oauth-expert(opus)Deliverables
pkg/authserver/server/tokenexchange/handler.go: incheckDelegationConsent, insertcase validatedClaims.ExternalActor != "":after theMayActcase and before theClientIDcases, returning nil. Extend the doc comment to describe all three consent sources and why the external
case is already authorized upstream.
"external_actor"to the existingslog.Debugcontext on the validation-failure path, or a newdebug line on success — whichever keeps the diff smaller. No actor values at INFO or above.
Acceptance criteria
MayAct→ExternalActor→ClientIDmismatch →ClientIDempty. A tokenwith both
may_actandExternalActorset cannot occur (Step 1 guarantees it), butMayActstill wins.task lint-fixclean.Step 3 — Unit tests for the consent policy
Worker:
unit-test-writer(sonnet) · Reviewer:code-reviewer(opus)Deliverables
pkg/authserver/server/tokenexchange/multi_issuer_validator_test.go: extend the existing table test.Reuse
newMultiValidator(:74),startJWKSServer(:29),externalClaims()(:91),signToken(
validator_test.go:58).Cases: actor in allowlist (default
azp); customActorClaim: "appid"/"cid"; actor not in allowlist;actor claim absent; actor claim non-string; actor claim empty string; empty
AllowedActors;ActorClaim: "client_id"fallback;may_actpresent with emptyAllowedActors(accepted,ExternalActorempty);may_actpresent with a non-allowlistedazp(accepted).Plus a leeway test: token with
nbf~30s in the future accepted;nbf~5m in the future rejected.pkg/authserver/server/tokenexchange/handler_test.go:checkDelegationConsentcases forExternalActorset (accepted),ExternalActorset with mismatchedmay_act(rejected —may_actwins),and the unchanged self-issued matrix.
Acceptance criteria
testing+ testify,t.Parallel()on parent and subtests,table-driven with
token func(t *testing.T) stringanderrContains.task testgreen for./pkg/authserver/....Step 4 — Config plumbing (RunConfig → Config → runner)
Worker:
go-expert-developer(sonnet) · Reviewer:go-architect(opus)Deliverables
pkg/authserver/config.goRunConfig:TrustedIssuers []tokenexchange.TrustedIssuerwith tagtrusted_issuers,omitempty, doc comment noting fail-closed semantics.Config(~line 587): same field, resolved layer.validateTrustedIssuers()called fromConfig.Validate()next tovalidateDelegationTokenLifespan()(config.go:748): per issuer require non-emptyIssuerURL,httpsscheme (parse and check, per the "Validate Parsed Results" rule), non-emptyExpectedAudience,httpsonJWKSURLwhen set, no duplicateIssuerURL, and rejectActorClaimin
{sub, iss, aud, exp, iat, nbf, jti}with an explicit "would reject every token" message.pkg/authserver/runner/embeddedauthserver.go: copy intoresolvedCfg(~:227) withslices.Clone, matching theAllowedAudiencesline.ensureAudienceSubsetOfSubject(
handler.go:393) bounds the requested audience by the subject token'saud. An external IdP'saudis typically an app-ID GUID or
api://<app-id>, so a normalresource=https://mcp.example.comyieldsinvalid_targeton every request, and the only grantable value is the external audience string — whichaudience.go:29requires to be an absolute http/https URI. The external path only works if theoperator sets the external API identifier to exactly one of ToolHive's allowed-audience URIs. This is
undocumented today and will look broken on first use. It needs a paragraph on the
TrustedIssuersconfigfield docs here, and in the arch doc in Step 9.
grantScopesintersects with the subject token'sscopeclaim, so an external tokenwhose scopes are meaningless to ToolHive (or absent) yields a zero-scope delegated token. Correct and
fail-closed, but surprising.
Acceptance criteria
RunConfigwith notrusted_issuersproduces byte-identical behaviour to today.Config.Validate()with a message naming the offending issuer URL.AllowedActorsis accepted by validation (may_act-only issuers are legitimate) — it failsclosed at runtime and warns at startup.
task lint-fixclean; no import cycle (pkg/authserveralready importstokenexchangeinserver_impl.go).Step 5 — Wire the validator into the factory
Worker:
go-expert-developer(sonnet) · Reviewer:go-security-reviewer(opus)Deliverables
pkg/authserver/server/tokenexchange/multi_issuer_validator.go: add a fourth parameterinsecureAllowHTTP booltoNewMultiIssuerTokenValidator, assigning the field currently namedinsecureSkipJWKSURLValidation— rename it toinsecureAllowHTTP(one in-package test call site atmulti_issuer_validator_test.go:85). Doc comment on both field and parameter must state that it relaxesthe HTTPS enforcement and the loopback/private-IP SSRF guard, and is for development and testing only.
pkg/authserver/server/tokenexchange/factory.go:Factory(delegationLifespan time.Duration, trustedIssuers []TrustedIssuer, insecureAllowHTTP bool) (server.Factory, error). Inside the closure,build the self validator as today, then when
len(trustedIssuers) > 0wrap it viaNewMultiIssuerTokenValidator(selfValidator, config.GetAccessTokenIssuer(), trustedIssuers, insecureAllowHTTP); otherwise keep the self validator. Assign to the existingHandler.validatorfield(
SubjectTokenValidatorinterface).pkg/authserver/server_impl.go:252(buildProvider): passcfg.TrustedIssuersandcfg.InsecureAllowHTTP(both already onConfig— the latter atconfig.go:687, no new field needed).Retarget the— already deleted in Step 2; see the correctedTODO(#5989)athandler.go:118"Out of scope" note above. Nothing to do here.
Security gap to close here (found during Step 4). An explicitly configured
TrustedIssuer.JWKSURLnever reaches
validateJWKSURL— that helper is only called insidediscoverJWKSURL, for discoveredURLs (see
resolveJWKS: it preserves a configuredJWKSURLand only re-discovers when it is empty). Soa configured
jwks_urlgets no HTTPS or private-IP check before being fetched; only the dial-timeisDisallowedIPguard and the redirect scheme check apply.Fix location settled by the Step 4 review:
resolveJWKS, not the constructor. Move thevalidateJWKSURLcall out ofdiscoverJWKSURLand intoresolveJWKSimmediately beforefetchJWKS—one choke point covering configured and discovered URLs, and it deletes the existing call rather than
adding a second one. The constructor is the wrong home:
insecureSkipJWKSURLValidationis currently setafter construction (
multi_issuer_validator_test.go:85-87), and ~20 table cases pass anhttp://127.0.0.1JWKS URL, so a constructor-time check rejects all of them before the flag exists.(This step renames that field to a constructor parameter, which would make the constructor possible —
but
resolveJWKSis still the better choke point.) No network-ordering concern either way:validateJWKSURLdoesurl.Parseplusnet.ParseIPon the literal host, andParseIPreturns nil fora hostname, so it never resolves.
Keep Step 4's config-layer check as well. It is deliberately weaker (no private-IP check, permits http
under
InsecureAllowHTTP) — that is the fail-fast layer;validateJWKSURLis the enforcement layer.Unify the operator-facing error vocabulary.
Config.validateTrustedIssuersspeaks wire keys(
trusted_issuers: issuer_url "x": ...) whilevalidateTrustedIssuerin the validator speaks Go fieldnames (
ExpectedAudience is required,ActorClaim %q is not supported). An operator who wroteexpected_audience:in YAML gets toldExpectedAudienceis required. Wrap the constructor error at thefactory call site with a
trusted_issuers:prefix, and consider having the constructor name wire keysnow that the struct is a wire type. Do not duplicate the checks — the layer split is correct.
Delete a false test comment.
multi_issuer_validator_test.go:1090-1094claims the checks on aconfigured
JWKSURLare "exercised end-to-end elsewhere". They are not — the check does not exist yet,and that comment is part of why the gap stayed invisible. Correct it when the guard lands.
Opportunity opened by this step. Step 2 detects "external" as
ExternalActor != "", because theHandlerhas no way to comparevalidatedClaims.Issueragainst the server's own issuer. Consequence: amay_act-bearing external token gets noact-chain provenance — its externalsubis copiedverbatim with the issuer dropped, which is exactly the deferred subject-collision problem. Once this step
wires the factory, the closure already has
config.GetAccessTokenIssuer(); consider givingHandleraselfIssuerfield so provenance nesting covers every external token, not just the allowlisted path.Keep the outermost
act.subasactorIDand keep the depth accounting correct.Acceptance criteria
*SelfIssuedTokenValidatoris used; behaviour and error stringsunchanged.
*MultiIssuerTokenValidatoris used, self-issued tokens still validate.NewMultiIssuerTokenValidatorfails server startup rather than degrading to theself validator.
InsecureAllowHTTPfalse (the default) leaves the HTTPS and SSRF guards fully active; no test-onlymutation path is needed any more, but the existing in-package direct field write still compiles.
task lint-fixclean; whole repo builds (Factoryhas exactly one caller).Step 6 — Tests for config + factory wiring
Worker:
unit-test-writer(sonnet) · Reviewer:code-reviewer(opus)Deliverables
pkg/authserver/server/tokenexchange/factory_test.go: currently never invokes the returned closure. Adda test that does — asserting the concrete validator type for both the empty and non-empty
trustedIssuerscases, and that a badTrustedIssuer(emptyExpectedAudience) surfaces an error fromthe closure. Build the
*server.AuthorizationServerConfigargument viaserver.NewAuthorizationServerConfig(provider.go:219) or the existing test helpers inpkg/authserver/server/provider_test.go.pkg/authserver/config_test.go:TestConfigValidate_TrustedIssuersin the style ofTestConfigValidate_DelegationTokenLifespan(:677), reusing thebase()closure andassertErrorhelper. Cover each validation rule plus the "no trusted issuers" happy path.
pkg/authserver/runner/embeddedauthserver_test.go: assert the RunConfig → Config copy, closing thedocumented gap that nothing tests that conversion step.
Acceptance criteria
task testgreen for./pkg/authserver/....Step 7 — HTTP-level integration tests
Worker:
go-expert-developer(sonnet) · Reviewer:oauth-expert(opus)Home is
pkg/authserver/integration_test.go(package authserver), copyingTestIntegration_TokenExchange_ConfidentialClientHappyPath:688— it already does a realPOST /oauth/tokenwith a confidential client seeded via the in-packagewithExtraClientoption (:141)and
registration.New(registration.Config{Public: false, GrantTypes: [token-exchange]}).Not
test/integration/authserver/: its only client-creation path is DCR, which hardcodesPublic: true(server/handlers/dcr.go:108) and rejects the token-exchange grant(
server/registration/dcr.go:93), so a confidential acting client cannot exist there.Deliverables
Test-local helper: an
httptestserver acting as the external IdP, serving/.well-known/openid-configuration(echoingr.Hostasissuer, permulti_issuer_validator_test.go:44) plus a JWKS endpoint, signed by a key that is not the authserver's. Configure the test server with
TrustedIssuerspointing at it.Changed by the Step 5 review: the global
RunConfig.InsecureAllowHTTPno longer reaches the externalpath at all. Set the per-issuer
insecure_allow_http: trueandallow_private_ips: trueon theTrustedIssuerentry instead — the loopback httptest server needs both (HTTP scheme and a 127.0.0.1dial). This is strictly better for the test: it no longer relaxes anything about the self issuer.
Cases:
azpinAllowedActors→200, response carriesissued_token_typeand anactclaim naming the acting client.
azpflipped to a non-allowlisted value →400 invalid_grant.azpbutmay_act.sub== acting client →200.TrustedIssuers→400. (Needs no external server — the issuer-mapmiss short-circuits before any JWKS fetch.)
Explicit JWKS-URL config (skipping discovery) covered by at least one case, so both resolution paths run.
Acceptance criteria
task testwith-race: no Docker, no network egress, no wall-clock sleeps(
test/integration/...andpkg/...are both in the defaulttask testset —Taskfile.yml:101— andCI runs
task test-coverage,.github/workflows/test.yml:66).//go:buildtag added (matches the surrounding files).Step 8 — Operator CRD surface
Worker:
kubernetes-go-expert(sonnet) · Reviewer:kubernetes-expert(opus)Deliverables
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:TrustedIssuers []TrustedIssuerConfigon
EmbeddedAuthServerConfig(:353), plus theTrustedIssuerConfigtype with kubebuilder markers —issuerURLandexpectedAudiencerequired withPattern=^https://,jwksURLoptional same pattern,actorClaimoptional (document theazp/appid/cidchoices; do not enum-constrain),allowedActorsoptional
[]string. Field docs must state the fail-closed semantics of an emptyallowedActors.The CRD deliberately requires
httpswith no dev escape hatch — plain-HTTP issuers stay reachable onlyvia a hand-written
RunConfigwithinsecure_allow_http, not through the operator.cmd/thv-operator/pkg/controllerutil/authserver.go: map intoRunConfiginsideBuildAuthServerRunConfig(:519) — the single choke point shared byMCPServer/MCPRemoteProxy(
:459) andVirtualMCPServer(vmcpconfig/converter.go:528), so no converter change is needed.task operator-generate(deepcopy),task operator-manifests(CRD YAML + helm chart CRDs),task crdref-gen(docs/operator/crd-api.md,virtualmcpserver-api.md),task docs(RunConfigisreflected into
docs/server/swagger.*— see the existingdelegation_token_lifespanentry).Acceptance criteria
task operator-generate && task operator-manifests && task crdref-gen && task docsleave a clean,committed diff (no uncommitted regeneration drift).
VirtualMCPServerand anMCPServerwithtrustedIssuersboth produce the expectedRunConfig.trustedIssuersproduces a RunConfig identical to today's.helm template/ct lintpass per.claude/skills/check-contribution.Step 9 — Operator tests + architecture doc
Worker:
unit-test-writer(sonnet) · Reviewer:documentation-writer(opus)Deliverables
cmd/thv-operator/pkg/controllerutil/authserver_test.go: add cases to theTestBuildAuthServerRunConfigtable (:797) — trusted issuers present, absent, multiple, and withactorClaim/allowedActorsset.docs/arch/11-auth-server-storage.md: a short subsection on external subject-token exchange —the trust model, the two consent signals, the fail-closed default, and the documented limitation that the
allowlist is not scoped to a specific ToolHive client.
validateJWKSURL, so its scheme backstop isconfig-time
validateIssuerURL(ti.IssuerURL, ti.InsecureAllowHTTP)— also env-free. The one crack isvalidateIssuerURL's localhost exception:issuer_url: http://localhost:8080passes with no flags set,and with
INSECURE_DISABLE_URL_VALIDATIONset the transport would allow the plaintext GET. It stillcannot complete, because the dial guard blocks loopback when
AllowPrivateIPsis false and that guardreads no environment. Unreachable, but the two guards are load-bearing in combination there rather than
independently — worth stating so neither is removed casually.
SameHostRedirectPolicymeans an issuer whose/.well-known/openid-configurationreturns a 30x to a different hostname will now fail discovery, andthe exact-match
doc.Issuer != IssuerURLcheck limits the workaround of configuring the post-redirectURL. The operator escape hatch is setting
jwks_urlexplicitly. Document it.audmust equal a ToolHive allowed-audience URI); that an external token's scopes are intersected andusually yield a zero-scope token; that a trusted issuer emitting
may_actbypassesAllowedActorsentirely, so that claim must be in ToolHive's namespace and not influenceable by an untrusted party; and
the subject-namespace collision below.
ledger): an external
subis written verbatim into the ToolHive-issued token and becomes the Cedarprincipal, with the issuer dropped. OIDC only guarantees
subuniqueness within an issuer, so atrusted issuer whose subject namespace overlaps the local one makes external and local identities
indistinguishable. Step 2 adds
act-chain provenance so the origin is at least auditable; a real fixneeds per-issuer subject qualification, which is a repo-wide identity decision beyond this issue.
Until then the arch doc must state that trusted issuers' subject namespaces MUST NOT overlap.
Acceptance criteria
task testgreen repo-wide (task test-allif the branch is otherwise ready).Verification
Automated coverage is a deliverable, not a manual afterthought: Step 3 (consent branches), Step 6 (config +
factory wiring), Step 7 (HTTP-level end-to-end), Step 9 (operator). All run under plain
task testwith-race— no Docker, no build tags, no network.Per-step, run once at the end of the step (not speculatively):
task lint-fix task testManual smoke test against a real IdP, after Step 8 — Keycloak emits
azp, which is the defaultActorClaim:.claude/skills/keycloak-kind-setupfor the IdP,deploying-vmcp-locallyfor the vMCP + authserver.trustedIssuersat the Keycloak realm issuer withexpectedAudience= the authserver's allowedaudience and
allowedActors= the Keycloak client ID.POST /oauth/tokenwithgrant_type=urn:ietf:params:oauth:grant-type:token-exchange,subject_token_type=urn:ietf:params:oauth:token-type:jwt, authenticating as a ToolHive confidentialclient holding the token-exchange grant. Expect
200and anact.subnaming that client.allowedActors, restart, retry → expect400 invalid_grant.This is also the check that the CRD field actually reaches the running authserver's
RunConfig, which nounit test can prove.
Generated with Claude Code