mirror of
https://github.com/Such-Software/smirk-backend-core.git
synced 2026-10-17 15:31:38 +02:00
fix(erasure): harden per adversarial review (Subsystem 5)
6 confirmed findings (review wqrd2gka3), 3 root-cause fixes: - HIGH: execute_erasure had no status recheck and the sweeper's SKIP LOCKED row lock was released before execution (separate connection), so (a) a request CANCELLED after the sweep's SELECT was still executed, and (b) a fleet could double-execute one erasure (duplicate hash-chained audit row, double count). Fixed by making execution atomically CLAIM the row as its first statement — `UPDATE ... SET status='completed' WHERE id=$1 AND status='confirmed' RETURNING user_id` — and skipping (returns false) when 0 rows match. Only the first executor of a still-confirmed request proceeds; a cancelled/already-done row is a no-op; on any later failure the whole tx (claim included) rolls back. - account-existence oracle: the absent-user request path returned a FRESH random erasure_id + advancing scheduled_for each call, while a real account returned a STABLE get-or-create id — a re-call distinguisher. The request response is now constant-shape: a STABLE id (real, or a deterministic integrity-secret-keyed synthetic for the absent path), a fixed "pending" status, and NO scheduled_for (it's meaningful only at confirm, where grace starts, and is returned there). - config: ERASURE_GRACE_PERIOD_HOURS is bounded (<= 10 years) in validate(), so the u64->i64 cast can't wrap negative (defeating grace) and Duration::hours can't panic. Dismissed after verification: ERASURE_ENABLED without FEATURE_NOSTR_IDENTITY (validate already requires PUBLIC_API_URL; the nostr_identity coupling is a nicety). Tests extended: cancelled-request-not-executed (HIGH regression), double-execute no-op. 140 lib + 140 integration green, no drift, scans clean.
This commit is contained in:
+45
-25
@@ -91,12 +91,31 @@ pub struct ProofRequest {
|
||||
pub struct ErasureRequestResponse {
|
||||
pub erasure_id: String,
|
||||
pub status: String,
|
||||
pub scheduled_for: String,
|
||||
}
|
||||
|
||||
#[derive(Debug, Serialize)]
|
||||
pub struct StatusResponse {
|
||||
pub status: String,
|
||||
/// When execution is scheduled (set on confirm, when grace starts).
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub scheduled_for: Option<String>,
|
||||
}
|
||||
|
||||
/// Deterministic, unguessable erasure id for the absent-user path — STABLE across
|
||||
/// repeated calls (keyed by the integrity secret), so it is indistinguishable
|
||||
/// from a real get-or-create-stable id and cannot be probed as an existence
|
||||
/// oracle.
|
||||
fn synthetic_erasure_id(pubkey: &str, secret: &str) -> String {
|
||||
let mut h = Sha256::new();
|
||||
h.update(secret.as_bytes());
|
||||
h.update([0x1f]);
|
||||
h.update(b"erasure-synthetic");
|
||||
h.update([0x1f]);
|
||||
h.update(pubkey.as_bytes());
|
||||
let digest = h.finalize();
|
||||
let mut bytes = [0u8; 16];
|
||||
bytes.copy_from_slice(&digest[..16]);
|
||||
Uuid::from_bytes(bytes).to_string()
|
||||
}
|
||||
|
||||
/// Verify a proof for `purpose` over `action_path`, consume the nonce, and
|
||||
@@ -175,7 +194,12 @@ pub async fn request_erasure(
|
||||
let grace = state.config.retention.grace_period_hours as i64;
|
||||
let (pubkey, user) = prove(&state, &body, PURPOSE_REQUEST, "/account/erasure", None).await?;
|
||||
|
||||
match user {
|
||||
// Constant-shape across present/absent AND across repeated calls: a STABLE
|
||||
// erasure_id (real for a user, deterministic-synthetic otherwise) and a fixed
|
||||
// "pending" status. No scheduled_for here (it is meaningful only at confirm,
|
||||
// when grace starts) — echoing a stable-vs-advancing timestamp would leak
|
||||
// account existence. The real scheduled_for is returned by confirm.
|
||||
let erasure_id = match user {
|
||||
Some(u) => {
|
||||
let req = state
|
||||
.db
|
||||
@@ -189,21 +213,14 @@ pub async fn request_erasure(
|
||||
)
|
||||
.await;
|
||||
info!(erasure_id = %req.id, "erasure requested");
|
||||
Ok(Json(ErasureRequestResponse {
|
||||
erasure_id: req.id.to_string(),
|
||||
status: req.status,
|
||||
scheduled_for: req.scheduled_for.to_rfc3339(),
|
||||
}))
|
||||
req.id.to_string()
|
||||
}
|
||||
None => {
|
||||
// Constant-shape: a synthetic id, no row, no audit.
|
||||
Ok(Json(ErasureRequestResponse {
|
||||
erasure_id: Uuid::new_v4().to_string(),
|
||||
status: "pending".into(),
|
||||
scheduled_for: (Utc::now() + chrono::Duration::hours(grace)).to_rfc3339(),
|
||||
}))
|
||||
}
|
||||
}
|
||||
None => synthetic_erasure_id(&pubkey, integrity_secret(&state)),
|
||||
};
|
||||
Ok(Json(ErasureRequestResponse {
|
||||
erasure_id,
|
||||
status: "pending".into(),
|
||||
}))
|
||||
}
|
||||
|
||||
/// Phase 2: confirm (a second fresh proof bound to this `erasure_id`). Revokes
|
||||
@@ -243,7 +260,10 @@ pub async fn confirm_erasure(
|
||||
)
|
||||
.await;
|
||||
info!(erasure_id = %req.id, "erasure confirmed");
|
||||
Ok(Json(StatusResponse { status: req.status }))
|
||||
Ok(Json(StatusResponse {
|
||||
status: req.status,
|
||||
scheduled_for: Some(req.scheduled_for.to_rfc3339()),
|
||||
}))
|
||||
}
|
||||
|
||||
/// Cancel a live erasure during grace (a fresh `erasure_cancel` proof).
|
||||
@@ -278,7 +298,10 @@ pub async fn cancel_erasure(
|
||||
integrity_secret(&state),
|
||||
)
|
||||
.await;
|
||||
Ok(Json(StatusResponse { status: req.status }))
|
||||
Ok(Json(StatusResponse {
|
||||
status: req.status,
|
||||
scheduled_for: None,
|
||||
}))
|
||||
}
|
||||
|
||||
#[derive(Debug, Serialize)]
|
||||
@@ -369,13 +392,10 @@ pub async fn run_erasure_sweep(state: &Arc<AppState>, batch: i64) -> Result<u64,
|
||||
let secret = integrity_secret(state);
|
||||
let mut done = 0;
|
||||
for req in due {
|
||||
let Some(user_id) = req.user_id else { continue };
|
||||
match state
|
||||
.db
|
||||
.execute_erasure(req.id, user_id, purge, secret)
|
||||
.await
|
||||
{
|
||||
Ok(()) => done += 1,
|
||||
match state.db.execute_erasure(req.id, purge, secret).await {
|
||||
Ok(true) => done += 1,
|
||||
// false = another node/sweep already claimed it, or it was cancelled.
|
||||
Ok(false) => {}
|
||||
Err(e) => warn!(erasure_id = %req.id, error = %e, "erasure execution failed"),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -582,6 +582,13 @@ impl Config {
|
||||
"PUBLIC_API_URL is required when ERASURE_ENABLED is on (it binds the signed-action proof)",
|
||||
));
|
||||
}
|
||||
// Bound the grace window: a huge value panics Duration::hours and an
|
||||
// overflow on the i64 cast could wrap negative (defeating grace).
|
||||
if self.retention.grace_period_hours > 24 * 365 * 10 {
|
||||
return Err(cfg_err(
|
||||
"ERASURE_GRACE_PERIOD_HOURS must be <= 87600 (10 years)",
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// Per-enabled-chain infra config: hard error in production, warn in dev.
|
||||
|
||||
+26
-12
@@ -137,16 +137,37 @@ impl Database {
|
||||
/// Execute a confirmed erasure in ONE fail-closed transaction: per-table
|
||||
/// policy + cascade delete + tombstone + hash-chained audit. `purge_login`
|
||||
/// chooses purge (default) vs anonymize for `login_events`.
|
||||
///
|
||||
/// The FIRST statement atomically CLAIMS the row (`status='confirmed' ->
|
||||
/// 'completed'`), re-asserting the precondition the sweeper's now-released
|
||||
/// `SKIP LOCKED` lock can no longer guarantee. So a request cancelled after
|
||||
/// the sweep, or a row another node already claimed, is skipped (returns
|
||||
/// `false`) — no double-execute, no duplicate audit row, no executing a
|
||||
/// cancelled request. On any later failure the whole tx rolls back (the claim
|
||||
/// included), so it is retried next sweep. Returns whether it executed.
|
||||
#[instrument(skip(self, secret))]
|
||||
pub async fn execute_erasure(
|
||||
&self,
|
||||
id: Uuid,
|
||||
user_id: Uuid,
|
||||
purge_login: bool,
|
||||
secret: &str,
|
||||
) -> Result<(), AppError> {
|
||||
) -> Result<bool, AppError> {
|
||||
let mut tx = self.pool().begin().await?;
|
||||
|
||||
// Atomic claim + tombstone: only the first executor of a still-confirmed
|
||||
// request proceeds. `user_id` is still set here (delete happens below).
|
||||
let claimed: Option<(Option<Uuid>,)> = sqlx::query_as(
|
||||
"UPDATE erasure_requests SET status = 'completed', completed_at = NOW() \
|
||||
WHERE id = $1 AND status = 'confirmed' RETURNING user_id",
|
||||
)
|
||||
.bind(id)
|
||||
.fetch_optional(&mut *tx)
|
||||
.await?;
|
||||
let Some((Some(user_id),)) = claimed else {
|
||||
tx.rollback().await?;
|
||||
return Ok(false);
|
||||
};
|
||||
|
||||
// login_events: the (origin, asset, time) quasi-identifier — the cascade
|
||||
// only nulls user_id, so purge (default) or anonymize all three here.
|
||||
if purge_login {
|
||||
@@ -174,20 +195,13 @@ impl Database {
|
||||
.await?;
|
||||
|
||||
// Structural delete: cascades wallets/sessions/user_keys/owned slatepacks;
|
||||
// SET NULL on counterparty/audit/login links (incl. this tombstone row).
|
||||
// SET NULL on counterparty/audit/login links (incl. this row's user_id,
|
||||
// leaving it a completed tombstone with the link scrubbed).
|
||||
sqlx::query("DELETE FROM users WHERE id = $1")
|
||||
.bind(user_id)
|
||||
.execute(&mut *tx)
|
||||
.await?;
|
||||
|
||||
// Tombstone (user_id was just nulled by the cascade).
|
||||
sqlx::query(
|
||||
"UPDATE erasure_requests SET status = 'completed', completed_at = NOW() WHERE id = $1",
|
||||
)
|
||||
.bind(id)
|
||||
.execute(&mut *tx)
|
||||
.await?;
|
||||
|
||||
self.append_admin_audit(
|
||||
&mut tx,
|
||||
&NewAdminAudit {
|
||||
@@ -203,6 +217,6 @@ impl Database {
|
||||
.await?;
|
||||
|
||||
tx.commit().await?;
|
||||
Ok(())
|
||||
Ok(true)
|
||||
}
|
||||
}
|
||||
|
||||
+75
-13
@@ -26,6 +26,21 @@ fn random_signer() -> SigningKey {
|
||||
}
|
||||
}
|
||||
|
||||
async fn mk_user(db: &smirk_backend_core::infra::db::Database, nostr_pubkey: &str) -> uuid::Uuid {
|
||||
db.create_user(NewUser {
|
||||
username: None,
|
||||
pubkey_hash: Some(format!("pk-{}", uuid::Uuid::new_v4())),
|
||||
nostr_pubkey: Some(nostr_pubkey.to_string()),
|
||||
wallet_birthday: None,
|
||||
seed_fingerprint: None,
|
||||
xmr_start_height: None,
|
||||
wow_start_height: None,
|
||||
})
|
||||
.await
|
||||
.unwrap()
|
||||
.id
|
||||
}
|
||||
|
||||
/// Build a `Nostr <base64(event)>` signed-action token for an erasure purpose.
|
||||
fn sign(
|
||||
sk: &SigningKey,
|
||||
@@ -83,19 +98,7 @@ async fn erasure_request_confirm_execute_flow() {
|
||||
// A user with a linked nostr identity that we control the key for.
|
||||
let sk = random_signer();
|
||||
let pk = hex::encode(sk.verifying_key().to_bytes());
|
||||
app.state
|
||||
.db
|
||||
.create_user(NewUser {
|
||||
username: None,
|
||||
pubkey_hash: Some(format!("pk-{}", uuid::Uuid::new_v4())),
|
||||
nostr_pubkey: Some(pk.clone()),
|
||||
wallet_birthday: None,
|
||||
seed_fingerprint: None,
|
||||
xmr_start_height: None,
|
||||
wow_start_height: None,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
mk_user(&app.state.db, &pk).await;
|
||||
|
||||
let challenge = |purpose: &'static str| {
|
||||
let app = &app;
|
||||
@@ -218,6 +221,65 @@ async fn erasure_request_confirm_execute_flow() {
|
||||
assert!(body["erasure_id"].as_str().is_some());
|
||||
assert_eq!(body["status"], "pending");
|
||||
|
||||
let secret = app.state.config.admin.key_integrity_secret.clone();
|
||||
|
||||
// [HIGH regression] a request CANCELLED after confirmation is NOT executed:
|
||||
// execute claims `WHERE status='confirmed'`, so a cancelled row is skipped.
|
||||
let pk_c = hex::encode(random_signer().verifying_key().to_bytes());
|
||||
let uc = mk_user(&app.state.db, &pk_c).await;
|
||||
let rc = app.state.db.request_erasure(uc, "h", 0).await.unwrap();
|
||||
app.state
|
||||
.db
|
||||
.confirm_erasure_request(rc.id, uc, 0)
|
||||
.await
|
||||
.unwrap();
|
||||
app.state
|
||||
.db
|
||||
.cancel_erasure_request(rc.id, uc)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(
|
||||
!app.state
|
||||
.db
|
||||
.execute_erasure(rc.id, true, &secret)
|
||||
.await
|
||||
.unwrap(),
|
||||
"cancelled request is not executed"
|
||||
);
|
||||
assert!(
|
||||
app.state
|
||||
.db
|
||||
.find_user_by_nostr_pubkey(&pk_c)
|
||||
.await
|
||||
.unwrap()
|
||||
.is_some(),
|
||||
"cancelled user survives"
|
||||
);
|
||||
|
||||
// Double-execution is a no-op (idempotent claim): the second call returns false.
|
||||
let pk_d = hex::encode(random_signer().verifying_key().to_bytes());
|
||||
let ud = mk_user(&app.state.db, &pk_d).await;
|
||||
let rd = app.state.db.request_erasure(ud, "h", 0).await.unwrap();
|
||||
app.state
|
||||
.db
|
||||
.confirm_erasure_request(rd.id, ud, 0)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(app
|
||||
.state
|
||||
.db
|
||||
.execute_erasure(rd.id, true, &secret)
|
||||
.await
|
||||
.unwrap());
|
||||
assert!(
|
||||
!app.state
|
||||
.db
|
||||
.execute_erasure(rd.id, true, &secret)
|
||||
.await
|
||||
.unwrap(),
|
||||
"second execute is a no-op"
|
||||
);
|
||||
|
||||
for k in [
|
||||
"ERASURE_ENABLED",
|
||||
"ADMIN_KEY_INTEGRITY_SECRET",
|
||||
|
||||
Reference in New Issue
Block a user