fix(auth): require out-of-band bootstrap secret for first root key#3221
fix(auth): require out-of-band bootstrap secret for first root key#3221frdomovic wants to merge 4 commits into
Conversation
On a fresh node with no root keys, the first unauthenticated caller to POST /auth/token was silently granted a ROOT admin key (trust-on-first- use). Whoever reached the node first — potentially an attacker on the network rather than the operator — became admin. Gate the bootstrap path behind an out-of-band secret: the caller must present a secret matching MERO_AUTH_BOOTSTRAP_SECRET (or the configured user_password.bootstrap_secret). The comparison is constant-time over SHA-256 digests. When no secret is configured, first-login bootstrap is disabled (fail closed); a root key must then be provisioned out of band. All failure paths return the same generic error so a probe cannot tell "disabled", "wrong secret", and "already bootstrapped" apart. Existing users are unaffected — they authenticate on the fast path before the bootstrap branch and never need the secret. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🤖 MeroReviewer
Reviewed by 2 agents | Quality score: 47% | Review time: 367.0s
🔴 1 critical. See inline comments.
🤖 Generated by MeroReviewer | Review ID: review-8e082ce7
| /// (the recommended out-of-band channel); the configured value is used as | ||
| /// a fallback. Returns `None` when neither is set, which disables | ||
| /// first-login bootstrap entirely. | ||
| fn effective_bootstrap_secret(&self) -> Option<String> { |
There was a problem hiding this comment.
🔴 Empty-string bootstrap_secret config value silently re-enables unauthenticated TOFU bootstrap
effective_bootstrap_secret() filters the MERO_AUTH_BOOTSTRAP_SECRET env var for emptiness (.filter(|s| !s.is_empty())) but does NOT apply the same filter to self.config.bootstrap_secret. If the config value deserializes to Some("") (e.g. a TOML/env-interpolation footgun such as bootstrap_secret = "", or AUTH__USER_PASSWORD__BOOTSTRAP_SECRET= left blank in a container env file), effective_bootstrap_secret() returns Some("") instead of None. Meanwhile, at line 238, bootstrap_secret.unwrap_or_default() yields "" whenever the caller omits the field entirely. bootstrap_secret_matches("", "") then returns true (SHA-256("") == SHA-256("")), so the very first unauthenticated caller is minted a ROOT admin key with zero secret presented — exactly the trust-on-first-use vulnerability this PR sets out to close. This directly contradicts the documented fail-closed guarantee ("when this is None... bootstrap is disabled") because an empty string is a distinct, unguarded value from None.
Suggested fix:
Apply the same non-empty filter to the config fallback, e.g. `.or_else(|| self.config.bootstrap_secret.clone().filter(|s| !s.is_empty()))`, and/or reject empty strings at config-load time (e.g. via a custom `Deserialize`/`validator` check on `UserPasswordConfig::bootstrap_secret`) so an accidentally-blank value can never be treated as "configured".
There was a problem hiding this comment.
Addressed in c7f9c26 — the config fallback now gets the same .filter(|s| !s.is_empty()) as the env var, the bootstrap_secret config doc states empty ≡ unset, and empty_config_secret_is_treated_as_unset pins the exact failure mode (omitted field defaulting to "" matching an empty configured secret).
There was a problem hiding this comment.
Fixed — belt and braces: (1) effective_bootstrap_secret() now applies the same .filter(|s| !s.is_empty()) to the config source as to the env var, so bootstrap_secret = "" behaves as unset (bootstrap disabled, fail closed) — c7f9c26; (2) independently, authenticate_core rejects an empty presented secret with the generic error before any comparison, so an all-empty pairing can never authenticate even if the config-side filter regresses — 90110a0. Tests: empty_config_bootstrap_secret_keeps_bootstrap_disabled, empty_presented_secret_never_bootstraps (both assert zero root keys minted on a fresh KeyManager).
Documentation ReviewThe following documentation may need updates based on the changes in this PR:
|
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 2 agents | Quality score: 90% | Review time: 1000.7s
⚠️ Review Incomplete
1 of 2 agent(s) did not finish (security-reviewer) and no findings were produced. Treat this PR as not yet reviewed, not as approved.
🤖 Generated by AI Code Reviewer | Review ID: review-fa89eff5
Review follow-up: the env var was filtered for emptiness but the config fallback was not, so bootstrap_secret = "" (blank env interpolation) would match a caller that omitted the field (unwrap_or_default) and silently re-enable unauthenticated TOFU bootstrap. Filter both sources and pin with a unit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
E2E Rust Apps FailedOne or more E2E workflows (scaffolding-e2e, xcall-example) failed. Please check the workflow logs for more details. |
Defense in depth for the empty-string bypass class: the empty-config
filter (previous commit) closes one end, but the caller side defaults an
omitted bootstrap_secret to "" (unwrap_or_default), so any future
regression that lets an empty *expected* secret through would re-enable
SHA-256("") == SHA-256("") TOFU bootstrap. Guard the presented secret
before any comparison so an all-empty pairing can never authenticate,
even if one of the two filters regresses.
Tests: empty_config_bootstrap_secret_keeps_bootstrap_disabled (renamed
from empty_config_secret_is_treated_as_unset) and the new
empty_presented_secret_never_bootstraps pin both ends against a fresh
KeyManager and assert zero root keys are minted.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The bootstrap gate (fail-closed since the TOFU removal) broke every e2e flow that relied on first-login root bootstrap. Re-provision each harness without weakening the server-side gate: * auth-seam: export MERO_AUTH_BOOTSTRAP_SECRET on the CI step. Binary- mode merobox spawns merod with os.environ inherited and local script steps inherit it too, so one export provisions the node's expected secret AND lets scripts/e2e-auth-seam.sh present the same value in provider_data.bootstrap_secret on the bootstrap login (omitted when unset, so an unprovisioned run still fails closed). * sdk-e2e: export the secret into merod's environment at boot and mint the MERO_E2E_USER root key with one bootstrap login BEFORE the mero-js suite runs — its plain username/password logins then authenticate on the existing-user fast path and never need the secret. Credentials are pinned via MERO_E2E_USER/MERO_E2E_PASS (mero-js resolveCreds reads them) so both sides stay in lockstep. * scenario matrix (e2e-rust-apps + release variant): pre-stage the same env var. Inert today — merobox (<=0.6.41) neither forwards env into node containers nor presents a bootstrap secret in its login step — but once merobox ships the pass-through via the APT repo these jobs heal without further changes here (then bump MIN_MEROBOX). Verified locally with merobox 0.6.41 (binary mode): auth-seam passes 7/7 with the secret exported, and fails closed (root login rejected) without it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Security finding
[H] Trust-on-first-use root key (security review item 2) —
crates/auth/src/providers/impls/user_password.rs.On a fresh node with no root keys,
authenticate_coretreats the first login as a "bootstrap" and mints a ROOT admin key for whatever username/password the caller supplies — over the unauthenticatedPOST /auth/tokenroute. On a freshly provisioned node, whoever reaches it first (potentially an attacker on the network, not the operator) becomes admin.Before
Any first caller → root admin. No secret, no gate.
After
The bootstrap path now requires the caller to prove possession of an out-of-band secret:
MERO_AUTH_BOOTSTRAP_SECRET(preferred, out-of-band) or the newuser_password.bootstrap_secretconfig field.bootstrap_secretin the provider data (or thex-bootstrap-secretheader on the header-based path).subtle::ConstantTimeEq), so it leaks neither timing nor length."Invalid username or password"so a probe can't distinguish disabled / wrong secret / already bootstrapped.Existing users are unaffected: they match on the fast
verify_credentialspath before the bootstrap branch and never need the secret.First-login bootstrap is now off unless a secret is configured. Deployments that relied on implicit TOFU bootstrap must set
MERO_AUTH_BOOTSTRAP_SECRET(oruser_password.bootstrap_secret) before the first login, or provision the root key out of band. This is the intended hardening; flagging it for release notes.How to test
Automated (added in this PR,
cargo test -p mero-auth user_password):bootstrap_disabled_by_default_rejects_first_login— no secret configured → first login errors, zero root keys created.bootstrap_requires_matching_secret— missing/wrong secret → rejected, no key; correct secret → exactly one admin root key.existing_user_authenticates_without_bootstrap_secret— after bootstrap, the existing user logs in with no secret; a different identity cannot bootstrap a second root key even with the correct secret.bootstrap_secret_matches_is_exact— the constant-time comparator is exact (length/case sensitive).Manual:
MERO_AUTH_BOOTSTRAP_SECRETunset →POST /auth/tokenmust not create a key.provider_data.bootstrap_secretmatching → root key minted.Notes
user_password.rs, so it will conflict with fix(auth): enforce password bounds, key expiry, and exact node-host binding #3081 (salted-KDF key id / password bounds) at merge — both are independent security fixes off master; whichever lands first, the other rebases.Update 2026-07-14 — review fix + e2e strategy
Empty-string bypass (meroreviewer 🔴) — fixed twice over (
c7f9c269+90110a0d): the config source now filters empty strings exactly like the env source (bootstrap_secret = ""behaves as unset → fail closed), AND an empty presented secret is rejected before any comparison — soSHA256("") == SHA256("")can never re-enable TOFU even if one guard regresses. New tests:empty_config_bootstrap_secret_keeps_bootstrap_disabled,empty_presented_secret_never_bootstraps.e2e strategy with the bootstrap gate on (
ffcba187): CI provisions the out-of-band secret instead of weakening the gate. The auth-seam job exportsMERO_AUTH_BOOTSTRAP_SECRETonce — binary-mode merobox starts merod with the inherited environment and local script steps inherit it too, so the same export configures the node's expected secret and letsscripts/e2e-auth-seam.shpresent it viaprovider_data.bootstrap_secreton the bootstrap login. The SDK-E2E job exports the secret into merod's environment and pre-mints thedevroot key with a single authenticated bootstrap call before the mero-js suite runs, so the SDK's plain username/password logins authenticate on the existing-user fast path (its wrong-credentials negative test stays valid). Verified locally with merobox 0.6.41: auth-seam passes 7/7 with the secret exported and fails closed without it.Known-remaining red: the docker-mode scenario matrix (
Test (group-*)) drives logins through merobox's built-inloginstep, which today can neither inject env into node containers nor present a bootstrap secret. The matrix jobs already export the CI test secret (inert until merobox supports it); one small merobox PR — forwardMERO_AUTH_BOOTSTRAP_SECRETinto node containers + present it in theloginstep, both defaulting from merobox's own environment — heals all 58 scenario workflows with no further core changes, after whichMIN_MEROBOXgets bumped in.github/actions/setup-merobox.