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: keyring errors #14974

Merged
merged 7 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
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
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")
Copy link
Member

Choose a reason for hiding this comment

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

Curious why extracting very specific errors like that. Will they be reused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this errors are returned at some point. I decided to make them public so other package can check against them and do what is convenient

)
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