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

fix: hardcoded ledger algo on keys add #9766

Merged
merged 9 commits into from
Jul 28, 2021
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* [\#9766](https://github.com/cosmos/cosmos-sdk/pull/9766) Fix hardcoded ledger signing algorithm on `keys add` command.
* [\#9720](https://github.com/cosmos/cosmos-sdk/pull/9720) Feegrant grant cli granter now accepts key name as well as address in general and accepts only address in --generate-only mode
* [\#9651](https://github.com/cosmos/cosmos-sdk/pull/9651) Change inconsistent limit of `0` to `MaxUint64` on InfiniteGasMeter and add GasRemaining func to GasMeter.
* [\#9639](https://github.com/cosmos/cosmos-sdk/pull/9639) Check store keys length before accessing them by making sure that `key` is of length `m+1` (for `key[n:m]`)
Expand Down
2 changes: 1 addition & 1 deletion client/keys/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf
// If we're using ledger, only thing we need is the path and the bech32 prefix.
if useLedger {
bech32PrefixAccAddr := sdk.GetConfig().GetBech32AccountAddrPrefix()
info, err := kb.SaveLedgerKey(name, hd.Secp256k1, bech32PrefixAccAddr, coinType, account, index)

info, err := kb.SaveLedgerKey(name, algo, bech32PrefixAccAddr, coinType, account, index)
if err != nil {
return err
}
Expand Down
7 changes: 5 additions & 2 deletions crypto/keyring/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,14 +390,17 @@ func (ks keystore) SignByAddress(address sdk.Address, msg []byte) ([]byte, types

func (ks keystore) SaveLedgerKey(uid string, algo SignatureAlgo, hrp string, coinType, account, index uint32) (Info, error) {
if !ks.options.SupportedAlgosLedger.Contains(algo) {
return nil, ErrUnsupportedSigningAlgo
return nil, fmt.Errorf(
"signature algo %s is not defined in the keyring options: %w",
algo.Name(), ErrUnsupportedSigningAlgo,
)
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
}

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

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

return ks.writeLedgerKey(uid, priv.PubKey(), *hdPath, algo.Name())
Expand Down
4 changes: 3 additions & 1 deletion crypto/keyring/keyring_ledger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,16 @@ func TestAltKeyring_SaveLedgerKey(t *testing.T) {

// Test unsupported Algo
_, err = keyring.SaveLedgerKey("key", notSupportedAlgo{}, "cosmos", 118, 0, 0)
require.EqualError(t, err, ErrUnsupportedSigningAlgo.Error())
require.Error(t, err)
require.Contains(t, err.Error(), ErrUnsupportedSigningAlgo.Error())

ledger, err := keyring.SaveLedgerKey("some_account", hd.Secp256k1, "cosmos", 118, 3, 1)
if err != nil {
require.Equal(t, "ledger nano S: support for ledger devices is not available in this executable", err.Error())
t.Skip("ledger nano S: support for ledger devices is not available in this executable")
return
}

// The mock is available, check that the address is correct
require.Equal(t, "some_account", ledger.GetName())
pubKey := ledger.GetPubKey()
Expand Down
6 changes: 3 additions & 3 deletions crypto/ledger/ledger_secp256k1.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,13 @@ func NewPrivKeySecp256k1Unsafe(path hd.BIP44Params) (types.LedgerPrivKey, error)
func NewPrivKeySecp256k1(path hd.BIP44Params, hrp string) (types.LedgerPrivKey, string, error) {
device, err := getDevice()
if err != nil {
return nil, "", err
return nil, "", fmt.Errorf("failed to retrieve device: %w", err)
}
defer warnIfErrors(device.Close)

pubKey, addr, err := getPubKeyAddrSafe(device, path, hrp)
if err != nil {
return nil, "", err
return nil, "", fmt.Errorf("failed to recover pubkey: %w", err)
}

return PrivKeyLedgerSecp256k1{pubKey, path}, addr, nil
Expand Down Expand Up @@ -261,7 +261,7 @@ func getPubKeyUnsafe(device SECP256K1, path hd.BIP44Params) (types.PubKey, error
func getPubKeyAddrSafe(device SECP256K1, path hd.BIP44Params, hrp string) (types.PubKey, string, error) {
publicKey, addr, err := device.GetAddressPubKeySECP256K1(path.DerivationPath(), hrp)
if err != nil {
return nil, "", fmt.Errorf("address %s rejected", addr)
return nil, "", fmt.Errorf("%w: address rejected for path %s", err, path.String())
}

// re-serialize in the 33-byte compressed format
Expand Down