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) {