From 4f666c13d664488fba529e44e6d1b3391e5da030 Mon Sep 17 00:00:00 2001 From: shamardy Date: Wed, 14 Feb 2024 16:16:00 +0200 Subject: [PATCH] Review Fixes: - Remove arguments descriptions in doc comments. - Fix read passphrase functions names by adding `if_available`. - Improve cross-platform test compatibility. --- Cargo.lock | 2 + mm2src/crypto/Cargo.toml | 5 ++ mm2src/crypto/src/decrypt.rs | 5 -- mm2src/crypto/src/encrypt.rs | 6 -- mm2src/crypto/src/key_derivation.rs | 10 --- mm2src/crypto/src/mnemonic.rs | 29 +++---- mm2src/crypto/src/slip21.rs | 22 +++--- mm2src/mm2_main/src/lp_wallet.rs | 37 ++++----- .../src/lp_wallet/mnemonics_storage.rs | 23 +----- .../src/lp_wallet/mnemonics_wasm_db.rs | 77 ++++++++----------- 10 files changed, 76 insertions(+), 140 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 09293ce3d9..27d3aa6f3a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1566,6 +1566,7 @@ dependencies = [ "bitcrypto", "bs58 0.4.0", "cbc", + "cfg-if 1.0.0", "cipher 0.4.4", "common", "derive_more", @@ -1595,6 +1596,7 @@ dependencies = [ "serde_derive", "serde_json", "sha2 0.10.7", + "tokio", "trezor", "wasm-bindgen-test", "web3", diff --git a/mm2src/crypto/Cargo.toml b/mm2src/crypto/Cargo.toml index 34279a2ce3..1c6575dd6f 100644 --- a/mm2src/crypto/Cargo.toml +++ b/mm2src/crypto/Cargo.toml @@ -48,10 +48,15 @@ trezor = { path = "../trezor" } zeroize = { version = "1.5", features = ["zeroize_derive"] } [target.'cfg(target_arch = "wasm32")'.dependencies] +cfg-if = "1.0" mm2_eth = { path = "../mm2_eth" } mm2_metamask = { path = "../mm2_metamask" } wasm-bindgen-test = { version = "0.3.2" } web3 = { git = "https://github.com/KomodoPlatform/rust-web3", tag = "v0.19.0", default-features = false } +[dev-dependencies] +cfg-if = "1.0" +tokio = { version = "1.20", default-features = false } + [features] trezor-udp = ["trezor/trezor-udp"] diff --git a/mm2src/crypto/src/decrypt.rs b/mm2src/crypto/src/decrypt.rs index 18aaf1c1b3..f85f5c8928 100644 --- a/mm2src/crypto/src/decrypt.rs +++ b/mm2src/crypto/src/decrypt.rs @@ -30,11 +30,6 @@ impl From for DecryptionError { /// - It verifies the HMAC tag before decrypting to ensure the integrity of the data. /// - It creates an AES-256-CBC cipher instance and decrypts the ciphertext with the provided key and the decoded IV. /// -/// # Arguments -/// * `encrypted_data` - A reference to the [`EncryptedData`] containing the encrypted data and related metadata. -/// * `key_aes` - A byte array reference to the AES key used for decryption. -/// * `key_hmac` - A byte array reference to the HMAC key used for verifying the HMAC tag. -/// /// # Returns /// `MmResult, DecryptionError>` - The result is either a byte vector containing the decrypted data, /// or a [`DecryptionError`] in case of failure. diff --git a/mm2src/crypto/src/encrypt.rs b/mm2src/crypto/src/encrypt.rs index 094c7a2dab..57d2af70f9 100644 --- a/mm2src/crypto/src/encrypt.rs +++ b/mm2src/crypto/src/encrypt.rs @@ -73,12 +73,6 @@ pub struct EncryptedData { /// - It creates an HMAC tag for verifying the integrity of the encrypted data. /// - It constructs an [`EncryptedData`] instance containing all the necessary components for decryption. /// -/// # Arguments -/// * `data` - A byte slice reference to the data that needs to be encrypted. -/// * `key_derivation_details` - A [`KeyDerivationDetails`] instance containing detailed information about the key derivation process. -/// * `key_aes` - A byte array reference to the AES key used for encryption. -/// * `key_hmac` - A byte array reference to the HMAC key used for creating the HMAC tag. -/// /// # Returns /// `MmResult` - The result is either an [`EncryptedData`] /// struct containing all the necessary components for decryption, or an [`EncryptionError`] in case of failure. diff --git a/mm2src/crypto/src/key_derivation.rs b/mm2src/crypto/src/key_derivation.rs index 1742e2c973..576ed463b9 100644 --- a/mm2src/crypto/src/key_derivation.rs +++ b/mm2src/crypto/src/key_derivation.rs @@ -98,11 +98,6 @@ pub enum KeyDerivationDetails { /// Derives AES and HMAC keys from a given password and salts for mnemonic encryption/decryption. /// -/// # Arguments -/// * `password` - The password used for key derivation. -/// * `salt_aes` - The salt used for AES key derivation. -/// * `salt_hmac` - The salt used for HMAC key derivation. -/// /// # Returns /// A tuple containing the AES key and HMAC key as byte arrays, or a `MnemonicError` in case of failure. #[allow(dead_code)] @@ -159,11 +154,6 @@ fn derive_key_from_path(master_node: &[u8], path: &str) -> MmResult<[u8; 32], Ke /// Derives encryption and authentication keys from the master private key using [SLIP-0021](https://github.com/satoshilabs/slips/blob/master/slip-0021.md). /// -/// # Arguments -/// * `master_secret` - The master private key used for key derivation. Can be a BIP-39 seed if this is used to derive keys for wallet data/files encryption. -/// * `encryption_path` - The derivation path used for encryption key derivation. -/// * `authentication_path` - The derivation path used for authentication key derivation. -/// /// # Returns /// A tuple containing the encryption and authentication keys as byte arrays, or a [`KeyDerivationError`] in case of failure. #[allow(dead_code)] diff --git a/mm2src/crypto/src/mnemonic.rs b/mm2src/crypto/src/mnemonic.rs index c57c7337ac..c92e23c05b 100644 --- a/mm2src/crypto/src/mnemonic.rs +++ b/mm2src/crypto/src/mnemonic.rs @@ -42,9 +42,6 @@ impl From for MnemonicError { /// This function creates a new mnemonic passphrase using a specified word count and randomness source. /// The generated mnemonic is intended for use as a wallet mnemonic. /// -/// # Arguments -/// * `ctx` - The `MmArc` context containing the application configuration. -/// /// # Returns /// `MmInitResult` - The generated mnemonic passphrase or an error if generation fails. /// @@ -65,10 +62,6 @@ pub fn generate_mnemonic(ctx: &MmArc) -> MmResult { /// - It encrypts the mnemonic using AES-256-CBC. /// - It creates an HMAC tag for verifying the integrity and authenticity of the encrypted data. /// -/// # Arguments -/// * `mnemonic` - A `&str` reference to the mnemonic that needs to be encrypted. -/// * `password` - A `&str` reference to the password used for key derivation. -/// /// # Returns /// `MmResult` - The result is either an `EncryptedData` /// struct containing all the necessary components for decryption, or a `MnemonicError` in case of failure. @@ -105,10 +98,6 @@ pub fn encrypt_mnemonic(mnemonic: &str, password: &str) -> MmResult` - The result is either a `Mnemonic` instance if decryption is successful, /// or a `MnemonicError` in case of failure. @@ -141,12 +130,17 @@ pub fn decrypt_mnemonic(encrypted_data: &EncryptedData, password: &str) -> MmRes Ok(mnemonic) } -#[cfg(test)] +#[cfg(any(test, target_arch = "wasm32"))] mod tests { use super::*; + use common::cross_test; - #[test] - fn test_encrypt_decrypt_mnemonic() { + common::cfg_wasm32! { + use wasm_bindgen_test::*; + wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); + } + + cross_test!(test_encrypt_decrypt_mnemonic, { let mnemonic = "tank abandon bind salon remove wisdom net size aspect direct source fossil"; let password = "password"; @@ -167,10 +161,9 @@ mod tests { // Verify if decrypted mnemonic matches the original assert_eq!(decrypted_mnemonic, parsed_mnemonic); - } + }); - #[test] - fn test_mnemonic_with_last_byte_zero() { + cross_test!(test_mnemonic_with_last_byte_zero, { let mnemonic = "tank abandon bind salon remove wisdom net size aspect direct source fossil\0".to_string(); let password = "password"; @@ -188,5 +181,5 @@ mod tests { .unwrap_err() .to_string() .contains("mnemonic contains an unknown word (word 11)")); - } + }); } diff --git a/mm2src/crypto/src/slip21.rs b/mm2src/crypto/src/slip21.rs index bf8cb4eeb2..bc2bb976d5 100644 --- a/mm2src/crypto/src/slip21.rs +++ b/mm2src/crypto/src/slip21.rs @@ -27,11 +27,6 @@ impl From for SLIP21Error { /// Encrypts data using SLIP-0021 derived keys. /// -/// # Arguments -/// * `data` - The data to be encrypted. -/// * `master_secret` - The master private key used for key derivation. -/// * `derivation_path` - The additional derivation path for key derivation. -/// /// # Returns /// `MmResult` - The encrypted data along with metadata for decryption, or an error. #[allow(dead_code)] @@ -58,10 +53,6 @@ pub fn encrypt_with_slip21( /// Decrypts data encrypted with SLIP-0021 derived keys. /// -/// # Arguments -/// * `encrypted_data` - The encrypted data. -/// * `master_secret` - The master private key used for key derivation. -/// /// # Returns /// `MmResult, DecryptionError>` - The decrypted data, or an error. #[allow(dead_code)] @@ -85,13 +76,18 @@ pub fn decrypt_with_slip21(encrypted_data: &EncryptedData, master_secret: &[u8; decrypt_data(encrypted_data, &key_aes, &key_hmac).mm_err(|e| SLIP21Error::DecryptionFailed(e.to_string())) } -#[cfg(test)] +#[cfg(any(test, target_arch = "wasm32"))] mod tests { use super::*; + use common::cross_test; use std::convert::TryInto; - #[test] - fn test_encrypt_decrypt_with_slip21() { + common::cfg_wasm32! { + use wasm_bindgen_test::*; + wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); + } + + cross_test!(test_encrypt_decrypt_with_slip21, { let data = b"Example data to encrypt and decrypt using SLIP-0021"; let master_secret = hex::decode("c76c4ac4f4e4a00d6b274d5c39c700bb4a7ddc04fbc6f78e85ca75007b5b495f74a9043eeb77bdd53aa6fc3a0e31462270316fa04b8c19114c8798706cd02ac8").unwrap().try_into().unwrap(); let derivation_path = "test/path"; @@ -108,5 +104,5 @@ mod tests { // Verify if decrypted data matches the original data assert_eq!(data.to_vec(), decrypted_data); - } + }); } diff --git a/mm2src/mm2_main/src/lp_wallet.rs b/mm2src/mm2_main/src/lp_wallet.rs index aaa443b96a..2726246909 100644 --- a/mm2src/mm2_main/src/lp_wallet.rs +++ b/mm2src/mm2_main/src/lp_wallet.rs @@ -11,14 +11,14 @@ cfg_wasm32! { use crate::mm2::lp_wallet::mnemonics_wasm_db::{WalletsDb, WalletsDBError}; use mm2_core::mm_ctx::from_ctx; use mm2_db::indexed_db::{ConstructibleDb, DbLocked, InitDbResult}; - use mnemonics_wasm_db::{read_encrypted_passphrase, save_encrypted_passphrase}; + use mnemonics_wasm_db::{read_encrypted_passphrase_if_available, save_encrypted_passphrase}; use std::sync::Arc; type WalletsDbLocked<'a> = DbLocked<'a, WalletsDb>; } cfg_native! { - use mnemonics_storage::{read_encrypted_passphrase, save_encrypted_passphrase, WalletsStorageError}; + use mnemonics_storage::{read_encrypted_passphrase_if_available, save_encrypted_passphrase, WalletsStorageError}; } #[cfg(not(target_arch = "wasm32"))] @@ -119,25 +119,22 @@ async fn encrypt_and_save_passphrase( .mm_err(|e| WalletInitError::WalletsStorageError(e.to_string())) } -/// Reads and decrypts the passphrase from a file associated with the given wallet name. +/// Reads and decrypts the passphrase from a file associated with the given wallet name, if available. /// -/// This function first reads the passphrase from the file. Since the passphrase is stored in an encrypted -/// format, it decrypts it before returning. -/// -/// # Arguments -/// * `ctx` - The `MmArc` context containing the application state and configuration. -/// * `wallet_name` - The name of the wallet for which the passphrase is to be retrieved. +/// This function first checks if a passphrase is available. If a passphrase is found, +/// since it is stored in an encrypted format, it decrypts it before returning. If no passphrase is found, +/// it returns `None`. /// /// # Returns /// `MmInitResult` - The decrypted passphrase or an error if any operation fails. /// /// # Errors /// Returns specific `MmInitError` variants for different failure scenarios. -async fn read_and_decrypt_passphrase( +async fn read_and_decrypt_passphrase_if_available( ctx: &MmArc, wallet_password: &str, ) -> MmResult, ReadPassphraseError> { - match read_encrypted_passphrase(ctx) + match read_encrypted_passphrase_if_available(ctx) .await .mm_err(|e| ReadPassphraseError::WalletsStorageError(e.to_string()))? { @@ -172,7 +169,7 @@ async fn retrieve_or_create_passphrase( wallet_name: &str, wallet_password: &str, ) -> WalletInitResult> { - match read_and_decrypt_passphrase(ctx, wallet_password).await? { + match read_and_decrypt_passphrase_if_available(ctx, wallet_password).await? { Some(passphrase_from_file) => { // If an existing passphrase is found, return it Ok(Some(passphrase_from_file)) @@ -194,7 +191,7 @@ async fn confirm_or_encrypt_and_store_passphrase( passphrase: &str, wallet_password: &str, ) -> WalletInitResult> { - match read_and_decrypt_passphrase(ctx, wallet_password).await? { + match read_and_decrypt_passphrase_if_available(ctx, wallet_password).await? { Some(passphrase_from_file) if passphrase == passphrase_from_file => { // If an existing passphrase is found and it matches the provided passphrase, return it Ok(Some(passphrase_from_file)) @@ -221,7 +218,7 @@ async fn decrypt_validate_or_save_passphrase( // Decrypt the provided encrypted passphrase let decrypted_passphrase = decrypt_mnemonic(&encrypted_passphrase_data, wallet_password)?.to_string(); - match read_and_decrypt_passphrase(ctx, wallet_password).await? { + match read_and_decrypt_passphrase_if_available(ctx, wallet_password).await? { Some(passphrase_from_file) if decrypted_passphrase == passphrase_from_file => { // If an existing passphrase is found and it matches the decrypted passphrase, return it Ok(Some(decrypted_passphrase)) @@ -301,9 +298,6 @@ fn initialize_crypto_context(ctx: &MmArc, passphrase: &str) -> WalletInitResult< /// and handles encryption and storage as needed. /// - Initializes the cryptographic context based on the `enable_hd` configuration. /// -/// # Arguments -/// * `ctx` - The `MmArc` context containing the application state and configuration. -/// /// # Returns /// `MmInitResult<()>` - Result indicating success or failure of the initialization process. /// @@ -459,11 +453,6 @@ impl From for GetMnemonicError { /// Retrieves the wallet mnemonic in the requested format. /// -/// # Arguments -/// -/// * `ctx` - The [`MmArc`] context containing the application state and configuration. -/// * `req` - The [`GetMnemonicRequest`] containing the requested mnemonic format. -/// /// # Returns /// /// A `Result` type containing: @@ -496,7 +485,7 @@ impl From for GetMnemonicError { pub async fn get_mnemonic_rpc(ctx: MmArc, req: GetMnemonicRequest) -> MmResult { match req.mnemonic_format { MnemonicFormat::Encrypted => { - let encrypted_mnemonic = read_encrypted_passphrase(&ctx) + let encrypted_mnemonic = read_encrypted_passphrase_if_available(&ctx) .await? .ok_or_else(|| GetMnemonicError::InvalidRequest("Wallet passphrase file not found".to_string()))?; Ok(GetMnemonicResponse { @@ -504,7 +493,7 @@ pub async fn get_mnemonic_rpc(ctx: MmArc, req: GetMnemonicRequest) -> MmResult { - let plaintext_mnemonic = read_and_decrypt_passphrase(&ctx, &wallet_password) + let plaintext_mnemonic = read_and_decrypt_passphrase_if_available(&ctx, &wallet_password) .await? .ok_or_else(|| GetMnemonicError::InvalidRequest("Wallet mnemonic file not found".to_string()))?; Ok(GetMnemonicResponse { diff --git a/mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs b/mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs index 0f849ac523..3cf40e61fb 100644 --- a/mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs +++ b/mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs @@ -7,8 +7,6 @@ type WalletsStorageResult = Result>; #[derive(Debug, Deserialize, Display, Serialize)] pub enum WalletsStorageError { - #[display(fmt = "{} db file is not writable", path)] - DbFileIsNotWritable { path: String }, #[display(fmt = "Error writing to file: {}", _0)] FsWriteError(String), #[display(fmt = "Error reading from file: {}", _0)] @@ -19,11 +17,6 @@ pub enum WalletsStorageError { /// Saves the passphrase to a file associated with the given wallet name. /// -/// # Arguments -/// -/// * `wallet_name` - The name of the wallet. -/// * `passphrase` - The passphrase to save. -/// /// # Returns /// Result indicating success or an error. pub(super) async fn save_encrypted_passphrase( @@ -32,25 +25,17 @@ pub(super) async fn save_encrypted_passphrase( encrypted_passphrase_data: &EncryptedData, ) -> WalletsStorageResult<()> { let wallet_path = ctx.wallet_file_path(wallet_name); - ensure_file_is_writable(&wallet_path).map_to_mm(|_| WalletsStorageError::DbFileIsNotWritable { - path: wallet_path.display().to_string(), - })?; + ensure_file_is_writable(&wallet_path).map_to_mm(WalletsStorageError::FsWriteError)?; mm2_io::fs::write_json(encrypted_passphrase_data, &wallet_path, true) .await .mm_err(|e| WalletsStorageError::FsWriteError(e.to_string())) } -/// Reads the encrypted passphrase data from the file associated with the given wallet name. +/// Reads the encrypted passphrase data from the file associated with the given wallet name, if available. /// -/// This function is responsible for retrieving the encrypted passphrase data from a file. +/// This function is responsible for retrieving the encrypted passphrase data from a file, if it exists. /// The data is expected to be in the format of `EncryptedData`, which includes /// all necessary components for decryption, such as the encryption algorithm, key derivation -/// details, salts, IV, ciphertext, and HMAC tag. -/// -/// # Arguments -/// -/// * `ctx` - The `MmArc` context, providing access to application configuration and state. -/// * `wallet_name` - The name of the wallet whose encrypted passphrase data is to be read. /// /// # Returns /// `io::Result` - The encrypted passphrase data or an error if the @@ -59,7 +44,7 @@ pub(super) async fn save_encrypted_passphrase( /// # Errors /// Returns an `io::Error` if the file cannot be read or the data cannot be deserialized into /// `EncryptedData`. -pub(super) async fn read_encrypted_passphrase(ctx: &MmArc) -> WalletsStorageResult> { +pub(super) async fn read_encrypted_passphrase_if_available(ctx: &MmArc) -> WalletsStorageResult> { let wallet_name = ctx .wallet_name .ok_or(WalletsStorageError::Internal( diff --git a/mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs b/mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs index 1640102344..7f3206df14 100644 --- a/mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs +++ b/mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs @@ -3,8 +3,8 @@ use async_trait::async_trait; use crypto::EncryptedData; use mm2_core::mm_ctx::MmArc; use mm2_core::DbNamespaceId; -use mm2_db::indexed_db::{DbIdentifier, DbInstance, DbUpgrader, IndexedDb, IndexedDbBuilder, InitDbResult, - OnUpgradeError, OnUpgradeResult, TableSignature}; +use mm2_db::indexed_db::{DbIdentifier, DbInstance, DbTransactionError, DbUpgrader, IndexedDb, IndexedDbBuilder, + InitDbError, InitDbResult, OnUpgradeError, OnUpgradeResult, TableSignature}; use mm2_err_handle::prelude::*; use std::collections::HashMap; use std::ops::Deref; @@ -28,6 +28,14 @@ pub enum WalletsDBError { Internal(String), } +impl From for WalletsDBError { + fn from(e: InitDbError) -> Self { WalletsDBError::Internal(e.to_string()) } +} + +impl From for WalletsDBError { + fn from(e: DbTransactionError) -> Self { WalletsDBError::Internal(e.to_string()) } +} + #[derive(Debug, Deserialize, Serialize)] struct MnemonicsTable { wallet_name: String, @@ -90,19 +98,10 @@ pub(super) async fn save_encrypted_passphrase( encrypted_passphrase_data: &EncryptedData, ) -> WalletsDBResult<()> { let wallets_ctx = WalletsContext::from_ctx(ctx).map_to_mm(WalletsDBError::Internal)?; - let db = wallets_ctx - .wallets_db() - .await - .mm_err(|e| WalletsDBError::Internal(e.to_string()))?; - - let transaction = db - .transaction() - .await - .mm_err(|e| WalletsDBError::Internal(e.to_string()))?; - let table = transaction - .table::() - .await - .mm_err(|e| WalletsDBError::Internal(e.to_string()))?; + + let db = wallets_ctx.wallets_db().await?; + let transaction = db.transaction().await?; + let table = transaction.table::().await?; let mnemonics_table_item = MnemonicsTable { wallet_name: wallet_name.to_string(), @@ -113,29 +112,17 @@ pub(super) async fn save_encrypted_passphrase( } })?, }; - table - .add_item(&mnemonics_table_item) - .await - .mm_err(|e| WalletsDBError::Internal(e.to_string()))?; + table.add_item(&mnemonics_table_item).await?; Ok(()) } -pub(super) async fn read_encrypted_passphrase(ctx: &MmArc) -> WalletsDBResult> { +pub(super) async fn read_encrypted_passphrase_if_available(ctx: &MmArc) -> WalletsDBResult> { let wallets_ctx = WalletsContext::from_ctx(ctx).map_to_mm(WalletsDBError::Internal)?; - let db = wallets_ctx - .wallets_db() - .await - .mm_err(|e| WalletsDBError::Internal(e.to_string()))?; - - let transaction = db - .transaction() - .await - .mm_err(|e| WalletsDBError::Internal(e.to_string()))?; - let table = transaction - .table::() - .await - .mm_err(|e| WalletsDBError::Internal(e.to_string()))?; + + let db = wallets_ctx.wallets_db().await?; + let transaction = db.transaction().await?; + let table = transaction.table::().await?; let wallet_name = ctx .wallet_name @@ -144,16 +131,16 @@ pub(super) async fn read_encrypted_passphrase(ctx: &MmArc) -> WalletsDBResult serde_json::from_str(&wallet_table_item.encrypted_mnemonic) - .map_to_mm(|e| WalletsDBError::DeserializationError { - field: "encrypted_mnemonic".to_string(), - error: e.to_string(), - }), - Ok(None) => Ok(None), - Err(e) => MmError::err(WalletsDBError::Internal(format!( - "Error retrieving encrypted passphrase: {}", - e - ))), - } + table + .get_item_by_unique_index("wallet_name", wallet_name) + .await? + .map(|(_item_id, wallet_table_item)| { + serde_json::from_str(&wallet_table_item.encrypted_mnemonic).map_to_mm(|e| { + WalletsDBError::DeserializationError { + field: "encrypted_mnemonic".to_string(), + error: e.to_string(), + } + }) + }) + .transpose() }