Skip to content

Conversation

domdomegg
Copy link
Member

@domdomegg domdomegg commented Aug 19, 2025

Add HTTP-based authentication to prove domain ownership, similar to #238 DNS authentication but using HTTPS well-known URIs instead of DNS TXT records.

Fixes #280

Motivation and Context

Some organizations don't have easy access to DNS records, but can easily deploy web assets. They would therefore prefer to be able to complete a HTTP challenge rather than a DNS one to authenticate to the registry.

This would in some way mirror ACME challenges: where you can do a DNS-01 or a HTTP-01 challenge.

How Has This Been Tested?

  • Tested locally by stubbing out the HTTP getter
  • Added unit tests

Breaking Changes

None

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

I concern I had when building this that it might expose us to SSRF attacks.

I think on reflection this is not a significant problem:

  • We only GET a fixed file .well-known/mcp-registry-auth (redirects disallowed) after sanitizing the domain, and then also don't expose the response to the user. This means the user shouldn't be able to hit anything very interesting.
  • The information they might get is what domains/servers exist in the network. Our infrastructure is already all open-source, so attackers aren't getting any additional real info by prodding at the environment this way.

@domdomegg
Copy link
Member Author

@claude can you review this PR?

Copy link
Contributor

claude bot commented Aug 19, 2025

Claude finished @domdomegg's task —— View job


Code Review Complete ✅

This PR adds HTTP-based authentication to validate domain ownership as an alternative to DNS authentication. Overall, this is a well-implemented feature with good security considerations and comprehensive testing.

Detailed Code Review

Strengths

Security Considerations:

  • SSRF Protection: Good security measures against SSRF attacks:
    • Redirects are disabled (CheckRedirect: func() error { return http.ErrUseLastResponse }) in http.go:48-50
    • Fixed well-known URI path (.well-known/mcp-registry-auth) prevents path traversal
    • Domain validation using proper regex pattern (reused from DNS auth)
    • Response content is not exposed to users
  • Crypto Implementation: Proper Ed25519 signature verification with appropriate key length validation
  • Timestamp Validation: 15-second window prevents replay attacks (http.go:143-147)

Code Quality:

  • Consistent Architecture: Mirrors DNS auth structure well, maintaining code consistency
  • Proper Error Handling: Comprehensive error handling throughout the flow
  • Good Abstraction: Clean separation with HTTPKeyFetcher interface for testability
  • Publisher Integration: Clean integration via shared CryptoProvider struct

Testing:

  • Comprehensive Test Coverage: 11 test cases covering all major error conditions
  • Mock Implementation: Proper mocking with MockHTTPKeyFetcher for isolated testing
  • Edge Cases: Tests invalid domains, timestamps, signatures, key formats, and network failures

🔍 Areas for Improvement

Security & Validation:

  1. Response Size Limiting: http.go:77 uses io.ReadAll() without size limits - could be DoS vector

    // Consider adding: resp.Body = http.MaxBytesReader(nil, resp.Body, 4096)
  2. Content-Type Validation: No validation that response is actually text/plain as requested in http.go:64

  3. Domain Sanitization: While domain validation exists, consider additional sanitization for edge cases

Code Issues:
4. Missing Function: http.go:133 calls isValidDomain() but it's not defined in this file - relies on DNS auth file

  • Solution: Move isValidDomain() to a shared location or duplicate it
  1. Missing Function: http.go:221 calls reverseString() but it's not defined in this file
    • Solution: Move reverseString() to a shared location or duplicate it

Documentation:
6. Key Format Documentation: The key format v=MCPv1; k=ed25519; p=<base64-key> should be documented in API docs
7. Well-known URI Spec: Consider documenting the .well-known/mcp-registry-auth specification

🚨 Critical Issues

Missing Dependencies: The HTTP auth handler references functions from the DNS auth file:

  • isValidDomain() at http.go:133
  • reverseString() at http.go:221

These functions need to be:

  • Moved to a shared utility file, OR
  • Duplicated in the HTTP auth file

📋 Minor Observations

Behavioral Differences from DNS:

  • Subdomain Permissions: HTTP auth correctly excludes subdomain permissions (unlike DNS), following ACME HTTP-01 precedent
  • Permission Scope: Only grants exact domain permissions (com.example/*), not subdomains

Implementation Details:

  • Timeout: 10-second HTTP timeout is reasonable
  • Headers: Proper User-Agent and Accept headers set
  • Key Parsing: Robust regex-based key parsing with proper validation

🔧 Required Fixes

Before merging, please address:

  1. Fix missing function dependencies - isValidDomain() and reverseString() functions are called but not defined in http.go
  2. Add response size limiting to prevent potential DoS via large responses

📊 Summary

This is a solid implementation that properly addresses the SSRF concerns mentioned in the PR description. The security model is sound, the code follows existing patterns well, and the test coverage is comprehensive. The publisher tool integration is clean and maintains consistency with the DNS auth approach.

The main issues are easily fixable function dependency problems rather than architectural concerns.


@domdomegg
Copy link
Member Author

Response to Claude review:

  1. I think it's fine to reuse these functions as they're in the same package - Go seems to complain if I duplicate them as requested. Am not a Go expert so might be doing something dumb here, human pushback welcome!
  2. Good idea, will implement

@domdomegg domdomegg requested review from toby and tadasant August 19, 2025 10:55
@domdomegg domdomegg linked an issue Aug 19, 2025 that may be closed by this pull request
Base automatically changed from adamj/dns-auth to main August 21, 2025 21:43
@domdomegg domdomegg dismissed jerome3o-anthropic’s stale review August 21, 2025 21:43

The base branch was changed.

@domdomegg
Copy link
Member Author

// rebased on main, after squash merging #85

@domdomegg domdomegg merged commit ee910ca into main Aug 21, 2025
5 checks passed
@domdomegg domdomegg deleted the adamj/http-auth branch August 21, 2025 22:13
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.

auth: HTTP priv/pub key auth
2 participants