security: add SSRF defence-in-depth for webhook notifier (fixes H-4)

The webhook notifier would previously accept any operator-configured URL and hand it to http.Client without validation. That exposed two SSRF classes (CWE-918): * Reserved-address reachability — a misconfigured or adversarial webhook URL pointing at 127.0.0.1, ::1, 169.254.169.254 (cloud metadata), or 0.0.0.0 would succeed, exfiltrating request bodies to local services or leaking short-lived cloud credentials. * DNS rebinding — a hostname resolving to a public IP at validation time and to a reserved IP at dial time would bypass any URL-string-only check. Fix installs two independent layers: * validation.ValidateSafeURL runs at config-ingest time and before every outbound POST. It rejects non-HTTP(S) schemes, empty hosts, and literal reserved-IP hosts with a clear operator-facing error. This is a fast early diagnostic. * validation.SafeHTTPDialContext is installed on the webhook http.Transport. It re-resolves the host at dial time, rejects any resolved address whose address lies in a reserved range (loopback, link-local, multicast, broadcast, unspecified, IPv6 link-local/multicast), and pins the resolved IP into the final dial address so the TLS handshake targets the exact IP the guard approved. This is the authoritative, TOCTOU-safe defence against DNS rebinding. The two layers are complementary — validateURL fails fast on obvious misconfiguration; SafeHTTPDialContext fails closed when DNS changes between validation and dial. The existing unexported isReservedIP helper in internal/service/network_scan.go is extracted into internal/validation.IsReservedIP with byte-identical behaviour so the webhook notifier and the network scanner share a single authoritative reserved-address list. RFC 1918 ranges remain intentionally allowed (certctl's self-hosted design). Broader unspecified / IPv6 link-local coverage lives only in the stricter dial-time policy, where it belongs for outbound HTTP egress. Test seam: Connector gains an unexported validateURL func field and a same-package newForTest constructor that installs a permissive validator and the stdlib default transport. Production callers cannot reach this constructor because it is unexported; only same-package tests (package webhook) can use it. Same-package happy-path tests call newForTest so they can point at httptest loopback servers without being blocked by the production guard. The four SSRF-rejection tests that verify the guard itself still call New so they exercise the real, strict validator. This keeps the production SSRF defence unconditionally on in real code while preserving legitimate unit-test coverage. Tests ----- * internal/validation/ssrf_test.go (new) — 16-subtest pin on IsReservedIP that is byte-identical with the original network- scanner behaviour; ValidateSafeURL accept/reject matrix covering HTTPS/HTTP, reserved-literal IPv4/IPv6, dangerous schemes (file/gopher/ftp/javascript/data/ldap/dict/jar), missing hosts, and malformed inputs; SafeHTTPDialContext rejects literal reserved addresses and hosts resolving to reserved addresses (DNS-rebinding coverage via localhost). * internal/connector/notifier/webhook/webhook_test.go — happy-path tests switched to newForTest; production-guard SSRF-rejection tests (TestValidateConfig_RejectsReservedURLs, TestValidateConfig_RejectsDangerousScheme, TestPostWebhook_RejectsReservedURL, TestPostWebhook_RejectsDangerousScheme) continue to call New so they exercise the unconditionally-installed production validator. Wire-format invariants preserved -------------------------------- * Outbound HTTP request shape (method, headers, body, HMAC signature) unchanged. * network_scan.go behaviour unchanged — validation.IsReservedIP is byte-identical with the deleted helper. * RFC 1918 (10/8, 172.16/12, 192.168/16) remain allowed for both outbound webhook and CIDR expansion, matching the self-hosted design. Verification ------------ * go test -race ./internal/validation/... ./internal/connector/ notifier/webhook/... ./internal/service/... — green. * Full-suite go test -race ./... — green (GOTMPDIR=/dev/shm to sidestep full /tmp on the sandbox host). * Coverage gates pass: service 68.8% >= 55%, handler 83.6% >= 60%, domain 82.0% >= 40%, middleware 63.8% >= 30%. Overall 67.8%. Webhook package 91.5% line coverage; validation package ValidateSafeURL/SafeHTTPDialContext 78-100% per function. * govulncheck ./... — no vulnerabilities found. * golangci-lint run on touched H-4 production code — clean. Pre- existing errcheck/gosimple warnings in scope-adjacent files (webhook_test.go:270 w.Write, network_scan.go:120/173/265/305) verified against 3853b74 to predate this commit; left alone per scope guard. Operational notes ----------------- * No migration needed. The guard is pure Go code; existing webhook configs continue to work unless they point at reserved addresses, in which case they now fail closed with a clear error. * Existing operators who rely on webhook POST to 127.0.0.1 or ::1 (e.g., local receivers on the same host as certctl-server) must expose their receiver on an RFC 1918 address or public IP. This is deliberate — the threat model for webhook notifiers includes untrusted operator-supplied URLs. Scope guard: H-4 only. H-5, H-6, M-*, L-*, and I-* findings remain open and are tracked separately. No drive-by refactors.
2026-06-07 15:01:32 +00:00 · 2026-04-17 00:34:47 +00:00
parent 3853b7460c
commit 119986fa7e
6 changed files with 684 additions and 62 deletions
@@ -14,8 +14,15 @@ import (
 	"time"

 	"github.com/shankar0123/certctl/internal/connector/notifier"
+	"github.com/shankar0123/certctl/internal/validation"
 )

+// webhookClientTimeout bounds every outbound webhook request and its
+// resolution/dial phase. Kept as a package-level constant so the timeout is
+// shared by the transport dialer and the http.Client, and so tests can reason
+// about it without plumbing configuration.
+const webhookClientTimeout = 30 * time.Second
+
 // Config represents the webhook notifier configuration.
 type Config struct {
 	URL     string            `json:"url"`
@@ -25,20 +32,69 @@ type Config struct {

 // Connector implements the notifier.Connector interface for webhook notifications.
 // It sends alert and event notifications via HTTP POST with optional HMAC signing.
+//
+// validateURL is injected so that the production constructor (New) installs the
+// strict validation.ValidateSafeURL guard while newForTest can install a
+// permissive validator. This is the only way to keep the production SSRF
+// defence unconditionally on in real code while still allowing tests to point
+// at httptest loopback servers. Without this seam, every test using
+// httptest.NewServer would be blocked by the guard's loopback rejection — that
+// is the correct behaviour in production but makes legitimate unit tests
+// impossible to write. The test seam is unexported so no external caller can
+// use it to disable the guard.
 type Connector struct {
-	config *Config
-	logger *slog.Logger
-	client *http.Client
+	config      *Config
+	logger      *slog.Logger
+	client      *http.Client
+	validateURL func(string) error
 }

 // New creates a new webhook notifier with the given configuration and logger.
+//
+// The returned connector uses an http.Transport whose DialContext is hardened
+// by validation.SafeHTTPDialContext. That guard re-resolves the target host
+// at dial time and refuses any connection whose resolved address lies in a
+// reserved range (loopback, cloud-metadata link-local, multicast, broadcast,
+// unspecified, IPv6 link-local/multicast). This is the authoritative SSRF
+// defence; validation.ValidateSafeURL inside ValidateConfig/postWebhook is a
+// fast early diagnostic. The two layers together defeat both misconfigured
+// URLs and DNS-rebinding attacks where a name's resolved address changes
+// between validation and dial.
 func New(config *Config, logger *slog.Logger) *Connector {
+	transport := &http.Transport{
+		DialContext:           validation.SafeHTTPDialContext(webhookClientTimeout),
+		TLSHandshakeTimeout:   10 * time.Second,
+		ResponseHeaderTimeout: 10 * time.Second,
+		ExpectContinueTimeout: 1 * time.Second,
+		ForceAttemptHTTP2:     true,
+	}
 	return &Connector{
 		config: config,
 		logger: logger,
 		client: &http.Client{
-			Timeout: 30 * time.Second,
+			Timeout:   webhookClientTimeout,
+			Transport: transport,
 		},
+		validateURL: validation.ValidateSafeURL,
+	}
+}
+
+// newForTest is an unexported constructor used exclusively by the webhook
+// package's own tests. It installs a permissive URL validator and the stdlib
+// default transport so tests can point the connector at httptest loopback
+// servers (127.0.0.1), which the production SafeHTTPDialContext guard would
+// correctly reject. Production callers cannot reach this constructor because
+// it is unexported; only same-package tests (package webhook) can use it.
+// The SSRF-rejection tests that verify the guard itself still call New so
+// they exercise the real, strict validator.
+func newForTest(config *Config, logger *slog.Logger) *Connector {
+	return &Connector{
+		config: config,
+		logger: logger,
+		client: &http.Client{
+			Timeout: webhookClientTimeout,
+		},
+		validateURL: func(string) error { return nil },
 	}
 }

@@ -54,6 +110,18 @@ func (c *Connector) ValidateConfig(ctx context.Context, rawConfig json.RawMessag
 		return fmt.Errorf("webhook url is required")
 	}

+	// SSRF guard (CWE-918). Reject reserved-address URLs before issuing any
+	// outbound HTTP — this catches the obvious 127.0.0.1 / ::1 /
+	// 169.254.169.254 / 0.0.0.0 cases at config-ingestion time and produces
+	// a clear operator-facing error. The authoritative, TOCTOU-safe check
+	// still runs at dial time inside SafeHTTPDialContext. Routed through
+	// c.validateURL so newForTest can install a permissive validator for
+	// same-package unit tests; production New always wires
+	// validation.ValidateSafeURL here.
+	if err := c.validateURL(cfg.URL); err != nil {
+		return fmt.Errorf("webhook url rejected: %w", err)
+	}
+
 	c.logger.Info("validating webhook configuration", "url", cfg.URL)

 	// Test webhook connectivity with a HEAD request
@@ -150,7 +218,17 @@ func (c *Connector) SendEvent(ctx context.Context, event notifier.Event) error {
 // postWebhook sends a payload to the webhook URL with proper headers and signing.
 // If a secret is configured, it signs the payload using HMAC-SHA256 and includes
 // the signature in the X-Signature header.
+//
+// The URL is re-validated here even though ValidateConfig already accepted it:
+// configuration can be mutated in place, reloaded dynamically, or set directly
+// by tests that bypass ValidateConfig, so this call is a defence-in-depth
+// guard that fails closed before any outbound request is built. Authoritative
+// DNS-rebinding defence still runs at dial time via SafeHTTPDialContext.
 func (c *Connector) postWebhook(ctx context.Context, payload interface{}) error {
+	if err := c.validateURL(c.config.URL); err != nil {
+		return fmt.Errorf("webhook url rejected: %w", err)
+	}
+
 	// Marshal payload to JSON
 	jsonData, err := json.Marshal(payload)
 	if err != nil {
@@ -32,7 +32,7 @@ func TestWebhook_ValidateConfig_ValidURL(t *testing.T) {

 	// Create a new logger (or use test logger)
 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	err := conn.ValidateConfig(context.Background(), rawConfig)
 	if err != nil {
@@ -47,7 +47,7 @@ func TestWebhook_ValidateConfig_MissingURL(t *testing.T) {

 	rawConfig, _ := json.Marshal(cfg)
 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	err := conn.ValidateConfig(context.Background(), rawConfig)
 	if err == nil {
@@ -96,7 +96,7 @@ func TestWebhook_SendAlert_Success(t *testing.T) {
 	}

 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	alert := notifier.Alert{
 		ID:        "alert-123",
@@ -160,7 +160,7 @@ func TestWebhook_SendAlert_HMACSignature(t *testing.T) {
 	}

 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	alert := notifier.Alert{
 		ID:        "alert-456",
@@ -199,7 +199,7 @@ func TestWebhook_SendAlert_NoSignatureWithoutSecret(t *testing.T) {
 	}

 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	alert := notifier.Alert{
 		ID:        "alert-789",
@@ -239,7 +239,7 @@ func TestWebhook_SendAlert_CustomHeaders(t *testing.T) {
 	}

 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	alert := notifier.Alert{
 		ID:        "alert-custom",
@@ -276,7 +276,7 @@ func TestWebhook_SendAlert_HTTPError(t *testing.T) {
 	}

 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	alert := notifier.Alert{
 		ID:        "alert-error",
@@ -318,7 +318,7 @@ func TestWebhook_SendEvent_Success(t *testing.T) {
 	}

 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	certID := "mc-api-prod"
 	event := notifier.Event{
@@ -367,7 +367,7 @@ func TestWebhook_SendEvent_WithoutCertificateID(t *testing.T) {
 	}

 	logger := newTestLogger()
-	conn := New(cfg, logger)
+	conn := newForTest(cfg, logger)

 	event := notifier.Event{
 		ID:        "event-456",
@@ -389,6 +389,130 @@ func TestWebhook_SendEvent_WithoutCertificateID(t *testing.T) {
 	}
 }

+// The SSRF tests below exercise the CWE-918 guard added alongside H-4. Each
+// case pairs a reserved-address URL with the call surface that should reject
+// it. ValidateConfig is the early-fail path; SendAlert/SendEvent reach the
+// same guard via postWebhook and are the defence-in-depth that still rejects
+// even when ValidateConfig was bypassed (e.g. dynamic config reload mutating
+// c.config.URL in place).
+
+func TestWebhook_ValidateConfig_RejectsReservedURLs(t *testing.T) {
+	// These must all fail at config-ingestion time without ever opening a
+	// socket — the reserved-address filter is the whole point of H-4.
+	cases := []struct {
+		name string
+		url  string
+	}{
+		{"loopback v4", "http://127.0.0.1/hook"},
+		{"loopback v4 with port", "http://127.0.0.1:8080/"},
+		{"loopback v6 bracketed", "http://[::1]/hook"},
+		{"AWS metadata", "http://169.254.169.254/latest/meta-data/"},
+		{"generic link-local", "http://169.254.1.2/"},
+		{"unspecified v4", "http://0.0.0.0/"},
+		{"unspecified v6", "http://[::]/"},
+		{"IPv6 link-local", "http://[fe80::1]/"},
+		{"multicast", "https://224.0.0.5/"},
+		{"broadcast", "http://255.255.255.255/"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := &Config{URL: tc.url}
+			rawConfig, _ := json.Marshal(cfg)
+			conn := New(cfg, newTestLogger())
+
+			err := conn.ValidateConfig(context.Background(), rawConfig)
+			if err == nil {
+				t.Fatalf("ValidateConfig(%q) returned nil, want SSRF rejection", tc.url)
+			}
+			if !strings.Contains(err.Error(), "reserved") && !strings.Contains(err.Error(), "rejected") {
+				t.Errorf("expected reserved/rejected error, got %q", err.Error())
+			}
+		})
+	}
+}
+
+func TestWebhook_ValidateConfig_RejectsDangerousSchemes(t *testing.T) {
+	// Only http(s) is a legitimate webhook transport. Every other scheme is
+	// an SSRF amplifier (file, gopher, ftp, javascript, data, ldap, dict,
+	// jar) and must be refused at config time.
+	cases := []struct {
+		name string
+		url  string
+	}{
+		{"file", "file:///etc/passwd"},
+		{"gopher", "gopher://example.com/_x"},
+		{"ftp", "ftp://example.com/"},
+		{"javascript", "javascript:alert(1)"},
+		{"data", "data:text/plain;base64,SGVsbG8="},
+		{"ldap", "ldap://example.com/"},
+		{"dict", "dict://example.com:2628/d:foo"},
+		{"jar", "jar:http://example.com/foo.jar!/"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cfg := &Config{URL: tc.url}
+			rawConfig, _ := json.Marshal(cfg)
+			conn := New(cfg, newTestLogger())
+
+			err := conn.ValidateConfig(context.Background(), rawConfig)
+			if err == nil {
+				t.Fatalf("ValidateConfig(%q) returned nil, want scheme rejection", tc.url)
+			}
+			if !strings.Contains(err.Error(), "rejected") && !strings.Contains(err.Error(), "scheme") {
+				t.Errorf("expected scheme/rejected error, got %q", err.Error())
+			}
+		})
+	}
+}
+
+func TestWebhook_SendAlert_RejectsReservedURLInPostWebhook(t *testing.T) {
+	// Simulate config drift: URL was legitimate at ValidateConfig time but
+	// has since been rewritten to an SSRF target. postWebhook must catch
+	// this on every call without ever hitting the wire.
+	cfg := &Config{URL: "http://169.254.169.254/latest/meta-data/"}
+	conn := New(cfg, newTestLogger())
+
+	alert := notifier.Alert{
+		ID:        "alert-ssrf",
+		Type:      "test",
+		Severity:  "info",
+		Subject:   "Test",
+		Message:   "Test",
+		Recipient: "ops@example.com",
+		CreatedAt: time.Now(),
+	}
+
+	err := conn.SendAlert(context.Background(), alert)
+	if err == nil {
+		t.Fatal("SendAlert returned nil, want SSRF rejection from postWebhook")
+	}
+	if !strings.Contains(err.Error(), "reserved") && !strings.Contains(err.Error(), "rejected") {
+		t.Errorf("expected reserved/rejected error, got %q", err.Error())
+	}
+}
+
+func TestWebhook_SendEvent_RejectsReservedURLInPostWebhook(t *testing.T) {
+	cfg := &Config{URL: "http://[::1]:9/webhook"}
+	conn := New(cfg, newTestLogger())
+
+	event := notifier.Event{
+		ID:        "event-ssrf",
+		Type:      "test",
+		Subject:   "Test",
+		Body:      "Test",
+		Recipient: "ops@example.com",
+		CreatedAt: time.Now(),
+	}
+
+	err := conn.SendEvent(context.Background(), event)
+	if err == nil {
+		t.Fatal("SendEvent returned nil, want SSRF rejection from postWebhook")
+	}
+	if !strings.Contains(err.Error(), "reserved") && !strings.Contains(err.Error(), "rejected") {
+		t.Errorf("expected reserved/rejected error, got %q", err.Error())
+	}
+}
+
 // Helper function to compute HMAC-SHA256 signature
 func computeHMACSHA256(data []byte, secret string) string {
 	h := hmac.New(sha256.New, []byte(secret))