Skip to content

Add ledger/multisig detection in SignTx functions #9026

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Mar 31, 2021
Merged

Add ledger/multisig detection in SignTx functions #9026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b21597a
Add ledger/multisig detection in SignTx functions
amaury1093 Mar 30, 2021
6371572
Fix tests
amaury1093 Mar 30, 2021
b820f61
Update CHANGELOG.md
amaury1093 Mar 30, 2021
2c0b329
update cl
amaury1093 Mar 30, 2021
51e8df8
Merge branch 'master' into am/9023-gentx
Mar 30, 2021
41499bc
Merge branch 'master' into am/9023-gentx
amaury1093 Mar 31, 2021
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* updated the keyring display structure (it uses protobuf JSON serialization) - the output is more verbose.
* Renamed `MarshalAny` and `UnmarshalAny` to `MarshalInterface` and `UnmarshalInterface` respectively. These functions must take an interface as parameter (not a concrete type nor `Any` object). Underneath they use `Any` wrapping for correct protobuf serialization.
* CLI: removed `--text` flag from `show-node-id` command; the text format for public keys is not used any more - instead we use ProtoJSON.
* [\#9026](https://github.com/cosmos/cosmos-sdk/pull/9026) The `tx sign` and `tx sign-batch` CLI commands use SIGN_MODE_DIRECT by default for local pubkeys. For multisigs and ledger keys, the default LEGACY_AMINO_JSON is kept.

### API Breaking Changes

Expand Down Expand Up @@ -106,6 +107,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/bank) [\#8434](https://github.com/cosmos/cosmos-sdk/pull/8434) Fix legacy REST API `GET /bank/total` and `GET /bank/total/{denom}` in swagger
* (x/slashing) [\#8427](https://github.com/cosmos/cosmos-sdk/pull/8427) Fix query signing infos command
* (server) [\#8399](https://github.com/cosmos/cosmos-sdk/pull/8399) fix gRPC-web flag default value
* [\#9026](https://github.com/cosmos/cosmos-sdk/pull/9026) Fix bug of `gentx` command not working with ledger keys.

## [v0.42.2](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.42.2) - 2021-03-19

Expand Down
22 changes: 12 additions & 10 deletions x/auth/client/cli/cli_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,15 @@ func (s *IntegrationTestSuite) TestCLISignBatch() {
s.Require().Error(err)
}

func (s *IntegrationTestSuite) TestCLISign() {
func (s *IntegrationTestSuite) TestCLISign_AminoJSON() {
require := s.Require()
val1 := s.network.Validators[0]
txCfg := val1.ClientCtx.TxConfig
txBz := s.createBankMsg(val1, val1.Address)
fileUnsigned := testutil.WriteToNewTempFile(s.T(), txBz.String())
chainFlag := fmt.Sprintf("--%s=%s", flags.FlagChainID, val1.ClientCtx.ChainID)
sigOnlyFlag := "--signature-only"
signModeAminoFlag := "--sign-mode=amino-json"

// SIC! validators have same key names and same adddresses as those registered in the keyring,
// BUT the keys are different!
Expand All @@ -154,7 +155,7 @@ func (s *IntegrationTestSuite) TestCLISign() {

/**** test signature-only ****/
res, err := authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag,
sigOnlyFlag)
sigOnlyFlag, signModeAminoFlag)
require.NoError(err)
checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey())
sigs, err := txCfg.UnmarshalSignatureJSON(res.Bytes())
Expand All @@ -163,7 +164,7 @@ func (s *IntegrationTestSuite) TestCLISign() {
require.Equal(account.GetSequence(), sigs[0].Sequence)

/**** test full output ****/
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag)
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, signModeAminoFlag)
require.NoError(err)

// txCfg.UnmarshalSignatureJSON can't unmarshal a fragment of the signature, so we create this structure.
Expand All @@ -178,15 +179,15 @@ func (s *IntegrationTestSuite) TestCLISign() {
/**** test file output ****/
filenameSigned := filepath.Join(s.T().TempDir(), "test_sign_out.json")
fileFlag := fmt.Sprintf("--%s=%s", flags.FlagOutputDocument, filenameSigned)
_, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, fileFlag)
_, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, fileUnsigned.Name(), chainFlag, fileFlag, signModeAminoFlag)
require.NoError(err)
fContent, err := ioutil.ReadFile(filenameSigned)
require.NoError(err)
require.Equal(res.String(), string(fContent))

/**** try to append to the previously signed transaction ****/
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag,
sigOnlyFlag)
sigOnlyFlag, signModeAminoFlag)
require.NoError(err)
checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey(), valInfo.GetPubKey())

Expand All @@ -197,12 +198,12 @@ func (s *IntegrationTestSuite) TestCLISign() {
// provide functionality to check / manage `auth_info`.
// Cases with different keys are are covered in unit tests of `tx.Sign`.
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag,
sigOnlyFlag, "--overwrite")
sigOnlyFlag, "--overwrite", signModeAminoFlag)
checkSignatures(require, txCfg, res.Bytes(), valInfo.GetPubKey())

/**** test flagAmino ****/
res, err = authtest.TxSignExec(val1.ClientCtx, val1.Address, filenameSigned, chainFlag,
"--amino=true")
"--amino=true", signModeAminoFlag)
require.NoError(err)

var txAmino authrest.BroadcastReq
Expand Down Expand Up @@ -1096,7 +1097,8 @@ func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() {
_, _, addr1 := testdata.KeyTestPubAddr()

// Creating a tx with 2 msgs from 2 signers: val0 and val1.
// The validators sign with SIGN_MODE_LEGACY_AMINO_JSON by default.
// The validators need to sign with SIGN_MODE_LEGACY_AMINO_JSON,
// because DIRECT doesn't support multi signers via the CLI.
// Since we we amino, we don't need to pre-populate signer_infos.
txBuilder := val0.ClientCtx.TxConfig.NewTxBuilder()
txBuilder.SetMsgs(
Expand All @@ -1113,7 +1115,7 @@ func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() {
unsignedTxFile := testutil.WriteToNewTempFile(s.T(), string(txJSON))

// Let val0 sign first the file with the unsignedTx.
signedByVal0, err := authtest.TxSignExec(val0.ClientCtx, val0.Address, unsignedTxFile.Name(), "--overwrite")
signedByVal0, err := authtest.TxSignExec(val0.ClientCtx, val0.Address, unsignedTxFile.Name(), "--overwrite", "--sign-mode=amino-json")
require.NoError(err)
signedByVal0File := testutil.WriteToNewTempFile(s.T(), signedByVal0.String())

Expand All @@ -1122,7 +1124,7 @@ func (s *IntegrationTestSuite) TestSignWithMultiSigners_AminoJSON() {
require.NoError(err)
signedTx, err := authtest.TxSignExec(
val1.ClientCtx, val1.Address, signedByVal0File.Name(),
"--offline", fmt.Sprintf("--account-number=%d", val1AccNum), fmt.Sprintf("--sequence=%d", val1Seq),
"--offline", fmt.Sprintf("--account-number=%d", val1AccNum), fmt.Sprintf("--sequence=%d", val1Seq), "--sign-mode=amino-json",
)
require.NoError(err)
signedTxFile := testutil.WriteToNewTempFile(s.T(), signedTx.String())
Expand Down
11 changes: 2 additions & 9 deletions x/auth/client/cli/tx_sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/client/tx"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
authclient "github.com/cosmos/cosmos-sdk/x/auth/client"
"github.com/cosmos/cosmos-sdk/x/auth/client/rest"
)
Expand Down Expand Up @@ -114,9 +113,6 @@ func makeSignBatchCmd() func(cmd *cobra.Command, args []string) error {
return err
}
} else {
if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED {
txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) //nolint:staticcheck
}
err = authclient.SignTxWithSignerAddress(
txFactory, clientCtx, multisigAddr, clientCtx.GetFromName(), txBuilder, clientCtx.Offline, true)
}
Expand Down Expand Up @@ -212,15 +208,12 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {
return err
}
f := cmd.Flags()
txFactory := tx.NewFactoryCLI(clientCtx, f)

clientCtx, txF, newTx, err := readTxAndInitContexts(clientCtx, cmd, args[0])
if err != nil {
return err
}
if txF.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED {
txF = txF.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) //nolint:staticcheck
}

txCfg := clientCtx.TxConfig
txBuilder, err := txCfg.WrapTxBuilder(newTx)
if err != nil {
Expand All @@ -230,7 +223,7 @@ func makeSignCmd() func(cmd *cobra.Command, args []string) error {
printSignatureOnly, _ := cmd.Flags().GetBool(flagSigOnly)
multisigAddrStr, _ := cmd.Flags().GetString(flagMultisig)
from, _ := cmd.Flags().GetString(flags.FlagFrom)
_, fromName, _, err := client.GetFromFields(txFactory.Keybase(), from, clientCtx.GenerateOnly)
_, fromName, _, err := client.GetFromFields(txF.Keybase(), from, clientCtx.GenerateOnly)
if err != nil {
return fmt.Errorf("error getting account from keybase: %w", err)
}
Expand Down
21 changes: 18 additions & 3 deletions x/auth/client/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/client/tx"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
)

Expand All @@ -37,13 +39,20 @@ func PrintUnsignedStdTx(txBldr tx.Factory, clientCtx client.Context, msgs []sdk.
// SignTx signs a transaction managed by the TxBuilder using a `name` key stored in Keybase.
// The new signature is appended to the TxBuilder when overwrite=false or overwritten otherwise.
// Don't perform online validation or lookups if offline is true.
func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx client.TxBuilder, offline, overwriteSig bool) error {
func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, txBuilder client.TxBuilder, offline, overwriteSig bool) error {
info, err := txFactory.Keybase().Key(name)
if err != nil {
return err
}

// Ledger and Multisigs only support LEGACY_AMINO_JSON signing.
if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED &&
(info.GetType() == keyring.TypeLedger || info.GetType() == keyring.TypeMulti) {
txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) //nolint:staticcheck
}

addr := sdk.AccAddress(info.GetPubKey().Address())
if !isTxSigner(addr, stdTx.GetTx().GetSigners()) {
if !isTxSigner(addr, txBuilder.GetTx().GetSigners()) {
return fmt.Errorf("%s: %s", sdkerrors.ErrorInvalidSigner, name)
}
if !offline {
Expand All @@ -53,14 +62,20 @@ func SignTx(txFactory tx.Factory, clientCtx client.Context, name string, stdTx c
}
}

return tx.Sign(txFactory, name, stdTx, overwriteSig)
return tx.Sign(txFactory, name, txBuilder, overwriteSig)
}

// SignTxWithSignerAddress attaches a signature to a transaction.
// Don't perform online validation or lookups if offline is true, else
// populate account and sequence numbers from a foreign account.
// This function should only be used when signing with a multisig. For
// normal keys, please use SignTx directly.
Comment on lines +71 to +72
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is called twice in the sdk, and both are with multisigs. As I understand, there's no use-case for calling this without a multisig, correct?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably right

func SignTxWithSignerAddress(txFactory tx.Factory, clientCtx client.Context, addr sdk.AccAddress,
name string, txBuilder client.TxBuilder, offline, overwrite bool) (err error) {
// Multisigs only support LEGACY_AMINO_JSON signing.
if txFactory.SignMode() == signing.SignMode_SIGN_MODE_UNSPECIFIED {
txFactory = txFactory.WithSignMode(signing.SignMode_SIGN_MODE_LEGACY_AMINO_JSON) //nolint:staticcheck
}

// check whether the address is a signer
if !isTxSigner(addr, txBuilder.GetTx().GetSigners()) {
Expand Down