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

pull kallewoof wants to merge 3 commits into bitcoin:master from kallewoof:stdinwalletpassphrase changing 4 files +124 −1
  1. 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.

  2. fanquake added the label Utils/log/libs on Jul 19, 2018
  3. kallewoof force-pushed on Jul 19, 2018
  4. kallewoof force-pushed on Jul 19, 2018
  5. practicalswift commented at 8:29 PM on July 19, 2018: contributor

    Concept ACK

  6. DrahtBot commented at 1:22 PM on July 22, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  7. DrahtBot cross-referenced this on Jul 22, 2018 from issue [refactor] Fix the chainparamsbase -> util circular dependency by Empact
  8. DrahtBot cross-referenced this on Jul 22, 2018 from issue [bugfix] Fix encoding issue for Windows by ken2812221
  9. kallewoof force-pushed on Jul 23, 2018
  10. kallewoof force-pushed on Jul 23, 2018
  11. kallewoof closed this on Jul 24, 2018
  12. kallewoof reopened this on Jul 24, 2018
  13. DrahtBot added the label Needs rebase on Jul 24, 2018
  14. kallewoof force-pushed on Jul 25, 2018
  15. DrahtBot removed the label Needs rebase on Jul 25, 2018
  16. DrahtBot cross-referenced this on Jul 25, 2018 from issue [WIP] Full unicode support on Windows by ken2812221
  17. DrahtBot cross-referenced this on Jul 25, 2018 from issue -masterdatadir for datadir bootstrapping by kallewoof
  18. DrahtBot cross-referenced this on Jul 28, 2018 from issue Test for Windows encoding issue by ken2812221
  19. DrahtBot cross-referenced this on Aug 5, 2018 from issue doc: fixed bitcoin-cli --help output to compatibility with help2man by hebasto
  20. 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.

  21. DrahtBot cross-referenced this on Aug 5, 2018 from issue utils: Convert Windows args to utf-8 string by ken2812221
  22. DrahtBot cross-referenced this on Aug 7, 2018 from issue docs: fixed bitcoin-cli -help output for help2man by hebasto
  23. DrahtBot added the label Needs rebase on Aug 13, 2018
  24. kallewoof force-pushed on Aug 14, 2018
  25. DrahtBot removed the label Needs rebase on Aug 14, 2018
  26. DrahtBot cross-referenced this on Aug 26, 2018 from issue Use std::unordered_set instead of set in blockfilter interface by jimpo
  27. 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.

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

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

    Concept ACK

  30. kallewoof force-pushed on Sep 11, 2018
  31. 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.

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

  33. kallewoof force-pushed on Sep 11, 2018
  34. 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?

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

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

  37. kallewoof force-pushed on Sep 11, 2018
  38. laanwj commented at 4:21 PM on September 11, 2018: member

    lightly-tested ACK 2836f6fc1ef98cef41993970f8b3f7c0758484f5

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

  40. 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?

  41. 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]
  42. 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?

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

  44. kallewoof force-pushed on Oct 6, 2018
  45. kallewoof commented at 12:38 AM on October 6, 2018: member

    Addressed @practicalswift nits.

  46. kallewoof force-pushed on Oct 29, 2018
  47. 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).

  48. promag cross-referenced this on Feb 6, 2019 from issue cli: encryptwallet password entered from stdin. fixes #15318 by darosior
  49. DrahtBot added the label Needs rebase on Feb 12, 2019
  50. 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?

  51. kallewoof force-pushed on Feb 13, 2019
  52. DrahtBot removed the label Needs rebase on Feb 13, 2019
  53. 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">

    <img width="560" alt="schermafbeelding 2019-02-13 om 10 17 12" src="https://user-images.githubusercontent.com/10217/52700569-8bd3b800-2f78-11e9-8449-cc1c4b44e1ed.png">

    re-tACK 110e821

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

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

    re-tACK d1688c9

  56. DrahtBot added the label Needs rebase on Aug 2, 2019
  57. kallewoof force-pushed on Aug 3, 2019
  58. kallewoof force-pushed on Aug 3, 2019
  59. DrahtBot removed the label Needs rebase on Aug 3, 2019
  60. laanwj added the label Feature on Sep 30, 2019
  61. 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.

  62. add stdin helpers for password input support 0da503e947
  63. cli: add -stdinwalletpassphrase for (slightly more) secure CLI 7f11fba2e3
  64. add newline after -stdin* 50c4afa3c4
  65. 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.
  66. kallewoof force-pushed on Oct 1, 2019
  67. laanwj commented at 11:15 AM on October 1, 2019: member

    Thanks, LGTM now code review ACK 50c4afa3c420f11329cffb091b62beeb96b39183

  68. laanwj referenced this in commit f4a0d27e85 on Oct 2, 2019
  69. laanwj merged this on Oct 2, 2019
  70. laanwj closed this on Oct 2, 2019
  71. kallewoof deleted the branch on Oct 8, 2019
  72. laanwj cross-referenced this on Oct 9, 2019 from issue build: Fix #include sys/poll.h to just poll.h (without sys/) by carnhofdaki
  73. MarkLTZ referenced this in commit 65a4450f40 on Nov 17, 2019
  74. MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
  75. adamjonas cross-referenced this on Apr 24, 2020 from issue `encryptwallet` password entered in clear by darosior
  76. deadalnix referenced this in commit 4289dc7952 on Oct 25, 2020
  77. jasonbcox referenced this in commit 7f275f57df on Oct 25, 2020
  78. jasonbcox referenced this in commit ffbceb44e4 on Oct 26, 2020
  79. bitcoin locked this on Dec 16, 2021
Contributors
kallewoofpracticalswiftDrahtBothebastolaanwjjonasschnelliSjorsMarcoFalke
Labels
FeatureUtils/log/libs
Linked (view graph)
#15318 `encryptwallet` password entered in clear#15346 cli: encryptwallet password entered from stdin. fixes #15318#17084 build: Fix #include sys/poll.h to just poll.h (without sys/)
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:54 UTC