From 1610de49fb8ded368e725247b87b3c0b46236a5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20=C5=BDivkovi=C4=87?= Date: Thu, 9 Jan 2025 21:09:19 +0100 Subject: [PATCH 1/6] Add support for ed25519 transaction signing --- crypto/keys/ed25519/ed25519.go | 36 ++++++++++++++++++++++++-- crypto/keys/ed25519/ed25519_test.go | 39 +++++++++++++++++++++++++++++ go.mod | 2 +- x/auth/ante/sigverify.go | 37 +++++++++++---------------- x/auth/ante/sigverify_test.go | 2 +- 5 files changed, 90 insertions(+), 26 deletions(-) diff --git a/crypto/keys/ed25519/ed25519.go b/crypto/keys/ed25519/ed25519.go index 792777b17de2..884d06f16782 100644 --- a/crypto/keys/ed25519/ed25519.go +++ b/crypto/keys/ed25519/ed25519.go @@ -7,6 +7,7 @@ import ( "fmt" "io" + "filippo.io/edwards25519" "github.com/cometbft/cometbft/crypto" "github.com/cometbft/cometbft/crypto/tmhash" "github.com/hdevalence/ed25519consensus" @@ -18,7 +19,7 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) -//------------------------------------- +// ------------------------------------- const ( PrivKeyName = "tendermint/PrivKeyEd25519" @@ -153,7 +154,7 @@ func GenPrivKeyFromSecret(secret []byte) *PrivKey { return &PrivKey{Key: ed25519.NewKeyFromSeed(seed)} } -//------------------------------------- +// ------------------------------------- var ( _ cryptotypes.PubKey = &PubKey{} @@ -230,3 +231,34 @@ func (pubKey PubKey) MarshalAminoJSON() ([]byte, error) { func (pubKey *PubKey) UnmarshalAminoJSON(bz []byte) error { return pubKey.UnmarshalAmino(bz) } + +// identityPoint is the “neutral element” in the ed25519 group, where +// point addition with identityPoint leaves the other point unchanged. +// It corresponds to coordinates (0, 1) in Edwards form and is not a valid public key +var identityPoint = edwards25519.NewIdentityPoint() + +// IsOnCurve checks that a 32B ed25519 public key is on the curve. +// The check fails for ed25519 identity points +func (pubKey *PubKey) IsOnCurve() bool { + // Make sure the public key is exactly 32B + if len(pubKey.Key) != ed25519.PublicKeySize { + // Invalid key size + return false + } + + // Make sure the public key bytes decodes into an ed25519 point + point, err := new(edwards25519.Point).SetBytes(pubKey.Key) + if err != nil || point == nil { + // Not a valid point on the curve + return false + } + + // Make sure the public key is not the identity point (all zeroes) + if point.Equal(identityPoint) == 1 { + // Public key is the identity point (useless) + return false + } + + // Public key is a valid point on the ed25519 curve + return true +} diff --git a/crypto/keys/ed25519/ed25519_test.go b/crypto/keys/ed25519/ed25519_test.go index 57c6f0563f3a..0708d3c32f8c 100644 --- a/crypto/keys/ed25519/ed25519_test.go +++ b/crypto/keys/ed25519/ed25519_test.go @@ -2,9 +2,11 @@ package ed25519_test import ( stded25519 "crypto/ed25519" + "crypto/rand" "encoding/base64" "testing" + "filippo.io/edwards25519" "github.com/cometbft/cometbft/crypto" tmed25519 "github.com/cometbft/cometbft/crypto/ed25519" "github.com/stretchr/testify/assert" @@ -254,3 +256,40 @@ func TestMarshalJSON(t *testing.T) { require.NoError(err) require.True(pk2.Equals(pk)) } + +func TestPubKeyOnCurve(t *testing.T) { + t.Parallel() + + t.Run("invalid public key size", func(t *testing.T) { + t.Parallel() + + key := &ed25519.PubKey{ + Key: make(stded25519.PublicKey, ed25519.PubKeySize+1), + } + + assert.False(t, key.IsOnCurve()) + }) + + t.Run("identity point", func(t *testing.T) { + t.Parallel() + + key := &ed25519.PubKey{ + Key: stded25519.PublicKey(edwards25519.NewIdentityPoint().Bytes()), + } + + assert.False(t, key.IsOnCurve()) + }) + + t.Run("valid public key", func(t *testing.T) { + t.Parallel() + + publicKey, _, err := stded25519.GenerateKey(rand.Reader) + require.NoError(t, err) + + key := &ed25519.PubKey{ + Key: publicKey, + } + + assert.True(t, key.IsOnCurve()) + }) +} diff --git a/go.mod b/go.mod index c2d31e222bbc..0f437508821e 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( cosmossdk.io/x/bank v0.0.0-00010101000000-000000000000 cosmossdk.io/x/staking v0.0.0-00010101000000-000000000000 cosmossdk.io/x/tx v1.0.0 + filippo.io/edwards25519 v1.1.0 github.com/99designs/keyring v1.2.2 github.com/bgentry/speakeasy v0.2.0 github.com/cometbft/cometbft v1.0.0 @@ -61,7 +62,6 @@ require ( require ( buf.build/gen/go/cometbft/cometbft/protocolbuffers/go v1.36.1-20241120201313-68e42a58b301.1 // indirect buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go v1.36.1-20240130113600-88ef6483f90f.1 // indirect - filippo.io/edwards25519 v1.1.0 // indirect github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect github.com/DataDog/datadog-go v4.8.3+incompatible // indirect github.com/DataDog/zstd v1.5.6 // indirect diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index fc93c3861a18..ce2a0d0b75d8 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -69,24 +69,22 @@ type AccountAbstractionKeeper interface { // // CONTRACT: Tx must implement SigVerifiableTx interface type SigVerificationDecorator struct { - ak AccountKeeper - aaKeeper AccountAbstractionKeeper - signModeHandler *txsigning.HandlerMap - sigGasConsumer SignatureVerificationGasConsumer - extraVerifyIsOnCurve func(pubKey cryptotypes.PubKey) (bool, error) + ak AccountKeeper + aaKeeper AccountAbstractionKeeper + signModeHandler *txsigning.HandlerMap + sigGasConsumer SignatureVerificationGasConsumer } func NewSigVerificationDecorator(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer, aaKeeper AccountAbstractionKeeper) SigVerificationDecorator { - return NewSigVerificationDecoratorWithVerifyOnCurve(ak, signModeHandler, sigGasConsumer, aaKeeper, nil) + return NewSigVerificationDecoratorWithVerifyOnCurve(ak, signModeHandler, sigGasConsumer, aaKeeper) } -func NewSigVerificationDecoratorWithVerifyOnCurve(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer, aaKeeper AccountAbstractionKeeper, verifyFn func(pubKey cryptotypes.PubKey) (bool, error)) SigVerificationDecorator { +func NewSigVerificationDecoratorWithVerifyOnCurve(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer, aaKeeper AccountAbstractionKeeper) SigVerificationDecorator { return SigVerificationDecorator{ - aaKeeper: aaKeeper, - ak: ak, - signModeHandler: signModeHandler, - sigGasConsumer: sigGasConsumer, - extraVerifyIsOnCurve: verifyFn, + aaKeeper: aaKeeper, + ak: ak, + signModeHandler: signModeHandler, + sigGasConsumer: sigGasConsumer, } } @@ -112,18 +110,16 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool { } func (svd SigVerificationDecorator) VerifyIsOnCurve(pubKey cryptotypes.PubKey) error { - if svd.extraVerifyIsOnCurve != nil { - handled, err := svd.extraVerifyIsOnCurve(pubKey) - if handled { - return err - } - } // when simulating pubKey.Key will always be nil if pubKey.Bytes() == nil { return nil } switch typedPubKey := pubKey.(type) { + case *ed25519.PubKey: + if !typedPubKey.IsOnCurve() { + return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "ed25519 key is not on curve") + } case *secp256k1.PubKey: pubKeyObject, err := secp256k1dcrd.ParsePubKey(typedPubKey.Bytes()) if err != nil { @@ -534,10 +530,7 @@ func DefaultSigVerificationGasConsumer(meter gas.Meter, sig signing.SignatureV2, switch pubkey := pubkey.(type) { case *ed25519.PubKey: - if err := meter.Consume(params.SigVerifyCostED25519, "ante verify: ed25519"); err != nil { - return err - } - return errorsmod.Wrap(sdkerrors.ErrInvalidPubKey, "ED25519 public keys are unsupported") + return meter.Consume(params.SigVerifyCostED25519, "ante verify: ed25519") case *secp256k1.PubKey: return meter.Consume(params.SigVerifyCostSecp256k1, "ante verify: secp256k1") diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index 3025706bf7e1..8c6ebd82412f 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -74,7 +74,7 @@ func TestConsumeSignatureVerificationGas(t *testing.T) { args{nil, ed25519.GenPrivKey().PubKey(), params, func(mm *gastestutil.MockMeter) { mm.EXPECT().Consume(p.SigVerifyCostED25519, "ante verify: ed25519").Times(1) }}, - true, + false, }, { "PubKeySecp256k1", From f546360b11027386ea3b07019421d313c533f547 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20=C5=BDivkovi=C4=87?= Date: Thu, 9 Jan 2025 21:20:49 +0100 Subject: [PATCH 2/6] Modify changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 34813000c836..74f7ab9e3c65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i * (baseapp) [#20291](https://github.com/cosmos/cosmos-sdk/pull/20291) Simulate nested messages. * (client/keys) [#21829](https://github.com/cosmos/cosmos-sdk/pull/21829) Add support for importing hex key using standard input. * (x/auth/ante) [#23128](https://github.com/cosmos/cosmos-sdk/pull/23128) Allow custom verifyIsOnCurve when validate tx for public key like ethsecp256k1. +* (x/auth/ante) [#23283](https://github.com/cosmos/cosmos-sdk/pull/23283) Allow ed25519 transaction signatures. ### Improvements From e69102c6e250757c5019fcc89bee37126b5712f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20=C5=BDivkovi=C4=87?= Date: Thu, 9 Jan 2025 21:33:40 +0100 Subject: [PATCH 3/6] Revert extraVerifyIsOnCurve change --- x/auth/ante/sigverify.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index ce2a0d0b75d8..49f91c56c782 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -69,22 +69,24 @@ type AccountAbstractionKeeper interface { // // CONTRACT: Tx must implement SigVerifiableTx interface type SigVerificationDecorator struct { - ak AccountKeeper - aaKeeper AccountAbstractionKeeper - signModeHandler *txsigning.HandlerMap - sigGasConsumer SignatureVerificationGasConsumer + ak AccountKeeper + aaKeeper AccountAbstractionKeeper + signModeHandler *txsigning.HandlerMap + sigGasConsumer SignatureVerificationGasConsumer + extraVerifyIsOnCurve func(pubKey cryptotypes.PubKey) (bool, error) } func NewSigVerificationDecorator(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer, aaKeeper AccountAbstractionKeeper) SigVerificationDecorator { - return NewSigVerificationDecoratorWithVerifyOnCurve(ak, signModeHandler, sigGasConsumer, aaKeeper) + return NewSigVerificationDecoratorWithVerifyOnCurve(ak, signModeHandler, sigGasConsumer, aaKeeper, nil) } -func NewSigVerificationDecoratorWithVerifyOnCurve(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer, aaKeeper AccountAbstractionKeeper) SigVerificationDecorator { +func NewSigVerificationDecoratorWithVerifyOnCurve(ak AccountKeeper, signModeHandler *txsigning.HandlerMap, sigGasConsumer SignatureVerificationGasConsumer, aaKeeper AccountAbstractionKeeper, verifyFn func(pubKey cryptotypes.PubKey) (bool, error)) SigVerificationDecorator { return SigVerificationDecorator{ - aaKeeper: aaKeeper, - ak: ak, - signModeHandler: signModeHandler, - sigGasConsumer: sigGasConsumer, + aaKeeper: aaKeeper, + ak: ak, + signModeHandler: signModeHandler, + sigGasConsumer: sigGasConsumer, + extraVerifyIsOnCurve: verifyFn, } } @@ -110,6 +112,12 @@ func OnlyLegacyAminoSigners(sigData signing.SignatureData) bool { } func (svd SigVerificationDecorator) VerifyIsOnCurve(pubKey cryptotypes.PubKey) error { + if svd.extraVerifyIsOnCurve != nil { + handled, err := svd.extraVerifyIsOnCurve(pubKey) + if handled { + return err + } + } // when simulating pubKey.Key will always be nil if pubKey.Bytes() == nil { return nil From f71bd94c99ca703202090c866e3acfe2bb1fa503 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20=C5=BDivkovi=C4=87?= Date: Thu, 9 Jan 2025 21:37:13 +0100 Subject: [PATCH 4/6] Update TestAnteHandlerChecks test case --- x/auth/ante/sigverify_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index 8c6ebd82412f..f91be111528c 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -414,7 +414,7 @@ func TestAnteHandlerChecks(t *testing.T) { testCases := []testCase{ {"secp256k1_onCurve", []cryptotypes.PrivKey{priv1}, msgs[0], []uint64{accs[0].GetAccountNumber()}, []uint64{0}, false, true}, {"secp256r1_onCurve", []cryptotypes.PrivKey{priv2}, msgs[1], []uint64{accs[1].GetAccountNumber()}, []uint64{0}, false, true}, - {"ed255619", []cryptotypes.PrivKey{priv3}, msgs[2], []uint64{accs[2].GetAccountNumber()}, []uint64{2}, true, false}, + {"ed255619", []cryptotypes.PrivKey{priv3}, msgs[2], []uint64{accs[2].GetAccountNumber()}, []uint64{2}, false, true}, } for i, tc := range testCases { From 1d816f0b6a99aaf24501a78a1daafcba5cdde914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20=C5=BDivkovi=C4=87?= Date: Thu, 9 Jan 2025 21:39:40 +0100 Subject: [PATCH 5/6] Update TestAnteHandlerChecks to not have redundant params --- x/auth/ante/sigverify_test.go | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/x/auth/ante/sigverify_test.go b/x/auth/ante/sigverify_test.go index f91be111528c..69fe87d05c0d 100644 --- a/x/auth/ante/sigverify_test.go +++ b/x/auth/ante/sigverify_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -401,23 +402,21 @@ func TestAnteHandlerChecks(t *testing.T) { anteHandler := sdk.ChainAnteDecorators(sigVerificationDecorator) type testCase struct { - name string - privs []cryptotypes.PrivKey - msg sdk.Msg - accNums []uint64 - accSeqs []uint64 - shouldErr bool - supported bool + name string + privs []cryptotypes.PrivKey + msg sdk.Msg + accNums []uint64 + accSeqs []uint64 } // Secp256r1 keys that are not on curve will fail before even doing any operation i.e when trying to get the pubkey testCases := []testCase{ - {"secp256k1_onCurve", []cryptotypes.PrivKey{priv1}, msgs[0], []uint64{accs[0].GetAccountNumber()}, []uint64{0}, false, true}, - {"secp256r1_onCurve", []cryptotypes.PrivKey{priv2}, msgs[1], []uint64{accs[1].GetAccountNumber()}, []uint64{0}, false, true}, - {"ed255619", []cryptotypes.PrivKey{priv3}, msgs[2], []uint64{accs[2].GetAccountNumber()}, []uint64{2}, false, true}, + {"secp256k1_onCurve", []cryptotypes.PrivKey{priv1}, msgs[0], []uint64{accs[0].GetAccountNumber()}, []uint64{0}}, + {"secp256r1_onCurve", []cryptotypes.PrivKey{priv2}, msgs[1], []uint64{accs[1].GetAccountNumber()}, []uint64{0}}, + {"ed255619", []cryptotypes.PrivKey{priv3}, msgs[2], []uint64{accs[2].GetAccountNumber()}, []uint64{2}}, } - for i, tc := range testCases { + for _, tc := range testCases { t.Run(fmt.Sprintf("%s key", tc.name), func(t *testing.T) { suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() // Create new txBuilder for each test @@ -434,16 +433,8 @@ func TestAnteHandlerChecks(t *testing.T) { byteCtx := suite.ctx.WithTxBytes(txBytes) _, err = anteHandler(byteCtx, tx, true) - if tc.shouldErr { - require.NotNil(t, err, "TestCase %d: %s did not error as expected", i, tc.name) - if tc.supported { - require.ErrorContains(t, err, "not on curve") - } else { - require.ErrorContains(t, err, "unsupported key type") - } - } else { - require.Nil(t, err, "TestCase %d: %s errored unexpectedly. Err: %v", i, tc.name, err) - } + + assert.NoError(t, err) }) } } From 18eeff8d53cf113e77faf298124428a5aee901b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Milo=C5=A1=20=C5=BDivkovi=C4=87?= Date: Mon, 27 Jan 2025 16:47:08 +0100 Subject: [PATCH 6/6] Update docs & go mod tidy --- docs/learn/beginner/03-accounts.md | 4 ++-- go.mod | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/learn/beginner/03-accounts.md b/docs/learn/beginner/03-accounts.md index 3d626f0d2d56..989aadc4dc33 100644 --- a/docs/learn/beginner/03-accounts.md +++ b/docs/learn/beginner/03-accounts.md @@ -53,10 +53,10 @@ The Cosmos SDK supports the following digital key schemes for creating digital s * `tm-ed25519`, as implemented in the [Cosmos SDK `crypto/keys/ed25519` package](https://github.com/cosmos/cosmos-sdk/blob/v0.52.0-beta.2/crypto/keys/ed25519/ed25519.go). This scheme is supported only for the consensus validation. | | Address length in bytes | Public key length in bytes | Used for transaction authentication | Used for consensus (cometbft) | -| :----------: | :---------------------: | :------------------------: | :---------------------------------: | :-----------------------------: | +| :----------: |:-----------------------:| :------------------------: |:-----------------------------------:| :-----------------------------: | | `secp256k1` | 20 | 33 | yes | no | | `secp256r1` | 32 | 33 | yes | no | -| `tm-ed25519` | -- not used -- | 32 | no | yes | +| `tm-ed25519` | 20 | 32 | yes | yes | ## Addresses diff --git a/go.mod b/go.mod index e772fc085892..aed73eeeacad 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,7 @@ require ( cosmossdk.io/x/bank v0.0.0-00010101000000-000000000000 cosmossdk.io/x/staking v0.0.0-00010101000000-000000000000 cosmossdk.io/x/tx v1.0.1 - filippo.io/edwards25519 v1.1.0 + filippo.io/edwards25519 v1.1.0 github.com/99designs/keyring v1.2.2 github.com/bgentry/speakeasy v0.2.0 github.com/cometbft/cometbft v1.0.0 @@ -60,8 +60,8 @@ require ( ) require ( - buf.build/gen/go/cometbft/cometbft/protocolbuffers/go v1.36.1-20241120201313-68e42a58b301.1 // indirect - buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go v1.36.1-20240130113600-88ef6483f90f.1 // indirect + buf.build/gen/go/cometbft/cometbft/protocolbuffers/go v1.36.3-20241120201313-68e42a58b301.1 // indirect + buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/go v1.36.3-20240130113600-88ef6483f90f.1 // indirect github.com/99designs/go-keychain v0.0.0-20191008050251-8e49817e8af4 // indirect github.com/DataDog/datadog-go v4.8.3+incompatible // indirect github.com/DataDog/zstd v1.5.6 // indirect