keyfork-shard: make clippy happy

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(),
     )?;

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(),
     )?;
 

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<dyn std::error::Error>> {
     let mut pm = PromptManager::new(stdin(), stdout())?;
     let wordlist = Wordlist::default();
@@ -48,12 +56,12 @@ pub fn remote_decrypt() -> Result<(), Box<dyn std::error::Error>> {
         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<dyn std::error::Error>> {
         }
 
         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 =

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<Path>) -> Result<Vec<Cert>> {
     }
 }
 
+/// # 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<VecDeque<EncryptedMessage>> {
     let mut pkesks = Vec::new();
     let mut encrypted_messages = VecDeque::new();
@@ -255,7 +266,7 @@ fn decrypt_with_manager(
     threshold: u8,
     messages: &mut HashMap<KeyID, EncryptedMessage>,
     certs: &[Cert],
-    policy: NullPolicy,
+    policy: &dyn Policy,
     manager: &mut SmartcardManager,
 ) -> Result<HashMap<KeyID, Vec<u8>>> {
     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<EncryptedMessage>,
     certs: &[Cert],
-    metadata: EncryptedMessage,
+    metadata: &EncryptedMessage,
 ) -> Result<(Vec<u8>, 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<KeyID, EncryptedMessage> =
-        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::<U12>::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 =
+    let shared_key = Aes256Gcm::new_from_slice(&shared_secret)?;
-        Aes256Gcm::new_from_slice(&shared_secret).expect("Invalid length of constant key size");
     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.
+    #[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)])
+    #[allow(clippy::cast_possible_truncation)]
-        .into_iter()
+    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<Cert>,
-    metadata: EncryptedMessage,
+    metadata: &EncryptedMessage,
     messages: Vec<EncryptedMessage>,
     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<KeyID, EncryptedMessage> =
-        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<Cert>, 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<Cert>, 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<Cert>, secret: &[u8], output: impl Write)
 
     let sharks = Sharks(threshold);
     let dealer = sharks.dealer(secret);
-    let shares = dealer.map(|s| Vec::from(&s)).collect::<Vec<_>>();
+    let generated_shares = dealer.map(|s| Vec::from(&s)).collect::<Vec<_>>();
     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::<Vec<_>>();
@@ -613,13 +640,13 @@ pub fn split(threshold: u8, certs: Vec<Cert>, secret: &[u8], output: impl Write)
         .skip(1)
         .zip(total_recipients.iter())
     {
-        if packet_cert.map_err(Error::Sequoia)? != *cert {
+        assert_eq!(
-            panic!(
+            &packet_cert.map_err(Error::Sequoia)?,
+            cert,
             "packet pile could not recreate cert: {}",
             cert.fingerprint()
         );
     }
-    }
 
     let valid_certs = total_recipients
         .iter()

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))
+                        .is_some_and(|(algo, sk)| decrypt(algo, &sk))
-                        .unwrap_or(false)
                     {
                         return Ok(Some(key.fingerprint()));
                     }

keyfork-shard/src/openpgp/smartcard.rs

@@ -61,6 +61,7 @@ fn format_name(input: impl AsRef<str>) -> String {
     n.join(" ")
 }
 
+#[allow(clippy::module_name_repetitions)]
 pub struct SmartcardManager {
     current_card: Option<Card<Open>>,
     root: Option<Cert>,
@@ -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<Vec<Cert>> {
+        #[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))
+                .is_some_and(|(algo, sk)| decrypt(algo, &sk))
-                .unwrap_or(false)
             {
                 return Ok(Some(fp));
             }

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,
         )?;