From 2efd0461d1db4ff3867539de7651775faa75d0cd Mon Sep 17 00:00:00 2001 From: link2xt Date: Wed, 1 Nov 2023 12:44:57 +0000 Subject: [PATCH] Revert "fix: add secondary verified key" This reverts commit 5efb100f128809821da99814a14a120f110f64bf. --- deltachat-rpc-client/tests/test_something.py | 1 + src/e2ee.rs | 5 +- src/peerstate.rs | 131 +++++-------------- src/qr.rs | 5 +- src/receive_imf.rs | 46 ++----- src/securejoin.rs | 5 +- src/sql/migrations.rs | 13 -- 7 files changed, 49 insertions(+), 157 deletions(-) diff --git a/deltachat-rpc-client/tests/test_something.py b/deltachat-rpc-client/tests/test_something.py index 7fe8c462ee..717ddc37d5 100644 --- a/deltachat-rpc-client/tests/test_something.py +++ b/deltachat-rpc-client/tests/test_something.py @@ -392,6 +392,7 @@ def test_qr_setup_contact(acfactory) -> None: return +@pytest.mark.xfail() def test_verified_group_recovery(acfactory, rpc) -> None: ac1, ac2, ac3 = acfactory.get_online_accounts(3) diff --git a/src/e2ee.rs b/src/e2ee.rs index 71ae180bef..6a966b488b 100644 --- a/src/e2ee.rs +++ b/src/e2ee.rs @@ -301,11 +301,8 @@ Sent with my Delta Chat Messenger: https://delta.chat"; gossip_key_fingerprint: Some(pub_key.fingerprint()), verified_key: Some(pub_key.clone()), verified_key_fingerprint: Some(pub_key.fingerprint()), - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, }; vec![(Some(peerstate), addr)] } diff --git a/src/peerstate.rs b/src/peerstate.rs index 7c67bcc81d..4efd1f96d7 100644 --- a/src/peerstate.rs +++ b/src/peerstate.rs @@ -1,5 +1,7 @@ //! # [Autocrypt Peer State](https://autocrypt.org/level1.html#peer-state-management) module. +use std::collections::HashSet; + use anyhow::{Context as _, Error, Result}; use num_traits::FromPrimitive; @@ -38,7 +40,7 @@ pub enum PeerstateVerifiedStatus { } /// Peerstate represents the state of an Autocrypt peer. -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq)] pub struct Peerstate { /// E-mail address of the contact. pub addr: String, @@ -80,24 +82,13 @@ pub struct Peerstate { /// Fingerprint of the verified public key. pub verified_key_fingerprint: Option, - /// The address that verified this verified key. - pub verifier: Option, - - /// Secondary public verified key of the contact. - /// It could be a contact gossiped by another verified contact in a shared group - /// or a key that was previously used as a verified key. - pub secondary_verified_key: Option, - - /// Fingerprint of the secondary verified public key. - pub secondary_verified_key_fingerprint: Option, - - /// The address that verified secondary verified key. - pub secondary_verifier: Option, - /// True if it was detected /// that the fingerprint of the key used in chats with /// opportunistic encryption was changed after Peerstate creation. pub fingerprint_changed: bool, + + /// The address that verified this contact + pub verifier: Option, } impl Peerstate { @@ -115,11 +106,8 @@ impl Peerstate { gossip_timestamp: 0, verified_key: None, verified_key_fingerprint: None, - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, } } @@ -144,11 +132,8 @@ impl Peerstate { gossip_timestamp: message_time, verified_key: None, verified_key_fingerprint: None, - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, } } @@ -156,10 +141,7 @@ impl Peerstate { pub async fn from_addr(context: &Context, addr: &str) -> Result> { let query = "SELECT addr, last_seen, last_seen_autocrypt, prefer_encrypted, public_key, \ gossip_timestamp, gossip_key, public_key_fingerprint, gossip_key_fingerprint, \ - verified_key, verified_key_fingerprint, \ - verifier, \ - secondary_verified_key, secondary_verified_key_fingerprint, \ - secondary_verifier \ + verified_key, verified_key_fingerprint, verifier \ FROM acpeerstates \ WHERE addr=? COLLATE NOCASE LIMIT 1;"; Self::from_stmt(context, query, (addr,)).await @@ -172,10 +154,7 @@ impl Peerstate { ) -> Result> { let query = "SELECT addr, last_seen, last_seen_autocrypt, prefer_encrypted, public_key, \ gossip_timestamp, gossip_key, public_key_fingerprint, gossip_key_fingerprint, \ - verified_key, verified_key_fingerprint, \ - verifier, \ - secondary_verified_key, secondary_verified_key_fingerprint, \ - secondary_verifier \ + verified_key, verified_key_fingerprint, verifier \ FROM acpeerstates \ WHERE public_key_fingerprint=? \ OR gossip_key_fingerprint=? \ @@ -195,10 +174,7 @@ impl Peerstate { ) -> Result> { let query = "SELECT addr, last_seen, last_seen_autocrypt, prefer_encrypted, public_key, \ gossip_timestamp, gossip_key, public_key_fingerprint, gossip_key_fingerprint, \ - verified_key, verified_key_fingerprint, \ - verifier, \ - secondary_verified_key, secondary_verified_key_fingerprint, \ - secondary_verifier \ + verified_key, verified_key_fingerprint, verifier \ FROM acpeerstates \ WHERE verified_key_fingerprint=? \ OR addr=? COLLATE NOCASE \ @@ -215,6 +191,11 @@ impl Peerstate { let peerstate = context .sql .query_row_optional(query, params, |row| { + // all the above queries start with this: SELECT + // addr, last_seen, last_seen_autocrypt, prefer_encrypted, + // public_key, gossip_timestamp, gossip_key, public_key_fingerprint, + // gossip_key_fingerprint, verified_key, verified_key_fingerprint + let res = Peerstate { addr: row.get("addr")?, last_seen: row.get("last_seen")?, @@ -249,24 +230,11 @@ impl Peerstate { .map(|s| s.parse::()) .transpose() .unwrap_or_default(), + fingerprint_changed: false, verifier: { let verifier: Option = row.get("verifier")?; - verifier.filter(|s| !s.is_empty()) - }, - secondary_verified_key: row - .get("secondary_verified_key") - .ok() - .and_then(|blob: Vec| SignedPublicKey::from_slice(&blob).ok()), - secondary_verified_key_fingerprint: row - .get::<_, Option>("secondary_verified_key_fingerprint")? - .map(|s| s.parse::()) - .transpose() - .unwrap_or_default(), - secondary_verifier: { - let secondary_verifier: Option = row.get("secondary_verifier")?; - secondary_verifier.filter(|s| !s.is_empty()) + verifier.filter(|verifier| !verifier.is_empty()) }, - fingerprint_changed: false, }; Ok(res) @@ -493,19 +461,6 @@ impl Peerstate { } } - /// Sets current gossiped key as the secondary verified key. - /// - /// If gossiped key is the same as the current verified key, - /// do nothing to avoid overwriting secondary verified key - /// which may be different. - pub fn set_secondary_verified_key_from_gossip(&mut self, verifier: String) { - if self.gossip_key_fingerprint != self.verified_key_fingerprint { - self.secondary_verified_key = self.gossip_key.clone(); - self.secondary_verified_key_fingerprint = self.gossip_key_fingerprint.clone(); - self.secondary_verifier = Some(verifier); - } - } - /// Saves the peerstate to the database. pub async fn save_to_db(&self, sql: &Sql) -> Result<()> { sql.execute( @@ -520,12 +475,9 @@ impl Peerstate { gossip_key_fingerprint, verified_key, verified_key_fingerprint, - verifier, - secondary_verified_key, - secondary_verified_key_fingerprint, - secondary_verifier, - addr) - VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?) + addr, + verifier) + VALUES (?,?,?,?,?,?,?,?,?,?,?,?) ON CONFLICT (addr) DO UPDATE SET last_seen = excluded.last_seen, @@ -538,10 +490,7 @@ impl Peerstate { gossip_key_fingerprint = excluded.gossip_key_fingerprint, verified_key = excluded.verified_key, verified_key_fingerprint = excluded.verified_key_fingerprint, - verifier = excluded.verifier, - secondary_verified_key = excluded.secondary_verified_key, - secondary_verified_key_fingerprint = excluded.secondary_verified_key_fingerprint, - secondary_verifier = excluded.secondary_verifier", + verifier = excluded.verifier", ( self.last_seen, self.last_seen_autocrypt, @@ -553,19 +502,23 @@ impl Peerstate { self.gossip_key_fingerprint.as_ref().map(|fp| fp.hex()), self.verified_key.as_ref().map(|k| k.to_bytes()), self.verified_key_fingerprint.as_ref().map(|fp| fp.hex()), - self.verifier.as_deref().unwrap_or(""), - self.secondary_verified_key.as_ref().map(|k| k.to_bytes()), - self.secondary_verified_key_fingerprint - .as_ref() - .map(|fp| fp.hex()), - self.secondary_verifier.as_deref().unwrap_or(""), &self.addr, + self.verifier.as_deref().unwrap_or(""), ), ) .await?; Ok(()) } + /// Returns true if verified key is contained in the given set of fingerprints. + pub fn has_verified_key(&self, fingerprints: &HashSet) -> bool { + if let Some(vkc) = &self.verified_key_fingerprint { + fingerprints.contains(vkc) && self.verified_key.is_some() + } else { + false + } + } + /// Returns the address that verified the contact pub fn get_verifier(&self) -> Option<&str> { self.verifier.as_deref() @@ -816,11 +769,8 @@ mod tests { gossip_key_fingerprint: Some(pub_key.fingerprint()), verified_key: Some(pub_key.clone()), verified_key_fingerprint: Some(pub_key.fingerprint()), - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, }; assert!( @@ -859,11 +809,8 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, }; assert!( @@ -895,11 +842,8 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, }; assert!( @@ -961,11 +905,8 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, }; peerstate.apply_header(&header, 100); diff --git a/src/qr.rs b/src/qr.rs index ac53815ec0..7d89d3e931 100644 --- a/src/qr.rs +++ b/src/qr.rs @@ -1052,11 +1052,8 @@ mod tests { gossip_key_fingerprint: None, verified_key: None, verified_key_fingerprint: None, - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, }; assert!( peerstate.save_to_db(&ctx.ctx.sql).await.is_ok(), diff --git a/src/receive_imf.rs b/src/receive_imf.rs index 8996114ef4..c15eee7a1c 100644 --- a/src/receive_imf.rs +++ b/src/receive_imf.rs @@ -2250,8 +2250,6 @@ enum VerifiedEncryption { /// /// This means that it is encrypted, signed with a verified key, /// and if it's a group, all the recipients are verified. -/// -/// Also propagates gossiped keys to verified if needed. async fn has_verified_encryption( context: &Context, mimeparser: &MimeMessage, @@ -2288,26 +2286,7 @@ async fn has_verified_encryption( )); }; - let signed_with_primary_verified_key = peerstate - .verified_key_fingerprint - .as_ref() - .filter(|fp| mimeparser.signatures.contains(fp)) - .is_some(); - let signed_with_secondary_verified_key = peerstate - .secondary_verified_key_fingerprint - .as_ref() - .filter(|fp| mimeparser.signatures.contains(fp)) - .is_some(); - if signed_with_secondary_verified_key { - let mut peerstate: Peerstate = peerstate.clone(); - peerstate.verified_key = peerstate.secondary_verified_key.take(); - peerstate.verified_key_fingerprint = - peerstate.secondary_verified_key_fingerprint.take(); - peerstate.verifier = peerstate.secondary_verifier.take(); - peerstate.save_to_db(&context.sql).await?; - } - - if !signed_with_primary_verified_key && !signed_with_secondary_verified_key { + if !peerstate.has_verified_key(&mimeparser.signatures) { return Ok(NotVerified( "The message was sent with non-verified encryption".to_string(), )); @@ -2353,23 +2332,16 @@ async fn has_verified_encryption( if mimeparser.gossiped_addr.contains(&to_addr.to_lowercase()) { if let Some(mut peerstate) = Peerstate::from_addr(context, &to_addr).await? { // If we're here, we know the gossip key is verified. - // - Store gossip key as secondary verified key if there is a verified key and - // gossiped key is different. - // - Use the gossip-key as verified-key if there is no verified-key - // - // See - // and for discussion. - let verifier_addr = contact.get_addr().to_owned(); - if is_verified { - // The contact already has a verified key. - // Store gossiped key as the secondary verified key. - peerstate.set_secondary_verified_key_from_gossip(verifier_addr); - peerstate.save_to_db(&context.sql).await?; - } else { - info!(context, "{verifier_addr} has verified {to_addr}."); + // Use the gossip-key as verified-key if there is no verified-key. + if !is_verified { + info!(context, "{} has verified {}.", contact.get_addr(), to_addr); let fp = peerstate.gossip_key_fingerprint.clone(); if let Some(fp) = fp { - peerstate.set_verified(PeerstateKeyType::GossipKey, fp, verifier_addr)?; + peerstate.set_verified( + PeerstateKeyType::GossipKey, + fp, + contact.get_addr().to_owned(), + )?; peerstate.save_to_db(&context.sql).await?; } } diff --git a/src/securejoin.rs b/src/securejoin.rs index 2a55a74d9d..1a50c39c1b 100644 --- a/src/securejoin.rs +++ b/src/securejoin.rs @@ -1060,11 +1060,8 @@ mod tests { gossip_key_fingerprint: Some(alice_pubkey.fingerprint()), verified_key: None, verified_key_fingerprint: None, - verifier: None, - secondary_verified_key: None, - secondary_verified_key_fingerprint: None, - secondary_verifier: None, fingerprint_changed: false, + verifier: None, }; peerstate.save_to_db(&bob.ctx.sql).await?; diff --git a/src/sql/migrations.rs b/src/sql/migrations.rs index 2b3ce60008..6e69311c5f 100644 --- a/src/sql/migrations.rs +++ b/src/sql/migrations.rs @@ -750,19 +750,6 @@ CREATE INDEX smtp_messageid ON imap(rfc724_mid); .await?; } - if dbversion < 104 { - sql.execute_migration( - "ALTER TABLE acpeerstates - ADD COLUMN secondary_verified_key; - ALTER TABLE acpeerstates - ADD COLUMN secondary_verified_key_fingerprint TEXT DEFAULT ''; - ALTER TABLE acpeerstates - ADD COLUMN secondary_verifier TEXT DEFAULT ''", - 104, - ) - .await?; - } - let new_version = sql .get_raw_config_int(VERSION_CFG) .await?