Libguestfs - Mar 2017 - [PATCH 2/4] v2v: chmod original OVA file if running as root (RHBZ#1430680).

This has to be applied on top of this series:

  https://www.redhat.com/archives/libguestfs/2017-March/msg00144.html

This is a fix for:

  https://bugzilla.redhat.com/show_bug.cgi?id=1430680

Kun Wei noticed that virt-v2v -i ova has a problem if we are running
as root and the OVA is not located on a path which is fully readable
by non-root.  The reason for this is that libvirt runs qemu as a
non-root user in this case (because there is no qemu:///session for
root, which is a libvirt bug).  qemu cannot access the path, and so
the conversion fails.

Now previously we'd seen this bug and worked around it by chmodding
the exploded directory, ie. the product of untarring the OVA file
(https://bugzilla.redhat.com/show_bug.cgi?id=1375157#c6).

However this workaround stopped working when we added support for
accessing the OVA file directly instead of unpacking it, since now the
unreadable OVA file is the backing file.

Fixing this is simple enough -- at least in the trivial csae where the
OVA file is not in an unreadable containing directory, there's no good
way to work around that.  But the fix revealed another bug in libvirt
to do with relative paths in backing files
(https://bugzilla.redhat.com/show_bug.cgi?id=1431652) so I had to work
around that as well.

This series also includes a regression test for this.  That's not
very simple either, since the regression test has to run as root,
requiring me to add infrastructure for running tests as root
('sudo make check-root').

Rich.

Works around a libvirt bug:
https://bugzilla.redhat.com/show_bug.cgi?id=1431652
---
 v2v/input_ova.ml                 | 6 +++++-
 v2v/test-v2v-i-ova-subfolders.sh | 2 +-
 v2v/test-v2v-i-ova-tar.sh        | 2 +-
 v2v/test-v2v-i-ova-two-disks.sh  | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index a0a42a7..fb6b286 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -284,6 +284,10 @@ object
              * tar also works with 512 byte blocks.
              *)
             let size = roundup64 size 512L in
+
+            (* Workaround for libvirt bug RHBZ#1431652. *)
+            let ova_path = absolute_path ova in
+
             let doc = [
                 "file", JSON.Dict [
                             "driver", JSON.String "raw";
@@ -291,7 +295,7 @@ object
                             "size", JSON.Int64 size;
                             "file", JSON.Dict [
                                         "driver", JSON.String
"file";
-                                        "filename", JSON.String ova]
+                                        "filename", JSON.String
ova_path]
                           ]
               ] in
             let uri diff --git a/v2v/test-v2v-i-ova-subfolders.sh
b/v2v/test-v2v-i-ova-subfolders.sh
index 12e0e87..be98d2c 100755
--- a/v2v/test-v2v-i-ova-subfolders.sh
+++ b/v2v/test-v2v-i-ova-subfolders.sh
@@ -55,7 +55,7 @@ if grep -sq json: $d/source ; then
     # Normalize the output.
     # Remove directory prefix.
     # Exact offset will vary because of tar.
-    sed -i -e "s,\"$d/,\"," \
+    sed -i -e "s,\"[^\"]*/$d/,\"," \
            -e "s|\"offset\": [0-9]*,|\"offset\":
x,|" $d/source
     diff -u test-v2v-i-ova-subfolders.expected2 $d/source
 else
diff --git a/v2v/test-v2v-i-ova-tar.sh b/v2v/test-v2v-i-ova-tar.sh
index 003d5e6..9458691 100755
--- a/v2v/test-v2v-i-ova-tar.sh
+++ b/v2v/test-v2v-i-ova-tar.sh
@@ -54,7 +54,7 @@ if grep -sq json: $d/source ; then
     # Normalize the output.
     # Remove directory prefix.
     # Exact offset will vary because of tar.
-    sed -i -e "s,\"$d/,\"," \
+    sed -i -e "s,\"[^\"]*/$d/,\"," \
            -e "s|\"offset\": [0-9]*,|\"offset\":
x,|" $d/source
     diff -u test-v2v-i-ova-tar.expected2 $d/source
 else
diff --git a/v2v/test-v2v-i-ova-two-disks.sh b/v2v/test-v2v-i-ova-two-disks.sh
index 9ecc380..d5cca06 100755
--- a/v2v/test-v2v-i-ova-two-disks.sh
+++ b/v2v/test-v2v-i-ova-two-disks.sh
@@ -59,7 +59,7 @@ if grep -sq json: $d/source ; then
     # Normalize the output.
     # Remove directory prefix.
     # Exact offset will vary because of tar.
-    sed -i -e "s,\"$d/,\"," \
+    sed -i -e "s,\"[^\"]*/$d/,\"," \
            -e "s|\"offset\": [0-9]*,|\"offset\":
x,|" $d/source
     diff -u test-v2v-i-ova-two-disks.expected2 $d/source
 else
-- 
2.9.3

In the case where we are going to read the disk directly from the OVA
file (partial = true), we will create a qcow2 image backed by the OVA.
If running as root, libvirt will run qemu as a non-root user (because
of no qemu:///session support for root, which is a libvirt bug).  qemu
will not be able to read the backing file and thus will fail.

To work around this problem, chmod the backing (OVA) file.

This isn't a complete fix since (eg) the OVA might be located in a
directory which isn't accessible by root, but there's only so much we
can do here until libvirt is fixed.

Thanks: Kun Wei.
---
 v2v/input_ova.ml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/v2v/input_ova.ml b/v2v/input_ova.ml
index fb6b286..e80ec82 100644
--- a/v2v/input_ova.ml
+++ b/v2v/input_ova.ml
@@ -171,7 +171,8 @@ object
      *)
     if Unix.geteuid () = 0 && backend_is_libvirt () then (
       warning (f_"making OVA directory public readable to work around
libvirt bug https://bugzilla.redhat.com/1045069");
-      let cmd = [ "chmod"; "-R"; "go=u,go-w";
exploded ] in
+      let cmd = [ "chmod"; "-R"; "go=u,go-w";
exploded ] @
+                if partial then [ ova ] else [] in
       ignore (run_command cmd)
     );
 
-- 
2.9.3

'[sudo] make check-root' is analogous to 'make check-slow'.
---
 Makefile.am              | 17 ++++++++++++++++-
 docs/guestfs-hacking.pod | 37 ++++++++++++++++++++++++++++++++++++-
 tests/test-functions.sh  |  9 +++++++++
 3 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index 7250cae..12486de 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -517,6 +517,20 @@ check-slow: build-test-guests
 	done; \
 	exit $$(( $$errors ? 1 : 0 ))
 
+check-root: build-test-guests
+	@if test "$$(id -u)" -ne 0; then \
+	  echo "***"; \
+	  echo "*** error: You must run 'check-root' as root."; \
+	  echo "***"; \
+	  exit 1; \
+	fi
+	@errors=0; \
+	for f in `grep -l '^$@:' $(SUBDIRS:%=%/Makefile.am)`; do \
+	  echo $(MAKE) -C `dirname $$f` $@; \
+	  $(MAKE) -C `dirname $$f` $@ || (( errors++ )); \
+	done; \
+	exit $$(( $$errors ? 1 : 0 ))
+
 build-test-guests:
 	$(MAKE) -C test-data/phony-guests check
 
@@ -590,8 +604,9 @@ help:
 	@echo "make check-with-upstream-qemu     Test using upstream qemu."
 	@echo "make check-with-upstream-libvirt  Test using upstream
libvirt."
 	@echo "make check-slow                   Slow/long-running tests."
+	@echo "sudo make check-root              Tests which must be run as
root."
 	@echo
-	@echo "make check-all                    Runs all 'check*'
rules."
+	@echo "make check-all                    Runs all 'check*' rules
except check-root."
 	@echo "make check-release                Runs 'check*' rules
required for release."
 	@echo
 	@echo "make installcheck                 Test installed libguestfs
packages."
diff --git a/docs/guestfs-hacking.pod b/docs/guestfs-hacking.pod
index df429a1..401dc95 100644
--- a/docs/guestfs-hacking.pod
+++ b/docs/guestfs-hacking.pod
@@ -662,9 +662,44 @@ Add a rule to the F<Makefile.am>:
 
 =back
 
+=item C<sudo make check-root>
+
+Runs some tests which require root privileges.  These are supposed to
+be safe, but take care.  You have to run this as root (eg. using
+L<sudo(8)> explicitly).
+
+To mark a test as requiring root:
+
+=over 4
+
+=item *
+
+Add it to the list of C<TESTS> in the F<Makefile.am>, just like a
+normal test.
+
+=item *
+
+Modify the test so it checks if euid == 0, and if I<not> set it skips
+(ie. returns with exit code 77).  If using C<$TEST_FUNCTIONS>, you can
+call the function C<root_test> for this.
+
+=item *
+
+Add a variable C<ROOT_TESTS> to the F<Makefile.am> listing the root
+tests.
+
+=item *
+
+Add a rule to the F<Makefile.am>:
+
+ check-root:
+   $(MAKE) check TESTS="$(ROOT_TESTS)"
+
+=back
+
 =item C<make check-all>
 
-Equivalent to running all C<make check*> rules.
+Equivalent to running all C<make check*> rules except
C<check-root>.
 
 =item C<make check-release>
 
diff --git a/tests/test-functions.sh b/tests/test-functions.sh
index 74ad2ef..e5e8246 100755
--- a/tests/test-functions.sh
+++ b/tests/test-functions.sh
@@ -242,6 +242,15 @@ slow_test ()
     fi
 }
 
+# Root tests should always call this function.  (See guestfs-hacking(1)).
+root_test ()
+{
+    if test "$(id -u)" -ne 0; then
+        echo "$(basename $0): use 'sudo make check-root' to run
this test"
+        exit 77
+    fi
+}
+
 do_md5 ()
 {
   case "$(uname)" in
-- 
2.9.3

---
 v2v/Makefile.am                | 11 ++++-
 v2v/test-v2v-i-ova-as-root.ovf | 95 ++++++++++++++++++++++++++++++++++++++++++
 v2v/test-v2v-i-ova-as-root.sh  | 65 +++++++++++++++++++++++++++++
 3 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 v2v/test-v2v-i-ova-as-root.ovf
 create mode 100755 v2v/test-v2v-i-ova-as-root.sh

diff --git a/v2v/Makefile.am b/v2v/Makefile.am
index f065654..e8ba2f8 100644
--- a/v2v/Makefile.am
+++ b/v2v/Makefile.am
@@ -305,7 +305,8 @@ TESTS += \
 	test-v2v-on-option.sh \
 	test-v2v-print-source.sh \
 	test-v2v-sound.sh \
-	$(SLOW_TESTS)
+	$(SLOW_TESTS) \
+	$(ROOT_TESTS)
 endif
 endif ENABLE_APPLIANCE
 
@@ -333,6 +334,12 @@ SLOW_TESTS = \
 check-slow:
 	$(MAKE) check TESTS="$(SLOW_TESTS)" SLOW=1
 
+ROOT_TESTS = \
+	test-v2v-i-ova-as-root.sh
+
+check-root:
+	$(MAKE) check TESTS="$(ROOT_TESTS)"
+
 # A selection of real guests that test-v2v-conversion-of.sh will
 # try to convert.  This is only used by 'make check-slow'.
 
@@ -376,6 +383,8 @@ EXTRA_DIST += \
 	test-v2v-floppy.sh \
 	test-v2v-floppy.xml \
 	test-v2v-i-disk.sh \
+	test-v2v-i-ova-as-root.ovf \
+	test-v2v-i-ova-as-root.sh \
 	test-v2v-i-ova-bad-sha1.sh \
 	test-v2v-i-ova-bad-sha256.sh \
 	test-v2v-i-ova-checksums.ovf \
diff --git a/v2v/test-v2v-i-ova-as-root.ovf b/v2v/test-v2v-i-ova-as-root.ovf
new file mode 100644
index 0000000..5bb7f6e
--- /dev/null
+++ b/v2v/test-v2v-i-ova-as-root.ovf
@@ -0,0 +1,95 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<Envelope xmlns="http://schemas.dmtf.org/ovf/envelope/1"
xmlns:cim="http://schemas.dmtf.org/wbem/wscim/1/common"
xmlns:ovf="http://schemas.dmtf.org/ovf/envelope/1"
xmlns:rasd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_ResourceAllocationSettingData"
xmlns:vssd="http://schemas.dmtf.org/wbem/wscim/1/cim-schema/2/CIM_VirtualSystemSettingData"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">
+  <References>
+    <File ovf:href="disk.vmdk" ovf:id="file1"
ovf:size="4063232" />
+  </References>
+  <DiskSection>
+    <Info>Virtual disk information</Info>
+    <Disk ovf:capacity="512"
ovf:capacityAllocationUnits="byte * 2^20"
ovf:diskId="vmdisk1" ovf:fileRef="file1" />
+  </DiskSection>
+  <VirtualSystem ovf:id="ova-test">
+    <Info>A virtual machine</Info>
+    <Name>ova-test</Name>
+    <OperatingSystemSection ovf:id="69">
+      <Info>The kind of installed guest operating system</Info>
+      <Description>Microsoft Windows Server 2003
(32-bit)</Description>
+    </OperatingSystemSection>
+    <VirtualHardwareSection>
+      <Info>Virtual hardware requirements</Info>
+      <System>
+        <vssd:ElementName>Virtual Hardware
Family</vssd:ElementName>
+        <vssd:InstanceID>0</vssd:InstanceID>
+       
<vssd:VirtualSystemIdentifier>ova-test</vssd:VirtualSystemIdentifier>
+        <vssd:VirtualSystemType>vmx-08</vssd:VirtualSystemType>
+      </System>
+      <Item>
+        <rasd:AllocationUnits>hertz * 10^6</rasd:AllocationUnits>
+        <rasd:Description>Number of Virtual CPUs</rasd:Description>
+        <rasd:ElementName>2 virtual CPU(s)</rasd:ElementName>
+        <rasd:InstanceID>1</rasd:InstanceID>
+        <rasd:ResourceType>3</rasd:ResourceType>
+        <rasd:VirtualQuantity>2</rasd:VirtualQuantity>
+      </Item>
+      <Item>
+        <rasd:AllocationUnits>byte * 2^20</rasd:AllocationUnits>
+        <rasd:Description>Memory Size</rasd:Description>
+        <rasd:ElementName>4096MB of memory</rasd:ElementName>
+        <rasd:InstanceID>2</rasd:InstanceID>
+        <rasd:ResourceType>4</rasd:ResourceType>
+        <rasd:VirtualQuantity>4096</rasd:VirtualQuantity>
+      </Item>
+      <Item>
+        <rasd:Address>0</rasd:Address>
+        <rasd:Description>SCSI Controller</rasd:Description>
+        <rasd:ElementName>SCSI controller 0</rasd:ElementName>
+        <rasd:InstanceID>3</rasd:InstanceID>
+        <rasd:ResourceSubType>lsilogic</rasd:ResourceSubType>
+        <rasd:ResourceType>6</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:Address>1</rasd:Address>
+        <rasd:Description>IDE Controller</rasd:Description>
+        <rasd:ElementName>IDE 1</rasd:ElementName>
+        <rasd:InstanceID>4</rasd:InstanceID>
+        <rasd:ResourceType>5</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:Address>0</rasd:Address>
+        <rasd:Description>IDE Controller</rasd:Description>
+        <rasd:ElementName>IDE 0</rasd:ElementName>
+        <rasd:InstanceID>5</rasd:InstanceID>
+        <rasd:ResourceType>5</rasd:ResourceType>
+      </Item>
+      <Item ovf:required="false">
+        <rasd:AutomaticAllocation>false</rasd:AutomaticAllocation>
+        <rasd:ElementName>Video card</rasd:ElementName>
+        <rasd:InstanceID>6</rasd:InstanceID>
+        <rasd:ResourceType>24</rasd:ResourceType>
+      </Item>
+      <Item ovf:required="false">
+        <rasd:AutomaticAllocation>false</rasd:AutomaticAllocation>
+        <rasd:ElementName>VMCI device</rasd:ElementName>
+        <rasd:InstanceID>7</rasd:InstanceID>
+        <rasd:ResourceSubType>vmware.vmci</rasd:ResourceSubType>
+        <rasd:ResourceType>1</rasd:ResourceType>
+      </Item>
+      <Item ovf:required="false">
+        <rasd:AddressOnParent>1</rasd:AddressOnParent>
+        <rasd:AutomaticAllocation>false</rasd:AutomaticAllocation>
+        <rasd:ElementName>CD/DVD drive 1</rasd:ElementName>
+        <rasd:InstanceID>8</rasd:InstanceID>
+        <rasd:Parent>4</rasd:Parent>
+       
<rasd:ResourceSubType>vmware.cdrom.remotepassthrough</rasd:ResourceSubType>
+        <rasd:ResourceType>15</rasd:ResourceType>
+      </Item>
+      <Item>
+        <rasd:AddressOnParent>0</rasd:AddressOnParent>
+        <rasd:ElementName>Hard disk 1</rasd:ElementName>
+        <rasd:HostResource>ovf:/disk/vmdisk1</rasd:HostResource>
+        <rasd:InstanceID>9</rasd:InstanceID>
+        <rasd:Parent>3</rasd:Parent>
+        <rasd:ResourceType>17</rasd:ResourceType>
+      </Item>
+    </VirtualHardwareSection>
+  </VirtualSystem>
+</Envelope>
diff --git a/v2v/test-v2v-i-ova-as-root.sh b/v2v/test-v2v-i-ova-as-root.sh
new file mode 100755
index 0000000..04d5d37
--- /dev/null
+++ b/v2v/test-v2v-i-ova-as-root.sh
@@ -0,0 +1,65 @@
+#!/bin/bash -
+# libguestfs virt-v2v test script
+# Copyright (C) 2017 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# Regression test for the case where a OVA file is only readable as
+# root.  Because libvirt runs qemu as qemu.qemu in this case, qemu
+# cannot open the OVA file.  See:
+# https://bugzilla.redhat.com/show_bug.cgi?id=1375157#c6
+# https://bugzilla.redhat.com/show_bug.cgi?id=1430680
+
+set -e
+
+$TEST_FUNCTIONS
+root_test
+skip_if_skipped
+skip_unless_backend libvirt
+skip_unless_phony_guest windows.img
+
+if [ ! -f windows.vmdk -o ! -s windows.vmdk ]; then
+    echo "$0: test skipped because windows.vmdk was not created"
+    exit 77
+fi
+
+export VIRT_TOOLS_DATA_DIR="$top_srcdir/test-data/fake-virt-tools"
+
+d=test-v2v-i-ova-as-root.d
+rm -rf $d
+mkdir $d
+
+pushd $d
+
+# Create the test OVA.
+cp ../test-v2v-i-ova-as-root.ovf test.ovf
+cp ../windows.vmdk disk.vmdk
+
+echo "SHA1(test.ovf)=" `do_sha1 test.ovf` > test.mf
+echo "SHA1(disk.vmdk)=" `do_sha1 disk.vmdk` >> test.mf
+
+tar cf test.ova test.ovf disk.vmdk test.mf
+
+# So it's unreadable by non-root.
+chown root.root test.ova
+chmod 0600 test.ova
+
+# Run virt-v2v.
+$VG virt-v2v --debug-gc --quiet \
+    -i ova test.ova \
+    -o null
+
+popd
+rm -rf $d
-- 
2.9.3

In data lunedì 13 marzo 2017 14:47:43 CET, Richard W.M. Jones ha
scritto:
> In the case where we are going to read the disk directly from the OVA
> file (partial = true), we will create a qcow2 image backed by the OVA.
> If running as root, libvirt will run qemu as a non-root user (because
> of no qemu:///session support for root, which is a libvirt bug).  qemu
> will not be able to read the backing file and thus will fail.
> 
> To work around this problem, chmod the backing (OVA) file.
> 
> This isn't a complete fix since (eg) the OVA might be located in a
> directory which isn't accessible by root, but there's only so much
we
> can do here until libvirt is fixed.
> 
> Thanks: Kun Wei.
> ---
Can you please mention in the commit message that this updates commit
d9b2a16c71d0c87195e28c1325fd83b344741339 ?

Thanks,
-- 
Pino Toscano

In data lunedì 13 marzo 2017 14:47:41 CET, Richard W.M. Jones ha
scritto:
> This has to be applied on top of this series:
> 
>   https://www.redhat.com/archives/libguestfs/2017-March/msg00144.html
> 
> This is a fix for:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1430680
> 
> Kun Wei noticed that virt-v2v -i ova has a problem if we are running
> as root and the OVA is not located on a path which is fully readable
> by non-root.  The reason for this is that libvirt runs qemu as a
> non-root user in this case (because there is no qemu:///session for
> root, which is a libvirt bug).  qemu cannot access the path, and so
> the conversion fails.
> 
> Now previously we'd seen this bug and worked around it by chmodding
> the exploded directory, ie. the product of untarring the OVA file
> (https://bugzilla.redhat.com/show_bug.cgi?id=1375157#c6).
> 
> However this workaround stopped working when we added support for
> accessing the OVA file directly instead of unpacking it, since now the
> unreadable OVA file is the backing file.
> 
> Fixing this is simple enough -- at least in the trivial csae where the
> OVA file is not in an unreadable containing directory, there's no good
> way to work around that.  But the fix revealed another bug in libvirt
> to do with relative paths in backing files
> (https://bugzilla.redhat.com/show_bug.cgi?id=1431652) so I had to work
> around that as well.
> 
> This series also includes a regression test for this.  That's not
> very simple either, since the regression test has to run as root,
> requiring me to add infrastructure for running tests as root
> ('sudo make check-root').
Let's ACK this, although I still keep thinking this root workaround is
only going to hit us again and again (for example, in case the OVA is
in a root-only directory).

-- 
Pino Toscano

On Mon, 13 Mar 2017 14:47:43 +0000
"Richard W.M. Jones" <rjones@redhat.com> wrote:
> In the case where we are going to read the disk directly from the OVA
> file (partial = true), we will create a qcow2 image backed by the OVA.
> If running as root, libvirt will run qemu as a non-root user (because
> of no qemu:///session support for root, which is a libvirt bug).  qemu
> will not be able to read the backing file and thus will fail.
I was under the impression that libvirt chmods/chowns all disks so they
are accessible by QEMU. Is this a bug in libvirt because the owner is
only changed for the overlay but not for all the backing files?

Or is libvirt just being sloppy in the job and it only changes the owner
of the file but does not check the path if there is any permission
problem along the way on some directory? (Although I'm not sure what
would be a proper response from libvirt in this case.)


    Tomas

-- 
Tomáš Golembiovský <tgolembi@redhat.com>