From c172deee7272dc9967d3861e99a3f1849ed514df Mon Sep 17 00:00:00 2001 From: sneak Date: Thu, 20 Aug 2026 04:11:01 +0000 Subject: [PATCH] Fail startup on half-set metrics credentials (closes #205) METRICS_USERNAME alone mounted /metrics behind a credential map whose only password was the empty string, so `curl -u 'metrics:'` returned 200 while the startup log reported hasMetricsAuth:false. The route mount tested the username and the log tested both, so the two could disagree about whether the endpoint existed. Config.MetricsAuthEnabled is now the single value behind both: the /metrics mount, the Prometheus recording middleware and the startup log's hasMetricsAuth field all read it, and it requires both credentials. loadFromEnv rejects a half-set pair outright with an error naming both variables in either direction, so a set-but-invalid configuration aborts startup rather than degrading into an endpoint the operator did not ask for. Both unset stays valid and leaves /metrics unmounted. Tests cover all four combinations at the config layer, counting "set to the empty string" and "not set at all" as separate inputs, plus the route tree's behaviour in each state. --- README.md | 26 +++-- internal/config/config.go | 61 +++++++++++- internal/config/config_test.go | 171 +++++++++++++++++++++++++++++++++ internal/server/routes.go | 17 +++- internal/server/routes_test.go | 148 ++++++++++++++++++++++++++-- 5 files changed, 401 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 2bb6e23..06acc8b 100644 --- a/README.md +++ b/README.md @@ -107,14 +107,26 @@ TTY detection, and security headers are always applied. | `DATA_DIR` | Directory for all SQLite databases | `/var/lib/webhooker` | | `DEBUG` | Enable debug logging | `false` | | `MAINTENANCE_MODE` | Report `maintenanceMode: true` in the healthcheck JSON. It does not change how any request is served — no maintenance page exists | `false` | -| `METRICS_USERNAME` | Basic auth username for `/metrics` | `""` | -| `METRICS_PASSWORD` | Basic auth password for `/metrics` | `""` | +| `METRICS_USERNAME` | Basic auth username for `/metrics`. Must be set together with `METRICS_PASSWORD`; one without the other fails startup | `""` | +| `METRICS_PASSWORD` | Basic auth password for `/metrics`. Must be set together with `METRICS_USERNAME`; one without the other fails startup | `""` | | `SENTRY_DSN` | Sentry error reporting DSN | `""` | | `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) | +#### Metrics credentials + +`METRICS_USERNAME` and `METRICS_PASSWORD` are set together or not at +all. With both set, `/metrics` is served behind basic auth. With +neither set, the route is not registered and returns 404. With one set +and the other empty or unset, the process refuses to start and exits +non-zero with an error naming both variables — mounting the endpoint +on the username alone would publish it behind a password that is the +empty string, and quietly withholding it would deny an endpoint that +was asked for. The `hasMetricsAuth` field in the startup log and the +existence of the route are the same value, so they cannot disagree. + #### Trusted proxies `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare @@ -1682,7 +1694,7 @@ abuse limit later; they are tracked as future work. | Method | Path | Description | | ------ | ---------- | ----------- | -| `GET` | `/metrics` | Prometheus metrics, behind basic auth. The route is registered only when `METRICS_USERNAME` is set; otherwise it does not exist and returns 404 | +| `GET` | `/metrics` | Prometheus metrics, behind basic auth. The route is registered only when `METRICS_USERNAME` and `METRICS_PASSWORD` are both set; with neither set it does not exist and returns 404, and with only one set the process refuses to start | #### API (Planned) @@ -1837,7 +1849,8 @@ Applied to all routes in this order: Permissions-Policy) 3. **Logging** — Structured request logging (method, URL, status, latency, remote IP, user agent, request ID) -4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` is set) +4. **Metrics** — Prometheus HTTP metrics (if `METRICS_USERNAME` and + `METRICS_PASSWORD` are both set) 5. **CORS** — Cross-origin resource sharing headers 6. **Timeout** — 60-second request timeout 7. **Recoverer** — Panic recovery: one `ERROR` record through @@ -1869,8 +1882,9 @@ being read and without reaching CSRF, the route group's remaining middleware, or the handler. It is not rejected before *any* other middleware, though: the global entries listed above all run first, so such a request is still logged and given the security headers — and -counted in the metrics, on a deployment where `METRICS_USERNAME` is -set and the Metrics middleware is therefore registered at all. The +counted in the metrics, on a deployment where the `/metrics` +credentials are set and the Metrics middleware is therefore registered +at all. The rejection itself is logged at `WARN` with the method, path and declared length. A chunked request, or one that lies about its length, is hard-capped by diff --git a/internal/config/config.go b/internal/config/config.go index 0d5acb5..5e0d919 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -71,6 +71,16 @@ var ErrInvalidPort = errors.New("invalid port") // nor a bare IP address. var ErrInvalidCIDR = errors.New("invalid CIDR") +// ErrIncompleteMetricsAuth is returned when exactly one of +// METRICS_USERNAME and METRICS_PASSWORD carries a value. Neither +// fallback is acceptable: serving /metrics on the username alone +// publishes an endpoint whose password is the empty string, and +// silently leaving it unmounted withholds an endpoint the operator +// asked for. Half-set is a configuration error, so startup fails. +var ErrIncompleteMetricsAuth = errors.New( + "incomplete metrics credentials", +) + //nolint:revive // ConfigParams is a standard fx naming convention. type ConfigParams struct { fx.In @@ -128,6 +138,21 @@ func (c *Config) IsProd() bool { return c.Environment == EnvironmentProd } +// MetricsAuthEnabled reports whether /metrics is served behind basic +// auth. It is the only answer to that question in the codebase: the +// route mount, the Prometheus recording middleware and the startup +// log's hasMetricsAuth field all read this one method, so the log +// cannot report auth as off while the route is mounted. +// +// It requires both credentials rather than the username alone. +// loadFromEnv already rejects a half-set pair, but a Config built in +// code bypasses that, and the failure mode this guards is an endpoint +// mounted with a credential map whose only password is the empty +// string. +func (c *Config) MetricsAuthEnabled() bool { + return c.MetricsUsername != "" && c.MetricsPassword != "" +} + // envString returns the value of the named environment variable, // or an empty string if not set. func envString(key string) string { @@ -329,6 +354,30 @@ func envPrefixList(key string) ([]netip.Prefix, error) { return prefixes, nil } +// resolveMetricsAuth reads the /metrics basic-auth credentials and +// rejects a half-set pair, naming both variables either way. The +// error carries neither value: the password is a secret. +func resolveMetricsAuth() (string, string, error) { + username := envString("METRICS_USERNAME") + password := envString("METRICS_PASSWORD") + + if (username == "") == (password == "") { + return username, password, nil + } + + set, empty := "METRICS_USERNAME", "METRICS_PASSWORD" + if username == "" { + set, empty = empty, set + } + + return "", "", fmt.Errorf( + "%w: %s is set but %s is empty; METRICS_USERNAME and "+ + "METRICS_PASSWORD must both be set to serve /metrics, "+ + "or both be empty to leave it unmounted", + ErrIncompleteMetricsAuth, set, empty, + ) +} + // resolveEnvironment reads WEBHOOKER_ENVIRONMENT, defaulting to // dev, and rejects unrecognised values. func resolveEnvironment() (string, error) { @@ -406,13 +455,18 @@ func loadFromEnv() (*Config, error) { return nil, err } + metricsUsername, metricsPassword, err := resolveMetricsAuth() + if err != nil { + return nil, err + } + return &Config{ DataDir: envString("DATA_DIR"), Debug: debug, MaintenanceMode: maintenanceMode, Environment: environment, - MetricsUsername: envString("METRICS_USERNAME"), - MetricsPassword: envString("METRICS_PASSWORD"), + MetricsUsername: metricsUsername, + MetricsPassword: metricsPassword, Port: port, SentryDSN: envString("SENTRY_DSN"), RetentionSweepInterval: retentionSweepInterval, @@ -512,8 +566,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) { "receiverRateLimit", s.ReceiverRateLimit, "trustedProxies", len(s.TrustedProxies), "hasSentryDSN", s.SentryDSN != "", - "hasMetricsAuth", - s.MetricsUsername != "" && s.MetricsPassword != "", + "hasMetricsAuth", s.MetricsAuthEnabled(), ) s.warnSharedRateLimitBucket(log) diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d7b3bc7..8a89219 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -26,6 +26,12 @@ const ( // cidrPrivateV4 is the sample trusted-proxy block the // TRUSTED_PROXIES cases are built from. cidrPrivateV4 = "10.0.0.0/8" + + // metricsAuthValue is the sample METRICS_PASSWORD the metrics + // credential cases are built from. It is asserted absent from + // the startup error, so it must not be a substring of either + // variable name that error prints. + metricsAuthValue = "s3cret" ) func TestEnvironmentConfig(t *testing.T) { @@ -726,3 +732,168 @@ func TestSharedRateLimitBucketWarning(t *testing.T) { }) } } + +// metricsEnv describes what one subtest below puts in the +// environment for a single METRICS_ variable. A variable that is +// set to the empty string and one that is not set at all are +// distinct inputs here, because the reported bug arrived through +// the first of them. +type metricsEnv struct { + set bool + value string +} + +// unset leaves the variable out of the environment entirely. +func unset() metricsEnv { + return metricsEnv{set: false, value: ""} +} + +// setTo sets the variable, including to the empty string. +func setTo(value string) metricsEnv { + return metricsEnv{set: true, value: value} +} + +// metricsAuthCase is one row of the table in TestMetricsAuthConfig, +// named so the table can live in its own function and keep the test +// itself short. +type metricsAuthCase struct { + name string + username metricsEnv + password metricsEnv + expectError bool + expectAuth bool +} + +// metricsAuthCases enumerates every combination of the two +// credentials, counting "set to the empty string" and "not set at +// all" as separate inputs on each side. +func metricsAuthCases() []metricsAuthCase { + return []metricsAuthCase{ + { + name: "both unset leaves metrics unmounted", + username: unset(), + password: unset(), + }, + { + name: "both empty leaves metrics unmounted", + username: setTo(""), + password: setTo(""), + }, + { + name: "both set enables metrics auth", + username: setTo("metrics"), + password: setTo(metricsAuthValue), + expectAuth: true, + }, + { + name: "username with unset password fails", + username: setTo("metrics"), + password: unset(), + expectError: true, + }, + { + name: "username with empty password fails", + username: setTo("metrics"), + password: setTo(""), + expectError: true, + }, + { + name: "password with unset username fails", + username: unset(), + password: setTo(metricsAuthValue), + expectError: true, + }, + { + name: "password with empty username fails", + username: setTo(""), + password: setTo(metricsAuthValue), + expectError: true, + }, + } +} + +// TestMetricsAuthConfig covers every combination of METRICS_USERNAME +// and METRICS_PASSWORD. Either both carry a value, in which case +// /metrics is served behind basic auth, or neither does, in which +// case the route is never mounted. One without the other is a +// startup error rather than a fallback: mounting on the username +// alone published /metrics behind a credential map that accepted an +// empty password, which is the defect this test exists to pin. See +// https://git.eeqj.de/sneak/webhooker/issues/205. +func TestMetricsAuthConfig(t *testing.T) { + for _, tt := range metricsAuthCases() { + t.Run(tt.name, func(t *testing.T) { + // Cannot use t.Parallel() here because t.Setenv + // is incompatible with parallel subtests. + if tt.username.set { + t.Setenv("METRICS_USERNAME", tt.username.value) + } else { + require.NoError( + t, os.Unsetenv("METRICS_USERNAME"), + ) + } + + if tt.password.set { + t.Setenv("METRICS_PASSWORD", tt.password.value) + } else { + require.NoError( + t, os.Unsetenv("METRICS_PASSWORD"), + ) + } + + if tt.expectError { + assertMetricsAuthRejected(t) + + return + } + + assertMetricsAuthAccepted(t, tt.expectAuth) + }) + } +} + +// assertMetricsAuthRejected requires that fx refused to build the +// graph, that the failure is ErrIncompleteMetricsAuth, and that the +// operator is told both variable names — the point of failing here +// rather than degrading is that the message says what to fix. +func assertMetricsAuthRejected(t *testing.T) { + t.Helper() + + var cfg *config.Config + + app := fx.New( + fx.NopLogger, + fx.Provide(globals.New, logger.New, config.New), + fx.Populate(&cfg), + ) + + err := app.Err() + require.Error(t, err) + require.ErrorIs(t, err, config.ErrIncompleteMetricsAuth) + assert.Contains(t, err.Error(), "METRICS_USERNAME") + assert.Contains(t, err.Error(), "METRICS_PASSWORD") + // The password is a secret and must not reach a startup error. + assert.NotContains(t, err.Error(), metricsAuthValue) +} + +// assertMetricsAuthAccepted requires that startup succeeded and that +// MetricsAuthEnabled — the single value the /metrics mount and the +// startup log both read — reports what the environment asked for. +func assertMetricsAuthAccepted(t *testing.T, expectAuth bool) { + t.Helper() + + var cfg *config.Config + + app := fxtest.New( + t, + fx.Provide(globals.New, logger.New, config.New), + fx.Populate(&cfg), + ) + require.NoError(t, app.Err()) + + app.RequireStart() + + defer app.RequireStop() + + assert.Equal(t, expectAuth, cfg.MetricsAuthEnabled()) +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 2fbcd09..6f3d4c7 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -55,8 +55,11 @@ func (s *Server) setupGlobalMiddleware() { s.router.Use(s.mw.SecurityHeaders()) s.router.Use(s.mw.Logging()) - // Metrics middleware (only if credentials are configured) - if s.params.Config.MetricsUsername != "" { + // Metrics recording middleware, registered only when the + // endpoint that exposes what it records is served. The + // condition is the same MetricsAuthEnabled the /metrics mount + // in setupRoutes reads. + if s.params.Config.MetricsAuthEnabled() { s.router.Use(s.mw.Metrics()) } @@ -103,8 +106,14 @@ func (s *Server) setupRoutes() { s.h.HandleHealthCheck(), ) - // set up authenticated /metrics route: - if s.params.Config.MetricsUsername != "" { + // Authenticated /metrics route. The condition is + // Config.MetricsAuthEnabled and never the username alone: a + // username with an empty password would otherwise mount the + // endpoint behind a credential map that accepts an empty + // password. Config rejects that combination at startup, and + // this reads the same value the startup log reports, so the + // two cannot disagree about whether the route exists. + if s.params.Config.MetricsAuthEnabled() { s.router.Group(func(r chi.Router) { r.Use(s.mw.MetricsAuth()) r.Get( diff --git a/internal/server/routes_test.go b/internal/server/routes_test.go index 13fbd0d..087dee2 100644 --- a/internal/server/routes_test.go +++ b/internal/server/routes_test.go @@ -34,6 +34,13 @@ import ( // the CSRF middleware executed. const csrfCookieName = "_gorilla_csrf" +const ( + // metricsUser and metricsAuthValue are the /metrics basic-auth + // credentials the metrics routing tests below configure. + metricsUser = "metrics" + metricsAuthValue = "s3cret" +) + type noopNotifier struct{} func (n *noopNotifier) Notify([]delivery.Task) {} @@ -69,9 +76,23 @@ type testEnv struct { func newTestEnv(t *testing.T) *testEnv { t.Helper() + return newTestEnvWithConfig(t, &config.Config{ + DataDir: t.TempDir(), + Environment: config.EnvironmentDev, + }) +} + +// newTestEnvWithConfig is newTestEnv over a caller-supplied Config, +// for the routes whose existence the configuration decides. The same +// pointer reaches the router and every middleware, so a test cannot +// accidentally configure one and not the other. +func newTestEnvWithConfig( + t *testing.T, cfg *config.Config, +) *testEnv { + t.Helper() + var ( log *logger.Logger - cfg *config.Config mw *middleware.Middleware hnd *handlers.Handlers sess *session.Session @@ -84,12 +105,7 @@ func newTestEnv(t *testing.T) *testEnv { fx.Provide( globals.New, logger.New, - func() *config.Config { - return &config.Config{ - DataDir: t.TempDir(), - Environment: config.EnvironmentDev, - } - }, + func() *config.Config { return cfg }, database.New, database.NewWebhookDBManager, healthcheck.New, @@ -99,7 +115,7 @@ func newTestEnv(t *testing.T) *testEnv { middleware.New, handlers.New, ), - fx.Populate(&log, &cfg, &mw, &hnd, &sess, &db, &dbMgr), + fx.Populate(&log, &mw, &hnd, &sess, &db, &dbMgr), ) app.RequireStart() t.Cleanup(app.RequireStop) @@ -657,3 +673,119 @@ func TestSourceLogsBody_OtherUser404s(t *testing.T) { assert.Equal(t, http.StatusSeeOther, anon.Code) assert.Equal(t, "/pages/login", anon.Header().Get("Location")) } + +// metricsConfig is a Config differing from the routing default only +// in the two /metrics credentials. +func metricsConfig( + t *testing.T, username, password string, +) *config.Config { + t.Helper() + + return &config.Config{ + DataDir: t.TempDir(), + Environment: config.EnvironmentDev, + MetricsUsername: username, + MetricsPassword: password, + } +} + +// metricsRequest asks the real router for /metrics with the given +// basic-auth credentials, or with no Authorization header when +// username is empty. +func (e *testEnv) metricsRequest( + username, password string, +) *httptest.ResponseRecorder { + req := httptest.NewRequestWithContext( + context.Background(), http.MethodGet, "/metrics", nil, + ) + + if username != "" { + req.SetBasicAuth(username, password) + } + + w := httptest.NewRecorder() + e.router.ServeHTTP(w, req) + + return w +} + +// TestMetricsRouteUnmountedWithoutCredentials pins that with neither +// credential configured the route does not exist, which is the +// documented behaviour and the only valid way for /metrics to be +// absent. +func TestMetricsRouteUnmountedWithoutCredentials(t *testing.T) { + t.Parallel() + + env := newTestEnvWithConfig(t, metricsConfig(t, "", "")) + + assert.Equal( + t, http.StatusNotFound, + env.metricsRequest("", "").Code, + ) +} + +// TestMetricsRouteRequiresCredentials pins that with both credentials +// configured the route exists and every request that does not carry +// the configured pair is refused — including the empty password that +// a half-set configuration used to make sufficient. +func TestMetricsRouteRequiresCredentials(t *testing.T) { + t.Parallel() + + env := newTestEnvWithConfig( + t, metricsConfig(t, metricsUser, metricsAuthValue), + ) + + assert.Equal( + t, http.StatusUnauthorized, + env.metricsRequest("", "").Code, + "no credentials must not reach the metrics handler", + ) + assert.Equal( + t, http.StatusUnauthorized, + env.metricsRequest(metricsUser, "").Code, + "an empty password must not reach the metrics handler", + ) + assert.Equal( + t, http.StatusUnauthorized, + env.metricsRequest(metricsUser, "wrong").Code, + ) + + ok := env.metricsRequest(metricsUser, metricsAuthValue) + assert.Equal(t, http.StatusOK, ok.Code) + assert.Contains(t, ok.Body.String(), "go_goroutines") +} + +// TestMetricsRouteUnmountedOnHalfSetConfig pins the defect from +// https://git.eeqj.de/sneak/webhooker/issues/205 at the routing +// layer. Config rejects a half-set pair at startup, so this Config +// cannot be reached from the environment; the assertion is that the +// route tree does not publish an endpoint accepting an empty +// password even when handed one anyway, because the mount and the +// startup log's hasMetricsAuth read the same value. +func TestMetricsRouteUnmountedOnHalfSetConfig(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + username string + password string + }{ + {name: "username only", username: metricsUser}, + {name: "password only", password: metricsAuthValue}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + cfg := metricsConfig(t, tc.username, tc.password) + env := newTestEnvWithConfig(t, cfg) + + assert.False(t, cfg.MetricsAuthEnabled()) + assert.Equal( + t, http.StatusNotFound, + env.metricsRequest( + tc.username, tc.password, + ).Code, + ) + }) + } +} -- 2.49.1