BREAKING: Remove "vc-contact-confirm-received", "vg-member-added-received" messages from Securejoin protocol (#3836)

They are an overkill and do not solve any problem. Also they aren't documented anywhere. And all
necessary tests were added before and they work w/o the removed code.

AFAIU, they were added for Alice (joiner) to be sure that Bob (joinee) has Alice two-way verified,
i.e. Bob can add Alice to other verified groups because Bob knows that Alice trusts them. But that
means Alice should retry sending vc-contact-confirm/vg-member-added messages until getting
*-received message from Bob. But then better let Bob retry sending *-auth-required until getting
vc-contact-confirm/vg-member-added from Alice. And if Alice sees no more retries from Bob, either
Bob has got vc-contact-confirm/vg-member-added from Alice or there are communication problems. But
the latter can't be solved anyway by adding extra messages to the protocol. Note that there are no
retries currently, instead we rely on that Bob will eventually receive a message from Alice and thus
know that Alice verified them. But i could miss some details why they were added, so just in case,
citing from #3836:

    @iequidoo: Btw, can somebody explain the purpose of
    vg-member-added-received/vc-contact-confirm-received messages in the protocol? They look
    excessive and for me it's like a try to solve that problem with two friends that want to meet
    and drink some beer but only if each of them is sure that their beermate is also going to the
    meeting :) Even if Bob didn't receive vg-member-added message (e.g. because of mail delivery
    problems), we can consider Bob joined -- Bob can try sending messages to the group and that must
    work afaiu.

    @flub: Yes, this is a weird state. It is currently the difference between an internal state
    "un-idirectional verified" and the exposed state "bi-directional verified". The latter (bi-)
    means both know that both have each other verified, in the former (uni-) only one of them knows
    this and the other hasn't figured this out yet.

    IIRC the last time this was discussed the revelation (at least to me) was that the main
    practical difference between these two is that bi-directional allows you to add the other person
    to a verified group, while if you only have them uni-directional verified you can not do they
    since they don't trust you enough (IIRC, there should be a cryptpad somewhere with this written
    down).

    @link2xt: When Bob receives vc-auth-required, he already has Alice one-way verified. When Alice
    receives vc-request-with-auth, she has Bob two-way verified (she has verified Bob's key and she
    know Bob has verified her), but Bob still does not know about this. When Bob receives
    vc-contact-confirm (or vg-member-added) he sets Alice state to "two-way verified".

    The question is what happens if vc-contact-confirm/vg-member-added is lost. In this case Alice
    has Bob two-way verified, while Bob has Alice only one-way verified. Because of this, Bob cannot
    safely add Alice to any verified group, because Bob thinks maybe Alice has not received
    vc-request-with-auth and has Bob completely unverified. However, Alice still can add Bob to
    verified groups and if Bob later receives any message from Alice in a verified group, he sets
    Alice to two-way verified, so eventually both Alice and Bob converge to a two-way verified
    state. This is not how it is currently implemented, but this was the idea during the discussion
    with @flub and @missytake.

    But vg-member-added-received/vc-contact-confirm-received is an overkill and can be removed IMO,
    it does not solve any problem.

CHANGELOG.md

@@ -6,6 +6,8 @@
 - BREAKING: jsonrpc:
   - `get_chatlist_items_by_entries` now takes only chatids instead of `ChatListEntries`
   - `get_chatlist_entries` now returns `Vec<u32>` of chatids instead of `ChatListEntries`
+- BREAKING: Remove Secure-Join-Fingerprint header from "vc-contact-confirm", "vg-member-added" messages.
+- BREAKING: Remove "vc-contact-confirm-received", "vg-member-added-received" messages from Securejoin protocol.
 
 
 ## [1.114.0] - 2023-04-24

src/mimefactory.rs

@@ -945,17 +945,6 @@ impl<'a> MimeFactory<'a> {
                             "Secure-Join".to_string(),
                             "vg-member-added".to_string(),
                         ));
-                        // FIXME: Old clients require Secure-Join-Fingerprint header. Remove this
-                        // eventually.
-                        let fingerprint = Peerstate::from_addr(context, email_to_add)
-                            .await?
-                            .context("No peerstate found in db")?
-                            .public_key_fingerprint
-                            .context("No public key fingerprint in db for the member to add")?;
-                        headers.protected.push(Header::new(
-                            "Secure-Join-Fingerprint".into(),
-                            fingerprint.hex(),
-                        ));
                     }
                 }
                 SystemMessage::GroupNameChanged => {

src/securejoin.rs

@@ -9,7 +9,7 @@ use crate::aheader::EncryptPreference;
 use crate::chat::{self, Chat, ChatId, ChatIdBlocked};
 use crate::config::Config;
 use crate::constants::Blocked;
-use crate::contact::{Contact, ContactId, Origin, VerifiedStatus};
+use crate::contact::{Contact, ContactId, Origin};
 use crate::context::Context;
 use crate::e2ee::ensure_secret_key_exists;
 use crate::events::EventType;
@@ -465,14 +465,9 @@ pub(crate) async fn handle_securejoin_handshake(
                     info_chat_id(context, contact_id).await?,
                 )
                 .await?;
-                send_alice_handshake_msg(
-                    context,
-                    contact_id,
-                    "vc-contact-confirm",
-                    Some(fingerprint),
-                )
-                .await
-                .context("failed sending vc-contact-confirm message")?;
+                send_alice_handshake_msg(context, contact_id, "vc-contact-confirm", None)
+                    .await
+                    .context("failed sending vc-contact-confirm message")?;
 
                 inviter_progress!(context, contact_id, 1000);
             }
@@ -494,33 +489,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  ====
-            ==========================================================*/
-
-            if let Ok(contact) = Contact::get_by_id(context, contact_id).await {
-                if contact.is_verified(context).await? == VerifiedStatus::Unverified {
-                    warn!(context, "{} invalid.", step);
-                    return Ok(HandshakeMessage::Ignore);
-                }
-                if join_vg {
-                    let field_grpid = mime_message
-                        .get_header(HeaderDef::SecureJoinGroup)
-                        .map(|s| s.as_str())
-                        .unwrap_or_else(|| "");
-                    if let Err(err) = chat::get_chat_id_by_grpid(context, field_grpid).await {
-                        warn!(context, "Failed to lookup chat_id from grpid: {}", err);
-                        return Err(
-                            err.context(format!("Chat for group {} not found", &field_grpid))
-                        );
-                    }
-                }
-                Ok(HandshakeMessage::Ignore) // "Done" deletes the message and breaks multi-device
-            } else {
-                warn!(context, "{} invalid.", step);
-                Ok(HandshakeMessage::Ignore)
-            }
+            // The peer isn't updated yet and still sends these messages.
+            Ok(HandshakeMessage::Ignore)
         }
         _ => {
             warn!(context, "invalid step: {}", step);
@@ -540,12 +510,6 @@ pub(crate) async fn handle_securejoin_handshake(
 ///   The inviting device has marked a peer as verified on vg-request-with-auth/vc-request-with-auth
 ///   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
-///   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,
-///   we can mark the peer as verified as well.
 pub(crate) async fn observe_securejoin_on_other_device(
     context: &Context,
     mime_message: &MimeMessage,
@@ -563,9 +527,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,
@@ -631,32 +593,6 @@ pub(crate) async fn observe_securejoin_on_other_device(
                 }
                 peerstate.prefer_encrypt = EncryptPreference::Mutual;
                 peerstate.save_to_db(&context.sql).await.unwrap_or_default();
-            } else if let Some(fingerprint) =
-                mime_message.get_header(HeaderDef::SecureJoinFingerprint)
-            {
-                // FIXME: Old versions of DC send this header instead of gossips. Remove this
-                // eventually.
-                let fingerprint = fingerprint.parse()?;
-                if mark_peer_as_verified(
-                    context,
-                    fingerprint,
-                    Contact::load_from_db(context, contact_id)
-                        .await?
-                        .get_addr()
-                        .to_owned(),
-                )
-                .await
-                .is_err()
-                {
-                    could_not_establish_secure_connection(
-                        context,
-                        contact_id,
-                        info_chat_id(context, contact_id).await?,
-                        format!("Fingerprint mismatch on observing {step}.").as_ref(),
-                    )
-                    .await?;
-                    return Ok(HandshakeMessage::Ignore);
-                }
             } else {
                 could_not_establish_secure_connection(
                     context,
@@ -960,7 +896,7 @@ mod tests {
             VerifiedStatus::Unverified
         );
 
-        // 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(),
@@ -985,15 +921,6 @@ mod tests {
             let text = msg.get_text().unwrap();
             assert!(text.contains("alice@example.org verified"));
         }
-
-        // 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"
-        );
     }
 
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
@@ -1111,20 +1038,12 @@ mod tests {
             VerifiedStatus::Unverified
         );
 
-        // 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?,
             VerifiedStatus::BidirectVerified
         );
-
-        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(())
     }
 
@@ -1308,7 +1227,7 @@ mod tests {
             VerifiedStatus::Unverified
         );
 
-        // 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.
@@ -1355,14 +1274,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);

src/securejoin/bobstate.rs

@@ -386,17 +386,6 @@ impl BobState {
             }
         }
 
-        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))
@@ -442,9 +431,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.
@@ -466,8 +452,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 {
@@ -495,10 +479,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",
-            },
         }
     }
 }