[PATCH] Don't allow templates to be created with ISOs that won't be usable.
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)) 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('/'): -- 1.8.5.3
Reviewed-by: Aline Manera <alinefm@linux.vnet.ibm.com> On 02/24/2014 09:23 PM, 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)) 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('/'):
The tests are failing with this patch set ====================================================================== ERROR: test_template_integrity (test_model.ModelTests) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/alinefm/kimchi/tests/test_model.py", line 477, in test_template_integrity info = inst.template_lookup('test') File "/home/alinefm/kimchi/src/kimchi/model/templates.py", line 86, in lookup t = self.get_template(name, self.objstore, self.conn) File "/home/alinefm/kimchi/src/kimchi/model/templates.py", line 83, in get_template return LibvirtVMTemplate(params, False, conn) File "/home/alinefm/kimchi/src/kimchi/model/templates.py", line 143, in __init__ VMTemplate.__init__(self, args, scan) File "/home/alinefm/kimchi/src/kimchi/vmtemplate.py", line 59, in __init__ not check_iso_path_perm(os.path.dirname(iso)): File "/home/alinefm/kimchi/src/kimchi/utils.py", line 244, in check_iso_path_perm return os.stat(path).st_mode & stat.S_IXOTH and \ OSError: [Errno 2] No such file or directory: '/tmp/kimchi-iso' ---------------------------------------------------------------------- Ran 157 tests in 129.686s FAILED (errors=1) [25/Feb/2014:11:28:02] ENGINE Waiting for child threads to terminate... make[3]: *** [check-local] Error 1 make[3]: Leaving directory `/home/alinefm/kimchi/tests' make[2]: *** [check-am] Error 2 make[2]: Leaving directory `/home/alinefm/kimchi/tests' make[1]: *** [check] Error 2 make[1]: Leaving directory `/home/alinefm/kimchi/tests' make: *** [check-recursive] Error 1 On 02/24/2014 09:23 PM, 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)) 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('/'):
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?
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?
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
> 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('/'):
--
Thanks and best regards!
Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com>
IBM Linux Technology Center
participants (4)
-
Aline Manera -
Christy Perez -
Crístian Viana -
Sheldon