From e5a1a100e8e4f625adbc3812e84754f6a1e3d09b Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Mon, 26 Aug 2024 11:52:55 -0400 Subject: [PATCH 1/6] Error when sending keys to previously-verified with identity-based strategy --- crates/matrix-sdk-crypto/src/error.rs | 13 +- .../group_sessions/share_strategy.rs | 239 ++++++++++++++++-- .../src/test_json/keys_query_sets.rs | 148 +++++++++++ 3 files changed, 384 insertions(+), 16 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index f11051d95db..48704801976 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -391,7 +391,7 @@ pub enum SessionRecipientCollectionError { /// /// Happens only with [`CollectStrategy::DeviceBasedStrategy`] when /// [`error_on_verified_user_problem`](`CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem`) - /// is true. + /// is true, or with [`CollectStrategy::IdentityBasedStrategy`]. /// /// In order to resolve this, the user can: /// @@ -407,4 +407,15 @@ pub enum SessionRecipientCollectionError { /// The caller can then retry the encryption operation. #[error("one or more users that were verified have changed their identity")] VerifiedUserChangedIdentity(Vec), + + /// Cross-signing is required for encryption when using + /// [`CollectStrategy::IdentityBased`]. + #[error("Encryption failed because cross-signing is not setup on your account")] + CrossSigningNotSetup, + + /// The current device needs to be verified when encrypting using + /// [`CollectStrategy::IdentityBased`]. Apps should prevent sending in the + /// UI to avoid hitting this case. + #[error("Encryption failed because your device is not verified")] + SendingFromUnverifiedDevice, } 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 5c426d0d425..bab3af7a24a 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 @@ -120,6 +120,7 @@ pub(crate) async fn collect_session_recipients( let users: BTreeSet<&UserId> = users.collect(); let mut devices: BTreeMap> = Default::default(); let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default(); + let mut verified_users_with_new_identities: Vec = Default::default(); trace!(?users, ?settings, "Calculating group session recipients"); @@ -145,6 +146,8 @@ 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()); + // Get the recipient and withheld devices, based on the collection strategy. match settings.sharing_strategy { CollectStrategy::DeviceBasedStrategy { @@ -153,10 +156,6 @@ pub(crate) async fn collect_session_recipients( } => { let mut unsigned_devices_of_verified_users: BTreeMap> = Default::default(); - let mut verified_users_with_new_identities: Vec = Default::default(); - - let own_identity = - store.get_user_identity(store.user_id()).await?.and_then(|i| i.into_own()); for user_id in users { trace!("Considering recipient devices for user {}", user_id); @@ -233,24 +232,39 @@ pub(crate) async fn collect_session_recipients( ), )); } - - // Alternatively, we may have encountered previously-verified users who have - // changed their identities. We bail out for that, too. - if !verified_users_with_new_identities.is_empty() { - return Err(OlmError::SessionRecipientCollectionError( - SessionRecipientCollectionError::VerifiedUserChangedIdentity( - verified_users_with_new_identities, - ), - )); - } } CollectStrategy::IdentityBasedStrategy => { + // We require our own cross-signing to be properly set-up for the + // identity-based strategy, so return an error if it isn't. + match &own_identity { + None => { + return Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::CrossSigningNotSetup, + )) + } + Some(identity) if !identity.is_verified() => { + return Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::SendingFromUnverifiedDevice, + )) + } + Some(_) => (), + } + for user_id in users { trace!("Considering recipient devices for user {}", user_id); let user_devices = store.get_device_data_for_user_filtered(user_id).await?; let device_owner_identity = store.get_user_identity(user_id).await?; + if has_identity_verification_violation( + own_identity.as_ref(), + device_owner_identity.as_ref(), + ) { + verified_users_with_new_identities.push(user_id.to_owned()); + // No point considering the individual devices of this user. + continue; + } + let recipient_devices = split_recipients_withhelds_for_user_based_on_identity( user_devices, &device_owner_identity, @@ -277,6 +291,16 @@ pub(crate) async fn collect_session_recipients( } } + // We may have encountered previously-verified users who have changed their + // identities. If so, we bail out with an error. + if !verified_users_with_new_identities.is_empty() { + return Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::VerifiedUserChangedIdentity( + verified_users_with_new_identities, + ), + )); + } + if should_rotate { debug!( should_rotate, @@ -491,10 +515,14 @@ fn is_user_verified( mod tests { use std::{collections::BTreeMap, iter, sync::Arc}; + use assert_matches::assert_matches; use assert_matches2::assert_let; use matrix_sdk_test::{ async_test, test_json, - test_json::keys_query_sets::{KeyDistributionTestData, PreviouslyVerifiedTestData}, + test_json::keys_query_sets::{ + IdentityChangeDataSet, KeyDistributionTestData, MaloIdentityChangeDataSet, + PreviouslyVerifiedTestData, + }, }; use ruma::{ device_id, events::room::history_visibility::HistoryVisibility, room_id, TransactionId, @@ -1122,6 +1150,187 @@ mod tests { assert_eq!(code, &WithheldCode::Unauthorised); } + #[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(), + ) + .await; + + let keys_query = KeyDistributionTestData::dan_keys_query_response(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + let fake_room_id = room_id!("!roomid:localhost"); + + let encryption_settings = EncryptionSettings { + sharing_strategy: CollectStrategy::new_identity_based(), + ..Default::default() + }; + + let request_result = machine + .share_room_key( + fake_room_id, + iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await; + + assert_matches!( + request_result, + Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::CrossSigningNotSetup + )) + ); + + // 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(); + + let request_result = machine + .share_room_key( + fake_room_id, + iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await; + + assert_matches!( + request_result, + Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::SendingFromUnverifiedDevice + )) + ); + + // 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(), + 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 requests = machine + .share_room_key( + fake_room_id, + iter::once(KeyDistributionTestData::dan_id()), + encryption_settings.clone(), + ) + .await + .unwrap(); + + // 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(), + ) + .await; + + machine.bootstrap_cross_signing(false).await.unwrap(); + + let keys_query = IdentityChangeDataSet::key_query_with_identity_a(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + let keys_query = MaloIdentityChangeDataSet::initial_key_query(); + machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); + + // 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 + .get_identity(IdentityChangeDataSet::user_id(), None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .mark_as_previously_verified() + .await + .unwrap(); + + let keys_query = MaloIdentityChangeDataSet::updated_key_query(); + machine.mark_request_as_sent(&TransactionId::new(), &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 encryption_settings = EncryptionSettings { + sharing_strategy: CollectStrategy::new_identity_based(), + ..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_let!( + Err(OlmError::SessionRecipientCollectionError( + SessionRecipientCollectionError::VerifiedUserChangedIdentity(affected_users) + )) = request_result + ); + assert_eq!(2, affected_users.len()); + + // This can be resolved by withdrawing verification. + for user_id in affected_users { + machine + .get_identity(&user_id, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .withdraw_verification() + .await + .unwrap() + } + + // And now the key share should succeed. + machine + .share_room_key( + fake_room_id, + iter::once(IdentityChangeDataSet::user_id()), + encryption_settings.clone(), + ) + .await + .unwrap(); + } + #[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 ea12ff899ef..4fa38e100a6 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 @@ -1263,3 +1263,151 @@ impl PreviouslyVerifiedTestData { ruma_response_from_json(&data) } } + +/// 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") + } + + /// @malo's keys before their identity change + pub fn initial_key_query() -> KeyQueryResponse { + let data = 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": {}, + }); + + ruma_response_from_json(&data) + } + + /// @malo's keys after their identity change + pub fn updated_key_query() -> KeyQueryResponse { + let data = 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": {} + }); + + ruma_response_from_json(&data) + } +} From 6ae0df56209a6c65fbf47bc05b528e535961d5f3 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 27 Aug 2024 21:39:52 -0400 Subject: [PATCH 2/6] fix CI issues --- bindings/matrix-sdk-ffi/src/timeline/mod.rs | 15 ++++++++++++++- crates/matrix-sdk-crypto/src/error.rs | 8 ++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/timeline/mod.rs b/bindings/matrix-sdk-ffi/src/timeline/mod.rs index 08f15ba076f..de95ef220aa 100644 --- a/bindings/matrix-sdk-ffi/src/timeline/mod.rs +++ b/bindings/matrix-sdk-ffi/src/timeline/mod.rs @@ -918,12 +918,21 @@ pub enum EventSendState { /// /// Happens only when the room key recipient strategy (as set by /// [`ClientBuilder::room_key_recipient_strategy`]) has - /// [`error_on_verified_user_problem`](CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem) set. + /// [`error_on_verified_user_problem`](CollectStrategy::DeviceBasedStrategy::error_on_verified_user_problem) + /// set, or when using [`CollectStrategy::IdentityBasedStrategy`]. VerifiedUserChangedIdentity { /// The users that were previously verified, but are no longer users: Vec, }, + /// The user does not have cross-signing set up, but + /// [`CollectStrategy::IdentityBasedStrategy`] was used. + CrossSigningNotSetup, + + /// The current device is not verified, but + /// [`CollectStrategy::IdentityBasedStrategy`] was used. + SendingFromUnverifiedDevice, + /// The local event has been sent to the server, but unsuccessfully: The /// sending has failed. SendingFailed { @@ -977,6 +986,10 @@ fn event_send_state_from_sending_failed(error: &Error, is_recoverable: bool) -> VerifiedUserChangedIdentity(bad_users) => EventSendState::VerifiedUserChangedIdentity { users: bad_users.iter().map(|user_id| user_id.to_string()).collect(), }, + + CrossSigningNotSetup => EventSendState::CrossSigningNotSetup, + + SendingFromUnverifiedDevice => EventSendState::SendingFromUnverifiedDevice, }, _ => EventSendState::SendingFailed { error: error.to_string(), is_recoverable }, diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index 48704801976..ea361fcaa43 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -409,13 +409,13 @@ pub enum SessionRecipientCollectionError { VerifiedUserChangedIdentity(Vec), /// Cross-signing is required for encryption when using - /// [`CollectStrategy::IdentityBased`]. - #[error("Encryption failed because cross-signing is not setup on your account")] + /// [`CollectStrategy::IdentityBasedStrategy`]. + #[error("Encryption failed because cross-signing is not set up on your account")] CrossSigningNotSetup, /// The current device needs to be verified when encrypting using - /// [`CollectStrategy::IdentityBased`]. Apps should prevent sending in the - /// UI to avoid hitting this case. + /// [`CollectStrategy::IdentityBasedStrategy`]. Apps should prevent sending + /// in the UI to avoid hitting this case. #[error("Encryption failed because your device is not verified")] SendingFromUnverifiedDevice, } From 4be7bf521fc2af5dcc7f371b71cbbceb7e85e6b8 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 27 Aug 2024 21:51:42 -0400 Subject: [PATCH 3/6] update changelog --- bindings/matrix-sdk-ffi/CHANGELOG.md | 12 ++++++++---- crates/matrix-sdk-crypto/CHANGELOG.md | 6 ++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/bindings/matrix-sdk-ffi/CHANGELOG.md b/bindings/matrix-sdk-ffi/CHANGELOG.md index 996921bdf17..619f742511b 100644 --- a/bindings/matrix-sdk-ffi/CHANGELOG.md +++ b/bindings/matrix-sdk-ffi/CHANGELOG.md @@ -2,10 +2,14 @@ Breaking changes: -- `EventSendState` now has two additional variants: `VerifiedUserHasUnsignedDevice` and - `VerifiedUserChangedIdentity`. These reflect problems with verified users in the room - and as such can only be returned when the room key recipient strategy has - `error_on_verified_user_problem` set. +- `EventSendState` now has four additional variants: `VerifiedUserHasUnsignedDevice`, + `VerifiedUserChangedIdentity`, `CrossSigningNotSetup`, and + `SendingFromUnverifiedDevice`. The first two reflect problems with verified users in + the room and as such can only be returned when the room key recipient strategy has + `error_on_verified_user_problem` set, or when using the identity-based strategy. The + last two indicate that your own device is not properly cross-signed, which is a + requirement when using the identity-based strategy, and can only be returned when + using the identity-based strategy. - The `AuthenticationService` has been removed: - Instead of calling `configure_homeserver`, build your own client with the `serverNameOrHomeserverUrl` builder diff --git a/crates/matrix-sdk-crypto/CHANGELOG.md b/crates/matrix-sdk-crypto/CHANGELOG.md index a463ee92a2e..96804cef64f 100644 --- a/crates/matrix-sdk-crypto/CHANGELOG.md +++ b/crates/matrix-sdk-crypto/CHANGELOG.md @@ -56,6 +56,11 @@ Breaking changes: `OlmMachine::share_room_key` to fail with an error if any verified users on the recipient list have unsigned devices, or are no lonver verified. + When `CallectStrategy::IdentityBasedStrategy` is used, + `OlmMachine::share_room_key` will fail with an error if any verified users on + the recipient list are no longer verified, or if our own device is not + properly cross-signed. + Also remove `CollectStrategy::new_device_based`: callers should construct a `CollectStrategy::DeviceBasedStrategy` directly. @@ -63,6 +68,7 @@ Breaking changes: a list of booleans. ([#3810](https://github.com/matrix-org/matrix-rust-sdk/pull/3810)) ([#3816](https://github.com/matrix-org/matrix-rust-sdk/pull/3816)) + ([#3896](https://github.com/matrix-org/matrix-rust-sdk/pull/3896)) - Remove the method `OlmMachine::clear_crypto_cache()`, crypto stores are not supposed to have any caches anymore. From 6d51d2b043d9a01f224adb655fa51909ca891632 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 30 Aug 2024 20:11:42 -0400 Subject: [PATCH 4/6] make changes from review --- bindings/matrix-sdk-ffi/CHANGELOG.md | 22 ++-- crates/matrix-sdk-crypto/src/error.rs | 19 ++- .../group_sessions/share_strategy.rs | 122 ++++++++++++++---- 3 files changed, 125 insertions(+), 38 deletions(-) diff --git a/bindings/matrix-sdk-ffi/CHANGELOG.md b/bindings/matrix-sdk-ffi/CHANGELOG.md index 619f742511b..44eaaa65eff 100644 --- a/bindings/matrix-sdk-ffi/CHANGELOG.md +++ b/bindings/matrix-sdk-ffi/CHANGELOG.md @@ -2,14 +2,20 @@ Breaking changes: -- `EventSendState` now has four additional variants: `VerifiedUserHasUnsignedDevice`, - `VerifiedUserChangedIdentity`, `CrossSigningNotSetup`, and - `SendingFromUnverifiedDevice`. The first two reflect problems with verified users in - the room and as such can only be returned when the room key recipient strategy has - `error_on_verified_user_problem` set, or when using the identity-based strategy. The - last two indicate that your own device is not properly cross-signed, which is a - requirement when using the identity-based strategy, and can only be returned when - using the identity-based strategy. +- `EventSendState` now has two additional variants: `CrossSigningNotSetup` and + `SendingFromUnverifiedDevice`. These indicate that your own device is not + properly cross-signed, which is a requirement when using the identity-based + strategy, and can only be returned when using the identity-based strategy. + + In addition, the `VerifiedUserHasUnsignedDevice` and + `VerifiedUserChangedIdentity` variants can be returned when using the + identity-based strategy, in addition to when using the device-based strategy + with `error_on_verified_user_problem` is set. + +- `EventSendState` now has two additional variants: `VerifiedUserHasUnsignedDevice` and + `VerifiedUserChangedIdentity`. These reflect problems with verified users in the room + and as such can only be returned when the room key recipient strategy has + `error_on_verified_user_problem` set. - The `AuthenticationService` has been removed: - Instead of calling `configure_homeserver`, build your own client with the `serverNameOrHomeserverUrl` builder diff --git a/crates/matrix-sdk-crypto/src/error.rs b/crates/matrix-sdk-crypto/src/error.rs index ea361fcaa43..0fcead5a609 100644 --- a/crates/matrix-sdk-crypto/src/error.rs +++ b/crates/matrix-sdk-crypto/src/error.rs @@ -408,14 +408,23 @@ pub enum SessionRecipientCollectionError { #[error("one or more users that were verified have changed their identity")] VerifiedUserChangedIdentity(Vec), - /// Cross-signing is required for encryption when using - /// [`CollectStrategy::IdentityBasedStrategy`]. + /// Cross-signing has not been configured on our own identity. + /// + /// Happens only with [`CollectStrategy::IdentityBasedStrategy`]. + /// (Cross-signing is required for encryption when using + /// `IdentityBasedStrategy`.) Apps should detect this condition and prevent + /// sending in the UI rather than waiting for this error to be returned when + /// encrypting. #[error("Encryption failed because cross-signing is not set up on your account")] CrossSigningNotSetup, - /// The current device needs to be verified when encrypting using - /// [`CollectStrategy::IdentityBasedStrategy`]. Apps should prevent sending - /// in the UI to avoid hitting this case. + /// The current device has not been cross-signed by our own identity. + /// + /// Happens only with [`CollectStrategy::IdentityBasedStrategy`]. + /// (Cross-signing is required for encryption when using + /// `IdentityBasedStrategy`.) Apps should detect this condition and prevent + /// sending in the UI rather than waiting for this error to be returned when + /// encrypting. #[error("Encryption failed because your device is not verified")] SendingFromUnverifiedDevice, } 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 bab3af7a24a..83f15867f91 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 @@ -234,7 +234,7 @@ pub(crate) async fn collect_session_recipients( } } CollectStrategy::IdentityBasedStrategy => { - // We require our own cross-signing to be properly set-up for the + // We require our own cross-signing to be properly set up for the // identity-based strategy, so return an error if it isn't. match &own_identity { None => { @@ -1150,11 +1150,10 @@ mod tests { assert_eq!(code, &WithheldCode::Unauthorised); } + /// Test key sharing with the identity-based strategy with different + /// states of our own verification. #[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( @@ -1238,10 +1237,11 @@ mod tests { assert_eq!(requests.len(), 1); } + /// Test that identity-based key sharing gives an error when a verified + /// user changes their identity, and that the key can be shared when the + /// identity change is resolved. #[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. + async fn test_share_identity_strategy_report_verification_violation() { let machine: OlmMachine = OlmMachine::new( KeyDistributionTestData::me_id(), KeyDistributionTestData::me_device_id(), @@ -1250,17 +1250,24 @@ mod tests { machine.bootstrap_cross_signing(false).await.unwrap(); + // We will try sending a key to two different users. + let user1 = IdentityChangeDataSet::user_id(); + let user2 = MaloIdentityChangeDataSet::user_id(); + + // We first get both users' initial device and identity keys. let keys_query = IdentityChangeDataSet::key_query_with_identity_a(); machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); let keys_query = MaloIdentityChangeDataSet::initial_key_query(); machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); - // Simulate pinning violation, in which case the key sharing should fail. + // And then we get both user' changed identity keys. We simulate a + // verification violation by marking both users as having been + // previously verified, 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 - .get_identity(IdentityChangeDataSet::user_id(), None) + .get_identity(user1, None) .await .unwrap() .unwrap() @@ -1273,7 +1280,7 @@ mod tests { let keys_query = MaloIdentityChangeDataSet::updated_key_query(); machine.mark_request_as_sent(&TransactionId::new(), &keys_query).await.unwrap(); machine - .get_identity(MaloIdentityChangeDataSet::user_id(), None) + .get_identity(user2, None) .await .unwrap() .unwrap() @@ -1285,6 +1292,7 @@ mod tests { let fake_room_id = room_id!("!roomid:localhost"); + // We share the key using the identity-based strategy. let encryption_settings = EncryptionSettings { sharing_strategy: CollectStrategy::new_identity_based(), ..Default::default() @@ -1293,38 +1301,102 @@ mod tests { let request_result = machine .share_room_key( fake_room_id, - vec![IdentityChangeDataSet::user_id(), MaloIdentityChangeDataSet::user_id()] - .into_iter(), + vec![user1, user2].into_iter(), encryption_settings.clone(), ) .await; + // The key share should fail with an error indicating that recipients + // were previously verified. assert_let!( Err(OlmError::SessionRecipientCollectionError( SessionRecipientCollectionError::VerifiedUserChangedIdentity(affected_users) )) = request_result ); + // Both our recipients should be in `affected_users`. assert_eq!(2, affected_users.len()); - // This can be resolved by withdrawing verification. - for user_id in affected_users { - machine - .get_identity(&user_id, None) - .await - .unwrap() - .unwrap() - .other() - .unwrap() - .withdraw_verification() - .await - .unwrap() + // We resolve this for user1 by withdrawing their verification. + machine + .get_identity(user1, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .withdraw_verification() + .await + .unwrap(); + + // We resolve this for user2 by re-verifying. + let verification_request = machine + .get_identity(user2, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .verify() + .await + .unwrap(); + let raw_extracted = + verification_request.signed_keys.get(user2).unwrap().iter().next().unwrap().1.get(); + dbg!(raw_extracted); + let signed_key: crate::types::CrossSigningKey = + serde_json::from_str(raw_extracted).unwrap(); + let new_signatures = signed_key.signatures.get(KeyDistributionTestData::me_id()).unwrap(); + let mut master_key = machine + .get_identity(user2, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .master_key + .as_ref() + .clone(); + + for (key_id, signature) in new_signatures.iter() { + master_key.as_mut().signatures.add_signature( + KeyDistributionTestData::me_id().to_owned(), + key_id.to_owned(), + signature.as_ref().unwrap().ed25519().unwrap(), + ); + } + let json = serde_json::json!({ + "device_keys": {}, + "failures": {}, + "master_keys": { + user2: master_key, + }, + "user_signing_keys": {}, + "self_signing_keys": MaloIdentityChangeDataSet::updated_key_query().self_signing_keys, } + ); + + let kq_response = matrix_sdk_test::ruma_response_from_json(dbg!(&json)); + machine + .mark_request_as_sent( + &TransactionId::new(), + crate::IncomingResponse::KeysQuery(&kq_response), + ) + .await + .unwrap(); + + assert!(machine + .get_identity(user2, None) + .await + .unwrap() + .unwrap() + .other() + .unwrap() + .is_verified()); // And now the key share should succeed. machine .share_room_key( fake_room_id, - iter::once(IdentityChangeDataSet::user_id()), + vec![user1, user2].into_iter(), encryption_settings.clone(), ) .await From 47c3db142120cfb1bd8065dd49ac6c5ee7f9bc12 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Fri, 30 Aug 2024 20:17:17 -0400 Subject: [PATCH 5/6] remove dbg statements --- .../src/session_manager/group_sessions/share_strategy.rs | 3 +-- 1 file changed, 1 insertion(+), 2 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 83f15867f91..8b874a96072 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 @@ -1341,7 +1341,6 @@ mod tests { .unwrap(); let raw_extracted = verification_request.signed_keys.get(user2).unwrap().iter().next().unwrap().1.get(); - dbg!(raw_extracted); let signed_key: crate::types::CrossSigningKey = serde_json::from_str(raw_extracted).unwrap(); let new_signatures = signed_key.signatures.get(KeyDistributionTestData::me_id()).unwrap(); @@ -1374,7 +1373,7 @@ mod tests { } ); - let kq_response = matrix_sdk_test::ruma_response_from_json(dbg!(&json)); + let kq_response = matrix_sdk_test::ruma_response_from_json(&json); machine .mark_request_as_sent( &TransactionId::new(), From 7324c3f9ce8abf369dca3dc36988e57cb6eb4141 Mon Sep 17 00:00:00 2001 From: Hubert Chathi Date: Tue, 3 Sep 2024 11:11:45 -0400 Subject: [PATCH 6/6] remove unnecessary specifiers --- .../session_manager/group_sessions/share_strategy.rs | 11 +---------- 1 file changed, 1 insertion(+), 10 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 8b874a96072..310b040e3ce 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 @@ -1322,8 +1322,6 @@ mod tests { .await .unwrap() .unwrap() - .other() - .unwrap() .withdraw_verification() .await .unwrap(); @@ -1382,14 +1380,7 @@ mod tests { .await .unwrap(); - assert!(machine - .get_identity(user2, None) - .await - .unwrap() - .unwrap() - .other() - .unwrap() - .is_verified()); + assert!(machine.get_identity(user2, None).await.unwrap().unwrap().is_verified()); // And now the key share should succeed. machine