Skip to content

Commit

Permalink
Merge "Reject unsafe qcow and vmdk files"
Browse files Browse the repository at this point in the history
  • Loading branch information
@openstack-gerrit
Zuul authored and openstack-gerrit committed Jul 4, 2024
2 parents 68b155e + 797e30e commit cc7d53a
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 14 deletions.
44 changes: 37 additions & 7 deletions glance/async_/flows/plugins/image_conversion.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from taskflow import task

from glance.async_ import utils
from glance.common import format_inspector
from glance.i18n import _, _LI

LOG = logging.getLogger(__name__)
Expand Down Expand Up @@ -87,8 +88,40 @@ def _execute(self, action, file_path, **kwargs):
'target': target_format}
self.dest_path = dest_path

source_format = action.image_disk_format
inspector_cls = format_inspector.get_inspector(source_format)
if not inspector_cls:
# We cannot convert from disk_format types that qemu-img doesn't
# support (like iso, ploop, etc). The ones it supports overlaps
# with the ones we have inspectors for, so reject conversion for
# any format we don't have an inspector for.
raise RuntimeError(
'Unable to convert from format %s' % source_format)

# Use our own cautious inspector module (if we have one for this
# format) to make sure a file is the format the submitter claimed
# it is and that it passes some basic safety checks _before_ we run
# qemu-img on it.
# See https://bugs.launchpad.net/nova/+bug/2059809 for details.
try:
inspector = inspector_cls.from_file(src_path)
if not inspector.safety_check():
LOG.error('Image failed %s safety check; aborting conversion',
source_format)
raise RuntimeError('Image has disallowed configuration')
except RuntimeError:
raise
except format_inspector.ImageFormatError as e:
LOG.error('Image claimed to be %s format failed format '
'inspection: %s', source_format, e)
raise RuntimeError('Image format detection failed')
except Exception as e:
LOG.exception('Unknown error inspecting image format: %s', e)
raise RuntimeError('Unable to inspect image')

try:
stdout, stderr = putils.trycmd("qemu-img", "info",
"-f", source_format,
"--output=json",
src_path,
prlimit=utils.QEMU_IMG_PROC_LIMITS,
Expand All @@ -105,13 +138,10 @@ def _execute(self, action, file_path, **kwargs):
raise RuntimeError(stderr)

metadata = json.loads(stdout)
try:
source_format = metadata['format']
except KeyError:
msg = ("Failed to do introspection as part of image "
"conversion for %(iid)s: Source format not reported")
LOG.error(msg, {'iid': self.image_id})
raise RuntimeError(msg)
if metadata.get('format') != source_format:
LOG.error('Image claiming to be %s reported as %s by qemu-img',
source_format, metadata.get('format', 'unknown'))
raise RuntimeError('Image metadata disagrees about format')

virtual_size = metadata.get('virtual-size', 0)
action.set_image_attribute(virtual_size=virtual_size)
Expand Down
57 changes: 50 additions & 7 deletions glance/tests/unit/async_/flows/plugins/test_image_conversion.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# License for the specific language governing permissions and limitations
# under the License.

import fixtures
import json
import os
from unittest import mock
Expand All @@ -24,6 +25,7 @@
import glance.async_.flows.api_image_import as import_flow
import glance.async_.flows.plugins.image_conversion as image_conversion
from glance.async_ import utils as async_utils
from glance.common import format_inspector
from glance.common import utils
from glance import domain
from glance import gateway
Expand Down Expand Up @@ -90,6 +92,11 @@ def setUp(self):
self.image_id,
self.task.task_id)

self.inspector_mock = mock.MagicMock()
self.useFixture(fixtures.MockPatch('glance.common.format_inspector.'
'get_inspector',
self.inspector_mock))

@mock.patch.object(os, 'stat')
@mock.patch.object(os, 'remove')
def test_image_convert_success(self, mock_os_remove, mock_os_stat):
Expand All @@ -104,7 +111,7 @@ def test_image_convert_success(self, mock_os_remove, mock_os_stat):
image = mock.MagicMock(image_id=self.image_id, virtual_size=None,
extra_properties={
'os_glance_import_task': self.task.task_id},
disk_format='qcow2')
disk_format='raw')
self.img_repo.get.return_value = image

with mock.patch.object(processutils, 'execute') as exc_mock:
Expand All @@ -126,7 +133,7 @@ def test_image_convert_success(self, mock_os_remove, mock_os_stat):
self.assertEqual(456, image.virtual_size)
self.assertEqual(123, image.size)

def _setup_image_convert_info_fail(self):
def _setup_image_convert_info_fail(self, disk_format='qcow2'):
image_convert = image_conversion._ConvertImage(self.context,
self.task.task_id,
self.task_type,
Expand All @@ -136,7 +143,7 @@ def _setup_image_convert_info_fail(self):
image = mock.MagicMock(image_id=self.image_id, virtual_size=None,
extra_properties={
'os_glance_import_task': self.task.task_id},
disk_format='qcow2')
disk_format=disk_format)
self.img_repo.get.return_value = image
return image_convert

Expand All @@ -148,6 +155,7 @@ def test_image_convert_fails_inspection(self):
convert.execute, 'file:///test/path.raw')
exc_mock.assert_called_once_with(
'qemu-img', 'info',
'-f', 'qcow2',
'--output=json',
'/test/path.raw',
prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
Expand All @@ -164,6 +172,7 @@ def test_image_convert_inspection_reports_error(self):
convert.execute, 'file:///test/path.raw')
exc_mock.assert_called_once_with(
'qemu-img', 'info',
'-f', 'qcow2',
'--output=json',
'/test/path.raw',
prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
Expand Down Expand Up @@ -200,14 +209,44 @@ def test_image_convert_invalid_qcow_data_file(self):
self.assertEqual('QCOW images with data-file set are not allowed',
str(e))

def test_image_convert_no_inspector_match(self):
convert = self._setup_image_convert_info_fail()
self.inspector_mock.return_value = None
self.assertRaisesRegex(RuntimeError,
'Unable to convert from format',
convert.execute, 'file:///test/path.hpfs')

def test_image_convert_fails_inspection_safety_check(self):
convert = self._setup_image_convert_info_fail()
inspector = self.inspector_mock.return_value.from_file.return_value
inspector.safety_check.return_value = False
self.assertRaisesRegex(RuntimeError,
'Image has disallowed configuration',
convert.execute, 'file:///test/path.qcow')

def test_image_convert_fails_inspection_format_check(self):
convert = self._setup_image_convert_info_fail()
self.inspector_mock.return_value.from_file.side_effect = (
format_inspector.ImageFormatError())
self.assertRaisesRegex(RuntimeError,
'Image format detection failed',
convert.execute, 'file:///test/path.qcow')

def test_image_convert_fails_inspection_error(self):
convert = self._setup_image_convert_info_fail()
self.inspector_mock.return_value.from_file.side_effect = ValueError
self.assertRaisesRegex(RuntimeError,
'Unable to inspect image',
convert.execute, 'file:///test/path.qcow')

def _test_image_convert_invalid_vmdk(self):
data = {'format': 'vmdk',
'format-specific': {
'data': {
'create-type': 'monolithicFlat',
}}}

convert = self._setup_image_convert_info_fail()
convert = self._setup_image_convert_info_fail(disk_format='vmdk')
with mock.patch.object(processutils, 'execute') as exc_mock:
exc_mock.return_value = json.dumps(data), ''
convert.execute('file:///test/path.vmdk')
Expand Down Expand Up @@ -236,14 +275,15 @@ def test_image_convert_valid_vmdk(self):
self._test_image_convert_invalid_vmdk)

def test_image_convert_fails(self):
convert = self._setup_image_convert_info_fail()
convert = self._setup_image_convert_info_fail(disk_format='raw')
with mock.patch.object(processutils, 'execute') as exc_mock:
exc_mock.side_effect = [('{"format":"raw"}', ''),
OSError('convert_fail')]
self.assertRaises(OSError,
convert.execute, 'file:///test/path.raw')
exc_mock.assert_has_calls(
[mock.call('qemu-img', 'info',
'-f', 'raw',
'--output=json',
'/test/path.raw',
prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
Expand All @@ -256,14 +296,15 @@ def test_image_convert_fails(self):
self.img_repo.save.assert_not_called()

def test_image_convert_reports_fail(self):
convert = self._setup_image_convert_info_fail()
convert = self._setup_image_convert_info_fail(disk_format='raw')
with mock.patch.object(processutils, 'execute') as exc_mock:
exc_mock.side_effect = [('{"format":"raw"}', ''),
('', 'some error')]
self.assertRaises(RuntimeError,
convert.execute, 'file:///test/path.raw')
exc_mock.assert_has_calls(
[mock.call('qemu-img', 'info',
'-f', 'raw',
'--output=json',
'/test/path.raw',
prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
Expand All @@ -281,9 +322,10 @@ def test_image_convert_fails_source_format(self):
exc_mock.return_value = ('{}', '')
exc = self.assertRaises(RuntimeError,
convert.execute, 'file:///test/path.raw')
self.assertIn('Source format not reported', str(exc))
self.assertIn('Image metadata disagrees about format', str(exc))
exc_mock.assert_called_once_with(
'qemu-img', 'info',
'-f', 'qcow2',
'--output=json',
'/test/path.raw',
prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
Expand All @@ -301,6 +343,7 @@ def test_image_convert_same_format_does_nothing(self):
# Make sure we only called qemu-img for inspection, not conversion
exc_mock.assert_called_once_with(
'qemu-img', 'info',
'-f', 'qcow2',
'--output=json',
'/test/path.qcow',
prlimit=async_utils.QEMU_IMG_PROC_LIMITS,
Expand Down

0 comments on commit cc7d53a

Please sign in to comment.