From 413938c5ed9d461f714f0604ed9e99b25f7b21e4 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Wed, 31 Mar 2021 11:25:23 +0200 Subject: [PATCH] Add ledger/multisig detection in SignTx functions (#9026) * Add ledger/multisig detection in SignTx functions * Fix tests * Update CHANGELOG.md * update cl Co-authored-by: Alessio Treglia --- CHANGELOG.md | 2 ++ x/auth/client/cli/cli_test.go | 22 ++++++++++++---------- x/auth/client/cli/tx_sign.go | 11 ++--------- x/auth/client/tx.go | 21 ++++++++++++++++++--- 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39cda9d4cc6f..7de63a77f6b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 @@ -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 diff --git a/x/auth/client/cli/cli_test.go b/x/auth/client/cli/cli_test.go index 0345406e4754..4622ca6a0be2 100644 --- a/x/auth/client/cli/cli_test.go +++ b/x/auth/client/cli/cli_test.go @@ -132,7 +132,7 @@ 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 @@ -140,6 +140,7 @@ func (s *IntegrationTestSuite) TestCLISign() { 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! @@ -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()) @@ -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. @@ -178,7 +179,7 @@ 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) @@ -186,7 +187,7 @@ func (s *IntegrationTestSuite) TestCLISign() { /**** 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()) @@ -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 @@ -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( @@ -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()) @@ -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()) diff --git a/x/auth/client/cli/tx_sign.go b/x/auth/client/cli/tx_sign.go index c5208bb3e1d6..1b887d1a059d 100644 --- a/x/auth/client/cli/tx_sign.go +++ b/x/auth/client/cli/tx_sign.go @@ -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" ) @@ -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) } @@ -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 { @@ -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) } diff --git a/x/auth/client/tx.go b/x/auth/client/tx.go index 2494ba22568f..fc3a32ac6afe 100644 --- a/x/auth/client/tx.go +++ b/x/auth/client/tx.go @@ -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" ) @@ -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 { @@ -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. 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()) {