keyfork-shard: slightly improved error handling

Cargo.lock

@@ -1073,6 +1073,7 @@ version = "0.1.0"
 dependencies = [
  "anyhow",
  "bincode",
+ "card-backend",
  "card-backend-pcsc",
  "keyfork-derive-openpgp",
  "keyfork-pinentry",

keyfork-shard/Cargo.toml

@@ -8,12 +8,13 @@ edition = "2021"
 [features]
 default = ["openpgp", "openpgp-card"]
 openpgp = ["sequoia-openpgp", "prompt"]
-openpgp-card = ["openpgp-card-sequoia", "card-backend-pcsc"]
+openpgp-card = ["openpgp-card-sequoia", "card-backend-pcsc", "card-backend"]
 prompt = ["keyfork-pinentry"]
 
 [dependencies]
 anyhow = "1.0.75"
 bincode = "1.3.3"
+card-backend = { version = "0.2.0", optional = true }
 card-backend-pcsc = { version = "0.5.0", optional = true }
 keyfork-derive-openpgp = { version = "0.1.0", path = "../keyfork-derive-openpgp" }
 keyfork-pinentry = { version = "0.5.0", path = "../keyfork-pinentry", optional = true }

keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs

@@ -58,6 +58,11 @@ fn main() -> ExitCode {
     let result = run();
     if let Err(e) = result {
         eprintln!("Error: {e}");
+        let mut source = e.source();
+        while let Some(new_error) = source.take() {
+            eprintln!("Source: {new_error}");
+            source = new_error.source();
+        }
         return ExitCode::FAILURE;
     }
     ExitCode::SUCCESS

keyfork-shard/src/openpgp.rs

@@ -23,7 +23,7 @@ use openpgp::{
         Marshal,
     },
     types::KeyFlags,
-    KeyID, PacketPile,
+    Fingerprint, KeyID, PacketPile,
 };
 pub use sequoia_openpgp as openpgp;
 use sharks::{Share, Sharks};
@@ -34,20 +34,43 @@ use keyring::Keyring;
 mod smartcard;
 use smartcard::SmartcardManager;
 
-// TODO: better error handling
+#[derive(Debug, thiserror::Error)]
+pub enum Error {
+    #[error("Error with creating Share: {0}")]
+    Share(String),
 
-#[derive(Debug, Clone)]
+    #[error("Error combining shares: {0}")]
-pub struct WrappedError(String);
+    CombineShares(String),
 
-impl std::fmt::Display for WrappedError {
+    #[error("Derived secret hash {0} != expected {1}")]
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+    InvalidSecret(Fingerprint, Fingerprint),
-        f.write_str(&self.0)
+
-    }
+    #[error("OpenPGP error: {0}")]
+    Sequoia(#[source] anyhow::Error),
+
+    #[error("OpenPGP IO error: {0}")]
+    SequoiaIo(#[source] std::io::Error),
+
+    #[error("Keyring error: {0}")]
+    Keyring(#[from] keyring::KeyringError),
+
+    #[error("Smartcard error: {0}")]
+    Smartcard(#[from] smartcard::SmartcardError),
+
+    #[error("IO error: {0}")]
+    Io(#[source] std::io::Error),
+
+    #[error("Derivation path: {0}")]
+    DerivationPath(#[from] keyfork_derive_openpgp::derive_util::path::Error),
+
+    #[error("Derivation request: {0}")]
+    DerivationRequest(#[from] keyfork_derive_openpgp::derive_util::request::DerivationError),
+
+    #[error("Keyfork OpenPGP: {0}")]
+    KeyforkOpenPGP(#[from] keyfork_derive_openpgp::Error),
 }
 
-impl std::error::Error for WrappedError {}
+pub type Result<T, E = Error> = std::result::Result<T, E>;
-
-pub type Result<T, E = Box<dyn std::error::Error>> = std::result::Result<T, E>;
 
 #[derive(Debug, Clone)]
 pub struct EncryptedMessage {
@@ -71,24 +94,30 @@ impl EncryptedMessage {
 
         for pkesk in &self.pkesks {
             let mut packet = vec![];
-            pkesk.serialize(&mut packet)?;
+            pkesk.serialize(&mut packet).map_err(Error::Sequoia)?;
             let message = Message::new(&mut packets);
-            let mut message = ArbitraryWriter::new(message, Tag::PKESK)?;
+            let mut message = ArbitraryWriter::new(message, Tag::PKESK).map_err(Error::Sequoia)?;
-            message.write_all(&packet)?;
+            message.write_all(&packet).map_err(Error::SequoiaIo)?;
-            message.finalize()?;
+            message.finalize().map_err(Error::Sequoia)?;
         }
         let mut packet = vec![];
-        self.message.serialize(&mut packet)?;
+        self.message
+            .serialize(&mut packet)
+            .map_err(Error::Sequoia)?;
         let message = Message::new(&mut packets);
-        let mut message = ArbitraryWriter::new(message, Tag::SEIP)?;
+        let mut message = ArbitraryWriter::new(message, Tag::SEIP).map_err(Error::Sequoia)?;
-        message.write_all(&packet)?;
+        message.write_all(&packet).map_err(Error::SequoiaIo)?;
-        message.finalize()?;
+        message.finalize().map_err(Error::Sequoia)?;
 
-        let mut decryptor =
+        let mut decryptor = DecryptorBuilder::from_bytes(&packets)
-            DecryptorBuilder::from_bytes(&packets)?.with_policy(policy, None, decryptor)?;
+            .map_err(Error::Sequoia)?
+            .with_policy(policy, None, decryptor)
+            .map_err(Error::Sequoia)?;
 
         let mut content = vec![];
-        decryptor.read_to_end(&mut content)?;
+        decryptor
+            .read_to_end(&mut content)
+            .map_err(Error::SequoiaIo)?;
         Ok(content)
     }
 }
@@ -98,18 +127,19 @@ pub fn discover_certs(path: impl AsRef<Path>) -> Result<Vec<Cert>> {
 
     if path.is_file() {
         let mut vec = vec![];
-        for cert in CertParser::from_file(path)? {
+        for cert in CertParser::from_file(path).map_err(Error::Sequoia)? {
-            vec.push(cert?);
+            vec.push(cert.map_err(Error::Sequoia)?);
         }
         Ok(vec)
     } else {
         let mut vec = vec![];
         for entry in path
-            .read_dir()?
+            .read_dir()
+            .map_err(Error::Io)?
             .filter_map(Result::ok)
             .filter(|p| p.path().is_file())
         {
-            vec.push(Cert::from_file(entry.path())?);
+            vec.push(Cert::from_file(entry.path()).map_err(Error::Sequoia)?);
         }
         Ok(vec)
     }
@@ -119,7 +149,10 @@ pub fn parse_messages(reader: impl Read + Send + Sync) -> Result<VecDeque<Encryp
     let mut pkesks = Vec::new();
     let mut encrypted_messages = VecDeque::new();
 
-    for packet in PacketPile::from_reader(reader)?.into_children() {
+    for packet in PacketPile::from_reader(reader)
+        .map_err(Error::Sequoia)?
+        .into_children()
+    {
         match packet {
             Packet::PKESK(p) => pkesks.push(p),
             Packet::SEIP(s) => {
@@ -186,13 +219,15 @@ pub fn combine(
         metadata.decrypt_with(&policy, &mut keyring)?
     };
 
-    let mut cert_parser = CertParser::from_bytes(&content)?;
+    let mut cert_parser = CertParser::from_bytes(&content).map_err(Error::Sequoia)?;
     let root_cert = match cert_parser.next() {
         Some(Ok(c)) => c,
         Some(Err(e)) => panic!("Could not find root (first) certificate: {e}"),
         None => panic!("No certs found in cert parser"),
     };
-    let certs = cert_parser.collect::<openpgp::Result<Vec<_>>>()?;
+    let certs = cert_parser
+        .collect::<openpgp::Result<Vec<_>>>()
+        .map_err(Error::Sequoia)?;
     keyring.set_root_cert(root_cert.clone());
     manager.set_root_cert(root_cert);
     let mut messages: HashMap<KeyID, EncryptedMessage> =
@@ -202,12 +237,14 @@ pub fn combine(
     // NOTE: This is ONLY stable because we control the generation of PKESK packets and
     // encode the policy to ourselves.
     for valid_cert in certs.iter().map(|cert| cert.with_policy(&policy, None)) {
-        let valid_cert = valid_cert?;
+        let valid_cert = valid_cert.map_err(Error::Sequoia)?;
         // get keys from keyring for cert
         let Some(secret_cert) = keyring.get_cert_for_primary_keyid(&valid_cert.keyid()) else {
             continue;
         };
-        let secret_cert = secret_cert.with_policy(&policy, None)?;
+        let secret_cert = secret_cert
+            .with_policy(&policy, None)
+            .map_err(Error::Sequoia)?;
         let keys = get_decryption_keys(&secret_cert).collect::<Vec<_>>();
         if !keys.is_empty() {
             if let Some(message) = messages.get_mut(&valid_cert.keyid()) {
@@ -252,7 +289,7 @@ pub fn combine(
                 .iter()
                 .map(|cert| cert.with_policy(&policy, None))
             {
-                let valid_cert = valid_cert?;
+                let valid_cert = valid_cert.map_err(Error::Sequoia)?;
                 let fp = valid_cert
                     .keys()
                     .for_storage_encryption()
@@ -282,8 +319,10 @@ pub fn combine(
         .values()
         .map(|message| Share::try_from(message.as_slice()))
         .collect::<Result<Vec<_>, &str>>()
-        .map_err(|e| WrappedError(e.to_string()))?;
+        .map_err(|e| Error::Share(e.to_string()))?;
-    let secret = Sharks(threshold).recover(&shares)?;
+    let secret = Sharks(threshold)
+        .recover(&shares)
+        .map_err(|e| Error::CombineShares(e.to_string()))?;
 
     let userid = UserID::from("keyfork-sss");
     let kdr = DerivationRequest::new(
@@ -298,19 +337,18 @@ pub fn combine(
     )?;
 
     // NOTE: Signatures on certs will be different. Compare fingerprints instead.
-    if Some(derived_cert.fingerprint()) != keyring.root_cert().map(Cert::fingerprint) {
+    let derived_fp = derived_cert.fingerprint();
-        return Err(WrappedError(format!(
+    let expected_fp = keyring
-            "Derived {} != expected {}",
-            derived_cert.fingerprint(),
-            keyring
         .root_cert()
         .expect("cert was previously set")
-                .fingerprint()
+        .fingerprint();
-        ))
+    if derived_fp != expected_fp {
-        .into());
+        return Err(Error::InvalidSecret(derived_fp, expected_fp));
     }
 
-    output.write_all(smex::encode(&secret).as_bytes())?;
+    output
+        .write_all(smex::encode(&secret).as_bytes())
+        .map_err(Error::Io)?;
 
     Ok(())
 }
@@ -330,23 +368,25 @@ pub fn split(threshold: u8, certs: Vec<Cert>, secret: &[u8], output: impl Write)
     )?;
     let signing_key = derived_cert
         .primary_key()
-        .parts_into_secret()?
+        .parts_into_secret()
+        .map_err(Error::Sequoia)?
         .key()
         .clone()
-        .into_keypair()?;
+        .into_keypair()
+        .map_err(Error::Sequoia)?;
 
     let sharks = Sharks(threshold);
     let dealer = sharks.dealer(secret);
     let shares = dealer.map(|s| Vec::from(&s)).collect::<Vec<_>>();
     let policy = StandardPolicy::new();
-    let mut writer = Writer::new(output, Kind::Message)?;
+    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) {
         total_recipients.push(cert.clone());
-        let valid_cert = cert.with_policy(&policy, None)?;
+        let valid_cert = cert.with_policy(&policy, None).map_err(Error::Sequoia)?;
         let encryption_keys = get_encryption_keys(&valid_cert).collect::<Vec<_>>();
 
         let mut message_output = vec![];
@@ -357,28 +397,34 @@ pub fn split(threshold: u8, certs: Vec<Cert>, secret: &[u8], output: impl Write)
                 .iter()
                 .map(|k| Recipient::new(KeyID::wildcard(), k.key())),
         )
-        .build()?;
+        .build()
-        let message = Signer::new(message, signing_key.clone()).build()?;
+        .map_err(Error::Sequoia)?;
-        let mut message = LiteralWriter::new(message).build()?;
+        let message = Signer::new(message, signing_key.clone())
-        message.write_all(share)?;
+            .build()
-        message.finalize()?;
+            .map_err(Error::Sequoia)?;
+        let mut message = LiteralWriter::new(message)
+            .build()
+            .map_err(Error::Sequoia)?;
+        message.write_all(share).map_err(Error::SequoiaIo)?;
+        message.finalize().map_err(Error::Sequoia)?;
 
         messages.push(message_output);
     }
 
     let mut pp = vec![];
     // store derived cert to verify provided shares
-    derived_cert.serialize(&mut pp)?;
+    derived_cert.serialize(&mut pp).map_err(Error::Sequoia)?;
     for recipient in &total_recipients {
-        recipient.serialize(&mut pp)?;
+        recipient.serialize(&mut pp).map_err(Error::Sequoia)?;
     }
 
     // verify packet pile
-    for (packet_cert, cert) in openpgp::cert::CertParser::from_bytes(&pp)?
+    for (packet_cert, cert) in openpgp::cert::CertParser::from_bytes(&pp)
+        .map_err(Error::Sequoia)?
         .skip(1)
         .zip(total_recipients.iter())
     {
-        if packet_cert? != *cert {
+        if packet_cert.map_err(Error::Sequoia)? != *cert {
             panic!(
                 "packet pile could not recreate cert: {}",
                 cert.fingerprint()
@@ -389,7 +435,8 @@ pub fn split(threshold: u8, certs: Vec<Cert>, secret: &[u8], output: impl Write)
     let valid_certs = total_recipients
         .iter()
         .map(|c| c.with_policy(&policy, None))
-        .collect::<openpgp::Result<Vec<_>>>()?;
+        .collect::<openpgp::Result<Vec<_>>>()
+        .map_err(Error::Sequoia)?;
 
     let total_recipients = valid_certs.iter().flat_map(|vc| {
         get_encryption_keys(vc).map(|key| Recipient::new(KeyID::wildcard(), key.key()))
@@ -398,17 +445,23 @@ pub fn split(threshold: u8, certs: Vec<Cert>, secret: &[u8], output: impl Write)
     // metadata
     let mut message_output = vec![];
     let message = Message::new(&mut message_output);
-    let message = Encryptor2::for_recipients(message, total_recipients).build()?;
+    let message = Encryptor2::for_recipients(message, total_recipients)
-    let mut message = LiteralWriter::new(message).build()?;
+        .build()
-    message.write_all(&pp)?;
+        .map_err(Error::Sequoia)?;
-    message.finalize()?;
+    let mut message = LiteralWriter::new(message)
-    writer.write_all(&message_output)?;
+        .build()
+        .map_err(Error::Sequoia)?;
+    message.write_all(&pp).map_err(Error::SequoiaIo)?;
+    message.finalize().map_err(Error::Sequoia)?;
+    writer
+        .write_all(&message_output)
+        .map_err(Error::SequoiaIo)?;
 
     for message in messages {
-        writer.write_all(&message)?;
+        writer.write_all(&message).map_err(Error::SequoiaIo)?;
     }
 
-    writer.finalize()?;
+    writer.finalize().map_err(Error::SequoiaIo)?;
 
     Ok(())
 }

keyfork-shard/src/openpgp/keyring.rs

@@ -9,27 +9,18 @@ use super::openpgp::{
     KeyHandle, KeyID,
 };
 
-use crate::prompt_manager::PromptManager;
+use crate::prompt_manager::{PromptManager, PinentryError};
 
-#[derive(Clone, Debug)]
+#[derive(thiserror::Error, Debug)]
-pub enum KeyringFailure {
+pub enum KeyringError {
+    #[error("Secret key was not found")]
     SecretKeyNotFound,
-    #[allow(dead_code)]
+
-    SmartcardDecrypt,
+    #[error("Prompt failed: {0}")]
+    Prompt(#[from] PinentryError)
 }
 
-impl std::fmt::Display for KeyringFailure {
+pub type Result<T, E = KeyringError> = std::result::Result<T, E>;
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            KeyringFailure::SecretKeyNotFound => f.write_str("Secret key was not found"),
-            KeyringFailure::SmartcardDecrypt => {
-                f.write_str("Smartcard could not decrypt any PKESKs")
-            }
-        }
-    }
-}
-
-impl std::error::Error for KeyringFailure {}
 
 pub struct Keyring {
     full_certs: Vec<Cert>,
@@ -38,7 +29,7 @@ pub struct Keyring {
 }
 
 impl Keyring {
-    pub fn new(certs: impl AsRef<[Cert]>) -> Result<Self, Box<dyn std::error::Error>> {
+    pub fn new(certs: impl AsRef<[Cert]>) -> Result<Self> {
         Ok(Self {
             full_certs: certs.as_ref().to_vec(),
             root: Default::default(),
@@ -164,6 +155,6 @@ impl DecryptionHelper for &mut Keyring {
             }
         }
 
-        Err(KeyringFailure::SecretKeyNotFound.into())
+        Err(KeyringError::SecretKeyNotFound.into())
     }
 }

keyfork-shard/src/openpgp/smartcard.rs

@@ -9,27 +9,30 @@ use super::openpgp::{
     parse::stream::{DecryptionHelper, MessageLayer, MessageStructure, VerificationHelper},
     Fingerprint,
 };
-use crate::prompt_manager::PromptManager;
+use crate::prompt_manager::{PinentryError, PromptManager};
 
 use card_backend_pcsc::PcscBackend;
 use openpgp_card_sequoia::{state::Open, Card};
 
-#[derive(Clone, Debug)]
+#[derive(thiserror::Error, Debug)]
-pub enum SmartcardFailure {
+pub enum SmartcardError {
+    #[error("No smart card backend was stored")]
     SmartCardNotFound,
+
+    #[error("Selected smart card has no decryption key")]
     SmartCardHasNoDecrypt,
+
+    #[error("Smart card backend error: {0}")]
+    SmartCardBackendError(#[from] card_backend::SmartcardError),
+
+    #[error("Smart card error: {0}")]
+    SmartCardEncounteredError(#[from] openpgp_card_sequoia::types::Error),
+
+    #[error("Prompt failed: {0}")]
+    Prompt(#[from] PinentryError),
 }
 
-impl std::fmt::Display for SmartcardFailure {
+pub type Result<T, E = SmartcardError> = std::result::Result<T, E>;
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            Self::SmartCardNotFound => f.write_str("No smart card backend was stored"),
-            Self::SmartCardHasNoDecrypt => f.write_str("Selected smart card has no decrypt key"),
-        }
-    }
-}
-
-impl std::error::Error for SmartcardFailure {}
 
 fn format_name(input: impl AsRef<str>) -> String {
     let mut n = input
@@ -49,7 +52,7 @@ pub struct SmartcardManager {
 }
 
 impl SmartcardManager {
-    pub fn new() -> Result<Self, Box<dyn std::error::Error>> {
+    pub fn new() -> Result<Self> {
         Ok(Self {
             current_card: None,
             root: None,
@@ -65,20 +68,20 @@ impl SmartcardManager {
     }
 
     /// Load any backend.
-    pub fn load_any_card(&mut self) -> Result<Fingerprint, Box<dyn std::error::Error>> {
+    pub fn load_any_card(&mut self) -> Result<Fingerprint> {
         PcscBackend::cards(None)?
             .next()
             .transpose()?
-            .ok_or(SmartcardFailure::SmartCardNotFound.into())
+            .ok_or(SmartcardError::SmartCardNotFound)
             .and_then(
-                |backend| -> Result<Fingerprint, Box<dyn std::error::Error>> {
+                |backend| {
                     let mut card = Card::<Open>::new(backend)?;
                     let transaction = card.transaction()?;
                     let fingerprint = transaction
                         .fingerprints()?
                         .decryption()
                         .map(|fp| Fingerprint::from_bytes(fp.as_bytes()))
-                        .ok_or(SmartcardFailure::SmartCardHasNoDecrypt)?;
+                        .ok_or(SmartcardError::SmartCardHasNoDecrypt)?;
                     drop(transaction);
                     self.current_card.replace(card);
                     Ok(fingerprint)
@@ -92,7 +95,7 @@ impl SmartcardManager {
     pub fn load_any_fingerprint(
         &mut self,
         fingerprints: impl IntoIterator<Item = Fingerprint>,
-    ) -> Result<Option<Fingerprint>, Box<dyn std::error::Error>> {
+    ) -> Result<Option<Fingerprint>> {
         // NOTE: This can't be HashSet::from_iter() because from_iter() requires a passed-in state
         // I do not want to provide.
         let mut requested_fingerprints = HashSet::new();
@@ -178,7 +181,7 @@ impl DecryptionHelper for &mut SmartcardManager {
     {
         let mut card = self.current_card.take();
         let Some(card) = card.as_mut() else {
-            return Err(SmartcardFailure::SmartCardNotFound.into());
+            return Err(SmartcardError::SmartCardNotFound.into());
         };
 
         let mut transaction = card.transaction()?;
@@ -187,7 +190,7 @@ impl DecryptionHelper for &mut SmartcardManager {
             .decryption()
             .map(|fp| Fingerprint::from_bytes(fp.as_bytes()));
         let Some(fp) = fp else {
-            return Err(SmartcardFailure::SmartCardHasNoDecrypt.into());
+            return Err(SmartcardError::SmartCardHasNoDecrypt.into());
         };
         let cardholder_name = format_name(transaction.cardholder_name()?);
         let card_id = transaction.application_identifier()?.ident();
@@ -210,6 +213,6 @@ impl DecryptionHelper for &mut SmartcardManager {
             }
         }
 
-        Err(SmartcardFailure::SmartCardNotFound.into())
+        Err(SmartcardError::SmartCardNotFound.into())
     }
 }