diff --git a/docs/integrations/matrix.md b/docs/integrations/matrix.md index b804dbc6..bd363f73 100644 --- a/docs/integrations/matrix.md +++ b/docs/integrations/matrix.md @@ -318,8 +318,9 @@ Initial rollout settings: store, and never replaces it. When `hive-matrix-daemon` brings an account up it reads that passphrase: with no backup on the account's homeserver it turns on server-side key backup and secret storage - (log line `matrix recovery enabled`); with one there it recovers from - it (`recovered from the server-side key backup`), which downloads the + (log line `matrix recovery enabled`), unless the account already has + secret storage, which it never replaces (a `WARN` instead). With a + backup there it recovers from it (`recovered from the server-side key backup`), which downloads the room keys and signs the new device with the account's existing cross-signing identity. A crypto store lost to `--purge` or to a primary-token reset therefore comes back with its history. With no passphrase diff --git a/hive-matrix-mcp/src/client.rs b/hive-matrix-mcp/src/client.rs index 629f9904..8b61e7ec 100644 --- a/hive-matrix-mcp/src/client.rs +++ b/hive-matrix-mcp/src/client.rs @@ -29,7 +29,7 @@ use std::path::Path; -use anyhow::{Context, Result, anyhow}; +use anyhow::{Context, Result, anyhow, bail}; use matrix_sdk::{ Client, SessionMeta, SessionTokens, authentication::matrix::MatrixSession, @@ -275,10 +275,10 @@ enum Backup { /// /// Every failure is logged and swallowed, as in [`ensure_cross_signing`]: the /// account comes up as it would have without backup. That includes secret -/// storage created with a different key, which this does not replace — the -/// backup under it would be lost. +/// storage this passphrase does not open, which is never replaced: with a +/// backup, `recover` fails; without one, this refuses. pub async fn ensure_recovery(client: &Client, account: &str, passphrase: &Passphrase) { - match recover_or_enable(client, passphrase).await { + match recover_or_enable(client, passphrase.expose()).await { Ok(Backup::AlreadyOn) => { tracing::debug!(account, "matrix key backup already on for this device"); } @@ -301,38 +301,96 @@ pub async fn ensure_recovery(client: &Client, account: &str, passphrase: &Passph } } +/// The homeserver and device calls [`recover_or_enable`] decides from, so a +/// test can stand in for matrix-sdk. +trait KeyBackup { + /// Wait for the e2ee tasks a restored client starts in the background. + async fn settle(&self); + /// Whether this device is already connected to the account's backup. + async fn on_this_device(&self) -> bool; + /// Whether the homeserver holds a key backup version. + async fn backup_on_server(&self) -> Result; + /// Whether the account has a default secret-storage key. + async fn secret_storage_on_server(&self) -> Result; + /// Open secret storage with `passphrase` and import what it holds. + async fn recover(&self, passphrase: &str) -> Result<()>; + /// Create a backup and new secret storage that `passphrase` opens. + async fn enable(&self, passphrase: &str) -> Result<()>; +} + +impl KeyBackup for Client { + async fn settle(&self) { + self.encryption().wait_for_e2ee_initialization_tasks().await; + } + + async fn on_this_device(&self) -> bool { + self.encryption().backups().are_enabled().await + } + + async fn backup_on_server(&self) -> Result { + Ok(self.encryption().backups().fetch_exists_on_server().await?) + } + + async fn secret_storage_on_server(&self) -> Result { + Ok(self.encryption().secret_storage().is_enabled().await?) + } + + async fn recover(&self, passphrase: &str) -> Result<()> { + Ok(self.encryption().recovery().recover(passphrase).await?) + } + + async fn enable(&self, passphrase: &str) -> Result<()> { + // The recovery key `enable` returns is not kept: the passphrase opens + // the same secret storage. + self.encryption() + .recovery() + .enable() + .with_passphrase(passphrase) + .await?; + Ok(()) + } +} + /// The body of [`ensure_recovery`], with its outcome returned rather than /// logged. -async fn recover_or_enable(client: &Client, passphrase: &Passphrase) -> Result { - let encryption = client.encryption(); +/// +/// ⚠️ `enable` makes a new secret-storage key and sets it as the account's +/// default, whatever default was there. With no backup but secret storage +/// present, this refuses rather than enable: that default key is someone +/// else's, and replacing it locks their recovery key out. +async fn recover_or_enable(account: &impl KeyBackup, passphrase: &str) -> Result { // A device restored from its own store reconnects to the backup in a - // background task; reading `are_enabled` before it finishes says no. - encryption.wait_for_e2ee_initialization_tasks().await; - let backups = encryption.backups(); - if backups.are_enabled().await { + // background task; asking before it finishes says no. + account.settle().await; + if account.on_this_device().await { return Ok(Backup::AlreadyOn); } - let recovery = encryption.recovery(); - if backups - .fetch_exists_on_server() + if account + .backup_on_server() .await .context("asking the homeserver for the account's key backup")? { - recovery - .recover(passphrase.expose()) + account + .recover(passphrase) .await .context("recovering from the key backup")?; - Ok(Backup::Restored) - } else { - // The recovery key `enable` returns is not kept: the passphrase opens - // the same secret storage. - recovery - .enable() - .with_passphrase(passphrase.expose()) - .await - .context("enabling key backup")?; - Ok(Backup::Enabled) + return Ok(Backup::Restored); } + if account + .secret_storage_on_server() + .await + .context("asking the homeserver for the account's secret storage")? + { + bail!( + "the account has secret storage but no key backup; not enabling, \ + which would replace a default secret-storage key this daemon did not create" + ); + } + account + .enable(passphrase) + .await + .context("enabling key backup")?; + Ok(Backup::Enabled) } /// Best-effort: set this account's matrix avatar from the rasterized icon @@ -394,3 +452,103 @@ pub async fn sync_avatar(client: &Client, state_dir: &Path, account: &str) { } } } + +#[cfg(test)] +mod tests { + use std::sync::Mutex; + + use super::*; + + /// An account as the homeserver and this device describe it, recording + /// which of the two writes [`recover_or_enable`] made. + #[derive(Default)] + struct Fake { + on_device: bool, + backup: bool, + secret_storage: bool, + calls: Mutex>, + } + + impl KeyBackup for Fake { + async fn settle(&self) {} + + async fn on_this_device(&self) -> bool { + self.on_device + } + + async fn backup_on_server(&self) -> Result { + Ok(self.backup) + } + + async fn secret_storage_on_server(&self) -> Result { + Ok(self.secret_storage) + } + + async fn recover(&self, _: &str) -> Result<()> { + self.calls.lock().unwrap().push("recover"); + Ok(()) + } + + async fn enable(&self, _: &str) -> Result<()> { + self.calls.lock().unwrap().push("enable"); + Ok(()) + } + } + + fn calls(fake: &Fake) -> Vec<&'static str> { + fake.calls.lock().unwrap().clone() + } + + #[tokio::test] + async fn secret_storage_without_a_backup_is_not_enabled_over() { + // `enable` would replace the account's default secret-storage key, + // which another client's recovery key opens. + let fake = Fake { + secret_storage: true, + ..Fake::default() + }; + assert!(recover_or_enable(&fake, "p").await.is_err()); + assert!(calls(&fake).is_empty(), "{:?}", calls(&fake)); + } + + #[tokio::test] + async fn an_account_with_nothing_on_the_server_is_enabled() { + // Control for the test above: the guard is the only thing stopping + // `enable` there. + let fake = Fake::default(); + assert!(matches!( + recover_or_enable(&fake, "p").await.unwrap(), + Backup::Enabled + )); + assert_eq!(calls(&fake), ["enable"]); + } + + #[tokio::test] + async fn a_backup_on_the_server_is_recovered_not_enabled() { + let fake = Fake { + backup: true, + secret_storage: true, + ..Fake::default() + }; + assert!(matches!( + recover_or_enable(&fake, "p").await.unwrap(), + Backup::Restored + )); + assert_eq!(calls(&fake), ["recover"]); + } + + #[tokio::test] + async fn a_device_already_on_the_backup_writes_nothing() { + let fake = Fake { + on_device: true, + backup: true, + secret_storage: true, + ..Fake::default() + }; + assert!(matches!( + recover_or_enable(&fake, "p").await.unwrap(), + Backup::AlreadyOn + )); + assert!(calls(&fake).is_empty()); + } +}