From 67af31a2b9be8b73ecc294dd91d86ac5c53a2ce8 Mon Sep 17 00:00:00 2001 From: Valere Date: Tue, 2 Jul 2024 09:33:18 +0200 Subject: [PATCH 01/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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 ba1298c30d6499b1c076f2d6c97f968b5913f435 Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 25 Jul 2024 10:43:07 +0200 Subject: [PATCH 10/11] doc: minor format change --- 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 8de38656ea1..092ae5852d1 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -615,7 +615,7 @@ impl OtherUserIdentityData { // 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`]). + // (see `has_pin_violation()`). let pinned_master_key = self.pinned_master_key.read().unwrap().clone(); let new = Self { From 42aae6aa3889ad04a459f8589121c2e5ce4dce9b Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 1 Aug 2024 18:06:32 +0200 Subject: [PATCH 11/11] Crypto review - Rename identity_mismatch + doc updates --- .../src/identities/manager.rs | 14 ++--- .../matrix-sdk-crypto/src/identities/user.rs | 60 ++++++++++--------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/manager.rs b/crates/matrix-sdk-crypto/src/identities/manager.rs index 61445a03c1e..b6d032e3722 100644 --- a/crates/matrix-sdk-crypto/src/identities/manager.rs +++ b/crates/matrix-sdk-crypto/src/identities/manager.rs @@ -1922,8 +1922,8 @@ pub(crate) mod tests { 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) + // We should now have an identity for the user but no pin violation + // (pinned master key is the current one) assert!(!other_identity.has_pin_violation()); let first_device = manager .store @@ -1970,9 +1970,9 @@ pub(crate) mod tests { let remember_previous_identity = other_identity.clone(); // 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. + // 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 + // subsequently serve a new one which would get automatically approved. manager .receive_keys_query_response( &TransactionId::new(), @@ -1989,7 +1989,7 @@ pub(crate) mod tests { } #[async_test] - async fn test_manager_resolve_identity_mismatch() { + async fn test_manager_resolve_identity_pin_violation() { use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; let manager = manager_test_helper(user_id(), device_id()).await; @@ -2018,7 +2018,7 @@ pub(crate) mod tests { // 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 + // Resolve the violation 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 092ae5852d1..88f6ce878a6 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -297,7 +297,7 @@ impl UserIdentity { ) } - /// Pin the current identity (Master public key). + /// Pin the current identity (public part of the master signing key). pub async fn pin_current_master_key(&self) -> Result<(), CryptoStoreError> { self.inner.pin(); let to_save = UserIdentityData::Other(self.inner.clone()); @@ -309,18 +309,21 @@ impl UserIdentity { Ok(()) } - /// Returns true if the identity is not verified and did change since the - /// last time we pinned it. + /// Did the identity change after an initial observation in a way that + /// requires approval from the user? /// - /// 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 (see - /// [`UserIdentity::pin_current_master_key`]). - pub fn has_identity_mismatch(&self) -> bool { + /// A user identity needs approval if it changed after the crypto machine + /// has already observed ("pinned") a different identity for that user *and* + /// it is not an explicitly verified identity (using for example interactive + /// verification). + /// + /// Such a change is to be considered a pinning violation which the + /// application should report to the local user, and can be resolved by: + /// + /// - Verifying the new identity with [`UserIdentity::request_verification`] + /// - Or by updating the pin to the new identity with + /// [`UserIdentity::pin_current_master_key`]. + pub fn identity_needs_user_approval(&self) -> bool { // First check if the current identity is verified. if self.is_verified() { return false; @@ -411,13 +414,16 @@ impl UserIdentityData { /// 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. +/// This struct also contains the currently pinned user identity (public master +/// key) for that user. +/// +/// The first time a cryptographic user identity is seen for a given user, it +/// will be associated with that user ("pinned"). Future interactions +/// will expect this identity to stay the same, to avoid MITM attacks from the +/// homeserver. +/// +/// The user can explicitly pin the new identity to allow for legitimate +/// identity changes (for example, in case of key material or device loss). #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(try_from = "OtherUserIdentityDataSerializer", into = "OtherUserIdentityDataSerializer")] pub struct OtherUserIdentityData { @@ -429,8 +435,9 @@ pub struct OtherUserIdentityData { /// 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. +/// part of migration we just pin the currently known public master key. #[derive(Deserialize, Serialize)] struct OtherUserIdentityDataSerializer { version: Option, @@ -1233,7 +1240,7 @@ pub(crate) mod tests { } #[async_test] - async fn resolve_identity_mismatch_with_verification() { + async fn resolve_identity_pin_violation_with_verification() { use test_json::keys_query_sets::IdentityChangeDataSet as DataSet; let my_user_id = user_id!("@me:localhost"); @@ -1243,12 +1250,11 @@ pub(crate) mod tests { 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; - 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(); - // Simulate an identity hange + // Simulate an identity change 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(); @@ -1258,8 +1264,8 @@ pub(crate) mod tests { let other_identity = machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); - // There should be an identity mismatch - assert!(other_identity.has_identity_mismatch()); + // The identity should need user approval now + assert!(other_identity.identity_needs_user_approval()); // Manually verify for the purpose of this test let sig_upload = other_identity.verify().await.unwrap(); @@ -1294,10 +1300,10 @@ pub(crate) mod tests { .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 + // The identity should not need any user approval now let other_identity = machine.get_identity(other_user_id, None).await.unwrap().unwrap().other().unwrap(); - assert!(!other_identity.has_identity_mismatch()); + assert!(!other_identity.identity_needs_user_approval()); // But there is still a pin violation assert!(other_identity.inner.has_pin_violation()); }