From 37e2b65c6f84f359296e599b9b83233da0a6f830 Mon Sep 17 00:00:00 2001 From: Justus Winter Date: Wed, 23 Oct 2024 17:02:42 +0200 Subject: [PATCH] Use cert designators for `sq key revoke`. - See #207. --- NEWS | 3 + src/cli/key/revoke.rs | 96 ++++++++++++++------------------ src/cli/types/cert_designator.rs | 17 ++++++ src/commands/key/revoke.rs | 34 +++-------- src/sq.rs | 38 ++++++++++++- tests/integration/common.rs | 3 +- 6 files changed, 110 insertions(+), 81 deletions(-) diff --git a/NEWS b/NEWS index a66d9d5c..5bcb60ec 100644 --- a/NEWS +++ b/NEWS @@ -121,6 +121,9 @@ - The argument `sq key expire --cert-file` has been renamed to `--file`. - The argument `sq key expire --file` now requires `--output`. + - The argument `sq key revoke --cert-file` has been renamed to + `--file`. + - The argument `sq key revoke --file` now requires `--output`. * Changes in 0.38.0 ** Notable changes diff --git a/src/cli/key/revoke.rs b/src/cli/key/revoke.rs index 634944e9..c2be7f80 100644 --- a/src/cli/key/revoke.rs +++ b/src/cli/key/revoke.rs @@ -1,17 +1,13 @@ //! Command-line parser for `sq key revoke`. use clap::Args; -use clap::ArgGroup; - -use sequoia_openpgp as openpgp; -use openpgp::KeyHandle; use crate::cli::types::ClapData; -use crate::cli::types::FileOrStdin; use crate::cli::types::FileOrStdout; use crate::cli::examples::*; use crate::cli::key::KeyReasonForRevocation; +use crate::cli::types::cert_designator::*; const REVOKE_EXAMPLES: Actions = Actions { actions: &[ @@ -67,56 +63,18 @@ instead of the current time. ", after_help = REVOKE_EXAMPLES, )] -#[clap(group(ArgGroup::new("cert_input").args(&["cert_file", "cert"]).required(true)))] -#[clap(group(ArgGroup::new("revoker_input").args(&["revoker_file", "revoker"])))] pub struct Command { - #[clap( - long, - value_name = "FINGERPRINT|KEYID", - help = "The certificate to revoke", - )] - pub cert: Option, - #[clap( - long, - value_name = "CERT_FILE", - conflicts_with = "cert", - help = "The certificate to revoke", - long_help = "\ -The certificate to revoke. + #[command(flatten)] + pub cert: CertDesignators, -Read the certificate to revoke from FILE or stdin, if `-`. It is \ -an error for the file to contain more than one certificate.", - )] - pub cert_file: Option, - - #[clap( - long, - value_name = "FINGERPRINT|KEYID", - help = "The certificate that issues the revocation", - long_help = "\ -The certificate that issues the revocation. - -Sign the revocation certificate using the specified key. By default, \ -the certificate being revoked is used. Using this option, it is \ -possible to create a third-party revocation.", - )] - pub revoker: Option, - #[clap( - long, - value_name = "KEY_FILE", - conflicts_with = "revoker", - help = "The certificate that issues the revocation", - long_help = "\ -The certificate that issues the revocation. - -Sign the revocation certificate using the specified key. By default, \ -the certificate being revoked is used. Using this option, it is \ -possible to create a third-party revocation. - -Read the certificate from KEY_FILE or stdin, if `-`. It is an error \ -for the file to contain more than one certificate.", - )] - pub revoker_file: Option, + #[command(flatten)] + pub revoker: CertDesignators, #[clap( value_name = "REASON", @@ -185,3 +143,35 @@ modified certificate to stdout.", )] pub binary: bool, } + +/// Documentation for the cert designators for the key revoke. +pub struct KeyRevokeCertDoc {} + +impl AdditionalDocs for KeyRevokeCertDoc { + fn help(arg: &'static str, help: &'static str) -> clap::builder::StyledStr { + match arg { + "file" => + "Revoke the key read from PATH" + .into(), + _ => { + debug_assert!(help.starts_with("Use certificates")); + help.replace("Use certificates", + "Revoke the key") + }, + }.into() + } +} + +/// Documentation for the revoker designators for the key revoke. +pub struct KeyRevokeRevokerDoc {} + +impl AdditionalDocs for KeyRevokeRevokerDoc { + fn help(_: &'static str, help: &'static str) -> clap::builder::StyledStr { + format!("{} to create the revocation certificate. + +Sign the revocation certificate using the specified key. By default, \ +the certificate being revoked is used. Using this option, it is \ +possible to create a third-party revocation.", + help.replace("certificates", "key")).into() + } +} diff --git a/src/cli/types/cert_designator.rs b/src/cli/types/cert_designator.rs index 0c233737..26709578 100644 --- a/src/cli/types/cert_designator.rs +++ b/src/cli/types/cert_designator.rs @@ -75,6 +75,19 @@ impl ArgumentPrefix for SignerPrefix { } } +/// "--revoker", "--revoker-userid", "--revoker-file", etc. +pub type RevokerPrefix = ConcreteArgumentPrefix; + +impl ArgumentPrefix for RevokerPrefix { + fn prefix() -> &'static str { + "revoker-" + } + + fn name() -> &'static str { + "revoker" + } +} + /// Adds a `--file` argument. pub type FileArg = typenum::U1; @@ -148,6 +161,10 @@ pub type OneValue = typenum::U1; /// completely optional. pub type OptionalValue = typenum::U2; +/// Combines OneValue and OptionalValue. +pub type OneOptionalValue + = >::Output; + /// Normally it is possible to designate multiple certificates. This /// errors out if there is more than one value. pub type FileRequiresOutput = typenum::U4; diff --git a/src/commands/key/revoke.rs b/src/commands/key/revoke.rs index 3757cf99..d2f8217e 100644 --- a/src/commands/key/revoke.rs +++ b/src/commands/key/revoke.rs @@ -1,7 +1,6 @@ use sequoia_openpgp as openpgp; use openpgp::cert::CertRevocationBuilder; use openpgp::packet::signature::subpacket::NotationData; -use openpgp::parse::Parse; use openpgp::types::ReasonForRevocation; use openpgp::Cert; use openpgp::Packet; @@ -9,7 +8,6 @@ use openpgp::Result; use crate::Sq; use crate::cli::key::revoke::Command; -use crate::cli::types::FileOrStdout; use crate::common::get_secret_signer; use crate::common::NULL_POLICY; use crate::common::RevocationOutput; @@ -87,32 +85,18 @@ impl RevocationOutput for CertificateRevocation /// Revoke a certificate pub fn certificate_revoke( sq: Sq, - mut command: Command, + command: Command, ) -> Result<()> { - let cert = if let Some(file) = command.cert_file { - if command.output.is_none() { - // None means to write to the cert store. When reading - // from a file, we want to write to stdout by default. - command.output = Some(FileOrStdout::new(None)); - } + let cert = + sq.resolve_cert_with_policy(&command.cert, + sequoia_wot::FULLY_TRUSTED, + NULL_POLICY, + sq.time)?.0; - let br = file.open()?; - Cert::from_buffered_reader(br)? - } else if let Some(kh) = command.cert { - sq.lookup_one_with_policy(&kh, None, true, - NULL_POLICY, sq.time)? - } else { - panic!("clap enforces --cert or --cert-file"); - }; - - let revoker = if let Some(file) = command.revoker_file { - let br = file.open()?; - Some(Cert::from_buffered_reader(br)?) - } else if let Some(kh) = command.revoker { - Some(sq.lookup_one_with_policy(&kh, None, true, - NULL_POLICY, sq.time)?) - } else { + let revoker = if command.revoker.is_empty() { None + } else { + Some(sq.resolve_cert(&command.revoker, sequoia_wot::FULLY_TRUSTED)?.0) }; let notations = parse_notations(command.notation)?; diff --git a/src/sq.rs b/src/sq.rs index 11a3aded..c52b1fd6 100644 --- a/src/sq.rs +++ b/src/sq.rs @@ -1701,6 +1701,22 @@ impl<'store: 'rstore, 'rstore> Sq<'store, 'rstore> { -> Result<(Vec, Vec)> where Prefix: ArgumentPrefix + { + self.resolve_certs_with_policy(designators, trust_amount, + self.policy, self.time) + } + + /// Like [`Sq::resolve_certs`] but with explicit policy. + pub fn resolve_certs_with_policy( + &self, + designators: &CertDesignators, + trust_amount: usize, + policy: &dyn Policy, + time: SystemTime, + ) + -> Result<(Vec, Vec)> + where + Prefix: ArgumentPrefix, { tracer!(TRACE, "Sq::resolve_certs"); t!("{:?}", designators); @@ -1850,7 +1866,7 @@ impl<'store: 'rstore, 'rstore> Sq<'store, 'rstore> { for ka in cert.keys().subkeys() { if ka.key_handle().aliases(kh) { - match ka.with_policy(self.policy, None) { + match ka.with_policy(policy, time) { Ok(_ka) => { ret(designator, Ok(Arc::new(cert.into())), @@ -2134,6 +2150,22 @@ impl<'store: 'rstore, 'rstore> Sq<'store, 'rstore> { -> Result<(Cert, FileStdinOrKeyHandle)> where Prefix: ArgumentPrefix + { + self.resolve_cert_with_policy(designators, trust_amount, + self.policy, self.time) + } + + /// Like [`Sq::resolve_cert`], but with explicit policy. + pub fn resolve_cert_with_policy( + &self, + designators: &CertDesignators, + trust_amount: usize, + policy: &dyn Policy, + time: SystemTime, + ) + -> Result<(Cert, FileStdinOrKeyHandle)> + where + Prefix: ArgumentPrefix, { // Assuming this is only called with OneValue, then the // following are not required. @@ -2147,7 +2179,9 @@ impl<'store: 'rstore, 'rstore> Sq<'store, 'rstore> { Prefix::name()); } - let (certs, errors) = self.resolve_certs(designators, trust_amount)?; + let (certs, errors) = + self.resolve_certs_with_policy(designators, trust_amount, + policy, time)?; if certs.len() > 1 { wprintln!("{} is ambiguous. It resolves to multiple certificates.", designators.designators[0].argument::()); diff --git a/tests/integration/common.rs b/tests/integration/common.rs index d7cb1c07..ff0fbd00 100644 --- a/tests/integration/common.rs +++ b/tests/integration/common.rs @@ -594,7 +594,8 @@ impl Sq { match &cert_handle { FileOrKeyHandle::FileOrStdin(path) => { - cmd.arg("--cert-file").arg(path); + cmd.arg("--file").arg(path); + assert!(output_file.is_some()); } FileOrKeyHandle::KeyHandle((_kh, s)) => { cmd.arg("--cert").arg(&s);