From f88a4d21f2605956e6ba2ff8486928f852cd83ac Mon Sep 17 00:00:00 2001 From: ryan Date: Sat, 6 Jan 2024 23:23:03 -0500 Subject: [PATCH] keyfork-shard: make clippy happy --- .../src/bin/keyfork-shard-combine-openpgp.rs | 2 +- .../src/bin/keyfork-shard-decrypt-openpgp.rs | 2 +- keyfork-shard/src/lib.rs | 16 ++- keyfork-shard/src/openpgp.rs | 97 ++++++++++++------- keyfork-shard/src/openpgp/keyring.rs | 5 +- keyfork-shard/src/openpgp/smartcard.rs | 12 ++- keyfork/src/cli/shard.rs | 2 +- 7 files changed, 86 insertions(+), 50 deletions(-) diff --git a/keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs b/keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs index 5a461ed..c7c4198 100644 --- a/keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs +++ b/keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs @@ -42,7 +42,7 @@ fn run() -> Result<()> { combine( cert_list, - encrypted_metadata, + &encrypted_metadata, encrypted_messages.into(), stdout(), )?; diff --git a/keyfork-shard/src/bin/keyfork-shard-decrypt-openpgp.rs b/keyfork-shard/src/bin/keyfork-shard-decrypt-openpgp.rs index 015dcd1..7290023 100644 --- a/keyfork-shard/src/bin/keyfork-shard-decrypt-openpgp.rs +++ b/keyfork-shard/src/bin/keyfork-shard-decrypt-openpgp.rs @@ -43,7 +43,7 @@ fn run() -> Result<()> { decrypt( &cert_list, - encrypted_metadata, + &encrypted_metadata, encrypted_messages.make_contiguous(), )?; diff --git a/keyfork-shard/src/lib.rs b/keyfork-shard/src/lib.rs index f23898a..ae4db50 100644 --- a/keyfork-shard/src/lib.rs +++ b/keyfork-shard/src/lib.rs @@ -24,6 +24,10 @@ pub enum SharksError { CombineShare(String), } +#[derive(thiserror::Error, Debug)] +#[error("Mnemonic did not store enough data")] +pub struct InvalidMnemonicData; + /// Decrypt hunk version 1: /// 1 byte: Version /// 1 byte: Threshold @@ -31,6 +35,10 @@ pub enum SharksError { pub(crate) const HUNK_VERSION: u8 = 1; pub(crate) const HUNK_OFFSET: usize = 2; +/// # Panics +/// +/// The function may panic if it is given payloads generated using a version of Keyfork that is +/// incompatible with the currently running version. pub fn remote_decrypt() -> Result<(), Box> { let mut pm = PromptManager::new(stdin(), stdout())?; let wordlist = Wordlist::default(); @@ -48,12 +56,12 @@ pub fn remote_decrypt() -> Result<(), Box> { let key_mnemonic = Mnemonic::from_entropy(PublicKey::from(&our_key).as_bytes(), Default::default())?; let combined_mnemonic = format!("{nonce_mnemonic} {key_mnemonic}"); - pm.prompt_message(PromptMessage::Text(format!( + pm.prompt_message(&PromptMessage::Text(format!( "Our words: {combined_mnemonic}" )))?; if let Ok(qrcode) = qrencode::qrencode(&combined_mnemonic) { - pm.prompt_message(PromptMessage::Data(qrcode))?; + pm.prompt_message(&PromptMessage::Data(qrcode))?; } let their_words = pm.prompt_wordlist("Their words: ", &wordlist)?; @@ -76,13 +84,13 @@ pub fn remote_decrypt() -> Result<(), Box> { } let their_key = Mnemonic::from_str(&pubkey_mnemonic)?.entropy(); - let their_key: [u8; 32] = their_key.try_into().expect("24 words"); + let their_key: [u8; 32] = their_key.try_into().map_err(|_| InvalidMnemonicData)?; let shared_secret = our_key .diffie_hellman(&PublicKey::from(their_key)) .to_bytes(); let shared_key = - Aes256Gcm::new_from_slice(&shared_secret).expect("Invalid length of constant key size"); + Aes256Gcm::new_from_slice(&shared_secret)?; let payload = Mnemonic::from_str(&payload_mnemonic)?.entropy(); let payload = diff --git a/keyfork-shard/src/openpgp.rs b/keyfork-shard/src/openpgp.rs index 5aefb82..daaaadd 100644 --- a/keyfork-shard/src/openpgp.rs +++ b/keyfork-shard/src/openpgp.rs @@ -7,6 +7,7 @@ use std::{ use aes_gcm::{ aead::{consts::U12, Aead}, + aes::cipher::InvalidLength, Aes256Gcm, Error as AesError, KeyInit, Nonce, }; use keyfork_derive_openpgp::derive_util::{ @@ -44,11 +45,11 @@ use smartcard::SmartcardManager; /// Shard metadata verson 1: /// 1 byte: Version /// 1 byte: Threshold -/// OpenPGP Packet Pile of Certs +/// Packet Pile of Certs const SHARD_METADATA_VERSION: u8 = 1; const SHARD_METADATA_OFFSET: usize = 2; -use super::{SharksError, HUNK_VERSION}; +use super::{SharksError, InvalidMnemonicData, HUNK_VERSION}; // 256 bit share is 49 bytes + some amount of hunk bytes, gives us reasonable padding const ENC_LEN: u8 = 4 * 16; @@ -61,6 +62,9 @@ pub enum Error { #[error("Error decrypting share: {0}")] SymDecryptShare(#[from] AesError), + #[error("Invalid length of AES key: {0}")] + AesLength(#[from] InvalidLength), + #[error("Derived secret hash {0} != expected {1}")] InvalidSecret(Fingerprint, Fingerprint), @@ -85,6 +89,9 @@ pub enum Error { #[error("Mnemonic parse error: {0}")] MnemonicFromStr(#[from] MnemonicFromStrError), + #[error("{0}")] + InvalidMnemonicData(#[from] InvalidMnemonicData), + #[error("IO error: {0}")] Io(#[source] std::io::Error), @@ -173,6 +180,10 @@ pub fn discover_certs(path: impl AsRef) -> Result> { } } +/// # Panics +/// +/// When given packets that are not a list of PKESK packets and SEIP packets, the function panics. +/// The `split` utility should never give packets that are not in this format. pub fn parse_messages(reader: impl Read + Send + Sync) -> Result> { let mut pkesks = Vec::new(); let mut encrypted_messages = VecDeque::new(); @@ -255,7 +266,7 @@ fn decrypt_with_manager( threshold: u8, messages: &mut HashMap, certs: &[Cert], - policy: NullPolicy, + policy: &dyn Policy, manager: &mut SmartcardManager, ) -> Result>> { let mut decrypted_messages = HashMap::new(); @@ -267,7 +278,7 @@ fn decrypt_with_manager( for valid_cert in certs .iter() .filter(|cert| !decrypted_messages.contains_key(&cert.keyid())) - .map(|cert| cert.with_policy(&policy, None)) + .map(|cert| cert.with_policy(policy, None)) { let valid_cert = valid_cert.map_err(Error::Sequoia)?; let fp = valid_cert @@ -285,7 +296,7 @@ fn decrypt_with_manager( if let Some(fp) = manager.load_any_fingerprint(unused_fingerprints)? { let cert_keyid = cert_by_fingerprint.get(&fp).unwrap().clone(); if let Some(message) = messages.remove(&cert_keyid) { - let message = message.decrypt_with(&policy, &mut *manager)?; + let message = message.decrypt_with(policy, &mut *manager)?; decrypted_messages.insert(cert_keyid, message); } } @@ -347,14 +358,14 @@ fn decrypt_metadata( fn decrypt_one( messages: Vec, certs: &[Cert], - metadata: EncryptedMessage, + metadata: &EncryptedMessage, ) -> Result<(Vec, u8, Cert)> { let policy = NullPolicy::new(); let mut keyring = Keyring::new(certs)?; let mut manager = SmartcardManager::new()?; - let content = decrypt_metadata(&metadata, &policy, &mut keyring, &mut manager)?; + let content = decrypt_metadata(metadata, &policy, &mut keyring, &mut manager)?; let (threshold, root_cert, certs) = decode_metadata_v1(&content)?; @@ -362,7 +373,7 @@ fn decrypt_one( manager.set_root_cert(root_cert.clone()); let mut messages: HashMap = - HashMap::from_iter(certs.iter().map(|c| c.keyid()).zip(messages)); + certs.iter().map(Cert::keyid).zip(messages).collect(); let decrypted_messages = decrypt_with_keyring(&mut messages, &certs, &policy, &mut keyring)?; @@ -370,7 +381,7 @@ fn decrypt_one( return Ok((message, threshold, root_cert)); } - let decrypted_messages = decrypt_with_manager(1, &mut messages, &certs, policy, &mut manager)?; + let decrypted_messages = decrypt_with_manager(1, &mut messages, &certs, &policy, &mut manager)?; if let Some(message) = decrypted_messages.into_values().next() { return Ok((message, threshold, root_cert)); @@ -379,9 +390,14 @@ fn decrypt_one( unreachable!("smartcard manager should always decrypt") } +/// # Panics +/// +/// The function may panic if a share is decrypted but has a length larger than 256 bits. This is +/// atypical usage and should not be encountered in normal usage, unless something that is not a +/// Keyfork seed has been fed into [`split`]. pub fn decrypt( certs: &[Cert], - metadata: EncryptedMessage, + metadata: &EncryptedMessage, encrypted_messages: &[EncryptedMessage], ) -> Result<()> { let mut pm = PromptManager::new(stdin(), stdout())?; @@ -404,7 +420,9 @@ pub fn decrypt( } } let their_key = Mnemonic::from_str(&pubkey_mnemonic)?.entropy(); - let their_key: [u8; 32] = their_key.try_into().expect("24 words"); + let their_key: [u8; 32] = their_key + .try_into() + .map_err(|_| InvalidMnemonicData)?; let their_nonce = Mnemonic::from_str(&nonce_mnemonic)?.entropy(); let their_nonce = Nonce::::from_slice(&their_nonce); @@ -416,7 +434,7 @@ pub fn decrypt( .diffie_hellman(&PublicKey::from(their_key)) .to_bytes(); - let (mut share, threshold, ..) = decrypt_one(encrypted_messages.to_vec(), &certs, metadata)?; + let (mut share, threshold, ..) = decrypt_one(encrypted_messages.to_vec(), certs, metadata)?; share.insert(0, HUNK_VERSION); share.insert(1, threshold); assert!( @@ -424,8 +442,7 @@ pub fn decrypt( "invalid share length (too long, max {ENC_LEN} bytes)" ); - let shared_key = - Aes256Gcm::new_from_slice(&shared_secret).expect("Invalid length of constant key size"); + let shared_key = Aes256Gcm::new_from_slice(&shared_secret)?; let bytes = shared_key.encrypt(their_nonce, share.as_slice())?; shared_key.decrypt(their_nonce, &bytes[..])?; @@ -434,15 +451,20 @@ pub fn decrypt( // difficulty when entering in prompts manually, as one's place could be lost due to repeated // keywords. This is done below by having sequentially increasing numbers up to but not // including the last byte. - assert!(ENC_LEN < u8::MAX, "padding byte can be u8"); + #[allow(clippy::assertions_on_constants)] + { + assert!(ENC_LEN < u8::MAX, "padding byte can be u8"); + } + #[allow(clippy::cast_possible_truncation)] let mut out_bytes = [bytes.len() as u8; ENC_LEN as usize]; assert!( bytes.len() < out_bytes.len(), "encrypted payload larger than acceptable limit" ); out_bytes[..bytes.len()].clone_from_slice(&bytes); - for (i, byte) in (&mut out_bytes[bytes.len()..(ENC_LEN as usize - 1)]) - .into_iter() + #[allow(clippy::cast_possible_truncation)] + for (i, byte) in (out_bytes[bytes.len()..(ENC_LEN as usize - 1)]) + .iter_mut() .enumerate() { *byte = (i % u8::MAX as usize) as u8; @@ -452,12 +474,12 @@ pub fn decrypt( let mnemonic = unsafe { Mnemonic::from_raw_entropy(&out_bytes, Default::default()) }; let combined_mnemonic = format!("{our_mnemonic} {mnemonic}"); - pm.prompt_message(PromptMessage::Text(format!( + pm.prompt_message(&PromptMessage::Text(format!( "Our words: {combined_mnemonic}" )))?; if let Ok(qrcode) = qrencode::qrencode(&combined_mnemonic) { - pm.prompt_message(PromptMessage::Data(qrcode))?; + pm.prompt_message(&PromptMessage::Data(qrcode))?; } Ok(()) @@ -465,7 +487,7 @@ pub fn decrypt( pub fn combine( certs: Vec, - metadata: EncryptedMessage, + metadata: &EncryptedMessage, messages: Vec, mut output: impl Write, ) -> Result<()> { @@ -475,7 +497,7 @@ pub fn combine( let mut keyring = Keyring::new(certs)?; let mut manager = SmartcardManager::new()?; - let content = decrypt_metadata(&metadata, &policy, &mut keyring, &mut manager)?; + let content = decrypt_metadata(metadata, &policy, &mut keyring, &mut manager)?; let (threshold, root_cert, certs) = decode_metadata_v1(&content)?; @@ -486,7 +508,7 @@ pub fn combine( // because we control the order packets are encrypted and certificates are stored. let mut messages: HashMap = - HashMap::from_iter(certs.iter().map(|c| c.keyid()).zip(messages)); + certs.iter().map(Cert::keyid).zip(messages).collect(); let mut decrypted_messages = decrypt_with_keyring(&mut messages, &certs, &policy, &mut keyring)?; @@ -496,14 +518,15 @@ pub fn combine( let left_from_threshold = threshold as usize - decrypted_messages.len(); if left_from_threshold > 0 { + #[allow(clippy::cast_possible_truncation)] let new_messages = decrypt_with_manager( left_from_threshold as u8, &mut messages, &certs, - policy, + &policy, &mut manager, )?; - decrypted_messages.extend(new_messages.into_iter()); + decrypted_messages.extend(new_messages); } let shares = decrypted_messages @@ -521,11 +544,11 @@ pub fn combine( DerivationAlgorithm::Ed25519, &DerivationPath::from_str("m/7366512'/0'")?, ) - .derive_with_master_seed(secret.to_vec())?; + .derive_with_master_seed(secret.clone())?; let derived_cert = keyfork_derive_openpgp::derive( kdr, &[KeyFlags::empty().set_certification().set_signing()], - userid, + &userid, )?; // NOTE: Signatures on certs will be different. Compare fingerprints instead. @@ -542,6 +565,10 @@ pub fn combine( Ok(()) } +/// # Panics +/// +/// The function may panic if the metadata can't properly store the certificates used to generate +/// the encrypted shares. pub fn split(threshold: u8, certs: Vec, secret: &[u8], output: impl Write) -> Result<()> { // build cert to sign encrypted shares let userid = UserID::from("keyfork-sss"); @@ -553,7 +580,7 @@ pub fn split(threshold: u8, certs: Vec, secret: &[u8], output: impl Write) let derived_cert = keyfork_derive_openpgp::derive( kdr, &[KeyFlags::empty().set_certification().set_signing()], - userid, + &userid, )?; let signing_key = derived_cert .primary_key() @@ -566,14 +593,14 @@ pub fn split(threshold: u8, certs: Vec, secret: &[u8], output: impl Write) let sharks = Sharks(threshold); let dealer = sharks.dealer(secret); - let shares = dealer.map(|s| Vec::from(&s)).collect::>(); + let generated_shares = dealer.map(|s| Vec::from(&s)).collect::>(); let policy = StandardPolicy::new(); let mut writer = Writer::new(output, Kind::Message).map_err(Error::SequoiaIo)?; let mut total_recipients = vec![]; let mut messages = vec![]; - for (share, cert) in shares.iter().zip(certs) { + for (share, cert) in generated_shares.iter().zip(certs) { total_recipients.push(cert.clone()); let valid_cert = cert.with_policy(&policy, None).map_err(Error::Sequoia)?; let encryption_keys = get_encryption_keys(&valid_cert).collect::>(); @@ -613,12 +640,12 @@ pub fn split(threshold: u8, certs: Vec, secret: &[u8], output: impl Write) .skip(1) .zip(total_recipients.iter()) { - if packet_cert.map_err(Error::Sequoia)? != *cert { - panic!( - "packet pile could not recreate cert: {}", - cert.fingerprint() - ); - } + assert_eq!( + &packet_cert.map_err(Error::Sequoia)?, + cert, + "packet pile could not recreate cert: {}", + cert.fingerprint() + ); } let valid_certs = total_recipients diff --git a/keyfork-shard/src/openpgp/keyring.rs b/keyfork-shard/src/openpgp/keyring.rs index 9ade88a..d380a4c 100644 --- a/keyfork-shard/src/openpgp/keyring.rs +++ b/keyfork-shard/src/openpgp/keyring.rs @@ -70,7 +70,7 @@ impl VerificationHelper for &mut Keyring { .collect()) } fn check(&mut self, structure: MessageStructure) -> openpgp::Result<()> { - for layer in structure.into_iter() { + for layer in structure { #[allow(unused_variables)] match layer { MessageLayer::Compression { algo } => {} @@ -149,8 +149,7 @@ impl DecryptionHelper for &mut Keyring { }; if pkesk .decrypt(&mut keypair, sym_algo) - .map(|(algo, sk)| decrypt(algo, &sk)) - .unwrap_or(false) + .is_some_and(|(algo, sk)| decrypt(algo, &sk)) { return Ok(Some(key.fingerprint())); } diff --git a/keyfork-shard/src/openpgp/smartcard.rs b/keyfork-shard/src/openpgp/smartcard.rs index b8fb690..5bef729 100644 --- a/keyfork-shard/src/openpgp/smartcard.rs +++ b/keyfork-shard/src/openpgp/smartcard.rs @@ -61,6 +61,7 @@ fn format_name(input: impl AsRef) -> String { n.join(" ") } +#[allow(clippy::module_name_repetitions)] pub struct SmartcardManager { current_card: Option>, root: Option, @@ -146,7 +147,7 @@ impl SmartcardManager { } #[rustfmt::skip] - self.pm.prompt_message(Message::Text("Please plug in a smart card and press enter".to_string()))?; + self.pm.prompt_message(&Message::Text("Please plug in a smart card and press enter".to_string()))?; } Ok(None) @@ -155,6 +156,7 @@ impl SmartcardManager { impl VerificationHelper for &mut SmartcardManager { fn get_certs(&mut self, ids: &[openpgp::KeyHandle]) -> openpgp::Result> { + #[allow(clippy::flat_map_option)] Ok(ids .iter() .flat_map(|kh| self.root.as_ref().filter(|cert| cert.key_handle() == *kh)) @@ -163,7 +165,7 @@ impl VerificationHelper for &mut SmartcardManager { } fn check(&mut self, structure: MessageStructure) -> openpgp::Result<()> { - for layer in structure.into_iter() { + for layer in structure { #[allow(unused_variables)] match layer { MessageLayer::Compression { algo } => {} @@ -241,12 +243,13 @@ impl DecryptionHelper for &mut SmartcardManager { let temp_pin = self.pm.prompt_passphrase(&message)?; let verification_status = transaction.verify_user_pin(temp_pin.as_str().trim()); match verification_status { + #[allow(clippy::ignored_unit_patterns)] Ok(_) => { self.pin_cache.insert(fp.clone(), temp_pin.clone()); pin.replace(temp_pin); } Err(CardError::CardStatus(StatusBytes::IncorrectParametersCommandDataField)) => { - self.pm.prompt_message(Message::Text("Invalid PIN length entered.".to_string()))?; + self.pm.prompt_message(&Message::Text("Invalid PIN length entered.".to_string()))?; } Err(_) => {} } @@ -261,8 +264,7 @@ impl DecryptionHelper for &mut SmartcardManager { for pkesk in pkesks { if pkesk .decrypt(&mut decryptor, sym_algo) - .map(|(algo, sk)| decrypt(algo, &sk)) - .unwrap_or(false) + .is_some_and(|(algo, sk)| decrypt(algo, &sk)) { return Ok(Some(fp)); } diff --git a/keyfork/src/cli/shard.rs b/keyfork/src/cli/shard.rs index 8b80e44..e9bce3f 100644 --- a/keyfork/src/cli/shard.rs +++ b/keyfork/src/cli/shard.rs @@ -88,7 +88,7 @@ impl ShardExec for OpenPGP { keyfork_shard::openpgp::combine( certs, - encrypted_metadata, + &encrypted_metadata, encrypted_messages.into(), output, )?;