From fceb463a6f30e3bf1309cdc06d97d4bfd7cc1365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 8 Nov 2022 14:16:36 +0100 Subject: [PATCH] RequestCertificate: reset unconditionally instead of during retrieve In https://github.com/Venafi/vcert/pull/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 #269 was that any extra HTTP call would slow the TPP server because the HTTP called are "queued" (not concurrently processed). --- pkg/venafi/tpp/connector.go | 67 ++++++++++++++++++++++++++++++++++++- pkg/venafi/tpp/tpp.go | 10 ++++++ 2 files changed, 76 insertions(+), 1 deletion(-) diff --git a/pkg/venafi/tpp/connector.go b/pkg/venafi/tpp/connector.go index 7a884d3a..0a9523ca 100644 --- a/pkg/venafi/tpp/connector.go +++ b/pkg/venafi/tpp/connector.go @@ -678,7 +678,11 @@ 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) @@ -686,10 +690,34 @@ func (c *Connector) RequestCertificate(req *certificate.Request) (requestID stri 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 diff --git a/pkg/venafi/tpp/tpp.go b/pkg/venafi/tpp/tpp.go index 26682517..635f75da 100644 --- a/pkg/venafi/tpp/tpp.go +++ b/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