-
Notifications
You must be signed in to change notification settings - Fork 171
Add embeddedAuthServer type to MCPExternalAuthConfig CRD #3488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Add support for embedded OAuth2/OIDC authorization server configuration in the MCPExternalAuthConfig CRD. This enables MCP servers in Kubernetes to integrate with an embedded auth server that delegates to upstream IDPs. New types added: - EmbeddedAuthServerConfig: Main config with issuer, signing keys, HMAC secrets, token lifespans, upstream providers, and allowed audiences - TokenLifespanConfig: Duration settings for access/refresh/auth code tokens - UpstreamProviderConfig: Upstream IDP config with OIDC/OAuth2 support - OIDCUpstreamConfig: OIDC-specific configuration with discovery support - OAuth2UpstreamConfig: OAuth2-specific configuration with explicit endpoints Webhook validation ensures: - Mutual exclusivity between auth config types - Upstream provider type matches its config (oidc/oauth2) - Currently only one upstream provider is supported Closes stacklok/stacklok-epics#226
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 #3488 +/- ##
==========================================
+ Coverage 65.15% 65.25% +0.09%
==========================================
Files 398 399 +1
Lines 38821 38960 +139
==========================================
+ Hits 25295 25422 +127
- Misses 11564 11568 +4
- Partials 1962 1970 +8 ☔ 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. |
jhrozek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the audiences field should be removed, I think it would be also nice to at least provide a nice default for scopes and redirectUrl
| // Per RFC 8707, the "resource" parameter in authorization and token requests is | ||
| // validated against this list. | ||
| // +optional | ||
| AllowedAudiences []string `json:"allowedAudiences,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the AllowedAudiences should be part of this type - sorry I missed that in the initial RFC. The fact that we even have an array in the Go package was me planning for the shared authserver deployment where we'd track the allowed audiences based on what servers are using this authserver instance.
I think an interesting dichotomy here is - each MCPServer will get its own authserver instance. This means that we can infer the allowed audience from this server's oidcConfig.inline.audience (or equivalent for oidc.Config.configMap/kubernetes). We can also default the scopesSupported to the scopes from that section.
However, I think it would be nice UX if the externalauthconfig could be shared and used by multiple MCPServer instances - in that case audience must not be shared (and thus not part of this CRD).
It's a similar issue with the scopes parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm re-reading my comment I realize that maybe it might be easier to understand with an example: we'd have a single MCPExternalAuthConfig pointing with an AuthServer definition with the same issuer (although the issuer could very well default to resourceUrl I guess). The MCPExternalAuthConfig would list a github IDP, pointing to a secret and client_id.
This ExternalAuthConfig could then be linked to by a MCP server that allows listing github issues (aud=mcp-github-issues-lister, scopes=[github:issues]) and another MCP server that allows reading github repos (aud=mcp-github-repos-reader, scopes=[github:repos])
|
|
||
| const ( | ||
| // UpstreamProviderTypeOIDC is for OIDC providers with discovery support | ||
| UpstreamProviderTypeOIDC UpstreamProviderType = "oidc" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I haven't merged the OIDC provider yet. Will do soon.
|
|
||
| // RedirectURI is the callback URL where the upstream IDP will redirect after authentication. | ||
| // +kubebuilder:validation:Required | ||
| RedirectURI string `json:"redirectUri"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another piece that could prevent from sharing the mcpexternalauthconfig CR - I wonder if we could default to resourceUrl/oauth/callback?
| return nil | ||
| } | ||
|
|
||
| // Note: MinItems=1 and MaxItems=1 are enforced by kubebuilder markers, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: only minItems exists
| if r.Spec.BearerToken != nil { | ||
| return fmt.Errorf("bearerToken must not be set when type is 'tokenExchange'") | ||
| } | ||
| if r.Spec.EmbeddedAuthServer != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i see lots of code duplication here, maybe we can create a helper for validating embeddedauthserver, passing some param for different error logs?
Summary
Add the
embeddedAuthServerauthentication type to theMCPExternalAuthConfigCRD, enabling MCP servers in Kubernetes to integrate with an embedded OAuth2/OIDC authorization server.Issue: stacklok/stacklok-epics#226
Changes
ExternalAuthTypeEmbeddedAuthServerconstant with value"embeddedAuthServer"EmbeddedAuthServerConfigstruct with configuration for:TokenLifespanConfig,UpstreamProviderConfig,OIDCUpstreamConfig, andOAuth2UpstreamConfigstructsTest Plan
task operator-generate && task operator-manifests)Large PR Justification