introduce asymetrically encrypted keyupdate messages informing contacts about relay changes - #8621
Conversation
baef67a to
bc8d4b1
Compare
bc8d4b1 to
941ad1d
Compare
941ad1d to
795ab48
Compare
link2xt
left a comment
link2xt
left a comment
There was a problem hiding this comment.
I still have not finished reading it to the end, especially the tests.
Largest comment is that key updates are not generated on the fly and are not actually low priority. With an actually low priority queue maybe a KeyupdateDebounce would not have even been needed. Not suggesting to change this, i think it is better to merge it sooner without large changes.
| ) | ||
| .await?; | ||
|
|
||
| let msg_id = message::insert_tombstone(context, &rfc724_mid).await?; |
There was a problem hiding this comment.
For other reviewers: insert_into_smtp now creates a tombstone on its own.
| // The address only serves as a fallback for keys advertising no relays, | ||
| // and does not travel beyond the SMTP envelope. | ||
| let mut relays = relay_addrs(&public_key, &addr); | ||
| relays.retain(|relay| !relay.is_empty()); |
There was a problem hiding this comment.
Should this be a debug_assert!(relays.iter().all(|s| !s.is_empty())? Maybe even in relay_addrs, to make sure it never returns invalid addresses. How does it happen that we get an empty relay address in this vector?
I see addresses_from_public_key may return a single empty relay because of how split works, but only if someone sends a key with a notation subpacket and no relays, should not happen normally.
There was a problem hiding this comment.
contacts.addr is empty for fingerprint-only contacts so moving the assert does not work i think.
add_or_lookup_ext allows such contacts
Lines 948 to 951 in 26923f9
There was a problem hiding this comment.
I commented at https://github.com/chatmail/core/pull/8620/changes#r3874278090, i think relay_addrs should then make sure to filter out empty addr. And here can be a debug assert.
795ab48 to
26923f9
Compare
Yes, in hindsight a new |
26923f9 to
6ae1fad
Compare
With autocrypt2 each recipient will add 1 kB ("32-octet X25519 ciphertext, 1088-octet ML-KEM-768 ciphertext" according to https://www.rfc-editor.org/info/rfc9980/#section-11-7.12). 200 kB of PKESK packets looks fine. |
link2xt
left a comment
link2xt
left a comment
There was a problem hiding this comment.
I have read everything, most important points:
- Do we need to store the baseline, can we simply store the time of the relay change (which is currently stored only in memory) and send key updates if it is in the past? It looks like baseline and comparing relay list to it was meant to help with complex scenarios, but if it does not really work with multi-device case anyway, it only protects against the case when the user reverts the change quickly on the same device, adding and immediately removing the relay. If we store the timestamp (that is currently stored in memory) and remove the baseline, then we also don't need
schedule_keyupdate_checkin the beginning of SMTP loop. - Comment about the SQL statement and the test
test_keyupdate_from_unknown_sender, maybe not send key updates simply because we have created a chat with a contact that we never sent anything to?
| .store(true, Ordering::Relaxed); | ||
| context.emit_event(EventType::TransportsModified); | ||
|
|
||
| // Ingesting devices only record the baseline, see `keyupdate.rs`. |
There was a problem hiding this comment.
Repeating my comment from other places, keyupdate.rs is huge and this kind of references often bitrot, we also had "see receive_imf" in the past and then nobody could recall what the comment referred to so it was deleted.
This sentence mostly says what happens on the next line if i understand it correctly ("ingesting devices" meaning devices that receive/ingest the sync message).
There was a problem hiding this comment.
i dropped two references to keyupdate.rs in favor of a direct explanation, but not sure what you mean with "huge"?
keyupdate.rs has 244 LOCs and receive_imf is 4350 LOCs.
You probably mean that keyupdate.rs has a 64 LOC module doc, but i do think we need a place where everything related to keyupdates is documented. Sprinkling some references to the module doc string would help so anyone changing code considers looking up the documented context.
FWIW ephemeral.rs has a 62 LOC doc string, and i think it's helpful, and i think rather more than less modules should have that.
| context.emit_event(EventType::TransportsModified); | ||
|
|
||
| // Ingesting devices only record the baseline, see `keyupdate.rs`. | ||
| // Acceptable gap: with concurrent changes on two devices, |
There was a problem hiding this comment.
So the scenario is:
- Device 1 and Device 2 have relay A.
- Device 1 adds relay B and sends key updates.
- Device 2 adds relay C and sends key updates.
Then both devices receive sync messages and have relays A, B and C, but all key updates sent have either relays A,B or relays A,C.
There was a problem hiding this comment.
yes, that is correct. I improved the docstring. A user doing concurrent multi-relay changes on two devices in the same 30 seconds is probably rare (for now). If it happens, then whatever contacts take as the current relay set, should not be wrong, and any chatting and read receipts would heal it, because both devices have a merged set. I don't think it's worth to do much on it, or document much more about this concurrent multi-device changes for now.
| // Ingesting devices only record the baseline, see `keyupdate.rs`. | ||
| // Acceptable gap: with concurrent changes on two devices, | ||
| // contacts may only learn the merged list with the next relay change or regular message. | ||
| set_current_relays_as_keyupdate_baseline(context).await?; |
There was a problem hiding this comment.
What changes if we don't do this? Key updates are only sent when we set keyupdate_check_deadline and this only happens when relays are changed on our device (not via sync, manually or by automatic relay management (to be renamed into "autorelay" if #8623 is merged). Is this only to avoid sending key updates in the case where user adds a relay and quickly deletes it back?
I wonder if we actually need to store baseline at all since it does not help with merging, can we just store the time we have last changed the relays on our device and if we did it more than debounce (30) seconds away or if this time is in the future (something happened to the clock), send key updates and reset it to 0.
There was a problem hiding this comment.
If we don't set the baseline, this fails:
core/src/keyupdate/keyupdate_tests.rs
Line 285 in 6ae1fad
The second device wrongly sends the same keyupdate out, so every synced device would duplicate keyupdate sending.
The deadline is not only set by local relay changes. sync_transports sets restart_io_after_fetch (transport.rs:681), and the restarting SMTP loop calls schedule_keyupdate_check.
And no, it is not only about adding a relay and quickly removing it again. That case is handled on the sender side in maybe_send_keyupdate_message. Here, setting the baseline is about second devices receiving the sync message. Both go through the baseline, which is probably why they look like one mechanism.
Under your timestamp suggestion, the line could indeed go, though.
There was a problem hiding this comment.
- Do we need to store the baseline, can we simply store the time of the relay change (which is currently stored only in memory) and send key updates if it is in the past? It looks like baseline and comparing relay list to it was meant to help with complex scenarios, but if it does not really work with multi-device case anyway, it only protects against the case when the user reverts the change quickly on the same device, adding and immediately removing the relay. If we store the timestamp (that is currently stored in memory) and remove the baseline, then we also don't need schedule_keyupdate_check in the beginning of SMTP loop.
I find KeyupdateBaseline and how our current relay setup diverges from it easier to reason about than a "send-keyupdate-deadline" where we would need to make sure that all call paths (now and in the future) triggering it would not cause unnecessary updates.
- Comment about the SQL statement and the test test_keyupdate_from_unknown_sender, maybe not send key updates simply because we have created a chat with a contact that we never sent anything to?
The test you refer to is about receiving, not sending. But i did a commit filtering out contacts with empty single chats only, where we never sent or received anything. a603c5a
| // Ingesting devices only record the baseline, see `keyupdate.rs`. | ||
| // Acceptable gap: with concurrent changes on two devices, | ||
| // contacts may only learn the merged list with the next relay change or regular message. | ||
| set_current_relays_as_keyupdate_baseline(context).await?; |
There was a problem hiding this comment.
If we don't set the baseline, this fails:
core/src/keyupdate/keyupdate_tests.rs
Line 285 in 6ae1fad
The second device wrongly sends the same keyupdate out, so every synced device would duplicate keyupdate sending.
The deadline is not only set by local relay changes. sync_transports sets restart_io_after_fetch (transport.rs:681), and the restarting SMTP loop calls schedule_keyupdate_check.
And no, it is not only about adding a relay and quickly removing it again. That case is handled on the sender side in maybe_send_keyupdate_message. Here, setting the baseline is about second devices receiving the sync message. Both go through the baseline, which is probably why they look like one mechanism.
Under your timestamp suggestion, the line could indeed go, though.
| .store(true, Ordering::Relaxed); | ||
| context.emit_event(EventType::TransportsModified); | ||
|
|
||
| // Ingesting devices only record the baseline, see `keyupdate.rs`. |
There was a problem hiding this comment.
i dropped two references to keyupdate.rs in favor of a direct explanation, but not sure what you mean with "huge"?
keyupdate.rs has 244 LOCs and receive_imf is 4350 LOCs.
You probably mean that keyupdate.rs has a 64 LOC module doc, but i do think we need a place where everything related to keyupdates is documented. Sprinkling some references to the module doc string would help so anyone changing code considers looking up the documented context.
FWIW ephemeral.rs has a 62 LOC doc string, and i think it's helpful, and i think rather more than less modules should have that.
| //! Contacts learn our relay list from messages carrying our key, | ||
| //! but without transport changes causing keyupdates the next message | ||
| //! of a "mutually silent" contact might go to relays that we no longer read. | ||
| //! Keyupdates can help speed up recovery during wider-scale network degradation | ||
| //! which typically leads to attempts to change a profile's relay setup. |
There was a problem hiding this comment.
To make this more understandable:
| //! Contacts learn our relay list from messages carrying our key, | |
| //! but without transport changes causing keyupdates the next message | |
| //! of a "mutually silent" contact might go to relays that we no longer read. | |
| //! Keyupdates can help speed up recovery during wider-scale network degradation | |
| //! which typically leads to attempts to change a profile's relay setup. | |
| //! The public key contains the list of relays, | |
| //! so that the list of relays is distributed to our contacts | |
| //! whenever our key is attached or gossiped. | |
| //! On relay changes, we send proactive keyupdate messages | |
| //! to inform our contacts about the new relay list, | |
| //! in order to retain connectivity after all our relays changed. | |
| //! This could happen e.g. in these cases: | |
| //! - The user played around with the relay settings, | |
| //! and replaced all existing relays. | |
| //! - After a wide-scale network degradation, | |
| //! all the previous relays stopped working and new ones were added. |
There was a problem hiding this comment.
see 8d3820a where i cut the docstring to <50% -- i'd kindly ask both you and link2xt to rather do a PR against the docs if you see any more needs, instead of us litigating comments and docs here further.
| //! Keyupdates are therefore *unsigned*: | ||
| //! | ||
| //! - Receivers generally apply the key while parsing the Autocrypt header | ||
| //! (`mimeparser` imports and merges it before looking at any signature), | ||
| //! so an older core that never heard of keyupdates still learns the update. | ||
| //! | ||
| //! - An unsigned message does not count as encrypted for `receive_imf`, | ||
| //! which drops it if `Config::ForceEncryption` is set (default) | ||
| //! before a contact, chat or a `last_seen` update can come out of it. | ||
| //! Cores with keyupdates trash it even earlier as a report naming no message, | ||
| //! and cores with encryption enforcement turned off still trash it, | ||
| //! because a keyupdate is a `multipart/report` (`mimefactory::keyupdate_body`) | ||
| //! which lacks an `Original-Message-ID`. |
There was a problem hiding this comment.
| //! Keyupdates are therefore *unsigned*: | |
| //! | |
| //! - Receivers generally apply the key while parsing the Autocrypt header | |
| //! (`mimeparser` imports and merges it before looking at any signature), | |
| //! so an older core that never heard of keyupdates still learns the update. | |
| //! | |
| //! - An unsigned message does not count as encrypted for `receive_imf`, | |
| //! which drops it if `Config::ForceEncryption` is set (default) | |
| //! before a contact, chat or a `last_seen` update can come out of it. | |
| //! Cores with keyupdates trash it even earlier as a report naming no message, | |
| //! and cores with encryption enforcement turned off still trash it, | |
| //! because a keyupdate is a `multipart/report` (`mimefactory::keyupdate_body`) | |
| //! which lacks an `Original-Message-ID`. | |
| //! Keyupdates are therefore *unsigned*. | |
| //! On the receiver's side, they key is applied | |
| //! while parsing the Autocrypt header and before the signature is checked, | |
| //! and the message is trashed later. |
- Not necessary to explain the details of receive_imf; if the
receive_imflogic changes, we won't remember to update the explanation here - Not necessary to explain how old cores handle this, this info will become irrelevant in 6-12 months
- Removed the word "generally", not sure why it was there
There was a problem hiding this comment.
your suggestion drops the fact that keyupdates are sent as MDN without referenced message id.
Also, i use function references to make the docs lines show up if any of the functions is moved or renamed, because usually this happens via "grep" for it or equivalent IDE tooling. It's how i did it a lot in other projects.
But i won't argue this here, after a lot of discussion about comments with link2xt already.
I cut the module docstring in half 8d3820a
| //! debouncing several changes into a single keyupdate message. | ||
| //! | ||
| //! - The SMTP loop sends once the deadline passed and its queue is drained, | ||
| //! so real messages are never delayed and a keyupdate is only attempted |
There was a problem hiding this comment.
| //! so real messages are never delayed and a keyupdate is only attempted | |
| //! so real messages are not delayed and a keyupdate is only attempted |
Hocuri
left a comment
Hocuri
left a comment
There was a problem hiding this comment.
I'm almost done with reviewing.
I tested this on my main account. Overall, it went well; the only problem is that apparently, testrun.org rejected a full message because one of the recipients in there does not exist anymore:
08-29 16:59:21.749 5737 5766 I DeltaChat: [accId=2] src/smtp.rs:206: SMTP failed to send: Permanent(Response { code: Code { severity: PermanentNegativeCompletion, category: MailSystem, detail: Zero }, message: ["5.1.1 <xstore@testrun.org>: Recipient address rejected: User unknown in virtual mailbox table"] }).
08-29 16:59:21.749 5737 5766 I DeltaChat: [accId=2] src/smtp.rs:239: Permanent error, message sending failed.
08-29 16:59:21.749 5737 5766 I DeltaChat: [accId=2] src/smtp.rs:282: Failed to send message over SMTP, disconnecting.
08-29 16:59:21.749 5737 5766 E DeltaChat: [accId=2] Failed to load Msg#319115 to mark it as failed: Message Msg#319115 does not exist.
08-29 16:59:21.749 5737 5766 E DeltaChat: Failed to load Msg#319115 to mark it as failed: Message Msg#319115 does not exist.
08-29 16:59:22.065 5737 5766 W DeltaChat: [accId=2] src/scheduler.rs:583: send_smtp_messages failed: Failed to send message: Permanent SMTP error: permanent: 5.1.1 <xstore@testrun.org>: Recipient address rejected: User unknown in virtual mailbox table.
...but we could solve this later, maybe create in issue for it for now.
Apart from that, I got quite some non-delivery-notifications, which is expected and nothing to worry about.
What I also got is 52 "Missing attachment" warnings:
08-29 16:59:09.418 5737 5766 W DeltaChat: [accId=2] src/mimeparser.rs:1371: Missing attachment
The reason for this log are mesages (or message parts) with application/pgp-encrypted and application/octet-stream mimetype that confuse core because it can't find the attachment's filename, but I didn't dig deeper than that.
| MAX(MAX(c.last_seen, | ||
| CASE WHEN ch.type=? THEN 0 ELSE ch.created_timestamp END, | ||
| cc.add_timestamp, | ||
| CASE WHEN ch.type=? THEN IFNULL( | ||
| (SELECT MAX(m.timestamp) FROM msgs m | ||
| WHERE m.chat_id=ch.id AND m.from_id=?), 0) | ||
| ELSE 0 END)) AS freshness |
There was a problem hiding this comment.
It would seem fine to only look at last_seen, and possibly add_timestamp, here. If the contact didn't write any message to us in the last 3 years, then they probably won't anytime soon.
But it's fine as-is, too.
There was a problem hiding this comment.
happy about follow up PRs :)
6aed8b2 to
c32da47
Compare
thanks for testing and reviewing! I filed two related issues, They are not super-high prio because relays work, and people can change to a relay for sending, but obviously good to fix in the not do distant future. |
…nges When the published relay list changes, key-contacts are informed with an unsigned message carrying the re-signed key, encrypted to a chunk of contacts at a time. It is shaped like a receipt notification naming no message, so that cores which know nothing about keyupdates trash it as well. See the src/keyupdate.rs module docs for the design.
`relay_addrs()` fell back to the contact address even when it is empty, which happens for key-contacts created from a sync message or for the self-contact, putting an empty string into the SMTP recipient list.
c32da47 to
6ef0cd1
Compare
3 participants
When the published relay list changes, key-contacts are informed with an unsigned message carrying the re-signed key, encrypted to a chunk of contacts at a time. It is shaped like a receipt notification naming no message, so cores that know nothing about keyupdates trash it as well. For more details see src/keyupdate.rs module docs.
This PR is based on the preparational #8620 and replaces #8588.
It shares some parts with #8601 and addresses review comments there, especially introducing two limits on the keyupdate recipient set computation: an absolute MAX number of recipients, and a 3-year cut of stale contacts (no interaction whtsoever with them since 3 years). While in #8601 an 8-year old profile with 7000 overall contacts, had 2000 keyupdate recipients, this branch here has 1400 recipients. Most profiles probably have well under 100 contacts, and fit into a single keyupdate chunk.