feat(wallet): wallet secret key file encryption

[mickvandijke]

Description

Adds an option to save the wallet private key as encrypted with a password to a file called main_secret_key.encrypted, inside the wallet directory.

The encrypted secret key file will look like this:

{
    "encrypted_secret_key": "bdbc29f1e0c3d2211ccfa78c262bd3f42d71235f13b31bab339c682fc9e56d039302870c1f69a004c80c6691b62d51e2",
    "salt": "ee69ccf0008631ac",
    "nonce": "d8a46c7ccef48113f10a6cc1"
}

Relevant encryption logic:

/// Encrypted secret key for storing on disk and decrypting with password
#[derive(Serialize, Deserialize)]
pub(crate) struct EncryptedSecretKey {
    encrypted_secret_key: String,
    pub salt: String,
    pub nonce: String,
}

...

/// Encrypts secret key using pbkdf2 with HMAC<Sha512>.
pub(crate) fn encrypt_secret_key(
    secret_key: &MainSecretKey,
    password: &str,
) -> Result<EncryptedSecretKey> {
    // Generate a random salt
    // Salt is used to ensure unique derived keys even for identical passwords
    let mut salt = [0u8; 8];
    rand::thread_rng().fill(&mut salt);

    // Generate a random nonce
    // Nonce is used to ensure unique encryption outputs even for identical inputs
    let mut nonce = [0u8; 12];
    rand::thread_rng().fill(&mut nonce);

    let mut key = [0; 32];

    // Derive a key from the password using PBKDF2 with HMAC<Sha512>
    // PBKDF2 is used for key derivation to mitigate brute-force attacks by making key derivation computationally expensive
    // HMAC<Sha512> is used as the pseudorandom function for its security properties
    ring::pbkdf2::derive(
        ring::pbkdf2::PBKDF2_HMAC_SHA512,
        NonZeroU32::new(ITERATIONS).unwrap(),
        &salt,
        password.as_bytes(),
        &mut key,
    );

    // Create an unbound key using CHACHA20_POLY1305 algorithm
    // CHACHA20_POLY1305 is a fast and secure AEAD (Authenticated Encryption with Associated Data) algorithm
    let unbound_key = ring::aead::UnboundKey::new(&ring::aead::CHACHA20_POLY1305, &key)
        .map_err(|_| Error::FailedToEncryptKey(String::from("Could not create unbound key.")))?;

    // Create a sealing key with the unbound key and nonce
    let mut sealing_key = ring::aead::SealingKey::new(unbound_key, NonceSeq(nonce));
    let aad = ring::aead::Aad::from(&[]);

    // Convert the secret key to bytes
    let secret_key_bytes = secret_key.to_bytes();
    let mut encrypted_secret_key = secret_key_bytes;

    // seal_in_place_append_tag encrypts the data and appends an authentication tag to ensure data integrity
    sealing_key
        .seal_in_place_append_tag(aad, &mut encrypted_secret_key)
        .map_err(|_| Error::FailedToEncryptKey(String::from("Could not seal sealing key.")))?;

    // Return the encrypted secret key along with salt and nonce encoded as hex strings
    Ok(EncryptedSecretKey {
        encrypted_secret_key: encode(encrypted_secret_key),
        salt: encode(salt),
        nonce: encode(nonce),
    })
}

CLI changes

wallet create command now prompts the user if they want to encrypt their new wallet with a password. Also, the following flags have been added:

Flag	Description
--no-password	Skip the set password prompt.
--password [PASSWORD]	Provide a password to encrypt the wallet with.

Added wallet encrypt command to encrypt an existing unencrypted wallet with a password.

Other changes

• CLI commands that use an encrypted wallet, now request a user to provide the password to unlock the wallet.

• The hotwallet stash function now uses a different folder name for saving old wallets. Instead of using the wallet public address: wallet_PUBLIC_ADDRESS, it now uses: wallet_CURRENT_DATETIME. This change is to support stashing encrypted wallets, without having to decrypt them to get the public address.

TODO List

• Update CLI flow to optionally encrypt a wallet private key file with a user supplied password
• CLI should prompt the user for a password if the wallet private key file is encrypted

Related Issue

Fixes #1943. Solution 1.

Type of Change

Please mark the types of changes made in this pull request.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update
• Other (please describe):

Checklist

Please ensure all of the following tasks have been completed:

• I have read the contributing guidelines.
• I have performed a self-review of my own code.
• I have commented my code, particularly in hard-to-understand areas.
• I have added tests that prove my fix is effective or that my feature works.
• New and existing unit tests pass locally with my changes.
• I have updated the documentation accordingly.
• I have followed the conventional commits guidelines for commit messages.
• I have verified my commit messages with commitlint.

[mickvandijke]

@grumbach @joshuef Should the CLI changes in the TODO list perhaps be added in a separate PR? Else this one might get big.

[grumbach]

Yeah include them in! It makes sense in your case!

[mickvandijke]

aa9b547 makes sure that the CLI prompts the user for a password when an encrypted wallet requires it for authentication.

[mickvandijke]

PR is ready for review.

[grumbach]

I would make sure the key is stored successfully before deleting the old one. Else what happens if the store_main_secret_key errors out?

[mickvandijke]

Ah yeah good call, that would be quite a catastrophic failure. I'll swap the order of actions around, so that store_main_secret_key is done first.

[mickvandijke]

Commit 71267b5 correctly handles this scenario now.

// Save the secret key as an encrypted file
store_main_secret_key(&wallet_dir, &wallet_key, Some(password.to_owned()))?;

// Delete the unencrypted secret key file
// Cleanup if it fails
if let Err(err) = delete_unencrypted_main_secret_key(&wallet_dir) {
    let _ = delete_encrypted_main_secret_key(&wallet_dir);
    return Err(err);
}

[grumbach]

Just to make sure, Cleanup if it fails with delete_encrypted_main_secret_key doesn't delete the encrypted one does it?

[grumbach]

If it does, are we 100% sure that an error above means the unencrypted_main_secret_key was NOT deleted?

[grumbach]

Just trying to make sure we don't lose keys

[mickvandijke]

It comes down to https://doc.rust-lang.org/std/fs/fn.remove_file.html. I think that we can reasonably expect that it only fails when it couldn't delete the file (unencrypted main_secret_key in this case).

But to be 100% on the safe side, we could not do a cleanup in case of failure of delete_unencrypted_main_secret_key. Worst case scenario then, we would have both a main_secret_key file and a main_secret_key.encoded file in the same wallet directory.

[grumbach]

Why switch to date/time?

[mickvandijke]

Makes it a lot easier to stash encrypted wallets without needing to decrypt them to get the public address.

[grumbach]

without needing to decrypt them to get the public address.

Not sure I understand, how can you get the public address if it's not in the name anymore?

[mickvandijke]

The public address can be derived from the secret key. The problem with encrypted wallets is that the secret key is encrypted and needs a password to reveal it.

If we look at the old code:

let wallet = HotWallet::load_from(root_dir)?; // Would fail if the wallet is encrypted
let addr_hex = &format!("{:?}", wallet.address()); // Needs wallet

To fix that, we would have to something like this:

let wallet = HotWallet::load_encrypted_from_path(root_dir, PASSWORD)?;
let addr_hex = &format!("{:?}", wallet.address());

But then the user needs to know and provide the correct password before an encrypted wallet can be stashed. Since this command is only used when creating a new wallet and replacing the old one, I think it makes more sense from a user perspective to not require the password of an encrypted wallet to stash it.

[grumbach]

Oooh I see! Although sorting by key is better, it's a hack so we don't need to unlock the wallet so do that?
But then on the other hand, filtering by date here is weird, and we will need to cleanup old ones every time...
We will also need to filter by public key when we want to support multi-key in the future so keeping pks is cleaner.
How about keeping the public key in a non-encrypted file along with the wallet?

[mickvandijke]

Oooh I see! Although sorting by key is better, it's a hack so we don't need to unlock the wallet so do that?

Correct!

How about keeping the public key in a non-encrypted file along with the wallet?

I thought about using the existing main_pubkey file. But the drawback for me was that we can not verify whether that pubkey is actually valid for a locked encrypted wallet/secret key. We'd still have to unlock the wallet to verify if the pubkey actually belongs to that wallet.

We will also need to filter by public key when we want to support multi-key in the future so keeping pks is cleaner.

When the project starts supporting multiple keys/wallets, I agree that wallets should be saved in folder names that represent their pubkey. I'm not sure what folder structure will be used then (perhaps #1944 ?). In that case, I see the current stash more as a function to backup an old wallet that you may or may not know the password of.

If we do decide on stashing old wallets under a folder name that contains the public address. I'm just a bit stuck on figuring out how we would best deal with stashing encrypted wallets where the user doesn't remember the password of?

[mickvandijke]

Hey @grumbach ,

I've made the changes we've discussed (using the main_pubkey file to get the wallet address): ce62fbf

[grumbach]

I think the wallet ends up in a file dumped on disk and this struct is a field of wallet.
So if the password is Some, it ends up dumped to the disk in clear too.
Even after expiry, an attacker could read the password by cat'ing the wallet file.
Or did I miss something?

[mickvandijke]

Isn't the wallet file only a watch-only-wallet and not a hotwallet? Also, the Secret<> wrapper from the secrecy crate, prevents just that scenario by not offering a serialize impl. And the debug impl will automatically redact the inner value.

[grumbach]

Isn't the wallet file only a watch-only-wallet and not a hotwallet?

That I'm not sure.

Also, the Secret<> wrapper from the secrecy crate, prevents just that scenario

Okay this is nice! We're safe then!

[grumbach]

Excellent work! Very clean code, love this! 🥇
Left some comments!

[grumbach]

LGTM! Excellent work!

[mickvandijke]

Hi @grumbach ,

The test for stashing/unstashing fails only on Windows because of a permission error. I do not have a Windows PC to test myself. Is this a known problem specific to a Windows Docker image?

EDIT: Managed to fix it.