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.