Move key loading from deprecated Key struct to DcKey trait

This moves the loading of the keys from the database to the trait and
thus with types differing between public and secret keys.  This
fetches the Config::ConfiguredAddr (configured_addr) directly from the
database in the SQL to simplify the API and consistency instead of
making this the responsiblity of all callers to get this right.

Since anyone invoking these methods also wants to be sure the keys
exist, move key generation here as well.  This already simplifies some
code in contact.rs and will eventually replace all manual checks for
existing keys.

To make errors more manageable this gives EmailAddress it's own error
type and adds some conversions for it.  Otherwise the general error
type leaks to far.  The EmailAddress type also gets its ToSql trait impl
to be able to save it to the database directly.

src/contact.rs

@@ -11,10 +11,9 @@ use crate::config::Config;
 use crate::constants::*;
 use crate::context::Context;
 use crate::dc_tools::*;
-use crate::e2ee;
 use crate::error::{bail, ensure, format_err, Result};
 use crate::events::Event;
-use crate::key::*;
+use crate::key::{DcKey, Key, SignedPublicKey};
 use crate::login_param::LoginParam;
 use crate::message::{MessageState, MsgId};
 use crate::mimeparser::AvatarAction;
@@ -647,8 +646,6 @@ impl Contact {
             let peerstate = Peerstate::from_addr(context, &context.sql, &contact.addr);
             let loginparam = LoginParam::from_database(context, "configured_");
 
-            let mut self_key = Key::from_self_public(context, &loginparam.addr, &context.sql);
-
             if peerstate.is_some()
                 && peerstate
                     .as_ref()
@@ -663,16 +660,11 @@ impl Contact {
                         StockMessage::E2eAvailable
                     });
                 ret += &p;
-                if self_key.is_none() {
+                let self_key = Key::from(SignedPublicKey::load_self(context)?);
-                    e2ee::ensure_secret_key_exists(context)?;
-                    self_key = Key::from_self_public(context, &loginparam.addr, &context.sql);
-                }
                 let p = context.stock_str(StockMessage::FingerPrints);
                 ret += &format!(" {}:", p);
 
-                let fingerprint_self = self_key
+                let fingerprint_self = self_key.formatted_fingerprint();
-                    .map(|k| k.formatted_fingerprint())
-                    .unwrap_or_default();
                 let fingerprint_other_verified = peerstate
                     .peek_key(PeerstateVerifiedStatus::BidirectVerified)
                     .map(|k| k.formatted_fingerprint())

src/context.rs

@@ -14,7 +14,7 @@ use crate::events::Event;
 use crate::imap::*;
 use crate::job::*;
 use crate::job_thread::JobThread;
-use crate::key::Key;
+use crate::key::{DcKey, Key, SignedPublicKey};
 use crate::login_param::LoginParam;
 use crate::lot::Lot;
 use crate::message::{self, Message, MessengerMessage, MsgId};
@@ -251,10 +251,9 @@ impl Context {
             rusqlite::NO_PARAMS,
         );
 
-        let fingerprint_str = if let Some(key) = Key::from_self_public(self, &l2.addr, &self.sql) {
+        let fingerprint_str = match SignedPublicKey::load_self(self) {
-            key.fingerprint()
+            Ok(key) => Key::from(key).fingerprint(),
-        } else {
+            Err(err) => format!("<key failure: {}>", err),
-            "<Not yet calculated>".into()
         };
 
         let inbox_watch = self.get_config_int(Config::InboxWatch);

src/dc_tools.rs

@@ -12,7 +12,7 @@ use chrono::{Local, TimeZone};
 use rand::{thread_rng, Rng};
 
 use crate::context::Context;
-use crate::error::{bail, ensure, Error};
+use crate::error::{bail, Error};
 use crate::events::Event;
 
 pub(crate) fn dc_exactly_one_bit_set(v: i32) -> bool {
@@ -452,6 +452,23 @@ pub(crate) fn time() -> i64 {
         .as_secs() as i64
 }
 
+/// An invalid email address was encountered
+#[derive(Debug, thiserror::Error)]
+#[error("Invalid email address: {message} ({addr})")]
+pub struct InvalidEmailError {
+    message: String,
+    addr: String,
+}
+
+impl InvalidEmailError {
+    fn new(msg: impl Into<String>, addr: impl Into<String>) -> InvalidEmailError {
+        InvalidEmailError {
+            message: msg.into(),
+            addr: addr.into(),
+        }
+    }
+}
+
 /// Very simple email address wrapper.
 ///
 /// Represents an email address, right now just the `name@domain` portion.
@@ -475,7 +492,7 @@ pub struct EmailAddress {
 }
 
 impl EmailAddress {
-    pub fn new(input: &str) -> Result<Self, Error> {
+    pub fn new(input: &str) -> Result<Self, InvalidEmailError> {
         input.parse::<EmailAddress>()
     }
 }
@@ -487,35 +504,58 @@ impl fmt::Display for EmailAddress {
 }
 
 impl FromStr for EmailAddress {
-    type Err = Error;
+    type Err = InvalidEmailError;
 
     /// Performs a dead-simple parse of an email address.
-    fn from_str(input: &str) -> Result<EmailAddress, Error> {
+    fn from_str(input: &str) -> Result<EmailAddress, InvalidEmailError> {
-        ensure!(!input.is_empty(), "empty string is not valid");
+        if input.is_empty() {
+            return Err(InvalidEmailError::new("empty string is not valid", input));
+        }
         let parts: Vec<&str> = input.rsplitn(2, '@').collect();
 
+        let err = |msg: &str| {
+            Err(InvalidEmailError {
+                message: msg.to_string(),
+                addr: input.to_string(),
+            })
+        };
         match &parts[..] {
             [domain, local] => {
-                ensure!(
+                if local.is_empty() {
-                    !local.is_empty(),
+                    return err("empty string is not valid for local part");
-                    "empty string is not valid for local part"
+                }
-                );
+                if domain.len() <= 3 {
-                ensure!(domain.len() > 3, "domain is too short");
+                    return err("domain is too short");
-
+                }
                 let dot = domain.find('.');
-                ensure!(dot.is_some(), "invalid domain");
+                match dot {
-                ensure!(dot.unwrap() < domain.len() - 2, "invalid domain");
+                    None => {
-
+                        return err("invalid domain");
+                    }
+                    Some(dot_idx) => {
+                        if dot_idx >= domain.len() - 2 {
+                            return err("invalid domain");
+                        }
+                    }
+                }
                 Ok(EmailAddress {
                     local: (*local).to_string(),
                     domain: (*domain).to_string(),
                 })
             }
-            _ => bail!("missing '@' character"),
+            _ => err("missing '@' character"),
         }
     }
 }
 
+impl rusqlite::types::ToSql for EmailAddress {
+    fn to_sql(&self) -> rusqlite::Result<rusqlite::types::ToSqlOutput> {
+        let val = rusqlite::types::Value::Text(self.to_string());
+        let out = rusqlite::types::ToSqlOutput::Owned(val);
+        Ok(out)
+    }
+}
+
 /// Utility to check if a in the binary represantion of listflags
 /// the bit at position bitindex is 1.
 pub(crate) fn listflags_has(listflags: u32, bitindex: usize) -> bool {

src/e2ee.rs

@@ -1,19 +1,16 @@
 //! End-to-end encryption support.
 
 use std::collections::HashSet;
-use std::convert::TryFrom;
 
 use mailparse::ParsedMail;
 use num_traits::FromPrimitive;
 
 use crate::aheader::*;
 use crate::config::Config;
-use crate::constants::KeyGenType;
 use crate::context::Context;
-use crate::dc_tools::EmailAddress;
 use crate::error::*;
 use crate::headerdef::{HeaderDef, HeaderDefMap};
-use crate::key::{self, Key, KeyPairUse, SignedPublicKey};
+use crate::key::{DcKey, Key, SignedPublicKey, SignedSecretKey};
 use crate::keyring::*;
 use crate::peerstate::*;
 use crate::pgp;
@@ -38,7 +35,7 @@ impl EncryptHelper {
             Some(addr) => addr,
         };
 
-        let public_key = load_or_generate_self_public_key(context, &addr)?;
+        let public_key = SignedPublicKey::load_self(context)?;
 
         Ok(EncryptHelper {
             prefer_encrypt,
@@ -108,8 +105,7 @@ impl EncryptHelper {
         }
         let public_key = Key::from(self.public_key.clone());
         keyring.add_ref(&public_key);
-        let sign_key = Key::from_self_private(context, self.addr.clone(), &context.sql)
+        let sign_key = Key::from(SignedSecretKey::load_self(context)?);
-            .ok_or_else(|| format_err!("missing own private key"))?;
 
         let raw_message = mail_to_encrypt.build().as_string().into_bytes();
 
@@ -189,41 +185,6 @@ pub fn try_decrypt(
     Ok((out_mail, signatures))
 }
 
-/// Load public key from database or generate a new one.
-///
-/// This will load a public key from the database, generating and
-/// storing a new one when one doesn't exist yet.  Care is taken to
-/// only generate one key per context even when multiple threads call
-/// this function concurrently.
-fn load_or_generate_self_public_key(
-    context: &Context,
-    self_addr: impl AsRef<str>,
-) -> Result<SignedPublicKey> {
-    if let Some(key) = Key::from_self_public(context, &self_addr, &context.sql) {
-        return SignedPublicKey::try_from(key).map_err(|_| format_err!("Not a public key"));
-    }
-    let _guard = context.generating_key_mutex.lock().unwrap();
-
-    // Check again in case the key was generated while we were waiting for the lock.
-    if let Some(key) = Key::from_self_public(context, &self_addr, &context.sql) {
-        return SignedPublicKey::try_from(key).map_err(|_| format_err!("Not a public key"));
-    }
-
-    let start = std::time::Instant::now();
-
-    let keygen_type =
-        KeyGenType::from_i32(context.get_config_int(Config::KeyGenType)).unwrap_or_default();
-    info!(context, "Generating keypair with type {}", keygen_type);
-    let keypair = pgp::create_keypair(EmailAddress::new(self_addr.as_ref())?, keygen_type)?;
-    key::store_self_keypair(context, &keypair, KeyPairUse::Default)?;
-    info!(
-        context,
-        "Keypair generated in {:.3}s.",
-        start.elapsed().as_secs()
-    );
-    Ok(keypair.public)
-}
-
 /// Returns a reference to the encrypted payload and validates the autocrypt structure.
 fn get_autocrypt_mime<'a, 'b>(mail: &'a ParsedMail<'b>) -> Result<&'a ParsedMail<'b>> {
     ensure!(
@@ -345,6 +306,7 @@ fn contains_report(mail: &ParsedMail<'_>) -> bool {
 ///
 /// If this succeeds you are also guaranteed that the
 /// [Config::ConfiguredAddr] is configured, this address is returned.
+// TODO, remove this once deltachat::key::Key no longer exists.
 pub fn ensure_secret_key_exists(context: &Context) -> Result<String> {
     let self_addr = context.get_config(Config::ConfiguredAddr).ok_or_else(|| {
         format_err!(concat!(
@@ -352,7 +314,7 @@ pub fn ensure_secret_key_exists(context: &Context) -> Result<String> {
             "cannot ensure secret key if not configured."
         ))
     })?;
-    load_or_generate_self_public_key(context, &self_addr)?;
+    SignedPublicKey::load_self(context)?;
     Ok(self_addr)
 }
 
@@ -403,47 +365,6 @@ Sent with my Delta Chat Messenger: https://delta.chat";
         );
     }
 
-    mod load_or_generate_self_public_key {
-        use super::*;
-
-        #[test]
-        fn test_existing() {
-            let t = dummy_context();
-            let addr = configure_alice_keypair(&t.ctx);
-            let key = load_or_generate_self_public_key(&t.ctx, addr);
-            assert!(key.is_ok());
-        }
-
-        #[test]
-        fn test_generate() {
-            let t = dummy_context();
-            let addr = "alice@example.org";
-            let key0 = load_or_generate_self_public_key(&t.ctx, addr);
-            assert!(key0.is_ok());
-            let key1 = load_or_generate_self_public_key(&t.ctx, addr);
-            assert!(key1.is_ok());
-            assert_eq!(key0.unwrap(), key1.unwrap());
-        }
-
-        #[test]
-        fn test_generate_concurrent() {
-            use std::sync::Arc;
-            use std::thread;
-
-            let t = dummy_context();
-            let ctx = Arc::new(t.ctx);
-            let ctx0 = Arc::clone(&ctx);
-            let thr0 =
-                thread::spawn(move || load_or_generate_self_public_key(&ctx0, "alice@example.org"));
-            let ctx1 = Arc::clone(&ctx);
-            let thr1 =
-                thread::spawn(move || load_or_generate_self_public_key(&ctx1, "alice@example.org"));
-            let res0 = thr0.join().unwrap();
-            let res1 = thr1.join().unwrap();
-            assert_eq!(res0.unwrap(), res1.unwrap());
-        }
-    }
-
     #[test]
     fn test_has_decrypted_pgp_armor() {
         let data = b" -----BEGIN PGP MESSAGE-----";

src/imex.rs

@@ -17,7 +17,7 @@ use crate::e2ee;
 use crate::error::*;
 use crate::events::Event;
 use crate::job::*;
-use crate::key::{self, Key};
+use crate::key::{self, DcKey, Key, SignedSecretKey};
 use crate::message::{Message, MsgId};
 use crate::mimeparser::SystemMessage;
 use crate::param::*;
@@ -175,9 +175,7 @@ pub fn render_setup_file(context: &Context, passphrase: &str) -> Result<String>
         passphrase.len() >= 2,
         "Passphrase must be at least 2 chars long."
     );
-    let self_addr = e2ee::ensure_secret_key_exists(context)?;
+    let private_key = Key::from(SignedSecretKey::load_self(context)?);
-    let private_key = Key::from_self_private(context, self_addr, &context.sql)
-        .ok_or_else(|| format_err!("Failed to get private key."))?;
     let ac_headers = match context.get_config_bool(Config::E2eeEnabled) {
         false => None,
         true => Some(("Autocrypt-Prefer-Encrypt", "mutual")),

src/key.rs

@@ -4,14 +4,16 @@ use std::collections::BTreeMap;
 use std::io::Cursor;
 use std::path::Path;
 
+use num_traits::FromPrimitive;
 use pgp::composed::Deserializable;
 use pgp::ser::Serialize;
 use pgp::types::{KeyTrait, SecretKeyTrait};
 
+use crate::config::Config;
 use crate::constants::*;
 use crate::context::Context;
-use crate::dc_tools::*;
+use crate::dc_tools::{dc_write_file, time, EmailAddress, InvalidEmailError};
-use crate::sql::Sql;
+use crate::sql;
 
 // Re-export key types
 pub use crate::pgp::KeyPair;
@@ -19,11 +21,22 @@ pub use pgp::composed::{SignedPublicKey, SignedSecretKey};
 
 /// Error type for deltachat key handling.
 #[derive(Debug, thiserror::Error)]
+#[non_exhaustive]
 pub enum Error {
     #[error("Could not decode base64")]
     Base64Decode(#[from] base64::DecodeError),
-    #[error("rPGP error: {0}")]
+    #[error("rPGP error: {}", _0)]
-    PgpError(#[from] pgp::errors::Error),
+    Pgp(#[from] pgp::errors::Error),
+    #[error("Failed to generate PGP key: {}", _0)]
+    Keygen(#[from] crate::pgp::PgpKeygenError),
+    #[error("Failed to load key: {}", _0)]
+    LoadKey(#[from] sql::Error),
+    #[error("Failed to save generated key: {}", _0)]
+    StoreKey(#[from] SaveKeyError),
+    #[error("No address configured")]
+    NoConfiguredAddr,
+    #[error("Configured address is invalid: {}", _0)]
+    InvalidConfiguredAddr(#[from] InvalidEmailError),
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -51,6 +64,9 @@ pub trait DcKey: Serialize + Deserializable {
         Self::from_slice(&bytes)
     }
 
+    /// Load the users' default key from the database.
+    fn load_self(context: &Context) -> Result<Self::KeyType>;
+
     /// Serialise the key to a base64 string.
     fn to_base64(&self) -> String {
         // Not using Serialize::to_bytes() to make clear *why* it is
@@ -65,10 +81,91 @@ pub trait DcKey: Serialize + Deserializable {
 
 impl DcKey for SignedPublicKey {
     type KeyType = SignedPublicKey;
+
+    fn load_self(context: &Context) -> Result<Self::KeyType> {
+        match context.sql.query_row(
+            r#"
+            SELECT public_key
+              FROM keypairs
+             WHERE addr=(SELECT value FROM config WHERE keyname="configured_addr")
+               AND is_default=1;
+            "#,
+            params![],
+            |row| row.get::<_, Vec<u8>>(0),
+        ) {
+            Ok(bytes) => Self::from_slice(&bytes),
+            Err(sql::Error::Sql(rusqlite::Error::QueryReturnedNoRows)) => {
+                let keypair = generate_keypair(context)?;
+                Ok(keypair.public)
+            }
+            Err(err) => Err(err.into()),
+        }
+    }
 }
 
 impl DcKey for SignedSecretKey {
     type KeyType = SignedSecretKey;
+
+    fn load_self(context: &Context) -> Result<Self::KeyType> {
+        match context.sql.query_row(
+            r#"
+            SELECT private_key
+              FROM keypairs
+             WHERE addr=(SELECT value FROM config WHERE keyname="configured_addr")
+               AND is_default=1;
+            "#,
+            params![],
+            |row| row.get::<_, Vec<u8>>(0),
+        ) {
+            Ok(bytes) => Self::from_slice(&bytes),
+            Err(sql::Error::Sql(rusqlite::Error::QueryReturnedNoRows)) => {
+                let keypair = generate_keypair(context)?;
+                Ok(keypair.secret)
+            }
+            Err(err) => Err(err.into()),
+        }
+    }
+}
+
+fn generate_keypair(context: &Context) -> Result<KeyPair> {
+    let addr = context
+        .get_config(Config::ConfiguredAddr)
+        .ok_or_else(|| Error::NoConfiguredAddr)?;
+    let addr = EmailAddress::new(&addr)?;
+    let _guard = context.generating_key_mutex.lock().unwrap();
+
+    // Check if the key appeared while we were waiting on the lock.
+    match context.sql.query_row(
+        r#"
+        SELECT public_key, private_key
+          FROM keypairs
+         WHERE addr=?1
+           AND is_default=1;
+        "#,
+        params![addr],
+        |row| Ok((row.get::<_, Vec<u8>>(0)?, row.get::<_, Vec<u8>>(1)?)),
+    ) {
+        Ok((pub_bytes, sec_bytes)) => Ok(KeyPair {
+            addr,
+            public: SignedPublicKey::from_slice(&pub_bytes)?,
+            secret: SignedSecretKey::from_slice(&sec_bytes)?,
+        }),
+        Err(sql::Error::Sql(rusqlite::Error::QueryReturnedNoRows)) => {
+            let start = std::time::Instant::now();
+            let keytype = KeyGenType::from_i32(context.get_config_int(Config::KeyGenType))
+                .unwrap_or_default();
+            info!(context, "Generating keypair with type {}", keytype);
+            let keypair = crate::pgp::create_keypair(addr, keytype)?;
+            store_self_keypair(context, &keypair, KeyPairUse::Default)?;
+            info!(
+                context,
+                "Keypair generated in {:.3}s.",
+                start.elapsed().as_secs()
+            );
+            Ok(keypair)
+        }
+        Err(err) => Err(err.into()),
+    }
 }
 
 /// Cryptographic key
@@ -185,34 +282,6 @@ impl Key {
         }
     }
 
-    pub fn from_self_public(
-        context: &Context,
-        self_addr: impl AsRef<str>,
-        sql: &Sql,
-    ) -> Option<Self> {
-        let addr = self_addr.as_ref();
-
-        sql.query_get_value(
-            context,
-            "SELECT public_key FROM keypairs WHERE addr=? AND is_default=1;",
-            &[addr],
-        )
-        .and_then(|blob: Vec<u8>| Self::from_slice(&blob, KeyType::Public))
-    }
-
-    pub fn from_self_private(
-        context: &Context,
-        self_addr: impl AsRef<str>,
-        sql: &Sql,
-    ) -> Option<Self> {
-        sql.query_get_value(
-            context,
-            "SELECT private_key FROM keypairs WHERE addr=? AND is_default=1;",
-            &[self_addr.as_ref()],
-        )
-        .and_then(|blob: Vec<u8>| Self::from_slice(&blob, KeyType::Private))
-    }
-
     pub fn to_bytes(&self) -> Vec<u8> {
         match self {
             Key::Public(k) => k.to_bytes().unwrap_or_default(),
@@ -539,6 +608,59 @@ i8pcjGO+IZffvyZJVRWfVooBJmWWbPB1pueo3tx8w3+fcuzpxz+RLFKaPyqXO+dD
         }
     }
 
+    #[test]
+    fn test_load_self_existing() {
+        let alice = alice_keypair();
+        let t = dummy_context();
+        configure_alice_keypair(&t.ctx);
+        let pubkey = SignedPublicKey::load_self(&t.ctx).unwrap();
+        assert_eq!(alice.public, pubkey);
+        let seckey = SignedSecretKey::load_self(&t.ctx).unwrap();
+        assert_eq!(alice.secret, seckey);
+    }
+
+    #[test]
+    #[ignore] // generating keys is expensive
+    fn test_load_self_generate_public() {
+        let t = dummy_context();
+        t.ctx
+            .set_config(Config::ConfiguredAddr, Some("alice@example.com"))
+            .unwrap();
+        let key = SignedPublicKey::load_self(&t.ctx);
+        assert!(key.is_ok());
+    }
+
+    #[test]
+    #[ignore] // generating keys is expensive
+    fn test_load_self_generate_secret() {
+        let t = dummy_context();
+        t.ctx
+            .set_config(Config::ConfiguredAddr, Some("alice@example.com"))
+            .unwrap();
+        let key = SignedSecretKey::load_self(&t.ctx);
+        assert!(key.is_ok());
+    }
+
+    #[test]
+    #[ignore] // generating keys is expensive
+    fn test_load_self_generate_concurrent() {
+        use std::sync::Arc;
+        use std::thread;
+
+        let t = dummy_context();
+        t.ctx
+            .set_config(Config::ConfiguredAddr, Some("alice@example.com"))
+            .unwrap();
+        let ctx = Arc::new(t.ctx);
+        let ctx0 = Arc::clone(&ctx);
+        let thr0 = thread::spawn(move || SignedPublicKey::load_self(&ctx0));
+        let ctx1 = Arc::clone(&ctx);
+        let thr1 = thread::spawn(move || SignedPublicKey::load_self(&ctx1));
+        let res0 = thr0.join().unwrap();
+        let res1 = thr1.join().unwrap();
+        assert_eq!(res0.unwrap(), res1.unwrap());
+    }
+
     #[test]
     fn test_ascii_roundtrip() {
         let public_key = Key::from(KEYPAIR.public.clone());

src/pgp.rs

@@ -119,7 +119,7 @@ pub fn split_armored_data(buf: &[u8]) -> Result<(BlockType, BTreeMap<String, Str
 /// since all variability is hardcoded.
 #[derive(Debug, thiserror::Error)]
 #[error("PgpKeygenError: {message}")]
-pub(crate) struct PgpKeygenError {
+pub struct PgpKeygenError {
     message: String,
     #[source]
     cause: anyhow::Error,

src/securejoin.rs

@@ -12,7 +12,7 @@ use crate::e2ee::*;
 use crate::error::{bail, Error};
 use crate::events::Event;
 use crate::headerdef::HeaderDef;
-use crate::key::{dc_normalize_fingerprint, Key};
+use crate::key::{dc_normalize_fingerprint, DcKey, Key, SignedPublicKey};
 use crate::lot::LotState;
 use crate::message::Message;
 use crate::mimeparser::*;
@@ -135,12 +135,13 @@ pub fn dc_get_securejoin_qr(context: &Context, group_chat_id: ChatId) -> Option<
 }
 
 fn get_self_fingerprint(context: &Context) -> Option<String> {
-    if let Some(self_addr) = context.get_config(Config::ConfiguredAddr) {
+    match SignedPublicKey::load_self(context) {
-        if let Some(key) = Key::from_self_public(context, self_addr, &context.sql) {
+        Ok(key) => Some(Key::from(key).fingerprint()),
-            return Some(key.fingerprint());
+        Err(_) => {
+            warn!(context, "get_self_fingerprint(): failed to load key");
+            None
         }
     }
-    None
 }
 
 /// Take a scanned QR-code and do the setup-contact/join-group handshake.