From 641994a39d0992acf3418aa0237b05beeeeb104a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Tue, 24 Jan 2023 15:00:58 +0100 Subject: [PATCH 1/6] Revert "Change RetrieveCertificate so that it resets the enrollment (if it exists) while retrieving the certificate" --- pkg/venafi/tpp/connector.go | 44 -------- pkg/venafi/tpp/connector_test.go | 174 +++---------------------------- pkg/venafi/tpp/tpp.go | 6 -- 3 files changed, 12 insertions(+), 212 deletions(-) diff --git a/pkg/venafi/tpp/connector.go b/pkg/venafi/tpp/connector.go index e4761c4b..7a884d3a 100644 --- a/pkg/venafi/tpp/connector.go +++ b/pkg/venafi/tpp/connector.go @@ -1232,32 +1232,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 +1256,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 { diff --git a/pkg/venafi/tpp/connector_test.go b/pkg/venafi/tpp/connector_test.go index 589eb1ff..0bd2a3bc 100644 --- a/pkg/venafi/tpp/connector_test.go +++ b/pkg/venafi/tpp/connector_test.go @@ -530,48 +530,6 @@ func TestRequestCertificateWithValidHours(t *testing.T) { DoRequestCertificateWithValidHours(t, tpp) } -func Test_shouldReset(t *testing.T) { - tests := []struct { - name string - givenErr error - want bool - }{ - { - name: "nil error", - givenErr: nil, - want: false, - }, - { - name: "error is not a 500", - givenErr: fmt.Errorf("unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 400 Certificate does not exist."), - want: false, - }, - { - name: "error is a 500 but not WebSDK or Click Retry", - givenErr: fmt.Errorf("unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500."), - want: false, - }, - { - name: "error is a 500 and is Click Retry", - givenErr: fmt.Errorf("unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry., Stage: 500."), - want: true, - }, - { - name: "error is a 500 and is WebSDK", - givenErr: fmt.Errorf("unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: WebSDK CertRequest Module Requested Certificate, Stage: 500."), - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := shouldReset(tt.givenErr) - if got != tt.want { - t.Errorf("shouldReset() = %v, want %v", got, tt.want) - } - }) - } -} - // The reason we are using a mock HTTP server rather than the live TPP server is // because consistently triggering the 500 error in a stage different than 0 // requires putting a powershell script on the TPP VM or turning the Microsoft @@ -591,7 +549,6 @@ func TestRetrieveCertificate(t *testing.T) { tests := []struct { name string mockRetrieve []mockResp - mockReset mockResp givenTimeout time.Duration expectErr string }{ @@ -629,18 +586,6 @@ func TestRetrieveCertificate(t *testing.T) { }, expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 403 Failed to issue grant: User is not authorized for the requested scope", }, - { - name: "should fail when 'private key'", - mockRetrieve: []mockResp{ - // This specific error message is relied upon by vcert itself as - // well as terraform-provider-venafi. So let's make sure we - // don't break it. See: - // https://github.com/Venafi/terraform-provider-venafi/blob/5374fa5/venafi/resource_venafi_certificate.go#L821 - {"400 Failed to lookup private key, error: Failed to lookup private key vault id", - `{"Error":"Failed to lookup private key, error: Failed to lookup private key vault id"}`}, - }, - expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 400 Failed to lookup private key, error: Failed to lookup private key vault id", - }, { name: "should succeed if cert immediately available regardless of the timeout value", mockRetrieve: []mockResp{ @@ -676,94 +621,19 @@ func TestRetrieveCertificate(t *testing.T) { expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500.", }, { - name: "should succeed after resetting the msg WebSDK CertRequest", + name: "should fail on msg WebSDK CertRequest", mockRetrieve: []mockResp{ {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: WebSDK CertRequest Module Requested Certificate, Stage: 500.`, `{"Stage": 500, "Status": "WebSDK CertRequest Module Requested Certificate"}`}, - {`202 Certificate \VED\Policy\TLS/SSL\aexample.com being processed, Status: Post CSR, Stage: 500.`, - `{"Stage": 500, "Status": "Post CSR"}`}, - {`200 OK`, - `{"CertificateData":"` + certData + `","Filename":"bexample.com.cer","Format":"base64"}`}, }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, - givenTimeout: 3 * time.Second, - }, - { - name: "should fail after resetting msg WebSDK CertRequest and enrollment fails", - mockRetrieve: []mockResp{ - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: WebSDK CertRequest Module Requested Certificate, Stage: 500.`, - `{"Stage": 500, "Status": "WebSDK CertRequest Module Requested Certificate"}`}, - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500.`, - `{"Stage": 500, "Status": "Post CSR failed with error: Cannot connect to the certificate authority (CA)."}`}, - }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, - expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500.", - }, - { - name: "should fail if msg WebSDK shows twice in a row", - mockRetrieve: []mockResp{ - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: WebSDK CertRequest Module Requested Certificate, Stage: 500.`, - `{"Stage": 500, "Status": "WebSDK CertRequest Module Requested Certificate"}`}, - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: WebSDK CertRequest Module Requested Certificate, Stage: 500.`, - `{"Stage": 500, "Status": "WebSDK CertRequest Module Requested Certificate"}`}, - }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: WebSDK CertRequest Module Requested Certificate, Stage: 500.", }, { - name: "should fail after resetting msg WebSDK CertRequest when enrollment in progress and timeout is 0", - mockRetrieve: []mockResp{ - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: WebSDK CertRequest Module Requested Certificate, Stage: 500.`, - `{"Stage": 500, "Status": "WebSDK CertRequest Module Requested Certificate"}`}, - {`202 Certificate \VED\Policy\TLS/SSL\aexample.com being processed, Status: Post CSR, Stage: 500.`, - `{"Stage": 500, "Status": "Post CSR"}`}, - }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, - expectErr: "Issuance is pending. You may try retrieving the certificate later using Pickup ID: \\VED\\Policy\\Test\\bexample.com\n\tStatus: Post CSR", - }, - { - name: "should succeed after resetting the msg Click Retry", - mockRetrieve: []mockResp{ - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry., Stage: 500.`, - `{"Stage": 500, "Status": "This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry."}`}, - {`200 OK`, - `{"CertificateData":"` + certData + `","Filename":"bexample.com.cer","Format":"base64"}`}, - }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, - }, - { - name: "should succeed after resetting the msg Click Retry and after waiting", - mockRetrieve: []mockResp{ - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry., Stage: 500.`, - `{"Stage": 500, "Status": "This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry."}`}, - {`202 Certificate \VED\Policy\TLS/SSL\aexample.com being processed, Status: Post CSR, Stage: 500.`, - `{"Stage": 500, "Status": "Post CSR"}`}, - {`200 OK`, - `{"CertificateData":"` + certData + `","Filename":"bexample.com.cer","Format":"base64"}`}, - }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, - givenTimeout: 3 * time.Second, - }, - { - name: "should fail when reset fails after msg Click Retry", + name: "should fail on msg Click Retry", mockRetrieve: []mockResp{ {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry., Stage: 500.`, `{"Stage": 500, "Status": "This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry."}`}, - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500.`, - `{"Stage": 500, "Status": "Post CSR failed with error: Cannot connect to the certificate authority (CA)."}`}, }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, - expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500.", - }, - { - name: "should fail if msg Click Retry shows twice in a row", - mockRetrieve: []mockResp{ - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry., Stage: 500.`, - `{"Stage": 500, "Status": "This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry."}`}, - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry., Stage: 500.`, - `{"Stage": 500, "Status": "This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry."}`}, - }, - mockReset: mockResp{`200 OK`, `{"ProcessingResetCompleted": true}`}, expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: This certificate cannot be processed while it is in an error state. Fix any errors, and then click Retry., Stage: 500.", }, { @@ -771,11 +641,11 @@ func TestRetrieveCertificate(t *testing.T) { mockRetrieve: []mockResp{ {`202 Certificate \VED\Policy\TLS/SSL\aexample.com being processed, Status: Post CSR, Stage: 500.`, `{"Stage": 500, "Status": "Post CSR"}`}, - {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500.`, + {`500 Certificate \VED\Policy\TLS/SSL\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA), Stage: 500.`, `{"Stage": 500, "Status": "Post CSR failed with error: Cannot connect to the certificate authority (CA)."}`}, }, givenTimeout: 3 * time.Second, - expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA)., Stage: 500.", + expectErr: "unable to retrieve: Unexpected status code on TPP Certificate Retrieval. Status: 500 Certificate \\VED\\Policy\\TLS/SSL\\aexample.com has encountered an error while processing, Status: Post CSR failed with error: Cannot connect to the certificate authority (CA), Stage: 500.", }, { name: "should fail when timeout too small while waiting for the cert", @@ -787,8 +657,9 @@ func TestRetrieveCertificate(t *testing.T) { expectErr: "Operation timed out. You may try retrieving the certificate later using Pickup ID: \\VED\\Policy\\Test\\bexample.com", }, } - serverWith := func(t *testing.T, mockRetrieve []mockResp, mockReset mockResp) (_ *httptest.Server, retrieveCount, resetCount *int32) { - retrieveCount, resetCount = new(int32), new(int32) + + serverWith := func(mockRetrieve []mockResp) (_ *httptest.Server, retrieveCount *int32) { + retrieveCount = new(int32) server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { case r.URL.Path == "/vedsdk/certificates/retrieve": @@ -800,40 +671,25 @@ func TestRetrieveCertificate(t *testing.T) { req := certificateRetrieveRequest{} _ = json.NewDecoder(r.Body).Decode(&req) if req.CertificateDN != `\VED\Policy\Test\bexample.com` { - t.Errorf("/retrieve: expected CertificateDN to be '%s' but got '%s'", `\VED\Policy\Test\bexample.com`, req.CertificateDN) + t.Fatalf("/retrieve: expected CertificateDN to be '%s' but got '%s'", `\VED\Policy\Test\bexample.com`, req.CertificateDN) } writeRespWithCustomStatus(w, mockRetrieve[index].status, mockRetrieve[index].body, ) - case r.URL.Path == "/vedsdk/certificates/reset": - atomic.AddInt32(resetCount, 1) - if mockReset == (mockResp{}) { - t.Errorf("/reset: no call was expected, but got 1 call") - } - req := certificateResetRequest{} - _ = json.NewDecoder(r.Body).Decode(&req) - if req.CertificateDN != `\VED\Policy\Test\bexample.com` { - t.Errorf("/vedsdk/certificates/reset: expected CertificateDN to be %s but got %s", `\VED\Policy\Test\bexample.com`, req.CertificateDN) - } - if req.Restart != true { - t.Errorf("/vedsdk/certificates/reset: expected Restart to be true but got false") - } - - writeRespWithCustomStatus(w, mockReset.status, mockReset.body) default: t.Fatalf("mock http server: unimplemented path " + r.URL.Path) } })) t.Cleanup(server.Close) - return server, retrieveCount, resetCount + return server, retrieveCount } for _, tt := range tests { tt := tt // Because t.Parallel. t.Run(tt.name, func(t *testing.T) { t.Parallel() - server, retrieveCount, resetCount := serverWith(t, tt.mockRetrieve, tt.mockReset) + server, retrieveCount := serverWith(tt.mockRetrieve) trusted := x509.NewCertPool() trusted.AddCert(server.Certificate()) @@ -844,17 +700,11 @@ func TestRetrieveCertificate(t *testing.T) { _, err = tpp.RetrieveCertificate(&certificate.Request{PickupID: `\VED\Policy\Test\bexample.com`, Timeout: tt.givenTimeout}) if atomic.LoadInt32(retrieveCount) != int32(len(tt.mockRetrieve)) { - t.Errorf("tpp.RetrieveCertificate: expected %d calls to /certificates/retrieve, but got %d", len(tt.mockRetrieve), atomic.LoadInt32(retrieveCount)) - } - if tt.mockReset == (mockResp{}) && atomic.LoadInt32(resetCount) != 0 { - t.Errorf("tpp.RetrieveCertificate: expected no call to /certificates/reset, but got %d", atomic.LoadInt32(resetCount)) - } - if tt.mockReset != (mockResp{}) && atomic.LoadInt32(resetCount) != 1 { - t.Errorf("tpp.RetrieveCertificate: expected 1 call to /certificates/reset, but got %d", atomic.LoadInt32(resetCount)) + t.Fatalf("tpp.RetrieveCertificate: expected %d calls to /certificates/retrieve, but got %d", len(tt.mockRetrieve), atomic.LoadInt32(retrieveCount)) } if tt.expectErr != "" { if err == nil || err.Error() != tt.expectErr { - t.Fatalf("tpp.RetrieveCertificate: \nexpected: %q\ngot: %q", tt.expectErr, err) + t.Fatalf("tpp.RetrieveCertificate: expected err to be %q but got %q", tt.expectErr, err) } return } diff --git a/pkg/venafi/tpp/tpp.go b/pkg/venafi/tpp/tpp.go index fbd3ab7a..26682517 100644 --- a/pkg/venafi/tpp/tpp.go +++ b/pkg/venafi/tpp/tpp.go @@ -107,11 +107,6 @@ type certificateRetrieveResponse struct { Stage int `json:",omitempty"` } -type certificateResetRequest struct { - CertificateDN string `json:",omitempty"` - Restart bool `json:",omitempty"` -} - type RevocationReason int // RevocationReasonsMap maps *certificate.RevocationRequest.Reason to TPP-specific webSDK codes @@ -357,7 +352,6 @@ const ( urlResourceCertificateRenew urlResource = "vedsdk/certificates/renew" urlResourceCertificateRequest urlResource = "vedsdk/certificates/request" urlResourceCertificateRetrieve urlResource = "vedsdk/certificates/retrieve" - urlResourceCertificateReset urlResource = "vedsdk/certificates/reset" urlResourceCertificateRevoke urlResource = "vedsdk/certificates/revoke" urlResourceCertificatesAssociate urlResource = "vedsdk/certificates/associate" urlResourceCertificatesDissociate urlResource = "vedsdk/certificates/dissociate" 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 2/6] 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 From 6a725c333430ea929c4b11651c574fde39f4419c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Thu, 26 Jan 2023 19:04:48 +0100 Subject: [PATCH 3/6] RequestCertificate: add tests --- go.mod | 1 + pkg/venafi/tpp/connector.go | 35 +++++++++--- pkg/venafi/tpp/connector_test.go | 98 ++++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 746d56b6..5e1d2f7a 100644 --- a/go.mod +++ b/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 diff --git a/pkg/venafi/tpp/connector.go b/pkg/venafi/tpp/connector.go index 0a9523ca..fe321362 100644 --- a/pkg/venafi/tpp/connector.go +++ b/pkg/venafi/tpp/connector.go @@ -20,6 +20,7 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" + "errors" "fmt" "log" "net/http" @@ -714,7 +715,12 @@ func (c *Connector) RequestCertificate(req *certificate.Request) (requestID stri // Note that resetting won't affect the existing certificate if one was // already issued. err = c.reset(certDN, false) - if err != nil { + notFound := &errCertNotFound{} + switch { + case errors.As(err, ¬Found): + // 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) } @@ -786,8 +792,21 @@ 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". +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, @@ -806,20 +825,18 @@ func (c *Connector) reset(certificateDN string, restart bool) error { 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. + // 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 } - // 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 &errCertNotFound{fmt.Errorf(decodedResetResponse.Error)} } - return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: ", status, string(body)) + return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: %s", status, string(body)) default: - return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: ", status, string(body)) + return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: %s", status, string(body)) } } diff --git a/pkg/venafi/tpp/connector_test.go b/pkg/venafi/tpp/connector_test.go index 0bd2a3bc..1e22154e 100644 --- a/pkg/venafi/tpp/connector_test.go +++ b/pkg/venafi/tpp/connector_test.go @@ -42,6 +42,8 @@ import ( "github.com/Venafi/vcert/v4/pkg/policy" "github.com/Venafi/vcert/v4/pkg/util" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/Venafi/vcert/v4/pkg/certificate" "github.com/Venafi/vcert/v4/pkg/endpoint" @@ -530,6 +532,102 @@ func TestRequestCertificateWithValidHours(t *testing.T) { DoRequestCertificateWithValidHours(t, tpp) } +// The mocked HTTP interactions have been recorded using TPP 22.4. +func TestRequestCertificate(t *testing.T) { + type mockCall struct { + expectReqPath string + expectReqBody string + mockStatus string // The HTTP status line, e.g. "400 Bad Request". + mockBody string + } + + tests := []struct { + name string + mockCalls []mockCall + givenTimeout time.Duration + expectErr string + }{ + { + name: "happy path", + mockCalls: []mockCall{ + {"/vedsdk/certificates/reset", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com"}`, + "200 OK", `{"ProcessingResetCompleted":true}`}, + {"/vedsdk/certificates/request", `{"PolicyDN":"\\VED\\Policy\\application-team-1", "CASpecificAttributes":[{"Name":"Origin", "Value":"Venafi VCert-Go"}], "CertificateType":"AUTO", "DisableAutomaticRenewal":true, "KeyAlgorithm":"RSA", "Origin":"Venafi VCert-Go", "Reenable":true}`, + "200 OK", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com","Guid":"{b86278bf-091f-4205-a0a3-8f3e8de140c3}"}`}, + }, + }, { + name: "should request even if reset cannot find the cert", + mockCalls: []mockCall{ + {"/vedsdk/certificates/reset", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com"}`, + "400 Bad request. Check the error in the response for details.", `{"Error":"CertificateDN error. CertificateDN: \"\\VED\\Policy\\application-team-1\\app1.example.com\" does not exist or you do not have sufficient rights to the object."}`}, + {"/vedsdk/certificates/request", `{"PolicyDN":"\\VED\\Policy\\application-team-1", "CASpecificAttributes":[{"Name":"Origin", "Value":"Venafi VCert-Go"}], "CertificateType":"AUTO", "DisableAutomaticRenewal":true, "KeyAlgorithm":"RSA", "Origin":"Venafi VCert-Go", "Reenable":true}`, + "200 OK", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com","Guid":"{b86278bf-091f-4205-a0a3-8f3e8de140c3}"}`}, + }, + }, { + name: "should request even if reset was not needed", + mockCalls: []mockCall{ + {"/vedsdk/certificates/reset", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com"}`, + "400 Bad request. Check the error in the response for details.", `{"Error":"Reset is not completed. No reset is required for the certificate."}`}, + {"/vedsdk/certificates/request", `{"PolicyDN":"\\VED\\Policy\\application-team-1", "CASpecificAttributes":[{"Name":"Origin", "Value":"Venafi VCert-Go"}], "CertificateType":"AUTO", "DisableAutomaticRenewal":true, "KeyAlgorithm":"RSA", "Origin":"Venafi VCert-Go", "Reenable":true}`, + "200 OK", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com","Guid":"{b86278bf-091f-4205-a0a3-8f3e8de140c3}"}`}, + }, + }, { + name: "should request even if reset fails with an unexpected 400", + mockCalls: []mockCall{ + {"/vedsdk/certificates/reset", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com"}`, + "400 Bad request. Check the error in the response for details.", `{"Error":"Invalid CertificateDN format. The Certificate DN contained null or white spaces for (CertificateDN)."}`}, + }, + expectErr: "while resetting certificate in case of previous enrollment: while resetting certificate due to a past failed enrollment. Status: 400 Bad request. Check the error in the response for details., Body: \r{\"Error\":\"Invalid CertificateDN format. The Certificate DN contained null or white spaces for (CertificateDN).\"}", + }, { + name: "should not request if reset fails with an unexpected 403", + mockCalls: []mockCall{ + {"/vedsdk/certificates/reset", `{"CertificateDN":"\\VED\\Policy\\application-team-1\\app1.example.com"}`, + "403 Forbidden", `{"error":"insufficient_scope","error_description":"Grant rejected scope 'certificate:delete'. Call POST Authorize\/OAuth with the correct scope and restriction. Update the header with the new token and retry."}`}, + }, + expectErr: "while resetting certificate in case of previous enrollment: while resetting certificate due to a past failed enrollment. Status: 403 Forbidden, Body: \r{\"error\":\"insufficient_scope\",\"error_description\":\"Grant rejected scope 'certificate:delete'. Call POST Authorize\\/OAuth with the correct scope and restriction. Update the header with the new token and retry.\"}", + }, + } + + serverWith := func(t *testing.T, mockCalls []mockCall) (_ *httptest.Server, ca *x509.CertPool) { + callCount := new(int32) + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + i := atomic.AddInt32(callCount, 1) - 1 + if i >= int32(len(mockCalls)) { + t.Fatalf("expected %d calls to %s but got %d", len(mockCalls), r.URL.Path, i+1) + } + assert.Equal(t, mockCalls[i].expectReqPath, r.URL.Path) + bytes, err := ioutil.ReadAll(r.Body) + require.NoError(t, err) + assert.JSONEq(t, mockCalls[i].expectReqBody, string(bytes)) + writeRespWithCustomStatus(w, mockCalls[i].mockStatus, mockCalls[i].mockBody) + })) + t.Cleanup(server.Close) + t.Cleanup(func() { + assert.Equal(t, len(mockCalls), int(atomic.LoadInt32(callCount))) + }) + ca = x509.NewCertPool() + ca.AddCert(server.Certificate()) + return server, ca + } + for _, tt := range tests { + tt := tt // Because t.Parallel. + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + server, ca := serverWith(t, tt.mockCalls) + + tpp, err := NewConnector(server.URL, `\VED\Policy\application-team-1`, true, ca) + require.NoError(t, err) + + _, err = tpp.RequestCertificate(&certificate.Request{Subject: pkix.Name{CommonName: "app1.example.com"}}) + if tt.expectErr != "" { + assert.EqualError(t, err, tt.expectErr) + return + } + require.NoError(t, err) + }) + } +} + // The reason we are using a mock HTTP server rather than the live TPP server is // because consistently triggering the 500 error in a stage different than 0 // requires putting a powershell script on the TPP VM or turning the Microsoft From 25fb56cac8e009f470864313437953bfe3191792 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Thu, 26 Jan 2023 19:07:50 +0100 Subject: [PATCH 4/6] RequestCertificate: fix PR comments --- pkg/venafi/tpp/connector.go | 2 +- pkg/venafi/tpp/tpp.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/venafi/tpp/connector.go b/pkg/venafi/tpp/connector.go index fe321362..f517ddee 100644 --- a/pkg/venafi/tpp/connector.go +++ b/pkg/venafi/tpp/connector.go @@ -834,7 +834,7 @@ func (c *Connector) reset(certificateDN string, restart bool) error { return &errCertNotFound{fmt.Errorf(decodedResetResponse.Error)} } - return fmt.Errorf("while resetting certificate due to a past failed enrollment. Status: %s, Body: %s", status, string(body)) + 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)) } diff --git a/pkg/venafi/tpp/tpp.go b/pkg/venafi/tpp/tpp.go index 635f75da..48f3c20c 100644 --- a/pkg/venafi/tpp/tpp.go +++ b/pkg/venafi/tpp/tpp.go @@ -152,7 +152,7 @@ type certificateResetRequest struct { } type certificateResetResponse struct { - Error string `json:""` + Error string `json:"Error"` } type sanItem struct { From e8caa32599799ce62a8dd613f1ae38d5ce2bcd20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 27 Jan 2023 10:01:23 +0100 Subject: [PATCH 5/6] RequestCertificate: add the JSON annotations to the struct --- pkg/venafi/tpp/tpp.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/venafi/tpp/tpp.go b/pkg/venafi/tpp/tpp.go index 48f3c20c..db2b6ecf 100644 --- a/pkg/venafi/tpp/tpp.go +++ b/pkg/venafi/tpp/tpp.go @@ -171,8 +171,8 @@ type nameSliceValuePair struct { } type certificateRequestResponse struct { - CertificateDN string `json:",omitempty"` - Error string `json:",omitempty"` + CertificateDN string `json:"CertificateDN,omitempty"` + Error string `json:"Error,omitempty"` } type importRequest struct { From efcecd693030cb1c7a3bf61f73ab2b534f973266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Fri, 27 Jan 2023 10:19:46 +0100 Subject: [PATCH 6/6] RequestCertificate: tests: don't use "require" in the server goroutine Cf: https://github.com/stretchr/testify/issues/772#issuecomment-945166599 --- pkg/venafi/tpp/connector_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/venafi/tpp/connector_test.go b/pkg/venafi/tpp/connector_test.go index 1e22154e..0680513a 100644 --- a/pkg/venafi/tpp/connector_test.go +++ b/pkg/venafi/tpp/connector_test.go @@ -597,7 +597,7 @@ func TestRequestCertificate(t *testing.T) { } assert.Equal(t, mockCalls[i].expectReqPath, r.URL.Path) bytes, err := ioutil.ReadAll(r.Body) - require.NoError(t, err) + assert.NoError(t, err) assert.JSONEq(t, mockCalls[i].expectReqBody, string(bytes)) writeRespWithCustomStatus(w, mockCalls[i].mockStatus, mockCalls[i].mockBody) }))