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

kallewoof commented at 2:52 PM on July 19, 2018: member

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.

fanquake added the label Utils/log/libs on Jul 19, 2018

kallewoof force-pushed on Jul 19, 2018

practicalswift commented at 8:29 PM on July 19, 2018: contributor

Concept ACK

DrahtBot commented at 1:22 PM on July 22, 2018: member

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

Conflicts

No conflicts as of last run.

kallewoof force-pushed on Jul 23, 2018

kallewoof closed this on Jul 24, 2018

kallewoof reopened this on Jul 24, 2018

DrahtBot added the label Needs rebase on Jul 24, 2018

kallewoof force-pushed on Jul 25, 2018

DrahtBot removed the label Needs rebase on Jul 25, 2018

hebasto commented at 8:47 AM on August 5, 2018: member

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.

DrahtBot added the label Needs rebase on Aug 13, 2018

kallewoof force-pushed on Aug 14, 2018

DrahtBot removed the label Needs rebase on Aug 14, 2018

in src/util.cpp:81 in fb1f6c9c29 outdated

  77 | @@ -78,6 +78,14 @@
  78 |  #include <openssl/conf.h>
  79 |  #include <thread>
  80 |  
  81 | +#ifdef WIN32

laanwj commented at 4:37 PM on September 10, 2018:

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.

kallewoof commented at 6:12 AM on September 11, 2018:

Good point. Fixing.

in src/util.cpp:84 in fb1f6c9c29 outdated

  77 | @@ -78,6 +78,14 @@
  78 |  #include <openssl/conf.h>
  79 |  #include <thread>
  80 |  
  81 | +#ifdef WIN32
  82 | +#include <windows.h>    // for SetStdinEcho()
  83 | +#else
  84 | +#include <termios.h>    // for SetStdinEcho()

laanwj commented at 4:38 PM on September 10, 2018:

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

kallewoof commented at 6:15 AM on September 11, 2018:

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

laanwj commented at 9:22 AM on September 11, 2018:

going to test on FreeBSD and OpenBSD

laanwj commented at 4:40 PM on September 10, 2018: member

Concept ACK

kallewoof force-pushed on Sep 11, 2018

in src/bitcoin-cli.cpp:419 in abd986f98f outdated

 415 | @@ -414,14 +416,33 @@ static int CommandLineRPC(int argc, char *argv[])
 416 |              argc--;
 417 |              argv++;
 418 |          }
 419 | +        SetStdinEcho(false);

laanwj commented at 9:49 AM on September 11, 2018:

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

kallewoof commented at 11:49 AM on September 11, 2018:

Good points. Addressed both.

laanwj commented at 9:56 AM on September 11, 2018: member

Checked that this compiles and works as-is on OpenBSD 6.3 and FreeBSD 11.2-RELEASE-p2.

Would be good to have functional tests for this, in as far as that is possible—seems tricky to simulate a terminal

kallewoof force-pushed on Sep 11, 2018

kallewoof commented at 11:50 AM on September 11, 2018: member

@laanwj Thanks for review and especially for testing on other distros! I'm a bit stumped on how to make tests for this. Is it even possible to test stdin echo?

laanwj commented at 12:15 PM on September 11, 2018: member

Is it even possible to test stdin echo?

Yes, it's certainly possible. However I'm afraid that it'd involve allocating a pty and emulating an actual terminal, all from python. There is code for that, but it's probably a bit too much to pull in a dependency for. It's a good indication that we should stop here at accommodating terminals, though, at the minimum possible code dedicated to this (which is easy to manually test.)

in src/bitcoin-cli.cpp:434 in 1d606e9401 outdated

 429 |              gArgs.ForceSetArg("-rpcpassword", rpcPass);
 430 | +            SetStdinEcho(true);
 431 |          }
 432 |          std::vector<std::string> args = std::vector<std::string>(&argv[1], &argv[argc]);
 433 | +        if (gArgs.GetBoolArg("-stdinwalletpassphrase", false)) {
 434 | +            SetStdinEcho(false);

laanwj commented at 12:24 PM on September 11, 2018:

will this leave the user's terminal in a bad state in case of an exception?

if so, might want to use a RAII class

kallewoof commented at 12:38 PM on September 11, 2018:

It didn't for Macs, but you're right, there's really no guarantees so a RAII makes sense. Will fix.

kallewoof commented at 12:58 PM on September 11, 2018:

Added RAII wrapper.

kallewoof force-pushed on Sep 11, 2018

laanwj commented at 4:21 PM on September 11, 2018: member

lightly-tested ACK 2836f6fc1ef98cef41993970f8b3f7c0758484f5

jonasschnelli commented at 7:25 PM on September 11, 2018: contributor

From the control flow, it seems a bit odd that one gets asked for the password first via std-in, but then get an error like RPC password> error: too few parameters (need at least command).

IMO a) we should newline after a password read and b) eventually check for missing command (and other static checks) before asking for the password.

kallewoof commented at 3:34 AM on September 12, 2018: member

@jonasschnelli That particular case will never happen, as the code already checks that the command starts with "walletpassphrase". Are there other cases or potential cases?

in src/compat/stdin.cpp:26 in 2836f6fc1e outdated

  21 | +{
  22 | +#ifdef WIN32
  23 | +    HANDLE hStdin = GetStdHandle(STD_INPUT_HANDLE);
  24 | +    DWORD mode;
  25 | +    GetConsoleMode(hStdin, &mode);
  26 | +    if (!enable) mode &= ~ENABLE_ECHO_INPUT; else mode |= ENABLE_ECHO_INPUT;

practicalswift commented at 8:14 AM on September 23, 2018:

2018-09-22 21:56:54 cpplint(pr=13716): src/compat/stdin.cpp:26:  Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]
2018-09-22 21:56:54 cpplint(pr=13716): src/compat/stdin.cpp:26:  If/else bodies with multiple statements require braces  [readability/braces] [4]

in src/compat/stdin.cpp:31 in 2836f6fc1e outdated

  26 | +    if (!enable) mode &= ~ENABLE_ECHO_INPUT; else mode |= ENABLE_ECHO_INPUT;
  27 | +    SetConsoleMode(hStdin, mode);
  28 | +#else
  29 | +    struct termios tty;
  30 | +    tcgetattr(STDIN_FILENO, &tty);
  31 | +    if (!enable) tty.c_lflag &= ~ECHO; else tty.c_lflag |= ECHO;

practicalswift commented at 8:14 AM on September 23, 2018:

2018-09-22 21:56:54 cpplint(pr=13716): src/compat/stdin.cpp:31:  Else clause should never be on same line as else (use 2 lines)  [whitespace/newline] [4]
2018-09-22 21:56:54 cpplint(pr=13716): src/compat/stdin.cpp:31:  If/else bodies with multiple statements require braces  [readability/braces] [4]

practicalswift commented at 8:33 AM on October 3, 2018:

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

kallewoof commented at 12:35 AM on October 6, 2018:

Is there a case when this would be a problem?

kallewoof commented at 2:31 AM on November 30, 2018:

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

in src/bitcoin-cli.cpp:435 in 2836f6fc1e outdated

 430 |          }
 431 |          std::vector<std::string> args = std::vector<std::string>(&argv[1], &argv[argc]);
 432 | +        if (gArgs.GetBoolArg("-stdinwalletpassphrase", false)) {
 433 | +            NO_STDIN_ECHO();
 434 | +            std::string walletPass;
 435 | +            if (args.size() < 1 || args[0].substr(0, strlen("walletpassphrase")) != "walletpassphrase") {

practicalswift commented at 11:27 AM on September 23, 2018:

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

kallewoof commented at 12:36 AM on October 6, 2018:

Fixed. :P

kallewoof force-pushed on Oct 6, 2018

kallewoof commented at 12:38 AM on October 6, 2018: member

Addressed @practicalswift nits.

kallewoof force-pushed on Oct 29, 2018

Sjors commented at 6:30 PM on January 31, 2019: member

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).

DrahtBot added the label Needs rebase on Feb 12, 2019

kallewoof commented at 6:01 AM on February 13, 2019: member

@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 force-pushed on Feb 13, 2019

DrahtBot removed the label Needs rebase on Feb 13, 2019

Sjors commented at 9:17 AM on February 13, 2019: member

@kallewoof it didn't return a new line for me (macOS 10.14.3): <img width="665" alt="schermafbeelding 2019-02-13 om 10 11 12" src="https://user-images.githubusercontent.com/10217/52700180-c1c46c80-2f77-11e9-8975-da11c9d0cc18.png">

re-tACK 110e821

kallewoof commented at 5:51 AM on February 14, 2019: member

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

Sjors commented at 3:59 PM on February 14, 2019: member

re-tACK d1688c9

DrahtBot added the label Needs rebase on Aug 2, 2019

kallewoof force-pushed on Aug 3, 2019

DrahtBot removed the label Needs rebase on Aug 3, 2019

laanwj added the label Feature on Sep 30, 2019

in src/bitcoin-cli.cpp:418 in f42176b688 outdated

 412 | @@ -411,18 +413,41 @@ static int CommandLineRPC(int argc, char *argv[])
 413 |          }
 414 |          std::string rpcPass;
 415 |          if (gArgs.GetBoolArg("-stdinrpcpass", false)) {
 416 | +            NO_STDIN_ECHO();
 417 | +            if (!StdinReady()) {
 418 | +                fprintf(stderr, "RPC password> ");

laanwj commented at 12:05 PM on September 30, 2019:

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

MarcoFalke commented at 12:28 PM on September 30, 2019:

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

kallewoof commented at 3:05 AM on October 1, 2019:

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

add stdin helpers for password input support 0da503e947

cli: add -stdinwalletpassphrase for (slightly more) secure CLI 7f11fba2e3

add newline after -stdin* 50c4afa3c4

in src/bitcoin-cli.cpp:424 in f42176b688 outdated

 419 | +                fflush(stderr);
 420 | +            }
 421 |              if (!std::getline(std::cin, rpcPass)) {
 422 |                  throw std::runtime_error("-stdinrpcpass specified but failed to read from standard input");
 423 |              }
 424 | +            fputc('\n', stdout);

laanwj commented at 12:06 PM on September 30, 2019:

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

kallewoof commented at 3:33 AM on October 1, 2019:

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 force-pushed on Oct 1, 2019

laanwj commented at 11:15 AM on October 1, 2019: member

Thanks, LGTM now code review ACK 50c4afa3c420f11329cffb091b62beeb96b39183

laanwj referenced this in commit f4a0d27e85 on Oct 2, 2019

laanwj merged this on Oct 2, 2019

laanwj closed this on Oct 2, 2019

kallewoof deleted the branch on Oct 8, 2019

MarkLTZ referenced this in commit 65a4450f40 on Nov 17, 2019

deadalnix referenced this in commit 4289dc7952 on Oct 25, 2020

jasonbcox referenced this in commit 7f275f57df on Oct 25, 2020

jasonbcox referenced this in commit ffbceb44e4 on Oct 26, 2020

DrahtBot locked this on Dec 16, 2021