Libguestfs - Sep 2016 - Re: [PATCH 2/2] v2v: ova: support SHA256 hashes in manifest

The OVF standard allows the use of SHA256 hashes in the manifest file.
Adding support for this.

Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
---
 v2v/input_ova.ml | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index 513fe30..5420c85 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -133,7 +133,7 @@ object
 
     (* Read any .mf (manifest) files and verify sha1. *)
     let mf = find_files exploded ".mf" in
-    let rex = Str.regexp "SHA1(\\(.*\\))= \\([0-9a-fA-F]+\\)\r?" in
+    let rex = Str.regexp "SHA\\(1\\|256\\)(\\(.*\\))=
\\([0-9a-fA-F]+\\)\r?" in
     List.iter (
       fun mf ->
         debug "Processing manifest %s" mf;
@@ -142,20 +142,23 @@ object
         let rec loop ()            let line = input_line chan in
           if Str.string_match rex line 0 then (
-            let disk = Str.matched_group 1 line in
-            let expected = Str.matched_group 2 line in
-            let cmd = sprintf "sha1sum %s" (quote (mf_folder //
disk)) in
+            let sha_num = Str.matched_group 1 line in
+            let disk = Str.matched_group 2 line in
+            let expected = Str.matched_group 3 line in
+            let sha = sprintf "sha%ssum" sha_num in
+            let cmd = sprintf "%s %s" sha (quote (mf_folder // disk))
in
             let out = external_command cmd in
             match out with
             | [] ->
-              error (f_"no output from sha1sum command, see previous
errors")
+              error (f_"no output from %s command, see previous
errors")
+                sha
             | [line] ->
               let actual, _ = String.split " " line in
               if actual <> expected then
-                error (f_"checksum of disk %s does not match manifest %s
(actual sha1(%s) = %s, expected sha1 (%s) = %s)")
-                  disk mf disk actual disk expected;
-              debug "sha1 of %s matches expected checksum %s" disk
expected
-            | _::_ -> error (f_"cannot parse output of sha1sum
command")
+                error (f_"checksum of disk %s does not match manifest %s
(actual sha%s(%s) = %s, expected sha%s (%s) = %s)")
+                  disk mf sha_num disk actual sha_num disk expected;
+              debug "%s of %s matches expected checksum %s" sha disk
expected
+            | _::_ -> error (f_"cannot parse output of %s
command") sha
           )
           else
             warning (f_"Unable to parse line from manifest file. Line is
\"%s\"")
-- 
2.10.0

On Thursday, 29 September 2016 14:59:31 CEST Tomáš Golembiovský
wrote:
> The OVF standard allows the use of SHA256 hashes in the manifest file.
> Adding support for this.
> 
> Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> ---
Maybe it would be a better idea to move the Checksums module from
virt-builder to e.g. mllib, making it slightly more generic in error
reporting, add SHA1 support to it, and use it in this case as well.
>  v2v/input_ova.ml | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
> index 513fe30..5420c85 100644
> --- a/v2v/input_ova.ml
> +++ b/v2v/input_ova.ml
> @@ -133,7 +133,7 @@ object
>  
>      (* Read any .mf (manifest) files and verify sha1. *)
>      let mf = find_files exploded ".mf" in
> -    let rex = Str.regexp "SHA1(\\(.*\\))= \\([0-9a-fA-F]+\\)\r?"
in
> +    let rex = Str.regexp "SHA\\(1\\|256\\)(\\(.*\\))=
\\([0-9a-fA-F]+\\)\r?" in
For example, with the above suggestion, catching the whole checksum
type in regex and passing it to a new Checksums.from_string function
could allow to support any checksum type that Checksums knows about.

Thanks,
-- 
Pino Toscano

On Thu, Sep 29, 2016 at 03:56:36PM +0200, Pino Toscano
wrote:
> On Thursday, 29 September 2016 14:59:31 CEST Tomáš Golembiovský wrote:
> > The OVF standard allows the use of SHA256 hashes in the manifest file.
> > Adding support for this.
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> 
> Maybe it would be a better idea to move the Checksums module from
> virt-builder to e.g. mllib, making it slightly more generic in error
> reporting, add SHA1 support to it, and use it in this case as well.
> 
> >  v2v/input_ova.ml | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
> > index 513fe30..5420c85 100644
> > --- a/v2v/input_ova.ml
> > +++ b/v2v/input_ova.ml
> > @@ -133,7 +133,7 @@ object
> >  
> >      (* Read any .mf (manifest) files and verify sha1. *)
> >      let mf = find_files exploded ".mf" in
> > -    let rex = Str.regexp "SHA1(\\(.*\\))=
\\([0-9a-fA-F]+\\)\r?" in
> > +    let rex = Str.regexp "SHA\\(1\\|256\\)(\\(.*\\))=
\\([0-9a-fA-F]+\\)\r?" in
> 
> For example, with the above suggestion, catching the whole checksum
> type in regex and passing it to a new Checksums.from_string function
> could allow to support any checksum type that Checksums knows about.
As long as this is safe with untrusted input (from the OVA file),
I agree.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On Thursday, 29 September 2016 15:56:36 CEST Pino Toscano
wrote:
> On Thursday, 29 September 2016 14:59:31 CEST Tomáš Golembiovský wrote:
> > The OVF standard allows the use of SHA256 hashes in the manifest file.
> > Adding support for this.
> > 
> > Signed-off-by: Tomáš Golembiovský <tgolembi@redhat.com>
> > ---
> 
> Maybe it would be a better idea to move the Checksums module from
> virt-builder to e.g. mllib, making it slightly more generic in error
> reporting, add SHA1 support to it, and use it in this case as well.
Just done that, see the series for this:
https://www.redhat.com/archives/libguestfs/2016-September/msg00198.html

-- 
Pino Toscano