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: Fallback to session-derived credentials #2883

Merged
merged 2 commits into from
Apr 4, 2018

Conversation

twang-rs
Copy link
Contributor

@twang-rs twang-rs commented Jan 5, 2018

#1608 allows for the session to figure out credentials by setting opt.SharedConfigState = session.SharedConfigEnable whenever the Profile is not empty.

This appropriately allows shared config (~/.aws/config) to work correctly (including using assumed roles) iff the profile is set within the Terraform provider configuration.

This does not work correctly in the case of an assumed role when relying on environment variables AWS_PROFILE and AWS_SDK_LOAD_CONFIG.

This patch alters the behavior to the following:

  • We still use the existing method of manually creating a credential chain and attempting to load credentials via a mix of explicit configuration and environment variables found in auth_helpers.go GetCredentials
  • If this fails, we create a default session object and attempt to extract credentials from AWS credential code found in the session. If this is successful, we use that credential.
  • Otherwise, we bomb out with an error message.

I am a little worried that assumed role (or any form of temporary credential) does not seem to have the capability to auto-renew. I'm not sure how much of a problem this is, as terraform plans are (generally) not long-lived processes.

This should effectively resolve #1184.

NOTE: we'll need to bump terraform proper (as in hashicorp/terraform#16661) again, if this PR is accepted/merged.

@jen20
Copy link
Contributor

jen20 commented Jan 6, 2018

Hi @tamsky! Thanks for opening a pull request. I'm going to refer this one to @catsby since it needs more than a quick merge, but I'll offer my 👍 here - I believe it will also resolve the issue with ECS task roles that have been reported in a few different places.

@jen20 jen20 added the bug Addresses a defect in current functionality. label Jan 6, 2018
@mikemoate
Copy link
Contributor

mikemoate commented Jan 10, 2018

I've built terraform (which includes the terraform-provider-aws source directly) and the provider with this change on macOS using https://gist.github.com/mikemoate/99776bee1c0b161f5bc2db207c771727

Testing locally shows that this change breaks the behaviour I fixed in #1608 (which was finally released in terraform 0.11.2), specifically the following configuration no longer works:

// main.tf

terraform {
  backend "s3" {
    bucket  = "some-bucket-name"
    key     = "some-bucket-key/some-state.tfstate"
    profile = "some_profile"
    region  = "eu-west-2"
    acl     = "some-bucket-acl"
  }
}

provider "aws" {
  profile = "some_profile"
  region  = "eu-west-2"
}
// ~/.aws/config

[default]
output = json
region = eu-west-2

[profile some_profile]
role_arn = arn:aws:iam::123456789012:role/some_role
source_profile = default
// ~/.aws/credentials

[default]
aws_access_key_id=SOMEACCESSKEY
aws_secret_access_key=SomeSecretAccessKey

terraform init gives the following error:

$ terraform init
Initializing modules...
- module.app_db

Initializing the backend...
Error loading state: AccessDenied: Access Denied
	status code: 403, request id: 53B359144E5BCDA6, host id: BWnGEXTGRp4+htxEcrhvDsRqvJNXXPT+N1fNqlAxt03apSQUHLWfCIl2Ox7yBVdNwPacKzm0RA4=

I suspect this is related to the fact that opt.SharedConfigState = session.SharedConfigEnable is no longer set with this change, though have not had chance to debug further.

As it stands I (unsurprisingly) would not be in favour of merging this PR.

@twang-rs
Copy link
Contributor Author

@mikemoate: Thanks for the report. I'll take a look this afternoon and update the PR.

@twang-rs
Copy link
Contributor Author

@mikemoate: pushed up a change the restores functionality in #1608. Did some preliminary testing and it seemed good, let me know if satisfies your use case.

@radeksimko radeksimko changed the title fallback to session-derived credentials provider: Fallback to session-derived credentials Jan 16, 2018
@bflad bflad added the provider Pertains to the provider itself, rather than any interaction with AWS. label Jan 28, 2018
@twang-rs
Copy link
Contributor Author

Any more feedback on the PR?

@mikemoate
Copy link
Contributor

@twang-rs I've re-tested our use case (that was originally fixed in #1608) with this latest change, and it now works for us, so we're in favour of the PR. Apologies for the delay in getting to this.

@mikemoate
Copy link
Contributor

mikemoate commented Apr 4, 2018

@catsby, @Ninir or @jen20 any chance we could get this PR reviewed and merged ASAP?

It fixes a valid problem pointed out in #1184 (which is becoming a highly trafficked thread) and improves on my original changes under #1608.

@bflad bflad self-requested a review April 4, 2018 15:51
@bflad
Copy link
Contributor

bflad commented Apr 4, 2018

I am a little worried that assumed role (or any form of temporary credential) does not seem to have the capability to auto-renew. I'm not sure how much of a problem this is, as terraform plans are (generally) not long-lived processes.

You're correct and we've run into this before, especially in large scale environments. Hopefully the addition of max session duration into IAM roles can help mitigate this until we can come up with a proper provider-wide fix. At first blush this pull request seems good so I'll probably pull it in.

@bflad bflad added this to the v1.14.0 milestone Apr 4, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

LGTM -- thanks for this contribution! It should help a bunch of people out. 🚀

@bflad bflad merged commit 2db5ccc into hashicorp:master Apr 4, 2018
bflad added a commit that referenced this pull request Apr 4, 2018
@bflad
Copy link
Contributor

bflad commented Apr 6, 2018

This has been released in version 1.14.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 5, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. provider Pertains to the provider itself, rather than any interaction with AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using terraform with assumed roles defined in shared configuration are ineffective
4 participants