diff --git a/pkg/internal/cyberark/identity/identity.go b/pkg/internal/cyberark/identity/identity.go index 1e48eeb1..51b1620b 100644 --- a/pkg/internal/cyberark/identity/identity.go +++ b/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) diff --git a/pkg/internal/cyberark/identity/identity_test.go b/pkg/internal/cyberark/identity/identity_test.go new file mode 100644 index 00000000..c392a0d8 --- /dev/null +++ b/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) + }) + } +} diff --git a/pkg/internal/cyberark/identity/mock.go b/pkg/internal/cyberark/identity/mock.go index d904a695..5f5d579d 100644 --- a/pkg/internal/cyberark/identity/mock.go +++ b/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") } } diff --git a/pkg/internal/cyberark/identity/testdata/start_authentication_bad_user_session_id.json b/pkg/internal/cyberark/identity/testdata/start_authentication_bad_user_session_id.json new file mode 100644 index 00000000..7e163a88 --- /dev/null +++ b/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 +} diff --git a/pkg/internal/cyberark/testing/testing.go b/pkg/internal/cyberark/testing/testing.go new file mode 100644 index 00000000..7b74abcc --- /dev/null +++ b/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") + } + +}