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

keyring: remove hardcoded default passphrase on NewMnemonic #8662

Merged
merged 15 commits into from
Feb 23, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion client/keys/delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func Test_runDeleteCmd(t *testing.T) {
_, err = kb.NewAccount(fakeKeyName1, testutil.TestMnemonic, "", path, hd.Secp256k1)
require.NoError(t, err)

_, _, err = kb.NewMnemonic(fakeKeyName2, keyring.English, sdk.FullFundraiserPath, hd.Secp256k1)
_, _, err = kb.NewMnemonic(fakeKeyName2, keyring.English, sdk.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
require.NoError(t, err)

cmd.SetArgs([]string{"blah", fmt.Sprintf("--%s=%s", flags.FlagHome, kbHome)})
Expand Down
4 changes: 2 additions & 2 deletions client/tx/tx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,10 +132,10 @@ func TestSign(t *testing.T) {
var from2 = "test_key2"

// create a new key using a mnemonic generator and test if we can reuse seed to recreate that account
_, seed, err := kr.NewMnemonic(from1, keyring.English, path, hd.Secp256k1)
_, seed, err := kr.NewMnemonic(from1, keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
requireT.NoError(err)
requireT.NoError(kr.Delete(from1))
info1, _, err := kr.NewMnemonic(from1, keyring.English, path, hd.Secp256k1)
info1, _, err := kr.NewMnemonic(from1, keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
requireT.NoError(err)

info2, err := kr.NewAccount(from2, seed, "", path, hd.Secp256k1)
Expand Down
2 changes: 1 addition & 1 deletion crypto/armor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func TestArmorUnarmorPubKey(t *testing.T) {
cstore := keyring.NewInMemory()

// Add keys and see they return in alphabetical order
info, _, err := cstore.NewMnemonic("Bob", keyring.English, types.FullFundraiserPath, hd.Secp256k1)
info, _, err := cstore.NewMnemonic("Bob", keyring.English, types.FullFundraiserPath, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
require.NoError(t, err)
armored := crypto.ArmorPubKeyBytes(legacy.Cdc.Amino.MustMarshalBinaryBare(info.GetPubKey()), "")
pubBytes, algo, err := crypto.UnarmorPubKeyBytes(armored)
Expand Down
25 changes: 19 additions & 6 deletions crypto/keyring/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,16 @@ type Keyring interface {
Delete(uid string) error
DeleteByAddress(address sdk.Address) error

// NewMnemonic generates a new mnemonic, derives a hierarchical deterministic
// key from that, and persists it to the storage. Returns the generated mnemonic and the key
// Info. It returns an error if it fails to generate a key for the given algo type, or if
// NewMnemonic generates a new mnemonic, derives a hierarchical deterministic key from that, and
// persists it to the storage. Returns the generated mnemonic and the key Info secured with the given passphrase.
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
// It returns an error if it fails to generate a key for the given algo type, or if
// another key is already stored under the same name.
NewMnemonic(uid string, language Language, hdPath string, algo SignatureAlgo) (Info, string, error)
//
// NOTE: a passphrase set to 'default' will set the passphrase to the DefaultBIP39Passphrase value.
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
NewMnemonic(uid string, language Language, hdPath, passphase string, algo SignatureAlgo) (Info, string, error)
fedekunze marked this conversation as resolved.
Show resolved Hide resolved

// NewAccount converts a mnemonic to a private key and BIP-39 HD Path and persists it.
// It fails if there is an existing key Info with the same address
NewAccount(uid, mnemonic, bip39Passwd, hdPath string, algo SignatureAlgo) (Info, error)

// SaveLedgerKey retrieves a public key reference from a Ledger device and persists it.
Expand Down Expand Up @@ -477,7 +480,7 @@ func (ks keystore) List() ([]Info, error) {
return res, nil
}

func (ks keystore) NewMnemonic(uid string, language Language, hdPath string, algo SignatureAlgo) (Info, string, error) {
func (ks keystore) NewMnemonic(uid string, language Language, hdPath, passphrase string, algo SignatureAlgo) (Info, string, error) {
if language != English {
return nil, "", ErrUnsupportedLanguage
}
Expand All @@ -498,7 +501,11 @@ func (ks keystore) NewMnemonic(uid string, language Language, hdPath string, alg
return nil, "", err
}

info, err := ks.NewAccount(uid, mnemonic, DefaultBIP39Passphrase, hdPath, algo)
if passphrase == "default" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmmm. Why don't we rely on the zero value ""?

Copy link
Collaborator Author

@fedekunze fedekunze Feb 22, 2021

Choose a reason for hiding this comment

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

in case the default passphrase changes

Copy link
Contributor

Choose a reason for hiding this comment

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

so I think we'll need to modify the command line interfaces as well, correct?

passphrase = DefaultBIP39Passphrase
}

info, err := ks.NewAccount(uid, mnemonic, passphrase, hdPath, algo)
if err != nil {
return nil, "", err
}
Expand All @@ -519,6 +526,12 @@ func (ks keystore) NewAccount(uid string, mnemonic string, bip39Passphrase strin

privKey := algo.Generate()(derivedPriv)

address := sdk.AccAddress(privKey.PubKey().Address())
_, err = ks.KeyByAddress(address)
if err == nil {
return nil, fmt.Errorf("account with address %s already exists in keyring, delete the key first if you want to recreate it", address)
}

return ks.writeLocalKey(uid, privKey, algo.Name())
}

Expand Down
Loading