diff --git a/README.md b/README.md index 2bb6e23..e322a67 100644 --- a/README.md +++ b/README.md @@ -114,6 +114,62 @@ TTY detection, and security headers are always applied. | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) | +| `ALLOWED_EGRESS_CIDRS` | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) | + +#### Allowing egress to your own network + +By default every delivery target must resolve to a public address. +The private and reserved ranges — RFC 1918, loopback, CGNAT, +link-local and the rest — are refused, which stops a target from being +used to make webhooker probe the network it sits in. + +That default is also inconvenient for the thing webhooker is mostly +for: taking a public webhook and forwarding it to something on your own +network. A container on the same Docker network, a box on `10.x`, a +service on `127.0.0.1` — all refused, until you name them. + +`ALLOWED_EGRESS_CIDRS` is a comma-separated list of CIDR blocks (a bare +address such as `10.0.0.7` is accepted and treated as a single host), +for example `10.0.0.0/8, 172.17.0.0/16`. Addresses inside those blocks +become valid delivery destinations. Everything outside them keeps the +default answer, so this only ever adds destinations — it never removes +any, and it cannot narrow what was already reachable. + +**The risk, plainly.** Each block you list is a network that anyone who +can create a delivery target can now make this process issue requests +into, and read the response body back out of via the delivery log. That +is server-side request forgery, deliberately enabled and scoped by you. +A webhooker admin account is therefore as trusted as the narrowest +thing on those networks: an unauthenticated admin panel, a database +listening without a password, or an internal API that trusts its +network position is reachable through it. List the smallest blocks that +cover the destinations you actually deliver to — prefer +`10.1.2.3/32` over `10.0.0.0/8` — and never list a block wider than the +network you are willing to expose. + +Two things this setting cannot do: + +- **It cannot turn the guard off.** There is no boolean, and no value + that disables SSRF protection wholesale. The guard is always on and + the list is always an allowlist; an empty list (the default) means + every private and reserved range stays refused. +- **It cannot open link-local.** `169.254.0.0/16` and `fe80::/10` stay + blocked no matter what you list, including when you list them + outright or list a supernet such as `0.0.0.0/0`. That range serves + cloud instance metadata (`169.254.169.254`), where reaching it is + credential theft rather than delivery to an internal service. + +The list is applied at one place in the code, which both target +creation and delivery consult, so a URL that the target form accepts is +one that delivery will actually attempt — the two cannot disagree. +Delivery re-resolves and re-checks the destination at dial time, so a +hostname that resolves to an allowed address during validation and a +different one later (DNS rebinding) is still refused unless the new +address is also allowed. + +A set but unparseable value aborts startup. When the list is non-empty +webhooker logs it at startup, blocks and all, so the hole is visible in +the log of any deployment that has one. #### Trusted proxies @@ -226,8 +282,9 @@ additionally be a number in the range 1–65535, `RECEIVER_RATE_LIMIT` must be at least 1, `RETENTION_SWEEP_INTERVAL` must be greater than zero (it is a ticker period, so `0s` or a negative value would crash the reaper after -startup), and every entry in `TRUSTED_PROXIES` must be a CIDR block or -a bare IP address. `SESSION_IDLE_TIMEOUT` is the exception: a +startup), and every entry in `TRUSTED_PROXIES` and +`ALLOWED_EGRESS_CIDRS` must be a CIDR block or a bare IP address. +`SESSION_IDLE_TIMEOUT` is the exception: a non-positive value there means idle expiry is disabled, not invalid. Boolean variables (`DEBUG`, `MAINTENANCE_MODE`) accept exactly the @@ -1920,7 +1977,12 @@ check, see [The login endpoint](#the-login-endpoint). ranges (RFC 1918, loopback, link-local, cloud metadata) are blocked both at target creation time (URL validation) and at delivery time (custom HTTP transport with SSRF-safe dialer that validates resolved - IPs before connecting, preventing DNS rebinding attacks) + IPs before connecting, preventing DNS rebinding attacks). Both paths + route through a single decision function, so they cannot disagree + about a destination. An operator can permit specific blocks with + [`ALLOWED_EGRESS_CIDRS`](#allowing-egress-to-your-own-network); the + guard cannot be switched off, and link-local stays blocked whatever + is listed - **Login limiting is inverted, deliberately.** The login `POST` has no pre-emptive rate limiter in front of it. Credentials are verified first and only a _failed_ attempt spends budget, so a diff --git a/cmd/webhooker/main.go b/cmd/webhooker/main.go index 114d154..0b69f01 100644 --- a/cmd/webhooker/main.go +++ b/cmd/webhooker/main.go @@ -75,6 +75,10 @@ func newApp() *fx.App { session.New, handlers.New, middleware.New, + // The one SSRF guard both target-creation validation + // and the delivery dialer consult, so they cannot + // disagree about a destination. + delivery.NewGuard, delivery.New, delivery.NewArchiveSweeper, // Wire *delivery.Engine as delivery.Notifier so the diff --git a/internal/config/config.go b/internal/config/config.go index 0d5acb5..d6d678c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -114,6 +114,17 @@ type Config struct { // clients. TrustedProxies []netip.Prefix + // AllowedEgressCIDRs is the set of networks a delivery target + // may reach even though the SSRF guard's default blocklist + // covers them. It is empty unless ALLOWED_EGRESS_CIDRS is set, + // and empty means every private/reserved range stays refused. + // + // This only ever adds destinations to what the guard would + // otherwise refuse. The guard itself is always on: there is no + // setting that disables SSRF protection, and link-local stays + // blocked no matter what is listed here. + AllowedEgressCIDRs []netip.Prefix + params *ConfigParams log *slog.Logger } @@ -406,6 +417,11 @@ func loadFromEnv() (*Config, error) { return nil, err } + allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS") + if err != nil { + return nil, err + } + return &Config{ DataDir: envString("DATA_DIR"), Debug: debug, @@ -419,9 +435,48 @@ func loadFromEnv() (*Config, error) { SessionIdleTimeout: sessionIdleTimeout, ReceiverRateLimit: receiverRateLimit, TrustedProxies: trustedProxies, + AllowedEgressCIDRs: allowedEgressCIDRs, }, nil } +// PrefixStrings renders a prefix list as its CIDR strings, for +// logging a list an operator has to be able to read back. +func PrefixStrings(prefixes []netip.Prefix) []string { + out := make([]string, 0, len(prefixes)) + + for _, prefix := range prefixes { + out = append(out, prefix.String()) + } + + return out +} + +// warnEgressAllowlist logs the effective ALLOWED_EGRESS_CIDRS +// whenever it is non-empty. +// +// It prints the blocks themselves rather than a count, because +// this is the one setting that lets a delivery target reach the +// host's own network: an operator reading the startup log has to +// be able to see exactly which hole is open. Silence means the +// list is empty and the SSRF guard is refusing every +// private/reserved range, which is the default. +func (c *Config) warnEgressAllowlist(log *slog.Logger) { + if len(c.AllowedEgressCIDRs) == 0 { + return + } + + log.Warn( + "ALLOWED_EGRESS_CIDRS lets delivery targets reach these "+ + "otherwise-blocked private/reserved networks. Anyone "+ + "who can create a delivery target can now make this "+ + "process issue requests into them, and read back the "+ + "response. Link-local (cloud instance metadata) stays "+ + "blocked regardless.", + "allowedEgressCIDRs", + strings.Join(PrefixStrings(c.AllowedEgressCIDRs), ","), + ) +} + // warnSharedRateLimitBucket logs a startup warning whenever // TRUSTED_PROXIES is empty, in any environment. // @@ -511,12 +566,14 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "sessionIdleTimeout", s.SessionIdleTimeout.String(), "receiverRateLimit", s.ReceiverRateLimit, "trustedProxies", len(s.TrustedProxies), + "allowedEgressCIDRs", len(s.AllowedEgressCIDRs), "hasSentryDSN", s.SentryDSN != "", "hasMetricsAuth", s.MetricsUsername != "" && s.MetricsPassword != "", ) s.warnSharedRateLimitBucket(log) + s.warnEgressAllowlist(log) return s, nil } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d7b3bc7..861011b 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -627,6 +627,183 @@ func testTrustedProxiesSuccess( assert.Equal(t, expected, got) } +// TestAllowedEgressCIDRs covers ALLOWED_EGRESS_CIDRS, the escape +// hatch that lets a self-hosted deployment forward to its own +// network. Unset it must stay empty, so the SSRF guard keeps +// refusing every private/reserved range; a set-but-unparseable +// value must abort startup naming the variable rather than +// silently running with a list the operator did not write. +func TestAllowedEgressCIDRs(t *testing.T) { + tests := []struct { + name string + set bool + value string + expected []string + expectError bool + }{ + { + name: caseUnsetUsesDefault, + set: false, + expected: []string{}, + }, + { + name: "empty value yields empty list", + set: true, + value: "", + expected: []string{}, + }, + { + name: caseValidValueParsed, + set: true, + value: cidrPrivateV4, + expected: []string{cidrPrivateV4}, + }, + { + name: "multiple blocks with whitespace", + set: true, + value: " 10.0.0.0/8 , 127.0.0.0/8 ", + expected: []string{cidrPrivateV4, "127.0.0.0/8"}, + }, + { + name: "bare address becomes a single host", + set: true, + value: "172.17.0.5", + expected: []string{"172.17.0.5/32"}, + }, + { + name: caseUnparseableFails, + set: true, + value: cidrPrivateV4 + ",not-an-address", + expectError: true, + }, + { + name: "out-of-range prefix length fails startup", + set: true, + value: "10.0.0.0/33", + expectError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", "dev") + + if tt.set { + t.Setenv("ALLOWED_EGRESS_CIDRS", tt.value) + } else { + require.NoError( + t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"), + ) + } + + if tt.expectError { + expectStartupErrorFor( + t, "ALLOWED_EGRESS_CIDRS", config.ErrInvalidCIDR, + ) + } else { + testAllowedEgressCIDRsSuccess(t, tt.expected) + } + }) + } +} + +func testAllowedEgressCIDRsSuccess( + t *testing.T, + expected []string, +) { + t.Helper() + + var cfg *config.Config + + app := fxtest.New( + t, + fx.Provide( + globals.New, + logger.New, + config.New, + ), + fx.Populate(&cfg), + ) + require.NoError(t, app.Err()) + + app.RequireStart() + + defer app.RequireStop() + + assert.Equal( + t, expected, config.PrefixStrings(cfg.AllowedEgressCIDRs), + ) +} + +// TestEgressAllowlistWarning covers the startup log that shows an +// operator the hole ALLOWED_EGRESS_CIDRS opened. It must stay +// silent on the default (empty) list and, when set, print the +// blocks themselves rather than a count. +func TestEgressAllowlistWarning(t *testing.T) { + tests := []struct { + name string + allowed string + expectWarning bool + }{ + { + name: "empty allowlist is quiet", + expectWarning: false, + }, + { + name: "non-empty allowlist warns", + allowed: "10.0.0.0/8,127.0.0.0/8", + expectWarning: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + t.Setenv("WEBHOOKER_ENVIRONMENT", config.EnvironmentDev) + + if tt.allowed == "" { + require.NoError( + t, os.Unsetenv("ALLOWED_EGRESS_CIDRS"), + ) + } else { + t.Setenv("ALLOWED_EGRESS_CIDRS", tt.allowed) + } + + var buf bytes.Buffer + + log := slog.New(slog.NewJSONHandler( + &buf, &slog.HandlerOptions{ + Level: slog.LevelDebug, + }, + )) + + require.NoError( + t, config.WarnEgressAllowlistForTest(log), + ) + + if !tt.expectWarning { + assert.Empty(t, buf.String()) + + return + } + + logged := buf.String() + + assert.Contains(t, logged, `"level":"WARN"`) + assert.Contains(t, logged, "ALLOWED_EGRESS_CIDRS") + // The blocks themselves, not a count: the operator has + // to be able to read back which networks are open. + assert.Contains(t, logged, "10.0.0.0/8") + assert.Contains(t, logged, "127.0.0.0/8") + // The warning must keep saying what stays shut. + assert.Contains(t, logged, "Link-local") + }) + } +} + // TestSharedRateLimitBucketWarning covers the startup warning that // tells an operator a deployment behind a reverse proxy shares one // rate-limit bucket between every client, which turns the receiver diff --git a/internal/config/export_test.go b/internal/config/export_test.go index 1d137a6..710e680 100644 --- a/internal/config/export_test.go +++ b/internal/config/export_test.go @@ -21,6 +21,21 @@ func WarnSharedRateLimitBucketForTest(log *slog.Logger) error { return nil } +// WarnEgressAllowlistForTest loads a Config from the current +// environment and emits its egress-allowlist startup warning to +// log, so a test can assert both that the warning fires only when +// the list is non-empty and that it names the blocks it opened. +func WarnEgressAllowlistForTest(log *slog.Logger) error { + c, err := loadFromEnv() + if err != nil { + return err + } + + c.warnEgressAllowlist(log) + + return nil +} + // EnvBoolForTest exposes envBool. func EnvBoolForTest(key string, defaultValue bool) (bool, error) { return envBool(key, defaultValue) diff --git a/internal/delivery/client_ssrf_test.go b/internal/delivery/client_ssrf_test.go index d31164a..44a676c 100644 --- a/internal/delivery/client_ssrf_test.go +++ b/internal/delivery/client_ssrf_test.go @@ -18,8 +18,9 @@ func newSSRFTestEngine() *delivery.Engine { log := slog.New(slog.DiscardHandler) client := &http.Client{ - Timeout: 30 * time.Second, - Transport: delivery.NewSSRFSafeTransport(), + Timeout: 30 * time.Second, + Transport: delivery.NewTestGuard(). + NewSSRFSafeTransport(), } return delivery.NewTestEngine(log, client, 1) @@ -36,8 +37,8 @@ func TestClientForConfig_TimeoutKeepsSSRFGuard(t *testing.T) { engine := newSSRFTestEngine() blocked := []string{ - "http://127.0.0.1/hook", - "http://169.254.169.254/latest/meta-data/", + loopbackHookURL, + metadataURL, "http://[fe80::1]/hook", } diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index a35ebd1..a50f6bf 100644 --- a/internal/delivery/engine.go +++ b/internal/delivery/engine.go @@ -120,6 +120,7 @@ type EngineParams struct { DB *database.Database DBManager *database.WebhookDBManager Logger *logger.Logger + SSRFGuard *Guard } // Engine processes queued deliveries in the background @@ -168,7 +169,7 @@ func New( e.initTargets(&http.Client{ Timeout: httpClientTimeout, - Transport: NewSSRFSafeTransport(), + Transport: params.SSRFGuard.NewSSRFSafeTransport(), }) e.registerHooks(lc) diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index fc3f434..0b8efa1 100644 --- a/internal/delivery/export_test.go +++ b/internal/delivery/export_test.go @@ -5,6 +5,7 @@ import ( "log/slog" "net" "net/http" + "net/netip" "time" "go.uber.org/fx" @@ -31,6 +32,26 @@ 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 +// range. +func NewTestGuard(allowed ...netip.Prefix) *Guard { + return &Guard{allowed: allowed} +} + +// ExportCheckIP exposes the guard's single decision point, so a +// test can assert the policy both the validator and the dialer +// inherit without needing a live destination. +func (g *Guard) ExportCheckIP(ip net.IP) error { + return g.checkIP(ip) +} + +// ExportAlwaysBlockedNetworks exposes alwaysBlockedNetworks. +func ExportAlwaysBlockedNetworks() []*net.IPNet { + return alwaysBlockedNetworks +} + // ExportBlockedNetworks exposes blockedNetworks. func ExportBlockedNetworks() []*net.IPNet { return blockedNetworks diff --git a/internal/delivery/ssrf.go b/internal/delivery/ssrf.go index fb6bacc..c381ede 100644 --- a/internal/delivery/ssrf.go +++ b/internal/delivery/ssrf.go @@ -6,8 +6,11 @@ import ( "fmt" "net" "net/http" + "net/netip" "net/url" "time" + + "sneak.berlin/go/webhooker/internal/config" ) const ( @@ -25,20 +28,39 @@ var ( errBlockedIP = errors.New( "blocked private/reserved IP range", ) + errBlockedLinkLocal = errors.New( + "blocked link-local range, which serves cloud instance " + + "metadata: ALLOWED_EGRESS_CIDRS cannot open it", + ) errInvalidScheme = errors.New( "only http and https are allowed", ) ) // blockedNetworks contains all private/reserved IP ranges -// that should be blocked to prevent SSRF attacks. +// that should be blocked to prevent SSRF attacks. An operator +// can permit specific blocks out of this set with +// ALLOWED_EGRESS_CIDRS; see Guard. // //nolint:gochecknoglobals // package-level network list is appropriate here var blockedNetworks []*net.IPNet +// alwaysBlockedNetworks are the ranges no configuration can +// open. They are the link-local blocks, which carry the cloud +// instance metadata services (169.254.169.254 and its IPv6 +// equivalents). Reaching one is credential theft rather than +// delivery to an internal service, so a supplied CIDR that +// covers a link-local address still leaves it blocked. +// +// These addresses are also in blockedNetworks; this list is what +// makes them unconditional. +// +//nolint:gochecknoglobals // package-level network list is appropriate here +var alwaysBlockedNetworks []*net.IPNet + //nolint:gochecknoinits // init is the idiomatic way to parse CIDRs once at startup func init() { - cidrs := []string{ + blockedNetworks = mustParseCIDRs([]string{ "127.0.0.0/8", "10.0.0.0/8", "172.16.0.0/12", @@ -56,7 +78,19 @@ func init() { "::1/128", "fc00::/7", "fe80::/10", - } + }) + + alwaysBlockedNetworks = mustParseCIDRs([]string{ + "169.254.0.0/16", + "fe80::/10", + }) +} + +// mustParseCIDRs parses a list of CIDR literals, panicking on a +// bad one. The inputs are compile-time constants, so a failure +// is a programming error rather than a runtime condition. +func mustParseCIDRs(cidrs []string) []*net.IPNet { + networks := make([]*net.IPNet, 0, len(cidrs)) for _, cidr := range cidrs { _, network, err := net.ParseCIDR(cidr) @@ -67,16 +101,15 @@ func init() { )) } - blockedNetworks = append( - blockedNetworks, network, - ) + networks = append(networks, network) } + + return networks } -// isBlockedIP checks whether an IP address falls within -// any blocked private/reserved network range. -func isBlockedIP(ip net.IP) bool { - for _, network := range blockedNetworks { +// matchesAny reports whether ip falls inside any of networks. +func matchesAny(networks []*net.IPNet, ip net.IP) bool { + for _, network := range networks { if network.Contains(ip) { return true } @@ -85,9 +118,40 @@ func isBlockedIP(ip net.IP) bool { return false } +// isBlockedIP checks whether an IP address falls within +// any blocked private/reserved network range, 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 +// applies it in exactly one place, checkIP, which both the +// target-creation validator (ValidateTargetURL) and the delivery +// dialer call. Routing both through the same function is the +// point: when the two paths decided separately they drifted and +// disagreed, which is what made a target creatable but +// undeliverable. +// +// The guard is always on. The allowlist only ever adds specific +// networks to what the default blocklist refuses, and no +// configuration turns the guard off wholesale. +type Guard struct { + // allowed is the operator's ALLOWED_EGRESS_CIDRS. Empty + // (the default) means the default blocklist stands as-is. + allowed []netip.Prefix +} + +// NewGuard builds the process-wide SSRF guard from configuration. +func NewGuard(cfg *config.Config) *Guard { + return &Guard{allowed: cfg.AllowedEgressCIDRs} +} + // ValidateTargetURL checks that an HTTP delivery target // URL is safe from SSRF attacks. -func ValidateTargetURL( +func (g *Guard) ValidateTargetURL( ctx context.Context, targetURL string, ) error { parsed, err := url.Parse(targetURL) @@ -111,36 +175,78 @@ func ValidateTargetURL( } if ip := net.ParseIP(host); ip != nil { - return checkBlockedIP(ip) + return g.checkIP(ip) } - return validateHostname(ctx, host) + return g.validateHostname(ctx, host) } -func validateScheme(scheme string) error { - if scheme != "http" && scheme != "https" { +// NewSSRFSafeTransport creates an http.Transport with a +// custom DialContext that refuses connections to any address +// this guard blocks. It resolves and checks at dial time, so a +// name that passed validation but now answers with a blocked +// address (DNS rebinding) is still refused. +func (g *Guard) NewSSRFSafeTransport() *http.Transport { + return &http.Transport{ + DialContext: g.ssrfDialContext, + } +} + +// allows reports whether ip falls inside the operator's +// configured egress allowlist. +func (g *Guard) allows(ip net.IP) bool { + if len(g.allowed) == 0 { + return false + } + + addr, ok := netip.AddrFromSlice(ip) + if !ok { + return false + } + + // Config unmaps every parsed prefix, so an IPv4-mapped + // address has to be unmapped too or it would never match. + addr = addr.Unmap() + + for _, prefix := range g.allowed { + if prefix.Contains(addr) { + return true + } + } + + return false +} + +// checkIP is the single point at which SSRF policy is decided. +// +// The order is the policy: +// +// 1. Link-local is refused before the allowlist is consulted, +// so no configured CIDR can reach cloud instance metadata. +// 2. The allowlist is consulted next, so a listed private +// network becomes reachable. +// 3. Everything else keeps the default blocklist's answer. +func (g *Guard) checkIP(ip net.IP) error { + if matchesAny(alwaysBlockedNetworks, ip) { return fmt.Errorf( - "unsupported URL scheme %q: %w", - scheme, errInvalidScheme, + "target IP %s: %w", ip, errBlockedLinkLocal, ) } - return nil -} + if g.allows(ip) { + return nil + } -func checkBlockedIP(ip net.IP) error { if isBlockedIP(ip) { return fmt.Errorf( - "target IP %s is in a blocked "+ - "private/reserved range: %w", - ip, errBlockedIP, + "target IP %s: %w", ip, errBlockedIP, ) } return nil } -func validateHostname( +func (g *Guard) validateHostname( ctx context.Context, host string, ) error { dnsCtx, cancel := context.WithTimeout( @@ -165,11 +271,11 @@ func validateHostname( } for _, ipAddr := range ips { - if isBlockedIP(ipAddr.IP) { + err = g.checkIP(ipAddr.IP) + if err != nil { return fmt.Errorf( - "hostname %q resolves to blocked "+ - "IP %s: %w", - host, ipAddr.IP, errBlockedIP, + "hostname %q resolves to a blocked address: %w", + host, err, ) } } @@ -177,16 +283,7 @@ func validateHostname( return nil } -// NewSSRFSafeTransport creates an http.Transport with a -// custom DialContext that blocks connections to -// private/reserved IP addresses. -func NewSSRFSafeTransport() *http.Transport { - return &http.Transport{ - DialContext: ssrfDialContext, - } -} - -func ssrfDialContext( +func (g *Guard) ssrfDialContext( ctx context.Context, network, addr string, ) (net.Conn, error) { @@ -209,11 +306,11 @@ func ssrfDialContext( } for _, ipAddr := range ips { - if isBlockedIP(ipAddr.IP) { + err = g.checkIP(ipAddr.IP) + if err != nil { return nil, fmt.Errorf( - "ssrf: connection to %s (%s) "+ - "blocked: %w", - host, ipAddr.IP, errBlockedIP, + "ssrf: connection to %s blocked: %w", + host, err, ) } } @@ -225,3 +322,14 @@ func ssrfDialContext( net.JoinHostPort(ips[0].IP.String(), port), ) } + +func validateScheme(scheme string) error { + if scheme != "http" && scheme != "https" { + return fmt.Errorf( + "unsupported URL scheme %q: %w", + scheme, errInvalidScheme, + ) + } + + return nil +} diff --git a/internal/delivery/ssrf_allowlist_test.go b/internal/delivery/ssrf_allowlist_test.go new file mode 100644 index 0000000..1dcfa6f --- /dev/null +++ b/internal/delivery/ssrf_allowlist_test.go @@ -0,0 +1,370 @@ +package delivery_test + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "net/netip" + "net/url" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/webhooker/internal/delivery" +) + +// Addresses the SSRF tests in this package share. +const ( + // metadataIP is the cloud instance metadata address, and + // metadataURL an endpoint on it. The guard must never reach + // either, whatever an operator lists. + metadataIP = "169.254.169.254" + metadataURL = "http://" + metadataIP + "/latest/meta-data/" + + // loopbackHookURL is a target on this host: blocked by + // default, reachable only once an operator allowlists + // loopback. + loopbackHookURL = "http://127.0.0.1/hook" + + // publicIP is an ordinary public address, which the guard + // permits with or without an allowlist. + publicIP = "93.184.216.34" +) + +// TestGuardAllowlist_PermittedCIDRDelivers proves the escape +// hatch actually works end to end: with 127.0.0.0/8 allowed, the +// guard's own transport connects to a loopback server and gets a +// response back. The default guard, given the identical URL, +// refuses it — so the delivery succeeds because of the allowlist +// and nothing else. +func TestGuardAllowlist_PermittedCIDRDelivers(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }, + )) + t.Cleanup(srv.Close) + + // httptest listens on loopback, which the default blocklist + // covers: exactly the "forward to a service on this host" + // case the allowlist exists for. + requireLoopback(t, srv.URL) + + guard := delivery.NewTestGuard( + netip.MustParsePrefix("127.0.0.0/8"), + ) + + require.NoError(t, + guard.ValidateTargetURL(context.Background(), srv.URL), + "an allowlisted loopback target must pass validation", + ) + + client := &http.Client{ + Timeout: 5 * time.Second, + Transport: guard.NewSSRFSafeTransport(), + } + + req, err := http.NewRequestWithContext( + context.Background(), http.MethodPost, srv.URL, nil, + ) + require.NoError(t, err) + + resp, err := client.Do(req) + require.NoError(t, + err, "an allowlisted loopback target must be deliverable", + ) + + defer func() { _ = resp.Body.Close() }() + + assert.Equal(t, http.StatusNoContent, resp.StatusCode) + + // The same URL through the default guard must still fail, or + // this test would pass without the allowlist doing anything. + assert.Error(t, + delivery.NewTestGuard().ValidateTargetURL( + context.Background(), srv.URL, + ), + "without the allowlist the same target must be refused", + ) +} + +// TestGuardAllowlist_UnlistedPrivateStillRefused proves the +// allowlist grants only what it names. A guard that opens one +// private block must keep refusing every other one, at both the +// validation and the delivery entry point. +func TestGuardAllowlist_UnlistedPrivateStillRefused(t *testing.T) { + t.Parallel() + + // Only 10.1.0.0/16 is open — a narrow block inside a much + // wider private range, so the test can tell "permits the + // listed block" from "permits anything private". + guard := delivery.NewTestGuard( + netip.MustParsePrefix("10.1.0.0/16"), + ) + + refused := []string{ + "http://192.168.1.10/hook", + "http://172.16.0.1/hook", + loopbackHookURL, + "http://[fc00::1]/hook", + "http://100.64.0.1/hook", + // Private, adjacent to the allowed block, outside it. + "http://10.2.0.1/hook", + } + + for _, target := range refused { + t.Run(target, func(t *testing.T) { + t.Parallel() + + err := guard.ValidateTargetURL( + context.Background(), target, + ) + require.Error(t, + err, "%s is not allowlisted and must be refused", + target, + ) + assert.Contains(t, err.Error(), "blocked") + + assertDialRefused(t, guard, target) + }) + } + + // The block that is listed must in fact be permitted, so the + // refusals above are selective rather than a guard that + // ignores its allowlist entirely. + assert.NoError(t, + guard.ValidateTargetURL( + context.Background(), "http://10.1.2.3/hook", + ), + "the allowlisted block must be permitted", + ) +} + +// TestGuardAllowlist_MetadataAlwaysRefused is the load-bearing +// case: link-local carries cloud instance metadata, so reaching +// it is credential theft rather than delivery to an internal +// service. Every one of these guards names a CIDR that covers +// 169.254.169.254 — including 0.0.0.0/0 — and the address must +// stay refused anyway, on both paths. +func TestGuardAllowlist_MetadataAlwaysRefused(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + allow string + target string + }{ + { + name: "exact metadata host", + allow: "169.254.169.254/32", + target: metadataURL, + }, + { + name: "whole link-local block", + allow: "169.254.0.0/16", + target: metadataURL, + }, + { + name: "supernet covering link-local", + allow: "169.0.0.0/8", + target: metadataURL, + }, + { + name: "the entire IPv4 internet", + allow: "0.0.0.0/0", + target: metadataURL, + }, + { + name: "other link-local address", + allow: "0.0.0.0/0", + target: "http://169.254.1.1/", + }, + { + name: "IPv6 link-local", + allow: "::/0", + target: "http://[fe80::1]/", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + guard := delivery.NewTestGuard( + netip.MustParsePrefix(tt.allow), + ) + + err := guard.ValidateTargetURL( + context.Background(), tt.target, + ) + require.Error(t, + err, + "%s must stay blocked even though %s covers it", + tt.target, tt.allow, + ) + assert.Contains(t, err.Error(), "link-local", + "the refusal must say why it cannot be opened", + ) + + assertDialRefused(t, guard, tt.target) + }) + } +} + +// TestGuardAllowlist_PublicUnaffected asserts the allowlist does +// not narrow anything: public addresses were reachable before it +// existed and stay reachable, whether or not a list is set. +func TestGuardAllowlist_PublicUnaffected(t *testing.T) { + t.Parallel() + + guards := map[string]*delivery.Guard{ + "default": delivery.NewTestGuard(), + "with allowlist": delivery.NewTestGuard( + netip.MustParsePrefix("10.0.0.0/8"), + ), + } + + for name, guard := range guards { + t.Run(name, func(t *testing.T) { + t.Parallel() + + assert.NoError(t, + guard.ValidateTargetURL( + context.Background(), + "http://"+publicIP+"/webhook", + ), + ) + }) + } +} + +// TestGuardCheckIP_BothPathsShareOneDecision asserts that the +// validator and the dialer are not two policies that happen to +// agree: both are defined in terms of checkIP, so the exported +// decision function is the whole answer for a given address. +func TestGuardCheckIP_BothPathsShareOneDecision(t *testing.T) { + t.Parallel() + + guard := delivery.NewTestGuard( + netip.MustParsePrefix("10.0.0.0/8"), + ) + + tests := []struct { + ip string + allowed bool + }{ + {"10.1.2.3", true}, + {publicIP, true}, + {"192.168.1.1", false}, + {"127.0.0.1", false}, + {metadataIP, false}, + } + + for _, tt := range tests { + t.Run(tt.ip, func(t *testing.T) { + t.Parallel() + + ip := net.ParseIP(tt.ip) + require.NotNil(t, ip) + + decision := guard.ExportCheckIP(ip) + + validation := guard.ValidateTargetURL( + context.Background(), "http://"+hostFor(tt.ip)+"/x", + ) + + if tt.allowed { + require.NoError(t, decision) + require.NoError(t, validation) + + return + } + + require.Error(t, decision) + require.Error(t, validation, + "validation must refuse what checkIP refuses", + ) + }) + } +} + +// TestAlwaysBlockedNetworks_AreLinkLocal pins the unconditional +// set to link-local only. It is deliberately small: everything +// else in the default blocklist is an operator's own network and +// must remain openable, or the escape hatch would not work. +func TestAlwaysBlockedNetworks_AreLinkLocal(t *testing.T) { + t.Parallel() + + nets := delivery.ExportAlwaysBlockedNetworks() + + got := make([]string, 0, len(nets)) + for _, n := range nets { + got = append(got, n.String()) + } + + assert.Equal(t, + []string{"169.254.0.0/16", "fe80::/10"}, got, + ) +} + +// requireLoopback fails the test unless rawURL's host is a +// loopback address, so the allowlist test cannot silently stop +// exercising a blocked range. +func requireLoopback(t *testing.T, rawURL string) { + t.Helper() + + parsed, err := url.Parse(rawURL) + require.NoError(t, err) + + ip := net.ParseIP(parsed.Hostname()) + require.NotNil(t, ip, "test server host must be an IP literal") + require.True(t, ip.IsLoopback(), + "test server must listen on loopback, got %s", ip, + ) +} + +// assertDialRefused asserts the guard's transport refuses to +// connect to target, which is the delivery-time half of the +// policy. It never reaches the network: the guard checks the +// resolved address before dialling. +func assertDialRefused( + t *testing.T, guard *delivery.Guard, target string, +) { + t.Helper() + + client := &http.Client{ + Timeout: 5 * time.Second, + Transport: guard.NewSSRFSafeTransport(), + } + + req, err := http.NewRequestWithContext( + context.Background(), http.MethodPost, target, nil, + ) + require.NoError(t, err) + + resp, err := client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + + require.Error(t, err, + "delivery to %s must be refused by the dialer", target, + ) + assert.Contains(t, err.Error(), "blocked", + "the refusal must come from the SSRF guard", + ) +} + +// hostFor renders an IP as it appears in a URL host, bracketing +// IPv6 literals. +func hostFor(ip string) string { + if net.ParseIP(ip).To4() == nil { + return "[" + ip + "]" + } + + return ip +} diff --git a/internal/delivery/ssrf_test.go b/internal/delivery/ssrf_test.go index d919d16..14454e9 100644 --- a/internal/delivery/ssrf_test.go +++ b/internal/delivery/ssrf_test.go @@ -31,10 +31,10 @@ func TestIsBlockedIP_PrivateRanges(t *testing.T) { {"192.168.0.1", "192.168.0.1", true}, {"192.168.255.255", "192.168.255.255", true}, {"169.254.0.1", "169.254.0.1", true}, - {"169.254.169.254", "169.254.169.254", true}, + {metadataIP, metadataIP, true}, {"8.8.8.8", "8.8.8.8", false}, {"1.1.1.1", "1.1.1.1", false}, - {"93.184.216.34", "93.184.216.34", false}, + {publicIP, publicIP, false}, {"::1", "::1", true}, {"fd00::1", "fd00::1", true}, {"fc00::1", "fc00::1", true}, @@ -72,12 +72,12 @@ func TestValidateTargetURL_Blocked(t *testing.T) { t.Parallel() blockedURLs := []string{ - "http://127.0.0.1/hook", + loopbackHookURL, "http://127.0.0.1:8080/hook", "https://10.0.0.1/hook", "http://192.168.1.1/webhook", "http://172.16.0.1/api", - "http://169.254.169.254/latest/meta-data/", + metadataURL, "http://[::1]/hook", "http://[fc00::1]/hook", "http://[fe80::1]/hook", @@ -88,7 +88,7 @@ func TestValidateTargetURL_Blocked(t *testing.T) { t.Run(u, func(t *testing.T) { t.Parallel() - err := delivery.ValidateTargetURL( + err := delivery.NewTestGuard().ValidateTargetURL( context.Background(), u, ) @@ -112,7 +112,7 @@ func TestValidateTargetURL_Allowed(t *testing.T) { t.Run(u, func(t *testing.T) { t.Parallel() - err := delivery.ValidateTargetURL( + err := delivery.NewTestGuard().ValidateTargetURL( context.Background(), u, ) @@ -126,7 +126,7 @@ func TestValidateTargetURL_Allowed(t *testing.T) { func TestValidateTargetURL_InvalidScheme(t *testing.T) { t.Parallel() - err := delivery.ValidateTargetURL( + err := delivery.NewTestGuard().ValidateTargetURL( context.Background(), "ftp://example.com/hook", ) @@ -140,7 +140,7 @@ func TestValidateTargetURL_InvalidScheme(t *testing.T) { func TestValidateTargetURL_EmptyHost(t *testing.T) { t.Parallel() - err := delivery.ValidateTargetURL( + err := delivery.NewTestGuard().ValidateTargetURL( context.Background(), "http:///path", ) @@ -150,7 +150,7 @@ func TestValidateTargetURL_EmptyHost(t *testing.T) { func TestValidateTargetURL_InvalidURL(t *testing.T) { t.Parallel() - err := delivery.ValidateTargetURL( + err := delivery.NewTestGuard().ValidateTargetURL( context.Background(), "://invalid", ) diff --git a/internal/delivery/url_mask_test.go b/internal/delivery/url_mask_test.go index e6cc152..f699b36 100644 --- a/internal/delivery/url_mask_test.go +++ b/internal/delivery/url_mask_test.go @@ -185,7 +185,7 @@ func TestDoHTTPRequest_TransportErrorMasksURL(t *testing.T) { func TestValidateTargetURL_UnparsableURLIsMasked(t *testing.T) { t.Parallel() - err := delivery.ValidateTargetURL( + err := delivery.NewTestGuard().ValidateTargetURL( context.TODO(), "https://hooks.slack.com"+maskSecretPath+"\n", ) diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 8d451bc..b1f251c 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -59,6 +59,7 @@ type HandlersParams struct { Middleware *middleware.Middleware Notifier delivery.Notifier Evictor delivery.WebhookEvictor + SSRFGuard *delivery.Guard } // Handlers provides HTTP handler methods for all application @@ -75,6 +76,11 @@ type Handlers struct { evictor delivery.WebhookEvictor templates map[string]*template.Template + // ssrf validates submitted target URLs. It is the same guard + // the delivery engine dials through, so a URL accepted here + // is one delivery will actually attempt. + ssrf *delivery.Guard + // dummyVerifications counts the equivalent-cost verifications // charged for usernames that do not exist. It exists so a test // can prove that path runs without measuring wall-clock time. @@ -114,6 +120,7 @@ func New( s.mw = params.Middleware s.notifier = params.Notifier s.evictor = params.Evictor + s.ssrf = params.SSRFGuard // Parse all page templates once at startup s.templates = map[string]*template.Template{ diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 83269d0..025e82a 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -84,6 +84,7 @@ func newTestApp( return r }, middleware.New, + delivery.NewGuard, handlers.New, ), fx.Populate(targets...), diff --git a/internal/handlers/source_management.go b/internal/handlers/source_management.go index 5e4602c..57c0f55 100644 --- a/internal/handlers/source_management.go +++ b/internal/handlers/source_management.go @@ -1160,7 +1160,7 @@ func (h *Handlers) buildURLTargetConfig( return "", errMissingURL } - err := delivery.ValidateTargetURL( + err := h.ssrf.ValidateTargetURL( r.Context(), targetURL, ) if err != nil { diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 13fbd0d..139856b 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -97,6 +97,7 @@ func newTestEnv(t *testing.T) *testEnv { func() delivery.Notifier { return &noopNotifier{} }, func() delivery.WebhookEvictor { return &noopEvictor{} }, middleware.New, + delivery.NewGuard, handlers.New, ), fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db, &dbMgr),