Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

christopher-henderson
Copy link
Member

@christopher-henderson christopher-henderson commented May 11, 2024

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

@@ -74,11 +74,11 @@ func TestOCSPIDPKIXOCSPNocheckExtNotIncludedServerAuth(t *testing.T) {
}, {
Name: "o1s0ep0a0nc0",
Filename: "o1s0ep0a0nc0.pem",
ExpectedResult: lint.NA,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are tests certs that do not have server auth, so they now apply.

@christopher-henderson christopher-henderson changed the title OCSP responder certificates are not actually linted Allow for individual lints to opt-out of the ZLint framework executing pre-flight applicability rules May 26, 2024
@christopher-henderson
Copy link
Member Author

@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. 🙏

@zakird
Copy link
Member

zakird commented Jun 29, 2024

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
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Member

zakird commented Jun 30, 2024

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
Copy link
Member

dadrian commented Jul 1, 2024

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.

@christopher-henderson
Copy link
Member Author

To help bring this conversation back to the ground, I'd like to point out that this issue was raised because of precisely one lint that has existed for over four years and had gone unnoticed until @defacto64 read the code base carefully.

So I am quite allergic to altering the fundamental scoping strategy of the project. If there is no solution that even a plurality of folks are comfortable with, then I would sooner leave the issue unresolved than risk major changes.

@defacto64
Copy link
Contributor

Right.
Let's put it this way: do we think it is OK that Zlint cannot currently be used to lint an OCSP certificate for BR compliance?
IMO it is an ugly limitation, but one that is easy to fix in the proposed way or onother.
Besides, the proposed fix is very light, neat, and apparently does not break anything.
The merits of other possible solutions are hard to judge until one is put forward.

@toddgaunt-gs
Copy link
Contributor

Not that anyone asked, but just to chime in I am fine with this solution. I agree with @defacto64 that this is the simplest fix that impacts the codebase the least and it is easily understandable. While we could introduce another field to determine scoping that isn't Source, that would require each lint currently relying on the Source scoping to be updated and would be very similar to this change.

Alternatively we could just add another check to see if the certificate being linted satisfies util.IsDelegatedOCSPResponderCert(c), however its been long enough that I forget if there was a reason we aren't just doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants