Add a consent model for external OIDC subject tokens - #6149
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6149 +/- ##
==========================================
+ Coverage 72.54% 72.62% +0.07%
==========================================
Files 739 739
Lines 76851 77121 +270
==========================================
+ Hits 55755 56009 +254
- Misses 17113 17152 +39
+ Partials 3983 3960 -23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
size/XL
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Panel review — 3 axes (Spec vs #5989, Standards, 7-agent Domain panel)
Ran a full panel review against origin/main: Spec axis vs issue #5989, Standards axis vs CLAUDE.md/.claude/rules/CONTRIBUTING.md, and a Domain panel (secure-code-reviewer, oauth-expert, software-architect, library-reuse-reviewer, code-duplication-reviewer, devex-reviewer, ux-reviewer). Findings below, unmerged per axis. One inline comment on the single ship-blocker.
Spec — does this implement what was asked?
Complete vs #5989. Every "ships in THIS PR" item is present: ActorClaim/AllowedActors, fail-closed consent (steps 1–3), RunConfig→Config→factory plumbing, validator wired into factory.go with TODO(#5989) deleted, 60s external / 0 self-issued leeway, per-branch unit + factory-wiring tests. The root cause is fixed rather than the named symptom: the ExternalActor case in checkDelegationConsent sits before the terminal ClientID == "" case. The error-code deviation (keeping invalid_request per RFC 8693 §2.2.2) matches the reasoning the issue pre-authorized and is internally consistent.
Unrequested additions (all fail-closed hardening, noted for the record): AllowedDelegateClients + its enforcement on both paths, may_act.iss == selfIssuer validation, ID-token claim rejection (at_hash/c_hash), external-issuer provenance nesting in the issued act claim.
Standards — project conventions
- Violation (go-style "Keep Comments Synchronized With Code"): stale references to the deleted
resolveJWKSremain as the named choke point —pkg/authserver/config.go:982,multi_issuer_validator.go:946,957,963,multi_issuer_validator_test.go:1299. The live caller isregisterOrRefresh/ensureRegistered. - Violation (same rule): arch doc line 149 claims "A successful fetch is cached 5 minutes" — the PR replaced the 5-minute TTL with
jwk.Cachedriven by Cache-Control headers (15min–30d). - Judgement: size (~1700 production lines) exceeds the 400-line guideline, but CONTRIBUTING.md's written escape clause (explanation + ordered commits) is satisfied. Splitting further would bisect the jwk.Cache rewrite.
- Checked clean: SPDX headers, testing conventions, HTTP body drains, no caller-input mutation, constructor validation, commit format, silent-success logging.
Domain — specialist panel
Ship-blocker (1, cross-confirmed by software-architect + oauth-expert, independently verified): the arch doc's claim that may_act bypasses allowedDelegateClients — see inline comment.
Judgement calls:
- [Medium]
invalid_targethint (handler.go:617-619) doesn't point at the #1 documented footgun (external IdPaudlikeapi://<guid>not covering the requested resource). Suggest appending "review the server's allowed_audiences and the trusted issuer's expected_audience." - [Low] Validator errors are opaque strings — fine for the wire collapse, but future consumers (audit/metrics) can only string-match. Records the residual cost of the deliberately-dropped error taxonomy.
- [Low] OIDC §3.1.3.7 azp-presence rule not enforced (self-audience
aud == [X]+azp == Xpasses if X ∈ allowed_actors). Bounded by trusted-issuer + allowlist; documenting "allowed_actors must name a distinct requesting client, never the API identifier" suffices. - [Low] DEBUG-only failure diagnostics are intentional (no user-enumeration); consider a future INFO log with a closed failure-reason enum, no actor values.
Polish:
invalid_scopehint (handler.go:487-488) could name thescope/scpclaims to check.- Arch doc would benefit from runnable per-provider config examples (Entra
azp/api://audience footgun, Oktacid, Keycloak) and a troubleshooting table. jwk.Cache.Shutdownnever called (no Close path) — acknowledged in-code, matchespkg/auth/token.goprecedent; track as follow-up, not a blocker.
Verified clean (notable given the risk surface):
- Consent gate traced exhaustively — an external token's own
client_idcan only ever reject, never authorize; theClientIDcases are unreachable for external tokens. No bypass shape exists. - SSRF defense is TOCTOU-safe and layered: post-resolution dial-time private-IP guard, same-host redirects,
ValidateJWKSURLchoke point independent of env vars, body/key caps. - JWT validation complete: alg-confusion/
nonestructurally blocked, per-keyuse/algfiltering, spoofed-kidfails without forced refresh, strict zero-toleranceexpre-check independent of leeway. actchain nesting correct per RFC 8693 §4.1;scope-over-scpprecedence correct per RFC 9068; ID-token rejection per RFC 8725 §3.12; lifetime correctly bounded.- Reuse: net negative lines — the PR deletes ~80 lines of hand-rolled SSRF guard / JWKS TTL cache / redirect logic in favor of existing
pkg/networkinghelpers +jwk.Cache, and eliminates two pre-existing duplications. - DevEx surface: naming consistent, zero-values fail-closed and documented,
Factorybreak confined to one internal caller, godoc thorough.
Overall: zero Critical/High code issues found by the panel. The remaining work is documentation accuracy (one ship-blocker + two mechanical comment fixes) and optional error-hint improvements. The prior review passes show — this is in good shape.
ReviewReviewed at the branch tip ( VerificationAll 12 What's rightThe core design holds up under scrutiny:
The Should fix before merge1. The arch doc contradicts the code on That is false, and four other places say so: 2. 3. 4. Stale JWKS caching numbers. Arch doc line 149: "A successful fetch is cached 5 minutes." No 5-minute TTL survives the Worth addressing5. The 6. 7. The 8. 9. ProcessSize. 5,420 additions across 19 files against the repo's 400-line / 10-file guideline. The PR flags it and offers a split, and the sequencing constraint named is real — steps 1/2/5 genuinely can't separate. But the 786-line integration test and the arch doc could land as a follow-up without weakening the security argument. I'd take the split. Comment density is the one place I'd push back on the approach. SummaryThe security model is sound and the hardening is thorough — the consent ordering, the non-spoofable provenance, and the shared- 🤖 Generated with Claude Code |
cea6418 to
32f1c1f
Compare
github-actions
Bot
added
size/XL
Re-reviewRe-reviewed at the new tip ( Verification
Findings 1–6: fixed correctly
Findings 7–9: fixed, but each fix has a defectAll three are the same shape: a correct-looking guard whose predicate is subtly wrong. Worth naming, because it's the failure mode that survives review most easily — the control flow reads right, so attention slides past the condition itself. C. The JWKS conversion cache never repopulates after a rotation (commit if issuerConfig.bridgeSource == nil || sameSetIdentity(issuerConfig.bridgeSource, set) {
issuerConfig.bridgeSource = set
issuerConfig.bridgeJWKS = converted
}It stores only when the source is unchanged — which is exactly the case that already hit the cache. On the miss path ( Not a correctness bug — the miss path returns the freshly converted set, so verification always uses current keys. Purely a defeated optimization. Since the conversion is a pure function of the source, last-writer-wins is safe: drop the guard and assign unconditionally. E. D. The policy-claim rollback uses an unsound predicate (commit Narrow (requires a failed probeCtx, probeCancel := context.WithTimeout(detached, httpTimeout)
registered := v.jwksCache.IsRegistered(probeCtx, issuerConfig.jwksURL)
inconclusive := probeCtx.Err() != nil
probeCancel()
if !registered && !inconclusive {
v.releaseJWKSURLPolicy(issuerConfig)
}B. Moving the Worse, the discovery-failure table test at This is a bad trade and my finding caused it. Note the invariant is already enforced at the point of harm: New blocking issueA. Data race in a new test — this will fail CI. Reproduces 3/3; The in-code comment reasons only about the sibling subtest:
But the parent Simplest fix: drop SummaryFindings 1–6 are genuinely closed. 7–9 are addressed in intent but each fix is defective: the conversion cache is inert after the first rotation (C), the policy rollback can fail open (D), and the private-IP check bought a third enforcement layer at the price of all discovery test coverage (B) — B and D are consequences of my suggestions, and I'd reverse course on B. On top of that the branch has a deterministic None of this touches the consent model, which still holds up. 🤖 Generated with Claude Code |
rdimitrov
left a comment
rdimitrov
left a comment
There was a problem hiding this comment.
Requesting changes for the security and correctness issues called out inline. The highest-risk themes are issuer-scoped identity collapse, insufficient binding of external delegation to the authenticated ToolHive client, and JWKS transport-policy isolation. Reviewed at commit 32f1c1f.
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
|
Thanks — this was a genuinely useful pass, and the three "correct-looking guard whose predicate is wrong" findings were all real. Fixed in B — I kept The layer that does always run is The coverage loss was real and worse than you wrote, though: 8-of-10 branches down to 1, and it was four vacuous cases rather than one — D — used the httprc sentinels rather than Keeping the rollback either way — the previous behaviour was a guaranteed availability bug on every deployment, traded for a fail-open needing a misconfiguration plus a timing coincidence. Worth noting it was symmetric too: an erroneous release left the second issuer's claim unreleased, locking out the rightful owner for the process lifetime, i.e. reintroducing exactly the poisoning C and E — deleted the cache rather than fixing the CAS. Confirmed inert by driving Also: the test guarding that cache couldn't have caught the bug. It asserted A — real, but not deterministic, and the writer wasn't the one you named. Clean 3/3 scoped to the test; it needs the full package, and reproduced 3 of 13 runs there. The racing write isn't So rather than dropping One defect in my own fix, found in review. The And your point about the oversized-document case was sharper than it looked — it would have passed with the cap deleted. Fixed to serve a well-formed oversized document, then mutation-tested it: removing the Not done, and I don't think any of it blocks:
|
|
Rechecked against the latest PR head, The response is persuasive on the issues it directly addresses: retaining the constructor/config The following blockers remain:
The following remain valid but should not individually block this PR:
The full local |
The embedded auth server's RFC 8693 token exchange only accepted subject tokens it had minted itself. MultiIssuerTokenValidator could already verify tokens from external issuers but was unreachable — factory.go built only the self-issued validator — and even wired in it would have rejected every external token, since they carry no client_id and checkDelegationConsent treats that as unbound. The gap was a missing authorization decision, not a bug: a trusted issuer, a valid signature and a matching audience authorize ToolHive as a resource, not any particular client to act for the subject. Accepting such a token wholesale is a confused-deputy risk. Add a fail-closed consent policy and ship it in the same change that first makes external tokens reachable. may_act stays authoritative when present; otherwise the per-issuer actor claim (azp by default, appid for Entra v1, cid for Okta) must appear in that issuer's allowed_actors, and an issuer with none accepts only may_act-bearing tokens. Provenance is recorded in the act chain so a delegation's origin stays auditable. trusted_issuers is plumbed through RunConfig and validated at both the config and constructor layers, so a typo fails before live client registration rather than crash-looping after it. Each issuer gets its own HTTP client with a dial-time private-IP guard and same-host redirect policy, and JWKS URLs pass a single validation choke point whether they were configured or discovered.
Review of the first round found comments and docs that had fallen out of step with the code, plus two reporting defects. The arch doc claimed may_act bypasses allowedDelegateClients and that the JWKS cache used a 5-minute TTL; neither matched the implementation. Six references named resolveJWKS, a function that no longer exists, and a config comment asserted a CRD CEL mirror that was never written. The scp fallback's effect on the self-issued path was undocumented. verifySignature reported "no keys" when a JWKS had keys but none of a compatible algorithm, which sends an operator looking in the wrong place. Distinguish the two. Also enforce allow_private_ips requiring an explicit jwks_url in the validator layer, release the JWKS URL policy claim when registration fails so a transient error cannot poison a URL for the process lifetime, and cache the converted JWKS to avoid a JSON round-trip on every validation.
Three defects, each a guard whose control flow read correctly while its predicate did not. A subtest swapped the process-global slog.Default() to a plain bytes.Buffer while its parent called t.Parallel(), racing every other parallel test in the package that logs. It reproduced in roughly 3 of 13 full-package -race runs. Guard the buffer with a mutex rather than dropping t.Parallel(): the set of concurrent writers grows with every new log line, so the buffer must be safe on its own. The converted-JWKS cache stored only when the source was unchanged, so it went inert after the first key rotation, and its identity helper used reflect.Value.Pointer(), which panics on the very value types its comment claimed to guard against. Delete both — the conversion costs tens of microseconds and is not worth a mutex and a panic path in the verification path. Moving the allow_private_ips check into the constructor had made an empty jwks_url unconstructible in tests, silently cutting discoverJWKSURL from eight covered branches to one and leaving a four-case table whose every case failed at the dial before any handler ran. Exercise the function through the direct seam already used elsewhere in the file, and keep one honest test for the private-address refusal.
Four findings, three of them regressions this branch introduced against the previous JWKS handling. A single jwk.Cache was shared across issuers. httprc keys a resource by URL alone and honours WithHTTPClient only on that URL's first Register, so two issuers resolving to the same jwks_url shared one transport — and Entra v1's JWKS endpoint is tenant-independent, so two Entra tenants collide by construction. A per-URL policy claim guarded that but could not be made correct, since the claim was released outside the registration it guarded. Give each issuer its own cache: the collision becomes unrepresentable and the policy map, its mutex, both claim helpers and their rollback all go away. That also makes the next two reliable, since a second issuer's Register options were previously discarded along with its client. Pin the refresh interval at five minutes rather than letting the issuer being audited choose it through Cache-Control, which httprc otherwise honours up to thirty days — a revoked key whose kid we still hold never triggers the unknown-kid refresh. And cap JWKS bodies at 1 MiB again by wrapping each issuer's transport; the previous justification for the ~1 GB default was wrong, as every refresh after the first is dispatched by a background timer with no request or client authentication involved. Separately, an external issuer's sub was copied verbatim into a token issued under ToolHive's own iss. Native subjects are UUIDs from UserResolver's immutable mapping and Cedar authorizes on sub alone, so an issuer choosing a native user's UUID produced an indistinguishable token. Qualify external subjects as <issuer>#<sub>; a UUID cannot contain '#', so disjointness becomes structural rather than something operators are asked to guarantee. Require may_act.iss on the external path for the same reason.
An empty allowed_delegate_clients meant any confidential ToolHive client holding the token-exchange grant could exchange any allowlisted external actor's token — permissive by omission, on a new federation feature. The external azp/appid/cid names the client at the upstream issuer; it says nothing about which ToolHive client is making the exchange request, so every such client was delegation-equivalent. Make the field required, with "*" as the explicit opt-out. A wildcard element rather than a new boolean: TrustedIssuer is the wire schema for RunConfig.TrustedIssuers and is reflected into swagger, so a new field would be permanent public schema, while the existing field already means exactly "which ToolHive clients may delegate". The binding applies to external may_act as well as the allowed_actors path — may_act bypasses allowed_actors entirely, so it is the path that most needs it. Self-issued may_act is exempt: allowed_delegate_clients comes from trusted-issuer config and has no equivalent there. Free to change now only because no supported path can yet provision a confidential client with this grant (#6082); once one exists this would be a breaking default change.
NewMultiIssuerTokenValidator had grown to 128 lines of code, its loop body doing validation, two startup warnings, HTTP client construction, transport wrapping and cache creation per issuer. Move the object construction into newExternalIssuerConfig; the constructor drops to 41 lines and the helper is 26. Error strings and the order of operations are unchanged, and no test needed touching. The file had also reached 52% comments against 39% code, well above the 40-45% of its siblings. Cut historical narration, restatements of the next line, and the multi-paragraph field docs on TrustedIssuer — those duplicated docs/arch/17-token-exchange-delegation.md, which each field now points at instead. Kept what the code cannot say: why IsRegistered is asked of the cache rather than remembered, why WithConstantInterval rather than WithMaxInterval, the RFC citations and provider quirks, and the invariants a future edit could silently break.
06c7598 to
4d9c3e1
Compare
|
Rebased onto current main and squashed to six commits, one per review round, so the finding-to-commit mapping you have been using still works. New tip Five of the seven blockers are closed, one is partial, one I have taken out of this PR. Details, including where I have gone a different way than you asked. Delegate binding is now fail-closed (your policy/default concern). Issuer-scoped subject collision (1). Fixed by qualifying the issued subject as Shared-URL rollback (4), JWKS allocation (5), key retention (6). All three traced to one decision — a single Your correction on the allocation finding was right and the previous justification was wrong on its central claim: only the first registration is request-driven, every later fetch comes off httprc's background timer with no client authentication involved. Also correct that my oversized-document test only covered discovery's
ID-token discrimination (2) — partial, and I would like your view. There is now a startup warning when IPv6 zone bypass (3) — real, but out of this PR. Confirmed by probe: through a Your four non-blocking items. The delegate default is done, above. Comment density. You raised this in your first review and I left it too long. On the split. Still declining, and it is a judgement you may disagree with. Production code is 7 files, and the seam you and the plan both identified would separate changes that the security argument depends on reading together. Splitting now also means re-opening a PR that has had four review rounds. I would rather land this and keep the follow-ups (#6082, #6232) small. Findings 1–9 from your first review are all closed, including the four documentation defects and the |
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
|
Re-reviewed the force-pushed branch at Correction: I missed the author response when I originally posted this update. After reviewing that response and the linked follow-up issues, I am accepting the two remaining items as scoped follow-ups rather than approval blockers:
The latest series resolves the branch-specific findings from the previous review: issuer-qualified external subjects, issuer-bound Local No remaining approval blockers from me. Approving at this head. |
3 participants
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.Also user-visible, on the self-issued path: a ToolHive-issued access token used as a
subject_tokennow has its scopes read from thescpclaim whenscopeis absent. fosite's default JWT claims strategy emits scopes as a JSON array underscp(RFC 9068 §2.2.1 spells itscope), and this server does not setScopeField— so before this change a genuine ToolHive-issued subject token presented for exchange was treated as carrying no scopes, andgrantScopesrejected every scoped request. Exchanges that previously failed with an empty-scope error will now succeed with the subject token's actual scopes.scopestill wins when both claims are present.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