[gh-755] encrypt w/ ChaCha20Poly1305 and password algorithm Argon2id #856

feliciss · 2023-09-23T02:43:09Z

Summary

Add encryption algorithm ChaCha20Poly1305 to encrypt the key pair.
Add password encryption algorithm Argon2id to encrypt password in plaintext and use input password compared with cipher password.
Decryption takes nonce, ciphertext, tag, password from key store to compare with input password and if successful, retrieve the private key and form a key pair to sign transactions.

ChaCha20Poly1305 is a standardized encryption algorithm (AEAD) widely used by network softwares:

Argon2id is a password encryption algorithm recommended by OWASP in their recent document:

https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html

Closes Encrypt the private key in the keystore file #854 and closes Provide a password option for the cli commands #855

vercel · 2023-09-23T02:43:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
rooch	⬜️ Ignored (Inspect)	Visit Preview		Oct 5, 2023 3:24am

jolestar

When to unlock the private key?

jolestar · 2023-09-24T14:04:16Z

crates/rooch-key/src/key_derive.rs

 ) -> Result<(Addr, KeyPair, KeyPairType, String), anyhow::Error>
 where
    KeyPairType: CoinOperations<Addr, KeyPair>,
 {
    let mnemonic = Mnemonic::new(parse_word_length(word_length)?, Language::English);
-    let seed = Seed::new(&mnemonic, "");
+    let password_str = password.as_deref().unwrap_or("");
+    let seed = Seed::new(&mnemonic, password_str);


I think using the password_str as part of the seed is not a good idea for CLI.

The user needs to save the mnemonic and password and use the mnemonic and password to recover the account. If use a wrong password, will recover a new account.

The user can not change the password.

Some wallet provides a salt to mnemonic, like this, and the user needs to know it is salt, not the password.

The user needs to save the mnemonic and password and use the mnemonic and password to recover the account. If use a wrong password, will recover a new account.

This is a common practice for hardware and software authentication schemes e.g. MetaMask with mnemonic and password, or BitBox02 with password option. Here, password is merely an option for the user and the user can opt in to choose whether to remember it.

The user can not change the password.

If changing password would generate a new account, it is the common practice for the bitcoin software bc we are using bip39 mnemonic word Rust lib. In other words, changing password would mean collapsing the existing seed infrastructure at the first time generated. There's an article introducing the passphrase trade-offs: https://help.blockstream.com/hc/en-us/articles/8712301763737-What-is-a-BIP39-passphrase-#h_01G8K3SK5VBHBYZSX5XS4309YG.

I think it's only a choice made by a user. If the user doesn't opt in with a password, we will still encrypt the key pair from the seed without password.

Some wallet provides a salt to mnemonic, like this, and the user needs to know it is salt, not the password.

The accurate answer is passphrase: https://github.com/bitcoin/bips/blob/master/bip-0039.mediawiki#user-content-From_mnemonic_to_seed. Salt is from a password with mnemonic string according to the original code:

let salt = format!("mnemonic{}", password);

We can add documents about how the seed phrase is constructed.

feliciss · 2023-09-26T00:12:17Z

When to unlock the private key?

When decrypting private key, the user would need to provide:

Mnemonic phrase
Optional password

To decrypt the private key and sign the transaction, by design.

feliciss · 2023-09-26T03:19:33Z

Discussion:

Remove salt, default to "".
Provide an optional password when initializing, and record it using argon2 hash to key store for password verification.
Provide a clap to enter the password in asterisk.

jolestar · 2023-09-28T01:06:41Z

crates/rooch/src/commands/init.rs

@@ -23,6 +24,9 @@ pub struct Init {
    /// Command line input of custom server URL
    #[clap(short = 's', long = "server-url")]
    pub server_url: Option<String>,
+    /// Whether a password should be provided.
+    #[clap(short = 'p', long = "password")]
+    password_required: bool,


Make password_required default value to true? And we can set it to false in the test case.

So, if we want to let password_required accept value, it should be Option<bool>.

#[clap(short = 'p', long = "password")] password_required: Option<bool>,

I've added it.

feliciss · 2023-09-28T03:26:18Z

TODO: do some refactor to key store and retrieve decrypted private keys from key store params in signing functions.

It may take some days and if it isn't included in Q3 release, I will handle this issue at a stable pace.

feliciss · 2023-09-28T03:44:53Z

This design may need further refactor:

nonce shouldn't be used twice for Argon2 and ChaCha20Poly1305 cipher in encrypt_private_key.
In encrypt_password, considering generate a random salt or encrypt current private_key as salt.

jolestar · 2023-09-30T14:42:52Z

crates/rooch-key/src/key_derive.rs

+        let tag = {
+            let start = ciphertext_with_tag.len() - 16;
+            let end = ciphertext_with_tag.len();
+            let mut tag = Vec::with_capacity(16);
+            tag.extend_from_slice(&ciphertext_with_tag[start..end]);
+            tag
+        };


Why split the tag? or include the tag in the ciphertext? I can not find the use case of the tag.

Why split the tag? or include the tag in the ciphertext? I can not find the use case of the tag.

The tag is useless here, and why did I split it was cause the private key should be 32 bytes. The ciphertext is composed of 32 bytes encrypted text and 16 bytes tag thus 48 bytes. In order to achieve key derivations, i.e. parsing the encrypted cipertext to private keys and vice versa, it needs to be split.

2.5. The Poly1305 Algorithm

Poly1305 is a one-time authenticator designed by D. J. Bernstein.
Poly1305 takes a 32-byte one-time key and a message and produces a
16-byte tag. This tag is used to authenticate the message.

https://www.rfc-editor.org/rfc/rfc7539

Add a comment to the tag.

jolestar · 2023-10-01T02:21:31Z

Merge this PR first or wait to implement the password option to decrypt private key for commands, like rooch move run, rooch move publish, etc.

feliciss · 2023-10-03T05:45:13Z

Merge this PR first or wait to implement the password option to decrypt private key for commands, like rooch move run, rooch move publish, etc.

You can merge this PR first and then I will refactor key store, yaml, etc. and test the signature verification feature with encrypted key pair.

The password option has already been added to those commands to decrypt the private key.

jolestar · 2023-10-03T11:36:34Z

crates/rooch/src/commands/move_cli/commands/publish.rs

+            // Prompt for a password if required
+            rpassword::prompt_password("Enter a password to encrypt the keys in the rooch keystore. Press return to have an empty value: ").unwrap()
+        };
+        println!("Your password is {}", password);


Do not need to print the password when decrypting the keypair.

jolestar · 2023-10-03T11:41:03Z

crates/rooch/src/commands/move_cli/commands/publish.rs

+
+    /// Whether a password should be provided
+    #[clap(short = 'p', long = "password")]
+    password_required: Option<bool>,


Is it possible to auto-detect that if the private key is encrypted, then prompt the password input?

The private key is always encrypted. When the user does not enter the password, the password defaults to "".

jolestar · 2023-10-03T11:42:53Z

Some tests need to be fixed.

feliciss · 2023-10-03T22:22:06Z

This command will fail:

rooch move run --function 0x3::empty::empty --sender-account 0xac00d67a15ae97af88aff69cd80befdfe430330a08a6092163de84753cb220c4 --session-key 975a3d052b5ee4bdcc7c8c8a16ae878ef427b4fe6ecb1aa9088b43645111ae20 --password false

The session key 975a3d052b5ee4bdcc7c8c8a16ae878ef427b4fe6ecb1aa9088b43645111ae20 seems not have been saved properly, and it results the error:

Sign message error: signature error: Cannot find SessionKey for address: [0xac00d67a15ae97af88aff69cd80befdfe430330a08a6092163de84753cb220c4]

The new structure of the key store:

keys:
      0xac00d67a15ae97af88aff69cd80befdfe430330a08a6092163de84753cb220c4:
        RoochKeyPairType:
          hashed_password: $argon2id$v=19$m=19456,t=2,p=1$7vLoelDnzdybYdaHLYZXYQrqYSyOix7z5jC6Imf175A$jvcGd8dyjCrG4tAhYTqyq9J1aI54Ugvr0bUYdB9ygSo
          ...

I don't know the session key's design since the structure of the key store has been changed to eliminate the key pair, but is it required to save the session key as key pair in key store or in session_keys: {}?

jolestar · 2023-10-04T02:24:07Z

crates/testsuite/features/cmd.feature

-      Then cmd: "session-key create --sender-account {default} --scope 0x3::empty::empty"
-      Then cmd: "move run --function 0x3::empty::empty --sender-account {default} --session-key {{$.session-key[-1].authentication_key}}"
+      Then cmd: "session-key create --sender-account {default} --scope 0x3::empty::empty --password false"
+      Then cmd: "move run --function 0x3::empty::empty --sender-account {default} --session-key {{$.session-key[-1].authentication_key}} --password false"


It seems like the session-key command output is broken, so this command can not get the --session-key argument.

This issue has been fixed.

jolestar · 2023-10-04T03:22:52Z

crates/rooch-key/src/key_derive.rs

-    fn derive_key_pair_from_path(
+type EncryptionKeyResult = Result<(Vec<u8>, Vec<u8>, Vec<u8>), RoochError>;
+
+pub trait CoinOperations<Addr, KeyPair, PrivKey> {


The CoinOperations name is not suitable yet.

jolestar · 2023-10-04T03:25:16Z

crates/rooch-key/src/key_derive.rs

+        // 96-bits; unique per message
+        let nonce = ChaCha20Poly1305::generate_nonce(&mut OsRng);
+
+        // Derive the key material using nonce and password
+        let mut output_key_material = [0u8; 32]; // Can be any desired size
+        Argon2::default()
+            .hash_password_into(
+                password.unwrap_or("".to_owned()).as_bytes(),
+                &nonce,
+                &mut output_key_material,
+            )
+            .map_err(|e| RoochError::KeyConversionError(e.to_string()))?;
+
+        // Create a ChaCha20Poly1305 cipher with the key material from password
+        let cipher = ChaCha20Poly1305::new_from_slice(&output_key_material)
+            .map_err(|e| RoochError::KeyConversionError(e.to_string()))?;
+
+        // Encrypt the private key data to a ciphertext with a tag
+        let ciphertext_with_tag = match cipher.encrypt(&nonce, private_key.as_bytes()) {
+            Ok(ciphertext) => ciphertext,
+            Err(_) => {
+                return Err(RoochError::KeyConversionError(
+                    "Encryption failed".to_owned(),
+                ))
+            }
+        };
+
+        // Extract the ciphertext without the tag
+        let ciphertext = ciphertext_with_tag[..ciphertext_with_tag.len() - 16].to_vec();
+
+        // Extract the tag (last 16 bytes)
+        // The tag is useless here for deriving the key pair in function derive_key_pair_from_ciphertext
+        // Because from_bytes needs exactly 32 bytes input to convert to the private keys
+        // Poly1305 is a one-time authenticator designed by D. J. Bernstein.
+        // Poly1305 takes a 32-byte one-time key and a message and produces a
+        // 16-byte tag. This tag is used to authenticate the message.
+        // https://www.rfc-editor.org/rfc/rfc7539
+        let tag = {
+            let start = ciphertext_with_tag.len() - 16;
+            let end = ciphertext_with_tag.len();
+            let mut tag = Vec::with_capacity(16);
+            tag.extend_from_slice(&ciphertext_with_tag[start..end]);
+            tag
+        };
+
+        Ok((nonce.to_vec(), ciphertext, tag))


Most lines repeat with CoinOperations<RoochKeyPair>

Simplified.

jolestar · 2023-10-04T03:27:30Z

crates/rooch-key/src/keystore.rs

@@ -47,30 +59,39 @@ pub enum Keystore<K: Ord, V> {
 }

 #[enum_dispatch]
-pub trait AccountKeystore<Addr: Copy, PubKey, KeyPair, Sig, TransactionData>: Send + Sync {
+pub trait AccountKeystore<Addr: Copy, PubKey, KeyPair, Sig, TransactionData, PrivKey>:


This should be part of #788. As discussed in the PR, Maybe we do not need to distinguish the Keystore via the KeyPair type and TransactionData.

Will refactor it in #788 then.

jolestar · 2023-10-04T03:36:13Z

crates/rooch/src/commands/init.rs

-                    active_address: Some(new_address),
+                    password: Some(result.result.encryption.hashed_password),
+                    nonce: Some(result.result.encryption.nonce.encode_hex()),
+                    ciphertext: Some(result.result.encryption.ciphertext.encode_hex()),


keystore file should be an independent file, not part of client_config.

jolestar · 2023-10-05T02:09:56Z

crates/rooch/src/commands/session_key/commands/create.rs

+            // Prompt for a password if required
+            rpassword::prompt_password("Enter a password to encrypt the keys in the rooch keystore. Press return to have an empty value: ").unwrap()
+        };
+        println!("Your password is {}", password);


Do not print the password when decrypting.

I leaved it with an empty string for a new password.

jolestar · 2023-10-05T02:21:03Z

We can merge this PR first and discuss some refactors later:

Should every private key have a new password? Or the whole keystore uses one password.
Is it possible to eliminate the --password false options?

feliciss · 2023-10-05T03:12:18Z

We can merge this PR first and discuss some refactors later:

Should every private key have a new password? Or the whole keystore uses one password.

Is it possible to eliminate the --password false options?

Every key has a password, but the user can ignore entering it. There's an issue with account list when different keys have different passwords.
Yes. We can eliminate the option and make every key encrypted with "" password string.

jolestar · 2023-10-05T15:11:28Z

2. Yes. We can eliminate the option and make every key encrypted with "" password string.

I didn't mean to remove the password prompt. But it is OK. I will merge this PR first, and we discuss how to refactor it.

feliciss requested review from wow-sven, baichuan3, templexxx and pause125 as code owners September 23, 2023 02:43

feliciss mentioned this pull request Sep 23, 2023

[CLI] Make cli stable #755

Closed

7 tasks

feliciss force-pushed the #755 branch from 94fff7a to 2fc2ec5 Compare September 23, 2023 04:39

jolestar reviewed Sep 24, 2023

View reviewed changes

feliciss force-pushed the #755 branch from 2fc2ec5 to ee593f1 Compare September 25, 2023 04:04

feliciss changed the title ~~[gh-755] encrypt w/ XChaCha20Poly1305 and password~~ [gh-755] encrypt w/ ChaCha20Poly1305 and password Sep 25, 2023

feliciss force-pushed the #755 branch from ee593f1 to 02a1deb Compare September 25, 2023 23:25

feliciss force-pushed the #755 branch 3 times, most recently from 2ea6dfd to eb86ae9 Compare September 27, 2023 17:08

jolestar reviewed Sep 28, 2023

View reviewed changes

feliciss force-pushed the #755 branch from 5d9b0b7 to cffae4f Compare September 28, 2023 02:42

feliciss changed the title ~~[gh-755] encrypt w/ ChaCha20Poly1305 and password~~ [gh-755] encrypt w/ ChaCha20Poly1305 and password algorithm Argon2id Sep 28, 2023

feliciss force-pushed the #755 branch from cffae4f to ae08d33 Compare September 30, 2023 05:56

jolestar reviewed Sep 30, 2023

View reviewed changes

feliciss force-pushed the #755 branch from ae08d33 to 883cfb0 Compare October 3, 2023 05:33

jolestar reviewed Oct 3, 2023

View reviewed changes

feliciss force-pushed the #755 branch from f9ed104 to 49fb3ec Compare October 3, 2023 17:54

jolestar reviewed Oct 4, 2023

View reviewed changes

Feliciss added 17 commits October 4, 2023 15:05

[rooch-networkgh-755] encrypt w/ XChaCha20Poly1305 and password

41fcbba

[rooch-networkgh-755] fallback to algorithm ChaCha20Poly1305.

83826d7

[rooch-networkgh-755] add decrypt private key method.

1b31af5

[rooch-networkgh-755] format encrypt & decrypt functions.

c6d7d51

[rooch-networkgh-755] add password encryption and verification.

dd03e26

[rooch-networkgh-755] add key config to config store.

e594ea0

[rooch-networkgh-755] add key materials to the password.

1632388

[rooch-networkgh-755] refactor types.

5f8a902

[rooch-networkgh-755] add flag to get around of integration.

7db030c

[rooch-networkgh-755] make password required default to true.

0a7d104

[rooch-networkgh-755] use full flag.

1e31980

[rooch-networkgh-755] refactor keystore.

a8c3155

[rooch-networkgh-755] add comment and sign with password.

f446dcb

[rooch-networkgh-755] fix integration tests.

2ad74f7

[rooch-networkgh-755] delete unnecessary output for password.

34114bb

[rooch-networkgh-755] add --password false to cmd.feature.

d9b66e4

[rooch-networkgh-755] remove ambiguous -p flag.

f06acf5

feliciss force-pushed the #755 branch from 57ac8ed to f06acf5 Compare October 4, 2023 21:15

Feliciss added 2 commits October 4, 2023 17:14

[rooch-networkgh-755] refactor key store and simplify code.

0610eb3

[rooch-networkgh-755] handle errors.

6b65478

jolestar approved these changes Oct 5, 2023

View reviewed changes

[rooch-networkgh-755] modify session key struct to fix it.

feefbfd

[rooch-networkgh-755] remove --password-required flag.

68ee6bb

jolestar merged commit 36f5567 into rooch-network:main Oct 5, 2023
4 checks passed

feliciss deleted the #755 branch October 5, 2023 15:21

[gh-755] encrypt w/ ChaCha20Poly1305 and password algorithm Argon2id #856

[gh-755] encrypt w/ ChaCha20Poly1305 and password algorithm Argon2id #856

Conversation

feliciss commented Sep 23, 2023 • edited Loading

Summary

vercel bot commented Sep 23, 2023 • edited Loading

jolestar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feliciss Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

feliciss commented Sep 26, 2023

feliciss commented Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feliciss commented Sep 28, 2023 • edited Loading

feliciss commented Sep 28, 2023

Choose a reason for hiding this comment

feliciss Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolestar commented Oct 1, 2023

feliciss commented Oct 3, 2023 • 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

jolestar commented Oct 3, 2023

feliciss commented Oct 3, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feliciss Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

jolestar commented Oct 5, 2023

feliciss commented Oct 5, 2023 • edited Loading

jolestar commented Oct 5, 2023

feliciss commented Sep 23, 2023 •

edited

Loading

vercel bot commented Sep 23, 2023 •

edited

Loading

feliciss Sep 25, 2023 •

edited

Loading

feliciss commented Sep 26, 2023 •

edited

Loading

feliciss commented Sep 28, 2023 •

edited

Loading

feliciss Sep 30, 2023 •

edited

Loading

feliciss commented Oct 3, 2023 •

edited

Loading

feliciss commented Oct 3, 2023 •

edited

Loading

feliciss Oct 5, 2023 •

edited

Loading

feliciss commented Oct 5, 2023 •

edited

Loading