diff --git a/server/Cargo.lock b/server/Cargo.lock index d08434b..c608b77 100644 --- a/server/Cargo.lock +++ b/server/Cargo.lock @@ -789,6 +789,7 @@ dependencies = [ "smlang", "strum 0.28.0", "subtle", + "tempfile", "test-log", "thiserror", "tokio", diff --git a/server/crates/arbiter-server/Cargo.toml b/server/crates/arbiter-server/Cargo.toml index b17e739..5bf6960 100644 --- a/server/crates/arbiter-server/Cargo.toml +++ b/server/crates/arbiter-server/Cargo.toml @@ -60,6 +60,7 @@ rstest.workspace = true test-log = { version = "0.2", default-features = false, features = ["trace"] } ml-dsa.workspace = true mockall = "0.15.0" +tempfile = "3.27.0" [lib] doctest = false diff --git a/server/crates/arbiter-server/src/actors/bootstrap.rs b/server/crates/arbiter-server/src/actors/bootstrap.rs index c1fe6af..ac2a5f9 100644 --- a/server/crates/arbiter-server/src/actors/bootstrap.rs +++ b/server/crates/arbiter-server/src/actors/bootstrap.rs @@ -4,28 +4,43 @@ use arbiter_proto::{BOOTSTRAP_PATH, home_path}; use diesel::QueryDsl; use diesel_async::RunQueryDsl; use kameo::{Actor, messages}; -use rand::{RngExt, distr::Alphanumeric, make_rng, rngs::StdRng}; +use rand::{ + distr::{Alphanumeric, SampleString as _}, + make_rng, + rngs::StdRng, +}; +use std::path::Path; use subtle::ConstantTimeEq as _; use thiserror::Error; const TOKEN_LENGTH: usize = 64; -pub async fn generate_token() -> Result { - let rng: StdRng = make_rng(); +pub async fn generate_token(home: &Path) -> Result { + let mut rng: StdRng = make_rng(); - let token = rng.sample_iter(Alphanumeric).take(TOKEN_LENGTH).fold( - String::default(), - |mut accum, char| { - accum += char.to_string().as_str(); - accum - }, - ); + // `Alphanumeric` samples raw `u8` ASCII codes, not `char`s -- `SampleString::sample_string` + // is `rand`'s own documented way to turn that into an actual TOKEN_LENGTH-character string + // (see the "Passwords" example on `Alphanumeric`'s docs). A prior version of this function + // called `.to_string()` on the sampled `u8` directly, which stringifies the numeric byte + // value (e.g. `65` instead of `'A'`) rather than the character it represents, silently + // producing a variable-length, all-decimal-digit string instead of a real token. + let token = Alphanumeric.sample_string(&mut rng, TOKEN_LENGTH); - tokio::fs::write(home_path()?.join(BOOTSTRAP_PATH), token.as_str()).await?; + tokio::fs::write(home.join(BOOTSTRAP_PATH), token.as_str()).await?; Ok(token) } +/// A token file is only trustworthy if it looks like something `generate_token` could have +/// produced. Anything else -- empty (a crash between the file's truncate and write), foreign +/// content, or a trailing newline added by an editor -- must not be adopted as a live +/// credential: an empty file would make every empty-string token verify, and mismatched +/// content would silently lock out every operator holding the real, already-printed token. +#[must_use] +fn is_valid_token(candidate: &str) -> bool { + candidate.len() == TOKEN_LENGTH && candidate.chars().all(|c| c.is_ascii_alphanumeric()) +} + #[derive(Error, Debug)] pub enum Error { #[error("Database error: {0}")] @@ -44,31 +59,77 @@ pub struct Bootstrapper { } impl Bootstrapper { + /// Production constructor: resolves the real `~/.arbiter` directory and delegates. pub async fn new(db: &DatabasePool) -> Result { - let row_count: i64 = { - let mut conn = db.get().await?; - - schema::operator::table - .count() - .get_result(&mut conn) - .await? - }; - - let token = if row_count == 0 { - let token = generate_token().await?; - Some(token) - } else { - None - }; - - Ok(Self { token }) + let home = home_path()?; + Self::new_in(db, &home).await } -} -#[messages] -impl Bootstrapper { - #[message] - pub fn is_correct_token(&self, token: String) -> bool { + /// Carries all of `new`'s logic, parameterized on the directory the token file lives in. + /// `new` resolves the real home directory and calls this; tests call it directly with a + /// throwaway temp directory so they never touch the real `~/.arbiter/bootstrap_token`. + pub async fn new_in(db: &DatabasePool, home: &Path) -> Result { + let mut conn = db.get().await?; + + let bootstrapped: bool = schema::arbiter_settings::table + .select(schema::arbiter_settings::root_key_id) + .first::>(&mut conn) + .await? + .is_some(); + + if bootstrapped { + return Ok(Self { token: None }); + } + + let any_operator_registered: bool = schema::operator_identity::table + .count() + .get_result::(&mut conn) + .await? + > 0; + + if !any_operator_registered { + // Nobody has used the current token yet, so there is nothing to preserve across a + // restart: generate a fresh one, exactly as on a first run. Reusing an old file + // here would let a token survive a database reset, silently reviving trust in + // whoever still held it. + return Ok(Self { + token: Some(generate_token(home).await?), + }); + } + + // At least one operator has already registered with the current token: every other + // declared operator still needs that same token, including across a restart, so an + // existing file is reused rather than replaced -- but only if it still looks like a + // real token. A truncated, foreign, or corrupted file must not become a live + // credential (see `is_valid_token`). + let path = home.join(BOOTSTRAP_PATH); + let token = match tokio::fs::read_to_string(&path).await { + Ok(existing) if is_valid_token(&existing) => existing, + Ok(_) => { + // Replacing the file invalidates whatever token the already-registered + // operators were handed, so it must not happen quietly. The content itself is + // a credential and stays out of the log; the path is enough to act on. + tracing::warn!( + ?path, + "Bootstrap token file is not a well-formed token; replacing it" + ); + generate_token(home).await? + } + Err(err) if err.kind() == std::io::ErrorKind::NotFound => generate_token(home).await?, + Err(err) => return Err(Error::Io(err)), + }; + + Ok(Self { token: Some(token) }) + } + + /// Drops the token from memory. Called once the vault is bootstrapped: from then on, + /// operators are added through governance rather than through the token. + pub(crate) fn forget_token(&mut self) { + self.token = None; + } + + #[must_use] + fn is_correct_token(&self, token: &str) -> bool { self.token.as_ref().is_some_and(|expected| { let expected_bytes = expected.as_bytes(); let token_bytes = token.as_bytes(); @@ -77,15 +138,28 @@ impl Bootstrapper { bool::from(choice) }) } +} +#[messages] +impl Bootstrapper { + /// Checks the token without retiring it: every operator in a declared committee + /// authenticates with the same token during bootstrap. #[message] - pub fn consume_token(&mut self, token: String) -> bool { - if self.is_correct_token(token) { - self.token = None; - true - } else { - false - } + #[must_use] + pub fn verify_token(&self, token: String) -> bool { + self.is_correct_token(&token) + } +} + +impl kameo::prelude::Message for Bootstrapper { + type Reply = (); + + async fn handle( + &mut self, + _msg: crate::actors::vault::events::Bootstrapped, + _ctx: &mut kameo::prelude::Context, + ) -> Self::Reply { + self.forget_token(); } } @@ -96,3 +170,150 @@ impl Bootstrapper { self.token.clone() } } + +#[cfg(test)] +mod tests { + use super::*; + use diesel::{ExpressionMethods as _, insert_into, update}; + + /// A multi-operator committee registers every member with the same token, so verifying it + /// must not consume it. Only a completed bootstrap retires the token. + #[tokio::test] + async fn token_verifies_repeatedly_until_bootstrap_completes() { + let mut bootstrapper = Bootstrapper { + token: Some("test-token".to_owned()), + }; + + assert!(bootstrapper.verify_token("test-token".to_owned())); + assert!(bootstrapper.verify_token("test-token".to_owned())); + assert!(!bootstrapper.verify_token("wrong-token".to_owned())); + + bootstrapper.forget_token(); + + assert!(!bootstrapper.verify_token("test-token".to_owned())); + assert!(bootstrapper.get_token().is_none()); + } + + /// Once the vault is bootstrapped, `Bootstrapper::new_in` must return with no token -- and + /// it must do so from `arbiter_settings.root_key_id` alone, taking the early return before + /// `home` is ever consulted. Uses `new_in` with a throwaway temp directory (never the + /// production `new`, which unconditionally resolves the real home directory before this + /// method even runs) so this test cannot touch the real filesystem regardless of outcome. + #[tokio::test] + async fn new_returns_no_token_once_the_vault_is_bootstrapped() { + let db = db::create_test_pool().await; + let mut conn = db.get().await.unwrap(); + + let root_key_history_id: i32 = insert_into(schema::root_key_history::table) + .values(&db::models::NewRootKeyHistory { + ciphertext: vec![0u8; 32], + tag: vec![0u8; 16], + root_key_encryption_nonce: vec![0u8; 24], + data_encryption_nonce: vec![0u8; 24], + schema_version: 1, + salt: vec![0u8; 16], + }) + .returning(schema::root_key_history::id) + .get_result(&mut conn) + .await + .unwrap(); + + update(schema::arbiter_settings::table) + .set(schema::arbiter_settings::root_key_id.eq(root_key_history_id)) + .execute(&mut conn) + .await + .unwrap(); + drop(conn); + + let home = tempfile::tempdir().unwrap(); + let bootstrapper = Bootstrapper::new_in(&db, home.path()).await.unwrap(); + + assert!(bootstrapper.get_token().is_none()); + } + + /// The file-reuse path (an operator has already registered, so bootstrap is unfinished) + /// must not adopt a corrupted token file as a live credential: it must reject it and + /// generate a fresh one instead. This is the Critical from the review, now reachable + /// safely because `new_in` takes a throwaway temp directory instead of the real home. + #[tokio::test] + async fn new_in_rejects_and_replaces_a_corrupted_token_file() { + let db = db::create_test_pool().await; + let mut conn = db.get().await.unwrap(); + + // At least one operator must have registered, or `new_in` would regenerate + // unconditionally regardless of the file (Important 2) and never exercise validation. + insert_into(schema::operator_identity::table) + .values(schema::operator_identity::public_key.eq(vec![0u8; 32])) + .execute(&mut conn) + .await + .unwrap(); + drop(conn); + + let home = tempfile::tempdir().unwrap(); + tokio::fs::write(home.path().join(BOOTSTRAP_PATH), "") + .await + .unwrap(); + + let bootstrapper = Bootstrapper::new_in(&db, home.path()).await.unwrap(); + + let token = bootstrapper + .get_token() + .expect("a fresh token must be generated"); + assert!(is_valid_token(&token)); + + // The replacement must also have landed on disk, not just in memory, so a restart + // reads back the same (now valid) token rather than the corrupted one again. + let on_disk = tokio::fs::read_to_string(home.path().join(BOOTSTRAP_PATH)) + .await + .unwrap(); + assert_eq!(on_disk, token); + } + + /// The second defect the task exists to fix: a restart between the first registration and + /// the completed bootstrap must not invalidate the token the other declared operators were + /// already given. With an operator registered and a well-formed file on disk, `new_in` has + /// to hand back exactly what it read instead of generating a replacement. + #[tokio::test] + async fn new_in_reuses_a_valid_token_file_across_a_restart() { + let db = db::create_test_pool().await; + let mut conn = db.get().await.unwrap(); + + // Without a registered operator, `new_in` regenerates unconditionally (Important 2 of + // the round-2 review) and never reaches the reuse path this test is about. + insert_into(schema::operator_identity::table) + .values(schema::operator_identity::public_key.eq(vec![0u8; 32])) + .execute(&mut conn) + .await + .unwrap(); + drop(conn); + + let home = tempfile::tempdir().unwrap(); + // Stands in for the token a previous run wrote and printed to the console. + let handed_out = "Zq7Z2rXaB90kLmNpQwErTyUiOpAsDfGhJkLzXcVbNmQwErTyUiOpAsDfGhJkLzXc"; + assert!(is_valid_token(handed_out)); + tokio::fs::write(home.path().join(BOOTSTRAP_PATH), handed_out) + .await + .unwrap(); + + let bootstrapper = Bootstrapper::new_in(&db, home.path()).await.unwrap(); + + assert_eq!(bootstrapper.get_token().as_deref(), Some(handed_out)); + } + + /// An empty file must not be adopted as a live credential: `is_correct_token`'s + /// `is_some_and` would enter its closure for `Some(String::new())`, and comparing two empty + /// byte slices is true, so an unauthenticated `Some("")` from the wire would otherwise + /// verify. Also pins the other corrupted-content shapes `is_valid_token` must reject. + #[test] + fn is_valid_token_rejects_anything_that_is_not_a_real_token() { + assert!(!is_valid_token("")); + assert!(!is_valid_token("too-short")); + assert!(!is_valid_token(&"a".repeat(TOKEN_LENGTH - 1))); + assert!(!is_valid_token(&"a".repeat(TOKEN_LENGTH + 1))); + // A trailing newline (e.g. from an editor) must not be silently accepted either. + assert!(!is_valid_token(&format!("{}\n", "a".repeat(TOKEN_LENGTH)))); + assert!(!is_valid_token(&"!".repeat(TOKEN_LENGTH))); + + assert!(is_valid_token(&"a".repeat(TOKEN_LENGTH))); + } +} diff --git a/server/crates/arbiter-server/src/actors/mod.rs b/server/crates/arbiter-server/src/actors/mod.rs index 812b1be..913ec0a 100644 --- a/server/crates/arbiter-server/src/actors/mod.rs +++ b/server/crates/arbiter-server/src/actors/mod.rs @@ -5,7 +5,7 @@ use crate::{ flow_coordinator::FlowCoordinator, operator_registry::OperatorRegistry, proposal_manager::{ProposalManager, events::ProposalApproved}, - vault::Vault, + vault::{Vault, events}, vault_coordinator::VaultCoordinator, }, db, @@ -17,6 +17,7 @@ use kameo_actors::{ message_bus::{MessageBus, Register}, }; use thiserror::Error; +use tracing::error; pub mod bootstrap; pub mod evm; @@ -54,6 +55,26 @@ impl GlobalActors { } pub async fn spawn(db: db::DatabasePool) -> Result { + let bootstrapper = Bootstrapper::new(&db).await?; + Self::spawn_with_bootstrapper(db, bootstrapper).await + } + + /// Test-facing: threads an explicit directory through to `Bootstrapper` instead of letting + /// it resolve the real home directory, so a test spawning a full `GlobalActors` can never + /// reach (let alone write to) the real `~/.arbiter/bootstrap_token`. Mirrors `spawn` + /// exactly, aside from where the token file lives. + pub async fn spawn_in( + db: db::DatabasePool, + home: &std::path::Path, + ) -> Result { + let bootstrapper = Bootstrapper::new_in(&db, home).await?; + Self::spawn_with_bootstrapper(db, bootstrapper).await + } + + async fn spawn_with_bootstrapper( + db: db::DatabasePool, + bootstrapper: Bootstrapper, + ) -> Result { let message_bus = Self::spawn_message_bus(); let key_holder = Vault::spawn(Vault::new(db.clone(), message_bus.clone()).await?); let operator_registry = OperatorRegistry::spawn(OperatorRegistry::default()); @@ -62,6 +83,7 @@ impl GlobalActors { db.clone(), key_holder.clone(), )); + let bootstrapper = Bootstrapper::spawn(bootstrapper); // Approved proposals are executed by whoever owns the kind, not by ProposalManager. for recipient in [ evm.clone().recipient::(), @@ -70,9 +92,23 @@ impl GlobalActors { ] { let _ = message_bus.tell(Register(recipient)).await; } + // The token guards bootstrap only: once the vault reports success, it must be retired. + // A dropped registration would leave the token valid forever with nothing else to + // notice, so a failure here is logged rather than silently discarded. + if let Err(err) = message_bus + .tell(Register( + bootstrapper.clone().recipient::(), + )) + .await + { + error!( + ?err, + "Failed to register Bootstrapper for the Bootstrapped event" + ); + } Ok(Self { - bootstrapper: Bootstrapper::spawn(Bootstrapper::new(&db).await?), + bootstrapper, proposal_manager: ProposalManager::spawn(ProposalManager::new(db, message_bus.clone())), vault: key_holder, vault_coordinator, diff --git a/server/crates/arbiter-server/src/actors/vault/mod.rs b/server/crates/arbiter-server/src/actors/vault/mod.rs index b4050c7..f994f72 100644 --- a/server/crates/arbiter-server/src/actors/vault/mod.rs +++ b/server/crates/arbiter-server/src/actors/vault/mod.rs @@ -222,7 +222,9 @@ impl Vault { }); info!("Vault bootstrapped successfully"); - let _ = self.events.tell(Publish(events::Bootstrapped)).await; + if let Err(err) = self.events.tell(Publish(events::Bootstrapped)).await { + error!(?err, "Failed to publish Bootstrapped event"); + } Ok(()) } diff --git a/server/crates/arbiter-server/src/db/functions.rs b/server/crates/arbiter-server/src/db/functions.rs index faf6465..3b5d119 100644 --- a/server/crates/arbiter-server/src/db/functions.rs +++ b/server/crates/arbiter-server/src/db/functions.rs @@ -1,6 +1,6 @@ //! Typed bindings for the SQLite scalar functions used in Diesel expressions. -use diesel::sql_types::{Integer, Text}; +use diesel::sql_types::Text; diesel::define_sql_function! { /// SQLite `unixepoch(modifier)` -- seconds since the Unix epoch. diff --git a/server/crates/arbiter-server/src/db/mod.rs b/server/crates/arbiter-server/src/db/mod.rs index 3f8352c..cffde4f 100644 --- a/server/crates/arbiter-server/src/db/mod.rs +++ b/server/crates/arbiter-server/src/db/mod.rs @@ -101,12 +101,17 @@ fn initialize_database(url: &str) -> Result<(), DatabaseSetupError> { /// # Panics /// Panics if the database path is not valid UTF-8. pub async fn create_pool(url: Option<&str>) -> Result { - let database_url = url.map(String::from).unwrap_or( - database_path()? + // Matched rather than `unwrap_or`, whose argument is evaluated even when `url` is `Some`: + // `database_path` resolves the real home directory and creates `~/.arbiter` as a side + // effect, so an eager call reaches the developer's home from every test that passes an + // explicit temp path, and fails outright wherever no home directory is writable. + let database_url = match url { + Some(url) => url.to_owned(), + None => database_path()? .to_str() .expect("database path is not valid UTF-8") .to_owned(), - ); + }; initialize_database(&database_url)?; diff --git a/server/crates/arbiter-server/src/peers/operator/auth/state.rs b/server/crates/arbiter-server/src/peers/operator/auth/state.rs index ab74c3e..9f86547 100644 --- a/server/crates/arbiter-server/src/peers/operator/auth/state.rs +++ b/server/crates/arbiter-server/src/peers/operator/auth/state.rs @@ -3,15 +3,18 @@ use super::{ Error, }; use crate::{ - actors::bootstrap::ConsumeToken, - db::{DatabasePool, schema::operator_identity}, + actors::bootstrap::VerifyToken, + db::{ + DatabasePool, + schema::{arbiter_settings, operator_identity}, + }, peers::operator::auth::Outbound, }; use arbiter_crypto::authn::{self, AuthChallenge, SigningContext}; use arbiter_proto::transport::Bi; use diesel::{ExpressionMethods as _, OptionalExtension as _, QueryDsl}; -use diesel_async::RunQueryDsl; +use diesel_async::{AsyncConnection as _, RunQueryDsl}; use tracing::error; pub(crate) struct ChallengeRequest { @@ -63,17 +66,32 @@ async fn register_key(db: &DatabasePool, pubkey: &authn::PublicKey) -> Result>(&mut *conn) + .await? + .is_some(); - Ok(id) + if already_bootstrapped { + error!("Bootstrap token used to register after the vault was already bootstrapped"); + return Err(Error::InvalidBootstrapToken); + } + + let id: i32 = diesel::insert_into(operator_identity::table) + .values((operator_identity::public_key.eq(pubkey_bytes),)) + .returning(operator_identity::id) + .get_result(&mut *conn) + .await?; + + Ok(id) + }) + .await } pub(super) struct AuthContext<'a, T: ?Sized> { @@ -151,20 +169,20 @@ where return Err(Error::InvalidChallengeSolution); } - // Resolve client id: bootstrap (consume token + register) or lookup + // Resolve client id: bootstrap (verify token, then register) or lookup let id = match bootstrap_token { Some(token) => { let token_ok: bool = self .conn .actors .bootstrapper - .ask(ConsumeToken { + .ask(VerifyToken { token: token.clone(), }) .await .map_err(|e| { - error!(?e, "Failed to consume bootstrap token"); - Error::internal("Failed to consume bootstrap token") + error!(?e, "Failed to verify bootstrap token"); + Error::internal("Failed to verify bootstrap token") })?; if !token_ok { @@ -176,7 +194,21 @@ where return Err(Error::InvalidBootstrapToken); } - register_key(&self.conn.db, pubkey).await? + match register_key(&self.conn.db, pubkey).await { + Ok(id) => id, + // `register_key` refuses a token that verified here but lost the race + // against bootstrap. Reported to the peer exactly like the refusal above, + // so that operator sees a protocol error rather than a handshake that + // stops with nothing on the wire. + Err(Error::InvalidBootstrapToken) => { + self.transport + .send(Err(Error::InvalidBootstrapToken)) + .await + .map_err(|_| Error::Transport)?; + return Err(Error::InvalidBootstrapToken); + } + Err(err) => return Err(err), + } } None => get_client_id(&self.conn.db, pubkey) .await? diff --git a/server/crates/arbiter-server/tests/client/auth.rs b/server/crates/arbiter-server/tests/client/auth.rs index 945e82b..4c1977a 100644 --- a/server/crates/arbiter-server/tests/client/auth.rs +++ b/server/crates/arbiter-server/tests/client/auth.rs @@ -1,4 +1,4 @@ -use super::common::ChannelTransport; +use super::common::{ChannelTransport, spawn_actors}; use arbiter_crypto::authn::{self, AuthChallenge, SigningContext}; use arbiter_proto::{ ClientMetadata, @@ -93,7 +93,7 @@ async fn insert_bootstrap_sentinel_operator(db: &db::DatabasePool) { async fn spawn_test_actors(db: &db::DatabasePool) -> GlobalActors { insert_bootstrap_sentinel_operator(db).await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { diff --git a/server/crates/arbiter-server/tests/common/mod.rs b/server/crates/arbiter-server/tests/common/mod.rs index 598eee9..42fe3a3 100644 --- a/server/crates/arbiter-server/tests/common/mod.rs +++ b/server/crates/arbiter-server/tests/common/mod.rs @@ -24,6 +24,36 @@ pub(crate) async fn bootstrapped_vault(db: &db::DatabasePool) -> Vault { actor } +/// Spawns a full `GlobalActors` for a test, backing `Bootstrapper`'s token file with a +/// throwaway temp directory rather than the real `~/.arbiter` -- a test must never be able to +/// reach, let alone write to, the developer's real bootstrap token file. +pub(crate) async fn spawn_actors(db: db::DatabasePool) -> GlobalActors { + let home = tempfile::tempdir().expect("failed to create a temp home directory for a test"); + GlobalActors::spawn_in(db, home.path()) + .await + .expect("failed to spawn GlobalActors for a test") +} + +/// Retries `probe` until it yields a value, then returns it. +/// +/// Effects that travel over the message bus are not visible the moment the publishing call +/// returns: `Publish` only enqueues to the bus's mailbox, which then delivers to each +/// subscriber's mailbox in turn. A test observing such an effect waits for it instead of +/// reading straight after the call that triggered it. +pub(crate) async fn eventually(what: &str, mut probe: F) -> T +where + F: FnMut() -> Fut, + Fut: Future>, +{ + for _ in 0..100 { + if let Some(value) = probe().await { + return value; + } + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + panic!("{what} did not happen within 2s"); +} + pub(crate) async fn root_key_history_id(db: &db::DatabasePool) -> i32 { let mut conn = db.get().await.unwrap(); let id = schema::arbiter_settings::table diff --git a/server/crates/arbiter-server/tests/governance.rs b/server/crates/arbiter-server/tests/governance.rs index 7e4cb13..21c4c31 100644 --- a/server/crates/arbiter-server/tests/governance.rs +++ b/server/crates/arbiter-server/tests/governance.rs @@ -46,6 +46,16 @@ where panic!("{what} did not happen within 2s"); } +/// Spawns a full `GlobalActors` for a test, backing `Bootstrapper`'s token file with a +/// throwaway temp directory rather than the real `~/.arbiter` -- a test must never be able to +/// reach, let alone write to, the developer's real bootstrap token file. +async fn spawn_actors(db: db::DatabasePool) -> GlobalActors { + let home = tempfile::tempdir().expect("failed to create a temp home directory for a test"); + GlobalActors::spawn_in(db, home.path()) + .await + .expect("failed to spawn GlobalActors for a test") +} + async fn register_operator(db: &db::DatabasePool, pubkey: &authn::PublicKey) -> OperatorIdentityId { let mut conn = db.get().await.unwrap(); insert_into(operator_identity::table) @@ -142,7 +152,7 @@ async fn insert_unapproved_client(db: &db::DatabasePool, pubkey: &authn::PublicK #[tokio::test] async fn create_proposal_returns_id() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { @@ -172,7 +182,7 @@ async fn create_proposal_returns_id() { #[tokio::test] async fn create_proposal_caps_the_ttl() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { @@ -213,7 +223,7 @@ async fn create_proposal_caps_the_ttl() { #[tokio::test] async fn single_operator_vote_reaches_quorum() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -258,7 +268,7 @@ async fn single_operator_vote_reaches_quorum() { #[tokio::test] async fn two_operator_first_vote_is_pending() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -304,7 +314,7 @@ async fn two_operator_first_vote_is_pending() { #[tokio::test] async fn duplicate_vote_rejected() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -365,7 +375,7 @@ async fn duplicate_vote_rejected() { #[tokio::test] async fn invalid_signature_rejected() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -406,7 +416,7 @@ async fn invalid_signature_rejected() { #[tokio::test] async fn query_pending_reports_a_tally_per_proposal() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { @@ -500,7 +510,7 @@ async fn query_pending_reports_a_tally_per_proposal() { #[tokio::test] async fn query_pending_excludes_already_voted() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -566,7 +576,7 @@ async fn query_pending_excludes_already_voted() { #[tokio::test] async fn expired_proposal_is_hidden_and_unvotable() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -624,7 +634,7 @@ async fn approve_sdk_client_writes_integrity_envelope() { use arbiter_server::db::schema::integrity_envelope; let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -683,7 +693,7 @@ async fn approve_sdk_client_writes_integrity_envelope() { #[tokio::test] async fn grant_wallet_access_on_quorum_approval() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -744,7 +754,7 @@ async fn grant_wallet_access_on_quorum_approval() { #[tokio::test] async fn approve_persistent_grant_creates_basic_grant_row() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -841,7 +851,7 @@ async fn approve_one_off_transaction_stores_result() { use chrono::Duration; let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -961,7 +971,7 @@ async fn approve_one_off_transaction_stores_result() { #[tokio::test] async fn replace_operator_updates_pubkey_and_starts_rekey() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -1033,7 +1043,7 @@ async fn replace_operator_updates_pubkey_and_starts_rekey() { #[tokio::test] async fn trigger_rekey_reaches_quorum() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -1075,7 +1085,7 @@ async fn trigger_rekey_reaches_quorum() { async fn key_rotation_requires_full_quorum() { // ยง3.3: ReplaceOperator needs all 3 operators to approve, not just shamir_threshold(3)=2 let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }) @@ -1129,7 +1139,7 @@ async fn key_rotation_requires_full_quorum() { #[tokio::test] async fn recovery_vote_rejected_when_sleeping() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors.vault.ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }).await.unwrap(); let op_key = authn::SigningKey::generate(); @@ -1175,7 +1185,7 @@ async fn recovery_vote_rejected_when_sleeping() { #[tokio::test] async fn recovery_vote_blocked_on_non_replace_proposal() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors.vault.ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }).await.unwrap(); let op_key = authn::SigningKey::generate(); @@ -1224,7 +1234,7 @@ async fn recovery_vote_blocked_on_non_replace_proposal() { #[tokio::test] async fn recovery_wakeup_can_be_cancelled() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors.vault.ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }).await.unwrap(); let key = authn::SigningKey::generate(); @@ -1253,7 +1263,7 @@ async fn recovery_wakeup_can_be_cancelled() { #[tokio::test] async fn recovery_wakeup_prevents_duplicate_request() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors.vault.ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }).await.unwrap(); let key = authn::SigningKey::generate(); @@ -1281,7 +1291,7 @@ async fn recovery_wakeup_prevents_duplicate_request() { async fn recovery_operator_vote_contributes_to_replace_quorum() { // 1 ordinary operator + 1 recovery operator; replace_operator needs both. let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors.vault.ask(Bootstrap { seal_key: KeyCell::from([0u8; 32]) }).await.unwrap(); let op_key = authn::SigningKey::generate(); diff --git a/server/crates/arbiter-server/tests/operator/auth.rs b/server/crates/arbiter-server/tests/operator/auth.rs index 5e12a9a..f5beb34 100644 --- a/server/crates/arbiter-server/tests/operator/auth.rs +++ b/server/crates/arbiter-server/tests/operator/auth.rs @@ -1,8 +1,11 @@ -use super::common::ChannelTransport; +use super::common::{ChannelTransport, bootstrapped_vault, eventually, spawn_actors}; use arbiter_crypto::authn::{self, AuthChallenge, SigningContext}; use arbiter_proto::transport::{Error as TransportError, Receiver, Sender}; use arbiter_server::{ - actors::{GlobalActors, bootstrap::GetToken, vault::Bootstrap}, + actors::{ + bootstrap::GetToken, + vault::{self, Bootstrap}, + }, crypto::integrity, db::{self, schema}, peers::operator::{self, Credentials, OperatorConnection, auth, vault_gate}, @@ -150,14 +153,7 @@ impl Sender for StartTestTransport { #[test_log::test] pub async fn bootstrap_token_auth() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); - actors - .vault - .ask(Bootstrap { - seal_key: arbiter_server::crypto::KeyCell::from([0u8; 32]), - }) - .await - .unwrap(); + let actors = spawn_actors(db.clone()).await; let token = actors.bootstrapper.ask(GetToken).await.unwrap().unwrap(); let (mut server_transport, mut test_transport) = ChannelTransport::new(); @@ -211,11 +207,131 @@ pub async fn bootstrap_token_auth() { assert_eq!(stored_pubkey, verifying_key(&new_key).encode().0.to_vec()); } +/// A multi-operator committee must all register with the same bootstrap token before bootstrap +/// completes, so verifying the token must not consume it. This is the reachability bug fixed by +/// replacing `consume_token` with `verify_token`. #[tokio::test] #[test_log::test] -pub async fn bootstrap_invalid_token_auth() { +pub async fn bootstrap_token_registers_every_committee_member() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; + let token = actors.bootstrapper.ask(GetToken).await.unwrap().unwrap(); + + for _ in 0..2 { + let (mut server_transport, mut test_transport) = ChannelTransport::new(); + let db_for_task = db.clone(); + let actors_for_task = actors.clone(); + let task = tokio::spawn(async move { + let mut props = OperatorConnection::new(db_for_task, actors_for_task); + auth::authenticate(&mut props, &mut server_transport).await + }); + + let new_key = MlDsa87::key_gen(&mut rand::rng()); + test_transport + .send(auth::Inbound::AuthChallengeRequest { + pubkey: verifying_key(&new_key).into(), + bootstrap_token: Some(token.clone()), + }) + .await + .unwrap(); + + let response = test_transport + .recv() + .await + .expect("should receive challenge"); + let challenge = match response { + Ok(auth::Outbound::AuthChallenge { challenge }) => challenge, + other => panic!("Expected AuthChallenge, got {other:?}"), + }; + + let signature = sign_operator_challenge(&new_key, &challenge); + test_transport + .send(auth::Inbound::AuthChallengeSolution { + signature: signature.to_bytes(), + }) + .await + .unwrap(); + + let response = test_transport + .recv() + .await + .expect("should receive auth result"); + assert!(matches!(response, Ok(auth::Outbound::AuthSuccess))); + + task.await.unwrap().unwrap(); + } + + let mut conn = db.get().await.unwrap(); + let registered: i64 = schema::operator_identity::table + .count() + .get_result(&mut conn) + .await + .unwrap(); + assert_eq!(registered, 2); + + // Bootstrap has not completed: the token must still be valid. + assert_eq!( + actors.bootstrapper.ask(GetToken).await.unwrap(), + Some(token) + ); +} + +/// `GlobalActors` must subscribe `Bootstrapper` to `events::Bootstrapped` on the message bus. +/// Without that one registration the token stays valid forever in production, which is the +/// defect this task exists to fix, and no other test notices: the test below drives the event +/// handler directly, and the `challenge_auth` family never re-reads the token after +/// bootstrapping. This one goes the whole way round -- real `Vault::bootstrap`, real bus -- +/// and waits for the effect rather than reading straight after the call, because `Publish` +/// only enqueues to the bus's mailbox. +#[tokio::test] +#[test_log::test] +pub async fn bootstrapped_event_retires_the_token_through_the_message_bus() { + let db = db::create_test_pool().await; + let actors = spawn_actors(db.clone()).await; + + assert!( + actors.bootstrapper.ask(GetToken).await.unwrap().is_some(), + "the token must exist before the vault is bootstrapped" + ); + + actors + .vault + .ask(Bootstrap { + seal_key: arbiter_server::crypto::KeyCell::from([0u8; 32]), + }) + .await + .unwrap(); + + eventually("the bootstrap token to be retired", || async { + actors + .bootstrapper + .ask(GetToken) + .await + .unwrap() + .is_none() + .then_some(()) + }) + .await; +} + +/// Once the vault reports `Bootstrapped`, the token is retired: further registrations must be +/// rejected even with a token that verified successfully moments earlier. +#[tokio::test] +#[test_log::test] +pub async fn bootstrap_token_rejected_after_bootstrapped_event() { + let db = db::create_test_pool().await; + let actors = spawn_actors(db.clone()).await; + let token = actors.bootstrapper.ask(GetToken).await.unwrap().unwrap(); + + // Drive the Bootstrapper's own event handler directly rather than through + // `actors.vault.ask(Bootstrap { .. })` + the message bus: bus delivery is fire-and-forget, + // so asserting on it would be racy. The handler under test is the same either way. + actors + .bootstrapper + .ask(vault::events::Bootstrapped) + .await + .unwrap(); + assert!(actors.bootstrapper.ask(GetToken).await.unwrap().is_none()); let (mut server_transport, mut test_transport) = ChannelTransport::new(); let db_for_task = db.clone(); @@ -228,7 +344,7 @@ pub async fn bootstrap_invalid_token_auth() { test_transport .send(auth::Inbound::AuthChallengeRequest { pubkey: verifying_key(&new_key).into(), - bootstrap_token: Some("invalid_token".to_owned()), + bootstrap_token: Some(token), }) .await .unwrap(); @@ -264,11 +380,150 @@ pub async fn bootstrap_invalid_token_auth() { assert_eq!(count, 0); } +/// `register_key`'s database gate must refuse a registration once `arbiter_settings.root_key_id` +/// is set, even when `Bootstrapper`'s own in-memory token has not yet been retired -- exactly +/// the two-mailbox-hop window between `Vault::bootstrap`'s commit and the `Bootstrapped` event +/// reaching `Bootstrapper` in production. "database bootstrapped, Bootstrapper not yet notified" +/// is reproduced deterministically by bootstrapping a throwaway `Vault` wired to its own message +/// bus: it commits `root_key_id` in the same database without ever publishing to the bus +/// `actors.bootstrapper` is registered on, so `actors.bootstrapper`'s token is left untouched. +#[tokio::test] +#[test_log::test] +pub async fn bootstrap_token_rejected_once_the_database_is_bootstrapped() { + let db = db::create_test_pool().await; + let actors = spawn_actors(db.clone()).await; + let token = actors.bootstrapper.ask(GetToken).await.unwrap().unwrap(); + + bootstrapped_vault(&db).await; + + // From Bootstrapper's point of view the token still verifies: it never received an event. + assert_eq!( + actors.bootstrapper.ask(GetToken).await.unwrap(), + Some(token.clone()) + ); + + let (mut server_transport, mut test_transport) = ChannelTransport::new(); + let db_for_task = db.clone(); + let task = tokio::spawn(async move { + let mut props = OperatorConnection::new(db_for_task, actors); + auth::authenticate(&mut props, &mut server_transport).await + }); + + let new_key = MlDsa87::key_gen(&mut rand::rng()); + test_transport + .send(auth::Inbound::AuthChallengeRequest { + pubkey: verifying_key(&new_key).into(), + bootstrap_token: Some(token), + }) + .await + .unwrap(); + + let response = test_transport + .recv() + .await + .expect("should receive challenge"); + let challenge = match response { + Ok(auth::Outbound::AuthChallenge { challenge }) => challenge, + other => panic!("Expected AuthChallenge, got {other:?}"), + }; + + let signature = sign_operator_challenge(&new_key, &challenge); + test_transport + .send(auth::Inbound::AuthChallengeSolution { + signature: signature.to_bytes(), + }) + .await + .unwrap(); + + // The refusal has to reach the peer, not just the task's return value: a registration + // refused after the database was bootstrapped must look like the refusal of a token that + // never verified, rather than a handshake that stops with nothing on the wire. + let refusal = test_transport + .recv() + .await + .expect("the refusal must be sent to the peer"); + assert!(matches!(refusal, Err(auth::Error::InvalidBootstrapToken))); + + assert!(matches!( + task.await.unwrap(), + Err(auth::Error::InvalidBootstrapToken) + )); + + let mut conn = db.get().await.unwrap(); + let count: i64 = schema::operator_identity::table + .count() + .get_result::(&mut conn) + .await + .unwrap(); + assert_eq!(count, 0); +} + +#[tokio::test] +#[test_log::test] +pub async fn bootstrap_invalid_token_auth() { + let db = db::create_test_pool().await; + let actors = spawn_actors(db.clone()).await; + + let (mut server_transport, mut test_transport) = ChannelTransport::new(); + let db_for_task = db.clone(); + let task = tokio::spawn(async move { + let mut props = OperatorConnection::new(db_for_task, actors); + auth::authenticate(&mut props, &mut server_transport).await + }); + + let new_key = MlDsa87::key_gen(&mut rand::rng()); + test_transport + .send(auth::Inbound::AuthChallengeRequest { + pubkey: verifying_key(&new_key).into(), + bootstrap_token: Some("invalid_token".to_owned()), + }) + .await + .unwrap(); + + let response = test_transport + .recv() + .await + .expect("should receive challenge"); + let challenge = match response { + Ok(auth::Outbound::AuthChallenge { challenge }) => challenge, + other => panic!("Expected AuthChallenge, got {other:?}"), + }; + + let signature = sign_operator_challenge(&new_key, &challenge); + test_transport + .send(auth::Inbound::AuthChallengeSolution { + signature: signature.to_bytes(), + }) + .await + .unwrap(); + + // The reference behaviour the refusal above has to match: a token that never verified is + // reported to the peer. Pinned here so the two refusal paths cannot drift apart again. + let refusal = test_transport + .recv() + .await + .expect("the refusal must be sent to the peer"); + assert!(matches!(refusal, Err(auth::Error::InvalidBootstrapToken))); + + assert!(matches!( + task.await.unwrap(), + Err(auth::Error::InvalidBootstrapToken) + )); + + let mut conn = db.get().await.unwrap(); + let count: i64 = schema::operator_identity::table + .count() + .get_result::(&mut conn) + .await + .unwrap(); + assert_eq!(count, 0); +} + #[tokio::test] #[test_log::test] pub async fn challenge_auth() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { @@ -353,7 +608,7 @@ pub async fn challenge_auth() { #[test_log::test] pub async fn challenge_auth_rejects_integrity_tag_mismatch_when_unsealed() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault @@ -427,7 +682,7 @@ pub async fn challenge_auth_rejects_integrity_tag_mismatch_when_unsealed() { #[test_log::test] pub async fn challenge_auth_rejects_invalid_signature() { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault .ask(Bootstrap { diff --git a/server/crates/arbiter-server/tests/operator/unseal.rs b/server/crates/arbiter-server/tests/operator/unseal.rs index 6acfdba..38f931d 100644 --- a/server/crates/arbiter-server/tests/operator/unseal.rs +++ b/server/crates/arbiter-server/tests/operator/unseal.rs @@ -1,9 +1,7 @@ +use super::common::spawn_actors; use arbiter_crypto::authn; use arbiter_server::{ - actors::{ - GlobalActors, - vault::{Bootstrap, Seal}, - }, + actors::vault::{Bootstrap, Seal}, db, peers::operator::{ Credentials, @@ -26,7 +24,7 @@ async fn setup_sealed_gate( oneshot::Receiver>, ) { let db = db::create_test_pool().await; - let actors = GlobalActors::spawn(db.clone()).await.unwrap(); + let actors = spawn_actors(db.clone()).await; actors .vault