From 53f1b32896e0d3367470f60021349747f81d400c Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 19 Jun 2024 16:30:37 -0400 Subject: [PATCH 01/11] initial work on re-sharing sessions after unwedging --- .../src/identities/device.rs | 11 +++++++++- .../src/olm/group_sessions/outbound.rs | 21 ++++++++++++------- .../src/session_manager/group_sessions.rs | 9 ++++++-- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/device.rs b/crates/matrix-sdk-crypto/src/identities/device.rs index 79cd50eea52..7e095f36803 100644 --- a/crates/matrix-sdk-crypto/src/identities/device.rs +++ b/crates/matrix-sdk-crypto/src/identities/device.rs @@ -89,6 +89,9 @@ pub struct ReadOnlyDevice { /// Default to epoch for migration purpose. #[serde(default = "default_timestamp")] first_time_seen_ts: MilliSecondsSinceUnixEpoch, + /// The number of times the device has tried to unwedge Olm sessions with us. + #[serde(default)] + pub(crate) olm_wedging_index: u32, } fn default_timestamp() -> MilliSecondsSinceUnixEpoch { @@ -580,6 +583,7 @@ impl ReadOnlyDevice { deleted: Arc::new(AtomicBool::new(false)), withheld_code_sent: Arc::new(AtomicBool::new(false)), first_time_seen_ts: MilliSecondsSinceUnixEpoch::now(), + olm_wedging_index: 0, } } @@ -833,7 +837,11 @@ impl ReadOnlyDevice { match self.encrypt(store, event_type, content).await { Ok((session, encrypted)) => Ok(MaybeEncryptedRoomKey::Encrypted { - share_info: ShareInfo::new_shared(session.sender_key().to_owned(), message_index), + share_info: ShareInfo::new_shared( + session.sender_key().to_owned(), + message_index, + self.olm_wedging_index, + ), used_session: session, message: encrypted.cast(), }), @@ -970,6 +978,7 @@ impl TryFrom<&DeviceKeys> for ReadOnlyDevice { trust_state: Arc::new(RwLock::new(LocalTrust::Unset)), withheld_code_sent: Arc::new(AtomicBool::new(false)), first_time_seen_ts: MilliSecondsSinceUnixEpoch::now(), + olm_wedging_index: 0, }; device.verify_device_keys(device_keys)?; diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs index c11f9020290..4465dacf215 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs @@ -69,7 +69,7 @@ const ROTATION_MESSAGES: u64 = 100; pub(crate) enum ShareState { NotShared, SharedButChangedSenderKey, - Shared(u32), + Shared(u32, u32), } /// Settings for an encrypted room. @@ -168,8 +168,12 @@ pub enum ShareInfo { impl ShareInfo { /// Helper to create a SharedWith info - pub fn new_shared(sender_key: Curve25519PublicKey, message_index: u32) -> Self { - ShareInfo::Shared(SharedWith { sender_key, message_index }) + pub fn new_shared( + sender_key: Curve25519PublicKey, + message_index: u32, + olm_wedging_index: u32, + ) -> Self { + ShareInfo::Shared(SharedWith { sender_key, message_index, olm_wedging_index }) } /// Helper to create a Withheld info @@ -184,6 +188,9 @@ pub struct SharedWith { pub sender_key: Curve25519PublicKey, /// The message index that the device received. pub message_index: u32, + /// The Olm wedging index of the device at the time the session was shared. + #[serde(default)] + pub olm_wedging_index: u32, } impl OutboundGroupSession { @@ -526,7 +533,7 @@ impl OutboundGroupSession { d.get(device.device_id()).map(|s| match s { ShareInfo::Shared(s) => { if device.curve25519_key() == Some(s.sender_key) { - ShareState::Shared(s.message_index) + ShareState::Shared(s.message_index, s.olm_wedging_index) } else { ShareState::SharedButChangedSenderKey } @@ -550,7 +557,7 @@ impl OutboundGroupSession { Some(match info { ShareInfo::Shared(info) => { if device.curve25519_key() == Some(info.sender_key) { - ShareState::Shared(info.message_index) + ShareState::Shared(info.message_index, info.olm_wedging_index) } else { ShareState::SharedButChangedSenderKey } @@ -602,7 +609,7 @@ impl OutboundGroupSession { .unwrap() .entry(user_id.to_owned()) .or_default() - .insert(device_id.to_owned(), ShareInfo::new_shared(sender_key, index)); + .insert(device_id.to_owned(), ShareInfo::new_shared(sender_key, index, 0)); } /// Mark the session as shared with the given user/device pair, starting @@ -614,7 +621,7 @@ impl OutboundGroupSession { device_id: &DeviceId, sender_key: Curve25519PublicKey, ) { - let share_info = ShareInfo::new_shared(sender_key, self.message_index().await); + let share_info = ShareInfo::new_shared(sender_key, self.message_index().await, 0); self.shared_with_set .write() .unwrap() diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs index 3ae12bc71da..dae20c9212b 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs @@ -797,8 +797,13 @@ impl GroupSessionManager { let devices: Vec<_> = devices .into_iter() .flat_map(|(_, d)| { - d.into_iter() - .filter(|d| matches!(outbound.is_shared_with(d), ShareState::NotShared)) + d.into_iter().filter(|d| match outbound.is_shared_with(d) { + ShareState::NotShared => true, + ShareState::Shared(_msg_index, olm_wedging_index) => { + olm_wedging_index < d.olm_wedging_index + } + _ => false, + }) }) .collect(); From 29db401036012a987b165f83cd5d6d7495f5e322 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 21 Jun 2024 21:11:20 -0400 Subject: [PATCH 02/11] update olm wedging index, and add test --- crates/matrix-sdk-crypto/src/olm/account.rs | 24 +++- .../src/session_manager/group_sessions.rs | 124 ++++++++++++++++++ 2 files changed, 141 insertions(+), 7 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/account.rs b/crates/matrix-sdk-crypto/src/olm/account.rs index 81c58c0e071..d493a64c5f5 100644 --- a/crates/matrix-sdk-crypto/src/olm/account.rs +++ b/crates/matrix-sdk-crypto/src/olm/account.rs @@ -62,7 +62,7 @@ use crate::{ error::{EventError, OlmResult, SessionCreationError}, identities::ReadOnlyDevice, requests::UploadSigningKeysRequest, - store::{Changes, Store}, + store::{Changes, DeviceChanges, Store}, types::{ events::{ olm_v1::AnyDecryptedOlmEvent, @@ -1305,12 +1305,22 @@ impl Account { // we might try to create the same session again. // TODO: separate the session cache from the storage so we only add // it to the cache but don't store it. - store - .save_changes(Changes { - sessions: vec![result.session.clone()], - ..Default::default() - }) - .await?; + let mut changes = + Changes { sessions: vec![result.session.clone()], ..Default::default() }; + // Any new Olm session will bump the Olm wedging index for the + // sender's device, if we have their device, which will cause + // us to re-send existing Megolm sessions to them the next time + // we use the session. If we don't have there device, this + // means that we haven't tried to send them any Megolm sessions + // yet, so we don't need to worry about it. + if let Some(device) = store.get_device_from_curve_key(sender, sender_key).await? { + let mut read_only_device = device.inner; + read_only_device.olm_wedging_index += 1; + debug!(read_only_device.olm_wedging_index, "Olm wedging index"); + changes.devices = + DeviceChanges { changed: vec![read_only_device], ..Default::default() }; + } + store.save_changes(changes).await?; Ok((SessionType::New(result.session), result.plaintext)) } diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs index dae20c9212b..0d23e13a28e 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs @@ -1412,4 +1412,128 @@ mod tests { assert!(device.was_withheld_code_sent()); } + + #[async_test] + async fn test_resend_session_after_unwedging() { + use super::GroupSessionManager; + use crate::{ + identities::ReadOnlyDevice, + olm::{Account, PrivateCrossSigningIdentity}, + store::{Changes, CryptoStoreWrapper, DeviceChanges, MemoryStore, Store}, + types::events::room::encrypted::EncryptedToDeviceEvent, + verification::VerificationMachine, + }; + use tokio::sync::Mutex; + use vodozemac::olm::SessionConfig; + + let room_id = room_id!("!test:localhost"); + let mut account = Account::new(alice_id()); + let crypto_store = Arc::new(CryptoStoreWrapper::new(alice_id(), MemoryStore::new())); + let user_identity = PrivateCrossSigningIdentity::empty(alice_id()); + let user_identity = Arc::new(Mutex::new(user_identity)); + let verification_machine = + VerificationMachine::new(account.clone(), user_identity.clone(), crypto_store.clone()); + let store = Store::new( + account.static_data.clone(), + user_identity, + crypto_store, + verification_machine, + ); + let group_sessions = GroupSessionManager::new(store.clone()); + + let bob_id = user_id!("@bob:localhost"); + let bob_account = Account::new(&bob_id); + + store + .save_changes(Changes { + devices: DeviceChanges { + new: vec![ReadOnlyDevice::from_account(&bob_account)], + ..Default::default() + }, + ..Default::default() + }) + .await + .unwrap(); + + let alice_otks = account.one_time_keys(); + let mut alice_otks = alice_otks.values(); + let alice_otk1 = dbg!(alice_otks.next().unwrap()); + let alice_otk2 = alice_otks.next().unwrap(); + let alice_device = ReadOnlyDevice::from_account(&account); + + // Bob creates an Olm session with Alice and encrypts a message to her + let mut session1 = bob_account.create_outbound_session_helper( + SessionConfig::default(), + account.identity_keys().curve25519, + *alice_otk1, + false, + ); + let content1 = session1.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap(); + + let to_device1 = + EncryptedToDeviceEvent::new(bob_id.to_owned(), content1.deserialize().unwrap()); + + // Alice decrypts the message + account.decrypt_to_device_event(&store, &to_device1).await.unwrap(); + + // Alice shares the room key with Bob + let requests1 = group_sessions + .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .await + .unwrap(); + + // We should have had one to-device event + let event_count1: usize = requests1 + .iter() + .filter(|r| r.event_type == "m.room.encrypted".into()) + .map(|r| r.message_count()) + .sum(); + assert_eq!(event_count1, 1); + + for request in requests1 { + group_sessions.mark_request_as_sent(&request.txn_id).await.unwrap(); + } + + // When Alice shares the room key again, there shouldn't be any + // to-device events, since we already shared with Bob + let requests2 = group_sessions + .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .await + .unwrap(); + + let event_count2: usize = requests2 + .iter() + .filter(|r| r.event_type == "m.room.encrypted".into()) + .map(|r| r.message_count()) + .sum(); + assert_eq!(event_count2, 0); + + // Pretend that Bob wasn't able to decrypt, so he tries to unwedge + let mut session2 = bob_account.create_outbound_session_helper( + SessionConfig::default(), + account.identity_keys().curve25519, + *alice_otk2, + false, + ); + let content2 = session2.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap(); + + let to_device2 = + EncryptedToDeviceEvent::new(bob_id.to_owned(), content2.deserialize().unwrap()); + + // Alice decrypts the unwedge message + account.decrypt_to_device_event(&store, &to_device2).await.unwrap(); + + // When Alice shares the room key again, it should be re-shared with Bob + let requests3 = group_sessions + .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .await + .unwrap(); + + let event_count3: usize = requests3 + .iter() + .filter(|r| r.event_type == "m.room.encrypted".into()) + .map(|r| r.message_count()) + .sum(); + assert_eq!(event_count3, 1); + } } From c6ea95dc9d0cd4ae8f37389dcd1fb3c65dd86cfc Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 24 Jun 2024 17:51:10 -0400 Subject: [PATCH 03/11] simplify test --- .../src/session_manager/group_sessions.rs | 266 +++++++++++------- 1 file changed, 158 insertions(+), 108 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs index 0d23e13a28e..d31ad36e6d2 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs @@ -856,13 +856,19 @@ impl GroupSessionManager { #[cfg(test)] mod tests { - use std::{collections::BTreeSet, iter, ops::Deref, sync::Arc}; + use std::{ + collections::{BTreeMap, BTreeSet}, + iter, + ops::Deref, + sync::Arc, + }; + use assert_matches2::assert_let; use matrix_sdk_test::{async_test, response_from_file}; use ruma::{ api::{ client::{ - keys::{claim_keys, get_keys}, + keys::{claim_keys, get_keys, upload_keys}, to_device::send_event_to_device::v3::Response as ToDeviceResponse, }, IncomingResponse, @@ -871,17 +877,23 @@ mod tests { events::room::history_visibility::HistoryVisibility, room_id, to_device::DeviceIdOrAllDevices, - user_id, DeviceId, TransactionId, UserId, + user_id, DeviceId, DeviceKeyAlgorithm, TransactionId, UInt, UserId, }; use serde_json::{json, Value}; use crate::{ + identities::ReadOnlyDevice, + machine::EncryptionSyncChanges, + olm::Account, session_manager::group_sessions::CollectRecipientsResult, types::{ - events::room_key_withheld::{ - RoomKeyWithheldContent, RoomKeyWithheldContent::MegolmV1AesSha2, WithheldCode, + events::{ + room::encrypted::EncryptedToDeviceEvent, + room_key_withheld::{ + RoomKeyWithheldContent, RoomKeyWithheldContent::MegolmV1AesSha2, WithheldCode, + }, }, - EventEncryptionAlgorithm, + DeviceKeys, EventEncryptionAlgorithm, }, EncryptionSettings, LocalTrust, OlmMachine, ToDeviceRequest, }; @@ -1415,125 +1427,163 @@ mod tests { #[async_test] async fn test_resend_session_after_unwedging() { - use super::GroupSessionManager; - use crate::{ - identities::ReadOnlyDevice, - olm::{Account, PrivateCrossSigningIdentity}, - store::{Changes, CryptoStoreWrapper, DeviceChanges, MemoryStore, Store}, - types::events::room::encrypted::EncryptedToDeviceEvent, - verification::VerificationMachine, - }; - use tokio::sync::Mutex; - use vodozemac::olm::SessionConfig; + let machine = OlmMachine::new(alice_id(), alice_device_id()).await; + assert_let!( + Ok(Some((txn_id, device_keys_request))) = dbg!(machine.upload_device_keys().await) + ); + let device_keys_response = upload_keys::v3::Response::new(BTreeMap::from([( + DeviceKeyAlgorithm::SignedCurve25519, + UInt::new(device_keys_request.one_time_keys.len() as u64).unwrap(), + )])); + machine.mark_request_as_sent(&txn_id, &device_keys_response).await.unwrap(); let room_id = room_id!("!test:localhost"); - let mut account = Account::new(alice_id()); - let crypto_store = Arc::new(CryptoStoreWrapper::new(alice_id(), MemoryStore::new())); - let user_identity = PrivateCrossSigningIdentity::empty(alice_id()); - let user_identity = Arc::new(Mutex::new(user_identity)); - let verification_machine = - VerificationMachine::new(account.clone(), user_identity.clone(), crypto_store.clone()); - let store = Store::new( - account.static_data.clone(), - user_identity, - crypto_store, - verification_machine, - ); - let group_sessions = GroupSessionManager::new(store.clone()); let bob_id = user_id!("@bob:localhost"); let bob_account = Account::new(&bob_id); + let keys_query_data = json!({ + "device_keys": { + "@bob:localhost": { + bob_account.device_id.clone(): bob_account.device_keys() + } + } + }); + let keys_query = + get_keys::v3::Response::try_from_http_response(response_from_file(&keys_query_data)) + .unwrap(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &dbg!(keys_query)).await.unwrap(); + + let alice_device_keys = + device_keys_request.device_keys.unwrap().deserialize_as::().unwrap(); + let mut alice_otks = device_keys_request.one_time_keys.iter(); + let alice_device = ReadOnlyDevice::new(alice_device_keys, LocalTrust::Unset); + + { + // Bob creates an Olm session with Alice and encrypts a message to her + let (alice_otk_id, alice_otk) = alice_otks.next().unwrap(); + let mut session = bob_account + .create_outbound_session( + &alice_device, + &BTreeMap::from([(alice_otk_id.clone(), alice_otk.clone())]), + ) + .unwrap(); + let content = session.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap(); + + let to_device = + EncryptedToDeviceEvent::new(bob_id.to_owned(), content.deserialize().unwrap()); + + // Alice decrypts the message + let sync_changes = EncryptionSyncChanges { + to_device_events: vec![crate::utilities::json_convert(&to_device).unwrap()], + changed_devices: &Default::default(), + one_time_keys_counts: &Default::default(), + unused_fallback_keys: None, + next_batch_token: None, + }; + let (decrypted, _) = machine.receive_sync_changes(sync_changes).await.unwrap(); - store - .save_changes(Changes { - devices: DeviceChanges { - new: vec![ReadOnlyDevice::from_account(&bob_account)], - ..Default::default() - }, - ..Default::default() - }) - .await - .unwrap(); - - let alice_otks = account.one_time_keys(); - let mut alice_otks = alice_otks.values(); - let alice_otk1 = dbg!(alice_otks.next().unwrap()); - let alice_otk2 = alice_otks.next().unwrap(); - let alice_device = ReadOnlyDevice::from_account(&account); - - // Bob creates an Olm session with Alice and encrypts a message to her - let mut session1 = bob_account.create_outbound_session_helper( - SessionConfig::default(), - account.identity_keys().curve25519, - *alice_otk1, - false, - ); - let content1 = session1.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap(); - - let to_device1 = - EncryptedToDeviceEvent::new(bob_id.to_owned(), content1.deserialize().unwrap()); - - // Alice decrypts the message - account.decrypt_to_device_event(&store, &to_device1).await.unwrap(); + assert_eq!(1, decrypted.len()); + } // Alice shares the room key with Bob - let requests1 = group_sessions - .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) - .await - .unwrap(); - - // We should have had one to-device event - let event_count1: usize = requests1 - .iter() - .filter(|r| r.event_type == "m.room.encrypted".into()) - .map(|r| r.message_count()) - .sum(); - assert_eq!(event_count1, 1); - - for request in requests1 { - group_sessions.mark_request_as_sent(&request.txn_id).await.unwrap(); + { + let requests = machine + .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .await + .unwrap(); + + // We should have had one to-device event + let event_count: usize = requests + .iter() + .filter(|r| r.event_type == "m.room.encrypted".into()) + .map(|r| r.message_count()) + .sum(); + assert_eq!(event_count, 1); + + let response = ToDeviceResponse::new(); + for request in requests { + machine.mark_request_as_sent(&request.txn_id, &response).await.unwrap(); + } } // When Alice shares the room key again, there shouldn't be any // to-device events, since we already shared with Bob - let requests2 = group_sessions - .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) - .await - .unwrap(); - - let event_count2: usize = requests2 - .iter() - .filter(|r| r.event_type == "m.room.encrypted".into()) - .map(|r| r.message_count()) - .sum(); - assert_eq!(event_count2, 0); + { + let requests = machine + .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .await + .unwrap(); + + let event_count: usize = requests + .iter() + .filter(|r| r.event_type == "m.room.encrypted".into()) + .map(|r| r.message_count()) + .sum(); + assert_eq!(event_count, 0); + } // Pretend that Bob wasn't able to decrypt, so he tries to unwedge - let mut session2 = bob_account.create_outbound_session_helper( - SessionConfig::default(), - account.identity_keys().curve25519, - *alice_otk2, - false, - ); - let content2 = session2.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap(); - - let to_device2 = - EncryptedToDeviceEvent::new(bob_id.to_owned(), content2.deserialize().unwrap()); + { + let (alice_otk_id, alice_otk) = alice_otks.next().unwrap(); + let mut session = bob_account + .create_outbound_session( + &alice_device, + &BTreeMap::from([(alice_otk_id.clone(), alice_otk.clone())]), + ) + .unwrap(); + let content = session.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap(); + + let to_device = + EncryptedToDeviceEvent::new(bob_id.to_owned(), content.deserialize().unwrap()); + + // Alice decrypts the unwedge message + let sync_changes = EncryptionSyncChanges { + to_device_events: vec![crate::utilities::json_convert(&to_device).unwrap()], + changed_devices: &Default::default(), + one_time_keys_counts: &Default::default(), + unused_fallback_keys: None, + next_batch_token: None, + }; + let (decrypted, _) = machine.receive_sync_changes(sync_changes).await.unwrap(); - // Alice decrypts the unwedge message - account.decrypt_to_device_event(&store, &to_device2).await.unwrap(); + assert_eq!(1, decrypted.len()); + } // When Alice shares the room key again, it should be re-shared with Bob - let requests3 = group_sessions - .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) - .await - .unwrap(); + { + let requests = machine + .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .await + .unwrap(); + + let event_count: usize = requests + .iter() + .filter(|r| r.event_type == "m.room.encrypted".into()) + .map(|r| r.message_count()) + .sum(); + assert_eq!(event_count, 1); + + let response = ToDeviceResponse::new(); + for request in requests { + machine.mark_request_as_sent(&request.txn_id, &response).await.unwrap(); + } + } - let event_count3: usize = requests3 - .iter() - .filter(|r| r.event_type == "m.room.encrypted".into()) - .map(|r| r.message_count()) - .sum(); - assert_eq!(event_count3, 1); + // When Alice shares the room key yet again, there shouldn't be any + // to-device events + { + let requests = machine + .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .await + .unwrap(); + + let event_count: usize = requests + .iter() + .filter(|r| r.event_type == "m.room.encrypted".into()) + .map(|r| r.message_count()) + .sum(); + assert_eq!(event_count, 0); + } } } From a1621a125086e70f9bad436454835356ba53f5bb Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 24 Jun 2024 20:46:46 -0400 Subject: [PATCH 04/11] fix typo, add comment --- crates/matrix-sdk-crypto/src/olm/account.rs | 2 +- .../matrix-sdk-crypto/src/session_manager/group_sessions.rs | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/olm/account.rs b/crates/matrix-sdk-crypto/src/olm/account.rs index d493a64c5f5..a90c5d112ca 100644 --- a/crates/matrix-sdk-crypto/src/olm/account.rs +++ b/crates/matrix-sdk-crypto/src/olm/account.rs @@ -1310,7 +1310,7 @@ impl Account { // Any new Olm session will bump the Olm wedging index for the // sender's device, if we have their device, which will cause // us to re-send existing Megolm sessions to them the next time - // we use the session. If we don't have there device, this + // we use the session. If we don't have their device, this // means that we haven't tried to send them any Megolm sessions // yet, so we don't need to worry about it. if let Some(device) = store.get_device_from_curve_key(sender, sender_key).await? { diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs index d31ad36e6d2..acbf871e5f4 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs @@ -800,6 +800,12 @@ impl GroupSessionManager { d.into_iter().filter(|d| match outbound.is_shared_with(d) { ShareState::NotShared => true, ShareState::Shared(_msg_index, olm_wedging_index) => { + // If the recipient device's Olm wedging index is + // higher than the value that we stored with the + // session, that means that they tried to unwedge the + // session since we last shared the room key. So we + // re-share it with them in case they weren't able to + // decrypt the room key the last time we shared it. olm_wedging_index < d.olm_wedging_index } _ => false, From acab0b21a0526b70f912ca3f0703ade53968f093 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 24 Jun 2024 21:01:03 -0400 Subject: [PATCH 05/11] add missing field in pattern --- crates/matrix-sdk-crypto/src/gossiping/machine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/gossiping/machine.rs b/crates/matrix-sdk-crypto/src/gossiping/machine.rs index 9691d71553f..505047593c0 100644 --- a/crates/matrix-sdk-crypto/src/gossiping/machine.rs +++ b/crates/matrix-sdk-crypto/src/gossiping/machine.rs @@ -642,7 +642,7 @@ impl GossipMachine { // information is recorded there. } else if let Some(outbound) = outbound_session { match outbound.is_shared_with(&device.inner) { - ShareState::Shared(message_index) => Ok(Some(message_index)), + ShareState::Shared(message_index, _) => Ok(Some(message_index)), ShareState::SharedButChangedSenderKey => Err(KeyForwardDecision::ChangedSenderKey), ShareState::NotShared => Err(KeyForwardDecision::OutboundSessionNotShared), } From 55f15471b325e212a6b6a4b56b87e9f3adc728f4 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 24 Jun 2024 21:12:14 -0400 Subject: [PATCH 06/11] lint --- .../matrix-sdk-crypto/src/identities/device.rs | 3 ++- .../src/session_manager/group_sessions.rs | 16 +++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/device.rs b/crates/matrix-sdk-crypto/src/identities/device.rs index 7e095f36803..9e5289aff6c 100644 --- a/crates/matrix-sdk-crypto/src/identities/device.rs +++ b/crates/matrix-sdk-crypto/src/identities/device.rs @@ -89,7 +89,8 @@ pub struct ReadOnlyDevice { /// Default to epoch for migration purpose. #[serde(default = "default_timestamp")] first_time_seen_ts: MilliSecondsSinceUnixEpoch, - /// The number of times the device has tried to unwedge Olm sessions with us. + /// The number of times the device has tried to unwedge Olm sessions with + /// us. #[serde(default)] pub(crate) olm_wedging_index: u32, } diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs index acbf871e5f4..1c0803de465 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs @@ -1434,9 +1434,7 @@ mod tests { #[async_test] async fn test_resend_session_after_unwedging() { let machine = OlmMachine::new(alice_id(), alice_device_id()).await; - assert_let!( - Ok(Some((txn_id, device_keys_request))) = dbg!(machine.upload_device_keys().await) - ); + assert_let!(Ok(Some((txn_id, device_keys_request))) = machine.upload_device_keys().await); let device_keys_response = upload_keys::v3::Response::new(BTreeMap::from([( DeviceKeyAlgorithm::SignedCurve25519, UInt::new(device_keys_request.one_time_keys.len() as u64).unwrap(), @@ -1446,7 +1444,7 @@ mod tests { let room_id = room_id!("!test:localhost"); let bob_id = user_id!("@bob:localhost"); - let bob_account = Account::new(&bob_id); + let bob_account = Account::new(bob_id); let keys_query_data = json!({ "device_keys": { "@bob:localhost": { @@ -1458,7 +1456,7 @@ mod tests { get_keys::v3::Response::try_from_http_response(response_from_file(&keys_query_data)) .unwrap(); let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &dbg!(keys_query)).await.unwrap(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); let alice_device_keys = device_keys_request.device_keys.unwrap().deserialize_as::().unwrap(); @@ -1495,7 +1493,7 @@ mod tests { // Alice shares the room key with Bob { let requests = machine - .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .share_room_key(room_id, [bob_id].into_iter(), EncryptionSettings::default()) .await .unwrap(); @@ -1517,7 +1515,7 @@ mod tests { // to-device events, since we already shared with Bob { let requests = machine - .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .share_room_key(room_id, [bob_id].into_iter(), EncryptionSettings::default()) .await .unwrap(); @@ -1559,7 +1557,7 @@ mod tests { // When Alice shares the room key again, it should be re-shared with Bob { let requests = machine - .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .share_room_key(room_id, [bob_id].into_iter(), EncryptionSettings::default()) .await .unwrap(); @@ -1580,7 +1578,7 @@ mod tests { // to-device events { let requests = machine - .share_room_key(&room_id, [bob_id].into_iter(), EncryptionSettings::default()) + .share_room_key(room_id, [bob_id].into_iter(), EncryptionSettings::default()) .await .unwrap(); From 0c4295dfca04ba6640de1bdeb3fc249e7e223395 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 26 Jun 2024 14:31:04 -0400 Subject: [PATCH 07/11] apply changes from review --- .../src/gossiping/machine.rs | 4 ++- .../src/identities/device.rs | 10 ++++--- crates/matrix-sdk-crypto/src/olm/account.rs | 15 +++++----- .../src/olm/group_sessions/outbound.rs | 30 +++++++++++-------- .../src/session_manager/group_sessions.rs | 14 ++++----- crates/matrix-sdk-crypto/src/store/caches.rs | 6 ++-- 6 files changed, 46 insertions(+), 33 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/gossiping/machine.rs b/crates/matrix-sdk-crypto/src/gossiping/machine.rs index 505047593c0..eda9d58be86 100644 --- a/crates/matrix-sdk-crypto/src/gossiping/machine.rs +++ b/crates/matrix-sdk-crypto/src/gossiping/machine.rs @@ -642,7 +642,9 @@ impl GossipMachine { // information is recorded there. } else if let Some(outbound) = outbound_session { match outbound.is_shared_with(&device.inner) { - ShareState::Shared(message_index, _) => Ok(Some(message_index)), + ShareState::Shared { message_index, olm_wedging_index: _ } => { + Ok(Some(message_index)) + } ShareState::SharedButChangedSenderKey => Err(KeyForwardDecision::ChangedSenderKey), ShareState::NotShared => Err(KeyForwardDecision::OutboundSessionNotShared), } diff --git a/crates/matrix-sdk-crypto/src/identities/device.rs b/crates/matrix-sdk-crypto/src/identities/device.rs index 9e5289aff6c..58e2840a89b 100644 --- a/crates/matrix-sdk-crypto/src/identities/device.rs +++ b/crates/matrix-sdk-crypto/src/identities/device.rs @@ -43,7 +43,9 @@ use crate::{ olm::{ InboundGroupSession, OutboundGroupSession, Session, ShareInfo, SignedJsonObject, VerifyJson, }, - store::{Changes, CryptoStoreWrapper, DeviceChanges, Result as StoreResult}, + store::{ + caches::SequenceNumber, Changes, CryptoStoreWrapper, DeviceChanges, Result as StoreResult, + }, types::{ events::{ forwarded_room_key::ForwardedRoomKeyContent, @@ -92,7 +94,7 @@ pub struct ReadOnlyDevice { /// The number of times the device has tried to unwedge Olm sessions with /// us. #[serde(default)] - pub(crate) olm_wedging_index: u32, + pub(crate) olm_wedging_index: SequenceNumber, } fn default_timestamp() -> MilliSecondsSinceUnixEpoch { @@ -584,7 +586,7 @@ impl ReadOnlyDevice { deleted: Arc::new(AtomicBool::new(false)), withheld_code_sent: Arc::new(AtomicBool::new(false)), first_time_seen_ts: MilliSecondsSinceUnixEpoch::now(), - olm_wedging_index: 0, + olm_wedging_index: Default::default(), } } @@ -979,7 +981,7 @@ impl TryFrom<&DeviceKeys> for ReadOnlyDevice { trust_state: Arc::new(RwLock::new(LocalTrust::Unset)), withheld_code_sent: Arc::new(AtomicBool::new(false)), first_time_seen_ts: MilliSecondsSinceUnixEpoch::now(), - olm_wedging_index: 0, + olm_wedging_index: Default::default(), }; device.verify_device_keys(device_keys)?; diff --git a/crates/matrix-sdk-crypto/src/olm/account.rs b/crates/matrix-sdk-crypto/src/olm/account.rs index a90c5d112ca..697cee6e6f3 100644 --- a/crates/matrix-sdk-crypto/src/olm/account.rs +++ b/crates/matrix-sdk-crypto/src/olm/account.rs @@ -1307,19 +1307,20 @@ impl Account { // it to the cache but don't store it. let mut changes = Changes { sessions: vec![result.session.clone()], ..Default::default() }; + // Any new Olm session will bump the Olm wedging index for the - // sender's device, if we have their device, which will cause - // us to re-send existing Megolm sessions to them the next time - // we use the session. If we don't have their device, this - // means that we haven't tried to send them any Megolm sessions - // yet, so we don't need to worry about it. + // sender's device, if we have their device, which will cause us + // to re-send existing Megolm sessions to them the next time we + // use the session. If we don't have their device, this means + // that we haven't tried to send them any Megolm sessions yet, + // so we don't need to worry about it. if let Some(device) = store.get_device_from_curve_key(sender, sender_key).await? { let mut read_only_device = device.inner; - read_only_device.olm_wedging_index += 1; - debug!(read_only_device.olm_wedging_index, "Olm wedging index"); + read_only_device.olm_wedging_index.increment(); changes.devices = DeviceChanges { changed: vec![read_only_device], ..Default::default() }; } + store.save_changes(changes).await?; Ok((SessionType::New(result.session), result.plaintext)) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs index 4465dacf215..455f95ef5f7 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs @@ -46,6 +46,7 @@ use super::SessionCreationError; #[cfg(feature = "experimental-algorithms")] use crate::types::events::room::encrypted::MegolmV2AesSha2Content; use crate::{ + store::caches::SequenceNumber, types::{ events::{ room::encrypted::{ @@ -69,7 +70,7 @@ const ROTATION_MESSAGES: u64 = 100; pub(crate) enum ShareState { NotShared, SharedButChangedSenderKey, - Shared(u32, u32), + Shared { message_index: u32, olm_wedging_index: SequenceNumber }, } /// Settings for an encrypted room. @@ -171,7 +172,7 @@ impl ShareInfo { pub fn new_shared( sender_key: Curve25519PublicKey, message_index: u32, - olm_wedging_index: u32, + olm_wedging_index: SequenceNumber, ) -> Self { ShareInfo::Shared(SharedWith { sender_key, message_index, olm_wedging_index }) } @@ -190,7 +191,7 @@ pub struct SharedWith { pub message_index: u32, /// The Olm wedging index of the device at the time the session was shared. #[serde(default)] - pub olm_wedging_index: u32, + pub olm_wedging_index: SequenceNumber, } impl OutboundGroupSession { @@ -533,7 +534,10 @@ impl OutboundGroupSession { d.get(device.device_id()).map(|s| match s { ShareInfo::Shared(s) => { if device.curve25519_key() == Some(s.sender_key) { - ShareState::Shared(s.message_index, s.olm_wedging_index) + ShareState::Shared { + message_index: s.message_index, + olm_wedging_index: s.olm_wedging_index, + } } else { ShareState::SharedButChangedSenderKey } @@ -557,7 +561,10 @@ impl OutboundGroupSession { Some(match info { ShareInfo::Shared(info) => { if device.curve25519_key() == Some(info.sender_key) { - ShareState::Shared(info.message_index, info.olm_wedging_index) + ShareState::Shared { + message_index: info.message_index, + olm_wedging_index: info.olm_wedging_index, + } } else { ShareState::SharedButChangedSenderKey } @@ -604,12 +611,10 @@ impl OutboundGroupSession { sender_key: Curve25519PublicKey, index: u32, ) { - self.shared_with_set - .write() - .unwrap() - .entry(user_id.to_owned()) - .or_default() - .insert(device_id.to_owned(), ShareInfo::new_shared(sender_key, index, 0)); + self.shared_with_set.write().unwrap().entry(user_id.to_owned()).or_default().insert( + device_id.to_owned(), + ShareInfo::new_shared(sender_key, index, Default::default()), + ); } /// Mark the session as shared with the given user/device pair, starting @@ -621,7 +626,8 @@ impl OutboundGroupSession { device_id: &DeviceId, sender_key: Curve25519PublicKey, ) { - let share_info = ShareInfo::new_shared(sender_key, self.message_index().await, 0); + let share_info = + ShareInfo::new_shared(sender_key, self.message_index().await, Default::default()); self.shared_with_set .write() .unwrap() diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs index 1c0803de465..28ced96cd8f 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions.rs @@ -799,13 +799,13 @@ impl GroupSessionManager { .flat_map(|(_, d)| { d.into_iter().filter(|d| match outbound.is_shared_with(d) { ShareState::NotShared => true, - ShareState::Shared(_msg_index, olm_wedging_index) => { - // If the recipient device's Olm wedging index is - // higher than the value that we stored with the - // session, that means that they tried to unwedge the - // session since we last shared the room key. So we - // re-share it with them in case they weren't able to - // decrypt the room key the last time we shared it. + ShareState::Shared { message_index: _, olm_wedging_index } => { + // If the recipient device's Olm wedging index is higher + // than the value that we stored with the session, that + // means that they tried to unwedge the session since we + // last shared the room key. So we re-share it with + // them in case they weren't able to decrypt the room + // key the last time we shared it. olm_wedging_index < d.olm_wedging_index } _ => false, diff --git a/crates/matrix-sdk-crypto/src/store/caches.rs b/crates/matrix-sdk-crypto/src/store/caches.rs index e1a85805ba8..db6254c3d06 100644 --- a/crates/matrix-sdk-crypto/src/store/caches.rs +++ b/crates/matrix-sdk-crypto/src/store/caches.rs @@ -27,6 +27,7 @@ use std::{ }; use ruma::{DeviceId, OwnedDeviceId, OwnedRoomId, OwnedUserId, RoomId, UserId}; +use serde::{Deserialize, Serialize}; use tokio::sync::Mutex; use tracing::{field::display, instrument, trace, Span}; @@ -197,7 +198,8 @@ impl DeviceStore { /// subtraction. For example, suppose we've just overflowed from i64::MAX to /// i64::MIN. (i64::MAX.wrapping_sub(i64::MIN)) is -1, which tells us that /// i64::MAX comes before i64::MIN in the sequence. -#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Deserialize, Serialize)] +#[serde(transparent)] pub(crate) struct SequenceNumber(i64); impl Display for SequenceNumber { @@ -219,7 +221,7 @@ impl Ord for SequenceNumber { } impl SequenceNumber { - fn increment(&mut self) { + pub(crate) fn increment(&mut self) { self.0 = self.0.wrapping_add(1) } From b102bad10a3cd70e22623b74e59aa42bf4b7c220 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 26 Jun 2024 15:30:35 -0400 Subject: [PATCH 08/11] make SequenceNumber pub --- crates/matrix-sdk-crypto/src/store/caches.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/store/caches.rs b/crates/matrix-sdk-crypto/src/store/caches.rs index db6254c3d06..59397529b50 100644 --- a/crates/matrix-sdk-crypto/src/store/caches.rs +++ b/crates/matrix-sdk-crypto/src/store/caches.rs @@ -200,7 +200,7 @@ impl DeviceStore { /// i64::MAX comes before i64::MIN in the sequence. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, Deserialize, Serialize)] #[serde(transparent)] -pub(crate) struct SequenceNumber(i64); +pub struct SequenceNumber(i64); impl Display for SequenceNumber { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { From 41dde2b17e428405df6040c686189df57ca94f99 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Wed, 26 Jun 2024 15:44:54 -0400 Subject: [PATCH 09/11] add a bit more whitespace --- crates/matrix-sdk-crypto/src/olm/account.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/matrix-sdk-crypto/src/olm/account.rs b/crates/matrix-sdk-crypto/src/olm/account.rs index 1e556f9208f..87c555a78c1 100644 --- a/crates/matrix-sdk-crypto/src/olm/account.rs +++ b/crates/matrix-sdk-crypto/src/olm/account.rs @@ -1321,6 +1321,7 @@ impl Account { if let Some(device) = store.get_device_from_curve_key(sender, sender_key).await? { let mut read_only_device = device.inner; read_only_device.olm_wedging_index.increment(); + changes.devices = DeviceChanges { changed: vec![read_only_device], ..Default::default() }; } From 9239cdeeeb2f7e3c22d409e16fa07ab8d4ec5db2 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 27 Jun 2024 16:47:11 -0400 Subject: [PATCH 10/11] add documentation --- .../matrix-sdk-crypto/src/olm/group_sessions/outbound.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs index ae5f302b919..6e8fe3a81aa 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs @@ -68,9 +68,18 @@ const ROTATION_PERIOD: Duration = ONE_WEEK; const ROTATION_MESSAGES: u64 = 100; #[derive(Debug, Clone, Copy, PartialEq, Eq)] +/// Information about whether a session was shared with a device. pub(crate) enum ShareState { + /// The session was not shared with the device. NotShared, + /// The session was shared with the device with the given device ID, but + /// with a different curve25519 key. SharedButChangedSenderKey, + /// The session was shared with the device, at the given message index. The + /// `olm_wedging_index` is the value of the `olm_wedging_index` from the + /// `ReadOnlyDevice` at the time that we last shared the session with the + /// device, and indicates whether we need to re-share the session with the + /// device. Shared { message_index: u32, olm_wedging_index: SequenceNumber }, } From 7273f28191e9c3db8c338d846279060965e14e0c Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 27 Jun 2024 16:56:31 -0400 Subject: [PATCH 11/11] make it work with changes from #3517 --- .../matrix-sdk-crypto/src/session_manager/group_sessions/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/mod.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/mod.rs index d0dd7ab0688..30ada4a82b8 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/mod.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/mod.rs @@ -1349,6 +1349,7 @@ mod tests { .create_outbound_session( &alice_device, &BTreeMap::from([(alice_otk_id.clone(), alice_otk.clone())]), + bob_account.device_keys(), ) .unwrap(); let content = session.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap(); @@ -1413,6 +1414,7 @@ mod tests { .create_outbound_session( &alice_device, &BTreeMap::from([(alice_otk_id.clone(), alice_otk.clone())]), + bob_account.device_keys(), ) .unwrap(); let content = session.encrypt(&alice_device, "m.dummy", json!({}), None).await.unwrap();