Reject revocation requests for expired certs (#3896)

Fixes #3894.

wfe/wfe.go

@@ -749,6 +749,10 @@ func (wfe *WebFrontEndImpl) RevokeCertificate(ctx context.Context, logEvent *web
 		wfe.sendError(response, logEvent, probs.ServerInternal("invalid parse of stored certificate"), err)
 		return
 	}
+	if parsedCertificate.NotAfter.Before(wfe.clk.Now()) {
+		wfe.sendError(response, logEvent, probs.Unauthorized("Certificate is expired"), nil)
+		return
+	}
 	logEvent.Extra["RetrievedCertificateSerial"] = core.SerialToString(parsedCertificate.SerialNumber)
 	logEvent.Extra["RetrievedCertificateDNSNames"] = parsedCertificate.DNSNames
 	logEvent.Extra["RetrievedCertificateEmailAddresses"] = parsedCertificate.EmailAddresses

wfe/wfe_test.go

@@ -1630,6 +1630,45 @@ func TestRevokeCertificateAlreadyRevoked(t *testing.T) {
 		`{"type":"`+probs.V1ErrorNS+`malformed","detail":"Certificate already revoked","status":409}`)
 }
 
+func TestRevokeCertificateExpired(t *testing.T) {
+	wfe, fc := setupWFE(t)
+	keyPemBytes, err := ioutil.ReadFile("test/178.key")
+	test.AssertNotError(t, err, "Failed to load key")
+	key := loadPrivateKey(t, keyPemBytes)
+	rsaKey, ok := key.(*rsa.PrivateKey)
+	test.Assert(t, ok, "Couldn't load RSA key")
+	signer := newJoseSigner(t, rsaKey, wfe.nonceService)
+
+	certPemBytes, err := ioutil.ReadFile("test/178.crt")
+	test.AssertNotError(t, err, "Failed to load cert")
+	certBlock, _ := pem.Decode(certPemBytes)
+	test.Assert(t, certBlock != nil, "Failed to decode PEM")
+	revokeRequest := struct {
+		Resource       string          `json:"resource"`
+		CertificateDER core.JSONBuffer `json:"certificate"`
+	}{
+		Resource:       "revoke-cert",
+		CertificateDER: certBlock.Bytes,
+	}
+	revokeRequestJSON, err := json.Marshal(revokeRequest)
+	test.AssertNotError(t, err, "Failed to marshal request")
+
+	parsedCert, err := x509.ParseCertificate(certBlock.Bytes)
+	test.AssertNotError(t, err, "failed to parse test cert")
+	fc.Set(parsedCert.NotAfter.Add(time.Hour))
+
+	wfe.SA = &mockSANoSuchRegistration{mocks.NewStorageAuthority(fc)}
+	responseWriter := httptest.NewRecorder()
+	responseWriter.Body.Reset()
+	result, _ := signer.Sign(revokeRequestJSON)
+	wfe.RevokeCertificate(ctx, newRequestEvent(), responseWriter,
+		makePostRequest(result.FullSerialize()))
+	test.AssertEquals(t, responseWriter.Code, 403)
+	test.AssertUnmarshaledEquals(t, responseWriter.Body.String(),
+		`{"type":"`+probs.V1ErrorNS+`unauthorized","detail":"Certificate is expired","status":403}`)
+
+}
+
 func TestRevokeCertificateWithAuthz(t *testing.T) {
 	wfe, _ := setupWFE(t)
 	responseWriter := httptest.NewRecorder()

wfe2/wfe.go

@@ -15,11 +15,10 @@ import (
 	"strings"
 	"time"
 
-	jose "gopkg.in/square/go-jose.v2"
-
 	"github.com/jmhodges/clock"
 	"github.com/prometheus/client_golang/prometheus"
 	"golang.org/x/net/context"
+	jose "gopkg.in/square/go-jose.v2"
 
 	"github.com/letsencrypt/boulder/core"
 	corepb "github.com/letsencrypt/boulder/core/proto"
@@ -624,6 +623,10 @@ func (wfe *WebFrontEndImpl) processRevocation(
 	logEvent.Extra["RetrievedCertificateSerial"] = core.SerialToString(parsedCertificate.SerialNumber)
 	logEvent.Extra["RetrievedCertificateDNSNames"] = parsedCertificate.DNSNames
 
+	if parsedCertificate.NotAfter.Before(wfe.clk.Now()) {
+		return probs.Unauthorized("Certificate is expired")
+	}
+
 	// Check the certificate status for the provided certificate to see if it is
 	// already revoked
 	certStatus, err := wfe.SA.GetCertificateStatus(ctx, serial)

wfe2/wfe_test.go

@@ -19,6 +19,7 @@ import (
 	"strconv"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/jmhodges/clock"
 	"golang.org/x/net/context"
@@ -2577,6 +2578,38 @@ func TestRevokeCertificateWithAuthz(t *testing.T) {
 	test.AssertEquals(t, responseWriter.Body.String(), "")
 }
 
+func TestRevokeCertificateExpired(t *testing.T) {
+	wfe, fc := setupWFE(t)
+	responseWriter := httptest.NewRecorder()
+	test4JWK := loadKey(t, []byte(test4KeyPrivatePEM))
+
+	certPemBytes, err := ioutil.ReadFile("test/238.crt")
+	test.AssertNotError(t, err, "failed to read test/238.crt")
+	certBlock, _ := pem.Decode(certPemBytes)
+	test.AssertNotError(t, err, "failed to parse test/238.crt")
+	reason := revocation.Reason(0)
+	revokeRequest := struct {
+		CertificateDER core.JSONBuffer    `json:"certificate"`
+		Reason         *revocation.Reason `json:"reason"`
+	}{
+		CertificateDER: certBlock.Bytes,
+		Reason:         &reason,
+	}
+	revokeRequestJSON, err := json.Marshal(revokeRequest)
+	test.AssertNotError(t, err, "failed to marshal revocation request")
+
+	parsedCertificate, err := x509.ParseCertificate(certBlock.Bytes)
+	test.AssertNotError(t, err, "failed to parse test cert")
+	fc.Set(parsedCertificate.NotAfter.Add(time.Hour))
+
+	_, _, jwsBody := signRequestKeyID(t, 4, test4JWK, "http://localhost/revoke-cert", string(revokeRequestJSON), wfe.nonceService)
+	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\": \"Certificate is expired\",\n  \"status\": 403\n}")
+}
+
 type mockSAGetRegByKeyFails struct {
 	core.StorageGetter
 }