From 61452ad19005c769b50c4a0295b3136b8cd62fcf Mon Sep 17 00:00:00 2001 From: Andy Uhnak Date: Mon, 10 Oct 2022 16:58:06 +0100 Subject: [PATCH 1/3] Replace QR with SAS verification --- .../matrix-sdk-crypto/src/identities/user.rs | 17 ++ crates/matrix-sdk-crypto/src/store/mod.rs | 2 +- .../src/verification/cache.rs | 23 ++- .../matrix-sdk-crypto/src/verification/mod.rs | 41 ++++- .../src/verification/requests.rs | 159 +++++++++++++++++- 5 files changed, 228 insertions(+), 14 deletions(-) diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index fb360216d..c5a0c7407 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -806,6 +806,23 @@ impl ReadOnlyOwnUserIdentity { }) } + #[cfg(test)] + pub(crate) async fn from_private(identity: &crate::olm::PrivateCrossSigningIdentity) -> Self { + let master_key = identity.master_key.lock().await.as_ref().unwrap().public_key.clone(); + let self_signing_key = + identity.self_signing_key.lock().await.as_ref().unwrap().public_key.clone(); + let user_signing_key = + identity.user_signing_key.lock().await.as_ref().unwrap().public_key.clone(); + + Self { + user_id: identity.user_id().into(), + master_key, + self_signing_key, + user_signing_key, + verified: Arc::new(AtomicBool::new(true)), + } + } + /// Get the user id of this identity. pub fn user_id(&self) -> &UserId { &self.user_id diff --git a/crates/matrix-sdk-crypto/src/store/mod.rs b/crates/matrix-sdk-crypto/src/store/mod.rs index 69f4023fa..4cf48151b 100644 --- a/crates/matrix-sdk-crypto/src/store/mod.rs +++ b/crates/matrix-sdk-crypto/src/store/mod.rs @@ -325,7 +325,7 @@ impl Store { } #[cfg(test)] - /// Testing helper to allo to save only a set of devices + /// Testing helper to allow to save only a set of devices pub async fn save_devices(&self, devices: &[ReadOnlyDevice]) -> Result<()> { let changes = Changes { devices: DeviceChanges { changed: devices.to_vec(), ..Default::default() }, diff --git a/crates/matrix-sdk-crypto/src/verification/cache.rs b/crates/matrix-sdk-crypto/src/verification/cache.rs index bb674b4e4..92f7527a8 100644 --- a/crates/matrix-sdk-crypto/src/verification/cache.rs +++ b/crates/matrix-sdk-crypto/src/verification/cache.rs @@ -16,7 +16,7 @@ use std::sync::Arc; use dashmap::DashMap; use ruma::{DeviceId, OwnedTransactionId, OwnedUserId, TransactionId, UserId}; -use tracing::trace; +use tracing::{trace, warn}; use super::{event_enums::OutgoingContent, Sas, Verification}; #[cfg(feature = "qrcode")] @@ -56,6 +56,8 @@ impl VerificationCache { let old_verification = old.value(); if !old_verification.is_cancelled() { + warn!("Received a new verification whilst another one with the same user is ongoing. Cancelling both verifications"); + if let Some(r) = old_verification.cancel() { self.add_request(r.into()) } @@ -78,11 +80,7 @@ impl VerificationCache { pub fn replace_sas(&self, sas: Sas) { let verification: Verification = sas.into(); - - self.verification - .entry(verification.other_user().to_owned()) - .or_default() - .insert(verification.flow_id().to_owned(), verification.clone()); + self.replace(verification); } #[cfg(feature = "qrcode")] @@ -90,6 +88,12 @@ impl VerificationCache { self.insert(qr) } + #[cfg(feature = "qrcode")] + pub fn replace_qr(&self, qr: QrVerification) { + let verification: Verification = qr.into(); + self.replace(verification); + } + #[cfg(feature = "qrcode")] pub fn get_qr(&self, sender: &UserId, flow_id: &str) -> Option { self.get(sender, flow_id).and_then(|v| { @@ -101,6 +105,13 @@ impl VerificationCache { }) } + pub fn replace(&self, verification: Verification) { + self.verification + .entry(verification.other_user().to_owned()) + .or_default() + .insert(verification.flow_id().to_owned(), verification.clone()); + } + pub fn get(&self, sender: &UserId, flow_id: &str) -> Option { self.verification.get(sender).and_then(|m| m.get(flow_id).map(|v| v.clone())) } diff --git a/crates/matrix-sdk-crypto/src/verification/mod.rs b/crates/matrix-sdk-crypto/src/verification/mod.rs index 6eb7b01c5..afa457c37 100644 --- a/crates/matrix-sdk-crypto/src/verification/mod.rs +++ b/crates/matrix-sdk-crypto/src/verification/mod.rs @@ -826,7 +826,9 @@ mod test { use super::VerificationStore; use crate::{ - olm::PrivateCrossSigningIdentity, store::MemoryStore, ReadOnlyAccount, ReadOnlyDevice, + olm::PrivateCrossSigningIdentity, + store::{Changes, CryptoStore, IdentityChanges, MemoryStore}, + ReadOnlyAccount, ReadOnlyDevice, ReadOnlyOwnUserIdentity, ReadOnlyUserIdentity, }; pub fn alice_id() -> &'static UserId { @@ -848,28 +850,57 @@ mod test { pub(crate) async fn setup_stores() -> (VerificationStore, VerificationStore) { let alice = ReadOnlyAccount::new(alice_id(), alice_device_id()); let alice_store = MemoryStore::new(); - let alice_identity = Mutex::new(PrivateCrossSigningIdentity::empty(alice_id())); + let (alice_private_identity, _, _) = + PrivateCrossSigningIdentity::with_account(&alice).await; + let alice_private_identity = Mutex::new(alice_private_identity); let bob = ReadOnlyAccount::new(bob_id(), bob_device_id()); let bob_store = MemoryStore::new(); - let bob_identity = Mutex::new(PrivateCrossSigningIdentity::empty(bob_id())); + let (bob_private_identity, _, _) = PrivateCrossSigningIdentity::with_account(&bob).await; + let bob_private_identity = Mutex::new(bob_private_identity); + + let alice_public_identity = + ReadOnlyUserIdentity::from_private(&*alice_private_identity.lock().await).await; + let alice_readonly_identity = + ReadOnlyOwnUserIdentity::from_private(&*alice_private_identity.lock().await).await; + let bob_public_identity = + ReadOnlyUserIdentity::from_private(&*bob_private_identity.lock().await).await; + let bob_readonly_identity = + ReadOnlyOwnUserIdentity::from_private(&*bob_private_identity.lock().await).await; let alice_device = ReadOnlyDevice::from_account(&alice).await; let bob_device = ReadOnlyDevice::from_account(&bob).await; + let alice_changes = Changes { + identities: IdentityChanges { + new: vec![alice_readonly_identity.into(), bob_public_identity.into()], + changed: vec![], + }, + ..Default::default() + }; + alice_store.save_changes(alice_changes).await.unwrap(); alice_store.save_devices(vec![bob_device]).await; + + let bob_changes = Changes { + identities: IdentityChanges { + new: vec![bob_readonly_identity.into(), alice_public_identity.into()], + changed: vec![], + }, + ..Default::default() + }; + bob_store.save_changes(bob_changes).await.unwrap(); bob_store.save_devices(vec![alice_device]).await; let alice_store = VerificationStore { account: alice, inner: Arc::new(alice_store), - private_identity: alice_identity.into(), + private_identity: alice_private_identity.into(), }; let bob_store = VerificationStore { account: bob.clone(), inner: Arc::new(bob_store), - private_identity: bob_identity.into(), + private_identity: bob_private_identity.into(), }; (alice_store, bob_store) diff --git a/crates/matrix-sdk-crypto/src/verification/requests.rs b/crates/matrix-sdk-crypto/src/verification/requests.rs index fd33342a0..112a65146 100644 --- a/crates/matrix-sdk-crypto/src/verification/requests.rs +++ b/crates/matrix-sdk-crypto/src/verification/requests.rs @@ -336,7 +336,21 @@ impl VerificationRequest { if let Some(future) = fut { let qr_verification = future.await?; - self.verification_cache.insert_qr(qr_verification.clone()); + + // We may have previously started our own QR verification (e.g. two devices + // displaying QR code at the same time), so we need to replace it with the newly + // scanned code. + if self + .verification_cache + .get_qr(qr_verification.other_user_id(), qr_verification.flow_id().as_str()) + .is_some() + { + info!("Replacing existing QR verification"); + self.verification_cache.replace_qr(qr_verification.clone()); + } else { + info!("Inserting new QR verification"); + self.verification_cache.insert_qr(qr_verification.clone()); + } Ok(Some(qr_verification)) } else { @@ -634,7 +648,23 @@ impl VerificationRequest { if let Some((sas, content)) = s.clone().start_sas(self.we_started, self.inner.clone().into()).await? { - self.verification_cache.insert_sas(sas.clone()); + // We may have previously started QR verification and generated a QR code. If we + // now switch to SAS flow, the previous verification has to be replaced + if cfg!(feature = "qrcode") { + #[cfg(feature = "qrcode")] + if self + .verification_cache + .get_qr(sas.other_user_id(), sas.flow_id().as_str()) + .is_some() + { + info!("We have an ongoing QR verification, replacing with SAS"); + self.verification_cache.replace(sas.clone().into()) + } else { + self.verification_cache.insert_sas(sas.clone()); + } + } else { + self.verification_cache.insert_sas(sas.clone()); + } let request = match content { OutgoingContent::ToDevice(content) => ToDeviceRequest::with_id( @@ -1222,7 +1252,11 @@ mod tests { use std::convert::{TryFrom, TryInto}; + #[cfg(feature = "qrcode")] + use matrix_sdk_qrcode::QrVerificationData; use matrix_sdk_test::async_test; + #[cfg(feature = "qrcode")] + use ruma::events::key::verification::VerificationMethod; use ruma::{event_id, room_id}; use super::VerificationRequest; @@ -1385,4 +1419,125 @@ mod tests { assert!(alice_sas.started_from_request()); assert!(bob_sas.started_from_request()); } + + #[async_test] + #[cfg(feature = "qrcode")] + async fn can_scan_another_qr_after_creating_mine() { + let (alice_store, bob_store) = setup_stores().await; + + let flow_id = FlowId::ToDevice("TEST_FLOW_ID".into()); + + // We setup the initial verification request + let bob_request = VerificationRequest::new( + VerificationCache::new(), + bob_store, + flow_id.clone(), + alice_id(), + vec![], + Some(vec![VerificationMethod::QrCodeScanV1, VerificationMethod::QrCodeShowV1]), + ); + + let request = bob_request.request_to_device(); + let content: OutgoingContent = request.try_into().unwrap(); + let content = RequestContent::try_from(&content).unwrap(); + + let alice_request = VerificationRequest::from_request( + VerificationCache::new(), + alice_store, + bob_id(), + flow_id, + &content, + ); + + let content: OutgoingContent = alice_request + .accept_with_methods(vec![ + VerificationMethod::QrCodeScanV1, + VerificationMethod::QrCodeShowV1, + ]) + .unwrap() + .try_into() + .unwrap(); + let content = ReadyContent::try_from(&content).unwrap(); + bob_request.receive_ready(alice_id(), &content); + + assert!(bob_request.is_ready()); + assert!(alice_request.is_ready()); + + // Each side can start its own QR verification flow by generating QR code + let alice_verification = alice_request.generate_qr_code().await.unwrap(); + let bob_verification = bob_request.generate_qr_code().await.unwrap(); + + assert!(alice_verification.is_some()); + assert!(bob_verification.is_some()); + + // Now only Alice scans Bob's code + let bob_qr_code = bob_verification.unwrap().to_bytes().unwrap(); + let bob_qr_code = QrVerificationData::from_bytes(bob_qr_code).unwrap(); + let alice_verification = alice_request.scan_qr_code(bob_qr_code).await.unwrap().unwrap(); + + // Finally we assert that the verification has been reciprocated rather than + // cancelled due to a duplicate verification flow + assert!(!alice_verification.is_cancelled()); + assert!(alice_verification.reciprocated()); + } + + #[async_test] + #[cfg(feature = "qrcode")] + async fn can_start_sas_after_generating_qr_code() { + let (alice_store, bob_store) = setup_stores().await; + + let flow_id = FlowId::ToDevice("TEST_FLOW_ID".into()); + + // We setup the initial verification request + let bob_request = VerificationRequest::new( + VerificationCache::new(), + bob_store, + flow_id.clone(), + alice_id(), + vec![], + Some(vec![ + VerificationMethod::QrCodeScanV1, + VerificationMethod::QrCodeShowV1, + VerificationMethod::SasV1, + ]), + ); + + let request = bob_request.request_to_device(); + let content: OutgoingContent = request.try_into().unwrap(); + let content = RequestContent::try_from(&content).unwrap(); + + let alice_request = VerificationRequest::from_request( + VerificationCache::new(), + alice_store, + bob_id(), + flow_id, + &content, + ); + + let content: OutgoingContent = alice_request + .accept_with_methods(vec![ + VerificationMethod::QrCodeScanV1, + VerificationMethod::QrCodeShowV1, + ]) + .unwrap() + .try_into() + .unwrap(); + let content = ReadyContent::try_from(&content).unwrap(); + bob_request.receive_ready(alice_id(), &content); + + assert!(bob_request.is_ready()); + assert!(alice_request.is_ready()); + + // Each side can start its own QR verification flow by generating QR code + let alice_verification = alice_request.generate_qr_code().await.unwrap(); + let bob_verification = bob_request.generate_qr_code().await.unwrap(); + + assert!(alice_verification.is_some()); + assert!(bob_verification.is_some()); + + // Alice can now start SAS verification flow instead of QR without cancelling + // the request + let (sas, _) = alice_request.start_sas().await.unwrap().unwrap(); + assert!(!sas.is_cancelled()); + } } From 8bbdd28a8b6dea1d62ccbd500418c0a763e5e564 Mon Sep 17 00:00:00 2001 From: Andy Uhnak Date: Tue, 18 Oct 2022 11:05:39 +0100 Subject: [PATCH 2/3] Use cfg-if and debug logs --- Cargo.lock | 1 + crates/matrix-sdk-crypto/Cargo.toml | 1 + .../matrix-sdk-crypto/src/identities/user.rs | 2 +- .../src/verification/cache.rs | 6 ++- .../src/verification/requests.rs | 37 ++++++++++++------- 5 files changed, 32 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1bb895bd9..e7cdc2137 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2473,6 +2473,7 @@ dependencies = [ "base64", "bs58", "byteorder", + "cfg-if", "ctr", "dashmap", "event-listener", diff --git a/crates/matrix-sdk-crypto/Cargo.toml b/crates/matrix-sdk-crypto/Cargo.toml index 1bcd2c814..5e10db58e 100644 --- a/crates/matrix-sdk-crypto/Cargo.toml +++ b/crates/matrix-sdk-crypto/Cargo.toml @@ -50,6 +50,7 @@ thiserror = "1.0.30" tracing = "0.1.34" vodozemac = { workspace = true } zeroize = { workspace = true, features = ["zeroize_derive"] } +cfg-if = "1.0" [target.'cfg(not(target_arch = "wasm32"))'.dependencies] tokio = { version = "1.18", default-features = false, features = ["time"] } diff --git a/crates/matrix-sdk-crypto/src/identities/user.rs b/crates/matrix-sdk-crypto/src/identities/user.rs index c5a0c7407..62ba60f5e 100644 --- a/crates/matrix-sdk-crypto/src/identities/user.rs +++ b/crates/matrix-sdk-crypto/src/identities/user.rs @@ -819,7 +819,7 @@ impl ReadOnlyOwnUserIdentity { master_key, self_signing_key, user_signing_key, - verified: Arc::new(AtomicBool::new(true)), + verified: Arc::new(AtomicBool::new(false)), } } diff --git a/crates/matrix-sdk-crypto/src/verification/cache.rs b/crates/matrix-sdk-crypto/src/verification/cache.rs index 92f7527a8..bb08a4776 100644 --- a/crates/matrix-sdk-crypto/src/verification/cache.rs +++ b/crates/matrix-sdk-crypto/src/verification/cache.rs @@ -56,7 +56,11 @@ impl VerificationCache { let old_verification = old.value(); if !old_verification.is_cancelled() { - warn!("Received a new verification whilst another one with the same user is ongoing. Cancelling both verifications"); + warn!( + user_id = verification.other_user().as_str(), + "Received a new verification whilst another one with \ + the same user is ongoing. Cancelling both verifications" + ); if let Some(r) = old_verification.cancel() { self.add_request(r.into()) diff --git a/crates/matrix-sdk-crypto/src/verification/requests.rs b/crates/matrix-sdk-crypto/src/verification/requests.rs index 112a65146..4f678694a 100644 --- a/crates/matrix-sdk-crypto/src/verification/requests.rs +++ b/crates/matrix-sdk-crypto/src/verification/requests.rs @@ -36,6 +36,8 @@ use ruma::{ DeviceId, MilliSecondsSinceUnixEpoch, OwnedDeviceId, OwnedUserId, RoomId, TransactionId, UserId, }; +#[cfg(feature = "qrcode")] +use tracing::debug; use tracing::{info, trace, warn}; #[cfg(feature = "qrcode")] @@ -345,10 +347,18 @@ impl VerificationRequest { .get_qr(qr_verification.other_user_id(), qr_verification.flow_id().as_str()) .is_some() { - info!("Replacing existing QR verification"); + debug!( + user_id = %self.other_user(), + flow_id = self.flow_id().as_str(), + "Replacing existing QR verification" + ); self.verification_cache.replace_qr(qr_verification.clone()); } else { - info!("Inserting new QR verification"); + debug!( + user_id = %self.other_user(), + flow_id = self.flow_id().as_str(), + "Inserting new QR verification" + ); self.verification_cache.insert_qr(qr_verification.clone()); } @@ -650,20 +660,21 @@ impl VerificationRequest { { // We may have previously started QR verification and generated a QR code. If we // now switch to SAS flow, the previous verification has to be replaced - if cfg!(feature = "qrcode") { - #[cfg(feature = "qrcode")] - if self - .verification_cache - .get_qr(sas.other_user_id(), sas.flow_id().as_str()) - .is_some() - { - info!("We have an ongoing QR verification, replacing with SAS"); - self.verification_cache.replace(sas.clone().into()) + cfg_if::cfg_if! { + if #[cfg(feature = "qrcode")] { + if self.verification_cache.get_qr(sas.other_user_id(), sas.flow_id().as_str()).is_some() { + debug!( + user_id = %self.other_user(), + flow_id = self.flow_id().as_str(), + "We have an ongoing QR verification, replacing with SAS" + ); + self.verification_cache.replace(sas.clone().into()) + } else { + self.verification_cache.insert_sas(sas.clone()); + } } else { self.verification_cache.insert_sas(sas.clone()); } - } else { - self.verification_cache.insert_sas(sas.clone()); } let request = match content { From 1a466eb6678c1d2cc22133bf4b5241c22ab8e9df Mon Sep 17 00:00:00 2001 From: Andy Uhnak Date: Tue, 18 Oct 2022 11:15:10 +0100 Subject: [PATCH 3/3] Add flow_id to logs --- crates/matrix-sdk-crypto/src/verification/cache.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/matrix-sdk-crypto/src/verification/cache.rs b/crates/matrix-sdk-crypto/src/verification/cache.rs index bb08a4776..5b6bb9c32 100644 --- a/crates/matrix-sdk-crypto/src/verification/cache.rs +++ b/crates/matrix-sdk-crypto/src/verification/cache.rs @@ -58,6 +58,8 @@ impl VerificationCache { if !old_verification.is_cancelled() { warn!( user_id = verification.other_user().as_str(), + old_flow_id = old_verification.flow_id(), + new_flow_id = verification.flow_id(), "Received a new verification whilst another one with \ the same user is ongoing. Cancelling both verifications" );