Use GetRevocationStatus instead of GetCertificateStatus (#8402)

In the SA, change the implementation of GetRevocationStatus to read from
the revokedCertificates table instead of reading from the
certificateStatus table.

In the WFE, switch to calling GetRevocationStatus when computing ARI
windows. Similarly, in the RA, make the same switch when checking if a
to-be-revoked certificate is already revoked.

Across all three locations, use new core constants to represent "good"
and "revoked", to avoid references to OCSP and unwieldy string/int
conversions.

This paves the way for removing sa.GetCertificateStatus, which now has
only one remaining caller which is not quite so easily changed.

Part of #8322

(cherry picked from commit a5cf372)

Author: aarongable

core/objects.go

@@ -83,6 +83,11 @@ 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, nil
+	return nil, errors.New("no revocation status")
 }
 
 // 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.GetCertificateStatus(ctx, &sapb.Serial{Serial: serialString})
+	status, err := ra.SA.GetRevocationStatus(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 != string(core.OCSPStatusRevoked) {
+	if status.Status != core.RevocationStatusRevoked {
 		// 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]*corepb.CertificateStatus
+	revoked map[string]*sapb.RevocationStatus
 	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]*corepb.CertificateStatus),
+		revoked: make(map[string]*sapb.RevocationStatus),
 		blocked: make([]*sapb.AddBlockedKeyRequest, 0),
 	}
 }
 
 func (msar *mockSARevocation) reset() {
-	msar.revoked = make(map[string]*corepb.CertificateStatus)
+	msar.revoked = make(map[string]*sapb.RevocationStatus)
 	msar.blocked = make([]*sapb.AddBlockedKeyRequest, 0)
 }
 
@@ -3552,14 +3552,13 @@ func (msar *mockSARevocation) GetLintPrecertificate(_ context.Context, req *sapb
 	return nil, berrors.UnknownSerialError()
 }
 
-func (msar *mockSARevocation) GetCertificateStatus(_ context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*corepb.CertificateStatus, error) {
+func (msar *mockSARevocation) GetRevocationStatus(_ context.Context, req *sapb.Serial, _ ...grpc.CallOption) (*sapb.RevocationStatus, error) {
 	if status, present := msar.revoked[req.Serial]; present {
 		return status, nil
 	}
-	if cert, present := msar.known[req.Serial]; present {
-		return &corepb.CertificateStatus{
-			Serial:   core.SerialToString(cert.SerialNumber),
-			IssuerID: int64(issuance.IssuerNameID(cert)),
+	if _, present := msar.known[req.Serial]; present {
+		return &sapb.RevocationStatus{
+			Status: core.RevocationStatusGood,
 		}, nil
 	}
 	return nil, berrors.UnknownSerialError()
@@ -3598,14 +3597,12 @@ func (msar *mockSARevocation) RevokeCertificate(_ context.Context, req *sapb.Rev
 	if _, present := msar.revoked[req.Serial]; present {
 		return nil, berrors.AlreadyRevokedError("already revoked")
 	}
-	cert, present := msar.known[req.Serial]
+	_, present := msar.known[req.Serial]
 	if !present {
 		return nil, berrors.UnknownSerialError()
 	}
-	msar.revoked[req.Serial] = &corepb.CertificateStatus{
-		Serial:        req.Serial,
-		IssuerID:      int64(issuance.IssuerNameID(cert)),
-		Status:        string(core.OCSPStatusRevoked),
+	msar.revoked[req.Serial] = &sapb.RevocationStatus{
+		Status:        core.RevocationStatusRevoked,
 		RevokedReason: req.Reason,
 	}
 	return &emptypb.Empty{}, nil
@@ -3772,7 +3769,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]*corepb.CertificateStatus)
+	mockSA.revoked = make(map[string]*sapb.RevocationStatus)
 	_, err = ra.RevokeCertByApplicant(context.Background(), &rapb.RevokeCertByApplicantRequest{
 		Cert:  cert.Raw,
 		Code:  int64(revocation.Unspecified),

sa/model.go

@@ -161,41 +161,6 @@ 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,15 +396,19 @@ func TestAddPrecertificate(t *testing.T) {
 	defer cleanUp()
 
 	reg := createWorkingRegistration(t, sa)
+	regID := reg.Id
 
-	// Create a throw-away self signed certificate with a random name and
-	// serial number
+	// Add a cert to the DB to test with.
 	serial, testCert := test.ThrowAwayCert(t, clk)
-
-	// Add the cert as a precertificate
-	regID := reg.Id
+	_, 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")
 	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,
@@ -413,11 +417,9 @@ func TestAddPrecertificate(t *testing.T) {
 	test.AssertNotError(t, err, "Couldn't add test cert")
 
 	// It should have the expected certificate status
-	certStatus, err := sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
+	certStatus, err := sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "Couldn't get status for test cert")
-	test.AssertEquals(t, certStatus.Status, string(core.OCSPStatusGood))
-	now := clk.Now()
-	test.AssertEquals(t, now, certStatus.OcspLastUpdated.AsTime())
+	test.AssertEquals(t, certStatus.Status, core.RevocationStatusGood)
 
 	// It should show up in the issued names table
 	issuedNamesSerial, err := findIssuedName(ctx, sa.dbMap, reverseFQDN(testCert.DNSNames[0]))
@@ -1787,10 +1789,9 @@ func TestRevokeCertificate(t *testing.T) {
 	sa, fc, cleanUp := initSA(t)
 	defer cleanUp()
 
-	reg := createWorkingRegistration(t, sa)
 	// Add a cert to the DB to test with.
+	reg := createWorkingRegistration(t, sa)
 	serial, testCert := test.ThrowAwayCert(t, fc)
-	issuedTime := sa.clk.Now()
 	_, err := sa.AddSerial(ctx, &sapb.AddSerialRequest{
 		RegID:   reg.Id,
 		Serial:  core.SerialToString(testCert.SerialNumber),
@@ -1801,14 +1802,14 @@ func TestRevokeCertificate(t *testing.T) {
 	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 		Der:          testCert.Raw,
 		RegID:        reg.Id,
-		Issued:       timestamppb.New(issuedTime),
+		Issued:       timestamppb.New(testCert.NotBefore),
 		IssuerNameID: 1,
 	})
 	test.AssertNotError(t, err, "Couldn't add test cert")
 
-	status, err := sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
+	status, err := sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusGood)
+	test.AssertEquals(t, status.Status, core.RevocationStatusGood)
 
 	fc.Add(1 * time.Hour)
 
@@ -1824,12 +1825,11 @@ func TestRevokeCertificate(t *testing.T) {
 	})
 	test.AssertNotError(t, err, "RevokeCertificate with no OCSP response should succeed")
 
-	status, err = sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
+	status, err = sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusRevoked)
+	test.AssertEquals(t, status.Status, core.RevocationStatusRevoked)
 	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,67 +1840,13 @@ 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),
@@ -1911,7 +1857,7 @@ func TestUpdateRevokedCertificate(t *testing.T) {
 	_, err = sa.AddPrecertificate(ctx, &sapb.AddCertificateRequest{
 		Der:          testCert.Raw,
 		RegID:        reg.Id,
-		Issued:       timestamppb.New(issuedTime),
+		Issued:       timestamppb.New(testCert.NotBefore),
 		IssuerNameID: 1,
 	})
 	test.AssertNotError(t, err, "Couldn't add test cert")
@@ -1944,10 +1890,10 @@ func TestUpdateRevokedCertificate(t *testing.T) {
 	test.AssertNotError(t, err, "RevokeCertificate failed")
 
 	// Double check that setup worked.
-	status, err := sa.GetCertificateStatus(ctx, &sapb.Serial{Serial: serial})
+	status, err := sa.GetRevocationStatus(ctx, &sapb.Serial{Serial: serial})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusRevoked)
-	test.AssertEquals(t, revocation.Reason(status.RevokedReason), revocation.CessationOfOperation)
+	test.AssertEquals(t, status.Status, core.RevocationStatusRevoked)
+	test.AssertEquals(t, status.RevokedReason, int64(revocation.CessationOfOperation))
 	fc.Add(1 * time.Hour)
 
 	// Try to update its revocation info with no backdate
@@ -3000,10 +2946,10 @@ func TestGetRevokedCerts(t *testing.T) {
 	test.AssertNotError(t, err, "failed to add test cert")
 
 	// Check that it worked.
-	status, err := sa.GetCertificateStatus(
+	status, err := sa.GetRevocationStatus(
 		ctx, &sapb.Serial{Serial: core.SerialToString(eeCert.SerialNumber)})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusGood)
+	test.AssertEquals(t, status.Status, core.RevocationStatusGood)
 
 	// Here's a little helper func we'll use to call GetRevokedCerts and count
 	// how many results it returned.
@@ -3107,10 +3053,10 @@ func TestGetRevokedCertsByShard(t *testing.T) {
 	test.AssertNotError(t, err, "failed to add test cert")
 
 	// Check that it worked.
-	status, err := sa.GetCertificateStatus(
+	status, err := sa.GetRevocationStatus(
 		ctx, &sapb.Serial{Serial: core.SerialToString(eeCert.SerialNumber)})
 	test.AssertNotError(t, err, "GetCertificateStatus failed")
-	test.AssertEquals(t, core.OCSPStatus(status.Status), core.OCSPStatusGood)
+	test.AssertEquals(t, status.Status, core.RevocationStatusGood)
 
 	// Here's a little helper func we'll use to call GetRevokedCertsByShard and count
 	// how many results it returned.