[et-mgmt-tools] [patch] virt-image / ImageParser disk signature verification function

[Cole Robinson]

Joey Boggs wrote:
> This adds a new function to the virtinst.ImageParser.Disk class to 
> verify disk signatures and runs by default when using virt-image. The 
> next patch will wrap up loose ends in the ImageParser.Disk class to weed 
> out unsupported checksum types. For now, if the checksum type is not 
> matched it skips the check process.
> 
> 


> diff -r db5d9aeca590 virt-image
> --- a/virt-image	Fri Oct 10 10:32:50 2008 -0400
> +++ b/virt-image	Tue Oct 14 22:25:21 2008 -0400
> @@ -197,6 +197,8 @@
>  
>      get_graphics(image.domain, options.vnc, options.vncport,
>                   options.nographics, options.sdl, options.keymap, guest)
> +    checksum = virtinst.ImageParser.Disk
> +    checksum.check_disk_signature(image)
>  

I don't really like this interface of having a method
of the Disk class that receives an Image object and
pulls disks from that. Doesn't really fit the whole
class dichotomy.

The way to do it is to have the Disk method calculate
the checksum for that particular disk. Then you can
add a convenience method to the Image class that will
calculate the checksums for every Disk object associated
with that Image.

>      if installer.is_hvm():
>          if options.noacpi:
> diff -r db5d9aeca590 virtinst/ImageParser.py
> --- a/virtinst/ImageParser.py	Fri Oct 10 10:32:50 2008 -0400
> +++ b/virtinst/ImageParser.py	Tue Oct 14 22:25:21 2008 -0400
> @@ -23,6 +23,8 @@
>  import libxml2
>  import CapabilitiesParser
>  from virtinst import _virtinst as _
> +import logging
> +import urlgrabber.progress as progress
>  
>  class ParserException(Exception):
>      def __init__(self, msg):
> @@ -224,6 +226,49 @@
>                    _("The format for disk %s must be one of %s") %
>                    (self.file, ",".join(formats)))
>  
> +    def check_disk_signature(self,image):
> +        try:
> +            import hashlib
> +            has_hashlib = True
> +        except:
> +            import sha
> +            has_hashlib = False
> +
> +        for disk in image.storage.values():
> +            meter_ct = 0
> +            m = None
> +            disk_size = os.path.getsize(disk.id)

Disk.id doesn't seem to be a required field, and
doesn't necessarily point to a path. I would say
use disk.file throughout.

> +            meter = progress.TextMeter()
> +            meter.start(size=disk_size, text=("Checking disk signature for %s" % disk.id))
> +

Make that text=_("... to translate it.

> +            if has_hashlib is True:
> +                if disk.csum.has_key("sha256"):
> +                    csumvalue = disk.csum["sha256"]
> +                    m = hashlib.sha256()
> +                elif disk.csum.has_key("sha1"):
> +                    csumvalue = disk.csum["sha1"]
> +                    m = hashlib.sha1()
> +            else:
> +                if disk.csum.has_key("sha1"):
> +                    csumvalue = disk.csum["sha1"]
> +                    m = sha.new()   
> +            if m:

At this point, I'd just do 

if not m:
    return

to avoid all the indentation.

> +                f = open(disk.id,"r")
> +                while 1:
> +                    chunk = f.read(65536)
> +                    meter.update(meter_ct)
> +                    meter_ct = meter_ct + 65536
> +                    if not chunk:
> +                        break

Pop this 'if not' right after the read() so we
don't update the meter if nothing was read.

> +                    m.update(chunk)
> +                checksum = m.hexdigest()
> +                if checksum != csumvalue:
> +                      logging.debug(_("Disk signature for %s does not match Expected: %s  Received: %s"
> +                                 % (disk.id,csumvalue,checksum)))
> +                      raise ValueError (_("Disk signature for %s does not match" % disk.id))
> +            
> +                        
> +

Please be consistent with the current white space
and just use 1 newline here.

>  def validate(cond, msg):
>      if not cond:
>          raise ParserException(msg)