From 6e6f71d970839b899568da0b05e8396f4f08f60d Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 6 Aug 2024 23:06:37 +0200 Subject: [PATCH] crypto: key sharing error for verified user with unverified devices --- bindings/matrix-sdk-crypto-ffi/src/lib.rs | 4 +- crates/matrix-sdk-base/src/client.rs | 2 +- crates/matrix-sdk-crypto/src/error.rs | 20 + .../src/identities/device.rs | 8 + crates/matrix-sdk-crypto/src/machine.rs | 2 +- .../src/olm/group_sessions/outbound.rs | 11 +- .../src/session_manager/group_sessions/mod.rs | 4 +- .../group_sessions/share_strategy.rs | 570 +++++++++++++++++- .../src/test_json/keys_query_sets.rs | 66 +- 9 files changed, 647 insertions(+), 40 deletions(-) diff --git a/bindings/matrix-sdk-crypto-ffi/src/lib.rs b/bindings/matrix-sdk-crypto-ffi/src/lib.rs index 1d34424b990..8603b9f5318 100644 --- a/bindings/matrix-sdk-crypto-ffi/src/lib.rs +++ b/bindings/matrix-sdk-crypto-ffi/src/lib.rs @@ -668,6 +668,8 @@ pub struct EncryptionSettings { /// Should untrusted devices receive the room key, or should they be /// excluded from the conversation. pub only_allow_trusted_devices: bool, + /// Should fail to send when a verified user has unverified devices. + pub error_on_unsigned_device_of_verified_users: bool, } impl From for RustEncryptionSettings { @@ -677,7 +679,7 @@ impl From for RustEncryptionSettings { rotation_period: Duration::from_secs(v.rotation_period), rotation_period_msgs: v.rotation_period_msgs, history_visibility: v.history_visibility.into(), - sharing_strategy: CollectStrategy::new_device_based(v.only_allow_trusted_devices), + sharing_strategy: CollectStrategy::new_device_based(v.only_allow_trusted_devices, v.error_on_unsigned_device_of_verified_users), } } } diff --git a/crates/matrix-sdk-base/src/client.rs b/crates/matrix-sdk-base/src/client.rs index 2091cf28e1d..6dfbbbdda49 100644 --- a/crates/matrix-sdk-base/src/client.rs +++ b/crates/matrix-sdk-base/src/client.rs @@ -1391,7 +1391,7 @@ impl BaseClient { let members = self.store.get_user_ids(room_id, filter).await?; let settings = settings.ok_or(Error::EncryptionNotEnabled)?; - let settings = EncryptionSettings::new(settings, history_visibility, false); + let settings = EncryptionSettings::new(settings, history_visibility, false, false); Ok(o.share_room_key(room_id, members.iter().map(Deref::deref), settings).await?) } diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index 7e0adc2aca2..ba72237562b 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::BTreeMap; + use ruma::{CanonicalJsonError, IdParseError, OwnedDeviceId, OwnedRoomId, OwnedUserId}; use serde::{ser::SerializeMap, Serializer}; use serde_json::Error as SerdeError; @@ -70,8 +72,26 @@ pub enum OlmError { have a valid Olm session with us" )] MissingSession, + + #[error(transparent)] + /// The room key that was to be shared was not shared because the sharing + /// strategy could not be fulfilled. + RoomKeySharingStrategyError(DeviceCollectError), } +/// Depending on the sharing strategy for room keys, the distribution of the +/// room key could fail. +#[derive(Error, Debug)] +pub enum DeviceCollectError { + /// When a verified user has some unsigned devices. + /// Happens only with `CollectStrategy::DeviceBasedStrategy` when + /// `error_on_unsigned_device_of_verified_users` is true. + /// + /// In order to resolve this the caller has to ignore or blacklist the given + /// devices (see [`Device::set_local_trust`]). + #[error("Encryption failed because one or several verified users have unsigned devices")] + DeviceBasedVerifiedUserHasUnsignedDevice(BTreeMap>), +} /// Error representing a failure during a group encryption operation. #[derive(Error, Debug)] pub enum MegolmError { diff --git a/crates/matrix-sdk-crypto/src/identities/device.rs b/crates/matrix-sdk-crypto/src/identities/device.rs index a7d0161fe0e..b8ddac8e19a 100644 --- a/crates/matrix-sdk-crypto/src/identities/device.rs +++ b/crates/matrix-sdk-crypto/src/identities/device.rs @@ -642,6 +642,14 @@ impl DeviceData { self.local_trust_state() == LocalTrust::BlackListed } + /// Is the device locally marked as whitelisted. + /// + /// Whitelisted devices will receive group sessions regardless of their + /// verification status. + pub fn is_whitelisted(&self) -> bool { + self.local_trust_state() == LocalTrust::Ignored + } + /// Set the trust state of the device to the given state. /// /// Note: This should only done in the crypto store where the trust state diff --git a/crates/matrix-sdk-crypto/src/machine.rs b/crates/matrix-sdk-crypto/src/machine.rs index 451c7d5491c..d2acfcf411d 100644 --- a/crates/matrix-sdk-crypto/src/machine.rs +++ b/crates/matrix-sdk-crypto/src/machine.rs @@ -3314,7 +3314,7 @@ pub(crate) mod tests { let encryption_settings = EncryptionSettings::default(); let encryption_settings = EncryptionSettings { - sharing_strategy: CollectStrategy::new_device_based(true), + sharing_strategy: CollectStrategy::new_device_based(true, false), ..encryption_settings }; 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 8d32538f342..11a4a0ba3b7 100644 --- a/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs +++ b/crates/matrix-sdk-crypto/src/olm/group_sessions/outbound.rs @@ -122,6 +122,7 @@ impl EncryptionSettings { content: RoomEncryptionEventContent, history_visibility: HistoryVisibility, only_allow_trusted_devices: bool, + error_on_unsigned_device_of_verified_users: bool, ) -> Self { let rotation_period: Duration = content.rotation_period_ms.map_or(ROTATION_PERIOD, |r| Duration::from_millis(r.into())); @@ -133,7 +134,10 @@ impl EncryptionSettings { rotation_period, rotation_period_msgs, history_visibility, - sharing_strategy: CollectStrategy::new_device_based(only_allow_trusted_devices), + sharing_strategy: CollectStrategy::new_device_based( + only_allow_trusted_devices, + error_on_unsigned_device_of_verified_users, + ), } } } @@ -787,7 +791,8 @@ mod tests { fn test_encryption_settings_conversion() { let mut content = RoomEncryptionEventContent::new(EventEncryptionAlgorithm::MegolmV1AesSha2); - let settings = EncryptionSettings::new(content.clone(), HistoryVisibility::Joined, false); + let settings = + EncryptionSettings::new(content.clone(), HistoryVisibility::Joined, false, false); assert_eq!(settings.rotation_period, ROTATION_PERIOD); assert_eq!(settings.rotation_period_msgs, ROTATION_MESSAGES); @@ -795,7 +800,7 @@ mod tests { content.rotation_period_ms = Some(uint!(3600)); content.rotation_period_msgs = Some(uint!(500)); - let settings = EncryptionSettings::new(content, HistoryVisibility::Shared, false); + let settings = EncryptionSettings::new(content, HistoryVisibility::Shared, false, false); assert_eq!(settings.rotation_period, Duration::from_millis(3600)); assert_eq!(settings.rotation_period_msgs, 500); 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 2ad1ed59a07..198355fe254 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 @@ -1175,7 +1175,7 @@ mod tests { .any(|d| d.user_id() == user_id && d.device_id() == device_id)); let settings = EncryptionSettings { - sharing_strategy: CollectStrategy::new_device_based(true), + sharing_strategy: CollectStrategy::new_device_based(true, false), ..Default::default() }; let users = [user_id].into_iter(); @@ -1235,7 +1235,7 @@ mod tests { let users = keys_claim.one_time_keys.keys().map(Deref::deref); let settings = EncryptionSettings { - sharing_strategy: CollectStrategy::new_device_based(true), + sharing_strategy: CollectStrategy::new_device_based(true, false), ..Default::default() }; diff --git a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs index 69b4ce2fe95..ea1a327d429 100644 --- a/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs +++ b/crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs @@ -24,8 +24,10 @@ use tracing::{debug, instrument, trace}; use super::OutboundGroupSession; use crate::{ - error::OlmResult, store::Store, types::events::room_key_withheld::WithheldCode, DeviceData, - EncryptionSettings, OwnUserIdentityData, UserIdentityData, + error::{DeviceCollectError, OlmResult}, + store::Store, + types::events::room_key_withheld::WithheldCode, + DeviceData, EncryptionSettings, OlmError, OwnUserIdentityData, UserIdentityData, }; /// Strategy to collect the devices that should receive room keys for the @@ -42,6 +44,21 @@ pub enum CollectStrategy { /// trusted via interactive verification. /// - It is the current own device of the user. only_allow_trusted_devices: bool, + /// If true, when a verified user has an unsigned device the key sharing + /// will fail with an error. If false the key will be + /// distributed to that unsigned device. In order to resolve + /// this sharing error the user can choose to ignore or blacklist the + /// device: + /// - [`Device::set_local_trust`] using [`LocalTrust::Ignored`], this + /// will ignore the warning for this device, and send it the key. + /// - [`Device::set_local_trust`] using [`LocalTrust::BlackListed`], + /// this will not distribute the key to this device. + /// + /// Once the problematic devices are blacklisted or whitelisted the + /// caller can retry to share a second time. This has to be done + /// once per problematic device. + #[serde(default)] + error_on_unsigned_device_of_verified_users: bool, }, /// Share based on identity. Only distribute to devices signed by their /// owner. If a user has no published identity he will not receive @@ -51,8 +68,14 @@ pub enum CollectStrategy { impl CollectStrategy { /// Creates a new legacy strategy, based on per device trust. - pub const fn new_device_based(only_allow_trusted_devices: bool) -> Self { - CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } + pub const fn new_device_based( + only_allow_trusted_devices: bool, + error_on_unsigned_device_of_verified_users: bool, + ) -> Self { + CollectStrategy::DeviceBasedStrategy { + only_allow_trusted_devices, + error_on_unsigned_device_of_verified_users, + } } /// Creates an identity based strategy @@ -63,7 +86,7 @@ impl CollectStrategy { impl Default for CollectStrategy { fn default() -> Self { - CollectStrategy::new_device_based(false) + CollectStrategy::new_device_based(false, false) } } @@ -128,22 +151,31 @@ pub(crate) async fn collect_session_recipients( let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); match settings.sharing_strategy { - CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } => { + CollectStrategy::DeviceBasedStrategy { + only_allow_trusted_devices, + error_on_unsigned_device_of_verified_users, + } => { + let mut unsigned_devices_of_verified_users: BTreeMap> = + Default::default(); for user_id in users { let user_devices = store.get_device_data_for_user_filtered(user_id).await?; - // We only need the user identity if only_allow_trusted_devices is set. - let device_owner_identity = if only_allow_trusted_devices { - store.get_user_identity(user_id).await? - } else { - None - }; + // We only need the user identity if only_allow_trusted_devices or + // error_on_unsigned_device_of_verified_users is set. + let device_owner_identity = + if only_allow_trusted_devices || error_on_unsigned_device_of_verified_users { + store.get_user_identity(user_id).await? + } else { + None + }; let recipient_devices = split_recipients_withhelds_for_user( user_devices, &own_identity, &device_owner_identity, only_allow_trusted_devices, + error_on_unsigned_device_of_verified_users, ); + let recipients = recipient_devices.allowed_devices; let withheld_recipients = recipient_devices.denied_devices_with_code; @@ -157,6 +189,24 @@ pub(crate) async fn collect_session_recipients( devices.entry(user_id.to_owned()).or_default().extend(recipients); withheld_devices.extend(withheld_recipients); + if let Some(blockers) = recipient_devices.unsigned_of_verified_user { + if !blockers.is_empty() { + unsigned_devices_of_verified_users + .entry(user_id.to_owned()) + .or_default() + .extend(blockers) + } + } + } + + if error_on_unsigned_device_of_verified_users + && !unsigned_devices_of_verified_users.is_empty() + { + return Err(OlmError::RoomKeySharingStrategyError( + DeviceCollectError::DeviceBasedVerifiedUserHasUnsignedDevice( + unsigned_devices_of_verified_users, + ), + )); } } CollectStrategy::IdentityBasedStrategy => { @@ -186,9 +236,6 @@ pub(crate) async fn collect_session_recipients( } }; - // } - // end of for each - if should_rotate { debug!( should_rotate, @@ -208,7 +255,7 @@ pub(crate) async fn collect_session_recipients( fn is_session_overshared_for_user( outbound: &OutboundGroupSession, user_id: &UserId, - recipients: &Vec, + recipients: &[DeviceData], ) -> bool { // Device IDs that should receive this session let recipient_device_ids: BTreeSet<&DeviceId> = @@ -245,6 +292,7 @@ fn is_session_overshared_for_user( struct RecipientDevices { allowed_devices: Vec, denied_devices_with_code: Vec<(DeviceData, WithheldCode)>, + unsigned_of_verified_user: Option>, } fn split_recipients_withhelds_for_user( @@ -252,7 +300,9 @@ fn split_recipients_withhelds_for_user( own_identity: &Option, device_owner_identity: &Option, only_allow_trusted_devices: bool, + error_on_unsigned_device_of_verified_users: bool, ) -> RecipientDevices { + let mut unsigned_of_verified: Vec = Default::default(); // From all the devices a user has, we're splitting them into two // buckets, a bucket of devices that should receive the // room key and a bucket of devices that should receive @@ -261,16 +311,61 @@ fn split_recipients_withhelds_for_user( user_devices.into_values().partition_map(|d| { if d.is_blacklisted() { Either::Right((d, WithheldCode::Blacklisted)) + } else if d.is_whitelisted() { + // Ignore the trust state of that device and share + Either::Left(d) } else if only_allow_trusted_devices && !d.is_verified(own_identity, device_owner_identity) { Either::Right((d, WithheldCode::Unverified)) } else { + // track and collect unsigned devices of verified users + if error_on_unsigned_device_of_verified_users + && is_unsigned_device_of_verified_user(own_identity, device_owner_identity, &d) + { + unsigned_of_verified.push(d.device_id().to_owned()) + } Either::Left(d) } }); - RecipientDevices { allowed_devices: recipients, denied_devices_with_code: withheld_recipients } + RecipientDevices { + allowed_devices: recipients, + denied_devices_with_code: withheld_recipients, + unsigned_of_verified_user: Some(unsigned_of_verified), + } +} + +fn is_unsigned_device_of_verified_user( + own_identity: &Option, + device_owner_identity: &Option, + device_data: &DeviceData, +) -> bool { + // If we don't have an identity the other user can't be verified so return false + let own_identity = match own_identity { + Some(o) => o, + _ => return false, + }; + + // If the device owner doesn't have an identity he can't be verified so return + // false + let owner_identity = match device_owner_identity { + Some(o) => o, + _ => return false, + }; + + let is_owner_verified = match owner_identity { + UserIdentityData::Other(ot) => own_identity.is_identity_signed(ot).is_ok(), + UserIdentityData::Own(ow) => ow.is_verified(), + }; + + if is_owner_verified { + // check the device + !device_data.is_cross_signed_by_owner(owner_identity) + } else { + // The owner is not verified + false + } } fn split_recipients_withhelds_for_user_based_on_identity( @@ -287,6 +382,7 @@ fn split_recipients_withhelds_for_user_based_on_identity( .into_values() .map(|d| (d, WithheldCode::Unauthorised)) .collect(), + unsigned_of_verified_user: None, } } Some(device_owner_identity) => { @@ -304,6 +400,7 @@ fn split_recipients_withhelds_for_user_based_on_identity( RecipientDevices { allowed_devices: recipients, denied_devices_with_code: withheld_recipients, + unsigned_of_verified_user: None, } } } @@ -311,19 +408,23 @@ fn split_recipients_withhelds_for_user_based_on_identity( #[cfg(test)] mod tests { - use std::sync::Arc; - use matrix_sdk_test::{async_test, test_json::keys_query_sets::KeyDistributionTestData}; - use ruma::{events::room::history_visibility::HistoryVisibility, room_id, TransactionId}; + use matrix_sdk_test::{ + async_test, test_json, test_json::keys_query_sets::KeyDistributionTestData, + }; + use ruma::{ + device_id, events::room::history_visibility::HistoryVisibility, room_id, TransactionId, + }; use crate::{ + error::DeviceCollectError, olm::OutboundGroupSession, session_manager::{ group_sessions::share_strategy::collect_session_recipients, CollectStrategy, }, types::events::room_key_withheld::WithheldCode, - CrossSigningKeyExport, EncryptionSettings, OlmMachine, + CrossSigningKeyExport, EncryptionSettings, LocalTrust, OlmMachine, }; async fn set_up_test_machine() -> OlmMachine { @@ -369,7 +470,7 @@ mod tests { async fn test_share_with_per_device_strategy_to_all() { let machine = set_up_test_machine().await; - let legacy_strategy = CollectStrategy::new_device_based(false); + let legacy_strategy = CollectStrategy::new_device_based(false, false); let encryption_settings = EncryptionSettings { sharing_strategy: legacy_strategy.clone(), ..Default::default() }; @@ -420,7 +521,7 @@ mod tests { let fake_room_id = room_id!("!roomid:localhost"); - let legacy_strategy = CollectStrategy::new_device_based(true); + let legacy_strategy = CollectStrategy::new_device_based(true, false); let encryption_settings = EncryptionSettings { sharing_strategy: legacy_strategy.clone(), ..Default::default() }; @@ -486,6 +587,425 @@ mod tests { assert_eq!(code, &WithheldCode::Unverified); } + #[async_test] + async fn test_share_with_per_device_strategy_only_trusted_error_on_unsigned_of_verified() { + let machine = set_up_test_machine().await; + + let fake_room_id = room_id!("!roomid:localhost"); + + let legacy_strategy = CollectStrategy::new_device_based(true, true); + + let encryption_settings = + EncryptionSettings { sharing_strategy: legacy_strategy.clone(), ..Default::default() }; + + let id_keys = machine.identity_keys(); + let group_session = OutboundGroupSession::new( + machine.device_id().into(), + Arc::new(id_keys), + fake_room_id, + encryption_settings.clone(), + ) + .unwrap(); + + let share_result = collect_session_recipients( + machine.store(), + vec![ + KeyDistributionTestData::dan_id(), + KeyDistributionTestData::dave_id(), + KeyDistributionTestData::good_id(), + ] + .into_iter(), + &encryption_settings, + &group_session, + ) + .await + .unwrap(); + + assert!(!share_result.should_rotate); + + let dave_devices_shared = share_result.devices.get(KeyDistributionTestData::dave_id()); + let good_devices_shared = share_result.devices.get(KeyDistributionTestData::good_id()); + // dave and good wouldn't receive any key + assert!(dave_devices_shared.unwrap().is_empty()); + assert!(good_devices_shared.unwrap().is_empty()); + + // dan is verified by me and has one of his devices self signed, so should get + // the key + let dan_devices_shared = + share_result.devices.get(KeyDistributionTestData::dan_id()).unwrap(); + + assert_eq!(dan_devices_shared.len(), 1); + let dan_device_that_will_get_the_key = &dan_devices_shared[0]; + assert_eq!( + dan_device_that_will_get_the_key.device_id().as_str(), + KeyDistributionTestData::dan_signed_device_id() + ); + + // Check withhelds for others + let (_, code) = share_result + .withheld_devices + .iter() + .find(|(d, _)| d.device_id() == KeyDistributionTestData::dan_unsigned_device_id()) + .expect("This dan's device should receive a withheld code"); + + assert_eq!(code, &WithheldCode::Unverified); + + let (_, code) = share_result + .withheld_devices + .iter() + .find(|(d, _)| d.device_id() == KeyDistributionTestData::dave_device_id()) + .expect("This daves's device should receive a withheld code"); + + assert_eq!(code, &WithheldCode::Unverified); + } + + async fn error_on_unsigned_of_verified_machine_setup() -> OlmMachine { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; + + let machine = OlmMachine::new(DataSet::own_id(), device_id!("LOCAL")).await; + + let keys_query = DataSet::own_keys_query_response_1(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + // Import own keys private parts + machine + .import_cross_signing_keys(CrossSigningKeyExport { + master_key: DataSet::MASTER_KEY_PRIVATE_EXPORT.to_owned().into(), + self_signing_key: DataSet::SELF_SIGNING_KEY_PRIVATE_EXPORT.to_owned().into(), + user_signing_key: DataSet::USER_SIGNING_KEY_PRIVATE_EXPORT.to_owned().into(), + }) + .await + .unwrap(); + + let keys_query = DataSet::bob_keys_query_response_signed(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + // Bob is verified and has 1 unsigned device + let bob_identity = machine.get_identity(DataSet::bob_id(), None).await.unwrap().unwrap(); + assert!(bob_identity.other().unwrap().is_verified()); + // bob_device_1_id + let bob_signed_device = machine + .get_device(DataSet::bob_id(), DataSet::bob_device_1_id(), None) + .await + .unwrap() + .unwrap(); + let bob_unsigned_device = machine + .get_device(DataSet::bob_id(), DataSet::bob_device_2_id(), None) + .await + .unwrap() + .unwrap(); + assert!(bob_signed_device.device_owner_identity.is_some()); + + assert!(bob_signed_device.is_verified()); + assert!(!bob_unsigned_device.is_verified()); + machine + } + #[async_test] + async fn test_error_on_unsigned_of_verified_resolve_by_whitelisting() { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; + + let machine = error_on_unsigned_of_verified_machine_setup().await; + + let bob_unsigned_device = machine + .get_device(DataSet::bob_id(), DataSet::bob_device_2_id(), None) + .await + .unwrap() + .unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + + let strategy = CollectStrategy::new_device_based(false, true); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let id_keys = machine.identity_keys(); + let group_session = OutboundGroupSession::new( + machine.device_id().into(), + Arc::new(id_keys), + fake_room_id, + encryption_settings.clone(), + ) + .unwrap(); + + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::bob_id()].into_iter(), + &encryption_settings, + &group_session, + ) + .await; + + assert!(share_result.is_err()); + let share_error = share_result.unwrap_err(); + match share_error { + crate::OlmError::RoomKeySharingStrategyError( + DeviceCollectError::DeviceBasedVerifiedUserHasUnsignedDevice(blockers), + ) => { + assert_eq!(1, blockers.len()); + assert_eq!(1, blockers.get(DataSet::bob_id()).unwrap().len()); + let blocking_device_id = + blockers.get(DataSet::bob_id()).unwrap().iter().next().unwrap(); + assert_eq!(blocking_device_id, bob_unsigned_device.device_id()); + // Try to resolve by ignoring + bob_unsigned_device.set_local_trust(LocalTrust::Ignored).await.unwrap(); + + // now sharing should work + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::bob_id()].into_iter(), + &encryption_settings, + &group_session, + ) + .await + .unwrap(); + + assert_eq!(2, share_result.devices.get(DataSet::bob_id()).unwrap().len()); + assert_eq!(0, share_result.withheld_devices.len()); + + // Ensure that this device will not cause problems for future messages + let group_session_2 = OutboundGroupSession::new( + machine.device_id().into(), + Arc::new(id_keys), + fake_room_id, + encryption_settings.clone(), + ) + .unwrap(); + + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::bob_id()].into_iter(), + &encryption_settings, + &group_session_2, + ) + .await + .unwrap(); + + assert_eq!(2, share_result.devices.get(DataSet::bob_id()).unwrap().len()); + assert_eq!(0, share_result.withheld_devices.len()); + } + _ => panic!("Expected a DeviceBasedVerifiedUserHasUnsignedDevice error"), + } + } + + #[async_test] + async fn test_error_on_unsigned_of_verified_resolve_by_blacklisting() { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; + + let machine = error_on_unsigned_of_verified_machine_setup().await; + + let bob_unsigned_device = machine + .get_device(DataSet::bob_id(), DataSet::bob_device_2_id(), None) + .await + .unwrap() + .unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + + let strategy = CollectStrategy::new_device_based(false, true); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let id_keys = machine.identity_keys(); + let group_session = OutboundGroupSession::new( + machine.device_id().into(), + Arc::new(id_keys), + fake_room_id, + encryption_settings.clone(), + ) + .unwrap(); + + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::bob_id()].into_iter(), + &encryption_settings, + &group_session, + ) + .await; + + assert!(share_result.is_err()); + let share_error = share_result.unwrap_err(); + match share_error { + crate::OlmError::RoomKeySharingStrategyError( + DeviceCollectError::DeviceBasedVerifiedUserHasUnsignedDevice(blockers), + ) => { + assert_eq!(1, blockers.len()); + assert_eq!(1, blockers.get(DataSet::bob_id()).unwrap().len()); + let blocking_device_id = + blockers.get(DataSet::bob_id()).unwrap().iter().next().unwrap(); + assert_eq!(blocking_device_id, bob_unsigned_device.device_id()); + // Try to resolve by blacklisting + bob_unsigned_device.set_local_trust(LocalTrust::BlackListed).await.unwrap(); + + // now sharing should work + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::bob_id()].into_iter(), + &encryption_settings, + &group_session, + ) + .await + .unwrap(); + + assert_eq!(1, share_result.devices.get(DataSet::bob_id()).unwrap().len()); + assert_eq!(1, share_result.withheld_devices.len()); + let (blocked, code) = share_result.withheld_devices.first().unwrap(); + assert_eq!(WithheldCode::Blacklisted.as_str(), code.as_str()); + assert_eq!(bob_unsigned_device.device_id(), blocked.device_id()); + + // Ensure that this device will not cause problems for future messages + let group_session_2 = OutboundGroupSession::new( + machine.device_id().into(), + Arc::new(id_keys), + fake_room_id, + encryption_settings.clone(), + ) + .unwrap(); + + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::bob_id()].into_iter(), + &encryption_settings, + &group_session_2, + ) + .await + .unwrap(); + + assert_eq!(1, share_result.devices.get(DataSet::bob_id()).unwrap().len()); + assert_eq!(1, share_result.withheld_devices.len()); + } + _ => panic!("Expected a DeviceBasedVerifiedUserHasUnsignedDevice error"), + } + } + + #[async_test] + async fn test_error_on_unsigned_of_verified_multiple_users() { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; + + let machine = error_on_unsigned_of_verified_machine_setup().await; + let bob_unsigned_device = machine + .get_device(DataSet::bob_id(), DataSet::bob_device_2_id(), None) + .await + .unwrap() + .unwrap(); + + // Add carol, verified with one unsigned device + let keys_query = DataSet::carol_keys_query_response_signed(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let carol_identity = + machine.get_identity(DataSet::carol_id(), None).await.unwrap().unwrap(); + assert!(carol_identity.other().unwrap().is_verified()); + let carol_unsigned_device = machine + .get_device(DataSet::carol_id(), DataSet::carol_unsigned_device_id(), None) + .await + .unwrap() + .unwrap(); + assert!(!carol_unsigned_device.is_verified()); + + let fake_room_id = room_id!("!roomid:localhost"); + + let strategy = CollectStrategy::new_device_based(false, true); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let id_keys = machine.identity_keys(); + let group_session = OutboundGroupSession::new( + machine.device_id().into(), + Arc::new(id_keys), + fake_room_id, + encryption_settings.clone(), + ) + .unwrap(); + + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::bob_id(), DataSet::carol_id()].into_iter(), + &encryption_settings, + &group_session, + ) + .await; + + assert!(share_result.is_err()); + let share_error = share_result.unwrap_err(); + match share_error { + crate::OlmError::RoomKeySharingStrategyError( + DeviceCollectError::DeviceBasedVerifiedUserHasUnsignedDevice(blockers), + ) => { + assert_eq!(2, blockers.len()); + assert_eq!(1, blockers.get(DataSet::bob_id()).unwrap().len()); + assert_eq!(1, blockers.get(DataSet::carol_id()).unwrap().len()); + + let blocking_bob_device_id = + blockers.get(DataSet::bob_id()).unwrap().iter().next().unwrap(); + assert_eq!(blocking_bob_device_id, bob_unsigned_device.device_id()); + + let blocking_carol_device_id = + blockers.get(DataSet::carol_id()).unwrap().iter().next().unwrap(); + assert_eq!(blocking_carol_device_id, carol_unsigned_device.device_id()); + } + _ => panic!("Unexpected error type"), + } + } + + #[async_test] + async fn test_error_on_unsigned_of_verified_owner_is_us() { + use test_json::keys_query_sets::PreviouslyVerifiedTestData as DataSet; + + let machine = error_on_unsigned_of_verified_machine_setup().await; + let me_unsigned_device = machine + .get_device(DataSet::own_id(), DataSet::own_unsigned_device_id(), None) + .await + .unwrap() + .unwrap(); + + assert!(!me_unsigned_device.is_verified()); + + let fake_room_id = room_id!("!roomid:localhost"); + + let strategy = CollectStrategy::new_device_based(false, true); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let id_keys = machine.identity_keys(); + let group_session = OutboundGroupSession::new( + machine.device_id().into(), + Arc::new(id_keys), + fake_room_id, + encryption_settings.clone(), + ) + .unwrap(); + + let share_result = collect_session_recipients( + machine.store(), + vec![DataSet::own_id()].into_iter(), + &encryption_settings, + &group_session, + ) + .await; + + assert!(share_result.is_err()); + let share_error = share_result.unwrap_err(); + match share_error { + crate::OlmError::RoomKeySharingStrategyError( + DeviceCollectError::DeviceBasedVerifiedUserHasUnsignedDevice(blockers), + ) => { + assert_eq!(1, blockers.len()); + assert_eq!(1, blockers.get(DataSet::own_id()).unwrap().len()); + + let blocking_own_device_id = + blockers.get(DataSet::own_id()).unwrap().iter().next().unwrap(); + assert_eq!(blocking_own_device_id, me_unsigned_device.device_id()); + } + _ => panic!("Unexpected error type"), + } + } #[async_test] async fn test_share_with_identity_strategy() { let machine = set_up_test_machine().await; @@ -567,7 +1087,7 @@ mod tests { let fake_room_id = room_id!("!roomid:localhost"); - let strategy = CollectStrategy::new_device_based(false); + let strategy = CollectStrategy::new_device_based(false, false); let encryption_settings = EncryptionSettings { sharing_strategy: strategy.clone(), @@ -621,7 +1141,7 @@ mod tests { let fake_room_id = room_id!("!roomid:localhost"); - let strategy = CollectStrategy::new_device_based(false); + let strategy = CollectStrategy::new_device_based(false, false); let encryption_settings = EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; diff --git a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs index cfa290427dc..42844707b59 100644 --- a/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs +++ b/testing/matrix-sdk-test/src/test_json/keys_query_sets.rs @@ -690,9 +690,63 @@ impl PreviouslyVerifiedTestData { user_id!("@carol:localhost") } - /// Current user keys query response containing the cross-signing keys + pub fn own_unsigned_device_id() -> &'static DeviceId { + device_id!("AHIVRZICJK") + } + pub fn own_signed_device_id() -> &'static DeviceId { + device_id!("LCNRWQAVWK") + } + + /// Current user keys query response containing the cross-signing keys. + /// Contains also two additional devices, one unsigned and one signed pub fn own_keys_query_response_1() -> KeyQueryResponse { let data = json!({ + "device_keys": { + "@alice:localhost": { + "AHIVRZICJK": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "AHIVRZICJK", + "keys": { + "curve25519:AHIVRZICJK": "3U73fbymtt6sn/H+5UYHiQxN2HfDmxzOMYZ+3JyPT2E", + "ed25519:AHIVRZICJK": "I0NV5nJYmnH+f5py4Fz2tdCeSKUChaaXV7m4UOq9bis" + }, + "signatures": { + "@alice:localhost": { + "ed25519:AHIVRZICJK": "HIs13b2GizN8gdZrYLWs9KZbcmKubXE+O4716Uow513e84JO8REy53OX4TDdoBfmVhPiZg5CIRrUDH7JxY4wAQ" + } + }, + "user_id": "@alice:localhost", + "unsigned": { + "device_display_name": "Element - dbg Android" + } + }, + "LCNRWQAVWK": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "LCNRWQAVWK", + "keys": { + "curve25519:LCNRWQAVWK": "fULFq9I6uYmsdDwRFU76wc43RqF7TVGvlWvKXhSrsS4", + "ed25519:LCNRWQAVWK": "F7E0EF0lzVJN31cnetLdeBuNvZ8jQqkUzt8/nGD9M/E" + }, + "signatures": { + "@alice:localhost": { + "ed25519:LCNRWQAVWK": "8kLsN76ytGRuHKMgIARaOds29QrPRzQ6Px+FOLsYK/ATmx5IVd65MpSh2pGjLAaPsSGWR1WLbBTq/LZtcpjTDQ", + "ed25519:WXLer0esHUanp8DCeu2Be0xB5ms9aKFFBrCFl50COjw": "lo4Vuuu+WvPt1hnOCv30iS1y/cF7DljfFZYF3ib5JH/6iPZTW4jYdlmWo4a7hDf0fb2pu3EFnghYMr7vVx41Aw" + } + }, + "user_id": "@alice:localhost", + "unsigned": { + "device_display_name": "develop.element.io: Chrome on macOSr" + } + } + } + }, + "failures": {}, "master_keys": { "@alice:localhost": { "keys": { @@ -875,9 +929,6 @@ impl PreviouslyVerifiedTestData { } }, "user_id": "@bob:localhost", - "unsigned": { - "device_display_name": "develop.element.io: Chrome on macOS" - } }, "XCYNVRMTER": { "algorithms": [ @@ -897,9 +948,6 @@ impl PreviouslyVerifiedTestData { } }, "user_id": "@bob:localhost", - "unsigned": { - "device_display_name": "app.element.io: Chrome on mac" - } } } }, @@ -946,6 +994,10 @@ impl PreviouslyVerifiedTestData { .expect("Can't parse the `/keys/upload` response") } + pub fn carol_unsigned_device_id() -> &'static DeviceId { + device_id!("BAZAPVEHGA") + } + pub fn device_1_keys_payload_carol() -> Value { json!({ // Not self signed