warrant icon indicating copy to clipboard operation
warrant copied to clipboard

Authentication failed not handling nested challenges

Open blade2005 opened this issue 8 years ago • 9 comments
trafficstars

Utilizing the develop branch commit a53cb082b6a07f32ddc5c9cacff6f5dfe926f170

Using this code which seems to be entirely correct doesn't seem to authenticate correctly

    config = ce.get_config('Cognito', args.configfile)
    u = Cognito(config['pool_id'],config['client_id'], username=args.username)
    response = u.authenticate(args.password)

2017-04-11 12:24:16 DEBUG parsers.parse Response body:
{"ChallengeName":"NEW_PASSWORD_REQUIRED","ChallengeParameters":{"requiredAttributes":"[]","userAttributes":"{\"email_verified\":\"true\",\"email\":\"<ommitted email>\"}"},"Session":"<sessiong string ommitted>"}
2017-04-11 12:24:16 DEBUG hooks._emit Event needs-retry.cognito-idp.RespondToAuthChallenge: calling handler <botocore.retryhandler.RetryHandler object at 0x7fc01d531d90>
2017-04-11 12:24:16 DEBUG retryhandler.__call__ No retry needed.
Traceback (most recent call last):
  File "bin/auth-cognito", line 37, in <module>
    main()
  File "bin/auth-cognito", line 33, in main
    response = u.authenticate(args.password)
  File "~/.local/lib/python2.7/site-packages/warrant/__init__.py", line 213, in authenticate
    self.id_token = tokens['AuthenticationResult']['IdToken']
KeyError: 'AuthenticationResult'

It doesn't look like there is support for Challenges of NEW_PASSWORD_REQUIRED and it failed to raise like it should https://github.com/capless/warrant/blob/develop/warrant/aws_srp.py#L187

blade2005 avatar Apr 11 '17 17:04 blade2005

Looks like it assumes there will only be one response challenge. We'll need to add the ability to complete the challenge with an additional input (the new password).

ebpetway avatar Apr 11 '17 18:04 ebpetway

Yeah I also just realized my initial diagnosis in where I thought the problem was was wrong. And you've hit the nail on the head for the conclusion I just came to.

blade2005 avatar Apr 11 '17 18:04 blade2005

The handling in #15 isn't perfect for NEW_PASSWORD_REQUIRED I'd like there to be a module global which is a callable and returns back the password. Allows users to specify a system to get the password rather than using a TTY is present.

blade2005 avatar Apr 12 '17 12:04 blade2005

I like the way #25 handles the authentication, it raises by default and then lets the user come up with their own way of getting the password which keeps in line with a web usage model.

blade2005 avatar Apr 28 '17 12:04 blade2005

Based on your suggested workflow @bjinwright I'm working on this aspect. Not sure if any one else is.

However admin_get_user looks like it requires developer credentials, credentials which we won't have. How would we get the user status without being able to login. I'm assuming a web framework which and the user provides credentials which calls warrant which calls cognito. Those are the only credentials in the framework. Can you expound on how your proposed workflow executes that?

blade2005 avatar Apr 28 '17 12:04 blade2005

Thanks for the feedback that makes sense

bjinwright avatar Apr 28 '17 12:04 bjinwright

So with that in mind we may need to implement the remaining work flows based on challenge types rather than user status. What do you think?

blade2005 avatar Apr 28 '17 13:04 blade2005

For historical purpose I'm still going to put my work for user statuses:

UNCONFIRMED - User has been created but not confirmed. ConfirmSignUp

CONFIRMED - User has been confirmed. Normal workflow

ARCHIVED - User is no longer active. Not sure there is a workflow here.

COMPROMISED - User is disabled due to a potential security threat. Not sure there is a workflow here.

UNKNOWN - User status is not known. Not sure there is a workflow here.

RESET_REQUIRED - User has issued a reset password request but not reset it. ConfirmForgotPassword, possibly RespondToAuthChallenge

FORCE_CHANGE_PASSWORD - User has not authenticated for the first time and must change password RespondToAuthChallenge

blade2005 avatar Apr 28 '17 13:04 blade2005

The current state:

  • Warrant raises ForceChangePasswordException for users which are required to change their passwords after successful first login (NEW_PASSWORD_REQUIRED challenge).

  • new_password_challenge method is not documented

  • Warrant raises NotImplementedError when a challenge is not supported.

  • Warrant doesn't support SMS_MFA, CUSTOM_CHALLENGE, DEVICE_SRP_AUTH DEVICE_PASSWORD_VERIFIER challenges.

The first post says about the problem with the NEW_PASSWORD_REQUIRED challenge only and this issue is fixed. The title has more broad meaning, it implies that warrant should support all challenges.

@bjinwright The decision of closing the issue depends on what to take into account: the title or the first post.

armicron avatar Sep 17 '17 19:09 armicron