Repository navigation
Add group-based upstream filter for vMCP auth - #6739
Merged
Merged
Conversation
hellouz818
changed the title
7 tasks done
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6739 +/- ##
==========================================
+ Coverage 79.48% 79.55% +0.06%
==========================================
Files 813 814 +1
Lines 82415 82534 +119
==========================================
+ Hits 65505 65656 +151
+ Misses 16905 16873 -32
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hellouz818
force-pushed
the
feature/6675-pr2-filter
branch
2 times, most recently
from
October 8, 2026 01:39
7844e3a to
d47f086
Compare
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). The Cedar authorizer decodes that split inline today, and the upcoming group-based upstream filter needs the same decoding with a stricter contract. Add pkg/auth/claims so neither caller has to carry its own copy. The decoder in this commit is strict by design: nil values, non-array types, and non-string array elements are errors, so a security-relevant caller can rely on it to fail closed on a malformed claim. Returned slices never alias the caller's input. The next commit moves Cedar onto the same package, which needs a second, permissive contract to preserve its current behavior. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
The decoder added in the previous commit is strict by design, and Cedar cannot use it as-is. Cedar reads a present-but-malformed group claim as "the IdP asserted no usable names" and relies on that to suppress the fallback to well-known claim names. Decoding it to nil instead would let an explicitly empty custom claim fall through to "roles" and grant access the operator did not intend. Add DecodeStringArrayLenient next to the strict decoder, preserving Cedar's existing semantics: non-string elements are dropped from []any, empty or typed-nil []any values produce a non-nil empty slice, and []string values retain their nil-ness. Rewrite extractGroups on top of it, so both contracts live in one place and the difference between them is documented rather than implicit. The one deliberate change is that the result no longer aliases a []string claim value. authorizeWithClaims appends the role claim onto the slice returned for the group claim, which could write a role name into the token's own backing array when that array had spare capacity. Cover the lenient decoder with a table of every shape where it diverges from the strict one, and add Cedar cases for []string(nil), []any(nil), empty arrays, all-non-string elements, and aliasing. The existing test pinning the present-but-empty fallback semantic passes unchanged. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
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 Signed-off-by: hellouz818 <hellouz818@gmail.com>
hellouz818
force-pushed
the
feature/6675-pr2-filter
branch
from
October 12, 2026 08:12
d47f086 to
f0cc7d7
Compare
Resolving the main merge conflict in the web editor dropped the comma after the expected schema count and left a stray blank line, so the SDK module stopped compiling. The count itself is correct: the merge base asserted 197 schemas, main added one and this branch adds two, so the merged contract holds 200. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
hellouz818
marked this pull request as ready for review
October 12, 2026 09:19
hellouz818
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
October 12, 2026 09:19
JAORMX
previously approved these changes
Oct 12, 2026
Collaborator
|
suggestion (non-blocking): Could we narrow “operator-internal” to “not yet exposed through the CRD”? The existing vMCP config loader can already consume this field from The change is additive, so I am not seeing a compatibility problem. We should just describe the surface accurately. |
The operator writes authserver-config.yaml and vMCP reads it back with gopkg.in/yaml.v3, which uses yaml tags rather than the json tags covered by the existing round-trip test. Decode a literal document covering a custom claim name, multiple rules, and defaultUpstreams, then re-encode and decode it again. The literal pins the expected YAML key names: a marshal/unmarshal round-trip alone can still pass if both sides use the same renamed tag. A second subtest verifies that an unset filter is omitted from YAML and decodes to nil. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
The filter reads its claim from the identity the first upstream resolves, and not every upstream path populates one. An OIDC upstream fills the claims map from the validated ID token and a pure OAuth2 upstream from the userinfo response, while identityFromToken and synthesized identities leave it empty. Login keeps working until the filter is enabled, and defaultUpstreams does not soften the failure: an absent claim fails closed rather than falling back. Record that on the Claim field, and note on the struct why its nested keys are camelCase where RunConfig's own fields are snake_case. The field comment is the source of the OpenAPI property description, so regenerate docs/server. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
Every matching rule in the existing fixture lists a single group, so none of them distinguishes any-of from all-of: flipping ruleMatches to require every group leaves the whole suite green. Add a rule listing two groups and assert that each one selects it on its own, that both together select it once, and that an unrelated group falls back. DefaultUpstreams is a different set from the rule's providers so a fallback cannot be mistaken for a match. Issue stacklok#6675 leaves the preferred semantic open, so this is the test a move to all-of would have to update. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
The filter reaches the callback handler through RunConfig, and nothing covered that leg: the existing integration test injects a stub straight into Config, so dropping upstreamFilter from resolvedCfg leaves the whole suite green. Build the server from RunConfig and drive the upstream authorization redirects and callbacks manually, recording each upstream authorize endpoint visited. A viewer visits Okta then Slack and receives an authorization code without being redirected to Salesforce. A missing claim fails on the first callback without issuing a code, even with defaultUpstreams configured. An unset filter visits all configured upstreams and issues a code. The helper stops at the client redirect without exchanging the code for tokens, keeping the test focused on filter wiring and chain selection. Upstream secrets go through files because the environment-variable form cannot be combined with t.Parallel, and the upstream scope list asks for "groups" because mockoidc only emits that claim when it is requested. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
The SDK contract carries schema descriptions, so documenting the Claim field left sdk/go stale and failed the Go SDK verification job. Rerun task sdk-generate. Refs: stacklok#6675 Signed-off-by: hellouz818 <hellouz818@gmail.com>
Contributor
Author
|
@JAORMX Thanks for the thorough review! All four suggestions are addressed, with no force-push, so the changes since
Static Analysis is failing on the goreleaser-action pin comment in The new commits dismissed the previous approval. Could you take another look |
JAORMX
approved these changes
Oct 13, 2026
JAORMX
approved these changes
Oct 13, 2026
1 task
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
2 participants
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
VirtualMCPServer currently walks every configured upstream in order for every user. A user without an account on one of the upstreams (e.g. Salesforce) is forced through that OAuth leg and fails there, failing the whole authorization even though the otherwise-valid identity chain is complete.
This PR introduces a built-in
GroupBasedFilterimplementing the existinghandlers.UpstreamFilterhook. The filter reads a configurable claim (default"groups") from the first upstream's resolved principal and returns the union ofUpstreamProvidersfrom every matching rule, with a fallback todefaultUpstreamsfor valid-but-empty or unmatched groups. If no rule matches anddefaultUpstreamsis unset, no optional upstreams are selected; the mandatory first upstream remains the only authentication leg. Missing or malformed claims fail the authorization per the filter's fail-closed contract;principal.Claimsis treated as read-only.The new filter is wired through
authserver.RunConfigso operator-supplied configuration can turn it on, with static invariants (unknown references, mandatory-first references, two-upstream minimum, duplicates, empty values) rejected both atRunConfig.Validatestartup and at filter construction.pkg/authserver/upstreamfilter/— new package with filter + config + unit testspkg/authserver/config.go—RunConfig.UpstreamFilterfield +Validate()extensionpkg/authserver/runner/embeddedauthserver.go— filter construction with typed-nil-safe patternpkg/authserver/runner/embeddedauthserver_upstreamfilter_test.go— integration coverage for the RunConfig → filter → callback pathRefs #6675
Type of change
Test plan
task test) — 24 cases inupstreamfilterplus a multi-group rule table pinning any-of matching; 6 validation cases and JSON and YAML round-trip cases inauthserver/config_test.goauthserver/runnerbuild the server from aRunConfigand drive the authorization redirects: a narrowed chain (okta → slack, never redirected to salesforce), a missing claim failing on the first callback, and an unset filter visiting every upstreamtask lint-fix) — 0 issuesAPI Compatibility
v1beta1API.(The new field is optional and its nil default preserves pre-filter behavior.
RunConfigis not part of thev1beta1CRD surface, but it is not internal either: the vMCP config loader already reads this field fromauthserver-config.yaml, and the model is published through the OpenAPI document and the Go SDK. CRD exposure follows in #6740.)Does this introduce a user-facing change?
No CRD-level change yet. The field is reachable through
authserver.RunConfig, which the vMCP config loader reads fromauthserver-config.yamland which ships in the OpenAPI document and the Go SDK; operator-facing CRD exposure comes in #6740.Special notes for reviewers
PR 2/3 of #6675.
Open design question (from issue thread)
TestFilterUpstreams_MultiGroupRuleMatchesAnyGroup. Change scope is a single predicate (ruleMatches) plus that test if the preferred semantic turns out to be all-of (AND).