Skip to content

feat: continuous verification job #219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 29 commits into
base: remote-verification
Choose a base branch
from

Conversation

aphansal123
Copy link
Collaborator

@aphansal123 aphansal123 commented Aug 4, 2025

Breaking Changes

None - This is a purely additive feature that:

  • Extends existing database interfaces with new methods
  • Adds new verification endpoints without modifying existing ones
  • Maintains backward compatibility with existing server registration flows
  • Uses feature flags and configuration to enable/disable the background job system

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

Key Features Implemented

  1. Background Verification Job System

    • Cron-based scheduling (configurable, default: daily at 2 AM)
    • Configurable concurrent processing (default: 10 parallel verifications)
    • Graceful lifecycle management with proper cleanup
    • Real-time status monitoring and health checks
  2. Dual Verification Methods

    • HTTP-01 Challenge: Verifies via /.well-known/mcp-challenge/{token} endpoint
    • DNS TXT Record: Verifies via _mcp-challenge.{domain} TXT records
    • Fallback logic with intelligent retry patterns
    • Security hardening: TLS 1.2+ enforcement, certificate validation
  3. Advanced Error Handling & Retry Logic

    • Exponential backoff with configurable limits
    • Error classification (retryable vs. permanent failures)
    • Context-aware timeouts with proper cancellation
    • Modern Go patterns using errors.Is() and errors.As()
  4. Comprehensive Database Layer

    • Domain verification lifecycle tracking
    • Consecutive failure counting with threshold-based actions
    • Automated cleanup of old verification records
    • Complete implementations for both MongoDB and in-memory storage
  5. Notification & Alerting System

    • Configurable failure thresholds (default: 3 consecutive failures)
    • Cooldown periods to prevent notification spam (default: 24 hours)
    • Extensible notification interface for different alert channels
    • Structured logging for debugging and monitoring

Configuration Options

type BackgroundJobConfig struct {
    CronSchedule               string        // Default: "0 0 2 * * *" (daily at 2 AM)
    MaxConcurrentVerifications int           // Default: 10
    VerificationTimeout        time.Duration // Default: 30s
    FailureThreshold           int           // Default: 3
    RetryDelay                 time.Duration // Default: 1s
    NotificationCooldown       time.Duration // Default: 24h
    CleanupInterval            time.Duration // Default: 7 days
}

aphansal123 and others added 15 commits July 29, 2025 15:27
…protocol/registry into thomas-sickert/token-storage

# Conflicts:
#	internal/verification/README.md
#	internal/verification/token.go
#	internal/verification/token_test.go
Closes [#22190](github/copilot#22190)

<!-- Provide a brief summary of your changes -->

## Motivation and Context

This PR implements DNS record verification functionality for the MCP
Registry's server name verification system. This feature enables domain
owners to prove ownership of their domains by adding specific TXT
records to their DNS, which is required for publishing MCP servers under
domain-scoped namespaces (e.g., com.example/my-server). This prevents
domain impersonation and ensures only legitimate domain owners can
publish under their domains.

The implementation follows industry best practices used by certificate
authorities and cloud services for domain ownership verification,
providing a secure and reliable method for continuous domain validation.

## How Has This Been Tested?
- Comprehensive unit tests: Added extensive test coverage for all DNS
verification functions including success cases, failure scenarios,
timeout handling, and retry logic
- Mock DNS resolver: Implemented a complete mock DNS resolver system for
reliable testing without external dependencies
- Token generation and validation: Added tests for cryptographically
secure token generation and format validation
Error handling: Tested various error conditions including DNS failures,
timeouts, and invalid inputs
- Integration scenarios: Tested with both mock and real DNS
configurations
- Performance benchmarks: Added benchmark tests for token generation and
validation functions
- Example CLI tool: Created a dns-verify example application for manual
testing and demonstration

## Breaking Changes
<!-- Will users need to update their code or configurations? -->

## Types of changes
<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [X] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Documentation update

## Checklist
<!-- Go over all the following points, and put an `x` in all the boxes
that apply. -->
- [X] I have read the [MCP
Documentation](https://modelcontextprotocol.io)
- [X] My code follows the repository's style guidelines
- [X] New and existing tests pass locally
- [X] I have added appropriate error handling
- [X] I have added or updated documentation as needed

## Additional context
- Cryptographically secure tokens: 128-bit random tokens using
crypto/rand with base64url encoding
- Robust DNS resolution: Configurable DNS resolvers with support for
secure resolvers (8.8.8.8, 1.1.1.1)
- Retry logic: Exponential backoff for transient DNS failures with
configurable timeouts and retry limits
- Comprehensive error handling: Detailed error types with proper error
wrapping and retry logic
- Security considerations: Uses secure DNS resolvers, validates token
formats, and implements proper timeout handling
- Extensive documentation: Added detailed README with usage examples,
security considerations, and integration guides

---------

Co-authored-by: Copilot <[email protected]>
Co-authored-by: Trent Jones <[email protected]>
… domain verification and 3-strike failure policy
@aphansal123 aphansal123 self-assigned this Aug 4, 2025
@aphansal123 aphansal123 changed the title Feature/continuous verification job feat: continuous verification job Aug 4, 2025
@aphansal123 aphansal123 changed the base branch from thomas-sickert/token-storage to main August 4, 2025 19:51
@aphansal123 aphansal123 changed the base branch from main to thomas-sickert/token-storage August 4, 2025 19:52
@aphansal123 aphansal123 requested a review from Copilot August 4, 2025 19:52
Copilot

This comment was marked as outdated.

@aphansal123
Copy link
Collaborator Author

I'll update the well-known namespace format based on our thread earlier - .well-known/mcp/verify

Base automatically changed from thomas-sickert/token-storage to remote-verification August 5, 2025 14:06
@aphansal123 aphansal123 requested a review from Copilot August 5, 2025 16:08
Copilot

This comment was marked as outdated.

@aphansal123 aphansal123 requested a review from Copilot August 5, 2025 17:16
Copilot

This comment was marked as outdated.

@aphansal123 aphansal123 requested review from thomas-sickert and trent-j and removed request for trent-j and thomas-sickert August 5, 2025 21:22
@aphansal123 aphansal123 requested a review from Copilot August 5, 2025 21:52
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 implements a comprehensive continuous verification job system for the MCP Registry to maintain domain ownership validation. The system provides automatic background verification of registered domains using both DNS TXT records and HTTP challenges, with robust error handling, retry logic, and notification systems.

Key changes include:

  • Background verification job with cron-based scheduling and concurrent processing
  • Enhanced database layer supporting domain verification lifecycle tracking
  • Configuration system for background job behavior and thresholds
  • Comprehensive test coverage for all verification components

Reviewed Changes

Copilot reviewed 18 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
internal/verification/background_job.go Core background verification job implementation with cron scheduling
internal/verification/http.go HTTP-01 challenge verification with security hardening
internal/verification/dns.go DNS TXT record verification with context support
internal/verification/validation.go Common input validation logic for both verification methods
internal/verification/wait.go Context-aware waiting utility
internal/model/model.go Enhanced domain verification data models
internal/database/*.go Database interface extensions for verification tracking
internal/config/config.go Configuration options for background job system
cmd/registry/main.go Integration of background job into main application

@@ -22,7 +22,7 @@ func (d *DefaultDNSResolver) LookupTXT(ctx context.Context, name string) ([]stri

// NewDefaultDNSResolver creates a DNS resolver with the given configuration
//
//nolint:ireturn // Factory function returning interface is acceptable for dependency injection
//nolint:ireturn // Factory function intentionally returns interface for dependency injection
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The comment has been corrected from 'Factory function returning interface is acceptable for dependency injection' to 'Factory function intentionally returns interface for dependency injection', which is clearer and more grammatically correct.

Copilot uses AI. Check for mistakes.

@@ -1,7 +1,7 @@
package config

import (
env "github.com/caarlos0/env/v11"
"github.com/caarlos0/env/v11"
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The import alias has been removed (changed from env \"github.com/caarlos0/env/v11\" to just the standard import). This is good practice as the package name env is already clear and the alias was unnecessary.

Copilot uses AI. Check for mistakes.

@@ -6,7 +6,6 @@ import (
"fmt"
"log"
"net"
"strings"
"time"
Copy link
Preview

Copilot AI Aug 5, 2025

Choose a reason for hiding this comment

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

The unused import \"strings\" has been removed from line 9, which improves code cleanliness. The string manipulation functionality has been moved to the shared validation module.

Copilot uses AI. Check for mistakes.

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