From 67af31a2b9be8b73ecc294dd91d86ac5c53a2ce8 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 2 Jul 2024 09:33:18 +0200 Subject: [PATCH 01/18] feature(crypto): Add support for master key local pinning --- .../src/identities/manager.rs | 128 ++++++- .../matrix-sdk-crypto/src/identities/user.rs | 343 +++++++++++++----- .../src/test_json/keys_query_sets.rs | 216 ++++++++++- 3 files changed, 602 insertions(+), 85 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/manager.rs b/crates/matrix-sdk-crypto/src/identities/manager.rs index 1fd7de79e9d..ff543f57e94 100644 --- a/crates/matrix-sdk-crypto/src/identities/manager.rs +++ b/crates/matrix-sdk-crypto/src/identities/manager.rs @@ -512,6 +512,7 @@ impl IdentityManager { *changed_private_identity = self.check_private_identity(&identity).await; Ok(identity.into()) } else { + // First time seen, create the identity. The current MSK will be pinned. let identity = OtherUserIdentityData::new(master_key, self_signing)?; Ok(identity.into()) } @@ -1338,7 +1339,7 @@ pub(crate) mod tests { use std::ops::Deref; use futures_util::pin_mut; - use matrix_sdk_test::{async_test, response_from_file}; + use matrix_sdk_test::{async_test, response_from_file, test_json}; use ruma::{ api::{client::keys::get_keys::v3::Response as KeysQueryResponse, IncomingResponse}, device_id, user_id, TransactionId, @@ -1897,4 +1898,129 @@ pub(crate) mod tests { manager.store.get_device_data(other_user, device_id!("OBEBOSKTBE")).await.unwrap().unwrap(); } + + #[async_test] + async fn test_manager_identity_updates() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; + + let manager = manager_test_helper(user_id(), device_id()).await; + let other_user = DataSet::user_id(); + let devices = manager.store.get_user_devices(other_user).await.unwrap(); + assert_eq!(devices.devices().count(), 0); + + let identity = manager.store.get_user_identity(other_user).await.unwrap(); + assert!(identity.is_none()); + + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_a(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // There should be now an identity and no pin violation (pinned msk is the + // current one) + assert!(!other_identity.has_pin_violation()); + let first_device = manager + .store + .get_device_data(other_user, DataSet::first_device_id()) + .await + .unwrap() + .unwrap(); + assert!(first_device.is_cross_signed_by_owner(&identity)); + + // We receive a new keys update for that user, with a new identity + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_b(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // The previous known identity has been replaced, there should be a pin + // violation + assert!(other_identity.has_pin_violation()); + + let second_device = manager + .store + .get_device_data(other_user, DataSet::second_device_id()) + .await + .unwrap() + .unwrap(); + + // There is a new device signed by the new identity + assert!(second_device.is_cross_signed_by_owner(&identity)); + + // The first device should not be signed by the new identity + let first_device = manager + .store + .get_device_data(other_user, DataSet::first_device_id()) + .await + .unwrap() + .unwrap(); + assert!(!first_device.is_cross_signed_by_owner(&identity)); + + let remember_previous_identity = other_identity.clone(); + // We receive a new keys update for that user, with no identity anymore + // Notice that there is no server API to delete identity, but we want to test + // here that a home server cannot clear the identity and serve a new one + // after that would get automatically approved. + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_no_identity(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + assert_eq!(other_identity, &remember_previous_identity); + assert!(other_identity.has_pin_violation()); + } + + #[async_test] + async fn test_manager_resolve_identity_mismatch() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; + + let manager = manager_test_helper(user_id(), device_id()).await; + let other_user = DataSet::user_id(); + + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_a(), + ) + .await + .unwrap(); + + // We receive a new keys update for that user, with a new identity + manager + .receive_keys_query_response( + &TransactionId::new(), + &DataSet::key_query_with_identity_b(), + ) + .await + .unwrap(); + + let identity = manager.store.get_user_identity(other_user).await.unwrap().unwrap(); + let other_identity = identity.other().unwrap(); + + // We have a new identity now, so there should be a pin violation + assert!(other_identity.has_pin_violation()); + + // Resolve the misatch by pinning the new identity + other_identity.pin(); + + assert!(!other_identity.has_pin_violation()); + } } diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 93b7f4bcffc..9229094cf51 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -17,7 +17,7 @@ use std::{ ops::Deref, sync::{ atomic::{AtomicBool, Ordering}, - Arc, + Arc, RwLock, }, }; @@ -30,6 +30,7 @@ use ruma::{ DeviceId, EventId, OwnedDeviceId, OwnedUserId, RoomId, UserId, }; use serde::{Deserialize, Serialize}; +use serde_json::Value; use tracing::error; use super::{atomic_bool_deserializer, atomic_bool_serializer}; @@ -295,6 +296,36 @@ impl UserIdentity { methods, ) } + + /// Pin the current identity (Master public key). + pub async fn pin_current_master_key(&self) -> Result<(), CryptoStoreError> { + self.inner.pin(); + let to_save = UserIdentityData::Other(self.inner.clone()); + let changes = Changes { + identities: IdentityChanges { changed: vec![to_save], ..Default::default() }, + ..Default::default() + }; + self.verification_machine.store.inner().save_changes(changes).await?; + Ok(()) + } + + /// An identity mismatch is detected when there is a trust problem with the + /// user identity. There is an identity mismatch if the current identity + /// is not verified and there is a pinning violation. An identity + /// mismatch must be reported to the user, and can be resolved by: + /// - Verifying the new identity (see + /// [`UserIdentity::request_verification`]) + /// - Or by updating the pinned key + /// ([`UserIdentity::pin_current_master_key`]). + pub fn has_identity_mismatch(&self) -> bool { + // First check if the current identity is verified. + if self.is_verified() { + return false; + } + // If not we can check the pinned identity. Verification always have + // higher priority than pinning. + self.inner.has_pin_violation() + } } /// Enum over the different user identity types we can have. @@ -377,10 +408,87 @@ impl UserIdentityData { /// only contain a master key and a self signing key, meaning that only device /// signatures can be checked with this identity. #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, self_signing_key: Arc, + /// The first time a cryptographic identity is seen for a given user, it + /// will be associated to that user (i.e pinned). Future interaction + /// will expect this user crypto identity to stay the same, + /// this will help prevent some MITM attacks. + /// In case of identity change, it will be possible to pin the new identity + /// is the user wants. + pinned_msk: Arc>, +} + +/// Intermediate struct to help serialize ReadOnlyUserIdentity and support +/// versioning and migration. +/// Version v1 is adding support for identity pinning (`pinned_msk`), as part +/// of migration we just pin the currently known msk. +#[derive(Deserialize, Serialize)] +struct ReadOnlyUserIdentitySerializer { + version: Option, + #[serde(flatten)] + other: Value, +} + +#[derive(Debug, Deserialize, Serialize)] +struct ReadOnlyUserIdentityV0 { + user_id: OwnedUserId, + master_key: MasterPubkey, + self_signing_key: SelfSigningPubkey, +} + +#[derive(Debug, Deserialize, Serialize)] +struct ReadOnlyUserIdentityV1 { + user_id: OwnedUserId, + master_key: MasterPubkey, + self_signing_key: SelfSigningPubkey, + pinned_msk: MasterPubkey, +} + +impl From for OtherUserIdentityData { + fn from(value: ReadOnlyUserIdentitySerializer) -> Self { + match value.version { + None => { + // Old format, migrate the pinned identity + let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other).unwrap(); + OtherUserIdentityData { + user_id: v0.user_id, + master_key: Arc::new(v0.master_key.clone()), + self_signing_key: Arc::new(v0.self_signing_key), + // We migrate by pinning the current msk + pinned_msk: Arc::new(RwLock::new(v0.master_key.clone())), + } + } + _ => { + // v1 format + let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other).unwrap(); + OtherUserIdentityData { + user_id: v1.user_id, + master_key: Arc::new(v1.master_key.clone()), + self_signing_key: Arc::new(v1.self_signing_key), + pinned_msk: Arc::new(RwLock::new(v1.pinned_msk)), + } + } + } + } +} + +impl From for ReadOnlyUserIdentitySerializer { + fn from(value: OtherUserIdentityData) -> Self { + let v1 = ReadOnlyUserIdentityV1 { + user_id: value.user_id.clone(), + master_key: value.master_key().to_owned(), + self_signing_key: value.self_signing_key().to_owned(), + pinned_msk: value.pinned_msk.read().unwrap().clone(), + }; + ReadOnlyUserIdentitySerializer { + version: Some("1".to_owned()), + other: serde_json::to_value(v1).unwrap(), + } + } } impl PartialEq for OtherUserIdentityData { @@ -423,19 +531,24 @@ impl OtherUserIdentityData { Ok(Self { user_id: master_key.user_id().into(), - master_key: master_key.into(), + master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), + pinned_msk: RwLock::new(master_key).into(), }) } #[cfg(test)] pub(crate) async fn from_private(identity: &crate::olm::PrivateCrossSigningIdentity) -> Self { - let master_key = - identity.master_key.lock().await.as_ref().unwrap().public_key().clone().into(); + let master_key = identity.master_key.lock().await.as_ref().unwrap().public_key().clone(); let self_signing_key = identity.self_signing_key.lock().await.as_ref().unwrap().public_key().clone().into(); - Self { user_id: identity.user_id().into(), master_key, self_signing_key } + Self { + user_id: identity.user_id().into(), + master_key: Arc::new(master_key.clone()), + self_signing_key, + pinned_msk: Arc::new(RwLock::new(master_key.clone())), + } } /// Get the user id of this identity. @@ -453,6 +566,24 @@ impl OtherUserIdentityData { &self.self_signing_key } + /// Pin the current identity + pub(crate) fn pin(&self) { + let mut m = self.pinned_msk.write().unwrap(); + *m = self.master_key.as_ref().clone() + } + + /// Key pinning acts as a trust on first use mechanism, the first time an + /// identity is known for a user it will be pinned. + /// For future interaction with a user, the identity is expected to be the + /// one that was pinned. In case of identity change the UI client should + /// receive reports of pinning violation and decide to act accordingly; + /// that is accept and pin the new identity, perform a verification or + /// stop communications. + pub(crate) fn has_pin_violation(&self) -> bool { + let pinned_msk = self.pinned_msk.read().unwrap(); + pinned_msk.get_first_key() != self.master_key().get_first_key() + } + /// Update the identity with a new master key and self signing key. /// /// # Arguments @@ -471,7 +602,15 @@ impl OtherUserIdentityData { ) -> Result { master_key.verify_subkey(&self_signing_key)?; - let new = Self::new(master_key, self_signing_key)?; + // The pin is maintained. + let pinned_msk = self.pinned_msk.read().unwrap().clone(); + + let new = Self { + user_id: master_key.user_id().into(), + master_key: master_key.clone().into(), + self_signing_key: self_signing_key.into(), + pinned_msk: RwLock::new(pinned_msk).into(), + }; let changed = new != *self; *self = new; @@ -792,8 +931,11 @@ pub(crate) mod tests { use std::{collections::HashMap, sync::Arc}; use assert_matches::assert_matches; - use matrix_sdk_test::async_test; - use ruma::{device_id, user_id, UserId}; + use matrix_sdk_test::{async_test, response_from_file, test_json}; + use ruma::{ + api::{client::keys::get_keys::v3::Response as KeyQueryResponse, IncomingResponse}, + device_id, user_id, TransactionId, + }; use serde_json::{json, Value}; use tokio::sync::Mutex; @@ -802,16 +944,16 @@ pub(crate) mod tests { OwnUserIdentityData, UserIdentityData, }; use crate::{ - identities::{manager::testing::own_key_query, Device}, - machine::tests::{ - get_machine_pair, mark_alice_identity_as_verified_test_helper, - setup_cross_signing_for_machine_test_helper, + identities::{ + manager::testing::own_key_query, + user::{ReadOnlyUserIdentitySerializer, ReadOnlyUserIdentityV1}, + Device, }, olm::{Account, PrivateCrossSigningIdentity}, - store::{Changes, CryptoStoreWrapper, MemoryStore}, + store::{CryptoStoreWrapper, MemoryStore}, types::{CrossSigningKey, MasterPubkey, SelfSigningPubkey, Signatures, UserSigningPubkey}, verification::VerificationMachine, - OlmMachine, + OlmMachine, OtherUserIdentityData, }; #[test] @@ -872,6 +1014,56 @@ pub(crate) mod tests { get_other_identity(); } + #[test] + fn deserialization_migration_test() { + let serialized_value = json!({ + "user_id":"@example2:localhost", + "master_key":{ + "user_id":"@example2:localhost", + "usage":[ + "master" + ], + "keys":{ + "ed25519:kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do":"kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do" + }, + "signatures":{ + "@example2:localhost":{ + "ed25519:SKISMLNIMH":"KdUZqzt8VScGNtufuQ8lOf25byYLWIhmUYpPENdmM8nsldexD7vj+Sxoo7PknnTX/BL9h2N7uBq0JuykjunCAw" + } + } + }, + "self_signing_key":{ + "user_id":"@example2:localhost", + "usage":[ + "self_signing" + ], + "keys":{ + "ed25519:ZtFrSkJ1qB8Jph/ql9Eo/lKpIYCzwvKAKXfkaS4XZNc":"ZtFrSkJ1qB8Jph/ql9Eo/lKpIYCzwvKAKXfkaS4XZNc" + }, + "signatures":{ + "@example2:localhost":{ + "ed25519:kC/HmRYw4HNqUp/i4BkwYENrf+hd9tvdB7A1YOf5+Do":"W/O8BnmiUETPpH02mwYaBgvvgF/atXnusmpSTJZeUSH/vHg66xiZOhveQDG4cwaW8iMa+t9N4h1DWnRoHB4mCQ" + } + } + } + }); + let migrated: OtherUserIdentityData = serde_json::from_value(serialized_value).unwrap(); + + let pinned_msk = migrated.pinned_msk.read().unwrap(); + assert_eq!(*pinned_msk, migrated.master_key().clone()); + + // Serialize back + let value = serde_json::to_value(migrated.clone()).unwrap(); + + // Should be serialized with latest version + let _: ReadOnlyUserIdentityV1 = + serde_json::from_value(value.clone()).expect("Should deserialize as version 1"); + + let with_serializer: ReadOnlyUserIdentitySerializer = + serde_json::from_value(value).unwrap(); + assert_eq!("1", with_serializer.version.unwrap()); + } + #[test] fn own_identity_check_signatures() { let response = own_key_query(); @@ -1027,86 +1219,73 @@ pub(crate) mod tests { ); } - async fn get_machine_pair_with_signed_identities( - alice: &UserId, - bob: &UserId, - ) -> (OlmMachine, OlmMachine) { - let (alice, bob, _) = get_machine_pair(alice, bob, false).await; - setup_cross_signing_for_machine_test_helper(&alice, &bob).await; - mark_alice_identity_as_verified_test_helper(&alice, &bob).await; + #[async_test] + async fn resolve_identity_mismacth_with_verification() { + use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; - (alice, bob) - } + let my_user_id = user_id!("@me:localhost"); + let machine = OlmMachine::new(my_user_id, device_id!("ABCDEFGH")).await; + machine.bootstrap_cross_signing(false).await.unwrap(); - #[async_test] - async fn test_other_user_is_verified_if_my_identity_is_verified_and_they_are_cross_signed() { - let alice_user_id = user_id!("@alice:localhost"); - let bob_user_id = user_id!("@bob:localhost"); - let (alice, bob) = - get_machine_pair_with_signed_identities(alice_user_id, bob_user_id).await; + let my_id = machine.get_identity(my_user_id, None).await.unwrap().unwrap().own().unwrap(); + let usk_key_id = my_id.inner.user_signing_key().keys().iter().next().unwrap().0; - let bobs_own_identity = - bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); - let bobs_alice_identity = - bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); + println!("USK ID: {}", usk_key_id); + let keys_query = DataSet::key_query_with_identity_a(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); - assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); - assert!(bobs_alice_identity.is_verified(), "Alice's identity should be verified as well."); - } + // Simulate an identity hange + let keys_query = DataSet::key_query_with_identity_b(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); - #[async_test] - async fn test_other_user_is_not_verified_if_they_are_not_cross_signed() { - let alice_user_id = user_id!("@alice:localhost"); - let bob_user_id = user_id!("@bob:localhost"); - let (alice, bob, _) = get_machine_pair(alice_user_id, bob_user_id, false).await; - setup_cross_signing_for_machine_test_helper(&alice, &bob).await; - - let bobs_own_identity = - bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); - let bobs_alice_identity = - bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); - - assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); - assert!( - !bobs_alice_identity.is_verified(), - "Alice's identity should not be considered verified since Bob has not signed it." - ); - } + let other_user_id = DataSet::user_id(); - #[async_test] - async fn test_other_user_is_not_verified_if_my_identity_is_not_verified() { - let alice_user_id = user_id!("@alice:localhost"); - let bob_user_id = user_id!("@bob:localhost"); + let other_identity = + machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); - let (alice, bob, _) = get_machine_pair(alice_user_id, bob_user_id, false).await; - setup_cross_signing_for_machine_test_helper(&alice, &bob).await; - mark_alice_identity_as_verified_test_helper(&alice, &bob).await; + // There should be an identity mismatch + assert!(other_identity.has_identity_mismatch()); - let bobs_own_identity = - bob.get_identity(bob.user_id(), None).await.unwrap().unwrap().own().unwrap(); - let bobs_alice_identity = - bob.get_identity(alice.user_id(), None).await.unwrap().unwrap().other().unwrap(); + // Manually verify for the purpose of this test + let sig_upload = other_identity.verify().await.unwrap(); - assert!(bobs_own_identity.is_verified(), "Bob's identity should be verified."); - assert!(bobs_alice_identity.is_verified(), "Alice's identity should be verified as well."); + let raw_extracted = + sig_upload.signed_keys.get(other_user_id).unwrap().iter().next().unwrap().1.get(); - bobs_own_identity.mark_as_unverified(); + let new_signature: CrossSigningKey = serde_json::from_str(raw_extracted).unwrap(); - bob.store() - .save_changes(Changes { - identities: crate::store::IdentityChanges { - changed: vec![bobs_own_identity.inner.clone().into()], - ..Default::default() - }, - ..Default::default() - }) - .await - .unwrap(); + let mut msk_to_update: CrossSigningKey = + serde_json::from_value(DataSet::msk_b().get("@bob:localhost").unwrap().clone()) + .unwrap(); - assert!(!bobs_own_identity.is_verified(), "Bob's identity should not be verified anymore."); - assert!( - !bobs_alice_identity.is_verified(), - "Alice's identity should not be verified either." + msk_to_update.signatures.add_signature( + my_user_id.to_owned(), + usk_key_id.to_owned(), + new_signature.signatures.get_signature(my_user_id, usk_key_id).unwrap(), ); + + // we want to update bob device keys with the new signature + let data = json!({ + "device_keys": {}, // For the purpose of this test we don't need devices here + "failures": {}, + "master_keys": { + DataSet::user_id(): msk_to_update + , + }, + "self_signing_keys": DataSet::ssk_b(), + }); + + let kq_response = KeyQueryResponse::try_from_http_response(response_from_file(&data)) + .expect("Can't parse the `/keys/upload` response"); + machine.mark_request_as_sent(&TransactionId::new(), &kq_response).await.unwrap(); + + // There should not be an identity mismatch anymore + let other_identity = + machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); + assert!(!other_identity.has_identity_mismatch()); + // But there is still a pin violation + assert!(other_identity.inner.has_pin_violation()); } } 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 c4e06e190fc..755369902bf 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 @@ -2,7 +2,7 @@ use ruma::{ api::{client::keys::get_keys::v3::Response as KeyQueryResponse, IncomingResponse}, device_id, user_id, DeviceId, UserId, }; -use serde_json::json; +use serde_json::{json, Value}; use crate::response_from_file; @@ -105,7 +105,7 @@ impl KeyDistributionTestData { /// but not the other one `FRGNMZVOKA`. /// `@dan` identity is signed by `@me` identity (alice trust dan) pub fn dan_keys_query_response() -> KeyQueryResponse { - let data: serde_json::Value = json!({ + let data: Value = json!({ "device_keys": { "@dan:localhost": { "JHPUERYQUW": { @@ -457,3 +457,215 @@ impl KeyDistributionTestData { user_id!("@good:localhost") } } + +/// A set of keys query to test identity changes, +/// For user @bob, several payloads with no identities then identity A and B. +pub struct IdentityChangeDataSet {} + +#[allow(dead_code)] +impl IdentityChangeDataSet { + pub fn user_id() -> &'static UserId { + user_id!("@bob:localhost") + } + + pub fn first_device_id() -> &'static DeviceId { + device_id!("GYKSNAWLVK") + } + + pub fn second_device_id() -> &'static DeviceId { + device_id!("ATWKQFSFRN") + } + + pub fn third_device_id() -> &'static DeviceId { + device_id!("OPABMDDXGX") + } + + fn device_keys_payload_1_signed_by_a() -> Value { + json!({ + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "GYKSNAWLVK", + "keys": { + "curve25519:GYKSNAWLVK": "dBcZBzQaiQYWf6rBPh2QypIOB/dxSoTeyaFaxNNbeHs", + "ed25519:GYKSNAWLVK": "6melQNnhoI9sT2b4VzNPAwa8aB179ym45fON8Yo7kVk" + }, + "signatures": { + "@bob:localhost": { + "ed25519:GYKSNAWLVK": "Fk45zHAbrd+1j9wZXLjL2Y/+DU/Mnz9yuvlfYBOOT7qExN2Jdud+5BAuNs8nZ/caS4wTF39Kg3zQpzaGERoCBg", + "ed25519:dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw": "md0Pa1MYlneFb1fp6KCsvZpi2ySb6/G+ULoCbQDWBeDxNEcoNMzf7PEKY04UToCZKUU4LifvRWmiWFDanOlkCQ" + } + }, + "user_id": "@bob:localhost", + }) + } + + fn msk_a() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY" + }, + "signatures": { + "@bob:localhost": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "6vGDbPO5XzlcwbU3aV+kcck+iHHEBtX85ow2gW5U05/DZdtda/JNVa5Nn7B9lQHNnnrMqt1sX00y/JrIkSS1Aw", + "ed25519:GYKSNAWLVK": "jLxmUPr0Ny2Ai9+NGKGhed9BAuKikOc7r6gr7MQVawePYS95w8NJ8Tzaq9zFFOmIiojACNdQ/ksy3QAdwD6vBQ" + } + }, + "usage": [ + "master" + ], + "user_id": "@bob:localhost" + } + }) + } + fn ssk_a() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw": "dO4gmBNW7WC0bXBK81j8uh4me6085fP+keoOm0pH3gw" + }, + "signatures": { + "@bob:localhost": { + "ed25519:/mULSzYNTdHJOBWnBmsvDHhqdHQcWnXRHHmqwzwC7DY": "7md6mwjUK8zjintmffJ0+kImC59/Y8PdySy99EZz5Neu+VMX3LT7txhKO2gC/hmDduRw+JGfGXIiDxR7GmQqDw" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@bob:localhost" + } + }) + } + /// A key query with an identity (Ia), and a first device `GYKSNAWLVK` + /// signed by Ia. + pub fn key_query_with_identity_a() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a() + } + }, + "failures": {}, + "master_keys": Self::msk_a(), + "self_signing_keys": Self::ssk_a(), + "user_signing_keys": {} + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + pub fn msk_b() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4" + }, + "signatures": { + "@bob:localhost": { + "ed25519:ATWKQFSFRN": "MBOzCKYPQLQMpBY2lFZJ4c8451xJfQCdhPBb1AHlTUSxKFiWi6V+k1oRRnhQein/PjkIY7ZO+HoOrIeOtbRMAw", + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "xqLhC3sIUci1W2CNVW7HZWXreQApgjv2RDwB0WPiMd1P4vbZ/qJM0KWqK2piGPWliPi8YVREMrg216KXM3IhCA" + } + }, + "usage": [ + "master" + ], + "user_id": "@bob:localhost" + } + }) + } + + pub fn ssk_b() -> Value { + json!({ + "@bob:localhost": { + "keys": { + "ed25519:At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc": "At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc" + }, + "signatures": { + "@bob:localhost": { + "ed25519:NmI78hY54kE7OZsIjbRE/iCox59t4nzScCNEO6fvtY4": "Ls6CeoA4LoPCHuSwG96kbhd1dEV09TgdMROIZi6vFz/MT9Wtik6joQi/tQ3zCwIZCSR53ksLO4jG1DD31AiBAA" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@bob:localhost" + } + }) + } + + pub fn device_keys_payload_2_signed_by_b() -> Value { + json!({ + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "ATWKQFSFRN", + "keys": { + "curve25519:ATWKQFSFRN": "CY0TWVK1/Kj3ZADuBcGe3UKvpT+IKAPMUsMeJhSDqno", + "ed25519:ATWKQFSFRN": "TyTQqd6j2JlWZh97r+kTYuCbvqnPoNwO6EGovYsjY00" + }, + "signatures": { + "@bob:localhost": { + "ed25519:ATWKQFSFRN": "BQ9Gp0p+6srF+c8OyruqKKd9R4yaub3THYAyyBB/7X/rG8BwcAqFynzl1aGyFYun4Q+087a5OSiglCXI+/kQAA", + "ed25519:At1ai1VUZrCncCI7V7fEAJmBShfpqZ30xRzqcEjTjdc": "TWmDPaG7t0rZ6luauonELD3dmBDTIRryqXhgsIQRiGint2rJdic8RVyZ6a61bgu6mtBjfvU3prqMNp6sVi16Cg" + } + }, + "user_id": "@bob:localhost", + }) + } + /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. + /// `ATWKQFSFRN` is signed with the new identity but + pub fn key_query_with_identity_b() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "ATWKQFSFRN": Self::device_keys_payload_2_signed_by_b(), + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a(), + } + }, + "failures": {}, + "master_keys": Self::msk_b(), + "self_signing_keys": Self::ssk_b(), + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. + /// `ATWKQFSFRN` is signed with the new identity but + pub fn key_query_with_identity_no_identity() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@bob:localhost": { + "ATWKQFSFRN": Self::device_keys_payload_2_signed_by_b(), + "GYKSNAWLVK": Self::device_keys_payload_1_signed_by_a(), + "OPABMDDXGX": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "OPABMDDXGX", + "keys": { + "curve25519:OPABMDDXGX": "O6bwa9Op0E+PQPCrbTOfdYwU+j95RRPhXIHuNpe94ns", + "ed25519:OPABMDDXGX": "DvjkSNOM9XrR1gWrr2YSDvTnwnLIgKDMRr5v8HgMKak" + }, + "signatures": { + "@bob:localhost": { + "ed25519:OPABMDDXGX": "o+BBnw/SIJWxSf799Adq6jEl9X3lwCg5MJkS8GlfId+pW3ReEETK0l+9bhCAgBsNSKRtB/fmZQBhjMx4FJr+BA" + } + }, + "user_id": "@bob:localhost", + "unsigned": { + "device_display_name": "develop.element.io: Chrome on macOS" + } + } + } + }, + "failures": {}, + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } +} From 8253618a9b9279b7a366cd1b4e889cd51fa078c3 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 11:23:10 +0200 Subject: [PATCH 02/18] Review: Do not use msk abbreviation --- .../matrix-sdk-crypto/src/identities/user.rs | 48 ++++++++++--------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 9229094cf51..db8541cc7c8 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -407,25 +407,27 @@ impl UserIdentityData { /// This is the user identity of a user that isn't our own. Other users will /// only contain a master key and a self signing key, meaning that only device /// signatures can be checked with this identity. +/// +/// This struct also contains the currently pinned master key for that user. +/// The first time a cryptographic identity is seen for a given user, it +/// will be associated to that user (i.e. pinned). Future interactions +/// will expect this user crypto identity to stay the same, +/// this will help prevent some MITM attacks. +/// In case of identity change, it will be possible to pin the new identity +/// is the user wants. #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, self_signing_key: Arc, - /// The first time a cryptographic identity is seen for a given user, it - /// will be associated to that user (i.e pinned). Future interaction - /// will expect this user crypto identity to stay the same, - /// this will help prevent some MITM attacks. - /// In case of identity change, it will be possible to pin the new identity - /// is the user wants. - pinned_msk: Arc>, + pinned_master_key: Arc>, } /// Intermediate struct to help serialize ReadOnlyUserIdentity and support /// versioning and migration. -/// Version v1 is adding support for identity pinning (`pinned_msk`), as part -/// of migration we just pin the currently known msk. +/// Version v1 is adding support for identity pinning (`pinned_master_key`), as +/// part of migration we just pin the currently known master key. #[derive(Deserialize, Serialize)] struct ReadOnlyUserIdentitySerializer { version: Option, @@ -445,7 +447,7 @@ struct ReadOnlyUserIdentityV1 { user_id: OwnedUserId, master_key: MasterPubkey, self_signing_key: SelfSigningPubkey, - pinned_msk: MasterPubkey, + pinned_master_key: MasterPubkey, } impl From for OtherUserIdentityData { @@ -458,8 +460,8 @@ impl From for OtherUserIdentityData { user_id: v0.user_id, master_key: Arc::new(v0.master_key.clone()), self_signing_key: Arc::new(v0.self_signing_key), - // We migrate by pinning the current msk - pinned_msk: Arc::new(RwLock::new(v0.master_key.clone())), + // We migrate by pinning the current master key + pinned_master_key: Arc::new(RwLock::new(v0.master_key.clone())), } } _ => { @@ -469,7 +471,7 @@ impl From for OtherUserIdentityData { user_id: v1.user_id, master_key: Arc::new(v1.master_key.clone()), self_signing_key: Arc::new(v1.self_signing_key), - pinned_msk: Arc::new(RwLock::new(v1.pinned_msk)), + pinned_master_key: Arc::new(RwLock::new(v1.pinned_master_key)), } } } @@ -482,7 +484,7 @@ impl From for ReadOnlyUserIdentitySerializer { user_id: value.user_id.clone(), master_key: value.master_key().to_owned(), self_signing_key: value.self_signing_key().to_owned(), - pinned_msk: value.pinned_msk.read().unwrap().clone(), + pinned_master_key: value.pinned_master_key.read().unwrap().clone(), }; ReadOnlyUserIdentitySerializer { version: Some("1".to_owned()), @@ -533,7 +535,7 @@ impl OtherUserIdentityData { user_id: master_key.user_id().into(), master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), - pinned_msk: RwLock::new(master_key).into(), + pinned_master_key: RwLock::new(master_key).into(), }) } @@ -547,7 +549,7 @@ impl OtherUserIdentityData { user_id: identity.user_id().into(), master_key: Arc::new(master_key.clone()), self_signing_key, - pinned_msk: Arc::new(RwLock::new(master_key.clone())), + pinned_master_key: Arc::new(RwLock::new(master_key.clone())), } } @@ -568,7 +570,7 @@ impl OtherUserIdentityData { /// Pin the current identity pub(crate) fn pin(&self) { - let mut m = self.pinned_msk.write().unwrap(); + let mut m = self.pinned_master_key.write().unwrap(); *m = self.master_key.as_ref().clone() } @@ -580,8 +582,8 @@ impl OtherUserIdentityData { /// that is accept and pin the new identity, perform a verification or /// stop communications. pub(crate) fn has_pin_violation(&self) -> bool { - let pinned_msk = self.pinned_msk.read().unwrap(); - pinned_msk.get_first_key() != self.master_key().get_first_key() + let pinned_master_key = self.pinned_master_key.read().unwrap(); + pinned_master_key.get_first_key() != self.master_key().get_first_key() } /// Update the identity with a new master key and self signing key. @@ -603,13 +605,13 @@ impl OtherUserIdentityData { master_key.verify_subkey(&self_signing_key)?; // The pin is maintained. - let pinned_msk = self.pinned_msk.read().unwrap().clone(); + let pinned_master_key = self.pinned_master_key.read().unwrap().clone(); let new = Self { user_id: master_key.user_id().into(), master_key: master_key.clone().into(), self_signing_key: self_signing_key.into(), - pinned_msk: RwLock::new(pinned_msk).into(), + pinned_master_key: RwLock::new(pinned_master_key).into(), }; let changed = new != *self; @@ -1049,8 +1051,8 @@ pub(crate) mod tests { }); let migrated: OtherUserIdentityData = serde_json::from_value(serialized_value).unwrap(); - let pinned_msk = migrated.pinned_msk.read().unwrap(); - assert_eq!(*pinned_msk, migrated.master_key().clone()); + let pinned_master_key = migrated.pinned_master_key.read().unwrap(); + assert_eq!(*pinned_master_key, migrated.master_key().clone()); // Serialize back let value = serde_json::to_value(migrated.clone()).unwrap(); From b7241c305b2b8423903a7c901c4d5604089a1cc8 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 15:41:55 +0200 Subject: [PATCH 03/18] Review: Use TryFrom instead of From for serializer helper struct --- .../matrix-sdk-crypto/src/identities/user.rs | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index db8541cc7c8..27eacee807f 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -416,7 +416,7 @@ impl UserIdentityData { /// In case of identity change, it will be possible to pin the new identity /// is the user wants. #[derive(Debug, Clone, Deserialize, Serialize)] -#[serde(from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] +#[serde(try_from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, @@ -450,30 +450,33 @@ struct ReadOnlyUserIdentityV1 { pinned_master_key: MasterPubkey, } -impl From for OtherUserIdentityData { - fn from(value: ReadOnlyUserIdentitySerializer) -> Self { +impl TryFrom for OtherUserIdentityData { + type Error = serde_json::Error; + fn try_from( + value: ReadOnlyUserIdentitySerializer, + ) -> Result { match value.version { None => { // Old format, migrate the pinned identity - let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other).unwrap(); - OtherUserIdentityData { + let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other)?; + Ok(OtherUserIdentityData { user_id: v0.user_id, master_key: Arc::new(v0.master_key.clone()), self_signing_key: Arc::new(v0.self_signing_key), // We migrate by pinning the current master key pinned_master_key: Arc::new(RwLock::new(v0.master_key.clone())), - } + }) } - _ => { - // v1 format - let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other).unwrap(); - OtherUserIdentityData { + Some(v) if v == "1" => { + let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other)?; + Ok(OtherUserIdentityData { user_id: v1.user_id, master_key: Arc::new(v1.master_key.clone()), self_signing_key: Arc::new(v1.self_signing_key), pinned_master_key: Arc::new(RwLock::new(v1.pinned_master_key)), - } + }) } + _ => Err(serde::de::Error::custom(format!("Unsupported Version {:?}", value.version))), } } } From 4d668280ab0593165f1b9d5a3038cd4f14e1a798 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 15:55:13 +0200 Subject: [PATCH 04/18] Review: Fix typo --- crates/matrix-sdk-crypto/src/identities/user.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 27eacee807f..d2f03329018 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -1225,7 +1225,7 @@ pub(crate) mod tests { } #[async_test] - async fn resolve_identity_mismacth_with_verification() { + async fn resolve_identity_mismatch_with_verification() { use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; let my_user_id = user_id!("@me:localhost"); From c045d26d0693f363c90085dce50099cca3571dbb Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 15:55:50 +0200 Subject: [PATCH 05/18] Review: Improve doc --- crates/matrix-sdk-crypto/src/identities/user.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index d2f03329018..ebfdd250f05 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -309,14 +309,17 @@ impl UserIdentity { Ok(()) } + /// Returns true if the identity is not verified and did change since the + /// last time we pinned it. + /// /// An identity mismatch is detected when there is a trust problem with the /// user identity. There is an identity mismatch if the current identity /// is not verified and there is a pinning violation. An identity /// mismatch must be reported to the user, and can be resolved by: /// - Verifying the new identity (see /// [`UserIdentity::request_verification`]) - /// - Or by updating the pinned key - /// ([`UserIdentity::pin_current_master_key`]). + /// - Or by updating the pinned key (see + /// [`UserIdentity::pin_current_master_key`]). pub fn has_identity_mismatch(&self) -> bool { // First check if the current identity is verified. if self.is_verified() { @@ -577,6 +580,8 @@ impl OtherUserIdentityData { *m = self.master_key.as_ref().clone() } + /// Returns true if the identity has changed since we last pinned it. + /// /// Key pinning acts as a trust on first use mechanism, the first time an /// identity is known for a user it will be pinned. /// For future interaction with a user, the identity is expected to be the From bfb88183ccb678b85cc3423c8f89118911268934 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 16:15:53 +0200 Subject: [PATCH 06/18] Review: Fix data set comments --- .../matrix-sdk-test/src/test_json/keys_query_sets.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) 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 755369902bf..bf5c1a3321e 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 @@ -616,7 +616,8 @@ impl IdentityChangeDataSet { }) } /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. - /// `ATWKQFSFRN` is signed with the new identity but + /// `ATWKQFSFRN` is signed with the new identity but `GYKSNAWLVK` is still + /// signed by the old identity (Ia). pub fn key_query_with_identity_b() -> KeyQueryResponse { let data = response_from_file(&json!({ "device_keys": { @@ -633,8 +634,8 @@ impl IdentityChangeDataSet { .expect("Can't parse the `/keys/upload` response") } - /// A key query with a new identity (Ib) and a new device `ATWKQFSFRN`. - /// `ATWKQFSFRN` is signed with the new identity but + /// A key query with no identity and a new device `OPABMDDXGX` (not + /// cross-signed). pub fn key_query_with_identity_no_identity() -> KeyQueryResponse { let data = response_from_file(&json!({ "device_keys": { @@ -657,9 +658,6 @@ impl IdentityChangeDataSet { } }, "user_id": "@bob:localhost", - "unsigned": { - "device_display_name": "develop.element.io: Chrome on macOS" - } } } }, From fb39a9ec82eb230f8a68d3d72bb0985a143d1c0b Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 17 Jul 2024 16:29:54 +0200 Subject: [PATCH 07/18] Review: Improve comments --- crates/matrix-sdk-crypto/src/identities/manager.rs | 2 +- crates/matrix-sdk-crypto/src/identities/user.rs | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/manager.rs b/crates/matrix-sdk-crypto/src/identities/manager.rs index ff543f57e94..61445a03c1e 100644 --- a/crates/matrix-sdk-crypto/src/identities/manager.rs +++ b/crates/matrix-sdk-crypto/src/identities/manager.rs @@ -1969,7 +1969,7 @@ pub(crate) mod tests { assert!(!first_device.is_cross_signed_by_owner(&identity)); let remember_previous_identity = other_identity.clone(); - // We receive a new keys update for that user, with no identity anymore + // We receive updated keys for that user, with no identity anymore. // Notice that there is no server API to delete identity, but we want to test // here that a home server cannot clear the identity and serve a new one // after that would get automatically approved. diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index ebfdd250f05..997e0b70df4 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -612,7 +612,10 @@ impl OtherUserIdentityData { ) -> Result { master_key.verify_subkey(&self_signing_key)?; - // The pin is maintained. + // We update the identity with the new master and self signing key, but we keep + // the previous pinned master key. + // This identity will have a pin violation until the new master key is pinned + // (see [`has_pin_violation`]). let pinned_master_key = self.pinned_master_key.read().unwrap().clone(); let new = Self { From 1d4cefbe90b9c671ae4d7b6166c1c52e204d216d Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 22 Jul 2024 17:29:06 +0200 Subject: [PATCH 08/18] Clippy | unneeded clone --- crates/matrix-sdk-crypto/src/identities/user.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 997e0b70df4..1e280c60af2 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -467,7 +467,7 @@ impl TryFrom for OtherUserIdentityData { master_key: Arc::new(v0.master_key.clone()), self_signing_key: Arc::new(v0.self_signing_key), // We migrate by pinning the current master key - pinned_master_key: Arc::new(RwLock::new(v0.master_key.clone())), + pinned_master_key: Arc::new(RwLock::new(v0.master_key)), }) } Some(v) if v == "1" => { From de60e6ed647a2fac4a3e604b14412f1f1111b877 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 22 Jul 2024 17:35:12 +0200 Subject: [PATCH 09/18] refactor: Rename serializer helper to match new OtherUserIdentityData --- .../matrix-sdk-crypto/src/identities/user.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index 1e280c60af2..8de38656ea1 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -419,7 +419,7 @@ impl UserIdentityData { /// In case of identity change, it will be possible to pin the new identity /// is the user wants. #[derive(Debug, Clone, Deserialize, Serialize)] -#[serde(try_from = "ReadOnlyUserIdentitySerializer", into = "ReadOnlyUserIdentitySerializer")] +#[serde(try_from = "OtherUserIdentityDataSerializer", into = "OtherUserIdentityDataSerializer")] pub struct OtherUserIdentityData { user_id: OwnedUserId, pub(crate) master_key: Arc, @@ -427,41 +427,41 @@ pub struct OtherUserIdentityData { pinned_master_key: Arc>, } -/// Intermediate struct to help serialize ReadOnlyUserIdentity and support +/// Intermediate struct to help serialize OtherUserIdentityData and support /// versioning and migration. /// Version v1 is adding support for identity pinning (`pinned_master_key`), as /// part of migration we just pin the currently known master key. #[derive(Deserialize, Serialize)] -struct ReadOnlyUserIdentitySerializer { +struct OtherUserIdentityDataSerializer { version: Option, #[serde(flatten)] other: Value, } #[derive(Debug, Deserialize, Serialize)] -struct ReadOnlyUserIdentityV0 { +struct OtherUserIdentityDataSerializerV0 { user_id: OwnedUserId, master_key: MasterPubkey, self_signing_key: SelfSigningPubkey, } #[derive(Debug, Deserialize, Serialize)] -struct ReadOnlyUserIdentityV1 { +struct OtherUserIdentityDataSerializerV1 { user_id: OwnedUserId, master_key: MasterPubkey, self_signing_key: SelfSigningPubkey, pinned_master_key: MasterPubkey, } -impl TryFrom for OtherUserIdentityData { +impl TryFrom for OtherUserIdentityData { type Error = serde_json::Error; fn try_from( - value: ReadOnlyUserIdentitySerializer, + value: OtherUserIdentityDataSerializer, ) -> Result { match value.version { None => { // Old format, migrate the pinned identity - let v0: ReadOnlyUserIdentityV0 = serde_json::from_value(value.other)?; + let v0: OtherUserIdentityDataSerializerV0 = serde_json::from_value(value.other)?; Ok(OtherUserIdentityData { user_id: v0.user_id, master_key: Arc::new(v0.master_key.clone()), @@ -471,7 +471,7 @@ impl TryFrom for OtherUserIdentityData { }) } Some(v) if v == "1" => { - let v1: ReadOnlyUserIdentityV1 = serde_json::from_value(value.other)?; + let v1: OtherUserIdentityDataSerializerV1 = serde_json::from_value(value.other)?; Ok(OtherUserIdentityData { user_id: v1.user_id, master_key: Arc::new(v1.master_key.clone()), @@ -484,15 +484,15 @@ impl TryFrom for OtherUserIdentityData { } } -impl From for ReadOnlyUserIdentitySerializer { +impl From for OtherUserIdentityDataSerializer { fn from(value: OtherUserIdentityData) -> Self { - let v1 = ReadOnlyUserIdentityV1 { + let v1 = OtherUserIdentityDataSerializerV1 { user_id: value.user_id.clone(), master_key: value.master_key().to_owned(), self_signing_key: value.self_signing_key().to_owned(), pinned_master_key: value.pinned_master_key.read().unwrap().clone(), }; - ReadOnlyUserIdentitySerializer { + OtherUserIdentityDataSerializer { version: Some("1".to_owned()), other: serde_json::to_value(v1).unwrap(), } @@ -959,7 +959,7 @@ pub(crate) mod tests { use crate::{ identities::{ manager::testing::own_key_query, - user::{ReadOnlyUserIdentitySerializer, ReadOnlyUserIdentityV1}, + user::{OtherUserIdentityDataSerializer, OtherUserIdentityDataSerializerV1}, Device, }, olm::{Account, PrivateCrossSigningIdentity}, @@ -1069,10 +1069,10 @@ pub(crate) mod tests { let value = serde_json::to_value(migrated.clone()).unwrap(); // Should be serialized with latest version - let _: ReadOnlyUserIdentityV1 = + let _: OtherUserIdentityDataSerializerV1 = serde_json::from_value(value.clone()).expect("Should deserialize as version 1"); - let with_serializer: ReadOnlyUserIdentitySerializer = + let with_serializer: OtherUserIdentityDataSerializer = serde_json::from_value(value).unwrap(); assert_eq!("1", with_serializer.version.unwrap()); } From 78036173beb84ca9bfbd6ef6eeae335e6aa25ca5 Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 3 Jul 2024 11:24:11 +0200 Subject: [PATCH 10/18] crypto: refactor extract detection of removed device from outbound --- .../group_sessions/share_strategy.rs | 67 +++++++++++-------- 1 file changed, 40 insertions(+), 27 deletions(-) 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 c2bec06f962..151583ff0b3 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 @@ -162,33 +162,7 @@ pub(crate) async fn collect_session_recipients( // of the devices in the session got deleted or blacklisted in the // meantime. If so, we should also rotate the session. if !should_rotate { - // Device IDs that should receive this session - let recipient_device_ids: BTreeSet<&DeviceId> = - recipients.iter().map(|d| d.device_id()).collect(); - - if let Some(shared) = outbound.shared_with_set.read().unwrap().get(user_id) { - // Devices that received this session - let shared: BTreeSet = shared.keys().cloned().collect(); - let shared: BTreeSet<&DeviceId> = shared.iter().map(|d| d.as_ref()).collect(); - - // The set difference between - // - // 1. Devices that had previously received the session, and - // 2. Devices that would now receive the session - // - // Represents newly deleted or blacklisted devices. If this - // set is non-empty, we must rotate. - let newly_deleted_or_blacklisted = - shared.difference(&recipient_device_ids).collect::>(); - - should_rotate = !newly_deleted_or_blacklisted.is_empty(); - if should_rotate { - debug!( - "Rotating a room key due to these devices being deleted/blacklisted {:?}", - newly_deleted_or_blacklisted, - ); - } - }; + should_rotate = should_rotate_due_to_left_device(user_id, &recipients, outbound); } devices.entry(user_id.to_owned()).or_default().extend(recipients); @@ -209,6 +183,45 @@ pub(crate) async fn collect_session_recipients( Ok(CollectRecipientsResult { should_rotate, devices, withheld_devices }) } +// Check whether any of the devices currently in the discussion (outbound +// session stores the devices it already got shared with) got deleted or +// blacklisted in the meantime. If it's the case the session should be rotated. +fn should_rotate_due_to_left_device( + user_id: &UserId, + future_recipients: &Vec, + outbound: &OutboundGroupSession, +) -> bool { + // Device IDs that should receive this session + let recipient_device_ids: BTreeSet<&DeviceId> = + future_recipients.iter().map(|d| d.device_id()).collect(); + + if let Some(shared) = outbound.shared_with_set.read().unwrap().get(user_id) { + // Devices that received this session + let shared: BTreeSet = shared.keys().cloned().collect(); + let shared: BTreeSet<&DeviceId> = shared.iter().map(|d| d.as_ref()).collect(); + + // The set difference between + // + // 1. Devices that had previously received the session, and + // 2. Devices that would now receive the session + // + // Represents newly deleted or blacklisted devices. If this + // set is non-empty, we must rotate. + let newly_deleted_or_blacklisted = + shared.difference(&recipient_device_ids).collect::>(); + + let should_rotate = !newly_deleted_or_blacklisted.is_empty(); + if should_rotate { + debug!( + "Rotating a room key due to these devices being deleted/blacklisted {:?}", + newly_deleted_or_blacklisted, + ); + } + return should_rotate; + }; + false +} + struct RecipientDevices { allowed_devices: Vec, denied_devices_with_code: Vec<(DeviceData, WithheldCode)>, From 9b00de374b5a4b74dcdb12299a5de7d973c6154b Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 3 Jul 2024 16:31:42 +0200 Subject: [PATCH 11/18] refactor: Move user loop inside strategy specific branch --- .../group_sessions/share_strategy.rs | 83 +++++++++++++------ 1 file changed, 56 insertions(+), 27 deletions(-) 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 151583ff0b3..54fb035036d 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 @@ -98,8 +98,6 @@ pub(crate) async fn collect_session_recipients( outbound: &OutboundGroupSession, ) -> OlmResult { let users: BTreeSet<&UserId> = users.collect(); - let mut devices: BTreeMap> = Default::default(); - let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default(); trace!(?users, ?settings, "Calculating group session recipients"); @@ -125,49 +123,80 @@ pub(crate) async fn collect_session_recipients( // This is calculated in the following code and stored in this variable. let mut should_rotate = user_left || visibility_changed || algorithm_changed; - let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + let mut devices: BTreeMap> = Default::default(); + let mut withheld_devices: Vec<(ReadOnlyDevice, WithheldCode)> = Default::default(); - for user_id in users { - let user_devices = store.get_device_data_for_user_filtered(user_id).await?; + match settings.sharing_strategy { + CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } => { + let own_identity = + store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + for user_id in users { + let user_devices = store.get_readonly_devices_filtered(user_id).await?; - let recipient_devices = match settings.sharing_strategy { - CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } => { // 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 }; - split_recipients_withhelds_for_user( + let recipient_devices = split_recipients_withhelds_for_user( user_devices, &own_identity, &device_owner_identity, only_allow_trusted_devices, - ) + ); + + // If we haven't already concluded that the session should be + // rotated for other reasons, we also need to check whether any + // of the devices in the session got deleted or blacklisted in the + // meantime. If so, we should also rotate the session. + if !should_rotate { + should_rotate = should_rotate_due_to_left_device( + user_id, + &recipient_devices.allowed_devices, + outbound, + ); + } + + devices + .entry(user_id.to_owned()) + .or_default() + .extend(recipient_devices.allowed_devices); + withheld_devices.extend(recipient_devices.denied_devices_with_code); } - CollectStrategy::IdentityBasedStrategy => { + } + CollectStrategy::IdentityBasedStrategy => { + let _maybe_own_identity = + store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + + for user_id in users { + let user_devices = store.get_readonly_devices_filtered(user_id).await?; let device_owner_identity = store.get_user_identity(user_id).await?; - split_recipients_withhelds_for_user_based_on_identity( + let recipient_devices = split_recipients_withhelds_for_user_based_on_identity( user_devices, &device_owner_identity, - ) - } - }; - - let recipients = recipient_devices.allowed_devices; - let withheld_recipients = recipient_devices.denied_devices_with_code; + ); + + // If we haven't already concluded that the session should be + // rotated for other reasons, we also need to check whether any + // of the devices in the session got deleted or blacklisted in the + // meantime. If so, we should also rotate the session. + if !should_rotate { + should_rotate = should_rotate_due_to_left_device( + user_id, + &recipient_devices.allowed_devices, + outbound, + ); + } - // If we haven't already concluded that the session should be - // rotated for other reasons, we also need to check whether any - // of the devices in the session got deleted or blacklisted in the - // meantime. If so, we should also rotate the session. - if !should_rotate { - should_rotate = should_rotate_due_to_left_device(user_id, &recipients, outbound); + devices + .entry(user_id.to_owned()) + .or_default() + .extend(recipient_devices.allowed_devices); + withheld_devices.extend(recipient_devices.denied_devices_with_code); + } } - - devices.entry(user_id.to_owned()).or_default().extend(recipients); - withheld_devices.extend(withheld_recipients); - } + }; if should_rotate { debug!( From e3092195ae1bec2641a3d0d687b101c1549b9259 Mon Sep 17 00:00:00 2001 From: Valere Date: Mon, 8 Jul 2024 14:45:04 +0200 Subject: [PATCH 12/18] feat(crypto): Key distribution errors for Pin violations --- crates/matrix-sdk-crypto/src/error.rs | 26 ++ .../group_sessions/share_strategy.rs | 230 +++++++++++++++++- .../src/test_json/keys_query_sets.rs | 146 +++++++++++ 3 files changed, 392 insertions(+), 10 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index c73a8ed27e9..5578f67753d 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -69,8 +69,34 @@ pub enum OlmError { have a valid Olm session with us" )] MissingSession, + #[error(transparent)] + /// The room key that should be shared was not due to an error. + KeyDistributionError(RoomKeyDistributionError), } +/// Depending on the sharing strategy for room keys, the distribution of the +/// room key could fail. +#[derive(Error, Debug)] +pub enum RoomKeyDistributionError { + /// When encrypting using the IdentityBased strategy. + /// Will be thrown when sharing room keys when there is a new identity for a + /// user that has not been confirmed by the user. + /// Application should display identity changes to the user as soon as + /// possible to avoid hitting this case. If it happens the app might + /// just retry automatically after the identity change has been + /// notified, or offer option to cancel. + #[error("Encryption failed because there are key pinning violation, please re-pin or verify the problematic users")] + KeyPinningViolation(Vec), + + /// Cross-signing is required for encryption with invisible crypto + #[error("Encryption failed: Setup cross-signing on your account")] + CrossSigningNotSetup, + /// The current device needs to be verified when encrypting using the + /// IdentityBased strategy. Apps should prevent sending in the UI to + /// avoid hitting this case. + #[error("Encryption failed: Verify your device to send encrypted messages")] + SendingFromUnverifiedDevice, +} /// Error representing a failure during a group encryption operation. #[derive(Error, Debug)] pub enum MegolmError { 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 54fb035036d..86ee62d9523 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::{OlmResult, RoomKeyDistributionError}, + 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 @@ -123,15 +125,15 @@ pub(crate) async fn collect_session_recipients( // This is calculated in the following code and stored in this variable. let mut should_rotate = user_left || visibility_changed || algorithm_changed; - let mut devices: BTreeMap> = Default::default(); - let mut withheld_devices: Vec<(ReadOnlyDevice, WithheldCode)> = Default::default(); + let mut devices: BTreeMap> = Default::default(); + let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default(); match settings.sharing_strategy { CollectStrategy::DeviceBasedStrategy { only_allow_trusted_devices } => { let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); for user_id in users { - let user_devices = store.get_readonly_devices_filtered(user_id).await?; + let user_devices = store.get_device_data_for_user(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 { @@ -166,12 +168,42 @@ pub(crate) async fn collect_session_recipients( } } CollectStrategy::IdentityBasedStrategy => { - let _maybe_own_identity = + let maybe_own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); + let own_verified_identity = match maybe_own_identity { + None => { + return Err(OlmError::KeyDistributionError( + RoomKeyDistributionError::CrossSigningNotSetup, + )) + } + Some(identity) if !identity.is_verified() => { + return Err(OlmError::KeyDistributionError( + RoomKeyDistributionError::SendingFromUnverifiedDevice, + )) + } + Some(identity) => identity, + }; + + // Collect all potential identity mismatch and report at the end if any. + let mut users_with_identity_mismatch: Vec = Vec::default(); for user_id in users { - let user_devices = store.get_readonly_devices_filtered(user_id).await?; let device_owner_identity = store.get_user_identity(user_id).await?; + // debug!(?device_owner_identity, "Other Checking identity"); + + if let Some(other_identity) = device_owner_identity.as_ref().and_then(|i| i.other()) + { + if other_identity.has_pin_violation() + && own_verified_identity.is_identity_signed(other_identity).is_err() + { + debug!(?other_identity, "Identity Mismatch detected"); + // Identity mismatch + users_with_identity_mismatch.push(other_identity.user_id().to_owned()); + continue; + } + } + let user_devices = store.get_device_data_for_user_filtered(user_id).await?; + let recipient_devices = split_recipients_withhelds_for_user_based_on_identity( user_devices, &device_owner_identity, @@ -195,6 +227,13 @@ pub(crate) async fn collect_session_recipients( .extend(recipient_devices.allowed_devices); withheld_devices.extend(recipient_devices.denied_devices_with_code); } + + // Abort sharing and fail when there are identities mismatch. + if !users_with_identity_mismatch.is_empty() { + return Err(OlmError::KeyDistributionError( + RoomKeyDistributionError::KeyPinningViolation(users_with_identity_mismatch), + )); + } } }; @@ -217,7 +256,7 @@ pub(crate) async fn collect_session_recipients( // blacklisted in the meantime. If it's the case the session should be rotated. fn should_rotate_due_to_left_device( user_id: &UserId, - future_recipients: &Vec, + future_recipients: &[DeviceData], outbound: &OutboundGroupSession, ) -> bool { // Device IDs that should receive this session @@ -323,7 +362,12 @@ mod tests { use std::sync::Arc; - use matrix_sdk_test::{async_test, test_json::keys_query_sets::KeyDistributionTestData}; + use matrix_sdk_test::{ + async_test, + test_json::keys_query_sets::{ + IdentityChangeDataSet, KeyDistributionTestData, MaloIdentityChangeDataSet, + }, + }; use ruma::{events::room::history_visibility::HistoryVisibility, room_id, TransactionId}; use crate::{ @@ -332,7 +376,7 @@ mod tests { group_sessions::share_strategy::collect_session_recipients, CollectStrategy, }, types::events::room_key_withheld::WithheldCode, - CrossSigningKeyExport, EncryptionSettings, OlmMachine, + CrossSigningKeyExport, EncryptionSettings, OlmError, OlmMachine, }; async fn set_up_test_machine() -> OlmMachine { @@ -570,6 +614,172 @@ mod tests { assert_eq!(code, &WithheldCode::Unauthorised); } + #[async_test] + async fn test_share_identity_strategy_no_cross_signing() { + let machine: OlmMachine = OlmMachine::new( + KeyDistributionTestData::me_id(), + KeyDistributionTestData::me_device_id(), + ) + .await; + + let keys_query = KeyDistributionTestData::dan_keys_query_response(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + + let strategy = CollectStrategy::new_identity_based(); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let request_result = machine + .share_room_key( + fake_room_id, + vec![KeyDistributionTestData::dan_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_err()); + let err = request_result.unwrap_err(); + match err { + OlmError::KeyDistributionError( + crate::error::RoomKeyDistributionError::CrossSigningNotSetup, + ) => { + // ok + } + _ => panic!("Unexpected share error"), + } + + // Let's now have cross-signing, but the identity is not trusted + let keys_query = KeyDistributionTestData::me_keys_query_response(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let request_result = machine + .share_room_key( + fake_room_id, + vec![KeyDistributionTestData::dan_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_err()); + let err = request_result.unwrap_err(); + match err { + OlmError::KeyDistributionError( + crate::error::RoomKeyDistributionError::SendingFromUnverifiedDevice, + ) => { + // ok + } + _ => panic!("Unexpected share error"), + } + + // If we are verified it should then work + machine + .import_cross_signing_keys(CrossSigningKeyExport { + master_key: KeyDistributionTestData::MASTER_KEY_PRIVATE_EXPORT.to_owned().into(), + self_signing_key: KeyDistributionTestData::SELF_SIGNING_KEY_PRIVATE_EXPORT + .to_owned() + .into(), + user_signing_key: KeyDistributionTestData::USER_SIGNING_KEY_PRIVATE_EXPORT + .to_owned() + .into(), + }) + .await + .unwrap(); + + let request_result = machine + .share_room_key( + fake_room_id, + vec![KeyDistributionTestData::dan_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_ok()); + } + + #[async_test] + async fn test_share_identity_strategy_report_pinning_violation() { + let machine: OlmMachine = OlmMachine::new( + KeyDistributionTestData::me_id(), + KeyDistributionTestData::me_device_id(), + ) + .await; + + machine.bootstrap_cross_signing(false).await.unwrap(); + + let keys_query = IdentityChangeDataSet::key_query_with_identity_a(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let keys_query = MaloIdentityChangeDataSet::initial_key_query(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + // Simulate pinning violation + let keys_query = IdentityChangeDataSet::key_query_with_identity_b(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let keys_query = MaloIdentityChangeDataSet::updated_key_query(); + let txn_id = TransactionId::new(); + machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + let strategy = CollectStrategy::new_identity_based(); + + let encryption_settings = + EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + + let request_result = machine + .share_room_key( + fake_room_id, + vec![IdentityChangeDataSet::user_id(), MaloIdentityChangeDataSet::user_id()] + .into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_err()); + let err = request_result.unwrap_err(); + match err { + OlmError::KeyDistributionError( + crate::error::RoomKeyDistributionError::KeyPinningViolation(affected_users), + ) => { + assert_eq!(2, affected_users.len()); + + // resolve by pinning the new identity + for user_id in affected_users { + machine + .get_identity(&user_id, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .pin_current_master_key() + .await + .unwrap() + } + + let request_result = machine + .share_room_key( + fake_room_id, + // vec![KeyDistributionTestData::dan_id()].into_iter(), + vec![IdentityChangeDataSet::user_id()].into_iter(), + encryption_settings.clone(), + ) + .await; + + assert!(request_result.is_ok()); + } + _ => panic!("Unexpected share error"), + } + } + #[async_test] async fn test_should_rotate_based_on_visibility() { let machine = set_up_test_machine().await; 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 bf5c1a3321e..070898b4581 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 @@ -667,3 +667,149 @@ impl IdentityChangeDataSet { .expect("Can't parse the `/keys/upload` response") } } + +/// A set of keys query to test identity changes, +/// For user @malo, that performed an identity change with the same device. +pub struct MaloIdentityChangeDataSet {} + +#[allow(dead_code)] +impl MaloIdentityChangeDataSet { + pub fn user_id() -> &'static UserId { + user_id!("@malo:localhost") + } + + pub fn device_id() -> &'static DeviceId { + device_id!("NZFSPBRLDO") + } + + pub fn initial_key_query() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@malo:localhost": { + "NZFSPBRLDO": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "NZFSPBRLDO", + "keys": { + "curve25519:NZFSPBRLDO": "L3jdbw42+9i+K7LPjAY+kmqG9nr2n/U0ow8hEbLCoCs", + "ed25519:NZFSPBRLDO": "VDJt3xI4SzrgQkuE3sEIauluaXawx3wWoWOynPI8Zko" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "lmtbdrJ5xBweo677Fg2qrSHsRi4R3x2WNlvSNJY6Zbg0R5lJS9syN2HZw/irL9PA644GYm4QM/t+DX0grnn+BQ", + "ed25519:+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8": "Ql1fq+SvVDx+8mjNMzSaR0hBCEkdPirbs2+BK0gwsIH1zkuMADnBoNWP7LJiKo/EO9gnpiCzyQQgI4e9pIVPDA" + } + }, + "user_id": "@malo:localhost", + "unsigned": {} + } + } + }, + "failures": {}, + "master_keys": { + "@malo:localhost": { + "keys": { + "ed25519:WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE": "WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "crJcXqFpEHRM8KNUw419XrVFaHoM8/kV4ebgpuuIiD9wfX0AhHE2iGRGpKzsrVCqne9k181/uN0sgDMpK2y4Aw", + "ed25519:WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE": "/xwFF5AC3GhkpvJ449Srh8kNQS6CXAxQMmBpQvPEHx5BHPXJ08u2ZDd1EPYY4zk4QsePk+tEYu8gDnB0bggHCA" + } + }, + "usage": [ + "master" + ], + "user_id": "@malo:localhost" + } + }, + "self_signing_keys": { + "@malo:localhost": { + "keys": { + "ed25519:+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8": "+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8" + }, + "signatures": { + "@malo:localhost": { + "ed25519:WBxliSP29guYr4ux0MW6otRe3V/wOLXXElpOcOmpdlE": "sSGQ6ny6aXtIvgKPGOYJzcmnNDSkbaJFVRe9wekOry7EaiWf2l28MkGTUBt4cPoRiMkNjuRBupNEARqHF72sAQ" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@malo:localhost" + } + }, + "user_signing_keys": {}, + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } + + pub fn updated_key_query() -> KeyQueryResponse { + let data = response_from_file(&json!({ + "device_keys": { + "@malo:localhost": { + "NZFSPBRLDO": { + "algorithms": [ + "m.olm.v1.curve25519-aes-sha2", + "m.megolm.v1.aes-sha2" + ], + "device_id": "NZFSPBRLDO", + "keys": { + "curve25519:NZFSPBRLDO": "L3jdbw42+9i+K7LPjAY+kmqG9nr2n/U0ow8hEbLCoCs", + "ed25519:NZFSPBRLDO": "VDJt3xI4SzrgQkuE3sEIauluaXawx3wWoWOynPI8Zko" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "lmtbdrJ5xBweo677Fg2qrSHsRi4R3x2WNlvSNJY6Zbg0R5lJS9syN2HZw/irL9PA644GYm4QM/t+DX0grnn+BQ", + "ed25519:+wbxNfSuDrch1jKuydQmEf4qlA4u4NgwqNXNuLVwug8": "Ql1fq+SvVDx+8mjNMzSaR0hBCEkdPirbs2+BK0gwsIH1zkuMADnBoNWP7LJiKo/EO9gnpiCzyQQgI4e9pIVPDA", + "ed25519:8my6+zgnzEP0ZqmQFyvscJh7isHlf8lxBmHg+fzdJkE": "OvqDE7C2mrHxjwNyMIEz+m/AO6I6lM5HoPYY2bvLjrJJDOF5sJOtw4JoYiCWyt90ZIWsbEqmfbazrblLD50tCg" + } + }, + "user_id": "@malo:localhost", + "unsigned": {} + } + } + }, + "failures": {}, + "master_keys": { + "@malo:localhost": { + "keys": { + "ed25519:dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU": "dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU" + }, + "signatures": { + "@malo:localhost": { + "ed25519:NZFSPBRLDO": "2Ye96l4srBSWskNQszuMpea1r97rFoUyfNqegvu/hGeP47w0OVvqYuNtZRNwqb7TMS7aPEn6l9lhWEk7v06wCg", + "ed25519:dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU": "btkxAJpJeVtc9wgBmeHUI9QDpojd6ddLxK11E3403KoTQtP6Mnr5GsVdQr1HJToG7PG4k4eEZGWxVZr1GPndAA" + } + }, + "usage": [ + "master" + ], + "user_id": "@malo:localhost" + } + }, + "self_signing_keys": { + "@malo:localhost": { + "keys": { + "ed25519:8my6+zgnzEP0ZqmQFyvscJh7isHlf8lxBmHg+fzdJkE": "8my6+zgnzEP0ZqmQFyvscJh7isHlf8lxBmHg+fzdJkE" + }, + "signatures": { + "@malo:localhost": { + "ed25519:dv2Mk7bFlRtP/0oSZpB01Ouc5frCXKfG8Bn9YrFxbxU": "KJt0y1p8v8RGLGk2wUyCMbX1irXJqup/mdRuG/cxJxs24BZhDMyIzyGrGXnWq2gx3I4fKIMtFPi/ecxf92ePAQ" + } + }, + "usage": [ + "self_signing" + ], + "user_id": "@malo:localhost" + } + }, + "user_signing_keys": {} + })); + KeyQueryResponse::try_from_http_response(data) + .expect("Can't parse the `/keys/upload` response") + } +} From 2a174b07780596e8b0c27977c13364a65a8e60b1 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 1 Aug 2024 17:57:54 -0400 Subject: [PATCH 13/18] naming and formatting changes --- crates/matrix-sdk-crypto/src/error.rs | 31 ++++++++++--------- .../group_sessions/share_strategy.rs | 28 +++++++++-------- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index 5578f67753d..71e8ea88f22 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -69,34 +69,37 @@ pub enum OlmError { have a valid Olm session with us" )] MissingSession, + #[error(transparent)] - /// The room key that should be shared was not due to an error. - KeyDistributionError(RoomKeyDistributionError), + /// The room key that was to be shared was not shared because the sharing + /// strategy could not be fulfilled. + RoomKeySharingStrategyError(RoomKeySharingStrategyError), } /// Depending on the sharing strategy for room keys, the distribution of the /// room key could fail. #[derive(Error, Debug)] -pub enum RoomKeyDistributionError { - /// When encrypting using the IdentityBased strategy. - /// Will be thrown when sharing room keys when there is a new identity for a - /// user that has not been confirmed by the user. - /// Application should display identity changes to the user as soon as - /// possible to avoid hitting this case. If it happens the app might - /// just retry automatically after the identity change has been - /// notified, or offer option to cancel. - #[error("Encryption failed because there are key pinning violation, please re-pin or verify the problematic users")] - KeyPinningViolation(Vec), +pub enum RoomKeySharingStrategyError { + /// When encrypting using the IdentityBased strategy, and there are changed + /// user identities that have not been confirmed by the user. The + /// application should display identity changes to the user as soon as + /// possible to avoid hitting this case. If this happens the app might just + /// retry automatically after the identity change has been notified, or + /// offer option to cancel. + #[error("Encryption failed because changed user identities have not been confirmed, please confirm or verify the problematic user identities")] + UnconfirmedIdentities(Vec), /// Cross-signing is required for encryption with invisible crypto - #[error("Encryption failed: Setup cross-signing on your account")] + #[error("Encryption failed because cross-signing is not setup on your account")] CrossSigningNotSetup, + /// The current device needs to be verified when encrypting using the /// IdentityBased strategy. Apps should prevent sending in the UI to /// avoid hitting this case. - #[error("Encryption failed: Verify your device to send encrypted messages")] + #[error("Encryption failed because your device is not verified")] SendingFromUnverifiedDevice, } + /// Error representing a failure during a group encryption operation. #[derive(Error, Debug)] pub enum MegolmError { 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 86ee62d9523..fa02eeaf4ee 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,7 +24,7 @@ use tracing::{debug, instrument, trace}; use super::OutboundGroupSession; use crate::{ - error::{OlmResult, RoomKeyDistributionError}, + error::{OlmResult, RoomKeySharingStrategyError}, store::Store, types::events::room_key_withheld::WithheldCode, DeviceData, EncryptionSettings, OlmError, OwnUserIdentityData, UserIdentityData, @@ -173,13 +173,13 @@ pub(crate) async fn collect_session_recipients( let own_verified_identity = match maybe_own_identity { None => { - return Err(OlmError::KeyDistributionError( - RoomKeyDistributionError::CrossSigningNotSetup, + return Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::CrossSigningNotSetup, )) } Some(identity) if !identity.is_verified() => { - return Err(OlmError::KeyDistributionError( - RoomKeyDistributionError::SendingFromUnverifiedDevice, + return Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::SendingFromUnverifiedDevice, )) } Some(identity) => identity, @@ -230,8 +230,10 @@ pub(crate) async fn collect_session_recipients( // Abort sharing and fail when there are identities mismatch. if !users_with_identity_mismatch.is_empty() { - return Err(OlmError::KeyDistributionError( - RoomKeyDistributionError::KeyPinningViolation(users_with_identity_mismatch), + return Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::UnconfirmedIdentities( + users_with_identity_mismatch, + ), )); } } @@ -644,8 +646,8 @@ mod tests { assert!(request_result.is_err()); let err = request_result.unwrap_err(); match err { - OlmError::KeyDistributionError( - crate::error::RoomKeyDistributionError::CrossSigningNotSetup, + OlmError::RoomKeySharingStrategyError( + crate::error::RoomKeySharingStrategyError::CrossSigningNotSetup, ) => { // ok } @@ -668,8 +670,8 @@ mod tests { assert!(request_result.is_err()); let err = request_result.unwrap_err(); match err { - OlmError::KeyDistributionError( - crate::error::RoomKeyDistributionError::SendingFromUnverifiedDevice, + OlmError::RoomKeySharingStrategyError( + crate::error::RoomKeySharingStrategyError::SendingFromUnverifiedDevice, ) => { // ok } @@ -746,8 +748,8 @@ mod tests { assert!(request_result.is_err()); let err = request_result.unwrap_err(); match err { - OlmError::KeyDistributionError( - crate::error::RoomKeyDistributionError::KeyPinningViolation(affected_users), + OlmError::RoomKeySharingStrategyError( + crate::error::RoomKeySharingStrategyError::UnconfirmedIdentities(affected_users), ) => { assert_eq!(2, affected_users.len()); From c2332ce74f3b2476a690ce6a1ccaab7853a19c63 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 2 Aug 2024 17:12:17 -0400 Subject: [PATCH 14/18] don't share with our own device --- .../src/session_manager/group_sessions/share_strategy.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 fa02eeaf4ee..27bfe34850a 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 @@ -133,7 +133,7 @@ pub(crate) async fn collect_session_recipients( let own_identity = store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); for user_id in users { - let user_devices = store.get_device_data_for_user(user_id).await?; + 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 { From 6b03ef3136fca2fef0c5ab66c040363477ad4bc2 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 9 Aug 2024 14:41:46 -0400 Subject: [PATCH 15/18] remove commented-out debugging line --- .../src/session_manager/group_sessions/share_strategy.rs | 1 - 1 file changed, 1 deletion(-) 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 27bfe34850a..57702751840 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 @@ -189,7 +189,6 @@ pub(crate) async fn collect_session_recipients( let mut users_with_identity_mismatch: Vec = Vec::default(); for user_id in users { let device_owner_identity = store.get_user_identity(user_id).await?; - // debug!(?device_owner_identity, "Other Checking identity"); if let Some(other_identity) = device_owner_identity.as_ref().and_then(|i| i.other()) { From afda06a69738dfa03ce3e954b274fe9cd683a865 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 9 Aug 2024 17:20:15 -0400 Subject: [PATCH 16/18] only error with a pin violation if they were previously verified --- .../group_sessions/share_strategy.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) 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 57702751840..51b72338baa 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 @@ -193,6 +193,7 @@ pub(crate) async fn collect_session_recipients( if let Some(other_identity) = device_owner_identity.as_ref().and_then(|i| i.other()) { if other_identity.has_pin_violation() + && other_identity.was_previously_verified() && own_verified_identity.is_identity_signed(other_identity).is_err() { debug!(?other_identity, "Identity Mismatch detected"); @@ -724,10 +725,30 @@ mod tests { let keys_query = IdentityChangeDataSet::key_query_with_identity_b(); let txn_id = TransactionId::new(); machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine + .get_identity(IdentityChangeDataSet::user_id(), None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .mark_as_previously_verified() + .await + .unwrap(); let keys_query = MaloIdentityChangeDataSet::updated_key_query(); let txn_id = TransactionId::new(); machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine + .get_identity(MaloIdentityChangeDataSet::user_id(), None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .mark_as_previously_verified() + .await + .unwrap(); let fake_room_id = room_id!("!roomid:localhost"); let strategy = CollectStrategy::new_identity_based(); From fd15720b271de828b66e8b7b179b2362022d9f0f Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 16 Aug 2024 23:27:43 -0400 Subject: [PATCH 17/18] apply changes from review --- .../group_sessions/share_strategy.rs | 137 ++++++++---------- 1 file changed, 60 insertions(+), 77 deletions(-) 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 51b72338baa..8f85234480c 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 @@ -267,8 +267,7 @@ fn should_rotate_due_to_left_device( if let Some(shared) = outbound.shared_with_set.read().unwrap().get(user_id) { // Devices that received this session - let shared: BTreeSet = shared.keys().cloned().collect(); - let shared: BTreeSet<&DeviceId> = shared.iter().map(|d| d.as_ref()).collect(); + let shared: BTreeSet<&DeviceId> = shared.keys().map(|d| d.as_ref()).collect(); // The set difference between // @@ -364,6 +363,8 @@ mod tests { use std::sync::Arc; + use assert_matches::assert_matches; + use assert_matches2::assert_let; use matrix_sdk_test::{ async_test, test_json::keys_query_sets::{ @@ -373,6 +374,7 @@ mod tests { use ruma::{events::room::history_visibility::HistoryVisibility, room_id, TransactionId}; use crate::{ + error::RoomKeySharingStrategyError, olm::OutboundGroupSession, session_manager::{ group_sessions::share_strategy::collect_session_recipients, CollectStrategy, @@ -625,58 +627,48 @@ mod tests { .await; let keys_query = KeyDistributionTestData::dan_keys_query_response(); - let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); let fake_room_id = room_id!("!roomid:localhost"); - let strategy = CollectStrategy::new_identity_based(); - - let encryption_settings = - EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + let encryption_settings = EncryptionSettings { + sharing_strategy: CollectStrategy::new_identity_based(), + ..Default::default() + }; let request_result = machine .share_room_key( fake_room_id, - vec![KeyDistributionTestData::dan_id()].into_iter(), + std::iter::once(KeyDistributionTestData::dan_id()), encryption_settings.clone(), ) .await; - assert!(request_result.is_err()); - let err = request_result.unwrap_err(); - match err { - OlmError::RoomKeySharingStrategyError( - crate::error::RoomKeySharingStrategyError::CrossSigningNotSetup, - ) => { - // ok - } - _ => panic!("Unexpected share error"), - } + assert_matches!( + request_result, + Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::CrossSigningNotSetup + )) + ); // Let's now have cross-signing, but the identity is not trusted let keys_query = KeyDistributionTestData::me_keys_query_response(); - let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); let request_result = machine .share_room_key( fake_room_id, - vec![KeyDistributionTestData::dan_id()].into_iter(), + std::iter::once(KeyDistributionTestData::dan_id()), encryption_settings.clone(), ) .await; - assert!(request_result.is_err()); - let err = request_result.unwrap_err(); - match err { - OlmError::RoomKeySharingStrategyError( - crate::error::RoomKeySharingStrategyError::SendingFromUnverifiedDevice, - ) => { - // ok - } - _ => panic!("Unexpected share error"), - } + assert_matches!( + request_result, + Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::SendingFromUnverifiedDevice + )) + ); // If we are verified it should then work machine @@ -695,7 +687,7 @@ mod tests { let request_result = machine .share_room_key( fake_room_id, - vec![KeyDistributionTestData::dan_id()].into_iter(), + std::iter::once(KeyDistributionTestData::dan_id()), encryption_settings.clone(), ) .await; @@ -714,17 +706,14 @@ mod tests { machine.bootstrap_cross_signing(false).await.unwrap(); let keys_query = IdentityChangeDataSet::key_query_with_identity_a(); - let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); let keys_query = MaloIdentityChangeDataSet::initial_key_query(); - let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); // Simulate pinning violation let keys_query = IdentityChangeDataSet::key_query_with_identity_b(); - let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); machine .get_identity(IdentityChangeDataSet::user_id(), None) .await @@ -737,8 +726,7 @@ mod tests { .unwrap(); let keys_query = MaloIdentityChangeDataSet::updated_key_query(); - let txn_id = TransactionId::new(); - machine.mark_request_as_sent(&txn_id, &keys_query).await.unwrap(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); machine .get_identity(MaloIdentityChangeDataSet::user_id(), None) .await @@ -751,10 +739,11 @@ mod tests { .unwrap(); let fake_room_id = room_id!("!roomid:localhost"); - let strategy = CollectStrategy::new_identity_based(); - let encryption_settings = - EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; + let encryption_settings = EncryptionSettings { + sharing_strategy: CollectStrategy::new_identity_based(), + ..Default::default() + }; let request_result = machine .share_room_key( @@ -765,41 +754,35 @@ mod tests { ) .await; - assert!(request_result.is_err()); - let err = request_result.unwrap_err(); - match err { - OlmError::RoomKeySharingStrategyError( - crate::error::RoomKeySharingStrategyError::UnconfirmedIdentities(affected_users), - ) => { - assert_eq!(2, affected_users.len()); - - // resolve by pinning the new identity - for user_id in affected_users { - machine - .get_identity(&user_id, None) - .await - .unwrap() - .unwrap() - .other() - .unwrap() - .pin_current_master_key() - .await - .unwrap() - } - - let request_result = machine - .share_room_key( - fake_room_id, - // vec![KeyDistributionTestData::dan_id()].into_iter(), - vec![IdentityChangeDataSet::user_id()].into_iter(), - encryption_settings.clone(), - ) - .await; + assert_let!( + Err(OlmError::RoomKeySharingStrategyError( + RoomKeySharingStrategyError::UnconfirmedIdentities(affected_users) + )) = request_result + ); + assert_eq!(2, affected_users.len()); - assert!(request_result.is_ok()); - } - _ => panic!("Unexpected share error"), + // resolve by pinning the new identity + for user_id in affected_users { + machine + .get_identity(&user_id, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .pin_current_master_key() + .await + .unwrap() } + + machine + .share_room_key( + fake_room_id, + std::iter::once(IdentityChangeDataSet::user_id()), + encryption_settings.clone(), + ) + .await + .unwrap(); } #[async_test] From f75934fd3a2db4c3cb7095081829f90ebf588764 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Thu, 22 Aug 2024 16:32:15 -0400 Subject: [PATCH 18/18] document tests --- .../group_sessions/share_strategy.rs | 28 ++++++++++++++----- 1 file changed, 21 insertions(+), 7 deletions(-) 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 8f85234480c..64f6dda09a3 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 @@ -620,6 +620,11 @@ mod tests { #[async_test] async fn test_share_identity_strategy_no_cross_signing() { + // Test key sharing with the identity-based strategy with different + // states of our own verification. + + // Starting off, we have not yet set up our own cross-signing, so + // sharing with the identity-based strategy should fail. let machine: OlmMachine = OlmMachine::new( KeyDistributionTestData::me_id(), KeyDistributionTestData::me_device_id(), @@ -651,7 +656,9 @@ mod tests { )) ); - // Let's now have cross-signing, but the identity is not trusted + // We now get our public cross-signing keys, but we don't trust them + // yet. In this case, sharing the keys should still fail since our own + // device is still unverified. let keys_query = KeyDistributionTestData::me_keys_query_response(); machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); @@ -670,7 +677,8 @@ mod tests { )) ); - // If we are verified it should then work + // Finally, after we trust our own cross-signing keys, key sharing + // should succeed. machine .import_cross_signing_keys(CrossSigningKeyExport { master_key: KeyDistributionTestData::MASTER_KEY_PRIVATE_EXPORT.to_owned().into(), @@ -684,19 +692,24 @@ mod tests { .await .unwrap(); - let request_result = machine + let requests = machine .share_room_key( fake_room_id, std::iter::once(KeyDistributionTestData::dan_id()), encryption_settings.clone(), ) - .await; + .await + .unwrap(); - assert!(request_result.is_ok()); + // Dan has two devices, but only one is cross-signed, so there should + // only be one key share. + assert_eq!(requests.len(), 1); } #[async_test] async fn test_share_identity_strategy_report_pinning_violation() { + // Test that identity-based key sharing gives an error when a verified + // user changes their identity. let machine: OlmMachine = OlmMachine::new( KeyDistributionTestData::me_id(), KeyDistributionTestData::me_device_id(), @@ -711,7 +724,7 @@ mod tests { let keys_query = MaloIdentityChangeDataSet::initial_key_query(); machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); - // Simulate pinning violation + // Simulate pinning violation, in which case the key sharing should fail. let keys_query = IdentityChangeDataSet::key_query_with_identity_b(); machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); machine @@ -761,7 +774,7 @@ mod tests { ); assert_eq!(2, affected_users.len()); - // resolve by pinning the new identity + // This can be resolved by pinning the new identities. for user_id in affected_users { machine .get_identity(&user_id, None) @@ -775,6 +788,7 @@ mod tests { .unwrap() } + // And now the key share should succeed. machine .share_room_key( fake_room_id,