From 399cc207d59cd8c1c80b791f903fd52d8f851622 Mon Sep 17 00:00:00 2001 From: Adam Rozman Date: Mon, 30 Dec 2024 16:00:26 +0200 Subject: [PATCH] disable ISO cache image format and safety checks This commit: - Disables image format checks and safety checks for ISO disk image cacheing After some discussion in the community it has been decided that instead of changing the format detection and safety check logic, disabling the format and safety checks have a smaller maintenance footprint. Related-Bug: 2091611 Change-Id: Iff2be28c64a0469a3796003f3b8ed28d70631761 Signed-off-by: Adam Rozman --- ironic/drivers/modules/image_cache.py | 20 +++++---- ironic/drivers/modules/image_utils.py | 4 +- .../unit/drivers/modules/test_image_utils.py | 43 +++++++++++++++++++ ...e_img_validation_iso-3d694a83576bf189.yaml | 14 ++++++ 4 files changed, 71 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/disable_img_validation_iso-3d694a83576bf189.yaml diff --git a/ironic/drivers/modules/image_cache.py b/ironic/drivers/modules/image_cache.py index f72ea6c7fc..9f44070785 100644 --- a/ironic/drivers/modules/image_cache.py +++ b/ironic/drivers/modules/image_cache.py @@ -52,7 +52,7 @@ class ImageCache(object): """Class handling access to cache for master images.""" def __init__(self, master_dir, cache_size, cache_ttl, - disable_validation=False): + disable_validation=False, force_raw=True): """Constructor. :param master_dir: cache directory to work on @@ -64,11 +64,12 @@ class ImageCache(object): self.master_dir = master_dir self._cache_size = cache_size self._cache_ttl = cache_ttl + self._force_raw = force_raw + self._disable_validation = disable_validation if master_dir is not None: fileutils.ensure_tree(master_dir) - self._disable_validation = disable_validation - def fetch_image(self, href, dest_path, ctx=None, force_raw=True, + def fetch_image(self, href, dest_path, ctx=None, force_raw=None, expected_format=None, expected_checksum=None, expected_checksum_algo=None): """Fetch image by given href to the destination path. @@ -90,6 +91,7 @@ class ImageCache(object): :param expected_checksum_algo: The expected image checksum algorithm, if needed/supplied. """ + force_raw = force_raw if force_raw is not None else self._force_raw if expected_format is not None and self._disable_validation: raise AssertionError("BUG: passing expected_format to caches with " "disabled validation makes no sense") @@ -176,9 +178,8 @@ class ImageCache(object): self.clean_up() def _download_image(self, href, master_path, dest_path, img_info, - ctx=None, force_raw=True, expected_format=None, - expected_checksum=None, - expected_checksum_algo=None): + ctx=None, force_raw=None, expected_format=None, + expected_checksum=None, expected_checksum_algo=None): """Download image by href and store at a given path. This method should be called with uuid-specific lock taken. @@ -201,7 +202,7 @@ class ImageCache(object): # TODO(ghe): logging when image cannot be created tmp_dir = tempfile.mkdtemp(dir=self.master_dir) tmp_path = os.path.join(tmp_dir, os.path.basename(master_path)) - + force_raw = force_raw if force_raw is not None else self._force_raw try: with _concurrency_semaphore: _fetch(ctx, href, tmp_path, force_raw, expected_format, @@ -371,8 +372,9 @@ def _free_disk_space_for(path): return stat.f_frsize * stat.f_bavail -def _fetch(context, image_href, path, force_raw=False, expected_format=None, - expected_checksum=None, expected_checksum_algo=None, +def _fetch(context, image_href, path, force_raw=False, + expected_format=None, expected_checksum=None, + expected_checksum_algo=None, disable_validation=False): """Fetch image and convert to raw format if needed.""" assert not (disable_validation and expected_format) diff --git a/ironic/drivers/modules/image_utils.py b/ironic/drivers/modules/image_utils.py index c74d6ccc88..2d1512c850 100644 --- a/ironic/drivers/modules/image_utils.py +++ b/ironic/drivers/modules/image_utils.py @@ -155,7 +155,9 @@ class ISOImageCache(image_cache.ImageCache): # MiB -> B cache_size=CONF.deploy.iso_cache_size * 1024 * 1024, # min -> sec - cache_ttl=CONF.deploy.iso_cache_ttl * 60) + cache_ttl=CONF.deploy.iso_cache_ttl * 60, + # disable image format inspection and safety checks for ISO + disable_validation=True, force_raw=False) def _get_name(node, prefix='', suffix=''): diff --git a/ironic/tests/unit/drivers/modules/test_image_utils.py b/ironic/tests/unit/drivers/modules/test_image_utils.py index ee0ebb519d..cbc6969c11 100644 --- a/ironic/tests/unit/drivers/modules/test_image_utils.py +++ b/ironic/tests/unit/drivers/modules/test_image_utils.py @@ -18,14 +18,18 @@ import os import tempfile from unittest import mock +from oslo_config import cfg from oslo_utils import uuidutils +from ironic.common import image_service from ironic.common import images from ironic.common import states from ironic.common import utils from ironic.conductor import task_manager from ironic.drivers.modules import deploy_utils +from ironic.drivers.modules import image_cache from ironic.drivers.modules import image_utils +from ironic.tests import base from ironic.tests.unit.db import base as db_base from ironic.tests.unit.db import utils as db_utils from ironic.tests.unit.objects import utils as obj_utils @@ -35,6 +39,45 @@ INFO_DICT = db_utils.get_test_redfish_info() INFO_DICT_ILO = db_utils.get_test_ilo_info() +@mock.patch.object(image_service, 'get_image_service', autospec=True) +@mock.patch.object(image_cache.ImageCache, 'clean_up', autospec=True) +class ISOCacheTestCase(base.TestCase): + + def setUp(self): + super().setUp() + self.master_dir = tempfile.mkdtemp() + cfg.CONF.set_override('iso_master_path', self.master_dir, + group='deploy') + self.cache = image_utils.ISOImageCache() + self.dest_dir = tempfile.mkdtemp() + self.dest_path = os.path.join(self.dest_dir, 'dest') + self.uuid = uuidutils.generate_uuid() + self.master_path = ''.join([os.path.join(self.master_dir, self.uuid), + '.converted']) + self.img_info = {} + + @mock.patch.object(image_cache.ImageCache, '_download_image', + autospec=True) + @mock.patch.object(image_cache, '_fetch', autospec=True) + def test_fetch_image_iso(self, mock_fetch, mock_download, mock_clean_up, + mock_image_service): + self.cache.master_dir = None + self.cache.fetch_image(self.uuid, self.dest_path) + mock_fetch.assert_called_once_with(mock.ANY, self.uuid, self.dest_path, + False, mock.ANY, mock.ANY, mock.ANY, + disable_validation=True) + + @mock.patch.object(os, 'link', autospec=True) + @mock.patch.object(image_cache, '_fetch', autospec=True) + def test__download_image_iso(self, mock_fetch, mock_link, mock_clean_up, + mock_image_service): + self.cache._download_image(self.uuid, self.master_path, self.dest_path, + self.img_info) + mock_fetch.assert_called_once_with(mock.ANY, self.uuid, mock.ANY, + False, mock.ANY, mock.ANY, mock.ANY, + disable_validation=True) + + class RedfishImageHandlerTestCase(db_base.DbTestCase): def setUp(self): diff --git a/releasenotes/notes/disable_img_validation_iso-3d694a83576bf189.yaml b/releasenotes/notes/disable_img_validation_iso-3d694a83576bf189.yaml new file mode 100644 index 0000000000..2f62f7a5bb --- /dev/null +++ b/releasenotes/notes/disable_img_validation_iso-3d694a83576bf189.yaml @@ -0,0 +1,14 @@ +--- +fixes: + - | + The image format inspection and image safety check have been disabled + for ISO images cached by Ironic. The feature has been disabled in order + to fix issues originally described in bug 2091611. On occasion Ironic has + detected multiple image formats for ISO image that contained GPT, the + issue originated from the fact that by default the oslo_utils.imageutils + library handles the GPT partition table format as additional image format + but allows only 1 image format for each image and throws an error if it + detects gpt+iso. As the image inspection and safety check was intended to + fix a security problem related to qemu-img-tools and qcow images, it has + been decided that the inspection and safety check can be disabled ISO + images without degrading Ironic's security.