char/matrix-rust-sdk
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

crypto: Only send out automatic key requests if we have a verified device

Browse source 
Sending out automatic key requests is a bit spammy for new logins,
they'll likely have many undecryptable events upon an initial sync.

It's unlikely that anyone will respond to such a key request since keys are
shared only with verified devices between devices of the same user or if
the key owner knows that the device should have received the key.

Upon initial sync it's unlikely that we have been verified and the key
owner likely did not intend to send us the key since we just created the
new device.
This commit is contained in:
Damir Jelić 2021-04-20 11:47:11 +02:00
parent 78b7dcac61
commit 4a7be13961
2 changed files with 95 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
matrix_sdk_crypto/src/identities/device.rs
View file

@ -258,6 +258,14 @@ impl UserDevices {
})
}
/// Returns true if there is at least one devices of this user that is
/// considered to be verified, false otherwise.
pub fn is_any_verified(&self) -> bool {
self.inner
.values()
.any(|d| d.trust_state(&self.own_identity, &self.device_owner_identity))
}
/// Iterator over all the device ids of the user devices.
pub fn keys(&self) -> impl Iterator<Item = &DeviceIdBox> {
self.inner.keys()

101
matrix_sdk_crypto/src/key_request.rs
View file

@ -407,7 +407,7 @@ impl KeyRequestMachine {
.await?;
if let Some(device) = device {
match self.should_share_session(&device, &session).await {
match self.should_share_key(&device, &session).await {
Err(e) => {
info!(
"Received a key request from {} {} that we won't serve: {}",
@ -509,7 +509,7 @@ impl KeyRequestMachine {
/// * `device` - The device that is requesting a session from us.
///
/// * `session` - The session that was requested to be shared.
async fn should_share_session(
async fn should_share_key(
&self,
device: &Device,
session: &InboundGroupSession,
@ -554,6 +554,38 @@ impl KeyRequestMachine {
}
}
/// Check if it's ok, or rather if it makes sense to automatically request
/// a key from our other devices.
///
/// # Arguments
///
/// * `key_info` - The info of our key request containing information about
/// the key we wish to request.
async fn should_request_key(
&self,
key_info: &RequestedKeyInfo,
) -> Result<bool, CryptoStoreError> {
let request = self.store.get_key_request_by_info(&key_info).await?;
// Don't send out duplicate requests, users can re-request them if they
// think a second request might succeed.
if request.is_none() {
let devices = self.store.get_user_devices(self.user_id()).await?;
// Devices will only respond to key requests if the devices are
// verified, if the device isn't verified by us it's unlikely that
// we're verified by them either. Don't request keys if there isn't
// at least one verified device.
if devices.is_any_verified() {
Ok(true)
} else {
Ok(false)
}
} else {
Ok(false)
}
}
/// Create a new outgoing key request for the key with the given session id.
///
/// This will queue up a new to-device request and store the key info so
@ -644,9 +676,7 @@ impl KeyRequestMachine {
session_id: session_id.to_owned(),
};
let request = self.store.get_key_request_by_info(&key_info).await?;
if request.is_none() {
if self.should_request_key(&key_info).await? {
self.request_key_helper(key_info).await?;
}
@ -828,6 +858,10 @@ mod test {
"ILMLKASTES".into()
}
fn alice2_device_id() -> DeviceIdBox {
"ILMLKASTES".into()
}
fn room_id() -> RoomId {
room_id!("!test:example.org")
}
@ -840,6 +874,10 @@ mod test {
ReadOnlyAccount::new(&bob_id(), &bob_device_id())
}
fn alice_2_account() -> ReadOnlyAccount {
ReadOnlyAccount::new(&alice_id(), &alice2_device_id())
}
fn bob_machine() -> KeyRequestMachine {
let user_id = Arc::new(bob_id());
let account = ReadOnlyAccount::new(&user_id, &alice_device_id());
@ -890,7 +928,7 @@ mod test {
}
#[async_test]
async fn create_key_request() {
async fn re_request_keys() {
let machine = get_machine().await;
let account = account();
@ -925,9 +963,15 @@ mod test {
}
#[async_test]
async fn re_request_keys() {
async fn create_key_request() {
let machine = get_machine().await;
let account = account();
let second_account = alice_2_account();
let alice_device = ReadOnlyDevice::from_account(&second_account).await;
// We need a trusted device, otherwise we won't request keys
alice_device.set_trust_state(LocalTrust::Verified);
machine.store.save_devices(&[alice_device]).await.unwrap();
let (_, session) = account
.create_group_session_pair_with_defaults(&room_id())
@ -987,6 +1031,13 @@ mod test {
let machine = get_machine().await;
let account = account();
let second_account = alice_2_account();
let alice_device = ReadOnlyDevice::from_account(&second_account).await;
// We need a trusted device, otherwise we won't request keys
alice_device.set_trust_state(LocalTrust::Verified);
machine.store.save_devices(&[alice_device]).await.unwrap();
let (_, session) = account
.create_group_session_pair_with_defaults(&room_id())
.await
@ -1118,7 +1169,7 @@ mod test {
// We don't share keys with untrusted devices.
assert_eq!(
machine
.should_share_session(&own_device, &inbound)
.should_share_key(&own_device, &inbound)
.await
.expect_err("Should not share with untrusted"),
KeyshareDecision::UntrustedDevice
@ -1126,7 +1177,7 @@ mod test {
own_device.set_trust_state(LocalTrust::Verified);
// Now we do want to share the keys.
assert!(machine
.should_share_session(&own_device, &inbound)
.should_share_key(&own_device, &inbound)
.await
.is_ok());
@ -1144,7 +1195,7 @@ mod test {
// session was provided.
assert_eq!(
machine
.should_share_session(&bob_device, &inbound)
.should_share_key(&bob_device, &inbound)
.await
.expect_err("Should not share with other."),
KeyshareDecision::MissingOutboundSession
@ -1161,7 +1212,7 @@ mod test {
// wasn't shared in the first place.
assert_eq!(
machine
.should_share_session(&bob_device, &inbound)
.should_share_key(&bob_device, &inbound)
.await
.expect_err("Should not share with other unless shared."),
KeyshareDecision::OutboundSessionNotShared
@ -1173,7 +1224,7 @@ mod test {
// wasn't shared in the first place even if the device is trusted.
assert_eq!(
machine
.should_share_session(&bob_device, &inbound)
.should_share_key(&bob_device, &inbound)
.await
.expect_err("Should not share with other unless shared."),
KeyshareDecision::OutboundSessionNotShared
@ -1182,7 +1233,7 @@ mod test {
// We now share the session, since it was shared before.
outbound.mark_shared_with(bob_device.user_id(), bob_device.device_id());
assert!(machine
.should_share_session(&bob_device, &inbound)
.should_share_key(&bob_device, &inbound)
.await
.is_ok());
@ -1195,7 +1246,7 @@ mod test {
assert_eq!(
machine
.should_share_session(&bob_device, &other_inbound)
.should_share_key(&bob_device, &other_inbound)
.await
.expect_err("Should not share with other unless shared."),
KeyshareDecision::MissingOutboundSession
@ -1213,6 +1264,17 @@ mod test {
let bob_machine = bob_machine();
let bob_account = bob_account();
let second_account = alice_2_account();
let alice_device = ReadOnlyDevice::from_account(&second_account).await;
// We need a trusted device, otherwise we won't request keys
alice_device.set_trust_state(LocalTrust::Verified);
alice_machine
.store
.save_devices(&[alice_device])
.await
.unwrap();
// Create Olm sessions for our two accounts.
let (alice_session, bob_session) = alice_account.create_session_for(&bob_account).await;
@ -1381,6 +1443,17 @@ mod test {
let bob_machine = bob_machine();
let bob_account = bob_account();
let second_account = alice_2_account();
let alice_device = ReadOnlyDevice::from_account(&second_account).await;
// We need a trusted device, otherwise we won't request keys
alice_device.set_trust_state(LocalTrust::Verified);
alice_machine
.store
.save_devices(&[alice_device])
.await
.unwrap();
// Create Olm sessions for our two accounts.
let (alice_session, bob_session) = alice_account.create_session_for(&bob_account).await;