-
Notifications
You must be signed in to change notification settings - Fork 137
Implement core authentication infrastructure for vMCP #2393
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
Conversation
Codecov Report❌ Patch coverage is 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. 🚀 New features to boost your workflow:
|
|
@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 finished @JAORMX's task —— View job Security-Focused Review of vMCP Authentication InfrastructureI've completed a comprehensive security review of the authentication infrastructure implementation for Virtual MCP Server, examining the code against the proposal in Todo List
SummaryThis 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 Issues1. Missing Token Storage in Identity During Authenticate() MethodLocation: The 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:
// 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 DuplicatedLocation: The token extraction logic appears in both 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 ValidationLocation: 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:
4. Thread-Safety Documentation Missing for DefaultOutgoingAuthenticatorLocation: While the implementation correctly uses Security Impact: Without clear thread-safety guarantees documented, future maintainers might introduce race conditions when extending this code. Recommendation: Add comprehensive documentation about:
High Priority Issues5. No Rate Limiting on Authentication AttemptsLocation: 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:
Recommendation: Add rate limiting middleware before authentication, keyed by source IP or other identifiers. 6. Missing Token Expiry Validation DocumentationLocation: While Risk: If token expiry isn't validated, expired tokens could be used indefinitely. Recommendation:
7. Strategy Metadata is
|
|
It is really hard to reply to the AI review wall of text :-(
maybe, I added a
this was fixed in a separate commit
AI review slop, claude doesn't understand the code. The audience validation happens in
I added a comment to stop AI from being confused.
rate-limit middleware should come later, not the point of this PR
AI slop, is checked down the call chain
fair, I added validation
this is by design. I actually don't think the groups will be useful but we can remove them later
I really don't think this is an issue
OK, this is a problem in thv in general and should be fixed in the general code. |
There was a problem hiding this 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.
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 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. |
|
@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 finished @jhrozek's task —— View job Security-Focused Review of vMCP Authentication (Revised)Todo List
SummaryThe refactored implementation successfully addresses most concerns from the previous review. The removal of the Critical Issues Resolved ✓
Remaining Security Concerns1. Resource Parameter Not Wired to TokenValidator
|
|
Quick reply to claude review:
it's not supposed to be, it's supposed to just advise clients
this is expected, can be hardened later
follow up work
incorrect, it does, in the shared auth code:
later |
|
@JAORMX another thing we discussed was the use of the |
There was a problem hiding this 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.
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