Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Export credentials as JSON, for use as an AWS CLI external process #5

Merged
merged 3 commits into from
Sep 27, 2019

Conversation

ajkerrigan
Copy link
Contributor

Hi there @wnkz , thanks a lot for this excellent utility!

I've been testing it out locally and it makes AWS SSO from the CLI much more pleasant. I made some local tweaks so that I can use aws-sso in the credential_process section of the AWS CLI configuration as described here. Would you be interested in having that merged back upstream (either as-is, or with any modifications you think are necessary)?

@wnkz
Copy link
Owner

wnkz commented Sep 27, 2019

@ajkerrigan thanks ! Sounds good, I'll have a look very soon 👌

awssso/saml.py Outdated
self._sts = boto3.client('sts')
def __init__(self, aws_profile, encoded_payload):
self._session = boto3.Session(profile_name=aws_profile)
self._sts = self._session.client('sts')
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this change is needed since the sts client does not use existing credentials ; any particular reason for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this change lets you use credential_process for one profile, but use a separate profile as an anchor point for the STS call. Otherwise boto3 goes down a recursive rabbit hole of credential_process. I see you made some other related comments, so we can chat more there!

Copy link
Owner

Choose a reason for hiding this comment

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

I've run some tests and using something like this works

self._sts = boto3.client('sts', aws_access_key_id='', aws_secret_access_key='', aws_session_token='')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good call, I like that. I was going to suggest creating an explicit awssso_samlhelper profile, where the credentials wouldn't even need to be valid. Your way seems cleaner 👍

awssso/cli.py Outdated
@@ -139,13 +139,15 @@ def login(args):
], answers=params, raise_keyboard_interrupt=True)

payload = sso.get_saml_payload(params['instance_id'], params['profile_id'])
saml = SAMLHelper(payload)
saml = SAMLHelper(aws_profile, payload)
Copy link
Owner

Choose a reason for hiding this comment

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

see comment on saml.py

@wnkz
Copy link
Owner

wnkz commented Sep 27, 2019

Currently I see few things to improve this feature:

  • Ensure SAMLHelper is not using any credentials (if you are currently using the same AWS profile as the one using credential_process configuration, you end up in an infinite process loop)
  • Handle some sort of caching system (like awscli does) so we don't make an STS call each time
  • Check what happens when awssso asks for input like MFA code and such

@wnkz
Copy link
Owner

wnkz commented Sep 27, 2019

On caching, I think this can wait.
On MFA, awscli does hang when awssso prompts for MFA Code and the prompt does not show so this is weird at best. We could explicitly return an error in this specific use case because credential_process should not be interactive and maybe implement a dedicated refresh command. Anyway, this can wait as well.

@ajkerrigan
Copy link
Contributor Author

I'm definitely with you on the first point, I like your suggested improvements to make SAMLHelper's starting credentials more obvious and focused.

Caching (ideally in keyring) seems like a solid improvement, but probably as part of a separate change. That would affect all uses (credential_process or not), and I was aiming for a small minimally invasive change here to avoid breaking your stuff unintentionally :).

As for the interactive MFA input, this is busted right now because the CLI is swallowing I/O. I need to look at that, hopefully fixing it isn't too deep a hole.

@wnkz wnkz added the enhancement New feature or request label Sep 27, 2019
@wnkz wnkz merged commit 3d4728a into wnkz:master Sep 27, 2019
@wnkz
Copy link
Owner

wnkz commented Sep 27, 2019

@ajkerrigan thanks a lot for your help and feedback, I'll publish a new release including this feature very soon and open issues for the problems we discussed about.

@wnkz wnkz mentioned this pull request Sep 27, 2019
@ajkerrigan
Copy link
Contributor Author

Thanks @wnkz for the quick review/improvement/merge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants