WIP

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"))]

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::<StatusUpdate>();
-
     let (register_tx, register_rx) = channel();
-    let (error_tx, error_rx) = channel::<FidoError>();
     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::<FidoError>));
+    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<u8, Error>) -> FidoError {
+        match result {
+            Err(Error::FidoError(e)) => e,
+            other => panic!("unexpected result: {:?}", other),
+        }
+    }
+
+    #[test]
+    fn successful_result_passes_through() {
+        let (tx, rx) = channel::<Result<u8, String>>();
+        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::<Result<u8, String>>();
+        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::<Result<u8, String>>();
+        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::<Result<u8, String>>();
+        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::<Result<u8, String>>();
+        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::<Result<u8, String>>();
+        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<Option<FidoError>>, 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<FidoError>>) -> Option<FidoError> {
+    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<T, E: Display>(
+    result_rx: Receiver<Result<T, E>>,
+    pin_error: &Mutex<Option<FidoError>>,
+    pin_error_rx: Receiver<()>,
+    pin_error_budget: Duration,
+) -> Result<T, Error> {
+    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))),
+    }
+}

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"))]

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<Vec<u8>> {
@@ -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::<StatusUpdate>();
-    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::<StatusUpdate>();
+    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<StatusUpdate>) {
+    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<Pin>, StatusUpdate) {
+        let (pin_tx, pin_rx) = channel::<Pin>();
+        (
+            pin_rx,
+            StatusUpdate::PinUvError(StatusPinUv::PinRequired(pin_tx)),
+        )
+    }
+
+    #[test]
+    fn pin_required_status_drops_the_pin_sender() {
+        let (status_tx, status_rx) = channel::<StatusUpdate>();
+        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::<StatusUpdate>();
+        let (pin_tx, pin_rx) = channel::<Pin>();
+        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::<StatusUpdate>();
+        drop(_status_tx);
+        drain_pin_statuses(status_rx);
+    }
+}