Add lint to check that an SCT list is not empty #837
Conversation
Added //nolint:all to comment block to avoid golangci-lint to complain about duplicate words in comment
Fixed import block
Fine to me. Co-authored-by: Christopher Henderson <chris@chenderson.org>
As per Chris Henderson's suggestion, to "improve readability".
As per Chris Henderson's suggestion.
Added CABFEV_Sec9_2_8_Date
| // CheckApplies returns true for any subscriber certificates that are not precertificates | ||
| // (i.e. that do not have the CT poison extension defined in RFC 6962) | ||
| func (l *emptySCTList) CheckApplies(c *x509.Certificate) bool { | ||
| return util.IsSubscriberCert(c) && !util.IsExtInCert(c, util.CtPoisonOID) |
There was a problem hiding this comment.
Should precertificates be excluded? The lint would currently pass with a precertificate because it wouldn't have a SCT extension. If you exclude precertificates, the lint wouldn't catch a misissued certificate that had a poison OID and an empty SCT extension. Maybe instead you can do something like this and check if the extension exists.
| return util.IsSubscriberCert(c) && !util.IsExtInCert(c, util.CtPoisonOID) | |
| return util.IsSubscriberCert(c) && util.IsExtInCert(c, util.TimestampOID) |
There was a problem hiding this comment.
Yes, Mathew, I think precertificates should be excluded from this lint because precertificates are not supposed to contain SCTs, and if they do it's a different kind of non-compliance which, IMO, would be better handled by a separate lint (which, by the way, I intended to contribute later on...quite soon).
There was a problem hiding this comment.
In the meantime, I just opened #841 to add the other lint I was talking about before.
If you agree, I would "Resolve conversation", but we can continue the discussion if you wish.
There was a problem hiding this comment.
This seems reasonable to me, and I agree that preissuance certificates are a separate matter. Of course, I will let @mathewhodson continue that conversation if he has further inputs.
v3/util/time.go
Outdated
| @@ -37,6 +37,7 @@ var ( | |||
| RFC4630Date = time.Date(2006, time.August, 1, 0, 0, 0, 0, time.UTC) | |||
| RFC5280Date = time.Date(2008, time.May, 1, 0, 0, 0, 0, time.UTC) | |||
| RFC6818Date = time.Date(2013, time.January, 1, 0, 0, 0, 0, time.UTC) | |||
| RFC6962Date = time.Date(2013, time.June, 1, 0, 0, 0, 0, time.UTC) | |||
There was a problem hiding this comment.
Comment on this already made in #841. So long as both go in, then it doesn't really matter which has the infra changes.
At the moment, Zlint does not check that the value of the SCTList extension complies with RFC 6962 section 3.3, in particular that "At least one SCT MUST be included." There is a lint that counts SCTs and issues just an INFO if the number doesn't meet Apple's policy, but that's a different matter. Here, instead, we check that, when the extension is present in the certificate, the list of SCTs therein contained is not empty. This problem has actually happened at least once in the past as can be seen on Bugzilla.