Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support passing in root keys on repo initialization #801

Merged
merged 1 commit into from
Jul 11, 2016

Conversation

ecordell
Copy link
Contributor

@ecordell ecordell commented Jun 23, 2016

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
Copy link

Can one of the admins verify this patch?

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

riyazdf commented Jun 23, 2016

jenkins, test this please

@riyazdf
Copy link
Contributor

riyazdf commented Jun 23, 2016

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 ecordell force-pushed the root-keys-on-init branch from cea9e8d to d2e7af3 Compare June 24, 2016 17:59
@ecordell
Copy link
Contributor Author

ecordell commented Jun 24, 2016

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 riyazdf modified the milestone: Notary 0.4 Jun 24, 2016
@riyazdf
Copy link
Contributor

riyazdf commented Jun 24, 2016

jenkins, test this please

@endophage
Copy link
Contributor

endophage commented Jun 24, 2016

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).

@@ -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 {
Copy link
Contributor

@riyazdf riyazdf Jun 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ecordell force-pushed the root-keys-on-init branch 2 times, most recently from 18d2ad4 to 8f3dec0 Compare July 5, 2016 19:13
@ecordell
Copy link
Contributor Author

ecordell commented Jul 5, 2016

@riyazdf Fixed the issues you noted and rebased.

@riyazdf
Copy link
Contributor

riyazdf commented Jul 6, 2016

jenkins, test this please

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)
Copy link
Contributor

@riyazdf riyazdf Jul 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely, will update

@riyazdf
Copy link
Contributor

riyazdf commented Jul 7, 2016

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
Copy link
Contributor

cyli commented Jul 7, 2016

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

@cyli
Copy link
Contributor

cyli commented Jul 7, 2016

@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
Copy link
Contributor

riyazdf commented Jul 7, 2016

@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
Copy link
Contributor

cyli commented Jul 7, 2016

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
Copy link
Contributor Author

ecordell commented Jul 7, 2016

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 ecordell force-pushed the root-keys-on-init branch 2 times, most recently from 3b1316c to 62ade6b Compare July 7, 2016 19:59
@dnwake dnwake mentioned this pull request Jul 8, 2016
@ecordell ecordell force-pushed the root-keys-on-init branch from 62ade6b to a2426a7 Compare July 8, 2016 18:54
@ecordell
Copy link
Contributor Author

ecordell commented Jul 8, 2016

@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
Copy link
Contributor

cyli commented Jul 8, 2016

jenkins, test this please

@dnwake
Copy link

dnwake commented Jul 8, 2016

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.

err = ioutil.WriteFile(badKeyFilename, nonPEMKey, 0644)
require.NoError(t, err)

_, err = runCommand(t, tempDir, "-s", server.URL, "init", "gun2", "--rootKey", badKeyFilename)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cyli
Copy link
Contributor

cyli commented Jul 8, 2016

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
Copy link
Contributor

riyazdf commented Jul 8, 2016

@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 :)

return err
}

privKey, _, err := trustmanager.GetPasswdDecryptBytes(t.retriever, pemBytes, "", "imported root")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

ecordell commented Jul 8, 2016

Updated and rebased!

@cyli
Copy link
Contributor

cyli commented Jul 8, 2016

jenkins, test this please

@riyazdf
Copy link
Contributor

riyazdf commented Jul 8, 2016

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@endophage
Copy link
Contributor

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

@endophage endophage merged commit fd2ea6d into notaryproject:master Jul 11, 2016
@ecordell ecordell deleted the root-keys-on-init branch July 12, 2016 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants