From 327445c6a0e9ef6d041f606e28c9a954ccb977a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damir=20Jeli=C4=87?= Date: Mon, 31 May 2021 16:31:35 +0200 Subject: [PATCH] crypto: Move the logic for marking identities as verified out of the Sas struct --- matrix_sdk/src/sas.rs | 2 +- matrix_sdk_crypto/src/verification/machine.rs | 15 +- matrix_sdk_crypto/src/verification/mod.rs | 265 +++++++++++++++- .../src/verification/requests.rs | 29 +- .../src/verification/sas/event_enums.rs | 4 + matrix_sdk_crypto/src/verification/sas/mod.rs | 283 ++---------------- 6 files changed, 335 insertions(+), 263 deletions(-) diff --git a/matrix_sdk/src/sas.rs b/matrix_sdk/src/sas.rs index 62474f8f..b2eed875 100644 --- a/matrix_sdk/src/sas.rs +++ b/matrix_sdk/src/sas.rs @@ -138,7 +138,7 @@ impl Sas { } /// Get the other users device that we're veryfying. - pub fn other_device(&self) -> ReadOnlyDevice { + pub fn other_device(&self) -> &ReadOnlyDevice { self.inner.other_device() } } diff --git a/matrix_sdk_crypto/src/verification/machine.rs b/matrix_sdk_crypto/src/verification/machine.rs index b871faeb..dff6a2d3 100644 --- a/matrix_sdk_crypto/src/verification/machine.rs +++ b/matrix_sdk_crypto/src/verification/machine.rs @@ -28,7 +28,8 @@ use tracing::{info, trace, warn}; use super::{ requests::VerificationRequest, - sas::{content_to_request, OutgoingContent, Sas, VerificationResult}, + sas::{content_to_request, OutgoingContent, Sas}, + VerificationResult, }; use crate::{ olm::PrivateCrossSigningIdentity, @@ -341,8 +342,10 @@ impl VerificationMachine { ); } } - VerificationResult::Cancel(r) => { - self.verifications.add_request(r.into()); + VerificationResult::Cancel(c) => { + if let Some(r) = s.cancel_with_code(c) { + self.verifications.add_request(r.into()); + } } VerificationResult::SignatureUpload(r) => { self.verifications.add_request(r.into()); @@ -453,8 +456,10 @@ impl VerificationMachine { if s.is_done() { match s.mark_as_done().await? { VerificationResult::Ok => (), - VerificationResult::Cancel(r) => { - self.verifications.add_request(r.into()); + VerificationResult::Cancel(c) => { + if let Some(r) = s.cancel_with_code(c) { + self.verifications.add_request(r.into()); + } } VerificationResult::SignatureUpload(r) => { self.verifications.add_request(r.into()); diff --git a/matrix_sdk_crypto/src/verification/mod.rs b/matrix_sdk_crypto/src/verification/mod.rs index 9b836ccb..63875e14 100644 --- a/matrix_sdk_crypto/src/verification/mod.rs +++ b/matrix_sdk_crypto/src/verification/mod.rs @@ -16,16 +16,28 @@ mod machine; mod requests; mod sas; -pub use machine::{VerificationCache, VerificationMachine}; +use std::sync::Arc; + use matrix_sdk_common::{ + api::r0::keys::upload_signatures::Request as SignatureUploadRequest, events::key::verification::{ cancel::{CancelCode, CancelEventContent, CancelToDeviceEventContent}, Relation, }, - identifiers::{EventId, RoomId}, + identifiers::{DeviceId, EventId, RoomId, UserId}, }; + +pub use machine::{VerificationCache, VerificationMachine}; pub use requests::VerificationRequest; -pub use sas::{AcceptSettings, Sas, VerificationResult}; +pub use sas::{AcceptSettings, Sas}; +use tracing::{error, info, trace, warn}; + +use crate::{ + error::SignatureError, + olm::PrivateCrossSigningIdentity, + store::{Changes, CryptoStore, DeviceChanges}, + CryptoStoreError, LocalTrust, ReadOnlyDevice, UserIdentities, +}; use self::sas::CancelContent; @@ -116,6 +128,253 @@ impl From<(RoomId, EventId)> for FlowId { } } +/// A result of a verification flow. +#[derive(Clone, Debug)] +pub enum VerificationResult { + /// The verification succeeded, nothing needs to be done. + Ok, + /// The verification was canceled. + Cancel(CancelCode), + /// The verification is done and has signatures that need to be uploaded. + SignatureUpload(SignatureUploadRequest), +} + +#[derive(Clone, Debug)] +pub struct IdentitiesBeingVerified { + private_identity: PrivateCrossSigningIdentity, + store: Arc>, + device_being_verified: ReadOnlyDevice, + identity_being_verified: Option, +} + +#[allow(dead_code)] +impl IdentitiesBeingVerified { + fn user_id(&self) -> &UserId { + self.private_identity.user_id() + } + + fn other_user_id(&self) -> &UserId { + self.device_being_verified.user_id() + } + + fn other_device_id(&self) -> &DeviceId { + self.device_being_verified.device_id() + } + + fn other_device(&self) -> &ReadOnlyDevice { + &self.device_being_verified + } + + pub async fn mark_as_done( + &self, + verified_devices: Option<&[ReadOnlyDevice]>, + verified_identities: Option<&[UserIdentities]>, + ) -> Result { + if let Some(device) = self.mark_device_as_verified(verified_devices).await? { + let identity = self.mark_identity_as_verified(verified_identities).await?; + + // We only sign devices of our own user here. + let signature_request = if device.user_id() == self.user_id() { + match self.private_identity.sign_device(&device).await { + Ok(r) => Some(r), + Err(SignatureError::MissingSigningKey) => { + warn!( + "Can't sign the device keys for {} {}, \ + no private user signing key found", + device.user_id(), + device.device_id(), + ); + + None + } + Err(e) => { + error!( + "Error signing device keys for {} {} {:?}", + device.user_id(), + device.device_id(), + e + ); + None + } + } + } else { + None + }; + + let mut changes = Changes { + devices: DeviceChanges { changed: vec![device], ..Default::default() }, + ..Default::default() + }; + + let identity_signature_request = if let Some(i) = identity { + // We only sign other users here. + let request = if let Some(i) = i.other() { + // Signing can fail if the user signing key is missing. + match self.private_identity.sign_user(&i).await { + Ok(r) => Some(r), + Err(SignatureError::MissingSigningKey) => { + warn!( + "Can't sign the public cross signing keys for {}, \ + no private user signing key found", + i.user_id() + ); + None + } + Err(e) => { + error!( + "Error signing the public cross signing keys for {} {:?}", + i.user_id(), + e + ); + None + } + } + } else { + None + }; + + changes.identities.changed.push(i); + + request + } else { + None + }; + + // If there are two signature upload requests, merge them. Otherwise + // use the one we have or None. + // + // Realistically at most one request will be used but let's make + // this future proof. + let merged_request = if let Some(mut r) = signature_request { + if let Some(user_request) = identity_signature_request { + r.signed_keys.extend(user_request.signed_keys); + Some(r) + } else { + Some(r) + } + } else { + identity_signature_request + }; + + // TODO store the signature upload request as well. + self.store.save_changes(changes).await?; + Ok(merged_request + .map(VerificationResult::SignatureUpload) + .unwrap_or(VerificationResult::Ok)) + } else { + Ok(VerificationResult::Cancel(CancelCode::UserMismatch)) + } + } + + async fn mark_identity_as_verified( + &self, + verified_identities: Option<&[UserIdentities]>, + ) -> Result, CryptoStoreError> { + // If there wasn't an identity available during the verification flow + // return early as there's nothing to do. + if self.identity_being_verified.is_none() { + return Ok(None); + } + + // TODO signal an error, e.g. when the identity got deleted so we don't + // verify/save the device either. + let identity = self.store.get_user_identity(self.other_user_id()).await?; + + if let Some(identity) = identity { + if self + .identity_being_verified + .as_ref() + .map_or(false, |i| i.master_key() == identity.master_key()) + { + if verified_identities.map_or(false, |i| i.contains(&identity)) { + trace!("Marking user identity of {} as verified.", identity.user_id(),); + + if let UserIdentities::Own(i) = &identity { + i.mark_as_verified(); + } + + Ok(Some(identity)) + } else { + info!( + "The interactive verification process didn't verify \ + the user identity of {} {:?}", + identity.user_id(), + verified_identities, + ); + + Ok(None) + } + } else { + warn!( + "The master keys of {} have changed while an interactive \ + verification was going on, not marking the identity as verified.", + identity.user_id(), + ); + + Ok(None) + } + } else { + info!( + "The identity for {} was deleted while an interactive \ + verification was going on.", + self.other_user_id(), + ); + Ok(None) + } + } + + async fn mark_device_as_verified( + &self, + verified_devices: Option<&[ReadOnlyDevice]>, + ) -> Result, CryptoStoreError> { + let device = self.store.get_device(self.other_user_id(), self.other_device_id()).await?; + + if let Some(device) = device { + if device.keys() == self.device_being_verified.keys() { + if verified_devices.map_or(false, |v| v.contains(&device)) { + trace!( + user_id = device.user_id().as_str(), + device_id = device.device_id().as_str(), + "Marking device as verified.", + ); + + device.set_trust_state(LocalTrust::Verified); + + Ok(Some(device)) + } else { + info!( + user_id = device.user_id().as_str(), + device_id = device.device_id().as_str(), + "The interactive verification process didn't verify \ + the device", + ); + + Ok(None) + } + } else { + warn!( + user_id = device.user_id().as_str(), + device_id = device.device_id().as_str(), + "The device keys have changed while an interactive \ + verification was going on, not marking the device as verified.", + ); + Ok(None) + } + } else { + let device = &self.device_being_verified; + + info!( + user_id = device.user_id().as_str(), + device_id = device.device_id().as_str(), + "The device was deleted while an interactive verification was \ + going on.", + ); + + Ok(None) + } + } +} + #[cfg(test)] pub(crate) mod test { use matrix_sdk_common::{ diff --git a/matrix_sdk_crypto/src/verification/requests.rs b/matrix_sdk_crypto/src/verification/requests.rs index 4a01ee1a..16d5bcc0 100644 --- a/matrix_sdk_crypto/src/verification/requests.rs +++ b/matrix_sdk_crypto/src/verification/requests.rs @@ -23,6 +23,7 @@ use matrix_sdk_common::{ api::r0::to_device::DeviceIdOrAllDevices, events::{ key::verification::{ + done::{DoneEventContent, DoneToDeviceEventContent}, ready::{ReadyEventContent, ReadyToDeviceEventContent}, request::RequestToDeviceEventContent, start::{StartEventContent, StartMethod, StartToDeviceEventContent}, @@ -35,7 +36,7 @@ use matrix_sdk_common::{ uuid::Uuid, MilliSecondsSinceUnixEpoch, }; -use tracing::{info, warn}; +use tracing::{info, trace, warn}; use super::{ sas::{content_to_request, OutgoingContent, StartContent as OwnedStartContent}, @@ -202,6 +203,32 @@ impl<'a> TryFrom<&'a OutgoingContent> for StartContent<'a> { } } +pub enum DoneContent<'a> { + ToDevice(&'a DoneToDeviceEventContent), + Room(&'a DoneEventContent), +} + +impl<'a> From<&'a DoneEventContent> for DoneContent<'a> { + fn from(c: &'a DoneEventContent) -> Self { + Self::Room(c) + } +} + +impl<'a> From<&'a DoneToDeviceEventContent> for DoneContent<'a> { + fn from(c: &'a DoneToDeviceEventContent) -> Self { + Self::ToDevice(c) + } +} + +impl<'a> DoneContent<'a> { + pub fn flow_id(&self) -> &str { + match self { + Self::ToDevice(c) => &c.transaction_id, + Self::Room(c) => &c.relation.event_id.as_str(), + } + } +} + #[derive(Clone, Debug)] /// TODO pub struct VerificationRequest { diff --git a/matrix_sdk_crypto/src/verification/sas/event_enums.rs b/matrix_sdk_crypto/src/verification/sas/event_enums.rs index 93d7a7cd..e092917a 100644 --- a/matrix_sdk_crypto/src/verification/sas/event_enums.rs +++ b/matrix_sdk_crypto/src/verification/sas/event_enums.rs @@ -109,6 +109,7 @@ impl From<(RoomId, AcceptEventContent)> for AcceptContent { } } +#[derive(Clone, Debug)] pub enum KeyContent { ToDevice(KeyToDeviceEventContent), Room(RoomId, KeyEventContent), @@ -142,6 +143,7 @@ impl From<(RoomId, KeyEventContent)> for KeyContent { } } +#[derive(Clone, Debug)] pub enum MacContent { ToDevice(MacToDeviceEventContent), Room(RoomId, MacEventContent), @@ -182,6 +184,7 @@ impl From<(RoomId, MacEventContent)> for MacContent { } } +#[derive(Clone, Debug)] pub enum CancelContent { ToDevice(CancelToDeviceEventContent), Room(RoomId, CancelEventContent), @@ -199,6 +202,7 @@ impl From for CancelContent { } } +#[derive(Clone, Debug)] pub enum DoneContent { Room(RoomId, DoneEventContent), } diff --git a/matrix_sdk_crypto/src/verification/sas/mod.rs b/matrix_sdk_crypto/src/verification/sas/mod.rs index 7c7c90f9..edd37ea8 100644 --- a/matrix_sdk_crypto/src/verification/sas/mod.rs +++ b/matrix_sdk_crypto/src/verification/sas/mod.rs @@ -38,39 +38,22 @@ use matrix_sdk_common::{ identifiers::{DeviceId, EventId, RoomId, UserId}, uuid::Uuid, }; -use tracing::{error, info, trace, warn}; -use super::FlowId; +use super::{FlowId, IdentitiesBeingVerified, VerificationResult}; use crate::{ - error::SignatureError, - identities::{LocalTrust, ReadOnlyDevice, UserIdentities}, + identities::{ReadOnlyDevice, UserIdentities}, olm::PrivateCrossSigningIdentity, requests::{OutgoingVerificationRequest, RoomMessageRequest}, - store::{Changes, CryptoStore, CryptoStoreError, DeviceChanges}, + store::{CryptoStore, CryptoStoreError}, ReadOnlyAccount, ToDeviceRequest, }; -#[derive(Debug)] -/// A result of a verification flow. -#[allow(clippy::large_enum_variant)] -pub enum VerificationResult { - /// The verification succeeded, nothing needs to be done. - Ok, - /// The verification was canceled. - Cancel(OutgoingVerificationRequest), - /// The verification is done and has signatures that need to be uploaded. - SignatureUpload(SignatureUploadRequest), -} - #[derive(Clone, Debug)] /// Short authentication string object. pub struct Sas { inner: Arc>, - store: Arc>, account: ReadOnlyAccount, - private_identity: PrivateCrossSigningIdentity, - other_device: ReadOnlyDevice, - other_identity: Option, + identities_being_verified: IdentitiesBeingVerified, flow_id: Arc, } @@ -87,17 +70,17 @@ impl Sas { /// Get the user id of the other side. pub fn other_user_id(&self) -> &UserId { - self.other_device.user_id() + self.identities_being_verified.other_user_id() } /// Get the device id of the other side. pub fn other_device_id(&self) -> &DeviceId { - self.other_device.device_id() + self.identities_being_verified.other_device_id() } /// Get the device of the other user. - pub fn other_device(&self) -> ReadOnlyDevice { - self.other_device.clone() + pub fn other_device(&self) -> &ReadOnlyDevice { + self.identities_being_verified.other_device() } /// Get the unique ID that identifies this SAS verification flow. @@ -127,14 +110,18 @@ impl Sas { ) -> Sas { let flow_id = inner_sas.verification_flow_id(); + let identities = IdentitiesBeingVerified { + private_identity: private_identity.clone(), + store: store.clone(), + device_being_verified: other_device.clone(), + identity_being_verified: other_identity.clone(), + }; + Sas { inner: Arc::new(Mutex::new(inner_sas)), account, - private_identity, - store, - other_device, + identities_being_verified: identities, flow_id, - other_identity, } } @@ -241,17 +228,14 @@ impl Sas { other_identity.clone(), )?; - let flow_id = inner.verification_flow_id(); - - Ok(Sas { - inner: Arc::new(Mutex::new(inner)), + Ok(Self::start_helper( + inner, account, private_identity, other_device, - other_identity, store, - flow_id, - }) + other_identity, + )) } /// Accept the SAS verification. @@ -322,7 +306,7 @@ impl Sas { if done { match self.mark_as_done().await? { - VerificationResult::Cancel(r) => Ok((Some(r), None)), + VerificationResult::Cancel(c) => Ok((self.cancel_with_code(c), None)), VerificationResult::Ok => Ok((mac_request, None)), VerificationResult::SignatureUpload(r) => Ok((mac_request, Some(r))), } @@ -332,205 +316,9 @@ impl Sas { } pub(crate) async fn mark_as_done(&self) -> Result { - if let Some(device) = self.mark_device_as_verified().await? { - let identity = self.mark_identity_as_verified().await?; - - // We only sign devices of our own user here. - let signature_request = if device.user_id() == self.user_id() { - match self.private_identity.sign_device(&device).await { - Ok(r) => Some(r), - Err(SignatureError::MissingSigningKey) => { - warn!( - "Can't sign the device keys for {} {}, \ - no private user signing key found", - device.user_id(), - device.device_id(), - ); - - None - } - Err(e) => { - error!( - "Error signing device keys for {} {} {:?}", - device.user_id(), - device.device_id(), - e - ); - None - } - } - } else { - None - }; - - let mut changes = Changes { - devices: DeviceChanges { changed: vec![device], ..Default::default() }, - ..Default::default() - }; - - let identity_signature_request = if let Some(i) = identity { - // We only sign other users here. - let request = if let Some(i) = i.other() { - // Signing can fail if the user signing key is missing. - match self.private_identity.sign_user(&i).await { - Ok(r) => Some(r), - Err(SignatureError::MissingSigningKey) => { - warn!( - "Can't sign the public cross signing keys for {}, \ - no private user signing key found", - i.user_id() - ); - None - } - Err(e) => { - error!( - "Error signing the public cross signing keys for {} {:?}", - i.user_id(), - e - ); - None - } - } - } else { - None - }; - - changes.identities.changed.push(i); - - request - } else { - None - }; - - // If there are two signature upload requests, merge them. Otherwise - // use the one we have or None. - // - // Realistically at most one reuqest will be used but let's make - // this future proof. - let merged_request = if let Some(mut r) = signature_request { - if let Some(user_request) = identity_signature_request { - r.signed_keys.extend(user_request.signed_keys); - Some(r) - } else { - Some(r) - } - } else { - identity_signature_request - }; - - // TODO store the request as well. - self.store.save_changes(changes).await?; - Ok(merged_request - .map(VerificationResult::SignatureUpload) - .unwrap_or(VerificationResult::Ok)) - } else { - Ok(self.cancel().map(VerificationResult::Cancel).unwrap_or(VerificationResult::Ok)) - } - } - - pub(crate) async fn mark_identity_as_verified( - &self, - ) -> Result, CryptoStoreError> { - // If there wasn't an identity available during the verification flow - // return early as there's nothing to do. - if self.other_identity.is_none() { - return Ok(None); - } - - // TODO signal an error, e.g. when the identity got deleted so we don't - // verify/save the device either. - let identity = self.store.get_user_identity(self.other_user_id()).await?; - - if let Some(identity) = identity { - if self - .other_identity - .as_ref() - .map_or(false, |i| i.master_key() == identity.master_key()) - { - if self.verified_identities().map_or(false, |i| i.contains(&identity)) { - trace!("Marking user identity of {} as verified.", identity.user_id(),); - - if let UserIdentities::Own(i) = &identity { - i.mark_as_verified(); - } - - Ok(Some(identity)) - } else { - info!( - "The interactive verification process didn't contain a \ - MAC for the user identity of {} {:?}", - identity.user_id(), - self.verified_identities(), - ); - - Ok(None) - } - } else { - warn!( - "The master keys of {} have changed while an interactive \ - verification was going on, not marking the identity as verified.", - identity.user_id(), - ); - - Ok(None) - } - } else { - info!( - "The identity for {} was deleted while an interactive \ - verification was going on.", - self.other_user_id(), - ); - Ok(None) - } - } - - pub(crate) async fn mark_device_as_verified( - &self, - ) -> Result, CryptoStoreError> { - let device = self.store.get_device(self.other_user_id(), self.other_device_id()).await?; - - if let Some(device) = device { - if device.keys() == self.other_device.keys() { - if self.verified_devices().map_or(false, |v| v.contains(&device)) { - trace!( - "Marking device {} {} as verified.", - device.user_id(), - device.device_id() - ); - - device.set_trust_state(LocalTrust::Verified); - - Ok(Some(device)) - } else { - info!( - "The interactive verification process didn't contain a \ - MAC for the device {} {}", - device.user_id(), - device.device_id() - ); - - Ok(None) - } - } else { - warn!( - "The device keys of {} {} have changed while an interactive \ - verification was going on, not marking the device as verified.", - device.user_id(), - device.device_id() - ); - Ok(None) - } - } else { - let device = self.other_device(); - - info!( - "The device {} {} was deleted while an interactive \ - verification was going on.", - device.user_id(), - device.device_id() - ); - Ok(None) - } + self.identities_being_verified + .mark_as_done(self.verified_devices().as_deref(), self.verified_identities().as_deref()) + .await } /// Cancel the verification. @@ -540,11 +328,14 @@ impl Sas { /// Returns None if the `Sas` object is already in a canceled state, /// otherwise it returns a request that needs to be sent out. pub fn cancel(&self) -> Option { + self.cancel_with_code(CancelCode::User) + } + + pub(crate) fn cancel_with_code(&self, code: CancelCode) -> Option { let mut guard = self.inner.lock().unwrap(); let sas: InnerSas = (*guard).clone(); - let (sas, content) = sas.cancel(CancelCode::User); + let (sas, content) = sas.cancel(code); *guard = sas; - content.map(|c| match c { CancelContent::Room(room_id, content) => RoomMessageRequest { room_id, @@ -562,21 +353,7 @@ impl Sas { if self.is_cancelled() || self.is_done() { None } else if self.timed_out() { - let mut guard = self.inner.lock().unwrap(); - let sas: InnerSas = (*guard).clone(); - let (sas, content) = sas.cancel(CancelCode::Timeout); - *guard = sas; - content.map(|c| match c { - CancelContent::Room(room_id, content) => RoomMessageRequest { - room_id, - txn_id: Uuid::new_v4(), - content: AnyMessageEventContent::KeyVerificationCancel(content), - } - .into(), - CancelContent::ToDevice(c) => self - .content_to_request(AnyToDeviceEventContent::KeyVerificationCancel(c)) - .into(), - }) + self.cancel_with_code(CancelCode::Timeout) } else { None }