feat(gateway): MCP API key + OAuth 2.1 auth [b51i+rcvm]#271
feat(gateway): MCP API key + OAuth 2.1 auth [b51i+rcvm]#271
Conversation
Phase 2 — MCP API key (ASYA_MCP_API_KEY): - Add BearerTokenAuthenticator (Authorization: Bearer <static-token>) - Add MCPAuthMiddleware: wraps /mcp, /mcp/sse, /tools/call - Disabled (no-op) when no authenticators configured (dev mode) - Returns 401 + WWW-Authenticate: Bearer on failure (RFC 6750) - Wire ASYA_MCP_API_KEY in main.go Phase 3 — MCP OAuth 2.1 + PKCE (ASYA_MCP_OAUTH_ENABLED): - Add OAuthBearerAuthenticator: validates gateway-issued HMAC-SHA256 JWTs - New internal/oauth package: - /.well-known/oauth-protected-resource (RFC 9728) - /.well-known/oauth-authorization-server (RFC 8414) - POST /oauth/register (RFC 7591 Dynamic Client Registration) - GET /oauth/authorize (Authorization Code + auto-approve) - POST /oauth/token (code exchange + refresh token rotation) - PKCE S256 verification - DB migration 010_oauth_tables (oauth_clients, oauth_authorization_codes, oauth_refresh_tokens) - Wire ASYA_MCP_OAUTH_ISSUER, ASYA_MCP_OAUTH_SECRET, ASYA_MCP_OAUTH_TOKEN_TTL Both phases use the existing Authenticator interface with OR semantics: API key and OAuth tokens are accepted in parallel.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces API key and OAuth 2.1 authentication for MCP endpoints, a significant feature that includes dynamic client registration, authorization code flow with PKCE, and refresh token rotation. However, several critical security vulnerabilities have been identified. These include predictable token generation due to ignored errors in the randomness source, two race conditions (TOCTOU) in the token and code exchange logic that could allow for token reuse, authentication bypass enabled by open dynamic client registration, and a 'fail open' behavior in the MCP authentication middleware. Additionally, issues such as lack of scope validation, potential for memory-based DoS in the registration endpoint, and recommendations for database schema integrity and context propagation need to be addressed. These issues must be resolved before merging.
…rcvm] Critical fixes: - TOCTOU auth code: replace findAuthCode+markAuthCodeUsed with atomic consumeAuthCode (UPDATE ... RETURNING) — concurrent token requests with the same code can no longer both succeed - TOCTOU refresh token: replace findRefreshToken+revokeRefreshToken with atomic consumeRefreshToken (UPDATE ... RETURNING) — token rotation is safe - rand.Read error: panic explicitly on entropy source failure (was silently ignored, could produce predictable tokens on low-entropy systems) Security improvements: - /oauth/register: add optional ASYA_MCP_OAUTH_REGISTRATION_TOKEN guard; when set, Bearer token required to register new clients - /oauth/authorize: validate requested scopes against client's registered scopes (intersect), reject if no valid scopes remain - /oauth/register: add http.MaxBytesReader(1MiB) to prevent DoS via large JSON payloads - DB: add ON DELETE CASCADE to FK constraints in oauth_authorization_codes and oauth_refresh_tokens — client deletion cleans up child rows atomically Code quality: - issueTokenResponse: propagate request context instead of context.Background() - HandleAuthorize: handle url.Parse error for registered redirect URIs; add comment explaining SSRF mitigation (allowlist validation before parse) - MCPAuthMiddleware: clarify intent of fail-open behavior with comment (intentional dev-mode design, not a security gap when ASYA_MCP_API_KEY is unset and the deployment is network-restricted)
|
@gemini-code-assist review |
Open redirect (Codacy CWE-601): - HandleAuthorize now looks up the canonical redirect URI from the DB-sourced client.RedirectURIs list via findRegisteredURI() and uses that value (not raw r.URL.Query() input) for url.Parse + http.Redirect - Breaks the user-input taint chain: redirect target provenance is DB, not request query string Code clone: - Extract extractBearerToken(r) helper that parses Authorization: Bearer header — was duplicated across JWTAuthenticator, BearerTokenAuthenticator, and OAuthBearerAuthenticator
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive authentication layer for MCP endpoints, supporting both static API keys and a full OAuth 2.1 flow, including a full OAuth server in the internal/oauth package. However, the OAuth server allows open client registration with auto-approval if a registration token is not explicitly set, which should be changed to a 'fail-secure' default to ensure the gateway remains protected even in the event of configuration errors. Additionally, a high-severity security recommendation regarding the comparison of the registration token to prevent potential timing attacks has been identified.
| if s.cfg.RegistrationToken != "" { | ||
| authHeader := r.Header.Get("Authorization") | ||
| provided := strings.TrimPrefix(authHeader, "Bearer ") | ||
| if !strings.HasPrefix(authHeader, "Bearer ") || provided != s.cfg.RegistrationToken { | ||
| w.Header().Set("WWW-Authenticate", `Bearer realm="asya-gateway"`) | ||
| writeOAuthError(w, http.StatusUnauthorized, "unauthorized_client", "registration token required") | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
The OAuth 2.1 implementation allows for open dynamic client registration if ASYA_MCP_OAUTH_REGISTRATION_TOKEN is not set. This can lead to unauthenticated attackers obtaining valid OAuth access tokens with mcp:invoke and mcp:read scopes, effectively bypassing intended security controls. Furthermore, the comparison of the registration token is not performed in constant time, which could expose the endpoint to timing attacks. To remediate, require a registration token by default, making open registration an explicit opt-in. Also, consider if auto-approval is appropriate for all dynamically registered clients. For the token comparison, use crypto/subtle.ConstantTimeCompare for a secure, constant-time comparison, consistent with other authenticators in the codebase. Remember to add import "crypto/subtle".
| if s.cfg.RegistrationToken != "" { | |
| authHeader := r.Header.Get("Authorization") | |
| provided := strings.TrimPrefix(authHeader, "Bearer ") | |
| if !strings.HasPrefix(authHeader, "Bearer ") || provided != s.cfg.RegistrationToken { | |
| w.Header().Set("WWW-Authenticate", `Bearer realm="asya-gateway"`) | |
| writeOAuthError(w, http.StatusUnauthorized, "unauthorized_client", "registration token required") | |
| return | |
| } | |
| } | |
| if !strings.HasPrefix(authHeader, "Bearer ") || subtle.ConstantTimeCompare([]byte(provided), []byte(s.cfg.RegistrationToken)) != 1 { |
References
- New API endpoints that allow modification of system configuration, such as tool registration, must be protected by authentication and authorization middleware.
- Add test_mcp_auth.py covering 401/200 behavior when ASYA_MCP_API_KEY is configured: missing token, wrong token, wrong scheme, correct token, RFC 6750 WWW-Authenticate header, public endpoints unaffected - Add docker-compose.auth.yml that starts gateway with ASYA_MCP_API_KEY set and passes MCP_API_KEY to tester container - Add test-auth Makefile target in gateway-mcp suite - Wire gateway-mcp suite (both test and test-auth) into testing/component/Makefile
The redirect target (u.String()) comes from client.RedirectURIs in the database — validated by findRegisteredURI() before use. Gosec's taint engine still flags the call because r (*http.Request) is marked tainted from the earlier r.URL.Query() reads in the same function, even though r is only used for HTTP protocol detection in http.Redirect, not as the URL. Add #nosec G601 annotation with explanation.
| params.Set("state", state) | ||
| } | ||
| u.RawQuery = params.Encode() | ||
| http.Redirect(w, r, u.String(), http.StatusFound) // #nosec G601 -- redirect target is client.RedirectURIs[i] (DB-sourced), not raw user input |
There was a problem hiding this comment.
Important
❗ Severity: LOW
CWE: ['CWE-918: Server-Side Request Forgery (SSRF)']
📟 Finding Description: A request was found to be crafted from user-input r. This can lead to Server-Side Request Forgery (SSRF) vulnerabilities, potentially exposing sensitive data. It is recommend where possible to not allow user-input to craft the base request, but to be treated as part of the path or query parameter. When user-input is necessary to craft the request, it is recommended to follow OWASP best practices to prevent abuse, including using an allowlist.
🤖 AI Analysis
Exploitability: It is not likely to be exploitable.
Comment: This issue looks unexploitable because user-controlled input is not present in the vulnerable code section. The code properly handles the configuration and only utilizes server-side values such as 'cfg.Issuer' to build URL components. There doesn't appear to be any direct manipulation of URL components based on externally supplied input.
Example Fix Snippet:
No Code Sample
Explaination of the Fix: ---
🔍Leveraging GPT-4 for a preliminary analysis of the Semgrep results. Take note that false positives are possible. A manual code review is required for confirmation.
Note
We understand that not every detected issue may be relevant or pose a security risk. If this issue doesn't apply to your situation, such as if the resource in question is local or it is only test code such as unittest, you have the option to ignore it.
You can find all the rules under the repository, https://github.com/deliveryhero/sast_rules/ and contribute with a pull request. If think the rule of the issue is requires improvement, please do not hesitate to create a pull request.
…arning http.Redirect(w, r, url, code) uses r only to resolve relative URLs. Since registeredURI is always absolute (DB-stored full URI), set the Location header directly and call w.WriteHeader instead — r never appears in the redirect code path, closing the taint chain for both Gosec and Opengrep without needing #nosec annotations.
2 participants
Summary
Implements MCP authentication in two phases, combined in one PR per aint tasks b51i (Phase 2) and rcvm (Phase 3).
Phase 2 — MCP API Key (
ASYA_MCP_API_KEY)BearerTokenAuthenticator: validatesAuthorization: Bearer <static-token>with constant-time comparisonMCPAuthMiddleware: wraps/mcp,/mcp/sse,/tools/call— disabled (no-op) when unconfigured, returns401 + WWW-Authenticate: Beareron failure (RFC 6750)apiandtestinggateway modesPhase 3 — MCP OAuth 2.1 + PKCE (
ASYA_MCP_OAUTH_ENABLED)OAuthBearerAuthenticator: validates self-issued HMAC-SHA256 JWTs viagolang-jwt/v5internal/oauthpackage implementing the full authorization server:GET /.well-known/oauth-protected-resource(RFC 9728 discovery)GET /.well-known/oauth-authorization-server(RFC 8414 metadata)POST /oauth/register(RFC 7591 Dynamic Client Registration)GET /oauth/authorize(Authorization Code flow, auto-approve for machine clients)POST /oauth/token(code exchange + refresh token rotation with PKCE S256)010_oauth_tablesaddsoauth_clients,oauth_authorization_codes,oauth_refresh_tokensASYA_MCP_OAUTH_ISSUER,ASYA_MCP_OAUTH_SECRET,ASYA_MCP_OAUTH_TOKEN_TTLBoth phases use the existing
Authenticatorinterface with OR semantics — API key and OAuth Bearer tokens accepted simultaneously.Architecture Notes
MCPAuthMiddleware(authenticators...)returns a no-op middleware when zero authenticators are passed (dev mode, no key configured)buildRoutesnow acceptsmcpMiddlewareandoauthSrvparameters; nil-safe (identity middleware when nil)Test Plan
make test-unit)BearerTokenAuthenticator(valid/invalid/missing/wrong-scheme),OAuthBearerAuthenticator(valid/expired/wrong-alg),MCPAuthMiddleware(enabled/disabled/valid/invalid/missing), PKCE S256 (valid/invalid/unsupported-method)make lint)