Change sq key subkey {password,delete} to work with weak bindings.

- Currently, it is not possible to delete secret key material that
    is only associated with a certificate that is not valid under the
    current policy.  The same goes for changing the password protecting
    the secret key material.

  - Users shouldn't have to first update a key's binding signature to
    delete it, or change its password.

  - Change `sq key subkey delete` and `sq key subkey password` to use
    the null policy.  This is not a security concern, because even if
    the binding signature is weak, both the certificate and the key
    are explicitly named.

  - See #375

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::<Vec<_>>();
+
+    delete::delete(sq, &cert, cert_source, &kas,
                    command.output, false)
 }

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::<Vec<_>>();
+
+    password::password(sq, &cert, cert_source, &kas,
                        command.clear_password,
                        command.new_password_file.as_deref(),
                        command.output, false)

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),
+    let (cert, cert_source)
-           command.output, false)
+        = 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)
 }

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)
 }

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<CA, CP, CO, CD, KO, KD>(
+pub fn get_keys<'a, P>(
     sq: &Sq,
-    cert: &CertDesignators<CA, CP, CO, CD>,
+    cert_source: &FileStdinOrKeyHandle,
-    keys: Option<&KeyDesignators<KO, KD>>)
+    kas: &[ValidErasedKeyAmalgamation<'a, P>])
-    -> Result<(Cert,
+    -> Result<Vec<(Key<key::PublicParts, key::UnspecifiedRole>,
-               FileStdinOrKeyHandle,
+                   bool,
-               Vec<(Key<key::PublicParts, key::UnspecifiedRole>,
+                   Option<Vec<keystore::Key>>)>>
-                    bool,
+where
-                    Option<Vec<keystore::Key>>)>)>
+    P: 'a + KeyParts,
-where CP: cert_designator::ArgumentPrefix,
-      KO: typenum::Unsigned,
 {
-    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::<Vec<_>>()
-    };
-
     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)
 }

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<CA, CP, CO, CD, KO, KD>(
+pub fn delete<'a, P>(
     sq: Sq,
-    cert: CertDesignators<CA, CP, CO, CD>,
+    cert: &Cert,
-    keys: Option<KeyDesignators<KO, KD>>,
+    cert_source: FileStdinOrKeyHandle,
+    kas: &[ValidErasedKeyAmalgamation<'a, P>],
     output: Option<FileOrStdout>,
     binary: bool)
     -> Result<()>
-where CP: cert_designator::ArgumentPrefix,
+where
-      KO: typenum::Unsigned,
+    P: 'a + KeyParts,
 {
-    let (cert, cert_source, to_delete)
+    let to_delete = get_keys(&sq, &cert_source, kas)?;
-        = get_keys(&sq, &cert, keys.as_ref())?;
 
     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));

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::Cert;
-use openpgp::serialize::Serialize;
 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<CA, CP, CO, CD, KO, KD>(
+pub fn password<'a, P>(
     sq: Sq,
-    cert: CertDesignators<CA, CP, CO, CD>,
+    cert: &Cert,
-    keys: Option<KeyDesignators<KO, KD>>,
+    cert_source: FileStdinOrKeyHandle,
+    kas: &[ValidErasedKeyAmalgamation<'a, P>],
     clear_password: bool,
     new_password_file: Option<&Path>,
     output: Option<FileOrStdout>,
     binary: bool)
     -> Result<()>
-where CP: cert_designator::ArgumentPrefix,
+where
-      KO: typenum::Unsigned,
+    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)
+    let mut list = super::get_keys(&sq, &cert_source, kas)?;
-        = super::get_keys(&sq, &cert, keys.as_ref())?;
     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)?;

tests/integration/common.rs

@@ -1233,12 +1233,12 @@ impl Sq {
             .expect("can parse certificate")
     }
 
-    /// Delete the specified key.
+    /// Delete the specified keys.
-    pub fn key_subkey_delete<'a, H, Q>(&self,
+    pub fn try_key_subkey_delete<'a, H, Q>(&self,
-                                       cert_handle: H,
+                                           cert_handle: H,
-                                       key_handles: &[KeyHandle],
+                                           key_handles: &[KeyHandle],
-                                       output_file: Q)
+                                           output_file: Q)
-        -> Cert
+        -> Result<Cert>
     where H: Into<FileOrKeyHandle>,
           Q: Into<Option<&'a Path>>,
     {
@@ -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 {
+        let output = self.run(cmd, None);
-            cmd.arg("--output").arg(output_file);
-        }
-
-        let output = self.run(cmd, Some(true));
-        assert!(output.status.success());
         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<FileOrKeyHandle>,
+          Q: Into<Option<&'a Path>>,
+    {
+        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,
+    pub fn try_key_subkey_password<'a, H, Q>(
-                                         cert_handle: H,
+        &self,
-                                         keys: &[KeyHandle],
+        cert_handle: H,
-                                         old_password_file: Option<&'a Path>,
+        keys: &[KeyHandle],
-                                         new_password_file: Option<&'a Path>,
+        old_password_file: Option<&'a Path>,
-                                         output_file: Q,
+        new_password_file: Option<&'a Path>,
-                                         success: bool)
+        output_file: Q)
-                                         -> Result<Cert>
+        -> Result<Cert>
     where
         H: Into<FileOrKeyHandle>,
         Q: Into<Option<&'a Path>>,
@@ -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 {
+        let output = self.run(cmd, None);
-            cmd.arg("--output").arg(output_file);
-        }
-
-        let output = self.run(cmd, Some(success));
         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<FileOrKeyHandle>,
+        Q: Into<Option<&'a Path>>,
+    {
+        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,

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<FileOrKeyHandle>,
+{
+    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<KeyHandle> = if by_fpr {
+        let to_delete: Vec<KeyHandle> = 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.
+        if keystore {
-        let got = if keystore {
             // Import it into the key store.
             sq.key_import(&cert_file);
-
+            check(&sq, cert.key_handle(), &to_delete, true);
-            sq.key_subkey_delete(
-                cert.key_handle(), &to_delete_kh, None)
         } else {
-            sq.key_subkey_delete(
+            check(&sq, &cert_file, &to_delete, true);
-                &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());
-            }
         }
-        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::<KeyHandle>().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);
+}

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::<Vec<_>>();
 
-    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 {
+                  cert_file.display().to_string());
-                      "the key store".to_string()
-                  } else {
-                      cert_file.display().to_string()
-                  });
         eprintln!("  Changing the password for:");
         for k in to_change.iter() {
             eprintln!("    {}", k);
         }
 
-        let to_change_kh: Vec<KeyHandle> = if by_fpr {
+        let to_change: Vec<KeyHandle> = 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.
+        check(&sq, &cert_file, &to_change, true);
-        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());
     }
 
     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::<KeyHandle>().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);
+}