Safer way to handle unlock command of cli_wallet #1171 #1382

cogutvalera · 2018-10-16T07:36:55Z

PR for #1171

bitshares/bitshares-fc#82

cogutvalera · 2018-10-24T07:13:44Z

this PR troglobit/editline#21 was merged already.

Guys check please other 2 PRs when you will have time for it:

Thanks !

cogutvalera · 2018-10-25T08:14:59Z

fixed conflicts with latest develop branch and rebased

cogutvalera · 2018-10-28T09:53:17Z

will bump FC now after merged PR bitshares/bitshares-fc#82

cogutvalera · 2018-10-28T19:19:04Z

need to be rebased after merging PR #1405

cogutvalera · 2018-10-29T20:19:09Z

rebased already

pmconrad · 2018-10-30T14:16:57Z

programs/cli_wallet/main.cpp

@@ -72,6 +72,9 @@ int main( int argc, char** argv )
 {
   try {

+      const char* regex_secret_name = "regex-secret";
+      const string regex_secret = "^(unlock|set_password)\\s.*";


Must allow \s* at the start of the line.

Done ! Thanks !

cogutvalera · 2018-10-30T14:28:03Z

Ok no problems ! Thanks !

pmconrad · 2018-10-30T14:54:18Z

Seems to work as designed

BUT

this solution is unusable IMO. Once the line matches the regexp there is no visual feedback - no cursor movement, no placeholder for keys pressed etc..

cogutvalera · 2018-10-30T14:59:42Z

I wanted to add placeholder '*' but made without it for security reasons for example as git works in command line with passwords

pmconrad · 2018-10-31T15:58:15Z

I'm aware of two ways to enter passwords (well, three if you count cleartext):

a separate prompt for only the password, with no visual feedback
visual feedback where password characters are replaced with asterisk or bullet points

I've never seen a normal editable command line that suddenly stops all visual feedback while still accepting (and expecting) input. A normal user will think the client is frozen, and will try to kill it, and then complain here.

cogutvalera · 2018-10-31T16:36:30Z

ok ! I will improve it !

visual feedback where password characters are replaced with asterisk or bullet points

or whitespaces also possible (just cursor feedback) ? Right ?

cogutvalera · 2018-11-01T06:46:46Z

I've added new PR in editline project troglobit/editline#22 for visual feedback while typing secret information

pmconrad · 2018-11-01T13:45:34Z

or whitespaces also possible (just cursor feedback) ? Right ?

I think something other would be preferrable. Try to imagine how you as an unsuspecting user would react when observing that behaviour for the first time. I bet you'd find it confusing.

cogutvalera · 2018-11-01T14:23:40Z

@pmconrad I mean cursor movement

Seems to work as designed

BUT

this solution is unusable IMO. Once the line matches the regexp there is no visual feedback - no cursor movement, no placeholder for keys pressed etc..

As you told cursor movement is already done in new PR for editline here troglobit/editline#22

cogutvalera · 2018-11-02T12:48:30Z

@pmconrad did you have a chance to check locally how works my new PR in editline with visual feedback by cursor as you told earlier to me ? Or didn't you have time to check it ? If yes, then later when having time to check it tell me please if it is good enough or not ?

Thanks !

JFYI @troglobit commented here troglobit/editline#22 (comment) in my new PR with visual feedback by cursor. I'm thinking what should I answer to him cause my answer depends on your review. Thanks for your time and efforts !

cogutvalera · 2018-11-14T18:54:21Z

rebased due to conflicts

cogutvalera · 2018-11-22T06:06:19Z

Guys what should we do with this PR ? Look please when you will have time + new PR in FC library

bitshares/bitshares-fc#97

troglobit · 2018-11-22T10:32:34Z

@cogutvalera I'm still open to a PR that adds visual feedback when in el_no_echo=1 mode. E.g., an el_no_echo_indicator="*" or similar would be useful.

pmconrad · 2019-04-12T13:22:55Z

You can remove the change in CMakeLists.txt - the dependency is inherited from fc.

I don't think it makes much sense to have a command line option for the regex. Better remove it.

cogutvalera · 2019-06-02T08:16:16Z

You can remove the change in CMakeLists.txt - the dependency is inherited from fc.

I don't think it makes much sense to have a command line option for the regex. Better remove it.

About command line option for the regex I did it before you told earlier that it can be more flexible if there will be possibility to configure it for any desired data like brain key for example (look here your comment #1171 (comment)). I see next 3 possible ways for configuring this mechanism:

Configure by command line argument
Configure by config file
Configure by both: command line arg & config file

Which one do you prefer @pmconrad ? Guys what do you think too ? What is the best way ?

pmconrad · 2019-06-02T13:04:10Z

What I meant with "configurable by the calling application" is that fc::rpc::cli shouldn't have this hardcoded, but that the application that uses fc::rpc::cli should define the regex. I didn't mean "configurable by the user".

IMO neither cli option nor config file option make sense. Definition via static const in main.cpp is sufficient.

pmconrad · 2019-06-05T10:59:15Z

Merged bitshares/bitshares-fc#97 . Please rebase on latest develop and bump fc to latest master.

programs/cli_wallet/main.cpp

CMakeLists.txt

programs/cli_wallet/main.cpp

pmconrad · 2019-06-17T09:44:57Z

Please squash your changes, we don't need 5 commits in the history for a one-liner.

cogutvalera · 2019-06-17T14:36:15Z

Please squash your changes, we don't need 5 commits in the history for a one-liner.

ok sure, not a problem at all !

abitmore

Thanks.

cogutvalera · 2019-06-18T10:36:10Z

Nice that it is already merged ! Thank you !

cogutvalera mentioned this pull request Oct 16, 2018

Safer way to handle unlock command of cli_wallet #1171

Closed

14 tasks

cogutvalera force-pushed the issue_1171 branch 3 times, most recently from 1263fc9 to 88029da Compare October 25, 2018 08:14

cogutvalera force-pushed the issue_1171 branch from 88029da to c2de850 Compare October 28, 2018 09:58

cogutvalera mentioned this pull request Oct 28, 2018

bump fc and fix node #1405

Merged

cogutvalera force-pushed the issue_1171 branch from c2de850 to 022b6c6 Compare October 29, 2018 20:14

cogutvalera mentioned this pull request Oct 30, 2018

Safer way to handle unlock command of cli_wallet #1171 bitshares/bitshares-fc#86

Merged

pmconrad reviewed Oct 30, 2018

View reviewed changes

cogutvalera mentioned this pull request Nov 1, 2018

Added visual feedback while hiding secret information troglobit/editline#22

Closed

cogutvalera force-pushed the issue_1171 branch from 328f06e to 84ad039 Compare November 14, 2018 18:53

abitmore added this to the 3.2.0 - Feature Release milestone Jun 6, 2019

abitmore reviewed Jun 12, 2019

View reviewed changes

programs/cli_wallet/main.cpp Outdated Show resolved Hide resolved

programs/cli_wallet/main.cpp Outdated Show resolved Hide resolved

CMakeLists.txt Outdated Show resolved Hide resolved

cogutvalera force-pushed the issue_1171 branch from f3e7aaf to b337247 Compare June 15, 2019 13:36

abitmore reviewed Jun 16, 2019

View reviewed changes

programs/cli_wallet/main.cpp Outdated Show resolved Hide resolved

cogutvalera force-pushed the issue_1171 branch 2 times, most recently from 12af65f to 6aad78b Compare June 17, 2019 14:43

Safer way to handle unlock command of cli_wallet bitshares#1171

b8f8591

cogutvalera force-pushed the issue_1171 branch from 6aad78b to b8f8591 Compare June 17, 2019 14:43

abitmore approved these changes Jun 18, 2019

View reviewed changes

abitmore merged commit 9cf1006 into bitshares:develop Jun 18, 2019

pmconrad mentioned this pull request Jun 20, 2019

Release Notes: BitShares Core 3.2.0 #1808

Closed

abitmore mentioned this pull request Aug 7, 2019

Update docs about set_password and unlock command in cli_wallet #1886

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safer way to handle unlock command of cli_wallet #1171 #1382

Safer way to handle unlock command of cli_wallet #1171 #1382

cogutvalera commented Oct 16, 2018 •

edited

Loading

cogutvalera commented Oct 24, 2018

cogutvalera commented Oct 25, 2018 •

edited

Loading

cogutvalera commented Oct 28, 2018

cogutvalera commented Oct 28, 2018

cogutvalera commented Oct 29, 2018

pmconrad Oct 30, 2018

cogutvalera Oct 30, 2018

cogutvalera commented Oct 30, 2018

pmconrad commented Oct 30, 2018

cogutvalera commented Oct 30, 2018

pmconrad commented Oct 31, 2018

cogutvalera commented Oct 31, 2018 •

edited

Loading

cogutvalera commented Nov 1, 2018

pmconrad commented Nov 1, 2018

cogutvalera commented Nov 1, 2018

cogutvalera commented Nov 2, 2018 •

edited

Loading

cogutvalera commented Nov 14, 2018

cogutvalera commented Nov 22, 2018

troglobit commented Nov 22, 2018

pmconrad commented Apr 12, 2019

cogutvalera commented Jun 2, 2019

pmconrad commented Jun 2, 2019

pmconrad commented Jun 5, 2019

pmconrad commented Jun 17, 2019

cogutvalera commented Jun 17, 2019

abitmore left a comment

cogutvalera commented Jun 18, 2019

Safer way to handle unlock command of cli_wallet #1171 #1382

Safer way to handle unlock command of cli_wallet #1171 #1382

Conversation

cogutvalera commented Oct 16, 2018 • edited Loading

cogutvalera commented Oct 24, 2018

cogutvalera commented Oct 25, 2018 • edited Loading

cogutvalera commented Oct 28, 2018

cogutvalera commented Oct 28, 2018

cogutvalera commented Oct 29, 2018

pmconrad Oct 30, 2018

Choose a reason for hiding this comment

cogutvalera Oct 30, 2018

Choose a reason for hiding this comment

cogutvalera commented Oct 30, 2018

pmconrad commented Oct 30, 2018

cogutvalera commented Oct 30, 2018

pmconrad commented Oct 31, 2018

cogutvalera commented Oct 31, 2018 • edited Loading

cogutvalera commented Nov 1, 2018

pmconrad commented Nov 1, 2018

cogutvalera commented Nov 1, 2018

cogutvalera commented Nov 2, 2018 • edited Loading

cogutvalera commented Nov 14, 2018

cogutvalera commented Nov 22, 2018

troglobit commented Nov 22, 2018

pmconrad commented Apr 12, 2019

cogutvalera commented Jun 2, 2019

pmconrad commented Jun 2, 2019

pmconrad commented Jun 5, 2019

pmconrad commented Jun 17, 2019

cogutvalera commented Jun 17, 2019

abitmore left a comment

Choose a reason for hiding this comment

cogutvalera commented Jun 18, 2019

cogutvalera commented Oct 16, 2018 •

edited

Loading

cogutvalera commented Oct 25, 2018 •

edited

Loading

cogutvalera commented Oct 31, 2018 •

edited

Loading

cogutvalera commented Nov 2, 2018 •

edited

Loading