Harden external may_act delegation - #6353
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6353 +/- ##
==========================================
+ Coverage 72.98% 72.99% +0.01%
==========================================
Files 742 742
Lines 78398 78491 +93
==========================================
+ Hits 57216 57293 +77
- Misses 17193 17208 +15
- Partials 3989 3990 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c2005b8 to
38aa57c
Compare
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Panel review
I reviewed commit 38aa57ce3a558195a4b275afe33c8f5d421bd034 against origin/main with independent OAuth, application-security, threat-model, MCP, operator/CRD, ToolHive-conventions, test-coverage, architecture, reuse, and duplication passes. I also checked the current primary standards: RFC 8693, RFC 9700 (OAuth Security BCP), RFC 8725 (JWT BCP), RFC 7519, and OIDC Core.
The first-hop may_act hardening itself is sound: opt-in is fail-closed, external may_act.iss is required and bound to the ToolHive issuer, allowedDelegateClients is still enforced against the authenticated client, wildcard + allowMayAct is rejected, and inherited actor claims are reduced to iss/sub with depth and size bounds. CI is fully green (44/44 checks, including unit, lint, operator/core E2E, CRD compatibility, codegen, and vulnerability scanning).
I found the following issues that should be addressed before merge:
1. High — MCPExternalAuthConfig can report Valid=True for an invalid trusted-issuer policy
TrustedIssuers is added to EmbeddedAuthServerConfig, but MCPExternalAuthConfig.validateEmbeddedAuthServer never validates it. Deterministically invalid policies (duplicate/self issuer, unsupported actorClaim, invalid URL, wildcard mixed with named delegate clients, etc.) therefore pass the config controller and receive ConditionTypeValid=True; they fail only later when a consuming workload assembles its RunConfig.
Locations: cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:687, :1852-1900; success status at cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go:156-175.
Please validate trusted issuers as part of the shared config's own Validate() path, reusing the runtime validator so the policy cannot drift.
2. High — new terminal config failures can leave Ready=True stale
The new InvalidEmbeddedAuthServerConfigError path sets Phase=Failed for VirtualMCPServer and MCPRemoteProxy, but neither handler clears a previously true Ready condition. Editing a ready resource to an invalid trusted-issuer policy can therefore produce Phase=Failed, validation=False, and Ready=True simultaneously. MCPServer already clears Ready in its equivalent path.
Locations: cmd/thv-operator/controllers/virtualmcpserver_controller.go:170-194, cmd/thv-operator/controllers/mcpremoteproxy_controller.go:69-99, comparison at cmd/thv-operator/controllers/mcpserver_controller.go:180-185.
Please set Ready=False with the terminal validation reason and add ready→invalid transition coverage.
3. Medium — the issuance path trusts a deliberately tolerant audit parser and can re-sign an empty actor hop
coreaudit.ParseDelegationChain explicitly accepts an act object with no iss or sub (for forensic logging). chainToAct then turns {"role":"admin"} into {} and ToolHive signs that empty object as a prior actor hop. Audit parsing is intentionally tolerant; issuance validation should be strict.
Locations: pkg/authserver/server/tokenexchange/handler.go:496-540, :562-577; parser contract in toolhive-core/audit/delegation.go says a map with no iss/sub/act yields an empty-identity hop.
Please reject any normalized hop that has neither a non-empty iss nor sub before minting. A regression test using an extras-only outer and nested hop would also prove that inner attributes are stripped without preserving meaningless {} hops.
4. Medium — newly added token-rejection paths use the wrong RFC 8693 error
The new malformed-chain and oversized-chain branches return invalid_grant. RFC 8693 §2.2.2 says an invalid subject/actor token, including one unacceptable by policy, MUST use invalid_request. The code comment relies on the later general allowance for other errors, but that does not override the preceding explicit requirement for invalid subject tokens.
Locations: pkg/authserver/server/tokenexchange/handler.go:503-513, :542-549.
Please use the RFC-defined error for these new rejection paths (and consider aligning the adjacent pre-existing token-policy failures separately).
5. Medium — admission validation omits deterministic policy invariants
The new CRD CEL checks only allowMayAct + "*" and allowPrivateIPs without jwksUrl. The API still admits wildcard mixed with named delegate clients, duplicate issuer URLs, self-issuer collisions, and reserved/unreadable actorClaim values, all of which runtime validation deterministically rejects. The operator rule requires constraints expressible in OpenAPI/CEL to be rejected at admission rather than discovered during reconciliation.
Location: cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:389-477.
At minimum add wildcard exclusivity. Prefer map-list/uniqueness validation for issuer URLs and CEL for the other spec-only invariants where practical; retain runtime validation as defense in depth.
6. Low — architecture example contradicts the newly issued token shape
buildActClaim now always emits act.iss, but the self-issued delegate example still shows "act": {"sub":"coding-agent"}.
Location: docs/arch/17-token-exchange-delegation.md:358-370.
Please update it to include the ToolHive issuer.
Targeted coverage gaps
The current tests are broad, but two security boundaries deserve direct coverage:
- configure issuer A with
allowMayAct=trueand issuer B with the default false, then prove identical claims are isolated per issuer; - exercise
MCPExternalAuthConfig.spec.embeddedAuthServer.trustedIssuersthrough a referenced consumer, not only the inlineVirtualMCPServersurface.
Non-blocking broader risks observed
These predate this diff, so I am not using them to block this PR, but they merit follow-up issues: external ID/access-token type confusion when expectedAudience is a client ID and the token lacks optional at_hash/c_hash (RFC 8725 §§2.8, 3.11–3.12), and loss of per-external-issuer delegate containment on later cross-client re-exchange after the first token becomes self-issued.
38aa57c to
1655de1
Compare
External subject tokens could use a well-formed `may_act` claim to bypass an issuer's `AllowedActors` policy. Make that consent path an explicit per-issuer decision, constrain it to named ToolHive delegate clients, and expose the policy through the operator CRD. ## Delegation policy An external issuer must opt in before ToolHive will accept its `may_act` claims: ```yaml trustedIssuers: - issuerUrl: https://idp.example.com expectedAudience: https://toolhive.example.com allowedDelegateClients: [agent-a] allowMayAct: true ``` `allowMayAct` defaults to false. The wildcard configuration is rejected when `may_act` is enabled: ```yaml allowMayAct: true allowedDelegateClients: ["*"] # rejected ``` This keeps a foreign issuer from authorizing every ToolHive client merely by emitting an attacker-influenced `may_act` claim. ## Token shape For an external subject token: ```json { "iss": "https://idp.example.com", "sub": "alice-42", "may_act": { "iss": "https://toolhive.example.com", "sub": "agent-a" } } ``` ToolHive issues a token whose external subject is issuer-qualified and whose `act` chain records both namespaces: ```json { "sub": "https://idp.example.com#alice-42", "client_id": "agent-a", "act": { "iss": "https://toolhive.example.com", "sub": "agent-a", "act": { "iss": "https://idp.example.com" } } } ``` When re-exchanging a delegated token, rebuild prior `act` hops from only `iss` and `sub`, bound the resulting serialized claim, and reject malformed or over-deep chains rather than re-signing untrusted nested attributes. Validate trusted-issuer policy during reconciliation so invalid configuration becomes a terminal CR status condition instead of an auth-server crash loop. Add unit, integration, and operator E2E coverage for both the opt-in gate and the operator projection. Closes stacklok#6323 Closes stacklok#6113
1655de1 to
50e640f
Compare
2 participants
Summary
may_actclaim without an explicit issuer-level opt-in, allowing that path to bypassallowedActors.allowMayAct: truebefore accepting theirmay_actclaims, require named ToolHive delegate clients on that path, and reject the unsafe wildcard combination.acthistory toiss/sub, and add unit, integration, and operator E2E coverage.Closes #6323
Closes #6113
Type of change
Test plan
task test)task test-e2e)task lint-fix)task test-e2ewas attempted but could not complete because the locally installed ToolHive Desktop application rejects the workspace binary as a CLI conflict before the relevant test can run.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Changes
may_act, qualify issued actor hops, and normalize prioractchains before re-signing.trustedIssuersandallowMayActto embedded auth-server configuration and generated CRDs.Does this introduce a user-facing change?
Yes. Cluster operators can configure trusted external OIDC issuers under
embeddedAuthServer.trustedIssuers. Externalmay_actdelegation is now disabled unless the issuer explicitly setsallowMayAct: true; configurations combining that opt-in withallowedDelegateClients: ["*"]are rejected.Special notes for reviewers
Review the consent boundary in
pkg/authserver/server/tokenexchange/multi_issuer_validator.goandhandler.go:may_actremains authoritative only after issuer opt-in and authenticated-client containment. The commit message includes concrete input and issued-token shapes.Generated with Claude Code