diff --git a/.changes/next-release/enhancement-clihistory-87056.json b/.changes/next-release/enhancement-clihistory-87056.json new file mode 100644 index 000000000000..8bc65bcbfa57 --- /dev/null +++ b/.changes/next-release/enhancement-clihistory-87056.json @@ -0,0 +1,5 @@ +{ + "type": "enhancement", + "category": "cli-history", + "description": "Create local history files with specific permissions" +} diff --git a/awscli/autoprompt/history.py b/awscli/autoprompt/history.py index 55a7c0b68a6b..af36357704cd 100644 --- a/awscli/autoprompt/history.py +++ b/awscli/autoprompt/history.py @@ -44,15 +44,24 @@ def load_history_strings(self): def store_string(self, string): history = {'version': self.HISTORY_VERSION, 'commands': []} try: + dir_path = os.path.dirname(self.filename) if os.path.exists(self.filename): with open(self.filename) as f: history = json.load(f) - elif not os.path.exists(os.path.dirname(self.filename)): - os.makedirs(os.path.dirname(self.filename)) + if not os.path.exists(dir_path): + os.makedirs(dir_path) + try: + os.chmod(dir_path, 0o700) + except OSError as e: + LOG.debug('Unable to set directory permissions: %s', e) history['commands'].append(string) history['commands'] = history['commands'][-self._max_commands :] with open(self.filename, 'w') as f: json.dump(history, f) + try: + os.chmod(self.filename, 0o600) + except OSError as e: + LOG.debug('Unable to set file permissions: %s', e) except Exception: LOG.debug('Exception on loading prompt history:', exc_info=True) diff --git a/awscli/botocore/utils.py b/awscli/botocore/utils.py index d7de8c102c85..4323862f2202 100644 --- a/awscli/botocore/utils.py +++ b/awscli/botocore/utils.py @@ -3943,6 +3943,10 @@ def __setitem__(self, cache_key, value): ) if not os.path.isdir(self._working_dir): os.makedirs(self._working_dir) + try: + os.chmod(self._working_dir, 0o700) + except OSError as e: + logger.debug('Unable to set directory permissions: %s', e) with os.fdopen( os.open(full_key, os.O_WRONLY | os.O_CREAT, 0o600), 'w' ) as f: diff --git a/awscli/customizations/history/__init__.py b/awscli/customizations/history/__init__.py index eaa23435b1a4..37a635791015 100644 --- a/awscli/customizations/history/__init__.py +++ b/awscli/customizations/history/__init__.py @@ -53,8 +53,14 @@ def attach_history_handler(session, parsed_args, **kwargs): history_filename = os.environ.get( HISTORY_FILENAME_ENV_VAR, DEFAULT_HISTORY_FILENAME ) - if not os.path.isdir(os.path.dirname(history_filename)): - os.makedirs(os.path.dirname(history_filename)) + + history_dir = os.path.dirname(history_filename) + if not os.path.isdir(history_dir): + os.makedirs(history_dir) + try: + os.chmod(history_dir, 0o700) + except OSError as e: + LOG.debug('Unable to set directory permissions: %s', e) connection = DatabaseConnection(history_filename) writer = DatabaseRecordWriter(connection) diff --git a/awscli/customizations/history/db.py b/awscli/customizations/history/db.py index aa0404c08382..255b54bfc322 100644 --- a/awscli/customizations/history/db.py +++ b/awscli/customizations/history/db.py @@ -13,6 +13,7 @@ import datetime import json import logging +import os import threading import time import uuid @@ -37,6 +38,15 @@ class DatabaseConnection: _ENABLE_WAL = 'PRAGMA journal_mode=WAL' def __init__(self, db_filename): + # Skip file operations for in-memory databases + if db_filename != ':memory:': + if not os.path.exists(db_filename): + # Create file so we can set permissions before sqlite opens it + open(db_filename, 'a').close() + try: + os.chmod(db_filename, 0o600) + except OSError as e: + LOG.debug('Unable to set file permissions: %s', e) self._connection = sqlite3.connect( db_filename, check_same_thread=False, isolation_level=None ) diff --git a/awscli/telemetry.py b/awscli/telemetry.py index 3cf44e83ace0..d46c07024b5c 100644 --- a/awscli/telemetry.py +++ b/awscli/telemetry.py @@ -11,6 +11,7 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. import io +import logging import os import sqlite3 import sys @@ -28,6 +29,8 @@ from awscli.compat import is_windows from awscli.utils import add_component_to_user_agent_extra +LOG = logging.getLogger(__name__) + _CACHE_DIR = Path.home() / '.aws' / 'cli' / 'cache' _DATABASE_FILENAME = 'session.db' _SESSION_LENGTH_SECONDS = 60 * 30 @@ -77,12 +80,15 @@ class CLISessionDatabaseConnection: _ENABLE_WAL = 'PRAGMA journal_mode=WAL' def __init__(self, connection=None): - self._connection = connection or sqlite3.connect( - _CACHE_DIR / _DATABASE_FILENAME, - check_same_thread=False, - isolation_level=None, - ) - self._ensure_cache_dir() + self._connection = connection + if self._connection is None: + self._ensure_cache_dir() + self._ensure_database_file() + self._connection = sqlite3.connect( + _CACHE_DIR / _DATABASE_FILENAME, + check_same_thread=False, + isolation_level=None, + ) self._ensure_database_setup() def execute(self, query, *parameters): @@ -96,6 +102,19 @@ def execute(self, query, *parameters): def _ensure_cache_dir(self): _CACHE_DIR.mkdir(parents=True, exist_ok=True) + try: + os.chmod(_CACHE_DIR, 0o700) + except OSError as e: + LOG.debug('Unable to set directory permissions: %s', e) + + def _ensure_database_file(self): + db_path = _CACHE_DIR / _DATABASE_FILENAME + if not db_path.exists(): + open(db_path, 'a').close() + try: + os.chmod(db_path, 0o600) + except OSError as e: + LOG.debug('Unable to set file permissions: %s', e) def _ensure_database_setup(self): self._create_session_table() diff --git a/tests/functional/test_telemetry.py b/tests/functional/test_telemetry.py index 698b639d0329..0575d2a36e9b 100644 --- a/tests/functional/test_telemetry.py +++ b/tests/functional/test_telemetry.py @@ -10,7 +10,10 @@ # 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 sqlite3 +import stat +from pathlib import Path from unittest.mock import MagicMock, PropertyMock, patch import pytest @@ -27,6 +30,7 @@ CLISessionOrchestrator, add_session_id_component_to_user_agent_extra, ) +from awscli.testutils import FileCreator from tests.markers import skip_if_windows @@ -130,6 +134,71 @@ def execute(self, query, *parameters): assert cursor.fetchall() == [] +@skip_if_windows +class TestCLISessionDatabaseConnectionPermissions: + def setup_method(self): + self.files = FileCreator() + + def teardown_method(self): + self.files.remove_all() + + def test_create_directory_with_secure_permissions(self): + cache_dir = self.files.full_path('cache') + cache_path = Path(cache_dir) + + with patch('awscli.telemetry._CACHE_DIR', cache_path): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + assert os.path.isdir(cache_dir) + dir_mode = stat.S_IMODE(os.stat(cache_dir).st_mode) + assert dir_mode == 0o700 + + def test_tighten_existing_directory_permissions(self): + cache_dir = self.files.full_path('cache') + os.makedirs(cache_dir, mode=0o755) + cache_path = Path(cache_dir) + + with patch('awscli.telemetry._CACHE_DIR', cache_path): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + dir_mode = stat.S_IMODE(os.stat(cache_dir).st_mode) + assert dir_mode == 0o700 + + def test_create_database_file_with_secure_permissions(self): + cache_dir = self.files.full_path('cache') + db_file = os.path.join(cache_dir, 'session.db') + cache_path = Path(cache_dir) + + with patch('awscli.telemetry._CACHE_DIR', cache_path): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + assert os.path.isfile(db_file) + file_mode = stat.S_IMODE(os.stat(db_file).st_mode) + assert file_mode == 0o600 + + def test_tighten_existing_database_file_permissions(self): + cache_dir = self.files.full_path('cache') + os.makedirs(cache_dir, mode=0o700) + db_file = os.path.join(cache_dir, 'session.db') + open(db_file, 'a').close() + os.chmod(db_file, 0o644) + cache_path = Path(cache_dir) + + with patch('awscli.telemetry._CACHE_DIR', cache_path): + with patch('awscli.telemetry._DATABASE_FILENAME', 'session.db'): + conn = CLISessionDatabaseConnection() + conn._connection.close() + + file_mode = stat.S_IMODE(os.stat(db_file).st_mode) + assert file_mode == 0o600 + + class TestCLISessionDatabaseWriter: def test_write(self, session_writer, session_reader, session_sweeper): session_writer.write( diff --git a/tests/unit/autoprompt/test_history.py b/tests/unit/autoprompt/test_history.py index 9c1475a2cc2d..0a3637d7cf96 100644 --- a/tests/unit/autoprompt/test_history.py +++ b/tests/unit/autoprompt/test_history.py @@ -13,8 +13,10 @@ import json import os import shutil +import stat import tempfile +import pytest from prompt_toolkit.buffer import Buffer from prompt_toolkit.completion import Completion from prompt_toolkit.document import Document @@ -23,6 +25,7 @@ from awscli.autoprompt.history import HistoryCompleter, HistoryDriver from awscli.testutils import mock, unittest +from tests.markers import skip_if_windows class TestHistoryCompleter(unittest.TestCase): @@ -190,3 +193,60 @@ def test_handle_io_errors(self, file_history_mock): history_driver = HistoryDriver(self.filename) file_history_mock.store_string.side_effect = IOError history_driver.store_string('aws dynamodb create-table') + + +@pytest.fixture +def temp_dir(): + dirname = tempfile.mkdtemp() + yield dirname + shutil.rmtree(dirname) + + +@skip_if_windows +def test_create_directory_with_secure_permissions(temp_dir): + subdir = os.path.join(temp_dir, 'newdir') + filename = os.path.join(subdir, 'prompt_history.json') + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + assert os.path.isdir(subdir) + dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) + assert dir_mode == 0o700 + + +@skip_if_windows +def test_tighten_existing_directory_permissions(temp_dir): + subdir = os.path.join(temp_dir, 'existingdir') + os.makedirs(subdir, mode=0o755) + filename = os.path.join(subdir, 'prompt_history.json') + + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + dir_mode = stat.S_IMODE(os.stat(subdir).st_mode) + assert dir_mode == 0o700 + + +@skip_if_windows +def test_create_file_with_secure_permissions(temp_dir): + filename = os.path.join(temp_dir, 'prompt_history.json') + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + assert os.path.isfile(filename) + file_mode = stat.S_IMODE(os.stat(filename).st_mode) + assert file_mode == 0o600 + + +@skip_if_windows +def test_tighten_existing_file_permissions(temp_dir): + filename = os.path.join(temp_dir, 'prompt_history.json') + with open(filename, 'w') as f: + json.dump({'version': 1, 'commands': []}, f) + os.chmod(filename, 0o644) + + history_driver = HistoryDriver(filename) + history_driver.store_string('aws ec2 describe-instances') + + file_mode = stat.S_IMODE(os.stat(filename).st_mode) + assert file_mode == 0o600 diff --git a/tests/unit/customizations/history/test_db.py b/tests/unit/customizations/history/test_db.py index e8237dd55a3f..7ee8f14970c9 100644 --- a/tests/unit/customizations/history/test_db.py +++ b/tests/unit/customizations/history/test_db.py @@ -15,6 +15,7 @@ import numbers import os import re +import stat import threading from awscli.compat import queue @@ -28,6 +29,7 @@ ) from awscli.testutils import FileCreator, mock, unittest from tests import CaseInsensitiveDict +from tests.markers import skip_if_windows class FakeDatabaseConnection: @@ -48,8 +50,20 @@ def tearDown(self): class TestDatabaseConnection(unittest.TestCase): + def setUp(self): + self.files = FileCreator() + + def tearDown(self): + self.files.remove_all() + + @skip_if_windows + @mock.patch('awscli.customizations.history.db.os.chmod') + @mock.patch('awscli.customizations.history.db.os.path.exists') @mock.patch('awscli.compat.sqlite3.connect') - def test_can_connect_to_argument_file(self, mock_connect): + def test_can_connect_to_argument_file( + self, mock_connect, mock_exists, mock_chmod + ): + mock_exists.return_value = True expected_location = os.path.expanduser( os.path.join('~', 'foo', 'bar', 'baz.db') ) @@ -64,7 +78,7 @@ def test_does_try_to_enable_wal(self, mock_connect): conn._connection.execute.assert_any_call('PRAGMA journal_mode=WAL') def test_does_ensure_table_created_first(self): - db = DatabaseConnection(":memory:") + db = DatabaseConnection(':memory:') cursor = db.execute('PRAGMA table_info(records)') schema = [col[:3] for col in cursor.fetchall()] expected_schema = [ @@ -85,6 +99,23 @@ def test_can_close(self, mock_connect): conn.close() self.assertTrue(connection.close.called) + @skip_if_windows + def test_create_new_file_with_secure_permissions(self): + db_filename = os.path.join(self.files.rootdir, 'new_secure.db') + DatabaseConnection(db_filename) + file_mode = stat.S_IMODE(os.stat(db_filename).st_mode) + self.assertEqual(file_mode, 0o600) + + @skip_if_windows + def test_tighten_existing_file_permissions(self): + db_filename = os.path.join(self.files.rootdir, 'existing.db') + open(db_filename, 'a').close() + os.chmod(db_filename, 0o644) + + DatabaseConnection(db_filename) + file_mode = stat.S_IMODE(os.stat(db_filename).st_mode) + self.assertEqual(file_mode, 0o600) + class TestDatabaseHistoryHandler(unittest.TestCase): UUID_PATTERN = re.compile( diff --git a/tests/unit/customizations/history/test_history.py b/tests/unit/customizations/history/test_history.py index f34830e4893a..1a8856bc4917 100644 --- a/tests/unit/customizations/history/test_history.py +++ b/tests/unit/customizations/history/test_history.py @@ -12,6 +12,7 @@ # language governing permissions and limitations under the License. import argparse import os +import stat from botocore.exceptions import ProfileNotFound from botocore.history import HistoryRecorder @@ -26,6 +27,7 @@ ) from awscli.customizations.history.db import DatabaseHistoryHandler from awscli.testutils import FileCreator, mock, unittest +from tests.markers import skip_if_windows class TestAttachHistoryHandler(unittest.TestCase): @@ -170,6 +172,59 @@ def test_profile_does_not_exist( self.assertFalse(mock_recorder.add_handler.called) self.assertFalse(mock_db_sqlite3.connect.called) + @skip_if_windows + @mock.patch('awscli.customizations.history.sqlite3') + @mock.patch('awscli.customizations.history.db.sqlite3') + @mock.patch( + 'awscli.customizations.history.HISTORY_RECORDER', spec=HistoryRecorder + ) + def test_create_directory_with_secure_permissions( + self, mock_recorder, mock_db_sqlite3, mock_sqlite3 + ): + mock_session = mock.Mock(Session) + mock_session.get_scoped_config.return_value = { + 'cli_history': 'enabled' + } + parsed_args = argparse.Namespace() + parsed_args.command = 's3' + + directory_to_create = os.path.join(self.files.rootdir, 'secure-dir') + db_filename = os.path.join(directory_to_create, 'name.db') + with mock.patch('os.environ', {'AWS_CLI_HISTORY_FILE': db_filename}): + attach_history_handler( + session=mock_session, parsed_args=parsed_args + ) + self.assertTrue(os.path.exists(directory_to_create)) + dir_mode = stat.S_IMODE(os.stat(directory_to_create).st_mode) + self.assertEqual(dir_mode, 0o700) + + @skip_if_windows + @mock.patch('awscli.customizations.history.sqlite3') + @mock.patch('awscli.customizations.history.db.sqlite3') + @mock.patch( + 'awscli.customizations.history.HISTORY_RECORDER', spec=HistoryRecorder + ) + def test_tighten_existing_directory_permissions( + self, mock_recorder, mock_db_sqlite3, mock_sqlite3 + ): + mock_session = mock.Mock(Session) + mock_session.get_scoped_config.return_value = { + 'cli_history': 'enabled' + } + parsed_args = argparse.Namespace() + parsed_args.command = 's3' + + directory_to_tighten = os.path.join(self.files.rootdir, 'loose-dir') + os.makedirs(directory_to_tighten, mode=0o755) + db_filename = os.path.join(directory_to_tighten, 'name.db') + + with mock.patch('os.environ', {'AWS_CLI_HISTORY_FILE': db_filename}): + attach_history_handler( + session=mock_session, parsed_args=parsed_args + ) + dir_mode = stat.S_IMODE(os.stat(directory_to_tighten).st_mode) + self.assertEqual(dir_mode, 0o700) + class TestAddHistoryCommand(unittest.TestCase): def test_add_history_command(self):