Skip to content

Commit

Permalink
Review Fixes:
Browse files Browse the repository at this point in the history
- Remove arguments descriptions in doc comments.
- Fix read passphrase functions names by adding `if_available`.
- Improve cross-platform test compatibility.
  • Loading branch information
shamardy committed Feb 14, 2024
1 parent 66085b0 commit 4f666c1
Show file tree
Hide file tree
Showing 10 changed files with 76 additions and 140 deletions.
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions mm2src/crypto/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
5 changes: 0 additions & 5 deletions mm2src/crypto/src/decrypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,6 @@ impl From<base64::DecodeError> 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<Vec<u8>, DecryptionError>` - The result is either a byte vector containing the decrypted data,
/// or a [`DecryptionError`] in case of failure.
Expand Down
6 changes: 0 additions & 6 deletions mm2src/crypto/src/encrypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<EncryptedData, EncryptionError>` - The result is either an [`EncryptedData`]
/// struct containing all the necessary components for decryption, or an [`EncryptionError`] in case of failure.
Expand Down
10 changes: 0 additions & 10 deletions mm2src/crypto/src/key_derivation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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)]
Expand Down
29 changes: 11 additions & 18 deletions mm2src/crypto/src/mnemonic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ impl From<KeyDerivationError> 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<String>` - The generated mnemonic passphrase or an error if generation fails.
///
Expand All @@ -65,10 +62,6 @@ pub fn generate_mnemonic(ctx: &MmArc) -> MmResult<Mnemonic, MnemonicError> {
/// - 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<EncryptedData, MnemonicError>` - The result is either an `EncryptedData`
/// struct containing all the necessary components for decryption, or a `MnemonicError` in case of failure.
Expand Down Expand Up @@ -105,10 +98,6 @@ pub fn encrypt_mnemonic(mnemonic: &str, password: &str) -> MmResult<EncryptedDat
/// - Verifies the integrity and authenticity of the data using the HMAC tag.
/// - Decrypts the mnemonic using AES-256-CBC.
///
/// # Arguments
/// * `encrypted_data` - A reference to the `EncryptedData` containing the encrypted mnemonic and related metadata.
/// * `password` - A `&str` reference to the password used for key derivation.
///
/// # Returns
/// `MmResult<Mnemonic, MnemonicError>` - The result is either a `Mnemonic` instance if decryption is successful,
/// or a `MnemonicError` in case of failure.
Expand Down Expand Up @@ -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";

Expand All @@ -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";

Expand All @@ -188,5 +181,5 @@ mod tests {
.unwrap_err()
.to_string()
.contains("mnemonic contains an unknown word (word 11)"));
}
});
}
22 changes: 9 additions & 13 deletions mm2src/crypto/src/slip21.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,6 @@ impl From<KeyDerivationError> 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<EncryptedData, EncryptionError>` - The encrypted data along with metadata for decryption, or an error.
#[allow(dead_code)]
Expand All @@ -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<Vec<u8>, DecryptionError>` - The decrypted data, or an error.
#[allow(dead_code)]
Expand All @@ -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";
Expand All @@ -108,5 +104,5 @@ mod tests {

// Verify if decrypted data matches the original data
assert_eq!(data.to_vec(), decrypted_data);
}
});
}
37 changes: 13 additions & 24 deletions mm2src/mm2_main/src/lp_wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down Expand Up @@ -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<String>` - 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<Option<String>, ReadPassphraseError> {
match read_encrypted_passphrase(ctx)
match read_encrypted_passphrase_if_available(ctx)
.await
.mm_err(|e| ReadPassphraseError::WalletsStorageError(e.to_string()))?
{
Expand Down Expand Up @@ -172,7 +169,7 @@ async fn retrieve_or_create_passphrase(
wallet_name: &str,
wallet_password: &str,
) -> WalletInitResult<Option<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 an existing passphrase is found, return it
Ok(Some(passphrase_from_file))
Expand All @@ -194,7 +191,7 @@ async fn confirm_or_encrypt_and_store_passphrase(
passphrase: &str,
wallet_password: &str,
) -> WalletInitResult<Option<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 passphrase == passphrase_from_file => {
// If an existing passphrase is found and it matches the provided passphrase, return it
Ok(Some(passphrase_from_file))
Expand All @@ -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))
Expand Down Expand Up @@ -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.
///
Expand Down Expand Up @@ -459,11 +453,6 @@ impl From<ReadPassphraseError> 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:
Expand Down Expand Up @@ -496,15 +485,15 @@ impl From<ReadPassphraseError> for GetMnemonicError {
pub async fn get_mnemonic_rpc(ctx: MmArc, req: GetMnemonicRequest) -> MmResult<GetMnemonicResponse, GetMnemonicError> {
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 {
mnemonic: encrypted_mnemonic.into(),
})
},
MnemonicFormat::PlainText(wallet_password) => {
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 {
Expand Down
23 changes: 4 additions & 19 deletions mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ type WalletsStorageResult<T> = Result<T, MmError<WalletsStorageError>>;

#[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)]
Expand All @@ -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(
Expand All @@ -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<EncryptedPassphraseData>` - The encrypted passphrase data or an error if the
Expand All @@ -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<Option<EncryptedData>> {
pub(super) async fn read_encrypted_passphrase_if_available(ctx: &MmArc) -> WalletsStorageResult<Option<EncryptedData>> {
let wallet_name = ctx
.wallet_name
.ok_or(WalletsStorageError::Internal(
Expand Down
Loading

0 comments on commit 4f666c1

Please sign in to comment.