New subcommand sq key subkey delete to delete secret key material.

NEWS

@@ -2,6 +2,10 @@
 #+TITLE: sequoia-sq NEWS – history of user-visible changes
 #+STARTUP: content hidestars
 
+* Changes in 0.38.0
+** Notable changes
+   - New subcommand `sq key subkey delete` to delete secret key
+     material.
 * Changes in 0.37.0
 ** Notable changes
    - Remove PKS support.

src/cli/key.rs

@@ -1161,25 +1161,6 @@ User ID."
     pub binary: bool,
 }
 
-#[derive(Debug, Subcommand)]
-#[clap(
-    name = "subkey",
-    about = "Manage subkeys",
-    long_about = "\
-Manage subkeys.
-
-Add new subkeys to an existing certificate, change their expiration, \
-and revoke them.",
-    subcommand_required = true,
-    arg_required_else_help = true,
-)]
-#[non_exhaustive]
-pub enum SubkeyCommand {
-    Add(SubkeyAddCommand),
-    Expire(SubkeyExpireCommand),
-    Revoke(SubkeyRevokeCommand),
-}
-
 const ADOPT_EXAMPLES: Actions = Actions {
     actions: &[
         Action::Example(Example {
@@ -1365,6 +1346,26 @@ modified certificate to stdout.",
     pub binary: bool,
 }
 
+#[derive(Debug, Subcommand)]
+#[clap(
+    name = "subkey",
+    about = "Manage subkeys",
+    long_about = "\
+Manage subkeys.
+
+Add new subkeys to an existing certificate, change their expiration, \
+and revoke them.",
+    subcommand_required = true,
+    arg_required_else_help = true,
+)]
+#[non_exhaustive]
+pub enum SubkeyCommand {
+    Add(SubkeyAddCommand),
+    Delete(SubkeyDeleteCommand),
+    Expire(SubkeyExpireCommand),
+    Revoke(SubkeyRevokeCommand),
+}
+
 const SUBKEY_ADD_EXAMPLES: Actions = Actions {
     actions: &[
         Action::Example(Example {
@@ -1526,6 +1527,100 @@ modified certificate to stdout.",
 }
 
 
+const SQ_KEY_SUBKEY_DELETE_EXAMPLES: Actions = Actions {
+    actions: &[
+        Action::Example(Example {
+            comment: "\
+Import Alice's key.",
+            command: &[
+                "sq", "key", "import", "alice-secret.pgp",
+            ],
+        }),
+        Action::Example(Example {
+            comment: "\
+Delete Alice's signing subkey.",
+            command: &[
+                "sq", "key", "subkey", "delete",
+                "--cert", "EB28F26E2739A4870ECC47726F0073F60FD0CBF0",
+                "--key", "42020B87D51877E5AF8D272124F3955B0B8DECC8",
+            ],
+        }),
+    ]
+};
+test_examples!(sq_key_subkey_delete, SQ_KEY_SUBKEY_DELETE_EXAMPLES);
+
+#[derive(Debug, Args)]
+#[clap(
+    name = "delete",
+    about = "Delete a certificate's secret key material",
+    long_about = "\
+Delete a certificate's secret key material.
+
+Unlike `sq key delete`, which deletes all the secret key material, this \
+command only deletes the specified secret key material.
+
+Although the secret key material is deleted, the public keys are \
+retained.  If you don't want the keys to be used anymore you should \
+revoke the keys using `sq key subkey revoke`.",
+    after_help = SQ_KEY_SUBKEY_DELETE_EXAMPLES,
+)]
+#[clap(group(ArgGroup::new("cert_input").args(&["cert_file", "cert"]).required(true)))]
+pub struct SubkeyDeleteCommand {
+    #[clap(
+        long,
+        help = "Delete secret key material from the specified certificate",
+        value_name = FileOrStdin::VALUE_NAME,
+    )]
+    pub cert: Option<KeyHandle>,
+    #[clap(
+        long,
+        value_name = "CERT_FILE",
+        help = "Delete secret key material from the specified certificate",
+        long_help = "\
+Delete secret key material from the specified certificate.
+
+Read the certificate from FILE or stdin, if `-`.  It is an error \
+for the file to contain more than one certificate.",
+    )]
+    pub cert_file: Option<FileOrStdin>,
+    #[clap(
+        long,
+        value_name = "FINGERPRINT|KEYID",
+        required = true,
+        help = "The keys to delete",
+        long_help = "\
+The keys to delete.
+
+The specified keys may be either the primary key or subkeys.
+
+If the secret key material is managed by multiple devices, it is \
+deleted from all of them.",
+    )]
+    pub key: Vec<KeyHandle>,
+
+    #[clap(
+        long,
+        short,
+        value_name = FileOrStdout::VALUE_NAME,
+        conflicts_with = "cert",
+        help = "Write the stripped certificate to the specified file",
+        long_help = "\
+Write the stripped certificate to the specified file.
+
+This option only makes sense when deleting the secret key material from a \
+file.  When deleting secret key material managed by the key store using \
+`--cert`, you can get the stripped certificate using `sq key export`.",
+    )]
+    pub output: Option<FileOrStdout>,
+
+    #[clap(
+        short = 'B',
+        long,
+        help = "Emit binary data",
+    )]
+    pub binary: bool,
+}
+
 const SQ_KEY_SUBKEY_EXPIRE_EXAMPLES: Actions = Actions {
     actions: &[
         Action::Example(Example {

src/commands/key/delete.rs

@@ -17,5 +17,5 @@ pub fn dispatch(sq: Sq, command: cli::key::DeleteCommand)
         panic!("clap enforces --cert or --cert-file is set");
     };
 
-    delete::delete(sq, handle, command.output, command.binary)
+    delete::delete(sq, handle, Vec::new(), command.output, command.binary)
 }

src/commands/key/subkey.rs

@@ -18,12 +18,14 @@ use openpgp::Result;
 use crate::Sq;
 use crate::cli::key::SubkeyAddCommand;
 use crate::cli::key::SubkeyCommand;
+use crate::cli::key::SubkeyDeleteCommand;
 use crate::cli::key::SubkeyExpireCommand;
 use crate::cli::key::SubkeyRevokeCommand;
 use crate::cli::types::EncryptPurpose;
 use crate::cli::types::FileOrStdout;
 use crate::common;
 use crate::common::expire;
+use crate::common::delete;
 use crate::common::NULL_POLICY;
 use crate::common::RevocationOutput;
 use crate::common::get_secret_signer;
@@ -32,6 +34,7 @@ use crate::parse_notations;
 pub fn dispatch(sq: Sq, command: SubkeyCommand) -> Result<()> {
     match command {
         SubkeyCommand::Add(c) => subkey_add(sq, c)?,
+        SubkeyCommand::Delete(c) => subkey_delete(sq, c)?,
         SubkeyCommand::Expire(c) => subkey_expire(sq, c)?,
         SubkeyCommand::Revoke(c) => subkey_revoke(sq, c)?,
     }
@@ -39,6 +42,23 @@ pub fn dispatch(sq: Sq, command: SubkeyCommand) -> Result<()> {
     Ok(())
 }
 
+fn subkey_delete(sq: Sq, command: SubkeyDeleteCommand)
+    -> Result<()>
+{
+    let handle = if let Some(file) = command.cert_file {
+        assert!(command.cert.is_none());
+        file.into()
+    } else if let Some(kh) = command.cert {
+        kh.into()
+    } else {
+        panic!("clap enforces --cert or --cert-file is set");
+    };
+
+    assert!(! command.key.is_empty());
+
+    delete(sq, handle, command.key, command.output, command.binary)
+}
+
 fn subkey_expire(sq: Sq, command: SubkeyExpireCommand)
     -> Result<()>
 {

src/common.rs

@@ -16,6 +16,8 @@ mod expire;
 pub use expire::expire;
 
 pub mod delete;
+pub use delete::delete;
+
 pub mod password;
 pub mod userid;
 

src/common/delete.rs

@@ -3,8 +3,11 @@ use anyhow::Context;
 
 use sequoia_openpgp as openpgp;
 use openpgp::Cert;
-use openpgp::parse::Parse;
+use openpgp::KeyHandle;
+use openpgp::Packet;
 use openpgp::Result;
+use openpgp::cert::amalgamation::key::PrimaryKey;
+use openpgp::parse::Parse;
 use openpgp::serialize::Serialize;
 
 use crate::cli::types::FileOrStdout;
@@ -12,49 +15,160 @@ use crate::cli::types::FileStdinOrKeyHandle;
 use crate::Sq;
 
 pub fn delete(sq: Sq,
-              cert: FileStdinOrKeyHandle,
+              cert_handle: FileStdinOrKeyHandle,
+              keys: Vec<KeyHandle>,
               output: Option<FileOrStdout>,
               binary: bool)
     -> Result<()>
 {
-    match cert {
-        FileStdinOrKeyHandle::FileOrStdin(file) => {
+    let (cert, mut ks) = match cert_handle {
+        FileStdinOrKeyHandle::FileOrStdin(ref file) => {
             let input = file.open()?;
             let cert = Cert::from_buffered_reader(input)?;
 
-            let output = output.unwrap_or_else(|| FileOrStdout::new(None));
-            let mut output = output.create_safe(sq.force)?;
-            if binary {
-                cert.serialize(&mut output)?;
-            } else {
-                cert.armored().serialize(&mut output)?;
+            // If it is not a TSK, there is nothing to strip.
+            if ! cert.is_tsk() {
+                return Err(anyhow::anyhow!(
+                    "{} does not contain any secret key material.",
+                    cert.fingerprint()));
             }
+
+            (cert, None)
         }
-        FileStdinOrKeyHandle::KeyHandle(kh) => {
+        FileStdinOrKeyHandle::KeyHandle(ref kh) => {
             let cert = sq.lookup_one(kh, None, true)?;
-            let vc = cert.with_policy(sq.policy, None)?;
 
             let ks = sq.key_store_or_else()?;
-            let mut ks = ks.lock().unwrap();
+            let ks = ks.lock().unwrap();
 
-            // Delete the primary last.
-            let keys: Vec<_> = vc.keys().collect();
-            for ka in keys.into_iter().rev() {
+            (cert, Some(ks))
+        }
+    };
+
+    let to_delete: Vec<(_, _)> = if keys.is_empty() {
+        // Delete all secret key material.
+        let to_delete: Vec<_>
+            = cert.keys().filter_map(|ka| {
+                if let Some(ks) = ks.as_mut() {
+                    let remote_keys = ks.find_key(ka.key_handle()).ok()?;
+                    if remote_keys.is_empty() {
+                        None
+                    } else {
+                        Some((ka, Some(remote_keys)))
+                    }
+                } else {
+                    if ka.has_secret() {
+                        Some((ka, None))
+                    } else {
+                        None
+                    }
+                }
+            }).collect();
+
+        // Delete the primary last so that if something goes wrong it
+        // is still possible to generate a revocation certificate.
+        to_delete.into_iter().rev().collect()
+    } else {
+        // Delete only the specified secret key material.
+        let mut to_delete = Vec::new();
+
+        let mut missing_key_count = 0;
+        let mut no_secret_key_material_count = 0;
+        for key in keys.into_iter() {
+            let ka = if let Some(ka)
+                = cert.keys().find(|ka| ka.fingerprint().aliases(&key))
+            {
+                ka
+            } else {
+                eprintln!("{} does not contain {}",
+                          cert.fingerprint(), key);
+                missing_key_count += 1;
+                continue;
+            };
+
+            let (no_secret_key_material, remote_keys)
+                = if let Some(ks) = ks.as_mut()
+            {
                 let remote_keys = ks.find_key(ka.key_handle())?;
-                if remote_keys.is_empty() {
-                    eprintln!("Skipping {}: the key store does not manage \
-                               its secret key material",
-                              ka.fingerprint());
-                }
-                for mut kh in remote_keys.into_iter() {
-                    kh.delete_secret_key_material().with_context(|| {
-                        format!("Deleting {}", ka.fingerprint())
-                    })?;
-                }
+                (remote_keys.is_empty(), Some(remote_keys))
+            } else {
+                (! ka.has_secret(), None)
+            };
+
+            if no_secret_key_material {
+                eprintln!("{} does not contain any secret key material",
+                          key);
+                no_secret_key_material_count += 1;
+                continue;
             }
+
+            to_delete.push((ka, remote_keys));
+        }
+
+        if missing_key_count > 1 {
+            // Plural.
+            return Err(anyhow::anyhow!(
+                "{} keys not found", missing_key_count));
+        } else if missing_key_count > 0 {
+            // Singular.
+            return Err(anyhow::anyhow!(
+                "{} key not found", missing_key_count));
+        }
+
+        if no_secret_key_material_count > 1 {
+            // Plural.
+            return Err(anyhow::anyhow!(
+                "{} of the keys to delete don't have secret key material",
+                no_secret_key_material_count));
+        } else if no_secret_key_material_count > 0 {
+            // Singular.
+            return Err(anyhow::anyhow!(
+                "{} of the keys to delete doesn't have secret key material",
+                no_secret_key_material_count));
+        }
+
+        to_delete
+    };
+
+    assert!(! to_delete.is_empty());
+
+    if ks.is_some() {
+        // Delete the secret key material from the key store.
+        for (ka, remote_keys) in to_delete.into_iter() {
+            let remote_keys = remote_keys.expect("have remote keys");
+            assert!(! remote_keys.is_empty());
+            for mut kh in remote_keys.into_iter() {
+                kh.delete_secret_key_material().with_context(|| {
+                    format!("Deleting {}", ka.fingerprint())
+                })?;
+            }
+        }
+    } else {
+        // Strip the secret key material from the certificate.
+        let mut stripped: Vec<Packet> = Vec::new();
+
+        for (ka, _) in to_delete.into_iter() {
+            let pk = ka.key().clone().take_secret().0;
+            if ka.primary() {
+                stripped.push(
+                    Packet::PublicKey(pk.role_into_primary()));
+            } else {
+                stripped.push(
+                    Packet::PublicSubkey(pk.role_into_subordinate()));
+            }
+        }
+
+        let cert = cert.insert_packets(
+            stripped.into_iter().map(|stripped| Packet::from(stripped)))?;
+
+        let output = output.unwrap_or_else(|| FileOrStdout::new(None));
+        let mut output = output.for_secrets().create_safe(sq.force)?;
+        if binary {
+            cert.as_tsk().serialize(&mut output)?;
+        } else {
+            cert.as_tsk().armored().serialize(&mut output)?;
         }
     }
 
-
     Ok(())
 }

tests/common.rs

@@ -647,6 +647,62 @@ 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
+    where H: Into<FileOrKeyHandle>,
+          Q: Into<Option<&'a Path>>,
+    {
+        let cert_handle = cert_handle.into();
+        let output_file = output_file.into();
+
+        let mut cmd = self.command();
+        cmd.arg("key").arg("subkey").arg("delete");
+
+        match &cert_handle {
+            FileOrKeyHandle::FileOrStdin(path) => {
+                cmd.arg("--cert-file").arg(path);
+            }
+            FileOrKeyHandle::KeyHandle((_kh, s)) => {
+                cmd.arg("--cert").arg(&s);
+            }
+        };
+
+        for kh in key_handles {
+            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());
+
+        if let Some(output_file) = output_file {
+            if output_file != &PathBuf::from("-") {
+                return Cert::from_file(output_file)
+                    .expect("can parse certificate");
+            }
+        } else if output_file.is_none() {
+            if let FileOrKeyHandle::KeyHandle((kh, _s)) = cert_handle {
+                // This will fail if the key no longer has any secret
+                // key material.  If it fails, fall back to `sq cert
+                // export`.
+                if let Ok(cert) = self.key_export_maybe(kh.clone()) {
+                    return cert;
+                } else {
+                    return self.cert_export(kh.clone());
+                }
+            }
+        }
+        Cert::from_bytes(&output.stdout)
+            .expect("can parse certificate")
+    }
+
     /// Imports the specified certificate into the keystore.
     pub fn cert_import<P>(&self, path: P)
     where P: AsRef<Path>

tests/sq-key-subkey-delete.rs

@@ -0,0 +1,116 @@
+use sequoia_openpgp as openpgp;
+use openpgp::KeyHandle;
+use openpgp::KeyID;
+use openpgp::Result;
+use openpgp::packet::Key;
+
+mod common;
+use common::power_set;
+use common::Sq;
+
+mod integration {
+    use super::*;
+
+    #[test]
+    fn sq_key_subkey_delete() -> Result<()>
+    {
+        let sq = Sq::new();
+
+        // Generate a key in a file.
+        let (cert, cert_file, _rev_file) = sq.key_generate(&[], &["alice"]);
+        assert!(cert.is_tsk());
+
+        // Delete each non-empty subset of keys.
+
+        eprintln!("Certificate:");
+        for k in cert.keys() {
+            eprintln!("  {}", k.fingerprint());
+        }
+
+        let keys: Vec<Key<_, _>> = cert.keys()
+            .map(|k| {
+                k.key().clone()
+            })
+            .collect();
+
+        let key_ids = keys.iter().map(|k| k.fingerprint()).collect::<Vec<_>>();
+
+        for (((i, to_delete), keystore), by_fpr) in power_set(&key_ids).into_iter()
+            .enumerate()
+            .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()
+                      });
+            eprintln!("  Deleting:");
+            for k in to_delete.iter() {
+                eprintln!("    {}", k);
+            }
+
+            let to_delete_kh: Vec<KeyHandle> = if by_fpr {
+                to_delete.iter()
+                    .map(|fpr| KeyHandle::from(fpr))
+                    .collect()
+            } else {
+                to_delete.iter()
+                    .map(|fpr| KeyHandle::from(KeyID::from(fpr)))
+                    .collect()
+            };
+
+            // Delete the selection.
+            let got = if keystore {
+                // Import it into the key store.
+                sq.key_import(&cert_file);
+
+                sq.key_subkey_delete(
+                    cert.key_handle(), &to_delete_kh, None)
+            } else {
+                sq.key_subkey_delete(
+                    &cert_file, &to_delete_kh, None)
+            };
+
+            // 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(())
+    }
+}