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

Improve rpcauth.py by using argparse and getpass modules #14756

Merged
merged 1 commit into from Nov 22, 2018

Conversation

Projects
None yet
6 participants
@promag
Copy link
Member

commented Nov 19, 2018

This PR improves argument handling in rpcauth.py script by using argparse module. Specifying - as password makes it prompt securely with getpass module which prevents leaking passwords to bash history.

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

Based on #14742.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

Concept ACK

Nice usability improvement!

@promag promag changed the title Improve by using argparse and getpass modules Improve rpcauth.py by using argparse and getpass modules Nov 19, 2018

@dongcarl

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

utACK

@promag promag force-pushed the promag:2018-11-improve-rpcauth.py branch Nov 21, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

Rebased.

@promag promag force-pushed the promag:2018-11-improve-rpcauth.py branch Nov 21, 2018

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

You're changing a ton of unrelated things in this PR, making it hard to review.

This is not a very complicated script so I'll utACK it, but please don't do this next time.

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

@laanwj indeed it escalated, the original intent was to add getpass module.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

Conditional tested ACK ffba16cab0f1549d8b5b18bd254fb52ab44b2876.

This changes the interface to rpcauth.py: previously rpcauth.py <username> <pw> would work. Now it errors with:

/rpcauth.py satoshi p4ssw0rd
usage: rpcauth.py [-h] [-p PASSWORD] username
rpcauth.py: error: unrecognized arguments: p4ssw0rd

Is that ok? If this script is only used manually and occasionally, then it's fine, but if it's called by other scripts it could be annoying.

Note that we don't have a standalone test for rpcauth, and the coverage provided by rpc_users.py doesn't test providing a password, so didn't catch this interface change.

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

@jnewbery I guess I can make it compatible.

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

@jnewbery updated to maintain the same interface. (I'll squash and update OP after feedback)

@jnewbery

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

tACK b8b321776e43b0fbf7a3e131e3af827eb892768b. Much better, thanks!

@promag promag force-pushed the promag:2018-11-improve-rpcauth.py branch to d6cde00 Nov 21, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 21, 2018

Thanks @jnewbery, squashed and updated OP.

@jnewbery

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

ACK d6cde00. Verified the same as b8b3217

@laanwj laanwj merged commit d6cde00 into bitcoin:master Nov 22, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 22, 2018

Merge #14756: Improve rpcauth.py by using argparse and getpass modules
d6cde00 rpcauth: Improve by using argparse and getpass modules (João Barbosa)

Pull request description:

  This PR improves argument handling in `rpcauth.py` script by using `argparse` module. Specifying `-` as password makes it prompt securely with `getpass` module which prevents leaking passwords to bash history.

Tree-SHA512: 489d66c95f66b5618cb75fd8f07ea5647281226ab9e32b03051eb43f758b9334ac19b7c82c2ed4f8c7ffbb0bee949b3d389e1564ec7a6e372f2864233bc7cb88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.