Use new RA methods from WFE revocation path (#5983)

Simplify the WFE `RevokeCertificate` API method in three ways:
- Remove most of the logic checking if the requester is authorized to
  revoke the certificate in question (based on who is making the
  request, what authorizations they have, and what reason they're
  requesting). That checking is now done by the RA. Instead, simply
  verify that the JWS is authenticated.
- Remove the hard-to-read `authorizedToRevoke` callbacks, and make the
  `revokeCertBySubscriberKey` (nee `revokeCertByKeyID`) and
  `revokeCertByCertKey` (nee `revokeCertByJWK`) helpers much more
  straight-line in their execution logic.
- Call the RA's new `RevokeCertByApplicant` and `RevokeCertByKey` gRPC
  methods, rather than the deprecated `RevokeCertificateWithReg`.

This change, without any flag flips, should be invisible to the
end-user. It will slightly change some of our log message formats.
However, by now relying on the new RA gRPC revocation methods, this
change allows us to change our revocation policies by enabling the
`AllowDoubleRevocation` and `MozRevocationReasons` feature flags, which
affect the behavior of those new helpers.

Fixes #5936

Author: aarongable

akamai/cache-client.go

@@ -279,7 +279,7 @@ func (cpc *CachePurgeClient) authedRequest(endpoint string, body v3PurgeRequest)
 	}
 
 	cpc.log.AuditInfof("Purge request sent successfully (ID %s) (body %s). Purge expected in %ds",
-		purgeInfo.PurgeID, purgeInfo.EstimatedSeconds, reqBody)
+		purgeInfo.PurgeID, reqBody, purgeInfo.EstimatedSeconds)
 	return nil
 }
 

errors/errors.go

@@ -146,5 +146,5 @@ func AlreadyRevokedError(msg string, args ...interface{}) error {
 }
 
 func BadRevocationReasonError(reason int64) error {
-	return New(AlreadyRevoked, "disallowed revocation reason: %d", reason)
+	return New(BadRevocationReason, "disallowed revocation reason: %d", reason)
 }

features/featureflag_string.go

@@ -59,6 +59,11 @@ const (
 	GetAuthzUseIndex
 	// Check the failed authorization limit before doing authz reuse.
 	CheckFailedAuthorizationsFirst
+	// AllowReRevocation causes the RA to allow the revocation reason of an
+	// already-revoked certificate to be updated to `keyCompromise` from any
+	// other reason if that compromise is demonstrated by making the second
+	// revocation request signed by the certificate keypair.
+	AllowReRevocation
 	// MozRevocationReasons causes the RA to enforce the following upcoming
 	// Mozilla policies regarding revocation:
 	// - A subscriber can request that their certificate be revoked with reason
@@ -97,6 +102,7 @@ var features = map[FeatureFlag]bool{
 	GetAuthzReadOnly:               false,
 	GetAuthzUseIndex:               false,
 	CheckFailedAuthorizationsFirst: false,
+	AllowReRevocation:              false,
 	MozRevocationReasons:           false,
 }
 

features/features.go

@@ -2111,7 +2111,17 @@ func (ra *RegistrationAuthorityImpl) RevokeCertByKey(ctx context.Context, req *r
 	// 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 reason == ocsp.KeyCompromise {
+	var shouldBlock bool
+	if features.Enabled(features.AllowReRevocation) {
+		// If we're allowing re-revocation, then block the key for all keyCompromise
+		// requests, no matter whether the revocation itself succeeded or failed.
+		shouldBlock = reason == ocsp.KeyCompromise
+	} else {
+		// Otherwise, only block the key if the revocation above succeeded, or
+		// failed for a reason other than "already revoked".
+		shouldBlock = (reason == ocsp.KeyCompromise && !errors.Is(revokeErr, berrors.AlreadyRevoked))
+	}
+	if shouldBlock {
 		var digest core.Sha256Digest
 		digest, err = core.KeyDigest(cert.PublicKey)
 		if err != nil {
@@ -2132,7 +2142,12 @@ func (ra *RegistrationAuthorityImpl) RevokeCertByKey(ctx context.Context, req *r
 	// than keyCompromise.
 	err = revokeErr
 	if err != nil {
-		if !errors.Is(err, berrors.AlreadyRevoked) || reason != ocsp.KeyCompromise {
+		// Immediately error out, rather than trying re-revocation, if the error was
+		// anything other than AlreadyRevoked, if the requested reason is anything
+		// other than keyCompromise, or if we're not yet using the new logic.
+		if !errors.Is(err, berrors.AlreadyRevoked) ||
+			reason != ocsp.KeyCompromise ||
+			!features.Enabled(features.AllowReRevocation) {
 			return nil, err
 		}
 		err = ra.updateRevocationForKeyCompromise(ctx, cert.SerialNumber, int64(issuerID))

ra/ra.go

@@ -3872,6 +3872,19 @@ func TestRevokeCertByKey(t *testing.T) {
 	test.AssertEquals(t, len(mockSA.blocked), 0)
 	test.AssertEquals(t, mockSA.revoked[core.SerialToString(cert.SerialNumber)], int64(ocsp.Unspecified))
 
+	// Re-revoking for any reason should fail, because it isn't enabled.
+	_, err = ra.RevokeCertByKey(context.Background(), &rapb.RevokeCertByKeyRequest{
+		Cert: cert.Raw,
+		Code: ocsp.KeyCompromise,
+	})
+	test.AssertError(t, err, "should have failed")
+
+	// Enable re-revocation.
+	_ = features.Set(map[string]bool{
+		features.MozRevocationReasons.String(): false,
+		features.AllowReRevocation.String():    true,
+	})
+
 	// Re-revoking for the same reason should fail.
 	_, err = ra.RevokeCertByKey(context.Background(), &rapb.RevokeCertByKeyRequest{
 		Cert: cert.Raw,
@@ -3950,13 +3963,26 @@ func TestRevokeCertByKey_Moz(t *testing.T) {
 	test.AssertEquals(t, len(mockSA.blocked[0].Comment), 0)
 	test.AssertEquals(t, mockSA.revoked[core.SerialToString(cert.SerialNumber)], int64(ocsp.KeyCompromise))
 
+	// Re-revoking should fail, because re-revocation is not allowed.
+	_, err = ra.RevokeCertByKey(context.Background(), &rapb.RevokeCertByKeyRequest{
+		Cert: cert.Raw,
+	})
+	test.AssertError(t, err, "should have failed")
+
+	// Enable re-revocation.
+	_ = features.Set(map[string]bool{
+		features.MozRevocationReasons.String(): true,
+		features.AllowReRevocation.String():    true,
+	})
+
 	// Re-revoking should fail, because it is already revoked for keyCompromise.
 	_, err = ra.RevokeCertByKey(context.Background(), &rapb.RevokeCertByKeyRequest{
 		Cert: cert.Raw,
 	})
 	test.AssertError(t, err, "should have failed")
 
-	// Reset, revoke for some other reason, and try again.
+	// Reset and have the Subscriber revoke for a different reason.
+	// Then re-revoking using the key should work.
 	mockSA.revoked = make(map[string]int64)
 	_, err = ra.RevokeCertByApplicant(context.Background(), &rapb.RevokeCertByApplicantRequest{
 		Cert:  cert.Raw,
@@ -3968,10 +3994,6 @@ func TestRevokeCertByKey_Moz(t *testing.T) {
 		Cert: cert.Raw,
 	})
 	test.AssertNotError(t, err, "should have succeeded")
-	_, err = ra.RevokeCertByKey(context.Background(), &rapb.RevokeCertByKeyRequest{
-		Cert: cert.Raw,
-	})
-	test.AssertError(t, err, "should have failed")
 }
 
 func TestAdministrativelyRevokeCertificate(t *testing.T) {

ra/ra_test.go

@@ -58,6 +58,7 @@
       "StoreRevokerInfo": true,
       "RestrictRSAKeySizes": true,
       "StreamlineOrderAndAuthzs": true,
+      "AllowReRevocation": true,
       "MozRevocationReasons": true
     },
     "CTLogGroups2": [

test/config-next/ra.json

@@ -96,7 +96,7 @@ def ocsp_verify(cert_file, issuer_file, ocsp_response):
         raise(Exception("OCSP verify failure"))
     return output
 
-def verify_ocsp(cert_file, issuer_file, url, status="revoked"):
+def verify_ocsp(cert_file, issuer_file, url, status="revoked", reason=None):
     ocsp_request = make_ocsp_req(cert_file, issuer_file)
     responses = fetch_ocsp(ocsp_request, url)
 
@@ -114,6 +114,10 @@ def verify_ocsp(cert_file, issuer_file, url, status="revoked"):
         if not re.search("%s: %s" % (cert_file, status), verify_output):
             print(verify_output)
             raise(Exception("OCSP response wasn't '%s'" % status))
+    if reason is not None:
+        if not re.search("Reason: %s" % reason, verify_output):
+            print(verify_output)
+            raise(Exception("OCSP response wasn't '%s'" % reason))
     return verify_output
 
 def reset_akamai_purges():