Skip to content

Commit

Permalink
Revert "Change RetrieveCertificate so that it resets the enrollment (…
Browse files Browse the repository at this point in the history 
…if it exists) while retrieving the certificate"
  • Loading branch information
@maelvls
maelvls authored Jan 24, 2023
1 parent 240fe8f commit 641994a
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 641994a

Please sign in to comment.