From 17e9e7fbb7c01e6fac3cb6d366f958b11c834c17 Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Wed, 23 Sep 2020 16:45:19 -0700 Subject: [PATCH] SA: Ensure that IssuerID is set when adding precertificates (#5099) This change adds `req.IssuerID` to the set of fields that the SA's `AddPrecertificate` method requires be non-zero. As a result, this also updates many tests, both unit and integration, to ensure that they supply a value (usually just 1) for that field. The most complex part of the test changes is a slight refactoring to the orphan-finder code, which makes it easier to reason about the separation between log line parsing and building and sending the request. Based on #5096 Fixes #5097 --- ca/ca.go | 10 ++- cmd/admin-revoker/main_test.go | 7 +- cmd/ocsp-updater/main_test.go | 97 +++++++++++----------- cmd/orphan-finder/main.go | 107 ++++++++++++++++--------- cmd/orphan-finder/main_test.go | 41 +++++++--- sa/precertificates.go | 2 +- sa/precertificates_test.go | 51 +++++++++--- sa/sa_test.go | 34 ++++---- test/integration/orphan_finder_test.go | 4 +- 9 files changed, 213 insertions(+), 140 deletions(-) diff --git a/ca/ca.go b/ca/ca.go index c44e8902e..3d6286f09 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -576,12 +576,14 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss return nil, err } + issuerID := idForCert(issuer.cert) + req := &sapb.AddCertificateRequest{ Der: precertDER, RegID: regID, Ocsp: ocspResp.Response, Issued: nowNanos, - IssuerID: idForCert(issuer.cert), + IssuerID: issuerID, } _, err = ca.sa.AddPrecertificate(ctx, req) @@ -590,15 +592,15 @@ func (ca *CertificateAuthorityImpl) IssuePrecertificate(ctx context.Context, iss err = berrors.InternalServerError(err.Error()) // Note: This log line is parsed by cmd/orphan-finder. If you make any // changes here, you should make sure they are reflected in orphan-finder. - ca.log.AuditErrf("Failed RPC to store at SA, orphaning precertificate: serial=[%s] cert=[%s] err=[%v], regID=[%d], orderID=[%d]", - serialHex, hex.EncodeToString(precertDER), err, issueReq.RegistrationID, issueReq.OrderID) + ca.log.AuditErrf("Failed RPC to store at SA, orphaning precertificate: serial=[%s], cert=[%s], issuerID=[%d], regID=[%d], orderID=[%d], err=[%v]", + serialHex, hex.EncodeToString(precertDER), issuerID, issueReq.RegistrationID, issueReq.OrderID, err) if ca.orphanQueue != nil { ca.queueOrphan(&orphanedCert{ DER: precertDER, RegID: regID, OCSPResp: ocspResp.Response, Precert: true, - IssuerID: idForCert(issuer.cert), + IssuerID: issuerID, }) } return nil, err diff --git a/cmd/admin-revoker/main_test.go b/cmd/admin-revoker/main_test.go index 4cb3949ba..e93cba118 100644 --- a/cmd/admin-revoker/main_test.go +++ b/cmd/admin-revoker/main_test.go @@ -73,9 +73,10 @@ func TestRevokeBatch(t *testing.T) { der, err := x509.CreateCertificate(rand.Reader, template, template, &k.PublicKey, k) test.AssertNotError(t, err, "failed to generate test cert") _, err = ssa.AddPrecertificate(context.Background(), &sapb.AddCertificateRequest{ - Der: der, - RegID: reg.ID, - Issued: time.Now().UnixNano(), + Der: der, + RegID: reg.ID, + Issued: time.Now().UnixNano(), + IssuerID: 1, }) test.AssertNotError(t, err, "failed to add test cert") now := time.Now() diff --git a/cmd/ocsp-updater/main_test.go b/cmd/ocsp-updater/main_test.go index 997481656..91eb7f342 100644 --- a/cmd/ocsp-updater/main_test.go +++ b/cmd/ocsp-updater/main_test.go @@ -93,10 +93,11 @@ func TestGenerateAndStoreOCSPResponse(t *testing.T) { parsedCert, err := core.LoadCert("test-cert.pem") test.AssertNotError(t, err, "Couldn't read test certificate") _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCert.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCert.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert.pem") @@ -117,19 +118,21 @@ func TestGenerateOCSPResponses(t *testing.T) { parsedCertA, err := core.LoadCert("test-cert.pem") test.AssertNotError(t, err, "Couldn't read test certificate") _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCertA.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCertA.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert.pem") parsedCertB, err := core.LoadCert("test-cert-b.pem") test.AssertNotError(t, err, "Couldn't read test certificate") _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCertB.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCertB.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert-b.pem") @@ -173,10 +176,11 @@ func TestFindStaleOCSPResponses(t *testing.T) { parsedCert, err := core.LoadCert("test-cert.pem") test.AssertNotError(t, err, "Couldn't read test certificate") _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCert.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCert.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert.pem") @@ -214,10 +218,11 @@ func TestFindStaleOCSPResponsesRevokedReason(t *testing.T) { parsedCert, err := core.LoadCert("test-cert.pem") test.AssertNotError(t, err, "Couldn't read test certificate") _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCert.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCert.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert.pem") @@ -246,10 +251,11 @@ func TestOldOCSPResponsesTick(t *testing.T) { parsedCert, err := core.LoadCert("test-cert.pem") test.AssertNotError(t, err, "Couldn't read test certificate") _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCert.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCert.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert.pem") @@ -277,10 +283,11 @@ func TestOldOCSPResponsesTickIsExpired(t *testing.T) { // Add a new test certificate _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCert.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCert.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert.pem") @@ -326,10 +333,11 @@ func TestStoreResponseGuard(t *testing.T) { parsedCert, err := core.LoadCert("test-cert.pem") test.AssertNotError(t, err, "Couldn't read test certificate") _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: parsedCert.Raw, - RegID: reg.ID, - Ocsp: nil, - Issued: nowNano(fc), + Der: parsedCert.Raw, + RegID: reg.ID, + Ocsp: nil, + Issued: nowNano(fc), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert.pem") @@ -383,10 +391,11 @@ func TestGenerateOCSPResponsePrecert(t *testing.T) { regID := reg.ID issuedTime := fc.Now().UnixNano() _, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: testCert.Raw, - RegID: regID, - Ocsp: ocspResp, - Issued: issuedTime, + Der: testCert.Raw, + RegID: regID, + Ocsp: ocspResp, + Issued: issuedTime, + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test-cert2.der") @@ -433,9 +442,6 @@ func TestIssuerInfo(t *testing.T) { } certA, err := x509.CreateCertificate(rand.Reader, template, template, &k.PublicKey, k) test.AssertNotError(t, err, "x509.CreateCertificate failed") - template.SerialNumber = big.NewInt(2) - certB, err := x509.CreateCertificate(rand.Reader, template, template, &k.PublicKey, k) - test.AssertNotError(t, err, "x509.CreateCertificate failed") now := fc.Now().UnixNano() id := int64(1234) @@ -447,27 +453,16 @@ func TestIssuerInfo(t *testing.T) { IssuerID: id, }) test.AssertNotError(t, err, "sa.AddPrecertificate failed") - _, err = sa.AddPrecertificate(context.Background(), &sapb.AddCertificateRequest{ - Der: certB, - RegID: reg.ID, - Ocsp: []byte{1, 2, 3}, - Issued: now, - }) - test.AssertNotError(t, err, "sa.AddPrecertificate failed") fc.Add(time.Hour * 24 * 4) statuses, err := updater.findStaleOCSPResponses(fc.Now().Add(-time.Hour), 10) test.AssertNotError(t, err, "findStaleOCSPResponses failed") - test.AssertEquals(t, len(statuses), 2) + test.AssertEquals(t, len(statuses), 1) test.AssertEquals(t, *statuses[0].IssuerID, id) - test.Assert(t, *statuses[1].IssuerID == 0, "second status doesn't have zero IssuerID") _, err = updater.generateResponse(context.Background(), statuses[0]) test.AssertNotError(t, err, "generateResponse failed") test.Assert(t, m.gotIssuer, "generateResponse didn't send issuer information and serial") - _, err = updater.generateResponse(context.Background(), statuses[1]) - test.AssertNotError(t, err, "generateResponse failed") - test.Assert(t, !m.gotIssuer, "generateResponse did send issuer information and serial when it shouldn't") } type brokenDB struct{} diff --git a/cmd/orphan-finder/main.go b/cmd/orphan-finder/main.go index 098d44e5c..349296dd8 100644 --- a/cmd/orphan-finder/main.go +++ b/cmd/orphan-finder/main.go @@ -91,9 +91,21 @@ func (t orphanType) String() string { } } +// An orphaned cert log line must contain at least the following tokens: +// "orphaning", "(pre)?certificate", "cert=[\w+]", "issuerID=[\d+]", and "regID=[\d]". +// For example: +// `[AUDIT] Failed RPC to store at SA, orphaning precertificate: serial=[04asdf1234], cert=[MIIdeafbeef], issuerID=[112358], regID=[1001], orderID=[1002], err=[Timed out]` +// The orphan-finder does not care about the serial, error, or orderID. +type parsedLine struct { + certDER []byte + issuerID int64 + regID int64 +} + var ( derOrphan = regexp.MustCompile(`cert=\[([0-9a-f]+)\]`) regOrphan = regexp.MustCompile(`regID=\[(\d+)\]`) + issuerOrphan = regexp.MustCompile(`issuerID=\[(\d+)\]`) errAlreadyExists = fmt.Errorf("Certificate already exists in DB") ) @@ -147,6 +159,41 @@ func checkDER(sai certificateStorage, der []byte) (*x509.Certificate, orphanType return nil, orphanTyp, fmt.Errorf("Existing %s lookup failed: %s", orphanTyp, err) } +func parseLogLine(line string, logger blog.Logger) (parsedLine, error) { + derStr := derOrphan.FindStringSubmatch(line) + if len(derStr) <= 1 { + return parsedLine{}, fmt.Errorf("unable to find cert der: %s", line) + } + der, err := hex.DecodeString(derStr[1]) + if err != nil { + return parsedLine{}, fmt.Errorf("unable to decode hex der from [%s]: %s", line, err) + } + + regStr := regOrphan.FindStringSubmatch(line) + if len(regStr) <= 1 { + return parsedLine{}, fmt.Errorf("unable to find regID: %s", line) + } + regID, err := strconv.ParseInt(regStr[1], 10, 64) + if err != nil { + return parsedLine{}, fmt.Errorf("unable to parse regID from [%s]: %s", line, err) + } + + issuerStr := issuerOrphan.FindStringSubmatch(line) + if len(issuerStr) <= 1 { + return parsedLine{}, fmt.Errorf("unable to find issuerID: %s", line) + } + issuerID, err := strconv.ParseInt(issuerStr[1], 10, 64) + if err != nil { + return parsedLine{}, fmt.Errorf("unable to parse issuerID from [%s]: %s", line, err) + } + + return parsedLine{ + certDER: der, + regID: regID, + issuerID: issuerID, + }, nil +} + // storeParsedLogLine attempts to parse one log line according to the format used when // orphaning certificates and precertificates. It returns two booleans and the // orphanType: The first boolean is true if the line was a match, and the second @@ -154,33 +201,23 @@ func checkDER(sai certificateStorage, der []byte) (*x509.Certificate, orphanType // orphan to the DB, it requests a fresh OCSP response from the CA to store // alongside the precertificate/certificate. func storeParsedLogLine(sa certificateStorage, ca ocspGenerator, logger blog.Logger, line string) (found bool, added bool, typ orphanType) { - ctx := context.Background() + // At a minimum, the log line should contain the word "orphaning" and the token + // "cert=". If it doesn't have those, short-circuit. + if (!strings.Contains(line, fmt.Sprintf("orphaning %s", certOrphan)) && + !strings.Contains(line, fmt.Sprintf("orphaning %s", precertOrphan))) || + !strings.Contains(line, "cert=") { + return false, false, unknownOrphan + } - // The log line should contain a label indicating it is a cert or a precert - // orphan. We will determine which it is in checkDER based on the DER instead - // of the log line label. - if !strings.Contains(line, fmt.Sprintf("orphaning %s", certOrphan)) && - !strings.Contains(line, fmt.Sprintf("orphaning %s", precertOrphan)) { - return false, false, unknownOrphan - } - // The log line should also contain certificate DER - if !strings.Contains(line, "cert=") { - return false, false, unknownOrphan - } - // Extract and decode the orphan DER - derStr := derOrphan.FindStringSubmatch(line) - if len(derStr) <= 1 { - logger.AuditErrf("Didn't match regex for cert: %s", line) - return true, false, unknownOrphan - } - der, err := hex.DecodeString(derStr[1]) + parsed, err := parseLogLine(line, logger) if err != nil { - logger.AuditErrf("Couldn't decode hex: %s, [%s]", err, line) + logger.AuditErr(fmt.Sprintf("Couldn't parse log line: %s", err)) return true, false, unknownOrphan } + // Parse the DER, determine the orphan type, and ensure it doesn't already // exist in the DB - cert, typ, err := checkDER(sa, der) + cert, typ, err := checkDER(sa, parsed.certDER) if err != nil { logFunc := logger.Errf if err == errAlreadyExists { @@ -189,22 +226,15 @@ func storeParsedLogLine(sa certificateStorage, ca ocspGenerator, logger blog.Log logFunc("%s, [%s]", err, line) return true, false, typ } - // extract the regID - regStr := regOrphan.FindStringSubmatch(line) - if len(regStr) <= 1 { - logger.AuditErrf("regID variable is empty, [%s]", line) - return true, false, typ - } - regID, err := strconv.ParseInt(regStr[1], 10, 64) - if err != nil { - logger.AuditErrf("Couldn't parse regID: %s, [%s]", err, line) - return true, false, typ - } - response, err := generateOCSP(ctx, ca, der) + + // generate an OCSP response + ctx := context.Background() + response, err := generateOCSP(ctx, ca, parsed.certDER) if err != nil { logger.AuditErrf("Couldn't generate OCSP: %s, [%s]", err, line) return true, false, typ } + // We use `cert.NotBefore` as the issued date to avoid the SA tagging this // certificate with an issued date of the current time when we know it was an // orphan issued in the past. Because certificates are backdated we need to @@ -212,13 +242,14 @@ func storeParsedLogLine(sa certificateStorage, ca ocspGenerator, logger blog.Log issuedDate := cert.NotBefore.Add(backdateDuration) switch typ { case certOrphan: - _, err = sa.AddCertificate(ctx, der, regID, response, &issuedDate) + _, err = sa.AddCertificate(ctx, parsed.certDER, parsed.regID, response, &issuedDate) case precertOrphan: _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: der, - RegID: regID, - Ocsp: response, - Issued: issuedDate.UnixNano(), + Der: parsed.certDER, + RegID: parsed.regID, + Ocsp: response, + Issued: issuedDate.UnixNano(), + IssuerID: parsed.issuerID, }) default: // Shouldn't happen but be defensive anyway diff --git a/cmd/orphan-finder/main_test.go b/cmd/orphan-finder/main_test.go index 6696c4315..6e4a67a02 100644 --- a/cmd/orphan-finder/main_test.go +++ b/cmd/orphan-finder/main_test.go @@ -61,6 +61,9 @@ func (m *mockSA) GetCertificate(ctx context.Context, s string) (core.Certificate } func (m *mockSA) AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*corepb.Empty, error) { + if core.IsAnyNilOrZero(req.Der, req.Issued, req.RegID, req.IssuerID) { + return nil, berrors.InternalServerError("Incomplete request") + } parsed, err := x509.ParseCertificate(req.Der) if err != nil { return nil, err @@ -122,12 +125,12 @@ func TestParseLine(t *testing.T) { testPreCertDER := "308204553082033da003020102021203e1dea6f3349009a90e0306dbb39c3e7ca2300d06092a864886f70d01010b0500304a310b300906035504061302555331163014060355040a130d4c6574277320456e6372797074312330210603550403131a4c6574277320456e637279707420417574686f72697479205833301e170d3139313031363132353431375a170d3230303131343132353431375a30133111300f060355040313086a756e74732e696f30820122300d06092a864886f70d01010105000382010f003082010a0282010100c91926403839aadbf2a73af4f85e3884df553880c7e9d11943121b941f284a2c805b6329a93d7fb2357c1298d811cfce61faa863c334149f948ff52a55a516e56b2d31d137b1d0319f2aabdea0e9d5e8630b54d7e53597e094c323e24a7ec1ab0db5d85651a641ec3fd7841fe5cbc675315c49b714238ead757e55409fd68c4b48d42f14c2124d381800fd2ec417ed7f363b00ab23aaddaf9113d5cf889bbf391431bffb91d425d11a1e79318b7007b8e75cc56633662c3d6c58175b5cab6225aa495361b1124642f19584820d215f23f46bd9fafa3341a0f7f387bf7cdecbccd7fcbcb3e917becb41562771e579884a0d8a1b170536f82ba90b398e9a6932150203010001a382016a30820166300e0603551d0f0101ff0404030205a0301d0603551d250416301406082b0601050507030106082b06010505070302300c0603551d130101ff04023000301d0603551d0e041604144d14d73117ca7f5a27394ed590b0d037eb5888a2301f0603551d23041830168014a84a6a63047dddbae6d139b7a64565eff3a8eca1306f06082b0601050507010104633061302e06082b060105050730018622687474703a2f2f6f6373702e696e742d78332e6c657473656e63727970742e6f7267302f06082b060105050730028623687474703a2f2f636572742e696e742d78332e6c657473656e63727970742e6f72672f30130603551d11040c300a82086a756e74732e696f304c0603551d20044530433008060667810c0102013037060b2b0601040182df130101013028302606082b06010505070201161a687474703a2f2f6370732e6c657473656e63727970742e6f72673013060a2b06010401d6790204030101ff04020500300d06092a864886f70d01010b0500038201010035f9d6620874966f2aa400f069c5f601dc11083f5859a15d20e9b1d2f9d87d3756a71a03cee0ab2a69b5173a4395b698163ba60394167c9eb4b66d20d9b3a76bf94995288e8d15c70bee969f77a71147718803e73df0a7832c1fcae1e3138601ebc61725bc7505c6d1e5b0eaf7797e09161d71e37d76370dc489312b1bf0600d1c952f846edb810c284c0d831f27481a8f2220ad178c87d8c4688023fa3798293dc9fdffa9e5b885a8107d8a2480226cd5f9121d6d7ea83b10292371ad6757e7729b27136a064f2901822b4f0ea52f8149a17860e37d3dc925488b1ba4aa26ef51e60de024e67e3d5e04ac97d8bd79a003e668ea2e1bd1c0b9d77c7cf7bfdc32" - logLine := func(typ orphanType, der, regID, orderID string) string { + logLine := func(typ orphanType, der, issuerID, regID, orderID string) string { return fmt.Sprintf( "0000-00-00T00:00:00+00:00 hostname boulder-ca[pid]: "+ "[AUDIT] Failed RPC to store at SA, orphaning %s: "+ - "cert=[%s] err=[context deadline exceeded], regID=[%s], orderID=[%s]", - typ, der, regID, orderID) + "serial=[unused], cert=[%s], issuerID=[%s], regID=[%s], orderID=[%s], err=[context deadline exceeded]", + typ, der, issuerID, regID, orderID) } testCases := []struct { @@ -148,21 +151,21 @@ func TestParseLine(t *testing.T) { }, { Name: "Empty cert in line", - LogLine: logLine(certOrphan, "", "1337", "0"), + LogLine: logLine(certOrphan, "", "1", "1337", "0"), ExpectFound: true, ExpectAdded: false, ExpectNoErrors: false, }, { Name: "Invalid cert in line", - LogLine: logLine(certOrphan, "deadbeef", "", ""), + LogLine: logLine(certOrphan, "deadbeef", "", "", ""), ExpectFound: true, ExpectAdded: false, ExpectNoErrors: false, }, { Name: "Valid cert in line", - LogLine: logLine(certOrphan, testCertDER, "1001", "0"), + LogLine: logLine(certOrphan, testCertDER, "1", "1001", "0"), ExpectFound: true, ExpectAdded: true, ExpectAddedDER: testCertDER, @@ -171,7 +174,7 @@ func TestParseLine(t *testing.T) { }, { Name: "Already inserted cert in line", - LogLine: logLine(certOrphan, testCertDER, "1001", "0"), + LogLine: logLine(certOrphan, testCertDER, "1", "1001", "0"), ExpectFound: true, // ExpectAdded is false because we have already added this cert in the // previous "Valid cert in line" test case. @@ -180,21 +183,21 @@ func TestParseLine(t *testing.T) { }, { Name: "Empty precert in line", - LogLine: logLine(precertOrphan, "", "1337", "0"), + LogLine: logLine(precertOrphan, "", "1", "1337", "0"), ExpectFound: true, ExpectAdded: false, ExpectNoErrors: false, }, { Name: "Invalid precert in line", - LogLine: logLine(precertOrphan, "deadbeef", "", ""), + LogLine: logLine(precertOrphan, "deadbeef", "", "", ""), ExpectFound: true, ExpectAdded: false, ExpectNoErrors: false, }, { Name: "Valid precert in line", - LogLine: logLine(precertOrphan, testPreCertDER, "9999", "0"), + LogLine: logLine(precertOrphan, testPreCertDER, "1", "9999", "0"), ExpectFound: true, ExpectAdded: true, ExpectAddedDER: testPreCertDER, @@ -203,7 +206,7 @@ func TestParseLine(t *testing.T) { }, { Name: "Already inserted precert in line", - LogLine: logLine(precertOrphan, testPreCertDER, "1001", "0"), + LogLine: logLine(precertOrphan, testPreCertDER, "1", "1001", "0"), ExpectFound: true, // ExpectAdded is false because we have already added this cert in the // previous "Valid cert in line" test case. @@ -212,11 +215,25 @@ func TestParseLine(t *testing.T) { }, { Name: "Unknown orphan type", - LogLine: logLine(unknownOrphan, testPreCertDER, "1001", "0"), + LogLine: logLine(unknownOrphan, testPreCertDER, "1", "1001", "0"), ExpectFound: false, ExpectAdded: false, ExpectNoErrors: false, }, + { + Name: "Empty issuerID in line", + LogLine: logLine(precertOrphan, testPreCertDER, "", "1001", "0"), + ExpectFound: true, + ExpectAdded: false, + ExpectNoErrors: false, + }, + { + Name: "Zero issuerID in line", + LogLine: logLine(precertOrphan, testPreCertDER, "0", "1001", "0"), + ExpectFound: true, + ExpectAdded: false, + ExpectNoErrors: false, + }, } for _, tc := range testCases { diff --git a/sa/precertificates.go b/sa/precertificates.go index 436b8213e..d45dc4a9c 100644 --- a/sa/precertificates.go +++ b/sa/precertificates.go @@ -37,7 +37,7 @@ func (ssa *SQLStorageAuthority) AddSerial(ctx context.Context, req *sapb.AddSeri // AddPrecertificate writes a record of a precertificate generation to the DB. func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb.AddCertificateRequest) (*corepb.Empty, error) { - if core.IsAnyNilOrZero(req.Der, req.Issued, req.RegID) { + if core.IsAnyNilOrZero(req.Der, req.Issued, req.RegID, req.IssuerID) { return nil, errIncompleteRequest } parsed, err := x509.ParseCertificate(req.Der) diff --git a/sa/precertificates_test.go b/sa/precertificates_test.go index 55352fdfe..d83cf08fb 100644 --- a/sa/precertificates_test.go +++ b/sa/precertificates_test.go @@ -44,10 +44,11 @@ func TestAddPrecertificate(t *testing.T) { regID := reg.ID issuedTime := time.Date(2018, 4, 1, 7, 0, 0, 0, time.UTC) _, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: testCert.Raw, - RegID: regID, - Ocsp: ocspResp, - Issued: issuedTime.UnixNano(), + Der: testCert.Raw, + RegID: regID, + Ocsp: ocspResp, + Issued: issuedTime.UnixNano(), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add test cert") @@ -86,10 +87,11 @@ func TestAddPrecertificate(t *testing.T) { // Adding the same certificate with the same serial should result in an // error _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: testCert.Raw, - RegID: regID, - Ocsp: ocspResp, - Issued: issuedTime.UnixNano(), + Der: testCert.Raw, + RegID: regID, + Ocsp: ocspResp, + Issued: issuedTime.UnixNano(), + IssuerID: 1, }) if err == nil { t.Fatalf("Expected error inserting duplicate precertificate, got none") @@ -102,6 +104,30 @@ func TestAddPrecertificate(t *testing.T) { addPrecert(true) } +func TestAddPrecertificateIncomplete(t *testing.T) { + sa, _, cleanUp := initSA(t) + defer cleanUp() + + reg := satest.CreateWorkingRegistration(t, sa) + + // Create a throw-away self signed certificate with a random name and + // serial number + _, testCert := test.ThrowAwayCert(t, 1) + + // Add the cert as a precertificate + ocspResp := []byte{0, 0, 1} + regID := reg.ID + _, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ + Der: testCert.Raw, + RegID: regID, + Ocsp: ocspResp, + Issued: time.Date(2018, 4, 1, 7, 0, 0, 0, time.UTC).UnixNano(), + // Leaving out IssuerID + }) + + test.AssertError(t, err, "Adding precert with no issuer did not fail") +} + func TestAddPrecertificateKeyHash(t *testing.T) { sa, _, cleanUp := initSA(t) defer cleanUp() @@ -109,10 +135,11 @@ func TestAddPrecertificateKeyHash(t *testing.T) { serial, testCert := test.ThrowAwayCert(t, 1) _, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: testCert.Raw, - RegID: reg.ID, - Ocsp: []byte{1, 2, 3}, - Issued: testCert.NotBefore.UnixNano(), + Der: testCert.Raw, + RegID: reg.ID, + Ocsp: []byte{1, 2, 3}, + Issued: testCert.NotBefore.UnixNano(), + IssuerID: 1, }) test.AssertNotError(t, err, "failed to add precert") diff --git a/sa/sa_test.go b/sa/sa_test.go index 911760bbb..edea96969 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -643,11 +643,11 @@ func TestPreviousCertificateExists(t *testing.T) { test.AssertNotError(t, err, "reading cert DER") issued := sa.clk.Now() - issuedUnix := issued.UnixNano() _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: certDER, - Issued: issuedUnix, - RegID: reg.ID, + Der: certDER, + Issued: issued.UnixNano(), + RegID: reg.ID, + IssuerID: 1, }) test.AssertNotError(t, err, "Failed to add precertificate") _, err = sa.AddCertificate(ctx, certDER, reg.ID, nil, &issued) @@ -1563,12 +1563,12 @@ func TestRevokeCertificate(t *testing.T) { // Add a cert to the DB to test with. certDER, err := ioutil.ReadFile("www.eff.org.der") test.AssertNotError(t, err, "Couldn't read example cert DER") - issued := sa.clk.Now().UnixNano() _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: certDER, - RegID: reg.ID, - Ocsp: nil, - Issued: issued, + Der: certDER, + RegID: reg.ID, + Ocsp: nil, + Issued: sa.clk.Now().UnixNano(), + IssuerID: 1, }) test.AssertNotError(t, err, "Couldn't add www.eff.org.der") @@ -1634,11 +1634,11 @@ func TestAddCertificateRenewalBit(t *testing.T) { test.AssertNotError(t, tx.Commit(), "Failed to commit transaction") // Add the certificate with the same names. - issuedUnix := issued.UnixNano() _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: certDER, - Issued: issuedUnix, - RegID: reg.ID, + Der: certDER, + Issued: issued.UnixNano(), + RegID: reg.ID, + IssuerID: 1, }) test.AssertNotError(t, err, "Failed to add precertificate") _, err = sa.AddCertificate(ctx, certDER, reg.ID, nil, &issued) @@ -1671,11 +1671,11 @@ func TestAddCertificateRenewalBit(t *testing.T) { test.AssertNotError(t, err, "Unexpected error parsing test-cert.der test file") names = cert.DNSNames - issuedUnix = issued.UnixNano() _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: certDER, - Issued: issuedUnix, - RegID: reg.ID, + Der: certDER, + Issued: issued.UnixNano(), + RegID: reg.ID, + IssuerID: 1, }) test.AssertNotError(t, err, "Failed to add precertificate") _, err = sa.AddCertificate(ctx, certDER, reg.ID, nil, &issued) diff --git a/test/integration/orphan_finder_test.go b/test/integration/orphan_finder_test.go index e35160e2f..743306c05 100644 --- a/test/integration/orphan_finder_test.go +++ b/test/integration/orphan_finder_test.go @@ -23,8 +23,8 @@ import ( "github.com/letsencrypt/pkcs11key/v4" ) -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]` +var template = `[AUDIT] Failed RPC to store at SA, orphaning precertificate: serial=[%x] cert=[%x] err=[sa.StorageAuthority.AddCertificate timed out after 5000 ms], issuerID=[1], 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], issuerID=[1], 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