feat(crypto): mnemonic generation/encryption/decryption/storage #2014

shamardy · 2023-11-24T18:11:34Z

#1939

Checklist

Seed generation using rust-bip39
Deriving a key from password using argon2
Encrypting the seed/mnemonic using AES-256-CBC and MAC. Crates used: aes, cbc, cipher, hmac
Saving encrypted seed on start if it's new or reading it if it was saved before, for non-wasm and wasm targets.

Todo:

Future:

Support different key derivations and encryption algorithms for the framework (SDK)

…ge functionality

…nssl

…k for mm2

artemii235

Amazing work! I have a few minor comments 🙂

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs

mm2src/crypto/src/mnemonic.rs

artemii235 · 2023-11-30T04:34:00Z

mm2src/mm2_main/src/lp_wallet.rs

+    // The passphrase will then be encrypted and saved whether it was generated or provided.
+    let wallet_name = deserialize_config_field::<Option<String>>(&ctx, "wallet_name")?;
+
+    let passphrase = match (wallet_name, passphrase) {


Relying on existence of wallet_name might a bit confusing (just IMHO/nitpick). What do you think about using more explicit config field like "use_encrypted_wallet"?

Do you mean that use_encrypted_wallet is a bool?

I wanted to cover the multi seed support for GUIs, so we need different wallet names as identifiers.

1 - For new wallet creation, only a wallet name and a password is passed, a seed is generated by mm2 and saved in wallet_name.dat

2 - If a wallet name already exists, we decrypt the wallet_name.dat using the password

3 - To import a wallet, we use wallet_name, passphrase and password

I guess I didn't cover the case where a seed file is uploaded, so it will be provided as encrypted to mm2, I can add this case I guess. Also, I can maybe use a struct for all the wallet data, so the parameter passed can look like the below:

"wallet": { "name": "wallet_name", // Optional for legacy operations where the passphrase is provided each time from GUIs "password": "PASSWORD", "passphrase": "PASSPHRASE" // This is an optional enum for either encrypted or plaintext }

This will not be easy to use on cli though.

One more note: For GUIs to migrate, they will need to provide the seed phrase, wallet name, password from their files similar to how importing a wallet works. GUIs can then delete their seed file which uses different encryption algorithms that we plan to deprecate.

What is your opinion on all this @artemii235 :)

I just understood now that you meant to use use_encrypted_wallet in addition to wallet_name, Will this be needed if I used a struct as shown in the previous comment?

I think it's not needed actually 🙂 Thanks for the detailed explanation!

Yeah, I also shouldn't use a struct as I suggested to maintain backward compatibility for the passphrase field. I will add the option to pass an encrypted passphrase for the case of uploading a seed file in next iteration, so I will leave this unresolved until then.

I will add the option to pass an encrypted passphrase for the case of uploading a seed file in next iteration, so I will leave this unresolved until then.

Done

…e upgrade logic

dimxy · 2023-11-30T10:38:28Z

Do I understand it right that we sometimes call 'mnemonic' as 'passphrase' in the code (as I can see in bip39 'passphrase' is an optional extra string to additionally protect the mnemonic)?

laruh

Impressive work!
I have just comment now

mm2src/crypto/src/mnemonic.rs

shamardy · 2023-11-30T11:42:17Z

Do I understand it right that we sometimes call 'mnemonic' as 'passphrase' in the code (as I can see in bip39 'passphrase' is an optional extra string to additionally protect the mnemonic)?

Yeah, passphrase is kept for backward compatibility since this is what is used in config. In lp_wallet.rs context passphrase is used, mnemonic is used in crypto crate. Do you think I need to modify any of the mnemonic or passphrase usages?

dimxy · 2023-11-30T14:18:18Z

Do I understand it right that we sometimes call 'mnemonic' as 'passphrase' in the code (as I can see in bip39 'passphrase' is an optional extra string to additionally protect the mnemonic)?

Yeah, passphrase is kept for backward compatibility since this is what is used in config. In lp_wallet.rs context passphrase is used, mnemonic is used in crypto crate. Do you think I need to modify any of the mnemonic or passphrase usages?

It was a bit confusing for me first time. Maybe we could make a separate PR for this

shamardy · 2023-11-30T16:09:47Z

It was a bit confusing for me first time. Maybe we could make a separate PR for this

I meant mnemonic named as passphrase. Not "bip39 passphrase", we don't use that at all. It is confusing 😕

artemii235

One more note

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs

…DEFAULT_WORD_COUNT = 12

shamardy · 2023-12-01T14:13:45Z

I changed the default word count of mnemonics to 12 since this is what is used in GUIs and is sufficient. Thank you @dimxy for the hint.

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs

artemii235

🔥

…ation method

onur-ozkan

Thank you for the work, here are some notes from the first review:

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs

onur-ozkan · 2024-02-13T08:28:18Z

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs

+/// Reads the encrypted passphrase data from the file associated with the given wallet name.
+///
+/// This function is responsible for retrieving the encrypted passphrase data from a file.
+/// 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.


Why not keep this encrypted data in a database (similar to how it's done in WASM)? Having separate files could cause problems for future development (e.g., if you need to perform transactional operations, you'll have to implement additional file locks).

I opted for file-based storage for encrypted passphrase/mnemonic data to allow portability, this also aligns with the industry standards afaik (e.g. wallet.dat file for bitcoin core and for komodo daemon), please also check the use case here #2014 (comment) where the user can upload a seed file.
For encrypted wallet data using the seed (e.g. swap files), how to save the data is not implemented yet. I will probably add methods for both file storage and database. Will add this comment to the checklist here #2014 (comment) to remember.

Storing them in the database wouldn't change the behaviour for users I believe. You just change the way you keep its data for mm2 (e.g., you can still upload seed files). Is there any other reason for keeping them as a file on native (non-WASM) mode ? If not, I would keep this behaviour simple and same on all supported mm2 platforms and keep the mm2 internal data in a central storage (in this case mm2 db).

I think storing encrypted passphrase/mnemonic data in a separate file instead of the main mm2 db makes backups easier for end users. They can just backup a small(KB sized) encrypted file instead of a possibly GB sized mm2 db.

Also, it will be easier to document recovery of passphrase from backup (independent of using mm2) if it was a separate file instead of a part of a db

If not, I would keep this behaviour simple and same on all supported mm2 platforms and keep the mm2 internal data in a central storage (in this case mm2 db).

We would need a new global database for all mnemonics/seeds, since MM2.db path depends on the public key hash, which in turn depends on the seed. Please note that mm2 can be initialized with only one wallet/mnemonic at a time for now and that MM2.db or MM2-shared.db is specific to each wallet/mnemonic, and not shared across all wallets. The seed file is retrieved when starting mm2, based on the wallet name, there are no additional transactional operations for this file, except when using the get_mnemonic RPC.

We now have 2 options:

1 - Create a file for each wallet's encrypted mnemonic in the the db root directory. This can even allow us to support importing/exporting seeds to/from different wallets by supporting different file formats in the future, e.g. https://cryptobook.nakov.com/symmetric-key-ciphers/ethereum-wallet-encryption. This is why I mentioned the industry standards.

komodo-defi-framework/mm2src/mm2_core/src/mm_ctx.rs

Lines 289 to 293 in 66085b0

#[cfg(not(target_arch = "wasm32"))]

pub fn wallet_file_path(&self, wallet_name: &str) -> PathBuf {

let db_root = path_to_db_root(self.conf["dbdir"].as_str());

db_root.join(wallet_name.to_string() + ".dat")

}

2 - Create a global database in the db root directory that is only used when starting mm2 and for one RPC. If a user wants to import their seed file to another wallet without using mm2 they would have to access the database to get it.

I think option 1 is better :)

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs

mm2src/crypto/src/slip21.rs

- Remove arguments descriptions in doc comments. - Fix read passphrase functions names by adding `if_available`. - Improve cross-platform test compatibility.

dimxy · 2024-02-15T10:46:49Z

mm2src/mm2_main/src/lp_wallet.rs

+///     Err(e) => println!("Error: {:?}", e),
+/// }
+/// ```
+pub async fn get_mnemonic_rpc(ctx: MmArc, req: GetMnemonicRequest) -> MmResult<GetMnemonicResponse, GetMnemonicError> {


I wonder maybe we could have a special rpc to return if mnemonic file was created. So GUI can ensure that migration to the api mnemonic has already happened and begin to use it

It's not need IMHO, using the new wallet_name config field ensures that

komodo-defi-framework/mm2src/mm2_main/src/lp_wallet.rs

Lines 159 to 162 in 4f666c1

// New approach for passphrase, `wallet_name` is needed in the config to enable multi-wallet support.

// In this case the passphrase will be generated if not provided.

// The passphrase will then be encrypted and saved whether it was generated or provided.

let wallet_name = deserialize_config_field::<Option<String>>(ctx, "wallet_name")?;

if not used, then it will be the same behaviour as before this PR. GUIs currently load the seed file and decrypt it then pass the mnemonic to mm2 initialization parameters. To migrate, they would pass the mnemonic with the password and the wallet name.

dimxy · 2024-02-15T10:50:10Z

mm2src/mm2_main/src/lp_wallet.rs

+        MnemonicFormat::PlainText(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()))?;


I guess both 'Wallet passphrase file' and 'Wallet mnemonic file' are actually the same file. I think this error may confuse the user and maybe it is better to return the same error description in both cases?

mm2src/mm2_main/src/lp_wallet.rs

mm2src/crypto/src/key_derivation.rs

mm2src/crypto/src/encrypt.rs

mm2src/crypto/src/key_derivation.rs

dimxy · 2024-02-15T11:50:12Z

mm2src/mm2_main/src/lp_wallet.rs

+    MnemonicError(String),
+    #[display(fmt = "Error initializing crypto context: {}", _0)]
+    CryptoInitError(String),
+    Internal(String),


Maybe good to name internal errors everywhere identically ('InternalError')

- Rename `SYMMETRIC_KEY_SEED` to `MASTER_NODE_HMAC_KEY` for clarity - Change `initialize_wallet_passphrase` to take a reference to `MmArc` - Remove unnecessary comments and fix some errors

onur-ozkan

Left some dependency information (please note that I haven't checked for sec advisories on them).

mm2src/crypto/Cargo.toml

onur-ozkan · 2024-02-16T09:33:36Z

mm2src/crypto/Cargo.toml

@@ -7,17 +7,24 @@ edition = "2018"
 doctest = false

 [dependencies]
+aes = "0.8.3"


Q: Can we use version 0.7.5 instead of duplicating this dependency?

I wanted to use the latest version for such an important encryption, after this #1957 is merged I can try to update 0.7.5 to 0.8.3 instead in librustzcash

mm2src/crypto/Cargo.toml

…nctions

onur-ozkan

Thank you for the fixes.

shamardy · 2024-04-08T19:47:00Z

Copied none finished items in this checklist to the parent issue #1939 (comment). Will resolve conflicts then write docs for QA and merge this PR.

shamardy · 2024-04-09T02:18:08Z

@KomodoPlatform/qa This Pull Request introduces a new optional parameter, wallet_name, to the mm2 configuration. If this parameter is not provided, the passphrase parameter will function as before. If a passphrase is found, it will be used as the mnemonic without being stored in the mm2 storage/database. If a passphrase is not found, mm2 will start in no-login mode.

When wallet_name is used, wallet_password must also be provided. This leads to one of the following scenarios:

If no passphrase is provided:
1. mm2 checks if this wallet_name has been used before, which would mean that there is an encrypted passphrase saved for it. If it has been used, the seed is decrypted using the password and mm2 is initialized with it.
2. If wallet_name has not been used before, mm2 generates a passphrase/mnemonic that is used to initialize mm2. The passphrase is then encrypted using the password and saved for future use with the same wallet_name as in point 1.1.
If a passphrase is provided along with wallet_name and wallet_password:
1. If it's a string as before, it means it's plaintext, the passphrase is used to initialize mm2. It's then encrypted using the password and saved under this wallet name. It can be reused as in point 1.1 without passing it again. This mode can be used to migrate passphrases from GUIs to mm2 to use its encryption by passing the passphrase as plaintext. This mode can also be used to import passphrases as plaintext.
2. If the passphrase is passed as an encrypted passphrase (which is a JSON), this is for the upload seed file mode. This is a file that was previously exported using the new API method get_mnemonic. The passphrase is decrypted using the wallet_password and mm2 is initialized with it. It is then saved encrypted under the wallet_name for subsequent use as in point 1.1.

So, when wallet_name is passed, seed management is completely handled by mm2 instead of GUIs. If it's not passed, it's the legacy approach where seed is managed by GUIs and the passphrase/mnemonic is passed to mm2 during initialization.

A new API method called get_mnemonic has been added. The request can be for either the encrypted passphrase (for the GUI to export it as a file) or as a plaintext passphrase (for the GUI to display it). The requests/responses for both modes are:

Request for encrypted passphrase:

{
    "userpass": "{{userpass}}",
    "mmrpc": "2.0",
    "method": "get_mnemonic",
    "params": {
        "format": "encrypted"
    }
}

Response for encrypted passphrase request:

{
  "format": "encrypted",
  "encrypted_mnemonic_data": {
    // Encrypted data
  }
}

Request for plaintext passphrase:

{
    "userpass": "{{userpass}}",
    "mmrpc": "2.0",
    "method": "get_mnemonic",
    "params": {
        "format": "plaintext",
        "password": "password123" // The password used to encrypt the passphrase when the wallet was created
    }
}

Response for plaintext passphrase request:

{
  "format": "plaintext",
  "mnemonic": "your_mnemonic_here"
}

* dev: feat(nft-swap): nft swap protocol v2 POC (KomodoPlatform#2084) fix(zcoin): syncing and activation improvements (KomodoPlatform#2089) feat(crypto): mnemonic generation/encryption/decryption/storage (KomodoPlatform#2014) fix(eth): error handling in RPCs (KomodoPlatform#2090)

wip: Implement mnemonic generation, encryption, decryption, and stora…

03c0e59

…ge functionality

shamardy added the in progress Changes will be made from the author label Nov 24, 2023

Wasm encryption/decryption and wallets mnemonic files storage

714e72d

shamardy marked this pull request as ready for review November 28, 2023 09:57

shamardy requested a review from dimxy November 28, 2023 09:58

shamardy added 2 commits November 28, 2023 12:44

rollback some Cargo.lock changes that were done due to the use of ope…

d0e3f44

…nssl

rollback some adex-cli Cargo.lock changes to be the same as cargo.loc…

2e50871

…k for mm2

artemii235 reviewed Nov 30, 2023

View reviewed changes

Fix review notes: use consts where applicable, refactor MnemonicsTabl…

eb04b12

…e upgrade logic

laruh reviewed Nov 30, 2023

View reviewed changes

mm2src/crypto/src/mnemonic.rs Outdated Show resolved Hide resolved

Fix review notes: add EncryptionAlgorithm enum

825ecdb

artemii235 requested changes Dec 1, 2023

View reviewed changes

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs Outdated Show resolved Hide resolved

Fix review notes: fix on_upgrade_needed fn for mnemonics table, make …

8365567

…DEFAULT_WORD_COUNT = 12

dimxy reviewed Dec 1, 2023

View reviewed changes

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs Outdated Show resolved Hide resolved

dimxy reviewed Dec 1, 2023

View reviewed changes

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs Outdated Show resolved Hide resolved

Fix review notes: fix doc comments of read_encrypted_passphrase

69117b8

artemii235 previously approved these changes Dec 4, 2023

View reviewed changes

allow importing of an encrypted passphrase

3bdb2d5

shamardy dismissed artemii235’s stale review via 3bdb2d5 January 9, 2024 12:44

shamardy added 6 commits January 10, 2024 13:27

refactor handle_passphrase_logic

2df7834

make global wallets db part of ctx

839beb6

fix wasm clippy

4b7c014

add get_mnemonic rpc

5a65a48

add SLIP-0021 keys derivation

6d5e271

add encrypt.rs for encryption of general data regardless of key deriv…

c37316e

…ation method

onur-ozkan reviewed Feb 13, 2024

View reviewed changes

onur-ozkan mentioned this pull request Feb 14, 2024

feat(nft): enable eth with non fungible tokens #2049

Merged

Review Fixes:

4f666c1

- Remove arguments descriptions in doc comments. - Fix read passphrase functions names by adding `if_available`. - Improve cross-platform test compatibility.

dimxy reviewed Feb 15, 2024

View reviewed changes

onur-ozkan reviewed Feb 15, 2024

View reviewed changes

dimxy reviewed Feb 15, 2024

View reviewed changes

Review fixes:

ba0fb59

- Rename `SYMMETRIC_KEY_SEED` to `MASTER_NODE_HMAC_KEY` for clarity - Change `initialize_wallet_passphrase` to take a reference to `MmArc` - Remove unnecessary comments and fix some errors

onur-ozkan reviewed Feb 16, 2024

View reviewed changes

Review fixes: Update base64 crate to 0.21.2 and replace deprecated fu…

eb26362

…nctions

onur-ozkan previously approved these changes Feb 18, 2024

View reviewed changes

shamardy mentioned this pull request Mar 24, 2024

feat(hd_wallet): evm hd wallet and trezor #1979

Merged

8 tasks

shamardy added under review 2.1.0-beta sec-review QA docs deps-review and removed in progress Changes will be made from the author labels Apr 8, 2024

shamardy mentioned this pull request Apr 8, 2024

Seed generation, encryption and storage support #1939

Open

Merge remote-tracking branch 'origin/dev' into feat-seed-gen

47cdc19

shamardy dismissed onur-ozkan’s stale review via 47cdc19 April 8, 2024 20:11

shamardy merged commit ffe761e into dev Apr 9, 2024
23 of 30 checks passed

shamardy deleted the feat-seed-gen branch April 9, 2024 02:23

shamardy mentioned this pull request May 16, 2024

feat(ETH): eip1559 gas fee estimator and rpcs #2051

Merged

19 tasks

shamardy mentioned this pull request Jul 24, 2024

Add docs for mnemonics configs and APIs KomodoPlatform/komodo-docs-mdx#299

Open

shamardy mentioned this pull request Aug 1, 2024

chore(release): v2.1.0-beta #2043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(crypto): mnemonic generation/encryption/decryption/storage #2014

feat(crypto): mnemonic generation/encryption/decryption/storage #2014

shamardy commented Nov 24, 2023 •

edited

Loading

artemii235 left a comment

artemii235 Nov 30, 2023

shamardy Nov 30, 2023 •

edited

Loading

shamardy Nov 30, 2023 •

edited

Loading

artemii235 Dec 1, 2023

shamardy Dec 1, 2023 •

edited

Loading

shamardy Jan 10, 2024

dimxy commented Nov 30, 2023

laruh left a comment

shamardy commented Nov 30, 2023

dimxy commented Nov 30, 2023

shamardy commented Nov 30, 2023

artemii235 left a comment

shamardy commented Dec 1, 2023

artemii235 left a comment

onur-ozkan left a comment

onur-ozkan Feb 13, 2024

shamardy Feb 14, 2024 •

edited

Loading

onur-ozkan Feb 15, 2024 •

edited

Loading

gcharang Feb 15, 2024

shamardy Feb 16, 2024

dimxy Feb 15, 2024

shamardy Feb 16, 2024

dimxy Feb 15, 2024

shamardy Feb 16, 2024

dimxy Feb 15, 2024

shamardy Feb 16, 2024

onur-ozkan left a comment

onur-ozkan Feb 16, 2024

shamardy Feb 16, 2024

onur-ozkan left a comment

shamardy commented Apr 8, 2024

shamardy commented Apr 9, 2024

	#[cfg(not(target_arch = "wasm32"))]
	pub fn wallet_file_path(&self, wallet_name: &str) -> PathBuf {
	let db_root = path_to_db_root(self.conf["dbdir"].as_str());
	db_root.join(wallet_name.to_string() + ".dat")
	}

	// New approach for passphrase, `wallet_name` is needed in the config to enable multi-wallet support.
	// In this case the passphrase will be generated if not provided.
	// The passphrase will then be encrypted and saved whether it was generated or provided.
	let wallet_name = deserialize_config_field::<Option<String>>(ctx, "wallet_name")?;

feat(crypto): mnemonic generation/encryption/decryption/storage #2014

feat(crypto): mnemonic generation/encryption/decryption/storage #2014

Conversation

shamardy commented Nov 24, 2023 • edited Loading

artemii235 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

shamardy Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimxy commented Nov 30, 2023

laruh left a comment

Choose a reason for hiding this comment

shamardy commented Nov 30, 2023

dimxy commented Nov 30, 2023

shamardy commented Nov 30, 2023

artemii235 left a comment

Choose a reason for hiding this comment

shamardy commented Dec 1, 2023

artemii235 left a comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

onur-ozkan Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

shamardy commented Apr 8, 2024

shamardy commented Apr 9, 2024

Request for encrypted passphrase:

Response for encrypted passphrase request:

Request for plaintext passphrase:

Response for plaintext passphrase request:

shamardy commented Nov 24, 2023 •

edited

Loading

shamardy Nov 30, 2023 •

edited

Loading

shamardy Nov 30, 2023 •

edited

Loading

shamardy Dec 1, 2023 •

edited

Loading

shamardy Feb 14, 2024 •

edited

Loading

onur-ozkan Feb 15, 2024 •

edited

Loading