letsencrypt / boulder

An ACME-based certificate authority, written in Go.
Mozilla Public License 2.0
5.15k stars 601 forks source link

Add zlint lints based on our specific CP/CPS #5492

Open aarongable opened 3 years ago

aarongable commented 3 years ago

We do pre-issuance linting using zlint to ensure that we are compliant with the baseline requirements and various root program requirements. As Bug 1715455 shows, it is possible to still misissue by being in violation of one's own CP/CPS, rather than in violation of root program requirements. We should consider forking zlint to add a new directory of lints based on the contents of our own CP/CPS.

cpu commented 3 years ago

Do you need to fork ZLint for this? As a library user I think it should be possible to compose a lint.Registry with lints of your own from outside the ZLint src tree that you can use with the same linting interface. If it turns out that there's something that prevents that from working well I'd be interested in trying to help fix it upstream in ZLint. Forking is always an option but I expect other CAs have this use-case too and I think it makes sense to support it upstream.

aarongable commented 3 years ago

Yeah, I'd prefer to do it without forking as well. In the period of time between coming up with this idea and filing this bug I took a very quick look at the zlint docs. The section on "Lint Sources" didn't mention bringing your own, nor did the section on "Extending Zlint", so I just assumed there weren't affordances for doing so. But I haven't looked any further than that. I'll definitely take a closer look when we actually try to implement this.

aarongable commented 3 years ago

Ooh, I wonder if we could actually store the lints in the CP/CPS repo itself, and then vendor that repo into boulder like any other dependency. That's probably too much of a pain from an automation / CI / ease-of-editing perspective but I kinda like the idea of guaranteeing they're in sync.

cpu commented 3 years ago

Providing out-of-tree lints was a use-case I had in mind when I did the refactoring that allowed excluding categories of lints but I don't think it's been tried in practice (or captured in docs :sweat:). There might be some bumps in the road but I think it should be achievable :crossed_fingers:

aarongable commented 3 years ago

Turns out this is pretty straightforward. I've created a proof-of-concept PR above, adding the very first lints. Tests and additional lints will be forthcoming.

aarongable commented 3 years ago

The first version of these lints have landed! Next steps are: 1) Add more lints. Currently there's just lifetime; we need to do policy OIDs, key sizes, key usages, and more. 2) Add tests for the lints. The zlint tests use a format I don't like very much (a bajillion hard-coded certs, one or more for every lint) so that'll be a little bit interesting. 3) Look into moving these lints to the cp-cps repo and then vendoring them in here. I think that would be pretty easy, actually, since none of the lints rely on Boulder common utilities.

sheurich commented 3 years ago

Would you consider moving the lints into configuration files? This would promote cleaner re-use of Boulder with other CA certificates.

aarongable commented 3 years ago

All lints can be excluded via existing configuration files. If/when we move the lints into our cp-cps repo, they could easily be substituted via a replace directive in go.mod. Zlint doesn't have a way to dynamically load lints at runtime based on e.g. lists of directories that should be searched for lints, so we can't use configuration files as allowlists, only as blocklists. The only possibility I see would be to extend our current configuration file syntax to allow excluding entire lint sources, but that feels like a low priority at the moment when the number of lints is so small.

sheurich commented 3 years ago

Ah didn't realize the allow list limitation. Thanks 😄