From 5cb450165a7e25fd50a9e6905ca9a1ae4d43ba57 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Wed, 28 Nov 2018 02:09:37 +0000 Subject: [PATCH 1/7] Force password to 'yes' when deleting ledger-offline keys Closes: #2921 --- client/keys/delete.go | 12 ++++++++++-- crypto/keys/keybase.go | 17 +++++++++++------ 2 files changed, 21 insertions(+), 8 deletions(-) diff --git a/client/keys/delete.go b/client/keys/delete.go index 406843663d38..fcd173ad67e3 100644 --- a/client/keys/delete.go +++ b/client/keys/delete.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http" + "os" "github.com/cosmos/cosmos-sdk/client" keys "github.com/cosmos/cosmos-sdk/crypto/keys" @@ -31,11 +32,18 @@ func runDeleteCmd(cmd *cobra.Command, args []string) error { return err } - _, err = kb.Get(name) + info, err := kb.Get(name) if err != nil { return err } + if info.GetType() == keys.TypeLedger || info.GetType() == keys.TypeOffline { + if err := kb.Delete(name, "yes"); err != nil { + return err + } + fmt.Fprintln(os.Stderr, "Deleted") + } + buf := client.BufferStdin() oldpass, err := client.GetPassword( "DANGER - enter password to permanently delete key:", buf) @@ -47,7 +55,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, "Password deleted forever (uh oh!)") return nil } diff --git a/crypto/keys/keybase.go b/crypto/keys/keybase.go index 0bd500feef5a..d8deba156114 100644 --- a/crypto/keys/keybase.go +++ b/crypto/keys/keybase.go @@ -386,13 +386,9 @@ func (kb dbKeybase) Delete(name, passphrase string) error { kb.db.DeleteSync(infoKey(name)) return nil case ledgerInfo: + return kb.deleteOfflineLedgerKey(info, passphrase) 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 + return kb.deleteOfflineLedgerKey(info, passphrase) } return nil @@ -469,3 +465,12 @@ func addrKey(address types.AccAddress) []byte { func infoKey(name string) []byte { return []byte(fmt.Sprintf("%s.%s", name, infoSuffix)) } + +func (kb dbKeybase) deleteOfflineLedgerKey(info Info, yesPassphrase string) error { + if yesPassphrase != "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(info.GetName())) + return nil +} From d93cf61dd4303f31cdfe1be4016ca1a5245de0b0 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Wed, 28 Nov 2018 13:27:37 +0000 Subject: [PATCH 2/7] Improve UX, better docs on removing offline/ledger keys --- client/keys/delete.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/client/keys/delete.go b/client/keys/delete.go index fcd173ad67e3..8485260b7944 100644 --- a/client/keys/delete.go +++ b/client/keys/delete.go @@ -18,8 +18,15 @@ 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), } return cmd } @@ -41,7 +48,7 @@ func runDeleteCmd(cmd *cobra.Command, args []string) error { if err := kb.Delete(name, "yes"); err != nil { return err } - fmt.Fprintln(os.Stderr, "Deleted") + fmt.Fprintln(os.Stderr, "Public key deleted") } buf := client.BufferStdin() From 8c45193ece5e178ec5defe2773d7deeefb4eea49 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Wed, 28 Nov 2018 13:39:36 +0000 Subject: [PATCH 3/7] Update PENDING.md --- PENDING.md | 1 + 1 file changed, 1 insertion(+) diff --git a/PENDING.md b/PENDING.md index b9b7e28c7499..ac02aa8a7cee 100644 --- a/PENDING.md +++ b/PENDING.md @@ -97,6 +97,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` From 8606d32666c293159074f184ac40b342d58e8ae0 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Wed, 28 Nov 2018 16:24:04 +0000 Subject: [PATCH 4/7] Address @cwgoes comments --- client/keys/delete.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/keys/delete.go b/client/keys/delete.go index 8485260b7944..a8198afc3f91 100644 --- a/client/keys/delete.go +++ b/client/keys/delete.go @@ -48,7 +48,7 @@ func runDeleteCmd(cmd *cobra.Command, args []string) error { if err := kb.Delete(name, "yes"); err != nil { return err } - fmt.Fprintln(os.Stderr, "Public key deleted") + fmt.Fprintln(os.Stderr, "Public key reference deleted") } buf := client.BufferStdin() @@ -62,7 +62,7 @@ func runDeleteCmd(cmd *cobra.Command, args []string) error { if err != nil { return err } - fmt.Fprintln(os.Stderr, "Password deleted forever (uh oh!)") + fmt.Fprintln(os.Stderr, "Key deleted forever (uh oh!)") return nil } From 4a5b52a76550c3a90541745a3430506e0ce0986b Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Wed, 28 Nov 2018 17:47:47 +0000 Subject: [PATCH 5/7] Ask for confirmation on offline/ledger keys deletion Add -y flag to skip confirmation. Also keybase should not handle confirmation input. --- client/keys/delete.go | 32 ++++++++++++++++++++++++++++++-- crypto/keys/keybase.go | 26 ++++---------------------- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/client/keys/delete.go b/client/keys/delete.go index a8198afc3f91..c53385f1a47c 100644 --- a/client/keys/delete.go +++ b/client/keys/delete.go @@ -1,11 +1,15 @@ 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" keyerror "github.com/cosmos/cosmos-sdk/crypto/keys/keyerror" @@ -14,6 +18,10 @@ import ( "github.com/spf13/cobra" ) +const ( + flagYes = "yes" +) + func deleteKeyCommand() *cobra.Command { cmd := &cobra.Command{ Use: "delete ", @@ -28,6 +36,9 @@ 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 } @@ -44,14 +55,20 @@ func runDeleteCmd(cmd *cobra.Command, args []string) error { return err } + buf := client.BufferStdin() if info.GetType() == keys.TypeLedger || info.GetType() == keys.TypeOffline { - if err := kb.Delete(name, "yes"); err != nil { + 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 } - buf := client.BufferStdin() oldpass, err := client.GetPassword( "DANGER - enter password to permanently delete key:", buf) if err != nil { @@ -113,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 d8deba156114..82f68bbf0118 100644 --- a/crypto/keys/keybase.go +++ b/crypto/keys/keybase.go @@ -375,22 +375,13 @@ func (kb dbKeybase) Delete(name, passphrase string) error { 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: - return kb.deleteOfflineLedgerKey(info, passphrase) - case offlineInfo: - return kb.deleteOfflineLedgerKey(info, passphrase) } - + kb.db.DeleteSync(addrKey(info.GetAddress())) + kb.db.DeleteSync(infoKey(name)) return nil } @@ -465,12 +456,3 @@ func addrKey(address types.AccAddress) []byte { func infoKey(name string) []byte { return []byte(fmt.Sprintf("%s.%s", name, infoSuffix)) } - -func (kb dbKeybase) deleteOfflineLedgerKey(info Info, yesPassphrase string) error { - if yesPassphrase != "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(info.GetName())) - return nil -} From c2f052803f36e0e74ca10013fe0d2d4ca03e2e72 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Thu, 29 Nov 2018 16:03:52 +0000 Subject: [PATCH 6/7] Fix keybase test --- crypto/keys/keybase_test.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) 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) From e3b2c0c9cfd2b67066b3212823d3d6e6ec07e951 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Thu, 29 Nov 2018 16:12:50 +0000 Subject: [PATCH 7/7] Update godoc comment --- crypto/keys/keybase.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crypto/keys/keybase.go b/crypto/keys/keybase.go index 82f68bbf0118..a5ec66893d00 100644 --- a/crypto/keys/keybase.go +++ b/crypto/keys/keybase.go @@ -367,8 +367,10 @@ 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)