fix(bootstrap): keep the token valid until the vault is bootstrapped

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<String, std::io::Error> {
-    let rng: StdRng = make_rng();
+pub async fn generate_token(home: &Path) -> Result<String, std::io::Error> {
+    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<Self, Error> {
-        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<Self, Error> {
+        let mut conn = db.get().await?;
+
+        let bootstrapped: bool = schema::arbiter_settings::table
+            .select(schema::arbiter_settings::root_key_id)
+            .first::<Option<i32>>(&mut conn)
+            .await?
+            .is_some();
+
+        if bootstrapped {
+            return Ok(Self { token: None });
+        }
+
+        let any_operator_registered: bool = schema::operator_identity::table
+            .count()
+            .get_result::<i64>(&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<crate::actors::vault::events::Bootstrapped> for Bootstrapper {
+    type Reply = ();
+
+    async fn handle(
+        &mut self,
+        _msg: crate::actors::vault::events::Bootstrapped,
+        _ctx: &mut kameo::prelude::Context<Self, Self::Reply>,
+    ) -> 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)));
+    }
+}

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<Self, SpawnError> {
+        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<Self, SpawnError> {
+        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<Self, SpawnError> {
         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::<ProposalApproved>(),
@@ -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::<events::Bootstrapped>(),
+            ))
+            .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,

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(())
     }

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.

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<DatabasePool, DatabaseSetupError> {
-    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)?;
 

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<i3
         Error::internal("Database unavailable")
     })?;
 
-    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
-        .map_err(|e| {
-            error!(error = ?e, "Database error");
-            Error::internal("Database operation failed")
-        })?;
+    conn.transaction(async move |conn| {
+        // The database is authoritative on whether bootstrap has completed: `Vault::bootstrap`
+        // commits `root_key_id` before it publishes `events::Bootstrapped`, and `Bootstrapper`
+        // only learns of that two mailbox hops later. Re-checking it here, in the same
+        // transaction as the insert, closes that window deterministically instead of trusting
+        // a token that verified against `Bootstrapper`'s possibly-stale in-memory state.
+        let already_bootstrapped: bool = arbiter_settings::table
+            .select(arbiter_settings::root_key_id)
+            .first::<Option<i32>>(&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?