From adbf5a71e6c62e787afc99694f226f3d3aa99480 Mon Sep 17 00:00:00 2001 From: Robert Zaremba Date: Mon, 15 Feb 2021 16:32:51 +0100 Subject: [PATCH] adr-028 address generation (#8415) * Optimize secp256k1 hashing * Add ADR-028 related functions * Update ed25519 * fix errors/handle * fix build * fix build * Add tests and update function names * wip * Use LengthPrefix for composed addresses * add tests for NewComposed * add module hash function * fix append * rollback ed25519 ADR-28 update * rollback ed25519 ADR-28 test * Adding Module tests and convert tests to test suite * convert store_key_test.go to test suite * rollback test check comment * Rename assert.Panic and add comment * add note to ed25519 about SDK support with regards to ADR-28 * Update ed25519 TestAddress * Adding Deprecated notes for ed25519.PrivKey * Update crypto/keys/ed25519/ed25519.go Co-authored-by: Marie Gauthier * Update types/address/hash_test.go Co-authored-by: Marie Gauthier * solve linter issues * linter: remove gocritic Co-authored-by: Marie Gauthier --- codec/amino.go | 4 +- crypto/codec/amino.go | 2 +- crypto/keyring/legacy.go | 2 +- crypto/keys/ed25519/ed25519.go | 8 ++- crypto/keys/ed25519/ed25519_test.go | 6 ++ crypto/keys/ed25519/keys.pb.go | 13 ++-- crypto/keys/secp256k1/secp256k1.go | 7 +- docs/core/proto-docs.md | 13 ++-- proto/cosmos/crypto/ed25519/keys.proto | 13 ++-- types/address/README.md | 7 ++ types/address/hash.go | 74 +++++++++++++++++++++ types/address/hash_test.go | 90 ++++++++++++++++++++++++++ types/address/store_key_test.go | 20 ++++-- types/errors/handle.go | 12 ++++ x/auth/types/account.go | 1 + x/staking/types/validator.go | 4 +- 16 files changed, 239 insertions(+), 37 deletions(-) create mode 100644 types/address/README.md create mode 100644 types/address/hash.go create mode 100644 types/address/hash_test.go create mode 100644 types/errors/handle.go diff --git a/codec/amino.go b/codec/amino.go index 78fd650ca4..ea4c8d4fd8 100644 --- a/codec/amino.go +++ b/codec/amino.go @@ -13,8 +13,8 @@ import ( "github.com/cosmos/cosmos-sdk/codec/types" ) -// deprecated: LegacyAmino defines a wrapper for an Amino codec that properly handles protobuf -// types with Any's +// LegacyAmino defines a wrapper for an Amino codec that properly +// handles protobuf types with Any's. Deprecated. type LegacyAmino struct { Amino *amino.Codec } diff --git a/crypto/codec/amino.go b/crypto/codec/amino.go index d50a08864c..50119ed198 100644 --- a/crypto/codec/amino.go +++ b/crypto/codec/amino.go @@ -26,7 +26,7 @@ func RegisterCrypto(cdc *codec.LegacyAmino) { cdc.RegisterInterface((*cryptotypes.PrivKey)(nil), nil) cdc.RegisterConcrete(sr25519.PrivKey{}, sr25519.PrivKeyName, nil) - cdc.RegisterConcrete(&ed25519.PrivKey{}, + cdc.RegisterConcrete(&ed25519.PrivKey{}, //nolint:staticcheck ed25519.PrivKeyName, nil) cdc.RegisterConcrete(&secp256k1.PrivKey{}, secp256k1.PrivKeyName, nil) diff --git a/crypto/keyring/legacy.go b/crypto/keyring/legacy.go index d6653e9561..94bff8232f 100644 --- a/crypto/keyring/legacy.go +++ b/crypto/keyring/legacy.go @@ -42,7 +42,7 @@ var _ LegacyKeybase = dbKeybase{} // dbKeybase combines encryption and storage implementation to provide a // full-featured key manager. // -// NOTE: dbKeybase will be deprecated in favor of keyringKeybase. +// Deprecated: dbKeybase will be removed in favor of keyringKeybase. type dbKeybase struct { db dbm.DB } diff --git a/crypto/keys/ed25519/ed25519.go b/crypto/keys/ed25519/ed25519.go index 17368c4b12..7c70d573ec 100644 --- a/crypto/keys/ed25519/ed25519.go +++ b/crypto/keys/ed25519/ed25519.go @@ -116,7 +116,8 @@ func (privKey *PrivKey) UnmarshalAminoJSON(bz []byte) error { return privKey.UnmarshalAmino(bz) } -// GenPrivKey generates a new ed25519 private key. +// GenPrivKey generates a new ed25519 private key. These ed25519 keys must not +// be used in SDK apps except in a tendermint validator context. // It uses OS randomness in conjunction with the current global random seed // in tendermint/libs/common to generate the private key. func GenPrivKey() *PrivKey { @@ -137,6 +138,7 @@ func genPrivKey(rand io.Reader) *PrivKey { // GenPrivKeyFromSecret hashes the secret with SHA2, and uses // that 32 byte output to create the private key. +// NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. // NOTE: secret should be the output of a KDF like bcrypt, // if it's derived from user input. func GenPrivKeyFromSecret(secret []byte) *PrivKey { @@ -151,10 +153,14 @@ var _ cryptotypes.PubKey = &PubKey{} var _ codec.AminoMarshaler = &PubKey{} // Address is the SHA256-20 of the raw pubkey bytes. +// It doesn't implement ADR-28 addresses and it must not be used +// in SDK except in a tendermint validator context. func (pubKey *PubKey) Address() crypto.Address { if len(pubKey.Key) != PubKeySize { panic("pubkey is incorrect size") } + // For ADR-28 compatible address we would need to + // return address.Hash(proto.MessageName(pubKey), pubKey.Key) return crypto.Address(tmhash.SumTruncated(pubKey.Key)) } diff --git a/crypto/keys/ed25519/ed25519_test.go b/crypto/keys/ed25519/ed25519_test.go index 59cce4066a..27cef37f92 100644 --- a/crypto/keys/ed25519/ed25519_test.go +++ b/crypto/keys/ed25519/ed25519_test.go @@ -84,6 +84,12 @@ func TestPubKeyEquals(t *testing.T) { } } +func TestAddressEd25519(t *testing.T) { + pk := ed25519.PubKey{[]byte{125, 80, 29, 208, 159, 53, 119, 198, 73, 53, 187, 33, 199, 144, 62, 255, 1, 235, 117, 96, 128, 211, 17, 45, 34, 64, 189, 165, 33, 182, 54, 206}} + addr := pk.Address() + require.Len(t, addr, 20, "Address must be 20 bytes long") +} + func TestPrivKeyEquals(t *testing.T) { ed25519PrivKey := ed25519.GenPrivKey() diff --git a/crypto/keys/ed25519/keys.pb.go b/crypto/keys/ed25519/keys.pb.go index 35a98cf058..227b187013 100644 --- a/crypto/keys/ed25519/keys.pb.go +++ b/crypto/keys/ed25519/keys.pb.go @@ -24,11 +24,11 @@ var _ = math.Inf // proto package needs to be updated. const _ = proto.GoGoProtoPackageIsVersion3 // please upgrade the proto package -// PubKey defines a ed25519 public key -// Key is the compressed form of the pubkey. The first byte depends is a 0x02 byte -// if the y-coordinate is the lexicographically largest of the two associated with -// the x-coordinate. Otherwise the first byte is a 0x03. -// This prefix is followed with the x-coordinate. +// PubKey is an ed25519 public key for handling Tendermint keys in SDK. +// It's needed for Any serialization and SDK compatibility. +// It must not be used in a non Tendermint key context because it doesn't implement +// ADR-28. Nevertheless, you will like to use ed25519 in app user level +// then you must create a new proto message and follow ADR-28 for Address construction. type PubKey struct { Key crypto_ed25519.PublicKey `protobuf:"bytes,1,opt,name=key,proto3,casttype=crypto/ed25519.PublicKey" json:"key,omitempty"` } @@ -72,7 +72,8 @@ func (m *PubKey) GetKey() crypto_ed25519.PublicKey { return nil } -// PrivKey defines a ed25519 private key. +// Deprecated: PrivKey defines a ed25519 private key. +// NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. type PrivKey struct { Key crypto_ed25519.PrivateKey `protobuf:"bytes,1,opt,name=key,proto3,casttype=crypto/ed25519.PrivateKey" json:"key,omitempty"` } diff --git a/crypto/keys/secp256k1/secp256k1.go b/crypto/keys/secp256k1/secp256k1.go index eebe72a452..231a895a61 100644 --- a/crypto/keys/secp256k1/secp256k1.go +++ b/crypto/keys/secp256k1/secp256k1.go @@ -151,12 +151,9 @@ func (pubKey *PubKey) Address() crypto.Address { panic("length of pubkey is incorrect") } - hasherSHA256 := sha256.New() - hasherSHA256.Write(pubKey.Key) // does not error - sha := hasherSHA256.Sum(nil) - + sha := sha256.Sum256(pubKey.Key) hasherRIPEMD160 := ripemd160.New() - hasherRIPEMD160.Write(sha) // does not error + hasherRIPEMD160.Write(sha[:]) // does not error return crypto.Address(hasherRIPEMD160.Sum(nil)) } diff --git a/docs/core/proto-docs.md b/docs/core/proto-docs.md index a0c6068d25..c98035efc0 100644 --- a/docs/core/proto-docs.md +++ b/docs/core/proto-docs.md @@ -2885,7 +2885,8 @@ Msg defines the bank Msg service. ### PrivKey -PrivKey defines a ed25519 private key. +Deprecated: PrivKey defines a ed25519 private key. +NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. | Field | Type | Label | Description | @@ -2900,11 +2901,11 @@ PrivKey defines a ed25519 private key. ### PubKey -PubKey defines a ed25519 public key -Key is the compressed form of the pubkey. The first byte depends is a 0x02 byte -if the y-coordinate is the lexicographically largest of the two associated with -the x-coordinate. Otherwise the first byte is a 0x03. -This prefix is followed with the x-coordinate. +PubKey is an ed25519 public key for handling Tendermint keys in SDK. +It's needed for Any serialization and SDK compatibility. +It must not be used in a non Tendermint key context because it doesn't implement +ADR-28. Nevertheless, you will like to use ed25519 in app user level +then you must create a new proto message and follow ADR-28 for Address construction. | Field | Type | Label | Description | diff --git a/proto/cosmos/crypto/ed25519/keys.proto b/proto/cosmos/crypto/ed25519/keys.proto index bed9c29cc7..6ffec34483 100644 --- a/proto/cosmos/crypto/ed25519/keys.proto +++ b/proto/cosmos/crypto/ed25519/keys.proto @@ -5,18 +5,19 @@ import "gogoproto/gogo.proto"; option go_package = "github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"; -// PubKey defines a ed25519 public key -// Key is the compressed form of the pubkey. The first byte depends is a 0x02 byte -// if the y-coordinate is the lexicographically largest of the two associated with -// the x-coordinate. Otherwise the first byte is a 0x03. -// This prefix is followed with the x-coordinate. +// PubKey is an ed25519 public key for handling Tendermint keys in SDK. +// It's needed for Any serialization and SDK compatibility. +// It must not be used in a non Tendermint key context because it doesn't implement +// ADR-28. Nevertheless, you will like to use ed25519 in app user level +// then you must create a new proto message and follow ADR-28 for Address construction. message PubKey { option (gogoproto.goproto_stringer) = false; bytes key = 1 [(gogoproto.casttype) = "crypto/ed25519.PublicKey"]; } -// PrivKey defines a ed25519 private key. +// Deprecated: PrivKey defines a ed25519 private key. +// NOTE: ed25519 keys must not be used in SDK apps except in a tendermint validator context. message PrivKey { bytes key = 1 [(gogoproto.casttype) = "crypto/ed25519.PrivateKey"]; } diff --git a/types/address/README.md b/types/address/README.md new file mode 100644 index 0000000000..ebc647ec0f --- /dev/null +++ b/types/address/README.md @@ -0,0 +1,7 @@ +# Account + +This package defines Cosmos SDK address related functions. + +## References + ++ [ADR-028](../../docs/architecture/adr-028-public-key-addresses.md) diff --git a/types/address/hash.go b/types/address/hash.go new file mode 100644 index 0000000000..8c0cddae02 --- /dev/null +++ b/types/address/hash.go @@ -0,0 +1,74 @@ +package address + +import ( + "bytes" + "crypto/sha256" + "fmt" + "reflect" + "sort" + "unsafe" + + "github.com/cosmos/cosmos-sdk/types/errors" +) + +// Len is the length of base addresses +const Len = sha256.Size + +type Addressable interface { + Address() []byte +} + +// Hash creates a new address from address type and key +func Hash(typ string, key []byte) []byte { + hasher := sha256.New() + hasher.Write(unsafeStrToByteArray(typ)) + th := hasher.Sum(nil) + + hasher.Reset() + _, err := hasher.Write(th) + // the error always nil, it's here only to satisfy the io.Writer interface + errors.AssertNil(err) + _, err = hasher.Write(key) + errors.AssertNil(err) + return hasher.Sum(nil) +} + +// NewComposed creates a new address based on sub addresses. +func NewComposed(typ string, subAddresses []Addressable) ([]byte, error) { + as := make([][]byte, len(subAddresses)) + totalLen := 0 + var err error + for i := range subAddresses { + a := subAddresses[i].Address() + as[i], err = LengthPrefix(a) + if err != nil { + return nil, fmt.Errorf("not compatible sub-adddress=%v at index=%d [%w]", a, i, err) + } + totalLen += len(as[i]) + } + + sort.Slice(as, func(i, j int) bool { return bytes.Compare(as[i], as[j]) <= 0 }) + key := make([]byte, totalLen) + offset := 0 + for i := range as { + copy(key[offset:], as[i]) + offset += len(as[i]) + } + return Hash(typ, key), nil +} + +// Module is a specialized version of a composed address for modules. Each module account +// is constructed from a module name and module account key. +func Module(moduleName string, key []byte) []byte { + mKey := append([]byte(moduleName), 0) + return Hash("module", append(mKey, key...)) +} + +// unsafeStrToByteArray uses unsafe to convert string into byte array. Returned array +// cannot be altered after this functions is called +func unsafeStrToByteArray(s string) []byte { + sh := *(*reflect.SliceHeader)(unsafe.Pointer(&s)) + sh.Cap = sh.Len + bs := *(*[]byte)(unsafe.Pointer(&sh)) + return bs +} diff --git a/types/address/hash_test.go b/types/address/hash_test.go new file mode 100644 index 0000000000..be096c357f --- /dev/null +++ b/types/address/hash_test.go @@ -0,0 +1,90 @@ +package address + +import ( + "crypto/sha256" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/suite" +) + +func TestAddressSuite(t *testing.T) { + suite.Run(t, new(AddressSuite)) +} + +type AddressSuite struct{ suite.Suite } + +func (suite *AddressSuite) TestHash() { + assert := suite.Assert() + typ := "1" + key := []byte{1} + part1 := sha256.Sum256([]byte(typ)) + expected := sha256.Sum256(append(part1[:], key...)) + received := Hash(typ, key) + assert.Equal(expected[:], received, "must create a correct address") + + received = Hash("other", key) + assert.NotEqual(expected[:], received, "must create a correct address") + assert.Len(received, Len, "must have correct length") +} + +func (suite *AddressSuite) TestComposed() { + assert := suite.Assert() + a1 := addrMock{[]byte{11, 12}} + a2 := addrMock{[]byte{21, 22}} + + typ := "multisig" + ac, err := NewComposed(typ, []Addressable{a1, a2}) + assert.NoError(err) + assert.Len(ac, Len) + + // check if optimizations work + checkingKey := append([]byte{}, a1.AddressWithLen(suite.T())...) + checkingKey = append(checkingKey, a2.AddressWithLen(suite.T())...) + ac2 := Hash(typ, checkingKey) + assert.Equal(ac, ac2, "NewComposed works correctly") + + // changing order of addresses shouldn't impact a composed address + ac2, err = NewComposed(typ, []Addressable{a2, a1}) + assert.NoError(err) + assert.Len(ac2, Len) + assert.Equal(ac, ac2, "NewComposed is not sensitive for order") + + // changing a type should change composed address + ac2, err = NewComposed(typ+"other", []Addressable{a2, a1}) + assert.NoError(err) + assert.NotEqual(ac, ac2, "NewComposed must be sensitive to type") + + // changing order of addresses shouldn't impact a composed address + ac2, err = NewComposed(typ, []Addressable{a1, addrMock{make([]byte, 300, 300)}}) + assert.Error(err) + assert.Contains(err.Error(), "should be max 255 bytes, got 300") +} + +func (suite *AddressSuite) TestModule() { + assert := suite.Assert() + var modName, key = "myModule", []byte{1, 2} + addr := Module(modName, key) + assert.Len(addr, Len, "must have address length") + + addr2 := Module("myModule2", key) + assert.NotEqual(addr, addr2, "changing module name must change address") + + addr3 := Module(modName, []byte{1, 2, 3}) + assert.NotEqual(addr, addr3, "changing key must change address") + assert.NotEqual(addr2, addr3, "changing key must change address") +} + +type addrMock struct { + Addr []byte +} + +func (a addrMock) Address() []byte { + return a.Addr +} + +func (a addrMock) AddressWithLen(t *testing.T) []byte { + addr, err := LengthPrefix(a.Addr) + assert.NoError(t, err) + return addr +} diff --git a/types/address/store_key_test.go b/types/address/store_key_test.go index 3bb00bd022..ac28f814cc 100644 --- a/types/address/store_key_test.go +++ b/types/address/store_key_test.go @@ -3,12 +3,19 @@ package address_test import ( "testing" - "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" "github.com/cosmos/cosmos-sdk/types/address" ) -func TestLengthPrefixedAddressStoreKey(t *testing.T) { +func TestStoreKeySuite(t *testing.T) { + suite.Run(t, new(StoreKeySuite)) +} + +type StoreKeySuite struct{ suite.Suite } + +func (suite *StoreKeySuite) TestLengthPrefix() { + require := suite.Require() addr10byte := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9} addr20byte := []byte{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19} addr256byte := make([]byte, 256) @@ -23,15 +30,16 @@ func TestLengthPrefixedAddressStoreKey(t *testing.T) { {"20-byte address", addr20byte, append([]byte{byte(20)}, addr20byte...), false}, {"256-byte address (too long)", addr256byte, nil, true}, } + for _, tt := range tests { tt := tt - t.Run(tt.name, func(t *testing.T) { + suite.Run(tt.name, func() { storeKey, err := address.LengthPrefix(tt.addr) if tt.expErr { - require.Error(t, err) + require.Error(err) } else { - require.NoError(t, err) - require.Equal(t, tt.expStoreKey, storeKey) + require.NoError(err) + require.Equal(tt.expStoreKey, storeKey) } }) } diff --git a/types/errors/handle.go b/types/errors/handle.go new file mode 100644 index 0000000000..33c3fbfdea --- /dev/null +++ b/types/errors/handle.go @@ -0,0 +1,12 @@ +package errors + +import "fmt" + +// AssertNil panics on error +// Should be only used with interface methods, which require return error, but the +// error is always nil +func AssertNil(err error) { + if err != nil { + panic(fmt.Errorf("logic error - this should never happen. %w", err)) + } +} diff --git a/x/auth/types/account.go b/x/auth/types/account.go index eb9939ffce..653f89bcac 100644 --- a/x/auth/types/account.go +++ b/x/auth/types/account.go @@ -48,6 +48,7 @@ func ProtoBaseAccount() AccountI { } // NewBaseAccountWithAddress - returns a new base account with a given address +// leaving AccountNumber and Sequence to zero. func NewBaseAccountWithAddress(addr sdk.AccAddress) *BaseAccount { return &BaseAccount{ Address: addr.String(), diff --git a/x/staking/types/validator.go b/x/staking/types/validator.go index e551e75252..b23a7708f0 100644 --- a/x/staking/types/validator.go +++ b/x/staking/types/validator.go @@ -103,9 +103,7 @@ func (v Validators) Less(i, j int) bool { // Implements sort interface func (v Validators) Swap(i, j int) { - it := v[i] - v[i] = v[j] - v[j] = it + v[i], v[j] = v[j], v[i] } // ValidatorsByVotingPower implements sort.Interface for []Validator based on