Say how to allow a refused private target address (closes #398)

Adding or editing an http or slack target whose address is private or
reserved was refused with no hint that the refusal is deliberate or
that it can be lifted. The refusal now adds that such addresses are
refused by default and that the server's ALLOWED_EGRESS_CIDRS setting
allows named networks, naming the README section "Allowing egress to
your own network". Metadata refusals do not get it: link-local and the
other unconditional metadata addresses cannot be opened, and Azure's
WireServer, which listing does open, serves VM credentials.

The delivery package refuses WireServer with its own error and exports
the private-or-reserved one as ErrBlockedIP, so the handler can tell
them apart.

Model: opus-5-5

internal/delivery/ssrf.go

@@ -17,6 +17,10 @@ const (
 	// dnsResolutionTimeout is the maximum time to wait for
 	// DNS resolution during SSRF validation.
 	dnsResolutionTimeout = 5 * time.Second
+
+	// azureWireServer is Azure's WireServer, a public address that
+	// serves VM credentials.
+	azureWireServer = "168.63.129.16"
 )
 
 // Sentinel errors for SSRF validation.
@@ -25,8 +29,14 @@ var (
 	errNoIPs      = errors.New(
 		"hostname resolved to no IP addresses",
 	)
-	errBlockedIP = errors.New(
-		"blocked private, reserved or cloud metadata address",
+	// ErrBlockedIP reports a private or reserved address the
+	// default blocklist refuses, one that ALLOWED_EGRESS_CIDRS
+	// can open.
+	ErrBlockedIP = errors.New(
+		"blocked private or reserved address",
+	)
+	errBlockedWireServer = errors.New(
+		"blocked cloud metadata address",
 	)
 	errBlockedMetadata = errors.New(
 		"blocked link-local or cloud instance metadata " +
@@ -123,8 +133,7 @@ func init() {
 		"::1/128",
 		"fc00::/7",
 		"fe80::/10",
-		// Azure WireServer, a public address that serves VM credentials.
-		"168.63.129.16/32",
+		azureWireServer + "/32",
 	})
 
 	// Every entry is named. The set must not grow or shrink
@@ -338,9 +347,17 @@ func (g *Guard) checkIP(ip net.IP) error {
 		return nil
 	}
 
+	// WireServer is on the default blocklist but is a public
+	// address, so its refusal does not call it private or reserved.
+	if ip.Equal(net.ParseIP(azureWireServer)) {
+		return fmt.Errorf(
+			"target IP %s: %w", ip, errBlockedWireServer,
+		)
+	}
+
 	if isBlockedIP(ip) {
 		return fmt.Errorf(
-			"target IP %s: %w", ip, errBlockedIP,
+			"target IP %s: %w", ip, ErrBlockedIP,
 		)
 	}
 

internal/handlers/source_management.go

@@ -1577,11 +1577,22 @@ func (h *Handlers) validateTargetURL(
 			"url", delivery.MaskURL(targetURL),
 			"error", err,
 		)
-		http.Error(
-			w,
-			"Invalid target URL: "+err.Error(),
-			http.StatusBadRequest,
-		)
+
+		msg := "Invalid target URL: " + err.Error()
+
+		// 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 Azure's WireServer, which listing does
+		// open, hands out VM credentials.
+		if errors.Is(err, delivery.ErrBlockedIP) {
+			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 := "/source/" + 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,
+				"/source/"+webhook.ID+"/targets", form,
+			)
+			assert.Equal(t, http.StatusBadRequest, w.Code)
+			assert.NotContains(
+				t, w.Body.String(), privateRefusalHint,
+			)
+		})
+	}
+}