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

WIP Commit. #656

Closed
wants to merge 4 commits into from
Closed

WIP Commit. #656

wants to merge 4 commits into from

Conversation

garnaat
Copy link
Contributor

@garnaat garnaat commented Feb 18, 2014

This is not ready for prime time yet. Also, this depends on boto/botocore#226.

I have session start/end working and also the launching of the web console with session credentials. Needs lots of cleanup and tests and support for federated and SAML credentials. I am using this at the moment with a lot of assumed roles using an IAM account in the dev environment that is authorized to assume certain roles in the prod environment. It's actually working pretty well and being able to launch the web console with the temporary credentials is really nice.

I don't like how you have to remember the ARN of the role you want to assume. It would be much nicer to be able to provide easy-to-remember names for roles in the config file. Perhaps something like:

[Roles]
myrole1 = ARN of IAM Role

Also, I'm still not 100% sold on the modal nature of this. You basically start a session via AssumeRole or some other mechanism and then everything you do with the CLI until you end the session is with those credentials. The alternative would be to allow for multiple named sessions and to require the user to specify a --session-name option to explicitly use a session for each command. That's kind of a pain and I think I like the start/end idea better but it can be kind of dangerous if you forget you started a session.

…f the web console with session credentials. Needs lots of cleanup and tests and support for federated and SAML credentials.
@markpeek
Copy link

We talked a little about this at re:invent so you know I'm very aligned with seeing this go in and glad you made this good start at it.

In a standalone boto command I had put together to allow this same thing but using IPython as a shell, I assumed environment credentials and passed in arguments for the account number and IAM role to construct the ARN. Your idea of adding the [Roles] would be good for accounts which you don't have specific credentials. But you could also add the account number to a profile, pass in an assumerole profile/IAM role and construct the ARN.

And I agree that having multiple sessions at the same time in the same shell would be dangerous and cumbersome to add to the commands. However, having different sessions in separate shells would be useful and might require some thought on how to cache the credentials uniquely (perhaps by parent pid?).

@ptone
Copy link

ptone commented Feb 18, 2014

This opens a can of worms somewhat, but I think the most sane way to do sessions is to have an interactive sub-shell, like you would have for db CLI tools, where you connect to specific DBs. Keeping a session "open" in the context of just a bash shell seems too risky to me.

Does a session auto-expire?

Why not use the role name, and get the ARN programatically - instead of duplicating that in the config file.

I think the console feature is really nice (but for some reason I couldn’t get it to work, even though other aspects of the session and role were working).

I would love to see the part that generates the console login URL factored out into a call in boto directly - for all kinds of other uses.

I also would make the console a command that takes the role/role ARN directly and launches the console without needing to be wrapped in session start/end calls.

@garnaat
Copy link
Contributor Author

garnaat commented Feb 19, 2014

Thanks @markpeek and @ptone for the comments.

I agree that pinning the session to the shell makes sense. I'm not sure how best to do that. The best I can come up with is os.getppid() and then use the returned process group id when creating the path to the cached session credentials but that doesn't work on Windows. Using os.getpid() does work on Windows but doesn't do us any good since it would be the process id of the CLI command not the parent shell.

The problem with having the user specify a role name and then trying to look it up is that you don't know what account in which to look it up. The role itself has been created in another account and the account you are currently using has been granted access to assume the role. But the role isn't owned by my account.

Sessions do expire but the botocore code managing the sessions takes care of that and automatically renews them.

@ptone - I would love to hear more about your console not working.

As for the sub-shell idea, we did discuss this in the early days of the CLI. It obviously didn't happen and it's up to @jamesls to decide now whether it ever will.

@markpeek
Copy link

To your point about not knowing the account to look up. I was implying you do something like this:

[profile dev]
aws_access_key_id=<dev access key>
aws_secret_access_key=<dev secret key>

[profile prod]
aws_access_key_id=<prod access key>
aws_secret_access_key=<prod secret key>
aws_account_id=0123456789

Run it something like this which should allow you to generate the ARN from the account id and role:
aws-cli --profile dev --assume-profile prod --assume-role myiamrole

And in this case you could omit the aws_access_key_id and aws_secret_access_key from the prod profile. But your [Roles] might end up being just as easy and not require as many command line arguments.

jamesls added a commit to jamesls/aws-cli that referenced this pull request Nov 7, 2014
This builds on the work of several existing
pull requests combining what I believe is
the combination of all the suggested changes
including:

* Use role_arn to trigger assume role behavior.
  Do not require an explicit session to be created
  and ended (aws#667).
* Cache the temporary credentials.  If the credentials
  are not expired then we should reuse them instead of
  making the assume role call every time
  (aws#656 and boto/botocore#226).

I think a strong case can be made to move this to botocore.
However, the file caching makes me somewhat hesitant to move
this into botocore (but I think the AssumeRoleProvider without
the caching could be added to botocore).  I'd like to evaluate this in
the future but for now, I don't think it's unreasonable to leave
this in the AWS CLI for the time being.  At any rate, this is
a straightforward and compatible move if we decide to do so.

Also, the ability to open a browser with the currently scoped
session has not been ported over.  I'd like to address that
in a separate pull request.
jamesls added a commit to jamesls/aws-cli that referenced this pull request Nov 8, 2014
This builds on the work of several existing
pull requests combining what I believe is
the combination of all the suggested changes
including:

* Use role_arn to trigger assume role behavior.
  Do not require an explicit session to be created
  and ended (aws#667).
* Cache the temporary credentials.  If the credentials
  are not expired then we should reuse them instead of
  making the assume role call every time
  (aws#656 and boto/botocore#226).

I think a strong case can be made to move this to botocore.
However, the file caching makes me somewhat hesitant to move
this into botocore (but I think the AssumeRoleProvider without
the caching could be added to botocore).  I'd like to evaluate this in
the future but for now, I don't think it's unreasonable to leave
this in the AWS CLI for the time being.  At any rate, this is
a straightforward and compatible move if we decide to do so.

Also, the ability to open a browser with the currently scoped
session has not been ported over.  I'd like to address that
in a separate pull request.
@jamesls
Copy link
Member

jamesls commented Nov 19, 2014

Closing out, superseded by 22932e5

@jamesls jamesls closed this Nov 19, 2014
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