From fbe429051e1fbcd494c71525870651e92e121449 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Wed, 10 Jul 2024 14:23:33 +0100 Subject: [PATCH] Change force_format strategy to catch mismatches When we moved the qemu-img command in fetch_to_raw() to force the format to what we expect, we lost the ability to identify and react to situations where qemu-img detected a file as a format that is not supported by us (i.e. identfied and safety-checked by format_inspector). In the case of some of the other VMDK variants that we don't support, we need to be sure to catch any case where qemu-img thinks it's something other than raw when we think it is, which will be the case for those formats we don't support. Note this also moves us from explicitly using the format_inspector that we're told by glance is appropriate, to using our own detection. We assert that we agree with glance and as above, qemu agrees with us. This helps us avoid cases where the uploader lies about the image format, causing us to not run the appropriate safety check. AMI formats are a liability here since we have a very hard time asserting what they are and what they will be detected as later in the pipeline, so there is still special-casing for those. Closes-Bug: #2071734 Change-Id: I4b792c5bc959a904854c21565682ed3a687baa1a (cherry picked from commit 8b4c522f6699514e7d1f20ac25cf426af6ea588f) (cherry picked from commit 8ef5ec9716c9edbd662ca27b6e39b7848b14f492) (cherry picked from commit 45d948938314997ba400a5fc2bb48bc821c260ab) --- nova/tests/unit/virt/libvirt/test_utils.py | 23 +++--- nova/tests/unit/virt/test_images.py | 96 +++++++++++++--------- nova/virt/images.py | 62 +++++++++----- 3 files changed, 108 insertions(+), 73 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index bc95e55cd41..6609db64323 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -443,12 +443,12 @@ def test_fetch_initrd_image(self, mock_images): _context, image_id, target, trusted_certs) @mock.patch.object(images, 'IMAGE_API') - @mock.patch.object(format_inspector, 'get_inspector') + @mock.patch.object(format_inspector, 'detect_file_format') @mock.patch.object(compute_utils, 'disk_ops_semaphore') @mock.patch('nova.privsep.utils.supports_direct_io', return_value=True) @mock.patch('nova.privsep.qemu.unprivileged_convert_image') def test_fetch_raw_image(self, mock_convert_image, mock_direct_io, - mock_disk_op_sema, mock_gi, mock_glance): + mock_disk_op_sema, mock_detect, mock_glance): def fake_rename(old, new): self.executes.append(('mv', old, new)) @@ -488,7 +488,7 @@ class FakeImgInfo(object): self.stub_out('oslo_utils.fileutils.delete_if_exists', fake_rm_on_error) - mock_inspector = mock_gi.return_value.from_file.return_value + mock_inspector = mock_detect.return_value # Since the remove param of fileutils.remove_path_on_error() # is initialized at load time, we must provide a wrapper @@ -502,6 +502,7 @@ class FakeImgInfo(object): # Make sure qcow2 gets converted to raw mock_inspector.safety_check.return_value = True + mock_inspector.__str__.return_value = 'qcow2' mock_glance.get.return_value = {'disk_format': 'qcow2'} target = 't.qcow2' self.executes = [] @@ -515,12 +516,13 @@ class FakeImgInfo(object): CONF.instances_path, False) mock_convert_image.reset_mock() mock_inspector.safety_check.assert_called_once_with() - mock_gi.assert_called_once_with('qcow2') + mock_detect.assert_called_once_with('t.qcow2.part') # Make sure raw does not get converted - mock_gi.reset_mock() + mock_detect.reset_mock() mock_inspector.safety_check.reset_mock() mock_inspector.safety_check.return_value = True + mock_inspector.__str__.return_value = 'raw' mock_glance.get.return_value = {'disk_format': 'raw'} target = 't.raw' self.executes = [] @@ -529,12 +531,13 @@ class FakeImgInfo(object): self.assertEqual(self.executes, expected_commands) mock_convert_image.assert_not_called() mock_inspector.safety_check.assert_called_once_with() - mock_gi.assert_called_once_with('raw') + mock_detect.assert_called_once_with('t.raw.part') # Make sure safety check failure prevents us from proceeding - mock_gi.reset_mock() + mock_detect.reset_mock() mock_inspector.safety_check.reset_mock() mock_inspector.safety_check.return_value = False + mock_inspector.__str__.return_value = 'qcow2' mock_glance.get.return_value = {'disk_format': 'qcow2'} target = 'backing.qcow2' self.executes = [] @@ -544,10 +547,10 @@ class FakeImgInfo(object): self.assertEqual(self.executes, expected_commands) mock_convert_image.assert_not_called() mock_inspector.safety_check.assert_called_once_with() - mock_gi.assert_called_once_with('qcow2') + mock_detect.assert_called_once_with('backing.qcow2.part') # Make sure a format mismatch prevents us from proceeding - mock_gi.reset_mock() + mock_detect.reset_mock() mock_inspector.safety_check.reset_mock() mock_inspector.safety_check.side_effect = ( format_inspector.ImageFormatError) @@ -560,7 +563,7 @@ class FakeImgInfo(object): self.assertEqual(self.executes, expected_commands) mock_convert_image.assert_not_called() mock_inspector.safety_check.assert_called_once_with() - mock_gi.assert_called_once_with('qcow2') + mock_detect.assert_called_once_with('backing.qcow2.part') del self.executes diff --git a/nova/tests/unit/virt/test_images.py b/nova/tests/unit/virt/test_images.py index cc285dc4fec..2e6a518cd70 100644 --- a/nova/tests/unit/virt/test_images.py +++ b/nova/tests/unit/virt/test_images.py @@ -21,7 +21,6 @@ from nova.compute import utils as compute_utils from nova import exception -from nova.image import format_inspector from nova import test from nova.virt import images @@ -101,15 +100,16 @@ def test_qemu_img_info_with_disk_not_found(self, exists, mocked_execute): mocked_execute.assert_called_once() @mock.patch.object(images, 'IMAGE_API') - @mock.patch('nova.image.format_inspector.get_inspector') + @mock.patch('nova.image.format_inspector.detect_file_format') @mock.patch.object(images, 'convert_image', side_effect=exception.ImageUnacceptable) @mock.patch.object(images, 'qemu_img_info') @mock.patch.object(images, 'fetch') def test_fetch_to_raw_errors(self, convert_image, qemu_img_info, fetch, - get_inspector, glance): - inspector = get_inspector.return_value.from_file.return_value + mock_detect, glance): + inspector = mock_detect.return_value inspector.safety_check.return_value = True + inspector.__str__.return_value = 'qcow2' glance.get.return_value = {'disk_format': 'qcow2'} qemu_img_info.backing_file = None qemu_img_info.file_format = 'qcow2' @@ -120,16 +120,17 @@ def test_fetch_to_raw_errors(self, convert_image, qemu_img_info, fetch, None, 'href123', '/no/path') @mock.patch.object(images, 'IMAGE_API') - @mock.patch('nova.image.format_inspector.get_inspector') + @mock.patch('nova.image.format_inspector.detect_file_format') @mock.patch.object(images, 'convert_image', side_effect=exception.ImageUnacceptable) @mock.patch.object(images, 'qemu_img_info') @mock.patch.object(images, 'fetch') def test_fetch_to_raw_data_file(self, convert_image, qemu_img_info_fn, - fetch, mock_gi, mock_glance): + fetch, mock_detect, mock_glance): mock_glance.get.return_value = {'disk_format': 'qcow2'} - inspector = mock_gi.return_value.from_file.return_value + inspector = mock_detect.return_value inspector.safety_check.return_value = True + inspector.__str__.return_value = 'qcow2' # NOTE(danms): the above test needs the following line as well, as it # is broken without it. qemu_img_info = qemu_img_info_fn.return_value @@ -142,16 +143,17 @@ def test_fetch_to_raw_data_file(self, convert_image, qemu_img_info_fn, images.fetch_to_raw, None, 'href123', '/no/path') - @mock.patch('nova.image.format_inspector.get_inspector') + @mock.patch('nova.image.format_inspector.detect_file_format') @mock.patch.object(images, 'IMAGE_API') @mock.patch('os.rename') @mock.patch.object(images, 'qemu_img_info') @mock.patch.object(images, 'fetch') def test_fetch_to_raw_from_raw(self, fetch, qemu_img_info_fn, mock_rename, - mock_glance, mock_gi): + mock_glance, mock_detect): # Make sure we support a case where we fetch an already-raw image and # qemu-img returns None for "format_specific". mock_glance.get.return_value = {'disk_format': 'raw'} + mock_detect.return_value.__str__.return_value = 'raw' qemu_img_info = qemu_img_info_fn.return_value qemu_img_info.file_format = 'raw' qemu_img_info.backing_file = None @@ -215,14 +217,15 @@ def test_convert_image_vmdk_allowed_list_checking(self): format='json')) @mock.patch.object(images, 'IMAGE_API') - @mock.patch('nova.image.format_inspector.get_inspector') + @mock.patch('nova.image.format_inspector.detect_file_format') @mock.patch.object(images, 'fetch') @mock.patch('nova.privsep.qemu.unprivileged_qemu_img_info') - def test_fetch_checks_vmdk_rules(self, mock_info, mock_fetch, mock_gi, + def test_fetch_checks_vmdk_rules(self, mock_info, mock_fetch, mock_detect, mock_glance): mock_glance.get.return_value = {'disk_format': 'vmdk'} - inspector = mock_gi.return_value.from_file.return_value + inspector = mock_detect.return_value inspector.safety_check.return_value = True + inspector.__str__.return_value = 'vmdk' info = {'format': 'vmdk', 'format-specific': { 'type': 'vmdk', @@ -238,13 +241,17 @@ def test_fetch_checks_vmdk_rules(self, mock_info, mock_fetch, mock_gi, @mock.patch('os.rename') @mock.patch.object(images, 'IMAGE_API') @mock.patch('nova.image.format_inspector.get_inspector') + @mock.patch('nova.image.format_inspector.detect_file_format') @mock.patch.object(images, 'fetch') @mock.patch('nova.privsep.qemu.unprivileged_qemu_img_info') - def test_fetch_iso_is_raw(self, mock_info, mock_fetch, mock_gi, - mock_glance, mock_rename): + def test_fetch_iso_is_raw( + self, mock_info, mock_fetch, mock_detect_file_format, mock_gi, + mock_glance, mock_rename): mock_glance.get.return_value = {'disk_format': 'iso'} inspector = mock_gi.return_value.from_file.return_value inspector.safety_check.return_value = True + inspector.__str__.return_value = 'iso' + mock_detect_file_format.return_value = inspector # qemu-img does not have a parser for iso so it is treated as raw info = { "virtual-size": 356352, @@ -258,27 +265,27 @@ def test_fetch_iso_is_raw(self, mock_info, mock_fetch, mock_gi, images.fetch_to_raw(None, 'foo', 'anypath') # Make sure we called info with -f raw for an iso, since qemu-img does # not support iso - mock_info.assert_called_once_with('anypath.part', format='raw') + mock_info.assert_called_once_with('anypath.part', format=None) # Make sure that since we considered this to be a raw file, we did the # just-rename-don't-convert path mock_rename.assert_called_once_with('anypath.part', 'anypath') @mock.patch.object(images, 'IMAGE_API') - @mock.patch('nova.image.format_inspector.get_inspector') + @mock.patch('nova.image.format_inspector.detect_file_format') @mock.patch.object(images, 'qemu_img_info') @mock.patch.object(images, 'fetch') - def test_fetch_to_raw_inspector(self, fetch, qemu_img_info, mock_gi, + def test_fetch_to_raw_inspector(self, fetch, qemu_img_info, mock_detect, mock_glance): # Image claims to be qcow2, is qcow2, but fails safety check, so we # abort before qemu-img-info mock_glance.get.return_value = {'disk_format': 'qcow2'} - inspector = mock_gi.return_value.from_file.return_value + inspector = mock_detect.return_value inspector.safety_check.return_value = False + inspector.__str__.return_value = 'qcow2' self.assertRaises(exception.ImageUnacceptable, images.fetch_to_raw, None, 'href123', '/no.path') qemu_img_info.assert_not_called() - mock_gi.assert_called_once_with('qcow2') - mock_gi.return_value.from_file.assert_called_once_with('/no.path.part') + mock_detect.assert_called_once_with('/no.path.part') inspector.safety_check.assert_called_once_with() mock_glance.get.assert_called_once_with(None, 'href123') @@ -292,18 +299,17 @@ def test_fetch_to_raw_inspector(self, fetch, qemu_img_info, mock_gi, # Image claims to be qcow2 in glance, but the image is something else, # so we abort before qemu-img-info qemu_img_info.reset_mock() - mock_gi.reset_mock() + mock_detect.reset_mock() inspector.safety_check.reset_mock() - mock_gi.return_value.from_file.side_effect = ( - format_inspector.ImageFormatError) + mock_detect.return_value.__str__.return_value = 'vmdk' self.assertRaises(exception.ImageUnacceptable, images.fetch_to_raw, None, 'href123', '/no.path') - mock_gi.assert_called_once_with('qcow2') - inspector.safety_check.assert_not_called() + mock_detect.assert_called_once_with('/no.path.part') + inspector.safety_check.assert_called_once_with() qemu_img_info.assert_not_called() @mock.patch.object(images, 'IMAGE_API') - @mock.patch('nova.image.format_inspector.get_inspector') + @mock.patch('nova.image.format_inspector.detect_file_format') @mock.patch.object(images, 'qemu_img_info') @mock.patch.object(images, 'fetch') def test_fetch_to_raw_inspector_disabled(self, fetch, qemu_img_info, @@ -316,36 +322,41 @@ def test_fetch_to_raw_inspector_disabled(self, fetch, qemu_img_info, # If deep inspection is disabled, we should never call the inspector mock_gi.assert_not_called() # ... and we let qemu-img detect the format itself. - qemu_img_info.assert_called_once_with('/no.path.part', - format=None) + qemu_img_info.assert_called_once_with('/no.path.part') mock_glance.get.assert_not_called() @mock.patch.object(images, 'IMAGE_API') @mock.patch.object(images, 'qemu_img_info') - def test_fetch_inspect_ami(self, imginfo, glance): + @mock.patch('nova.image.format_inspector.detect_file_format') + def test_fetch_inspect_ami(self, detect, imginfo, glance): glance.get.return_value = {'disk_format': 'ami'} + detect.return_value.__str__.return_value = 'raw' self.assertRaises(exception.ImageUnacceptable, images.fetch_to_raw, None, 'href123', '/no.path') # Make sure 'ami was translated into 'raw' before we call qemu-img - imginfo.assert_called_once_with('/no.path.part', format='raw') + imginfo.assert_called_once_with('/no.path.part') @mock.patch.object(images, 'IMAGE_API') @mock.patch.object(images, 'qemu_img_info') - def test_fetch_inspect_aki(self, imginfo, glance): + @mock.patch('nova.image.format_inspector.detect_file_format') + def test_fetch_inspect_aki(self, detect, imginfo, glance): glance.get.return_value = {'disk_format': 'aki'} + detect.return_value.__str__.return_value = 'raw' self.assertRaises(exception.ImageUnacceptable, images.fetch_to_raw, None, 'href123', '/no.path') # Make sure 'aki was translated into 'raw' before we call qemu-img - imginfo.assert_called_once_with('/no.path.part', format='raw') + imginfo.assert_called_once_with('/no.path.part') @mock.patch.object(images, 'IMAGE_API') @mock.patch.object(images, 'qemu_img_info') - def test_fetch_inspect_ari(self, imginfo, glance): + @mock.patch('nova.image.format_inspector.detect_file_format') + def test_fetch_inspect_ari(self, detect, imginfo, glance): glance.get.return_value = {'disk_format': 'ari'} + detect.return_value.__str__.return_value = 'raw' self.assertRaises(exception.ImageUnacceptable, images.fetch_to_raw, None, 'href123', '/no.path') # Make sure 'aki was translated into 'raw' before we call qemu-img - imginfo.assert_called_once_with('/no.path.part', format='raw') + imginfo.assert_called_once_with('/no.path.part') @mock.patch.object(images, 'IMAGE_API') @mock.patch.object(images, 'qemu_img_info') @@ -358,13 +369,16 @@ def test_fetch_inspect_unknown_format(self, imginfo, glance): @mock.patch.object(images, 'IMAGE_API') @mock.patch.object(images, 'qemu_img_info') - @mock.patch('nova.image.format_inspector.get_inspector') - def test_fetch_inspect_disagrees_qemu(self, mock_gi, imginfo, glance): + @mock.patch('nova.image.format_inspector.detect_file_format') + def test_fetch_inspect_disagrees_qemu(self, mock_detect, imginfo, glance): glance.get.return_value = {'disk_format': 'qcow2'} + mock_detect.return_value.__str__.return_value = 'qcow2' # Glance and inspector think it is a qcow2 file, but qemu-img does not - # agree. It was forced to interpret as a qcow2, but returned no - # format information as a result. + # agree. imginfo.return_value.data_file = None - self.assertRaises(exception.ImageUnacceptable, - images.fetch_to_raw, None, 'href123', '/no.path') - imginfo.assert_called_once_with('/no.path.part', format='qcow2') + imginfo.return_value.file_format = 'vmdk' + ex = self.assertRaises(exception.ImageUnacceptable, + images.fetch_to_raw, + None, 'href123', '/no.path') + self.assertIn('content does not match disk_format', str(ex)) + imginfo.assert_called_once_with('/no.path.part') diff --git a/nova/virt/images.py b/nova/virt/images.py index 813696ed7d7..193c80fb636 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -140,42 +140,50 @@ def check_vmdk_image(image_id, data): def do_image_deep_inspection(img, image_href, path): + ami_formats = ('ami', 'aki', 'ari') disk_format = img['disk_format'] try: # NOTE(danms): Use our own cautious inspector module to make sure # the image file passes safety checks. # See https://bugs.launchpad.net/nova/+bug/2059809 for details. - inspector_cls = format_inspector.get_inspector(disk_format) - if not inspector_cls.from_file(path).safety_check(): + + # Make sure we have a format inspector for the claimed format, else + # it is something we do not support and must reject. AMI is excluded. + if (disk_format not in ami_formats and + not format_inspector.get_inspector(disk_format)): + raise exception.ImageUnacceptable( + image_id=image_href, + reason=_('Image not in a supported format')) + + inspector = format_inspector.detect_file_format(path) + if not inspector.safety_check(): raise exception.ImageUnacceptable( image_id=image_href, reason=(_('Image does not pass safety check'))) + + # AMI formats can be other things, so don't obsess over this + # requirement for them. Otherwise, make sure our detection agrees + # with glance. + if disk_format not in ami_formats and str(inspector) != disk_format: + # If we detected the image as something other than glance claimed, + # we abort. + raise exception.ImageUnacceptable( + image_id=image_href, + reason=_('Image content does not match disk_format')) except format_inspector.ImageFormatError: # If the inspector we chose based on the image's metadata does not # think the image is the proper format, we refuse to use it. raise exception.ImageUnacceptable( image_id=image_href, reason=_('Image content does not match disk_format')) - except AttributeError: - # No inspector was found - LOG.warning('Unable to perform deep image inspection on type %r', - img['disk_format']) - if disk_format in ('ami', 'aki', 'ari'): - # A lot of things can be in a UEC, although it is typically a raw - # filesystem. We really have nothing we can do other than treat it - # like a 'raw', which is what qemu-img will detect a filesystem as - # anyway. If someone puts a qcow2 inside, we should fail because - # we won't do our inspection. - disk_format = 'raw' - else: - raise exception.ImageUnacceptable( - image_id=image_href, - reason=_('Image not in a supported format')) - - if disk_format == 'iso': - # ISO image passed safety check; qemu will treat this as raw from here + except Exception: + raise exception.ImageUnacceptable( + image_id=image_href, + reason=_('Image not in a supported format')) + if disk_format in ('iso',) + ami_formats: + # ISO or AMI image passed safety check; qemu will treat this as raw + # from here so return the expected formats it will find. disk_format = 'raw' - return disk_format @@ -194,12 +202,22 @@ def fetch_to_raw(context, image_href, path, trusted_certs=None): # Only run qemu-img after we have done deep inspection (if enabled). # If it was not enabled, we will let it detect the format. - data = qemu_img_info(path_tmp, format=force_format) + data = qemu_img_info(path_tmp) fmt = data.file_format if fmt is None: raise exception.ImageUnacceptable( reason=_("'qemu-img info' parsing failed."), image_id=image_href) + elif force_format is not None and fmt != force_format: + # Format inspector and qemu-img must agree on the format, else + # we reject. This will catch VMDK some variants that we don't + # explicitly support because qemu will identify them as such + # and we will not. + LOG.warning('Image %s detected by qemu as %s but we expected %s', + image_href, fmt, force_format) + raise exception.ImageUnacceptable( + image_id=image_href, + reason=_('Image content does not match disk_format')) backing_file = data.backing_file if backing_file is not None: