fix(operator): harden S3/S2/S4 per adversarial review

9 confirmed findings (review wbdf3c0sd), all fixed:

S4 (capabilities):
- REGRESSION: /capabilities reported grin_relay:false for a working relay (S4
  wrongly coupled it to Grin chain serviceability). The relay is a non-custodial
  mailbox — wallet broadcasts locally — so it is decoupled again. Removed the now-
  dead grin_relay downgrade note.

S3 (bootstrap latch):
- A DELETE of the latch row escaped Locked without Tampered detection on a
  CLI-bootstrapped-but-no-users instance (adoption keyed off `users` only). Boot
  now adopts on has_any_users() OR has_any_admin_keys(), so a deleted latch
  re-latches Locked instead of silently dropping to uninitialized.
- bootstrap_admin's single-bootstrap guard was a check-then-insert race (two
  concurrent `setup` could seed two admins). It now takes the admin-keys mutation
  advisory lock (shared with add/revoke/rotate) at the top of its tx.

S2 (admin posture + landing):
- Hard bind guard matched loopback by string prefix ("localhost".starts_with) —
  "localhost.evil.com" slipped past. Now resolves via to_socket_addrs and requires
  EVERY resolved addr to be loopback.
- host_allowed's IPv6 loopback check was an unanchored prefix ("[::1]" matched
  "[::1].evil"); now anchored to exactly "[::1]" / "[::1]:port". +regression tests.
- Anti-clickjacking headers (X-Frame-Options/CSP) are now applied on the 403
  Host-reject path too (computed on both branches, headers added once).
- GET /api/v1/server-info now returns a BARE 404 when landing is off (the
  {error,code} envelope itself hinted the route exists).
- Public landing `chains` now uses the SAME serviceability predicate as
  /capabilities (no over-advertising an unserviceable chain).
- Removed the dead/misleading PUBLIC_EXPOSE_FEATURES toggle (never read; the
  feature tuple is intentionally never emitted anonymously).

Dismissed after verification: latch chains-by-flag (dup), title length bound
(operator-owned + escaped), prices:true with empty PRICES_ASSETS (documented).
140 lib + 140 integration green, no openapi drift, scans clean.
This commit is contained in:
jwinterm
2026-06-27 19:44:08 -04:00
parent 2d0077b2b9
commit e618574ee7
7 changed files with 66 additions and 48 deletions
+13 -12
View File
@@ -89,8 +89,8 @@ fn host_allowed(host: &str, onion: Option<&str>) -> bool {
return true;
}
let h = host.to_ascii_lowercase();
// IPv6 loopback (with or without :port), e.g. "[::1]" / "[::1]:8081".
if h.starts_with("[::1]") || h == "::1" {
// IPv6 loopback, anchored: exactly "[::1]" or "[::1]:port" (not "[::1].evil").
if h == "[::1]" || h.starts_with("[::1]:") || h == "::1" {
return true;
}
let bare = h.split(':').next().unwrap_or(&h);
@@ -119,10 +119,13 @@ pub async fn admin_plane_guard(
.get(axum::http::header::HOST)
.and_then(|h| h.to_str().ok())
.unwrap_or("");
if !host_allowed(host, state.config.admin.onion.as_deref()) {
return (StatusCode::FORBIDDEN, "forbidden").into_response();
}
let mut resp = next.run(req).await;
// Compute the response on both paths, then apply the security headers once so
// the 403 Host-reject carries them too.
let mut resp = if host_allowed(host, state.config.admin.onion.as_deref()) {
next.run(req).await
} else {
(StatusCode::FORBIDDEN, "forbidden").into_response()
};
let h = resp.headers_mut();
h.insert("x-frame-options", HeaderValue::from_static("DENY"));
h.insert(
@@ -719,12 +722,6 @@ pub async fn admin_features(
"grin",
"GRIN_OWNER_API_SECRET unset",
);
note(
cfg.features.grin_relay,
effective.features.grin_relay,
"grin_relay",
"Grin chain not serviceable",
);
note(
cfg.features.nostr_identity,
effective.features.nostr_identity,
@@ -771,6 +768,10 @@ mod tests {
// A foreign host (DNS-rebinding) is rejected.
assert!(!host_allowed("evil.example", None));
assert!(!host_allowed("attacker.test:8081", None));
// Anchored: a host merely STARTING with a loopback form is rejected.
assert!(!host_allowed("[::1].evil.com", None));
assert!(!host_allowed("localhost.evil.com", None));
assert!(!host_allowed("127.0.0.1.evil.com", None));
// The configured onion is allowed; others still rejected.
assert!(host_allowed("abc.onion", Some("abc.onion")));
assert!(host_allowed("abc.onion:80", Some("abc.onion")));
+4 -3
View File
@@ -63,7 +63,7 @@ pub struct CapabilitiesResponse {
/// present). A chain whose flag is on but whose source is unconfigured reports
/// `enabled:false` — so `/capabilities` never advertises a chain that 404s, and
/// a missing-secret downgrade is indistinguishable from a deliberately-off chain.
fn chain_serviceable(config: &Config, asset: &str) -> bool {
pub(crate) fn chain_serviceable(config: &Config, asset: &str) -> bool {
let f = &config.features.chains;
let c = &config.chains;
match asset {
@@ -106,8 +106,9 @@ pub fn effective_capabilities(config: &Config) -> CapabilitiesResponse {
},
},
features: FeatureCapabilities {
// The relay needs Grin chain access to be serviceable.
grin_relay: config.features.grin_relay && chain_serviceable(config, "grin"),
// The relay is a non-custodial mailbox (the wallet broadcasts
// locally), so it is NOT coupled to this backend's Grin chain access.
grin_relay: config.features.grin_relay,
prices: config.features.prices,
// Nostr identity needs the canonical PUBLIC_API_URL.
nostr_identity: config.features.nostr_identity
+13 -19
View File
@@ -19,7 +19,6 @@ use axum::{
use serde::Serialize;
use crate::config::Config;
use crate::error::AppError;
use crate::AppState;
#[derive(Debug, Serialize)]
@@ -62,18 +61,13 @@ fn coarse_version() -> String {
}
fn enabled_chain_symbols(config: &Config) -> Vec<String> {
let c = &config.features.chains;
[
("btc", c.btc),
("ltc", c.ltc),
("xmr", c.xmr),
("wow", c.wow),
("grin", c.grin),
]
.iter()
.filter(|(_, on)| *on)
.map(|(sym, _)| sym.to_string())
.collect()
// Use the SAME serviceability predicate as /capabilities, so the public
// landing never advertises a chain that capabilities reports as off.
["btc", "ltc", "xmr", "wow", "grin"]
.iter()
.filter(|sym| crate::api::capabilities::chain_serviceable(config, sym))
.map(|sym| sym.to_string())
.collect()
}
/// Build the public projection from config, honoring each per-field toggle.
@@ -94,14 +88,14 @@ pub fn build_server_info(config: &Config) -> ServerInfo {
}
}
/// `GET /api/v1/server-info` — the landing read model. `404` when landing is off
/// (so it does not hint that an admin plane exists).
#[allow(clippy::result_large_err)]
pub async fn server_info(State(state): State<Arc<AppState>>) -> Result<Json<ServerInfo>, AppError> {
/// `GET /api/v1/server-info` — the landing read model. A BARE `404` when landing
/// is off (matching an unmatched route — the `{error,code}` envelope would itself
/// hint the route exists, so it is deliberately not used here).
pub async fn server_info(State(state): State<Arc<AppState>>) -> Response {
if !state.config.landing.enabled {
return Err(AppError::NotFound("not found".into()));
return StatusCode::NOT_FOUND.into_response();
}
Ok(Json(build_server_info(&state.config)))
Json(build_server_info(&state.config)).into_response()
}
/// `GET /` — minimal HTML rendered from the read model; bare `404` when off.
-3
View File
@@ -291,7 +291,6 @@ pub struct LandingConfig {
pub expose_chains: bool,
/// Emit `price_feed.enabled`. Default off.
pub expose_price_feed: bool,
pub expose_features: bool,
pub expose_uptime: bool,
pub stats_enabled: bool,
pub stats_cache_hours: u64,
@@ -443,7 +442,6 @@ impl Config {
expose_version: env_bool("PUBLIC_EXPOSE_VERSION", false),
expose_chains: env_bool("PUBLIC_EXPOSE_CHAINS", false),
expose_price_feed: env_bool("PUBLIC_EXPOSE_PRICE_FEED", false),
expose_features: env_bool("PUBLIC_EXPOSE_FEATURES", false),
expose_uptime: env_bool("PUBLIC_EXPOSE_UPTIME", false),
stats_enabled: env_bool("PUBLIC_STATS_ENABLED", false),
stats_cache_hours: env_parse("PUBLIC_STATS_CACHE_HOURS", 24u64)?,
@@ -706,7 +704,6 @@ mod tests {
expose_version: false,
expose_chains: false,
expose_price_feed: false,
expose_features: false,
expose_uptime: false,
stats_enabled: false,
stats_cache_hours: 24,
+6 -5
View File
@@ -24,10 +24,11 @@ use super::{unique_violation_as, Database};
const COLS: &str = "id, pubkey, label, scope, created_at, created_by_kind, \
activated_at, activation_deadline, revoked_at, last_used_at, integrity_mac";
/// Advisory-lock key serializing add/revoke/rotate, so the count-based guards
/// (the `ADMIN_MAX_KEYS` cap and the last-key floor) are evaluated and acted on
/// atomically rather than as racy check-then-mutate.
const ADMIN_KEYS_LOCK_KEY: i64 = 0x5311_4D17_4B59_0001;
/// Advisory-lock key serializing all admin-key mutations (add/revoke/rotate, and
/// bootstrap), so the count-based guards (the `ADMIN_MAX_KEYS` cap, the last-key
/// floor, the single-bootstrap guard) are evaluated and acted on atomically
/// rather than as racy check-then-mutate.
pub(crate) const ADMIN_KEYS_LOCK_KEY: i64 = 0x5311_4D17_4B59_0001;
/// Outcome of an audited key add.
pub enum AddKeyOutcome {
@@ -396,7 +397,7 @@ async fn insert_admin_key(
/// Take the admin-keys mutation advisory lock (transaction-scoped). Serializes
/// add/revoke/rotate so the cap and last-key floor are race-free.
async fn admin_keys_mutate_lock(conn: &mut PgConnection) -> Result<(), AppError> {
pub(crate) async fn admin_keys_mutate_lock(conn: &mut PgConnection) -> Result<(), AppError> {
sqlx::query("SELECT pg_advisory_xact_lock($1)")
.bind(ADMIN_KEYS_LOCK_KEY)
.execute(&mut *conn)
+14
View File
@@ -80,6 +80,17 @@ impl Database {
Ok(exists)
}
/// Whether the DB has any admin key (live or revoked) — i.e. it was once
/// bootstrapped. Used so a deleted latch row on a CLI-bootstrapped instance
/// re-adopts `locked` instead of silently dropping to `uninitialized`.
#[instrument(skip(self))]
pub async fn has_any_admin_keys(&self) -> Result<bool, AppError> {
let exists = sqlx::query_scalar::<_, bool>("SELECT EXISTS (SELECT 1 FROM admin_keys)")
.fetch_one(self.pool())
.await?;
Ok(exists)
}
/// Create the latch row if absent. `locked` adopts an existing deployment
/// (state=locked, completed/locked now); otherwise a fresh install begins
/// `uninitialized`. No-op if the row already exists.
@@ -116,6 +127,9 @@ impl Database {
pub async fn bootstrap_admin(&self, pubkey: &str, secret: &str) -> Result<(), AppError> {
use crate::core::crypto::admin_mac::{compute_admin_key_mac, AdminKeyMacInput};
let mut tx = self.pool().begin().await?;
// Serialize with all admin-key mutations so two concurrent `setup` runs
// cannot both pass the single-bootstrap guard and seed two admins.
super::admin_keys::admin_keys_mutate_lock(&mut tx).await?;
let has_admin = sqlx::query_scalar::<_, bool>(
"SELECT EXISTS (SELECT 1 FROM admin_keys WHERE revoked_at IS NULL AND activated_at IS NOT NULL)",
+16 -6
View File
@@ -46,8 +46,11 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
SetupState::Fresh => {
// Adopt an existing deployment as already-bootstrapped (so a live
// upgrade never exposes a setup window); a truly empty DB begins
// uninitialized, awaiting `smirk-admin setup`.
let adopt = db.has_any_users().await?;
// uninitialized, awaiting `smirk-admin setup`. Adopt on EITHER
// wallet users OR admin keys, so deleting the latch row on a
// CLI-bootstrapped-but-no-users instance re-latches Locked rather
// than silently downgrading to uninitialized.
let adopt = db.has_any_users().await? || db.has_any_admin_keys().await?;
db.init_server_config(secret, adopt).await?;
if adopt {
tracing::info!("existing deployment adopted: bootstrap latched (locked)");
@@ -172,10 +175,17 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
if state.config.admin.enabled {
let admin_addr = state.config.admin.bind.clone();
// Fail-closed bind guard: refuse a non-loopback admin bind unless the
// operator explicitly opts in (confidentiality is by socket).
let is_loopback = admin_addr.starts_with("127.")
|| admin_addr.starts_with("[::1]")
|| admin_addr.starts_with("localhost");
// operator explicitly opts in (confidentiality is by socket). Resolve the
// address (so "localhost" works) and require EVERY resolved addr to be
// loopback — a prefix check would accept "localhost.evil.com".
use std::net::ToSocketAddrs;
let is_loopback = match admin_addr.to_socket_addrs() {
Ok(addrs) => {
let addrs: Vec<_> = addrs.collect();
!addrs.is_empty() && addrs.iter().all(|a| a.ip().is_loopback())
}
Err(_) => false,
};
if !is_loopback && !state.config.admin.allow_public_bind {
return Err(format!(
"ADMIN_BIND {admin_addr} is not loopback; set ADMIN_ALLOW_PUBLIC_BIND=true to override \