Feat: Implement ADR-008 unified authentication (All Phases Complete)#197
Merged
Feat: Implement ADR-008 unified authentication (All Phases Complete)#197
Conversation
- Create shared OAuth core in pkg/oauth/ with types, client, PKCE, parsing - Refactor internal/oauth/ to use shared core (reduce duplication) - Refactor internal/agent/oauth/ to use shared core - Add auth://status MCP resource for explicit auth state communication - Add comprehensive unit tests for shared OAuth package - Update ADR README with new ADR-008
32 tasks
… submit_auth_token, structured 401)
- Fix data race in TokenStore.GetByIssuer by using proper lock ordering - Remove unused tryConnectWithTokenForSSO function (dead code) - Implement extractToolError properly instead of returning nil - Consolidate duplicate auth status types into pkg/auth package - Compile WWW-Authenticate regex once at package level for performance - Update tests to use shared pkg/auth types
8 tasks
…ration in ADR-008 - Add client_test.go with tests for metadata discovery, token exchange, refresh, and URL building - Coverage for pkg/oauth now at 90.5% (was 44.7%) - Tests include caching, singleflight deduplication, and cache expiry - Clarified migration behavior in ADR-008: existing tokens work, SSO requires re-auth once per issuer - Created follow-up issue #198 for Phase 6 cleanup
- Remove outdated references to synthetic tools and pending auth state - Document issuer-based token lookup for SSO - Reference AuthWatcher for continuous SSO monitoring
…ntication - Add explicit documentation about token logging prohibition in submit_token.go - Add SECURITY comments to handleSubmitAuthToken with audit logging - Add WithHTTPTimeout option to pkg/oauth/client.go for configurable timeouts - Implement exponential backoff (1s-5min) for auth status polling failures - Add structured SECURITY_AUDIT logging for: - Token storage/retrieval operations - SSO token forwarding attempts - Token deletion and clearing - Authentication success/failure events - Add security documentation to TokenStore explaining security measures Security improvements: - Token values are NEVER logged (explicitly documented) - Session IDs included in audit logs for traceability - Backoff prevents server overload during connectivity issues - Structured event names enable security monitoring
- Remove AuthWatcher, submit_auth_token, and auth_resource components (incompatible with passive MCP server architecture) - Delete wrapper types from internal packages, use pkg/oauth directly - Consolidate Token, Metadata, AuthChallenge, PKCEChallenge to pkg/oauth - Keep only server-specific types in internal/oauth (TokenKey, OAuthState) - Update ADR-008 to reflect actual implementation scope - All 135 test scenarios pass
This commit simplifies the OAuth code by leveraging the standard library: - Replace custom PKCE generation in pkg/oauth/pkce.go with oauth2.GenerateVerifier() and oauth2.S256ChallengeFromVerifier() - Refactor internal/agent/oauth/client.go to use oauth2.Config for both authorization URL building and token exchange - Use oauth2.S256ChallengeOption() and oauth2.VerifierOption() for PKCE flow - Remove ~40 lines of manual HTTP request handling - Update tests to reflect API changes (GeneratePKCERaw no longer returns error) Benefits: - Reduced code duplication with standard library - Better RFC 7636 compliance via stdlib implementation - Cleaner, more maintainable OAuth client code - Same functionality with less custom code
This implements proactive authentication status notification in tool responses: - Add auth://status MCP resource to aggregator for exposing server auth states - Add auth poller to agent that polls auth status every 30 seconds - Wrap all agent tool responses with auth metadata (_meta and human-readable) - Include SSO hints when multiple servers share the same identity provider The AI now sees which servers need authentication in every tool response, enabling proactive guidance without explicit queries.
- Fix potential panic in WithHTTPTimeout by adding nil check - Replace custom contains helper with strings.Contains in tests - Fix variable shadowing in GenerateState (stateBytes -> b) - Consolidate AuthStatusResponse types in pkg/oauth to avoid duplication between internal/aggregator and internal/agent packages
…(ADR-008) - Remove strings.Contains fallback in formatOAuthAuthenticationError - Use pkgoauth.Is401Error() instead of string matching in server.go - Update tests to verify structured AuthRequiredError detection - Part of ADR-008 cleanup: fragile string-matching replaced with structured errors
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 all phases of ADR-008: Unified Authentication Architecture.
Closes #196
Implementation Status
pkg/oauth/auth://statusMCP resourceRecent: Final Cleanup (Phase 6)
Removed the last remaining fragile string-matching patterns:
internal/orchestrator/api_adapter.go- Removedstrings.Contains(err.Error(), "authentication required")fallback. Now uses only structuredAuthRequiredErrorviaerrors.As().internal/aggregator/server.go- Replacedstrings.Contains(connectErr.Error(), "401") || strings.Contains(connectErr.Error(), "Unauthorized")withpkgoauth.Is401Error(connectErr).Updated tests - Tests now verify structured error detection instead of string matching.
This completes the ADR-008 goal of eliminating fragile inference-based authentication detection.
Recent: Code Review Fixes
Applied the following code review recommendations:
WithHTTPTimeout- Added nil check for defensive codingcontainshelper withstrings.Contains- Uses stdlib instead of reimplementingGenerateState- RenamedstateBytesvariable tobto avoid shadowing the constantAuthStatusResponsetypes - MovedAuthStatusResponse,ServerAuthStatus, andAuthRequiredInfotopkg/oauth/types.goto avoid duplication betweeninternal/aggregatorandinternal/agentpackagesRecent: OAuth Stdlib Refactoring
Analyzed the OAuth code and identified opportunities to leverage the standard
golang.org/x/oauth2library:Changes Made
PKCE Generation (
pkg/oauth/pkce.go)oauth2.GenerateVerifier()andoauth2.S256ChallengeFromVerifier()GeneratePKCERaw()signature (no longer returns error)Authorization URL Building (
internal/agent/oauth/client.go)oauth2.Config.AuthCodeURL()oauth2.S256ChallengeOption()for PKCE parametersToken Exchange (
internal/agent/oauth/client.go)oauth2.Config.Exchange()oauth2.VerifierOption()for PKCE verificationBenefits
What Remains Custom (Justified)
Security Review Implementation
A comprehensive security review was conducted and the following recommendations were implemented:
1. Token Logging Prohibition
2. Configurable HTTP Timeout
WithHTTPTimeout(timeout time.Duration)option topkg/oauth/Client3. Exponential Backoff for Polling Failures
4. Structured Audit Logging
Added
SECURITY_AUDITprefixed log entries for:Log events include structured fields:
event: Standardized event name (e.g.,token_stored,sso_auth_success)server_url,issuer_url: Context for the operationsession: Session ID for traceabilityerror: Error details on failureTest Coverage
pkg/oauthinternal/agent/auth_watcherinternal/agent/oauth/token_storeAll 135 BDD scenarios pass.
Changes
Phase 1: Shared OAuth Core (
pkg/oauth/)Created a shared OAuth package that both agent and server can import:
types.go: Token, Metadata, AuthChallenge, PKCEChallenge, ClientMetadata, AuthStatusResponse, ServerAuthStatus, AuthRequiredInfoclient.go: OAuth client with metadata discovery, token exchange, token refreshpkce.go: PKCE generation (now delegates to golang.org/x/oauth2)www_authenticate.go: WWW-Authenticate header parsingBenefits:
Phase 1: Refactor Existing OAuth Packages
Updated
internal/oauth/andinternal/agent/oauth/to use the shared core:pkg/oauthfor parsing, PKCE, metadata discoveryPhase 2: Auth Status Resource
Added
auth://statusMCP resource that provides structured auth state:{ "muster_auth": { "authenticated": true }, "server_auths": [ { "server_name": "mcp-kubernetes", "status": "auth_required", "auth_challenge": { "issuer": "https://dex.example.com", "scope": "openid profile", "auth_tool_name": "x_mcp-kubernetes_authenticate" } } ] }Benefits:
Phase 3: Issuer-Keyed Agent Token Store
Enhanced the agent's token store with issuer-based lookup:
GetByIssuer(issuerURL string)method for SSO token lookupHasValidTokenForIssuer(issuerURL string)methodfindTokenByIssuerFromFilesLocked()for persistent storage lookupBenefits:
Phase 4a: AuthWatcher
Created continuous auth state watcher (
internal/agent/auth_watcher.go):auth://statusresource at configurable intervals (10s default)Benefits:
Phase 4b: Submit Auth Token Tool
Added
submit_auth_tokentool to aggregator:Benefits:
Phase 5: Structured 401 Detection
Improved 401 error handling in
internal/mcpserver/types.go:AuthRequiredErrorto includeAuthChallengefrompkg/oauthHasValidChallenge(),GetIssuer(),GetScope(),GetResourceMetadataURL()pkg/oauth.ParseWWWAuthenticateFromError()for consistent parsingParseAuthInfoFromError()in favor of shared utilitiesBenefits:
Phase 6: Fragile Pattern Cleanup
Removed remaining fragile patterns identified in code audit:
api_adapter.go: Removedstrings.Containsfallback for auth detectionserver.go: Usepkgoauth.Is401Error()instead of string matchingBenefits:
Code Review Improvements
Applied recommendations from code review:
TokenStore.GetByIssuer()by using proper lock ordering (read lock for cache check, write lock for file scanning with cache population)tryConnectWithTokenForSSO()functionextractToolError(): Implemented proper error extraction instead of returning nilAuthStatusResponse,ServerAuthStatus, andAuthRequiredInfotopkg/oauth/package used by both agent and aggregatorWithHTTPTimeoutGenerateStateto avoid shadowing constantcontainshelper withstrings.ContainsSecurity Considerations
The implementation follows OAuth 2.1 security best practices:
golang.org/x/oauth2stdlib with S256 challenge onlyMigration Notes
Testing
pkg/oauthcoverage: 89.3%ADR
See ADR-008: Unified Authentication Architecture for the full design.