Fix race condition in db.MemoryStore.RevokeCertificate()

db/memorystore.go

@@ -395,7 +395,7 @@ func (m *MemoryStore) GetCertificateByDER(der []byte) *core.Certificate {
 	return nil
 }
 
-// GetCertificateByDER loops over all revoked certificates to find the one that matches the provided
+// GetRevokedCertificateByDER loops over all revoked certificates to find the one that matches the provided
 // DER bytes. This method is linear and it's not optimized to give you a quick response.
 func (m *MemoryStore) GetRevokedCertificateByDER(der []byte) *core.RevokedCertificate {
 	m.RLock()
@@ -409,10 +409,17 @@ func (m *MemoryStore) GetRevokedCertificateByDER(der []byte) *core.RevokedCertif
 	return nil
 }
 
-func (m *MemoryStore) RevokeCertificate(cert *core.RevokedCertificate) {
+// RevokeCertificate atomically marks a certificate as revoked. It returns false
+// without changing anything if the certificate was already revoked, and true
+// if this call revoked it.
+func (m *MemoryStore) RevokeCertificate(cert *core.RevokedCertificate) bool {
 	m.Lock()
 	defer m.Unlock()
+	if _, ok := m.revokedCertificatesByID[cert.Certificate.ID]; ok {
+		return false
+	}
 	m.revokedCertificatesByID[cert.Certificate.ID] = cert
+	return true
 }
 
 /*

db/memorystore_test.go

@@ -0,0 +1,35 @@
+package db
+
+import (
+	"testing"
+	"time"
+
+	"github.com/letsencrypt/pebble/v2/core"
+)
+
+func TestRevokeCertificateTwice(t *testing.T) {
+	m := NewMemoryStore()
+	cert := &core.Certificate{ID: "01"}
+
+	reason := uint(1)
+	first := &core.RevokedCertificate{
+		Certificate: cert,
+		RevokedAt:   time.Now(),
+		Reason:      &reason,
+	}
+	if !m.RevokeCertificate(first) {
+		t.Fatal("first RevokeCertificate returned false, want true")
+	}
+
+	second := &core.RevokedCertificate{
+		Certificate: cert,
+		RevokedAt:   first.RevokedAt.Add(time.Hour),
+	}
+	if m.RevokeCertificate(second) {
+		t.Fatal("second RevokeCertificate returned true, want false")
+	}
+
+	if got := m.revokedCertificatesByID[cert.ID]; got != first {
+		t.Errorf("stored entry = %+v, want the first entry %+v", got, first)
+	}
+}

wfe/wfe.go

@@ -104,6 +104,10 @@ const (
 	unusedRevocationReason       = 7
 	aACompromiseRevocationReason = 10
 
+	// alreadyRevokedDetail is the problem detail returned when a revocation
+	// request names a certificate that is already revoked.
+	alreadyRevokedDetail = "Certificate has already been revoked."
+
 	// authzReuseEnvVar defines an environment variable name used to provide a
 	// percentage value for how often Pebble should try to reuse valid authorizations
 	// for each identifier in an order. The percentage is independent of whether a
@@ -3036,7 +3040,7 @@ func (wfe *WebFrontEndImpl) processRevocation(
 
 	revokedCert := wfe.db.GetRevokedCertificateByDER(derBytes)
 	if revokedCert != nil {
-		return acme.AlreadyRevokedProblem("Certificate has already been revoked.")
+		return acme.AlreadyRevokedProblem(alreadyRevokedDetail)
 	}
 
 	cert := wfe.db.GetCertificateByDER(derBytes)
@@ -3048,11 +3052,14 @@ func (wfe *WebFrontEndImpl) processRevocation(
 		return prob
 	}
 
-	wfe.db.RevokeCertificate(&core.RevokedCertificate{
+	revoked := wfe.db.RevokeCertificate(&core.RevokedCertificate{
 		Certificate: cert,
 		RevokedAt:   time.Now(),
 		Reason:      revokeCertReq.Reason,
 	})
+	if !revoked {
+		return acme.AlreadyRevokedProblem(alreadyRevokedDetail)
+	}
 	return nil
 }
 

wfe/wfe_test.go

@@ -0,0 +1,115 @@
+package wfe
+
+import (
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
+	"crypto/x509"
+	"encoding/base64"
+	"encoding/json"
+	"io"
+	"log"
+	"sync"
+	"testing"
+
+	"github.com/letsencrypt/pebble/v2/acme"
+	"github.com/letsencrypt/pebble/v2/ca"
+	"github.com/letsencrypt/pebble/v2/core"
+	"github.com/letsencrypt/pebble/v2/db"
+	"github.com/letsencrypt/pebble/v2/va"
+)
+
+// newTestWFE builds a WebFrontEndImpl backed by a real MemoryStore, CA and VA.
+func newTestWFE(t *testing.T) *WebFrontEndImpl {
+	t.Helper()
+	logger := log.New(io.Discard, "", 0)
+	memoryStore := db.NewMemoryStore()
+
+	caImpl := ca.New(logger, memoryStore, "", "ecdsa", 0, 1, map[string]ca.Profile{"default": {}})
+	vaImpl := va.New(logger, 0, 0, false, "", memoryStore)
+
+	wfeImpl := New(logger, memoryStore, vaImpl, caImpl, nil, false, false, 0, 0)
+	return &wfeImpl
+}
+
+// issueTestCert issues a leaf certificate through the WFE's CA and returns it.
+func issueTestCert(t *testing.T, wfe *WebFrontEndImpl) *core.Certificate {
+	t.Helper()
+
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("generating key: %s", err)
+	}
+
+	order := &core.Order{
+		Order: acme.Order{Profile: "default"},
+		ParsedCSR: &x509.CertificateRequest{
+			DNSNames:  []string{"example.com"},
+			PublicKey: key.Public(),
+		},
+		BeganProcessing: true,
+	}
+
+	wfe.ca.CompleteOrder(order)
+	if order.CertificateObject == nil {
+		t.Fatal("CA did not issue a certificate")
+	}
+
+	return order.CertificateObject
+}
+
+// revocationBody returns a revocation request JSON body for cert.
+func revocationBody(t *testing.T, cert *core.Certificate) []byte {
+	t.Helper()
+
+	body, err := json.Marshal(map[string]string{
+		"certificate": base64.RawURLEncoding.EncodeToString(cert.DER),
+	})
+	if err != nil {
+		t.Fatalf("marshaling revocation body: %s", err)
+	}
+
+	return body
+}
+
+func TestProcessRevocationConcurrent(t *testing.T) {
+	wfe := newTestWFE(t)
+	cert := issueTestCert(t, wfe)
+	body := revocationBody(t, cert)
+
+	const workers = 10
+	var authWg sync.WaitGroup
+	authWg.Add(workers)
+
+	// This callback is called right before the actual attempt to mark the
+	// certificate revoked, which allows us to synchronize the workers and test
+	// what happens when revocations race.
+	var authorize authorizedToRevokeCert = func(*core.Certificate) *acme.ProblemDetails {
+		authWg.Done()
+		authWg.Wait()
+		return nil
+	}
+
+	results := make([]*acme.ProblemDetails, workers)
+	var wg sync.WaitGroup
+	for i := range workers {
+		wg.Go(func() {
+			results[i] = wfe.processRevocation(body, authorize)
+		})
+	}
+	wg.Wait()
+
+	alreadyRevokedType := acme.AlreadyRevokedProblem("").Type
+	var successes int
+	for _, prob := range results {
+		switch {
+		case prob == nil:
+			successes++
+		case prob.Type != alreadyRevokedType:
+			t.Errorf("unexpected problem: %+v", prob)
+		}
+	}
+	if successes != 1 {
+		t.Errorf("got %d successful revocations, want exactly 1", successes)
+	}
+}