From f1a99b10d9c9fe74f786559a95a4b02563d436cc Mon Sep 17 00:00:00 2001 From: "Neal H. Walfield" Date: Sun, 9 Jun 2024 13:33:14 +0200 Subject: [PATCH] Change sq key adopt to support the cert store. - Change `sq key adopt` to use the cert store. - See #205. --- NEWS | 2 +- src/cli/key.rs | 17 +- src/commands/key/adopt.rs | 61 ++++-- tests/common.rs | 19 +- tests/sq-key-adopt.rs | 391 +++++++++++++++++++++++++++++--------- 5 files changed, 378 insertions(+), 112 deletions(-) diff --git a/NEWS b/NEWS index c18d887a..86693fcf 100644 --- a/NEWS +++ b/NEWS @@ -58,7 +58,7 @@ - `sq pki certify` can now use the cert store and the key store. - In `sq key adopt`, change the certificate file parameter from a positional parameter to a named parameter, `--cert-file`. - - `sq key adopt` can now use the key store. + - `sq key adopt` can now use the cert store and the key store. * Changes in 0.36.0 - Missing * Changes in 0.35.0 diff --git a/src/cli/key.rs b/src/cli/key.rs index b68c4903..00414c1c 100644 --- a/src/cli/key.rs +++ b/src/cli/key.rs @@ -1003,6 +1003,7 @@ $ sq key adopt --keyring juliet-old.pgp --key 0123456789ABCDEF \\ juliet-new.pgp ", )] +#[clap(group(ArgGroup::new("cert_input").args(&["cert_file", "cert"]).required(true)))] pub struct AdoptCommand { #[clap( short = 'k', @@ -1026,19 +1027,23 @@ pub struct AdoptCommand { pub allow_broken_crypto: bool, #[clap( long, - default_value_t = FileOrStdin::default(), - value_name = "TARGET-KEY", - help = "Add keys to TARGET-KEY or reads keys from stdin if omitted", + help = "Add keys to TARGET-KEY", + value_name = FileOrStdin::VALUE_NAME, )] - pub cert_file: FileOrStdin, + pub cert: Option, + #[clap( + long, + value_name = "TARGET-KEY", + help = "Add keys to TARGET-KEY", + )] + pub cert_file: Option, #[clap( - default_value_t = FileOrStdout::default(), help = FileOrStdout::HELP_OPTIONAL, long, short, value_name = FileOrStdout::VALUE_NAME, )] - pub output: FileOrStdout, + pub output: Option, #[clap( short = 'B', long, diff --git a/src/commands/key/adopt.rs b/src/commands/key/adopt.rs index c579b849..f8416df4 100644 --- a/src/commands/key/adopt.rs +++ b/src/commands/key/adopt.rs @@ -6,7 +6,6 @@ use openpgp::packet::signature::subpacket::SubpacketTag; use openpgp::packet::signature::SignatureBuilder; use openpgp::packet::Key; use openpgp::packet::Signature; -use openpgp::parse::Parse; use openpgp::policy::Policy; use openpgp::serialize::Serialize; use openpgp::types::SignatureType; @@ -18,11 +17,29 @@ use sequoia_openpgp as openpgp; use crate::Sq; use crate::cli; +use crate::cli::types::FileOrStdout; +use crate::cli::types::FileStdinOrKeyHandle; -pub fn adopt(sq: Sq, command: cli::key::AdoptCommand) -> Result<()> +pub fn adopt(sq: Sq, mut command: cli::key::AdoptCommand) -> Result<()> { - let input = command.cert_file.open()?; - let cert = Cert::from_buffered_reader(input)?; + let handle: FileStdinOrKeyHandle = 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"); + }; + + if handle.is_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.lookup_one(handle, None, true)?; let null_policy_; let adoptee_policy: &dyn Policy = if command.allow_broken_crypto { @@ -188,13 +205,6 @@ pub fn adopt(sq: Sq, command: cli::key::AdoptCommand) -> Result<()> let cert = cert.clone().insert_packets(packets.clone())?; - let mut sink = command.output.for_secrets().create_safe(sq.force)?; - if command.binary { - cert.as_tsk().serialize(&mut sink)?; - } else { - cert.as_tsk().armored().serialize(&mut sink)?; - } - let vc = cert.with_policy(sq.policy, None).expect("still valid"); for pair in packets[..].chunks(2) { let newkey: &Key = match pair[0] @@ -224,5 +234,34 @@ pub fn adopt(sq: Sq, command: cli::key::AdoptCommand) -> Result<()> assert!(found, "Subkey: {:?}\nSignature: {:?}", newkey, newsig); } + if let Some(output) = command.output { + let mut sink = output.for_secrets().create_safe(sq.force)?; + if command.binary { + cert.as_tsk().serialize(&mut sink)?; + } else { + cert.as_tsk().armored().serialize(&mut sink)?; + } + } else { + // Import it into the key store. + let keyid = cert.keyid(); + let result = if cert.is_tsk() { + sq.import_key(cert) + } else { + sq.import_cert(cert) + }; + if let Err(err) = result { + wprintln!("Error importing updated cert: {}", err); + return Err(err); + } else { + sq.hint(format_args!( + "Imported updated cert into the cert store. \ + To make the update effective, it has to be published \ + so that others can find it, for example using:")) + .command(format_args!( + "sq cert export --cert {} | sq network keyserver publish", + keyid)); + } + } + Ok(()) } diff --git a/tests/common.rs b/tests/common.rs index db948f72..927eee57 100644 --- a/tests/common.rs +++ b/tests/common.rs @@ -21,6 +21,7 @@ use openpgp::packet::Signature; use openpgp::parse::Parse; use openpgp::policy::StandardPolicy; use openpgp::Cert; +use openpgp::Fingerprint; use openpgp::KeyHandle; use openpgp::Result; use sequoia_openpgp as openpgp; @@ -98,6 +99,18 @@ impl From for FileOrKeyHandle { } } +impl From<&Fingerprint> for FileOrKeyHandle { + fn from(fpr: &Fingerprint) -> Self { + KeyHandle::from(fpr).into() + } +} + +impl From for FileOrKeyHandle { + fn from(fpr: Fingerprint) -> Self { + KeyHandle::from(fpr).into() + } +} + impl From<&FileOrKeyHandle> for FileOrKeyHandle { fn from(h: &FileOrKeyHandle) -> Self { h.clone() @@ -399,7 +412,11 @@ impl Sq { cmd.arg("--keyring").arg(k.as_ref()); } - cmd.arg("--cert-file").arg(target); + if target.is_file() { + cmd.arg("--cert-file").arg(target); + } else { + cmd.arg("--cert").arg(target); + }; assert!(! keys.is_empty()); for k in keys.into_iter() { diff --git a/tests/sq-key-adopt.rs b/tests/sq-key-adopt.rs index 5142e51f..de43077c 100644 --- a/tests/sq-key-adopt.rs +++ b/tests/sq-key-adopt.rs @@ -1,4 +1,5 @@ mod common; +use common::FileOrKeyHandle; use common::Sq; #[cfg(test)] @@ -141,20 +142,42 @@ mod integration { #[test] fn adopt_encryption() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice() ], + // Handle + bob().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), bob() ], + // Handle + bob_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(alice()); - Vec::new() - } else { - vec![ alice() ] - }; + for file in key_imports { + sq.key_import(file); + } // Have Bob adopt alice's encryption subkey. let cert = sq.key_adopt( - keyrings, - bob(), + keyrings.to_vec(), + handle, [ alice_encryption().0.clone() ].to_vec(), None, false, @@ -171,20 +194,42 @@ mod integration { #[test] fn adopt_signing() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice() ], + // Handle + bob().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), bob() ], + // Handle + bob_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(alice()); - Vec::new() - } else { - vec![ alice() ] - }; + for file in key_imports { + sq.key_import(file); + } // Adopt a signing subkey (subkey has secret key material). let cert = sq.key_adopt( - keyrings, - bob(), + keyrings.to_vec(), + handle, [ alice_signing().0.clone() ].to_vec(), None, false, @@ -201,21 +246,43 @@ mod integration { #[test] fn adopt_certification() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(carol()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice() ], + // Handle + carol().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), carol() ], + // Handle + carol_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(alice()); - Vec::new() - } else { - vec![ alice() ] - }; + for file in key_imports { + sq.key_import(file); + } // Adopt a certification subkey (subkey has secret key // material). let cert = sq.key_adopt( - keyrings, - carol(), + keyrings.to_vec(), + handle, [ alice_primary().0.clone() ].to_vec(), None, false, @@ -231,20 +298,41 @@ mod integration { #[test] fn adopt_encryption_and_signing() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice() ], + // Handle + bob().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), bob() ], + // Handle + bob_primary().0.into() + ), + ] { + let sq = Sq::new(); - // Adopt an encryption subkey and a signing subkey. - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(alice()); - Vec::new() - } else { - vec![ alice() ] - }; + for file in key_imports { + sq.key_import(file); + } let cert = sq.key_adopt( - keyrings, - bob(), + keyrings.to_vec(), + handle, [ alice_signing().0.clone(), alice_encryption().0.clone(), @@ -267,20 +355,42 @@ mod integration { #[test] fn adopt_twice() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice() ], + // Handle + bob().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), bob() ], + // Handle + bob_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(alice()); - Vec::new() - } else { - vec![ alice() ] - }; + for file in key_imports { + sq.key_import(file); + } // Adopt the same an encryption subkey twice. let cert = sq.key_adopt( - keyrings, - bob(), + keyrings.to_vec(), + handle, [ alice_encryption().0.clone(), alice_encryption().0.clone(), @@ -302,7 +412,7 @@ mod integration { fn adopt_key_appears_twice() -> Result<()> { let sq = Sq::new(); - // Adopt the an encryption subkey that appears twice. + // Adopt an encryption subkey that appears twice. let cert = sq.key_adopt( [ alice(), alice(), ].to_vec(), bob(), @@ -323,20 +433,42 @@ mod integration { #[test] fn adopt_own_encryption() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(alice()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice() ], + // Handle + alice().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), bob() ], + // Handle + alice_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(alice()); - Vec::new() - } else { - vec![ alice() ] - }; + for file in key_imports { + sq.key_import(file); + } // Adopt its own encryption subkey. This should be a noop. let cert = sq.key_adopt( - keyrings, - alice(), + keyrings.to_vec(), + handle, [ alice_encryption().0.clone(), ].to_vec(), @@ -355,20 +487,42 @@ mod integration { #[test] fn adopt_own_primary() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ bob() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ bob() ], + // Handle + bob().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ bob() ], + // Handle + bob_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(bob()); - Vec::new() - } else { - vec![ bob() ] - }; + for file in key_imports { + sq.key_import(file); + } // Adopt own primary key. let cert = sq.key_adopt( - keyrings, - bob(), + keyrings.to_vec(), + handle, [ bob_primary().0.clone(), ].to_vec(), @@ -387,20 +541,50 @@ mod integration { #[test] fn adopt_missing() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice(), bob() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ bob() ], + // Handle + bob().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ bob() ], + // Handle + bob_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(bob()); - Vec::new() - } else { - vec![ bob() ] - }; + for file in key_imports { + sq.key_import(file); + } // Adopt a key that is not present. let r = sq.key_adopt( - keyrings, - bob(), + keyrings.to_vec(), + handle, [ "1234 5678 90AB CDEF 1234 5678 90AB CDEF" .parse::() @@ -419,21 +603,42 @@ mod integration { #[test] fn adopt_from_multiple() -> Result<()> { - let sq = Sq::new(); + for (keyrings, key_imports, handle) in [ + ( + // Keyrings + &[ alice(), carol() ][..], + // Key store imports. + &[][..], + // Handle + FileOrKeyHandle::from(bob()), + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), carol() ], + // Handle + bob().into() + ), + ( + // Keyrings + &[][..], + // Key store imports. + &[ alice(), bob(), carol(), ], + // Handle + bob_primary().0.into() + ), + ] { + let sq = Sq::new(); - for keystore in [false, true] { - let keyrings = if keystore { - sq.key_import(alice()); - sq.key_import(carol()); - Vec::new() - } else { - vec![ alice(), carol() ] - }; + for file in key_imports { + sq.key_import(file); + } // Adopt own primary key. let cert = sq.key_adopt( - keyrings, - bob(), + keyrings.to_vec(), + handle, [ alice_signing().0.clone(), alice_encryption().0.clone(),