wildcarded trustpinning for cert ids #1126
Conversation
endophage
force-pushed
the
wildcard_trustpin_key
branch
from
1ff3f2a to
d13ebc4
Compare
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
endophage
force-pushed
the
wildcard_trustpin_key
branch
from
d13ebc4 to
d7352fc
Compare
|
@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. |
|
@riyazdf you did trustpinning originally so would like your review too. |
riyazdf
left a comment
riyazdf
left a comment
There was a problem hiding this comment.
code generally LGTM but have a test case and a couple of nits before the final green light 👍
trustpinning/certs_test.go
Outdated
| } | ||
| require.Equal(t, typedSignedRoot, validatedRoot) | ||
|
|
||
| // test is also works with a wildcarded gun in certs |
| return t.certsCheck, nil | ||
| } | ||
| var ok bool | ||
| t.pinnedCertIDs, ok = wildcardMatch(gun, trustPinConfig.Certs) |
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Will double check but I think one of the go tools was shouting at me when I originally had :=
There was a problem hiding this comment.
Yeah, it complains about non-name t.pinnedCertIDs on left side of :=
trustpinning/trustpin.go
Outdated
| longest = "" | ||
| ids []string | ||
| ) | ||
| for k, v := range certs { |
There was a problem hiding this comment.
can we rename k and v to something more descriptive like gunPrefix and keyIDs?
| }, | ||
| DisableTOFU: true, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
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"},
},
There was a problem hiding this comment.
Those are the other way around right? The first should succeed and the second should fail?
There was a problem hiding this comment.
I thought that exact matches had precedence over wildcards?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
sorry, yes that's what I meant!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh hm, nm - looks like https://github.com/docker/notary/pull/1126/files#diff-46adf7b1ba08ec329d671042ce0c84efR29 provides the test for the longest.
|
@riyazdf I think I addressed all your comments. |
riyazdf
left a comment
riyazdf
left a comment
There was a problem hiding this comment.
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)
|
@riyazdf |
endophage
force-pushed
the
wildcard_trustpin_key
branch
from
ff3d1b1 to
dc7fbd9
Compare
trustpinning/trustpin_test.go
Outdated
| require.True(t, ok) | ||
|
|
||
| // wildcardMatch should also match between segment boundaries, and take | ||
| // the first match is finds as the ONLY match. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
Minor nitpick on a comment, but other than that LGTM! |
Signed-off-by: David Lawrence <[email protected]> (github: endophage)
endophage
force-pushed
the
wildcard_trustpin_key
branch
from
dc7fbd9 to
576bdbf
Compare
|
Please sign your commits following these rules: $ git clone -b "wildcard_trustpin_key" [email protected]: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 -fAmending updates the existing PR. You DO NOT need to open a new one. |
4 participants
Closes #1123
Signed-off-by: David Lawrence [email protected] (github: endophage)