From 214b0048ddc18f9a75194274b012b9c541d8d6c5 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Thu, 29 Aug 2024 17:39:05 +0200 Subject: [PATCH 1/4] Refactors HID connections to support busy response The main intended difference is that OpenSK now responds with ERR_CHANNEL_BUSY when it receives a packet while waiting for touch in in a CTAP2 command. To support this more elegantly, we changed `HidConnection` to have a `send` and `recv` instead of `send_and_maybe_recv` for a cleaner API. This also resolves an older TODO to respond to incoming channels on the other USB endpoint. It doesn't process the incoming traffic correctly, but unblock the host at least, and tells it to come back later. We still only allow one active channel at the same time. We now don't need the `TRANSMIT_AND_RECEIVE` syscall anymore. I didn't remove it from the Tock pack for simplicity, but cleaned up libtock-drivers. The Env API changes from having one connection per endpoint, to having one send function that takes an endpoint, and a receive function that receives on all endpoints at the same time. The `HidConnection` already received on all endpoints before, which was inconsistent with the existance of, e.g., `VendorHidConnection` being able to receive on the main endpoint. Since we now have a cleaner send and receive API, we use this in `src/main.rs` and simplify it a bit, while making it more consistent with other calls to the HID API. I found an additional inaccuracy in our implementation while refactoring: We want to send keepalives every 100 ms. To do so, we first wait for a button callback for 100 ms. Then we send the keepalive and check if a cancel packet appeared. What should have happened instead is that we listen for HID packets and button presses at the same time during these 100 ms. If nothing happens that stops the loop, we send the keepalive. The old implementation would, in some implementations, wait 200 ms for each keepalive: First 100 for touch, then 100 for `send_and_maybe_receive`. The new implementation can loop faster in case there is a lot of unrelated HID traffic. To make the touch timeout last 30 seconds, we introduce an extra timer and loop until time is up. This might still make us blink the LEDs too fast, but that is already the case in the main loop, and generally would need a bigger refactoring. The only simple workaround to make the LEDs slightly more precise is to wait for touch and packets until it is time to flip the LEDs, and if we return too early within some thresholds, wait again. If we care enough, we can fix that in a later PR. Not sure how to make the test work that I removed in ctap/mod.rs. I'd need to advance the time while the loop is running. Setting a user presence function that advances time seems hard with the Rust borrowing rules. --- Cargo.lock | 1 + libraries/opensk/src/api/connection.rs | 3 +- libraries/opensk/src/api/user_presence.rs | 8 +- libraries/opensk/src/ctap/hid/mod.rs | 24 +++ libraries/opensk/src/ctap/mod.rs | 180 ++++++----------- libraries/opensk/src/env/mod.rs | 8 +- libraries/opensk/src/env/test/mod.rs | 20 +- src/env/tock/mod.rs | 182 ++++++------------ src/main.rs | 65 +++---- third_party/libtock-drivers/Cargo.toml | 1 + .../libtock-drivers/src/usb_ctap_hid.rs | 149 +++++--------- 11 files changed, 247 insertions(+), 394 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 08ff0630..0986339f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -412,6 +412,7 @@ name = "libtock_drivers" version = "0.1.0" dependencies = [ "libtock_alarm", + "libtock_buttons", "libtock_console", "libtock_platform", ] diff --git a/libraries/opensk/src/api/connection.rs b/libraries/opensk/src/api/connection.rs index b3a577ce..f66f4ac2 100644 --- a/libraries/opensk/src/api/connection.rs +++ b/libraries/opensk/src/api/connection.rs @@ -46,7 +46,8 @@ pub struct SendOrRecvError; pub type SendOrRecvResult = Result; 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) -> SendOrRecvResult; + fn recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> SendOrRecvResult; } #[cfg(test)] diff --git a/libraries/opensk/src/api/user_presence.rs b/libraries/opensk/src/api/user_presence.rs index 6b0dbbe6..7f7f24c7 100644 --- a/libraries/opensk/src/api/user_presence.rs +++ b/libraries/opensk/src/api/user_presence.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use crate::api::connection::UsbEndpoint; + #[derive(Debug)] pub enum UserPresenceError { /// User explicitly declined user presence check. @@ -24,7 +26,10 @@ pub enum UserPresenceError { Fail, } -pub type UserPresenceResult = Result<(), UserPresenceError>; +pub type UserPresenceResult = ( + Result<(), UserPresenceError>, + Option<([u8; 64], UsbEndpoint)>, +); pub trait UserPresence { /// Initializes for a user presence check. @@ -35,6 +40,7 @@ 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`]. + /// Additionally returns a packet if one was received during the wait. fn wait_with_timeout(&mut self, timeout_ms: usize) -> UserPresenceResult; /// Finalizes a user presence check. diff --git a/libraries/opensk/src/ctap/hid/mod.rs b/libraries/opensk/src/ctap/hid/mod.rs index 955b949f..ac48f32e 100644 --- a/libraries/opensk/src/ctap/hid/mod.rs +++ b/libraries/opensk/src/ctap/hid/mod.rs @@ -440,6 +440,11 @@ impl CtapHid { }) } + /// Generates a 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::::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]; diff --git a/libraries/opensk/src/ctap/mod.rs b/libraries/opensk/src/ctap/mod.rs index 9cc430ec..75e8f2d3 100644 --- a/libraries/opensk/src/ctap/mod.rs +++ b/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, @@ -149,11 +151,11 @@ pub enum Transport { } impl Transport { - pub fn hid_connection(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,82 +257,17 @@ 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( - env: &mut E, - channel: Channel, - timeout_ms: usize, -) -> Result<(), UserPresenceError> { - 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::::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) { +/// Send non-critical packets using fire-and-forget. +fn send_packets(env: &mut E, endpoint: UsbEndpoint, packets: HidPacketIterator) { + for pkt in packets { + match env.hid_connection().send(&pkt, endpoint) { 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::::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", - ); - } - } + debug_ctap!(env, "Timeout sending packet"); } + Ok(SendOrRecvStatus::Sent) => (), + _ => panic!("Error sending packet"), } } - Ok(()) } /// Blocks for user presence. @@ -338,37 +275,62 @@ fn send_keepalive_up_needed( /// Returns an error in case of timeout, user declining presence request, or keepalive error. pub fn check_user_presence(env: &mut E, channel: Channel) -> CtapResult<()> { env.user_presence().check_init(); + let loop_timer = env.clock().make_timer(TOUCH_TIMEOUT_MS); - // The timeout is N times the keepalive delay. - const TIMEOUT_ITERATIONS: usize = TOUCH_TIMEOUT_MS / KEEPALIVE_DELAY_MS; + let (cid, transport) = match channel { + Channel::MainHid(cid) => (cid, Transport::MainHid), + #[cfg(feature = "vendor_hid")] + Channel::VendorHid(cid) => (cid, Transport::VendorHid), + }; + let endpoint = transport.usb_endpoint(); // 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; + while !env.clock().is_elapsed(&loop_timer) { + let (status, data) = env.user_presence().wait_with_timeout(KEEPALIVE_DELAY_MS); + if let Some((packet, rx_endpoint)) = data { + let (received_cid, processed_packet) = CtapHid::::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::::busy_error(received_cid); + send_packets(env, rx_endpoint, busy_error); + } + match processed_packet { + ProcessedPacket::InitPacket { cmd, .. } => { + if cmd == CtapHidCommand::Cancel as u8 { + // 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 with command {} received while sending a KEEPALIVE packet", + cmd, + ); + } + } + // Ignored. We likely ignore the init packet before as well. + ProcessedPacket::ContinuationPacket { .. } => { + debug_ctap!( + env, + "Discarded continuation packet received while sending a KEEPALIVE packet", + ); + } + } } - // 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; + if !matches!(status, Err(UserPresenceError::Timeout)) { + result = status; break; } + let keepalive_msg = CtapHid::::keepalive(cid, KeepaliveStatus::UpNeeded); + send_packets(env, endpoint, keepalive_msg); } env.user_presence().check_complete(); @@ -2290,7 +2252,8 @@ mod test { #[test] fn test_process_make_credential_cancelled() { let mut env = TestEnv::default(); - env.user_presence().set(|| Err(UserPresenceError::Canceled)); + env.user_presence() + .set(|| (Err(UserPresenceError::Canceled), None)); let mut ctap_state = CtapState::::new(&mut env); let make_credential_params = create_minimal_make_credential_parameters(); @@ -3170,7 +3133,8 @@ mod test { #[test] fn test_process_reset_cancelled() { let mut env = TestEnv::default(); - env.user_presence().set(|| Err(UserPresenceError::Canceled)); + env.user_presence() + .set(|| (Err(UserPresenceError::Canceled), None)); let mut ctap_state = CtapState::::new(&mut env); let reset_reponse = ctap_state.process_reset(&mut env, DUMMY_CHANNEL); @@ -3396,22 +3360,6 @@ mod test { assert!(matches!(response, Ok(_))); } - #[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 response = check_user_presence(&mut env, DUMMY_CHANNEL); - assert!(matches!( - response, - Err(Ctap2StatusCode::CTAP2_ERR_USER_ACTION_TIMEOUT) - )); - } - #[test] fn test_channel_interleaving() { let mut env = TestEnv::default(); diff --git a/libraries/opensk/src/env/mod.rs b/libraries/opensk/src/env/mod.rs index 427450fa..18edfd4a 100644 --- a/libraries/opensk/src/env/mod.rs +++ b/libraries/opensk/src/env/mod.rs @@ -66,12 +66,8 @@ pub trait Env { fn customization(&self) -> &Self::Customization; - /// I/O connection for sending packets implementing CTAP HID protocol. - fn main_hid_connection(&mut self) -> &mut Self::HidConnection; - - /// I/O connection for sending packets implementing vendor extensions to CTAP HID protocol. - #[cfg(feature = "vendor_hid")] - fn vendor_hid_connection(&mut self) -> &mut Self::HidConnection; + /// I/O connection for sending HID packets. + fn hid_connection(&mut self) -> &mut Self::HidConnection; /// Indicates that the last power cycle was not caused by user action. fn boots_after_soft_reset(&self) -> bool; diff --git a/libraries/opensk/src/env/test/mod.rs b/libraries/opensk/src/env/test/mod.rs index 2d9f6119..836720b4 100644 --- a/libraries/opensk/src/env/test/mod.rs +++ b/libraries/opensk/src/env/test/mod.rs @@ -13,7 +13,7 @@ // limitations under the License. use crate::api::clock::Clock; -use crate::api::connection::{HidConnection, SendOrRecvResult, SendOrRecvStatus}; +use crate::api::connection::{HidConnection, SendOrRecvResult, SendOrRecvStatus, UsbEndpoint}; use crate::api::crypto::software_crypto::SoftwareCrypto; use crate::api::customization::DEFAULT_CUSTOMIZATION; use crate::api::key_store; @@ -128,17 +128,22 @@ impl Persist for TestEnv { } impl HidConnection for TestEnv { - fn send_and_maybe_recv(&mut self, _buf: &mut [u8; 64], _timeout_ms: usize) -> SendOrRecvResult { - // TODO: Implement I/O from canned requests/responses for integration testing. + // TODO: Implement I/O from canned requests/responses for integration testing. + + fn send(&mut self, _buf: &[u8; 64], _endpoint: UsbEndpoint) -> SendOrRecvResult { Ok(SendOrRecvStatus::Sent) } + + fn recv(&mut self, _buf: &mut [u8; 64], _timeout_ms: usize) -> SendOrRecvResult { + Ok(SendOrRecvStatus::Received(UsbEndpoint::MainHid)) + } } impl Default for TestEnv { fn default() -> Self { let rng = StdRng::seed_from_u64(0); let user_presence = TestUserPresence { - check: Box::new(|| Ok(())), + check: Box::new(|| (Ok(()), None)), }; let storage = new_storage(); let store = Store::new(storage).ok().unwrap(); @@ -224,12 +229,7 @@ impl Env for TestEnv { &self.customization } - fn main_hid_connection(&mut self) -> &mut Self::HidConnection { - self - } - - #[cfg(feature = "vendor_hid")] - fn vendor_hid_connection(&mut self) -> &mut Self::HidConnection { + fn hid_connection(&mut self) -> &mut Self { self } diff --git a/src/env/tock/mod.rs b/src/env/tock/mod.rs index 6ebc5778..15bbcce3 100644 --- a/src/env/tock/mod.rs +++ b/src/env/tock/mod.rs @@ -15,20 +15,17 @@ use alloc::boxed::Box; use alloc::vec::Vec; use clock::TockClock; -use core::cell::Cell; use core::convert::TryFrom; use core::marker::PhantomData; #[cfg(all(target_has_atomic = "8", not(feature = "std")))] use core::sync::atomic::{AtomicBool, Ordering}; -use libtock_buttons::{ButtonListener, ButtonState, Buttons}; use libtock_console::{Console, ConsoleWriter}; -use libtock_drivers::result::{FlexUnwrap, TockError}; use libtock_drivers::timer::Duration; use libtock_drivers::usb_ctap_hid::UsbCtapHid; -use libtock_drivers::{rng, timer, usb_ctap_hid}; +use libtock_drivers::{rng, usb_ctap_hid}; use libtock_leds::Leds; use libtock_platform as platform; -use libtock_platform::{ErrorCode, Syscalls}; +use libtock_platform::Syscalls; use opensk::api::connection::{ HidConnection, SendOrRecvError, SendOrRecvResult, SendOrRecvStatus, UsbEndpoint, }; @@ -44,7 +41,7 @@ use opensk::env::Env; #[cfg(feature = "std")] use persistent_store::BufferOptions; use persistent_store::{StorageResult, Store}; -use platform::{share, DefaultConfig, Subscribe}; +use platform::DefaultConfig; use rand_core::{impls, CryptoRng, Error, RngCore}; #[cfg(feature = "std")] @@ -76,6 +73,9 @@ const TOCK_CUSTOMIZATION: CustomizationImpl = CustomizationImpl { ..DEFAULT_CUSTOMIZATION }; +// This timeout should rarely be relevant, execution returns without blocking. +const SEND_TIMEOUT_MS: Duration = Duration::from_ms(1000); + /// RNG backed by the TockOS rng driver. pub struct TockRng { _syscalls: PhantomData, @@ -112,28 +112,6 @@ impl RngCore for TockRng { impl Rng for TockRng {} -pub struct TockHidConnection { - endpoint: UsbEndpoint, - s: PhantomData, -} - -impl HidConnection for TockHidConnection { - fn send_and_maybe_recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> SendOrRecvResult { - match UsbCtapHid::::send_or_recv_with_timeout( - buf, - Duration::from_ms(timeout_ms as isize), - self.endpoint as u32, - ) { - Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Ok(SendOrRecvStatus::Timeout), - Ok(usb_ctap_hid::SendOrRecvStatus::Sent) => Ok(SendOrRecvStatus::Sent), - Ok(usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint)) => { - UsbEndpoint::try_from(recv_endpoint as usize).map(SendOrRecvStatus::Received) - } - _ => Err(SendOrRecvError), - } - } -} - pub struct TockEnv< S: Syscalls, C: platform::subscribe::Config + platform::allow_ro::Config = DefaultConfig, @@ -141,9 +119,6 @@ pub struct TockEnv< rng: TockRng, store: Store>, upgrade_storage: Option>, - main_connection: TockHidConnection, - #[cfg(feature = "vendor_hid")] - vendor_connection: TockHidConnection, blink_pattern: usize, clock: TockClock, c: PhantomData, @@ -167,15 +142,6 @@ impl D rng, store, upgrade_storage, - main_connection: TockHidConnection { - endpoint: UsbEndpoint::MainHid, - s: PhantomData, - }, - #[cfg(feature = "vendor_hid")] - vendor_connection: TockHidConnection { - endpoint: UsbEndpoint::VendorHid, - s: PhantomData, - }, blink_pattern: 0, clock: TockClock::default(), c: PhantomData, @@ -287,6 +253,36 @@ where } } +impl HidConnection for TockEnv +where + S: Syscalls, + C: platform::subscribe::Config + platform::allow_ro::Config, +{ + fn send(&mut self, buf: &[u8; 64], endpoint: UsbEndpoint) -> SendOrRecvResult { + match UsbCtapHid::::send(buf, SEND_TIMEOUT_MS, endpoint as u32) { + Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Ok(SendOrRecvStatus::Timeout), + Ok(usb_ctap_hid::SendOrRecvStatus::Sent) => Ok(SendOrRecvStatus::Sent), + Ok(usb_ctap_hid::SendOrRecvStatus::Received(_)) => { + panic!("Returned Received status on send") + } + Err(_) => Err(SendOrRecvError), + } + } + + fn recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> SendOrRecvResult { + match UsbCtapHid::::recv_with_timeout(buf, Duration::from_ms(timeout_ms as isize)) { + Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Ok(SendOrRecvStatus::Timeout), + Ok(usb_ctap_hid::SendOrRecvStatus::Sent) => { + panic!("Returned Sent status on receive") + } + Ok(usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint)) => { + UsbEndpoint::try_from(recv_endpoint as usize).map(SendOrRecvStatus::Received) + } + Err(_) => Err(SendOrRecvError), + } + } +} + impl UserPresence for TockEnv where S: Syscalls, @@ -297,87 +293,32 @@ where } fn wait_with_timeout(&mut self, timeout_ms: usize) -> UserPresenceResult { - if timeout_ms == 0 { - return Err(UserPresenceError::Timeout); - } blink_leds::(self.blink_pattern); self.blink_pattern += 1; - // enable interrupts for all buttons - let num_buttons = Buttons::::count().map_err(|_| UserPresenceError::Fail)?; - (0..num_buttons) - .try_for_each(|n| Buttons::::enable_interrupts(n)) - .map_err(|_| UserPresenceError::Fail)?; - - let button_touched = Cell::new(false); - let button_listener = ButtonListener(|_button_num, state| { - match state { - ButtonState::Pressed => button_touched.set(true), - ButtonState::Released => (), - }; - }); - - // Setup a keep-alive callback but don't enable it yet - let keepalive_expired = Cell::new(false); - let mut keepalive_callback = - timer::with_callback::(|_| keepalive_expired.set(true)); - share::scope::< - ( - Subscribe<_, { libtock_buttons::DRIVER_NUM }, 0>, - Subscribe< - S, - { libtock_drivers::timer::DRIVER_NUM }, - { libtock_drivers::timer::subscribe::CALLBACK }, - >, - ), - _, - _, - >(|handle| { - let (sub_button, sub_timer) = handle.split(); - Buttons::::register_listener(&button_listener, sub_button) - .map_err(|_| UserPresenceError::Fail)?; - - let mut keepalive = keepalive_callback.init().flex_unwrap(); - keepalive_callback - .enable(sub_timer) - .map_err(|_| UserPresenceError::Fail)?; - keepalive - .set_alarm(timer::Duration::from_ms(timeout_ms as isize)) - .flex_unwrap(); - - // Wait for a button touch or an alarm. - libtock_drivers::util::Util::::yieldk_for(|| { - button_touched.get() || keepalive_expired.get() - }); - - Buttons::::unregister_listener(); - - // disable event interrupts for all buttons - (0..num_buttons) - .try_for_each(|n| Buttons::::disable_interrupts(n)) - .map_err(|_| UserPresenceError::Fail)?; - - // Cleanup alarm callback. - match keepalive.stop_alarm() { - Ok(()) => (), - Err(TockError::Command(ErrorCode::Already)) => assert!(keepalive_expired.get()), - Err(_e) => { - #[cfg(feature = "debug_ctap")] - panic!("Unexpected error when stopping alarm: {:?}", _e); - #[cfg(not(feature = "debug_ctap"))] - panic!("Unexpected error when stopping alarm: "); - } + let mut packet = [0; 64]; + let result = + UsbCtapHid::::recv_with_buttons(&mut packet, Duration::from_ms(timeout_ms as isize)); + let (status, button_touched) = match result { + Ok((status, button_touched)) => (status, button_touched), + Err(_) => return (Err(UserPresenceError::Fail), None), + }; + let data = match status { + usb_ctap_hid::SendOrRecvStatus::Timeout => None, + usb_ctap_hid::SendOrRecvStatus::Sent => { + panic!("Returned Sent status on receive") } + usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint) => { + UsbEndpoint::try_from(recv_endpoint as usize) + .ok() + .map(|e| (packet, e)) + } + }; - Ok::<(), UserPresenceError>(()) - })?; - - if button_touched.get() { - Ok(()) - } else if keepalive_expired.get() { - Err(UserPresenceError::Timeout) + if button_touched { + (Ok(()), data) } else { - panic!("Unexpected exit condition"); + (Err(UserPresenceError::Timeout), data) } } @@ -403,7 +344,7 @@ impl E type Clock = TockClock; type Write = ConsoleWriter; type Customization = CustomizationImpl; - type HidConnection = TockHidConnection; + type HidConnection = Self; type Crypto = SoftwareCrypto; fn rng(&mut self) -> &mut Self::Rng { @@ -434,13 +375,8 @@ impl E &TOCK_CUSTOMIZATION } - fn main_hid_connection(&mut self) -> &mut Self::HidConnection { - &mut self.main_connection - } - - #[cfg(feature = "vendor_hid")] - fn vendor_hid_connection(&mut self) -> &mut Self::HidConnection { - &mut self.vendor_connection + fn hid_connection(&mut self) -> &mut Self { + self } fn process_vendor_command(&mut self, bytes: &[u8], channel: Channel) -> Option> { diff --git a/src/main.rs b/src/main.rs index a71c8ff9..fecb0f56 100644 --- a/src/main.rs +++ b/src/main.rs @@ -34,15 +34,13 @@ use libtock_buttons::Buttons; use libtock_console::Console; #[cfg(feature = "debug_ctap")] use libtock_console::ConsoleWriter; -use libtock_drivers::result::FlexUnwrap; -use libtock_drivers::timer::Duration; use libtock_drivers::usb_ctap_hid; #[cfg(not(feature = "std"))] use libtock_runtime::{set_main, stack_size, TockSyscalls}; #[cfg(feature = "std")] use libtock_unittest::fake; use opensk::api::clock::Clock; -use opensk::api::connection::UsbEndpoint; +use opensk::api::connection::{HidConnection, SendOrRecvStatus, UsbEndpoint}; use opensk::ctap::hid::HidPacketIterator; use opensk::ctap::KEEPALIVE_DELAY_MS; use opensk::env::Env; @@ -53,9 +51,6 @@ stack_size! {0x4000} #[cfg(not(feature = "std"))] set_main! {main} -const SEND_TIMEOUT_MS: Duration = Duration::from_ms(1000); -const KEEPALIVE_DELAY_MS_TOCK: Duration = Duration::from_ms(KEEPALIVE_DELAY_MS as isize); - #[cfg(not(feature = "vendor_hid"))] const NUM_ENDPOINTS: usize = 1; #[cfg(feature = "vendor_hid")] @@ -160,14 +155,19 @@ fn main() { let mut pkt_request = [0; 64]; if let Some(packet) = replies.next_packet() { - match usb_ctap_hid::UsbCtapHid::::send( - &packet.packet, - SEND_TIMEOUT_MS, - packet.endpoint as u32, - ) - .flex_unwrap() - { - usb_ctap_hid::SendOrRecvStatus::Sent => { + let hid_connection = ctap.env().hid_connection(); + match hid_connection.send(&packet.packet, packet.endpoint) { + Ok(SendOrRecvStatus::Timeout) => { + #[cfg(feature = "debug_ctap")] + print_packet_notice::( + "Timeout while sending packet", + ctap.env().clock().timestamp_us(), + &mut writer, + ); + // The client is unresponsive, so we discard all pending packets. + replies.clear(packet.endpoint); + } + Ok(SendOrRecvStatus::Sent) => { #[cfg(feature = "debug_ctap")] print_packet_notice::( "Sent packet", @@ -175,40 +175,23 @@ fn main() { &mut writer, ); } - usb_ctap_hid::SendOrRecvStatus::Timeout => { + _ => panic!("Unexpected status on USB send"), + } + } else { + let hid_connection = ctap.env().hid_connection(); + usb_endpoint = match hid_connection.recv(&mut pkt_request, KEEPALIVE_DELAY_MS) { + Ok(SendOrRecvStatus::Timeout) => None, + Ok(SendOrRecvStatus::Received(endpoint)) => { #[cfg(feature = "debug_ctap")] print_packet_notice::( - "Timeout while sending packet", + "Received packet", ctap.env().clock().timestamp_us(), &mut writer, ); - // The client is unresponsive, so we discard all pending packets. - replies.clear(packet.endpoint); + UsbEndpoint::try_from(endpoint as usize).ok() } - _ => panic!("Unexpected status on USB transmission"), + _ => panic!("Unexpected status on USB recv"), }; - } else { - usb_endpoint = - match usb_ctap_hid::UsbCtapHid::::recv_with_timeout( - &mut pkt_request, - KEEPALIVE_DELAY_MS_TOCK, - ) - .flex_unwrap() - { - usb_ctap_hid::SendOrRecvStatus::Received(endpoint) => { - #[cfg(feature = "debug_ctap")] - print_packet_notice::( - "Received packet", - ctap.env().clock().timestamp_us(), - &mut writer, - ); - UsbEndpoint::try_from(endpoint as usize).ok() - } - usb_ctap_hid::SendOrRecvStatus::Sent => { - panic!("Returned transmit status on receive") - } - usb_ctap_hid::SendOrRecvStatus::Timeout => None, - }; } #[cfg(feature = "with_ctap1")] diff --git a/third_party/libtock-drivers/Cargo.toml b/third_party/libtock-drivers/Cargo.toml index 3b3841fa..27237e98 100644 --- a/third_party/libtock-drivers/Cargo.toml +++ b/third_party/libtock-drivers/Cargo.toml @@ -10,6 +10,7 @@ edition = "2018" [dependencies] libtock_alarm = { path = "../../third_party/libtock-rs/apis/alarm" } +libtock_buttons = { path = "../../third_party/libtock-rs/apis/buttons" } libtock_console = { path = "../../third_party/libtock-rs/apis/console" } libtock_platform = { path = "../../third_party/libtock-rs/platform" } diff --git a/third_party/libtock-drivers/src/usb_ctap_hid.rs b/third_party/libtock-drivers/src/usb_ctap_hid.rs index 0faaefad..edd09a58 100644 --- a/third_party/libtock-drivers/src/usb_ctap_hid.rs +++ b/third_party/libtock-drivers/src/usb_ctap_hid.rs @@ -26,6 +26,7 @@ use libtock_platform::{share, DefaultConfig, ErrorCode, Syscalls}; use platform::share::Handle; use platform::subscribe::OneId; use platform::{AllowRo, AllowRw, Subscribe, Upcall}; +use libtock_buttons::{ButtonListener, ButtonState, Buttons}; const DRIVER_NUMBER: u32 = 0x20009; @@ -35,7 +36,7 @@ mod command_nr { pub const CONNECT: u32 = 1; pub const TRANSMIT: u32 = 2; pub const RECEIVE: u32 = 3; - pub const TRANSMIT_OR_RECEIVE: u32 = 4; + pub const _TRANSMIT_OR_RECEIVE: u32 = 4; pub const CANCEL: u32 = 5; } @@ -169,49 +170,6 @@ impl UsbCtapHid { Self::send_detail(buf, timeout_delay, endpoint) } - /// Either sends or receives a packet within a given time. - /// - /// Because USB transactions are initiated by the host, we don't decide whether an IN transaction - /// (send for us), an OUT transaction (receive for us), or no transaction at all will happen next. - /// - /// - If an IN transaction happens first, the initial content of buf is sent to the host and the - /// Sent status is returned. - /// - If an OUT transaction happens first, the content of buf is replaced by the packet received - /// from the host and Received status is returned. In that case, the original content of buf is not - /// sent to the host, and it's up to the caller to retry sending or to handle the packet received - /// from the host. - /// If the timeout elapses, return None. - #[allow(clippy::let_and_return)] - pub fn send_or_recv_with_timeout( - buf: &mut [u8; 64], - timeout_delay: Duration, - endpoint: u32, - ) -> TockResult { - #[cfg(feature = "verbose_usb")] - writeln!( - Console::::writer(), - "Sending packet with timeout of {} ms = {:02x?}", - timeout_delay.ms(), - buf as &[u8] - ) - .unwrap(); - - let result = Self::send_or_recv_with_timeout_detail(buf, timeout_delay, endpoint); - - #[cfg(feature = "verbose_usb")] - if let Ok(SendOrRecvStatus::Received(received_endpoint)) = result { - writeln!( - Console::::writer(), - "Received packet = {:02x?} on endpoint {}", - buf as &[u8], - received_endpoint as u8, - ) - .unwrap(); - } - - result - } - fn recv_with_timeout_detail( buf: &mut [u8; 64], timeout_delay: Duration, @@ -393,7 +351,7 @@ impl UsbCtapHid { // The app should wait for it, but it may never happen if the remote app crashes. // We just return to avoid a deadlock. #[cfg(feature = "debug_ctap")] - writeln!(Console::::writer(), "Couldn't cancel the USB receive").unwrap(); + writeln!(Console::::writer(), "Couldn't cancel the USB send").unwrap(); } Err(e) => panic!("Unexpected error when cancelling USB receive: {:?}", e), } @@ -402,61 +360,69 @@ impl UsbCtapHid { status } - fn send_or_recv_with_timeout_detail( + pub fn recv_with_buttons( buf: &mut [u8; 64], timeout_delay: Duration, - endpoint: u32, - ) -> TockResult { + ) -> TockResult<(SendOrRecvStatus, bool)> { let status: Cell> = Cell::new(None); - let alarm = UsbCtapHidListener(|direction, endpoint| { - let option = match direction { - subscribe_nr::TRANSMIT => Some(SendOrRecvStatus::Sent), - subscribe_nr::RECEIVE => Some(SendOrRecvStatus::Received(endpoint)), - _ => None, - }; - status.set(option); + + let alarm = UsbCtapHidListener(|direction, endpoint| match direction { + subscribe_nr::RECEIVE => status.set(Some(SendOrRecvStatus::Received(endpoint))), + // Unknown direction or "transmitted" sent by the kernel + _ => status.set(None), }); - let mut recv_buf = [0; 64]; - // init the time-out callback but don't enable it yet - let mut timeout_callback = timer::with_callback::(|_| { - status.set(Some(SendOrRecvStatus::Timeout)); + let num_buttons = Buttons::::count().map_err(|_| ErrorCode::Fail)?; + (0..num_buttons) + .try_for_each(|n| Buttons::::enable_interrupts(n)) + .map_err(|_| ErrorCode::Fail)?; + + let button_touched = Cell::new(false); + let button_listener = ButtonListener(|_button_num, state| { + match state { + ButtonState::Pressed => { + status.set(Some(SendOrRecvStatus::Timeout)); + button_touched.set(true); + } + ButtonState::Released => (), + }; }); + + let mut timeout_callback = + timer::with_callback::(|_| status.set(Some(SendOrRecvStatus::Timeout))); let status = share::scope::< ( - AllowRo<_, DRIVER_NUMBER, { ro_allow_nr::TRANSMIT }>, AllowRw<_, DRIVER_NUMBER, { rw_allow_nr::RECEIVE }>, - Subscribe<_, DRIVER_NUMBER, { subscribe_nr::TRANSMIT }>, Subscribe<_, DRIVER_NUMBER, { subscribe_nr::RECEIVE }>, - Subscribe<_, { timer::DRIVER_NUM }, { timer::subscribe::CALLBACK }>, + Subscribe, + Subscribe<_, { libtock_buttons::DRIVER_NUM }, 0>, ), _, _, >(|handle| { - let (allow_ro, allow_rw, sub_send, sub_recv, sub_timer) = handle.split(); - - S::allow_ro::(allow_ro, buf)?; - S::allow_rw::(allow_rw, &mut recv_buf)?; + let (allow, subscribe_recv, subscribe_timer, sub_button) = handle.split(); + S::allow_rw::(allow, buf)?; - Self::register_listener::<{ subscribe_nr::TRANSMIT }, _>(&alarm, sub_send)?; - Self::register_listener::<{ subscribe_nr::RECEIVE }, _>(&alarm, sub_recv)?; + Self::register_listener::<{ subscribe_nr::RECEIVE }, _>(&alarm, subscribe_recv)?; + Buttons::::register_listener(&button_listener, sub_button) + .map_err(|_| ErrorCode::Fail)?; let mut timeout = timeout_callback.init()?; - timeout_callback.enable(sub_timer)?; - timeout.set_alarm(timeout_delay)?; - - // Trigger USB transmission. - S::command( - DRIVER_NUMBER, - command_nr::TRANSMIT_OR_RECEIVE, - endpoint as u32, - 0, - ) - .to_result::<(), ErrorCode>()?; + timeout_callback.enable(subscribe_timer)?; + timeout + .set_alarm(timeout_delay) + .map_err(|_| ErrorCode::Fail)?; - util::Util::::yieldk_for(|| status.get().is_some()); - Self::unregister_listener(subscribe_nr::TRANSMIT); + S::command(DRIVER_NUMBER, command_nr::RECEIVE, 0, 0).to_result::<(), ErrorCode>()?; + + Util::::yieldk_for(|| button_touched.get() || status.get().is_some()); Self::unregister_listener(subscribe_nr::RECEIVE); + Buttons::::unregister_listener(); + + // disable event interrupts for all buttons + (0..num_buttons) + .try_for_each(|n| Buttons::::disable_interrupts(n)) + .map_err(|_| ErrorCode::Fail)?; let status = match status.get() { Some(status) => Ok::(status), @@ -465,15 +431,11 @@ impl UsbCtapHid { // Cleanup alarm callback. match timeout.stop_alarm() { - Ok(_) => (), + Ok(()) => (), Err(TockError::Command(ErrorCode::Already)) => { if matches!(status, SendOrRecvStatus::Timeout) { #[cfg(feature = "debug_ctap")] - writeln!( - Console::::writer(), - "The send/receive timeout already expired, but the callback wasn't executed." - ) - .unwrap(); + write!(Console::::writer(), ".").unwrap(); } } Err(_e) => { @@ -491,7 +453,7 @@ impl UsbCtapHid { #[cfg(feature = "verbose_usb")] writeln!( Console::::writer(), - "Cancelling USB transaction due to timeout" + "Cancelling USB receive due to timeout" ) .unwrap(); let result = @@ -505,17 +467,12 @@ impl UsbCtapHid { // The app should wait for it, but it may never happen if the remote app crashes. // We just return to avoid a deadlock. #[cfg(feature = "debug_ctap")] - writeln!(Console::::writer(), "Couldn't cancel the transaction").unwrap(); + writeln!(Console::::writer(), "Couldn't cancel the USB receive with buttons").unwrap(); } - Err(e) => panic!("Unexpected error when cancelling USB transaction: {:?}", e), + Err(e) => panic!("Unexpected error when cancelling USB receive: {:?}", e), } - #[cfg(feature = "debug_ctap")] - writeln!(Console::::writer(), "Cancelled USB transaction!").unwrap(); } - if matches!(status, Ok(SendOrRecvStatus::Received(_))) { - buf.copy_from_slice(&recv_buf); - } - status + status.map(|s| (s, button_touched.get())) } } From 01d7ee34cddd67000ae70ad0e78ff6f31c9d4814 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Mon, 2 Sep 2024 11:22:24 +0200 Subject: [PATCH 2/4] Refactors status and error codes There are a few small logic fixes hidden in this comit: - We always call `check_complete`, even if we received a cancel. - We only accept Cancel packets from the active channel. - Send timeouts now return `Ctap2StatusCode::CTAP1_ERR_TIMEOUT`. --- libraries/opensk/src/api/connection.rs | 24 ++-- libraries/opensk/src/api/user_presence.rs | 20 ++-- libraries/opensk/src/ctap/hid/mod.rs | 2 +- libraries/opensk/src/ctap/mod.rs | 134 +++++++++++++--------- libraries/opensk/src/ctap/status_code.rs | 1 - libraries/opensk/src/env/test/mod.rs | 38 +++--- src/env/tock/mod.rs | 53 +++++---- src/main.rs | 17 +-- 8 files changed, 159 insertions(+), 130 deletions(-) diff --git a/libraries/opensk/src/api/connection.rs b/libraries/opensk/src/api/connection.rs index f66f4ac2..5d7e0f93 100644 --- a/libraries/opensk/src/api/connection.rs +++ b/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,32 +23,26 @@ pub enum UsbEndpoint { } impl TryFrom for UsbEndpoint { - type Error = SendOrRecvError; + type Error = Ctap2StatusCode; - fn try_from(endpoint_num: usize) -> Result { + fn try_from(endpoint_num: usize) -> CtapResult { 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; - pub trait HidConnection { - fn send(&mut self, buf: &[u8; 64], endpoint: UsbEndpoint) -> SendOrRecvResult; - fn 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; } #[cfg(test)] @@ -59,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) + ); } } diff --git a/libraries/opensk/src/api/user_presence.rs b/libraries/opensk/src/api/user_presence.rs index 7f7f24c7..dc9e9557 100644 --- a/libraries/opensk/src/api/user_presence.rs +++ b/libraries/opensk/src/api/user_presence.rs @@ -12,7 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -use crate::api::connection::UsbEndpoint; +use crate::api::connection::RecvStatus; +use crate::ctap::status_code::CtapResult; #[derive(Debug)] pub enum UserPresenceError { @@ -22,14 +23,10 @@ pub enum UserPresenceError { Canceled, /// User presence check timed out. Timeout, - /// Unexpected (e.g., hardware) failures - Fail, } +pub type UserPresenceResult = Result<(), UserPresenceError>; -pub type UserPresenceResult = ( - Result<(), UserPresenceError>, - Option<([u8; 64], UsbEndpoint)>, -); +pub type UserPresenceWaitResult = CtapResult<(UserPresenceResult, RecvStatus)>; pub trait UserPresence { /// Initializes for a user presence check. @@ -40,8 +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`]. - /// Additionally returns a packet if one was received during the wait. - 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. /// diff --git a/libraries/opensk/src/ctap/hid/mod.rs b/libraries/opensk/src/ctap/hid/mod.rs index ac48f32e..4ad1dbc4 100644 --- a/libraries/opensk/src/ctap/hid/mod.rs +++ b/libraries/opensk/src/ctap/hid/mod.rs @@ -440,7 +440,7 @@ impl CtapHid { }) } - /// Generates a the HID response packets for a busy error. + /// 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)) } diff --git a/libraries/opensk/src/ctap/mod.rs b/libraries/opensk/src/ctap/mod.rs index 75e8f2d3..6146e542 100644 --- a/libraries/opensk/src/ctap/mod.rs +++ b/libraries/opensk/src/ctap/mod.rs @@ -63,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; @@ -258,25 +258,25 @@ fn truncate_to_char_boundary(s: &str, mut max: usize) -> &str { } /// Send non-critical packets using fire-and-forget. -fn send_packets(env: &mut E, endpoint: UsbEndpoint, packets: HidPacketIterator) { +fn send_packets( + env: &mut E, + endpoint: UsbEndpoint, + packets: HidPacketIterator, +) -> CtapResult<()> { for pkt in packets { - match env.hid_connection().send(&pkt, endpoint) { - Ok(SendOrRecvStatus::Timeout) => { - debug_ctap!(env, "Timeout sending packet"); - } - Ok(SendOrRecvStatus::Sent) => (), - _ => panic!("Error sending packet"), - } + env.hid_connection().send(&pkt, endpoint)?; } + Ok(()) } -/// Blocks for user presence. -/// -/// Returns an error in case of timeout, user declining presence request, or keepalive error. -pub fn check_user_presence(env: &mut E, channel: Channel) -> CtapResult<()> { - env.user_presence().check_init(); - let loop_timer = env.clock().make_timer(TOUCH_TIMEOUT_MS); +fn is_cancel(packet: &ProcessedPacket) -> bool { + match packet { + ProcessedPacket::InitPacket { cmd, .. } => *cmd == CtapHidCommand::Cancel as u8, + ProcessedPacket::ContinuationPacket { .. } => false, + } +} +fn wait_and_respond_busy(env: &mut E, channel: Channel) -> CtapResult<()> { let (cid, transport) = match channel { Channel::MainHid(cid) => (cid, Transport::MainHid), #[cfg(feature = "vendor_hid")] @@ -284,13 +284,13 @@ pub fn check_user_presence(env: &mut E, channel: Channel) -> CtapResult< }; let endpoint = transport.usb_endpoint(); - // All fallible functions are called without '?' operator to always reach - // check_complete(...) cleanup function. - - let mut result = Err(UserPresenceError::Timeout); - while !env.clock().is_elapsed(&loop_timer) { - let (status, data) = env.user_presence().wait_with_timeout(KEEPALIVE_DELAY_MS); - if let Some((packet, rx_endpoint)) = data { + 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::::process_single_packet(&packet); if rx_endpoint != endpoint || received_cid != cid { debug_ctap!( @@ -299,42 +299,49 @@ pub fn check_user_presence(env: &mut E, channel: Channel) -> CtapResult< rx_endpoint, ); let busy_error = CtapHid::::busy_error(received_cid); - send_packets(env, rx_endpoint, busy_error); - } - match processed_packet { - ProcessedPacket::InitPacket { cmd, .. } => { - if cmd == CtapHidCommand::Cancel as u8 { - // 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 with command {} received while sending a KEEPALIVE packet", - cmd, - ); - } - } - // Ignored. We likely ignore the init packet before as well. - ProcessedPacket::ContinuationPacket { .. } => { - debug_ctap!( - env, - "Discarded continuation packet received while sending a KEEPALIVE packet", - ); - } + // 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."); } } - if !matches!(status, Err(UserPresenceError::Timeout)) { - result = status; - break; - } + } + if matches!(up_status, Err(UserPresenceError::Timeout)) { let keepalive_msg = CtapHid::::keepalive(cid, KeepaliveStatus::UpNeeded); - send_packets(env, endpoint, keepalive_msg); + 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(env: &mut E, channel: Channel) -> CtapResult<()> { + const TIMEOUT_ERROR: Ctap2StatusCode = Ctap2StatusCode::CTAP2_ERR_USER_ACTION_TIMEOUT; + + 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. @@ -1425,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; @@ -2252,8 +2258,7 @@ mod test { #[test] fn test_process_make_credential_cancelled() { let mut env = TestEnv::default(); - env.user_presence() - .set(|| (Err(UserPresenceError::Canceled), None)); + env.user_presence().set(|| Err(UserPresenceError::Canceled)); let mut ctap_state = CtapState::::new(&mut env); let make_credential_params = create_minimal_make_credential_parameters(); @@ -3133,8 +3138,7 @@ mod test { #[test] fn test_process_reset_cancelled() { let mut env = TestEnv::default(); - env.user_presence() - .set(|| (Err(UserPresenceError::Canceled), None)); + env.user_presence().set(|| Err(UserPresenceError::Canceled)); let mut ctap_state = CtapState::::new(&mut env); let reset_reponse = ctap_state.process_reset(&mut env, DUMMY_CHANNEL); @@ -3360,6 +3364,22 @@ mod test { assert!(matches!(response, Ok(_))); } + #[test] + fn test_check_user_presence_timeout() { + let mut env = TestEnv::default(); + let now_ms = env.clock().access(); + env.user_presence().set(move || { + let mut locked_now_ms = now_ms.lock().unwrap(); + *locked_now_ms += 100; + Err(UserPresenceError::Timeout) + }); + let response = check_user_presence(&mut env, DUMMY_CHANNEL); + assert!(matches!( + response, + Err(Ctap2StatusCode::CTAP2_ERR_USER_ACTION_TIMEOUT) + )); + } + #[test] fn test_channel_interleaving() { let mut env = TestEnv::default(); diff --git a/libraries/opensk/src/ctap/status_code.rs b/libraries/opensk/src/ctap/status_code.rs index 2e47bcdd..27126180 100644 --- a/libraries/opensk/src/ctap/status_code.rs +++ b/libraries/opensk/src/ctap/status_code.rs @@ -93,7 +93,6 @@ impl From for Ctap2StatusCode { UserPresenceError::Timeout => Self::CTAP2_ERR_USER_ACTION_TIMEOUT, UserPresenceError::Declined => Self::CTAP2_ERR_OPERATION_DENIED, UserPresenceError::Canceled => Self::CTAP2_ERR_KEEPALIVE_CANCEL, - UserPresenceError::Fail => Self::CTAP2_ERR_VENDOR_HARDWARE_FAILURE, } } } diff --git a/libraries/opensk/src/env/test/mod.rs b/libraries/opensk/src/env/test/mod.rs index 836720b4..33bc9763 100644 --- a/libraries/opensk/src/env/test/mod.rs +++ b/libraries/opensk/src/env/test/mod.rs @@ -13,19 +13,20 @@ // limitations under the License. use crate::api::clock::Clock; -use crate::api::connection::{HidConnection, SendOrRecvResult, SendOrRecvStatus, UsbEndpoint}; +use crate::api::connection::{HidConnection, RecvStatus, UsbEndpoint}; use crate::api::crypto::software_crypto::SoftwareCrypto; use crate::api::customization::DEFAULT_CUSTOMIZATION; use crate::api::key_store; use crate::api::persist::{Persist, PersistIter}; use crate::api::rng::Rng; -use crate::api::user_presence::{UserPresence, UserPresenceResult}; +use crate::api::user_presence::{UserPresence, UserPresenceResult, UserPresenceWaitResult}; use crate::ctap::status_code::CtapResult; use crate::env::Env; use customization::TestCustomization; use persistent_store::{BufferOptions, BufferStorage, Store}; use rand::rngs::StdRng; use rand::SeedableRng; +use std::sync::{Arc, Mutex}; pub mod customization; @@ -50,12 +51,17 @@ pub struct TestTimer { #[derive(Debug, Default)] pub struct TestClock { /// The current time, as advanced, in milliseconds. - now_ms: usize, + now_ms: Arc>, } impl TestClock { pub fn advance(&mut self, milliseconds: usize) { - self.now_ms += milliseconds; + let mut locked_now_ms = self.now_ms.lock().unwrap(); + *locked_now_ms += milliseconds; + } + + pub fn access(&self) -> Arc> { + self.now_ms.clone() } } @@ -64,18 +70,18 @@ impl Clock for TestClock { fn make_timer(&mut self, milliseconds: usize) -> Self::Timer { TestTimer { - end_ms: self.now_ms + milliseconds, + end_ms: *self.now_ms.lock().unwrap() + milliseconds, } } fn is_elapsed(&mut self, timer: &Self::Timer) -> bool { - self.now_ms >= timer.end_ms + *self.now_ms.lock().unwrap() >= timer.end_ms } #[cfg(feature = "debug_ctap")] fn timestamp_us(&mut self) -> usize { // Unused, but let's implement something because it's easy. - self.now_ms * 1000 + *self.now_ms.lock().unwrap() * 1000 } } @@ -130,12 +136,12 @@ impl Persist for TestEnv { impl HidConnection for TestEnv { // TODO: Implement I/O from canned requests/responses for integration testing. - fn send(&mut self, _buf: &[u8; 64], _endpoint: UsbEndpoint) -> SendOrRecvResult { - Ok(SendOrRecvStatus::Sent) + fn send(&mut self, _buf: &[u8; 64], _endpoint: UsbEndpoint) -> CtapResult<()> { + Ok(()) } - fn recv(&mut self, _buf: &mut [u8; 64], _timeout_ms: usize) -> SendOrRecvResult { - Ok(SendOrRecvStatus::Received(UsbEndpoint::MainHid)) + fn recv(&mut self, _buf: &mut [u8; 64], _timeout_ms: usize) -> CtapResult { + Ok(RecvStatus::Received(UsbEndpoint::MainHid)) } } @@ -143,7 +149,7 @@ impl Default for TestEnv { fn default() -> Self { let rng = StdRng::seed_from_u64(0); let user_presence = TestUserPresence { - check: Box::new(|| (Ok(()), None)), + check: Box::new(|| Ok(())), }; let storage = new_storage(); let store = Store::new(storage).ok().unwrap(); @@ -182,8 +188,12 @@ impl TestUserPresence { impl UserPresence for TestUserPresence { fn check_init(&mut self) {} - fn wait_with_timeout(&mut self, _timeout_ms: usize) -> UserPresenceResult { - (self.check)() + fn wait_with_timeout( + &mut self, + _buf: &mut [u8; 64], + _timeout_ms: usize, + ) -> UserPresenceWaitResult { + Ok(((self.check)(), RecvStatus::Timeout)) } fn check_complete(&mut self) {} } diff --git a/src/env/tock/mod.rs b/src/env/tock/mod.rs index 15bbcce3..cbdf1b49 100644 --- a/src/env/tock/mod.rs +++ b/src/env/tock/mod.rs @@ -26,16 +26,14 @@ use libtock_drivers::{rng, usb_ctap_hid}; use libtock_leds::Leds; use libtock_platform as platform; use libtock_platform::Syscalls; -use opensk::api::connection::{ - HidConnection, SendOrRecvError, SendOrRecvResult, SendOrRecvStatus, UsbEndpoint, -}; +use opensk::api::connection::{HidConnection, RecvStatus, UsbEndpoint}; use opensk::api::crypto::software_crypto::SoftwareCrypto; use opensk::api::customization::{CustomizationImpl, AAGUID_LENGTH, DEFAULT_CUSTOMIZATION}; use opensk::api::key_store; use opensk::api::persist::{Persist, PersistIter}; use opensk::api::rng::Rng; -use opensk::api::user_presence::{UserPresence, UserPresenceError, UserPresenceResult}; -use opensk::ctap::status_code::CtapResult; +use opensk::api::user_presence::{UserPresence, UserPresenceError, UserPresenceWaitResult}; +use opensk::ctap::status_code::{Ctap2StatusCode, CtapResult}; use opensk::ctap::Channel; use opensk::env::Env; #[cfg(feature = "std")] @@ -258,27 +256,27 @@ where S: Syscalls, C: platform::subscribe::Config + platform::allow_ro::Config, { - fn send(&mut self, buf: &[u8; 64], endpoint: UsbEndpoint) -> SendOrRecvResult { + fn send(&mut self, buf: &[u8; 64], endpoint: UsbEndpoint) -> CtapResult<()> { match UsbCtapHid::::send(buf, SEND_TIMEOUT_MS, endpoint as u32) { - Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Ok(SendOrRecvStatus::Timeout), - Ok(usb_ctap_hid::SendOrRecvStatus::Sent) => Ok(SendOrRecvStatus::Sent), + Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Err(Ctap2StatusCode::CTAP1_ERR_TIMEOUT), + Ok(usb_ctap_hid::SendOrRecvStatus::Sent) => Ok(()), Ok(usb_ctap_hid::SendOrRecvStatus::Received(_)) => { panic!("Returned Received status on send") } - Err(_) => Err(SendOrRecvError), + Err(_) => Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE), } } - fn recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> SendOrRecvResult { + fn recv(&mut self, buf: &mut [u8; 64], timeout_ms: usize) -> CtapResult { match UsbCtapHid::::recv_with_timeout(buf, Duration::from_ms(timeout_ms as isize)) { - Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Ok(SendOrRecvStatus::Timeout), + Ok(usb_ctap_hid::SendOrRecvStatus::Timeout) => Ok(RecvStatus::Timeout), Ok(usb_ctap_hid::SendOrRecvStatus::Sent) => { panic!("Returned Sent status on receive") } Ok(usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint)) => { - UsbEndpoint::try_from(recv_endpoint as usize).map(SendOrRecvStatus::Received) + UsbEndpoint::try_from(recv_endpoint as usize).map(RecvStatus::Received) } - Err(_) => Err(SendOrRecvError), + Err(_) => Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE), } } } @@ -292,34 +290,35 @@ where self.blink_pattern = 0; } - fn wait_with_timeout(&mut self, timeout_ms: usize) -> UserPresenceResult { + fn wait_with_timeout( + &mut self, + packet: &mut [u8; 64], + timeout_ms: usize, + ) -> UserPresenceWaitResult { blink_leds::(self.blink_pattern); self.blink_pattern += 1; - let mut packet = [0; 64]; let result = - UsbCtapHid::::recv_with_buttons(&mut packet, Duration::from_ms(timeout_ms as isize)); + UsbCtapHid::::recv_with_buttons(packet, Duration::from_ms(timeout_ms as isize)); let (status, button_touched) = match result { Ok((status, button_touched)) => (status, button_touched), - Err(_) => return (Err(UserPresenceError::Fail), None), + Err(_) => return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_HARDWARE_FAILURE), }; - let data = match status { - usb_ctap_hid::SendOrRecvStatus::Timeout => None, + let recv_status = match status { + usb_ctap_hid::SendOrRecvStatus::Timeout => RecvStatus::Timeout, usb_ctap_hid::SendOrRecvStatus::Sent => { panic!("Returned Sent status on receive") } usb_ctap_hid::SendOrRecvStatus::Received(recv_endpoint) => { - UsbEndpoint::try_from(recv_endpoint as usize) - .ok() - .map(|e| (packet, e)) + RecvStatus::Received(UsbEndpoint::try_from(recv_endpoint as usize)?) } }; - - if button_touched { - (Ok(()), data) + let up_result = if button_touched { + Ok(()) } else { - (Err(UserPresenceError::Timeout), data) - } + Err(UserPresenceError::Timeout) + }; + Ok((up_result, recv_status)) } fn check_complete(&mut self) { diff --git a/src/main.rs b/src/main.rs index fecb0f56..069ee580 100644 --- a/src/main.rs +++ b/src/main.rs @@ -40,8 +40,9 @@ use libtock_runtime::{set_main, stack_size, TockSyscalls}; #[cfg(feature = "std")] use libtock_unittest::fake; use opensk::api::clock::Clock; -use opensk::api::connection::{HidConnection, SendOrRecvStatus, UsbEndpoint}; +use opensk::api::connection::{HidConnection, RecvStatus, UsbEndpoint}; use opensk::ctap::hid::HidPacketIterator; +use opensk::ctap::status_code::Ctap2StatusCode; use opensk::ctap::KEEPALIVE_DELAY_MS; use opensk::env::Env; use opensk::Transport; @@ -157,17 +158,17 @@ fn main() { if let Some(packet) = replies.next_packet() { let hid_connection = ctap.env().hid_connection(); match hid_connection.send(&packet.packet, packet.endpoint) { - Ok(SendOrRecvStatus::Timeout) => { + Err(Ctap2StatusCode::CTAP1_ERR_TIMEOUT) => { #[cfg(feature = "debug_ctap")] print_packet_notice::( - "Timeout while sending packet", + "Timeout on USB send", ctap.env().clock().timestamp_us(), &mut writer, ); // The client is unresponsive, so we discard all pending packets. replies.clear(packet.endpoint); } - Ok(SendOrRecvStatus::Sent) => { + Ok(()) => { #[cfg(feature = "debug_ctap")] print_packet_notice::( "Sent packet", @@ -175,13 +176,13 @@ fn main() { &mut writer, ); } - _ => panic!("Unexpected status on USB send"), + Err(_) => panic!("Error on USB send"), } } else { let hid_connection = ctap.env().hid_connection(); usb_endpoint = match hid_connection.recv(&mut pkt_request, KEEPALIVE_DELAY_MS) { - Ok(SendOrRecvStatus::Timeout) => None, - Ok(SendOrRecvStatus::Received(endpoint)) => { + Ok(RecvStatus::Timeout) => None, + Ok(RecvStatus::Received(endpoint)) => { #[cfg(feature = "debug_ctap")] print_packet_notice::( "Received packet", @@ -190,7 +191,7 @@ fn main() { ); UsbEndpoint::try_from(endpoint as usize).ok() } - _ => panic!("Unexpected status on USB recv"), + Err(_) => panic!("Error on USB recv"), }; } From 269578ba2aa97aa2e9781e8e08b1281fb2b9e5fd Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Thu, 5 Sep 2024 12:22:47 +0200 Subject: [PATCH 3/4] Stable blinking pattern Before, if the device gets a CTAP2 UP request and another channel sends packets, the blinking pattern would have sped up. The patch for the Tock USB implementation fixes a panic after a timeout in send. --- patches/tock/03-add-ctap-modules.patch | 7 +++---- src/env/tock/mod.rs | 15 +++++++++++++-- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/patches/tock/03-add-ctap-modules.patch b/patches/tock/03-add-ctap-modules.patch index d4adb6e9..f4d41350 100644 --- a/patches/tock/03-add-ctap-modules.patch +++ b/patches/tock/03-add-ctap-modules.patch @@ -559,10 +559,10 @@ index 000000000..30cac1323 +} diff --git a/capsules/src/usb/usbc_ctap_hid.rs b/capsules/src/usb/usbc_ctap_hid.rs new file mode 100644 -index 000000000..5ad2c44b3 +index 000000000..fdf92a28e --- /dev/null +++ b/capsules/src/usb/usbc_ctap_hid.rs -@@ -0,0 +1,554 @@ +@@ -0,0 +1,553 @@ +//! A USB HID client of the USB hardware interface + +use super::app::App; @@ -927,9 +927,8 @@ index 000000000..5ad2c44b3 + if self + .client + .map_or(false, |client| client.can_receive_packet(&app)) ++ && self.pending_out.take() + { -+ assert!(self.pending_out.take()); -+ + // Clear any pending packet on the transmitting side. + // It's up to the client to handle the received packet and decide if this packet + // should be re-transmitted or not. diff --git a/src/env/tock/mod.rs b/src/env/tock/mod.rs index cbdf1b49..edb4b50c 100644 --- a/src/env/tock/mod.rs +++ b/src/env/tock/mod.rs @@ -17,6 +17,7 @@ use alloc::vec::Vec; use clock::TockClock; use core::convert::TryFrom; use core::marker::PhantomData; +use core::mem; #[cfg(all(target_has_atomic = "8", not(feature = "std")))] use core::sync::atomic::{AtomicBool, Ordering}; use libtock_console::{Console, ConsoleWriter}; @@ -26,6 +27,7 @@ use libtock_drivers::{rng, usb_ctap_hid}; use libtock_leds::Leds; use libtock_platform as platform; use libtock_platform::Syscalls; +use opensk::api::clock::Clock; use opensk::api::connection::{HidConnection, RecvStatus, UsbEndpoint}; use opensk::api::crypto::software_crypto::SoftwareCrypto; use opensk::api::customization::{CustomizationImpl, AAGUID_LENGTH, DEFAULT_CUSTOMIZATION}; @@ -118,6 +120,7 @@ pub struct TockEnv< store: Store>, upgrade_storage: Option>, blink_pattern: usize, + blink_timer: as Clock>::Timer, clock: TockClock, c: PhantomData, } @@ -141,6 +144,7 @@ impl D store, upgrade_storage, blink_pattern: 0, + blink_timer: as Clock>::Timer::default(), clock: TockClock::default(), c: PhantomData, } @@ -295,8 +299,14 @@ where packet: &mut [u8; 64], timeout_ms: usize, ) -> UserPresenceWaitResult { - blink_leds::(self.blink_pattern); - self.blink_pattern += 1; + let mut new_timer = self.clock.make_timer(timeout_ms); + mem::swap(&mut self.blink_timer, &mut new_timer); + if self.clock().is_elapsed(&new_timer) { + blink_leds::(self.blink_pattern); + self.blink_pattern += 1; + } else { + mem::swap(&mut self.blink_timer, &mut new_timer); + } let result = UsbCtapHid::::recv_with_buttons(packet, Duration::from_ms(timeout_ms as isize)); @@ -323,6 +333,7 @@ where fn check_complete(&mut self) { switch_off_leds::(); + self.blink_timer = as Clock>::Timer::default(); } } From f4d3969021f09b32d92a41b6ae450b27ece60012 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Thu, 5 Sep 2024 14:53:27 +0200 Subject: [PATCH 4/4] Better TestClock API --- libraries/opensk/src/ctap/mod.rs | 5 ++--- libraries/opensk/src/env/test/mod.rs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/libraries/opensk/src/ctap/mod.rs b/libraries/opensk/src/ctap/mod.rs index 6146e542..8ef166ad 100644 --- a/libraries/opensk/src/ctap/mod.rs +++ b/libraries/opensk/src/ctap/mod.rs @@ -3367,10 +3367,9 @@ mod test { #[test] fn test_check_user_presence_timeout() { let mut env = TestEnv::default(); - let now_ms = env.clock().access(); + let clock = env.clock().clone(); env.user_presence().set(move || { - let mut locked_now_ms = now_ms.lock().unwrap(); - *locked_now_ms += 100; + clock.advance(100); Err(UserPresenceError::Timeout) }); let response = check_user_presence(&mut env, DUMMY_CHANNEL); diff --git a/libraries/opensk/src/env/test/mod.rs b/libraries/opensk/src/env/test/mod.rs index 33bc9763..75b072db 100644 --- a/libraries/opensk/src/env/test/mod.rs +++ b/libraries/opensk/src/env/test/mod.rs @@ -48,20 +48,20 @@ pub struct TestTimer { end_ms: usize, } -#[derive(Debug, Default)] +#[derive(Clone, Debug, Default)] pub struct TestClock { /// The current time, as advanced, in milliseconds. now_ms: Arc>, } impl TestClock { - pub fn advance(&mut self, milliseconds: usize) { - let mut locked_now_ms = self.now_ms.lock().unwrap(); - *locked_now_ms += milliseconds; + pub fn now(&self) -> usize { + *self.now_ms.lock().unwrap() } - pub fn access(&self) -> Arc> { - self.now_ms.clone() + pub fn advance(&self, milliseconds: usize) { + let mut locked_now_ms = self.now_ms.lock().unwrap(); + *locked_now_ms += milliseconds; } } @@ -70,18 +70,18 @@ impl Clock for TestClock { fn make_timer(&mut self, milliseconds: usize) -> Self::Timer { TestTimer { - end_ms: *self.now_ms.lock().unwrap() + milliseconds, + end_ms: self.now() + milliseconds, } } fn is_elapsed(&mut self, timer: &Self::Timer) -> bool { - *self.now_ms.lock().unwrap() >= timer.end_ms + self.now() >= timer.end_ms } #[cfg(feature = "debug_ctap")] fn timestamp_us(&mut self) -> usize { // Unused, but let's implement something because it's easy. - *self.now_ms.lock().unwrap() * 1000 + self.now() * 1000 } }