On 2014年03月10日 09:52, Sheldon wrote:
On 03/08/2014 04:20 AM, Christy Perez wrote:
Sorry this took me a while to get back to you. Answers below.

Do you want to pick this back up in the overall kimchi thread?


On Thu, 2014-03-06 at 13:29 +0800, Sheldon wrote:
On 03/04/2014 06:38 AM, Christy Perez wrote:
On Wed, 2014-02-26 at 20:26 +0800, Sheldon wrote:
On 02/25/2014 08:23 AM, Christy Perez wrote:
qemu can't use an ISO if 'other' can't execute all the directories
containing the file. Return an error explaning this, and suggest
the way to fix it.

Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com>
---
   po/en_US.po              | 6 ++++++
   src/kimchi/exception.py  | 4 ++++
   src/kimchi/i18n.py       | 2 ++
   src/kimchi/utils.py      | 9 +++++++++
   src/kimchi/vmtemplate.py | 7 +++++--
   5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/po/en_US.po b/po/en_US.po
index ca1fe86..38af64c 100644
--- a/po/en_US.po
+++ b/po/en_US.po
@@ -674,6 +674,12 @@ msgid "Bad format while reading volume descriptor in ISO %(filename)s"
   msgstr ""

   #, python-format
+msgid "The hypervisor doesn't have permission to use this ISO %(filename)s. \
+                     Consider moving it under /var/lib/libvirt, or \
+                     (not recommended) 'chmod -R o+x 'path_to_iso'."
+msgstr ""
+
+#, python-format
   msgid "Virtual machine %(name)s already exists"
   msgstr ""

diff --git a/src/kimchi/exception.py b/src/kimchi/exception.py
index 2d974a8..263bd0c 100644
--- a/src/kimchi/exception.py
+++ b/src/kimchi/exception.py
@@ -75,6 +75,10 @@ class InvalidParameter(KimchiException):
       pass


+class InvalidPermission(KimchiException):
+    pass
+
+
   class InvalidOperation(KimchiException):
       pass

diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py
index fea0184..05b347c 100644
--- a/src/kimchi/i18n.py
+++ b/src/kimchi/i18n.py
@@ -57,6 +57,8 @@ messages = {
       "KCHISO0005E": _("Invalid El Torito boot indicator in ISO %(filename)s"),
       "KCHISO0006E": _("Unexpected volume type for primary volume in ISO %(filename)s"),
       "KCHISO0007E": _("Bad format while reading volume descriptor in ISO %(filename)s"),
+    "KCHISO0008E": _("The hypervisor doesn't have permission to use this ISO %(filename)s. \
+           Consider moving it under /var/lib/libvirt, or (not recommended) 'chmod -R o+x 'path_to_iso'." ),

       "KCHVM0001E": _("Virtual machine %(name)s already exists"),
       "KCHVM0002E": _("Virtual machine %(name)s does not exist"),
diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py
index 6be1c04..c325be3 100644
--- a/src/kimchi/utils.py
+++ b/src/kimchi/utils.py
@@ -27,6 +27,7 @@ import psutil
   import re
   import subprocess
   import urllib2
+import stat
   from threading import Timer

   from cherrypy.lib.reprconf import Parser
@@ -234,3 +235,11 @@ def run_setfacl_set_attr(path, attr="r", user=""):
       set_user = ["setfacl", "--modify", "user:%s:%s" % (user, attr), path]
       out, error, ret = run_command(set_user)
       return ret == 0
+
+def check_iso_path_perm(path):
+    """
+    libvirt requires that all parent dirs have o+x
+    """
+    if path == '/': return True
+    return os.stat(path).st_mode & stat.S_IXOTH and \
+                            check_iso_path_perm(os.path.dirname(path))
just test other's permission is not enough.
1. when the path user is qemu. why we need S_IXOTH?
That's what the libvirt doc says, and what I've found is needed: " The
directories containing disk images must either have their ownership set
to match the user/group configured for QEMU, or their UNIX file
permissions must have the 'execute/search' bit enabled for 'others'. "
Source: http://libvirt.org/drvqemu.html
so that's means the disk images without S_IXOTH, qemu still can have 
search permissions right?
Yep.
The images themselves won't have S_IXOTH.  qemu will own them, though:

[root@kop4 kimchi-ginger-pkvm]# ls -lah /var/lib | grep libvirt
drwxr-xr-x.  9 root   root    4.0K Feb 19 11:16 libvirt
[root@kop4 kimchi-ginger-pkvm]# ls -lah /var/lib/libvirt | grep images
drwx--x--x.  2 root root 4.0K Mar  7 06:22 images
[root@kop4 kimchi-ginger-pkvm]# ls -lah /var/lib/libvirt/images/
total 2.9G
drwx--x--x. 2 root root 4.0K Mar  7 06:22 .
drwxr-xr-x. 9 root root 4.0K Feb 19 11:16 ..
-rw-------. 1 qemu qemu 193K Mar  7 06:22
2f7130eb-539c-4401-82eb-cccd18708f56-0.img
-r--r--r--. 1 qemu qemu 2.9G Mar  7 06:20
RHEL-7.0-20140226.0-Server-ppc64-dvd1.iso
I think Sheldon means besides condition  of others have x of ISO's ancestor directories,  directory owner/group is qemu with x access can also guarantee qemu access ISO directoy success.

    directory owner==qemu AND qemu can x OR directory group==qemu AND qemu can x OR other can x => qemu can access the iso file directory(check should pass)

So based on these complex check personally I favour sudo 'qemu' and try open iso file to validate access. It is simple and avoid handling corner case.


o+x is  sufficient but not necessary condition?
o+x is necessary.
2. as you said "libvirt requires that all parent dirs have o+x"
you has adding 'X' to the ACL for each directory to try it.
I wander have check your ACL enable?
I don't think the ACL will be enough. It looks like it has to have o+x
for all parent paths. And yes, my ACLs were enabled on my filesystem.
That's strangely.  but the docs  http://libvirt.org/drvqemu.html
says:
it simple to set o+x,  means this is  the only way.
Yes, I think so. 
it is strange from what I test, it seems o+x is  not necessary but sufficient.
I really want to know why o+x is necessary and why it is the only way
to check the permission.

I have try 5 test case with kimchi.

1. There's a iso on my laptop, with o+x permission
$ ls /home/shhfeng/iso/RHEL6.4-20130123.0-Server-x86_64-DVD1.iso
/home/shhfeng/iso/RHEL6.4-20130123.0-Server-x86_64-DVD1.iso
$ ls /home/shhfeng/ -l |grep iso
drwxrwxrwx.  4 shhfeng shhfeng      4096 Oct  3 00:22 iso

I can create a vm and start is successfully.


2. Now I remove x permission
$ sudo chmod o-x /home/shhfeng/iso
$ ls /home/shhfeng/ -l |grep iso
drwxrwxrw-.  4 shhfeng shhfeng      4096 Oct  3 00:22 iso

I failed to start it for permission.

3. I change the own of this iso, still without x permission.
$ sudo chown qemu:qemu /home/shhfeng/iso
$ ls /home/shhfeng/ -l |grep iso
drwxrwxrw-.  4 qemu    qemu         4096 Oct  3 00:22 iso

Now I can start it successfully.


4. just set the qemu user as search permission.
$ sudo chown shhfeng:shhfeng /home/shhfeng/iso/
$ ls -l /home/shhfeng/ |grep iso
drwxrwxrw-.  4 shhfeng shhfeng      4096 Oct  3 00:22 iso
$ setfacl -m u:qemu:x /home/shhfeng/iso
$ getfacl /home/shhfeng/iso
getfacl: Removing leading '/' from absolute path names
# file: home/shhfeng/iso
# owner: shhfeng
# group: shhfeng
user::rwx
user:qemu:--x
group::rwx
mask::rwx
other::rw-

Now I can also start it successfully.


5. remove qemu user as search permission,
add set the 'kvm' group to the path, qemu is in "kvm" group
$ setfacl -x u:qemu /home/shhfeng/iso
$ groups qemu
qemu : qemu kvm
$ sudo chown shhfeng:kvm /home/shhfeng/iso/

Now I can also start it successfully.


only case 2, the kimchi deny to start vm for permission.



I write a python script checkperm.py
It is the same result of libvirt run qemu. you can have a try.
If others do like like checkperm.py,
I think we can probe the permission by let libvirt to start the guest with the iso.

$ sudo python checkperm.py /home/shhfeng/iso/RHEL6.4-20130123.0-Server-x86_64-DVD1.iso

# checkperm.py
  1 from multiprocessing import Process, Queue
  2 import sys
  3 import grp
  4 import os
  5 import pwd
  6
  7
  8 def probe_file_permission_as_user(file, user):
  9     def probe_permission(q, file, user):
 10         uid = pwd.getpwnam(user).pw_uid
 11         gid = pwd.getpwnam(user).pw_gid
 12         gids = [g.gr_gid for g in grp.getgrall() if user in g.gr_mem]
 13         os.setgid(gid)
 14         os.setgroups(gids)
 15         os.setuid(uid)
 16         try:
 17             with open(file):
 18                 q.put(True)
 19         except Exception:
 20             q.put(False)
 21
 22     queue = Queue()
 23     p = Process(target=probe_permission, args=(queue, file, user))
 24     p.start()
 25     p.join()
 26     return queue.get()
 27
 28 if __name__ == '__main__':
 29     print probe_file_permission_as_user(sys.argv[1], 'qemu')
"""
The simplest option is the latter one, of just enabling the 
'execute/search' bit. For any directory to be used for storing disk 
images, this can be achieved by running the following command on the 
directory itself, and any parent directories

chmod o+x /path/to/directory

In particular note that if using the "system" instance and attempting to 
store disk images in a user home directory, the default permissions on 
$HOME are typically too restrictive to allow access.
"""


3. have you check add qemu in all the paths group?
and open set g+x?


can you try my patch, seen it can works?
add a method to probe the permission as qemu user
I don't see how the patch listed there will apply. Maybe github
reformats it strangely when you paste it in?

Even so, as I pasted from the libvirt doc, I don't think that will be
enough to make libvirt happy.
save as, and git apply. but I think it will not successfully, for the 
upstream changes so quickly.
Yes,  we should make libvirt and kimchi user happy?
I think it's a security leak to allow someone to put a file in one
directory, and then we give o+x to every parent of it. If I put an ISO
at /home/christy/example.iso and kimchi changed recursively all parents
to o+x, it would give all users the ability to see what's in /home/. 
diff --git a/src/kimchi/vmtemplate.py b/src/kimchi/vmtemplate.py
index af07ee3..76ac772 100644
--- a/src/kimchi/vmtemplate.py
+++ b/src/kimchi/vmtemplate.py
@@ -29,9 +29,9 @@ import urlparse

   from kimchi import isoinfo
   from kimchi import osinfo
-from kimchi.exception import InvalidParameter, IsoFormatError
+from kimchi.exception import InvalidParameter, IsoFormatError, InvalidPermission
   from kimchi.isoinfo import IsoImage
-
+from kimchi.utils import check_iso_path_perm

   QEMU_NAMESPACE = "xmlns:qemu='http://libvirt.org/schemas/domain/qemu/1.0'"

@@ -55,6 +55,9 @@ class VMTemplate(object):
           iso_distro = iso_version = 'unknown'
           iso = args.get('cdrom', '')

+        if iso.startswith('/') and \
+                not check_iso_path_perm(os.path.dirname(iso)):
+            raise InvalidPermission("KCHISO0008E", {'filename': iso})
           if scan and len(iso) > 0:
               iso_distro, iso_version = self.get_iso_info(iso)
               if not iso.startswith('/'):
Regards,

- Christy


        
Regards,

- Christy



-- 
Thanks and best regards!

Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com>
IBM Linux Technology Center