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

provider/aws: Enable account ID check for AWS with assumed roles #4290

Closed
wants to merge 3 commits into from

Conversation

ColinHebert
Copy link
Contributor

Currently we're assuming that if there is no current user (iamconn.GetUser(nil) fails), we're in an IAM instance profile therefore the check is superfluous.

This isn't quite true, in the case where we're in an assumed role (cross account assumed role, SAML assumed role, etc.) it's still important to check whether we're applying the changes on the correct AWS account.

Given that we're in an assumed role, it's safe to make the assumption that at least one role will be available (and that the role will be readable).

This PR uses this assumption to check the AWS account ID we're currently on even if we're not logged in through a user.

@radeksimko
Copy link
Member

Related: #3431

Thanks for the PR @ColinHebert

Have a look at suggestions in the linked issue which are mentioning the metadata API. The API is (de facto) always available on EC2 instances.

IAM Policies attached to the role & instance profile may not allow the instance calling iam:ListRoles. In real production environment I personally would not want every instance to be able to list all roles in my AWS account - hence this wouldn't work in many cases where people use strict IAM policies for EC2 instances.

Would you mind changing the PR to use the metadata API?

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Dec 15, 2015
@ColinHebert
Copy link
Contributor Author

@radeksimko The metadata API works with instance profile, but the case I'm working with is role assumed with SAML (see the first message).

Because I'm assuming a role with SAML from my own machine there is no metadata API available for me.

@ColinHebert
Copy link
Contributor Author

Regarding the restrictions on the iam:ListRoles, isn't safe to assume that in most cases the role which runs terraform has this kind of privileges anyway?

What would be possible is to attempt to check the list roles, if it isn't accessible fall back on the default behaviour we have right now (which is no check)

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Dec 16, 2015
@radeksimko
Copy link
Member

the case I'm working with is role assumed with SAML

Ah, my bad, sorry for misreading the initial post, Colin!
This is, obviously, different situation.

I will set up a sample SAML integration and have a look at this.

I'm wondering if we could ask for that specific IAM role which is being assumed. That would be probably safer since we would only need iam:GetRole and only for a specific role ARN (which should do less harm than ListRoles), then we could take the account ID from that ARN.

@ColinHebert
Copy link
Contributor Author

That would be fantastic if we could, unfortunately I went thought the AWS API and this isn't available.

@sedan07
Copy link

sedan07 commented Jan 7, 2016

This would be really useful.

@radeksimko
Copy link
Member

Hi all,
I was having a chat with Amazon about this, also submitted a feature request for an API call that would be useful here (iam:GetCurrentRole or sts:GetCurrectRole).

Amazon obviously won't tell when or if it will ever be implemented, but I wanted to make sure we're not missing anything obvious and that we've done as much as we can to solve it cleanly.

In terms of using SAML assumed roles and AWS credentials created via sts for specific roles, I think this is ok for short term solution, but I'd like to get it sorted along with instance profiles - that may take some time for thorough testing.

Long-term solution may include calling the STS API - so we get the chance to capture the chosen IAM role, but it also means Terraform would have to call IdP specific APIs and prompt the user for credentials to get the SAML response which it would then pass back to STS. Such solution won't be trivial as there is likely many different IdPs with different APIs (or no APIs).

@ColinHebert @sedan07 what solutions do you guys use for managing the credentials with SAML? The only tool I found is https://github.com/electronicarts/awsudo and it somehow parses HTML of the IdP which makes me think it cannot be compatible with many SAML providers (IdPs).

@ColinHebert
Copy link
Contributor Author

@radeksimko I think it really depends on the SAML provider and the authentication method in use. We're using a mix of of SimpleSAMLphp (we're looking into a different provider) with DUOSecurity. We have a custom script (bash/python) that allows us to authenticate from the CLI and go through the various auth factors to obtain the SAML payload.

FWIW, I'm totally on board with AWS implementing a proper way to check the arn of current assumed role (we could use that in other applications as well). But as you said AWS roadmap works in mysterious ways.

@sedan07
Copy link

sedan07 commented Jan 8, 2016

@radeksimko I'm using it with normal IAM users but across multiple accounts. Single identity account then using assumeRole to gain access to the other accounts. It would be really helpful to be able to ensure the role you've assumed is for the account you expect so Terraform doesn't go off and start modifying resources on the wrong account.

@radeksimko
Copy link
Member

FYI: I sent a PR aws/aws-sdk-go#512 to upstream (AWS SDK) which should help us to avoid calling some APIs twice and also to avoid calling iam:ListRoles on EC2 instances.

Thank you for the patience.

@radeksimko
Copy link
Member

aws/aws-sdk-go#512 has been merged which means I will soon revisit this PR + similar issue with IAM Instance Profiles and solve it (hopefully) in a clean way. 😺

@radeksimko
Copy link
Member

@ColinHebert Can you please confirm you're happy with my modifications over in #5030 ?

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Feb 6, 2016
@ColinHebert
Copy link
Contributor Author

Closing in favour of #5030

@ColinHebert ColinHebert closed this Feb 6, 2016
@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Feb 6, 2016
@ghost
Copy link

ghost commented Apr 28, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants