[Preston12321]

Two concurrent revocation requests could both succeed, with the latter overwriting the former's RevokedAt and Reason. This isn't much of an issue today, but will cause non-compliant behavior when CRLs are implemented per #531.

The issue is that revocation status is never checked in the same guard as it is written. Here's the relevant section of wfe.WebFrontEndImpl.processRevocation():

	revokedCert := wfe.db.GetRevokedCertificateByDER(derBytes)
	if revokedCert != nil {
		return acme.AlreadyRevokedProblem("Certificate has already been revoked.")
	}

	cert := wfe.db.GetCertificateByDER(derBytes)
	if cert == nil {
		return acme.MalformedProblem("Unable to find specified certificate.")
	}

	if prob := authorizedToRevoke(cert); prob != nil {
		return prob
	}

	wfe.db.RevokeCertificate(&core.RevokedCertificate{
		Certificate: cert,
		RevokedAt:   time.Now(),
		Reason:      revokeCertReq.Reason,
	})

The GetRevokedCertificateByDER() method uses a mutex read lock and checks whether the given certificate is already considered revoked. Then RevokeCertificate() uses a mutex write lock and immediately writes an entry to the internal revokedCertificatesByID map.

Because those two operations happen in different critical sections, it's possible for two goroutines to sync up right before the RevokeCertificate() call and result in two atomic writes to the same map entry, effectively erasing the first write.

This change fixes the race by having RevokeCertificate() check the map first and return whether it made any change. Now, the first write will succeed while all subsequent writes result in an AlreadyRevokedProblem.

Note: I used Claude to discover this issue and work up the changeset. I've reviewed it myself and made stylistic edits to code structure and comments before opening the PR. This description is my own writing 🙂

[pgporada]

Suggested change

	// 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.
	// RevokeCertificate returns false if a certificate was previously revoked, otherwise true.

[pgporada]

Suggested change

	// newTestWFE builds a WebFrontEndImpl backed by a real MemoryStore, CA and VA.
	// newTestWFE instantiates a functional WebFrontEndImpl

[aarongable]

It would be more idiomatic for RevokeCertificate to return an AlreadyRevoked error if the cert is already revoked. Then the WFE can just pass that error through. There are other functions in memorystore.go that already return acme.SomeSpecificProblem, so doing the same here seems good to me.

[aarongable]

If both returned errors are identical, there's no need for this early check anymore. Just try to revoke it, and if it's already revoked, return the appropriate error.