diff --git a/control-plane/internal/handlers/coverage_handlers_92_target_test.go b/control-plane/internal/handlers/coverage_handlers_92_target_test.go index ac366881..1bb461c2 100644 --- a/control-plane/internal/handlers/coverage_handlers_92_target_test.go +++ b/control-plane/internal/handlers/coverage_handlers_92_target_test.go @@ -207,7 +207,11 @@ func TestAgentConcurrencyLimiter_MaxPerAgentNilAndConfigured(t *testing.T) { } func TestExecutionCleanupService_StartStopBranches(t *testing.T) { - t.Parallel() + // NOT t.Parallel(): this test swaps the process-global logger.Logger via + // setupExecutionCleanupTestLogger. Running it in parallel let a sibling test + // reassign the global logger mid-run, contaminating log buffers and flaking + // the coverage CI job. Keep it in the serial phase, like the other + // logger-swapping tests in execution_cleanup_test.go. t.Run("disabled start is no-op", func(t *testing.T) { service := NewExecutionCleanupService(&cleanupStoreMock{}, config.ExecutionCleanupConfig{Enabled: false}) @@ -341,7 +345,10 @@ func TestIsLightweightRequestQueryVariants(t *testing.T) { } func TestDiscoveryLoggingIncludesOptionalRequestID(t *testing.T) { - t.Parallel() + // NOT t.Parallel(): this test swaps the process-global logger.Logger via + // setupExecutionCleanupTestLogger and then asserts on the captured buffer. + // In parallel, a sibling test reassigning the global logger emptied this + // buffer and flaked the coverage CI job. Keep it in the serial phase. gin.SetMode(gin.TestMode) logBuffer := setupExecutionCleanupTestLogger(t) diff --git a/sdk/go/client/client.go b/sdk/go/client/client.go index 3860e33e..d7a23585 100644 --- a/sdk/go/client/client.go +++ b/sdk/go/client/client.go @@ -28,10 +28,16 @@ type Client struct { // Option mutates Client configuration. type Option func(*Client) -// WithHTTPClient allows custom HTTP transport configuration. +// WithHTTPClient allows custom HTTP transport configuration. If the supplied +// client does not define its own redirect policy, the credential-stripping +// policy (stripSensitiveHeadersOnRedirect) is applied so a custom client does +// not silently reintroduce the cross-host credential leak (GHSA-jp8j-g39q-qxwx). func WithHTTPClient(h *http.Client) Option { return func(c *Client) { if h != nil { + if h.CheckRedirect == nil { + h.CheckRedirect = stripSensitiveHeadersOnRedirect + } c.httpClient = h } } @@ -65,6 +71,42 @@ func WithDIDAuth(did, privateKeyJWK string) Option { } } +// sensitiveCrossHostHeaders are every credential-bearing request header this +// client attaches in do(): the static control-plane credentials plus the +// per-request DID signature headers. None of them may be replayed to a host +// other than the one the operator configured. Go's net/http already strips +// Authorization, Cookie, Cookie2 and WWW-Authenticate on a cross-host redirect, +// but it does NOT strip arbitrary application headers such as X-API-Key or our +// DID headers, so we strip the full set explicitly. The DID entries reference +// the did_auth constants so this list cannot drift if a header is renamed. +// See GHSA-jp8j-g39q-qxwx. +var sensitiveCrossHostHeaders = []string{ + "Authorization", + "X-API-Key", + HeaderCallerDID, + HeaderDIDSignature, + HeaderDIDTimestamp, + HeaderDIDNonce, +} + +// stripSensitiveHeadersOnRedirect is an http.Client.CheckRedirect hook that +// removes credential headers when a redirect targets a host other than the +// originally configured one. The comparison is against via[0] (the first +// request) so credentials only ever reach the operator-configured host, even +// across a multi-hop redirect chain. Same-host redirects keep the headers so +// ordinary redirect following (e.g. a trailing-slash or path redirect on the +// same host) still works, while a redirect to an attacker-controlled host can +// no longer exfiltrate the operator's control-plane credentials. +func stripSensitiveHeadersOnRedirect(req *http.Request, via []*http.Request) error { + if len(via) == 0 || req.URL.Host == via[0].URL.Host { + return nil + } + for _, h := range sensitiveCrossHostHeaders { + req.Header.Del(h) + } + return nil +} + // New creates a new Client instance. func New(baseURL string, opts ...Option) (*Client, error) { if baseURL == "" { @@ -79,7 +121,8 @@ func New(baseURL string, opts ...Option) (*Client, error) { c := &Client{ baseURL: parsed, httpClient: &http.Client{ - Timeout: 10 * time.Second, + Timeout: 10 * time.Second, + CheckRedirect: stripSensitiveHeadersOnRedirect, }, } diff --git a/sdk/go/client/client_redirect_test.go b/sdk/go/client/client_redirect_test.go new file mode 100644 index 00000000..7f66a790 --- /dev/null +++ b/sdk/go/client/client_redirect_test.go @@ -0,0 +1,184 @@ +package client + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "testing" +) + +// These tests cover GHSA-jp8j-g39q-qxwx: the SDK control-plane client must not +// replay any credential header it attaches (the static X-API-Key / Authorization +// and the per-request DID signature headers) to a host other than the configured +// baseURL when following a redirect. +// +// Validation contract: +// - cross-host redirect -> EVERY credential header MUST NOT reach the new host +// - same-host redirect -> credential headers MUST still be sent (no regression) +// - no redirect -> credential header MUST be sent (sanity) +// - non-credential headers (e.g. Accept) MUST be preserved in all cases + +// TestStripsCredentialsOnCrossHostRedirect mirrors the reporter's PoC: the +// operator endpoint 302-redirects to a different host, which records the headers +// it receives. The two httptest servers share 127.0.0.1 but differ by port, so +// url.Host (host:port) differs and the redirect is treated as cross-host. +func TestStripsCredentialsOnCrossHostRedirect(t *testing.T) { + var reached bool + var gotKey, gotAuth string + + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + gotKey = r.Header.Get("X-API-Key") + gotAuth = r.Header.Get("Authorization") + _, _ = w.Write([]byte(`{}`)) + })) + defer attacker.Close() + + operator := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, attacker.URL+r.URL.Path, http.StatusFound) + })) + defer operator.Close() + + c, err := New(operator.URL, WithAPIKey("SECRET-AGENTFIELD-KEY"), WithBearerToken("SECRET-BEARER")) + if err != nil { + t.Fatalf("New: %v", err) + } + _, _ = c.GetNode(context.Background(), "node-1") + + if !reached { + t.Fatal("redirect target was never reached; test setup is wrong") + } + if gotKey != "" { + t.Errorf("X-API-Key leaked to cross-host redirect target: got %q, want empty", gotKey) + } + if gotAuth != "" { + t.Errorf("Authorization leaked to cross-host redirect target: got %q, want empty", gotAuth) + } +} + +// TestKeepsCredentialsOnSameHostRedirect ensures we did not break ordinary +// same-host redirect following: a path-only redirect on the same server must +// still carry the credential to the final handler. +func TestKeepsCredentialsOnSameHostRedirect(t *testing.T) { + var finalReached bool + var gotKey string + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/final" { + finalReached = true + gotKey = r.Header.Get("X-API-Key") + _, _ = w.Write([]byte(`{}`)) + return + } + http.Redirect(w, r, "/final", http.StatusFound) + })) + defer srv.Close() + + c, err := New(srv.URL, WithAPIKey("SECRET-AGENTFIELD-KEY")) + if err != nil { + t.Fatalf("New: %v", err) + } + _, _ = c.GetNode(context.Background(), "node-1") + + if !finalReached { + t.Fatal("same-host redirect target was never reached") + } + if gotKey != "SECRET-AGENTFIELD-KEY" { + t.Errorf("X-API-Key dropped on same-host redirect: got %q, want %q", gotKey, "SECRET-AGENTFIELD-KEY") + } +} + +// TestSendsCredentialsWithoutRedirect is the baseline: without any redirect the +// configured credential reaches the server. +func TestSendsCredentialsWithoutRedirect(t *testing.T) { + var gotKey string + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotKey = r.Header.Get("X-API-Key") + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + c, err := New(srv.URL, WithAPIKey("SECRET-AGENTFIELD-KEY")) + if err != nil { + t.Fatalf("New: %v", err) + } + _, _ = c.GetNode(context.Background(), "node-1") + + if gotKey != "SECRET-AGENTFIELD-KEY" { + t.Errorf("X-API-Key not sent on direct request: got %q, want %q", gotKey, "SECRET-AGENTFIELD-KEY") + } +} + +// TestStripSensitiveHeadersOnRedirectUnit deterministically exercises the +// CheckRedirect hook over the FULL set of credential headers the client +// attaches (static creds + DID signature headers), without needing a live DID +// setup or TLS. It also guards that a non-credential header is preserved. +func TestStripSensitiveHeadersOnRedirectUnit(t *testing.T) { + mustURL := func(s string) *url.URL { + u, err := url.Parse(s) + if err != nil { + t.Fatalf("parse %q: %v", s, err) + } + return u + } + credHeaders := func() http.Header { + h := http.Header{} + h.Set("Authorization", "Bearer SECRET") + h.Set("X-API-Key", "SECRET-KEY") + h.Set(HeaderCallerDID, "did:web:example.com:agents:a") + h.Set(HeaderDIDSignature, "sig") + h.Set(HeaderDIDTimestamp, "123") + h.Set(HeaderDIDNonce, "nonce") + h.Set("Accept", "application/json") // non-credential: must survive + return h + } + orig := &http.Request{URL: mustURL("https://cp.example.com/api/v1/nodes/n1")} + + t.Run("cross-host strips every credential header", func(t *testing.T) { + req := &http.Request{ + URL: mustURL("https://attacker.example.net/api/v1/nodes/n1"), + Header: credHeaders(), + } + if err := stripSensitiveHeadersOnRedirect(req, []*http.Request{orig}); err != nil { + t.Fatalf("hook returned error: %v", err) + } + for _, h := range sensitiveCrossHostHeaders { + if v := req.Header.Get(h); v != "" { + t.Errorf("credential header %q leaked cross-host: %q", h, v) + } + } + if got := req.Header.Get("Accept"); got != "application/json" { + t.Errorf("non-credential header Accept was dropped: %q", got) + } + }) + + t.Run("same-host preserves every credential header", func(t *testing.T) { + req := &http.Request{ + URL: mustURL("https://cp.example.com/v2/nodes/n1"), + Header: credHeaders(), + } + if err := stripSensitiveHeadersOnRedirect(req, []*http.Request{orig}); err != nil { + t.Fatalf("hook returned error: %v", err) + } + for _, h := range sensitiveCrossHostHeaders { + if req.Header.Get(h) == "" { + t.Errorf("credential header %q was dropped on same-host redirect", h) + } + } + }) + + t.Run("no prior request is a no-op", func(t *testing.T) { + req := &http.Request{ + URL: mustURL("https://attacker.example.net/x"), + Header: credHeaders(), + } + if err := stripSensitiveHeadersOnRedirect(req, nil); err != nil { + t.Fatalf("hook returned error: %v", err) + } + if req.Header.Get("X-API-Key") == "" { + t.Error("headers must be untouched when there is no prior request") + } + }) +}