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

Jump to bottom

Replace Guardian Key with abstracted Guardian Signer #4120

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Replace Guardian Key with abstracted Guardian Signer #4120

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2c23b76
node: add guardiansigner node/pkg
Sep 19, 2024
857662d
node: replace use of guardian key with guardian signer
Sep 19, 2024
3917271
node: replace use of vaa.AddSigner with guardian signer
Sep 19, 2024
e8b07d3
node: add nolint for armor import and fix test
Sep 19, 2024
8b91ed9
node: handle error returned from signing
Sep 19, 2024
0b8b14d
apply draft review suggestions
Sep 19, 2024
7cc1ebd
apply pr reviews
Sep 26, 2024
32fa31d
apply pr reviews
Oct 3, 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
15 changes: 9 additions & 6 deletions node/cmd/guardiand/adminclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"github.com/spf13/pflag"
"golang.org/x/crypto/sha3"

"github.com/certusone/wormhole/node/pkg/common"
"github.com/certusone/wormhole/node/pkg/guardiansigner"
gossipv1 "github.com/certusone/wormhole/node/pkg/proto/gossip/v1"
publicrpcv1 "github.com/certusone/wormhole/node/pkg/proto/publicrpc/v1"
"github.com/wormhole-foundation/wormhole/sdk"
Expand Down Expand Up @@ -102,7 +102,7 @@ var AdminCmd = &cobra.Command{
}

var AdminClientSignWormchainAddress = &cobra.Command{
Use: "sign-wormchain-address [/path/to/guardianKey] [wormchain-validator-address]",
Use: "sign-wormchain-address [vaa-signer-uri] [wormchain-validator-address]",
Short: "Sign a wormchain validator address. Only sign the address that you control the key for and will be for your validator.",
RunE: runSignWormchainValidatorAddress,
Args: cobra.ExactArgs(2),
Expand Down Expand Up @@ -236,22 +236,25 @@ func getPublicRPCServiceClient(ctx context.Context, addr string) (*grpc.ClientCo
}

func runSignWormchainValidatorAddress(cmd *cobra.Command, args []string) error {
guardianKeyPath := args[0]
guardianSignerUri := args[0]
wormchainAddress := args[1]
if !strings.HasPrefix(wormchainAddress, "wormhole") || strings.HasPrefix(wormchainAddress, "wormholeval") {
return errors.New("must provide a bech32 address that has 'wormhole' prefix")
}
gk, err := common.LoadGuardianKey(guardianKeyPath, *unsafeDevnetMode)

guardianSigner, err := guardiansigner.NewGuardianSignerFromUri(guardianSignerUri, *unsafeDevnetMode)
if err != nil {
return fmt.Errorf("failed to load guardian key: %w", err)
return fmt.Errorf("failed to create new guardian signer from uri: %w", err)
}

addr, err := types.GetFromBech32(wormchainAddress, "wormhole")
if err != nil {
return fmt.Errorf("failed to decode wormchain address: %w", err)
}

// Hash and sign address
addrHash := crypto.Keccak256Hash(sdk.SignedWormchainAddressPrefix, addr)
sig, err := crypto.Sign(addrHash[:], gk)
sig, err := guardianSigner.Sign(addrHash.Bytes())
if err != nil {
return fmt.Errorf("failed to sign wormchain address: %w", err)
}
Expand Down
44 changes: 30 additions & 14 deletions node/cmd/guardiand/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"syscall"
"time"

"github.com/certusone/wormhole/node/pkg/guardiansigner"
"github.com/certusone/wormhole/node/pkg/watchers"
"github.com/certusone/wormhole/node/pkg/watchers/ibc"
ethcrypto "github.com/ethereum/go-ethereum/crypto"
Expand Down Expand Up @@ -62,8 +63,9 @@ var (

statusAddr *string

guardianKeyPath *string
solanaContract *string
guardianKeyPath *string
guardianSignerUri *string
solanaContract *string

ethRPC *string
ethContract *string
Expand Down Expand Up @@ -266,7 +268,8 @@ func init() {

dataDir = NodeCmd.Flags().String("dataDir", "", "Data directory")

guardianKeyPath = NodeCmd.Flags().String("guardianKey", "", "Path to guardian key (required)")
guardianKeyPath = NodeCmd.Flags().String("guardianKey", "", "Path to guardian key")
guardianSignerUri = NodeCmd.Flags().String("guardianSignerUri", "", "Guardian signer URI")
solanaContract = NodeCmd.Flags().String("solanaContract", "", "Address of the Solana program (required)")

ethRPC = node.RegisterFlagWithValidationOrFail(NodeCmd, "ethRPC", "Ethereum RPC URL", "ws://eth-devnet:8545", []string{"ws", "wss"})
Expand Down Expand Up @@ -622,7 +625,17 @@ func runNode(cmd *cobra.Command, args []string) {
logger.Fatal("Please specify --nodeKey")
}
if *guardianKeyPath == "" {
logger.Fatal("Please specify --guardianKey")
// This if-statement is nested, since checking if both are empty at once will always result in the else-branch
// being executed if at least one is specified. For example, in the case where the singer URI is specified and
// the guardianKeyPath not, then the else-statement will create an empty `file://` URI.
if *guardianSignerUri == "" {
logger.Fatal("Please specify --guardianKey or --guardianSignerUri")
}
pleasew8t marked this conversation as resolved.
Show resolved Hide resolved
} else {
// If guardianKeyPath is set, set guardianSignerUri to the file signer URI, pointing to guardianKeyPath.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you throw if both are set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I decided that I would default to using guardianKeyPath, even if the signer URI is set. I don't have any strong feelings about this decision. Do you think it should rather throw if both are set?

// This ensures that the signer-abstracted guardian has backwards compatibility with guardians that would
// just like to ignore the new guardianSignerUri altogether.
*guardianSignerUri = fmt.Sprintf("file://%s", *guardianKeyPath)
pleasew8t marked this conversation as resolved.
Show resolved Hide resolved
}
if *adminSocketPath == "" {
logger.Fatal("Please specify --adminSocket")
Expand All @@ -644,20 +657,23 @@ func runNode(cmd *cobra.Command, args []string) {

// In devnet mode, we generate a deterministic guardian key and write it to disk.
if env == common.UnsafeDevNet {
err := devnet.GenerateAndStoreDevnetGuardianKey(*guardianKeyPath)
if err != nil {
logger.Fatal("failed to generate devnet guardian key", zap.Error(err))
// Only if the signer is file-based should we generate the deterministic key and write it to disk
if st, _, _ := guardiansigner.ParseSignerUri(*guardianSignerUri); st == guardiansigner.FileSignerType {
err := devnet.GenerateAndStoreDevnetGuardianKey(*guardianKeyPath)
if err != nil {
logger.Fatal("failed to generate devnet guardian key", zap.Error(err))
}
}
}

// Load guardian key
gk, err := common.LoadGuardianKey(*guardianKeyPath, env == common.UnsafeDevNet)
// Create the Guardian Signer
guardianSigner, err := guardiansigner.NewGuardianSignerFromUri(*guardianSignerUri, env == common.UnsafeDevNet)
if err != nil {
logger.Fatal("failed to load guardian key", zap.Error(err))
logger.Fatal("failed to create a new guardian signer", zap.Error(err))
}

logger.Info("Loaded guardian key", zap.String(
"address", ethcrypto.PubkeyToAddress(gk.PublicKey).String()))
"address", ethcrypto.PubkeyToAddress(guardianSigner.PublicKey()).String()))

// Load p2p private key
var p2pKey libp2p_crypto.PrivKey
Expand Down Expand Up @@ -713,7 +729,7 @@ func runNode(cmd *cobra.Command, args []string) {
labels := map[string]string{
"node_name": *nodeName,
"node_key": peerID.String(),
"guardian_addr": ethcrypto.PubkeyToAddress(gk.PublicKey).String(),
"guardian_addr": ethcrypto.PubkeyToAddress(guardianSigner.PublicKey()).String(),
"network": *p2pNetworkID,
"version": version.Version(),
}
Expand Down Expand Up @@ -1052,7 +1068,7 @@ func runNode(cmd *cobra.Command, args []string) {
info.PromRemoteURL = *promRemoteURL
info.Labels = map[string]string{
"node_name": *nodeName,
"guardian_addr": ethcrypto.PubkeyToAddress(gk.PublicKey).String(),
"guardian_addr": ethcrypto.PubkeyToAddress(guardianSigner.PublicKey()).String(),
"network": *p2pNetworkID,
"version": version.Version(),
"product": "wormhole",
Expand Down Expand Up @@ -1565,7 +1581,7 @@ func runNode(cmd *cobra.Command, args []string) {

guardianNode := node.NewGuardianNode(
env,
gk,
guardianSigner,
)

guardianOptions := []*node.GuardianOption{
Expand Down
45 changes: 23 additions & 22 deletions node/hack/accountant/send_obs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ package main

import (
"context"
"crypto/ecdsa"
"encoding/hex"
"time"

"github.com/certusone/wormhole/node/pkg/accountant"
"github.com/certusone/wormhole/node/pkg/common"
"github.com/certusone/wormhole/node/pkg/guardiansigner"
"github.com/certusone/wormhole/node/pkg/wormconn"
"github.com/wormhole-foundation/wormhole/sdk/vaa"

Expand All @@ -26,7 +26,7 @@ func main() {
wormchainURL := string("localhost:9090")
wormchainKeyPath := string("./dev.wormchain.key")
contract := "wormhole14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9srrg465"
guardianKeyPath := string("./dev.guardian.key")
guardianSignerUri := string("file://dev.guardian.key")

wormchainKey, err := wormconn.LoadWormchainPrivKey(wormchainKeyPath, "test0000")
if err != nil {
Expand All @@ -44,40 +44,41 @@ func main() {
zap.String("senderAddress", wormchainConn.SenderAddress()),
)

logger.Info("Loading guardian key", zap.String("guardianKeyPath", guardianKeyPath))
gk, err := common.LoadGuardianKey(guardianKeyPath, true)
logger.Info("Initializing guardian signer", zap.String("guardianSignerUri", guardianSignerUri))
guardianSigner, err := guardiansigner.NewGuardianSignerFromUri(guardianSignerUri, true)

if err != nil {
logger.Fatal("failed to load guardian key", zap.Error(err))
}

sequence := uint64(time.Now().Unix())
timestamp := time.Now()

if !testSubmit(ctx, logger, gk, wormchainConn, contract, "0000000000000000000000000290fb167208af455bb137780163b7b7a9a10c16", timestamp, sequence, true, "Submit should succeed") {
if !testSubmit(ctx, logger, guardianSigner, wormchainConn, contract, "0000000000000000000000000290fb167208af455bb137780163b7b7a9a10c16", timestamp, sequence, true, "Submit should succeed") {
return
}

if !testSubmit(ctx, logger, gk, wormchainConn, contract, "0000000000000000000000000290fb167208af455bb137780163b7b7a9a10c16", timestamp, sequence, true, "Already commited should succeed") {
if !testSubmit(ctx, logger, guardianSigner, wormchainConn, contract, "0000000000000000000000000290fb167208af455bb137780163b7b7a9a10c16", timestamp, sequence, true, "Already commited should succeed") {
return
}

sequence += 10
if !testSubmit(ctx, logger, gk, wormchainConn, contract, "0000000000000000000000000290fb167208af455bb137780163b7b7a9a10c17", timestamp, sequence, false, "Bad emitter address should fail") {
if !testSubmit(ctx, logger, guardianSigner, wormchainConn, contract, "0000000000000000000000000290fb167208af455bb137780163b7b7a9a10c17", timestamp, sequence, false, "Bad emitter address should fail") {
return
}

sequence += 10
if !testBatch(ctx, logger, gk, wormchainConn, contract, timestamp, sequence) {
if !testBatch(ctx, logger, guardianSigner, wormchainConn, contract, timestamp, sequence) {
return
}

sequence += 10
if !testBatchWithcommitted(ctx, logger, gk, wormchainConn, contract, timestamp, sequence) {
if !testBatchWithcommitted(ctx, logger, guardianSigner, wormchainConn, contract, timestamp, sequence) {
return
}

sequence += 10
if !testBatchWithDigestError(ctx, logger, gk, wormchainConn, contract, timestamp, sequence) {
if !testBatchWithDigestError(ctx, logger, guardianSigner, wormchainConn, contract, timestamp, sequence) {
return
}

Expand All @@ -87,7 +88,7 @@ func main() {
func testSubmit(
ctx context.Context,
logger *zap.Logger,
gk *ecdsa.PrivateKey,
guardianSigner guardiansigner.GuardianSigner,
wormchainConn *wormconn.ClientConn,
contract string,
emitterAddressStr string,
Expand All @@ -112,7 +113,7 @@ func testSubmit(
}

msgs := []*common.MessagePublication{&msg}
txResp, err := submit(ctx, logger, gk, wormchainConn, contract, msgs)
txResp, err := submit(ctx, logger, guardianSigner, wormchainConn, contract, msgs)
if err != nil {
logger.Error("failed to broadcast Observation request", zap.String("test", tag), zap.Error(err))
return false
Expand Down Expand Up @@ -151,7 +152,7 @@ func testSubmit(
func testBatch(
ctx context.Context,
logger *zap.Logger,
gk *ecdsa.PrivateKey,
guardianSigner guardiansigner.GuardianSigner,
wormchainConn *wormconn.ClientConn,
contract string,
timestamp time.Time,
Expand Down Expand Up @@ -191,7 +192,7 @@ func testBatch(
}
msgs = append(msgs, &msg2)

txResp, err := submit(ctx, logger, gk, wormchainConn, contract, msgs)
txResp, err := submit(ctx, logger, guardianSigner, wormchainConn, contract, msgs)
if err != nil {
logger.Error("failed to broadcast Observation request", zap.Error(err))
return false
Expand Down Expand Up @@ -229,7 +230,7 @@ func testBatch(
func testBatchWithcommitted(
ctx context.Context,
logger *zap.Logger,
gk *ecdsa.PrivateKey,
guardianSigner guardiansigner.GuardianSigner,
wormchainConn *wormconn.ClientConn,
contract string,
timestamp time.Time,
Expand All @@ -256,7 +257,7 @@ func testBatchWithcommitted(
}
msgs = append(msgs, &msg1)

_, err := submit(ctx, logger, gk, wormchainConn, contract, msgs)
_, err := submit(ctx, logger, guardianSigner, wormchainConn, contract, msgs)
if err != nil {
logger.Error("failed to submit initial observation that should work", zap.Error(err))
return false
Expand All @@ -283,7 +284,7 @@ func testBatchWithcommitted(
msg3 := msg1
msgs = append(msgs, &msg3)

txResp, err := submit(ctx, logger, gk, wormchainConn, contract, msgs)
txResp, err := submit(ctx, logger, guardianSigner, wormchainConn, contract, msgs)
if err != nil {
logger.Error("failed to broadcast Observation request", zap.Error(err))
return false
Expand Down Expand Up @@ -321,7 +322,7 @@ func testBatchWithcommitted(
func testBatchWithDigestError(
ctx context.Context,
logger *zap.Logger,
gk *ecdsa.PrivateKey,
guardianSigner guardiansigner.GuardianSigner,
wormchainConn *wormconn.ClientConn,
contract string,
timestamp time.Time,
Expand All @@ -348,7 +349,7 @@ func testBatchWithDigestError(
}
msgs = append(msgs, &msg1)

_, err := submit(ctx, logger, gk, wormchainConn, contract, msgs)
_, err := submit(ctx, logger, guardianSigner, wormchainConn, contract, msgs)
if err != nil {
logger.Error("failed to submit initial observation that should work", zap.Error(err))
return false
Expand Down Expand Up @@ -376,7 +377,7 @@ func testBatchWithDigestError(
msg3.Nonce = msg3.Nonce + 1
msgs = append(msgs, &msg3)

txResp, err := submit(ctx, logger, gk, wormchainConn, contract, msgs)
txResp, err := submit(ctx, logger, guardianSigner, wormchainConn, contract, msgs)
if err != nil {
logger.Error("failed to submit second observation that should work", zap.Error(err))
return false
Expand Down Expand Up @@ -429,13 +430,13 @@ func testBatchWithDigestError(
func submit(
ctx context.Context,
logger *zap.Logger,
gk *ecdsa.PrivateKey,
guardianSigner guardiansigner.GuardianSigner,
wormchainConn *wormconn.ClientConn,
contract string,
msgs []*common.MessagePublication,
) (*sdktx.BroadcastTxResponse, error) {
gsIndex := uint32(0)
guardianIndex := uint32(0)

return accountant.SubmitObservationsToContract(ctx, logger, gk, gsIndex, guardianIndex, wormchainConn, contract, accountant.SubmitObservationPrefix, msgs)
return accountant.SubmitObservationsToContract(ctx, logger, guardianSigner, gsIndex, guardianIndex, wormchainConn, contract, accountant.SubmitObservationPrefix, msgs)
}
10 changes: 5 additions & 5 deletions node/pkg/accountant/accountant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ package accountant

import (
"context"
"crypto/ecdsa"
"fmt"
"sync"
"time"

"github.com/certusone/wormhole/node/pkg/common"
"github.com/certusone/wormhole/node/pkg/db"
"github.com/certusone/wormhole/node/pkg/guardiansigner"
gossipv1 "github.com/certusone/wormhole/node/pkg/proto/gossip/v1"
"github.com/certusone/wormhole/node/pkg/supervisor"
sdktypes "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -83,7 +83,7 @@ type Accountant struct {
wsUrl string
wormchainConn AccountantWormchainConn
enforceFlag bool
gk *ecdsa.PrivateKey
guardianSigner guardiansigner.GuardianSigner
gst *common.GuardianSetState
guardianAddr ethCommon.Address
msgChan chan<- *common.MessagePublication
Expand Down Expand Up @@ -120,7 +120,7 @@ func NewAccountant(
enforceFlag bool, // whether or not accountant should be enforced
nttContract string, // the address of the NTT smart contract on wormchain
nttWormchainConn AccountantWormchainConn, // used for communicating with the NTT smart contract
gk *ecdsa.PrivateKey, // the guardian key used for signing observation requests
guardianSigner guardiansigner.GuardianSigner, // the guardian signer used for signing observation requests
gst *common.GuardianSetState, // used to get the current guardian set index when sending observation requests
msgChan chan<- *common.MessagePublication, // the channel where transfers received by the accountant runnable should be published
env common.Environment, // Controls the set of token bridges to be monitored
Expand All @@ -134,9 +134,9 @@ func NewAccountant(
wsUrl: wsUrl,
wormchainConn: wormchainConn,
enforceFlag: enforceFlag,
gk: gk,
guardianSigner: guardianSigner,
gst: gst,
guardianAddr: ethCrypto.PubkeyToAddress(gk.PublicKey),
guardianAddr: ethCrypto.PubkeyToAddress(guardianSigner.PublicKey()),
msgChan: msgChan,
tokenBridges: make(validEmitters),
pendingTransfers: make(map[string]*pendingEntry),
Expand Down
Loading
Loading