Repository: boto/botocore
Pull request #486 (2015-03-10 20:24:59)

Explicit profile overrides environment variables
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:
```bash
# 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:
```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: 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.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/2085023/badge)](https://coveralls.io/builds/2085023)

Coverage increased (+0.03%) to 94.58% when pulling **bb67322b02bc4dd22ef5ccfe7ac87324d9fb2a16 on danielgtaylor:explicit-profile** into **2eee0f1a2a459c2ab285d49975cd2621b0bb5789 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:

![screen shot 2015-03-11 at 10 37 30 am](https://cloud.githubusercontent.com/assets/4605355/6602762/fb679e48-c7da-11e4-93c3-737aeaec7243.png)

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:
* `AWS_ACCESS_KEY_ID=abc123`
* `--profile`                             <------ takes precedence

```bash
$ 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.

Patch:
diff --git a/botocore/credentials.py b/botocore/credentials.py
index 66b08ac86..004ff84da 100644
--- a/botocore/credentials.py
+++ b/botocore/credentials.py
@@ -46,8 +46,8 @@ def create_credential_resolver(session):
     config_file = session.get_config_variable('config_file')
     metadata_timeout = session.get_config_variable('metadata_service_timeout')
     num_attempts = session.get_config_variable('metadata_service_num_attempts')
+
     providers = [
-        EnvProvider(),
         SharedCredentialProvider(
             creds_filename=credential_file,
             profile_name=profile_name
@@ -63,6 +63,18 @@ def create_credential_resolver(session):
                 num_attempts=num_attempts)
         )
     ]
+
+    # We use ``session.profile`` for EnvProvider rather than
+    # ``profile_name`` because it is ``None`` when unset.
+    if session.profile is None:
+        # No profile has been explicitly set, so we prepend the environment
+        # variable provider. That provider, in turn, may set a profile
+        # or credentials.
+        providers.insert(0, EnvProvider())
+    else:
+        logger.debug('Skipping environment variable credential check'
+                     ' because profile name was explicitly set.')
+
     resolver = CredentialResolver(providers=providers)
     return resolver
 
@@ -580,7 +592,7 @@ class CredentialResolver(object):
         # If we got here, no credentials could be found.
         # This feels like it should be an exception, but historically, ``None``
         # is returned.
-        # 
+        #
         # +1
         # -js
         return None


Test patch:
diff --git a/tests/integration/test-credentials b/tests/integration/test-credentials
new file mode 100644
index 000000000..3bf3ae1dd
--- /dev/null
+++ b/tests/integration/test-credentials
@@ -0,0 +1,7 @@
+[default]
+aws_access_key_id = default
+aws_secret_access_key = default-secret
+
+[test]
+aws_access_key_id = test
+aws_secret_access_key = test-secret
diff --git a/tests/integration/test_credentials.py b/tests/integration/test_credentials.py
new file mode 100644
index 000000000..f61d04f1f
--- /dev/null
+++ b/tests/integration/test_credentials.py
@@ -0,0 +1,111 @@
+# Copyright 2015 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License"). You
+# may not use this file except in compliance with the License. A copy of
+# the License is located at
+#
+# http://aws.amazon.com/apache2.0/
+#
+# or in the "license" file accompanying this file. This file is
+# distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
+# ANY KIND, either express or implied. See the License for the specific
+# language governing permissions and limitations under the License.
+
+import os
+import mock
+
+from botocore.session import Session
+from tests import BaseEnvVar
+
+
+class TestCredentialPrecedence(BaseEnvVar):
+    def setUp(self):
+        super(TestCredentialPrecedence, self).setUp()
+
+        # Set the config file to something that doesn't exist so
+        # that we don't accidentally load a config.
+        os.environ['AWS_CONFIG_FILE'] = '~/.aws/config-missing'
+
+    def create_session(self, *args, **kwargs):
+        """
+        Create a new session with the given arguments. Additionally,
+        this method will set the credentials file to the test credentials
+        used by the following test cases.
+        """
+        kwargs['session_vars'] = {
+            'credentials_file': (
+                None, None,
+                os.path.join(os.path.dirname(__file__), 'test-credentials'))
+        }
+
+        return Session(*args, **kwargs)
+
+    def test_access_secret_vs_profile_env(self):
+        # If all three are given, then the access/secret keys should
+        # take precedence.
+        os.environ['AWS_ACCESS_KEY_ID'] = 'env'
+        os.environ['AWS_SECRET_ACCESS_KEY'] = 'env-secret'
+        os.environ['BOTO_DEFAULT_PROFILE'] = 'test'
+
+        s = self.create_session()
+        credentials = s.get_credentials()
+
+        self.assertEqual(credentials.access_key, 'env')
+        self.assertEqual(credentials.secret_key, 'env-secret')
+
+    @mock.patch('botocore.credentials.Credentials')
+    def test_access_secret_vs_profile_code(self, credentials_cls):
+        # If all three are given, then the access/secret keys should
+        # take precedence.
+        s = self.create_session()
+        s.profile = 'test'
+
+        client = s.create_client('s3', aws_access_key_id='code',
+                                 aws_secret_access_key='code-secret')
+
+        credentials_cls.assert_called_with(
+            access_key='code', secret_key='code-secret', token=mock.ANY)
+
+    def test_profile_env_vs_code(self):
+        # If the profile is set both by the env var and by code,
+        # then the one set by code should take precedence.
+        os.environ['BOTO_DEFAULT_PROFILE'] = 'test'
+        s = self.create_session()
+        s.profile = 'default'
+
+        credentials = s.get_credentials()
+
+        self.assertEqual(credentials.access_key, 'default')
+        self.assertEqual(credentials.secret_key, 'default-secret')
+
+    @mock.patch('botocore.credentials.Credentials')
+    def test_access_secret_env_vs_code(self, credentials_cls):
+        # If the access/secret keys are set both as env vars and via
+        # code, then those set by code should take precedence.
+        os.environ['AWS_ACCESS_KEY_ID'] = 'env'
+        os.environ['AWS_SECRET_ACCESS_KEY'] = 'secret'
+        s = self.create_session()
+
+        client = s.create_client('s3', aws_access_key_id='code',
+                                 aws_secret_access_key='code-secret')
+
+        credentials_cls.assert_called_with(
+            access_key='code', secret_key='code-secret', token=mock.ANY)
+
+    def test_access_secret_env_vs_profile_code(self):
+        # If access/secret keys are set in the environment, but then a
+        # specific profile is passed via code, then the access/secret
+        # keys defined in that profile should take precedence over
+        # the environment variables. Example:
+        #
+        # ``aws --profile dev s3 ls``
+        #
+        os.environ['AWS_ACCESS_KEY_ID'] = 'env'
+        os.environ['AWS_SECRET_ACCESS_KEY'] = 'env-secret'
+        s = self.create_session()
+        s.profile = 'test'
+
+        credentials = s.get_credentials()
+
+        self.assertEqual(credentials.access_key, 'test')
+        self.assertEqual(credentials.secret_key, 'test-secret')
diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py
index 7252f86c6..b7cfaa96d 100644
--- a/tests/unit/test_credentials.py
+++ b/tests/unit/test_credentials.py
@@ -19,6 +19,7 @@ import os
 from dateutil.tz import tzlocal
 
 from botocore import credentials
+from botocore.credentials import EnvProvider
 import botocore.exceptions
 import botocore.session
 from tests import unittest, BaseEnvVar
@@ -195,7 +196,6 @@ class TestEnvVar(BaseEnvVar):
         with self.assertRaises(botocore.exceptions.PartialCredentialsError):
             provider.load()
 
-
 class TestSharedCredentialsProvider(BaseEnvVar):
     def setUp(self):
         super(TestSharedCredentialsProvider, self).setUp()
@@ -577,9 +577,11 @@ class CredentialResolverTest(BaseEnvVar):
 
 
 class TestCreateCredentialResolver(BaseEnvVar):
-    def test_create_credential_resolver(self):
-        fake_session = mock.Mock()
-        config = {
+    def setUp(self):
+        super(TestCreateCredentialResolver, self).setUp()
+
+        self.session = mock.Mock()
+        self.config = {
             'credentials_file': 'a',
             'legacy_config_file': 'b',
             'config_file': 'c',
@@ -587,10 +589,34 @@ class TestCreateCredentialResolver(BaseEnvVar):
             'metadata_service_num_attempts': 'e',
             'profile': 'profilename',
         }
-        fake_session.get_config_variable = lambda x: config[x]
-        resolver = credentials.create_credential_resolver(fake_session)
+        self.session.get_config_variable = lambda x: self.config[x]
+
+    def test_create_credential_resolver(self):
+        resolver = credentials.create_credential_resolver(self.session)
         self.assertIsInstance(resolver, credentials.CredentialResolver)
 
+    def test_explicit_profile_ignores_env_provider(self):
+        self.config['profile'] = 'dev'
+        resolver = credentials.create_credential_resolver(self.session)
+
+        self.assertTrue(
+            all(not isinstance(p, EnvProvider) for p in resolver.providers))
+
+    def test_no_profile_checks_env_provider(self):
+        self.config['profile'] = None
+        self.session.profile = None
+        resolver = credentials.create_credential_resolver(self.session)
+
+        self.assertTrue(
+            any(isinstance(p, EnvProvider) for p in resolver.providers))
+
+    def test_no_profile_env_provider_is_first(self):
+        self.config['profile'] = None
+        self.session.profile = None
+        resolver = credentials.create_credential_resolver(self.session)
+
+        self.assertIsInstance(resolver.providers[0], credentials.EnvProvider)
+
 
 if __name__ == "__main__":
     unittest.main()