Re: [Kimchi-devel] [PATCH] Don't allow templates to be created with ISOs that won't be usable.

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&gt; >>>> --- >>>> 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 docs http://libvirt.org/drvqemu.html > says: > it simple to set o+x, means this is the only way. Yes, I think so.

> """ > 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