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

aws/session: Fix SDK AWS_PROFILE and static environment credential behavior #2694

Merged
merged 1 commit into from
Jul 17, 2019

Conversation

jasdel
Copy link
Contributor

@jasdel jasdel commented Jul 15, 2019

Fixes the SDK's behavior when determining the source of credentials to
load. Previously the SDK would ignore the AWS_PROFILE environment, if
static environment credentials were also specified.

If both AWS_PROFILE and static environment credentials are defined, the
SDK will load any credentials from the shared config/credentials file
for the AWS_PROFILE first. Only if there are no credentials defined in
the shared config/credentials file will the SDK use the static
environment credentials instead.

@jasdel jasdel requested a review from skotambkar July 15, 2019 22:23
@jasdel jasdel self-assigned this Jul 16, 2019
@jasdel jasdel added the needs-review This issue or pull request needs review from a core team member. label Jul 16, 2019
@@ -5,3 +5,6 @@
### SDK Enhancements

### SDK Bugs
* `aws/session`: Fix SDK AWS_PROFILE and static environment credential behavior ()
* Fixes the SDK's behavior when determining the source of credentials to load. Previously the SDK would ignore the AWS_PROFILE environment, if static environment credentials were also specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a SDK bug or an Enhancement, as it would be a minor release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug in this case, because it was incorrect, unexpected, behavior in the SDK.

@jasdel jasdel removed the needs-review This issue or pull request needs review from a core team member. label Jul 17, 2019
…havior

Fixes the SDK's behavior when determining the source of credentials to
load. Previously the SDK would ignore the AWS_PROFILE environment, if
static environment credentials were also specified.

If both AWS_PROFILE and static environment credentials are defined, the
SDK will load any credentials from the shared config/credentials file
for the AWS_PROFILE first. Only if there are no credentials defined in
the shared config/credentials file will the SDK use the static
environment credentials instead.
@jasdel jasdel merged commit 262e4f3 into aws:master Jul 17, 2019
@jasdel jasdel deleted the fixup/CfgProfileOrder branch July 17, 2019 23:41
@007
Copy link

007 commented Aug 2, 2019

@jasdel The "wrong" behavior is how the CLI works, and it seems like this change makes the SDK and the CLI have different assumptions on env precedence. Am I misunderstanding something in how credential resolution works?

I have these (bad) keys in my default profile:

$ cat ~/.aws/credentials
[default]
aws_access_key_id = accesskey
aws_secret_access_key = secretkey

Then with no env vars set, it fails as expected:

$ aws sts get-caller-identity

An error occurred (InvalidClientTokenId) when calling the GetCallerIdentity operation: The security token included in the request is invalid.

Then set valid AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY and I get

$ aws sts get-caller-identity
{
    "UserId": "AIDAREDACTEDXREDACTED",
    "Account": "123456789123",
    "Arn": "arn:aws:iam::123456789123:user/rmoore"
}

Then I set export AWS_PROFILE=default and CLI gives:

$ aws sts get-caller-identity
{
    "UserId": "AIDAREDACTEDXREDACTED",
    "Account": "123456789123",
    "Arn": "arn:aws:iam::123456789123:user/rmoore"
}

Would that still happen given this change?

@jasdel
Copy link
Contributor Author

jasdel commented Aug 2, 2019

Hi @007 thanks for reaching out to us. I've verified the difference in behavior you mentioned between the SDK and CLI. It looks like the SDK and CLI differ in behavior with regards to the AWS_PROFILE environment variable vs specifying a profile via the CLI args.

This will use the environment credentials, ignoring the credentials in myProfile.

AWS_ACCESS_KEY_ID=<key> \
AWS_SECRET_ACCESS_KEY=<secret> \
AWS_PROFILE=myProfile \
aws sts get-caller-identity

Specifying the profile as a CLI's input arguments will use the profileToUse profile instead of environment variables first.

AWS_ACCESS_KEY_ID=<key> \
AWS_SECRET_ACCESS_KEY=<secret> \
aws --profile profileToUse sts get-caller-identity

@jasdel
Copy link
Contributor Author

jasdel commented Aug 2, 2019

It looks like the SDK's behavior with regard to the CLI is a little bit more complex than initially seemed. The CLI has an additional parameter to specify the profile separate from the AWS_PROFILE environment variable driving precedence of the shared config profile's credentials. I'm not sure this concept doesn't directly translate to the SDK's well.

I'm investigating how this should best be represented in the SDK. The AWS SDK for Go does allow a profile to be specified in code when creating a Session via the session.NewSessionsWithOptions function and session.Options.Profile. I think this option is the closest analogy to the CLI's --profile option.

sess, err := session.NewSessionWithOptions(session.Options{
    Profile: "myProfileToUse",
})

@jasdel
Copy link
Contributor Author

jasdel commented Aug 2, 2019

I created #2727 to track this bug, and fix.

@007
Copy link

007 commented Aug 3, 2019

I think that behavior is correct, so order of resolution goes:

  • an explicit --profile
  • env keys and/or STS keys (not sure differentiation?)
  • env profile
  • default profile

@ghost
Copy link

ghost commented Aug 25, 2019

Sorry but is the `AWS_DEFAULT_PROFILE´ the proper name of the variable that controls the profiles? Using it a lot with the actual aws cli and boto3

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.

3 participants