zmap / zlint

X.509 Certificate Linter focused on Web PKI standards and requirements.
https://zmap.io
Apache License 2.0
347 stars 106 forks source link

Allow for individual lints to opt-out of the ZLint framework executing pre-flight applicability rules #842

Open christopher-henderson opened 1 month ago

christopher-henderson commented 1 month ago

Addresses #838

The core issue here was CABF places requirements, by-and-large, on server certificates. Thus, the framework did a kindness to CABF lints by baking in a pre-flight IsServerAuth check for all CABF lints.

This however, precluded that ability to lint certificates that are governed by CABF, but are not themselves server certificates. A prime example is v3/lints/cabf_br/lint_ocsp_id_pkix_ocsp_nocheck_ext_not_included_server_auth.go which is a CABF requirement placed on OCSP signing certificates.

As such, we would like for individual lints to be able to opt out of the framework's pre-flight applicability rules when necessary.w

Thank you @defacto64 for reporting the issue and thank you to @toddgaunt-gs for the cleaner boolean flag idea (I had forgotten that we had concrete structs available to us, rather than just interfaces that lints implement).

christopher-henderson commented 6 days ago

@zakird howdy sir. Could I solicit a quick glance at this PR? It's not PKI related so it's a bit difficult to garnish interest in a review. :pray:

zakird commented 6 days ago

Offhand, this feels like adding in an exception path to bypass fixing scoping of lints rather than fixing that our lints are currently not scoped correctly. Why not instead change the CheckApplies for subscriber certificates to check util.IsServerAuthCert(cert)?

defacto64 commented 5 days ago

This PR seems just fine to me. It adds the capability for a lint to do its own pre-flight applicability check, which sometimes can be useful when the general (framework) applicability pre-flight check would unwantedly exclude a certificate from such lint. Relying on util.IsServerAuthCert(cert) it's not the same thing, because that means relying on util.IsServerAuthCert(cert) doing always the "right" thing, which in fact is not always the case. For instance, as I found out myself, without this PR or an equivalent change, there is currently no way to lint an OCSP responder certificate according to the BR. At the same time, I do not think we shoud focus on just solving this particular annoyance, but rather to address the more general need - that is, increase flexibility.

christopher-henderson commented 5 days ago

@zakird ah yes, here's some added context. As @defacto64 brought up in his own experience, there are handful of times where a governing document declares requirements on a classification of certificates that are otherwise normally outside of the document's domain.

Concretely, in this case, CABF/BRs has an odd handful of requirements against OCSP responder certificates. This would be fine were it not for the fact that the framework applies it's own CheckApplies as a courtesy to lints. That is,

Listen, 99.9% of CABF/BR lints are going to be against server certs. So instead of asking every single lint check for that, we are going to apply this condition to every CABF/BR lint.

This is generally great and avoids a lot of mistakes from being made. It does, however, cause edge case certificates (such as the aforementioned OCSP responder certs) to go silently unloved.

zakird commented 5 days ago

Yeah, I'm with you that we'd have to update a huge number of CABF certs. I guess the question is, do we want this to be explicit and update all the lints or have it be implicit with a bypass mechanism. We'd have to do a lot fewer updates to lints, but it does make it more likely that we'll have this bug again in the future if someone doesn't know what they have to opt out of something. In general, ZLint tries to be pretty explicit about everything to avoid this kind of complication, but I also don't know if I'm putting code cleanliness over all practicality. I don't know if I have too strong an opinion. @dadrian what's your take?

dadrian commented 5 days ago

It seems like Source shouldn't define Scope. Might this make more sense as either as new subdirectory for OCSP lints, or a new property on the lint struct used for the CABF lints? I'm not sure it's good to just have a random opt-out for filtering, especially when the filtering is arbitrarily based on source. Instead, we should either make another source, or not base the filter (IsServerAuth) on source.