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, + ) + }) + } +}