-
Notifications
You must be signed in to change notification settings - Fork 156
Handle OIDC issuer mismatch for remote MCP servers #1959
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1959 +/- ##
==========================================
+ Coverage 47.00% 47.07% +0.07%
==========================================
Files 223 223
Lines 27679 27704 +25
==========================================
+ Hits 13011 13043 +32
+ Misses 13670 13665 -5
+ Partials 998 996 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c7edb49 to
f9d3710
Compare
| flowConfig.RegistrationEndpoint = authServerInfo.RegistrationEndpoint | ||
| // Mark that we're in resource metadata discovery context | ||
| // This allows issuer mismatch which is legitimate for resource metadata | ||
| flowConfig.IsResourceMetadataDiscovery = true |
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.
My only concern is when we derive the issuer from Remote URL, because Realm and resource_metadata do not include issuer
…aware validation This change fixes authentication failures with remote MCP servers where the OAuth/OIDC issuer identifier differs from the metadata discovery URL, while maintaining strict security validation. Problem: - Remote MCP servers like Atlassian return different issuers than their metadata URLs - Metadata URL: https://mcp.atlassian.com - Actual issuer: https://atlassian-remote-mcp-production.workers.dev - This pattern is allowed by RFC 8414 but was causing authentication failures Solution: - Implemented context-aware issuer validation using IsResourceMetadataDiscovery flag - Resource metadata discovery context: allows issuer mismatch (RFC 8414 compliant) - Direct issuer specification: enforces strict issuer match (security critical) - Modified getDiscoveryDocument() to use appropriate validation based on context Security: - HTTPS enforcement for all non-localhost connections - Endpoint URL validation blocks malicious URIs (data:/file:/javascript:) - Response size limiting and content-type validation - Issuer validation deferred to token validation phase for resource metadata - Strict issuer validation maintained for direct issuer specification Testing: - Added comprehensive test coverage for issuer mismatch scenarios - Added security validation tests for various attack vectors - Tests confirm protection against SSRF, XSS, and redirect attacks - All existing tests continue to pass This approach balances the need to support legitimate issuer mismatches (as per RFC 8414) while maintaining security against potential attacks. Fixes #1957
f9d3710 to
2987ed5
Compare
|
The implementation handles the issuer mismatch scenario where OAuth/OIDC providers return a different issuer in their metadata than the URL used to fetch that metadata. This is explicitly allowed by RFC 8414 and used by providers like Atlassian. Key changes:
Testing: Security: Strict validation remains for explicitly configured issuers. Only resource metadata discovery contexts allow issuer mismatch per RFC 8414. |
Summary
This PR fixes authentication failures with remote MCP servers where the OAuth/OIDC issuer identifier differs from the metadata discovery URL, while maintaining strict security validation.
Problem
Remote MCP servers like Atlassian return different issuers than their metadata URLs:
https://mcp.atlassian.comhttps://atlassian-remote-mcp-production.workers.devThis pattern is explicitly allowed by RFC 8414 Section 3 but was causing authentication failures in ToolHive.
Solution
Context-Aware Issuer Validation
Implemented a context-aware approach using the
IsResourceMetadataDiscoveryflag to distinguish between:Resource Metadata Discovery Context (
IsResourceMetadataDiscovery = true)Direct Issuer Specification (
IsResourceMetadataDiscovery = false)Key Changes
pkg/auth/discovery/discovery.goIsResourceMetadataDiscoveryfield toOAuthFlowConfiggetDiscoveryDocument()to conditionally useDiscoverActualIssuer()based on contextpkg/runner/remote_auth.goIsResourceMetadataDiscovery = truewhen using discovered endpointspkg/auth/oauth/oidc.goDiscoverActualIssuer()function skips issuer validation when appropriatediscoverOIDCEndpointsWithClientAndValidation()acceptsvalidateIssuerparameterSecurity Considerations
Maintained Security Protections
data:,file:,javascript:)Attack Vector Protection
Added comprehensive tests confirming protection against:
Testing
New Test Coverage
Issuer Mismatch Tests (
pkg/auth/discovery/discovery_issuer_mismatch_test.go)Security Validation Tests (
pkg/auth/oauth/oidc_malicious_scenarios_test.go)Context-Aware Validation Tests (
pkg/auth/oauth/oidc_issuer_validation_test.go)IsResourceMetadataDiscovery = trueIsResourceMetadataDiscovery = falseTest Results
RFC Compliance
This implementation aligns with:
Fixes
Fixes #1957
Review Notes
The solution balances the need to support legitimate issuer mismatches (as per RFC 8414) while maintaining security against potential attacks. The context-aware approach ensures we only relax validation where it's explicitly expected and safe to do so.