From 01e9125761f96b1d0d693859108cb7f912882edc Mon Sep 17 00:00:00 2001 From: james-prysm <90280386+james-prysm@users.noreply.github.com> Date: Thu, 24 Feb 2022 11:24:11 -0500 Subject: [PATCH] web3signer: url parsing bug (#10278) * adding in fixes for url * fixing gazelle * fixing wrong keymanager kind * adding required scheme to urls * fixing another unit test * removing unused file * adding new commit to retrigger deepsource ci --- .../remote-web3signer/internal/client.go | 5 +- .../remote-web3signer/keymanager.go | 2 +- .../remote-web3signer/keymanager_test.go | 16 +-- validator/node/BUILD.bazel | 3 + validator/node/node.go | 18 +-- validator/node/node_test.go | 124 ++++++++++++++++++ validator/rpc/standard_api_test.go | 8 +- 7 files changed, 153 insertions(+), 23 deletions(-) diff --git a/validator/keymanager/remote-web3signer/internal/client.go b/validator/keymanager/remote-web3signer/internal/client.go index e75fdef91..5f4f6ee89 100644 --- a/validator/keymanager/remote-web3signer/internal/client.go +++ b/validator/keymanager/remote-web3signer/internal/client.go @@ -42,10 +42,13 @@ type ApiClient struct { // NewApiClient method instantiates a new ApiClient object. func NewApiClient(baseEndpoint string) (*ApiClient, error) { - u, err := url.Parse(baseEndpoint) + u, err := url.ParseRequestURI(baseEndpoint) if err != nil { return nil, errors.Wrap(err, "invalid format, unable to parse url") } + if u.Scheme == "" || u.Host == "" { + return nil, fmt.Errorf("web3signer url must be in the format of http(s)://host:port url used: %v", baseEndpoint) + } return &ApiClient{ BaseURL: u, RestClient: &http.Client{}, diff --git a/validator/keymanager/remote-web3signer/keymanager.go b/validator/keymanager/remote-web3signer/keymanager.go index 9acd9e2ce..e2812382c 100644 --- a/validator/keymanager/remote-web3signer/keymanager.go +++ b/validator/keymanager/remote-web3signer/keymanager.go @@ -78,7 +78,7 @@ func (km *Keymanager) FetchValidatingPublicKeys(ctx context.Context) ([][fieldpa providedPublicKeys, err := km.client.GetPublicKeys(ctx, km.publicKeysURL) if err != nil { erroredResponsesTotal.Inc() - return nil, err + return nil, errors.Wrap(err, fmt.Sprintf("could not get public keys from remote server url: %v", km.publicKeysURL)) } km.providedPublicKeys = providedPublicKeys } diff --git a/validator/keymanager/remote-web3signer/keymanager_test.go b/validator/keymanager/remote-web3signer/keymanager_test.go index f396a1dc9..17790bb30 100644 --- a/validator/keymanager/remote-web3signer/keymanager_test.go +++ b/validator/keymanager/remote-web3signer/keymanager_test.go @@ -55,9 +55,9 @@ func TestKeymanager_Sign(t *testing.T) { fmt.Printf("error: %v", err) } config := &SetupConfig{ - BaseEndpoint: "example.com", + BaseEndpoint: "http://example.com", GenesisValidatorsRoot: root, - PublicKeysURL: "example2.com/api/v1/eth2/publicKeys", + PublicKeysURL: "http://example2.com/api/v1/eth2/publicKeys", } km, err := NewKeymanager(ctx, config) if err != nil { @@ -189,7 +189,7 @@ func TestKeymanager_FetchValidatingPublicKeys_HappyPath_WithKeyList(t *testing.T fmt.Printf("error: %v", err) } config := &SetupConfig{ - BaseEndpoint: "example.com", + BaseEndpoint: "http://example.com", GenesisValidatorsRoot: root, ProvidedPublicKeys: keys, } @@ -223,9 +223,9 @@ func TestKeymanager_FetchValidatingPublicKeys_HappyPath_WithExternalURL(t *testi fmt.Printf("error: %v", err) } config := &SetupConfig{ - BaseEndpoint: "example.com", + BaseEndpoint: "http://example.com", GenesisValidatorsRoot: root, - PublicKeysURL: "example2.com/api/v1/eth2/publicKeys", + PublicKeysURL: "http://example2.com/api/v1/eth2/publicKeys", } km, err := NewKeymanager(ctx, config) if err != nil { @@ -252,9 +252,9 @@ func TestKeymanager_FetchValidatingPublicKeys_WithExternalURL_ThrowsError(t *tes fmt.Printf("error: %v", err) } config := &SetupConfig{ - BaseEndpoint: "example.com", + BaseEndpoint: "http://example.com", GenesisValidatorsRoot: root, - PublicKeysURL: "example2.com/api/v1/eth2/publicKeys", + PublicKeysURL: "http://example2.com/api/v1/eth2/publicKeys", } km, err := NewKeymanager(ctx, config) if err != nil { @@ -264,5 +264,5 @@ func TestKeymanager_FetchValidatingPublicKeys_WithExternalURL_ThrowsError(t *tes resp, err := km.FetchValidatingPublicKeys(ctx) assert.NotNil(t, err) assert.Nil(t, resp) - assert.Equal(t, fmt.Errorf("mock error"), err) + assert.Equal(t, "could not get public keys from remote server url: http://example2.com/api/v1/eth2/publicKeys: mock error", fmt.Sprintf("%v", err)) } diff --git a/validator/node/BUILD.bazel b/validator/node/BUILD.bazel index f780c9d1b..e8db9be68 100644 --- a/validator/node/BUILD.bazel +++ b/validator/node/BUILD.bazel @@ -7,10 +7,13 @@ go_test( embed = [":go_default_library"], deps = [ "//cmd/validator/flags:go_default_library", + "//encoding/bytesutil:go_default_library", "//testing/require:go_default_library", "//validator/accounts:go_default_library", "//validator/accounts/wallet:go_default_library", "//validator/keymanager:go_default_library", + "//validator/keymanager/remote-web3signer:go_default_library", + "@com_github_ethereum_go_ethereum//common/hexutil:go_default_library", "@com_github_sirupsen_logrus//hooks/test:go_default_library", "@com_github_urfave_cli_v2//:go_default_library", ], diff --git a/validator/node/node.go b/validator/node/node.go index 1f5cb81d2..a25d3c9b6 100644 --- a/validator/node/node.go +++ b/validator/node/node.go @@ -429,26 +429,26 @@ func web3SignerConfig(cliCtx *cli.Context) (*remote_web3signer.SetupConfig, erro if cliCtx.IsSet(flags.Web3SignerURLFlag.Name) && cliCtx.IsSet(flags.Web3SignerPublicValidatorKeysFlag.Name) { urlStr := cliCtx.String(flags.Web3SignerURLFlag.Name) publicKeysStr := cliCtx.String(flags.Web3SignerPublicValidatorKeysFlag.Name) - u, err := url.Parse(urlStr) + u, err := url.ParseRequestURI(urlStr) if err != nil { return nil, errors.Wrapf(err, "web3signer url %s is invalid", urlStr) } + if u.Scheme == "" || u.Host == "" { + return nil, fmt.Errorf("web3signer url must be in the format of http(s)://host:port url used: %v", urlStr) + } web3signerConfig = &remote_web3signer.SetupConfig{ BaseEndpoint: u.String(), GenesisValidatorsRoot: nil, } - u, err = url.Parse(publicKeysStr) - if err != nil { - return nil, errors.Wrapf(err, "could not parse %s as a URL for web3signer remote public keys", publicKeysStr) - } - if u != nil { + pURL, err := url.ParseRequestURI(publicKeysStr) + if err == nil && pURL.Scheme != "" && pURL.Host != "" { web3signerConfig.PublicKeysURL = publicKeysStr } else { var validatorKeys [][48]byte for _, key := range strings.Split(publicKeysStr, ",") { - decodedKey, err := hexutil.Decode(key) - if err != nil { - return nil, errors.Wrapf(err, "could not decode public key for web3signer: %s", key) + decodedKey, decodeErr := hexutil.Decode(key) + if decodeErr != nil { + return nil, errors.Wrapf(decodeErr, "could not decode public key for web3signer: %s", key) } validatorKeys = append(validatorKeys, bytesutil.ToBytes48(decodedKey)) } diff --git a/validator/node/node_test.go b/validator/node/node_test.go index 6e79b1df0..852358a2a 100644 --- a/validator/node/node_test.go +++ b/validator/node/node_test.go @@ -3,16 +3,20 @@ package node import ( "context" "flag" + "fmt" "io/ioutil" "os" "path/filepath" "testing" + "github.com/ethereum/go-ethereum/common/hexutil" "github.com/prysmaticlabs/prysm/cmd/validator/flags" + "github.com/prysmaticlabs/prysm/encoding/bytesutil" "github.com/prysmaticlabs/prysm/testing/require" "github.com/prysmaticlabs/prysm/validator/accounts" "github.com/prysmaticlabs/prysm/validator/accounts/wallet" "github.com/prysmaticlabs/prysm/validator/keymanager" + remote_web3signer "github.com/prysmaticlabs/prysm/validator/keymanager/remote-web3signer" logTest "github.com/sirupsen/logrus/hooks/test" "github.com/urfave/cli/v2" ) @@ -60,3 +64,123 @@ func TestClearDB(t *testing.T) { require.NoError(t, clearDB(context.Background(), tmp, true)) require.LogsContain(t, hook, "Removing database") } + +// TestWeb3SignerConfig tests the web3 signer config returns the correct values. +func TestWeb3SignerConfig(t *testing.T) { + pubkey1decoded, err := hexutil.Decode("0xa99a76ed7796f7be22d5b7e85deeb7c5677e88e511e0b337618f8c4eb61349b4bf2d153f649f7b53359fe8b94a38e44c") + require.NoError(t, err) + bytepubkey1 := bytesutil.ToBytes48(pubkey1decoded) + + pubkey2decoded, err := hexutil.Decode("0xb89bebc699769726a318c8e9971bd3171297c61aea4a6578a7a4f94b547dcba5bac16a89108b6b6a1fe3695d1a874a0b") + require.NoError(t, err) + bytepubkey2 := bytesutil.ToBytes48(pubkey2decoded) + + type args struct { + baseURL string + publicKeysOrURL string + } + tests := []struct { + name string + args args + want *remote_web3signer.SetupConfig + wantErrMsg string + }{ + { + name: "happy path with public keys", + args: args{ + baseURL: "http://localhost:8545", + publicKeysOrURL: "0xa99a76ed7796f7be22d5b7e85deeb7c5677e88e511e0b337618f8c4eb61349b4bf2d153f649f7b53359fe8b94a38e44c," + + "0xb89bebc699769726a318c8e9971bd3171297c61aea4a6578a7a4f94b547dcba5bac16a89108b6b6a1fe3695d1a874a0b", + }, + want: &remote_web3signer.SetupConfig{ + BaseEndpoint: "http://localhost:8545", + GenesisValidatorsRoot: nil, + PublicKeysURL: "", + ProvidedPublicKeys: [][48]byte{ + bytepubkey1, + bytepubkey2, + }, + }, + }, + { + name: "happy path with external url", + args: args{ + baseURL: "http://localhost:8545", + publicKeysOrURL: "http://localhost:8545/api/v1/eth2/publicKeys", + }, + want: &remote_web3signer.SetupConfig{ + BaseEndpoint: "http://localhost:8545", + GenesisValidatorsRoot: nil, + PublicKeysURL: "http://localhost:8545/api/v1/eth2/publicKeys", + ProvidedPublicKeys: nil, + }, + }, + { + name: "Bad base URL", + args: args{ + baseURL: "0xa99a76ed7796f7be22d5b7e85deeb7c5677e88,", + publicKeysOrURL: "0xa99a76ed7796f7be22d5b7e85deeb7c5677e88e511e0b337618f8c4eb61349b4bf2d153f649f7b53359fe8b94a38e44c," + + "0xb89bebc699769726a318c8e9971bd3171297c61aea4a6578a7a4f94b547dcba5bac16a89108b6b6a1fe3695d1a874a0b", + }, + want: nil, + wantErrMsg: "web3signer url 0xa99a76ed7796f7be22d5b7e85deeb7c5677e88, is invalid: parse \"0xa99a76ed7796f7be22d5b7e85deeb7c5677e88,\": invalid URI for request", + }, + { + name: "Bad publicKeys", + args: args{ + baseURL: "http://localhost:8545", + publicKeysOrURL: "0xa99a76ed7796f7be22c," + + "0xb89bebc699769726a318c8e9971bd3171297c61aea4a6578a7a4f94b547dcba5bac16a89108b6b6a1fe3695d1a874a0b", + }, + want: nil, + wantErrMsg: "could not decode public key for web3signer: 0xa99a76ed7796f7be22c: hex string of odd length", + }, + { + name: "Bad publicKeysURL", + args: args{ + baseURL: "http://localhost:8545", + publicKeysOrURL: "localhost", + }, + want: nil, + wantErrMsg: "could not decode public key for web3signer: localhost: hex string without 0x prefix", + }, + { + name: "Base URL missing scheme or host", + args: args{ + baseURL: "localhost:8545", + publicKeysOrURL: "localhost", + }, + want: nil, + wantErrMsg: "web3signer url must be in the format of http(s)://host:port url used: localhost:8545", + }, + { + name: "Public Keys URL missing scheme or host", + args: args{ + baseURL: "http://localhost:8545", + publicKeysOrURL: "localhost:8545", + }, + want: nil, + wantErrMsg: "could not decode public key for web3signer: localhost:8545: hex string without 0x prefix", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := web3SignerConfig(newWeb3SignerCli(t, tt.args.baseURL, tt.args.publicKeysOrURL)) + if (tt.wantErrMsg != "") && (tt.wantErrMsg != fmt.Sprintf("%v", err)) { + t.Errorf("web3SignerConfig error = %v, wantErrMsg = %v", err, tt.wantErrMsg) + return + } + require.DeepEqual(t, got, tt.want) + }) + } +} + +func newWeb3SignerCli(t *testing.T, baseUrl string, publicKeysOrURL string) *cli.Context { + app := cli.App{} + set := flag.NewFlagSet("test", 0) + set.String("validators-external-signer-url", baseUrl, "baseUrl") + set.String("validators-external-signer-public-keys", publicKeysOrURL, "publicKeys or URL") + require.NoError(t, set.Set(flags.Web3SignerURLFlag.Name, baseUrl)) + require.NoError(t, set.Set(flags.Web3SignerPublicValidatorKeysFlag.Name, publicKeysOrURL)) + return cli.NewContext(&app, set, nil) +} diff --git a/validator/rpc/standard_api_test.go b/validator/rpc/standard_api_test.go index d01b77d31..92b5fc3ac 100644 --- a/validator/rpc/standard_api_test.go +++ b/validator/rpc/standard_api_test.go @@ -239,9 +239,9 @@ func TestServer_ImportKeystores_WrongKeymanagerKind(t *testing.T) { root := make([]byte, fieldparams.RootLength) root[0] = 1 km, err := w.InitializeKeymanager(ctx, iface.InitKeymanagerConfig{ListenForChanges: false, Web3SignerConfig: &remote_web3signer.SetupConfig{ - BaseEndpoint: "example.com", + BaseEndpoint: "http://example.com", GenesisValidatorsRoot: root, - PublicKeysURL: "example.com/public_keys", + PublicKeysURL: "http://example.com/public_keys", }}) require.NoError(t, err) vs, err := client.NewValidatorService(ctx, &client.Config{ @@ -455,9 +455,9 @@ func TestServer_DeleteKeystores_WrongKeymanagerKind(t *testing.T) { root[0] = 1 km, err := w.InitializeKeymanager(ctx, iface.InitKeymanagerConfig{ListenForChanges: false, Web3SignerConfig: &remote_web3signer.SetupConfig{ - BaseEndpoint: "example.com", + BaseEndpoint: "http://example.com", GenesisValidatorsRoot: root, - PublicKeysURL: "example.com/public_keys", + PublicKeysURL: "http://example.com/public_keys", }}) require.NoError(t, err) vs, err := client.NewValidatorService(ctx, &client.Config{