encapsulate signer #3576

richardpringle · 2024-11-27T20:26:27Z

Depends on ava-labs/coreth#693. But ava-labs/coreth#693 also depends on this. Not sure about merge order...

Why this should be merged

Currently, signing happens all over the place and should definitely be abstracted away, especially when dealing with sensitive data like a private key.

Once this abstraction lands, we could further enhance security by using secure hardware and signing outside of the avalanchego application and potentially integrate platforms like cubist.

How this works

Encapsulate signing such that the secret-key does not leave our bls utility package.
The main changes are in this file: utils/crypto/bls/secret.go

How this was tested

CI

Need to be documented in RELEASES.md?

I don't think so?

richardpringle · 2024-11-28T16:20:52Z

go.mod

@@ -190,3 +190,5 @@ require (
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
 	sigs.k8s.io/yaml v1.3.0 // indirect
 )
+
+replace github.com/ava-labs/coreth v0.13.9-rc.1 => github.com/ava-labs/coreth v0.13.9-rc.1.0.20241127225719-c225e43551e1


Not sure what the process is here

richardpringle · 2024-11-28T16:21:32Z

network/config.go

@@ -128,7 +128,7 @@ type Config struct {
 	// TLSKey is this node's TLS key that is used to sign IPs.
 	TLSKey crypto.Signer `json:"-"`
 	// BLSKey is this node's BLS key that is used to sign IPs.
-	BLSKey *bls.SecretKey `json:"-"`
+	BLSKey bls.Signer `json:"-"`


I assume this means the BLSKey property gets serialized as the string, "-"?

IIRC this actually omits it from serialization, i.e it's not serialized or deserialized. This is done because we use the Config struct to also hold fields and not just config values.... so our hack is to ignore serialization for non-config values (which is weird IMO).

Makes sense.

Yeah, I generally prefer to split up raw config data vs actual dependent services/structs, but 🤷. I've seen this pattern before too.

Yup think it makes sense to leave this as is in this PR. Separating it would be an improvement

richardpringle · 2024-11-28T16:23:23Z

chains/manager.go

@@ -186,7 +186,7 @@ type ManagerConfig struct {
 	SybilProtectionEnabled bool
 	StakingTLSSigner       crypto.Signer
 	StakingTLSCert         *staking.Certificate
-	StakingBLSKey          *bls.SecretKey
+	StakingBLSKey          bls.Signer


Do we want to change the property name here too?

richardpringle · 2024-11-28T16:24:40Z

config/config.go

@@ -694,7 +694,7 @@ func getStakingSigner(v *viper.Viper) (*bls.SecretKey, error) {
 		return nil, fmt.Errorf("couldn't create path for signing key at %s: %w", signingKeyPath, err)
 	}

-	keyBytes := bls.SecretKeyToBytes(key)
+	keyBytes := key.ToBytes()


I'll have to change this eventually to fully abstract the signer. I'm open to ideas on how best to do this

Within the config, I'd expect us to have a switch that offers different BLS Signers ie. generate and store a new BLS key on the machine or use an external signing service.

I'd think this is either AvalancheGo defining the options it wants to support directly here or it supports a gRPC service that dials an external signer and handles signature requests.

Yeah, looking at the code again, signer creation would be specific. A LocalSigner should actually write the key to disk before returning from a constructor. We could have an EphemeralSigner that's similar but doesn't save the key and is used for testing.

With that said, I would probably create a

func initializeSigner(signerConfig) (Signer) { /* ... */ }

and deal with saving or not saving there. It should not be dealt with here IMO.

This is all for a future PR though

richardpringle · 2024-11-28T16:26:25Z

network/network_test.go

@@ -175,7 +175,7 @@ func newTestNetwork(t *testing.T, count int) (*testDialer, []*testListener, []id
 		require.NoError(t, err)
 		nodeID := ids.NodeIDFromCert(cert)

-		blsKey, err := bls.NewSecretKey()
+		blsKey, err := bls.NewSigner()


🤔 If we want to change all the *Key names (variables or properties), I would recommend/request that I do that in a follow-up to limit the scope of both changes.

Seems reasonable

yacovm · 2024-11-28T20:15:05Z

Once this abstraction lands, we could further enhance security by using secure hardware and signing outside of the avalanchego application and potentially integrate platforms like cubist.

Do they support BLS? Most hardware security modules only support ECDSA / ed25519...

richardpringle · 2024-11-28T20:16:31Z

Once this abstraction lands, we could further enhance security by using secure hardware and signing outside of the avalanchego application and potentially integrate platforms like cubist.

Do they support BLS? Most hardware security modules only support ECDSA / ed25519...

Yeah, they specifically have support for Ethereum validators

richardpringle · 2024-11-29T18:56:07Z

utils/crypto/bls/secret.go

@@ -25,49 +25,64 @@ var (

 type SecretKey = blst.SecretKey


I should get rid of this type

marun

tests/ changes LGTM

snow/context.go

utils/crypto/bls/secret.go

joshua-kim · 2024-12-03T16:27:41Z

utils/crypto/bls/secret.go

 // NewSecretKey generates a new secret key from the local source of
 // cryptographically secure randomness.
-func NewSecretKey() (*SecretKey, error) {
+func NewSigner() (Signer, error) {


nit: export signer and return it as a concrete type here so that the calling code can decide whether to use it as an interface/as a concrete type. We'll have to figure out a fix/good naming scheme since it'll collide w/ the interface name as it currently is though. We could also as an alternative argue that the interface is premature and just use the struct.

🤔 I don't think the interface is premature though, I don't plan on adding any functionality for the other implementations. What I'm trying to get is encapsulation so that you can't access the secret-key outside of this package. I think want to make a LocalSigner, then some "cloud-based" signers (there are a couple of options here that will each have their own concrete types).

Having it generic from day 1 minimizes the impact of converting what's here to a LocalSigner (which would also be used in all tests), and other signer types would be created based on config values on application start.

Thoughts?

Discussed offline. Change signer to LocalSigner. Will move this out in a follow PR

This reverts commit 3f46a5a.

joshua-kim

Another idea is to cache the key bytes and public key but not going to block on it since it's an increase in scope.

go.mod

Signed-off-by: Richard Pringle <[email protected]>

aaronbuchwald

Could we update to a coreth rc rather than a commit before merging to master? After that, this LGTM

aaronbuchwald · 2024-12-06T19:14:16Z

network/config.go

@@ -128,7 +128,7 @@ type Config struct {
 	// TLSKey is this node's TLS key that is used to sign IPs.
 	TLSKey crypto.Signer `json:"-"`
 	// BLSKey is this node's BLS key that is used to sign IPs.
-	BLSKey *bls.SecretKey `json:"-"`
+	BLSKey bls.Signer `json:"-"`


Yup think it makes sense to leave this as is in this PR. Separating it would be an improvement

aaronbuchwald · 2024-12-06T19:14:35Z

network/network_test.go

@@ -175,7 +175,7 @@ func newTestNetwork(t *testing.T, count int) (*testDialer, []*testListener, []id
 		require.NoError(t, err)
 		nodeID := ids.NodeIDFromCert(cert)

-		blsKey, err := bls.NewSecretKey()
+		blsKey, err := bls.NewSigner()


Seems reasonable

aaronbuchwald · 2024-12-06T19:20:51Z

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/DataDog/zstd v1.5.2
 	github.com/NYTimes/gziphandler v1.1.1
 	github.com/antithesishq/antithesis-sdk-go v0.3.8
-	github.com/ava-labs/coreth v0.13.9-rc.1
+	github.com/ava-labs/coreth v0.13.9-rc.1.0.20241204232641-e73e655356b7


Can we create an rc with a name that indicates what the change is for (ref to previous update of coreth dependency 33bd401 ) ?

aaronbuchwald · 2024-12-06T19:23:06Z

config/config.go

@@ -694,7 +694,7 @@ func getStakingSigner(v *viper.Viper) (*bls.SecretKey, error) {
 		return nil, fmt.Errorf("couldn't create path for signing key at %s: %w", signingKeyPath, err)
 	}

-	keyBytes := bls.SecretKeyToBytes(key)
+	keyBytes := key.ToBytes()


Within the config, I'd expect us to have a switch that offers different BLS Signers ie. generate and store a new BLS key on the machine or use an external signing service.

I'd think this is either AvalancheGo defining the options it wants to support directly here or it supports a gRPC service that dials an external signer and handles signature requests.

StephenButtolph · 2024-12-09T20:39:12Z

utils/crypto/bls/secret.go

 }

 // SecretKeyFromBytes parses the big-endian format of the secret key into a
 // secret key.
-func SecretKeyFromBytes(skBytes []byte) (*SecretKey, error) {
+func SecretKeyFromBytes(skBytes []byte) (Signer, error) {


Suggested change

func SecretKeyFromBytes(skBytes []byte) (Signer, error) {

func SecretKeyFromBytes(skBytes []byte) (*LocalSigner, error) {

done fd7f326

StephenButtolph · 2024-12-09T20:39:40Z

utils/crypto/bls/secret.go

+	// TODO: delete me
+	ToBytes() []byte


If we adopt the suggested change on SecretKeyFromBytes - then this can be deleted now.

Suggested change

// TODO: delete me

ToBytes() []byte

done fd7f326

StephenButtolph · 2024-12-09T20:50:06Z

utils/crypto/bls/secret.go

+	Sign(msg []byte) *Signature
+	SignProofOfPossession(msg []byte) *Signature


I feel like these functions are going to need to return error once we want to use them for remote signing. Are we waiting to add error to the return value for when that other implementation is introduced?

Yeah, I don't think there's any point in pre-maturely defining the interface here. It's unclear to me at this point whether or not we'll want the specific signer implementation to handle its own errors or wether we would want the caller to handle errors (I'm missing some details on how to gracefully shutdown).

I'm interested to hear your thoughts here, but I definitely don't think this is something that we should deal with yet.

Sounds good. I think the only complex question will be around the PoP in the networking code... Everything else should be fairly straightforward I think... But 🤷 we can deal with that when we need to I suppose.

richardpringle force-pushed the encapsulate-signer branch 2 times, most recently from c522086 to fc6bc8d Compare November 27, 2024 23:30

richardpringle added 4 commits November 28, 2024 11:12

Rename NewSecretKey to NewSigner

4b1d0d2

WIP

4177305

Straglers

a3af684

Point to different version of coreth

57c6ec7

richardpringle force-pushed the encapsulate-signer branch from fc6bc8d to 57c6ec7 Compare November 28, 2024 16:14

richardpringle commented Nov 28, 2024

View reviewed changes

richardpringle mentioned this pull request Nov 28, 2024

Encapsulate Signer ava-labs/coreth#693

Merged

richardpringle marked this pull request as ready for review November 28, 2024 20:10

richardpringle requested review from marun, joshua-kim and StephenButtolph as code owners November 28, 2024 20:10

richardpringle added 2 commits November 29, 2024 10:30

Move test function for coreth to avalanchego

3f46a5a

Point to new coreth

e3f0933

richardpringle commented Nov 29, 2024

View reviewed changes

utils/crypto/bls/secret.go

@@ -25,49 +25,64 @@ var (

type SecretKey = blst.SecretKey

Copy link

Contributor Author

richardpringle Nov 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should get rid of this type

marun approved these changes Dec 2, 2024

View reviewed changes

joshua-kim reviewed Dec 3, 2024

View reviewed changes

richardpringle added 5 commits December 4, 2024 10:59

Merge remote-tracking branch 'origin/master' into encapsulate-signer

e6672ea

Make local BLS signer public

ed70f23

Revert "Move test function for coreth to avalanchego"

0c37712

This reverts commit 3f46a5a.

Move interface check to top of file

3b8d76a

Update coreth

39e9877

joshua-kim approved these changes Dec 4, 2024

View reviewed changes

richardpringle commented Dec 5, 2024

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Remove replace from go.mod

6a023b0

Signed-off-by: Richard Pringle <[email protected]>

aaronbuchwald reviewed Dec 6, 2024

View reviewed changes

richardpringle and others added 2 commits December 9, 2024 12:43

Merge branch 'master' into encapsulate-signer

5241d03

Use a tag for coreth

1150175

StephenButtolph reviewed Dec 9, 2024

View reviewed changes

Remove serializing the key from the signer interface

fd7f326

StephenButtolph assigned richardpringle Dec 9, 2024

StephenButtolph modified the milestones: v1.12.0, v1.12.1 Dec 9, 2024

StephenButtolph approved these changes Dec 9, 2024

View reviewed changes

StephenButtolph added this pull request to the merge queue Dec 9, 2024

Merged via the queue into master with commit 1dc4192 Dec 9, 2024
23 checks passed

StephenButtolph deleted the encapsulate-signer branch December 9, 2024 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

encapsulate signer #3576

encapsulate signer #3576

richardpringle commented Nov 27, 2024 •

edited

Loading

richardpringle Nov 28, 2024

richardpringle Nov 28, 2024

joshua-kim Dec 2, 2024

richardpringle Dec 3, 2024

aaronbuchwald Dec 6, 2024

richardpringle Dec 6, 2024

richardpringle Nov 28, 2024

richardpringle Nov 28, 2024

aaronbuchwald Dec 6, 2024

richardpringle Dec 6, 2024

richardpringle Nov 28, 2024

aaronbuchwald Dec 6, 2024

yacovm commented Nov 28, 2024

richardpringle commented Nov 28, 2024

richardpringle Nov 29, 2024

marun left a comment

joshua-kim Dec 3, 2024

richardpringle Dec 3, 2024

richardpringle Dec 4, 2024

joshua-kim left a comment •

edited

Loading

aaronbuchwald left a comment

aaronbuchwald Dec 6, 2024

aaronbuchwald Dec 6, 2024

aaronbuchwald Dec 6, 2024

richardpringle Dec 6, 2024

richardpringle Dec 9, 2024

aaronbuchwald Dec 6, 2024

StephenButtolph Dec 9, 2024

richardpringle Dec 9, 2024

StephenButtolph Dec 9, 2024

richardpringle Dec 9, 2024

StephenButtolph Dec 9, 2024

richardpringle Dec 9, 2024

StephenButtolph Dec 9, 2024

	func SecretKeyFromBytes(skBytes []byte) (Signer, error) {
	func SecretKeyFromBytes(skBytes []byte) (*LocalSigner, error) {

		Sign(msg []byte) *Signature
		SignProofOfPossession(msg []byte) *Signature

encapsulate signer #3576

encapsulate signer #3576

Conversation

richardpringle commented Nov 27, 2024 • edited Loading

Why this should be merged

How this works

How this was tested

Need to be documented in RELEASES.md?

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

Choose a reason for hiding this comment

yacovm commented Nov 28, 2024

richardpringle commented Nov 28, 2024

Choose a reason for hiding this comment

marun 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

joshua-kim left a comment • edited Loading

Choose a reason for hiding this comment

aaronbuchwald 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle commented Nov 27, 2024 •

edited

Loading

joshua-kim left a comment •

edited

Loading