Add support for multiple credential providers via aws-sdk-go

[radeksimko]

The new library already brings in the functionality, so I just took the advantage of it and gave it a higher priority as in the current state goamz was loading everything the same old way and then passing these credentials as static into aws-sdk-go.

I first wanted to write some tests for this, but I'm finding it really hard to mock the calls of remote API in aws-sdk-go as it doesn't have any similar argument for passing mocked http client instance like each API part has.

Either way, I guess we may want to maintain the "backward compatibility" with the way goamz loads credentials, especially because the new library doesn't support following (so far):

• AWS_CREDENTIAL_FILE - we could pass a filename into ProfileCreds and use that one instead DetectCreds if the variable exists
• the same applies to AWS_PROFILE - same solution, profile parameter
• the library does not support older naming convention of variables:
  • AWS_ACCESS_KEY
  • AWS_SECRET_KEY
  • AWS_SECURITY_TOKEN

Both AWS_CREDENTIAL_FILE & AWS_PROFILE should probably be raised as an issue and/or pull request in the aws-sdk-go library as well in the meantime I guess as it's pretty standard variable used by some other tools like AWS CLI.

I can add that compatibility layer into this PR too, I just first want to know what your thoughts are and if you actually want such layer there.

cc @catsby

Related to:
#866 #266 #801

[catsby]

This looks good to me.

the library does not support older naming convention of variables:

I'm not sure this is an issue, since Terraform currently supports both. If a user supplies either of the variable names in the environment, Terraform should pick those up and populate c.AccessKey and c.SecretKey respectively. In this scenario the EnvCreds() func in aws-go won't be called. Unless I'm missing something...

If neither are supplied, it would fall into the ProfileCreds(), which we don't support but would be great to add.

[radeksimko]

since Terraform currently supports both

Yeah, that was the point - both = the deprecated ones too. But I'm ok with only supporting new ones and force people to start using the new convention as long as you guys (as maintainers) are ok with that.

That said, I still think there should be a logic for both AWS_CREDENTIAL_FILE & AWS_PROFILE.

[mitchellh]

I think we should just maintain backwards compatibility with the old ones. We can do a simple if v := os.Getenv("the-deprecated-key"); v != "" { os.SetEnv("the-not-deprecated-key", v) }. I just don't see a reason to drop support and potential hurt folks on the upgrade on this.

But otherwise this LGTM.

[radeksimko]

Rebased latest commits from master + added the compatibility layer as mentioned.

I misread the source code of https://github.com/awslabs/aws-sdk-go/blob/master/aws/auth.go#L90-98 last time, so I didn't notice that both AWS_ACCESS_KEY & AWS_SECRET_KEY are actually supported, so I'm only adding AWS_SECURITY_TOKEN.

[BRMatt]

Hmm, might there be a problem using ec2/elb functionality if credentials are loaded from the auth file? It looks like you could end up in a situation where ec2 is using one set of credentials from the env, and s3 is using a different set loaded from the config file. Maybe getCredentialsProvider() could update Config.AccessKey and Config.SecretKey like AWSAuth() does?

[BRMatt]

I've been playing around with this branch, and I've noticed that the only way to have credentials sourced from the config file is to specify blank versions in the AWS provider block. For example:

~/.aws/credentials:

[default]
aws_access_key_id = XXXXXXX
aws_secret_access_key = YYYY

Using a simple main.tf:

# Specify the provider and access details
provider "aws" {
    region = "us-east-1"
}

resource "aws_instance" "web" {
  instance_type = "m1.small"
  ami = "ami-4c7a3924"
}

When I run terraform apply:

~/terraform ❯❯❯ terraform apply
provider.aws.access_key
  The access key for API operations. You can retrieve this
  from the 'Security & Credentials' section of the AWS console.

  Enter a value: asd

provider.aws.secret_key
  The secret key for API operations. You can retrieve this
  from the 'Security & Credentials' section of the AWS console.

  Enter a value: asd

There are warnings and/or errors related to your configuration. Please
fix these before continuing.

Errors:

  * provider.aws: access_key: required field is not set
  * provider.aws: secret_key: required field is not set

After setting the access_key and secret_key to blank values in main.tf:

# Specify the provider and access details
provider "aws" {
    region = "us-east-1"
    access_key = ""
    secret_key = ""
}

resource "aws_instance" "web" {
  instance_type = "m1.small"
  ami = "ami-4c7a3924"
}

The apply command uses the correct credentials:

~/terraform ❯❯❯ terraform apply                                                                                                                                                   
aws_instance.web: Refreshing state... (ID: i-03f989f9)
aws_instance.web: Creating...
  ami:                 "" => "ami-4c7a3924"
  availability_zone:   "" => "<computed>"
  block_device.#:      "" => "<computed>"
  instance_type:       "" => "m1.small"
  key_name:            "" => "<computed>"
  private_dns:         "" => "<computed>"
  private_ip:          "" => "<computed>"
  public_dns:          "" => "<computed>"
  public_ip:           "" => "<computed>"
  root_block_device.#: "" => "<computed>"
  security_groups.#:   "" => "<computed>"
  subnet_id:           "" => "<computed>"
  tenancy:             "" => "<computed>"
aws_instance.web: Error: 1 error(s) occurred:

* You may only modify the sourceDestCheck attribute for VPC instances (InvalidParameterCombination)
Error applying plan:

1 error(s) occurred:

* 1 error(s) occurred:

* 1 error(s) occurred:

* You may only modify the sourceDestCheck attribute for VPC instances (InvalidParameterCombination)

Terraform does not automatically rollback in the face of errors.
Instead, your Terraform state file has been partially updated with
any resources that successfully completed. Please address the error
above and apply again to incrementally change your infrastructure.

I'm guessing that the sourceDestCheck error, and the fact that setting the access_key/secret_key interactively fails are unrelated to this change and deserve separate tickets. The issue is that because the secret_key/access_key options are marked as required the credentials won't be sourced from the credentials file unless you provide an empty string for these keys. Should this be the expected behaviour?

On the one hand it seems counterintuitive to require these options be set to such a "magic value", but on the other having the provider require an explicit option to use the default credentials could prevent someone from unintentionally running terraform against the wrong account.

[radeksimko]

the only way to have credentials sourced from the config file is to specify blank versions in the AWS provider block

Good point, although that was so far the way to work around this if you wanted credentials to be autoloaded (not really an excuse for that behavior, just an explanation where it comes from).

sourceDestCheck has been already fixed, just need to rebase the bugfix to that branch.

On the one hand it seems counterintuitive to require these options be set to such a "magic value", but on the other having the provider require an explicit option to use the default credentials could prevent someone from unintentionally running terraform against the wrong account.

That is an interesting thought. Generally it's not just about wrong account (that could be solved via #1063) but also about set of credentials with different permissions to the same account.

We could have config option source that could have values like environment_variables or file_config or detect which would make access_key & secret_key optional and make it more obvious that credentials will be loaded magically from elsewhere:

provider "aws" {
   source = "detect"
}

provider "aws" {
  source = "file_config"
  config_path = "~/.aws/credentials"
}

Running TF against wrong account or with different keys is pain, the first can be prevented as mentioned, but the other one? Would you recognise any difference between two keys if you'd see them? I wouldn't.

To make it even more obvious for user, we could be printing to console where did the credentials come from to allow user panicking sooner & doing last-minute Ctrl+C.

[BRMatt]

After playing around with the master branch, it seems that a by-product of this line is that terraform has always supported loading credentials from ~/.aws/credentials if secret_key and access_key are set to empty strings 😕 ...? I'm guessing that might've been unintentional, as the documentation doesn't mention this.

[radeksimko]

it seems that a by-product of this line is that terraform has always supported loading credentials from ~/.aws/credentials if secret_key and access_key are set to empty strings

I think you're right, although the implementation in goamz is a bit poor as it does not support session token.

Generally I agree that there should be probably more systematic approach to this which will make these things more explicit rather than implicit and it should all be documented.

What do you think about the suggested config options above?

[BRMatt]

Apologies, GH didn't load your reply when I was submitting mine!

sourceDestCheck has been already fixed, just need to rebase the bugfix to that branch.

Thanks, I had a look through the issue list but missed that as it's already closed. Glad to see it's already fixed!

that could be solved via #1063

Thanks, that sounds like a much better fix for that particular problem.

but also about set of credentials with different permissions to the same account.

That sounds interesting, I hadn't considered that using the credentials with the wrong permissions could leave you in an inconsistent state. That's definitely something to avoid.

We could have config option source that could have values like environment_variables or file_config or detect which would make access_key & secret_key optional and make it more obvious that credentials will be loaded magically from elsewhere:

That sounds like it could be very helpful, especially for people who're joining an existing terraform project; my only comment would be that I'm not sure if the schema allows for the Required/Optional settings to be changed dynamically. At the moment they're marked as Required, which causes Terraform interactively prompts for the access_key/secret_key if they're not provided, and is very helpful for folks copying/pasting examples from the documentation.

I think you're correct, although the implementation in goamz is a bit poor as it does not support session token.

Hmmm, I'm just re-reading all the code and it seems that goamz causes terraform to support session tokens via the AWS_SECURITY_TOKEN env variable, or is there something off with this implementation? Admittedly I've never used tokens before so I'm not entirely familiar with how this works.

Generally I agree that there should be probably more systematic approach to this which will make these things more explicit rather than implicit and it should all be documented.

Definitely! 👍

[BRMatt]

At the moment they're marked as Required, which causes Terraform interactively prompts for the access_key/secret_key if they're not provided, and is very helpful for folks copying/pasting examples from the documentation.

After thinking about it one solution that springs to mind is to move the detection of credentials from the ~/.aws/credentials file into the schema's DefaultFunc (ala the current system/my PR), and use an environment variable to enable loading of credentials from the credentials file. Admittedly it means you're calling the DetectCreds() method in two places, but it allows terraform to fallback to interactively prompting for the credentials if they're not provided and prevents loading credentials from the file unless it's explicitly enabled. Syntax for the credentials flag could be:

$ TERRAFORM_AWS_CLI_CREDENTIALS=1 terraform apply

Thoughts?

[radeksimko]

to support session tokens via the AWS_SECURITY_TOKEN env variable

I was merely replying to your sentence about config-loading (= it does not support loading token from file). Token is something you send along with access_key + secret_key to the API.

At the end it's all about API requests. Only the tool (TF in this case) cares how you give it the token - either via ENV variable - which was supported in goamz (although via deprecated variable name) or via ~/.aws/credentials => aws_session_token. See http://docs.aws.amazon.com/STS/latest/UsingSTS/Welcome.html for more.

to move the detection of credentials from the ~/.aws/credentials file into the schema's DefaultFunc (ala the current system/my PR), and use an environment variable to enable loading of credentials from the credentials file

I like the idea of using schema's DefaultFunc, but I have a feeling that you're trying to mix two approaches - ENV vars & config file loading. These are two independent things (coincidently and for user fortunately having very similar naming convention) and you pick either of those and use it, not load access_key from file and secret_key from ENV variable nor try to convert variable name into config option name.

Syntax for the credentials flag could be

Will all respect, 👎 on that. I think if the ENV variable can be replaced with provider config option, it should be. So far TF only has two ENV variables (TF_LOG + TF_LOG_PATH) and it would be great to keep it like that.

[BRMatt]

although via deprecated variable name

Gotcha, thanks for explaining that.

I like the idea of using schema's DefaultFunc, but I have a feeling that you're trying to mix two approaches - ENV vars & config file loading. These are two independent things (coincidently and for user fortunately having very similar naming convention) and you pick either of those and use it, not load access_key from file and secret_key from ENV variable nor try to convert variable name into config option name.

I completely agree. I should've been clearer with my explanation, by

calling the DetectCreds() method in two places

I meant calling it in DefaultFunc and Client, though I guess that's not really an issue. It's possible to use DefaultFunc and have both schema options use the same source if you extract the sourcing.

nor try to convert variable name into config option name.

Not quite sure what you mean by this?

Will all respect, 👎 on that. I think if the ENV variable can be replaced with provider config option, it should be.

I agree that it's not pretty, it's just an easy way to preserve interactive prompting of unset variables while allowing people to opt into loading config from a file. If there's an alternative way to do this I'd much prefer that.

[radeksimko]

I wanted to rebase this + try implementing all the suggestions, but I think I'll rather hold off until Clint deals with the last bits of goamz -> aws-sdk-go conversion.

This mid-stage is just creating way too many edge cases that would be hard to work around.
When goamz is gone, it's gonna be much easier.

[radeksimko]

Just thinking about this... it will be probably best to move the whole logic for building credentials provider into aws/provider.go so it's easier to make the input actually optional + do some nice detection there and provide correct credentials back to aws/config.go, where we can configure all the API endpoints (EC2, ELB, R53, ...) using that provider.

[bobtfish]

Ping! This no longer applies to master in a sensible way, and I'd really really like to see this get merged, as multi-account support is pretty essential to our workflow at Yelp.

@radeksimko - do you have time / inclination to get this updated to merge cleanly again, or even better finished off so the interface suits @mitchellh 's demo requirements? I'm happy to try to help out if you're busy!

TIA

[radeksimko]

@bobtfish I'll be happy to update it, but @catsby is working on some further aws refactoring (switching back to AWS' upstream library), so I'd rather do it after he finishes that, so we don't step on each other's toes.

Either way, some help would be appreciated in https://github.com/hashicorp/terraform/issues/1255
We generally need to come up with an idea that will lead to less obstructive and more logical UX, which is AFAIK the only reason why this can't be merged yet (even if there was no merge conflict).

[pmoust]

Let's look at this again and solve any UX issues left.
@radeksimko could you rebase with master? Need to hack at it privately a bit :/

[radeksimko]

Let's look at this again and solve any UX issues left.

First UX issues need to be solved.

Have a look at #1311 where I tried to demonstrate (and fix) the problem.

[mitchellh]

@radeksimko I think we get all this for free now from aws-go, can you verify? There is still #1311 but the core issue here I think is fixed. Let me know if I'm wrong.

[radeksimko]

I don't think it is entirely fixed, but it's just because the original title+purpose of this PR is wrong (too wide).
I will create a new one with more concise description.

TL;DR: My point (and the missing feature) is ability to choose one specific provider (one of them being "detect").

[ghost]

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.