diff --git a/README.md b/README.md index 320b0f7..7956481 100644 --- a/README.md +++ b/README.md @@ -199,23 +199,25 @@ image, `make run` builds it and runs it on port 8080. | `DATA_DIR` | `./data/reports` | Directory for compressed reports | | `DATA_DIR_MAX_BYTES` | `1073741824` (1 GiB) | Most bytes of report files kept in `DATA_DIR`, oldest deleted first; see [Report limits](#report-limits) | | `DEBUG` | `false` | Enable debug logging | -| `TRUSTED_PROXIES` | loopback + RFC1918 | Comma-separated CIDRs whose `X-Forwarded-For` / `X-Real-IP` headers are trusted for client IP resolution | +| `TRUSTED_PROXIES` | RFC 1918 ranges | Comma-separated CIDRs whose `X-Forwarded-For` / `X-Real-IP` headers are trusted for client IP resolution | | `REPORTS_PER_MINUTE` | `60` | Reports each client address may send a minute; see [Report limits](#report-limits) | | `CORS_ALLOWED_ORIGINS` | empty | Comma-separated origins whose pages may call the API; see [CORS](#cors) | | `METRICS_USERNAME` | empty | Basic auth user name for `/metrics`; see [Metrics](#metrics) | | `METRICS_PASSWORD` | empty | Basic auth password for `/metrics`; see [Metrics](#metrics) | | `SENTRY_DSN` | empty | DSN of the Sentry project to send errors to; see [Sentry](#sentry) | -`TRUSTED_PROXIES` defaults to -`127.0.0.1/32,::1/128,10.0.0.0/8,172.16.0.0/12,192.168.0.0/16`. The loopback -entries cover a reverse proxy on the same host. A request whose direct peer is -outside this set has its forwarded headers ignored, and the direct peer is -logged and rate-limited instead. The container image does not use this default; -see [Deployment](#deployment). +`TRUSTED_PROXIES` defaults to `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16`. A list +replaces the default; set and empty, the server trusts no proxy. Loopback is not +in the default, so a reverse proxy on the same host has to be named, as in +`TRUSTED_PROXIES=127.0.0.1/32`. A request whose direct peer is outside this set +has its forwarded headers ignored, and the direct peer is logged and +rate-limited instead. The container image does not use this default; see +[Deployment](#deployment). A variable set to a value the server cannot use, such as `PORT=abc`, `DEBUG=maybe` or a `BIND_ADDRESS` that is not an IP address, stops it from -starting, with an error naming the variable. An empty variable counts as unset. +starting, with an error naming the variable. An empty variable counts as unset, +except `TRUSTED_PROXIES`. #### Report storage diff --git a/TODO.md b/TODO.md index 158bfd6..de845ba 100644 --- a/TODO.md +++ b/TODO.md @@ -21,6 +21,14 @@ Run `make frontend-viewport-test` in CI as its own step; it is not part of # Completed Steps +- 2026-10-14: unset, the backend's own `TRUSTED_PROXIES` trusts `10.0.0.0/8`, + `172.16.0.0/12` and `192.168.0.0/16`, and no longer loopback; a list replaces + that; set and empty, it trusts no proxy, where it used to get the default, as + viper reads an empty environment variable as unset + ([#129](https://git.eeqj.de/sneak/netwatch/issues/129)). A value it cannot + parse still stops the start, and the image still starts the backend with + `TRUSTED_PROXIES=127.0.0.1/32`. Tests load the config with it unset, empty and + set to a list - 2026-10-14: while report files cannot be written, `POST /api/v1/reports` answers 500, and storing recovers once writing works again, with no restart ([#123](https://git.eeqj.de/sneak/netwatch/issues/123)). A failed write puts diff --git a/internal/config/config.go b/internal/config/config.go index f92d0f1..69cca58 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -9,6 +9,7 @@ import ( "math" "net/netip" "net/url" + "os" "strconv" "strings" @@ -21,13 +22,11 @@ import ( ) // defaultTrustedProxies lists the networks whose forwarded -// headers are honoured by default: IPv4 and IPv6 loopback, -// for a reverse proxy on the same host, and the RFC1918 -// ranges. The container image does not use it: +// headers are honoured when TRUSTED_PROXIES is unset: the +// RFC 1918 ranges. The container image does not use it: // bin/entrypoint.sh gives the server 127.0.0.1/32, since // nginx is its only client there. -const defaultTrustedProxies = "127.0.0.1/32,::1/128," + - "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" +const defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" // Default limits on stored reports; README.md gives the reasons for // these values. @@ -135,6 +134,14 @@ func New( viper.GetString("DEBUG"), errNotBool) } + // An empty TRUSTED_PROXIES trusts no proxy, but viper reads an + // empty environment variable as unset and would give the default, + // so the environment is read first. + trustedProxies, set := os.LookupEnv("TRUSTED_PROXIES") + if !set { + trustedProxies = viper.GetString("TRUSTED_PROXIES") + } + s := &Config{ BindAddress: viper.GetString("BIND_ADDRESS"), CORSAllowedOrigins: splitList(viper.GetString("CORS_ALLOWED_ORIGINS")), @@ -146,7 +153,7 @@ func New( Port: port, ReportsPerMinute: viper.GetInt("REPORTS_PER_MINUTE"), SentryDSN: viper.GetString("SENTRY_DSN"), - TrustedProxies: splitList(viper.GetString("TRUSTED_PROXIES")), + TrustedProxies: splitList(trustedProxies), log: log, params: ¶ms, } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a6a42e5..7de411a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,8 @@ package config_test import ( + "os" + "slices" "strings" "testing" @@ -29,6 +31,62 @@ func requireConfigError(t *testing.T, setting string) { } } +// requireTrustedProxies builds the config as main does and fails the +// test unless its TrustedProxies are want. +func requireTrustedProxies(t *testing.T, want []string) { + t.Helper() + + var cfg *config.Config + + app := fx.New( + fx.NopLogger, + fx.Provide(globals.New, logger.New, config.New), + fx.Populate(&cfg), + ) + + err := app.Err() + if err != nil { + t.Fatalf("config error = %v", err) + } + + if !slices.Equal(cfg.TrustedProxies, want) { + t.Fatalf("TrustedProxies = %q, want %q", cfg.TrustedProxies, want) + } +} + +// TestTrustedProxiesUnsetTrustsRFC1918: unset, the server trusts the +// RFC 1918 ranges and nothing else, loopback included. +func TestTrustedProxiesUnsetTrustsRFC1918(t *testing.T) { + // t.Setenv first, so the variable is put back as it was after the + // test. + t.Setenv("TRUSTED_PROXIES", "") + + err := os.Unsetenv("TRUSTED_PROXIES") + if err != nil { + t.Fatal(err) + } + + requireTrustedProxies(t, + []string{"10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"}) +} + +// TestTrustedProxiesEmptyTrustsNothing: set and empty, the server +// trusts no proxy. viper alone would read the empty variable as unset +// and give the default. +func TestTrustedProxiesEmptyTrustsNothing(t *testing.T) { + t.Setenv("TRUSTED_PROXIES", "") + + requireTrustedProxies(t, []string{}) +} + +// TestTrustedProxiesListReplacesDefault: a list is used as given, with +// none of the default ranges added. +func TestTrustedProxiesListReplacesDefault(t *testing.T) { + t.Setenv("TRUSTED_PROXIES", "172.17.0.1/32, 2001:db8::/32") + + requireTrustedProxies(t, []string{"172.17.0.1/32", "2001:db8::/32"}) +} + // TestSettingsLoadAsGiven: valid values pass the checks and are used // as given. bin/entrypoint.sh starts the server with these // BIND_ADDRESS and PORT values.