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
This commit is contained in:
Dan Smith 2024-07-10 14:23:33 +01:00 committed by Sean Mooney
parent 8c4a2e1b47
commit 8b4c522f66
3 changed files with 108 additions and 73 deletions

View File

@ -443,12 +443,12 @@ class LibvirtUtilsTestCase(test.NoDBTestCase):
_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 LibvirtUtilsTestCase(test.NoDBTestCase):
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 LibvirtUtilsTestCase(test.NoDBTestCase):
# 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 = []
@ -516,12 +517,13 @@ class LibvirtUtilsTestCase(test.NoDBTestCase):
dest_encryption=None)
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 = []
@ -530,12 +532,13 @@ class LibvirtUtilsTestCase(test.NoDBTestCase):
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 = []
@ -545,10 +548,10 @@ class LibvirtUtilsTestCase(test.NoDBTestCase):
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)
@ -561,7 +564,7 @@ class LibvirtUtilsTestCase(test.NoDBTestCase):
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

View File

@ -21,7 +21,6 @@ from oslo_utils import imageutils
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 @@ class QemuTestCase(test.NoDBTestCase):
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 @@ class QemuTestCase(test.NoDBTestCase):
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 @@ class QemuTestCase(test.NoDBTestCase):
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 @@ class QemuTestCase(test.NoDBTestCase):
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 @@ class QemuTestCase(test.NoDBTestCase):
@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,
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 @@ class QemuTestCase(test.NoDBTestCase):
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 @@ class QemuTestCase(test.NoDBTestCase):
# 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 @@ class QemuTestCase(test.NoDBTestCase):
# 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 @@ class QemuTestCase(test.NoDBTestCase):
@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')

View File

@ -143,42 +143,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:
except Exception:
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
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
@ -197,12 +205,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: