refactored crypto, addresses, and the engines auth to be better pluggable with custom / new auth #335

brennanjl · 2023-10-03T05:47:10Z

This PR is meant to simplify our supported authentication types.

Overview

The tldr of this PR is that it better delineates (imo) the responsibilities of pkg/crypto, pkg/crypto/addresses (now gone), and how we handle authentication in the engine. It introduces a new package pkg/auth, which handles the semantics of signature digests. The intent of this is to make for more "pluggable" auth, whether it be us, or another party implementing an auth driver (as discussed in Slack).

What I think is pretty exciting about this is that it makes it trivially easy to implement many types of authentication. It would be dead simple to add, for example, RSA signatures into kwild (we don't actually want to do that, but it would be very easy).

`pkg/auth`

I'm still not quite sure on this package name.

This package defines two very clear interfaces for signing and verification. The intent here is that users can implement their own signature verification, and we support a standard set of signing methods. To implement new signature verification logic, all a user needs to do is implement the following interface:

type Authenticator interface {
	// Verify verifies the signature against the given public key and data.
	Verify(sender, msg, signature []byte) error

	// Address returns an address from a public key
	Address(sender []byte) (string, error)
}

The user can then register that Authenticator with a unique id, which will then be registered as a driver. This works very similar to Go's database/sql drivers.

The Signer interface and Signature struct from pkg/crypto was moved here. This is because the specifics of the signature algorithm that we are using are higher level than what pkg/crypto handles now.

pkg/crypto/addresses was also moved into pkg/auth, however ~70% was actually just deleted, since the system is quite a bit simpler and didn't need all of it.

Changes to `pkg/crypto`

This pr really simplifies pkg/crypto. Previously, we had support for all different types of signers implemented there, as well as logic to switch and validate the signatures generated by them. Now, we only implement basic secp256k1 and ed25519 keypairs there, and enforce any other logic in pkg/auth.

The one hangup I'm not quite sure on is go-ethereum's recovery ID. As far as I can tell, it is not technically "standard" secp256k1, but both Ethereum and Bitcoin use it, and it is so widely adopted that it is actually hard to find libraries that do not contain it. CometBFT, however, does not use it. I left it to pkg/crypto for now, but I feel like it probably should live in pkg/auth.

One of the other big changes in pkg/crypto is that we no longer have semantics like KeyType, the PrivateKey interface, the PublicKey interface, and the Address interface. This wasn't really intentional, but by the time I had made all of the other changes, they actually were just not needed anymore.

Changes to `pkg/crypto/addresses`

This entire package got deleted. All of the KeyIdentifier logic was totally removed, and the address derivation was moved to pkg/auth.

Changes to the engine

Previously, the engine relied on pkg/crypto/addresses/KeyIdentifier for user authentication (within the master db, in order to unmarshal). I always knew this was weird, but due to the rush we were in previously, kept it. The engine now defines it's own type for identifying users.

The one thing I really do not like still is that pkg/sql/sqlite/functions/addresses is very coupled to both the engine as well as pkg/auth. This now makes me think that we should not actually support public_key() and address() scalar functions, and instead we should revert to the @caller and @caller_address semantic. Obviously, this would require getting Fractal onboard with a slight breaking change, so I punted on it for now.

Other misc changes

A lot of other miscellaneous changes were made, mostly due to how prevalent pkg/crypto is in our system. Some of these include:

abci logging, where we were logging the caller's wallet address (which actually doesn't feel right, since we are public key centric)
kwil-admin and kwil-cli, which relied on the pkg/crypto/PrivateKey interface. This change was really minor, since we only support secp256k1 in kwil-cli, and ed25519 in kwil-admin.
slightly altering pkg/transactions to use the new pkg/auth.
modifying pkg/modules/dataset. The new auth from both pkg/auth as well as the new way the engine identifies "users" necessitated an obvious change, but it does feel a lot cleaner. It also got rid of an unnecessary interface that decoupled pkg/transactions/CallMessage. I felt this was justifiable since the module is heavily reliant on pkg/transactions/Transaction, and the interface was really only used in once place and had one implementation. I still think something along those lines is probably appropriate, but it felt outside the scope of this PR, and if we are going to include it, then we should fully decouple pkg/transactions
And other minor changes for conversions, tests, etc.

…able with custom / new auth

Yaiba · 2023-10-03T15:57:25Z

pkg/crypto/secp256k1.go

@@ -79,33 +78,17 @@ func (pub *Secp256k1PublicKey) Verify(sig []byte, hash []byte) error {
 		sig = sig[:len(sig)-1]
 	}

-	if len(sig) != 64 {
-		return errInvalidSignature
+	if len(sig) != secp256k1SignatureLength {


Maybe also change the line above using 65

Yaiba · 2023-10-03T15:59:52Z

pkg/crypto/secp256k1_test.go

@@ -12,13 +15,13 @@ import (

 func TestSecp256k1PrivateKey_Sign(t *testing.T) {


Should change the test name since you're not testing Sign method

Actually should two separated test functions for Sign and SignWithRecoveryID

Yaiba · 2023-10-03T16:05:49Z

pkg/crypto/secp256k1_test.go

-			err := pubKey.Verify(tt.sigBytes, hash)
-			assert.ErrorIs(t, err, tt.wantErr)
+			err := pubKey.Verify(tt.sigBytes, hash[:])
+			if tt.wantErr != nil {


assert.ErrorIs should work here, no?

Yaiba · 2023-10-03T16:31:37Z

test/driver/client_driver.go

@@ -126,7 +126,7 @@ func (d *KwildClientDriver) DropDatabase(ctx context.Context, dbName string) ([]
 		return nil, fmt.Errorf("error dropping database: %w", err)
 	}

-	d.logger.Info("drop database", zap.String("name", dbName), zap.String("owner", d.GetUserAddress()),
+	d.logger.Info("drop database", zap.String("name", dbName), zap.Binary("owner", d.GetUserPublicKey()),


Will zap.Binary will use HEX encoding?

Not sure, will test it. If not, I will change it to fmt.Sprintf("%x", d.GetUserPublicKey())

It encodes as base64, which I think is fine

I think we kind of agree on using HEX everywhere for PublicKey

Right. Ok I'll go back and change that

Yaiba · 2023-10-03T17:00:09Z

pkg/auth/defaults.go

So authenticator is only for verification, I'm not exactly sure what kind of different validation logic could be applied except what we included. I mean it seems only one way to 'correctly' verify the signature?

A observation is that we already verified TX or Message when it's received by a Kwil node, also in CheckTx, in engine we verified again. This seems redundant, and also, because this verification logic is share by blockchain and engine, we kind allow 3rd part to change the concensus (although engine is also concensus).

Just a thought, would it be useful if we can allow another signature in the payload, and the smartcontract(e.g. kuneiform) could use separeted verification logic to verify. So we only give out the ability to do different verification in engine(or kuneiform), but not for Tx or Message.

But then you need a separate wallet to forward messages to the blockchain, and actually pay the gas. That seems like a hack around the system, but if our most important users are hacking around a system this early in its product lifecycle, then it probably isn't made well.

The entire point of this is that other devs can implement their own logic easily. A great example is NEAR's new signer, which uses their own custom logic for the signing.

The authenticators used here are exactly what would be used in CheckTx; if we implemented another and registered it, it would be globally available.

I just realized in js-sdk we allow developer pass a customized Signer, now this make sense to me, since developer can use their pair of signing and verifying, correct? I doesn't make sense to me to use our Signer and their customized Authenticator.

Yeah this another signature is weird, we don't need it.

But still, I think verify once when Tx/Message is received from the node is enough? Not sure why we did multiple times

This doesn't change the point in which signatures get checked. That is all consistent for what was before this PR and after. The only difference is how we separate the logic. Nowhere are there any extra signature checks that we did not do before.

Yeah I agree, verified multiple times is not related to this change.

So it seems that the only places that we verify transactions are when we receive them over gRPC and in CheckTx, which is indeed redundant since we broadcast synchronously. We can probably take it out in a different PR.

Yeah, also, CallMessage are verified in both gRPC and dataset module.

jchappelow · 2023-10-03T18:56:52Z

pkg/auth/defaults.go

+// constants for eth personal sign
+const (
+	// ethAuth is the authenticator name
+	EthAuth = "secp256k1_ep"


The registration system makes sense, as does the Verify method of the Authenticator interface. Just thinking through usability a bit here...

These exported strings are used for the registered authenticator name, and they are the values set for Signature.Type. Except in tests, only the signer implementations really have to know of these const values e.g. EthPersonalSigner makes a sig with Type: EthAuth.

The current (*Transaction).Verify, as an example use case:

func (t *Transaction) Verify() error { msg, err := t.Body.SerializeMsg(t.Serialization) if err != nil { return err } authenticator, err := auth.GetAuthenticator(t.Signature.Type) if err != nil { return err } return authenticator.Verify(t.Sender, msg, t.Signature.Signature) }

I like that we don't have to trouble with crypto.PublicKeyFromBytes any more, but it looks like because the Signature.Type determines what verification code to use then a consumer like this Transaction method does not have to be concerned with the authenticator registry or authenticators at all.

The type is pkg/auth.Signature, same package as the registry, so :

pkg/auth/signer.go

func (sig *Signature) Verify(msg, pubkey []byte) error { a, err := GetAuthenticator(sig.Type) if err != nil { return err } return a.Verify(pubkey, msg, sig.Signature) }

pkg/transactions/transaction.go

func (t *Transaction) Verify() error { msg, err := t.Body.SerializeMsg(t.Serialization) if err != nil { return err } return t.Signature.Verify(msg, t.Sender) }

I think the win with the Authenticator interface and the driver registry is flexibility in expanding the authentication / verification capabilities of kwild, but I'm not sure much of the kwild code has to directly access the registry in the case of signature verification.

Still looking.

Ah yeah that makes a lot of sense. I'll make that change.

It still is used for Address(), particularly in the pkg/sql/sqlite/functions/addresses package. I don't love this, but not quite sure of a workaround.

jchappelow · 2023-10-03T19:08:59Z

pkg/sql/sqlite/functions/addresses/addresses.go

+	authenticator, err := auth.GetAuthenticator(ident.AuthType)
+	if err != nil {
+		return raiseErr(addressFuncName, fmt.Errorf("failed to get authenticator: %w", err))
+	}
+
+	address, err := authenticator.Address(ident.PublicKey)


The second general purpose of the Authenticator interface is to provide an address from a pubkey. Starting to feel a bit like the Authenticator concept is mainly of concern to the implementor of a new signature scheme, but for the kwild code, it's concern is with one function or the other.

So as with my comment on signature verification, do we trouble this caller with the Authenticator interface or just let them do auth.Address(ident.PublicKey, ident.AuthType).

I'm looking at this wondering what is gained from having the public GetAuthenticator function vs accessing the registry indirectly via the specific types and functions provided by the auth package.

Yeah that makes a lot more sense. Another area I was sort've hung up on was having Address be tied to the Authenticator. For the user of Kwil, I think it makes sense; people would use this to create a new way to authenticate users that use some existing standard, and that usually also involves a unique way of identifying those users.

Within kwild, a bit of a different beast, so I'm not totally tied to this structure.

jchappelow

Cool, just some tiny nits. Haven't manually tested either, but acceptance tests are working.

jchappelow · 2023-10-03T20:28:48Z

pkg/auth/auth.go

@@ -0,0 +1,81 @@
+package auth


A package doc would be valuable in the future. Accepted format is // Package auth ... just above package auth. https://go.dev/doc/comment

jchappelow · 2023-10-03T20:29:38Z

pkg/auth/auth.go

+// ListAuthenticators returns a list of registered authenticators
+func ListAuthenticators() []struct {
+	Name          string
+	Authenticator Authenticator
+} {
+	var authenticators []struct {
+		Name          string
+		Authenticator Authenticator
+	}


Can probably toast this function too now, but no objection if you like it for debugging or whatever.

jchappelow · 2023-10-03T20:36:24Z

pkg/client/client.go

@@ -65,7 +66,7 @@ func Dial(target string, opts ...Option) (c *Client, err error) {
 		zap.String("host", c.transportClient.GetTarget()),
 	}
 	if c.Signer != nil {
-		zapFields = append(zapFields, zap.String("from", c.Signer.PubKey().Address().String()))
+		zapFields = append(zapFields, zap.String("from", fmt.Sprintf("%x", c.Signer.PublicKey())))


So I think I might have provided a slightly misleading tip about the %x trick a little while ago. It's super handy for logging to embed a hex string for a []byte in a longer message, but there is one kinda nasty gotcha to be aware of, which is that if the type upon which %x is applied is also a Stringer, it takes the very unexpected action of running the String method first and then casting a la []byte(...) before applying hex conversion again. In other words you can end up converting twice if the type happens to have a String method.

For this reason, it's probably best to limit it's use when the value is from a function-local variable of type []byte (not returned by a method of something from another package that could add a String method at any time breaking the log) and when the entirety of the msg is more than just %x.

Sorry about that.

That's a weird quirk. Noted

jchappelow · 2023-10-03T20:38:19Z

pkg/transactions/transaction.go

+// func (t *Transaction) GetSenderPubKey() (crypto.PublicKey, error) {
+// 	return crypto.PublicKeyFromBytes(t.Signature.KeyType(), t.Sender)
+// }


jchappelow · 2023-10-03T20:38:56Z

pkg/transactions/transaction.go

@@ -121,7 +122,8 @@ func CreateTransaction(contents Payload, nonce uint64) (*Transaction, error) {
 type Transaction struct {
 	// Signature is the signature of the transaction
 	// It can be nil if the transaction is unsigned
-	Signature *crypto.Signature
+	// This should probably be renamed to "Authentication"


meh, but I feel like signatures and transactions are a very tight knit pair and it would be confusing as "Authentication".

jchappelow · 2023-10-03T20:40:09Z

pkg/transactions/message_test.go

+		// {
+		// 	name: "non support message serialization type",
+		// 	args: args{
+		// 		mst:    transactions.SignedMsgSerializationType("non support message serialization type"),
+		// 		signer: &ethPersonalSigner,
+		// 	},
+		// 	wantErr: true,
+		// },


temporarily commented?

jchappelow · 2023-10-03T20:40:57Z

pkg/engine/types/user.go

+// // UserIdentifier is an interface for identifying a user by public key
+// type UserIdentifier interface {
+// 	MarshalBinary() ([]byte, error)
+// 	PubKey() (crypto.PublicKey, error)
+// 	UnmarshalBinary(data []byte) error
+// 	Address() (string, error)
+// }
+
+type User struct {


Fine to rm the old UserIdentifier imo, maybe also a short doc on User.

…able with custom / new auth (#335) * refactored crypto, addresses, and the engines auth to be better pluggable with custom / new auth * added gavins changes * changed public key logging from base64 to hex * better abstracted away the authenticators, as suggested by Jon * added jons changed

refactored crypto, addresses, and the engines auth to be better plugg…

8ef407f

…able with custom / new auth

Yaiba reviewed Oct 3, 2023

View reviewed changes

added gavins changes

d4a9439

jchappelow reviewed Oct 3, 2023

View reviewed changes

changed public key logging from base64 to hex

e110a4e

jchappelow reviewed Oct 3, 2023

View reviewed changes

better abstracted away the authenticators, as suggested by Jon

4ec61cb

brennanjl force-pushed the pluggable-signatures branch from c4a072e to 4ec61cb Compare October 3, 2023 19:41

jchappelow approved these changes Oct 3, 2023

View reviewed changes

added jons changed

71b7e90

brennanjl merged commit a0b3205 into main Oct 3, 2023
2 checks passed

brennanjl deleted the pluggable-signatures branch October 3, 2023 21:09

jchappelow added this to the v0.6.0 milestone Nov 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactored crypto, addresses, and the engines auth to be better pluggable with custom / new auth #335

refactored crypto, addresses, and the engines auth to be better pluggable with custom / new auth #335

brennanjl commented Oct 3, 2023 •

edited

Loading

Yaiba Oct 3, 2023

Yaiba Oct 3, 2023

Yaiba Oct 3, 2023

Yaiba Oct 3, 2023

brennanjl Oct 3, 2023

Yaiba Oct 3, 2023

brennanjl Oct 3, 2023 •

edited

Loading

brennanjl Oct 3, 2023

Yaiba Oct 3, 2023

brennanjl Oct 3, 2023

Yaiba Oct 3, 2023 •

edited

Loading

brennanjl Oct 3, 2023

Yaiba Oct 3, 2023 •

edited

Loading

Yaiba Oct 3, 2023

brennanjl Oct 3, 2023

Yaiba Oct 3, 2023

brennanjl Oct 3, 2023

Yaiba Oct 3, 2023

jchappelow Oct 3, 2023

brennanjl Oct 3, 2023

brennanjl Oct 3, 2023

jchappelow Oct 3, 2023 •

edited

Loading

brennanjl Oct 3, 2023 •

edited

Loading

jchappelow left a comment

jchappelow Oct 3, 2023

jchappelow Oct 3, 2023

jchappelow Oct 3, 2023 •

edited

Loading

brennanjl Oct 3, 2023

jchappelow Oct 3, 2023

jchappelow Oct 3, 2023

jchappelow Oct 3, 2023

jchappelow Oct 3, 2023

		@@ -12,13 +15,13 @@ import (

		func TestSecp256k1PrivateKey_Sign(t *testing.T) {

refactored crypto, addresses, and the engines auth to be better pluggable with custom / new auth #335

refactored crypto, addresses, and the engines auth to be better pluggable with custom / new auth #335

Conversation

brennanjl commented Oct 3, 2023 • edited Loading

Overview

pkg/auth

Changes to pkg/crypto

Changes to pkg/crypto/addresses

Changes to the engine

Other misc changes

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

brennanjl Oct 3, 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

Yaiba Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yaiba Oct 3, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

brennanjl Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

jchappelow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jchappelow Oct 3, 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

brennanjl commented Oct 3, 2023 •

edited

Loading

`pkg/auth`

Changes to `pkg/crypto`

Changes to `pkg/crypto/addresses`

brennanjl Oct 3, 2023 •

edited

Loading

Yaiba Oct 3, 2023 •

edited

Loading

Yaiba Oct 3, 2023 •

edited

Loading

jchappelow Oct 3, 2023 •

edited

Loading

brennanjl Oct 3, 2023 •

edited

Loading

jchappelow Oct 3, 2023 •

edited

Loading