-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add CRL linting infrastructure #699
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Howdy Amir!
Thank you so much. Not only for your contributions, but also for your clear care and technical due-diligence.
While I indeed have comments to leave behind, this is an important enough change that I believe code collaboration may be the best path forward (in the interest of both time and clarity).
As such, I've opened up my on own PR against your source branch (aaomidi#2). A fork-of-a-fork is somewhat unwieldy, but I believe that we can make it work. At the very least, I believe that it is (one of) the best ways for me to share my inputs. If you have an alternative workflow, then I welcome it!
v3/lint/registration.go
Outdated
} | ||
|
||
// LinterLookup is an interface describing how registered lints can be looked up. | ||
type LinterLookup[T Lints] interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, so while ZLint does indeed compile against 1.18+, we have yet to do a survey of those who consume ZLint as a library to ensure that everyone else has moved on as well.
As such, we have refrained from using generics so far in order to not cause any breakages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been working on this and it's a bit unwieldy so far, going to try to re-approach it and see where I end up. However I did want to make note that I believe go < 1.18 has hit end of life.
v3/lint/registration.go
Outdated
} | ||
|
||
type lookupImpl[T Lints] struct { | ||
sync.RWMutex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for considering thread safety in your datastructures!
While the ZLint (as a binary) is single threaded, it is easy to imagine library consumes attempting to run lints in parallel. Indeed, this was a motivator in shifting lints from being singleton structs to storing constructors that can return new structs on a per-lint-run basis.
Chris Henderson's Code Suggestions for CRL Linting Infrastructure
@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 (aaomidi#4) that pulls in a commit from ZCrypto which itself properly brings in the Speaking of which, I went ahead and imported the 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 |
* Take out generics from the registration struct (#3) * updating to use zcrypto * pointing zcrypto back to master * go tidy up --------- Co-authored-by: Amir Omidi <amir@aaomidi.com>
@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: zmap/zcrypto#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: aaomidi#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. |
Yes, I took a look at the example that you provided and looks perfectly reasonable to me.
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:
If instead I feed it a CRL in PEM encoding, the error message is like this: 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 af90382 that allows for PEM encode DERs with the X509 CRL header armor to be accepted and dispatched to the appropriate infrastructure. |
Nice! |
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
.