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

Improvement: Password input #770

Merged
merged 3 commits into from
Jul 10, 2020
Merged

Improvement: Password input #770

merged 3 commits into from
Jul 10, 2020

Conversation

philmb3487
Copy link
Contributor

@philmb3487 philmb3487 commented Apr 8, 2020

Problem

This PR aims to improve the cli program by bringing in hidden password input.

Root Cause

We don’t want to show passwords in clear text.

Solution

Bring in two commits from Bitcoin upstream.
One introduces the stdin interfaces, the other to patch in -stdin* interfaces.

Unit Testing Results

  1. The build is successful.
  2. Test veil-cli with the following flags, in random combinations thereof:

→ -stdin
→ -stdinrpcpass
→ -stdinwalletpassphrase

  1. Is the behaviour acceptable?

@mimirmim
Copy link
Contributor

mimirmim commented Apr 8, 2020

Sweet this addresses my issues with #768. I'll give this a test when I wake up.

@CaveSpectre11 CaveSpectre11 added the Tag: Waiting For Code Review Waiting for code review from a core developer label May 7, 2020
Copy link
Collaborator

@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 968977f

@CaveSpectre11 CaveSpectre11 added the Component: RPC Related to the console commands themselves. label Jun 27, 2020
Copy link
Collaborator

@CaveSpectre11 CaveSpectre11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only problem was the lack of carriage return when using multiple inputs; or seeing the response from an rcp command:

$ ./veil-cli -stdinrpcpass -stdinwalletpassphrase walletpassphrase
RPC password> Wallet passphrase> error code: -1

These have been added:

$ ./veil-cli -stdinrpcpass -stdinwalletpassphrase walletpassphrase
RPC password>
Wallet passphrase>
error: Could not connect to the server

ACK 27e0d2b

@codeofalltrades give it another look please

@CaveSpectre11 CaveSpectre11 added Code Review: Passed Tag: Waiting For QA A pull review is waiting for QA to test the pull request and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Jul 5, 2020
@CaveSpectre11 CaveSpectre11 added QA: In Progress This is being tested by the QA team. KEEP CALM and RELAX. and removed Tag: Waiting For QA A pull review is waiting for QA to test the pull request labels Jul 7, 2020
@seanPhill
Copy link
Collaborator

seanPhill commented Jul 10, 2020

ACK Tested on Win64 build.

veil-cli -testnet -stdinwalletpassphrase walletpassphrase false 10000

This gave a prompt as below, and correctly unlocked the wallet.
Wallet passphrase>

@seanPhill
Copy link
Collaborator

seanPhill commented Jul 10, 2020

C:\Users\me\Veil-Project\veil-pr770> veil-cli -testnet walletlock

C:\Users\me\Veil-Project\veil-pr770> veil-cli -testnet listzerocoinamounts
error code: -13
error message:
Error: Please enter the wallet passphrase with walletpassphrase first.

C:\Users\me\Veil-Project\veil-pr770> veil-cli -testnet -stdinwalletpassphrase walletpassphrase false 10000
Wallet passphrase>

C:\Users\me\Veil-Project\veil-pr770> veil-cli -testnet listzerocoinamounts
[
  {
    "denomination": 10,
    "mints_spendable": 172,
    "mints_pending": 9
  },
  {
    "denomination": 100,
    "mints_spendable": 7,
    "mints_pending": 0
  },
  {
    "denomination": 1000,
    "mints_spendable": 2,
    "mints_pending": 0
  },
  {
    "denomination": 10000,
    "mints_spendable": 0,
    "mints_pending": 0
  }
]

@CaveSpectre11 CaveSpectre11 added QA: Passed This has passed QA testing and can be merged to master and removed QA: In Progress This is being tested by the QA team. KEEP CALM and RELAX. labels Jul 10, 2020
@CaveSpectre11 CaveSpectre11 merged commit 0f6e34e into Veil-Project:master Jul 10, 2020
@CaveSpectre11 CaveSpectre11 added the Dev Status: Merged Issue is completely finished. label Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Review: Passed Component: RPC Related to the console commands themselves. Dev Status: Merged Issue is completely finished. QA: Passed This has passed QA testing and can be merged to master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants