Inlining, refactoring and cleanup

libraries/opensk/src/api/persist.rs

@@ -32,10 +32,16 @@ pub type PersistCredentialIter<'a> = Box<dyn Iterator<Item = CtapResult<(usize,
 ///
 /// This trait might get appended to with new versions of CTAP.
 ///
+/// The default implementations using the key-value store have assumptions on the ranges for key
+/// and value, if you decide to use them:
+/// - Keys must be valid at least in [0, 4095[.
+/// - Values must be byte arrays of size at least 1023.
+///
 /// To implement this trait, you have 2 options:
 /// - Implement all high level functions with default implementations,
 ///   calling `unimplemented!` in the key-value accessors.
 ///   When we update this trait in a new version, OpenSK will panic when calling any new functions.
+///   If you need special implementation for new functions, you need to manually add them.
 /// - Implement the key-value accessors, and special case as many default implemented high level
 ///   functions as desired.
 ///   When the trait gets extended, new features will silently work.
@@ -45,8 +51,6 @@ pub trait Persist {
     fn find(&self, key: usize) -> CtapResult<Option<Vec<u8>>>;
 
     /// Inserts the value at the given key.
-    ///
-    /// Values up to a length of 1023 Byte must be supported.
     fn insert(&mut self, key: usize, value: &[u8]) -> CtapResult<()>;
 
     /// Removes a key, if present.
@@ -55,12 +59,13 @@ pub trait Persist {
     /// Iterator for all present keys.
     fn iter(&self) -> CtapResult<PersistIter<'_>>;
 
-    /// Checks consistency on boot, and if necessary fixes or initializes problems.
+    /// Checks consistency on boot, and if necessary fixes problems or initializes.
+    ///
+    /// Calling this function after successful init should be a NO-OP.
     fn init(&mut self) -> CtapResult<()> {
         if self.find(keys::RESET_COMPLETION)?.is_some() {
             self.reset()?;
         }
-        // TODO don't forget to call, add other init functionality
         Ok(())
     }
 
@@ -213,6 +218,8 @@ pub trait Persist {
     }
 
     /// Returns the list of RP IDs that may read the minimum PIN length.
+    ///
+    /// Defaults to an empty vector if not found.
     fn min_pin_length_rp_ids_bytes(&self) -> CtapResult<Vec<u8>> {
         Ok(self
             .find(keys::MIN_PIN_LENGTH_RP_IDS)?
@@ -225,6 +232,13 @@ pub trait Persist {
         self.insert(keys::MIN_PIN_LENGTH_RP_IDS, min_pin_length_rp_ids_bytes)
     }
 
+    // TODO rework LargeBlob
+    // Problem 1: Env should be allowed to choose whether to buffer in memory or persist
+    // Otherwise small RAM devices have limited large blog size.
+    // Problem 2: LargeBlob is a stateful command, but doesn't use the safeguard and infrastructure
+    // of Stateful command. It has to be migrated there.
+    // While doing that, check if PinUvAuthToken timers and StatefulCommand timeouts are working
+    // together correctly.
     /// Reads the byte vector stored as the serialized large blobs array.
     ///
     /// If too few bytes exist at that offset, return the maximum number
@@ -352,9 +366,17 @@ pub trait Persist {
     fn get_attestation(&self, id: AttestationId) -> CtapResult<Option<Attestation>> {
         let stored_id_bytes = self.find(keys::ATTESTATION_ID)?;
         if let Some(bytes) = stored_id_bytes {
-            if bytes.len() != 1 || id != AttestationId::try_from(bytes[0])? {
+            if bytes.len() != 1 {
                 return Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR);
             }
+            if id != AttestationId::try_from(bytes[0])? {
+                return Ok(None);
+            }
+        } else {
+            // This is for backwards compatibility. No ID stored implies batch.
+            if id != AttestationId::Batch {
+                return Ok(None);
+            }
         }
         let private_key = self.find(keys::ATTESTATION_PRIVATE_KEY)?;
         let certificate = self.find(keys::ATTESTATION_CERTIFICATE)?;
@@ -446,6 +468,7 @@ pub struct Attestation {
 mod test {
     use super::*;
     use crate::api::customization::Customization;
+    use crate::api::rng::Rng;
     use crate::env::test::TestEnv;
     use crate::env::Env;
 
@@ -469,4 +492,59 @@ mod test {
             Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR)
         );
     }
+
+    #[test]
+    fn test_global_signature_counter() {
+        let mut env = TestEnv::default();
+        let persist = env.persist();
+
+        let mut counter_value = 1;
+        assert_eq!(persist.global_signature_counter().unwrap(), counter_value);
+        for increment in 1..10 {
+            assert!(persist.incr_global_signature_counter(increment).is_ok());
+            counter_value += increment;
+            assert_eq!(persist.global_signature_counter().unwrap(), counter_value);
+        }
+    }
+
+    #[test]
+    fn test_force_pin_change() {
+        let mut env = TestEnv::default();
+        let persist = env.persist();
+
+        assert!(!persist.has_force_pin_change().unwrap());
+        assert_eq!(persist.force_pin_change(), Ok(()));
+        assert!(persist.has_force_pin_change().unwrap());
+        assert_eq!(persist.set_pin(&[0x88; 16], 8), Ok(()));
+        assert!(!persist.has_force_pin_change().unwrap());
+    }
+
+    #[test]
+    fn test_pin_hash_and_length() {
+        let mut env = TestEnv::default();
+        let random_data = env.rng().gen_uniform_u8x32();
+        let persist = env.persist();
+
+        // Pin hash is initially not set.
+        assert!(persist.pin_hash().unwrap().is_none());
+        assert!(persist.pin_code_point_length().unwrap().is_none());
+
+        // Setting the pin sets the pin hash.
+        assert_eq!(random_data.len(), 2 * PIN_AUTH_LENGTH);
+        let pin_hash_1 = *array_ref!(random_data, 0, PIN_AUTH_LENGTH);
+        let pin_hash_2 = *array_ref!(random_data, PIN_AUTH_LENGTH, PIN_AUTH_LENGTH);
+        let pin_length_1 = 4;
+        let pin_length_2 = 63;
+        assert_eq!(persist.set_pin(&pin_hash_1, pin_length_1), Ok(()));
+        assert_eq!(persist.pin_hash().unwrap(), Some(pin_hash_1));
+        assert_eq!(persist.pin_code_point_length().unwrap(), Some(pin_length_1));
+        assert_eq!(persist.set_pin(&pin_hash_2, pin_length_2), Ok(()));
+        assert_eq!(persist.pin_hash().unwrap(), Some(pin_hash_2));
+        assert_eq!(persist.pin_code_point_length().unwrap(), Some(pin_length_2));
+
+        // Resetting the storage resets the pin hash.
+        assert_eq!(persist.reset(), Ok(()));
+        assert!(persist.pin_hash().unwrap().is_none());
+        assert!(persist.pin_code_point_length().unwrap().is_none());
+    }
 }

libraries/opensk/src/ctap/client_pin.rs

@@ -27,6 +27,7 @@ use crate::api::crypto::hmac256::Hmac256;
 use crate::api::crypto::sha256::Sha256;
 use crate::api::customization::Customization;
 use crate::api::key_store::KeyStore;
+use crate::api::persist::Persist;
 use crate::ctap::storage;
 #[cfg(test)]
 use crate::env::EcdhSk;
@@ -103,11 +104,8 @@ fn check_and_store_new_pin<E: Env>(
         .key_store()
         .encrypt_pin_hash(array_ref![pin_hash, 0, PIN_AUTH_LENGTH])?;
     // The PIN length is always < PIN_PADDED_LENGTH < 256.
-    storage::set_pin(
-        env,
-        array_ref!(pin_hash, 0, PIN_AUTH_LENGTH),
-        pin_length as u8,
-    )?;
+    env.persist()
+        .set_pin(array_ref!(pin_hash, 0, PIN_AUTH_LENGTH), pin_length as u8)?;
     Ok(())
 }
 
@@ -187,7 +185,7 @@ impl<E: Env> ClientPin<E> {
         shared_secret: &SharedSecret<E>,
         pin_hash_enc: Vec<u8>,
     ) -> Result<(), Ctap2StatusCode> {
-        match storage::pin_hash(env)? {
+        match env.persist().pin_hash()? {
             Some(pin_hash) => {
                 if self.consecutive_pin_mismatches >= 3 {
                     return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_BLOCKED);
@@ -263,7 +261,7 @@ impl<E: Env> ClientPin<E> {
         let pin_uv_auth_param = ok_or_missing(pin_uv_auth_param)?;
         let new_pin_enc = ok_or_missing(new_pin_enc)?;
 
-        if storage::pin_hash(env)?.is_some() {
+        if env.persist().pin_hash()?.is_some() {
             return Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID);
         }
         let shared_secret = self.get_shared_secret(pin_uv_auth_protocol, key_agreement)?;
@@ -331,7 +329,7 @@ impl<E: Env> ClientPin<E> {
         }
         let shared_secret = self.get_shared_secret(pin_uv_auth_protocol, key_agreement)?;
         self.verify_pin_hash_enc(env, pin_uv_auth_protocol, &shared_secret, pin_hash_enc)?;
-        if storage::has_force_pin_change(env)? {
+        if env.persist().has_force_pin_change()? {
             return Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID);
         }
 
@@ -618,7 +616,7 @@ mod test {
         pin[..4].copy_from_slice(b"1234");
         let mut pin_hash = [0u8; 16];
         pin_hash.copy_from_slice(&Sha::<TestEnv>::digest(&pin[..])[..16]);
-        storage::set_pin(env, &pin_hash, 4).unwrap();
+        env.persist().set_pin(&pin_hash, 4).unwrap();
     }
 
     /// Fails on PINs bigger than 64 bytes.
@@ -752,7 +750,7 @@ mod test {
             0x01, 0xD9, 0x88, 0x40, 0x50, 0xBB, 0xD0, 0x7A, 0x23, 0x1A, 0xEB, 0x69, 0xD8, 0x36,
             0xC4, 0x12,
         ];
-        storage::set_pin(&mut env, &pin_hash, 4).unwrap();
+        env.persist().set_pin(&pin_hash, 4).unwrap();
 
         let pin_hash_enc = shared_secret.encrypt(&mut env, &pin_hash).unwrap();
         assert_eq!(
@@ -1047,7 +1045,7 @@ mod test {
         let mut env = TestEnv::default();
         set_standard_pin(&mut env);
 
-        assert_eq!(storage::force_pin_change(&mut env), Ok(()));
+        assert_eq!(env.persist().force_pin_change(), Ok(()));
         assert_eq!(
             client_pin.process_command(&mut env, params),
             Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID),
@@ -1157,7 +1155,7 @@ mod test {
         let mut env = TestEnv::default();
         set_standard_pin(&mut env);
 
-        assert_eq!(storage::force_pin_change(&mut env), Ok(()));
+        assert_eq!(env.persist().force_pin_change(), Ok(()));
         assert_eq!(
             client_pin.process_command(&mut env, params),
             Err(Ctap2StatusCode::CTAP2_ERR_PIN_INVALID)
@@ -1249,17 +1247,17 @@ mod test {
             ),
         ];
         for (pin, result) in test_cases {
-            let old_pin_hash = storage::pin_hash(&mut env).unwrap();
+            let old_pin_hash = env.persist().pin_hash().unwrap();
             let new_pin_enc = encrypt_pin(&shared_secret, pin);
 
             assert_eq!(
                 check_and_store_new_pin(&mut env, &shared_secret, new_pin_enc),
                 result
             );
             if result.is_ok() {
-                assert_ne!(old_pin_hash, storage::pin_hash(&mut env).unwrap());
+                assert_ne!(old_pin_hash, env.persist().pin_hash().unwrap());
             } else {
-                assert_eq!(old_pin_hash, storage::pin_hash(&mut env).unwrap());
+                assert_eq!(old_pin_hash, env.persist().pin_hash().unwrap());
             }
         }
     }

libraries/opensk/src/ctap/config_command.rs

@@ -18,6 +18,7 @@ use super::data_formats::{ConfigSubCommand, ConfigSubCommandParams, SetMinPinLen
 use super::response::ResponseData;
 use super::status_code::Ctap2StatusCode;
 use crate::api::customization::Customization;
+use crate::api::persist::Persist;
 use crate::ctap::storage;
 use crate::env::Env;
 use alloc::vec;
@@ -56,14 +57,14 @@ fn process_set_min_pin_length(
         return Err(Ctap2StatusCode::CTAP2_ERR_PIN_POLICY_VIOLATION);
     }
     let mut force_change_pin = force_change_pin.unwrap_or(false);
-    if force_change_pin && storage::pin_hash(env)?.is_none() {
+    if force_change_pin && env.persist().pin_hash()?.is_none() {
         return Err(Ctap2StatusCode::CTAP2_ERR_PIN_NOT_SET);
     }
-    if let Some(old_length) = storage::pin_code_point_length(env)? {
+    if let Some(old_length) = env.persist().pin_code_point_length()? {
         force_change_pin |= new_min_pin_length > old_length;
     }
     if force_change_pin {
-        storage::force_pin_change(env)?;
+        env.persist().force_pin_change()?;
     }
     storage::set_min_pin_length(env, new_min_pin_length)?;
     if let Some(min_pin_length_rp_ids) = min_pin_length_rp_ids {
@@ -87,7 +88,7 @@ pub fn process_config<E: Env>(
 
     let enforce_uv =
         !matches!(sub_command, ConfigSubCommand::ToggleAlwaysUv) && storage::has_always_uv(env)?;
-    if storage::pin_hash(env)?.is_some() || enforce_uv {
+    if env.persist().pin_hash()?.is_some() || enforce_uv {
         let pin_uv_auth_param =
             pin_uv_auth_param.ok_or(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED)?;
         let pin_uv_auth_protocol =
@@ -211,7 +212,7 @@ mod test {
             pin_uv_auth_token,
             pin_uv_auth_protocol,
         );
-        storage::set_pin(&mut env, &[0x88; 16], 4).unwrap();
+        env.persist().set_pin(&[0x88; 16], 4).unwrap();
 
         let mut config_data = vec![0xFF; 32];
         config_data.extend(&[0x0D, ConfigSubCommand::ToggleAlwaysUv as u8]);
@@ -295,7 +296,7 @@ mod test {
 
         // Second, increase minimum PIN length from 6 to 8 with PIN auth.
         // The stored PIN or its length don't matter since we control the token.
-        storage::set_pin(&mut env, &[0x88; 16], 8).unwrap();
+        env.persist().set_pin(&[0x88; 16], 8).unwrap();
         let min_pin_length = 8;
         let mut config_params = create_min_pin_config_params(min_pin_length, None);
         let pin_uv_auth_param = vec![
@@ -351,7 +352,7 @@ mod test {
         let min_pin_length = 8;
         let min_pin_length_rp_ids = vec!["another.example.com".to_string()];
         // The stored PIN or its length don't matter since we control the token.
-        storage::set_pin(&mut env, &[0x88; 16], 8).unwrap();
+        env.persist().set_pin(&[0x88; 16], 8).unwrap();
         let mut config_params =
             create_min_pin_config_params(min_pin_length, Some(min_pin_length_rp_ids.clone()));
         let pin_uv_auth_param = vec![
@@ -414,7 +415,7 @@ mod test {
             PinUvAuthProtocol::V1,
         );
 
-        storage::set_pin(&mut env, &[0x88; 16], 4).unwrap();
+        env.persist().set_pin(&[0x88; 16], 4).unwrap();
         // Increase min PIN, force PIN change.
         let min_pin_length = 6;
         let mut config_params = create_min_pin_config_params(min_pin_length, None);
@@ -426,7 +427,7 @@ mod test {
         let config_response = process_config(&mut env, &mut client_pin, config_params);
         assert_eq!(config_response, Ok(ResponseData::AuthenticatorConfig));
         assert_eq!(storage::min_pin_length(&mut env), Ok(min_pin_length));
-        assert_eq!(storage::has_force_pin_change(&mut env), Ok(true));
+        assert_eq!(env.persist().has_force_pin_change(), Ok(true));
     }
 
     #[test]
@@ -441,7 +442,7 @@ mod test {
             PinUvAuthProtocol::V1,
         );
 
-        storage::set_pin(&mut env, &[0x88; 16], 4).unwrap();
+        env.persist().set_pin(&[0x88; 16], 4).unwrap();
         let pin_uv_auth_param = Some(vec![
             0xE3, 0x74, 0xF4, 0x27, 0xBE, 0x7D, 0x40, 0xB5, 0x71, 0xB6, 0xB4, 0x1A, 0xD2, 0xC1,
             0x53, 0xD7,
@@ -461,7 +462,7 @@ mod test {
         };
         let config_response = process_config(&mut env, &mut client_pin, config_params);
         assert_eq!(config_response, Ok(ResponseData::AuthenticatorConfig));
-        assert_eq!(storage::has_force_pin_change(&mut env), Ok(true));
+        assert_eq!(env.persist().has_force_pin_change(), Ok(true));
     }
 
     #[test]