From e80dc53b0ef6cbd27a740d2208e6c3a9282798d8 Mon Sep 17 00:00:00 2001 From: CleverWild Date: Mon, 7 Sep 2026 16:22:33 +0200 Subject: [PATCH] fix(vault): read the unseal threshold from the recorded split parameters --- .../2026-02-14-171124-0000_init/up.sql | 6 +- .../src/actors/vault_coordinator/mod.rs | 51 +++++--- server/crates/arbiter-server/src/db/schema.rs | 1 + .../arbiter-server/tests/vault/lifecycle.rs | 109 +++++++++++++++++- 4 files changed, 152 insertions(+), 15 deletions(-) diff --git a/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql b/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql index 42c7756..8db1fb0 100644 --- a/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql +++ b/server/crates/arbiter-server/migrations/2026-02-14-171124-0000_init/up.sql @@ -37,7 +37,11 @@ create table if not exists tls_history ( create table if not exists arbiter_settings ( id INTEGER not null PRIMARY KEY CHECK (id = 1), -- singleton row, id must be 1 root_key_id integer references root_key_history (id) on delete RESTRICT, -- if null, means wasn't bootstrapped yet - tls_id integer references tls_history (id) on delete RESTRICT + tls_id integer references tls_history (id) on delete RESTRICT, + -- Shamir threshold of the split that produced the stored shares. Null before bootstrap. + -- Recorded rather than recomputed: an aborted operator replacement leaves fewer share + -- rows than the split has shares, and a recomputed threshold would then be wrong. + shamir_threshold integer ) STRICT; insert into arbiter_settings (id) values (1) on conflict do nothing; diff --git a/server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs b/server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs index af967fd..524bd6e 100644 --- a/server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs +++ b/server/crates/arbiter-server/src/actors/vault_coordinator/mod.rs @@ -142,6 +142,36 @@ fn decrypt_share( Ok(share_buffer.read().clone()) } +/// Records the threshold of the split that produced the stored shares. +async fn store_threshold(conn: &mut db::DatabaseConnection, threshold: usize) -> Result<(), Error> { + // A threshold that doesn't fit in the column is a bug, not a real empty-committee refusal. + let threshold = i32::try_from(threshold).map_err(|_| Error::BrokenDatabase)?; + let rows_updated = diesel::update(schema::arbiter_settings::table) + .set(schema::arbiter_settings::shamir_threshold.eq(Some(threshold))) + .execute(conn) + .await?; + // The singleton row always exists (up.sql seeds it), so anything else means the update did + // not land -- bootstrap would then report success while the threshold stays NULL forever. + if rows_updated != 1 { + return Err(Error::BrokenDatabase); + } + Ok(()) +} + +/// Reads back the threshold recorded at bootstrap or re-key time. +async fn load_threshold(conn: &mut db::DatabaseConnection) -> Result { + let stored: Option = schema::arbiter_settings::table + .select(schema::arbiter_settings::shamir_threshold) + .first(conn) + .await?; + + // A missing or out-of-range value here means the recorded threshold is corrupt, not that the + // committee is genuinely empty -- `EmptyCommittee` is reserved for the real domain refusal. + stored + .and_then(|threshold| usize::try_from(threshold).ok()) + .ok_or(Error::BrokenDatabase) +} + /// §3.4: Split the seal key across ordinary + recovery operators. /// Threshold = `shamir_threshold(ordinary_count)`; total shares = ordinary + recovery. /// When `ordinary_count` == 1 (threshold = 1), vsss-rs does not support a proper split, @@ -212,6 +242,8 @@ async fn finalize_bootstrap( .await?; } + store_threshold(&mut conn, threshold).await?; + vault.ask(Bootstrap { seal_key }).await.map_err(|err| { error!(?err, "Vault bootstrap failed"); Error::VaultError @@ -230,12 +262,7 @@ async fn finalize_unseal( let mut conn = db.get().await?; // Determine whether shares were stored as raw keys (threshold=1) or vsss-rs splits (threshold>=2). - let ordinary_operator_count: i64 = schema::operator::table - .count() - .get_result(&mut conn) - .await?; - let threshold = - shamir_threshold(ordinary_operator_count as usize).ok_or(Error::EmptyCommittee)?; + let threshold = load_threshold(&mut conn).await?; let mut shares: Vec> = Vec::new(); @@ -372,6 +399,8 @@ async fn finalize_rekey( .await?; } + store_threshold(&mut conn, threshold).await?; + drop(conn); let new_seal_key = KeyCell::from(new_seal_key_bytes); @@ -582,16 +611,12 @@ impl VaultCoordinator { impl VaultCoordinator { /// Initializes `CoordinatorState::Unsealing` on first call if still `Idle`. - /// Threshold is based on ordinary operator count only (§3.4). + /// Threshold comes from the recorded split parameters (§3.4), not from a live row count. async fn ensure_unsealing_state(&mut self) -> Result<(), Error> { if matches!(self.state, CoordinatorState::Idle) { let mut conn = self.db.get().await?; - let ordinary_count: i64 = schema::operator::table - .count() - .get_result(&mut conn) - .await?; - let threshold = shamir_threshold(usize::try_from(ordinary_count).unwrap_or_default()) - .ok_or(Error::EmptyCommittee)?; + let threshold = load_threshold(&mut conn).await?; + drop(conn); self.state = CoordinatorState::Unsealing { threshold, ordinary_passphrases: HashMap::new(), diff --git a/server/crates/arbiter-server/src/db/schema.rs b/server/crates/arbiter-server/src/db/schema.rs index c71717c..9922733 100644 --- a/server/crates/arbiter-server/src/db/schema.rs +++ b/server/crates/arbiter-server/src/db/schema.rs @@ -17,6 +17,7 @@ diesel::table! { id -> Integer, root_key_id -> Nullable, tls_id -> Nullable, + shamir_threshold -> Nullable, } } diff --git a/server/crates/arbiter-server/tests/vault/lifecycle.rs b/server/crates/arbiter-server/tests/vault/lifecycle.rs index 012e251..8551659 100644 --- a/server/crates/arbiter-server/tests/vault/lifecycle.rs +++ b/server/crates/arbiter-server/tests/vault/lifecycle.rs @@ -6,7 +6,7 @@ use arbiter_server::{ vault::{Error, GetState, Vault, VaultState}, vault_coordinator::{ ContributeBootstrap, ContributeRecoveryBootstrap, ContributeRecoveryUnseal, - Error as CoordinatorError, StartBootstrap, VaultCoordinator, + ContributeUnseal, Error as CoordinatorError, StartBootstrap, VaultCoordinator, }, }, crypto::{KeyCell, encryption::v1::{Nonce, ROOT_KEY_TAG}}, @@ -310,3 +310,110 @@ async fn empty_committee_is_rejected_without_panicking() { kameo::error::SendError::HandlerError(CoordinatorError::EmptyCommittee) )); } + +/// An approved-but-unfinished operator replacement deletes a share row. The unseal threshold +/// must still describe the split that is actually stored, not the surviving row count. +/// +/// Four ordinary operators give a real 3-of-4 `vsss-rs` split (`shamir_threshold(4) == 3`). +/// Deleting one share row leaves 3 rows, and `shamir_threshold(3) == 2` -- a *different* number +/// from the recorded threshold. A recount-based unseal would therefore finalize one contribution +/// early, combine only 2 shares of a 3-of-4 split, and fail to reconstruct the seal key. +#[tokio::test] +#[test_log::test] +async fn unseal_threshold_survives_a_deleted_share_row() { + let db = db::create_test_pool().await; + let bus = GlobalActors::spawn_message_bus(); + let vault_ref = Vault::spawn(Vault::new(db.clone(), bus).await.unwrap()); + let coordinator = VaultCoordinator::spawn(VaultCoordinator::new(db.clone(), vault_ref)); + + // Four ordinary operators: threshold is 3-of-4. + let mut ids = Vec::new(); + for n in 1..=4u8 { + let mut conn = db.get().await.unwrap(); + let id: i32 = insert_into(schema::operator_identity::table) + .values(schema::operator_identity::public_key.eq(vec![n; 32])) + .returning(schema::operator_identity::id) + .get_result(&mut conn) + .await + .unwrap(); + ids.push(id); + } + + coordinator + .ask(StartBootstrap { + operator_id: ids[0], + declared_count: 4, + recovery_count: 0, + }) + .await + .unwrap(); + for (n, id) in ids.iter().enumerate() { + coordinator + .ask(ContributeBootstrap { + operator_id: *id, + passphrase: SafeCell::new(format!("pass-{n}").into_bytes()), + }) + .await + .unwrap(); + } + + let stored: Option = { + let mut conn = db.get().await.unwrap(); + schema::arbiter_settings::table + .select(schema::arbiter_settings::shamir_threshold) + .first(&mut conn) + .await + .unwrap() + }; + assert_eq!(stored, Some(3), "bootstrap must record the split threshold"); + + // Simulate the aborted replacement: one share row is gone, leaving 3 of the 4 shares. + { + let mut conn = db.get().await.unwrap(); + diesel::delete(schema::operator::table) + .filter(schema::operator::id.eq(Some(ids[3]))) + .execute(&mut conn) + .await + .unwrap(); + } + + // Restart and unseal with the three surviving operators' original passphrases. + drop(coordinator); + let bus2 = GlobalActors::spawn_message_bus(); + let vault_ref2 = Vault::spawn(Vault::new(db.clone(), bus2).await.unwrap()); + let coordinator2 = VaultCoordinator::spawn(VaultCoordinator::new(db.clone(), vault_ref2.clone())); + + let done = coordinator2 + .ask(ContributeUnseal { + operator_id: ids[0], + passphrase: SafeCell::new(b"pass-0".to_vec()), + }) + .await + .unwrap(); + assert!(!done, "one share must not be enough for a 3-of-4 split"); + + let done = coordinator2 + .ask(ContributeUnseal { + operator_id: ids[1], + passphrase: SafeCell::new(b"pass-1".to_vec()), + }) + .await + .unwrap(); + assert!( + !done, + "two shares must not be enough for a 3-of-4 split -- a recount would wrongly finalize here" + ); + + let done = coordinator2 + .ask(ContributeUnseal { + operator_id: ids[2], + passphrase: SafeCell::new(b"pass-2".to_vec()), + }) + .await + .unwrap(); + assert!(done, "three shares must reconstruct a 3-of-4 split"); + assert_eq!( + vault_ref2.ask(GetState {}).await.unwrap(), + VaultState::Unsealed + ); +}