feat: support secp256k1 key #187

Yaiba · 2023-08-15T19:33:20Z

add secp256k1 key support
default secp256k1 singing method using EIP-191 personal_sign
add Signer interface
support verify signature from EIP-191 personal_sign and cometbft secp256k1

Update:

use signer instead of PrivateKey in client
parse PublicKey on Chain side based on signature_type

Yaiba · 2023-08-15T19:34:09Z

This pr doesn't include code to differentiate different private key type.

After merge, Acceptance and integration test won't panic on private keys

pkg/crypto/secp256k1.go

brennanjl

As discussed above, we should remove ethAccount.TextHash in the Sign method, as well as create some sort of optionality for Secp256k1PublicKey.Verify. Below is the code from go-ethereum implementing that:

// TextHash is a helper function that calculates a hash for the given message that can be
// safely used to calculate a signature from.
//
// The hash is calculated as
//
//	keccak256("\x19Ethereum Signed Message:\n"${message length}${message}).
//
// This gives context to the signed message and prevents signing of transactions.
func TextHash(data []byte) []byte {
	hash, _ := TextAndHash(data)
	return hash
}

// TextAndHash is a helper function that calculates a hash for the given message that can be
// safely used to calculate a signature from.
//
// The hash is calculated as
//
//	keccak256("\x19Ethereum Signed Message:\n"${message length}${message}).
//
// This gives context to the signed message and prevents signing of transactions.
func TextAndHash(data []byte) ([]byte, string) {
	msg := fmt.Sprintf("\x19Ethereum Signed Message:\n%d%s", len(data), string(data))
	hasher := sha3.NewLegacyKeccak256()
	hasher.Write([]byte(msg))
	return hasher.Sum(nil), msg
}

It seems that usage of TextHash should be dependent on some user flag. Furthermore, I can't really think of a reason why someone would care to use this library to sign with that; from my perspective, we really only need it for signature validation (and still, a flag should probably be used).

jchappelow · 2023-08-16T15:35:14Z

@Yaiba and I seem to have settled on a reasonable plan for this.

PubkeyType are limited to the actual real type (ed25519, secp256k1, etc), not concerned with variants of message digesting
The Signature type can specify the variant and perform the necessary digest (either of prefixing or hash function application)

This lets the PublicKey's Verify look like Verify(sig, hash []byte), where the signature is a canonical type and the message is hashed, with the (*Signature).Verify wrapping that and preparing both inputs as required.

jchappelow · 2023-08-16T15:36:57Z

BTW, I actually didn't realize that ed25519 really does specify the hash function for sign/verify. That's notably different from secp256 where you find many different hash functions (keccak, keccak-variants, and sha256) and prefixing schemes used in the wild. My bad.

jchappelow · 2023-08-17T15:39:49Z

pkg/crypto/signature.go

+func (s SignatureType) KeyType() KeyType {
+	switch s {
+	case SIGNATURE_TYPE_SECP256K1_COMETBFT, SIGNATURE_TYPE_SECP256K1_PERSONAL:
+		return Secp256k1
+	case SIGNATURE_TYPE_ED25519:
+		return Ed25519


This is sorta what we chatted about regarding how the signature field of the transaction will indicate the type of the pubkey in the Sender field. Thus for purposes of making (de)serialization sane and straightforward, it seems to me that the Transaction struct can have Sender []byte // pubkey.

When it comes time to Verify:

// Verify verifies the signature of the transaction func (t *Transaction) Verify() error { data, err := t.Body.MarshalBinary() if err != nil { return err } var pubkey crypto.PublicKey switch t.Signature.Type.KeyType() { case crypto.Secp256k1: pubkey = Secp256k1PublicKeyFromBytes(...) case crypto.Secp256k1: pubkey = Ed25519PublicKeyFromBytes(...) default errors.New("invalid key type") } return t.Signature.Verify(pubkey, data) }

Will look at the bigger picture, but that would keep serialization simple.

Its a little inefficient if there are repeated verifications...

cool, will change Sender type

jchappelow · 2023-08-17T15:40:31Z

pkg/crypto/signer.go

+func (c *ComebftSecp256k1Signer) SignMsg(msg []byte) (*Signature, error) {
+	hash := Sha256(msg)
+	sig, err := c.key.Sign(hash)
+	if err != nil {
+		return nil, err
+	}
+	return &Signature{
+		Signature: sig[:len(sig)-1],
+		Type:      SIGNATURE_TYPE_SECP256K1_COMETBFT,


I really like the Signer approach.

jchappelow · 2023-08-17T15:57:46Z

pkg/crypto/secp256k1.go

 	"crypto/ecdsa"
-
+	"encoding/hex"
+	ethAccount "github.com/ethereum/go-ethereum/accounts"


Unrelated to pkg/crypto, but what do you all think about adopting an import organization convention like this (but without the comments):

import ( // standard library imports "fmt" "math/big" // same repo imports "github.com/kwilteam/kwil-db/pkg/crypto" "github.com/kwilteam/kwil-db/pkg/serialize" "github.com/kwilteam/kwil-db/pkg/utils/random" // third party imports ethAccount "github.com/ethereum/go-ethereum/accounts" // any other groupings we want, like all cometbft or something )

And let gofmt -s do the ordering.

The imports in internal/app/kwild/server/root.go have been a little wild in particular, although they are getting tamer. This also makes it easier to see when something is maybe being imported twice, just with a different name assigned.

Totally agree.
This is sort like python's import convention, but in different order:

Imports should be grouped in the following order:

Standard library imports.

Related third party imports.

Local application/library specific imports.

Which ever order is fine, it's good for readability.

Is linter able to do this?

Is linter able to do this?

Not that I'm aware of, but maybe one linter can be made super picky. Best I know of is that gofmt sorts, but doesn't regroup if there's blank lines separating.

I was looking at a github action for linting now, but wasn't aiming to solve this in particular.

goimports does this actually. Making a task and including in linter.

jchappelow

LGTM. We can try changing the Transactions.Sender field in this PR if you want, but the pkg/crypto changes look good to me, worth getting merged.

* support ed25519 key

Yaiba · 2023-08-17T20:41:24Z

This pr is ready to merge, PrivateKey and PublicKey related changes are done

brennanjl · 2023-08-17T21:24:12Z

pkg/crypto/crypto.go

+		return Ed25519PublicKeyFromBytes(key)
+	default:
+		return nil, fmt.Errorf("invalid key type %s", keyType)
+	}
 }


Seems a little redundant, as well as inconsistent to have all of these here. I feel like it should either just be:

Secp256k1PrivateKeyFromBytes([]byte)
Ed25519PrivateKeyFromBytes([]byte)

OR

PrivateKeyFromBytes(KeyType, []byte)

and so on. I think we could probably even leave hex encoding outside of this library, since it's part of the standard library anyways (I know I'm the one who stubbed this out originally).

I think having a single way of doing it is probably just more clear; my vote is that a function takes the key type and bytes, and returns the responsible public/private key (seems like the most minimal interface to me), but I don't really have a preference. @jchappelow thoughts?

I would personally find it frustrating to have no way to construct a concrete instance of the type you may want.

Ok. I guess we can leave as is

Thanks. Even the wonky cometbft/crypto packages let you use the types directly if you don't want to use the generic interface way. Although they are usable directly because they are just underlying []byte rather than e.g. and ecdsa type. We're more efficient; they have to continually reparse things whenever they sign or verify, which is nuts.

I think PrivateKeyFromBytes is more general, but it will be called only by caller that knows what the key format is. Plus, we don't know what future Key parsing interface will be, but definitely will have a interface for parsing from supported format(which is string).

I think PrivateKeyFromBytes is more general, but it will be called only by caller that knows what the key format is. Plus, we don't know what future Key parsing interface will be, but definitely will have a interface for parsing from supported format(which is string).

Well i was wrong, an example is ed25519, it's key only cares about bytes

brennanjl · 2023-08-17T21:28:05Z

pkg/crypto/secp256k1.go

+	return &Signature{
+		Signature: sig,
+		Type:      SIGNATURE_TYPE_SECP256K1_PERSONAL,
+	}, nil
 }


I still think having a type called SignMsg that signs the Ethereum specific digest is weird. I feel like a user should be specifying the Ethereum digest using some sort of enumerator. It doesn't seem immediately apparent from the methods Sign and SignMsg which one I should use by default.

I thought the plan was to make a Signer for this specific digest like ComebftSecp256k1Signer but called EthSecp256k1Signer.

Maybe put SignMsg in a dedicated PersonalSigner? I put this here so it's the default.

And signer.SignMsg for raw message, privateKey.Sign for whatever is required by underlying library

Well, I felt like that was the nice thing that you accomplished with the Signer interface, which is to separate the core crypto from the weird variations and the quirks of the application (e.g. "Ethereum Signed Message" data prefix and keccak hash even though it's not like Ethereum invented secp256k1 keys and signatures).

Oh well I guess you still get the Sign method. 😊 Nah this all makes sense, I was indeed thrown off by the key type being the a signer as well.

brennanjl · 2023-08-17T21:29:20Z

pkg/crypto/secp256k1.go

+		return errVerifySignatureFailed
+	}
+
+	return nil


Why does this verify personal sign? Personal sign is only one of the digests we need to support. I feel like this should be more general, with the personal sign being a signature option

Oh I forget to change the comment, this verify function only verify against given data, doesn't know whether it's personal_sign or eip712

sonarcloud · 2023-08-17T21:55:56Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
13 Code Smells

No Coverage information
0.0% Duplication

brennanjl

lgtm

* feat: support secp256k1 key * support ed25519 key (#191) * support ed25519 key * chore: use Signer instead of PrivateKey * change signature type to string in API * change `Transaction.Sender` to `[]byte` * chore: run linter * chore: update func docs

Yaiba requested review from brennanjl and jchappelow August 15, 2023 19:33

Yaiba marked this pull request as ready for review August 15, 2023 19:35

Yaiba requested a review from charithabandi August 15, 2023 19:35

Yaiba added the enhancement New feature or request label Aug 15, 2023

jchappelow reviewed Aug 15, 2023

View reviewed changes

pkg/crypto/secp256k1.go Outdated Show resolved Hide resolved

jchappelow reviewed Aug 15, 2023

View reviewed changes

pkg/crypto/secp256k1.go Outdated Show resolved Hide resolved

jchappelow reviewed Aug 15, 2023

View reviewed changes

pkg/crypto/secp256k1.go Outdated Show resolved Hide resolved

brennanjl reviewed Aug 16, 2023

View reviewed changes

Yaiba force-pushed the feature/eth-secp256k1-key branch from d1dc77f to 1cfcbe1 Compare August 16, 2023 19:16

Yaiba changed the title ~~feat: support eth secp256k1 key~~ feat: support secp256k1 key Aug 16, 2023

Yaiba requested review from brennanjl and jchappelow August 16, 2023 23:28

jchappelow reviewed Aug 17, 2023

View reviewed changes

jchappelow previously approved these changes Aug 17, 2023

View reviewed changes

Yaiba mentioned this pull request Aug 17, 2023

chore: update signature types kwilteam/proto#4

Merged

Yaiba changed the title ~~feat: support secp256k1 key~~ [WIP] feat: support secp256k1 key Aug 17, 2023

Yaiba dismissed jchappelow’s stale review via d854d0d August 17, 2023 19:40

Yaiba added 6 commits August 17, 2023 14:48

feat: support secp256k1 key

3b5770e

support ed25519 key (#191)

663d94a

* support ed25519 key

chore: use Signer instead of PrivateKey

b22b2d3

change signature type to string in API

3c01893

change Transaction.Sender to []byte

7f0ffe1

chore: run linter

9193965

Yaiba force-pushed the feature/eth-secp256k1-key branch from d854d0d to 9193965 Compare August 17, 2023 20:25

Yaiba changed the title ~~[WIP] feat: support secp256k1 key~~ feat: support secp256k1 key Aug 17, 2023

brennanjl reviewed Aug 17, 2023

View reviewed changes

chore: update func docs

1e75d34

Yaiba self-assigned this Aug 17, 2023

brennanjl approved these changes Aug 17, 2023

View reviewed changes

brennanjl merged commit 16ecde8 into main Aug 17, 2023
2 of 3 checks passed

brennanjl deleted the feature/eth-secp256k1-key branch August 17, 2023 22:41

jchappelow mentioned this pull request Aug 25, 2023

Integrate Kwild and CometBft configs through a TOML file #220

Merged

jchappelow added this to the v0.6.0 milestone Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support secp256k1 key #187

feat: support secp256k1 key #187

Yaiba commented Aug 15, 2023 •

edited

Loading

Yaiba commented Aug 15, 2023

brennanjl left a comment

jchappelow commented Aug 16, 2023

jchappelow commented Aug 16, 2023 •

edited

Loading

jchappelow Aug 17, 2023 •

edited

Loading

Yaiba Aug 17, 2023

jchappelow Aug 17, 2023

jchappelow Aug 17, 2023

Yaiba Aug 17, 2023

jchappelow Aug 17, 2023

jchappelow Aug 17, 2023

jchappelow left a comment

Yaiba commented Aug 17, 2023

brennanjl Aug 17, 2023

jchappelow Aug 17, 2023

brennanjl Aug 17, 2023

jchappelow Aug 17, 2023

Yaiba Aug 17, 2023

Yaiba Aug 17, 2023

brennanjl Aug 17, 2023

jchappelow Aug 17, 2023

Yaiba Aug 17, 2023

jchappelow Aug 17, 2023 •

edited

Loading

jchappelow Aug 17, 2023

brennanjl Aug 17, 2023

Yaiba Aug 17, 2023 •

edited

Loading

sonarcloud bot commented Aug 17, 2023

brennanjl left a comment

feat: support secp256k1 key #187

feat: support secp256k1 key #187

Conversation

Yaiba commented Aug 15, 2023 • edited Loading

Yaiba commented Aug 15, 2023

brennanjl left a comment

Choose a reason for hiding this comment

jchappelow commented Aug 16, 2023

jchappelow commented Aug 16, 2023 • edited Loading

jchappelow Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow left a comment

Choose a reason for hiding this comment

Yaiba commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaiba Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Aug 17, 2023

brennanjl left a comment

Choose a reason for hiding this comment

Yaiba commented Aug 15, 2023 •

edited

Loading

jchappelow commented Aug 16, 2023 •

edited

Loading

jchappelow Aug 17, 2023 •

edited

Loading

jchappelow Aug 17, 2023 •

edited

Loading

Yaiba Aug 17, 2023 •

edited

Loading