Don't add approvals for non-exportable certifications or certs.

- Change `sq key approvals list` and `sq key approvals update` to
    ignore certifications that are not exportable, and certificates
    that are not exportable, or are a shadow CA.

  - Fixes #402.

src/commands/key/approvals.rs

@@ -11,8 +11,9 @@ use sequoia_cert_store::Store;
 use sequoia_wot as wot;
 
 use crate::Sq;
-use crate::cli;
 use crate::cli::key::approvals;
+use crate::cli;
+use crate::common::ca_creation_time;
 
 pub fn dispatch(sq: Sq, command: approvals::Command)
                 -> Result<()>
@@ -50,14 +51,27 @@ fn list(sq: Sq, cmd: approvals::ListCommand) -> Result<()> {
 
         let mut any = false;
         for c in uid.certifications() {
+            // Ignore non-exportable certifications.
+            if c.exportable().is_err() {
+                sq.info(format_args!(
+                    "Ignoring non-exportable certification from {} on {}.",
+                    c.get_issuers()
+                        .into_iter()
+                        .next()
+                        .map(|kh| kh.to_string())
+                        .unwrap_or_else(|| "unknown certificate".to_string()),
+                    String::from_utf8_lossy(uid.value())));
+                continue;
+            }
+
             let approved = approved.contains(c);
             if ! approved {
                 pending += 1;
             }
 
             if approved == cmd.pending {
-                // It's approved and we pending, or it's pending and
+                // It's approved and we want pending, or it's pending
-                // we want approved.
+                // and we want approved.
                 continue;
             }
 
@@ -75,6 +89,25 @@ fn list(sq: Sq, cmd: approvals::ListCommand) -> Result<()> {
                 }
             }
 
+            // If the certificate should not be exported, we don't
+            // approve the certification.
+            if let Some(Ok(i)) = issuer.as_ref().map(|i| i.to_cert()) {
+                if ! i.exportable() {
+                    sq.info(format_args!(
+                        "Ignoring certification from non-exportable \
+                         certificate {} on {}.",
+                        i.fingerprint(), String::from_utf8_lossy(uid.value())));
+                    continue;
+                }
+                if i.primary_key().creation_time() == ca_creation_time() {
+                    sq.info(format_args!(
+                        "Ignoring certification from local shadow CA \
+                         {} on {}.",
+                        i.fingerprint(), String::from_utf8_lossy(uid.value())));
+                    continue;
+                }
+            }
+
             wprintln!(initial_indent = "   - ", "{}{}: {}",
                       issuer.as_ref()
                       .map(|c| format!("{} ", c.fingerprint()))
@@ -180,20 +213,103 @@ fn update(
         // Selectively add approvals.
         let next_approved_cloned = next_approved.clone();
         for sig in uid.certifications()
-        // Don't consider those that we already approved.
+            // Don't consider those that we already approved.
             .filter(|s| ! next_approved_cloned.contains(s))
-        // Don't consider those explicitly removed.
-            .filter(|s| ! s.get_issuers().iter().any(
-                // Quadratic, but how bad can it be...?
-                |i| command.remove_by.iter().any(|j| i.aliases(j))))
         {
+            // Ignore non-exportable certifications.
+            if sig.exportable().is_err() {
+                sq.info(format_args!(
+                    "Ignoring non-exportable certification from {} on {}.",
+                    sig.get_issuers()
+                        .into_iter()
+                        .next()
+                        .map(|kh| kh.to_string())
+                        .unwrap_or_else(|| "unknown certificate".to_string()),
+                    String::from_utf8_lossy(uid.value())));
+                continue;
+            }
+
+            // Try and get the issuer's certificate.
+            let mut issuer = None;
+            let mut err = None;
+            for i in sig.get_issuers().into_iter()
+                .filter_map(|i| store.lookup_by_cert(&i).ok()
+                            .map(IntoIterator::into_iter))
+                .flatten()
+            {
+                match sig.verify_signature(&i.primary_key()) {
+                    Ok(_) => {
+                        issuer = Some(i)
+                    }
+                    Err(e) => err = Some((i.fingerprint(), e)),
+                }
+            }
+
+            if issuer.is_none() {
+                if let Some((fpr, err)) = err {
+                    // We have the alleged signer, but we couldn't
+                    // verify the certification.  It's bad; silently
+                    // ignore it.
+                    sq.info(format_args!(
+                        "Ignoring invalid certification from {}: {}",
+                        fpr, err));
+                    continue;
+                }
+            }
+
+            // Convert it from a lazy cert to a cert.
+            let issuer = if let Some(Ok(i))
+                = issuer.as_ref().map(|i| i.to_cert())
+            {
+                Some(i)
+            } else {
+                None
+            };
+
+            // If the certificate should not be exported, we don't
+            // approve the certification.
+            if let Some(i) = issuer.as_ref() {
+                if ! i.exportable() {
+                    sq.info(format_args!(
+                        "Ignoring certification from non-exportable \
+                         certificate {} on {}.",
+                        i.fingerprint(), String::from_utf8_lossy(uid.value())));
+                    continue;
+                }
+                if i.primary_key().creation_time() == ca_creation_time() {
+                    sq.info(format_args!(
+                        "Ignoring certification from local shadow CA \
+                         {} on {}.",
+                        i.fingerprint(), String::from_utf8_lossy(uid.value())));
+                    continue;
+                }
+            }
+
+            // Skip if the issuer is in --remove-by.
+            if let Some(issuer) = issuer.as_ref() {
+                if command.remove_by.iter().any(|j| issuer.key_handle().aliases(j)) {
+                    continue;
+                }
+            } else if ! sig.get_issuers().iter().any(
+                // Quadratic, but how bad can it be...?
+                |i| command.remove_by.iter().any(|j| i.aliases(j)))
+            {
+                continue;
+            }
+
+            // Add if --add-all is passed.
             if command.add_all {
                 next_approved.insert(sig);
                 continue;
             }
 
-            // Add by issuer handle.
+            // Add if the issuer is in --add-by.
-            if let Some(cert) = sig.get_issuers().iter().find_map(
+            if let Some(issuer) = issuer.as_ref() {
+                if command.add_by.iter().any(|j| issuer.key_handle().aliases(j)) {
+                    next_approved.insert(sig);
+                    continue;
+                }
+            } else if let Some(cert) = sig.get_issuers().iter().find_map(
                 // Quadratic, but how bad can it be...?
                 |i| add_by.iter().find_map(
                     |cert| i.aliases(cert.key_handle()).then_some(cert)))
@@ -205,16 +321,11 @@ fn update(
             }
 
             // Add authenticated certifiers.
-            if let Some((ref network, threshold)) = network_threshold {
+            if let Some(issuer) = issuer.as_ref() {
-                if let Some(cert) = sig.get_issuers().iter().find_map(
+                if let Some((ref network, threshold)) = network_threshold {
-                    |i| store.lookup_by_cert(i).unwrap_or_default().into_iter()
+                    if issuer.userids().any(
-                        .find_map(
+                        |u| network.authenticate(u.userid(),
-                            |cert| sig.verify_signature(&cert.primary_key())
+                                                 issuer.fingerprint(),
-                                .is_ok().then_some(cert)))
-                {
-                    // We found the certifier.
-                    if cert.userids().any(
-                        |u| network.authenticate(u, cert.fingerprint(),
                                                  threshold)
                             .amount() >= threshold)
                     {
@@ -251,7 +362,7 @@ fn update(
                 removed += 1;
             }
 
-            wprintln!(initial_indent = "  ", "{} {}: {}",
+            wprintln!(initial_indent = "  ", "{} {}{}: {}",
                       match (prev, next) {
                           (false, false) => '.',
                           (true, false) => '-',
@@ -259,6 +370,9 @@ fn update(
                           (true, true) => '=',
                       },
                       issuer.as_ref()
+                      .map(|c| format!("{} ", c.fingerprint()))
+                      .unwrap_or_else(|| "".into()),
+                      issuer.as_ref()
                       .and_then(|i| Some(sq.best_userid(i.to_cert().ok()?, true)
                                          .to_string()))
                       .or(c.get_issuers().into_iter().next()

tests/data/keys/_sequoia_ca_keys.openpgp.org.pgp

@@ -0,0 +1,16 @@
+-----BEGIN PGP PRIVATE KEY BLOCK-----
+
+xVgEPHQAuBYJKwYBBAHaRw8BAQdAdj1Bdymbgv661mg69NOMiEbo2SblOrBFQCBz
+SWIls2sAAP4yVCgMXysVntnVhzFErsGcQ9K44zuFHJEEKZPldWMt+A9pwsAOBB8W
+CgCABYI8dAC4AoQAAwsJBwkQoXk4gaUwvGhHFAAAAAAAHgAgc2FsdEBub3RhdGlv
+bnMuc2VxdW9pYS1wZ3Aub3Jnym9gn2Srmqbsbc9wAprNMKt76LUshY3X7RxPTwQN
+O0sDFQoIApsBAh4BFiEETCXLbIWHf12JX8DwoXk4gaUwvGgAAEcwAP48y0OuUYaA
+cfQl/+cHCFmQNwV5MdPN17Vp2CJVAuFKIAEAj1p/tsZRvvKfxY/PIlPbzdbcL38B
+xUgOo6t4ThYYYQPNIERvd25sb2FkZWQgZnJvbSBrZXlzLm9wZW5wZ3Aub3JnwsAR
+BBMWCgCDBYI8dAC4AoQAAwsJBwkQoXk4gaUwvGhHFAAAAAAAHgAgc2FsdEBub3Rh
+dGlvbnMuc2VxdW9pYS1wZ3Aub3JnXYn7g9ifxe2I7n2L5780u+XbbdayGe8KcJLL
+adtE8iIDFQoIApkBApsBAh4BFiEETCXLbIWHf12JX8DwoXk4gaUwvGgAAOjvAQCQ
+gG6j/pAOYZTmoFB2gWDgvaAs329+ZEUsFCiJcNbDwAD/UY9sbeM/jpn/bCrvIeeR
+xe/IG6KCqkZrMgDnFmCIGAw=
+=S9qn
+-----END PGP PRIVATE KEY BLOCK-----

tests/integration/sq_key_approvals_update.rs

@@ -1,13 +1,14 @@
 use std::ffi::OsStr;
 use std::path::Path;
 
-use super::common::{Sq, STANDARD_POLICY};
+use super::common::{artifact, Sq, STANDARD_POLICY};
 
 use sequoia_openpgp as openpgp;
 use openpgp::{
     Cert,
     Result,
     cert::amalgamation::ValidateAmalgamation,
+    parse::Parse,
 };
 
 #[test]
@@ -236,3 +237,80 @@ fn update_authenticated() -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn ignore_shadow_ca() {
+    // Check that update ignores certificates made by shadow CAs.
+    let now = std::time::SystemTime::now()
+        - std::time::Duration::new(60 * 60, 0);
+
+    let sq = Sq::at(now);
+    let (alice, bob) = make_keys(&sq).unwrap();
+
+    // Have Bob certify Alice.
+    let alice2 = sq.pki_vouch_certify(&[],
+                                      bob.key_handle(),
+                                      alice.key_handle(),
+                                      &[ALICE_USERID],
+                                      None);
+    assert_eq!(alice2.fingerprint(), alice.fingerprint());
+
+    let shadow_ca = artifact("keys/_sequoia_ca_keys.openpgp.org.pgp");
+    sq.key_import(&shadow_ca);
+    let shadow_ca = Cert::from_file(&shadow_ca).unwrap();
+
+    // Have the shadow CA certify Alice.
+    let alice2 = sq.pki_vouch_certify(&[],
+                                      &shadow_ca.key_handle(),
+                                      alice.key_handle(),
+                                      &[ALICE_USERID],
+                                      None);
+    assert_eq!(alice2.fingerprint(), alice.fingerprint());
+
+    // Attest to all certifications.  This should ignore the shadow
+    // CA's certification.
+    let approval = sq.key_approvals_update(
+        &alice.key_handle(), &["--add-all"], None);
+
+    assert_eq!(approval.bad_signatures().count(), 0);
+    let approval_ua = approval.userids().next().unwrap();
+    // We have an attestation key signature.
+    assert_eq!(approval_ua.attestations().count(), 1);
+    // With one attestation (not two!).
+    assert_eq!(approval_ua.with_policy(STANDARD_POLICY, None).unwrap()
+               .attested_certifications().count(), 1);
+}
+
+#[test]
+fn ignore_unexportable_certifications() {
+    // Check that update ignores certificates that are not exportable.
+    let now = std::time::SystemTime::now()
+        - std::time::Duration::new(60 * 60, 0);
+
+    let sq = Sq::at(now);
+    let (alice, bob) = make_keys(&sq).unwrap();
+
+    // Have Bob create a non-exportable certification for Alice.
+    let alice2 = sq.pki_vouch_certify(&["--local"],
+                                      bob.key_handle(),
+                                      alice.key_handle(),
+                                      &[ALICE_USERID],
+                                      None);
+    assert_eq!(alice2.fingerprint(), alice.fingerprint());
+
+    // Attest to all certifications.  This should ignore
+    // non-exportable certifications.
+    let approval = sq.key_approvals_update(
+        &alice.key_handle(), &["--add-all"], None);
+
+    assert_eq!(approval.bad_signatures().count(), 0);
+    let approval_ua = approval.userids().next().unwrap();
+    for attestation in approval_ua.attestations() {
+        eprintln!(" - {:?}", attestation);
+    }
+    // We have an attestation key signature.
+    assert_eq!(approval_ua.attestations().count(), 1);
+    // With zero attestations.
+    assert_eq!(approval_ua.with_policy(STANDARD_POLICY, None).unwrap()
+               .attested_certifications().count(), 0);
+}