Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/geth: remove unlock commandline flag #30737

Merged
merged 9 commits into from
Nov 15, 2024
3 changes: 1 addition & 2 deletions accounts/keystore/account_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ func byURL(a, b accounts.Account) int {
return a.URL.Cmp(b.URL)
}

// AmbiguousAddrError is returned when attempting to unlock
// an address for which more than one file exists.
// AmbiguousAddrError is returned when an address matches multiple files.
type AmbiguousAddrError struct {
Addr common.Address
Matches []accounts.Account
Expand Down
1 change: 0 additions & 1 deletion accounts/keystore/testdata/dupes/1

This file was deleted.

1 change: 0 additions & 1 deletion accounts/keystore/testdata/dupes/2

This file was deleted.

1 change: 0 additions & 1 deletion accounts/keystore/testdata/dupes/foo

This file was deleted.

17 changes: 1 addition & 16 deletions accounts/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,14 +29,6 @@ import (
// the manager will buffer in its channel.
const managerSubBufferSize = 50

// Config contains the settings of the global account manager.
//
// TODO(rjl493456442, karalabe, holiman): Get rid of this when account management
// is removed in favor of Clef.
type Config struct {
InsecureUnlockAllowed bool // Whether account unlocking in insecure environment is allowed
}

// newBackendEvent lets the manager know it should
// track the given backend for wallet updates.
type newBackendEvent struct {
Expand All @@ -47,7 +39,6 @@ type newBackendEvent struct {
// Manager is an overarching account manager that can communicate with various
// backends for signing transactions.
type Manager struct {
config *Config // Global account manager configurations
backends map[reflect.Type][]Backend // Index of backends currently registered
updaters []event.Subscription // Wallet update subscriptions for all backends
updates chan WalletEvent // Subscription sink for backend wallet changes
Expand All @@ -63,7 +54,7 @@ type Manager struct {

// NewManager creates a generic account manager to sign transaction via various
// supported backends.
func NewManager(config *Config, backends ...Backend) *Manager {
func NewManager(backends ...Backend) *Manager {
Copy link
Contributor

@fjl fjl Nov 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate to be that guy again, but this function has had a stable signature for a long time. So I would prefer not to change it. We can simply ignore the setting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit weird that we even added it. In hindsight it would've been better to add a method like SetInsecureUnlockAllowed on the Manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just make it an any ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not, for the same reason. Ugh

// Retrieve the initial list of wallets from the backends and sort by URL
var wallets []Wallet
for _, backend := range backends {
Expand All @@ -78,7 +69,6 @@ func NewManager(config *Config, backends ...Backend) *Manager {
}
// Assemble the account manager and return
am := &Manager{
config: config,
backends: make(map[reflect.Type][]Backend),
updaters: subs,
updates: updates,
Expand Down Expand Up @@ -106,11 +96,6 @@ func (am *Manager) Close() error {
return <-errc
}

// Config returns the configuration of account manager.
func (am *Manager) Config() *Config {
return am.config
}

// AddBackend starts the tracking of an additional backend for wallet updates.
// cmd/geth assumes once this func returns the backends have been already integrated.
func (am *Manager) AddBackend(backend Backend) {
Expand Down
113 changes: 48 additions & 65 deletions cmd/geth/accountcmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,16 @@
package main

import (
"errors"
"fmt"
"os"
"strings"

"github.com/ethereum/go-ethereum/accounts"
"github.com/ethereum/go-ethereum/accounts/keystore"
"github.com/ethereum/go-ethereum/cmd/utils"
"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/crypto"
"github.com/ethereum/go-ethereum/log"
"github.com/urfave/cli/v2"
)

Expand Down Expand Up @@ -191,7 +193,7 @@ nodes.
// makeAccountManager creates an account manager with backends
func makeAccountManager(ctx *cli.Context) *accounts.Manager {
cfg := loadBaseConfig(ctx)
am := accounts.NewManager(&accounts.Config{InsecureUnlockAllowed: cfg.Node.InsecureUnlockAllowed})
am := accounts.NewManager()
keydir, isEphemeral, err := cfg.Node.GetKeyStoreDir()
if err != nil {
utils.Fatalf("Failed to get the keystore directory: %v", err)
Expand Down Expand Up @@ -219,60 +221,22 @@ func accountList(ctx *cli.Context) error {
return nil
}

// tries unlocking the specified account a few times.
func unlockAccount(ks *keystore.KeyStore, address string, i int, passwords []string) (accounts.Account, string) {
account, err := utils.MakeAddress(ks, address)
if err != nil {
utils.Fatalf("Could not list accounts: %v", err)
}
for trials := 0; trials < 3; trials++ {
prompt := fmt.Sprintf("Unlocking account %s | Attempt %d/%d", address, trials+1, 3)
password := utils.GetPassPhraseWithList(prompt, false, i, passwords)
err = ks.Unlock(account, password)
if err == nil {
log.Info("Unlocked account", "address", account.Address.Hex())
return account, password
}
if err, ok := err.(*keystore.AmbiguousAddrError); ok {
log.Info("Unlocked account", "address", account.Address.Hex())
return ambiguousAddrRecovery(ks, err, password), password
}
if err != keystore.ErrDecrypt {
// No need to prompt again if the error is not decryption-related.
break
}
}
// All trials expended to unlock account, bail out
utils.Fatalf("Failed to unlock account %s (%v)", address, err)

return accounts.Account{}, ""
}

func ambiguousAddrRecovery(ks *keystore.KeyStore, err *keystore.AmbiguousAddrError, auth string) accounts.Account {
fmt.Printf("Multiple key files exist for address %x:\n", err.Addr)
for _, a := range err.Matches {
fmt.Println(" ", a.URL)
}
fmt.Println("Testing your password against all of them...")
var match *accounts.Account
for i, a := range err.Matches {
if e := ks.Unlock(a, auth); e == nil {
match = &err.Matches[i]
break
}
// readPasswordFromFile reads the first line of the given file, trims line endings,
// and returns the password and whether the reading was successful.
func readPasswordFromFile(path string) (string, bool) {
if path == "" {
return "", false
}
if match == nil {
utils.Fatalf("None of the listed files could be unlocked.")
return accounts.Account{}
text, err := os.ReadFile(path)
if err != nil {
utils.Fatalf("Failed to read password file: %v", err)
}
fmt.Printf("Your password unlocked %s\n", match.URL)
fmt.Println("In order to avoid this warning, you need to remove the following duplicate key files:")
for _, a := range err.Matches {
if a != *match {
fmt.Println(" ", a.URL)
}
lines := strings.Split(string(text), "\n")
if len(lines) == 0 {
return "", false
}
return *match
// Sanitise DOS line endings.
return strings.TrimRight(lines[0], "\r"), true
}

// accountCreate creates a new account into the keystore defined by the CLI flags.
Expand All @@ -292,8 +256,10 @@ func accountCreate(ctx *cli.Context) error {
scryptP = keystore.LightScryptP
}

password := utils.GetPassPhraseWithList("Your new account is locked with a password. Please give a password. Do not forget this password.", true, 0, utils.MakePasswordList(ctx))

password, ok := readPasswordFromFile(ctx.Path(utils.PasswordFileFlag.Name))
if !ok {
password = utils.GetPassPhrase("Your new account is locked with a password. Please give a password. Do not forget this password.", true)
}
account, err := keystore.StoreKey(keydir, password, scryptN, scryptP)

if err != nil {
Expand Down Expand Up @@ -323,10 +289,23 @@ func accountUpdate(ctx *cli.Context) error {
ks := backends[0].(*keystore.KeyStore)

for _, addr := range ctx.Args().Slice() {
account, oldPassword := unlockAccount(ks, addr, 0, nil)
newPassword := utils.GetPassPhraseWithList("Please give a new password. Do not forget this password.", true, 0, nil)
if err := ks.Update(account, oldPassword, newPassword); err != nil {
utils.Fatalf("Could not update the account: %v", err)
if !common.IsHexAddress(addr) {
return errors.New("address must be specified in hexadecimal form")
}
account := accounts.Account{Address: common.HexToAddress(addr)}
newPassword := utils.GetPassPhrase("Please give a NEW password. Do not forget this password.", true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the only major change here, you turned the prompts around, so it asks for the NEW password before it asks for the OLD one to update. This way it only asks you once for the new one and not multiple times if the old fails.
Seemed a bit counter-intuitive to me at first, but I think I agree that this makes most sense

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. Change or order, yes, but what do you mean I changed about the confirmations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way it only asks you once for the new one and not multiple times if the old fails.

So, with the old pw, you still have three retries. And there's a confirmation on the new password, so you do have to enter that twice. So afaict the only change really is that it now asks for the new password before it asks for the old one.

?

updateFn := func(attempt int) error {
prompt := fmt.Sprintf("Please provide the OLD password for account %s | Attempt %d/%d", addr, attempt+1, 3)
password := utils.GetPassPhrase(prompt, false)
return ks.Update(account, password, newPassword)
}
// let user attempt unlock thrice.
err := updateFn(0)
for attempts := 1; attempts < 3 && errors.Is(err, keystore.ErrDecrypt); attempts++ {
err = updateFn(attempts)
}
if err != nil {
return fmt.Errorf("could not update account: %w", err)
}
}
return nil
Expand All @@ -347,10 +326,12 @@ func importWallet(ctx *cli.Context) error {
if len(backends) == 0 {
utils.Fatalf("Keystore is not available")
}
password, ok := readPasswordFromFile(ctx.Path(utils.PasswordFileFlag.Name))
if !ok {
password = utils.GetPassPhrase("", false)
}
ks := backends[0].(*keystore.KeyStore)
passphrase := utils.GetPassPhraseWithList("", false, 0, utils.MakePasswordList(ctx))

acct, err := ks.ImportPreSaleKey(keyJSON, passphrase)
acct, err := ks.ImportPreSaleKey(keyJSON, password)
if err != nil {
utils.Fatalf("%v", err)
}
Expand All @@ -373,9 +354,11 @@ func accountImport(ctx *cli.Context) error {
utils.Fatalf("Keystore is not available")
}
ks := backends[0].(*keystore.KeyStore)
passphrase := utils.GetPassPhraseWithList("Your new account is locked with a password. Please give a password. Do not forget this password.", true, 0, utils.MakePasswordList(ctx))

acct, err := ks.ImportECDSA(key, passphrase)
password, ok := readPasswordFromFile(ctx.Path(utils.PasswordFileFlag.Name))
if !ok {
password = utils.GetPassPhrase("Your new account is locked with a password. Please give a password. Do not forget this password.", true)
}
acct, err := ks.ImportECDSA(key, password)
if err != nil {
utils.Fatalf("Could not create the account: %v", err)
}
Expand Down
Loading