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

Interchangable PrivKey implementations in keybase #5278

Merged
merged 27 commits into from
Dec 12, 2019
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
9d3227a
Added keybase options to be able to override key algo
austinabell Nov 5, 2019
71b0c89
merge
austinabell Nov 5, 2019
5bff74e
Refactor
austinabell Nov 5, 2019
91f072d
Fix typo
austinabell Nov 5, 2019
80ed922
merge
austinabell Nov 5, 2019
3b5f8c0
Updates keybase options function and added function documentation
austinabell Nov 6, 2019
d378296
Changes necessary to apply options to keyring keybase
austinabell Nov 21, 2019
de4a612
Merge branch 'master' into austin/kbsigning
fedekunze Nov 21, 2019
d0c2800
address PR comments
austinabell Nov 22, 2019
566ebe1
Address documentation changes and reused buffer
austinabell Nov 25, 2019
e02ab8e
update CHANGELOG entry
austinabell Nov 26, 2019
350ad67
Merge branch 'master' into austin/kbsigning
austinabell Dec 3, 2019
bf1699d
Merge branch 'master' into austin/kbsigning
austinabell Dec 5, 2019
6f4482d
Merge branch 'master' into austin/kbsigning
austinabell Dec 10, 2019
6b24c38
Merge branch 'master' into austin/kbsigning
austinabell Dec 11, 2019
372411c
fix merge conflicts
austinabell Dec 11, 2019
dd8068b
Merge branch 'master' into austin/kbsigning
austinabell Dec 11, 2019
80fc9b9
Merge branch 'master' into austin/kbsigning
austinabell Dec 12, 2019
b13d540
address comments other than test
austinabell Dec 12, 2019
f0c82cf
Add keygen override test
austinabell Dec 12, 2019
d54277e
misspell lol
austinabell Dec 12, 2019
df1d4f4
Merge branch 'master' into austin/kbsigning
Dec 12, 2019
9ef070b
remove duplicate dep ref
austinabell Dec 12, 2019
bae2c1b
Merge branch 'austin/kbsigning' of github.com:ChainSafe/cosmos-sdk in…
austinabell Dec 12, 2019
e211980
Merge branch 'master' into austin/kbsigning
Dec 12, 2019
e87e815
change example test
austinabell Dec 12, 2019
4d68fb2
remove duplicate dep ref
austinabell Dec 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ that allows for arbitrary vesting periods.
* `ValidateSigCountDecorator`: Validate the number of signatures in tx based on app-parameters.
* `IncrementSequenceDecorator`: Increments the account sequence for each signer to prevent replay attacks.
* (cli) [\#5223](https://github.com/cosmos/cosmos-sdk/issues/5223) Cosmos Ledger App v2.0.0 is now supported. The changes are backwards compatible and App v1.5.x is still supported.
* (keys) [\#4941](https://github.com/cosmos/cosmos-sdk/issues/4941) Introduce keybase option to allow overriding the default private key implementation of a key generated through the `keys add` cli command
alessio marked this conversation as resolved.
Show resolved Hide resolved

### Improvements

Expand Down
38 changes: 23 additions & 15 deletions client/keys/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ const (
DefaultKeyPass = "12345678"
)

func addKeyCommand() *cobra.Command {
// AddKeyCommand defines a keybase command to add a generated or recovered private key to keybase
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func AddKeyCommand() *cobra.Command {
austinabell marked this conversation as resolved.
Show resolved Hide resolved
cmd := &cobra.Command{
Use: "add <name>",
Short: "Add an encrypted private key (either newly generated or recovered), encrypt it, and save to disk",
Expand Down Expand Up @@ -75,6 +76,23 @@ the flag --nosort is set.
return cmd
}

func getKeybase(cmd *cobra.Command, dryrun bool) (keys.Keybase, error) {
if dryrun {
austinabell marked this conversation as resolved.
Show resolved Hide resolved
return keys.NewInMemory(), nil
}

return NewKeyringFromHomeFlag(cmd.InOrStdin())
austinabell marked this conversation as resolved.
Show resolved Hide resolved
}

func runAddCmd(cmd *cobra.Command, args []string) error {
kb, err := getKeybase(cmd, viper.GetBool(flagDryRun))
if err != nil {
return err
}

return RunAddCmd(cmd, args, kb)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks odd. Shouldn't a public function call a private one instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree, the rationale was that I did not want to require an application to duplicate all of the command logic just to add options to the keybase. Ideally I would have liked to restructure the flow of this command, to split the logic into generating the key bytes from the command parameters and saving the key, so that instead of options passed to the keybase, the key type that is saved could just be defined in the command. I didn't want to make breaking changes though, so I will try to think of a better way of doing this without making breaking changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I see and appreciate the rationale, though I'm not convinced about the implementation. I like see a suboptimal design flow better than a potentially broken one.

Copy link
Contributor Author

@austinabell austinabell Dec 12, 2019

Choose a reason for hiding this comment

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

Oh I'm sorry, I missed this comment. I'll explain what I meant by a different flow by some vague pseudocode and hopefully this will be sufficient to get the point across:

currently the code vaguely does:

addcommand() {
  info ... = kb.CreateAccount(...)
}
...) CreateAccount(..) {
 ...
  derivedBz = ComputeDerivedKey(seed,...)
  writeKey(...derivedBz)
}

Where it would be more ideal to pass the bytes back to the addcommand to be able to derive whatever implementation there instead of having to add keybase options to do this deep in the function calls. It doesn't make much sense to me why the key is derived and written all in one flow anyway. This would have removed the need for this PR altogether because I could have just overriden the add command and been done with it. Example in pseudo:

addcommand() {
  derivedBz = ComputeDerivedKey(...)
  writeKey(...derivedBz)
}

}

/*
input
- bip39 mnemonic
Expand All @@ -84,8 +102,7 @@ input
output
- armor encrypted private key (saved to file)
*/
func runAddCmd(cmd *cobra.Command, args []string) error {
var kb keys.Keybase
func RunAddCmd(cmd *cobra.Command, args []string, kb keys.Keybase) error {
var err error

inBuf := bufio.NewReader(cmd.InOrStdin())
Expand All @@ -94,16 +111,7 @@ func runAddCmd(cmd *cobra.Command, args []string) error {
interactive := viper.GetBool(flagInteractive)
showMnemonic := !viper.GetBool(flagNoBackup)

if viper.GetBool(flagDryRun) {
// we throw this away, so don't enforce args,
// we want to get a new random seed phrase quickly
kb = keys.NewInMemory()
} else {
kb, err = NewKeyringFromHomeFlag(cmd.InOrStdin())
if err != nil {
return err
}

if !viper.GetBool(flagDryRun) {
_, err = kb.Get(name)
if err == nil {
// account exists, ask for user confirmation
Expand Down Expand Up @@ -273,9 +281,9 @@ func printCreate(cmd *cobra.Command, info keys.Info, showMnemonic bool, mnemonic

var jsonString []byte
if viper.GetBool(flags.FlagIndentResponse) {
jsonString, err = cdc.MarshalJSONIndent(out, "", " ")
jsonString, err = KeysCdc.MarshalJSONIndent(out, "", " ")
} else {
jsonString, err = cdc.MarshalJSON(out)
jsonString, err = KeysCdc.MarshalJSON(out)
}

if err != nil {
Expand Down
4 changes: 2 additions & 2 deletions client/keys/add_ledger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func Test_runAddCmdLedgerWithCustomCoinType(t *testing.T) {
config.SetBech32PrefixForValidator(bech32PrefixValAddr, bech32PrefixValPub)
config.SetBech32PrefixForConsensusNode(bech32PrefixConsAddr, bech32PrefixConsPub)

cmd := addKeyCommand()
cmd := AddKeyCommand()
require.NotNil(t, cmd)

// Prepare a keybase
Expand Down Expand Up @@ -80,7 +80,7 @@ func Test_runAddCmdLedgerWithCustomCoinType(t *testing.T) {

func Test_runAddCmdLedger(t *testing.T) {
runningUnattended := isRunningUnattended()
cmd := addKeyCommand()
cmd := AddKeyCommand()
require.NotNil(t, cmd)
mockIn, _, _ := tests.ApplyMockIO(cmd)

Expand Down
2 changes: 1 addition & 1 deletion client/keys/add_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (

func Test_runAddCmdBasic(t *testing.T) {
runningUnattended := isRunningUnattended()
cmd := addKeyCommand()
cmd := AddKeyCommand()
assert.NotNil(t, cmd)
mockIn, _, _ := tests.ApplyMockIO(cmd)

Expand Down
13 changes: 7 additions & 6 deletions client/keys/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,21 @@ import (
"github.com/cosmos/cosmos-sdk/codec"
)

var cdc *codec.Codec
// KeysCdc defines codec to be used with key operations
var KeysCdc *codec.Codec

func init() {
cdc = codec.New()
codec.RegisterCrypto(cdc)
cdc.Seal()
KeysCdc = codec.New()
alessio marked this conversation as resolved.
Show resolved Hide resolved
codec.RegisterCrypto(KeysCdc)
KeysCdc.Seal()
}

// marshal keys
func MarshalJSON(o interface{}) ([]byte, error) {
return cdc.MarshalJSON(o)
return KeysCdc.MarshalJSON(o)
}

// unmarshal json
func UnmarshalJSON(bz []byte, ptr interface{}) error {
return cdc.UnmarshalJSON(bz, ptr)
return KeysCdc.UnmarshalJSON(bz, ptr)
}
3 changes: 2 additions & 1 deletion client/keys/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ const (
flagForce = "force"
)

func deleteKeyCommand() *cobra.Command {
// DeleteKeyCommand deletes a key from the keybase
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func DeleteKeyCommand() *cobra.Command {
austinabell marked this conversation as resolved.
Show resolved Hide resolved
cmd := &cobra.Command{
Use: "delete <name>",
Short: "Delete the given key",
Expand Down
2 changes: 1 addition & 1 deletion client/keys/delete_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

func Test_runDeleteCmd(t *testing.T) {
runningUnattended := isRunningUnattended()
deleteKeyCommand := deleteKeyCommand()
deleteKeyCommand := DeleteKeyCommand()
mockIn, _, _ := tests.ApplyMockIO(deleteKeyCommand)

yesF, _ := deleteKeyCommand.Flags().GetBool(flagYes)
Expand Down
3 changes: 2 additions & 1 deletion client/keys/export.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import (
"github.com/cosmos/cosmos-sdk/client/input"
)

func exportKeyCommand() *cobra.Command {
// ExportKeyCommand exports private keys from the keybase
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func ExportKeyCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "export <name>",
Short: "Export private keys",
Expand Down
2 changes: 1 addition & 1 deletion client/keys/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

func Test_runExportCmd(t *testing.T) {
runningUnattended := isRunningUnattended()
exportKeyCommand := exportKeyCommand()
exportKeyCommand := ExportKeyCommand()
mockIn, _, _ := tests.ApplyMockIO(exportKeyCommand)

// Now add a temporary keybase
Expand Down
3 changes: 2 additions & 1 deletion client/keys/import.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ import (
"github.com/cosmos/cosmos-sdk/client/input"
)

func importKeyCommand() *cobra.Command {
// ImportKeyCommand imports private keys from a keyfile
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func ImportKeyCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "import <name> <keyfile>",
Short: "Import private keys into the local keybase",
Expand Down
2 changes: 1 addition & 1 deletion client/keys/import_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

func Test_runImportCmd(t *testing.T) {
runningUnattended := isRunningUnattended()
importKeyCommand := importKeyCommand()
importKeyCommand := ImportKeyCommand()
mockIn, _, _ := tests.ApplyMockIO(importKeyCommand)

// Now add a temporary keybase
Expand Down
3 changes: 2 additions & 1 deletion client/keys/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ import (
"github.com/cosmos/cosmos-sdk/client/flags"
)

func listKeysCmd() *cobra.Command {
// ListKeysCmd lists all keys in the keybase
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func ListKeysCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "list",
Short: "List all keys",
Expand Down
2 changes: 1 addition & 1 deletion client/keys/list_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func Test_runListCmd(t *testing.T) {
args []string
}

cmdBasic := listKeysCmd()
cmdBasic := ListKeysCmd()

// Prepare some keybases
kbHome1, cleanUp1 := tests.NewTestCaseDir(t)
Expand Down
3 changes: 2 additions & 1 deletion client/keys/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import (
// is not needed for importing into the Keyring keystore.
const migratePassphrase = "NOOP_PASSPHRASE"

func migrateCommand() *cobra.Command {
// MigrateCommand migrates key information from legacy keybase to OS secret store
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func MigrateCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "migrate",
Short: "Migrate key information from the lagacy key database to the OS secret store, or encrypted file store as a fall-back and save it",
Expand Down
4 changes: 2 additions & 2 deletions client/keys/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
)

func Test_runMigrateCmd(t *testing.T) {
cmd := addKeyCommand()
cmd := AddKeyCommand()
assert.NotNil(t, cmd)
mockIn, _, _ := tests.ApplyMockIO(cmd)

Expand All @@ -29,7 +29,7 @@ func Test_runMigrateCmd(t *testing.T) {
assert.NoError(t, err)

viper.Set(flags.FlagDryRun, true)
cmd = migrateCommand()
cmd = MigrateCommand()
mockIn, _, _ = tests.ApplyMockIO(cmd)
mockIn.Reset("test1234\n")
assert.NoError(t, runMigrateCmd(cmd, []string{}))
Expand Down
3 changes: 2 additions & 1 deletion client/keys/mnemonic.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ const (
mnemonicEntropySize = 256
)

func mnemonicKeyCommand() *cobra.Command {
// MnemonicKeyCommand computes the bip39 memonic for input entropy
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func MnemonicKeyCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "mnemonic",
Short: "Compute the bip39 mnemonic for some input entropy",
Expand Down
4 changes: 2 additions & 2 deletions client/keys/mnemonic_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,13 @@ import (
)

func Test_RunMnemonicCmdNormal(t *testing.T) {
cmdBasic := mnemonicKeyCommand()
cmdBasic := MnemonicKeyCommand()
err := runMnemonicCmd(cmdBasic, []string{})
require.NoError(t, err)
}

func Test_RunMnemonicCmdUser(t *testing.T) {
cmdUser := mnemonicKeyCommand()
cmdUser := MnemonicKeyCommand()
err := cmdUser.Flags().Set(flagUserEntropy, "1")
assert.NoError(t, err)

Expand Down
7 changes: 4 additions & 3 deletions client/keys/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ func (bo bech32Output) String() string {
return fmt.Sprintf("Bech32 Formats:\n%s", strings.Join(out, "\n"))
}

func parseKeyStringCommand() *cobra.Command {
// ParseKeyStringCommand parses an address from hex to bech32 and vice versa
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func ParseKeyStringCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "parse <hex-or-bech32-address>",
Short: "Parse address from hex to bech32 and vice versa",
Expand Down Expand Up @@ -124,9 +125,9 @@ func displayParseKeyInfo(stringer fmt.Stringer) {
case OutputFormatJSON:

if viper.GetBool(flags.FlagIndentResponse) {
out, err = cdc.MarshalJSONIndent(stringer, "", " ")
out, err = KeysCdc.MarshalJSONIndent(stringer, "", " ")
} else {
out = cdc.MustMarshalJSON(stringer)
out = KeysCdc.MustMarshalJSON(stringer)
}

}
Expand Down
20 changes: 10 additions & 10 deletions client/keys/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,17 @@ func Commands() *cobra.Command {
needs to sign with a private key.`,
}
cmd.AddCommand(
mnemonicKeyCommand(),
addKeyCommand(),
exportKeyCommand(),
importKeyCommand(),
listKeysCmd(),
showKeysCmd(),
MnemonicKeyCommand(),
AddKeyCommand(),
ExportKeyCommand(),
ImportKeyCommand(),
ListKeysCmd(),
ShowKeysCmd(),
flags.LineBreak,
deleteKeyCommand(),
updateKeyCommand(),
parseKeyStringCommand(),
migrateCommand(),
DeleteKeyCommand(),
UpdateKeyCommand(),
ParseKeyStringCommand(),
MigrateCommand(),
)
return cmd
}
3 changes: 2 additions & 1 deletion client/keys/show.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ const (
defaultMultiSigKeyName = "multi"
)

func showKeysCmd() *cobra.Command {
// ShowKeysCmd shows key info for a key name
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func ShowKeysCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "show [name [name...]]",
Short: "Show key info for the given name",
Expand Down
4 changes: 2 additions & 2 deletions client/keys/show_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,15 @@ func Test_multiSigKey_Properties(t *testing.T) {
}

func Test_showKeysCmd(t *testing.T) {
cmd := showKeysCmd()
cmd := ShowKeysCmd()
require.NotNil(t, cmd)
require.Equal(t, "false", cmd.Flag(FlagAddress).DefValue)
require.Equal(t, "false", cmd.Flag(FlagPublicKey).DefValue)
}

func Test_runShowCmd(t *testing.T) {
runningUnattended := isRunningUnattended()
cmd := showKeysCmd()
cmd := ShowKeysCmd()
mockIn, _, _ := tests.ApplyMockIO(cmd)
require.EqualError(t, runShowCmd(cmd, []string{"invalid"}), "The specified item could not be found in the keyring")
require.EqualError(t, runShowCmd(cmd, []string{"invalid1", "invalid2"}), "The specified item could not be found in the keyring")
Expand Down
3 changes: 2 additions & 1 deletion client/keys/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import (
"github.com/cosmos/cosmos-sdk/client/input"
)

func updateKeyCommand() *cobra.Command {
// UpdateKeyCommand changes the password of a key in the keybase
austinabell marked this conversation as resolved.
Show resolved Hide resolved
func UpdateKeyCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "update <name>",
Short: "Change the password used to protect private key",
Expand Down
4 changes: 2 additions & 2 deletions client/keys/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

func Test_updateKeyCommand(t *testing.T) {
cmd := updateKeyCommand()
cmd := UpdateKeyCommand()
assert.NotNil(t, cmd)
// No flags or defaults to validate
}
Expand All @@ -20,7 +20,7 @@ func Test_runUpdateCmd(t *testing.T) {
fakeKeyName1 := "runUpdateCmd_Key1"
fakeKeyName2 := "runUpdateCmd_Key2"

cmd := updateKeyCommand()
cmd := UpdateKeyCommand()

// fails because it requests a password
assert.EqualError(t, runUpdateCmd(cmd, []string{fakeKeyName1}), "EOF")
Expand Down
Loading