From 4c9a7c80cf26b65fa6d00bb2cad7a9f411bfabed Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Wed, 10 Jul 2024 14:23:33 +0100 Subject: [PATCH 1/3] 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) --- nova/tests/unit/virt/libvirt/test_utils.py | 23 ++-- nova/tests/unit/virt/test_images.py | 116 ++++++++++++++------- nova/virt/images.py | 59 +++++++---- 3 files changed, 133 insertions(+), 65 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/test_utils.py b/nova/tests/unit/virt/libvirt/test_utils.py index 26de82790d9..5bad7d2a779 100644 --- a/nova/tests/unit/virt/libvirt/test_utils.py +++ b/nova/tests/unit/virt/libvirt/test_utils.py @@ -405,12 +405,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)) @@ -450,7 +450,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 @@ -464,6 +464,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 = [] @@ -477,12 +478,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 = [] @@ -491,12 +493,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 = [] @@ -506,10 +509,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) @@ -522,7 +525,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 46c9f9a8b5d..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', @@ -235,22 +238,54 @@ def test_fetch_checks_vmdk_rules(self, mock_info, mock_fetch, mock_gi, images.fetch_to_raw, None, 'foo', 'anypath') self.assertIn('Invalid VMDK create-type specified', str(e)) + @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_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, + "filename": "foo.iso", + "format": "raw", + "actual-size": 356352, + "dirty-flag": False + } + mock_info.return_value = jsonutils.dumps(info) + with mock.patch('os.path.exists', return_value=True): + 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=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.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') @@ -264,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, @@ -288,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') @@ -330,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 5ec0dc0b6ba..193c80fb636 100644 --- a/nova/virt/images.py +++ b/nova/virt/images.py @@ -140,37 +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')) + 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 @@ -189,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: From dccdd0b58d210287544ed55b93ec99accf58160e Mon Sep 17 00:00:00 2001 From: Pierre Riteau Date: Tue, 23 Jul 2024 18:17:41 +0200 Subject: [PATCH 2/3] CI: Install qemu-img for unit tests Change-Id: I3b38344a127764b4fa62fe062e825074d6deab60 (cherry picked from commit 416ac36b8dcbeb3d65a6271d4ff3696e379c0ec7) --- bindep.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bindep.txt b/bindep.txt index b129058d301..8bf1eaf0d6c 100644 --- a/bindep.txt +++ b/bindep.txt @@ -54,3 +54,5 @@ pcre-devel [platform:rpm test] # runtime and in unit tests. Net result is the same that lsscsi will be # installed for any nova installation. lsscsi +# NOTE(priteau): Install qemu-img for format inspector tests +qemu-utils [platform:dpkg test] From a6a573bb33332ec5947d0f0a1f15994cc7a8abc4 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 15 Dec 2022 00:09:04 +0000 Subject: [PATCH 3/3] Remove use of removeprefix This is not supported on Python 3.8 [1]. I have no idea why this was not failing CI. [1] https://docs.python.org/3.9/library/stdtypes.html#str.removeprefix Change-Id: I225e9ced0f75c415b1d2fee05440291e3d8635c0 Signed-off-by: Stephen Finucane (cherry picked from commit 3ccf82ef9e2c87a1d33a0dda8929c05e80844087) --- nova/tests/unit/console/test_websocketproxy.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nova/tests/unit/console/test_websocketproxy.py b/nova/tests/unit/console/test_websocketproxy.py index fc25bef2bc3..639623bbb58 100644 --- a/nova/tests/unit/console/test_websocketproxy.py +++ b/nova/tests/unit/console/test_websocketproxy.py @@ -635,7 +635,9 @@ def test_reject_open_redirect(self, url='//example.com/%2F..'): # now the same url but with extra leading '/' characters removed. if expected_cpython in errmsg: location = result[3].decode() - location = location.removeprefix('Location: ').rstrip('\r\n') + if location.startswith('Location: '): + location = location[len('Location: '):] + location = location.rstrip('\r\n') self.assertTrue( location.startswith('/example.com/%2F..'), msg='Redirect location is not the expected sanitized URL',