boulder-observer: check certificate status via CRL too (#8186)

Let's Encrypt [recently removed OCSP URLs from
certificates](https://community.letsencrypt.org/t/removing-ocsp-urls-from-certificates/236699)
which unfortunately caused the boulder-observer TLS prober to panic.
This change short circuits the OCSP checking logic if no OCSP URL exists
in the to-be-checked certificate.

Fixes #8185

---------

Co-authored-by: Aaron Gable <[email protected]>

Author: pgporada

docker-compose.yml

@@ -44,6 +44,9 @@ services:
       # we can put that name inside our integration test certs (e.g. as a crl
       # url) and have it look like a publicly-accessible name.
       - "ca.example.org:10.77.77.77"
+      # Allow the boulder container to be reached as "integration.trust", for
+      # similar reasons, but intended for use as a SAN rather than a CRLDP.
+      - "integration.trust:10.77.77.77"
     ports:
       - 4001:4001 # ACMEv2
       - 4002:4002 # OCSP

observer/probers/tls/tls.go

@@ -5,33 +5,35 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"io"
 	"net"
 	"net/http"
 	"time"
 
-	"github.com/letsencrypt/boulder/observer/obsdialer"
 	"github.com/prometheus/client_golang/prometheus"
 	"golang.org/x/crypto/ocsp"
+
+	"github.com/letsencrypt/boulder/observer/obsdialer"
 )
 
 type reason int
 
 const (
 	none reason = iota
 	internalError
-	ocspError
+	revocationStatusError
 	rootDidNotMatch
-	responseDidNotMatch
+	statusDidNotMatch
 )
 
 var reasonToString = map[reason]string{
-	none:                "nil",
-	internalError:       "internalError",
-	ocspError:           "ocspError",
-	rootDidNotMatch:     "rootDidNotMatch",
-	responseDidNotMatch: "responseDidNotMatch",
+	none:                  "nil",
+	internalError:         "internalError",
+	revocationStatusError: "revocationStatusError",
+	rootDidNotMatch:       "rootDidNotMatch",
+	statusDidNotMatch:     "statusDidNotMatch",
 }
 
 func getReasons() []string {
@@ -65,14 +67,19 @@ func (p TLSProbe) Kind() string {
 }
 
 // Get OCSP status (good, revoked or unknown) of certificate
-func checkOCSP(cert, issuer *x509.Certificate, want int) (bool, error) {
+func checkOCSP(ctx context.Context, cert, issuer *x509.Certificate, want int) (bool, error) {
 	req, err := ocsp.CreateRequest(cert, issuer, nil)
 	if err != nil {
 		return false, err
 	}
 
 	url := fmt.Sprintf("%s/%s", cert.OCSPServer[0], base64.StdEncoding.EncodeToString(req))
-	res, err := http.Get(url)
+	r, err := http.NewRequestWithContext(ctx, "GET", url, nil)
+	if err != nil {
+		return false, err
+	}
+
+	res, err := http.DefaultClient.Do(r)
 	if err != nil {
 		return false, err
 	}
@@ -90,6 +97,45 @@ func checkOCSP(cert, issuer *x509.Certificate, want int) (bool, error) {
 	return ocspRes.Status == want, nil
 }
 
+func checkCRL(ctx context.Context, cert, issuer *x509.Certificate, want int) (bool, error) {
+	if len(cert.CRLDistributionPoints) != 1 {
+		return false, errors.New("cert does not contain CRLDP URI")
+	}
+
+	req, err := http.NewRequestWithContext(ctx, "GET", cert.CRLDistributionPoints[0], nil)
+	if err != nil {
+		return false, fmt.Errorf("creating HTTP request: %w", err)
+	}
+
+	resp, err := http.DefaultClient.Do(req)
+	if err != nil {
+		return false, fmt.Errorf("downloading CRL: %w", err)
+	}
+	defer resp.Body.Close()
+
+	der, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return false, fmt.Errorf("reading CRL: %w", err)
+	}
+
+	crl, err := x509.ParseRevocationList(der)
+	if err != nil {
+		return false, fmt.Errorf("parsing CRL: %w", err)
+	}
+
+	err = crl.CheckSignatureFrom(issuer)
+	if err != nil {
+		return false, fmt.Errorf("validating CRL: %w", err)
+	}
+
+	for _, entry := range crl.RevokedCertificateEntries {
+		if entry.SerialNumber.Cmp(cert.SerialNumber) == 0 {
+			return want == ocsp.Revoked, nil
+		}
+	}
+	return want == ocsp.Good, nil
+}
+
 // Return an error if the root settings are nonempty and do not match the
 // expected root.
 func (p TLSProbe) checkRoot(rootOrg, rootCN string) error {
@@ -109,40 +155,54 @@ func (p TLSProbe) exportMetrics(cert *x509.Certificate, reason reason) {
 }
 
 func (p TLSProbe) probeExpired(timeout time.Duration) bool {
-	config := &tls.Config{
-		// Set InsecureSkipVerify to skip the default validation we are
-		// replacing. This will not disable VerifyConnection.
-		InsecureSkipVerify: true,
-		VerifyConnection: func(cs tls.ConnectionState) error {
-			opts := x509.VerifyOptions{
-				CurrentTime:   cs.PeerCertificates[0].NotAfter,
-				Intermediates: x509.NewCertPool(),
-			}
-			for _, cert := range cs.PeerCertificates[1:] {
-				opts.Intermediates.AddCert(cert)
-			}
-			_, err := cs.PeerCertificates[0].Verify(opts)
-			return err
-		},
+	addr := p.hostname
+	_, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		addr = net.JoinHostPort(addr, "443")
 	}
-	ctx, cancel := context.WithTimeout(context.Background(), timeout)
-	defer cancel()
+
 	tlsDialer := tls.Dialer{
 		NetDialer: &obsdialer.Dialer,
-		Config:    config,
+		Config: &tls.Config{
+			// Set InsecureSkipVerify to skip the default validation we are
+			// replacing. This will not disable VerifyConnection.
+			InsecureSkipVerify: true,
+			VerifyConnection: func(cs tls.ConnectionState) error {
+				issuers := x509.NewCertPool()
+				for _, cert := range cs.PeerCertificates[1:] {
+					issuers.AddCert(cert)
+				}
+				opts := x509.VerifyOptions{
+					// We set the current time to be the cert's expiration date so that
+					// the validation routine doesn't complain that the cert is expired.
+					CurrentTime: cs.PeerCertificates[0].NotAfter,
+					// By settings roots and intermediates to be whatever was presented
+					// in the handshake, we're saying that we don't care about the cert
+					// chaining up to the system trust store. This is safe because we
+					// check the root ourselves in checkRoot().
+					Intermediates: issuers,
+					Roots:         issuers,
+				}
+				_, err := cs.PeerCertificates[0].Verify(opts)
+				return err
+			},
+		},
 	}
-	conn, err := tlsDialer.DialContext(ctx, "tcp", p.hostname+":443")
+
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+
+	conn, err := tlsDialer.DialContext(ctx, "tcp", addr)
 	if err != nil {
 		p.exportMetrics(nil, internalError)
 		return false
 	}
 	defer conn.Close()
 
 	// tls.Dialer.DialContext is documented to always return *tls.Conn
-	tlsConn := conn.(*tls.Conn)
-	peers := tlsConn.ConnectionState().PeerCertificates
+	peers := conn.(*tls.Conn).ConnectionState().PeerCertificates
 	if time.Until(peers[0].NotAfter) > 0 {
-		p.exportMetrics(peers[0], responseDidNotMatch)
+		p.exportMetrics(peers[0], statusDidNotMatch)
 		return false
 	}
 
@@ -158,35 +218,77 @@ func (p TLSProbe) probeExpired(timeout time.Duration) bool {
 }
 
 func (p TLSProbe) probeUnexpired(timeout time.Duration) bool {
-	conn, err := tls.DialWithDialer(&net.Dialer{Timeout: timeout}, "tcp", p.hostname+":443", &tls.Config{})
+	addr := p.hostname
+	_, _, err := net.SplitHostPort(addr)
+	if err != nil {
+		addr = net.JoinHostPort(addr, "443")
+	}
+
+	tlsDialer := tls.Dialer{
+		NetDialer: &obsdialer.Dialer,
+		Config: &tls.Config{
+			// Set InsecureSkipVerify to skip the default validation we are
+			// replacing. This will not disable VerifyConnection.
+			InsecureSkipVerify: true,
+			VerifyConnection: func(cs tls.ConnectionState) error {
+				issuers := x509.NewCertPool()
+				for _, cert := range cs.PeerCertificates[1:] {
+					issuers.AddCert(cert)
+				}
+				opts := x509.VerifyOptions{
+					// By settings roots and intermediates to be whatever was presented
+					// in the handshake, we're saying that we don't care about the cert
+					// chaining up to the system trust store. This is safe because we
+					// check the root ourselves in checkRoot().
+					Intermediates: issuers,
+					Roots:         issuers,
+				}
+				_, err := cs.PeerCertificates[0].Verify(opts)
+				return err
+			},
+		},
+	}
+
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
+	defer cancel()
+
+	conn, err := tlsDialer.DialContext(ctx, "tcp", addr)
 	if err != nil {
 		p.exportMetrics(nil, internalError)
 		return false
 	}
-
 	defer conn.Close()
-	peers := conn.ConnectionState().PeerCertificates
+
+	// tls.Dialer.DialContext is documented to always return *tls.Conn
+	peers := conn.(*tls.Conn).ConnectionState().PeerCertificates
 	root := peers[len(peers)-1].Issuer
 	err = p.checkRoot(root.Organization[0], root.CommonName)
 	if err != nil {
 		p.exportMetrics(peers[0], rootDidNotMatch)
 		return false
 	}
 
-	var ocspStatus bool
+	var wantStatus int
 	switch p.response {
 	case "valid":
-		ocspStatus, err = checkOCSP(peers[0], peers[1], ocsp.Good)
+		wantStatus = ocsp.Good
 	case "revoked":
-		ocspStatus, err = checkOCSP(peers[0], peers[1], ocsp.Revoked)
+		wantStatus = ocsp.Revoked
+	}
+
+	var statusMatch bool
+	if len(peers[0].OCSPServer) != 0 {
+		statusMatch, err = checkOCSP(ctx, peers[0], peers[1], wantStatus)
+	} else {
+		statusMatch, err = checkCRL(ctx, peers[0], peers[1], wantStatus)
 	}
 	if err != nil {
-		p.exportMetrics(peers[0], ocspError)
+		p.exportMetrics(peers[0], revocationStatusError)
 		return false
 	}
 
-	if !ocspStatus {
-		p.exportMetrics(peers[0], responseDidNotMatch)
+	if !statusMatch {
+		p.exportMetrics(peers[0], statusDidNotMatch)
 		return false
 	}
 

observer/probers/tls/tls_conf.go

@@ -2,12 +2,15 @@ package probers
 
 import (
 	"fmt"
+	"net"
 	"net/url"
+	"strconv"
 	"strings"
 
+	"github.com/prometheus/client_golang/prometheus"
+
 	"github.com/letsencrypt/boulder/observer/probers"
 	"github.com/letsencrypt/boulder/strictyaml"
-	"github.com/prometheus/client_golang/prometheus"
 )
 
 const (
@@ -42,15 +45,28 @@ func (c TLSConf) UnmarshalSettings(settings []byte) (probers.Configurer, error)
 }
 
 func (c TLSConf) validateHostname() error {
-	url, err := url.Parse(c.Hostname)
+	hostname := c.Hostname
+
+	if strings.Contains(c.Hostname, ":") {
+		host, port, err := net.SplitHostPort(c.Hostname)
+		if err != nil {
+			return fmt.Errorf("invalid 'hostname', got %q, expected a valid hostport: %s", c.Hostname, err)
+		}
+
+		_, err = strconv.Atoi(port)
+		if err != nil {
+			return fmt.Errorf("invalid 'hostname', got %q, expected a valid hostport: %s", c.Hostname, err)
+		}
+		hostname = host
+	}
+
+	url, err := url.Parse(hostname)
 	if err != nil {
-		return fmt.Errorf(
-			"invalid 'hostname', got %q, expected a valid hostname: %s", c.Hostname, err)
+		return fmt.Errorf("invalid 'hostname', got %q, expected a valid hostname: %s", c.Hostname, err)
 	}
 
 	if url.Scheme != "" {
-		return fmt.Errorf(
-			"invalid 'hostname', got: %q, should not include scheme", c.Hostname)
+		return fmt.Errorf("invalid 'hostname', got: %q, should not include scheme", c.Hostname)
 	}
 
 	return nil

observer/probers/tls/tls_conf_test.go

@@ -4,9 +4,10 @@ import (
 	"reflect"
 	"testing"
 
-	"github.com/letsencrypt/boulder/observer/probers"
 	"github.com/prometheus/client_golang/prometheus"
 	"gopkg.in/yaml.v3"
+
+	"github.com/letsencrypt/boulder/observer/probers"
 )
 
 func TestTLSConf_MakeProber(t *testing.T) {
@@ -33,10 +34,12 @@ func TestTLSConf_MakeProber(t *testing.T) {
 		// valid
 		{"valid hostname", fields{"example.com", goodRootCN, "valid"}, colls, false},
 		{"valid hostname with path", fields{"example.com/foo/bar", "ISRG Root X2", "Revoked"}, colls, false},
+		{"valid hostname with port", fields{"example.com:8080", goodRootCN, "expired"}, colls, false},
 
 		// invalid hostname
 		{"bad hostname", fields{":::::", goodRootCN, goodResponse}, colls, true},
 		{"included scheme", fields{"https://example.com", goodRootCN, goodResponse}, colls, true},
+		{"included scheme and port", fields{"https://example.com:443", goodRootCN, goodResponse}, colls, true},
 
 		// invalid response
 		{"empty response", fields{goodHostname, goodRootCN, ""}, colls, true},