Skip to content

Commit

Permalink
Merge pull request #274 from inteon/revert_reset
Browse files Browse the repository at this point in the history 
Revert "Merge pull request #269 from maelvls/reset-before-request"
  • Loading branch information
@luispresuelVenafi
luispresuelVenafi authored Feb 9, 2023
2 parents 240fe8f + 85ea317 commit df2a331
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 212 deletions.
44 changes: 0 additions & 44 deletions pkg/venafi/tpp/connector.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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 {
Expand Down
174 changes: 12 additions & 162 deletions pkg/venafi/tpp/connector_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -591,7 +549,6 @@ func TestRetrieveCertificate(t *testing.T) {
tests := []struct {
name string
mockRetrieve []mockResp
mockReset mockResp
givenTimeout time.Duration
expectErr string
}{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -676,106 +621,31 @@ 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.",
},
{
name: "should fail when there is a 500 after waiting for the cert",
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",
Expand All @@ -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":
Expand All @@ -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())

Expand All @@ -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
}
Expand Down
6 changes: 0 additions & 6 deletions pkg/venafi/tpp/tpp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down

0 comments on commit df2a331

Please sign in to comment.