From 767c5d168bc98544d54fee61c8172a069dc07e3b Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Thu, 13 Mar 2025 18:35:09 -0500 Subject: [PATCH] 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 https://github.com/letsencrypt/boulder/issues/7786 --- cmd/cert-checker/main.go | 34 ++++++++++++------- cmd/cert-checker/main_test.go | 52 ++++++++++++++++-------------- test/config-next/cert-checker.json | 1 + 3 files changed, 51 insertions(+), 36 deletions(-) diff --git a/cmd/cert-checker/main.go b/cmd/cert-checker/main.go index 883378779..909da4ff6 100644 --- a/cmd/cert-checker/main.go +++ b/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,6 +578,14 @@ 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(), @@ -579,15 +593,11 @@ func main() { 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()) }() } diff --git a/cmd/cert-checker/main_test.go b/cmd/cert-checker/main_test.go index e6a53f4c3..4cfc10019 100644 --- a/cmd/cert-checker/main_test.go +++ b/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") } diff --git a/test/config-next/cert-checker.json b/test/config-next/cert-checker.json index a4b760e20..723912baf 100644 --- a/test/config-next/cert-checker.json +++ b/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",