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

providers/aws: detect credentials more robustly #1841

Merged
merged 1 commit into from
Jun 29, 2015

Conversation

josharian
Copy link
Contributor

aws hides its credentials in many places:
multiple env vars, config files,
ec2 metadata.

Terraform currently recognizes only the env vars;
to use the other options, you had to put in a
dummy empty value for access_key and secret_key.

Rather than duplicate all aws checks, ask the
aws sdk to fetch credentials earlier.


Please review skeptically. For example, this could cause network access inside Provider, which could block.

aws hides its credentials in many places:
multiple env vars, config files,
ec2 metadata.

Terraform currently recognizes only the env vars;
to use the other options, you had to put in a
dummy empty value for access_key and secret_key.

Rather than duplicate all aws checks, ask the
aws sdk to fetch credentials earlier.
@mitchellh
Copy link
Contributor

Ah, interesting! I think we're going to ship 0.5 the way it is, which is likely the same behavior as it was before. But lets discuss how to fix this in the future.

I agree with you that I'm skeptical because this will cause network access on provder init which is kind of odd...

@josharian
Copy link
Contributor Author

The other obvious possibility is to mark access_key and secret_key optional instead of required. Failures occur later than they otherwise would, but it leaves everything else unimpacted.

@josharian
Copy link
Contributor Author

Sounds fine to ship 0.5 as-is. This is just an annoyance. I'm game to fix however you see fit.

@apparentlymart
Copy link
Contributor

For me the most useful addition here would be to make the AWS_PROFILE environment variable work, as described here: http://blogs.aws.amazon.com/security/post/Tx3D6U6WSFGOK2H/A-New-and-Standardized-Way-to-Manage-Credentials-in-the-AWS-SDKs

I have a few different AWS accounts and with the AWS command line tools I use the AWS_PROFILE mechanism to easily switch between them.

Running Terraform on EC2 instances with their local IAM roles is less interesting to me, but I could see it being useful to some. I enabled support for that in the S3 remote state backend because I just used the default stack of credential providers in the client, but it does have the quirk of failing with an error message about reaching the EC2 metadata API if you happen to run it not on an EC2 instance and without credentials specified others, which is a little user-hostile.

Perhaps using the metadata API for credentials could be an option in the provider config, with the rest just applying automatically since they don't hit the network?

@josharian
Copy link
Contributor Author

Running Terraform on EC2 instances with their local IAM roles is less interesting to me

This is exactly the case that originally motivated this case. :)

Perhaps using the metadata API for credentials could be an option in the provider config, with the rest just applying automatically since they don't hit the network?

Sounds fine, although it might be easier to just make access_key and secret_key optional. Then everything just works, which is better than having to add, document, and find more options.

@apparentlymart
Copy link
Contributor

access_key and secret_key are already optional when the environment variables are set, right?

So your patch extends that to them being optional if the env vars are set, if a credentials file is present with a default profile in it, or if you happen to be running on an EC2 instance.

All of those extensions seem like no-brainers except the machine IAM crededentials, because that should only be done if Terraform is running on an EC2 instance, and yet it seems the only way to detect automatically that is to do a network request to a link-local IP address that might end up referring to some other random host when not within an EC2 network.

Perhaps a compromise: an environment variable that tells Terraform to try to fetch instance IAM credentials. That way it's possible to write a generic Terraform config that doesn't make any presumption about how the credentials are obtained, and just make sure the right environment variables are set when you run Terraform to tell it what you want to do.

@josharian
Copy link
Contributor Author

do a network request to a link-local IP address that might end up referring to some other random host when not within an EC2 network.

Sadly and frustratingly, yes. However, terraform does this already. The only change in this PR is when that happens. Maybe that's an issue worth discussing, but if so, it's a distinct one.

Perhaps a compromise: an environment variable that tells Terraform to try to fetch instance IAM credentials.

I see the appeal of this, but it seems like additional complexity compared to just making access_key and secret_key optional.

@mitchellh
Copy link
Contributor

This is looking pretty great @josharian. I'm going to make some stylistic changes but I like the look of this.

@josharian
Copy link
Contributor Author

Great. Let me know if I can help.

mitchellh added a commit that referenced this pull request Jun 29, 2015
providers/aws: detect credentials more robustly
@mitchellh mitchellh merged commit 2a5ed6c into hashicorp:master Jun 29, 2015
@ghost
Copy link

ghost commented May 1, 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 May 1, 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.

4 participants