Merge lp:~smoser/nova/lp853330 into lp:~hudson-openstack/nova/trunk

[Scott Moser]

convert images that are not 'raw' to 'raw' during caching to node

convert images that are not 'raw' to 'raw' during caching to node

This uses 'qemu-img' to convert images that are not 'raw' to be 'raw'.
By doing so, it
 a.) refuses to run uploaded images that have a backing image reference
     (LP: #853330, CVE-2011-3147)
 b.) ensures that when FLAGS.use_cow_images is False, and the libvirt
     xml written specifies 'driver_type="raw"' that the disk referenced
     is also raw format. (LP: #837102)
 c.) removes compression that might be present to avoid cpu bottlenecks
     (LP: #837100)

It does have the negative side affect of using more space in the case where
the user uploaded a qcow2 (or other advanced image format) that could have
been used directly by the hypervisor. That could, later, be remedied by
another 'qemu-img convert' being done to the "preferred" format of the
hypervisor.

[Vish Ishaya (vishvananda)]

lgtm

review: Approve

[Soren Hansen (soren)]

Thanks so much for highlighting and addressing this bug.

A couple of issues:

 * qemu-img's output may be translated. We need to make sure LANG and LC_* either are set to "C" or not set at all, otherwise parsing its output might fail.

 * (as you pointed out yourself to me on IRC:) You don't clean up after yourself if you raise the ImageUnacceptable exception.

 * Calling the final two arguments to fetch_to_raw "_user_id" and "_project_id" suggests they're not used, while in fact they're passed on to fetch (which may or may not ignore them).

review: Needs Fixing

[lp:~smoser/nova/lp853330]

1601. By Scott Moser on 2011-09-20

Address Soren's comments:
 * clean up temp files if an ImageUnacceptable is going to be raised
   Note, a qemu-img execution error will not clean up the image, but I
   think thats reasonable. We leave the image on disk so the user can
   easily investigate.
 * Change final 2 arguments to fetch_to_raw to not start with an _
 * use 'env' utility to change environment variables LC_ALL and LANG so
   that qemu-img output parsing is not locale dependent.
   Note, I considered the following, but found using 'env' more readable
     out, err = utils.execute('sh', '-c', 'export LC_ALL=C LANG=C && exec "$@"',
         'qemu-img', 'info', path)

[Soren Hansen (soren)]

Thanks.

I'd prefer extending nova.utils.execute() to accept an environment dict, but this is fine for now. Thanks again!

review: Approve

[Scott Moser (smoser)]

On Tue, 20 Sep 2011, Soren Hansen wrote:

> I'd prefer extending nova.utils.execute() to accept an environment dict,
> but this is fine for now. Thanks again!

I agree with that. but did not want to do it for milestone.

=== modified file 'nova/virt/images.py'
--- nova/virt/images.py	2011-09-10 17:56:54 +0000
+++ nova/virt/images.py	2011-09-20 09:48:25 +0000
@@ -21,6 +21,9 @@
 Handling of VM disk images.
 """
 
+import os
+
+from nova import exception
 from nova import flags
 from nova.image import glance as glance_image_service
 import nova.image
@@ -42,3 +45,59 @@
     with open(path, "wb") as image_file:
         metadata = image_service.get(context, image_id, image_file)
     return metadata
+
+
+def fetch_to_raw(context, image_href, path, user_id, project_id):
+    path_tmp = "%s.part" % path
+    metadata = fetch(context, image_href, path_tmp, user_id, project_id)
+
+    def _qemu_img_info(path):
+
+        out, err = utils.execute('env', 'LC_ALL=C', 'LANG=C',
+            'qemu-img', 'info', path)
+
+        # output of qemu-img is 'field: value'
+        # the fields of interest are 'file format' and 'backing file'
+        data = {}
+        for line in out.splitlines():
+            (field, val) = line.split(':', 1)
+            if val[0] == " ":
+                val = val[1:]
+            data[field] = val
+
+        return(data)
+
+    data = _qemu_img_info(path_tmp)
+
+    fmt = data.get("file format", None)
+    if fmt == None:
+        os.unlink(path_tmp)
+        raise exception.ImageUnacceptable(
+            reason=_("'qemu-img info' parsing failed."), image_id=image_href)
+
+    if fmt != "raw":
+        staged = "%s.converted" % path
+        if "backing file" in data:
+            backing_file = data['backing file']
+            os.unlink(path_tmp)
+            raise exception.ImageUnacceptable(image_id=image_href,
+                reason=_("fmt=%(fmt)s backed by: %(backing_file)s") % locals())
+
+        LOG.debug("%s was %s, converting to raw" % (image_href, fmt))
+        out, err = utils.execute('qemu-img', 'convert', '-O', 'raw',
+                                 path_tmp, staged)
+        os.unlink(path_tmp)
+
+        data = _qemu_img_info(staged)
+        if data.get('file format', None) != "raw":
+            os.unlink(staged)
+            raise exception.ImageUnacceptable(image_id=image_href,
+                reason=_("Converted to raw, but format is now %s") %
+                data.get('file format', None))
+
+        os.rename(staged, path)
+
+    else:
+        os.rename(path_tmp, path)
+
+    return metadata

=== modified file 'nova/virt/libvirt/connection.py'
--- nova/virt/libvirt/connection.py	2011-09-20 08:41:51 +0000
+++ nova/virt/libvirt/connection.py	2011-09-20 09:48:25 +0000
@@ -773,7 +773,7 @@
     def _fetch_image(self, context, target, image_id, user_id, project_id,
                      size=None):
         """Grab image and optionally attempt to resize it"""
-        images.fetch(context, image_id, target, user_id, project_id)
+        images.fetch_to_raw(context, image_id, target, user_id, project_id)
         if size:
             disk.extend(target, size)