Refactors HID connections to support busy response

libraries/opensk/src/api/connection.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use crate::ctap::status_code::{Ctap2StatusCode, CtapResult};
 use core::convert::TryFrom;
 
 #[derive(Clone, Copy, Debug, PartialEq, Eq)]
@@ -22,31 +23,26 @@ pub enum UsbEndpoint {
 }
 
 impl TryFrom<usize> for UsbEndpoint {
-    type Error = SendOrRecvError;
+    type Error = Ctap2StatusCode;
 
-    fn try_from(endpoint_num: usize) -> Result<Self, SendOrRecvError> {
+    fn try_from(endpoint_num: usize) -> CtapResult<Self> {
         match endpoint_num {
             1 => Ok(UsbEndpoint::MainHid),
             #[cfg(feature = "vendor_hid")]
             2 => Ok(UsbEndpoint::VendorHid),
-            _ => Err(SendOrRecvError),
+            _ => Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE),
         }
     }
 }
 
-pub enum SendOrRecvStatus {
+pub enum RecvStatus {
     Timeout,
-    Sent,
     Received(UsbEndpoint),
 }
 
-#[derive(Debug, PartialEq, Eq)]
-pub struct SendOrRecvError;
-
-pub type SendOrRecvResult = Result<SendOrRecvStatus, SendOrRecvError>;
-
 pub trait HidConnection {
-    fn send_and_maybe_recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> SendOrRecvResult;
+    fn send(&mut self, buf: &[u8; 64], endpoint: UsbEndpoint) -> CtapResult<()>;
+    fn recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> CtapResult<RecvStatus>;
 }
 
 #[cfg(test)]
@@ -58,6 +54,9 @@ mod test {
         assert_eq!(UsbEndpoint::try_from(1), Ok(UsbEndpoint::MainHid));
         #[cfg(feature = "vendor_hid")]
         assert_eq!(UsbEndpoint::try_from(2), Ok(UsbEndpoint::VendorHid));
-        assert_eq!(UsbEndpoint::try_from(3), Err(SendOrRecvError));
+        assert_eq!(
+            UsbEndpoint::try_from(3),
+            Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE)
+        );
     }
 }

libraries/opensk/src/api/user_presence.rs

@@ -12,6 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use crate::api::connection::RecvStatus;
+use crate::ctap::status_code::CtapResult;
+
 #[derive(Debug)]
 pub enum UserPresenceError {
     /// User explicitly declined user presence check.
@@ -20,12 +23,11 @@ pub enum UserPresenceError {
     Canceled,
     /// User presence check timed out.
     Timeout,
-    /// Unexpected (e.g., hardware) failures
-    Fail,
 }
-
 pub type UserPresenceResult = Result<(), UserPresenceError>;
 
+pub type UserPresenceWaitResult = CtapResult<(UserPresenceResult, RecvStatus)>;
+
 pub trait UserPresence {
     /// Initializes for a user presence check.
     ///
@@ -35,7 +37,13 @@ pub trait UserPresence {
     /// Waits until user presence is confirmed, rejected, or the given timeout expires.
     ///
     /// Must be called between calls to [`Self::check_init`] and [`Self::check_complete`].
-    fn wait_with_timeout(&mut self, timeout_ms: usize) -> UserPresenceResult;
+    /// The function may write to the packet buffer, if it receives one during the wait.
+    /// If it does, the returned option contains the endpoint it received the data from.
+    fn wait_with_timeout(
+        &mut self,
+        packet: &mut [u8; 64],
+        timeout_ms: usize,
+    ) -> UserPresenceWaitResult;
 
     /// Finalizes a user presence check.
     ///

libraries/opensk/src/ctap/hid/mod.rs

@@ -440,6 +440,11 @@ impl<E: Env> CtapHid<E> {
         })
     }
 
+    /// Generates the HID response packets for a busy error.
+    pub fn busy_error(cid: ChannelID) -> HidPacketIterator {
+        Self::split_message(Self::error_message(cid, CtapHidError::ChannelBusy))
+    }
+
     #[cfg(test)]
     pub fn new_initialized() -> (Self, ChannelID) {
         (
@@ -633,6 +638,25 @@ mod test {
         }
     }
 
+    #[test]
+    fn test_busy_error() {
+        let cid = [0x12, 0x34, 0x56, 0x78];
+        let mut response = CtapHid::<TestEnv>::busy_error(cid);
+        let mut expected_packet = [0x00; 64];
+        expected_packet[..8].copy_from_slice(&[
+            0x12,
+            0x34,
+            0x56,
+            0x78,
+            0xBF,
+            0x00,
+            0x01,
+            CtapHidError::ChannelBusy as u8,
+        ]);
+        assert_eq!(response.next(), Some(expected_packet));
+        assert_eq!(response.next(), None);
+    }
+
     #[test]
     fn test_process_single_packet() {
         let cid = [0x12, 0x34, 0x56, 0x78];

libraries/opensk/src/ctap/mod.rs

@@ -50,7 +50,9 @@ use self::data_formats::{
     PublicKeyCredentialDescriptor, PublicKeyCredentialParameter, PublicKeyCredentialSource,
     PublicKeyCredentialType, PublicKeyCredentialUserEntity, SignatureAlgorithm,
 };
-use self::hid::{ChannelID, CtapHid, CtapHidCommand, KeepaliveStatus, ProcessedPacket};
+use self::hid::{
+    ChannelID, CtapHid, CtapHidCommand, HidPacketIterator, KeepaliveStatus, ProcessedPacket,
+};
 use self::large_blobs::LargeBlobState;
 use self::response::{
     AuthenticatorGetAssertionResponse, AuthenticatorGetInfoResponse,
@@ -61,7 +63,7 @@ use self::status_code::{Ctap2StatusCode, CtapResult};
 #[cfg(feature = "with_ctap1")]
 use self::u2f_up::U2fUserPresenceState;
 use crate::api::clock::Clock;
-use crate::api::connection::{HidConnection, SendOrRecvStatus, UsbEndpoint};
+use crate::api::connection::{HidConnection, RecvStatus, UsbEndpoint};
 use crate::api::crypto::ecdsa::{SecretKey as _, Signature};
 use crate::api::crypto::hkdf256::Hkdf256;
 use crate::api::crypto::sha256::Sha256;
@@ -149,11 +151,11 @@ pub enum Transport {
 }
 
 impl Transport {
-    pub fn hid_connection<E: Env>(self, env: &mut E) -> &mut E::HidConnection {
+    pub fn usb_endpoint(self) -> UsbEndpoint {
         match self {
-            Transport::MainHid => env.main_hid_connection(),
+            Transport::MainHid => UsbEndpoint::MainHid,
             #[cfg(feature = "vendor_hid")]
-            Transport::VendorHid => env.vendor_hid_connection(),
+            Transport::VendorHid => UsbEndpoint::VendorHid,
         }
     }
 }
@@ -255,124 +257,91 @@ fn truncate_to_char_boundary(s: &str, mut max: usize) -> &str {
     }
 }
 
-// Sends keepalive packet during user presence checking. If user agent replies with CANCEL response,
-// returns Err(UserPresenceError::Canceled).
-fn send_keepalive_up_needed<E: Env>(
+/// Send non-critical packets using fire-and-forget.
+fn send_packets<E: Env>(
     env: &mut E,
-    channel: Channel,
-    timeout_ms: usize,
-) -> Result<(), UserPresenceError> {
+    endpoint: UsbEndpoint,
+    packets: HidPacketIterator,
+) -> CtapResult<()> {
+    for pkt in packets {
+        env.hid_connection().send(&pkt, endpoint)?;
+    }
+    Ok(())
+}
+
+fn is_cancel(packet: &ProcessedPacket) -> bool {
+    match packet {
+        ProcessedPacket::InitPacket { cmd, .. } => *cmd == CtapHidCommand::Cancel as u8,
+        ProcessedPacket::ContinuationPacket { .. } => false,
+    }
+}
+
+fn wait_and_respond_busy<E: Env>(env: &mut E, channel: Channel) -> CtapResult<()> {
     let (cid, transport) = match channel {
         Channel::MainHid(cid) => (cid, Transport::MainHid),
         #[cfg(feature = "vendor_hid")]
         Channel::VendorHid(cid) => (cid, Transport::VendorHid),
     };
-    let keepalive_msg = CtapHid::<E>::keepalive(cid, KeepaliveStatus::UpNeeded);
-    for mut pkt in keepalive_msg {
-        let ctap_hid_connection = transport.hid_connection(env);
-        match ctap_hid_connection.send_and_maybe_recv(&mut pkt, timeout_ms) {
-            Ok(SendOrRecvStatus::Timeout) => {
-                debug_ctap!(env, "Sending a KEEPALIVE packet timed out");
-                // The client is likely unresponsive, but let's retry.
-            }
-            Err(_) => panic!("Error sending KEEPALIVE packet"),
-            Ok(SendOrRecvStatus::Sent) => {
-                debug_ctap!(env, "Sent KEEPALIVE packet");
-            }
-            Ok(SendOrRecvStatus::Received(endpoint)) => {
-                let rx_transport = match endpoint {
-                    UsbEndpoint::MainHid => Transport::MainHid,
-                    #[cfg(feature = "vendor_hid")]
-                    UsbEndpoint::VendorHid => Transport::VendorHid,
-                };
-                if rx_transport != transport {
-                    debug_ctap!(
-                        env,
-                        "Received a packet on transport {:?} while sending a KEEPALIVE packet on transport {:?}",
-                         rx_transport, transport
-                    );
-                    // Ignore this packet.
-                    // TODO(liamjm): Support receiving packets on both interfaces.
-                    continue;
-                }
-
-                // We only parse one packet, because we only care about CANCEL.
-                let (received_cid, processed_packet) = CtapHid::<E>::process_single_packet(&pkt);
-                if received_cid != cid {
-                    debug_ctap!(
-                        env,
-                        "Received a packet on channel ID {:?} while sending a KEEPALIVE packet",
-                        received_cid,
-                    );
-                    return Ok(());
-                }
-                match processed_packet {
-                    ProcessedPacket::InitPacket { cmd, .. } => {
-                        if cmd == CtapHidCommand::Cancel as u8 {
-                            // We ignore the payload, we can't answer with an error code anyway.
-                            debug_ctap!(env, "User presence check cancelled");
-                            return Err(UserPresenceError::Canceled);
-                        } else {
-                            debug_ctap!(
-                                env,
-                                "Discarded packet with command {} received while sending a KEEPALIVE packet",
-                                cmd,
-                            );
-                        }
-                    }
-                    ProcessedPacket::ContinuationPacket { .. } => {
-                        debug_ctap!(
-                            env,
-                            "Discarded continuation packet received while sending a KEEPALIVE packet",
-                        );
-                    }
-                }
+    let endpoint = transport.usb_endpoint();
+
+    let mut packet = [0; 64];
+    let (up_status, recv_status) = env
+        .user_presence()
+        .wait_with_timeout(&mut packet, KEEPALIVE_DELAY_MS)?;
+    match recv_status {
+        RecvStatus::Timeout => (),
+        RecvStatus::Received(rx_endpoint) => {
+            let (received_cid, processed_packet) = CtapHid::<E>::process_single_packet(&packet);
+            if rx_endpoint != endpoint || received_cid != cid {
+                debug_ctap!(
+                    env,
+                    "Received an unrelated packet on endpoint {:?} while sending a KEEPALIVE packet",
+                     rx_endpoint,
+                );
+                let busy_error = CtapHid::<E>::busy_error(received_cid);
+                // Don't send errors from other channels on the active channel.
+                let _ = send_packets(env, rx_endpoint, busy_error);
+            } else if is_cancel(&processed_packet) {
+                // Ignored, CANCEL specification says: "the authenticator MUST NOT reply"
+                debug_ctap!(env, "User presence check cancelled");
+                return Err(UserPresenceError::Canceled.into());
+            } else {
+                // Ignored. A client shouldn't try to talk to us on this channel yet.
+                debug_ctap!(env, "Discarded packet while checking user presence.");
             }
         }
     }
-    Ok(())
+    if matches!(up_status, Err(UserPresenceError::Timeout)) {
+        let keepalive_msg = CtapHid::<E>::keepalive(cid, KeepaliveStatus::UpNeeded);
+        send_packets(env, endpoint, keepalive_msg)?;
+    }
+    up_status.map_err(|e| e.into())
 }
 
 /// Blocks for user presence.
 ///
 /// Returns an error in case of timeout, user declining presence request, or keepalive error.
 pub fn check_user_presence<E: Env>(env: &mut E, channel: Channel) -> CtapResult<()> {
-    env.user_presence().check_init();
+    const TIMEOUT_ERROR: Ctap2StatusCode = Ctap2StatusCode::CTAP2_ERR_USER_ACTION_TIMEOUT;
 
-    // The timeout is N times the keepalive delay.
-    const TIMEOUT_ITERATIONS: usize = TOUCH_TIMEOUT_MS / KEEPALIVE_DELAY_MS;
-
-    // All fallible functions are called without '?' operator to always reach
-    // check_complete(...) cleanup function.
-
-    let mut result = Err(UserPresenceError::Timeout);
-    for i in 0..=TIMEOUT_ITERATIONS {
-        // First presence check is made without timeout. That way Env implementation may return
-        // user presence check result immediately to client, without sending any keepalive packets.
-        result = env
-            .user_presence()
-            .wait_with_timeout(if i == 0 { 0 } else { KEEPALIVE_DELAY_MS });
-        if !matches!(result, Err(UserPresenceError::Timeout)) {
-            break;
-        }
-        // TODO: this may take arbitrary time. Next wait's delay should be adjusted
-        // accordingly, so that all wait_with_timeout invocations are separated by
-        // equal time intervals. That way token indicators, such as LEDs, will blink
-        // with a consistent pattern.
-        let keepalive_result = send_keepalive_up_needed(env, channel, KEEPALIVE_DELAY_MS);
-        if keepalive_result.is_err() {
-            debug_ctap!(
-                env,
-                "Sending keepalive failed with error {:?}",
-                keepalive_result.as_ref().unwrap_err()
-            );
-            result = keepalive_result;
-            break;
+    env.user_presence().check_init();
+    let loop_timer = env.clock().make_timer(TOUCH_TIMEOUT_MS);
+
+    // We don't use the '?' operator to always reach check_complete(...).
+    let mut result = Err(TIMEOUT_ERROR);
+    while !env.clock().is_elapsed(&loop_timer) {
+        match wait_and_respond_busy(env, channel) {
+            Err(TIMEOUT_ERROR) => (),
+            r => {
+                result = r;
+                // We want to break on Ok(()) too, indicating touch.
+                break;
+            }
         }
     }
 
     env.user_presence().check_complete();
-    result.map_err(|e| e.into())
+    result
 }
 
 /// Holds data necessary to sign an assertion for a credential.
@@ -1463,7 +1432,6 @@ mod test {
     use crate::api::crypto::ecdh::SecretKey as _;
     use crate::api::customization;
     use crate::api::key_store::CBOR_CREDENTIAL_ID_SIZE;
-    use crate::api::user_presence::UserPresenceResult;
     use crate::ctap::command::AuthenticatorLargeBlobsParameters;
     use crate::env::test::TestEnv;
     use crate::env::EcdhSk;
@@ -3381,13 +3349,12 @@ mod test {
 
     #[test]
     fn test_check_user_presence_timeout() {
-        // This will always return timeout.
-        fn user_presence_timeout() -> UserPresenceResult {
-            Err(UserPresenceError::Timeout)
-        }
-
         let mut env = TestEnv::default();
-        env.user_presence().set(user_presence_timeout);
+        let clock = env.clock().clone();
+        env.user_presence().set(move || {
+            clock.advance(100);
+            Err(UserPresenceError::Timeout)
+        });
         let response = check_user_presence(&mut env, DUMMY_CHANNEL);
         assert!(matches!(
             response,