Match revocation reason and request signing method (letsencrypt#5713)

Match revocation reason and request signing method

Add more detailed logging about request signing methods

test/integration/revocation_test.go

@@ -1,3 +1,4 @@
+//go:build integration
 // +build integration
 
 package integration
@@ -15,6 +16,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/eggsampler/acme/v3"
 	"github.com/letsencrypt/boulder/test"
 	ocsp_helper "github.com/letsencrypt/boulder/test/ocsp/helper"
 	"golang.org/x/crypto/ocsp"
@@ -153,9 +155,9 @@ func TestRevokeWithKeyCompromise(t *testing.T) {
 	cert := res.certs[0]
 
 	err = c.RevokeCertificate(
-		c.Account,
+		acme.Account{},
 		cert,
-		c.Account.PrivateKey,
+		certKey,
 		ocsp.KeyCompromise,
 	)
 	test.AssertNotError(t, err, "failed to revoke certificate")
@@ -202,9 +204,9 @@ func TestBadKeyRevoker(t *testing.T) {
 	}
 
 	err = cA.RevokeCertificate(
-		cA.Account,
+		acme.Account{},
 		badCert.certs[0],
-		cA.Account.PrivateKey,
+		certKey,
 		ocsp.KeyCompromise,
 	)
 	test.AssertNotError(t, err, "failed to revoke certificate")
@@ -243,5 +245,5 @@ func TestBadKeyRevoker(t *testing.T) {
 	defer func() { _ = zeroCountResp.Body.Close() }()
 	body, err = ioutil.ReadAll(zeroCountResp.Body)
 	test.AssertNotError(t, err, "failed to read body")
-	test.AssertEquals(t, string(body), "0\n")
+	test.AssertEquals(t, string(body), "1\n")
 }

test/v2_integration.py

@@ -1565,8 +1565,8 @@ def ocsp_resigning_setup():
 
     cert = OpenSSL.crypto.load_certificate(
         OpenSSL.crypto.FILETYPE_PEM, order.fullchain_pem)
-    # Revoke for reason 1: keyCompromise
-    client.revoke(josepy.ComparableX509(cert), 1)
+    # Revoke for reason 3: affiliationChanged
+    client.revoke(josepy.ComparableX509(cert), 3)
 
     ocsp_response, reason = get_ocsp_response_and_reason(
         cert_file.name, "/tmp/intermediate-cert-rsa-a.pem", "http://localhost:4002")
@@ -1596,5 +1596,5 @@ def test_ocsp_resigning():
     if reason != ocsp_resigning_setup_data['reason']:
         raise(Exception("re-signed ocsp response has different reason %s expected %s" % (
             reason, ocsp_resigning_setup_data['reason'])))
-    if reason != "keyCompromise":
+    if reason != "affiliationChanged":
         raise(Exception("re-signed ocsp response has wrong reason %s" % reason))

wfe2/wfe.go

@@ -36,6 +36,7 @@ import (
 	sapb "github.com/letsencrypt/boulder/sa/proto"
 	"github.com/letsencrypt/boulder/web"
 	"github.com/prometheus/client_golang/prometheus"
+	"golang.org/x/crypto/ocsp"
 	"google.golang.org/protobuf/types/known/emptypb"
 	jose "gopkg.in/square/go-jose.v2"
 )
@@ -771,7 +772,7 @@ func (wfe *WebFrontEndImpl) acctHoldsAuthorizations(ctx context.Context, acctID
 // revoke the certificate a problem is returned. It is expected to be a closure
 // containing additional state (an account ID or key) that will be used to make
 // the decision.
-type authorizedToRevokeCert func(*x509.Certificate) *probs.ProblemDetails
+type authorizedToRevokeCert func(*x509.Certificate, revocation.Reason) *probs.ProblemDetails
 
 // processRevocation accepts the payload for a revocation request along with
 // an account ID and a callback used to decide if the requester is authorized to
@@ -846,12 +847,6 @@ func (wfe *WebFrontEndImpl) processRevocation(
 		return probs.AlreadyRevoked("Certificate already revoked")
 	}
 
-	// Validate that the requester is authenticated to revoke the given certificate
-	prob := authorizedToRevoke(parsedCertificate)
-	if prob != nil {
-		return prob
-	}
-
 	// Verify the revocation reason supplied is allowed
 	reason := revocation.Reason(0)
 	if revokeRequest.Reason != nil {
@@ -869,6 +864,12 @@ func (wfe *WebFrontEndImpl) processRevocation(
 		reason = *revokeRequest.Reason
 	}
 
+	// Validate that the requester is authenticated to revoke the given certificate
+	prob := authorizedToRevoke(parsedCertificate, reason)
+	if prob != nil {
+		return prob
+	}
+
 	// Revoke the certificate. AcctID may be 0 if there is no associated account
 	// (e.g. it was a self-authenticated JWS using the certificate public key)
 	_, err = wfe.RA.RevokeCertificateWithReg(ctx, &rapb.RevokeCertificateWithRegRequest{
@@ -897,6 +898,13 @@ func (wfe *WebFrontEndImpl) processRevocation(
 	return nil
 }
 
+type revocationEvidence struct {
+	Serial string
+	Reason revocation.Reason
+	RegID  int64
+	Method string
+}
+
 // revokeCertByKeyID processes an outer JWS as a revocation request that is
 // authenticated by a KeyID and the associated account.
 func (wfe *WebFrontEndImpl) revokeCertByKeyID(
@@ -913,7 +921,12 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID(
 	// For Key ID revocations we decide if an account is able to revoke a specific
 	// certificate by checking that the account has valid authorizations for all
 	// of the names in the certificate or was the issuing account
-	authorizedToRevoke := func(parsedCertificate *x509.Certificate) *probs.ProblemDetails {
+	authorizedToRevoke := func(parsedCertificate *x509.Certificate, reason revocation.Reason) *probs.ProblemDetails {
+		// If revocation reason is keyCompromise, reject the request.
+		if reason == revocation.Reason(ocsp.KeyCompromise) {
+			return probs.Unauthorized("Revocation with reason keyCompromise is only supported by signing with the certificate private key")
+		}
+
 		// Try to find a stored final certificate for the serial number
 		serial := core.SerialToString(parsedCertificate.SerialNumber)
 		cert, err := wfe.SA.GetCertificate(ctx, &sapb.Serial{Serial: serial})
@@ -938,6 +951,12 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID(
 		// If the cert/precert is owned by the requester then return nil, it is an
 		// authorized revocation.
 		if cert.RegistrationID == acct.ID {
+			wfe.log.AuditObject("Authorizing revocation", revocationEvidence{
+				Serial: core.SerialToString(parsedCertificate.SerialNumber),
+				Reason: reason,
+				RegID:  acct.ID,
+				Method: "owner",
+			})
 			return nil
 		}
 		// Otherwise check if the account, while not the owner, has equivalent authorizations
@@ -950,6 +969,12 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID(
 			return probs.Unauthorized(
 				"The key ID specified in the revocation request does not hold valid authorizations for all names in the certificate to be revoked")
 		}
+		wfe.log.AuditObject("Authorizing revocation", revocationEvidence{
+			Serial: core.SerialToString(parsedCertificate.SerialNumber),
+			Reason: reason,
+			RegID:  acct.ID,
+			Method: "authorizations",
+		})
 		// If it does, return nil. It is an an authorized revocation.
 		return nil
 	}
@@ -980,11 +1005,17 @@ func (wfe *WebFrontEndImpl) revokeCertByJWK(
 	// For embedded JWK revocations we decide if a requester is able to revoke a specific
 	// certificate by checking that to-be-revoked certificate has the same public
 	// key as the JWK that was used to authenticate the request
-	authorizedToRevoke := func(parsedCertificate *x509.Certificate) *probs.ProblemDetails {
+	authorizedToRevoke := func(parsedCertificate *x509.Certificate, reason revocation.Reason) *probs.ProblemDetails {
 		if !core.KeyDigestEquals(requestKey, parsedCertificate.PublicKey) {
 			return probs.Unauthorized(
 				"JWK embedded in revocation request must be the same public key as the cert to be revoked")
 		}
+		wfe.log.AuditObject("Authorizing revocation", revocationEvidence{
+			Serial: core.SerialToString(parsedCertificate.SerialNumber),
+			Reason: reason,
+			RegID:  0,
+			Method: "privkey",
+		})
 		return nil
 	}
 	// We use `0` as the account ID provided to `processRevocation` because this

wfe2/wfe_test.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/jmhodges/clock"
 	"github.com/prometheus/client_golang/prometheus"
+	"golang.org/x/crypto/ocsp"
 	"google.golang.org/grpc"
 	"google.golang.org/protobuf/types/known/emptypb"
 	jose "gopkg.in/square/go-jose.v2"
@@ -2934,6 +2935,56 @@ func TestRevokeCertificateValid(t *testing.T) {
 	test.AssertEquals(t, responseWriter.Body.String(), "")
 }
 
+// A revocation request with reason == keyCompromise should only succeed
+// if it was signed by the private key.
+func TestRevokeCertificateKeyCompromiseValid(t *testing.T) {
+	wfe, _ := setupWFE(t)
+	wfe.SA = newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)
+
+	mockLog := wfe.log.(*blog.Mock)
+	mockLog.Clear()
+
+	keyPemBytes, err := ioutil.ReadFile("../test/hierarchy/ee-r3.key.pem")
+	test.AssertNotError(t, err, "Failed to load key")
+	key := loadKey(t, keyPemBytes)
+
+	revocationReason := revocation.Reason(ocsp.KeyCompromise)
+	revokeRequestJSON, err := makeRevokeRequestJSON(&revocationReason)
+	test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
+	_, _, jwsBody := signRequestEmbed(t,
+		key, "http://localhost/revoke-cert", string(revokeRequestJSON), wfe.nonceService)
+
+	responseWriter := httptest.NewRecorder()
+	wfe.RevokeCertificate(ctx, newRequestEvent(), responseWriter,
+		makePostRequestWithPath("revoke-cert", jwsBody))
+	test.AssertEquals(t, responseWriter.Code, 200)
+	test.AssertEquals(t, responseWriter.Body.String(), "")
+	test.AssertDeepEquals(t, mockLog.GetAllMatching("Authorizing revocation"), []string{
+		`INFO: [AUDIT] Authorizing revocation JSON={"Serial":"000000000000000000001d72443db5189821","Reason":1,"RegID":0,"Method":"privkey"}`,
+	})
+}
+
+func TestRevokeCertificateKeyCompromiseInvalid(t *testing.T) {
+	wfe, _ := setupWFE(t)
+	wfe.SA = newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)
+
+	revocationReason := revocation.Reason(ocsp.KeyCompromise)
+	revokeRequestJSON, err := makeRevokeRequestJSON(&revocationReason)
+	test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
+	// NOTE: this account doesn't have any authorizations for the
+	// names in the cert, but it is the account that issued it
+	// originally
+	_, _, jwsBody := signRequestKeyID(
+		t, 1, nil, "http://localhost/revoke-cert", string(revokeRequestJSON), wfe.nonceService)
+
+	responseWriter := httptest.NewRecorder()
+	wfe.RevokeCertificate(ctx, newRequestEvent(), responseWriter,
+		makePostRequestWithPath("revoke-cert", jwsBody))
+
+	test.AssertEquals(t, responseWriter.Code, 403)
+	test.AssertEquals(t, responseWriter.Body.String(), "{\n  \"type\": \"urn:ietf:params:acme:error:unauthorized\",\n  \"detail\": \"Revocation with reason keyCompromise is only supported by signing with the certificate private key\",\n  \"status\": 403\n}")
+}
+
 // Invalid revocation request: although signed with the cert key, the cert
 // wasn't issued by any issuer the Boulder is aware of.
 func TestRevokeCertificateNotIssued(t *testing.T) {
@@ -3048,6 +3099,9 @@ func TestRevokeCertificateIssuingAccount(t *testing.T) {
 	wfe, _ := setupWFE(t)
 	wfe.SA = newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)
 
+	mockLog := wfe.log.(*blog.Mock)
+	mockLog.Clear()
+
 	revokeRequestJSON, err := makeRevokeRequestJSON(nil)
 	test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
 	// NOTE: this account doesn't have any authorizations for the
@@ -3062,6 +3116,9 @@ func TestRevokeCertificateIssuingAccount(t *testing.T) {
 
 	test.AssertEquals(t, responseWriter.Code, 200)
 	test.AssertEquals(t, responseWriter.Body.String(), "")
+	test.AssertDeepEquals(t, mockLog.GetAllMatching("Authorizing revocation"), []string{
+		`INFO: [AUDIT] Authorizing revocation JSON={"Serial":"000000000000000000001d72443db5189821","Reason":0,"RegID":1,"Method":"owner"}`,
+	})
 }
 
 type mockSAWithValidAuthz struct {
@@ -3085,6 +3142,9 @@ func TestRevokeCertificateWithAuthorizations(t *testing.T) {
 	wfe, _ := setupWFE(t)
 	wfe.SA = mockSAWithValidAuthz{newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)}
 
+	mockLog := wfe.log.(*blog.Mock)
+	mockLog.Clear()
+
 	revokeRequestJSON, err := makeRevokeRequestJSON(nil)
 	test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
 
@@ -3099,6 +3159,9 @@ func TestRevokeCertificateWithAuthorizations(t *testing.T) {
 		makePostRequestWithPath("revoke-cert", jwsBody))
 	test.AssertEquals(t, responseWriter.Code, 200)
 	test.AssertEquals(t, responseWriter.Body.String(), "")
+	test.AssertDeepEquals(t, mockLog.GetAllMatching("Authorizing revocation"), []string{
+		`INFO: [AUDIT] Authorizing revocation JSON={"Serial":"000000000000000000001d72443db5189821","Reason":0,"RegID":5,"Method":"authorizations"}`,
+	})
 }
 
 // A revocation request signed by an unauthorized key.