diff --git a/.coverage-baseline b/.coverage-baseline index d6cc22a060f..a63cb12299b 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -6,8 +6,8 @@ github.com/certusone/wormhole/node 0.0 github.com/certusone/wormhole/node/cmd 0.0 github.com/certusone/wormhole/node/cmd/ccq 15.5 github.com/certusone/wormhole/node/cmd/debug 0.0 -github.com/certusone/wormhole/node/cmd/guardiand 21.6 -github.com/certusone/wormhole/node/cmd/spy 31.1 +github.com/certusone/wormhole/node/cmd/guardiand 21.5 +github.com/certusone/wormhole/node/cmd/spy 31.2 github.com/certusone/wormhole/node/cmd/txverifier 0.0 github.com/certusone/wormhole/node/hack/accountant 0.0 github.com/certusone/wormhole/node/hack/evm_test 0.0 @@ -22,21 +22,22 @@ github.com/certusone/wormhole/node/hack/repair_eth 0.0 github.com/certusone/wormhole/node/hack/repair_solana 0.0 github.com/certusone/wormhole/node/hack/solana 0.0 github.com/certusone/wormhole/node/pkg/accountant 49.8 -github.com/certusone/wormhole/node/pkg/adminrpc 49.2 +github.com/certusone/wormhole/node/pkg/adminrpc 50.5 github.com/certusone/wormhole/node/pkg/altpub 87.4 -github.com/certusone/wormhole/node/pkg/common 62.5 +github.com/certusone/wormhole/node/pkg/common 69.1 github.com/certusone/wormhole/node/pkg/db 76.6 github.com/certusone/wormhole/node/pkg/devnet 41.2 -github.com/certusone/wormhole/node/pkg/governor 49.7 -github.com/certusone/wormhole/node/pkg/guardiansigner 42.3 +github.com/certusone/wormhole/node/pkg/governor 49.6 +github.com/certusone/wormhole/node/pkg/guardiansigner 39.7 github.com/certusone/wormhole/node/pkg/gwrelayer 31.9 -github.com/certusone/wormhole/node/pkg/manager 23.3 +github.com/certusone/wormhole/node/pkg/manager 29.3 github.com/certusone/wormhole/node/pkg/manager/delegatedmanagersetabi 0.0 -github.com/certusone/wormhole/node/pkg/manager/dogecoin 93.8 -github.com/certusone/wormhole/node/pkg/node 65.2 +github.com/certusone/wormhole/node/pkg/manager/dogecoin 94.0 +github.com/certusone/wormhole/node/pkg/manager/xrpl 91.9 +github.com/certusone/wormhole/node/pkg/node 65.9 github.com/certusone/wormhole/node/pkg/notary 36.9 github.com/certusone/wormhole/node/pkg/p2p 40.3 -github.com/certusone/wormhole/node/pkg/processor 10.2 +github.com/certusone/wormhole/node/pkg/processor 25.0 github.com/certusone/wormhole/node/pkg/proto/gossip/v1 0.0 github.com/certusone/wormhole/node/pkg/proto/node/v1 0.0 github.com/certusone/wormhole/node/pkg/proto/prometheus/v1 0.0 @@ -46,6 +47,7 @@ github.com/certusone/wormhole/node/pkg/publicrpc 12.4 github.com/certusone/wormhole/node/pkg/query 70.2 github.com/certusone/wormhole/node/pkg/random 0.0 github.com/certusone/wormhole/node/pkg/readiness 0.0 +github.com/certusone/wormhole/node/pkg/suiclient 71.7 github.com/certusone/wormhole/node/pkg/supervisor 0.0 github.com/certusone/wormhole/node/pkg/telemetry 16.0 github.com/certusone/wormhole/node/pkg/telemetry/prom_remote_write 59.5 @@ -53,9 +55,9 @@ github.com/certusone/wormhole/node/pkg/txverifier 54.7 github.com/certusone/wormhole/node/pkg/version 0.0 github.com/certusone/wormhole/node/pkg/watchers/algorand 30.4 github.com/certusone/wormhole/node/pkg/watchers/aptos 29.8 -github.com/certusone/wormhole/node/pkg/watchers/cosmwasm 0.0 -github.com/certusone/wormhole/node/pkg/watchers/evm 23.6 -github.com/certusone/wormhole/node/pkg/watchers/evm/connectors 43.6 +github.com/certusone/wormhole/node/pkg/watchers/cosmwasm 2.3 +github.com/certusone/wormhole/node/pkg/watchers/evm 23.5 +github.com/certusone/wormhole/node/pkg/watchers/evm/connectors 59.3 github.com/certusone/wormhole/node/pkg/watchers/evm/connectors/delegated_guardians 0.0 github.com/certusone/wormhole/node/pkg/watchers/evm/connectors/ethabi 0.0 github.com/certusone/wormhole/node/pkg/watchers/evm/custom_consistency_level_abi 0.0 @@ -63,11 +65,13 @@ github.com/certusone/wormhole/node/pkg/watchers/evm/finalizers/optimismctcabi 0. github.com/certusone/wormhole/node/pkg/watchers/evm/verify_chain_config 0.0 github.com/certusone/wormhole/node/pkg/watchers/ibc 22.0 github.com/certusone/wormhole/node/pkg/watchers/mock 0.0 -github.com/certusone/wormhole/node/pkg/watchers/near 75.7 +github.com/certusone/wormhole/node/pkg/watchers/near 76.0 github.com/certusone/wormhole/node/pkg/watchers/near/nearapi 61.8 github.com/certusone/wormhole/node/pkg/watchers/near/nearapi/mock 0.0 -github.com/certusone/wormhole/node/pkg/watchers/solana 39.9 -github.com/certusone/wormhole/node/pkg/watchers/sui 7.9 -github.com/certusone/wormhole/node/pkg/wormconn 0.0 +github.com/certusone/wormhole/node/pkg/watchers/solana 41.0 +github.com/certusone/wormhole/node/pkg/watchers/sui 26.9 +github.com/certusone/wormhole/node/pkg/watchers/xrpl 72.8 +github.com/certusone/wormhole/node/pkg/watchers/xrpl/currencycodec 100.0 +github.com/certusone/wormhole/node/pkg/wormconn 2.3 github.com/wormhole-foundation/wormhole/sdk 70.2 -github.com/wormhole-foundation/wormhole/sdk/vaa 61.4 +github.com/wormhole-foundation/wormhole/sdk/vaa 66.8 diff --git a/devnet/node.yaml b/devnet/node.yaml index 62f01a2c84c..bbc9f53c0b5 100644 --- a/devnet/node.yaml +++ b/devnet/node.yaml @@ -50,6 +50,7 @@ spec: - name: node-wormchain-key secret: secretName: node-wormchain-key + defaultMode: 0600 optional: false items: - key: accountantKey0 diff --git a/node/Dockerfile b/node/Dockerfile index 07eba4e4ef3..1556046cba6 100644 --- a/node/Dockerfile +++ b/node/Dockerfile @@ -11,6 +11,7 @@ RUN --mount=type=cache,target=/root/.cache --mount=type=cache,target=/go \ cd tools/ && go build -mod=readonly -o /dlv github.com/go-delve/delve/cmd/dlv COPY node node +RUN chmod 0600 node/cmd/ccq/devnet.signing.key COPY sdk sdk COPY wormchain wormchain diff --git a/node/pkg/common/armoredKey.go b/node/pkg/common/armoredKey.go index cc0dcf81557..939f6658013 100644 --- a/node/pkg/common/armoredKey.go +++ b/node/pkg/common/armoredKey.go @@ -24,10 +24,15 @@ func LoadGuardianKey(filename string, unsafeDevMode bool) (*ecdsa.PrivateKey, er // LoadArmoredKey loads a serialized key from disk. func LoadArmoredKey(filename string, blockType string, unsafeDevMode bool) (*ecdsa.PrivateKey, error) { + if err := ValidatePrivateKeyFilePermissions(filename); err != nil { + return nil, err + } + f, err := os.Open(filename) if err != nil { return nil, fmt.Errorf("failed to open file: %w", err) } + defer f.Close() p, err := armor.Decode(f) if err != nil { @@ -61,6 +66,20 @@ func LoadArmoredKey(filename string, blockType string, unsafeDevMode bool) (*ecd return gk, nil } +// ValidatePrivateKeyFilePermissions rejects regular private-key files that are readable by group or other users. +func ValidatePrivateKeyFilePermissions(filename string) error { + info, err := os.Stat(filename) + if err != nil { + return fmt.Errorf("failed to stat private key file: %w", err) + } + + if info.Mode().IsRegular() && info.Mode().Perm()&0077 != 0 { + return fmt.Errorf("private key file %q has insecure permissions %o; expected no group or other permissions", filename, info.Mode().Perm()) + } + + return nil +} + // WriteArmoredKey serializes a key and writes it to disk. func WriteArmoredKey(key *ecdsa.PrivateKey, description string, filename string, blockType string, unsafe bool) error { if _, err := os.Stat(filename); !os.IsNotExist(err) { diff --git a/node/pkg/common/armoredKey_test.go b/node/pkg/common/armoredKey_test.go new file mode 100644 index 00000000000..70c99f07db7 --- /dev/null +++ b/node/pkg/common/armoredKey_test.go @@ -0,0 +1,36 @@ +package common + +import ( + "os" + "testing" + + "github.com/ethereum/go-ethereum/crypto" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadArmoredKeyRejectsUnsafePermissions(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + keyPath := t.TempDir() + "/guardian.key" + require.NoError(t, WriteArmoredKey(key, "test key", keyPath, GuardianKeyArmoredBlock, false)) + require.NoError(t, os.Chmod(keyPath, 0644)) + + loadedKey, err := LoadArmoredKey(keyPath, GuardianKeyArmoredBlock, false) + + require.Error(t, err) + assert.Nil(t, loadedKey) + assert.Contains(t, err.Error(), "insecure permissions") +} + +func TestLoadArmoredKeyAllowsOwnerOnlyPermissions(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + keyPath := t.TempDir() + "/guardian.key" + require.NoError(t, WriteArmoredKey(key, "test key", keyPath, GuardianKeyArmoredBlock, false)) + + loadedKey, err := LoadArmoredKey(keyPath, GuardianKeyArmoredBlock, false) + + require.NoError(t, err) + assert.Equal(t, key, loadedKey) +} diff --git a/node/pkg/common/scissors.go b/node/pkg/common/scissors.go index aab8b61febe..864ef9e0186 100644 --- a/node/pkg/common/scissors.go +++ b/node/pkg/common/scissors.go @@ -65,12 +65,12 @@ func StartRunnable(ctx context.Context, errC chan error, catchPanics bool, name default: err = fmt.Errorf("%s: %v", name, x) } + ScissorsPanicsCaught.WithLabelValues(name).Inc() // We don't want this to hang if the listener has already gone away. select { case errC <- err: default: } - ScissorsPanicsCaught.WithLabelValues(name).Inc() } }() @@ -83,11 +83,11 @@ func StartRunnable(ctx context.Context, errC chan error, catchPanics bool, name func startRunnable(ctx context.Context, errC chan error, name string, runnable supervisor.Runnable) { err := runnable(ctx) if err != nil { + ScissorsErrorsCaught.WithLabelValues(name).Inc() // We don't want this to hang if the listener has already gone away. select { case errC <- err: default: } - ScissorsErrorsCaught.WithLabelValues(name).Inc() } } diff --git a/node/pkg/guardiansigner/filesigner.go b/node/pkg/guardiansigner/filesigner.go index c2bd435eee3..abdfa5880b5 100644 --- a/node/pkg/guardiansigner/filesigner.go +++ b/node/pkg/guardiansigner/filesigner.go @@ -3,17 +3,12 @@ package guardiansigner import ( "context" "crypto/ecdsa" - "errors" "fmt" - "os" "github.com/ethereum/go-ethereum/crypto" ethcrypto "github.com/ethereum/go-ethereum/crypto" - "google.golang.org/protobuf/proto" "github.com/certusone/wormhole/node/pkg/common" - nodev1 "github.com/certusone/wormhole/node/pkg/proto/node/v1" - "golang.org/x/crypto/openpgp/armor" //nolint:staticcheck // Package is deprecated but we need it in the codebase still. ) // FileSigner is a signer that loads a guardian key from a file. The URI is expected to be @@ -35,38 +30,9 @@ func NewFileSigner(_ context.Context, unsafeDevMode bool, signerKeyPath string) keyPath: signerKeyPath, } - f, err := os.Open(signerKeyPath) + gk, err := common.LoadArmoredKey(signerKeyPath, GuardianKeyArmoredBlock, unsafeDevMode) if err != nil { - return nil, fmt.Errorf("failed to open file: %w", err) - } - - p, err := armor.Decode(f) - if err != nil { - return nil, fmt.Errorf("failed to read armored file: %w", err) - } - - if p.Type != GuardianKeyArmoredBlock { - return nil, fmt.Errorf("invalid block type: %s", p.Type) - } - - b, err := common.SafeRead(p.Body) - if err != nil { - return nil, fmt.Errorf("failed to read file: %w", err) - } - - var m nodev1.GuardianKey - err = proto.Unmarshal(b, &m) - if err != nil { - return nil, fmt.Errorf("failed to deserialize protobuf: %w", err) - } - - if !unsafeDevMode && m.UnsafeDeterministicKey { - return nil, errors.New("refusing to use deterministic key in production") - } - - gk, err := ethcrypto.ToECDSA(m.Data) - if err != nil { - return nil, fmt.Errorf("failed to deserialize raw key data: %w", err) + return nil, err } fileSigner.privateKey = gk diff --git a/node/pkg/guardiansigner/guardiansigner_test.go b/node/pkg/guardiansigner/guardiansigner_test.go index 2351b293ed2..d75a0062c8c 100644 --- a/node/pkg/guardiansigner/guardiansigner_test.go +++ b/node/pkg/guardiansigner/guardiansigner_test.go @@ -3,8 +3,10 @@ package guardiansigner import ( "context" "encoding/hex" + "os" "testing" + "github.com/certusone/wormhole/node/pkg/common" "github.com/ethereum/go-ethereum/crypto" ethcrypto "github.com/ethereum/go-ethereum/crypto" "github.com/stretchr/testify/assert" @@ -61,8 +63,12 @@ func TestFileSignerNonExistentFile(t *testing.T) { func TestFileSigner(t *testing.T) { ctx := context.Background() - fileUri := "file://../query/dev.guardian.key" - expectedEthAddress := "0xbeFA429d57cD18b7F8A4d91A2da9AB4AF05d0FBe" + key, err := crypto.GenerateKey() + require.NoError(t, err) + keyPath := t.TempDir() + "/guardian.key" + require.NoError(t, common.WriteArmoredKey(key, "test key", keyPath, GuardianKeyArmoredBlock, false)) + fileUri := "file://" + keyPath + expectedEthAddress := ethcrypto.PubkeyToAddress(key.PublicKey).Hex() // For each file signer generation attempt, check: // That the signer returned is not nil @@ -102,6 +108,43 @@ func TestFileSigner(t *testing.T) { } +func TestFileSignerRejectsUnsafePermissions(t *testing.T) { + key, err := crypto.GenerateKey() + require.NoError(t, err) + keyPath := t.TempDir() + "/guardian.key" + require.NoError(t, common.WriteArmoredKey(key, "test key", keyPath, GuardianKeyArmoredBlock, false)) + require.NoError(t, os.Chmod(keyPath, 0644)) + + fileSigner, err := NewFileSigner(context.Background(), true, keyPath) + + require.Error(t, err) + assert.Nil(t, fileSigner) + assert.Contains(t, err.Error(), "insecure permissions") +} + +func TestGeneratedSigner(t *testing.T) { + ctx := context.Background() + key, err := crypto.GenerateKey() + require.NoError(t, err) + + signer, err := GenerateSignerWithPrivatekeyUnsafe(key) + require.NoError(t, err) + assert.Equal(t, "generated", signer.TypeAsString()) + assert.Equal(t, key.PublicKey, signer.PublicKey(ctx)) + + hash := crypto.Keccak256Hash([]byte("data")) + sig, err := signer.Sign(ctx, hash.Bytes()) + require.NoError(t, err) + + valid, err := signer.Verify(ctx, sig, hash.Bytes()) + require.NoError(t, err) + assert.True(t, valid) + + valid, err = signer.Verify(ctx, sig, crypto.Keccak256Hash([]byte("other data")).Bytes()) + require.NoError(t, err) + assert.False(t, valid) +} + func TestAmazonKmsAdjustBufferSize(t *testing.T) { bytes_30_null_0102, _ := hex.DecodeString("0000000000000000000000000000000000000000000000000000000000000102") diff --git a/node/pkg/query/query_test.go b/node/pkg/query/query_test.go index 4ce0d57e4eb..9b9f035514a 100644 --- a/node/pkg/query/query_test.go +++ b/node/pkg/query/query_test.go @@ -7,6 +7,7 @@ import ( "encoding/hex" "fmt" "math" + "os" "strconv" "strings" "sync" @@ -417,7 +418,16 @@ func createQueryHandlerForTestWithoutPublisher(t *testing.T, ctx context.Context md := mockData{} var err error - md.sk, err = common.LoadGuardianKey("dev.guardian.key", true) + devKey, err := os.ReadFile("dev.guardian.key") + require.NoError(t, err) + devKeyFile, err := os.CreateTemp(t.TempDir(), "dev.guardian.key") + require.NoError(t, err) + _, err = devKeyFile.Write(devKey) + require.NoError(t, err) + require.NoError(t, devKeyFile.Close()) + devKeyPath := devKeyFile.Name() + + md.sk, err = common.LoadGuardianKey(devKeyPath, true) require.NoError(t, err) require.NotNil(t, md.sk) diff --git a/node/pkg/wormconn/wormchain_key.go b/node/pkg/wormconn/wormchain_key.go index c2d28cc6682..14441053d11 100644 --- a/node/pkg/wormconn/wormchain_key.go +++ b/node/pkg/wormconn/wormchain_key.go @@ -3,11 +3,16 @@ package wormconn import ( "os" + "github.com/certusone/wormhole/node/pkg/common" "github.com/cosmos/cosmos-sdk/crypto" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" ) func LoadWormchainPrivKey(path string, passPhrase string) (cryptotypes.PrivKey, error) { + if err := common.ValidatePrivateKeyFilePermissions(path); err != nil { + return nil, err + } + armor, err := os.ReadFile(path) if err != nil { return nil, err diff --git a/node/pkg/wormconn/wormchain_key_test.go b/node/pkg/wormconn/wormchain_key_test.go new file mode 100644 index 00000000000..8f2f47de544 --- /dev/null +++ b/node/pkg/wormconn/wormchain_key_test.go @@ -0,0 +1,20 @@ +package wormconn + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLoadWormchainPrivKeyRejectsUnsafePermissions(t *testing.T) { + keyPath := t.TempDir() + "/wormchain.key" + require.NoError(t, os.WriteFile(keyPath, []byte("not a valid encrypted key"), 0644)) // #nosec G306 -- This test verifies unsafe key-file permissions are rejected. + + key, err := LoadWormchainPrivKey(keyPath, "passphrase") + + require.Error(t, err) + assert.Nil(t, key) + assert.Contains(t, err.Error(), "insecure permissions") +}