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

refactor(client/keys): hide inputting when keys add -i ask for bip39 passphrase #18743

Merged
merged 12 commits into from
Dec 15, 2023

Conversation

Halimao
Copy link
Contributor

@Halimao Halimao commented Dec 14, 2023

Description

I think we should hide the inputting for bip39 passphrase when running <simd> keys add -i, and there is a repeat confirmation flow already here

Note:

This maybe break some old scripts that using length < 8 passphrase

Screenshots

Case using empty passphrase:
image

Case using custom passphrase hat length > 8:
image

Case using custom passphrase that length < 8:
image

@Halimao Halimao requested a review from a team as a code owner December 14, 2023 02:38
Copy link
Contributor

coderabbitai bot commented Dec 14, 2023

Walkthrough

The changes involve enhancing security and robustness in the handling of sensitive input, specifically for key management in the client application. The passphrase input process now hides the input and checks for minimum length, improving security. Additionally, the update includes error handling for non-existent or unauthorized module accounts in the x/bank module, and improved genesis validation in the x/gov module. These modifications aim to reduce the risk of errors and increase the application's resilience against potential misuse.

Changes

Files Summary
client/keys/add.go Enhanced passphrase input security with hidden input and minimum length check. Improved handling for multisig duplicate keys.
client/keys/add_test.go Updated tests to cover interactive key generation with password input validation.
client/input/input.go Added GetSecretString function for secure input retrieval.
CHANGELOG.md Documented improvements in client/keys, x/gov, and x/bank modules. Error handling in x/bank for module account issues and improved genesis validation in x/gov.

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ?


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can reply to a review comment made by CodeRabbit.
  • You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
  • You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@github-actions github-actions bot added C:CLI C:Keys Keybase, KMS and HSMs labels Dec 14, 2023
@Halimao Halimao changed the title refactor: use "input.GetPassword" for bip39 passphrase refactor(client/keys): use "input.GetPassword" for bip39 passphrase Dec 14, 2023
@Halimao Halimao marked this pull request as draft December 14, 2023 02:45
@Halimao Halimao changed the title refactor(client/keys): use "input.GetPassword" for bip39 passphrase refactor(client/keys): hide inputting when keys add -i ask for bip39 passphrase Dec 14, 2023
@Halimao Halimao marked this pull request as ready for review December 14, 2023 02:55
client/input/input.go Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
client/keys/add_test.go Outdated Show resolved Hide resolved
client/keys/add_test.go Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

This maybe break some old scripts that using length < 8 passphrase

Yeah, so I don't think we can merge that. People having short passwords would get locked out of re-adding their keys on a new machine.

@Halimao
Copy link
Contributor Author

Halimao commented Dec 15, 2023

This maybe break some old scripts that using length < 8 passphrase

Yeah, so I don't think we can merge that. People having short passwords would get locked out of re-adding their keys on a new machine.

Okay, let's just remove the length limitation~

@Halimao
Copy link
Contributor Author

Halimao commented Dec 15, 2023

This maybe break some old scripts that using length < 8 passphrase

Yeah, so I don't think we can merge that. People having short passwords would get locked out of re-adding their keys on a new machine.

@julienrbrt Done, I have reverted the limit for inputting length

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

ACK. I hope people will write it down properly and don't make the same typo twice :D

@tac0turtle tac0turtle added this pull request to the merge queue Dec 15, 2023
Merged via the queue into cosmos:main with commit 6cca5e3 Dec 15, 2023
55 of 57 checks passed
@Halimao Halimao deleted the feat/keys-add-safe branch December 15, 2023 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:CLI C:Keys Keybase, KMS and HSMs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants