From 18ebeebb3e4ddb3e376bc2ff56de5fc1042e5837 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Mon, 11 Jan 2021 11:51:01 +0100 Subject: [PATCH 1/3] adds storage changes for credential management --- src/ctap/mod.rs | 12 +++- src/ctap/response.rs | 19 ++++- src/ctap/storage.rs | 164 +++++++++++++++++++++++++++++++++++-------- 3 files changed, 163 insertions(+), 32 deletions(-) diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index fe60e80..4e93210 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -848,13 +848,19 @@ where CtapState::::PIN_PROTOCOL_VERSION, ]), max_credential_count_in_list: MAX_CREDENTIAL_COUNT_IN_LIST.map(|c| c as u64), - // #TODO(106) update with version 2.1 of HMAC-secret + // TODO(#106) update with version 2.1 of HMAC-secret max_credential_id_length: Some(CREDENTIAL_ID_SIZE as u64), transports: Some(vec![AuthenticatorTransport::Usb]), algorithms: Some(vec![ES256_CRED_PARAM]), default_cred_protect: DEFAULT_CRED_PROTECT, min_pin_length: self.persistent_store.min_pin_length()?, firmware_version: None, + max_cred_blob_length: None, + // TODO(kaczmarczyck) update when extension is implemented + max_rp_ids_for_set_min_pin_length: None, + remaining_discoverable_credentials: Some( + self.persistent_store.remaining_credentials()? as u64, + ), }, )) } @@ -1015,7 +1021,7 @@ mod test { let mut ctap_state = CtapState::new(&mut rng, user_immediately_present, DUMMY_CLOCK_VALUE); let info_reponse = ctap_state.process_command(&[0x04], DUMMY_CHANNEL_ID, DUMMY_CLOCK_VALUE); - let mut expected_response = vec![0x00, 0xAA, 0x01]; + let mut expected_response = vec![0x00, 0xAB, 0x01]; // The version array differs with CTAP1, always including 2.0 and 2.1. #[cfg(not(feature = "with_ctap1"))] let version_count = 2; @@ -1039,7 +1045,7 @@ mod test { 0x65, 0x6E, 0x74, 0x50, 0x69, 0x6E, 0xF4, 0x05, 0x19, 0x04, 0x00, 0x06, 0x81, 0x01, 0x08, 0x18, 0x70, 0x09, 0x81, 0x63, 0x75, 0x73, 0x62, 0x0A, 0x81, 0xA2, 0x63, 0x61, 0x6C, 0x67, 0x26, 0x64, 0x74, 0x79, 0x70, 0x65, 0x6A, 0x70, 0x75, 0x62, 0x6C, 0x69, - 0x63, 0x2D, 0x6B, 0x65, 0x79, 0x0D, 0x04, + 0x63, 0x2D, 0x6B, 0x65, 0x79, 0x0D, 0x04, 0x14, 0x18, 0x96, ] .iter(), ); diff --git a/src/ctap/response.rs b/src/ctap/response.rs index 390b0cb..3ebab3b 100644 --- a/src/ctap/response.rs +++ b/src/ctap/response.rs @@ -107,7 +107,6 @@ impl From for cbor::Value { #[cfg_attr(test, derive(PartialEq))] #[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))] pub struct AuthenticatorGetInfoResponse { - // TODO(kaczmarczyck) add maxAuthenticatorConfigLength and defaultCredProtect pub versions: Vec, pub extensions: Option>, pub aaguid: [u8; 16], @@ -121,6 +120,9 @@ pub struct AuthenticatorGetInfoResponse { pub default_cred_protect: Option, pub min_pin_length: u8, pub firmware_version: Option, + pub max_cred_blob_length: Option, + pub max_rp_ids_for_set_min_pin_length: Option, + pub remaining_discoverable_credentials: Option, } impl From for cbor::Value { @@ -139,6 +141,9 @@ impl From for cbor::Value { default_cred_protect, min_pin_length, firmware_version, + max_cred_blob_length, + max_rp_ids_for_set_min_pin_length, + remaining_discoverable_credentials, } = get_info_response; let options_cbor: Option = options.map(|options| { @@ -163,6 +168,9 @@ impl From for cbor::Value { 0x0C => default_cred_protect.map(|p| p as u64), 0x0D => min_pin_length as u64, 0x0E => firmware_version, + 0x0F => max_cred_blob_length, + 0x10 => max_rp_ids_for_set_min_pin_length, + 0x14 => remaining_discoverable_credentials, } } } @@ -285,6 +293,9 @@ mod test { default_cred_protect: None, min_pin_length: 4, firmware_version: None, + max_cred_blob_length: None, + max_rp_ids_for_set_min_pin_length: None, + remaining_discoverable_credentials: None, }; let response_cbor: Option = ResponseData::AuthenticatorGetInfo(get_info_response).into(); @@ -314,6 +325,9 @@ mod test { default_cred_protect: Some(CredentialProtectionPolicy::UserVerificationRequired), min_pin_length: 4, firmware_version: Some(0), + max_cred_blob_length: Some(1024), + max_rp_ids_for_set_min_pin_length: Some(8), + remaining_discoverable_credentials: Some(150), }; let response_cbor: Option = ResponseData::AuthenticatorGetInfo(get_info_response).into(); @@ -331,6 +345,9 @@ mod test { 0x0C => CredentialProtectionPolicy::UserVerificationRequired as u64, 0x0D => 4, 0x0E => 0, + 0x0F => 1024, + 0x10 => 8, + 0x14 => 150, }; assert_eq!(response_cbor, Some(expected_cbor)); } diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 28c1599..76c2fd6 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -16,6 +16,7 @@ mod key; use crate::ctap::data_formats::{ extract_array, extract_text_string, CredentialProtectionPolicy, PublicKeyCredentialSource, + PublicKeyCredentialUserEntity, }; use crate::ctap::key_material; use crate::ctap::pin_protocol_v1::PIN_AUTH_LENGTH; @@ -116,6 +117,29 @@ impl PersistentStore { Ok(()) } + /// Finds the key and value for a given credential ID. + /// + /// # Errors + /// + /// Returns `CTAP2_ERR_NO_CREDENTIALS` if the credential is not found. + fn find_credential_item( + &self, + credential_id: &[u8], + ) -> Result<(usize, PublicKeyCredentialSource), Ctap2StatusCode> { + let mut iter_result = Ok(()); + let iter = self.iter_credentials(&mut iter_result)?; + let mut credentials: Vec<(usize, PublicKeyCredentialSource)> = iter + .filter(|(_, credential)| credential.credential_id == credential_id) + .collect(); + iter_result?; + if credentials.len() > 1 { + return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR); + } + credentials + .pop() + .ok_or(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS) + } + /// Returns the first matching credential. /// /// Returns `None` if no credentials are matched or if `check_cred_protect` is set and the first @@ -126,22 +150,17 @@ impl PersistentStore { credential_id: &[u8], check_cred_protect: bool, ) -> Result, Ctap2StatusCode> { - let mut iter_result = Ok(()); - let iter = self.iter_credentials(&mut iter_result)?; - // We don't check whether there is more than one matching credential to be able to exit - // early. - let result = iter.map(|(_, credential)| credential).find(|credential| { - credential.rp_id == rp_id && credential.credential_id == credential_id - }); - iter_result?; - if let Some(cred) = &result { - let user_verification_required = cred.cred_protect_policy - == Some(CredentialProtectionPolicy::UserVerificationRequired); - if check_cred_protect && user_verification_required { - return Ok(None); - } + let credential = match self.find_credential_item(credential_id) { + Err(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS) => return Ok(None), + Err(e) => return Err(e), + Ok(credential) => credential.1, + }; + let is_protected = credential.cred_protect_policy + == Some(CredentialProtectionPolicy::UserVerificationRequired); + if credential.rp_id != rp_id || (check_cred_protect && is_protected) { + return Ok(None); } - Ok(result) + Ok(Some(credential)) } /// Stores or updates a credential. @@ -196,6 +215,34 @@ impl PersistentStore { Ok(()) } + /// Deletes a credential. + /// + /// # Errors + /// + /// Returns `CTAP2_ERR_NO_CREDENTIALS` if the credential is not found. + pub fn _delete_credential(&mut self, credential_id: &[u8]) -> Result<(), Ctap2StatusCode> { + let (key, _) = self.find_credential_item(credential_id)?; + Ok(self.store.remove(key)?) + } + + /// Updates a credential's user information. + /// + /// # Errors + /// + /// Returns `CTAP2_ERR_NO_CREDENTIALS` if the credential is not found. + pub fn _update_credential( + &mut self, + credential_id: &[u8], + user: PublicKeyCredentialUserEntity, + ) -> Result<(), Ctap2StatusCode> { + let (key, mut credential) = self.find_credential_item(credential_id)?; + credential.user_name = user.user_name; + credential.user_display_name = user.user_display_name; + credential.user_icon = user.user_icon; + let value = serialize_credential(credential)?; + Ok(self.store.insert(key, &value)?) + } + /// Returns the list of matching credentials. /// /// Does not return credentials that are not discoverable if `check_cred_protect` is set. @@ -221,7 +268,6 @@ impl PersistentStore { } /// Returns the number of credentials. - #[cfg(test)] pub fn count_credentials(&self) -> Result { let mut iter_result = Ok(()); let iter = self.iter_credentials(&mut iter_result)?; @@ -230,10 +276,17 @@ impl PersistentStore { Ok(result) } + /// Returns the estimated number of credentials that can still be stored. + pub fn remaining_credentials(&self) -> Result { + MAX_SUPPORTED_RESIDENTIAL_KEYS + .checked_sub(self.count_credentials()?) + .ok_or(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR) + } + /// Iterates through the credentials. /// /// If an error is encountered during iteration, it is written to `result`. - fn iter_credentials<'a>( + pub fn iter_credentials<'a>( &'a self, result: &'a mut Result<(), Ctap2StatusCode>, ) -> Result, Ctap2StatusCode> { @@ -494,7 +547,7 @@ impl From for Ctap2StatusCode { } /// Iterator for credentials. -struct IterCredentials<'a> { +pub struct IterCredentials<'a> { /// The store being iterated. store: &'a persistent_store::Store, @@ -629,6 +682,66 @@ mod test { assert!(persistent_store.count_credentials().unwrap() > 0); } + #[test] + fn test_delete_credential() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + assert_eq!(persistent_store.count_credentials().unwrap(), 0); + + let mut credential_ids = vec![]; + for i in 0..MAX_SUPPORTED_RESIDENTIAL_KEYS { + let user_handle = i.to_ne_bytes().to_vec(); + let credential_source = create_credential_source(&mut rng, "example.com", user_handle); + credential_ids.push(credential_source.credential_id.clone()); + assert!(persistent_store.store_credential(credential_source).is_ok()); + assert_eq!(persistent_store.count_credentials().unwrap(), i + 1); + } + let mut count = persistent_store.count_credentials().unwrap(); + for credential_id in credential_ids { + assert!(persistent_store._delete_credential(&credential_id).is_ok()); + count -= 1; + assert_eq!(persistent_store.count_credentials().unwrap(), count); + } + } + + #[test] + fn test_update_credential() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + let user = PublicKeyCredentialUserEntity { + // User ID is ignored. + user_id: vec![0x00], + user_name: Some("name".to_string()), + user_display_name: Some("display_name".to_string()), + user_icon: Some("icon".to_string()), + }; + assert_eq!( + persistent_store._update_credential(&[0x1D], user.clone()), + Err(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS) + ); + + let credential_source = create_credential_source(&mut rng, "example.com", vec![0x1D]); + let credential_id = credential_source.credential_id.clone(); + assert!(persistent_store.store_credential(credential_source).is_ok()); + let stored_credential = persistent_store + .find_credential("example.com", &credential_id, false) + .unwrap() + .unwrap(); + assert_eq!(stored_credential.user_name, None); + assert_eq!(stored_credential.user_display_name, None); + assert_eq!(stored_credential.user_icon, None); + assert!(persistent_store + ._update_credential(&credential_id, user.clone()) + .is_ok()); + let stored_credential = persistent_store + .find_credential("example.com", &credential_id, false) + .unwrap() + .unwrap(); + assert_eq!(stored_credential.user_name, user.user_name); + assert_eq!(stored_credential.user_display_name, user.user_display_name); + assert_eq!(stored_credential.user_icon, user.user_icon); + } + #[test] fn test_credential_order() { let mut rng = ThreadRng256 {}; @@ -645,17 +758,14 @@ mod test { } #[test] - #[allow(clippy::assertions_on_constants)] fn test_fill_store() { let mut rng = ThreadRng256 {}; let mut persistent_store = PersistentStore::new(&mut rng); assert_eq!(persistent_store.count_credentials().unwrap(), 0); - // To make this test work for bigger storages, implement better int -> Vec conversion. - assert!(MAX_SUPPORTED_RESIDENTIAL_KEYS < 256); for i in 0..MAX_SUPPORTED_RESIDENTIAL_KEYS { - let credential_source = - create_credential_source(&mut rng, "example.com", vec![i as u8]); + let user_handle = i.to_ne_bytes().to_vec(); + let credential_source = create_credential_source(&mut rng, "example.com", user_handle); assert!(persistent_store.store_credential(credential_source).is_ok()); assert_eq!(persistent_store.count_credentials().unwrap(), i + 1); } @@ -675,7 +785,6 @@ mod test { } #[test] - #[allow(clippy::assertions_on_constants)] fn test_overwrite() { let mut rng = ThreadRng256 {}; let mut persistent_store = PersistentStore::new(&mut rng); @@ -699,11 +808,10 @@ mod test { &[expected_credential] ); - // To make this test work for bigger storages, implement better int -> Vec conversion. - assert!(MAX_SUPPORTED_RESIDENTIAL_KEYS < 256); + let mut persistent_store = PersistentStore::new(&mut rng); for i in 0..MAX_SUPPORTED_RESIDENTIAL_KEYS { - let credential_source = - create_credential_source(&mut rng, "example.com", vec![i as u8]); + let user_handle = i.to_ne_bytes().to_vec(); + let credential_source = create_credential_source(&mut rng, "example.com", user_handle); assert!(persistent_store.store_credential(credential_source).is_ok()); assert_eq!(persistent_store.count_credentials().unwrap(), i + 1); } From 4cee0c4c656374664426a8c7ed5d8221ceb9b983 Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Mon, 11 Jan 2021 14:31:13 +0100 Subject: [PATCH 2/3] only keeps keys instead of credentials as state --- src/ctap/mod.rs | 76 ++++++++++++++++++++++++++------------------- src/ctap/storage.rs | 64 ++++++++++++++++++++++++++------------ 2 files changed, 89 insertions(+), 51 deletions(-) diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 4e93210..5072a47 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -142,7 +142,7 @@ struct AssertionInput { struct AssertionState { assertion_input: AssertionInput, // Sorted by ascending order of creation, so the last element is the most recent one. - next_credentials: Vec, + next_credential_keys: Vec, } enum StatefulCommand { @@ -606,7 +606,7 @@ where // and returns the correct Get(Next)Assertion response. fn assertion_response( &mut self, - credential: PublicKeyCredentialSource, + mut credential: PublicKeyCredentialSource, assertion_input: AssertionInput, number_of_credentials: Option, ) -> Result { @@ -642,6 +642,12 @@ where key_id: credential.credential_id, transports: None, // You can set USB as a hint here. }; + // Remove user identifiable information without uv. + if !has_uv { + credential.user_name = None; + credential.user_display_name = None; + credential.user_icon = None; + } let user = if !credential.user_handle.is_empty() { Some(PublicKeyCredentialUserEntity { user_id: credential.user_handle, @@ -749,26 +755,23 @@ where } let rp_id_hash = Sha256::hash(rp_id.as_bytes()); - let mut applicable_credentials = if let Some(allow_list) = allow_list { - if let Some(credential) = - self.get_any_credential_from_allow_list(allow_list, &rp_id, &rp_id_hash, has_uv)? - { - vec![credential] - } else { - vec![] - } + let (credential, next_credential_keys) = if let Some(allow_list) = allow_list { + ( + self.get_any_credential_from_allow_list(allow_list, &rp_id, &rp_id_hash, has_uv)?, + vec![], + ) } else { - self.persistent_store.filter_credential(&rp_id, !has_uv)? + let mut stored_credentials = + self.persistent_store.filter_credentials(&rp_id, !has_uv)?; + stored_credentials.sort_unstable_by_key(|c| c.1); + let mut stored_credentials: Vec = + stored_credentials.into_iter().map(|c| c.0).collect(); + let credential = stored_credentials + .pop() + .map(|key| self.persistent_store.get_credential(key)) + .transpose()?; + (credential, stored_credentials) }; - // Remove user identifiable information without uv. - if !has_uv { - for credential in &mut applicable_credentials { - credential.user_name = None; - credential.user_display_name = None; - credential.user_icon = None; - } - } - applicable_credentials.sort_unstable_by_key(|c| c.creation_order); // This check comes before CTAP2_ERR_NO_CREDENTIALS in CTAP 2.0. // For CTAP 2.1, it was moved to a later protocol step. @@ -776,9 +779,7 @@ where (self.check_user_presence)(cid)?; } - let credential = applicable_credentials - .pop() - .ok_or(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS)?; + let credential = credential.ok_or(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS)?; self.increment_global_signature_counter()?; @@ -788,15 +789,15 @@ where hmac_secret_input, has_uv, }; - let number_of_credentials = if applicable_credentials.is_empty() { + let number_of_credentials = if next_credential_keys.is_empty() { None } else { - let number_of_credentials = Some(applicable_credentials.len() + 1); + let number_of_credentials = Some(next_credential_keys.len() + 1); self.stateful_command_permission = TimedPermission::granted(now, STATEFUL_COMMAND_TIMEOUT_DURATION); self.stateful_command_type = Some(StatefulCommand::GetAssertion(AssertionState { assertion_input: assertion_input.clone(), - next_credentials: applicable_credentials, + next_credential_keys, })); number_of_credentials }; @@ -812,10 +813,11 @@ where if let Some(StatefulCommand::GetAssertion(assertion_state)) = &mut self.stateful_command_type { - let credential = assertion_state - .next_credentials + let credential_key = assertion_state + .next_credential_keys .pop() .ok_or(Ctap2StatusCode::CTAP2_ERR_NOT_ALLOWED)?; + let credential = self.persistent_store.get_credential(credential_key)?; (assertion_state.assertion_input.clone(), credential) } else { return Err(Ctap2StatusCode::CTAP2_ERR_NOT_ALLOWED); @@ -1250,11 +1252,16 @@ mod test { ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID); assert!(make_credential_response.is_ok()); - let stored_credential = ctap_state + let credential_key = ctap_state .persistent_store - .filter_credential("example.com", false) + .filter_credentials("example.com", false) .unwrap() .pop() + .unwrap() + .0; + let stored_credential = ctap_state + .persistent_store + .get_credential(credential_key) .unwrap(); let credential_id = stored_credential.credential_id; assert_eq!(stored_credential.cred_protect_policy, Some(test_policy)); @@ -1275,11 +1282,16 @@ mod test { ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID); assert!(make_credential_response.is_ok()); - let stored_credential = ctap_state + let credential_key = ctap_state .persistent_store - .filter_credential("example.com", false) + .filter_credentials("example.com", false) .unwrap() .pop() + .unwrap() + .0; + let stored_credential = ctap_state + .persistent_store + .get_credential(credential_key) .unwrap(); let credential_id = stored_credential.credential_id; assert_eq!(stored_credential.cred_protect_policy, Some(test_policy)); diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 76c2fd6..343751a 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -117,6 +117,24 @@ impl PersistentStore { Ok(()) } + /// Returns the credential at the given key. + /// + /// # Errors + /// + /// Returns `CTAP2_ERR_VENDOR_INTERNAL_ERROR` if the key does not hold a valid credential. + pub fn get_credential(&self, key: usize) -> Result { + let min_key = key::CREDENTIALS.start; + if key < min_key || key >= min_key + MAX_SUPPORTED_RESIDENTIAL_KEYS { + return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR); + } + let credential_entry = self + .store + .find(key)? + .ok_or(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR)?; + deserialize_credential(&credential_entry) + .ok_or(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR) + } + /// Finds the key and value for a given credential ID. /// /// # Errors @@ -246,22 +264,23 @@ impl PersistentStore { /// Returns the list of matching credentials. /// /// Does not return credentials that are not discoverable if `check_cred_protect` is set. - pub fn filter_credential( + pub fn filter_credentials( &self, rp_id: &str, check_cred_protect: bool, - ) -> Result, Ctap2StatusCode> { + ) -> Result, Ctap2StatusCode> { let mut iter_result = Ok(()); let iter = self.iter_credentials(&mut iter_result)?; let result = iter - .filter_map(|(_, credential)| { - if credential.rp_id == rp_id { - Some(credential) + .filter_map(|(key, credential)| { + if credential.rp_id == rp_id + && (!check_cred_protect || credential.is_discoverable()) + { + Some((key, credential.creation_order)) } else { None } }) - .filter(|cred| !check_cred_protect || cred.is_discoverable()) .collect(); iter_result?; Ok(result) @@ -801,12 +820,13 @@ mod test { .store_credential(credential_source1) .is_ok()); assert_eq!(persistent_store.count_credentials().unwrap(), 1); - assert_eq!( - &persistent_store - .filter_credential("example.com", false) - .unwrap(), - &[expected_credential] - ); + let filtered_credentials = persistent_store + .filter_credentials("example.com", false) + .unwrap(); + let retrieved_credential_source = persistent_store + .get_credential(filtered_credentials[0].0) + .unwrap(); + assert_eq!(retrieved_credential_source, expected_credential); let mut persistent_store = PersistentStore::new(&mut rng); for i in 0..MAX_SUPPORTED_RESIDENTIAL_KEYS { @@ -831,7 +851,7 @@ mod test { } #[test] - fn test_filter() { + fn test_filter_get_credentials() { let mut rng = ThreadRng256 {}; let mut persistent_store = PersistentStore::new(&mut rng); assert_eq!(persistent_store.count_credentials().unwrap(), 0); @@ -852,14 +872,20 @@ mod test { .is_ok()); let filtered_credentials = persistent_store - .filter_credential("example.com", false) + .filter_credentials("example.com", false) .unwrap(); assert_eq!(filtered_credentials.len(), 2); + let retrieved_credential0 = persistent_store + .get_credential(filtered_credentials[0].0) + .unwrap(); + let retrieved_credential1 = persistent_store + .get_credential(filtered_credentials[1].0) + .unwrap(); assert!( - (filtered_credentials[0].credential_id == id0 - && filtered_credentials[1].credential_id == id1) - || (filtered_credentials[1].credential_id == id0 - && filtered_credentials[0].credential_id == id1) + (retrieved_credential0.credential_id == id0 + && retrieved_credential1.credential_id == id1) + || (retrieved_credential1.credential_id == id0 + && retrieved_credential0.credential_id == id1) ); } @@ -886,7 +912,7 @@ mod test { assert!(persistent_store.store_credential(credential).is_ok()); let no_credential = persistent_store - .filter_credential("example.com", true) + .filter_credentials("example.com", true) .unwrap(); assert_eq!(no_credential, vec![]); } From 27a7108328fcea9bba7bfe1b1411167e9a7551ef Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Tue, 12 Jan 2021 07:01:25 +0100 Subject: [PATCH 3/3] moves filter_credentials to call side --- src/ctap/mod.rs | 52 +++++++++++--------- src/ctap/storage.rs | 112 ++++++++------------------------------------ 2 files changed, 49 insertions(+), 115 deletions(-) diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 5072a47..2047500 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -761,11 +761,23 @@ where vec![], ) } else { - let mut stored_credentials = - self.persistent_store.filter_credentials(&rp_id, !has_uv)?; - stored_credentials.sort_unstable_by_key(|c| c.1); - let mut stored_credentials: Vec = - stored_credentials.into_iter().map(|c| c.0).collect(); + let mut iter_result = Ok(()); + let iter = self.persistent_store.iter_credentials(&mut iter_result)?; + let mut stored_credentials: Vec<(usize, u64)> = iter + .filter_map(|(key, credential)| { + if credential.rp_id == rp_id && (has_uv || credential.is_discoverable()) { + Some((key, credential.creation_order)) + } else { + None + } + }) + .collect(); + iter_result?; + stored_credentials.sort_unstable_by_key(|&(_key, order)| order); + let mut stored_credentials: Vec = stored_credentials + .into_iter() + .map(|(key, _order)| key) + .collect(); let credential = stored_credentials .pop() .map(|key| self.persistent_store.get_credential(key)) @@ -1252,17 +1264,14 @@ mod test { ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID); assert!(make_credential_response.is_ok()); - let credential_key = ctap_state + let mut iter_result = Ok(()); + let iter = ctap_state .persistent_store - .filter_credentials("example.com", false) - .unwrap() - .pop() - .unwrap() - .0; - let stored_credential = ctap_state - .persistent_store - .get_credential(credential_key) + .iter_credentials(&mut iter_result) .unwrap(); + // There is only 1 credential, so last is good enough. + let (_, stored_credential) = iter.last().unwrap(); + iter_result.unwrap(); let credential_id = stored_credential.credential_id; assert_eq!(stored_credential.cred_protect_policy, Some(test_policy)); @@ -1282,17 +1291,14 @@ mod test { ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID); assert!(make_credential_response.is_ok()); - let credential_key = ctap_state + let mut iter_result = Ok(()); + let iter = ctap_state .persistent_store - .filter_credentials("example.com", false) - .unwrap() - .pop() - .unwrap() - .0; - let stored_credential = ctap_state - .persistent_store - .get_credential(credential_key) + .iter_credentials(&mut iter_result) .unwrap(); + // There is only 1 credential, so last is good enough. + let (_, stored_credential) = iter.last().unwrap(); + iter_result.unwrap(); let credential_id = stored_credential.credential_id; assert_eq!(stored_credential.cred_protect_policy, Some(test_policy)); diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 343751a..8df987d 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -171,7 +171,7 @@ impl PersistentStore { let credential = match self.find_credential_item(credential_id) { Err(Ctap2StatusCode::CTAP2_ERR_NO_CREDENTIALS) => return Ok(None), Err(e) => return Err(e), - Ok(credential) => credential.1, + Ok((_key, credential)) => credential, }; let is_protected = credential.cred_protect_policy == Some(CredentialProtectionPolicy::UserVerificationRequired); @@ -261,31 +261,6 @@ impl PersistentStore { Ok(self.store.insert(key, &value)?) } - /// Returns the list of matching credentials. - /// - /// Does not return credentials that are not discoverable if `check_cred_protect` is set. - pub fn filter_credentials( - &self, - rp_id: &str, - check_cred_protect: bool, - ) -> Result, Ctap2StatusCode> { - let mut iter_result = Ok(()); - let iter = self.iter_credentials(&mut iter_result)?; - let result = iter - .filter_map(|(key, credential)| { - if credential.rp_id == rp_id - && (!check_cred_protect || credential.is_discoverable()) - { - Some((key, credential.creation_order)) - } else { - None - } - }) - .collect(); - iter_result?; - Ok(result) - } - /// Returns the number of credentials. pub fn count_credentials(&self) -> Result { let mut iter_result = Ok(()); @@ -811,7 +786,8 @@ mod test { // These should have different IDs. let credential_source0 = create_credential_source(&mut rng, "example.com", vec![0x00]); let credential_source1 = create_credential_source(&mut rng, "example.com", vec![0x00]); - let expected_credential = credential_source1.clone(); + let credential_id0 = credential_source0.credential_id.clone(); + let credential_id1 = credential_source1.credential_id.clone(); assert!(persistent_store .store_credential(credential_source0) @@ -820,13 +796,14 @@ mod test { .store_credential(credential_source1) .is_ok()); assert_eq!(persistent_store.count_credentials().unwrap(), 1); - let filtered_credentials = persistent_store - .filter_credentials("example.com", false) - .unwrap(); - let retrieved_credential_source = persistent_store - .get_credential(filtered_credentials[0].0) - .unwrap(); - assert_eq!(retrieved_credential_source, expected_credential); + assert!(persistent_store + .find_credential("example.com", &credential_id0, false) + .unwrap() + .is_none()); + assert!(persistent_store + .find_credential("example.com", &credential_id1, false) + .unwrap() + .is_some()); let mut persistent_store = PersistentStore::new(&mut rng); for i in 0..MAX_SUPPORTED_RESIDENTIAL_KEYS { @@ -851,70 +828,21 @@ mod test { } #[test] - fn test_filter_get_credentials() { + fn test_get_credential() { let mut rng = ThreadRng256 {}; let mut persistent_store = PersistentStore::new(&mut rng); - assert_eq!(persistent_store.count_credentials().unwrap(), 0); let credential_source0 = create_credential_source(&mut rng, "example.com", vec![0x00]); let credential_source1 = create_credential_source(&mut rng, "example.com", vec![0x01]); let credential_source2 = create_credential_source(&mut rng, "another.example.com", vec![0x02]); - let id0 = credential_source0.credential_id.clone(); - let id1 = credential_source1.credential_id.clone(); - assert!(persistent_store - .store_credential(credential_source0) - .is_ok()); - assert!(persistent_store - .store_credential(credential_source1) - .is_ok()); - assert!(persistent_store - .store_credential(credential_source2) - .is_ok()); - - let filtered_credentials = persistent_store - .filter_credentials("example.com", false) - .unwrap(); - assert_eq!(filtered_credentials.len(), 2); - let retrieved_credential0 = persistent_store - .get_credential(filtered_credentials[0].0) - .unwrap(); - let retrieved_credential1 = persistent_store - .get_credential(filtered_credentials[1].0) - .unwrap(); - assert!( - (retrieved_credential0.credential_id == id0 - && retrieved_credential1.credential_id == id1) - || (retrieved_credential1.credential_id == id0 - && retrieved_credential0.credential_id == id1) - ); - } - - #[test] - fn test_filter_with_cred_protect() { - let mut rng = ThreadRng256 {}; - let mut persistent_store = PersistentStore::new(&mut rng); - assert_eq!(persistent_store.count_credentials().unwrap(), 0); - let private_key = crypto::ecdsa::SecKey::gensk(&mut rng); - let credential = PublicKeyCredentialSource { - key_type: PublicKeyCredentialType::PublicKey, - credential_id: rng.gen_uniform_u8x32().to_vec(), - private_key, - rp_id: String::from("example.com"), - user_handle: vec![0x00], - user_display_name: None, - cred_protect_policy: Some( - CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList, - ), - creation_order: 0, - user_name: None, - user_icon: None, - }; - assert!(persistent_store.store_credential(credential).is_ok()); - - let no_credential = persistent_store - .filter_credentials("example.com", true) - .unwrap(); - assert_eq!(no_credential, vec![]); + let credential_sources = vec![credential_source0, credential_source1, credential_source2]; + for credential_source in credential_sources.into_iter() { + let cred_id = credential_source.credential_id.clone(); + assert!(persistent_store.store_credential(credential_source).is_ok()); + let (key, _) = persistent_store.find_credential_item(&cred_id).unwrap(); + let cred = persistent_store.get_credential(key).unwrap(); + assert_eq!(&cred_id, &cred.credential_id); + } } #[test]