diff --git a/src/commands/key/approvals.rs b/src/commands/key/approvals.rs index 7019ced9..32e28590 100644 --- a/src/commands/key/approvals.rs +++ b/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 - // we want approved. + // It's approved and we want pending, or it's pending + // 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. - if let Some(cert) = sig.get_issuers().iter().find_map( + // Add if the issuer is in --add-by. + 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(cert) = sig.get_issuers().iter().find_map( - |i| store.lookup_by_cert(i).unwrap_or_default().into_iter() - .find_map( - |cert| sig.verify_signature(&cert.primary_key()) - .is_ok().then_some(cert))) - { - // We found the certifier. - if cert.userids().any( - |u| network.authenticate(u, cert.fingerprint(), + if let Some(issuer) = issuer.as_ref() { + if let Some((ref network, threshold)) = network_threshold { + if issuer.userids().any( + |u| network.authenticate(u.userid(), + issuer.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() diff --git a/tests/data/keys/_sequoia_ca_keys.openpgp.org.pgp b/tests/data/keys/_sequoia_ca_keys.openpgp.org.pgp new file mode 100644 index 00000000..7802f3df --- /dev/null +++ b/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----- diff --git a/tests/integration/sq_key_approvals_update.rs b/tests/integration/sq_key_approvals_update.rs index 80f29711..1d517e5f 100644 --- a/tests/integration/sq_key_approvals_update.rs +++ b/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); +}