Repository navigation
Expose group-based upstream filter via vMCP CRD - #6740
Draft
hellouz818 wants to merge 3 commits into
Draft
hellouz818 wants to merge 3 commits into
hellouz818 wants to merge 3 commits into
Conversation
JWT/OIDC claim values that are string arrays commonly arrive at Go as either []string (from typed JWT libraries) or []any whose elements are all strings (from standard encoding/json). Cedar's extractGroups and addMultiValuedClaimSets, and the vmcp xaa strategy, each handle this split independently today. Extract that normalization into pkg/auth/claims so the upcoming group-based upstream filter can consume it without introducing another duplicate, and so a follow-up refactor can collapse the existing Cedar/xaa variants onto the same helper without changing their observable behavior. The helper is strict by design: nil values, non-array types, and non-string array elements are treated as errors so a security-relevant caller can rely on it to fail closed on malformed claims. Returned slices never alias the caller's input. Refs: stacklok#6675
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6740 +/- ##
==========================================
+ Coverage 79.34% 79.35% +0.01%
==========================================
Files 802 804 +2
Lines 81211 81610 +399
==========================================
+ Hits 64436 64761 +325
- Misses 16770 16844 +74
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hellouz818
force-pushed
the
feature/6675-pr3-crd
branch
from
October 2, 2026 08:41
b6542fd to
40e2300
Compare
VirtualMCPServer currently walks every configured upstream in order for every user. When a user is missing an account on one of the upstreams (e.g. Salesforce), the login they are forced through there fails and the whole authorization fails — even though the otherwise-valid upstream chain is identity-complete. Introduce a built-in GroupBasedFilter implementing the existing handlers.UpstreamFilter hook. The filter reads a configurable claim (default "groups") from the first upstream's resolved principal and returns the union of UpstreamProviders from every matching rule, with a fallback to defaultUpstreams for valid-but-empty or unmatched groups. Missing or malformed claims fail the authorization per the filter's fail-closed contract; principal.Claims is treated as read-only. Wire the new filter through authserver.RunConfig so operator-supplied configuration can turn it on, with the same static invariants (unknown references, mandatory-first references, two-upstream minimum, duplicates, empty values) rejected both at RunConfig.Validate startup and at filter construction. This is step two of three; the matching VirtualMCPServer CRD surface and conversion follow in a separate commit so each layer is reviewable independently. Refs: stacklok#6675
Surface the auth-server GroupBasedFilter through VirtualMCPServerSpec so operators can configure per-user upstream chain narrowing with YAML, without reaching into the embedded auth-server RunConfig directly. The new AuthServerUpstreamFilter field lives beside AuthServerConfig (rather than nested inside EmbeddedAuthServerConfig, which is shared with MCPExternalAuthConfig and has no meaning for multi-upstream filtering). CEL at admission enforces the two cheap cross-field invariants Kubernetes can check cheaply: the field requires authServerConfig, and at least two upstream providers must be configured. The two deeper reference-integrity checks the plan called for — every referenced provider must name a configured non-first upstream, and the mandatory first provider must not be referenced — were originally expressed as CEL but produced a per-schema x-kubernetes-validations cost estimate 100x over the per-cluster budget in realistic deployments where upstreamProviders is unbounded. They are enforced at reconciliation time instead, by validateAuthServerUpstreamFilter in the VirtualMCPServer controller, which surfaces a terminal AuthServerConfigValidated=False condition. A proper restoration would require adding MaxItems bounds on the shared upstreamProviders type to tame the cost estimator; keeping that out of scope here keeps the shared EmbeddedAuthServerConfig type unchanged for MCPExternalAuthConfig consumers. Reconciliation also protects against admission bypass (fake client, pre-CEL stored object): the full Go filter constructor runs again against the configured upstream list before the ConfigMap is written, rejecting malformed configuration before it reaches a crash-looping pod. The vMCP converter extends the shared BuildAuthServerRunConfig call with the vMCP-only field translation, keeping the shared builder free of vMCP-specific concerns. Adding the new filter construction to newEmbeddedAuthServerWithStorage pushes its cyclomatic complexity one step over the package ceiling. The function is a sequential auth-server bootstrap — each step is a required stage with its own error-return — so the complexity is a property of the ordered boot flow rather than avoidable branching. A gocyclo suppression with an explanatory docstring matches the shape other sequential bootstraps elsewhere in the repository already use (rmCmdFunc, runCmdFunc, MCPServerReconciler.Reconcile), and keeps the error-cleanup defer block in the same form the original upstream code uses, rather than extracting a helper whose only purpose would be to reshape the complexity count. Generated artifacts (deepcopy, CRD manifests, CRD reference) are regenerated via task operator-generate, task operator-manifests, and task crdref-gen. Refs: stacklok#6675
hellouz818
force-pushed
the
feature/6675-pr3-crd
branch
from
October 8, 2026 01:59
40e2300 to
e599d67
Compare
5 tasks done
This branch has not been deployed
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
PR 3/3 of #6675. Surfaces the auth-server
GroupBasedFilter(added in PR 2/3) throughVirtualMCPServerSpecso operators can configure per-user upstream chain narrowing with YAML.The new
AuthServerUpstreamFilterfield lives besideAuthServerConfig(rather than nested inside the sharedEmbeddedAuthServerConfig, which is also used byMCPExternalAuthConfigwhere multi-upstream filtering has no meaning). Admission-time CEL enforces two cheap cross-field invariants: the field requiresauthServerConfig, and at least two upstream providers must be configured. Reconciliation re-runs the full Go filter constructor against the configured upstream list so a resource that bypassed admission (fake client, pre-CEL stored object) is rejected with a terminalAuthServerConfigValidated=Falsecondition before its ConfigMap is written.The vMCP converter extends the shared
BuildAuthServerRunConfigcall with the vMCP-only field translation, keeping the shared builder free of vMCP-specific concerns.VirtualMCPServerSpec+ CEL + OpenAPI markersConvertUpstreamFilter) layered onBuildAuthServerRunConfigvalidateAuthServerUpstreamFilter)Stacked on #6739. Refs #6675
Type of change
Test plan
task test) — 6 cases forvalidateAuthServerUpstreamFilter, 4 cases forConvertUpstreamFiltertest-integration/virtualmcp/virtualmcpserver_upstreamfilter_cel_test.goexercising the CEL rules through the real apiservertask lint-fix) — 0 issuesslackupstream, successfully invokes slack tools via claude-code, withatlassian/cobi/salesforceupstream tokens correctly absent (confirmed via operator audit logs and absent-token warnings)API Compatibility
v1beta1API.New optional field on
VirtualMCPServerSpec. Omitting it preserves pre-filter behavior (full chain). No existing CRs change meaning.Does this introduce a user-facing change?
Yes — new optional field
VirtualMCPServer.spec.authServerUpstreamFilterlets operators narrow the OAuth chain per user based on an identity claim. See the "Per-User Upstream Filtering" section indocs/arch/10-virtual-mcp-architecture.mdfor the configuration shape and semantics.Special notes for reviewers
CEL cost budget
The two deeper reference-integrity CEL rules the issue plan called for (every referenced provider must name a configured non-first upstream; mandatory first provider must not be referenced) were originally implemented but produced a per-schema
x-kubernetes-validationscost estimate over 100× the per-cluster budget in realistic deployments whereupstreamProvidersis unbounded.They are enforced at reconciliation time instead, by
validateAuthServerUpstreamFilter, which surfaces a terminalAuthServerConfigValidated=Falsecondition. The reconciler calls the same Go constructor used in PR 2/3, so the admission vs. reconciliation path contains no logic drift.A proper restoration path would add
MaxItemsbounds on the sharedupstreamProviderstype to tame the cost estimator; keeping that out of scope here keeps the sharedEmbeddedAuthServerConfigtype unchanged forMCPExternalAuthConfigconsumers. Happy to open a follow-up PR for the bounded-list restoration if preferred.Note on OAuth-flow E2E
The issue plan includes a functional E2E deliverable. This PR does not add a full in-repo OAuth-flow E2E; admission behavior is covered through envtest and the end-to-end flow has been validated manually against a real mcphub deployment. If maintainers prefer the automated OAuth E2E to land as part of #6675, I can add it as a follow-up in this series.
This follows the test layering used for the SPIFFE CRD exposure in #6500: envtest coverage for admission behavior, without a full OAuth-flow E2E.
Follow-up work tracked
pkg/auth/claimshelper from PR 1/3.MaxItemsbounds on the shared upstream type.