Improve how cert-checker runs lints (#8063)

Give cert-checker the ability to load zlint configs, so that it can be
configured to talk to PKIMetal in CI and hopefully in staging/production
in the future.

Also update how cert-checker executes lints, so that it uses a real lint
registry instead of using the global registry and passing around a
dictionary of lints to filter out of the results.

Fixes #7786

Author: aarongable

cmd/cert-checker/main.go

@@ -29,7 +29,7 @@ import (
 	"github.com/letsencrypt/boulder/features"
 	"github.com/letsencrypt/boulder/goodkey"
 	"github.com/letsencrypt/boulder/goodkey/sagoodkey"
-	_ "github.com/letsencrypt/boulder/linter"
+	"github.com/letsencrypt/boulder/linter"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/policy"
 	"github.com/letsencrypt/boulder/precert"
@@ -105,6 +105,7 @@ type certChecker struct {
 	issuedReport                report
 	checkPeriod                 time.Duration
 	acceptableValidityDurations map[time.Duration]bool
+	lints                       lint.Registry
 	logger                      blog.Logger
 }
 
@@ -114,6 +115,7 @@ func newChecker(saDbMap certDB,
 	kp goodkey.KeyPolicy,
 	period time.Duration,
 	avd map[time.Duration]bool,
+	lints lint.Registry,
 	logger blog.Logger,
 ) certChecker {
 	precertGetter := func(ctx context.Context, serial string) ([]byte, error) {
@@ -134,6 +136,7 @@ func newChecker(saDbMap certDB,
 		issuedReport:                report{Entries: make(map[string]reportEntry)},
 		checkPeriod:                 period,
 		acceptableValidityDurations: avd,
+		lints:                       lints,
 		logger:                      logger,
 	}
 }
@@ -252,9 +255,9 @@ func (c *certChecker) getCerts(ctx context.Context) error {
 	return nil
 }
 
-func (c *certChecker) processCerts(ctx context.Context, wg *sync.WaitGroup, badResultsOnly bool, ignoredLints map[string]bool) {
+func (c *certChecker) processCerts(ctx context.Context, wg *sync.WaitGroup, badResultsOnly bool) {
 	for cert := range c.certs {
-		dnsNames, problems := c.checkCert(ctx, cert, ignoredLints)
+		dnsNames, problems := c.checkCert(ctx, cert)
 		valid := len(problems) == 0
 		c.rMu.Lock()
 		if !badResultsOnly || (badResultsOnly && !valid) {
@@ -330,7 +333,7 @@ func (c *certChecker) checkValidations(ctx context.Context, cert core.Certificat
 }
 
 // checkCert returns a list of DNS names in the certificate and a list of problems with the certificate.
-func (c *certChecker) checkCert(ctx context.Context, cert core.Certificate, ignoredLints map[string]bool) ([]string, []string) {
+func (c *certChecker) checkCert(ctx context.Context, cert core.Certificate) ([]string, []string) {
 	var dnsNames []string
 	var problems []string
 
@@ -345,9 +348,9 @@ func (c *certChecker) checkCert(ctx context.Context, cert core.Certificate, igno
 	} else {
 		dnsNames = parsedCert.DNSNames
 		// Run zlint checks.
-		results := zlint.LintCertificate(parsedCert)
+		results := zlint.LintCertificateEx(parsedCert, c.lints)
 		for name, res := range results.Results {
-			if ignoredLints[name] || res.Status <= lint.Pass {
+			if res.Status <= lint.Pass {
 				continue
 			}
 			prob := fmt.Sprintf("zlint %s: %s", res.Status, name)
@@ -502,6 +505,9 @@ type Config struct {
 		// public keys in the certs it checks.
 		GoodKey goodkey.Config
 
+		// LintConfig is a path to a zlint config file, which can be used to control
+		// the behavior of zlint's "customizable lints".
+		LintConfig string
 		// IgnoredLints is a list of zlint names. Any lint results from a lint in
 		// the IgnoredLists list are ignored regardless of LintStatus level.
 		IgnoredLints []string
@@ -572,22 +578,26 @@ func main() {
 		cmd.FailOnError(err, "Failed to load CT Log List")
 	}
 
+	lints, err := linter.NewRegistry(config.CertChecker.IgnoredLints)
+	cmd.FailOnError(err, "Failed to create zlint registry")
+	if config.CertChecker.LintConfig != "" {
+		lintconfig, err := lint.NewConfigFromFile(config.CertChecker.LintConfig)
+		cmd.FailOnError(err, "Failed to load zlint config file")
+		lints.SetConfiguration(lintconfig)
+	}
+
 	checker := newChecker(
 		saDbMap,
 		cmd.Clock(),
 		pa,
 		kp,
 		config.CertChecker.CheckPeriod.Duration,
 		acceptableValidityDurations,
+		lints,
 		logger,
 	)
 	fmt.Fprintf(os.Stderr, "# Getting certificates issued in the last %s\n", config.CertChecker.CheckPeriod)
 
-	ignoredLintsMap := make(map[string]bool)
-	for _, name := range config.CertChecker.IgnoredLints {
-		ignoredLintsMap[name] = true
-	}
-
 	// Since we grab certificates in batches we don't want this to block, when it
 	// is finished it will close the certificate channel which allows the range
 	// loops in checker.processCerts to break
@@ -602,7 +612,7 @@ func main() {
 		wg.Add(1)
 		go func() {
 			s := checker.clock.Now()
-			checker.processCerts(context.TODO(), wg, config.CertChecker.BadResultsOnly, ignoredLintsMap)
+			checker.processCerts(context.TODO(), wg, config.CertChecker.BadResultsOnly)
 			checkerLatency.Observe(checker.clock.Since(s).Seconds())
 		}()
 	}

cmd/cert-checker/main_test.go

@@ -30,6 +30,7 @@ import (
 	"github.com/letsencrypt/boulder/ctpolicy/loglist"
 	"github.com/letsencrypt/boulder/goodkey"
 	"github.com/letsencrypt/boulder/goodkey/sagoodkey"
+	"github.com/letsencrypt/boulder/linter"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 	"github.com/letsencrypt/boulder/policy"
@@ -65,7 +66,7 @@ func init() {
 }
 
 func BenchmarkCheckCert(b *testing.B) {
-	checker := newChecker(nil, clock.New(), pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(nil, clock.New(), pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 	testKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	expiry := time.Now().AddDate(0, 0, 1)
 	serial := big.NewInt(1337)
@@ -87,7 +88,7 @@ func BenchmarkCheckCert(b *testing.B) {
 	}
 	b.ResetTimer()
 	for range b.N {
-		checker.checkCert(context.Background(), cert, nil)
+		checker.checkCert(context.Background(), cert)
 	}
 }
 
@@ -101,7 +102,7 @@ func TestCheckWildcardCert(t *testing.T) {
 
 	testKey, _ := rsa.GenerateKey(rand.Reader, 2048)
 	fc := clock.NewFake()
-	checker := newChecker(saDbMap, fc, pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(saDbMap, fc, pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 	issued := checker.clock.Now().Add(-time.Minute)
 	goodExpiry := issued.Add(testValidityDuration - time.Second)
 	serial := big.NewInt(1337)
@@ -131,7 +132,7 @@ func TestCheckWildcardCert(t *testing.T) {
 		Issued:  parsed.NotBefore,
 		DER:     wildcardCertDer,
 	}
-	_, problems := checker.checkCert(context.Background(), cert, nil)
+	_, problems := checker.checkCert(context.Background(), cert)
 	for _, p := range problems {
 		t.Error(p)
 	}
@@ -144,7 +145,7 @@ func TestCheckCertReturnsDNSNames(t *testing.T) {
 	defer func() {
 		saCleanup()
 	}()
-	checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 
 	certPEM, err := os.ReadFile("testdata/quite_invalid.pem")
 	if err != nil {
@@ -164,7 +165,7 @@ func TestCheckCertReturnsDNSNames(t *testing.T) {
 		DER:     block.Bytes,
 	}
 
-	names, problems := checker.checkCert(context.Background(), cert, nil)
+	names, problems := checker.checkCert(context.Background(), cert)
 	if !slices.Equal(names, []string{"quite_invalid.com", "al--so--wr--ong.com"}) {
 		t.Errorf("didn't get expected DNS names. other problems: %s", strings.Join(problems, "\n"))
 	}
@@ -211,7 +212,7 @@ func TestCheckCert(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			testKey, _ := tc.key.genKey()
 
-			checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+			checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 
 			// Create a RFC 7633 OCSP Must Staple Extension.
 			// OID 1.3.6.1.5.5.7.1.24
@@ -268,7 +269,7 @@ func TestCheckCert(t *testing.T) {
 				Expires: goodExpiry.AddDate(0, 0, 2), // Expiration doesn't match
 			}
 
-			_, problems := checker.checkCert(context.Background(), cert, nil)
+			_, problems := checker.checkCert(context.Background(), cert)
 
 			problemsMap := map[string]int{
 				"Stored digest doesn't match certificate digest":                            1,
@@ -295,7 +296,7 @@ func TestCheckCert(t *testing.T) {
 
 			// Same settings as above, but the stored serial number in the DB is invalid.
 			cert.Serial = "not valid"
-			_, problems = checker.checkCert(context.Background(), cert, nil)
+			_, problems = checker.checkCert(context.Background(), cert)
 			foundInvalidSerialProblem := false
 			for _, p := range problems {
 				if p == "Stored serial is invalid" {
@@ -320,7 +321,7 @@ func TestCheckCert(t *testing.T) {
 			cert.DER = goodCertDer
 			cert.Expires = parsed.NotAfter
 			cert.Issued = parsed.NotBefore
-			_, problems = checker.checkCert(context.Background(), cert, nil)
+			_, problems = checker.checkCert(context.Background(), cert)
 			test.AssertEquals(t, len(problems), 0)
 		})
 	}
@@ -332,7 +333,7 @@ func TestGetAndProcessCerts(t *testing.T) {
 	fc := clock.NewFake()
 	fc.Set(fc.Now().Add(time.Hour))
 
-	checker := newChecker(saDbMap, fc, pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(saDbMap, fc, pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 	sa, err := sa.NewSQLStorageAuthority(saDbMap, saDbMap, nil, 1, 0, fc, blog.NewMock(), metrics.NoopRegisterer)
 	test.AssertNotError(t, err, "Couldn't create SA to insert certificates")
 	saCleanUp := test.ResetBoulderTestDatabase(t)
@@ -371,7 +372,7 @@ func TestGetAndProcessCerts(t *testing.T) {
 	test.AssertEquals(t, len(checker.certs), 5)
 	wg := new(sync.WaitGroup)
 	wg.Add(1)
-	checker.processCerts(context.Background(), wg, false, nil)
+	checker.processCerts(context.Background(), wg, false)
 	test.AssertEquals(t, checker.issuedReport.BadCerts, int64(5))
 	test.AssertEquals(t, len(checker.issuedReport.Entries), 5)
 }
@@ -426,7 +427,7 @@ func (db mismatchedCountDB) SelectOne(_ context.Context, _ interface{}, _ string
 func TestGetCertsEmptyResults(t *testing.T) {
 	saDbMap, err := sa.DBMapForTest(vars.DBConnSA)
 	test.AssertNotError(t, err, "Couldn't connect to database")
-	checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 	checker.dbMap = mismatchedCountDB{}
 
 	batchSize = 3
@@ -452,7 +453,7 @@ func (db emptyDB) SelectNullInt(_ context.Context, _ string, _ ...interface{}) (
 // expected if the DB finds no certificates to match the SELECT query and
 // should return an error.
 func TestGetCertsNullResults(t *testing.T) {
-	checker := newChecker(emptyDB{}, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(emptyDB{}, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 
 	err := checker.getCerts(context.Background())
 	test.AssertError(t, err, "Should have gotten error from empty DB")
@@ -496,7 +497,7 @@ func TestGetCertsLate(t *testing.T) {
 	clk := clock.NewFake()
 	db := &lateDB{issuedTime: clk.Now().Add(-time.Hour)}
 	checkPeriod := 24 * time.Hour
-	checker := newChecker(db, clk, pa, kp, checkPeriod, testValidityDurations, blog.NewMock())
+	checker := newChecker(db, clk, pa, kp, checkPeriod, testValidityDurations, nil, blog.NewMock())
 
 	err := checker.getCerts(context.Background())
 	test.AssertNotError(t, err, "getting certs")
@@ -581,7 +582,7 @@ func TestIgnoredLint(t *testing.T) {
 	err = loglist.InitLintList("../../test/ct-test-srv/log_list.json")
 	test.AssertNotError(t, err, "failed to load ct log list")
 	testKey, _ := rsa.GenerateKey(rand.Reader, 2048)
-	checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(saDbMap, clock.NewFake(), pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 	serial := big.NewInt(1337)
 
 	x509OID, err := x509.OIDFromInts([]uint64{1, 2, 3})
@@ -643,23 +644,26 @@ func TestIgnoredLint(t *testing.T) {
 
 	// Check the certificate with a nil ignore map. This should return the
 	// expected zlint problems.
-	_, problems := checker.checkCert(context.Background(), cert, nil)
+	_, problems := checker.checkCert(context.Background(), cert)
 	slices.Sort(problems)
 	test.AssertDeepEquals(t, problems, expectedProblems)
 
 	// Check the certificate again with an ignore map that excludes the affected
 	// lints. This should return no problems.
-	_, problems = checker.checkCert(context.Background(), cert, map[string]bool{
-		"w_subject_common_name_included":                          true,
-		"w_ext_subject_key_identifier_not_recommended_subscriber": true,
-		"w_ct_sct_policy_count_unsatisfied":                       true,
-		"e_scts_from_same_operator":                               true,
+	lints, err := linter.NewRegistry([]string{
+		"w_subject_common_name_included",
+		"w_ext_subject_key_identifier_not_recommended_subscriber",
+		"w_ct_sct_policy_count_unsatisfied",
+		"e_scts_from_same_operator",
 	})
+	test.AssertNotError(t, err, "creating test lint registry")
+	checker.lints = lints
+	_, problems = checker.checkCert(context.Background(), cert)
 	test.AssertEquals(t, len(problems), 0)
 }
 
 func TestPrecertCorrespond(t *testing.T) {
-	checker := newChecker(nil, clock.New(), pa, kp, time.Hour, testValidityDurations, blog.NewMock())
+	checker := newChecker(nil, clock.New(), pa, kp, time.Hour, testValidityDurations, nil, blog.NewMock())
 	checker.getPrecert = func(_ context.Context, _ string) ([]byte, error) {
 		return []byte("hello"), nil
 	}
@@ -682,7 +686,7 @@ func TestPrecertCorrespond(t *testing.T) {
 		Issued:  time.Now(),
 		Expires: expiry,
 	}
-	_, problems := checker.checkCert(context.Background(), cert, nil)
+	_, problems := checker.checkCert(context.Background(), cert)
 	if len(problems) == 0 {
 		t.Errorf("expected precert correspondence problem")
 	}

test/config-next/cert-checker.json

@@ -12,6 +12,7 @@
 		"acceptableValidityDurations": [
 			"7776000s"
 		],
+		"lintConfig": "test/config-next/zlint.toml",
 		"ignoredLints": [
 			"w_subject_common_name_included",
 			"w_ext_subject_key_identifier_missing_sub_cert",