Skip to content

Guard token exchange may_act CEL - #6532

Open
kocaemre wants to merge 1 commit into
stacklok:mainfrom
kocaemre:fix/token-exchange-allowmayact-cel
Open

Guard token exchange may_act CEL#6532
kocaemre wants to merge 1 commit into
stacklok:mainfrom
kocaemre:fix/token-exchange-allowmayact-cel

Conversation

@kocaemre

@kocaemre kocaemre commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Canonical token-exchange issuer policies currently reject a valid wildcard delegate-client policy when allowMayAct is omitted, because the CEL rule dereferences the optional field without has().
  • Add the same has(self.allowMayAct) guard already used by the legacy issuer-policy rule, regenerate the CRDs/Helm-wrapped CRDs, and cover the omitted-allowMayAct wildcard case in the envtest CEL suite.

Fixes #6531

Type of change

  • Bug fix
  • New feature
  • Refactoring (no behavior change)
  • Dependency update
  • Documentation
  • Other (describe):

Test plan

  • Unit tests (task test)
  • E2E tests (task test-e2e)
  • Linting (task lint-fix)
  • Manual testing (describe below)

Commands run:

PATH=/tmp/go1.26/bin:/usr/local/go/bin:/root/go/bin:$PATH task -d cmd/thv-operator operator-manifests
# passed; regenerated CRDs and Helm-wrapped CRDs

PATH=/tmp/go1.26/bin:/usr/local/go/bin:/root/go/bin:$PATH \
KUBEBUILDER_ASSETS="$($(go env GOPATH)/bin/setup-envtest use 1.31.0 -p path)" \
go test -ldflags=-extldflags=-Wl,-w -v ./cmd/thv-operator/test-integration/mcp-external-auth \
  -run '^TestControllers$' \
  -args -ginkgo.focus='canonical token exchange wildcard delegate with omitted may_act'
# passed: 1 passed, 120 skipped

PATH=/tmp/go1.26/bin:/usr/local/go/bin:/root/go/bin:$PATH \
KUBEBUILDER_ASSETS="$($(go env GOPATH)/bin/setup-envtest use 1.31.0 -p path)" \
go test -ldflags=-extldflags=-Wl,-w -v ./cmd/thv-operator/test-integration/mcp-external-auth -run '^TestControllers$'
# passed: 121 passed

PATH=/tmp/go1.26/bin:/usr/local/go/bin:/root/go/bin:$PATH task operator-test
# passed

git diff --check origin/main..HEAD
# passed

I also ran task lint; it failed before reaching any changed file on two existing gci formatting findings in pkg/authserver/server/provider.go and pkg/authserver/server_impl.go, so I did not include lint-fix churn in this focused PR.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

This relaxes an admission rule so omitted allowMayAct is handled as false, matching the legacy issuer-policy rule. No fields are added, removed, or renamed.

Changes

File Change
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.go Guard optional allowMayAct access in the canonical token-exchange CEL rule.
cmd/thv-operator/test-integration/mcp-external-auth/inbound_grants_cel_test.go Add an envtest regression for wildcard delegate clients with omitted allowMayAct.
deploy/charts/operator-crds/** Regenerate CRDs and Helm-wrapped CRDs.

Does this introduce a user-facing change?

Yes. A canonical inboundGrants.tokenExchange.issuerPolicies[] entry may now omit allowMayAct while using allowedDelegateClients: ["*"]; admission treats the omitted optional bool as false instead of failing with no such key: allowMayAct.

@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.73%. Comparing base (1e80f9d) to head (8ebd5e5).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6532      +/-   ##
==========================================
+ Coverage   78.69%   78.73%   +0.04%     
==========================================
  Files         777      777              
  Lines       76797    76797              
==========================================
+ Hits        60434    60466      +32     
+ Misses      16358    16326      -32     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>
@kocaemre
kocaemre force-pushed the fix/token-exchange-allowmayact-cel branch from a20f7cb to 8ebd5e5 Compare September 8, 2026 11:02
@kocaemre

kocaemre commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Refreshed this branch onto current main (1e80f9de) and force-pushed rebased head 8ebd5e5b.

No code changes beyond replaying the existing PR commit on top of current upstream.

Why I refreshed: the prior CI run had a single failing E2E Test Lifecycle (kindest/node:v1.33.7) job. The failed spec was VirtualMCPServer Redis-Backed Session Sharing ... Should allow a session established on pod A to be reconstructed on pod B (virtualmcp_redis_session_test.go:306), with the assertion that pod B returned an empty tool list; the remaining reported failures were Ginkgo interruptions after that first failure. That path appears unrelated to this PR's CEL validation/schema change.

Local verification in this cron environment:

git diff --check origin/main..HEAD
# passed

git log --format='%h %s%n%b' origin/main..HEAD
# 8ebd5e5b Guard token exchange may_act CEL
# Signed-off-by: Emre K <110906681+kocaemre@users.noreply.github.com>

I attempted the targeted operator integration task, but local execution is blocked by the cron shell's old Go toolchain:

task operator-test-integration -- mcp-external-auth/inbound_grants_cel_test.go
# go install sigs.k8s.io/controller-runtime/tools/setup-envtest@release-0.22
# go.mod:3: invalid go version '1.24.0': must match format 1.23
# task: Failed to run task "operator-test-integration": exit status 1

GitHub CI is running again on the refreshed head; current status at the time of this comment is queued/in-progress for the ToolHive checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant