Revert "Use GetRevocationStatus instead of GetCertificateStatus" (#8426)

This reverts #8402

The revokedCertificates table does not have an index on the `serial`
column, unlike the certificateStatus table. This means that these
highly-targeted, single-certificate lookups are actually very
inefficient, and led to performance problems in the database.

This revert will be followed by a schema change to add an index to the
revokedCertificates table, and then by a reland of the original PR.

Part of #8322

Author: aarongable

core/objects.go

@@ -83,11 +83,6 @@ var OCSPStatusToInt = map[OCSPStatus]int{
 	OCSPStatusRevoked: ocsp.Revoked,
 }
 
-const (
-	RevocationStatusGood    int64 = 0
-	RevocationStatusRevoked int64 = 1
-)
-
 // DNSPrefix is attached to DNS names in DNS challenges
 const DNSPrefix = "_acme-challenge"
 

mocks/sa.go

@@ -213,7 +213,7 @@ func (sa *StorageAuthorityReadOnly) GetCertificateStatus(_ context.Context, req
 
 // GetRevocationStatus is a mock
 func (sa *StorageAuthorityReadOnly) GetRevocationStatus(_ context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*sapb.RevocationStatus, error) {
-	return nil, errors.New("no revocation status")
+	return nil, nil
 }
 
 // SerialsForIncident is a mock

ra/ra.go

@@ -1683,12 +1683,12 @@ func (ra *RegistrationAuthorityImpl) revokeCertificate(ctx context.Context, cert
 // certificates that were previously revoked for a reason other than
 // keyCompromise, and which are now being updated to keyCompromise instead.
 func (ra *RegistrationAuthorityImpl) updateRevocationForKeyCompromise(ctx context.Context, serialString string, issuerID issuance.NameID) error {
-	status, err := ra.SA.GetRevocationStatus(ctx, &sapb.Serial{Serial: serialString})
+	status, err := ra.SA.GetCertificateStatus(ctx, &sapb.Serial{Serial: serialString})
 	if err != nil {
 		return berrors.NotFoundError("unable to confirm that serial %q was ever issued: %s", serialString, err)
 	}
 
-	if status.Status != core.RevocationStatusRevoked {
+	if status.Status != string(core.OCSPStatusRevoked) {
 		// Internal server error, because we shouldn't be in the function at all
 		// unless the cert was already revoked.
 		return fmt.Errorf("unable to re-revoke serial %q which is not currently revoked", serialString)

ra/ra_test.go

@@ -3511,20 +3511,20 @@ type mockSARevocation struct {
 	sapb.StorageAuthorityClient
 
 	known   map[string]*x509.Certificate
-	revoked map[string]*sapb.RevocationStatus
+	revoked map[string]*corepb.CertificateStatus
 	blocked []*sapb.AddBlockedKeyRequest
 }
 
 func newMockSARevocation(known *x509.Certificate) *mockSARevocation {
 	return &mockSARevocation{
 		known:   map[string]*x509.Certificate{core.SerialToString(known.SerialNumber): known},
-		revoked: make(map[string]*sapb.RevocationStatus),
+		revoked: make(map[string]*corepb.CertificateStatus),
 		blocked: make([]*sapb.AddBlockedKeyRequest, 0),
 	}
 }
 
 func (msar *mockSARevocation) reset() {
-	msar.revoked = make(map[string]*sapb.RevocationStatus)
+	msar.revoked = make(map[string]*corepb.CertificateStatus)
 	msar.blocked = make([]*sapb.AddBlockedKeyRequest, 0)
 }
 
@@ -3552,13 +3552,14 @@ func (msar *mockSARevocation) GetLintPrecertificate(_ context.Context, req *sapb
 	return nil, berrors.UnknownSerialError()
 }
 
-func (msar *mockSARevocation) GetRevocationStatus(_ context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*sapb.RevocationStatus, error) {
+func (msar *mockSARevocation) GetCertificateStatus(_ context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*corepb.CertificateStatus, error) {
 	if status, present := msar.revoked[req.Serial]; present {
 		return status, nil
 	}
-	if _, present := msar.known[req.Serial]; present {
-		return &sapb.RevocationStatus{
-			Status: core.RevocationStatusGood,
+	if cert, present := msar.known[req.Serial]; present {
+		return &corepb.CertificateStatus{
+			Serial:   core.SerialToString(cert.SerialNumber),
+			IssuerID: int64(issuance.IssuerNameID(cert)),
 		}, nil
 	}
 	return nil, berrors.UnknownSerialError()
@@ -3597,12 +3598,14 @@ func (msar *mockSARevocation) RevokeCertificate(_ context.Context, req *sapb.Rev
 	if _, present := msar.revoked[req.Serial]; present {
 		return nil, berrors.AlreadyRevokedError("already revoked")
 	}
-	_, present := msar.known[req.Serial]
+	cert, present := msar.known[req.Serial]
 	if !present {
 		return nil, berrors.UnknownSerialError()
 	}
-	msar.revoked[req.Serial] = &sapb.RevocationStatus{
-		Status:        core.RevocationStatusRevoked,
+	msar.revoked[req.Serial] = &corepb.CertificateStatus{
+		Serial:        req.Serial,
+		IssuerID:      int64(issuance.IssuerNameID(cert)),
+		Status:        string(core.OCSPStatusRevoked),
 		RevokedReason: req.Reason,
 	}
 	return &emptypb.Empty{}, nil
@@ -3769,7 +3772,7 @@ func TestRevokeCertByKey(t *testing.T) {
 
 	// Reset and have the Subscriber revoke for a different reason.
 	// Then re-revoking using the key should work.
-	mockSA.revoked = make(map[string]*sapb.RevocationStatus)
+	mockSA.revoked = make(map[string]*corepb.CertificateStatus)
 	_, err = ra.RevokeCertByApplicant(context.Background(), &rapb.RevokeCertByApplicantRequest{
 		Cert:  cert.Raw,
 		Code:  int64(revocation.Unspecified),

sa/model.go

@@ -161,6 +161,41 @@ func SelectCertificateStatus(ctx context.Context, s db.OneSelector, serial strin
 	return model.toPb(), err
 }
 
+// RevocationStatusModel represents a small subset of the columns in the
+// certificateStatus table, used to determine the authoritative revocation
+// status of a certificate.
+type RevocationStatusModel struct {
+	Status        core.OCSPStatus   `db:"status"`
+	RevokedDate   time.Time         `db:"revokedDate"`
+	RevokedReason revocation.Reason `db:"revokedReason"`
+}
+
+// SelectRevocationStatus returns the authoritative revocation information for
+// the certificate with the given serial.
+func SelectRevocationStatus(ctx context.Context, s db.OneSelector, serial string) (*sapb.RevocationStatus, error) {
+	var model RevocationStatusModel
+	err := s.SelectOne(
+		ctx,
+		&model,
+		"SELECT status, revokedDate, revokedReason FROM certificateStatus WHERE serial = ? LIMIT 1",
+		serial,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	statusInt, ok := core.OCSPStatusToInt[model.Status]
+	if !ok {
+		return nil, fmt.Errorf("got unrecognized status %q", model.Status)
+	}
+
+	return &sapb.RevocationStatus{
+		Status:        int64(statusInt),
+		RevokedDate:   timestamppb.New(model.RevokedDate),
+		RevokedReason: int64(model.RevokedReason),
+	}, nil
+}
+
 var mediumBlobSize = int(math.Pow(2, 24))
 
 type issuedNameModel struct {

sa/sa_test.go

@@ -396,19 +396,15 @@ func TestAddPrecertificate(t *testing.T) {
 	defer cleanUp()
 
 	reg := createWorkingRegistration(t, sa)
-	regID := reg.Id
 
-	// Add a cert to the DB to test with.
+	// Create a throw-away self signed certificate with a random name and
+	// serial number
 	serial, testCert := test.ThrowAwayCert(t, clk)
-	_, err := sa.AddSerial(ctx, &sapb.AddSerialRequest{
-		RegID:   regID,
-		Serial:  serial,
-		Created: timestamppb.New(testCert.NotBefore),
-		Expires: timestamppb.New(testCert.NotAfter),
-	})
-	test.AssertNotError(t, err, "failed to add test serial")
+
+	// Add the cert as a precertificate
+	regID := reg.Id
 	issuedTime := mustTimestamp("2018-04-01 07:00")
-	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+	_, err := sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 		Der:          testCert.Raw,
 		RegID:        regID,
 		Issued:       issuedTime,
@@ -417,9 +413,11 @@ func TestAddPrecertificate(t *testing.T) {
 	test.AssertNotError(t, err, "Couldn't add test cert")
 
 	// It should have the expected certificate status
-	certStatus, err := sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
+	certStatus, err := sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "Couldn't get status for test cert")
-	test.AssertEquals(t, certStatus.Status, core.RevocationStatusGood)
+	test.AssertEquals(t, certStatus.Status, string(core.OCSPStatusGood))
+	now := clk.Now()
+	test.AssertEquals(t, now, certStatus.OcspLastUpdated.AsTime())
 
 	// It should show up in the issued names table
 	issuedNamesSerial, err := findIssuedName(ctx, sa.dbMap, reverseFQDN(testCert.DNSNames[0]))
@@ -1789,9 +1787,10 @@ func TestRevokeCertificate(t *testing.T) {
 	sa, fc, cleanUp := initSA(t)
 	defer cleanUp()
 
-	// Add a cert to the DB to test with.
 	reg := createWorkingRegistration(t, sa)
+	// Add a cert to the DB to test with.
 	serial, testCert := test.ThrowAwayCert(t, fc)
+	issuedTime := sa.clk.Now()
 	_, err := sa.AddSerial(ctx, &sapb.AddSerialRequest{
 		RegID:   reg.Id,
 		Serial:  core.SerialToString(testCert.SerialNumber),
@@ -1802,14 +1801,14 @@ func TestRevokeCertificate(t *testing.T) {
 	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 		Der:          testCert.Raw,
 		RegID:        reg.Id,
-		Issued:       timestamppb.New(testCert.NotBefore),
+		Issued:       timestamppb.New(issuedTime),
 		IssuerNameID: 1,
 	})
 	test.AssertNotError(t, err, "Couldn't add test cert")
 
-	status, err := sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
+	status, err := sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, status.Status, core.RevocationStatusGood)
+	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusGood)
 
 	fc.Add(1 * time.Hour)
 
@@ -1825,11 +1824,12 @@ func TestRevokeCertificate(t *testing.T) {
 	})
 	test.AssertNotError(t, err, "RevokeCertificate with no OCSP response should succeed")
 
-	status, err = sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
+	status, err = sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, status.Status, core.RevocationStatusRevoked)
+	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusRevoked)
 	test.AssertEquals(t, status.RevokedReason, reason)
 	test.AssertEquals(t, status.RevokedDate.AsTime(), now)
+	test.AssertEquals(t, status.OcspLastUpdated.AsTime(), now)
 
 	_, err = sa.RevokeCertificate(context.Background(), &sapb.RevokeCertificateRequest{
 		IssuerID: 1,
@@ -1840,13 +1840,67 @@ func TestRevokeCertificate(t *testing.T) {
 	test.AssertError(t, err, "RevokeCertificate should've failed when certificate already revoked")
 }
 
+func TestRevokeCertificateWithShard(t *testing.T) {
+	sa, fc, cleanUp := initSA(t)
+	defer cleanUp()
+
+	// Add a cert to the DB to test with.
+	reg := createWorkingRegistration(t, sa)
+	eeCert, err := core.LoadCert("../test/hierarchy/ee-e1.cert.pem")
+	test.AssertNotError(t, err, "failed to load test cert")
+	_, err = sa.AddSerial(ctx, &sapb.AddSerialRequest{
+		RegID:   reg.Id,
+		Serial:  core.SerialToString(eeCert.SerialNumber),
+		Created: timestamppb.New(eeCert.NotBefore),
+		Expires: timestamppb.New(eeCert.NotAfter),
+	})
+	test.AssertNotError(t, err, "failed to add test serial")
+	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
+		Der:          eeCert.Raw,
+		RegID:        reg.Id,
+		Issued:       timestamppb.New(eeCert.NotBefore),
+		IssuerNameID: 1,
+	})
+	test.AssertNotError(t, err, "failed to add test cert")
+
+	serial := core.SerialToString(eeCert.SerialNumber)
+	fc.Add(1 * time.Hour)
+	now := fc.Now()
+	reason := int64(1)
+
+	_, err = sa.RevokeCertificate(context.Background(), &sapb.RevokeCertificateRequest{
+		IssuerID: 1,
+		ShardIdx: 9,
+		Serial:   serial,
+		Date:     timestamppb.New(now),
+		Reason:   reason,
+	})
+	test.AssertNotError(t, err, "RevokeCertificate with no OCSP response should succeed")
+
+	status, err := sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
+	test.AssertNotError(t, err, "GetCertificateStatus failed")
+	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusRevoked)
+	test.AssertEquals(t, status.RevokedReason, reason)
+	test.AssertEquals(t, status.RevokedDate.AsTime(), now)
+	test.AssertEquals(t, status.OcspLastUpdated.AsTime(), now)
+	test.AssertEquals(t, status.NotAfter.AsTime(), eeCert.NotAfter)
+
+	var result revokedCertModel
+	err = sa.dbMap.SelectOne(
+		ctx, &result, `SELECT * FROM revokedCertificates WHERE serial = ?`, core.SerialToString(eeCert.SerialNumber))
+	test.AssertNotError(t, err, "should be exactly one row in revokedCertificates")
+	test.AssertEquals(t, result.ShardIdx, int64(9))
+	test.AssertEquals(t, result.RevokedReason, revocation.KeyCompromise)
+}
+
 func TestUpdateRevokedCertificate(t *testing.T) {
 	sa, fc, cleanUp := initSA(t)
 	defer cleanUp()
 
 	// Add a cert to the DB to test with.
 	reg := createWorkingRegistration(t, sa)
 	serial, testCert := test.ThrowAwayCert(t, fc)
+	issuedTime := fc.Now()
 	_, err := sa.AddSerial(ctx, &sapb.AddSerialRequest{
 		RegID:   reg.Id,
 		Serial:  core.SerialToString(testCert.SerialNumber),
@@ -1857,7 +1911,7 @@ func TestUpdateRevokedCertificate(t *testing.T) {
 	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 		Der:          testCert.Raw,
 		RegID:        reg.Id,
-		Issued:       timestamppb.New(testCert.NotBefore),
+		Issued:       timestamppb.New(issuedTime),
 		IssuerNameID: 1,
 	})
 	test.AssertNotError(t, err, "Couldn't add test cert")
@@ -1890,10 +1944,10 @@ func TestUpdateRevokedCertificate(t *testing.T) {
 	test.AssertNotError(t, err, "RevokeCertificate failed")
 
 	// Double check that setup worked.
-	status, err := sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
+	status, err := sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, status.Status, core.RevocationStatusRevoked)
-	test.AssertEquals(t, status.RevokedReason, int64(revocation.CessationOfOperation))
+	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusRevoked)
+	test.AssertEquals(t, revocation.Reason(status.RevokedReason), revocation.CessationOfOperation)
 	fc.Add(1 * time.Hour)
 
 	// Try to update its revocation info with no backdate
@@ -2946,10 +3000,10 @@ func TestGetRevokedCerts(t *testing.T) {
 	test.AssertNotError(t, err, "failed to add test cert")
 
 	// Check that it worked.
-	status, err := sa.GetRevocationStatus(
+	status, err := sa.GetCertificateStatus(
 		ctx, &sapb.Serial{Serial: core.SerialToString(eeCert.SerialNumber)})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, status.Status, core.RevocationStatusGood)
+	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusGood)
 
 	// Here's a little helper func we'll use to call GetRevokedCerts and count
 	// how many results it returned.
@@ -3053,10 +3107,10 @@ func TestGetRevokedCertsByShard(t *testing.T) {
 	test.AssertNotError(t, err, "failed to add test cert")
 
 	// Check that it worked.
-	status, err := sa.GetRevocationStatus(
+	status, err := sa.GetCertificateStatus(
 		ctx, &sapb.Serial{Serial: core.SerialToString(eeCert.SerialNumber)})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, status.Status, core.RevocationStatusGood)
+	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusGood)
 
 	// Here's a little helper func we'll use to call GetRevokedCertsByShard and count
 	// how many results it returned.