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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Displaying sensitive information during key creation with using less so is not saved in terminal history #50

Merged
merged 11 commits into from
Feb 20, 2024

Conversation

DanielBoye
Copy link
Contributor

@DanielBoye DanielBoye commented Feb 9, 2024

Fixes the TODO in the code for displaying sensitive information in:

// TODO: display it using `less` of `vi` so that it is not saved in terminal history
fmt.Println("BLS Private Key: " + keyPair.PrivKey.String())
fmt.Println("Please backup the above private key in safe place.")
fmt.Println()
fmt.Println("BLS Pub key: " + keyPair.PubKey.String())
fmt.Println("Key location: " + fileLoc)
return nil

and
// TODO: display it using `less` of `vi` so that it is not saved in terminal history
fmt.Println("ECDSA Private Key (Hex): ", privateKeyHex)
fmt.Println("\033[1;32m馃攼 Please backup the above private key hex in a safe place 馃敀\033[0m")
fmt.Println("Key location: " + fileLoc)
publicKey := privateKey.Public()
publicKeyECDSA, ok := publicKey.(*ecdsa.PublicKey)
if !ok {
return err
}

Motivation

  • Increased Security: By removing direct terminal outputs of private keys, we significantly reduce the risk of sensitive data exposure. This change is crucial for users who manage keys in shared or insecure environments.

  • Improved Usability: The introduction of a formatted display for key information, including private and public keys, addresses, and file locations, enhances clarity and user experience. This structured presentation makes it easier for users to securely manage and backup their key information.

  • Error Handling and Validation: Enhancements in error handling and input validation further contribute to the robustness of key management.

Solution

The solution of how to secure the display of key information was to implement a function that displays the key details securely using less, avoiding direct terminal output. A new function called displayWithLess is introduced and called in the saveBlsKey and saveEcdsaKey. The ASCII art is dynamically generated by taking the length and calculating padding for a nice format.

Open questions

Would like feedback on the ASCII art and how we would improve more clarity on the saving of the hash or key (regarding bls or ecdsa) since it is not quite intuitive to add colours with the less command.

Pictures

BLS Private key

image

ECDSA Private key generation

image

@DanielBoye
Copy link
Contributor Author

@shrimalmadhur does the solution align with what you thought it would look like?

@shrimalmadhur
Copy link
Contributor

@shrimalmadhur does the solution align with what you thought it would look like?

oh so sorry @DanielBoye I missed it but this is nice. can you rebase and resolve conflict?

@shrimalmadhur shrimalmadhur added the enhancement New feature or request label Feb 16, 2024
@DanielBoye
Copy link
Contributor Author

@shrimalmadhur does the solution align with what you thought it would look like?

oh so sorry @DanielBoye I missed it but this is nice. can you rebase and resolve conflict?

Will do this when we land on wenether the display of key location and public key should be before or after the private key as for my question in our conversation 馃槈

@DanielBoye
Copy link
Contributor Author

Looks like this now for ecdsa:
image
image

and for bls:
image
image

@DanielBoye
Copy link
Contributor Author

@shrimalmadhur updated it now and resolved the conflict. Can merge now :)

@shrimalmadhur
Copy link
Contributor

@shrimalmadhur updated it now and resolved the conflict. Can merge now :)

Thank you!! Sorry I have one more question - I recently added this commit fdabe37 which adds a way for users to automate key creation by piping the password and no human interaction. What will happen with this case, do you know?

@DanielBoye
Copy link
Contributor Author

That will work. Your commit has been incorporated into my fork, accessible at: https://github.com/DanielBoye/eigenlayer-cli/blob/master/pkg/operator/keys/create.go

Last time I checked it worked. If the functionality does not work, I will fix it today with another PR for traceability. That is no problem 馃槈

I suggest we merge this pr, and should any issues arise with the piping, I will fix it today.

@shrimalmadhur shrimalmadhur merged commit 23809a3 into Layr-Labs:master Feb 20, 2024
3 checks passed
@shrimalmadhur
Copy link
Contributor

That will work. Your commit has been incorporated into my fork, accessible at: https://github.com/DanielBoye/eigenlayer-cli/blob/master/pkg/operator/keys/create.go

Last time I checked it worked. If the functionality does not work, I will fix it today with another PR for traceability. That is no problem 馃槈

I suggest we merge this pr, and should any issues arise with the piping, I will fix it today.

merged. thank you for this. we need to make sure that - this works with piping and no human intervention is required when someone use pipe to input password.

@DanielBoye
Copy link
Contributor Author

@shrimalmadhur tested it now and it works completely fine with your work on piping the password 馃挴:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants