diff --git a/cmd/ocsp-updater/main_test.go b/cmd/ocsp-updater/main_test.go index 5f0d2b458..ff6212c56 100644 --- a/cmd/ocsp-updater/main_test.go +++ b/cmd/ocsp-updater/main_test.go @@ -7,7 +7,6 @@ import ( "crypto/x509" "errors" "fmt" - "io/ioutil" "math/big" "os" "strings" @@ -450,16 +449,16 @@ func TestGenerateOCSPResponsePrecert(t *testing.T) { reg := satest.CreateWorkingRegistration(t, sa) + // Create a throw-away self signed certificate with some names + serial, testCert := test.ThrowAwayCert(t, 5) + // Use AddPrecertificate to set up a precertificate, serials, and // certificateStatus row for the testcert. - certDER, err := ioutil.ReadFile("../../test/test-ca.der") - test.AssertNotError(t, err, "Couldn't read example cert DER") - serial := "00000000000000000000000000000000124d" ocspResp := []byte{0, 0, 1} regID := reg.ID issuedTime := fc.Now().UnixNano() - _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: certDER, + _, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ + Der: testCert.Raw, RegID: ®ID, Ocsp: ocspResp, Issued: &issuedTime, diff --git a/features/featureflag_string.go b/features/featureflag_string.go index 338ef3ec7..54058ff36 100644 --- a/features/featureflag_string.go +++ b/features/featureflag_string.go @@ -39,11 +39,12 @@ func _() { _ = x[PrecertificateRevocation-28] _ = x[StripDefaultSchemePort-29] _ = x[StoreIssuerInfo-30] + _ = x[WriteIssuedNamesPrecert-31] } -const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRANewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitFasterGetOrderForNamesPrecertificateOCSPGetAuthorizationsPerfCAAValidationMethodsCAAAccountURIHeadNonceStatusOKEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGETAllowV1RegistrationParallelCheckFailedValidationDeleteUnusedChallengesV1DisableNewValidationsPrecertificateRevocationStripDefaultSchemePortStoreIssuerInfo" +const _FeatureFlag_name = "unusedPerformValidationRPCACME13KeyRolloverSimplifiedVAHTTPTLSSNIRevalidationAllowRenewalFirstRLSetIssuedNamesRenewalBitFasterRateLimitProbeCTLogsRevokeAtRANewAuthorizationSchemaDisableAuthz2OrdersEarlyOrderRateLimitFasterGetOrderForNamesPrecertificateOCSPGetAuthorizationsPerfCAAValidationMethodsCAAAccountURIHeadNonceStatusOKEnforceMultiVAMultiVAFullResultsRemoveWFE2AccountIDCheckRenewalFirstMandatoryPOSTAsGETAllowV1RegistrationParallelCheckFailedValidationDeleteUnusedChallengesV1DisableNewValidationsPrecertificateRevocationStripDefaultSchemePortStoreIssuerInfoWriteIssuedNamesPrecert" -var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 178, 197, 216, 238, 256, 277, 297, 310, 327, 341, 359, 378, 395, 413, 432, 461, 483, 506, 530, 552, 567} +var _FeatureFlag_index = [...]uint16{0, 6, 26, 43, 59, 77, 96, 120, 135, 146, 156, 178, 197, 216, 238, 256, 277, 297, 310, 327, 341, 359, 378, 395, 413, 432, 461, 483, 506, 530, 552, 567, 590} func (i FeatureFlag) String() string { if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) { diff --git a/features/features.go b/features/features.go index 81965ddce..704630ba5 100644 --- a/features/features.go +++ b/features/features.go @@ -69,6 +69,10 @@ const ( // StoreIssuerInfo enables storage of information identifying the issuer of // a certificate in the certificateStatus table. StoreIssuerInfo + // WriteIssuedNamesPrecert moves the issuedNames and fqdnSet insertions from + // happening when final certificates are saved by the SA to when + // precertificates are saved. + WriteIssuedNamesPrecert ) // List of features and their default value, protected by fMu @@ -104,6 +108,7 @@ var features = map[FeatureFlag]bool{ StripDefaultSchemePort: false, GetAuthorizationsPerf: false, StoreIssuerInfo: false, + WriteIssuedNamesPrecert: false, } var fMu = new(sync.RWMutex) diff --git a/sa/precertificates.go b/sa/precertificates.go index 8ddf817fc..9904aceb0 100644 --- a/sa/precertificates.go +++ b/sa/precertificates.go @@ -11,6 +11,7 @@ import ( "github.com/letsencrypt/boulder/core" corepb "github.com/letsencrypt/boulder/core/proto" + "github.com/letsencrypt/boulder/db" berrors "github.com/letsencrypt/boulder/errors" "github.com/letsencrypt/boulder/features" bgrpc "github.com/letsencrypt/boulder/grpc" @@ -49,56 +50,87 @@ func (ssa *SQLStorageAuthority) AddPrecertificate(ctx context.Context, req *sapb } issued := time.Unix(0, *req.Issued) serialHex := core.SerialToString(parsed.SerialNumber) - err = ssa.dbMap.WithContext(ctx).Insert(&precertificateModel{ + + preCertModel := &precertificateModel{ Serial: serialHex, RegistrationID: *req.RegID, DER: req.Der, Issued: issued, Expires: parsed.NotAfter, - }) - if err != nil { - if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") { - return nil, berrors.DuplicateError("cannot add a duplicate precertificate") + } + + _, overallError := db.WithTransaction(ctx, ssa.dbMap, func(txWithCtx db.Transaction) (interface{}, error) { + err = txWithCtx.Insert(preCertModel) + if err != nil { + if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") { + return nil, berrors.DuplicateError("cannot add a duplicate precertificate") + } + return nil, err } - return nil, err - } - // With feature.StoreIssuerInfo we've added a new field to certStatusModel - // so when we try and use dbMap.Insert it will always try to insert that field. - // That will break when the relevant migration hasn't been applied so we need - // to use an explicit INSERT statement that we can manipulate to include the - // field only when the feature is enabled (and as such the migration has been - // applied). - csFields := certStatusFields - if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil { - csFields += ", issuerID" - } - qmarks := []string{} - for range strings.Split(csFields, ",") { - qmarks = append(qmarks, "?") - } - args := []interface{}{ - serialHex, // serial - string(core.OCSPStatusGood), // stauts - ssa.clk.Now(), // ocspLastUpdated - time.Time{}, // revokedDate - 0, // revokedReason - time.Time{}, // lastExpirationNagSent - req.Ocsp, // ocspResponse - parsed.NotAfter, // notAfter - false, // isExpired - } - if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil { - args = append(args, req.IssuerID) - } + // With feature.StoreIssuerInfo we've added a new field to certStatusModel + // so when we try and use dbMap.Insert it will always try to insert that field. + // That will break when the relevant migration hasn't been applied so we need + // to use an explicit INSERT statement that we can manipulate to include the + // field only when the feature is enabled (and as such the migration has been + // applied). + csFields := certStatusFields + if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil { + csFields += ", issuerID" + } + qmarks := []string{} + for range strings.Split(csFields, ",") { + qmarks = append(qmarks, "?") + } + args := []interface{}{ + serialHex, // serial + string(core.OCSPStatusGood), // status + ssa.clk.Now(), // ocspLastUpdated + time.Time{}, // revokedDate + 0, // revokedReason + time.Time{}, // lastExpirationNagSent + req.Ocsp, // ocspResponse + parsed.NotAfter, // notAfter + false, // isExpired + } + if features.Enabled(features.StoreIssuerInfo) && req.IssuerID != nil { + args = append(args, req.IssuerID) + } - _, err = ssa.dbMap.WithContext(ctx).Exec(fmt.Sprintf( - "INSERT INTO certificateStatus (%s) VALUES (%s)", - csFields, - strings.Join(qmarks, ","), - ), args...) - if err != nil { - return nil, err + _, err = ssa.dbMap.WithContext(ctx).Exec(fmt.Sprintf( + "INSERT INTO certificateStatus (%s) VALUES (%s)", + csFields, + strings.Join(qmarks, ","), + ), args...) + if err != nil { + return nil, err + } + + // NOTE(@cpu): When we collect up names to check if an FQDN set exists (e.g. + // that it is a renewal) we use just the DNSNames from the certificate and + // ignore the Subject Common Name (if any). This is a safe assumption because + // if a certificate we issued were to have a Subj. CN not present as a SAN it + // would be a misissuance and miscalculating whether the cert is a renewal or + // not for the purpose of rate limiting is the least of our troubles. + isRenewal, err := ssa.checkFQDNSetExists( + txWithCtx.SelectOne, + parsed.DNSNames) + if err != nil { + return nil, err + } + + // If the WriteIssuedNamesPrecert feature flag is *enabled* then we need to + // record the issued names from the precertificate being added by the SA. + if features.Enabled(features.WriteIssuedNamesPrecert) { + if err := addIssuedNames(txWithCtx, parsed, isRenewal); err != nil { + return nil, err + } + } + + return nil, nil + }) + if overallError != nil { + return nil, overallError } return &corepb.Empty{}, nil } diff --git a/sa/precertificates_test.go b/sa/precertificates_test.go index 7ec7ad35c..cfab485f7 100644 --- a/sa/precertificates_test.go +++ b/sa/precertificates_test.go @@ -2,19 +2,35 @@ package sa import ( "bytes" + "database/sql" "fmt" - "io/ioutil" "os" "strings" "testing" "time" + "github.com/letsencrypt/boulder/db" berrors "github.com/letsencrypt/boulder/errors" + "github.com/letsencrypt/boulder/features" sapb "github.com/letsencrypt/boulder/sa/proto" "github.com/letsencrypt/boulder/sa/satest" "github.com/letsencrypt/boulder/test" ) +// findIssuedName is a small helper test function to directly query the +// issuedNames table for a given name to find a serial (or return an err). +func findIssuedName(dbMap db.OneSelector, name string) (string, error) { + var issuedNamesSerial string + err := dbMap.SelectOne( + &issuedNamesSerial, + `SELECT serial FROM issuedNames + WHERE reversedName = ? + ORDER BY notBefore DESC + LIMIT 1`, + ReverseName(name)) + return issuedNamesSerial, err +} + func TestAddPrecertificate(t *testing.T) { if !strings.HasSuffix(os.Getenv("BOULDER_CONFIG_DIR"), "config-next") { return @@ -25,43 +41,79 @@ func TestAddPrecertificate(t *testing.T) { reg := satest.CreateWorkingRegistration(t, sa) - certDER, err := ioutil.ReadFile("test-cert2.der") - test.AssertNotError(t, err, "Couldn't read example cert DER") - serial := "ffa0160630d618b2eb5c0510824b14274856" - ocspResp := []byte{0, 0, 1} - regID := reg.ID - issuedTime := time.Date(2018, 4, 1, 7, 0, 0, 0, time.UTC).UnixNano() - _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: certDER, - RegID: ®ID, - Ocsp: ocspResp, - Issued: &issuedTime, - }) - test.AssertNotError(t, err, "Couldn't add test-cert2.der") + addPrecert := func(expectIssuedNamesUpdate bool) { + // Create a throw-away self signed certificate with a random name and + // serial number + serial, testCert := test.ThrowAwayCert(t, 1) - certStatus, err := sa.GetCertificateStatus(ctx, serial) - test.AssertNotError(t, err, "Couldn't get status for test-cert2.der") - test.Assert( - t, - bytes.Compare(certStatus.OCSPResponse, ocspResp) == 0, - fmt.Sprintf("OCSP responses don't match, expected: %x, got %x", certStatus.OCSPResponse, ocspResp), - ) - test.Assert( - t, - clk.Now().Equal(certStatus.OCSPLastUpdated), - fmt.Sprintf("OCSPLastUpdated doesn't match, expected %s, got %s", clk.Now(), certStatus.OCSPLastUpdated), - ) + // Add the cert as a precertificate + ocspResp := []byte{0, 0, 1} + regID := reg.ID + issuedTime := time.Date(2018, 4, 1, 7, 0, 0, 0, time.UTC) + issuedTimeNano := issuedTime.UnixNano() + _, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ + Der: testCert.Raw, + RegID: ®ID, + Ocsp: ocspResp, + Issued: &issuedTimeNano, + }) + test.AssertNotError(t, err, "Couldn't add test cert") - _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ - Der: certDER, - RegID: ®ID, - Ocsp: ocspResp, - Issued: &issuedTime, - }) - if err == nil { - t.Fatalf("Expected error inserting duplicate precertificate, got none") - } - if !berrors.Is(err, berrors.Duplicate) { - t.Fatalf("Expected berrors.Duplicate inserting duplicate precertificate, got %#v", err) + // It should have the expected certificate status + certStatus, err := sa.GetCertificateStatus(ctx, serial) + test.AssertNotError(t, err, "Couldn't get status for test cert") + test.Assert( + t, + bytes.Compare(certStatus.OCSPResponse, ocspResp) == 0, + fmt.Sprintf("OCSP responses don't match, expected: %x, got %x", certStatus.OCSPResponse, ocspResp), + ) + test.Assert( + t, + clk.Now().Equal(certStatus.OCSPLastUpdated), + fmt.Sprintf("OCSPLastUpdated doesn't match, expected %s, got %s", clk.Now(), certStatus.OCSPLastUpdated), + ) + + issuedNamesSerial, err := findIssuedName(sa.dbMap, testCert.DNSNames[0]) + if expectIssuedNamesUpdate { + // If we expectIssuedNamesUpdate then there should be no err and the + // expected serial + test.AssertNotError(t, err, "expected no err querying issuedNames for precert") + test.AssertEquals(t, issuedNamesSerial, serial) + + // We should also be able to call AddCertificate with the same cert + // without it being an error. The duplicate err on inserting to + // issuedNames should be ignored. + _, err := sa.AddCertificate(ctx, testCert.Raw, regID, nil, &issuedTime) + test.AssertNotError(t, err, "unexpected err adding final cert after precert") + } else { + // Otherwise we expect sql.ErrNoRows because AddCertificate not + // AddPrecertificate will be updating this table. + test.AssertEquals(t, err, sql.ErrNoRows) + } + + // Adding the same certificate with the same serial should result in an + // error + _, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{ + Der: testCert.Raw, + RegID: ®ID, + Ocsp: ocspResp, + Issued: &issuedTimeNano, + }) + if err == nil { + t.Fatalf("Expected error inserting duplicate precertificate, got none") + } + if !berrors.Is(err, berrors.Duplicate) { + t.Fatalf("Expected berrors.Duplicate inserting duplicate precertificate, got %#v", err) + } } + + // With no feature flag set we don't expect addPrecertficate to update + // issuedNames + addPrecert(false) + defer features.Reset() + // With the feature flag set we do expect addPrecertificate to update + // issuedNames + err := features.Set(map[string]bool{"WriteIssuedNamesPrecert": true}) + test.AssertNotError(t, err, "failed to set WriteIssuedNamesPrecert feature flag") + addPrecert(true) } diff --git a/sa/sa.go b/sa/sa.go index 5efc2ea9b..2aada09c5 100644 --- a/sa/sa.go +++ b/sa/sa.go @@ -463,6 +463,7 @@ func (ssa *SQLStorageAuthority) AddCertificate( } _, overallError := db.WithTransaction(ctx, ssa.dbMap, func(txWithCtx db.Transaction) (interface{}, error) { + // Save the final certificate err = txWithCtx.Insert(cert) if err != nil { if strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") { @@ -484,33 +485,41 @@ func (ssa *SQLStorageAuthority) AddCertificate( return nil, err } - err = addIssuedNames(txWithCtx, parsedCertificate, isRenewal) - if err != nil { - return nil, err + // Record the issued names from the final certificate being added by the SA. + // If features.WriteIssuedNamesPrecert was enabled when the corresponding + // precertificate was added in AddPrecertificate then this will prompt + // a duplicate entry error that we can safely ignore. + // + // TODO(@cpu): Once features.WriteIssuedNamesPrecert has been deployed + // globally we can remove this call to ssa.addIssuedNames from + // AddCertificate + if err := addIssuedNames(txWithCtx, parsedCertificate, isRenewal); err != nil { + // if it wasn't a duplicate entry error, return the err. Otherwise ignore + // it. + if !strings.HasPrefix(err.Error(), "Error 1062: Duplicate entry") { + return nil, err + } } // Add to the rate limit table, but only for new certificates. Renewals // don't count against the certificatesPerName limit. if !isRenewal { timeToTheHour := parsedCertificate.NotBefore.Round(time.Hour) - err = ssa.addCertificatesPerName(ctx, txWithCtx, parsedCertificate.DNSNames, timeToTheHour) - if err != nil { + if err := ssa.addCertificatesPerName(ctx, txWithCtx, parsedCertificate.DNSNames, timeToTheHour); err != nil { return nil, err } } + // Update the FQDN sets now that there is a final certificate to ensure rate + // limits are calculated correctly. err = addFQDNSet( txWithCtx, parsedCertificate.DNSNames, - serial, + core.SerialToString(parsedCertificate.SerialNumber), parsedCertificate.NotBefore, parsedCertificate.NotAfter, ) - if err != nil { - return nil, err - } - - return digest, nil + return nil, err }) if overallError != nil { return "", overallError diff --git a/sa/sa_test.go b/sa/sa_test.go index 5e673f6d6..1a2173ff5 100644 --- a/sa/sa_test.go +++ b/sa/sa_test.go @@ -185,6 +185,7 @@ func TestNoSuchRegistrationErrors(t *testing.T) { t.Errorf("UpdateRegistration: expected a berrors.NotFound type error, got %T type error (%v)", err, err) } } + func TestAddCertificate(t *testing.T) { sa, clk, cleanUp := initSA(t) defer cleanUp() @@ -233,6 +234,26 @@ func TestAddCertificate(t *testing.T) { ocspResp := []byte{0, 0, 1} _, err = sa.AddCertificate(ctx, certDER3, reg.ID, ocspResp, &issuedTime) test.AssertNotError(t, err, "Couldn't add test-cert2.der") + + // Test adding a certificate with the features.WriteIssuedNamesPrecert feature + // flag enabled doesn't result in issuedNames and fqdnSet updates since we + // expect AddPrecertificate to handle it in this case. + err = features.Set(map[string]bool{"WriteIssuedNamesPrecert": true}) + test.AssertNotError(t, err, "failed to set WriteIssuedNamesPrecert feature flag") + + // Create a throw-away self signed certificate with a random name and + // serial number + _, testCert := test.ThrowAwayCert(t, 1) + + // Add the test cert + _, err = sa.AddCertificate(ctx, testCert.Raw, reg.ID, ocspResp, &issuedTime) + test.AssertNotError(t, err, "unexpected error adding testcert") + + // Check the issuedNames table + _, err = findIssuedName(sa.dbMap, testCert.DNSNames[0]) + // We expect no error because AddCertificate should have updated the issued + // names table. + test.AssertNotError(t, err, "unexpected error finding issued names after addCert") } func TestCountCertificatesByNames(t *testing.T) { diff --git a/test/config-next/sa.json b/test/config-next/sa.json index 5a6ae9b87..eaacce07e 100644 --- a/test/config-next/sa.json +++ b/test/config-next/sa.json @@ -26,7 +26,8 @@ "features": { "DeleteUnusedChallenges": true, "FasterGetOrderForNames": true, - "StoreIssuerInfo": true + "StoreIssuerInfo": true, + "WriteIssuedNamesPrecert": true } }, diff --git a/test/test-tools.go b/test/test-tools.go index f3d46a0df..b4ce9c12e 100644 --- a/test/test-tools.go +++ b/test/test-tools.go @@ -2,9 +2,14 @@ package test import ( "bytes" + "crypto/rand" + "crypto/rsa" + "crypto/x509" "encoding/base64" + "encoding/hex" "encoding/json" "fmt" + "math/big" "reflect" "runtime" "strings" @@ -204,3 +209,34 @@ func GaugeValueWithLabels(vecGauge *prometheus.GaugeVec, labels prometheus.Label return int(iom.Gauge.GetValue()), nil } + +// ThrowAwayCert is a small test helper function that creates a self-signed +// certificate for nameCount random example.com subdomains and returns the +// parsed certificate and the random serial in string form or aborts the test. +// The certificate returned from this function is the bare minimum needed for +// most tests and isn't a robust example of a complete end entity certificate. +func ThrowAwayCert(t *testing.T, nameCount int) (string, *x509.Certificate) { + k, err := rsa.GenerateKey(rand.Reader, 512) + AssertNotError(t, err, "rsa.GenerateKey failed") + + var serialBytes [16]byte + _, _ = rand.Read(serialBytes[:]) + serialNum := big.NewInt(0).SetBytes(serialBytes[:]) + + var names []string + for i := 0; i < nameCount; i++ { + var nameBytes [3]byte + _, _ = rand.Read(nameBytes[:]) + names = append(names, fmt.Sprintf("%s.example.com", hex.EncodeToString(nameBytes[:]))) + } + + template := &x509.Certificate{ + SerialNumber: serialNum, + DNSNames: names, + } + testCertDER, err := x509.CreateCertificate(rand.Reader, template, template, &k.PublicKey, k) + AssertNotError(t, err, "x509.CreateCertificate failed") + testCert, err := x509.ParseCertificate(testCertDER) + AssertNotError(t, err, "failed to parse self-signed cert DER") + return fmt.Sprintf("%036x", serialNum), testCert +}