-
Notifications
You must be signed in to change notification settings - Fork 20.5k
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
Changes from 7 commits
f7088ba
e1a7cc7
3eadab6
c503ac8
8e1a7d9
9bb772b
5a8c4ff
afa2247
bc201f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
) | ||
|
||
|
@@ -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) | ||
|
@@ -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. | ||
|
@@ -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 { | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 | ||
|
@@ -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) | ||
} | ||
|
@@ -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) | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theManager
.There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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