Fail loudly on half-set metrics auth credentials (closes #205) (#216)

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)

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

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(

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