From 4c108eea6411df12e9292e4171c1ca2c8d8b129f Mon Sep 17 00:00:00 2001 From: Stefan Agner Date: Fri, 14 Feb 2025 18:19:35 +0100 Subject: [PATCH] Always validate Backup before restoring (#5632) * Validate Backup always before restoring Since #5519 we check the encryption password early in restore case. This has the side effect that we check the file existance early too. However, in the non-encryption case, the file is not checked early. This PR changes the behavior to always validate the backup file before restoring, ensuring both encryption and non-encryption cases are handled consistently. In particular, the last case of test_restore_immediate_errors actually validates that behavior. That test should actually have failed so far. But it seems that because we validate the backup shortly after freeze anyways, the exception still got raised early enough. A simply `await asyncio.sleep(10)` right after the freeze makes the test case fail. With this change, the test works consistently. * Address pylint * Fix backup_manager tests * Drop warning message --- supervisor/backups/backup.py | 26 ++++++++++++-------------- supervisor/backups/manager.py | 12 +++++------- tests/api/test_backups.py | 20 ++++++++++++++++---- tests/backups/conftest.py | 2 ++ tests/backups/test_backup.py | 26 ++++++++++++-------------- tests/backups/test_manager.py | 4 ++-- 6 files changed, 49 insertions(+), 41 deletions(-) diff --git a/supervisor/backups/backup.py b/supervisor/backups/backup.py index 2e379df6e..49d67a845 100644 --- a/supervisor/backups/backup.py +++ b/supervisor/backups/backup.py @@ -366,14 +366,14 @@ class Backup(JobGroup): backend=default_backend(), ) - async def validate_password(self, location: str | None) -> bool: - """Validate backup password. + async def validate_backup(self, location: str | None) -> None: + """Validate backup. - Returns false only when the password is known to be wrong. + Checks if we can access the backup file and decrypt if necessary. """ backup_file: Path = self.all_locations[location][ATTR_PATH] - def _validate_file() -> bool: + def _validate_file() -> None: ending = f".tar{'.gz' if self.compressed else ''}" with tarfile.open(backup_file, "r:") as backup: @@ -386,8 +386,8 @@ class Backup(JobGroup): None, ) if not test_tar_name: - _LOGGER.warning("No tar file found to validate password with") - return True + # From Supervisor perspective, a metadata only backup only is valid. + return test_tar_file = backup.extractfile(test_tar_name) try: @@ -399,16 +399,14 @@ class Backup(JobGroup): fileobj=test_tar_file, ): # If we can read the tar file, the password is correct - return True - except tarfile.ReadError: - _LOGGER.debug("Invalid password") - return False - except Exception: # pylint: disable=broad-exception-caught - _LOGGER.exception("Unexpected error validating password") - return True + return + except tarfile.ReadError as ex: + raise BackupInvalidError( + f"Invalid password for backup {backup.slug}", _LOGGER.error + ) from ex try: - return await self.sys_run_in_executor(_validate_file) + await self.sys_run_in_executor(_validate_file) except FileNotFoundError as err: self.sys_create_task(self.sys_backups.reload(location)) raise BackupFileNotFoundError( diff --git a/supervisor/backups/manager.py b/supervisor/backups/manager.py index 87e7b2de5..14f45dcc2 100644 --- a/supervisor/backups/manager.py +++ b/supervisor/backups/manager.py @@ -717,7 +717,7 @@ class BackupManager(FileConfiguration, JobGroup): _job_override__cleanup=False ) - async def _set_location_password( + async def _validate_backup_location( self, backup: Backup, password: str | None = None, @@ -732,13 +732,11 @@ class BackupManager(FileConfiguration, JobGroup): location = location if location != DEFAULT else backup.location if backup.all_locations[location][ATTR_PROTECTED]: backup.set_password(password) - if not await backup.validate_password(location): - raise BackupInvalidError( - f"Invalid password for backup {backup.slug}", _LOGGER.error - ) else: backup.set_password(None) + await backup.validate_backup(location) + @Job( name=JOB_FULL_RESTORE, conditions=[ @@ -767,7 +765,7 @@ class BackupManager(FileConfiguration, JobGroup): f"{backup.slug} is only a partial backup!", _LOGGER.error ) - await self._set_location_password(backup, password, location) + await self._validate_backup_location(backup, password, location) if backup.supervisor_version > self.sys_supervisor.version: raise BackupInvalidError( @@ -832,7 +830,7 @@ class BackupManager(FileConfiguration, JobGroup): folder_list.remove(FOLDER_HOMEASSISTANT) homeassistant = True - await self._set_location_password(backup, password, location) + await self._validate_backup_location(backup, password, location) if backup.homeassistant is None and homeassistant: raise BackupInvalidError( diff --git a/tests/api/test_backups.py b/tests/api/test_backups.py index 45ddca10a..39fc8758b 100644 --- a/tests/api/test_backups.py +++ b/tests/api/test_backups.py @@ -16,7 +16,11 @@ from supervisor.backups.backup import Backup from supervisor.const import CoreState from supervisor.coresys import CoreSys from supervisor.docker.manager import DockerAPI -from supervisor.exceptions import AddonsError, HomeAssistantBackupError +from supervisor.exceptions import ( + AddonsError, + BackupInvalidError, + HomeAssistantBackupError, +) from supervisor.homeassistant.core import HomeAssistantCore from supervisor.homeassistant.module import HomeAssistant from supervisor.homeassistant.websocket import HomeAssistantWebSocket @@ -466,6 +470,7 @@ async def test_restore_immediate_errors( assert "only a partial backup" in (await resp.json())["message"] with ( + patch.object(Backup, "validate_backup"), patch.object( Backup, "supervisor_version", @@ -488,7 +493,11 @@ async def test_restore_immediate_errors( patch.object( Backup, "all_locations", new={None: {"path": None, "protected": True}} ), - patch.object(Backup, "validate_password", return_value=False), + patch.object( + Backup, + "validate_backup", + side_effect=BackupInvalidError("Invalid password"), + ), ): resp = await api_client.post( f"/backups/{mock_partial_backup.slug}/restore/partial", @@ -497,7 +506,10 @@ async def test_restore_immediate_errors( assert resp.status == 400 assert "Invalid password" in (await resp.json())["message"] - with patch.object(Backup, "homeassistant", new=PropertyMock(return_value=None)): + with ( + patch.object(Backup, "validate_backup"), + patch.object(Backup, "homeassistant", new=PropertyMock(return_value=None)), + ): resp = await api_client.post( f"/backups/{mock_partial_backup.slug}/restore/partial", json={"background": True, "homeassistant": True}, @@ -944,7 +956,7 @@ async def test_restore_backup_from_location( body = await resp.json() assert ( body["message"] - == f"Cannot open backup at {backup_local_path.as_posix()}, file does not exist!" + == f"Cannot validate backup at {backup_local_path.as_posix()}, file does not exist!" ) resp = await api_client.post( diff --git a/tests/backups/conftest.py b/tests/backups/conftest.py index 450b7a9e1..7fa2910f5 100644 --- a/tests/backups/conftest.py +++ b/tests/backups/conftest.py @@ -42,6 +42,7 @@ def partial_backup_mock(backup_mock): backup_instance.supervisor_version = "9999.09.9.dev9999" backup_instance.location = None backup_instance.all_locations = {None: {"protected": False}} + backup_instance.validate_backup = AsyncMock() yield backup_mock @@ -55,6 +56,7 @@ def full_backup_mock(backup_mock): backup_instance.supervisor_version = "9999.09.9.dev9999" backup_instance.location = None backup_instance.all_locations = {None: {"protected": False}} + backup_instance.validate_backup = AsyncMock() yield backup_mock diff --git a/tests/backups/test_backup.py b/tests/backups/test_backup.py index 93dca5200..d2098cc4a 100644 --- a/tests/backups/test_backup.py +++ b/tests/backups/test_backup.py @@ -11,7 +11,7 @@ import pytest from supervisor.backups.backup import Backup from supervisor.backups.const import BackupType from supervisor.coresys import CoreSys -from supervisor.exceptions import BackupFileNotFoundError +from supervisor.exceptions import BackupFileNotFoundError, BackupInvalidError from tests.common import get_fixture_path @@ -79,22 +79,21 @@ async def test_consolidate( @pytest.mark.asyncio @pytest.mark.parametrize( - "tarfile_side_effect, securetar_side_effect, expected_result, expect_exception", + "tarfile_side_effect, securetar_side_effect, expected_exception", [ - (None, None, True, False), # Successful validation - (FileNotFoundError, None, None, True), # File not found - (None, tarfile.ReadError, False, False), # Invalid password + (None, None, None), # Successful validation + (FileNotFoundError, None, BackupFileNotFoundError), # File not found + (None, tarfile.ReadError, BackupInvalidError), # Invalid password ], ) -async def test_validate_password( +async def test_validate_backup( coresys: CoreSys, tmp_path: Path, tarfile_side_effect, securetar_side_effect, - expected_result, - expect_exception, + expected_exception, ): - """Parameterized test for validate_password.""" + """Parameterized test for validate_backup.""" enc_tar = Path(copy(get_fixture_path("backup_example_enc.tar"), tmp_path)) enc_backup = Backup(coresys, enc_tar, "test", None) await enc_backup.load() @@ -120,9 +119,8 @@ async def test_validate_password( MagicMock(side_effect=securetar_side_effect), ), ): - if expect_exception: - with pytest.raises(BackupFileNotFoundError): - await enc_backup.validate_password(None) + if expected_exception: + with pytest.raises(expected_exception): + await enc_backup.validate_backup(None) else: - result = await enc_backup.validate_password(None) - assert result == expected_result + await enc_backup.validate_backup(None) diff --git a/tests/backups/test_manager.py b/tests/backups/test_manager.py index 9be702d7f..b11b8bfba 100644 --- a/tests/backups/test_manager.py +++ b/tests/backups/test_manager.py @@ -344,7 +344,7 @@ async def test_fail_invalid_full_backup( backup_instance = full_backup_mock.return_value backup_instance.all_locations[None]["protected"] = True - backup_instance.validate_password = AsyncMock(return_value=False) + backup_instance.validate_backup.side_effect = BackupInvalidError() with pytest.raises(BackupInvalidError): await manager.do_restore_full(backup_instance) @@ -373,7 +373,7 @@ async def test_fail_invalid_partial_backup( backup_instance = partial_backup_mock.return_value backup_instance.all_locations[None]["protected"] = True - backup_instance.validate_password = AsyncMock(return_value=False) + backup_instance.validate_backup.side_effect = BackupInvalidError() with pytest.raises(BackupInvalidError): await manager.do_restore_partial(backup_instance)