Skip to content

Commit

Permalink
refactor: keyring errors (cosmos#14974)
Browse files Browse the repository at this point in the history
  • Loading branch information
JulianToledano committed Feb 9, 2023
1 parent dc20731 commit a0aef94
Show file tree
Hide file tree
Showing 9 changed files with 87 additions and 59 deletions.
29 changes: 27 additions & 2 deletions crypto/keyring/errors.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,38 @@
package keyring

import "github.com/pkg/errors"
import "github.com/cockroachdb/errors"

var (
// ErrUnsupportedSigningAlgo is raised when the caller tries to use a
// different signing scheme than secp256k1.
ErrUnsupportedSigningAlgo = errors.New("unsupported signing algo")

// ErrUnsupportedLanguage is raised when the caller tries to use a
// different language than english for creating a mnemonic sentence.
ErrUnsupportedLanguage = errors.New("unsupported language: only english is supported")
// ErrUnknownBacked is raised when the keyring backend is unknown
ErrUnknownBacked = errors.New("unknown keyring backend")
// ErrOverwriteKey is raised when a key cannot be overwritten
ErrOverwriteKey = errors.New("cannot overwrite key")
// ErrKeyAlreadyExists is raised when creating a key that already exists
ErrKeyAlreadyExists = errors.Newf("key already exists")
// ErrInvalidSignMode is raised when trying to sign with an invaled method
ErrInvalidSignMode = errors.New("invalid sign mode, expected LEGACY_AMINO_JSON or TEXTUAL")
// ErrMaxPassPhraseAttempts is raised when the maxPassphraseEntryAttempts is reached
ErrMaxPassPhraseAttempts = errors.New("too many failed passphrase attempts")
// ErrUnableToSerialize is raised when codec fails to serialize
ErrUnableToSerialize = errors.New("unable to serialize record")
// ErrOfflineSign is raised when trying to sign offline record.
ErrOfflineSign = errors.New("cannot sign with offline keys")
// ErrDuplicatedAddress is raised when creating a key with the same address as a key that already exists.
ErrDuplicatedAddress = errors.New("duplicated address created")
// ErrLedgerGenerateKey is raised when a ledger can't generate a key
ErrLedgerGenerateKey = errors.New("failed to generate ledger key")
// ErrNotLedgerObj is raised when record.GetLedger() returns nil.
ErrNotLedgerObj = errors.New("not a ledger object")
// ErrLedgerInvalidSignature is raised when ledger generates an invalid signature.
ErrLedgerInvalidSignature = errors.New("Ledger generated an invalid signature. Perhaps you have multiple ledgers and need to try another one")
// ErrLegacyToRecord is raised when cannot be converted to a Record
ErrLegacyToRecord = errors.New("unable to convert LegacyInfo to Record")
// ErrUnknownLegacyType is raised when a LegacyInfo type is unknown.
ErrUnknownLegacyType = errors.New("unknown LegacyInfo type")
)
64 changes: 30 additions & 34 deletions crypto/keyring/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import (
"strings"

"github.com/99designs/keyring"
"github.com/cockroachdb/errors"
cmtcrypto "github.com/cometbft/cometbft/crypto"
"github.com/pkg/errors"

"github.com/cosmos/cosmos-sdk/client/input"
"github.com/cosmos/cosmos-sdk/codec"
Expand Down Expand Up @@ -70,7 +70,7 @@ type Keyring interface {
DeleteByAddress(address sdk.Address) error

// Rename an existing key from the Keyring
Rename(from string, to string) error
Rename(from, to string) error

// NewMnemonic generates a new mnemonic, derives a hierarchical deterministic key from it, and
// persists the key to storage. Returns the generated mnemonic and the key Info.
Expand Down Expand Up @@ -116,7 +116,7 @@ type Importer interface {
ImportPrivKey(uid, armor, passphrase string) error

// ImportPubKey imports ASCII armored public keys.
ImportPubKey(uid string, armor string) error
ImportPubKey(uid, armor string) error
}

// Migrator is implemented by key stores and enables migration of keys from amino to proto
Expand Down Expand Up @@ -194,7 +194,7 @@ func New(
case BackendPass:
db, err = keyring.Open(newPassBackendKeyringConfig(appName, rootDir, userInput))
default:
return nil, fmt.Errorf("unknown keyring backend %v", backend)
return nil, errors.Wrap(ErrUnknownBacked, backend)
}

if err != nil {
Expand Down Expand Up @@ -317,7 +317,7 @@ func (ks keystore) ExportPrivKeyArmorByAddress(address sdk.Address, encryptPassp
func (ks keystore) ImportPrivKey(uid, armor, passphrase string) error {
if k, err := ks.Key(uid); err == nil {
if uid == k.Name {
return fmt.Errorf("cannot overwrite key: %s", uid)
return errors.Wrap(ErrOverwriteKey, uid)
}
}

Expand All @@ -334,9 +334,9 @@ func (ks keystore) ImportPrivKey(uid, armor, passphrase string) error {
return nil
}

func (ks keystore) ImportPubKey(uid string, armor string) error {
func (ks keystore) ImportPubKey(uid, armor string) error {
if _, err := ks.Key(uid); err == nil {
return fmt.Errorf("cannot overwrite key: %s", uid)
return errors.Wrap(ErrOverwriteKey, uid)
}

pubBytes, _, err := crypto.UnarmorPubKeyBytes(armor)
Expand Down Expand Up @@ -386,8 +386,7 @@ func (ks keystore) Sign(uid string, msg []byte, signMode signing.SignMode) ([]by
if err != nil {
return nil, nil, err
}

return nil, pub, errors.New("cannot sign with offline keys")
return nil, pub, ErrOfflineSign
}
}

Expand All @@ -402,17 +401,14 @@ func (ks keystore) SignByAddress(address sdk.Address, msg []byte, signMode signi

func (ks keystore) SaveLedgerKey(uid string, algo SignatureAlgo, hrp string, coinType, account, index uint32) (*Record, error) {
if !ks.options.SupportedAlgosLedger.Contains(algo) {
return nil, fmt.Errorf(
"%w: signature algo %s is not defined in the keyring options",
ErrUnsupportedSigningAlgo, algo.Name(),
)
return nil, errors.Wrap(ErrUnsupportedSigningAlgo, fmt.Sprintf("signature algo %s is not defined in the keyring options", algo.Name()))
}

hdPath := hd.NewFundraiserParams(account, coinType, index)

priv, _, err := ledger.NewPrivKeySecp256k1(*hdPath, hrp)
if err != nil {
return nil, fmt.Errorf("failed to generate ledger key: %w", err)
return nil, errors.CombineErrors(ErrLedgerGenerateKey, err)
}

return ks.writeLedgerKey(uid, priv.PubKey(), hdPath)
Expand Down Expand Up @@ -452,7 +448,7 @@ func (ks keystore) DeleteByAddress(address sdk.Address) error {
func (ks keystore) Rename(oldName, newName string) error {
_, err := ks.Key(newName)
if err == nil {
return fmt.Errorf("rename failed: %s already exists in the keyring", newName)
return errors.Wrap(ErrKeyAlreadyExists, fmt.Sprintf("rename failed, %s", newName))
}

armor, err := ks.ExportPrivKeyArmor(oldName, passPhrase)
Expand Down Expand Up @@ -512,7 +508,7 @@ func (ks keystore) KeyByAddress(address sdk.Address) (*Record, error) {

func wrapKeyNotFound(err error, msg string) error {
if err == keyring.ErrKeyNotFound {
return sdkerrors.Wrap(sdkerrors.ErrKeyNotFound, msg)
return errors.Wrap(sdkerrors.ErrKeyNotFound, msg)
}
return err
}
Expand Down Expand Up @@ -554,7 +550,7 @@ func (ks keystore) NewMnemonic(uid string, language Language, hdPath, bip39Passp
return k, mnemonic, nil
}

func (ks keystore) NewAccount(name string, mnemonic string, bip39Passphrase string, hdPath string, algo SignatureAlgo) (*Record, error) {
func (ks keystore) NewAccount(name, mnemonic, bip39Passphrase, hdPath string, algo SignatureAlgo) (*Record, error) {
if !ks.isSupportedSigningAlgo(algo) {
return nil, ErrUnsupportedSigningAlgo
}
Expand All @@ -567,11 +563,11 @@ func (ks keystore) NewAccount(name string, mnemonic string, bip39Passphrase stri

privKey := algo.Generate()(derivedPriv)

// check if the a key already exists with the same address and return an error
// check if the key already exists with the same address and return an error
// if found
address := sdk.AccAddress(privKey.PubKey().Address())
if _, err := ks.KeyByAddress(address); err == nil {
return nil, errors.New("duplicated address created")
return nil, ErrDuplicatedAddress
}

return ks.writeLocalKey(name, privKey)
Expand Down Expand Up @@ -602,7 +598,7 @@ func (ks keystore) SupportedAlgorithms() (SigningAlgoList, SigningAlgoList) {
func SignWithLedger(k *Record, msg []byte, signMode signing.SignMode) (sig []byte, pub types.PubKey, err error) {
ledgerInfo := k.GetLedger()
if ledgerInfo == nil {
return nil, nil, errors.New("not a ledger object")
return nil, nil, ErrNotLedgerObj
}

path := ledgerInfo.GetPath()
Expand All @@ -624,11 +620,11 @@ func SignWithLedger(k *Record, msg []byte, signMode signing.SignMode) (sig []byt
return nil, nil, err
}
default:
return nil, nil, fmt.Errorf("got invalid sign mode %d, expected LEGACY_AMINO_JSON or TEXTUAL", signMode)
return nil, nil, errors.Wrap(ErrInvalidSignMode, fmt.Sprintf("%v", signMode))
}

if !priv.PubKey().VerifySignature(msg, sig) {
return nil, nil, errors.New("Ledger generated an invalid signature. Perhaps you have multiple ledgers and need to try another one")
return nil, nil, ErrLedgerInvalidSignature
}

return sig, priv.PubKey(), nil
Expand Down Expand Up @@ -697,7 +693,7 @@ func newRealPrompt(dir string, buf io.Reader) func(string) (string, error) {
case err == nil:
keyhash, err = os.ReadFile(keyhashFilePath)
if err != nil {
return "", fmt.Errorf("failed to read %s: %v", keyhashFilePath, err)
return "", errors.Wrap(err, fmt.Sprintf("failed to read %s", keyhashFilePath))
}

keyhashStored = true
Expand All @@ -706,15 +702,15 @@ func newRealPrompt(dir string, buf io.Reader) func(string) (string, error) {
keyhashStored = false

default:
return "", fmt.Errorf("failed to open %s: %v", keyhashFilePath, err)
return "", errors.Wrap(err, fmt.Sprintf("failed to open %s", keyhashFilePath))
}

failureCounter := 0

for {
failureCounter++
if failureCounter > maxPassphraseEntryAttempts {
return "", fmt.Errorf("too many failed passphrase attempts")
return "", ErrMaxPassPhraseAttempts
}

buf := bufio.NewReader(buf)
Expand Down Expand Up @@ -795,12 +791,12 @@ func (ks keystore) writeRecord(k *Record) error {
return err
}
if exists {
return fmt.Errorf("public key %s already exists in keybase", key)
return errors.Wrap(ErrKeyAlreadyExists, key)
}

serializedRecord, err := ks.cdc.Marshal(k)
if err != nil {
return fmt.Errorf("unable to serialize record; %+w", err)
return errors.CombineErrors(ErrUnableToSerialize, err)
}

item := keyring.Item{
Expand Down Expand Up @@ -927,7 +923,7 @@ func (ks keystore) migrate(key string) (*Record, error) {
}

if len(item.Data) == 0 {
return nil, sdkerrors.Wrap(sdkerrors.ErrKeyNotFound, key)
return nil, errors.Wrap(sdkerrors.ErrKeyNotFound, key)
}

// 2. Try to deserialize using proto
Expand All @@ -940,18 +936,18 @@ func (ks keystore) migrate(key string) (*Record, error) {
// 4. Try to decode with amino
legacyInfo, err := unMarshalLegacyInfo(item.Data)
if err != nil {
return nil, fmt.Errorf("unable to unmarshal item.Data, err: %w", err)
return nil, errors.Wrap(err, "unable to unmarshal item.Data")
}

// 5. Convert and serialize info using proto
k, err = ks.convertFromLegacyInfo(legacyInfo)
if err != nil {
return nil, fmt.Errorf("convertFromLegacyInfo, err: %w", err)
return nil, errors.Wrap(err, "convertFromLegacyInfo")
}

serializedRecord, err := ks.cdc.Marshal(k)
if err != nil {
return nil, fmt.Errorf("unable to serialize record, err: %w", err)
return nil, errors.CombineErrors(ErrUnableToSerialize, err)
}

item = keyring.Item{
Expand All @@ -961,7 +957,7 @@ func (ks keystore) migrate(key string) (*Record, error) {

// 6. Overwrite the keyring entry with the new proto-encoded key.
if err := ks.SetItem(item); err != nil {
return nil, fmt.Errorf("unable to set keyring.Item, err: %w", err)
return nil, errors.Wrap(err, "unable to set keyring.Item")
}

fmt.Printf("Successfully migrated key %s.\n", key)
Expand All @@ -984,7 +980,7 @@ func (ks keystore) SetItem(item keyring.Item) error {

func (ks keystore) convertFromLegacyInfo(info LegacyInfo) (*Record, error) {
if info == nil {
return nil, errors.New("unable to convert LegacyInfo to Record cause info is nil")
return nil, errors.Wrap(ErrLegacyToRecord, "info is nil")
}

name := info.GetName()
Expand All @@ -1010,7 +1006,7 @@ func (ks keystore) convertFromLegacyInfo(info LegacyInfo) (*Record, error) {

return NewLedgerRecord(name, pk, path)
default:
return nil, errors.New("unknown LegacyInfo type")
return nil, ErrUnknownLegacyType

}
}
Expand Down
5 changes: 2 additions & 3 deletions crypto/keyring/keyring_ledger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ package keyring

import (
"bytes"
"strings"
"testing"

"github.com/cockroachdb/errors"
"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/crypto/hd"
Expand Down Expand Up @@ -101,8 +101,7 @@ func TestAltKeyring_SaveLedgerKey(t *testing.T) {

// Test unsupported Algo
_, err = kr.SaveLedgerKey("key", notSupportedAlgo{}, "cosmos", 118, 0, 0)
require.Error(t, err)
require.True(t, strings.Contains(err.Error(), ErrUnsupportedSigningAlgo.Error()))
require.True(t, errors.Is(err, ErrUnsupportedSigningAlgo))

k, err := kr.SaveLedgerKey("some_account", hd.Secp256k1, "cosmos", 118, 3, 1)
if err != nil {
Expand Down
17 changes: 9 additions & 8 deletions crypto/keyring/keyring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keyring

import (
"encoding/hex"
"errors"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -52,7 +53,7 @@ func TestNewKeyring(t *testing.T) {
nilKr, err := New("cosmos", "fuzzy", dir, mockIn, cdc)
require.Error(t, err)
require.Nil(t, nilKr)
require.Equal(t, "unknown keyring backend fuzzy", err.Error())
require.True(t, errors.Is(err, ErrUnknownBacked))

mockIn.Reset("password\npassword\n")
k, _, err := kr.NewMnemonic("foo", English, sdk.FullFundraiserPath, DefaultBIP39Passphrase, hd.Secp256k1)
Expand Down Expand Up @@ -442,7 +443,7 @@ func TestKeyringKeybaseExportImportPrivKey(t *testing.T) {

// overwrite is not allowed
err = kb.ImportPrivKey("john2", keystr, "password")
require.Equal(t, "cannot overwrite key: john2", err.Error())
require.True(t, errors.Is(err, ErrOverwriteKey))

// try export non existing key
_, err = kb.ExportPrivKeyArmor("john3", "wrongpassword")
Expand Down Expand Up @@ -1254,7 +1255,7 @@ func TestAltKeyring_ImportExportPrivKey(t *testing.T) {

// Should fail importing private key on existing key.
err = kr.ImportPrivKey(newUID, armor, passphrase)
require.EqualError(t, err, fmt.Sprintf("cannot overwrite key: %s", newUID))
require.True(t, errors.Is(err, ErrOverwriteKey))
}

func TestAltKeyring_ImportExportPrivKey_ByAddress(t *testing.T) {
Expand Down Expand Up @@ -1284,7 +1285,7 @@ func TestAltKeyring_ImportExportPrivKey_ByAddress(t *testing.T) {

// Should fail importing private key on existing key.
err = kr.ImportPrivKey(newUID, armor, passphrase)
require.EqualError(t, err, fmt.Sprintf("cannot overwrite key: %s", newUID))
require.True(t, errors.Is(err, ErrOverwriteKey))
}

func TestAltKeyring_ImportExportPubKey(t *testing.T) {
Expand All @@ -1307,7 +1308,7 @@ func TestAltKeyring_ImportExportPubKey(t *testing.T) {

// Should fail importing private key on existing key.
err = kr.ImportPubKey(newUID, armor)
require.EqualError(t, err, fmt.Sprintf("cannot overwrite key: %s", newUID))
require.True(t, errors.Is(err, ErrOverwriteKey))
}

func TestAltKeyring_ImportExportPubKey_ByAddress(t *testing.T) {
Expand All @@ -1332,7 +1333,7 @@ func TestAltKeyring_ImportExportPubKey_ByAddress(t *testing.T) {

// Should fail importing private key on existing key.
err = kr.ImportPubKey(newUID, armor)
require.EqualError(t, err, fmt.Sprintf("cannot overwrite key: %s", newUID))
require.True(t, errors.Is(err, ErrOverwriteKey))
}

func TestAltKeyring_UnsafeExportPrivKeyHex(t *testing.T) {
Expand Down Expand Up @@ -1426,7 +1427,7 @@ func TestRenameKey(t *testing.T) {
newKeyRecord(t, kr, key1)
newKeyRecord(t, kr, key2)
err := kr.Rename(key2, key1)
require.Equal(t, fmt.Errorf("rename failed: %s already exists in the keyring", key1), err)
require.True(t, errors.Is(err, ErrKeyAlreadyExists))
assertKeysExist(t, kr, key1, key2) // keys should still exist after failed rename
},
},
Expand All @@ -1436,7 +1437,7 @@ func TestRenameKey(t *testing.T) {
keyName := "keyName"
newKeyRecord(t, kr, keyName)
err := kr.Rename(keyName, keyName)
require.Equal(t, fmt.Errorf("rename failed: %s already exists in the keyring", keyName), err)
require.True(t, errors.Is(err, ErrKeyAlreadyExists))
assertKeysExist(t, kr, keyName)
},
},
Expand Down
Loading

0 comments on commit a0aef94

Please sign in to comment.