From 499791b10fb3bf0a1186df8f3b2e8b06076623fe Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 29 Nov 2023 02:57:32 +0000 Subject: [PATCH 1/7] api: make Contact.is_verified() return bool --- deltachat-ffi/src/lib.rs | 11 +++- deltachat-jsonrpc/src/api/types/contact.rs | 3 +- deltachat-repl/src/cmdline.rs | 9 +--- src/chat.rs | 8 ++- src/contact.rs | 28 ++-------- src/securejoin.rs | 61 +++++----------------- src/tests/verified_chats.rs | 18 ++----- 7 files changed, 36 insertions(+), 102 deletions(-) diff --git a/deltachat-ffi/src/lib.rs b/deltachat-ffi/src/lib.rs index 081e7198a7..a4e2516878 100644 --- a/deltachat-ffi/src/lib.rs +++ b/deltachat-ffi/src/lib.rs @@ -4113,10 +4113,17 @@ pub unsafe extern "C" fn dc_contact_is_verified(contact: *mut dc_contact_t) -> l let ffi_contact = &*contact; let ctx = &*ffi_contact.context; - block_on(ffi_contact.contact.is_verified(ctx)) + if block_on(ffi_contact.contact.is_verified(ctx)) .context("is_verified failed") .log_err(ctx) - .unwrap_or_default() as libc::c_int + .unwrap_or_default() + { + // Return value is essentially a boolean, + // but we return 2 for true for backwards compatibility. + 2 + } else { + 0 + } } #[no_mangle] diff --git a/deltachat-jsonrpc/src/api/types/contact.rs b/deltachat-jsonrpc/src/api/types/contact.rs index c5676aba7d..60acd6b198 100644 --- a/deltachat-jsonrpc/src/api/types/contact.rs +++ b/deltachat-jsonrpc/src/api/types/contact.rs @@ -1,5 +1,4 @@ use anyhow::Result; -use deltachat::contact::VerifiedStatus; use deltachat::context::Context; use serde::Serialize; use typescript_type_def::TypeDef; @@ -57,7 +56,7 @@ impl ContactObject { Some(path_buf) => path_buf.to_str().map(|s| s.to_owned()), None => None, }; - let is_verified = contact.is_verified(context).await? == VerifiedStatus::BidirectVerified; + let is_verified = contact.is_verified(context).await?; let is_profile_verified = contact.is_profile_verified(context).await?; let verifier_id = contact diff --git a/deltachat-repl/src/cmdline.rs b/deltachat-repl/src/cmdline.rs index 40fa149fad..30f4c9185f 100644 --- a/deltachat-repl/src/cmdline.rs +++ b/deltachat-repl/src/cmdline.rs @@ -284,13 +284,8 @@ async fn log_contactlist(context: &Context, contacts: &[ContactId]) -> Result<() let contact = Contact::get_by_id(context, *contact_id).await?; let name = contact.get_display_name(); let addr = contact.get_addr(); - let verified_state = contact.is_verified(context).await?; - let verified_str = if VerifiedStatus::Unverified != verified_state { - if verified_state == VerifiedStatus::BidirectVerified { - " √√" - } else { - " √" - } + let verified_str = if contact.is_verified(context).await? { + " √" } else { "" }; diff --git a/src/chat.rs b/src/chat.rs index d7163419e5..18db1f05c9 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -21,7 +21,7 @@ use crate::constants::{ Blocked, Chattype, DC_CHAT_ID_ALLDONE_HINT, DC_CHAT_ID_ARCHIVED_LINK, DC_CHAT_ID_LAST_SPECIAL, DC_CHAT_ID_TRASH, DC_RESEND_USER_AVATAR_DAYS, }; -use crate::contact::{self, Contact, ContactAddress, ContactId, Origin, VerifiedStatus}; +use crate::contact::{self, Contact, ContactAddress, ContactId, Origin}; use crate::context::Context; use crate::debug_logging::maybe_set_logging_xdc; use crate::download::DownloadState; @@ -503,7 +503,7 @@ impl ChatId { let contact_ids = get_chat_contacts(context, self).await?; for contact_id in contact_ids { let contact = Contact::get_by_id(context, contact_id).await?; - if contact.is_verified(context).await? != VerifiedStatus::BidirectVerified { + if !contact.is_verified(context).await? { bail!("{} is not verified.", contact.get_display_name()); } } @@ -3444,9 +3444,7 @@ pub(crate) async fn add_contact_to_chat_ex( } } else { // else continue and send status mail - if chat.is_protected() - && contact.is_verified(context).await? != VerifiedStatus::BidirectVerified - { + if chat.is_protected() && !contact.is_verified(context).await? { error!( context, "Only bidirectional verified contacts can be added to protected chats." diff --git a/src/contact.rs b/src/contact.rs index eba291654c..91b11fe55b 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -348,24 +348,6 @@ pub(crate) enum Modifier { Created, } -/// Verification status of the contact. -#[derive(Debug, PartialEq, Eq, Clone, Copy, FromPrimitive)] -#[repr(u8)] -pub enum VerifiedStatus { - /// Contact is not verified. - Unverified = 0, - /// SELF has verified the fingerprint of a contact. Currently unused. - Verified = 1, - /// SELF and contact have verified their fingerprints in both directions; in the UI typically checkmarks are shown. - BidirectVerified = 2, -} - -impl Default for VerifiedStatus { - fn default() -> Self { - Self::Unverified - } -} - impl Contact { /// Loads a single contact object from the database. /// @@ -1281,20 +1263,20 @@ impl Contact { /// otherwise use is_chat_protected(). /// Use [Self::get_verifier_id] to display the verifier contact /// in the info section of the contact profile. - pub async fn is_verified(&self, context: &Context) -> Result { + pub async fn is_verified(&self, context: &Context) -> Result { // We're always sort of secured-verified as we could verify the key on this device any time with the key // on this device if self.id == ContactId::SELF { - return Ok(VerifiedStatus::BidirectVerified); + return Ok(true); } if let Some(peerstate) = Peerstate::from_addr(context, &self.addr).await? { if peerstate.is_using_verified_key() { - return Ok(VerifiedStatus::BidirectVerified); + return Ok(true); } } - Ok(VerifiedStatus::Unverified) + Ok(false) } /// Returns the `ContactId` that verified the contact. @@ -1349,7 +1331,7 @@ impl Contact { Ok(chat_id.is_protected(context).await? == ProtectionStatus::Protected) } else { // 1:1 chat does not exist. - Ok(self.is_verified(context).await? == VerifiedStatus::BidirectVerified) + Ok(self.is_verified(context).await?) } } diff --git a/src/securejoin.rs b/src/securejoin.rs index 04cd0caa9d..39a14901b5 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -774,7 +774,6 @@ mod tests { use crate::chatlist::Chatlist; use crate::constants::Chattype; use crate::contact::ContactAddress; - use crate::contact::VerifiedStatus; use crate::peerstate::Peerstate; use crate::receive_imf::receive_imf; use crate::stock_str::chat_protection_enabled; @@ -893,17 +892,11 @@ mod tests { let contact_bob = Contact::get_by_id(&alice.ctx, contact_bob_id) .await .unwrap(); - assert_eq!( - contact_bob.is_verified(&alice.ctx).await.unwrap(), - VerifiedStatus::Unverified - ); + assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), false); // Step 5+6: Alice receives vc-request-with-auth, sends vc-contact-confirm alice.recv_msg(&sent).await; - assert_eq!( - contact_bob.is_verified(&alice.ctx).await.unwrap(), - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact_bob.is_verified(&alice.ctx).await.unwrap(), true); // exactly one one-to-one chat should be visible for both now // (check this before calling alice.create_chat() explicitly below) @@ -946,17 +939,11 @@ mod tests { let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id) .await .unwrap(); - assert_eq!( - contact_bob.is_verified(&bob.ctx).await.unwrap(), - VerifiedStatus::Unverified - ); + assert_eq!(contact_bob.is_verified(&bob.ctx).await.unwrap(), false); // Step 7: Bob receives vc-contact-confirm, sends vc-contact-confirm-received bob.recv_msg(&sent).await; - assert_eq!( - contact_alice.is_verified(&bob.ctx).await.unwrap(), - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact_alice.is_verified(&bob.ctx).await.unwrap(), true); // Check Bob got the verified message in his 1:1 chat. { @@ -1063,17 +1050,11 @@ mod tests { ) .await?; let contact_bob = Contact::get_by_id(&alice.ctx, contact_bob_id).await?; - assert_eq!( - contact_bob.is_verified(&alice.ctx).await?, - VerifiedStatus::Unverified - ); + assert_eq!(contact_bob.is_verified(&alice.ctx).await?, false); // Step 5+6: Alice receives vc-request-with-auth, sends vc-contact-confirm alice.recv_msg(&sent).await; - assert_eq!( - contact_bob.is_verified(&alice.ctx).await?, - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact_bob.is_verified(&alice.ctx).await?, true); let sent = alice.pop_sent_msg().await; let msg = bob.parse_msg(&sent).await; @@ -1090,17 +1071,11 @@ mod tests { .expect("Error looking up contact") .expect("Contact not found"); let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id).await?; - assert_eq!( - contact_bob.is_verified(&bob.ctx).await?, - VerifiedStatus::Unverified - ); + assert_eq!(contact_bob.is_verified(&bob.ctx).await?, false); // Step 7: Bob receives vc-contact-confirm, sends vc-contact-confirm-received bob.recv_msg(&sent).await; - assert_eq!( - contact_alice.is_verified(&bob.ctx).await?, - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact_alice.is_verified(&bob.ctx).await?, true); let sent = bob.pop_sent_msg().await; let msg = alice.parse_msg(&sent).await; @@ -1231,17 +1206,11 @@ mod tests { .await? .expect("Contact not found"); let contact_bob = Contact::get_by_id(&alice.ctx, contact_bob_id).await?; - assert_eq!( - contact_bob.is_verified(&alice.ctx).await?, - VerifiedStatus::Unverified - ); + assert_eq!(contact_bob.is_verified(&alice.ctx).await?, false); // Step 5+6: Alice receives vg-request-with-auth, sends vg-member-added alice.recv_msg(&sent).await; - assert_eq!( - contact_bob.is_verified(&alice.ctx).await?, - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact_bob.is_verified(&alice.ctx).await?, true); let sent = alice.pop_sent_msg().await; let msg = bob.parse_msg(&sent).await; @@ -1277,19 +1246,13 @@ mod tests { .expect("Error looking up contact") .expect("Contact not found"); let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id).await?; - assert_eq!( - contact_bob.is_verified(&bob.ctx).await?, - VerifiedStatus::Unverified - ); + assert_eq!(contact_bob.is_verified(&bob.ctx).await?, false); // Step 7: Bob receives vg-member-added, sends vg-member-added-received bob.recv_msg(&sent).await; { // Bob has Alice verified, message shows up in the group chat. - assert_eq!( - contact_alice.is_verified(&bob.ctx).await?, - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact_alice.is_verified(&bob.ctx).await?, true); let chat = bob.get_chat(&alice).await; assert_eq!( chat.blocked, diff --git a/src/tests/verified_chats.rs b/src/tests/verified_chats.rs index ca75a11e94..aacf79d9f0 100644 --- a/src/tests/verified_chats.rs +++ b/src/tests/verified_chats.rs @@ -5,7 +5,6 @@ use crate::chat::ProtectionStatus; use crate::chatlist::Chatlist; use crate::config::Config; use crate::constants::DC_GCL_FOR_FORWARDING; -use crate::contact::VerifiedStatus; use crate::contact::{Contact, Origin}; use crate::message::{Message, Viewtype}; use crate::mimefactory::MimeFactory; @@ -70,10 +69,7 @@ async fn check_verified_oneonone_chat(broken_by_classical_email: bool) { tcm.send_recv(&bob, &alice, "Using DC again").await; let contact = alice.add_or_lookup_contact(&bob).await; - assert_eq!( - contact.is_verified(&alice.ctx).await.unwrap(), - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact.is_verified(&alice.ctx).await.unwrap(), true); // Bob's chat is marked as verified again assert_verified(&alice, &bob, ProtectionStatus::Protected).await; @@ -121,10 +117,7 @@ async fn test_create_verified_oneonone_chat() -> Result<()> { // Alice and Fiona should now be verified because of gossip let alice_fiona_contact = alice.add_or_lookup_contact(&fiona).await; - assert_eq!( - alice_fiona_contact.is_verified(&alice).await.unwrap(), - VerifiedStatus::BidirectVerified - ); + assert!(alice_fiona_contact.is_verified(&alice).await.unwrap(),); // Alice should have a hidden protected chat with Fiona { @@ -684,7 +677,7 @@ async fn test_break_protection_then_verify_again() -> Result<()> { // Bob sent a message with a new key, so he most likely doesn't have // the old key anymore. This means that Alice's device should show // him as unverified: - VerifiedStatus::Unverified + false ); let chat = alice.get_chat(&bob_new).await; assert_eq!(chat.is_protected(), false); @@ -786,10 +779,7 @@ async fn test_create_oneonone_chat_with_former_verified_contact() -> Result<()> async fn assert_verified(this: &TestContext, other: &TestContext, protected: ProtectionStatus) { let contact = this.add_or_lookup_contact(other).await; - assert_eq!( - contact.is_verified(this).await.unwrap(), - VerifiedStatus::BidirectVerified - ); + assert_eq!(contact.is_verified(this).await.unwrap(), true); let chat = this.get_chat(other).await; let (expect_protected, expect_broken) = match protected { From c9b85f04a1ac8384974901b6e9582a4433109bf4 Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 29 Nov 2023 02:40:48 +0000 Subject: [PATCH 2/7] refactor: make `min_verified` a boolean We either need a securejoin or autocrypt key, there are no intermediate states. --- src/chat.rs | 8 ++------ src/contact.rs | 14 ++++++-------- src/e2ee.rs | 6 +++--- src/mimefactory.rs | 12 ++++++------ src/peerstate.rs | 41 +++++++++++++++++++---------------------- 5 files changed, 36 insertions(+), 45 deletions(-) diff --git a/src/chat.rs b/src/chat.rs index 18db1f05c9..4729de4a76 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -34,7 +34,7 @@ use crate::message::{self, Message, MessageState, MsgId, Viewtype}; use crate::mimefactory::MimeFactory; use crate::mimeparser::SystemMessage; use crate::param::{Param, Params}; -use crate::peerstate::{Peerstate, PeerstateVerifiedStatus}; +use crate::peerstate::Peerstate; use crate::receive_imf::ReceivedMsg; use crate::smtp::send_msg_to_smtp; use crate::sql; @@ -1202,11 +1202,7 @@ impl ChatId { let peerstate = Peerstate::from_addr(context, addr).await?; match peerstate - .filter(|peerstate| { - peerstate - .peek_key(PeerstateVerifiedStatus::Unverified) - .is_some() - }) + .filter(|peerstate| peerstate.peek_key(false).is_some()) .map(|peerstate| peerstate.prefer_encrypt) { Some(EncryptPreference::Mutual) => ret_mutual += &format!("{addr}\n"), diff --git a/src/contact.rs b/src/contact.rs index 91b11fe55b..fbd3aa44b2 100644 --- a/src/contact.rs +++ b/src/contact.rs @@ -30,7 +30,7 @@ use crate::login_param::LoginParam; use crate::message::MessageState; use crate::mimeparser::AvatarAction; use crate::param::{Param, Params}; -use crate::peerstate::{Peerstate, PeerstateVerifiedStatus}; +use crate::peerstate::Peerstate; use crate::sql::{self, params_iter}; use crate::sync::{self, Sync::*, SyncData}; use crate::tools::{ @@ -1037,11 +1037,9 @@ impl Contact { let loginparam = LoginParam::load_configured_params(context).await?; let peerstate = Peerstate::from_addr(context, &contact.addr).await?; - if let Some(peerstate) = peerstate.filter(|peerstate| { - peerstate - .peek_key(PeerstateVerifiedStatus::Unverified) - .is_some() - }) { + if let Some(peerstate) = + peerstate.filter(|peerstate| peerstate.peek_key(false).is_some()) + { let stock_message = match peerstate.prefer_encrypt { EncryptPreference::Mutual => stock_str::e2e_preferred(context).await, EncryptPreference::NoPreference => stock_str::e2e_available(context).await, @@ -1056,11 +1054,11 @@ impl Contact { .fingerprint() .to_string(); let fingerprint_other_verified = peerstate - .peek_key(PeerstateVerifiedStatus::BidirectVerified) + .peek_key(true) .map(|k| k.fingerprint().to_string()) .unwrap_or_default(); let fingerprint_other_unverified = peerstate - .peek_key(PeerstateVerifiedStatus::Unverified) + .peek_key(false) .map(|k| k.fingerprint().to_string()) .unwrap_or_default(); if loginparam.addr < peerstate.addr { diff --git a/src/e2ee.rs b/src/e2ee.rs index c328a3da04..e2e20452d2 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -7,7 +7,7 @@ use crate::aheader::{Aheader, EncryptPreference}; use crate::config::Config; use crate::context::Context; use crate::key::{load_self_public_key, load_self_secret_key, SignedPublicKey}; -use crate::peerstate::{Peerstate, PeerstateVerifiedStatus}; +use crate::peerstate::Peerstate; use crate::pgp; #[derive(Debug)] @@ -94,7 +94,7 @@ impl EncryptHelper { pub async fn encrypt( self, context: &Context, - min_verified: PeerstateVerifiedStatus, + min_verified: bool, mail_to_encrypt: lettre_email::PartBuilder, peerstates: Vec<(Option, &str)>, ) -> Result { @@ -118,7 +118,7 @@ impl EncryptHelper { // Encrypt to secondary verified keys // if we also encrypt to the introducer ("verifier") of the key. - if min_verified == PeerstateVerifiedStatus::BidirectVerified { + if min_verified { for (peerstate, _addr) in peerstates { if let Some(peerstate) = peerstate { if let (Some(key), Some(verifier)) = ( diff --git a/src/mimefactory.rs b/src/mimefactory.rs index e5e738c7bb..0aa0b90b46 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -22,7 +22,7 @@ use crate::location; use crate::message::{self, Message, MsgId, Viewtype}; use crate::mimeparser::SystemMessage; use crate::param::Param; -use crate::peerstate::{Peerstate, PeerstateVerifiedStatus}; +use crate::peerstate::Peerstate; use crate::simplify::escape_message_footer_marks; use crate::stock_str; use crate::tools::IsNoneOrEmpty; @@ -312,7 +312,7 @@ impl<'a> MimeFactory<'a> { } } - fn min_verified(&self) -> PeerstateVerifiedStatus { + fn min_verified(&self) -> bool { match &self.loaded { Loaded::Message { chat } => { if chat.is_protected() { @@ -321,15 +321,15 @@ impl<'a> MimeFactory<'a> { // In order to do this, it is necessary that they can be sent // to a key that is not yet verified. // This has to work independently of whether the chat is protected right now. - PeerstateVerifiedStatus::Unverified + false } else { - PeerstateVerifiedStatus::BidirectVerified + true } } else { - PeerstateVerifiedStatus::Unverified + false } } - Loaded::Mdn { .. } => PeerstateVerifiedStatus::Unverified, + Loaded::Mdn { .. } => false, } } diff --git a/src/peerstate.rs b/src/peerstate.rs index 3335e8b412..2dbb52d6fc 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -373,7 +373,7 @@ impl Peerstate { } /// Returns the contents of the `Autocrypt-Gossip` header for outgoing messages. - pub fn render_gossip_header(&self, min_verified: PeerstateVerifiedStatus) -> Option { + pub fn render_gossip_header(&self, min_verified: bool) -> Option { if let Some(key) = self.peek_key(min_verified) { let header = Aheader::new( self.addr.clone(), @@ -397,12 +397,11 @@ impl Peerstate { /// Converts the peerstate into the contact public key. /// /// Similar to [`Self::peek_key`], but consumes the peerstate and returns owned key. - pub fn take_key(mut self, min_verified: PeerstateVerifiedStatus) -> Option { - match min_verified { - PeerstateVerifiedStatus::BidirectVerified => self.verified_key.take(), - PeerstateVerifiedStatus::Unverified => { - self.public_key.take().or_else(|| self.gossip_key.take()) - } + pub fn take_key(mut self, min_verified: bool) -> Option { + if min_verified { + self.verified_key.take() + } else { + self.public_key.take().or_else(|| self.gossip_key.take()) } } @@ -415,25 +414,24 @@ impl Peerstate { /// Returned key is suitable for sending in `Autocrypt-Gossip` header. /// /// Returns `None` if there is no suitable public key. - pub fn peek_key(&self, min_verified: PeerstateVerifiedStatus) -> Option<&SignedPublicKey> { - match min_verified { - PeerstateVerifiedStatus::BidirectVerified => self.verified_key.as_ref(), - PeerstateVerifiedStatus::Unverified => { - self.public_key.as_ref().or(self.gossip_key.as_ref()) - } + pub fn peek_key(&self, min_verified: bool) -> Option<&SignedPublicKey> { + if min_verified { + self.verified_key.as_ref() + } else { + self.public_key.as_ref().or(self.gossip_key.as_ref()) } } /// Returns a reference to the contact's public key fingerprint. /// /// Similar to [`Self::peek_key`], but returns the fingerprint instead of the key. - fn peek_key_fingerprint(&self, min_verified: PeerstateVerifiedStatus) -> Option<&Fingerprint> { - match min_verified { - PeerstateVerifiedStatus::BidirectVerified => self.verified_key_fingerprint.as_ref(), - PeerstateVerifiedStatus::Unverified => self - .public_key_fingerprint + fn peek_key_fingerprint(&self, min_verified: bool) -> Option<&Fingerprint> { + if min_verified { + self.verified_key_fingerprint.as_ref() + } else { + self.public_key_fingerprint .as_ref() - .or(self.gossip_key_fingerprint.as_ref()), + .or(self.gossip_key_fingerprint.as_ref()) } } @@ -443,10 +441,9 @@ impl Peerstate { /// Note that verified groups always use the verified key no matter if the /// opportunistic key matches or not. pub(crate) fn is_using_verified_key(&self) -> bool { - let verified = self.peek_key_fingerprint(PeerstateVerifiedStatus::BidirectVerified); + let verified = self.peek_key_fingerprint(true); - verified.is_some() - && verified == self.peek_key_fingerprint(PeerstateVerifiedStatus::Unverified) + verified.is_some() && verified == self.peek_key_fingerprint(false) } /// Set this peerstate to verified From 3d918067a1c5248d81bae8d5f693aade76f3ddcd Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 29 Nov 2023 01:16:25 +0000 Subject: [PATCH 3/7] refactor: remove {vc-contact-confirm,vg-member-added}-received steps --- src/securejoin.rs | 60 +++++++++----------------------------- src/securejoin/bobstate.rs | 20 ------------- 2 files changed, 14 insertions(+), 66 deletions(-) diff --git a/src/securejoin.rs b/src/securejoin.rs index 39a14901b5..dfdb74c064 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -515,12 +515,8 @@ pub(crate) async fn handle_securejoin_handshake( } "vg-member-added-received" | "vc-contact-confirm-received" => { - /*========================================================== - ==== Alice - the inviter side ==== - ==== Step 8 in "Out-of-band verified groups" protocol ==== - ==========================================================*/ - - Ok(HandshakeMessage::Done) // "Done" deletes the message + // Deprecated steps, delete them immediately. + Ok(HandshakeMessage::Done) } _ => { warn!(context, "invalid step: {}", step); @@ -541,10 +537,10 @@ pub(crate) async fn handle_securejoin_handshake( /// before sending vg-member-added/vc-contact-confirm - so, if we observe vg-member-added/vc-contact-confirm, /// we can mark the peer as verified as well. /// -/// - if we see the self-sent-message vg-member-added-received +/// - if we see the self-sent-message vg-request-with-auth/vc-request-with-auth /// we know that we're an joiner-observer. -/// the joining device has marked the peer as verified on vg-member-added/vc-contact-confirm -/// before sending vg-member-added-received - so, if we observe vg-member-added-received, +/// the joining device has marked the peer as verified +/// before sending vg-request-with-auth/vc-request-with-auth - so, if we observe vg-member-added-received, /// we can mark the peer as verified as well. pub(crate) async fn observe_securejoin_on_other_device( context: &Context, @@ -563,9 +559,7 @@ pub(crate) async fn observe_securejoin_on_other_device( "vg-request-with-auth" | "vc-request-with-auth" | "vg-member-added" - | "vc-contact-confirm" - | "vg-member-added-received" - | "vc-contact-confirm-received" => { + | "vc-contact-confirm" => { if !encrypted_and_signed( context, mime_message, @@ -941,27 +935,16 @@ mod tests { .unwrap(); assert_eq!(contact_bob.is_verified(&bob.ctx).await.unwrap(), false); - // Step 7: Bob receives vc-contact-confirm, sends vc-contact-confirm-received + // Step 7: Bob receives vc-contact-confirm bob.recv_msg(&sent).await; assert_eq!(contact_alice.is_verified(&bob.ctx).await.unwrap(), true); // Check Bob got the verified message in his 1:1 chat. - { - let chat = bob.create_chat(&alice).await; - let msg = get_chat_msg(&bob, chat.get_id(), 0, 1).await; - assert!(msg.is_info()); - let expected_text = chat_protection_enabled(&bob).await; - assert_eq!(msg.get_text(), expected_text); - } - - // Check Bob sent the final message - let sent = bob.pop_sent_msg().await; - let msg = alice.parse_msg(&sent).await; - assert!(msg.was_encrypted()); - assert_eq!( - msg.get_header(HeaderDef::SecureJoin).unwrap(), - "vc-contact-confirm-received" - ); + let chat = bob.create_chat(&alice).await; + let msg = get_chat_msg(&bob, chat.get_id(), 0, 1).await; + assert!(msg.is_info()); + let expected_text = chat_protection_enabled(&bob).await; + assert_eq!(msg.get_text(), expected_text); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -1073,17 +1056,10 @@ mod tests { let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id).await?; assert_eq!(contact_bob.is_verified(&bob.ctx).await?, false); - // Step 7: Bob receives vc-contact-confirm, sends vc-contact-confirm-received + // Step 7: Bob receives vc-contact-confirm bob.recv_msg(&sent).await; assert_eq!(contact_alice.is_verified(&bob.ctx).await?, true); - let sent = bob.pop_sent_msg().await; - let msg = alice.parse_msg(&sent).await; - assert!(msg.was_encrypted()); - assert_eq!( - msg.get_header(HeaderDef::SecureJoin).unwrap(), - "vc-contact-confirm-received" - ); Ok(()) } @@ -1248,7 +1224,7 @@ mod tests { let contact_alice = Contact::get_by_id(&bob.ctx, contact_alice_id).await?; assert_eq!(contact_bob.is_verified(&bob.ctx).await?, false); - // Step 7: Bob receives vg-member-added, sends vg-member-added-received + // Step 7: Bob receives vg-member-added bob.recv_msg(&sent).await; { // Bob has Alice verified, message shows up in the group chat. @@ -1268,14 +1244,6 @@ mod tests { } } - let sent = bob.pop_sent_msg().await; - let msg = alice.parse_msg(&sent).await; - assert!(msg.was_encrypted()); - assert_eq!( - msg.get_header(HeaderDef::SecureJoin).unwrap(), - "vg-member-added-received" - ); - let bob_chat = Chat::load_from_db(&bob.ctx, bob_chatid).await?; assert!(bob_chat.is_protected()); assert!(bob_chat.typ == Chattype::Group); diff --git a/src/securejoin/bobstate.rs b/src/securejoin/bobstate.rs index a32c4348bc..227c75cf29 100644 --- a/src/securejoin/bobstate.rs +++ b/src/securejoin/bobstate.rs @@ -345,17 +345,6 @@ impl BobState { .await?; context.emit_event(EventType::ContactsChanged(None)); - self.send_handshake_message(context, BobHandshakeMsg::ContactConfirmReceived) - .await - .map_err(|_| { - warn!( - context, - "Failed to send vc-contact-confirm-received/vg-member-added-received" - ); - }) - // This is not an error affecting the protocol outcome. - .ok(); - self.update_next(&context.sql, SecureJoinStep::Completed) .await?; Ok(Some(BobHandshakeStage::Completed)) @@ -401,9 +390,6 @@ async fn send_handshake_message( msg.param.set(Param::Arg2, invite.authcode()); msg.param.set_int(Param::GuaranteeE2ee, 1); } - BobHandshakeMsg::ContactConfirmReceived => { - msg.param.set_int(Param::GuaranteeE2ee, 1); - } }; // Sends our own fingerprint in the Secure-Join-Fingerprint header. @@ -425,8 +411,6 @@ enum BobHandshakeMsg { Request, /// vc-request-with-auth or vg-request-with-auth RequestWithAuth, - /// vc-contact-confirm-received or vg-member-added-received - ContactConfirmReceived, } impl BobHandshakeMsg { @@ -454,10 +438,6 @@ impl BobHandshakeMsg { QrInvite::Contact { .. } => "vc-request-with-auth", QrInvite::Group { .. } => "vg-request-with-auth", }, - Self::ContactConfirmReceived => match invite { - QrInvite::Contact { .. } => "vc-contact-confirm-received", - QrInvite::Group { .. } => "vg-member-added-received", - }, } } } From 7bbd682006bb6b4c5c6405235d1f7889c5fe3029 Mon Sep 17 00:00:00 2001 From: link2xt Date: Tue, 28 Nov 2023 20:27:04 +0000 Subject: [PATCH 4/7] refactor: factor securejoin processing out of add_parts --- src/receive_imf.rs | 132 ++++++++++++++++++++++----------------------- src/securejoin.rs | 1 + 2 files changed, 66 insertions(+), 67 deletions(-) diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 7d2c463958..2120008f89 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -238,25 +238,70 @@ pub(crate) async fn receive_imf_inner( update_verified_keys(context, &mut mime_parser, from_id).await?; - // Add parts - let received_msg = add_parts( - context, - &mut mime_parser, - imf_raw, - incoming, - &to_ids, - rfc724_mid, - sent_timestamp, - rcvd_timestamp, - from_id, - seen || replace_partial_download.is_some(), - is_partial_download, - replace_partial_download, - fetching_existing_messages, - prevent_rename, - ) - .await - .context("add_parts error")?; + let received_msg; + if let Some(securejoin_step) = mime_parser.get_header(HeaderDef::SecureJoin) { + info!(context, "Received securejoin step {securejoin_step}."); + + let res; + if incoming { + res = handle_securejoin_handshake(context, &mime_parser, from_id) + .await + .context("error in Secure-Join message handling")?; + + // Peerstate could be updated by handling the Securejoin handshake. + let contact = Contact::get_by_id(context, from_id).await?; + mime_parser.decryption_info.peerstate = + Peerstate::from_addr(context, contact.get_addr()).await?; + } else { + let to_id = to_ids.get(0).copied().unwrap_or_default(); + // handshake may mark contacts as verified and must be processed before chats are created + res = observe_securejoin_on_other_device(context, &mime_parser, to_id) + .await + .context("error in Secure-Join watching")? + } + + match res { + securejoin::HandshakeMessage::Done | securejoin::HandshakeMessage::Ignore => { + let msg_id = insert_tombstone(context, rfc724_mid).await?; + received_msg = Some(ReceivedMsg { + chat_id: DC_CHAT_ID_TRASH, + state: MessageState::InSeen, + sort_timestamp: sent_timestamp, + msg_ids: vec![msg_id], + needs_delete_job: res == securejoin::HandshakeMessage::Done, + }); + } + securejoin::HandshakeMessage::Propagate => { + received_msg = None; + } + } + } else { + received_msg = None; + } + + let received_msg = if let Some(received_msg) = received_msg { + received_msg + } else { + // Add parts + add_parts( + context, + &mut mime_parser, + imf_raw, + incoming, + &to_ids, + rfc724_mid, + sent_timestamp, + rcvd_timestamp, + from_id, + seen || replace_partial_download.is_some(), + is_partial_download, + replace_partial_download, + fetching_existing_messages, + prevent_rename, + ) + .await + .context("add_parts error")? + }; if !from_id.is_special() { contact::update_last_seen(context, from_id, sent_timestamp).await?; @@ -519,38 +564,6 @@ async fn add_parts( if incoming { to_id = ContactId::SELF; - // Whether the message is a part of securejoin handshake that should be marked as seen - // automatically. - let securejoin_seen; - - // handshake may mark contacts as verified and must be processed before chats are created - if mime_parser.get_header(HeaderDef::SecureJoin).is_some() { - match handle_securejoin_handshake(context, mime_parser, from_id) - .await - .context("error in Secure-Join message handling")? - { - securejoin::HandshakeMessage::Done => { - chat_id = Some(DC_CHAT_ID_TRASH); - needs_delete_job = true; - securejoin_seen = true; - } - securejoin::HandshakeMessage::Ignore => { - chat_id = Some(DC_CHAT_ID_TRASH); - securejoin_seen = true; - } - securejoin::HandshakeMessage::Propagate => { - // process messages as "member added" normally - securejoin_seen = false; - } - } - // Peerstate could be updated by handling the Securejoin handshake. - let contact = Contact::get_by_id(context, from_id).await?; - mime_parser.decryption_info.peerstate = - Peerstate::from_addr(context, contact.get_addr()).await?; - } else { - securejoin_seen = false; - } - let test_normal_chat = if from_id == ContactId::UNDEFINED { None } else { @@ -804,7 +817,6 @@ async fn add_parts( || is_mdn || is_reaction || is_location_kml - || securejoin_seen || chat_id_blocked == Blocked::Yes { MessageState::InSeen @@ -822,21 +834,7 @@ async fn add_parts( let self_sent = from_id == ContactId::SELF && to_ids.len() == 1 && to_ids.contains(&ContactId::SELF); - // handshake may mark contacts as verified and must be processed before chats are created - if mime_parser.get_header(HeaderDef::SecureJoin).is_some() { - match observe_securejoin_on_other_device(context, mime_parser, to_id) - .await - .context("error in Secure-Join watching")? - { - securejoin::HandshakeMessage::Done | securejoin::HandshakeMessage::Ignore => { - chat_id = Some(DC_CHAT_ID_TRASH); - } - securejoin::HandshakeMessage::Propagate => { - // process messages as "member added" normally - chat_id = None; - } - } - } else if mime_parser.sync_items.is_some() && self_sent { + if mime_parser.sync_items.is_some() && self_sent { chat_id = Some(DC_CHAT_ID_TRASH); } diff --git a/src/securejoin.rs b/src/securejoin.rs index dfdb74c064..e7c689f52f 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -243,6 +243,7 @@ async fn fingerprint_equals_sender( /// next with this incoming setup-contact/secure-join handshake message. /// /// [`receive_imf`]: crate::receive_imf::receive_imf +#[derive(Debug, PartialEq, Eq)] pub(crate) enum HandshakeMessage { /// The message has been fully handled and should be removed/delete. /// From 9e57752058e23ac681ab7f19e67fc240eeb3c9d4 Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 29 Nov 2023 02:17:50 +0000 Subject: [PATCH 5/7] feat: send Chat-Verified headers in 1:1 chats Chat-Verified is going to be useful to upgrade one-way verification to bidirectional verification. --- src/mimefactory.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 0aa0b90b46..2e415d87fc 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -924,9 +924,7 @@ impl<'a> MimeFactory<'a> { let mut meta_part = None; let send_verified_headers = match chat.typ { - // In single chats, the protection status isn't necessarily the same for both sides, - // so we don't send the Chat-Verified header: - Chattype::Single => false, + Chattype::Single => true, Chattype::Group => true, // Mailinglists and broadcast lists can actually never be verified: Chattype::Mailinglist => false, From 407a2ecc019b5af94166b00930e4b27a18ef0e7e Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 29 Nov 2023 03:33:10 +0000 Subject: [PATCH 6/7] refactor: remove unused PeerstateVerifiedStatus --- src/peerstate.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/peerstate.rs b/src/peerstate.rs index 2dbb52d6fc..c998dfbaae 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -26,17 +26,6 @@ pub enum PeerstateKeyType { PublicKey, } -/// Verification status of the contact peerstate. -#[derive(Debug, PartialEq, Eq, Clone, Copy, FromPrimitive)] -#[repr(u8)] -pub enum PeerstateVerifiedStatus { - /// Peerstate is not verified. - Unverified = 0, - //Verified = 1, // not used - /// Peerstate is verified and we assume that the contact has verified our peerstate. - BidirectVerified = 2, -} - /// Peerstate represents the state of an Autocrypt peer. #[derive(Debug, PartialEq, Eq, Clone)] pub struct Peerstate { From f13869668adc7088ea83810f0edb63ffd11a919c Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 29 Nov 2023 17:06:54 +0000 Subject: [PATCH 7/7] refactor: rename `min_verified` into `verified` --- src/e2ee.rs | 6 +++--- src/mimefactory.rs | 8 ++++---- src/peerstate.rs | 18 +++++++++--------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/e2ee.rs b/src/e2ee.rs index e2e20452d2..2453a5bd6d 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -94,7 +94,7 @@ impl EncryptHelper { pub async fn encrypt( self, context: &Context, - min_verified: bool, + verified: bool, mail_to_encrypt: lettre_email::PartBuilder, peerstates: Vec<(Option, &str)>, ) -> Result { @@ -107,7 +107,7 @@ impl EncryptHelper { .filter_map(|(state, addr)| state.clone().map(|s| (s, addr))) { let key = peerstate - .take_key(min_verified) + .take_key(verified) .with_context(|| format!("proper enc-key for {addr} missing, cannot encrypt"))?; keyring.push(key); verifier_addresses.push(addr); @@ -118,7 +118,7 @@ impl EncryptHelper { // Encrypt to secondary verified keys // if we also encrypt to the introducer ("verifier") of the key. - if min_verified { + if verified { for (peerstate, _addr) in peerstates { if let Some(peerstate) = peerstate { if let (Some(key), Some(verifier)) = ( diff --git a/src/mimefactory.rs b/src/mimefactory.rs index 2e415d87fc..85837edee2 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -312,7 +312,7 @@ impl<'a> MimeFactory<'a> { } } - fn min_verified(&self) -> bool { + fn verified(&self) -> bool { match &self.loaded { Loaded::Message { chat } => { if chat.is_protected() { @@ -627,7 +627,7 @@ impl<'a> MimeFactory<'a> { )); } - let min_verified = self.min_verified(); + let verified = self.verified(); let grpimage = self.grpimage(); let force_plaintext = self.should_force_plaintext(); let skip_autocrypt = self.should_skip_autocrypt(); @@ -723,7 +723,7 @@ impl<'a> MimeFactory<'a> { && self.should_do_gossip(context).await? { for peerstate in peerstates.iter().filter_map(|(state, _)| state.as_ref()) { - if let Some(header) = peerstate.render_gossip_header(min_verified) { + if let Some(header) = peerstate.render_gossip_header(verified) { message = message.header(Header::new("Autocrypt-Gossip".into(), header)); is_gossiped = true; } @@ -756,7 +756,7 @@ impl<'a> MimeFactory<'a> { } let encrypted = encrypt_helper - .encrypt(context, min_verified, message, peerstates) + .encrypt(context, verified, message, peerstates) .await?; outer_message diff --git a/src/peerstate.rs b/src/peerstate.rs index c998dfbaae..64e6b56895 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -362,8 +362,8 @@ impl Peerstate { } /// Returns the contents of the `Autocrypt-Gossip` header for outgoing messages. - pub fn render_gossip_header(&self, min_verified: bool) -> Option { - if let Some(key) = self.peek_key(min_verified) { + pub fn render_gossip_header(&self, verified: bool) -> Option { + if let Some(key) = self.peek_key(verified) { let header = Aheader::new( self.addr.clone(), key.clone(), // TODO: avoid cloning @@ -386,8 +386,8 @@ impl Peerstate { /// Converts the peerstate into the contact public key. /// /// Similar to [`Self::peek_key`], but consumes the peerstate and returns owned key. - pub fn take_key(mut self, min_verified: bool) -> Option { - if min_verified { + pub fn take_key(mut self, verified: bool) -> Option { + if verified { self.verified_key.take() } else { self.public_key.take().or_else(|| self.gossip_key.take()) @@ -396,15 +396,15 @@ impl Peerstate { /// Returns a reference to the contact public key. /// - /// `min_verified` determines the minimum required verification status of the key. + /// `verified` determines the required verification status of the key. /// If verified key is requested, returns the verified key, /// otherwise returns the Autocrypt key. /// /// Returned key is suitable for sending in `Autocrypt-Gossip` header. /// /// Returns `None` if there is no suitable public key. - pub fn peek_key(&self, min_verified: bool) -> Option<&SignedPublicKey> { - if min_verified { + pub fn peek_key(&self, verified: bool) -> Option<&SignedPublicKey> { + if verified { self.verified_key.as_ref() } else { self.public_key.as_ref().or(self.gossip_key.as_ref()) @@ -414,8 +414,8 @@ impl Peerstate { /// Returns a reference to the contact's public key fingerprint. /// /// Similar to [`Self::peek_key`], but returns the fingerprint instead of the key. - fn peek_key_fingerprint(&self, min_verified: bool) -> Option<&Fingerprint> { - if min_verified { + fn peek_key_fingerprint(&self, verified: bool) -> Option<&Fingerprint> { + if verified { self.verified_key_fingerprint.as_ref() } else { self.public_key_fingerprint