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 e4761c4b..f517ddee 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" @@ -678,7 +679,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 +691,39 @@ 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) + 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) + } + 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 { diff --git a/pkg/venafi/tpp/connector_test.go b/pkg/venafi/tpp/connector_test.go index 589eb1ff..0680513a 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,44 +532,98 @@ func TestRequestCertificateWithValidHours(t *testing.T) { DoRequestCertificateWithValidHours(t, tpp) } -func Test_shouldReset(t *testing.T) { +// 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 - givenErr error - want bool + name string + mockCalls []mockCall + givenTimeout time.Duration + expectErr string }{ { - 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, + 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) + assert.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) { - got := shouldReset(tt.givenErr) - if got != tt.want { - t.Errorf("shouldReset() = %v, want %v", got, tt.want) + 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) }) } } @@ -591,7 +647,6 @@ func TestRetrieveCertificate(t *testing.T) { tests := []struct { name string mockRetrieve []mockResp - mockReset mockResp givenTimeout time.Duration expectErr string }{ @@ -629,18 +684,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 +719,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", - 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", + 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"}`}, - {`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", + 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."}`}, - {`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", - 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 +739,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 +755,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 +769,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 +798,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..db2b6ecf 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 @@ -151,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:"Error"` +} + type sanItem struct { Type int `json:""` Name string `json:""` @@ -167,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 { @@ -357,10 +361,10 @@ 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" + urlResourceCertificateReset urlResource = "vedsdk/certificates/reset" urlResourceCertificate urlResource = "vedsdk/certificates/" urlResourceCertificateSearch = urlResourceCertificate urlResourceCertificatesList = urlResourceCertificate