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

Added assume role credentials support from profiles #1132

Closed
wants to merge 1 commit into from

Conversation

hguo0303
Copy link

Similar to #1092. But I made a few changes to make it consistent with python and java sdk.

  1. Source_profile is mandatory when assuming role with a role_arn.
  2. Only one level of source profile is allowed in assume role profile provider, it's reading access_key, secret_key and secret_token directly from source_profile's hash. This will avoid a chain of profiles having source_arn referencing another profile.

@hguo0303 hguo0303 force-pushed the AssumeRoleProvider branch 3 times, most recently from 70c7ca1 to 1d5986a Compare March 15, 2016 16:36
end

def load_profile
if profile = profiles[profile_name]
if profile.key?('role_arn')
if source_profile = profile['source_profile']
profile['aws_access_key_id'] = profiles[source_profile]['aws_access_key_id']
Copy link

Choose a reason for hiding this comment

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

But what happens if we will have the defined one from these parameters in non-source profile? Seems they will be re-written from source one. I believe the usage of method "merge" is more correct. But I agree with check the existence of source profile.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I agreed it would be more convenient to have access_key and session_token in the non-source profile. But I made these changes so that it can be uniform with python and java implementations. In botocore, for example, source profile is a mandatory field, and access_key and session_token are only retrieved from source profile. Please refer https://github.com/boto/botocore/blob/develop/botocore/credentials.py#L840-L866 for more information. I believe it is the same idea in java implementation.

@hguo0303
Copy link
Author

hguo0303 commented Apr 1, 2016

@awood45 Could you comment if this PR can be merged or what update is needed to get it merged? Thanks

@awood45
Copy link
Member

awood45 commented Apr 1, 2016

Primarily a matter of discussion on our end about the details of how we can best support this across SDKs (not just Ruby). I'm driving that now, working towards a sustainable and backwards-compatible solution.

@temujin9
Copy link

temujin9 commented Apr 5, 2016

@awood45 FYI, the lack of this is forcing us to write horrid workarounds: https://github.com/bazaarvoice/cloudformation-ruby-dsl/pull/83

Please consider accepting this PR as an interim solution, possibly marked with "alpha code" warnings, if you are unable to reach an internal solution quickly.

@awood45
Copy link
Member

awood45 commented Apr 8, 2016

Alright we're moving forward, here's what I'd like to see before merging (I'll pull this down and do these things next week, but if you'd like to tackle it first, go for it):

  • Unit Tests: Let's make sure that we're testing these code paths with mock config files.
  • Feature Flag: This change is not backwards compatible. Therefore, we're going to require that ENV['AWS_SDK_LOAD_CONFIG'] is set to do any of this. We'll need to add that before we can push this to production.

Additionally, I'm looking at MFA support and a bit more. But we can pull down this PR once we have tests and the ENV flag in place.

@awood45
Copy link
Member

awood45 commented May 6, 2016

Closed in favor of #1178

@awood45 awood45 closed this May 6, 2016
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