virt-image / ImageParser disk signature verification function
 

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.
_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

virt-image / ImageParser disk signature verification function
 

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)

_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

virt-image / ImageParser disk signature verification function
 

Cole Robinson wrote:

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.


Made all the corrections. I understand calculating for just one disk,
which will be more reusable for other code than just this case, but why
create separate method to check all the disks as well?



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)






_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

virt-image / ImageParser disk signature verification function
 

Joey Boggs wrote:
> Cole Robinson wrote:
>> 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.
>>
>>
> Made all the corrections. I understand calculating for just one disk,
> which will be more reusable for other code than just this case, but
> why create separate method to check all the disks as well?
>

It's certainly not required, it would just be a convenience
function. I'm not attached to the idea if you want to skip
it.

Not sure if you intended to or not, but the patch wasn't
attached to this email.

Thanks,
Cole

_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

virt-image / ImageParser disk signature verification function
 

Better integrated into the Disk class and runs when a Disk object is created


Cole Robinson wrote:

Joey Boggs wrote:


Cole Robinson wrote:


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.




Made all the corrections. I understand calculating for just one disk,
which will be more reusable for other code than just this case, but
why create separate method to check all the disks as well?




It's certainly not required, it would just be a convenience
function. I'm not attached to the idea if you want to skip
it.

Not sure if you intended to or not, but the patch wasn't
attached to this email.

Thanks,
Cole



_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

virt-image / ImageParser disk signature verification function
 

Joey Boggs wrote:
>
> Better integrated into the Disk class and runs when a Disk object is
> created
>
> diff -r db5d9aeca590 virtinst/ImageParser.py
> --- a/virtinst/ImageParser.py Fri Oct 10 10:32:50 2008 -0400
> +++ b/virtinst/ImageParser.py Thu Oct 16 09:41:22 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):
> @@ -207,6 +209,7 @@
> self.csum = {}
> if not node is None:
> self.parseXML(node)
> + self.check_disk_signature()
>

Do we really want this at init time? That doesn't
provide the API user any way to prevent the csum
checks from running. We should really make this
optional from an API standpoint, but still make
it the default in virt-image. Just dropping the
above should solve that.

That's really my only gripe, so I can fix it when
I commit if it's okay with you.

Thanks,
Cole

_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

virt-image / ImageParser disk signature verification function
 

Cole,

That's fine I'll take a look after it's commited and write that down for
future reference.


Cole Robinson wrote:

Joey Boggs wrote:


Better integrated into the Disk class and runs when a Disk object is
created

diff -r db5d9aeca590 virtinst/ImageParser.py
--- a/virtinst/ImageParser.py Fri Oct 10 10:32:50 2008 -0400
+++ b/virtinst/ImageParser.py Thu Oct 16 09:41:22 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):
@@ -207,6 +209,7 @@
self.csum = {}
if not node is None:
self.parseXML(node)
+ self.check_disk_signature()




Do we really want this at init time? That doesn't
provide the API user any way to prevent the csum
checks from running. We should really make this
optional from an API standpoint, but still make
it the default in virt-image. Just dropping the
above should solve that.

That's really my only gripe, so I can fix it when
I commit if it's okay with you.

Thanks,
Cole



_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools

virt-image / ImageParser disk signature verification function
 

Joey Boggs wrote:
> Cole,
>
> That's fine I'll take a look after it's commited and write that down
> for future reference.
>

Thanks, committed now:

http://hg.et.redhat.com/virt/applications/virtinst--devel?cs=9f45a36d8242

- Cole

_______________________________________________
et-mgmt-tools mailing list
et-mgmt-tools@redhat.com
https://www.redhat.com/mailman/listinfo/et-mgmt-tools