Support for initializing repository with root certificate #1144

cyc115 · 2017-05-01T22:03:45Z

Supports initializing a notary repository by passing in a root certificate alone with the corresponding root key. This would make wildcard certificate trust pinning usable as an existing trusted certificate can now be included in root.json metadata.

usage : notary init [ GUN ] --rootkey path/to/key.key --rootcert path/to/cert.crt

--rootcert can only be used when --rootkey is specified.

related works include #821 and #883.

docker-jenkins · 2017-05-01T22:03:47Z

Can one of the admins verify this patch?

GordonTheTurtle · 2017-05-01T22:03:47Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "cyc_rootcert" git@github.com:cyc115/notary.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354469616
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

cyli · 2017-05-02T19:01:03Z

jenkins, test this please

endophage · 2017-05-04T21:33:07Z

I think it probably makes sense to allow --rootcert to be specified without a --rootkey. The use case is that the root private key may be in an HSM/Yubikey. You would still be able to generate a CSR and go get a certificate from your CA, but you wouldn't be able to provide a file path to the private key on the CLI.

The behavior in the case that only a --rootcert is provided would be to use it to look up the root key in any and all available key storage.

lewiada-zz · 2017-05-04T22:41:57Z

@endophage - even if it were on a Yubikey/HSM, it seems one might still need to pass at least a KeyID, if not the path to the cert. Or are you thinking that it would follow the current algorithm and take the first root key found?

Also are you proposing that this be added to this PR, or would you be okay with it being done as part of a separate PR?

endophage · 2017-05-05T00:32:01Z

The KeyID can be generated from the certificate. KeyIDs are the SHA256 checksum of the following canonically serialized JSON object:

{
    "type": "<key type i.e. rsa-x509>"
    "value": {
        "public": "<public bytes of key, i.e. the x509 cert>",
        "private": ""
    }
}

More recent versions of the TUF spec have been updated to simply exclude the empty private field. Specifically for certificates, we also implemented an internal concept of a "Canonical" KeyID. This is generated the same way but rather than using the x509 certificate, it uses only the public key bytes extracted from the certificate.

cyc115 · 2017-05-05T21:35:27Z

@endophage great advice David! I have updated the PR to allow init with --rootcert and lookup key from existing key store. I will update key rotation PR to allow the same thing. Let me know if you have any more suggestions !

cyli · 2017-05-10T20:22:49Z

jenkins, test this please

riyazdf

thank you @cyc115 for the contribution! I've done a first pass and have the following comments.

I'd also like to figure out if there's a way for us to clean up and consolidate matchKeyIdsWithCerts and keyExistsInList though I haven't come to a concrete suggestion yet

riyazdf · 2017-05-03T02:54:07Z

client/client.go

@@ -14,6 +14,7 @@ import (

 	"github.com/Sirupsen/logrus"
 	"github.com/docker/notary"
+


nit whitespace

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

client/client.go

-	if err != nil {
-		return err
-	}
+// repoInitialize initializes the notary repository wit a set of rootkeys, root certificates and roles.


typo: wit

riyazdf · 2017-05-12T19:06:09Z

cmd/notary/tuf_test.go

 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"os"
 	"path/filepath"
 	"testing"

+	"strings"


nit: can this go into the above block?

riyazdf · 2017-05-12T19:10:11Z

tuf/signed/verify.go

+
+// VerifyPublicKeyMatchesPrivateKey checks if the private key and the public keys forms valid key pairs.
+// This should work with both ecdsa-x509 certificate PublicKey as well as ecdsa PublicKey
+func VerifyPublicKeyMatchesPrivateKey(privKey data.PrivateKey, pubKey data.PublicKey) error {


I'm wondering if we can do something simpler here - I think I've previously discussed this with @lewiada or @dnwake but didn't get to a final solution. Perhaps we could check the private key's fields against those shared with data.PublicKey directly?

Yes I agree! I have come across utils.CanonicalKeyID() and thought maybe that will be a good way of comparing key pair since it will extract the key id of the public key embedded in the certificate which should match privateKey.ID().

this looks great now, thanks! 👍

riyazdf · 2017-05-12T19:10:26Z

client/client.go

@@ -15,6 +15,7 @@ import (
 	"github.com/Sirupsen/logrus"
 	canonicaljson "github.com/docker/go/canonical/json"
 	"github.com/docker/notary"
+


nit whitespace

riyazdf · 2017-05-12T19:10:48Z

client/client.go

@@ -256,6 +246,116 @@ func (r *NotaryRepository) Initialize(rootKeyIDs []string, serverManagedRoles ..
 	return r.saveMetadata(serverManagesSnapshot)
 }

+// certsOfKeyIDs either confirms that the certs and keys (represented by Key IDs) forms valid key pairs.
+// Or throw error when they missmatch.


typo: mismatch

riyazdf · 2017-05-12T19:11:55Z

client/client.go

@@ -256,6 +246,116 @@ func (r *NotaryRepository) Initialize(rootKeyIDs []string, serverManagedRoles ..
 	return r.saveMetadata(serverManagesSnapshot)
 }

+// certsOfKeyIDs either confirms that the certs and keys (represented by Key IDs) forms valid key pairs.


Can we document that the lists are strictly ordered in pairs? Ex: keyIDs[0] and certs[0] must match?

riyazdf · 2017-05-12T20:20:24Z

client/client.go

@@ -168,24 +170,14 @@ func rootCertKey(gun data.GUN, privKey data.PrivateKey) (data.PublicKey, error)

 	x509PublicKey := utils.CertToKey(cert)
 	if x509PublicKey == nil {
-		return nil, fmt.Errorf(
-			"cannot use regenerated certificate: format %s", cert.PublicKeyAlgorithm)
+		return nil, fmt.Errorf("cannot use regenerated certificate: format %d", cert.PublicKeyAlgorithm)


nice catch on the type, though I'm wondering if we should instead print the privKey.Algorithm() string since that will be more descriptive?

Agreed. How is
return nil, fmt.Errorf("cannot generate public key from private key with id: %v. %v is not a supported type", privKey.ID(), privKey.Algorithm())

that would be great! I'd simplify slightly to something more generic so that it isn't tightly bound to the algorithm being the error returned from CertToKey:

return nil, fmt.Errorf("cannot generate public key from private key with id: %v and algorithm: %v", privKey.ID(), privKey.Algorithm())

riyazdf · 2017-05-12T20:26:11Z

client/client.go

+			return id, nil
+		}
+	}
+	return "", nil


should this just return an error? You could use a typed error or fmt.Errorf(...) so that error is differentiated from the other error case

riyazdf · 2017-05-12T20:27:00Z

cmd/notary/tuf_test.go

 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"os"
 	"path/filepath"
 	"testing"

+	"strings"


nit: same here

cyc115 · 2017-05-18T20:47:40Z

@cyli Ying the pipe line seems to be blocked again :P ?

endophage · 2017-05-19T23:34:47Z

@cyc115 yeah, it's at the point we need to plug in a keyboard and monitor to fix it :-( We're at PyCon right now but somebody will look at it next week.

riyazdf · 2017-05-24T22:19:50Z

@cyc115: we're diagnosing the CI issue still, in the meantime could you please squash your commits?

edit: should be fixed now!

riyazdf · 2017-05-25T02:39:45Z

jenkins, test this please

riyazdf

this is really close @cyc115 - thank you for the hard work! I have some fix comments and it would be great if you could rebase and squash commits as well.

riyazdf · 2017-05-25T04:29:07Z

client/client.go

@@ -256,6 +251,111 @@ func (r *NotaryRepository) Initialize(rootKeyIDs []string, serverManagedRoles ..
 	return r.saveMetadata(serverManagesSnapshot)
 }

+// createNewPublicKeyFromKeyIDs generate a set of public keys corresponding to the given list of


nit typo: generates

riyazdf · 2017-05-25T04:30:26Z

client/client.go

+	return errKeyNotFound{}
+}
+
+// InitializeWithCertificate initializes the repository with root key and their corresponding certificates


nit: s/root key/specified root keys

riyazdf · 2017-05-25T04:32:43Z

client/client_test.go

+		args    args
+		wantErr bool
+	}{
+	// TODO: Add test cases.


which test cases are you planning on placing in here?

@riyazdf good catch! I think it was generated by accident and checked into the code. I have gone over existing tests and they are not needed. :) Thanks!

riyazdf · 2017-05-25T04:32:51Z

client/client_test.go

+		wantTimestamp data.BaseRole
+		wantErr       bool
+	}{
+	// TODO: Add test cases.


same comment: which test cases are you planning on placing in here?

riyazdf · 2017-05-25T04:33:13Z

cmd/notary/integration_test.go

+		"init", gun+"4",
+		"--rootkey", nonMatchingKeyFilename,
+		"--rootcert", certFilename)
+	require.Error(t, err, "should not be able to init a repository with missmatched key and cert")


nit typo: mismatched

riyazdf · 2017-05-25T04:33:58Z

cross.Dockerfile

@@ -1,4 +1,4 @@
-FROM golang:1.7.3
+FROM golang:1.8.1


could you please rebase? This should not be in this PR.

riyazdf · 2017-05-25T04:34:11Z

tuf/signed/sign_test.go

@@ -208,7 +208,7 @@ func TestSignReturnsNoSigs(t *testing.T) {
 }

 func TestSignWithX509(t *testing.T) {
-	// generate a key becase we need a cert
+	// generate a key because we need a cert


thanks for the typo fix! 👍

riyazdf · 2017-05-25T04:34:32Z

tuf/signed/verify.go

+
+// VerifyPublicKeyMatchesPrivateKey checks if the private key and the public keys forms valid key pairs.
+// This should work with both ecdsa-x509 certificate PublicKey as well as ecdsa PublicKey
+func VerifyPublicKeyMatchesPrivateKey(privKey data.PrivateKey, pubKey data.PublicKey) error {


this looks great now, thanks! 👍

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

Clean up, update tests, code refactoring and various fixes. Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

…other minor changes. Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

riyazdf

LGTM after nits, thank you @cyc115 :)

riyazdf · 2017-05-25T20:26:43Z

cmd/notary/integration_test.go

+	//set up temp dir
+	tempDir := tempDirWithConfig(t, `{
+		"trust_pinning" : {
+			"disable_tufu" : false


typo: "disable_tofu" - this shouldn't affect behavior since disable_tofu will disable to false anyhow, but would be nice to be explicit here.

Hey there @riyazdf ! Thanks for helping me to cache all those typos :P, I will make sure to have those cached in advance for subsequent PRs !

riyazdf · 2017-05-25T20:27:42Z

cmd/notary/tuf.go

@@ -133,6 +136,7 @@ type tufCommander struct {
 func (t *tufCommander) AddToCommand(cmd *cobra.Command) {
 	cmdTUFInit := cmdTUFInitTemplate.ToCommand(t.tufInit)
 	cmdTUFInit.Flags().StringVar(&t.rootKey, "rootkey", "", "Root key to initialize the repository with")
+	cmdTUFInit.Flags().StringVar(&t.rootCert, "rootcert", "", "Root certificate to initialize the repository with")


should we note that if you provide a rootcert that you must provide a matching root key?

ex: "Root certificate to initialize the repository with, must provide a matching root key"

It looks like a root cert could be provided without a root key, so long as that root cert corresponds to a private key that is in the existing keystore.

But if both a key and a cert are provided, they must match (e.g. the cert must correspond to the key provided). Agree with @riyazdf though that this would be good to include in the comment, since it's possible someone could interpret this as "add this new root key, and also use this cert which corresponds to a key already in the key store".

what do you guys think of : "root certificate must match root key if a root key is supplied, otherwise it must match a key present in keystore"

@cyc115 capitalize the first Root for consistency with other flags but otherwise SGTM!

cyli · 2017-05-25T21:13:09Z

tuf/signed/verify.go

@@ -107,3 +108,18 @@ func VerifySignature(msg []byte, sig *data.Signature, pk data.PublicKey) error {
 	sig.IsValid = true
 	return nil
 }
+
+// VerifyPublicKeyMatchesPrivateKey checks if the private key and the public keys forms valid key pairs.
+// This should work with both ecdsa-x509 certificate PublicKey as well as ecdsa PublicKey


Nitpick: Should this say just "x509 certificate PublicKeys as well as non-certificate PublicKeys"? (I think RSA keys are also supported, for instance)

cyli · 2017-05-25T21:19:15Z

tuf/signed/verify.go

+// This should work with both ecdsa-x509 certificate PublicKey as well as ecdsa PublicKey
+func VerifyPublicKeyMatchesPrivateKey(privKey data.PrivateKey, pubKey data.PublicKey) error {
+	pubKeyID, err := utils.CanonicalKeyID(pubKey)
+	privKeyID := privKey.ID()


Before we call privKey.ID(), do we want to check to make sure it's not nil? (maybe we can just do so in the if block below: if privKey == nil || pubKeyID != privKey.ID {...)

Likewise, in the tests (thanks for adding those!), particularly in TestVerifyPublicKeyMatchesPrivateKeyFails, could we also test passing a nil public key and/or nil private key?

Actually apologies it looks like utils.CanonicalKeyID also assumes that the key is not nil, and will also panic :| If you wouldn't mind fixing it in this PR, that would be awesome but I'd also be happy to fix it in a separate PR.

@cyli good catch, I missed this!

I agree that for this PR, at minimum we should check the error (if err != nil {) before any operations with the private key, and then check as @cyli suggests by doing if privKey == nil || pubKeyID != privKey.ID {...

Fixing utils.CanonicalKeyID would be a nice bonus if you're comfortable but agree that we can handle it in a separate PR 👍

Ok! I have just added a check in CanonicalKeyID to error if nil is provided, I am ok if you guys don't mind it to be in this PR.

That's awesome, thanks!

cyli

Apologies for coming into this so late. Thanks for working on this @cyc115!

This LGTM too, aside from a few minor nitpicks.

cyli · 2017-05-25T21:28:22Z

cmd/notary/tuf.go

@@ -446,6 +453,38 @@ func importRootKey(cmd *cobra.Command, rootKey string, nRepo *notaryclient.Notar
 	return []string{}, nil
 }

+// importRootCert imports the base64 encrypted public certificate corresponding to the root key


Nitpick: "encoded", not "encrypted", since certs aren't encrypted. :)

Good cache :)

cyli · 2017-05-25T21:29:04Z

cmd/notary/tuf.go

+	}
+	defer certFile.Close()
+
+	certPEM, err := ioutil.ReadAll(certFile)


Should we be checking this error and returning it if not nil? Also, if we use ioutil.ReadFile here, this read and the previous open section can be merged into a single function call.

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

cyli · 2017-05-30T17:49:17Z

Thanks for the fixes @cyc115! LGTM on yubikey green (the builder has a connection failure again - I'm prodding it every so often)

cyc115 · 2017-05-30T18:21:05Z

:) No problem @cyli ! Thank you for helping me through my first pull request to notary :P

endophage

LGTM!

endophage · 2017-05-30T22:05:53Z

I'll deal with any merge issues if anything unexpected comes up. GH wasn't highlighting any conflicts though and I don't think this conflicts with my key generation PR that I merged earlier

This will allow user to rotate a repository's root key to a pinned trust, make trust pinning more useful. - add `--rootcert` flag to key rotation - add `-y` flag to key rotate to allow auto-confirmation of rotating root keys (no user interaction required) - allow mismatched key-certificate pair to be provided. an example usage would be : The PR includes the following: `notary key rotate [GUN] root --key path/to/key.key --rootcert path/to/rootcert.pem` related issues: notaryproject#1144, notaryproject#1118, notaryproject#731 Signed-off-by: Chen Yuechuan-XJQW46 <Yuechuan.Chen@motorolasolutions.com>

GordonTheTurtle added the dco/no label May 1, 2017

cyc115 force-pushed the cyc_rootcert branch from 1a11b90 to 1d2e5ca Compare May 1, 2017 22:05

GordonTheTurtle added dco/no and removed dco/no labels May 1, 2017

cyc115 force-pushed the cyc_rootcert branch 3 times, most recently from 3b374bd to 364a531 Compare May 2, 2017 18:52

cyc115 mentioned this pull request May 4, 2017

adding --rootcert flag to key rotation #1147

Open

cyc115 force-pushed the cyc_rootcert branch 5 times, most recently from 238a381 to 5c1e88f Compare May 8, 2017 15:43

endophage mentioned this pull request May 9, 2017

Import key and cert #821

Closed

cyc115 force-pushed the cyc_rootcert branch 4 times, most recently from 78aee10 to 51d2943 Compare May 10, 2017 19:47

riyazdf reviewed May 12, 2017

View reviewed changes

GordonTheTurtle added the dco/no label May 12, 2017

cyc115 force-pushed the cyc_rootcert branch from 4dc08a1 to e7fd4e4 Compare May 12, 2017 21:59

GordonTheTurtle removed the dco/no label May 12, 2017

cyc115 force-pushed the cyc_rootcert branch from 0216549 to ab59674 Compare May 18, 2017 14:19

riyazdf reviewed May 25, 2017

View reviewed changes

cyc115 force-pushed the cyc_rootcert branch from ab59674 to 88710b5 Compare May 25, 2017 16:06

Yuechuan Chen added 7 commits May 25, 2017 11:08

Added various tests for --rootcert feature

40ef858

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

wip, implemented import root certificate

0f0fa25

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

Allow initialize repo with keys and certs

f821d8d

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

Passing in certificate via --rootcert and verify

3ddfd36

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

Support for importing matching keys for a given cert from keystore.

1ceb801

Clean up, update tests, code refactoring and various fixes. Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

fix broken key pair check

d1096b7

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

refactors according to review comments : tablefy tests, renaming and …

9344fb8

…other minor changes. Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

cyc115 force-pushed the cyc_rootcert branch from 88710b5 to 9344fb8 Compare May 25, 2017 16:09

riyazdf approved these changes May 25, 2017

View reviewed changes

cyli reviewed May 25, 2017

View reviewed changes

cyli approved these changes May 25, 2017

View reviewed changes

GordonTheTurtle added the dco/no label May 26, 2017

various changes

8cce5e5

Signed-off-by: Yuechuan Chen <yuechuan.chen@motorolasolutions.com>

cyc115 force-pushed the cyc_rootcert branch from 99fce5b to 8cce5e5 Compare May 30, 2017 14:35

GordonTheTurtle removed the dco/no label May 30, 2017

endophage approved these changes May 30, 2017

View reviewed changes

endophage merged commit 101275f into notaryproject:master May 30, 2017

		@@ -14,6 +14,7 @@ import (

		"github.com/Sirupsen/logrus"
		"github.com/docker/notary"

Support for initializing repository with root certificate #1144

Support for initializing repository with root certificate #1144

Conversation

cyc115 commented May 1, 2017

docker-jenkins commented May 1, 2017

GordonTheTurtle commented May 1, 2017

cyli commented May 2, 2017

endophage commented May 4, 2017

lewiada-zz commented May 4, 2017

endophage commented May 5, 2017

cyc115 commented May 5, 2017 • edited Loading

cyli commented May 10, 2017

riyazdf 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

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

cyc115 commented May 18, 2017

endophage commented May 19, 2017

riyazdf commented May 24, 2017 • edited Loading

riyazdf commented May 25, 2017

riyazdf 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

riyazdf 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

cyli May 25, 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 May 25, 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

Choose a reason for hiding this comment

cyli 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

cyli commented May 30, 2017

cyc115 commented May 30, 2017

endophage left a comment

Choose a reason for hiding this comment

endophage commented May 30, 2017

cyc115 commented May 5, 2017 •

edited

Loading

riyazdf commented May 24, 2017 •

edited

Loading

cyli May 25, 2017 •

edited

Loading

cyli May 25, 2017 •

edited

Loading