Skip to content

Change RetrieveCertificate so that it resets the enrollment (if it exists) while retrieving the certificate #269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
luispresuelVenafi merged 1 commit into from
Dec 16, 2022
Merged

Change RetrieveCertificate so that it resets the enrollment (if it exists) while retrieving the certificate #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions pkg/venafi/tpp/connector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1232,9 +1232,32 @@ 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 @@ -1256,6 +1279,27 @@ 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.") ||
Comment thread
luispresuelVenafi marked this conversation as resolved.
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: 162 additions & 12 deletions pkg/venafi/tpp/connector_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,48 @@ 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 @@ -549,6 +591,7 @@ func TestRetrieveCertificate(t *testing.T) {
tests := []struct {
name string
mockRetrieve []mockResp
mockReset mockResp
givenTimeout time.Duration
expectErr string
}{
Expand Down Expand Up @@ -586,6 +629,18 @@ 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 @@ -621,31 +676,106 @@ 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 fail on msg WebSDK CertRequest",
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",
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 on msg Click Retry",
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",
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 @@ -657,9 +787,8 @@ 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(mockRetrieve []mockResp) (_ *httptest.Server, retrieveCount *int32) {
retrieveCount = new(int32)
serverWith := func(t *testing.T, mockRetrieve []mockResp, mockReset mockResp) (_ *httptest.Server, retrieveCount, resetCount *int32) {
retrieveCount, resetCount = new(int32), new(int32)
server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch {
case r.URL.Path == "/vedsdk/certificates/retrieve":
Expand All @@ -671,25 +800,40 @@ func TestRetrieveCertificate(t *testing.T) {
req := certificateRetrieveRequest{}
_ = json.NewDecoder(r.Body).Decode(&req)
if req.CertificateDN != `\VED\Policy\Test\bexample.com` {
t.Fatalf("/retrieve: expected CertificateDN to be '%s' but got '%s'", `\VED\Policy\Test\bexample.com`, req.CertificateDN)
t.Errorf("/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
return server, retrieveCount, resetCount
}
for _, tt := range tests {
tt := tt // Because t.Parallel.
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
server, retrieveCount := serverWith(tt.mockRetrieve)
server, retrieveCount, resetCount := serverWith(t, tt.mockRetrieve, tt.mockReset)
trusted := x509.NewCertPool()
trusted.AddCert(server.Certificate())

Expand All @@ -700,11 +844,17 @@ 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.Fatalf("tpp.RetrieveCertificate: expected %d calls to /certificates/retrieve, but got %d", len(tt.mockRetrieve), atomic.LoadInt32(retrieveCount))
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))
}
if tt.expectErr != "" {
if err == nil || err.Error() != tt.expectErr {
t.Fatalf("tpp.RetrieveCertificate: expected err to be %q but got %q", tt.expectErr, err)
t.Fatalf("tpp.RetrieveCertificate: \nexpected: %q\ngot: %q", tt.expectErr, err)
}
return
}
Expand Down
6 changes: 6 additions & 0 deletions pkg/venafi/tpp/tpp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ 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 @@ -352,6 +357,7 @@ 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