diff --git a/README.md b/README.md index d05ce68..136a2c3 100644 --- a/README.md +++ b/README.md @@ -1339,8 +1339,9 @@ Every session has a hostmask in IRC's form, `nick!username@hostname`: from, or that IP address when the lookup gives no name. The lookup waits up to three seconds. -Over the HTTP API, the server takes the client's IP address from the first entry -of the `X-Forwarded-For` header, then `X-Real-IP`, then the TCP connection (see +Over the HTTP API, the client's IP address is the address of the TCP connection. +When that connection comes from a trusted proxy, the server reads the address +from `X-Forwarded-For` or `X-Real-IP` instead (see [Reverse proxy](#reverse-proxy)). Every user can see a hostmask: in `RPL_WHOISUSER` (`311`), `RPL_WHOREPLY` @@ -1450,10 +1451,9 @@ over the limit gets `429` `too many login attempts, try again later` with attempt is more than 15 minutes old. Session creation has no rate limit; the hashcash stamp is its cost. -The IP address comes from the first entry of `X-Forwarded-For`, else from -`X-Real-IP`, else from the TCP connection. Without a reverse proxy that -overwrites those headers, a client can set them itself and so pick any address -(see [Reverse proxy](#reverse-proxy)). +The IP address is the client's address as described in +[Reverse proxy](#reverse-proxy). A client outside the trusted proxies cannot +change it with `X-Forwarded-For` or `X-Real-IP`. ### Writing a client @@ -1714,10 +1714,14 @@ Every setting is an environment variable. The server also reads a `.env` file in its working directory, and a YAML file `neoirc.yaml` in `/etc/neoirc/` or `$HOME/.config/neoirc/` with the same names as keys. A variable from the environment wins over one in `.env`, and both win over `neoirc.yaml`. An -environment variable set to the empty string counts as unset, except -`IRC_LISTEN_ADDR`, where the empty string turns the IRC listener off. A -`neoirc.yaml` that is not valid YAML stops the server at startup. A setting that -does not parse or is out of range also stops the server at startup, and the +environment variable set to the empty string counts as unset, with two +exceptions: + +- For `IRC_LISTEN_ADDR`, the empty string turns the IRC listener off. +- For `TRUSTED_PROXIES`, the empty string trusts no proxy. + +A `neoirc.yaml` that is not valid YAML stops the server at startup. A setting +that does not parse or is out of range also stops the server at startup, and the error names the setting. - `PORT` (default `8080`): the HTTP port. `0` lets the system pick a free port. @@ -1747,6 +1751,11 @@ error names the setting. address. A fraction such as `0.5` is allowed. It must be above `0`. - `LOGIN_RATE_BURST` (default `5`): how many attempts an address may make at once before the rate applies. It must be at least `1`. +- `TRUSTED_PROXIES` (default `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16`): the + reverse proxies whose `X-Forwarded-For` and `X-Real-IP` headers the server + believes. It is a comma-separated list of networks, such as `10.0.0.0/8`, and + single addresses, such as `192.0.2.1`. A list replaces the default entirely. + The empty string trusts nothing. See [Reverse proxy](#reverse-proxy). - `SESSION_IDLE_TIMEOUT` (default `720h`, 30 days): a duration with a unit, `h` for hours, `m` for minutes or `s` for seconds, such as `720h` or `36h30m`. A client with no request for this long is removed, and a session left with no @@ -1804,14 +1813,34 @@ Run the server behind a reverse proxy that terminates TLS. The server itself speaks plain HTTP, and the auth cookie is `Secure`, so browsers send it only over HTTPS. The proxy must: -- Set `X-Forwarded-For` to the client's address, replacing any value the client - sent. The server reads that header first and trusts it from anyone; it reads - `X-Real-IP` only when `X-Forwarded-For` is absent. Without a proxy that - overwrites the header, a client can choose its own address, which sets its - hostname and lets it dodge the login rate limit. +- Connect from an address in `TRUSTED_PROXIES`. The default trusts the private + ranges `10.0.0.0/8`, `172.16.0.0/12` and `192.168.0.0/16`. The setting must + hold the address the server sees the proxy connect from. For a proxy on the + same host: + - With the server outside Docker, that address is `127.0.0.1` or `::1`. The + default does not trust them, so set `TRUSTED_PROXIES=127.0.0.1,::1`. + - Through a published Docker port, that address is the Docker bridge + gateway, such as `172.17.0.1`. The default trusts it. +- Add the client's address to `X-Forwarded-For`. - Wait at least 60 seconds for a response. A long poll can take 30 seconds, and the server's own write timeout is 60 seconds. +The server finds the client's address in these steps: + +- A connection from outside `TRUSTED_PROXIES` is the client. The server ignores + its `X-Forwarded-For` and `X-Real-IP` headers. +- For a connection from a trusted proxy, the server reads `X-Forwarded-For` from + right to left. It skips trusted addresses and takes the first untrusted one. + When every entry is trusted, it takes the leftmost. +- Without `X-Forwarded-For`, the server takes `X-Real-IP`. +- When an entry it reads is not an IP address, the server uses the proxy's + address. + +The client's address sets the session's hostname and counts toward the login +rate limit. Any connection from a trusted address can choose its client address +through `X-Forwarded-For`. If clients connect directly from private addresses, +set `TRUSTED_PROXIES` to the proxy's address alone. + Caddy: ``` @@ -1861,9 +1890,21 @@ The IRC listener has no TLS. The server logs with Go's `slog`, one line per event, to standard output. The lines are JSON, or plain text when standard output is a terminal. Every HTTP -request is logged with its method, URL, protocol, status, latency, remote -address, user agent, referrer and request ID. `DEBUG=true` adds debug-level -lines. +request is logged with: + +- start time +- method +- URL +- protocol +- status +- latency +- client address +- user agent +- referrer +- request ID + +The client address is the one found as described in +[Reverse proxy](#reverse-proxy). `DEBUG=true` adds debug-level lines. ### The database @@ -1946,8 +1987,8 @@ and their sessions to end. Then it closes the database. gets a message. - `internal/hashcash/`: stamp checking and minting. - `internal/ratelimit/`: the per-address login rate limiter. -- `internal/middleware/`: request logging, cross-origin requests, the - `Content-Security-Policy` header and metrics. +- `internal/middleware/`: finding the client's address, request logging, + cross-origin requests, the `Content-Security-Policy` header and metrics. - `internal/healthcheck/` and `internal/stats/`: the health check and its counters. - `internal/logger/` and `internal/globals/`: logging setup and build diff --git a/go.mod b/go.mod index 21dc68a..73baceb 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( github.com/slok/go-http-metrics v0.13.0 github.com/spf13/viper v1.21.0 go.uber.org/fx v1.24.0 + go4.org/netipx v0.0.0-20260823151212-3075585bcbeb golang.org/x/crypto v0.48.0 golang.org/x/time v0.6.0 modernc.org/sqlite v1.45.0 diff --git a/go.sum b/go.sum index 902e77d..e8c87ca 100644 --- a/go.sum +++ b/go.sum @@ -111,6 +111,8 @@ go.yaml.in/yaml/v2 v2.4.2 h1:DzmwEr2rDGHl7lsFgAHxmNz/1NlQ7xLIrlN2h5d1eGI= go.yaml.in/yaml/v2 v2.4.2/go.mod h1:081UH+NErpNdqlCXm3TtEran0rJZGxAYx9hb/ELlsPU= go.yaml.in/yaml/v3 v3.0.4 h1:tfq32ie2Jv2UxXFdLJdh3jXuOzWiL1fo0bu/FbuKpbc= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= +go4.org/netipx v0.0.0-20260823151212-3075585bcbeb h1:XBM4hvfwGAttkkiTIFfeigdfcL1xIfdKXqFdgiHGtDs= +go4.org/netipx v0.0.0-20260823151212-3075585bcbeb/go.mod h1:PLyyIXexvUFg3Owu6p/WfdlivPbZJsZdgWZlrGope/Y= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/crypto v0.48.0 h1:/VRzVqiRSggnhY7gNRxPauEQ5Drw9haKdM0jqfcCFts= diff --git a/internal/config/config.go b/internal/config/config.go index f345dac..01c47a5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -6,12 +6,15 @@ import ( "fmt" "log/slog" "math" + "net/netip" "os" "strconv" + "strings" "time" "github.com/spf13/viper" "go.uber.org/fx" + "go4.org/netipx" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/logger" @@ -32,8 +35,16 @@ Type /help for available commands.` const DefaultDBURL = "file:///var/lib/neoirc/state.db" + "?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)" +// defaultTrustedProxies is TRUSTED_PROXIES when it is not set: the +// private address ranges of RFC 1918. +const defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16" + var errOutOfRange = errors.New("setting out of range") +var errNeverMatches = errors.New("entry can never match a connection") + +var errNotString = errors.New("not a string") + // Params defines the dependencies for creating a Config. type Params struct { fx.In @@ -64,13 +75,14 @@ type Config struct { LoginRateLimit float64 LoginRateBurst int IRCListenAddr string + TrustedProxies *netipx.IPSet params *Params log *slog.Logger } // New creates a new Config by reading from files and environment variables. -// It returns an error when a number, true/false or duration setting does -// not parse or is out of range. +// It returns an error when a number, true/false, duration or address +// setting does not parse or is out of range. func New( _ fx.Lifecycle, params Params, ) (*Config, error) { @@ -103,6 +115,7 @@ func New( viper.SetDefault("LOGIN_RATE_LIMIT", "1") viper.SetDefault("LOGIN_RATE_BURST", "5") viper.SetDefault("IRC_LISTEN_ADDR", ":6667") + viper.SetDefault("TRUSTED_PROXIES", defaultTrustedProxies) err := viper.ReadInConfig() if err != nil { @@ -151,10 +164,10 @@ func New( return cfg, nil } -// parseSettings reads the number and true/false settings into cfg and -// checks the duration settings. Each number and duration must be above -// zero. PORT and NEOIRC_HASHCASH_BITS may also be zero: port 0 lets the -// system pick a port, and 0 bits turns proof of work off. The error +// parseSettings reads the number, true/false and address settings into +// cfg and checks the duration settings. Each number and duration must be +// above zero. PORT and NEOIRC_HASHCASH_BITS may also be zero: port 0 lets +// the system pick a port, and 0 bits turns proof of work off. The error // names each setting that does not parse or is out of range. func parseSettings(cfg *Config) error { return errors.Join( @@ -168,9 +181,89 @@ func parseSettings(cfg *Config) error { checkDuration("MESSAGE_MAX_AGE", cfg.MessageMaxAge), checkDuration("QUEUE_MAX_AGE", cfg.QueueMaxAge), checkDuration("SESSION_IDLE_TIMEOUT", cfg.SessionIdleTimeout), + parseTrustedProxies(&cfg.TrustedProxies), ) } +// parseTrustedProxies sets *value to the addresses in TRUSTED_PROXIES, a +// comma-separated list of networks such as 10.0.0.0/8 and single +// addresses such as 192.0.2.1. The empty string trusts nothing. Viper +// counts an empty environment variable as unset, so the environment is +// read directly as well. +func parseTrustedProxies(value **netipx.IPSet) error { + setting := viper.Get("TRUSTED_PROXIES") + + fromEnv, set := os.LookupEnv("TRUSTED_PROXIES") + if set { + setting = fromEnv + } + + // A YAML list or map in the config file is not a string, and + // viper.GetString would turn it into the empty string. + raw, isString := setting.(string) + if !isString { + return fmt.Errorf("cannot parse TRUSTED_PROXIES: %v is %w", + setting, errNotString) + } + + var builder netipx.IPSetBuilder + + if strings.TrimSpace(raw) != "" { + for entry := range strings.SplitSeq(raw, ",") { + err := addTrustedProxy(&builder, strings.TrimSpace(entry)) + if err != nil { + return fmt.Errorf("cannot parse TRUSTED_PROXIES: %w", err) + } + } + } + + ipSet, err := builder.IPSet() + if err != nil { + return fmt.Errorf("cannot parse TRUSTED_PROXIES: %w", err) + } + + *value = ipSet + + return nil +} + +// addTrustedProxy adds entry, a network with a slash or a single +// address, to builder. An IPv4 address written in IPv6 form, such as +// ::ffff:192.0.2.1, is added as IPv4. A network in that form and an +// address with a zone are errors, since no connection could match them. +func addTrustedProxy(builder *netipx.IPSetBuilder, entry string) error { + if strings.Contains(entry, "/") { + prefix, err := netip.ParsePrefix(entry) + if err != nil { + return err //nolint:wrapcheck // the caller names the setting + } + + // A connection from an IPv4 address is checked in IPv4 form. + if prefix.Addr().Is4In6() { + return fmt.Errorf("%w: %s is an IPv4 network in IPv6 form", + errNeverMatches, entry) + } + + builder.AddPrefix(prefix) + + return nil + } + + addr, err := netip.ParseAddr(entry) + if err != nil { + return err //nolint:wrapcheck // the caller names the setting + } + + // IPSet.Contains is false for every address with a zone. + if addr.Zone() != "" { + return fmt.Errorf("%w: %s has a zone", errNeverMatches, entry) + } + + builder.Add(addr.Unmap()) + + return nil +} + // parseInt sets *value to the setting key read as a whole number. It // returns an error when the value does not parse or is below minimum. func parseInt(key string, minimum int, value *int) error { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index dc57b38..9ab5dfd 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,11 +1,14 @@ package config_test import ( + "net/netip" "os" + "path/filepath" "strings" "testing" "time" + "github.com/spf13/viper" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/logger" @@ -140,8 +143,7 @@ func settingsOf(cfg *config.Config) settings { } func TestBadSettingStopsStartup(t *testing.T) { - // For each setting, the first value does not parse and the others - // parse but are out of range. + // Each value is invalid for its setting. tests := []struct { key string values []string @@ -158,6 +160,10 @@ func TestBadSettingStopsStartup(t *testing.T) { {key: "MESSAGE_MAX_AGE", values: []string{"30d", "0", "-1h"}}, {key: "QUEUE_MAX_AGE", values: []string{"720", "0s", "-24h"}}, {key: "SESSION_IDLE_TIMEOUT", values: []string{"a month", "0", "-1m"}}, + {key: "TRUSTED_PROXIES", values: []string{ + "proxy.example.com", "10.0.0.0/33", "192.0.2.1:8080", + "::ffff:10.0.0.0/104", "fe80::1%eth0", + }}, } for _, test := range tests { @@ -206,3 +212,124 @@ func TestIRCListenAddrEmptyTurnsListenerOff(t *testing.T) { t.Errorf("IRCListenAddr = %q, want empty", cfg.IRCListenAddr) } } + +func TestTrustedProxies(t *testing.T) { + // Each case lists addresses that must be in the trusted set and + // addresses that must not. + tests := []struct { + name string + set bool + value string + trusted []string + untrusted []string + }{ + { + name: "unset trusts the RFC 1918 ranges", + set: false, + value: "", + trusted: []string{ + "10.1.2.3", "172.16.0.1", "172.31.255.254", "192.168.1.1", + }, + untrusted: []string{ + "127.0.0.1", "172.32.0.1", "192.0.2.1", "2001:db8::1", + }, + }, + { + name: "a value replaces the default", + set: true, + value: "198.51.100.0/24, 192.0.2.7,2001:db8::/32", + trusted: []string{"198.51.100.200", "192.0.2.7", "2001:db8::5"}, + untrusted: []string{"10.4.5.6", "192.168.4.4", "192.0.2.8"}, + }, + { + name: "a mapped IPv6 address is trusted as IPv4", + set: true, + value: "::ffff:192.0.2.7", + trusted: []string{"192.0.2.7"}, + untrusted: []string{"10.7.8.9"}, + }, + { + name: "the empty string trusts nothing", + set: true, + value: "", + trusted: nil, + untrusted: []string{"10.0.0.1", "192.168.0.1", "::1"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // t.Setenv puts back whatever value was there after the + // test, including when Unsetenv removes it. + t.Setenv("TRUSTED_PROXIES", test.value) + + if !test.set { + err := os.Unsetenv("TRUSTED_PROXIES") + if err != nil { + t.Fatal(err) + } + } + + proxies := newConfig(t).TrustedProxies + + for _, addr := range test.trusted { + if !proxies.Contains(netip.MustParseAddr(addr)) { + t.Errorf("%s is not trusted", addr) + } + } + + for _, addr := range test.untrusted { + if proxies.Contains(netip.MustParseAddr(addr)) { + t.Errorf("%s is trusted", addr) + } + } + }) + } +} + +func TestTrustedProxiesNotAStringInYAMLStopsStartup(t *testing.T) { + // YAML reads the first value as a list and the second as a map. + values := []string{"[10.0.0.1]", "{proxy: 10.0.0.1}"} + + for _, value := range values { + t.Run(value, func(t *testing.T) { + // t.Setenv puts back whatever value was there after the + // test, including when Unsetenv removes it. + t.Setenv("TRUSTED_PROXIES", "") + + err := os.Unsetenv("TRUSTED_PROXIES") + if err != nil { + t.Fatal(err) + } + + // loadConfig names the app neoirc-test, so config.New reads + // $HOME/.config/neoirc-test/neoirc-test.yaml. + home := t.TempDir() + t.Setenv("HOME", home) + + dir := filepath.Join(home, ".config", "neoirc-test") + + err = os.MkdirAll(dir, 0o700) + if err != nil { + t.Fatal(err) + } + + err = os.WriteFile( + filepath.Join(dir, "neoirc-test.yaml"), + []byte("TRUSTED_PROXIES: "+value+"\n"), 0o600, + ) + if err != nil { + t.Fatal(err) + } + + // Viper keeps the values it read from this file after the + // file is gone. Reset clears them for later tests. + t.Cleanup(viper.Reset) + + _, err = loadConfig(t) + if err == nil || !strings.Contains(err.Error(), "TRUSTED_PROXIES") { + t.Errorf("error = %v, want one naming TRUSTED_PROXIES", err) + } + }) + } +} diff --git a/internal/handlers/api.go b/internal/handlers/api.go index 0843b31..fb3dfe1 100644 --- a/internal/handlers/api.go +++ b/internal/handlers/api.go @@ -42,25 +42,9 @@ func (hdlr *Handlers) maxBodySize() int64 { return defaultMaxBodySize } -// clientIP extracts the connecting client's IP address -// from the request, checking X-Forwarded-For and -// X-Real-IP headers before falling back to RemoteAddr. +// clientIP returns the IP address in request.RemoteAddr, which the +// ClientIP middleware has set to the client's address. func clientIP(request *http.Request) string { - if forwarded := request.Header.Get("X-Forwarded-For"); forwarded != "" { - // X-Forwarded-For can contain a comma-separated list; - // the first entry is the original client. - parts := strings.SplitN(forwarded, ",", 2) //nolint:mnd - ip := strings.TrimSpace(parts[0]) - - if ip != "" { - return ip - } - } - - if realIP := request.Header.Get("X-Real-IP"); realIP != "" { - return strings.TrimSpace(realIP) - } - host, _, err := net.SplitHostPort(request.RemoteAddr) if err != nil { return request.RemoteAddr diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index 00c6be4..18137e2 100644 --- a/internal/handlers/api_test.go +++ b/internal/handlers/api_test.go @@ -4281,6 +4281,117 @@ func TestLoginRateLimitExceeded(t *testing.T) { } } +func TestLoginRateLimitIgnoresForwardedHeaders(t *testing.T) { + t.Parallel() + + // The test client connects from 127.0.0.1, which is outside the + // default trusted proxies, so the server must ignore these headers. + tests := []struct { + name string + header string + }{ + {name: "X-Forwarded-For", header: "X-Forwarded-For"}, + {name: "X-Real-IP", header: "X-Real-IP"}, + } + + loginBody, err := json.Marshal(map[string]string{ + nickKey: "unknownnick", passwordKey: "wrongpassword", + }) + if err != nil { + t.Fatal(err) + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + tserver := newTestServer(t) + + // The default burst is 5 attempts, so the sixth gets 429 + // although each claims a new address. + var status int + + for attempt := range 6 { + request, rErr := http.NewRequestWithContext( + t.Context(), http.MethodPost, + tserver.url("/api/v1/login"), + bytes.NewReader(loginBody), + ) + if rErr != nil { + t.Fatal(rErr) + } + + request.Header.Set("Content-Type", "application/json") + request.Header.Set( + test.header, fmt.Sprintf("203.0.113.%d", attempt+1), + ) + + resp, dErr := http.DefaultClient.Do(request) + if dErr != nil { + t.Fatal(dErr) + } + + status = resp.StatusCode + _ = resp.Body.Close() + } + + if status != http.StatusTooManyRequests { + t.Fatalf("sixth attempt got %d, want 429", status) + } + }) + } +} + +func TestLoginRateLimitPerClientBehindTrustedProxy(t *testing.T) { + // The test client connects from 127.0.0.1. Trusted as a proxy, it + // passes each client's address in X-Forwarded-For. t.Setenv rules + // out t.Parallel. + t.Setenv("TRUSTED_PROXIES", "127.0.0.1") + + tserver := newTestServer(t) + + // An empty JSON object is refused after the rate limit counts the + // attempt, so no attempt checks a password or looks up a hostname. + loginFrom := func(forwardedFor string) int { + t.Helper() + + request, err := http.NewRequestWithContext( + t.Context(), http.MethodPost, + tserver.url("/api/v1/login"), strings.NewReader("{}"), + ) + if err != nil { + t.Fatal(err) + } + + request.Header.Set("Content-Type", "application/json") + request.Header.Set("X-Forwarded-For", forwardedFor) + + resp, err := http.DefaultClient.Do(request) + if err != nil { + t.Fatal(err) + } + + _ = resp.Body.Close() + + return resp.StatusCode + } + + // The default burst is 5 attempts per address. + for range 5 { + loginFrom("203.0.113.1") + } + + status := loginFrom("203.0.113.1") + if status != http.StatusTooManyRequests { + t.Errorf("sixth attempt from 203.0.113.1 got %d, want 429", status) + } + + status = loginFrom("203.0.113.2") + if status == http.StatusTooManyRequests { + t.Error("first attempt from 203.0.113.2 got 429") + } +} + func TestLoginRateLimitAllowsNormalUse(t *testing.T) { t.Parallel() diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 3f32705..52469ef 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -5,6 +5,9 @@ import ( "log/slog" "net" "net/http" + "net/netip" + "slices" + "strings" "time" basicauth "github.com/99designs/basicauth-go" @@ -15,6 +18,7 @@ import ( "github.com/slok/go-http-metrics/middleware/std" "github.com/spf13/viper" "go.uber.org/fx" + "go4.org/netipx" "sneak.berlin/go/neoirc/internal/config" "sneak.berlin/go/neoirc/internal/globals" "sneak.berlin/go/neoirc/internal/logger" @@ -62,6 +66,77 @@ func ipFromHostPort(hostPort string) string { return host } +// ClientIP returns middleware that replaces request.RemoteAddr with the +// client's address and keeps the port of the TCP connection. The +// X-Forwarded-For and X-Real-IP headers count only when the connection +// comes from an address in TRUSTED_PROXIES. +func (mware *Middleware) ClientIP() func(http.Handler) http.Handler { + trusted := mware.params.Config.TrustedProxies + + return func(next http.Handler) http.Handler { + return http.HandlerFunc( + func( + writer http.ResponseWriter, + request *http.Request, + ) { + peer, err := netip.ParseAddrPort(request.RemoteAddr) + if err == nil { + client := clientAddr( + peer.Addr().Unmap(), request.Header, trusted, + ) + request.RemoteAddr = netip.AddrPortFrom( + client, peer.Port(), + ).String() + } + + next.ServeHTTP(writer, request) + }) + } +} + +// clientAddr returns the client's address for a request from peer. A +// peer outside trusted is the client, whatever its headers say. A +// trusted peer is a proxy. Its X-Forwarded-For list is read from the +// right, past the trusted addresses, to the first untrusted one; when +// all are trusted, the client is the leftmost. Without X-Forwarded-For, +// the client is X-Real-IP. An entry that is not an IP address gives +// peer. +func clientAddr( + peer netip.Addr, header http.Header, trusted *netipx.IPSet, +) netip.Addr { + if !trusted.Contains(peer) { + return peer + } + + forwarded := strings.Join(header.Values("X-Forwarded-For"), ",") + if forwarded == "" { + realIP, err := netip.ParseAddr( + strings.TrimSpace(header.Get("X-Real-IP")), + ) + if err != nil { + return peer + } + + return realIP.Unmap() + } + + client := peer + + for _, entry := range slices.Backward(strings.Split(forwarded, ",")) { + addr, err := netip.ParseAddr(strings.TrimSpace(entry)) + if err != nil { + return peer + } + + client = addr.Unmap() + if !trusted.Contains(client) { + break + } + } + + return client +} + type loggingResponseWriter struct { http.ResponseWriter diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go new file mode 100644 index 0000000..e2ef069 --- /dev/null +++ b/internal/middleware/middleware_test.go @@ -0,0 +1,189 @@ +package middleware_test + +import ( + "net/http" + "net/http/httptest" + "net/netip" + "testing" + "time" + + "go4.org/netipx" + "sneak.berlin/go/neoirc/internal/config" + "sneak.berlin/go/neoirc/internal/globals" + "sneak.berlin/go/neoirc/internal/logger" + "sneak.berlin/go/neoirc/internal/middleware" +) + +// The tests trust 10.0.0.0/8. Clients use the documentation ranges +// 192.0.2.0/24, 198.51.100.0/24 and 203.0.113.0/24. +const ( + trustedNetwork = "10.0.0.0/8" + proxyPeer = "10.0.0.1:4000" + directPeer = "192.0.2.1:4000" + client = "203.0.113.7" + clientWithPort = "203.0.113.7:4000" + realIPClient = "203.0.113.8" +) + +type clientIPTest struct { + name string + remoteAddr string + forwarded []string + realIP string + want string +} + +func clientIPTests() []clientIPTest { + return []clientIPTest{ + { + name: "untrusted peer ignores forged headers", + remoteAddr: directPeer, + forwarded: []string{client}, + realIP: realIPClient, + want: directPeer, + }, + {name: "trusted peer without headers", remoteAddr: proxyPeer, want: proxyPeer}, + { + name: "trusted peer gives rightmost untrusted entry", + remoteAddr: proxyPeer, + forwarded: []string{"198.51.100.9, 203.0.113.7, 10.0.0.2"}, + want: clientWithPort, + }, + { + name: "entries split over several headers", + remoteAddr: proxyPeer, + forwarded: []string{"198.51.100.9", client, "10.0.0.2"}, + want: clientWithPort, + }, + { + name: "all-trusted chain gives leftmost entry", + remoteAddr: proxyPeer, + forwarded: []string{"10.0.0.3, 10.0.0.2"}, + want: "10.0.0.3:4000", + }, + { + name: "X-Real-IP without X-Forwarded-For", + remoteAddr: proxyPeer, + realIP: realIPClient, + want: "203.0.113.8:4000", + }, + { + name: "X-Forwarded-For wins over X-Real-IP", + remoteAddr: proxyPeer, + forwarded: []string{client}, + realIP: realIPClient, + want: clientWithPort, + }, + { + name: "unparseable X-Forwarded-For entry gives peer", + remoteAddr: proxyPeer, + forwarded: []string{"203.0.113.7, not-an-ip, 10.0.0.2"}, + want: proxyPeer, + }, + { + name: "unparseable entry past the client is not read", + remoteAddr: proxyPeer, + forwarded: []string{"not-an-ip, 203.0.113.7"}, + want: clientWithPort, + }, + { + name: "unparseable X-Real-IP gives peer", + remoteAddr: proxyPeer, + realIP: "not-an-ip", + want: proxyPeer, + }, + { + name: "mapped IPv6 peer is unmapped and trusted", + remoteAddr: "[::ffff:10.0.0.1]:4000", + forwarded: []string{client}, + want: clientWithPort, + }, + { + name: "mapped IPv6 peer outside the set", + remoteAddr: "[::ffff:192.0.2.1]:4000", + forwarded: []string{client}, + want: directPeer, + }, + { + name: "IPv6 client", + remoteAddr: proxyPeer, + forwarded: []string{"2001:db8::1"}, + want: "[2001:db8::1]:4000", + }, + } +} + +func TestClientIP(t *testing.T) { + t.Parallel() + + mware := newMiddleware(t, trustedNetwork) + + for _, test := range clientIPTests() { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + var got string + + handler := mware.ClientIP()(http.HandlerFunc(func( + _ http.ResponseWriter, request *http.Request, + ) { + got = request.RemoteAddr + })) + + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) + request.RemoteAddr = test.remoteAddr + + for _, value := range test.forwarded { + request.Header.Add("X-Forwarded-For", value) + } + + if test.realIP != "" { + request.Header.Set("X-Real-IP", test.realIP) + } + + handler.ServeHTTP(httptest.NewRecorder(), request) + + if got != test.want { + t.Errorf("RemoteAddr = %q, want %q", got, test.want) + } + }) + } +} + +// newMiddleware returns a Middleware that trusts the network trusted. +func newMiddleware(t *testing.T, trusted string) *middleware.Middleware { + t.Helper() + + var builder netipx.IPSetBuilder + + builder.AddPrefix(netip.MustParsePrefix(trusted)) + + trustedSet, err := builder.IPSet() + if err != nil { + t.Fatal(err) + } + + globs := &globals.Globals{ + Appname: "neoirc-test", + Version: "test", + StartTime: time.Now(), + } + + log, err := logger.New(nil, logger.Params{Globals: globs}) + if err != nil { + t.Fatal(err) + } + + mware, err := middleware.New(nil, middleware.Params{ + Logger: log, + Globals: globs, + Config: &config.Config{TrustedProxies: trustedSet}, + }) + if err != nil { + t.Fatal(err) + } + + return mware +} diff --git a/internal/server/routes.go b/internal/server/routes.go index f428c92..98312d5 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -23,6 +23,7 @@ func (srv *Server) SetupRoutes() { srv.router.Use(middleware.Recoverer) srv.router.Use(middleware.RequestID) + srv.router.Use(srv.mw.ClientIP()) srv.router.Use(srv.mw.Logging()) if viper.GetString("METRICS_USERNAME") != "" {