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") +}