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

@@ -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`

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 <name>",
 		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
+}

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
 }
 

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)