Skip to content
This repository has been archived by the owner on Jul 1, 2024. It is now read-only.

Implement (most of the) MFA support for Okta. #14

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

pcolmer
Copy link

@pcolmer pcolmer commented Dec 22, 2017

Issue #13

I've extended the Okta authenticator to recognise the response from Okta that indicates at least one MFA is active and to act accordingly.

I have not been able to code for the hardware tokens (e.g. RSA) as I don't have one to test the code against. The implemented factors have been tested.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-io
Copy link

codecov-io commented Dec 22, 2017

Codecov Report

Merging #14 into master will decrease coverage by 1.96%.
The diff coverage is 91.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
- Coverage    98.1%   96.14%   -1.97%     
==========================================
  Files           5        5              
  Lines         317      441     +124     
  Branches       42       72      +30     
==========================================
+ Hits          311      424     +113     
- Misses          3        4       +1     
- Partials        3       13      +10
Impacted Files Coverage Δ
awsprocesscreds/saml.py 96.82% <91.53%> (-3.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a216e70...d66a173. Read the comment docs.

@pcolmer
Copy link
Author

pcolmer commented Jan 9, 2018

Just a note that although this code does work on its own, it doesn't work with AWS CLI because the latter consumes all output received from the running authentication process. If you try to use this enhancement with AWS CLI, it appears to stall but it is actually waiting for the user to specify which MFA action to take.

My initial thought was to use sys.stderr to output the prompts but the existing awsprocesscreds code uses getpass.getpass to get the user's password and that works - apparently by writing to sys.stdout.

Also, it looks like botocore might not allow stderr to be used either: aws/aws-cli#3057

So ... not sure how to proceed. I don't know if this is something that needs to be altered in awsprocesscreds or if aws-cli needs a cleaner way of getting the results back from awsprocesscreds so that the user can be allowed to interact with awsprocesscreds?

@pcolmer
Copy link
Author

pcolmer commented Jan 17, 2018

In the absence of botocore not handling stderr at the moment, I've created a separate branch on my code - revised_prompting. This uses getpass to provide all of the prompting which gets around the problem that the user can't see the output.

If the repo maintainers would prefer that version, I'll merge it into the PR.

@rprashad
Copy link

Hey @pcolmer I've patched master w/ your changes and rolled this into a docker image for beta testing, however a select group of individuals with Google Authenticator seem to not get prompted for an MFA token, any suggestions?

Copy link
Contributor

@JordonPhillips JordonPhillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good! Thanks for the pr! Could you please add in tests (and adjust the existing ones which are apparently not valid)?

awsprocesscreds/saml.py Outdated Show resolved Hide resolved
@pcolmer
Copy link
Author

pcolmer commented Apr 7, 2018

@rprashad You may want to take a look at https://github.com/pcolmer/awsprocesscreds/tree/revised_prompting instead of the commit I'm referencing in this PR. It takes a different approach to prompt the users due to a limitation in botocore.

(I've now merged revised_prompting into my master branch. The CI build is failing because of the new tests I've added but the code works :) ).

@pcolmer
Copy link
Author

pcolmer commented May 3, 2018

@JordonPhillips I'm going to need some help with writing the tests. I'm getting really stuck on how to properly handle the input and output in the tests. As you'll see from the CI build, one of the tests is failing under Python 3 because of how pytest handles input, but it passes on my computer.

To make matters worse, I've commented out one of the tests I was working on (test_process_mfa_totp) because the test process is hanging on waiting for input even though I'm re-using an approach that has worked in a different test.

I'm more than happy to put the effort into writing the tests but I'm hitting a brick wall here and just don't know what to do next.

@itaifrenkel
Copy link

@pcolmer I took a quick look on the test results. Have you considered mocking saml#get_response() in the test? This is the recommended approach as described here pytest-dev/pytest#1598 (comment)

@lorengordon
Copy link

fwiw, just tested @pcolmer fork and it works great with MFA... would love to see this supported upstream!

$ awsprocesscreds-saml -e <endpoint> -u <username> -p okta -a <role>
Password:
Please choose from the following authentication choices:
1: Push notification
2: SMS text message
3: GOOGLE authenticator app
4: OKTA authenticator app
Enter the number corresponding to your choice or press RETURN to cancel authentication: 1
Waiting for result of push notification ...press RETURN to continue
{
  "AccessKeyId": "<key>",
  "SecretAccessKey": "<secret>",
  "SessionToken": "<session>",
  "Expiration": "<expiration>",
  "Version": 1
}

@lorengordon
Copy link

@JordonPhillips anything blocking this pr? would be great to have MFA support for okta via credential_process!

@pcolmer
Copy link
Author

pcolmer commented Nov 15, 2018

@lorengordon I think the main thing blocking this PR is the lack of tests to run against the code I added. I frankly haven't had the time to write the tests lately and the maintainer won't accept the PR with such a poor code coverage.

Also, it looks like some of the tests are actually failing for some reason so that in itself is a blocker.

I do want to get this code to a state where it is acceptable to the maintainers - I am just struggling to find the time but I will try and carve some out from somewhere.

@lorengordon
Copy link

@pcolmer I can understand that. If it would help, we can commit some time starting maybe next month to get this done (need to close out other tasks first). Would just like some indication from a maintainer (@JordonPhillips) that if we get tests passing and improve coverage that the work would get merged and released (if not, would rather not spend the time).

@pcolmer
Copy link
Author

pcolmer commented Nov 15, 2018

@lorengordon certainly @JordonPhillips comment on Apr 6 suggests that, tests aside, the code would seem to be acceptable.

I have started looking at the tests again. The biggest challenge I have is that I've not written many Python tests before so trying to mock/patch routines is proving to be quite a stumbling block. I've tried @itaifrenkel suggestion but that doesn't work against the code. Part of the problem is that the input function is within the class and therefore the patching has to be done against the class instance, I believe, rather than against the class itself. Like I said, a bit of a newbie so lots of fumbling going on.

I'm going to review the other tests to see what they are doing; another part of the problem is that so much time has passed that I'm partly forgetting what I've written and what was already there … my thinking being that I can look at what was already there to try and learn from it.

Such fun :)

@pcolmer
Copy link
Author

pcolmer commented Nov 15, 2018

A small progress report … I've got past the input testing now. Currently stuck on the TOTP testing because I can't get the responses (mocking) library to react the way it seems to be documented for dynamic responses. I've raised an issue on that repo to try and get some help with that as I just can't see what I'm doing wrong.

@pcolmer
Copy link
Author

pcolmer commented Nov 16, 2018

Tests have been written and the Travis CI build now passes.

The CodeCov report hasn't been updated but coverage is now 94% for saml.py.

@lorengordon
Copy link

@pcolmer Played around with this more as a user today and there are a couple things that feel a little odd...

  1. User input is not echoed when selecting the authentication choice:
Please choose from the following authentication choices:
1: Push notification
2: GOOGLE authenticator app
3: SMS text message
4: OKTA authenticator app
Enter the number corresponding to your choice or press RETURN to cancel authentication:

Entering a passcode is echoed, so it seems it should be possible to echo the response at that prompt.

  1. Using Push notification or SMS text message results in an extra prompt that requires the user to press RETURN:
# push notification selected
Enter the number corresponding to your choice or press RETURN to cancel authentication:
Waiting for result of push notification ...press RETURN to continue
# <push not sent until RETURN pressed again>

# sms selected
Enter the number corresponding to your choice or press RETURN to cancel authentication:
Requesting code to be sent to your phone ... press RETURN to continue
# <sms not sent until RETURN pressed again>
Authentication code (RETURN to cancel, 'RESEND' to get new code sent): <code echoed>

awsprocesscreds/saml.py Outdated Show resolved Hide resolved
awsprocesscreds/saml.py Outdated Show resolved Hide resolved
awsprocesscreds/saml.py Outdated Show resolved Hide resolved
@pcolmer
Copy link
Author

pcolmer commented Nov 20, 2018

@lorengordon thanks for the feedback and code inspection. I'm a bit torn over the MFA push and SMS changes, to be honest, partly because I wanted to provide feedback to the user as to what is happening and there is a limitation in the current Boto library where it "swallows" anything sent to stdout and stderr (boto/botocore#1348). This is why I'm using _password_prompter as a backdoor mechanism to inform the user of what is happening, but it does have the downside of needing the user to press RETURN to continue.

For SMS, it can be somewhat mitigated by swapping the call to _password_prompter with the call to verify_sms_factor, in that this will then cause the SMS code to be delivered to your phone even if you haven't pressed RETURN yet.

For MFA Push, I do think the user needs to know that awsprocesscreds is triggering a push notification so that they get their phone/device ready. Without that prompt, awsprocesscreds could appear to just hang whilst it is waiting for the user's action.

Finally, thanks for the suggested change from _password_prompter to get_response. I'm not sure why I didn't have it written like that in the first place :). I'll make that change when we've agreed on the best way forwards with the other points.

@lorengordon
Copy link

lorengordon commented Nov 20, 2018

@pcolmer I suppose I feel like when I answer the prompt for the authentication type, that if I choose push or sms, then I am expecting the notification/text to be sent immediately.

As it is,

Without that prompt, awsprocesscreds could appear to just hang whilst it is waiting for the user's action.

Hanging is exactly what it does now that I find confusing. It is not clear that I must press RETURN again for some reason to get the notification.

I actually did make the suggested changes locally, and it worked perfectly.

@pcolmer
Copy link
Author

pcolmer commented Nov 20, 2018

@lorengordon the problem with removing those prompts, though, is that the user has no indication of what the script is doing. Suppose they haven't realised that their device hasn't got a signal to receive SMS or Internet access to receive Push? Yes, the code works perfectly without the prompts but I do feel it is less informative to the user.

Also, are you testing this code by running it directly or by calling it from within AWS CLI? The reason why get_mfa_choice uses _password_prompter is because get_response uses input, which outputs the prompt to stdout, and that breaks the JSON response returned from awsprocesscreds to AWS CLI. So, in actual fact, rather than switching from _password_prompter to get_response, I've realised that I've got to go the other way and stop using get_response completely.

Basically the link between AWS CLI and awsprocesscreds relies on the latter only emitting a valid JSON block on stdout, which means that the script can only use stderr to display anything to the user, and Boto swallows that. The only workaround I've managed to find is to use _password_prompter which uses getpass. Now, I could try and dig into the source for getpass and try to replicate that inside the script but it would hopefully only be short-lived and would require me to add more tests.

I can try rewording the prompts to see if that makes it clearer?

@lorengordon
Copy link

Oh, quite right! I was testing execution directly for the sake of expediency. Sorry! 🤕

Rewording the prompts would help I think. Maybe:

"Press RETURN when you are ready to request the SMS text message"
and
"Press RETURN when you are ready to request the push notification"

@lorengordon
Copy link

lorengordon commented Nov 20, 2018

Actually, for SMS, changing this line to use _password_prompter and deleting the first prompt to press RETURN a second time works pretty well...

response = self.get_response(self._MSG_SMS_CODE)  # change to self._password_prompter

Using it looks like this:

$ aws s3 ls --profile okta
Password:
Please choose from the following authentication choices:
1: SMS text message
2: GOOGLE authenticator app
3: Push notification
4: OKTA authenticator app
Enter the number corresponding to your choice or press RETURN to cancel authentication:
Authentication code (RETURN to cancel, 'RESEND' to get new code sent):

@pcolmer
Copy link
Author

pcolmer commented Nov 20, 2018

OK - I've updated the user prompting code and I've also restructured some of the code in order to allow me to get a higher coverage with the tests. I'll see if I can get the coverage any higher.

@lorengordon
Copy link

Tested and looks great, nice work!

@pcolmer
Copy link
Author

pcolmer commented Dec 3, 2018

@JordonPhillips can you please let me know if anything further needs to be done for this PR in order to facilitate getting it merged into the base branch?

Or is this repo dead? I find it a bit concerning that it hasn't been touched since it was announced at last year's re:Invent. Is that because the functionality is going to be folded into CLI 2?

I want to push ahead with adopting Okta for federated authentication inside my company but I can't until the MFA support is there. This PR needs to be merged for that to happen, or for AWS to provide their own alternative code for MFA support.

Thanks.

@lorengordon
Copy link

@pcolmer If Amazon isn't going to maintain this project, perhaps someone (maybe you or I) could fork it and maintain it instead?

@pcolmer
Copy link
Author

pcolmer commented Dec 11, 2018

@lorengordon I agree that that would seem to be the sensible way forward … but I would prefer it if someone from Amazon could categorically state what is happening with this repo. Is it dead? It is being subsumed into CLI 2?

I'm happy to maintain my own contribution but my coding skills are not strong enough to maintain the whole repo, I'm afraid.

@lorengordon
Copy link

This project isn't all that active, and we have experience publishing packages to PyPi and generating self-contained executables. I don't expect it would be much of a burden to maintain (famous last words). I'll talk with our team and explore the code base, and if we don't hear back from @JordonPhillips, we may be able to take it on.

Or perhaps, another option would be to add a compatible credential_process outputter to another project, and maybe okta support as well:

@pcolmer
Copy link
Author

pcolmer commented Dec 11, 2018

Just a little bit of feedback on the other potential projects:

  • saml2aws already supports Okta
  • I like how awscli-login integrates with AWS CLI as a login provider. It looks like it supports 2-factor so this could be worth further investigation/testing.
  • aws-saml is written in Ruby. I'd personally prefer to stick to one programming language.

I'll try and find the time for a closer look at awscli-login.

@lorengordon
Copy link

Hi @pcolmer, any chance you might be able to resolve the new conflicts?

@pcolmer
Copy link
Author

pcolmer commented Feb 9, 2019

Hi @lorengordon, all sorted.

@ghost
Copy link

ghost commented Oct 31, 2019

Using @pcolmer fork I am having issues.

I am able to get a token, but I get a SAML error and it does not prompt me for MFA.

`C:>awsprocesscreds-saml -e -u -p okta -a
Password: --verbose
Password:
Sending HTTP POST with username (username) and password to Okta API endpoint: (endpoint)
Got status 200 and response: {"expiresAt":"2019-10-31T18:06:20.000Z","status":"SUCCESS","sessionToken":"sessionToken","_embedded":{"user":{"id":"id","profile":{"login":"login","firstName":"firstName","lastName":"lastName","locale":"en","timeZone":"America/Los_Angeles"}}},"_links":{"cancel":{"href":"href","hints":{"allow":["POST"]}}}}
Received HTTP response of status code: 200
Traceback (most recent call last):
File "C:\Users\user\AppData\Local\Programs\Python\Python36\Scripts\awsprocesscreds-saml-script.py", line 11, in
load_entry_point('awsprocesscreds==0.0.2', 'console_scripts', 'awsprocesscreds-saml')()
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\awsprocesscreds\cli.py", line 81, in saml
creds = fetcher.fetch_credentials()
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\awsprocesscreds\saml.py", line 555, in fetch_credentials
creds = super(SAMLCredentialFetcher, self).fetch_credentials()
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\botocore\credentials.py", line 643, in fetch_credentials
return self._get_cached_credentials()
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\botocore\credentials.py", line 653, in _get_cached_credentials
response = self._get_credentials()
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\awsprocesscreds\saml.py", line 564, in _get_credentials
kwargs = self._get_assume_role_kwargs()
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\awsprocesscreds\saml.py", line 605, in _get_assume_role_kwargs
assertion = self._authenticator.retrieve_saml_assertion(config)
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\awsprocesscreds\saml.py", line 442, in retrieve_saml_assertion
return self.get_assertion_from_response(endpoint, parsed)
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\awsprocesscreds\saml.py", line 262, in get_assertion_from_response
r = self._extract_saml_assertion_from_response(response.text)
File "c:\users\user\appdata\local\programs\python\python36\lib\site-packages\awsprocesscreds\saml.py", line 210, in _extract_saml_assertion_from_response
raise SAMLError(self._ERROR_LOGIN_FAILED)
awsprocesscreds.saml.SAMLError: Login failed, could not retrieve SAML assertion. Double check you have entered your password correctly.

@lorengordon
Copy link

@livt0ride Maybe try using aws-okta-processor instead. That project implements credential_process support, works with MFA, and has responsive maintainers.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants