Enforce Cedar policies on upstream IDP token claims - #4448
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4448 +/- ##
==========================================
- Coverage 69.64% 69.63% -0.02%
==========================================
Files 491 497 +6
Lines 50304 50685 +381
==========================================
+ Hits 35036 35294 +258
- Misses 12580 12682 +102
- Partials 2688 2709 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
Fixed all issues from code review of issue #4408: - CRITICAL: Add groups []string to authorizePromptGet, authorizeResourceRead, and authorizeFeatureList; update CreateEntitiesForRequest call sites to pass groups in each. Fix existing test call sites for updated signatures. - HIGH: Rewrite claims resolution as a true IIFE (direct assignment, not a stored function variable) in AuthorizeWithJWTClaims. Populate identity.Groups after group extraction. Implement injectUpstreamProviderIfNeeded in pkg/runner/middleware.go and call it in PopulateMiddlewareConfigs. Add PrimaryUpstreamProvider to vMCP AuthzConfig and propagate it to cedar options in the factory. - MEDIUM: Move ExtractGroupsFromClaims and defaultGroupClaimNames from context.go to identity.go. Add GroupClaimName field to cedar.ConfigOptions. Document IsAuthorized group entity limitation. Change ExtractGroupsFromClaims to accept jwt.MapClaims directly (removes redundant cast). - Add comprehensive unit tests: ExtractGroupsFromClaims (13 cases), parseUpstreamJWTClaims (5), AuthorizeWithJWTClaims upstream-provider path (5), group membership Cedar evaluation (3), identity.Groups population (1), custom group claim name (1), InjectUpstreamProvider (3), CreatePrincipalEntity with groups (4), CreateEntitiesForRequest with groups (3), injectUpstreamProviderIfNeeded (5). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixed issues from second round of code review: - HIGH: Remove identity.Groups mutation from AuthorizeWithJWTClaims to respect the Identity immutability contract after context placement - HIGH: Update addAuthzMiddleware in config_builder.go to inject the upstream provider name, matching the operator path behavior - MEDIUM: Use authserver.ResolveUpstreamName instead of hardcoded "default" literal in addUpstreamSwapMiddleware - MEDIUM: Make InjectUpstreamProvider a no-op for non-Cedar configs rather than returning an error - MEDIUM: Add TestAddAuthzMiddleware_InjectsUpstreamProvider to cover the CLI code path for upstream provider injection - MEDIUM: Add TestNewCedarAuthzMiddleware_PropagatesPrimaryUpstreamProvider to verify PrimaryUpstreamProvider wiring in the vMCP factory Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixed issues from third round of code review: - MEDIUM: Use authserver.ResolveUpstreamName in injectUpstreamProviderIfNeeded to eliminate divergent inline normalisation logic - MEDIUM: Document that InjectUpstreamProvider overwrites any existing PrimaryUpstreamProvider — godoc update only - MEDIUM: Propagate authz.LoadConfig errors in addAuthzMiddleware instead of silently falling back to ConfigPath-only mode - MEDIUM: Add upstream_token_has_no_sub_claim test case covering the ErrMissingPrincipal path when upstream JWT has no sub claim - MEDIUM: Add TestInjectUpstreamProvider_NonCedarPassThrough to verify the no-op contract for non-Cedar authorizer types - MEDIUM: Add group_claim_name_preserved_after_inject test case verifying GroupClaimName survives the InjectUpstreamProvider round-trip Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- move `extractGroupsFromClaims` to `cedar/core.go` - add more groups tests
10c730c to
2ee4cff
Compare
JAORMX
left a comment
There was a problem hiding this comment.
LGTM, left some questions and minor follow-ups.
- Move `THVGroup` into constant and document - Document source of default group claim names
…embedded auth server is active (#4529) When a `VirtualMCPServer` uses the embedded authorization server alongside a `token_exchange` outgoing auth strategy, omitting `subjectProviderName` caused the strategy to silently fall back to `identity.Token` (the ToolHive-issued JWT) as the RFC 8693 subject token. The exchange endpoint rejects the ToolHive JWT, but the failure was opaque — nothing in the error indicated that `subjectProviderName` needed to be set. This mirrors the same footgun that was fixed for Cedar authorization policies in #4448 with `injectUpstreamProviderIfNeeded`. - Added `injectSubjectProviderIfNeeded` to the operator controller (`virtualmcpserver_controller.go`) to auto-populate `SubjectProviderName` on `token_exchange` strategies where it is empty, using the first upstream from `vmcp.Spec.AuthServerConfig` (resolved via `authserver.ResolveUpstreamName`, same logic as Cedar). Applied to both the default strategy and all inline per-backend strategies. - Added `InjectSubjectProviderNames` to `pkg/vmcp/config/defaults.go` for the YAML config path, so the same defaulting applies when the vMCP binary is run directly with an `authserver-config.yaml` sibling file. - Called `config.InjectSubjectProviderNames` in `cmd/vmcp/app/commands.go` immediately after loading the auth server config, before the embedded auth server is started. - Updated the `SubjectProviderName` field comments in `MCPExternalAuthConfig` (`mcpexternalauthconfig_types.go`) and `TokenExchangeConfig` (`pkg/vmcp/auth/types/types.go`) to document the auto-population behavior. - Regenerated CRD manifests and API docs to reflect the updated field comment.
…embedded auth server is active (stacklok#4529) When a `VirtualMCPServer` uses the embedded authorization server alongside a `token_exchange` outgoing auth strategy, omitting `subjectProviderName` caused the strategy to silently fall back to `identity.Token` (the ToolHive-issued JWT) as the RFC 8693 subject token. The exchange endpoint rejects the ToolHive JWT, but the failure was opaque — nothing in the error indicated that `subjectProviderName` needed to be set. This mirrors the same footgun that was fixed for Cedar authorization policies in stacklok#4448 with `injectUpstreamProviderIfNeeded`. - Added `injectSubjectProviderIfNeeded` to the operator controller (`virtualmcpserver_controller.go`) to auto-populate `SubjectProviderName` on `token_exchange` strategies where it is empty, using the first upstream from `vmcp.Spec.AuthServerConfig` (resolved via `authserver.ResolveUpstreamName`, same logic as Cedar). Applied to both the default strategy and all inline per-backend strategies. - Added `InjectSubjectProviderNames` to `pkg/vmcp/config/defaults.go` for the YAML config path, so the same defaulting applies when the vMCP binary is run directly with an `authserver-config.yaml` sibling file. - Called `config.InjectSubjectProviderNames` in `cmd/vmcp/app/commands.go` immediately after loading the auth server config, before the embedded auth server is started. - Updated the `SubjectProviderName` field comments in `MCPExternalAuthConfig` (`mcpexternalauthconfig_types.go`) and `TokenExchangeConfig` (`pkg/vmcp/auth/types/types.go`) to document the auto-population behavior. - Regenerated CRD manifests and API docs to reflect the updated field comment.
Summary
identity.UpstreamTokenswere never read.Identity.Groupswas never populated despite a code comment saying authorization logic "MUST" do this extraction, andCreatePrincipalEntityalways produced emptyParentssets, makingprincipal in THVGroup::"engineering"always evaluate to false.PrimaryUpstreamProviderconfig option to the Cedar authorizer so it can read upstream IDP token claims, addsExtractGroupsFromClaimsto populate groups from those claims, and updatesCreatePrincipalEntity/CreateEntitiesForRequestto build theTHVGroupparent entity hierarchy so Cedar'sinoperator works for group membership.Closes #4408
Type of change
Test plan
task test)task lint-fix)Changes
pkg/auth/identity.goExtractGroupsFromClaimswith configurable claim name and well-known defaults (groups,roles,cognito:groups)pkg/authz/authorizers/cedar/core.goPrimaryUpstreamProviderandGroupClaimNametoConfigOptions/Authorizer; addparseUpstreamJWTClaims; addInjectUpstreamProvider; branch inAuthorizeWithJWTClaimsto resolve claims from upstream token when configured; pass extracted groups into all authorize sub-methodspkg/authz/authorizers/cedar/entity.goCreatePrincipalEntityto accept agroupsslice and createTHVGroupparent entities; updateCreateEntitiesForRequestto accept and forward groupspkg/runner/middleware.goinjectUpstreamProviderIfNeededhelper; call it when building the authz middleware config so Cedar uses upstream IDP claims whenever the embedded auth server is activepkg/runner/config_builder.goembeddedAuthServerCfgintoaddAuthzMiddlewareso the Kubernetes operator code path also injects the upstream provider namepkg/vmcp/config/config.goPrimaryUpstreamProviderfield toAuthzConfigpkg/vmcp/auth/factory/incoming.goPrimaryUpstreamProviderfrom vMCPAuthzConfigintocedar.ConfigOptionsDoes this introduce a user-facing change?
Operators using Cedar policies with the embedded auth server can now write policies that reference upstream IDP claims (e.g. a GitHub
loginor Oktagroupsclaim) by settingprimary_upstream_providerin the Cedar config options. Group-based policies usingprincipal in THVGroup::"<group-name>"now evaluate correctly when the upstream token carries a standard group claim.Special notes for reviewers
The upstream token's JWT signature is intentionally not re-verified. The token was already validated by the IDP during the OAuth 2.0 code exchange; re-verification would require a round-trip to the IDP's JWKS endpoint on every authorization call and provides no security benefit in this trust model. If the upstream token is an opaque (non-JWT) token rather than a JWT access token, the authorizer returns an error and denies the request — there is no silent fallback to ToolHive claims, which is deliberate to prevent misconfigured deployments from authorizing against unintended claims.
The
InjectUpstreamProviderfunction incore.gois a no-op for non-Cedar authorizer configs, making it safe to call unconditionally from the runner middleware without needing to know the authorizer type ahead of time.Large PR Justification
Generated with Claude Code