Closed aaomidi closed 1 year ago
@aaomidi the code aspects aside, do you happen to have a notion of a what a good first lint would look like for this infrastructure?
It would serve several purposes:
Hey @christopher-henderson, my plan is to initially implement the lints Let's Encrypt has implemented: https://github.com/letsencrypt/boulder/blob/main/linter/lints/crl/lints.go
Howdy @aaomidi
I fired up a PR against your branch (https://github.com/aaomidi/zlint/pull/4) that pulls in a commit from ZCrypto which itself properly brings in the x509.RevocationList
type so that we don't have to muck about with having both Zcrypto and the stdlib operating at the same time.
Speaking of which, I went ahead and imported the RevocationList
into Zcrypto (https://github.com/zmap/zcrypto/pull/349). It is not merged yet, athough we can certainly refer to this particular branch in go.mod
for the meantime.
I also went ahead and tested the changes against go 1.16 (our minimum required version) and everything appears okay so far (thank you for taking the generics out! I am considering firing up a tracking issue of all of the breaking change that we would like to accomplish a a major update one day).
Awesome news @christopher-henderson.
Let's maybe brainstorm a bit on on maintaining a patch-list for zcrypto as well, so we can follow upstream a bit more?
I'll review your PR today - cheers!
@aaomidi I believe that I had scraped through all references to the mainline x509
so I don't think that there are any outstanding changes to zcrypto that needs addressing (unless I missed something or there are further requirements/usecases you had in mind).
@christopher-henderson Would you like to see this PR bring in some revocation list lints too, or should I make a PR on top of this one?
I was thinking of keeping these separate, but I'm open to opinions.
@aaomidi thank you for asking! I think it would be easier to read if they were branches based off of this one. This change alone touched just enough files that some smaller details can get easily get lost.
@christopher-henderson I have a new PR open against zmap: https://github.com/zmap/zcrypto/pull/354 this will enable me to write a CRL lint
@aaomidi oh good! I was planning on reaching out today regardless,
Just so that I can make sure that you're never blocked on me, is the plan that we get the changes into zcrypto that we need, write a single lint-or-two, and then that's the it for the merge candidate?
That's the plan, I started writing one of the lints (checking if CRL hasUpdate is valid, and probably a few more), and realized I need the code to parse a CRL.
I think once that's done we should be able to merge this.
@christopher-henderson I've added a PR onto the crls branch that adds a single CRL lint: https://github.com/aaomidi/zlint/pull/5
I think my plan currently is:
If this looks good, let's get the the single lint merged into this branch. Then maybe let's get this branch merged in? We can then follow up with a few more lints in separate PRs.
My justification is that this is a simple lint and doesn't really need a lot of external review? The other lints might be a bit more complex, and might warrant more review on them. This PR is big enough already.
If this looks good, let's get the the single lint merged into this branch.
Yes, I took a look at the example that you provided and looks perfectly reasonable to me.
Then maybe let's get this branch merged in?
I believe that that would be appropriate as both the lint interface as well as the testing interfaces appear to not be out of line with extant patterns. So I believe the consumers of the library aspect of this repo will be able to succeed.
Thank you @aaomidi! We'll be sure to highlight your change in the next release!
Dear all,
I am not clear if it is possible to lint a CRL from the command line....?
@defacto64 I actually am not sure. I don't think I tested this when I was implementing CRL linting. I'll turn that into an issue and write up a PR if the investigation shows that I can't use this through CLI.
Well, if I feed a CRL in DER encoding to zlint (with option -format der), it shows this kind of error:
time="2023-05-31T17:13:50+02:00" level=fatal msg="unable to parse certificate: asn1: structure error: tags don't match (16 vs {class:0 tag:23 length:13 isCompound:false}) {optional:false explicit:false application:false private:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false} validity @211"
If instead I feed it a CRL in PEM encoding, the error message is like this:
time="2023-05-31T17:15:44+02:00" level=fatal msg="unable to parse PEM"
So I guess at this time Zlint cannot lint a CRL from the command-line....
Indeed this is quite the embarrassing little oversight/bug. Thank you, @defacto64, for your testing!
A patch has been merged at https://github.com/zmap/zlint/commit/af903824a31385208566fa640cc13036a0e4d8e4 that allows for PEM encode DERs with the X509 CRL header armor to be accepted and dispatched to the appropriate infrastructure. A new release candidate has also been pushed to https://github.com/zmap/zlint/releases/tag/v3.5.0-rc2
Nice! Now it works, at least with PEM-encoded CRLs, although there is only one CRL lint at this time ("e_crl_has_next_update"). Maybe I will try to develop one for checking valid CRL entry reason codes, if that's not already in progress....
ZLint is an invaluable asset for pre-linting certificate issuances and creating a technical enforcement of the various rules around certificate issuance. The recent incidents 1, 2 involving CRLs have created a need to extend linting to CRLs as well.
This PR aims to bring CRL linting and create the necessary code infrastructure to address #458 to add OCSP linting support to ZLint without having to do any large modifications to the codebase.
The major requirement with this PR is to not unintentionally break the existing lints. The decisions made in this PR are generally a reflection of this goal. The major highlights of these changes are as follows:
CertificateLintInterface
andRevocationListLintInterface
. Deprecate the existingLintInterface
.CertificateLint
andRevocationListLint
. Deprecate the existingLint
struct.LintMeta
to reduce repeating code between the two new lints.Lint
toCertificateLint
and vice versa.LinterLookup
as a generic interface that acts as the storage backend for the different types of lints.Lint
is now deprecated, and it is recommended to use RegisterCertificateLint and RegisterRevocationListLint for Certificates and CRLs respectively.Some other notes:
@deprecated
to the godoc description of these methods.Future work:
Lint
s to beCertificateLint
instead. This migration is only necessary due to the introduction ofLintMeta
.