Explicit profile overrides environment variables

[danielgtaylor]

This changes the behavior of environment variable credential loading to
function as described in aws/aws-cli#113. Here is what this looks like
for both the CLI and Botocore/Boto 3:

CLI:

# Use the environment variables
$ AWS_ACCESS_KEY_ID=foo AWS_SECRET_ACCESS_KEY=bar aws s3 ls

# Ignore the environment variables and use a profile from the config instead
$ AWS_ACCESS_KEY_ID=foo AWS_SECRET_ACCESS_KEY=bar aws --profile dev s3 ls

Python:

$ AWS_ACCESS_KEY_ID=foo AWS_SECRET_ACCESS_KEY=bar python
>>> import boto3
>>> boto3.setup_default_session(profile_name='dev')
>>> # The following will use the profile, not the env vars!
>>> s3dev = boto3.resource('s3')

Added a test and a new log message to ensure it's obvious what is happening.

cc @jamesls @kyleknap

[jamesls]

I think this is the wrong level of abstraction and couples profiles to retrieving environment variables when they don't need to be. An env provider should just be a simple object that pulls credentials from environment variables and that's it. If something up the call stack wants to either re-arrange cred providers or ignore what this class returns, that's another thing, but this class should just stay minimal.

It also gives this class weird semantics. If you pass a profile_name to this class, then it won't do anything.

[danielgtaylor]

Would something along these lines be more like what you were thinking?

https://gist.github.com/danielgtaylor/075fcd07b381f1b73e78

[jamesls]

Yeah something along those lines seems better abstracted to me.

[kyleknap]

I agree. That would look better.

[jamesls]

We also don't really clarify what precedence the AWS_DEFAULT_PROFILE env var has. Should that be a higher precedence than the AWS_ACCESS*, AWS_SECRET* env vars? I'm thinking it should, but it currently does not.

EDIT: To clarify, what are we saying is the expected behavior for these cases:

Env vars:
AWS_DEFAULT_PROFILE=foo
AWS_ACCESS_KEY_ID=bar
AWS_SECRET_ACCESS_KEY=qux
Cmd line: <nothing>

If we're consistent with --profile, then we should be looking for the foo profile over pulling from the env creds which I don't believe is the current case with the existing PR.

We'd probably also want to double check that given:

Env vars:
AWS_DEFAULT_PROFILE=foo
AWS_ACCESS_KEY_ID=bar
AWS_SECRET_ACCESS_KEY=qux
Cmd line: --profile other-profile

That the end result is that we pull creds from other-profile (or fail). I believe the current PR handles this last case already.

[coveralls]

Coverage increased (+0.03%) to 94.58% when pulling bb67322b02bc4dd22ef5ccfe7ac87324d9fb2a16 on danielgtaylor:explicit-profile into 2eee0f1 on boto:develop.

[kyleknap]

@jamesls @danielgtaylor

Here is the precedence leveling that I think we should be using:

Location	CLI Representation	Boto3 Representation
Access Keys in Code	NA	client(access_key_id=..., secret_access_key_id=...)
Profile in Code	--profile <profilename>	client(profile_name=...)
Access Keys in Environment Variable	AWS_ACCESS_KEY_ID	AWS_ACCESS_KEY_ID
Profile in Environment Variable	AWS_DEFAULT_PROFILE	AWS_DEFAULT_PROFILE
Shared Credentials File	~/.aws/credentials	~/.aws/credentials
AWS Config File	~/.aws/config	~/.aws/config
Instance Metadata	NA	NA

The way I see it is that precedence should be based off of what is the most explicit. Similarly how access key in code is taking precedence of profile_name in code, access key in environment variable should take precedence over profile name in environment variable because the user is being more explicit and it is consistent with the code parallel of precedence.

I honestly don't think AWS_DEFAULT_PROFILE should affect precedence at all. It should just be switching what profile is default when we look into the shared credentials file and/or the config file. Or that is what I expect it would do given the fact that the name of the environment variable seems to imply changing what the default profile is, which is a little different than how it is described in our docs:

But yet again the docs is a little vague in that it really does not say where its precedence lies when it describes it as "name of the CLI profile to use".

Let me know what you guys think.

[danielgtaylor]

Spoke with @kyleknap this morning about this and I think we both agree. To put this into more concrete examples, if these are set:

Code / arguments:

• aws_access_key_id='abc123' <----- takes precedence
• profile_name='dev'

Environment

• AWS_ACCESS_KEY_ID=abc123 <------ takes precedence
• AWS_DEFAULT_PROFILE=dev

The case we really need to get working is this one (which this PR solves):

• AWS_ACCESS_KEY_ID=abc123
• --profile <------ takes precedence

$ AWS_ACCESS_KEY_ID=abc123 aws --profile dev s3 ls

Thoughts?

[jamesls]

Sorry, just catching up on this now. To summarize, it sounds like we're saying:

For any given precedence level (i.e config file, env vars, cmdline args, etc), the akid/sak will always trump a profile value if all three are specified at the same precedence value.

A consequence of this is that in order to get a "profile" to take, you have to specify it at the next precedence level (or higher) of whatever is explicitly specifying the akid/skid. So if akid/sak are in env vars, profile has to be as a cmd line option or in boto3 via a method arg.

In which case, I think that makes sense. Thanks for looking into this.

I would suggest adding integration tests for these cases as well.

[coveralls]

Coverage increased (+0.02%) to 94.6% when pulling 856eaec on danielgtaylor:explicit-profile into 73b59f1 on boto:develop.

[danielgtaylor]

@jamesls @kyleknap please take another look.

[jamesls]

This is general test case best practice advice: Do not leave global state modified outside the scope of a test. It's ok to mess with global state in a test if there's no better way to test what you need to test, but you have to put the state back to the way it initially was.

Here, if the vars you're deleting were defined in os.environ before the test, this setUp() is deleting these values and not restoring them after the test executes. This can lead to all sorts of hard to track down problems where the test order may affect whether or not a test passes depending on if it passes before/after the global state modification.

That being, said, just use BaseEnvVar to handle the patching of env vars for you.

[jamesls]

I think your integ tests cover a good set of cases, I just have some comments on how the tests themselves are implemented.

Also, there are no unit tests for this. While I think we want the integ tests to ensure we don't regress as we refactor some of the internals, I still think this change should have a unit test. We should be able to call into create_credential_resolver and assert how the cred resolver is being created.

The actual code change itself looks good.

[jamesls]

This seems more like a debug message. We'll still log which provider ultimately gives creds, but the fact that we're disabling a check seems mostly of interest to a dev.

[coveralls]

Coverage increased (+0.02%) to 94.6% when pulling d17c52d on danielgtaylor:explicit-profile into 73b59f1 on boto:develop.

[danielgtaylor]

@jamesls please take another look.

[jamesls]

Or to assert that all the providers are not an instance of the env provider, you can just use: self.assertTrue(all(not isinstance(p, EnvProvider) for p in p.providers))

[jamesls]

Looks good, just had a small suggestion. Thanks for updating.

[coveralls]

Coverage increased (+0.03%) to 94.61% when pulling d0e7add on danielgtaylor:explicit-profile into 73b59f1 on boto:develop.