From a6bc60e4c6cb07b04fc3935712128bc5794e0f02 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Thu, 29 Nov 2018 20:55:23 +0000 Subject: [PATCH] Merge PR #2923: Prompt user for confirmation when deleting ledger and offline keys * Force password to 'yes' when deleting ledger-offline keys * Improve UX, better docs on removing offline/ledger keys * Ask for confirmation on offline/ledger keys deletion --- PENDING.md | 1 + client/keys/delete.go | 51 ++++++++++++++++++++++++++++++++++--- crypto/keys/keybase.go | 27 ++++++-------------- crypto/keys/keybase_test.go | 4 +-- 4 files changed, 57 insertions(+), 26 deletions(-) diff --git a/PENDING.md b/PENDING.md index 3de7c103dbf3..f65cb638551a 100644 --- a/PENDING.md +++ b/PENDING.md @@ -99,6 +99,7 @@ BUG FIXES * #2907 Refactor and fix the way Gaia Lite is started. * Gaia CLI (`gaiacli`) + * [\#2921](https://github.com/cosmos/cosmos-sdk/issues/2921) Fix `keys delete` inability to delete offline and ledger keys. * Gaia * [\#2723] Use `cosmosvalcons` Bech32 prefix in `tendermint show-address` diff --git a/client/keys/delete.go b/client/keys/delete.go index 406843663d38..c53385f1a47c 100644 --- a/client/keys/delete.go +++ b/client/keys/delete.go @@ -1,9 +1,14 @@ package keys import ( + "bufio" "encoding/json" + "errors" "fmt" "net/http" + "os" + + "github.com/spf13/viper" "github.com/cosmos/cosmos-sdk/client" keys "github.com/cosmos/cosmos-sdk/crypto/keys" @@ -13,13 +18,27 @@ import ( "github.com/spf13/cobra" ) +const ( + flagYes = "yes" +) + func deleteKeyCommand() *cobra.Command { cmd := &cobra.Command{ Use: "delete ", Short: "Delete the given key", - RunE: runDeleteCmd, - Args: cobra.ExactArgs(1), + Long: `Delete a key from the store. + +Note that removing offline or ledger keys will remove +only the public key references stored locally, i.e. +private keys stored in a ledger device cannot be deleted with +gaiacli. +`, + RunE: runDeleteCmd, + Args: cobra.ExactArgs(1), } + + cmd.Flags().BoolP(flagYes, "y", false, + "Skip confirmation prompt when deleting offline or ledger key references") return cmd } @@ -31,12 +50,25 @@ func runDeleteCmd(cmd *cobra.Command, args []string) error { return err } - _, err = kb.Get(name) + info, err := kb.Get(name) if err != nil { return err } buf := client.BufferStdin() + if info.GetType() == keys.TypeLedger || info.GetType() == keys.TypeOffline { + if !viper.GetBool(flagYes) { + if err := confirmDeletion(buf); err != nil { + return err + } + } + if err := kb.Delete(name, ""); err != nil { + return err + } + fmt.Fprintln(os.Stderr, "Public key reference deleted") + return nil + } + oldpass, err := client.GetPassword( "DANGER - enter password to permanently delete key:", buf) if err != nil { @@ -47,7 +79,7 @@ func runDeleteCmd(cmd *cobra.Command, args []string) error { if err != nil { return err } - fmt.Println("Password deleted forever (uh oh!)") + fmt.Fprintln(os.Stderr, "Key deleted forever (uh oh!)") return nil } @@ -98,3 +130,14 @@ func DeleteKeyRequestHandler(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) } + +func confirmDeletion(buf *bufio.Reader) error { + answer, err := client.GetConfirmation("Key reference will be deleted. Continue?", buf) + if err != nil { + return err + } + if !answer { + return errors.New("aborted") + } + return nil +} diff --git a/crypto/keys/keybase.go b/crypto/keys/keybase.go index 0bd500feef5a..a5ec66893d00 100644 --- a/crypto/keys/keybase.go +++ b/crypto/keys/keybase.go @@ -367,34 +367,23 @@ func (kb dbKeybase) ImportPubKey(name string, armor string) (err error) { // Delete removes key forever, but we must present the // proper passphrase before deleting it (for security). -// A passphrase of 'yes' is used to delete stored -// references to offline and Ledger / HW wallet keys +// It returns an error if the key doesn't exist or +// passphrases don't match. +// Passphrase is ignored when deleting references to +// offline and Ledger / HW wallet keys. func (kb dbKeybase) Delete(name, passphrase string) error { // verify we have the proper password before deleting info, err := kb.Get(name) if err != nil { return err } - switch info.(type) { - case localInfo: - linfo := info.(localInfo) - _, err = mintkey.UnarmorDecryptPrivKey(linfo.PrivKeyArmor, passphrase) - if err != nil { + if linfo, ok := info.(localInfo); ok { + if _, err = mintkey.UnarmorDecryptPrivKey(linfo.PrivKeyArmor, passphrase); err != nil { return err } - kb.db.DeleteSync(addrKey(linfo.GetAddress())) - kb.db.DeleteSync(infoKey(name)) - return nil - case ledgerInfo: - case offlineInfo: - if passphrase != "yes" { - return fmt.Errorf("enter 'yes' exactly to delete the key - this cannot be undone") - } - kb.db.DeleteSync(addrKey(info.GetAddress())) - kb.db.DeleteSync(infoKey(name)) - return nil } - + kb.db.DeleteSync(addrKey(info.GetAddress())) + kb.db.DeleteSync(infoKey(name)) return nil } diff --git a/crypto/keys/keybase_test.go b/crypto/keys/keybase_test.go index cafa2382a598..c8c65609ea8f 100644 --- a/crypto/keys/keybase_test.go +++ b/crypto/keys/keybase_test.go @@ -96,9 +96,7 @@ func TestKeyManagement(t *testing.T) { require.Equal(t, 2, len(keyS)) // delete the offline key - err = cstore.Delete(o1, "no") - require.NotNil(t, err) - err = cstore.Delete(o1, "yes") + err = cstore.Delete(o1, "") require.NoError(t, err) keyS, err = cstore.List() require.NoError(t, err)