lint e_organizational_unit_name_prohibited CheckApplies may be overly permissive

[robplee]

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:

zlint/v3/lint/base.go

Line 101 in 44e12c1

if l.Source == CABFBaselineRequirements && !util.IsServerAuthCert(cert) {

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.

[christopher-henderson]

So #167 is the original discussion which ultimately resulted in this precondition being implemented in #171.

I believe the original intent was as follows:

1. Each lint features a 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.
2. The above, however, would have been remarkably noisy in code when whole categories of lints need to be skipped (for example, CA certificates for CABF/BR).

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.

zlint/v3/lints/cabf_br/lint_ca_key_cert_sign_not_set.go

Line 47 in 44e12c1

return c.IsCA && util.IsExtInCert(c, util.KeyUsageOID)

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 😉

[defacto64]

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):

1. does not correctly deal with the case of CA certificates to which the prohibition of the OU does not apply;

2. 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.

[robplee]

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)