From 5df12d3df89b224ff1bde9498da1ff43d3c443d8 Mon Sep 17 00:00:00 2001 From: jwinterm Date: Fri, 26 Jun 2026 07:14:03 -0400 Subject: [PATCH] fix(admin): harden allowlist per adversarial review (Subsystems 0-1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a 5-lens adversarial review of the admin surface (6 confirmed findings, collapsing to 4 fixes; 2 dismissed after verification): - HIGH: last-key-revoke TOCTOU (lockout). The count(live)<=1 floor was checked in the handler, then revoke ran separately — two concurrent revokes could both pass and empty the allowlist. Moved the floor INTO revoke_admin_key_full under a per-tx advisory lock (admin_keys_mutate_lock); returns RevokeKeyOutcome {Revoked,NotFound,WouldEmptyAllowlist}. - LOW: ADMIN_MAX_KEYS cap TOCTOU. Same check-then-mutate race on add. Cap is now counted INSIDE create_admin_key_audited under the same lock; returns AddKeyOutcome {Created,CapReached}. rotate takes the lock too, so all add/revoke/rotate serialize. - LOW: integrity MAC didn't cover activation_deadline — a DB-write attacker could null/extend the deadline to revive an expired pending key without breaking the MAC. Added activation_deadline to AdminKeyMacInput + the canonical input + every compute/verify site; new KAT binds it. - LOW: get_active_admin_key (and create_*) logged the full pubkey via #[instrument] (params weren't skipped), defeating the prefix-only / no-social-graph design. Skipped pubkey/input from those spans. Dismissed (both skeptics not-a-bug): audit-chain JSONB `details` round-trip — the handlers always pass details:None and serde's sorted-key default makes the string case round-trip (the L1 chain test proves it). Documented v1 deferrals (login timing-oracle hardening, per-pubkey backoff, hard non-loopback bind guard) remain tracked, not regressions. All gates green: 138 lib + 138 integration, no openapi drift, secret-scan, leak. --- src/api/admin.rs | 50 +++++++++++++--------- src/core/crypto/admin_mac.rs | 16 ++++++++ src/infra/db/admin_keys.rs | 80 +++++++++++++++++++++++++++++++----- src/infra/db/mod.rs | 1 + 4 files changed, 118 insertions(+), 29 deletions(-) diff --git a/src/api/admin.rs b/src/api/admin.rs index 6fce255..4232e90 100644 --- a/src/api/admin.rs +++ b/src/api/admin.rs @@ -35,6 +35,7 @@ use crate::core::admin_session::AdminSessionManager; use crate::core::crypto::nip98::{descriptor_sha256, request_descriptor, verify_signed_action}; use crate::core::session::hash_refresh_token; use crate::error::AppError; +use crate::infra::db::{AddKeyOutcome, RevokeKeyOutcome}; use crate::models::db::{AdminKey, NewAdminAudit, NewAdminKey, NewAdminSession}; use crate::AppState; @@ -478,13 +479,6 @@ pub async fn admin_keys_add( let pubkey = req.pubkey.to_lowercase(); validate_admin_pubkey(&pubkey)?; - // Cap total live (active + pending) keys to prevent allowlist-stuffing. - if state.db.count_live_admin_keys().await? >= state.config.admin.max_keys as i64 { - return Err(AppError::Forbidden( - "admin key limit reached; revoke an existing key first".into(), - )); - } - let secret = &state.config.admin.key_integrity_secret; let audit = NewAdminAudit { action: "admin_key_added".into(), @@ -494,10 +488,24 @@ pub async fn admin_keys_add( details: None, ip_address: None, }; - let key = state + // The live-key cap is enforced INSIDE the insert transaction (race-free). + let key = match state .db - .create_admin_key_audited(pending_key(&state, pubkey, req.label), &audit, secret) - .await?; + .create_admin_key_audited( + pending_key(&state, pubkey, req.label), + &audit, + secret, + state.config.admin.max_keys as i64, + ) + .await? + { + AddKeyOutcome::Created(key) => key, + AddKeyOutcome::CapReached => { + return Err(AppError::Forbidden( + "admin key limit reached; revoke an existing key first".into(), + )) + } + }; let status = key_status(&key).to_string(); Ok(Json(KeyResponse { id: key.id.to_string(), @@ -528,11 +536,6 @@ pub async fn admin_keys_revoke( Path(id): Path, ) -> Result, AppError> { let ctx = admin_guard(&state, &headers).await?; - if state.db.count_live_admin_keys().await? <= 1 { - return Err(AppError::Forbidden( - "refusing to revoke the last admin key over the network; use the CLI".into(), - )); - } let secret = &state.config.admin.key_integrity_secret; let audit = NewAdminAudit { action: "admin_key_revoked".into(), @@ -542,12 +545,21 @@ pub async fn admin_keys_revoke( details: None, ip_address: None, }; - state + // The last-key floor is enforced INSIDE the revoke transaction (race-free): + // concurrent revokes cannot both pass the check and empty the allowlist. + match state .db - .revoke_admin_key_full(id, &audit, secret) + .revoke_admin_key_full(id, &audit, secret, true) .await? - .ok_or_else(|| AppError::NotFound("admin key not found or already revoked".into()))?; - Ok(Json(OkResponse { ok: true })) + { + RevokeKeyOutcome::Revoked(_) => Ok(Json(OkResponse { ok: true })), + RevokeKeyOutcome::NotFound => Err(AppError::NotFound( + "admin key not found or already revoked".into(), + )), + RevokeKeyOutcome::WouldEmptyAllowlist => Err(AppError::Forbidden( + "refusing to revoke the last admin key over the network; use the CLI".into(), + )), + } } /// Atomically rotate a key: revoke the old one (+ its sessions) and add a fresh diff --git a/src/core/crypto/admin_mac.rs b/src/core/crypto/admin_mac.rs index 686c8d7..1bd1b96 100644 --- a/src/core/crypto/admin_mac.rs +++ b/src/core/crypto/admin_mac.rs @@ -22,6 +22,9 @@ pub struct AdminKeyMacInput<'a> { pub scope: &'a str, pub created_at: DateTime, pub activated_at: Option>, + /// Bound so a DB-write attacker cannot revive an expired pending key by + /// pushing out / nulling its deadline. + pub activation_deadline: Option>, pub revoked_at: Option>, } @@ -35,6 +38,7 @@ pub fn compute_admin_key_mac(secret: &str, input: &AdminKeyMacInput) -> String { input.scope, input.created_at.timestamp_micros(), input.activated_at.map(|t| t.timestamp_micros()), + input.activation_deadline.map(|t| t.timestamp_micros()), input.revoked_at.map(|t| t.timestamp_micros()), ]) .to_string(); @@ -60,6 +64,7 @@ mod tests { scope: "admin", created_at: DateTime::from_timestamp(1_700_000_000, 0).unwrap(), activated_at: None, + activation_deadline: None, revoked_at: None, } } @@ -93,6 +98,17 @@ mod tests { assert!(!verify_admin_key_mac("secret", &active, &mac_revoked)); } + #[test] + fn activation_deadline_is_bound() { + // Nulling/extending a pending key's deadline must invalidate the MAC. + let mut with_deadline = input(); + with_deadline.activation_deadline = + Some(DateTime::from_timestamp(1_700_000_300, 0).unwrap()); + let mac = compute_admin_key_mac("secret", &with_deadline); + // Deadline cleared (the revive attack) no longer verifies. + assert!(!verify_admin_key_mac("secret", &input(), &mac)); + } + #[test] fn activation_state_is_bound() { let pending = input(); diff --git a/src/infra/db/admin_keys.rs b/src/infra/db/admin_keys.rs index 70931e3..f6862b9 100644 --- a/src/infra/db/admin_keys.rs +++ b/src/infra/db/admin_keys.rs @@ -24,6 +24,26 @@ 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; + +/// Outcome of an audited key add. +pub enum AddKeyOutcome { + Created(AdminKey), + /// The live-key cap (`ADMIN_MAX_KEYS`) is already reached. + CapReached, +} + +/// Outcome of a network key revoke. +pub enum RevokeKeyOutcome { + Revoked(AdminKey), + NotFound, + /// Refused: revoking would leave the allowlist with no live key (a lockout). + WouldEmptyAllowlist, +} + fn mac_input<'a>(k: &'a AdminKey) -> AdminKeyMacInput<'a> { AdminKeyMacInput { id: k.id, @@ -31,6 +51,7 @@ fn mac_input<'a>(k: &'a AdminKey) -> AdminKeyMacInput<'a> { scope: &k.scope, created_at: k.created_at, activated_at: k.activated_at, + activation_deadline: k.activation_deadline, revoked_at: k.revoked_at, } } @@ -38,7 +59,7 @@ fn mac_input<'a>(k: &'a AdminKey) -> AdminKeyMacInput<'a> { impl Database { /// Add an allowlist entry (pending — `activated_at` NULL until first login). /// Fails 409 if an active key with this pubkey already exists. - #[instrument(skip(self, secret))] + #[instrument(skip(self, input, secret))] pub async fn create_admin_key( &self, input: NewAdminKey, @@ -50,23 +71,33 @@ impl Database { /// Add an allowlist entry AND append its audit row in ONE transaction, so the /// audit write is fail-closed (a failed audit rolls back the key creation). - #[instrument(skip(self, audit, secret))] + #[instrument(skip(self, input, audit, secret))] pub async fn create_admin_key_audited( &self, input: NewAdminKey, audit: &NewAdminAudit, secret: &str, - ) -> Result { + max_live: i64, + ) -> Result { let mut tx = self.pool().begin().await?; + admin_keys_mutate_lock(&mut tx).await?; + // Count under the lock so the cap can't be raced by concurrent adds. + let live = live_key_count(&mut tx).await?; + if live >= max_live { + tx.rollback().await?; + return Ok(AddKeyOutcome::CapReached); + } let key = insert_admin_key(&mut tx, &input, secret).await?; self.append_admin_audit(&mut tx, audit, secret).await?; tx.commit().await?; - Ok(key) + Ok(AddKeyOutcome::Created(key)) } /// The active (non-revoked) allowlist entry for `pubkey`, IF its integrity /// MAC verifies. A mismatch returns `None` (fail-closed) and logs tampering. - #[instrument(skip(self, secret))] + /// `pubkey` is skipped from the span — the surface keeps only prefixes, no + /// full keys, in logs/audit (no social graph). + #[instrument(skip(self, pubkey, secret))] pub async fn get_active_admin_key( &self, pubkey: &str, @@ -208,16 +239,24 @@ impl Database { /// Soft-revoke a key, revoke ALL its sessions, and append the audit row — all /// in ONE transaction (so a revoked admin's still-valid access token is - /// rejected on its next call, and the audit is fail-closed). Returns the - /// updated key, or `None` if it was already revoked / does not exist. + /// rejected on its next call, and the audit is fail-closed). When + /// `keep_min_live` is set, refuses (atomically) to revoke the last live key. #[instrument(skip(self, audit, secret))] pub async fn revoke_admin_key_full( &self, id: Uuid, audit: &NewAdminAudit, secret: &str, - ) -> Result, AppError> { + keep_min_live: bool, + ) -> Result { let mut tx = self.pool().begin().await?; + admin_keys_mutate_lock(&mut tx).await?; + // Floor check INSIDE the locked tx: two concurrent revokes cannot both + // pass and empty the allowlist (a network lockout). + if keep_min_live && live_key_count(&mut tx).await? <= 1 { + tx.rollback().await?; + return Ok(RevokeKeyOutcome::WouldEmptyAllowlist); + } let sel = format!( "SELECT {COLS} FROM admin_keys WHERE id = $1 AND revoked_at IS NULL FOR UPDATE" ); @@ -227,7 +266,7 @@ impl Database { .await? else { tx.rollback().await?; - return Ok(None); + return Ok(RevokeKeyOutcome::NotFound); }; let revoked_at = Utc::now().trunc_subsecs(6); let mac = compute_admin_key_mac( @@ -255,7 +294,7 @@ impl Database { .await?; self.append_admin_audit(&mut tx, audit, secret).await?; tx.commit().await?; - Ok(Some(updated)) + Ok(RevokeKeyOutcome::Revoked(updated)) } /// Atomically rotate a key: revoke `old_id` (+ its sessions), create a fresh @@ -272,6 +311,7 @@ impl Database { secret: &str, ) -> Result, AppError> { let mut tx = self.pool().begin().await?; + admin_keys_mutate_lock(&mut tx).await?; let sel = format!( "SELECT {COLS} FROM admin_keys WHERE id = $1 AND revoked_at IS NULL FOR UPDATE" ); @@ -329,6 +369,7 @@ async fn insert_admin_key( scope: &input.scope, created_at, activated_at: None, + activation_deadline: input.activation_deadline, revoked_at: None, }, ); @@ -352,3 +393,22 @@ async fn insert_admin_key( "an active admin key with this pubkey already exists", )) } + +/// 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> { + sqlx::query("SELECT pg_advisory_xact_lock($1)") + .bind(ADMIN_KEYS_LOCK_KEY) + .execute(&mut *conn) + .await?; + Ok(()) +} + +/// Count live (non-revoked) keys on `conn` (so it reflects the locked tx). +async fn live_key_count(conn: &mut PgConnection) -> Result { + let n = + sqlx::query_scalar::<_, i64>("SELECT COUNT(*) FROM admin_keys WHERE revoked_at IS NULL") + .fetch_one(&mut *conn) + .await?; + Ok(n) +} diff --git a/src/infra/db/mod.rs b/src/infra/db/mod.rs index f1577fd..1cef8bc 100644 --- a/src/infra/db/mod.rs +++ b/src/infra/db/mod.rs @@ -18,6 +18,7 @@ mod sessions; mod user_keys; mod users; +pub use admin_keys::{AddKeyOutcome, RevokeKeyOutcome}; pub use challenges::ConsumedChallenge; pub use login_events::LoginStats;