diff --git a/internal/delivery/ssrf.go b/internal/delivery/ssrf.go index 0fa73b3..d923bdc 100644 --- a/internal/delivery/ssrf.go +++ b/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, ) } diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 9147c1f..08c4137 100644 --- a/internal/handlers/source_management.go +++ b/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 } diff --git a/internal/handlers/target_private_refusal_test.go b/internal/handlers/target_private_refusal_test.go new file mode 100644 index 0000000..2d80731 --- /dev/null +++ b/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, + ) + }) + } +}