Merge branch 'mathias-CRP-1761-split-secretkeystoreerror-persistencee…

…rror' into 'master'

chore(crypto): CRP-1761: Split SecretKeyStoreError::PersistenceError in two

Split `SecretKeyStoreError::PersistenceError` into 2 enums variants as per the discussion in [MR-10574](https://gitlab.com/dfinity-lab/public/ic/-/merge_requests/10574#note_1266480721) since one of the variant corresponds to a reproducible error (`SecretKeyStorePersistenceError::SerializationError`) while the other (`SecretKeyStorePersistenceError::IoError`) is transient. As such error mapping while matching on `SecretKeyStoreError::PersistenceError` was error-prone. The introduction of `SecretKeyStoreError:PersistenceIo` and `SecretKeyStoreError:PersistenceSerialization` aims to solve this issue. 

See merge request dfinity-lab/public/ic!12010

rs/crypto/internal/crypto_service_provider/src/secret_key_store/mock_secret_key_store.rs

@@ -1,6 +1,6 @@
 use crate::key_id::KeyId;
 use crate::secret_key_store::{
-    Scope, SecretKeyStore, SecretKeyStoreError, SecretKeyStorePersistenceError,
+    Scope, SecretKeyStore, SecretKeyStoreInsertionError, SecretKeyStoreWriteError,
 };
 use crate::types::CspSecretKey;
 use mockall::predicate::*;
@@ -10,11 +10,11 @@ mock! {
     pub SecretKeyStore {}
 
     pub trait SecretKeyStore {
-        fn insert(&mut self, id: KeyId, key: CspSecretKey, scope: Option<Scope>) -> Result<(), SecretKeyStoreError>;
+        fn insert(&mut self, id: KeyId, key: CspSecretKey, scope: Option<Scope>) -> Result<(), SecretKeyStoreInsertionError>;
         fn get(&self, id: &KeyId) -> Option<CspSecretKey>;
         fn contains(&self, id: &KeyId) -> bool;
-        fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStorePersistenceError>;
-        fn retain<F>(&mut self, filter: F, scope: Scope) -> Result<(), SecretKeyStorePersistenceError>
+        fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStoreWriteError>;
+        fn retain<F>(&mut self, filter: F, scope: Scope) -> Result<(), SecretKeyStoreWriteError>
             where F: Fn(&KeyId, &CspSecretKey) -> bool + 'static;
     }
 }

rs/crypto/internal/crypto_service_provider/src/secret_key_store/mod.rs

@@ -31,7 +31,7 @@ pub trait SecretKeyStore: Send + Sync {
         id: KeyId,
         key: CspSecretKey,
         scope: Option<Scope>,
-    ) -> Result<(), SecretKeyStoreError>;
+    ) -> Result<(), SecretKeyStoreInsertionError>;
 
     /// Inserts a key with a given `id` into the store, replacing any existing
     /// entry.
@@ -44,17 +44,22 @@ pub trait SecretKeyStore: Send + Sync {
         id: KeyId,
         key: CspSecretKey,
         scope: Option<Scope>,
-    ) -> Result<(), SecretKeyStorePersistenceError> {
+    ) -> Result<(), SecretKeyStoreWriteError> {
         self.remove(&id)?;
         self.insert(id, key, scope).map_err(|e| match e {
-            SecretKeyStoreError::DuplicateKeyId(e) => {
+            SecretKeyStoreInsertionError::DuplicateKeyId(e) => {
                 // unreachable, because key with `id` was removed prior to insertion
                 panic!(
                     "Duplicate key error although the key was just removed: {}",
                     e
                 );
             }
-            SecretKeyStoreError::PersistenceError(e) => e,
+            SecretKeyStoreInsertionError::SerializationError(e) => {
+                SecretKeyStoreWriteError::SerializationError(e)
+            }
+            SecretKeyStoreInsertionError::TransientError(e) => {
+                SecretKeyStoreWriteError::TransientError(e)
+            }
         })
     }
 
@@ -74,7 +79,7 @@ pub trait SecretKeyStore: Send + Sync {
     /// The return value indicates whether a key with the given `id` was
     /// previously contained and removed, or an error if the updated secret key store
     /// could not be written.
-    fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStorePersistenceError>;
+    fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStoreWriteError>;
 
     /// Keeps only entries in a scope for which the filter function returns
     /// `true` and removes the rest.
@@ -104,53 +109,72 @@ pub trait SecretKeyStore: Send + Sync {
     /// can be added to this implementation and we may require `panic="unwind"`.
     /// See the (book)[https://doc.rust-lang.org/edition-guide/rust-2018/error-handling-and-panics/controlling-panics-with-std-panic.html]
     /// and function documentation for more details.
-    fn retain<F>(&mut self, _filter: F, _scope: Scope) -> Result<(), SecretKeyStorePersistenceError>
+    fn retain<F>(&mut self, _filter: F, _scope: Scope) -> Result<(), SecretKeyStoreWriteError>
     where
         F: Fn(&KeyId, &CspSecretKey) -> bool + 'static,
     {
         unimplemented!()
     }
 }
 
-/// Errors that can occur while interacting with the secret key store
+/// Errors that can occur while inserting a key into the secret key store
 #[derive(Clone, Debug)]
-pub enum SecretKeyStoreError {
+pub enum SecretKeyStoreInsertionError {
     DuplicateKeyId(KeyId),
-    PersistenceError(SecretKeyStorePersistenceError),
+    /// Happens when writing to disk, see `SecretKeyStoreWriteError::SerializationError`
+    SerializationError(String),
+    /// Happens when writing to disk, see `SecretKeyStoreWriteError::TransientError`
+    TransientError(String),
 }
 
-impl std::error::Error for SecretKeyStoreError {}
+impl std::error::Error for SecretKeyStoreInsertionError {}
 
-impl fmt::Display for SecretKeyStoreError {
+impl fmt::Display for SecretKeyStoreInsertionError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            SecretKeyStoreError::DuplicateKeyId(key_id) => {
+            SecretKeyStoreInsertionError::DuplicateKeyId(key_id) => {
                 write!(f, "Key with ID {} already exists in the key store", key_id)
             }
-            SecretKeyStoreError::PersistenceError(e) => {
-                write!(f, "{}", e)
+            SecretKeyStoreInsertionError::SerializationError(e) => {
+                write!(f, "Error serializing secret key store: {}", e)
+            }
+            SecretKeyStoreInsertionError::TransientError(e) => {
+                write!(f, "Transient error persisting secret key store: {}", e)
+            }
+        }
+    }
+}
+
+impl From<SecretKeyStoreWriteError> for SecretKeyStoreInsertionError {
+    fn from(e: SecretKeyStoreWriteError) -> Self {
+        match e {
+            SecretKeyStoreWriteError::SerializationError(e) => {
+                SecretKeyStoreInsertionError::SerializationError(e)
+            }
+            SecretKeyStoreWriteError::TransientError(e) => {
+                SecretKeyStoreInsertionError::TransientError(e)
             }
         }
     }
 }
 
-/// Errors that can occur while persisting the secret key store
+/// Errors that can occur while writing a secret key store to disk
 #[derive(Clone, Debug)]
-pub enum SecretKeyStorePersistenceError {
+pub enum SecretKeyStoreWriteError {
     SerializationError(String),
-    IoError(String),
+    TransientError(String),
 }
 
-impl std::error::Error for SecretKeyStorePersistenceError {}
+impl std::error::Error for SecretKeyStoreWriteError {}
 
-impl fmt::Display for SecretKeyStorePersistenceError {
+impl fmt::Display for SecretKeyStoreWriteError {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
-            SecretKeyStorePersistenceError::SerializationError(e) => {
+            SecretKeyStoreWriteError::SerializationError(e) => {
                 write!(f, "Error serializing secret key store: {}", e)
             }
-            SecretKeyStorePersistenceError::IoError(e) => {
-                write!(f, "I/O error persisting secret key store: {}", e)
+            SecretKeyStoreWriteError::TransientError(e) => {
+                write!(f, "Transient error persisting secret key store: {}", e)
             }
         }
     }

rs/crypto/internal/crypto_service_provider/src/secret_key_store/proto_store.rs

@@ -3,7 +3,7 @@
 use crate::canister_threshold::IDKG_MEGA_SCOPE;
 use crate::key_id::KeyId;
 use crate::secret_key_store::{
-    Scope, SecretKeyStore, SecretKeyStoreError, SecretKeyStorePersistenceError,
+    Scope, SecretKeyStore, SecretKeyStoreInsertionError, SecretKeyStoreWriteError,
 };
 use crate::types::CspSecretKey;
 use hex::{FromHex, ToHex};
@@ -178,7 +178,7 @@ impl ProtoSecretKeyStore {
     fn write_secret_keys_to_disk(
         &self,
         secret_keys: &SecretKeys,
-    ) -> Result<(), SecretKeyStorePersistenceError> {
+    ) -> Result<(), SecretKeyStoreWriteError> {
         let sks_proto = ProtoSecretKeyStore::secret_keys_to_sks_proto(secret_keys)?;
         match self.proto_file.try_exists() {
             Ok(exists) => {
@@ -209,7 +209,7 @@ impl ProtoSecretKeyStore {
         // Write the new keystore to a new file and atomically replace the existing keystore.
         // The previously created hard link still points to the old keystore file.
         ic_utils::fs::write_protobuf_using_tmp_file(&self.proto_file, &sks_proto).map_err(|e| {
-            SecretKeyStorePersistenceError::IoError(format!(
+            SecretKeyStoreWriteError::TransientError(format!(
                 "Secret key store internal error writing protobuf using tmp file: {}",
                 e
             ))
@@ -329,7 +329,7 @@ impl ProtoSecretKeyStore {
 
     fn secret_keys_to_sks_proto(
         secret_keys: &SecretKeys,
-    ) -> Result<pb::SecretKeyStore, SecretKeyStorePersistenceError> {
+    ) -> Result<pb::SecretKeyStore, SecretKeyStoreWriteError> {
         let mut sks_proto = pb::SecretKeyStore {
             version: CURRENT_SKS_VERSION,
             ..Default::default()
@@ -338,7 +338,7 @@ impl ProtoSecretKeyStore {
             let key_id_hex = key_id.encode_hex();
             let key_as_cbor =
                 serde_cbor::to_vec(&csp_key).map_err(|_ignored_so_that_no_data_is_leaked| {
-                    SecretKeyStorePersistenceError::SerializationError(format!(
+                    SecretKeyStoreWriteError::SerializationError(format!(
                         "Error serializing key with ID {}",
                         key_id
                     ))
@@ -365,20 +365,19 @@ impl SecretKeyStore for ProtoSecretKeyStore {
         id: KeyId,
         key: CspSecretKey,
         scope: Option<Scope>,
-    ) -> Result<(), SecretKeyStoreError> {
-        let inserted: Result<bool, SecretKeyStorePersistenceError> =
-            with_write_lock(&self.keys, |keys| match keys.get(&id) {
-                Some(_) => Ok(false),
-                None => {
-                    keys.insert(id, (key, scope));
-                    self.write_secret_keys_to_disk(keys)?;
-                    Ok(true)
-                }
-            });
-        match inserted {
-            Ok(false) => Err(SecretKeyStoreError::DuplicateKeyId(id)),
-            Ok(true) => Ok(()),
-            Err(e) => Err(SecretKeyStoreError::PersistenceError(e)),
+    ) -> Result<(), SecretKeyStoreInsertionError> {
+        let inserted = with_write_lock(&self.keys, |keys| match keys.get(&id) {
+            Some(_) => Ok(false),
+            None => {
+                keys.insert(id, (key, scope));
+                self.write_secret_keys_to_disk(keys)?;
+                Ok(true)
+            }
+        })?;
+        if inserted {
+            Ok(())
+        } else {
+            Err(SecretKeyStoreInsertionError::DuplicateKeyId(id))
         }
     }
 
@@ -392,7 +391,7 @@ impl SecretKeyStore for ProtoSecretKeyStore {
         self.get(id).is_some()
     }
 
-    fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStorePersistenceError> {
+    fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStoreWriteError> {
         with_write_lock(&self.keys, |keys| match keys.get(id) {
             Some(_) => {
                 keys.remove(id);
@@ -403,7 +402,7 @@ impl SecretKeyStore for ProtoSecretKeyStore {
         })
     }
 
-    fn retain<F>(&mut self, filter: F, scope: Scope) -> Result<(), SecretKeyStorePersistenceError>
+    fn retain<F>(&mut self, filter: F, scope: Scope) -> Result<(), SecretKeyStoreWriteError>
     where
         F: Fn(&KeyId, &CspSecretKey) -> bool,
     {
@@ -476,10 +475,10 @@ fn overwrite_file_with_zeroes_and_delete_if_it_exists<P: AsRef<Path>>(
     };
 }
 
-fn with_write_lock<T, I, R, F>(v: T, f: F) -> Result<R, SecretKeyStorePersistenceError>
+fn with_write_lock<T, I, R, F>(v: T, f: F) -> Result<R, SecretKeyStoreWriteError>
 where
     T: AsRef<RwLock<I>>,
-    F: FnOnce(&mut I) -> Result<R, SecretKeyStorePersistenceError>,
+    F: FnOnce(&mut I) -> Result<R, SecretKeyStoreWriteError>,
 {
     let mut lock_result = v.as_ref().write();
     f(lock_result.borrow_mut())

rs/crypto/internal/crypto_service_provider/src/secret_key_store/proto_store/tests.rs

@@ -3,7 +3,7 @@ use crate::canister_threshold::IDKG_MEGA_SCOPE;
 use crate::secret_key_store::temp_secret_key_store::TempSecretKeyStore;
 use crate::secret_key_store::test_utils::{make_key_id, make_secret_key};
 use crate::secret_key_store::{
-    scope::ConstScope, Scope, SecretKeyStore, SecretKeyStoreError, SecretKeyStorePersistenceError,
+    scope::ConstScope, Scope, SecretKeyStore, SecretKeyStoreInsertionError,
 };
 use crate::types::CspSecretKey;
 use assert_matches::assert_matches;
@@ -415,10 +415,8 @@ fn should_fail_to_write_to_read_only_secret_key_store_directory() {
 
     assert_matches!(
         secret_key_store.insert(key_id, key, None),
-        Err(SecretKeyStoreError::PersistenceError(
-            SecretKeyStorePersistenceError::IoError(msg)
-        ))
-        if msg.to_lowercase().contains("permission denied")
+        Err(SecretKeyStoreInsertionError::TransientError(msg))
+        if msg.to_lowercase().contains("secret key store internal error writing protobuf using tmp file: permission denied")
     );
 
     fs::set_permissions(temp_dir.path(), Permissions::from_mode(0o700)).expect(
@@ -444,9 +442,7 @@ fn should_fail_to_write_to_secret_key_store_directory_without_execute_permission
 
     assert_matches!(
         secret_key_store.insert(key_id, key, None),
-        Err(SecretKeyStoreError::PersistenceError(
-            SecretKeyStorePersistenceError::IoError(msg)
-        ))
+        Err(SecretKeyStoreInsertionError::TransientError(msg))
         if msg.to_lowercase().contains("permission denied")
     );
 
@@ -473,9 +469,7 @@ fn should_fail_to_write_to_secret_key_store_directory_without_write_permissions(
 
     assert_matches!(
         secret_key_store.insert(key_id, key, None),
-        Err(SecretKeyStoreError::PersistenceError(
-            SecretKeyStorePersistenceError::IoError(msg)
-        ))
+        Err(SecretKeyStoreInsertionError::TransientError(msg))
         if msg.to_lowercase().contains("permission denied")
     );
 

rs/crypto/internal/crypto_service_provider/src/secret_key_store/temp_secret_key_store.rs

@@ -1,7 +1,7 @@
 use crate::key_id::KeyId;
 use crate::secret_key_store::proto_store::ProtoSecretKeyStore;
 use crate::secret_key_store::{
-    SecretKeyStore, SecretKeyStoreError, SecretKeyStorePersistenceError,
+    SecretKeyStore, SecretKeyStoreInsertionError, SecretKeyStoreWriteError,
 };
 use crate::types::CspSecretKey;
 use ic_crypto_internal_types::scope::Scope;
@@ -47,7 +47,7 @@ impl SecretKeyStore for TempSecretKeyStore {
         id: KeyId,
         key: CspSecretKey,
         scope: Option<Scope>,
-    ) -> Result<(), SecretKeyStoreError> {
+    ) -> Result<(), SecretKeyStoreInsertionError> {
         self.store.insert(id, key, scope)
     }
 
@@ -59,15 +59,15 @@ impl SecretKeyStore for TempSecretKeyStore {
         self.store.contains(id)
     }
 
-    fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStorePersistenceError> {
+    fn remove(&mut self, id: &KeyId) -> Result<bool, SecretKeyStoreWriteError> {
         self.store.remove(id)
     }
 
     fn retain<F: 'static>(
         &mut self,
         filter: F,
         scope: Scope,
-    ) -> Result<(), SecretKeyStorePersistenceError>
+    ) -> Result<(), SecretKeyStoreWriteError>
     where
         F: Fn(&KeyId, &CspSecretKey) -> bool,
     {

rs/crypto/internal/crypto_service_provider/src/vault/local_csp_vault/basic_sig/mod.rs

@@ -2,9 +2,7 @@
 use crate::key_id::KeyId;
 use crate::keygen::utils::node_signing_pk_to_proto;
 use crate::public_key_store::{PublicKeySetOnceError, PublicKeyStore};
-use crate::secret_key_store::{
-    SecretKeyStore, SecretKeyStoreError, SecretKeyStorePersistenceError,
-};
+use crate::secret_key_store::{SecretKeyStore, SecretKeyStoreInsertionError};
 use crate::types::{CspPublicKey, CspSecretKey, CspSignature};
 use crate::vault::api::{
     BasicSignatureCspVault, CspBasicSignatureError, CspBasicSignatureKeygenError,
@@ -81,20 +79,18 @@ impl<R: Rng + CryptoRng, S: SecretKeyStore, C: SecretKeyStore, P: PublicKeyStore
         sks_write_lock
             .insert(key_id, secret_key, None)
             .map_err(|sks_error| match sks_error {
-                SecretKeyStoreError::DuplicateKeyId(key_id) => {
+                SecretKeyStoreInsertionError::DuplicateKeyId(key_id) => {
                     CspBasicSignatureKeygenError::DuplicateKeyId { key_id }
                 }
-                SecretKeyStoreError::PersistenceError(SecretKeyStorePersistenceError::IoError(e)) => {
+                SecretKeyStoreInsertionError::TransientError(e) => {
                     CspBasicSignatureKeygenError::TransientInternalError {
                         internal_error: format!(
                             "Error persisting secret key store during CSP basic signature key generation: {}",
                             e
                         ),
                     }
                 }
-                SecretKeyStoreError::PersistenceError(
-                    SecretKeyStorePersistenceError::SerializationError(e),
-                ) => CspBasicSignatureKeygenError::InternalError {
+                SecretKeyStoreInsertionError::SerializationError(e) => CspBasicSignatureKeygenError::InternalError {
                     internal_error: format!(
                         "Error persisting secret key store during CSP basic signature key generation: {}",
                         e