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

Safer way to handle unlock command of cli_wallet #1171

Closed
7 of 14 tasks
HarukaMa opened this issue Jul 22, 2018 · 58 comments
Closed
7 of 14 tasks

Safer way to handle unlock command of cli_wallet #1171

HarukaMa opened this issue Jul 22, 2018 · 58 comments
Assignees
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX)

Comments

@HarukaMa
Copy link
Contributor

HarukaMa commented Jul 22, 2018

User Story
The unlock command should not echo back the wallet password for safety concerns.

Ideally, this command should be handled interactively, like

locked >>> unlock
Enter password:
unlocked >>>

completely without showing the password on the console.

Additional Context (optional)
Maybe all commands related to unencrypted WIF should also apply to this.

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
  • Refine User Stories / Requirements
  • Define Test Cases
    • Windows
  • Design / Develop Solution
    • Linux & MacOS
      • Assigned: @cogutvalera
      • Estimated: 15 hours
      • Remitted: weeks 42-43
    • Windows
  • Perform QA/Testing
  • Update Documentation
@abitmore
Copy link
Member

Just a note: please keep the unlock wallet JSON-RPC API usable for backward compatibility. Exchanges may be using it.

@oxarbitrage
Copy link
Member

oxarbitrage commented Jul 31, 2018

Agree @abitmore . We need to add a new call with no arguments safe_unlock.

Problem is it seems there is no easy way of making it work in linux/windows/macos with the same code.
It seems for linux/macos the use of https://www.gnu.org/software/libc/manual/html_node/getpass.html is how to do it as the c++ call getpass(present in most systems) will be deprecated.

In windows it needs to be done different, it seems an ncurses solution may work in both cases but i think it will be too much to add ncurses support just to do this.

We can start building a version only for linux and decide what to do in windows later. Other suggestions are welcome.

@cogutvalera
Copy link
Member

I want to claim this issue

@ryanRfox ryanRfox added 1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX) labels Aug 3, 2018
@cogutvalera
Copy link
Member

Thanks !

@ryanRfox
Copy link
Contributor

ryanRfox commented Aug 3, 2018

@cogutvalera please note @oxarbitrage comment above about implementing safe_unlock so no impact to existing implementation. I suggest to begin first by building your test case for the Linux platform, then get a working implementation for that prior to moving on to the other platforms. As needed, we can merge each to develop to obtain this added functionality. Please be active with your comments about your research into a cross-platform solution.

May I suggest we start with 2 hours for research and first test case, then discuss estimation for a Linux implementation?

@cogutvalera
Copy link
Member

ok sure ! Thank you very much !

@cogutvalera
Copy link
Member

@ryanRfox after my researches I found that the best cross-platform solution without impacting existing JSON-RPC API implementation as @abitmore mentioned, is not to show password thus we should replace password with empty string on the fly in editline. I think @oxarbitrage told very nice solution, but IMHO more complex & complicated at the same time. So my solution is cross-platform and will fix issue without changing existing implementation.

Key points:

  1. We shouldn't break existing JSON-RPC API implementation
  2. We should extend editline existing implementation to change password on the fly
  3. We shouldn't break editline existing implementation

@ryanRfox my estimation for this issue is approximately 10 hours.

Thanks !

@pmconrad
Copy link
Contributor

Interesting idea. If you choose that route, please make it somehow configurable (for the calling application) when input is blanked out. That would make it possible to use the same mechanism not only for unlock but also for entering a brain key for example.

Glancing over the editline code my impression is that your approach will be tricky to get right, to say the least. Can you give us some high-level pseudocode of what your plan is?

@cogutvalera
Copy link
Member

cogutvalera commented Oct 13, 2018

@pmconrad yes for sure I want to make it configurable for any other sensitive information also and not only for unlock.

After my researches and some debugging inside editline code, I want to extend tty_flush method (it is the main idea will be for this issue, but not only this of course) so inside this method we have write call which will hide on the fly sensitive information :) something like that !

High-level pseudo-code:

      if (rl_line_safe)
         res = write(el_outfd, "", 1);
      else
         res = write(el_outfd, Screen, ScreenCount);

What should I do:

  1. Do not break JSON-RPC API existing implementation
  2. Do not break editline existing implementation
  3. Do configurable mechanism for any sensitive information (not only for unlock)

Thanks !

@abitmore
Copy link
Member

AFAIK editline doesn't work in Windows.

@cogutvalera
Copy link
Member

@abitmore yes you're absolutely right about editline !

https://github.com/troglobit/editline

Editline was originally designed for older UNIX systems and Plan 9. The current maintainer works exclusively on GNU/Linux systems, so it may use GCC and GNU Make specific extensions here and there. This is not on purpose and patches or pull requests to correct this are most welcome!

For Windows I want to make change on the fly too (the same idea), but after finishing with editline for GNU/Linux systems.

@ryanRfox my current estimation of 10 hours doesn't include Windows solution. Let's discuss about Windows estimation after Linux/MacOS solution will be implemented as @oxarbitrage and you told earlier.

Thanks !

@cogutvalera
Copy link
Member

@ryanRfox extend please my estimation from 10 hours to 15 hours approximately for this issue, cause I'm fighting with segfault in boost::regex for editline, also I've spent much more hours than my estimation for this issue and still working on it.

Thanks !

@cogutvalera
Copy link
Member

there are 3 PRs (core, fc, editline):

  1. Safer way to handle unlock command of cli_wallet #1171 #1382
  2. Safer way to handle unlock command of cli_wallet #1171 bitshares-fc#82
  3. Hide secret information troglobit/editline#21

What about PR for editline ? Who will approve it ? @abitmore @pmconrad @jmjatlanta @ryanRfox @oxarbitrage can you help me please to understand more clearly how PR in editline will be approved for BitShares ?

Thank you very much !

@jmjatlanta
Copy link
Contributor

Editline is a project by troglobit. They can choose to accept your change or not.

The joy of open source is that you can fork their project and change what you want.

If you want Bitshares to use your code instead of troglobit, I see two options:

  1. Make a PR in the FC repo that changes the editline repo from troglobit to your forked version.
  2. Have the bitshares repo fork off troglobit. You can then make a PR for the core team to approve.

If we think there will be only minor changes to editline, I would say option 1 is fine. If we think there will be many, and many contributors, I would say option 2 is better.

@cogutvalera
Copy link
Member

The joy of open source is that you can fork their project and change what you want.
@jmjatlanta absolutely agree with you !

my question is: what way should we choose from 2 options that you told :)

@pmconrad
Copy link
Contributor

We should not link to versions in individual developer repos. If we want your solution and troglobit doesn't approve your PR, we will have to fork it in the bitshares project.

cogutvalera added a commit to cogutvalera/bitshares-core that referenced this issue Jun 17, 2019
abitmore added a commit that referenced this issue Jun 18, 2019
Safer way to handle unlock command of cli_wallet #1171
@abitmore abitmore added the 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added label Jun 18, 2019
@abitmore
Copy link
Member

Implemented with PR #1382.

Docs are needed for closing this.

@cogutvalera
Copy link
Member

cogutvalera commented Jun 18, 2019

Also do we need windows (without editline) solution ? right ?

@cogutvalera
Copy link
Member

cogutvalera commented Jun 19, 2019

should I estimate windows (without editline) solution @ryanRfox ?

@cogutvalera
Copy link
Member

@abitmore @pmconrad what do you think guys ?

@abitmore
Copy link
Member

IMHO don't waste time on Windows so far.

@cogutvalera
Copy link
Member

ok

@oxarbitrage
Copy link
Member

Is the windows behaviour the same for unlock than it was for linux before the new code ?

If so, we should close this issue and open a new one for windows fix.

@abitmore
Copy link
Member

Note: docs are needed for closing this issue (including readme, wiki, tutorials, how.bitshares.works and etc).

  • If typed unlock or set_password command in cli_wallet with no parameter, there will be a prompt for inputting password
  • the new behavior only applies to Linux and macOS

@pmconrad
Copy link
Contributor

Ping @cedar-book - no rush

@cedar-book
Copy link

Okay! Thank you.

@cedar-book
Copy link

Hi @pmconrad, I updated this section 3.Unlock the Cli_Wallet to show how it works.

@abitmore
Copy link
Member

@cedar-book as mentioned in Telegram and in above comment (#1171 (comment)), please update tutorial about set_password command as well, since we've extended the behavior.

new >>> set_password
Enter password:
locked >>>

By the way, the current tutorial is a bit confusing, since there are 2 unlock commands inside, one with the password in the command and with no additional note, the other with the password in a new line and with a note. In the note, it's not mentioned that both syntax are supported. I think it's better to put the note after the first unlock command.

@abitmore
Copy link
Member

Update: https://dev.bitshares.works/en/master/development/apps/cli_wallet.html#unlock-the-cli-wallet has been updated.

We may still want to update README of this repository, the CLI cook book, and etc.

@abitmore
Copy link
Member

abitmore commented Aug 7, 2019

Created new issue for the docs: #1886. Closing this one in order to close the 3.2 project and milestone.

@abitmore abitmore closed this as completed Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1b User Story The User Story details a requirement. It may reference a parent Epic. It may reference child Task(s) 2d Developing Status indicating currently designing and developing a solution 3c Enhancement Classification indicating a change to the functionality of the existing imlementation 5a Docs Needed Status specific to Documentation indicating the need for proper documentation to be added 6 API Impact flag identifying the application programing interface (API) 6 CLI Impact flag identifying the command line interface (CLI) wallet application 6 Security Impact flag identifying system/user security 6 UX Impact flag identifying the User Interface (UX)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants