internal/delivery/export_test.go

@@ -40,11 +40,6 @@ const (
 	ExportPendingSweepMinAge = pendingSweepMinAge
 )
 
-// ExportIsBlockedIP exposes isBlockedIP for testing.
-func ExportIsBlockedIP(ip net.IP) bool {
-	return isBlockedIP(ip)
-}
-
 // NewTestGuard builds an SSRF Guard from an explicit egress
 // allowlist, without going through config. Passing no prefixes
 // yields the default guard, which blocks every private/reserved
@@ -70,6 +65,11 @@ func ExportBlockedNetworks() []*net.IPNet {
 	return blockedNetworks
 }
 
+// ExportBlockedPublicNetworks exposes blockedPublicNetworks.
+func ExportBlockedPublicNetworks() []*net.IPNet {
+	return blockedPublicNetworks
+}
+
 // ExportIsForwardableHeader exposes isForwardableHeader.
 func ExportIsForwardableHeader(name string) bool {
 	return isForwardableHeader(name)

internal/delivery/ssrf.go

@@ -25,8 +25,16 @@ var (
 	errNoIPs      = errors.New(
 		"hostname resolved to no IP addresses",
 	)
-	errBlockedIP = errors.New(
+	// ErrBlockedPrivateOrReservedIP reports an address in the
-		"blocked private, reserved or cloud metadata address",
+	// default blocklist's private and reserved ranges,
+	// blockedNetworks.
+	ErrBlockedPrivateOrReservedIP = errors.New(
+		"blocked private or reserved address",
+	)
+	// errBlockedPublicMetadata reports a public address on the
+	// default blocklist, one in blockedPublicNetworks.
+	errBlockedPublicMetadata = errors.New(
+		"blocked cloud metadata address",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -37,22 +45,32 @@ var (
 	)
 )
 
-// blockedNetworks is the default blocklist: the private and
+// blockedNetworks and blockedPublicNetworks together are the
-// reserved IP ranges, plus the public cloud metadata addresses,
+// default blocklist: the private and reserved IP ranges, plus
-// that are blocked to prevent SSRF attacks. An operator can
+// the public cloud metadata addresses, that are blocked to
-// permit specific blocks out of this set with
+// prevent SSRF attacks. An operator can permit specific blocks
-// ALLOWED_EGRESS_CIDRS; see Guard.
+// out of this set with ALLOWED_EGRESS_CIDRS; see Guard.
 //
-// A public address belongs on the default blocklist only if it
+// blockedNetworks holds the private and reserved IP ranges.
-// hands credentials, user data or bootstrap material to whatever
-// can reach it, without the caller presenting anything. A
-// provider's other public addresses are not refused, since
-// reaching them can be legitimate and no list of them could be
-// complete.
 //
 //nolint:gochecknoglobals // package-level network list is appropriate here
 var blockedNetworks []*net.IPNet
 
+// blockedPublicNetworks holds the default blocklist's public
+// addresses, kept apart from blockedNetworks so that they are
+// refused as cloud metadata addresses, never as private or
+// reserved ones.
+//
+// A public address belongs on the default blocklist only if it
+// hands credentials, user data or bootstrap material to whatever
+// can reach it, without the caller presenting anything; it goes
+// in this list. A provider's other public addresses are not
+// refused, since reaching them can be legitimate and no list of
+// them could be complete.
+//
+//nolint:gochecknoglobals // package-level network list is appropriate here
+var blockedPublicNetworks []*net.IPNet
+
 // alwaysBlockedNetworks are the ranges no configuration can
 // open: the link-local blocks and the cloud instance metadata
 // endpoints that live outside them. Reaching one is credential
@@ -88,8 +106,8 @@ var blockedNetworks []*net.IPNet
 // when it clears both halves. Nothing in this list can be
 // reopened, so putting a public address here leaves the operator
 // no escape hatch at all — the condition ALLOWED_EGRESS_CIDRS
-// exists to remove. Default-block it in blockedNetworks instead,
+// exists to remove. Default-block it in blockedPublicNetworks
-// which an allowlist can override.
+// instead, which an allowlist can override.
 //
 // This is a criterion, not an enumeration of every metadata
 // address in existence.
@@ -130,6 +148,9 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
+	})
+
+	blockedPublicNetworks = mustParseCIDRs([]string{
 		// Azure WireServer, a public address that serves VM credentials.
 		"168.63.129.16/32",
 	})
@@ -225,13 +246,6 @@ func matchesAny(networks []*net.IPNet, ip net.IP) bool {
 	return false
 }
 
-// isBlockedIP checks whether an IP address falls within
-// the default blocklist, before any operator allowlist is
-// considered.
-func isBlockedIP(ip net.IP) bool {
-	return matchesAny(blockedNetworks, ip)
-}
-
 // Guard makes every SSRF decision in the process.
 //
 // It holds the operator's ALLOWED_EGRESS_CIDRS allowlist and
@@ -332,7 +346,8 @@ func (g *Guard) allows(ip net.IP) bool {
 //     consulted, so no configured CIDR reaches link-local or a
 //     cloud metadata endpoint at a non-public address.
 //  2. The allowlist is consulted next, so a listed private
-//     network becomes reachable.
+//     network, or a listed public address on the default
+//     blocklist, becomes reachable.
 //  3. Everything else keeps the default blocklist's answer.
 func (g *Guard) checkIP(ip net.IP) error {
 	if matchesAny(alwaysBlockedNetworks, ip) {
@@ -345,9 +360,15 @@ func (g *Guard) checkIP(ip net.IP) error {
 		return nil
 	}
 
-	if isBlockedIP(ip) {
+	if matchesAny(blockedNetworks, ip) {
 		return fmt.Errorf(
-			"target IP %s: %w", ip, errBlockedIP,
+			"target IP %s: %w", ip, ErrBlockedPrivateOrReservedIP,
+		)
+	}
+
+	if matchesAny(blockedPublicNetworks, ip) {
+		return fmt.Errorf(
+			"target IP %s: %w", ip, errBlockedPublicMetadata,
 		)
 	}
 

internal/delivery/ssrf_allowlist_test.go

@@ -23,6 +23,10 @@ const (
 	metadataIP  = "169.254.169.254"
 	metadataURL = "http://" + metadataIP + "/latest/meta-data/"
 
+	// linkLocalIPv4 is the IPv4 link-local block, which holds
+	// metadataIP.
+	linkLocalIPv4 = "169.254.0.0/16"
+
 	// loopbackHookURL is a target on this host: blocked by
 	// default, reachable only once an operator allowlists
 	// loopback.
@@ -237,7 +241,7 @@ func linkLocalRefusedCases() []metadataAlwaysRefusedCase {
 		},
 		{
 			name:   "whole link-local block",
-			allow:  "169.254.0.0/16",
+			allow:  linkLocalIPv4,
 			target: metadataURL,
 		},
 		{
@@ -412,6 +416,9 @@ func TestGuardAllowlist_AzureWireServerReopenable(t *testing.T) {
 		"WireServer must be refused by the default blocklist, "+
 			"which an allowlist can override",
 	)
+	require.NotErrorIs(t, err, delivery.ErrBlockedPrivateOrReservedIP,
+		"WireServer is public, not private or reserved",
+	)
 
 	assertDialRefused(t, defaultGuard, target)
 
@@ -496,7 +503,7 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 	want := []string{
 		// IPv4 link-local: the 169.254.169.254 metadata
 		// service on AWS, Azure and others.
-		"169.254.0.0/16",
+		linkLocalIPv4,
 		// IPv6 link-local.
 		"fe80::/10",
 		// AWS IPv6 IMDS, inside the ULA space an operator may
@@ -526,6 +533,90 @@ func TestAlwaysBlockedNetworks_PinnedSet(t *testing.T) {
 	assert.Equal(t, want, got)
 }
 
+// TestDefaultBlocklist_PinnedSet pins each list of the default
+// blocklist on its own, the private and reserved ranges in
+// blockedNetworks and the public addresses in
+// blockedPublicNetworks, so moving an entry from one list to the
+// other fails it. For the first address of each entry it then
+// checks that the default guard refuses it, and that listing the
+// entry in ALLOWED_EGRESS_CIDRS opens it unless the unconditional
+// set holds that address.
+func TestDefaultBlocklist_PinnedSet(t *testing.T) {
+	t.Parallel()
+
+	// public marks an entry of blockedPublicNetworks; every other
+	// entry belongs in blockedNetworks.
+	tests := []struct {
+		cidr       string
+		public     bool
+		reopenable bool
+	}{
+		{cidr: "127.0.0.0/8", reopenable: true},
+		{cidr: "10.0.0.0/8", reopenable: true},
+		{cidr: "172.16.0.0/12", reopenable: true},
+		{cidr: "192.168.0.0/16", reopenable: true},
+		{cidr: linkLocalIPv4, reopenable: false},
+		{cidr: "0.0.0.0/8", reopenable: true},
+		{cidr: "100.64.0.0/10", reopenable: true},
+		{cidr: "192.0.0.0/24", reopenable: true},
+		{cidr: "192.0.2.0/24", reopenable: true},
+		{cidr: "198.18.0.0/15", reopenable: true},
+		{cidr: "198.51.100.0/24", reopenable: true},
+		{cidr: "203.0.113.0/24", reopenable: true},
+		{cidr: "224.0.0.0/4", reopenable: true},
+		{cidr: "240.0.0.0/4", reopenable: true},
+		{cidr: "::1/128", reopenable: true},
+		{cidr: "fc00::/7", reopenable: true},
+		{cidr: "fe80::/10", reopenable: false},
+		{cidr: "168.63.129.16/32", public: true, reopenable: true},
+	}
+
+	wantPrivate := make([]string, 0, len(tests))
+	wantPublic := make([]string, 0, len(tests))
+
+	for _, tt := range tests {
+		if tt.public {
+			wantPublic = append(wantPublic, tt.cidr)
+		} else {
+			wantPrivate = append(wantPrivate, tt.cidr)
+		}
+	}
+
+	gotPrivate := make([]string, 0, len(tests))
+	for _, n := range delivery.ExportBlockedNetworks() {
+		gotPrivate = append(gotPrivate, n.String())
+	}
+
+	gotPublic := make([]string, 0, len(tests))
+	for _, n := range delivery.ExportBlockedPublicNetworks() {
+		gotPublic = append(gotPublic, n.String())
+	}
+
+	assert.ElementsMatch(t, wantPrivate, gotPrivate, "blockedNetworks")
+	assert.ElementsMatch(t, wantPublic, gotPublic, "blockedPublicNetworks")
+
+	for _, tt := range tests {
+		t.Run(tt.cidr, func(t *testing.T) {
+			t.Parallel()
+
+			prefix := netip.MustParsePrefix(tt.cidr)
+			ip := net.IP(prefix.Addr().AsSlice())
+
+			require.Error(t,
+				delivery.NewTestGuard().ExportCheckIP(ip),
+				"the default guard must refuse %s", ip,
+			)
+
+			err := delivery.NewTestGuard(prefix).ExportCheckIP(ip)
+			if tt.reopenable {
+				assert.NoError(t, err, "listing %s must open it", tt.cidr)
+			} else {
+				assert.Error(t, err, "listing %s must not open it", tt.cidr)
+			}
+		})
+	}
+}
+
 // requireLoopback fails the test unless rawURL's host is a
 // loopback address, so the allowlist test cannot silently stop
 // exercising a blocked range.

internal/delivery/ssrf_test.go

@@ -10,7 +10,7 @@ import (
 	"sneak.berlin/go/webhooker/internal/delivery"
 )
 
-func TestIsBlockedIP_PrivateRanges(t *testing.T) {
+func TestGuardCheckIP_PrivateRanges(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
@@ -56,12 +56,14 @@ func TestIsBlockedIP_PrivateRanges(t *testing.T) {
 				"failed to parse IP %s", tt.ip,
 			)
 
+			refused := delivery.NewTestGuard().ExportCheckIP(ip) != nil
+
 			assert.Equal(t,
 				tt.blocked,
-				delivery.ExportIsBlockedIP(ip),
+				refused,
-				"isBlockedIP(%s) = %v, want %v",
+				"default guard refuses %s = %v, want %v",
 				tt.ip,
-				delivery.ExportIsBlockedIP(ip),
+				refused,
 				tt.blocked,
 			)
 		})

internal/handlers/source_management.go

@@ -1578,11 +1578,22 @@ func (h *Handlers) validateTargetURL(
 			"url", delivery.MaskURL(targetURL),
 			"error", err,
 		)
-		http.Error(
+
-			w,
+		msg := "Invalid target URL: " + err.Error()
-			"Invalid target URL: "+err.Error(),
+
-			http.StatusBadRequest,
+		// Only a private or reserved address's refusal says how
-		)
+		// to allow it. Metadata refusals never do: link-local and
+		// the other unconditional metadata addresses cannot be
+		// opened, and the default blocklist's public addresses,
+		// which listing does open, hand out credentials.
+		if errors.Is(err, delivery.ErrBlockedPrivateOrReservedIP) {
+			msg += ". Private and reserved addresses are refused " +
+				"by default; the server's ALLOWED_EGRESS_CIDRS " +
+				"setting allows named networks (see \"Allowing " +
+				"egress to your own network\" in the README)."
+		}
+
+		http.Error(w, msg, http.StatusBadRequest)
 
 		return err
 	}

internal/handlers/target_private_refusal_test.go

@@ -0,0 +1,116 @@
+package handlers_test
+
+import (
+	"net/http"
+	"net/url"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/database"
+)
+
+// privateRefusalHint is the sentence that tells an operator a private
+// destination is refused on purpose, and how to allow one.
+const privateRefusalHint = "Private and reserved addresses are " +
+	"refused by default; the server's ALLOWED_EGRESS_CIDRS setting " +
+	"allows named networks (see \"Allowing egress to your own " +
+	"network\" in the README)."
+
+// TestTargetRefusal_PrivateDestinationSaysHowToAllowIt covers both
+// target types that take a URL, on add and on edit.
+func TestTargetRefusal_PrivateDestinationSaysHowToAllowIt(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	targetTypes := []database.TargetType{
+		database.TargetTypeHTTP,
+		database.TargetTypeSlack,
+	}
+
+	for _, targetType := range targetTypes {
+		t.Run(string(targetType), func(t *testing.T) {
+			t.Parallel()
+
+			webhook := seedWebhookWithRetention(t, env.db, 30)
+			targetsPath := "/hook/" + webhook.ID + "/targets"
+
+			form := url.Values{}
+			form.Set("name", "private")
+			form.Set("type", string(targetType))
+			form.Set("url", editBlockedURL)
+
+			added := serveTarget(
+				env, http.MethodPost, targetsPath, form,
+			)
+			assert.Equal(t, http.StatusBadRequest, added.Code)
+			assert.Contains(
+				t, added.Body.String(), privateRefusalHint,
+			)
+
+			form.Set("url", editOriginalURL)
+
+			created := serveTarget(
+				env, http.MethodPost, targetsPath, form,
+			)
+			require.Equal(
+				t, http.StatusSeeOther, created.Code,
+				created.Body.String(),
+			)
+
+			targets := targetsForWebhook(t, env.db, webhook.ID)
+			require.Len(t, targets, 1)
+
+			form.Set("url", editBlockedURL)
+
+			edited := submitTargetEdit(
+				env, webhook.ID, targets[0].ID, form,
+			)
+			assert.Equal(t, http.StatusBadRequest, edited.Code)
+			assert.Contains(
+				t, edited.Body.String(), privateRefusalHint,
+			)
+		})
+	}
+}
+
+// TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt: no
+// setting opens a link-local address, and Azure's WireServer hands out
+// VM credentials, so neither refusal points at the setting.
+func TestTargetRefusal_MetadataDestinationDoesNotSayHowToAllowIt(
+	t *testing.T,
+) {
+	t.Parallel()
+
+	env := setupSourceTest(t)
+
+	metadataURLs := map[string]string{
+		"link-local": "http://169.254.169.254/latest/meta-data/",
+		"wireserver": "http://168.63.129.16/?comp=versions",
+	}
+
+	for name, metadataURL := range metadataURLs {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			webhook := seedWebhookWithRetention(t, env.db, 30)
+
+			form := url.Values{}
+			form.Set("name", "metadata")
+			form.Set("type", string(database.TargetTypeHTTP))
+			form.Set("url", metadataURL)
+
+			w := serveTarget(
+				env, http.MethodPost,
+				"/hook/"+webhook.ID+"/targets", form,
+			)
+			assert.Equal(t, http.StatusBadRequest, w.Code)
+			assert.NotContains(
+				t, w.Body.String(), privateRefusalHint,
+			)
+		})
+	}
+}