[et-mgmt-tools] [PATCH] virtinst adding in disk signature support

[Cole Robinson]

Joey Boggs wrote:
> This will add in disk signature support for ISV's and others folks that 
> wish to verify the disk has not been altered prior to running 
> virt-image. Supports md5 and sha1signatures.
> 
> Sample image.xml attached
> 

For the sample xml it would be nice to see an md5 example
as well.

> iff -r 58a909b4f71c doc/image.rng
> --- a/doc/image.rng	Mon Sep 22 11:32:11 2008 -0400
> +++ b/doc/image.rng	Wed Sep 24 13:56:34 2008 -0400
> @@ -197,6 +197,14 @@
>              </choice>
>            </attribute>
>          </optional>
> +        <optional>
> +          <element name="checksum">
> +            <attribute name="type">
> +              <value>sha1</value>

Need to add <value> for md5 as well.

> +            </attribute>
> +            <text/>
> +          </element>
> +        </optional>
>        </element>
>      </oneOrMore>
>    </define>
> diff -r 58a909b4f71c virt-image
> --- a/virt-image	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virt-image	Wed Sep 24 13:56:34 2008 -0400
> @@ -97,6 +97,8 @@
>                        help=_("Number of vcpus to configure for your guest"))
>      parser.add_option("", "--check-cpu", action="store_true", dest="check_cpu",
>                        help=_("Check that vcpus do not exceed physical CPUs and warn if they do."))
> +    parser.add_option("", "--checksum-ignore", action="store_true", dest="checksum_ignore",
> +                      help=_("Ignore unmatching checksum values for disk signatures."))
>  
>      # network options
>      parser.add_option("-m", "--mac", type="string",
> @@ -188,6 +190,10 @@
>      # now let's get some of the common questions out of the way
>      get_name(options.name, image.name, guest)
>      get_memory(options.memory, image.domain.memory, guest)
> +
> +    if not options.checksum_ignore:
> +        cli.check_disk_signature(image,guest)
> +
>      cli.get_uuid(options.uuid, guest)
>      get_vcpus(options.vcpus, image.domain.vcpu, options.check_cpu,
>                guest, conn)
> diff -r 58a909b4f71c virtinst/ImageParser.py
> --- a/virtinst/ImageParser.py	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtinst/ImageParser.py	Wed Sep 24 13:56:34 2008 -0400
> @@ -213,7 +213,8 @@
>          self.format = xpathString(node, "@format", Disk.FORMAT_RAW)
>          self.size = xpathString(node, "@size")
>          self.use = xpathString(node, "@use", Disk.USE_SYSTEM)
> -
> +        self.checksum = xpathString(node, "checksum") 
> +        self.checksumtype = xpathString(node, "checksum/@type")
>          formats = [Disk.FORMAT_RAW, Disk.FORMAT_QCOW, Disk.FORMAT_QCOW2, Disk.FORMAT_VMDK, Disk.FORMAT_ISO]
>          validate (formats.count(self.format) > 0,
>                    _("The format for disk %s must be one of %s") %
> diff -r 58a909b4f71c virtinst/cli.py
> --- a/virtinst/cli.py	Mon Sep 22 11:32:11 2008 -0400
> +++ b/virtinst/cli.py	Wed Sep 24 13:56:34 2008 -0400
> @@ -352,6 +352,27 @@
>      if sound:
>          guest.sound_devs.append(VirtualAudio(model="es1370"))
>  
> +def check_disk_signature(image,guest):
> +    i = 0
> +    disks = {}
> +    for k in image.storage.keys():
> +        disks[i] = image.storage[k]
> +        if disks[i].checksumtype == "sha1" or disks[i].checksumtype == "md5" and not None:

I'd rather not silently ignore any values that aren't in the
white list of supported hash types. Probably at the image
parser level, when we set checksumtype, we can raise an
exception if the type is not supported.

> +            checksum = os.popen("/usr/bin/%ssum %s|awk {'print $1'}" % (disks[i].checksumtype,disks[i].file))
> +            print _("\n\nChecking disk signature for: %s...") % disks[i].file 
> +            checksum = checksum.read().strip()

Looks like python has support for this with default modules 
('sha1' and 'md5', prob don't want to use 'hashlib' since it
is python 2.5 only). This would definitely be preferable to
the above: we won't have to worry about binary naming on other
distros, and potentially requiring dependencies, etc. etc.

> +            if checksum != disks[i].checksum:
> +                fail(_("Disk signature for %s does not match \n Expected: %s \n Received: %s\n\n To override the signature check add the --checksum-ignore option" % (disks[i].file,disks[i].checksum,checksum)))

This line is huuuuuuuge :) You can cut up strings like:

_("I am a very long string")

as:

_("I am a "
  "very long "
  "string")

In the above case it probably wouldn't hurt to do it
with each newline so the look in the code is similar
to what the user will see.

> +            else:
> +                continue
> +            i = i + 1
> +        else:
> +            if disks[i].checksumtype is None:
> +                continue
> +            else:
> +                fail(_("\"%s\" is an invalid disk signature type for %s" % (disks[i].checksumtype,disks[i].file)))

Just as a nit, rather than escape the double quotes here,
use single quotes ('%s') or wrap the whole string like 
"""blah""", rather then mess with escapes.

> +    return
> +
>  ### Option parsing
>  def check_before_store(option, opt_str, value, parser):
>      if len(value) == 0:

Thanks,
Cole