From 08db225141f3b794454570306879a91b1828b59a Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Thu, 18 Sep 2014 20:41:55 -0700 Subject: [PATCH 1/4] Add test coverage to credentials file Cleaned up the code a bit before making feature changes. --- awscli/compat.py | 4 ++ awscli/customizations/configure.py | 32 ++++----- tests/unit/customizations/test_configure.py | 76 +++++++++++++++++---- 3 files changed, 79 insertions(+), 33 deletions(-) diff --git a/awscli/compat.py b/awscli/compat.py index 5951a75e4aa2..2039aa282118 100644 --- a/awscli/compat.py +++ b/awscli/compat.py @@ -17,6 +17,8 @@ import locale import urllib.parse as urlparse + raw_input = input + def get_stdout_text_writer(): return sys.stdout @@ -41,6 +43,8 @@ def compat_open(filename, mode='r', encoding=None): import io import urlparse + raw_input = raw_input + def get_stdout_text_writer(): # In python3, all the sys.stdout/sys.stderr streams are in text # mode. This means they expect unicode, and will encode the diff --git a/awscli/customizations/configure.py b/awscli/customizations/configure.py index 3467604fadb7..d87256137d6c 100644 --- a/awscli/customizations/configure.py +++ b/awscli/customizations/configure.py @@ -18,12 +18,7 @@ from botocore.exceptions import ProfileNotFound from awscli.customizations.commands import BasicCommand - - -try: - raw_input = raw_input -except NameError: - raw_input = input +from awscli.compat import raw_input logger = logging.getLogger(__name__) @@ -55,7 +50,7 @@ def _mask_value(current_value): if current_value is None: return 'None' else: - return ('*' * 16) + current_value[-4:] + return ('*' * 16) + current_value[-4:] class InteractivePrompter(object): @@ -98,11 +93,11 @@ def update_config(self, new_values, config_filename): def _create_file(self, config_filename): # Create the file as well as the parent dir if needed. - dirname, basename = os.path.split(config_filename) + dirname = os.path.split(config_filename)[0] if not os.path.isdir(dirname): os.makedirs(dirname) with os.fdopen(os.open(config_filename, - os.O_WRONLY|os.O_CREAT, 0o600), 'w'): + os.O_WRONLY | os.O_CREAT, 0o600), 'w'): pass def _write_new_section(self, section_name, new_values, config_filename): @@ -124,15 +119,15 @@ def _find_section_start(self, contents, section_name): if match is not None and self._matches_section(match, section_name): return i - else: - raise SectionNotFoundError(section_name) + raise SectionNotFoundError(section_name) def _update_section_contents(self, contents, section_name, new_values): # First, find the line where the section_name is defined. # This will be the value of i. new_values = new_values.copy() # ``contents`` is a list of file line contents. - section_start_line_num = self._find_section_start(contents, section_name) + section_start_line_num = self._find_section_start(contents, + section_name) # If we get here, then we've found the section. We now need # to figure out if we're updating a value or adding a new value. # There's 2 cases. Either we're setting a normal scalar value @@ -182,7 +177,8 @@ def _update_subattributes(self, index, contents, values, starting_indent): line = contents[i] match = self.OPTION_REGEX.search(line) if match is not None: - current_indent = len(match.group(1)) - len(match.group(1).lstrip()) + current_indent = len( + match.group(1)) - len(match.group(1).lstrip()) key_name = match.group(1).strip() if key_name in values: option_value = values[key_name] @@ -205,7 +201,8 @@ def _insert_new_values(self, line_number, contents, new_values, indent=''): subindent = indent + ' ' new_contents.append('%s%s =\n' % (indent, key)) for subkey, subval in list(value.items()): - new_contents.append('%s%s = %s\n' % (subindent, subkey, subval)) + new_contents.append('%s%s = %s\n' % (subindent, subkey, + subval)) else: new_contents.append('%s%s = %s\n' % (indent, key, value)) del new_values[key] @@ -302,9 +299,9 @@ def _lookup_credentials(self): # the credentials.method is sufficient to show # where the credentials are coming from. access_key = ConfigValue(credentials.access_key, - credentials.method, '') + credentials.method, '') secret_key = ConfigValue(credentials.secret_key, - credentials.method, '') + credentials.method, '') access_key.mask_value() secret_key.mask_value() return access_key, secret_key @@ -322,6 +319,7 @@ def _lookup_config(self, name): else: return ConfigValue(NOT_SET, None, None) + class ConfigureSetCommand(BasicCommand): NAME = 'set' DESCRIPTION = BasicCommand.FROM_FILE('configure', 'set', @@ -362,7 +360,6 @@ def _run_main(self, args, parsed_globals): section = 'profile %s' % self._session.profile else: # First figure out if it's been scoped to a profile. - # This will happen if parts = varname.split('.') if parts[0] in ('default', 'profile'): # Then we know we're scoped to a profile. @@ -456,7 +453,6 @@ def _get_dotted_config_value(self, varname): return value - class ConfigureCommand(BasicCommand): NAME = 'configure' DESCRIPTION = BasicCommand.FROM_FILE() diff --git a/tests/unit/customizations/test_configure.py b/tests/unit/customizations/test_configure.py index 70241cba8fe0..b70276b878ea 100644 --- a/tests/unit/customizations/test_configure.py +++ b/tests/unit/customizations/test_configure.py @@ -182,9 +182,15 @@ def test_session_says_profile_does_not_exist(self): class TestInteractivePrompter(unittest.TestCase): - @mock.patch('awscli.customizations.configure.raw_input') - def test_access_key_is_masked(self, mock_raw_input): - mock_raw_input.return_value = 'foo' + def setUp(self): + self.patch = mock.patch('awscli.customizations.configure.raw_input') + self.mock_raw_input = self.patch.start() + + def tearDown(self): + self.patch.stop() + + def test_access_key_is_masked(self): + self.mock_raw_input.return_value = 'foo' prompter = configure.InteractivePrompter() response = prompter.get_value( current_value='myaccesskey', config_name='aws_access_key_id', @@ -192,45 +198,54 @@ def test_access_key_is_masked(self, mock_raw_input): # First we should return the value from raw_input. self.assertEqual(response, 'foo') # We should also not display the entire access key. - prompt_text = mock_raw_input.call_args[0][0] + prompt_text = self.mock_raw_input.call_args[0][0] self.assertNotIn('myaccesskey', prompt_text) self.assertRegexpMatches(prompt_text, r'\[\*\*\*\*.*\]') - @mock.patch('awscli.customizations.configure.raw_input') - def test_access_key_not_masked_when_none(self, mock_raw_input): - mock_raw_input.return_value = 'foo' + def test_access_key_not_masked_when_none(self): + self.mock_raw_input.return_value = 'foo' prompter = configure.InteractivePrompter() response = prompter.get_value( current_value=None, config_name='aws_access_key_id', prompt_text='Access key') # First we should return the value from raw_input. self.assertEqual(response, 'foo') - prompt_text = mock_raw_input.call_args[0][0] + prompt_text = self.mock_raw_input.call_args[0][0] self.assertIn('[None]', prompt_text) - @mock.patch('awscli.customizations.configure.raw_input') - def test_secret_key_is_masked(self, mock_raw_input): + def test_secret_key_is_masked(self): prompter = configure.InteractivePrompter() prompter.get_value( current_value='mysupersecretkey', config_name='aws_secret_access_key', prompt_text='Secret Key') # We should also not display the entire secret key. - prompt_text = mock_raw_input.call_args[0][0] + prompt_text = self.mock_raw_input.call_args[0][0] self.assertNotIn('mysupersecretkey', prompt_text) self.assertRegexpMatches(prompt_text, r'\[\*\*\*\*.*\]') - @mock.patch('awscli.customizations.configure.raw_input') - def test_non_secret_keys_are_not_masked(self, mock_raw_input): + def test_non_secret_keys_are_not_masked(self): prompter = configure.InteractivePrompter() prompter.get_value( current_value='mycurrentvalue', config_name='not_a_secret_key', prompt_text='Enter value') # We should also not display the entire secret key. - prompt_text = mock_raw_input.call_args[0][0] + prompt_text = self.mock_raw_input.call_args[0][0] self.assertIn('mycurrentvalue', prompt_text) self.assertRegexpMatches(prompt_text, r'\[mycurrentvalue\]') + def test_user_hits_enter_returns_none(self): + # If a user hits enter, then raw_input returns the empty string. + self.mock_raw_input.return_value = '' + + prompter = configure.InteractivePrompter() + response = prompter.get_value( + current_value=None, config_name='aws_access_key_id', + prompt_text='Access key') + # We convert the empty string to None to indicate that there + # was no input. + self.assertIsNone(response) + class TestConfigFileWriter(unittest.TestCase): def setUp(self): @@ -395,6 +410,22 @@ def test_update_config_with_comments(self): 'foo = newvalue\n' ) + def test_update_config_with_commented_section(self): + original = ( + '#[default]\n' + '[default]\n' + '#foo = 1\n' + 'bar = 1\n' + ) + self.assert_update_config( + original, {'foo': 'newvalue'}, + '#[default]\n' + '[default]\n' + '#foo = 1\n' + 'bar = 1\n' + 'foo = newvalue\n' + ) + def test_spaces_around_key_names(self): original = ( '[default]\n' @@ -720,7 +751,7 @@ def test_configure_set_triple_dotted(self): {'__section__': 'default', 's3': {'signature_version': 's3v4'}}, 'myconfigfile') - def test_configure_set_with_profile(self): + def test_configure_set_with_profile_nested(self): # aws configure set default.s3.signature_version s3v4 set_command = configure.ConfigureSetCommand(self.session, self.config_writer) set_command(args=['profile.foo.s3.signature_version', 's3v4'], @@ -728,3 +759,18 @@ def test_configure_set_with_profile(self): self.config_writer.update_config.assert_called_with( {'__section__': 'profile foo', 's3': {'signature_version': 's3v4'}}, 'myconfigfile') + + +class TestConfigValueMasking(unittest.TestCase): + def test_config_value_is_masked(self): + config_value = configure.ConfigValue( + 'fake_access_key', 'config_file', 'aws_access_key_id') + self.assertEqual(config_value.value, 'fake_access_key') + config_value.mask_value() + self.assertEqual(config_value.value, '****************_key') + + def test_dont_mask_unset_value(self): + no_config = configure.ConfigValue(configure.NOT_SET, None, None) + self.assertEqual(no_config.value, configure.NOT_SET) + no_config.mask_value() + self.assertEqual(no_config.value, configure.NOT_SET) From af9ed8027a326d9dea5a5bccc1a1bf9139a59dc3 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Thu, 18 Sep 2014 22:18:37 -0700 Subject: [PATCH 2/4] The configure command writes out cred vars to shared credentials file Fixes #847. The change is implemented as specified in the issue: * Anytime you set credential variables (access_key/secret_key/session_token) using the `configure` command or the `configure set` command, the values are always written to `~/.aws/credentials`. * Getting access_key/secret_key/session_token will look in the shared credentials file first before looking in the CLI config file. This does *not* do anything with automatically migrating over to the shared credentials file, that is, if you have credentials in the CLI config file and you run `aws configure` and hit enter 4 times, we will not write out values to the shared credentials file because there are no new values to write out. --- awscli/customizations/configure.py | 57 +++++++++++++++++ awscli/examples/configure/_description.rst | 5 ++ .../examples/configure/set/_description.rst | 5 ++ tests/unit/customizations/test_configure.py | 63 ++++++++++++++++--- 4 files changed, 123 insertions(+), 7 deletions(-) diff --git a/awscli/customizations/configure.py b/awscli/customizations/configure.py index d87256137d6c..0359a3b44416 100644 --- a/awscli/customizations/configure.py +++ b/awscli/customizations/configure.py @@ -75,6 +75,32 @@ class ConfigFileWriter(object): ) def update_config(self, new_values, config_filename): + """Update config file with new values. + + This method will update a section in a config file with + new key value pairs. + + This method provides a few conveniences: + + * If the ``config_filename`` does not exist, it will + be created. Any parent directories will also be created + if necessary. + * If the section to update does not exist, it will be created. + * Any existing lines that specified by ``new_values`` + **will not be touched**. This ensures that commented out + values are left unaltered. + + :type new_values: dict + :param new_values: The values to update. There is a special + key ``__section__``, that specifies what section in the INI + file to update. If this key is not present, then the + ``default`` section will be updated with the new values. + + :type config_filename: str + :param config_filename: The config filename where values will be + written. + + """ section_name = new_values.pop('__section__', 'default') if not os.path.isfile(config_filename): self._create_file(config_filename) @@ -336,6 +362,8 @@ class ConfigureSetCommand(BasicCommand): 'action': 'store', 'cli_type_name': 'string', 'positional_arg': True}, ] + _WRITE_TO_CREDS_FILE = ['aws_access_key_id', 'aws_secret_access_key', + 'aws_session_token'] def __init__(self, session, config_writer=None): super(ConfigureSetCommand, self).__init__(session) @@ -380,6 +408,12 @@ def _run_main(self, args, parsed_globals): config_filename = os.path.expanduser( self._session.get_config_variable('config_file')) updated_config = {'__section__': section, varname: value} + if varname in self._WRITE_TO_CREDS_FILE: + config_filename = os.path.expanduser( + self._session.get_config_variable('credentials_file')) + section_name = updated_config['__section__'] + if section_name.startswith('profile '): + updated_config['__section__'] = section_name[8:] self._config_writer.update_config(updated_config, config_filename) @@ -518,7 +552,30 @@ def _run_main(self, parsed_args, parsed_globals): config_filename = os.path.expanduser( self._session.get_config_variable('config_file')) if new_values: + self._write_out_creds_file_values(new_values, + parsed_globals.profile) if parsed_globals.profile is not None: new_values['__section__'] = ( 'profile %s' % parsed_globals.profile) self._config_writer.update_config(new_values, config_filename) + + def _write_out_creds_file_values(self, new_values, profile_name): + # The access_key/secret_key are now *always* written to the shared + # credentials file (~/.aws/credentials), see aws/aws-cli#847. + # post-conditions: ~/.aws/credentials will have the updated credential + # file values and new_values will have the cred vars removed. + credential_file_values = {} + if 'aws_access_key_id' in new_values: + credential_file_values['aws_access_key_id'] = new_values.pop( + 'aws_access_key_id') + if 'aws_secret_access_key' in new_values: + credential_file_values['aws_secret_access_key'] = new_values.pop( + 'aws_secret_access_key') + if credential_file_values: + if profile_name is not None: + credential_file_values['__section__'] = profile_name + shared_credentials_filename = self._session.get_config_variable( + 'credentials_file') + self._config_writer.update_config( + credential_file_values, + shared_credentials_filename) diff --git a/awscli/examples/configure/_description.rst b/awscli/examples/configure/_description.rst index 040dd5b88e8c..a9c01ac3f3da 100644 --- a/awscli/examples/configure/_description.rst +++ b/awscli/examples/configure/_description.rst @@ -10,6 +10,11 @@ When you are prompted for information, the current value will be displayed in config file. It does not use any configuration values from environment variables or the IAM role. +Note: the values you provide for the AWS Access Key ID and the AWS Secret +Access Key will be written to the shared credentials file +(``~/.aws/credentials``). + + ======================= Configuration Variables ======================= diff --git a/awscli/examples/configure/set/_description.rst b/awscli/examples/configure/set/_description.rst index b4bad9e53829..b915e39680cf 100644 --- a/awscli/examples/configure/set/_description.rst +++ b/awscli/examples/configure/set/_description.rst @@ -11,3 +11,8 @@ configuration value. If the config file does not exist, one will automatically be created. If the configuration value already exists in the config file, it will updated with the new configuration value. + +Setting a value for the ``aws_access_key_id``, ``aws_secret_access_key``, or +the ``aws_session_token`` will result in the value being writen to the +shared credentials file (``~/.aws/credentials``). All other values will +be written to the config file (default location is ``~/.aws/config``). diff --git a/tests/unit/customizations/test_configure.py b/tests/unit/customizations/test_configure.py index b70276b878ea..12511139fa5f 100644 --- a/tests/unit/customizations/test_configure.py +++ b/tests/unit/customizations/test_configure.py @@ -70,6 +70,10 @@ def get_scoped_config(self): return self.config def get_config_variable(self, name, methods=None): + if name == 'credentials_file': + # The credentials_file var doesn't require a + # profile to exist. + return 'fake_credentials_file' if self.profile_does_not_exist and not name == 'config_file': raise ProfileNotFound(profile='foo') if methods is not None: @@ -98,12 +102,22 @@ def setUp(self): prompter=self.precanned, config_writer=self.writer) + def assert_credentials_file_updated_with(self, new_values): + called_args = self.writer.update_config.call_args_list + credentials_file_call = called_args[0] + self.assertEqual(credentials_file_call, + mock.call(new_values, 'fake_credentials_file')) + def test_configure_command_sends_values_to_writer(self): self.configure(args=[], parsed_globals=self.global_args) - self.writer.update_config.assert_called_with( + # Credentials are always written to the shared credentials file. + self.assert_credentials_file_updated_with( {'aws_access_key_id': 'new_value', - 'aws_secret_access_key': 'new_value', - 'region': 'new_value', + 'aws_secret_access_key': 'new_value'}) + + # Non-credentials config is written to the config file. + self.writer.update_config.assert_called_with( + {'region': 'new_value', 'output': 'new_value'}, 'myconfigfile') def test_same_values_are_not_changed(self): @@ -154,10 +168,12 @@ def test_section_name_can_be_changed_for_profiles(self): self.global_args.profile = 'myname' self.configure(args=[], parsed_globals=self.global_args) # Note the __section__ key name. + self.assert_credentials_file_updated_with( + {'aws_access_key_id': 'new_value', + 'aws_secret_access_key': 'new_value', + '__section__': 'myname'}) self.writer.update_config.assert_called_with( {'__section__': 'profile myname', - 'aws_access_key_id': 'new_value', - 'aws_secret_access_key': 'new_value', 'region': 'new_value', 'output': 'new_value'}, 'myconfigfile') @@ -173,10 +189,12 @@ def test_session_says_profile_does_not_exist(self): config_writer=self.writer) self.global_args.profile = 'profile-does-not-exist' self.configure(args=[], parsed_globals=self.global_args) + self.assert_credentials_file_updated_with( + {'aws_access_key_id': 'new_value', + 'aws_secret_access_key': 'new_value', + '__section__': 'profile-does-not-exist'}) self.writer.update_config.assert_called_with( {'__section__': 'profile profile-does-not-exist', - 'aws_access_key_id': 'new_value', - 'aws_secret_access_key': 'new_value', 'region': 'new_value', 'output': 'new_value'}, 'myconfigfile') @@ -760,6 +778,37 @@ def test_configure_set_with_profile_nested(self): {'__section__': 'profile foo', 's3': {'signature_version': 's3v4'}}, 'myconfigfile') + def test_access_key_written_to_shared_credentials_file(self): + set_command = configure.ConfigureSetCommand(self.session, self.config_writer) + set_command(args=['aws_access_key_id', 'foo'], + parsed_globals=None) + self.config_writer.update_config.assert_called_with( + {'__section__': 'default', + 'aws_access_key_id': 'foo'}, 'fake_credentials_file') + + def test_secret_key_written_to_shared_credentials_file(self): + set_command = configure.ConfigureSetCommand(self.session, self.config_writer) + set_command(args=['aws_secret_access_key', 'foo'], + parsed_globals=None) + self.config_writer.update_config.assert_called_with( + {'__section__': 'default', + 'aws_secret_access_key': 'foo'}, 'fake_credentials_file') + + def test_session_token_written_to_shared_credentials_file(self): + set_command = configure.ConfigureSetCommand(self.session, self.config_writer) + set_command(args=['aws_session_token', 'foo'], + parsed_globals=None) + self.config_writer.update_config.assert_called_with( + {'__section__': 'default', + 'aws_session_token': 'foo'}, 'fake_credentials_file') + + def test_access_key_written_to_shared_credentials_file_profile(self): + set_command = configure.ConfigureSetCommand(self.session, self.config_writer) + set_command(args=['profile.foo.aws_access_key_id', 'bar'], + parsed_globals=None) + self.config_writer.update_config.assert_called_with( + {'__section__': 'foo', + 'aws_access_key_id': 'bar'}, 'fake_credentials_file') class TestConfigValueMasking(unittest.TestCase): def test_config_value_is_masked(self): From 0f9a5a72d31c7ca291ec0616d736a136e6b51d50 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Tue, 23 Sep 2014 11:01:56 -0700 Subject: [PATCH 3/4] Incorporate review feedback. --- awscli/customizations/configure.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/awscli/customizations/configure.py b/awscli/customizations/configure.py index 0359a3b44416..ab40ba4cf13b 100644 --- a/awscli/customizations/configure.py +++ b/awscli/customizations/configure.py @@ -86,7 +86,7 @@ def update_config(self, new_values, config_filename): be created. Any parent directories will also be created if necessary. * If the section to update does not exist, it will be created. - * Any existing lines that specified by ``new_values`` + * Any existing lines that are specified by ``new_values`` **will not be touched**. This ensures that commented out values are left unaltered. @@ -362,6 +362,8 @@ class ConfigureSetCommand(BasicCommand): 'action': 'store', 'cli_type_name': 'string', 'positional_arg': True}, ] + # Any variables specified in this list will be written to + # the ~/.aws/credentials file instead of ~/.aws/config. _WRITE_TO_CREDS_FILE = ['aws_access_key_id', 'aws_secret_access_key', 'aws_session_token'] From 889c45a581f968793aa30eeda3b0481feb1db0a4 Mon Sep 17 00:00:00 2001 From: James Saryerwinnie Date: Tue, 23 Sep 2014 11:16:50 -0700 Subject: [PATCH 4/4] Add issue #847 to the changelog --- CHANGELOG.rst | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index abc23a3f910f..c87b5e3cc068 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -2,6 +2,15 @@ CHANGELOG ========= +Next Release (TBD) +================== + +* feature:Shared Credentials File: The ``aws configure`` and + ``aws configure set`` command now write out all credential + variables to the shared credentials file ``~/.aws/credentials`` + (`issue 847 `__) + + 1.4.4 =====