diff --git a/README.md b/README.md index 86226a3..b067a9e 100644 --- a/README.md +++ b/README.md @@ -469,19 +469,29 @@ Configuration is handled via environment variables and OS-specific paths: | `XDG_DATA_HOME` | `/var/lib` (in the Docker image) | Base of the state directory; must be an absolute path | | `GOMEMLIMIT` | `2560MiB` (in the Docker image) | Go soft memory limit; see Memory | | `MALLOC_ARENA_MAX` | `2` (in the Docker image) | glibc malloc arena cap, a positive whole number; see Memory | -| `TRUSTED_PROXIES` | (empty) | Addresses and CIDR ranges of trusted proxies; see below | +| `TRUSTED_PROXIES` | the RFC 1918 ranges | Addresses and CIDR ranges of trusted proxies; see below | A variable that is set to an invalid value stops the start with an error and a -non-zero exit. An empty variable counts as unset. +non-zero exit. An empty variable counts as unset. `TRUSTED_PROXIES` is the one +exception: set but empty, it makes the server trust no proxy. -`TRUSTED_PROXIES` lists the reverse proxies whose `X-Forwarded-For` the server -believes, comma-separated, such as `10.0.0.0/8,192.0.2.1`. For a request from -one of them, the request log and `/ip/` use the last address in -`X-Forwarded-For` that is not a trusted proxy: each proxy adds the address it -received the request from to the end, so the entries before it are whatever the -client sent. For a request from any other address they use the address the -request came from, whatever forwarding headers it carries. `X-Real-IP` and -`True-Client-IP` are not read. +`TRUSTED_PROXIES` names the reverse proxies whose `X-Forwarded-For` the server +believes: + +- When it is not set, the server trusts the private ranges of RFC 1918: + - `10.0.0.0/8` + - `172.16.0.0/12` + - `192.168.0.0/16` +- When it lists addresses and CIDR ranges, comma-separated, such as + `10.0.0.0/8,192.0.2.1`, the server trusts those instead. +- When it is set but empty (`TRUSTED_PROXIES=`), the server trusts no proxy. + +For a request from a trusted proxy, the request log and `/ip/` use the last +address in `X-Forwarded-For` that is not a trusted proxy. Each proxy adds the +address it received the request from to the end, so the entries before it are +whatever the client sent. For a request from any other address they use the +address the request came from, whatever forwarding headers it carries. +`X-Real-IP` and `True-Client-IP` are not read. State directory (the database and the blocklist copies): @@ -575,12 +585,11 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app needs: - Container port: `8080`. - Volume: one, at container path `/var/lib/berlin.sneak.app.routewatch`. -- Environment: `TRUSTED_PROXIES`, set to the address the reverse proxy in front - of the app connects from, as the container sees it; without it the request log - and `/ip/` give the proxy's address for every request. Nothing else is needed: - leave `XDG_DATA_HOME`, `GOMEMLIMIT` and `MALLOC_ARENA_MAX` at the image's - values. `DEBUG=routewatch` is optional and adds the `System stats` memory line - to the log. +- Environment: nothing. The server trusts a reverse proxy at a private address + without `TRUSTED_PROXIES` (see Configuration). Set it only when the proxy in + front of the app connects from a public address. Leave `XDG_DATA_HOME`, + `GOMEMLIMIT` and `MALLOC_ARENA_MAX` at the image's values. `DEBUG=routewatch` + is optional and adds the `System stats` memory line to the log. - Memory Limit: `5g`, the 5 GiB limit from Memory above. upaas sets no swap limit, so on a host with swap Docker allows the same amount of swap again. - Health check: the image's `HEALTHCHECK` requests diff --git a/TODO.md b/TODO.md index b63666d..c29bf0f 100644 --- a/TODO.md +++ b/TODO.md @@ -35,6 +35,11 @@ tested. # Completed Steps +- 2026-10-27: with `TRUSTED_PROXIES` unset, the server trusts reverse proxies at + the private addresses of RFC 1918, so `/ip/`, the home page's `curl` card and + the request log give the client a proxy names in `X-Forwarded-For`. They used + to give the proxy's address. A list in `TRUSTED_PROXIES` replaces the default, + and `TRUSTED_PROXIES` set but empty trusts no proxy (closes #198) - 2026-10-27: the home page has cards with figures of the routing table: the newest and oldest kept route, the newest and oldest updated AS, the AS with the most prefixes, the ASes with a single prefix, and fifteen well-known diff --git a/internal/config/config.go b/internal/config/config.go index 4fde480..d83f2e1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -30,6 +30,14 @@ const ( maxPort = 65535 ) +// The private address ranges of RFC 1918. The HTTP server trusts a reverse +// proxy in any of them when TRUSTED_PROXIES is not set. +const ( + privateRange10 = "10.0.0.0/8" + privateRange172 = "172.16.0.0/12" + privateRange192 = "192.168.0.0/16" +) + var ( errRelativeXDGDataHome = errors.New("XDG_DATA_HOME must be an absolute path") errUnsupportedOS = errors.New("unsupported operating system") @@ -159,11 +167,21 @@ func getPort() (int, error) { return int(port), nil } -// getTrustedProxies returns the addresses and CIDR ranges listed in -// TRUSTED_PROXIES, such as "10.0.0.0/8,192.0.2.1", or none when it is not set. -// An address is returned as a range holding only that address. +// getTrustedProxies returns the reverse proxies the HTTP server trusts. When +// TRUSTED_PROXIES is not set, they are the private ranges of RFC 1918. When it +// is set, they are the addresses and CIDR ranges it lists, such as +// "10.0.0.0/8,192.0.2.1", and none when it is empty. An address is returned as +// a range holding only that address. func getTrustedProxies() ([]netip.Prefix, error) { - value := os.Getenv("TRUSTED_PROXIES") + value, set := os.LookupEnv("TRUSTED_PROXIES") + if !set { + return []netip.Prefix{ + netip.MustParsePrefix(privateRange10), + netip.MustParsePrefix(privateRange172), + netip.MustParsePrefix(privateRange192), + }, nil + } + if value == "" { return nil, nil } diff --git a/internal/config/config_internal_test.go b/internal/config/config_internal_test.go index 4ca4386..7380d40 100644 --- a/internal/config/config_internal_test.go +++ b/internal/config/config_internal_test.go @@ -2,6 +2,7 @@ package config import ( "net/netip" + "os" "runtime" "slices" "testing" @@ -46,25 +47,47 @@ func TestNewRefusesInvalidPort(t *testing.T) { } } -func TestNewReadsTrustedProxies(t *testing.T) { +// TestNewTrustsPrivateRangesByDefault checks that with TRUSTED_PROXIES unset +// the trusted proxies are the private ranges of RFC 1918. Before the fix no +// proxy was trusted, so a reverse proxy at a private address was taken for the +// client (https://git.eeqj.de/sneak/routewatch/issues/198). +func TestNewTrustsPrivateRangesByDefault(t *testing.T) { t.Setenv("PORT", "") t.Setenv("XDG_DATA_HOME", "") + // t.Setenv puts the variable back when the test ends. t.Setenv("TRUSTED_PROXIES", "") + err := os.Unsetenv("TRUSTED_PROXIES") + if err != nil { + t.Fatal(err) + } + cfg, err := New() if err != nil { t.Fatalf("New() without TRUSTED_PROXIES: %v", err) } - if len(cfg.TrustedProxies) != 0 { - t.Errorf("New() without TRUSTED_PROXIES: TrustedProxies = %v, want none", - cfg.TrustedProxies) + want := []netip.Prefix{ + netip.MustParsePrefix("10.0.0.0/8"), + netip.MustParsePrefix("172.16.0.0/12"), + netip.MustParsePrefix("192.168.0.0/16"), } + if !slices.Equal(cfg.TrustedProxies, want) { + t.Errorf("New() without TRUSTED_PROXIES: TrustedProxies = %v, want %v", + cfg.TrustedProxies, want) + } +} + +// TestNewReadsTrustedProxies checks that a list in TRUSTED_PROXIES replaces +// the default, and that TRUSTED_PROXIES set but empty trusts no proxy. +func TestNewReadsTrustedProxies(t *testing.T) { + t.Setenv("PORT", "") + t.Setenv("XDG_DATA_HOME", "") t.Setenv("TRUSTED_PROXIES", "10.0.0.0/8, 192.0.2.1,2001:db8::/32") - cfg, err = New() + cfg, err := New() if err != nil { t.Fatalf("New() with TRUSTED_PROXIES: %v", err) } @@ -77,6 +100,18 @@ func TestNewReadsTrustedProxies(t *testing.T) { if !slices.Equal(cfg.TrustedProxies, want) { t.Errorf("TrustedProxies = %v, want %v", cfg.TrustedProxies, want) } + + t.Setenv("TRUSTED_PROXIES", "") + + cfg, err = New() + if err != nil { + t.Fatalf("New() with TRUSTED_PROXIES set but empty: %v", err) + } + + if len(cfg.TrustedProxies) != 0 { + t.Errorf("New() with TRUSTED_PROXIES set but empty: TrustedProxies = %v, "+ + "want none", cfg.TrustedProxies) + } } func TestNewRefusesInvalidTrustedProxies(t *testing.T) { diff --git a/internal/server/middleware_internal_test.go b/internal/server/middleware_internal_test.go index fc06be0..39fa7d8 100644 --- a/internal/server/middleware_internal_test.go +++ b/internal/server/middleware_internal_test.go @@ -1,25 +1,38 @@ package server import ( + "bytes" "encoding/json" + "io" + "log/slog" + "net" "net/http" "net/http/httptest" "net/netip" + "os" "strings" "testing" "time" + + "git.eeqj.de/sneak/routewatch/internal/config" + "git.eeqj.de/sneak/routewatch/internal/database" + "git.eeqj.de/sneak/routewatch/internal/logger" + "git.eeqj.de/sneak/routewatch/internal/metrics" + "git.eeqj.de/sneak/routewatch/internal/streamer" ) // The addresses the forwarding tests use: the client a proxy names in // X-Forwarded-For, the range of trusted proxies, a peer inside it, and a peer // outside it, which lies in testPrefixV4 so that an IP lookup of it finds a -// route. +// route. reverseProxyPeer is a reverse proxy at a private address, as Docker +// gives one. const ( forwardedClient = "203.0.113.9" trustedProxyRange = "10.0.0.0/8" trustedPeer = "10.0.0.2:1234" untrustedPeerAddr = "192.0.2.1" untrustedPeer = untrustedPeerAddr + ":1234" + reverseProxyPeer = "172.19.0.14:1234" ) // TestResponsesCarrySecurityHeaders checks that a page, a JSON route and a @@ -183,6 +196,141 @@ func TestForwardedForFromUntrustedPeerIsIgnored(t *testing.T) { } } +// TestDefaultTrustedProxiesGiveTheForwardedClient builds the server from the +// configuration with TRUSTED_PROXIES unset. A request from a reverse proxy at +// a private address must be looked up by /ip/, shown on the home page's card +// and logged as the client its X-Forwarded-For names. A direct request from a +// public address with a forged X-Forwarded-For keeps its own address. Before +// the fix no proxy was trusted unless TRUSTED_PROXIES named it, so /ip/ gave +// the proxy's address (https://git.eeqj.de/sneak/routewatch/issues/198). +func TestDefaultTrustedProxiesGiveTheForwardedClient(t *testing.T) { + t.Setenv("PORT", "") + t.Setenv("XDG_DATA_HOME", "") + + // t.Setenv puts the variable back when the test ends. + t.Setenv("TRUSTED_PROXIES", "") + + err := os.Unsetenv("TRUSTED_PROXIES") + if err != nil { + t.Fatal(err) + } + + cfg, err := config.New() + if err != nil { + t.Fatalf("config.New: %v", err) + } + + var requestLog bytes.Buffer + + s := newServerWithConfig(t, cfg, &requestLog) + + tests := []struct { + name string + peer string + want string + }{ + {"through a reverse proxy", reverseProxyPeer, forwardedClient}, + {"direct from a public address", untrustedPeer, untrustedPeerAddr}, + } + + for _, tt := range tests { + requestLog.Reset() + + get := func(path string) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, path, nil) + req.RemoteAddr = tt.peer + req.Header.Set("X-Forwarded-For", forwardedClient) + + rec := httptest.NewRecorder() + s.router.ServeHTTP(rec, req) + + return rec + } + + var answer struct { + Data IPLookupResponse `json:"data"` + } + + rec := get("/ip/") + + err := json.Unmarshal(rec.Body.Bytes(), &answer) + if err != nil { + t.Fatalf("%s: decoding the answer %q: %v", tt.name, rec.Body, err) + } + + if answer.Data.Query != tt.want { + t.Errorf("%s: /ip/ looked up %q, want %q", tt.name, answer.Data.Query, tt.want) + } + + page := get("/").Body.String() + if !strings.Contains(page, `data-client-ip="`+tt.want+`"`) { + t.Errorf("%s: the home page's card does not hold %s", tt.name, tt.want) + } + + logged := loggedAddresses(requestLog.String()) + if len(logged) == 0 { + t.Errorf("%s: the request log names no address", tt.name) + } + + for _, addr := range logged { + if addr != tt.want { + t.Errorf("%s: the request log gives %s, want %s", tt.name, addr, tt.want) + } + } + } +} + +// newServerWithConfig returns a server built with cfg, on a new database, that +// reads the routes of newTestTable and logs its requests as JSON to +// requestLog. +func newServerWithConfig( + t *testing.T, cfg *config.Config, requestLog io.Writer, +) *Server { + t.Helper() + + cfg.StateDir = t.TempDir() + + db, err := database.New(cfg, logger.New()) + if err != nil { + t.Fatalf("database.New: %v", err) + } + + t.Cleanup(func() { _ = db.Close() }) + + s := New(db, newTestTable(), streamer.New(logger.New(), metrics.New()), + &logger.Logger{Logger: slog.New(slog.NewJSONHandler(requestLog, nil))}, cfg) + s.resolver = &fakeResolver{} + + return s +} + +// loggedAddresses returns the address each line of the JSON request log +// names, without the port a direct request is logged with. +func loggedAddresses(requestLog string) []string { + var addrs []string + + for line := range strings.Lines(requestLog) { + //nolint:tagliatelle // the field name is the key the request log uses + var entry struct { + RemoteAddr string `json:"remote_addr"` + } + + err := json.Unmarshal([]byte(line), &entry) + if err != nil || entry.RemoteAddr == "" { + continue + } + + addr, _, err := net.SplitHostPort(entry.RemoteAddr) + if err != nil { + addr = entry.RemoteAddr + } + + addrs = append(addrs, addr) + } + + return addrs +} + // TestRealIPMiddlewareTrustsOnlyConfiguredProxies checks which address // RealIPMiddleware gives a request: the TCP peer, unless the peer is a trusted // proxy; then the last X-Forwarded-For entry that is not a trusted proxy,