Merge branch 'malte/polish' into 'main'

Polish the code

See merge request husk-project/husk-milter!10

build.rs

@@ -1,7 +1,6 @@
 //! build
 
 use anyhow::Result;
-use anyhow;
 use clap::CommandFactory;
 use clap::ValueEnum;
 use clap_complete::Shell;

src/commands.rs

@@ -29,6 +29,8 @@ pub enum CommandError {
     CertRevoked(Fingerprint),
     #[error("No signing key available")]
     SigningKeyNotAvailable,
+    #[error("UserID contains ANSI control characters")]
+    ANSIControlCharacters,
 }
 
 pub async fn dispatch(cli: CliArgs, config: HuskConfigContainer) -> Result<()> {

src/commands/locals.rs

@@ -20,11 +20,12 @@ pub async fn dispatch(cmd: LocalsCommand, config: HuskConfigContainer) -> Result
         LocalsSubcommand::List => {
             let context = HuskContext::new(&config.into())?;
 
-            println!("list locals");
+            // XXX: sort the output
             for local in crypto::get_locals(
                 &context.cert_store,
                 &context.policy,
-                context.local_trust_root.fingerprint()) {
+                context.local_trust_root.fingerprint(),
+                context.signing_key) {
 
                 println!("{}", local);
             }

src/commands/signkey/create.rs

@@ -7,6 +7,7 @@ use sequoia_openpgp::cert::CertBuilder;
 use sequoia_directories::Home;
 
 use crate::cli::cli_args::SignkeyCreateCommand;
+use crate::commands::CommandError;
 use crate::common::crypto;
 use crate::config::{HuskConfig, HuskConfigContainer};
 use crate::types::husk_context::HuskContext;
@@ -18,17 +19,25 @@ pub async fn dispatch(cmd: SignkeyCreateCommand, config: HuskConfigContainer)
     let context = HuskContext::new(&config)?;
     let sequoia_home = Home::new(PathBuf::from(&config.sequoia_home))?;
 
-    // XXX: cleanup the userid before using
+    if cmd.userid.chars().any(char::is_control) {
+        return Err(CommandError::ANSIControlCharacters.into());
+    }
+
     let (cert, _) = CertBuilder::new()
         .add_signing_subkey()
         .add_userid(cmd.userid)
         .generate()?;
 
-    if crypto::import_key(sequoia_home, &cert) {
+    match crypto::import_key(sequoia_home, &cert) {
-        let vc = cert.with_policy(&context.policy, None)?;
+        Ok(_) => {
-        crypto::set_signing_key(&context, &vc)?;
+            let vc = cert.with_policy(&context.policy, None)?;
+            crypto::set_signing_key(&context, &vc)?;
 
-        println!("New signing key: {}", vc.fingerprint());
+            println!("New signing key: {}", vc.fingerprint());
+        },
+        Err(e) => {
+            println!("Error: {}", e);
+        }
     }
 
     Ok(())

src/common/crypto.rs

@@ -113,6 +113,7 @@ pub async fn lookup_certificates<'hc>(context: &HuskContext<'hc>, email: &str)
     // local function to check certificates for usability
     let collect_certs = |certs: Vec<Result<Cert, anyhow::Error>>, email: String, policy: &StandardPolicy| {
         let mut result = Vec::new();
+        let mut seen: HashSet<Fingerprint> = HashSet::new();
 
         for cert in certs.into_iter().flatten() {
             if let Ok(vc) = cert.with_policy(policy, None) {
@@ -136,9 +137,11 @@ pub async fn lookup_certificates<'hc>(context: &HuskContext<'hc>, email: &str)
                         .revoked(false)
                         .for_transport_encryption()
                         .for_each(|_| {
-                            // XXX: cert gets returned for each transport key
+                            let fpr = cert.fingerprint();
-                            // once would be enough
+                            if ! seen.contains(&fpr) {
-                            result.push(cert.clone());
+                                result.push(cert.clone());
+                                seen.insert(fpr);
+                            }
                         });
                 }
             }
@@ -287,26 +290,48 @@ pub fn get_introducers<'c>(cert_store: &CertStore<'c>, policy: &StandardPolicy,
         if depth > Depth::Limit(0) {
             if let Some(p) = target {
                 if let Some(c) = p.certifications().last() {
-                    result.push( Introducer {
+                    if let Some(r) = c.regular_expressions() {
-                        cert: p.target().clone(),
+                        if ! r.matches_everything() {
-                        certification: c.clone(),
+                            result.push( Introducer {
-                    });
+                                cert: p.target().clone(),
+                                certification: c.clone(),
+                            });
+                        }
+                    }
                 }
             }
         }
     });
 
-    result
+    // Certificates may have multiple authenticating pathes. Dedup.
+    let mut seen = HashSet::new();
+    result.into_iter()
+        .filter(|i|
+            if seen.contains(i) { false }
+            else {
+                seen.insert(i.clone());
+                true
+            }
+        )
+        .collect()
 }
 
 /// Returns all local certificates available in cert_store.
-pub fn get_locals<'c>(cert_store: &CertStore<'c>, policy: &StandardPolicy, local_trust_root: Fingerprint) -> Vec<CertSynopsis> {
+pub fn get_locals<'c>(cert_store: &CertStore<'c>,
+        policy: &StandardPolicy,
+        local_trust_root: Fingerprint,
+        signing_key: Option<Key>) -> Vec<CertSynopsis> {
 
     let mut set = HashMap::new();
     let mut introducers = HashSet::new();
     let mut targets = Vec::new();
     let local_root_fpr = local_trust_root.clone();
 
+    let signing_fpr = if let Some(signing_key) = signing_key {
+        Some(signing_key.fingerprint().clone())
+    } else { None };
+
+
     let trust_roots = vec![(local_trust_root, wot::FULLY_TRUSTED)];
     let wot_store = wot::store::CertStore::from_store(cert_store, policy, None);
     let n = wot::NetworkBuilder::rooted(&wot_store, &*trust_roots)
@@ -335,6 +360,12 @@ pub fn get_locals<'c>(cert_store: &CertStore<'c>, policy: &StandardPolicy, local
     for p in targets {
         let cert_synopsis = p.target().clone();
 
+        if let Some(ref s) = signing_fpr {
+            if &cert_synopsis.fingerprint() == s {
+                continue;
+            }
+        }
+
         if let Vacant(_) = set.entry(cert_synopsis.fingerprint()) {
             // Only consider non introduced certificates.
             if !p.certifications().any(|c|
@@ -444,7 +475,13 @@ pub fn authenticate(context: &HuskContext, cert: &ValidCert, role: Role, activat
 
 /// Make `cert` the new signing key.
 pub fn set_signing_key(context: &HuskContext, cert: &ValidCert) -> anyhow::Result<()> {
-    internal_authenticate(context, cert, Role::SigningKey, true, None, Some((HUSK_SIG_NOTATION.to_string(), "placeholder".to_string())))
+    internal_authenticate(
+        context,
+        cert,
+        Role::SigningKey,
+        true,
+        None,
+        Some((HUSK_SIG_NOTATION.to_string(), "placeholder".to_string())))
 }
 
 /// Internal authentication method.
@@ -521,6 +558,7 @@ fn internal_authenticate(context: &HuskContext, cert: &ValidCert, role: Role, ac
             false)?;
     }
 
+    // XXX Check if there are better ways then to use user ids.
     // For a certification, user ids are needed. Build a list of
     // valid (non self revoked) user ids.
     let user_ids: Vec<_> = cert.userids()
@@ -588,7 +626,13 @@ pub fn can_sign(cert: &ValidCert) -> bool {
         .count() > 0
 }
 
-
+// Get the signing key:
+// - get all keys from the keystore.
+// - get the corresponding certificates.
+// - check the user ids of those certificates for characteristic notations.
+// We start with keys as there will be way more certificates than keys, thus
+// reducing the number of certificates we check.
+// XXX find a more efficient way.
 /// Get Husks signing key
 pub fn get_signing_key(sequoia_home: Home, cert_store: &CertStore<'_>, policy: &StandardPolicy<'_>, local_trust_root: &Cert) -> anyhow::Result<Option<Key>> {
 
@@ -640,7 +684,8 @@ pub fn contains_signing_key(vc: &ValidCert, local_root_fpr: &Fingerprint) -> Opt
 }
 
 /// Get all keys from all backends
-// XXX: Maybe the softkeys backend is sufficient
+// XXX: Maybe the softkeys backend is sufficient. As we are using a custom
+// SEQUOIA_HOME, the gpg-agent backend is disabled.
 pub fn get_all_keys(sequoia_home: Home) -> Vec<Key> {
 
     let mut result = Vec::new();
@@ -671,7 +716,7 @@ pub fn get_all_keys(sequoia_home: Home) -> Vec<Key> {
     result
 }
 
-pub fn import_key(sequoia_home: Home, cert: &Cert) -> bool {
+pub fn import_key(sequoia_home: Home, cert: &Cert) -> anyhow::Result<()> {
     let mut imported = false;
     std::thread::scope(|s| {
         s.spawn(|| {
@@ -694,7 +739,11 @@ pub fn import_key(sequoia_home: Home, cert: &Cert) -> bool {
         });
     });
 
-    imported
+    if imported {
+        Ok(())
+    } else {
+        Err(HuskError::KeyImportFailed.into())
+    }
 }
 
 #[cfg(test)]

src/common/process_descr.rs

@@ -59,8 +59,8 @@ impl ProcessDescr {
             let mut descr_file = File::create(&location_path)?;
             let content = ProcessDescr {
                 pid: pid as usize,
-                name: name,
+                name,
-                started: started,
+                started,
                 file_location: location,
             };
             descr_file.write_all(toml::to_string(&content)?.as_bytes())?;

src/daemon/mod.rs

@@ -99,7 +99,7 @@ impl Daemon {
         let mut sender: Option<String> = None;
 
         for arg in args {
-            if let Some(s) = mail::to_email(&arg) {
+            if let Ok(s) = mail::to_email(&arg) {
                 sender = Some(s);
                 break;
             }
@@ -135,7 +135,7 @@ impl Daemon {
 
             let mut rcpt: Option<String> = None;
             for arg in args {
-                if let Some(s) = mail::to_email(&arg) {
+                if let Ok(s) = mail::to_email(&arg) {
                     rcpt = Some(s);
                     break;
                 }
@@ -171,15 +171,20 @@ impl Daemon {
         }
     }
 
-    async fn handle_eoh(_cx: &mut Context<HuskContext<'_>>) -> Status {
+    async fn handle_eoh(cx: &mut Context<HuskContext<'_>>) -> Status {
         log::debug!("EOH");
 
-        // XXX: check for early accepting
+        if let Some(ref mut context) = cx.data {
-        // if there are no recipients with authenticated certificates
+            if context.mail.encryptable() {
-        // tell the MTA that this milter is done and the mail can be further
+                Status::Continue
-        // processed.
+            } else {
-
+                // We cannot encrypt the mail, signal the MTA that we
-        Status::Continue
+                // are done.
+                Status::Accept
+            }
+        } else {
+            Status::Tempfail
+        }
     }
 
     async fn handle_body(cx: &mut Context<HuskContext<'_>>, chunk: Bytes) -> Status {

src/mail.rs

@@ -19,19 +19,20 @@ use crate::types::errors::HuskError;
 use crate::types::recipient::Recipient;
 
 /// Parse the user id fields from a string.
-pub fn to_userid(rcpt: &CString) -> Option<UserID> {
+pub fn to_userid(rcpt: &CString) -> Result<UserID> {
-    match rcpt.to_str() {
+    let rcpt = rcpt.to_str()?;
-        Ok(s) => Some(UserID::from(s)),
+    Ok(UserID::from(rcpt))
-        _ => None
-    }
 }
 
 /// Get the email from the string representation of a user id.
-pub fn to_email(rcpt: &CString) -> Option<String> {
+pub fn to_email(rcpt: &CString) -> Result<String> {
-    if let Some(u) = to_userid(rcpt) {
+    let u = to_userid(rcpt)?;
-       if let Ok(Some(e)) = u.email() { Some(e.to_string()) }
+
-       else { None }
+    if let Some(e) = u.email()? {
-    } else { None }
+        Ok(e.to_string())
+    } else {
+        Err(HuskError::CannotConvertToEmail(u.to_string()).into())
+    }
 }
 
 /// Checks if the parameter has a valid email format.
@@ -78,7 +79,7 @@ pub async fn smtp_inject(from: String, to: &Vec<Recipient<'_>>, header: CHeader,
 
     // convert addresses
     let from_email = CString::new(from.clone())?;
-    let from_email = to_email(&from_email).unwrap(); // XXX
+    let from_email = to_email(&from_email)?;
     let from_email: Address = from_email.try_into()?;
 
     let mut rcpts: Vec<Address> = Vec::new();
@@ -87,7 +88,8 @@ pub async fn smtp_inject(from: String, to: &Vec<Recipient<'_>>, header: CHeader,
         rcpts.push(rcpt.try_into()?);
     }
 
-    let my_id = ClientId::Domain(defaults::HELO_REINJECT_ID.to_owned()); // TODO: add secret token?
+    // XXX: add secret token?
+    let my_id = ClientId::Domain(defaults::HELO_REINJECT_ID.to_owned());
     joinset.spawn(async move {
 
         // XXX

src/types/cheader.rs

@@ -118,6 +118,12 @@ impl fmt::Display for CHeader {
     }
 }
 
+impl Default for CHeader {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 #[cfg(test)]
 pub mod tests {
     use std::ffi::CString;

src/types/errors.rs

@@ -26,4 +26,10 @@ pub enum HuskError {
     NoUserIDs(String),
     #[error("Failing to send mail: {0}")]
     SmtpInjectionFailure(String),
+    #[error("Cannot use certificate as key")]
+    CannotUseCertificateAsKey,
+    #[error("Key import failed")]
+    KeyImportFailed,
+    #[error("Cannot convert to email: {0}")]
+    CannotConvertToEmail(String),
 }

src/types/introducer.rs

@@ -1,5 +1,9 @@
 //! introducers
 
+use std::hash::Hash;
+use std::hash::Hasher;
+use std::fmt::{Display, Formatter};
+
 use anyhow::Result;
 
 use sequoia_wot as wot;
@@ -105,13 +109,14 @@ impl Introducer {
     }
 }
 
-use std::fmt::{Display, Formatter};
 
 impl Display for Introducer {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        // XXX: This is unsafe as ANSI control chars are not handled.
         let uids = self.cert.self_signed_userids()
             .map(|u| String::from_utf8_lossy(u.userid().value()).to_string())
+            .map(|u| u.chars()
+                .map(|c| if c.is_control() { '_' } else { c })
+                .collect::<String>())
             .collect::<Vec<String>>()
             .join(", ");
 
@@ -119,7 +124,7 @@ impl Display for Introducer {
         writeln!(f, "  UserIDs: {}", uids)?;
 
         if let Some(regex_set) = self.certification.regular_expressions() {
-            for regex in regex_set.as_bytes().into_iter() {
+            for regex in regex_set.as_bytes().iter() {
                 let regex = String::from_utf8_lossy(regex);
                 if let Ok(domain) = escape::regex_domain_unescape(regex.to_string().as_str()) {
                     writeln!(f,"  Domain: {}", domain)?;
@@ -132,3 +137,80 @@ impl Display for Introducer {
     }
 }
 
+impl PartialEq for Introducer {
+    fn eq(&self, other: &Self) -> bool {
+        self.cert.fingerprint() == other.cert.fingerprint()
+        && self.certification.regular_expressions() == other.certification.regular_expressions()
+    }
+}
+
+impl Eq for Introducer { }
+
+impl Hash for Introducer {
+    fn hash<H: Hasher>(&self, state: &mut H) {
+        self.cert.fingerprint().hash(state);
+        if let Some(r) = self.certification.regular_expressions() {
+            r.as_bytes().hash(state);
+        }
+    }
+}
+
+#[cfg(test)]
+pub mod tests {
+    use std::iter;
+    use std::time::SystemTime;
+    use std::hash::Hash;
+    use std::hash::Hasher;
+    use std::hash::DefaultHasher;
+    use sequoia_openpgp::Fingerprint;
+    use sequoia_wot::{CertSynopsis, Certification, RevocationStatus, UserIDSynopsis};
+
+    use super::Introducer;
+
+
+    #[test]
+    // XXX improve, add RegExes
+    fn introducer_equality() {
+
+        let alice_fpr: Fingerprint = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"
+            .parse().expect("valid fingerprint");
+        let alice_uid = UserIDSynopsis::from("<alice@example.com>");
+        let alice = CertSynopsis::new(alice_fpr.clone(),
+                        None, RevocationStatus::NotAsFarAsWeKnow,
+                        iter::once(alice_uid.clone()));
+
+        let bob_fpr: Fingerprint = "BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"
+            .parse().expect("valid fingerprint");
+        let bob_uid = UserIDSynopsis::from("<bob@example.com>");
+        let bob = CertSynopsis::new(bob_fpr,
+                        None, RevocationStatus::NotAsFarAsWeKnow,
+                        iter::once(bob_uid));
+
+        let alice_intro = Introducer {
+            cert: CertSynopsis::new(alice_fpr.clone(),
+                        None, RevocationStatus::NotAsFarAsWeKnow,
+                        iter::once(alice_uid.clone())),
+            certification: Certification::new(bob.clone(), Some(alice_uid.userid().clone()), alice.clone(), SystemTime::now()),
+        };
+
+        let alice_twin = Introducer {
+            cert: CertSynopsis::new(alice_fpr,
+                        None, RevocationStatus::NotAsFarAsWeKnow,
+                        iter::once(alice_uid.clone())),
+            certification: Certification::new(bob.clone(), Some(alice_uid.userid().clone()), alice.clone(), SystemTime::now()),
+        };
+
+        assert!(alice_intro == alice_twin);
+
+        let mut hasher = DefaultHasher::new();
+        alice_intro.hash(&mut hasher);
+        let alice_hash = hasher.finish();
+
+        let mut hasher = DefaultHasher::new();
+        alice_twin.hash(&mut hasher);
+        let alice_twin_hash = hasher.finish();
+
+        assert!(alice_hash == alice_twin_hash);
+
+    }
+}

src/types/mail_context.rs

@@ -19,6 +19,8 @@ pub enum ProtectionPossibility<'mc> {
     // no recipient has a cert
     NoProtection,
     // some recipients have a cert (but not all)
+    // The first param are encryptable recipients, the second the
+    // unencryptable.
     Partial(Vec<Recipient<'mc>>, Vec<Recipient<'mc>>),
     // all recipients have a cert
     Full
@@ -147,6 +149,14 @@ impl<'mc> MailContext<'mc> {
         outer
     }
 
+    /// Fast check for recipients with certificates.
+    pub fn encryptable(&self) -> bool {
+        for rcpt in &self.recipients {
+            if rcpt.can_encrypt() { return true; }
+        }
+        false
+    }
+
 }
 
 /// Default Mailcontext

src/types/mime/header.rs

@@ -44,6 +44,12 @@ impl fmt::Display for Header<'_> {
     }
 }
 
+impl<'h> Default for Header<'h> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/types/mime/multi_part.rs

@@ -25,7 +25,7 @@ impl<'m> MultiPart<'m> {
         MultiPart {
             header: Header::new(),
             parts: Vec::new(),
-            boundary: boundary
+            boundary
         }
     }
 
@@ -88,6 +88,11 @@ impl fmt::Display for MultiPart<'_> {
     }
 }
 
+impl<'m> Default for MultiPart<'m> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
 
 #[cfg(test)]
 mod tests {

src/types/mime/part.rs

@@ -15,14 +15,14 @@ impl<'p> Part<'p> {
     pub fn with_body(body: Bytes) -> Self {
         Part {
             header: Header::new(),
-            body: body
+            body
         }
     }
 
     pub fn from_body(body: Bytes) -> Self {
         Part {
             header: Header::new(),
-            body: body
+            body
         }
     }