custom key types #1254

jchappelow · 2025-01-20T22:37:05Z

There is now a type core/crypto.KeyDefinition that is registered with core/crypto.RegisterKeyType`. An arbitrary external key type can be supported with basic capabilities defined, including unmarshal/generate/type. The main purpose is to recognize key types in various config, DBs, and network comms, and provide the ability to unmarshal and create these keys.

type KeyDefinition interface {
	Type() KeyType      // name of key type e.g. "secp256k1"
	EncodeFlag() uint32 // prefix for compact unique binary encoding

	UnmarshalPrivateKey(b []byte) (PrivateKey, error)
	UnmarshalPublicKey(b []byte) (PublicKey, error)

	Generate() PrivateKey
}

The package level core/crypto.UnmarshalPrivateKey (and one for public) dispatch to the registered implementation based on KeyType. Note that the interfaces PrivateKey and PublicKey include the crypto.Key interface, which has its own marshal method (Bytes()).

KeyType is a string now to make it self describing since we can't really have methods like Valid and String anymore now that the key type registry is not in core.

Regarding the weird new EncodeFlag, this is for node internal use to make compact serializations. See core/crypto.WireEncodeKey etc. Its seemed silly to encode a string key type in non-user-facing places like storage, network transmission, hashing, etc. So this does create a second value that must be unique to register, but I don't think it's a problem.

common/context.go

jchappelow · 2025-01-21T15:25:23Z

This needs a lot more polish in some particularly rough spots, and there are now conflicts with main, but otherwise ready for review.

brennanjl · 2025-01-21T16:05:35Z

cmd/kwil-cli/cmds/account/balance.go

-					typeStr = args[1]
+				keyType := crypto.KeyTypeSecp256k1
+				if len(args) > 1 {
+					// keyType, err = authExt.ParseKeyType(args[1]) // ok to use extns in here?


I think you came to this conclusion / it is now a non-issue, but probably not ok to use exts here since it is kwil-cli

I'm not actually sure why anymore though. Should we strive to keep a vanilla kwil-cli build workable for all kwild chains no matter what extensions are used on the nodes? i.e. use strings and other POD types in the client and simply let node interpret based on its own extension-based capabilities?

That's been the approach so far. I see two good options:

We keep a vanilla kwil-cli that works with any extended node

We allow kwil-cli to access extensions, and we bundle kwil-cli, kwil-admin, and kwild.

So far, it seems like keeping a vanilla kwil-cli is fine though. There are two places where it gets awkward and extensions don't work:

If someone wants to use a custom key type, they cannot (IMO seems like nbd)

If somebody wants to use a precompile within their tests (idOS ran into this, I have another solution I am kicking around though)

brennanjl · 2025-01-21T16:13:26Z

Will have to just incrementally review b/c I cannot partake in discussion and review at the same time (Github doesn't allow you)

jchappelow · 2025-01-21T18:46:50Z

Minimal PoC:

$  go build -tags rsa2048_kwil_key

$  ./kwild key gen rsa2048
Key type: rsa2048
Private key (PKCS #1, ASN.1 DER base64): 
-----BEGIN RSA PRIVATE KEY-----
MIIEpQIBAAKCAQEArqlw9Uf52xlWICuoubiBm/yxVrgr3pM5P3e19rUP2pF65Os8vzMcavNDoOhOqLzr6WwDeXX5LA56+1u6QI9ugVY3kYq1eVAPndA/uthpfi1wo6eQPZqBabx2xIOFi/jdZt2QsyZLtff1fboXHZRCzGeGfKhKLikAGkMe1dpDacK4B05/8MExuWgoCYjk0FeyVbYsts3cPRK3glv6uCwgCWh4EDzZykCvJ6MxXZUXWojAPr9jSWq2ajCXj1zNWlfBhPse9XuHagvYlXi1759YbzHWTzmx0sHcBGrdQU7pxnEG48iwDG0LXFLpXGpwEgNi0KYYuvWGGX1kEq2biDsCdQIDAQABAoIBAEbxJaNKz85RNS0t7oM40NQFyBX8Cj2GONtxiSQimMTjEDKI5YyXevRpyE9D7CcXTQ7rQ2IDK+jcHNGZiUYNvAmmdj2URG94uE8WNbxziC2q0TR3q9uJtQS4etkIvqaCt5K/bbWGWXwYscqWWis2fAdACVz6kd674oHIP28HDxiWRQKIR3k9eaeyZ2e37LUrCEmX1IcuoOfyzghRZFIrE1slcXXmT4ylOf4hSYFSNUnDiqGxodW5AL4cJhx+xAxUZ2hsTRX545cKSyQVoSQV4AjH4uquFUjSlUMMW+Wv7KYwQF3l5qln8FIhSKADJ+cJHsqNmplR5nd5toSdsEQ6ggECgYEAwrEu4M/c3ZqjzGYUiLrjSVyHUosJbtNsvlrPdsLU4PsMc7FSuPvYNKPPu8Md1tx3+qG3KowavnVnAYWQz3uJMadwxpvpD63lT5+b/Df/88pedOLwTuXxz/IFOYKiUwhGggvrXaZBA3cfkGdf3EDPwxioCHBoXYXoP5FdllUgZPUCgYEA5amMxFx7AwHUDV9j98kFEUSoWFRDhv+cqfwOTYxsTYwMZm8XxtGTB0wtojim8oQ7VXSZUhsya4tMKoptjiWCFDgFkLatKNr+dVtftZ0M2GFkBqj9iTZFIrlRDzJ+52kmrSNHrEm84MMmYFI0hNACR55oKywIAD0GsV1tyWGpt4ECgYEAlm/ymSQEtfdXugjmtQAzv+wPZGiq0UftLulchZfmhjF2Xa8XDHkNhx9M1Pjfhba1vqRWtDmuMJznCvX5/1ilmgiXy9+NJuBn4C6kiS98RxSICCmTJmPnhZatEtMuOpqaYMJ1C9f7RII3kY1PNRGfeFEFBgBVpG5kFzpr5k3AykECgYEA2/yk0hKj4iWGMakF4Pu01u2lM7bDWDTQpCLEsJOpyj40Qop3hAKLi5n0vEb346W2EA04L+XNTiaINteSSW60QxnGevAAvfnHdDKqYxSkkT/4yEhnU1+gqbpJYv1Dgfa/DWmV5fpb6hAEWo9jrzRwepnJvlDW+SN7nbmYyCW8pAECgYEAraeRCMiTiixjauzGkm0gGbSilVHV4Mqh/ln1+mjm9p2xhadQiw4msi26t1qGDJPYxeFxc63egIC57hB1BxYymMtER+Xvi0DpLyP4BXJj8kZdr7lgF9Ll5DtaEfo8R7J79R58B3KprLokfYQME7Y82fBsQBSUF/aXamK/6qgHnoc=
-----END RSA PRIVATE KEY-----
Public key (plain hex): 0100010000000000aea970f547f9db1956202ba8b9b8819bfcb156b82bde93393f77b5f6b50fda917ae4eb3cbf331c6af343a0e84ea8bcebe96c037975f92c0e7afb5bba408f6e815637918ab579500f9dd03fbad8697e2d70a3a7903d9a8169bc76c483858bf8dd66dd90b3264bb5f7f57dba171d9442cc67867ca84a2e29001a431ed5da4369c2b8074e7ff0c131b968280988e4d057b255b62cb6cddc3d12b7825bfab82c20096878103cd9ca40af27a3315d95175a88c03ebf63496ab66a30978f5ccd5a57c184fb1ef57b876a0bd89578b5ef9f586f31d64f39b1d2c1dc046add414ee9c67106e3c8b00c6d0b5c52e95c6a70120362d0a618baf586197d6412ad9b883b0275

brennanjl · 2025-01-22T05:21:03Z

Apologies, I got caught up in an insane amount of bugs and required fixes for what seemed like it would've been the last .1% of a PR.

Getting to this now, will get a review tonight

brennanjl

Aside from a few comments lgtm

brennanjl · 2025-01-22T05:24:45Z

core/crypto/keytype.go

+)
+
+// KeyType is the type of key, which may be public or private depending on context.
+type KeyType string // uint32


seems like the // uint32 comment is not relevant anymore

brennanjl · 2025-01-22T05:35:06Z

cmd/kwil-cli/cmds/account/balance.go

-					typeStr = crypto.KeyTypeSecp256k1.String()
-				} else {
-					typeStr = args[1]
+				keyType := crypto.KeyTypeSecp256k1


Might be worth handling in another PR, and I know we've already discussed this, but we need to be able to recognize 0x addresses for balance, transfer, account, etc.

Also, do you think key type should be a flag? Not super strongly held, just feel like it maybe makes more sense because it is optional. (I know it was added in a previous PR, just thought I'd bring it up before people get their hands on it b/c it was not in v0.9)

Indeed. Done.

Also, do you think key type should be a flag? Not super strongly held, just feel like it maybe makes more sense because it is optional. (I know it was added in a previous PR, just thought I'd bring it up before people get their hands on it b/c it was not in v0.9)

Ah, on this for some reason an optional arg felt better. I do think that required inputs should be args not flags, but the reverse for optional inputs being flags isn't typically my rule. It could create difficulty though if we end up adding a second required arg, so I'll change it to a flag to future proof.

Also special-cased the output of the account balance command:

0xDC18F4993e93b50486e3e54e27d91D57cEE1dA07 (Ethereum secp256k1) Balance: 0 Nonce: 0

Otherwise it is the hex compact id with keytype as usual.

oh lord I forgot that we have a nil accountID set from the get account RPC response if the account isn't found. Adjusted... b06239c

brennanjl · 2025-01-22T05:39:10Z

core/crypto/auth/auth.go

+	// correspond to crypto.KeyTypeSecp256k1. This information is important to
+	// determine the key type of a transaction Sender from the AuthType in the
+	// signature.
+	KeyType() crypto.KeyType


I noticed that this Authenticator interface in core/crypto/auth is only ever referenced in core within tests. Do you think we should move this to the main module?

Good observation. I think it could. Will check to see if there's a coupling between Signer and Authenticator that might prevent this.

brennanjl · 2025-01-22T05:44:23Z

node/block_processor/transactions.go

+	}
+
+	return authn.Verify(tx.Sender, msg, tx.Signature.Data)
+}


Doesn't really matter, but you have a function in the authExt package that wraps these:

func verifyTransaction(tx *types.Transaction) error { msg, err := tx.SerializeMsg() if err != nil { return err } return authExt.VerifySignature(tx.Sender, msg, tx.Signature) }

Yeah, I was over-optimizing here probably. Wanted to skip the SerializeMsg if there was no authenticator found. Will just use the helper.

There is now a type core/crypto.KeyDefinition that is registered with core/crypto.RegisterKeyType. An arbitrary external key type can be supported with basic capabilities defined, including unmarshal/generate/type. The main purpose is to recognize key types in various config, DBs, and network comms, and provide the ability to unmarshal and create these keys. type KeyDefinition interface { Type() KeyType // name of key type e.g. "secp256k1" EncodeFlag() uint32 // prefix for compact unique binary encoding UnmarshalPrivateKey(b []byte) (PrivateKey, error) UnmarshalPublicKey(b []byte) (PublicKey, error) Generate() PrivateKey } The package level core/crypto.UnmarshalPrivateKey (and one for public) dispatch to the registered implementation based on KeyType. Note that the interfaces PrivateKey and PublicKey include the crypto.Key interface, which has its own marshal method (Bytes()). KeyType is a string now to make it self describing since we can't really have methods like Valid and String anymore now that the key type registry is not in core. Regarding the weird new EncodeFlag, this is for node internal use to make compact serializations. See core/crypto.WireEncodeKey etc. It seemed silly to encode a string key type in non-user-facing places like storage, network transmission, hashing, etc. So this does create a second value that must be unique to register, but I don't think it's a problem. permit eth address in account balance,transfer commands eth addr in account balance output

jchappelow mentioned this pull request Jan 21, 2025

Cli act tests #1260

Merged

jchappelow commented Jan 21, 2025

View reviewed changes

common/context.go Outdated Show resolved Hide resolved

This was referenced Jan 21, 2025

kwild setup init should give info on allocs #1258

Closed

incorrect allocs flag causes unexpected nil pointer dereference #1261

Closed

jchappelow force-pushed the keys-extn branch from caf95f5 to 61c6c71 Compare January 21, 2025 16:01

brennanjl reviewed Jan 21, 2025

View reviewed changes

jchappelow force-pushed the keys-extn branch 2 times, most recently from fd78975 to 6437169 Compare January 21, 2025 16:29

jchappelow marked this pull request as ready for review January 21, 2025 16:31

jchappelow linked an issue Jan 21, 2025 that may be closed by this pull request

incorrect allocs flag causes unexpected nil pointer dereference #1261

Closed

jchappelow force-pushed the keys-extn branch 2 times, most recently from b643ba9 to c74e8b8 Compare January 21, 2025 22:52

brennanjl previously approved these changes Jan 22, 2025

View reviewed changes

jchappelow dismissed brennanjl’s stale review via 80468df January 22, 2025 17:39

jchappelow force-pushed the keys-extn branch 2 times, most recently from b06239c to 40c210b Compare January 22, 2025 22:31

charithabandi previously approved these changes Jan 22, 2025

View reviewed changes

jchappelow dismissed charithabandi’s stale review via c27631e January 22, 2025 22:36

jchappelow force-pushed the keys-extn branch from 40c210b to c27631e Compare January 22, 2025 22:36

charithabandi approved these changes Jan 22, 2025

View reviewed changes

jchappelow merged commit 8daebee into kwilteam:main Jan 22, 2025
2 checks passed

jchappelow deleted the keys-extn branch January 22, 2025 22:41

This was referenced Jan 22, 2025

Bug: kwil-cli account balance not working #1277

Closed

kwil-cli account transfer recipient does not work as expected #1268

Closed

Genesis config allocs does not accept Ethereum addresses #1259

Closed

charithabandi linked an issue Jan 23, 2025 that may be closed by this pull request

kwil-cli account transfer recipient does not work as expected #1268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

custom key types #1254

custom key types #1254

jchappelow commented Jan 20, 2025 •

edited

Loading

jchappelow commented Jan 21, 2025

brennanjl Jan 21, 2025

jchappelow Jan 21, 2025

brennanjl Jan 21, 2025

brennanjl commented Jan 21, 2025

jchappelow commented Jan 21, 2025

brennanjl commented Jan 22, 2025

brennanjl left a comment

brennanjl Jan 22, 2025

brennanjl Jan 22, 2025

jchappelow Jan 22, 2025

jchappelow Jan 22, 2025

jchappelow Jan 22, 2025

jchappelow Jan 22, 2025

brennanjl Jan 22, 2025

jchappelow Jan 22, 2025

brennanjl Jan 22, 2025

jchappelow Jan 22, 2025

custom key types #1254

custom key types #1254

Conversation

jchappelow commented Jan 20, 2025 • edited Loading

jchappelow commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brennanjl commented Jan 21, 2025

jchappelow commented Jan 21, 2025

brennanjl commented Jan 22, 2025

brennanjl left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow commented Jan 20, 2025 •

edited

Loading