From d0f576e478f9e4deb4bde90a4b8656fed624a324 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Fri, 15 May 2026 18:00:59 +0200 Subject: [PATCH] oauth/cimd: post-merge review nits (4 findings, all addressed) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 (high) — /authorize and /callback emitted plain text/plain errors via http.Error. Now all error paths in those two handlers route through writeOAuthTokenError, producing the RFC 6749 §5.2 JSON shape that the rest of /oauth/* already used. Error codes mapped: - missing client_id/redirect_uri/response_type → invalid_request - resolver failure → invalid_client - redirect_uri not registered → invalid_request - missing/wrong PKCE → invalid_request - resource indicator mismatch → invalid_target (RFC 8707) - PKCE-verifier generation failure → server_error (500) - pending-auth JWE encode failure → server_error (500) - upstream-AS URL resolve failure → server_error (502) - missing state/code at /callback → invalid_request - unknown/expired pending-auth at /callback → invalid_request - auth-code JWE encode failure → server_error (500) - unparseable pending.RedirectURI at /callback → server_error (502) Method-not-allowed at /authorize stays 405 but as JSON. F2 (medium) — writeOAuthTokenError now sets `WWW-Authenticate: Bearer error="...", error_description="..."` when status == 401. RFC 7235 §3.1 mandates the header on 401s; RFC 6750 §3 specifies the Bearer challenge shape. The two existing 401 callers on /oauth/token (`invalid_client` for missing CIMD client + unsupported client auth) now carry the challenge for free. F6 (nit) — pkg/config/config.go:105 said "secrets like GatingSecretKey can be injected" — the field is SigningSecret; comment updated. While in the file, also rewrote the SigningSecret desc tag (was referring to "refresh tokens, DCR client_secrets" — both gone post-#115) to accurately describe what the secret HKDF-derives keys for in v1: pending-auth state JWE + downstream auth-code JWE. F7 (nit) — docs/oauth_authorization.md listed `refresh_revokes_tracking` in the gating-mode forbidden list at three places. The field never existed under that name post-DCR-removal — validateOAuthRuntimeConfig forbids client_id / client_secret / token_url / auth_url / userinfo_url / public_auth_server_url, and that's it. Stripped from the doc. Tests added: - TestWriteOAuthTokenError gains a 401-Bearer-challenge subtest pinning the new WWW-Authenticate behaviour. - TestOAuthAuthorizeErrorsAreJSON pins F1: four error paths through /authorize and /callback verified to return application/json bodies with RFC 6749 error/error_description. No behaviour change beyond the four fixes; all existing tests stay green; `go vet ./...` clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/altinity-mcp/oauth_server.go | 39 +++++++---- cmd/altinity-mcp/oauth_server_test.go | 93 ++++++++++++++++++++++++--- docs/oauth_authorization.md | 7 +- pkg/config/config.go | 15 +++-- 4 files changed, 122 insertions(+), 32 deletions(-) diff --git a/cmd/altinity-mcp/oauth_server.go b/cmd/altinity-mcp/oauth_server.go index 06f56dd..9c8d0f6 100644 --- a/cmd/altinity-mcp/oauth_server.go +++ b/cmd/altinity-mcp/oauth_server.go @@ -113,7 +113,20 @@ type oauthIssuedCode struct { // whoever holds the verifier — i.e. the legitimate client. Trading strict // RFC 6749 §4.1.2 single-use for zero shared state across replicas. +// writeOAuthTokenError writes an RFC 6749 §5.2 JSON error response. When +// status is 401 it also sets a Bearer-scheme WWW-Authenticate per RFC 7235 +// §3.1 (which mandates the header on 401) and RFC 6750 §3 (Bearer challenge +// shape). The header value carries the OAuth error code so a client can +// parse it from either the header or the body. +// +// Used uniformly across /oauth/{authorize,callback,token} so every broker- +// mode error response shares one shape — JSON body, optional WWW-Authenticate. +// Resource-server 401s use writeOAuthError instead because they need the +// RFC 9728 `resource_metadata=` hint. func writeOAuthTokenError(w http.ResponseWriter, status int, code, description string) { + if status == http.StatusUnauthorized { + w.Header().Set("WWW-Authenticate", fmt.Sprintf(`Bearer error=%q, error_description=%q`, code, description)) + } w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) _ = json.NewEncoder(w).Encode(map[string]string{ @@ -1023,14 +1036,14 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques return } if r.Method != http.MethodGet { - http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + writeOAuthTokenError(w, http.StatusMethodNotAllowed, "invalid_request", "method not allowed") return } q := r.URL.Query() clientID := q.Get("client_id") redirectURI := q.Get("redirect_uri") if clientID == "" || redirectURI == "" || q.Get("response_type") != "code" { - http.Error(w, "Invalid authorization request", http.StatusBadRequest) + writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "missing client_id, redirect_uri, or response_type=code") return } // CIMD inbound (#115): client_id is the HTTPS URL of the MCP client's @@ -1041,15 +1054,15 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques client, err := a.resolveCIMDClient(r.Context(), clientID) if err != nil { log.Debug().Err(err).Str("client_id", truncateForLog(clientID, 80)).Msg("OAuth /authorize rejected: CIMD resolution failed") - http.Error(w, "Unknown OAuth client", http.StatusBadRequest) + writeOAuthTokenError(w, http.StatusBadRequest, "invalid_client", "unknown OAuth client") return } if !slices.Contains(client.RedirectURIs, redirectURI) { - http.Error(w, "Unknown OAuth client", http.StatusBadRequest) + writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "redirect_uri not registered for this client") return } if q.Get("code_challenge") == "" || q.Get("code_challenge_method") != "S256" { - http.Error(w, "PKCE S256 is required", http.StatusBadRequest) + writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "PKCE S256 is required") return } // RFC 8707 §2 / MCP authorization spec: clients SHOULD include `resource`. @@ -1063,7 +1076,7 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques got := strings.TrimRight(resource, "/") if got != want { log.Debug().Str("got", resource).Str("want", a.resourceBaseURL(r)).Msg("OAuth /authorize rejected: resource indicator mismatch") - http.Error(w, "Invalid resource indicator", http.StatusBadRequest) + writeOAuthTokenError(w, http.StatusBadRequest, "invalid_target", "resource indicator does not identify this MCP server") return } } @@ -1075,7 +1088,7 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques // the /token exchange in handleOAuthCallback. upstreamVerifier, err := newPKCEVerifier() if err != nil { - http.Error(w, "Failed to generate PKCE verifier", http.StatusInternalServerError) + writeOAuthTokenError(w, http.StatusInternalServerError, "server_error", "failed to generate PKCE verifier") return } @@ -1092,14 +1105,14 @@ func (a *application) handleOAuthAuthorize(w http.ResponseWriter, r *http.Reques }) if err != nil { log.Error().Err(err).Msg("Failed to encode pending-auth JWE") - http.Error(w, "Failed to initialize OAuth state", http.StatusInternalServerError) + writeOAuthTokenError(w, http.StatusInternalServerError, "server_error", "failed to initialize OAuth state") return } cfg := a.GetCurrentConfig() authURL, err := a.resolveUpstreamAuthURL() if err != nil { - http.Error(w, "Failed to resolve upstream authorization endpoint", http.StatusBadGateway) + writeOAuthTokenError(w, http.StatusBadGateway, "server_error", "failed to resolve upstream authorization endpoint") return } callbackURL := joinURLPath(a.oauthAuthorizationServerBaseURL(r), a.oauthCallbackPath()) @@ -1129,13 +1142,13 @@ func (a *application) handleOAuthCallback(w http.ResponseWriter, r *http.Request requestID := r.URL.Query().Get("state") code := r.URL.Query().Get("code") if requestID == "" || code == "" { - http.Error(w, "Missing callback parameters", http.StatusBadRequest) + writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "missing state or code on callback") return } pending, ok := a.decodePendingAuth(requestID) if !ok { - http.Error(w, "Unknown OAuth request", http.StatusBadRequest) + writeOAuthTokenError(w, http.StatusBadRequest, "invalid_request", "unknown or expired authorization request") return } @@ -1159,7 +1172,7 @@ func (a *application) handleOAuthCallback(w http.ResponseWriter, r *http.Request authCode, err := a.encodeAuthCode(issuedCode) if err != nil { log.Error().Err(err).Msg("Failed to encode auth-code JWE") - http.Error(w, "Failed to issue authorization code", http.StatusInternalServerError) + writeOAuthTokenError(w, http.StatusInternalServerError, "server_error", "failed to issue authorization code") return } @@ -1170,7 +1183,7 @@ func (a *application) handleOAuthCallback(w http.ResponseWriter, r *http.Request redirect, err := url.Parse(pending.RedirectURI) if err != nil { - http.Error(w, "Invalid redirect URI", http.StatusBadGateway) + writeOAuthTokenError(w, http.StatusBadGateway, "server_error", "pending-auth carried an unparseable redirect_uri") return } params := redirect.Query() diff --git a/cmd/altinity-mcp/oauth_server_test.go b/cmd/altinity-mcp/oauth_server_test.go index 01fe94e..ad8a622 100644 --- a/cmd/altinity-mcp/oauth_server_test.go +++ b/cmd/altinity-mcp/oauth_server_test.go @@ -597,13 +597,90 @@ func TestTtlSeconds(t *testing.T) { func TestWriteOAuthTokenError(t *testing.T) { t.Parallel() - rr := httptest.NewRecorder() - writeOAuthTokenError(rr, http.StatusBadRequest, "invalid_request", "bad thing happened") - require.Equal(t, http.StatusBadRequest, rr.Code) - require.Equal(t, "application/json", rr.Header().Get("Content-Type")) - var body map[string]string - require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) - require.Equal(t, "invalid_request", body["error"]) - require.Equal(t, "bad thing happened", body["error_description"]) + t.Run("400 has no WWW-Authenticate", func(t *testing.T) { + t.Parallel() + rr := httptest.NewRecorder() + writeOAuthTokenError(rr, http.StatusBadRequest, "invalid_request", "bad thing happened") + require.Equal(t, http.StatusBadRequest, rr.Code) + require.Equal(t, "application/json", rr.Header().Get("Content-Type")) + require.Empty(t, rr.Header().Get("WWW-Authenticate"), "non-401 responses must not advertise an auth challenge") + var body map[string]string + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + require.Equal(t, "invalid_request", body["error"]) + require.Equal(t, "bad thing happened", body["error_description"]) + }) + t.Run("401 carries Bearer challenge per RFC 7235 §3.1", func(t *testing.T) { + t.Parallel() + rr := httptest.NewRecorder() + writeOAuthTokenError(rr, http.StatusUnauthorized, "invalid_client", "unknown OAuth client") + require.Equal(t, http.StatusUnauthorized, rr.Code) + challenge := rr.Header().Get("WWW-Authenticate") + require.NotEmpty(t, challenge, "401 responses MUST carry WWW-Authenticate") + require.Contains(t, challenge, "Bearer ") + require.Contains(t, challenge, `error="invalid_client"`) + require.Contains(t, challenge, `error_description="unknown OAuth client"`) + }) +} + +// TestOAuthAuthorizeErrorsAreJSON pins F1 from the post-merge review: every +// 4xx/5xx error response from /oauth/authorize and /oauth/callback returns +// the RFC 6749 §5.2 JSON shape (application/json + error/error_description), +// never the bare text/plain Go default that http.Error produces. Regression +// guard for the high-severity finding. +func TestOAuthAuthorizeErrorsAreJSON(t *testing.T) { + t.Parallel() + app := &application{ + config: config.Config{Server: config.ServerConfig{OAuth: config.OAuthConfig{ + Enabled: true, + Mode: "forward", + Issuer: "https://idp.example.com", + PublicAuthServerURL: "https://mcp.example.com", + SigningSecret: "regression-f1-jsonerr-32bytes!!!!", + }}}, + } + + t.Run("/authorize missing params → JSON invalid_request", func(t *testing.T) { + t.Parallel() + rr := httptest.NewRecorder() + app.handleOAuthAuthorize(rr, httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/authorize", nil)) + require.Equal(t, http.StatusBadRequest, rr.Code) + require.Equal(t, "application/json", rr.Header().Get("Content-Type")) + var body map[string]string + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + require.Equal(t, "invalid_request", body["error"]) + }) + + t.Run("/authorize wrong method → JSON invalid_request", func(t *testing.T) { + t.Parallel() + rr := httptest.NewRecorder() + app.handleOAuthAuthorize(rr, httptest.NewRequest(http.MethodPost, "https://mcp.example.com/oauth/authorize", nil)) + require.Equal(t, http.StatusMethodNotAllowed, rr.Code) + require.Equal(t, "application/json", rr.Header().Get("Content-Type")) + var body map[string]string + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + require.Equal(t, "invalid_request", body["error"]) + }) + + t.Run("/callback missing state+code → JSON invalid_request", func(t *testing.T) { + t.Parallel() + rr := httptest.NewRecorder() + app.handleOAuthCallback(rr, httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/callback", nil)) + require.Equal(t, http.StatusBadRequest, rr.Code) + require.Equal(t, "application/json", rr.Header().Get("Content-Type")) + var body map[string]string + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + require.Equal(t, "invalid_request", body["error"]) + }) + + t.Run("/callback bogus state → JSON invalid_request", func(t *testing.T) { + t.Parallel() + rr := httptest.NewRecorder() + app.handleOAuthCallback(rr, httptest.NewRequest(http.MethodGet, "https://mcp.example.com/oauth/callback?state=bogus&code=x", nil)) + require.Equal(t, http.StatusBadRequest, rr.Code) + require.Equal(t, "application/json", rr.Header().Get("Content-Type")) + var body map[string]string + require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &body)) + require.Equal(t, "invalid_request", body["error"]) + }) } diff --git a/docs/oauth_authorization.md b/docs/oauth_authorization.md index 24bcc9f..f0d44e5 100644 --- a/docs/oauth_authorization.md +++ b/docs/oauth_authorization.md @@ -64,7 +64,7 @@ config: # signing_secret injected via MCP_OAUTH_SIGNING_SECRET env var ``` -Fields that **must not be present** under gating (startup refuses with a clear error naming the field): `client_id`, `client_secret` / `MCP_OAUTH_CLIENT_SECRET`, `token_url`, `auth_url`, `userinfo_url`, `public_auth_server_url`, `refresh_revokes_tracking`, `callback_path`. +Fields that **must not be present** under gating (startup refuses with a clear error naming the field): `client_id`, `client_secret` / `MCP_OAUTH_CLIENT_SECRET`, `token_url`, `auth_url`, `userinfo_url`, `public_auth_server_url`, `callback_path`. **Forward mode** (live example: `$MCP_DEPLOY_DIR/antalya/mcp-values.yaml`): @@ -112,7 +112,6 @@ An operator moving from the old gating (MCP-as-AS) to new gating (pure resource - `auth_url` - `userinfo_url` - `public_auth_server_url` -- `refresh_revokes_tracking` - `callback_path` The canonical diff pattern is the `otel` values change committed on `feature/dcr-via-auth0` (`$MCP_DEPLOY_DIR/otel/mcp-values.yaml`). @@ -294,7 +293,7 @@ config: - "https://mcp.example.com" # signing_secret via MCP_OAUTH_SIGNING_SECRET env var # DO NOT set: client_id, client_secret, token_url, auth_url, - # userinfo_url, public_auth_server_url, refresh_revokes_tracking + # userinfo_url, public_auth_server_url ``` ### Cluster-secret authentication (optional) @@ -478,7 +477,7 @@ server: # - "gating": pure resource server — validate AS-issued JWTs (JWKS + RFC 8707 aud + scopes). # Upstream IdP handles DCR/authorize/token. Requires issuer + audience. # Forbidden fields: client_id, client_secret, token_url, auth_url, - # userinfo_url, public_auth_server_url, refresh_revokes_tracking. + # userinfo_url, public_auth_server_url. # - "forward": MCP proxies DCR + authorize + token to upstream; relays upstream tokens. # Requires client_id, client_secret, auth_url, token_url. mode: "gating" diff --git a/pkg/config/config.go b/pkg/config/config.go index 886a25f..b3094db 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -102,7 +102,7 @@ type JWEConfig struct { // // Every flag-tagged field is settable via CLI flag (`flag:` tag) or env var // (`env:` tag). The env-var convention here is `MCP_OAUTH_` so -// secrets like GatingSecretKey can be injected from a Kubernetes Secret via +// secrets like SigningSecret can be injected from a Kubernetes Secret via // the Helm chart's env: array using valueFrom.secretKeyRef. type OAuthConfig struct { // Mode controls whether altinity-mcp forwards external OAuth bearers or gates them into local MCP tokens. @@ -218,12 +218,13 @@ type OAuthConfig struct { // RefreshTokenTTLSeconds controls how long minted refresh tokens remain valid. RefreshTokenTTLSeconds int `json:"refresh_token_ttl_seconds" yaml:"refresh_token_ttl_seconds" flag:"oauth-refresh-token-ttl-seconds" env:"MCP_OAUTH_REFRESH_TOKEN_TTL_SECONDS" desc:"Refresh token lifetime in seconds"` - // SigningSecret is the server-side symmetric secret used to HMAC-sign every - // stateless OAuth artifact this server mints: self-issued JWT access tokens - // (HS256), authorization codes, refresh tokens, and RFC 7591 dynamic-client- - // registration `client_secret`s. Required whenever OAuth is enabled, in both - // forward and gating modes. - SigningSecret string `json:"signing_secret" yaml:"signing_secret" flag:"oauth-signing-secret" env:"MCP_OAUTH_SIGNING_SECRET" desc:"Server-side HMAC secret for all stateless OAuth artifacts (JWTs, auth codes, refresh tokens, DCR client_secrets)"` + // SigningSecret is the server-side symmetric secret used to HKDF-derive + // keys for every stateless OAuth JWE this server mints: pending-auth + // state (the upstream `state` parameter) and the downstream auth-code + // returned from /oauth/callback. Required whenever OAuth broker mode is + // active (forward, or gating + broker_upstream). Per #115 v1 issues no + // downstream refresh tokens and no DCR client_secrets. + SigningSecret string `json:"signing_secret" yaml:"signing_secret" flag:"oauth-signing-secret" env:"MCP_OAUTH_SIGNING_SECRET" desc:"Server-side HKDF master secret for OAuth JWE artifacts (pending-auth state, downstream auth codes). Required whenever broker mode is active."` } func (cfg OAuthConfig) NormalizedMode() string {