From 49bcb009ccb6066c8d27d42c0a3dd871592427b3 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 00:30:03 +0200 Subject: [PATCH 1/2] feat(service): restrict a service by client address with --allow-ip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Adds `kamal-proxy deploy --allow-ip ` (plus `--trusted-proxy`) and `kamal-proxy run --metrics-allow-ip `. Requests from outside the list get a 403. **The filter matches the connecting peer, never a header.** Every IP-shaped value on a request is written by the client, so honouring one by default would make the list decorative — anyone could send `X-Forwarded-For: 10.0.0.1`. Note that the anchor this issue and the ROADMAP both named, `logging_middleware.go:70`, is exactly the trap: that function's `remote_addr` field is raw `X-Forwarded-For` with no trust check at all. Both are corrected here. Headers are consulted only when the peer is itself inside `--trusted-proxy`, and then the chain is walked from the nearest hop backwards past our own proxies. An unresolvable chain denies rather than falling back to the peer, which would be a bypass whenever the allow list contains the proxy's own range. Three bypasses were closed that a straightforward implementation would ship: - `Header.Get` returns only the FIRST of repeated header lines, and HAProxy's `option forwardfor` appends a new line rather than folding. A client that sends its own header therefore owns the whole walk. Reads use `Values`. - `netip.Prefix.Contains` reports false for `::ffff:203.0.113.5` against `203.0.113.0/24`, so connecting over IPv6 would bypass a v4 list. Addresses are unmapped before matching. - A zoned address such as `fe80::1%eth0` parses fine but matches nothing, so storing one would silently do nothing. Rejected at parse time. Not a `createMiddleware` middleware: that chain includes the certificate manager's handler, so filtering there would block ACME HTTP-01 and break renewal weeks later. The check is the first thing `serviceRequestWithTarget` does, before the HTTPS redirect — a 403 solicits nothing, so there is no reason to redirect a peer we are about to refuse. Allow-only; no `--deny-ip`. Ordered allow/deny semantics depend on argument order, which is a footgun in config a gem regenerates, and a static deny list is the wrong tool for the abuse-blocking people reach for it with. Adding it later is additive. Also extracts `validateInterceptErrorStatuses`/`validateDynamicDomains` into `service_options_validation.go`: `service.go` was at 790 of the hard 800-line ceiling, and the rule says to split before adding. ## Test Coverage - TestIPAllowList_IgnoresForgedFirstForwardedHeaderLine: the repeated-header bypass - TestIPAllowList_MatchesNormalizedAddresses: v4-mapped v6, zones, v6-vs-v4 - TestIPAllowList_IgnoresForwardedHeadersWithoutTrustedProxy: no header buys access - TestIPAllowList_ResolvesThroughTrustedProxy / _DeniesUnresolvableForwardedChain - TestIPAllowList_RejectsBeforeRedirecting / _RejectsBeforeChallengingBasicAuth - TestIPAllowList_ExemptsHealthCheckRequests / _RejectsRootHealthCheckPath - TestIPAllowList_StateWrittenBeforeTheOptionStaysUnrestricted, _EmptyStoredListStaysUnrestricted, _UnreadableStoredEntryFailsClosed - TestMetricsAllowList, incl. an invalid entry failing the boot with metrics off Three mutations were applied and confirmed to fail their tests: Values reverted to Get, Unmap dropped, and unresolvable-chain falling back to the peer. ## Verification - [x] gofmt -l internal/ cmd/ clean - [x] go vet ./... clean - [x] make test passes - [x] go test -race ./internal/server/ ./internal/cmd/ — 766 tests, clean - [x] BenchmarkIPAllowList_Permits: 51 ns/op, 0 allocs/op Closes #9 --- README.md | 50 ++ ROADMAP.md | 2 +- internal/cmd/deploy.go | 2 + internal/cmd/deploy_test.go | 70 +++ internal/cmd/run.go | 1 + internal/server/config.go | 4 + internal/server/ip_allow_list.go | 369 ++++++++++++++ internal/server/ip_allow_list_service_test.go | 343 +++++++++++++ internal/server/ip_allow_list_test.go | 456 ++++++++++++++++++ internal/server/router.go | 4 + internal/server/server.go | 9 +- internal/server/service.go | 68 +-- internal/server/service_options_validation.go | 59 +++ 13 files changed, 1384 insertions(+), 53 deletions(-) create mode 100644 internal/server/ip_allow_list.go create mode 100644 internal/server/ip_allow_list_service_test.go create mode 100644 internal/server/ip_allow_list_test.go create mode 100644 internal/server/service_options_validation.go diff --git a/README.md b/README.md index 582d86f8..75e01dda 100644 --- a/README.md +++ b/README.md @@ -190,6 +190,56 @@ If you use `--error-pages`, add a `401.html` to that directory; otherwise the challenge falls back to the proxy's built-in plain response. +### Restricting a service by client address + +To serve a service only to certain networks, deploy it with `--allow-ip`: + + kamal-proxy deploy service1 --target web-1:3000 --allow-ip 10.0.0.0/8,203.0.113.7 + +Requests from anywhere else get a `403`. The flag takes addresses or CIDR +ranges, and may be repeated or comma-separated. Metrics have their own list: + + kamal-proxy run --metrics-port 9090 --metrics-allow-ip 10.0.0.0/8 + +Things worth knowing: + +* **It matches the address that connected**, not any header. `X-Forwarded-For` + and friends are written by the client, so honouring them by default would + make the list decorative — anyone could send `X-Forwarded-For: 10.0.0.1`. +* **Behind a load balancer or CDN, say so with `--trusted-proxy`.** Only when + the connecting address is inside one of those ranges is the forwarded chain + consulted, and then the client is the nearest address in the chain that none + of your proxies wrote. **List every hop, not just the one that connects to + kamal-proxy** — behind a CDN in front of a load balancer, list both, or the + CDN's edge address becomes the one matched against `--allow-ip`. +* **If the chain cannot be resolved, the request is denied.** A trusted edge + that stops sending the header denies everything rather than silently falling + back to the edge's own address, which would be a bypass whenever your allow + list contains the proxy's own range. Denials are logged with the address the + decision used and why, rate-limited per service. +* **List your IPv6 ranges too.** A client reaching the proxy over IPv6 is + matched on its IPv6 address; an IPv4-only list denies it. The proxy warns at + deploy when a list has no IPv6 ranges. (`::ffff:` forms of IPv4 addresses are + matched against IPv4 ranges, so those do not need listing separately.) +* **The health check path stays open**, so downstream load balancers can still + see the service drain during a deploy. Deploying with both `--allow-ip` and a + health check path of `/` is rejected. +* **`--client-ip-header` requires `--trusted-proxy`.** Without it the deploy is + rejected, because the header would be ignored while appearing to be honoured. + With it, be sure the header names one your edge *overwrites or strips* on + every request — a header the edge merely passes through can be set by anyone. +* **Redeploying without the flag removes the restriction**, and rolling the + proxy image back to a version without this feature removes it silently. +* **Check what the proxy actually sees.** The access log's `client_addr` is the + connecting address; `remote_addr` is the client's own claim and is not what + the filter uses. Under some Docker port drivers every request appears to come + from the bridge gateway, in which case a list cannot distinguish anyone. +* **A `--path-prefix` service is routing, not a security boundary** — the same + caveat as basic auth above. + +If you use `--error-pages`, add a `403.html` to that directory. + + ### Automatic TLS Kamal Proxy can automatically obtain and renew TLS certificates for your diff --git a/ROADMAP.md b/ROADMAP.md index ada6f694..ad76c4b4 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -25,7 +25,7 @@ Proxy-side roadmap for the dash fork. The cross-repo release sequencing, strateg | Item | Evidence | Anchor | |---|---|---| | Basic auth per service | port PR #216 (open); kamal#1604 | DONE — `--basic-auth :` (`internal/server/basic_auth.go`). Deliberately **not** a `createMiddleware` middleware: that chain wraps `serviceRequestWithTarget`, which is where the HTTPS redirect lives, so a middleware there challenges before the 301 and the browser sends the password in cleartext. The check sits inline after `handleRedirectsIfNeeded`. Credentials are hashed CLI-side (salted SHA-256), so no plaintext crosses the RPC socket or reaches the state file. Per-path scoping is served by deploying a `--path-prefix` service with its own credential | -| IP allow/deny (CIDR) | discussions #143/#144 | middleware; client addr extraction exists (`logging_middleware.go:70`) | +| IP allow list (CIDR) | discussions #143/#144 | DONE — `--allow-ip`/`--trusted-proxy` on deploy, `--metrics-allow-ip` on run (`internal/server/ip_allow_list.go`). Allow-only; a static deny list is the wrong tool for the abuse-blocking people reach for it with. **Not** a `createMiddleware` middleware — that chain includes the cert manager's handler, so filtering there breaks ACME HTTP-01 and certificates fail to renew weeks later. Runs as the first check in `serviceRequestWithTarget`, before the HTTPS redirect. Matches the connecting peer, never a header, unless the peer is inside `--trusted-proxy`. The `logging_middleware.go:70` anchor this row used to name is the trap: that function's `remote_addr` is raw `X-Forwarded-For` with no trust check | | Per-IP rate limiting (token bucket + burst + allowlist) | rejected #20 | global chain (`server.go:211 buildHandler`) or per-service; `golang.org/x/time/rate` | | PROXY protocol | rejected #31, discussion #41 | `go-proxyproto` listener wrap in `server.go`; `run` flag | | mTLS (`--tls-client-ca-path`) | port PR #204 (open); kamal#1628 | `tls.Config.ClientCAs/ClientAuth` on HTTPS listener (`server.go:158`) | diff --git a/internal/cmd/deploy.go b/internal/cmd/deploy.go index 721dd9ce..5c7be88d 100644 --- a/internal/cmd/deploy.go +++ b/internal/cmd/deploy.go @@ -82,6 +82,8 @@ func newDeployCommand() *deployCommand { deployCommand.cmd.Flags().StringVar(&deployCommand.args.ServiceOptions.ErrorPagePath, "error-pages", "", "Path to custom error pages") deployCommand.cmd.Flags().IntSliceVar(&deployCommand.args.ServiceOptions.InterceptErrorStatuses, "intercept-errors", nil, "Replace these response statuses from the target with the proxy's error pages, as 4xx or 5xx codes (e.g. 502,503,504; default none)") deployCommand.cmd.Flags().StringVar(&deployCommand.basicAuth, "basic-auth", "", "Require HTTP Basic credentials on every request to this service, as :. The health check path stays open. Use with --tls, or terminate TLS in front of the proxy -- Basic credentials are replayable and are sent on every request") + deployCommand.cmd.Flags().StringSliceVar(&deployCommand.args.ServiceOptions.AllowIPs, "allow-ip", nil, "Serve this service only to these addresses or CIDR ranges (e.g. 10.0.0.0/8,203.0.113.7; default empty, serve everyone). Matches the connecting address, so list IPv6 ranges too if clients reach the proxy over IPv6. The health check path stays open") + deployCommand.cmd.Flags().StringSliceVar(&deployCommand.args.ServiceOptions.TrustedProxies, "trusted-proxy", nil, "Addresses or CIDR ranges of proxies in front of this one. Only when the connecting address is one of these is --allow-ip matched against the forwarded chain instead. List every hop, not just the one that connects to kamal-proxy") deployCommand.cmd.Flags().StringSliceVar(&deployCommand.args.TargetOptions.LogRequestHeaders, "log-request-header", nil, "Additional request header to log (may be specified multiple times)") deployCommand.cmd.Flags().StringSliceVar(&deployCommand.args.TargetOptions.LogResponseHeaders, "log-response-header", nil, "Additional response header to log (may be specified multiple times)") diff --git a/internal/cmd/deploy_test.go b/internal/cmd/deploy_test.go index 9661ca59..74109c10 100644 --- a/internal/cmd/deploy_test.go +++ b/internal/cmd/deploy_test.go @@ -468,3 +468,73 @@ func TestDeployCommand_BasicAuthAbsentLeavesServiceUnprotected(t *testing.T) { require.NoError(t, cmd.preRun(cmd.cmd, []string{"test-service"})) assert.Empty(t, cmd.args.ServiceOptions.BasicAuth) } + +func TestDeployCommand_AllowIPFlags(t *testing.T) { + tests := []struct { + name string + args []string + expectedAllow []string + expectedTrusted []string + expectedError string + }{ + { + name: "unset leaves the service unrestricted", + args: []string{"--target=web:3000"}, + }, + { + name: "a single range", + args: []string{"--target=web:3000", "--allow-ip=10.0.0.0/8"}, + expectedAllow: []string{"10.0.0.0/8"}, + }, + { + name: "comma-separated ranges", + args: []string{"--target=web:3000", "--allow-ip=10.0.0.0/8,203.0.113.7"}, + expectedAllow: []string{"10.0.0.0/8", "203.0.113.7"}, + }, + { + name: "with trusted proxies", + args: []string{"--target=web:3000", "--allow-ip=10.0.0.0/8", "--trusted-proxy=172.16.0.0/12"}, + expectedAllow: []string{"10.0.0.0/8"}, + expectedTrusted: []string{"172.16.0.0/12"}, + }, + { + name: "a malformed range is rejected", + args: []string{"--target=web:3000", "--allow-ip=nonsense"}, + expectedError: "allow-ip", + }, + { + name: "trusted proxies without an allow list are rejected", + args: []string{"--target=web:3000", "--trusted-proxy=172.16.0.0/12"}, + expectedError: "trusted-proxy requires allow-ip", + }, + { + name: "a default route cannot be trusted", + args: []string{"--target=web:3000", "--allow-ip=10.0.0.0/8", "--trusted-proxy=0.0.0.0/0"}, + expectedError: "default route", + }, + { + name: "client-ip-header without trusted proxies is rejected", + args: []string{"--target=web:3000", "--allow-ip=10.0.0.0/8", "--client-ip-header=CF-Connecting-IP"}, + expectedError: "requires trusted-proxy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := newDeployCommand() + require.NoError(t, cmd.cmd.Flags().Parse(tt.args)) + + err := cmd.preRun(cmd.cmd, []string{"test-service"}) + + if tt.expectedError != "" { + require.ErrorIs(t, err, server.ErrServiceOptionsInvalid) + require.ErrorContains(t, err, tt.expectedError) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expectedAllow, cmd.args.ServiceOptions.AllowIPs) + assert.Equal(t, tt.expectedTrusted, cmd.args.ServiceOptions.TrustedProxies) + }) + } +} diff --git a/internal/cmd/run.go b/internal/cmd/run.go index 8af873d1..6cdc365e 100644 --- a/internal/cmd/run.go +++ b/internal/cmd/run.go @@ -34,6 +34,7 @@ func newRunCommand() *runCommand { runCommand.cmd.Flags().IntVar(&globalConfig.HttpPort, "http-port", getEnvInt("HTTP_PORT", server.DefaultHttpPort), "Port to serve HTTP traffic on") runCommand.cmd.Flags().IntVar(&globalConfig.HttpsPort, "https-port", getEnvInt("HTTPS_PORT", server.DefaultHttpsPort), "Port to serve HTTPS traffic on") runCommand.cmd.Flags().IntVar(&globalConfig.MetricsPort, "metrics-port", getEnvInt("METRICS_PORT", 0), "Publish metrics on the specified port (default zero to disable)") + runCommand.cmd.Flags().StringSliceVar(&globalConfig.MetricsAllowIPs, "metrics-allow-ip", nil, "Serve the metrics endpoint only to these addresses or CIDR ranges (default empty, serve everyone that can reach the port)") runCommand.cmd.Flags().BoolVar(&globalConfig.HTTP3Enabled, "http3", false, "Enable HTTP/3") runCommand.cmd.Flags().BoolVar(&runCommand.ignoreRestoreErrors, "ignore-restore-errors", getEnvBool("IGNORE_RESTORE_ERRORS", false), "Boot with an empty routing state when restoring the saved state fails") runCommand.cmd.Flags().BoolVar(&runCommand.recheckTargetsOnRestore, "recheck-targets-on-restore", getEnvBool("RECHECK_TARGETS_ON_RESTORE", false), "Re-verify restored targets with health checks instead of assuming they are healthy") diff --git a/internal/server/config.go b/internal/server/config.go index 51617439..9fbb8e56 100644 --- a/internal/server/config.go +++ b/internal/server/config.go @@ -46,6 +46,10 @@ type Config struct { MetricsPort int HTTP3Enabled bool + // MetricsAllowIPs restricts the metrics endpoint to these addresses and CIDR + // ranges. Empty (the default) serves everyone that can reach the port. + MetricsAllowIPs []string + ReadHeaderTimeout time.Duration ReadTimeout time.Duration WriteTimeout time.Duration diff --git a/internal/server/ip_allow_list.go b/internal/server/ip_allow_list.go new file mode 100644 index 00000000..f49e8a44 --- /dev/null +++ b/internal/server/ip_allow_list.go @@ -0,0 +1,369 @@ +package server + +import ( + "fmt" + "log/slog" + "net" + "net/http" + "net/netip" + "slices" + "strings" + "time" +) + +const ( + // forwardedChainLimit bounds how far back through a forwarded chain we walk, + // so a long header cannot turn every request into a linear scan. + forwardedChainLimit = 32 + + // Denials are logged at most this often per service. A scanner would + // otherwise fill the log with one line per probe. + deniedLogBurst = 10 + deniedLogInterval = 10 * time.Second +) + +// ipAllowList decides whether a request's client address is permitted. +// +// The address it matches is the one net/http wrote into r.RemoteAddr from the +// accepted TCP connection. Nothing in this proxy lets a client influence that +// value, which is what makes the filter meaningful. Every other IP-shaped value +// on a request -- X-Forwarded-For, X-Real-IP, and whatever --client-ip-header +// names -- is written by the client and is only consulted when the peer itself +// is one of the operator's declared proxies. +type ipAllowList struct { + prefixes []netip.Prefix + trusted []netip.Prefix + clientIPHeader string + + denied *tokenBucket +} + +func newIPAllowList(allowIPs, trustedProxies []string, clientIPHeader string) (*ipAllowList, error) { + prefixes, err := parseIPPrefixes(allowIPs, "allow-ip") + if err != nil { + return nil, err + } + + trusted, err := parseIPPrefixes(trustedProxies, "trusted-proxy") + if err != nil { + return nil, err + } + + header := "" + if clientIPHeader != "" { + header = http.CanonicalHeaderKey(clientIPHeader) + } + + return &ipAllowList{ + prefixes: prefixes, + trusted: trusted, + clientIPHeader: header, + denied: newTokenBucket(deniedLogBurst, deniedLogInterval), + }, nil +} + +// permits reports whether addr falls inside the allow list. The zero Addr -- +// which is what an unparseable peer or an unresolvable forwarded chain produces +// -- is never permitted, not even by a default route. +func (l *ipAllowList) permits(addr netip.Addr) bool { + if !addr.IsValid() { + return false + } + + return containsAddr(l.prefixes, addr) +} + +// clientAddr resolves the address to match this request against. +// +// With no trusted proxies declared, that is always the peer. Otherwise, when +// the peer is one of ours, the client is taken from the forwarded chain by +// walking it from the nearest hop backwards past any other proxy of ours; the +// first address a proxy of ours did not write is the client. An unresolvable +// chain returns the zero Addr, which denies: falling back to the peer would be +// a bypass on the very plausible configuration where the allow list contains +// the proxy's own range. +func (l *ipAllowList) clientAddr(r *http.Request) netip.Addr { + peer := parseHostAddr(r.RemoteAddr) + + if len(l.trusted) == 0 || !peer.IsValid() || !containsAddr(l.trusted, peer) { + return peer + } + + return l.forwardedAddr(r) +} + +// forwardedAddr walks the forwarded chain from the nearest hop backwards. +// +// The header is read with Values, not Get: repeated header lines are distinct +// entries, and a hop that appends its own line rather than folding into the +// client's leaves the client's forged line first. Reading only that line would +// hand the whole walk to the attacker. +func (l *ipAllowList) forwardedAddr(r *http.Request) netip.Addr { + header := "X-Forwarded-For" + if l.clientIPHeader != "" { + // ClientIPMiddleware rewrites X-Forwarded-For from this header before we + // run, so read the original rather than the value it left behind. + header = l.clientIPHeader + } + + entries := []string{} + for _, value := range r.Header.Values(header) { + for _, entry := range strings.Split(value, ",") { + entries = append(entries, entry) + } + } + + if len(entries) > forwardedChainLimit { + entries = entries[len(entries)-forwardedChainLimit:] + } + + for i := len(entries) - 1; i >= 0; i-- { + addr := parseForwardedAddr(entries[i]) + if !addr.IsValid() { + return netip.Addr{} + } + + if !containsAddr(l.trusted, addr) { + return addr + } + } + + return netip.Addr{} +} + +// Private + +// normalizeAddr puts an address into the one form the prefixes are compared +// against: IPv4-mapped IPv6 unwrapped to plain IPv4, and any zone dropped. +// Without the unmap, ::ffff:203.0.113.5 does not match 203.0.113.0/24 and the +// list is bypassed by connecting over IPv6. +func normalizeAddr(addr netip.Addr) netip.Addr { + return addr.Unmap().WithZone("") +} + +func containsAddr(prefixes []netip.Prefix, addr netip.Addr) bool { + addr = normalizeAddr(addr) + + return slices.ContainsFunc(prefixes, func(prefix netip.Prefix) bool { + return prefix.Contains(addr) + }) +} + +// parseHostAddr reads the address out of a "host:port" pair such as +// r.RemoteAddr, returning the zero Addr when it is not one. +func parseHostAddr(hostPort string) netip.Addr { + host, _, err := net.SplitHostPort(hostPort) + if err != nil { + host = hostPort + } + + addr, err := netip.ParseAddr(strings.TrimSpace(host)) + if err != nil { + return netip.Addr{} + } + + return normalizeAddr(addr) +} + +// parseForwardedAddr reads one entry of a forwarded chain. Entries are normally +// bare addresses, but some proxies append a port. +func parseForwardedAddr(entry string) netip.Addr { + entry = strings.TrimSpace(entry) + + if addr, err := netip.ParseAddr(entry); err == nil { + return normalizeAddr(addr) + } + + return parseHostAddr(entry) +} + +func parseIPPrefix(entry string) (netip.Prefix, error) { + entry = strings.TrimSpace(entry) + if entry == "" { + return netip.Prefix{}, fmt.Errorf("address or CIDR range cannot be empty") + } + + if strings.Contains(entry, "/") { + prefix, err := netip.ParsePrefix(entry) + if err != nil { + return netip.Prefix{}, fmt.Errorf("%q is not a valid CIDR range", entry) + } + if prefix.Addr().Is4In6() { + return netip.Prefix{}, fmt.Errorf("%q is an IPv4-mapped range; write it as plain IPv4", entry) + } + + // Masked so that 10.1.2.3/8 and 10.0.0.0/8 behave identically. + return prefix.Masked(), nil + } + + addr, err := netip.ParseAddr(entry) + if err != nil { + return netip.Prefix{}, fmt.Errorf("%q is not a valid address", entry) + } + if addr.Is4In6() { + return netip.Prefix{}, fmt.Errorf("%q is an IPv4-mapped address; write it as plain IPv4", entry) + } + // A zoned address matches nothing, so accepting one would silently do + // nothing rather than what the operator asked for. + if addr.Zone() != "" { + return netip.Prefix{}, fmt.Errorf("%q carries an IPv6 zone; write the address without it", entry) + } + + return netip.PrefixFrom(addr, addr.BitLen()), nil +} + +func parseIPPrefixes(entries []string, flagName string) ([]netip.Prefix, error) { + if len(entries) == 0 { + return nil, nil + } + + prefixes := make([]netip.Prefix, 0, len(entries)) + for _, entry := range entries { + prefix, err := parseIPPrefix(entry) + if err != nil { + return nil, fmt.Errorf("%w: %s: %s", ErrServiceOptionsInvalid, flagName, err) + } + prefixes = append(prefixes, prefix) + } + + return prefixes, nil +} + +func (so ServiceOptions) validateAllowIPs() error { + if len(so.TrustedProxies) > 0 && len(so.AllowIPs) == 0 { + return fmt.Errorf("%w: trusted-proxy requires allow-ip", ErrServiceOptionsInvalid) + } + + if len(so.AllowIPs) > 0 && so.ClientIPHeader != "" && len(so.TrustedProxies) == 0 { + return fmt.Errorf("%w: allow-ip with client-ip-header requires trusted-proxy, or the header would be ignored while appearing to be honored", ErrServiceOptionsInvalid) + } + + if _, err := parseIPPrefixes(so.AllowIPs, "allow-ip"); err != nil { + return err + } + + trusted, err := parseIPPrefixes(so.TrustedProxies, "trusted-proxy") + if err != nil { + return err + } + + // Trusting everything means trusting every client to speak for someone else, + // which is the whole bypass this flag exists to prevent. + for _, prefix := range trusted { + if prefix.Bits() == 0 { + return fmt.Errorf("%w: trusted-proxy cannot contain a default route (%s)", ErrServiceOptionsInvalid, prefix) + } + } + + return nil +} + +// validateAllowIPsHealthCheck rejects the health check path that would quietly +// unrestrict the service, matching the equivalent rule for basic auth. +func validateAllowIPsHealthCheck(options ServiceOptions, targetOptions TargetOptions) error { + if len(options.AllowIPs) == 0 { + return nil + } + + path := targetOptions.HealthCheckConfig.Path + if path == "" || path == rootPath { + return fmt.Errorf("%w: health-check-path cannot be %q when allow-ip is set, as that path is served without an address check", ErrServiceOptionsInvalid, rootPath) + } + + return nil +} + +// resolveIPAllowList prepares the stored list for serving. It never returns an +// error: this runs from initialize, which runs while decoding saved state, and +// failing there would abort the decode of every other service too. +func (s *Service) resolveIPAllowList(options ServiceOptions) *ipAllowList { + if len(options.AllowIPs) == 0 { + return nil + } + + list, err := newIPAllowList(options.AllowIPs, options.TrustedProxies, options.ClientIPHeader) + if err != nil { + slog.Error("Unable to read the stored allow list; denying every request to this service", "service", s.name, "error", err) + + return &ipAllowList{denied: newTokenBucket(deniedLogBurst, deniedLogInterval)} + } + + if !slices.ContainsFunc(list.prefixes, func(p netip.Prefix) bool { return p.Addr().Is6() }) { + slog.Warn("allow-ip lists no IPv6 ranges; clients reaching the proxy over IPv6 will be denied", "service", s.name) + } + if len(list.trusted) > 0 && !slices.ContainsFunc(list.trusted, func(p netip.Prefix) bool { return p.Addr().Is6() }) { + slog.Warn("trusted-proxy lists no IPv6 ranges; requests arriving over IPv6 will be matched on the connecting address instead of the forwarded one", "service", s.name) + } + + slog.Info("Client address filtering enabled", "service", s.name, "allow", options.AllowIPs, "trusted_proxies", options.TrustedProxies) + + return list +} + +// rejectDisallowedIP denies a request whose client address is outside the +// service's allow list, reporting whether it handled the response. +// +// It runs as the first check in serviceRequestWithTarget, before the HTTPS +// redirect: a 403 solicits nothing, so there is no reason to redirect a peer we +// are about to refuse. It deliberately does NOT live in createMiddleware -- +// that chain wraps this handler and includes the certificate manager's own +// handler, so filtering up there would block ACME HTTP-01 validation and break +// certificate issuance and renewal weeks later. +func (s *Service) rejectDisallowedIP(w http.ResponseWriter, r *http.Request) bool { + if s.allowedIPs == nil { + return false + } + + // Probes the proxy makes about itself, and health checks a downstream load + // balancer needs in order to see this service drain during a deploy. + if isInternalRequest(r) || s.targetOptions.IsHealthCheckRequest(r) { + return false + } + + addr := s.allowedIPs.clientAddr(r) + if s.allowedIPs.permits(addr) { + return false + } + + s.logDeniedIP(r, addr) + SetErrorResponse(w, r, http.StatusForbidden, nil) + + return true +} + +func (s *Service) logDeniedIP(r *http.Request, addr netip.Addr) { + if !s.allowedIPs.denied.TryTake() { + return + } + + // The access log records the peer and the client's own header claim, neither + // of which is necessarily the address this decision used, so say which it + // was and why it failed. + reason := "address not allowed" + resolved := addr.String() + if !addr.IsValid() { + reason = "could not determine a client address" + resolved = "" + } + + slog.Warn("Denied by allow-ip", "service", s.name, "peer", r.RemoteAddr, "client_addr", resolved, "reason", reason, "path", r.URL.Path) +} + +// withMetricsAllowList restricts the metrics endpoint to the given ranges. The +// metrics server is its own listener with no service behind it, so it matches +// the connecting address only -- there is no forwarded chain to trust here. +func withMetricsAllowList(allowed []netip.Prefix, next http.Handler) http.Handler { + if len(allowed) == 0 { + return next + } + + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + addr := parseHostAddr(r.RemoteAddr) + if !addr.IsValid() || !containsAddr(allowed, addr) { + http.Error(w, http.StatusText(http.StatusForbidden), http.StatusForbidden) + return + } + + next.ServeHTTP(w, r) + }) +} diff --git a/internal/server/ip_allow_list_service_test.go b/internal/server/ip_allow_list_service_test.go new file mode 100644 index 00000000..08d0e0a7 --- /dev/null +++ b/internal/server/ip_allow_list_service_test.go @@ -0,0 +1,343 @@ +package server + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + testAllowedPeer = "10.0.0.5:44321" + testDeniedPeer = "203.0.113.9:44321" +) + +// testRestrictedService deploys a service behind an allow list and returns a +// handler wired the way the real server wires one. +func testRestrictedService(t *testing.T, options ServiceOptions, handler http.HandlerFunc) http.Handler { + t.Helper() + + router := testRouter(t) + _, target := testBackendWithHandler(t, handler) + + if options.AllowIPs == nil { + options.AllowIPs = []string{"10.0.0.0/8"} + } + + require.NoError(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, + options, defaultTargetOptions, defaultDeploymentOptions)) + + return testRoutedHandler(t, router) +} + +func testRequestFromPeer(peer, url string) *http.Request { + req := httptest.NewRequest(http.MethodGet, url, nil) + req.RemoteAddr = peer + + return req +} + +func TestIPAllowList_EmptyOptionServesEveryone(t *testing.T) { + options := defaultServiceOptions + options.AllowIPs = []string{} + + handler := testRestrictedService(t, options, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("ok")) + }) + + resp := testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + + assert.Equal(t, http.StatusOK, resp.StatusCode) +} + +func TestIPAllowList_AllowsMatchingPeer(t *testing.T) { + handler := testRestrictedService(t, defaultServiceOptions, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("ok")) + }) + + resp := testAuthRequest(handler, testRequestFromPeer(testAllowedPeer, "http://example.com/")) + + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Equal(t, "ok", testAuthBody(t, resp)) +} + +func TestIPAllowList_RejectsNonMatchingPeer(t *testing.T) { + var reachedTarget atomic.Int64 + + handler := testRestrictedService(t, defaultServiceOptions, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != DefaultHealthCheckPath { + reachedTarget.Add(1) + } + w.Write([]byte("secret")) + }) + + resp := testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + assert.Zero(t, reachedTarget.Load(), "the target must never see a denied request") +} + +func TestIPAllowList_RejectsSpoofedForwardedHeader(t *testing.T) { + // The end-to-end form of the anti-regression test: no --trusted-proxy, so a + // header cannot buy access no matter what it claims. + handler := testRestrictedService(t, defaultServiceOptions, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("secret")) + }) + + req := testRequestFromPeer(testDeniedPeer, "http://example.com/") + req.Header.Add("X-Forwarded-For", "10.0.0.1") + req.Header.Add("X-Real-IP", "10.0.0.1") + + resp := testAuthRequest(handler, req) + + assert.Equal(t, http.StatusForbidden, resp.StatusCode) +} + +func TestIPAllowList_RejectsBeforeRedirecting(t *testing.T) { + // The deliberate inverse of the basic-auth ordering: a 403 solicits nothing, + // so a denied peer is refused rather than redirected first. + options := defaultServiceOptions + options.TLSEnabled = true + options.TLSRedirect = true + options.Hosts = []string{"example.com"} + + handler := testRestrictedService(t, options, func(w http.ResponseWriter, r *http.Request) {}) + + resp := testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + assert.Empty(t, resp.Header.Get("Location")) +} + +func TestIPAllowList_RejectsBeforeChallengingBasicAuth(t *testing.T) { + options := defaultServiceOptions + options.BasicAuth = testEncodedCredential(t, testAuthUser, testAuthPassword) + + handler := testRestrictedService(t, options, func(w http.ResponseWriter, r *http.Request) {}) + + resp := testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + + // A denied network never learns that the service wants credentials. + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + assert.Empty(t, resp.Header.Get("WWW-Authenticate")) +} + +func TestIPAllowList_ExemptsHealthCheckRequests(t *testing.T) { + handler := testRestrictedService(t, defaultServiceOptions, func(w http.ResponseWriter, r *http.Request) {}) + + tests := []struct { + name string + method string + path string + expectedStatus int + }{ + {"GET on the health check path", http.MethodGet, DefaultHealthCheckPath, http.StatusOK}, + {"HEAD on the health check path", http.MethodHead, DefaultHealthCheckPath, http.StatusOK}, + {"POST on the health check path", http.MethodPost, DefaultHealthCheckPath, http.StatusForbidden}, + {"any other path", http.MethodGet, "/", http.StatusForbidden}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, "http://example.com"+tt.path, nil) + req.RemoteAddr = testDeniedPeer + + assert.Equal(t, tt.expectedStatus, testAuthRequest(handler, req).StatusCode) + }) + } +} + +func TestIPAllowList_ExemptsInternalRequests(t *testing.T) { + handler := testRestrictedService(t, defaultServiceOptions, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("ok")) + }) + + req := testRequestFromPeer(testDeniedPeer, "http://example.com/") + req = req.WithContext(markInternalRequest(req.Context())) + + assert.Equal(t, http.StatusOK, testAuthRequest(handler, req).StatusCode) +} + +func TestIPAllowList_RejectsRootHealthCheckPath(t *testing.T) { + router := testRouter(t) + _, target := testBackendWithHandler(t, func(w http.ResponseWriter, r *http.Request) {}) + + options := defaultServiceOptions + options.AllowIPs = []string{"10.0.0.0/8"} + + targetOptions := defaultTargetOptions + targetOptions.HealthCheckConfig.Path = "/" + + err := router.DeployService("service1", []string{target}, defaultEmptyReaders, + options, targetOptions, defaultDeploymentOptions) + + require.ErrorIs(t, err, ErrServiceOptionsInvalid) + require.ErrorContains(t, err, "health-check-path") +} + +func TestIPAllowList_DeployRejectsClientIPHeaderWithoutTrustedProxy(t *testing.T) { + router := testRouter(t) + _, target := testBackendWithHandler(t, func(w http.ResponseWriter, r *http.Request) {}) + + options := defaultServiceOptions + options.AllowIPs = []string{"10.0.0.0/8"} + options.ClientIPHeader = "CF-Connecting-IP" + + err := router.DeployService("service1", []string{target}, defaultEmptyReaders, + options, defaultTargetOptions, defaultDeploymentOptions) + + require.ErrorIs(t, err, ErrServiceOptionsInvalid) + require.ErrorContains(t, err, "requires trusted-proxy") +} + +func TestIPAllowList_RejectionRendersCustomErrorPage(t *testing.T) { + pagesDir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(pagesDir, "403.html"), []byte("

not from here

"), 0644)) + + options := defaultServiceOptions + options.ErrorPagePath = pagesDir + + handler := testRestrictedService(t, options, func(w http.ResponseWriter, r *http.Request) {}) + + resp := testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + + assert.Equal(t, http.StatusForbidden, resp.StatusCode) + assert.Contains(t, testAuthBody(t, resp), "not from here") +} + +func TestIPAllowList_RejectionSurvivesInterceptErrors(t *testing.T) { + options := defaultServiceOptions + options.InterceptErrorStatuses = []int{403, 503} + + handler := testRestrictedService(t, options, func(w http.ResponseWriter, r *http.Request) {}) + + resp := testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + + assert.Equal(t, http.StatusForbidden, resp.StatusCode) +} + +func TestIPAllowList_StateWrittenBeforeTheOptionStaysUnrestricted(t *testing.T) { + state := ` + { + "name": "my-app", + "hosts": ["app.example.com"], + "active_target": "localhost:3000", + "options": {}, + "target_options": { + "health_check_config": {"path": "/up", "interval": 1000000000, "timeout": 5000000000}, + "response_timeout": 30000000000 + }, + "pause_controller": {"state": 0, "stop_message": "", "fail_after": 0}, + "rollout_controller": null + } + ` + + var service Service + require.NoError(t, json.NewDecoder(strings.NewReader(state)).Decode(&service)) + t.Cleanup(service.Dispose) + + // An upgrade must never silently black-hole an existing service. + assert.Empty(t, service.options.AllowIPs) + assert.Nil(t, service.allowedIPs) +} + +func TestIPAllowList_EmptyStoredListStaysUnrestricted(t *testing.T) { + state := ` + { + "name": "my-app", + "active_target": "localhost:3000", + "options": {"allow_ips": []}, + "target_options": { + "health_check_config": {"path": "/up", "interval": 1000000000, "timeout": 5000000000}, + "response_timeout": 30000000000 + }, + "pause_controller": {"state": 0, "stop_message": "", "fail_after": 0}, + "rollout_controller": null + } + ` + + var service Service + require.NoError(t, json.NewDecoder(strings.NewReader(state)).Decode(&service)) + t.Cleanup(service.Dispose) + + // An empty list means "no filter", not "deny everyone" -- otherwise a gem + // emitting an empty array would take every service down. + assert.Nil(t, service.allowedIPs) +} + +func TestIPAllowList_UnreadableStoredEntryFailsClosed(t *testing.T) { + state := ` + { + "name": "my-app", + "active_target": "localhost:3000", + "options": {"allow_ips": ["not-an-ip"]}, + "target_options": { + "health_check_config": {"path": "/up", "interval": 1000000000, "timeout": 5000000000}, + "response_timeout": 30000000000 + }, + "pause_controller": {"state": 0, "stop_message": "", "fail_after": 0}, + "rollout_controller": null + } + ` + + // Decoding must succeed, or one bad entry takes down every other service in + // the state file. + var service Service + require.NoError(t, json.NewDecoder(strings.NewReader(state)).Decode(&service)) + t.Cleanup(service.Dispose) + + require.NotNil(t, service.allowedIPs) + assert.False(t, service.allowedIPs.permits(parseHostAddr(testAllowedPeer))) + assert.False(t, service.allowedIPs.permits(parseHostAddr(testDeniedPeer))) +} + +func TestIPAllowList_SurvivesStateRoundTrip(t *testing.T) { + options := defaultServiceOptions + options.AllowIPs = []string{"10.0.0.0/8"} + options.TrustedProxies = []string{"172.16.0.0/12"} + + service := testCreateService(t, options, defaultTargetOptions) + t.Cleanup(service.Dispose) + + encoded, err := json.Marshal(service) + require.NoError(t, err) + + var restored Service + require.NoError(t, json.Unmarshal(encoded, &restored)) + t.Cleanup(restored.Dispose) + + require.NotNil(t, restored.allowedIPs) + assert.True(t, restored.allowedIPs.permits(parseHostAddr(testAllowedPeer))) + assert.False(t, restored.allowedIPs.permits(parseHostAddr(testDeniedPeer))) + assert.Equal(t, []string{"172.16.0.0/12"}, restored.options.TrustedProxies) +} + +func TestIPAllowList_RedeployWithoutTheFlagRemovesRestriction(t *testing.T) { + router := testRouter(t) + _, target := testBackendWithHandler(t, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("ok")) + }) + + restricted := defaultServiceOptions + restricted.AllowIPs = []string{"10.0.0.0/8"} + + require.NoError(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, + restricted, defaultTargetOptions, defaultDeploymentOptions)) + + handler := testRoutedHandler(t, router) + resp := testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + require.NoError(t, router.DeployService("service1", []string{target}, defaultEmptyReaders, + defaultServiceOptions, defaultTargetOptions, defaultDeploymentOptions)) + + resp = testAuthRequest(handler, testRequestFromPeer(testDeniedPeer, "http://example.com/")) + assert.Equal(t, http.StatusOK, resp.StatusCode) +} diff --git a/internal/server/ip_allow_list_test.go b/internal/server/ip_allow_list_test.go new file mode 100644 index 00000000..8c596a6e --- /dev/null +++ b/internal/server/ip_allow_list_test.go @@ -0,0 +1,456 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "net/netip" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func testAllowList(t testing.TB, allow, trusted []string, clientIPHeader string) *ipAllowList { + t.Helper() + + list, err := newIPAllowList(allow, trusted, clientIPHeader) + require.NoError(t, err) + require.NotNil(t, list) + + return list +} + +func testRequestFrom(peer string, headers ...string) *http.Request { + req, _ := http.NewRequest(http.MethodGet, "http://example.com/", nil) + req.RemoteAddr = peer + + for i := 0; i+1 < len(headers); i += 2 { + req.Header.Add(headers[i], headers[i+1]) + } + + return req +} + +func TestIPAllowList_ParseAcceptsAddressesAndPrefixes(t *testing.T) { + tests := []struct { + name string + entry string + expected string + }{ + {"v4 prefix", "10.0.0.0/8", "10.0.0.0/8"}, + {"bare v4 becomes a /32", "192.168.1.7", "192.168.1.7/32"}, + {"v6 prefix", "2001:db8::/32", "2001:db8::/32"}, + {"bare v6 becomes a /128", "::1", "::1/128"}, + {"host bits are masked off", "10.1.2.3/8", "10.0.0.0/8"}, + {"surrounding whitespace is ignored", " 10.0.0.0/8 ", "10.0.0.0/8"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prefix, err := parseIPPrefix(tt.entry) + require.NoError(t, err) + assert.Equal(t, tt.expected, prefix.String()) + }) + } +} + +func TestIPAllowList_ParseRejectsMalformedEntries(t *testing.T) { + tests := []struct { + name string + entry string + }{ + {"empty", ""}, + {"not an address", "not-an-ip"}, + {"prefix out of range", "10.0.0.0/33"}, + {"v4-mapped prefix", "::ffff:10.0.0.0/104"}, + {"v4-mapped bare address", "::ffff:10.0.0.1"}, + {"zoned prefix", "fe80::1%eth0/64"}, + // ParseAddr accepts this and keeps the zone, but a zoned address matches + // nothing, so storing it would silently do nothing. + {"zoned bare address", "fe80::1%eth0"}, + {"host name", "example.com"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := parseIPPrefix(tt.entry) + require.Error(t, err) + }) + } +} + +func TestIPAllowList_MatchesNormalizedAddresses(t *testing.T) { + tests := []struct { + name string + allow []string + addr string + expected bool + }{ + {"plain v4 inside", []string{"203.0.113.0/24"}, "203.0.113.5", true}, + {"plain v4 outside", []string{"203.0.113.0/24"}, "198.51.100.5", false}, + // The classic bypass: netip.Prefix.Contains reports false for a v4-mapped + // v6 address against a v4 prefix unless it is unmapped first. + {"v4-mapped v6 inside", []string{"203.0.113.0/24"}, "::ffff:203.0.113.5", true}, + {"v4-mapped v6 private", []string{"10.0.0.0/8"}, "::ffff:10.1.2.3", true}, + {"v4-mapped v6 outside", []string{"203.0.113.0/24"}, "::ffff:198.51.100.5", false}, + {"v6 inside", []string{"2001:db8::/32"}, "2001:db8::1", true}, + {"zoned v6 still matches", []string{"fe80::/10"}, "fe80::1%eth0", true}, + {"v6 loopback is not v4 loopback", []string{"127.0.0.0/8"}, "::1", false}, + {"any-v4 does not swallow v6", []string{"0.0.0.0/0"}, "2001:db8::1", false}, + {"multiple ranges, second matches", []string{"10.0.0.0/8", "203.0.113.0/24"}, "203.0.113.9", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + list := testAllowList(t, tt.allow, nil, "") + addr, err := netip.ParseAddr(tt.addr) + require.NoError(t, err) + + assert.Equal(t, tt.expected, list.permits(normalizeAddr(addr))) + }) + } +} + +func TestIPAllowList_ZeroAddressMatchesNothing(t *testing.T) { + list := testAllowList(t, []string{"0.0.0.0/0", "::/0"}, nil, "") + + // An unparseable peer resolves to the zero Addr; it must never be permitted, + // even by a default route. + assert.False(t, list.permits(netip.Addr{})) +} + +func TestIPAllowList_IgnoresForwardedHeadersWithoutTrustedProxy(t *testing.T) { + // The anti-regression test for the whole feature: with no --trusted-proxy, + // no header can influence the decision. + list := testAllowList(t, []string{"10.0.0.0/8"}, nil, "") + + tests := []struct { + name string + headers []string + }{ + {"no headers", nil}, + {"forged X-Forwarded-For", []string{"X-Forwarded-For", "10.0.0.1"}}, + {"forged X-Real-IP", []string{"X-Real-IP", "10.0.0.1"}}, + {"forged Forwarded", []string{"Forwarded", "for=10.0.0.1"}}, + {"forged CF-Connecting-IP", []string{"CF-Connecting-IP", "10.0.0.1"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := testRequestFrom("203.0.113.9:44321", tt.headers...) + assert.False(t, list.permits(list.clientAddr(req))) + }) + } +} + +func TestIPAllowList_IgnoresForwardedHeaderFromUntrustedPeer(t *testing.T) { + list := testAllowList(t, []string{"10.0.0.0/8"}, []string{"172.16.0.0/12"}, "") + + // The peer is not one of our proxies, so its claim carries no weight. + req := testRequestFrom("203.0.113.9:44321", "X-Forwarded-For", "10.0.0.1") + assert.False(t, list.permits(list.clientAddr(req))) + + // Including when the peer arrives over IPv6 and the trusted list is v4-only. + req = testRequestFrom("[2001:db8::9]:44321", "X-Forwarded-For", "10.0.0.1") + assert.False(t, list.permits(list.clientAddr(req))) +} + +func TestIPAllowList_IgnoresForgedFirstForwardedHeaderLine(t *testing.T) { + // http.Header.Get returns only the FIRST of repeated header lines. A hop that + // appends its own line (HAProxy's option forwardfor does exactly this) leaves + // the client's forged line first, so reading only that line hands the walk to + // the attacker. + list := testAllowList(t, []string{"10.0.0.0/8"}, []string{"172.16.0.0/12"}, "") + + req := testRequestFrom("172.16.5.9:44321", + "X-Forwarded-For", "10.0.0.1", // forged by the client + "X-Forwarded-For", "203.0.113.9", // appended by the real hop + ) + + assert.Equal(t, "203.0.113.9", list.clientAddr(req).String()) + assert.False(t, list.permits(list.clientAddr(req))) +} + +func TestIPAllowList_ResolvesThroughTrustedProxy(t *testing.T) { + tests := []struct { + name string + trusted []string + peer string + headers []string + expected string + }{ + { + name: "single hop", + trusted: []string{"172.16.0.0/12"}, + peer: "172.16.5.9:44321", + headers: []string{"X-Forwarded-For", "10.0.0.1"}, + expected: "10.0.0.1", + }, + { + name: "two hops, both trusted, client is leftmost", + trusted: []string{"172.16.0.0/12", "192.0.2.0/24"}, + peer: "172.16.5.9:44321", + headers: []string{"X-Forwarded-For", "10.0.0.1, 192.0.2.7"}, + expected: "10.0.0.1", + }, + { + name: "untrusted entry to the right wins over one further left", + trusted: []string{"172.16.0.0/12"}, + peer: "172.16.5.9:44321", + headers: []string{"X-Forwarded-For", "10.0.0.1, 203.0.113.9"}, + expected: "203.0.113.9", + }, + { + name: "entries carrying ports", + trusted: []string{"172.16.0.0/12"}, + peer: "172.16.5.9:44321", + headers: []string{"X-Forwarded-For", "10.0.0.1:8080"}, + expected: "10.0.0.1", + }, + { + name: "bracketed v6 entry carrying a port", + trusted: []string{"172.16.0.0/12"}, + peer: "172.16.5.9:44321", + headers: []string{"X-Forwarded-For", "[2001:db8::1]:443"}, + expected: "2001:db8::1", + }, + { + name: "whitespace around entries", + trusted: []string{"172.16.0.0/12"}, + peer: "172.16.5.9:44321", + headers: []string{"X-Forwarded-For", " 10.0.0.1 , 172.16.5.9 "}, + expected: "10.0.0.1", + }, + { + name: "three hops across two header lines", + trusted: []string{"172.16.0.0/12", "192.0.2.0/24"}, + peer: "172.16.5.9:44321", + headers: []string{ + "X-Forwarded-For", "10.0.0.1", + "X-Forwarded-For", "192.0.2.7", + }, + expected: "10.0.0.1", + }, + { + name: "v4-mapped entry is unmapped", + trusted: []string{"172.16.0.0/12"}, + peer: "172.16.5.9:44321", + headers: []string{"X-Forwarded-For", "::ffff:10.0.0.1"}, + expected: "10.0.0.1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + list := testAllowList(t, []string{"10.0.0.0/8"}, tt.trusted, "") + req := testRequestFrom(tt.peer, tt.headers...) + + assert.Equal(t, tt.expected, list.clientAddr(req).String()) + }) + } +} + +func TestIPAllowList_DeniesUnresolvableForwardedChain(t *testing.T) { + // Falling back to the peer here would be a bypass whenever the allow list + // contains the proxy's own range, so an unresolvable chain denies. + tests := []struct { + name string + headers []string + }{ + {"header absent", nil}, + {"header empty", []string{"X-Forwarded-For", ""}}, + {"chain entirely trusted", []string{"X-Forwarded-For", "172.16.5.9"}}, + {"junk nearest hop", []string{"X-Forwarded-For", "10.0.0.1, evil"}}, + {"all junk", []string{"X-Forwarded-For", "evil, worse"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + list := testAllowList(t, []string{"10.0.0.0/8", "172.16.0.0/12"}, []string{"172.16.0.0/12"}, "") + req := testRequestFrom("172.16.5.9:44321", tt.headers...) + + assert.False(t, list.clientAddr(req).IsValid()) + assert.False(t, list.permits(list.clientAddr(req))) + }) + } +} + +func TestIPAllowList_ReadsTheOperatorNamedClientIPHeader(t *testing.T) { + // ClientIPMiddleware overwrites X-Forwarded-For from the named header, so the + // allow list reads the named header itself rather than the value that + // middleware left behind. + list := testAllowList(t, []string{"10.0.0.0/8"}, []string{"172.16.0.0/12"}, "CF-Connecting-IP") + + req := testRequestFrom("172.16.5.9:44321", + "CF-Connecting-IP", "10.0.0.1", + "X-Forwarded-For", "203.0.113.9", + ) + + assert.Equal(t, "10.0.0.1", list.clientAddr(req).String()) + assert.True(t, list.permits(list.clientAddr(req))) +} + +func TestIPAllowList_DeniesUnparseablePeer(t *testing.T) { + list := testAllowList(t, []string{"10.0.0.0/8"}, nil, "") + + for _, peer := range []string{"", "/tmp/kamal-proxy.sock", "garbage"} { + t.Run(peer, func(t *testing.T) { + req := testRequestFrom(peer) + assert.False(t, list.permits(list.clientAddr(req))) + }) + } +} + +func TestIPAllowList_ChainLengthIsBounded(t *testing.T) { + list := testAllowList(t, []string{"10.0.0.0/8"}, []string{"172.16.0.0/12"}, "") + + // The client is the leftmost entry, with more trusted hops appended to its + // right than we are willing to walk back through. + chain := "10.0.0.1" + for range forwardedChainLimit + 5 { + chain += ", 172.16.5.9" + } + + req := testRequestFrom("172.16.5.9:44321", "X-Forwarded-For", chain) + + // The client entry sits beyond the hop limit, so the chain is unresolvable + // rather than walked indefinitely. + assert.False(t, list.clientAddr(req).IsValid()) +} + +func TestServiceOptions_ValidateAllowIPs(t *testing.T) { + tests := []struct { + name string + allowIPs []string + trustedProxies []string + clientIPHeader string + expectedError string + }{ + {name: "no options"}, + {name: "allow only", allowIPs: []string{"10.0.0.0/8"}}, + {name: "allow with trusted proxy", allowIPs: []string{"10.0.0.0/8"}, trustedProxies: []string{"172.16.0.0/12"}}, + { + name: "allow with client ip header and trusted proxy", + allowIPs: []string{"10.0.0.0/8"}, + trustedProxies: []string{"172.16.0.0/12"}, + clientIPHeader: "CF-Connecting-IP", + }, + { + name: "malformed allow entry", + allowIPs: []string{"nonsense"}, + expectedError: "allow-ip", + }, + { + name: "malformed trusted entry", + allowIPs: []string{"10.0.0.0/8"}, + trustedProxies: []string{"nonsense"}, + expectedError: "trusted-proxy", + }, + { + name: "trusted proxy without an allow list", + trustedProxies: []string{"172.16.0.0/12"}, + expectedError: "trusted-proxy requires allow-ip", + }, + { + name: "a default route cannot be trusted", + allowIPs: []string{"10.0.0.0/8"}, + trustedProxies: []string{"0.0.0.0/0"}, + expectedError: "trusted-proxy cannot contain a default route", + }, + { + name: "a v6 default route cannot be trusted either", + allowIPs: []string{"10.0.0.0/8"}, + trustedProxies: []string{"::/0"}, + expectedError: "trusted-proxy cannot contain a default route", + }, + { + name: "client ip header without trusted proxy is rejected", + allowIPs: []string{"10.0.0.0/8"}, + clientIPHeader: "CF-Connecting-IP", + expectedError: "allow-ip with client-ip-header requires trusted-proxy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + options := defaultServiceOptions + options.AllowIPs = tt.allowIPs + options.TrustedProxies = tt.trustedProxies + options.ClientIPHeader = tt.clientIPHeader + + err := options.Validate() + + if tt.expectedError == "" { + require.NoError(t, err) + return + } + + require.ErrorIs(t, err, ErrServiceOptionsInvalid) + require.ErrorContains(t, err, tt.expectedError) + }) + } +} + +func BenchmarkIPAllowList_Permits(b *testing.B) { + list, err := newIPAllowList([]string{ + "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "203.0.113.0/24", + "198.51.100.0/24", "2001:db8::/32", "fd00::/8", "100.64.0.0/10", + }, nil, "") + if err != nil { + b.Fatal(err) + } + + req := testRequestFrom("203.0.113.9:44321") + + b.ReportAllocs() + for b.Loop() { + list.permits(list.clientAddr(req)) + } +} + +func TestMetricsAllowList(t *testing.T) { + backend := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte("metrics")) + }) + + tests := []struct { + name string + allowed []string + peer string + expectedStatus int + }{ + {"no list serves everyone", nil, testDeniedPeer, http.StatusOK}, + {"matching peer", []string{"10.0.0.0/8"}, testAllowedPeer, http.StatusOK}, + {"non-matching peer", []string{"10.0.0.0/8"}, testDeniedPeer, http.StatusForbidden}, + {"v4-mapped v6 peer still matches", []string{"10.0.0.0/8"}, "[::ffff:10.0.0.5]:44321", http.StatusOK}, + {"unparseable peer", []string{"10.0.0.0/8"}, "garbage", http.StatusForbidden}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + prefixes, err := parseIPPrefixes(tt.allowed, "metrics-allow-ip") + require.NoError(t, err) + + handler := withMetricsAllowList(prefixes, backend) + + req := testRequestFrom(tt.peer) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + assert.Equal(t, tt.expectedStatus, w.Result().StatusCode) + }) + } +} + +func TestMetricsAllowList_InvalidEntryIsRejectedEvenWhenMetricsAreOff(t *testing.T) { + config := testConfig(t) + config.MetricsPort = 0 + config.MetricsAllowIPs = []string{"not-an-ip"} + + server := NewServer(config, testRouter(t)) + + err := server.startMetricsServer() + + require.ErrorIs(t, err, ErrServiceOptionsInvalid) + require.ErrorContains(t, err, "metrics-allow-ip") +} diff --git a/internal/server/router.go b/internal/server/router.go index 3076461e..9fb20258 100644 --- a/internal/server/router.go +++ b/internal/server/router.go @@ -275,6 +275,10 @@ func (r *Router) DeployService(name string, targetURLs, readerURLs []string, opt return err } + if err := validateAllowIPsHealthCheck(options, targetOptions); err != nil { + return err + } + options.Normalize() slog.Info("Deploying", "service", name, "targets", targetURLs, "hosts", options.Hosts, "paths", options.PathPrefixes, "tls", options.TLSEnabled) diff --git a/internal/server/server.go b/internal/server/server.go index 381da866..07bfd80f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -268,13 +268,20 @@ func (s *Server) startHTTPServers() error { } func (s *Server) startMetricsServer() error { + // Parsed before the disabled check, so a typo fails the boot rather than + // waiting until someone turns metrics on. + allowed, err := parseIPPrefixes(s.config.MetricsAllowIPs, "metrics-allow-ip") + if err != nil { + return err + } + if s.config.MetricsPort == 0 { slog.Debug("Metrics server disabled") return nil } addr := fmt.Sprintf("%s:%d", s.config.Bind, s.config.MetricsPort) - handler := metrics.Enable() + handler := withMetricsAllowList(allowed, metrics.Enable()) l, err := s.listen("tcp", addr) if err != nil { diff --git a/internal/server/service.go b/internal/server/service.go index 7ae32d28..6ec43149 100644 --- a/internal/server/service.go +++ b/internal/server/service.go @@ -133,6 +133,14 @@ type ServiceOptions struct { // credential crosses the RPC socket or reaches the state file. Empty (the // default) leaves the service open. BasicAuth string `json:"basic_auth,omitempty"` + + // AllowIPs restricts this service to the given addresses and CIDR ranges. + // Empty (the default) serves everyone. See ip_allow_list.go for which + // address is matched and why. + AllowIPs []string `json:"allow_ips,omitempty"` + // TrustedProxies names the proxies in front of this one, allowing AllowIPs + // to be matched against the forwarded chain rather than the connecting peer. + TrustedProxies []string `json:"trusted_proxies,omitempty"` } func (so *ServiceOptions) ShouldExcludeMetrics(r *http.Request) bool { @@ -195,59 +203,11 @@ func (so ServiceOptions) Validate() error { return err } - return so.validateDynamicDomains() -} - -func (so ServiceOptions) validateInterceptErrorStatuses() error { - for _, status := range so.InterceptErrorStatuses { - if status < 400 || status > 599 { - return fmt.Errorf("%w: intercept-errors must be a 4xx or 5xx status code, got %d", ErrServiceOptionsInvalid, status) - } - } - - return nil -} - -func (so ServiceOptions) validateDynamicDomains() error { - if so.TLSDomainsSource == "" { - if so.TLSDomainsBatchSize != 0 { - return fmt.Errorf("%w: tls-domains-batch-size requires tls-domains-source", ErrServiceOptionsInvalid) - } - if so.TLSDomainsInterval != 0 { - return fmt.Errorf("%w: tls-domains-interval requires tls-domains-source", ErrServiceOptionsInvalid) - } - return nil - } - - if !so.TLSEnabled { - return fmt.Errorf("%w: tls-domains-source requires TLS to be enabled", ErrServiceOptionsInvalid) - } - - // Both provision certificates for hosts that aren't known at deploy time, but - // through different managers -- only one of them can serve the handshake. - if so.TLSOnDemandURL != "" { - return fmt.Errorf("%w: tls-domains-source cannot be combined with tls-on-demand-url", ErrServiceOptionsInvalid) - } - - // Dynamic domains are routed via the host-less catch-all binding; a - // host-scoped service would issue certificates that can never be served. - if so.HasConfiguredHosts() { - return fmt.Errorf("%w: tls-domains-source requires the service to be the catch-all (no --host)", ErrServiceOptionsInvalid) - } - - if !validDomainSource(so.TLSDomainsSource) { - return fmt.Errorf("%w: tls-domains-source must be a path or an http(s) URL: %q", ErrServiceOptionsInvalid, so.TLSDomainsSource) - } - - if so.TLSDomainsBatchSize < 0 || so.TLSDomainsBatchSize > MaxTLSDomainsBatchSize { - return fmt.Errorf("%w: tls-domains-batch-size must be between 1 and %d", ErrServiceOptionsInvalid, MaxTLSDomainsBatchSize) - } - - if so.TLSDomainsInterval != 0 && so.TLSDomainsInterval < MinTLSDomainsInterval { - return fmt.Errorf("%w: tls-domains-interval must be at least %s", ErrServiceOptionsInvalid, MinTLSDomainsInterval) + if err := so.validateAllowIPs(); err != nil { + return err } - return nil + return so.validateDynamicDomains() } func (so *ServiceOptions) WithHosts(hosts []string) ServiceOptions { @@ -295,6 +255,7 @@ type Service struct { certManager CertManager middleware http.Handler basicAuth *basicAuthCredential + allowedIPs *ipAllowList } func NewService(name string, options ServiceOptions, targetOptions TargetOptions, sanCertManager *SANCertManager) (*Service, error) { @@ -537,6 +498,7 @@ func (s *Service) initialize(options ServiceOptions, targetOptions TargetOptions s.certManager = certManager s.middleware = middleware s.basicAuth = s.resolveBasicAuth(options) + s.allowedIPs = s.resolveIPAllowList(options) return nil } @@ -687,6 +649,10 @@ func (s *Service) createMiddleware(options ServiceOptions, targetOptions TargetO func (s *Service) serviceRequestWithTarget(w http.ResponseWriter, r *http.Request) { LoggingRequestContext(r).Service = s.name + if s.rejectDisallowedIP(w, r) { + return + } + if !s.options.TLSEnabled && r.TLS != nil { SetErrorResponse(w, r, http.StatusServiceUnavailable, nil) return diff --git a/internal/server/service_options_validation.go b/internal/server/service_options_validation.go new file mode 100644 index 00000000..9a73f63b --- /dev/null +++ b/internal/server/service_options_validation.go @@ -0,0 +1,59 @@ +package server + +import "fmt" + +// Validators for the fork-only ServiceOptions fields, kept out of service.go so +// that file stays under the size ceiling and the upstream merge surface stays +// localized. + +func (so ServiceOptions) validateInterceptErrorStatuses() error { + for _, status := range so.InterceptErrorStatuses { + if status < 400 || status > 599 { + return fmt.Errorf("%w: intercept-errors must be a 4xx or 5xx status code, got %d", ErrServiceOptionsInvalid, status) + } + } + + return nil +} + +func (so ServiceOptions) validateDynamicDomains() error { + if so.TLSDomainsSource == "" { + if so.TLSDomainsBatchSize != 0 { + return fmt.Errorf("%w: tls-domains-batch-size requires tls-domains-source", ErrServiceOptionsInvalid) + } + if so.TLSDomainsInterval != 0 { + return fmt.Errorf("%w: tls-domains-interval requires tls-domains-source", ErrServiceOptionsInvalid) + } + return nil + } + + if !so.TLSEnabled { + return fmt.Errorf("%w: tls-domains-source requires TLS to be enabled", ErrServiceOptionsInvalid) + } + + // Both provision certificates for hosts that aren't known at deploy time, but + // through different managers -- only one of them can serve the handshake. + if so.TLSOnDemandURL != "" { + return fmt.Errorf("%w: tls-domains-source cannot be combined with tls-on-demand-url", ErrServiceOptionsInvalid) + } + + // Dynamic domains are routed via the host-less catch-all binding; a + // host-scoped service would issue certificates that can never be served. + if so.HasConfiguredHosts() { + return fmt.Errorf("%w: tls-domains-source requires the service to be the catch-all (no --host)", ErrServiceOptionsInvalid) + } + + if !validDomainSource(so.TLSDomainsSource) { + return fmt.Errorf("%w: tls-domains-source must be a path or an http(s) URL: %q", ErrServiceOptionsInvalid, so.TLSDomainsSource) + } + + if so.TLSDomainsBatchSize < 0 || so.TLSDomainsBatchSize > MaxTLSDomainsBatchSize { + return fmt.Errorf("%w: tls-domains-batch-size must be between 1 and %d", ErrServiceOptionsInvalid, MaxTLSDomainsBatchSize) + } + + if so.TLSDomainsInterval != 0 && so.TLSDomainsInterval < MinTLSDomainsInterval { + return fmt.Errorf("%w: tls-domains-interval must be at least %s", ErrServiceOptionsInvalid, MinTLSDomainsInterval) + } + + return nil +} From eb4273ffe80abb043452b245121aa7ae12c8511b Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Wed, 29 Jul 2026 00:32:15 +0200 Subject: [PATCH 2/2] fix(ip-allow-list): flatten the forwarded chain with append staticcheck S1011. Same behavior, and CI is the only place golangci-lint runs -- it is not installed locally. --- internal/server/ip_allow_list.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/server/ip_allow_list.go b/internal/server/ip_allow_list.go index f49e8a44..e8480c39 100644 --- a/internal/server/ip_allow_list.go +++ b/internal/server/ip_allow_list.go @@ -108,9 +108,7 @@ func (l *ipAllowList) forwardedAddr(r *http.Request) netip.Addr { entries := []string{} for _, value := range r.Header.Values(header) { - for _, entry := range strings.Split(value, ",") { - entries = append(entries, entry) - } + entries = append(entries, strings.Split(value, ",")...) } if len(entries) > forwardedChainLimit {