From acd8717839c0980cd08f27d483b4fa1a29547fad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20M=C3=A9taireau?= Date: Thu, 17 Apr 2025 17:16:10 +0200 Subject: [PATCH] Update container installation logic to allow in-place updates The isolation provider `install()` method is now passed a `should_upgrade` argument, which is read from the settings and represents the user decision about updates. The tests have been updated to reflect these changes. --- dangerzone/cli.py | 5 +- dangerzone/gui/main_window.py | 4 +- dangerzone/isolation_provider/container.py | 55 ++++++---- dangerzone/isolation_provider/dummy.py | 2 +- dangerzone/isolation_provider/qubes.py | 2 +- tests/conftest.py | 1 + tests/gui/test_updater.py | 6 +- tests/isolation_provider/test_container.py | 119 +++++++++++++-------- 8 files changed, 120 insertions(+), 74 deletions(-) diff --git a/dangerzone/cli.py b/dangerzone/cli.py index 1353995..d5699dd 100644 --- a/dangerzone/cli.py +++ b/dangerzone/cli.py @@ -71,8 +71,8 @@ def cli_main( ) -> None: setup_logging() display_banner() + settings = Settings() if set_container_runtime: - settings = Settings() if set_container_runtime == "default": settings.unset_custom_runtime() click.echo( @@ -117,7 +117,8 @@ def cli_main( sys.exit(1) # Ensure container is installed - dangerzone.isolation_provider.install() + should_upgrade = bool(settings.get("updater_check_all")) + dangerzone.isolation_provider.install(should_upgrade) # Convert the document print_header("Converting document to safe PDF") diff --git a/dangerzone/gui/main_window.py b/dangerzone/gui/main_window.py index 2383222..f78b6ad 100644 --- a/dangerzone/gui/main_window.py +++ b/dangerzone/gui/main_window.py @@ -450,9 +450,9 @@ class InstallContainerThread(QtCore.QThread): def run(self) -> None: error = None try: - should_upgrade = self.dangerzone.settings.get("updater_check_all") + should_upgrade = bool(self.dangerzone.settings.get("updater_check_all")) installed = self.dangerzone.isolation_provider.install( - should_upgrade=bool(should_upgrade), callback=self.process_stdout.emit + should_upgrade=should_upgrade, callback=self.process_stdout.emit ) except Exception as e: log.error("Container installation problem") diff --git a/dangerzone/isolation_provider/container.py b/dangerzone/isolation_provider/container.py index 50bf64d..f6f8e03 100644 --- a/dangerzone/isolation_provider/container.py +++ b/dangerzone/isolation_provider/container.py @@ -95,31 +95,49 @@ class Container(IsolationProvider): @staticmethod def install( - should_upgrade: bool, callback: Callable, last_try: bool = False + should_upgrade: bool, + callback: Optional[Callable] = sys.stdout.write, + last_try: bool = False, ) -> bool: - """Check if an update is available and install it if necessary.""" + """ + Install a (local or remote) container image. + + Use the local `container.tar` image if: + + - No image is currently installed and `should_upgrade` is set to False + - No image is currently installed and no upgrades are available + + Upgrade to the last remote container image if: + + - An upgrade is available and `should_upgrade` is set to True + """ + + installed_tags = container_utils.list_image_tags() if not should_upgrade: log.debug("Skipping container upgrade check as requested by the settings") + if not installed_tags: + install_local_container_tar() else: - update_available, image_digest = updater.is_update_available( - container_utils.CONTAINER_NAME, - updater.DEFAULT_PUBKEY_LOCATION, + update_available, image_digest = is_update_available( + CONTAINER_NAME, + DEFAULT_PUBKEY_LOCATION, ) if update_available and image_digest: log.debug("Upgrading container image to %s", image_digest) - updater.upgrade_container_image( - container_utils.CONTAINER_NAME, + upgrade_container_image( + CONTAINER_NAME, image_digest, - updater.DEFAULT_PUBKEY_LOCATION, + DEFAULT_PUBKEY_LOCATION, callback=callback, ) else: - log.debug("No update available for the container") + log.debug("No update available for the container.") + if not installed_tags: + install_local_container_tar() try: - updater.verify_local_image( - container_utils.CONTAINER_NAME, updater.DEFAULT_PUBKEY_LOCATION - ) - except errors.ImageNotPresentException: + verify_local_image(CONTAINER_NAME) + except UpdaterError: + # delete_image() if last_try: raise log.debug("Container image not found, trying to install it.") @@ -210,13 +228,8 @@ class Container(IsolationProvider): ) -> subprocess.Popen: runtime = Runtime() - image_digest = container_utils.get_local_image_digest( - container_utils.CONTAINER_NAME - ) - updater.verify_local_image( - container_utils.CONTAINER_NAME, - updater.DEFAULT_PUBKEY_LOCATION, - ) + image_digest = container_utils.get_local_image_digest(CONTAINER_NAME) + verify_local_image(CONTAINER_NAME) security_args = self.get_runtime_security_args() debug_args = [] if self.debug: @@ -225,7 +238,7 @@ class Container(IsolationProvider): enable_stdin = ["-i"] set_name = ["--name", name] prevent_leakage_args = ["--rm"] - image_name = [container_utils.CONTAINER_NAME + "@sha256:" + image_digest] + image_name = [CONTAINER_NAME + "@sha256:" + image_digest] args = ( ["run"] + security_args diff --git a/dangerzone/isolation_provider/dummy.py b/dangerzone/isolation_provider/dummy.py index fac973f..a70a4ef 100644 --- a/dangerzone/isolation_provider/dummy.py +++ b/dangerzone/isolation_provider/dummy.py @@ -36,7 +36,7 @@ class Dummy(IsolationProvider): ) super().__init__() - def install(self) -> bool: + def install(self, *args, **kwargs) -> bool: return True @staticmethod diff --git a/dangerzone/isolation_provider/qubes.py b/dangerzone/isolation_provider/qubes.py index d46e6cc..defc4e8 100644 --- a/dangerzone/isolation_provider/qubes.py +++ b/dangerzone/isolation_provider/qubes.py @@ -18,7 +18,7 @@ log = logging.getLogger(__name__) class Qubes(IsolationProvider): """Uses a disposable qube for performing the conversion""" - def install(self) -> bool: + def install(self, *args, **kwargs) -> bool: return True @staticmethod diff --git a/tests/conftest.py b/tests/conftest.py index 64f1a44..1b8a9ca 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -9,6 +9,7 @@ import pytest from dangerzone.document import SAFE_EXTENSION from dangerzone.gui import Application +from dangerzone.isolation_provider import container sys.dangerzone_dev = True # type: ignore[attr-defined] diff --git a/tests/gui/test_updater.py b/tests/gui/test_updater.py index a0ee4ad..614b4cf 100644 --- a/tests/gui/test_updater.py +++ b/tests/gui/test_updater.py @@ -283,13 +283,13 @@ def test_update_errors( ) -> None: """Test update check errors.""" settings = updater.dangerzone.settings + # Always assume that we can perform multiple update checks in a row. + monkeypatch.setattr(releases, "_should_postpone_update_check", lambda _: False) + # Mock requests.get(). mocker.patch("dangerzone.updater.releases.requests.get") requests_mock = releases.requests.get - # Always assume that we can perform multiple update checks in a row. - monkeypatch.setattr(releases, "_should_postpone_update_check", lambda: False) - # Test 1 - Check that request exceptions are being detected as errors. requests_mock.side_effect = Exception("bad url") # type: ignore [attr-defined] report = releases.check_for_updates(settings) diff --git a/tests/isolation_provider/test_container.py b/tests/isolation_provider/test_container.py index 8a5f170..fc74eb3 100644 --- a/tests/isolation_provider/test_container.py +++ b/tests/isolation_provider/test_container.py @@ -6,9 +6,10 @@ from pytest_mock import MockerFixture from pytest_subprocess import FakeProcess from dangerzone import errors -from dangerzone.container_utils import Runtime +from dangerzone.container_utils import CONTAINER_NAME, Runtime from dangerzone.isolation_provider.container import Container from dangerzone.isolation_provider.qubes import is_qubes_native_conversion +from dangerzone.updater import SignatureError, UpdaterError from dangerzone.util import get_resource_path from .base import IsolationProviderTermination, IsolationProviderTest @@ -57,8 +58,13 @@ class TestContainer(IsolationProviderTest): ) provider.is_available() - def test_install_raise_if_image_cant_be_installed( - self, provider: Container, fp: FakeProcess, runtime_path: str + def test_install_raise_if_local_image_cant_be_installed( + self, + provider: Container, + fp: FakeProcess, + runtime_path: str, + skip_image_verification, + mocker: MockerFixture, ) -> None: """When an image installation fails, an exception should be raised""" @@ -74,60 +80,85 @@ class TestContainer(IsolationProviderTest): "list", "--format", "{{ .Tag }}", - "dangerzone.rocks/dangerzone", + CONTAINER_NAME, ], occurrences=2, ) - - fp.register_subprocess( - [ - runtime_path, - "load", - "-i", - get_resource_path("container.tar").absolute(), - ], - returncode=-1, + mocker.patch( + "dangerzone.isolation_provider.container.install_local_container_tar", + side_effect=UpdaterError, ) - with pytest.raises(errors.ImageInstallationException): - provider.install() + with pytest.raises(UpdaterError): + provider.install(should_upgrade=False) - def test_install_raises_if_still_not_installed( - self, provider: Container, fp: FakeProcess, runtime_path: str + def test_install_raise_if_local_image_cant_be_verified( + self, + provider: Container, + runtime_path: str, + skip_image_verification, + mocker: MockerFixture, ) -> None: - """When an image keep being not installed, it should return False""" - fp.register_subprocess( - [runtime_path, "version", "-f", "{{.Client.Version}}"], - stdout="4.0.0", + """In case an image has been installed but its signature cannot be verified, an exception should be raised""" + + mocker.patch( + "dangerzone.isolation_provider.container.container_utils.list_image_tags", + return_value=["a-tag"], + ) + mocker.patch( + "dangerzone.isolation_provider.container.verify_local_image", + side_effect=SignatureError, ) - fp.register_subprocess( - [runtime_path, "image", "ls"], + with pytest.raises(SignatureError): + provider.install(should_upgrade=False) + + def test_install_raise_if_local_image_install_works_on_second_try( + self, + provider: Container, + runtime_path: str, + skip_image_verification, + mocker: MockerFixture, + ) -> None: + """In case an image has been installed but its signature cannot be verified, an exception should be raised""" + + mocker.patch( + "dangerzone.isolation_provider.container.container_utils.list_image_tags", + return_value=["a-tag"], + ) + mocker.patch( + "dangerzone.isolation_provider.container.verify_local_image", + side_effect=[SignatureError, True], ) - # First check should return nothing. - fp.register_subprocess( - [ - runtime_path, - "image", - "list", - "--format", - "{{ .Tag }}", - "dangerzone.rocks/dangerzone", - ], - occurrences=2, + provider.install(should_upgrade=False) + + def test_install_upgrades_if_available( + self, + provider: Container, + runtime_path: str, + skip_image_verification, + mocker: MockerFixture, + ) -> None: + """In case an image has been installed but its signature cannot be verified, an exception should be raised""" + + mocker.patch( + "dangerzone.isolation_provider.container.container_utils.list_image_tags", + return_value=["a-tag"], + ) + mocker.patch( + "dangerzone.isolation_provider.container.is_update_available", + return_value=(True, "digest"), + ) + upgrade = mocker.patch( + "dangerzone.isolation_provider.container.upgrade_container_image", + ) + mocker.patch( + "dangerzone.isolation_provider.container.verify_local_image", ) - fp.register_subprocess( - [ - runtime_path, - "load", - "-i", - get_resource_path("container.tar").absolute(), - ], - ) - with pytest.raises(errors.ImageNotPresentException): - provider.install() + provider.install(should_upgrade=True) + upgrade.assert_called() @pytest.mark.skipif( platform.system() not in ("Windows", "Darwin"),