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

add duration parameter #3

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

Conversation

colbyprior
Copy link

Working on something over the day got annoying when my credentials kept expiring. This allows a duration to be set for the temporary keys but defaults to 1 hour.

@aidan-
Copy link
Owner

aidan- commented Apr 5, 2018

AWS have announced the ability to increase the assume-role time. It looks like your patch may now work, but you need to adjust the Maximum CLI/API session duration value on a per Role basis.

Do you want to try this and see if your PR still works? I'm not sure what happens if you try and set the expiration to greater than the set max value. Perhaps the user should be informed that the value can/needs to be increased?

It might also be a good idea to do some checking to make sure the user doesn't set it passed 12hours (that seems to be the upper limit).

EDIT: link to AWS announcement: https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_use.html?icmpid=docs_iam_console

@colbyprior
Copy link
Author

It works with the higher duration now! I also tested using the -account flag to use a different duration and added it to the readme.

If you try to use a duration higher than allowed for the role the error that comes through is pretty readable by default:

ERROR: Failed to assume role: Unable to assume role: ValidationError: The requested DurationSeconds exceeds the MaxSessionDuration set for this role.

Should I catch this and rewrite the error to specifically suggest "you need to lower your duration value below the MaxSessionDuration"?

@d-lord
Copy link

d-lord commented May 8, 2018

👋
In my opinion, yes, because a small tool like this is easy to make extremely user-friendly.

I'm going to try out your fork @colbyprior and give any, ahem, feedback.

@aidan-
Copy link
Owner

aidan- commented May 8, 2018

@colbyprior Nice work! I think its worth capturing this specific exception and returning a more friendly error with some details about this new feature. The configuration itself may not be very obvious for someone who isn't aware of this feature change (most people just assume you cannot do it).

The other thing I am still pondering about is how best to allow a user to configure an increased role duration. Implementing it against the account config might not be the best place, as it is very likely that one role has the extended duration enabled while no other roles in that account do. This would result in the user unable to assume these other roles.

@colbyprior
Copy link
Author

Berg reminded me to look at this again. So the way I was managing accounts that had limits on their session duration was like this:

[account_map]
0123456789 = account-1
1123456789 = account-2

[default]
sp_identity_url = https://example.com/

[long]
sp_identity_url = https://example.com/
duration = 7200

Then calling it with aws-cli-federator --account long. This is still the case however I also added a flag --duration so that you can override the value.
eg: aws-cli-federator --duration 7200

What do you think?

@aidan-
Copy link
Owner

aidan- commented Nov 2, 2018

@colbyprior That's looking good! I still think it would be ideal to capture the error raised when the user provides a duration greater than what the configured maximum is for that role and return something a bit more helpful. Even if its the same message but informing the user that this is a Role specific AWS configuration.

You need to either change the requested duration to be >900 and <3600 or alter the role's MaxSessionDuration via the console.

@aidan- aidan- closed this Nov 2, 2018
@aidan- aidan- reopened this Nov 2, 2018
@colbyprior
Copy link
Author

I struggled a bit with getting code to catch the exception, I hope I added that correctly.

It will now catch the ValidationError and display that error if the session duration is higher then what the account allows.

@gtmtech
Copy link

gtmtech commented Apr 19, 2019

@colbyprior @aidan- - will this be merged anytime soon? Thanks!

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

Successfully merging this pull request may close these issues.

4 participants