From d40e67d4abec4d3293934b0d34b3f88bd2aec102 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Sun, 4 Oct 2026 06:19:06 +0200 Subject: [PATCH] Rate limit password attempts on /metrics (closes #104) Each client address may make 60 requests to /metrics a minute, through the same httprate middleware and TRUSTED_PROXIES resolution the report route uses, with an allowance of its own. The limit runs before the basic auth, so past it the answer is 429 and the password is not checked. backend/README.md says so; a test uses up one client's allowance on wrong passwords, gets 429 with the right one, and checks that another client behind the same nginx still gets in. Model: opus-5-5 --- TODO.md | 7 ++++ backend/README.md | 8 +++++ backend/internal/server/export_test.go | 4 +++ backend/internal/server/routes.go | 18 ++++++++--- backend/internal/server/routes_test.go | 44 ++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 4 deletions(-) diff --git a/TODO.md b/TODO.md index 6b802e0..555d543 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,13 @@ latest run passes. # Completed Steps +- 2026-10-04: password guesses at `/metrics` are rate limited (issue #104): each + client address, resolved through `TRUSTED_PROXIES` as for reports, may make 60 + requests to `/metrics` a minute, counted by `go-chi/httprate` apart from its + reports; past that it gets 429 and its basic auth credentials are not checked. + The limit is a constant in `backend/internal/server/routes.go`. A test uses up + one client's allowance on wrong passwords, gets 429 with the right one, and + checks that another client behind the same nginx still gets in - 2026-10-04: the backend reports errors to Sentry (issue #95). With `SENTRY_DSN` set, it sets up `sentry-go` with the release `netwatch-server-` and its version, reports each panic in a handler through `sentryhttp`, the diff --git a/backend/README.md b/backend/README.md index 5ac252d..0cbefe9 100644 --- a/backend/README.md +++ b/backend/README.md @@ -199,6 +199,14 @@ is recorded and `/metrics` answers 404. One without the other stops the server from starting, with an error naming both; so does a `METRICS_USERNAME` containing `:`, which basic auth cannot carry, with an error naming it. +`/metrics` is rate limited, so that its password cannot be guessed quickly: each +client address, resolved through `TRUSTED_PROXIES`, may make 60 requests to it a +minute, whatever their credentials. Past that it gets 429 with +`Retry-After: 60`, and its credentials are not checked. The minute slides as it +does for reports (see [Report limits](#report-limits)), so a scraper polling +every 2 seconds or less often is never refused. This allowance is apart from the +one for reports. + ### Sentry With `SENTRY_DSN` set, the server sends its errors to that Sentry project: each diff --git a/backend/internal/server/export_test.go b/backend/internal/server/export_test.go index 01e263e..a113052 100644 --- a/backend/internal/server/export_test.go +++ b/backend/internal/server/export_test.go @@ -12,6 +12,10 @@ func (s *Server) Router() *chi.Mux { // external tests. const MaxRequestBodyBytes = maxRequestBodyBytes +// MetricsRequestsPerMinute exposes the /metrics rate limit to the +// external tests. +const MetricsRequestsPerMinute = metricsRequestsPerMinute + // ListenAddr exposes the address the server listens on to the // external tests. func (s *Server) ListenAddr() string { diff --git a/backend/internal/server/routes.go b/backend/internal/server/routes.go index 836bbf2..d13ecf6 100644 --- a/backend/internal/server/routes.go +++ b/backend/internal/server/routes.go @@ -18,6 +18,12 @@ const ( // can mount s.mw.MaxBodyBytes with a smaller value to lower // its bound, but cannot raise it: this cap runs first. maxRequestBodyBytes int64 = 1 << 20 // 1 MiB + + // metricsRequestsPerMinute is how many requests to /metrics each + // client address may make a minute, whatever their credentials. A + // scraper polling every 2 seconds sends half of it, which httprate + // never refuses. + metricsRequestsPerMinute = 60 ) // SetupRoutes configures the chi router with middleware and @@ -66,10 +72,14 @@ func (s *Server) SetupRoutes() { Post("/api/v1/reports", s.h.HandleReport()) }) + // The rate limit comes before the basic auth, so a client past it + // gets 429 and its password is not checked. if s.params.Config.MetricsUsername != "" { - s.router.With(s.mw.MetricsAuth()). - Get("/metrics", promhttp.HandlerFor( - registry, promhttp.HandlerOpts{}, - ).ServeHTTP) + s.router.With( + s.mw.RateLimit(metricsRequestsPerMinute), + s.mw.MetricsAuth(), + ).Get("/metrics", promhttp.HandlerFor( + registry, promhttp.HandlerOpts{}, + ).ServeHTTP) } } diff --git a/backend/internal/server/routes_test.go b/backend/internal/server/routes_test.go index d39e86d..8e076cf 100644 --- a/backend/internal/server/routes_test.go +++ b/backend/internal/server/routes_test.go @@ -188,6 +188,50 @@ func TestMetricsBehindBasicAuth(t *testing.T) { } } +// TestMetricsAreRateLimited: a client that has used up its /metrics +// allowance on wrong passwords gets 429 even with the right one, which +// is then not checked, while another client behind the same nginx +// still gets in. +func TestMetricsAreRateLimited(t *testing.T) { + t.Setenv("METRICS_USERNAME", "prometheus") + t.Setenv("METRICS_PASSWORD", "right") + // As in the container: nginx connects from loopback and names the + // client in X-Forwarded-For. + t.Setenv("TRUSTED_PROXIES", "127.0.0.1/32") + + srv := newServer(t) + srv.SetupRoutes() + + get := func(client, password string) int { + rec := httptest.NewRecorder() + req := httptest.NewRequestWithContext(t.Context(), + http.MethodGet, "/metrics", http.NoBody) + req.RemoteAddr = "127.0.0.1:40000" + req.Header.Set("X-Forwarded-For", client) + req.SetBasicAuth("prometheus", password) + srv.ServeHTTP(rec, req) + + return rec.Code + } + + for i := range server.MetricsRequestsPerMinute { + if code := get("203.0.113.7", "wrong"); code != http.StatusUnauthorized { + t.Fatalf("guess %d: status = %d, want %d", + i+1, code, http.StatusUnauthorized) + } + } + + if code := get("203.0.113.7", "right"); code != http.StatusTooManyRequests { + t.Fatalf("right password past the limit: status = %d, want %d", + code, http.StatusTooManyRequests) + } + + if code := get("203.0.113.8", "right"); code != http.StatusOK { + t.Fatalf("another client: status = %d, want %d", + code, http.StatusOK) + } +} + // TestMetricsInTwoServers: two servers in one process can both have // metrics on. func TestMetricsInTwoServers(t *testing.T) {