RequestCertificate: reset unconditionally instead of during retrieve

In Venafi#269, I got convinced that it
would be a good solution to call "Reset" only if the "Retrieve" call
returned a known message.

Later on, we realized that there was a bad interaction between "Request"
and "Reset(restart=true)". For some reason, when a problem arises (such
as CA being down), TPP returns the old certificate, and vcert ends up
showing the message "unmatched key modulus".

We realized that calling "Reset(restart=false)" before Request prevents
this bug. Although that's one extra HTTP call, it seems this call is
very inexpensive. One downside that was brought up during the PR Venafi#269
was that any extra HTTP call would slow the TPP server because the HTTP
called are "queued" (not concurrently processed).

Author: maelvls

pkg/venafi/tpp/connector.go

@@ -678,18 +678,46 @@ 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)
+	if 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 +786,43 @@ func (c *Connector) RequestCertificate(req *certificate.Request) (requestID stri
 	return
 }
 
+// For the purpose of using this function in RequestCertificate, we ignore the
+// errors "certificate not found" and "reset was not needed".
+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)
+		}
+
+		// We don't need to error out if the certificate is already reset.
+		if decodedResetResponse.Error == "Reset is not completed. No reset is required for the certificate." {
+			return nil
+		}
+
+		// This is specific to RequestCertificate: we want to silently skip
+		// certificates that don't exist.
+		if strings.HasSuffix(decodedResetResponse.Error, "does not exist or you do not have sufficient rights to the object.") {
+			return nil
+		}
+
+		return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: ", status, string(body))
+	default:
+		return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: ", status, string(body))
+	}
+}
+
 func (c *Connector) GetPolicy(name string) (*policy.PolicySpecification, error) {
 	var ps *policy.PolicySpecification
 	var tp policy.TppPolicy

pkg/venafi/tpp/tpp.go

@@ -146,6 +146,15 @@ type certificateRenewResponse struct {
 	Error   string `json:",omitempty"`
 }
 
+type certificateResetRequest struct {
+	CertificateDN string `json:",omitempty"`
+	Restart       bool   `json:",omitempty"`
+}
+
+type certificateResetResponse struct {
+	Error string `json:""`
+}
+
 type sanItem struct {
 	Type int    `json:""`
 	Name string `json:""`
@@ -355,6 +364,7 @@ const (
 	urlResourceCertificateRevoke      urlResource = "vedsdk/certificates/revoke"
 	urlResourceCertificatesAssociate  urlResource = "vedsdk/certificates/associate"
 	urlResourceCertificatesDissociate urlResource = "vedsdk/certificates/dissociate"
+	urlResourceCertificateReset       urlResource = "vedsdk/certificates/reset"
 	urlResourceCertificate            urlResource = "vedsdk/certificates/"
 	urlResourceCertificateSearch                  = urlResourceCertificate
 	urlResourceCertificatesList                   = urlResourceCertificate