From fa61d31f3f52ee0af14829baee0752ce9aa65b2f Mon Sep 17 00:00:00 2001 From: ryan Date: Sun, 5 Nov 2023 16:21:54 -0600 Subject: [PATCH] keyfork-shard: further improve error handling, add multiline prompt and PIN retry detection --- keyfork-shard/src/openpgp.rs | 4 +- keyfork-shard/src/openpgp/keyring.rs | 36 ++++--- keyfork-shard/src/openpgp/smartcard.rs | 137 ++++++++++++++++--------- 3 files changed, 115 insertions(+), 62 deletions(-) diff --git a/keyfork-shard/src/openpgp.rs b/keyfork-shard/src/openpgp.rs index eda5d19..3bd29c4 100644 --- a/keyfork-shard/src/openpgp.rs +++ b/keyfork-shard/src/openpgp.rs @@ -52,10 +52,10 @@ pub enum Error { SequoiaIo(#[source] std::io::Error), #[error("Keyring error: {0}")] - Keyring(#[from] keyring::KeyringError), + Keyring(#[from] keyring::Error), #[error("Smartcard error: {0}")] - Smartcard(#[from] smartcard::SmartcardError), + Smartcard(#[from] smartcard::Error), #[error("IO error: {0}")] Io(#[source] std::io::Error), diff --git a/keyfork-shard/src/openpgp/keyring.rs b/keyfork-shard/src/openpgp/keyring.rs index 7889129..d849420 100644 --- a/keyfork-shard/src/openpgp/keyring.rs +++ b/keyfork-shard/src/openpgp/keyring.rs @@ -9,18 +9,20 @@ use super::openpgp::{ KeyHandle, KeyID, }; -use crate::prompt_manager::{PromptManager, PinentryError}; +use crate::prompt_manager::{PinentryError, PromptManager}; + +use anyhow::Context; #[derive(thiserror::Error, Debug)] -pub enum KeyringError { +pub enum Error { #[error("Secret key was not found")] SecretKeyNotFound, #[error("Prompt failed: {0}")] - Prompt(#[from] PinentryError) + Prompt(#[from] PinentryError), } -pub type Result = std::result::Result; +pub type Result = std::result::Result; pub struct Keyring { full_certs: Vec, @@ -89,8 +91,9 @@ impl VerificationHelper for &mut Keyring { MessageLayer::SignatureGroup { results } => { for result in results { if let Err(e) = result { - // FIXME: anyhow leak - return Err(anyhow::anyhow!(e.to_string())); + // FIXME: anyhow leak: VerificationError impl std::error::Error + // return Err(e.context("Invalid signature")); + return Err(anyhow::anyhow!("Invalid signature: {e}")); } } } @@ -121,7 +124,9 @@ impl DecryptionHelper for &mut Keyring { .next() .map(|userid| userid.userid().name().transpose()) .flatten() - .transpose()?; + .transpose() + .ok() + .flatten(); for key in cert .keys() .with_policy(&null, None) @@ -130,7 +135,9 @@ impl DecryptionHelper for &mut Keyring { { let secret_key = key.key().clone(); let mut keypair = if secret_key.has_unencrypted_secret() { - secret_key.into_keypair()? + secret_key + .into_keypair() + .context("Has unencrypted secret")? } else { let message = if let Some(name) = name.as_ref() { format!("Decryption key for: {} ({name})", secret_key.keyid()) @@ -139,10 +146,13 @@ impl DecryptionHelper for &mut Keyring { }; let passphrase = self .pm - .prompt_passphrase("Decryption passphrase", message)?; - let key = secret_key - .decrypt_secret(&passphrase.expose_secret().as_str().into())?; - key.into_keypair()? + .prompt_passphrase("Decryption passphrase", message) + .context("Decryption passphrase")?; + secret_key + .decrypt_secret(&passphrase.expose_secret().as_str().into()) + .context("has_unencrypted_secret is false, could not decrypt secret")? + .into_keypair() + .context("just-decrypted key")? }; if pkesk .decrypt(&mut keypair, sym_algo) @@ -155,6 +165,6 @@ impl DecryptionHelper for &mut Keyring { } } - Err(KeyringError::SecretKeyNotFound.into()) + Err(Error::SecretKeyNotFound.into()) } } diff --git a/keyfork-shard/src/openpgp/smartcard.rs b/keyfork-shard/src/openpgp/smartcard.rs index 1673db9..096d9a7 100644 --- a/keyfork-shard/src/openpgp/smartcard.rs +++ b/keyfork-shard/src/openpgp/smartcard.rs @@ -11,11 +11,12 @@ use super::openpgp::{ }; use crate::prompt_manager::{PinentryError, PromptManager}; +use anyhow::Context; use card_backend_pcsc::PcscBackend; -use openpgp_card_sequoia::{state::Open, Card}; +use openpgp_card_sequoia::{state::Open, types::Error as SequoiaCardError, Card}; #[derive(thiserror::Error, Debug)] -pub enum SmartcardError { +pub enum Error { #[error("No smart card backend was stored")] SmartCardNotFound, @@ -23,16 +24,28 @@ pub enum SmartcardError { SmartCardHasNoDecrypt, #[error("Smart card backend error: {0}")] - SmartCardBackendError(#[from] card_backend::SmartcardError), + SmartCardBackend(#[from] card_backend::SmartcardError), - #[error("Smart card error: {0}")] - SmartCardEncounteredError(#[from] openpgp_card_sequoia::types::Error), + #[error("Smartcard password status unavailable: {0}")] + PwStatusBytes(SequoiaCardError), + + #[error("Could not open smart card")] + OpenSmartCard(SequoiaCardError), + + #[error("Could not initialize transaction")] + Transaction(SequoiaCardError), + + #[error("Could not load fingerprints")] + Fingerprints(SequoiaCardError), + + #[error("Invalid PIN entered too many times")] + InvalidPIN, #[error("Prompt failed: {0}")] Prompt(#[from] PinentryError), } -pub type Result = std::result::Result; +pub type Result = std::result::Result; fn format_name(input: impl AsRef) -> String { let mut n = input @@ -72,21 +85,20 @@ impl SmartcardManager { PcscBackend::cards(None)? .next() .transpose()? - .ok_or(SmartcardError::SmartCardNotFound) - .and_then( - |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(SmartcardError::SmartCardHasNoDecrypt)?; - drop(transaction); - self.current_card.replace(card); - Ok(fingerprint) - }, - ) + .ok_or(Error::SmartCardNotFound) + .and_then(|backend| { + let mut card = Card::::new(backend).map_err(Error::OpenSmartCard)?; + let transaction = card.transaction().map_err(Error::Transaction)?; + let fingerprint = transaction + .fingerprints() + .map_err(Error::Fingerprints)? + .decryption() + .map(|fp| Fingerprint::from_bytes(fp.as_bytes())) + .ok_or(Error::SmartCardHasNoDecrypt)?; + drop(transaction); + self.current_card.replace(card); + Ok(fingerprint) + }) } /// Load a backend if any [`Fingerprint`] has been matched by a currently active card. @@ -108,11 +120,12 @@ impl SmartcardManager { for backend in PcscBackend::cards(None)? { had_any_backend = true; let backend = backend?; - let mut card = Card::::new(backend)?; - let transaction = card.transaction()?; + let mut card = Card::::new(backend).map_err(Error::OpenSmartCard)?; + let transaction = card.transaction().map_err(Error::Transaction)?; let mut fingerprint = None; if let Some(fp) = transaction - .fingerprints()? + .fingerprints() + .map_err(Error::Fingerprints)? .decryption() .map(|fp| Fingerprint::from_bytes(fp.as_bytes())) { @@ -127,9 +140,8 @@ impl SmartcardManager { } } - eprintln!("No matching smartcard detected."); - self.pm - .prompt_message("Please plug in a smart card and press enter")?; + #[rustfmt::skip] + self.pm.prompt_message("Please plug in a smart card and press enter")?; } Ok(None) @@ -158,7 +170,7 @@ impl VerificationHelper for &mut SmartcardManager { for result in results { if let Err(e) = result { // FIXME: anyhow leak - return Err(anyhow::anyhow!(e.to_string())); + return Err(anyhow::anyhow!("Verification error: {}", e.to_string())); } } } @@ -181,28 +193,59 @@ impl DecryptionHelper for &mut SmartcardManager { { let mut card = self.current_card.take(); let Some(card) = card.as_mut() else { - return Err(SmartcardError::SmartCardNotFound.into()); + return Err(Error::SmartCardNotFound.into()); }; - let mut transaction = card.transaction()?; + let mut transaction = card + .transaction() + .context("Could not initialize transaction")?; let fp = transaction - .fingerprints()? + .fingerprints() + .context("Could not load fingerprints")? .decryption() - .map(|fp| Fingerprint::from_bytes(fp.as_bytes())); - let Some(fp) = fp else { - return Err(SmartcardError::SmartCardHasNoDecrypt.into()); - }; - let cardholder_name = format_name(transaction.cardholder_name()?); - let card_id = transaction.application_identifier()?.ident(); - let message = if cardholder_name.is_empty() { - format!("Unlock card {card_id}") - } else { - format!("Unlock card {card_id} ({cardholder_name})") - }; - let pin = self.pm.prompt_passphrase("Smartcard User PIN", message)?; - let mut user = transaction.to_user_card(pin.expose_secret().as_str().trim())?; - let mut decryptor = - user.decryptor(&|| eprintln!("Touch confirmation needed for decryption"))?; + .map(|fp| Fingerprint::from_bytes(fp.as_bytes())) + .ok_or(Error::SmartCardHasNoDecrypt)?; + let cardholder_name = format_name( + transaction + .cardholder_name() + .context("Could not load (optionally empty) cardholder name")?, + ); + let card_id = transaction + .application_identifier() + .context("Could not load application identifier")? + .ident(); + let pw_status = transaction + .pw_status_bytes() + .map_err(Error::PwStatusBytes)?; + let mut pin = None; + for _ in 0..pw_status.err_count_pw1() { + transaction.reload_ard()?; + let attempts = transaction + .pw_status_bytes() + .map_err(Error::PwStatusBytes)? + .err_count_pw1(); + let rpea = "Remaining PIN entry attempts"; + let message = if cardholder_name.is_empty() { + format!("Unlock card {card_id}\n\n{rpea}: {attempts}") + } else { + format!("Unlock card {card_id} ({cardholder_name})\n\n{rpea}: {attempts}") + }; + let temp_pin = self.pm.prompt_passphrase("Smartcard User PIN", message)?; + if transaction + .verify_user_pin(temp_pin.expose_secret().as_str().trim()) + .is_ok() + { + pin.replace(temp_pin); + break; + } + } + let pin = pin.ok_or(Error::InvalidPIN)?; + let mut user = transaction + .to_user_card(pin.expose_secret().as_str().trim()) + .context("Could not load user smartcard from PIN")?; + let mut decryptor = user + .decryptor(&|| eprintln!("Touch confirmation needed for decryption")) + .context("Could not decrypt using smartcard")?; for pkesk in pkesks { if pkesk .decrypt(&mut decryptor, sym_algo) @@ -213,6 +256,6 @@ impl DecryptionHelper for &mut SmartcardManager { } } - Err(SmartcardError::SmartCardNotFound.into()) + Err(Error::SmartCardNotFound.into()) } }