Feat: Implement OAuth Proxy for Remote MCP Servers#149
Merged
Conversation
14 tasks
…ents - Remove duplicate AuthChallenge type from agent, use api.AuthChallenge instead - Add mutex protection to metadataCache in oauth/client.go for thread safety - Remove redundant pendingFlows from Handler and pendingVerifiers from Manager - Store issuer and code verifier with OAuth state to fix empty issuer bug - Simplify callback handling by consolidating state management
This commit adds unit tests for the internal/oauth package, covering: - token_store_test.go: Tests for token storage, retrieval, expiration, SSO-based lookup by issuer, deletion, and cleanup - state_store_test.go: Tests for OAuth state generation, validation, CSRF protection, code verifier security, and state expiration - www_authenticate_test.go: Tests for parsing WWW-Authenticate headers, identifying OAuth challenges, and extracting issuer information - client_test.go: Tests for redirect URI generation, PKCE generation, token storage/retrieval, and OAuth metadata fetching with caching - handler_test.go: Tests for OAuth callback handling, error cases, success/error page rendering, and missing parameter validation - manager_test.go: Tests for manager lifecycle, server registration, nil-safety, and configuration handling All tests pass with race detection enabled.
Security improvements to the OAuth proxy implementation: 1. XSS Prevention: Added html.EscapeString() to sanitize serverName and message parameters before embedding them in HTML pages (success/error). 2. Information Disclosure: Sanitized error messages from token exchange and refresh operations to avoid exposing sensitive response bodies. Full errors are logged at debug level for troubleshooting. 3. Metadata Cache TTL: Added 1-hour TTL to OAuth metadata cache to ensure stale metadata is periodically refreshed from issuers.
…shake auth This implements the Synthetic Tool Placeholder pattern to solve the chicken-and-egg problem where OAuth authentication is required before the MCP protocol handshake can complete. Changes: - Add AuthRequiredError type in aggregator/types.go and mcpserver/types.go - Add ServerStatus enum with StatusAuthRequired for tracking auth state - Detect 401 errors in StreamableHTTPClient and SSEClient Initialize() - Parse WWW-Authenticate headers to extract OAuth information - Add RegisterPendingAuth to registry for servers requiring auth - Create synthetic authenticate_<server> tools for auth-required servers - Handle synthetic auth tool calls by creating OAuth challenges - Add UpgradeToConnected to registry for post-auth upgrade - Update addNewItems and collectItemsFromServers to include auth tools - Add MCPServerAuthRequired event reason and template The flow: 1. Server returns 401 during Initialize() -> AuthRequiredError 2. Server registered in StatusAuthRequired with synthetic auth tool 3. User calls authenticate_<server> tool 4. Tool creates OAuth challenge with sign-in link 5. User authenticates in browser -> token stored 6. User retries -> server upgraded to connected status Addresses issue #144 comment about OAuth before MCP handshake
- Extracted checkForAuthRequiredError and parseAuthInfoFromError from SSEClient and StreamableHTTPClient to shared functions in mcpserver/types.go - Replaced duplicate AuthInfo/AuthRequiredError types in aggregator/types.go with type alias to mcpserver.AuthInfo - Reduces code duplication by ~90 lines while maintaining functionality This follows DRY/KISS principles by centralizing the auth error detection logic that was previously duplicated in both MCP client implementations.
- Extract session ID from MCP ClientSession context (fixes shared session issue) - Add security headers to OAuth HTML responses (X-Frame-Options, CSP, etc.) - Reduce metadata cache TTL from 1h to 30min for faster key rotation updates - Add comprehensive tests for security headers
…tion - Add comprehensive TestTokenStore_SessionIsolation test to verify users cannot access each other's tokens - Improve getSessionIDFromContext documentation with security implications - Add debug logging when falling back to default session (stdio mode only) - The mcp-go library provides unique UUID session IDs for SSE and Streamable HTTP connections
- Add muster.oauth section to values.yaml with enabled, publicUrl, clientId, callbackPath options - Update values.schema.json with OAuth schema validation - Update deployment.yaml to pass OAuth flags when enabled - Update README.md with OAuth configuration documentation
- Add RegisterServerPendingAuth to AggregatorHandler interface - Orchestrator now catches AuthRequiredError and registers servers in pending auth state - Event handler skips deregistration for servers in auth_required state - Fix nil pointer in registry Deregister when client is nil (auth_required servers) - Add isServerAuthRequired callback to event handler - Update api_adapter to implement RegisterServerPendingAuth
- Add discoverAuthorizationServer() to fetch issuer from /.well-known/oauth-protected-resource - Fix URL display bug in agent logger when text contains % (URL-encoded chars) - Print strings literally when no format args are provided
- Extract tokenToAPIToken helper to remove duplicate Token -> OAuthToken conversion - Define tokenExpiryMargin constant to eliminate magic numbers - Use embed package for HTML templates (success.html, error.html) for better maintainability - Use singleflight.Group to prevent concurrent metadata fetches for same issuer - Sanitize OAuth error messages to prevent leaking sensitive information - Simplify StateStore.GenerateState to return only encodedState (nonce is embedded) - Update tests for new API signatures and security improvements
- Document CIMD redirect URI limitation in Helm chart README - Add security notes to values.yaml about TLS requirements - Add warning log for default session (stdio) token storage - Add TLS assumption comment to metadata cache - Enhance doc.go with comprehensive security documentation - Token storage (in-memory only, no persistence) - Session isolation details - TLS requirements for production
Muster can now serve its own Client ID Metadata Document (CIMD) at /.well-known/oauth-client.json, eliminating the need for external static file hosting. Changes: - Add CIMD serving endpoint in OAuth handler - Auto-derive clientId from publicUrl when not explicitly set - Mount CIMD endpoint in aggregator HTTP mux when self-hosting - Add OAuthConfig methods: GetEffectiveClientID, ShouldServeCIMD, GetCIMDPath, GetRedirectURI - Update helm values with new oauth.cimdPath option and docs - Add comprehensive tests for new functionality Users can now simply set oauth.publicUrl and muster will auto-generate and serve the CIMD with correct redirect_uris for their deployment.
- Increase test coverage for OAuth package from 53% to 82%+ - Add tests for api_adapter, manager, client, stores - Fix DRY violation: use strings.TrimSuffix consistently in config/types.go - Extract HTTP timeout to named constant (httpClientTimeout) - Extract software version to constant for CIMD handler - Fix unused variable assignment in server.go GetToolsWithStatus - Remove sleep in OnToolsUpdated, use goroutine scheduling instead - Document goroutine lifecycle requirements in TokenStore/StateStore Addresses code review recommendations.
- Add session ID truncation in logs to prevent full session IDs from appearing in debug logs (first 8 chars + ...) - Upgrade token refresh logging to INFO level for operations monitoring with duration metrics for performance tracking - Add comprehensive TLS/HTTPS requirements documentation in doc.go - Add rate limiting recommendations for OAuth callback endpoint in doc.go and Helm values/README - Document logging security practices (token truncation, no access tokens logged) - Add self-hosted CIMD documentation in Helm README
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.
Summary
This PR implements the OAuth Proxy feature for remote MCP server authentication as outlined in Epic #144.
Changes
Phase 1: Foundation & Configuration
OAuthConfigtoMusterConfig.Aggregatorwith fields forpublicUrl,clientId,callbackPath, andenabled--oauth,--oauth-public-url, and--oauth-client-idflags tomuster servePhase 2: Server-Side OAuth Logic
New
internal/oauthpackage containing:types.go: OAuth types includingToken,AuthChallenge,OAuthState,OAuthMetadata,ClientMetadata,WWWAuthenticateParamstoken_store.go: Thread-safe in-memory token storage with automatic cleanupstate_store.go: Thread-safe state parameter storage for CSRF protection (stores issuer and code verifier with state)client.go: OAuth 2.1 client with PKCE support, metadata discovery (with thread-safe caching and TTL), token exchange, and refreshwww_authenticate.go: WWW-Authenticate header parser for extracting issuer and scopehandler.go: HTTP handler for OAuth callbacks with success/error pagesmanager.go: Coordinates OAuth flows and integrates with the aggregatorapi_adapter.go: Implementsapi.OAuthHandlerfollowing the service locator patternAPI layer integration: Added
OAuthHandlerinterface ininternal/api/oauth.gowith registration functionsAggregator integration: Updated aggregator to mount OAuth callback handler on the HTTP mux
Phase 3: Agent Integration
auth_requiredresponses and displays formatted messages usingapi.AuthChallengePhase 4: Documentation
docs/oauth-client.jsonfor GitHub Pages hosting (Client ID Metadata Document)Phase 5: Synthetic Tool Placeholder Pattern
Implemented the "Synthetic Tool Placeholder" pattern to solve the chicken-and-egg problem where OAuth authentication is required before the MCP protocol handshake can complete:
StreamableHTTPClientandSSEClientnow detect 401 errors during initialization and returnAuthRequiredErrorwith parsed OAuth information from theWWW-Authenticateheadermcpserver/types.gocontaining URL and OAuth info (issuer, scope, resource metadata URL)aggregator/types.gowithStatusConnected,StatusDisconnected, andStatusAuthRequiredStatusAuthRequiredstate with a syntheticauthenticate_<server>toolUpgradeToConnected()method in registry upgrades a pending auth server to connected status after successful OAuthSynthetic Tool Flow
Initialize()→AuthRequiredErrorStatusAuthRequiredwith syntheticauthenticate_<server>toolauthenticate_<server>Phase 6: Helm Chart Configuration
Added OAuth proxy configuration to the Helm chart:
muster.oauthsection with:enabled: Enable/disable OAuth proxy (default: false)publicUrl: Publicly accessible URL for OAuth callbacksclientId: OAuth client identifier (CIMD URL)callbackPath: OAuth callback endpoint path (default:/oauth/callback)Example Helm values:
Phase 7: Integration Testing & Bug Fixes
Tested with real remote MCP server (mcp-kubernetes on Gazelle cluster):
AuthRequiredErrorand registers servers inauth_requiredstate with the aggregator viaRegisterServerPendingAuthauth_requiredstate (they should remain registered with their synthetic auth tool)RegisterServerPendingAuthtoAggregatorHandlerinterface and implemented inapi_adapter.goisServerAuthRequiredcallback to event handler for checking server statusVerified working flow:
Security Review & Improvements
A comprehensive security review was conducted and the following findings were identified:
Security Strengths (Already Present)
crypto/randverifiershtml.EscapeString()for all user-controlled HTML outputSecurity Review Findings (All Addressed)
Critical: Session Isolation (Multi-User Security)
How Session IDs Work:
The mcp-go library provides unique session IDs for each MCP connection:
uuid.New().String()sessionIdManager.Generate()The session ID is automatically injected into the context by the mcp-go library and retrieved using
server.ClientSessionFromContext(ctx).Token Isolation:
Tokens are stored with a composite key:
(SessionID, Issuer, Scope). This ensures:Stdio Fallback:
For stdio transport (single-user CLI), falls back to "default-session" which is acceptable since stdio is inherently single-user (one process = one user). A warning is now logged when this fallback is used.
Security Test Coverage:
Added
TestTokenStore_SessionIsolationwhich explicitly verifies:GetByIssuer()respects session boundariesSecurity Fixes Applied
server.ClientSessionFromContext(ctx)to extract the mcp-go library's unique session IDX-Content-Type-Options: nosniffX-Frame-Options: DENYContent-Security-Policy: default-src 'none'; style-src 'unsafe-inline'Referrer-Policy: no-referrerCache-Control: no-store, no-cache, must-revalidateCode Quality Improvements (Post-Review)
Applied the following improvements based on Go code review:
AuthChallengetype from agent package - now usesapi.AuthChallengemetadataCachein OAuth client to prevent race conditionspendingFlowsfrom Handler andpendingVerifiersfrom ManagercheckForAuthRequiredErrorandparseAuthInfoFromErrorfromSSEClientandStreamableHTTPClientto shared helper functions inmcpserver/types.goAuthInfoandAuthRequiredErrortypes inaggregator/types.gowith type alias tomcpserver.AuthInfo, reducing code duplication by ~90 linesLatest Code Quality Improvements (Post-Security-Review)
tokenToAPITokenhelper function to remove duplicate Token → OAuthToken conversion inapi_adapter.gotokenExpiryMarginconstant (30s) to eliminate magic numbers in token expiration checksinternal/oauth/templates/using Go'sembedpackage for better maintainability and syntax highlightingsingleflight.GrouptofetchMetadatato prevent concurrent duplicate fetches for the same issuer (fixes TOCTOU race condition)error_descriptionfrom OAuth providers (prevents information disclosure)StateStore.GenerateStateto return onlyencodedState(the nonce is embedded within and callers don't need it separately)Code Review Improvements (Latest Commit)
api_adapter.go(was 0%)strings.TrimSuffixconsistently inconfig/types.go(removed customtrimTrailingSlashfunction)httpClientTimeout(30s) andsoftwareVersion("1.0.0") constants in OAuth clientserver.goGetToolsWithStatustime.SleepfromOnToolsUpdated- goroutine scheduling provides sufficient separationTokenStoreandStateStoregodocs (callers MUST callStop())truncateSessionIDhelper to truncate session IDs to first 8 chars in all log outputUnit Tests
Comprehensive unit tests added for the
internal/oauthpackage (82%+ coverage):token_store_test.go: Token storage, retrieval, expiration, SSO lookup by issuer, deletion, cleanup, session isolation securitystate_store_test.go: State generation, validation, CSRF protection, code verifier security, state expiration, cleanupwww_authenticate_test.go: WWW-Authenticate header parsing, OAuth challenge detection, issuer extractionclient_test.go: Redirect URI generation, PKCE generation, token storage/retrieval, metadata fetching with caching, code exchange, token refreshhandler_test.go: OAuth callback handling, error cases, success/error page rendering, parameter validation, security headers verification, CIMD servingmanager_test.go: Manager lifecycle, server registration, nil-safety, configuration handling, GetToken flowsapi_adapter_test.go: Adapter methods, token conversion, registration delegationArchitecture
The OAuth proxy follows the architecture outlined in
docs/explanation/decisions/004-oauth-proxy.md:Testing
goimportsandgo fmtRelated
Implements #144
Security Review Checklist (All Passed)