diff --git a/src/commands/key/delete.rs b/src/commands/key/delete.rs index 2385875e..15b4fcfd 100644 --- a/src/commands/key/delete.rs +++ b/src/commands/key/delete.rs @@ -1,14 +1,30 @@ -//! Changes key expiration. +//! Deletes all of a certificate's secret key material. + +use anyhow::Context; + +use sequoia_openpgp as openpgp; +use openpgp::Cert; use crate::Result; use crate::Sq; -use crate::cli::types::KeyDesignators; use crate::cli; use crate::common::key::delete; pub fn dispatch(sq: Sq, command: cli::key::delete::Command) -> Result<()> { - delete::delete(sq, command.cert, KeyDesignators::none(), + let (cert, cert_source) + = sq.resolve_cert(&command.cert, sequoia_wot::FULLY_TRUSTED)?; + + let vc = Cert::with_policy(&cert, sq.policy, sq.time) + .with_context(|| { + format!("The certificate {} is not valid under the \ + current policy.", + cert.fingerprint()) + })?; + + let kas = vc.keys().collect::>(); + + delete::delete(sq, &cert, cert_source, &kas, command.output, false) } diff --git a/src/commands/key/password.rs b/src/commands/key/password.rs index 33077f6c..1895156e 100644 --- a/src/commands/key/password.rs +++ b/src/commands/key/password.rs @@ -1,15 +1,31 @@ //! Changes a key's password. +use anyhow::Context; + +use sequoia_openpgp as openpgp; +use openpgp::Cert; + use crate::Result; use crate::Sq; -use crate::cli::types::KeyDesignators; use crate::cli; use crate::common::key::password; pub fn dispatch(sq: Sq, command: cli::key::password::Command) -> Result<()> { - password::password(sq, command.cert, KeyDesignators::none(), + let (cert, cert_source) + = sq.resolve_cert(&command.cert, sequoia_wot::FULLY_TRUSTED)?; + + let vc = Cert::with_policy(&cert, sq.policy, sq.time) + .with_context(|| { + format!("The certificate {} is not valid under the \ + current policy.", + cert.fingerprint()) + })?; + + let kas = vc.keys().collect::>(); + + password::password(sq, &cert, cert_source, &kas, command.clear_password, command.new_password_file.as_deref(), command.output, false) diff --git a/src/commands/key/subkey/delete.rs b/src/commands/key/subkey/delete.rs index 72bd28ee..f4003ba2 100644 --- a/src/commands/key/subkey/delete.rs +++ b/src/commands/key/subkey/delete.rs @@ -1,12 +1,32 @@ +//! Deletes one or more key's secret key material. + +use anyhow::Context; + +use sequoia_openpgp as openpgp; +use openpgp::Cert; + use crate::Result; use crate::Sq; use crate::common::key::delete; +use crate::common::NULL_POLICY; pub fn dispatch(sq: Sq, command: crate::cli::key::subkey::delete::Command) -> Result<()> { assert!(! command.keys.is_empty()); - delete(sq, command.cert, Some(command.keys), - command.output, false) + let (cert, cert_source) + = sq.resolve_cert(&command.cert, sequoia_wot::FULLY_TRUSTED)?; + + let vc = Cert::with_policy(&cert, NULL_POLICY, sq.time) + .with_context(|| { + format!("The certificate {} is not valid under the \ + null policy.", + cert.fingerprint()) + })?; + + let kas = sq.resolve_keys(&vc, &cert_source, &command.keys, true)?; + + delete::delete(sq, &cert, cert_source, &kas, + command.output, false) } diff --git a/src/commands/key/subkey/password.rs b/src/commands/key/subkey/password.rs index 0c957d2e..195c92f3 100644 --- a/src/commands/key/subkey/password.rs +++ b/src/commands/key/subkey/password.rs @@ -1,13 +1,34 @@ +//! Changes the password protecting one or more keys. + +use anyhow::Context; + +use sequoia_openpgp as openpgp; +use openpgp::Cert; + use crate::Result; use crate::Sq; use crate::common::key::password; +use crate::common::NULL_POLICY; pub fn dispatch(sq: Sq, command: crate::cli::key::subkey::password::Command) -> Result<()> { assert!(! command.keys.is_empty()); - password(sq, command.cert, Some(command.keys), + let (cert, cert_source) + = sq.resolve_cert(&command.cert, sequoia_wot::FULLY_TRUSTED)?; + + let vc = Cert::with_policy(&cert, NULL_POLICY, sq.time) + .with_context(|| { + format!("The certificate {} is not valid under the \ + null policy.", + cert.fingerprint()) + })?; + + let kas = sq.resolve_keys(&vc, &cert_source, &command.keys, true)?; + + + password(sq, &cert, cert_source, &kas, command.clear_password, command.new_password_file.as_deref(), command.output, false) } diff --git a/src/common/key.rs b/src/common/key.rs index 2d228767..0cf4bc99 100644 --- a/src/common/key.rs +++ b/src/common/key.rs @@ -1,19 +1,15 @@ -use anyhow::Context; - use sequoia_openpgp as openpgp; -use openpgp::Cert; use openpgp::Result; use openpgp::cert::amalgamation::key::PrimaryKey; -use openpgp::packet::key; +use openpgp::cert::amalgamation::key::ValidErasedKeyAmalgamation; use openpgp::packet::Key; +use openpgp::packet::key::KeyParts; +use openpgp::packet::key; use sequoia_keystore as keystore; use crate::Sq; -use crate::cli::types::CertDesignators; use crate::cli::types::FileStdinOrKeyHandle; -use crate::cli::types::KeyDesignators; -use crate::cli::types::cert_designator; mod expire; @@ -37,52 +33,23 @@ pub use password::password; /// secret key material. /// /// The returned keys are not unlocked. -pub fn get_keys( +pub fn get_keys<'a, P>( sq: &Sq, - cert: &CertDesignators, - keys: Option<&KeyDesignators>) - -> Result<(Cert, - FileStdinOrKeyHandle, - Vec<(Key, - bool, - Option>)>)> -where CP: cert_designator::ArgumentPrefix, - KO: typenum::Unsigned, + cert_source: &FileStdinOrKeyHandle, + kas: &[ValidErasedKeyAmalgamation<'a, P>]) + -> Result, + bool, + Option>)>> +where + P: 'a + KeyParts, { - assert_eq!(cert.len(), 1); - if let Some(keys) = keys { - assert!(keys.len() > 0); - } - - let (cert, cert_source) - = sq.resolve_cert(&cert, sequoia_wot::FULLY_TRUSTED)?; - - let vc = Cert::with_policy(&cert, sq.policy, sq.time) - .with_context(|| { - format!("The certificate {} is not valid under the \ - current policy.", - cert.fingerprint()) - })?; - - let kas = if let Some(keys) = keys { - sq.resolve_keys(&vc, &cert_source, &keys, true)? - } else { - vc.keys().collect::>() - }; - let mut list: Vec<(Key<_, _>, bool, Option<_>)> = Vec::new(); let mut no_secret_key_material_count = 0; match cert_source { - FileStdinOrKeyHandle::FileOrStdin(ref file) => { + FileStdinOrKeyHandle::FileOrStdin(ref _file) => { // If it is not a TSK, there is nothing to do. - if ! cert.is_tsk() { - return Err(anyhow::anyhow!( - "{} (read from {}) does not contain any secret \ - key material.", - cert.fingerprint(), file)); - } for ka in kas.into_iter() { let no_secret_key_material = ! ka.has_secret(); if no_secret_key_material { @@ -92,7 +59,10 @@ where CP: cert_designator::ArgumentPrefix, continue; } - list.push((ka.key().clone(), ka.primary(), None)); + list.push(( + ka.key().clone().parts_into_public(), + ka.primary(), + None)); } } FileStdinOrKeyHandle::KeyHandle(ref _kh) => { @@ -110,7 +80,10 @@ where CP: cert_designator::ArgumentPrefix, continue; } - list.push((ka.key().clone(), ka.primary(), Some(remote_keys))); + list.push(( + ka.key().clone().parts_into_public(), + ka.primary(), + Some(remote_keys))); } } } @@ -129,5 +102,5 @@ where CP: cert_designator::ArgumentPrefix, assert!(! list.is_empty()); - Ok((cert, cert_source, list)) + Ok(list) } diff --git a/src/common/key/delete.rs b/src/common/key/delete.rs index fc288940..b9e490e6 100644 --- a/src/common/key/delete.rs +++ b/src/common/key/delete.rs @@ -2,31 +2,31 @@ use anyhow::Context; use sequoia_openpgp as openpgp; -use openpgp::Result; +use openpgp::Cert; use openpgp::Packet; +use openpgp::Result; +use openpgp::cert::amalgamation::key::ValidErasedKeyAmalgamation; +use openpgp::packet::key::KeyParts; use openpgp::serialize::Serialize; use crate::Sq; -use crate::cli::types::CertDesignators; use crate::cli::types::FileOrStdout; use crate::cli::types::FileStdinOrKeyHandle; -use crate::cli::types::KeyDesignators; -use crate::cli::types::cert_designator; use super::get_keys; -pub fn delete( +pub fn delete<'a, P>( sq: Sq, - cert: CertDesignators, - keys: Option>, + cert: &Cert, + cert_source: FileStdinOrKeyHandle, + kas: &[ValidErasedKeyAmalgamation<'a, P>], output: Option, binary: bool) -> Result<()> -where CP: cert_designator::ArgumentPrefix, - KO: typenum::Unsigned, +where + P: 'a + KeyParts, { - let (cert, cert_source, to_delete) - = get_keys(&sq, &cert, keys.as_ref())?; + let to_delete = get_keys(&sq, &cert_source, kas)?; let ks = matches!(cert_source, FileStdinOrKeyHandle::KeyHandle(_)); if ks { @@ -55,7 +55,7 @@ where CP: cert_designator::ArgumentPrefix, } } - let cert = cert.insert_packets( + let cert = cert.clone().insert_packets( stripped.into_iter().map(|stripped| Packet::from(stripped)))?; let output = output.unwrap_or_else(|| FileOrStdout::new(None)); diff --git a/src/common/key/password.rs b/src/common/key/password.rs index 8579161a..58ae6cc8 100644 --- a/src/common/key/password.rs +++ b/src/common/key/password.rs @@ -3,34 +3,35 @@ use std::path::Path; use anyhow::Context; use sequoia_openpgp as openpgp; -use openpgp::crypto::Password; -use openpgp::serialize::Serialize; +use openpgp::Cert; use openpgp::Packet; use openpgp::Result; +use openpgp::cert::amalgamation::key::ValidErasedKeyAmalgamation; +use openpgp::crypto::Password; +use openpgp::packet::key::KeyParts; +use openpgp::serialize::Serialize; use sequoia_keystore as keystore; use keystore::Protection; use crate::common; use crate::Sq; -use crate::cli::types::CertDesignators; use crate::cli::types::FileOrStdout; use crate::cli::types::FileStdinOrKeyHandle; -use crate::cli::types::KeyDesignators; -use crate::cli::types::cert_designator; use crate::common::password; -pub fn password( +pub fn password<'a, P>( sq: Sq, - cert: CertDesignators, - keys: Option>, + cert: &Cert, + cert_source: FileStdinOrKeyHandle, + kas: &[ValidErasedKeyAmalgamation<'a, P>], clear_password: bool, new_password_file: Option<&Path>, output: Option, binary: bool) -> Result<()> -where CP: cert_designator::ArgumentPrefix, - KO: typenum::Unsigned, +where + P: 'a + KeyParts, { let mut new_password_ = None; // Some(password) => new password @@ -49,8 +50,7 @@ where CP: cert_designator::ArgumentPrefix, Ok(new_password_.clone().unwrap()) }; - let (cert, cert_source, mut list) - = super::get_keys(&sq, &cert, keys.as_ref())?; + let mut list = super::get_keys(&sq, &cert_source, kas)?; let uid = sq.best_userid(&cert, true); let ks = matches!(cert_source, FileStdinOrKeyHandle::KeyHandle(_)); @@ -156,7 +156,7 @@ where CP: cert_designator::ArgumentPrefix, } } - let cert = cert.insert_packets(packets)?; + let cert = cert.clone().insert_packets(packets)?; let output = output.unwrap_or_else(|| FileOrStdout::new(None)); let mut output = output.for_secrets().create_safe(&sq)?; diff --git a/tests/integration/common.rs b/tests/integration/common.rs index 2b28dab6..9d851f5f 100644 --- a/tests/integration/common.rs +++ b/tests/integration/common.rs @@ -1233,12 +1233,12 @@ impl Sq { .expect("can parse certificate") } - /// Delete the specified key. - pub fn key_subkey_delete<'a, H, Q>(&self, - cert_handle: H, - key_handles: &[KeyHandle], - output_file: Q) - -> Cert + /// Delete the specified keys. + pub fn try_key_subkey_delete<'a, H, Q>(&self, + cert_handle: H, + key_handles: &[KeyHandle], + output_file: Q) + -> Result where H: Into, Q: Into>, { @@ -1251,10 +1251,17 @@ impl Sq { match &cert_handle { FileOrKeyHandle::FileOrStdin(path) => { cmd.arg("--cert-file").arg(path); - assert!(output_file.is_some()); + if let Some(output_file) = output_file { + cmd.arg("--output").arg(output_file); + } else { + cmd.arg("--output").arg("-"); + } } FileOrKeyHandle::KeyHandle((_kh, s)) => { cmd.arg("--cert").arg(&s); + if let Some(output_file) = output_file { + cmd.arg("--output").arg(output_file); + } } }; @@ -1262,25 +1269,32 @@ impl Sq { cmd.arg("--key").arg(kh.to_string()); } - if let Some(output_file) = output_file { - cmd.arg("--output").arg(output_file); - } - - let output = self.run(cmd, Some(true)); - assert!(output.status.success()); + let output = self.run(cmd, None); self.handle_cert_output(output, cert_handle, output_file, None) - .expect("can parse certificate") + } + + /// Delete the specified keys. + pub fn key_subkey_delete<'a, H, Q>(&self, + cert_handle: H, + key_handles: &[KeyHandle], + output_file: Q) + -> Cert + where H: Into, + Q: Into>, + { + self.try_key_subkey_delete(cert_handle, key_handles, output_file) + .expect("success") } /// Change the key's password. - pub fn key_subkey_password<'a, H, Q>(&self, - cert_handle: H, - keys: &[KeyHandle], - old_password_file: Option<&'a Path>, - new_password_file: Option<&'a Path>, - output_file: Q, - success: bool) - -> Result + pub fn try_key_subkey_password<'a, H, Q>( + &self, + cert_handle: H, + keys: &[KeyHandle], + old_password_file: Option<&'a Path>, + new_password_file: Option<&'a Path>, + output_file: Q) + -> Result where H: Into, Q: Into>, @@ -1293,9 +1307,16 @@ impl Sq { if cert_handle.is_file() { cmd.arg("--cert-file").arg(&cert_handle); - assert!(output_file.is_some()); + if let Some(output_file) = output_file { + cmd.arg("--output").arg(output_file); + } else { + cmd.arg("--output").arg("-"); + } } else { cmd.arg("--cert").arg(&cert_handle); + if let Some(output_file) = output_file { + cmd.arg("--output").arg(output_file); + } }; for key in keys.iter() { @@ -1312,14 +1333,29 @@ impl Sq { cmd.arg("--clear-password"); } - if let Some(output_file) = output_file { - cmd.arg("--output").arg(output_file); - } - - let output = self.run(cmd, Some(success)); + let output = self.run(cmd, None); self.handle_cert_output(output, cert_handle, output_file, None) } + /// Change the key's password. + pub fn key_subkey_password<'a, H, Q>(&self, + cert_handle: H, + keys: &[KeyHandle], + old_password_file: Option<&'a Path>, + new_password_file: Option<&'a Path>, + output_file: Q) + -> Cert + where + H: Into, + Q: Into>, + { + self.try_key_subkey_password( + cert_handle, keys, + old_password_file, new_password_file, + output_file) + .expect("success") + } + /// Change the key's expiration. pub fn key_subkey_expire<'a, H, Q>(&self, cert_handle: H, diff --git a/tests/integration/sq_key_subkey_delete.rs b/tests/integration/sq_key_subkey_delete.rs index 7012ef47..c7da3954 100644 --- a/tests/integration/sq_key_subkey_delete.rs +++ b/tests/integration/sq_key_subkey_delete.rs @@ -1,11 +1,80 @@ use sequoia_openpgp as openpgp; +use openpgp::Cert; use openpgp::KeyHandle; use openpgp::KeyID; use openpgp::Result; +use openpgp::cert::amalgamation::ValidAmalgamation; use openpgp::packet::Key; +use openpgp::parse::Parse; +use openpgp::types::RevocationStatus; +use super::common::FileOrKeyHandle; use super::common::power_set; use super::common::Sq; +use super::common::STANDARD_POLICY; + +fn check<'a, H>( + sq: &Sq, + cert_handle: H, + to_delete: &[KeyHandle], + success: bool) +where H: Into, +{ + let cert_handle = cert_handle.into(); + + eprintln!("Deleting keys from {:?}:", cert_handle); + for k in to_delete.iter() { + eprintln!(" - {}", k); + } + + // Delete the selection. + let result = sq.try_key_subkey_delete(&cert_handle, to_delete, None); + let got = match (success, result) { + (true, Ok(cert)) => cert, + (true, Err(err)) => { + panic!("Failed, but should have succeeded: {}", err) + } + (false, Ok(_)) => { + panic!("Succeded, but should have failed") + } + (false, Err(_)) => return, + }; + + // Make sure we got exactly what we asked for; no + // more, no less. + eprintln!("Result:"); + + let mut deletions = 0; + for got in got.keys() { + eprintln!(" {} {} secret key material", + got.fingerprint(), + if got.has_secret() { + "has" + } else { + "doesn't have" + }); + + let should_have_deleted + = to_delete.iter().find(|kh| kh.aliases(got.key_handle())).is_some(); + + if should_have_deleted { + assert!( + ! got.has_secret(), + "got secret key material \ + for a key we should have deleted ({})", + got.fingerprint()); + + deletions += 1; + } else { + assert!( + got.has_secret(), + "didn't get secret key material \ + for a key we didn't delete ({})", + got.fingerprint()); + } + } + assert_eq!(deletions, to_delete.len()); +} #[test] fn sq_key_subkey_delete() -> Result<()> @@ -44,12 +113,8 @@ fn sq_key_subkey_delete() -> Result<()> } else { cert_file.display().to_string() }); - eprintln!(" Deleting:"); - for k in to_delete.iter() { - eprintln!(" {}", k); - } - let to_delete_kh: Vec = if by_fpr { + let to_delete: Vec = if by_fpr { to_delete.iter() .map(|fpr| KeyHandle::from(fpr)) .collect() @@ -59,54 +124,130 @@ fn sq_key_subkey_delete() -> Result<()> .collect() }; - // Delete the selection. - let got = if keystore { + if keystore { // Import it into the key store. sq.key_import(&cert_file); - - sq.key_subkey_delete( - cert.key_handle(), &to_delete_kh, None) + check(&sq, cert.key_handle(), &to_delete, true); } else { - sq.key_subkey_delete( - &cert_file, &to_delete_kh, - std::path::PathBuf::from("-").as_path()) - }; - - // Make sure we got exactly what we asked for; no - // more, no less. - eprintln!(" Got:"); - - let mut deletions = 0; - for got in got.keys() { - eprintln!(" {} {} secret key material", - got.fingerprint(), - if got.has_secret() { - "has" - } else { - "doesn't have" - }); - - let should_have_deleted - = to_delete.contains(&got.fingerprint()); - - if should_have_deleted { - assert!( - ! got.has_secret(), - "got secret key material \ - for a key we deleted ({})", - got.fingerprint()); - - deletions += 1; - } else { - assert!( - got.has_secret(), - "didn't get secret key material \ - for a key we didn't delete ({})", - got.fingerprint()); - } + check(&sq, &cert_file, &to_delete, true); } - assert_eq!(deletions, to_delete.len()); } Ok(()) } + +#[test] +fn unbound_subkey() { + // Make sure we can't delete an unbound subkey. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("unbound-subkey.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // One subkey should be considered invalid. + assert!(vc.keys().count() < cert.keys().count()); + + let unbound = "E992BF8BA7A27BB4FBB71D973857E47B14874045" + .parse::().expect("valid"); + + check(&sq, &cert_path, &[ unbound ], false); +} + +#[test] +fn soft_revoked_subkey() { + // Make sure we can delete a soft revoked subkey. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("soft-revoked-subkey.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // Make sure the revoked key is there and is really revoked. + let mut revoked = None; + for k in vc.keys().subkeys() { + if let RevocationStatus::Revoked(_) = k.revocation_status() { + assert!(revoked.is_none(), + "Only expected a single revoked subkey"); + revoked = Some(k.key_handle()); + } + } + let revoked = if let Some(revoked) = revoked { + revoked + } else { + panic!("Expected a revoked subkey, but didn't fine one"); + }; + + check(&sq, &cert_path, &[ revoked ], true); +} + +#[test] +fn hard_revoked_subkey() { + // Make sure we can delete a hard revoked subkey. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("hard-revoked-subkey.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // Make sure the revoked key is there and is really revoked. + let mut revoked = None; + for k in vc.keys().subkeys() { + if let RevocationStatus::Revoked(_) = k.revocation_status() { + assert!(revoked.is_none(), + "Only expected a single revoked subkey"); + revoked = Some(k.key_handle()); + } + } + let revoked = if let Some(revoked) = revoked { + revoked + } else { + panic!("Expected a revoked subkey, but didn't fine one"); + }; + + check(&sq, &cert_path, &[ revoked ], true); +} + +#[test] +fn sha1_subkey() { + // Make sure we can delete a subkey that is bound using SHA-1. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("sha1-subkey-priv.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // Make sure the subkey key is there and really uses SHA-1. + let valid_subkeys: Vec<_> = vc.keys().subkeys() + .map(|ka| ka.fingerprint()) + .collect(); + let all_subkeys: Vec<_> = cert.keys().subkeys() + .map(|ka| ka.fingerprint()) + .collect(); + + assert_eq!(valid_subkeys.len(), 0); + assert_eq!(all_subkeys.len(), 1); + + let subkey = all_subkeys[0].clone(); + check(&sq, &cert_path, &[ KeyHandle::from(subkey) ], true); +} diff --git a/tests/integration/sq_key_subkey_password.rs b/tests/integration/sq_key_subkey_password.rs index 0a49437e..ebc7a232 100644 --- a/tests/integration/sq_key_subkey_password.rs +++ b/tests/integration/sq_key_subkey_password.rs @@ -1,11 +1,101 @@ +use std::path::Path; + use sequoia_openpgp as openpgp; +use openpgp::Cert; use openpgp::KeyHandle; use openpgp::KeyID; use openpgp::Result; +use openpgp::cert::amalgamation::ValidAmalgamation; use openpgp::packet::Key; +use openpgp::parse::Parse; +use openpgp::types::RevocationStatus; use super::common::power_set; use super::common::Sq; +use super::common::STANDARD_POLICY; + +fn check( + sq: &Sq, + cert_file: &Path, + to_change: &[KeyHandle], + success: bool) +{ + let cert = Cert::from_file(&cert_file).expect("can read"); + + eprintln!("Changing password for {:?}:", cert_file); + for k in to_change.iter() { + eprintln!(" - {}", k); + } + + let password = sq.scratch_file("password"); + std::fs::write(&password, "this is a super secret password") + .expect("can write"); + + for keystore in [false, true] { + // Change the password for the selection. + let result = if keystore { + // Import it into the key store. + sq.key_import(&cert_file); + + sq.try_key_subkey_password( + cert.key_handle(), &to_change, + None, Some(&password), + None) + } else { + sq.try_key_subkey_password( + cert_file, &to_change, + None, Some(&password), + None) + }; + let got = match (success, result) { + (true, Ok(cert)) => cert, + (true, Err(err)) => { + panic!("Failed, but should have succeeded: {}", err) + } + (false, Ok(_)) => { + panic!("Succeded, but should have failed") + } + (false, Err(_)) => return, + }; + + // Make sure we got exactly what we asked for; no + // more, no less. + eprintln!(" Got:"); + + let mut changes = 0; + for got in got.keys() { + eprintln!(" {} {} encrypted secret key material", + got.fingerprint(), + if got.has_unencrypted_secret() { + "doesn't have" + } else { + "has" + }); + + let should_have_changed = to_change.iter() + .find(|kh| kh.aliases(got.key_handle())) + .is_some(); + + if should_have_changed { + assert!( + ! got.has_unencrypted_secret(), + "got unencrypted secret key material \ + for a key whose password we changed ({})", + got.fingerprint()); + + changes += 1; + } else { + assert!( + got.has_unencrypted_secret(), + "didn't get encrypted secret key material \ + for a key whose password we changed ({})", + got.fingerprint()); + } + } + assert_eq!(changes, to_change.len()); + } +} + #[test] fn sq_key_subkey_password_0() -> Result<()> { @@ -31,9 +121,6 @@ fn sq_key_subkey_password_mod(modulus: usize) -> Result<()> { let sq = Sq::new(); - let password_txt = sq.scratch_file("password_txt"); - std::fs::write(&password_txt, "a new password 1234").expect("can write"); - // Generate a key in a file. let (cert, cert_file, _rev_file) = sq.key_generate(&[], &["alice"]); assert!(cert.is_tsk()); @@ -53,26 +140,21 @@ fn sq_key_subkey_password_mod(modulus: usize) -> Result<()> let key_ids = keys.iter().map(|k| k.fingerprint()).collect::>(); - for (((i, to_change), keystore), by_fpr) in power_set(&key_ids).into_iter() + for ((i, to_change), by_fpr) in power_set(&key_ids).into_iter() .enumerate() .filter(|(i, _)| i % 4 == modulus) .flat_map(|x| [(x.clone(), false), (x.clone(), true)]) - .flat_map(|x| [(x.clone(), false), (x.clone(), true)]) { eprintln!("Test #{}, by {}, from {}:", i + 1, if by_fpr { "fingerprint" } else { "key ID" }, - if keystore { - "the key store".to_string() - } else { - cert_file.display().to_string() - }); + cert_file.display().to_string()); eprintln!(" Changing the password for:"); for k in to_change.iter() { eprintln!(" {}", k); } - let to_change_kh: Vec = if by_fpr { + let to_change: Vec = if by_fpr { to_change.iter() .map(|fpr| KeyHandle::from(fpr)) .collect() @@ -82,59 +164,124 @@ fn sq_key_subkey_password_mod(modulus: usize) -> Result<()> .collect() }; - // Change the password for the selection. - let got = if keystore { - // Import it into the key store. - sq.key_import(&cert_file); - - sq.key_subkey_password( - cert.key_handle(), &to_change_kh, - None, Some(&password_txt), - None, true) - .expect("can change password") - } else { - sq.key_subkey_password( - &cert_file, &to_change_kh, - None, Some(&password_txt), - std::path::PathBuf::from("-").as_path(), true) - .expect("can change password") - }; - - // Make sure we got exactly what we asked for; no - // more, no less. - eprintln!(" Got:"); - - let mut changes = 0; - for got in got.keys() { - eprintln!(" {} {} encrypted secret key material", - got.fingerprint(), - if got.has_unencrypted_secret() { - "doesn't have" - } else { - "has" - }); - - let should_have_changed - = to_change.contains(&got.fingerprint()); - - if should_have_changed { - assert!( - ! got.has_unencrypted_secret(), - "got unencrypted secret key material \ - for a key whose password we changed ({})", - got.fingerprint()); - - changes += 1; - } else { - assert!( - got.has_unencrypted_secret(), - "didn't get encrypted secret key material \ - for a key whose password we changed ({})", - got.fingerprint()); - } - } - assert_eq!(changes, to_change.len()); + check(&sq, &cert_file, &to_change, true); } Ok(()) } + +#[test] +fn unbound_subkey() { + // Make sure we can't delete an unbound subkey. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("unbound-subkey.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // One subkey should be considered invalid. + assert!(vc.keys().count() < cert.keys().count()); + + let unbound = "E992BF8BA7A27BB4FBB71D973857E47B14874045" + .parse::().expect("valid"); + + check(&sq, &cert_path, &[ unbound ], false); +} + +#[test] +fn soft_revoked_subkey() { + // Make sure we can delete a soft revoked subkey. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("soft-revoked-subkey.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // Make sure the revoked key is there and is really revoked. + let mut revoked = None; + for k in vc.keys().subkeys() { + if let RevocationStatus::Revoked(_) = k.revocation_status() { + assert!(revoked.is_none(), + "Only expected a single revoked subkey"); + revoked = Some(k.key_handle()); + } + } + let revoked = if let Some(revoked) = revoked { + revoked + } else { + panic!("Expected a revoked subkey, but didn't fine one"); + }; + + check(&sq, &cert_path, &[ revoked ], true); +} + +#[test] +fn hard_revoked_subkey() { + // Make sure we can delete a hard revoked subkey. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("hard-revoked-subkey.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // Make sure the revoked key is there and is really revoked. + let mut revoked = None; + for k in vc.keys().subkeys() { + if let RevocationStatus::Revoked(_) = k.revocation_status() { + assert!(revoked.is_none(), + "Only expected a single revoked subkey"); + revoked = Some(k.key_handle()); + } + } + let revoked = if let Some(revoked) = revoked { + revoked + } else { + panic!("Expected a revoked subkey, but didn't fine one"); + }; + + check(&sq, &cert_path, &[ revoked ], true); +} + +#[test] +fn sha1_subkey() { + // Make sure we can delete a subkey that is bound using SHA-1. + + let sq = Sq::new(); + + let cert_path = sq.test_data() + .join("keys") + .join("sha1-subkey-priv.pgp"); + + let cert = Cert::from_file(&cert_path).expect("can read"); + let vc = cert.with_policy(STANDARD_POLICY, sq.now()) + .expect("valid cert"); + + // Make sure the subkey key is there and really uses SHA-1. + let valid_subkeys: Vec<_> = vc.keys().subkeys() + .map(|ka| ka.fingerprint()) + .collect(); + let all_subkeys: Vec<_> = cert.keys().subkeys() + .map(|ka| ka.fingerprint()) + .collect(); + + assert_eq!(valid_subkeys.len(), 0); + assert_eq!(all_subkeys.len(), 1); + + let subkey = all_subkeys[0].clone(); + check(&sq, &cert_path, &[ KeyHandle::from(subkey) ], true); +}