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

Remove Sensitive RPC Data from Logs #2520

Merged
merged 4 commits into from Apr 3, 2019

Conversation

@JeffVandrewJr
Copy link
Contributor

commented Apr 1, 2019

This is a patch to fix #2424.

It modifies the bcli_args function to skip any arguments with sensitive RPC username/password data when printing bitcoin-cli data to logs.

This is my first code PR to the project; please accept my apologies ahead of time if I inadvertently violated any standards/procedures.

JeffVandrewJr added some commits Apr 1, 2019

@rustyrussell
Copy link
Contributor

left a comment

Thanks, this is a great fix!

I would recommend replacing, rather than eliminating, since user may get confused when debugging. Perhaps a temp var, and if it starts with rpcuser, replace with -rpcuser=..., and similar with rpcpassword?

JeffVandrewJr added some commits Apr 2, 2019

@JeffVandrewJr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@rustyrussell Thanks! I have made the changes which you suggested. Instead of skipping the rpcusername and rpcpassword arguments when printing, it appends them as normal but replaces the actual username or password with an ellipsis.

@niftynei
Copy link
Collaborator

left a comment

this looks great, but one of your commits d650bd6 won't compile, which breaks git bisect. can you sqaush your commit set into a single commit?

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

I can squash on merge. It's also 4 spaces vs tabs, but that's also a trivial fix :)

@rustyrussell

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Ack a88f39d

@JeffVandrewJr

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@rustyrussell Thank you. I will make sure that the next time I have a PR I will squash any non-compiling commits as well as be more careful with my tabs/spaces. 🙏

@rustyrussell rustyrussell merged commit 1130100 into ElementsProject:master Apr 3, 2019

2 checks passed

ackbot PR ack'd by rustyrussell
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.