ceremony: Add support for CRL onlyContainsCACerts

[pgporada]

• Allows the ceremony tool to add the onlyContainsCACerts flag to the IssuingDistributionPoint extension[1] for CRLs.
• Add a lint to detect basic usage of this new flag.
• Add a helper function which doesn't (yet) exist in golang x/crypto/cryptobyte named ReadOptionalASN1BooleanWithTag which searches for an optional DER-encoded ASN.1 element tagged with a given tag e.g. onlyContainsUserCerts and reports values back to the caller.
• Each revoked certificate in the CRL config is checked for is IsCA to maintain conformance with RFC 5280 Section 6.3.3 b.2.iii [2].

  (iii) If the onlyContainsCACerts boolean is asserted in the
  IDP CRL extension, verify that the certificate
  includes the basic constraints extension with the cA
  boolean asserted.

Fixes #7047

1. https://datatracker.ietf.org/doc/html/rfc5280#section-5.2.5
2. https://datatracker.ietf.org/doc/html/rfc5280#section-6.3.3

[aarongable]

Ceremony code LGTM! Just iterating on the lints.

[pgporada]

Hey that's ok, I still got some experience playing with this code. I'll get it all fixed up tomorrow.

…

On Thu, Sep 7, 2023, 6:03 PM Aaron Gable ***@***.***> wrote: ***@***.**** commented on this pull request. Ceremony code LGTM! Just iterating on the lints. ------------------------------ In cmd/ceremony/crl.go <#7064 (comment)>: > // We skip this lint because our ceremony tooling issues CRLs with validity // periods up to 12 months, but the lint only allows up to 10 days (which // is the limit for CRLs containing Subscriber Certificates). - "e_crl_validity_period", - // We skip this lint because it is only applicable for sharded/partitioned - // CRLs, which our Subscriber CRLs are, but our higher-level CRLs issued by - // this tool are not. - "e_crl_has_idp", + "e_crl_validity_period_subscriber_cert", + // This lint is skipped because it only applies to CRLs generated by the + // crl-updater which contain strictly subscriber certificates. The + // ceremony tool produced CRLs must contain strictly Subordinate CA + // certificates. + "e_crl_has_idp_subscriber_cert", Rather than skipping these lints here, we should use the linting system's CheckApplies function to determine whether the lints apply, or use the contents of the IDP extension to determine a code-path through the lint. I've left more comments detailing my thoughts below. ------------------------------ On linter/lints/cabf_br/lint_crl_validity_period_subordinate_ca.go <#7064 (comment)>: I don't think we need this file. See my comment on lint_crl_validity_period_subscriber_cert.go, below. ------------------------------ In linter/lints/cabf_br/lint_crl_validity_period_subscriber_cert.go <#7064 (comment)>: > return true } -func (l *crlValidityPeriod) Execute(c *x509.RevocationList) *lint.LintResult { +func (l *crlValidityPeriodSubscriberCert) Execute(c *x509.RevocationList) *lint.LintResult { I think this file can remain lint_crl_validity_period.go. This Execute function can: 1. Check for the presence of an IDP extension and the OnlyContainsCACerts bool in it 2. If it finds both, then assert validity is less than 12 months 3. Otherwise, assert validity is less than 10 days So basically a CRL which asserts it only contains CA certs is allowed to fall into the 12-month bucket specified by BRs 7.2, while all other CRLs must be assumed to possibly contain Subscriber certs and therefore fall into the 10-day bucket specified by the same section. ------------------------------ In linter/lints/cpcps/lint_crl_has_idp_subscriber_cert.go <#7064 (comment)>: > return true } -func (l *crlHasIDP) Execute(c *x509.RevocationList) *lint.LintResult { +func (l *crlHasIDPSubscriberCert) Execute(c *x509.RevocationList) *lint.LintResult { Similar idea here. I think we can get away with: 1. Extract the OnlyContainsUserCerts, OnlyContainsCACerts, and DistributionPoint fields 2. Assert that either: - just the OnlyContainsCACerts bool is set; or - both OnlyContainsUserCerts and DistributionPoint are set; - any other configuration is an error I'm sorry that I led you astray in slack DMs. I wasn't thinking about the fact that lints need to know whether or not to run, and the only way for these lints to know whether or not to run is... to parse the whole IDP extension, at which point we might as well just do the check then and there. When I suggested two separate lints for the Subscriber and Subordinate CA paths here, I was forgetting that we can't inherently know which kind of CRL we're linting until we actually get to this parsing stage. — Reply to this email directly, view it on GitHub <#7064 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AASFVZN6RW2ARJYTTQNBJQTXZJAB3ANCNFSM6AAAAAA4GUYB7Q> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[pgporada]

Tomorrow turned out to take a whole week, but 🤷🏼‍♂️.

[aarongable]

Just a few small comments on the lints. This looks so good!

[jsha]

This is looking really nice now. In particular, you've now split out the parsing of the ASN.1 from validating the parsed contents. One final step if you'd like to take it: you could factor out the parsing of the ASN.1 into a helper function that returns an issuingDistributionPoint struct:

type issuingDistributionPoint struct {
  distributionPointURI *url.URL
  onlyContainsUserCerts bool
  onlyContainsCACerts bool
}

This would allow you to reuse some code between cpcps and the cabf_br lifetime check.