-
-
Notifications
You must be signed in to change notification settings - Fork 106
Remove protected chats #7116
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.
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
base: link2xt/recv-gossip-verified
Are you sure you want to change the base?
Remove protected chats #7116
Conversation
c7e6f1c
to
e180d7f
Compare
mimeparser: &MimeMessage, | ||
) -> Result<()> { | ||
if mimeparser.get_header(HeaderDef::ChatVerified).is_none() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Old Chat-Verified
mechanism is completely ignored here.
All the tests now work using only the new mechanism.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test how group members using the old&new version interact? E.g., would be bad if the old version rejects messages sent by new Delta Chat because of a missing ChatVerified header.
I found one bug:
- Bob is using the old version (without this PR), Alice and Hocuri are on the new version (with this PR)
- Bob creates a group with Hocuri (-> the group is protected for Bob)
- Hocuri adds Alice, Alice sends a message into the group
- -> Bob's device replaces the message by an error:

I think there are two possible solutions for the problem:
- First make a release that accepts verifications via the _verified field, and that, ignores the verified property. And then make a release that actually removes protected chats.
- From a user side, this is the most seamless.
- Or: Modify this PR to still keep track of which group is regarded as 'protected' by group members running older clients. Then, continue to send the
Chat-Verified:
header in these groups.- From a user side, this is less seamless, because a user using the new version could still add a non-verified member, and users using the old versions will get an error when the non-verified member sends messages. OTOH, it arguably is already a UX bug that you can't add unverified members to some groups.
@@ -34,88 +34,112 @@ Content-Transfer-Encoding: 7bit | |||
|
|||
-----BEGIN PGP MESSAGE----- | |||
|
|||
wU4D5tq63hTeebASAQdATHbs7R5uRADpjsyAvrozHqQ/9nSrspwbLN6XJKuR3xcg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Payload here is replaced to have no Chat-Verified
header inside and have _verified=1
in gossip headers. Payload got larger likely because rsop
does not do compression and I had to reencrypt the payload.
867c2ba
to
780ca50
Compare
53d6a2d
to
a6301b9
Compare
88700f4
to
5b31b5d
Compare
6b67844
to
6052d1a
Compare
6052d1a
to
4e956bd
Compare
f1389dd
to
701dafe
Compare
src/receive_imf.rs
Outdated
// is verified, but it is not known by whom. | ||
context | ||
.sql | ||
.execute("UPDATE contacts SET verifier=?1 WHERE id=?1", (to_id,)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We mustn't reverify already verified contacts by "unknown contact" here, but i suggest to just merge at least the first commit from #7138, to make this PR a bit smaller and easier to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 for 900 removed lines!
Partial review. Some smaller things, and I found a bug: #7116 (comment) (and on first sight, it's a rather bad bug, I think :/)
Concerning backwards-compatibility: I would prefer to deprecate rather than remove functions / change function signatures. This will mean that UIs can be compiled with current core until they update. (compiling with current core is useful for trying things out)
In a few weeks/months, we can then remove the deprecated functions.
(is_profile_verified
and is_protection_broken
can just be removed without any difficulties for UIs)
But I also see that it's multiple breaking changes at once, and that UIs anyways have to adapt before they can make a new release.
In any case, would be good if you write a checklist of what UIs have to do, and then copy-paste it into new issues in all UIs.
@@ -5278,8 +5236,7 @@ int dc_contact_is_blocked (const dc_contact_t* contact); | |||
* | |||
* @memberof dc_contact_t | |||
* @param contact The contact object. | |||
* @return 0: contact is not verified. | |||
* 2: SELF and contact have verified their fingerprints in both directions. | |||
* @return 1=contact is verified, 0=contact is not verified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backwards-compat: At least Android expects this to return 2, rather than 1:
return dc_contact_is_verified(get_dc_contact(env, obj))==2;
I think it's easier to just keep returning 2 - at some point in the future, we will probably get rid of the C bindings, anyways.
* @param chat The chat object. | ||
* @return 1=chat protected, 0=chat is not protected. | ||
*/ | ||
int dc_chat_is_protected (const dc_chat_t* chat); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backwards-compat: Current UIs still check this because they are supposed to only let the user add verified contacts to protected chats.
So, would be nice to keep (and deprecate) this for another release, and let it always return 0.
mimeparser: &MimeMessage, | ||
) -> Result<()> { | ||
if mimeparser.get_header(HeaderDef::ChatVerified).is_none() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test how group members using the old&new version interact? E.g., would be bad if the old version rejects messages sent by new Delta Chat because of a missing ChatVerified header.
I found one bug:
- Bob is using the old version (without this PR), Alice and Hocuri are on the new version (with this PR)
- Bob creates a group with Hocuri (-> the group is protected for Bob)
- Hocuri adds Alice, Alice sends a message into the group
- -> Bob's device replaces the message by an error:

I think there are two possible solutions for the problem:
- First make a release that accepts verifications via the _verified field, and that, ignores the verified property. And then make a release that actually removes protected chats.
- From a user side, this is the most seamless.
- Or: Modify this PR to still keep track of which group is regarded as 'protected' by group members running older clients. Then, continue to send the
Chat-Verified:
header in these groups.- From a user side, this is less seamless, because a user using the new version could still add a non-verified member, and users using the old versions will get an error when the non-verified member sends messages. OTOH, it arguably is already a UX bug that you can't add unverified members to some groups.
python/tests/test_1_online.py
Outdated
assert chat3.is_promoted() | ||
messages = chat3.get_messages() | ||
assert len(messages) == 2 | ||
assert len(messages) == 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it 3 messages instead of 2 now? Was no "Messages are end-to-end encrypted" message added before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, previously unprotected groups had no such message.
python/tests/test_3_offline.py
Outdated
assert len(sysmessages) == 3 | ||
assert len(sysmessages) == 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, why is it 4 messages now rather than 3?
src/aheader.rs
Outdated
// `_verified` attribute is an extension to `Autocrypt-Gossip` | ||
// header that is used to tell that the sender | ||
// marked this key as verified. | ||
pub is_verified: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to document this at https://github.com/chatmail/core/blob/main/spec.md
src/aheader.rs
Outdated
if self.is_verified { | ||
write!(fmt, " _verified=1;")?; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might this make the first line of the Autocrypt: header too large, so that some email providers reject it?
I don't know why, but the first line of a random message in my DeltaChat inbox already is quite large:
Autocrypt: addr=******@*********.de; prefer-encrypt=mutual; keydata=**********************************************************************
******************************************************************************
******************************************************************************
Apparently some line-wrapping isn't working here - anyways, the _verified=1;
will make this line even longer, not sure if this will create any problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_verified
is only sent in Autocrypt-Gossip
which goes into encrypted part.
0b8c3ca
to
904e5a3
Compare
ec369bc
to
4ab43d6
Compare
904e5a3
to
d8c237b
Compare
4ab43d6
to
355ceaa
Compare
6d9ce24
to
b91322a
Compare
This mechanism replaces `Chat-Verified` header. New parameter `_verified=1` in `Autocrypt-Gossip` header marks that the sender has the gossiped key verified. Using `_verified=1` instead of `_verified` because it is less likely to cause troubles with existing Autocrypt header parsers. This is also how https://www.rfc-editor.org/rfc/rfc2045 defines parameter syntax.
2000568
to
04eda07
Compare
b91322a
to
49f6b8d
Compare
Create unprotected group in test_create_protected_grp_multidev The test is renamed accordingly. SystemMessage::ChatE2ee is added in encrypted groups regardless of whether they are protected or not. Previously new encrypted unprotected groups had no message saying that messages are end-to-end encrypted at all.
49f6b8d
to
83a9265
Compare
Based on #7147
Since key-contacts cannot change their keys and having "verification" is less important for contact identification than having a chat history or shared chats with a contact, UIs have stopped displaying green checkmarks everywhere (deltachat/deltachat-android#3828).
There was also a bug #7107 (closed in #7113) that resulted in updating "verified by" information for already verified contacts, so the information about "verifiers" is lost.
We also want to make "verification" more meaningful by only verifying contacts if contact QR code is scanned directly within a short period of time and not just because someone used an invite link to join the group: #7111
Because of this it makes sense to reset existing verification status for contacts and start from scratch.
We can also allow to remove verification from contacts (https://support.delta.chat/t/how-would-i-remove-the-verified-checkmarks-from-one-invite-code-in-retrospect/3403) as it will not break "protected" chats anymore, but this is out of scope for this PR.
The plan is to stop creating new "protected" chats, convert already "protected" group chats into just group chats, reset existing verifications and switch to a new mechanism for verification gossip so old verifications don't resurrect via gossip.
Based on #7146. I have moved easy to review changes there.
What this PR does:
is_protection_broken
APIs. (moved to Various protected group changes #7146)is_profile_verified
API. (moved to Various protected group changes #7146)Chat-Verified: 1
header._verified
attribute forAutocrypt-Gossip
and use it to indirectly verify key-contacts when such attribute is received from a verified contact (whether directly verified or not).add_parts
that result in "The message was sent by non-verified contact" messages. We likely don't need to passverified_encryption
around, it is only used to decide whether we want to accept verification gossip from the contact.Chat.is_protected()
APIs.dc_contact_is_verified
documentation.To close #7111 we also need to:
Closes #7080 (replace verification gossip mechanism with a new one that works using Autocrypt-Gossip header and is independent of "protected chats")
Closes #7112 (removing deprecated and unneeded APIs)