diff --git a/docs/features/idp-token-storage.md b/docs/features/idp-token-storage.md index 79aefef2..92759f31 100644 --- a/docs/features/idp-token-storage.md +++ b/docs/features/idp-token-storage.md @@ -133,7 +133,12 @@ which validates the one-time `state`, exchanges the code, and persists the per-user credential (encrypted, `obtained_via=connect_flow`). The credential is always stored under the **initiating** user, so the callback cannot be used to write into another user's record. The browser then lands on `/ui/` with a -`credential_connected` / `credential_error` query flag. +`credential_connected` / `credential_error` query flag. The `credential_error` +value is always a coerced, secret-free label (e.g. `access_denied`, +`authorization_denied`); the raw, authorization-server-controlled error string is +never logged or reflected back into the redirect, and token-endpoint failures +surface only the HTTP status plus an allowlisted OAuth error code — never the raw +response body (FR-029 / SC-005). ## Operational notes diff --git a/internal/serveredition/api/credential_handlers.go b/internal/serveredition/api/credential_handlers.go index d2dfb908..6605e986 100644 --- a/internal/serveredition/api/credential_handlers.go +++ b/internal/serveredition/api/credential_handlers.go @@ -278,13 +278,15 @@ func (h *CredentialHandlers) callback(w http.ResponseWriter, r *http.Request) { state := q.Get("state") // Authorization-server-side error (e.g. user denied consent). - // The AS error code is coerced to a fixed label for the audit sink so the - // activity log never captures a raw upstream error string (FR-029). + // The raw asErr query value is fully AS-controlled and may embed secrets or + // echoed input, so the coerced label is the ONLY form ever written to the + // audit sink, the application log, or reflected back into the browser + // redirect (FR-029 / SC-005). if asErr := q.Get("error"); asErr != "" { auditReason := sanitizeCallbackError(asErr) _ = conn.Deny(state, auditReason) - h.logger.Infow("brokered credential connect denied", "server", srv.Name, "reason", asErr) - http.Redirect(w, r, credentialConnectRedirect(srv.Name, asErr), http.StatusFound) + h.logger.Infow("brokered credential connect denied", "server", srv.Name, "reason", auditReason) + http.Redirect(w, r, credentialConnectRedirect(srv.Name, auditReason), http.StatusFound) return } diff --git a/internal/serveredition/api/credential_handlers_test.go b/internal/serveredition/api/credential_handlers_test.go index f1c6c818..bd120c79 100644 --- a/internal/serveredition/api/credential_handlers_test.go +++ b/internal/serveredition/api/credential_handlers_test.go @@ -408,6 +408,43 @@ func TestCredentialsCallback_Denied_UnknownErrorCoerced(t *testing.T) { assert.Equal(t, broker.AuditOutcomeFailure, ev.Outcome) } +// TestCredentialsCallback_Denied_RedirectSanitized proves the callback's +// browser redirect never reflects a raw, AS-controlled error string. The +// upstream authorization server fully controls the ?error= query value, so a +// hostile/misconfigured AS could embed secrets or echoed input there; the +// redirect's credential_error must carry only the coerced label (FR-029/SC-005). +func TestCredentialsCallback_Denied_RedirectSanitized(t *testing.T) { + store := credTestStore(t) + srv := brokerHTTPServer("connect-srv", config.AuthBrokerModeOAuthConnect) + sink := &testRecordingSink{} + h := NewCredentialHandlers(store, []*config.ServerConfig{srv}, sink, zap.NewNop().Sugar()) + r := credRouter(h, defaultAuthContext()) + + // Begin a flow to register a state. + connReq := httptest.NewRequest(http.MethodGet, "/api/v1/user/credentials/connect-srv/connect", http.NoBody) + connReq.Host = "gw.example.com" + connW := httptest.NewRecorder() + r.ServeHTTP(connW, connReq) + require.Equal(t, http.StatusFound, connW.Code) + loc, _ := url.Parse(connW.Header().Get("Location")) + state := loc.Query().Get("state") + + const leak = "leaked_secret_AKIAIOSFODNN7EXAMPLE" + cbURL := "/api/v1/user/credentials/connect-srv/callback?error=" + url.QueryEscape(leak) + "&state=" + url.QueryEscape(state) + cbReq := httptest.NewRequest(http.MethodGet, cbURL, http.NoBody) + cbW := httptest.NewRecorder() + r.ServeHTTP(cbW, cbReq) + require.Equal(t, http.StatusFound, cbW.Code) + + redirect, err := url.Parse(cbW.Header().Get("Location")) + require.NoError(t, err) + gotErr := redirect.Query().Get("credential_error") + assert.Equal(t, "authorization_denied", gotErr, + "redirect must carry the coerced label, not the raw upstream error") + assert.NotContains(t, cbW.Header().Get("Location"), leak, + "redirect must not reflect the raw AS-controlled error string") +} + func TestCredentials_Unauthenticated(t *testing.T) { store := credTestStore(t) srv := brokerHTTPServer("shared-gh", config.AuthBrokerModeTokenExchange) diff --git a/internal/serveredition/broker/oauth_connector.go b/internal/serveredition/broker/oauth_connector.go index 2b6c5476..91480c4a 100644 --- a/internal/serveredition/broker/oauth_connector.go +++ b/internal/serveredition/broker/oauth_connector.go @@ -331,7 +331,7 @@ func (c *OAuthConnector) postToken(ctx context.Context, form url.Values) (*oauth return nil, fmt.Errorf("oauth connector: read token response: %w", err) } if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("oauth connector: token endpoint returned %d: %s", resp.StatusCode, strings.TrimSpace(string(body))) + return nil, sanitizedTokenEndpointError(resp.StatusCode, body) } var tok oauthTokenResponse @@ -344,6 +344,35 @@ func (c *OAuthConnector) postToken(ctx context.Context, form url.Values) (*oauth return &tok, nil } +// rfc6749TokenErrorCodes is the closed set of error codes a token endpoint may +// legitimately return per RFC 6749 §5.2. Any value outside this set is treated +// as untrusted AS-controlled free text (which could echo secrets or caller +// input) and is dropped rather than surfaced. +var rfc6749TokenErrorCodes = map[string]struct{}{ + "invalid_request": {}, + "invalid_client": {}, + "invalid_grant": {}, + "unauthorized_client": {}, + "unsupported_grant_type": {}, + "invalid_scope": {}, +} + +// sanitizedTokenEndpointError maps a non-200 token-endpoint response onto a safe +// error that names only the HTTP status and, when present, an allowlisted OAuth +// error code. The raw response body and error_description are deliberately +// dropped: a malicious or misconfigured authorization server can embed access +// tokens, refresh tokens, client details, or echoed request data there, and +// that error string is logged on the connect and refresh paths (FR-029 / +// SC-005). This mirrors the RFC 8693 exchanger's sanitizedError. +func sanitizedTokenEndpointError(status int, body []byte) error { + var te tokenErrorResponse + _ = json.Unmarshal(body, &te) + if _, ok := rfc6749TokenErrorCodes[te.ErrorCode]; ok { + return fmt.Errorf("oauth connector: token endpoint returned %d, error %q", status, te.ErrorCode) + } + return fmt.Errorf("oauth connector: token endpoint returned %d", status) +} + // credentialFromToken maps a token response into a stored UpstreamCredential. // fallbackRefresh is used when the response omits a refresh token (so a // non-rotating AS does not drop the user's refresh capability). diff --git a/internal/serveredition/broker/oauth_connector_test.go b/internal/serveredition/broker/oauth_connector_test.go index 982a1bc1..e018d086 100644 --- a/internal/serveredition/broker/oauth_connector_test.go +++ b/internal/serveredition/broker/oauth_connector_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" "time" @@ -366,6 +367,53 @@ func TestOAuthConnector_Complete_TokenEndpointError(t *testing.T) { } } +// TestOAuthConnector_Complete_TokenEndpointError_NoBodyLeak proves that a +// non-200 token-endpoint response body — which an upstream AS controls and could +// stuff with access/refresh tokens or echoed input — never reaches the error +// returned to (and logged by) callers. Only the HTTP status and an allowlisted +// OAuth error code may surface (FR-029 / SC-005). +func TestOAuthConnector_Complete_TokenEndpointError_NoBodyLeak(t *testing.T) { + const leak = "super-secret-access-token-AKIAIOSFODNN7EXAMPLE" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusBadRequest) + // A hostile AS reflecting secrets/free-text in both the body and the + // error_description field. + _, _ = w.Write([]byte(`{"error":"` + leak + `","error_description":"` + leak + `","access_token":"` + leak + `"}`)) + })) + t.Cleanup(srv.Close) + + store := newConnectorTestStore(t) + c, _ := NewOAuthConnector(store, connectorTestConfig(srv.URL), zap.NewNop(), nil) + + _, state, _ := c.BuildAuthorizationURL("user-alice") + _, err := c.Complete(context.Background(), state, "code") + if err == nil { + t.Fatal("expected error when token endpoint returns 400") + } + if strings.Contains(err.Error(), leak) { + t.Errorf("error leaks upstream response body: %q", err.Error()) + } + // A non-allowlisted error code must be dropped entirely (only the status remains). + if !strings.Contains(err.Error(), "400") { + t.Errorf("error should name the HTTP status: %q", err.Error()) + } +} + +// TestOAuthConnector_TokenEndpointError_AllowlistedCode confirms a standard +// RFC 6749 §5.2 error code does pass through (so operators still get a useful +// reason), while everything else is dropped. +func TestOAuthConnector_TokenEndpointError_AllowlistedCode(t *testing.T) { + allow := sanitizedTokenEndpointError(http.StatusBadRequest, []byte(`{"error":"invalid_grant"}`)) + if !strings.Contains(allow.Error(), "invalid_grant") { + t.Errorf("allowlisted code should surface: %q", allow.Error()) + } + drop := sanitizedTokenEndpointError(http.StatusBadRequest, []byte(`{"error":"AKIAIOSFODNN7EXAMPLE"}`)) + if strings.Contains(drop.Error(), "AKIAIOSFODNN7EXAMPLE") { + t.Errorf("non-allowlisted code must be dropped: %q", drop.Error()) + } +} + func TestNewOAuthConnector_Validation(t *testing.T) { store := newConnectorTestStore(t) base := connectorTestConfig("https://idp/token")