refactor(mimefactory): separate rendering of message payload and sendable message - #8345
refactor(mimefactory): separate rendering of message payload and sendable message#8345link2xt wants to merge 1 commit into
Conversation
link2xt
changed the title
45102bc to
1956c42
Compare
5f3c57c to
f9c33de
Compare
f9c33de to
e314fc0
Compare
3d4610c to
b1b0e3f
Compare
ce43887 to
6abd90c
Compare
6abd90c to
e350dd6
Compare
a18ab11 to
2e20218
Compare
a39da96 to
1c19dd8
Compare
23c8c8a to
9351ae3
Compare
c85e622 to
908b761
Compare
9351ae3 to
598ac45
Compare
| } else { | ||
| SeipdVersion::V1 | ||
| }; | ||
| let display_name = if is_securejoin_message && !is_encrypted { |
There was a problem hiding this comment.
Btw, this is apparently not the correct implementation of #7396 , see the protocol flow chart -- Alice should attach her profile (avatar, displayname, etc.) only at the final step, but not in the "vc-pubkey" message which is encrypted with AUTH. It seems this doesn't create any security issue, but it's just enough to send the profile at the final step. Also if we let Alice review "Securejoin requests" in the future before fully executing them, showing Bob's profile to Alice first (not otherwise) makes sense:
Since we want to give Alice the opportunity to review join requests (as a future improvement that doesn't require breaking the protocol), Alice can't send any private data in the second message. [...]
EDIT: For Bob it's even worse because he sends his displayname already in "vc-request-pubkey" apparently, so e.g. other broadcast subscribers can see it.
CC @Hocuri
This is not related to the refactoring, but i'd like this to be reviewed by others.
There was a problem hiding this comment.
Ok, this misunderstanding comes from improper naming of is_encrypted which is evaluated by this function:
pub fn will_be_encrypted(&self) -> bool {
self.encryption_pubkeys.is_some()
}Could it be renamed in a separate commit to e.g. is_pubkey_encrypted? I've created #8399 and tested it with this PR as well, everything works as expected actually.
There was a problem hiding this comment.
IIUC, the naming of will_be_encrypted is correct. If the message is symmetrically-encrypted, then encryption_pubkeys is Some(vec![]). As @link2xt said elsewhere, would be a nice refactoring to put this into an enum instead in order to prevent such confusion.
The "vc-pubkey" and "vc-request-pubkey" messages don't use the function here. Instead, they are created by render_symm_encrypted_securejoin_message. This is why the code works as correctly. Thanks for writing a test for it!
There was a problem hiding this comment.
Thanks for making it clear! I think that just renaming encryption_pubkeys to encryption would make the code more understandable (because it's actually not only about pubkeys). And as for this check, it could use self.encryption.is_none_or(|pubkeys| pubkeys.is_empty()), but maybe it's better to add an assertion that we're not rendering a symmetrically encrypted SecureJoin message here.
Agreed, there is no need to support chat-like features in unencrypted messages anymore. Hidden headers are from a time when we wanted to enable nice DC-to-DC communication via unencrypted messages with avatars etc. |
|
ftr, some history: we had late-mime-generation until 2018 and switched to early-mime-generation for various reasons, mainly speed and disappearing-messages, thid old core-c issue shows more detailes deltachat/deltachat-core#427 while performance might still be a concern, esp when there are only short time windows to get things out, iiuc, in this PR most data to be sent out are still generated early, so eg it is fine if database records the message originates from are deleted, this is important eg. for disappearing messages that time, however, mime-generation included encryption, so doing that late uses the state of the database when message goes out, not the state when send button is pressed, which led to all kind of issues. if i get it corectly, this is not the case with this PR, the mime |
|
I made some measurements on my 5 year old middle-class phone (Fairphone 3), compiled in release mode. FTR, this PR here doesn't switch to late encryption yet (it's just refactoring that will enable us to switch to late encryption). And yes, the plan is to switch to late encryption, but keep early mime generation. Once we switch to late encryption, the function Click to see the diff I used for measuringdiff --git a/src/chat.rs b/src/chat.rs
index 0ad2fd49d..9ee6bfc65 100644
--- a/src/chat.rs
+++ b/src/chat.rs
@@ -6,7 +6,7 @@
use std::io::Cursor;
use std::marker::Sync;
use std::path::{Path, PathBuf};
-use std::time::Duration;
+use std::time::{Duration, Instant};
use anyhow::{Context as _, Result, anyhow, bail, ensure};
use chrono::TimeZone;
@@ -2780,10 +2780,17 @@ async fn render_mime_message_and_pre_message(
let mut mimefactory_post_msg = mimefactory.clone();
mimefactory_post_msg.set_as_post_message();
+ let start = Instant::now();
let (queued_msg, side_effects) = Box::pin(mimefactory_post_msg.pre_render(context))
.await
.context("Failed to render post-message")?;
+ info!(
+ context,
+ "post-message pre_render took {:?}",
+ start.elapsed()
+ );
+ let start = Instant::now();
let rendered_msg = mimefactory::render_queued_mail(
queued_msg,
&public_key,
@@ -2792,12 +2799,20 @@ async fn render_mime_message_and_pre_message(
timestamp,
side_effects,
)?;
+ info!(
+ context,
+ "post-message render_queued_mail took {:?}",
+ start.elapsed()
+ );
let mut mimefactory_pre_msg = mimefactory;
mimefactory_pre_msg.set_as_pre_message_for(&rendered_msg);
+ let start = Instant::now();
let (queued_pre_msg, pre_side_effects) = Box::pin(mimefactory_pre_msg.pre_render(context))
.await
.context("pre-message failed to render")?;
+ info!(context, "pre-message pre_render took {:?}", start.elapsed());
+ let start = Instant::now();
let rendered_pre_msg = mimefactory::render_queued_mail(
queued_pre_msg,
&public_key,
@@ -2806,6 +2821,11 @@ async fn render_mime_message_and_pre_message(
timestamp,
pre_side_effects,
)?;
+ info!(
+ context,
+ "pre-message render_queued_mail took {:?}",
+ start.elapsed()
+ );
if rendered_pre_msg.message.len() > PRE_MSG_SIZE_WARNING_THRESHOLD {
warn!(
@@ -2818,7 +2838,10 @@ async fn render_mime_message_and_pre_message(
Ok((Some(rendered_pre_msg), rendered_msg))
} else {
+ let start = Instant::now();
let (queued_msg, side_effects) = Box::pin(mimefactory.pre_render(context)).await?;
+ info!(context, "pre_render took {:?}", start.elapsed());
+ let start = Instant::now();
let rendered_msg = mimefactory::render_queued_mail(
queued_msg,
&public_key,
@@ -2827,6 +2850,7 @@ async fn render_mime_message_and_pre_message(
timestamp,
side_effects,
)?;
+ info!(context, "render_queued_mail took {:?}", start.elapsed());
Ok((None, rendered_msg))
} |
| let (queued_mail, side_effects) = Box::pin(self.pre_render(context)).await?; | ||
| let rendered_mail = render_queued_mail( | ||
| queued_mail, | ||
| &public_key, | ||
| &secret_key, | ||
| from_addr, | ||
| timestamp, | ||
| side_effects, | ||
| )?; |
There was a problem hiding this comment.
render_queued_mail() is a blocking function (possibly blocking for multiple seconds) that is called without spawn_blocking() or block_in_place() both here and in multiple other places. This needs to be fixed in order not to block the executor
There was a problem hiding this comment.
Indeed, spawn_blocking() was removed from pk_encrypt(). I think, to protect from blocking the executor in such cases, we should use block_in_place() at the lower level, i.e. in pk_encrypt() while still calling spawn_blocking() here and in other high-level functions. So, even if spawn_blocking() is forgotten, the worst thing is other futures in the same task getting blocked, not other tasks. Nested block_in_place() should be fine (no-op), see discussion in tokio-rs/tokio#2327
There was a problem hiding this comment.
I wrapped calls into block_in_place() and added a note to the documentation of render_queued_mail.
There was a problem hiding this comment.
It's better to express this using the type system than the documentation that could be missed by function users, i.e. add some artificial token parameter to render_queued_mail() and wrappers for block_in_place() and spawn_blocking() that generate a token that can be passed into the function. At least this way you can grep all places in the code where such tokens are generated and check that blocking is fine there.
There was a problem hiding this comment.
Moved block_in_place() into pk_encrypt().
I don't know how we actually want to handle this, it does not look great to make functions that have nothing to do with async depend on tokio and it is not clear how much CPU usage is considered "blocking". Maybe we should move all block_in_place to lower levels, but then we should also do it for create_keypair, instead of using spawn_blocking in generate_keypair.
|
|
||
| if is_encrypted { | ||
| // Copy not protected headers to outer headers. | ||
| let (parsed_headers, _index) = mailparse::parse_headers(&raw_message)?; |
There was a problem hiding this comment.
FTR, we're first building the message and then parsing it again, but this parse_headers call takes just 5µs-80µs in my measurements (5 year old middle-class phone, release mode), and code-wise it seems like the easiest solution, so, it's good as-is.
There was a problem hiding this comment.
...although the only header we actually need is Chat-Is-Post-Message. For all the others (Subject, To, Chat-Version) it's already clear that they should be added and with which value. So, the code here could be slightly simplified by putting pre_message_mode on QueuedMail, and then always mechanically adding these four outer headers. Then we wouldn't need to parse the message here and iterate over all headers.
But it's fine as-is, too
|
|
||
| if is_encrypted { | ||
| // Copy not protected headers to outer headers. | ||
| let (parsed_headers, _index) = mailparse::parse_headers(&raw_message)?; |
There was a problem hiding this comment.
...although the only header we actually need is Chat-Is-Post-Message. For all the others (Subject, To, Chat-Version) it's already clear that they should be added and with which value. So, the code here could be slightly simplified by putting pre_message_mode on QueuedMail, and then always mechanically adding these four outer headers. Then we wouldn't need to parse the message here and iterate over all headers.
But it's fine as-is, too
| } else { | ||
| SeipdVersion::V1 | ||
| }; | ||
| let display_name = if is_securejoin_message && !is_encrypted { |
There was a problem hiding this comment.
IIUC, the naming of will_be_encrypted is correct. If the message is symmetrically-encrypted, then encryption_pubkeys is Some(vec![]). As @link2xt said elsewhere, would be a nice refactoring to put this into an enum instead in order to prevent such confusion.
The "vc-pubkey" and "vc-request-pubkey" messages don't use the function here. Instead, they are created by render_symm_encrypted_securejoin_message. This is why the code works as correctly. Thanks for writing a test for it!
| @@ -2022,84 +2293,26 @@ struct HeadersByConfidentiality { | |||
| /// See [`HeadersByConfidentiality`] for more info. | |||
| fn group_headers_by_confidentiality( | |||
There was a problem hiding this comment.
Maybe put a comment here that this function isn't really needed anymore and can be removed once we remove hidden headers?
598ac45 to
72d547a
Compare
Could you check if removing compression support from Asymmetric encryption is constant-time and should not depend on the message size, and symmetric AES encryption is supposed to be cheap and not taking seconds for 10 MB message, so I suspect the slowest part is OpenPGP compression. If it is that slow, it's one more reason to get rid of it. For encrypted attachments we don't need to worry about compatibility, so can even send them as binary MIME without base64-encoding, this will also reduce memory usage for the receivers and may help iOS. Large attachments are likely don't compress well as videos and images are already compressed, webxdcs are zip archives, and zlib compression cannot compress them further, so even just dropping compression already might be an improvement. The only reason for OpenPGP compression currently is compensating base64-encoding of binary attachments. Attachments are base64-encoded, then compressed, then signed and encrypted, then the whole encrypted messages is ASCII-armored. Compression negates the first base64-encoding, but we don't have to do it in the first place, mailparse at least theoretically supports binary attachments. |
|
Removing compression only helped a little bit, reducing the time from 3.5s to 2.6s. Additionally Ordering Rust to optimize for speed rather than binary size gets it down to 1.6s (I did not check how this affects the APK size). |
de45e20 to
95aba6d
Compare
af647e7 to
5eb92c2
Compare
…able message This change separates rendering into two separate steps: 1. Rendering of the message payload without the From, Date and Autocrypt headers. 2. Adding the From, Date and Autocrypt headers and possibly encrypting the message. The goal is to have serializable result of the first step that can be persisted in the database and sent later with any email address. This way it will be possible to send queued messages over any relay. This will make it possible not to remove all messages from the queue when the sending relay is changed. Currently changing `configured_addr` deletes everything from `smtp` table. This change is however only a refactoring and does not implement any features.
5eb92c2 to
eef6ea4
Compare
5 participants
This change separates rendering into two separate steps:
The goal is to have serializable result of the first step
that can be persisted in the database and sent later with any email address.
This way it will be possible to send queued messages over any relay.
This will make it possible not to remove all messages from the queue
when the sending relay is changed.
Currently changing
configured_addrdeletes everything fromsmtptable.This change is however only a refactoring and does not implement any features.
This is a refactoring PR in preparation for automatic relay failover.
As a side effect it also makes possible to change the Date of the message for #8112 if we decide on this approach (unlikely).
Serializable mail is currently called
mimefactory::QueuedMail. Everything except the public keys is trivially serializable, public keys should likely be serialized as recipient fingerprints rather than as OpenPGP certificates.I also noticed that we likely can remove the concept of "hidden headers" which are headers that are sent on the mulipart/mixed level of unencrypted messages. They are used to send
Chat-Editheaders and avatars in unencrypted messages. Sending avatars in unencrypted messages is not useful because they are not displayed anyway. And we can decide to make it impossible to edit and delete unencrypted messages. I have not changed anything in this PR, however, hidden headers work as before.