feat(crypto): mnemonic generation/encryption/decryption/storage by shamardy · Pull Request #2014 · GLEECBTC/komodo-defi-framework

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

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

 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 (GLEECBTC#2084) fix(zcoin): syncing and activation improvements (GLEECBTC#2089) feat(crypto): mnemonic generation/encryption/decryption/storage (GLEECBTC#2014) fix(eth): error handling in RPCs (GLEECBTC#2090)

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

03c0e59

…ge functionality

shamardy added the in progress 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 suggested 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 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 and removed in progress labels Apr 8, 2024

shamardy mentioned this pull request Apr 8, 2024

Seed generation, encryption and storage support #1939

Closed

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

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 GLEECBTC/komodo-docs-mdx#299

Closed

shamardy mentioned this pull request Aug 1, 2024

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

Merged

DeckerSU mentioned this pull request Feb 6, 2025

mnemonic encryption/decryption params error #2341

Closed

smk762 mentioned this pull request Feb 11, 2025

Expand KDF seed encryption docs GLEECBTC/komodo-docs-mdx#417

Open

1 task

DeckerSU mentioned this pull request Feb 11, 2025

security: potential path traversal in wallet_file_path #2355

Closed

smk762 mentioned this pull request Mar 24, 2025

[FR] update_wallet_password method #2402

Closed

mariocynicys mentioned this pull request May 6, 2025

Enhancements for crypto seed/keys API and security #2442

Open

6 tasks

Conversation

shamardy commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artemii235 Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

shamardy Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemii235 Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

shamardy Dec 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy Jan 10, 2024

Choose a reason for hiding this comment

Uh oh!

dimxy commented Nov 30, 2023

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy commented Nov 30, 2023

Uh oh!

dimxy commented Nov 30, 2023

Uh oh!

shamardy commented Nov 30, 2023

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy commented Dec 1, 2023

Uh oh!

Uh oh!

Uh oh!

artemii235 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimxy Feb 15, 2024

Choose a reason for hiding this comment

Uh oh!

shamardy Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

onur-ozkan Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

shamardy Feb 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

shamardy commented Apr 8, 2024

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