fix(recovery): Delete the known secrets from 4s when disabling recovery

poljar · poljar · commit c444bc9b9907 · 2025-02-11T14:44:20.000+01:00
diff --git a/crates/matrix-sdk/CHANGELOG.md b/crates/matrix-sdk/CHANGELOG.md
@@ -12,6 +12,14 @@ All notable changes to this project will be documented in this file.
   `cleanup_frequency` setting.
   ([#4603](https://github.com/matrix-org/matrix-rust-sdk/pull/4603))
 
+### Bug Fixes
+
+- Ensure all known secrets are removed from secret storage when invoking the
+  `Recovery::disable()` method. While the server is not guaranteed to delete
+  these secrets, making an attempt to remove them is considered good practice.
+  Note that all secrets are uploaded to the server in an encrypted form.
+  ([#4629](https://github.com/matrix-org/matrix-rust-sdk/pull/4629))
+
 ## [0.10.0] - 2025-02-04
 
 ### Features
diff --git a/crates/matrix-sdk/src/encryption/recovery/mod.rs b/crates/matrix-sdk/src/encryption/recovery/mod.rs
@@ -95,10 +95,13 @@ use futures_util::StreamExt as _;
 use ruma::{
     api::client::keys::get_keys,
     events::{
-        secret::send::ToDeviceSecretSendEvent,
-        secret_storage::default_key::SecretStorageDefaultKeyEvent,
+        secret::{request::SecretName, send::ToDeviceSecretSendEvent},
+        secret_storage::{default_key::SecretStorageDefaultKeyEvent, secret::SecretEventContent},
+        GlobalAccountDataEventType,
     },
+    serde::Raw,
 };
+use serde_json::{json, value::to_raw_value};
 use tracing::{error, info, instrument, warn};
 
 #[cfg(doc)]
@@ -124,6 +127,15 @@ pub struct Recovery {
 }
 
 impl Recovery {
+    /// The list of known secrets that are contained in secret storage once
+    /// recover is enabled.
+    pub const KNOWN_SECRETS: &[SecretName] = &[
+        SecretName::CrossSigningMasterKey,
+        SecretName::CrossSigningUserSigningKey,
+        SecretName::CrossSigningSelfSigningKey,
+        SecretName::RecoveryKey,
+    ];
+
     /// Get the current [`RecoveryState`] for this [`Client`].
     pub fn state(&self) -> RecoveryState {
         self.client.inner.e2ee.recovery_state.get()
@@ -285,11 +297,36 @@ impl Recovery {
     #[instrument(skip_all)]
     pub async fn disable(&self) -> Result<()> {
         self.client.encryption().backups().disable().await?;
+
         // Why oh why, can't we delete account data events?
+        //
+        // Alright, let's attempt to "delete" the content of our current default key,
+        // for this we first need to check if there is a default key, then
+        // deserialize the content and find out the key ID.
+        //
+        // Then we finally set the event to an empty JSON content.
+        if let Ok(Some(default_event)) =
+            self.client.encryption().secret_storage().fetch_default_key_id().await
+        {
+            if let Ok(default_event) = default_event.deserialize() {
+                let key_id = default_event.key_id;
+                let event_type = GlobalAccountDataEventType::SecretStorageKey(key_id);
+
+                self.client
+                    .account()
+                    .set_account_data_raw(event_type, Raw::new(&json!({})).expect("").cast())
+                    .await?;
+            }
+        }
+
+        // Now let's "delete" the actual `m.secret.storage.default_key` event.
         self.client.account().set_account_data(SecretStorageDisabledContent {}).await?;
+        // Make sure that we don't re-enable backups automatically.
         self.client.account().set_account_data(BackupDisabledContent { disabled: true }).await?;
+        // Finally, "delete" all the known secrets we have in the account data.
+        self.delete_all_known_secrets().await?;
+
         self.update_recovery_state().await?;
-        // TODO: Do we want to "delete" the known secrets as well?
 
         Ok(())
     }
@@ -527,6 +564,28 @@ impl Recovery {
         Ok(())
     }
 
+    /// Delete all the known secrets we are keeping in secret storage.
+    ///
+    /// The exact list of secrets is defined in [`Recovery::KNOWN_SECRETS`] and
+    /// might change over time.
+    ///
+    /// Since account data events can't actually be deleted, due to a missing
+    /// DELETE API, we're replacing the events with an empty
+    /// [`SecretEventContent`].
+    async fn delete_all_known_secrets(&self) -> Result<()> {
+        for secret_name in Self::KNOWN_SECRETS {
+            let event_type = GlobalAccountDataEventType::from(secret_name.to_owned());
+            let content = SecretEventContent::new(Default::default());
+            let secret_content = Raw::from_json(
+                to_raw_value(&content)
+                    .expect("We should be able to serialize a raw empty secret event content"),
+            );
+            self.client.account().set_account_data_raw(event_type, secret_content).await?;
+        }
+
+        Ok(())
+    }
+
     /// Run a network request to figure whether backups have been disabled at
     /// the account level.
     async fn are_backups_marked_as_disabled(&self) -> Result<bool> {
diff --git a/crates/matrix-sdk/src/encryption/secret_storage/mod.rs b/crates/matrix-sdk/src/encryption/secret_storage/mod.rs
@@ -66,11 +66,14 @@ use matrix_sdk_base::crypto::{
     secret_storage::{DecodeError, MacError, SecretStorageKey},
     CryptoStoreError, SecretImportError,
 };
-use ruma::events::{
-    secret_storage::{
-        default_key::SecretStorageDefaultKeyEventContent, key::SecretStorageKeyEventContent,
+use ruma::{
+    events::{
+        secret_storage::{
+            default_key::SecretStorageDefaultKeyEventContent, key::SecretStorageKeyEventContent,
+        },
+        EventContentFromType, GlobalAccountDataEventType,
     },
-    EventContentFromType, GlobalAccountDataEventType,
+    serde::Raw,
 };
 use serde_json::value::to_raw_value;
 use thiserror::Error;
@@ -186,11 +189,7 @@ impl SecretStorage {
     /// # anyhow::Ok(()) };
     /// ```
     pub async fn open_secret_store(&self, secret_storage_key: &str) -> Result<SecretStore> {
-        let maybe_default_key_id = self
-            .client
-            .account()
-            .fetch_account_data(GlobalAccountDataEventType::SecretStorageDefaultKey)
-            .await?;
+        let maybe_default_key_id = self.fetch_default_key_id().await?;
 
         if let Some(default_key_id) = maybe_default_key_id {
             let default_key_id =
@@ -271,12 +270,7 @@ impl SecretStorage {
 
     /// Run a network request to find if secret storage is set up for this user.
     pub async fn is_enabled(&self) -> crate::Result<bool> {
-        if let Some(content) = self
-            .client
-            .account()
-            .fetch_account_data(GlobalAccountDataEventType::SecretStorageDefaultKey)
-            .await?
-        {
+        if let Some(content) = self.fetch_default_key_id().await? {
             // Since we can't delete account data events, we're going to treat
             // deserialization failures as secret storage being disabled.
             Ok(content.deserialize_as::<SecretStorageDefaultKeyEventContent>().is_ok())
@@ -285,4 +279,17 @@ impl SecretStorage {
             Ok(false)
         }
     }
+
+    /// Fetch the `m.secret_storage.default_key` event from the server.
+    pub async fn fetch_default_key_id(
+        &self,
+    ) -> crate::Result<Option<Raw<SecretStorageDefaultKeyEventContent>>> {
+        let maybe_default_key_id = self
+            .client
+            .account()
+            .fetch_account_data(GlobalAccountDataEventType::SecretStorageDefaultKey)
+            .await?;
+
+        Ok(maybe_default_key_id.map(|event| event.cast()))
+    }
 }
diff --git a/crates/matrix-sdk/src/test_utils/mod.rs b/crates/matrix-sdk/src/test_utils/mod.rs
@@ -247,17 +247,17 @@ macro_rules! assert_next_eq_with_timeout_impl {
 
         assert_eq!(next_value, $expected);
     };
-    ($stream:expr, $expected:expr, $timeout:expr, $($msg:tt)*) => {
+    ($stream:expr, $expected:expr, $timeout:expr, $($msg:tt)*) => {{
         let next_value = tokio::time::timeout(
             $timeout,
-            $stream.next()
+            futures_util::StreamExt::next(&mut $stream)
         )
         .await
         .expect("We should be able to get the next value out of the stream by now")
         .expect("The stream should have given us a new value instead of None");
 
         assert_eq!(next_value, $expected, $($msg)*);
-    };
+    }};
 }
 
 /// Like `assert_let`, but with the possibility to add an optional timeout.
diff --git a/crates/matrix-sdk/tests/integration/encryption/recovery.rs b/crates/matrix-sdk/tests/integration/encryption/recovery.rs
@@ -643,11 +643,19 @@ async fn test_recovery_disabling() {
                 ResponseTemplate::new(404)
             }
         })
-        .expect(2)
+        .expect(3)
         .named("m.secret_storage.default_key account data GET")
         .mount(&server)
         .await;
 
+    Mock::given(method("PUT"))
+        .and(path(format!("_matrix/client/r0/user/{user_id}/account_data/m.megolm_backup.v1",)))
+        .and(header("authorization", "Bearer 1234"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(json!({})))
+        .expect(1..)
+        .mount(&server)
+        .await;
+
     recovery.disable().await.expect("We should be able to disable recovery again.");
     assert_eq!(client.encryption().backups().state(), BackupState::Unknown);
     assert_eq!(recovery.state(), RecoveryState::Disabled);
diff --git a/testing/matrix-sdk-integration-testing/src/tests/e2ee.rs b/testing/matrix-sdk-integration-testing/src/tests/e2ee.rs
@@ -5,10 +5,11 @@ use assert_matches::assert_matches;
 use assert_matches2::assert_let;
 use assign::assign;
 use matrix_sdk::{
+    assert_next_eq_with_timeout,
     crypto::{format_emojis, SasState},
     encryption::{
         backups::BackupState,
-        recovery::RecoveryState,
+        recovery::{Recovery, RecoveryState},
         verification::{
             QrVerificationData, QrVerificationState, Verification, VerificationRequestState,
         },
@@ -22,13 +23,15 @@ use matrix_sdk::{
                 MessageType, OriginalSyncRoomMessageEvent, RoomMessageEventContent,
                 SyncRoomMessageEvent,
             },
-            OriginalSyncMessageLikeEvent,
+            secret_storage::{key::SecretStorageKeyEventContent, secret::SecretEventContent},
+            EventContentFromType, GlobalAccountDataEventType, OriginalSyncMessageLikeEvent,
         },
     },
     Client,
 };
+use serde_json::value::to_raw_value;
 use similar_asserts::assert_eq;
-use tracing::warn;
+use tracing::{debug, warn};
 
 use crate::helpers::{SyncTokenAwareClient, TestClientBuilder};
 
@@ -974,3 +977,107 @@ async fn test_secret_gossip_after_interactive_verification() -> Result<()> {
 
     Ok(())
 }
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
+async fn test_recovery_disabling_deletes_secret_storage_secrets() -> Result<()> {
+    let encryption_settings = EncryptionSettings {
+        auto_enable_cross_signing: true,
+        auto_enable_backups: true,
+        ..Default::default()
+    };
+    let client = SyncTokenAwareClient::new(
+        TestClientBuilder::new("alice_recovery_deletion_test")
+            .encryption_settings(encryption_settings)
+            .build()
+            .await?,
+    );
+
+    debug!("Enabling recovery");
+
+    client.encryption().wait_for_e2ee_initialization_tasks().await;
+    client.encryption().recovery().enable().await?;
+
+    // Let's wait for recovery to become enabled.
+    let mut recovery_state = client.encryption().recovery().state_stream();
+
+    debug!("Checking that recovery has been enabled");
+
+    assert_next_eq_with_timeout!(
+        recovery_state,
+        RecoveryState::Enabled,
+        "Recovery should have been enabled"
+    );
+
+    debug!("Checking that the secrets have been stored on the server");
+
+    let default_event = client
+        .encryption()
+        .secret_storage()
+        .fetch_default_key_id()
+        .await?
+        .expect("The default key event should have been uploaded to the server")
+        .deserialize()
+        .expect("We should be able to deserialize the default event content");
+
+    let key_id = default_event.key_id;
+
+    for event_type in Recovery::KNOWN_SECRETS {
+        let event_type = GlobalAccountDataEventType::from(event_type.clone());
+        let event = client
+            .account()
+            .fetch_account_data(event_type.clone())
+            .await?
+            .expect("The secret event should still exist");
+
+        let event = event
+            .deserialize_as::<SecretEventContent>()
+            .expect("We should be able to deserialize the content of known secrets");
+
+        assert!(
+            !event.encrypted.is_empty(),
+            "The known secret {event_type} should exist on the server"
+        );
+    }
+
+    debug!("Disabling recovery");
+
+    client.encryption().recovery().disable().await?;
+
+    debug!("Checking that the secrets have been removed from the server");
+
+    for event_type in Recovery::KNOWN_SECRETS {
+        let event_type = GlobalAccountDataEventType::from(event_type.clone());
+        let event = client
+            .account()
+            .fetch_account_data(event_type.clone())
+            .await?
+            .expect("The secret event should still exist");
+
+        let event = event
+            .deserialize_as::<SecretEventContent>()
+            .expect("We should be able to deserialize the content since that's what we uploaded");
+
+        assert!(
+            event.encrypted.is_empty(),
+            "The known secret {event_type} should have been deleted from the server"
+        );
+    }
+
+    debug!("Checking that the info about the default key has been deleted");
+    let event_type = GlobalAccountDataEventType::SecretStorageKey(key_id);
+    let content = client
+        .account()
+        .fetch_account_data(event_type.clone())
+        .await?
+        .expect("The key event should be available on the server");
+
+    SecretStorageKeyEventContent::from_parts(
+        &event_type.to_string(),
+        &to_raw_value(&content).unwrap(),
+    )
+    .expect_err(
+        "The key event should have been “deleted” so we shouldn't be able to deserialize it",
+    );
+
+    Ok(())
+}
