From efda9cb28f727abb8334b48d49bc9082247d38ba Mon Sep 17 00:00:00 2001 From: Janne Sinivirta Date: Tue, 11 Aug 2026 16:12:11 +0300 Subject: [PATCH] Share one HTTP client and stop promcheck following redirects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit httpcheck and promcheck each defined HTTPClient and RealHTTPClient. The copies were identical except promcheck's never gained the CheckRedirect guard, so `preflight prometheus` followed 3xx anywhere — including to another host. Go strips Authorization and Cookie across hosts but not custom headers, and Mimir, Cortex, Thanos and Grafana Cloud carry tenancy in exactly those. Reproduced against a Prometheus that 302s elsewhere: X-Api-Key: SUPERSECRET arrived at the redirect destination X-Scope-OrgID: tenant-42 arrived too [OK] verdict came from the attacker's response body So the destination both harvested the credentials and decided the check passed. pkg/httpclient now holds the single Client interface and Real implementation with redirects off unless FollowRedirects is set, which is what httpcheck already did and promcheck did not. This is the duplication causing the bug, so removing the duplication is the fix rather than a cleanup alongside it. The RealHTTPClient tests move to pkg/httpclient, where the type now lives. --- cmd/preflight/cmd_http.go | 3 +- cmd/preflight/cmd_prometheus.go | 3 +- integration_test.go | 3 +- pkg/httpcheck/check.go | 40 ++------------- pkg/httpcheck/check_test.go | 76 ----------------------------- pkg/httpclient/client.go | 49 +++++++++++++++++++ pkg/httpclient/client_test.go | 86 +++++++++++++++++++++++++++++++++ pkg/promcheck/check.go | 32 ++---------- pkg/promcheck/check_test.go | 38 +++++++++++++++ 9 files changed, 185 insertions(+), 145 deletions(-) create mode 100644 pkg/httpclient/client.go create mode 100644 pkg/httpclient/client_test.go diff --git a/cmd/preflight/cmd_http.go b/cmd/preflight/cmd_http.go index 63e5ec2..cf3db1b 100644 --- a/cmd/preflight/cmd_http.go +++ b/cmd/preflight/cmd_http.go @@ -7,6 +7,7 @@ import ( "github.com/spf13/cobra" "github.com/vertti/preflight/pkg/httpcheck" + "github.com/vertti/preflight/pkg/httpclient" ) var ( @@ -70,7 +71,7 @@ func runHTTPCheck(_ *cobra.Command, args []string) error { Contains: httpContains, FollowRedirects: httpFollowRedirects, JSONPath: httpJSONPath, - Client: &httpcheck.RealHTTPClient{Timeout: httpTimeout, Insecure: httpInsecure, FollowRedirects: httpFollowRedirects}, + Client: &httpclient.Real{Timeout: httpTimeout, Insecure: httpInsecure, FollowRedirects: httpFollowRedirects}, } return runCheck(c) diff --git a/cmd/preflight/cmd_prometheus.go b/cmd/preflight/cmd_prometheus.go index 85bf7b3..9428b93 100644 --- a/cmd/preflight/cmd_prometheus.go +++ b/cmd/preflight/cmd_prometheus.go @@ -5,6 +5,7 @@ import ( "github.com/spf13/cobra" + "github.com/vertti/preflight/pkg/httpclient" "github.com/vertti/preflight/pkg/promcheck" ) @@ -71,7 +72,7 @@ func runPrometheusCheck(cmd *cobra.Command, args []string) error { RetryDelay: promRetryDelay, Insecure: promInsecure, Headers: headers, - Client: &promcheck.RealHTTPClient{Timeout: promTimeout, Insecure: promInsecure}, + Client: &httpclient.Real{Timeout: promTimeout, Insecure: promInsecure}, } // Only set threshold pointers if flags were explicitly provided diff --git a/integration_test.go b/integration_test.go index c5c4f4e..09472d9 100644 --- a/integration_test.go +++ b/integration_test.go @@ -18,6 +18,7 @@ import ( "github.com/vertti/preflight/pkg/gitcheck" "github.com/vertti/preflight/pkg/hashcheck" "github.com/vertti/preflight/pkg/httpcheck" + "github.com/vertti/preflight/pkg/httpclient" "github.com/vertti/preflight/pkg/jsoncheck" "github.com/vertti/preflight/pkg/resourcecheck" "github.com/vertti/preflight/pkg/syscheck" @@ -377,7 +378,7 @@ func TestIntegration_HTTP(t *testing.T) { c := httpcheck.Check{ URL: server.URL, - Client: &httpcheck.RealHTTPClient{Timeout: 5 * time.Second}, + Client: &httpclient.Real{Timeout: 5 * time.Second}, } result := c.Run() diff --git a/pkg/httpcheck/check.go b/pkg/httpcheck/check.go index 28a1cc8..f85d212 100644 --- a/pkg/httpcheck/check.go +++ b/pkg/httpcheck/check.go @@ -2,7 +2,6 @@ package httpcheck import ( "bytes" - "crypto/tls" "fmt" "io" "net/http" @@ -12,43 +11,10 @@ import ( "time" "github.com/vertti/preflight/pkg/check" + "github.com/vertti/preflight/pkg/httpclient" "github.com/vertti/preflight/pkg/jsonpath" ) -// HTTPClient abstracts HTTP requests for testability. -type HTTPClient interface { - Do(req *http.Request) (*http.Response, error) -} - -// RealHTTPClient uses the real net/http package. -type RealHTTPClient struct { - Timeout time.Duration - Insecure bool - FollowRedirects bool -} - -// Do executes an HTTP request. -func (c *RealHTTPClient) Do(req *http.Request) (*http.Response, error) { - transport := &http.Transport{} - if c.Insecure { - transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // intentional for --insecure flag - } - - client := &http.Client{ - Timeout: c.Timeout, - Transport: transport, - } - - // Disable automatic redirects unless explicitly enabled - if !c.FollowRedirects { - client.CheckRedirect = func(req *http.Request, via []*http.Request) error { - return http.ErrUseLastResponse - } - } - - return client.Do(req) -} - // FileReader abstracts file reading for testability. type FileReader interface { ReadFile(path string) ([]byte, error) @@ -77,7 +43,7 @@ type Check struct { Contains string // response body must contain this string FollowRedirects bool // follow HTTP redirects (3xx) JSONPath string // JSON path to check (format: "path=expectedValue" or just "path") - Client HTTPClient // injected for testing + Client httpclient.Client // injected for testing FileReader FileReader // injected for testing } @@ -117,7 +83,7 @@ func (c *Check) Run() check.Result { // Initialize client if not injected client := c.Client if client == nil { - client = &RealHTTPClient{Timeout: timeout, Insecure: c.Insecure, FollowRedirects: c.FollowRedirects} + client = &httpclient.Real{Timeout: timeout, Insecure: c.Insecure, FollowRedirects: c.FollowRedirects} } // Resolve request body diff --git a/pkg/httpcheck/check_test.go b/pkg/httpcheck/check_test.go index d842c16..8a970b2 100644 --- a/pkg/httpcheck/check_test.go +++ b/pkg/httpcheck/check_test.go @@ -5,7 +5,6 @@ import ( "errors" "io" "net/http" - "net/http/httptest" "os" "path/filepath" "strings" @@ -276,81 +275,6 @@ func TestHTTPCheckJSONPathRetry(t *testing.T) { }) } -func TestRealHTTPClient(t *testing.T) { - t.Run("basic request", func(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - _, _ = w.Write([]byte("OK")) - })) - defer ts.Close() - - client := &RealHTTPClient{Timeout: 5 * time.Second} - req, err := http.NewRequest(http.MethodGet, ts.URL, http.NoBody) - require.NoError(t, err) - - resp, err := client.Do(req) - require.NoError(t, err) - defer func() { _ = resp.Body.Close() }() - assert.Equal(t, 200, resp.StatusCode) - }) - - t.Run("insecure TLS", func(t *testing.T) { - ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) - defer ts.Close() - - client := &RealHTTPClient{Timeout: 5 * time.Second, Insecure: true} - req, err := http.NewRequest(http.MethodGet, ts.URL, http.NoBody) - require.NoError(t, err) - - resp, err := client.Do(req) - require.NoError(t, err) - defer func() { _ = resp.Body.Close() }() - assert.Equal(t, 200, resp.StatusCode) - }) - - t.Run("redirects disabled", func(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/redirect" { - http.Redirect(w, r, "/target", http.StatusFound) - return - } - w.WriteHeader(http.StatusOK) - })) - defer ts.Close() - - client := &RealHTTPClient{Timeout: 5 * time.Second, FollowRedirects: false} - req, err := http.NewRequest(http.MethodGet, ts.URL+"/redirect", http.NoBody) - require.NoError(t, err) - - resp, err := client.Do(req) - require.NoError(t, err) - defer func() { _ = resp.Body.Close() }() - assert.Equal(t, 302, resp.StatusCode) - }) - - t.Run("redirects enabled", func(t *testing.T) { - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == "/redirect" { - http.Redirect(w, r, "/target", http.StatusFound) - return - } - w.WriteHeader(http.StatusOK) - })) - defer ts.Close() - - client := &RealHTTPClient{Timeout: 5 * time.Second, FollowRedirects: true} - req, err := http.NewRequest(http.MethodGet, ts.URL+"/redirect", http.NoBody) - require.NoError(t, err) - - resp, err := client.Do(req) - require.NoError(t, err) - defer func() { _ = resp.Body.Close() }() - assert.Equal(t, 200, resp.StatusCode) - }) -} - func TestRealFileReader(t *testing.T) { tmpFile := filepath.Join(t.TempDir(), "testfile") content := []byte("test content") diff --git a/pkg/httpclient/client.go b/pkg/httpclient/client.go new file mode 100644 index 0000000..c695774 --- /dev/null +++ b/pkg/httpclient/client.go @@ -0,0 +1,49 @@ +// Package httpclient provides the HTTP client shared by the checks that make +// requests. It exists because httpcheck and promcheck each had their own copy: +// the copies drifted, promcheck's never gained redirect protection, and it +// forwarded custom auth headers to whatever host a 3xx named. +package httpclient + +import ( + "crypto/tls" + "net/http" + "time" +) + +// Client abstracts HTTP requests for testability. +type Client interface { + Do(req *http.Request) (*http.Response, error) +} + +// Real is a Client backed by net/http. +type Real struct { + Timeout time.Duration + Insecure bool + FollowRedirects bool +} + +// Do executes an HTTP request. +// +// Redirects are not followed unless FollowRedirects is set. Go strips +// Authorization and Cookie when a redirect crosses hosts, but not custom +// headers — and tenancy headers like X-Scope-OrgID are custom. Following a +// redirect would also let the destination decide the check's verdict. +func (c *Real) Do(req *http.Request) (*http.Response, error) { + transport := &http.Transport{} + if c.Insecure { + transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // intentional for --insecure flag + } + + client := &http.Client{ + Timeout: c.Timeout, + Transport: transport, + } + + if !c.FollowRedirects { + client.CheckRedirect = func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + } + } + + return client.Do(req) +} diff --git a/pkg/httpclient/client_test.go b/pkg/httpclient/client_test.go new file mode 100644 index 0000000..fb653ce --- /dev/null +++ b/pkg/httpclient/client_test.go @@ -0,0 +1,86 @@ +package httpclient + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestReal(t *testing.T) { + t.Run("basic request", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("OK")) + })) + defer ts.Close() + + client := &Real{Timeout: 5 * time.Second} + req, err := http.NewRequest(http.MethodGet, ts.URL, http.NoBody) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, 200, resp.StatusCode) + }) + + t.Run("insecure TLS", func(t *testing.T) { + ts := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + client := &Real{Timeout: 5 * time.Second, Insecure: true} + req, err := http.NewRequest(http.MethodGet, ts.URL, http.NoBody) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, 200, resp.StatusCode) + }) + + t.Run("redirects disabled", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/redirect" { + http.Redirect(w, r, "/target", http.StatusFound) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + client := &Real{Timeout: 5 * time.Second, FollowRedirects: false} + req, err := http.NewRequest(http.MethodGet, ts.URL+"/redirect", http.NoBody) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, 302, resp.StatusCode) + }) + + t.Run("redirects enabled", func(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/redirect" { + http.Redirect(w, r, "/target", http.StatusFound) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + + client := &Real{Timeout: 5 * time.Second, FollowRedirects: true} + req, err := http.NewRequest(http.MethodGet, ts.URL+"/redirect", http.NoBody) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + assert.Equal(t, 200, resp.StatusCode) + }) +} diff --git a/pkg/promcheck/check.go b/pkg/promcheck/check.go index 2b61bb0..f7f2eda 100644 --- a/pkg/promcheck/check.go +++ b/pkg/promcheck/check.go @@ -1,7 +1,6 @@ package promcheck import ( - "crypto/tls" "errors" "fmt" "io" @@ -12,35 +11,10 @@ import ( "time" "github.com/vertti/preflight/pkg/check" + "github.com/vertti/preflight/pkg/httpclient" "github.com/vertti/preflight/pkg/jsonpath" ) -// HTTPClient abstracts HTTP requests for testability. -type HTTPClient interface { - Do(req *http.Request) (*http.Response, error) -} - -// RealHTTPClient uses the real net/http package. -type RealHTTPClient struct { - Timeout time.Duration - Insecure bool -} - -// Do executes an HTTP request. -func (c *RealHTTPClient) Do(req *http.Request) (*http.Response, error) { - transport := &http.Transport{} - if c.Insecure { - transport.TLSClientConfig = &tls.Config{InsecureSkipVerify: true} //nolint:gosec // intentional for --insecure flag - } - - client := &http.Client{ - Timeout: c.Timeout, - Transport: transport, - } - - return client.Do(req) -} - // Check queries Prometheus and validates metric values. type Check struct { URL string // Prometheus server URL (required) @@ -53,7 +27,7 @@ type Check struct { RetryDelay time.Duration // delay between retries (default: 1s) Insecure bool // skip TLS verification Headers map[string]string // custom headers (for auth) - Client HTTPClient // injected for testing + Client httpclient.Client // injected for testing } // Run executes the Prometheus query check. @@ -89,7 +63,7 @@ func (c *Check) Run() check.Result { // Initialize client if not injected client := c.Client if client == nil { - client = &RealHTTPClient{Timeout: timeout, Insecure: c.Insecure} + client = &httpclient.Real{Timeout: timeout, Insecure: c.Insecure} } // Build query URL (trim trailing slash to avoid double slash) diff --git a/pkg/promcheck/check_test.go b/pkg/promcheck/check_test.go index f4c5def..77f68a4 100644 --- a/pkg/promcheck/check_test.go +++ b/pkg/promcheck/check_test.go @@ -3,11 +3,14 @@ package promcheck import ( "errors" "net/http" + "net/http/httptest" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/vertti/preflight/pkg/check" + "github.com/vertti/preflight/pkg/httpclient" "github.com/vertti/preflight/pkg/testutil" ) @@ -151,3 +154,38 @@ func TestPrometheusCheckRetry(t *testing.T) { } }) } + +// A Prometheus endpoint that redirects must not carry custom headers to the +// new host. Go strips Authorization across domains but not custom headers, and +// Mimir/Cortex/Thanos/Grafana Cloud carry tenancy in exactly those. Following +// the redirect would also let the destination decide the pass/fail verdict. +func TestRealHTTPClient_DoesNotFollowRedirects(t *testing.T) { + var leaked http.Header + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + leaked = r.Header.Clone() + _, _ = w.Write([]byte(promSuccessVector)) + })) + defer attacker.Close() + + prometheus := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + //nolint:gosec // G710: redirecting to another host is the attack being simulated + http.Redirect(w, r, attacker.URL+r.URL.Path, http.StatusFound) + })) + defer prometheus.Close() + + c := Check{ + URL: prometheus.URL, + Query: "up", + Exact: testutil.Ptr(1.0), + Timeout: 2 * time.Second, + Headers: map[string]string{ + "X-Scope-OrgID": "tenant-42", + "X-Api-Key": "SUPERSECRET", + }, + Client: &httpclient.Real{Timeout: 2 * time.Second}, + } + result := c.Run() + + assert.Nil(t, leaked, "credentials must not reach the redirect destination") + assert.Equal(t, check.StatusFail, result.Status, "a 302 is not a successful query") +}