fix(admin): harden allowlist per adversarial review (Subsystems 0-1)

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.
This commit is contained in:
jwinterm
2026-06-26 07:14:03 -04:00
parent 62104ca85e
commit 5df12d3df8
4 changed files with 118 additions and 29 deletions
+31 -19
View File
@@ -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<Uuid>,
) -> Result<Json<OkResponse>, 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
+16
View File
@@ -22,6 +22,9 @@ pub struct AdminKeyMacInput<'a> {
pub scope: &'a str,
pub created_at: DateTime<Utc>,
pub activated_at: Option<DateTime<Utc>>,
/// Bound so a DB-write attacker cannot revive an expired pending key by
/// pushing out / nulling its deadline.
pub activation_deadline: Option<DateTime<Utc>>,
pub revoked_at: Option<DateTime<Utc>>,
}
@@ -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();
+70 -10
View File
@@ -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<AdminKey, AppError> {
max_live: i64,
) -> Result<AddKeyOutcome, AppError> {
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<Option<AdminKey>, AppError> {
keep_min_live: bool,
) -> Result<RevokeKeyOutcome, AppError> {
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<Option<AdminKey>, 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<i64, AppError> {
let n =
sqlx::query_scalar::<_, i64>("SELECT COUNT(*) FROM admin_keys WHERE revoked_at IS NULL")
.fetch_one(&mut *conn)
.await?;
Ok(n)
}
+1
View File
@@ -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;