Improve CRL IssuingDistributionPoint lint (letsencrypt#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 letsencrypt#7296
orangepizza · Feb 8, 2024 · ef1e9a3 · ef1e9a3
1 parent f10abd2
commit ef1e9a3
Show file tree

Hide file tree

Showing 7 changed files with 114 additions and 76 deletions.
diff --git 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
@@ -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
@@ -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/...r/lints/cpcps/testdata/crl_idp_no_uri.pem → ...r/lints/cpcps/testdata/crl_idp_no_dpn.pem b/...r/lints/cpcps/testdata/crl_idp_no_uri.pem → ...r/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
@@ -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
@@ -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
@@ -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-----