diff --git a/.pending/breaking/sdk/The-default-signatur b/.pending/breaking/sdk/The-default-signatur new file mode 100644 index 0000000000..009c142693 --- /dev/null +++ b/.pending/breaking/sdk/The-default-signatur @@ -0,0 +1 @@ +#3685 The default signature verification gas logic (`DefaultSigVerificationGasConsumer`) now specifies explicit key types rather than string pattern matching. This means that zones that depended on string matching to allow other keys will need to write a custom `SignatureVerificationGasConsumer` function. diff --git a/.pending/improvements/sdk/Add-SetAddressVerifi b/.pending/improvements/sdk/Add-SetAddressVerifi new file mode 100644 index 0000000000..fa12bad267 --- /dev/null +++ b/.pending/improvements/sdk/Add-SetAddressVerifi @@ -0,0 +1 @@ +#3685 Add `SetAddressVerifier` and `GetAddressVerifier` to `sdk.Config` to allow SDK users to configure custom address format verification logic (to override the default limitation of 20-byte addresses). diff --git a/.pending/improvements/sdk/Add-an-additional-pa b/.pending/improvements/sdk/Add-an-additional-pa new file mode 100644 index 0000000000..2aa5445ba4 --- /dev/null +++ b/.pending/improvements/sdk/Add-an-additional-pa @@ -0,0 +1 @@ +#3685 Add an additional parameter to NewAnteHandler for a custom `SignatureVerificationGasConsumer` (the default logic is now in `DefaultSigVerificationGasConsumer). This allows SDK users to configure their own logic for which key types are accepted and how those key types consume gas. diff --git a/cmd/gaia/app/app.go b/cmd/gaia/app/app.go index de910348c7..3ff83747de 100644 --- a/cmd/gaia/app/app.go +++ b/cmd/gaia/app/app.go @@ -195,7 +195,7 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest b ) app.SetInitChainer(app.initChainer) app.SetBeginBlocker(app.BeginBlocker) - app.SetAnteHandler(auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper)) + app.SetAnteHandler(auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper, auth.DefaultSigVerificationGasConsumer)) app.SetEndBlocker(app.EndBlocker) if loadLatest { diff --git a/cmd/gaia/cmd/gaiadebug/hack.go b/cmd/gaia/cmd/gaiadebug/hack.go index 94f8bc6f32..8f42bbf61a 100644 --- a/cmd/gaia/cmd/gaiadebug/hack.go +++ b/cmd/gaia/cmd/gaiadebug/hack.go @@ -191,7 +191,7 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, baseAppOptions ...func(*bam.BaseAp app.SetInitChainer(app.initChainer) app.SetBeginBlocker(app.BeginBlocker) app.SetEndBlocker(app.EndBlocker) - app.SetAnteHandler(auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper)) + app.SetAnteHandler(auth.NewAnteHandler(app.accountKeeper, app.feeCollectionKeeper, auth.DefaultSigVerificationGasConsumer)) app.MountStores(app.keyMain, app.keyAccount, app.keyStaking, app.keySlashing, app.keyParams) app.MountStore(app.tkeyParams, sdk.StoreTypeTransient) err := app.LoadLatestVersion(app.keyMain) diff --git a/types/address.go b/types/address.go index d62f947af1..5b73ca47df 100644 --- a/types/address.go +++ b/types/address.go @@ -86,6 +86,21 @@ func AccAddressFromHex(address string) (addr AccAddress, err error) { return AccAddress(bz), nil } +// VerifyAddressFormat verifies that the provided bytes form a valid address +// according to the default address rules or a custom address verifier set by +// GetConfig().SetAddressVerifier() +func VerifyAddressFormat(bz []byte) error { + verifier := GetConfig().GetAddressVerifier() + if verifier != nil { + return verifier(bz) + } else { + if len(bz) != AddrLen { + return errors.New("Incorrect address length") + } + } + return nil +} + // AccAddressFromBech32 creates an AccAddress from a Bech32 string. func AccAddressFromBech32(address string) (addr AccAddress, err error) { if len(strings.TrimSpace(address)) == 0 { @@ -99,8 +114,9 @@ func AccAddressFromBech32(address string) (addr AccAddress, err error) { return nil, err } - if len(bz) != AddrLen { - return nil, errors.New("Incorrect address length") + err = VerifyAddressFormat(bz) + if err != nil { + return nil, err } return AccAddress(bz), nil @@ -229,8 +245,9 @@ func ValAddressFromBech32(address string) (addr ValAddress, err error) { return nil, err } - if len(bz) != AddrLen { - return nil, errors.New("Incorrect address length") + err = VerifyAddressFormat(bz) + if err != nil { + return nil, err } return ValAddress(bz), nil @@ -360,8 +377,9 @@ func ConsAddressFromBech32(address string) (addr ConsAddress, err error) { return nil, err } - if len(bz) != AddrLen { - return nil, errors.New("Incorrect address length") + err = VerifyAddressFormat(bz) + if err != nil { + return nil, err } return ConsAddress(bz), nil diff --git a/types/address_test.go b/types/address_test.go index 754d3e754c..406ff1b7e7 100644 --- a/types/address_test.go +++ b/types/address_test.go @@ -2,6 +2,7 @@ package types_test import ( "encoding/hex" + "fmt" "math/rand" "testing" @@ -290,3 +291,39 @@ func TestAddressInterface(t *testing.T) { } } + +func TestCustomAddressVerifier(t *testing.T) { + // Create a 10 byte address + addr := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} + accBech := types.AccAddress(addr).String() + valBech := types.ValAddress(addr).String() + consBech := types.ConsAddress(addr).String() + // Verifiy that the default logic rejects this 10 byte address + err := types.VerifyAddressFormat(addr) + require.NotNil(t, err) + _, err = types.AccAddressFromBech32(accBech) + require.NotNil(t, err) + _, err = types.ValAddressFromBech32(valBech) + require.NotNil(t, err) + _, err = types.ConsAddressFromBech32(consBech) + require.NotNil(t, err) + + // Set a custom address verifier that accepts 10 or 20 byte addresses + types.GetConfig().SetAddressVerifier(func(bz []byte) error { + n := len(bz) + if n == 10 || n == types.AddrLen { + return nil + } + return fmt.Errorf("incorrect address length %d", n) + }) + + // Verifiy that the custom logic accepts this 10 byte address + err = types.VerifyAddressFormat(addr) + require.Nil(t, err) + _, err = types.AccAddressFromBech32(accBech) + require.Nil(t, err) + _, err = types.ValAddressFromBech32(valBech) + require.Nil(t, err) + _, err = types.ConsAddressFromBech32(consBech) + require.Nil(t, err) +} diff --git a/types/config.go b/types/config.go index 91cd924d4f..dae512d6fe 100644 --- a/types/config.go +++ b/types/config.go @@ -11,6 +11,7 @@ type Config struct { sealed bool bech32AddressPrefix map[string]string txEncoder TxEncoder + addressVerifier func([]byte) error } var ( @@ -73,6 +74,13 @@ func (config *Config) SetTxEncoder(encoder TxEncoder) { config.txEncoder = encoder } +// SetAddressVerifier builds the Config with the provided function for verifying that addresses +// have the correct format +func (config *Config) SetAddressVerifier(addressVerifier func([]byte) error) { + config.assertNotSealed() + config.addressVerifier = addressVerifier +} + // Seal seals the config such that the config state could not be modified further func (config *Config) Seal() *Config { config.mtx.Lock() @@ -116,3 +124,8 @@ func (config *Config) GetBech32ConsensusPubPrefix() string { func (config *Config) GetTxEncoder() TxEncoder { return config.txEncoder } + +// GetAddressVerifier returns the function to verify that addresses have the correct format +func (config *Config) GetAddressVerifier() func([]byte) error { + return config.addressVerifier +} diff --git a/x/auth/ante.go b/x/auth/ante.go index 376f36018e..c7021e10bd 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -4,7 +4,7 @@ import ( "bytes" "encoding/hex" "fmt" - "strings" + "github.com/tendermint/tendermint/crypto/ed25519" "time" "github.com/tendermint/tendermint/crypto" @@ -27,10 +27,14 @@ func init() { copy(simSecp256k1Pubkey[:], bz) } +// SignatureVerificationGasConsumer is the type of function that is used to both consume gas when verifying signatures +// and also to accept or reject different types of PubKey's. This is where apps can define their own PubKey types. +type SignatureVerificationGasConsumer = func(meter sdk.GasMeter, sig []byte, pubkey crypto.PubKey, params Params) sdk.Result + // NewAnteHandler returns an AnteHandler that checks and increments sequence // numbers, checks signatures & account numbers, and deducts fees from the first // signer. -func NewAnteHandler(ak AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { +func NewAnteHandler(ak AccountKeeper, fck FeeCollectionKeeper, sigGasConsumer SignatureVerificationGasConsumer) sdk.AnteHandler { return func( ctx sdk.Context, tx sdk.Tx, simulate bool, ) (newCtx sdk.Context, res sdk.Result, abort bool) { @@ -127,7 +131,7 @@ func NewAnteHandler(ak AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { // check signature, return account with incremented nonce signBytes := GetSignBytes(newCtx.ChainID(), stdTx, signerAccs[i], isGenesis) - signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytes, simulate, params) + signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytes, simulate, params, sigGasConsumer) if !res.IsOK() { return newCtx, res, true } @@ -168,6 +172,7 @@ func ValidateMemo(stdTx StdTx, params Params) sdk.Result { // a pubkey, set it. func processSig( ctx sdk.Context, acc Account, sig StdSignature, signBytes []byte, simulate bool, params Params, + sigGasConsumer SignatureVerificationGasConsumer, ) (updatedAcc Account, res sdk.Result) { pubKey, res := ProcessPubKey(acc, sig, simulate) @@ -188,7 +193,7 @@ func processSig( consumeSimSigGas(ctx.GasMeter(), pubKey, sig, params) } - if res := consumeSigVerificationGas(ctx.GasMeter(), sig.Signature, pubKey, params); !res.IsOK() { + if res := sigGasConsumer(ctx.GasMeter(), sig.Signature, pubKey, params); !res.IsOK() { return nil, res } @@ -254,36 +259,30 @@ func ProcessPubKey(acc Account, sig StdSignature, simulate bool) (crypto.PubKey, return pubKey, sdk.Result{} } -// consumeSigVerificationGas consumes gas for signature verification based upon -// the public key type. The cost is fetched from the given params and is matched +// DefaultSigVerificationGasConsumer is the default implementation of SignatureVerificationGasConsumer. It consumes gas +// for signature verification based upon the public key type. The cost is fetched from the given params and is matched // by the concrete type. -// -// TODO: Design a cleaner and flexible way to match concrete public key types. -func consumeSigVerificationGas( +func DefaultSigVerificationGasConsumer( meter sdk.GasMeter, sig []byte, pubkey crypto.PubKey, params Params, ) sdk.Result { - - pubkeyType := strings.ToLower(fmt.Sprintf("%T", pubkey)) - - switch { - case strings.Contains(pubkeyType, "ed25519"): + switch pubkey := pubkey.(type) { + case ed25519.PubKeyEd25519: meter.ConsumeGas(params.SigVerifyCostED25519, "ante verify: ed25519") return sdk.ErrInvalidPubKey("ED25519 public keys are unsupported").Result() - case strings.Contains(pubkeyType, "secp256k1"): + case secp256k1.PubKeySecp256k1: meter.ConsumeGas(params.SigVerifyCostSecp256k1, "ante verify: secp256k1") return sdk.Result{} - case strings.Contains(pubkeyType, "multisigthreshold"): + case multisig.PubKeyMultisigThreshold: var multisignature multisig.Multisignature codec.Cdc.MustUnmarshalBinaryBare(sig, &multisignature) - multisigPubKey := pubkey.(multisig.PubKeyMultisigThreshold) - consumeMultisignatureVerificationGas(meter, multisignature, multisigPubKey, params) + consumeMultisignatureVerificationGas(meter, multisignature, pubkey, params) return sdk.Result{} default: - return sdk.ErrInvalidPubKey(fmt.Sprintf("unrecognized public key type: %s", pubkeyType)).Result() + return sdk.ErrInvalidPubKey(fmt.Sprintf("unrecognized public key type: %T", pubkey)).Result() } } @@ -295,7 +294,7 @@ func consumeMultisignatureVerificationGas(meter sdk.GasMeter, sigIndex := 0 for i := 0; i < size; i++ { if sig.BitArray.GetIndex(i) { - consumeSigVerificationGas(meter, sig.Sigs[sigIndex], pubkey.PubKeys[i], params) + DefaultSigVerificationGasConsumer(meter, sig.Sigs[sigIndex], pubkey.PubKeys[i], params) sigIndex++ } } diff --git a/x/auth/ante_test.go b/x/auth/ante_test.go index beff481e9a..380b8eb914 100644 --- a/x/auth/ante_test.go +++ b/x/auth/ante_test.go @@ -47,7 +47,7 @@ func TestAnteHandlerSigErrors(t *testing.T) { // setup input := setupTestInput() ctx := input.ctx - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) // keys and addresses priv1, _, addr1 := keyPubAddr() @@ -95,7 +95,7 @@ func TestAnteHandlerSigErrors(t *testing.T) { func TestAnteHandlerAccountNumbers(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(1) // keys and addresses @@ -150,7 +150,7 @@ func TestAnteHandlerAccountNumbers(t *testing.T) { func TestAnteHandlerAccountNumbersAtBlockHeightZero(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(0) // keys and addresses @@ -205,7 +205,7 @@ func TestAnteHandlerAccountNumbersAtBlockHeightZero(t *testing.T) { func TestAnteHandlerSequences(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(1) // keys and addresses @@ -280,7 +280,7 @@ func TestAnteHandlerFees(t *testing.T) { // setup input := setupTestInput() ctx := input.ctx - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) // keys and addresses priv1, _, addr1 := keyPubAddr() @@ -319,7 +319,7 @@ func TestAnteHandlerFees(t *testing.T) { func TestAnteHandlerMemoGas(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(1) // keys and addresses @@ -358,7 +358,7 @@ func TestAnteHandlerMemoGas(t *testing.T) { func TestAnteHandlerMultiSigner(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(1) // keys and addresses @@ -405,7 +405,7 @@ func TestAnteHandlerMultiSigner(t *testing.T) { func TestAnteHandlerBadSignBytes(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(1) // keys and addresses @@ -480,7 +480,7 @@ func TestAnteHandlerBadSignBytes(t *testing.T) { func TestAnteHandlerSetPubKey(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(1) // keys and addresses @@ -594,7 +594,7 @@ func TestConsumeSignatureVerificationGas(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - res := consumeSigVerificationGas(tt.args.meter, tt.args.sig, tt.args.pubkey, tt.args.params) + res := DefaultSigVerificationGasConsumer(tt.args.meter, tt.args.sig, tt.args.pubkey, tt.args.params) if tt.shouldErr { require.False(t, res.IsOK()) @@ -674,7 +674,7 @@ func TestCountSubkeys(t *testing.T) { func TestAnteHandlerSigLimitExceeded(t *testing.T) { // setup input := setupTestInput() - anteHandler := NewAnteHandler(input.ak, input.fck) + anteHandler := NewAnteHandler(input.ak, input.fck, DefaultSigVerificationGasConsumer) ctx := input.ctx.WithBlockHeight(1) // keys and addresses @@ -756,3 +756,47 @@ func TestEnsureSufficientMempoolFees(t *testing.T) { ) } } + +// Test custom SignatureVerificationGasConsumer +func TestCustomSignatureVerificationGasConsumer(t *testing.T) { + // setup + input := setupTestInput() + // setup an ante handler that only accepts PubKeyEd25519 + anteHandler := NewAnteHandler(input.ak, input.fck, func(meter sdk.GasMeter, sig []byte, pubkey crypto.PubKey, params Params) sdk.Result { + switch pubkey := pubkey.(type) { + case ed25519.PubKeyEd25519: + meter.ConsumeGas(params.SigVerifyCostED25519, "ante verify: ed25519") + return sdk.Result{} + default: + return sdk.ErrInvalidPubKey(fmt.Sprintf("unrecognized public key type: %T", pubkey)).Result() + } + }) + ctx := input.ctx.WithBlockHeight(1) + + // verify that an secp256k1 account gets rejected + priv1, _, addr1 := keyPubAddr() + acc1 := input.ak.NewAccountWithAddress(ctx, addr1) + _ = acc1.SetCoins(sdk.NewCoins(sdk.NewInt64Coin("atom", 150))) + input.ak.SetAccount(ctx, acc1) + var tx sdk.Tx + msg := newTestMsg(addr1) + privs, accnums, seqs := []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0} + fee := newStdFee() + msgs := []sdk.Msg{msg} + tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee) + checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeInvalidPubKey) + + // verify that an ed25519 account gets accepted + priv2 := ed25519.GenPrivKey() + pub2 := priv2.PubKey() + addr2 := sdk.AccAddress(pub2.Address()) + acc2 := input.ak.NewAccountWithAddress(ctx, addr2) + _ = acc2.SetCoins(sdk.NewCoins(sdk.NewInt64Coin("atom", 150))) + input.ak.SetAccount(ctx, acc2) + msg = newTestMsg(addr2) + privs, accnums, seqs = []crypto.PrivKey{priv2}, []uint64{1}, []uint64{0} + fee = newStdFee() + msgs = []sdk.Msg{msg} + tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee) + checkValidTx(t, anteHandler, ctx, tx, false) +} diff --git a/x/mock/app.go b/x/mock/app.go index 33375c6269..75deed6a5c 100644 --- a/x/mock/app.go +++ b/x/mock/app.go @@ -82,7 +82,7 @@ func NewApp() *App { // Initialize the app. The chainers and blockers can be overwritten before // calling complete setup. app.SetInitChainer(app.InitChainer) - app.SetAnteHandler(auth.NewAnteHandler(app.AccountKeeper, app.FeeCollectionKeeper)) + app.SetAnteHandler(auth.NewAnteHandler(app.AccountKeeper, app.FeeCollectionKeeper, auth.DefaultSigVerificationGasConsumer)) // Not sealing for custom extension