Skip to content

Commit ba67367

Browse files
andygabbyandygabby
authored
Match revocation reason and request signing method (letsencrypt#5713)
Match revocation reason and request signing method Add more detailed logging about request signing methods
1 parent 18e5f40 commit ba67367

File tree

4 files changed

+113
-17
lines changed

4 files changed

+113
-17
lines changed

‎test/integration/revocation_test.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
//go:build integration
12
// +build integration
23

34
package integration
@@ -15,6 +16,7 @@ import (
1516
"testing"
1617
"time"
1718

19+
"github.com/eggsampler/acme/v3"
1820
"github.com/letsencrypt/boulder/test"
1921
ocsp_helper "github.com/letsencrypt/boulder/test/ocsp/helper"
2022
"golang.org/x/crypto/ocsp"
@@ -153,9 +155,9 @@ func TestRevokeWithKeyCompromise(t *testing.T) {
153155
cert := res.certs[0]
154156

155157
err = c.RevokeCertificate(
156-
c.Account,
158+
acme.Account{},
157159
cert,
158-
c.Account.PrivateKey,
160+
certKey,
159161
ocsp.KeyCompromise,
160162
)
161163
test.AssertNotError(t, err, "failed to revoke certificate")
@@ -202,9 +204,9 @@ func TestBadKeyRevoker(t *testing.T) {
202204
}
203205

204206
err = cA.RevokeCertificate(
205-
cA.Account,
207+
acme.Account{},
206208
badCert.certs[0],
207-
cA.Account.PrivateKey,
209+
certKey,
208210
ocsp.KeyCompromise,
209211
)
210212
test.AssertNotError(t, err, "failed to revoke certificate")
@@ -243,5 +245,5 @@ func TestBadKeyRevoker(t *testing.T) {
243245
defer func() { _ = zeroCountResp.Body.Close() }()
244246
body, err = ioutil.ReadAll(zeroCountResp.Body)
245247
test.AssertNotError(t, err, "failed to read body")
246-
test.AssertEquals(t, string(body), "0\n")
248+
test.AssertEquals(t, string(body), "1\n")
247249
}

‎test/v2_integration.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1565,8 +1565,8 @@ def ocsp_resigning_setup():
15651565

15661566
cert = OpenSSL.crypto.load_certificate(
15671567
OpenSSL.crypto.FILETYPE_PEM, order.fullchain_pem)
1568-
# Revoke for reason 1: keyCompromise
1569-
client.revoke(josepy.ComparableX509(cert), 1)
1568+
# Revoke for reason 3: affiliationChanged
1569+
client.revoke(josepy.ComparableX509(cert), 3)
15701570

15711571
ocsp_response, reason = get_ocsp_response_and_reason(
15721572
cert_file.name, "/tmp/intermediate-cert-rsa-a.pem", "http://localhost:4002")
@@ -1596,5 +1596,5 @@ def test_ocsp_resigning():
15961596
if reason != ocsp_resigning_setup_data['reason']:
15971597
raise(Exception("re-signed ocsp response has different reason %s expected %s" % (
15981598
reason, ocsp_resigning_setup_data['reason'])))
1599-
if reason != "keyCompromise":
1599+
if reason != "affiliationChanged":
16001600
raise(Exception("re-signed ocsp response has wrong reason %s" % reason))

‎wfe2/wfe.go‎

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
sapb "github.com/letsencrypt/boulder/sa/proto"
3737
"github.com/letsencrypt/boulder/web"
3838
"github.com/prometheus/client_golang/prometheus"
39+
"golang.org/x/crypto/ocsp"
3940
"google.golang.org/protobuf/types/known/emptypb"
4041
jose "gopkg.in/square/go-jose.v2"
4142
)
@@ -771,7 +772,7 @@ func (wfe *WebFrontEndImpl) acctHoldsAuthorizations(ctx context.Context, acctID
771772
// revoke the certificate a problem is returned. It is expected to be a closure
772773
// containing additional state (an account ID or key) that will be used to make
773774
// the decision.
774-
type authorizedToRevokeCert func(*x509.Certificate) *probs.ProblemDetails
775+
type authorizedToRevokeCert func(*x509.Certificate, revocation.Reason) *probs.ProblemDetails
775776

776777
// processRevocation accepts the payload for a revocation request along with
777778
// an account ID and a callback used to decide if the requester is authorized to
@@ -846,12 +847,6 @@ func (wfe *WebFrontEndImpl) processRevocation(
846847
return probs.AlreadyRevoked("Certificate already revoked")
847848
}
848849

849-
// Validate that the requester is authenticated to revoke the given certificate
850-
prob := authorizedToRevoke(parsedCertificate)
851-
if prob != nil {
852-
return prob
853-
}
854-
855850
// Verify the revocation reason supplied is allowed
856851
reason := revocation.Reason(0)
857852
if revokeRequest.Reason != nil {
@@ -869,6 +864,12 @@ func (wfe *WebFrontEndImpl) processRevocation(
869864
reason = *revokeRequest.Reason
870865
}
871866

867+
// Validate that the requester is authenticated to revoke the given certificate
868+
prob := authorizedToRevoke(parsedCertificate, reason)
869+
if prob != nil {
870+
return prob
871+
}
872+
872873
// Revoke the certificate. AcctID may be 0 if there is no associated account
873874
// (e.g. it was a self-authenticated JWS using the certificate public key)
874875
_, err = wfe.RA.RevokeCertificateWithReg(ctx, &rapb.RevokeCertificateWithRegRequest{
@@ -897,6 +898,13 @@ func (wfe *WebFrontEndImpl) processRevocation(
897898
return nil
898899
}
899900

901+
type revocationEvidence struct {
902+
Serial string
903+
Reason revocation.Reason
904+
RegID int64
905+
Method string
906+
}
907+
900908
// revokeCertByKeyID processes an outer JWS as a revocation request that is
901909
// authenticated by a KeyID and the associated account.
902910
func (wfe *WebFrontEndImpl) revokeCertByKeyID(
@@ -913,7 +921,12 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID(
913921
// For Key ID revocations we decide if an account is able to revoke a specific
914922
// certificate by checking that the account has valid authorizations for all
915923
// of the names in the certificate or was the issuing account
916-
authorizedToRevoke := func(parsedCertificate *x509.Certificate) *probs.ProblemDetails {
924+
authorizedToRevoke := func(parsedCertificate *x509.Certificate, reason revocation.Reason) *probs.ProblemDetails {
925+
// If revocation reason is keyCompromise, reject the request.
926+
if reason == revocation.Reason(ocsp.KeyCompromise) {
927+
return probs.Unauthorized("Revocation with reason keyCompromise is only supported by signing with the certificate private key")
928+
}
929+
917930
// Try to find a stored final certificate for the serial number
918931
serial := core.SerialToString(parsedCertificate.SerialNumber)
919932
cert, err := wfe.SA.GetCertificate(ctx, &sapb.Serial{Serial: serial})
@@ -938,6 +951,12 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID(
938951
// If the cert/precert is owned by the requester then return nil, it is an
939952
// authorized revocation.
940953
if cert.RegistrationID == acct.ID {
954+
wfe.log.AuditObject("Authorizing revocation", revocationEvidence{
955+
Serial: core.SerialToString(parsedCertificate.SerialNumber),
956+
Reason: reason,
957+
RegID: acct.ID,
958+
Method: "owner",
959+
})
941960
return nil
942961
}
943962
// Otherwise check if the account, while not the owner, has equivalent authorizations
@@ -950,6 +969,12 @@ func (wfe *WebFrontEndImpl) revokeCertByKeyID(
950969
return probs.Unauthorized(
951970
"The key ID specified in the revocation request does not hold valid authorizations for all names in the certificate to be revoked")
952971
}
972+
wfe.log.AuditObject("Authorizing revocation", revocationEvidence{
973+
Serial: core.SerialToString(parsedCertificate.SerialNumber),
974+
Reason: reason,
975+
RegID: acct.ID,
976+
Method: "authorizations",
977+
})
953978
// If it does, return nil. It is an an authorized revocation.
954979
return nil
955980
}
@@ -980,11 +1005,17 @@ func (wfe *WebFrontEndImpl) revokeCertByJWK(
9801005
// For embedded JWK revocations we decide if a requester is able to revoke a specific
9811006
// certificate by checking that to-be-revoked certificate has the same public
9821007
// key as the JWK that was used to authenticate the request
983-
authorizedToRevoke := func(parsedCertificate *x509.Certificate) *probs.ProblemDetails {
1008+
authorizedToRevoke := func(parsedCertificate *x509.Certificate, reason revocation.Reason) *probs.ProblemDetails {
9841009
if !core.KeyDigestEquals(requestKey, parsedCertificate.PublicKey) {
9851010
return probs.Unauthorized(
9861011
"JWK embedded in revocation request must be the same public key as the cert to be revoked")
9871012
}
1013+
wfe.log.AuditObject("Authorizing revocation", revocationEvidence{
1014+
Serial: core.SerialToString(parsedCertificate.SerialNumber),
1015+
Reason: reason,
1016+
RegID: 0,
1017+
Method: "privkey",
1018+
})
9881019
return nil
9891020
}
9901021
// We use `0` as the account ID provided to `processRevocation` because this

‎wfe2/wfe_test.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626

2727
"github.com/jmhodges/clock"
2828
"github.com/prometheus/client_golang/prometheus"
29+
"golang.org/x/crypto/ocsp"
2930
"google.golang.org/grpc"
3031
"google.golang.org/protobuf/types/known/emptypb"
3132
jose "gopkg.in/square/go-jose.v2"
@@ -2934,6 +2935,56 @@ func TestRevokeCertificateValid(t *testing.T) {
29342935
test.AssertEquals(t, responseWriter.Body.String(), "")
29352936
}
29362937

2938+
// A revocation request with reason == keyCompromise should only succeed
2939+
// if it was signed by the private key.
2940+
func TestRevokeCertificateKeyCompromiseValid(t *testing.T) {
2941+
wfe, _ := setupWFE(t)
2942+
wfe.SA = newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)
2943+
2944+
mockLog := wfe.log.(*blog.Mock)
2945+
mockLog.Clear()
2946+
2947+
keyPemBytes, err := ioutil.ReadFile("../test/hierarchy/ee-r3.key.pem")
2948+
test.AssertNotError(t, err, "Failed to load key")
2949+
key := loadKey(t, keyPemBytes)
2950+
2951+
revocationReason := revocation.Reason(ocsp.KeyCompromise)
2952+
revokeRequestJSON, err := makeRevokeRequestJSON(&revocationReason)
2953+
test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
2954+
_, _, jwsBody := signRequestEmbed(t,
2955+
key, "http://localhost/revoke-cert", string(revokeRequestJSON), wfe.nonceService)
2956+
2957+
responseWriter := httptest.NewRecorder()
2958+
wfe.RevokeCertificate(ctx, newRequestEvent(), responseWriter,
2959+
makePostRequestWithPath("revoke-cert", jwsBody))
2960+
test.AssertEquals(t, responseWriter.Code, 200)
2961+
test.AssertEquals(t, responseWriter.Body.String(), "")
2962+
test.AssertDeepEquals(t, mockLog.GetAllMatching("Authorizing revocation"), []string{
2963+
`INFO: [AUDIT] Authorizing revocation JSON={"Serial":"000000000000000000001d72443db5189821","Reason":1,"RegID":0,"Method":"privkey"}`,
2964+
})
2965+
}
2966+
2967+
func TestRevokeCertificateKeyCompromiseInvalid(t *testing.T) {
2968+
wfe, _ := setupWFE(t)
2969+
wfe.SA = newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)
2970+
2971+
revocationReason := revocation.Reason(ocsp.KeyCompromise)
2972+
revokeRequestJSON, err := makeRevokeRequestJSON(&revocationReason)
2973+
test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
2974+
// NOTE: this account doesn't have any authorizations for the
2975+
// names in the cert, but it is the account that issued it
2976+
// originally
2977+
_, _, jwsBody := signRequestKeyID(
2978+
t, 1, nil, "http://localhost/revoke-cert", string(revokeRequestJSON), wfe.nonceService)
2979+
2980+
responseWriter := httptest.NewRecorder()
2981+
wfe.RevokeCertificate(ctx, newRequestEvent(), responseWriter,
2982+
makePostRequestWithPath("revoke-cert", jwsBody))
2983+
2984+
test.AssertEquals(t, responseWriter.Code, 403)
2985+
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}")
2986+
}
2987+
29372988
// Invalid revocation request: although signed with the cert key, the cert
29382989
// wasn't issued by any issuer the Boulder is aware of.
29392990
func TestRevokeCertificateNotIssued(t *testing.T) {
@@ -3048,6 +3099,9 @@ func TestRevokeCertificateIssuingAccount(t *testing.T) {
30483099
wfe, _ := setupWFE(t)
30493100
wfe.SA = newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)
30503101

3102+
mockLog := wfe.log.(*blog.Mock)
3103+
mockLog.Clear()
3104+
30513105
revokeRequestJSON, err := makeRevokeRequestJSON(nil)
30523106
test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
30533107
// NOTE: this account doesn't have any authorizations for the
@@ -3062,6 +3116,9 @@ func TestRevokeCertificateIssuingAccount(t *testing.T) {
30623116

30633117
test.AssertEquals(t, responseWriter.Code, 200)
30643118
test.AssertEquals(t, responseWriter.Body.String(), "")
3119+
test.AssertDeepEquals(t, mockLog.GetAllMatching("Authorizing revocation"), []string{
3120+
`INFO: [AUDIT] Authorizing revocation JSON={"Serial":"000000000000000000001d72443db5189821","Reason":0,"RegID":1,"Method":"owner"}`,
3121+
})
30653122
}
30663123

30673124
type mockSAWithValidAuthz struct {
@@ -3085,6 +3142,9 @@ func TestRevokeCertificateWithAuthorizations(t *testing.T) {
30853142
wfe, _ := setupWFE(t)
30863143
wfe.SA = mockSAWithValidAuthz{newMockSAWithCert(t, wfe.SA, core.OCSPStatusGood)}
30873144

3145+
mockLog := wfe.log.(*blog.Mock)
3146+
mockLog.Clear()
3147+
30883148
revokeRequestJSON, err := makeRevokeRequestJSON(nil)
30893149
test.AssertNotError(t, err, "Failed to make revokeRequestJSON")
30903150

@@ -3099,6 +3159,9 @@ func TestRevokeCertificateWithAuthorizations(t *testing.T) {
30993159
makePostRequestWithPath("revoke-cert", jwsBody))
31003160
test.AssertEquals(t, responseWriter.Code, 200)
31013161
test.AssertEquals(t, responseWriter.Body.String(), "")
3162+
test.AssertDeepEquals(t, mockLog.GetAllMatching("Authorizing revocation"), []string{
3163+
`INFO: [AUDIT] Authorizing revocation JSON={"Serial":"000000000000000000001d72443db5189821","Reason":0,"RegID":5,"Method":"authorizations"}`,
3164+
})
31023165
}
31033166

31043167
// A revocation request signed by an unauthorized key.

0 commit comments

Comments
 (0)