Merge "disable ISO cache image format and safety checks"

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,
+                        ctx=None, force_raw=None, expected_format=None,
-                        expected_checksum=None,
+                        expected_checksum=None, expected_checksum_algo=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,
+def _fetch(context, image_href, path, force_raw=False,
-           expected_checksum=None, expected_checksum_algo=None,
+           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)

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=''):

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):

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.