From 759421ebbf4643713c8d76ffbda715970adf0dba Mon Sep 17 00:00:00 2001 From: Evgenii Stratonikov Date: Wed, 19 Jan 2022 13:56:10 +0300 Subject: [PATCH] [#1083] neofs-cli: use single flag for key and wallet Currently have static priority of what key is used irregardless of whether a flag was provided via CLI or in config. This makes it impossible to override some of the config settings. While we could try to check if the key is provided by CLI by binding CLI flag under to viper under a different name the same problem would occur for config/environment variables. Fixing all of this with current set of keys is too complicate. In this commit we revert changes from #610 and use a single flag for all types of keys. Signed-off-by: Evgenii Stratonikov --- cmd/neofs-cli/modules/accounting.go | 4 - cmd/neofs-cli/modules/control.go | 2 - cmd/neofs-cli/modules/key_test.go | 123 ++++++++++++++++++++++++++++ cmd/neofs-cli/modules/root.go | 53 +++++------- cmd/neofs-cli/modules/util.go | 4 - 5 files changed, 145 insertions(+), 41 deletions(-) create mode 100644 cmd/neofs-cli/modules/key_test.go diff --git a/cmd/neofs-cli/modules/accounting.go b/cmd/neofs-cli/modules/accounting.go index 83868a49..5e0cc50b 100644 --- a/cmd/neofs-cli/modules/accounting.go +++ b/cmd/neofs-cli/modules/accounting.go @@ -23,9 +23,7 @@ var accountingCmd = &cobra.Command{ PersistentPreRun: func(cmd *cobra.Command, args []string) { flags := cmd.Flags() - _ = viper.BindPFlag(binaryKey, flags.Lookup(binaryKey)) _ = viper.BindPFlag(walletPath, flags.Lookup(walletPath)) - _ = viper.BindPFlag(wif, flags.Lookup(wif)) _ = viper.BindPFlag(address, flags.Lookup(address)) _ = viper.BindPFlag(rpc, flags.Lookup(rpc)) _ = viper.BindPFlag(verbose, flags.Lookup(verbose)) @@ -68,9 +66,7 @@ var accountingBalanceCmd = &cobra.Command{ func initAccountingBalanceCmd() { ff := accountingBalanceCmd.Flags() - ff.StringP(binaryKey, binaryKeyShorthand, binaryKeyDefault, binaryKeyUsage) ff.StringP(walletPath, walletPathShorthand, walletPathDefault, walletPathUsage) - ff.StringP(wif, wifShorthand, wifDefault, wifUsage) ff.StringP(address, addressShorthand, addressDefault, addressUsage) ff.StringP(rpc, rpcShorthand, rpcDefault, rpcUsage) ff.BoolP(verbose, verboseShorthand, verboseDefault, verboseUsage) diff --git a/cmd/neofs-cli/modules/control.go b/cmd/neofs-cli/modules/control.go index 6d08f697..8a76acc7 100644 --- a/cmd/neofs-cli/modules/control.go +++ b/cmd/neofs-cli/modules/control.go @@ -25,9 +25,7 @@ var controlCmd = &cobra.Command{ ff := cmd.Flags() _ = viper.BindPFlag(generateKey, ff.Lookup(generateKey)) - _ = viper.BindPFlag(binaryKey, ff.Lookup(binaryKey)) _ = viper.BindPFlag(walletPath, ff.Lookup(walletPath)) - _ = viper.BindPFlag(wif, ff.Lookup(wif)) _ = viper.BindPFlag(address, ff.Lookup(address)) _ = viper.BindPFlag(controlRPC, ff.Lookup(controlRPC)) _ = viper.BindPFlag(verbose, ff.Lookup(verbose)) diff --git a/cmd/neofs-cli/modules/key_test.go b/cmd/neofs-cli/modules/key_test.go new file mode 100644 index 00000000..5b35cef0 --- /dev/null +++ b/cmd/neofs-cli/modules/key_test.go @@ -0,0 +1,123 @@ +package cmd + +import ( + "bytes" + "errors" + "io/ioutil" + "os" + "path" + "path/filepath" + "testing" + + "github.com/nspcc-dev/neo-go/cli/input" + "github.com/nspcc-dev/neo-go/pkg/crypto/keys" + "github.com/nspcc-dev/neo-go/pkg/wallet" + "github.com/spf13/viper" + "github.com/stretchr/testify/require" + "golang.org/x/term" +) + +func Test_getKey(t *testing.T) { + dir := t.TempDir() + + wallPath := path.Join(dir, "wallet.json") + w, err := wallet.NewWallet(wallPath) + require.NoError(t, err) + + acc1, err := wallet.NewAccount() + require.NoError(t, err) + require.NoError(t, acc1.Encrypt("pass", keys.NEP2ScryptParams())) + w.AddAccount(acc1) + + acc2, err := wallet.NewAccount() + require.NoError(t, err) + require.NoError(t, acc2.Encrypt("pass", keys.NEP2ScryptParams())) + acc2.Default = true + w.AddAccount(acc2) + require.NoError(t, w.Save()) + w.Close() + + keyPath := path.Join(dir, "binary.key") + rawKey, err := keys.NewPrivateKey() + require.NoError(t, err) + require.NoError(t, ioutil.WriteFile(keyPath, rawKey.Bytes(), os.ModePerm)) + + wifKey, err := keys.NewPrivateKey() + require.NoError(t, err) + + nep2Key, err := keys.NewPrivateKey() + require.NoError(t, err) + nep2, err := keys.NEP2Encrypt(nep2Key, "pass", keys.NEP2ScryptParams()) + require.NoError(t, err) + + in := bytes.NewBuffer(nil) + input.Terminal = term.NewTerminal(input.ReadWriter{ + Reader: in, + Writer: ioutil.Discard, + }, "") + + checkKeyError(t, filepath.Join(dir, "badfile"), errInvalidKey) + + t.Run("wallet", func(t *testing.T) { + checkKeyError(t, wallPath, errInvalidPassword) + + in.WriteString("invalid\r") + checkKeyError(t, wallPath, errInvalidPassword) + + in.WriteString("pass\r") + checkKey(t, wallPath, acc2.PrivateKey()) // default account + + viper.Set(address, acc1.Address) + in.WriteString("pass\r") + checkKey(t, wallPath, acc1.PrivateKey()) + + viper.Set(address, "not an address") + checkKeyError(t, wallPath, errInvalidAddress) + + acc, err := wallet.NewAccount() + require.NoError(t, err) + viper.Set(address, acc.Address) + checkKeyError(t, wallPath, errInvalidAddress) + }) + + t.Run("WIF", func(t *testing.T) { + checkKey(t, wifKey.WIF(), wifKey) + }) + + t.Run("NEP-2", func(t *testing.T) { + checkKeyError(t, nep2, errInvalidPassword) + + in.WriteString("invalid\r") + checkKeyError(t, nep2, errInvalidPassword) + + in.WriteString("pass\r") + checkKey(t, nep2, nep2Key) + }) + + t.Run("raw key", func(t *testing.T) { + checkKey(t, keyPath, rawKey) + }) + + t.Run("generate", func(t *testing.T) { + viper.Set(generateKey, true) + actual, err := getKey() + require.NoError(t, err) + require.NotNil(t, actual) + for _, p := range []*keys.PrivateKey{nep2Key, rawKey, wifKey, acc1.PrivateKey(), acc2.PrivateKey()} { + require.NotEqual(t, p, actual, "expected new key to be generated") + } + }) +} + +func checkKeyError(t *testing.T, desc string, err error) { + viper.Set(walletPath, desc) + _, actualErr := getKey() + require.True(t, errors.Is(actualErr, err), "got: %v", actualErr) +} + +func checkKey(t *testing.T, desc string, expected *keys.PrivateKey) { + viper.Set(walletPath, desc) + actual, err := getKey() + require.NoError(t, err) + require.Equal(t, &expected.PrivateKey, actual) +} diff --git a/cmd/neofs-cli/modules/root.go b/cmd/neofs-cli/modules/root.go index 1aef0e39..0979a25a 100644 --- a/cmd/neofs-cli/modules/root.go +++ b/cmd/neofs-cli/modules/root.go @@ -44,20 +44,10 @@ const ( generateKeyDefault = false generateKeyUsage = "generate new private key" - binaryKey = "binary-key" - binaryKeyShorthand = "" - binaryKeyDefault = "" - binaryKeyUsage = "path to the raw private key file" - walletPath = "wallet" walletPathShorthand = "w" walletPathDefault = "" - walletPathUsage = "path to the wallet" - - wif = "wif" - wifShorthand = "" - wifDefault = "" - wifUsage = "WIF or NEP-2" + walletPathUsage = "WIF (NEP-2) string or path to the wallet or binary key" address = "address" addressShorthand = "" @@ -183,29 +173,34 @@ func getKey() (*ecdsa.PrivateKey, error) { return &priv.PrivateKey, nil } - if keyPath := viper.GetString(binaryKey); keyPath != "" { - return getKeyFromFile(keyPath) + // Ideally we want to touch file-system on the last step. + // However, asking for NEP-2 password seems to be confusing if we provide a wallet. + // Thus we try keys in the following order: + // 1. WIF + // 2. Raw binary key + // 3. Wallet file + // 4. NEP-2 encrypted WIF. + keyDesc := viper.GetString(walletPath) + priv, err := keys.NewPrivateKeyFromWIF(keyDesc) + if err == nil { + return &priv.PrivateKey, nil } - if walletPath := viper.GetString(walletPath); walletPath != "" { - w, err := wallet.NewWalletFromFile(walletPath) - if err != nil { - return nil, fmt.Errorf("%w: %v", errInvalidKey, err) - } + p, err := getKeyFromFile(keyDesc) + if err == nil { + return p, nil + } + + w, err := wallet.NewWalletFromFile(keyDesc) + if err == nil { return getKeyFromWallet(w, viper.GetString(address)) } - wif := viper.GetString(wif) - if len(wif) == nep2Base58Length { - return getKeyFromNEP2(wif) + if len(keyDesc) == nep2Base58Length { + return getKeyFromNEP2(keyDesc) } - priv, err := keys.NewPrivateKeyFromWIF(wif) - if err != nil { - return nil, fmt.Errorf("%w: %v", errInvalidKey, err) - } - - return &priv.PrivateKey, nil + return nil, errInvalidKey } func getKeyFromFile(keyPath string) (*ecdsa.PrivateKey, error) { @@ -404,9 +399,7 @@ func initCommonFlags(cmd *cobra.Command) { ff := cmd.Flags() ff.BoolP(generateKey, generateKeyShorthand, generateKeyDefault, generateKeyUsage) - ff.StringP(binaryKey, binaryKeyShorthand, binaryKeyDefault, binaryKeyUsage) ff.StringP(walletPath, walletPathShorthand, walletPathDefault, walletPathUsage) - ff.StringP(wif, wifShorthand, wifDefault, wifUsage) ff.StringP(address, addressShorthand, addressDefault, addressUsage) ff.StringP(rpc, rpcShorthand, rpcDefault, rpcUsage) ff.BoolP(verbose, verboseShorthand, verboseDefault, verboseUsage) @@ -417,9 +410,7 @@ func bindCommonFlags(cmd *cobra.Command) { ff := cmd.Flags() _ = viper.BindPFlag(generateKey, ff.Lookup(generateKey)) - _ = viper.BindPFlag(binaryKey, ff.Lookup(binaryKey)) _ = viper.BindPFlag(walletPath, ff.Lookup(walletPath)) - _ = viper.BindPFlag(wif, ff.Lookup(wif)) _ = viper.BindPFlag(address, ff.Lookup(address)) _ = viper.BindPFlag(rpc, ff.Lookup(rpc)) _ = viper.BindPFlag(verbose, ff.Lookup(verbose)) diff --git a/cmd/neofs-cli/modules/util.go b/cmd/neofs-cli/modules/util.go index ab0daf7e..f88a0579 100644 --- a/cmd/neofs-cli/modules/util.go +++ b/cmd/neofs-cli/modules/util.go @@ -33,9 +33,7 @@ var ( flags := cmd.Flags() _ = viper.BindPFlag(generateKey, flags.Lookup(generateKey)) - _ = viper.BindPFlag(binaryKey, flags.Lookup(binaryKey)) _ = viper.BindPFlag(walletPath, flags.Lookup(walletPath)) - _ = viper.BindPFlag(wif, flags.Lookup(wif)) _ = viper.BindPFlag(address, flags.Lookup(address)) _ = viper.BindPFlag(verbose, flags.Lookup(verbose)) }, @@ -194,9 +192,7 @@ func initCommonFlagsWithoutRPC(cmd *cobra.Command) { flags := cmd.Flags() flags.BoolP(generateKey, generateKeyShorthand, generateKeyDefault, generateKeyUsage) - flags.StringP(binaryKey, binaryKeyShorthand, binaryKeyDefault, binaryKeyUsage) flags.StringP(walletPath, walletPathShorthand, walletPathDefault, walletPathUsage) - flags.StringP(wif, wifShorthand, wifDefault, wifUsage) flags.StringP(address, addressShorthand, addressDefault, addressUsage) flags.BoolP(verbose, verboseShorthand, verboseDefault, verboseUsage) }