From dcee4eed35d5f85c24fcf02f7a7cbedc3d04c317 Mon Sep 17 00:00:00 2001 From: "John M. Schanck" Date: Tue, 22 Aug 2023 10:29:52 -0700 Subject: [PATCH 1/5] Separate StateMachine::init_and_select into init_device and maybe_select_device --- src/statemachine.rs | 140 +++++++++++++++++++++++++++++--------------- 1 file changed, 93 insertions(+), 47 deletions(-) diff --git a/src/statemachine.rs b/src/statemachine.rs index e1594b64..75be7cdf 100644 --- a/src/statemachine.rs +++ b/src/statemachine.rs @@ -5,6 +5,7 @@ use crate::authenticatorservice::{RegisterArgs, SignArgs}; use crate::consts::PARAMETER_SIZE; +use crate::ctap2; use crate::ctap2::client_data::ClientDataHash; use crate::ctap2::commands::client_pin::Pin; use crate::ctap2::commands::get_assertion::GetAssertionResult; @@ -22,7 +23,6 @@ use crate::transport::device_selector::{ use crate::transport::platform::transaction::Transaction; use crate::transport::{hid::HIDDevice, FidoDevice, FidoProtocol}; use crate::u2fprotocol::{u2f_init_device, u2f_is_keyhandle_valid, u2f_register, u2f_sign}; -use crate::ctap2; use crate::{ AuthenticatorTransports, InteractiveRequest, KeyHandle, ManageResult, RegisterFlags, SignFlags, }; @@ -69,19 +69,16 @@ impl StateMachine { Default::default() } - fn init_and_select( + fn init_device( info: DeviceBuildParameters, selector: &Sender, - status: &Sender, - ctap2_only: bool, - keep_alive: &dyn Fn() -> bool, ) -> Option { // Create a new device. let mut dev = match Device::new(info) { Ok(dev) => dev, Err((e, id)) => { info!("error happened with device: {}", e); - selector.send(DeviceSelectorEvent::NotAToken(id)).ok()?; + let _ = selector.send(DeviceSelectorEvent::NotAToken(id)); return None; } }; @@ -89,20 +86,29 @@ impl StateMachine { // Try initializing it. if let Err(e) = dev.init() { warn!("error while initializing device: {}", e); - selector.send(DeviceSelectorEvent::NotAToken(dev.id())).ok(); + let _ = selector.send(DeviceSelectorEvent::NotAToken(dev.id())); return None; } - if ctap2_only && dev.get_protocol() != FidoProtocol::CTAP2 { - info!("Device does not support CTAP2"); - selector.send(DeviceSelectorEvent::NotAToken(dev.id())).ok(); - return None; - } + Some(dev) + } + fn wait_for_device_selector( + dev: &mut Device, + selector: &Sender, + status: &Sender, + auto_select: bool, + keep_alive: &dyn Fn() -> bool, + ) -> bool { let (tx, rx) = channel(); - selector + if selector .send(DeviceSelectorEvent::ImAToken((dev.id(), tx))) - .ok()?; + .is_err() + { + // If we fail to register with the device selector, then we're not going + // to be selected. + return false; + } // We can be cancelled from the user (through keep_alive()) or from the device selector // (through a DeviceCommand::Cancel on rx). We'll combine those signals into a single @@ -111,42 +117,49 @@ impl StateMachine { // Blocking recv. DeviceSelector will tell us what to do match rx.recv() { + Ok(DeviceCommand::Blink) if auto_select => { + // The caller knows it wants to select this device. Now that we've received + // a blink command from the device selector, we can request to be selected. + // We may be racing several devices here. + selector + .send(DeviceSelectorEvent::SelectedToken(dev.id())) + .is_ok() + } Ok(DeviceCommand::Blink) => { - // Inform the user that there are multiple devices available. - // NOTE: We'll send this once per device, so the recipient should be prepared - // to receive this message multiple times. + // The caller wants the user to choose a device. Send a status update and blink + // this device. NOTE: We send one status update per device, so the recipient should be + // prepared to receive the message multiple times. send_status(status, crate::StatusUpdate::SelectDeviceNotice); match dev.block_and_blink(&keep_blinking) { BlinkResult::DeviceSelected => { // User selected us. Let DeviceSelector know, so it can cancel all other - // outstanding open blink-requests. + // outstanding open blink-requests. If we fail to send the SelectedToken + // message to the device selector, then don't consider this token as having + // been selected. selector .send(DeviceSelectorEvent::SelectedToken(dev.id())) - .ok()?; + .is_ok() } BlinkResult::Cancelled => { info!("Device {:?} was not selected", dev.id()); - return None; + false } } } Ok(DeviceCommand::Cancel) => { info!("Device {:?} was not selected", dev.id()); - return None; + false } Ok(DeviceCommand::Removed) => { info!("Device {:?} was removed", dev.id()); - return None; - } - Ok(DeviceCommand::Continue) => { - // Just continue + false } + Ok(DeviceCommand::Continue) => true, Err(_) => { warn!("Error when trying to receive messages from DeviceSelector! Exiting."); - return None; + false } } - Some(dev) } pub fn register( @@ -198,11 +211,12 @@ impl StateMachine { cbc.clone(), status, move |info, selector, status, alive| { - let mut dev = match Self::init_and_select(info, &selector, &status, false, alive) { - None => { - return; - } + let mut dev = match Self::init_device(info, &selector) { Some(dev) => dev, + None => return, + }; + if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + return; }; info!("Device {:?} continues with the register process", dev.id()); @@ -275,11 +289,19 @@ impl StateMachine { callback.clone(), status, move |info, selector, status, alive| { - let mut dev = match Self::init_and_select(info, &selector, &status, false, alive) { - None => { - return; - } + let mut dev = match Self::init_device(info, &selector) { Some(dev) => dev, + None => return, + }; + + if !Self::wait_for_device_selector( + &mut dev, + &selector, + &status, + auto_select, + alive, + ) { + return; }; info!("Device {:?} continues with the signing process", dev.id()); @@ -319,11 +341,19 @@ impl StateMachine { callback.clone(), status, move |info, selector, status, alive| { - let mut dev = match Self::init_and_select(info, &selector, &status, true, alive) { - None => { - return; - } + let mut dev = match Self::init_device(info, &selector) { Some(dev) => dev, + None => return, + }; + + if dev.get_protocol() != FidoProtocol::CTAP2 { + info!("Device does not support CTAP2"); + let _ = selector.send(DeviceSelectorEvent::NotAToken(dev.id())); + return; + } + + if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + return; }; ctap2::reset_helper(&mut dev, selector, status, callback.clone(), alive); }, @@ -349,11 +379,19 @@ impl StateMachine { callback.clone(), status, move |info, selector, status, alive| { - let mut dev = match Self::init_and_select(info, &selector, &status, true, alive) { - None => { - return; - } + let mut dev = match Self::init_device(info, &selector) { Some(dev) => dev, + None => return, + }; + + if dev.get_protocol() != FidoProtocol::CTAP2 { + info!("Device does not support CTAP2"); + let _ = selector.send(DeviceSelectorEvent::NotAToken(dev.id())); + return; + } + + if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + return; }; ctap2::set_or_change_pin_helper( @@ -607,11 +645,19 @@ impl StateMachine { callback.clone(), status, move |info, selector, status, alive| { - let mut dev = match Self::init_and_select(info, &selector, &status, true, alive) { - None => { - return; - } + let mut dev = match Self::init_device(info, &selector) { Some(dev) => dev, + None => return, + }; + + if dev.get_protocol() != FidoProtocol::CTAP2 { + info!("Device does not support CTAP2"); + let _ = selector.send(DeviceSelectorEvent::NotAToken(dev.id())); + return; + } + + if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + return; }; info!("Device {:?} selected for interactive management.", dev.id()); From f038907cf84a11d404b006eeffd1a7623c73a75c Mon Sep 17 00:00:00 2001 From: "John M. Schanck" Date: Tue, 22 Aug 2023 16:28:24 -0700 Subject: [PATCH 2/5] Skip device selection when useful credentials are discovered silently --- examples/ctap2.rs | 53 ++++++++++++++++++----------- src/ctap2/commands/get_assertion.rs | 14 +++----- src/ctap2/preflight.rs | 31 +++++++++++------ src/ctap2/server.rs | 9 +++++ src/statemachine.rs | 36 ++++++++++++++++---- 5 files changed, 96 insertions(+), 47 deletions(-) diff --git a/examples/ctap2.rs b/examples/ctap2.rs index b9d6aa73..65f87fa9 100644 --- a/examples/ctap2.rs +++ b/examples/ctap2.rs @@ -10,13 +10,13 @@ use authenticator::{ crypto::COSEAlgorithm, ctap2::server::{ PublicKeyCredentialDescriptor, PublicKeyCredentialParameters, RelyingParty, - ResidentKeyRequirement, Transport, User, UserVerificationRequirement, + RelyingPartyWrapper, ResidentKeyRequirement, Transport, User, UserVerificationRequirement, }, statecallback::StateCallback, Pin, StatusPinUv, StatusUpdate, }; use getopts::Options; -use sha2::{Digest, Sha256}; +use rand::{thread_rng, RngCore}; use std::sync::mpsc::{channel, RecvError}; use std::{env, thread}; @@ -31,6 +31,9 @@ fn main() { let args: Vec = env::args().collect(); let program = args[0].clone(); + let rp_id = "example.com".to_string(); + let app_id = "https://fido.example.com/myApp".to_string(); + let mut opts = Options::new(); opts.optflag("x", "no-u2f-usb-hid", "do not enable u2f-usb-hid platforms"); opts.optflag("h", "help", "print this help menu").optopt( @@ -39,6 +42,11 @@ fn main() { "timeout in seconds", "SEC", ); + opts.optflag( + "a", + "app_id", + &format!("Using App ID {app_id} from origin 'https://{rp_id}'"), + ); opts.optflag("s", "hmac_secret", "With hmac-secret"); opts.optflag("h", "help", "print this help menu"); opts.optflag("f", "fallback", "Use CTAP1 fallback implementation"); @@ -73,14 +81,8 @@ fn main() { }; println!("Asking a security key to register now..."); - let challenge_str = format!( - "{}{}", - r#"{"challenge": "1vQ9mxionq0ngCnjD-wTsv1zUSrGRtFqG2xP09SbZ70","#, - r#" "version": "U2F_V2", "appId": "http://example.com"}"# - ); - let mut challenge = Sha256::new(); - challenge.update(challenge_str.as_bytes()); - let chall_bytes: [u8; 32] = challenge.finalize().into(); + let mut chall_bytes = [0u8; 32]; + thread_rng().fill_bytes(&mut chall_bytes); let (status_tx, status_rx) = channel::(); thread::spawn(move || loop { @@ -146,14 +148,19 @@ fn main() { name: Some("A. User".to_string()), display_name: None, }; - let origin = "https://example.com".to_string(); + // If we're testing AppID support, then register with an RP ID that isn't valid for the origin. + let relying_party = RelyingParty { + id: if matches.opt_present("app_id") { + app_id.clone() + } else { + rp_id.clone() + }, + name: None, + }; let ctap_args = RegisterArgs { client_data_hash: chall_bytes, - relying_party: RelyingParty { - id: "example.com".to_string(), - name: None, - }, - origin: origin.clone(), + relying_party, + origin: format!("https://{rp_id}"), user, pub_cred_params: vec![ PublicKeyCredentialParameters { @@ -226,10 +233,11 @@ fn main() { allow_list = Vec::new(); } + let alternate_rp_id = matches.opt_present("app_id").then(|| app_id.clone()); let ctap_args = SignArgs { client_data_hash: chall_bytes, - origin, - relying_party_id: "example.com".to_string(), + origin: format!("https://{rp_id}"), + relying_party_id: rp_id.clone(), allow_list, user_verification_req: UserVerificationRequirement::Preferred, user_presence_req: true, @@ -248,7 +256,7 @@ fn main() { }, }, pin: None, - alternate_rp_id: None, + alternate_rp_id: alternate_rp_id.clone(), use_ctap1_fallback: fallback, }; @@ -270,6 +278,13 @@ fn main() { match sign_result { Ok(assertion_object) => { println!("Assertion Object: {assertion_object:?}"); + if let Some(alt_rp_id) = alternate_rp_id { + assert_eq!( + assertion_object.0[0].auth_data.rp_id_hash, + RelyingPartyWrapper::from(alt_rp_id.as_str()).hash() + ); + println!("Used AppID"); + } println!("Done."); break; } diff --git a/src/ctap2/commands/get_assertion.rs b/src/ctap2/commands/get_assertion.rs index 754cb670..fef7c4fe 100644 --- a/src/ctap2/commands/get_assertion.rs +++ b/src/ctap2/commands/get_assertion.rs @@ -1155,7 +1155,7 @@ pub mod test { // Sending first GetAssertion with first allow_list-entry, that will return an error let mut msg = cid.to_vec(); - msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x94]); + msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x90]); msg.extend(vec![0x2]); // u2f command msg.extend(vec![ 0xa4, // map(4) @@ -1191,10 +1191,7 @@ pub mod test { 0x6a, // text(10) 0x70, 0x75, 0x62, 0x6C, 0x69, 0x63, 0x2D, 0x6B, 0x65, 0x79, // public-key 0x5, // options - 0xa2, // map(2) - 0x62, // text(2) - 0x75, 0x76, // uv - 0xf4, // false + 0xa1, // map(1) 0x62, // text(2) 0x75, 0x70, // up 0xf4, // false @@ -1210,7 +1207,7 @@ pub mod test { // Sending second GetAssertion with first allow_list-entry, that will return a success let mut msg = cid.to_vec(); - msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x94]); + msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x90]); msg.extend(vec![0x2]); // u2f command msg.extend(vec![ 0xa4, // map(4) @@ -1246,10 +1243,7 @@ pub mod test { 0x6a, // text(10) 0x70, 0x75, 0x62, 0x6C, 0x69, 0x63, 0x2D, 0x6B, 0x65, 0x79, // public-key 0x5, // options - 0xa2, // map(2) - 0x62, // text(2) - 0x75, 0x76, // uv - 0xf4, // false + 0xa1, // map(1) 0x62, // text(2) 0x75, 0x70, // up 0xf4, // false diff --git a/src/ctap2/preflight.rs b/src/ctap2/preflight.rs index a7011ebf..3b3321b8 100644 --- a/src/ctap2/preflight.rs +++ b/src/ctap2/preflight.rs @@ -7,7 +7,7 @@ use crate::crypto::PinUvAuthToken; use crate::ctap2::server::{PublicKeyCredentialDescriptor, RelyingPartyWrapper}; use crate::errors::AuthenticatorError; use crate::transport::errors::{ApduErrorStatus, HIDError}; -use crate::transport::{FidoDevice, VirtualFidoDevice}; +use crate::transport::{FidoDevice, FidoProtocol, VirtualFidoDevice}; use crate::u2ftypes::CTAP1RequestAPDU; use sha2::{Digest, Sha256}; @@ -144,11 +144,6 @@ pub(crate) fn do_credential_list_filtering_ctap2( } } - // Step 1.2: Return early, if we only have one chunk anyways - if cred_list.len() <= chunk_size { - return Ok(cred_list); - } - let chunked_list = cred_list.chunks(chunk_size); // Step 2: If we have more than one chunk: Loop over all, doing GetAssertion @@ -160,11 +155,7 @@ pub(crate) fn do_credential_list_filtering_ctap2( rp.clone(), chunk.to_vec(), GetAssertionOptions { - user_verification: if pin_uv_auth_token.is_some() { - None - } else { - Some(false) - }, + user_verification: None, // defaults to Some(false) if puap is absent user_presence: Some(false), }, GetAssertionExtensions::default(), @@ -200,3 +191,21 @@ pub(crate) fn do_credential_list_filtering_ctap2( // MakeCredential or a Success in case of GetAssertion Ok(final_list) } + +pub(crate) fn silently_discover_credentials( + dev: &mut Dev, + cred_list: &[PublicKeyCredentialDescriptor], + rp: &RelyingPartyWrapper, + client_data_hash: &ClientDataHash, +) -> Vec { + if dev.get_protocol() == FidoProtocol::CTAP2 { + if let Ok(cred_list) = do_credential_list_filtering_ctap2(dev, cred_list, rp, None) { + return cred_list; + } + } else if let Some(key_handle) = + do_credential_list_filtering_ctap1(dev, cred_list, rp, client_data_hash) + { + return vec![key_handle]; + } + vec![] +} diff --git a/src/ctap2/server.rs b/src/ctap2/server.rs index 4f683d12..7d56ce9f 100644 --- a/src/ctap2/server.rs +++ b/src/ctap2/server.rs @@ -60,6 +60,15 @@ pub enum RelyingPartyWrapper { Hash(RpIdHash), } +impl From<&str> for RelyingPartyWrapper { + fn from(rp_id: &str) -> Self { + Self::Data(RelyingParty { + id: rp_id.to_string(), + name: None, + }) + } +} + impl RelyingPartyWrapper { pub fn hash(&self) -> RpIdHash { match *self { diff --git a/src/statemachine.rs b/src/statemachine.rs index 75be7cdf..e945c4a5 100644 --- a/src/statemachine.rs +++ b/src/statemachine.rs @@ -10,6 +10,7 @@ use crate::ctap2::client_data::ClientDataHash; use crate::ctap2::commands::client_pin::Pin; use crate::ctap2::commands::get_assertion::GetAssertionResult; use crate::ctap2::commands::make_credentials::MakeCredentialsResult; +use crate::ctap2::preflight::silently_discover_credentials; use crate::ctap2::server::{ PublicKeyCredentialDescriptor, RelyingParty, RelyingPartyWrapper, ResidentKeyRequirement, RpIdHash, UserVerificationRequirement, @@ -294,19 +295,40 @@ impl StateMachine { None => return, }; - if !Self::wait_for_device_selector( + let args = args.clone(); + + // Attempt to silently discover credentials. If we find useful credentials, then we + // can potentially skip a device selection tap. We may find acceptable credentials + // on several devices. In that case, the first device to ask for automatic + // selection will be selected. + let silent_creds = silently_discover_credentials( &mut dev, - &selector, - &status, - auto_select, - alive, - ) { + &args.allow_list, + &RelyingPartyWrapper::from(args.relying_party_id.as_str()), + &ClientDataHash(args.client_data_hash), + ); + let mut auto_select = !silent_creds.is_empty(); + if !auto_select && !args.allow_list.is_empty() { + // Check if we have (U2F / server-side) credentials bound to the alternate RP ID. + if let Some(ref alt_rp_id) = args.alternate_rp_id { + let silent_creds = silently_discover_credentials( + &mut dev, + &args.allow_list, + &RelyingPartyWrapper::from(alt_rp_id.as_str()), + &ClientDataHash(args.client_data_hash), + ); + auto_select = !silent_creds.is_empty(); + } + } + + if !Self::wait_for_device_selector(&mut dev, &selector, &status, auto_select, alive) + { return; }; info!("Device {:?} continues with the signing process", dev.id()); - if ctap2::sign(&mut dev, args.clone(), status, callback.clone(), alive) { + if ctap2::sign(&mut dev, args, status, callback.clone(), alive) { // ctap2::sign returns true if it called the callback with Ok(..). Cancel all // other tokens in case we skipped the "blinking"-action and went straight for // the actual request. From 4adb695b69007a4c46edc79a1e932e3eb33cc6c5 Mon Sep 17 00:00:00 2001 From: "John M. Schanck" Date: Tue, 22 Aug 2023 20:17:52 -0700 Subject: [PATCH 3/5] Improve CTAP2 support of AppID extension --- examples/ctap2.rs | 2 +- examples/interactive_management.rs | 2 +- src/ctap2/commands/get_assertion.rs | 9 ------ src/ctap2/mod.rs | 47 ++++++++++++++++------------- src/ctap2/preflight.rs | 1 - src/lib.rs | 2 +- 6 files changed, 29 insertions(+), 34 deletions(-) diff --git a/examples/ctap2.rs b/examples/ctap2.rs index 65f87fa9..b47bb269 100644 --- a/examples/ctap2.rs +++ b/examples/ctap2.rs @@ -237,7 +237,7 @@ fn main() { let ctap_args = SignArgs { client_data_hash: chall_bytes, origin: format!("https://{rp_id}"), - relying_party_id: rp_id.clone(), + relying_party_id: rp_id, allow_list, user_verification_req: UserVerificationRequirement::Preferred, user_presence_req: true, diff --git a/examples/interactive_management.rs b/examples/interactive_management.rs index 434ee377..98ac54b4 100644 --- a/examples/interactive_management.rs +++ b/examples/interactive_management.rs @@ -472,7 +472,7 @@ fn ask_user_bio_options( .expect("error: unable to read user input"); let name = input.trim().to_string(); tx.send(InteractiveRequest::BioEnrollment( - BioEnrollmentCmd::ChangeName(chosen_id, name.clone()), + BioEnrollmentCmd::ChangeName(chosen_id, name), puat, )) .expect("Failed to send GetEnrollments request."); diff --git a/src/ctap2/commands/get_assertion.rs b/src/ctap2/commands/get_assertion.rs index fef7c4fe..cbcac1b1 100644 --- a/src/ctap2/commands/get_assertion.rs +++ b/src/ctap2/commands/get_assertion.rs @@ -168,9 +168,6 @@ pub struct GetAssertion { pub extensions: GetAssertionExtensions, pub options: GetAssertionOptions, pub pin_uv_auth_param: Option, - - // This is used to implement the FIDO AppID extension. - pub alternate_rp_id: Option, } impl GetAssertion { @@ -180,7 +177,6 @@ impl GetAssertion { allow_list: Vec, options: GetAssertionOptions, extensions: GetAssertionExtensions, - alternate_rp_id: Option, ) -> Self { Self { client_data_hash, @@ -189,7 +185,6 @@ impl GetAssertion { extensions, options, pin_uv_auth_param: None, - alternate_rp_id, } } } @@ -636,7 +631,6 @@ pub mod test { user_verification: None, }, Default::default(), - None, ); let mut device = Device::new("commands/get_assertion").unwrap(); assert_eq!(device.get_protocol(), FidoProtocol::CTAP2); @@ -835,7 +829,6 @@ pub mod test { user_verification: None, }, Default::default(), - None, ); let mut device = Device::new("commands/get_assertion").unwrap(); // not really used (all functions ignore it) // channel id @@ -925,7 +918,6 @@ pub mod test { user_verification: None, }, Default::default(), - None, ); let mut device = Device::new("commands/get_assertion").unwrap(); // not really used (all functions ignore it) @@ -1106,7 +1098,6 @@ pub mod test { user_verification: None, }, Default::default(), - None, ); let mut device = Device::new("commands/get_assertion").unwrap(); assert_eq!(device.get_protocol(), FidoProtocol::CTAP2); diff --git a/src/ctap2/mod.rs b/src/ctap2/mod.rs index 5e32b76c..50a82998 100644 --- a/src/ctap2/mod.rs +++ b/src/ctap2/mod.rs @@ -32,9 +32,10 @@ use crate::ctap2::commands::{ }; use crate::ctap2::preflight::{ do_credential_list_filtering_ctap1, do_credential_list_filtering_ctap2, + silently_discover_credentials, }; use crate::ctap2::server::{ - RelyingParty, RelyingPartyWrapper, ResidentKeyRequirement, UserVerificationRequirement, + RelyingPartyWrapper, ResidentKeyRequirement, UserVerificationRequirement, }; use crate::errors::{AuthenticatorError, UnsupportedOption}; use crate::statecallback::StateCallback; @@ -531,19 +532,35 @@ pub fn sign( } } + let mut allow_list = args.allow_list; + let mut rp_id = args.relying_party_id; + let client_data_hash = ClientDataHash(args.client_data_hash); + if let Some(alt_rp_id) = args.alternate_rp_id { + if !allow_list.is_empty() { + // Try to silently discover U2F credentials that require the FIDO App ID extension. If + // any are found, we should use the alternate RP ID instead of the provided RP ID. + let silent_creds = silently_discover_credentials( + dev, + &allow_list, + &RelyingPartyWrapper::from(alt_rp_id.as_str()), + &client_data_hash, + ); + if !silent_creds.is_empty() { + allow_list = silent_creds; + rp_id = alt_rp_id; + } + } + } + let mut get_assertion = GetAssertion::new( - ClientDataHash(args.client_data_hash), - RelyingPartyWrapper::Data(RelyingParty { - id: args.relying_party_id, - name: None, - }), - args.allow_list, + client_data_hash, + RelyingPartyWrapper::from(rp_id.as_str()), + allow_list, GetAssertionOptions { user_presence: Some(args.user_presence_req), user_verification: None, }, args.extensions, - args.alternate_rp_id, ); let mut skip_uv = false; @@ -622,19 +639,7 @@ pub fn sign( debug!("{get_assertion:?} using {pin_uv_auth_result:?}"); debug!("------------------------------------------------------------------"); send_status(&status, crate::StatusUpdate::PresenceRequired); - let mut resp = dev.send_msg_cancellable(&get_assertion, alive); - if resp.is_err() { - // Retry with a different RP ID if one was supplied. This is intended to be - // used with the AppID provided in the WebAuthn FIDO AppID extension. - if let Some(alternate_rp_id) = get_assertion.alternate_rp_id { - get_assertion.rp = RelyingPartyWrapper::Data(RelyingParty { - id: alternate_rp_id, - ..Default::default() - }); - get_assertion.alternate_rp_id = None; - resp = dev.send_msg_cancellable(&get_assertion, alive); - } - } + let resp = dev.send_msg_cancellable(&get_assertion, alive); match resp { Ok(result) => { callback.call(Ok(result)); diff --git a/src/ctap2/preflight.rs b/src/ctap2/preflight.rs index 3b3321b8..bff3bc8c 100644 --- a/src/ctap2/preflight.rs +++ b/src/ctap2/preflight.rs @@ -159,7 +159,6 @@ pub(crate) fn do_credential_list_filtering_ctap2( user_presence: Some(false), }, GetAssertionExtensions::default(), - None, ); silent_assert.set_pin_uv_auth_param(pin_uv_auth_token.clone())?; let res = dev.send_msg(&silent_assert); diff --git a/src/lib.rs b/src/lib.rs index 2d4fe9a0..f35ecc4f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -46,8 +46,8 @@ pub use ctap2::commands::client_pin::{Pin, PinError}; pub use ctap2::commands::credential_management::CredentialManagementResult; pub use ctap2::commands::get_assertion::{Assertion, GetAssertionResult}; pub use ctap2::commands::get_info::AuthenticatorInfo; -use serde::Serialize; pub use ctap2::commands::make_credentials::MakeCredentialsResult; +use serde::Serialize; pub use statemachine::StateMachine; pub use status_update::{ BioEnrollmentCmd, CredManagementCmd, InteractiveRequest, InteractiveUpdate, StatusPinUv, From 1b9d0e0436ce2540810bacd9a076a430054b6963 Mon Sep 17 00:00:00 2001 From: "John M. Schanck" Date: Wed, 23 Aug 2023 09:52:42 -0700 Subject: [PATCH 4/5] Replace `!auto_select` with `silent_creds.is_empty()` --- src/statemachine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/statemachine.rs b/src/statemachine.rs index e945c4a5..a50798ca 100644 --- a/src/statemachine.rs +++ b/src/statemachine.rs @@ -308,7 +308,7 @@ impl StateMachine { &ClientDataHash(args.client_data_hash), ); let mut auto_select = !silent_creds.is_empty(); - if !auto_select && !args.allow_list.is_empty() { + if silent_creds.is_empty() && !args.allow_list.is_empty() { // Check if we have (U2F / server-side) credentials bound to the alternate RP ID. if let Some(ref alt_rp_id) = args.alternate_rp_id { let silent_creds = silently_discover_credentials( From 822d7ee6f416654239512ed3730a8c425e34ad9d Mon Sep 17 00:00:00 2001 From: "John M. Schanck" Date: Thu, 24 Aug 2023 09:06:50 -0700 Subject: [PATCH 5/5] backout auto_select feature --- src/statemachine.rs | 50 ++++++--------------------------------------- 1 file changed, 6 insertions(+), 44 deletions(-) diff --git a/src/statemachine.rs b/src/statemachine.rs index a50798ca..e1c430f0 100644 --- a/src/statemachine.rs +++ b/src/statemachine.rs @@ -10,7 +10,6 @@ use crate::ctap2::client_data::ClientDataHash; use crate::ctap2::commands::client_pin::Pin; use crate::ctap2::commands::get_assertion::GetAssertionResult; use crate::ctap2::commands::make_credentials::MakeCredentialsResult; -use crate::ctap2::preflight::silently_discover_credentials; use crate::ctap2::server::{ PublicKeyCredentialDescriptor, RelyingParty, RelyingPartyWrapper, ResidentKeyRequirement, RpIdHash, UserVerificationRequirement, @@ -98,7 +97,6 @@ impl StateMachine { dev: &mut Device, selector: &Sender, status: &Sender, - auto_select: bool, keep_alive: &dyn Fn() -> bool, ) -> bool { let (tx, rx) = channel(); @@ -118,14 +116,6 @@ impl StateMachine { // Blocking recv. DeviceSelector will tell us what to do match rx.recv() { - Ok(DeviceCommand::Blink) if auto_select => { - // The caller knows it wants to select this device. Now that we've received - // a blink command from the device selector, we can request to be selected. - // We may be racing several devices here. - selector - .send(DeviceSelectorEvent::SelectedToken(dev.id())) - .is_ok() - } Ok(DeviceCommand::Blink) => { // The caller wants the user to choose a device. Send a status update and blink // this device. NOTE: We send one status update per device, so the recipient should be @@ -216,7 +206,7 @@ impl StateMachine { Some(dev) => dev, None => return, }; - if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + if !Self::wait_for_device_selector(&mut dev, &selector, &status, alive) { return; }; @@ -294,41 +284,13 @@ impl StateMachine { Some(dev) => dev, None => return, }; - - let args = args.clone(); - - // Attempt to silently discover credentials. If we find useful credentials, then we - // can potentially skip a device selection tap. We may find acceptable credentials - // on several devices. In that case, the first device to ask for automatic - // selection will be selected. - let silent_creds = silently_discover_credentials( - &mut dev, - &args.allow_list, - &RelyingPartyWrapper::from(args.relying_party_id.as_str()), - &ClientDataHash(args.client_data_hash), - ); - let mut auto_select = !silent_creds.is_empty(); - if silent_creds.is_empty() && !args.allow_list.is_empty() { - // Check if we have (U2F / server-side) credentials bound to the alternate RP ID. - if let Some(ref alt_rp_id) = args.alternate_rp_id { - let silent_creds = silently_discover_credentials( - &mut dev, - &args.allow_list, - &RelyingPartyWrapper::from(alt_rp_id.as_str()), - &ClientDataHash(args.client_data_hash), - ); - auto_select = !silent_creds.is_empty(); - } - } - - if !Self::wait_for_device_selector(&mut dev, &selector, &status, auto_select, alive) - { + if !Self::wait_for_device_selector(&mut dev, &selector, &status, alive) { return; }; info!("Device {:?} continues with the signing process", dev.id()); - if ctap2::sign(&mut dev, args, status, callback.clone(), alive) { + if ctap2::sign(&mut dev, args.clone(), status, callback.clone(), alive) { // ctap2::sign returns true if it called the callback with Ok(..). Cancel all // other tokens in case we skipped the "blinking"-action and went straight for // the actual request. @@ -374,7 +336,7 @@ impl StateMachine { return; } - if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + if !Self::wait_for_device_selector(&mut dev, &selector, &status, alive) { return; }; ctap2::reset_helper(&mut dev, selector, status, callback.clone(), alive); @@ -412,7 +374,7 @@ impl StateMachine { return; } - if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + if !Self::wait_for_device_selector(&mut dev, &selector, &status, alive) { return; }; @@ -678,7 +640,7 @@ impl StateMachine { return; } - if !Self::wait_for_device_selector(&mut dev, &selector, &status, false, alive) { + if !Self::wait_for_device_selector(&mut dev, &selector, &status, alive) { return; };