From c480a87a29a6ab9ddbce2fd9b6a513ccad417e9f Mon Sep 17 00:00:00 2001 From: Daniel Vincze Date: Fri, 23 Oct 2026 15:49:38 +0300 Subject: [PATCH] Fix icacls permission grant error Fix an issue where the permission grant would fail when migrating hardened Windows Server images. The permission grant was initially added when using Nano Server as the minion image, which needed these extra permissions in order for DISM to write driver directories under `FileRepository`. This can fail either way if the migrated image is hardened, so best chance we have at success is to not fail on the `icacls` command and attempt installation anyway. NOTE: DISM only needs Modify permissions when installing drivers, while the permission grant command needs Full Control in order to change permissions, so there's a great chance that DISM installation would work anyway in a standard Server minion. --- AGENTS.md | 2 ++ coriolis/osmorphing/windows.py | 19 ++++++++++++++++--- coriolis/tests/osmorphing/test_windows.py | 23 +++++++++++++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ba5fe55bc..e22ab110d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,6 +24,8 @@ Users configure source/destination **endpoints** (credentials) and - Use `.tox/py3/bin/` (`stestr`, `ruff`); it has project deps. Ignore `.mypy_cache`, `.ruff_cache`, `.tox`. - Follow ruff (`tox.ini`, `ruff.toml`). +- Prefer f-strings over `%` formatting and `str.format` in new or + touched code. - Public methods need docstrings (subclasses may inherit). Use type hints when the type is known. - Do not add helpers for trivial checks such as diff --git a/coriolis/osmorphing/windows.py b/coriolis/osmorphing/windows.py index bd34fc277..f2a7f3694 100644 --- a/coriolis/osmorphing/windows.py +++ b/coriolis/osmorphing/windows.py @@ -950,8 +950,11 @@ def _install_dism_drivers(self, driver_paths): """Installs the drivers located at the given paths into the offline Windows image via DISM. - Temporarily grants permissions on the DriverStore FileRepository + Attempts to grant permissions on the DriverStore FileRepository (required on Nano Server) for the duration of the installation. + A failure to grant permissions is logged and ignored, since driver + installation usually succeeds without the extra permissions. + Permissions are revoked only when the grant succeeded. Paths which do not exist are skipped with a warning. """ sid = self._get_sid() @@ -959,7 +962,16 @@ def _install_dism_drivers(self, driver_paths): file_repo_path = ( "%sWindows\\System32\\DriverStore\\FileRepository" % self._os_root_dir ) - self._grant_permissions(file_repo_path, "*%s" % sid) + user = "*%s" % sid + permissions_granted = False + try: + self._grant_permissions(file_repo_path, user) + permissions_granted = True + except Exception as err: + LOG.warning( + f"Failed to grant permissions on '{file_repo_path}' for " + f"'{user}'. Continuing with driver installation: {err}" + ) try: for driver_path in driver_paths: if self._conn.test_path(driver_path): @@ -969,7 +981,8 @@ def _install_dism_drivers(self, driver_paths): "Could not locate driver dir '%s', skipping.", driver_path ) finally: - self._revoke_permissions(file_repo_path, "*%s" % sid) + if permissions_granted: + self._revoke_permissions(file_repo_path, user) def _setup_qemu_agent_installation_local_script( self, msi_source_path=None, msi_url=None, qemu_ga_source_dir=None diff --git a/coriolis/tests/osmorphing/test_windows.py b/coriolis/tests/osmorphing/test_windows.py index 220eff560..88344b4ef 100644 --- a/coriolis/tests/osmorphing/test_windows.py +++ b/coriolis/tests/osmorphing/test_windows.py @@ -1402,6 +1402,29 @@ def test_install_dism_drivers_revokes_on_failure( # Permissions must be revoked even if driver installation fails: mock_revoke_permissions.assert_called_once_with(exp_repo_path, f"*{null_sid}") + @mock.patch.object(windows.BaseWindowsMorphingTools, "_get_sid") + @mock.patch.object(windows.BaseWindowsMorphingTools, "_grant_permissions") + @mock.patch.object(windows.BaseWindowsMorphingTools, "_add_dism_driver") + @mock.patch.object(windows.BaseWindowsMorphingTools, "_revoke_permissions") + def test_install_dism_drivers_ignores_grant_failure( + self, + mock_revoke_permissions, + mock_add_dism_driver, + mock_grant_permissions, + mock_get_sid, + ): + null_sid = "S-1-0-0" + mock_get_sid.return_value = null_sid + mock_grant_permissions.side_effect = IOError("access denied") + self.conn.test_path.return_value = True + + self.morphing_tools._install_dism_drivers(["e:\\NetKVM"]) + + exp_repo_path = "C:\\Windows\\System32\\DriverStore\\FileRepository" + mock_grant_permissions.assert_called_once_with(exp_repo_path, f"*{null_sid}") + mock_add_dism_driver.assert_called_once_with("e:\\NetKVM") + mock_revoke_permissions.assert_not_called() + @mock.patch.object(windows.BaseWindowsMorphingTools, "register_firstboot_script") def test_setup_qemu_agent_installation_local_script_from_url( self, mock_register_firstboot_script