From 66aae2a0b57b4be7e34882754c76e236c42e0a4d Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 May 2024 22:18:31 +0100 Subject: [PATCH 1/8] crypto: Add a failing test demonstrating our problem --- crates/matrix-sdk-crypto/src/backups/mod.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/backups/mod.rs b/crates/matrix-sdk-crypto/src/backups/mod.rs index e0205fbcb..372c7cb21 100644 --- a/crates/matrix-sdk-crypto/src/backups/mod.rs +++ b/crates/matrix-sdk-crypto/src/backups/mod.rs @@ -826,6 +826,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(); @@ -844,14 +850,23 @@ mod tests { .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] From 4cd619ccdd5143da7e6f11647afe098c7d022ab3 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 22 May 2024 23:45:54 +0100 Subject: [PATCH 2/8] crypto: New method `CryptoStore::save_inbound_group_sessions` When we add a batch of inbound group sessions to the store, if they came from a backup, we need to record which backup version they came from. `CryptoStore::save_changes` doesn't give us an easy way to set the backup version (we could add it to the `Changes` struct, but ugh). So, we add a new method `save_inbound_group_sessions` which takes the backup version as a separate param. This works fine, because whenever we import sessions there are no other changes to store. The new method isn't used outside of tests yet -- that comes in a follow-up commit. --- .../src/store/integration_tests.rs | 88 ++++++++++++++++--- .../src/store/memorystore.rs | 54 ++++++++++-- crates/matrix-sdk-crypto/src/store/traits.rs | 24 +++++ .../src/crypto_store/mod.rs | 21 +++++ crates/matrix-sdk-sqlite/src/crypto_store.rs | 22 +++++ 5 files changed, 189 insertions(+), 20 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/store/integration_tests.rs b/crates/matrix-sdk-crypto/src/store/integration_tests.rs index b81cc81e3..55f7cd1fd 100644 --- a/crates/matrix-sdk-crypto/src/store/integration_tests.rs +++ b/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() }; - - store.save_changes(changes).await.expect("Can't save group session"); + session.mark_as_backed_up(); + store + .save_inbound_group_sessions(vec![session.clone()], Some(&"bkpver1")) + .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); + + // 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 + .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] diff --git a/crates/matrix-sdk-crypto/src/store/memorystore.rs b/crates/matrix-sdk-crypto/src/store/memorystore.rs index 6fcfb63af..cace29e12 100644 --- a/crates/matrix-sdk-crypto/src/store/memorystore.rs +++ b/crates/matrix-sdk-crypto/src/store/memorystore.rs @@ -25,6 +25,7 @@ use ruma::{ OwnedUserId, RoomId, TransactionId, UserId, }; use tokio::sync::{Mutex, RwLock}; +use tracing::warn; use super::{ caches::{DeviceStore, GroupSessionStore, SessionStore}, @@ -144,12 +145,6 @@ impl MemoryStore { } } - fn save_inbound_group_sessions(&self, sessions: Vec) { - for session in sessions { - self.inbound_group_sessions.add(session); - } - } - fn save_outbound_group_sessions(&self, sessions: Vec) { self.outbound_group_sessions .write() @@ -225,7 +220,7 @@ impl CryptoStore for MemoryStore { async fn save_changes(&self, changes: Changes) -> Result<()> { self.save_sessions(changes.sessions).await; - self.save_inbound_group_sessions(changes.inbound_group_sessions); + self.save_inbound_group_sessions(changes.inbound_group_sessions, None).await?; self.save_outbound_group_sessions(changes.outbound_group_sessions); self.save_private_identity(changes.private_identity); @@ -299,6 +294,39 @@ impl CryptoStore for MemoryStore { Ok(()) } + async fn save_inbound_group_sessions( + &self, + sessions: Vec, + backed_up_to_version: Option<&str>, + ) -> Result<()> { + let mut inbound_group_sessions_backed_up_to = + self.inbound_group_sessions_backed_up_to.write().unwrap(); + + for session in sessions { + let room_id = session.room_id(); + let session_id = session.session_id(); + + // Sanity-check that the data in the sessions corresponds to backed_up_version + let backed_up = session.backed_up(); + if backed_up != backed_up_to_version.is_some() { + warn!( + backed_up, + backed_up_to_version, + "Session backed-up flag does not correspond to backup version setting", + ); + } + + if let Some(backup_version) = backed_up_to_version { + inbound_group_sessions_backed_up_to + .entry(room_id.to_owned()) + .or_default() + .insert(session_id.to_owned(), BackupVersion::from(backup_version)); + } + self.inbound_group_sessions.add(session); + } + Ok(()) + } + async fn get_sessions(&self, sender_key: &str) -> Result>>>> { Ok(self.sessions.get(sender_key)) } @@ -632,7 +660,7 @@ mod tests { .unwrap(); let store = MemoryStore::new(); - store.save_inbound_group_sessions(vec![inbound.clone()]); + store.save_inbound_group_sessions(vec![inbound.clone()], None).await.unwrap(); let loaded_session = store.get_inbound_group_session(room_id, outbound.session_id()).await.unwrap().unwrap(); @@ -1007,7 +1035,7 @@ mod tests { sessions.sort_by_key(|s| s.session_id().to_owned()); let store = MemoryStore::new(); - store.save_inbound_group_sessions(sessions.clone()); + store.save_inbound_group_sessions(sessions.clone(), None).await.unwrap(); (store, sessions) } @@ -1137,6 +1165,14 @@ mod integration_tests { self.0.save_pending_changes(changes).await } + async fn save_inbound_group_sessions( + &self, + sessions: Vec, + backed_up_to_version: Option<&str>, + ) -> Result<(), Self::Error> { + self.0.save_inbound_group_sessions(sessions, backed_up_to_version).await + } + async fn get_sessions( &self, sender_key: &str, diff --git a/crates/matrix-sdk-crypto/src/store/traits.rs b/crates/matrix-sdk-crypto/src/store/traits.rs index 028611741..442b30c25 100644 --- a/crates/matrix-sdk-crypto/src/store/traits.rs +++ b/crates/matrix-sdk-crypto/src/store/traits.rs @@ -65,6 +65,22 @@ pub trait CryptoStore: AsyncTraitDeps { /// * `changes` - The set of changes that should be stored. async fn save_pending_changes(&self, changes: PendingChanges) -> Result<(), Self::Error>; + /// Save a list of inbound group sessions to the store. + /// + /// # Arguments + /// + /// * `sessions` - The sessions to be saved. + /// * `backed_up_to_version` - If the keys should be marked as having been + /// backed up, the version of the backup. + /// + /// Note: some implementations ignore `backup_version` and assume the + /// current backup version, which is normally the same. + async fn save_inbound_group_sessions( + &self, + sessions: Vec, + backed_up_to_version: Option<&str>, + ) -> Result<(), Self::Error>; + /// Get all the sessions that belong to the given sender key. /// /// # Arguments @@ -347,6 +363,14 @@ impl CryptoStore for EraseCryptoStoreError { self.0.save_pending_changes(changes).await.map_err(Into::into) } + async fn save_inbound_group_sessions( + &self, + sessions: Vec, + backed_up_to_version: Option<&str>, + ) -> Result<()> { + self.0.save_inbound_group_sessions(sessions, backed_up_to_version).await.map_err(Into::into) + } + async fn get_sessions(&self, sender_key: &str) -> Result>>>> { self.0.get_sessions(sender_key).await.map_err(Into::into) } diff --git a/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs b/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs index d74192dce..c8c2b33ad 100644 --- a/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs +++ b/crates/matrix-sdk-indexeddb/src/crypto_store/mod.rs @@ -721,6 +721,27 @@ impl_crypto_store! { Ok(()) } + async fn save_inbound_group_sessions( + &self, + sessions: Vec, + backed_up_to_version: Option<&str>, + ) -> Result<()> { + // Sanity-check that the data in the sessions corresponds to backed_up_version + sessions.iter().for_each(|s| { + let backed_up = s.backed_up(); + if backed_up != backed_up_to_version.is_some() { + warn!( + backed_up, backed_up_to_version, + "Session backed-up flag does not correspond to backup version setting", + ); + } + }); + + // Currently, this store doesn't save the backup version separately, so this + // just delegates to save_changes. + self.save_changes(Changes { inbound_group_sessions: sessions, ..Changes::default() }).await + } + async fn load_tracked_users(&self) -> Result> { let tx = self .inner diff --git a/crates/matrix-sdk-sqlite/src/crypto_store.rs b/crates/matrix-sdk-sqlite/src/crypto_store.rs index 27ee1c5a5..39b5fd729 100644 --- a/crates/matrix-sdk-sqlite/src/crypto_store.rs +++ b/crates/matrix-sdk-sqlite/src/crypto_store.rs @@ -886,6 +886,28 @@ impl CryptoStore for SqliteCryptoStore { Ok(()) } + async fn save_inbound_group_sessions( + &self, + sessions: Vec, + backed_up_to_version: Option<&str>, + ) -> matrix_sdk_crypto::store::Result<(), Self::Error> { + // Sanity-check that the data in the sessions corresponds to backed_up_version + sessions.iter().for_each(|s| { + let backed_up = s.backed_up(); + if backed_up != backed_up_to_version.is_some() { + warn!( + backed_up, + backed_up_to_version, + "Session backed-up flag does not correspond to backup version setting", + ); + } + }); + + // Currently, this store doesn't save the backup version separately, so this + // just delegates to save_changes. + self.save_changes(Changes { inbound_group_sessions: sessions, ..Changes::default() }).await + } + async fn get_sessions(&self, sender_key: &str) -> Result>>>> { let account_info = self.get_static_account().ok_or(Error::AccountUnset)?; From f77d2cd83f25790c910afda2debff9459e74737a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 23 May 2024 09:40:05 +0100 Subject: [PATCH 3/8] crypto: Use new `CryptoStore::save_inbound_group_sessions` method When we are importing a batch of room keys, use the newly-added `CryptoStore::save_inbound_group_sessions` method instead of `CryptoStore::save_changes`. To do this, we need to pass the backup version into `Store::import_room_keys` instead of just `from_backup` flag. --- crates/matrix-sdk-crypto/src/backups/mod.rs | 10 +++++++++- crates/matrix-sdk-crypto/src/machine.rs | 6 +++++- crates/matrix-sdk-crypto/src/store/mod.rs | 22 +++++++++++++++------ 3 files changed, 30 insertions(+), 8 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/backups/mod.rs b/crates/matrix-sdk-crypto/src/backups/mod.rs index 372c7cb21..1568aa548 100644 --- a/crates/matrix-sdk-crypto/src/backups/mod.rs +++ b/crates/matrix-sdk-crypto/src/backups/mod.rs @@ -619,7 +619,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 } } diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 91bf21338..4479c1541 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1823,7 +1823,11 @@ impl OlmMachine { from_backup: bool, progress_listener: impl Fn(usize, usize), ) -> StoreResult { - self.store().import_room_keys(exported_keys, from_backup, progress_listener).await + let backup_version = + if from_backup { self.backup_machine().backup_version().await } else { None }; + self.store() + .import_room_keys(exported_keys, backup_version.as_deref(), progress_listener) + .await } /// Get the status of the private cross signing keys. diff --git a/crates/matrix-sdk-crypto/src/store/mod.rs b/crates/matrix-sdk-crypto/src/store/mod.rs index bbc184719..0ec4e36c3 100644 --- a/crates/matrix-sdk-crypto/src/store/mod.rs +++ b/crates/matrix-sdk-crypto/src/store/mod.rs @@ -1630,10 +1630,22 @@ impl Store { self.inner.store.secrets_stream() } + /// Import the given room keys into the store. + /// + /// # Arguments + /// + /// * `exported_keys` - The keys to be imported. + /// * `from_backup_version` - If the keys came from key backup, the key + /// backup version. This will cause the keys to be marked as already + /// backed up, and therefore not requiring another backup. + /// * `progress_listener` - Callback which will be called after each key is + /// processed. Called with arguments `(processed, total)` where + /// `processed` is the number of keys processed so far, and `total` is the + /// total number of keys (i.e., `exported_keys.len()`). pub(crate) async fn import_room_keys( &self, exported_keys: Vec, - from_backup: bool, + from_backup_version: Option<&str>, progress_listener: impl Fn(usize, usize), ) -> Result { let mut sessions = Vec::new(); @@ -1664,7 +1676,7 @@ impl Store { // Only import the session if we didn't have this session or // if it's a better version of the same session. if new_session_better(&session, old_session).await { - if from_backup { + if from_backup_version.is_some() { session.mark_as_backed_up(); } @@ -1693,9 +1705,7 @@ impl Store { let imported_count = sessions.len(); - let changes = Changes { inbound_group_sessions: sessions, ..Default::default() }; - - self.save_changes(changes).await?; + self.inner.store.save_inbound_group_sessions(sessions, from_backup_version).await?; info!(total_count, imported_count, room_keys = ?keys, "Successfully imported room keys"); @@ -1733,7 +1743,7 @@ impl Store { exported_keys: Vec, progress_listener: impl Fn(usize, usize), ) -> Result { - self.import_room_keys(exported_keys, false, progress_listener).await + self.import_room_keys(exported_keys, None, progress_listener).await } pub(crate) fn crypto_store(&self) -> Arc { From f7cc17e1e04958092ab5beb053091d8b5dacfc90 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 23 May 2024 00:32:57 +0100 Subject: [PATCH 4/8] crypto: Deprecate `BackupMachine::import_backed_up_room_keys` This whole method is a bit broken. It assumes that, when we did an import from backup, the backup that they came from is the same as the "current" version. We *could* add another argument, but to be honest I find the whole method a bit pointless. It's not particularly tied to the `BackupMachine` abstraction. Let's instead expose `Store::import_room_keys` and `ExportedRooomKey::from_backed_up_room_key` publicly, and tell callers to use that directly. --- crates/matrix-sdk-crypto/src/backups/mod.rs | 2 ++ crates/matrix-sdk-crypto/src/machine.rs | 2 +- crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs | 5 ++++- crates/matrix-sdk-crypto/src/store/mod.rs | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/backups/mod.rs b/crates/matrix-sdk-crypto/src/backups/mod.rs index 1568aa548..bd7f5118e 100644 --- a/crates/matrix-sdk-crypto/src/backups/mod.rs +++ b/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>, @@ -853,6 +854,7 @@ 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 diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 4479c1541..695f5ff9f 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -1815,7 +1815,7 @@ impl OlmMachine { /// ``` #[deprecated( since = "0.7.0", - note = "Use the OlmMachine::store::import_exported_room_keys method instead" + note = "Use the OlmMachine::store::import_room_keys method instead" )] pub async fn import_room_keys( &self, diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs index b6ef4bc13..f832723e0 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs +++ b/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, diff --git a/crates/matrix-sdk-crypto/src/store/mod.rs b/crates/matrix-sdk-crypto/src/store/mod.rs index 0ec4e36c3..6d6201d54 100644 --- a/crates/matrix-sdk-crypto/src/store/mod.rs +++ b/crates/matrix-sdk-crypto/src/store/mod.rs @@ -1642,7 +1642,7 @@ impl Store { /// processed. Called with arguments `(processed, total)` where /// `processed` is the number of keys processed so far, and `total` is the /// total number of keys (i.e., `exported_keys.len()`). - pub(crate) async fn import_room_keys( + pub async fn import_room_keys( &self, exported_keys: Vec, from_backup_version: Option<&str>, From ba38d0c1de80adc6e3ee26c6078a5417d5b9ace6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 23 May 2024 11:15:29 +0100 Subject: [PATCH 5/8] sdk: avoid deprecated `BackupMachine::import_backed_up_room_keys` ... and use its replacement, `Store::import_room_keys` --- .../matrix-sdk/src/encryption/backups/mod.rs | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/crates/matrix-sdk/src/encryption/backups/mod.rs b/crates/matrix-sdk/src/encryption/backups/mod.rs index 0ddab7024..48f2445ca 100644 --- a/crates/matrix-sdk/src/encryption/backups/mod.rs +++ b/crates/matrix-sdk/src/encryption/backups/mod.rs @@ -52,7 +52,9 @@ pub(crate) mod types; pub use types::{BackupState, UploadState}; use self::futures::WaitForSteadyState; -use crate::{encryption::BackupDownloadStrategy, Client, Error, Room}; +use crate::{ + crypto::olm::ExportedRoomKey, encryption::BackupDownloadStrategy, Client, Error, Room, +}; /// The backups manager for the [`Client`]. #[derive(Debug, Clone)] @@ -370,7 +372,7 @@ impl Backups { if let Some(decryption_key) = backup_keys.decryption_key { if let Some(version) = backup_keys.backup_version { let request = - get_backup_keys_for_room::v3::Request::new(version, room_id.to_owned()); + get_backup_keys_for_room::v3::Request::new(version.clone(), room_id.to_owned()); let response = self.client.send(request, Default::default()).await?; // Transform response to standard format (map of room ID -> room key). @@ -379,7 +381,8 @@ impl Backups { RoomKeyBackup::new(response.sessions), )])); - self.handle_downloaded_room_keys(response, decryption_key, olm_machine).await?; + self.handle_downloaded_room_keys(response, decryption_key, &version, olm_machine) + .await?; } } @@ -396,7 +399,7 @@ impl Backups { if let Some(decryption_key) = backup_keys.decryption_key { if let Some(version) = backup_keys.backup_version { let request = get_backup_keys_for_session::v3::Request::new( - version, + version.clone(), room_id.to_owned(), session_id.to_owned(), ); @@ -411,7 +414,8 @@ impl Backups { )])), )])); - self.handle_downloaded_room_keys(response, decryption_key, olm_machine).await?; + self.handle_downloaded_room_keys(response, decryption_key, &version, olm_machine) + .await?; } } @@ -445,9 +449,10 @@ impl Backups { &self, backed_up_keys: get_backup_keys::v3::Response, backup_decryption_key: BackupDecryptionKey, + backup_version: &str, olm_machine: &OlmMachine, ) -> Result<(), Error> { - let mut decrypted_room_keys: BTreeMap<_, BTreeMap<_, _>> = BTreeMap::new(); + let mut decrypted_room_keys: Vec<_> = Vec::new(); for (room_id, room_keys) in backed_up_keys.rooms { for (session_id, room_key) in room_keys.sessions { @@ -474,16 +479,17 @@ impl Backups { } }; - decrypted_room_keys - .entry(room_id.to_owned()) - .or_default() - .insert(session_id, room_key); + decrypted_room_keys.push(ExportedRoomKey::from_backed_up_room_key( + room_id.to_owned(), + session_id, + room_key, + )); } } let result = olm_machine - .backup_machine() - .import_backed_up_room_keys(decrypted_room_keys, |_, _| {}) + .store() + .import_room_keys(decrypted_room_keys, Some(backup_version), |_, _| {}) .await?; // Since we can't use the usual room keys stream from the `OlmMachine` @@ -499,13 +505,13 @@ impl Backups { decryption_key: BackupDecryptionKey, version: String, ) -> Result<(), Error> { - let request = get_backup_keys::v3::Request::new(version); + let request = get_backup_keys::v3::Request::new(version.clone()); let response = self.client.send(request, Default::default()).await?; let olm_machine = self.client.olm_machine().await; let olm_machine = olm_machine.as_ref().ok_or(Error::NoOlmMachine)?; - self.handle_downloaded_room_keys(response, decryption_key, olm_machine).await?; + self.handle_downloaded_room_keys(response, decryption_key, &version, olm_machine).await?; Ok(()) } @@ -920,7 +926,6 @@ impl Backups { mod test { use std::time::Duration; - use matrix_sdk_base::crypto::olm::ExportedRoomKey; use matrix_sdk_test::async_test; use serde_json::json; use wiremock::{ From 05e4d7a5027d9f3258b900a48663c67c0eacfaa6 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 23 May 2024 11:43:12 +0100 Subject: [PATCH 6/8] ffi: Deprecate `import_decrypted_room_keys` ... and expose a new method `import_room_keys_from_backup` which does the right thing. --- bindings/matrix-sdk-crypto-ffi/src/machine.rs | 46 ++++++++++++++++--- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/bindings/matrix-sdk-crypto-ffi/src/machine.rs b/bindings/matrix-sdk-crypto-ffi/src/machine.rs index f9f17dd9e..755383fe4 100644 --- a/bindings/matrix-sdk-crypto-ffi/src/machine.rs +++ b/bindings/matrix-sdk-crypto-ffi/src/machine.rs @@ -991,7 +991,7 @@ impl OlmMachine { ) -> Result { 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, ) -> Result { + // 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 = 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, + ) -> Result { + let keys: Vec = 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, - from_backup: bool, + from_backup_version: Option<&str>, progress_listener: Box, ) -> Result { 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, From 3945071446ad6a81e981c3f5f7a49e95815bc9d2 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 23 May 2024 11:08:35 +0100 Subject: [PATCH 7/8] crypto: Update changelog --- crates/matrix-sdk-crypto/CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index ddcb73893..0db0d972d 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -31,8 +31,16 @@ Breaking changes: - Add new `dehydrated` property to `olm::account::PickledAccount`. ([#3164](https://github.com/matrix-org/matrix-rust-sdk/pull/3164)) +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)) From 0777aa6ece74bb1b6aa39896a15c08a75b15ed62 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 23 May 2024 15:11:19 +0100 Subject: [PATCH 8/8] crypto: Remove `Olm::import_room_keys` altogether --- crates/matrix-sdk-crypto/CHANGELOG.md | 3 ++ .../src/file_encryption/key_export.rs | 2 +- crates/matrix-sdk-crypto/src/machine.rs | 54 ++----------------- crates/matrix-sdk-crypto/src/store/mod.rs | 2 +- 4 files changed, 8 insertions(+), 53 deletions(-) diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index 0db0d972d..9d6b45fbe 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -31,6 +31,9 @@ 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`. diff --git a/crates/matrix-sdk-crypto/src/file_encryption/key_export.rs b/crates/matrix-sdk-crypto/src/file_encryption/key_export.rs index ed7f1740a..be168efe3 100644 --- a/crates/matrix-sdk-crypto/src/file_encryption/key_export.rs +++ b/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( diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 695f5ff9f..1e0827551 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/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,53 +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_room_keys method instead" - )] - pub async fn import_room_keys( - &self, - exported_keys: Vec, - from_backup: bool, - progress_listener: impl Fn(usize, usize), - ) -> StoreResult { - let backup_version = - if from_backup { self.backup_machine().backup_version().await } else { None }; - self.store() - .import_room_keys(exported_keys, backup_version.as_deref(), 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 diff --git a/crates/matrix-sdk-crypto/src/store/mod.rs b/crates/matrix-sdk-crypto/src/store/mod.rs index 6d6201d54..f1fe9a119 100644 --- a/crates/matrix-sdk-crypto/src/store/mod.rs +++ b/crates/matrix-sdk-crypto/src/store/mod.rs @@ -1735,7 +1735,7 @@ impl Store { /// # 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_exported_room_keys(exported_keys, |_, _| {}).await.unwrap(); /// # }; /// ``` pub async fn import_exported_room_keys(