Expose explicit primaryUpstreamProvider for Cedar authz on VirtualMCPServer#5199
Draft
tgrunnagle wants to merge 2 commits intomainfrom
Draft
Expose explicit primaryUpstreamProvider for Cedar authz on VirtualMCPServer#5199tgrunnagle wants to merge 2 commits intomainfrom
tgrunnagle wants to merge 2 commits intomainfrom
Conversation
Adds an optional primaryUpstreamProvider field to the inline authz config on VirtualMCPServer so users with multiple upstream IDPs can pin Cedar to a non-first provider, instead of being silently bound to whichever upstream happens to be listed first. Changes for issue #5197: - Add PrimaryUpstreamProvider to InlineAuthzConfig (shared type, vMCP-only in practice, mirroring the SubjectProviderName precedent on the token- exchange and AWS-STS strategies). - Switch the converter from unconditional first-upstream binding to an explicit-then-fallback resolution; both branches normalize through authserver.ResolveUpstreamName. - Reject the spec with AuthServerConfigValidated=False (AuthzUpstreamUnknown) when the explicit name does not match any declared upstream — Cedar would otherwise deny every request at runtime. - Suppress the AuthzUpstreamSelectionWarning advisory when the user has set the field explicitly; the auto-selection it warns about is no longer happening. - Extend converter and validator tests; regenerate CRD YAMLs and API docs. Existing manifests without the new field keep current behavior — the fallback branch is unchanged for that path.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5199 +/- ##
==========================================
+ Coverage 67.65% 67.71% +0.05%
==========================================
Files 607 607
Lines 61982 62060 +78
==========================================
+ Hits 41937 42023 +86
+ Misses 16883 16879 -4
+ Partials 3162 3158 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f24fcbe to
b5d2821
Compare
Fixed issues from code review: - MEDIUM: Reject explicit primaryUpstreamProvider when no embedded auth server is configured. The early-return direct-IdP branch in validateAuthzUpstreamAvailable now checks for a non-empty explicit name first and returns SpecValidationError with ConditionReasonAuthzUpstreamUnknown when set — closing the silent misconfiguration where the converter would forward an unresolvable name into Cedar config at runtime. - MEDIUM: Update the converter block comment so it accurately describes both rejection paths (mismatch with declared upstreams AND explicit name without an embedded AS), keeping the comment synchronized with the validator's behavior per the go-style.md rule. - MEDIUM: Replace the misleading "is normalized via ResolveUpstreamName" converter test with a case that actually exercises normalization (upstream Name:"" resolving to "default", user pinning to "default"). Removes redundancy with the single-upstream-honored case and matches the test's claimed assertion. - MEDIUM: Add validator test case for explicit primaryUpstreamProvider with no embedded auth server, locking the new rejection in.
b5d2821 to
b1499ed
Compare
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
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.
DRAFT - not ready for review
Summary
A VirtualMCPServer with multiple upstream IDPs configured on its embedded auth server has no way to point Cedar at a non-first upstream — the operator unconditionally binds
PrimaryUpstreamProviderto whichever upstream happens to be listed first in the spec. This forces users to reorder the list as a side-channel for an authz decision and offers no opt-out beyond editing the upstream order. This PR exposes an explicitprimaryUpstreamProviderfield on the inline authz config so users can pin Cedar's claim source, while preserving existing first-upstream behavior when the field is empty so existing manifests continue to work unchanged. Behavior brings Cedar/authz to parity with theSubjectProviderNameprecedent already established on the token-exchange and AWS-STS strategies ofMCPExternalAuthConfig.Closes #5197
Type of change
Test plan
task test)task lint-fix)API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.The new
primaryUpstreamProviderfield is optional withomitempty. Existing manifests without the field continue to use the first-upstream auto-binding fallback verbatim.Changes
cmd/thv-operator/api/v1beta1/mcpserver_types.goPrimaryUpstreamProviderto sharedInlineAuthzConfig(vMCP-only in practice, mirrorsSubjectProviderName).cmd/thv-operator/api/v1beta1/virtualmcpserver_types.goConditionReasonAuthzUpstreamUnknownfor the new rejection paths.cmd/thv-operator/pkg/vmcpconfig/converter.goauthserver.ResolveUpstreamName.cmd/thv-operator/controllers/virtualmcpserver_controller.goAuthServerConfigValidated=False,AuthzUpstreamUnknown); suppress theAuthzUpstreamSelectionWarningadvisory when an explicit choice is set; advisory message now points users at the new field.cmd/thv-operator/controllers/virtualmcpserver_controller_test.gocmd/thv-operator/pkg/vmcpconfig/converter_test.goResolveUpstreamName(""upstream +"default"explicit), and explicit-forwarded-without-AS contract.deploy/charts/operator-crds/files/crds/*.yaml,templates/*.yaml,docs/operator/crd-api.mdDoes this introduce a user-facing change?
Yes. Users authoring
VirtualMCPServermanifests can now setspec.incomingAuth.authzConfig.inline.primaryUpstreamProviderto explicitly choose which upstream IDP's access token claims Cedar evaluates. When unset, behavior is unchanged: the controller defaults to the first configured upstream and emits the existingAuthzUpstreamSelectionWarningadvisory if multiple upstreams are declared. Setting the field to a name that does not match any declared upstream — or setting it without an embedded auth server — now rejects the spec at admission rather than failing silently at request time.Implementation plan
Approved implementation plan
Sub-issue of #5146, addressing root cause 1: the operator auto-binds
PrimaryUpstreamProviderto the first upstream provider's name without exposing a CRD field that lets users override or opt out.PrimaryUpstreamProvider stringto the sharedInlineAuthzConfig(matches theSubjectProviderNameprecedent on token-exchange/AWS-STS strategies — also defined on shared types but documented as vMCP-only).ResolveUpstreamNameon the user-supplied value too, so the field is normalized consistently with the auto-selected path.validateAuthzUpstreamAvailable): Reject (SpecValidationError,AuthServerConfigValidated=False) when an explicitPrimaryUpstreamProvideris set but does not match any entry inSpec.AuthServerConfig.UpstreamProvidersafterResolveUpstreamNamenormalization on both sides. Suppress theAuthzUpstreamSelectionWarningadvisory when the user has set the field explicitly. Leave the existing "embedded AS but zero upstreams" reject path unchanged. (Code review added: also reject when an explicit name is set with no embedded auth server at all.)task operator-generate,task operator-manifests,task -d cmd/thv-operator crdref-gen.Special notes for reviewers
InlineAuthzConfigtype (also reachable fromMCPServerandMCPRemoteProxy) but is documented as vMCP-only and has no effect elsewhere. This matches theSubjectProviderNameprecedent. The alternative — introducing a vMCP-specificVirtualMCPInlineAuthzConfigandAuthzConfigRef— was rejected as a larger refactor with knock-on changes in the converter,controllerutil.GenerateOrUpdateInlineAuthzConfigMap, and tests. Happy to revisit if reviewers prefer the stricter typing.pkg/authz/authorizers/cedar/core.goand will be a separate change. This PR only makes the auto-binding overridable.Address code review feedback) reflects feedback from/review-code-for-issueand adds the "explicit name without embedded AS" rejection plus a converter test that actually exercisesResolveUpstreamNamenormalization (the previous test was misleading becauseResolveUpstreamNameis the identity function for non-empty input).