Merge pull request #705 from jetstack/handle-auth-failures

[VC-43403] CyberArk(identity): remove backoff library and simplify error handling

Author: wallrj-cyberark

pkg/internal/cyberark/identity/identity.go

@@ -9,9 +9,7 @@ import (
 	"net/http"
 	"net/url"
 	"sync"
-	"time"
 
-	"github.com/cenkalti/backoff/v5"
 	"k8s.io/klog/v2"
 
 	"github.com/jetstack/preflight/pkg/logs"
@@ -209,25 +207,17 @@ func (c *Client) LoginUsernamePassword(ctx context.Context, username string, pas
 		}
 	}()
 
-	operation := func() (any, error) {
-		advanceRequestBody, err := c.doStartAuthentication(ctx, username)
-		if err != nil {
-			return struct{}{}, err
-		}
-
-		// NB: We explicitly pass advanceRequestBody by value here so that when we add the password
-		// in doAdvanceAuthentication we don't create a copy of the password slice elsewhere.
-		err = c.doAdvanceAuthentication(ctx, username, &password, advanceRequestBody)
-		if err != nil {
-			return struct{}{}, err
-		}
-
-		return struct{}{}, nil
+	advanceRequestBody, err := c.doStartAuthentication(ctx, username)
+	if err != nil {
+		return err
 	}
 
-	backoffPolicy := backoff.NewConstantBackOff(10 * time.Second)
-
-	_, err := backoff.Retry(ctx, operation, backoff.WithBackOff(backoffPolicy))
+	// NB: We explicitly pass advanceRequestBody by value here so that when we add the password
+	// in doAdvanceAuthentication we don't create a copy of the password slice elsewhere.
+	err = c.doAdvanceAuthentication(ctx, username, &password, advanceRequestBody)
+	if err != nil {
+		return err
+	}
 
 	return err
 }
@@ -281,8 +271,7 @@ func (c *Client) doStartAuthentication(ctx context.Context, username string) (ad
 		}
 
 		// If we got a 4xx error, we shouldn't retry
-		return response, backoff.Permanent(err)
-
+		return response, err
 	}
 
 	startAuthResponse := startAuthenticationResponseBody{}
@@ -354,19 +343,19 @@ func (c *Client) doStartAuthentication(ctx context.Context, username string) (ad
 // and receiving a token in response.
 func (c *Client) doAdvanceAuthentication(ctx context.Context, username string, password *[]byte, requestBody advanceAuthenticationRequestBody) error {
 	if password == nil {
-		return backoff.Permanent(fmt.Errorf("password must not be nil; this is a programming error"))
+		return fmt.Errorf("password must not be nil; this is a programming error")
 	}
 
 	requestBody.Answer = string(*password)
 
 	bodyJSON, err := json.Marshal(requestBody)
 	if err != nil {
-		return backoff.Permanent(fmt.Errorf("failed to marshal JSON for request to AdvanceAuthentication endpoint: %s", err))
+		return fmt.Errorf("failed to marshal JSON for request to AdvanceAuthentication endpoint: %s", err)
 	}
 
 	endpoint, err := url.JoinPath(c.baseURL, "Security", "AdvanceAuthentication")
 	if err != nil {
-		return backoff.Permanent(fmt.Errorf("failed to create URL for request to CyberArk Identity AdvanceAuthentication: %s", err))
+		return fmt.Errorf("failed to create URL for request to CyberArk Identity AdvanceAuthentication: %s", err)
 	}
 
 	request, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(bodyJSON))
@@ -386,13 +375,7 @@ func (c *Client) doAdvanceAuthentication(ctx context.Context, username string, p
 	// Important: Even login failures can produce a 200 status code, so this
 	// check won't catch all failures
 	if httpResponse.StatusCode != http.StatusOK {
-		err := fmt.Errorf("got unexpected status code %s from request to advance authentication in CyberArk Identity API", httpResponse.Status)
-		if httpResponse.StatusCode >= 500 || httpResponse.StatusCode < 400 {
-			return err
-		}
-
-		// If we got a 4xx error, we shouldn't retry
-		return backoff.Permanent(err)
+		return fmt.Errorf("got unexpected status code %s from request to advance authentication in CyberArk Identity API", httpResponse.Status)
 	}
 
 	advanceAuthResponse := advanceAuthenticationResponseBody{}
@@ -413,7 +396,7 @@ func (c *Client) doAdvanceAuthentication(ctx context.Context, username string, p
 	if advanceAuthResponse.Result.Summary != SummaryLoginSuccess {
 		// IF MFA was enabled and we got here, there's probably nothing to be gained from a retry
 		// and the best thing to do is fail now so the user can fix MFA settings.
-		return backoff.Permanent(fmt.Errorf("got a %s response from AdvanceAuthentication; this implies that the user account %s requires MFA, which is not supported. Try unlocking MFA for this user", advanceAuthResponse.Result.Summary, username))
+		return fmt.Errorf("got a %s response from AdvanceAuthentication; this implies that the user account %s requires MFA, which is not supported. Try unlocking MFA for this user", advanceAuthResponse.Result.Summary, username)
 	}
 
 	klog.FromContext(ctx).Info("successfully completed AdvanceAuthentication request to CyberArk Identity; login complete", "username", username)

pkg/internal/cyberark/identity/identity_test.go

@@ -0,0 +1,130 @@
+package identity
+
+// This file contains tests for the LoginUsernamePassword function in the
+// identity package. The tests cover both a mock API server and the real API,
+// depending on the environment variables set. The tests are intended to
+// demonstrate that the mock API behaves the same as the real API
+
+import (
+	"net/http"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/klog/v2"
+	"k8s.io/klog/v2/ktesting"
+
+	"github.com/jetstack/preflight/pkg/internal/cyberark/servicediscovery"
+	arktesting "github.com/jetstack/preflight/pkg/internal/cyberark/testing"
+
+	_ "k8s.io/klog/v2/ktesting/init"
+)
+
+// inputs holds the various input values for the tests.
+type inputs struct {
+	httpClient *http.Client
+	baseURL    string
+	subdomain  string
+	username   string
+	password   string
+}
+
+// TestLoginUsernamePassword_MockAPI tests the LoginUsernamePassword function
+// against a mock API server. The mock server is configured to return different
+// responses based on the username and password used in the request.
+func TestLoginUsernamePassword_MockAPI(t *testing.T) {
+	loginUsernamePasswordTests(t, func(t testing.TB) inputs {
+		baseURL, httpClient := MockIdentityServer(t)
+		return inputs{
+			httpClient: httpClient,
+			baseURL:    baseURL,
+			subdomain:  "subdomain-ignored-by-mock",
+			username:   successUser,
+			password:   successPassword,
+		}
+	})
+}
+
+// TestLoginUsernamePassword_RealAPI tests the LoginUsernamePassword function
+// against the real API. The environment variables are used to configure the
+// client.
+func TestLoginUsernamePassword_RealAPI(t *testing.T) {
+	arktesting.SkipIfNoEnv(t)
+	subdomain := os.Getenv("ARK_SUBDOMAIN")
+	httpClient := http.DefaultClient
+	services, err := servicediscovery.New(httpClient).DiscoverServices(t.Context(), subdomain)
+	require.NoError(t, err)
+
+	loginUsernamePasswordTests(t, func(t testing.TB) inputs {
+		return inputs{
+			httpClient: httpClient,
+			baseURL:    services.Identity.API,
+			subdomain:  subdomain,
+			username:   os.Getenv("ARK_USERNAME"),
+			password:   os.Getenv("ARK_SECRET"),
+		}
+	})
+}
+
+// loginUsernamePasswordTests runs tests which are expected to pass regardless of
+// whether the mock or real API is used.
+func loginUsernamePasswordTests(t *testing.T, inputsGenerator func(t testing.TB) inputs) {
+	type testCase struct {
+		name          string
+		modifier      func(in *inputs)
+		expectedError string
+	}
+	tests := []testCase{
+		{
+			name: "success",
+		},
+		{
+			name: "bad-username",
+			modifier: func(in *inputs) {
+				in.username = failureUser
+			},
+			expectedError: `^got a failure response from request to advance authentication: ` +
+				`message="Authentication \(login or challenge\) has failed\. ` +
+				`Please try again or contact your system administrator\."`,
+		},
+		{
+			name: "empty-username",
+			modifier: func(in *inputs) {
+				in.username = ""
+			},
+			expectedError: `^got a failure response from request to start authentication: ` +
+				`message="Authentication \(login or challenge\) has failed\. ` +
+				`Please try again or contact your system administrator\."`,
+		},
+		{
+			name: "bad-password",
+			modifier: func(in *inputs) {
+				in.password = "bad-password"
+			},
+			expectedError: `^got a failure response from request to advance authentication: ` +
+				`message="Authentication \(login or challenge\) has failed\. ` +
+				`Please try again or contact your system administrator\."`,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			logger := ktesting.NewLogger(t, ktesting.DefaultConfig)
+			ctx := klog.NewContext(t.Context(), logger)
+
+			in := inputsGenerator(t)
+			if test.modifier != nil {
+				test.modifier(&in)
+			}
+			cl := New(in.httpClient, in.baseURL, in.subdomain)
+			err := cl.LoginUsernamePassword(ctx, in.username, []byte(in.password))
+			if test.expectedError != "" {
+				if assert.Error(t, err) {
+					assert.Regexp(t, test.expectedError, err.Error())
+				}
+				return
+			}
+			require.NoError(t, err)
+		})
+	}
+}

pkg/internal/cyberark/identity/mock.go

@@ -17,6 +17,7 @@ import (
 
 const (
 	successUser                   = "test@example.com"
+	failureUser                   = "test-fail@example.com"
 	successUserMultipleChallenges = "test-multiple-challenges@example.com"
 	successUserMultipleMechanisms = "test-multiple-mechanisms@example.com"
 	noUPMechanism                 = "noup@example.com"
@@ -35,6 +36,9 @@ var (
 	//go:embed testdata/start_authentication_success.json
 	startAuthenticationSuccessResponse string
 
+	//go:embed testdata/start_authentication_bad_user_session_id.json
+	startAuthenticationBadUserResponse string
+
 	//go:embed testdata/start_authentication_success_multiple_challenges.json
 	startAuthenticationSuccessMultipleChallengesResponse string
 
@@ -133,11 +137,6 @@ func (mis *mockIdentityServer) handleStartAuthentication(w http.ResponseWriter,
 		return
 	}
 
-	// Important: Experimentally, the Identity server we generated the testdata from seems to accept
-	// any user and provide a success response for StartAuthentication, so filtering on the user here
-	// doesn't match the server's behavior; however, it's helpful to do so since it lets us test different
-	// error handling capabilities in the client.
-
 	switch reqBody.User {
 	case successUser:
 		w.WriteHeader(http.StatusOK)
@@ -160,9 +159,16 @@ func (mis *mockIdentityServer) handleStartAuthentication(w http.ResponseWriter,
 		w.WriteHeader(http.StatusOK)
 		_, _ = w.Write([]byte(startAuthenticationFailureResponse))
 
+	case failureUser:
+		// Experimentally, the real API produces a 200 response and what looks
+		// like a success response body. but the login is rejected later by the
+		// AdvanceAuthentication stage, perhaps by virtue of the sessionID which
+		// is returned here and supplied to AdvanceAuthentication.
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(startAuthenticationBadUserResponse))
+
 	default:
-		w.WriteHeader(http.StatusForbidden)
-		_, _ = w.Write([]byte(`{"message":"MOCK SERVER: invalid user"}`))
+		panic("programmer error: should not be reached")
 	}
 }
 

pkg/internal/cyberark/identity/testdata/start_authentication_bad_user_session_id.json

@@ -0,0 +1,40 @@
+{
+  "success": true,
+  "Result": {
+    "ClientHints": {
+      "PersistDefault": false,
+      "AllowPersist": true,
+      "AllowForgotPassword": true,
+      "EndpointAuthenticationEnabled": false
+    },
+    "Version": "1.0",
+    "SessionId": "bad-user-session-id",
+    "EventDescription": null,
+    "RetryWaitingTime": 0,
+    "SecurityImageName": null,
+    "AllowLoginMfaCache": false,
+    "Challenges": [
+      {
+        "Mechanisms": [
+          {
+            "AnswerType": "Text",
+            "Name": "UP",
+            "PromptMechChosen": "Enter Password",
+            "PromptSelectMech": "Password",
+            "MechanismId": "aaaaaaa_AAAAAAAAAAAAAAAAAAAAAAAAAAAA-1111111",
+            "Enrolled": true
+          }
+        ]
+      }
+    ],
+    "Summary": "NewPackage",
+    "TenantId": "TENANTID"
+  },
+  "Message": null,
+  "MessageID": null,
+  "Exception": null,
+  "ErrorID": null,
+  "ErrorCode": null,
+  "IsSoftError": false,
+  "InnerExceptions": null
+}

pkg/internal/cyberark/testing/testing.go

@@ -0,0 +1,18 @@
+package testing
+
+import (
+	"os"
+	"testing"
+)
+
+// SkipIfNoEnv skips the test if the required CyberArk environment variables are not set.
+func SkipIfNoEnv(t testing.TB) {
+	t.Helper()
+
+	if os.Getenv("ARK_SUBDOMAIN") == "" ||
+		os.Getenv("ARK_USERNAME") == "" ||
+		os.Getenv("ARK_SECRET") == "" {
+		t.Skip("Skipping test because one of ARK_SUBDOMAIN, ARK_USERNAME or ARK_SECRET isn't set")
+	}
+
+}