wildcarded trustpinning for cert ids #1126

endophage · 2017-03-30T17:20:58Z

Signed-off-by: David Lawrence david.lawrence@docker.com (github: endophage)

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage · 2017-03-31T00:10:51Z

@ecordell figure you'll be interested in this. It basically makes individual key pinning useful in light of your wildcarded root cert CNs PR we merged. It'll also be useful when we roll back the requirement for root keys to be certs in the first place.

endophage · 2017-03-31T00:11:26Z

@riyazdf you did trustpinning originally so would like your review too.

riyazdf

code generally LGTM but have a test case and a couple of nits before the final green light 👍

riyazdf · 2017-03-31T01:04:35Z

trustpinning/certs_test.go

@@ -363,6 +363,26 @@ func TestValidateRootWithPinnedCertAndIntermediates(t *testing.T) {
 		}
 	}
 	require.Equal(t, typedSignedRoot, validatedRoot)
+
+	// test is also works with a wildcarded gun in certs


super nit: s/is/it/

riyazdf · 2017-03-31T01:06:47Z

trustpinning/trustpin.go

@@ -37,6 +37,11 @@ func NewTrustPinChecker(trustPinConfig TrustPinConfig, gun data.GUN, firstBootst
 		t.pinnedCertIDs = pinnedCerts
 		return t.certsCheck, nil
 	}
+	var ok bool
+	t.pinnedCertIDs, ok = wildcardMatch(gun, trustPinConfig.Certs)


nit: I think you can get away with := here and avoiding the extra var, but only because this is the last use of t.pinnedCertIDs. I'm generally of the opinion of the way you wrote this is more defensive though, so 👍

Will double check but I think one of the go tools was shouting at me when I originally had :=

Yeah, it complains about non-name t.pinnedCertIDs on left side of :=

riyazdf · 2017-03-31T01:08:40Z

trustpinning/trustpin.go

+		longest = ""
+		ids     []string
+	)
+	for k, v := range certs {


can we rename k and v to something more descriptive like gunPrefix and keyIDs?

riyazdf · 2017-03-31T01:11:12Z

trustpinning/certs_test.go

+			},
+			DisableTOFU: true,
+		},
+	)


can we add another test case or two for the precedence between globbed and non-globbed matches?

Ex:
this fails

Certs: map[string][]string{ "docker.io/notary": {"badID"}, "docker.io/notar*": {ecdsax509Key.ID()}, },

this succeeds

Certs: map[string][]string{ "docker.io/notary": {ecdsax509Key.ID()}, "docker.io/notar*": {"badID"}, },

Those are the other way around right? The first should succeed and the second should fail?

I thought that exact matches had precedence over wildcards?

nevermind, misunderstood because the docker.io/notary was short, you meant it to be docker.io/notary/test right? (i.e. the gun in use in the test)

sorry, yes that's what I meant!

Similar to this, can we also add a test that the more specific glob takes precedence over the less specific glob?
e.g.

Certs: map[string][]string{ "docker.io/notary/te*": {"badID"}, "docker.io/notar*": {ecdsax509Key.ID()}, },

fails while

Certs: map[string][]string{ "docker.io/notary/te*": {ecdsax509Key.ID()}, "docker.io/notar*": {"badID"}, },

succeeds?

Oh hm, nm - looks like https://github.com/docker/notary/pull/1126/files#diff-46adf7b1ba08ec329d671042ce0c84efR29 provides the test for the longest.

endophage · 2017-03-31T22:26:42Z

@riyazdf I think I addressed all your comments.

riyazdf

LGTM on green - I wasn't able to view the logs for the circleci failure but seemed like it was just on one container (the one with linting and postgres)

endophage · 2017-04-03T17:33:45Z

@riyazdf /go/src/github.com/docker/notary/trustpinning/certs_test.go:388:2: ineffectual assignment to validatedRoot fixing it now :-P

cyli · 2017-04-05T00:06:09Z

trustpinning/trustpin_test.go

+	require.True(t, ok)
+
+	// wildcardMatch should also match between segment boundaries, and take
+	// the first match is finds as the ONLY match.


Is docker.io/endophage/b* actually the first match it finds? Since we can't depend on map order, couldn't the keys have come in any order? It chooses the longest matching one, right?

ah sorry, I need to update the comment. I originally did first and was just going to have the user prioritize in their config file however they want, but that was before I remembered the random map ordering. Will update the comment.

cyli · 2017-04-05T00:06:31Z

Minor nitpick on a comment, but other than that LGTM!

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

GordonTheTurtle · 2017-04-05T18:06:22Z

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

$ git clone -b "wildcard_trustpin_key" git@github.com:endophage/notary.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354364088
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.

endophage force-pushed the wildcard_trustpin_key branch from 1ff3f2a to d13ebc4 Compare March 30, 2017 17:40

wildcarded trustpinning for cert ids

d7352fc

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage force-pushed the wildcard_trustpin_key branch from d13ebc4 to d7352fc Compare March 30, 2017 18:01

endophage requested review from riyazdf and ecordell March 31, 2017 00:10

riyazdf reviewed Mar 31, 2017

View reviewed changes

riyazdf approved these changes Mar 31, 2017

View reviewed changes

endophage force-pushed the wildcard_trustpin_key branch from ff3d1b1 to dc7fbd9 Compare April 3, 2017 17:38

cyli reviewed Apr 5, 2017

View reviewed changes

addressing riyaz's review

576bdbf

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)

endophage force-pushed the wildcard_trustpin_key branch from dc7fbd9 to 576bdbf Compare April 5, 2017 03:33

Merge branch 'master' into wildcard_trustpin_key

568d5b6

GordonTheTurtle added the dco/no label Apr 5, 2017

endophage merged commit 86e2ee9 into notaryproject:master Apr 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wildcarded trustpinning for cert ids #1126

wildcarded trustpinning for cert ids #1126

endophage commented Mar 30, 2017

endophage commented Mar 31, 2017

endophage commented Mar 31, 2017

riyazdf left a comment

riyazdf Mar 31, 2017

riyazdf Mar 31, 2017

endophage Mar 31, 2017

endophage Mar 31, 2017

riyazdf Mar 31, 2017

riyazdf Mar 31, 2017

endophage Mar 31, 2017

riyazdf Mar 31, 2017

endophage Mar 31, 2017

riyazdf Mar 31, 2017

cyli Apr 4, 2017

cyli Apr 5, 2017

endophage commented Mar 31, 2017

riyazdf left a comment

endophage commented Apr 3, 2017

cyli Apr 5, 2017

endophage Apr 5, 2017

cyli commented Apr 5, 2017

GordonTheTurtle commented Apr 5, 2017

wildcarded trustpinning for cert ids #1126

wildcarded trustpinning for cert ids #1126

Conversation

endophage commented Mar 30, 2017

endophage commented Mar 31, 2017

endophage commented Mar 31, 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

endophage commented Mar 31, 2017

riyazdf left a comment

Choose a reason for hiding this comment

endophage commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyli commented Apr 5, 2017

GordonTheTurtle commented Apr 5, 2017