harden http_post: ssrf-safe client, scheme check, redacted logs, bounded drain

Author: dgageot

pkg/hooks/builtins/export_test.go

@@ -0,0 +1,18 @@
+package builtins
+
+import (
+	"time"
+
+	"github.com/docker/docker-agent/pkg/httpclient"
+)
+
+// SetHTTPPostClientUnsafeForTest swaps the httpPost client for one that
+// bypasses SSRF dial-time protection so tests can talk to
+// httptest.NewServer (which binds to 127.0.0.1). Returns a restore
+// function. Test-only — this file is *_test.go so it never compiles
+// into release binaries.
+func SetHTTPPostClientUnsafeForTest() func() {
+	prev := httpPostClient
+	httpPostClient = httpclient.NewSafeClient(30*time.Second, true)
+	return func() { httpPostClient = prev }
+}

pkg/hooks/builtins/http_post.go

@@ -2,48 +2,68 @@ package builtins
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
 	"net/http"
+	"net/url"
 	"strings"
+	"time"
 
 	"github.com/docker/docker-agent/pkg/hooks"
+	"github.com/docker/docker-agent/pkg/httpclient"
 )
 
 // HTTPPost is the registered name of the http_post builtin.
 const HTTPPost = "http_post"
 
+// httpPostClient is the HTTP client used by httpPost. It refuses
+// connections to non-public IPs at dial time (defeating DNS rebinding
+// to loopback / RFC1918 / link-local incl. cloud metadata at
+// 169.254.169.254) and bounds redirects at 10 hops. Tests swap it for
+// an unsafe variant via export_test.go since httptest.NewServer binds
+// to 127.0.0.1.
+var httpPostClient = httpclient.NewSafeClient(30*time.Second, false)
+
 // httpPost POSTs args[1] to args[0] with Content-Type: application/json.
-// Empty URL is a no-op; network errors and non-2xx responses are
-// logged and swallowed so the dispatch verdict stays nil. The hook
-// executor already wraps ctx with [Hook.GetTimeout].
+// An empty URL is a no-op (lenient args contract). A non-http(s) or
+// otherwise unparseable URL surfaces as an error so on_error: warn
+// flags the misconfig. Network errors and non-2xx responses are
+// logged (with credentials redacted) and swallowed so a bad webhook
+// never breaks the run loop. The hook executor already wraps ctx with
+// [Hook.GetTimeout]; the client's Timeout is a backstop.
 func httpPost(ctx context.Context, _ *hooks.Input, args []string) (*hooks.Output, error) {
 	if len(args) == 0 || args[0] == "" {
 		return nil, nil
 	}
-	url := args[0]
+	target, err := url.Parse(args[0])
+	if err != nil || target.Host == "" || (target.Scheme != "http" && target.Scheme != "https") {
+		return nil, errors.New("http_post: only http(s) URLs are supported")
+	}
 	var body string
 	if len(args) >= 2 {
 		body = args[1]
 	}
+	redacted := target.Redacted()
 
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body))
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, target.String(), strings.NewReader(body))
 	if err != nil {
 		return nil, fmt.Errorf("http_post: build request: %w", err)
 	}
 	req.Header.Set("Content-Type", "application/json")
 
-	resp, err := http.DefaultClient.Do(req)
+	resp, err := httpPostClient.Do(req)
 	if err != nil {
-		slog.WarnContext(ctx, "http_post: request failed", "url", url, "error", err)
+		slog.WarnContext(ctx, "http_post: request failed", "url", redacted, "error", err)
 		return nil, nil
 	}
 	defer resp.Body.Close()
-	// Drain so the connection can be reused by keep-alive.
-	_, _ = io.Copy(io.Discard, resp.Body)
+	// Cap the drain so a malicious receiver can't pin the goroutine on
+	// an unbounded read; 64 KiB is plenty for a webhook ack.
+	_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 64<<10))
 	if resp.StatusCode >= 400 {
-		slog.WarnContext(ctx, "http_post: non-success response", "url", url, "status", resp.StatusCode)
+		slog.WarnContext(ctx, "http_post: non-success response", "url", redacted, "status", resp.StatusCode)
 	}
 	return nil, nil
 }

pkg/hooks/builtins/http_post_test.go

@@ -117,18 +117,29 @@ func TestHTTPPostSwallowsErrors(t *testing.T) {
 	}
 }
 
-// TestHTTPPostMalformedURLReturnsError: an unparseable URL is the
-// one error path that surfaces, so `on_error: warn` flags the
-// misconfig.
-func TestHTTPPostMalformedURLReturnsError(t *testing.T) {
+// TestHTTPPostRejectsNonHTTPSchemes: file://, ftp://, javascript: and
+// scheme-less or host-less inputs all surface as a config error
+// rather than being silently dispatched to a transport.
+func TestHTTPPostRejectsNonHTTPSchemes(t *testing.T) {
 	t.Parallel()
 
 	fn := lookup(t, builtins.HTTPPost)
 
-	out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{"http://\x7f\x00.example", "{}"})
-	require.Error(t, err)
-	assert.Nil(t, out)
-	assert.Contains(t, err.Error(), "http_post:")
+	cases := []string{
+		"file:///etc/passwd",
+		"ftp://example.com/",
+		"javascript:alert(1)",
+		"not-a-url",
+		"http://",
+		"http://\x7f\x00.example",
+	}
+	for _, raw := range cases {
+		out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{raw, "{}"})
+		require.Errorf(t, err, "input %q must be rejected", raw)
+		assert.Nil(t, out)
+		assert.Contains(t, err.Error(), "http_post:")
+		assert.Contains(t, err.Error(), "http(s)")
+	}
 }
 
 // TestHTTPPostHonoursContextCancellation: the request returns

pkg/hooks/builtins/main_test.go

@@ -0,0 +1,18 @@
+package builtins_test
+
+import (
+	"os"
+	"testing"
+
+	"github.com/docker/docker-agent/pkg/hooks/builtins"
+)
+
+// TestMain flips the http_post client to its unsafe variant for the
+// duration of this test binary, since httptest.NewServer binds to
+// 127.0.0.1 and is otherwise rejected by the production SSRF dialer.
+// Production callers always go through the safe client wired in
+// http_post.go.
+func TestMain(m *testing.M) {
+	builtins.SetHTTPPostClientUnsafeForTest()
+	os.Exit(m.Run())
+}