Reset before request in order to work around VCert's "unmatched key modulus"

go.mod

@@ -5,6 +5,7 @@ require (
 	github.com/pavel-v-chernykh/keystore-go/v4 v4.1.0
 	github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d
 	github.com/spf13/viper v1.7.0
+	github.com/stretchr/testify v1.3.0
 	github.com/urfave/cli/v2 v2.1.1
 	github.com/youmark/pkcs8 v0.0.0-20201027041543-1326539a0a0a
 	golang.org/x/crypto v0.0.0-20210921155107-089bfa567519

pkg/venafi/tpp/connector.go

@@ -20,6 +20,7 @@ import (
 	"crypto/x509"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"log"
 	"net/http"
@@ -678,18 +679,51 @@ func (c *Connector) proccessLocation(req *certificate.Request) error {
 	return nil
 }
 
-// RequestCertificate submits the CSR to TPP returning the DN of the requested Certificate
+// RequestCertificate requests the certificate to TPP and returns the DN of the
+// requested Certificate (this is called the "pickup ID" in the vcert CLI). If
+// the certificate is in the middle of an enrollment, the enrollment will be
+// reset before requesting (this doesn't affect the existing certificate if one
+// was already issued).
 func (c *Connector) RequestCertificate(req *certificate.Request) (requestID string, err error) {
 	if req.Location != nil {
 		err = c.proccessLocation(req)
 		if err != nil {
 			return
 		}
 	}
+
 	tppCertificateRequest, err := prepareRequest(req, c.zone)
 	if err != nil {
 		return "", err
 	}
+
+	certDN := getCertificateDN(c.zone, req.Subject.CommonName)
+
+	// We unconditionally reset any prior failed enrollment so that we don't get
+	// stuck with "Fix any errors, and then click Retry." (60% of the time) or
+	// "WebSDK CertRequest" (40% of the time).
+	//
+	// It would be preferable to only reset when necessary to avoid the extra
+	// call. We tried that in https://github.com/Venafi/vcert/pull/269. It turns
+	// out that calling "request" followed by "reset(restart=true)" causes a
+	// race in TPP.
+	//
+	// Unconditionally resetting isn't optimal, but "reset(restart=false)" is
+	// lightweight. We haven't verified that it doesn't slow things down on
+	// large TPP instances.
+	//
+	// Note that resetting won't affect the existing certificate if one was
+	// already issued.
+	err = c.reset(certDN, false)
+	notFound := &errCertNotFound{}
+	switch {
+	case errors.As(err, &notFound):
+		// Continue below. We expect the certificate not be found since we're
+		// going to be requesting it afterwards.
+	case err != nil:
+		return "", fmt.Errorf("while resetting certificate in case of previous enrollment: %w", err)
+	}
+
 	statusCode, status, body, err := c.request("POST", urlResourceCertificateRequest, tppCertificateRequest)
 	if err != nil {
 		return "", err
@@ -758,6 +792,54 @@ func (c *Connector) RequestCertificate(req *certificate.Request) (requestID stri
 	return
 }
 
+type errCertNotFound struct {
+	error
+}
+
+func (e *errCertNotFound) Error() string {
+	return e.error.Error()
+}
+
+func (e *errCertNotFound) Unwrap() error {
+	return e.error
+}
+
+// This function is idempotent, i.e., it won't fail if there is nothing to be
+// reset. It returns an error of type *errCertNotFound if the certificate is not
+// found.
+func (c *Connector) reset(certificateDN string, restart bool) error {
+	statusCode, status, body, err := c.request("POST", urlResourceCertificateReset, certificateResetRequest{
+		CertificateDN: certificateDN,
+		Restart:       restart,
+	})
+	if err != nil {
+		return fmt.Errorf("while resetting certificate due to a past failed enrollment: %w", err)
+	}
+
+	switch {
+	case statusCode == http.StatusOK:
+		return nil
+	case statusCode == http.StatusBadRequest:
+		var decodedResetResponse certificateRequestResponse
+		if err := json.Unmarshal(body, &decodedResetResponse); err != nil {
+			return fmt.Errorf("failed to decode reset response: HTTP %d: %s: %s", statusCode, status, body)
+		}
+
+		// No need to error out if the certificate was already reset.
+		if decodedResetResponse.Error == "Reset is not completed. No reset is required for the certificate." {
+			return nil
+		}
+
+		if strings.HasSuffix(decodedResetResponse.Error, "does not exist or you do not have sufficient rights to the object.") {
+			return &errCertNotFound{fmt.Errorf(decodedResetResponse.Error)}
+		}
+
+		return fmt.Errorf("while resetting certificate due to a past failed enrollment: %s", decodedResetResponse.Error)
+	default:
+		return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: %s", status, string(body))
+	}
+}
+
 func (c *Connector) GetPolicy(name string) (*policy.PolicySpecification, error) {
 	var ps *policy.PolicySpecification
 	var tp policy.TppPolicy
@@ -1232,32 +1314,9 @@ func (c *Connector) RetrieveCertificate(req *certificate.Request) (certificates
 	}
 
 	startTime := time.Now()
-	retrieveCount := 0
 	for {
-		retrieveCount++
-
 		var retrieveResponse *certificateRetrieveResponse
 		retrieveResponse, err = c.retrieveCertificateOnce(certReq)
-
-		// As a workaround to certificates being stuck due to a past failed
-		// enrollment, we reset the certificate as part of RetrieveCertificate
-		// to avoid making an extra HTTP call when requesting a certificate. We
-		// only reset once, since it only makes sense to reset the past failed
-		// enrollment.
-		if shouldReset(err) && retrieveCount == 1 {
-			statusCode, status, _, err := c.request("POST", urlResourceCertificateReset, certificateResetRequest{
-				CertificateDN: req.PickupID,
-				Restart:       true,
-			})
-			if err != nil {
-				return nil, fmt.Errorf("while resetting certificate due to a past failed enrollment: %w", err)
-			}
-			if statusCode != http.StatusOK {
-				return nil, fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s", status)
-			}
-
-			continue
-		}
 		if err != nil {
 			return nil, fmt.Errorf("unable to retrieve: %s", err)
 		}
@@ -1279,27 +1338,6 @@ func (c *Connector) RetrieveCertificate(req *certificate.Request) (certificates
 	}
 }
 
-// After many tests, we found that the only way to know if the current
-// certificate request is stuck due to an old failed enrollment is to look for
-// "WebSDK CertRequest" or "Fix any errors, and then click Retry." in the
-// retrieve response when it returns a 500.
-func shouldReset(err error) bool {
-	if err == nil {
-		return false
-	}
-
-	if !strings.Contains(err.Error(), "Status: 500") {
-		return false
-	}
-
-	if strings.Contains(err.Error(), "This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry.") ||
-		strings.Contains(err.Error(), "WebSDK CertRequest Module Requested Certificate") {
-		return true
-	}
-
-	return false
-}
-
 func (c *Connector) retrieveCertificateOnce(certReq certificateRetrieveRequest) (*certificateRetrieveResponse, error) {
 	statusCode, status, body, err := c.request("POST", urlResourceCertificateRetrieve, certReq)
 	if err != nil {