add no_password option for the seed command

[nicbus]

This PR adds the -N option to the seed command, as is already available for the derive and sign commands.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes missing coverage. Please review.

Project coverage is 0.0%. Comparing base (1604ca1) to head (e57b67c).
Report is 35 commits behind head on master.

Files	Patch %	Lines
src/hot/command.rs	0.0%	29 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##           master    #57    +/-   ##
======================================
  Coverage     0.0%   0.0%            
======================================
  Files          16     23     +7     
  Lines        1437   2140   +703     
======================================
- Misses       1437   2140   +703     

Flag	Coverage Δ
rust	0.0% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dr-orlovsky]

Thank you for your effort, I actually had that originally, but had removed lately. The seeds are shared between testnet and mainnet keys. They are generated only once and can be re-used, thus allowing to store them with no encryption is extremely dangerous. I do not want to repeat the story of Eric Voskuil (his libbitcoinconsensus wallet was hacked which led to the loss of funds and his reputation), thus I can't accept this change to the library.

For tests, you can manually generate the seed, then derive a (set) of private keys for testnet with no password (that option is already present). This can be done once and manually, since you can make them part of the git history. With that approach, they can be used from test scripts with no passwords.

[nicbus]

For tests, you can manually generate the seed, then derive a (set) of private keys for testnet with no password (that option is already present). This can be done once and manually, since you can make them part of the git history. With that approach, they can be used from test scripts with no passwords.

For testing purposes, I can manually generate a seed (with a password) but then I cannot derive it without a password, as with -N I don't get prompted for the account password but I still do for the seed one.

My goal is to make bp-wallet's CLI interface usable from a script, not to lower security, so I propose an alternative solution: drop -N from the seed command and add a --seed-password option to both seed and derive commands, so it can be provided as an option rather than being entered interactively.

[dr-orlovsky]

From my understanding, password is required for working with seed, including account derivation - but the account has its own password, which for testnet accounts can be empty. And when you sign PSBT with such testnet account having empty password, you already have an -N option allowing to do the signature - meaning it should work for scripts.

If it doesn't work that way, this is a bug which has to be fixed.

[nicbus]

When I wrote

For testing purposes, I can manually generate a seed (with a password) but then I cannot derive it without a password, as with -N I don't get prompted for the account password but I still do for the seed one

I meant that the current code cannot support the usage you suggest, as it's impossible to call derive without providing the seed password interactively.

Signing is not an issue and already works for this use case, in fact my PR doesn't change it.

What this PR currently does is:

• add a -p / --seed-password to the seed command, so it can be provided via cmdline option, please note that the entropy check cannot be skipped, so I see no security issues here as every seed will always have a strong password set
• add a --seed-password to the derive command, so it can be specified via cmdline, which is necessary to be able to use the derive command in a non-interactive way

Other workflows have not changed, specifically:

• it's still necessary to provide a strong password for the seed
• it's still necessary to provide a strong password for the derivation, when on mainnet
• it's still possible to use no password or a weak one for derivation, except on mainnet

This PR can be seen as a fix for the bug you mention.

[dr-orlovsky]

I meant that the current code cannot support the usage you suggest, as it's impossible to call derive without providing the seed password interactively.

No, that is not what I said :)

I said that you can derive manually providing seed password - and no password for the derived private key, which is stored in file and can be then used in the script (with no password). You do not need to access seed file from the script, ever - that's the idea

[nicbus]

In master, derive begins with:

fn derive(
    seed_file: &Path,
    scheme: Bip43,
    account: HardenedIndex,
    mainnet: bool,
    output_file: &Path,
    no_password: bool,
) -> Result<(), DataError> {
    let seed_password = rpassword::prompt_password("Seed password: ")?;

This means that when calling the derive command the user is immediately prompted for the seed password and there's no way to avoid this, which in turn means the derive command cannot be used non-interactively (scripted).

If by "you can derive manually providing seed password" you mean entering it interactively, then that's not what I'm looking for. I'm looking to non-interactively provide it, e.g. via the --seed-password option added by this PR.

[dr-orlovsky]

My problem with providing password as a cli argument is the fact that this is very bad practice, which will make passwords becoming parts of the command history in explicit form...

BP wallet cli is intended for the real-world use by users for keeping their bitcoins and thus should do whatever is possible not to leak any information like seed passwords even if used by fools.

I am trying to figure out how to combine that requirement with testing needs.

[nicbus]

My problem with providing password as a cli argument is the fact that this is very bad practice, which will make passwords becoming parts of the command history in explicit form...

BP wallet cli is intended for the real-world use by users for keeping their bitcoins and thus should do whatever is possible not to leak any information like seed passwords even if used by fools.

Command history can be disabled (e.g. HISTCONTROL=ignorespace in bash) but I agree that suggesting to place passwords in the cmdline is a bad idea.

I am trying to figure out how to combine that requirement with testing needs.

I tried to use echo "password" | bp-wallet derive ... as you suggested but it doesn't work (and it wouldn't solve the issue with bp-hot seed as that requires password confirmation as well). Plus, it would still leave the password in the shell's history.

I checked ssh and gpg, which you were wondering about, but they use agents to provide credentials in a non-interactive way and providing a bp-wallet agent would be overkill.

I think using an environment variable would be a reasonable compromise:

• a user could set it via a .env file or set it via a password prompt (e.g. via the shell's read builtin) and export it
• a script can set the var in code or source it from a .env file and export it

With the var set in the environment, calling seed or derive should then work without any additional cmdline options.

A user could still set the variable on the cmdline (e.g. SEED_PASSWORD="password" bp-hot derive ...), so this solution would not completely prevent the password from landing on the cmdline, but I guess documentation covering this could be enough. After all, the user can already leak their seed password in many other (possibly more dangerous) ways.

If you agree, I'll update the PR with this solution.

[dr-orlovsky]

Yes, it seems environment variable is the exact solution we need. Thank you!

[nicbus]

Updated to use the SEED_PASSWORD env var.

I briefly documented the possible security issue in both seed and derive commands.
I didn't include .env handling as that requires to depend on another crate and it can be added in the future if needed.

IMO this is good enough, do you agree?

[dr-orlovsky]

Hey @nicbus, while reviewing your PR, I'd suggest the following code changes:

👉 Code Suggestion for #57

#57

You can also review and apply these suggestions locally on your machine.

Learn more about GitKraken Code Suggest

Code Suggest liberates your code reviews from GitHub's restrictive, comment-only feedback style. As simple as suggesting changes in a Google-doc, provide real code suggestions from where you code, e.g. your IDE, and on anything in your project — not just on the lines of code changed in the PR.

Join your team on GitKraken to speed up PR review.

[dr-orlovsky]

Just small nit from my side in d2d49ce, otherwise looks great