Compute best-effort primary user ids, escape them when displaying.

- They may contain special characters and trick the user.  The
    current way to detect dodgy characters and escape them may not be
    the best, but it is better than the status quo, and we now encode
    intent.

src/commands/import.rs

@@ -12,7 +12,11 @@ use sequoia_cert_store as cert_store;
 use cert_store::LazyCert;
 use cert_store::StoreUpdate;
 
-use crate::Config;
+use crate::{
+    Config,
+    best_effort_primary_uid,
+    output::sanitize::Safe,
+};
 use crate::cli::import;
 use crate::cli::types::FileOrStdin;
 
@@ -33,6 +37,8 @@ pub fn dispatch<'store>(mut config: Config<'store>, cmd: import::Command)
             let input = FileOrStdin::from(input).open()?;
             let raw_certs = RawCertParser::from_reader(input)?;
 
+            let policy = config.policy.clone();
+            let time = config.time;
             let cert_store = config.cert_store_mut_or_else()?;
 
             for raw_cert in raw_certs {
@@ -46,18 +52,15 @@ pub fn dispatch<'store>(mut config: Config<'store>, cmd: import::Command)
                 };
 
                 let fingerprint = cert.fingerprint();
-                let userid = cert.userids().next()
-                    .map(|userid| {
-                        String::from_utf8_lossy(userid.value()).to_string()
-                    })
-                    .unwrap_or_else(|| "<unknown>".to_string());
+                let userid = best_effort_primary_uid(
+                    cert.to_cert()?, &policy, time).clone();
                 if let Err(err) = cert_store.update_by(Cow::Owned(cert), &mut stats) {
                     eprintln!("Error importing {}, {:?}: {}",
                               fingerprint, userid, err);
                     stats.errors += 1;
                     continue;
                 } else {
-                    eprintln!("Imported {}, {:?}", fingerprint, userid);
+                    eprintln!("Imported {}, {}", fingerprint, Safe(&userid));
                 }
             }
         }

src/commands/net.rs

@@ -23,7 +23,6 @@ use openpgp::{
         UserID,
     },
     parse::Parse,
-    policy::NullPolicy,
     types::SignatureType,
 };
 use sequoia_net as net;
@@ -43,8 +42,10 @@ use crate::{
         active_certification,
         get_certification_keys,
     },
+    output::sanitize::Safe,
     Config,
     Model,
+    best_effort_primary_uid,
     merge_keyring,
     serialize_keyring,
     output::WkdUrlVariant,
@@ -53,8 +54,6 @@ use crate::{
 
 use crate::cli;
 
-const NP: NullPolicy = NullPolicy::new();
-
 /// Import the certificates into the local certificate store.
 ///
 /// This does not certify the certificates.
@@ -64,14 +63,9 @@ pub fn import_certs(config: &mut Config, certs: Vec<Cert>) -> Result<()> {
     let certs = merge_keyring(certs)?.into_values()
         .map(|cert| {
             let fpr = cert.fingerprint();
-            let userid = cert.with_policy(&config.policy, config.time)
-                .or_else(|_| cert.with_policy(&NP, config.time))
-                .and_then(|vc| vc.primary_userid())
-                .map(|userid| {
-                    String::from_utf8_lossy(userid.value())
-                        .to_string()
-                })
-                .unwrap_or_else(|_| "<unknown>".to_string());
+            let userid =
+                best_effort_primary_uid(&cert, &config.policy, config.time)
+                .clone();
 
             (fpr, userid, cert)
         })
@@ -86,8 +80,8 @@ pub fn import_certs(config: &mut Config, certs: Vec<Cert>) -> Result<()> {
     eprintln!("Importing {} certificates into the certificate store:\n", certs.len());
     for (i, (fpr, userid, cert)) in certs.into_iter().enumerate() {
         cert_store.update_by(Cow::Owned(cert.into()), &mut stats)
-            .with_context(|| format!("Inserting {}, {}", fpr, userid))?;
-        eprintln!("  {}. {} {}", i + 1, fpr, userid);
+            .with_context(|| format!("Inserting {}, {}", fpr, Safe(&userid)))?;
+        eprintln!("  {}. {} {}", i + 1, fpr, Safe(&userid));
     }
 
     eprintln!("\nImported {} new certificates, \

src/output.rs

@@ -10,6 +10,8 @@ use std::io::Write;
 use anyhow::{anyhow, Result};
 use serde::Serialize;
 
+pub mod sanitize;
+
 pub use keyring::ListItem as KeyringListItem;
 pub use wkd::WkdUrlVariant;
 

src/output/sanitize.rs

@@ -0,0 +1,30 @@
+//! Safely displays various data types from untrusted sources.
+
+use std::fmt;
+
+use sequoia_openpgp::{
+    packet::UserID,
+};
+
+/// Safely displays values.
+pub struct Safe<T>(pub T);
+
+impl fmt::Display for Safe<&UserID> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        let s = String::from_utf8_lossy(self.0.value());
+        write!(f, "{}", Safe(s.as_ref()))
+    }
+}
+
+impl fmt::Display for Safe<&str> {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        // XXX: Better detect dodgy strings and better sanitize them.
+        // I bet there is a crate for that.  For now, this is better
+        // than the status quo, and it encodes intent.
+        if self.0.chars().any(char::is_control) {
+            write!(f, "{:?}", self.0)
+        } else {
+            write!(f, "{}", self.0)
+        }
+    }
+}

src/sq.rs

@@ -35,7 +35,7 @@ use openpgp::packet::signature::subpacket::NotationData;
 use openpgp::packet::signature::subpacket::NotationDataFlags;
 use openpgp::serialize::Serialize;
 use openpgp::cert::prelude::*;
-use openpgp::policy::StandardPolicy as P;
+use openpgp::policy::{Policy, StandardPolicy as P};
 use openpgp::serialize::SerializeInto;
 use openpgp::types::KeyFlags;
 use openpgp::types::RevocationStatus;
@@ -193,6 +193,56 @@ fn serialize_keyring(mut output: &mut dyn io::Write, certs: Vec<Cert>,
     Ok(())
 }
 
+/// Best-effort heuristic to compute the primary User ID of a given cert.
+pub fn best_effort_primary_uid<'u, T>(cert: &'u Cert,
+                                      policy: &'u dyn Policy,
+                                      time: T)
+                                      -> &'u UserID
+where
+    T: Into<Option<SystemTime>>,
+{
+    let time = time.into();
+
+    // Try to be more helpful by including a User ID in the
+    // listing.  We'd like it to be the primary one.  Use
+    // decreasingly strict policies.
+    let mut primary_uid = None;
+
+    // First, apply our policy.
+    if let Ok(vcert) = cert.with_policy(policy, time) {
+        if let Ok(primary) = vcert.primary_userid() {
+            primary_uid = Some(primary.userid());
+        }
+    }
+
+    // Second, apply the null policy.
+    if primary_uid.is_none() {
+        const NULL: openpgp::policy::NullPolicy =
+            openpgp::policy::NullPolicy::new();
+        if let Ok(vcert) = cert.with_policy(&NULL, time) {
+            if let Ok(primary) = vcert.primary_userid() {
+                primary_uid = Some(primary.userid());
+            }
+        }
+    }
+
+    // As a last resort, pick the first user id.
+    if primary_uid.is_none() {
+        if let Some(primary) = cert.userids().next() {
+            primary_uid = Some(primary.userid());
+        } else {
+            // Special case, there is no user id.
+            use once_cell::sync::Lazy;
+            static FALLBACK: Lazy<UserID> = Lazy::new(|| {
+                UserID::from("<unknown>")
+            });
+            primary_uid = Some(&FALLBACK);
+        }
+    }
+
+    primary_uid.expect("set at this point")
+}
+
 // Decrypts a key, if possible.
 //
 // The passwords in `passwords` are tried first.  If the key can't be