Add custom lints covering our CP/CPS profiles - #8485
Conversation
e0879bd to
cbb7695
Compare
mcpherrinm
left a comment
mcpherrinm
left a comment
There was a problem hiding this comment.
I think this looks pretty good structurally.
I think the biggest concern is I think we could issue a cert for which CheckApplies is false on all lints, and we wouldn't lint it as a result.
My secondary concern is that I think we could make more use of some helper functions to validate each field, as the checks are nearly the same across several lints. I left one comment to that effect but it holds in numerous places.
|
|
||
| func (l *subscriberServerCertificateMatchesCPSProfile) CheckApplies(c *x509.Certificate) bool { | ||
| // Precertificates are covered by the Precertificate Profile instead. | ||
| return util.IsSubscriberCert(c) && util.IsServerAuthCert(c) && !util.IsExtInCert(c, util.CtPoisonOID) |
There was a problem hiding this comment.
If we ever decided to issue a client auth only cert for whatever reason, I think we'd get a false CheckApplies here.
I think we want a lint that ensures a check is either a cert or precert (for prod boulder), or makes sure it is a root, cross, or intermediate (for ceremony)
There was a problem hiding this comment.
Yeah I was having similar thoughts. I don't love the idea of combining them all into just two lints, but I've been contemplating if it would make sense to require that all of our lint environments (the CA, cert-checker, and the ceremony tool) explicitly disable all of the CP/CPS lints except for the one that applies in each situation. Would that make sense to you? Or would you prefer to go with some sort of meta-lint system?
There was a problem hiding this comment.
What if we had a separate lint that has no CheckApplies, but instead the lint execution verifies that calling at least one of the other lint's CheckApplies returns true?
Or equivalently, the lint Execute always returns an error, and CheckApplies returns true when all the others return false.
It's a bit weird encapsulation-wise, in that you'd invoke the CheckApplies function from the other lints, but that seems relatively safe to me.
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Partial review; I'm as far as linter/lints/cpcps/lint_cross_certified_subordinate_ca_certificate.go.
|
I've responded to all comments except for the one about |
5 participants
Add six new custom zlint lints, each covering one of the profiles listed in Version TKTK of our CP/CPS:
Each lint walks down the corresponding table in our CP/CPS, checking each row in order. Every check includes a quote of the relevant statement and a link to the exact commit and line of our CP/CPS. This will allow us to automated enforcement of updates to our lints when our CP/CPS changes, and vice versa. The meta lint ensures that every certificate qualifies for exactly one of the individual lints, ensuring that we don't issue any certificates which dodge being checked by at least one, and ensuring that we never try to count a cert as meeting multiple profiles.
Several of the items in our CP/CPS profiles state that our certificates either match their issuer certificate, or (for Cross-Certified certs) match a pre-existing certificate. In order to support this, abuse zlint's configuration mechanism to allow us to configure the issuer and pre-existing cert before each lint run. Each of the new lints uses the configured issuer to perform several of its checks, and fails the lint if not so configured.
Fixes #5492
An LLM was used to generate three of the lints in the style of the first two hand-written ones, and to generate some test cases for all five.
Warning
DO NOT MERGE until we are confident that we are going to publish a new version of our CP/CPS based on letsencrypt/cp-cps#307. Then, update this PR description and the links within each lint to point at a version tag instead of a commit sha1.