From 5d3f9ed61cecd369bbe60463a1620f9fefbcd27c Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 14 Jun 2024 17:09:16 +0100 Subject: [PATCH 01/27] crypto: Calculate sender data for incoming sessions --- .../src/gossiping/machine.rs | 4 +- crates/matrix-sdk-crypto/src/machine.rs | 40 +- crates/matrix-sdk-crypto/src/olm/account.rs | 13 +- .../src/olm/group_sessions/mod.rs | 2 + .../src/olm/group_sessions/outbound.rs | 11 +- .../olm/group_sessions/sender_data_finder.rs | 891 ++++++++++++++++++ crates/matrix-sdk-crypto/src/olm/mod.rs | 2 +- .../src/session_manager/group_sessions/mod.rs | 37 +- 8 files changed, 976 insertions(+), 24 deletions(-) create mode 100644 crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs diff --git a/crates/matrix-sdk-crypto/src/gossiping/machine.rs b/crates/matrix-sdk-crypto/src/gossiping/machine.rs index 72d1da34550..0702dff0331 100644 --- a/crates/matrix-sdk-crypto/src/gossiping/machine.rs +++ b/crates/matrix-sdk-crypto/src/gossiping/machine.rs @@ -1218,6 +1218,8 @@ mod tests { create_sessions: bool, algorithm: EventEncryptionAlgorithm, ) -> (GossipMachine, OutboundGroupSession, GossipMachine) { + use crate::olm::SenderData; + let alice_machine = get_machine_test_helper().await; let alice_device = ReadOnlyDevice::from_account( &alice_machine.inner.store.cache().await.unwrap().account().await.unwrap(), @@ -1270,7 +1272,7 @@ mod tests { .inner .store .static_account() - .create_group_session_pair(room_id(), settings) + .create_group_session_pair(room_id(), settings, SenderData::unknown()) .await .unwrap(); diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 95822e77454..3e98ae462b3 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -66,7 +66,8 @@ use crate::{ identities::{user::UserIdentities, Device, IdentityManager, UserDevices}, olm::{ Account, CrossSigningStatus, EncryptionSettings, IdentityKeys, InboundGroupSession, - OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, SessionType, StaticAccountData, + OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, SenderDataFinder, SessionType, + StaticAccountData, }, requests::{IncomingResponse, OutgoingRequest, UploadSigningKeysRequest}, session_manager::{GroupSessionManager, SessionManager}, @@ -816,7 +817,7 @@ impl OlmMachine { event: &DecryptedRoomKeyEvent, content: &MegolmV1AesSha2Content, ) -> OlmResult> { - let sender_data = SenderData::unknown(); + let sender_data = SenderDataFinder::find_using_event(self, sender_key, event).await?; let session = InboundGroupSession::new( sender_key, @@ -900,7 +901,11 @@ impl OlmMachine { let (_, session) = self .inner .group_session_manager - .create_outbound_group_session(room_id, EncryptionSettings::default()) + .create_outbound_group_session( + room_id, + EncryptionSettings::default(), + SenderData::unknown(), + ) .await?; self.store().save_inbound_group_sessions(&[session]).await?; @@ -917,7 +922,11 @@ impl OlmMachine { let (_, session) = self .inner .group_session_manager - .create_outbound_group_session(room_id, EncryptionSettings::default()) + .create_outbound_group_session( + room_id, + EncryptionSettings::default(), + SenderData::unknown(), + ) .await?; Ok(session) @@ -1016,7 +1025,26 @@ impl OlmMachine { users: impl Iterator, encryption_settings: impl Into, ) -> OlmResult>> { - self.inner.group_session_manager.share_room_key(room_id, users, encryption_settings).await + // Use our own device info to populate the SenderData that validates the + // InboundGroupSession that we create as a pair to the OutboundGroupSession we + // are sending out. + let account = self.store().static_account(); + let device = self.store().get_device(account.user_id(), account.device_id()).await; + let own_sender_data = match device { + Ok(Some(device)) => { + SenderDataFinder::find_using_device_keys(self, device.as_device_keys().clone()) + .await? + } + _ => { + error!("Unable to find our own device!"); + SenderData::unknown() + } + }; + + self.inner + .group_session_manager + .share_room_key(room_id, users, encryption_settings, own_sender_data) + .await } /// Receive an unencrypted verification event. @@ -4169,7 +4197,7 @@ pub(crate) mod tests { let (outbound, mut inbound) = alice .store() .static_account() - .create_group_session_pair(room_id, Default::default()) + .create_group_session_pair(room_id, Default::default(), SenderData::unknown()) .await .unwrap(); diff --git a/crates/matrix-sdk-crypto/src/olm/account.rs b/crates/matrix-sdk-crypto/src/olm/account.rs index eae5bcea90f..8dd556d5b31 100644 --- a/crates/matrix-sdk-crypto/src/olm/account.rs +++ b/crates/matrix-sdk-crypto/src/olm/account.rs @@ -198,6 +198,7 @@ impl StaticAccountData { &self, room_id: &RoomId, settings: EncryptionSettings, + own_sender_data: SenderData, ) -> Result<(OutboundGroupSession, InboundGroupSession), MegolmSessionCreationError> { trace!(?room_id, algorithm = settings.algorithm.as_str(), "Creating a new room key"); @@ -221,7 +222,7 @@ impl StaticAccountData { signing_key, room_id, &outbound.session_key().await, - SenderData::unknown(), + own_sender_data, algorithm, Some(visibility), )?; @@ -237,9 +238,13 @@ impl StaticAccountData { &self, room_id: &RoomId, ) -> (OutboundGroupSession, InboundGroupSession) { - self.create_group_session_pair(room_id, EncryptionSettings::default()) - .await - .expect("Can't create default group session pair") + self.create_group_session_pair( + room_id, + EncryptionSettings::default(), + SenderData::unknown(), + ) + .await + .expect("Can't create default group session pair") } /// Get the key ID of our Ed25519 signing key. 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 cf1c5255357..8663eaee6ee 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/mod.rs @@ -18,6 +18,7 @@ use serde::{Deserialize, Serialize}; mod inbound; mod outbound; mod sender_data; +mod sender_data_finder; pub use inbound::{InboundGroupSession, PickledInboundGroupSession}; pub(crate) use outbound::ShareState; @@ -25,6 +26,7 @@ pub use outbound::{ EncryptionSettings, OutboundGroupSession, PickledOutboundGroupSession, ShareInfo, }; pub use sender_data::{SenderData, SenderDataRetryDetails}; +pub(crate) use sender_data_finder::SenderDataFinder; use thiserror::Error; pub use vodozemac::megolm::{ExportedSessionKey, SessionKey}; use vodozemac::{megolm::SessionKeyDecodeError, Curve25519PublicKey}; 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 6e8fe3a81aa..f5616541555 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs @@ -811,7 +811,10 @@ mod tests { user_id, SecondsSinceUnixEpoch, }; - use crate::{olm::OutboundGroupSession, Account, EncryptionSettings, MegolmError}; + use crate::{ + olm::{OutboundGroupSession, SenderData}, + Account, EncryptionSettings, MegolmError, + }; const TWO_HOURS: Duration = Duration::from_secs(60 * 60 * 2); @@ -999,7 +1002,11 @@ mod tests { Account::with_device_id(user_id!("@alice:example.org"), device_id!("DEVICEID")) .static_data; let (session, _) = account - .create_group_session_pair(room_id!("!test_room:example.org"), settings) + .create_group_session_pair( + room_id!("!test_room:example.org"), + settings, + SenderData::unknown(), + ) .await .unwrap(); session diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs new file mode 100644 index 00000000000..ac8bb55e7c3 --- /dev/null +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -0,0 +1,891 @@ +// Copyright 2024 The Matrix.org Foundation C.I.C. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::ops::Deref; + +use async_trait::async_trait; +use ruma::UserId; +use tracing::error; +use vodozemac::Curve25519PublicKey; + +use super::{SenderData, SenderDataRetryDetails}; +use crate::{ + error::OlmResult, + identities::Device, + store::{self, Store}, + types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys, MasterPubkey}, + EventError, OlmError, OlmMachine, ReadOnlyDevice, ReadOnlyOwnUserIdentity, + ReadOnlyUserIdentities, +}; + +/// Temporary struct that is used to look up [`SenderData`] based on the +/// information supplied with in [`InboundGroupSession`]. +/// +/// The letters A, B etc. in the documentation refer to the algorithm described +/// in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 +pub(crate) struct SenderDataFinder<'a, S: FinderCryptoStore> { + own_crypto_store: &'a S, + own_user_id: &'a UserId, +} + +impl<'a> SenderDataFinder<'a, Store> { + /// As described in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 + /// and https://github.com/matrix-org/matrix-rust-sdk/issues/3544 + /// find the device info associated with the to-device message used to + /// create the InboundGroupSession we are about to create, and decide + /// whether we trust the sender. + pub(crate) async fn find_using_event( + own_olm_machine: &'a OlmMachine, + sender_curve_key: Curve25519PublicKey, + room_key_event: &'a DecryptedRoomKeyEvent, + ) -> OlmResult { + let finder = Self { + own_crypto_store: own_olm_machine.store(), + own_user_id: own_olm_machine.user_id(), + }; + finder.have_event(sender_curve_key, room_key_event).await + } + + /// As described in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 + /// and https://github.com/matrix-org/matrix-rust-sdk/issues/3544 + /// use the supplied device info to decide whether we trust the sender. + pub(crate) async fn find_using_device_keys( + own_olm_machine: &'a OlmMachine, + device_keys: DeviceKeys, + ) -> OlmResult { + let finder = Self { + own_crypto_store: own_olm_machine.store(), + own_user_id: own_olm_machine.user_id(), + }; + finder.have_device_keys(&device_keys).await + } +} + +impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { + async fn have_event( + &self, + sender_curve_key: Curve25519PublicKey, + room_key_event: &'a DecryptedRoomKeyEvent, + ) -> OlmResult { + // A (start) + // + // TODO: take the session lock for session_id + // TODO: if we fail to get the lock, we bail out immediately. Does this open an + // attack vector for someone to use someone else's session ID and send + // invalid sessions? + // + // Does the to-device message contain the device_keys property from MSC4147? + if let Some(sender_device_keys) = &room_key_event.device_keys { + // Yes: use the device info to continue + self.have_device_keys(sender_device_keys).await + } else { + // No: look for the device in the store + self.search_for_device(sender_curve_key, room_key_event).await + } + } + + async fn search_for_device( + &self, + sender_curve_key: Curve25519PublicKey, + room_key_event: &'a DecryptedRoomKeyEvent, + ) -> OlmResult { + // B (no device info in to-device message) + // + // Does the locally-cached (in the store) devices list contain a device with the + // curve key of the sender of the to-device message? + if let Some(sender_device) = self + .own_crypto_store + .get_device_from_curve_key(&room_key_event.sender, sender_curve_key) + .await? + { + // Yes: use the device info to continue + self.have_device(sender_device.inner).await + } else { + // C (no device info locally) + // + // We have no device data for this session so we can't continue in the "fast + // lane" (blocking sync). + let sender_data = SenderData::UnknownDevice { + retry_details: SenderDataRetryDetails::retry_soon(), + // This is not a legacy session since we did attempt to look + // up its sender data at the time of reception. + // legacy_session: false, + // TODO: we set legacy to true for now, since our implementation is incomplete, so + // we may not have had a proper chance to look up the sender data. + legacy_session: true, + }; + // sender_data will be persisted to the store when this function returns to + // `handle_key`, meaning that if our process is killed, we will still retry it + // later. + // + // Switch to the "slow lane" (don't block sync, but retry after /keys/query + // returns). + // + // TODO: kick off an async task [keep the lock]: run + // OlmMachine::get_user_devices (which waits for /keys/query to complete, then + // repeats the lookup we did above for the device that matches this session. + // + // If the device is there, -> D + // + // If we still don’t have the device info, -> Wait to see whether we get device + // info later. Increment retry_count and set next_retry_time_ms per backoff + // algorithm; let the background job pick it up [drop the lock] + Ok(sender_data) + } + } + + async fn have_device_keys(&self, sender_device_keys: &DeviceKeys) -> OlmResult { + // Validate the signature of the DeviceKeys supplied. + if let Ok(sender_device) = ReadOnlyDevice::try_from(sender_device_keys) { + self.have_device(sender_device).await + } else { + // The device keys supplied did not validate. + // TODO: log an error + // TODO: Err(OlmError::SessionCreation(SessionCreationError::InvalidDeviceKeys)) + Err(OlmError::EventError(EventError::UnsupportedAlgorithm)) + } + } + + /// Step D from https://github.com/matrix-org/matrix-rust-sdk/issues/3543 + /// We have device info for the sender of this to-device message. Look up + /// whether it's cross-signed. + async fn have_device(&self, sender_device: ReadOnlyDevice) -> OlmResult { + // D (we have device info) + // + // Is the device info cross-signed? + + let user_id = sender_device.user_id(); + let Some(signatures) = sender_device.signatures().get(user_id) else { + // This should never happen: we would not have managed to get a ReadOnlyDevice + // if it did not contain a signature. + error!( + "Found a device for user_id {user_id} but it has no signatures for that user id!" + ); + + // Return the same result as if the device is not cross-signed. + return Ok(SenderData::unknown()); + }; + + // Count number of signatures - we know there is 1, because we would not have + // been able to construct a ReadOnlyDevice without a signature. + // If there are more than 1, we assume this device was cross-signed by some + // identity. + if signatures.len() > 1 { + // Yes, the device info is cross-signed + self.device_is_cross_signed(sender_device).await + } else { + // No, the device info is not cross-signed. + // Wait to see whether the device becomes cross-signed later. Drop + // out of both the "fast lane" and the "slow lane" and let the + // background retry task try this later. + // + // We will need new, cross-signed device info for this to work, so there is no + // point storing the device info we have in the session. + Ok(SenderData::unknown()) + + // TODO: Wait to see if the device becomes cross-signed soon. + // Increment retry_count and set next_retry_time_ms per + // backoff algorithm; let the background job pick it up [drop + // the lock] + } + } + + async fn device_is_cross_signed(&self, sender_device: ReadOnlyDevice) -> OlmResult { + // E (we have cross-signed device info) + // + // Do we have the cross-signing key for this user? + + let sender_user_id = sender_device.user_id(); + let sender_user_identity = self.own_crypto_store.get_user_identity(sender_user_id).await?; + + if let Some(sender_user_identity) = sender_user_identity { + // Yes: check the device is signed by the identity + self.have_user_cross_signing_keys(sender_device, sender_user_identity).await + } else { + // No: F (we have cross-signed device info, but no cross-signing keys) + + // TODO: bump the retry count + time + + Ok(SenderData::DeviceInfo { + device_keys: sender_device.as_device_keys().clone(), + retry_details: SenderDataRetryDetails::retry_soon(), + legacy_session: true, // TODO: change to false when we have all the retry code + }) + + // TODO: Return, and kick off an async task + // [keep the lock]: run OlmMachine::get_identity (which waits for + // /keys/query to complete, then fetches this user's cross-signing + // key from the store.) If we still don’t have a cross-signing key + // -> Wait to see if we get one soon. Do nothing; let the + // background job pick it up [drop the lock] + } + } + + async fn have_user_cross_signing_keys( + &self, + sender_device: ReadOnlyDevice, + sender_user_identity: ReadOnlyUserIdentities, + ) -> OlmResult { + // G (we have cross-signing key) + // + // Does the cross-signing key match that used to sign the device info? + // And is the signature in the device info valid? + let maybe_msk_info = + self.msk_if_device_is_signed_by_user(&sender_device, &sender_user_identity).await; + + if let Some((msk, msk_verified)) = maybe_msk_info { + // Yes: H (cross-signing key matches that used to sign the device info!) + // and: J (device info is verified by matching cross-signing key) + + // Find the actual key within the MasterPubkey struct + if let Some(msk) = msk.get_first_key() { + // We have MXID and MSK for the user sending the to-device message. + // Decide the MSK trust level based on whether we have verified this user. + // Set the MXID, MSK and trust level in the session. Remove the device + // info and retries since we don't need them. + Ok(SenderData::SenderKnown { + user_id: sender_device.user_id().to_owned(), + msk, + msk_verified, + }) + // TODO: [drop the lock] + } else { + // Surprisingly, there was no key in the MasterPubkey. We did not expect this: + // treat it as if the device was not signed by this master key. + // + tracing::error!( + "MasterPubkey for user {} does not contain any keys!", + msk.user_id() + ); + + Ok(SenderData::DeviceInfo { + device_keys: sender_device.as_device_keys().clone(), + retry_details: SenderDataRetryDetails::retry_soon(), + legacy_session: true, // TODO: change to false when retries etc. are done + }) + // TODO: [drop the lock] + } + } else { + // No: Device was not signed by the known identity of the sender. + // (Or the signature was invalid. We don't know which unfortunately, so we can't + // bail out completely if the signature was invalid, as we'd like + // to.) + // + // Since we've already checked there are >1 signatures, we guess + // it was signed by a different identity of this user, so we should retry + // later, in case either the device info or the user's identity changes. + Ok(SenderData::DeviceInfo { + device_keys: sender_device.as_device_keys().clone(), + retry_details: SenderDataRetryDetails::retry_soon(), + legacy_session: true, // TODO: change to false when retries etc. are done + }) + + // TODO: Increment retry_count and set + // next_retry_time_ms per backoff algorithm; let the background job + // pick it up [drop the lock] + } + } + + /// If sender_device is correctly cross-signed by user_identity, return + /// user_identity's master key and whether it is verified. + /// Otherwise, return None. + async fn msk_if_device_is_signed_by_user<'i>( + &self, + sender_device: &'_ ReadOnlyDevice, + sender_user_identity: &'i ReadOnlyUserIdentities, + ) -> Option<(&'i MasterPubkey, bool)> { + match sender_user_identity { + ReadOnlyUserIdentities::Own(own_identity) => { + if own_identity.is_device_signed(sender_device).is_ok() { + Some((own_identity.master_key(), own_identity.is_verified())) + } else { + None + } + } + ReadOnlyUserIdentities::Other(other_identity) => { + if other_identity.is_device_signed(sender_device).is_err() { + return None; + } + + let msk = other_identity.master_key(); + + // Use our own identity to determine whether this other identity is signed + let msk_verified = if let Some(own_identity) = self.own_identity().await { + own_identity.is_identity_signed(other_identity).is_ok() + } else { + // Couldn't get own identity! Assume msk is not verified. + false + }; + + Some((msk, msk_verified)) + } + } + } + + /// Return the user identity of the current user, or None if we failed to + /// find it (which is unexpected) + async fn own_identity(&self) -> Option { + let own_identity = + self.own_crypto_store.get_user_identity(self.own_user_id).await.ok()??; + + let ReadOnlyUserIdentities::Own(own_identity) = own_identity else { + panic!("The user identity for our own user ID was not an own identity!"); + }; + + Some(own_identity) + } +} + +#[cfg_attr(target_arch = "wasm32", async_trait(?Send))] +#[cfg_attr(not(target_arch = "wasm32"), async_trait)] +pub(crate) trait FinderCryptoStore { + async fn get_device_from_curve_key( + &self, + user_id: &UserId, + curve_key: Curve25519PublicKey, + ) -> store::Result>; + + async fn get_user_identity( + &self, + user_id: &UserId, + ) -> OlmResult>; +} + +#[cfg_attr(target_arch = "wasm32", async_trait(?Send))] +#[cfg_attr(not(target_arch = "wasm32"), async_trait)] +impl FinderCryptoStore for Store { + async fn get_device_from_curve_key( + &self, + user_id: &UserId, + curve_key: Curve25519PublicKey, + ) -> store::Result> { + self.get_device_from_curve_key(user_id, curve_key).await + } + + async fn get_user_identity( + &self, + user_id: &UserId, + ) -> OlmResult> { + Ok(self.deref().get_user_identity(user_id).await?) + } +} + +#[cfg(test)] +mod tests { + use std::{ops::Deref as _, sync::Arc}; + + use assert_matches2::assert_let; + use async_trait::async_trait; + use matrix_sdk_test::async_test; + use ruma::{device_id, owned_room_id, user_id, UserId}; + use tokio::sync::Mutex; + use vodozemac::{megolm::SessionKey, Curve25519PublicKey, Ed25519PublicKey}; + + use super::{FinderCryptoStore, SenderDataFinder}; + use crate::{ + error::OlmResult, + olm::{PrivateCrossSigningIdentity, SenderData}, + store::{self, CryptoStoreWrapper, MemoryStore}, + types::events::{ + olm_v1::DecryptedRoomKeyEvent, + room_key::{MegolmV1AesSha2Content, RoomKeyContent}, + }, + verification::VerificationMachine, + Account, Device, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, + ReadOnlyUserIdentity, + }; + + #[async_test] + async fn test_providing_no_device_data_returns_sender_data_with_no_device_info() { + // Given that the crypto store is empty and the initial event has no device info + let room_key_content = room_key_content(); + let room_key_event = room_key_event(None, &room_key_content); + let own_crypto_store = FakeCryptoStore::empty(); + + // When we try to find sender data + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back no useful information at all + assert_let!(SenderData::UnknownDevice { retry_details, legacy_session } = sender_data); + assert_eq!(retry_details.retry_count, 0); + + // TODO: This should not be marked as a legacy session, but for now it is + // because we haven't finished implementing the whole sender_data and + // retry mechanism. + assert!(legacy_session); + } + + #[async_test] + async fn test_if_the_todevice_event_contains_device_info_it_is_captured() { + // Given an account and user identity + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event containing device info + let room_key_content = room_key_content(); + let room_key_event = room_key_event(Some(&device), &room_key_content); + + // When we try to find sender data + let own_crypto_store = FakeCryptoStore::empty(); + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the device keys that were in the store + assert_let!( + SenderData::DeviceInfo { device_keys, retry_details, legacy_session } = sender_data + ); + assert_eq!(&device_keys, device.as_device_keys()); + assert_eq!(retry_details.retry_count, 0); + + // TODO: This should not be marked as a legacy session, but for now it is + // because we haven't finished implementing the whole sender_data and + // retry mechanism. + assert!(legacy_session); + } + + #[async_test] + async fn test_picks_up_device_info_from_the_store_if_missing_from_the_todevice_event() { + // Given an account and user identity + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(None, &room_key_content); + + // When we try to find sender data (but the user identity can't be found in the + // store) + let own_crypto_store = FakeCryptoStore::device_only(device.clone()); + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the device keys that were in the store + assert_let!( + SenderData::DeviceInfo { device_keys, retry_details, legacy_session } = sender_data + ); + assert_eq!(&device_keys, device.as_device_keys()); + assert_eq!(retry_details.retry_count, 0); + + // TODO: This should not be marked as a legacy session, but for now it is + // because we haven't finished implementing the whole sender_data and + // retry mechanism. + assert!(legacy_session); + } + + #[async_test] + async fn test_does_not_add_sender_data_if_device_is_not_signed() { + // Given an account and user identity + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; + let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); + + // And a device (not signed) + let device = create_unsigned_device(&account); + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(None, &room_key_content); + + // When we try to find sender data + let own_crypto_store = FakeCryptoStore::device_and_user(device, user_identities); + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we treat it as if there is no device info at all + assert_let!(SenderData::UnknownDevice { retry_details, legacy_session } = sender_data); + assert_eq!(retry_details.retry_count, 0); + + // TODO: This should not be marked as a legacy session, but for now it is + // because we haven't finished implementing the whole sender_data and + // retry mechanism. + assert!(legacy_session); + } + + #[async_test] + async fn test_adds_sender_data_for_own_verified_device_and_user_using_device_from_store() { + // Given an account and user identity + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; + let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(None, &room_key_content); + + // When we try to find sender data + let own_crypto_store = FakeCryptoStore::device_and_user(device, user_identities); + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the information about the sender + assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_eq!(user_id, account.user_id()); + assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + assert!(!msk_verified); + } + + #[async_test] + async fn test_adds_sender_data_for_other_verified_device_and_user_using_device_from_store() { + // Given an account, user identity, and a separate identity to be our own + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let user_identity = ReadOnlyUserIdentity::from_private(&private_identity).await; + let user_identities = ReadOnlyUserIdentities::Other(user_identity.clone()); + let own_private_identity = create_private_identity(&account).await; + let own_user_identity = ReadOnlyOwnUserIdentity::from_private(&own_private_identity).await; + let own_user_identities = ReadOnlyUserIdentities::Own(own_user_identity.clone()); + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(None, &room_key_content); + + // When we try to find sender data + let own_crypto_store = + FakeCryptoStore::new(Some(device), Some(user_identities), Some(own_user_identities)); + + let finder = create_finder(&own_crypto_store, Some(user_id!("@myself:s.co"))); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the information about the sender + assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_eq!(user_id, account.user_id()); + assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + assert!(!msk_verified); + } + + #[async_test] + async fn test_adds_sender_data_for_own_verified_device_and_user_using_device_from_event() { + // Given an account and user identity + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; + let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(Some(&device), &room_key_content); + + // When we try to find sender data + let own_crypto_store = FakeCryptoStore::user_only(user_identities); + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the information about the sender + assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_eq!(user_id, account.user_id()); + assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + assert!(!msk_verified); + } + + #[async_test] + async fn test_adds_sender_data_for_other_verified_device_and_user_using_device_from_event() { + // Given an account, user identity, and a separate identity to be our own + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let user_identity = ReadOnlyUserIdentity::from_private(&private_identity).await; + let user_identities = ReadOnlyUserIdentities::Other(user_identity.clone()); + let own_private_identity = create_private_identity(&account).await; + let own_user_identity = ReadOnlyOwnUserIdentity::from_private(&own_private_identity).await; + let own_user_identities = ReadOnlyUserIdentities::Own(own_user_identity.clone()); + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(Some(&device), &room_key_content); + + // When we try to find sender data + let own_crypto_store = + FakeCryptoStore::new(None, Some(user_identities), Some(own_user_identities)); + + let finder = create_finder(&own_crypto_store, Some(user_id!("@myself:s.co"))); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the information about the sender + assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_eq!(user_id, account.user_id()); + assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + assert!(!msk_verified); + } + + #[async_test] + async fn test_marks_msk_as_verified_if_it_is_for_own_identity() { + // Given an account and user identity which is verified + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; + user_identity.mark_as_verified(); + let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(Some(&device), &room_key_content); + + // When we try to find sender data + let own_crypto_store = FakeCryptoStore::user_only(user_identities); + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the information about the sender + assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_eq!(user_id, account.user_id()); + assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + // Including the fact that it was verified + assert!(msk_verified); + } + + #[async_test] + async fn test_marks_msk_as_verified_if_it_is_for_other_identity() { + // Given an account, user identity, and a separate identity to be our own + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let mut user_identity = ReadOnlyUserIdentity::from_private(&private_identity).await; + let own_private_identity = create_private_identity(&account).await; + let own_user_identity = ReadOnlyOwnUserIdentity::from_private(&own_private_identity).await; + + // Where the other identity is verified (signed by our identity) + { + let user_signing = own_private_identity.user_signing_key.lock().await; + let user_signing = user_signing.as_ref().unwrap(); + let master = user_signing.sign_user(&user_identity).unwrap(); + user_identity.master_key = Arc::new(master.try_into().unwrap()); + user_signing.public_key().verify_master_key(user_identity.master_key()).unwrap(); + } + + let own_user_identities = ReadOnlyUserIdentities::Own(own_user_identity.clone()); + let user_identities = ReadOnlyUserIdentities::Other(user_identity.clone()); + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // And an event (not containing device info) + let room_key_content = room_key_content(); + let room_key_event = room_key_event(Some(&device), &room_key_content); + + // When we try to find sender data + let own_crypto_store = + FakeCryptoStore::new(None, Some(user_identities), Some(own_user_identities)); + + let finder = create_finder(&own_crypto_store, Some(user_id!("@myself:s.co"))); + let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + + // Then we get back the information about the sender + assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_eq!(user_id, account.user_id()); + assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + // Including the fact that it was verified + assert!(msk_verified); + } + + #[async_test] + async fn test_adds_sender_data_based_on_existing_device() { + // Given an account and user identity + let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); + let private_identity = create_private_identity(&account).await; + let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; + let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); + + // And a device signed by the user identity + let device = create_signed_device(&account, &private_identity).await; + + // When we try to find sender data directly using based in the device instead of + // using a room key event + let own_crypto_store = FakeCryptoStore::user_only(user_identities); + let finder = create_finder(&own_crypto_store, None); + let sender_data = finder.have_device_keys(device.as_device_keys()).await.unwrap(); + + // Then we get back the information about the sender + assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_eq!(user_id, account.user_id()); + assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + assert!(!msk_verified); + } + + async fn create_private_identity(account: &Account) -> PrivateCrossSigningIdentity { + PrivateCrossSigningIdentity::with_account(account).await.0 + } + + async fn create_signed_device( + account: &Account, + private_identity: &PrivateCrossSigningIdentity, + ) -> Device { + let mut read_only_device = ReadOnlyDevice::from_account(account); + + let self_signing = private_identity.self_signing_key.lock().await; + let self_signing = self_signing.as_ref().unwrap(); + + let mut device_keys = read_only_device.as_device_keys().to_owned(); + self_signing.sign_device(&mut device_keys).unwrap(); + read_only_device.update_device(&device_keys).unwrap(); + + wrap_device(account, read_only_device) + } + + fn create_unsigned_device(account: &Account) -> Device { + wrap_device(account, ReadOnlyDevice::from_account(account)) + } + + fn wrap_device(account: &Account, read_only_device: ReadOnlyDevice) -> Device { + Device { + inner: read_only_device, + verification_machine: VerificationMachine::new( + account.deref().clone(), + Arc::new(Mutex::new(PrivateCrossSigningIdentity::new( + account.user_id().to_owned(), + ))), + Arc::new(CryptoStoreWrapper::new(account.user_id(), MemoryStore::new())), + ), + own_identity: None, + device_owner_identity: None, + } + } + + fn create_curve_key() -> Curve25519PublicKey { + Curve25519PublicKey::from_base64("7PUPP6Ijt5R8qLwK2c8uK5hqCNF9tOzWYgGaAay5JBs").unwrap() + } + + fn create_finder<'a, S>( + store: &'a S, + own_user_id: Option<&'a UserId>, + ) -> SenderDataFinder<'a, S> + where + S: FinderCryptoStore, + { + SenderDataFinder { + own_crypto_store: store, + own_user_id: own_user_id.unwrap_or(user_id!("@u:s.co")), + } + } + + fn room_key_event( + device: Option<&Device>, + content: &MegolmV1AesSha2Content, + ) -> DecryptedRoomKeyEvent { + DecryptedRoomKeyEvent::new( + user_id!("@s:s.co"), + user_id!("@u:s.co"), + Ed25519PublicKey::from_base64("loz5i40dP+azDtWvsD0L/xpnCjNkmrcvtXVXzCHX8Vw").unwrap(), + device.map(|d| d.as_device_keys().clone()), + RoomKeyContent::MegolmV1AesSha2(Box::new(clone_content(content))), + ) + } + + fn room_key_content() -> MegolmV1AesSha2Content { + MegolmV1AesSha2Content::new( + owned_room_id!("!r:s.co"), + "mysession".to_owned(), + SessionKey::from_base64( + "\ + AgAAAADBy9+YIYTIqBjFT67nyi31gIOypZQl8day2hkhRDCZaHoG+cZh4tZLQIAZimJail0\ + 0zq4DVJVljO6cZ2t8kIto/QVk+7p20Fcf2nvqZyL2ZCda2Ei7VsqWZHTM/gqa2IU9+ktkwz\ + +KFhENnHvDhG9f+hjsAPZd5mTTpdO+tVcqtdWhX4dymaJ/2UpAAjuPXQW+nXhQWQhXgXOUa\ + JCYurJtvbCbqZGeDMmVIoqukBs2KugNJ6j5WlTPoeFnMl6Guy9uH2iWWxGg8ZgT2xspqVl5\ + CwujjC+m7Dh1toVkvu+bAw\ + ", + ) + .unwrap(), + ) + } + + fn clone_content(content: &MegolmV1AesSha2Content) -> MegolmV1AesSha2Content { + MegolmV1AesSha2Content::new( + content.room_id.clone(), + content.session_id.clone(), + SessionKey::from_base64(&content.session_key.to_base64()).unwrap(), + ) + } + + struct FakeCryptoStore { + device: Option, + user_identities: Option, + own_user_identities: Option, + } + + impl FakeCryptoStore { + fn new( + device: Option, + user_identities: Option, + own_user_identities: Option, + ) -> Self { + Self { device, user_identities, own_user_identities } + } + + fn device_and_user(device: Device, user_identities: ReadOnlyUserIdentities) -> Self { + Self { + device: Some(device), + user_identities: Some(user_identities), + own_user_identities: None, + } + } + + fn device_only(device: Device) -> Self { + Self { device: Some(device), user_identities: None, own_user_identities: None } + } + + fn user_only(user_identities: ReadOnlyUserIdentities) -> Self { + Self { device: None, user_identities: Some(user_identities), own_user_identities: None } + } + + fn empty() -> Self { + Self { device: None, user_identities: None, own_user_identities: None } + } + } + + #[cfg_attr(target_arch = "wasm32", async_trait(?Send))] + #[cfg_attr(not(target_arch = "wasm32"), async_trait)] + impl FinderCryptoStore for FakeCryptoStore { + async fn get_device_from_curve_key( + &self, + _user_id: &UserId, + _curve_key: Curve25519PublicKey, + ) -> store::Result> { + Ok(self.device.clone()) + } + + async fn get_user_identity( + &self, + user_id: &UserId, + ) -> OlmResult> { + if user_id == user_id!("@myself:s.co") { + // We are being asked for our own user identity, different from + // self.user_identities because that one was of Other type. + Ok(self.own_user_identities.clone()) + } else { + Ok(self.user_identities.clone()) + } + } + } +} diff --git a/crates/matrix-sdk-crypto/src/olm/mod.rs b/crates/matrix-sdk-crypto/src/olm/mod.rs index 51396e67c89..a5f41de4a4d 100644 --- a/crates/matrix-sdk-crypto/src/olm/mod.rs +++ b/crates/matrix-sdk-crypto/src/olm/mod.rs @@ -25,12 +25,12 @@ mod utility; pub use account::{Account, OlmMessageHash, PickledAccount, StaticAccountData}; pub(crate) use account::{OlmDecryptionInfo, SessionType}; -pub(crate) use group_sessions::ShareState; pub use group_sessions::{ BackedUpRoomKey, EncryptionSettings, ExportedRoomKey, InboundGroupSession, OutboundGroupSession, PickledInboundGroupSession, PickledOutboundGroupSession, SenderData, SenderDataRetryDetails, SessionCreationError, SessionExportError, SessionKey, ShareInfo, }; +pub(crate) use group_sessions::{SenderDataFinder, ShareState}; pub use session::{PickledSession, Session}; pub use signing::{CrossSigningStatus, PickledCrossSigningIdentity, PrivateCrossSigningIdentity}; pub(crate) use utility::{SignedJsonObject, VerifyJson}; 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 30ada4a82b8..da18a5955a4 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 @@ -36,7 +36,7 @@ use tracing::{debug, error, info, instrument, trace}; use crate::{ error::{EventError, MegolmResult, OlmResult}, identities::device::MaybeEncryptedRoomKey, - olm::{InboundGroupSession, OutboundGroupSession, Session, ShareInfo, ShareState}, + olm::{InboundGroupSession, OutboundGroupSession, SenderData, Session, ShareInfo, ShareState}, store::{Changes, CryptoStoreWrapper, Result as StoreResult, Store}, types::events::{room::encrypted::RoomEncryptedEventContent, room_key_withheld::WithheldCode}, EncryptionSettings, OlmError, ReadOnlyDevice, ToDeviceRequest, @@ -211,11 +211,12 @@ impl GroupSessionManager { &self, room_id: &RoomId, settings: EncryptionSettings, + own_sender_data: SenderData, ) -> OlmResult<(OutboundGroupSession, InboundGroupSession)> { let (outbound, inbound) = self .store .static_account() - .create_group_session_pair(room_id, settings) + .create_group_session_pair(room_id, settings, own_sender_data) .await .map_err(|_| EventError::UnsupportedAlgorithm)?; @@ -227,6 +228,7 @@ impl GroupSessionManager { &self, room_id: &RoomId, settings: EncryptionSettings, + own_sender_data: SenderData, ) -> OlmResult<(OutboundGroupSession, Option)> { let outbound_session = self.sessions.get_or_load(room_id).await; @@ -234,14 +236,16 @@ impl GroupSessionManager { // create a new one. if let Some(s) = outbound_session { if s.expired() || s.invalidated() { - self.create_outbound_group_session(room_id, settings) + self.create_outbound_group_session(room_id, settings, own_sender_data) .await .map(|(o, i)| (o, i.into())) } else { Ok((s, None)) } } else { - self.create_outbound_group_session(room_id, settings).await.map(|(o, i)| (o, i.into())) + self.create_outbound_group_session(room_id, settings, own_sender_data) + .await + .map(|(o, i)| (o, i.into())) } } @@ -372,12 +376,14 @@ impl GroupSessionManager { outbound: OutboundGroupSession, encryption_settings: EncryptionSettings, changes: &mut Changes, + sender_data: SenderData, ) -> OlmResult { Ok(if should_rotate { let old_session_id = outbound.session_id(); - let (outbound, inbound) = - self.create_outbound_group_session(room_id, encryption_settings).await?; + let (outbound, inbound) = self + .create_outbound_group_session(room_id, encryption_settings, sender_data) + .await?; changes.outbound_group_sessions.push(outbound.clone()); changes.inbound_group_sessions.push(inbound); @@ -632,6 +638,7 @@ impl GroupSessionManager { room_id: &RoomId, users: impl Iterator, encryption_settings: impl Into, + own_sender_data: SenderData, ) -> OlmResult>> { trace!("Checking if a room key needs to be shared"); @@ -639,8 +646,13 @@ impl GroupSessionManager { let mut changes = Changes::default(); // Try to get an existing session or create a new one. - let (outbound, inbound) = - self.get_or_create_outbound_session(room_id, encryption_settings.clone()).await?; + let (outbound, inbound) = self + .get_or_create_outbound_session( + room_id, + encryption_settings.clone(), + own_sender_data.clone(), + ) + .await?; tracing::Span::current().record("session_id", outbound.session_id()); // Having an inbound group session here means that we created a new @@ -663,6 +675,7 @@ impl GroupSessionManager { outbound, encryption_settings, &mut changes, + own_sender_data, ) .await?; @@ -764,7 +777,7 @@ mod tests { use crate::{ identities::ReadOnlyDevice, machine::EncryptionSyncChanges, - olm::Account, + olm::{Account, SenderData}, session_manager::{group_sessions::CollectRecipientsResult, CollectStrategy}, types::{ events::{ @@ -1113,7 +1126,11 @@ mod tests { let (outbound, _) = machine .inner .group_session_manager - .get_or_create_outbound_session(room_id, EncryptionSettings::default()) + .get_or_create_outbound_session( + room_id, + EncryptionSettings::default(), + SenderData::unknown(), + ) .await .expect("We should be able to create a new session"); let history_visibility = HistoryVisibility::Joined; From e66f4433d552e1963f4b112c8168f16ca71c758e Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 10 Jul 2024 15:22:01 +0100 Subject: [PATCH 02/27] fixup: rename msk to master_key --- .../olm/group_sessions/sender_data_finder.rs | 94 +++++++++++-------- 1 file changed, 55 insertions(+), 39 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index ac8bb55e7c3..172c8489411 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -241,23 +241,25 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { // // Does the cross-signing key match that used to sign the device info? // And is the signature in the device info valid? - let maybe_msk_info = - self.msk_if_device_is_signed_by_user(&sender_device, &sender_user_identity).await; + let maybe_master_key_info = self + .master_key_if_device_is_signed_by_user(&sender_device, &sender_user_identity) + .await; - if let Some((msk, msk_verified)) = maybe_msk_info { + if let Some((master_key, master_key_verified)) = maybe_master_key_info { // Yes: H (cross-signing key matches that used to sign the device info!) // and: J (device info is verified by matching cross-signing key) // Find the actual key within the MasterPubkey struct - if let Some(msk) = msk.get_first_key() { - // We have MXID and MSK for the user sending the to-device message. - // Decide the MSK trust level based on whether we have verified this user. - // Set the MXID, MSK and trust level in the session. Remove the device - // info and retries since we don't need them. + if let Some(master_key) = master_key.get_first_key() { + // We have MXID and master_key for the user sending the to-device message. + // Decide the master_key trust level based on whether we have verified this + // user. Set the MXID, master_key and trust level in the + // session. Remove the device info and retries since we don't + // need them. Ok(SenderData::SenderKnown { user_id: sender_device.user_id().to_owned(), - msk, - msk_verified, + master_key, + master_key_verified, }) // TODO: [drop the lock] } else { @@ -266,7 +268,7 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { // tracing::error!( "MasterPubkey for user {} does not contain any keys!", - msk.user_id() + master_key.user_id() ); Ok(SenderData::DeviceInfo { @@ -300,7 +302,7 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { /// If sender_device is correctly cross-signed by user_identity, return /// user_identity's master key and whether it is verified. /// Otherwise, return None. - async fn msk_if_device_is_signed_by_user<'i>( + async fn master_key_if_device_is_signed_by_user<'i>( &self, sender_device: &'_ ReadOnlyDevice, sender_user_identity: &'i ReadOnlyUserIdentities, @@ -318,17 +320,17 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { return None; } - let msk = other_identity.master_key(); + let master_key = other_identity.master_key(); // Use our own identity to determine whether this other identity is signed - let msk_verified = if let Some(own_identity) = self.own_identity().await { + let master_key_verified = if let Some(own_identity) = self.own_identity().await { own_identity.is_identity_signed(other_identity).is_ok() } else { - // Couldn't get own identity! Assume msk is not verified. + // Couldn't get own identity! Assume master_key is not verified. false }; - Some((msk, msk_verified)) + Some((master_key, master_key_verified)) } } } @@ -541,10 +543,12 @@ mod tests { let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); // Then we get back the information about the sender - assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_let!( + SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data + ); assert_eq!(user_id, account.user_id()); - assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); - assert!(!msk_verified); + assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert!(!master_key_verified); } #[async_test] @@ -573,10 +577,12 @@ mod tests { let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); // Then we get back the information about the sender - assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_let!( + SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data + ); assert_eq!(user_id, account.user_id()); - assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); - assert!(!msk_verified); + assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert!(!master_key_verified); } #[async_test] @@ -600,10 +606,12 @@ mod tests { let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); // Then we get back the information about the sender - assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_let!( + SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data + ); assert_eq!(user_id, account.user_id()); - assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); - assert!(!msk_verified); + assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert!(!master_key_verified); } #[async_test] @@ -632,14 +640,16 @@ mod tests { let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); // Then we get back the information about the sender - assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_let!( + SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data + ); assert_eq!(user_id, account.user_id()); - assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); - assert!(!msk_verified); + assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert!(!master_key_verified); } #[async_test] - async fn test_marks_msk_as_verified_if_it_is_for_own_identity() { + async fn test_marks_master_key_as_verified_if_it_is_for_own_identity() { // Given an account and user identity which is verified let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); let private_identity = create_private_identity(&account).await; @@ -660,15 +670,17 @@ mod tests { let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); // Then we get back the information about the sender - assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_let!( + SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data + ); assert_eq!(user_id, account.user_id()); - assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); // Including the fact that it was verified - assert!(msk_verified); + assert!(master_key_verified); } #[async_test] - async fn test_marks_msk_as_verified_if_it_is_for_other_identity() { + async fn test_marks_master_key_as_verified_if_it_is_for_other_identity() { // Given an account, user identity, and a separate identity to be our own let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); let private_identity = create_private_identity(&account).await; @@ -703,11 +715,13 @@ mod tests { let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); // Then we get back the information about the sender - assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_let!( + SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data + ); assert_eq!(user_id, account.user_id()); - assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); // Including the fact that it was verified - assert!(msk_verified); + assert!(master_key_verified); } #[async_test] @@ -728,10 +742,12 @@ mod tests { let sender_data = finder.have_device_keys(device.as_device_keys()).await.unwrap(); // Then we get back the information about the sender - assert_let!(SenderData::SenderKnown { user_id, msk, msk_verified } = sender_data); + assert_let!( + SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data + ); assert_eq!(user_id, account.user_id()); - assert_eq!(msk, user_identity.master_key().get_first_key().unwrap()); - assert!(!msk_verified); + assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert!(!master_key_verified); } async fn create_private_identity(account: &Account) -> PrivateCrossSigningIdentity { From e41fe9474bc3250acacbf27df3bdb8558b581f41 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 10 Jul 2024 15:28:12 +0100 Subject: [PATCH 03/27] fixup: Don't panic if our identity is not our own - just give up instead --- .../src/olm/group_sessions/sender_data_finder.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 172c8489411..2f88d13b167 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -341,11 +341,7 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { let own_identity = self.own_crypto_store.get_user_identity(self.own_user_id).await.ok()??; - let ReadOnlyUserIdentities::Own(own_identity) = own_identity else { - panic!("The user identity for our own user ID was not an own identity!"); - }; - - Some(own_identity) + own_identity.into_own() } } From 36192f9ed51f6df3c9b25142c6babb45cbaa64ec Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 10 Jul 2024 15:29:52 +0100 Subject: [PATCH 04/27] fixup: Fix references to arguments in doc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Damir Jelić Signed-off-by: Andy Balaam --- .../src/olm/group_sessions/sender_data_finder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 2f88d13b167..31545489550 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -299,7 +299,7 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { } } - /// If sender_device is correctly cross-signed by user_identity, return + /// If `sender_device` is correctly cross-signed by `sender_user_identity`, return /// user_identity's master key and whether it is verified. /// Otherwise, return None. async fn master_key_if_device_is_signed_by_user<'i>( From 4f34e0c968780fc4172fd734348d9274f1198153 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 10 Jul 2024 15:30:43 +0100 Subject: [PATCH 05/27] fixup: formatting --- .../src/olm/group_sessions/sender_data_finder.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 31545489550..b587a77fa4e 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -299,8 +299,8 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { } } - /// If `sender_device` is correctly cross-signed by `sender_user_identity`, return - /// user_identity's master key and whether it is verified. + /// If `sender_device` is correctly cross-signed by `sender_user_identity`, + /// return user_identity's master key and whether it is verified. /// Otherwise, return None. async fn master_key_if_device_is_signed_by_user<'i>( &self, From 50e4acfe6fa3aae375a02b90aa687ed610b9ad44 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Wed, 10 Jul 2024 15:46:32 +0100 Subject: [PATCH 06/27] fixup: surface store errors --- .../olm/group_sessions/sender_data_finder.rs | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index b587a77fa4e..7ce54a8f814 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -243,7 +243,7 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { // And is the signature in the device info valid? let maybe_master_key_info = self .master_key_if_device_is_signed_by_user(&sender_device, &sender_user_identity) - .await; + .await?; if let Some((master_key, master_key_verified)) = maybe_master_key_info { // Yes: H (cross-signing key matches that used to sign the device info!) @@ -306,8 +306,8 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { &self, sender_device: &'_ ReadOnlyDevice, sender_user_identity: &'i ReadOnlyUserIdentities, - ) -> Option<(&'i MasterPubkey, bool)> { - match sender_user_identity { + ) -> OlmResult> { + Ok(match sender_user_identity { ReadOnlyUserIdentities::Own(own_identity) => { if own_identity.is_device_signed(sender_device).is_ok() { Some((own_identity.master_key(), own_identity.is_verified())) @@ -316,32 +316,34 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { } } ReadOnlyUserIdentities::Other(other_identity) => { - if other_identity.is_device_signed(sender_device).is_err() { - return None; - } - - let master_key = other_identity.master_key(); - - // Use our own identity to determine whether this other identity is signed - let master_key_verified = if let Some(own_identity) = self.own_identity().await { - own_identity.is_identity_signed(other_identity).is_ok() + if other_identity.is_device_signed(sender_device).is_ok() { + let master_key = other_identity.master_key(); + + // Use our own identity to determine whether this other identity is signed + let master_key_verified = + if let Some(own_identity) = self.own_identity().await? { + own_identity.is_identity_signed(other_identity).is_ok() + } else { + // Couldn't get own identity! Assume master_key is not verified. + false + }; + + Some((master_key, master_key_verified)) } else { - // Couldn't get own identity! Assume master_key is not verified. - false - }; - - Some((master_key, master_key_verified)) + None + } } - } + }) } /// Return the user identity of the current user, or None if we failed to /// find it (which is unexpected) - async fn own_identity(&self) -> Option { - let own_identity = - self.own_crypto_store.get_user_identity(self.own_user_id).await.ok()??; - - own_identity.into_own() + async fn own_identity(&self) -> OlmResult> { + Ok(self + .own_crypto_store + .get_user_identity(self.own_user_id) + .await? + .and_then(|i| i.into_own())) } } From b154b8777f21578d0af5fe7be6fd81ad906a498e Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 14:02:22 +0100 Subject: [PATCH 07/27] fixup: Set up for tests using a real MemoryStore instead of a fake --- .../olm/group_sessions/sender_data_finder.rs | 660 +++++++++++------- 1 file changed, 395 insertions(+), 265 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 7ce54a8f814..bb341c68088 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -388,7 +388,7 @@ mod tests { use assert_matches2::assert_let; use async_trait::async_trait; use matrix_sdk_test::async_test; - use ruma::{device_id, owned_room_id, user_id, UserId}; + use ruma::{device_id, owned_room_id, user_id, DeviceId, OwnedUserId, UserId}; use tokio::sync::Mutex; use vodozemac::{megolm::SessionKey, Curve25519PublicKey, Ed25519PublicKey}; @@ -396,7 +396,7 @@ mod tests { use crate::{ error::OlmResult, olm::{PrivateCrossSigningIdentity, SenderData}, - store::{self, CryptoStoreWrapper, MemoryStore}, + store::{self, Changes, CryptoStoreWrapper, MemoryStore, Store}, types::events::{ olm_v1::DecryptedRoomKeyEvent, room_key::{MegolmV1AesSha2Content, RoomKeyContent}, @@ -406,16 +406,32 @@ mod tests { ReadOnlyUserIdentity, }; + impl<'a> SenderDataFinder<'a, Store> { + fn new(own_crypto_store: &'a Store, own_user_id: &'a UserId) -> Self { + Self { own_crypto_store, own_user_id } + } + } + #[async_test] async fn test_providing_no_device_data_returns_sender_data_with_no_device_info() { - // Given that the crypto store is empty and the initial event has no device info - let room_key_content = room_key_content(); - let room_key_event = room_key_event(None, &room_key_content); - let own_crypto_store = FakeCryptoStore::empty(); + // Given that the device is not in the store and the initial event has no device + // info + let setup = TestSetup::new(TestOptions { + store_contains_device: false, + store_contains_sender_identity: false, + device_is_signed: true, + event_contains_device_keys: false, + sender_is_ourself: false, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back no useful information at all assert_let!(SenderData::UnknownDevice { retry_details, legacy_session } = sender_data); @@ -429,27 +445,29 @@ mod tests { #[async_test] async fn test_if_the_todevice_event_contains_device_info_it_is_captured() { - // Given an account and user identity - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event containing device info - let room_key_content = room_key_content(); - let room_key_event = room_key_event(Some(&device), &room_key_content); + // Given that the signed device keys are in the event + let setup = TestSetup::new(TestOptions { + store_contains_device: false, + store_contains_sender_identity: false, + device_is_signed: true, + event_contains_device_keys: true, + sender_is_ourself: false, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = FakeCryptoStore::empty(); - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); - // Then we get back the device keys that were in the store + // Then we get back the device keys that were in the event assert_let!( SenderData::DeviceInfo { device_keys, retry_details, legacy_session } = sender_data ); - assert_eq!(&device_keys, device.as_device_keys()); + assert_eq!(&device_keys, setup.sender_device.as_device_keys()); assert_eq!(retry_details.retry_count, 0); // TODO: This should not be marked as a legacy session, but for now it is @@ -460,28 +478,30 @@ mod tests { #[async_test] async fn test_picks_up_device_info_from_the_store_if_missing_from_the_todevice_event() { - // Given an account and user identity - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(None, &room_key_content); + // Given that the device keys are not in the event but the device is in the + // store + let setup = TestSetup::new(TestOptions { + store_contains_device: true, + store_contains_sender_identity: false, + device_is_signed: true, + event_contains_device_keys: false, + sender_is_ourself: false, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); - // When we try to find sender data (but the user identity can't be found in the - // store) - let own_crypto_store = FakeCryptoStore::device_only(device.clone()); - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + // When we try to find sender data + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back the device keys that were in the store assert_let!( SenderData::DeviceInfo { device_keys, retry_details, legacy_session } = sender_data ); - assert_eq!(&device_keys, device.as_device_keys()); + assert_eq!(&device_keys, setup.sender_device.as_device_keys()); assert_eq!(retry_details.retry_count, 0); // TODO: This should not be marked as a legacy session, but for now it is @@ -492,23 +512,24 @@ mod tests { #[async_test] async fn test_does_not_add_sender_data_if_device_is_not_signed() { - // Given an account and user identity - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; - let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); - - // And a device (not signed) - let device = create_unsigned_device(&account); - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(None, &room_key_content); + // Given that the the device is in the store + // But it is not signed + let setup = TestSetup::new(TestOptions { + store_contains_device: true, + store_contains_sender_identity: false, + device_is_signed: false, + event_contains_device_keys: false, + sender_is_ourself: false, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = FakeCryptoStore::device_and_user(device, user_identities); - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we treat it as if there is no device info at all assert_let!(SenderData::UnknownDevice { retry_details, legacy_session } = sender_data); @@ -522,232 +543,388 @@ mod tests { #[async_test] async fn test_adds_sender_data_for_own_verified_device_and_user_using_device_from_store() { - // Given an account and user identity - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; - let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(None, &room_key_content); + // Given the device is in the store, and we sent the event + let setup = TestSetup::new(TestOptions { + store_contains_device: true, + store_contains_sender_identity: true, + device_is_signed: true, + event_contains_device_keys: false, + sender_is_ourself: true, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = FakeCryptoStore::device_and_user(device, user_identities); - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back the information about the sender assert_let!( SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data ); - assert_eq!(user_id, account.user_id()); - assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(user_id, setup.sender.user_id); + assert_eq!(master_key, setup.sender_master_key()); assert!(!master_key_verified); } #[async_test] async fn test_adds_sender_data_for_other_verified_device_and_user_using_device_from_store() { - // Given an account, user identity, and a separate identity to be our own - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let user_identity = ReadOnlyUserIdentity::from_private(&private_identity).await; - let user_identities = ReadOnlyUserIdentities::Other(user_identity.clone()); - let own_private_identity = create_private_identity(&account).await; - let own_user_identity = ReadOnlyOwnUserIdentity::from_private(&own_private_identity).await; - let own_user_identities = ReadOnlyUserIdentities::Own(own_user_identity.clone()); - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(None, &room_key_content); + // Given the device is in the store, and someone else sent the event + let setup = TestSetup::new(TestOptions { + store_contains_device: true, + store_contains_sender_identity: true, + device_is_signed: true, + event_contains_device_keys: false, + sender_is_ourself: false, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = - FakeCryptoStore::new(Some(device), Some(user_identities), Some(own_user_identities)); - - let finder = create_finder(&own_crypto_store, Some(user_id!("@myself:s.co"))); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back the information about the sender assert_let!( SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data ); - assert_eq!(user_id, account.user_id()); - assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(user_id, setup.sender.user_id); + assert_eq!(master_key, setup.sender_master_key()); assert!(!master_key_verified); } #[async_test] - async fn test_adds_sender_data_for_own_verified_device_and_user_using_device_from_event() { - // Given an account and user identity - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; - let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(Some(&device), &room_key_content); + async fn test_adds_sender_data_for_own_device_and_user_using_device_from_event() { + // Given the device keys are in the event, and we sent the event + let setup = TestSetup::new(TestOptions { + store_contains_device: false, + store_contains_sender_identity: true, + device_is_signed: true, + event_contains_device_keys: true, + sender_is_ourself: true, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = FakeCryptoStore::user_only(user_identities); - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back the information about the sender assert_let!( SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data ); - assert_eq!(user_id, account.user_id()); - assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(user_id, setup.sender.user_id); + assert_eq!(master_key, setup.sender_master_key()); assert!(!master_key_verified); } #[async_test] async fn test_adds_sender_data_for_other_verified_device_and_user_using_device_from_event() { - // Given an account, user identity, and a separate identity to be our own - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let user_identity = ReadOnlyUserIdentity::from_private(&private_identity).await; - let user_identities = ReadOnlyUserIdentities::Other(user_identity.clone()); - let own_private_identity = create_private_identity(&account).await; - let own_user_identity = ReadOnlyOwnUserIdentity::from_private(&own_private_identity).await; - let own_user_identities = ReadOnlyUserIdentities::Own(own_user_identity.clone()); - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(Some(&device), &room_key_content); + // Given the device keys are in the event, and someone else sent the event + let setup = TestSetup::new(TestOptions { + store_contains_device: false, + store_contains_sender_identity: true, + device_is_signed: true, + event_contains_device_keys: true, + sender_is_ourself: false, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = - FakeCryptoStore::new(None, Some(user_identities), Some(own_user_identities)); - - let finder = create_finder(&own_crypto_store, Some(user_id!("@myself:s.co"))); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back the information about the sender assert_let!( SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data ); - assert_eq!(user_id, account.user_id()); - assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(user_id, setup.sender.user_id); + assert_eq!(master_key, setup.sender_master_key()); assert!(!master_key_verified); } #[async_test] - async fn test_marks_master_key_as_verified_if_it_is_for_own_identity() { - // Given an account and user identity which is verified - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; - user_identity.mark_as_verified(); - let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(Some(&device), &room_key_content); + async fn test_notes_master_key_is_verified_for_own_identity() { + // Given we can find the device info, and we sent the event, and we are verified + let setup = TestSetup::new(TestOptions { + store_contains_device: true, + store_contains_sender_identity: true, + device_is_signed: true, + event_contains_device_keys: true, + sender_is_ourself: true, + sender_is_verified: true, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = FakeCryptoStore::user_only(user_identities); - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back the information about the sender assert_let!( SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data ); - assert_eq!(user_id, account.user_id()); - assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(user_id, setup.sender.user_id); + assert_eq!(master_key, setup.sender_master_key()); // Including the fact that it was verified assert!(master_key_verified); } #[async_test] - async fn test_marks_master_key_as_verified_if_it_is_for_other_identity() { - // Given an account, user identity, and a separate identity to be our own - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let mut user_identity = ReadOnlyUserIdentity::from_private(&private_identity).await; - let own_private_identity = create_private_identity(&account).await; - let own_user_identity = ReadOnlyOwnUserIdentity::from_private(&own_private_identity).await; - - // Where the other identity is verified (signed by our identity) - { - let user_signing = own_private_identity.user_signing_key.lock().await; - let user_signing = user_signing.as_ref().unwrap(); - let master = user_signing.sign_user(&user_identity).unwrap(); - user_identity.master_key = Arc::new(master.try_into().unwrap()); - user_signing.public_key().verify_master_key(user_identity.master_key()).unwrap(); - } - - let own_user_identities = ReadOnlyUserIdentities::Own(own_user_identity.clone()); - let user_identities = ReadOnlyUserIdentities::Other(user_identity.clone()); - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // And an event (not containing device info) - let room_key_content = room_key_content(); - let room_key_event = room_key_event(Some(&device), &room_key_content); + async fn test_notes_master_key_is_verified_for_other_identity() { + // Given we can find the device info, and someone else sent the event + // And the sender is verified + let setup = TestSetup::new(TestOptions { + store_contains_device: true, + store_contains_sender_identity: true, + device_is_signed: true, + event_contains_device_keys: true, + sender_is_ourself: false, + sender_is_verified: true, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); // When we try to find sender data - let own_crypto_store = - FakeCryptoStore::new(None, Some(user_identities), Some(own_user_identities)); - - let finder = create_finder(&own_crypto_store, Some(user_id!("@myself:s.co"))); - let sender_data = finder.have_event(create_curve_key(), &room_key_event).await.unwrap(); + let sender_data = finder + .have_event(setup.sender_device_curve_key(), &setup.room_key_event) + .await + .unwrap(); // Then we get back the information about the sender assert_let!( SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data ); - assert_eq!(user_id, account.user_id()); - assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(user_id, setup.sender.user_id); + assert_eq!(master_key, setup.sender_master_key()); // Including the fact that it was verified assert!(master_key_verified); } #[async_test] - async fn test_adds_sender_data_based_on_existing_device() { - // Given an account and user identity - let account = Account::with_device_id(user_id!("@u:s.co"), device_id!("DEVICEID")); - let private_identity = create_private_identity(&account).await; - let user_identity = ReadOnlyOwnUserIdentity::from_private(&private_identity).await; - let user_identities = ReadOnlyUserIdentities::Own(user_identity.clone()); - - // And a device signed by the user identity - let device = create_signed_device(&account, &private_identity).await; - - // When we try to find sender data directly using based in the device instead of - // using a room key event - let own_crypto_store = FakeCryptoStore::user_only(user_identities); - let finder = create_finder(&own_crypto_store, None); - let sender_data = finder.have_device_keys(device.as_device_keys()).await.unwrap(); + async fn test_can_add_user_sender_data_based_on_a_provided_device() { + // Given the device is not in the store or the event + let setup = TestSetup::new(TestOptions { + store_contains_device: false, + store_contains_sender_identity: true, + device_is_signed: true, + event_contains_device_keys: false, + sender_is_ourself: false, + sender_is_verified: false, + }) + .await; + let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); - // Then we get back the information about the sender + // When we supply the device keys directly while asking for the sender data + let sender_data = + finder.have_device_keys(setup.sender_device.as_device_keys()).await.unwrap(); + + // Then it is found using the device we supplied assert_let!( SenderData::SenderKnown { user_id, master_key, master_key_verified } = sender_data ); - assert_eq!(user_id, account.user_id()); - assert_eq!(master_key, user_identity.master_key().get_first_key().unwrap()); + assert_eq!(user_id, setup.sender.user_id); + assert_eq!(master_key, setup.sender_master_key()); assert!(!master_key_verified); } + struct TestOptions { + store_contains_device: bool, + store_contains_sender_identity: bool, + device_is_signed: bool, + event_contains_device_keys: bool, + sender_is_ourself: bool, + sender_is_verified: bool, + } + + struct TestSetup { + me: TestUser, + sender: TestUser, + sender_device: Device, + store: Store, + room_key_event: DecryptedRoomKeyEvent, + } + + impl TestSetup { + async fn new(options: TestOptions) -> Self { + let me = TestUser::own().await; + let sender = TestUser::other(&me, &options).await; + + let sender_device = if options.device_is_signed { + create_signed_device(&sender.account, &*sender.private_identity.lock().await).await + } else { + create_unsigned_device(&sender.account) + }; + + let store = create_store(&sender); + + save_to_store(&store, &me, &sender, &sender_device, &options).await; + + let room_key_event = + create_room_key_event(&sender.user_id, &me.user_id, &sender_device, &options); + + Self { me, sender, sender_device, store, room_key_event } + } + + fn sender_device_curve_key(&self) -> Curve25519PublicKey { + self.sender_device.curve25519_key().unwrap().clone() + } + + fn sender_master_key(&self) -> Ed25519PublicKey { + self.sender.user_identities.master_key().get_first_key().unwrap() + } + } + + fn create_store(sender: &TestUser) -> Store { + let store_wrapper = Arc::new(CryptoStoreWrapper::new(&sender.user_id, MemoryStore::new())); + + let verification_machine = VerificationMachine::new( + sender.account.deref().clone(), + Arc::clone(&sender.private_identity), + Arc::new(CryptoStoreWrapper::new(&sender.user_id, MemoryStore::new())), + ); + + Store::new( + sender.account.static_data.clone(), + Arc::clone(&sender.private_identity), + store_wrapper, + verification_machine, + ) + } + + async fn save_to_store( + store: &Store, + me: &TestUser, + sender: &TestUser, + sender_device: &Device, + options: &TestOptions, + ) { + let mut changes = Changes::default(); + + // If the device should exist in the store, add it + if options.store_contains_device { + changes.devices.new.push(sender_device.inner.clone()) + } + + // Add the sender identity to the store + if options.store_contains_sender_identity { + changes.identities.new.push(sender.user_identities.clone()); + } + + // If it's different from the sender, add our identity too + if !options.sender_is_ourself { + changes.identities.new.push(me.user_identities.clone()); + } + + store.save_changes(changes).await.unwrap(); + } + + struct TestUser { + user_id: OwnedUserId, + account: Account, + private_identity: Arc>, + user_identities: ReadOnlyUserIdentities, + } + + impl TestUser { + async fn new( + user_id: &UserId, + device_id: &DeviceId, + is_me: bool, + is_verified: bool, + signer: Option<&TestUser>, + ) -> Self { + let account = Account::with_device_id(user_id, device_id); + let user_id = user_id.to_owned(); + let private_identity = Arc::new(Mutex::new(create_private_identity(&account).await)); + + let user_identities = + create_user_identities(&*private_identity.lock().await, is_me, is_verified, signer) + .await; + + Self { user_id, account, private_identity, user_identities } + } + + async fn own() -> Self { + Self::new(user_id!("@myself:s.co"), device_id!("OWNDEVICEID"), true, false, None).await + } + + async fn other(me: &TestUser, options: &TestOptions) -> Self { + let user_id = + if options.sender_is_ourself { &me.user_id } else { user_id!("@other:s.co") }; + + Self::new( + user_id, + device_id!("SENDERDEVICEID"), + options.sender_is_ourself, + options.sender_is_verified, + Some(me), + ) + .await + } + } + + async fn create_user_identities( + private_identity: &PrivateCrossSigningIdentity, + is_me: bool, + is_verified: bool, + signer: Option<&TestUser>, + ) -> ReadOnlyUserIdentities { + if is_me { + let user_identity = ReadOnlyOwnUserIdentity::from_private(private_identity).await; + + if is_verified { + user_identity.mark_as_verified(); + } + + ReadOnlyUserIdentities::Own(user_identity) + } else { + let mut user_identity = ReadOnlyUserIdentity::from_private(private_identity).await; + + if is_verified { + sign_other_identity(signer, &mut user_identity).await; + } + + ReadOnlyUserIdentities::Other(user_identity) + } + } + + async fn sign_other_identity( + signer: Option<&TestUser>, + user_identity: &mut ReadOnlyUserIdentity, + ) { + if let Some(signer) = signer { + let signer_private_identity = signer.private_identity.lock().await; + + let user_signing = signer_private_identity.user_signing_key.lock().await; + + let user_signing = user_signing.as_ref().unwrap(); + let master = user_signing.sign_user(&*user_identity).unwrap(); + user_identity.master_key = Arc::new(master.try_into().unwrap()); + + user_signing.public_key().verify_master_key(user_identity.master_key()).unwrap(); + } else { + panic!("You must provide a `signer` if you want an Other to be verified!"); + } + } + async fn create_private_identity(account: &Account) -> PrivateCrossSigningIdentity { PrivateCrossSigningIdentity::with_account(account).await.0 } @@ -787,33 +964,24 @@ mod tests { } } - fn create_curve_key() -> Curve25519PublicKey { - Curve25519PublicKey::from_base64("7PUPP6Ijt5R8qLwK2c8uK5hqCNF9tOzWYgGaAay5JBs").unwrap() - } - - fn create_finder<'a, S>( - store: &'a S, - own_user_id: Option<&'a UserId>, - ) -> SenderDataFinder<'a, S> - where - S: FinderCryptoStore, - { - SenderDataFinder { - own_crypto_store: store, - own_user_id: own_user_id.unwrap_or(user_id!("@u:s.co")), - } - } - - fn room_key_event( - device: Option<&Device>, - content: &MegolmV1AesSha2Content, + fn create_room_key_event( + sender: &UserId, + receiver: &UserId, + sender_device: &Device, + options: &TestOptions, ) -> DecryptedRoomKeyEvent { + let device = if options.event_contains_device_keys { + Some(sender_device.as_device_keys().clone()) + } else { + None + }; + DecryptedRoomKeyEvent::new( - user_id!("@s:s.co"), - user_id!("@u:s.co"), + sender, + receiver, Ed25519PublicKey::from_base64("loz5i40dP+azDtWvsD0L/xpnCjNkmrcvtXVXzCHX8Vw").unwrap(), - device.map(|d| d.as_device_keys().clone()), - RoomKeyContent::MegolmV1AesSha2(Box::new(clone_content(content))), + device, + RoomKeyContent::MegolmV1AesSha2(Box::new(room_key_content())), ) } @@ -834,50 +1002,12 @@ mod tests { ) } - fn clone_content(content: &MegolmV1AesSha2Content) -> MegolmV1AesSha2Content { - MegolmV1AesSha2Content::new( - content.room_id.clone(), - content.session_id.clone(), - SessionKey::from_base64(&content.session_key.to_base64()).unwrap(), - ) - } - struct FakeCryptoStore { device: Option, user_identities: Option, own_user_identities: Option, } - impl FakeCryptoStore { - fn new( - device: Option, - user_identities: Option, - own_user_identities: Option, - ) -> Self { - Self { device, user_identities, own_user_identities } - } - - fn device_and_user(device: Device, user_identities: ReadOnlyUserIdentities) -> Self { - Self { - device: Some(device), - user_identities: Some(user_identities), - own_user_identities: None, - } - } - - fn device_only(device: Device) -> Self { - Self { device: Some(device), user_identities: None, own_user_identities: None } - } - - fn user_only(user_identities: ReadOnlyUserIdentities) -> Self { - Self { device: None, user_identities: Some(user_identities), own_user_identities: None } - } - - fn empty() -> Self { - Self { device: None, user_identities: None, own_user_identities: None } - } - } - #[cfg_attr(target_arch = "wasm32", async_trait(?Send))] #[cfg_attr(not(target_arch = "wasm32"), async_trait)] impl FinderCryptoStore for FakeCryptoStore { From 41d024b42069fc45907bfb2b006f8b1a11d10d14 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 14:13:33 +0100 Subject: [PATCH 08/27] fixup: Remove generics from SenderDataFinder --- .../olm/group_sessions/sender_data_finder.rs | 87 ++----------------- 1 file changed, 7 insertions(+), 80 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index bb341c68088..a14a7c05836 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -12,9 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::ops::Deref; - -use async_trait::async_trait; use ruma::UserId; use tracing::error; use vodozemac::Curve25519PublicKey; @@ -22,8 +19,7 @@ use vodozemac::Curve25519PublicKey; use super::{SenderData, SenderDataRetryDetails}; use crate::{ error::OlmResult, - identities::Device, - store::{self, Store}, + store::Store, types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys, MasterPubkey}, EventError, OlmError, OlmMachine, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, @@ -34,12 +30,12 @@ use crate::{ /// /// The letters A, B etc. in the documentation refer to the algorithm described /// in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 -pub(crate) struct SenderDataFinder<'a, S: FinderCryptoStore> { - own_crypto_store: &'a S, +pub(crate) struct SenderDataFinder<'a> { + own_crypto_store: &'a Store, own_user_id: &'a UserId, } -impl<'a> SenderDataFinder<'a, Store> { +impl<'a> SenderDataFinder<'a> { /// As described in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 /// and https://github.com/matrix-org/matrix-rust-sdk/issues/3544 /// find the device info associated with the to-device message used to @@ -70,9 +66,7 @@ impl<'a> SenderDataFinder<'a, Store> { }; finder.have_device_keys(&device_keys).await } -} -impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { async fn have_event( &self, sender_curve_key: Curve25519PublicKey, @@ -347,56 +341,20 @@ impl<'a, S: FinderCryptoStore> SenderDataFinder<'a, S> { } } -#[cfg_attr(target_arch = "wasm32", async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait)] -pub(crate) trait FinderCryptoStore { - async fn get_device_from_curve_key( - &self, - user_id: &UserId, - curve_key: Curve25519PublicKey, - ) -> store::Result>; - - async fn get_user_identity( - &self, - user_id: &UserId, - ) -> OlmResult>; -} - -#[cfg_attr(target_arch = "wasm32", async_trait(?Send))] -#[cfg_attr(not(target_arch = "wasm32"), async_trait)] -impl FinderCryptoStore for Store { - async fn get_device_from_curve_key( - &self, - user_id: &UserId, - curve_key: Curve25519PublicKey, - ) -> store::Result> { - self.get_device_from_curve_key(user_id, curve_key).await - } - - async fn get_user_identity( - &self, - user_id: &UserId, - ) -> OlmResult> { - Ok(self.deref().get_user_identity(user_id).await?) - } -} - #[cfg(test)] mod tests { use std::{ops::Deref as _, sync::Arc}; use assert_matches2::assert_let; - use async_trait::async_trait; use matrix_sdk_test::async_test; use ruma::{device_id, owned_room_id, user_id, DeviceId, OwnedUserId, UserId}; use tokio::sync::Mutex; use vodozemac::{megolm::SessionKey, Curve25519PublicKey, Ed25519PublicKey}; - use super::{FinderCryptoStore, SenderDataFinder}; + use super::SenderDataFinder; use crate::{ - error::OlmResult, olm::{PrivateCrossSigningIdentity, SenderData}, - store::{self, Changes, CryptoStoreWrapper, MemoryStore, Store}, + store::{Changes, CryptoStoreWrapper, MemoryStore, Store}, types::events::{ olm_v1::DecryptedRoomKeyEvent, room_key::{MegolmV1AesSha2Content, RoomKeyContent}, @@ -406,7 +364,7 @@ mod tests { ReadOnlyUserIdentity, }; - impl<'a> SenderDataFinder<'a, Store> { + impl<'a> SenderDataFinder<'a> { fn new(own_crypto_store: &'a Store, own_user_id: &'a UserId) -> Self { Self { own_crypto_store, own_user_id } } @@ -1001,35 +959,4 @@ mod tests { .unwrap(), ) } - - struct FakeCryptoStore { - device: Option, - user_identities: Option, - own_user_identities: Option, - } - - #[cfg_attr(target_arch = "wasm32", async_trait(?Send))] - #[cfg_attr(not(target_arch = "wasm32"), async_trait)] - impl FinderCryptoStore for FakeCryptoStore { - async fn get_device_from_curve_key( - &self, - _user_id: &UserId, - _curve_key: Curve25519PublicKey, - ) -> store::Result> { - Ok(self.device.clone()) - } - - async fn get_user_identity( - &self, - user_id: &UserId, - ) -> OlmResult> { - if user_id == user_id!("@myself:s.co") { - // We are being asked for our own user identity, different from - // self.user_identities because that one was of Other type. - Ok(self.own_user_identities.clone()) - } else { - Ok(self.user_identities.clone()) - } - } - } } From 5a604d716f622337754ebbd2826b34472be2a8c7 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 14:25:45 +0100 Subject: [PATCH 09/27] fixup: remove unnecessary clone --- .../src/olm/group_sessions/sender_data_finder.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index a14a7c05836..451b3d71cd2 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -742,7 +742,7 @@ mod tests { } fn sender_device_curve_key(&self) -> Curve25519PublicKey { - self.sender_device.curve25519_key().unwrap().clone() + self.sender_device.curve25519_key().unwrap() } fn sender_master_key(&self) -> Ed25519PublicKey { From b8cb11e8b69a2ded53b0c8b4a3e4bbf7a4c83136 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 14:31:46 +0100 Subject: [PATCH 10/27] fixup: Fix doc link --- .../src/olm/group_sessions/sender_data_finder.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 451b3d71cd2..b349fb1c360 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -26,7 +26,8 @@ use crate::{ }; /// Temporary struct that is used to look up [`SenderData`] based on the -/// information supplied with in [`InboundGroupSession`]. +/// information supplied in +/// [`crate::types::events::olm_v1::DecryptedRoomKeyEvent`]. /// /// The letters A, B etc. in the documentation refer to the algorithm described /// in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 From df5b352c67061057e920db24a9b939c74874663d Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 15:04:45 +0100 Subject: [PATCH 11/27] fixup: Forget user_id since Store already knows it --- .../olm/group_sessions/sender_data_finder.rs | 79 ++++++++----------- 1 file changed, 33 insertions(+), 46 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index b349fb1c360..eed649ccd4d 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -12,7 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use ruma::UserId; use tracing::error; use vodozemac::Curve25519PublicKey; @@ -32,8 +31,7 @@ use crate::{ /// The letters A, B etc. in the documentation refer to the algorithm described /// in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 pub(crate) struct SenderDataFinder<'a> { - own_crypto_store: &'a Store, - own_user_id: &'a UserId, + store: &'a Store, } impl<'a> SenderDataFinder<'a> { @@ -47,10 +45,7 @@ impl<'a> SenderDataFinder<'a> { sender_curve_key: Curve25519PublicKey, room_key_event: &'a DecryptedRoomKeyEvent, ) -> OlmResult { - let finder = Self { - own_crypto_store: own_olm_machine.store(), - own_user_id: own_olm_machine.user_id(), - }; + let finder = Self { store: own_olm_machine.store() }; finder.have_event(sender_curve_key, room_key_event).await } @@ -61,10 +56,7 @@ impl<'a> SenderDataFinder<'a> { own_olm_machine: &'a OlmMachine, device_keys: DeviceKeys, ) -> OlmResult { - let finder = Self { - own_crypto_store: own_olm_machine.store(), - own_user_id: own_olm_machine.user_id(), - }; + let finder = Self { store: own_olm_machine.store() }; finder.have_device_keys(&device_keys).await } @@ -99,10 +91,8 @@ impl<'a> SenderDataFinder<'a> { // // Does the locally-cached (in the store) devices list contain a device with the // curve key of the sender of the to-device message? - if let Some(sender_device) = self - .own_crypto_store - .get_device_from_curve_key(&room_key_event.sender, sender_curve_key) - .await? + if let Some(sender_device) = + self.store.get_device_from_curve_key(&room_key_event.sender, sender_curve_key).await? { // Yes: use the device info to continue self.have_device(sender_device.inner).await @@ -202,7 +192,7 @@ impl<'a> SenderDataFinder<'a> { // Do we have the cross-signing key for this user? let sender_user_id = sender_device.user_id(); - let sender_user_identity = self.own_crypto_store.get_user_identity(sender_user_id).await?; + let sender_user_identity = self.store.get_user_identity(sender_user_id).await?; if let Some(sender_user_identity) = sender_user_identity { // Yes: check the device is signed by the identity @@ -334,11 +324,7 @@ impl<'a> SenderDataFinder<'a> { /// Return the user identity of the current user, or None if we failed to /// find it (which is unexpected) async fn own_identity(&self) -> OlmResult> { - Ok(self - .own_crypto_store - .get_user_identity(self.own_user_id) - .await? - .and_then(|i| i.into_own())) + Ok(self.store.get_user_identity(self.store.user_id()).await?.and_then(|i| i.into_own())) } } @@ -366,8 +352,8 @@ mod tests { }; impl<'a> SenderDataFinder<'a> { - fn new(own_crypto_store: &'a Store, own_user_id: &'a UserId) -> Self { - Self { own_crypto_store, own_user_id } + fn new(store: &'a Store) -> Self { + Self { store } } } @@ -384,7 +370,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -414,7 +400,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -448,7 +434,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -482,7 +468,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -512,7 +498,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -541,7 +527,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -570,7 +556,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -599,7 +585,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -628,7 +614,7 @@ mod tests { sender_is_verified: true, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -659,7 +645,7 @@ mod tests { sender_is_verified: true, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we try to find sender data let sender_data = finder @@ -689,7 +675,7 @@ mod tests { sender_is_verified: false, }) .await; - let finder = SenderDataFinder::new(&setup.store, &setup.me.user_id); + let finder = SenderDataFinder::new(&setup.store); // When we supply the device keys directly while asking for the sender data let sender_data = @@ -714,7 +700,6 @@ mod tests { } struct TestSetup { - me: TestUser, sender: TestUser, sender_device: Device, store: Store, @@ -732,14 +717,14 @@ mod tests { create_unsigned_device(&sender.account) }; - let store = create_store(&sender); + let store = create_store(&me); save_to_store(&store, &me, &sender, &sender_device, &options).await; let room_key_event = create_room_key_event(&sender.user_id, &me.user_id, &sender_device, &options); - Self { me, sender, sender_device, store, room_key_event } + Self { sender, sender_device, store, room_key_event } } fn sender_device_curve_key(&self) -> Curve25519PublicKey { @@ -751,21 +736,23 @@ mod tests { } } - fn create_store(sender: &TestUser) -> Store { - let store_wrapper = Arc::new(CryptoStoreWrapper::new(&sender.user_id, MemoryStore::new())); + fn create_store(me: &TestUser) -> Store { + let store_wrapper = Arc::new(CryptoStoreWrapper::new(&me.user_id, MemoryStore::new())); let verification_machine = VerificationMachine::new( - sender.account.deref().clone(), - Arc::clone(&sender.private_identity), - Arc::new(CryptoStoreWrapper::new(&sender.user_id, MemoryStore::new())), + me.account.deref().clone(), + Arc::clone(&me.private_identity), + Arc::clone(&store_wrapper), ); - Store::new( - sender.account.static_data.clone(), - Arc::clone(&sender.private_identity), + let store = Store::new( + me.account.static_data.clone(), + Arc::clone(&me.private_identity), store_wrapper, verification_machine, - ) + ); + + store } async fn save_to_store( From d74561c3904781d156ad1ff93494932c00c5cdeb Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 15:09:00 +0100 Subject: [PATCH 12/27] fixup: Take a Store directly in SenderDataFinder --- crates/matrix-sdk-crypto/src/machine.rs | 10 +++++++--- .../src/olm/group_sessions/sender_data_finder.rs | 11 +++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 6d9542cf79a..4a081267657 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -817,7 +817,8 @@ impl OlmMachine { event: &DecryptedRoomKeyEvent, content: &MegolmV1AesSha2Content, ) -> OlmResult> { - let sender_data = SenderDataFinder::find_using_event(self, sender_key, event).await?; + let sender_data = + SenderDataFinder::find_using_event(self.store(), sender_key, event).await?; let session = InboundGroupSession::new( sender_key, @@ -1032,8 +1033,11 @@ impl OlmMachine { let device = self.store().get_device(account.user_id(), account.device_id()).await; let own_sender_data = match device { Ok(Some(device)) => { - SenderDataFinder::find_using_device_keys(self, device.as_device_keys().clone()) - .await? + SenderDataFinder::find_using_device_keys( + self.store(), + device.as_device_keys().clone(), + ) + .await? } _ => { error!("Unable to find our own device!"); diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index eed649ccd4d..f2accbe7a3d 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -20,8 +20,7 @@ use crate::{ error::OlmResult, store::Store, types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys, MasterPubkey}, - EventError, OlmError, OlmMachine, ReadOnlyDevice, ReadOnlyOwnUserIdentity, - ReadOnlyUserIdentities, + EventError, OlmError, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, }; /// Temporary struct that is used to look up [`SenderData`] based on the @@ -41,11 +40,11 @@ impl<'a> SenderDataFinder<'a> { /// create the InboundGroupSession we are about to create, and decide /// whether we trust the sender. pub(crate) async fn find_using_event( - own_olm_machine: &'a OlmMachine, + store: &'a Store, sender_curve_key: Curve25519PublicKey, room_key_event: &'a DecryptedRoomKeyEvent, ) -> OlmResult { - let finder = Self { store: own_olm_machine.store() }; + let finder = Self { store }; finder.have_event(sender_curve_key, room_key_event).await } @@ -53,10 +52,10 @@ impl<'a> SenderDataFinder<'a> { /// and https://github.com/matrix-org/matrix-rust-sdk/issues/3544 /// use the supplied device info to decide whether we trust the sender. pub(crate) async fn find_using_device_keys( - own_olm_machine: &'a OlmMachine, + store: &'a Store, device_keys: DeviceKeys, ) -> OlmResult { - let finder = Self { store: own_olm_machine.store() }; + let finder = Self { store }; finder.have_device_keys(&device_keys).await } From 93ba085fd6e1511dbbf7dac10ad5467a729ad2a9 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 15:23:36 +0100 Subject: [PATCH 13/27] fixup: Calculate SenderData inside share_room_key --- crates/matrix-sdk-crypto/src/machine.rs | 30 ++++--------------- .../src/session_manager/group_sessions/mod.rs | 25 ++++++++++++++-- 2 files changed, 29 insertions(+), 26 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 4a081267657..b8b1ffd2586 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -66,7 +66,7 @@ use crate::{ identities::{user::UserIdentities, Device, IdentityManager, UserDevices}, olm::{ Account, CrossSigningStatus, EncryptionSettings, IdentityKeys, InboundGroupSession, - OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderData, SenderDataFinder, SessionType, + OlmDecryptionInfo, PrivateCrossSigningIdentity, SenderDataFinder, SessionType, StaticAccountData, }, requests::{IncomingResponse, OutgoingRequest, UploadSigningKeysRequest}, @@ -899,6 +899,8 @@ impl OlmMachine { &self, room_id: &RoomId, ) -> OlmResult<()> { + use crate::olm::SenderData; + let (_, session) = self .inner .group_session_manager @@ -920,6 +922,8 @@ impl OlmMachine { &self, room_id: &RoomId, ) -> OlmResult { + use crate::olm::SenderData; + let (_, session) = self .inner .group_session_manager @@ -1026,29 +1030,7 @@ impl OlmMachine { users: impl Iterator, encryption_settings: impl Into, ) -> OlmResult>> { - // Use our own device info to populate the SenderData that validates the - // InboundGroupSession that we create as a pair to the OutboundGroupSession we - // are sending out. - let account = self.store().static_account(); - let device = self.store().get_device(account.user_id(), account.device_id()).await; - let own_sender_data = match device { - Ok(Some(device)) => { - SenderDataFinder::find_using_device_keys( - self.store(), - device.as_device_keys().clone(), - ) - .await? - } - _ => { - error!("Unable to find our own device!"); - SenderData::unknown() - } - }; - - self.inner - .group_session_manager - .share_room_key(room_id, users, encryption_settings, own_sender_data) - .await + self.inner.group_session_manager.share_room_key(room_id, users, encryption_settings).await } /// Receive an unencrypted verification event. 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 da18a5955a4..b2500363d30 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 @@ -36,7 +36,10 @@ use tracing::{debug, error, info, instrument, trace}; use crate::{ error::{EventError, MegolmResult, OlmResult}, identities::device::MaybeEncryptedRoomKey, - olm::{InboundGroupSession, OutboundGroupSession, SenderData, Session, ShareInfo, ShareState}, + olm::{ + InboundGroupSession, OutboundGroupSession, SenderData, SenderDataFinder, Session, + ShareInfo, ShareState, + }, store::{Changes, CryptoStoreWrapper, Result as StoreResult, Store}, types::events::{room::encrypted::RoomEncryptedEventContent, room_key_withheld::WithheldCode}, EncryptionSettings, OlmError, ReadOnlyDevice, ToDeviceRequest, @@ -638,13 +641,31 @@ impl GroupSessionManager { room_id: &RoomId, users: impl Iterator, encryption_settings: impl Into, - own_sender_data: SenderData, ) -> OlmResult>> { trace!("Checking if a room key needs to be shared"); let encryption_settings = encryption_settings.into(); let mut changes = Changes::default(); + // Use our own device info to populate the SenderData that validates the + // InboundGroupSession that we create as a pair to the OutboundGroupSession we + // are sending out. + let account = self.store.static_account(); + let device = self.store.get_device(account.user_id(), account.device_id()).await; + let own_sender_data = match device { + Ok(Some(device)) => { + SenderDataFinder::find_using_device_keys( + &self.store, + device.as_device_keys().clone(), + ) + .await? + } + _ => { + error!("Unable to find our own device!"); + SenderData::unknown() + } + }; + // Try to get an existing session or create a new one. let (outbound, inbound) = self .get_or_create_outbound_session( From ee787227e670861c7fc6720e914c2a1ee021bc1b Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 16:42:56 +0100 Subject: [PATCH 14/27] fixup: Include a flow chart explaining SenderDataFinder --- .../olm/group_sessions/sender_data_finder.rs | 171 +++++++++++++----- 1 file changed, 122 insertions(+), 49 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index f2accbe7a3d..dd267847cd9 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -27,8 +27,124 @@ use crate::{ /// information supplied in /// [`crate::types::events::olm_v1::DecryptedRoomKeyEvent`]. /// -/// The letters A, B etc. in the documentation refer to the algorithm described -/// in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 +/// # Algorithm +/// +/// When we receive a to-device message establishing a megolm session (i.e. when +/// [`crate::machine::OlmMachine::add_room_key`] is called): +/// +/// ┌───────────────────────────────────────────────────────────────────┐ +/// │ A (start - we have a to-device message containing a room key) │ +/// └───────────────────────────────────────────────────────────────────┘ +/// │ +/// __________________________________▼______________________________ +/// ╱ ╲ +/// ╱ Does the to-device message contain the device_keys property from ╲yes +/// ╲ MSC4147? ╱ │ +/// ╲_________________________________________________________________╱ │ +/// │ no │ +/// ▼ │ +/// ┌───────────────────────────────────────────────────────────────────┐ │ +/// │ B (there is no device info in the to-device message) │ │ +/// │ │ │ +/// │ We need to find the device details. │ │ +/// └───────────────────────────────────────────────────────────────────┘ │ +/// │ │ +/// __________________________________▼______________________________ │ +/// ╱ ╲ │ +/// ╱ Does the store contain a device whose curve key matches the ╲ ▼ +/// ╲ sender of the to-device message? ╱yes +/// ╲_________________________________________________________________╱ │ +/// │ no │ +/// ▼ │ +/// ╭───────────────────────────────────────────────────────────────────╮ │ +/// │ C (there is no device info locally) │ │ +/// │ │ │ +/// │ Give up: we have no sender info for this room key. │ │ +/// ╰───────────────────────────────────────────────────────────────────╯ │ +/// ┌─────────────────────────────────┘ +/// ▼ +/// ┌───────────────────────────────────────────────────────────────────┐ +/// │ D (we have device info) │ +/// └───────────────────────────────────────────────────────────────────┘ +/// │ +/// __________________________________▼______________________________ +/// ╱ ╲ +/// ╱ Is the device info cross-signed? ╲yes +/// ╲___________________________________________________________________╱ │ +/// │ no │ +/// ▼ │ +/// ╭───────────────────────────────────────────────────────────────────╮ │ +/// │ Give up: unsigned device info is useless. │ │ +/// ╰───────────────────────────────────────────────────────────────────╯ │ +/// ┌─────────────────────────────────┘ +/// ▼ +/// ┌───────────────────────────────────────────────────────────────────┐ +/// │ E (we have cross-signed device info) │ +/// └───────────────────────────────────────────────────────────────────┘ +/// │ +/// __________________________________▼______________________________ +/// ╱ ╲ +/// ╱ Do we have the cross-signing key for this user? ╲yes +/// ╲___________________________________________________________________╱ │ +/// │ no │ +/// ▼ │ +/// ┌───────────────────────────────────────────────────────────────────┐ │ +/// │ F (we have cross-signed device info, but no cross-signing keys) │ │ +/// └───────────────────────────────────────────────────────────────────┘ │ +/// │ │ +/// ▼ │ +/// ╭───────────────────────────────────────────────────────────────────╮ │ +/// │ Store the device info with the session, in case we can │ │ +/// │ confirm it later. │ │ +/// ╰───────────────────────────────────────────────────────────────────╯ │ +/// ┌─────────────────────────────────┘ +/// ▼ +/// ┌───────────────────────────────────────────────────────────────────┐ +/// │ G (we have a cross-signing key for the sender) │ +/// └───────────────────────────────────────────────────────────────────┘ +/// │ +/// __________________________________▼______________________________ +/// ╱ ╲ +/// ╱ Does the cross-signing key match that used ╲yes +/// ╲ to sign the device info? ╱ │ +/// ╲_________________________________________________________________╱ │ +/// │ no │ +/// ▼ │ +/// ╭───────────────────────────────────────────────────────────────────╮ │ +/// │ Store the device info with the session, in case we get the │ │ +/// │ right cross-signing key later. │ │ +/// ╰───────────────────────────────────────────────────────────────────╯ │ +/// ┌─────────────────────────────────┘ +/// ▼ +/// ┌───────────────────────────────────────────────────────────────────┐ +/// │ H (cross-signing key matches that used to sign the device info!) │ +/// └───────────────────────────────────────────────────────────────────┘ +/// │ +/// __________________________________▼______________________________ +/// ╱ ╲ +/// ╱ Is the signature in the device info valid? ╲yes +/// ╲___________________________________________________________________╱ │ +/// │ no │ +/// ▼ │ +/// ╭───────────────────────────────────────────────────────────────────╮ │ +/// │ !Session is invalid: drop it from the store and forget it. │ │ +/// ╰───────────────────────────────────────────────────────────────────╯ │ +/// ┌─────────────────────────────────┘ +/// ▼ +/// ╭───────────────────────────────────────────────────────────────────╮ +/// │ J (device info is verified by matching cross-signing key) │ +/// │ │ +/// │ Look up the user_id and master_key for the user sending the │ +/// │ to-device message. │ +/// │ │ +/// │ Decide the master_key trust level based on whether we have │ +/// │ verified this user. │ +/// │ │ +/// │ Store this information with the session. │ +/// ╰───────────────────────────────────────────────────────────────────╯ +/// +/// Note: the sender data may become out-of-date if we later verify the user. We +/// have no plans to update it if so. pub(crate) struct SenderDataFinder<'a> { store: &'a Store, } @@ -64,12 +180,7 @@ impl<'a> SenderDataFinder<'a> { sender_curve_key: Curve25519PublicKey, room_key_event: &'a DecryptedRoomKeyEvent, ) -> OlmResult { - // A (start) - // - // TODO: take the session lock for session_id - // TODO: if we fail to get the lock, we bail out immediately. Does this open an - // attack vector for someone to use someone else's session ID and send - // invalid sessions? + // A (start - we have a to-device message containing a room key) // // Does the to-device message contain the device_keys property from MSC4147? if let Some(sender_device_keys) = &room_key_event.device_keys { @@ -109,22 +220,6 @@ impl<'a> SenderDataFinder<'a> { // we may not have had a proper chance to look up the sender data. legacy_session: true, }; - // sender_data will be persisted to the store when this function returns to - // `handle_key`, meaning that if our process is killed, we will still retry it - // later. - // - // Switch to the "slow lane" (don't block sync, but retry after /keys/query - // returns). - // - // TODO: kick off an async task [keep the lock]: run - // OlmMachine::get_user_devices (which waits for /keys/query to complete, then - // repeats the lookup we did above for the device that matches this session. - // - // If the device is there, -> D - // - // If we still don’t have the device info, -> Wait to see whether we get device - // info later. Increment retry_count and set next_retry_time_ms per backoff - // algorithm; let the background job pick it up [drop the lock] Ok(sender_data) } } @@ -177,11 +272,6 @@ impl<'a> SenderDataFinder<'a> { // We will need new, cross-signed device info for this to work, so there is no // point storing the device info we have in the session. Ok(SenderData::unknown()) - - // TODO: Wait to see if the device becomes cross-signed soon. - // Increment retry_count and set next_retry_time_ms per - // backoff algorithm; let the background job pick it up [drop - // the lock] } } @@ -198,21 +288,11 @@ impl<'a> SenderDataFinder<'a> { self.have_user_cross_signing_keys(sender_device, sender_user_identity).await } else { // No: F (we have cross-signed device info, but no cross-signing keys) - - // TODO: bump the retry count + time - Ok(SenderData::DeviceInfo { device_keys: sender_device.as_device_keys().clone(), retry_details: SenderDataRetryDetails::retry_soon(), legacy_session: true, // TODO: change to false when we have all the retry code }) - - // TODO: Return, and kick off an async task - // [keep the lock]: run OlmMachine::get_identity (which waits for - // /keys/query to complete, then fetches this user's cross-signing - // key from the store.) If we still don’t have a cross-signing key - // -> Wait to see if we get one soon. Do nothing; let the - // background job pick it up [drop the lock] } } @@ -235,17 +315,15 @@ impl<'a> SenderDataFinder<'a> { // Find the actual key within the MasterPubkey struct if let Some(master_key) = master_key.get_first_key() { - // We have MXID and master_key for the user sending the to-device message. + // We have user_id and master_key for the user sending the to-device message. // Decide the master_key trust level based on whether we have verified this - // user. Set the MXID, master_key and trust level in the - // session. Remove the device info and retries since we don't - // need them. + // user. Set the user_id, master_key and trust level in the + // session. Ok(SenderData::SenderKnown { user_id: sender_device.user_id().to_owned(), master_key, master_key_verified, }) - // TODO: [drop the lock] } else { // Surprisingly, there was no key in the MasterPubkey. We did not expect this: // treat it as if the device was not signed by this master key. @@ -260,7 +338,6 @@ impl<'a> SenderDataFinder<'a> { retry_details: SenderDataRetryDetails::retry_soon(), legacy_session: true, // TODO: change to false when retries etc. are done }) - // TODO: [drop the lock] } } else { // No: Device was not signed by the known identity of the sender. @@ -276,10 +353,6 @@ impl<'a> SenderDataFinder<'a> { retry_details: SenderDataRetryDetails::retry_soon(), legacy_session: true, // TODO: change to false when retries etc. are done }) - - // TODO: Increment retry_count and set - // next_retry_time_ms per backoff algorithm; let the background job - // pick it up [drop the lock] } } From a4000421c1754c764065f8cbde4948051a887139 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 17:05:34 +0100 Subject: [PATCH 15/27] fixup: Remove excess let binding --- .../src/olm/group_sessions/sender_data_finder.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index dd267847cd9..c8180777dd7 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -817,14 +817,12 @@ mod tests { Arc::clone(&store_wrapper), ); - let store = Store::new( + Store::new( me.account.static_data.clone(), Arc::clone(&me.private_identity), store_wrapper, verification_machine, - ); - - store + ) } async fn save_to_store( From 5168210b4a4eebb7e89792492d15abb9f340922a Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Fri, 12 Jul 2024 17:28:08 +0100 Subject: [PATCH 16/27] fixup: Adapt to renames --- .../olm/group_sessions/sender_data_finder.rs | 74 +++++++++---------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index c8180777dd7..689785f0e4b 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -20,7 +20,7 @@ use crate::{ error::OlmResult, store::Store, types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys, MasterPubkey}, - EventError, OlmError, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, + DeviceData, EventError, OlmError, OwnUserIdentityData, UserIdentityData, }; /// Temporary struct that is used to look up [`SenderData`] based on the @@ -226,7 +226,7 @@ impl<'a> SenderDataFinder<'a> { async fn have_device_keys(&self, sender_device_keys: &DeviceKeys) -> OlmResult { // Validate the signature of the DeviceKeys supplied. - if let Ok(sender_device) = ReadOnlyDevice::try_from(sender_device_keys) { + if let Ok(sender_device) = DeviceData::try_from(sender_device_keys) { self.have_device(sender_device).await } else { // The device keys supplied did not validate. @@ -239,14 +239,14 @@ impl<'a> SenderDataFinder<'a> { /// Step D from https://github.com/matrix-org/matrix-rust-sdk/issues/3543 /// We have device info for the sender of this to-device message. Look up /// whether it's cross-signed. - async fn have_device(&self, sender_device: ReadOnlyDevice) -> OlmResult { + async fn have_device(&self, sender_device: DeviceData) -> OlmResult { // D (we have device info) // // Is the device info cross-signed? let user_id = sender_device.user_id(); let Some(signatures) = sender_device.signatures().get(user_id) else { - // This should never happen: we would not have managed to get a ReadOnlyDevice + // This should never happen: we would not have managed to get a DeviceData // if it did not contain a signature. error!( "Found a device for user_id {user_id} but it has no signatures for that user id!" @@ -257,7 +257,7 @@ impl<'a> SenderDataFinder<'a> { }; // Count number of signatures - we know there is 1, because we would not have - // been able to construct a ReadOnlyDevice without a signature. + // been able to construct a DeviceData without a signature. // If there are more than 1, we assume this device was cross-signed by some // identity. if signatures.len() > 1 { @@ -275,7 +275,7 @@ impl<'a> SenderDataFinder<'a> { } } - async fn device_is_cross_signed(&self, sender_device: ReadOnlyDevice) -> OlmResult { + async fn device_is_cross_signed(&self, sender_device: DeviceData) -> OlmResult { // E (we have cross-signed device info) // // Do we have the cross-signing key for this user? @@ -298,8 +298,8 @@ impl<'a> SenderDataFinder<'a> { async fn have_user_cross_signing_keys( &self, - sender_device: ReadOnlyDevice, - sender_user_identity: ReadOnlyUserIdentities, + sender_device: DeviceData, + sender_user_identity: UserIdentityData, ) -> OlmResult { // G (we have cross-signing key) // @@ -361,18 +361,18 @@ impl<'a> SenderDataFinder<'a> { /// Otherwise, return None. async fn master_key_if_device_is_signed_by_user<'i>( &self, - sender_device: &'_ ReadOnlyDevice, - sender_user_identity: &'i ReadOnlyUserIdentities, + sender_device: &'_ DeviceData, + sender_user_identity: &'i UserIdentityData, ) -> OlmResult> { Ok(match sender_user_identity { - ReadOnlyUserIdentities::Own(own_identity) => { + UserIdentityData::Own(own_identity) => { if own_identity.is_device_signed(sender_device).is_ok() { Some((own_identity.master_key(), own_identity.is_verified())) } else { None } } - ReadOnlyUserIdentities::Other(other_identity) => { + UserIdentityData::Other(other_identity) => { if other_identity.is_device_signed(sender_device).is_ok() { let master_key = other_identity.master_key(); @@ -395,7 +395,7 @@ impl<'a> SenderDataFinder<'a> { /// Return the user identity of the current user, or None if we failed to /// find it (which is unexpected) - async fn own_identity(&self) -> OlmResult> { + async fn own_identity(&self) -> OlmResult> { Ok(self.store.get_user_identity(self.store.user_id()).await?.and_then(|i| i.into_own())) } } @@ -419,8 +419,7 @@ mod tests { room_key::{MegolmV1AesSha2Content, RoomKeyContent}, }, verification::VerificationMachine, - Account, Device, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentities, - ReadOnlyUserIdentity, + Account, Device, DeviceData, OtherUserIdentityData, OwnUserIdentityData, UserIdentityData, }; impl<'a> SenderDataFinder<'a> { @@ -804,7 +803,7 @@ mod tests { } fn sender_master_key(&self) -> Ed25519PublicKey { - self.sender.user_identities.master_key().get_first_key().unwrap() + self.sender.user_identity.master_key().get_first_key().unwrap() } } @@ -841,12 +840,12 @@ mod tests { // Add the sender identity to the store if options.store_contains_sender_identity { - changes.identities.new.push(sender.user_identities.clone()); + changes.identities.new.push(sender.user_identity.clone()); } // If it's different from the sender, add our identity too if !options.sender_is_ourself { - changes.identities.new.push(me.user_identities.clone()); + changes.identities.new.push(me.user_identity.clone()); } store.save_changes(changes).await.unwrap(); @@ -856,7 +855,7 @@ mod tests { user_id: OwnedUserId, account: Account, private_identity: Arc>, - user_identities: ReadOnlyUserIdentities, + user_identity: UserIdentityData, } impl TestUser { @@ -871,11 +870,11 @@ mod tests { let user_id = user_id.to_owned(); let private_identity = Arc::new(Mutex::new(create_private_identity(&account).await)); - let user_identities = - create_user_identities(&*private_identity.lock().await, is_me, is_verified, signer) + let user_identity = + create_user_identity(&*private_identity.lock().await, is_me, is_verified, signer) .await; - Self { user_id, account, private_identity, user_identities } + Self { user_id, account, private_identity, user_identity } } async fn own() -> Self { @@ -897,34 +896,35 @@ mod tests { } } - async fn create_user_identities( + async fn create_user_identity( private_identity: &PrivateCrossSigningIdentity, is_me: bool, is_verified: bool, signer: Option<&TestUser>, - ) -> ReadOnlyUserIdentities { + ) -> UserIdentityData { if is_me { - let user_identity = ReadOnlyOwnUserIdentity::from_private(private_identity).await; + let own_user_identity = OwnUserIdentityData::from_private(private_identity).await; if is_verified { - user_identity.mark_as_verified(); + own_user_identity.mark_as_verified(); } - ReadOnlyUserIdentities::Own(user_identity) + UserIdentityData::Own(own_user_identity) } else { - let mut user_identity = ReadOnlyUserIdentity::from_private(private_identity).await; + let mut other_user_identity = + OtherUserIdentityData::from_private(private_identity).await; if is_verified { - sign_other_identity(signer, &mut user_identity).await; + sign_other_identity(signer, &mut other_user_identity).await; } - ReadOnlyUserIdentities::Other(user_identity) + UserIdentityData::Other(other_user_identity) } } async fn sign_other_identity( signer: Option<&TestUser>, - user_identity: &mut ReadOnlyUserIdentity, + other_user_identity: &mut OtherUserIdentityData, ) { if let Some(signer) = signer { let signer_private_identity = signer.private_identity.lock().await; @@ -932,10 +932,10 @@ mod tests { let user_signing = signer_private_identity.user_signing_key.lock().await; let user_signing = user_signing.as_ref().unwrap(); - let master = user_signing.sign_user(&*user_identity).unwrap(); - user_identity.master_key = Arc::new(master.try_into().unwrap()); + let master = user_signing.sign_user(&*other_user_identity).unwrap(); + other_user_identity.master_key = Arc::new(master.try_into().unwrap()); - user_signing.public_key().verify_master_key(user_identity.master_key()).unwrap(); + user_signing.public_key().verify_master_key(other_user_identity.master_key()).unwrap(); } else { panic!("You must provide a `signer` if you want an Other to be verified!"); } @@ -949,7 +949,7 @@ mod tests { account: &Account, private_identity: &PrivateCrossSigningIdentity, ) -> Device { - let mut read_only_device = ReadOnlyDevice::from_account(account); + let mut read_only_device = DeviceData::from_account(account); let self_signing = private_identity.self_signing_key.lock().await; let self_signing = self_signing.as_ref().unwrap(); @@ -962,10 +962,10 @@ mod tests { } fn create_unsigned_device(account: &Account) -> Device { - wrap_device(account, ReadOnlyDevice::from_account(account)) + wrap_device(account, DeviceData::from_account(account)) } - fn wrap_device(account: &Account, read_only_device: ReadOnlyDevice) -> Device { + fn wrap_device(account: &Account, read_only_device: DeviceData) -> Device { Device { inner: read_only_device, verification_machine: VerificationMachine::new( From 4fef0a6fca43e606111acb7f39144b02b7aba3bb Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 15 Jul 2024 12:01:01 +0100 Subject: [PATCH 17/27] fixup: Allow creating Devices in Store, and pass around devices in SenderDataFinder --- .../olm/group_sessions/sender_data_finder.rs | 20 +++++++++-------- crates/matrix-sdk-crypto/src/store/mod.rs | 22 +++++++++++++++---- 2 files changed, 29 insertions(+), 13 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 689785f0e4b..12959bfcff9 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -20,7 +20,7 @@ use crate::{ error::OlmResult, store::Store, types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys, MasterPubkey}, - DeviceData, EventError, OlmError, OwnUserIdentityData, UserIdentityData, + Device, DeviceData, EventError, OlmError, OwnUserIdentityData, UserIdentityData, }; /// Temporary struct that is used to look up [`SenderData`] based on the @@ -205,7 +205,7 @@ impl<'a> SenderDataFinder<'a> { self.store.get_device_from_curve_key(&room_key_event.sender, sender_curve_key).await? { // Yes: use the device info to continue - self.have_device(sender_device.inner).await + self.have_device(sender_device).await } else { // C (no device info locally) // @@ -226,7 +226,9 @@ impl<'a> SenderDataFinder<'a> { async fn have_device_keys(&self, sender_device_keys: &DeviceKeys) -> OlmResult { // Validate the signature of the DeviceKeys supplied. - if let Ok(sender_device) = DeviceData::try_from(sender_device_keys) { + if let Ok(sender_device_data) = DeviceData::try_from(sender_device_keys) { + let sender_device = self.store.create_device(sender_device_data).await?; + self.have_device(sender_device).await } else { // The device keys supplied did not validate. @@ -239,14 +241,14 @@ impl<'a> SenderDataFinder<'a> { /// Step D from https://github.com/matrix-org/matrix-rust-sdk/issues/3543 /// We have device info for the sender of this to-device message. Look up /// whether it's cross-signed. - async fn have_device(&self, sender_device: DeviceData) -> OlmResult { + async fn have_device(&self, sender_device: Device) -> OlmResult { // D (we have device info) // // Is the device info cross-signed? let user_id = sender_device.user_id(); let Some(signatures) = sender_device.signatures().get(user_id) else { - // This should never happen: we would not have managed to get a DeviceData + // This should never happen: we would not have managed to get a Device // if it did not contain a signature. error!( "Found a device for user_id {user_id} but it has no signatures for that user id!" @@ -257,7 +259,7 @@ impl<'a> SenderDataFinder<'a> { }; // Count number of signatures - we know there is 1, because we would not have - // been able to construct a DeviceData without a signature. + // been able to construct a Device without a signature. // If there are more than 1, we assume this device was cross-signed by some // identity. if signatures.len() > 1 { @@ -275,7 +277,7 @@ impl<'a> SenderDataFinder<'a> { } } - async fn device_is_cross_signed(&self, sender_device: DeviceData) -> OlmResult { + async fn device_is_cross_signed(&self, sender_device: Device) -> OlmResult { // E (we have cross-signed device info) // // Do we have the cross-signing key for this user? @@ -298,7 +300,7 @@ impl<'a> SenderDataFinder<'a> { async fn have_user_cross_signing_keys( &self, - sender_device: DeviceData, + sender_device: Device, sender_user_identity: UserIdentityData, ) -> OlmResult { // G (we have cross-signing key) @@ -361,7 +363,7 @@ impl<'a> SenderDataFinder<'a> { /// Otherwise, return None. async fn master_key_if_device_is_signed_by_user<'i>( &self, - sender_device: &'_ DeviceData, + sender_device: &'_ Device, sender_user_identity: &'i UserIdentityData, ) -> OlmResult> { Ok(match sender_user_identity { diff --git a/crates/matrix-sdk-crypto/src/store/mod.rs b/crates/matrix-sdk-crypto/src/store/mod.rs index 5d26c17dee4..dc251e921e8 100644 --- a/crates/matrix-sdk-crypto/src/store/mod.rs +++ b/crates/matrix-sdk-crypto/src/store/mod.rs @@ -1143,20 +1143,34 @@ impl Store { user_id: &UserId, device_id: &DeviceId, ) -> Result> { + if let Some(device_data) = self.inner.store.get_device(user_id, device_id).await? { + Ok(Some(self.create_device(device_data).await?)) + } else { + Ok(None) + } + } + + /// Create a new device using the supplied [`DeviceData`]. Normally we would + /// call [`Self::get_device`] to find an existing device inside this + /// store. Only call this if you have some existing DeviceData and want + /// to wrap it with the extra information provided by a [`Device`]. + pub(crate) async fn create_device(&self, device_data: DeviceData) -> Result { let own_identity = self .inner .store .get_user_identity(self.user_id()) .await? .and_then(|i| i.own().cloned()); - let device_owner_identity = self.inner.store.get_user_identity(user_id).await?; - Ok(self.inner.store.get_device(user_id, device_id).await?.map(|d| Device { - inner: d, + let device_owner_identity = + self.inner.store.get_user_identity(device_data.user_id()).await?; + + Ok(Device { + inner: device_data, verification_machine: self.inner.verification_machine.clone(), own_identity, device_owner_identity, - })) + }) } /// Get the Identity of `user_id` From 831d99d94a05f8ec2a6265290531d90fb701a6a6 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 15 Jul 2024 13:44:47 +0100 Subject: [PATCH 18/27] fixup: rename create_device to wrap_device_data --- .../src/olm/group_sessions/sender_data_finder.rs | 2 +- crates/matrix-sdk-crypto/src/store/mod.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 12959bfcff9..5082e34727f 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -227,7 +227,7 @@ impl<'a> SenderDataFinder<'a> { async fn have_device_keys(&self, sender_device_keys: &DeviceKeys) -> OlmResult { // Validate the signature of the DeviceKeys supplied. if let Ok(sender_device_data) = DeviceData::try_from(sender_device_keys) { - let sender_device = self.store.create_device(sender_device_data).await?; + let sender_device = self.store.wrap_device_data(sender_device_data).await?; self.have_device(sender_device).await } else { diff --git a/crates/matrix-sdk-crypto/src/store/mod.rs b/crates/matrix-sdk-crypto/src/store/mod.rs index dc251e921e8..d6eb6bda53d 100644 --- a/crates/matrix-sdk-crypto/src/store/mod.rs +++ b/crates/matrix-sdk-crypto/src/store/mod.rs @@ -1144,7 +1144,7 @@ impl Store { device_id: &DeviceId, ) -> Result> { if let Some(device_data) = self.inner.store.get_device(user_id, device_id).await? { - Ok(Some(self.create_device(device_data).await?)) + Ok(Some(self.wrap_device_data(device_data).await?)) } else { Ok(None) } @@ -1154,7 +1154,7 @@ impl Store { /// call [`Self::get_device`] to find an existing device inside this /// store. Only call this if you have some existing DeviceData and want /// to wrap it with the extra information provided by a [`Device`]. - pub(crate) async fn create_device(&self, device_data: DeviceData) -> Result { + pub(crate) async fn wrap_device_data(&self, device_data: DeviceData) -> Result { let own_identity = self .inner .store From fb773ae057e41dbe946624c1d428e4388b4e7671 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 15 Jul 2024 13:55:27 +0100 Subject: [PATCH 19/27] fixup: Remove github links and align doc comments with flowchart --- .../olm/group_sessions/sender_data_finder.rs | 28 ++++++------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 5082e34727f..9600e53b3ab 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -150,9 +150,7 @@ pub(crate) struct SenderDataFinder<'a> { } impl<'a> SenderDataFinder<'a> { - /// As described in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 - /// and https://github.com/matrix-org/matrix-rust-sdk/issues/3544 - /// find the device info associated with the to-device message used to + /// Find the device info associated with the to-device message used to /// create the InboundGroupSession we are about to create, and decide /// whether we trust the sender. pub(crate) async fn find_using_event( @@ -164,9 +162,7 @@ impl<'a> SenderDataFinder<'a> { finder.have_event(sender_curve_key, room_key_event).await } - /// As described in https://github.com/matrix-org/matrix-rust-sdk/issues/3543 - /// and https://github.com/matrix-org/matrix-rust-sdk/issues/3544 - /// use the supplied device info to decide whether we trust the sender. + /// Use the supplied device info to decide whether we trust the sender. pub(crate) async fn find_using_device_keys( store: &'a Store, device_keys: DeviceKeys, @@ -175,13 +171,12 @@ impl<'a> SenderDataFinder<'a> { finder.have_device_keys(&device_keys).await } + /// Step A (start - we have a to-device message containing a room key) async fn have_event( &self, sender_curve_key: Curve25519PublicKey, room_key_event: &'a DecryptedRoomKeyEvent, ) -> OlmResult { - // A (start - we have a to-device message containing a room key) - // // Does the to-device message contain the device_keys property from MSC4147? if let Some(sender_device_keys) = &room_key_event.device_keys { // Yes: use the device info to continue @@ -192,13 +187,12 @@ impl<'a> SenderDataFinder<'a> { } } + /// Step B (there is no device info in the to-device message) async fn search_for_device( &self, sender_curve_key: Curve25519PublicKey, room_key_event: &'a DecryptedRoomKeyEvent, ) -> OlmResult { - // B (no device info in to-device message) - // // Does the locally-cached (in the store) devices list contain a device with the // curve key of the sender of the to-device message? if let Some(sender_device) = @@ -207,7 +201,7 @@ impl<'a> SenderDataFinder<'a> { // Yes: use the device info to continue self.have_device(sender_device).await } else { - // C (no device info locally) + // Step C (no device info locally) // // We have no device data for this session so we can't continue in the "fast // lane" (blocking sync). @@ -238,12 +232,8 @@ impl<'a> SenderDataFinder<'a> { } } - /// Step D from https://github.com/matrix-org/matrix-rust-sdk/issues/3543 - /// We have device info for the sender of this to-device message. Look up - /// whether it's cross-signed. + /// Step D (we have device info) async fn have_device(&self, sender_device: Device) -> OlmResult { - // D (we have device info) - // // Is the device info cross-signed? let user_id = sender_device.user_id(); @@ -277,9 +267,8 @@ impl<'a> SenderDataFinder<'a> { } } + /// E (we have cross-signed device info) async fn device_is_cross_signed(&self, sender_device: Device) -> OlmResult { - // E (we have cross-signed device info) - // // Do we have the cross-signing key for this user? let sender_user_id = sender_device.user_id(); @@ -298,13 +287,12 @@ impl<'a> SenderDataFinder<'a> { } } + /// Step G (we have a cross-signing key for the sender) async fn have_user_cross_signing_keys( &self, sender_device: Device, sender_user_identity: UserIdentityData, ) -> OlmResult { - // G (we have cross-signing key) - // // Does the cross-signing key match that used to sign the device info? // And is the signature in the device info valid? let maybe_master_key_info = self From 8f1a4b1b2bcd3be8d15bace75c5216bf5a9e8833 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 15 Jul 2024 16:33:41 +0100 Subject: [PATCH 20/27] fixup: Use methods on Device instead of rolling our own --- .../olm/group_sessions/sender_data_finder.rs | 136 +++++------------- 1 file changed, 34 insertions(+), 102 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 9600e53b3ab..a315c8f0938 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -19,8 +19,8 @@ use super::{SenderData, SenderDataRetryDetails}; use crate::{ error::OlmResult, store::Store, - types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys, MasterPubkey}, - Device, DeviceData, EventError, OlmError, OwnUserIdentityData, UserIdentityData, + types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys}, + Device, DeviceData, EventError, OlmError, }; /// Temporary struct that is used to look up [`SenderData`] based on the @@ -84,7 +84,7 @@ use crate::{ /// │ /// __________________________________▼______________________________ /// ╱ ╲ -/// ╱ Do we have the cross-signing key for this user? ╲yes +/// ╱ Is the device cross-signed by the sender? ╲yes /// ╲___________________________________________________________________╱ │ /// │ no │ /// ▼ │ @@ -100,7 +100,7 @@ use crate::{ /// ┌─────────────────────────────────┘ /// ▼ /// ┌───────────────────────────────────────────────────────────────────┐ -/// │ G (we have a cross-signing key for the sender) │ +/// │ G (device is cross-signed by the sender) │ /// └───────────────────────────────────────────────────────────────────┘ /// │ /// __________________________________▼______________________________ @@ -253,7 +253,7 @@ impl<'a> SenderDataFinder<'a> { // If there are more than 1, we assume this device was cross-signed by some // identity. if signatures.len() > 1 { - // Yes, the device info is cross-signed + // Yes, the device info is cross-signed by someone self.device_is_cross_signed(sender_device).await } else { // No, the device info is not cross-signed. @@ -269,14 +269,12 @@ impl<'a> SenderDataFinder<'a> { /// E (we have cross-signed device info) async fn device_is_cross_signed(&self, sender_device: Device) -> OlmResult { - // Do we have the cross-signing key for this user? - - let sender_user_id = sender_device.user_id(); - let sender_user_identity = self.store.get_user_identity(sender_user_id).await?; + // Does the cross-signing key match that used to sign the device info? + // And is the signature in the device info valid? - if let Some(sender_user_identity) = sender_user_identity { - // Yes: check the device is signed by the identity - self.have_user_cross_signing_keys(sender_device, sender_user_identity).await + if sender_device.is_cross_signed_by_owner() { + // Yes: check the device is signed by the sender + self.device_is_cross_signed_by_sender(sender_device).await } else { // No: F (we have cross-signed device info, but no cross-signing keys) Ok(SenderData::DeviceInfo { @@ -287,57 +285,34 @@ impl<'a> SenderDataFinder<'a> { } } - /// Step G (we have a cross-signing key for the sender) - async fn have_user_cross_signing_keys( + /// Step G (device is cross-signed by the sender) + async fn device_is_cross_signed_by_sender( &self, sender_device: Device, - sender_user_identity: UserIdentityData, ) -> OlmResult { - // Does the cross-signing key match that used to sign the device info? - // And is the signature in the device info valid? - let maybe_master_key_info = self - .master_key_if_device_is_signed_by_user(&sender_device, &sender_user_identity) - .await?; - - if let Some((master_key, master_key_verified)) = maybe_master_key_info { - // Yes: H (cross-signing key matches that used to sign the device info!) - // and: J (device info is verified by matching cross-signing key) - - // Find the actual key within the MasterPubkey struct - if let Some(master_key) = master_key.get_first_key() { - // We have user_id and master_key for the user sending the to-device message. - // Decide the master_key trust level based on whether we have verified this - // user. Set the user_id, master_key and trust level in the - // session. - Ok(SenderData::SenderKnown { - user_id: sender_device.user_id().to_owned(), - master_key, - master_key_verified, - }) - } else { - // Surprisingly, there was no key in the MasterPubkey. We did not expect this: - // treat it as if the device was not signed by this master key. - // - tracing::error!( - "MasterPubkey for user {} does not contain any keys!", - master_key.user_id() - ); - - Ok(SenderData::DeviceInfo { - device_keys: sender_device.as_device_keys().clone(), - retry_details: SenderDataRetryDetails::retry_soon(), - legacy_session: true, // TODO: change to false when retries etc. are done - }) - } + // H (cross-signing key matches that used to sign the device info!) + // And: J (device info is verified by matching cross-signing key) + let user_id = sender_device.user_id().to_owned(); + + let master_key = sender_device + .device_owner_identity + .as_ref() + .expect( + "device_owner_identity must exist because this device is cross-signing trusted!", + ) + .master_key() + .clone(); + + if let Some(master_key) = master_key.get_first_key() { + // We have user_id and master_key for the user sending the to-device message. + let master_key_verified = sender_device.is_cross_signing_trusted(); + Ok(SenderData::SenderKnown { user_id, master_key, master_key_verified }) } else { - // No: Device was not signed by the known identity of the sender. - // (Or the signature was invalid. We don't know which unfortunately, so we can't - // bail out completely if the signature was invalid, as we'd like - // to.) + // Surprisingly, there was no key in the MasterPubkey. We did not expect this: + // treat it as if the device was not signed by this master key. // - // Since we've already checked there are >1 signatures, we guess - // it was signed by a different identity of this user, so we should retry - // later, in case either the device info or the user's identity changes. + tracing::error!("MasterPubkey for user {user_id} does not contain any keys!",); + Ok(SenderData::DeviceInfo { device_keys: sender_device.as_device_keys().clone(), retry_details: SenderDataRetryDetails::retry_soon(), @@ -345,49 +320,6 @@ impl<'a> SenderDataFinder<'a> { }) } } - - /// If `sender_device` is correctly cross-signed by `sender_user_identity`, - /// return user_identity's master key and whether it is verified. - /// Otherwise, return None. - async fn master_key_if_device_is_signed_by_user<'i>( - &self, - sender_device: &'_ Device, - sender_user_identity: &'i UserIdentityData, - ) -> OlmResult> { - Ok(match sender_user_identity { - UserIdentityData::Own(own_identity) => { - if own_identity.is_device_signed(sender_device).is_ok() { - Some((own_identity.master_key(), own_identity.is_verified())) - } else { - None - } - } - UserIdentityData::Other(other_identity) => { - if other_identity.is_device_signed(sender_device).is_ok() { - let master_key = other_identity.master_key(); - - // Use our own identity to determine whether this other identity is signed - let master_key_verified = - if let Some(own_identity) = self.own_identity().await? { - own_identity.is_identity_signed(other_identity).is_ok() - } else { - // Couldn't get own identity! Assume master_key is not verified. - false - }; - - Some((master_key, master_key_verified)) - } else { - None - } - } - }) - } - - /// Return the user identity of the current user, or None if we failed to - /// find it (which is unexpected) - async fn own_identity(&self) -> OlmResult> { - Ok(self.store.get_user_identity(self.store.user_id()).await?.and_then(|i| i.into_own())) - } } #[cfg(test)] @@ -868,7 +800,7 @@ mod tests { } async fn own() -> Self { - Self::new(user_id!("@myself:s.co"), device_id!("OWNDEVICEID"), true, false, None).await + Self::new(user_id!("@myself:s.co"), device_id!("OWNDEVICEID"), true, true, None).await } async fn other(me: &TestUser, options: &TestOptions) -> Self { From c35b4ecdd215ef505517f77d0bae318919aa6c0a Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 15 Jul 2024 16:37:52 +0100 Subject: [PATCH 21/27] fixup: remove unnecessary async --- .../src/olm/group_sessions/sender_data_finder.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index a315c8f0938..b24b4daa401 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -274,7 +274,7 @@ impl<'a> SenderDataFinder<'a> { if sender_device.is_cross_signed_by_owner() { // Yes: check the device is signed by the sender - self.device_is_cross_signed_by_sender(sender_device).await + self.device_is_cross_signed_by_sender(sender_device) } else { // No: F (we have cross-signed device info, but no cross-signing keys) Ok(SenderData::DeviceInfo { @@ -286,10 +286,7 @@ impl<'a> SenderDataFinder<'a> { } /// Step G (device is cross-signed by the sender) - async fn device_is_cross_signed_by_sender( - &self, - sender_device: Device, - ) -> OlmResult { + fn device_is_cross_signed_by_sender(&self, sender_device: Device) -> OlmResult { // H (cross-signing key matches that used to sign the device info!) // And: J (device info is verified by matching cross-signing key) let user_id = sender_device.user_id().to_owned(); From a798e841010e7afc8820c4aa54a0f4267d324a3c Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Mon, 15 Jul 2024 16:42:19 +0100 Subject: [PATCH 22/27] fixup: Remove more unneeded asyncs --- .../src/olm/group_sessions/sender_data_finder.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index b24b4daa401..9413c837168 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -199,7 +199,7 @@ impl<'a> SenderDataFinder<'a> { self.store.get_device_from_curve_key(&room_key_event.sender, sender_curve_key).await? { // Yes: use the device info to continue - self.have_device(sender_device).await + self.have_device(sender_device) } else { // Step C (no device info locally) // @@ -223,7 +223,7 @@ impl<'a> SenderDataFinder<'a> { if let Ok(sender_device_data) = DeviceData::try_from(sender_device_keys) { let sender_device = self.store.wrap_device_data(sender_device_data).await?; - self.have_device(sender_device).await + self.have_device(sender_device) } else { // The device keys supplied did not validate. // TODO: log an error @@ -233,7 +233,7 @@ impl<'a> SenderDataFinder<'a> { } /// Step D (we have device info) - async fn have_device(&self, sender_device: Device) -> OlmResult { + fn have_device(&self, sender_device: Device) -> OlmResult { // Is the device info cross-signed? let user_id = sender_device.user_id(); @@ -254,7 +254,7 @@ impl<'a> SenderDataFinder<'a> { // identity. if signatures.len() > 1 { // Yes, the device info is cross-signed by someone - self.device_is_cross_signed(sender_device).await + self.device_is_cross_signed(sender_device) } else { // No, the device info is not cross-signed. // Wait to see whether the device becomes cross-signed later. Drop @@ -268,7 +268,7 @@ impl<'a> SenderDataFinder<'a> { } /// E (we have cross-signed device info) - async fn device_is_cross_signed(&self, sender_device: Device) -> OlmResult { + fn device_is_cross_signed(&self, sender_device: Device) -> OlmResult { // Does the cross-signing key match that used to sign the device info? // And is the signature in the device info valid? From 22f93656bd6eaccefb4e2c4bfa0b66f45008c45a Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 16 Jul 2024 10:50:34 +0100 Subject: [PATCH 23/27] fixup: Remove references to 'device info' to avoid creating a new term --- .../olm/group_sessions/sender_data_finder.rs | 70 +++++++++---------- 1 file changed, 35 insertions(+), 35 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 9413c837168..90f7f0c9b1d 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -44,7 +44,7 @@ use crate::{ /// │ no │ /// ▼ │ /// ┌───────────────────────────────────────────────────────────────────┐ │ -/// │ B (there is no device info in the to-device message) │ │ +/// │ B (there are no device keys in the to-device message) │ │ /// │ │ │ /// │ We need to find the device details. │ │ /// └───────────────────────────────────────────────────────────────────┘ │ @@ -57,29 +57,29 @@ use crate::{ /// │ no │ /// ▼ │ /// ╭───────────────────────────────────────────────────────────────────╮ │ -/// │ C (there is no device info locally) │ │ +/// │ C (we don't know the sending device) │ │ /// │ │ │ /// │ Give up: we have no sender info for this room key. │ │ /// ╰───────────────────────────────────────────────────────────────────╯ │ /// ┌─────────────────────────────────┘ /// ▼ /// ┌───────────────────────────────────────────────────────────────────┐ -/// │ D (we have device info) │ +/// │ D (we have the device) │ /// └───────────────────────────────────────────────────────────────────┘ /// │ /// __________________________________▼______________________________ /// ╱ ╲ -/// ╱ Is the device info cross-signed? ╲yes +/// ╱ Is the device cross-signed? ╲yes /// ╲___________________________________________________________________╱ │ /// │ no │ /// ▼ │ /// ╭───────────────────────────────────────────────────────────────────╮ │ -/// │ Give up: unsigned device info is useless. │ │ +/// │ Give up: an unsigned device is useless. │ │ /// ╰───────────────────────────────────────────────────────────────────╯ │ /// ┌─────────────────────────────────┘ /// ▼ /// ┌───────────────────────────────────────────────────────────────────┐ -/// │ E (we have cross-signed device info) │ +/// │ E (we have a cross-signed device) │ /// └───────────────────────────────────────────────────────────────────┘ /// │ /// __________________________________▼______________________________ @@ -89,13 +89,13 @@ use crate::{ /// │ no │ /// ▼ │ /// ┌───────────────────────────────────────────────────────────────────┐ │ -/// │ F (we have cross-signed device info, but no cross-signing keys) │ │ +/// │ F (we have a cross-signed device, but no cross-signing keys) │ │ /// └───────────────────────────────────────────────────────────────────┘ │ /// │ │ /// ▼ │ /// ╭───────────────────────────────────────────────────────────────────╮ │ -/// │ Store the device info with the session, in case we can │ │ -/// │ confirm it later. │ │ +/// │ Store the device with the session, in case we can confirm it │ │ +/// │ later. │ │ /// ╰───────────────────────────────────────────────────────────────────╯ │ /// ┌─────────────────────────────────┘ /// ▼ @@ -106,23 +106,23 @@ use crate::{ /// __________________________________▼______________________________ /// ╱ ╲ /// ╱ Does the cross-signing key match that used ╲yes -/// ╲ to sign the device info? ╱ │ +/// ╲ to sign the device? ╱ │ /// ╲_________________________________________________________________╱ │ /// │ no │ /// ▼ │ /// ╭───────────────────────────────────────────────────────────────────╮ │ -/// │ Store the device info with the session, in case we get the │ │ +/// │ Store the device with the session, in case we get the │ │ /// │ right cross-signing key later. │ │ /// ╰───────────────────────────────────────────────────────────────────╯ │ /// ┌─────────────────────────────────┘ /// ▼ /// ┌───────────────────────────────────────────────────────────────────┐ -/// │ H (cross-signing key matches that used to sign the device info!) │ +/// │ H (cross-signing key matches that used to sign the device!) │ /// └───────────────────────────────────────────────────────────────────┘ /// │ /// __________________________________▼______________________________ /// ╱ ╲ -/// ╱ Is the signature in the device info valid? ╲yes +/// ╱ Is the signature in the device valid? ╲yes /// ╲___________________________________________________________________╱ │ /// │ no │ /// ▼ │ @@ -132,7 +132,7 @@ use crate::{ /// ┌─────────────────────────────────┘ /// ▼ /// ╭───────────────────────────────────────────────────────────────────╮ -/// │ J (device info is verified by matching cross-signing key) │ +/// │ J (device is verified by matching cross-signing key) │ /// │ │ /// │ Look up the user_id and master_key for the user sending the │ /// │ to-device message. │ @@ -150,7 +150,7 @@ pub(crate) struct SenderDataFinder<'a> { } impl<'a> SenderDataFinder<'a> { - /// Find the device info associated with the to-device message used to + /// Find the device associated with the to-device message used to /// create the InboundGroupSession we are about to create, and decide /// whether we trust the sender. pub(crate) async fn find_using_event( @@ -162,7 +162,7 @@ impl<'a> SenderDataFinder<'a> { finder.have_event(sender_curve_key, room_key_event).await } - /// Use the supplied device info to decide whether we trust the sender. + /// Use the supplied device keys to decide whether we trust the sender. pub(crate) async fn find_using_device_keys( store: &'a Store, device_keys: DeviceKeys, @@ -179,7 +179,7 @@ impl<'a> SenderDataFinder<'a> { ) -> OlmResult { // Does the to-device message contain the device_keys property from MSC4147? if let Some(sender_device_keys) = &room_key_event.device_keys { - // Yes: use the device info to continue + // Yes: use the device keys to continue self.have_device_keys(sender_device_keys).await } else { // No: look for the device in the store @@ -187,7 +187,7 @@ impl<'a> SenderDataFinder<'a> { } } - /// Step B (there is no device info in the to-device message) + /// Step B (there are no device keys in the to-device message) async fn search_for_device( &self, sender_curve_key: Curve25519PublicKey, @@ -198,10 +198,10 @@ impl<'a> SenderDataFinder<'a> { if let Some(sender_device) = self.store.get_device_from_curve_key(&room_key_event.sender, sender_curve_key).await? { - // Yes: use the device info to continue + // Yes: use the device to continue self.have_device(sender_device) } else { - // Step C (no device info locally) + // Step C (we don't know the sending device) // // We have no device data for this session so we can't continue in the "fast // lane" (blocking sync). @@ -232,9 +232,9 @@ impl<'a> SenderDataFinder<'a> { } } - /// Step D (we have device info) + /// Step D (we have a device) fn have_device(&self, sender_device: Device) -> OlmResult { - // Is the device info cross-signed? + // Is the device cross-signed? let user_id = sender_device.user_id(); let Some(signatures) = sender_device.signatures().get(user_id) else { @@ -253,30 +253,30 @@ impl<'a> SenderDataFinder<'a> { // If there are more than 1, we assume this device was cross-signed by some // identity. if signatures.len() > 1 { - // Yes, the device info is cross-signed by someone + // Yes, the device is cross-signed by someone self.device_is_cross_signed(sender_device) } else { - // No, the device info is not cross-signed. + // No, the device is not cross-signed. // Wait to see whether the device becomes cross-signed later. Drop // out of both the "fast lane" and the "slow lane" and let the // background retry task try this later. // - // We will need new, cross-signed device info for this to work, so there is no - // point storing the device info we have in the session. + // We will need a new, cross-signed device for this to work, so there is no + // point storing the device we have in the session. Ok(SenderData::unknown()) } } - /// E (we have cross-signed device info) + /// E (we have a cross-signed device) fn device_is_cross_signed(&self, sender_device: Device) -> OlmResult { - // Does the cross-signing key match that used to sign the device info? - // And is the signature in the device info valid? + // Does the cross-signing key match that used to sign the device? + // And is the signature in the device valid? if sender_device.is_cross_signed_by_owner() { // Yes: check the device is signed by the sender self.device_is_cross_signed_by_sender(sender_device) } else { - // No: F (we have cross-signed device info, but no cross-signing keys) + // No: F (we have a cross-signed device, but no cross-signing keys) Ok(SenderData::DeviceInfo { device_keys: sender_device.as_device_keys().clone(), retry_details: SenderDataRetryDetails::retry_soon(), @@ -287,8 +287,8 @@ impl<'a> SenderDataFinder<'a> { /// Step G (device is cross-signed by the sender) fn device_is_cross_signed_by_sender(&self, sender_device: Device) -> OlmResult { - // H (cross-signing key matches that used to sign the device info!) - // And: J (device info is verified by matching cross-signing key) + // H (cross-signing key matches that used to sign the device!) + // And: J (device is verified by matching cross-signing key) let user_id = sender_device.user_id().to_owned(); let master_key = sender_device @@ -466,7 +466,7 @@ mod tests { .await .unwrap(); - // Then we treat it as if there is no device info at all + // Then we treat it as if there is no device at all assert_let!(SenderData::UnknownDevice { retry_details, legacy_session } = sender_data); assert_eq!(retry_details.retry_count, 0); @@ -594,7 +594,7 @@ mod tests { #[async_test] async fn test_notes_master_key_is_verified_for_own_identity() { - // Given we can find the device info, and we sent the event, and we are verified + // Given we can find the device, and we sent the event, and we are verified let setup = TestSetup::new(TestOptions { store_contains_device: true, store_contains_sender_identity: true, @@ -624,7 +624,7 @@ mod tests { #[async_test] async fn test_notes_master_key_is_verified_for_other_identity() { - // Given we can find the device info, and someone else sent the event + // Given we can find the device, and someone else sent the event // And the sender is verified let setup = TestSetup::new(TestOptions { store_contains_device: true, From aaf8b27ccb31f3399088b2c6dc8aee64b7aacf5d Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 16 Jul 2024 10:55:03 +0100 Subject: [PATCH 24/27] fixup: Stop assuming device_owner_identity exists --- .../src/olm/group_sessions/sender_data_finder.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 90f7f0c9b1d..37eb0bfff7d 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -294,13 +294,10 @@ impl<'a> SenderDataFinder<'a> { let master_key = sender_device .device_owner_identity .as_ref() - .expect( - "device_owner_identity must exist because this device is cross-signing trusted!", - ) - .master_key() - .clone(); + .map(|i| i.master_key().get_first_key()) + .flatten(); - if let Some(master_key) = master_key.get_first_key() { + if let Some(master_key) = master_key { // We have user_id and master_key for the user sending the to-device message. let master_key_verified = sender_device.is_cross_signing_trusted(); Ok(SenderData::SenderKnown { user_id, master_key, master_key_verified }) From 20d834c4e34481f5cbe3cf2c11e1669404e5b0be Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 16 Jul 2024 11:24:58 +0100 Subject: [PATCH 25/27] fixup: Add missing error handling for invalid devices --- crates/matrix-sdk-crypto/src/error.rs | 4 ++++ .../olm/group_sessions/sender_data_finder.rs | 21 ++++++++++--------- 2 files changed, 15 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index cedb9712da5..c73a8ed27e9 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -297,6 +297,10 @@ pub enum SessionCreationError { /// Error when creating an Olm Session from an incoming Olm message. #[error(transparent)] InboundCreation(#[from] vodozemac::olm::SessionCreationError), + + /// The given device keys are invalid. + #[error("The given device keys are invalid")] + InvalidDeviceKeys(#[from] SignatureError), } /// Errors that can be returned by diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 37eb0bfff7d..4b5f892d031 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -17,10 +17,10 @@ use vodozemac::Curve25519PublicKey; use super::{SenderData, SenderDataRetryDetails}; use crate::{ - error::OlmResult, + error::{OlmResult, SessionCreationError}, store::Store, types::{events::olm_v1::DecryptedRoomKeyEvent, DeviceKeys}, - Device, DeviceData, EventError, OlmError, + Device, DeviceData, OlmError, }; /// Temporary struct that is used to look up [`SenderData`] based on the @@ -220,15 +220,16 @@ impl<'a> SenderDataFinder<'a> { async fn have_device_keys(&self, sender_device_keys: &DeviceKeys) -> OlmResult { // Validate the signature of the DeviceKeys supplied. - if let Ok(sender_device_data) = DeviceData::try_from(sender_device_keys) { - let sender_device = self.store.wrap_device_data(sender_device_data).await?; + match DeviceData::try_from(sender_device_keys) { + Ok(sender_device_data) => { + let sender_device = self.store.wrap_device_data(sender_device_data).await?; - self.have_device(sender_device) - } else { - // The device keys supplied did not validate. - // TODO: log an error - // TODO: Err(OlmError::SessionCreation(SessionCreationError::InvalidDeviceKeys)) - Err(OlmError::EventError(EventError::UnsupportedAlgorithm)) + self.have_device(sender_device) + } + Err(e) => { + // The device keys supplied did not validate. + Err(OlmError::SessionCreation(SessionCreationError::InvalidDeviceKeys(e))) + } } } From 78aab4381afe2344f0e039a2d66d609d4403801c Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 16 Jul 2024 11:29:22 +0100 Subject: [PATCH 26/27] fixup: Clippy fix --- .../src/olm/group_sessions/sender_data_finder.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 4b5f892d031..8042c3c887d 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -295,8 +295,7 @@ impl<'a> SenderDataFinder<'a> { let master_key = sender_device .device_owner_identity .as_ref() - .map(|i| i.master_key().get_first_key()) - .flatten(); + .and_then(|i| i.master_key().get_first_key()); if let Some(master_key) = master_key { // We have user_id and master_key for the user sending the to-device message. From d496196252185a3f4d53fd180da75ebb23c49275 Mon Sep 17 00:00:00 2001 From: Andy Balaam Date: Tue, 16 Jul 2024 11:50:22 +0100 Subject: [PATCH 27/27] fixup: Remove extra checking for unsigned devices and treat them similarly to ones signed by unknown keys --- .../olm/group_sessions/sender_data_finder.rs | 86 +++---------------- 1 file changed, 11 insertions(+), 75 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs index 8042c3c887d..3058e5edb84 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/sender_data_finder.rs @@ -69,31 +69,13 @@ use crate::{ /// │ /// __________________________________▼______________________________ /// ╱ ╲ -/// ╱ Is the device cross-signed? ╲yes -/// ╲___________________________________________________________________╱ │ -/// │ no │ -/// ▼ │ -/// ╭───────────────────────────────────────────────────────────────────╮ │ -/// │ Give up: an unsigned device is useless. │ │ -/// ╰───────────────────────────────────────────────────────────────────╯ │ -/// ┌─────────────────────────────────┘ -/// ▼ -/// ┌───────────────────────────────────────────────────────────────────┐ -/// │ E (we have a cross-signed device) │ -/// └───────────────────────────────────────────────────────────────────┘ -/// │ -/// __________________________________▼______________________________ -/// ╱ ╲ /// ╱ Is the device cross-signed by the sender? ╲yes /// ╲___________________________________________________________________╱ │ /// │ no │ /// ▼ │ /// ┌───────────────────────────────────────────────────────────────────┐ │ -/// │ F (we have a cross-signed device, but no cross-signing keys) │ │ -/// └───────────────────────────────────────────────────────────────────┘ │ -/// │ │ -/// ▼ │ -/// ╭───────────────────────────────────────────────────────────────────╮ │ +/// │ F (we have device keys, but they are not signed by the sender) │ │ +/// │ │ │ /// │ Store the device with the session, in case we can confirm it │ │ /// │ later. │ │ /// ╰───────────────────────────────────────────────────────────────────╯ │ @@ -118,21 +100,6 @@ use crate::{ /// ▼ /// ┌───────────────────────────────────────────────────────────────────┐ /// │ H (cross-signing key matches that used to sign the device!) │ -/// └───────────────────────────────────────────────────────────────────┘ -/// │ -/// __________________________________▼______________________________ -/// ╱ ╲ -/// ╱ Is the signature in the device valid? ╲yes -/// ╲___________________________________________________________________╱ │ -/// │ no │ -/// ▼ │ -/// ╭───────────────────────────────────────────────────────────────────╮ │ -/// │ !Session is invalid: drop it from the store and forget it. │ │ -/// ╰───────────────────────────────────────────────────────────────────╯ │ -/// ┌─────────────────────────────────┘ -/// ▼ -/// ╭───────────────────────────────────────────────────────────────────╮ -/// │ J (device is verified by matching cross-signing key) │ /// │ │ /// │ Look up the user_id and master_key for the user sending the │ /// │ to-device message. │ @@ -236,40 +203,6 @@ impl<'a> SenderDataFinder<'a> { /// Step D (we have a device) fn have_device(&self, sender_device: Device) -> OlmResult { // Is the device cross-signed? - - let user_id = sender_device.user_id(); - let Some(signatures) = sender_device.signatures().get(user_id) else { - // This should never happen: we would not have managed to get a Device - // if it did not contain a signature. - error!( - "Found a device for user_id {user_id} but it has no signatures for that user id!" - ); - - // Return the same result as if the device is not cross-signed. - return Ok(SenderData::unknown()); - }; - - // Count number of signatures - we know there is 1, because we would not have - // been able to construct a Device without a signature. - // If there are more than 1, we assume this device was cross-signed by some - // identity. - if signatures.len() > 1 { - // Yes, the device is cross-signed by someone - self.device_is_cross_signed(sender_device) - } else { - // No, the device is not cross-signed. - // Wait to see whether the device becomes cross-signed later. Drop - // out of both the "fast lane" and the "slow lane" and let the - // background retry task try this later. - // - // We will need a new, cross-signed device for this to work, so there is no - // point storing the device we have in the session. - Ok(SenderData::unknown()) - } - } - - /// E (we have a cross-signed device) - fn device_is_cross_signed(&self, sender_device: Device) -> OlmResult { // Does the cross-signing key match that used to sign the device? // And is the signature in the device valid? @@ -277,7 +210,7 @@ impl<'a> SenderDataFinder<'a> { // Yes: check the device is signed by the sender self.device_is_cross_signed_by_sender(sender_device) } else { - // No: F (we have a cross-signed device, but no cross-signing keys) + // No: F (we have device keys, but they are not signed by the sender) Ok(SenderData::DeviceInfo { device_keys: sender_device.as_device_keys().clone(), retry_details: SenderDataRetryDetails::retry_soon(), @@ -289,7 +222,6 @@ impl<'a> SenderDataFinder<'a> { /// Step G (device is cross-signed by the sender) fn device_is_cross_signed_by_sender(&self, sender_device: Device) -> OlmResult { // H (cross-signing key matches that used to sign the device!) - // And: J (device is verified by matching cross-signing key) let user_id = sender_device.user_id().to_owned(); let master_key = sender_device @@ -305,7 +237,7 @@ impl<'a> SenderDataFinder<'a> { // Surprisingly, there was no key in the MasterPubkey. We did not expect this: // treat it as if the device was not signed by this master key. // - tracing::error!("MasterPubkey for user {user_id} does not contain any keys!",); + error!("MasterPubkey for user {user_id} does not contain any keys!",); Ok(SenderData::DeviceInfo { device_keys: sender_device.as_device_keys().clone(), @@ -443,7 +375,7 @@ mod tests { } #[async_test] - async fn test_does_not_add_sender_data_if_device_is_not_signed() { + async fn test_adds_device_info_even_if_it_is_not_signed() { // Given that the the device is in the store // But it is not signed let setup = TestSetup::new(TestOptions { @@ -463,8 +395,12 @@ mod tests { .await .unwrap(); - // Then we treat it as if there is no device at all - assert_let!(SenderData::UnknownDevice { retry_details, legacy_session } = sender_data); + // Then we store the device info even though it is useless, in case we want to + // check it matches up later. + assert_let!( + SenderData::DeviceInfo { device_keys, retry_details, legacy_session } = sender_data + ); + assert_eq!(&device_keys, setup.sender_device.as_device_keys()); assert_eq!(retry_details.retry_count, 0); // TODO: This should not be marked as a legacy session, but for now it is