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(a)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

> >> 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 docshttp://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(a)linux.vnet.ibm.com> IBM Linux Technology Center