Signing CLI #566

lukasz-zimnoch · 2020-09-30T10:19:10Z

This pull request introduces an out-of-band signing CLI.

That signing CLI contains the following commands:

decrypt-key-share: allows to decrypt a key share for given keep and store it in a file using the client config toml
sign-digest: allows signing a given digest using provided key shares

Implemented a decrypt key share command as part of the new signing cli. That command allows to decrypt a key share for given keep and store it in a file using the provided client config toml.

Implemented a sign command which allows to sign a given digest using key shares in the provided directory.

Shadowfiend

Left some thoughts on how to make this a little more “command”-y.

Shadowfiend · 2020-09-30T13:30:00Z

cmd/signing.go

+					cli.StringFlag{
+						Name:  "keep-address,k",
+						Usage: "address of the keep",
+					},


Should this be a flag or just a parameter? Feels like ./keep-ecdsa signing decrypt-key-share <keep-address> makes more sense since the command without the flag can't do anything.

Shadowfiend · 2020-09-30T13:30:34Z

cmd/signing.go

+						Name: "digest,d",
+						Usage: "digest to sign in hex format without " +
+							"the `0x` prefix",
+					},


As with decrypt-key-share above, how about we just make this an argument rather than a flag?

Shadowfiend · 2020-09-30T14:09:23Z

cmd/signing.go

+		"key_share_%.10s_%.10s",
+		keepAddress.String(),
+		keyFile.Address.String(),
+	)


Rather than try to generate a target file, since this is meant to be invoked manually, what do you think about just spitting the bytes out to stdout? Then the caller can redirect to a file as desired, and we're not trying to guess filenames. Longer term or alternatively, we could provide a -o/--output-file flag to specify a filename.

Shadowfiend · 2020-09-30T14:11:57Z

cmd/signing.go

+	logger.Infof(
+		"key share has been decrypted successfully and written to file [%s]",
+		targetFilePath,
+	)


We should also probably avoid the logger here IMO. This is a CLI utility, rather than a running system---if we have user-facing messages to convey, we should be using simple fmt.Printf/fmt.Println calls. Logging should be reserved for some sort of debug or verbose flag, though I don't know that we need to go that far right now.

I think if we rework this to function more like a command-line utility, e.g. by outputting the decrypted data to stdout and potentially allowing a flag that specifies an output file, no additional output is needed. If there's an error, we can print it to stderr and exit 1 or so. The mental model for me is what happens when you call gpg to decrypt something.

Shadowfiend · 2020-09-30T14:30:28Z

cmd/signing.go

+							"the `0x` prefix",
+					},
+					cli.StringFlag{
+						Name: "key-shares-dir,k",


With the above change, we could make this flag a more standard and guessable directory,d as well? Alternatively, since it's not optional, I wonder if we make this an argument rather than a flag as well 🤔 We can leave it as a flag initially, I'm just always hesitant to put optional parameters behind flags---it's not typically how commands work.

Shadowfiend · 2020-09-30T15:07:55Z

cmd/signing.go

+				"[signer:%v] error: [%v]",
+				signingOutcome.signerIndex,
+				signingOutcome.err,
+			)


As above, this feels like something we should do as not-a-log, printing to stderr with something solely for human consumption.

Shadowfiend · 2020-09-30T15:11:56Z

cmd/signing.go

+		}
+
+		logger.Infof(
+			"[signer:%v] signature: [%+v]",


Is the outcome here a single signature? I don't want to slow us down too much, but I would expect this to:

Gather all available signatures.

If there are <len(signers), print an error to stderr and exit 1.

If there are len(signers) signatures and they are not all the same, print an error to stderr (possibly listing the three signatures by signer id) and exit 1.

If there are len(signers) signatures and they are all the same, print the signature to stdout and exit normally (which is to say, return).

I think the error to stderr + exit 1 is doable by just returning an error.

lukasz-zimnoch · 2020-10-01T10:42:45Z

Thanks for review @Shadowfiend! Ready for the next look.

Shadowfiend · 2020-10-01T19:46:02Z

cmd/signing.go

 		Name:  "signing",
 		Usage: "Provides several tools useful for out-of-band signing",
+		Before: func(c *cli.Context) error {
+			_ = log.SetLogLevel("*", "fatal") // disable the regular logger


Thoughts on going through our existing keep-common logging abstraction here and doing a logging.Configure("*", "fatal")? My thinking is it'll be easier to bridge this if and when we bump our logging dependencies to a different major version or so (e.g. with keep-network/keep-common#30).

Addressed in the follow-up PR: #568

Shadowfiend · 2020-10-01T19:47:55Z

cmd/signing.go

-	}
-
-	err = ioutil.WriteFile(targetFilePath, signerBytes, 0444) // read-only
+	_, err = os.Stdout.Write(signerBytes)


If we do support -o, we shouldn't write to stdout if the output file is specified.

Addressed in the follow-up PR: #568

Shadowfiend · 2020-10-01T20:00:04Z

cmd/signing.go

 	SigningCommand = cli.Command{
 		Name:  "signing",
 		Usage: "Provides several tools useful for out-of-band signing",
+		Before: func(c *cli.Context) error {


We can also use Before here and below to check the Args length and make sure it matches what we expect, returning an error if it does not.

Yep, but I feel it's a little bit clearer if we validate the arguments in the subcommand right in the place where we use them.

Shadowfiend · 2020-10-01T20:11:05Z

cmd/signing.go

+		if signersCount != len(signers) {
+			return fmt.Errorf(
+				"signing failed; all signers should support the signature",
+			)


Turns out I was totally wrong about returning an error. It does do us the favor of exiting with a non-zero status, but it prints the error out to a logger. This… Feels like a mistake, but probably not one we need to block this PR on. In particular, it feels like main.go should be fmt.Printf-ing an error from app.Run rather than using a logger.

Addressed in the follow-up PR: #568

Shadowfiend

I think this is ready to try on a real keep 😎

Shadowfiend · 2020-10-01T20:21:14Z

cmd/signing.go

+
+	keepRegistry := registry.NewKeepsRegistry(persistence)
+
+	keepRegistry.LoadExistingKeeps()


Actually a random note I missed making: we will need to make this work more flexibly. Right now you need a full config file and to have your path set up just right to decrypt the keyshare. We should be able to just point this to a keyshare and, if we have the password configured properly, have that keyshare decrypted to stdout. Otherwise if we have a snapshot that didn't get properly persisted, for example, you'd have to move it into the current directory to make this work.

For followup work though.

Agree but from the other side, seems we won't be able to use the existing registry code as we do here and we will be forced to copy-paste relevant parts.

Yeah I figured structuring it all right will be a bit more complicated.

Signing CLI - decrypt key share command

ccdf5d2

Implemented a decrypt key share command as part of the new signing cli. That command allows to decrypt a key share for given keep and store it in a file using the provided client config toml.

lukasz-zimnoch requested review from Shadowfiend and pdyraga September 30, 2020 10:27

lukasz-zimnoch marked this pull request as ready for review September 30, 2020 11:53

Signing CLI - sign digest command

ee03339

Implemented a sign command which allows to sign a given digest using key shares in the provided directory.

Shadowfiend suggested changes Sep 30, 2020

View reviewed changes

lukasz-zimnoch added 5 commits October 1, 2020 10:10

Pass keep address as argument

3ad2a63

Change sign-digest flags to be arguments

2a32271

Print decrypted shares to stdout

8856b8c

Fix stdout writing

1e10d33

Change sign-digest output

8aa57cd

lukasz-zimnoch requested a review from Shadowfiend October 1, 2020 10:42

Shadowfiend reviewed Oct 1, 2020

View reviewed changes

Shadowfiend approved these changes Oct 1, 2020

View reviewed changes

Shadowfiend merged commit 88b54e9 into master Oct 1, 2020

Shadowfiend deleted the signing-cli branch October 1, 2020 20:18

Shadowfiend reviewed Oct 1, 2020

View reviewed changes

pdyraga added this to the v1.3.0 milestone Oct 2, 2020

lukasz-zimnoch mentioned this pull request Oct 2, 2020

Improvements of the signing CLI #568

Merged

pdyraga modified the milestones: v1.4.0, v1.3.0 Oct 2, 2020

pdyraga added the 📟 client label Oct 2, 2020


		keepRegistry := registry.NewKeepsRegistry(persistence)

		keepRegistry.LoadExistingKeeps()

Signing CLI #566

Signing CLI #566

Uh oh!

Conversation

lukasz-zimnoch commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Shadowfiend left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasz-zimnoch commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shadowfiend left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lukasz-zimnoch commented Sep 30, 2020 •

edited

Loading