Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bitcoin-cli: -stdinwalletpassphrase and non-echo stdin passwords #13716

Merged
merged 3 commits into from Oct 2, 2019

Conversation

@kallewoof
Copy link
Member

commented Jul 19, 2018

This PR

  • adds -stdinwalletpassphrase for use with walletpasshprase(change)
  • adds no-echo for passwords (-stdinrpcpass and above)

It may not be ideal, but it's better than having to clear the screen whenever you unlock the wallet.

@kallewoof kallewoof force-pushed the kallewoof:stdinwalletpassphrase branch 2 times, most recently Jul 19, 2018
@practicalswift

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@hebasto

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

There are conflicting pull requests:

  • this one adds new features and, as a side effect, fixes one of two bugs in bitcoin-cli --help output;
  • #13879 fixes all found bugs in bitcoin-cli --help output and is safe for the whole code.

So, #13879 can be merged regardless of this pull request.

src/util.cpp Outdated
@@ -78,6 +78,14 @@
#include <openssl/conf.h>
#include <thread>

#ifdef WIN32

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 10, 2018

Member

As this is only used in bitcoin-cli, it doesn't need to be in util.cpp (which is shared between the server and client), would be better in a client-specific utility, e.g. compat/noecho.{cpp,h} or something like that, that is only linked into -cli.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Sep 11, 2018

Author Member

Good point. Fixing.

src/util.cpp Outdated
#ifdef WIN32
#include <windows.h> // for SetStdinEcho()
#else
#include <termios.h> // for SetStdinEcho()

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 10, 2018

Member

is this generally supported on all UNIX, or does it need checks in the build system?

This comment has been minimized.

Copy link
@kallewoof

kallewoof Sep 11, 2018

Author Member

termios.h? It seems to be pretty standard FWICT.

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 11, 2018

Member

going to test on FreeBSD and OpenBSD

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Concept ACK

@kallewoof kallewoof force-pushed the kallewoof:stdinwalletpassphrase branch Sep 11, 2018
src/bitcoin-cli.cpp Outdated
@@ -414,14 +416,33 @@ static int CommandLineRPC(int argc, char *argv[])
argc--;
argv++;
}
SetStdinEcho(false);

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 11, 2018

Member
  • need to re-enable echoing before the normal -stdin processing
  • make sure that this is not called unless either -stdinrpcpass or -stdinwalletpassphrase is called to avoid interfering with scripts that pipe in data, not attached to a TTY

This comment has been minimized.

Copy link
@kallewoof

kallewoof Sep 11, 2018

Author Member

Good points. Addressed both.

src/bitcoin-cli.cpp Outdated
if (gArgs.GetBoolArg("-stdinwalletpassphrase", false)) {
NO_STDIN_ECHO();
std::string walletPass;
if (args.size() < 1 || args[0].substr(0, strlen("walletpassphrase")) != "walletpassphrase") {

This comment has been minimized.

Copy link
@practicalswift

practicalswift Sep 23, 2018

Member

Nit: Could be written with something more idiomatic than strlen? :-)

This comment has been minimized.

Copy link
@kallewoof

kallewoof Oct 6, 2018

Author Member

Fixed. :P

src/compat/stdin.cpp Outdated
#else
struct termios tty;
tcgetattr(STDIN_FILENO, &tty);
if (!enable) tty.c_lflag &= ~ECHO; else tty.c_lflag |= ECHO;

This comment has been minimized.

Copy link
@practicalswift

practicalswift Oct 3, 2018

Member

A negative integer is implicitly converted to unsigned type here. Please make the conversion explicit.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Oct 6, 2018

Author Member

Is there a case when this would be a problem?

This comment has been minimized.

Copy link
@kallewoof

kallewoof Nov 30, 2018

Author Member

I may have misunderstood what you were saying here; what are you suggesting I do, exactly?

@kallewoof kallewoof force-pushed the kallewoof:stdinwalletpassphrase branch Oct 6, 2018
@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2018

Addressed @practicalswift nits.

@kallewoof kallewoof force-pushed the kallewoof:stdinwalletpassphrase branch to b197e57 Oct 29, 2018
@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

tACK b197e57 for macOS 10.14.3

Nit: return \n after password entry.

Needs Windows testing too, @ken2812221?

For a followup it would be great if all wallet RPC's that need a private key worked directly with -stdinwalletpassphrase and just lock when they're done (though then the question is what default timeout you need).

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Feb 13, 2019

@Sjors

Nit: return \n after password entry.

Hitting enter adds a newline on my mac. Where do you want one to be added? After the std::getline call?

@kallewoof kallewoof force-pushed the kallewoof:stdinwalletpassphrase branch from b197e57 to 110e821 Feb 13, 2019
@DrahtBot DrahtBot removed the Needs rebase label Feb 13, 2019
@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 13, 2019

@kallewoof it didn't return a new line for me (macOS 10.14.3):
schermafbeelding 2019-02-13 om 10 11 12

schermafbeelding 2019-02-13 om 10 17 12

re-tACK 110e821

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Feb 14, 2019

Oh, okay! So this is the case for pre-existing -stdinrpcpass as well. Fixed in d1688c9.

@Sjors

This comment has been minimized.

Copy link
Member

commented Feb 14, 2019

re-tACK d1688c9

@kallewoof kallewoof force-pushed the kallewoof:stdinwalletpassphrase branch 2 times, most recently from e80b84a to 404beda Aug 3, 2019
@DrahtBot DrahtBot removed the Needs rebase label Aug 3, 2019
@laanwj laanwj added the Feature label Sep 30, 2019
@@ -411,18 +413,41 @@ static int CommandLineRPC(int argc, char *argv[])
}
std::string rpcPass;
if (gArgs.GetBoolArg("-stdinrpcpass", false)) {
NO_STDIN_ECHO();
if (!StdinReady()) {
fprintf(stderr, "RPC password> ");

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 30, 2019

Member

you could use fputs here (same below) and avoid introducing the locale dependency exception

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Sep 30, 2019

Member

tfm::format(std::cerr, ... should work as well

This comment has been minimized.

Copy link
@kallewoof

kallewoof Oct 1, 2019

Author Member

D'oh, yeah. Removing linter changes and using fputs.

if (!std::getline(std::cin, rpcPass)) {
throw std::runtime_error("-stdinrpcpass specified but failed to read from standard input");
}
fputc('\n', stdout);

This comment has been minimized.

Copy link
@laanwj

laanwj Sep 30, 2019

Member

please only print this extra newline if stdin terminal is detected, and not when, say, piping in from a script (same below 2x)

This comment has been minimized.

Copy link
@kallewoof

kallewoof Oct 1, 2019

Author Member

Understood. I think the latest version achieves this. (See StdinTerminal().)

$ ./bitcoin-cli -datadir=d -regtest encryptwallet foobar38
wallet encrypted; The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup.
$ ./bitcoin-cli -datadir=d -regtest -stdinwalletpassphrase walletpassphrase 10
Wallet passphrase>
$ echo foobar38 | ./bitcoin-cli -datadir=d -regtest -stdinwalletpassphrase walletpassphrase 10
$ echo xfoobar38 | ./bitcoin-cli -datadir=d -regtest -stdinwalletpassphrase walletpassphrase 10
error code: -14
error message:
Error: The wallet passphrase entered was incorrect.
kallewoof added 3 commits Jul 19, 2018
@kallewoof kallewoof force-pushed the kallewoof:stdinwalletpassphrase branch from f42176b to 50c4afa Oct 1, 2019
@laanwj

This comment has been minimized.

Copy link
Member

commented Oct 1, 2019

Thanks, LGTM now
code review ACK 50c4afa

laanwj added a commit that referenced this pull request Oct 2, 2019
…passwords

50c4afa add newline after -stdin* (Karl-Johan Alm)
7f11fba cli: add -stdinwalletpassphrase for (slightly more) secure CLI (Karl-Johan Alm)
0da503e add stdin helpers for password input support (Karl-Johan Alm)

Pull request description:

  This PR
  * adds `-stdinwalletpassphrase` for use with `walletpasshprase(change)`
  * adds no-echo for passwords (`-stdinrpcpass` and above)

  It may not be ideal, but it's better than having to clear the screen whenever you unlock the wallet.

ACKs for top commit:
  laanwj:
    code review ACK 50c4afa

Tree-SHA512: 473db8a303ff360ffaa36ac81a2f82be2136fa82696df0bc4f33cb44033a3ae258b5aa5bbcc1f101f88ae9abe9598ed564ce52877ab139bd5d709833f5275ec6
@laanwj laanwj merged commit 50c4afa into bitcoin:master Oct 2, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kallewoof kallewoof deleted the kallewoof:stdinwalletpassphrase branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.