cmd/ethkey: new tool for key files #15438

stevenroose · 2017-11-08T18:13:56Z

Since I failed building Parity's similar ethkey command line tool, I implemented this one in Go.

It is based entirely on functionality in the codebase, just interfaces them easily through a CLI.

There is currently one issue: v1.20.0 from urfave/cli breaks the --help flag on commands. I created upstream issue urfave/cli#678 to track this and will add a commit updating the cli dependency when it is resolved.

GitCop · 2017-11-08T18:13:57Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 74ba4982442e1a8f85b1c92787b366b131d1a6f6
Commits must be prefixed with the package(s) they modify
Commit: 234744a16adbb3793d3a4ea5ae430c68e7c9cd31
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

GitCop · 2017-11-08T18:20:41Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: b75596104fba3e7a66c5ad471ce175b9f9529339
Commits must be prefixed with the package(s) they modify
Commit: 3e38d0a88c040eb2d5d752e6898ca69236a2fe5e
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

holiman · 2017-11-09T08:33:26Z

Have you seen https://github.com/bas-vk/ethsign ? Is there anythiing which differs in the functionality between ethsign (https://github.com/bas-vk/ethsign) and your proposed utility?

Because I'd rather get the code into ethsign and then (perhaps) merge that into a standalone binary. ethsign relies on the account storage, which can either be raw keys, but also other things, such as hardware wallets.

stevenroose · 2017-11-09T09:07:23Z

I didn't know about ethsign. It seems to use keystores though. And it is an interactive shell instead of a standalone CLI. What I needed it for was dumping the private key. I didn't find a tool that could do it. Ethsign doesn't do it either.

…

On 9 Nov 2017 9:33 am, "Martin Holst Swende" ***@***.***> wrote: Have you seen https://github.com/bas-vk/ethsign ? Is there anythiing which differs in the functionality between ethsign ( https://github.com/bas-vk/ethsign) and your proposed utility? Because I'd rather get the code into ethsign and then (perhaps) merge that into a standalone binary. ethsign relies on the account storage, which can either be raw keys, but also other things, such as hardware wallets. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15438 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0F3LIbwH4afNhrBktaQJAhI7gl2unmks5s0rjugaJpZM4QWz3G> .

fjl · 2017-11-09T09:28:39Z

Makefile

@@ -21,6 +21,11 @@ swarm:
 	@echo "Done building."
 	@echo "Run \"$(GOBIN)/swarm\" to launch swarm."

+ethkey:


There is no need to add this command to Makefile. It can be built using make all.

I removed it.

fjl · 2017-11-09T09:28:39Z

cmd/ethkey/main.go

+)
+
+var ( // Commonly used command line flags.
+	passphraseFlag = cli.StringFlag{


This is a dangerous flag because it encourages setting the flag on the command line, where it might end up in history. Command line arguments are visible to any user. Please remove the flag.

Well, it's one in three ways. For testing or for the large quantities of files with an empty passphrase, I think it's worth it. I just made a change that makes --passphrase "" work for files with empty passphrases. I think it's easier than touch tempfile; ethkey --passphrasefile tempfile :D

You can always pipe empty strings into the interactive password input to do stuff like that.

fjl · 2017-11-09T09:28:39Z

cmd/ethkey/main.go

@@ -0,0 +1,322 @@
+package main


New files in cmd/ need the GPLv3 header. Please copy it from cmd/geth/main.go.

fjl · 2017-11-09T09:28:40Z

cmd/ethkey/main.go

+
+// Configure the app instance.
+func init() {
+	app = cli.NewApp()


Please use utils.NewApp, which sets version automatically. For its gitCommit, define a global variable called gitCommit. It will be filled in by the linker.

Please don't set app.Copyright.

The reason why I avoided that is because utils.NewApp sets the version to the geth version. That might be misleading since geth versions might change while the functionality of this CLI will remain the same. (That's actually very likely given the simplicity of this tool.)

All tools including rlpdump, evm, swarm use the same version. Releases in this repo are go-ethereum releases, not just geth releases.

That's fair enough.

fjl · 2017-11-09T09:28:40Z

cmd/ethkey/main.go

+		keyfilepath = defaultKeyfilePath
+	}
+	var err error
+	keyfilepath, err = filepath.Abs(keyfilepath)


Why do you need the absolute path?

os.Stat was failing for some reason while I was testing. I tried without and it works fine, so I remove all the filepath.Abs calls. Thanks for the critical suggestion :)

fjl · 2017-11-09T09:29:06Z

Thank you for this PR! It's a good addition to the toolchain.

fjl · 2017-11-10T10:56:47Z

cmd/ethkey/README.md

+
+# Usage
+
+## generate


Please use full command as heading, e.g.

## `ethkey generate` Generates a new keyfile.

Done. Not sure how well that would work with the longer arguments in sign and verify, though. Let me check how GitHub renders it here :)

Doesn't look that nice. I'll probably change that to regular text

*`ethkey generate`*

stevenroose · 2017-11-10T14:15:46Z

I'm also thinking about a --json flag that prints results in json so that it is machine-usable. F.e. you could store the private key of a keyfile using the following command:

ethkey inspect --passphrase "" myfile.json --json | jq '.privateKey' > myfile.key

GitCop · 2017-11-10T16:52:20Z

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

Commit: 28077828f3f82fb0532e37865438352c3512e306
Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.

This message was auto-generated by https://gitcop.com

stevenroose · 2017-11-13T10:10:43Z

One extra thing I'd like to add before having this merged is creating a keyfile from a raw private key.

holiman · 2017-11-29T10:33:08Z

I am not overly fond of making it so easy to extract the raw private keys. I mean, sure, it should be possible , but I think it's good that it's not easy. People are, today, not exporting and importing raw private keys, and I think that's a good thing.

By releasing this, we're enabling people to handle raw privkeys on usb-sticks between their computers. And I'm not sure that's a net benefit.

stevenroose · 2017-11-29T10:42:20Z

@holiman The reason why I created this for myself in the first place was to be able to extract the raw private keys. I agree that it should be hard, but it should be possible without having to write your own tool.. There is really not a single other way to get the raw private key than to create something like this yourself.

It doesn't have to be advertised. I could also f.e. have inspect only print public info and have inspect -p print the private info (to not accidentally have your private key in stdout).

holiman · 2017-11-29T10:49:07Z

@stevenroose exporting the raw private key should, imo, at least involve being shown a very explicit warning about the operation, and having to enter some text to confirm - maybe random text, e.g. "Do you want to proceed? Enter 93281 to confirm".

fjl · 2017-11-29T10:56:13Z

I'm super happy about this PR and want to see ethkey in the next release. There's only a few remaining concerns that I have with this PR:

Should the tool handle transaction signing? I think yes, but we can add that in a subsequent PR.

Argument handling is complicated:

Users can pass the private key as a hex string or a file.
Users can pass the message to sign as a file or string.
There are too many ways to input the password (flag, file, terminal input).

I'll think about a good proposal for the latter issue.

stevenroose · 2017-11-29T11:14:37Z

Yeah that's always concerns for those kind of utilities. Too many ways to input data.

I will rename sign to signmessage to make sure it uses the Ethereum signed message: %s way for signing so it cannot be used for raw data.

Input is very welcome for the way we handle input data.

@fjl what is your opinion about outputting private keys? I'm personally against some random input like @holiman proposed. But I'd be ok with moving it to a --private flag. I just want to make sure the CLI stays usable by machines using the --json output.

stevenroose · 2017-11-29T17:50:51Z

On an extra note: most of those "too many options to input data" comes to making the code messy and harder to maintain. So I believe a great deal can be done code-wise to make it acceptable. Having a ton of flags is not necessarily a problem, most users will just look for the one they need, find it, and use it.

So if we can keep it clean code-wise, I think it should be acceptable.

EDIT: Next to that, intuitive defaults are also key. Extra functionality can confuse users, but having a very simple default functionality can resolve that for a big part.

fjl · 2017-12-01T12:43:15Z

Not sure about inputting random text. I can see both sides. My own view is that users already need to enter the password to dump the key so maybe that's good enough. Other ideas:

Give an unwieldy name to the subcommand that does this, e.g. ethkey export-private-key
Do not provide a way to dump into a file from the command itself, output the raw private key bytes to stdout so they can be copied/pasted without hitting disk. Users can still use shell redirection if they absolutely want it in a file.

About "too many ways to input data":

ethkey should be a sharp, focused tool.

There should be one way to provide the private key. IMHO that way should be "encrypted key file". All subcommands should take a key path as the (required) first argument, pointing to a JSON file on disk. That's sufficient for the beginning. We can expand this at some point if lots of people see the need to deal with keys in a different way.

Likewise, we should reduce the number of ways to input the passphrase. My suggestion is to only accept it via stdin because it is the most secure/private way to pass data to another program. Users can always script themselves out of this by piping it in.

There is a conflict for the signing commands: we can't read passphrase and message from the same stdin stream. IMHO these commands should accept the unsigned tx/message as the second argument (because unsigned tx/message needs less security than the passphrase). There can be a local flag (e.g. -f) to read it from a file instead.

For output, I'm fine with having a human-readable and machine-readable (JSON) version.

holiman · 2017-12-01T13:48:57Z

I agree with the points @fjl made. However, I still think ethkey export-private-key should be accompanied with a text, something like this:

WARNING!
This will output the raw private key to the console. A raw private key gives total 
access to the corresponding account. Use extreme caution when handling raw 
keys. Most tools can handle encrypted keyfiles, and you don't normally even
need the raw private key. 

Are you quite sure you want to export the raw private key? [y/N]

As Felix says, you can use bash to skip through that anyway, but at least it would be harder to trick an unsuspecting user into doing it.

stevenroose · 2017-12-07T15:08:59Z

Made some changes. Most of the comments should have been addressed.

All subcommands should take a key path as the (required) first argument, pointing to a JSON file on disk.

This is now true, except of course for verifymessage since it has to take an address, verifying when you have the keyfile makes little sense.

However, I still think ethkey export-private-key should be accompanied with a text, something like this:

This is not true right now, but the private key is hidden behind a --private flag. Requiring additional input makes it too hard to use the tool from a shell script.

My suggestion is to only accept it via stdin because it is the most secure/private way to pass data to another program. Users can always script themselves out of this by piping it in.

I kept a flag to point to a passprase file (a lot of keyfiles have an empty passphrase so they can be used by passing /dev/null in this case). No longer being able to pass the passphrase literally in the arguments prevents leaking it into shell history.

It's a tool that serves as a command line interface to the basic key management functionalities of geth. It currently supports: - generating keyfiles - inspecting keyfiles (print public and private key) - signing messages - verifying signed messages

ethkey is a new tool that serves as a command line interface to the basic key management functionalities of geth. It currently supports: - generating keyfiles - inspecting keyfiles (print public and private key) - signing messages - verifying signed messages

stevenroose force-pushed the ethkey branch from 234744a to 3e38d0a Compare November 8, 2017 18:20

stevenroose force-pushed the ethkey branch from 3e38d0a to 9cb6e6c Compare November 8, 2017 18:23

fjl requested changes Nov 9, 2017

View reviewed changes

stevenroose force-pushed the ethkey branch 2 times, most recently from 8a9db48 to 4418c15 Compare November 10, 2017 10:13

fjl reviewed Nov 10, 2017

View reviewed changes

stevenroose force-pushed the ethkey branch from 4418c15 to 2d84599 Compare November 10, 2017 11:23

fjl changed the title ~~Add ethkey command line tool~~ cmd/ethkey: new tool for key files Nov 10, 2017

stevenroose force-pushed the ethkey branch from 2807782 to 5b02627 Compare November 10, 2017 16:52

fjl added this to the 1.8.0 milestone Nov 13, 2017

stevenroose force-pushed the ethkey branch from 5b02627 to 89cf0d0 Compare December 7, 2017 15:08

stevenroose force-pushed the ethkey branch from 89cf0d0 to 9f013e3 Compare December 7, 2017 15:10

fjl approved these changes Dec 21, 2017

View reviewed changes

fjl merged commit eeb53bc into ethereum:master Dec 21, 2017

stevenroose mentioned this pull request Jan 23, 2018

cmd/ethkey: --help doesn't work on subcommands #15952

Closed

5chdn mentioned this pull request Jan 30, 2018

Merge go-ethereum updates into 2.5.4 Musicoin/go-musicoin#77

Merged

holiman mentioned this pull request Feb 8, 2018

Mobile, accounts/keystore: export private key #16024

Closed

epappas mentioned this pull request Aug 7, 2018

cmd, ethkey: Fixing the README of ethkey, to match the updated commands #17332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/ethkey: new tool for key files #15438

cmd/ethkey: new tool for key files #15438

stevenroose commented Nov 8, 2017

GitCop commented Nov 8, 2017

GitCop commented Nov 8, 2017

holiman commented Nov 9, 2017

stevenroose commented Nov 9, 2017 via email

fjl Nov 9, 2017

stevenroose Nov 10, 2017

fjl Nov 9, 2017

stevenroose Nov 10, 2017

fjl Nov 10, 2017

fjl Nov 9, 2017

stevenroose Nov 10, 2017

fjl Nov 9, 2017

stevenroose Nov 10, 2017

fjl Nov 10, 2017

stevenroose Nov 10, 2017

stevenroose Nov 10, 2017

fjl Nov 9, 2017

stevenroose Nov 10, 2017

fjl commented Nov 9, 2017

fjl Nov 10, 2017

stevenroose Nov 10, 2017

stevenroose Nov 10, 2017

stevenroose commented Nov 10, 2017

GitCop commented Nov 10, 2017

stevenroose commented Nov 13, 2017

holiman commented Nov 29, 2017

stevenroose commented Nov 29, 2017

holiman commented Nov 29, 2017

fjl commented Nov 29, 2017

stevenroose commented Nov 29, 2017

stevenroose commented Nov 29, 2017 •

edited

Loading

fjl commented Dec 1, 2017 •

edited

Loading

holiman commented Dec 1, 2017

stevenroose commented Dec 7, 2017

cmd/ethkey: new tool for key files #15438

cmd/ethkey: new tool for key files #15438

Conversation

stevenroose commented Nov 8, 2017

GitCop commented Nov 8, 2017

GitCop commented Nov 8, 2017

holiman commented Nov 9, 2017

stevenroose commented Nov 9, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjl commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenroose commented Nov 10, 2017

GitCop commented Nov 10, 2017

stevenroose commented Nov 13, 2017

holiman commented Nov 29, 2017

stevenroose commented Nov 29, 2017

holiman commented Nov 29, 2017

fjl commented Nov 29, 2017

stevenroose commented Nov 29, 2017

stevenroose commented Nov 29, 2017 • edited Loading

fjl commented Dec 1, 2017 • edited Loading

holiman commented Dec 1, 2017

stevenroose commented Dec 7, 2017

stevenroose commented Nov 29, 2017 •

edited

Loading

fjl commented Dec 1, 2017 •

edited

Loading