From ead91a1e6b177d1a99b0a5a3232cd68694ff99b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 12 Jul 2021 18:12:02 +0200 Subject: [PATCH] crypto: Send cancellations if the other device picks up over to-device --- matrix_sdk_base/src/store/memory_store.rs | 3 +- matrix_sdk_crypto/src/identities/user.rs | 26 ++++- matrix_sdk_crypto/src/requests.rs | 23 ++++ .../src/verification/event_enums.rs | 27 +++++ matrix_sdk_crypto/src/verification/machine.rs | 31 +++--- .../src/verification/requests.rs | 105 ++++++++++++++++-- 6 files changed, 183 insertions(+), 32 deletions(-) diff --git a/matrix_sdk_base/src/store/memory_store.rs b/matrix_sdk_base/src/store/memory_store.rs index 6c919a4b..87d94093 100644 --- a/matrix_sdk_base/src/store/memory_store.rs +++ b/matrix_sdk_base/src/store/memory_store.rs @@ -69,7 +69,7 @@ pub struct MemoryStore { } impl MemoryStore { - #[cfg(not(feature = "sled_state_store"))] + #[allow(dead_code)] pub fn new() -> Self { Self { sync_token: Arc::new(RwLock::new(None)), @@ -558,7 +558,6 @@ impl StateStore for MemoryStore { } #[cfg(test)] -#[cfg(not(feature = "sled_state_store"))] mod test { use matrix_sdk_test::async_test; use ruma::{ diff --git a/matrix_sdk_crypto/src/identities/user.rs b/matrix_sdk_crypto/src/identities/user.rs index c124d4e4..13ca15d6 100644 --- a/matrix_sdk_crypto/src/identities/user.rs +++ b/matrix_sdk_crypto/src/identities/user.rs @@ -27,7 +27,7 @@ use ruma::{ events::{ key::verification::VerificationMethod, room::message::KeyVerificationRequestEventContent, }, - DeviceKeyId, EventId, RoomId, UserId, + DeviceIdBox, DeviceKeyId, EventId, RoomId, UserId, }; use serde::{Deserialize, Serialize}; use serde_json::to_value; @@ -108,7 +108,7 @@ impl OwnUserIdentity { pub async fn request_verification( &self, ) -> Result<(VerificationRequest, OutgoingVerificationRequest), CryptoStoreError> { - self.verification_machine.request_self_verification(&self.inner, None).await + self.request_verification_helper(None).await } /// Send a verification request to our other devices while specifying our @@ -121,7 +121,27 @@ impl OwnUserIdentity { &self, methods: Vec, ) -> Result<(VerificationRequest, OutgoingVerificationRequest), CryptoStoreError> { - self.verification_machine.request_self_verification(&self.inner, Some(methods)).await + self.request_verification_helper(Some(methods)).await + } + + async fn request_verification_helper( + &self, + methods: Option>, + ) -> Result<(VerificationRequest, OutgoingVerificationRequest), CryptoStoreError> { + let devices: Vec = self + .verification_machine + .store + .get_user_devices(self.user_id()) + .await? + .into_iter() + .map(|(d, _)| d) + .filter(|d| &**d != self.verification_machine.own_device_id()) + .collect(); + + Ok(self + .verification_machine + .request_to_device_verification(self.user_id(), devices, methods) + .await) } } diff --git a/matrix_sdk_crypto/src/requests.rs b/matrix_sdk_crypto/src/requests.rs index 428edaef..79981b26 100644 --- a/matrix_sdk_crypto/src/requests.rs +++ b/matrix_sdk_crypto/src/requests.rs @@ -78,6 +78,29 @@ impl ToDeviceRequest { Self::new_with_id(recipient, recipient_device, content, Uuid::new_v4()) } + pub(crate) fn new_for_recipients( + recipient: &UserId, + recipient_devices: Vec, + content: AnyToDeviceEventContent, + txn_id: Uuid, + ) -> Self { + let mut messages = BTreeMap::new(); + let event_type = EventType::from(content.event_type()); + + if recipient_devices.is_empty() { + Self::new(recipient, DeviceIdOrAllDevices::AllDevices, content) + } else { + let device_messages = recipient_devices + .into_iter() + .map(|d| (DeviceIdOrAllDevices::DeviceId(d), Raw::from(content.clone()))) + .collect(); + + messages.insert(recipient.clone(), device_messages); + + ToDeviceRequest { event_type, txn_id, messages } + } + } + pub(crate) fn new_with_id( recipient: &UserId, recipient_device: impl Into, diff --git a/matrix_sdk_crypto/src/verification/event_enums.rs b/matrix_sdk_crypto/src/verification/event_enums.rs index e7c66725..0a01c8ad 100644 --- a/matrix_sdk_crypto/src/verification/event_enums.rs +++ b/matrix_sdk_crypto/src/verification/event_enums.rs @@ -379,6 +379,33 @@ try_from_outgoing_content!(MacContent, KeyVerificationMac); try_from_outgoing_content!(CancelContent, KeyVerificationCancel); try_from_outgoing_content!(DoneContent, KeyVerificationDone); +impl<'a> TryFrom<&'a OutgoingContent> for RequestContent<'a> { + type Error = (); + + fn try_from(value: &'a OutgoingContent) -> Result { + match value { + OutgoingContent::Room(_, c) => { + if let AnyMessageEventContent::RoomMessage(m) = c { + if let MessageType::VerificationRequest(c) = &m.msgtype { + Ok(Self::Room(c)) + } else { + Err(()) + } + } else { + Err(()) + } + } + OutgoingContent::ToDevice(c) => { + if let AnyToDeviceEventContent::KeyVerificationRequest(c) = c { + Ok(Self::ToDevice(c)) + } else { + Err(()) + } + } + } + } +} + #[derive(Debug)] pub enum StartContent<'a> { ToDevice(&'a StartToDeviceEventContent), diff --git a/matrix_sdk_crypto/src/verification/machine.rs b/matrix_sdk_crypto/src/verification/machine.rs index 3bea8480..8d0c4858 100644 --- a/matrix_sdk_crypto/src/verification/machine.rs +++ b/matrix_sdk_crypto/src/verification/machine.rs @@ -25,8 +25,7 @@ use ruma::{ ToDeviceEvent, }, serde::Raw, - to_device::DeviceIdOrAllDevices, - DeviceId, EventId, MilliSecondsSinceUnixEpoch, RoomId, UserId, + DeviceId, DeviceIdBox, EventId, MilliSecondsSinceUnixEpoch, RoomId, UserId, }; use tracing::{info, trace, warn}; @@ -41,8 +40,8 @@ use crate::{ olm::PrivateCrossSigningIdentity, requests::OutgoingRequest, store::{CryptoStore, CryptoStoreError}, - OutgoingVerificationRequest, ReadOnlyAccount, ReadOnlyDevice, ReadOnlyOwnUserIdentity, - ReadOnlyUserIdentity, RoomMessageRequest, ToDeviceRequest, + OutgoingVerificationRequest, ReadOnlyAccount, ReadOnlyDevice, ReadOnlyUserIdentity, + RoomMessageRequest, ToDeviceRequest, }; #[derive(Clone, Debug)] @@ -77,11 +76,12 @@ impl VerificationMachine { self.account.device_id() } - pub(crate) async fn request_self_verification( + pub(crate) async fn request_to_device_verification( &self, - identity: &ReadOnlyOwnUserIdentity, + user_id: &UserId, + recipient_devices: Vec, methods: Option>, - ) -> Result<(VerificationRequest, OutgoingVerificationRequest), CryptoStoreError> { + ) -> (VerificationRequest, OutgoingVerificationRequest) { let flow_id = FlowId::from(Uuid::new_v4().to_string()); let verification = VerificationRequest::new( @@ -90,22 +90,16 @@ impl VerificationMachine { self.private_identity.lock().await.clone(), self.store.clone(), flow_id, - identity.user_id(), + user_id, + recipient_devices, methods, ); - // TODO get all the device ids of the user instead of using AllDevices - // make sure to remember this so we can cancel once someone picks up - let request: OutgoingVerificationRequest = ToDeviceRequest::new( - identity.user_id(), - DeviceIdOrAllDevices::AllDevices, - AnyToDeviceEventContent::KeyVerificationRequest(verification.request_to_device()), - ) - .into(); - self.insert_request(verification.clone()); - Ok((verification, request)) + let request = verification.request_to_device(); + + (verification, request.into()) } pub async fn request_verification( @@ -124,6 +118,7 @@ impl VerificationMachine { self.store.clone(), flow_id, identity.user_id(), + vec![], methods, ); diff --git a/matrix_sdk_crypto/src/verification/requests.rs b/matrix_sdk_crypto/src/verification/requests.rs index 230407d6..450dfeb0 100644 --- a/matrix_sdk_crypto/src/verification/requests.rs +++ b/matrix_sdk_crypto/src/verification/requests.rs @@ -75,6 +75,7 @@ pub struct VerificationRequest { inner: Arc>, creation_time: Arc, we_started: bool, + recipient_devices: Arc>, } /// A handle to a request so child verification flows can cancel the request. @@ -110,6 +111,7 @@ impl VerificationRequest { store: Arc, flow_id: FlowId, other_user: &UserId, + recipient_devices: Vec, methods: Option>, ) -> Self { let inner = Mutex::new(InnerRequest::Created(RequestState::new( @@ -131,6 +133,7 @@ impl VerificationRequest { other_user_id: other_user.to_owned().into(), creation_time: Instant::now().into(), we_started: true, + recipient_devices: recipient_devices.into(), } } @@ -138,7 +141,7 @@ impl VerificationRequest { /// verification from the other side. This should be used only for /// self-verifications and it should be sent to the specific device that we /// want to verify. - pub(crate) fn request_to_device(&self) -> RequestToDeviceEventContent { + pub(crate) fn request_to_device(&self) -> ToDeviceRequest { let inner = self.inner.lock().unwrap(); let methods = if let InnerRequest::Created(c) = &*inner { @@ -147,11 +150,18 @@ impl VerificationRequest { SUPPORTED_METHODS.to_vec() }; - RequestToDeviceEventContent::new( + let content = RequestToDeviceEventContent::new( self.account.device_id().into(), self.flow_id().as_str().to_string(), methods, MilliSecondsSinceUnixEpoch::now(), + ); + + ToDeviceRequest::new_for_recipients( + self.other_user(), + self.recipient_devices.to_vec(), + AnyToDeviceEventContent::KeyVerificationRequest(content), + Uuid::new_v4(), ) } @@ -358,6 +368,7 @@ impl VerificationRequest { flow_id: flow_id.into(), we_started: false, creation_time: Instant::now().into(), + recipient_devices: vec![].into(), } } @@ -403,6 +414,10 @@ impl VerificationRequest { fn cancel_with_code(&self, cancel_code: CancelCode) -> Option { let mut inner = self.inner.lock().unwrap(); + + let send_to_everyone = self.we_started() && matches!(&*inner, InnerRequest::Created(_)); + let other_device = inner.other_device_id(); + inner.cancel(true, &cancel_code); let content = if let InnerRequest::Cancelled(c) = &*inner { @@ -413,7 +428,17 @@ impl VerificationRequest { let request = content.map(|c| match c { OutgoingContent::ToDevice(content) => { - ToDeviceRequest::new(&self.other_user(), inner.other_device_id(), content).into() + if send_to_everyone { + ToDeviceRequest::new_for_recipients( + &self.other_user(), + self.recipient_devices.to_vec(), + content, + Uuid::new_v4(), + ) + .into() + } else { + ToDeviceRequest::new(&self.other_user(), other_device, content).into() + } } OutgoingContent::Room(room_id, content) => { RoomMessageRequest { room_id, txn_id: Uuid::new_v4(), content }.into() @@ -444,12 +469,65 @@ impl VerificationRequest { } } + /// Create a key verification cancellation for devices that received the + /// request but either shouldn't continue in the verification or didn't get + /// notified that the other side cancelled. + /// + /// The spec states the following[1]: + /// When Bob accepts or declines the verification on one of his devices + /// (sending either an m.key.verification.ready or m.key.verification.cancel + /// event), Alice will send an m.key.verification.cancel event to Bob’s + /// other devices with a code of m.accepted in the case where Bob accepted + /// the verification, or m.user in the case where Bob rejected the + /// verification. + /// + /// Realistically sending the cancellation to Bob's other devices is only + /// possible if Bob accepted the verification since we don't know the device + /// id of Bob's device that rejected the verification. + /// + /// Thus, we're sending the cancellation to all devices that received the + /// request in the rejection case. + /// + /// [1]: https://spec.matrix.org/unstable/client-server-api/#key-verification-framework + pub(crate) fn cancel_for_other_devices( + &self, + code: CancelCode, + filter_device: Option<&DeviceId>, + ) -> Option { + let cancelled = Cancelled::new(true, code); + let cancel_content = cancelled.as_content(self.flow_id()); + + if let OutgoingContent::ToDevice(c) = cancel_content { + let recipients: Vec = self + .recipient_devices + .to_vec() + .into_iter() + .filter(|d| if let Some(device) = filter_device { &**d != device } else { true }) + .collect(); + + Some(ToDeviceRequest::new_for_recipients( + self.other_user(), + recipients, + c, + Uuid::new_v4(), + )) + } else { + None + } + } + pub(crate) fn receive_ready(&self, sender: &UserId, content: &ReadyContent) { let mut inner = self.inner.lock().unwrap(); match &*inner { InnerRequest::Created(s) => { *inner = InnerRequest::Ready(s.clone().into_ready(sender, content)); + + if let Some(request) = + self.cancel_for_other_devices(CancelCode::Accepted, Some(content.from_device())) + { + self.verification_cache.add_verification_request(request.into()); + } } InnerRequest::Requested(s) => { if sender == self.own_user_id() && content.from_device() != self.account.device_id() @@ -500,6 +578,12 @@ impl VerificationRequest { ); let mut inner = self.inner.lock().unwrap(); inner.cancel(false, content.cancel_code()); + + if let Some(request) = + self.cancel_for_other_devices(content.cancel_code().to_owned(), None) + { + self.verification_cache.add_verification_request(request.into()); + } } } @@ -597,9 +681,7 @@ impl InnerRequest { InnerRequest::Created(s) => s.clone().into_canceled(cancelled_by_us, cancel_code), InnerRequest::Requested(s) => s.clone().into_canceled(cancelled_by_us, cancel_code), InnerRequest::Ready(s) => s.clone().into_canceled(cancelled_by_us, cancel_code), - InnerRequest::Passive(s) => s.clone().into_canceled(cancelled_by_us, cancel_code), - InnerRequest::Done(_) => return, - InnerRequest::Cancelled(_) => return, + InnerRequest::Passive(_) | InnerRequest::Done(_) | InnerRequest::Cancelled(_) => return, }); } @@ -1133,7 +1215,7 @@ mod test { store::{Changes, CryptoStore, MemoryStore}, verification::{ cache::VerificationCache, - event_enums::{OutgoingContent, ReadyContent, StartContent}, + event_enums::{OutgoingContent, ReadyContent, RequestContent, StartContent}, FlowId, }, ReadOnlyDevice, @@ -1180,6 +1262,7 @@ mod test { bob_store.into(), flow_id.clone(), &alice_id(), + vec![], None, ); @@ -1237,6 +1320,7 @@ mod test { bob_store.into(), flow_id.clone(), &alice_id(), + vec![], None, ); @@ -1301,10 +1385,13 @@ mod test { bob_store.into(), flow_id, &alice_id(), + vec![], None, ); - let content = bob_request.request_to_device(); + let request = bob_request.request_to_device(); + let content: OutgoingContent = request.try_into().unwrap(); + let content = RequestContent::try_from(&content).unwrap(); let flow_id = bob_request.flow_id().to_owned(); let alice_request = VerificationRequest::from_request( @@ -1314,7 +1401,7 @@ mod test { alice_store.into(), &bob_id(), flow_id, - &(&content).into(), + &content, ); let content: OutgoingContent = alice_request.accept().unwrap().try_into().unwrap();