diff --git a/test/integration/revocation_test.go b/test/integration/revocation_test.go index b136e1ed7c7..4804d4944cd 100644 --- a/test/integration/revocation_test.go +++ b/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") } diff --git a/test/v2_integration.py b/test/v2_integration.py index 341a17e0e1d..de1a5c09699 100644 --- a/test/v2_integration.py +++ b/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)) diff --git a/wfe2/wfe.go b/wfe2/wfe.go index 277b68080ad..f285f37611f 100644 --- a/wfe2/wfe.go +++ b/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 diff --git a/wfe2/wfe_test.go b/wfe2/wfe_test.go index 5b0cd31c61c..5c11b25003b 100644 --- a/wfe2/wfe_test.go +++ b/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.