hive-matrix-mcp: never enable recovery over secret storage it did not create

`recovery().enable()` refuses only when a key backup exists. It then makes
a new secret-storage key and sets it as the account's default regardless
of the default already there, so an account with secret storage but no
backup (set up in another client, backup since deleted) would have its
default key replaced and its other clients' recovery key locked out.

`recover_or_enable` now asks `secret_storage().is_enabled()` before
`enable` and returns an error instead, which `ensure_recovery` logs at
WARN with the account and the passphrase's store path. The decision is
behind a small trait over the matrix-sdk calls so a fake can pin it: with
no backup and secret storage present, neither `enable` nor `recover` runs.

Review: argus on #5188.

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

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<bool>;
+    /// Whether the account has a default secret-storage key.
+    async fn secret_storage_on_server(&self) -> Result<bool>;
+    /// 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<bool> {
+        Ok(self.encryption().backups().fetch_exists_on_server().await?)
+    }
+
+    async fn secret_storage_on_server(&self) -> Result<bool> {
+        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<Backup> {
-    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<Backup> {
     // 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<Vec<&'static str>>,
+    }
+
+    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<bool> {
+            Ok(self.backup)
+        }
+
+        async fn secret_storage_on_server(&self) -> Result<bool> {
+            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());
+    }
+}