Fix orphan-finder (#4507)

This creates the correct type of backend service for the OCSP generator.
It also adds an invocation of orphan-finder during the integration
tests.

This also adds a minor safety check to SA that I hit while writing the
test. Without this safety check, passing a certificate with no DNSNames
to AddCertificate would result in an obscure MariaDB syntax error
without enough context to track it down. In normal circumstances this
shouldn't be hit, but it will be good to have a solid error message if
we hit it in tests sometime.

Also, this tweaks the .travis.yml so it explicitly sets BOULDER_CONFIG_DIR
to test/config in the default case. Because the docker-compose run
command uses -e BOULDER_CONFIG_DIR="${BOULDER_CONFIG_DIR}",
we were setting a blank BOULDER_CONFIG_DIR in default case.
Since the Python startservers script sets a default if BOULDER_CONFIG_DIR
is not set, we haven't noticed this before. But since this test case relies
on the actual environment variable, it became an issue.

Fixes #4499

.travis.yml

@@ -33,7 +33,7 @@ env:
     #
     # Current Go version build tasks:
     #
-    - RUN="lints integration generate rpm"
+    - RUN="lints integration generate rpm" BOULDER_CONFIG_DIR="test/config"
     # Config changes that have landed in master but not yet been applied to
     # production can be made in boulder-config-next.json.
     - RUN="integration" BOULDER_CONFIG_DIR="test/config-next"

cmd/orphan-finder/main.go

@@ -26,6 +26,7 @@ import (
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 	sapb "github.com/letsencrypt/boulder/sa/proto"
+	"google.golang.org/grpc"
 )
 
 var usageString = `
@@ -61,7 +62,7 @@ type certificateStorage interface {
 }
 
 type ocspGenerator interface {
-	GenerateOCSP(context.Context, core.OCSPSigningRequest) ([]byte, error)
+	GenerateOCSP(context.Context, *capb.GenerateOCSPRequest, ...grpc.CallOption) (*capb.OCSPResponse, error)
 }
 
 // orphanType is a numeric identifier for the type of orphan being processed.
@@ -199,11 +200,7 @@ func storeParsedLogLine(sa certificateStorage, ca ocspGenerator, logger blog.Log
 		logger.AuditErrf("Couldn't parse regID: %s, [%s]", err, line)
 		return true, false, typ
 	}
-	// generate a fresh OCSP response
-	ocspResponse, err := ca.GenerateOCSP(ctx, core.OCSPSigningRequest{
-		CertDER: der,
-		Status:  string(core.OCSPStatusGood),
-	})
+	response, err := generateOCSP(ctx, ca, der)
 	if err != nil {
 		logger.AuditErrf("Couldn't generate OCSP: %s, [%s]", err, line)
 		return true, false, typ
@@ -215,13 +212,13 @@ func storeParsedLogLine(sa certificateStorage, ca ocspGenerator, logger blog.Log
 	issuedDate := cert.NotBefore.Add(backdateDuration)
 	switch typ {
 	case certOrphan:
-		_, err = sa.AddCertificate(ctx, der, regID, ocspResponse, &issuedDate)
+		_, err = sa.AddCertificate(ctx, der, regID, response, &issuedDate)
 	case precertOrphan:
 		issued := issuedDate.UnixNano()
 		_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 			Der:    der,
 			RegID:  &regID,
-			Ocsp:   ocspResponse,
+			Ocsp:   response,
 			Issued: &issued,
 		})
 	default:
@@ -235,7 +232,24 @@ func storeParsedLogLine(sa certificateStorage, ca ocspGenerator, logger blog.Log
 	return true, true, typ
 }
 
-func setup(configFile string) (blog.Logger, core.StorageAuthority, core.CertificateAuthority) {
+func generateOCSP(ctx context.Context, ca ocspGenerator, certDER []byte) ([]byte, error) {
+	// generate a fresh OCSP response
+	statusGood := string(core.OCSPStatusGood)
+	zeroInt32 := int32(0)
+	zeroInt64 := int64(0)
+	ocspResponse, err := ca.GenerateOCSP(ctx, &capb.GenerateOCSPRequest{
+		CertDER:   certDER,
+		Status:    &statusGood,
+		Reason:    &zeroInt32,
+		RevokedAt: &zeroInt64,
+	})
+	if err != nil {
+		return nil, err
+	}
+	return ocspResponse.Response, nil
+}
+
+func setup(configFile string) (blog.Logger, core.StorageAuthority, capb.OCSPGeneratorClient) {
 	configJSON, err := ioutil.ReadFile(configFile)
 	cmd.FailOnError(err, "Failed to read config file")
 	var conf config
@@ -255,7 +269,7 @@ func setup(configFile string) (blog.Logger, core.StorageAuthority, core.Certific
 
 	caConn, err := bgrpc.ClientSetup(conf.OCSPGeneratorService, tlsConfig, clientMetrics, cmd.Clock())
 	cmd.FailOnError(err, "Failed to load credentials and create gRPC connection to CA")
-	cac := bgrpc.NewCertificateAuthorityClient(nil, capb.NewCertificateAuthorityClient(caConn))
+	cac := capb.NewOCSPGeneratorClient(caConn)
 
 	backdateDuration = conf.Backdate.Duration
 	return logger, sac, cac
@@ -298,6 +312,9 @@ func main() {
 
 		var certOrphansFound, certOrphansAdded, precertOrphansFound, precertOrphansAdded int64
 		for _, line := range strings.Split(string(logData), "\n") {
+			if line == "" {
+				continue
+			}
 			found, added, typ := storeParsedLogLine(sa, ca, logger, line)
 			var foundStat, addStat *int64
 			switch typ {
@@ -334,21 +351,18 @@ func main() {
 		// Because certificates are backdated we need to add the backdate duration
 		// to find the true issued time.
 		issuedDate := cert.NotBefore.Add(1 * backdateDuration)
-		ocspResponse, err := ca.GenerateOCSP(ctx, core.OCSPSigningRequest{
-			CertDER: der,
-			Status:  string(core.OCSPStatusGood),
-		})
+		response, err := generateOCSP(ctx, ca, der)
 		cmd.FailOnError(err, "Generating OCSP")
 
 		switch typ {
 		case certOrphan:
-			_, err = sa.AddCertificate(ctx, der, *regID, ocspResponse, &issuedDate)
+			_, err = sa.AddCertificate(ctx, der, *regID, response, &issuedDate)
 		case precertOrphan:
 			issued := issuedDate.UnixNano()
 			_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 				Der:    der,
 				RegID:  regID,
-				Ocsp:   ocspResponse,
+				Ocsp:   response,
 				Issued: &issued,
 			})
 		default:

cmd/orphan-finder/main_test.go

@@ -8,7 +8,10 @@ import (
 	"testing"
 	"time"
 
+	"google.golang.org/grpc"
+
 	"github.com/jmhodges/clock"
+	capb "github.com/letsencrypt/boulder/ca/proto"
 	"github.com/letsencrypt/boulder/core"
 	corepb "github.com/letsencrypt/boulder/core/proto"
 	berrors "github.com/letsencrypt/boulder/errors"
@@ -90,8 +93,10 @@ func (m *mockSA) GetPrecertificate(ctx context.Context, req *sapb.Serial) (*core
 
 type mockCA struct{}
 
-func (ca *mockCA) GenerateOCSP(ctx context.Context, xferObj core.OCSPSigningRequest) (ocsp []byte, err error) {
-	return []byte("HI"), nil
+func (ca *mockCA) GenerateOCSP(context.Context, *capb.GenerateOCSPRequest, ...grpc.CallOption) (*capb.OCSPResponse, error) {
+	return &capb.OCSPResponse{
+		Response: []byte("HI"),
+	}, nil
 }
 
 func checkNoErrors(t *testing.T) {

grpc/ca-wrappers.go

@@ -112,12 +112,15 @@ func (cas *CertificateAuthorityServerWrapper) IssueCertificateForPrecertificate(
 	return CertToPB(cert), nil
 }
 
-func (cas *CertificateAuthorityServerWrapper) GenerateOCSP(ctx context.Context, request *caPB.GenerateOCSPRequest) (*caPB.OCSPResponse, error) {
+func (cas *CertificateAuthorityServerWrapper) GenerateOCSP(ctx context.Context, req *caPB.GenerateOCSPRequest) (*caPB.OCSPResponse, error) {
+	if req.CertDER == nil || req.Status == nil || req.Reason == nil || req.RevokedAt == nil {
+		return nil, errIncompleteRequest
+	}
 	res, err := cas.inner.GenerateOCSP(ctx, core.OCSPSigningRequest{
-		CertDER:   request.CertDER,
-		Status:    *request.Status,
-		Reason:    revocation.Reason(*request.Reason),
-		RevokedAt: time.Unix(0, *request.RevokedAt),
+		CertDER:   req.CertDER,
+		Status:    *req.Status,
+		Reason:    revocation.Reason(*req.Reason),
+		RevokedAt: time.Unix(0, *req.RevokedAt),
 	})
 	if err != nil {
 		return nil, err

grpc/client.go

@@ -18,6 +18,9 @@ import (
 // on the provided *tls.Config.
 // It dials the remote service and returns a grpc.ClientConn if successful.
 func ClientSetup(c *cmd.GRPCClientConfig, tlsConfig *tls.Config, metrics clientMetrics, clk clock.Clock) (*grpc.ClientConn, error) {
+	if c == nil {
+		return nil, errors.New("nil gRPC client config provided. JSON config is probably missing a fooService section.")
+	}
 	if c.ServerAddress == "" {
 		return nil, errors.New("ServerAddress must not be empty")
 	}

sa/sa.go

@@ -946,6 +946,9 @@ func deleteOrderFQDNSet(
 }
 
 func addIssuedNames(db dbExecer, cert *x509.Certificate, isRenewal bool) error {
+	if len(cert.DNSNames) == 0 {
+		return berrors.InternalServerError("certificate has no DNSNames")
+	}
 	var qmarks []string
 	var values []interface{}
 	for _, name := range cert.DNSNames {

test/config-next/ca-b.json

@@ -24,7 +24,8 @@
     "grpcOCSPGenerator": {
       "address": ":9096",
       "clientNames": [
-        "ocsp-updater.boulder"
+        "ocsp-updater.boulder",
+        "orphan-finder.boulder"
       ]
     },
     "Issuers": [{

test/config-next/orphan-finder.json

@@ -6,9 +6,9 @@
   },
 
   "tls": {
-    "caCertFile": "test/grpc-creds/minica.pem",
-    "certFile": "test/grpc-creds/orphan-finder.boulder/cert.pem",
-    "keyFile": "test/grpc-creds/orphan-finder.boulder/key.pem"
+    "caCertFile": "../../test/grpc-creds/minica.pem",
+    "certFile": "../../test/grpc-creds/orphan-finder.boulder/cert.pem",
+    "keyFile": "../../test/grpc-creds/orphan-finder.boulder/key.pem"
   },
 
   "ocspGeneratorService": {

test/config/ca-a.json

@@ -23,7 +23,8 @@
     "grpcOCSPGenerator": {
       "address": ":9096",
       "clientNames": [
-        "ocsp-updater.boulder"
+        "ocsp-updater.boulder",
+        "orphan-finder.boulder"
       ]
     },
     "Issuers": [{

test/config/ca-b.json

@@ -23,7 +23,8 @@
     "grpcOCSPGenerator": {
       "address": ":9096",
       "clientNames": [
-        "ocsp-updater.boulder"
+        "ocsp-updater.boulder",
+        "orphan-finder.boulder"
       ]
     },
     "Issuers": [{

test/config/orphan-finder.json

@@ -6,11 +6,15 @@
   },
 
   "tls": {
-    "caCertFile": "test/grpc-creds/minica.pem",
-    "certFile": "test/grpc-creds/orphan-finder.boulder/cert.pem",
-    "keyFile": "test/grpc-creds/orphan-finder.boulder/key.pem"
+    "caCertFile": "../../test/grpc-creds/minica.pem",
+    "certFile": "../../test/grpc-creds/orphan-finder.boulder/cert.pem",
+    "keyFile": "../../test/grpc-creds/orphan-finder.boulder/key.pem"
   },
 
+  "ocspGeneratorService": {
+    "serverAddress": "ca.boulder:9096",
+    "timeout": "15s"
+  },
   "saService": {
     "serverAddress": "sa.boulder:9095",
     "timeout": "15s"

test/integration/orphan_finder_test.go

@@ -0,0 +1,118 @@
+// +build integration
+
+package integration
+
+import (
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"encoding/pem"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"log"
+	"math/big"
+	"os"
+	"os/exec"
+	"strings"
+	"testing"
+	"time"
+)
+
+var template = `[AUDIT] Failed RPC to store at SA, orphaning precertificate: serial=[%x] cert=[%x] err=[sa.StorageAuthority.AddCertificate timed out after 5000 ms], regID=[1], orderID=[1]
+[AUDIT] Failed RPC to store at SA, orphaning certificate: serial=[%x] cert=[%x] err=[sa.StorageAuthority.AddCertificate timed out after 5000 ms], regID=[1], orderID=[1]`
+
+// TestOrphanFinder runs the orphan-finder with an example input file. This must
+// be run after other tests so the account ID 1 exists (since the inserted
+// certificates will be associated with that account).
+func TestOrphanFinder(t *testing.T) {
+	precert, err := makeFakeCert(true)
+	if err != nil {
+		log.Fatal(err)
+	}
+	cert, err := makeFakeCert(false)
+	if err != nil {
+		log.Fatal(err)
+	}
+	f, _ := ioutil.TempFile("", "orphaned.log")
+	io.WriteString(f, fmt.Sprintf(template, precert.SerialNumber.Bytes(),
+		precert.Raw, cert.SerialNumber.Bytes(), cert.Raw))
+	f.Close()
+	cmd := exec.Command("../../bin/orphan-finder", "parse-ca-log",
+		"--config", "../../"+os.Getenv("BOULDER_CONFIG_DIR")+"/orphan-finder.json",
+		"--log-file", f.Name())
+	out, err := cmd.Output()
+	if err != nil {
+		t.Fatalf("orphan finder failed (%s). Output was: %s", err, out)
+	}
+	if !strings.Contains(string(out), "Found 1 precertificate orphans and added 1 to the database") {
+		t.Fatalf("Failed to insert orphaned precertificate. orphan-finder output was: %s", out)
+	}
+	if !strings.Contains(string(out), "Found 1 certificate orphans and added 1 to the database") {
+		t.Fatalf("Failed to insert orphaned certificate. orphan-finder output was: %s", out)
+	}
+}
+
+// makeFakeCert a unique fake cert for each run of TestOrphanFinder to avoid duplicate
+// errors. This fake cert will have its issuer equal to the issuer we use in the
+// general integration test setup, and will be signed by that issuer key.
+// Otherwise, the request orphan-finder makes to sign OCSP would be rejected.
+func makeFakeCert(precert bool) (*x509.Certificate, error) {
+	serialBytes := make([]byte, 18)
+	_, err := rand.Read(serialBytes[:])
+	if err != nil {
+		return nil, err
+	}
+	serial := big.NewInt(0)
+	serial.SetBytes(serialBytes)
+	key, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		return nil, err
+	}
+	issuerKeyBytes, err := ioutil.ReadFile("../test-ca.key")
+	if err != nil {
+		return nil, err
+	}
+	block, _ := pem.Decode(issuerKeyBytes)
+	if block == nil {
+		return nil, fmt.Errorf("no PEM found")
+	}
+	issuerKey, err := x509.ParsePKCS8PrivateKey(block.Bytes)
+	if err != nil {
+		return nil, fmt.Errorf("parsing private key: %s", err)
+	}
+	issuerTemplate := &x509.Certificate{
+		Subject: pkix.Name{
+			CommonName: "h2ppy h2cker fake CA",
+		},
+	}
+	template := &x509.Certificate{
+		Subject: pkix.Name{
+			CommonName: "fake cert for TestOrphanFinder",
+		},
+		SerialNumber: serial,
+		NotBefore:    time.Now(),
+		NotAfter:     time.Now().AddDate(0, 90, 0),
+		DNSNames:     []string{"fakecert.example.com"},
+	}
+	if precert {
+		template.ExtraExtensions = []pkix.Extension{
+			pkix.Extension{
+				Id:       OIDExtensionCTPoison,
+				Critical: true,
+				Value:    []byte{5, 0},
+			},
+		}
+	}
+
+	der, err := x509.CreateCertificate(rand.Reader, template, issuerTemplate, key.Public(), issuerKey)
+	if err != nil {
+		return nil, err
+	}
+	cert, err := x509.ParseCertificate(der)
+	if err != nil {
+		return nil, err
+	}
+	return cert, err
+}