feat(service): restrict a service by client address with --allow-ip

[mhenrixon]

Summary

kamal-proxy deploy service1 --target web-1:3000 --allow-ip 10.0.0.0/8,203.0.113.7

Requests from outside the list get 403. Metrics get their own list: kamal-proxy run --metrics-port 9090 --metrics-allow-ip 10.0.0.0/8. Closes #9.

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 makes the list decorative — anyone sends X-Forwarded-For: 10.0.0.1 and walks in.

The anchor this issue and ROADMAP.md both named is the trap. logging_middleware.go:70 computes two values: client_addr from r.RemoteAddr (trustworthy) and remote_addr from r.Header.Get("X-Forwarded-For") — unconditionally, with no trust check (logging_middleware.go:77). Reusing the one called remote_addr would have shipped a bypass. Both the issue's framing and the ROADMAP row are corrected in this PR.

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 — that fallback is a bypass whenever the allow list contains the proxy's own range, which is a very plausible config.

Three bypasses closed that a straightforward implementation ships

Each verified by hand against go1.26.5 before writing code:

Bypass	Evidence
Repeated header lines. Header.Get returns only the first line. HAProxy's option forwardfor appends a new line rather than folding, so a client that sends its own header owns the entire right-to-left walk.	h.Add("X-Forwarded-For","10.0.0.1"); h.Add(...,"203.0.113.9") → Get() == "10.0.0.1". Reads use Values.
IPv4-mapped IPv6. Connecting over IPv6 bypasses a v4 list.	netip.Prefix("203.0.113.0/24").Contains(::ffff:203.0.113.5) is false; .Unmap() makes it true.
Zoned addresses. fe80::1%eth0 parses fine but matches nothing, so storing one silently does nothing.	ParseAddr keeps the zone; fe80::/10.Contains() of it is false. Rejected at parse time.

Placement

Not a createMiddleware middleware. That chain includes the certificate manager's handler, so filtering there blocks ACME HTTP-01 and breaks renewal 60–90 days later — a failure that never shows up at deploy. 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. This is the deliberate inverse of the basic-auth ordering from #46, and both directions are pinned by tests.

Allow-only, no --deny-ip

The issue title says "allow/deny" but its body describes two allow lists. Ordered allow/deny semantics depend on argument order, which is a footgun in config a gem regenerates; Traefik shipped IPAllowList with no deny counterpart; and a static deny list is the wrong tool for the abuse-blocking people reach for it with. Adding it later is additive.

Also in this PR

validateInterceptErrorStatuses and validateDynamicDomains move to service_options_validation.go. service.go was at 790 of the hard 800-line ceiling in .claude/rules/coding-style.md, which says to split before adding rather than grow. It now sits at 756. Both are fork-only validators, so moving them also keeps upstream merge surface localized.

Test plan

• make test, go vet ./..., gofmt -l internal/ cmd/ clean
• go test -race ./internal/server/ ./internal/cmd/ — 766 tests, clean
• make build + --help shows all three flags
• BenchmarkIPAllowList_Permits — 51 ns/op, 0 B/op, 0 allocs/op (8-entry list, resolve + match). No before/after claimed: this is a new path that did not exist on dash, and it is skipped entirely when no list is set.

Three mutations applied and confirmed to fail their tests, so these prove behavior rather than plumbing:

Mutation	Caught by
Values() reverted to Get()	TestIPAllowList_IgnoresForgedFirstForwardedHeaderLine
Unmap() dropped	TestIPAllowList_MatchesNormalizedAddresses (2 cases) + TestMetricsAllowList
Unresolvable chain falls back to the peer	TestIPAllowList_DeniesUnresolvableForwardedChain (5 cases)

Known gaps, documented not fixed

1. Under some Docker port drivers r.RemoteAddr is the bridge gateway. Rootless Docker or a userland-proxy driver collapses every source to e.g. 172.17.0.1, and the list then allows everyone or no one. Unspoofable is not the same as meaningful. README tells operators to check client_addr in the access log.
2. --client-ip-header names a header the edge must overwrite or strip. The combination now requires --trusted-proxy (hard error otherwise), but a header the edge merely passes through — True-Client-IP on non-Enterprise Cloudflare is the canonical trap — can still be set by anyone. Documented.
3. The health check path is unauthenticated by address, so a downstream LB can still drain the proxy during a deploy. Bounded by rejecting a health check path of / when a list is set. Same shape as basic auth.
4. s.allowedIPs is read without a lock, exactly as s.basicAuth and s.options already are, so a redeploy that adds a list has a brief window where in-flight requests see the old value. Inherited, not introduced — I am not claiming -race covers it, because no test drives a redeploy concurrently with traffic. Worth its own issue alongside the same problem for basic auth.
5. Rolling the proxy image back silently removes the restriction. Same class as basic auth's documented hazard.
6. ClientIPMiddleware and the remote_addr log field remain unconditionally header-trusting. Changing them is a behavior change in an upstream file and belongs in its own PR — but it means that on a spoofed request the access log's remote_addr and the filter's decision disagree.

Deviations & judgment calls

Design came from a multi-agent pass and three adversarial reviews. The reviews found a blocker and six other real problems, all folded in before any code was written.

• Header.Values() joined, not Header.Get() — the blocker, found independently by two of the three reviewers, and reproduced by hand before I trusted it. Without this the trusted-proxy walk runs entirely inside attacker-authored data.
• The allow list reads the operator-named --client-ip-header directly, not the X-Forwarded-For that ClientIPMiddleware rewrote. That middleware does r.Header.Set("X-Forwarded-For", ...) from an unvalidated header and runs outermost, so by the time the check runs the genuine chain is gone, replaced by one attacker-settable value. Reading the original named header keeps the middleware's outbound rewrite from deciding access.
• --allow-ip + --client-ip-header without --trusted-proxy is a hard deploy error — otherwise the combination looks like it works while silently matching the edge.
• Added a rate-limited denial log (reusing the in-tree token_bucket.go) carrying the peer, the resolved address, and a reason. The design declined one; the review pointed out that its own worst failure mode — a trusted edge that stops sending the header, denying everything — is then completely invisible, since the access log shows the edge as client_addr and a chain that looks fine as remote_addr.
• A second IPv6 warning for the trusted list, not just the allow list. The likeliest dual-stack lockout is an IPv6 client arriving via an IPv6 edge that was never listed as trusted.
• Metrics allow list parses before the MetricsPort == 0 early return, so a typo fails the boot even with metrics off. The design put it after, where it could never run, while claiming it "fails the boot".
• Rejected one reviewer suggestion: they proposed also suppressing header trust in a way that would have broken the two-tier CDN→LB topology. Instead --trusted-proxy documents that every hop must be listed, with the failure mode spelled out in the README.
• My own chain-limit test was built backwards and failed on first run: I put the client as the rightmost entry, which is the nearest hop, so it resolved immediately and the bound never applied. Rebuilt with the client leftmost.
• Empty-but-present list means no filter, same as absent — an empty list meaning "deny everyone" would turn a gem emitting [] into a total outage. Absent fails open (the operator never asked for a filter); an unreadable stored entry fails closed for that one service (they did ask, and we cannot honour it) without aborting the whole state decode.
• Metrics shipped in this PR rather than deferred. The open question raised whether --bind on a private interface is the more idiomatic answer; it is, but it is a larger change (Config.Bind currently governs all three listeners), and the issue explicitly asks for metrics by CIDR. Happy to file the --bind follow-up.