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

Merged

clawbot merged 1 commits from issue-205-metrics-auth-fail-loud into next 2026-08-20 06:30:25 +02:00

Conversation 1 Commits 1 Files Changed 5 +401 -22

clawbot commented 2026-08-20 06:19:18 +02:00

Collaborator

Copy Link

Closes #205.

The defect

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 (internal/server/routes.go), the log tested both variables (internal/config/config.go), so the two could disagree about whether the endpoint existed.

Approach

One value behind both. Config.MetricsAuthEnabled() is now the only answer to "is /metrics served": the route mount, the Prometheus recording middleware and the startup log's hasMetricsAuth field all read it. It requires both credentials, not the username alone, so a Config assembled in code (which bypasses environment validation) still cannot publish an endpoint that accepts an empty password.

Fail loudly on half-set. loadFromEnv rejects a half-set pair via a new ErrIncompleteMetricsAuth, naming both variables in either direction. A set-but-invalid configuration aborts startup rather than degrading into something the operator did not ask for. Neither fallback was acceptable: mounting on the username alone publishes an endpoint with an empty password, and quietly leaving it unmounted withholds an endpoint that was asked for. The error carries neither credential value — the password is a secret.

Both unset remains valid and leaves /metrics unmounted.

Definition of done

Requirement	Where
METRICS_USERNAME set + password empty/unset is a startup error naming both variables, exit non-zero	resolveMetricsAuth
Converse (METRICS_PASSWORD set, username empty) is the same error	same, direction-independent
Both unset stays valid, /metrics unmounted	MetricsAuthEnabled() false
hasMetricsAuth cannot disagree with the mount	both read MetricsAuthEnabled()
Tests cover all four combinations	TestMetricsAuthConfig

TestMetricsAuthConfig treats "set to the empty string" and "not set at all" as separate inputs on each side (7 rows), since the reported bug arrived through the first. TestMetricsRouteUnmountedWithoutCredentials, TestMetricsRouteRequiresCredentials and TestMetricsRouteUnmountedOnHalfSetConfig pin the same states against the real route tree; the middle one asserts an empty password gets 401.

newTestEnv in internal/server/routes_test.go gained a newTestEnvWithConfig variant so a test can drive the production route tree with a chosen Config; the same pointer reaches the router and every middleware.

TODO.md deliberately untouched (#112).

Gate evidence

make check — exit 0

Lint ran in Docker (Dockerfile.lint, golangci-lint v2.12.2 pinned by digest):

#12 [lint 3/3] RUN --network=none golangci-lint run --config .golangci.yml ./...
#12 88.21 0 issues.
#12 DONE 92.8s
EXIT=0

Cache-defeated container build — exit 0

docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain ., run on the rebased tree. The CACHED layers are only the two base-image FROM resolutions and the dependency-download steps (go.mod/go.sum unchanged); the lint and test layers executed:

#22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./...
#22 75.18 0 issues.
#35 [builder  9/11] RUN make test

All 15 packages ran with real durations, no (cached) lines:

#35 93.16 ok  sneak.berlin/go/webhooker/internal/config   1.472s
#35 139.1 ok  sneak.berlin/go/webhooker/internal/server   6.802s
#35 139.1 ok  sneak.berlin/go/webhooker/internal/middleware  5.300s
#35 139.1 ok  sneak.berlin/go/webhooker/internal/handlers   52.040s

New tests, in-container:

#35 93.16 --- PASS: TestMetricsAuthConfig (0.16s)
#35 93.16     --- PASS: TestMetricsAuthConfig/both_unset_leaves_metrics_unmounted
#35 93.16     --- PASS: TestMetricsAuthConfig/both_empty_leaves_metrics_unmounted
#35 93.16     --- PASS: TestMetricsAuthConfig/both_set_enables_metrics_auth
#35 93.16     --- PASS: TestMetricsAuthConfig/username_with_unset_password_fails
#35 93.16     --- PASS: TestMetricsAuthConfig/username_with_empty_password_fails
#35 93.16     --- PASS: TestMetricsAuthConfig/password_with_unset_username_fails
#35 93.16     --- PASS: TestMetricsAuthConfig/password_with_empty_username_fails
#35 139.1 --- PASS: TestMetricsRouteRequiresCredentials (2.13s)
#35 139.1 --- PASS: TestMetricsRouteUnmountedWithoutCredentials (2.35s)
#35 139.1 --- PASS: TestMetricsRouteUnmountedOnHalfSetConfig (0.08s)

Build images and containers were removed afterwards; docker ps -a and docker images show nothing of this run. No prune was used.

Live verification against the issue's own reproduction

Half-set, both directions — the process refuses to start, exit 1:

$ METRICS_USERNAME=metrics ./bin/webhooker    # METRICS_PASSWORD unset
incomplete metrics credentials: METRICS_USERNAME is set but METRICS_PASSWORD is empty;
METRICS_USERNAME and METRICS_PASSWORD must both be set to serve /metrics, or both be
empty to leave it unmounted
exit=1

$ METRICS_PASSWORD=s3cret ./bin/webhooker     # METRICS_USERNAME unset
incomplete metrics credentials: METRICS_PASSWORD is set but METRICS_USERNAME is empty;
...
exit=1

Both set — hasMetricsAuth:true, and the issue's curl -u 'metrics:' now gets 401 where it previously got 200:

"hasMetricsAuth":true
curl -u 'metrics:'        /metrics  -->  401
curl -u 'metrics:s3cret'  /metrics  -->  200
curl (no auth)            /metrics  -->  401

Both unset — hasMetricsAuth:false and the route does not exist:

"hasMetricsAuth":false
curl -u 'metrics:' /metrics  -->  404

Docs

README: both env-var rows note the pairing requirement, a new "Metrics credentials" subsection states the three outcomes, and the endpoint table plus the middleware list no longer describe the mount as keyed on METRICS_USERNAME alone. make fmt run and included.

Closes https://git.eeqj.de/sneak/webhooker/issues/205. ## The defect `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 (`internal/server/routes.go`), the log tested both variables (`internal/config/config.go`), so the two could disagree about whether the endpoint existed. ## Approach **One value behind both.** `Config.MetricsAuthEnabled()` is now the only answer to "is `/metrics` served": the route mount, the Prometheus recording middleware and the startup log's `hasMetricsAuth` field all read it. It requires *both* credentials, not the username alone, so a `Config` assembled in code (which bypasses environment validation) still cannot publish an endpoint that accepts an empty password. **Fail loudly on half-set.** `loadFromEnv` rejects a half-set pair via a new `ErrIncompleteMetricsAuth`, naming both variables in either direction. A set-but-invalid configuration aborts startup rather than degrading into something the operator did not ask for. Neither fallback was acceptable: mounting on the username alone publishes an endpoint with an empty password, and quietly leaving it unmounted withholds an endpoint that was asked for. The error carries neither credential value — the password is a secret. Both unset remains valid and leaves `/metrics` unmounted. ## Definition of done | Requirement | Where | | --- | --- | | `METRICS_USERNAME` set + password empty/unset is a startup error naming both variables, exit non-zero | `resolveMetricsAuth` | | Converse (`METRICS_PASSWORD` set, username empty) is the same error | same, direction-independent | | Both unset stays valid, `/metrics` unmounted | `MetricsAuthEnabled()` false | | `hasMetricsAuth` cannot disagree with the mount | both read `MetricsAuthEnabled()` | | Tests cover all four combinations | `TestMetricsAuthConfig` | `TestMetricsAuthConfig` treats "set to the empty string" and "not set at all" as separate inputs on each side (7 rows), since the reported bug arrived through the first. `TestMetricsRouteUnmountedWithoutCredentials`, `TestMetricsRouteRequiresCredentials` and `TestMetricsRouteUnmountedOnHalfSetConfig` pin the same states against the real route tree; the middle one asserts an empty password gets 401. `newTestEnv` in `internal/server/routes_test.go` gained a `newTestEnvWithConfig` variant so a test can drive the production route tree with a chosen `Config`; the same pointer reaches the router and every middleware. `TODO.md` deliberately untouched (https://git.eeqj.de/sneak/webhooker/issues/112). ## Gate evidence ### `make check` — exit 0 Lint ran in Docker (`Dockerfile.lint`, golangci-lint v2.12.2 pinned by digest): ``` #12 [lint 3/3] RUN --network=none golangci-lint run --config .golangci.yml ./... #12 88.21 0 issues. #12 DONE 92.8s EXIT=0 ``` ### Cache-defeated container build — exit 0 `docker build --no-cache-filter=lint --no-cache-filter=builder --progress=plain .`, run on the rebased tree. The `CACHED` layers are only the two base-image `FROM` resolutions and the dependency-download steps (`go.mod`/`go.sum` unchanged); the lint and test layers executed: ``` #22 [lint 9/9] RUN --network=none golangci-lint run --config .golangci.yml ./... #22 75.18 0 issues. #35 [builder 9/11] RUN make test ``` All 15 packages ran with real durations, no `(cached)` lines: ``` #35 93.16 ok sneak.berlin/go/webhooker/internal/config 1.472s #35 139.1 ok sneak.berlin/go/webhooker/internal/server 6.802s #35 139.1 ok sneak.berlin/go/webhooker/internal/middleware 5.300s #35 139.1 ok sneak.berlin/go/webhooker/internal/handlers 52.040s ``` New tests, in-container: ``` #35 93.16 --- PASS: TestMetricsAuthConfig (0.16s) #35 93.16 --- PASS: TestMetricsAuthConfig/both_unset_leaves_metrics_unmounted #35 93.16 --- PASS: TestMetricsAuthConfig/both_empty_leaves_metrics_unmounted #35 93.16 --- PASS: TestMetricsAuthConfig/both_set_enables_metrics_auth #35 93.16 --- PASS: TestMetricsAuthConfig/username_with_unset_password_fails #35 93.16 --- PASS: TestMetricsAuthConfig/username_with_empty_password_fails #35 93.16 --- PASS: TestMetricsAuthConfig/password_with_unset_username_fails #35 93.16 --- PASS: TestMetricsAuthConfig/password_with_empty_username_fails #35 139.1 --- PASS: TestMetricsRouteRequiresCredentials (2.13s) #35 139.1 --- PASS: TestMetricsRouteUnmountedWithoutCredentials (2.35s) #35 139.1 --- PASS: TestMetricsRouteUnmountedOnHalfSetConfig (0.08s) ``` Build images and containers were removed afterwards; `docker ps -a` and `docker images` show nothing of this run. No prune was used. ### Live verification against the issue's own reproduction Half-set, both directions — the process refuses to start, exit 1: ``` $ METRICS_USERNAME=metrics ./bin/webhooker # METRICS_PASSWORD unset incomplete metrics credentials: METRICS_USERNAME is set but METRICS_PASSWORD is empty; METRICS_USERNAME and METRICS_PASSWORD must both be set to serve /metrics, or both be empty to leave it unmounted exit=1 $ METRICS_PASSWORD=s3cret ./bin/webhooker # METRICS_USERNAME unset incomplete metrics credentials: METRICS_PASSWORD is set but METRICS_USERNAME is empty; ... exit=1 ``` Both set — `hasMetricsAuth:true`, and the issue's `curl -u 'metrics:'` now gets 401 where it previously got 200: ``` "hasMetricsAuth":true curl -u 'metrics:' /metrics --> 401 curl -u 'metrics:s3cret' /metrics --> 200 curl (no auth) /metrics --> 401 ``` Both unset — `hasMetricsAuth:false` and the route does not exist: ``` "hasMetricsAuth":false curl -u 'metrics:' /metrics --> 404 ``` ## Docs README: both env-var rows note the pairing requirement, a new "Metrics credentials" subsection states the three outcomes, and the endpoint table plus the middleware list no longer describe the mount as keyed on `METRICS_USERNAME` alone. `make fmt` run and included.

clawbot added 1 commit 2026-08-20 06:19:18 +02:00

Fail startup on half-set metrics credentials (closes #205) ... c172deee72

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.

clawbot added the needs-review label 2026-08-20 06:19:26 +02:00

clawbot self-assigned this 2026-08-20 06:19:28 +02:00

clawbot commented 2026-08-20 06:29:20 +02:00

Author

Collaborator

Copy Link

PASS — satisfies every DoD item in #205; independently re-ran docker build --no-cache-filter=lint --no-cache-filter=builder on c172dee: lint 78.6s / 0 issues, make test 126.4s, all 15 packages with real durations and zero (cached) lines, exit 0.

Anomalies that pass anyway, for the record:

• metrics.NewRecorder(metrics.Config{}) in internal/middleware/middleware.go:444 registers on prometheus.DefaultRegisterer via MustRegister, which panics on a duplicate. TestMetricsRouteRequiresCredentials is the first test in internal/server to construct a metrics-enabled router, so exactly one registration happens today and it passes. A second metrics-enabled router in that package (or go test -count=2) would panic. Relevant to #209: no conflict with registering new collectors — /metrics serves promhttp.Handler() over the default gatherer, so collectors registered elsewhere are picked up — but #209 must not add a second metrics-enabled router test without giving the recorder its own registry.
• Middleware.MetricsAuth() is unchanged and still builds map[string][]string{username: {password}} unconditionally, so it would accept an empty password if ever called outside the MetricsAuthEnabled() guard. Unreachable today (single call site, inside the guard) and the password compare is constant-time (subtle.ConstantTimeCompare, with the length-mismatch dummy), so -u 'metrics:' gets 401. Noted as hardening, not a defect.

Disclosure — judgement call waived: Config.MetricsAuthEnabled() returns false for a half-set Config built in code rather than panicking, i.e. auth-off without noise. That is not a fail-loudly violation because every production Config comes from config.New/loadFromEnv, which now aborts, and it is documented at the method. Verified there is no config-reload path and no second production Config provider, so hasMetricsAuth and the mount read the same fields on the same never-mutated pointer.

Also checked: base next, fast-forward on 10c8dd2; CI green (4m49s, real); title and commit both end (closes #205); TODO.md untouched; no attribution trailers or vendor references; make fmt-check clean in-container; the two half-set config rows assert ErrIncompleteMetricsAuth and the absence of the password from the message, and TestMetricsRouteUnmountedOnHalfSetConfig is the assertion that actually regresses against pre-fix behaviour.

PASS — satisfies every DoD item in https://git.eeqj.de/sneak/webhooker/issues/205; independently re-ran `docker build --no-cache-filter=lint --no-cache-filter=builder` on `c172dee`: lint 78.6s / 0 issues, `make test` 126.4s, all 15 packages with real durations and zero `(cached)` lines, exit 0. Anomalies that pass anyway, for the record: - `metrics.NewRecorder(metrics.Config{})` in `internal/middleware/middleware.go:444` registers on `prometheus.DefaultRegisterer` via `MustRegister`, which panics on a duplicate. `TestMetricsRouteRequiresCredentials` is the first test in `internal/server` to construct a metrics-enabled router, so exactly one registration happens today and it passes. A second metrics-enabled router in that package (or `go test -count=2`) would panic. Relevant to https://git.eeqj.de/sneak/webhooker/issues/209: no conflict with registering new collectors — `/metrics` serves `promhttp.Handler()` over the default gatherer, so collectors registered elsewhere are picked up — but #209 must not add a second metrics-enabled router test without giving the recorder its own registry. - `Middleware.MetricsAuth()` is unchanged and still builds `map[string][]string{username: {password}}` unconditionally, so it would accept an empty password if ever called outside the `MetricsAuthEnabled()` guard. Unreachable today (single call site, inside the guard) and the password compare is constant-time (`subtle.ConstantTimeCompare`, with the length-mismatch dummy), so `-u 'metrics:'` gets 401. Noted as hardening, not a defect. Disclosure — judgement call waived: `Config.MetricsAuthEnabled()` returns false for a half-set `Config` built in code rather than panicking, i.e. auth-off without noise. That is not a fail-loudly violation because every production `Config` comes from `config.New`/`loadFromEnv`, which now aborts, and it is documented at the method. Verified there is no config-reload path and no second production `Config` provider, so `hasMetricsAuth` and the mount read the same fields on the same never-mutated pointer. Also checked: base `next`, fast-forward on `10c8dd2`; CI green (4m49s, real); title and commit both end `(closes #205)`; `TODO.md` untouched; no attribution trailers or vendor references; `make fmt-check` clean in-container; the two half-set config rows assert `ErrIncompleteMetricsAuth` and the absence of the password from the message, and `TestMetricsRouteUnmountedOnHalfSetConfig` is the assertion that actually regresses against pre-fix behaviour.

clawbot merged commit bb30b3ad64 into next 2026-08-20 06:30:25 +02:00

clawbot deleted branch issue-205-metrics-auth-fail-loud 2026-08-20 06:30:25 +02:00

clawbot referenced this issue from a commit 2026-08-20 06:30:26 +02:00

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

clawbot referenced this pull request 2026-08-20 06:43:12 +02:00

Expose delivery metrics on /metrics (closes #209) #224

clawbot referenced this pull request 2026-08-20 06:44:18 +02:00

go test -count=2 ./internal/server/... panics on duplicate metrics collector registration #227

clawbot referenced this pull request 2026-08-20 06:54:14 +02:00

Lock DATA_DIR against a second instance (closes #201) #220

clawbot referenced this pull request 2026-08-20 07:07:11 +02:00

Lock DATA_DIR against a second instance (closes #201) #220

clawbot referenced this pull request 2026-08-20 07:07:53 +02:00

Add an egress CIDR allowlist to the SSRF guard (closes #204) #217

clawbot referenced this pull request 2026-08-20 07:18:06 +02:00

Add an egress CIDR allowlist to the SSRF guard (closes #204) #217

clawbot referenced this pull request 2026-08-20 07:22:24 +02:00

Lock DATA_DIR against a second instance (closes #201) #220

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No Label needs-review

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

clawbot sneak

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#216