From ac2dae70f223bec42776a84218f242f97c2c4646 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Fri, 16 May 2025 16:22:56 -0700 Subject: [PATCH] cert-checker: add support for ipAddress SANs (#8188) In cert-checker, inspect both the DNS Names and the IP Addresses contained within the certificate being examined. Also add a check that no other kinds of SANs exist in the certificate. Fixes https://github.com/letsencrypt/boulder/issues/8183 --- cmd/cert-checker/main.go | 293 +++++++++++--------- cmd/cert-checker/main_test.go | 4 +- cmd/cert-checker/testdata/quite_invalid.pem | 20 +- 3 files changed, 175 insertions(+), 142 deletions(-) diff --git a/cmd/cert-checker/main.go b/cmd/cert-checker/main.go index 974308ab9..a323e70b8 100644 --- a/cmd/cert-checker/main.go +++ b/cmd/cert-checker/main.go @@ -8,6 +8,7 @@ import ( "encoding/json" "flag" "fmt" + "net/netip" "os" "regexp" "slices" @@ -78,7 +79,7 @@ func (r *report) dump() error { type reportEntry struct { Valid bool `json:"valid"` - DNSNames []string `json:"dnsNames"` + SANs []string `json:"sans"` Problems []string `json:"problems,omitempty"` } @@ -258,13 +259,13 @@ func (c *certChecker) getCerts(ctx context.Context) error { func (c *certChecker) processCerts(ctx context.Context, wg *sync.WaitGroup, badResultsOnly bool) { for cert := range c.certs { - dnsNames, problems := c.checkCert(ctx, cert) + sans, problems := c.checkCert(ctx, cert) valid := len(problems) == 0 c.rMu.Lock() if !badResultsOnly || (badResultsOnly && !valid) { c.issuedReport.Entries[cert.Serial] = reportEntry{ Valid: valid, - DNSNames: dnsNames, + SANs: sans, Problems: problems, } } @@ -333,164 +334,196 @@ func (c *certChecker) checkValidations(ctx context.Context, cert *corepb.Certifi return nil } -// checkCert returns a list of DNS names in the certificate and a list of problems with the certificate. +// checkCert returns a list of Subject Alternative Names in the certificate and a list of problems with the certificate. func (c *certChecker) checkCert(ctx context.Context, cert *corepb.Certificate) ([]string, []string) { - var dnsNames []string var problems []string // Check that the digests match. if cert.Digest != core.Fingerprint256(cert.Der) { problems = append(problems, "Stored digest doesn't match certificate digest") } + // Parse the certificate. parsedCert, err := zX509.ParseCertificate(cert.Der) if err != nil { problems = append(problems, fmt.Sprintf("Couldn't parse stored certificate: %s", err)) - } else { - dnsNames = parsedCert.DNSNames - // Run zlint checks. - results := zlint.LintCertificateEx(parsedCert, c.lints) - for name, res := range results.Results { - if res.Status <= lint.Pass { - continue - } - prob := fmt.Sprintf("zlint %s: %s", res.Status, name) - if res.Details != "" { - prob = fmt.Sprintf("%s %s", prob, res.Details) - } - problems = append(problems, prob) + // This is a fatal error, we can't do any further processing. + return nil, problems + } + + // Now that it's parsed, we can extract the SANs. + sans := slices.Clone(parsedCert.DNSNames) + for _, ip := range parsedCert.IPAddresses { + sans = append(sans, ip.String()) + } + + // Run zlint checks. + results := zlint.LintCertificateEx(parsedCert, c.lints) + for name, res := range results.Results { + if res.Status <= lint.Pass { + continue } - // Check if stored serial is correct. - storedSerial, err := core.StringToSerial(cert.Serial) + prob := fmt.Sprintf("zlint %s: %s", res.Status, name) + if res.Details != "" { + prob = fmt.Sprintf("%s %s", prob, res.Details) + } + problems = append(problems, prob) + } + + // Check if stored serial is correct. + storedSerial, err := core.StringToSerial(cert.Serial) + if err != nil { + problems = append(problems, "Stored serial is invalid") + } else if parsedCert.SerialNumber.Cmp(storedSerial) != 0 { + problems = append(problems, "Stored serial doesn't match certificate serial") + } + + // Check that we have the correct expiration time. + if !parsedCert.NotAfter.Equal(cert.Expires.AsTime()) { + problems = append(problems, "Stored expiration doesn't match certificate NotAfter") + } + + // Check if basic constraints are set. + if !parsedCert.BasicConstraintsValid { + problems = append(problems, "Certificate doesn't have basic constraints set") + } + + // Check that the cert isn't able to sign other certificates. + if parsedCert.IsCA { + problems = append(problems, "Certificate can sign other certificates") + } + + // Check that the cert has a valid validity period. The validity + // period is computed inclusive of the whole final second indicated by + // notAfter. + validityDuration := parsedCert.NotAfter.Add(time.Second).Sub(parsedCert.NotBefore) + _, ok := c.acceptableValidityDurations[validityDuration] + if !ok { + problems = append(problems, "Certificate has unacceptable validity period") + } + + // Check that the stored issuance time isn't too far back/forward dated. + if parsedCert.NotBefore.Before(cert.Issued.AsTime().Add(-6*time.Hour)) || parsedCert.NotBefore.After(cert.Issued.AsTime().Add(6*time.Hour)) { + problems = append(problems, "Stored issuance date is outside of 6 hour window of certificate NotBefore") + } + + // Check that the cert doesn't contain any SANs of unexpected types. + if len(parsedCert.EmailAddresses) != 0 || len(parsedCert.URIs) != 0 { + problems = append(problems, "Certificate contains SAN of unacceptable type (email or URI)") + } + + if parsedCert.Subject.CommonName != "" { + // Check if the CommonName is <= 64 characters. + if len(parsedCert.Subject.CommonName) > 64 { + problems = append( + problems, + fmt.Sprintf("Certificate has common name >64 characters long (%d)", len(parsedCert.Subject.CommonName)), + ) + } + + // Check that the CommonName is included in the SANs. + if !slices.Contains(sans, parsedCert.Subject.CommonName) { + problems = append(problems, fmt.Sprintf("Certificate Common Name does not appear in Subject Alternative Names: %q !< %v", + parsedCert.Subject.CommonName, parsedCert.DNSNames)) + } + } + + // Check that the PA is still willing to issue for each DNS name and IP + // address in the SANs. We do not check the CommonName here, as (if it exists) + // we already checked that it is identical to one of the DNSNames in the SAN. + for _, name := range parsedCert.DNSNames { + err = c.pa.WillingToIssue(identifier.ACMEIdentifiers{identifier.NewDNS(name)}) if err != nil { - problems = append(problems, "Stored serial is invalid") - } else if parsedCert.SerialNumber.Cmp(storedSerial) != 0 { - problems = append(problems, "Stored serial doesn't match certificate serial") + problems = append(problems, fmt.Sprintf("Policy Authority isn't willing to issue for '%s': %s", name, err)) + continue } - // Check that we have the correct expiration time. - if !parsedCert.NotAfter.Equal(cert.Expires.AsTime()) { - problems = append(problems, "Stored expiration doesn't match certificate NotAfter") + // For defense-in-depth, even if the PA was willing to issue for a name + // we double check it against a list of forbidden domains. This way even + // if the hostnamePolicyFile malfunctions we will flag the forbidden + // domain matches + if forbidden, pattern := isForbiddenDomain(name); forbidden { + problems = append(problems, fmt.Sprintf( + "Policy Authority was willing to issue but domain '%s' matches "+ + "forbiddenDomains entry %q", name, pattern)) } - // Check if basic constraints are set. - if !parsedCert.BasicConstraintsValid { - problems = append(problems, "Certificate doesn't have basic constraints set") - } - // Check that the cert isn't able to sign other certificates. - if parsedCert.IsCA { - problems = append(problems, "Certificate can sign other certificates") - } - // Check that the cert has a valid validity period. The validity - // period is computed inclusive of the whole final second indicated by - // notAfter. - validityDuration := parsedCert.NotAfter.Add(time.Second).Sub(parsedCert.NotBefore) - _, ok := c.acceptableValidityDurations[validityDuration] + } + for _, name := range parsedCert.IPAddresses { + ip, ok := netip.AddrFromSlice(name) if !ok { - problems = append(problems, "Certificate has unacceptable validity period") + problems = append(problems, fmt.Sprintf("SANs contain malformed IP %q", name)) + continue } - // Check that the stored issuance time isn't too far back/forward dated. - if parsedCert.NotBefore.Before(cert.Issued.AsTime().Add(-6*time.Hour)) || parsedCert.NotBefore.After(cert.Issued.AsTime().Add(6*time.Hour)) { - problems = append(problems, "Stored issuance date is outside of 6 hour window of certificate NotBefore") - } - if parsedCert.Subject.CommonName != "" { - // Check if the CommonName is <= 64 characters. - if len(parsedCert.Subject.CommonName) > 64 { - problems = append( - problems, - fmt.Sprintf("Certificate has common name >64 characters long (%d)", len(parsedCert.Subject.CommonName)), - ) - } - - // Check that the CommonName is included in the SANs. - if !slices.Contains(parsedCert.DNSNames, parsedCert.Subject.CommonName) { - problems = append(problems, fmt.Sprintf("Certificate Common Name does not appear in Subject Alternative Names: %q !< %v", - parsedCert.Subject.CommonName, parsedCert.DNSNames)) - } - } - // Check that the PA is still willing to issue for each name in DNSNames. - // We do not check the CommonName here, as (if it exists) we already checked - // that it is identical to one of the DNSNames in the SAN. - // - // TODO(#7311): We'll need to iterate over IP address identifiers too. - for _, name := range parsedCert.DNSNames { - err = c.pa.WillingToIssue(identifier.ACMEIdentifiers{identifier.NewDNS(name)}) - if err != nil { - problems = append(problems, fmt.Sprintf("Policy Authority isn't willing to issue for '%s': %s", name, err)) - } else { - // For defense-in-depth, even if the PA was willing to issue for a name - // we double check it against a list of forbidden domains. This way even - // if the hostnamePolicyFile malfunctions we will flag the forbidden - // domain matches - if forbidden, pattern := isForbiddenDomain(name); forbidden { - problems = append(problems, fmt.Sprintf( - "Policy Authority was willing to issue but domain '%s' matches "+ - "forbiddenDomains entry %q", name, pattern)) - } - } - } - // Check the cert has the correct key usage extensions - serverAndClient := slices.Equal(parsedCert.ExtKeyUsage, []zX509.ExtKeyUsage{zX509.ExtKeyUsageServerAuth, zX509.ExtKeyUsageClientAuth}) - serverOnly := slices.Equal(parsedCert.ExtKeyUsage, []zX509.ExtKeyUsage{zX509.ExtKeyUsageServerAuth}) - if !(serverAndClient || serverOnly) { - problems = append(problems, "Certificate has incorrect key usage extensions") - } - - for _, ext := range parsedCert.Extensions { - _, ok := allowedExtensions[ext.Id.String()] - if !ok { - problems = append(problems, fmt.Sprintf("Certificate contains an unexpected extension: %s", ext.Id)) - } - expectedContent, ok := expectedExtensionContent[ext.Id.String()] - if ok { - if !bytes.Equal(ext.Value, expectedContent) { - problems = append(problems, fmt.Sprintf("Certificate extension %s contains unexpected content: has %x, expected %x", ext.Id, ext.Value, expectedContent)) - } - } - } - - // Check that the cert has a good key. Note that this does not perform - // checks which rely on external resources such as weak or blocked key - // lists, or the list of blocked keys in the database. This only performs - // static checks, such as against the RSA key size and the ECDSA curve. - p, err := x509.ParseCertificate(cert.Der) + err = c.pa.WillingToIssue(identifier.ACMEIdentifiers{identifier.NewIP(ip)}) if err != nil { - problems = append(problems, fmt.Sprintf("Couldn't parse stored certificate: %s", err)) + problems = append(problems, fmt.Sprintf("Policy Authority isn't willing to issue for '%s': %s", name, err)) + continue } + } + + // Check the cert has the correct key usage extensions + serverAndClient := slices.Equal(parsedCert.ExtKeyUsage, []zX509.ExtKeyUsage{zX509.ExtKeyUsageServerAuth, zX509.ExtKeyUsageClientAuth}) + serverOnly := slices.Equal(parsedCert.ExtKeyUsage, []zX509.ExtKeyUsage{zX509.ExtKeyUsageServerAuth}) + if !(serverAndClient || serverOnly) { + problems = append(problems, "Certificate has incorrect key usage extensions") + } + + for _, ext := range parsedCert.Extensions { + _, ok := allowedExtensions[ext.Id.String()] + if !ok { + problems = append(problems, fmt.Sprintf("Certificate contains an unexpected extension: %s", ext.Id)) + } + expectedContent, ok := expectedExtensionContent[ext.Id.String()] + if ok { + if !bytes.Equal(ext.Value, expectedContent) { + problems = append(problems, fmt.Sprintf("Certificate extension %s contains unexpected content: has %x, expected %x", ext.Id, ext.Value, expectedContent)) + } + } + } + + // Check that the cert has a good key. Note that this does not perform + // checks which rely on external resources such as weak or blocked key + // lists, or the list of blocked keys in the database. This only performs + // static checks, such as against the RSA key size and the ECDSA curve. + p, err := x509.ParseCertificate(cert.Der) + if err != nil { + problems = append(problems, fmt.Sprintf("Couldn't parse stored certificate: %s", err)) + } else { err = c.kp.GoodKey(ctx, p.PublicKey) if err != nil { problems = append(problems, fmt.Sprintf("Key Policy isn't willing to issue for public key: %s", err)) } + } - precertDER, err := c.getPrecert(ctx, cert.Serial) + precertDER, err := c.getPrecert(ctx, cert.Serial) + if err != nil { + // Log and continue, since we want the problems slice to only contains + // problems with the cert itself. + c.logger.Errf("fetching linting precertificate for %s: %s", cert.Serial, err) + atomic.AddInt64(&c.issuedReport.DbErrs, 1) + } else { + err = precert.Correspond(precertDER, cert.Der) if err != nil { - // Log and continue, since we want the problems slice to only contains - // problems with the cert itself. - c.logger.Errf("fetching linting precertificate for %s: %s", cert.Serial, err) - atomic.AddInt64(&c.issuedReport.DbErrs, 1) - } else { - err = precert.Correspond(precertDER, cert.Der) - if err != nil { - problems = append(problems, - fmt.Sprintf("Certificate does not correspond to precert for %s: %s", cert.Serial, err)) - } + problems = append(problems, fmt.Sprintf("Certificate does not correspond to precert for %s: %s", cert.Serial, err)) } + } - if features.Get().CertCheckerChecksValidations { - idents := identifier.FromCert(p) - err = c.checkValidations(ctx, cert, idents) - if err != nil { - if features.Get().CertCheckerRequiresValidations { - problems = append(problems, err.Error()) - } else { - var identValues []string - for _, ident := range idents { - identValues = append(identValues, ident.Value) - } - c.logger.Errf("Certificate %s %s: %s", cert.Serial, identValues, err) + if features.Get().CertCheckerChecksValidations { + idents := identifier.FromCert(p) + err = c.checkValidations(ctx, cert, idents) + if err != nil { + if features.Get().CertCheckerRequiresValidations { + problems = append(problems, err.Error()) + } else { + var identValues []string + for _, ident := range idents { + identValues = append(identValues, ident.Value) } + c.logger.Errf("Certificate %s %s: %s", cert.Serial, identValues, err) } } } - return dnsNames, problems + + return sans, problems } type Config struct { diff --git a/cmd/cert-checker/main_test.go b/cmd/cert-checker/main_test.go index 4a79d1b80..615cfdbee 100644 --- a/cmd/cert-checker/main_test.go +++ b/cmd/cert-checker/main_test.go @@ -143,7 +143,7 @@ func TestCheckWildcardCert(t *testing.T) { } } -func TestCheckCertReturnsDNSNames(t *testing.T) { +func TestCheckCertReturnsSANs(t *testing.T) { saDbMap, err := sa.DBMapForTest(vars.DBConnSA) test.AssertNotError(t, err, "Couldn't connect to database") saCleanup := test.ResetBoulderTestDatabase(t) @@ -171,7 +171,7 @@ func TestCheckCertReturnsDNSNames(t *testing.T) { } names, problems := checker.checkCert(context.Background(), cert) - if !slices.Equal(names, []string{"quite_invalid.com", "al--so--wr--ong.com"}) { + if !slices.Equal(names, []string{"quite_invalid.com", "al--so--wr--ong.com", "127.0.0.1"}) { t.Errorf("didn't get expected DNS names. other problems: %s", strings.Join(problems, "\n")) } } diff --git a/cmd/cert-checker/testdata/quite_invalid.pem b/cmd/cert-checker/testdata/quite_invalid.pem index 632b8b67e..5a5b86c02 100644 --- a/cmd/cert-checker/testdata/quite_invalid.pem +++ b/cmd/cert-checker/testdata/quite_invalid.pem @@ -1,5 +1,5 @@ -----BEGIN CERTIFICATE----- -MIIDUzCCAjugAwIBAgIILgLqdMwyzT4wDQYJKoZIhvcNAQELBQAwIDEeMBwGA1UE +MIIDWTCCAkGgAwIBAgIILgLqdMwyzT4wDQYJKoZIhvcNAQELBQAwIDEeMBwGA1UE AxMVbWluaWNhIHJvb3QgY2EgOTMzZTM5MB4XDTIxMTExMTIwMjMzMloXDTIzMTIx MTIwMjMzMlowHDEaMBgGA1UEAwwRcXVpdGVfaW52YWxpZC5jb20wggEiMA0GCSqG SIb3DQEBAQUAA4IBDwAwggEKAoIBAQDi4jBbqMyvhMonDngNsvie9SHPB16mdpiy @@ -7,14 +7,14 @@ Y/agreU84xUz/roKK07TpVmeqvwWvDkvHTFov7ytKdnCY+z/NXKJ3hNqflWCwU7h Uk9TmpBp0vg+5NvalYul/+bq/B4qDhEvTBzAX3k/UYzd0GQdMyAbwXtG41f5cSK6 cWTQYfJL3gGR5/KLoTz3/VemLgEgAP/CvgcUJPbQceQViiZ4opi9hFIfUqxX2NsD 49klw8cDFu/BG2LEC+XtbdT8XevD0aGIOuYVr+Pa2mxb2QCDXu4tXOsDXH9Y/Cmk -8103QbdB8Y+usOiHG/IXxK2q4J7QNPal4ER4/PGA06V0gwrjNH8BAgMBAAGjgZQw -gZEwDgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcD +8103QbdB8Y+usOiHG/IXxK2q4J7QNPal4ER4/PGA06V0gwrjNH8BAgMBAAGjgZow +gZcwDgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcD AjAMBgNVHRMBAf8EAjAAMB8GA1UdIwQYMBaAFNIcaCjv32YRafE065dZO57ONWuk -MDEGA1UdEQQqMCiCEXF1aXRlX2ludmFsaWQuY29tghNhbC0tc28tLXdyLS1vbmcu -Y29tMA0GCSqGSIb3DQEBCwUAA4IBAQAjSv0o5G4VuLnnwHON4P53bLvGnYqaqYju -TEafi3hSgHAfBuhOQUVgwujoYpPp1w1fm5spfcbSwNNRte79HgV97kAuZ4R4RHk1 -5Xux1ITLalaHR/ilu002N0eJ7dFYawBgV2xMudULzohwmW2RjPJ5811iWwtiVf1b -A3V5SZJWSJll1BhANBs7R0pBbyTSNHR470N8TGG0jfXqgTKd0xZaH91HrwEMo+96 -llbfp90Y5OfHIfym/N1sH2hVgd+ZAkhiVEiNBWZlbSyOgbZ1cCBvBXg6TuwpQMZK -9RWjlpni8yuzLGduPl8qHG1dqsUvbVqcG+WhHLbaZMNhiMfiWInL +MDcGA1UdEQQwMC6CEXF1aXRlX2ludmFsaWQuY29tghNhbC0tc28tLXdyLS1vbmcu +Y29thwR/AAABMA0GCSqGSIb3DQEBCwUAA4IBAQAjSv0o5G4VuLnnwHON4P53bLvG +nYqaqYjuTEafi3hSgHAfBuhOQUVgwujoYpPp1w1fm5spfcbSwNNRte79HgV97kAu +Z4R4RHk15Xux1ITLalaHR/ilu002N0eJ7dFYawBgV2xMudULzohwmW2RjPJ5811i +WwtiVf1bA3V5SZJWSJll1BhANBs7R0pBbyTSNHR470N8TGG0jfXqgTKd0xZaH91H +rwEMo+96llbfp90Y5OfHIfym/N1sH2hVgd+ZAkhiVEiNBWZlbSyOgbZ1cCBvBXg6 +TuwpQMZK9RWjlpni8yuzLGduPl8qHG1dqsUvbVqcG+WhHLbaZMNhiMfiWInL -----END CERTIFICATE-----