zmap / zlint

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

[LSC] Convert all Lints to CertificateLints #767

Closed aaomidi closed 7 months ago

aaomidi commented 8 months ago

Partially addresses #765

Using the code on: https://github.com/aaomidi/zlint-migration, I've converted all lint.RegisterLint to lint.RegisterCertificateLint.

To make it easier for reviewers: look at the code here: https://github.com/aaomidi/zlint-migration, re-run this on the lints, it should create no diff compared to this PR.

Note that I've reviewed every file manually as well just in case, and I didn't spot anything.

robplee commented 7 months ago

I'm not sure how one would do this exactly, but I wonder if we could/should add something to the build process to try to make it impossible to add new lint.Lints or lint.LintInterfaces in PRs? Something on a similar line to Chris' magic that blocks people from merging changes to genTestCerts.go but that works everywhere?

I say this because I'm pretty sure you're going to have to fix some lints with this PR that I've had merged in since #699 was merged so I think there might be value in having a good solution that isn't "We'll just review it better" because there's enough stuff to check with zlint PRs to "just" make sure they're accurately implementing the rule they're adding. This feels like something we probably can check automatically so we probably should try.

aaomidi commented 7 months ago

I can actually extend the code I wrote to actively look for that in the CI step (?)

I probably don't want to include it in this PR though but I'll make a separate PR for this.

aaomidi commented 7 months ago

Are we waiting on anything else for this PR?

cardonator commented 7 months ago

I meant to try to use these changes in our build environment to make sure they wouldn't have any immediate effect on our usage. I was wondering if this change is significant enough that we should consider incrementing the version. Not sure if there is anything else blocking.

aaomidi commented 7 months ago

I think this change should be invisible for most folk. Good point though it makes sense to leave it ready for a few folks to test it out on their environment.

Also if this change does accidentally break anyone, we should definitely roll it back.

aaomidi commented 7 months ago

@robplee: I have the check you requested #770.

For maintainers: please let me know if you'd prefer the detection code to be part of this PR instead.

cardonator commented 7 months ago

Alright, I did some high level testing against this PR in our integration and confirmed that there were no issues for our use case. Ours is definitely not super complicated. I think the second step you described above would definitely break us so we should probably make sure we can cut a new release prior to that to give time for people to migrate over to the new interfaces. 👍

aaomidi commented 7 months ago

Thank you so much for testing this change on your end!

zakird commented 7 months ago

This looks reasonable to me. @christopher-henderson would you also be willing to take a look?

christopher-henderson commented 7 months ago

I'm not sure how one would do this exactly, but I wonder if we could/should add something to the build process to try to make it impossible to add new lint.Lints or lint.LintInterfaces in PRs? Something on a similar line to Chris' magic that blocks people from merging changes to genTestCerts.go but that works everywhere?

Indeed, doing so is possible (it parses the AST of lints and does something similar to what @aaomidi's code migrator is doing).

I'll be able to take this on (pending the conversation on implementing the breaking changes), as well as some repo cleanup activities regarding lint templating being slightly off.