diff --git a/CHANGELOG.md b/CHANGELOG.md index b4c8bb2..ce5d0d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,23 @@ exact tag (`ghcr.io/calnode/calnode:0.1.0`) if you need stability between upgrad copied price is how a paid meeting quietly starts selling for nothing. Bookings are not copied. +- **`TRUSTED_PROXY_CIDRS`: per-IP rate limits that work behind a CDN.** Rate limits key + on the TCP peer, which is right for a directly-reachable instance and useless behind a + fronting CDN, where every visitor arrives from the same handful of addresses and shares + one bucket. List the networks you control, a fronting CDN's own ranges included, and + the client IP is taken from `X-Forwarded-For` walked right to left past those hops. + + Nothing changes if you do not set it: a header from a peer you have not listed is still + not read at all, because it is a value the client chose. Within the header the *leftmost* + entry is likewise client-chosen, so the walk stops at the rightmost address one of your + proxies actually observed, and a malformed hop ends the walk on the peer rather than + being stepped over. Repeated `X-Forwarded-For` field lines are joined in order rather + than only the first being read, so a client's own line in front of a proxy that adds a + second one cannot hide the hop that matters. Single-value vendor headers (`CF-Connecting-IP`, `X-Real-IP`, + `True-Client-IP`) are never read, from any peer: the setting names networks rather than + CDNs, and a plain reverse proxy in the list forwards whatever the client sent. + + ## [0.8.0] - 2026-09-03 ### Added diff --git a/DEPLOY.md b/DEPLOY.md index 81d15b1..3ad7cbb 100644 --- a/DEPLOY.md +++ b/DEPLOY.md @@ -35,6 +35,7 @@ This guide covers a generic Docker deploy and a step-by-step **Railway** deploy | `GOOGLE_CLIENT_ID` / `GOOGLE_CLIENT_SECRET` | no | — | Google sign-in + calendar. Can also be set in Settings → Google OAuth. | | `LITESTREAM_REPLICA_URL` | recommended | — | Enables continuous SQLite backup (see §6). | | `COOKIE_SECURE` | no | https→true | Override cookie Secure flag; defaults from `BASE_URL` scheme. | +| `TRUSTED_PROXY_CIDRS` | no | — | Comma-separated CIDRs (a bare address = one host) whose `X-Forwarded-For` is believed when keying per-IP rate limits, e.g. `10.0.0.0/8`. Include a fronting CDN's own ranges so the walk steps over its edge and lands on the visitor. Unset ⇒ the header is ignored and the limit keys on the TCP peer, so behind a CDN every visitor shares one bucket. **Only list networks you control**: anything in the list can name any client IP it likes. Single-value vendor headers (`CF-Connecting-IP`, `X-Real-IP`) are never read, from any peer. | | `LOG_LEVEL` | no | `info` | `debug`/`info`/`warn`/`error`. | ¹ Email is optional to boot, but bookings won't send confirmations until SMTP is configured (env **or** the admin UI). Precedence is **env var > DB setting > default**. diff --git a/audit/claims.yaml b/audit/claims.yaml index 28bca98..79aea67 100644 --- a/audit/claims.yaml +++ b/audit/claims.yaml @@ -276,25 +276,38 @@ claims: - id: rate-limit-keys-on-tcp-source-address claim: > - Per-IP rate limiting (internal/server/middleware.go's RateLimit) keys - strictly on the TCP-level remote address of the connection, never on client- - supplied X-Forwarded-For/X-Real-IP headers — a client cannot spoof those - headers to evade or split its rate-limit bucket. + Per-IP rate limiting (internal/server/middleware.go's RateLimit) keys on the + TCP-level remote address of the connection, and never on a client-supplied + X-Forwarded-For unless the peer that sent it is inside an operator-configured + TRUSTED_PROXY_CIDRS range — so a client cannot spoof headers to evade or split + its rate-limit bucket. Single-value vendor headers (X-Real-IP, + CF-Connecting-IP, True-Client-IP) are never read, from any peer. verify: - "internal/server/middleware.go's remoteIP — net.SplitHostPort(r.RemoteAddr) - only; the proxy headers are never read." + via peerIP, unless TrustClientIP has resolved a client IP into the request + context, which it only does for a peer matching a trusted CIDR." + - "internal/server/middleware.go's resolveClientIP — returns the peer outright + for an untrusted peer, before any header is read." - "internal/server/ratelimit_test.go's TestRemoteIP_* — assert X-Forwarded-For and X-Real-IP are ignored even when RemoteAddr is loopback." + - "internal/server/trustedproxy_test.go — asserts an untrusted peer's spoofed + headers are ignored, that the X-Forwarded-For walk goes right-to-left past + trusted hops (never the client-seeded leftmost entry), that a malformed + header falls back to the peer, and that a vendor header is ignored even from + a TRUSTED peer." status: verified caveat: > Recorded here because a prior Layer 2 audit pass flagged this as trusting spoofable proxy headers — it doesn't; the flagged behavior traced back to a stale doc comment describing the opposite of what the code does (fixed - alongside this entry). Correct behavior does require the deployment's - reverse proxy to connect to Calnode directly (or over a trusted private - network) — see the deployment docs for reverse-proxy requirements (forward - the original Host header, connect over a trusted path, strip client-supplied - proxy headers at the edge). + alongside this entry). TRUSTED_PROXY_CIDRS is empty by default, so an + unconfigured instance behaves exactly as this claim originally described. + Anything an operator does list can name any client IP it likes — that is what + trusting a proxy means — so the list must hold only networks they control. + Correct behavior otherwise requires the deployment's reverse proxy to connect + to Calnode directly (or over a trusted private network) — see the deployment + docs for reverse-proxy requirements (forward the original Host header, connect + over a trusted path, strip client-supplied proxy headers at the edge). - id: caldav-connect-self-service-no-admin-gate claim: > diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e81e86c..177b7bd 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -694,10 +694,31 @@ as the desired state: **original `Host` header**. The CSRF same-origin check (§6) compares the request's `Origin`/`Referer` against `Host`, so a proxy that rewrites Host would *false-block admin writes* (403). Fly and Railway preserve Host by default; a hand-rolled nginx - needs `proxy_set_header Host $host;`. Related: per-IP rate limits (§8) key on the - **TCP remote address** (proxy headers like `X-Forwarded-For` are intentionally - ignored as forgeable), so behind a shared proxy the limit keys on the proxy's - connection — fine for per-instance Fly/Railway, worth knowing for a fronting proxy. + needs `proxy_set_header Host $host;`. +- **Per-IP rate limits (§8) key on the TCP remote address by default**, and + `X-Forwarded-For` / `X-Real-IP` / `CF-Connecting-IP` are not read at all. That is not + an oversight: those headers are client-chosen values, so believing them unconditionally + would let anyone split their own rate-limit bucket by sending a different one each + request. Behind a shared proxy the limit therefore keys on the proxy's connection — + fine for a per-instance Fly/Railway deploy, worth knowing for a fronting CDN. + **`TRUSTED_PROXY_CIDRS`** (comma-separated CIDRs, a bare address meaning one host) + opts in per network: for a peer inside one of those ranges, `TrustClientIP` + (`internal/server/middleware.go`) resolves the client IP by walking `X-Forwarded-For` + **right to left past trusted hops** and taking the first untrusted address, else the + peer. Single-value vendor headers (`CF-Connecting-IP`, `X-Real-IP`, `True-Client-IP`) + are never consulted, even from a trusted peer: the list names *networks*, not CDNs, so + an ordinary reverse proxy in it forwards whatever the client sent, and the header + itself carries nothing that says which hop wrote it. A CDN sets `X-Forwarded-For` too + and its own ranges belong in the list, so the walk reaches the same visitor without + believing a header for a reason the code cannot check. ⛔ Not the leftmost entry: the left + of that header is whatever the original client sent, and every well-behaved proxy + preserves it. A hop that does not parse ends the walk and falls back to the peer rather + than being skipped, so one malformed entry cannot push the walk onto a value the client + chose. `X-Forwarded-For` is read as **every** field line joined in order, not just the + first, because a client's own line in front of a proxy that adds a second one would + otherwise be the only thing the walk saw. Headers from an **untrusted** peer are never read, which is what keeps the + default un-weakenable by a header. Resolution happens once, in the outermost + middleware, and is carried in the request context. --- diff --git a/internal/config/config.go b/internal/config/config.go index ac96fa0..8e19f05 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -59,6 +59,14 @@ type Config struct { // the public endpoints are rate-limited regardless. Comma-separated. EmbedAllowedOrigins []string + // TrustedProxyCIDRs lists the networks whose forwarded headers are believed when + // resolving the client IP for per-IP rate limiting. Empty (the default) ⇒ the limit + // keys on the TCP peer and X-Forwarded-For is ignored entirely, because a header + // from an unvetted peer is a client-chosen value. Comma-separated CIDRs; a bare + // address is taken as a single host. A fronting CDN's own ranges belong here: the + // walk steps over its edge address and lands on the visitor. + TrustedProxyCIDRs []string + // DemoMode turns this instance into a public, self-resetting demo: seeds sample // data on every boot (there's no persistent volume, so every boot is a fresh DB), // disables calendar/Zoom connect, serves a disallow-all robots.txt, and exposes @@ -97,6 +105,7 @@ func Load() *Config { EmbedAllowedOrigins: splitCSV(getEnv("EMBED_ALLOWED_ORIGINS", "")), DataDir: getEnv("DATA_DIR", "data"), + TrustedProxyCIDRs: splitCSV(getEnv("TRUSTED_PROXY_CIDRS", "")), } cfg.EncryptionKey = os.Getenv("CALNODE_ENCRYPTION_KEY") diff --git a/internal/server/middleware.go b/internal/server/middleware.go index 363b10e..0f5c412 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -18,6 +18,10 @@ type contextKey string const requestIDKey contextKey = "request_id" +// clientIPKey carries the client IP resolved by TrustClientIP. Absent unless +// TRUSTED_PROXY_CIDRS is configured, in which case remoteIP falls back to the peer. +const clientIPKey contextKey = "client_ip" + func RequestID(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { id := r.Header.Get("X-Request-Id") @@ -236,18 +240,162 @@ func (rl *rateLimiter) cleanup() { } } -// remoteIP returns the TCP-level remote address, stripped of its port. -// X-Real-IP and X-Forwarded-For are intentionally ignored: without a -// configured trusted-proxy allowlist, those headers can be forged by any -// client and would bypass the rate limit entirely. Operators behind a reverse -// proxy should strip proxy headers at the proxy level and rely on the TCP -// address the proxy connects with. See audit/claims.yaml's -// rate-limit-keys-on-tcp-source-address claim, recorded specifically because a -// prior Layer 2 audit pass mistook this deliberate behavior for a spoofable gap. +// remoteIP returns the IP a per-IP limit keys on. +// +// By default that is the TCP-level remote address, stripped of its port, and the +// forwarded headers are ignored: without a configured trusted-proxy allowlist those +// headers can be forged by any client and would bypass the rate limit entirely. +// Operators behind a reverse proxy should strip proxy headers at the proxy and rely on +// the TCP address the proxy connects with. See audit/claims.yaml's +// rate-limit-keys-on-tcp-source-address claim, recorded specifically because a prior +// Layer 2 audit pass mistook this deliberate behavior for a spoofable gap. +// +// When TRUSTED_PROXY_CIDRS is set, TrustClientIP has already resolved the client IP for +// this request and left it in the context — see resolveClientIP for what that means and +// what it deliberately does not do. This function reads that value when it is there, so +// the untrusted-peer path stays byte-for-byte the old behaviour. func remoteIP(r *http.Request) string { + if ip, ok := r.Context().Value(clientIPKey).(string); ok && ip != "" { + return ip + } + return peerIP(r) +} + +// peerIP returns the TCP-level remote address, stripped of its port. This is the only +// value in a request that a client cannot choose. +func peerIP(r *http.Request) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { return r.RemoteAddr } return host } + +// ParseTrustedProxies parses TRUSTED_PROXY_CIDRS entries. A bare address is accepted and +// treated as a single-host range (/32 or /128), because "10.0.0.7" is what an operator +// naming one proxy will write. +// +// Every parseable entry is returned even when others fail, and the error names the ones +// that did not: one typo should cost that hop's trust, not the whole list's. The caller +// is expected to log the error — an unparsed entry means that proxy's headers are NOT +// believed, which degrades to per-proxy rate limiting rather than to trusting a forgery. +func ParseTrustedProxies(entries []string) ([]*net.IPNet, error) { + var nets []*net.IPNet + var bad []string + for _, e := range entries { + e = strings.TrimSpace(e) + if e == "" { + continue + } + if _, n, err := net.ParseCIDR(e); err == nil { + nets = append(nets, n) + continue + } + if ip := net.ParseIP(e); ip != nil { + bits := 32 + if ip.To4() == nil { + bits = 128 + } + nets = append(nets, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) + continue + } + bad = append(bad, e) + } + if len(bad) > 0 { + return nets, fmt.Errorf("not a CIDR or IP address: %s", strings.Join(bad, ", ")) + } + return nets, nil +} + +// TrustClientIP returns middleware that resolves the client IP once per request and +// stores it in the request context for remoteIP to read. With no trusted proxies it is a +// pass-through, so an instance that has not configured any is unchanged. +// +// It is middleware rather than a package-level setting because the trust list belongs to +// one server instance: a mutable global would leak between instances in a test binary +// and would make "which requests are affected" unanswerable from the wiring. +func TrustClientIP(trusted []*net.IPNet) func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + if len(trusted) == 0 { + return next + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + ctx := context.WithValue(r.Context(), clientIPKey, resolveClientIP(r, trusted)) + next.ServeHTTP(w, r.WithContext(ctx)) + }) + } +} + +// resolveClientIP applies the forwarded headers, but only for a peer inside a trusted +// CIDR. A header from an untrusted peer is never read at all — that is the whole point, +// and it is why the default (no trusted proxies) cannot be weakened by a header. +// +// For a trusted peer the answer comes from X-Forwarded-For, walked RIGHT TO LEFT past +// trusted hops, returning the first untrusted address. ⛔ Not the leftmost entry: the +// left of that header is whatever the original client sent, so a client that pre-seeds +// "X-Forwarded-For: 1.2.3.4" gets it prepended and preserved by every well-behaved +// proxy. The rightmost non-trusted hop is the last address a trusted proxy actually +// observed, which is the only one in the header that anything vouched for. The peer is +// the answer whenever the header is absent or unusable. +// +// ⛔ Single-value vendor headers (CF-Connecting-IP, X-Real-IP, True-Client-IP) are NOT +// consulted, and that is deliberate rather than an omission. Trusting one means +// trusting it from every peer in the list, and the list is a list of *networks* rather +// than a list of CDNs: an ordinary reverse proxy inside it forwards whatever headers +// the client sent, so a client could name its own rate-limit bucket by sending one. The +// header carries nothing that says which hop set it, so there is no way to tell the +// value a CDN wrote from the value a visitor typed. +// +// Nothing is lost by leaving them out. A CDN sets X-Forwarded-For as well, and its own +// ranges belong in TRUSTED_PROXY_CIDRS anyway, so the walk steps over its edge address +// and lands on the visitor. That is the same answer the vendor header would have given, +// reached without having to believe a header for a reason the code cannot check. +// +// A hop that does not parse ends the walk and falls back to the peer rather than being +// skipped. Skipping it would let a client inject one malformed entry to push the walk +// past the real hop and onto a value it chose. Note this also means a proxy that appends +// "ip:port" (rather than a bare address) reads as malformed and lands on the peer, which +// is the safe direction to be wrong in. +func resolveClientIP(r *http.Request, trusted []*net.IPNet) string { + peer := peerIP(r) + if !ipInAny(peer, trusted) { + return peer + } + + // ⛔ Values, not Get. A header may arrive as several field lines, and Get returns + // only the FIRST. A client that sends its own "X-Forwarded-For: 1.2.3.4" followed by + // a proxy that ADDS a line rather than appending to the existing one leaves two + // lines, and Get would hand the walk the client's line alone — no trusted hop in it, + // so the walk returns the client's chosen address on its first step. RFC 9110 says + // repeated field lines are equivalent to one comma-joined value in the order + // received, so joining them is both correct and what the walk already assumes. + if xff := strings.Join(r.Header.Values("X-Forwarded-For"), ","); xff != "" { + hops := strings.Split(xff, ",") + for i := len(hops) - 1; i >= 0; i-- { + ip := net.ParseIP(strings.TrimSpace(hops[i])) + if ip == nil { + break + } + if ipInAny(ip.String(), trusted) { + continue // a trusted hop of our own; keep walking left + } + return ip.String() + } + } + + return peer +} + +// ipInAny reports whether ip (a textual address) falls inside any of nets. +func ipInAny(ip string, nets []*net.IPNet) bool { + parsed := net.ParseIP(ip) + if parsed == nil { + return false + } + for _, n := range nets { + if n.Contains(parsed) { + return true + } + } + return false +} diff --git a/internal/server/server.go b/internal/server/server.go index b25f7ab..460b46f 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -544,7 +544,19 @@ func New(ctx context.Context, cfg *config.Config, db *sql.DB, logger *slog.Logge // permanently if a marketing landing page is ever added here. mux.Handle("GET /{$}", http.RedirectHandler("/admin/", http.StatusFound)) - return RequestID(Logging(logger, SameOriginCheck(mux))), drain + // Trusted-proxy resolution wraps everything, so the per-IP limiters and anything else + // asking for the client IP see one answer computed once. A bad CIDR is logged and + // dropped rather than fatal: the consequence is that that hop's headers are not + // believed, which costs shared rate-limit buckets, never a trusted forgery. + trustedProxies, err := ParseTrustedProxies(cfg.TrustedProxyCIDRs) + if err != nil { + logger.Error("TRUSTED_PROXY_CIDRS: ignoring unparseable entries", "error", err) + } + if len(trustedProxies) > 0 { + logger.Info("trusting forwarded headers from proxies", "cidrs", cfg.TrustedProxyCIDRs) + } + + return TrustClientIP(trustedProxies)(RequestID(Logging(logger, SameOriginCheck(mux)))), drain } // seedSMTPToDB writes env-var SMTP settings into the DB on first boot so they diff --git a/internal/server/trustedproxy_test.go b/internal/server/trustedproxy_test.go new file mode 100644 index 0000000..8792d8f --- /dev/null +++ b/internal/server/trustedproxy_test.go @@ -0,0 +1,207 @@ +package server + +import ( + "net" + "net/http" + "net/http/httptest" + "testing" +) + +// serveThroughTrust runs one request through TrustClientIP and reports what remoteIP +// resolved to inside the handler — the value a rate limiter would key on. +func serveThroughTrust(t *testing.T, cidrs []string, peer string, headers map[string]string) string { + t.Helper() + trusted, err := ParseTrustedProxies(cidrs) + if err != nil { + t.Fatalf("ParseTrustedProxies(%v): %v", cidrs, err) + } + var got string + h := TrustClientIP(trusted)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + got = remoteIP(r) + })) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = peer + for k, v := range headers { + req.Header.Set(k, v) + } + h.ServeHTTP(httptest.NewRecorder(), req) + return got +} + +// The default. An untrusted peer's forwarded headers are not read at all, so the spoof +// keys on the connection the spoofer actually made. +func TestTrustClientIP_untrustedPeerIgnoresSpoofedHeaders(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "203.0.113.9:1234", map[string]string{ + "X-Forwarded-For": "198.51.100.1", + "CF-Connecting-IP": "198.51.100.2", + "X-Real-IP": "198.51.100.3", + }) + if got != "203.0.113.9" { + t.Errorf("client IP = %q; want the peer 203.0.113.9", got) + } +} + +// With no trusted proxies configured at all, TrustClientIP is a pass-through and the +// behaviour is exactly what it was before the setting existed. +func TestTrustClientIP_noTrustedProxiesIsUnchanged(t *testing.T) { + got := serveThroughTrust(t, nil, "10.0.0.5:1234", map[string]string{ + "X-Forwarded-For": "198.51.100.1", + }) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } +} + +// ⛔ Single-value vendor headers are ignored even from a TRUSTED peer, and this is the +// case that says why. Trusting one means trusting it from every network in the list, +// and an ordinary reverse proxy in that list forwards whatever headers the client sent. +// If CF-Connecting-IP were preferred, the client below would have chosen its own +// rate-limit key by sending one header, which is exactly the spoof the right-to-left +// walk exists to prevent. +// +// The chain is what is believed: 10.0.0.9 is one of ours, so the walk steps over it and +// stops at 198.51.100.7, the address our outermost proxy actually observed. +func TestTrustClientIP_ignoresVendorHeadersFromATrustedPeer(t *testing.T) { + for _, header := range []string{"CF-Connecting-IP", "X-Real-IP", "True-Client-IP"} { + t.Run(header, func(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", map[string]string{ + header: "1.2.3.4", + "X-Forwarded-For": "198.51.100.7, 10.0.0.9", + }) + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7 from the chain, never %s", got, header) + } + }) + } +} + +// And with no X-Forwarded-For to fall back on, a vendor header still buys the client +// nothing: the answer is the peer, not the address the client named. +func TestTrustClientIP_vendorHeaderAloneFallsBackToThePeer(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"CF-Connecting-IP": "1.2.3.4"}) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } +} + +// ⛔ X-Forwarded-For can arrive as SEVERAL field lines, and Header.Get returns only the +// first one. +// +// A client that sends its own X-Forwarded-For, in front of a proxy that ADDS a line +// rather than appending to the existing one, produces exactly this: line 1 is the +// client's, line 2 is the proxy's. Reading only line 1 hands the walk a chain with no +// trusted hop in it, so it returns the client's chosen address on the first step and the +// client has named its own rate-limit bucket. RFC 9110 makes repeated field lines +// equivalent to one comma-joined value in order, which is what the walk needs. +// +// This uses Add rather than the shared helper on purpose: the helper calls Set, which +// collapses everything to one line and cannot express the case. +func TestTrustClientIP_joinsRepeatedForwardedForLines(t *testing.T) { + trusted, err := ParseTrustedProxies([]string{"10.0.0.0/8"}) + if err != nil { + t.Fatalf("ParseTrustedProxies: %v", err) + } + var got string + h := TrustClientIP(trusted)(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + got = remoteIP(r) + })) + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.RemoteAddr = "10.0.0.5:1234" + req.Header.Add("X-Forwarded-For", "1.2.3.4") // the client's own line + req.Header.Add("X-Forwarded-For", "198.51.100.7, 10.0.0.9") // what our proxies observed + h.ServeHTTP(httptest.NewRecorder(), req) + + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7 — with only the first field line read, "+ + "the client's own 1.2.3.4 becomes the rate-limit key", got) + } +} + +// Two of our own hops appended themselves; the walk goes right to left past both and +// stops at the address the outermost trusted proxy actually saw. +func TestTrustClientIP_walksTrustedChainRightToLeft(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8", "192.168.0.0/16"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": "198.51.100.7, 192.168.1.1, 10.0.0.9"}) + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7", got) + } +} + +// The leftmost entry is client-supplied. A client that pre-seeds the header must not be +// able to choose its own key by putting a value to the left of the real one. +func TestTrustClientIP_ignoresEntriesLeftOfTheRealHop(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": "1.2.3.4, 198.51.100.7, 10.0.0.9"}) + if got != "198.51.100.7" { + t.Errorf("client IP = %q; want 198.51.100.7 (not the client-seeded 1.2.3.4)", got) + } +} + +func TestTrustClientIP_malformedHeaderFallsBackToPeer(t *testing.T) { + cases := map[string]string{ + "garbage": "not-an-ip", + "empty entries": " , ,", + "address with port": "198.51.100.7:443", + "malformed left hop": "not-an-ip, 10.0.0.9", + } + for name, xff := range cases { + t.Run(name, func(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": xff}) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } + }) + } +} + +// A chain of nothing but our own proxies has no client address in it. The peer is the +// only honest answer left. +func TestTrustClientIP_allHopsTrustedFallsBackToPeer(t *testing.T) { + got := serveThroughTrust(t, []string{"10.0.0.0/8"}, "10.0.0.5:1234", + map[string]string{"X-Forwarded-For": "10.0.0.7, 10.0.0.9"}) + if got != "10.0.0.5" { + t.Errorf("client IP = %q; want the peer 10.0.0.5", got) + } +} + +func TestTrustClientIP_ipv6(t *testing.T) { + // The proxies sit in 2001:db8:0::/48; the client is one prefix over, so the walk + // skips the trusted hop and stops on it. + got := serveThroughTrust(t, []string{"2001:db8:0::/48"}, "[2001:db8::1]:1234", + map[string]string{"X-Forwarded-For": "2001:db8:1::5, 2001:db8::9"}) + if got != "2001:db8:1::5" { + t.Errorf("client IP = %q; want 2001:db8:1::5", got) + } +} + +func TestParseTrustedProxies(t *testing.T) { + // A bare address is what an operator naming one proxy writes. + nets, err := ParseTrustedProxies([]string{"10.0.0.7", "192.168.0.0/16", "2001:db8::1"}) + if err != nil { + t.Fatalf("err = %v; want nil", err) + } + if len(nets) != 3 { + t.Fatalf("parsed %d entries; want 3", len(nets)) + } + if !nets[0].Contains(net.ParseIP("10.0.0.7")) || nets[0].Contains(net.ParseIP("10.0.0.8")) { + t.Errorf("bare IPv4 %v should be exactly one host", nets[0]) + } + if !nets[2].Contains(net.ParseIP("2001:db8::1")) || nets[2].Contains(net.ParseIP("2001:db8::2")) { + t.Errorf("bare IPv6 %v should be exactly one host", nets[2]) + } +} + +// One typo costs that hop's trust, not the whole list's — and it is reported rather than +// swallowed, because an operator who thinks a proxy is trusted and is wrong gets shared +// rate-limit buckets with no explanation. +func TestParseTrustedProxies_reportsBadEntriesAndKeepsTheRest(t *testing.T) { + nets, err := ParseTrustedProxies([]string{"10.0.0.0/8", "10.0.0.0/99", "nonsense"}) + if err == nil { + t.Fatal("err = nil; want the bad entries named") + } + if len(nets) != 1 { + t.Errorf("parsed %d entries; want the 1 good one kept", len(nets)) + } +}