`notary key generate` shouldn't be restricted to root keys #1134

endophage · 2017-04-07T06:33:50Z

It also gains the -o/--output flag, which if provided will cause the public and private key to be output to the filepath specified in the flag with the .pub and .priv file extensions respectively.

Signed-off-by: David Lawrence [email protected] (github: endophage)

ecordell

This is a great addition :) I have a few UX concerns but I'm excited that this is being added.

ecordell · 2017-04-07T14:07:38Z

cmd/notary/keys.go

@@ -14,10 +14,13 @@ import (
 	store "github.com/docker/notary/storage"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
+	tufutils "github.com/docker/notary/tuf/utils"


I think in other packages this is tufUtils

Did a search in the project, it's consistently tufutils (all lower case)

ecordell · 2017-04-07T14:07:50Z

cmd/notary/keys.go

@@ -38,7 +41,7 @@ var cmdRotateKeyTemplate = usageTemplate{
 	Long:  `Generates a new key for the given Globally Unique Name and role (one of "snapshot", "targets", "root", or "timestamp").  If rotating to a server-managed key, a new key is requested from the server rather than generated.  If the generation or key request is successful, the key rotation is immediately published.  No other changes, even if they are staged, will be published.`,
 }

-var cmdKeyGenerateRootKeyTemplate = usageTemplate{
+var cmdKeyGenerateKeyTemplate = usageTemplate{
 	Use:   "generate [ algorithm ]",
 	Short: "Generates a new root key with a given algorithm.",


Should update the description

ecordell · 2017-04-07T14:09:07Z

cmd/notary/keys.go

-	cmd.AddCommand(cmdKeyGenerateRootKeyTemplate.ToCommand(k.keysGenerateRootKey))
+	cmdGenerate := cmdKeyGenerateKeyTemplate.ToCommand(k.keysGenerate)
+	cmdGenerate.Flags().StringVarP(
+		&k.outFile,


Should --output only output the public key? Or at least have different flags for pub/priv out?

I'm just thinking about the case where you're storing in a yubikey but still want the public part output to a file.

This is to work in tandem with the PR to update notary add which allows keys to be provided on the CLI. My expectation is this would be used to generate delegation keys, which at the moment is a pain as they have to be generated with OpenSSL.

ecordell · 2017-04-07T14:11:06Z

cmd/notary/keys.go

@@ -159,7 +170,7 @@ func (k *keyCommander) keysList(cmd *cobra.Command, args []string) error {
 	return nil
 }

-func (k *keyCommander) keysGenerateRootKey(cmd *cobra.Command, args []string) error {
+func (k *keyCommander) keysGenerate(cmd *cobra.Command, args []string) error {


Shouldn't this take a role now?

The command functions have a set signature required to meet one of the Cobra interfaces. It can't be changed.

Right! I meant as one of the command args, which you took care of.

Ah sorry, misunderstood!

ecordell · 2017-04-07T14:57:12Z

cmd/notary/keys.go

+		}
+		cs := cryptoservice.NewCryptoService(ks...)
+
+		pubKey, err := cs.Create(data.CanonicalRootRole, "", algorithm)


I think the UX is a little confusing - it generates a root key if no output is defined?

Yeah, I wasn't sure about how to handle that. I didn't want to add a flag while I was uncertain about whether we'd be following up the PKCS#8 stuff with a PR to remove roles and guns from the PEMs.

Having seen how people use Notary, and have trouble using Notary, one of the things I wanted to discuss at DockerCon was how much we try to farm out some of the data management. We're not doing any special with keys so would it be more obvious for users if we just made them manage their own keys? Key management right now is almost too magic... We'd still need integrations for things like Yubikeys, but maybe a user would provide CLI args saying "Use Slot 3", rather than all the automatic detection we do right now.

ecordell · 2017-04-07T17:57:39Z

@endophage this looks good to me if there's an extra test or two coming

endophage · 2017-04-07T20:22:22Z

@ecordell test added to cover the new functionality.

cyli · 2017-04-11T21:52:46Z

cmd/notary/keys.go

+		"storage (e.g. a Yubikey) is available, the key will be stored both " +
+		"on hardware and on disk (so that it can be backed up).  Please make " +
+		"sure to back up and then remove this on-key disk immediately" +
+		"afterwards. If a `--output` file name is provided, two files will " +


Non-blocking grammar nit: Maybe something like: "If the --output flag is provided, two files will be written to disk relative to the current working directory (as opposed to in Notary's default on-disk key storage): the public key as <output>.pub and a private key as <output>.priv."?

If --output is absolute is won't be relative to the cwd :-P I didn't want to get into the intricacies and was relying on user general knowledge of CLI tools to make sensible assumptions about providing paths to a tool.

I think that still may be the term, though. If you give something a relative link, it's relative to X, but absolute is always just absolute. I totally understand why we may want to leave that part out, though.

But would it make sense to include the rest of the above? I just mainly wanted to directly state that the extensions will be appended to whatever output file name the user gives us.

I can rephrase to make that clearer

cyli · 2017-04-11T22:05:54Z

Since there are no restrictions on which roles can be used for generateRole, we can generate targets/timestamp/snapshot keys as well. For those, should we also accept a GUN, or do we basically not really care about GUNs for those for now?

Also, could we add one more test for generating a delegation key without passing --output? In particular, could we also include this in the yubikey tests to make sure that the generated key also gets added to the yubikey? I tried it manually, and it didn’t work for me, but I think my setup is completely broken :( (I think maybe setting up the gpg-agent has interfered with piv stuff for me)

cyli · 2017-04-11T22:16:50Z

cmd/notary/keys.go

+
+	// if we had an outfile set, we'll write 2 files with the given name, appending .pub and .priv for the
+	// public and private keys respectively
+	return generateKeyToFile(k.generateRole, algorithm, k.getRetriever(), k.outFile)


This seems to only write the keys to a file, but does not store the key in the yubikey, if it's available.

Oh is that what the key will not be stored in Notary's key storage means? Neither the key storage disk or the yubikey? Could we clarify that in the docstring for the command? I thought providing an outfile would back up the keys to a different location, but also write to the yubikey.

Yeah, it only outputs to the provided file and doesn't store it anywhere else. I'm trying to reduce the magic of the commands. It would still be possible to subsequently notary key import to load the key in to a yubikey.

At some point, not in this PR, I wonder if it'd make sense to just generate the key directly in the yubikey if available unless --output is provided (no backups - the output can be your backup, since we've had issues where folks would prefer not having the key anywhere else).

endophage · 2017-04-11T23:15:55Z

@cyli it came up in the PCKS#8 PR, I think GUNs in the key PEMs are going to be deprecated. Especially as keys are used across repos it become less relevant.

cyli · 2017-04-12T02:33:13Z

I think GUNs in the key PEMs are going to be deprecated. Especially as keys are used across repos it become less relevant.

That makes sense, thanks

endophage · 2017-05-01T23:33:05Z

@ecordell @riyazdf @cyli was there anything else I can do here? (will rebase it now)

cyli · 2017-05-02T17:00:23Z

@endophage Are we expecting that this PR allows non-root keys to be imported into the yubikey?

endophage · 2017-05-02T17:01:12Z

I wasn't planning on doing that here.

cyli · 2017-05-02T17:03:43Z

@endophage Er sorry, not import, but that the generated non-root keys will be stored in the yubikey? I think if so, we may need to remove https://github.com/docker/notary/blob/master/trustmanager/yubikey/yubikeystore.go#L704, because currently generated keys that aren't root won't get stored in the yubikey.

If not, and the plan is to do that in another PR, could we please update the description and maybe the CLI description for key generation to say that for easier change tracking?

endophage · 2017-05-02T22:06:39Z

Same answer, wasn't planning to make any changes to the yubikey logic in this PR.

cyli · 2017-05-03T01:27:22Z

@endophage This LGTM with the caveat of updating the CLI docstring to make it clear that only root keys will be generated and put into the yubikey for now. I know the next one is probably coming soon, but if it gets delayed or if we have a release between this one and when that one comes, it may be confusing for users who may expect otherwise.

riyazdf · 2017-05-03T02:40:44Z

cmd/notary/keys.go

-	Long:  "Generates a new root key with a given algorithm. If hardware key storage (e.g. a Yubikey) is available, the key will be stored both on hardware and on disk (so that it can be backed up).  Please make sure to back up and then remove this on-key disk immediately afterwards.",
+	Short: "Generates a new key with a given algorithm.",
+	Long: "Generates a new key with a given algorithm. If hardware key " +
+		"storage (e.g. a Yubikey) is available, the key will be stored both " +


Proposed amendment:

Generates a new key with a given algorithm. If the key is a root key and hardware key storage (e.g. a Yubikey) is available...

riyazdf · 2017-05-03T02:46:23Z

cmd/notary/keys.go

+
+		pubKey, err := cs.Create(data.RoleName(k.generateRole), "", algorithm)
+		if err != nil {
+			return fmt.Errorf("Failed to create a new root key: %v", err)


s/"root key"/k.generateRole

riyazdf · 2017-05-03T02:46:45Z

cmd/notary/keys.go

+		}
+		cs := cryptoservice.NewCryptoService(ks...)
+
+		pubKey, err := cs.Create(data.RoleName(k.generateRole), "", algorithm)


is empty GUN intended? I noticed a comment about GUNs in PEMs being potentially deprecated but lost track if it was referring to this.

This would be necessary for the "templating", right?

Perhaps this PR should remove the GUN checks when parsing PEM files, too?

I think but I'm not certain we have those checks in a few scattered places. With the PRs coming in for providing certs and keys on the CLI, I'd rather hold off on making those changes here to keep potential conflicts down.

riyazdf · 2017-05-03T02:49:00Z

cmd/notary/keys.go

+		return errors.New("no password provided")
+	}
+
+	privFile := strings.Join([]string{outFile, "priv"}, ".")


I'm wondering if we should use .key instead, so that manually moving the key and importing should just work? .priv is conceptually fine with me, just that I'd like to add a notary key import test for it if we want to keep it that way.

the cfssl defaults are:

private: <name>-key.pem
public: <name>.pem

which seems reasonable to me, though I don't have a strong preference

I'm happy to go with the cfssl filenames for consistency.

ecordell · 2017-05-09T11:44:41Z

To clarify my approval: LGTM after @riyazdf's comments are addressed

Excited to see this in! This is a solid UX improvement.

riyazdf · 2017-05-15T20:46:05Z

cmd/notary/keys.go

+		return nil
+	}
+
+	// if we had an outfile set, we'll write 2 files with the given name, appending .pub and .priv for the


nit: this should be .pem and -key.pem

riyazdf · 2017-05-15T20:46:59Z

cmd/notary/keys_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/docker/notary/trustpinning"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/utils"
+	"path/filepath"


nit: can we put this import into the first block?

riyazdf · 2017-05-15T20:48:56Z

cmd/notary/keys_test.go

+	require.NoError(t, err)
+	assertNumKeys(t, tempDir, 0, 1, false) // key shouldn't be written to store and won't show up in keylist
+
+	// test that we can read the keys we created


could we additionally test a key rotate and/or key import to check that the notary CLI can ingest the key directly?

Added a test for import.

riyazdf · 2017-05-25T02:40:12Z

jenkins, test this please

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

riyazdf · 2017-07-11T03:28:11Z

@rubenmachuca FYI - you might have posted some personal information on a comment you left here. I've deleted the original comment, please reach out (my email is in my signoffs) if you have additional questions

endophage force-pushed the generate_delegation_keys branch 3 times, most recently from 4b2a686 to 4070365 Compare April 7, 2017 07:27

ecordell suggested changes Apr 7, 2017

View reviewed changes

endophage force-pushed the generate_delegation_keys branch 2 times, most recently from 51c1203 to a18a003 Compare April 7, 2017 17:39

endophage force-pushed the generate_delegation_keys branch 2 times, most recently from 23acee6 to 78d6438 Compare April 7, 2017 18:36

cyli reviewed Apr 11, 2017

View reviewed changes

endophage force-pushed the generate_delegation_keys branch from 78d6438 to 911ffa0 Compare April 11, 2017 23:03

endophage force-pushed the generate_delegation_keys branch from 911ffa0 to 296c3bd Compare May 1, 2017 23:38

riyazdf reviewed May 3, 2017

View reviewed changes

ecordell approved these changes May 4, 2017

View reviewed changes

endophage force-pushed the generate_delegation_keys branch 3 times, most recently from c7a45bb to b97f9ee Compare May 15, 2017 20:28

riyazdf reviewed May 15, 2017

View reviewed changes

David Lawrence added 2 commits May 30, 2017 11:03

generate any key and output it to a pair of .pub and .priv files

e2c0ea5

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

updating from PR review

ade502a

Signed-off-by: David Lawrence <[email protected]> (github: endophage)

endophage force-pushed the generate_delegation_keys branch from b97f9ee to ade502a Compare May 30, 2017 18:06

endophage merged commit 18ed13f into notaryproject:master May 30, 2017

endophage deleted the generate_delegation_keys branch May 30, 2017 21:56

notary key generate shouldn't be restricted to root keys #1134

notary key generate shouldn't be restricted to root keys #1134

Conversation

endophage commented Apr 7, 2017

ecordell left a 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

ecordell commented Apr 7, 2017

endophage commented Apr 7, 2017

cyli Apr 11, 2017 • edited Loading

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

cyli commented Apr 11, 2017 • edited Loading

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

endophage commented Apr 11, 2017 • edited Loading

cyli commented Apr 12, 2017

endophage commented May 1, 2017

cyli commented May 2, 2017

endophage commented May 2, 2017

cyli commented May 2, 2017 • edited Loading

endophage commented May 2, 2017

cyli commented May 3, 2017

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

ecordell commented May 9, 2017

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

riyazdf commented May 25, 2017

riyazdf commented Jul 11, 2017

`notary key generate` shouldn't be restricted to root keys #1134

`notary key generate` shouldn't be restricted to root keys #1134

cyli Apr 11, 2017 •

edited

Loading

cyli commented Apr 11, 2017 •

edited

Loading

endophage commented Apr 11, 2017 •

edited

Loading

cyli commented May 2, 2017 •

edited

Loading