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.

Sign up for GitHub

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

feat(client): Add flag & reading mnemonic from file #20690

Merged
merged 7 commits into from
Jun 18, 2024
Merged

feat(client): Add flag & reading mnemonic from file #20690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e0a3735
add flag for reading mnemonic from file
hieuvubk Jun 17, 2024
6d7a634
Merge branch 'main' into hieu/mnemonic_from_file
hieuvubk Jun 17, 2024
e065be7
add changelog & fix comment
hieuvubk Jun 17, 2024
b838bcd
Merge branch 'main' into hieu/mnemonic_from_file
hieuvubk Jun 17, 2024
e05bfa2
example doc
hieuvubk Jun 18, 2024
454a697
Merge branch 'hieu/mnemonic_from_file' of https://github.com/cosmos/c…
hieuvubk Jun 18, 2024
a86dda8
Merge branch 'main' into hieu/mnemonic_from_file
hieuvubk Jun 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i

### Features

* (client) [#20690](https://github.com/cosmos/cosmos-sdk/pull/20690) Import mnemonic from file
* (tests) [#20013](https://github.com/cosmos/cosmos-sdk/pull/20013) Introduce system tests to run multi node local testnet in CI
* (runtime) [#19953](https://github.com/cosmos/cosmos-sdk/pull/19953) Implement `core/transaction.Service` in runtime.
* (client) [#19905](https://github.com/cosmos/cosmos-sdk/pull/19905) Add grpc client config to `client.toml`.
Expand Down
50 changes: 44 additions & 6 deletions client/keys/add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
"encoding/json"
"errors"
"fmt"
"io"
"os"
"sort"
"strings"

Expand Down Expand Up @@ -38,6 +40,7 @@
flagHDPath = "hd-path"
flagPubKeyBase64 = "pubkey-base64"
flagIndiscreet = "indiscreet"
flagMnemonicSrc = "source"

// DefaultKeyPass contains the default key password for genesis transactions
DefaultKeyPass = "12345678"
Expand All @@ -60,6 +63,11 @@
Use the --pubkey flag to add arbitrary public keys to the keystore for constructing
multisig transactions.

Use the --source flag to import mnemonic from a file in recover or interactive mode.
Example:

keys add testing --recover --source ./mnemonic.txt

You can create and store a multisig key by passing the list of key names stored in a keyring
and the minimum number of signatures required through --multisig-threshold. The keys are
sorted by address, unless the flag --nosort is set.
Expand Down Expand Up @@ -87,6 +95,7 @@
f.Uint32(flagIndex, 0, "Address index number for HD derivation (less than equal 2147483647)")
f.String(flags.FlagKeyType, string(hd.Secp256k1Type), "Key signing algorithm to generate keys for")
f.Bool(flagIndiscreet, false, "Print seed phrase directly on current terminal (only valid when --no-backup is false)")
f.String(flagMnemonicSrc, "", "Import mnemonic from a file (only usable when recover or interactive is passed)")

// support old flags name for backwards compatibility
f.SetNormalizeFunc(func(f *pflag.FlagSet, name string) pflag.NormalizedName {
Expand Down Expand Up @@ -282,19 +291,34 @@
var mnemonic, bip39Passphrase string

recoverFlag, _ := cmd.Flags().GetBool(flagRecover)
mnemonicSrc, _ := cmd.Flags().GetString(flagMnemonicSrc)
if recoverFlag {
mnemonic, err = input.GetString("Enter your bip39 mnemonic", inBuf)
if err != nil {
return err
if mnemonicSrc != "" {
mnemonic, err = readMnemonicFromFile(mnemonicSrc)
if err != nil {
return err
}
} else {
mnemonic, err = input.GetString("Enter your bip39 mnemonic", inBuf)
if err != nil {
return err
}
}

if !bip39.IsMnemonicValid(mnemonic) {
return errors.New("invalid mnemonic")
}
} else if interactive {
mnemonic, err = input.GetString("Enter your bip39 mnemonic, or hit enter to generate one.", inBuf)
if err != nil {
return err
if mnemonicSrc != "" {
mnemonic, err = readMnemonicFromFile(mnemonicSrc)
if err != nil {
return err
}
} else {
mnemonic, err = input.GetString("Enter your bip39 mnemonic, or hit enter to generate one.", inBuf)
if err != nil {
return err
}
Comment on lines +294 to +321
Copy link
Contributor

Choose a reason for hiding this comment

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

Tip

Codebase Verification

Consider enhancing the error handling in readMnemonicFromFile to provide more user-friendly and informative error messages. Currently, the function returns raw errors from os.Open and io.ReadAll, which may not be clear to end-users.

  • client/keys/add.go:430: Wrap the error from os.Open with a message indicating the file could not be opened.
  • client/keys/add.go:436: Wrap the error from io.ReadAll with a message indicating the file could not be read.
Analysis chain

The implementation for reading the mnemonic from a file is correctly placed under the conditions checking the flagRecover and flagInteractive. However, consider handling the scenario where the file path might be incorrect or the file might be empty.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify error handling for empty or incorrect file paths.

# Test: Check if error messages for file read failures are user-friendly and informative.
rg --type go $'readMnemonicFromFile' client/keys/add.go

Length of output: 221

Script:

#!/bin/bash
# Extract the implementation of the readMnemonicFromFile function to review error handling.
ast-grep --lang go --pattern $'func readMnemonicFromFile(filePath string) (string, error) {
  $$$
}' client/keys/add.go

Length of output: 662

}

if !bip39.IsMnemonicValid(mnemonic) && mnemonic != "" {
Expand Down Expand Up @@ -404,3 +428,17 @@

return nil
}

func readMnemonicFromFile(filePath string) (string, error) {
file, err := os.Open(filePath)
Dismissed Show dismissed Hide dismissed
Copy link
Contributor

Choose a reason for hiding this comment

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

Mitigate potential file inclusion vulnerability.

The function readMnemonicFromFile opens a file based on a user-provided path without sanitizing or validating the path, which could lead to a file inclusion vulnerability. Consider validating the file path or using a safer method to handle file paths.

+ if !isValidPath(filePath) {
+     return "", fmt.Errorf("invalid file path provided")
+ }
  file, err := os.Open(filePath)

Committable suggestion was skipped due to low confidence.

Tools
GitHub Check: gosec

[failure] 428-428: Potential file inclusion via variable
Potential file inclusion via variable

if err != nil {
return "", err
}
defer file.Close()

bz, err := io.ReadAll(file)
if err != nil {
return "", err
}
return string(bz), nil
}
Comment on lines +431 to +444
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error handling in readMnemonicFromFile.

The function readMnemonicFromFile directly returns the errors from os.Open and io.ReadAll without any contextual information. Enhance the error messages to provide more context to the user.

- return "", err
+ return "", fmt.Errorf("failed to open mnemonic source file: %w", err)
- return "", err
+ return "", fmt.Errorf("failed to read mnemonic from file: %w", err)
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func readMnemonicFromFile(filePath string) (string, error) {
file, err := os.Open(filePath)
if err != nil {
return "", err
}
defer file.Close()
bz, err := io.ReadAll(file)
if err != nil {
return "", err
}
return string(bz), nil
}
func readMnemonicFromFile(filePath string) (string, error) {
file, err := os.Open(filePath)
if (err != nil) {
return "", fmt.Errorf("failed to open mnemonic source file: %w", err)
}
defer file.Close()
bz, err := io.ReadAll(file)
if (err != nil) {
return "", fmt.Errorf("failed to read mnemonic from file: %w", err)
}
return string(bz), nil
}
Tools
GitHub Check: gosec

[failure] 428-428: Potential file inclusion via variable
Potential file inclusion via variable

Loading