Skip to content

Conversation

@saucow
Copy link
Collaborator

@saucow saucow commented Oct 9, 2025

What I Did

This PR enhances the OAuth discovery implementation to handle partially MCP-compliant servers while adding comprehensive logging, test coverage, and CI infrastructure.

Core Changes

  1. OAuth Discovery Fallback Logic - Enable discovery for servers without WWW-Authenticate headers
  2. Structured Logging - Context-based logger integration with detailed discovery flow logging
  3. Comprehensive Tests - 11 test cases covering discovery, parsing, and DCR flows
  4. CI/CD Infrastructure - GitHub Actions workflow for automated testing and linting
  5. Code Cleanup - Removed unused functions, fixed documentation

Background:

What the MCP Spec Requires (Lines 87-90)

From the MCP Authorization Specification:

Line 87-88: MCP servers MUST use the HTTP header WWW-Authenticate when returning a 401 Unauthorized to indicate the location of the resource server metadata URL as described in RFC9728 Section 5.1.

Line 90: MCP clients MUST be able to parse WWW-Authenticate headers and respond appropriately to HTTP 401 Unauthorized responses from the MCP server.

What We Were Doing (Before This PR)

Strict spec enforcement - Failed immediately if WWW-Authenticate was missing or unparseable:

// Old code - Hard failure
wwwAuth := resp.Header.Get("WWW-Authenticate")
if wwwAuth == "" {
    return nil, fmt.Errorf("server returned 401 but no WWW-Authenticate header")
}

challenges, err := ParseWWWAuthenticate(wwwAuth)
if err != nil {
    return nil, fmt.Errorf("parsing WWW-Authenticate header: %w", err)
}

Result: Discovery failed for servers like Neon that don't provide WWW-Authenticate headers.

The Foundation: RFC 9728

Note: The MCP spec requires servers to implement RFC 9728 (OAuth 2.0 Protected Resource Metadata):

Line 76: MCP servers MUST implement the OAuth 2.0 Protected Resource Metadata (RFC9728) specification to indicate the locations of authorization servers.

RFC 9728 requires:

  • Section 3: Protected resources MUST provide /.well-known/oauth-protected-resource endpoint
  • Section 5.1: WWW-Authenticate resource_metadata parameter is OPTIONAL (MAY)

The MCP spec adds a stricter requirement by upgrading WWW-Authenticate from MAY (optional in RFC 9728) to MUST (required in MCP).

What We Do Now (After This PR)

Fallback - Try WWW-Authenticate first, fall back to RFC 9728-required well-known endpoint:

// New code - Graceful fallback
wwwAuth := resp.Header.Get("WWW-Authenticate")

var challenges []WWWAuthenticateChallenge
if wwwAuth != "" {
    challenges, err = ParseWWWAuthenticate(wwwAuth)
    if err != nil {
        logger.Warnf("could not parse WWW-Authenticate header: %v", err)
        challenges = nil
    }
}

// Fallback to RFC 9728-required well-known endpoint
if resourceMetadataURL == "" {
    wellKnownURL := fmt.Sprintf("%s/.well-known/oauth-protected-resource", defaultAuthServerURL)
    logger.Infof("fallback: trying well-known resource metadata endpoint: %s", wellKnownURL)
    resourceMetadata, _ = fetchOAuthProtectedResourceMetadata(ctx, client, wellKnownURL)
}

Result: Discovery succeeds for servers that are RFC 9728-compliant but not fully MCP-compliant.


Detailed Changes

1. Discovery Robustness (discovery.go)

Changes:

  • Remove early exit when WWW-Authenticate missing (lines 69-92)
  • Add nil check before accessing challenges (line 107-109)
  • Enable fallback to /.well-known/oauth-protected-resource (lines 123-131)
  • Continue discovery even if initial response isn't 401 (lines 70-72)

Spec compliance:

  • ✅ We still parse WWW-Authenticate when present (MCP line 90 requirement)
  • ✅ We still use RFC 9728 for discovery (MCP line 62 requirement)
  • ✅ We follow RFC 9728 correctly (use required well-known endpoint)
  • ⚠️ We don't enforce servers to provide WWW-Authenticate (fallback)

Impact: Servers like Neon (https://mcp.neon.tech/mcp) that lack WWW-Authenticate headers now work.

2. Structured Logging (log.go)

New file: Minimal Logger interface for library logging

type Logger interface {
    Infof(format string, args ...any)
    Warnf(format string, args ...any)
    Debugf(format string, args ...any)
}

func WithLogger(ctx context.Context, logger Logger) context.Context
func loggerFromContext(ctx context.Context) Logger

Integration: Callers (Pinata, MCP Gateway) inject their logger via context:

ctx = oauth.WithLogger(ctx, log)  // log is Pinata's component logger
discovery, err := oauth.DiscoverOAuthRequirements(ctx, serverURL)

Logging added at each decision point:

  • HTTP response status
  • WWW-Authenticate header presence/parsing
  • Fallback triggers
  • Metadata fetching success/failure
  • Discovery completion

3. Test Coverage (*_test.go, testutil.go)

New files:

  • discovery_test.go - 3 tests for discovery flow
  • www_authenticate_test.go - 4 tests for parsing
  • dcr_test.go - 2 tests for DCR
  • testutil.go - Test logger utility

(not mandatory) A picture of a cute animal, if possible in relation to what you did

🦦 Otter navigating around obstacles - Just like our fallback logic navigates around missing WWW-Authenticate headers to find the well-known endpoint!

saucow and others added 4 commits October 8, 2025 14:05
Make WWW-Authenticate header optional with fallback to well-known endpoints.
This enables discovery to work with partially-compliant MCP servers that:
- Don't provide WWW-Authenticate headers
- Provide unparseable WWW-Authenticate headers
- Return non-401 responses that still require OAuth

Changes:
- Make WWW-Authenticate optional: log warning if missing/unparseable instead of failing
- Add nil check before calling FindResourceMetadataURL(challenges)
- Continue discovery even if initial response isn't 401 (log warning)
- Fallback to /.well-known/oauth-protected-resource when resource_metadata URL not available

This aligns with RFC 9728 requirement that servers MUST provide the well-known
endpoint, making it a valid fallback when WWW-Authenticate is unavailable.

Fixes issue with Neon server and other servers that don't fully implement
MCP Authorization Specification Section 4.1.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
Replace fmt.Printf with proper structured logging that integrates
with caller's logging infrastructure.

Changes:
- Add Logger interface with Infof/Warnf/Errorf methods
- Support context-based logger injection via WithLogger/LoggerFromContext
- Provide WrapLogger helper to adapt any compatible logger
- Update DiscoverOAuthRequirements to use logger from context
- Fallback to default stderr logger if no logger provided

Benefits:
- Logs from library now appear with proper component tags ([com.docker.backend.dcr])
- No messy grep patterns needed - logs integrate naturally
- Pluggable logging - works with any logger (logrus, zap, slog, etc.)
- Backward compatible - uses default logger if none provided

This is the Go-idiomatic approach for library logging, following the
context pattern used throughout the Go ecosystem.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
@saucow saucow changed the title Bugfix oauth discovery bugfix: OAuth Discovery Robustness, Logging, and Test Coverage Oct 10, 2025
@saucow saucow marked this pull request as ready for review October 10, 2025 00:06
@saucow saucow requested a review from a team as a code owner October 10, 2025 00:06
Copy link
Collaborator Author

@saucow saucow Oct 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note moved to .github/ to match mcp-gateway location: https://github.com/docker/mcp-gateway/blob/main/.github/SECURITY.md

@saucow saucow merged commit c15705d into docker:main Oct 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants