From ef1e9a34125175daf8378eedcb458864ea90b7c2 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 8 Feb 2024 15:14:57 -0800 Subject: [PATCH] Improve CRL IssuingDistributionPoint lint (#7299) Improve our custom CRL IssuingDistributionPoint lint to more accurately reflect the fact that an IssuingDistributionPoint's DistributionPoint field's FullName field can contain multiple GeneralNames. Part of https://github.com/letsencrypt/boulder/issues/7296 --- linter/lints/common.go | 55 +++++++------- linter/lints/cpcps/lint_crl_has_idp.go | 73 ++++++++++--------- linter/lints/cpcps/lint_crl_has_idp_test.go | 27 +++++-- ...{crl_idp_no_uri.pem => crl_idp_no_dpn.pem} | 0 .../cpcps/testdata/crl_idp_no_fullname.pem | 10 +++ .../lints/cpcps/testdata/crl_idp_no_uris.pem | 10 +++ .../lints/cpcps/testdata/crl_idp_two_uris.pem | 15 ++-- 7 files changed, 114 insertions(+), 76 deletions(-) rename linter/lints/cpcps/testdata/{crl_idp_no_uri.pem => crl_idp_no_dpn.pem} (100%) create mode 100644 linter/lints/cpcps/testdata/crl_idp_no_fullname.pem create mode 100644 linter/lints/cpcps/testdata/crl_idp_no_uris.pem diff --git a/linter/lints/common.go b/linter/lints/common.go index b3a0f0da4fc..4efe482869d 100644 --- a/linter/lints/common.go +++ b/linter/lints/common.go @@ -29,33 +29,36 @@ var ( MozillaPolicy281Date = time.Date(2023, time.February, 15, 0, 0, 0, 0, time.UTC) ) -/* - IssuingDistributionPoint stores the IA5STRING value of the optional - distribution point, and the (implied OPTIONAL) BOOLEAN values of - onlyContainsUserCerts and onlyContainsCACerts. - - RFC 5280 - * Section 5.2.5 - IssuingDistributionPoint ::= SEQUENCE { - distributionPoint [0] DistributionPointName OPTIONAL, - onlyContainsUserCerts [1] BOOLEAN DEFAULT FALSE, - onlyContainsCACerts [2] BOOLEAN DEFAULT FALSE, - ... - } - - * Section 4.2.1.13 - DistributionPointName ::= CHOICE { - fullName [0] GeneralNames, - ... } - - * Appendix A.1, Page 128 - GeneralName ::= CHOICE { - ... - uniformResourceIdentifier [6] IA5String, - ... } -*/ +// IssuingDistributionPoint stores the IA5STRING value(s) of the optional +// distributionPoint, and the (implied OPTIONAL) BOOLEAN values of +// onlyContainsUserCerts and onlyContainsCACerts. +// +// RFC 5280 +// * Section 5.2.5 +// IssuingDistributionPoint ::= SEQUENCE { +// distributionPoint [0] DistributionPointName OPTIONAL, +// onlyContainsUserCerts [1] BOOLEAN DEFAULT FALSE, +// onlyContainsCACerts [2] BOOLEAN DEFAULT FALSE, +// ... +// } +// +// * Section 4.2.1.13 +// DistributionPointName ::= CHOICE { +// fullName [0] GeneralNames, +// ... } +// +// * Appendix A.1, Page 128 +// GeneralNames ::= SEQUENCE SIZE (1..MAX) OF GeneralName +// GeneralName ::= CHOICE { +// ... +// uniformResourceIdentifier [6] IA5String, +// ... } +// +// Because this struct is used by cryptobyte (not by encoding/asn1), and because +// we only care about the uniformResourceIdentifier flavor of GeneralName, we +// are able to flatten the DistributionPointName down into a slice of URIs. type IssuingDistributionPoint struct { - DistributionPointURI *url.URL + DistributionPointURIs []*url.URL OnlyContainsUserCerts bool OnlyContainsCACerts bool } diff --git a/linter/lints/cpcps/lint_crl_has_idp.go b/linter/lints/cpcps/lint_crl_has_idp.go index 1e341ba7a6d..a616c640712 100644 --- a/linter/lints/cpcps/lint_crl_has_idp.go +++ b/linter/lints/cpcps/lint_crl_has_idp.go @@ -92,7 +92,7 @@ func (l *crlHasIDP) Execute(c *x509.RevocationList) *lint.LintResult { idp := lints.NewIssuingDistributionPoint() if distributionPointExists { - lintErr := parseSingleDistributionPointName(&dpName, idp) + lintErr := parseDistributionPointName(&dpName, idp) if lintErr != nil { return lintErr } @@ -121,24 +121,23 @@ func (l *crlHasIDP) Execute(c *x509.RevocationList) *lint.LintResult { } } - if idp.OnlyContainsUserCerts { - if idp.OnlyContainsCACerts { - return &lint.LintResult{ - Status: lint.Error, - Details: "IssuingDistributionPoint should not have both onlyContainsUserCerts: TRUE and onlyContainsCACerts: TRUE", - } + if idp.OnlyContainsUserCerts && idp.OnlyContainsCACerts { + return &lint.LintResult{ + Status: lint.Error, + Details: "IssuingDistributionPoint should not have both onlyContainsUserCerts: TRUE and onlyContainsCACerts: TRUE", } - if idp.DistributionPointURI == nil { + } else if idp.OnlyContainsUserCerts { + if len(idp.DistributionPointURIs) == 0 { return &lint.LintResult{ Status: lint.Error, - Details: "IssuingDistributionPoint should have both DistributionPointName and onlyContainsUserCerts: TRUE", + Details: "User certificate CRLs MUST have at least one DistributionPointName FullName", } } } else if idp.OnlyContainsCACerts { - if idp.DistributionPointURI != nil { + if len(idp.DistributionPointURIs) != 0 { return &lint.LintResult{ Status: lint.Error, - Details: "IssuingDistributionPoint should not have both DistributionPointName and onlyContainsCACerts: TRUE", + Details: "CA certificate CRLs SHOULD NOT have a DistributionPointName FullName", } } } else { @@ -151,47 +150,51 @@ func (l *crlHasIDP) Execute(c *x509.RevocationList) *lint.LintResult { return &lint.LintResult{Status: lint.Pass} } -// parseSingleDistributionPointName examines the provided distributionPointName +// parseDistributionPointName examines the provided distributionPointName // and updates idp with the URI if it is found. The distribution point name is // checked for validity and returns a non-nil LintResult if there were any // problems. -func parseSingleDistributionPointName(distributionPointName *cryptobyte.String, idp *lints.IssuingDistributionPoint) *lint.LintResult { +func parseDistributionPointName(distributionPointName *cryptobyte.String, idp *lints.IssuingDistributionPoint) *lint.LintResult { fullNameTag := cryptobyte_asn1.Tag(0).ContextSpecific().Constructed() if !distributionPointName.ReadASN1(distributionPointName, fullNameTag) { return &lint.LintResult{ - Status: lint.Warn, + Status: lint.Error, Details: "Failed to read IssuingDistributionPoint distributionPoint fullName", } } - var uriBytes []byte - uriTag := cryptobyte_asn1.Tag(6).ContextSpecific() - if !distributionPointName.ReadASN1Bytes(&uriBytes, uriTag) { - return &lint.LintResult{ - Status: lint.Warn, - Details: "Failed to read IssuingDistributionPoint URI", + for !distributionPointName.Empty() { + var uriBytes []byte + uriTag := cryptobyte_asn1.Tag(6).ContextSpecific() + if !distributionPointName.ReadASN1Bytes(&uriBytes, uriTag) { + return &lint.LintResult{ + Status: lint.Error, + Details: "Failed to read IssuingDistributionPoint URI", + } } - } - var err error - idp.DistributionPointURI, err = url.Parse(string(uriBytes)) - if err != nil { - return &lint.LintResult{ - Status: lint.Error, - Details: "Failed to parse IssuingDistributionPoint URI", + uri, err := url.Parse(string(uriBytes)) + if err != nil { + return &lint.LintResult{ + Status: lint.Error, + Details: "Failed to parse IssuingDistributionPoint URI", + } } - } - if idp.DistributionPointURI.Scheme != "http" { - return &lint.LintResult{ - Status: lint.Error, - Details: "IssuingDistributionPoint URI MUST use http scheme", + if uri.Scheme != "http" { + return &lint.LintResult{ + Status: lint.Error, + Details: "IssuingDistributionPoint URI MUST use http scheme", + } } + idp.DistributionPointURIs = append(idp.DistributionPointURIs, uri) } - if !distributionPointName.Empty() { + if len(idp.DistributionPointURIs) == 0 { return &lint.LintResult{ - Status: lint.Warn, - Details: "IssuingDistributionPoint should contain only one distributionPoint", + Status: lint.Error, + Details: "IssuingDistributionPoint FullName URI MUST be present", } } + // TODO(#7296): When we're back to only including one GeneralName within the + // distributionPoint's FullName, re-add a check that len(uris) == 1. return nil } diff --git a/linter/lints/cpcps/lint_crl_has_idp_test.go b/linter/lints/cpcps/lint_crl_has_idp_test.go index e2d94419aef..ff48364b48d 100644 --- a/linter/lints/cpcps/lint_crl_has_idp_test.go +++ b/linter/lints/cpcps/lint_crl_has_idp_test.go @@ -5,8 +5,9 @@ import ( "strings" "testing" - linttest "github.com/letsencrypt/boulder/linter/lints/test" "github.com/zmap/zlint/v3/lint" + + linttest "github.com/letsencrypt/boulder/linter/lints/test" ) func TestCrlHasIDP(t *testing.T) { @@ -30,16 +31,26 @@ func TestCrlHasIDP(t *testing.T) { want: lint.Warn, wantSubStr: "CRL missing IssuingDistributionPoint", }, - { - name: "idp_no_uri", + name: "idp_no_dpn", want: lint.Error, - wantSubStr: "IssuingDistributionPoint should have both DistributionPointName and onlyContainsUserCerts: TRUE", + wantSubStr: "User certificate CRLs MUST have at least one DistributionPointName FullName", }, { - name: "idp_two_uris", - want: lint.Warn, - wantSubStr: "IssuingDistributionPoint should contain only one distributionPoint", + name: "idp_no_fullname", + want: lint.Error, + wantSubStr: "Failed to read IssuingDistributionPoint distributionPoint fullName", + }, + { + name: "idp_no_uris", + want: lint.Error, + wantSubStr: "IssuingDistributionPoint FullName URI MUST be present", + }, + { + // TODO(#7296): When we're back to only including one GeneralName within + // the distributionPoint's FullName, change this to expect a lint.Notice. + name: "idp_two_uris", + want: lint.Pass, }, { name: "idp_https", @@ -59,7 +70,7 @@ func TestCrlHasIDP(t *testing.T) { { name: "idp_distributionPoint_and_onlyCA", want: lint.Error, - wantSubStr: "IssuingDistributionPoint should not have both DistributionPointName and onlyContainsCACerts: TRUE", + wantSubStr: "CA certificate CRLs SHOULD NOT have a DistributionPointName FullName", }, { name: "idp_distributionPoint_and_onlyUser_and_onlyCA", diff --git a/linter/lints/cpcps/testdata/crl_idp_no_uri.pem b/linter/lints/cpcps/testdata/crl_idp_no_dpn.pem similarity index 100% rename from linter/lints/cpcps/testdata/crl_idp_no_uri.pem rename to linter/lints/cpcps/testdata/crl_idp_no_dpn.pem diff --git a/linter/lints/cpcps/testdata/crl_idp_no_fullname.pem b/linter/lints/cpcps/testdata/crl_idp_no_fullname.pem new file mode 100644 index 00000000000..036dbbca035 --- /dev/null +++ b/linter/lints/cpcps/testdata/crl_idp_no_fullname.pem @@ -0,0 +1,10 @@ +-----BEGIN X509 CRL----- +MIIBZjCB7gIBATAKBggqhkjOPQQDAzBJMQswCQYDVQQGEwJYWDEVMBMGA1UEChMM +Qm91bGRlciBUZXN0MSMwIQYDVQQDExooVEVTVCkgRWxlZ2FudCBFbGVwaGFudCBF +MRcNMjIxMDEwMjAxMjA3WhcNMjIxMDE5MjAxMjA2WjApMCcCCAOuUdtRFVo8Fw0y +MjEwMTAxOTEyMDdaMAwwCgYDVR0VBAMKAQGgSTBHMB8GA1UdIwQYMBaAFAHau3rL +JSCOXnnW+ZZCLwJBKQe+MBEGA1UdFAQKAggXHM495IK6YTARBgNVHRwBAf8EBzAF +oACBAf8wCgYIKoZIzj0EAwMDZwAwZAIwLw5CFJ02GryeofOtOjA8hYdrsP3BfjlZ +zOLBPJ1HRstF2INIRn20eDGPpEcUUGsiAjBXkD6XKSK9M4F6T+pVF1NK3C0r2rbm +nkEla9uJxo2ez8tL3PeMNJ+4nWfMI3ufvzM= +-----END X509 CRL----- diff --git a/linter/lints/cpcps/testdata/crl_idp_no_uris.pem b/linter/lints/cpcps/testdata/crl_idp_no_uris.pem new file mode 100644 index 00000000000..117d36bda45 --- /dev/null +++ b/linter/lints/cpcps/testdata/crl_idp_no_uris.pem @@ -0,0 +1,10 @@ +-----BEGIN X509 CRL----- +MIIBaDCB8AIBATAKBggqhkjOPQQDAzBJMQswCQYDVQQGEwJYWDEVMBMGA1UEChMM +Qm91bGRlciBUZXN0MSMwIQYDVQQDExooVEVTVCkgRWxlZ2FudCBFbGVwaGFudCBF +MRcNMjIxMDEwMjAxMjA3WhcNMjIxMDE5MjAxMjA2WjApMCcCCAOuUdtRFVo8Fw0y +MjEwMTAxOTEyMDdaMAwwCgYDVR0VBAMKAQGgSzBJMB8GA1UdIwQYMBaAFAHau3rL +JSCOXnnW+ZZCLwJBKQe+MBEGA1UdFAQKAggXHM495IK6YTATBgNVHRwBAf8ECTAH +oAKgAIEB/zAKBggqhkjOPQQDAwNnADBkAjAvDkIUnTYavJ6h8606MDyFh2uw/cF+ +OVnM4sE8nUdGy0XYg0hGfbR4MY+kRxRQayICMFeQPpcpIr0zgXpP6lUXU0rcLSva +tuaeQSVr24nGjZ7Py0vc94w0n7idZ8wje5+/Mw== +-----END X509 CRL----- diff --git a/linter/lints/cpcps/testdata/crl_idp_two_uris.pem b/linter/lints/cpcps/testdata/crl_idp_two_uris.pem index 3b7971fe6ec..4294a25267b 100644 --- a/linter/lints/cpcps/testdata/crl_idp_two_uris.pem +++ b/linter/lints/cpcps/testdata/crl_idp_two_uris.pem @@ -1,11 +1,12 @@ -----BEGIN X509 CRL----- -MIIBmDCCAR8CAQEwCgYIKoZIzj0EAwMwSTELMAkGA1UEBhMCWFgxFTATBgNVBAoT +MIIByTCCAVACAQEwCgYIKoZIzj0EAwMwSTELMAkGA1UEBhMCWFgxFTATBgNVBAoT DEJvdWxkZXIgVGVzdDEjMCEGA1UEAxMaKFRFU1QpIEVsZWdhbnQgRWxlcGhhbnQg RTEXDTIyMTAxMDIwMTIwN1oXDTIyMTAxOTIwMTIwNlowKTAnAggDrlHbURVaPBcN -MjIxMDEwMTkxMjA3WjAMMAoGA1UdFQQDCgEBoHoweDAfBgNVHSMEGDAWgBQB2rt6 -yyUgjl551vmWQi8CQSkHvjARBgNVHRQECgIIFxzOPeSCumEwQgYDVR0cAQH/BDgw -NqAxoC+GKmh0dHA6Ly9jLmJvdWxkZXIudGVzdC82NjI4Mzc1NjkxMzU4ODI4OC8w -LoYBbIEB/zAKBggqhkjOPQQDAwNnADBkAjAvDkIUnTYavJ6h8606MDyFh2uw/cF+ -OVnM4sE8nUdGy0XYg0hGfbR4MY+kRxRQayICMFeQPpcpIr0zgXpP6lUXU0rcLSva -tuaeQSVr24nGjZ7Py0vc94w0n7idZ8wje5+/Mw== +MjIxMDEwMTkxMjA3WjAMMAoGA1UdFQQDCgEBoIGqMIGnMB8GA1UdIwQYMBaAFAHa +u3rLJSCOXnnW+ZZCLwJBKQe+MBEGA1UdFAQKAggXHM495IK6YTBxBgNVHRwBAf8E +ZzBloGCgXoYtaHR0cDovL2MuYm91bGRlci50ZXN0LzY2MjgzNzU2OTEzNTg4Mjg4 +LzAuY3Jshi1odHRwOi8vYy5ib3VsZGVyLnRlc3QvNjYyODM3NTY5MTM1ODgyODgv +MS5jcmyBAf8wCgYIKoZIzj0EAwMDZwAwZAIwLw5CFJ02GryeofOtOjA8hYdrsP3B +fjlZzOLBPJ1HRstF2INIRn20eDGPpEcUUGsiAjBXkD6XKSK9M4F6T+pVF1NK3C0r +2rbmnkEla9uJxo2ez8tL3PeMNJ+4nWfMI3ufvzM= -----END X509 CRL-----