feat(login): key the rate limiter on the real client behind trusted proxies

Author: Romsik788

README.md

@@ -47,6 +47,7 @@
       - ADMIN_USERNAME=admin
       - ADMIN_PASSWORD=<any password>
       - PORT=8798
+      - TRUSTED_PROXIES=172.16.0.0/12 # the docker network traefik reaches OnLogs over, see below
     #  - ONLOGS_PATH_PREFIX=/onlogs if want to use with path prefix
 
     labels:
@@ -93,6 +94,25 @@ Once done, just go to <your host> and login as "admin" with <any password>.
 | MAX_LOGS_SIZE | Maximum allowed total logs size before cleanup triggers. Accepts human-readable formats like 5GB, 500MB, 1.5GB etc. When exceeded, 10% of logs (by count) will be removed proportionally across containers starting from oldest. Validated at startup: an unparseable value stops OnLogs rather than silently disabling retention | 10GB | -
 | DISABLE_AUTH | Option to completely disable built in authentication in the application. When this option is set to `true` the app will behave like if the Administrator is logged in. The option to manage users will be removed. | false | -
 | METRICS_TOKEN | Bearer token for the Prometheus endpoint at `/api/v1/metrics`. While it is unset the endpoint returns `401` and exposes nothing, so metrics are off by default. See [Metrics](#metrics) | | only for `/api/v1/metrics`
+| TRUSTED_PROXIES | Peers allowed to name the client through `X-Forwarded-For` / `X-Real-IP`, as comma separated IPs and CIDR ranges. See [Behind a reverse proxy](#behind-a-reverse-proxy) | | only if behind nginx/traefik
+
+## Behind a reverse proxy
+
+Failed logins are rate limited per client address. Behind nginx or traefik every request
+arrives from the proxy, so without `TRUSTED_PROXIES` all your users count as one client and
+a single password sprayer locks out the rest. Set it to the addresses your proxy connects
+from:
+
+```
+TRUSTED_PROXIES=172.16.0.0/12    # docker networks land here, so any containerised proxy does too
+TRUSTED_PROXIES=127.0.0.1        # a proxy on the host
+```
+
+Keep the range as tight as your proxy allows — anything reaching OnLogs from a listed address
+can call itself any client.
+
+Make sure the proxy actually sends the headers — traefik does by default, nginx needs
+`proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;`.
 
 ## Metrics
 

application/backend/app/routes/loginlimit.go

@@ -3,8 +3,11 @@ package routes
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"fmt"
 	"net"
 	"net/http"
+	"net/netip"
+	"strings"
 	"sync"
 	"time"
 )
@@ -110,12 +113,121 @@ func (l *loginAttempts) prune(now time.Time) {
 	}
 }
 
-// RemoteAddr only: X-Forwarded-For is caller-controlled and would let an
-// attacker rotate past the limit.
+// Empty means no peer may speak for anyone else.
+var trustedProxies []netip.Prefix
+
+// SetTrustedProxies takes a comma separated list of IPs and CIDR ranges.
+func SetTrustedProxies(spec string) error {
+	parsed, err := parseTrustedProxies(spec)
+	if err != nil {
+		return err
+	}
+	trustedProxies = parsed
+	return nil
+}
+
+func parseTrustedProxies(spec string) ([]netip.Prefix, error) {
+	var parsed []netip.Prefix
+	for _, item := range strings.Split(spec, ",") {
+		item = strings.TrimSpace(item)
+		switch {
+		case item == "":
+		case strings.Contains(item, "/"):
+			prefix, err := netip.ParsePrefix(item)
+			if err != nil {
+				return nil, fmt.Errorf("%q is not a CIDR range: %w", item, err)
+			}
+			parsed = append(parsed, prefix.Masked())
+		default:
+			addr, err := netip.ParseAddr(item)
+			if err != nil {
+				return nil, fmt.Errorf("%q is not an IP address or CIDR range: %w", item, err)
+			}
+			addr = addr.Unmap()
+			parsed = append(parsed, netip.PrefixFrom(addr, addr.BitLen()))
+		}
+	}
+	return parsed, nil
+}
+
+func isTrustedProxy(addr netip.Addr) bool {
+	for _, prefix := range trustedProxies {
+		if prefix.Contains(addr) {
+			return true
+		}
+	}
+	return false
+}
+
+// Zones and the IPv4-in-IPv6 form are dropped so one client cannot hold two
+// buckets.
+func parseIP(value string) (netip.Addr, bool) {
+	value = strings.TrimSpace(value)
+	if addr, err := netip.ParseAddr(value); err == nil {
+		return addr.Unmap().WithZone(""), true
+	}
+	if addrPort, err := netip.ParseAddrPort(value); err == nil {
+		return addrPort.Addr().Unmap().WithZone(""), true
+	}
+	return netip.Addr{}, false
+}
+
+// The peer address, unless it is a trusted proxy: then the address it
+// forwarded. The headers are caller-controlled, so taking them from anyone
+// would let an attacker rotate past the limit or wear someone else's address.
 func clientAddr(req *http.Request) string {
 	host, _, err := net.SplitHostPort(req.RemoteAddr)
 	if err != nil {
-		return req.RemoteAddr
+		host = req.RemoteAddr
+	}
+
+	peer, ok := parseIP(host)
+	if !ok {
+		return host
+	}
+	if !isTrustedProxy(peer) {
+		warnUntrustedForward(req)
+		return peer.String()
+	}
+	if forwarded, ok := forwardedClient(req); ok {
+		return forwarded
+	}
+	return peer.String()
+}
+
+// Right to left: every hop appends the address it saw, so the rightmost entry
+// no trusted proxy could have written is the last one still worth believing.
+func forwardedClient(req *http.Request) (string, bool) {
+	entries := strings.Split(strings.Join(req.Header.Values("X-Forwarded-For"), ","), ",")
+	origin := ""
+	for i := len(entries) - 1; i >= 0; i-- {
+		addr, ok := parseIP(entries[i])
+		if !ok {
+			break
+		}
+		if !isTrustedProxy(addr) {
+			return addr.String(), true
+		}
+		origin = addr.String()
+	}
+
+	if addr, ok := parseIP(req.Header.Get("X-Real-IP")); ok {
+		return addr.String(), true
+	}
+	// Every hop was trusted, so the chain names its own origin.
+	return origin, origin != ""
+}
+
+var forwardWarning sync.Once
+
+// Either a proxy nobody told us about, whose users then share one bucket, or
+// someone trying on another address for size.
+func warnUntrustedForward(req *http.Request) {
+	if req.Header.Get("X-Forwarded-For") == "" && req.Header.Get("X-Real-IP") == "" {
+		return
 	}
-	return host
+	forwardWarning.Do(func() {
+		fmt.Printf("WARNING: a login from %s carried a forwarded client address, but that peer is not in TRUSTED_PROXIES;"+
+			" rate limiting keys on the peer, so everyone behind it shares one limit.\n", req.RemoteAddr)
+	})
 }

application/backend/app/routes/loginlimit_test.go

@@ -85,6 +85,83 @@ func TestLoginLimiterStillThrottlesRepeatedFailures(t *testing.T) {
 	_ = time.Second
 }
 
+func withTrustedProxies(t *testing.T, spec string) {
+	t.Helper()
+	if err := SetTrustedProxies(spec); err != nil {
+		t.Fatalf("SetTrustedProxies(%q): %v", spec, err)
+	}
+	t.Cleanup(func() { trustedProxies = nil })
+}
+
+func forwardedRequest(peer string, headers map[string]string) *http.Request {
+	req, _ := http.NewRequest("POST", "/api/v1/login", nil)
+	req.RemoteAddr = peer
+	for name, value := range headers {
+		req.Header.Set(name, value)
+	}
+	return req
+}
+
+func TestForwardedAddressesAreIgnoredFromUntrustedPeers(t *testing.T) {
+	req := forwardedRequest("203.0.113.9:40000", map[string]string{
+		"X-Forwarded-For": "198.51.100.7",
+		"X-Real-IP":       "198.51.100.7",
+	})
+
+	if got := clientAddr(req); got != "203.0.113.9" {
+		t.Fatalf("a caller wore another address with no proxy configured: %q", got)
+	}
+
+	// Trusting some other proxy is not trusting this caller.
+	withTrustedProxies(t, "192.0.2.1,10.0.0.0/8")
+	if got := clientAddr(req); got != "203.0.113.9" {
+		t.Fatalf("a caller outside TRUSTED_PROXIES wore another address: %q", got)
+	}
+}
+
+func TestClientAddrResolvesThroughTrustedProxies(t *testing.T) {
+	withTrustedProxies(t, "10.0.0.0/8,172.16.0.0/12")
+
+	cases := []struct {
+		name    string
+		peer    string
+		headers map[string]string
+		want    string
+	}{
+		{"single hop", "10.0.0.1:40000", map[string]string{"X-Forwarded-For": "198.51.100.7"}, "198.51.100.7"},
+		{"chained proxies", "10.0.0.1:40000", map[string]string{"X-Forwarded-For": "198.51.100.7, 10.0.0.5"}, "198.51.100.7"},
+		{"client prepended a fake hop", "10.0.0.1:40000", map[string]string{"X-Forwarded-For": "1.1.1.1, 198.51.100.7"}, "198.51.100.7"},
+		{"x-real-ip only", "10.0.0.1:40000", map[string]string{"X-Real-IP": "198.51.100.7"}, "198.51.100.7"},
+		{"entry carries a port", "10.0.0.1:40000", map[string]string{"X-Forwarded-For": "198.51.100.7:1234"}, "198.51.100.7"},
+		{"ipv4 in ipv6 form", "[::ffff:10.0.0.1]:40000", map[string]string{"X-Forwarded-For": "::ffff:198.51.100.7"}, "198.51.100.7"},
+		{"unparseable chain", "10.0.0.1:40000", map[string]string{"X-Forwarded-For": "not-an-address"}, "10.0.0.1"},
+		{"proxy forwarded nothing", "10.0.0.1:40000", nil, "10.0.0.1"},
+		{"internal end to end", "10.0.0.1:40000", map[string]string{"X-Forwarded-For": "10.9.9.9, 10.0.0.5"}, "10.9.9.9"},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := clientAddr(forwardedRequest(tc.peer, tc.headers)); got != tc.want {
+				t.Fatalf("got %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestTrustedProxySpecIsValidated(t *testing.T) {
+	if _, err := parseTrustedProxies(" 10.0.0.0/8 , 192.0.2.1,::1 "); err != nil {
+		t.Fatalf("a valid spec was rejected: %v", err)
+	}
+	if parsed, err := parseTrustedProxies(""); err != nil || parsed != nil {
+		t.Fatalf("an empty spec gave (%v, %v)", parsed, err)
+	}
+	for _, spec := range []string{"10.0.0.0/99", "not-an-address", "10.0.0.1-10.0.0.5", "10.0.0.0/8,junk"} {
+		if _, err := parseTrustedProxies(spec); err == nil {
+			t.Errorf("%q was accepted", spec)
+		}
+	}
+}
+
 // The lockout lived in which key the handler chose, so it has to be exercised
 // through the handler.
 func TestLoginLockoutCannotBeInflictedOnAnotherUser(t *testing.T) {
@@ -116,3 +193,33 @@ func TestLoginLockoutCannotBeInflictedOnAnotherUser(t *testing.T) {
 		t.Fatalf("an attacker guessing at the account locked its real owner out: status %d", code)
 	}
 }
+
+func TestOneAttackerBehindAProxyDoesNotThrottleEveryoneElse(t *testing.T) {
+	userdb.CreateUser("proxieduser", "the-real-password")
+	t.Cleanup(func() { userdb.DeleteUser("proxieduser") })
+	withTrustedProxies(t, "10.0.0.0/8,172.16.0.0/12")
+
+	loginLimiter.mu.Lock()
+	loginLimiter.entries = map[string]*loginAttempt{}
+	loginLimiter.mu.Unlock()
+
+	attempt := func(client, password string) int {
+		body, _ := json.Marshal(map[string]string{"Login": "proxieduser", "Password": password})
+		req, _ := http.NewRequest("POST", "/api/v1/login", bytes.NewBuffer(body))
+		req.RemoteAddr = "172.18.0.2:40000"
+		req.Header.Set("X-Forwarded-For", client)
+		rr := httptest.NewRecorder()
+		http.HandlerFunc(testCtrl.Login).ServeHTTP(rr, req)
+		return rr.Result().StatusCode
+	}
+
+	for i := 0; i < 20; i++ {
+		attempt("203.0.113.9", "guess-"+strconv.Itoa(i))
+	}
+	if code := attempt("203.0.113.9", "the-real-password"); code != http.StatusTooManyRequests {
+		t.Errorf("the attacker was not throttled through the proxy: status %d", code)
+	}
+	if code := attempt("198.51.100.7", "the-real-password"); code != http.StatusOK {
+		t.Fatalf("one attacker behind the proxy locked out everyone else: status %d", code)
+	}
+}

application/backend/main.go

@@ -84,6 +84,12 @@ func init_config() {
 		os.Exit(1)
 	}
 
+	if err := routes.SetTrustedProxies(os.Getenv("TRUSTED_PROXIES")); err != nil {
+		fmt.Printf("FATAL: TRUSTED_PROXIES=%q is invalid (%v); refusing to start with a rate limiter that cannot tell clients apart.\n",
+			os.Getenv("TRUSTED_PROXIES"), err)
+		os.Exit(1)
+	}
+
 	fmt.Println("INFO: OnLogs configs done!")
 }