Remove Account Creation Privilege For Imported Keymanager (#7555)

* rem create

* remove create account privilege for nonhd wallets

* fix bazel

* radek feedback

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

validator/accounts/BUILD.bazel

@@ -78,7 +78,6 @@ go_test(
         "//shared/mock:go_default_library",
         "//shared/params:go_default_library",
         "//shared/petnames:go_default_library",
-        "//shared/promptutil:go_default_library",
         "//shared/testutil:go_default_library",
         "//shared/testutil/assert:go_default_library",
         "//shared/testutil/require:go_default_library",

validator/accounts/accounts_create.go

@@ -13,7 +13,6 @@ import (
 	"github.com/prysmaticlabs/prysm/validator/flags"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/derived"
-	"github.com/prysmaticlabs/prysm/validator/keymanager/imported"
 	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli/v2"
 )
@@ -56,14 +55,7 @@ func CreateAccount(ctx context.Context, cfg *CreateAccountConfig) error {
 	case keymanager.Remote:
 		return errors.New("cannot create a new account for a remote keymanager")
 	case keymanager.Imported:
-		km, ok := km.(*imported.Keymanager)
-		if !ok {
-			return errors.New("not a imported keymanager")
-		}
-		// Create a new validator account using the specified keymanager.
-		if _, _, err := km.CreateAccount(ctx); err != nil {
-			return errors.Wrap(err, "could not create account in wallet")
-		}
+		return errors.New("cannot create a new account for an imported wallet")
 	case keymanager.Derived:
 		km, ok := km.(*derived.Keymanager)
 		if !ok {

validator/accounts/accounts_create_test.go

@@ -4,19 +4,15 @@ import (
 	"context"
 	"encoding/hex"
 	"fmt"
-	"io/ioutil"
-	"os"
 	"testing"
 
 	ethpb "github.com/prysmaticlabs/ethereumapis/eth/v1alpha1"
 	"github.com/prysmaticlabs/prysm/shared/params"
-	"github.com/prysmaticlabs/prysm/shared/promptutil"
 	"github.com/prysmaticlabs/prysm/shared/testutil/assert"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
 	"github.com/prysmaticlabs/prysm/validator/accounts/wallet"
 	"github.com/prysmaticlabs/prysm/validator/keymanager"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/derived"
-	logTest "github.com/sirupsen/logrus/hooks/test"
 )
 
 func TestCreateAccount_Derived(t *testing.T) {
@@ -65,86 +61,6 @@ func TestCreateAccount_Derived(t *testing.T) {
 	require.Equal(t, len(names), int(numAccounts))
 }
 
-// passwordReader will store data that will be later used to mock Stdin by Test_KeysConsistency_Direct
-type passwordReader struct {
-	password string
-	counter  int // counter equals the maximum number of times method passwordReaderFunc can be called
-}
-
-// Instead of forwarding the read request to terminal.ReadPassword(), we simply provide a canned response.
-func (p *passwordReader) passwordReaderFunc(_ *os.File) ([]byte, error) {
-	p.counter--
-	if p.counter <= 0 {
-		log.Fatalln("Too many password attempts using passwordReaderFunc()")
-	}
-	return []byte(p.password), nil
-}
-
-// Test_KeysConsistency_Imported checks that the password does not change due to account creation in a Imported wallet
-func Test_KeysConsistency_Imported(t *testing.T) {
-	walletDir, passwordsDir, walletPasswordFile := setupWalletAndPasswordsDir(t)
-
-	// Specify the 'initial'/correct password locally to this file for convenience.
-	require.NoError(t, ioutil.WriteFile(walletPasswordFile, []byte("Pa$sW0rD0__Fo0xPr"), os.ModePerm))
-
-	cliCtx := setupWalletCtx(t, &testWalletConfig{
-		walletDir:          walletDir,
-		passwordsDir:       passwordsDir,
-		keymanagerKind:     keymanager.Imported,
-		walletPasswordFile: walletPasswordFile,
-	})
-
-	w, err := CreateAndSaveWalletCli(cliCtx)
-	require.NoError(t, err)
-
-	// Create an account using "Pa$sW0rD0__Fo0xPr"
-	err = CreateAccount(cliCtx.Context, &CreateAccountConfig{
-		Wallet:      w,
-		NumAccounts: 1,
-	})
-	require.NoError(t, err)
-
-	/* The bug this test checks for works like this:  Input wrong password followed by the correct password.
-	This causes the wallet's password to change to the (initially) wrong provided password.
-	*/
-
-	// Now we change the password to "SecoNDxyzPass__9!@#"
-	require.NoError(t, ioutil.WriteFile(walletPasswordFile, []byte("SecoNDxyzPass__9!@#"), os.ModePerm))
-	_, err = wallet.OpenWalletOrElseCli(cliCtx, CreateAndSaveWalletCli)
-	require.ErrorContains(t, "wrong password for wallet", err)
-
-	require.NoError(t, ioutil.WriteFile(walletPasswordFile, []byte("Pa$sW0rD0__Fo0xPr"), os.ModePerm))
-	w, err = wallet.OpenWalletOrElseCli(cliCtx, CreateAndSaveWalletCli)
-	require.NoError(t, err)
-
-	/*  The purpose of using a passwordReader object is to store a 'canned' response for when the program
-	asks for more passwords.  As we are about to call CreateAccount() with an incorrect password, we expect the
-	program to ask for more attempts via Stdin.	 This will provide the correct password.*/
-	mockPasswordReader := passwordReader{password: "Pa$sW0rD0__Fo0xPr", counter: 3}
-	// Redirect promptutil's PasswordReader to our function which bypasses/mocks Stdin
-	promptutil.PasswordReader = mockPasswordReader.passwordReaderFunc
-
-	err = CreateAccount(cliCtx.Context, &CreateAccountConfig{
-		Wallet:      w,
-		NumAccounts: 1,
-	})
-	require.NoError(t, err)
-
-	// Now we make sure a bug did not change the password to "SecoNDxyzPass__9!@#"
-	logHook := logTest.NewGlobal()
-	require.NoError(t, ioutil.WriteFile(walletPasswordFile, []byte("Pa$sW0rD0__Fo0xPr"), os.ModePerm))
-	w, err = wallet.OpenWalletOrElseCli(cliCtx, CreateAndSaveWalletCli)
-	require.NoError(t, err)
-	mockPasswordReader.counter = 3
-
-	err = CreateAccount(cliCtx.Context, &CreateAccountConfig{
-		Wallet:      w,
-		NumAccounts: 1,
-	})
-	require.NoError(t, err)
-	assert.LogsContain(t, logHook, "Successfully created new validator account")
-}
-
 func TestDepositDataJSON(t *testing.T) {
 	// Use a real deposit data JSON fixture generated by the eth2 deposit cli
 	fixture := make(map[string]string)

validator/accounts/accounts_list_test.go

@@ -9,6 +9,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/uuid"
 	validatorpb "github.com/prysmaticlabs/prysm/proto/validator/accounts/v2"
 	"github.com/prysmaticlabs/prysm/shared/bls"
 	"github.com/prysmaticlabs/prysm/shared/bytesutil"
@@ -20,6 +21,7 @@ import (
 	"github.com/prysmaticlabs/prysm/validator/keymanager/derived"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/imported"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/remote"
+	keystorev4 "github.com/wealdtech/go-eth2-wallet-encryptor-keystorev4"
 )
 
 type mockRemoteKeymanager struct {
@@ -35,6 +37,23 @@ func (m *mockRemoteKeymanager) Sign(context.Context, *validatorpb.SignRequest) (
 	return nil, nil
 }
 
+func createRandomKeystore(t testing.TB, password string) *keymanager.Keystore {
+	encryptor := keystorev4.New()
+	id, err := uuid.NewRandom()
+	require.NoError(t, err)
+	validatingKey := bls.RandKey()
+	pubKey := validatingKey.PublicKey().Marshal()
+	cryptoFields, err := encryptor.Encrypt(validatingKey.Marshal(), password)
+	require.NoError(t, err)
+	return &keymanager.Keystore{
+		Crypto:  cryptoFields,
+		Pubkey:  fmt.Sprintf("%x", pubKey),
+		ID:      id.String(),
+		Version: encryptor.Version(),
+		Name:    encryptor.Name(),
+	}
+}
+
 func TestListAccounts_ImportedKeymanager(t *testing.T) {
 	walletDir, passwordsDir, walletPasswordFile := setupWalletAndPasswordsDir(t)
 	cliCtx := setupWalletCtx(t, &testWalletConfig{
@@ -51,7 +70,7 @@ func TestListAccounts_ImportedKeymanager(t *testing.T) {
 		},
 	})
 	require.NoError(t, err)
-	keymanager, err := imported.NewKeymanager(
+	km, err := imported.NewKeymanager(
 		cliCtx.Context,
 		&imported.SetupConfig{
 			Wallet: w,
@@ -61,17 +80,27 @@ func TestListAccounts_ImportedKeymanager(t *testing.T) {
 	require.NoError(t, err)
 
 	numAccounts := 5
+	keystores := make([]*keymanager.Keystore, numAccounts)
 	for i := 0; i < numAccounts; i++ {
-		_, _, err := keymanager.CreateAccount(cliCtx.Context)
-		require.NoError(t, err)
+		keystores[i] = createRandomKeystore(t, password)
 	}
+	require.NoError(t, km.ImportKeystores(cliCtx.Context, keystores, password))
+
 	rescueStdout := os.Stdout
 	r, writer, err := os.Pipe()
 	require.NoError(t, err)
 	os.Stdout = writer
 
 	// We call the list imported keymanager accounts function.
-	require.NoError(t, listImportedKeymanagerAccounts(context.Background(), true /* show deposit data */, true /*show private keys */, keymanager))
+	require.NoError(
+		t,
+		listImportedKeymanagerAccounts(
+			context.Background(),
+			true, /* show deposit data */
+			true, /*show private keys */
+			km,
+		),
+	)
 
 	require.NoError(t, writer.Close())
 	out, err := ioutil.ReadAll(r)
@@ -140,7 +169,7 @@ func TestListAccounts_ImportedKeymanager(t *testing.T) {
 	assert.Equal(t, true, kindFound, "Keymanager Kind %s not found on the first line", kindString)
 
 	// Get account names and require the correct count
-	accountNames, err := keymanager.ValidatingAccountNames()
+	accountNames, err := km.ValidatingAccountNames()
 	require.NoError(t, err)
 	require.Equal(t, numAccounts, len(accountNames))
 
@@ -152,7 +181,7 @@ func TestListAccounts_ImportedKeymanager(t *testing.T) {
 	}
 
 	// Get public keys and require the correct count
-	pubKeys, err := keymanager.FetchValidatingPublicKeys(cliCtx.Context)
+	pubKeys, err := km.FetchValidatingPublicKeys(cliCtx.Context)
 	require.NoError(t, err)
 	require.Equal(t, numAccounts, len(pubKeys))
 
@@ -165,7 +194,7 @@ func TestListAccounts_ImportedKeymanager(t *testing.T) {
 	}
 
 	// Get private keys and require the correct count
-	privKeys, err := keymanager.FetchValidatingPrivateKeys(cliCtx.Context)
+	privKeys, err := km.FetchValidatingPrivateKeys(cliCtx.Context)
 	require.NoError(t, err)
 	require.Equal(t, numAccounts, len(pubKeys))
 

validator/accounts/wallet_edit_test.go

@@ -14,7 +14,7 @@ import (
 )
 
 func TestEditWalletConfiguration(t *testing.T) {
-	walletDir, _, _ := setupWalletAndPasswordsDir(t)
+	walletDir, _, passwordFile := setupWalletAndPasswordsDir(t)
 	cliCtx := setupWalletCtx(t, &testWalletConfig{
 		walletDir:      walletDir,
 		keymanagerKind: keymanager.Remote,
@@ -51,11 +51,13 @@ func TestEditWalletConfiguration(t *testing.T) {
 	app := cli.App{}
 	set := flag.NewFlagSet("test", 0)
 	set.String(flags.WalletDirFlag.Name, walletDir, "")
+	set.String(flags.WalletPasswordFileFlag.Name, passwordFile, "")
 	set.String(flags.GrpcRemoteAddressFlag.Name, wantCfg.RemoteAddr, "")
 	set.String(flags.RemoteSignerCertPathFlag.Name, wantCfg.RemoteCertificate.ClientCertPath, "")
 	set.String(flags.RemoteSignerKeyPathFlag.Name, wantCfg.RemoteCertificate.ClientKeyPath, "")
 	set.String(flags.RemoteSignerCACertPathFlag.Name, wantCfg.RemoteCertificate.CACertPath, "")
 	assert.NoError(t, set.Set(flags.WalletDirFlag.Name, walletDir))
+	assert.NoError(t, set.Set(flags.WalletPasswordFileFlag.Name, passwordFile))
 	assert.NoError(t, set.Set(flags.GrpcRemoteAddressFlag.Name, wantCfg.RemoteAddr))
 	assert.NoError(t, set.Set(flags.RemoteSignerCertPathFlag.Name, wantCfg.RemoteCertificate.ClientCertPath))
 	assert.NoError(t, set.Set(flags.RemoteSignerKeyPathFlag.Name, wantCfg.RemoteCertificate.ClientKeyPath))

validator/keymanager/imported/BUILD.bazel

@@ -16,16 +16,13 @@ go_library(
         "//validator:__subpackages__",
     ],
     deps = [
-        "//beacon-chain/core/helpers:go_default_library",
         "//proto/validator/accounts/v2:go_default_library",
         "//shared/asyncutil:go_default_library",
         "//shared/bls:go_default_library",
         "//shared/bytesutil:go_default_library",
-        "//shared/depositutil:go_default_library",
         "//shared/event:go_default_library",
         "//shared/fileutil:go_default_library",
         "//shared/interop:go_default_library",
-        "//shared/params:go_default_library",
         "//shared/petnames:go_default_library",
         "//validator/accounts/iface:go_default_library",
         "//validator/keymanager:go_default_library",
@@ -34,7 +31,6 @@ go_library(
         "@com_github_k0kubun_go_ansi//:go_default_library",
         "@com_github_logrusorgru_aurora//:go_default_library",
         "@com_github_pkg_errors//:go_default_library",
-        "@com_github_prysmaticlabs_ethereumapis//eth/v1alpha1:go_default_library",
         "@com_github_schollz_progressbar_v3//:go_default_library",
         "@com_github_sirupsen_logrus//:go_default_library",
         "@com_github_wealdtech_go_eth2_wallet_encryptor_keystorev4//:go_default_library",
@@ -56,7 +52,6 @@ go_test(
         "//shared/bls:go_default_library",
         "//shared/bytesutil:go_default_library",
         "//shared/event:go_default_library",
-        "//shared/petnames:go_default_library",
         "//shared/testutil/assert:go_default_library",
         "//shared/testutil/require:go_default_library",
         "//validator/accounts/testing:go_default_library",