stacklok / stacklok/toolhive

CEL rule on TokenExchangeIssuerPolicyConfig errors when allowMayAct is omitted + wildcard delegate client

Open Beginner friendly
#6,531 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

authentication bug needs-triage operator
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Summary

TokenExchangeIssuerPolicyConfig.allowMayAct (the canonical
inboundGrants.tokenExchange.issuerPolicies[] shape) has a CEL admission rule
that dereferences the optional field without a has() guard, so admission
fails with no such key instead of treating the omitted field as false.

Where

cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go:830:

// +kubebuilder:validation:XValidation:rule="!(self.allowMayAct && '*' in self.allowedDelegateClients)",message="allowMayAct must not be enabled when allowedDelegateClients contains the wildcard \"*\""

Compare with the legacy TrustedIssuerConfig version of the same rule, one
struct up, which does guard it correctly (line 397):

// +kubebuilder:validation:XValidation:rule="!(has(self.allowMayAct) && self.allowMayAct && '*' in self.allowedDelegateClients)",message="..."

and with the neighboring actorClaim rule on the same canonical struct
(line 831), which also guards correctly:

// +kubebuilder:validation:XValidation:rule="!has(self.actorClaim) || !(self.actorClaim in [...])",message="..."

So the canonical TokenExchangeIssuerPolicyConfig.allowMayAct rule is the
odd one out — it was presumably ported from the legacy rule without carrying
over the has() guard.

Why it only sometimes reproduces

CEL's && is "absorbing" on false: if the right-hand operand
('*' in self.allowedDelegateClients) evaluates to false, the whole
expression short-circuits to false and the left-hand self.allowMayAct
access is never actually forced to error out. So this only surfaces when
both:

  • allowedDelegateClients contains the wildcard "*", AND
  • allowMayAct is omitted from the YAML (no default is applied for CEL
    purposes on this optional bool)

Repro

Apply a VirtualMCPServer (or MCPExternalAuthConfig) with:

authServerConfig:
  trustedIssuers:
    - name: some-issuer
      issuerUrl: "https://example.com"
  inboundGrants:
    tokenExchange:
      issuerPolicies:
        - issuerRef: some-issuer
          expectedAudience: "https://resource.example"
          allowedDelegateClients:
            - "*"
          # allowMayAct intentionally omitted

Result:

The VirtualMCPServer "..." is invalid: spec.authServerConfig.inboundGrants.tokenExchange.issuerPolicies[0]: Invalid value: "object": no such key: allowMayAct evaluating rule: allowMayAct must not be enabled when allowedDelegateClients contains the wildcard "*"

Expected: admission should succeed, since an omitted allowMayAct should be
treated as false (matching the legacy struct's already-correct behavior).

Suggested fix

Add the same has() guard the legacy rule already uses:

// +kubebuilder:validation:XValidation:rule="!(has(self.allowMayAct) && self.allowMayAct && '*' in self.allowedDelegateClients)",message="allowMayAct must not be enabled when allowedDelegateClients contains the wildcard \"*\""

How this was found

Found while re-verifying ~/devel/manifests/rfc8693-delegation-private-key-jwt-demo
against current main after migrating it off the deprecated
trustedIssuers[].allowedDelegateClients/allowMayAct fields onto the
canonical inboundGrants.tokenExchange.issuerPolicies[] shape. That demo
legitimately needs allowedDelegateClients: ["*"] (the delegate client
self-registers via DCR after the CRD is applied, so its client ID isn't
known yet) and omits allowMayAct, which is exactly the failing combination.
Worked around in the demo by setting allowMayAct: false explicitly.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go around line 830 and compare the canonical rule with the guarded legacy rule at line 397 and neighboring actorClaim rule at line 831. Verify the reproduced VirtualMCPServer or MCPExternalAuthConfig admission case with wildcard allowedDelegateClients and omitted allowMayAct; done means admission succeeds and the existing wildcard restriction remains enforced when allowMayAct is true.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
api, security
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
88/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.