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

Support additional sources for assume role #1313

Merged
merged 11 commits into from Nov 21, 2017

Conversation

Projects
None yet
7 participants
@JordonPhillips
Contributor

JordonPhillips commented Nov 11, 2017

This adds support for credential sources aside from profiles with
hard-coded credentials. Now customers will be able to point to
another assume role profile with source_profile. Additionally,
they will be able to specify a new credential_source parameter
to load source credentials from other credential providers. This
currently has three valid values:

  • Environment - This loads source credentials from environment
    variables.
  • Ec2InstanceMetadata - This loads source credentials from EC2's
    instance metadata service.
  • EcsContainer - This loads source credentials from ECS's
    container credentials.

Resolves aws/aws-cli#2938
Resolves aws/aws-cli#2590
Resolves aws/aws-cli#1390
Closes #946
Closes #851

Addresses some, but not all, of #811

@codecov-io

This comment has been minimized.

codecov-io commented Nov 11, 2017

Codecov Report

Merging #1313 into develop will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #1313      +/-   ##
==========================================
+ Coverage    98.06%   98.1%   +0.03%     
==========================================
  Files           46      46              
  Lines         7460    7608     +148     
==========================================
+ Hits          7316    7464     +148     
  Misses         144     144
Impacted Files Coverage Δ
botocore/credentials.py 98.22% <100%> (+0.49%) ⬆️
botocore/exceptions.py 100% <100%> (ø) ⬆️

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 01b5f93...065b451. Read the comment docs.

@JordonPhillips JordonPhillips force-pushed the JordonPhillips:assume branch Nov 13, 2017

@kyleknap

Looks good. 🚢 Did some testing on it as well. Things that I tried using the CLI included:

  • Recursively assuming a role. Used two roles for my case.
  • Assuming a role from credentials set in the environment.
  • Assuming a role from EC2 instance metadata.
  • Have a profile reference itself with source_profile and provide access and secret keys.

Also did the following to try to break it and all of them had an appropriate error message:

  • Set a non-supported credential_source value
  • Created a circular reference with source_profile
  • Provided a source_profile that does not exist
  • Set a credential_source that did not load data
  • Did not set credential_source or source_profile but specified a role_arn
  • Have a profile reference itself with source_profile and not provide access and secret keys.
botocore/credentials.py Outdated
@@ -814,6 +991,10 @@ def __init__(self, load_config, client_creator, cache, profile_name,
:param prompter: A callable that returns input provided
by the user (i.e raw_input, getpass.getpass, etc.).
:type credential_sourcer: CredentialSource

This comment has been minimized.

@kyleknap

kyleknap Nov 13, 2017

Member

Probably want to update the type here to: CanonicalNameCredentialSourcer

This comment has been minimized.

@JordonPhillips

JordonPhillips Nov 14, 2017

Contributor

Updated

@JordonPhillips

This comment has been minimized.

Contributor

JordonPhillips commented Nov 14, 2017

Cool. I'm gonna leave this up for a couple of days to give people a chance to try it out / leave feedback.

@jamesls

Overall looks good, just had a few minor comments.

botocore/credentials.py Outdated
"""
# On windows, ':' is not allowed in file names, so we'll
# replace them with '_' instead.
cache_key = self._role_arn.replace(':', '_').replace(os.path.sep, '_')

This comment has been minimized.

@jamesls

jamesls Nov 15, 2017

Member

Interesting. The cache key will no longer have the profile name as part of its cache key which means you will share cached credentials across different profiles if they happen to assume the same role. e.g

[profile one]
role_arn=MYROLE
[profile two]
role_arn=MYROLE

This isn't a safe assumption because different profiles could have different arguments but still assume the same role (external_id, mfa_serial, etc).

This comment has been minimized.

@JordonPhillips

JordonPhillips Nov 16, 2017

Contributor

That's a good point. How about doing a hash of all of self._assume_role_kwargs? It would also serve to make the generation much simpler.

This will check the cache for up-to-date credentials, calling assume
role if none are available.
"""
response = self._load_from_cache()

This comment has been minimized.

@jamesls

jamesls Nov 15, 2017

Member

The original version had debug logs about whether or not we load from cache that don't appear to be ported over with this change (line 856 in the original file).

It would be helpful to log when we load from the cache and when we load from cache but they're expired.

This comment has been minimized.

@JordonPhillips
credential_source, parent_profile)
))
if not self._credential_sourcer.is_supported(credential_source):
raise InvalidConfigError(error_msg=(

This comment has been minimized.

@jamesls

jamesls Nov 15, 2017

Member

It would be helpful if we can also surface the list of canonical names with the _credential_sourcer knows what they are. I ran into this my mistakenly using EC2InstanceMetadata instead of Ec2InstanceMetadata

return seconds < self.EXPIRY_WINDOW_SECONDS
def _load_creds_via_assume_role(self, profile_name):
role_config = self._get_role_config(profile_name)
source_credentials = self._resolve_source_credentials(

This comment has been minimized.

@jamesls

jamesls Nov 15, 2017

Member

This resolution is now happening before the cache checks. This results in an unnecessary requirement that the entire chain of assumed profiles is not expired.

However, these intermediate results aren't actually used if a subsequent role is cached. For example, consider the role chain A->B->C. Let's say C and B are expired in the cache, but A's entry in ~/.aws/cli/cache is up to date. This means the sequence of calls you'll make is:

  • AssumeRole(C)
  • AssumeRole(B)
  • Used cache credentials from A

It seems like the ideal behavior would be if we're trying to assume the role in profile A, as long as it's cache entry is up to date we should just use it.

You can test this behavior by just deleting the cache file for B and C in ~/.aws/cli/cache

This comment has been minimized.

@JordonPhillips

JordonPhillips Nov 16, 2017

Contributor

Looks like this crept in when moving from assume role credentials to the fetcher since I didn't have a test for it. The problem is that RefreshableCredentials needs to have initial credentials.

@kyleknap

Changes look fine just marking down some of the comments that we had offline.

botocore/credentials.py Outdated
args = json.dumps(args, sort_keys=True)
cache_key = sha256(args.encode('utf-8')).hexdigest()

This comment has been minimized.

@kyleknap

kyleknap Nov 18, 2017

Member

We talked about this offline but did we want to include the role_arn in the key name as well so it is easier to find the cached credentials if it is stored on disk?

botocore/credentials.py Outdated
secret_key=base_credentials['secret_key'],
token=base_credentials['token'],
expiry_time=expiration,
access_key='',

This comment has been minimized.

@kyleknap

kyleknap Nov 18, 2017

Member

Taked to you offline about this as well. But I would prefer we have a explicit value as the access and secret key (i.e. NOT_SET, NEEDS_REFRESH) to indicate on the first load of these we need to do the refresh logic.

@kyleknap

🚢 Looks good to me.

jstewmon and others added some commits Feb 26, 2016

Assign each credentail provider a canonical name
Credential providers need names that are shared between SDKs in order
to support features across SDKs. This assigns those names to each of
the existing credential providers and provides a way to use those names
to get them from a CredentialResolver.
Add assume role credentials
This adds an AssumeRoleCredentials object that is able to manage
a set of assumed credentials. It is designed to be as open as
possible so that customers can freely use it for their own needs.
Create new assume role provider
This creates a new AssumeRoleProvider which supports loading source
credentials from other providers as well as recursive role
assumption.

This also updates the AssumeRoleProvider tests to account for the
changing of two internal behaviors:

1. The first call to AssumeRole is now delayed until the credentials
   are actually used instead of when they are initially loaded.
2. The format of the cache key has changed.
Clean up Assume Role
This removes most of the changes introduced in the previous commit.
Many of those changes will be re-introduced in a very different form,
but the previous commit is left in to give the author much deserved
credit.
Add load to CredentialResolver
This adds a ``load`` method to the CredentialResolver, allwing it to
be treated like a CredentialProvider.
Add support for credential sources
This adds in support for the Environment, EcsContainer, and
Ec2InstanceMetadta credential sources.

While previous commits had added support for adding credential sources,
this actually configures the default provider chain to accept them.

@JordonPhillips JordonPhillips force-pushed the JordonPhillips:assume branch Nov 21, 2017

@kyleknap

Looks good again.

self._time_fetcher = time_fetcher
self._refresh_lock = threading.Lock()
self.method = method
self._frozen_credentials = None

This comment has been minimized.

@kyleknap

kyleknap Nov 21, 2017

Member

Just thinking out loud here. I understand why you didn't just use super here because you do not want _normalize() to be called. I'm a little concerned that these internal variables may grow out of sync with the RefreshableCredentials, but given that we hardly touch this code and we now have a lot of test, I'm not too concerned about it.

@jamesls

Awesome!

JordonPhillips added some commits Aug 22, 2017

Refactor assume role credentials
This removes the AssumeRoleCredentials object, using most of its
functionality to implement a AssumeRoleCredentialsRefresher object,
which provides a refresh function for RefreshableCredentials.

It further adds a CrendentialSourcer concept to abstract out the
behavior of resolving source credentials.
Introduce DeferredRefreshableCredentials
This introduces a new credentials class that subclasses from
RefreshableCredentials, which takes no initial credentials and
instead calls the refresh function on initial access.

@JordonPhillips JordonPhillips force-pushed the JordonPhillips:assume branch to 065b451 Nov 21, 2017

@JordonPhillips JordonPhillips merged commit e32e818 into boto:develop Nov 21, 2017

3 checks passed

codecov/patch 100% of diff hit (target 98.06%)
Details
codecov/project 98.1% (+0.03%) compared to 01b5f93
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@surkls

This comment has been minimized.

surkls commented Nov 29, 2017

Hi guys, I'm using this new feature so far with great success, thanks! This made it so much easier than doing a cli assume-role call. I do have a question though, I'm trying to add the assumed role as a principle in a bucket policy. Using this new feature, how do I know what the role-session-name is?

https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html

Edit: After reading through the code, I see now that it will read the value of role_session_name from the .aws/config file if provided, tested it out and works as expected!

@sarathkumar21

This comment has been minimized.

sarathkumar21 commented Jan 10, 2018

so now i can try without using access and secret key

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment