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

chore: not display privatekey #11006

Merged
merged 1 commit into from
Jun 28, 2023

Conversation

beck-8
Copy link
Contributor

@beck-8 beck-8 commented Jun 26, 2023

Related Issues

Let the user's private key no longer be displayed in the terminal to prevent malicious monitoring of the screen and stealing the private key.

Proposed Changes

$ ./lotus wallet import
Enter private key(not display in the terminal): 
imported key f1xxxxxxx successfully!

$ ./lotus wallet import
Enter private key(not display in the terminal): 
ERROR: saving to keystore: checking key before put 'wallet-f1xxxxxxx': key already exists

$ echo 7bxxxxxxx | ./lotus wallet import
imported key f1xxxxxxx successfully!

$ echo 7bxxxxxxx | ./lotus wallet import
ERROR: saving to keystore: checking key before put 'wallet-f1xxxxxxx': key already exists

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

@beck-8 beck-8 requested a review from a team as a code owner June 26, 2023 16:02
@Stebalien
Copy link
Member

Seems reasonable to me but could you switch to https://pkg.go.dev/golang.org/x/term? The library you're using is deprecated.

@Stebalien
Copy link
Member

Ah, wait. So, it looks like this code is designed for reading the key from stdin, not from a user typing it.

@Stebalien
Copy link
Member

Stebalien commented Jun 26, 2023

Hm... although it still prompts.... Ok, I think the right answer is:

  1. Check IsTerminal.
  2. If it's a terminal, read it as a password (letting the user type it).
  3. Otherwise, just read it from stdin.

@beck-8
Copy link
Contributor Author

beck-8 commented Jun 26, 2023

Seems reasonable to me but could you switch to https://pkg.go.dev/golang.org/x/term? The library you're using is deprecated.

this is ok

@beck-8
Copy link
Contributor Author

beck-8 commented Jun 26, 2023

Hm... although it still prompts.... Ok, I think the right answer is:

  1. Check IsTerminal.
  2. If it's a terminal, read it as a password (letting the user type it).
  3. Otherwise, just read it from stdin.

I just want to hide the private key display in the original logic, and the terminal's judgment already exists.I try my best to keep the original logic unchanged, so as to prevent some people's scripts from failing.

@beck-8 beck-8 force-pushed the not_display_privatekey branch from 931d3fc to 5244d89 Compare June 26, 2023 16:27
@beck-8 beck-8 closed this Jun 27, 2023
@beck-8 beck-8 force-pushed the not_display_privatekey branch from 5244d89 to a2431ff Compare June 27, 2023 18:36
@beck-8 beck-8 reopened this Jun 27, 2023
@beck-8 beck-8 force-pushed the not_display_privatekey branch from 3c81156 to c7ef091 Compare June 27, 2023 18:39
@Stebalien
Copy link
Member

Are you sure this works when stdin is a pipe, not a terminal? That's why I suggested calling IsTerminal first.

@beck-8
Copy link
Contributor Author

beck-8 commented Jun 27, 2023

Are you sure this works when stdin is a pipe, not a terminal? That's why I suggested calling IsTerminal first.

I see what you mean, thank you very much.

@beck-8 beck-8 force-pushed the not_display_privatekey branch from c7ef091 to f5680c7 Compare June 27, 2023 19:13
@beck-8
Copy link
Contributor Author

beck-8 commented Jun 27, 2023

forgive me for using force-push, I just want to make the commit cleaner

@beck-8 beck-8 force-pushed the not_display_privatekey branch from f5680c7 to 2cf19d4 Compare June 27, 2023 19:30
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM!

@arajasek arajasek merged commit 2df1697 into filecoin-project:master Jun 28, 2023
@beck-8 beck-8 deleted the not_display_privatekey branch June 29, 2023 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants