Skip to content

Commit 402ea1f

Browse files
SgtCoDFishSgtCoDFish
committed
rework challenge handling to expect exactly one challenge
Signed-off-by: Ashley Davis <[email protected]>
1 parent cbb3aeb commit 402ea1f

File tree

6 files changed

+93
-44
lines changed

6 files changed

+93
-44
lines changed

‎pkg/internal/cyberark/identity/identity.go‎

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ type advanceAuthenticationResponseResult struct {
172172
// Other fields omitted as they're not needed
173173
}
174174

175-
// Client is an client for interacting with the CyberArk Identity API and performing a login
175+
// Client is an client for interacting with the CyberArk Identity API and performing a login using a username and password.
176+
// For context on the behaviour of this client, see the Pytho SDK: https://github.com/cyberark/ark-sdk-python/blob/3be12c3f2d3a2d0407025028943e584b6edc5996/ark_sdk_python/auth/identity/ark_identity.py
176177
type Client struct {
177178
client *http.Client
178179

@@ -331,41 +332,41 @@ func (c *Client) doStartAuthentication(ctx context.Context, username string) (ad
331332
klog.FromContext(ctx).Info("got an unexpected Summary from StartAuthentication response; will attempt to complete a login challenge anyway", "summary", startAuthResponse.Result.Summary)
332333
}
333334

334-
if len(startAuthResponse.Result.Challenges) == 0 {
335+
// We can only handle a UP type challenge, and if there are any other challenges, we'll have to fail because we can't handle them.
336+
// https://github.com/cyberark/ark-sdk-python/blob/3be12c3f2d3a2d0407025028943e584b6edc5996/ark_sdk_python/auth/identity/ark_identity.py#L405
337+
switch len(startAuthResponse.Result.Challenges) {
338+
case 0:
335339
return response, fmt.Errorf("got no valid challenges in response to start authentication; unable to log in")
336-
}
337340

338-
mechanismID := ""
341+
case 1:
342+
// do nothing, this is ideal
339343

340-
for i, challenge := range startAuthResponse.Result.Challenges {
341-
logger.V(logs.Debug).Info("found a challenge", "idx", i, "mechanismCount", len(challenge.Mechanisms))
344+
default:
345+
return response, fmt.Errorf("got %d challenges in response to start authentication, which means MFA may be enabled; unable to log in", len(startAuthResponse.Result.Challenges))
346+
}
342347

343-
if len(challenge.Mechanisms) == 0 {
344-
// presumably this shouldn't happen, but handle the case anyway
345-
logger.Info("got no mechanisms for challenge from Identity server; skipping this challenge")
346-
continue
347-
}
348+
challenge := startAuthResponse.Result.Challenges[0]
348349

349-
for j, mechanism := range challenge.Mechanisms {
350-
logger.V(logs.Debug).Info("found a mechanism in challenge", "idx", j, "enrolled", mechanism.Enrolled, "name", mechanism.Name)
350+
switch len(challenge.Mechanisms) {
351+
case 0:
352+
// presumably this shouldn't happen, but handle the case anyway
353+
return response, fmt.Errorf("got no mechanisms for challenge from Identity server")
351354

352-
if !mechanism.Enrolled || mechanism.Name != MechanismUsernamePassword {
353-
continue
354-
}
355+
case 1:
356+
// do nothing, this is ideal
355357

356-
// got a username/password mechanism, so use it
357-
mechanismID = mechanism.MechanismID
358-
break
359-
}
358+
default:
359+
return response, fmt.Errorf("got %d mechanisms in response to start authentication, which means MFA may be enabled; unable to log in", len(challenge.Mechanisms))
360360
}
361361

362-
if mechanismID == "" {
363-
klog.FromContext(ctx).Info("ctx", "response", startAuthResponse)
362+
mechanism := challenge.Mechanisms[0]
363+
364+
if !mechanism.Enrolled || mechanism.Name != MechanismUsernamePassword {
364365
return response, errNoUPMechanism
365366
}
366367

367368
response.Action = ActionAnswer
368-
response.MechanismID = mechanismID
369+
response.MechanismID = mechanism.MechanismID
369370
response.SessionID = startAuthResponse.Result.SessionID
370371
response.TenantID = c.subdomain
371372
response.PersistantLogin = true

‎pkg/internal/cyberark/identity/mock.go‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ import (
1414

1515
const (
1616
successUser = "[email protected]"
17-
successUserChallengesSwitched = "[email protected]"
17+
successUserMultipleChallenges = "[email protected]"
18+
successUserMultipleMechanisms = "[email protected]"
1819
noUPMechanism = "[email protected]"
1920

2021
successMechanismID = "aaaaaaa_AAAAAAAAAAAAAAAAAAAAAAAAAAAA-1111111"
@@ -31,8 +32,11 @@ var (
3132
//go:embed testdata/start_authentication_success.json
3233
startAuthenticationSuccessResponse string
3334

34-
//go:embed testdata/start_authentication_success_challenges_switched.json
35-
startAuthenticationSuccessChallengesSwitchedResponse string
35+
//go:embed testdata/start_authentication_success_multiple_challenges.json
36+
startAuthenticationSuccessMultipleChallengesResponse string
37+
38+
//go:embed testdata/start_authentication_success_multiple_mechanisms.json
39+
startAuthenticationSuccessMultipleMechanismsResponse string
3640

3741
//go:embed testdata/start_authentication_success_no_up_mechanism.json
3842
startAuthenticationNoUPMechanismResponse string
@@ -136,9 +140,13 @@ func (mis *mockIdentityServer) handleStartAuthentication(w http.ResponseWriter,
136140
w.WriteHeader(http.StatusOK)
137141
_, _ = w.Write([]byte(startAuthenticationSuccessResponse))
138142

139-
case successUserChallengesSwitched:
143+
case successUserMultipleChallenges:
144+
w.WriteHeader(http.StatusOK)
145+
_, _ = w.Write([]byte(startAuthenticationSuccessMultipleChallengesResponse))
146+
147+
case successUserMultipleMechanisms:
140148
w.WriteHeader(http.StatusOK)
141-
_, _ = w.Write([]byte(startAuthenticationSuccessChallengesSwitchedResponse))
149+
_, _ = w.Write([]byte(startAuthenticationSuccessMultipleMechanismsResponse))
142150

143151
case noUPMechanism:
144152
w.WriteHeader(http.StatusOK)

‎pkg/internal/cyberark/identity/start_authentication_test.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,13 @@ func Test_IdentityStartAuthentication(t *testing.T) {
1818
username: successUser,
1919
expectedError: nil,
2020
},
21-
"successful request, challenges switched": {
22-
username: successUserChallengesSwitched,
23-
expectedError: nil,
21+
"successful request, multiple challenges": {
22+
username: successUserMultipleChallenges,
23+
expectedError: fmt.Errorf("got 2 challenges in response to start authentication, which means MFA may be enabled; unable to log in"),
24+
},
25+
"successful request, multiple mechanisms": {
26+
username: successUserMultipleMechanisms,
27+
expectedError: fmt.Errorf("got 2 mechanisms in response to start authentication, which means MFA may be enabled; unable to log in"),
2428
},
2529
"successful request, no username / password (UP) mechanism available": {
2630
username: noUPMechanism,

‎pkg/internal/cyberark/identity/testdata/start_authentication_success.json‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -25,19 +25,6 @@
2525
"Enrolled": true
2626
}
2727
]
28-
},
29-
{
30-
"Mechanisms": [
31-
{
32-
"AnswerType": "StartOob",
33-
"Name": "PF",
34-
"PartialPhoneNumber": "0775",
35-
"PromptMechChosen": "We will now attempt to call your phone (0000). Please follow the instructions to proceed with authentication.",
36-
"PromptSelectMech": "Phone Call... XXX-0000",
37-
"MechanismId": "bbbbbbb_BBBBBBBBBBBBBBBBBBBBBBBBBBBB-2222222",
38-
"Enrolled": true
39-
}
40-
]
4128
}
4229
],
4330
"Summary": "NewPackage",

pkg/internal/cyberark/identity/testdata/start_authentication_success_challenges_switched.json renamed to pkg/internal/cyberark/identity/testdata/start_authentication_success_multiple_challenges.json

File renamed without changes.

‎pkg/internal/cyberark/identity/testdata/start_authentication_success_multiple_mechanisms.json‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
{
2+
"success": true,
3+
"Result": {
4+
"ClientHints": {
5+
"PersistDefault": false,
6+
"AllowPersist": true,
7+
"AllowForgotPassword": true,
8+
"EndpointAuthenticationEnabled": false
9+
},
10+
"Version": "1.0",
11+
"SessionId": "mysessionid101",
12+
"EventDescription": null,
13+
"RetryWaitingTime": 0,
14+
"SecurityImageName": null,
15+
"AllowLoginMfaCache": false,
16+
"Challenges": [
17+
{
18+
"Mechanisms": [
19+
{
20+
"AnswerType": "Text",
21+
"Name": "UP",
22+
"PromptMechChosen": "Enter Password",
23+
"PromptSelectMech": "Password",
24+
"MechanismId": "aaaaaaa_AAAAAAAAAAAAAAAAAAAAAAAAAAAA-1111111",
25+
"Enrolled": true
26+
},
27+
{
28+
"AnswerType": "StartOob",
29+
"Name": "PF",
30+
"PartialPhoneNumber": "0775",
31+
"PromptMechChosen": "We will now attempt to call your phone (0000). Please follow the instructions to proceed with authentication.",
32+
"PromptSelectMech": "Phone Call... XXX-0000",
33+
"MechanismId": "bbbbbbb_BBBBBBBBBBBBBBBBBBBBBBBBBBBB-2222222",
34+
"Enrolled": true
35+
}
36+
]
37+
}
38+
],
39+
"Summary": "NewPackage",
40+
"TenantId": "TENANTID"
41+
},
42+
"Message": null,
43+
"MessageID": null,
44+
"Exception": null,
45+
"ErrorID": null,
46+
"ErrorCode": null,
47+
"IsSoftError": false,
48+
"InnerExceptions": null
49+
}

0 commit comments

Comments
 (0)