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 |
| // https://github.com/letsencrypt/cp-cps/blob/TKTK-replace-with-version-tag/CP-CPS.md?plain=1#L1117 | ||
| // When used in the context of a signature, fields of type `AlgorithmIdentifier` of all objects signed by ISRG CAs are byte-for-byte identical with one of the hexadecimal encodings specified by Section 7.1.3.2 of the Baseline Requirements. | ||
| // These are the AlgorithmIdentifier encodings specified by Section | ||
| // 7.1.3.2 of the Baseline Requirements. |
There was a problem hiding this comment.
| // 7.1.3.2 of the Baseline Requirements. | |
| // 7.1.3.2 of the Baseline Requirements, except the three RSASSA-PSS | |
| // encodings, which ISRG never uses. |
| } | ||
|
|
||
| // https://github.com/letsencrypt/cp-cps/blob/TKTK-replace-with-version-tag/CP-CPS.md?plain=1#L1032 | ||
| // | `certificatePolicies` | Contains only the Baseline Requirements Domain Validated Reserved Policy Identifier (OID 2.23.140.1.2.1) | |
There was a problem hiding this comment.
A certificatePolicies extension containing the DV OID plus an id-qt-cps policyQualifier passes this check and all of stock zlint because the BRs section 7.1.2.7.9 permits them. We never include them, though, so we should exclude them explicitly:
for _, qualifiers := range c.QualifierId {
if len(qualifiers) != 0 {
return errResult("certificatePolicies contains a policyQualifier")
}
}| } | ||
|
|
||
| // https://github.com/letsencrypt/cp-cps/blob/TKTK-replace-with-version-tag/CP-CPS.md?plain=1#L1086 | ||
| // | `certificatePolicies` | Contains only the Baseline Requirements Domain Validated Reserved Policy Identifier (OID 2.23.140.1.2.1) | |
There was a problem hiding this comment.
Same comment regarding policyQualifiers.
| } | ||
|
|
||
| // https://github.com/letsencrypt/cp-cps/blob/TKTK-replace-with-version-tag/CP-CPS.md?plain=1#L1059 | ||
| // | `certificatePolicies` | Contains only the Baseline Requirements Domain Validated Reserved Policy Identifier (OID 2.23.140.1.2.1) | |
There was a problem hiding this comment.
Same comment regarding policyQualifiers.
4 participants
Add five 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.
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.
Warning
DO NOT MERGE until we have a solution for cert-checker, which does not currently have a way to supply the issuer certificate to each of these lints. We can either give it that capability, or configure it to ignore these lints, or make the lints not fail if they're not configured.