Skip to content

Conversation

@jhrozek
Copy link
Contributor

@jhrozek jhrozek commented Oct 30, 2025

This PR implements the foundational authentication components for Virtual MCP Server, including context propagation, incoming client authentication, and outgoing backend authentication framework.

Commits

Add context helpers for Identity propagation in vmcp auth

Adds several context-related helpers that will be used to propagate
Identity through vMCP.

Related: #2377

Add OIDC incoming authenticator for vmcp

Implement IncomingAuthenticator interface using existing TokenValidator from
pkg/auth. This adapter validates JWT tokens from clients connecting to the
Virtual MCP Server and extracts identity information.

Related: #2377

Add a registry of outgoing auth strategies with a stub of AuthenticateRequest()

Implement OutgoingAuthenticator interface with pluggable authentication
strategies for backend MCP server connections.

The actual strategies will be implemented in a follow-up commit.

Fixes: #2377
Fixes: #2379

@codecov
Copy link

codecov bot commented Oct 30, 2025

Codecov Report

❌ Patch coverage is 77.90698% with 38 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.82%. Comparing base (9c3ad92) to head (0abe419).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
pkg/vmcp/auth/incoming_factory.go 65.21% 15 Missing and 1 partial ⚠️
cmd/vmcp/app/commands.go 0.00% 11 Missing ⚠️
pkg/vmcp/server/server.go 25.00% 4 Missing and 2 partials ⚠️
...g/transport/proxy/transparent/transparent_proxy.go 0.00% 3 Missing ⚠️
pkg/vmcp/auth/auth.go 93.10% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2393      +/-   ##
==========================================
+ Coverage   54.60%   54.82%   +0.21%     
==========================================
  Files         265      271       +6     
  Lines       25591    25748     +157     
==========================================
+ Hits        13975    14116     +141     
- Misses      10295    10303       +8     
- Partials     1321     1329       +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
Copy link
Collaborator

JAORMX commented Oct 30, 2025

@claude please review this in context of the virtual MCP proposal in docs/proposals/THV-2106-virtual-mcp-server.md and do so with a security focus

@claude
Copy link
Contributor

claude bot commented Oct 30, 2025

Claude finished @JAORMX's task —— View job


Security-Focused Review of vMCP Authentication Infrastructure

I've completed a comprehensive security review of the authentication infrastructure implementation for Virtual MCP Server, examining the code against the proposal in docs/proposals/THV-2106-virtual-mcp-server.md.

Todo List

  • Read the Virtual MCP proposal document
  • Review context propagation implementation
  • Review incoming OIDC authenticator
  • Review outgoing authenticator framework
  • Analyze security concerns and test coverage
  • Compile comprehensive security review feedback

Summary

This PR implements solid foundational authentication components with good security practices. The code demonstrates proper separation of concerns, comprehensive test coverage, and adherence to security standards. However, there are several critical security concerns that should be addressed before merging.

Critical Security Issues

1. Missing Token Storage in Identity During Authenticate() Method

Location: pkg/vmcp/auth/incoming_oidc.go:74-75

The Authenticate() method stores the token in the Identity, but this represents a token leakage risk if the Identity object is logged, serialized, or passed to untrusted components.

Risk: If Identity is ever logged (e.g., for debugging) or serialized into audit logs without proper redaction, the bearer token will be exposed in plaintext.

Recommendation:

  • Add documentation warning about token sensitivity
  • Consider whether the raw token needs to be stored at all in the Identity struct, or if it should only be retrievable from context
  • If token storage is necessary for pass-through auth, ensure audit logging and serialization properly redact it
  • Add a Redacted() method to Identity that returns a safe-to-log version
// Example of safe logging pattern:
type Identity struct {
    // ... existing fields ...
    
    // Token is SENSITIVE and should never be logged or serialized without redaction
    Token string `json:"-"` // Exclude from JSON serialization
}

// Redacted returns a copy of the identity safe for logging
func (i *Identity) Redacted() *Identity {
    copy := *i
    copy.Token = "[REDACTED]"
    return copy
}

2. Token Extraction from Authorization Header is Duplicated

Location: pkg/vmcp/auth/incoming_oidc.go:46-61 and 102-108

The token extraction logic appears in both Authenticate() and Middleware() methods. This duplication increases the risk of inconsistencies and bugs.

Risk: If token extraction logic needs to be updated for security reasons (e.g., to handle edge cases), developers must remember to update both locations.

Recommendation: Extract token parsing into a private helper function:

func extractBearerToken(authHeader string) (string, error) {
    if authHeader == "" {
        return "", errors.New("authorization header required")
    }
    
    bearerPrefix := bearerTokenType + " "
    if !strings.HasPrefix(authHeader, bearerPrefix) {
        return "", errors.New("invalid authorization header format, expected 'Bearer <token>'")
    }
    
    token := strings.TrimPrefix(authHeader, bearerPrefix)
    if token == "" {
        return "", errors.New("empty token in authorization header")
    }
    
    return token, nil
}

3. Missing Audience Validation

Location: pkg/vmcp/auth/incoming_oidc.go:64-78

The incoming authenticator validates tokens but does not explicitly verify that the token's audience claim matches the expected vMCP audience as specified in the proposal (line 140-141: "Token must have aud=vmcp").

Risk: Without audience validation, tokens issued for other services could be accepted, enabling cross-service token reuse attacks.

Recommendation:

  • Verify that the underlying TokenValidator enforces audience validation
  • Add explicit audience checking or document that it's delegated to TokenValidator
  • Add test cases specifically for audience validation failure scenarios

4. Thread-Safety Documentation Missing for DefaultOutgoingAuthenticator

Location: pkg/vmcp/auth/outgoing_authenticator.go:11-26

While the implementation correctly uses sync.RWMutex for thread-safety, there's insufficient documentation about the concurrent access patterns and guarantees.

Security Impact: Without clear thread-safety guarantees documented, future maintainers might introduce race conditions when extending this code.

Recommendation: Add comprehensive documentation about:

  • Thread-safety guarantees for all public methods
  • Whether strategies themselves need to be thread-safe
  • Expected concurrent usage patterns in HTTP servers

High Priority Issues

5. No Rate Limiting on Authentication Attempts

Location: pkg/vmcp/auth/incoming_oidc.go:44-78

There's no rate limiting on failed authentication attempts, making the system vulnerable to brute-force attacks.

Risk: Attackers could repeatedly attempt authentication with invalid tokens, potentially causing:

  • DoS through resource exhaustion
  • Token guessing attacks
  • Log flooding

Recommendation: Add rate limiting middleware before authentication, keyed by source IP or other identifiers.

6. Missing Token Expiry Validation Documentation

Location: pkg/vmcp/auth/incoming_oidc.go:64

While TokenValidator.ValidateToken() likely checks expiry, there's no explicit documentation confirming this critical security check.

Risk: If token expiry isn't validated, expired tokens could be used indefinitely.

Recommendation:

  • Add explicit documentation that token expiry is validated by TokenValidator
  • Consider adding an integration test that verifies expired tokens are rejected

7. Strategy Metadata is map[string]any Without Validation

Location: pkg/vmcp/auth/auth.go:68 and pkg/vmcp/auth/outgoing_authenticator.go:110-120

The Strategy.Authenticate() method accepts unvalidated map[string]any metadata, which could lead to type confusion or injection attacks.

Risk:

  • Type assertion failures causing panics
  • Injection attacks if metadata is used to construct URLs or commands
  • Missing required configuration leading to silent failures

Recommendation:

  • Call Strategy.Validate() before Strategy.Authenticate() in AuthenticateRequest()
  • Add validation that ensures required metadata fields are present and properly typed
  • Document expected metadata schema for each strategy
func (a *DefaultOutgoingAuthenticator) AuthenticateRequest(
    ctx context.Context,
    req *http.Request,
    strategyName string,
    metadata map[string]any,
) error {
    strategy, err := a.GetStrategy(strategyName)
    if err != nil {
        return err
    }
    
    // Validate metadata before use
    if err := strategy.Validate(metadata); err != nil {
        return fmt.Errorf("invalid metadata for strategy %q: %w", strategyName, err)
    }
    
    return strategy.Authenticate(ctx, req, metadata)
}

Medium Priority Issues

8. Groups Field Intentionally Left Empty

Location: pkg/vmcp/auth/incoming_oidc.go:132

The claimsToIdentity() function intentionally doesn't extract groups from claims, leaving that to authorization logic. This design decision should be more clearly documented.

Security Impact: Authorization logic must explicitly handle group extraction. If future code assumes groups are populated, authorization bypasses could occur.

Recommendation: Add prominent documentation explaining:

  • Why groups aren't extracted here
  • Where groups should be extracted
  • Security implications of this design choice

9. Identity Context Key Collision Risk

Location: pkg/vmcp/auth/context.go:9-10

While using an empty struct as a context key is correct, there's no package-level uniqueness guarantee if other packages use similar patterns.

Security Impact: Low risk of context value collision, but could lead to authentication bypasses if another package overwrites the identity.

Recommendation: Consider using a more explicit key type or add documentation about the collision prevention strategy.

10. No Input Validation on Identity Fields

Location: pkg/vmcp/auth/incoming_oidc.go:129-156

The claimsToIdentity() function validates that sub is present but doesn't validate the format or length of any identity fields.

Security Impact: Maliciously crafted tokens could include extremely long strings causing memory issues or breaking downstream assumptions.

Recommendation: Add validation for:

  • Maximum lengths for Subject, Name, Email
  • Email format validation (if used for authorization)
  • Subject format (alphanumeric, etc.)
  • Maximum number of claims to prevent memory exhaustion

Positive Security Practices

✅ Strong Points

  1. Proper Token Validation Delegation: Uses existing TokenValidator from pkg/auth, avoiding reimplementation of complex JWT validation logic
  2. Comprehensive Test Coverage: Excellent test coverage including edge cases, concurrent access, and error conditions (86.96% patch coverage)
  3. Thread-Safe Implementation: Proper use of sync.RWMutex in DefaultOutgoingAuthenticator
  4. OIDC Compliance: Correctly enforces sub claim requirement per OpenID Connect Core 1.0 section 5.1
  5. Immutability: claimsToIdentity() doesn't modify original claims (verified in tests)
  6. Security-First Design: Clear separation between incoming and outgoing authentication boundaries
  7. Extensible Architecture: Plugin-based strategy pattern allows for custom authentication methods
  8. Context Propagation: Type-safe context storage prevents accidental overwrites

✅ Test Coverage Highlights

  • Concurrent access testing with race detector (-race flag)
  • Boundary condition testing (empty tokens, missing headers, invalid formats)
  • Error path testing for all validation failures
  • Integration testing between middleware and handlers

Alignment with Virtual MCP Proposal

✅ Matches Proposal Requirements

  1. Two Authentication Boundaries (lines 136-148): ✅ Correctly implements IncomingAuthenticator and OutgoingAuthenticator interfaces
  2. Identity Propagation (lines 273-304): ✅ Context helpers properly store and retrieve Identity
  3. Token Storage for Pass-Through (line 378): ✅ Identity stores original token for backend auth
  4. Zero-Trust Design (lines 189-210): ✅ Architecture supports independent validation at each boundary
  5. Pluggable Strategies (lines 84-91): ✅ Strategy pattern enables multiple backend auth methods

⚠️ Missing from Proposal

  1. Audience Validation: Proposal specifies aud=vmcp validation (line 140) but implementation doesn't explicitly show this
  2. Token Caching: Proposal mentions token caching (lines 380-386, 912-961) but not implemented in this PR (acceptable for foundational work)
  3. Backend Strategies: Only framework is present, actual strategies (pass_through, token_exchange, etc.) will come in follow-up PRs

Recommendations

Immediate Actions (Before Merge)

  1. Fix token leakage risk: Add token redaction to Identity struct and document sensitivity
  2. Extract token parsing logic: Create shared helper to prevent duplication
  3. Verify audience validation: Confirm TokenValidator checks audience or add explicit check
  4. Validate strategy metadata: Call Strategy.Validate() before Authenticate()
  5. Document thread-safety: Add clear concurrency guarantees to all public APIs

Follow-Up Work (Future PRs)

  1. Add rate limiting middleware for authentication endpoints
  2. Implement input validation for identity fields (length limits, format checks)
  3. Add token expiry validation integration tests
  4. Implement audit logging with proper token redaction
  5. Add monitoring/alerting for authentication failures
  6. Implement actual backend authentication strategies (token_exchange, pass_through, etc.)

Architecture Review

The code follows ToolHive's existing patterns well:

  • ✅ Uses existing TokenValidator from pkg/auth
  • ✅ Follows middleware patterns from pkg/auth
  • ✅ Uses standard context propagation patterns
  • ✅ Integrates with existing JWT validation infrastructure
  • ✅ Maintains clean separation of concerns

The architecture is well-designed for the Virtual MCP use case and sets a solid foundation for outgoing authentication strategies.

Test Coverage Analysis

From codecov report: 86.96% patch coverage with 12 lines missing coverage in pkg/vmcp/auth/incoming_oidc.go.

Missing coverage areas:

  • Line 66: Token validation error path
  • Lines 71-72: Claims to identity conversion error path
  • Some middleware error paths

Recommendation: Add integration tests that trigger these error paths, particularly:

  • Invalid token format from TokenValidator
  • Token validation network failures
  • Malformed claims from provider

Conclusion

This is solid foundational work with good security practices and comprehensive testing. The critical issues identified are relatively straightforward to fix and don't require architectural changes. I recommend addressing the critical issues (especially token leakage and audience validation) before merging.

The code aligns well with the Virtual MCP proposal and provides a secure foundation for the authentication infrastructure. Once the identified issues are addressed, this will be a strong base for implementing the remaining authentication strategies.

Recommendation: Request changes to address critical security issues, then approve.


Files Reviewed

  • pkg/vmcp/auth/context.go - Context helpers for Identity propagation
  • pkg/vmcp/auth/context_test.go - Context helper tests
  • pkg/vmcp/auth/incoming_oidc.go - OIDC incoming authenticator
  • pkg/vmcp/auth/incoming_oidc_test.go - Comprehensive authenticator tests
  • pkg/vmcp/auth/outgoing_authenticator.go - Outgoing auth framework
  • pkg/vmcp/auth/outgoing_authenticator_test.go - Outgoing auth tests
  • pkg/vmcp/auth/auth.go - Core interfaces and types
  • docs/proposals/THV-2106-virtual-mcp-server.md - Virtual MCP proposal

@jhrozek
Copy link
Contributor Author

jhrozek commented Oct 31, 2025

It is really hard to reply to the AI review wall of text :-(

  1. Missing Token Storage in Identity During Authenticate() Method

maybe, I added a String() and MarshalJSON methods to the commit "Add token redaction to Identity serialization". Going forward, we should re-evaluate the passthrough method and see if we need to carry the token through.

  1. Token Extraction from Authorization Header is Duplicated

this was fixed in a separate commit

  1. Missing Audience Validation

AI review slop, claude doesn't understand the code. The audience validation happens in TokenValidator.validateClaims() called from OIDCIncomingAuthenticator. The thing we need to do is to wire the proper configuration in a follow-up.

  1. Thread-Safety Documentation Missing for DefaultOutgoingAuthenticator

I added a comment to stop AI from being confused.

  1. No Rate Limiting on Authentication Attempts

rate-limit middleware should come later, not the point of this PR

  1. Missing Token Expiry Validation Documentation

AI slop, is checked down the call chain

  1. Strategy Metadata is map[string]any Without Validation

fair, I added validation

  1. Groups Field Intentionally Left Empty

this is by design. I actually don't think the groups will be useful but we can remove them later

  1. Identity Context Key Collision Risk

I really don't think this is an issue

  1. No Input Validation on Identity Fields

OK, this is a problem in thv in general and should be fixed in the general code.

Copy link
Collaborator

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

@jhrozek why not reuse pkg/auth? You would already have GetClaimsFromContext which would allow you to reuse the auth middleware? That auth middleware could then be used by the authz middleware... and so on. I'm concerned that by introducing new structures and a new key to store and fetch them from the context we would be hampering reusability

Adds several context-related helpers that will be used to propagate
Identity through vMCP.

Related: #2377
Implement IncomingAuthenticator interface using existing TokenValidator from
pkg/auth. This adapter validates JWT tokens from clients connecting to the
Virtual MCP Server and extracts identity information.

Related: #2377
…eRequest()

Implement OutgoingAuthenticator interface with pluggable authentication
strategies for backend MCP server connections.

The actual strategies will be implemented in a follow-up commit.

Fixes: #2377
Implement String() and MarshalJSON() methods on the Identity struct to
prevent accidental token leakage when logging or serializing identities.
Add concise documentation explaining why Identity.Groups is intentionally
not populated by OIDCIncomingAuthenticator. This clarifies that group
extraction is an authorization concern handled via the Claims map, as
different OIDC providers use different claim names.
Add explicit documentation that RegisterStrategy and AuthenticateRequest
are safe for concurrent use, and that Strategy implementations must be
thread-safe.
Call strategy.Validate() before strategy.Authenticate() to catch invalid
or malicious metadata early. This prevents type confusion, injection
attacks, and panics from invalid metadata in strategy implementations.

Changes:
- Add Validate() call in AuthenticateRequest()
- Proper error wrapping with strategy name
- Add test verifying validation is enforced
- Update existing tests to expect Validate() calls
Implement new authentication layer that converts JWT claims from
pkg/auth.TokenValidator to vMCP's Identity domain type.

Changes:
- Add ClaimsToIdentity() to convert JWT claims to Identity struct
  * Validates required 'sub' claim per OIDC Core 1.0 § 5.1
  * Preserves original Bearer token for passthrough scenarios
  * Intentionally does not populate Groups field (stays in Claims)
- Add IdentityMiddleware() to apply conversion in HTTP middleware chain
  * Runs after TokenValidator.Middleware() which stores Claims
  * Passes through unauthenticated requests unchanged
- Add NewOIDCAuthMiddleware() factory helper
  * Composes TokenValidator + IdentityMiddleware into single chain
  * Returns authInfo handler for OIDC discovery endpoint
- Add comprehensive test coverage for conversion logic

This establishes the foundation for simplified vMCP authentication that
reuses pkg/auth.GetAuthenticationMiddleware() instead of duplicating
token validation logic.

Affected components:
- pkg/vmcp/auth (new files)
Update server configuration and routing to support optional authentication
middleware for MCP endpoints while keeping health endpoints unauthenticated.
Integrate NewOIDCAuthMiddleware() into the serve command to enable
authentication based on configuration.
Remove the now-unused OIDCIncomingAuthenticator that wrapped
TokenValidator, along with its interfaces and tests. This code
is replaced by the simpler NewOIDCAuthMiddleware() helper that
directly composes pkg/auth middleware with IdentityMiddleware.
Create NewWellKnownHandler() in pkg/auth to provide RFC 9728 compliant
routing for /.well-known/oauth-protected-resource endpoints.

This fixes a bug where vMCP server only matched exact paths, causing
requests to subpaths like /.well-known/oauth-protected-resource/mcp
to fall through to authenticated routes and return 401.
Replace inline well-known handler implementations with the reusable
auth.NewWellKnownHandler() to eliminate code duplication.

This fixes a bug in vMCP server where only the exact path
/.well-known/oauth-protected-resource was registered, causing requests
to subpaths (e.g., /mcp) to fall through to authenticated routes and
return 401 instead of discovery metadata.

The reusable handler ensures consistent behavior across both components
and maintains the requirement that discovery endpoints remain
unauthenticated.
Add support for OAuth 2.0 resource indicators (RFC 8707) in vMCP
incoming authentication configuration.

The Resource field is used in WWW-Authenticate headers and OAuth
discovery metadata per RFC 9728. When not specified, it defaults to
the Audience value for backward compatibility.

This allows operators to explicitly configure the resource identifier
that appears in 401 responses, separate from the token audience claim
validation.
Wire the Resource field from config through to the TokenValidator by
using it as the ResourceURL in TokenValidatorConfig.

The ResourceURL is used in WWW-Authenticate challenge headers when
authentication fails. This allows operators to specify a different
resource identifier than the audience claim being validated.
@jhrozek
Copy link
Contributor Author

jhrozek commented Nov 2, 2025

@jhrozek why not reuse pkg/auth? You would already have GetClaimsFromContext which would allow you to reuse the auth middleware? That auth middleware could then be used by the authz middleware... and so on. I'm concerned that by introducing new structures and a new key to store and fetch them from the context we would be hampering reusability

We chatted about this with @JAORMX on slack and it was apparent I got confused about the incoming interface. I ended up getting rid of it and using just plain middleware instead.

With this PR push, I also added the wiring between the serve command and the auth middleware and I was able to successfully authenticate to vMCP backed server from VSCode against Okta.

Some minor refactoring was needed to reuse code to handle the well-known OAuth endpoints and add a Resource parameter to the config. I can split these to different PRs (they probably should be separate), but I wanted to get the end-to-end auth working. I also manually tested anonymous and local auth.

@jhrozek
Copy link
Contributor Author

jhrozek commented Nov 2, 2025

@claude redo the review, focusing on security and alignment with OAuth standard as well as reuse of the toolhive auth middleware. When generating your review, don't be verbose and do take into account my reply to your previous review in #2393 (comment)

@claude
Copy link
Contributor

claude bot commented Nov 2, 2025

Claude finished @jhrozek's task —— View job


Security-Focused Review of vMCP Authentication (Revised)

Todo List

  • Read changed files and understand current implementation
  • Review security concerns (OAuth compliance, token handling)
  • Check middleware reuse patterns
  • Compile concise review feedback

Summary

The refactored implementation successfully addresses most concerns from the previous review. The removal of the IncomingAuthenticator interface in favor of direct middleware reuse from pkg/auth is a significant improvement. The code now properly leverages ToolHive's existing authentication infrastructure.

Critical Issues Resolved ✓

  1. Middleware Reuse: Now properly uses pkg/auth.GetAuthenticationMiddleware() for OIDC, local, and anonymous auth
  2. Token Extraction: Deduplicated - now uses auth.ExtractBearerToken()
  3. Token Leakage: Fixed with String() and MarshalJSON() redaction methods
  4. Strategy Validation: Now calls Strategy.Validate() before Authenticate() (pkg/vmcp/auth/outgoing_authenticator.go:125)
  5. Audience Validation: Confirmed - TokenValidator.validateClaims() checks audience (pkg/auth/token.go:688-696)

Remaining Security Concerns

1. Resource Parameter Not Wired to TokenValidator ⚠️

Location: pkg/vmcp/auth/incoming_factory.go:80-89

The Resource field is read from config but not passed to TokenValidator. The TokenValidatorConfig struct has a ResourceURL field (line 89), but audience validation still uses the Audience field.

Issue: RFC 9728 OAuth Protected Resource metadata and RFC 8707 resource indicators require that the resource parameter be properly validated. The warning "No Resource defined in OIDC configuration" (line 82) suggests this is expected to be set.

Recommendation:

  • Verify that TokenValidator uses ResourceURL for validation when present
  • If not, update TokenValidator.validateClaims() to check ResourceURL as an alternative to Audience
  • Add integration test verifying tokens with resource claim are validated correctly

2. Well-Known Handler Doesn't Validate Subpaths ⚠️

Location: pkg/auth/well_known.go:52

The handler uses strings.HasPrefix() to match /.well-known/oauth-protected-resource and any subpaths. This is correct per RFC 9728, but there's no validation that subpaths are expected.

Issue: An attacker could probe for information by requesting arbitrary subpaths like /.well-known/oauth-protected-resource/../../etc/passwd.

Recommendation: Path traversal is unlikely here (no file access), but document that authInfoHandler must validate/sanitize paths if it handles subpaths differently.

3. Identity Token Storage Design Requires Documentation ℹ️

Location: pkg/vmcp/auth/auth.go:78-79

While token redaction is now implemented, the security implications of storing the raw token in Identity need clearer documentation for future maintainers.

Risk: If passthrough authentication is removed in the future, developers may not realize the token should also be removed from the Identity struct.

Recommendation: Add prominent comment:

// Token is the original authentication token.
// SECURITY: Only stored for passthrough authentication to backend APIs.
// If passthrough auth is removed, this field should be removed to minimize
// credential exposure. Never log or serialize without redaction.

Medium Priority Issues

4. Anonymous Auth Creates No Identity ℹ️

Location: pkg/vmcp/auth/incoming_factory.go:120-123

Anonymous middleware returns nil for authInfoHandler, and auth.AnonymousMiddleware likely doesn't create Claims in context, meaning no Identity is created.

Security Impact: Authorization logic must explicitly handle the case where IdentityFromContext() returns (nil, false). If authz code assumes Identity is always present, it could lead to bypasses.

Recommendation: Document this behavior clearly and add integration test verifying authz correctly handles anonymous requests.

5. Groups Field Still Present Despite Being Unused ℹ️

Location: pkg/vmcp/auth/auth.go:68-73

The comment explains why Groups is not populated, but the field remains in the struct.

Recommendation: Consider removing the Groups field entirely if it won't be used. If kept for future use, the current documentation is sufficient.

OAuth/OIDC Compliance ✓

Excellent alignment with standards:

  1. OIDC Core 1.0: Correctly requires sub claim (identity.go:66-70)
  2. RFC 9728: Implements OAuth Protected Resource metadata endpoint via WellKnownHandler
  3. RFC 8707: Accepts Resource parameter in config (though validation needs verification)
  4. Token Validation: Delegates to pkg/auth.TokenValidator which properly validates:
    • Signature (via JWKS)
    • Expiration (exp claim)
    • Audience (aud claim) - verified in token.go:688-696
    • Issuer (iss claim)

Middleware Reuse Patterns ✓

Strong reuse of existing infrastructure:

  1. OIDC: Uses auth.GetAuthenticationMiddleware(ctx, oidcConfig) (incoming_factory.go:93)
  2. Local: Uses auth.GetAuthenticationMiddleware(ctx, nil) (incoming_factory.go:110)
  3. Anonymous: Uses auth.AnonymousMiddleware (incoming_factory.go:123)
  4. Token Extraction: Uses auth.ExtractBearerToken() (identity.go:36)
  5. Claims Context: Uses auth.GetClaimsFromContext() (identity.go:27)

Architecture: The layered approach (AuthMiddleware → IdentityMiddleware) is clean and maintains separation of concerns:

  • pkg/auth handles token validation → Claims
  • pkg/vmcp/auth handles Claims → Identity conversion

Test Coverage ✓

From codecov: 86.96% patch coverage is good. The comprehensive test suite in identity_test.go covers:

  • Claims to Identity conversion edge cases
  • Missing/invalid sub claim handling
  • Token extraction scenarios
  • Middleware composition

Alignment with Virtual MCP Proposal ✓

  1. Two Authentication Boundaries (proposal lines 136-148): ✓ Correctly implemented
  2. Token Audience aud=vmcp (proposal line 140): ⚠️ Needs verification that Resource parameter is validated
  3. Reuse of ToolHive Auth (proposal line 142): ✓ Excellent reuse
  4. Identity Propagation: ✓ Clean context-based propagation
  5. Outgoing Auth Framework: ✓ Solid foundation with strategy pattern

Recommendations

Before Merge

  1. Verify Resource parameter validation - Confirm TokenValidator validates ResourceURL when present
  2. Add Resource validation test - Integration test with resource parameter in token claims

Follow-Up Work

  1. Rate limiting - Add to prevent brute-force attacks (as noted by original reviewer)
  2. Input validation - Add max length checks for Subject/Email/Name fields
  3. Anonymous auth documentation - Document authz handling for anonymous users
  4. Token caching - Implement per proposal (lines 380-386, 912-961)

Conclusion

This is high-quality work that successfully addresses the previous review feedback. The refactoring to reuse pkg/auth middleware is exactly the right approach and significantly improves maintainability and security posture.

The only remaining concern is verifying that the Resource parameter is properly validated by TokenValidator. Once confirmed, this PR provides a solid, secure foundation for vMCP authentication.

Recommendation: Approve with minor follow-up to verify Resource parameter validation.


Files Reviewed

  • pkg/vmcp/auth/auth.go - Core types and Identity
  • pkg/vmcp/auth/identity.go - IdentityMiddleware and claims conversion
  • pkg/vmcp/auth/incoming_factory.go - Middleware factory for all auth types
  • pkg/vmcp/auth/context.go - Context helpers
  • pkg/vmcp/auth/outgoing_authenticator.go - Outgoing auth framework
  • pkg/auth/well_known.go - RFC 9728 well-known handler
  • pkg/vmcp/server/server.go - Server integration
  • pkg/vmcp/config/config.go - Configuration model
  • cmd/vmcp/app/commands.go - CLI wiring

@jhrozek
Copy link
Contributor Author

jhrozek commented Nov 2, 2025

Quick reply to claude review:

  1. Resource Parameter Not Wired to TokenValidator

it's not supposed to be, it's supposed to just advise clients

Well-Known Handler Doesn't Validate Subpaths

this is expected, can be hardened later

Identity Token Storage Design Requires Documentation

follow up work

Anonymous Auth Creates No Identity

incorrect, it does, in the shared auth code:

  1. anonymous.go:20-39 - AnonymousMiddleware creates Claims:
  claims := jwt.MapClaims{
      "sub":   "anonymous",
      "email": "anonymous@localhost",
      "name":  "Anonymous User",
      // ...
  }
  ctx := context.WithValue(r.Context(), ClaimsContextKey{}, claims)
  2. incoming_factory.go:61-63 - All auth types are composed the same way:
  composed := func(next http.Handler) http.Handler {
      return baseAuthMiddleware(IdentityMiddleware(next))
  }

Groups Field Still Present Despite Being Unused

later

@jhrozek
Copy link
Contributor Author

jhrozek commented Nov 2, 2025

@JAORMX another thing we discussed was the use of the Identity struct. I kept it in, I think the token passing will find it useful. The primary piece to look at is IdentityMiddleware which just grabs the claims from the auth middleware via GetClaimsFromContext and then converts to the Identity struct.

Copy link
Collaborator

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I wonder if we should merge the Identity middleware with the existing auth middleware. Returning claims as we do today is minimal... but maybe we could just return the Identity struct instead and simplify.

@jhrozek jhrozek merged commit db9a6f5 into main Nov 3, 2025
29 checks passed
@jhrozek jhrozek deleted the vmcp_auth branch November 3, 2025 09:29
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.

Wire authentication configuration through vMCP discovery and server setup Implement core authentication infrastructure for vMCP

3 participants