diff --git a/README.md b/README.md index 0365448..de6445b 100644 --- a/README.md +++ b/README.md @@ -347,9 +347,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. **`DNSWATCHER_DNS_INTERVAL` and `DNSWATCHER_TLS_INTERVAL`** take a positive duration: a number followed by a unit such as `s`, `m` or `h`, for example diff --git a/TODO.md b/TODO.md index 3fae221..c38b103 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ nameserver IP address changes: https://git.eeqj.de/sneak/dnswatcher/issues/105 # 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: a nameserver that does not answer is saved as `error` with the reason, and NS failure and NS recovery are notified (closes #104). - 2026-10-01: a `DNSWATCHER_DNS_INTERVAL` or `DNSWATCHER_TLS_INTERVAL` that is 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..9e306a9 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -342,9 +342,9 @@ func TestDashboardRendersWithSecurityHeaders(t *testing.T) { } } -// Addresses for the rate limit tests: a client connecting directly, a -// trusted proxy, and a client behind that proxy as its X-Real-IP -// header names it. +// Addresses for the rate limit and realIP tests: a client connecting +// directly, a trusted proxy, and a client behind that proxy as the +// proxy's X-Real-IP or X-Forwarded-For header names it. const ( directClient = "198.51.100.1:4000" trustedProxy = "10.0.0.1:4000" @@ -482,3 +482,84 @@ 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", + directClient, proxiedClient, []string{"198.51.100.9"}, + "198.51.100.1", + }, + { + "X-Real-IP from a trusted proxy wins", + trustedProxy, proxiedClient, []string{"203.0.113.8"}, + proxiedClient, + }, + { + "client's own entry, then the one the proxy added", + trustedProxy, "", []string{"198.51.100.9, 203.0.113.1"}, + proxiedClient, + }, + { + "several trusted proxies", + trustedProxy, "", + []string{"198.51.100.9, 203.0.113.1, 10.0.0.3, 10.0.0.2"}, + proxiedClient, + }, + { + "proxy adds a header line of its own", + trustedProxy, "", []string{"198.51.100.9", proxiedClient}, + proxiedClient, + }, + { + "every entry a trusted proxy", + trustedProxy, "", []string{"10.0.0.3, 10.0.0.2"}, + "10.0.0.3", + }, + { + "empty where the client address belongs", + trustedProxy, "", []string{"203.0.113.1, , 10.0.0.2"}, + "10.0.0.1", + }, + { + "no headers from a trusted proxy", + trustedProxy, "", 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) + } + }) + } +}