diff --git a/README.md b/README.md index ba79f23..399a3ad 100644 --- a/README.md +++ b/README.md @@ -340,9 +340,9 @@ minute, failed logins included; beyond that it answers `429 Too Many Requests` without checking the password. A Prometheus server scraping every 15 seconds sends 4 a minute. IPv6 addresses in one /64 count as one client. When the request comes from a private or loopback address, such as a reverse proxy's, -the client address is taken from the `X-Real-IP` or `X-Forwarded-For` header -the proxy sets; a proxy that sets neither makes all its clients share one -allowance. +the client address is taken from the `X-Real-IP` header the proxy sets, or else +from `X-Forwarded-For`, as the last address in it that is not private or +loopback. A proxy that sets neither makes all its clients share one allowance. ### Example `.env` diff --git a/TODO.md b/TODO.md index 0b048af..507ac20 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,8 @@ https://git.eeqj.de/sneak/dnswatcher/issues/104 # Completed Steps +- 2026-10-01: the client address from `X-Forwarded-For` is the last entry that + is not a trusted proxy, not the first, which the client sets (closes #181). - 2026-10-01: `/metrics` allows each client address 30 requests a minute, counted before Basic Auth, and answers 429 beyond that (closes #101). - 2026-10-01: the image built by `make docker` reports the `git describe` diff --git a/internal/middleware/export_test.go b/internal/middleware/export_test.go index 1abde0f..93680e8 100644 --- a/internal/middleware/export_test.go +++ b/internal/middleware/export_test.go @@ -1,6 +1,9 @@ package middleware -import "time" +import ( + "net/http" + "time" +) // The /metrics rate limit, exported so the tests can count requests // against it. @@ -8,3 +11,9 @@ const ( MetricsRequestLimit = metricsRequestLimit MetricsRequestWindow time.Duration = metricsRequestWindow ) + +// RealIP is realIP, exported so the tests can check which address it +// takes as the client's. +func RealIP(r *http.Request) string { + return realIP(r) +} diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index f65f63f..e558b0d 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -209,6 +209,12 @@ func isTrustedProxy(ip net.IP) bool { // realIP extracts the client's real IP address from the request. // Proxy headers are only trusted from RFC1918/loopback addresses. +// +// Each proxy adds to the end of X-Forwarded-For the address it got the +// request from, so the client can write every entry before the one the +// first trusted proxy added. The client address is therefore the +// rightmost entry that is not a trusted proxy, or the leftmost entry +// when they all are. func realIP(r *http.Request) string { addr := ipFromHostPort(r.RemoteAddr) remoteIP := net.ParseIP(addr) @@ -223,16 +229,26 @@ func realIP(r *http.Request) string { return ip } - if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - if parts := strings.SplitN( - xff, ",", 2, //nolint:mnd - ); len(parts) > 0 { - if ip := strings.TrimSpace(parts[0]); ip != "" { - return ip - } + // A proxy may add its entry as a header line of its own instead of + // appending to the line the client sent, so all lines form one list. + entries := strings.Split( + strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",", + ) + client := strings.TrimSpace(entries[0]) + + for i := len(entries) - 1; i > 0; i-- { + entry := strings.TrimSpace(entries[i]) + if !isTrustedProxy(net.ParseIP(entry)) { + client = entry + + break } } + if client != "" { + return client + } + return addr } diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index c7c1a8c..e6676c4 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -482,3 +482,99 @@ func TestMetricsRateLimitKeysOnClientAddress(t *testing.T) { }) } } + +// TestRealIP checks which address realIP takes as the client's. Each +// element of forwardedFor is sent as an X-Forwarded-For header line of +// its own, and 198.51.100.9 is always an entry the client wrote itself. +func TestRealIP(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + remoteAddr string + xRealIP string + forwardedFor []string + want string + }{ + { + "untrusted peer, both headers ignored", + "198.51.100.1:4000", + "203.0.113.1", + []string{"203.0.113.2"}, + "198.51.100.1", + }, + { + "X-Real-IP from a trusted proxy wins", + "10.0.0.1:4000", + "203.0.113.1", + []string{"203.0.113.2"}, + "203.0.113.1", + }, + { + "client's own entry, then the one the proxy added", + "10.0.0.1:4000", + "", + []string{"198.51.100.9, 203.0.113.1"}, + "203.0.113.1", + }, + { + "several trusted proxies", + "10.0.0.1:4000", + "", + []string{"198.51.100.9, 203.0.113.1, 10.0.0.3, 10.0.0.2"}, + "203.0.113.1", + }, + { + "proxy adds a header line of its own", + "10.0.0.1:4000", + "", + []string{"198.51.100.9", "203.0.113.1"}, + "203.0.113.1", + }, + { + "every entry a trusted proxy", + "10.0.0.1:4000", + "", + []string{"10.0.0.3, 10.0.0.2"}, + "10.0.0.3", + }, + { + "empty where the client address belongs", + "10.0.0.1:4000", + "", + []string{"203.0.113.1, , 10.0.0.2"}, + "10.0.0.1", + }, + { + "no headers from a trusted proxy", + "10.0.0.1:4000", + "", + nil, + "10.0.0.1", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) + req.RemoteAddr = tt.remoteAddr + + if tt.xRealIP != "" { + req.Header.Set("X-Real-IP", tt.xRealIP) + } + + for _, line := range tt.forwardedFor { + req.Header.Add("X-Forwarded-For", line) + } + + got := middleware.RealIP(req) + if got != tt.want { + t.Errorf("realIP = %q, want %q", got, tt.want) + } + }) + } +}