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,19 +678,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
 		}
 	}
+
+	// Let's unconditionally reset any prior pending or failed enrollment. This
+	// doesn't affect the existing certificate if one was already issued. At
+	// first, we wanted to decide whether to reset or not by first retrieving
+	// the certificate, but found that resetting unconditionally is simpler.
+	// When the response to /reset is 400, we just ignore it; here are the three
+	// expected responses:
+	//
+	// | Happy responses                         | Description                |
+	// |-----------------------------------------|----------------------------|
+	// | 200 {"ProcessingResetCompleted": true}  | Success                    |
+	// | 400 {"Error":"CertificateDN error..."}  | Certificate DN not found   |
+	// | 400 {"Error":"No reset is required..."} | No enrollment to be reset  |
+	certDN := getCertificateDN(c.zone, req.Subject.CommonName)
+	statusCode, status, body, err := c.request("POST", urlResourceCertificateReset, struct {
+		CertificateDN string `json:"CertificateDN"`
+		Restart       bool   `json:"Restart"`
+	}{
+		CertificateDN: certDN,
+		Restart:       false,
+	})
+	if err != nil {
+		return "", fmt.Errorf("while resetting certificate in case of previous enrollment: %w", err)
+	}
+	if statusCode != http.StatusOK && statusCode != http.StatusBadRequest {
+		return "", fmt.Errorf("while resetting certificate in case of previous enrollment: %s, body: '%s'", status, body)
+	}
+
 	tppCertificateRequest, err := prepareRequest(req, c.zone)
 	if err != nil {
 		return "", err
 	}
-	statusCode, status, body, err := c.request("POST", urlResourceCertificateRequest, tppCertificateRequest)
+	statusCode, status, body, err = c.request("POST", urlResourceCertificateRequest, tppCertificateRequest)
 	if err != nil {
 		return "", err
 	}