Skip to content

Commit

Permalink
crypto/keys: remove DecodeSignature from baseKeybase (#5844)
Browse files Browse the repository at this point in the history
Keybase implementations should return errors when
signature generation is attempted with offline/multisig
keys since theyr lack private keys.

This is a change in Keyring/Keybase.Sign() methods
semantics that have been broken for long time.
  • Loading branch information
Alessio Treglia authored Mar 20, 2020
1 parent 2828f1c commit 75a375b
Show file tree
Hide file tree
Showing 6 changed files with 9 additions and 37 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ are now replaced by human-readable expressions. This change can potentially brea
that parse log messages.
* (client) [\#5799](https://github.com/cosmos/cosmos-sdk/pull/5799) The `tx encode/decode` commands, due to change on encoding break compatibility with
older clients.
* (x/auth) [\#5844](https://github.com/cosmos/cosmos-sdk/pull/5844) `tx sign` command now returns an error when signing is attempted with offline/multisig keys.

### API Breaking Changes

Expand Down Expand Up @@ -87,6 +88,8 @@ resulted in a panic when the tx execution mode was `CheckTx`.
* (modules) [\#5569](https://github.com/cosmos/cosmos-sdk/issues/5569) `InitGenesis`, for the relevant modules, now ensures module accounts exist.
* (crypto/keys/mintkey) [\#5823](https://github.com/cosmos/cosmos-sdk/pull/5823) fix errors handling in UnarmorPubKeyBytes (underlying armoring function's
return error was not being checked).
* (crypto/keys) [\#5844](https://github.com/cosmos/cosmos-sdk/pull/5844) Keybase/Keyring `Sign()` methods no longer decode amino signatures
when method receivers are offline/multisig keys.

### State Machine Breaking

Expand Down
2 changes: 1 addition & 1 deletion crypto/keys/keybase.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ func (kb dbKeybase) Sign(name, passphrase string, msg []byte) (sig []byte, pub t
return SignWithLedger(info, msg)

case offlineInfo, multiInfo:
return kb.base.DecodeSignature(info, msg)
return nil, info.GetPubKey(), errors.New("cannot sign with offline keys")
}

sig, err = priv.Sign(msg)
Expand Down
31 changes: 0 additions & 31 deletions crypto/keys/keybase_base.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
package keys

import (
"bufio"
"fmt"
"os"

"github.com/cosmos/go-bip39"
"github.com/pkg/errors"
tmcrypto "github.com/tendermint/tendermint/crypto"
Expand Down Expand Up @@ -104,33 +100,6 @@ func SecpPrivKeyGen(bz []byte) tmcrypto.PrivKey {
return secp256k1.PrivKeySecp256k1(bzArr)
}

// DecodeSignature decodes a an length-prefixed binary signature from standard input
// and return it as a byte slice.
func (kb baseKeybase) DecodeSignature(info Info, msg []byte) (sig []byte, pub tmcrypto.PubKey, err error) {
_, err = fmt.Fprintf(os.Stderr, "Message to sign:\n\n%s\n", msg)
if err != nil {
return nil, nil, err
}

buf := bufio.NewReader(os.Stdin)
_, err = fmt.Fprintf(os.Stderr, "\nEnter Amino-encoded signature:\n")
if err != nil {
return nil, nil, err
}

// will block until user inputs the signature
signed, err := buf.ReadString('\n')
if err != nil {
return nil, nil, err
}

if err := CryptoCdc.UnmarshalBinaryLengthPrefixed([]byte(signed), sig); err != nil {
return nil, nil, errors.Wrap(err, "failed to decode signature")
}

return sig, info.GetPubKey(), nil
}

// CreateAccount creates an account Info object.
func (kb baseKeybase) CreateAccount(
keyWriter keyWriter, name, mnemonic, bip39Passphrase, encryptPasswd, hdPath string, algo SigningAlgo,
Expand Down
5 changes: 2 additions & 3 deletions crypto/keys/keybase_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
package keys

import (
"errors"
"fmt"
"io"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -280,7 +278,8 @@ func TestSignVerify(t *testing.T) {

// Now try to sign data with a secret-less key
_, _, err = cstore.Sign(n3, p3, d3)
require.True(t, errors.Is(io.EOF, err))
require.Error(t, err)
require.Equal(t, "cannot sign with offline keys", err.Error())
}

func assertPassword(t *testing.T, cstore Keybase, name, pass, badpass string) {
Expand Down
2 changes: 1 addition & 1 deletion crypto/keys/keyring.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func (kb keyringKeybase) Sign(name, passphrase string, msg []byte) (sig []byte,
return SignWithLedger(info, msg)

case offlineInfo, multiInfo:
return kb.base.DecodeSignature(info, msg)
return nil, info.GetPubKey(), errors.New("cannot sign with offline keys")
}

sig, err = priv.Sign(msg)
Expand Down
3 changes: 2 additions & 1 deletion crypto/keys/keyring_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,8 @@ func TestLazySignVerifyKeyRing(t *testing.T) {

// Now try to sign data with a secret-less key
_, _, err = kb.Sign(n3, p3, d3)
require.NotNil(t, err)
require.Error(t, err)
require.Equal(t, "cannot sign with offline keys", err.Error())
}

func TestLazyExportImportKeyRing(t *testing.T) {
Expand Down

0 comments on commit 75a375b

Please sign in to comment.