Merge pull request #3448 from matrix-org/rav/fix_backup_import

Crypto: fix backed-up keys being re-backed-up

Author: richvdh

bindings/matrix-sdk-crypto-ffi/src/machine.rs

@@ -991,7 +991,7 @@ impl OlmMachine {
     ) -> Result<KeysImportResult, KeyImportError> {
         let keys = Cursor::new(keys);
         let keys = decrypt_room_key_export(keys, &passphrase)?;
-        self.import_room_keys_helper(keys, false, progress_listener)
+        self.import_room_keys_helper(keys, None, progress_listener)
     }
 
     /// Import room keys from the given serialized unencrypted key export.
@@ -1001,6 +1001,9 @@ impl OlmMachine {
     /// should be used if the room keys are coming from the server-side backup,
     /// the method will mark all imported room keys as backed up.
     ///
+    /// **Note**: This has been deprecated. Use
+    /// [`OlmMachine::import_room_keys_from_backup`] instead.
+    ///
     /// # Arguments
     ///
     /// * `keys` - The serialized version of the unencrypted key export.
@@ -1012,11 +1015,38 @@ impl OlmMachine {
         keys: String,
         progress_listener: Box<dyn ProgressListener>,
     ) -> Result<KeysImportResult, KeyImportError> {
+        // Assume that the keys came from the current backup version.
+        let backup_version = self.runtime.block_on(self.inner.backup_machine().backup_version());
         let keys: Vec<Value> = serde_json::from_str(&keys)?;
-
         let keys = keys.into_iter().map(serde_json::from_value).filter_map(|k| k.ok()).collect();
+        self.import_room_keys_helper(keys, backup_version.as_deref(), progress_listener)
+    }
 
-        self.import_room_keys_helper(keys, true, progress_listener)
+    /// Import room keys from the given serialized unencrypted key export.
+    ///
+    /// This method is the same as [`OlmMachine::import_room_keys`] but the
+    /// decryption step is skipped and should be performed by the caller. This
+    /// should be used if the room keys are coming from the server-side backup.
+    /// The method will mark all imported room keys as backed up.
+    ///
+    /// # Arguments
+    ///
+    /// * `keys` - The serialized version of the unencrypted key export.
+    ///
+    /// * `backup_version` - The version of the backup that these keys came
+    /// from.
+    ///
+    /// * `progress_listener` - A callback that can be used to introspect the
+    /// progress of the key import.
+    pub fn import_room_keys_from_backup(
+        &self,
+        keys: String,
+        backup_version: String,
+        progress_listener: Box<dyn ProgressListener>,
+    ) -> Result<KeysImportResult, KeyImportError> {
+        let keys: Vec<Value> = serde_json::from_str(&keys)?;
+        let keys = keys.into_iter().map(serde_json::from_value).filter_map(|k| k.ok()).collect();
+        self.import_room_keys_helper(keys, Some(&backup_version), progress_listener)
     }
 
     /// Discard the currently active room key for the given room if there is
@@ -1506,16 +1536,18 @@ impl OlmMachine {
     fn import_room_keys_helper(
         &self,
         keys: Vec<ExportedRoomKey>,
-        from_backup: bool,
+        from_backup_version: Option<&str>,
         progress_listener: Box<dyn ProgressListener>,
     ) -> Result<KeysImportResult, KeyImportError> {
         let listener = |progress: usize, total: usize| {
             progress_listener.on_progress(progress as i32, total as i32)
         };
 
-        #[allow(deprecated)]
-        let result =
-            self.runtime.block_on(self.inner.import_room_keys(keys, from_backup, listener))?;
+        let result = self.runtime.block_on(self.inner.store().import_room_keys(
+            keys,
+            from_backup_version,
+            listener,
+        ))?;
 
         Ok(KeysImportResult {
             imported: result.imported_count as i64,

crates/matrix-sdk-crypto/CHANGELOG.md

@@ -31,8 +31,19 @@ Breaking changes:
 - Add new `dehydrated` property to `olm::account::PickledAccount`.
   ([#3164](https://github.com/matrix-org/matrix-rust-sdk/pull/3164))
 
+- Remove deprecated `OlmMachine::import_room_keys`.
+  ([#3448](https://github.com/matrix-org/matrix-rust-sdk/pull/3448))
+
+Deprecations:
+
+- Deprecate `BackupMachine::import_backed_up_room_keys`.
+  ([#3448](https://github.com/matrix-org/matrix-rust-sdk/pull/3448))
+
 Additions:
 
+- Expose new method `CryptoStore::import_room_keys`.
+  ([#3448](https://github.com/matrix-org/matrix-rust-sdk/pull/3448))
+
 - Expose new method `BackupMachine::backup_version`.
   ([#3320](https://github.com/matrix-org/matrix-rust-sdk/pull/3320))
 

crates/matrix-sdk-crypto/src/backups/mod.rs

@@ -600,6 +600,7 @@ impl BackupMachine {
     ///
     /// Returns a [`RoomKeyImportResult`] containing information about room keys
     /// which were imported.
+    #[deprecated(note = "Use the OlmMachine::store::import_room_keys method instead")]
     pub async fn import_backed_up_room_keys(
         &self,
         room_keys: BTreeMap<OwnedRoomId, BTreeMap<String, BackedUpRoomKey>>,
@@ -619,7 +620,15 @@ impl BackupMachine {
             }
         }
 
-        self.store.import_room_keys(decrypted_room_keys, true, progress_listener).await
+        // FIXME: This method is a bit flawed: we have no real idea which backup version
+        //   these keys came from. For example, we might have reset the backup
+        //   since the keys were downloaded. For now, let's assume they came from
+        //   the "current" backup version.
+        let backup_version = self.backup_version().await;
+
+        self.store
+            .import_room_keys(decrypted_room_keys, backup_version.as_deref(), progress_listener)
+            .await
     }
 }
 
@@ -826,6 +835,12 @@ mod tests {
         let machine = OlmMachine::new(alice_id(), alice_device_id()).await;
         let backup_machine = machine.backup_machine();
 
+        // We set up a backup key, so that we can test `backup_machine.backup()` later.
+        let decryption_key = BackupDecryptionKey::new().expect("Couldn't create new recovery key");
+        let backup_key = decryption_key.megolm_v1_public_key();
+        backup_key.set_version("1".to_owned());
+        backup_machine.enable_backup_v1(backup_key).await.expect("Couldn't enable backup");
+
         let room_id = room_id!("!DovneieKSTkdHKpIXy:morpheus.localhost");
         let session_id = "gM8i47Xhu0q52xLfgUXzanCMpLinoyVyH7R58cBuVBU";
         let room_key = room_key();
@@ -839,19 +854,29 @@ mod tests {
 
         assert!(session.is_none(), "Initially we should not have the session in the store");
 
+        #[allow(deprecated)]
         backup_machine
             .import_backed_up_room_keys(room_keys, |_, _| {})
             .await
             .expect("We should be able to import a room key");
 
+        // Now check that the session was correctly imported, and that it is marked as
+        // backed up
         let session = machine.store().get_inbound_group_session(room_id, session_id).await.unwrap();
-
         assert_let!(Some(session) = session);
         assert!(
             session.backed_up(),
             "If a session was imported from a backup, it should be considered to be backed up"
         );
         assert!(session.has_been_imported());
+
+        // Also check that it is not returned by a backup request.
+        let backup_request =
+            backup_machine.backup().await.expect("We should be able to create a backup request");
+        assert!(
+            backup_request.is_none(),
+            "If a session was imported from backup, it should not be backed up again."
+        );
     }
 
     #[async_test]

crates/matrix-sdk-crypto/src/file_encryption/key_export.rs

@@ -74,7 +74,7 @@ pub enum KeyExportError {
 /// # let machine = OlmMachine::new(&alice, device_id!("DEVICEID")).await;
 /// # let export = Cursor::new("".to_owned());
 /// let exported_keys = decrypt_room_key_export(export, "1234").unwrap();
-/// machine.import_room_keys(exported_keys, false, |_, _| {}).await.unwrap();
+/// machine.store().import_room_keys(exported_keys, None, |_, _| {}).await.unwrap();
 /// # };
 /// ```
 pub fn decrypt_room_key_export(

crates/matrix-sdk-crypto/src/machine.rs

@@ -62,9 +62,8 @@ use crate::{
     gossiping::GossipMachine,
     identities::{user::UserIdentities, Device, IdentityManager, UserDevices},
     olm::{
-        Account, CrossSigningStatus, EncryptionSettings, ExportedRoomKey, IdentityKeys,
-        InboundGroupSession, OlmDecryptionInfo, PrivateCrossSigningIdentity, SessionType,
-        StaticAccountData,
+        Account, CrossSigningStatus, EncryptionSettings, IdentityKeys, InboundGroupSession,
+        OlmDecryptionInfo, PrivateCrossSigningIdentity, SessionType, StaticAccountData,
     },
     requests::{IncomingResponse, OutgoingRequest, UploadSigningKeysRequest},
     session_manager::{GroupSessionManager, SessionManager},
@@ -91,7 +90,7 @@ use crate::{
     utilities::timestamp_to_iso8601,
     verification::{Verification, VerificationMachine, VerificationRequest},
     CrossSigningKeyExport, CryptoStoreError, KeysQueryRequest, LocalTrust, ReadOnlyDevice,
-    RoomKeyImportResult, SignatureError, ToDeviceRequest,
+    SignatureError, ToDeviceRequest,
 };
 
 /// State machine implementation of the Olm/Megolm encryption protocol used for
@@ -1783,49 +1782,6 @@ impl OlmMachine {
         self.store().get_user_devices(user_id).await
     }
 
-    /// Import the given room keys into our store.
-    ///
-    /// # Arguments
-    ///
-    /// * `exported_keys` - A list of previously exported keys that should be
-    /// imported into our store. If we already have a better version of a key
-    /// the key will *not* be imported.
-    ///
-    /// * `from_backup` - Were the room keys imported from the backup, if true
-    /// will mark the room keys as already backed up. This will prevent backing
-    /// up keys that are already backed up.
-    ///
-    /// Returns a tuple of numbers that represent the number of sessions that
-    /// were imported and the total number of sessions that were found in the
-    /// key export.
-    ///
-    /// # Examples
-    ///
-    /// ```no_run
-    /// # use std::io::Cursor;
-    /// # use matrix_sdk_crypto::{OlmMachine, decrypt_room_key_export};
-    /// # use ruma::{device_id, user_id};
-    /// # let alice = user_id!("@alice:example.org");
-    /// # async {
-    /// # let machine = OlmMachine::new(&alice, device_id!("DEVICEID")).await;
-    /// # let export = Cursor::new("".to_owned());
-    /// let exported_keys = decrypt_room_key_export(export, "1234").unwrap();
-    /// machine.import_room_keys(exported_keys, false, |_, _| {}).await.unwrap();
-    /// # };
-    /// ```
-    #[deprecated(
-        since = "0.7.0",
-        note = "Use the OlmMachine::store::import_exported_room_keys method instead"
-    )]
-    pub async fn import_room_keys(
-        &self,
-        exported_keys: Vec<ExportedRoomKey>,
-        from_backup: bool,
-        progress_listener: impl Fn(usize, usize),
-    ) -> StoreResult<RoomKeyImportResult> {
-        self.store().import_room_keys(exported_keys, from_backup, progress_listener).await
-    }
-
     /// Get the status of the private cross signing keys.
     ///
     /// This can be used to check which private cross signing keys we have

crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs

@@ -98,7 +98,10 @@ pub struct ExportedRoomKey {
 }
 
 impl ExportedRoomKey {
-    pub(crate) fn from_backed_up_room_key(
+    /// Create an `ExportedRoomKey` from a `BackedUpRoomKey`.
+    ///
+    /// This can be used when importing the keys from a backup into the store.
+    pub fn from_backed_up_room_key(
         room_id: OwnedRoomId,
         session_id: String,
         room_key: BackedUpRoomKey,

crates/matrix-sdk-crypto/src/store/integration_tests.rs

@@ -290,8 +290,9 @@ macro_rules! cryptostore_integration_tests {
                 );
             }
 
+            /// Test that we can import an inbound group session via [`CryptoStore::save_changes`]
             #[async_test]
-            async fn save_inbound_group_session() {
+            async fn save_changes_save_inbound_group_session() {
                 let (account, store) = get_loaded_store("save_inbound_group_session").await;
 
                 let room_id = &room_id!("!test:localhost");
@@ -303,31 +304,96 @@ macro_rules! cryptostore_integration_tests {
                 store.save_changes(changes).await.expect("Can't save group session");
             }
 
+            /// Test that we can import a backed-up group session via
+            /// [`CryptoStore::save_inbound_group_sessions`]
             #[async_test]
-            async fn save_inbound_group_session_for_backup() {
+            async fn save_inbound_group_session_from_backup() {
                 let (account, store) =
-                    get_loaded_store("save_inbound_group_session_for_backup").await;
+                    get_loaded_store("save_inbound_group_session_from_backup").await;
 
                 let room_id = &room_id!("!test:localhost");
                 let (_, session) = account.create_group_session_pair_with_defaults(room_id).await;
 
-                let changes =
-                    Changes { inbound_group_sessions: vec![session.clone()], ..Default::default() };
+                session.mark_as_backed_up();
+                store
+                    .save_inbound_group_sessions(vec![session.clone()], Some(&"bkpver1"))
+                    .await
+                    .expect("could not save sessions");
 
-                store.save_changes(changes).await.expect("Can't save group session");
+                let loaded_session = store
+                    .get_inbound_group_session(&session.room_id, session.session_id())
+                    .await
+                    .expect("error when loading session")
+                    .expect("session not found in store");
+                assert_eq!(session, loaded_session);
+                assert_eq!(store.get_inbound_group_sessions().await.unwrap().len(), 1);
+                assert_eq!(store.inbound_group_session_counts(None).await.unwrap().total, 1);
+
+                // It should *not* be returned by a request for backup for the same backup version
+                let to_back_up = store.inbound_group_sessions_for_backup("bkpver1", 1).await.unwrap();
+                assert_eq!(to_back_up.len(), 0, "backup was returned by backup query");
+                assert_eq!(
+                    store.inbound_group_session_counts(Some(&"bkpver1")).await.unwrap().backed_up, 1,
+                    "backed_up count",
+                );
+            }
+
+            /// Test that the behaviour of a key imported from an *old* backup is correct
+            ///
+            /// This currently only works on the MemoryStore, so is ignored. The other stores
+            /// are waiting for more work on https://github.com/element-hq/element-web/issues/26892.
+            #[ignore]
+            #[async_test]
+            async fn save_inbound_group_session_from_old_backup() {
+                let (account, store) =
+                    get_loaded_store("save_inbound_group_session_from_old_backup").await;
+
+                let room_id = &room_id!("!test:localhost");
+                let (_, session) = account.create_group_session_pair_with_defaults(room_id).await;
+
+                session.mark_as_backed_up();
+                store
+                    .save_inbound_group_sessions(vec![session.clone()], Some(&"bkpver1"))
+                    .await
+                    .expect("could not save sessions");
+
+                // The session should be returned by a request for backup from a different backup version.
+                let to_back_up = store.inbound_group_sessions_for_backup("bkpver2", 1).await.unwrap();
+                assert_eq!(to_back_up, vec![session]);
+                assert_eq!(
+                    store.inbound_group_session_counts(Some(&"bkpver2")).await.unwrap().backed_up, 0,
+                    "backed_up count for backup version 2",
+                );
+            }
+
+            /// Test that we can import a not-backed-up group session via
+            /// [`CryptoStore::save_inbound_group_sessions`]
+            #[async_test]
+            async fn save_inbound_group_session_from_import() {
+                let (account, store) =
+                    get_loaded_store("save_inbound_group_session_from_import").await;
+
+                let room_id = &room_id!("!test:localhost");
+                let (_, session) = account.create_group_session_pair_with_defaults(room_id).await;
+
+                store
+                    .save_inbound_group_sessions(vec![session.clone()], None)
+                    .await
+                    .expect("could not save sessions");
 
                 let loaded_session = store
                     .get_inbound_group_session(&session.room_id, session.session_id())
                     .await
-                    .unwrap()
-                    .unwrap();
+                    .expect("error when loading session")
+                    .expect("session not found in store");
                 assert_eq!(session, loaded_session);
                 assert_eq!(store.get_inbound_group_sessions().await.unwrap().len(), 1);
                 assert_eq!(store.inbound_group_session_counts(None).await.unwrap().total, 1);
                 assert_eq!(store.inbound_group_session_counts(None).await.unwrap().backed_up, 0);
 
-                let to_back_up = store.inbound_group_sessions_for_backup("bkpver", 1).await.unwrap();
-                assert_eq!(to_back_up, vec![session])
+                // It should be returned by a request for backup
+                let to_back_up = store.inbound_group_sessions_for_backup("bkpver1", 1).await.unwrap();
+                assert_eq!(to_back_up, vec![session]);
             }
 
             #[async_test]