Make glance able to require disk_format match

This adds a config knob to enable strict format compliance when
uploading image data. If enabled, uploads will be rejected for formats
we can inspect if the content does not match. For formats we cannot
match (other than raw), we will reject content if we _do_ recognize
the format.

Related to blueprint glance-as-defender
Depends-On: https://review.opendev.org/c/openstack/tempest/+/931028
Depends-On: https://review.opendev.org/c/openstack/glance-tempest-plugin/+/931287
Depends-On: https://review.opendev.org/c/openstack/nova/+/930754
Change-Id: I2d0301d168d2b55bf38351bf5c6d780e95a00401

glance/api/v2/image_data.py

@@ -270,6 +270,12 @@ class ImageDataController(object):
             raise webob.exc.HTTPRequestEntityTooLarge(explanation=str(e),
                                                       request=req)
 
+        except exception.InvalidImageData as e:
+            LOG.error(str(e))
+            self._restore(image_repo, image)
+            raise webob.exc.HTTPUnsupportedMediaType(explanation=str(e),
+                                                     request=req)
+
         except glance_store.StorageWriteDenied as e:
             msg = _("Insufficient permissions on image "
                     "storage media: %s") % encodeutils.exception_to_unicode(e)

glance/common/config.py

@@ -110,6 +110,17 @@ image_format_opts = [
                        "checked during image conversion (if enabled), and "
                        "limits the types of VMDK images we will convert "
                        "from.")),
+    cfg.BoolOpt('require_image_format_match',
+                default=True,
+                help=_('When enabled, glance will inspect the content of '
+                       'uploads and require that they match the disk_format '
+                       'set on the image. A mismatch will abort the upload '
+                       'with an error. Disabling this may be required in the '
+                       'case of false match (or mismatch) issues, but those '
+                       'are bugs that should be filed and fixed. Enabling '
+                       'this feature improves security and consistency by '
+                       'ensuring that images claiming to be a given format '
+                       'have content matching that format.')),
 ]
 task_opts = [
     cfg.IntOpt('task_time_to_live',

glance/common/exception.py

@@ -458,3 +458,7 @@ class InvalidDataMigrationScript(GlanceException):
 class GlanceEndpointNotFound(NotFound):
     message = _("%(interface)s glance endpoint not "
                 "found for region %(region)s")
+
+
+class InvalidImageData(Invalid):
+    message = _("Image data is not acceptable or does not match parameters")

glance/location.py

@@ -584,48 +584,70 @@ class ImageProxy(glance.domain.proxy.Image):
             # FIXME(danms): We do not pass an expected_format here because
             # we do not (currently) want to interrupt the data pipeline if
             # the format does not match.
-            data = format_inspector.InspectWrapper(data)
             LOG.debug('Enabling in-flight format inspection for %s',
                       self.image.disk_format)
-
-        self._upload_to_store(data, verifier, backend, size)
+            data = format_inspector.InspectWrapper(data)
+        else:
+            # FIXME(danms): Either warn here if we are unable to inspect non-
+            # bare images or maybe make this a hard fail
+            pass
 
         try:
-            data.close()
-        except AttributeError:
-            # We did not get a closeable or file-like object as data and/or
-            # did not wrap it because of container_format
-            pass
+            self._upload_to_store(data, verifier, backend, size)
+        except format_inspector.ImageFormatError as e:
+            raise exception.InvalidImageData(str(e))
+        finally:
+            try:
+                data.close()
+            except AttributeError:
+                # We did not get a closeable or file-like object as data and/or
+                # did not wrap it because of container_format
+                pass
 
         virtual_size = 0
         try:
             inspector = data.format
             format = str(inspector)
-            # format_inspector detects GPT, which we need to treat as raw for
-            # compatibility reasons
-            if format == 'gpt':
-                format = 'raw'
             if format == self.image.disk_format:
                 virtual_size = inspector.virtual_size
                 LOG.info('Image format matched and virtual size computed: %i',
                          virtual_size)
+            elif (format == 'raw' and
+                  self.image.disk_format not in format_inspector.ALL_FORMATS):
+                # Things like AMI are not inspectable, so as long as we did not
+                # recognize the format of the payload, allow it through.
+                LOG.info('Image format %s is not an inspectable type',
+                         self.image.disk_format)
+            elif self.image.disk_format == 'raw':
+                # This is something we can't inspect, so we allow it
+                LOG.warning('Image claims %r disk_format but is actually %r',
+                            data.format, format)
+                # If we are pretending this is really raw, don't use the
+                # detected rich format's virtual_size, but just use the
+                # actual size in bytes, as if we did not recognize the format.
+                virtual_size = inspector.actual_size
             else:
-                LOG.info('Image declared as %s but detected as %s, '
-                         'not updating virtual_size',
-                         self.image.disk_format, inspector)
+                raise format_inspector.ImageFormatError(
+                    _('Image disk_format %s does not match stream data %s' % (
+                        self.image.disk_format, format)))
         except AttributeError:
             # We must not have wrapped this for inspection because of
             # container_format
             pass
-        except format_inspector.ImageFormatError:
+        except format_inspector.ImageFormatError as e:
             # No format matched!
-            # FIXME(danms): This should be an error in the future for most
-            # cases
-            LOG.warning('Image format %s did not match; '
-                        'unable to calculate virtual size',
-                        self.image.disk_format)
+            if CONF.image_format.require_image_format_match:
+                raise exception.InvalidImageData(str(e))
+            else:
+                LOG.warning('Image format %s did not match; '
+                            'unable to calculate virtual size',
+                            self.image.disk_format)
         except Exception as e:
-            LOG.error(_LE('Unable to determine virtual_size because: %s'), e)
+            LOG.error(_LE('Unable to determine stream format because: %s'), e)
+            # FIXME(danms): Do this based on config
+            if CONF.image_format.require_image_format_match:
+                raise exception.InvalidImageData(
+                    _('Image format inspection failed'))
 
         if virtual_size:
             self.image.virtual_size = virtual_size

glance/tests/unit/async_/flows/test_convert.py

@@ -158,7 +158,7 @@ class TestImportTask(test_utils.BaseTestCase):
                     "virtual-size": 10737418240,
                     "filename": "/tmp/image.qcow2",
                     "cluster-size": 65536,
-                    "format": "qcow2",
+                    "format": "raw",
                     "actual-size": 373030912,
                     "format-specific": {
                         "type": "qcow2",
@@ -186,7 +186,7 @@ class TestImportTask(test_utils.BaseTestCase):
                 # NOTE(flaper87): Workdir should be empty after all
                 # the tasks have been executed.
                 self.assertEqual([], os.listdir(self.work_dir))
-                self.assertEqual('qcow2', image.disk_format)
+                self.assertEqual('raw', image.disk_format)
                 self.assertEqual(10737418240, image.virtual_size)
 
                 # NOTE(hemanthm): Asserting that the source format is passed

glance/tests/unit/async_/flows/test_import.py

@@ -74,7 +74,7 @@ class TestImportTask(test_utils.BaseTestCase):
         self.task_factory = domain.TaskFactory()
         self.img_factory = self.gateway.get_image_factory(self.context)
         self.image = self.img_factory.new_image(image_id=UUID1,
-                                                disk_format='qcow2',
+                                                disk_format='raw',
                                                 container_format='bare')
 
         task_input = {
@@ -127,7 +127,7 @@ class TestImportTask(test_utils.BaseTestCase):
 
             with mock.patch.object(putils, 'trycmd') as tmock:
                 tmock.return_value = (json.dumps({
-                    'format': 'qcow2',
+                    'format': 'raw',
                 }), None)
 
                 executor.begin_processing(self.task.task_id)

glance/tests/unit/test_store_image.py

@@ -18,6 +18,8 @@ from cryptography import exceptions as crypto_exception
 from cursive import exception as cursive_exception
 from cursive import signature_utils
 import glance_store
+from oslo_config import cfg
+import struct
 from unittest import mock
 
 from glance.common import exception
@@ -34,6 +36,7 @@ USER1 = '54492ba0-f4df-4e4e-be62-27f4d76b29cf'
 TENANT1 = '6838eb7b-6ded-434a-882c-b344c77fe8df'
 TENANT2 = '2c014f32-55eb-467d-8fcb-4bd706012f81'
 TENANT3 = '228c6da5-29cd-4d67-9457-ed632e083fc0'
+CONF = cfg.CONF
 
 
 class ImageRepoStub(object):
@@ -116,7 +119,7 @@ class TestStoreMultiBackends(utils.BaseTestCase):
         image_stub = ImageStub(UUID2, status='queued', locations=[],
                                extra_properties=extra_properties,
                                virtual_size=4)
-        image_stub.disk_format = 'iso'
+        image_stub.disk_format = 'raw'
         image = glance.location.ImageProxy(image_stub, context,
                                            self.store_api, self.store_utils)
         found_data = []
@@ -285,6 +288,27 @@ class TestStoreImage(utils.BaseTestCase):
         # We are going to pass an iterable data source, so use the
         # FakeStoreAPIReader that actually reads from that data
         store_api = unit_test_utils.FakeStoreAPIReader()
+        image = glance.location.ImageProxy(image_stub, context,
+                                           store_api, self.store_utils)
+        if CONF.image_format.require_image_format_match:
+            self.assertRaises(exception.InvalidImageData,
+                              image.set_data, iter(['YYYY']), 4)
+        else:
+            image.set_data(iter(['YYYY']), 4)
+            self.assertEqual(4, image.size)
+            # NOTE(markwash): FakeStore returns image_id for location
+            self.assertEqual(UUID2, image.locations[0]['url'])
+            self.assertEqual('Z', image.checksum)
+            self.assertEqual('active', image.status)
+            self.assertEqual(0, image.virtual_size)
+
+    def test_image_set_data_inspector_unsupported_format(self):
+        context = glance.context.RequestContext(user=USER1)
+        image_stub = ImageStub(UUID2, status='queued', locations=[])
+        image_stub.disk_format = 'ami'
+        # We are going to pass an iterable data source, so use the
+        # FakeStoreAPIReader that actually reads from that data
+        store_api = unit_test_utils.FakeStoreAPIReader()
         image = glance.location.ImageProxy(image_stub, context,
                                            store_api, self.store_utils)
         image.set_data(iter(['YYYY']), 4)
@@ -295,6 +319,67 @@ class TestStoreImage(utils.BaseTestCase):
         self.assertEqual('active', image.status)
         self.assertEqual(0, image.virtual_size)
 
+    def _get_stub_gpt(self):
+        """Returns a valid, safe GPT-based image"""
+        data = bytearray(b'\x00' * 1024)
+        # The last two bytes of the first sector is the little-endian signature
+        # value 0xAA55
+        data[510:512] = b'\x55\xAA'
+
+        # This is one EFI Protective MBR partition in the first PTE slot,
+        # which is 16 bytes starting at offset 446.
+        data[446:446 + 16] = struct.pack('<BBBBBBBBII',
+                                         0x00,  # boot
+                                         0x00,  # start C
+                                         0x02,  # start H
+                                         0x00,  # start S
+                                         0xEE,  # OS type
+                                         0x00,  # end C
+                                         0x00,  # end H
+                                         0x00,  # end S
+                                         0x01,  # start LBA
+                                         0x00,  # size LBA
+                                         )
+        return data
+
+    def test_image_set_data_inspector_gpt_as_raw_compat(self):
+        context = glance.context.RequestContext(user=USER1)
+        image_stub = ImageStub(UUID2, status='queued', locations=[])
+        image_stub.disk_format = 'raw'
+        # We are going to pass an iterable data source, so use the
+        # FakeStoreAPIReader that actually reads from that data
+
+        store_api = unit_test_utils.FakeStoreAPIReader(max_size=2048)
+        image = glance.location.ImageProxy(image_stub, context,
+                                           store_api, self.store_utils)
+        data = self._get_stub_gpt()
+        image.set_data(iter((data,)), 1024)
+        self.assertEqual(1024, image.size)
+        # NOTE(markwash): FakeStore returns image_id for location
+        self.assertEqual(UUID2, image.locations[0]['url'])
+        self.assertEqual('Z', image.checksum)
+        self.assertEqual('active', image.status)
+        self.assertEqual(1024, image.virtual_size)
+
+    def test_image_set_data_inspector_format_unsupported_hiding(self):
+        context = glance.context.RequestContext(user=USER1)
+        image_stub = ImageStub(UUID2, status='queued', locations=[])
+        image_stub.disk_format = 'ami'
+        # We are going to pass an iterable data source, so use the
+        # FakeStoreAPIReader that actually reads from that data
+        data = self._get_stub_gpt()
+        store_api = unit_test_utils.FakeStoreAPIReader(max_size=2048)
+        image = glance.location.ImageProxy(image_stub, context,
+                                           store_api, self.store_utils)
+        # Image was claimed to be AMI, which we cannot inspect, but we found
+        # a known format inside, which is invalid
+        self.assertRaises(exception.InvalidImageData,
+                          image.set_data, iter((data,)), 1024)
+
+    def test_image_set_data_inspector_no_match_disabled(self):
+        self.config(require_image_format_match=False, group='image_format')
+        self.test_image_set_data_inspector_no_match()
+
     @mock.patch('oslo_utils.imageutils.format_inspector.QcowInspector.'
                 'virtual_size',
                 new_callable=mock.PropertyMock)
@@ -318,15 +403,23 @@ class TestStoreImage(utils.BaseTestCase):
         image = glance.location.ImageProxy(image_stub, context,
                                            store_api, self.store_utils)
 
-        # Make sure set_data proceeds even though the format clearly
-        # does not match
-        image.set_data(iter(['YYYY']), 4)
-        self.assertEqual(4, image.size)
-        # NOTE(markwash): FakeStore returns image_id for location
-        self.assertEqual(UUID2, image.locations[0]['url'])
-        self.assertEqual('Z', image.checksum)
-        self.assertEqual('active', image.status)
-        self.assertEqual(0, image.virtual_size)
+        if CONF.image_format.require_image_format_match:
+            self.assertRaises(exception.InvalidImageData,
+                              image.set_data, iter(['YYYY']), 4)
+        else:
+            # Make sure set_data proceeds even though the format clearly
+            # does not match
+            image.set_data(iter(['YYYY']), 4)
+            self.assertEqual(4, image.size)
+            # NOTE(markwash): FakeStore returns image_id for location
+            self.assertEqual(UUID2, image.locations[0]['url'])
+            self.assertEqual('Z', image.checksum)
+            self.assertEqual('active', image.status)
+            self.assertEqual(0, image.virtual_size)
+
+    def test_image_set_data_inspector_virtual_size_failure_disabled(self):
+        self.config(require_image_format_match=False, group='image_format')
+        self.test_image_set_data_inspector_virtual_size_failure()
 
     @mock.patch('oslo_utils.imageutils.format_inspector.InspectWrapper')
     def test_image_set_data_inspector_not_needed(self, mock_inspect):
@@ -378,17 +471,26 @@ class TestStoreImage(utils.BaseTestCase):
         image_stub.disk_format = 'iso'
         image = glance.location.ImageProxy(image_stub, context,
                                            self.store_api, self.store_utils)
-        image.set_data('YYYY', None)
-        self.assertEqual(4, image.size)
-        # NOTE(markwash): FakeStore returns image_id for location
-        self.assertEqual(UUID2, image.locations[0]['url'])
-        self.assertEqual('Z', image.checksum)
-        self.assertEqual('active', image.status)
-        image.delete()
-        self.assertEqual(image.status, 'deleted')
-        self.assertRaises(glance_store.NotFound,
-                          self.store_api.get_from_backend,
-                          image.locations[0]['url'], context={})
+
+        if CONF.image_format.require_image_format_match:
+            self.assertRaises(exception.InvalidImageData,
+                              image.set_data, 'YYYY', None)
+        else:
+            image.set_data('YYYY', None)
+            self.assertEqual(4, image.size)
+            # NOTE(markwash): FakeStore returns image_id for location
+            self.assertEqual(UUID2, image.locations[0]['url'])
+            self.assertEqual('Z', image.checksum)
+            self.assertEqual('active', image.status)
+            image.delete()
+            self.assertEqual(image.status, 'deleted')
+            self.assertRaises(glance_store.NotFound,
+                              self.store_api.get_from_backend,
+                              image.locations[0]['url'], context={})
+
+    def test_image_set_data_unknown_size_disabled(self):
+        self.config(require_image_format_match=False, group='image_format')
+        self.test_image_set_data_unknown_size()
 
     @mock.patch('glance.location.LOG')
     def test_image_set_data_valid_signature(self, mock_log):

glance/tests/unit/utils.py

@@ -191,7 +191,8 @@ class FakeStoreUtils(object):
 
 
 class FakeStoreAPI(object):
-    def __init__(self, store_metadata=None):
+    def __init__(self, store_metadata=None, max_size=7):
+        self._max_size = max_size
         self.data = {
             '%s/%s' % (BASE_URI, UUID1): ('XXX', 3),
             '%s/fake_location' % (BASE_URI): ('YYY', 3)
@@ -233,7 +234,7 @@ class FakeStoreAPI(object):
 
     def add_to_backend(self, conf, image_id, data, size,
                        scheme=None, context=None, verifier=None):
-        store_max_size = 7
+        store_max_size = self._max_size
         current_store_size = 2
         for location in self.data.keys():
             if image_id in location:
@@ -256,7 +257,7 @@ class FakeStoreAPI(object):
     def add_to_backend_with_multihash(
             self, conf, image_id, data, size, hashing_algo,
             scheme=None, context=None, verifier=None):
-        store_max_size = 7
+        store_max_size = self._max_size
         current_store_size = 2
         for location in self.data.keys():
             if image_id in location: