Log the client address next to the peer address (closes #270)

The access log, the rate-limit rejection lines, the CSRF warning and
the receiver's "webhook request received" line now carry clientIP,
the address the rate limiters key on, next to remoteIP, the
connecting peer. Logging works it out once per request from the same
code the rate limiters use and stores it on the request context for
the other lines. The CSRF and receiver lines name the peer as
remoteIP instead of remote_addr. The README documents the field and
that it is only as trustworthy as TRUSTED_PROXIES.

Model: opus-5-5

internal/middleware/accesslog_test.go

@@ -63,6 +63,11 @@ const (
 // capturingMiddleware returns a Middleware whose logger writes JSON
 // lines into the returned buffer, so the access log can be asserted
 // on directly.
+//
+// It trusts 192.0.2.1, the peer httptest sends every request from, as
+// a proxy, the way a deployment trusts its reverse proxy: a request
+// carrying X-Forwarded-For is logged with the client that header names
+// as clientIP, and one without it with the peer.
 func capturingMiddleware(t *testing.T) (*middleware.Middleware, *bytes.Buffer) {
 	t.Helper()
 
@@ -72,7 +77,10 @@ func capturingMiddleware(t *testing.T) (*middleware.Middleware, *bytes.Buffer) {
 		&slog.HandlerOptions{Level: slog.LevelInfo},
 	))
 
-	cfg := &config.Config{Environment: config.EnvironmentDev}
+	cfg := &config.Config{
+		Environment:    config.EnvironmentDev,
+		TrustedProxies: trustedProxies("192.0.2.1/32"),
+	}
 
 	return middleware.NewForTest(log, cfg, nil), buf
 }
@@ -81,7 +89,7 @@ func capturingMiddleware(t *testing.T) (*middleware.Middleware, *bytes.Buffer) {
 // internal/logger can select: slog's text handler, which
 // internal/logger/logger.go installs when stderr is a tty. It escapes
 // differently from the JSON one, so the line bound has to be asserted
-// against both.
+// against both. It trusts the same peer.
 func capturingTextMiddleware(
 	t *testing.T,
 ) (*middleware.Middleware, *bytes.Buffer) {
@@ -93,7 +101,10 @@ func capturingTextMiddleware(
 		&slog.HandlerOptions{Level: slog.LevelInfo},
 	))
 
-	cfg := &config.Config{Environment: config.EnvironmentDev}
+	cfg := &config.Config{
+		Environment:    config.EnvironmentDev,
+		TrustedProxies: trustedProxies("192.0.2.1/32"),
+	}
 
 	return middleware.NewForTest(log, cfg, nil), buf
 }
@@ -334,11 +345,12 @@ func oversizedHeaders(value string) map[string]string {
 // sizeCase is one way of pointing 8 KB of client-chosen text at the
 // access log.
 type sizeCase struct {
-	target     string
-	headers    map[string]string
-	wantStatus int
-	wantURL    string
-	bound      int
+	target       string
+	headers      map[string]string
+	wantStatus   int
+	wantURL      string
+	wantClientIP string
+	bound        int
 }
 
 // lineSizeCases enumerates every part of a request that reaches the
@@ -375,8 +387,7 @@ func lineSizeCases() map[string]sizeCase {
 	}
 
 	// The url field on a 5xx keeps the concrete path, so it reaches its
-	// own budget on the same line as the three header fields. That is
-	// the widest access log line the service can be made to write.
+	// own budget on the same line as the three header fields.
 	longPath := "/boom/" + strings.Repeat("x", oversizedSegmentBytes)
 	wantLongURL := longPath[:maxFieldBytes] + truncationSuffix
 
@@ -420,6 +431,26 @@ func lineSizeCases() map[string]sizeCase {
 			}
 	}
 
+	// From a trusted proxy, clientIP is read out of X-Forwarded-For,
+	// which the client writes. Only the address the header ends in may
+	// reach the line, and an IPv6 address with all eight groups at four
+	// digits is that address at its longest. It goes on the 5xx line
+	// with all three header fields at their budget.
+	const longestIPv6 = "ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff"
+
+	forwarded := oversizedHeaders(oversizedValue("h"))
+	forwarded[headerXFF] = oversizedValue("h") + ", " + longestIPv6
+
+	cases["oversized X-Forwarded-For from a trusted proxy "+
+		"with a 5xx concrete url"] = sizeCase{
+		target:       longPath,
+		headers:      forwarded,
+		wantStatus:   http.StatusInternalServerError,
+		wantURL:      wantLongURL,
+		wantClientIP: longestIPv6,
+		bound:        maxCappedLineBytes,
+	}
+
 	return cases
 }
 
@@ -460,6 +491,14 @@ func TestAccessLog_LineSizeDoesNotTrackInputSize(t *testing.T) {
 			require.Len(t, entries, 1)
 			assert.Equal(t, tc.wantURL, entries[0]["url"])
 
+			// Set only by the X-Forwarded-For case, where it proves
+			// the header was read rather than ignored.
+			if tc.wantClientIP != "" {
+				assert.Equal(
+					t, tc.wantClientIP, entries[0]["clientIP"],
+				)
+			}
+
 			// The markers sit at the far end of the client-chosen
 			// text, so their absence is what proves the redaction and
 			// the truncation actually ran.
@@ -648,7 +687,8 @@ func TestAccessLog_RetainsEveryOtherField(t *testing.T) {
 
 	for _, key := range []string{
 		"request_start", "method", "url", "useragent", "request_id",
-		"referer", "proto", "remoteIP", "status", "latency_ms",
+		"referer", "proto", "remoteIP", "clientIP", "status",
+		"latency_ms",
 	} {
 		assert.Contains(t, entries[0], key)
 	}

internal/middleware/clientip_test.go

@@ -0,0 +1,200 @@
+package middleware_test
+
+import (
+	"bytes"
+	"context"
+	"log/slog"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/middleware"
+)
+
+const (
+	// forwardedChain is the X-Forwarded-For a request arrives with:
+	// the client, then a second proxy inside trustedProxyCIDR that the
+	// request passed through before reaching trustedPeer.
+	forwardedChain = clientIPv4 + ", 10.0.0.2"
+
+	// untrustedPeer is a peer outside trustedProxyCIDR, so its
+	// X-Forwarded-For is ignored and the peer is the client.
+	untrustedPeer = "192.0.2.10:5555"
+
+	// oneRequestPerMinute is the receiver limit these tests install:
+	// the second request on a path is rejected, and the aggregate
+	// limit is ReceiverAggregateMultiplierConst.
+	oneRequestPerMinute = 1
+)
+
+// clientLogSite is one log line that names the client. build wraps the
+// middleware that writes it around a handler, and requests is how many
+// identical requests it takes before the line is written.
+type clientLogSite struct {
+	build    func(m *middleware.Middleware) http.Handler
+	requests int
+}
+
+// clientLogSites maps the message of each line that names the client
+// to the way to make it be written.
+func clientLogSites() map[string]clientLogSite {
+	served := func(*middleware.Middleware) http.Handler {
+		return okHandler()
+	}
+
+	receiver := func(m *middleware.Middleware) http.Handler {
+		return m.ReceiverRateLimit()(okHandler())
+	}
+
+	login := func(m *middleware.Middleware) http.Handler {
+		return http.HandlerFunc(
+			func(w http.ResponseWriter, r *http.Request) {
+				m.RecordLoginFailure(r, "someone")
+				w.WriteHeader(http.StatusUnauthorized)
+			},
+		)
+	}
+
+	csrf := func(m *middleware.Middleware) http.Handler {
+		return m.CSRF(http.HandlerFunc(forbidden))(okHandler())
+	}
+
+	passwordChange := func(m *middleware.Middleware) http.Handler {
+		return m.PasswordChangeRateLimit()(okHandler())
+	}
+
+	replay := func(m *middleware.Middleware) http.Handler {
+		return m.ReplayRateLimit()(okHandler())
+	}
+
+	resubmit := func(m *middleware.Middleware) http.Handler {
+		return m.ResubmitRateLimit()(okHandler())
+	}
+
+	return map[string]clientLogSite{
+		"http request": {
+			build:    served,
+			requests: 1,
+		},
+		"webhook receiver rate limit exceeded": {
+			build:    receiver,
+			requests: oneRequestPerMinute + 1,
+		},
+		// The aggregate limit sits in front of the per-entrypoint
+		// one, so the requests that one rejects count towards it.
+		"webhook receiver aggregate rate limit exceeded": {
+			build: receiver,
+			requests: middleware.ReceiverAggregateMultiplierConst*
+				oneRequestPerMinute + 1,
+		},
+		"login failure limit exceeded": {
+			build:    login,
+			requests: middleware.LoginRateLimitConst + 1,
+		},
+		"csrf: token validation failed": {
+			build:    csrf,
+			requests: 1,
+		},
+		"password change rate limit exceeded": {
+			build:    passwordChange,
+			requests: middleware.PasswordChangeRateLimitConst + 1,
+		},
+		"delivery replay rate limit exceeded": {
+			build:    replay,
+			requests: middleware.ReplayRateLimitConst + 1,
+		},
+		"event resubmit rate limit exceeded": {
+			build:    resubmit,
+			requests: middleware.ResubmitRateLimitConst + 1,
+		},
+	}
+}
+
+// clientLogLines sends the site's requests from peer, each carrying
+// forwardedChain, through Logging and then the site, as production
+// does, and returns the logged lines whose message is msg.
+func clientLogLines(
+	t *testing.T, site clientLogSite, msg, peer string,
+) []map[string]any {
+	t.Helper()
+
+	buf := new(bytes.Buffer)
+	log := slog.New(slog.NewJSONHandler(
+		buf,
+		&slog.HandlerOptions{Level: slog.LevelDebug},
+	))
+
+	cfg := &config.Config{
+		Environment:       config.EnvironmentDev,
+		ReceiverRateLimit: oneRequestPerMinute,
+		TrustedProxies:    trustedProxies(trustedProxyCIDR),
+	}
+
+	m := middleware.NewForTest(
+		log, cfg, newTestSessionManager(cfg, log, nil),
+	)
+	handler := m.Logging()(site.build(m))
+
+	for range site.requests {
+		req := httptest.NewRequestWithContext(
+			context.Background(), http.MethodPost, "/h/x", nil,
+		)
+		req.RemoteAddr = peer
+		req.Header.Set(headerXFF, forwardedChain)
+
+		handler.ServeHTTP(httptest.NewRecorder(), req)
+	}
+
+	var lines []map[string]any
+
+	for _, entry := range accessLogEntries(t, buf) {
+		if entry["msg"] == msg {
+			lines = append(lines, entry)
+		}
+	}
+
+	return lines
+}
+
+// TestClientIP_LoggedNextToThePeer checks that every line that names
+// the client carries both addresses: remoteIP, the connecting peer,
+// and clientIP, the client the rate limiters key on.
+func TestClientIP_LoggedNextToThePeer(t *testing.T) {
+	t.Parallel()
+
+	cases := map[string]struct {
+		peer       string
+		wantRemote string
+		wantClient string
+	}{
+		"trusted proxy with a forwarded chain": {
+			peer:       trustedPeer,
+			wantRemote: "10.0.0.1",
+			wantClient: clientIPv4,
+		},
+		"untrusted peer": {
+			peer:       untrustedPeer,
+			wantRemote: "192.0.2.10",
+			wantClient: "192.0.2.10",
+		},
+	}
+
+	for msg, site := range clientLogSites() {
+		for name, tc := range cases {
+			t.Run(msg+"/"+name, func(t *testing.T) {
+				t.Parallel()
+
+				lines := clientLogLines(t, site, msg, tc.peer)
+				require.NotEmpty(t, lines, "%q was never logged", msg)
+
+				for _, line := range lines {
+					assert.Equal(t, tc.wantRemote, line["remoteIP"])
+					assert.Equal(t, tc.wantClient, line["clientIP"])
+				}
+			})
+		}
+	}
+}

internal/middleware/csrf.go

@@ -45,10 +45,10 @@ func (m *Middleware) CSRF(
 		// unauthenticated client: a POST with no token to
 		// /hook/<any length of any text>/edit lands here. The
 		// method and path are capped against the same budgets as
-		// the access log. remote_addr is set by net/http from the
-		// accepted connection rather than by the client, and
+		// the access log. remoteIP and clientIP are the same
+		// addresses the access log carries, and
 		// csrf.FailureReason returns one of gorilla/csrf's own
-		// fixed error values, so neither is client-sized.
+		// fixed error values, so none of them is client-sized.
 		m.log.Warn("csrf: token validation failed",
 			"method", logfield.Truncate(
 				r.Method, maxLogMethodBytes,
@@ -56,7 +56,8 @@ func (m *Middleware) CSRF(
 			"path", logfield.Truncate(
 				r.URL.Path, logfield.MaxBytes,
 			),
-			"remote_addr", r.RemoteAddr,
+			"remoteIP", RemoteIP(r),
+			"clientIP", ClientIP(r),
 			"reason", csrf.FailureReason(r),
 		)
 		forbidden.ServeHTTP(w, r)

internal/middleware/export_test.go

@@ -132,6 +132,12 @@ func (g *LoginGuard) TrackedKeysForTest() (int, int) {
 // passwordChangeRateLimit constant.
 const PasswordChangeRateLimitConst = passwordChangeRateLimit
 
+// ReplayRateLimitConst exposes the replayRateLimit constant.
+const ReplayRateLimitConst = replayRateLimit
+
+// ResubmitRateLimitConst exposes the resubmitRateLimit constant.
+const ResubmitRateLimitConst = resubmitRateLimit
+
 // ReceiverAggregateMultiplierConst exposes the
 // receiverAggregateMultiplier constant.
 const ReceiverAggregateMultiplierConst = receiverAggregateMultiplier

internal/middleware/loginguard.go

@@ -385,6 +385,8 @@ func (m *Middleware) RecordLoginFailure(
 			"path", logfield.Truncate(
 				r.URL.Path, logfield.MaxBytes,
 			),
+			"remoteIP", RemoteIP(r),
+			"clientIP", ClientIP(r),
 		)
 	}
 

internal/middleware/middleware.go

@@ -3,6 +3,7 @@
 package middleware
 
 import (
+	"context"
 	"log/slog"
 	"net"
 	"net/http"
@@ -69,18 +70,19 @@ const (
 	//	url, useragent, referer   3*(512+11) = 1569
 	//	request_id                    128+11 =  139
 	//	method                         32+11 =   43
-	//	fixed portion                        =  336
+	//	fixed portion                        =  405
 	//	                                        ----
-	//	                                        2087
+	//	                                        2156
 	//
 	// The 512 is logfield.MaxBytes; the 11 is the truncation marker,
 	// charged on top of each budget rather than inside it.
 	//
 	// The fixed portion is the JSON punctuation, the field names, the
-	// level and the message, both timestamps at their longest, an IPv6
-	// remoteIP with a zone, a three-digit status and a full-width int64
-	// latency. Stated at 2560 so the figure carries headroom rather
-	// than sitting on the arithmetic.
+	// level and the message, both timestamps at their longest, remoteIP
+	// and clientIP each charged as an IPv6 address with a zone, a
+	// three-digit status and a full-width int64 latency. Stated at 2560
+	// so the figure carries headroom rather than sitting on the
+	// arithmetic.
 	//
 	// The tty text handler in internal/logger is covered by the same
 	// figure. logfield.EncodedBytes charges every rune at least what
@@ -88,8 +90,8 @@ const (
 	// bytes strconv.Quote spends on a non-printable rune at or above
 	// U+10000, which is four more than the JSON handler ever spends —
 	// so each budget bounds the encoded field under either handler.
-	// The text handler's fixed portion is 286, the smaller of the two,
-	// which puts its worst case at 2037.
+	// The text handler's fixed portion is 351, the smaller of the two,
+	// which puts its worst case at 2102.
 	//
 	// It is also the ceiling on every OTHER line this service writes
 	// THROUGH SLOG that carries text an UNAUTHENTICATED client
@@ -215,6 +217,28 @@ func ipFromHostPort(hp string) string {
 	return h
 }
 
+// RemoteIP returns the address of the connecting peer, without its
+// port. Behind a reverse proxy it is the proxy. Every log line that
+// names the client logs it as remoteIP, next to clientIP.
+func RemoteIP(r *http.Request) string {
+	return ipFromHostPort(r.RemoteAddr)
+}
+
+// clientIPKey is the request context key under which Logging stores
+// the value ClientIP returns.
+type clientIPKey struct{}
+
+// ClientIP returns the address the request is attributed to, which
+// Logging works out once per request with clientAddr in ratelimit.go
+// and logs as clientIP. The other lines that name the client read it
+// from here, so all of them agree. It is empty for a request Logging
+// has not seen.
+func ClientIP(r *http.Request) string {
+	ip, _ := r.Context().Value(clientIPKey{}).(string)
+
+	return ip
+}
+
 type loggingResponseWriter struct {
 	http.ResponseWriter
 
@@ -316,6 +340,13 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler {
 			lrw := newLoggingResponseWriter(w)
 			ctx := r.Context()
 
+			// When RemoteAddr is not an address, the peer's own
+			// text is all the request can be attributed to.
+			clientIP := RemoteIP(r)
+			if addr, ok := s.clientAddr(r); ok {
+				clientIP = addr.String()
+			}
+
 			defer func() {
 				latency := time.Since(start)
 				requestID := ""
@@ -350,13 +381,16 @@ func (s *Middleware) Logging() func(http.Handler) http.Handler {
 						r.Referer(), logfield.MaxBytes,
 					),
 					"proto", r.Proto,
-					"remoteIP", ipFromHostPort(r.RemoteAddr),
+					"remoteIP", RemoteIP(r),
+					"clientIP", clientIP,
 					"status", lrw.statusCode,
 					"latency_ms", latency.Milliseconds(),
 				)
 			}()
 
-			next.ServeHTTP(lrw, r)
+			next.ServeHTTP(lrw, r.WithContext(
+				context.WithValue(ctx, clientIPKey{}, clientIP),
+			))
 		})
 	}
 }

internal/middleware/ratelimit.go

@@ -202,24 +202,17 @@ func (m *Middleware) forwardedClientAddr(
 }
 
 // rateLimitKey is the client identity every rate limiter in this
-// package buckets on. Forwarded headers are honoured only when the
-// direct peer (RemoteAddr) is inside the configured trusted-proxy
-// set; otherwise the peer address itself is the key. Without that
-// gate any client could mint a fresh bucket per request, or starve
-// another client's bucket, by picking an X-Forwarded-For value —
-// which makes every limit here decorative against a deliberate
-// attacker.
-//
-// The address that identifies the client is then reduced to a bucket
-// by bucketKey: full address for IPv4, /64 prefix for IPv6.
+// package buckets on: the address clientAddr attributes the request
+// to, reduced to a bucket by bucketKey — full address for IPv4, /64
+// prefix for IPv6.
 func (m *Middleware) rateLimitKey(r *http.Request) (string, error) {
 	return m.clientKey(r), nil
 }
 
 // clientKey computes the bucket key described on rateLimitKey.
 func (m *Middleware) clientKey(r *http.Request) string {
-	peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
-	if err != nil {
+	addr, ok := m.clientAddr(r)
+	if !ok {
 		// Not an address we can reason about; key on the raw
 		// value, the most specific identity left. Distinct
 		// RemoteAddr values stay in distinct buckets, so this
@@ -230,16 +223,36 @@ func (m *Middleware) clientKey(r *http.Request) string {
 		return r.RemoteAddr
 	}
 
+	return bucketKey(addr)
+}
+
+// clientAddr is the address a request is attributed to. The rate
+// limiters key on it and the logs name it as clientIP.
+//
+// Forwarded headers are honoured only when the direct peer
+// (RemoteAddr) is inside the configured trusted-proxy set; otherwise
+// the peer address itself is the client. Without that gate any client
+// could mint a fresh bucket per request, or starve another client's
+// bucket, by picking an X-Forwarded-For value — which makes every
+// limit here decorative against a deliberate attacker.
+//
+// ok is false when RemoteAddr is not an address at all.
+func (m *Middleware) clientAddr(r *http.Request) (netip.Addr, bool) {
+	peer, err := netip.ParseAddr(ipFromHostPort(r.RemoteAddr))
+	if err != nil {
+		return netip.Addr{}, false
+	}
+
 	peer = normalizeAddr(peer)
 	if !m.isTrustedProxy(peer) {
-		return bucketKey(peer)
+		return peer, true
 	}
 
 	if addr, ok := m.forwardedClientAddr(r); ok {
-		return bucketKey(addr)
+		return addr, true
 	}
 
-	return bucketKey(peer)
+	return peer, true
 }
 
 // tooManyRequests returns the 429 handler used by the
@@ -262,6 +275,8 @@ func (m *Middleware) tooManyRequests(
 			"path", logfield.Truncate(
 				r.URL.Path, logfield.MaxBytes,
 			),
+			"remoteIP", RemoteIP(r),
+			"clientIP", ClientIP(r),
 		)
 		http.Error(w, responseMessage, http.StatusTooManyRequests)
 	}
@@ -286,8 +301,12 @@ func (m *Middleware) tooManyRequests(
 func (m *Middleware) floodTooManyRequests(
 	logMessage, responseMessage string,
 ) http.HandlerFunc {
-	return func(w http.ResponseWriter, _ *http.Request) {
-		m.log.Debug(logMessage)
+	return func(w http.ResponseWriter, r *http.Request) {
+		m.log.Debug(
+			logMessage,
+			"remoteIP", RemoteIP(r),
+			"clientIP", ClientIP(r),
+		)
 		http.Error(w, responseMessage, http.StatusTooManyRequests)
 	}
 }