From 307941087a414f11e26b0cd1b3b49d10a38b8727 Mon Sep 17 00:00:00 2001 From: ryan Date: Sun, 5 Nov 2023 13:57:22 -0600 Subject: [PATCH] keyfork-shard: slightly improved error handling --- Cargo.lock | 1 + keyfork-shard/Cargo.toml | 3 +- .../src/bin/keyfork-shard-combine-openpgp.rs | 5 + keyfork-shard/src/openpgp.rs | 185 +++++++++++------- keyfork-shard/src/openpgp/keyring.rs | 29 +-- keyfork-shard/src/openpgp/smartcard.rs | 47 ++--- 6 files changed, 162 insertions(+), 108 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index fb0badb..57691de 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1073,6 +1073,7 @@ version = "0.1.0" dependencies = [ "anyhow", "bincode", + "card-backend", "card-backend-pcsc", "keyfork-derive-openpgp", "keyfork-pinentry", diff --git a/keyfork-shard/Cargo.toml b/keyfork-shard/Cargo.toml index 221a0e2..2bf7786 100644 --- a/keyfork-shard/Cargo.toml +++ b/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 } diff --git a/keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs b/keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs index 4edf5c3..883a528 100644 --- a/keyfork-shard/src/bin/keyfork-shard-combine-openpgp.rs +++ b/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 diff --git a/keyfork-shard/src/openpgp.rs b/keyfork-shard/src/openpgp.rs index 14351b6..eda5d19 100644 --- a/keyfork-shard/src/openpgp.rs +++ b/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)] -pub struct WrappedError(String); + #[error("Error combining shares: {0}")] + CombineShares(String), -impl std::fmt::Display for WrappedError { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&self.0) - } + #[error("Derived secret hash {0} != expected {1}")] + InvalidSecret(Fingerprint, Fingerprint), + + #[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> = std::result::Result; +pub type Result = std::result::Result; #[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)?; - message.write_all(&packet)?; - message.finalize()?; + let mut message = ArbitraryWriter::new(message, Tag::PKESK).map_err(Error::Sequoia)?; + message.write_all(&packet).map_err(Error::SequoiaIo)?; + 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)?; - message.write_all(&packet)?; - message.finalize()?; + let mut message = ArbitraryWriter::new(message, Tag::SEIP).map_err(Error::Sequoia)?; + message.write_all(&packet).map_err(Error::SequoiaIo)?; + message.finalize().map_err(Error::Sequoia)?; - let mut decryptor = - DecryptorBuilder::from_bytes(&packets)?.with_policy(policy, None, decryptor)?; + let mut decryptor = DecryptorBuilder::from_bytes(&packets) + .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) -> Result> { if path.is_file() { let mut vec = vec![]; - for cert in CertParser::from_file(path)? { - vec.push(cert?); + for cert in CertParser::from_file(path).map_err(Error::Sequoia)? { + 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 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::>>()?; + let certs = cert_parser + .collect::>>() + .map_err(Error::Sequoia)?; keyring.set_root_cert(root_cert.clone()); manager.set_root_cert(root_cert); let mut messages: HashMap = @@ -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::>(); 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::, &str>>() - .map_err(|e| WrappedError(e.to_string()))?; - let secret = Sharks(threshold).recover(&shares)?; + .map_err(|e| Error::Share(e.to_string()))?; + 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) { - return Err(WrappedError(format!( - "Derived {} != expected {}", - derived_cert.fingerprint(), - keyring - .root_cert() - .expect("cert was previously set") - .fingerprint() - )) - .into()); + let derived_fp = derived_cert.fingerprint(); + let expected_fp = keyring + .root_cert() + .expect("cert was previously set") + .fingerprint(); + if derived_fp != expected_fp { + 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, 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::>(); 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::>(); let mut message_output = vec![]; @@ -357,28 +397,34 @@ pub fn split(threshold: u8, certs: Vec, secret: &[u8], output: impl Write) .iter() .map(|k| Recipient::new(KeyID::wildcard(), k.key())), ) - .build()?; - let message = Signer::new(message, signing_key.clone()).build()?; - let mut message = LiteralWriter::new(message).build()?; - message.write_all(share)?; - message.finalize()?; + .build() + .map_err(Error::Sequoia)?; + let message = Signer::new(message, signing_key.clone()) + .build() + .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, secret: &[u8], output: impl Write) let valid_certs = total_recipients .iter() .map(|c| c.with_policy(&policy, None)) - .collect::>>()?; + .collect::>>() + .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, 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 mut message = LiteralWriter::new(message).build()?; - message.write_all(&pp)?; - message.finalize()?; - writer.write_all(&message_output)?; + let message = Encryptor2::for_recipients(message, total_recipients) + .build() + .map_err(Error::Sequoia)?; + let mut message = LiteralWriter::new(message) + .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(()) } diff --git a/keyfork-shard/src/openpgp/keyring.rs b/keyfork-shard/src/openpgp/keyring.rs index 21dc39a..7889129 100644 --- a/keyfork-shard/src/openpgp/keyring.rs +++ b/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)] -pub enum KeyringFailure { +#[derive(thiserror::Error, Debug)] +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 { - 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 type Result = std::result::Result; pub struct Keyring { full_certs: Vec, @@ -38,7 +29,7 @@ pub struct Keyring { } impl Keyring { - pub fn new(certs: impl AsRef<[Cert]>) -> Result> { + pub fn new(certs: impl AsRef<[Cert]>) -> Result { 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()) } } diff --git a/keyfork-shard/src/openpgp/smartcard.rs b/keyfork-shard/src/openpgp/smartcard.rs index 8565a3e..1673db9 100644 --- a/keyfork-shard/src/openpgp/smartcard.rs +++ b/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)] -pub enum SmartcardFailure { +#[derive(thiserror::Error, Debug)] +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 { - 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 {} +pub type Result = std::result::Result; fn format_name(input: impl AsRef) -> String { let mut n = input @@ -49,7 +52,7 @@ pub struct SmartcardManager { } impl SmartcardManager { - pub fn new() -> Result> { + pub fn new() -> Result { Ok(Self { current_card: None, root: None, @@ -65,20 +68,20 @@ impl SmartcardManager { } /// Load any backend. - pub fn load_any_card(&mut self) -> Result> { + pub fn load_any_card(&mut self) -> Result { PcscBackend::cards(None)? .next() .transpose()? - .ok_or(SmartcardFailure::SmartCardNotFound.into()) + .ok_or(SmartcardError::SmartCardNotFound) .and_then( - |backend| -> Result> { + |backend| { let mut card = Card::::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, - ) -> Result, Box> { + ) -> Result> { // 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()) } }