Support wildcard CNs in root leaf certificates #1088
Conversation
ecordell
force-pushed
the
wildcard-certs
branch
3 times, most recently
from
b481fb3 to
fb428da
Compare
| // MatchCNToGun checks that the common name in a cert is valid for the given gun. | ||
| // This allows wildcards as suffixes, e.g. `namespace/*` | ||
| func MatchCNToGun(commonName, gun string) bool { | ||
| wildcard := "*" |
ecordell
force-pushed
the
wildcard-certs
branch
from
fb428da to
00a7cba
Compare
|
Glad you avoided One thought, should we require the |
| // MatchCNToGun checks that the common name in a cert is valid for the given gun. | ||
| // This allows wildcards as suffixes, e.g. `namespace/*` | ||
| func MatchCNToGun(commonName, gun string) bool { | ||
| const wildcard = "*" |
There was a problem hiding this comment.
I figured a const at probably package scope :-P This works too as far as giving us immutability though.
There was a problem hiding this comment.
figured the go compiler would do what I wanted here, didn't really verify though
There was a problem hiding this comment.
It probably will as far as optimization, I was thinking more along code organization.
Personally I think that's a little too restrictive; I was considering an organization using a wildcard cert for a subset of their packages, e.g. |
| {"test/*/wild", "test/test/wild", false}, | ||
| {"*/all", "test/all", false}, | ||
| {"docker.com/*/*", "docker.com/notary/test", false}, | ||
| {"*", "docker.com/any", true}, |
There was a problem hiding this comment.
can we add the following test cases:
CN: "", gun: *, out: false
CN: *, gun: "", out: true
CN: *abc*, gun: "abc", out: false (since we check against *abc, no longer wildcarded)
CN: test/*/wild*, gun: "test/test/wild", out: false (since we check against test/*/wild, no longer wildcarded)
There was a problem hiding this comment.
Out of curiosity, if the common name is **, should we treat that as *? If not, can we also add one more test?
CN: **, gun: "docker.com/any", out: false
There was a problem hiding this comment.
Non-blocking: Since we're not actually checking any trust pinning here, do we need a CA+intermediate+leaf for this test? Might make the test a little shorter to just include the leaf.
There was a problem hiding this comment.
@cyli: good idea - I like treating ** as * for now to keep things simple so that test would be great 👍
There was a problem hiding this comment.
I went ahead and switched TrimSuffix to TrimRight so that any number of trailing *s will collapse down to 1.
| func MatchCNToGun(commonName, gun string) bool { | ||
| const wildcard = "*" | ||
| if strings.HasSuffix(commonName, wildcard) { | ||
| prefix := commonName[:len(commonName)-len(wildcard)] |
There was a problem hiding this comment.
nit style: I'd prefer using strings.TrimSuffix instead of indexing directly
There was a problem hiding this comment.
Also, could we add a log debug to display which prefix we're comparing against?
My concern is if folks use something like prefix** and expect matching against prefix instead of prefix*
ecordell
force-pushed
the
wildcard-certs
branch
from
00a7cba to
70d3da2
Compare
| @@ -1299,3 +1299,193 @@ func TestValidateRootWithExpiredIntermediate(t *testing.T) { | |||
| ) | |||
| require.Error(t, err, "failed to invalidate expired intermediate certificate") | |||
| } | |||
|
|
|||
| func TestCheckingWilcardCert(t *testing.T) { | |||
There was a problem hiding this comment.
nit: "TestCheckingWildcartCert"
ecordell
force-pushed
the
wildcard-certs
branch
2 times, most recently
from
d474056 to
764e056
Compare
Signed-off-by: Evan Cordell <cordell.evan@gmail.com>
ecordell
force-pushed
the
wildcard-certs
branch
from
764e056 to
dfa8a5b
Compare
|
How do I enable this feature? Today the root public key in the root.json file is a self-signed cert with CN == GUN. How do I use this such that the CN is a wildcard such as www.example.com/* ? |
|
@endophage - we are making the changes (actually already done, just testing now) as well as some others that you and I discussed. You should see a pull request in the next few days. Once this is in place, we are planning to work on CA pinning, and then moving trust pinning into DCT. Once that's all finished, we can work on getting generalizing the pkcs11 interface to support HSMs other than Yubikey :-) |
This adds support for explicit wildcard suffixes in root (leaf) certificates of the form
prefix*.I considered doing something more flexible using
filepath.Matchbut I think this more conservative form is safer and (probably) just as useful. It would be easy to change in the future if more flexibility (e.g.docker.io/*/test) is needed.Note the wildcard
*indicator is required to avoid accidentally using wildcard certs (e.g.docker.io/testanddocker.io/test2).Fixes #883
Possibly addresses #914 as well?