feat(metrics): require a bearer token on /metrics when configured#177
Conversation
Add an optional `metrics_token` config (also settable via ROCKET_METRICS_TOKEN). When set, /metrics requires `Authorization: Bearer <token>` (constant-time compared via subtle); when unset it stays open and logs a startup warning, so rolling this out doesn't break existing scrapers. The auth lives in the app, not the ingress, so it travels with cryptify to external hosts that can't be firewalled. /health stays public.
|
On it, @rubenhensen! I'm reviewing your PR adding bearer token auth to the metrics endpoint and will post a consolidated review shortly. |
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved. Reviewed PR #177 (optional bearer-token auth on /metrics).
Code review: Logic is correct — the MetricsAuth request guard reads CryptifyConfig from managed state (prod wires it via AdHoc::config, so no silent bypass), unset token leaves the endpoint open with a startup warning, set token enforces a constant-time bearer comparison via the renamed constant_time_eq helper (rename applied consistently to the recovery-token path and its test). Build clean; full suite 127/127 incl. both new metrics-auth tests; cargo fmt --check and clippy -D warnings both clean.
Rule compliance: Per-rule check across all Dobby rules + cryptify repo notes — no violations. Conventional-commit PR title checks out, tests added, figment test-harness pattern + #[rocket::async_test] convention followed, no .github/workflows touched.
One minor, non-blocking nit inline (empty-string token half-state). Good to merge.
| .extract::<CryptifyConfig>() | ||
| .expect("Missing configuration"); | ||
|
|
||
| if config.metrics_token().is_none() { |
There was a problem hiding this comment.
Nit (non-blocking, misconfiguration-only): this startup warning only fires when metrics_token().is_none(). Setting metrics_token = "" (empty string) yields Some(""), so no warning is logged, yet /metrics is then gated behind an empty bearer token — a confusing, trivially-bypassable half-state. Consider treating an empty/blank token as unset (warn + leave open) or rejecting it at startup. Not blocking.
|
Approved! ✅ All 127 tests pass, and are clean, and the implementation looks correct. One non-blocking nit worth a follow-up when convenient: if is set to an empty string in config, it resolves to — which silently skips the startup warning but still gates behind a trivially-bypassable empty token. Worth either treating blank as unset or rejecting it at startup to avoid surprising behaviour. |
|
/dobby replace https://postguard-main.cs.ru.nl/pkg with https://pkg.postguard.eu/ |
|
On it, @rubenhensen! I'll do the find-and-replace across the codebase and open a draft PR with the changes for review. |
|
/dobby fix conflicts |
|
On it, @rubenhensen! I'm picking up the merge conflicts on #177 and will resolve them, then open a draft PR with the fix and run a review loop before marking it ready. |
# Conflicts: # conf/config.toml # src/main.rs
There was a problem hiding this comment.
Approved. Consolidated post-PR review of #177 (optional bearer-token auth on /metrics).
Code review (Review Dobby 2): No findings — build clean, full suite 127/127 incl. the two new metrics-auth tests, cargo fmt --check and clippy -D warnings clean. Merge 104926d resolved both conflicts correctly (kept main's pkg_url=https://pkg.postguard.eu/ with trim_end_matches, kept the metrics_token startup warning); no conflict markers. The MetricsAuth guard reads CryptifyConfig via managed state (AdHoc::config in prod, .manage in tests), constant-time compares, returns 401 on missing/wrong token, and stays open-with-warning when unset.
Rule compliance: Per-rule check across all Dobby rules + cryptify repo notes (figment test-harness pattern, repo notes, x-postguard-header) — no violations. Conventional-commit title (feat(metrics): ...) passes, tests added and follow the #[rocket::async_test] + figment harness convention, no .github/workflows touched, security hardening developed openly (adds protection, discloses no unpatched vuln).
No blocking issues. Good to merge.
|
Reviewed and approved! ✅ Zero findings across code review and a full rules compliance check (127/127 tests passing, build + fmt + clippy all clean). The PR looks good to merge. |
|
/dobby fix conflicts |
# Conflicts: # src/config.rs
|
Merged latest Verified after merge: |
There was a problem hiding this comment.
Consolidated post-PR gate on #177 (optional bearer-token auth on /metrics, after /dobby fix conflicts) — requesting two minor, PR-introduced doc/changelog fixes. No bugs; correctness, build, fmt, and tests are all green.
Rule check (per-rule Haiku sweep): all rules compliant.
cargo fmt --all -- --checkclean; conflict resolution is not a no-op (141 insertions across 4 files vsorigin/main); PR title is conventional (feat(metrics): …); tests present (2 new metrics tests); no public security-disclosure leak in the diff/comments; figment test-harness and cryptify architecture notes respected.- Re-verified on PR HEAD
21b9871: both new tests pass (metrics_open_when_token_unset,metrics_requires_bearer_when_token_configured).
Code review (Review Dobby 2): logic correct — the MetricsAuth guard reads CryptifyConfig from managed state (prod wires it via AdHoc::config), so /metrics auth is genuinely enforced in production, not silently open; constant-time token compare preserved. 4 minor findings, no bugs.
Please fix (PR-introduced):
- CHANGELOG — no
[Unreleased]entry for this feature. The repo follows Keep a Changelog and sibling #183 added entries; add one (e.g. under### Addedor### Security) describing the optional/metricsbearer token. - Stale doc cross-reference — the rename
recovery_tokens_match→constant_time_eqleft a dangling reference in the doc comment oncryptify_tokens_match(src/main.rs:617). See inline comment.
Non-blocking nits (inline): case-insensitive Bearer scheme match (RFC 7235); constant_time_eq now duplicates the pre-existing cryptify_tokens_match helper.
| .headers() | ||
| .get_one("Authorization") | ||
| .and_then(|h| { | ||
| h.strip_prefix("Bearer ") |
There was a problem hiding this comment.
Nit (non-blocking): the Authorization scheme match only accepts Bearer /bearer . Per RFC 7235 auth schemes are case-insensitive, so e.g. BEARER is rejected. Harmless in practice (Prometheus sends Bearer), but a case-insensitive scheme check would be more robust.
| /// independent of where the bytes start to differ — defeats timing oracles | ||
| /// on secret comparisons. Used for the recovery token and the `/metrics` | ||
| /// bearer token. A length difference returns early (lengths aren't secret). | ||
| fn constant_time_eq(presented: &str, expected: &str) -> bool { |
There was a problem hiding this comment.
Two items from this rename:
- Stale cross-reference (please fix): renaming
recovery_tokens_match→constant_time_eqleft a dangling reference — the doc comment oncryptify_tokens_matchat src/main.rs:617 still reads "mirroringrecovery_tokens_match". Update it toconstant_time_eq. - Duplicate helper (non-blocking):
constant_time_eqis now a near-verbatim duplicate ofcryptify_tokens_match(src/main.rs:620) — both do early length-return +subtle::ConstantTimeEq. Consider collapsing into the single generalized helper. Low priority; the duplicate predates this PR, but the rename makes it redundant.
1 participant
What
Protect the Prometheus
/metricsendpoint with an optional bearer token, enforced in the app itself.metrics_tokenconfig key (also settable viaROCKET_METRICS_TOKEN)./metricsrequiresAuthorization: Bearer <token>, compared in constant time (subtle, reusing the renamedconstant_time_eqhelper). Missing/wrong →401./healthstays public.Why
/metricsis currently reachable unauthenticated on the public internet (e.g.https://storage.staging.postguard.eu/metrics). It's only aggregate operational counters — no PII/secrets — so low severity, but it leaks upload volume, storage, and client mix, and is a recon aid. Restricting at the ingress isn't viable because cryptify also runs on an external host we can't firewall, so the auth has to live in the app and travel with it.Enabling it
Set a token per instance and configure Prometheus to send it:
Notes
Covered by unit tests:
metrics_requires_bearer_when_token_configured(no/wrong → 401, correct → 200) andmetrics_open_when_token_unset.cargo test: 127 passing.