Change sq pki certify to use a named argument for the certificate.

- `sq pki certify` uses a positional argument to specify the certificate to certify. Change it to be a named argument, either `--cert`, or `--cert-file`. - See #318.
2024-10-11 16:27:52 +02:00 · 2024-10-11 16:27:52 +02:00 · 3d63b8de96
commit 3d63b8de96
parent e40181bb57
10 changed files with 132 additions and 32 deletions
--- a/3
+++ b/3
@ -62,6 +62,9 @@
     changes the meaning of how `--userid` is interpreted, but takes
     an email address.  The `--userid` and `--email` arguments may be
     given multiple times to certify multiple user IDs at once.
+   - `sq pki certify`'s positional argument for specifying the
+     certificate to certify must now be specified using a named
+     argument, `--cert` or `--cert-file`.

 * Changes in 0.38.0
 ** Notable changes
--- a/sq-subplot.md
+++ b/sq-subplot.md
@ -1044,7 +1044,7 @@ when I run sq --no-cert-store --no-key-store toolbox extract-cert bob.pgp --outp
 when I run sq --no-cert-store --no-key-store inspect bob-cert.pgp
 then stdout doesn't contain "Certifications:"

-when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp bob-cert.pgp Bob --output cert.pgp
+when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --cert-file bob-cert.pgp --userid Bob --output cert.pgp
 then file cert.pgp contains "-----BEGIN PGP PUBLIC KEY BLOCK-----"
 then file cert.pgp contains "-----END PGP PUBLIC KEY BLOCK-----"
 when I run sq --no-cert-store --no-key-store inspect cert.pgp
@ -1065,7 +1065,7 @@ when I run sq --no-cert-store --no-key-store toolbox extract-cert bob.pgp --outp
 when I run sq --no-cert-store --no-key-store inspect bob-cert.pgp
 then stdout doesn't contain "Certifications:"

-when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp bob-cert.pgp Bob --output cert.pgp --binary
+when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --cert-file bob-cert.pgp --userid Bob --output cert.pgp --binary
 when I run cat cert.pgp
 then stdout doesn't contain "-----BEGIN PGP PUBLIC KEY BLOCK-----"
 when I run sq --no-cert-store --no-key-store inspect cert.pgp
@ -1084,7 +1084,7 @@ when I run sq --no-cert-store --no-key-store toolbox extract-cert alice.pgp --ou
 when I run sq --no-cert-store --no-key-store key generate --without-password --userid "<bob@example.org>" --output bob.pgp --rev-cert bob.pgp.rev
 when I run sq --no-cert-store --no-key-store toolbox extract-cert bob.pgp --output bob-cert.pgp

-when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp bob-cert.pgp --email bob@example.org --output cert.pgp
+when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --cert-file bob-cert.pgp --email bob@example.org --output cert.pgp
 when I run sq --no-cert-store --no-key-store inspect cert.pgp
 then stdout contains "Certifications: 1,"
 ~~~
@ -1101,7 +1101,7 @@ when I run sq --no-cert-store --no-key-store toolbox extract-cert alice.pgp --ou
 when I run sq --no-cert-store --no-key-store key generate --without-password --userid "<bob@example.org>" --userid "Bob <bob@example.org>" --output bob.pgp --rev-cert bob.pgp.rev
 when I run sq --no-cert-store --no-key-store toolbox extract-cert bob.pgp --output bob-cert.pgp

-when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp bob-cert.pgp --email bob@example.org --output cert.pgp
+when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --cert-file bob-cert.pgp --email bob@example.org --output cert.pgp

 when I run sq --no-cert-store --no-key-store toolbox strip-userid --cert-file cert.pgp --userid "<bob@example.org>" --output cert.0.pgp
 when I run sq --no-cert-store --no-key-store inspect cert.0.pgp
@ -1128,7 +1128,7 @@ when I run sq --no-cert-store --no-key-store toolbox extract-cert bob.pgp --outp
 when I run sq --no-cert-store --no-key-store inspect bob-cert.pgp
 then stdout doesn't contain "Certifications:"

-when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --add-userid bob-cert.pgp "My friend Bob" --output cert.pgp
+when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --cert-file bob-cert.pgp --add-userid --userid "My friend Bob" --output cert.pgp
 when I run sq --no-cert-store --no-key-store inspect cert.pgp
 then stdout contains "My friend Bob"
 then stdout contains "Certifications: 1,"
@ -1147,7 +1147,7 @@ when I run sq --no-cert-store --no-key-store toolbox extract-cert alice.pgp --ou
 when I run sq --no-cert-store --no-key-store key generate --without-password --userid Bob --output bob.pgp --rev-cert bob.pgp.rev
 when I run sq --no-cert-store --no-key-store toolbox extract-cert bob.pgp --output bob-cert.pgp

-when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --add-userid bob-cert.pgp --email "bob@example.org" --output cert.pgp
+when I run sq --no-cert-store --no-key-store pki certify --certifier-file alice.pgp --cert-file bob-cert.pgp --add-userid --email "bob@example.org" --output cert.pgp
 when I run sq --no-cert-store --no-key-store inspect cert.pgp
 then stdout contains "<bob@example.org>"
 then stdout contains "Certifications: 1,"
--- a/src/cli/key/approvals.rs
+++ b/src/cli/key/approvals.rs
@ -141,7 +141,7 @@ const UPDATE_EXAMPLES: Actions = Actions {
            command: &[
                "sq", "pki", "certify",
                "--certifier", "511257EBBF077B7AEDAE5D093F68CB84CE537C9A",
-                "EB28F26E2739A4870ECC47726F0073F60FD0CBF0",
+                "--cert", "EB28F26E2739A4870ECC47726F0073F60FD0CBF0",
                "--email", "alice@example.org",
            ],
        }),
--- a/src/cli/pki/certify.rs
+++ b/src/cli/pki/certify.rs
@ -15,7 +15,10 @@ use crate::cli::types::Expiration;
 use crate::cli::types::FileOrStdin;
 use crate::cli::types::FileOrStdout;
 use crate::cli::types::TrustAmount;
+use crate::cli::types::cert_designator::CertFileArgs;
+use crate::cli::types::cert_designator::CertPrefix;
 use crate::cli::types::cert_designator::NoPrefix;
+use crate::cli::types::cert_designator::OneValue;
 use crate::cli::types::cert_designator::UserIDEmailArgs;

 use crate::cli::examples::*;
@ -34,7 +37,7 @@ Alice certifies that Bob controls 3F68CB84CE537C9A and bob@example.org.",
            command: &[
                "sq", "pki", "certify",
                "--certifier", "EB28F26E2739A4870ECC47726F0073F60FD0CBF0",
-                "511257EBBF077B7AEDAE5D093F68CB84CE537C9A",
+                "--cert", "511257EBBF077B7AEDAE5D093F68CB84CE537C9A",
                "--email", "bob@example.org",
            ],
        }),
@ -46,7 +49,7 @@ which is not a self-signed user ID.",
            command: &[
                "sq", "pki", "certify",
                "--certifier", "EB28F26E2739A4870ECC47726F0073F60FD0CBF0",
-                "511257EBBF077B7AEDAE5D093F68CB84CE537C9A",
+                "--cert", "511257EBBF077B7AEDAE5D093F68CB84CE537C9A",
                "--add-userid",
                "--email", "bob@bobs.lair.net",
            ],
@ -206,13 +209,9 @@ pub struct Command {
        help = "Create the certification using KEY-FILE.",
    )]
    pub certifier_file: Option<FileOrStdin>,
-    #[clap(
-        value_name = "KEY_ID|FINGERPRINT|FILE",
-        required = true,
-        index = 1,
-        help = "Certify CERTIFICATE.",
-    )]
-    pub certificate: String,
+
+    #[command(flatten)]
+    pub cert: CertDesignators<CertFileArgs, CertPrefix, OneValue>,

    #[command(flatten)]
    pub userids: CertDesignators<UserIDEmailArgs, NoPrefix>,
--- a/src/cli/types/cert_designator.rs
+++ b/src/cli/types/cert_designator.rs
@ -15,6 +15,9 @@ use openpgp::packet::UserID;
 /// See [`NoPrefix`], [`CertPrefix`], etc.
 pub trait ArgumentPrefix {
    fn prefix() -> &'static str;
+
+    /// The argument group's name, e.g., "cert", "for".
+    fn name() -> &'static str;
 }

 pub struct ConcreteArgumentPrefix<T>(std::marker::PhantomData<T>)
@ -32,18 +35,30 @@ impl ArgumentPrefix for NoPrefix {
    fn prefix() -> &'static str {
        ""
    }
+
+    fn name() -> &'static str {
+        "cert"
+    }
 }

 impl ArgumentPrefix for CertPrefix {
    fn prefix() -> &'static str {
        "cert-"
    }
+
+    fn name() -> &'static str {
+        "cert"
+    }
 }

 impl ArgumentPrefix for RecipientPrefix {
    fn prefix() -> &'static str {
        "for-"
    }
+
+    fn name() -> &'static str {
+        "for"
+    }
 }

 /// Adds a `--file` argument.
@ -84,6 +99,11 @@ pub type CertUserIDEmailFileArgs
 pub type UserIDEmailArgs
    = <UserIDArg as std::ops::BitOr<EmailArg>>::Output;

+/// Enables --cert, and --file (i.e., not --userid, --email, --domain,
+/// or --grep).
+pub type CertFileArgs = <CertArg as std::ops::BitOr<FileArg>>::Output;
+
+
 /// Argument parser options.

 /// Normally it is possible to designate multiple certificates.  This
@ -200,6 +220,16 @@ impl CertDesignator {
            Grep(pattern) => format!("{} {:?}", argument_name, pattern),
        }
    }
+
+    /// Whether the argument reads from a file.
+    pub fn from_file(&self) -> bool
+    {
+        if let CertDesignator::File(_) = self {
+            true
+        } else {
+            false
+        }
+    }
 }

 /// A data structure that can be flattened into a clap `Command`, and
@ -270,7 +300,7 @@ where
        let optional_value = (options & OptionalValue::to_usize()) > 0;

        let group = format!("cert-designator-{}-{:X}-{:X}",
-                            Prefix::prefix(),
+                            Prefix::name(),
                            arguments,
                            options);
        let mut arg_group = clap::ArgGroup::new(group);
--- a/src/commands/pki/certify.rs
+++ b/src/commands/pki/certify.rs
@ -1,12 +1,10 @@
 use sequoia_openpgp as openpgp;
-use openpgp::KeyHandle;
 use openpgp::packet::UserID;
 use openpgp::Result;
 use openpgp::types::KeyFlags;

 use crate::Sq;
 use crate::cli::pki::certify;
-use crate::cli::types::FileOrStdin;
 use crate::cli::types::FileStdinOrKeyHandle;
 use crate::cli::types::cert_designator::CertDesignator;
 use crate::commands::FileOrStdout;
@ -27,15 +25,8 @@ pub fn certify(sq: Sq, mut c: certify::Command)
    let certifier = sq.lookup_one(
        certifier, Some(KeyFlags::empty().set_certification()), true)?;

-    // XXX: Change this interface: it's dangerous to guess whether an
-    // identifier is a file or a key handle.
-    let cert = if let Ok(kh) = c.certificate.parse::<KeyHandle>() {
-        FileStdinOrKeyHandle::KeyHandle(kh)
-    } else {
-        FileStdinOrKeyHandle::FileOrStdin(
-            FileOrStdin::new(Some(c.certificate.into())))
-    };
-    if cert.is_file() {
+    let (cert, from_file) = sq.resolve_cert(&c.cert, sequoia_wot::FULLY_TRUSTED)?;
+    if from_file {
        // If the cert is read from a file, we default to stdout.
        // (None means write to the cert store.)
        if c.output.is_none() {
@ -43,7 +34,6 @@ pub fn certify(sq: Sq, mut c: certify::Command)
        }
    }

-    let cert = sq.lookup_one(cert, None, true)?;
    let vc = cert.with_policy(sq.policy, Some(sq.time))?;

    // Find the matching User ID.
--- a/src/sq.rs
+++ b/src/sq.rs
@ -47,6 +47,7 @@ use keystore::Protection;
 use crate::cli::types::CertDesignators;
 use crate::cli::types::cert_designator::ArgumentPrefix;
 use crate::cli::types::cert_designator::CertDesignator;
+use crate::cli::types::cert_designator::OneValue;
 use crate::cli::types::FileStdinOrKeyHandle;
 use crate::common::password;
 use crate::output::hint::Hint;
@ -2102,4 +2103,53 @@ impl<'store: 'rstore, 'rstore> Sq<'store, 'rstore> {

        Ok((results, errors))
    }
+
+    /// Like `Sq::resolve_certs`, but bails if there is not exactly
+    /// one designator, or the designator resolves to multiple
+    /// certificates.
+    ///
+    /// Returns whether the certificate was read from a file.
+    pub fn resolve_cert<Arguments, Prefix>(
+        &self,
+        designators: &CertDesignators<Arguments, Prefix, OneValue>,
+        trust_amount: usize,
+    )
+        -> Result<(Cert, bool)>
+    where
+        Prefix: ArgumentPrefix
+    {
+        // Assuming this is only called with OneValue, then the
+        // following are not required.
+        if designators.designators.len() == 0 {
+            panic!("clap failed to enforce that the {} argument is \
+                    required.",
+                   Prefix::name());
+        } else if designators.designators.len() > 1 {
+            panic!("clap failed to enforce that the {} argument is \
+                    specified at most once.",
+                   Prefix::name());
+        }
+
+        let (certs, errors) = self.resolve_certs(designators, trust_amount)?;
+        if certs.len() > 1 {
+            wprintln!("{} is ambiguous.  It resolves to multiple certificates.",
+                      designators.designators[0].argument::<Prefix>());
+            for cert in certs.iter() {
+                eprintln!("  - {} {}",
+                          cert.fingerprint(),
+                          self.best_userid(cert, true));
+            }
+
+            return Err(anyhow::anyhow!(
+                "{} is ambiguous.  It resolves to multiple certificates.",
+                designators.designators[0].argument::<Prefix>()))
+        }
+
+        if let Some(errors) = errors.into_iter().next() {
+            return Err(errors);
+        }
+
+        Ok((certs.into_iter().next().unwrap(),
+            designators.designators[0].from_file()))
+    }
 }
--- a/tests/integration/common.rs
+++ b/tests/integration/common.rs
@ -1316,7 +1316,15 @@ impl Sq {
                cmd.arg("--certifier").arg(s);
            }
        }
-        cmd.arg(&cert).arg("--userid").arg(userid);
+        match &cert {
+            FileOrKeyHandle::FileOrStdin(file) => {
+                cmd.arg("--cert-file").arg(file);
+            }
+            FileOrKeyHandle::KeyHandle((_kh, s)) => {
+                cmd.arg("--cert").arg(s);
+            }
+        }
+        cmd.arg("--userid").arg(userid);

        if let Some(output_file) = output_file {
            cmd.arg("--overwrite").arg("--output").arg(output_file);
--- a/tests/integration/sq_pki_link.rs
+++ b/tests/integration/sq_pki_link.rs
@ -5,10 +5,14 @@ use std::sync::{Mutex, OnceLock};
 use tempfile::TempDir;

 use sequoia_openpgp as openpgp;
+use openpgp::KeyHandle;
 use openpgp::Result;
 use openpgp::Cert;

-use super::common::{Sq, artifact};
+use super::common::artifact;
+use super::common::Sq;
+use super::common::FileOrKeyHandle;
+

 // We are going to replace certifications, and we want to make sure
 // that the newest one is the active one.  This means ensuring that
@ -150,6 +154,13 @@ fn sq_certify(sq: &Sq,

    let certification = sq.scratch_file(Some(&format!(
        "certification {} {} by {}", cert, userid, certifier)[..]));
+
+    let cert = if let Ok(kh) = cert.parse::<KeyHandle>() {
+        kh.into()
+    } else {
+        FileOrKeyHandle::FileOrStdin(cert.into())
+    };
+
    sq.pki_certify(&extra_args, certifier, cert, userid,
                   Some(certification.as_path()));
    sq.cert_import(&certification);
--- a/tests/integration/sq_sign.rs
+++ b/tests/integration/sq_sign.rs
@ -19,7 +19,9 @@ use openpgp::policy::StandardPolicy;
 use openpgp::serialize::stream::{Message, Signer, Compressor, LiteralWriter};
 use openpgp::serialize::Serialize;

-use super::common::{Sq, artifact};
+use super::common::artifact;
+use super::common::Sq;
+use super::common::FileOrKeyHandle;

 const P: &StandardPolicy = &StandardPolicy::new();

@ -1059,6 +1061,13 @@ fn sq_verify_wot() -> Result<()> {

        let certification = sq.scratch_file(Some(&format!(
            "certification {} {} by {}", cert, userid, certifier)[..]));
+
+        let cert = if let Ok(kh) = cert.parse::<KeyHandle>() {
+            kh.into()
+        } else {
+            FileOrKeyHandle::FileOrStdin(cert.into())
+        };
+
        sq.pki_certify(&extra_args, certifier, cert, userid,
                       Some(certification.as_path()));
        sq.cert_import(&certification);