fix: make it impossible to overwrite default key

Replacing default key
when a profile is already part of
verified groups results in
`[The message was sent with non-verified encryption. See 'Info' for more details]`
messages for other users.

It is still possible
to import the default key before
Delta Chat generates the key.

deltachat-rpc-client/src/deltachat_rpc_client/account.py

@@ -359,3 +359,7 @@ class Account:
         """Import keys."""
         passphrase = ""  # Importing passphrase-protected keys is currently not supported.
         self._rpc.import_self_keys(self.id, str(path), passphrase)
+
+    def initiate_autocrypt_key_transfer(self) -> None:
+        """Send Autocrypt Setup Message."""
+        return self._rpc.initiate_autocrypt_key_transfer(self.id)

deltachat-rpc-client/src/deltachat_rpc_client/message.py

@@ -52,6 +52,9 @@ class Message:
         """Mark the message as seen."""
         self._rpc.markseen_msgs(self.account.id, [self.id])
 
+    def continue_autocrypt_key_transfer(self, setup_code: str) -> None:
+        self._rpc.continue_autocrypt_key_transfer(self.account.id, self.id, setup_code)
+
     def send_webxdc_status_update(self, update: Union[dict, str], description: str) -> None:
         """Send a webxdc status update. This message must be a webxdc."""
         if not isinstance(update, str):

deltachat-rpc-client/tests/test_key_transfer.py

@@ -0,0 +1,50 @@
+import pytest
+
+from deltachat_rpc_client import EventType
+from deltachat_rpc_client.rpc import JsonRpcError
+
+
+def wait_for_autocrypt_setup_message(account):
+    while True:
+        event = account.wait_for_event()
+        if event.kind == EventType.MSGS_CHANGED and event.msg_id != 0:
+            msg_id = event.msg_id
+            msg = account.get_message_by_id(msg_id)
+            if msg.get_snapshot().is_setupmessage:
+                return msg
+
+
+def test_autocrypt_setup_message_key_transfer(acfactory):
+    alice1 = acfactory.get_online_account()
+
+    alice2 = acfactory.get_unconfigured_account()
+    alice2.set_config("addr", alice1.get_config("addr"))
+    alice2.set_config("mail_pw", alice1.get_config("mail_pw"))
+    alice2.configure()
+
+    setup_code = alice1.initiate_autocrypt_key_transfer()
+    msg = wait_for_autocrypt_setup_message(alice2)
+
+    # Test that entering wrong code returns an error.
+    with pytest.raises(JsonRpcError):
+        msg.continue_autocrypt_key_transfer("7037-0673-6287-3013-4095-7956-5617-6806-6756")
+
+    msg.continue_autocrypt_key_transfer(setup_code)
+
+
+def test_ac_setup_message_twice(acfactory):
+    alice1 = acfactory.get_online_account()
+    alice2 = acfactory.get_unconfigured_account()
+    alice2.set_config("addr", alice1.get_config("addr"))
+    alice2.set_config("mail_pw", alice1.get_config("mail_pw"))
+    alice2.configure()
+
+    # Send the first Autocrypt Setup Message and ignore it.
+    _setup_code = alice1.initiate_autocrypt_key_transfer()
+    wait_for_autocrypt_setup_message(alice2)
+
+    # Send the second Autocrypt Setup Message and import it.
+    setup_code = alice1.initiate_autocrypt_key_transfer()
+    msg = wait_for_autocrypt_setup_message(alice2)
+
+    msg.continue_autocrypt_key_transfer(setup_code)

deltachat-rpc-client/tests/test_securejoin.py

@@ -4,6 +4,7 @@ import time
 import pytest
 
 from deltachat_rpc_client import Chat, EventType, SpecialContactId
+from deltachat_rpc_client.rpc import JsonRpcError
 
 
 def test_qr_setup_contact(acfactory, tmp_path) -> None:
@@ -26,17 +27,21 @@ def test_qr_setup_contact(acfactory, tmp_path) -> None:
     bob_contact_alice_snapshot = bob_contact_alice.get_snapshot()
     assert bob_contact_alice_snapshot.is_verified
 
-    # Test that if Bob changes the key, backwards verification is lost.
+    # Test that if Bob imports a key,
+    # backwards verification is not lost
+    # because default key is not changed.
     logging.info("Bob 2 is created")
     bob2 = acfactory.new_configured_account()
     bob2.export_self_keys(tmp_path)
 
-    logging.info("Bob imports a key")
-    bob.import_self_keys(tmp_path)
+    logging.info("Bob tries to import a key")
+    # Importing a second key is not allowed.
+    with pytest.raises(JsonRpcError):
+        bob.import_self_keys(tmp_path)
 
-    assert bob.get_config("key_id") == "2"
+    assert bob.get_config("key_id") == "1"
     bob_contact_alice_snapshot = bob_contact_alice.get_snapshot()
-    assert not bob_contact_alice_snapshot.is_verified
+    assert bob_contact_alice_snapshot.is_verified
 
 
 def test_qr_setup_contact_svg(acfactory) -> None:

python/src/deltachat/message.py

@@ -215,7 +215,7 @@ class Message:
         """extract key and use it as primary key for this account."""
         res = lib.dc_continue_key_transfer(self.account._dc_context, self.id, as_dc_charpointer(setup_code))
         if res == 0:
-            raise ValueError("could not decrypt")
+            raise ValueError("Importing the key from Autocrypt Setup Message failed")
 
     @props.with_doc
     def time_sent(self):

python/tests/test_0_complex_or_slow.py

@@ -510,6 +510,7 @@ def test_see_new_verified_member_after_going_online(acfactory, tmp_path, lp):
     """
     ac1, ac2 = acfactory.get_online_accounts(2)
     ac2_addr = ac2.get_config("addr")
+    acfactory.remove_preconfigured_keys()
     ac1_offl = acfactory.new_online_configuring_account(cloned_from=ac1)
     for ac in [ac1, ac1_offl]:
         ac.set_config("bcc_self", "1")
@@ -560,6 +561,7 @@ def test_use_new_verified_group_after_going_online(acfactory, data, tmp_path, lp
       missing, cannot encrypt".
     """
     ac1, ac2 = acfactory.get_online_accounts(2)
+    acfactory.remove_preconfigured_keys()
     ac2_offl = acfactory.new_online_configuring_account(cloned_from=ac2)
     for ac in [ac2, ac2_offl]:
         ac.set_config("bcc_self", "1")
@@ -615,6 +617,7 @@ def test_verified_group_vs_delete_server_after(acfactory, tmp_path, lp):
     - Now the seconds device has all members verified.
     """
     ac1, ac2 = acfactory.get_online_accounts(2)
+    acfactory.remove_preconfigured_keys()
     ac2_offl = acfactory.new_online_configuring_account(cloned_from=ac2)
     for ac in [ac2, ac2_offl]:
         ac.set_config("bcc_self", "1")

python/tests/test_1_online.py

@@ -85,27 +85,6 @@ def test_configure_unref(tmp_path):
     lib.dc_context_unref(dc_context)
 
 
-def test_export_import_self_keys(acfactory, tmp_path, lp):
-    ac1, ac2 = acfactory.get_online_accounts(2)
-
-    dir = tmp_path / "exportdir"
-    dir.mkdir()
-    export_files = ac1.export_self_keys(str(dir))
-    assert len(export_files) == 2
-    for x in export_files:
-        assert x.startswith(str(dir))
-    (key_id,) = ac1._evtracker.get_info_regex_groups(r".*xporting.*KeyId\((.*)\).*")
-    ac1._evtracker.consume_events()
-
-    lp.sec("exported keys (private and public)")
-    for name in dir.iterdir():
-        lp.indent(str(dir / name))
-    lp.sec("importing into existing account")
-    ac2.import_self_keys(str(dir))
-    (key_id2,) = ac2._evtracker.get_info_regex_groups(r".*stored.*KeyId\((.*)\).*")
-    assert key_id2 == key_id
-
-
 def test_one_account_send_bcc_setting(acfactory, lp):
     ac1 = acfactory.new_online_configuring_account()
     ac2 = acfactory.new_online_configuring_account()
@@ -1597,53 +1576,6 @@ def test_import_export_online_all(acfactory, tmp_path, data, lp):
     assert ac2.get_latest_backupfile(str(backupdir)) == path2
 
 
-def test_ac_setup_message(acfactory, lp):
-    # note that the receiving account needs to be configured and running
-    # before the setup message is send. DC does not read old messages
-    # as of Jul2019
-    ac1 = acfactory.new_online_configuring_account()
-    ac2 = acfactory.new_online_configuring_account(cloned_from=ac1)
-    acfactory.bring_accounts_online()
-
-    lp.sec("trigger ac setup message and return setupcode")
-    assert ac1.get_info()["fingerprint"] != ac2.get_info()["fingerprint"]
-    setup_code = ac1.initiate_key_transfer()
-    ev = ac2._evtracker.get_matching("DC_EVENT_INCOMING_MSG|DC_EVENT_MSGS_CHANGED")
-    msg = ac2.get_message_by_id(ev.data2)
-    assert msg.is_setup_message()
-    assert msg.get_setupcodebegin() == setup_code[:2]
-    lp.sec("try a bad setup code")
-    with pytest.raises(ValueError):
-        msg.continue_key_transfer(str(reversed(setup_code)))
-    lp.sec("try a good setup code")
-    print("*************** Incoming ASM File at: ", msg.filename)
-    print("*************** Setup Code: ", setup_code)
-    msg.continue_key_transfer(setup_code)
-    assert ac1.get_info()["fingerprint"] == ac2.get_info()["fingerprint"]
-
-
-def test_ac_setup_message_twice(acfactory, lp):
-    ac1 = acfactory.new_online_configuring_account()
-    ac2 = acfactory.new_online_configuring_account(cloned_from=ac1)
-    acfactory.bring_accounts_online()
-
-    lp.sec("trigger ac setup message but ignore")
-    assert ac1.get_info()["fingerprint"] != ac2.get_info()["fingerprint"]
-    ac1.initiate_key_transfer()
-    ac2._evtracker.get_matching("DC_EVENT_INCOMING_MSG|DC_EVENT_MSGS_CHANGED")
-
-    lp.sec("trigger second ac setup message, wait for receive ")
-    setup_code2 = ac1.initiate_key_transfer()
-    ev = ac2._evtracker.get_matching("DC_EVENT_INCOMING_MSG|DC_EVENT_MSGS_CHANGED")
-    msg = ac2.get_message_by_id(ev.data2)
-    assert msg.is_setup_message()
-    assert msg.get_setupcodebegin() == setup_code2[:2]
-
-    lp.sec("process second setup message")
-    msg.continue_key_transfer(setup_code2)
-    assert ac1.get_info()["fingerprint"] == ac2.get_info()["fingerprint"]
-
-
 def test_qr_email_capitalization(acfactory, lp):
     """Regression test for a bug
     that resulted in failure to propagate verification via gossip in a verified group

src/configure.rs

@@ -38,7 +38,7 @@ use crate::provider::{Protocol, Socket, UsernamePattern};
 use crate::smtp::Smtp;
 use crate::sync::Sync::*;
 use crate::tools::time;
-use crate::{chat, e2ee, provider};
+use crate::{chat, provider};
 use crate::{stock_str, EventType};
 use deltachat_contact_tools::addr_cmp;
 
@@ -473,9 +473,6 @@ async fn configure(ctx: &Context, param: &EnteredLoginParam) -> Result<Configure
 
     progress!(ctx, 920);
 
-    e2ee::ensure_secret_key_exists(ctx).await?;
-    info!(ctx, "key generation completed");
-
     ctx.set_config_internal(Config::FetchedExistingMsgs, config::from_bool(false))
         .await?;
     ctx.scheduler.interrupt_inbox().await;

src/imex.rs

@@ -138,7 +138,7 @@ pub async fn has_backup(_context: &Context, dir_name: &Path) -> Result<String> {
     }
 }
 
-async fn set_self_key(context: &Context, armored: &str, set_default: bool) -> Result<()> {
+async fn set_self_key(context: &Context, armored: &str) -> Result<()> {
     // try hard to only modify key-state
     let (private_key, header) = SignedSecretKey::from_asc(armored)?;
     let public_key = private_key.split_public_key()?;
@@ -170,16 +170,7 @@ async fn set_self_key(context: &Context, armored: &str, set_default: bool) -> Re
         public: public_key,
         secret: private_key,
     };
-    key::store_self_keypair(
-        context,
-        &keypair,
-        if set_default {
-            key::KeyPairUse::Default
-        } else {
-            key::KeyPairUse::ReadOnly
-        },
-    )
-    .await?;
+    key::store_self_keypair(context, &keypair).await?;
 
     info!(context, "stored self key: {:?}", keypair.secret.key_id());
     Ok(())
@@ -599,10 +590,10 @@ where
 }
 
 /// Imports secret key from a file.
-async fn import_secret_key(context: &Context, path: &Path, set_default: bool) -> Result<()> {
+async fn import_secret_key(context: &Context, path: &Path) -> Result<()> {
     let buf = read_file(context, path).await?;
     let armored = std::string::String::from_utf8_lossy(&buf);
-    set_self_key(context, &armored, set_default).await?;
+    set_self_key(context, &armored).await?;
     Ok(())
 }
 
@@ -624,8 +615,7 @@ async fn import_self_keys(context: &Context, path: &Path) -> Result<()> {
             "Importing secret key from {} as the default key.",
             path.display()
         );
-        let set_default = true;
-        import_secret_key(context, path, set_default).await?;
+        import_secret_key(context, path).await?;
         return Ok(());
     }
 
@@ -643,14 +633,13 @@ async fn import_self_keys(context: &Context, path: &Path) -> Result<()> {
         } else {
             continue;
         };
-        let set_default = !name_f.contains("legacy");
         info!(
             context,
             "Considering key file: {}.",
             path_plus_name.display()
         );
 
-        if let Err(err) = import_secret_key(context, &path_plus_name, set_default).await {
+        if let Err(err) = import_secret_key(context, &path_plus_name).await {
             warn!(
                 context,
                 "Failed to import secret key from {}: {:#}.",
@@ -871,7 +860,7 @@ mod tests {
 
         assert_eq!(bytes, key.to_asc(None).into_bytes());
 
-        let alice = &TestContext::new_alice().await;
+        let alice = &TestContext::new().await;
         if let Err(err) = imex(alice, ImexMode::ImportSelfKeys, Path::new(&filename), None).await {
             panic!("got error on import: {err:#}");
         }
@@ -893,7 +882,7 @@ mod tests {
             panic!("got error on export: {err:#}");
         }
 
-        let context2 = TestContext::new_alice().await;
+        let context2 = TestContext::new().await;
         if let Err(err) = imex(
             &context2.ctx,
             ImexMode::ImportSelfKeys,
@@ -920,15 +909,18 @@ mod tests {
         let alice = &TestContext::new_alice().await;
         let old_key = key::load_self_secret_key(alice).await?;
 
-        imex(alice, ImexMode::ImportSelfKeys, export_dir.path(), None).await?;
-
-        let new_key = key::load_self_secret_key(alice).await?;
-        assert_ne!(new_key, old_key);
-        assert_eq!(
-            key::load_self_secret_keyring(alice).await?,
-            vec![new_key, old_key]
+        assert!(
+            imex(alice, ImexMode::ImportSelfKeys, export_dir.path(), None)
+                .await
+                .is_err()
         );
 
+        // Importing a second key is not allowed anymore,
+        // even as a non-default key.
+        assert_eq!(key::load_self_secret_key(alice).await?, old_key);
+
+        assert_eq!(key::load_self_secret_keyring(alice).await?, vec![old_key]);
+
         let msg = alice.recv_msg(&sent).await;
         assert!(msg.get_showpadlock());
         assert_eq!(msg.chat_id, alice.get_self_chat().await.id);

src/imex/key_transfer.rs

@@ -75,7 +75,7 @@ pub async fn continue_key_transfer(
         let file = open_file_std(context, filename)?;
         let sc = normalize_setup_code(setup_code);
         let armored_key = decrypt_setup_file(&sc, file).await?;
-        set_self_key(context, &armored_key, true).await?;
+        set_self_key(context, &armored_key).await?;
         context.set_config_bool(Config::BccSelf, true).await?;
 
         Ok(())
@@ -315,18 +315,19 @@ mod tests {
         alice2.recv_msg(&sent).await;
         let msg = alice2.get_last_msg().await;
         assert!(msg.is_setupmessage());
-
-        // Send a message that cannot be decrypted because the keys are
-        // not synchronized yet.
-        let sent = alice2.send_text(msg.chat_id, "Test").await;
-        let trashed_message = alice.recv_msg_opt(&sent).await;
-        assert!(trashed_message.is_none());
-        assert_ne!(alice.get_last_msg().await.get_text(), "Test");
+        assert_eq!(
+            crate::key::load_self_secret_keyring(&alice2).await?.len(),
+            0
+        );
 
         // Transfer the key.
         alice2.set_config(Config::BccSelf, Some("0")).await?;
         continue_key_transfer(&alice2, msg.id, &setup_code).await?;
         assert_eq!(alice2.get_config_bool(Config::BccSelf).await?, true);
+        assert_eq!(
+            crate::key::load_self_secret_keyring(&alice2).await?.len(),
+            1
+        );
 
         // Alice sends a message to self from the new device.
         let sent = alice2.send_text(msg.chat_id, "Test").await;

src/key.rs

@@ -289,7 +289,7 @@ async fn generate_keypair(context: &Context) -> Result<KeyPair> {
                 .spawn_blocking(move || crate::pgp::create_keypair(addr, keytype))
                 .await??;
 
-            store_self_keypair(context, &keypair, KeyPairUse::Default).await?;
+            store_self_keypair(context, &keypair).await?;
             info!(
                 context,
                 "Keypair generated in {:.3}s.",
@@ -326,18 +326,6 @@ pub(crate) async fn load_keypair(context: &Context) -> Result<Option<KeyPair>> {
     })
 }
 
-/// Use of a key pair for encryption or decryption.
-///
-/// This is used by `store_self_keypair` to know what kind of key is
-/// being saved.
-#[derive(Debug, Clone, Eq, PartialEq)]
-pub enum KeyPairUse {
-    /// The default key used to encrypt new messages.
-    Default,
-    /// Only used to decrypt existing message.
-    ReadOnly,
-}
-
 /// Store the keypair as an owned keypair for addr in the database.
 ///
 /// This will save the keypair as keys for the given address.  The
@@ -350,11 +338,7 @@ pub enum KeyPairUse {
 /// same key again overwrites it.
 ///
 /// [Config::ConfiguredAddr]: crate::config::Config::ConfiguredAddr
-pub(crate) async fn store_self_keypair(
-    context: &Context,
-    keypair: &KeyPair,
-    default: KeyPairUse,
-) -> Result<()> {
+pub(crate) async fn store_self_keypair(context: &Context, keypair: &KeyPair) -> Result<()> {
     let mut config_cache_lock = context.sql.config_cache.write().await;
     let new_key_id = context
         .sql
@@ -362,29 +346,28 @@ pub(crate) async fn store_self_keypair(
             let public_key = DcKey::to_bytes(&keypair.public);
             let secret_key = DcKey::to_bytes(&keypair.secret);
 
-            let is_default = match default {
-                KeyPairUse::Default => true,
-                KeyPairUse::ReadOnly => false,
-            };
-
+            // private_key and public_key columns
+            // are UNIQUE since migration 107,
+            // so this fails if we already have this key.
             transaction
                 .execute(
-                    "INSERT OR REPLACE INTO keypairs (public_key, private_key)
+                    "INSERT INTO keypairs (public_key, private_key)
                      VALUES (?,?)",
                     (&public_key, &secret_key),
                 )
                 .context("Failed to insert keypair")?;
 
-            if is_default {
-                let new_key_id = transaction.last_insert_rowid();
-                transaction.execute(
-                    "INSERT OR REPLACE INTO config (keyname, value) VALUES ('key_id', ?)",
-                    (new_key_id,),
-                )?;
-                Ok(Some(new_key_id))
-            } else {
-                Ok(None)
-            }
+            let new_key_id = transaction.last_insert_rowid();
+
+            // This will fail if we already have `key_id`.
+            //
+            // Setting default key is only possible if we don't
+            // have a key already.
+            transaction.execute(
+                "INSERT INTO config (keyname, value) VALUES ('key_id', ?)",
+                (new_key_id,),
+            )?;
+            Ok(Some(new_key_id))
         })
         .await?;
 
@@ -405,7 +388,7 @@ pub async fn preconfigure_keypair(context: &Context, secret_data: &str) -> Resul
     let secret = SignedSecretKey::from_asc(secret_data)?.0;
     let public = secret.split_public_key()?;
     let keypair = KeyPair { public, secret };
-    store_self_keypair(context, &keypair, KeyPairUse::Default).await?;
+    store_self_keypair(context, &keypair).await?;
     Ok(())
 }
 
@@ -700,6 +683,7 @@ i8pcjGO+IZffvyZJVRWfVooBJmWWbPB1pueo3tx8w3+fcuzpxz+RLFKaPyqXO+dD
         assert_eq!(pubkey.primary_key, KEYPAIR.public.primary_key);
     }
 
+    /// Tests that setting a default key second time is not allowed.
     #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
     async fn test_save_self_key_twice() {
         // Saving the same key twice should result in only one row in
@@ -714,13 +698,13 @@ i8pcjGO+IZffvyZJVRWfVooBJmWWbPB1pueo3tx8w3+fcuzpxz+RLFKaPyqXO+dD
                 .unwrap()
         };
         assert_eq!(nrows().await, 0);
-        store_self_keypair(&ctx, &KEYPAIR, KeyPairUse::Default)
-            .await
-            .unwrap();
+        store_self_keypair(&ctx, &KEYPAIR).await.unwrap();
         assert_eq!(nrows().await, 1);
-        store_self_keypair(&ctx, &KEYPAIR, KeyPairUse::Default)
-            .await
-            .unwrap();
+
+        // Saving a second key fails.
+        let res = store_self_keypair(&ctx, &KEYPAIR).await;
+        assert!(res.is_err());
+
         assert_eq!(nrows().await, 1);
     }
 

src/receive_imf/receive_imf_tests.rs

@@ -4705,8 +4705,10 @@ async fn test_outgoing_msg_forgery() -> Result<()> {
     // We need Bob only to encrypt the forged message to Alice's key, actually Bob doesn't
     // participate in the scenario.
     let bob = &TestContext::new().await;
+    assert_eq!(crate::key::load_self_secret_keyring(bob).await?.len(), 0);
     bob.configure_addr("bob@example.net").await;
     imex(bob, ImexMode::ImportSelfKeys, export_dir.path(), None).await?;
+    assert_eq!(crate::key::load_self_secret_keyring(bob).await?.len(), 1);
     let malice = &TestContext::new().await;
     malice.configure_addr(alice_addr).await;
 
@@ -4714,6 +4716,7 @@ async fn test_outgoing_msg_forgery() -> Result<()> {
         .send_recv_accept(bob, malice, "hi from bob")
         .await
         .chat_id;
+    assert_eq!(crate::key::load_self_secret_keyring(bob).await?.len(), 1);
 
     let sent_msg = malice.send_text(malice_chat_id, "hi from malice").await;
     let msg = alice.recv_msg(&sent_msg).await;

src/test_utils.rs

@@ -33,7 +33,7 @@ use crate::contact::{Contact, ContactId, Modifier, Origin};
 use crate::context::Context;
 use crate::e2ee::EncryptHelper;
 use crate::events::{Event, EventEmitter, EventType, Events};
-use crate::key::{self, DcKey, KeyPairUse};
+use crate::key::{self, DcKey};
 use crate::message::{update_msg_state, Message, MessageState, MsgId, Viewtype};
 use crate::mimeparser::{MimeMessage, SystemMessage};
 use crate::peerstate::Peerstate;
@@ -271,7 +271,7 @@ impl TestContextBuilder {
 
             let test_context = TestContext::new_internal(Some(name), self.log_sink).await;
             test_context.configure_addr(&addr).await;
-            key::store_self_keypair(&test_context, &key_pair, KeyPairUse::Default)
+            key::store_self_keypair(&test_context, &key_pair)
                 .await
                 .expect("Failed to save key");
             test_context