diff --git a/.changes/next-release/bugfix-codedeploy-61873.json b/.changes/next-release/bugfix-codedeploy-61873.json new file mode 100644 index 000000000000..3c4fdea9defb --- /dev/null +++ b/.changes/next-release/bugfix-codedeploy-61873.json @@ -0,0 +1,5 @@ +{ + "type": "bugfix", + "category": "codedeploy", + "description": "Restrict access to the on-premises instance configuration file written by ``aws deploy install`` on Windows" +} diff --git a/awscli/customizations/codedeploy/install.py b/awscli/customizations/codedeploy/install.py index 71858a05fa2a..94d52de0a345 100644 --- a/awscli/customizations/codedeploy/install.py +++ b/awscli/customizations/codedeploy/install.py @@ -113,16 +113,29 @@ def _create_config(self, params): except OSError as e: if e.errno != errno.EEXIST: raise e - if params.config_file != params.system.CONFIG_PATH: + # On Windows the file is always replaced, even when it is already the + # installed config, so it never keeps an ACL it had before. + if ( + params.config_file != params.system.CONFIG_PATH + or params.system.RECREATE_CONFIG_FILE + ): + # Read the contents first, since the source may be the file that + # is about to be replaced. with open(params.config_file, 'rb') as f: contents = f.read() - fd = os.open( - params.system.CONFIG_PATH, - os.O_WRONLY | os.O_CREAT | os.O_TRUNC | _O_BINARY, - 0o600, - ) + if params.system.RECREATE_CONFIG_FILE: + fd = params.system.create_config_file() + else: + fd = os.open( + params.system.CONFIG_PATH, + os.O_WRONLY | os.O_CREAT | os.O_TRUNC | _O_BINARY, + 0o600, + ) with os.fdopen(fd, 'wb') as f: - os.chmod(params.system.CONFIG_PATH, 0o600) + # create_config_file sets the file's ACL itself, and Windows + # ignores the mode, so chmod is only needed for os.open. + if not params.system.RECREATE_CONFIG_FILE: + os.chmod(params.system.CONFIG_PATH, 0o600) f.write(contents) sys.stdout.write('DONE\n') diff --git a/awscli/customizations/codedeploy/systems.py b/awscli/customizations/codedeploy/systems.py index 19e24f6fbdeb..0defa947d187 100644 --- a/awscli/customizations/codedeploy/systems.py +++ b/awscli/customizations/codedeploy/systems.py @@ -13,11 +13,102 @@ import ctypes import os +import stat import subprocess from awscli.utils import create_nested_client DEFAULT_CONFIG_FILE = 'codedeploy.onpremises.yml' +# Security descriptor for the Windows configuration file: a protected DACL +# (nothing inherited from the directory) granting full control only to +# LocalSystem and Administrators. +_WINDOWS_CONFIG_FILE_SDDL = 'D:P(A;;FA;;;SY)(A;;FA;;;BA)' + + +def _create_windows_file(path, sddl): + """Create ``path`` with the DACL in ``sddl`` and return a writable fd. + + Windows ignores the mode passed to os.open, so the file would take its + ACL from its directory. Here CreateFileW sets the DACL when it creates + the file, so the file is never accessible with any other ACL. + CREATE_NEW makes this fail if ``path`` already exists. + """ + import msvcrt + from ctypes import wintypes + + class SecurityAttributes(ctypes.Structure): + _fields_ = [ + ('nLength', wintypes.DWORD), + ('lpSecurityDescriptor', ctypes.c_void_p), + ('bInheritHandle', wintypes.BOOL), + ] + + advapi32 = ctypes.WinDLL('advapi32', use_last_error=True) + kernel32 = ctypes.WinDLL('kernel32', use_last_error=True) + + convert = advapi32.ConvertStringSecurityDescriptorToSecurityDescriptorW + convert.argtypes = [ + wintypes.LPCWSTR, + wintypes.DWORD, + ctypes.POINTER(ctypes.c_void_p), + ctypes.POINTER(wintypes.ULONG), + ] + convert.restype = wintypes.BOOL + + create_file = kernel32.CreateFileW + create_file.argtypes = [ + wintypes.LPCWSTR, + wintypes.DWORD, + wintypes.DWORD, + ctypes.c_void_p, + wintypes.DWORD, + wintypes.DWORD, + wintypes.HANDLE, + ] + create_file.restype = wintypes.HANDLE + + local_free = kernel32.LocalFree + local_free.argtypes = [ctypes.c_void_p] + local_free.restype = ctypes.c_void_p + + close_handle = kernel32.CloseHandle + close_handle.argtypes = [wintypes.HANDLE] + close_handle.restype = wintypes.BOOL + + sddl_revision_1 = 1 + generic_write = 0x40000000 + # No other handle may read, write or delete the file while it is open. + no_sharing = 0 + create_new = 1 + file_attribute_normal = 0x80 + invalid_handle_value = ctypes.c_void_p(-1).value + + descriptor = ctypes.c_void_p() + if not convert(sddl, sddl_revision_1, ctypes.byref(descriptor), None): + raise ctypes.WinError(ctypes.get_last_error()) + try: + attributes = SecurityAttributes( + ctypes.sizeof(SecurityAttributes), descriptor.value, False + ) + handle = create_file( + path, + generic_write, + no_sharing, + ctypes.byref(attributes), + create_new, + file_attribute_normal, + None, + ) + if handle is None or handle == invalid_handle_value: + raise ctypes.WinError(ctypes.get_last_error()) + finally: + local_free(descriptor) + try: + return msvcrt.open_osfhandle(handle, os.O_WRONLY | os.O_BINARY) + except Exception: + close_handle(handle) + raise + class System: UNSUPPORTED_SYSTEM_MSG = ( @@ -25,6 +116,10 @@ class System: 'Windows Server operating systems are supported.' ) + # Whether install replaces an existing configuration file with one from + # create_config_file, rather than truncating it in place. + RECREATE_CONFIG_FILE = False + def __init__(self, params): self.session = params.session self.s3 = create_nested_client( @@ -48,6 +143,9 @@ class Windows(System): CONFIG_FILE = 'conf.onpremises.yml' CONFIG_PATH = r'{0}\{1}'.format(CONFIG_DIR, CONFIG_FILE) INSTALLER = 'codedeploy-agent.msi' + # An existing file is replaced rather than truncated, which would keep + # its ACL and any handles opened to it before. + RECREATE_CONFIG_FILE = True def validate_administrator(self): if not ctypes.windll.shell32.IsUserAnAdmin(): @@ -55,6 +153,31 @@ def validate_administrator(self): 'You must run this command as an Administrator.' ) + def create_config_file(self): + """Replace CONFIG_PATH with a new, empty file and return its fd. + + The new file grants access only to LocalSystem and Administrators, + whatever the ACL of CONFIG_DIR. + """ + try: + # os.remove fails on read-only files, so clear the attribute. + os.chmod(self.CONFIG_PATH, stat.S_IWRITE) + os.remove(self.CONFIG_PATH) + except FileNotFoundError: + pass + except PermissionError as e: + raise RuntimeError( + 'Failed to replace the existing on-premises instance ' + f'configuration file {self.CONFIG_PATH}: {e}. Make sure no ' + 'other process has the file open, then run install again.' + ) + try: + return _create_windows_file( + self.CONFIG_PATH, _WINDOWS_CONFIG_FILE_SDDL + ) + except OSError as e: + raise RuntimeError(f'Failed to create {self.CONFIG_PATH}: {e}') + def install(self, params): if 'installer' in params: self.INSTALLER = params.installer diff --git a/tests/unit/customizations/codedeploy/test_install.py b/tests/unit/customizations/codedeploy/test_install.py index fc3a810f5da7..e4864171a70e 100644 --- a/tests/unit/customizations/codedeploy/test_install.py +++ b/tests/unit/customizations/codedeploy/test_install.py @@ -11,14 +11,19 @@ # ANY KIND, either express or implied. See the License for the specific # language governing permissions and limitations under the License. +import ctypes import os +import re +import shutil import stat import sys import tempfile from argparse import Namespace from awscli.customizations.codedeploy.install import Install -from awscli.customizations.codedeploy.systems import Ubuntu, Windows, RHEL, System +from awscli.customizations.codedeploy.systems import ( + Ubuntu, Windows, RHEL, System, _create_windows_file +) from awscli.testutils import mock, unittest from socket import timeout @@ -81,6 +86,11 @@ def setUp(self): ) self.os_chmod = self.os_chmod_patcher.start() + self.create_config_file_patcher = mock.patch.object( + Windows, 'create_config_file', return_value=4 + ) + self.create_config_file = self.create_config_file_patcher.start() + self.open_patcher = mock.patch( 'awscli.customizations.codedeploy.systems.open', mock.mock_open(), create=True @@ -116,6 +126,7 @@ def tearDown(self): self.os_open_patcher.stop() self.os_fdopen_patcher.stop() self.os_chmod_patcher.stop() + self.create_config_file_patcher.stop() self.open_patcher.stop() def test_install_throws_on_invalid_region(self): @@ -208,15 +219,50 @@ def test_install_for_windows(self, validate_administrator, install): self.assertEqual('codedeploy-agent.msi', self.args.installer) self.makedirs.assert_called_with(r'C:\ProgramData\Amazon\CodeDeploy') self.config_open.assert_called_with(self.config_file, 'rb') - self.os_open.assert_called_with( - r'C:\ProgramData\Amazon\CodeDeploy\conf.onpremises.yml', - mock.ANY, - 0o600, - ) + self.create_config_file.assert_called_with() + self.assertFalse(self.os_open.called) + self.assertFalse(self.os_chmod.called) + self.os_fdopen.assert_called_with(4, 'wb') self.os_fdopen().write.assert_called_with(b'config-contents') validate_administrator.assert_called_with() install.assert_called_with(self.args) + @mock.patch.object(Windows, 'install') + @mock.patch.object(Windows, 'validate_administrator') + def test_create_config_on_windows_replaces_when_source_is_destination( + self, validate_administrator, install + ): + self.system.return_value = 'Windows' + config_path = r'C:\ProgramData\Amazon\CodeDeploy\conf.onpremises.yml' + self.args.config_file = config_path + self.isfile.return_value = True + self.args.override_config = True + calls = mock.Mock() + calls.attach_mock(self.config_open, 'read') + calls.attach_mock(self.create_config_file, 'create_config_file') + self.install._run_main(self.args, self.globals) + self.assertEqual( + [name for name, _, _ in calls.mock_calls if '.' not in name], + ['read', 'create_config_file'], + ) + self.config_open.assert_called_with(config_path, 'rb') + self.os_fdopen().write.assert_called_with(b'config-contents') + install.assert_called_with(self.args) + + @mock.patch.object(Windows, 'install') + @mock.patch.object(Windows, 'validate_administrator') + def test_create_config_on_windows_stops_if_file_cannot_be_replaced( + self, validate_administrator, install + ): + self.system.return_value = 'Windows' + self.create_config_file.side_effect = RuntimeError('in use') + with mock.patch('sys.stderr', new_callable=mock.MagicMock) as stderr: + self.install._run_main(self.args, self.globals) + stderr.write.assert_called_with(mock.ANY) + self.assertIn('ERROR\nin use\n', stderr.write.call_args[0][0]) + self.assertFalse(self.os_fdopen().write.called) + self.assertFalse(install.called) + @mock.patch.object(Ubuntu, 'install') def test_create_config_uses_restricted_permissions(self, install): self.system.return_value = 'Linux' @@ -269,7 +315,9 @@ def setUp(self): self.params = Namespace( config_file=self.source, system=Namespace( - CONFIG_DIR=self.config_dir, CONFIG_PATH=self.config_path + CONFIG_DIR=self.config_dir, + CONFIG_PATH=self.config_path, + RECREATE_CONFIG_FILE=False, ), ) self.install = Install(mock.MagicMock()) @@ -303,5 +351,129 @@ def test_existing_config_file_is_tightened(self): self.assertEqual(f.read(), 'aws_secret_access_key: secret\n') +def _is_windows_admin(): + return os.name == 'nt' and bool(ctypes.windll.shell32.IsUserAnAdmin()) + + +def _get_windows_dacl(path): + """Return the DACL of ``path`` as an SDDL string, such as 'D:P(...)'.""" + from ctypes import wintypes + + advapi32 = ctypes.WinDLL('advapi32', use_last_error=True) + kernel32 = ctypes.WinDLL('kernel32', use_last_error=True) + get_info = advapi32.GetNamedSecurityInfoW + get_info.argtypes = [ + wintypes.LPCWSTR, + ctypes.c_int, + wintypes.DWORD, + ctypes.c_void_p, + ctypes.c_void_p, + ctypes.c_void_p, + ctypes.c_void_p, + ctypes.POINTER(ctypes.c_void_p), + ] + get_info.restype = wintypes.DWORD + to_string = advapi32.ConvertSecurityDescriptorToStringSecurityDescriptorW + to_string.argtypes = [ + ctypes.c_void_p, + wintypes.DWORD, + wintypes.DWORD, + ctypes.POINTER(ctypes.c_void_p), + ctypes.c_void_p, + ] + to_string.restype = wintypes.BOOL + kernel32.LocalFree.argtypes = [ctypes.c_void_p] + kernel32.LocalFree.restype = ctypes.c_void_p + + se_file_object = 1 + dacl_security_information = 0x4 + descriptor = ctypes.c_void_p() + result = get_info( + path, + se_file_object, + dacl_security_information, + None, + None, + None, + None, + ctypes.byref(descriptor), + ) + if result != 0: + raise ctypes.WinError(result) + try: + sddl = ctypes.c_void_p() + if not to_string( + descriptor, 1, dacl_security_information, ctypes.byref(sddl), None + ): + raise ctypes.WinError(ctypes.get_last_error()) + try: + return ctypes.wstring_at(sddl.value) + finally: + kernel32.LocalFree(sddl) + finally: + kernel32.LocalFree(descriptor) + + +def _dacl_trustees(dacl): + return {ace.split(';')[5] for ace in re.findall(r'\(([^)]*)\)', dacl)} + + +@unittest.skipIf( + not _is_windows_admin(), 'Windows ACLs; requires an elevated session' +) +class TestInstallConfigFileWindowsAcl(unittest.TestCase): + def setUp(self): + self.tempdir = tempfile.mkdtemp() + self.config_dir = os.path.join(self.tempdir, 'CodeDeploy') + self.config_path = os.path.join(self.config_dir, 'conf.onpremises.yml') + self.source = os.path.join(self.tempdir, 'codedeploy.onpremises.yml') + with open(self.source, 'w') as f: + f.write('aws_secret_access_key: secret\n') + system = Windows(Namespace(session=mock.MagicMock(), region=None)) + system.CONFIG_DIR = self.config_dir + system.CONFIG_PATH = self.config_path + self.params = Namespace(config_file=self.source, system=system) + self.install = Install(mock.MagicMock()) + + def tearDown(self): + shutil.rmtree(self.tempdir) + + def create_world_readable_config(self, contents): + # An explicit Everyone-read entry, which a change to the directory + # ACL or truncating the file would not remove. + os.makedirs(self.config_dir) + fd = _create_windows_file( + self.config_path, 'D:(A;;FA;;;BA)(A;;FR;;;WD)' + ) + with os.fdopen(fd, 'w') as f: + f.write(contents) + self.assertIn( + 'WD', _dacl_trustees(_get_windows_dacl(self.config_path)) + ) + + def assert_admin_only_config(self): + dacl = _get_windows_dacl(self.config_path) + # Protected, so nothing is inherited from the directory. + self.assertTrue(dacl.startswith('D:P'), dacl) + self.assertEqual(_dacl_trustees(dacl), {'SY', 'BA'}) + with open(self.config_path) as f: + self.assertEqual(f.read(), 'aws_secret_access_key: secret\n') + + def test_new_config_file_is_admin_only(self): + self.install._create_config(self.params) + self.assert_admin_only_config() + + def test_existing_config_file_with_explicit_access_is_replaced(self): + self.create_world_readable_config('old\n') + self.install._create_config(self.params) + self.assert_admin_only_config() + + def test_installed_config_as_source_is_replaced(self): + self.create_world_readable_config('aws_secret_access_key: secret\n') + self.params.config_file = self.config_path + self.install._create_config(self.params) + self.assert_admin_only_config() + + if __name__ == "__main__": unittest.main() diff --git a/tests/unit/customizations/codedeploy/test_systems.py b/tests/unit/customizations/codedeploy/test_systems.py index c4e50ff9db84..45035b2bb8f9 100644 --- a/tests/unit/customizations/codedeploy/test_systems.py +++ b/tests/unit/customizations/codedeploy/test_systems.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 stat import subprocess from argparse import Namespace @@ -74,6 +75,65 @@ def test_config_path(self): def test_installer(self): self.assertEqual(self.installer, self.windows.INSTALLER) + def test_recreates_config_file(self): + self.assertTrue(self.windows.RECREATE_CONFIG_FILE) + + def _patch_create(self): + module = 'awscli.customizations.codedeploy.systems.' + patchers = { + 'chmod': mock.patch(module + 'os.chmod'), + 'remove': mock.patch(module + 'os.remove'), + 'create': mock.patch( + module + '_create_windows_file', return_value=4 + ), + } + mocks = {} + for name, patcher in patchers.items(): + mocks[name] = patcher.start() + self.addCleanup(patcher.stop) + return mocks + + def test_create_config_file_replaces_existing_file(self): + mocks = self._patch_create() + calls = mock.Mock() + for name, m in mocks.items(): + calls.attach_mock(m, name) + self.assertEqual(self.windows.create_config_file(), 4) + self.assertEqual( + calls.mock_calls, + [ + mock.call.chmod(self.config_path, stat.S_IWRITE), + mock.call.remove(self.config_path), + mock.call.create( + self.config_path, 'D:P(A;;FA;;;SY)(A;;FA;;;BA)' + ), + ], + ) + + def test_create_config_file_handles_missing_file(self): + mocks = self._patch_create() + mocks['chmod'].side_effect = FileNotFoundError() + self.assertEqual(self.windows.create_config_file(), 4) + self.assertFalse(mocks['remove'].called) + self.assertTrue(mocks['create'].called) + + def test_create_config_file_explains_file_in_use(self): + mocks = self._patch_create() + mocks['remove'].side_effect = PermissionError( + 13, 'The process cannot access the file' + ) + with self.assertRaisesRegex( + RuntimeError, 'Make sure no other process has the file open' + ): + self.windows.create_config_file() + self.assertFalse(mocks['create'].called) + + def test_create_config_file_raises_on_create_failure(self): + mocks = self._patch_create() + mocks['create'].side_effect = OSError(80, 'The file exists') + with self.assertRaisesRegex(RuntimeError, 'Failed to create'): + self.windows.create_config_file() + def test_install(self): process = mock.MagicMock() process.communicate.side_effect = [('', ''), ('Running', '')] @@ -231,6 +291,9 @@ def test_config_path(self): def test_installer(self): self.assertEqual(self.installer, self.ubuntu.INSTALLER) + def test_does_not_recreate_config_file(self): + self.assertFalse(self.ubuntu.RECREATE_CONFIG_FILE) + @mock.patch('os.geteuid', create=True) def test_validate_administrator_throws(self, geteuid): geteuid.return_value = 1