Dima/matrix-rust-sdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

crypto: Error when sending keys to previously-verified users with identity-based strategy (#3896)

Browse Source
This commit is contained in:
Hubert Chathi
2024-09-03 13:06:32 -04:00
committed by GitHub
parent 5b14fe6f34
commit c6cda4c0dd
6 changed files with 485 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files
bindings/matrix-sdk-ffi/CHANGELOG.md
+10
View File
@@ -2,6 +2,16 @@
Breaking changes:
- `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
bindings/matrix-sdk-ffi/src/timeline/mod.rs
+14 -1
View File
@@ -929,12 +929,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<String>,
},
/// 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 {
@@ -988,6 +997,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 },
crates/matrix-sdk-crypto/CHANGELOG.md
+6
View File
@@ -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.
crates/matrix-sdk-crypto/src/error.rs
+21 -1
View File
@@ -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,24 @@ 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<OwnedUserId>),
/// 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 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,
}
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs
+286 -15
View File
@@ -120,6 +120,7 @@ pub(crate) async fn collect_session_recipients(
let users: BTreeSet<&UserId> = users.collect();
let mut devices: BTreeMap<OwnedUserId, Vec<DeviceData>> = Default::default();
let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default();
let mut verified_users_with_new_identities: Vec<OwnedUserId> = 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<OwnedUserId, Vec<OwnedDeviceId>> =
Default::default();
let mut verified_users_with_new_identities: Vec<OwnedUserId> = 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,249 @@ 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() {
// 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);
}
/// 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_verification_violation() {
let machine: OlmMachine = OlmMachine::new(
KeyDistributionTestData::me_id(),
KeyDistributionTestData::me_device_id(),
)
.await;
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();
// 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(user1, 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(user2, None)
.await
.unwrap()
.unwrap()
.other()
.unwrap()
.mark_as_previously_verified()
.await
.unwrap();
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()
};
let request_result = machine
.share_room_key(
fake_room_id,
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());
// We resolve this for user1 by withdrawing their verification.
machine
.get_identity(user1, None)
.await
.unwrap()
.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();
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(&json);
machine
.mark_request_as_sent(
&TransactionId::new(),
crate::IncomingResponse::KeysQuery(&kq_response),
)
.await
.unwrap();
assert!(machine.get_identity(user2, None).await.unwrap().unwrap().is_verified());
// And now the key share should succeed.
machine
.share_room_key(
fake_room_id,
vec![user1, user2].into_iter(),
encryption_settings.clone(),
)
.await
.unwrap();
}
#[async_test]
async fn test_should_rotate_based_on_visibility() {
let machine = set_up_test_machine().await;
testing/matrix-sdk-test/src/test_json/keys_query_sets.rs
+148
View File
@@ -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)
}
}