Refactor HD Wallets for Enhanced Security (#7821)

* begin hd wallet refactor

* further simplify the new derived keymanager

* make it almost a full wrapper around an imported keymanager

* fix up the EIP test

* deprecated derived

* fixing keymanager tests

* fix up derived tests

* refactor initialize keymanager

* simplify hd

* pass some tests

* pass accounts list test

* gaz

* regenerate protos without create account privilege

* enforce account recovery on wallet create

* allow accounts delete to work

* remove mentions of accounts create

* resolve comments and go mod

* fix up tests

* build fixes

* remove insecure warning

* revert

* fix proto file

* remove create account message

* gaz

* remove account create

* update web api protos

* fix up imports

* change func sig

* tidy

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

validator/rpc/BUILD.bazel

@@ -25,7 +25,6 @@ go_library(
         "//shared/timeutils:go_default_library",
         "//shared/traceutil:go_default_library",
         "//validator/accounts:go_default_library",
-        "//validator/accounts/iface:go_default_library",
         "//validator/accounts/wallet:go_default_library",
         "//validator/client:go_default_library",
         "//validator/db:go_default_library",
@@ -39,7 +38,6 @@ go_library(
         "@com_github_grpc_ecosystem_go_grpc_middleware//tracing/opentracing:go_default_library",
         "@com_github_grpc_ecosystem_go_grpc_prometheus//:go_default_library",
         "@com_github_pkg_errors//:go_default_library",
-        "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
         "@com_github_tyler_smith_go_bip39//:go_default_library",
         "@io_opencensus_go//plugin/ocgrpc:go_default_library",
@@ -69,12 +67,10 @@ go_test(
         "//shared/bls:go_default_library",
         "//shared/event:go_default_library",
         "//shared/fileutil:go_default_library",
-        "//shared/params:go_default_library",
         "//shared/testutil/assert:go_default_library",
         "//shared/testutil/require:go_default_library",
         "//shared/timeutils:go_default_library",
         "//validator/accounts:go_default_library",
-        "//validator/accounts/iface:go_default_library",
         "//validator/accounts/wallet:go_default_library",
         "//validator/client:go_default_library",
         "//validator/db/testing:go_default_library",

validator/rpc/accounts.go

@@ -4,45 +4,16 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/pkg/errors"
-	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	pb "github.com/prysmaticlabs/prysm/proto/validator/accounts/v2"
 	"github.com/prysmaticlabs/prysm/shared/cmd"
 	"github.com/prysmaticlabs/prysm/shared/pagination"
 	"github.com/prysmaticlabs/prysm/shared/petnames"
-	"github.com/prysmaticlabs/prysm/validator/accounts"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/derived"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
 
-type accountCreator interface {
-	CreateAccount(ctx context.Context) ([]byte, *ethpb.Deposit_Data, error)
-}
-
-// CreateAccount allows creation of a new account in a user's wallet via RPC.
-func (s *Server) CreateAccount(ctx context.Context, req *pb.CreateAccountRequest) (*pb.DepositDataResponse, error) {
-	if !s.walletInitialized {
-		return nil, status.Error(codes.FailedPrecondition, "Wallet not yet initialized")
-	}
-	km, ok := s.keymanager.(*derived.Keymanager)
-	if !ok {
-		return nil, status.Error(codes.InvalidArgument, "Only HD wallets can create accounts")
-	}
-	dataList := make([]*pb.DepositDataResponse_DepositData, req.NumAccounts)
-	for i := uint64(0); i < req.NumAccounts; i++ {
-		data, err := createAccountWithDepositData(ctx, km)
-		if err != nil {
-			return nil, err
-		}
-		dataList[i] = data
-	}
-	return &pb.DepositDataResponse{
-		DepositDataList: dataList,
-	}, nil
-}
-
 // ListAccounts allows retrieval of validating keys and their petnames
 // for a user's wallet via RPC.
 func (s *Server) ListAccounts(ctx context.Context, req *pb.ListAccountsRequest) (*pb.ListAccountsResponse, error) {
@@ -88,18 +59,3 @@ func (s *Server) ListAccounts(ctx context.Context, req *pb.ListAccountsRequest)
 		NextPageToken: nextPageToken,
 	}, nil
 }
-
-func createAccountWithDepositData(ctx context.Context, km accountCreator) (*pb.DepositDataResponse_DepositData, error) {
-	// Create a new validator account using the specified keymanager.
-	_, depositData, err := km.CreateAccount(ctx)
-	if err != nil {
-		return nil, errors.Wrap(err, "could not create account in wallet")
-	}
-	data, err := accounts.DepositDataJSON(depositData)
-	if err != nil {
-		return nil, errors.Wrap(err, "could not create deposit data JSON")
-	}
-	return &pb.DepositDataResponse_DepositData{
-		Data: data,
-	}, nil
-}

validator/rpc/accounts_test.go

@@ -2,62 +2,23 @@ package rpc
 
 import (
 	"context"
-	"fmt"
 	"path/filepath"
 	"testing"
 
-	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	pb "github.com/prysmaticlabs/prysm/proto/validator/accounts/v2"
-	"github.com/prysmaticlabs/prysm/shared/bls"
-	"github.com/prysmaticlabs/prysm/shared/params"
 	"github.com/prysmaticlabs/prysm/shared/testutil/assert"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
 	"github.com/prysmaticlabs/prysm/validator/accounts"
-	"github.com/prysmaticlabs/prysm/validator/accounts/iface"
 	"github.com/prysmaticlabs/prysm/validator/accounts/wallet"
 	"github.com/prysmaticlabs/prysm/validator/flags"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/derived"
 )
 
-var defaultWalletPath = filepath.Join(flags.DefaultValidatorDir(), flags.WalletDefaultDirName)
-
-type mockAccountCreator struct {
-	data   *ethpb.Deposit_Data
-	pubKey []byte
-}
-
-func (m *mockAccountCreator) CreateAccount(ctx context.Context) ([]byte, *ethpb.Deposit_Data, error) {
-	return m.pubKey, m.data, nil
-}
-
-func TestServer_CreateAccount(t *testing.T) {
-	ctx := context.Background()
-	localWalletDir := setupWalletDir(t)
-	defaultWalletPath = localWalletDir
-	strongPass := "29384283xasjasd32%%&*@*#*"
-	// We attempt to create the wallet.
-	w, err := accounts.CreateWalletWithKeymanager(ctx, &accounts.CreateWalletConfig{
-		WalletCfg: &wallet.Config{
-			WalletDir:      defaultWalletPath,
-			KeymanagerKind: keymanager.Derived,
-			WalletPassword: strongPass,
-		},
-		SkipMnemonicConfirm: true,
-	})
-	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
-	require.NoError(t, err)
-	s := &Server{
-		keymanager:        km,
-		walletInitialized: true,
-		wallet:            w,
-	}
-	_, err = s.CreateAccount(ctx, &pb.CreateAccountRequest{})
-	require.NoError(t, err)
-}
+var (
+	defaultWalletPath = filepath.Join(flags.DefaultValidatorDir(), flags.WalletDefaultDirName)
+	testMnemonic      = "tumble turn jewel sudden social great water general cabin jacket bounce dry flip monster advance problem social half flee inform century chicken hard reason"
+)
 
 func TestServer_ListAccounts(t *testing.T) {
 	ctx := context.Background()
@@ -74,9 +35,7 @@ func TestServer_ListAccounts(t *testing.T) {
 		SkipMnemonicConfirm: true,
 	})
 	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err := w.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	s := &Server{
 		keymanager:        km,
@@ -84,20 +43,15 @@ func TestServer_ListAccounts(t *testing.T) {
 		wallet:            w,
 	}
 	numAccounts := 50
-	keys := make([][]byte, numAccounts)
-	for i := 0; i < numAccounts; i++ {
-		key, _, err := km.(*derived.Keymanager).CreateAccount(ctx)
-		require.NoError(t, err)
-		keys[i] = key
-	}
+	dr, ok := km.(*derived.Keymanager)
+	require.Equal(t, true, ok)
+	err = dr.RecoverAccountsFromMnemonic(ctx, testMnemonic, "", numAccounts)
+	require.NoError(t, err)
 	resp, err := s.ListAccounts(ctx, &pb.ListAccountsRequest{
 		PageSize: int32(numAccounts),
 	})
 	require.NoError(t, err)
 	require.Equal(t, len(resp.Accounts), numAccounts)
-	for i := 0; i < numAccounts; i++ {
-		assert.DeepEqual(t, resp.Accounts[i].ValidatingPublicKey, keys[i])
-	}
 
 	tests := []struct {
 		req *pb.ListAccountsRequest
@@ -131,36 +85,3 @@ func TestServer_ListAccounts(t *testing.T) {
 		assert.DeepEqual(t, res, test.res)
 	}
 }
-
-func Test_createAccountWithDepositData(t *testing.T) {
-	ctx := context.Background()
-	secretKey, err := bls.RandKey()
-	require.NoError(t, err)
-	pubKey := secretKey.PublicKey().Marshal()
-	m := &mockAccountCreator{
-		data: &ethpb.Deposit_Data{
-			PublicKey:             pubKey,
-			WithdrawalCredentials: make([]byte, 32),
-			Amount:                params.BeaconConfig().MaxEffectiveBalance,
-			Signature:             make([]byte, 96),
-		},
-		pubKey: pubKey,
-	}
-	rawResp, err := createAccountWithDepositData(ctx, m)
-	require.NoError(t, err)
-	assert.DeepEqual(
-		t, rawResp.Data["pubkey"], fmt.Sprintf("%x", pubKey),
-	)
-	assert.DeepEqual(
-		t, rawResp.Data["withdrawal_credentials"], fmt.Sprintf("%x", make([]byte, 32)),
-	)
-	assert.DeepEqual(
-		t, rawResp.Data["amount"], fmt.Sprintf("%d", params.BeaconConfig().MaxEffectiveBalance),
-	)
-	assert.DeepEqual(
-		t, rawResp.Data["signature"], fmt.Sprintf("%x", make([]byte, 96)),
-	)
-	assert.DeepEqual(
-		t, rawResp.Data["fork_version"], fmt.Sprintf("%x", params.BeaconConfig().GenesisForkVersion),
-	)
-}

validator/rpc/wallet.go

@@ -14,7 +14,6 @@ import (
 	pb "github.com/prysmaticlabs/prysm/proto/validator/accounts/v2"
 	"github.com/prysmaticlabs/prysm/shared/rand"
 	"github.com/prysmaticlabs/prysm/validator/accounts"
-	"github.com/prysmaticlabs/prysm/validator/accounts/iface"
 	"github.com/prysmaticlabs/prysm/validator/accounts/wallet"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/imported"
@@ -111,13 +110,12 @@ func (s *Server) CreateWallet(ctx context.Context, req *pb.CreateWalletRequest)
 		if req.Mnemonic == "" {
 			return nil, status.Error(codes.InvalidArgument, "Must include mnemonic in request")
 		}
-		_, depositData, err := accounts.RecoverWallet(ctx, &accounts.RecoverWalletConfig{
+		if _, err := accounts.RecoverWallet(ctx, &accounts.RecoverWalletConfig{
 			WalletDir:      walletDir,
 			WalletPassword: req.WalletPassword,
 			Mnemonic:       req.Mnemonic,
-			NumAccounts:    int64(req.NumAccounts),
-		})
-		if err != nil {
+			NumAccounts:    int(req.NumAccounts),
+		}); err != nil {
 			return nil, err
 		}
 		if err := s.initializeWallet(ctx, &wallet.Config{
@@ -128,24 +126,11 @@ func (s *Server) CreateWallet(ctx context.Context, req *pb.CreateWalletRequest)
 			return nil, err
 		}
 
-		depositDataList := make([]*pb.DepositDataResponse_DepositData, len(depositData))
-		for i, item := range depositData {
-			data, err := accounts.DepositDataJSON(item)
-			if err != nil {
-				return nil, err
-			}
-			depositDataList[i] = &pb.DepositDataResponse_DepositData{
-				Data: data,
-			}
-		}
 		return &pb.CreateWalletResponse{
 			Wallet: &pb.WalletResponse{
 				WalletPath:     walletDir,
 				KeymanagerKind: pb.KeymanagerKind_DERIVED,
 			},
-			AccountsCreated: &pb.DepositDataResponse{
-				DepositDataList: depositDataList,
-			},
 		}, nil
 	case pb.KeymanagerKind_REMOTE:
 		return nil, status.Error(codes.Unimplemented, "Remote keymanager not yet supported")
@@ -335,9 +320,7 @@ func (s *Server) initializeWallet(ctx context.Context, cfg *wallet.Config) error
 	}
 
 	s.walletInitialized = true
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err := w.InitializeKeymanager(ctx)
 	if err != nil {
 		return errors.Wrap(err, "could not initialize keymanager")
 	}

validator/rpc/wallet_test.go

@@ -15,7 +15,6 @@ import (
 	"github.com/prysmaticlabs/prysm/shared/testutil/assert"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
 	"github.com/prysmaticlabs/prysm/validator/accounts"
-	"github.com/prysmaticlabs/prysm/validator/accounts/iface"
 	"github.com/prysmaticlabs/prysm/validator/accounts/wallet"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/imported"
@@ -36,9 +35,7 @@ func createImportedWalletWithAccounts(t testing.TB, numAccounts int) (*Server, [
 		SkipMnemonicConfirm: true,
 	})
 	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err := w.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	ss := &Server{
 		keymanager:            km,
@@ -75,9 +72,7 @@ func createImportedWalletWithAccounts(t testing.TB, numAccounts int) (*Server, [
 		KeystoresPassword: strongPass,
 	})
 	require.NoError(t, err)
-	ss.keymanager, err = ss.wallet.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	ss.keymanager, err = ss.wallet.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	return ss, pubKeys
 }
@@ -198,9 +193,7 @@ func TestServer_WalletConfig(t *testing.T) {
 		SkipMnemonicConfirm: true,
 	})
 	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err := w.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	s.wallet = w
 	s.keymanager = km
@@ -272,9 +265,7 @@ func TestServer_ImportKeystores_FailedPreconditions_WrongKeymanagerKind(t *testi
 		SkipMnemonicConfirm: true,
 	})
 	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err := w.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	ss := &Server{
 		wallet:     w,
@@ -298,9 +289,7 @@ func TestServer_ImportKeystores_FailedPreconditions(t *testing.T) {
 		SkipMnemonicConfirm: true,
 	})
 	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err := w.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	ss := &Server{
 		keymanager: km,
@@ -336,9 +325,7 @@ func TestServer_ImportKeystores_OK(t *testing.T) {
 		SkipMnemonicConfirm: true,
 	})
 	require.NoError(t, err)
-	km, err := w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err := w.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	ss := &Server{
 		keymanager:            km,
@@ -386,9 +373,7 @@ func TestServer_ImportKeystores_OK(t *testing.T) {
 		ImportedPublicKeys: pubKeys,
 	}, res)
 
-	km, err = w.InitializeKeymanager(ctx, &iface.InitializeKeymanagerConfig{
-		SkipMnemonicConfirm: true,
-	})
+	km, err = w.InitializeKeymanager(ctx)
 	require.NoError(t, err)
 	keys, err = km.FetchValidatingPublicKeys(ctx)
 	require.NoError(t, err)