improved testing, addresses comments and a default level fix

README.md

@@ -99,9 +99,10 @@ a few things you can personalize:
 4.  Depending on your available flash storage, choose an appropriate maximum
     number of supported residential keys and number of pages in
     `ctap/storage.rs`.
-5.  Change the default level for the credProtect extension. Resident credentials
+5.  Change the default level for the credProtect extension in `ctap/mod.rs`.
-    become undiscoverable without user verification. This helps privacy, but
+    When changing the default, resident credentials become undiscoverable without
-    can make usage less comfortable for less important credentials.
+    user verification. This helps privacy, but can make usage less comfortable
+    for credentials that need less protection.
 
 ### 3D printed enclosure
 

src/ctap/data_formats.rs

@@ -440,7 +440,7 @@ impl TryFrom<&cbor::Value> for SignatureAlgorithm {
     }
 }
 
-#[derive(Clone, PartialEq, PartialOrd)]
+#[derive(Clone, Copy, PartialEq, PartialOrd)]
 #[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))]
 pub enum CredentialProtectionPolicy {
     UserVerificationOptional = 0x01,
@@ -1079,6 +1079,22 @@ mod test {
         assert_eq!(unknown_algorithm, Ok(expected_unknown_algorithm));
     }
 
+    #[test]
+    fn test_cred_protection_policy_order() {
+        assert!(
+            CredentialProtectionPolicy::UserVerificationOptional
+                < CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList
+        );
+        assert!(
+            CredentialProtectionPolicy::UserVerificationOptional
+                < CredentialProtectionPolicy::UserVerificationRequired
+        );
+        assert!(
+            CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList
+                < CredentialProtectionPolicy::UserVerificationRequired
+        );
+    }
+
     #[test]
     fn test_from_into_cred_protection_policy() {
         let cbor_policy = cbor_int!(CredentialProtectionPolicy::UserVerificationOptional as i64);

src/ctap/mod.rs

@@ -438,12 +438,9 @@ where
             let mut cred_protect = extensions
                 .make_credential_cred_protect_policy()
                 .transpose()?;
-            if cred_protect
+            if cred_protect.unwrap_or(CredentialProtectionPolicy::UserVerificationOptional)
-                .as_ref()
+                < DEFAULT_CRED_PROTECT
-                .unwrap_or(&CredentialProtectionPolicy::UserVerificationOptional)
+                    .unwrap_or(CredentialProtectionPolicy::UserVerificationOptional)
-                > DEFAULT_CRED_PROTECT
-                    .as_ref()
-                    .unwrap_or(&CredentialProtectionPolicy::UserVerificationOptional)
             {
                 cred_protect = DEFAULT_CRED_PROTECT;
             }
@@ -704,15 +701,7 @@ where
             found_credentials
         } else {
             // TODO(kaczmarczyck) use GetNextAssertion
-            let found_credentials = self.persistent_store.filter_credential(&rp_id);
+            self.persistent_store.filter_credential(&rp_id, !has_uv)
-            if has_uv {
-                found_credentials
-            } else {
-                found_credentials
-                    .into_iter()
-                    .filter(|cred| cred.is_discoverable())
-                    .collect()
-            }
         };
 
         let credential = if let Some(credential) = credentials.first() {
@@ -1213,6 +1202,31 @@ mod test {
         }
     }
 
+    fn create_make_credential_parameters_with_exclude_list(
+        excluded_credential_id: &[u8],
+    ) -> AuthenticatorMakeCredentialParameters {
+        let excluded_credential_descriptor = PublicKeyCredentialDescriptor {
+            key_type: PublicKeyCredentialType::PublicKey,
+            key_id: excluded_credential_id.to_vec(),
+            transports: None,
+        };
+        let exclude_list = Some(vec![excluded_credential_descriptor]);
+        let mut make_credential_params = create_minimal_make_credential_parameters();
+        make_credential_params.exclude_list = exclude_list;
+        make_credential_params
+    }
+
+    fn create_make_credential_parameters_with_cred_protect_policy(
+        policy: CredentialProtectionPolicy,
+    ) -> AuthenticatorMakeCredentialParameters {
+        let mut extension_map = BTreeMap::new();
+        extension_map.insert("credProtect".to_string(), cbor_int!(policy as i64));
+        let extensions = Some(Extensions::new(extension_map));
+        let mut make_credential_params = create_minimal_make_credential_parameters();
+        make_credential_params.extensions = extensions;
+        make_credential_params
+    }
+
     #[test]
     fn test_residential_process_make_credential() {
         let mut rng = ThreadRng256 {};
@@ -1311,9 +1325,11 @@ mod test {
         let mut ctap_state = CtapState::new(&mut rng, user_immediately_present);
 
         let excluded_credential_id = vec![0x01, 0x23, 0x45, 0x67];
+        let make_credential_params =
+            create_make_credential_parameters_with_exclude_list(&excluded_credential_id);
         let excluded_credential_source = PublicKeyCredentialSource {
             key_type: PublicKeyCredentialType::PublicKey,
-            credential_id: excluded_credential_id.clone(),
+            credential_id: excluded_credential_id,
             private_key: excluded_private_key,
             rp_id: String::from("example.com"),
             user_handle: vec![],
@@ -1326,17 +1342,8 @@ mod test {
             .store_credential(excluded_credential_source)
             .is_ok());
 
-        let excluded_credential_descriptor = PublicKeyCredentialDescriptor {
-            key_type: PublicKeyCredentialType::PublicKey,
-            key_id: excluded_credential_id,
-            transports: None,
-        };
-        let exclude_list = Some(vec![excluded_credential_descriptor]);
-        let mut make_credential_params = create_minimal_make_credential_parameters();
-        make_credential_params.exclude_list = exclude_list;
         let make_credential_response =
             ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID);
-
         assert_eq!(
             make_credential_response,
             Err(Ctap2StatusCode::CTAP2_ERR_CREDENTIAL_EXCLUDED)
@@ -1346,53 +1353,50 @@ mod test {
     #[test]
     fn test_process_make_credential_credential_with_cred_protect() {
         let mut rng = ThreadRng256 {};
-        let excluded_private_key = crypto::ecdsa::SecKey::gensk(&mut rng);
         let user_immediately_present = |_| Ok(());
         let mut ctap_state = CtapState::new(&mut rng, user_immediately_present);
 
-        let excluded_credential_id = vec![0x01, 0x23, 0x45, 0x67];
+        let test_policy = CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList;
-        let mut excluded_credential_source = PublicKeyCredentialSource {
+        let make_credential_params =
-            key_type: PublicKeyCredentialType::PublicKey,
+            create_make_credential_parameters_with_cred_protect_policy(test_policy);
-            credential_id: excluded_credential_id.clone(),
-            private_key: excluded_private_key,
-            rp_id: String::from("example.com"),
-            user_handle: vec![],
-            other_ui: None,
-            cred_random: None,
-            cred_protect_policy: Some(
-                CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList,
-            ),
-        };
-        assert!(ctap_state
-            .persistent_store
-            .store_credential(excluded_credential_source.clone())
-            .is_ok());
-
-        let excluded_credential_descriptor = PublicKeyCredentialDescriptor {
-            key_type: PublicKeyCredentialType::PublicKey,
-            key_id: excluded_credential_id,
-            transports: None,
-        };
-        let exclude_list = Some(vec![excluded_credential_descriptor.clone()]);
-        let mut make_credential_params = create_minimal_make_credential_parameters();
-        make_credential_params.exclude_list = exclude_list;
         let make_credential_response =
             ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID);
+        assert!(make_credential_response.is_ok());
 
+        let stored_credential = ctap_state
+            .persistent_store
+            .filter_credential("example.com", false)
+            .pop()
+            .unwrap();
+        let credential_id = stored_credential.credential_id;
+        assert_eq!(stored_credential.cred_protect_policy, Some(test_policy));
+
+        let make_credential_params =
+            create_make_credential_parameters_with_exclude_list(&credential_id);
+        let make_credential_response =
+            ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID);
         assert_eq!(
             make_credential_response,
             Err(Ctap2StatusCode::CTAP2_ERR_CREDENTIAL_EXCLUDED)
         );
 
-        excluded_credential_source.cred_protect_policy =
+        let test_policy = CredentialProtectionPolicy::UserVerificationRequired;
-            Some(CredentialProtectionPolicy::UserVerificationRequired);
+        let make_credential_params =
-        assert!(ctap_state
+            create_make_credential_parameters_with_cred_protect_policy(test_policy);
+        let make_credential_response =
+            ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID);
+        assert!(make_credential_response.is_ok());
+
+        let stored_credential = ctap_state
             .persistent_store
-            .store_credential(excluded_credential_source)
+            .filter_credential("example.com", false)
-            .is_ok());
+            .pop()
-        let exclude_list = Some(vec![excluded_credential_descriptor]);
+            .unwrap();
-        let mut make_credential_params = create_minimal_make_credential_parameters();
+        let credential_id = stored_credential.credential_id;
-        make_credential_params.exclude_list = exclude_list;
+        assert_eq!(stored_credential.cred_protect_policy, Some(test_policy));
+
+        let make_credential_params =
+            create_make_credential_parameters_with_exclude_list(&credential_id);
         let make_credential_response =
             ctap_state.process_make_credential(make_credential_params, DUMMY_CHANNEL_ID);
         assert!(make_credential_response.is_ok());

src/ctap/storage.rs

@@ -269,7 +269,11 @@ impl PersistentStore {
         Ok(())
     }
 
-    pub fn filter_credential(&self, rp_id: &str) -> Vec<PublicKeyCredentialSource> {
+    pub fn filter_credential(
+        &self,
+        rp_id: &str,
+        check_cred_protect: bool,
+    ) -> Vec<PublicKeyCredentialSource> {
         self.store
             .find_all(&Key::Credential {
                 rp_id: Some(rp_id.into()),
@@ -282,6 +286,7 @@ impl PersistentStore {
                 debug_assert!(credential.is_some());
                 credential
             })
+            .filter(|cred| !check_cred_protect || cred.is_discoverable())
             .collect()
     }
 
@@ -549,7 +554,7 @@ mod test {
             .is_ok());
         assert_eq!(persistent_store.count_credentials(), 1);
         assert_eq!(
-            &persistent_store.filter_credential("example.com"),
+            &persistent_store.filter_credential("example.com", false),
             &[expected_credential]
         );
 
@@ -597,7 +602,7 @@ mod test {
             .store_credential(credential_source2)
             .is_ok());
 
-        let filtered_credentials = persistent_store.filter_credential("example.com");
+        let filtered_credentials = persistent_store.filter_credential("example.com", false);
         assert_eq!(filtered_credentials.len(), 2);
         assert!(
             (filtered_credentials[0].credential_id == id0
@@ -607,6 +612,30 @@ mod test {
         );
     }
 
+    #[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(), 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],
+            other_ui: None,
+            cred_random: None,
+            cred_protect_policy: Some(
+                CredentialProtectionPolicy::UserVerificationOptionalWithCredentialIdList,
+            ),
+        };
+        assert!(persistent_store.store_credential(credential).is_ok());
+
+        let no_credential = persistent_store.filter_credential("example.com", true);
+        assert_eq!(no_credential, vec![]);
+    }
+
     #[test]
     fn test_find() {
         let mut rng = ThreadRng256 {};