Support passing in root keys on repo initialization #801

ecordell · 2016-06-23T20:21:08Z

This addresses #731 by adding a --rootkey flag to init.

Example:

$ notary init my/collection --rootkey=keys/root.key

resulting root.json:

{
    "signed": {
        "_type": "Root",
        "consistent_snapshot": false,
        "expires": "2026-06-21T14:20:03.15135742-04:00",
        "keys": {
            "956fbe52f38738cb5c31b1cbff06842ceda8e429f55164a61349cd233a50813f": {
                "keytype": "ecdsa-x509",
                "keyval": {
                    "private": null,
                    "public": "..."
                }
            }
            // ...
        },
        "roles": {
            "root": {
                "keyids": [
                    "956fbe52f38738cb5c31b1cbff06842ceda8e429f55164a61349cd233a50813f"
                ],
                "threshold": 1
            }
            // ...
        },
        "version": 1
    },
    "signatures": [{
        "keyid": "956fbe52f38738cb5c31b1cbff06842ceda8e429f55164a61349cd233a50813f",
        "method": "ecdsa",
        "sig": "70BvEbnWA1bHoc2IjnIE8XFI1N4I1WQxyd2GDNX/Avfmy89Z5J1w0pL507LpLd1DbV6p6CtqPgCdb5oCMeXt5w=="
    }
    }]
}

Additionally, the underlying TUF apis are updated to allow multiple root keys and certs, but that is not currently exposed to the CLI.

docker-jenkins · 2016-06-23T20:21:10Z

Can one of the admins verify this patch?

riyazdf · 2016-06-23T21:01:52Z

jenkins, test this please

riyazdf · 2016-06-23T22:06:06Z

thank you @ecordell for this awesome work!

I had some design concerns about how this might work with a --rootcert flag; I think it would be nice to optionally allow users to provide their own certificate for each root key they use. This would address the other part of #731, which is critical for pinning to a CA

Maybe we could only allow for up to 1 key on init:
notary init <GUN> --rootkey root.key - uses the provided private key and generates a self-signed cert for the root.json
notary init <GUN> --rootkey root.key --rootcert cert.pem - uses the provided private key and certificate (provided that they form a valid keypair) for the root.json

And if we wanted to add more keys:
notary key add <GUN> root root.key [--rootcert cert.pem] - same behavior as above with certs, either using the one if provided (and valid), otherwise generating one
ping @endophage for his thoughts

What do you think?

ecordell · 2016-06-24T18:01:16Z

Thanks for taking a look @riyazdf !

To limit the scope, I've simply rebased on master and changed rootkeys to rootkey to support only a single key being passed in. Adding additional root keys and supporting certs other than self-signed, generated certs can come in a future PR.

Looks like circle is failing because of temporary connection issue; I don't have the ability to restart it afaict.

riyazdf · 2016-06-24T19:43:17Z

jenkins, test this please

endophage · 2016-06-24T19:43:22Z

I'll review this fully next week but in the meantime, thank you! This is a feature we've had quite a few people asking for. I largely agree with @riyazdf's suggestions, though it might be nice to find a way to cleanly combine the key and cert so that if we do decide to allow multiple keys/certs to be added on init in the future there's a nice way to do it.

Also worth noting there might be a use case to add multiple certs but fewer keys (i.e. you want other keys to be able to sign root but you personally only hold one of them. This is a specific use case we discussed with one user at DockerCon).

riyazdf · 2016-06-27T17:14:10Z

cmd/notary/tuf.go

@@ -285,7 +320,7 @@ func (t *tufCommander) tufInit(cmd *cobra.Command, args []string) error {
 		cmd.Printf("Root key found, using: %s\n", rootKeyID)
 	}

-	if err = nRepo.Initialize(rootKeyID); err != nil {
+	if err = nRepo.Initialize([]string{rootKeyID}); err != nil {


If other root keys already exist on this host, this rootKeyID may not necessarily correspond to the --rootkey we pass in. We should change the conditional logic above such that it prioritizes keys from the --rootkey flag if we passed any in

Edit: this is resolved now in above logic

ecordell · 2016-07-05T19:13:33Z

@riyazdf Fixed the issues you noted and rebased.

riyazdf · 2016-07-06T21:21:41Z

jenkins, test this please

riyazdf · 2016-07-06T21:31:08Z

client/client_test.go

+	rootPubKey2, err := repo.CryptoService.Create("root", repo.gun, data.ECDSAKey)
+	require.NoError(t, err, "error generating second root key: %s", err)
+
+	err = repo.Initialize([]string{rootPubKeyID, rootPubKey2.ID()}, data.CanonicalTimestampRole)


could we verify that both keys were included in the root metadata?

Something like:
require.Len(t, repo.tufRepo.Root.Signed.Roles[data.CanonicalRootRole].KeyIDs, 2)

Definitely, will update

riyazdf · 2016-07-07T00:36:07Z

thank you @ecordell for working on this! I have a couple of small nits and test case additions, but I really like how this looks, cc @cyli @endophage for additional review.

I'll also link #813 for tracking, since it is somewhat coupled with the discussion above

Also, if you could update the initial PR description to reflect the updated behavior (just single root key) that would be awesome :)

cyli · 2016-07-07T16:48:47Z

@riyazdf @endophage maybe --rootkey path-to-privkey:path-to-rootcert?

cyli · 2016-07-07T17:06:49Z

@ecordell This is awesome - thanks so much for adding this! Being able to import existing root keys and certs will become really important for our use cases in the future. It looks fantastic aside from the test nitpicks above!

riyazdf · 2016-07-07T17:46:22Z

@cyli: @endophage and I just discussed that it might be useful to allow for multiple --rootcert and multiple --rootkey flags on initialization, potentially allowing for initializing with extra certs that you may not have the keys for.

The use case would be to add other admins to be root-signers, but without having to share private key material directly. Ex: notary init --rootkey alice.key --rootcert alice.crt --rootcert bob.crt --rootcert eve.crt

cyli · 2016-07-07T17:55:07Z

That makes sense - and I guess certs and keys don't really need to be
associated?

On Thursday, July 7, 2016, Riyaz Faizullabhoy [email protected]
wrote:

@cyli https://github.com/cyli: @endophage https://github.com/endophage
and I just discussed that it might be useful to allow for multiple
--rootcert and multiple --rootkey flags on initialization, potentially
allowing for initializing with extra certs that you may not have the keys
for.

The use case would be to add other admins to be root-signers, but without
having to share private key material directly. Ex: notary init --rootkey
alice.key --rootcert alice.crt --rootcert bob.crt --rootcert eve.crt

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#801 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AANn4M5eLrFylY6rH6G8GWpnIA56EJ6fks5qTTtvgaJpZM4I9M0o
.

ecordell · 2016-07-07T18:33:03Z

Thanks @riyazdf and @cyli for taking the time to review!

I agree that passing multiple certs and root keys (and not needing to correlate them) makes sense. I'm happy to work on the implementation but I think it makes more sense to do that in a separate PR.

Just updated with the last couple of points w.r.t. the tests; just let me know if it's important that cert loading be in this PR as well or if it can wait for a separate one.

ecordell · 2016-07-08T19:32:29Z

@riyazdf @cyli I think I've addressed all of your comments at this point, please let me know if there's anything else I should do.

cyli · 2016-07-08T19:37:17Z

jenkins, test this please

dnwake · 2016-07-08T19:40:49Z

Hi everyone! Sorry to make things more complicated, but I have created new pull request at #821 based on top of a (probably slightly out-of-date) version of this commit. It adds the --rootcert flag to allow a root cert and root key to be imported together.

I'm very willing to work with everyone here to get all the functionality approved asap -- please let me know the best way to co-ordinate things. Cheers.

cyli · 2016-07-08T19:53:08Z

cmd/notary/integration_test.go

+	err = ioutil.WriteFile(badKeyFilename, nonPEMKey, 0644)
+	require.NoError(t, err)
+
+	_, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2", "--rootKey", badKeyFilename)


I think the flag for these next few test cases should have --rootkey as opposed to --rootKey?

cyli · 2016-07-08T19:54:31Z

Thanks so much for all your work and your responsiveness on this @ecordell! Aside from one last nitpick about the --rootkey flag in tests, this LGTM!

riyazdf · 2016-07-08T20:03:05Z

@ecordell thank you for updating!

LGTM after resolving @cyli's comments

@dnwake: thanks for checking in -- I think we'll probably want to merge this PR first before #821 but I'll take a look and review in the meantime. We can rebase on top of this once merged :)

riyazdf · 2016-07-08T20:22:27Z

cmd/notary/tuf.go

+			return err
+		}
+
+		privKey, _, err := trustmanager.GetPasswdDecryptBytes(t.retriever, pemBytes, "", "imported root")


something I just noticed from #821 - it might make sense to just change the last arg here to data.CanonicalRootRole, so that NOTARY_ROOT_PASSPHRASE can be used to automate this

Signed-off-by: Evan Cordell <[email protected]>

ecordell · 2016-07-08T23:05:01Z

Updated and rebased!

cyli · 2016-07-08T23:09:09Z

jenkins, test this please

riyazdf · 2016-07-08T23:58:54Z

LGTM after changes, thank you again @ecordell for contributing this!

endophage · 2016-07-11T22:00:50Z

client/client.go

-	if err != nil {
-		return err
+func (r *NotaryRepository) Initialize(rootKeyIDs []string, serverManagedRoles ...string) error {
+	privKeys := []data.PrivateKey{}


In all likelihood privKeys will have the same length as rootKeyIDs so we should initialize appropriately: privKeys := make([]data.PrivateKey, 0, len(rootKeyIDs))

I'm not so sure - I think you should be able to initialize with one private key and multiple public keys, so that these two don't match up.

That's not implemented in this PR though. Although @dnwake's PR for --rootcert may make my comment moot.

endophage · 2016-07-11T22:13:49Z

Left some comments on minor stuff but overall good to merge this now. LGTM and thank you!

GordonTheTurtle added status/0-triage labels Jun 23, 2016

ecordell force-pushed the root-keys-on-init branch from 540cdd4 to cea9e8d Compare June 23, 2016 20:36

ecordell force-pushed the root-keys-on-init branch from cea9e8d to d2e7af3 Compare June 24, 2016 17:59

riyazdf modified the milestone: Notary 0.4 Jun 24, 2016

riyazdf reviewed Jun 27, 2016
View reviewed changes

ecordell force-pushed the root-keys-on-init branch 2 times, most recently from 18d2ad4 to 8f3dec0 Compare July 5, 2016 19:13

riyazdf reviewed Jul 6, 2016
View reviewed changes

ecordell force-pushed the root-keys-on-init branch 2 times, most recently from 3b1316c to 62ade6b Compare July 7, 2016 19:59

dnwake mentioned this pull request Jul 8, 2016

Import key and cert #821

Closed

ecordell force-pushed the root-keys-on-init branch from 62ade6b to a2426a7 Compare July 8, 2016 18:54

cyli reviewed Jul 8, 2016
View reviewed changes

riyazdf reviewed Jul 8, 2016
View reviewed changes

Support passing in root keys on repo initialization

96daed2

Signed-off-by: Evan Cordell <[email protected]>

ecordell force-pushed the root-keys-on-init branch from a2426a7 to 96daed2 Compare July 8, 2016 23:04

endophage reviewed Jul 11, 2016
View reviewed changes

endophage merged commit fd2ea6d into notaryproject:master Jul 11, 2016

riyazdf mentioned this pull request Jul 11, 2016

rootkey init code cleanups #829

Merged

ecordell deleted the root-keys-on-init branch July 12, 2016 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support passing in root keys on repo initialization #801

Support passing in root keys on repo initialization #801

ecordell commented Jun 23, 2016 •

edited

Loading

docker-jenkins commented Jun 23, 2016

riyazdf commented Jun 23, 2016

riyazdf commented Jun 23, 2016 •

edited

Loading

ecordell commented Jun 24, 2016 •

edited

Loading

riyazdf commented Jun 24, 2016

endophage commented Jun 24, 2016 •

edited

Loading

riyazdf Jun 27, 2016 •

edited

Loading

ecordell commented Jul 5, 2016

riyazdf commented Jul 6, 2016

riyazdf Jul 6, 2016 •

edited

Loading

ecordell Jul 7, 2016

riyazdf commented Jul 7, 2016 •

edited

Loading

cyli commented Jul 7, 2016

cyli commented Jul 7, 2016

riyazdf commented Jul 7, 2016

cyli commented Jul 7, 2016

ecordell commented Jul 7, 2016 •

edited

Loading

ecordell commented Jul 8, 2016

cyli commented Jul 8, 2016

dnwake commented Jul 8, 2016 •

edited

Loading

cyli Jul 8, 2016

cyli commented Jul 8, 2016

riyazdf commented Jul 8, 2016

riyazdf Jul 8, 2016

ecordell commented Jul 8, 2016

cyli commented Jul 8, 2016

riyazdf commented Jul 8, 2016

endophage Jul 11, 2016

ecordell Jul 12, 2016

endophage Jul 12, 2016

endophage commented Jul 11, 2016

Support passing in root keys on repo initialization #801

Support passing in root keys on repo initialization #801

Conversation

ecordell commented Jun 23, 2016 • edited Loading

docker-jenkins commented Jun 23, 2016

riyazdf commented Jun 23, 2016

riyazdf commented Jun 23, 2016 • edited Loading

ecordell commented Jun 24, 2016 • edited Loading

riyazdf commented Jun 24, 2016

endophage commented Jun 24, 2016 • edited Loading

riyazdf Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

ecordell commented Jul 5, 2016

riyazdf commented Jul 6, 2016

riyazdf Jul 6, 2016 • edited Loading

Choose a reason for hiding this comment

ecordell Jul 7, 2016

Choose a reason for hiding this comment

riyazdf commented Jul 7, 2016 • edited Loading

cyli commented Jul 7, 2016

cyli commented Jul 7, 2016

riyazdf commented Jul 7, 2016

cyli commented Jul 7, 2016

ecordell commented Jul 7, 2016 • edited Loading

ecordell commented Jul 8, 2016

cyli commented Jul 8, 2016

dnwake commented Jul 8, 2016 • edited Loading

cyli Jul 8, 2016

Choose a reason for hiding this comment

cyli commented Jul 8, 2016

riyazdf commented Jul 8, 2016

riyazdf Jul 8, 2016

Choose a reason for hiding this comment

ecordell commented Jul 8, 2016

cyli commented Jul 8, 2016

riyazdf commented Jul 8, 2016

endophage Jul 11, 2016

Choose a reason for hiding this comment

ecordell Jul 12, 2016

Choose a reason for hiding this comment

endophage Jul 12, 2016

Choose a reason for hiding this comment

endophage commented Jul 11, 2016

ecordell commented Jun 23, 2016 •

edited

Loading

riyazdf commented Jun 23, 2016 •

edited

Loading

ecordell commented Jun 24, 2016 •

edited

Loading

endophage commented Jun 24, 2016 •

edited

Loading

riyazdf Jun 27, 2016 •

edited

Loading

riyazdf Jul 6, 2016 •

edited

Loading

riyazdf commented Jul 7, 2016 •

edited

Loading

ecordell commented Jul 7, 2016 •

edited

Loading

dnwake commented Jul 8, 2016 •

edited

Loading