Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BREAKING: Remove Secure-Join-Fingerprint header from "vc-contact-confirm", "vg-member-added" messages. Remove "vc-contact-confirm-received", "vg-member-added-received" messages from Securejoin protocol. #4345

Closed
wants to merge 2 commits into from
Closed

BREAKING: Remove Secure-Join-Fingerprint header from "vc-contact-confirm", "vg-member-added" messages. Remove "vc-contact-confirm-received", "vg-member-added-received" messages from Securejoin protocol. #4345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4da9e39
BREAKING: Remove Secure-Join-Fingerprint header from "vc-contact-conf…
iequidoo Apr 28, 2023
3178308
BREAKING: Remove "vc-contact-confirm-received", "vg-member-added-rece…
iequidoo Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 0 additions & 11 deletions src/mimefactory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down
109 changes: 10 additions & 99 deletions src/securejoin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -494,33 +489,8 @@ pub(crate) async fn handle_securejoin_handshake(
}
}
"vg-member-added-received" | "vc-contact-confirm-received" => {
iequidoo marked this conversation as resolved.
Show resolved Hide resolved
/*==========================================================
==== 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);
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum

  • This is pretty quick to remove the support for this isn't it? I don't know if we have a policy but IIRC users start to be nagged to upgrade after about 1 year, so I guess you need to wait for at least 1 year?
  • It was kind of nice that you could read the securejoin protocol and get the fingerprint without having to understand the autocrypt gossip stuff. This makes the dependence stronger, and thus increases the number of things you need to understand to work on it. On the other hand it duplicates information and that's also confusing. I realise I may have said something that led to this... I don't always say smart things... of course this could also be one of those.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Agree, let's wait for 1 year, made it a draft.
  • Maybe it would be nice for the Securejoin to not depend on Autocrypt things, but anyway knowing a joiner's fingerpint isn't sufficient for other devices on a joining side to encrypt to the joiner, a key (gossip) is needed. There's also the corresponding PR to the Securejoin documentation: Document when gossips in 'vc-request-with-auth' are needed nextleap-project/countermitm#85 . So, i think better not to complicate the Securejoin protocol with passing keys and rely on the Autocrypt. CC @hpk42

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,
Expand Down Expand Up @@ -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(),
Expand All @@ -985,15 +921,6 @@ mod tests {
let text = msg.get_text().unwrap();
assert!(text.contains("[email protected] 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)]
Expand Down Expand Up @@ -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(())
}

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down
20 changes: 0 additions & 20 deletions src/securejoin/bobstate.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down Expand Up @@ -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",
},
}
}
}
Expand Down