fix: Prefer to encrypt if E2eeEnabled even if peers have EncryptPreference::NoPreference #6377
Conversation
iequidoo
force-pushed
the
iequidoo/chatmail-prefer-to-encrypt
branch
2 times, most recently
from
027e215 to
46ed76d
Compare
|
Hm, if a peer has the same domain, we can send unencrypted messages to it even if we know its key, but idk how frequent and important this scenario is and if we want to add more code complexity for it. Probably we need to rethink this anyway when we add multi-transport accounts. |
src/e2ee.rs
Outdated
| if match peerstate.prefer_encrypt { | ||
| EncryptPreference::NoPreference | EncryptPreference::Reset => { | ||
| let verified = false; | ||
| peerstate.peek_key(verified).is_some() && context.is_chatmail().await? |
There was a problem hiding this comment.
is_chatmail() call can be done once at the beginning of the function or even passed from outside to make it non-async again.
If is_chatmail() is true I would also ignore own preference, at least on Android encryption preference setting is even hidden for chatmail profiles.
There was a problem hiding this comment.
I think there's no much sense to make the function non-async, it's quite complex, so let it be async to stabilize the API. It can be made pub(crate) though.
As for ignoring our own preference, if the user somehow changed it, they understand what they do and can revert the change, so i suggest to keep this as is.
There was a problem hiding this comment.
As for ignoring our own preference, if the user somehow changed it, they understand what they do and can revert the change, so i suggest to keep this as is.
yes, please, maybe some 3rd party client would allow to change preference and then core should not force it because the account is chatmail
iequidoo
force-pushed
the
iequidoo/chatmail-prefer-to-encrypt
branch
from
46ed76d to
f90b28c
Compare
| } | ||
| EncryptPreference::Mutual => true, | ||
| } { | ||
| prefer_encrypt_count += 1; |
There was a problem hiding this comment.
Note that if we don't know the keys of the chat majority, we still try to send an unencrypted message. So, the majority rule is preserved in this sense. Idk if we want to change this, we can send an encrypted message, but it won't be very useful it seems
There was a problem hiding this comment.
I think it is better if own preference is enforced, regardless of receiver preference, if receiver supports encrypted messages and I prefer to send encrypted, then DC should send encrypted, if I prefer to send unencrypted, core should allow ME to send unencrypted, while receiver can decide if they want to encrypt on their own
I think this is more powerful and flexible than current behavior, user can't "disable" encryption nor force sending encrypted and have peace of mind, in this regard autocrypt handling is better in k9mail (aka thunderbird android) where you can explicitly control encryption enable/disable of the message you are sending
There was a problem hiding this comment.
I think it is better if own preference is enforced, regardless of receiver preference, if receiver supports encrypted messages and I prefer to send encrypted, then DC should send encrypted, if I prefer to send unencrypted, core should allow ME to send unencrypted, while receiver can decide if they want to encrypt on their own
With this PR you get the described behaviour for chatmail in 1:1 chats. The only thing is that you can't change the setting in the UI for chatmail. But for group chats there's still the "majority rule", only the condition is changed for peers -- instead of looking at encryption preference we check if we have the peer's key.
Tbh, if we need to change the logic this way for chatmail, i'd change it for all setups. I agree that it's better to prefer own setting. But such a behaviuor diverges from Autocrypt.
There was a problem hiding this comment.
I'd only make one more improvement -- if we prefer to send unencrypted (e2ee_enabled=0) then look at the peer's encryption preference, not at the presense of the peer's key. For chatmail this doesn't really matter because of no UI setting, but for non-chatmail this allows unencrypted communication as currently.
EDIT: Then better not to look at is_chatmail at all, but only at e2ee_enabled. This way the fix still works for chatmail (but the user can manually change the setting e.g. via environment), but we get the discussed improvement for non-chatmail (prefer the local setting). We deviate from Autocrypt for chatmail anyway, so maybe do that for all setups and see if users complain.
There was a problem hiding this comment.
I'm also fine with ignoring everyone else's preference, this "majority vote" was never practically usable as there are no groups where majority prefers not to encrypt except maybe very small groups where it will be unexpected. Before the majority vote we had "veto" rule where anyone preferring not to encrypt resulted in unencrypted group.
There was a problem hiding this comment.
Still, i left this "majority rule", but now the majority with NoPreference can't disable encryption if the local preference is Mutual. Additionally, for chatmail even the majority with Reset can't do so.
The majority "enabling" encryption if the local preference is NoPreference still has sense. Anyway there are other cases when we have no choice and must send encrypted messages -- protected groups, replies to encrypted messages etc.
iequidoo
force-pushed
the
iequidoo/chatmail-prefer-to-encrypt
branch
from
f90b28c to
80faf6a
Compare
iequidoo
changed the title
iequidoo
force-pushed
the
iequidoo/chatmail-prefer-to-encrypt
branch
from
80faf6a to
1841599
Compare
|
Still, in groups, if one of the peers has a missing peerstate, for chatmail we try to send an unencrypted message, but it probably will be rejected by the server. I have no simple idea how to fix this. |
…rence::NoPreference First of all, chatmail servers normally forbid to send unencrypted mail, so if we know the peer's key, we should encrypt to it. Chatmail setups have `E2eeEnabled=1` by default and this isn't possible to change in UIs, so this change fixes the chatmail case. Additionally, for chatmail, if a peer has `EncryptPreference::Reset`, let's handle it as `EncryptPreference::NoPreference` for the reason above. Still, if `E2eeEnabled` is 0 for a chatmail setup somehow, e.g. the user set it via environment, let's assume that the user knows what they do and ignore `IsChatmail` flag. NB: - If we don't know the peer's key, we should try to send an unencrypted message as before for a chatmail setup. - This change doesn't remove the "majority rule", but now the majority with `EncryptPreference::NoPreference` can't disable encryption if the local preference is `Mutual`. To disable encryption, some peer should have a missing peerstate or, for the non-chatmail case, the majority should have `EncryptPreference::Reset`.
iequidoo
force-pushed
the
iequidoo/chatmail-prefer-to-encrypt
branch
from
1841599 to
8aa990d
Compare
No description provided.