Fix issue with direct wallet's password being overwritten by incorrect password during import. (#7212)

* Maintain working password instead of overwriting it.

* Check for error first

* fix test

* Address linter

* TEMP, INCOMPLETE;  Working on regression test

* WORK IN PROGRESS - changed promptutil to create a mock password reader

* Test working now

* Address testing bugs

* Spacing

* Comments.  Fixed counter bug.

* gazelle fix

Co-authored-by: dv8silencer <15720668+dv8silencer@users.noreply.github.com>

shared/promptutil/prompt.go

@@ -29,6 +29,15 @@ const (
 	ConfirmPass
 )
 
+// PasswordReaderFunc takes in a *file and returns a password using the terminal package
+func passwordReaderFunc(file *os.File) ([]byte, error) {
+	pass, err := terminal.ReadPassword(int(file.Fd()))
+	return pass, err
+}
+
+// PasswordReader has passwordReaderFunc as the default but can be changed for testing purposes.
+var PasswordReader func(file *os.File) ([]byte, error) = passwordReaderFunc
+
 // ValidatePrompt requests the user for text and expects the user to fulfill the provided validation function.
 func ValidatePrompt(r io.Reader, promptText string, validateFunc func(string) error) (string, error) {
 	var responseValid bool
@@ -101,7 +110,7 @@ func PasswordPrompt(promptText string, validateFunc func(string) error) (string,
 	var response string
 	for !responseValid {
 		fmt.Printf("%s: ", au.Bold(promptText))
-		bytePassword, err := terminal.ReadPassword(int(os.Stdin.Fd()))
+		bytePassword, err := PasswordReader(os.Stdin)
 		if err != nil {
 			return "", err
 		}

validator/accounts/v2/BUILD.bazel

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

validator/accounts/v2/accounts_create_test.go

@@ -2,12 +2,16 @@ package v2
 
 import (
 	"context"
+	"io/ioutil"
+	"os"
 	"testing"
 
+	"github.com/prysmaticlabs/prysm/shared/promptutil"
 	"github.com/prysmaticlabs/prysm/shared/testutil/assert"
 	"github.com/prysmaticlabs/prysm/shared/testutil/require"
 	v2keymanager "github.com/prysmaticlabs/prysm/validator/keymanager/v2"
 	"github.com/prysmaticlabs/prysm/validator/keymanager/v2/derived"
+	logTest "github.com/sirupsen/logrus/hooks/test"
 )
 
 func TestCreateAccount_Derived(t *testing.T) {
@@ -54,3 +58,80 @@ func TestCreateAccount_Derived(t *testing.T) {
 	assert.NoError(t, err)
 	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(file *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_Direct checks that the password does not change due to account creation in a Direct wallet
+func Test_KeysConsistency_Direct(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:     v2keymanager.Direct,
+		walletPasswordFile: walletPasswordFile,
+	})
+
+	wallet, err := CreateAndSaveWalletCli(cliCtx)
+	require.NoError(t, err)
+
+	// Create an account using "Pa$sW0rD0__Fo0xPr"
+	err = CreateAccount(cliCtx.Context, &CreateAccountConfig{
+		Wallet:      wallet,
+		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))
+	// OpenWalletOrElseCli() doesn't really 'challenge' the wrong password
+	wallet, err = 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:      wallet,
+		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))
+	wallet, err = OpenWalletOrElseCli(cliCtx, CreateAndSaveWalletCli)
+	require.NoError(t, err)
+	mockPasswordReader.counter = 3
+
+	err = CreateAccount(cliCtx.Context, &CreateAccountConfig{
+		Wallet:      wallet,
+		NumAccounts: 1,
+	})
+	require.NoError(t, err)
+	assert.LogsContain(t, logHook, "Successfully created new validator account")
+}

validator/accounts/v2/iface/wallet.go

@@ -14,6 +14,7 @@ type Wallet interface {
 	// Methods to retrieve wallet and accounts metadata.
 	AccountsDir() string
 	Password() string
+	SetPassword(newPass string)
 	// Read methods for important wallet and accounts-related files.
 	ReadEncryptedSeedFromDisk(ctx context.Context) (io.ReadCloser, error)
 	ReadFileAtPath(ctx context.Context, filePath string, fileName string) ([]byte, error)

validator/accounts/v2/testing/mock.go

@@ -50,6 +50,11 @@ func (m *Wallet) Password() string {
 	return m.WalletPassword
 }
 
+// SetPassword sets a new password for the wallet.
+func (m *Wallet) SetPassword(newPass string) {
+	m.WalletPassword = newPass
+}
+
 // WriteFileAtPath --
 func (m *Wallet) WriteFileAtPath(ctx context.Context, pathName string, fileName string, data []byte) error {
 	m.lock.Lock()

validator/accounts/v2/wallet.go

@@ -169,6 +169,11 @@ func (w *Wallet) Password() string {
 	return w.walletPassword
 }
 
+// SetPassword sets a new password for the wallet.
+func (w *Wallet) SetPassword(newPass string) {
+	w.walletPassword = newPass
+}
+
 // InitializeKeymanager reads a keymanager config from disk at the wallet path,
 // unmarshals it based on the wallet's keymanager kind, and returns its value.
 func (w *Wallet) InitializeKeymanager(

validator/keymanager/v2/direct/direct.go

@@ -373,6 +373,7 @@ func (dr *Keymanager) initializeAccountKeystore(ctx context.Context) error {
 		if err != nil {
 			return errors.Wrap(err, "could not confirm password via prompt")
 		}
+		dr.wallet.SetPassword(password) // Write the correct password to the wallet.
 	} else if err != nil {
 		return errors.Wrap(err, "could not decrypt keystore")
 	}