From 012e762d38a002b8ba72d860164bfb4470ad2ae9 Mon Sep 17 00:00:00 2001 From: Justus Winter Date: Thu, 28 Nov 2024 17:38:52 +0100 Subject: [PATCH] Align user ID designators in `sq pki link retract`. - User IDs have to be explicitly given, or `--all` has to be used to select them all (this was previously the default). - This aligns the retract subcommand with the other link and vouch management commands. - Fixes #442. --- src/cli/pki/link.rs | 6 ++++-- src/cli/types/userid_designator.rs | 24 ++++++++++++++++++++++ src/commands/key/generate.rs | 2 ++ src/commands/pki/link.rs | 15 ++------------ src/common/types/userid_designator.rs | 24 +++++++++++++++++++--- tests/integration/common.rs | 15 +++++++++++++- tests/integration/sq_pki_link.rs | 11 ++++++---- tests/integration/sq_pki_link_authorize.rs | 2 +- 8 files changed, 75 insertions(+), 24 deletions(-) diff --git a/src/cli/pki/link.rs b/src/cli/pki/link.rs index 24438c92..3a4842e1 100644 --- a/src/cli/pki/link.rs +++ b/src/cli/pki/link.rs @@ -116,6 +116,7 @@ and any associated user IDs. This effectively invalidates all links.", command: &[ "sq", "pki", "link", "retract", "--cert=EB28F26E2739A4870ECC47726F0073F60FD0CBF0", + "--all", ], }), ], @@ -538,8 +539,8 @@ to force the signature to be re-created anyway.", #[command(flatten)] pub userids: UserIDDesignators< - userid_designator::AnyUserIDEmailArgs, - userid_designator::OptionalValueNoLinting>, + userid_designator::AllAnyUserIDEmailArgs, + userid_designator::AllMatchesNonSelfSignedNoLinting>, } const RETRACT_EXAMPLES: Actions = Actions { @@ -573,6 +574,7 @@ and any associated user IDs. This effectively invalidates all links.", command: &[ "sq", "pki", "link", "retract", "--cert=EB28F26E2739A4870ECC47726F0073F60FD0CBF0", + "--all", ], }), ], diff --git a/src/cli/types/userid_designator.rs b/src/cli/types/userid_designator.rs index b817c697..e5daec6f 100644 --- a/src/cli/types/userid_designator.rs +++ b/src/cli/types/userid_designator.rs @@ -85,6 +85,11 @@ pub type AllExistingAndAddXUserIDEmailArgs = >::Output; +/// Enables --all, --userid, and --email (but not --name, --userid-or-add, +/// --email-or-add, or --name-or-add). +pub type AllAnyUserIDEmailArgs + = >::Output; + /// Enables --userid, --email, --name, --userid-or-add, /// --email-or-add, and --name-or-add (but not --all). pub type ExistingAndAddXUserIDEmailNameArgs @@ -107,12 +112,18 @@ pub type OptionalValue = typenum::U2; /// the `--allow-non-canonical-userid` flag. pub type NoLinting = typenum::U4; +/// Makes --all match non-self signed user IDs. +pub type AllMatchesNonSelfSigned = typenum::U8; + pub type OneValueNoLinting = >::Output; pub type OptionalValueNoLinting = >::Output; +pub type AllMatchesNonSelfSignedNoLinting + = >::Output; + /// A user ID designator. #[derive(Debug, Clone)] pub enum UserIDDesignator { @@ -286,6 +297,9 @@ pub struct UserIDDesignators /// Use all self-signed user IDs. pub all: Option, + /// Whether --all should match non-self signed user IDs. + all_matches_non_self_signed: bool, + /// Whether --allow-non-canonical-userids was passed. pub allow_non_canonical_userids: bool, @@ -332,6 +346,11 @@ impl UserIDDesignators { self.all } + /// Returns whether --all should match non-self signed user IDs. + pub fn all_matches_non_self_signed(&self) -> bool { + self.all_matches_non_self_signed + } + /// Returns whether the allow-non-canonical-userids flag was set. pub fn allow_non_canonical_userids(&self) -> bool { self.allow_non_canonical_userids @@ -622,6 +641,8 @@ where let options = Options::to_usize(); let no_linting = (options & NoLinting::to_usize()) > 0; + let all_matches_non_self_signed = + (options & AllMatchesNonSelfSigned::to_usize()) > 0; // Can't provide both ExistingUserIDArg and AnyUserIDArg. assert!(! (userid_arg && any_userid_arg)); @@ -639,6 +660,7 @@ where } else { None }; + self.all_matches_non_self_signed = all_matches_non_self_signed; if let Some(Some(userids)) = matches.try_get_many::("userid") @@ -745,6 +767,7 @@ where designators: Vec::new(), arguments: std::marker::PhantomData, all: None, + all_matches_non_self_signed: false, allow_non_canonical_userids: false, }; @@ -790,6 +813,7 @@ impl ResolvedUserID { /// Return implicitly resolved user IDs for all user IDs /// associated with a certificate. + #[allow(dead_code)] pub fn implicit_for_cert(cert: &Cert) -> Vec { cert.userids() .map(|ua| Self::implicit(ua.userid().clone())) diff --git a/src/commands/key/generate.rs b/src/commands/key/generate.rs index f23d36bd..b1551971 100644 --- a/src/commands/key/generate.rs +++ b/src/commands/key/generate.rs @@ -355,6 +355,7 @@ pub fn generate( with:")) .sq().arg("pki").arg("link").arg("retract") .arg_value("--cert", cert.fingerprint()) + .arg("--all") .done(); } @@ -365,6 +366,7 @@ pub fn generate( with:")) .sq().arg("pki").arg("link").arg("retract") .arg_value("--cert", cert.fingerprint()) + .arg("--all") .done(); } diff --git a/src/commands/pki/link.rs b/src/commands/pki/link.rs index 2884fd74..f53156b7 100644 --- a/src/commands/pki/link.rs +++ b/src/commands/pki/link.rs @@ -17,7 +17,6 @@ use crate::parse_notations; use crate::cli::pki::link; use crate::cli::types::Expiration; use crate::cli::types::TrustAmount; -use crate::cli::types::userid_designator::ResolvedUserID; pub fn link(sq: Sq, c: link::Command) -> Result<()> { use link::Subcommands::*; @@ -122,17 +121,7 @@ pub fn retract(sq: Sq, c: link::RetractCommand) = sq.resolve_cert(&c.cert, sequoia_wot::FULLY_TRUSTED)?; let vc = cert.with_policy(NULL_POLICY, Some(sq.time))?; - let mut userids = c.userids.resolve(&vc)?; - - let user_supplied_userids = if userids.is_empty() { - // Nothing was specified. Retract links for all user IDs - // (self signed or not). - userids = ResolvedUserID::implicit_for_cert(&cert); - - false - } else { - true - }; + let userids = c.userids.resolve(&vc)?; let notations = parse_notations(c.notation)?; @@ -142,7 +131,7 @@ pub fn retract(sq: Sq, c: link::RetractCommand) &trust_root, &cert, &userids[..], - user_supplied_userids, + true, // User supplied user IDs. &[(TrustAmount::None, Expiration::Never)], 0, &[][..], &[][..], // Domain, regex. diff --git a/src/common/types/userid_designator.rs b/src/common/types/userid_designator.rs index 356a97fb..9c5898cd 100644 --- a/src/common/types/userid_designator.rs +++ b/src/common/types/userid_designator.rs @@ -58,21 +58,39 @@ where let mut bad = None; if let Some(true) = self.all() { - let all_userids = vc.userids() + let mut revoked_userids = BTreeSet::new(); + let valid_userids = vc.userids() .filter_map(|ua| { if let RevocationStatus::Revoked(_) = ua.revocation_status() { + revoked_userids.insert(ua.userid()); None } else { Some(ua.userid().clone()) } }) + .collect::>(); + + let non_self_signed_userids = vc.cert().userids() + .filter(|_| self.all_matches_non_self_signed()) + .filter(|u| ! revoked_userids.contains(u.userid())) + .filter(|u| ! valid_userids.contains(u.userid())) + .map(|u| u.userid().clone()) + .collect::>(); + + let all_userids = valid_userids.into_iter() + .chain(non_self_signed_userids.into_iter()) .map(|userid| ResolvedUserID::implicit(userid)) .collect::>(); if all_userids.is_empty() { return Err(anyhow::anyhow!( - "{} has no valid self-signed user IDs", - vc.fingerprint())); + "{} has no {}user IDs", + vc.fingerprint(), + if self.all_matches_non_self_signed() { + "" + } else { + "valid self-signed " + })); } userids.extend(all_userids); diff --git a/tests/integration/common.rs b/tests/integration/common.rs index 1458dc3b..d24e979a 100644 --- a/tests/integration/common.rs +++ b/tests/integration/common.rs @@ -519,7 +519,20 @@ impl Sq { pub fn run(&self, mut cmd: Command, expect: E) -> Output where E: Into> { - eprintln!("Running: {:?}", cmd); + eprintln!("Running: {}", + std::iter::once(cmd.get_program()) + .chain(cmd.get_args()) + .map(|arg| { + let arg = arg.to_string_lossy(); + if arg.contains(" ") { + format!("{:?}", arg) + } else { + arg.into_owned() + } + }) + .collect::>() + .join(" ")); + let output = cmd.output().expect("can run command"); let expect = expect.into(); match (output.status.success(), expect) { diff --git a/tests/integration/sq_pki_link.rs b/tests/integration/sq_pki_link.rs index cfe498de..bfc8dde4 100644 --- a/tests/integration/sq_pki_link.rs +++ b/tests/integration/sq_pki_link.rs @@ -137,6 +137,9 @@ fn sq_retract(sq: &Sq, cert: &str, userids: &[&str], emails: &[&str]) for email in emails { cmd.arg("--email").arg(email); } + if userids.is_empty() && emails.is_empty() { + cmd.arg("--all"); + } eprintln!("{:?}", cmd); let output = sq.run(cmd, true); @@ -838,9 +841,9 @@ fn special_names() { check("add", &["--all"], "xxx", false); for name in SPECIAL_STRINGS.iter() { - check("retract", &[], name, true); + check("retract", &["--all"], name, true); } - check("retract", &[], "xxx", false); + check("retract", &["--all"], "xxx", false); for name in SPECIAL_STRINGS.iter() { check("authorize", &["--all", "--unconstrained"], name, true); @@ -848,7 +851,7 @@ fn special_names() { check("authorize", &["--all", "--unconstrained"], "xxx", false); for name in SPECIAL_STRINGS.iter() { - check("retract", &[], name, true); + check("retract", &["--all"], name, true); } - check("retract", &[], "xxx", false); + check("retract", &["--all"], "xxx", false); } diff --git a/tests/integration/sq_pki_link_authorize.rs b/tests/integration/sq_pki_link_authorize.rs index aab5b756..9e28e195 100644 --- a/tests/integration/sq_pki_link_authorize.rs +++ b/tests/integration/sq_pki_link_authorize.rs @@ -459,7 +459,7 @@ fn retract_all() { // Retract all authorizations. It should no longer be considered // a trusted introducer. sq.tick(1); - sq.pki_link_retract(&[], ca.key_handle(), NO_USERIDS); + sq.pki_link_retract(&["--all"], ca.key_handle(), NO_USERIDS); check(&sq, false); }