Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

encapsulate signer #3576

Merged
merged 15 commits into from
Dec 9, 2024
Merged

encapsulate signer #3576

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
4b1d0d2
Rename `NewSecretKey` to `NewSigner`
richardpringle Nov 26, 2024
4177305
WIP
richardpringle Nov 26, 2024
a3af684
Straglers
richardpringle Nov 27, 2024
57c6ec7
Point to different version of coreth
richardpringle Nov 27, 2024
3f46a5a
Move test function for coreth to avalanchego
richardpringle Nov 29, 2024
e3f0933
Point to new coreth
richardpringle Nov 29, 2024
e6672ea
Merge remote-tracking branch 'origin/master' into encapsulate-signer
richardpringle Dec 4, 2024
ed70f23
Make local BLS signer public
richardpringle Dec 4, 2024
0c37712
Revert "Move test function for coreth to avalanchego"
richardpringle Dec 4, 2024
3b8d76a
Move interface check to top of file
richardpringle Dec 4, 2024
39e9877
Update coreth
richardpringle Dec 4, 2024
6a023b0
Remove `replace` from go.mod
richardpringle Dec 5, 2024
5241d03
Merge branch 'master' into encapsulate-signer
richardpringle Dec 9, 2024
1150175
Use a tag for coreth
richardpringle Dec 9, 2024
fd7f326
Remove serializing the key from the signer interface
richardpringle Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions chains/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ type ManagerConfig struct {
SybilProtectionEnabled bool
StakingTLSSigner crypto.Signer
StakingTLSCert *staking.Certificate
StakingBLSKey *bls.SecretKey
StakingBLSKey bls.Signer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to change the property name here too?

TracingEnabled bool
// Must not be used unless [TracingEnabled] is true as this may be nil.
Tracer trace.Tracer
Expand Down Expand Up @@ -497,7 +497,7 @@ func (m *manager) buildChain(chainParams ChainParameters, sb subnets.Subnet) (*c
SubnetID: chainParams.SubnetID,
ChainID: chainParams.ID,
NodeID: m.NodeID,
PublicKey: bls.PublicFromSecretKey(m.StakingBLSKey),
PublicKey: m.StakingBLSKey.PublicKey(),
NetworkUpgrades: m.Upgrades,

XChainID: m.XChainID,
Expand Down
8 changes: 4 additions & 4 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,9 +645,9 @@ func getStakingTLSCert(v *viper.Viper) (tls.Certificate, error) {
}
}

func getStakingSigner(v *viper.Viper) (*bls.SecretKey, error) {
func getStakingSigner(v *viper.Viper) (bls.Signer, error) {
if v.GetBool(StakingEphemeralSignerEnabledKey) {
key, err := bls.NewSecretKey()
key, err := bls.NewSigner()
if err != nil {
return nil, fmt.Errorf("couldn't generate ephemeral signing key: %w", err)
}
Expand Down Expand Up @@ -685,7 +685,7 @@ func getStakingSigner(v *viper.Viper) (*bls.SecretKey, error) {
return nil, errMissingStakingSigningKeyFile
}

key, err := bls.NewSecretKey()
key, err := bls.NewSigner()
if err != nil {
return nil, fmt.Errorf("couldn't generate new signing key: %w", err)
}
Expand All @@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if err := os.WriteFile(signingKeyPath, keyBytes, perms.ReadWrite); err != nil {
return nil, fmt.Errorf("couldn't write new signing key to %s: %w", signingKeyPath, err)
}
Expand Down
2 changes: 2 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.20241129155209-a857e7c5a2da
richardpringle marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ github.com/antithesishq/antithesis-sdk-go v0.3.8/go.mod h1:IUpT2DPAKh6i/YhSbt6Gl
github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio=
github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs=
github.com/ava-labs/coreth v0.13.9-rc.1 h1:qIICpC/OZGYUP37QnLgIqqwGmxnLwLpZaUlqJNI85vU=
github.com/ava-labs/coreth v0.13.9-rc.1/go.mod h1:7aMsRIo/3GBE44qWZMjnfqdqfcfZ5yShTTm2LObLaYo=
github.com/ava-labs/coreth v0.13.9-rc.1.0.20241129155209-a857e7c5a2da h1:Ob3b91PXCUORPCUIoUfSEBiOQ93t9RPggaJthOtNQ8Q=
github.com/ava-labs/coreth v0.13.9-rc.1.0.20241129155209-a857e7c5a2da/go.mod h1:mEjC/EAEVVb7nsvbi66XFBD7TSNANiUAdsghaIruq4U=
github.com/ava-labs/ledger-avalanche/go v0.0.0-20241009183145-e6f90a8a1a60 h1:EL66gtXOAwR/4KYBjOV03LTWgkEXvLePribLlJNu4g0=
github.com/ava-labs/ledger-avalanche/go v0.0.0-20241009183145-e6f90a8a1a60/go.mod h1:/7qKobTfbzBu7eSTVaXMTr56yTYk4j2Px6/8G+idxHo=
github.com/aymerick/raymond v2.0.3-0.20180322193309-b565731e1464+incompatible/go.mod h1:osfaiScAUVup+UC9Nfq76eWqDhXlp+4UYaA8uhTBO6g=
Expand Down
2 changes: 1 addition & 1 deletion network/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:"-"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one day...


// TrackedSubnets of the node.
// It must not include the primary network ID.
Expand Down
2 changes: 1 addition & 1 deletion network/network_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems reasonable

require.NoError(t, err)

config := defaultConfig
Expand Down
4 changes: 2 additions & 2 deletions network/p2p/acp118/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,9 @@ func TestHandler(t *testing.T) {
require := require.New(t)

ctx := context.Background()
sk, err := bls.NewSecretKey()
sk, err := bls.NewSigner()
require.NoError(err)
pk := bls.PublicFromSecretKey(sk)
pk := sk.PublicKey()
networkID := uint32(123)
chainID := ids.GenerateTestID()
signer := warp.NewSigner(sk, networkID, chainID)
Expand Down
4 changes: 2 additions & 2 deletions network/peer/ip.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ type UnsignedIP struct {
}

// Sign this IP with the provided signer and return the signed IP.
func (ip *UnsignedIP) Sign(tlsSigner crypto.Signer, blsSigner *bls.SecretKey) (*SignedIP, error) {
func (ip *UnsignedIP) Sign(tlsSigner crypto.Signer, blsSigner bls.Signer) (*SignedIP, error) {
ipBytes := ip.bytes()
tlsSignature, err := tlsSigner.Sign(
rand.Reader,
hashing.ComputeHash256(ipBytes),
crypto.SHA256,
)
blsSignature := bls.SignProofOfPossession(blsSigner, ipBytes)
blsSignature := blsSigner.SignProofOfPossession(ipBytes)
return &SignedIP{
UnsignedIP: *ip,
TLSSignature: tlsSignature,
Expand Down
4 changes: 2 additions & 2 deletions network/peer/ip_signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ type IPSigner struct {
ip *utils.Atomic[netip.AddrPort]
clock mockable.Clock
tlsSigner crypto.Signer
blsSigner *bls.SecretKey
blsSigner bls.Signer

// Must be held while accessing [signedIP]
signedIPLock sync.RWMutex
Expand All @@ -30,7 +30,7 @@ type IPSigner struct {
func NewIPSigner(
ip *utils.Atomic[netip.AddrPort],
tlsSigner crypto.Signer,
blsSigner *bls.SecretKey,
blsSigner bls.Signer,
) *IPSigner {
return &IPSigner{
ip: ip,
Expand Down
2 changes: 1 addition & 1 deletion network/peer/ip_signer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestIPSigner(t *testing.T) {
require.NoError(err)

tlsKey := tlsCert.PrivateKey.(crypto.Signer)
blsKey, err := bls.NewSecretKey()
blsKey, err := bls.NewSigner()
require.NoError(err)

s := NewIPSigner(dynIP, tlsKey, blsKey)
Expand Down
4 changes: 2 additions & 2 deletions network/peer/ip_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func TestSignedIpVerify(t *testing.T) {
cert1, err := staking.ParseCertificate(tlsCert1.Leaf.Raw)
require.NoError(t, err)
tlsKey1 := tlsCert1.PrivateKey.(crypto.Signer)
blsKey1, err := bls.NewSecretKey()
blsKey1, err := bls.NewSigner()
require.NoError(t, err)

tlsCert2, err := staking.NewTLSCert()
Expand All @@ -38,7 +38,7 @@ func TestSignedIpVerify(t *testing.T) {
type test struct {
name string
tlsSigner crypto.Signer
blsSigner *bls.SecretKey
blsSigner bls.Signer
expectedCert *staking.Certificate
ip UnsignedIP
maxTimestamp time.Time
Expand Down
34 changes: 17 additions & 17 deletions network/peer/peer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func newRawTestPeer(t *testing.T, config Config) *rawTestPeer {
1,
))
tls := tlsCert.PrivateKey.(crypto.Signer)
bls, err := bls.NewSecretKey()
bls, err := bls.NewSigner()
require.NoError(err)

config.IPSigner = NewIPSigner(ip, tls, bls)
Expand Down Expand Up @@ -322,17 +322,17 @@ func TestInvalidBLSKeyDisconnects(t *testing.T) {
require.NoError(rawPeer0.config.Validators.AddStaker(
constants.PrimaryNetworkID,
rawPeer1.config.MyNodeID,
bls.PublicFromSecretKey(rawPeer1.config.IPSigner.blsSigner),
rawPeer1.config.IPSigner.blsSigner.PublicKey(),
ids.GenerateTestID(),
1,
))

bogusBLSKey, err := bls.NewSecretKey()
bogusBLSKey, err := bls.NewSigner()
require.NoError(err)
require.NoError(rawPeer1.config.Validators.AddStaker(
constants.PrimaryNetworkID,
rawPeer0.config.MyNodeID,
bls.PublicFromSecretKey(bogusBLSKey), // This is the wrong BLS key for this peer
bogusBLSKey.PublicKey(), // This is the wrong BLS key for this peer
ids.GenerateTestID(),
1,
))
Expand All @@ -348,7 +348,7 @@ func TestInvalidBLSKeyDisconnects(t *testing.T) {
func TestShouldDisconnect(t *testing.T) {
peerID := ids.GenerateTestNodeID()
txID := ids.GenerateTestID()
blsKey, err := bls.NewSecretKey()
blsKey, err := bls.NewSigner()
require.NoError(t, err)

tests := []struct {
Expand Down Expand Up @@ -458,7 +458,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -478,7 +478,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -502,7 +502,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -522,7 +522,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -546,7 +546,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -556,7 +556,7 @@ func TestShouldDisconnect(t *testing.T) {
id: peerID,
version: version.CurrentApp,
ip: &SignedIP{
BLSSignature: bls.SignProofOfPossession(blsKey, []byte("wrong message")),
BLSSignature: blsKey.SignProofOfPossession([]byte("wrong message")),
},
},
expectedPeer: &peer{
Expand All @@ -568,7 +568,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -578,7 +578,7 @@ func TestShouldDisconnect(t *testing.T) {
id: peerID,
version: version.CurrentApp,
ip: &SignedIP{
BLSSignature: bls.SignProofOfPossession(blsKey, []byte("wrong message")),
BLSSignature: blsKey.SignProofOfPossession([]byte("wrong message")),
},
},
expectedShouldDisconnect: true,
Expand All @@ -594,7 +594,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -604,7 +604,7 @@ func TestShouldDisconnect(t *testing.T) {
id: peerID,
version: version.CurrentApp,
ip: &SignedIP{
BLSSignature: bls.SignProofOfPossession(blsKey, (&UnsignedIP{}).bytes()),
BLSSignature: blsKey.SignProofOfPossession((&UnsignedIP{}).bytes()),
},
},
expectedPeer: &peer{
Expand All @@ -616,7 +616,7 @@ func TestShouldDisconnect(t *testing.T) {
require.NoError(t, vdrs.AddStaker(
constants.PrimaryNetworkID,
peerID,
bls.PublicFromSecretKey(blsKey),
blsKey.PublicKey(),
txID,
1,
))
Expand All @@ -626,7 +626,7 @@ func TestShouldDisconnect(t *testing.T) {
id: peerID,
version: version.CurrentApp,
ip: &SignedIP{
BLSSignature: bls.SignProofOfPossession(blsKey, (&UnsignedIP{}).bytes()),
BLSSignature: blsKey.SignProofOfPossession((&UnsignedIP{}).bytes()),
},
txIDOfVerifiedBLSKey: txID,
},
Expand Down
2 changes: 1 addition & 1 deletion network/peer/test_peer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func StartTestPeer(
}

tlsKey := tlsCert.PrivateKey.(crypto.Signer)
blsKey, err := bls.NewSecretKey()
blsKey, err := bls.NewSigner()
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion network/test_network.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func NewTestNetwork(
return nil, err
}

blsKey, err := bls.NewSecretKey()
blsKey, err := bls.NewSigner()
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion node/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ type StakingConfig struct {
SybilProtectionEnabled bool `json:"sybilProtectionEnabled"`
PartialSyncPrimaryNetwork bool `json:"partialSyncPrimaryNetwork"`
StakingTLSCert tls.Certificate `json:"-"`
StakingSigningKey *bls.SecretKey `json:"-"`
StakingSigningKey bls.Signer `json:"-"`
SybilProtectionDisabledWeight uint64 `json:"sybilProtectionDisabledWeight"`
StakingKeyPath string `json:"stakingKeyPath"`
StakingCertPath string `json:"stakingCertPath"`
Expand Down
3 changes: 1 addition & 2 deletions node/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ import (
"github.com/ava-labs/avalanchego/trace"
"github.com/ava-labs/avalanchego/utils"
"github.com/ava-labs/avalanchego/utils/constants"
"github.com/ava-labs/avalanchego/utils/crypto/bls"
"github.com/ava-labs/avalanchego/utils/dynamicip"
"github.com/ava-labs/avalanchego/utils/filesystem"
"github.com/ava-labs/avalanchego/utils/hashing"
Expand Down Expand Up @@ -583,7 +582,7 @@ func (n *Node) initNetworking(reg prometheus.Registerer) error {
err := n.vdrs.AddStaker(
constants.PrimaryNetworkID,
n.ID,
bls.PublicFromSecretKey(n.Config.StakingSigningKey),
n.Config.StakingSigningKey.PublicKey(),
dummyTxID,
n.Config.SybilProtectionDisabledWeight,
)
Expand Down
Loading
Loading