From 8833603eff5495f0775293e4d753f09f352be887 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 08:55:47 +0200 Subject: [PATCH] nginx: trust X-Forwarded-For only from TRUSTED_PROXIES (closes #64) nginx trusted X-Forwarded-For from every RFC1918 address, so a client reaching it from one could write a new address on each request and get a fresh rate-limit allowance. The container's TRUSTED_PROXIES now names the reverse proxies nginx trusts, none by default. bin/entrypoint.sh makes each entry a CIDR, checks it with the new "netwatch-server check-cidr", which runs the server's own TRUSTED_PROXIES parsing, and writes one set_real_ip_from line per entry into /etc/nginx/trusted-proxies.conf, which nginx.conf includes. The backend is started with TRUSTED_PROXIES=127.0.0.1/32, since nginx is its only client. The viewport test mounts an empty file there. Model: opus-5-5 --- README.md | 18 ++++++--- TODO.md | 9 +++++ backend/README.md | 22 ++++++++--- backend/cmd/netwatch-server/main.go | 16 ++++++++ backend/internal/config/config.go | 9 +++-- backend/internal/middleware/export_test.go | 4 -- backend/internal/middleware/middleware.go | 10 +++-- .../internal/middleware/middleware_test.go | 23 +++++++++-- bin/entrypoint.sh | 39 +++++++++++++++++-- nginx.conf | 10 +++-- script/frontend-viewport-test | 3 ++ 11 files changed, 129 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index 89e5c5c..2f46400 100644 --- a/README.md +++ b/README.md @@ -184,8 +184,8 @@ container: nginx serves the built frontend and passes `/api/` and only inside the container, on `127.0.0.1:8081`. The image: - Listens on port 8080 by default (override with `PORT` env var) -- Trusts `X-Forwarded-For` from RFC1918 reverse proxies (10/8, 172.16/12, - 192.168/16) +- Takes the client address from `X-Forwarded-For` only on requests from the + reverse proxies named in `TRUSTED_PROXIES`, and by default from none - Sends access logs to stdout - Caches static assets with immutable headers - Stores reports in `DATA_DIR`, `/data/reports` by default, on the `/data` @@ -224,10 +224,16 @@ What the [upaas](https://git.eeqj.de/sneak/upaas) app for netwatch needs: - `DEBUG`, default `false`: debug logging - `DATA_DIR`, default `/data/reports`: leave unset; reports kept outside `/data` do not survive a redeploy - - `TRUSTED_PROXIES`, default loopback and RFC1918: leave unset. The - backend's only client is nginx, on loopback, which passes on the client - address; nginx takes it from `X-Forwarded-For` only from RFC1918 - addresses. + - `TRUSTED_PROXIES`, default empty: set it to the address the reverse proxy + in front of the container connects from, as an IP address or CIDR; several + are separated by commas. nginx takes the client address from + `X-Forwarded-For` only on a request from one of them, and the rate limit + counts that address. Unset, `X-Forwarded-For` is ignored and every client + behind the proxy shares the proxy's one allowance of `REPORTS_PER_MINUTE`. + Name only addresses nothing but the proxy connects from: any client that + connects from one can write its own `X-Forwarded-For`, and through a port + Docker publishes, every client may connect from the Docker network's + gateway, such as `172.17.0.1`. - **Health check:** the image's `HEALTHCHECK` requests `/.well-known/healthcheck` through nginx every 30 seconds, so it fails unless both nginx and the backend answer. upaas reads the container's health 60 diff --git a/TODO.md b/TODO.md index da6fdcd..552f191 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,15 @@ latest run passes. # Completed Steps +- 2026-09-29: nginx takes the client address from `X-Forwarded-For` only on + requests from the reverse proxies named in the container's `TRUSTED_PROXIES` + (issue #64), and by default from none, where it trusted every RFC1918 address + before, so a client could write a new address on each request and escape the + rate limit. `bin/entrypoint.sh` writes one `set_real_ip_from` line per entry + into `/etc/nginx/trusted-proxies.conf`, which `nginx.conf` includes, refusing + an entry that is not an IP address or CIDR, as `netwatch-server check-cidr` + finds; it starts the backend with `TRUSTED_PROXIES=127.0.0.1/32`, since nginx + is its only client - 2026-09-29: report file names can no longer collide (issue #61): each is `reports--.jsonl.zst`, where the number goes up by one for each file the server starts to write, so two flushes in the same millisecond, diff --git a/backend/README.md b/backend/README.md index 1cb73ca..5f80ef2 100644 --- a/backend/README.md +++ b/backend/README.md @@ -87,9 +87,10 @@ Internal packages in `internal/` follow standard Go project layout: | `CORS_ALLOWED_ORIGINS` | empty | Comma-separated origins whose pages may call the API; see [CORS](#cors) | `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 the reverse proxy that shares the container; the -RFC1918 ranges match `nginx.conf`. A request whose direct peer is outside this -set has its forwarded headers ignored, and the direct peer is logged instead. +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 [Container image](#container-image). 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 @@ -101,8 +102,19 @@ The root `Dockerfile` builds one image in which nginx listens on the public port 8080, serves the frontend, and proxies `/api/` and `/.well-known/healthcheck` to this server. The image's entrypoint, `bin/entrypoint.sh`, starts the server as user `netwatch` (uid 1000) with `BIND_ADDRESS=127.0.0.1` and `PORT=8081`, so -only nginx reaches it. `DATA_DIR` is `/data/reports`, on the `/data` volume, -which `netwatch` owns. +only nginx reaches it, and with `TRUSTED_PROXIES=127.0.0.1/32`, so it takes the +client address nginx passes on and no other. `DATA_DIR` is `/data/reports`, on +the `/data` volume, which `netwatch` owns. + +The container's own `TRUSTED_PROXIES` goes to nginx instead: IP addresses or +CIDRs, separated by commas, of the reverse proxies in front of the container. +nginx takes the client address from `X-Forwarded-For` only on a request from one +of them. Unset or empty, nginx trusts no proxy, and the client address is the +one each request comes from, so every client behind a proxy shares one rate +limit. An entry that is not an IP address or CIDR, such as a hostname or +`1.2.3`, stops the container at start with an error naming `TRUSTED_PROXIES`: +the entrypoint checks each entry with `netwatch-server check-cidr`, which parses +it as this server parses its own `TRUSTED_PROXIES`. ### Report storage diff --git a/backend/cmd/netwatch-server/main.go b/backend/cmd/netwatch-server/main.go index 3f3d86b..01c77a4 100644 --- a/backend/cmd/netwatch-server/main.go +++ b/backend/cmd/netwatch-server/main.go @@ -2,6 +2,9 @@ package main import ( + "fmt" + "os" + "sneak.berlin/go/netwatch/internal/config" "sneak.berlin/go/netwatch/internal/globals" "sneak.berlin/go/netwatch/internal/handlers" @@ -22,6 +25,19 @@ var ( ) func main() { + // "netwatch-server check-cidr CIDR" exits 1, with the error, if + // this server would refuse CIDR in its TRUSTED_PROXIES. + // bin/entrypoint.sh runs it on each entry it gives nginx. + if len(os.Args) == 3 && os.Args[1] == "check-cidr" { + _, err := middleware.ParseTrustedProxies(os.Args[2:]) + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } + + return + } + globals.Appname = Appname globals.Version = Version globals.Buildarch = Buildarch diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index c8236d0..a258978 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -21,10 +21,11 @@ import ( ) // defaultTrustedProxies lists the networks whose forwarded -// headers are honoured by default. It covers the RFC1918 -// ranges (to match nginx.conf) plus IPv4 and IPv6 loopback, -// because the reverse proxy shares the container and reaches -// the backend over loopback. +// 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: +// 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" diff --git a/backend/internal/middleware/export_test.go b/backend/internal/middleware/export_test.go index 22e1e7e..ed47271 100644 --- a/backend/internal/middleware/export_test.go +++ b/backend/internal/middleware/export_test.go @@ -29,7 +29,3 @@ func ClientIP( ) string { return clientIP(remoteAddr, header, trusted) } - -func ParseTrustedProxies(cidrs []string) ([]netip.Prefix, error) { - return parseTrustedProxies(cidrs) -} diff --git a/backend/internal/middleware/middleware.go b/backend/internal/middleware/middleware.go index d59ecdf..a962c85 100644 --- a/backend/internal/middleware/middleware.go +++ b/backend/internal/middleware/middleware.go @@ -64,7 +64,7 @@ func New( _ fx.Lifecycle, params Params, ) (*Middleware, error) { - trusted, err := parseTrustedProxies(params.Config.TrustedProxies) + trusted, err := ParseTrustedProxies(params.Config.TrustedProxies) if err != nil { return nil, err } @@ -77,9 +77,11 @@ func New( return s, nil } -// parseTrustedProxies converts the TRUSTED_PROXIES entries into -// prefixes, failing fast on any malformed entry. -func parseTrustedProxies(cidrs []string) ([]netip.Prefix, error) { +// ParseTrustedProxies converts the TRUSTED_PROXIES entries into +// prefixes, failing fast on any malformed entry. Each entry must be +// a CIDR; a lone address is refused. "netwatch-server check-cidr" +// runs it too. +func ParseTrustedProxies(cidrs []string) ([]netip.Prefix, error) { prefixes := make([]netip.Prefix, 0, len(cidrs)) for _, cidr := range cidrs { diff --git a/backend/internal/middleware/middleware_test.go b/backend/internal/middleware/middleware_test.go index 559c308..30e7cbe 100644 --- a/backend/internal/middleware/middleware_test.go +++ b/backend/internal/middleware/middleware_test.go @@ -36,15 +36,32 @@ func mustPrefixes(t *testing.T, cidrs ...string) []netip.Prefix { return prefixes } +// TestParseTrustedProxiesRejectsMalformed includes entries nginx would +// read as another address or look up as a hostname, in the CIDR form +// bin/entrypoint.sh gives "netwatch-server check-cidr". func TestParseTrustedProxiesRejectsMalformed(t *testing.T) { t.Parallel() - _, err := middleware.ParseTrustedProxies([]string{"not-a-cidr"}) - if err == nil || !strings.Contains(err.Error(), "TRUSTED_PROXIES") { - t.Fatalf("error = %v, want one naming TRUSTED_PROXIES", err) + for _, cidr := range []string{ + "not-a-cidr", "10.0.0.1", "1.2.3/32", "172.30/32", "10/32", + "cafe/32", "999.1.1.1/32", "10.0.0.0/33", "::1/129", + "fe80::1%eth0/128", + } { + _, err := middleware.ParseTrustedProxies([]string{cidr}) + if err == nil || !strings.Contains(err.Error(), "TRUSTED_PROXIES") { + t.Errorf("%q: error = %v, want one naming TRUSTED_PROXIES", + cidr, err) + } } } +func TestParseTrustedProxiesAcceptsCIDRs(t *testing.T) { + t.Parallel() + + mustPrefixes(t, "172.17.0.1/32", "10.0.0.0/8", "2001:db8::1/128", + "2001:db8::/32", "::ffff:192.0.2.1/128") +} + type clientIPCase struct { name string remoteAddr string diff --git a/bin/entrypoint.sh b/bin/entrypoint.sh index e0d4fb3..4458f9e 100755 --- a/bin/entrypoint.sh +++ b/bin/entrypoint.sh @@ -32,16 +32,47 @@ if [ "$PORT" -eq 8081 ]; then exit 1 fi +# TRUSTED_PROXIES names the reverse proxies in front of the container, +# as IP addresses or CIDRs separated by commas. nginx takes the client +# address from X-Forwarded-For only on a request from one of them, so +# unset or empty, it trusts no one. nginx.conf includes the file written +# here, one set_real_ip_from line per entry. +# +# nginx looks up an entry it cannot read as an address as a hostname, +# and trusts what it finds (1.2.3 is found as 1.2.0.3). So each entry +# is made a CIDR, a lone address getting /128 if it is IPv6 and /32 if +# not, and netwatch-server checks it with the parsing it gives its own +# TRUSTED_PROXIES. Its error, naming the CIDR, is dropped for the one +# below, naming the entry as written. set -f keeps a * in an entry from +# becoming a list of file names. +TRUSTED_PROXIES="${TRUSTED_PROXIES:-}" +set -f +for proxy in $(printf '%s' "$TRUSTED_PROXIES" | tr ',' ' '); do + case "$proxy" in + */*) cidr="$proxy" ;; + *:*) cidr="$proxy/128" ;; + *) cidr="$proxy/32" ;; + esac + if ! netwatch-server check-cidr "$cidr" 2> /dev/null; then + echo "entrypoint: TRUSTED_PROXIES must be IP addresses or CIDRs" \ + "separated by commas; '$proxy' is neither" >&2 + exit 1 + fi + echo "set_real_ip_from $cidr;" +done > /etc/nginx/trusted-proxies.conf + # A stop signal is only noted here; the loop below acts on it. stop_requested="" trap 'stop_requested=yes' TERM INT # netwatch-server runs as the netwatch user and listens on loopback # only, on a port other than the public one; nginx.conf proxies to this -# address. The netwatch user has no login shell, hence -s /bin/sh. -# busybox su replaces itself with the command instead of staying on as -# its parent, so $! is the server's own PID. -BIND_ADDRESS=127.0.0.1 PORT=8081 \ +# address. Its only client is nginx, so it takes the client address +# nginx passes on from 127.0.0.1 alone, whatever TRUSTED_PROXIES the +# container has. The netwatch user has no login shell, hence -s +# /bin/sh. busybox su replaces itself with the command instead of +# staying on as its parent, so $! is the server's own PID. +BIND_ADDRESS=127.0.0.1 PORT=8081 TRUSTED_PROXIES=127.0.0.1/32 \ su -s /bin/sh netwatch -c 'exec netwatch-server' & backend=$! diff --git a/nginx.conf b/nginx.conf index 697340b..0322c27 100644 --- a/nginx.conf +++ b/nginx.conf @@ -11,10 +11,12 @@ server { root /usr/share/nginx/html; index index.html; - # Trust RFC1918 reverse proxies for X-Forwarded-For - set_real_ip_from 10.0.0.0/8; - set_real_ip_from 172.16.0.0/12; - set_real_ip_from 192.168.0.0/16; + # The client address comes from X-Forwarded-For only on a request + # from the reverse proxies in TRUSTED_PROXIES: bin/entrypoint.sh + # writes one set_real_ip_from line for each into this file, and + # leaves it empty when TRUSTED_PROXIES is unset, so that by default + # the client address is the one each request comes from. + include /etc/nginx/trusted-proxies.conf; real_ip_header X-Forwarded-For; real_ip_recursive on; diff --git a/script/frontend-viewport-test b/script/frontend-viewport-test index 5a97c99..e4b53d1 100755 --- a/script/frontend-viewport-test +++ b/script/frontend-viewport-test @@ -63,11 +63,14 @@ main() { # nginx.conf is a template: the image renders it over its own # default.conf, with the same port and limit bin/entrypoint.sh uses. + # The empty file it includes trusts no proxy, as bin/entrypoint.sh + # writes it when TRUSTED_PROXIES is unset. docker run -d --rm --name "$SERVER" \ --network "$NETWORK" --network-alias netwatch \ -e PORT=8080 -e NGINX_ENVSUBST_FILTER='^PORT$' \ -v "$ROOT/dist:/usr/share/nginx/html:ro" \ -v "$ROOT/nginx.conf:/etc/nginx/templates/default.conf.template:ro" \ + -v /dev/null:/etc/nginx/trusted-proxies.conf:ro \ "$SERVER_IMAGE" > /dev/null # The image's own entrypoint already exposes CDP on 9222 and passes