diff --git a/python/tests/test_0_complex_or_slow.py b/python/tests/test_0_complex_or_slow.py index 276c4c46e..0a347e18e 100644 --- a/python/tests/test_0_complex_or_slow.py +++ b/python/tests/test_0_complex_or_slow.py @@ -16,8 +16,6 @@ class TestGroupStressTests: lp.sec("ac1: send message to new group chat") msg1 = chat.send_text("hello") assert msg1.is_encrypted() - gossiped_timestamp = chat.get_summary()["gossiped_timestamp"] - assert gossiped_timestamp > 0 assert chat.num_contacts() == 3 + 1 @@ -46,19 +44,13 @@ class TestGroupStressTests: assert to_remove.addr in sysmsg.text assert sysmsg.chat.num_contacts() == 3 - # Receiving message about removed contact does not reset gossip - assert chat.get_summary()["gossiped_timestamp"] == gossiped_timestamp - lp.sec("ac1: sending another message to the chat") chat.send_text("hello2") msg = ac2._evtracker.wait_next_incoming_message() assert msg.text == "hello2" - assert chat.get_summary()["gossiped_timestamp"] == gossiped_timestamp lp.sec("ac1: adding fifth member to the chat") chat.add_contact(ac5) - # Adding contact to chat resets gossiped_timestamp - assert chat.get_summary()["gossiped_timestamp"] >= gossiped_timestamp lp.sec("ac2: receiving system message about contact addition") sysmsg = ac2._evtracker.wait_next_incoming_message() diff --git a/python/tests/test_1_online.py b/python/tests/test_1_online.py index d5f8b75fc..48f88d0ca 100644 --- a/python/tests/test_1_online.py +++ b/python/tests/test_1_online.py @@ -798,12 +798,6 @@ def test_send_and_receive_will_encrypt_decrypt(acfactory, lp): msg3.mark_seen() assert not list(ac1.get_fresh_messages()) - # Test that we do not gossip peer keys in 1-to-1 chat, - # as it makes no sense to gossip to peers their own keys. - # Gossip is only sent in encrypted messages, - # and we sent encrypted msg_back right above. - assert chat2b.get_summary()["gossiped_timestamp"] == 0 - lp.sec("create group chat with two members, one of which has no encrypt state") chat = ac1.create_group_chat("encryption test") chat.add_contact(ac2) @@ -813,41 +807,6 @@ def test_send_and_receive_will_encrypt_decrypt(acfactory, lp): ac1._evtracker.get_matching("DC_EVENT_SMTP_MESSAGE_SENT") -def test_gossip_optimization(acfactory, lp): - """Test that gossip timestamp is updated when someone else sends gossip, - so we don't have to send gossip ourselves. - """ - ac1, ac2, ac3 = acfactory.get_online_accounts(3) - - acfactory.introduce_each_other([ac1, ac2]) - acfactory.introduce_each_other([ac2, ac3]) - - lp.sec("ac1 creates a group chat with ac2") - group_chat = ac1.create_group_chat("hello") - group_chat.add_contact(ac2) - msg = group_chat.send_text("hi") - - # No Autocrypt gossip was sent yet. - gossiped_timestamp = msg.chat.get_summary()["gossiped_timestamp"] - assert gossiped_timestamp == 0 - - msg = ac2._evtracker.wait_next_incoming_message() - assert msg.is_encrypted() - assert msg.text == "hi" - - lp.sec("ac2 adds ac3 to the group") - msg.chat.add_contact(ac3) - - lp.sec("ac1 receives message from ac2 and updates gossip timestamp") - msg = ac1._evtracker.wait_next_incoming_message() - assert msg.is_encrypted() - - # ac1 has updated the gossip timestamp even though no gossip was sent by ac1. - # ac1 does not need to send gossip because ac2 already did it. - gossiped_timestamp = msg.chat.get_summary()["gossiped_timestamp"] - assert gossiped_timestamp == int(msg.time_sent.timestamp()) - - def test_send_first_message_as_long_unicode_with_cr(acfactory, lp): ac1, ac2 = acfactory.get_online_accounts(2) diff --git a/src/chat.rs b/src/chat.rs index 67de10669..40ec59f18 100644 --- a/src/chat.rs +++ b/src/chat.rs @@ -1376,41 +1376,10 @@ impl ChatId { } pub(crate) async fn reset_gossiped_timestamp(self, context: &Context) -> Result<()> { - self.set_gossiped_timestamp(context, 0).await - } - - /// Get timestamp of the last gossip sent in the chat. - /// Zero return value means that gossip was never sent. - pub async fn get_gossiped_timestamp(self, context: &Context) -> Result { - let timestamp: Option = context - .sql - .query_get_value("SELECT gossiped_timestamp FROM chats WHERE id=?;", (self,)) - .await?; - Ok(timestamp.unwrap_or_default()) - } - - pub(crate) async fn set_gossiped_timestamp( - self, - context: &Context, - timestamp: i64, - ) -> Result<()> { - ensure!( - !self.is_special(), - "can not set gossiped timestamp for special chats" - ); - info!( - context, - "Set gossiped_timestamp for chat {} to {}.", self, timestamp, - ); - context .sql - .execute( - "UPDATE chats SET gossiped_timestamp=? WHERE id=?;", - (timestamp, self), - ) + .execute("DELETE FROM gossip_timestamp WHERE chat_id=?", (self,)) .await?; - Ok(()) } @@ -1917,7 +1886,6 @@ impl Chat { name: self.name.clone(), archived: self.visibility == ChatVisibility::Archived, param: self.param.to_string(), - gossiped_timestamp: self.id.get_gossiped_timestamp(context).await?, is_sending_locations: self.is_sending_locations, color: self.get_color(context).await?, profile_image: self @@ -2460,9 +2428,6 @@ pub struct ChatInfo { /// This is the string-serialised version of `Params` currently. pub param: String, - /// Last time this client sent autocrypt gossip headers to this chat. - pub gossiped_timestamp: i64, - /// Whether this chat is currently sending location-stream messages. pub is_sending_locations: bool, @@ -3101,10 +3066,6 @@ pub(crate) async fn create_send_msg_jobs(context: &Context, msg: &mut Message) - let now = smeared_time(context); - if rendered_msg.is_gossiped { - msg.chat_id.set_gossiped_timestamp(context, now).await?; - } - if rendered_msg.last_added_location_id.is_some() { if let Err(err) = location::set_kml_sent_timestamp(context, msg.chat_id, now).await { error!(context, "Failed to set kml sent_timestamp: {err:#}."); diff --git a/src/mimefactory.rs b/src/mimefactory.rs index bad271494..af3596c05 100644 --- a/src/mimefactory.rs +++ b/src/mimefactory.rs @@ -13,6 +13,7 @@ use mail_builder::headers::HeaderType; use mail_builder::mime::MimePart; use tokio::fs; +use crate::aheader::{Aheader, EncryptPreference}; use crate::blob::BlobObject; use crate::chat::{self, Chat}; use crate::config::Config; @@ -22,6 +23,7 @@ use crate::contact::{Contact, ContactId, Origin}; use crate::context::Context; use crate::e2ee::EncryptHelper; use crate::ephemeral::Timer as EphemeralTimer; +use crate::key::DcKey; use crate::location; use crate::message::{self, Message, MsgId, Viewtype}; use crate::mimeparser::{is_hidden, SystemMessage}; @@ -131,7 +133,6 @@ pub struct RenderedEmail { pub message: String, // pub envelope: Envelope, pub is_encrypted: bool, - pub is_gossiped: bool, pub last_added_location_id: Option, /// A comma-separated string of sync-IDs that are used by the rendered email and must be deleted @@ -433,41 +434,6 @@ impl MimeFactory { } } - async fn should_do_gossip(&self, context: &Context, multiple_recipients: bool) -> Result { - match &self.loaded { - Loaded::Message { chat, msg } => { - if chat.typ == Chattype::Broadcast { - // Never send Autocrypt-Gossip in broadcast lists - // as it discloses recipient email addresses. - return Ok(false); - } - - let cmd = msg.param.get_cmd(); - if cmd == SystemMessage::MemberAddedToGroup - || cmd == SystemMessage::SecurejoinMessage - { - Ok(true) - } else if multiple_recipients { - // beside key- and member-changes, force a periodic re-gossip. - let gossiped_timestamp = chat.id.get_gossiped_timestamp(context).await?; - let gossip_period = context.get_config_i64(Config::GossipPeriod).await?; - // `gossip_period == 0` is a special case for testing, - // enabling gossip in every message. - // Otherwise "smeared timestamps" may result in the condition - // to fail even if the clock is monotonic. - if gossip_period == 0 || time() >= gossiped_timestamp + gossip_period { - Ok(true) - } else { - Ok(false) - } - } else { - Ok(false) - } - } - Loaded::Mdn { .. } => Ok(false), - } - } - fn should_attach_profile_data(msg: &Message) -> bool { msg.param.get_cmd() != SystemMessage::SecurejoinMessage || { let step = msg.param.get(Param::Arg).unwrap_or_default(); @@ -787,8 +753,6 @@ impl MimeFactory { } } - let mut is_gossiped = false; - let peerstates = self.peerstates_for_recipients(context).await?; let is_encrypted = !self.should_force_plaintext() && (e2ee_guaranteed || encrypt_helper.should_encrypt(context, &peerstates).await?); @@ -956,16 +920,78 @@ impl MimeFactory { // Add gossip headers in chats with multiple recipients let multiple_recipients = peerstates.len() > 1 || context.get_config_bool(Config::BccSelf).await?; - if self.should_do_gossip(context, multiple_recipients).await? { - for peerstate in peerstates.iter().filter_map(|(state, _)| state.as_ref()) { - if let Some(header) = peerstate.render_gossip_header(verified) { - message = message.header( - "Autocrypt-Gossip", - mail_builder::headers::raw::Raw::new(header), - ); - is_gossiped = true; + + let gossip_period = context.get_config_i64(Config::GossipPeriod).await?; + let now = time(); + + match &self.loaded { + Loaded::Message { chat, msg } => { + if chat.typ != Chattype::Broadcast { + for peerstate in peerstates.iter().filter_map(|(state, _)| state.as_ref()) { + let Some(key) = peerstate.peek_key(verified) else { + continue; + }; + + let fingerprint = key.dc_fingerprint().hex(); + let cmd = msg.param.get_cmd(); + let should_do_gossip = cmd == SystemMessage::MemberAddedToGroup + || cmd == SystemMessage::SecurejoinMessage + || multiple_recipients && { + let gossiped_timestamp: Option = context + .sql + .query_get_value( + "SELECT timestamp + FROM gossip_timestamp + WHERE chat_id=? AND fingerprint=?", + (chat.id, &fingerprint), + ) + .await?; + + // `gossip_period == 0` is a special case for testing, + // enabling gossip in every message. + // + // If current time is in the past compared to + // `gossiped_timestamp`, we also gossip because + // either the `gossiped_timestamp` or clock is wrong. + gossip_period == 0 + || gossiped_timestamp + .is_none_or(|ts| now >= ts + gossip_period || now < ts) + }; + + if !should_do_gossip { + continue; + } + + let header = Aheader::new( + peerstate.addr.clone(), + key.clone(), + // Autocrypt 1.1.0 specification says that + // `prefer-encrypt` attribute SHOULD NOT be included. + EncryptPreference::NoPreference, + ) + .to_string(); + + message = message.header( + "Autocrypt-Gossip", + mail_builder::headers::raw::Raw::new(header), + ); + + context + .sql + .execute( + "INSERT INTO gossip_timestamp (chat_id, fingerprint, timestamp) + VALUES (?, ?, ?) + ON CONFLICT (chat_id, fingerprint) + DO UPDATE SET timestamp=excluded.timestamp", + (chat.id, &fingerprint, now), + ) + .await?; + } } } + Loaded::Mdn { .. } => { + // Never gossip in MDNs. + } } // Set the appropriate Content-Type for the inner message. @@ -1109,7 +1135,6 @@ impl MimeFactory { message, // envelope: Envelope::new, is_encrypted, - is_gossiped, last_added_location_id, sync_ids_to_delete: self.sync_ids_to_delete, rfc724_mid, diff --git a/src/peerstate.rs b/src/peerstate.rs index d0074e5fe..daef7bead 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -405,28 +405,6 @@ impl Peerstate { }; } - /// Returns the contents of the `Autocrypt-Gossip` header for outgoing messages. - pub fn render_gossip_header(&self, verified: bool) -> Option { - if let Some(key) = self.peek_key(verified) { - let header = Aheader::new( - self.addr.clone(), - key.clone(), // TODO: avoid cloning - // Autocrypt 1.1.0 specification says that - // `prefer-encrypt` attribute SHOULD NOT be included, - // but we include it anyway to propagate encryption - // preference to new members in group chats. - if self.last_seen_autocrypt > 0 { - self.prefer_encrypt - } else { - EncryptPreference::NoPreference - }, - ); - Some(header.to_string()) - } else { - None - } - } - /// Converts the peerstate into the contact public key. /// /// Similar to [`Self::peek_key`], but consumes the peerstate and returns owned key. diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 0e4983ea7..0d9646688 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -24,6 +24,7 @@ use crate::ephemeral::{stock_ephemeral_timer_changed, Timer as EphemeralTimer}; use crate::events::EventType; use crate::headerdef::{HeaderDef, HeaderDefMap}; use crate::imap::{markseen_on_imap_table, GENERATED_PREFIX}; +use crate::key::DcKey; use crate::log::LogExt; use crate::message::{ self, rfc724_mid_exists, Message, MessageState, MessengerMessage, MsgId, Viewtype, @@ -452,22 +453,25 @@ pub(crate) async fn receive_imf_inner( } // Update gossiped timestamp for the chat if someone else or our other device sent - // Autocrypt-Gossip for all recipients in the chat to avoid sending Autocrypt-Gossip ourselves + // Autocrypt-Gossip header to avoid sending Autocrypt-Gossip ourselves // and waste traffic. let chat_id = received_msg.chat_id; - if !chat_id.is_special() - && mime_parser.recipients.iter().all(|recipient| { - recipient.addr == mime_parser.from.addr - || mime_parser.gossiped_keys.contains_key(&recipient.addr) - }) - { - info!( - context, - "Received message contains Autocrypt-Gossip for all members of {chat_id}, updating timestamp." - ); - if chat_id.get_gossiped_timestamp(context).await? < mime_parser.timestamp_sent { - chat_id - .set_gossiped_timestamp(context, mime_parser.timestamp_sent) + if !chat_id.is_special() { + for gossiped_key in mime_parser.gossiped_keys.values() { + context + .sql + .transaction(move |transaction| { + let fingerprint = gossiped_key.dc_fingerprint().hex(); + transaction.execute( + "INSERT INTO gossip_timestamp (chat_id, fingerprint, timestamp) + VALUES (?, ?, ?) + ON CONFLICT (chat_id, fingerprint) + DO UPDATE SET timestamp=MAX(timestamp, excluded.timestamp)", + (chat_id, &fingerprint, mime_parser.timestamp_sent), + )?; + + Ok(()) + }) .await?; } } diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 2532d68ad..428a5d345 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -1169,6 +1169,23 @@ CREATE INDEX msgs_status_updates_index2 ON msgs_status_updates (uid); .await?; } + inc_and_check(&mut migration_version, 130)?; + if dbversion < migration_version { + sql.execute_migration( + " +CREATE TABLE gossip_timestamp ( + chat_id INTEGER NOT NULL, + fingerprint TEXT NOT NULL, -- Upper-case fingerprint of the key. + timestamp INTEGER NOT NULL, + UNIQUE (chat_id, fingerprint) +) STRICT; +CREATE INDEX gossip_timestamp_index ON gossip_timestamp (chat_id, fingerprint); +", + migration_version, + ) + .await?; + } + let new_version = sql .get_raw_config_int(VERSION_CFG) .await? diff --git a/src/sql/tables.sql b/src/sql/tables.sql index 6e5c7f255..30317ead1 100644 --- a/src/sql/tables.sql +++ b/src/sql/tables.sql @@ -32,7 +32,7 @@ CREATE TABLE chats ( grpid TEXT DEFAULT '', param TEXT DEFAULT '', archived INTEGER DEFAULT 0, - gossiped_timestamp INTEGER DEFAULT 0, + gossiped_timestamp INTEGER DEFAULT 0, -- deprecated 2025-04-08, replaced with gossiped_timestamp table locations_send_begin INTEGER DEFAULT 0, locations_send_until INTEGER DEFAULT 0, locations_last_sent INTEGER DEFAULT 0,