Add option to force encryption#8220
Conversation
8914248 to
f1652d4
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
bb4bde4 to
b47d145
Compare
b47d145 to
c67dc51
Compare
15d971f to
a1137cb
Compare
a412867 to
5836a72
Compare
5836a72 to
098f508
Compare
This change is a preparation for ignoring unencrypted messages by default. New test_utils::encrypt_raw_message and test_utils::receive_encrypted_imf are used to encrypt the messages before "receiving" them with receive_imf.
098f508 to
cb8ffb6
Compare
2b97913 to
5e61bba
Compare
e1886a0 to
ec863ca
Compare
ec863ca to
ae4c3f3
Compare
| // | ||
| // Corner case of ad hoc groups with only self as a member is ignored. | ||
| let max_unencrypted_timestamp: i64 = transaction.query_row( | ||
| "SELECT IFNULL(MAX(msgs.timestamp), 0) |
There was a problem hiding this comment.
SQLite "explain query plan" output:
QUERY PLAN
|--SEARCH contacts USING COVERING INDEX contacts_fingerprint_index (fingerprint=? AND rowid>?)
|--SEARCH chats_contacts USING INDEX chats_contacts_index2 (contact_id=?)
`--SEARCH msgs USING INDEX msgs_index2 (chat_id=?)
| let is_encrypted = if let Some(content_type) = headers.get_header_value(HeaderDef::ContentType) | ||
| { | ||
| mailparse::parse_content_type(&content_type).mimetype == "multipart/encrypted" | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| if flags.any(|f| f == Flag::Draft) { | ||
| info!(context, "Ignoring draft message"); | ||
| return Ok(false); | ||
| } | ||
|
|
||
| let should_download = !blocked_contact || maybe_ndn; | ||
| let should_download = maybe_ndn | ||
| || (!blocked_contact | ||
| && (is_encrypted || !context.get_config_bool(Config::ForceEncryption).await?)); |
There was a problem hiding this comment.
this drops SecureJoin V2 messages from being downloaded. Probably no test is catching that?
There was a problem hiding this comment.
Yes, there is no online test. There are Rust tests, but will need to copy manipulate_qr to Python tests.
There was a problem hiding this comment.
Actually there are cross-core SecureJoin tests, testing that 2.24.0 Bob can scan Alice's QR on a new core, and new core somehow downloads the message. SecureJoin v3 was added in 2.45.0
There was a problem hiding this comment.
alice_and_remote_bob fixture sends Alice as vCard to Bob first, so the test is not testing requesting the key because Bob already has Alice's key from a vCard. Will need to fix this, so full securejoin is tested.
There was a problem hiding this comment.
Fixed this and expanded cross-core test to catch the problem (in separate commits for now, will squash the fixup later)
| let chat = alice.create_email_chat(bob).await; | ||
|
|
||
| let mut msg = Message::new_text("Hello!".to_string()); | ||
| assert!(chat::send_msg(alice, chat.id, &mut msg).await.is_err()); |
There was a problem hiding this comment.
Hum, if the profile is e2ee only, create_email_chat is never called from UIs.
but i guess a user that toggles "force encryption" and then tries to send a message in an email chat would get an error. However, the error here is pretty internal, and there is no stock string for "can not send unencrypted message".
There was a problem hiding this comment.
There is InvalidUnencryptedMail stock string, we can try to make sure it is used.
| Subject: [...]\r | ||
| \r | ||
| \r | ||
| --{boundary} |
| \r | ||
| {encrypted_payload} |
There was a problem hiding this comment.
missing "\r" ? (for some gh UI reason i could not just comment on the {encrypted_payload} line.
There was a problem hiding this comment.
i guess mailparse is lenient enough about just "\n" but i can not claim i fully understand all line-ending business at play.
| let is_encrypted = headers | ||
| .get_header_value(HeaderDef::ContentType) | ||
| .is_some_and(|content_type| { | ||
| mailparse::parse_content_type(&content_type).mimetype == "multipart/encrypted" |
There was a problem hiding this comment.
Do i understand correctly that we don't want to support "multipart/mixed" encrypted email? An example is test-data/message/google-workspace-mixed-up.eml
| let encrypted_message = test_utils::encrypt_raw_message( | ||
| bob, | ||
| &[alice], | ||
| b"From: bob@example.net\r\n\ |
There was a problem hiding this comment.
Maybe replace "\n" with "\r\n" in encrypt_raw_message() before encryption, or do we sometimes need just "\n"?
|
|
||
| let now = tools::time(); | ||
| let max_unencrypted_timestamp = std::cmp::max(max_unencrypted_timestamp, max_mailing_list_timestamp); | ||
| if max_unencrypted_timestamp.saturating_add(3600 * 24 * 90) > now { |
There was a problem hiding this comment.
delete_device_after affects this if it's set to <= 5 weeks. It may even be disabled, but set right before, we can't know that. Just disabling force_encryption for existing users is probably not what we want however. Maybe it makes sense to also check if there are any messages older than 90 days at all, not sure.
| assert!(chat::send_msg(alice, chat.id, &mut msg).await.is_err()); | ||
| assert_eq!( | ||
| msg.error().unwrap(), | ||
| "\u{26a0}\u{fe0f} Your email provider example.org requires end-to-end encryption which is not setup yet." |
There was a problem hiding this comment.
"Your server" or "Your relay" may be better these days, may be reworded in another PR
Closes #7494
There is a new
force_encryptionconfig which is enabled by default. For users with recently active unencrypted chats it is disabled in a migration. Enabling it prevents both sending and receiving unencrypted messages, so when sending to unencrypted chats we will no longer send unencrypted message to a chatmail relay for it to be rejected, but fail locally.There are many changes to tests because we had a lot of tests using unencrypted chats, I have put them into separate commits.
The setting is not per-relay as there have been more discussion after the comments below. UIs will put the setting somewhere deeper than "Advanced" likely inside the "Relays" configuration but not in the individual relay settings.
Based on #8226 but does not really depend on it. There are small merge conflicts if #8226 is not merged first, but I can rebase this PR on
mainif needed.