Closed robplee closed 1 year ago
So #167 is the original discussion which ultimately resulted in this precondition being implemented in #171.
I believe the original intent was as follows:
CheckApplies
method whose intent is to simply bail on the lint of the given certificate is not bound to the requirement cited for that lint.The result is an ad-hoc check in the one case that I reckon seemed apparent at the time - BR only applies to ServerAuth certificates.
Now, we do have BR lints that target CA certs (as an example cabf_br/lint_ca_key_cert_sign_not_set.go). This lint simply checks for IsCA
win CheckApplies
.
So we should be able to just as easily add !c.IsCA
to #675.
I am intrigued at the idea of fine-grain scoping, however. We categorize by requirement documents, but not really anything beyond that. Any additional preconditions get shoved into CheckApplies
. While this works perfectly fine, it can get redundant and, more important, lossy if the author is not entirely certain on their target scope.
One could easily imagine an additional Scope
method added to the lint interface.
func (ml *MyLint) Scope() []lint.Scope {
return []lint.Scope{
lint.CA,
lint.Intermediate,
lint.NotServerAuth,
...
}
}
This, of course, is not entirely orthogonal with CheckApplies
and could just as easily cause confusion. Just spitballing, though.
rather than having this extra check elsewhere that is easily forgotten about and probably causes frustration for some of the people
Early on in my introduction to this codebase I got bit - hard - by this particular line of code :wink:
In my opinion the lint proposed by @robplee - while certainly useful - requires some more (easy) work, for two reasons (one already discussed here but not fixed yet):
does not correctly deal with the case of CA certificates to which the prohibition of the OU does not apply;
does not consider the case of certificates issued with OU before Sep 2022.
We must consider that zlint is not only used on certificates that are about to be issued (in order to avoid misissuance) but also on already existing certificates (e.g. by widely used instruments such as Censys and crt.sh), so handling case 2 correctly seems important to me.
Ok. On that note, I'll open a PR today to add the !c.IsCA to the #675 CheckApplies and a unit test to check it's working.
@defacto64, your second point is covered by the effective date of the lint which is set to 1st September so fortunately no changes are needed there (see the test case using certificate ouPresentBeforeSep22.pem)
Following from the conversation between @defacto64 and myself on #689 it seems like the lint I added in #675 may need some adjustment?
Currently the CheckApplies function returns true meaning the only check to determine if the lint is to be applied is the one in here:
https://github.com/zmap/zlint/blob/44e12c12ca43a4af86f0dc2da4a71493ac9f8345/v3/lint/base.go#L101
Which ensures that the cert being tested contain either ServerAuth EKU or Any. However, @defacto64 's comment is that this would mean the lint complains about intermediate CA certs with an OU cert so I guess the question for the room is if this is a bug or a feature. I'll also add that the next section in the BRs on Root and Subordinate CAs doesn't mention OUs at all so that might suggest that OUs are to be prohibited in these certs but I thought I'd raise an issue and now we have somewhere to discuss this.