From d4168626add7702cd008c8dbe84e1e92ab20846a Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 25 Oct 2019 09:51:14 -0700 Subject: [PATCH] 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 | 2 +- cmd/orphan-finder/main.go | 46 ++++++---- cmd/orphan-finder/main_test.go | 9 +- grpc/ca-wrappers.go | 13 +-- grpc/client.go | 3 + sa/sa.go | 3 + test/config-next/ca-b.json | 3 +- test/config-next/orphan-finder.json | 6 +- test/config/ca-a.json | 3 +- test/config/ca-b.json | 3 +- test/config/orphan-finder.json | 10 ++- test/integration/orphan_finder_test.go | 118 +++++++++++++++++++++++++ 12 files changed, 186 insertions(+), 33 deletions(-) create mode 100644 test/integration/orphan_finder_test.go diff --git a/.travis.yml b/.travis.yml index 0708ab4c5..39ce4e692 100644 --- a/.travis.yml +++ b/.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" diff --git a/cmd/orphan-finder/main.go b/cmd/orphan-finder/main.go index 1378c2cd8..63956ec92 100644 --- a/cmd/orphan-finder/main.go +++ b/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: ®ID, - 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: diff --git a/cmd/orphan-finder/main_test.go b/cmd/orphan-finder/main_test.go index d00c7c48b..21b352120 100644 --- a/cmd/orphan-finder/main_test.go +++ b/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) { diff --git a/grpc/ca-wrappers.go b/grpc/ca-wrappers.go index 62e15fcc7..e9495e44f 100644 --- a/grpc/ca-wrappers.go +++ b/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 diff --git a/grpc/client.go b/grpc/client.go index e759ba6b7..94f223ee1 100644 --- a/grpc/client.go +++ b/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") } diff --git a/sa/sa.go b/sa/sa.go index 1e56fc484..ca06df07b 100644 --- a/sa/sa.go +++ b/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 { diff --git a/test/config-next/ca-b.json b/test/config-next/ca-b.json index 6c41703c9..e03b6f521 100644 --- a/test/config-next/ca-b.json +++ b/test/config-next/ca-b.json @@ -24,7 +24,8 @@ "grpcOCSPGenerator": { "address": ":9096", "clientNames": [ - "ocsp-updater.boulder" + "ocsp-updater.boulder", + "orphan-finder.boulder" ] }, "Issuers": [{ diff --git a/test/config-next/orphan-finder.json b/test/config-next/orphan-finder.json index 84bffb3bd..d5332d1ca 100644 --- a/test/config-next/orphan-finder.json +++ b/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": { diff --git a/test/config/ca-a.json b/test/config/ca-a.json index 9e8b2eb0e..3ad5be631 100644 --- a/test/config/ca-a.json +++ b/test/config/ca-a.json @@ -23,7 +23,8 @@ "grpcOCSPGenerator": { "address": ":9096", "clientNames": [ - "ocsp-updater.boulder" + "ocsp-updater.boulder", + "orphan-finder.boulder" ] }, "Issuers": [{ diff --git a/test/config/ca-b.json b/test/config/ca-b.json index fa7874933..e47dbd255 100644 --- a/test/config/ca-b.json +++ b/test/config/ca-b.json @@ -23,7 +23,8 @@ "grpcOCSPGenerator": { "address": ":9096", "clientNames": [ - "ocsp-updater.boulder" + "ocsp-updater.boulder", + "orphan-finder.boulder" ] }, "Issuers": [{ diff --git a/test/config/orphan-finder.json b/test/config/orphan-finder.json index 6c48fd822..d5332d1ca 100644 --- a/test/config/orphan-finder.json +++ b/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" diff --git a/test/integration/orphan_finder_test.go b/test/integration/orphan_finder_test.go new file mode 100644 index 000000000..cddf985d0 --- /dev/null +++ b/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 +}