Skip to content

Conversation

JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Sep 19, 2025

Summary

This PR fixes issue #1957 where authentication with remote MCP servers was failing when the server returns a different issuer URL than its public-facing URL in the OIDC discovery response.

Problem

Some OAuth providers (like those using Cloudflare Workers) return an issuer URL in their .well-known/openid-configuration that differs from their public-facing server URL. ToolHive was previously deriving the issuer from the server URL and strictly validating against it, causing authentication failures.

Solution

The fix adds a new priority level to the issuer discovery algorithm, placing well-known endpoint discovery before URL derivation. This allows accepting the authoritative issuer from the .well-known/openid-configuration endpoint even when it differs from the server URL, complying with RFC 8414 which states that the issuer in the metadata is authoritative.

New Discovery Priority Chain

  1. Configured issuer (if provided)
  2. Realm-derived issuer (RFC 8414)
  3. Resource metadata discovery (RFC 9728)
  4. Well-known endpoint discovery (NEW - accepts actual issuer)
  5. URL-derived issuer (fallback)

Changes

  • Added tryDiscoverFromWellKnown function to attempt discovery before URL derivation
  • Modified to accept the actual issuer from ValidateAndDiscoverAuthServer
  • Updated DeriveIssuerFromURL to preserve HTTP scheme for localhost URLs (supports testing)
  • Added path normalization to DeriveIssuerFromRealm for security
  • Added comprehensive test coverage for the discovery priority chain including security edge cases
  • Fixed linting issues with constants and parallel test execution

Testing

  • ✅ All existing tests pass
  • ✅ Added new tests for issuer mismatch scenarios
  • ✅ Added security tests for path traversal and HTTPS validation
  • ✅ Linting passes

Compatibility

The fix maintains backward compatibility while supporting real-world OAuth deployments where the issuer differs from the server endpoint.

Fixes #1957

JAORMX added a commit that referenced this pull request Sep 19, 2025
This adds detailed documentation about how ToolHive handles remote MCP
server authentication, including compliance with various RFCs and the
MCP authorization specification.

The documentation covers:
- Specification compliance (RFC 9728, RFC 8414, RFC 7591)
- Authentication flow and discovery priority chain
- Well-known endpoint discovery for issuer mismatch handling
- Dynamic client registration
- Security features and configuration options
- Implementation details and key components

This documentation helps developers understand the authentication
architecture and serves as a reference for the implementation that
was fixed in PR #1980.
Copy link

codecov bot commented Sep 19, 2025

Codecov Report

❌ Patch coverage is 86.95652% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.18%. Comparing base (06e638c) to head (80fb02c).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
pkg/auth/discovery/discovery.go 57.14% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1980      +/-   ##
==========================================
+ Coverage   47.91%   48.18%   +0.26%     
==========================================
  Files         233      233              
  Lines       29172    29213      +41     
==========================================
+ Hits        13978    14075      +97     
+ Misses      14166    14102      -64     
- Partials     1028     1036       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX JAORMX requested a review from Copilot September 19, 2025 19:23
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes OAuth authentication failures with remote MCP servers where the server's OIDC discovery response returns a different issuer URL than the public-facing server URL. The solution adds well-known endpoint discovery as a priority step before URL derivation, allowing acceptance of the authoritative issuer from the .well-known/openid-configuration endpoint.

Key changes:

  • Added tryDiscoverFromWellKnown function for OAuth issuer discovery without strict validation
  • Modified discovery priority chain to check well-known endpoints before URL derivation fallback
  • Enhanced localhost HTTP scheme preservation for testing scenarios

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
pkg/runner/remote_auth.go Implements new well-known discovery step in OAuth issuer discovery chain
pkg/runner/remote_auth_test.go Comprehensive test suite for OAuth discovery priority chain and security scenarios
pkg/runner/remote_auth_test_helpers_test.go Test helper functions for mock server setup and test case processing
pkg/runner/config_test.go Replaces string literals with localhostStr constant for consistency
pkg/auth/discovery/discovery.go Adds localhost HTTP scheme preservation and path normalization security improvements

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

This fixes authentication failures when remote MCP servers return a
different issuer URL than their public-facing URL in the OIDC discovery
response.

The fix adds a new priority level to the issuer discovery algorithm,
placing well-known endpoint discovery before URL derivation. This allows
accepting the authoritative issuer from the .well-known/openid-configuration
endpoint even when it differs from the server URL, complying with RFC 8414.

New discovery priority chain:
1. Configured issuer (if provided)
2. Realm-derived issuer (RFC 8414)
3. Resource metadata discovery (RFC 9728)
4. Well-known endpoint discovery (NEW - accepts actual issuer)
5. URL-derived issuer (fallback)

Changes:
- Add tryDiscoverFromWellKnown to attempt discovery before URL derivation
- Accept the actual issuer from ValidateAndDiscoverAuthServer
- Preserve HTTP scheme for localhost URLs in DeriveIssuerFromURL
- Add path normalization to DeriveIssuerFromRealm for security
- Add comprehensive test coverage for discovery priority chain
- Fix linting issues with constants and parallel test execution

The fix maintains backward compatibility while supporting real-world
OAuth deployments where the issuer differs from the server endpoint.

Fixes #1957

Signed-off-by: Juan Antonio Osorio <[email protected]>
@JAORMX JAORMX force-pushed the fix-atlassian-issuer-mismatch branch from 0532851 to 80fb02c Compare September 29, 2025 07:20
Copy link
Contributor

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

So if we are having the issuer mismatches then let's address them like this, the only thing I wonder is if, based on the patterns we are seeing, we could restrict the discovered issues to be subdomains of the same top-level domain? In the code, we'd read the top-level domain of both derivedURL and authServerInfo.Issuer and make sure if they are the same? I don't know what the real world mismatches are so I don't know if this is realistic.

JAORMX added a commit that referenced this pull request Sep 30, 2025
Add comprehensive remote MCP authentication documentation

This adds detailed documentation about how ToolHive handles remote MCP
server authentication, including compliance with various RFCs and the
MCP authorization specification.

The documentation covers:
- Specification compliance (RFC 9728, RFC 8414, RFC 7591)
- Authentication flow and discovery priority chain
- Well-known endpoint discovery for issuer mismatch handling
- Dynamic client registration
- Security features and configuration options
- Implementation details and key components

This documentation helps developers understand the authentication
architecture and serves as a reference for the implementation that
was fixed in PR #1980.
@JAORMX JAORMX merged commit f89b103 into main Sep 30, 2025
26 checks passed
@JAORMX JAORMX deleted the fix-atlassian-issuer-mismatch branch September 30, 2025 01:36
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.

OIDC issuer mismatch prevents authentication with Atlassian remote MCP server
2 participants