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.

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

One config file to rule them all and in cryptography bind them #147

Merged
merged 10 commits into from
May 10, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ build-linux:
@GOOS=linux GOARCH=amd64 go build --mod readonly $(BUILD_FLAGS) -o ./build/horcrux ./cmd/horcrux

test:
@go test -race -timeout 20m -mod readonly -v ./...
@go test -race -timeout 30m -mod readonly -v ./...

test-short:
@go test -mod readonly -run TestDownedSigners2of3 -v ./...
Expand Down
22 changes: 0 additions & 22 deletions cmd/horcrux/cmd/config.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
package cmd

import (
"errors"
"fmt"
"net"
"net/url"
"os"

"github.com/spf13/cobra"
Expand Down Expand Up @@ -68,28 +65,10 @@ func initCmd() *cobra.Command {
return err
}

listen, _ := cmdFlags.GetString("listen")
if listen == "" {
return errors.New("must input at least one node")
}
url, err := url.Parse(listen)
if err != nil {
return fmt.Errorf("error parsing listen address: %s, %v", listen, err)
}
host, _, err := net.SplitHostPort(url.Host)
if err != nil {
return err
}
if host == "0.0.0.0" {
return errors.New("host cannot be 0.0.0.0, must be reachable from other peers")
}

cfg = signer.Config{
PrivValKeyDir: keyDir,
CosignerConfig: &signer.CosignerConfig{
Threshold: threshold,
Shares: len(peers) + 1,
P2PListen: listen,
Peers: peers,
Timeout: timeout,
},
Expand Down Expand Up @@ -133,7 +112,6 @@ func initCmd() *cobra.Command {
"cosigner peer addresses in format tcp://{addr}:{port}|{share-id} \n"+
"(i.e. \"tcp://node-1:2222|2,tcp://node-2:2222|3\")")
cmd.Flags().IntP("threshold", "t", 0, "indicate number of signatures required for threshold signature")
cmd.Flags().StringP("listen", "l", "", "listen address of the signer")
cmd.Flags().StringP("debug-addr", "d", "", "listen address for Debug and Prometheus metrics in format localhost:8543")
cmd.Flags().StringP("key-dir", "k", "", "priv val key directory")
cmd.Flags().String("timeout", "1500ms", "configure cosigner rpc server timeout value, \n"+
Expand Down
9 changes: 3 additions & 6 deletions cmd/horcrux/cmd/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,8 @@ func TestConfigInitCmd(t *testing.T) {
args: []string{
"tcp://10.168.0.1:1234",
"-c",
"-p", "tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3",
"-p", "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we remove this flag completely. The format is unorthodox and tricky to read. Given the necessary complexity of multiple signers, I'm in favor of forcing a config file. Any thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Flags are switched around a bit now.

  • --peers/-p replaced by string slice --cosigner/-c. The delimiter is removed. The order in which the -c flags are used determines shard ID 1->n.
  • args[0] csv for chain nodes replaced by string slice flag --node/-n. This makes it easier to string -n flags together for multiple chain node privValAddr.
    --mode (string, either default-threshold or single) replaces -c.

"-t", "2",
"-l", "tcp://10.168.1.1:2222",
"--timeout", "1500ms",
},
expectErr: false,
Expand All @@ -36,9 +35,8 @@ func TestConfigInitCmd(t *testing.T) {
args: []string{
"://10.168.0.1:1234", // Missing/malformed protocol scheme
"-c",
"-p", "tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3",
"-p", "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3",
"-t", "2",
"-l", "tcp://10.168.1.1:2222",
"--timeout", "1500ms",
},
expectErr: true,
Expand All @@ -49,9 +47,8 @@ func TestConfigInitCmd(t *testing.T) {
args: []string{
"tcp://10.168.0.1:1234",
"-c",
"-p", "tcp://10.168.1.2:2222,tcp://10.168.1.3:2222", // Missing share IDs
"-p", "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222,tcp://10.168.1.3:2222", // Missing share IDs
"-t", "2",
"-l", "tcp://10.168.1.1:2222",
"--timeout", "1500ms",
},
expectErr: true,
Expand Down
43 changes: 23 additions & 20 deletions cmd/horcrux/cmd/cosigner.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,36 +143,39 @@ func startCosignerCmd() *cobra.Command {
return fmt.Errorf("error reading cosigner key (%s): %w", keyFile, err)
}

cosigners := []signer.Cosigner{}
cosignerConfig := config.Config.CosignerConfig

// add ourselves as a peer so localcosigner can handle GetEphSecPart requests
peers := []signer.CosignerPeer{{
ID: key.ID,
PublicKey: key.RSAKey.PublicKey,
}}
remoteCosigners := make([]signer.Cosigner, 0, len(cosignerConfig.Peers)-1)
peers := make([]signer.CosignerPeer, len(cosignerConfig.Peers))
agouin marked this conversation as resolved.
Show resolved Hide resolved

cosignerConfig := config.Config.CosignerConfig
var p2pListen string

for _, cosignerParams := range cosignerConfig.Peers {
cosigner := signer.NewRemoteCosigner(cosignerParams.ShareID, cosignerParams.P2PAddr)
cosigners = append(cosigners, cosigner)
for i, cosignerParams := range cosignerConfig.Peers {
if cosignerParams.ShareID != key.ID {
remoteCosigners = append(
remoteCosigners,
signer.NewRemoteCosigner(cosignerParams.ShareID, cosignerParams.P2PAddr),
)
} else {
p2pListen = cosignerParams.P2PAddr
}

pubKey := key.CosignerKeys[cosignerParams.ShareID-1]
peers = append(peers, signer.CosignerPeer{
ID: cosigner.GetID(),
peers[i] = signer.CosignerPeer{
ID: cosignerParams.ShareID,
PublicKey: *pubKey,
})
}
}

total := len(cosignerConfig.Peers) + 1
if p2pListen == "" {
return fmt.Errorf("peer config does not exist for our share ID %d", key.ID)
}

localCosigner := signer.NewLocalCosigner(
&config,
key.ID,
key.RSAKey,
key,
peers,
cosignerConfig.P2PListen,
uint8(total),
p2pListen,
uint8(cosignerConfig.Threshold),
)

Expand All @@ -191,7 +194,7 @@ func startCosignerCmd() *cobra.Command {

// Start RAFT store listener
raftStore := signer.NewRaftStore(nodeID,
raftDir, cosignerConfig.P2PListen, timeout, logger, localCosigner, cosigners)
raftDir, p2pListen, timeout, logger, localCosigner, remoteCosigners)
if err := raftStore.Start(); err != nil {
return fmt.Errorf("error starting raft store: %w", err)
}
Expand All @@ -202,7 +205,7 @@ func startCosignerCmd() *cobra.Command {
&config,
cosignerConfig.Threshold,
localCosigner,
cosigners,
remoteCosigners,
raftStore,
)

Expand Down
30 changes: 27 additions & 3 deletions cmd/horcrux/cmd/leader_election.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
grpcretry "github.com/grpc-ecosystem/go-grpc-middleware/retry"
"github.com/spf13/cobra"
"github.com/strangelove-ventures/horcrux/client"
"github.com/strangelove-ventures/horcrux/signer"
"github.com/strangelove-ventures/horcrux/signer/proto"
"google.golang.org/grpc"
"google.golang.org/grpc/credentials/insecure"
Expand Down Expand Up @@ -93,20 +94,43 @@ func getLeaderCmd() *cobra.Command {
Example: `horcrux leader`,
SilenceUsage: true,
RunE: func(cmd *cobra.Command, args []string) (err error) {
if config.Config.CosignerConfig == nil {
cosignerConfig := config.Config.CosignerConfig
if cosignerConfig == nil {
return fmt.Errorf("cosigner configuration is not present in config file")
}

if len(config.Config.CosignerConfig.Peers) == 0 {
if len(cosignerConfig.Peers) == 0 {
return fmt.Errorf("cosigner configuration has no peers")
}

keyFile, err := config.KeyFileExistsCosignerRSA()
if err != nil {
return err
}

key, err := signer.LoadCosignerKeyRSA(keyFile)
if err != nil {
return fmt.Errorf("error reading cosigner key (%s): %w", keyFile, err)
}

var p2pListen string

for _, peer := range cosignerConfig.Peers {
if peer.ShareID == key.ID {
p2pListen = peer.P2PAddr
}
}

if p2pListen == "" {
return fmt.Errorf("peer config does not exist for our share ID %d", key.ID)
}

retryOpts := []grpcretry.CallOption{
grpcretry.WithBackoff(grpcretry.BackoffExponential(100 * time.Millisecond)),
grpcretry.WithMax(5),
}

grpcAddress, err := client.SanitizeAddress(config.Config.CosignerConfig.P2PListen)
grpcAddress, err := client.SanitizeAddress(p2pListen)
if err != nil {
return err
}
Expand Down
28 changes: 22 additions & 6 deletions cmd/horcrux/cmd/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ type (
v2Config struct {
ChainID string `json:"chain-id" yaml:"chain-id"`
PrivValKeyFile *string `json:"key-file,omitempty" yaml:"key-file,omitempty"`
Cosigner struct {
P2PListen string `json:"p2p-listen" yaml:"p2p-listen"`
} `json:"cosigner" yaml:"cosigner"`
}

v2CosignerKey struct {
Expand Down Expand Up @@ -118,10 +121,10 @@ func (cosignerKey *v2CosignerKey) validate() error {

func migrateCmd() *cobra.Command {
return &cobra.Command{
Use: "migrate",
Use: "migrate [chain-id]",
Short: "Migrate config and key files from v2 to v3",
SilenceUsage: true,
Args: cobra.NoArgs,
Args: cobra.RangeArgs(0, 1),
RunE: func(cmd *cobra.Command, args []string) error {
cmd.SilenceUsage = true

Expand All @@ -130,17 +133,23 @@ func migrateCmd() *cobra.Command {
return err
}

var chainID string

var legacyConfig v2Config

if err := yaml.Unmarshal(configFile, &legacyConfig); err != nil {
return fmt.Errorf("failed to read config file as legacy: %w", err)
}

if legacyConfig.ChainID == "" {
return fmt.Errorf("unable to migrate v2 config without chain-id")
}
if len(args) == 1 {
chainID = args[0]
} else {
if legacyConfig.ChainID == "" {
return fmt.Errorf("unable to migrate v2 config without chain-id. please provide [chain-id] argument")
}

chainID := legacyConfig.ChainID
chainID = legacyConfig.ChainID
}

var legacyCosignerKeyFile string

Expand Down Expand Up @@ -203,6 +212,13 @@ func migrateCmd() *cobra.Command {
return fmt.Errorf("failed to write new RSA key to %s: %w", newRSAPath, err)
}

if legacyConfig.Cosigner.P2PListen != "" {
config.Config.CosignerConfig.Peers = append(config.Config.CosignerConfig.Peers, signer.CosignerPeerConfig{
ShareID: legacyCosignerKey.ID,
P2PAddr: legacyConfig.Cosigner.P2PListen,
})
}

if err := config.WriteConfigFile(); err != nil {
return err
}
Expand Down
3 changes: 1 addition & 2 deletions cmd/horcrux/cmd/state_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,7 @@ func TestStateSetCmd(t *testing.T) {
"tcp://10.168.0.1:1234",
"-c",
"-t", "2",
"-p", "tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3",
"-l", "tcp://10.168.1.1:2222",
"-p", "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3",
"--timeout", "1500ms",
})
err := cmd.Execute()
Expand Down
4 changes: 2 additions & 2 deletions cmd/horcrux/cmd/testdata/config-migrated.yaml
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
cosigner:
threshold: 2
shares: 3
p2p-listen: tcp://127.0.0.1:2222
peers:
- share-id: 1
p2p-addr: tcp://127.0.0.1:2222
Copy link
Contributor

Choose a reason for hiding this comment

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

Re UX: I'm wondering if there is any benefit to letting the user set the id.

We could use the index of the array as the id. However, if the user re-orders the array, I'm not sure what would happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

The name share-id isn't quite resonating with me. cosigner-id or signer-id sits better.

In the code (like error messages) it indicates peer id and in the documentation it indicates cosigner id. The code and docs do not mention a "share id".

I'm good with either but not both. We need consistency across the code, config, and documentation. In the config, I'm in favor of simply id. And since it's under a peers object, peer.id makes the most sense to me. The code also supports peer id over cosigner.

Copy link
Member Author

Choose a reason for hiding this comment

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

from our discussion off GH, we talked about having these be shards instead of peers, with an id property within each. After playing around with it some more I changed it to cosigners instead of peers, and changed share-id -> shardID within. I changed the higher level cosigner property to thresholdMode, since I think it better represents the configuration that is specific to using threshold mode. The grouping of shardID and p2pAddr adequately represents what cosigners need to know about each other, the network reachable address and the key shard ID for the ephemeral shares (nonce) and combined signature.

- share-id: 2
p2p-addr: tcp://127.0.0.1:2223
- share-id: 3
Expand Down
6 changes: 3 additions & 3 deletions cmd/horcrux/cmd/testdata/config-v2.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ chain-id: test
cosigner:
threshold: 2
shares: 3
p2p-listen: tcp://127.0.0.1:2222
p2p-listen: tcp://127.0.0.1:2224
peers:
- share-id: 1
p2p-addr: tcp://127.0.0.1:2222
- share-id: 2
p2p-addr: tcp://127.0.0.1:2223
- share-id: 3
p2p-addr: tcp://127.0.0.1:2224
rpc-timeout: 1000ms
chain-nodes:
- priv-val-addr: tcp://127.0.0.1:1234
Expand Down
17 changes: 4 additions & 13 deletions docs/migrating.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,20 +63,11 @@ $ sudo nano /etc/systemd/system/horcrux.service
$ sudo systemctl daemon-reload
```

After that is done, initialize the configuration for each node using the `horcrux` cli. Each node will require a slightly different command. Below are the commands for each of the 3 signer nodes given the private IPs above. Input your own data here:
After that is done, initialize the configuration for the cosigners using the `horcrux` cli. If you would like different cosigners to connect to different sentry node(s), modify the first argument to generate different config for each cosigner.

```bash
# Run this command on the signer-1 VM
# signer-1 connects to sentry-1
$ horcrux config init "tcp://10.168.0.1:1234" -c -p "tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3" -l "tcp://10.168.1.1:2222" -t 2

# Run this command on the signer-2 VM
# signer-2 connects to sentry-2
$ horcrux config init "tcp://10.168.0.2:1234" -c -p "tcp://10.168.1.1:2222|1,tcp://10.168.1.3:2222|3" -l "tcp://10.168.1.2:2222" -t 2

# Run this command on the signer-3 VM
# signer-3 connects to sentry-3
$ horcrux config init "tcp://10.168.0.3:1234" -c -p "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222|2" -l "tcp://10.168.1.3:2222" -t 2
# Run this command to generate a config file that can be used on all config nodes.
$ horcrux config init "tcp://10.168.0.1:1234" -c -p "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3" -t 2 --timeout 1000ms
```

> **Note**
Expand Down Expand Up @@ -219,6 +210,6 @@ You now can sleep much better at night because you are much less likely to have

### 8. Administration Commands

`horcrux elect` - Elect a new cluster leader. Pass an optional argument with the intended leader ID to elect that cosigner as the new leader, e.g. `horcrux elect 3` to elect cosigner with `ID: 3` as leader
`horcrux elect` - Elect a new cluster leader. Pass an optional argument with the intended leader ID to elect that cosigner as the new leader, e.g. `horcrux elect 3` to elect cosigner with `ID: 3` as leader. This is an optimistic leader election, it is not guaranteed that the exact requested leader will be elected.
agouin marked this conversation as resolved.
Show resolved Hide resolved

`horcrux cosigner address` - Get the public key address as both hex and optionally the validator consensus bech32 address. To retrieve the valcons bech32 address, pass an optional argument with the chain's bech32 valcons prefix, e.g. `horcrux cosigner address cosmosvalcons`
Loading