From cfc5a92f462b9aa2aefd6dcf6f56ec9067cc5eb9 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Tue, 10 Mar 2015 13:17:47 -0700 Subject: [PATCH 1/7] 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 $ AWS_ACCESS_KEY_ID=foo AWS_SECRET_ACCESS_KEY=bar aws s3 ls $ 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. --- botocore/credentials.py | 12 ++++++++++-- tests/unit/test_credentials.py | 12 ++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/botocore/credentials.py b/botocore/credentials.py index 66b08ac86e..e97de789dd 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -47,7 +47,9 @@ def create_credential_resolver(session): metadata_timeout = session.get_config_variable('metadata_service_timeout') num_attempts = session.get_config_variable('metadata_service_num_attempts') providers = [ - EnvProvider(), + # We use ``session.profile`` for EnvProvider rather than + # ``profile_name`` so that it can be ``None`` when unset. + EnvProvider(profile_name=session.profile), SharedCredentialProvider( creds_filename=credential_file, profile_name=profile_name @@ -275,7 +277,7 @@ class EnvProvider(CredentialProvider): # AWS_SESSION_TOKEN is what other AWS SDKs have standardized on. TOKENS = ['AWS_SECURITY_TOKEN', 'AWS_SESSION_TOKEN'] - def __init__(self, environ=None, mapping=None): + def __init__(self, environ=None, mapping=None, profile_name=None): """ :param environ: The environment variables (defaults to @@ -289,6 +291,7 @@ def __init__(self, environ=None, mapping=None): if environ is None: environ = os.environ self.environ = environ + self._profile_name = profile_name self._mapping = self._build_mapping(mapping) def _build_mapping(self, mapping): @@ -314,6 +317,11 @@ def load(self): """ Search for credentials in explicit environment variables. """ + if self._profile_name is not None: + logger.info('Skipping environment variable credential check' + ' because profile name was explicitly set') + return None + if self._mapping['access_key'] in self.environ: logger.info('Found credentials in environment variables.') access_key, secret_key = self._extract_creds_from_mapping( diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 7252f86c60..9c05da9879 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -195,6 +195,18 @@ def test_partial_creds_is_an_error(self): with self.assertRaises(botocore.exceptions.PartialCredentialsError): provider.load() + def test_profile_override_returns_none(self): + # If a profile has been explicitly set, e.g. by something + # like ``session.profile = 'dev'``, then we want to ignore + # the environment variables in favor of loading the correct + # info from the config file based on the profile name. + environ = { + 'AWS_ACCESS_KEY_ID': 'prod', + 'AWS_SECRET_ACCESS_KEY': 'abc123' + } + provider = credentials.EnvProvider(environ, profile_name='dev') + creds = provider.load() + self.assertEqual(creds, None) class TestSharedCredentialsProvider(BaseEnvVar): def setUp(self): From 856eaec5586599b136f770076951428167d822e3 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Mon, 16 Mar 2015 12:54:54 -0700 Subject: [PATCH 2/7] Add integration tests --- botocore/credentials.py | 26 ++++--- tests/integration/test-credentials | 7 ++ tests/integration/test_credentials.py | 106 ++++++++++++++++++++++++++ tests/unit/test_credentials.py | 13 ---- 4 files changed, 128 insertions(+), 24 deletions(-) create mode 100644 tests/integration/test-credentials create mode 100644 tests/integration/test_credentials.py diff --git a/botocore/credentials.py b/botocore/credentials.py index e97de789dd..333544cdb8 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -46,10 +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 = [ - # We use ``session.profile`` for EnvProvider rather than - # ``profile_name`` so that it can be ``None`` when unset. - EnvProvider(profile_name=session.profile), SharedCredentialProvider( creds_filename=credential_file, profile_name=profile_name @@ -65,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.info('Skipping environment variable credential check' + ' because profile name was explicitly set.') + resolver = CredentialResolver(providers=providers) return resolver @@ -277,7 +287,7 @@ class EnvProvider(CredentialProvider): # AWS_SESSION_TOKEN is what other AWS SDKs have standardized on. TOKENS = ['AWS_SECURITY_TOKEN', 'AWS_SESSION_TOKEN'] - def __init__(self, environ=None, mapping=None, profile_name=None): + def __init__(self, environ=None, mapping=None): """ :param environ: The environment variables (defaults to @@ -291,7 +301,6 @@ def __init__(self, environ=None, mapping=None, profile_name=None): if environ is None: environ = os.environ self.environ = environ - self._profile_name = profile_name self._mapping = self._build_mapping(mapping) def _build_mapping(self, mapping): @@ -317,11 +326,6 @@ def load(self): """ Search for credentials in explicit environment variables. """ - if self._profile_name is not None: - logger.info('Skipping environment variable credential check' - ' because profile name was explicitly set') - return None - if self._mapping['access_key'] in self.environ: logger.info('Found credentials in environment variables.') access_key, secret_key = self._extract_creds_from_mapping( @@ -588,7 +592,7 @@ def load_credentials(self): # 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 diff --git a/tests/integration/test-credentials b/tests/integration/test-credentials new file mode 100644 index 0000000000..3bf3ae1ddf --- /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 0000000000..8fdb0fce73 --- /dev/null +++ b/tests/integration/test_credentials.py @@ -0,0 +1,106 @@ +# 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 unittest + + +class TestCredentialPrecedence(unittest.TestCase): + def setUp(self): + # Clean up any existing environment + for name in ['AWS_ACCESS_KEY_ID', + 'AWS_SECRET_ACCESS_KEY', + 'BOTO_DEFAULT_PROFILE']: + if name in os.environ: + del os.environ[name] + + # Set the config file to something that doesn't exist, then + # set the shared credential file to our test config. + os.environ['AWS_CONFIG_FILE'] = '~/.aws/config-missing' + Session.SessionVariables['credentials_file'] = ( + None, None, + os.path.join(os.path.dirname(__file__), 'test-credentials')) + + 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 = 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 = 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 = 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 = 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) + + @mock.patch('botocore.credentials.Credentials') + def test_access_secret_env_vs_profile_code(self, credentials_cls): + # 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 = Session() + s.profile = 'test' + + client = s.create_client('s3') + + credentials_cls.assert_called_with( + 'test', 'test-secret', None, method='shared-credentials-file') diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 9c05da9879..37f72e3389 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -195,19 +195,6 @@ def test_partial_creds_is_an_error(self): with self.assertRaises(botocore.exceptions.PartialCredentialsError): provider.load() - def test_profile_override_returns_none(self): - # If a profile has been explicitly set, e.g. by something - # like ``session.profile = 'dev'``, then we want to ignore - # the environment variables in favor of loading the correct - # info from the config file based on the profile name. - environ = { - 'AWS_ACCESS_KEY_ID': 'prod', - 'AWS_SECRET_ACCESS_KEY': 'abc123' - } - provider = credentials.EnvProvider(environ, profile_name='dev') - creds = provider.load() - self.assertEqual(creds, None) - class TestSharedCredentialsProvider(BaseEnvVar): def setUp(self): super(TestSharedCredentialsProvider, self).setUp() From ae1f175f212aa264c1eba3854b5420fac500aff1 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Tue, 17 Mar 2015 10:47:05 -0700 Subject: [PATCH 3/7] Switch message to debug instead of info --- botocore/credentials.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/botocore/credentials.py b/botocore/credentials.py index 333544cdb8..004ff84da7 100644 --- a/botocore/credentials.py +++ b/botocore/credentials.py @@ -72,8 +72,8 @@ def create_credential_resolver(session): # or credentials. providers.insert(0, EnvProvider()) else: - logger.info('Skipping environment variable credential check' - ' because profile name was explicitly set.') + logger.debug('Skipping environment variable credential check' + ' because profile name was explicitly set.') resolver = CredentialResolver(providers=providers) return resolver From 616f70c3214a3f585943388f079bf92da9c14ce7 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Tue, 17 Mar 2015 10:47:25 -0700 Subject: [PATCH 4/7] Do not modify global state in tests --- tests/integration/test_credentials.py | 44 +++++++++++++++------------ 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/tests/integration/test_credentials.py b/tests/integration/test_credentials.py index 8fdb0fce73..be908fb3b5 100644 --- a/tests/integration/test_credentials.py +++ b/tests/integration/test_credentials.py @@ -15,24 +15,30 @@ import mock from botocore.session import Session -from tests import unittest +from tests import BaseEnvVar -class TestCredentialPrecedence(unittest.TestCase): +class TestCredentialPrecedence(BaseEnvVar): def setUp(self): - # Clean up any existing environment - for name in ['AWS_ACCESS_KEY_ID', - 'AWS_SECRET_ACCESS_KEY', - 'BOTO_DEFAULT_PROFILE']: - if name in os.environ: - del os.environ[name] - - # Set the config file to something that doesn't exist, then - # set the shared credential file to our test config. + 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' - Session.SessionVariables['credentials_file'] = ( - None, None, - os.path.join(os.path.dirname(__file__), 'test-credentials')) + + 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 @@ -41,7 +47,7 @@ def test_access_secret_vs_profile_env(self): os.environ['AWS_SECRET_ACCESS_KEY'] = 'env-secret' os.environ['BOTO_DEFAULT_PROFILE'] = 'test' - s = Session() + s = self.create_session() credentials = s.get_credentials() self.assertEqual(credentials.access_key, 'env') @@ -51,7 +57,7 @@ def test_access_secret_vs_profile_env(self): def test_access_secret_vs_profile_code(self, credentials_cls): # If all three are given, then the access/secret keys should # take precedence. - s = Session() + s = self.create_session() s.profile = 'test' client = s.create_client('s3', aws_access_key_id='code', @@ -64,7 +70,7 @@ 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 = Session() + s = self.create_session() s.profile = 'default' credentials = s.get_credentials() @@ -78,7 +84,7 @@ def test_access_secret_env_vs_code(self, credentials_cls): # code, then those set by code should take precedence. os.environ['AWS_ACCESS_KEY_ID'] = 'env' os.environ['AWS_SECRET_ACCESS_KEY'] = 'secret' - s = Session() + s = self.create_session() client = s.create_client('s3', aws_access_key_id='code', aws_secret_access_key='code-secret') @@ -97,7 +103,7 @@ def test_access_secret_env_vs_profile_code(self, credentials_cls): # os.environ['AWS_ACCESS_KEY_ID'] = 'env' os.environ['AWS_SECRET_ACCESS_KEY'] = 'env-secret' - s = Session() + s = self.create_session() s.profile = 'test' client = s.create_client('s3') From d7cd6d2e3974a6183380e2172f632f3fce0c2d74 Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Tue, 17 Mar 2015 10:58:51 -0700 Subject: [PATCH 5/7] Remove unneeded mock --- tests/integration/test_credentials.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/integration/test_credentials.py b/tests/integration/test_credentials.py index be908fb3b5..f61d04f1f8 100644 --- a/tests/integration/test_credentials.py +++ b/tests/integration/test_credentials.py @@ -92,8 +92,7 @@ def test_access_secret_env_vs_code(self, credentials_cls): credentials_cls.assert_called_with( access_key='code', secret_key='code-secret', token=mock.ANY) - @mock.patch('botocore.credentials.Credentials') - def test_access_secret_env_vs_profile_code(self, credentials_cls): + 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 @@ -106,7 +105,7 @@ def test_access_secret_env_vs_profile_code(self, credentials_cls): s = self.create_session() s.profile = 'test' - client = s.create_client('s3') + credentials = s.get_credentials() - credentials_cls.assert_called_with( - 'test', 'test-secret', None, method='shared-credentials-file') + self.assertEqual(credentials.access_key, 'test') + self.assertEqual(credentials.secret_key, 'test-secret') From d17c52d0bb3617da5abd43f83db0f780a19c77ae Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Tue, 17 Mar 2015 11:28:57 -0700 Subject: [PATCH 6/7] Add unit tests for explicit profile --- tests/unit/test_credentials.py | 40 +++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 37f72e3389..17f1db997b 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -576,9 +576,11 @@ def test_provider_unknown(self): 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', @@ -586,10 +588,38 @@ def test_create_credential_resolver(self): '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) + + for provider in resolver.providers: + self.assertNotIsInstance(provider, credentials.EnvProvider) + + def test_no_profile_checks_env_provider(self): + self.config['profile'] = None + self.session.profile = None + resolver = credentials.create_credential_resolver(self.session) + + found = False + for provider in resolver.providers: + if isinstance(provider, credentials.EnvProvider): + found = True + + self.assertTrue(found) + + 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() From d0e7addf37dbc346e21617c072ebc4753774837b Mon Sep 17 00:00:00 2001 From: "Daniel G. Taylor" Date: Tue, 17 Mar 2015 16:42:34 -0700 Subject: [PATCH 7/7] Switch to any/all checks --- tests/unit/test_credentials.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/tests/unit/test_credentials.py b/tests/unit/test_credentials.py index 17f1db997b..b7cfaa96d8 100644 --- a/tests/unit/test_credentials.py +++ b/tests/unit/test_credentials.py @@ -19,6 +19,7 @@ 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 @@ -598,20 +599,16 @@ def test_explicit_profile_ignores_env_provider(self): self.config['profile'] = 'dev' resolver = credentials.create_credential_resolver(self.session) - for provider in resolver.providers: - self.assertNotIsInstance(provider, credentials.EnvProvider) + 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) - found = False - for provider in resolver.providers: - if isinstance(provider, credentials.EnvProvider): - found = True - - self.assertTrue(found) + self.assertTrue( + any(isinstance(p, EnvProvider) for p in resolver.providers)) def test_no_profile_env_provider_is_first(self): self.config['profile'] = None