Skip to content

Commit

Permalink
Remove DecodeSignature from baseKeybase
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 committed Mar 20, 2020
1 parent 2828f1c commit ae3a14c
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 ae3a14c

Please sign in to comment.