From 069ef3a661c822d51bd5ff99d7971380e99c34d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Thu, 27 May 2021 16:11:54 +0200 Subject: [PATCH] crypto: Move the SAS starting logic into the verification request struct --- matrix_sdk_crypto/src/verification/machine.rs | 61 +++---- .../src/verification/requests.rs | 150 ++++++++++++------ matrix_sdk_crypto/src/verification/sas/mod.rs | 8 +- 3 files changed, 123 insertions(+), 96 deletions(-) diff --git a/matrix_sdk_crypto/src/verification/machine.rs b/matrix_sdk_crypto/src/verification/machine.rs index 68e4870c..b871faeb 100644 --- a/matrix_sdk_crypto/src/verification/machine.rs +++ b/matrix_sdk_crypto/src/verification/machine.rs @@ -62,6 +62,13 @@ impl VerificationCache { self.room_sas_verifications.get(event_id).map(|s| s.clone()) } + pub fn insert_sas(&self, sas: Sas) { + match sas.flow_id() { + super::FlowId::ToDevice(t) => self.sas_verification.insert(t.to_owned(), sas), + super::FlowId::InRoom(_, e) => self.room_sas_verifications.insert(e.to_owned(), sas), + }; + } + pub fn garbage_collect(&self) -> Vec { self.sas_verification.retain(|_, s| !(s.is_done() || s.is_cancelled())); self.room_sas_verifications.retain(|_, s| !(s.is_done() || s.is_cancelled())); @@ -297,46 +304,8 @@ impl VerificationMachine { } } AnySyncMessageEvent::KeyVerificationStart(e) => { - info!( - "Received a new verification start event from {} {}", - e.sender, e.content.from_device - ); - - if let Some((_, request)) = - self.requests.remove(e.content.relation.event_id.as_str()) - { - if let Some(d) = - self.store.get_device(&e.sender, &e.content.from_device).await? - { - match request.into_started_sas( - &e.content, - d, - self.store.get_user_identity(&e.sender).await?, - ) { - Ok(s) => { - info!( - "Started a new SAS verification, \ - automatically accepting because we accepted from a request" - ); - - // TODO remove this unwrap - let accept_request = s.accept().unwrap(); - - self.verifications - .room_sas_verifications - .insert(e.content.relation.event_id.clone(), s); - - self.verifications.add_request(accept_request.into()); - } - Err(c) => { - warn!( - "Can't start key verification with {} {}, canceling: {:?}", - e.sender, e.content.from_device, c - ); - self.queue_up_content(&e.sender, &e.content.from_device, c) - } - } - } + if let Some(request) = self.requests.get(e.content.relation.event_id.as_str()) { + request.receive_start(&e.sender, &e.content).await? } } AnySyncMessageEvent::KeyVerificationKey(e) => { @@ -428,14 +397,20 @@ impl VerificationMachine { e.content.from_device ); - if let Some(d) = self.store.get_device(&e.sender, &e.content.from_device).await? { + if let Some(verification) = self.get_request(&e.content.transaction_id) { + verification.receive_start(&e.sender, &e.content).await?; + } else if let Some(d) = + self.store.get_device(&e.sender, &e.content.from_device).await? + { + // TODO remove this soon, this has been deprecated by + // MSC3122 https://github.com/matrix-org/matrix-doc/pull/3122 let private_identity = self.private_identity.lock().await.clone(); match Sas::from_start_event( + e.content.clone(), + self.store.clone(), self.account.clone(), private_identity, d, - self.store.clone(), - e.content.clone(), self.store.get_user_identity(&e.sender).await?, ) { Ok(s) => { diff --git a/matrix_sdk_crypto/src/verification/requests.rs b/matrix_sdk_crypto/src/verification/requests.rs index 5be97bc7..645455f8 100644 --- a/matrix_sdk_crypto/src/verification/requests.rs +++ b/matrix_sdk_crypto/src/verification/requests.rs @@ -35,6 +35,7 @@ use matrix_sdk_common::{ uuid::Uuid, MilliSecondsSinceUnixEpoch, }; +use tracing::{info, warn}; use super::{ sas::{content_to_request, OutgoingContent, StartContent as OwnedStartContent}, @@ -43,8 +44,8 @@ use super::{ use crate::{ olm::{PrivateCrossSigningIdentity, ReadOnlyAccount}, store::CryptoStore, - OutgoingVerificationRequest, ReadOnlyDevice, RoomMessageRequest, Sas, ToDeviceRequest, - UserIdentities, + CryptoStoreError, OutgoingVerificationRequest, ReadOnlyDevice, RoomMessageRequest, Sas, + ToDeviceRequest, UserIdentities, }; const SUPPORTED_METHODS: &[VerificationMethod] = &[VerificationMethod::MSasV1]; @@ -158,7 +159,7 @@ impl<'a> StartContent<'a> { } } - pub fn methods(&self) -> &StartMethod { + pub fn method(&self) -> &StartMethod { match self { StartContent::ToDevice(c) => &c.method, StartContent::Room(c) => &c.method, @@ -227,6 +228,7 @@ impl VerificationRequest { let inner = Mutex::new(InnerRequest::Created(RequestState::new( account.clone(), private_cross_signing_identity, + cache.clone(), store, other_user, &flow_id, @@ -334,10 +336,11 @@ impl VerificationRequest { content: RequestContent, ) -> Self { Self { - verification_cache: cache, + verification_cache: cache.clone(), inner: Arc::new(Mutex::new(InnerRequest::Requested(RequestState::from_request_event( account.clone(), private_cross_signing_identity, + cache, store, sender, &flow_id, @@ -379,32 +382,26 @@ impl VerificationRequest { Ok(()) } + pub(crate) async fn receive_start<'a>( + &self, + sender: &UserId, + content: impl Into>, + ) -> Result<(), CryptoStoreError> { + match &*self.inner.lock().unwrap() { + InnerRequest::Created(_) => {} + InnerRequest::Requested(_) => {} + InnerRequest::Ready(s) => s.receive_start(sender, content).await?, + InnerRequest::Passive(_) => {} + } + + Ok(()) + } + /// Is the verification request ready to start a verification flow. pub fn is_ready(&self) -> bool { matches!(&*self.inner.lock().unwrap(), InnerRequest::Ready(_)) } - pub(crate) fn into_started_sas<'a>( - self, - content: impl Into>, - device: ReadOnlyDevice, - user_identity: Option, - ) -> Result { - match &*self.inner.lock().unwrap() { - InnerRequest::Ready(s) => s.clone().into_started_sas( - content, - s.store.clone(), - s.account.clone(), - s.private_cross_signing_identity.clone(), - device, - user_identity, - ), - // TODO cancel here since we got a missmatched message or do - // nothing? - _ => todo!(), - } - } - pub(crate) fn start( &self, device: ReadOnlyDevice, @@ -471,24 +468,14 @@ impl InnerRequest { } } - fn into_started_sas<'a>( - self, + fn to_started_sas<'a>( + &self, content: impl Into>, - store: Arc>, - account: ReadOnlyAccount, - private_identity: PrivateCrossSigningIdentity, other_device: ReadOnlyDevice, other_identity: Option, ) -> Result, OutgoingContent> { if let InnerRequest::Ready(s) = self { - Ok(Some(s.into_started_sas( - content, - store, - account, - private_identity, - other_device, - other_identity, - )?)) + Ok(Some(s.to_started_sas(content, other_device, other_identity)?)) } else { Ok(None) } @@ -499,6 +486,7 @@ impl InnerRequest { struct RequestState { account: ReadOnlyAccount, private_cross_signing_identity: PrivateCrossSigningIdentity, + verification_cache: VerificationCache, store: Arc>, flow_id: Arc, @@ -513,6 +501,7 @@ impl RequestState { fn new( account: ReadOnlyAccount, private_identity: PrivateCrossSigningIdentity, + cache: VerificationCache, store: Arc>, other_user_id: &UserId, flow_id: &FlowId, @@ -522,6 +511,7 @@ impl RequestState { other_user_id: other_user_id.to_owned(), private_cross_signing_identity: private_identity, state: Created { methods: SUPPORTED_METHODS.to_vec(), flow_id: flow_id.to_owned() }, + verification_cache: cache, store, flow_id: flow_id.to_owned().into(), } @@ -532,6 +522,7 @@ impl RequestState { RequestState { account: self.account, flow_id: self.flow_id, + verification_cache: self.verification_cache, private_cross_signing_identity: self.private_cross_signing_identity, store: self.store, other_user_id: self.other_user_id, @@ -571,6 +562,7 @@ impl RequestState { fn from_request_event( account: ReadOnlyAccount, private_identity: PrivateCrossSigningIdentity, + cache: VerificationCache, store: Arc>, sender: &UserId, flow_id: &FlowId, @@ -581,6 +573,7 @@ impl RequestState { account, private_cross_signing_identity: private_identity, store, + verification_cache: cache, flow_id: flow_id.to_owned().into(), other_user_id: sender.clone(), state: Requested { @@ -595,6 +588,7 @@ impl RequestState { let state = RequestState { account: self.account.clone(), store: self.store, + verification_cache: self.verification_cache, private_cross_signing_identity: self.private_cross_signing_identity, flow_id: self.flow_id, other_user_id: self.other_user_id, @@ -643,12 +637,9 @@ struct Ready { } impl RequestState { - fn into_started_sas<'a>( - self, + fn to_started_sas<'a>( + &self, content: impl Into>, - store: Arc>, - account: ReadOnlyAccount, - private_identity: PrivateCrossSigningIdentity, other_device: ReadOnlyDevice, other_identity: Option, ) -> Result { @@ -665,15 +656,70 @@ impl RequestState { }; Sas::from_start_event( - account, - private_identity, - other_device, - store, content, + self.store.clone(), + self.account.clone(), + self.private_cross_signing_identity.clone(), + other_device, other_identity, ) } + async fn receive_start<'a>( + &self, + sender: &UserId, + content: impl Into>, + ) -> Result<(), CryptoStoreError> { + let content = content.into(); + + info!( + sender = sender.as_str(), + device = content.from_device().as_str(), + "Received a new verification start event", + ); + + let device = if let Some(d) = self.store.get_device(&sender, content.from_device()).await? { + d + } else { + warn!( + sender = sender.as_str(), + device = content.from_device().as_str(), + "Received a key verification start event from an unknown device", + ); + + return Ok(()); + }; + + let identity = self.store.get_user_identity(&sender).await?; + + match content.method() { + StartMethod::SasV1(_) => match self.to_started_sas(content, device.clone(), identity) { + Ok(s) => { + info!("Started a new SAS verification."); + self.verification_cache.insert_sas(s); + } + Err(c) => { + warn!( + user_id = device.user_id().as_str(), + device_id = device.device_id().as_str(), + content =? c, + "Can't start key verification, canceling.", + ); + self.verification_cache.queue_up_content( + device.user_id(), + device.device_id(), + c, + ) + } + }, + m => { + warn!(method =? m, "Received a key verificaton start event with an unknown method") + } + } + + Ok(()) + } + fn start_sas( self, store: Arc>, @@ -730,7 +776,7 @@ mod test { use super::{StartContent, VerificationRequest}; use crate::{ olm::{PrivateCrossSigningIdentity, ReadOnlyAccount}, - store::{CryptoStore, MemoryStore}, + store::{Changes, CryptoStore, MemoryStore}, verification::{requests::ReadyContent, sas::OutgoingContent, VerificationCache}, ReadOnlyDevice, }; @@ -812,6 +858,10 @@ mod test { let bob_store: Box = Box::new(MemoryStore::new()); let bob_identity = PrivateCrossSigningIdentity::empty(alice_id()); + let mut changes = Changes::default(); + changes.devices.new.push(bob_device.clone()); + alice_store.save_changes(changes).await.unwrap(); + let content = VerificationRequest::request(bob.user_id(), bob.device_id(), &alice_id()); let bob_request = VerificationRequest::new( @@ -846,7 +896,9 @@ mod test { let (bob_sas, start_content) = bob_request.start(alice_device, None).unwrap(); let content = StartContent::try_from(&start_content).unwrap(); - let alice_sas = alice_request.into_started_sas(content, bob_device, None).unwrap(); + let flow_id = content.flow_id().to_owned(); + alice_request.receive_start(bob_device.user_id(), content).await.unwrap(); + let alice_sas = alice_request.verification_cache.get_sas(&flow_id).unwrap(); assert!(!bob_sas.is_cancelled()); assert!(!alice_sas.is_cancelled()); diff --git a/matrix_sdk_crypto/src/verification/sas/mod.rs b/matrix_sdk_crypto/src/verification/sas/mod.rs index 5773ef89..7c7c90f9 100644 --- a/matrix_sdk_crypto/src/verification/sas/mod.rs +++ b/matrix_sdk_crypto/src/verification/sas/mod.rs @@ -227,11 +227,11 @@ impl Sas { /// * `event` - The m.key.verification.start event that was sent to us by /// the other side. pub(crate) fn from_start_event( + content: impl Into, + store: Arc>, account: ReadOnlyAccount, private_identity: PrivateCrossSigningIdentity, other_device: ReadOnlyDevice, - store: Arc>, - content: impl Into, other_identity: Option, ) -> Result { let inner = InnerSas::from_start_event( @@ -758,11 +758,11 @@ mod test { ); let bob = Sas::from_start_event( + content, + bob_store, bob, PrivateCrossSigningIdentity::empty(bob_id()), alice_device, - bob_store, - content, None, ) .unwrap();