From 114de631a59572c3048ceccb110d1466c5a9ad7b Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Thu, 2 May 2019 15:36:42 -0400 Subject: [PATCH] Allow custom key types and address formats (#4232) Add additional parameter to NewAnteHandler for custom SignatureVerificationGasConsumer (the existing one is now called DefaultSigVerificationGasConsumer). Add addressVerifier field to sdk.Config which allows for custom address verification (to override the current fixed 20 byte address format). DefaultSigVerificationGasConsumer now uses type switching as opposed to string comparison. Other zones like Ethermint can now concretely specify which key types they accept. Closes: #3685 --- .pending/breaking/sdk/The-default-signatur | 1 + .../improvements/sdk/Add-SetAddressVerifi | 1 + .../improvements/sdk/Add-an-additional-pa | 1 + cmd/gaia/app/app.go | 2 +- cmd/gaia/cmd/gaiadebug/hack.go | 2 +- types/address.go | 30 +++++++-- types/address_test.go | 37 +++++++++++ types/config.go | 13 ++++ x/auth/ante.go | 39 ++++++----- x/auth/ante_test.go | 66 +++++++++++++++---- x/mock/app.go | 2 +- 11 files changed, 154 insertions(+), 40 deletions(-) create mode 100644 .pending/breaking/sdk/The-default-signatur create mode 100644 .pending/improvements/sdk/Add-SetAddressVerifi create mode 100644 .pending/improvements/sdk/Add-an-additional-pa 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