From 3e3c114af562063b822084d2e4cbc4c19c8414e3 Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 5 May 2022 09:52:01 +0200 Subject: [PATCH] chore: x/auth audit changes (#11860) ## Description ref: #11362 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/main/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- proto/cosmos/auth/v1beta1/query.proto | 32 ++++++++++++++++----- x/auth/keeper/grpc_query.go | 5 ++++ x/auth/keeper/keeper.go | 4 +-- x/auth/signing/sign_mode_handler.go | 2 +- x/auth/spec/03_antehandlers.md | 40 --------------------------- x/auth/spec/03_middlewares.md | 38 +++++++++++++++++++++++++ 6 files changed, 70 insertions(+), 51 deletions(-) delete mode 100644 x/auth/spec/03_antehandlers.md create mode 100644 x/auth/spec/03_middlewares.md diff --git a/proto/cosmos/auth/v1beta1/query.proto b/proto/cosmos/auth/v1beta1/query.proto index 7798da00233a..aa51cfb0123b 100644 --- a/proto/cosmos/auth/v1beta1/query.proto +++ b/proto/cosmos/auth/v1beta1/query.proto @@ -34,17 +34,23 @@ service Query { option (google.api.http).get = "/cosmos/auth/v1beta1/module_accounts"; } - // Bech32 queries bech32Prefix + // Bech32Prefix queries bech32Prefix + // + // Since: cosmos-sdk 0.46 rpc Bech32Prefix(Bech32PrefixRequest) returns (Bech32PrefixResponse) { option (google.api.http).get = "/cosmos/auth/v1beta1/bech32"; } // AddressBytesToString converts Account Address bytes to string + // + // Since: cosmos-sdk 0.46 rpc AddressBytesToString(AddressBytesToStringRequest) returns (AddressBytesToStringResponse) { option (google.api.http).get = "/cosmos/auth/v1beta1/bech32/{address_bytes}"; } // AddressStringToBytes converts Address string to bytes + // + // Since: cosmos-sdk 0.46 rpc AddressStringToBytes(AddressStringToBytesRequest) returns (AddressStringToBytesResponse) { option (google.api.http).get = "/cosmos/auth/v1beta1/bech32/{address_string}"; } @@ -101,30 +107,42 @@ message QueryModuleAccountsResponse { repeated google.protobuf.Any accounts = 1 [(cosmos_proto.accepts_interface) = "ModuleAccountI"]; } -// Bech32PrefixRequest is the request type for Bech32Prefix rpc method +// Bech32PrefixRequest is the request type for Bech32Prefix rpc method. +// +// Since: cosmos-sdk 0.46 message Bech32PrefixRequest {} -// Bech32PrefixResponse is the response type for Bech32Prefix rpc method +// Bech32PrefixResponse is the response type for Bech32Prefix rpc method. +// +// Since: cosmos-sdk 0.46 message Bech32PrefixResponse { string bech32_prefix = 1; } -// AddressBytesToStringRequest is the request type for AddressString rpc method +// AddressBytesToStringRequest is the request type for AddressString rpc method. +// +// Since: cosmos-sdk 0.46 message AddressBytesToStringRequest { bytes address_bytes = 1; } -// AddressBytesToStringResponse is the response type for AddressString rpc method +// AddressBytesToStringResponse is the response type for AddressString rpc method. +// +// Since: cosmos-sdk 0.46 message AddressBytesToStringResponse { string address_string = 1; } -// AddressStringToBytesRequest is the request type for AccountBytes rpc method +// AddressStringToBytesRequest is the request type for AccountBytes rpc method. +// +// Since: cosmos-sdk 0.46 message AddressStringToBytesRequest { string address_string = 1; } -// AddressStringToBytesResponse is the response type for AddressBytes rpc method +// AddressStringToBytesResponse is the response type for AddressBytes rpc method. +// +// Since: cosmos-sdk 0.46 message AddressStringToBytesResponse { bytes address_bytes = 1; } diff --git a/x/auth/keeper/grpc_query.go b/x/auth/keeper/grpc_query.go index 30187644dba2..83fbb07c6be8 100644 --- a/x/auth/keeper/grpc_query.go +++ b/x/auth/keeper/grpc_query.go @@ -119,6 +119,7 @@ func (ak AccountKeeper) ModuleAccounts(c context.Context, req *types.QueryModule return &types.QueryModuleAccountsResponse{Accounts: modAccounts}, nil } +// Bech32Prefix returns the keeper internally stored bech32 prefix. func (ak AccountKeeper) Bech32Prefix(ctx context.Context, req *types.Bech32PrefixRequest) (*types.Bech32PrefixResponse, error) { bech32Prefix, err := ak.getBech32Prefix() if err != nil { @@ -128,6 +129,8 @@ func (ak AccountKeeper) Bech32Prefix(ctx context.Context, req *types.Bech32Prefi return &types.Bech32PrefixResponse{Bech32Prefix: bech32Prefix}, nil } +// AddressBytesToString converts an address from bytes to string, using the +// keeper's bech32 prefix. func (ak AccountKeeper) AddressBytesToString(ctx context.Context, req *types.AddressBytesToStringRequest) (*types.AddressBytesToStringResponse, error) { if req == nil { return nil, status.Error(codes.InvalidArgument, "empty request") @@ -145,6 +148,8 @@ func (ak AccountKeeper) AddressBytesToString(ctx context.Context, req *types.Add return &types.AddressBytesToStringResponse{AddressString: text}, nil } +// AddressStringToBytes converts an address from string to bytes, using the +// keeper's bech32 prefix. func (ak AccountKeeper) AddressStringToBytes(ctx context.Context, req *types.AddressStringToBytesRequest) (*types.AddressStringToBytesResponse, error) { if req == nil { return nil, status.Error(codes.InvalidArgument, "empty request") diff --git a/x/auth/keeper/keeper.go b/x/auth/keeper/keeper.go index 25592115a3e9..d83e30ee00c4 100644 --- a/x/auth/keeper/keeper.go +++ b/x/auth/keeper/keeper.go @@ -1,7 +1,6 @@ package keeper import ( - "errors" "fmt" gogotypes "github.com/gogo/protobuf/types" @@ -11,7 +10,6 @@ import ( cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/cosmos-sdk/types/address" - storetypes "github.com/cosmos/cosmos-sdk/store/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/auth/types" @@ -249,7 +247,7 @@ func (ak AccountKeeper) GetCodec() codec.BinaryCodec { return ak.cdc } func (ak AccountKeeper) getBech32Prefix() (string, error) { bech32Codec, ok := ak.addressCdc.(bech32Codec) if !ok { - return "", errors.New("unable cast addressCdc to bech32Codec") + return "", fmt.Errorf("unable cast addressCdc to bech32Codec; expected %T got %T", bech32Codec, ak.addressCdc) } return bech32Codec.bech32Prefix, nil diff --git a/x/auth/signing/sign_mode_handler.go b/x/auth/signing/sign_mode_handler.go index 57d201dbdedd..d4faff3d05cf 100644 --- a/x/auth/signing/sign_mode_handler.go +++ b/x/auth/signing/sign_mode_handler.go @@ -42,7 +42,7 @@ type SignerData struct { // since in SIGN_MODE_DIRECT the account sequence is already in the signer // info. // - // In case of multisigs, this should be the multisig account number. + // In case of multisigs, this should be the multisig sequence. Sequence uint64 // PubKey is the public key of the signer. diff --git a/x/auth/spec/03_antehandlers.md b/x/auth/spec/03_antehandlers.md deleted file mode 100644 index b695d8597dc5..000000000000 --- a/x/auth/spec/03_antehandlers.md +++ /dev/null @@ -1,40 +0,0 @@ - - -# AnteHandlers - -The `x/auth` module presently has no transaction handlers of its own, but does expose the special `AnteHandler`, used for performing basic validity checks on a transaction, such that it could be thrown out of the mempool. -The `AnteHandler` can be seen as a set of decorators that check transactions within the current context, per [ADR 010](https://github.com/cosmos/cosmos-sdk/blob/v0.43.0-alpha1/docs/architecture/adr-010-modular-antehandler.md). - -Note that the `AnteHandler` is called on both `CheckTx` and `DeliverTx`, as Tendermint proposers presently have the ability to include in their proposed block transactions which fail `CheckTx`. - -## Decorators - -The auth module provides `AnteDecorator`s that are recursively chained together into a single `AnteHandler` in the following order: - -* `SetUpContextDecorator`: Sets the `GasMeter` in the `Context` and wraps the next `AnteHandler` with a defer clause to recover from any downstream `OutOfGas` panics in the `AnteHandler` chain to return an error with information on gas provided and gas used. - -* `RejectExtensionOptionsDecorator`: Rejects all extension options which can optionally be included in protobuf transactions. - -* `MempoolFeeDecorator`: Checks if the `tx` fee is above local mempool `minFee` parameter during `CheckTx`. - -* `ValidateBasicDecorator`: Calls `tx.ValidateBasic` and returns any non-nil error. - -* `TxTimeoutHeightDecorator`: Check for a `tx` height timeout. - -* `ValidateMemoDecorator`: Validates `tx` memo with application parameters and returns any non-nil error. - -* `ConsumeGasTxSizeDecorator`: Consumes gas proportional to the `tx` size based on application parameters. - -* `DeductFeeDecorator`: Deducts the `FeeAmount` from first signer of the `tx`. If the `x/feegrant` module is enabled and a fee granter is set, it deducts fees from the fee granter account. - -* `SetPubKeyDecorator`: Sets the pubkey from a `tx`'s signers that does not already have its corresponding pubkey saved in the state machine and in the current context. - -* `ValidateSigCountDecorator`: Validates the number of signatures in `tx` based on app-parameters. - -* `SigGasConsumeDecorator`: Consumes parameter-defined amount of gas for each signature. This requires pubkeys to be set in context for all signers as part of `SetPubKeyDecorator`. - -* `SigVerificationDecorator`: Verifies all signatures are valid. This requires pubkeys to be set in context for all signers as part of `SetPubKeyDecorator`. - -* `IncrementSequenceDecorator`: Increments the account sequence for each signer to prevent replay attacks. diff --git a/x/auth/spec/03_middlewares.md b/x/auth/spec/03_middlewares.md new file mode 100644 index 000000000000..37bf6613df72 --- /dev/null +++ b/x/auth/spec/03_middlewares.md @@ -0,0 +1,38 @@ + + +# Middlewares + +The `x/auth` module presently has no transaction handlers of its own, but does expose middlewares directly called from BaseApp's `CheckTx` and `DeliverTx`, which can be used for performing any operations on transactions, such as basic validity checks on a transaction such that it could be thrown out of the mempool, or routing the transactions to their `Msg` service to perform state transitions. +The middlewares can be seen as a set of decorators wrapped one on top of the other, that check transactions within the current context, per [ADR-045](https://github.com/cosmos/cosmos-sdk/blob/v0.46.0-beta2/docs/architecture/adr-045-check-delivertx-middlewares.md). + +Note that the middlewares are called on both `CheckTx` and `DeliverTx`, as Tendermint proposers presently have the ability to include in their proposed block transactions which fail `CheckTx`. + +## List of Middleware + +The auth module provides: + +- one `tx.Handler`, called `RunMsgsTxHandler`, which routes each `sdk.Msg` from a transaction to the correct module `Msg` service, and runs each `sdk.Msg` to perform state transitions, +- a set of middlewares that are recursively chained together around the base `tx.Handler` in the following order (the first middleware's `pre`-hook is run first, and `post`-hook is run last): + + - `NewTxDecoderMiddleware`: Decodes the transaction bytes from ABCI `CheckTx` and `DeliverTx` into the SDK transaction type. This middleware is generally called first, as most middlewares logic rely on a decoded SDK transaction. + - `GasTxMiddleware`: Sets the `GasMeter` in the `Context`. + - `RecoveryTxMiddleware`: Wraps the next middleware with a defer clause to recover from any downstream panics in the middleware chain to return an error with information on gas provided and gas used. + - `RejectExtensionOptionsMiddleware`: Rejects all extension options which can optionally be included in protobuf transactions. + - `IndexEventsTxMiddleware`: Choose which events to index in Tendermint. Make sure no events are emitted outside of this middleware. + - `ValidateBasicMiddleware`: Calls `tx.ValidateBasic` and returns any non-nil error. + - `TxTimeoutHeightMiddleware`: Check for a `tx` height timeout. + - `ValidateMemoMiddleware`: Validates `tx` memo with application parameters and returns any non-nil error. + - `ConsumeGasTxSizeMiddleware`: Consumes gas proportional to the `tx` size based on application parameters. + - `DeductFeeMiddleware`: Deducts the `FeeAmount` from first signer of the `tx`. If the `x/feegrant` module is enabled and a fee granter is set, it deducts fees from the fee granter account. + - `SetPubKeyMiddleware`: Sets the pubkey from a `tx`'s signers that does not already have its corresponding pubkey saved in the state machine and in the current context. + - `ValidateSigCountMiddleware`: Validates the number of signatures in the `tx` based on app-parameters. + - `SigGasConsumeMiddleware`: Consumes parameter-defined amount of gas for each signature. This requires pubkeys to be set in context for all signers as part of `SetPubKeyMiddleware`. + - `SigVerificationMiddleware`: Verifies all signatures are valid. This requires pubkeys to be set in context for all signers as part of `SetPubKeyMiddleware`. + - `IncrementSequenceMiddleware`: Increments the account sequence for each signer to prevent replay attacks. + - `WithBranchedStore`: Creates a new MultiStore branch, discards downstream writes if the downstream returns error. + - `ConsumeBlockGasMiddleware`: Consume block gas. + - `TipMiddleware`: Transfer tips to the fee payer in transactions with tips. + +This default list of middlewares can be instantiated using the `NewDefaultTxHandler` function. If a chain wants to tweak the list of middlewares, they can create their own `NewTxHandler` function using the same template as `NewDefaultTxHandler`, and chain new middlewares in the `ComposeMiddleware` function.