From 752f0a410c5db3c3197d0d071dbc3db12f3210fb Mon Sep 17 00:00:00 2001 From: Thanh Nguyen Date: Mon, 21 Sep 2026 20:53:51 -0400 Subject: [PATCH] WIP --- src/fido/generate/mod.rs | 11 ++ src/fido/generate/mozilla.rs | 201 +++++++++++++++++++++++++++------- src/fido/signing/mod.rs | 11 ++ src/fido/signing/mozilla.rs | 93 +++++++++++++--- src/yubikey/piv/management.rs | 32 +++++- src/yubikey/piv/mod.rs | 12 +- 6 files changed, 307 insertions(+), 53 deletions(-) diff --git a/src/fido/generate/mod.rs b/src/fido/generate/mod.rs index fbd2521..d739439 100644 --- a/src/fido/generate/mod.rs +++ b/src/fido/generate/mod.rs @@ -1,3 +1,14 @@ +//! For generating new SSH keys on FIDO devices +//! +//! # Blocking +//! +//! Key generation requires user presence, so every backend blocks until the +//! user touches the device. With the `fido-support-mozilla` backend the wait +//! is bounded by a 15 second operation timeout; with the `fido-support` +//! backend the wait is typically bounded by the device's own user-presence +//! timeout (about 30 seconds). Run generation on a worker thread if a +//! different timeout is needed. + use crate::{PrivateKey, TouchRequirement}; #[cfg(any(feature = "fido-support"))] diff --git a/src/fido/generate/mozilla.rs b/src/fido/generate/mozilla.rs index e7a21d9..4376873 100644 --- a/src/fido/generate/mozilla.rs +++ b/src/fido/generate/mozilla.rs @@ -30,10 +30,19 @@ use authenticator::{ use authenticator::{crypto::COSEAlgorithm, StatusPinUv}; use std::{ - sync::mpsc::{channel, RecvError}, + fmt::Display, + sync::{ + mpsc::{channel, Receiver, RecvError}, + Arc, Mutex, + }, thread, + time::Duration, }; +/// How long to keep waiting for a late PIN error report after the operation +/// already failed for another reason +const PIN_ERROR_BUDGET: Duration = Duration::from_millis(250); + /// Generate a new SSH key on a FIDO/U2F device pub fn generate_new_ssh_key( application: &str, @@ -51,7 +60,9 @@ pub fn generate_new_ssh_key( let mut client_data = [0u8; 32]; // Fill it with random data because we don't support taking in // challenge data at this point. - SystemRandom::new().fill(&mut client_data).unwrap(); + if let Err(e) = SystemRandom::new().fill(&mut client_data) { + return Err(Error::FidoError(FidoError::Unknown(e.to_string()))); + } // Hash the data because that is what will actually be signed let client_data_digest = digest::digest(&digest::SHA256, &client_data); @@ -83,56 +94,48 @@ pub fn generate_new_ssh_key( }; let (status_tx, status_rx) = channel::(); - let (register_tx, register_rx) = channel(); - let (error_tx, error_rx) = channel::(); let callback = StateCallback::new(Box::new(move |rv| { let _ = register_tx.send(rv); })); - if let Err(e) = manager.register(15_000, ctap_args, status_tx.clone(), callback) { + if let Err(e) = manager.register(15_000, ctap_args, status_tx, callback) { return Err(Error::FidoError(FidoError::Unknown(e.to_string()))); }; + // PIN failures are recorded in shared state rather than on a second + // blocking channel: once the result channel has yielded an error, a PIN + // status may never arrive (for example on a touch timeout), so waiting + // for one can deadlock. + let (pin_error_tx, pin_error_rx) = channel::<()>(); + let pin_error = Arc::new(Mutex::new(None::)); + let status_pin_error = Arc::clone(&pin_error); thread::spawn(move || loop { - let msg = status_rx.recv(); - match msg { - Ok(StatusUpdate::PinUvError(StatusPinUv::PinRequired(_))) => { - let _ = error_tx.send(FidoError::PinRequired); - return; - } - Ok(StatusUpdate::PinUvError(StatusPinUv::PinAuthBlocked)) => { - let _ = error_tx.send(FidoError::KeyLocked); - return; - } - Ok(StatusUpdate::PinUvError(StatusPinUv::PinBlocked)) => { - let _ = error_tx.send(FidoError::KeyBlocked); - return; + let error = match status_rx.recv() { + // Dropping the embedded PIN sender unblocks the authenticator + // crate's device thread, which is otherwise waiting for a PIN + // that will never arrive. + Ok(StatusUpdate::PinUvError(StatusPinUv::PinRequired(_sender))) => { + Some(FidoError::PinRequired) } + Ok(StatusUpdate::PinUvError(StatusPinUv::PinAuthBlocked)) => Some(FidoError::KeyLocked), + Ok(StatusUpdate::PinUvError(StatusPinUv::PinBlocked)) => Some(FidoError::KeyBlocked), Ok(StatusUpdate::PinUvError(StatusPinUv::InvalidPin(_sender, attempts))) => { - let _ = error_tx.send(FidoError::InvalidPin(attempts)); - return; + Some(FidoError::InvalidPin(attempts)) } - Ok(_) => (), - Err(RecvError) => { - return; - } - } + Ok(_) => None, + Err(RecvError) => return, + }; + let Some(error) = error else { + continue; + }; + store_pin_error(&status_pin_error, error); + let _ = pin_error_tx.send(()); + return; }); - let register_result = register_rx - .recv() - .map_err(|e| Error::FidoError(FidoError::Unknown(e.to_string())))?; - let attestation_object = match register_result { - Ok(attestation) => attestation, - Err(e) => { - if let Ok(error) = error_rx.recv() { - return Err(Error::FidoError(error)); - } else { - return Err(Error::FidoError(FidoError::Unknown(e.to_string()))); - } - } - }; + let attestation_object = + wait_for_result(register_rx, &pin_error, pin_error_rx, PIN_ERROR_BUDGET)?; let raw_auth_data = attestation_object.att_obj.auth_data.to_vec(); @@ -189,3 +192,125 @@ pub fn generate_new_ssh_key( attestation, }) } + +#[cfg(test)] +mod tests { + use super::*; + + fn fido_error(result: Result) -> FidoError { + match result { + Err(Error::FidoError(e)) => e, + other => panic!("unexpected result: {:?}", other), + } + } + + #[test] + fn successful_result_passes_through() { + let (tx, rx) = channel::>(); + let (_wake_tx, wake_rx) = channel::<()>(); + tx.send(Ok(42)).unwrap(); + let result = wait_for_result(rx, &Mutex::new(None), wake_rx, Duration::from_millis(1)); + assert_eq!(result.unwrap(), 42); + } + + #[test] + fn propagated_error_without_pin_error_is_unknown() { + let (tx, rx) = channel::>(); + let (_wake_tx, wake_rx) = channel::<()>(); + tx.send(Err("token error".to_string())).unwrap(); + let result = wait_for_result(rx, &Mutex::new(None), wake_rx, Duration::from_millis(1)); + match fido_error(result) { + FidoError::Unknown(msg) => assert!(msg.contains("token error")), + other => panic!("unexpected error: {:?}", other), + } + } + + #[test] + fn recorded_pin_error_wins_over_propagated_error() { + let (tx, rx) = channel::>(); + let (_wake_tx, wake_rx) = channel::<()>(); + tx.send(Err("token error".to_string())).unwrap(); + let pin_error = Mutex::new(Some(FidoError::PinRequired)); + let result = wait_for_result(rx, &pin_error, wake_rx, Duration::from_millis(1)); + assert!(matches!(fido_error(result), FidoError::PinRequired)); + } + + #[test] + fn recorded_pin_error_survives_recv_error() { + let (tx, rx) = channel::>(); + let (_wake_tx, wake_rx) = channel::<()>(); + drop(tx); + let pin_error = Mutex::new(Some(FidoError::KeyLocked)); + let result = wait_for_result(rx, &pin_error, wake_rx, Duration::from_millis(1)); + assert!(matches!(fido_error(result), FidoError::KeyLocked)); + } + + #[test] + fn missing_pin_error_falls_back_after_budget() { + let (tx, rx) = channel::>(); + let (_wake_tx, wake_rx) = channel::<()>(); + tx.send(Err("token error".to_string())).unwrap(); + let result = wait_for_result(rx, &Mutex::new(None), wake_rx, Duration::from_millis(10)); + match fido_error(result) { + FidoError::Unknown(msg) => assert!(msg.contains("token error")), + other => panic!("unexpected error: {:?}", other), + } + } + + #[test] + fn late_pin_error_is_detected_within_budget() { + let (tx, rx) = channel::>(); + let (wake_tx, wake_rx) = channel::<()>(); + let pin_error = Arc::new(Mutex::new(None)); + let late_pin_error = Arc::clone(&pin_error); + tx.send(Err("token error".to_string())).unwrap(); + // Record the PIN error from another thread, at a point unrelated to + // the waiter's progress. The wake signal is buffered and the final + // check re-reads the shared state, so every interleaving detects it. + let handle = thread::spawn(move || { + store_pin_error(&late_pin_error, FidoError::PinRequired); + wake_tx.send(()).unwrap(); + }); + let result = wait_for_result(rx, &pin_error, wake_rx, Duration::from_secs(2)); + handle.join().unwrap(); + assert!(matches!(fido_error(result), FidoError::PinRequired)); + } +} + +fn store_pin_error(pin_error: &Mutex>, error: FidoError) { + let mut guard = pin_error.lock().unwrap_or_else(|e| e.into_inner()); + *guard = Some(error); +} + +fn take_pin_error(pin_error: &Mutex>) -> Option { + pin_error.lock().unwrap_or_else(|e| e.into_inner()).take() +} + +/// Wait for the result of a FIDO operation, preferring a recorded PIN error +/// over the propagated authenticator error because it describes the failure +/// more precisely +fn wait_for_result( + result_rx: Receiver>, + pin_error: &Mutex>, + pin_error_rx: Receiver<()>, + pin_error_budget: Duration, +) -> Result { + let error = match result_rx.recv() { + Ok(Ok(value)) => return Ok(value), + Ok(Err(e)) => e.to_string(), + Err(e) => e.to_string(), + }; + let recorded = take_pin_error(pin_error); + if recorded.is_none() { + // PinAuthBlocked and PinBlocked are reported on the status channel + // independently of the result, so give a late report a bounded chance + // to arrive. The signal is buffered, and the final check below re-reads + // the shared state, so a report recorded at any point during the wait + // is observed. + let _ = pin_error_rx.recv_timeout(pin_error_budget); + } + match recorded.or_else(|| take_pin_error(pin_error)) { + Some(error) => Err(Error::FidoError(error)), + None => Err(Error::FidoError(FidoError::Unknown(error))), + } +} diff --git a/src/fido/signing/mod.rs b/src/fido/signing/mod.rs index 68ed05a..3ff3293 100644 --- a/src/fido/signing/mod.rs +++ b/src/fido/signing/mod.rs @@ -1,3 +1,14 @@ +//! For signing data with FIDO-backed SSH keys +//! +//! # Blocking +//! +//! Signing requires user presence, so every backend blocks until the user +//! touches the device. With the `fido-support-mozilla` backend the wait is +//! bounded by a 15 second operation timeout; with the `fido-support` backend +//! the wait is typically bounded by the device's own user-presence timeout +//! (about 30 seconds). Run signing on a worker thread if a different timeout +//! is needed. + #[cfg(any(feature = "fido-support"))] mod ctap2_hid; #[cfg(any(feature = "fido-support"))] diff --git a/src/fido/signing/mozilla.rs b/src/fido/signing/mozilla.rs index 15d417a..e14284b 100644 --- a/src/fido/signing/mozilla.rs +++ b/src/fido/signing/mozilla.rs @@ -12,7 +12,8 @@ use authenticator::{ statecallback::StateCallback, Pin, StatusUpdate, }; -use std::sync::mpsc::channel; +use std::sync::mpsc::{channel, Receiver}; +use std::thread; /// Sign data with a SK type private key pub fn sign_with_private_key(private_key: &PrivateKey, challenge: &[u8]) -> Option> { @@ -62,21 +63,26 @@ pub fn sign_with_private_key(private_key: &PrivateKey, challenge: &[u8]) -> Opti let (sign_tx, sign_rx) = channel(); let callback = StateCallback::new(Box::new(move |rv| { - sign_tx.send(rv).unwrap(); + let _ = sign_tx.send(rv); })); - let (status_tx, _status_rx) = channel::(); - if let Err(e) = manager.sign( - 15_000, - ctap_args.clone().into(), - status_tx.clone(), - callback, - ) { - panic!("Couldn't sign: {:?}", e); + // Consume PIN-related status updates and drop their embedded PIN sender. + // If the sender were left unread, the authenticator crate's device thread + // would stay blocked waiting for a PIN that will never arrive, and its + // join during transaction cleanup would deadlock this function. + let (status_tx, status_rx) = channel::(); + thread::spawn(move || drain_pin_statuses(status_rx)); + + if manager + .sign(15_000, ctap_args.clone().into(), status_tx, callback) + .is_err() + { + return None; } - let sign_result = sign_rx - .recv() - .expect("Problem receiving, unable to continue"); + let sign_result = match sign_rx.recv() { + Ok(result) => result, + Err(_) => return None, + }; let assertion = match sign_result { Ok(assertion_object) => assertion_object.assertion, @@ -94,3 +100,64 @@ pub fn sign_with_private_key(private_key: &PrivateKey, challenge: &[u8]) -> Opti format.extend_from_slice(&assertion.auth_data.counter.to_be_bytes()); Some(format) } + +/// Drain PIN-related status updates until the operation ends, dropping any +/// embedded PIN sender so the authenticator crate's device thread is never +/// left blocked waiting for a PIN that will not arrive +fn drain_pin_statuses(status_rx: Receiver) { + loop { + match status_rx.recv() { + Ok(StatusUpdate::PinUvError(_)) => return, + Ok(_) => (), + Err(_) => return, + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use authenticator::StatusPinUv; + + fn dropped_pin_sender_status() -> (Receiver, StatusUpdate) { + let (pin_tx, pin_rx) = channel::(); + ( + pin_rx, + StatusUpdate::PinUvError(StatusPinUv::PinRequired(pin_tx)), + ) + } + + #[test] + fn pin_required_status_drops_the_pin_sender() { + let (status_tx, status_rx) = channel::(); + let (pin_rx, status) = dropped_pin_sender_status(); + status_tx.send(status).unwrap(); + drain_pin_statuses(status_rx); + assert!(pin_rx + .recv_timeout(std::time::Duration::from_secs(1)) + .is_err()); + } + + #[test] + fn invalid_pin_status_drops_the_pin_sender() { + let (status_tx, status_rx) = channel::(); + let (pin_tx, pin_rx) = channel::(); + status_tx + .send(StatusUpdate::PinUvError(StatusPinUv::InvalidPin( + pin_tx, + Some(3), + ))) + .unwrap(); + drain_pin_statuses(status_rx); + assert!(pin_rx + .recv_timeout(std::time::Duration::from_secs(1)) + .is_err()); + } + + #[test] + fn drain_exits_when_the_status_channel_closes() { + let (_status_tx, status_rx) = channel::(); + drop(_status_tx); + drain_pin_statuses(status_rx); + } +} diff --git a/src/yubikey/piv/management.rs b/src/yubikey/piv/management.rs index 800820a..676015c 100644 --- a/src/yubikey/piv/management.rs +++ b/src/yubikey/piv/management.rs @@ -296,7 +296,14 @@ impl super::Yubikey { .unwrap_or(TouchRequirement::Unknown)) } - /// Generate CSR for slot + /// Generate a PKCS#10 certificate signing request for the slot, signed by + /// the key in the slot + /// + /// # Blocking + /// + /// Signing the CSR uses the slot's private key, so if the slot is + /// configured with [`TouchPolicy::Always`] or [`TouchPolicy::Once`], this + /// call blocks until the user touches the device; there is no timeout. pub fn generate_csr(&mut self, slot: &SlotId, common_name: &str) -> Result> { let mut params = rcgen::CertificateParams::new(vec![]); let cert = self.configured(&slot).map_err(|e| { @@ -363,6 +370,13 @@ impl super::Yubikey { /// Provisions the YubiKey with a new certificate generated on the device. /// Only keys that are generated this way can use the attestation functionality. /// This is a nongeneric version to generate a p384 key + /// + /// # Blocking + /// + /// Generating the key itself never requires a touch, but creating the + /// self-signed certificate does. If `touch_policy` is + /// [`TouchPolicy::Always`] or [`TouchPolicy::Once`], this call blocks + /// until the user touches the device; there is no timeout. pub fn provision_p384( &mut self, slot: &SlotId, @@ -376,6 +390,13 @@ impl super::Yubikey { /// Provisions the YubiKey with a new certificate generated on the device. /// Only keys that are generated this way can use the attestation functionality. /// This is a nongeneric version to generate a p256 key + /// + /// # Blocking + /// + /// Generating the key itself never requires a touch, but creating the + /// self-signed certificate does. If `touch_policy` is + /// [`TouchPolicy::Always`] or [`TouchPolicy::Once`], this call blocks + /// until the user touches the device; there is no timeout. pub fn provision_p256( &mut self, slot: &SlotId, @@ -390,6 +411,15 @@ impl super::Yubikey { /// /// If the requested algorithm doesn't match the key in the slot (or the slot /// is empty) this will error. + /// + /// # Blocking + /// + /// If the slot is configured with [`TouchPolicy::Always`] or + /// [`TouchPolicy::Once`], this call blocks until the user touches the + /// device; there is no timeout and no way to cancel the pending card + /// transaction. Use [`Yubikey::touch_requirement`] to check whether a + /// touch is expected, and run this call on a worker thread if a timeout + /// is needed. pub fn sign_data(&mut self, data: &[u8], alg: AlgorithmId, slot: &SlotId) -> Result> { let cert = self.configured(&slot).map_err(|e| { Error::InternalYubiKeyError(format!("failed to read slot for signing: {}", e)) diff --git a/src/yubikey/piv/mod.rs b/src/yubikey/piv/mod.rs index 9a52d70..4684d6e 100644 --- a/src/yubikey/piv/mod.rs +++ b/src/yubikey/piv/mod.rs @@ -3,6 +3,16 @@ pub mod keytype; /// Contains all the functions used for creating new keys, unlocking, and /// managing the yubikey +/// +/// # Blocking +/// +/// Operations that use a slot's private key (signing, provisioning with a +/// self-signed certificate, CSR generation) block until the user touches the +/// device when the slot is configured with [`TouchPolicy::Always`] or +/// [`TouchPolicy::Once`]. The underlying PC/SC card transaction has no +/// timeout and cannot be cancelled, so consumers needing timeouts should +/// check [`Yubikey::touch_requirement`] first and run the operation on their +/// own worker thread. pub mod management; /// The SSH submodule contains functions relevant to SSH uses that are backed /// by the Yubikey. This includes things like signing and SSH public key @@ -49,8 +59,8 @@ type Result = std::result::Result; pub use crate::ssh::TouchRequirement; pub use keytype::{NistP256, NistP384}; -pub use yubikey::piv::{AlgorithmId, RetiredSlotId, SlotId}; pub use management::ManagementKeyAlgorithm; +pub use yubikey::piv::{AlgorithmId, RetiredSlotId, SlotId}; pub use yubikey::{PinPolicy, TouchPolicy}; /// Structure to wrap a yubikey and abstract actions