Improve error handling in admin block-key and revoke-cert (#8647)

Within the admin tool itself, multiple parallel goroutines blocking SPKI
hashes were all writing to the same `err` variable, resulting in a
potential data race. Fix this by giving each goroutine its own local
error.

Within ra.AdministrativelyRevokeCertificate, a failure in the initial
sa.RevokeCertificate call would properly fall back to attempting
sa.UpdateRevokedCertificate, but would not then continue on to calling
sa.AddBlockedKey. Restructure this method to have the same control flow
as ra.RevokeCertByKey, which properly ensures that sa.AddBlockedKey is
called no matter whether the revocation succeeds or fails.

Author: aarongable

cmd/admin/key.go

@@ -218,7 +218,7 @@ func (a *admin) blockSPKIHashes(ctx context.Context, spkiHashes [][]byte, commen
 	for range parallelism {
 		wg.Go(func() {
 			for spkiHash := range work {
-				err = a.blockSPKIHash(ctx, spkiHash, u, comment)
+				err := a.blockSPKIHash(ctx, spkiHash, u, comment)
 				if err != nil {
 					errCount.Add(1)
 					if errors.Is(err, berrors.AlreadyRevoked) {

ra/ra.go

@@ -2030,23 +2030,21 @@ func (ra *RegistrationAuthorityImpl) AdministrativelyRevokeCertificate(ctx conte
 		shard = req.CrlShard
 	}
 
-	_, err = ra.SA.RevokeCertificate(ctx, &sapb.RevokeCertificateRequest{
+	// We revoke the cert before adding it to the blocked keys list, to avoid a
+	// race between this and the bad-key-revoker. But we don't check the error
+	// from this operation until after we add the key to the blocked keys list,
+	// since that addition needs to happen no matter what.
+	_, revokeErr := ra.SA.RevokeCertificate(ctx, &sapb.RevokeCertificateRequest{
 		Serial:   req.Serial,
 		Reason:   int64(reasonCode),
 		Date:     timestamppb.New(ra.clk.Now()),
 		IssuerID: int64(issuerID),
 		ShardIdx: shard,
 	})
-	if err != nil {
-		if reasonCode == revocation.KeyCompromise && errors.Is(err, berrors.AlreadyRevoked) {
-			err = ra.updateRevocationForKeyCompromise(ctx, req.Serial, issuerID)
-			if err != nil {
-				return nil, err
-			}
-		}
-		return nil, err
-	}
 
+	// Failing to add the key to the blocked keys list is a worse failure than
+	// failing to revoke in the first place, because it means that
+	// bad-key-revoker won't revoke the cert anyway.
 	if reasonCode == revocation.KeyCompromise && !req.SkipBlockKey {
 		if cert == nil {
 			return nil, errors.New("revoking for key compromise requires providing the certificate's DER")
@@ -2057,6 +2055,18 @@ func (ra *RegistrationAuthorityImpl) AdministrativelyRevokeCertificate(ctx conte
 		}
 	}
 
+	// Check the error returned from sa.RevokeCertificate itself.
+	err = revokeErr
+	if errors.Is(err, berrors.AlreadyRevoked) && reasonCode == revocation.KeyCompromise {
+		err = ra.updateRevocationForKeyCompromise(ctx, req.Serial, issuerID)
+		if err != nil {
+			return nil, err
+		}
+		return &emptypb.Empty{}, nil
+	} else if err != nil {
+		return nil, err
+	}
+
 	return &emptypb.Empty{}, nil
 }
 

ra/ra_test.go

@@ -3956,6 +3956,41 @@ func TestAdministrativelyRevokeCertificate(t *testing.T) {
 	test.AssertError(t, err, "AdministrativelyRevokeCertificate should have failed with just serial for keyCompromise")
 }
 
+func TestAdministrativelyRevokeCertificateReRevokeKeyCompromiseBlocksKey(t *testing.T) {
+	_, _, ra, _, clk, _, cleanUp := initAuthorities(t)
+	defer cleanUp()
+
+	serial, cert := test.ThrowAwayCert(t, clk)
+	cert.IsCA = true
+	ic, err := issuance.NewCertificate(cert)
+	test.AssertNotError(t, err, "failed to create issuer cert")
+	ra.issuersByNameID = map[issuance.NameID]*issuance.Certificate{
+		ic.NameID(): ic,
+	}
+	mockSA := newMockSARevocation(cert)
+	ra.SA = mockSA
+
+	// First, revoke for a non-keyCompromise reason. This should succeed but not block the key.
+	_, err = ra.AdministrativelyRevokeCertificate(context.Background(), &rapb.AdministrativelyRevokeCertificateRequest{
+		Serial:    serial,
+		Code:      int64(revocation.Unspecified),
+		AdminName: "root",
+	})
+	test.AssertNotError(t, err, "initial administrative revocation failed")
+	test.AssertEquals(t, len(mockSA.blocked), 0)
+	test.AssertEquals(t, mockSA.revoked[serial].RevokedReason, int64(revocation.Unspecified))
+
+	// Now, re-revoke for keyCompromise. This should both update the reason AND block the key.
+	_, err = ra.AdministrativelyRevokeCertificate(context.Background(), &rapb.AdministrativelyRevokeCertificateRequest{
+		Serial:    serial,
+		Code:      int64(revocation.KeyCompromise),
+		AdminName: "root",
+	})
+	test.AssertNotError(t, err, "keyCompromise re-revocation should have succeeded")
+	test.AssertEquals(t, len(mockSA.blocked), 1)
+	test.AssertEquals(t, mockSA.revoked[serial].RevokedReason, int64(revocation.KeyCompromise))
+}
+
 // An authority that returns an error from NewOrderAndAuthzs if the
 // "ReplacesSerial" field of the request is empty.
 type mockNewOrderMustBeReplacementAuthority struct {