[RFC][PATCH V4 0/4] Issue #322

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> V3 -> V4 update test case V2 -> V3 sort the import. V1 -> V2 use debug to log the open exception trace stack. It is complex to check the permission for qemu user. There are too many access controlling mechanisms in Linux, such as ACL, traditional ugo+-rwx, SELinux and AppArmor. It's not possible to enumerate and check every access mechanisms if it allows QEMU to access a file. So now I'm simply trying to access the file with qemu user and see if it's OK It is simple and avoid handling corner case. Also I can try to start a simple guest with the given iso. ShaoHe Feng (4): add a method to probe the permission as qemu user qemu user tests: probe the username of qemu process started by libvirt Don't allow templates to be created with ISOs that won't be usable. probe iso permission: update test case Makefile.am | 1 + src/kimchi/i18n.py | 5 ++++ src/kimchi/kvmusertests.py | 64 +++++++++++++++++++++++++++++++++++++++++++ src/kimchi/model/templates.py | 13 ++++++++- src/kimchi/utils.py | 26 ++++++++++++++++++ tests/test_model.py | 38 ++++++++++++++++--------- 6 files changed, 133 insertions(+), 14 deletions(-) create mode 100644 src/kimchi/kvmusertests.py -- 1.8.4.2

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Now we need to check the 'qemu' user can open an iso files. This patch is used to check 'qemu' user has permission to open a file. Test this patch: $ mkdir -p a/b/c $ touch a/b/c/f $ chmod o-x a/b/c $ sudo PYTHONPATH=src python -c ' from kimchi.utils import probe_file_permission_as_user print probe_file_permission_as_user("a/b/c/f", "qemu")' It will return False change another user, it may return True Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/utils.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 7b15d7f..6c29e0e 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -19,11 +19,15 @@ # import cherrypy +import grp import os import psutil +import pwd import re import subprocess +import traceback import urllib2 +from multiprocessing import Process, Queue from threading import Timer from cherrypy.lib.reprconf import Parser @@ -236,3 +240,25 @@ 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 probe_file_permission_as_user(file, user): + def probe_permission(q, file, user): + uid = pwd.getpwnam(user).pw_uid + gid = pwd.getpwnam(user).pw_gid + gids = [g.gr_gid for g in grp.getgrall() if user in g.gr_mem] + os.setgid(gid) + os.setgroups(gids) + os.setuid(uid) + try: + with open(file): + q.put((True, None)) + except Exception as e: + kimchi_log.debug(traceback.format_exc()) + q.put((False, e)) + + queue = Queue() + p = Process(target=probe_permission, args=(queue, file, user)) + p.start() + p.join() + return queue.get() -- 1.8.4.2

Reviewed-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> on 2014/03/11 14:08, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
Now we need to check the 'qemu' user can open an iso files.
This patch is used to check 'qemu' user has permission to open a file.
Test this patch: $ mkdir -p a/b/c $ touch a/b/c/f $ chmod o-x a/b/c $ sudo PYTHONPATH=src python -c ' from kimchi.utils import probe_file_permission_as_user print probe_file_permission_as_user("a/b/c/f", "qemu")'
It will return False change another user, it may return True
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Royce Lv <lvroyce@linux.vnet.ibm.com> --- src/kimchi/utils.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+)
diff --git a/src/kimchi/utils.py b/src/kimchi/utils.py index 7b15d7f..6c29e0e 100644 --- a/src/kimchi/utils.py +++ b/src/kimchi/utils.py @@ -19,11 +19,15 @@ #
import cherrypy +import grp import os import psutil +import pwd import re import subprocess +import traceback import urllib2 +from multiprocessing import Process, Queue from threading import Timer
from cherrypy.lib.reprconf import Parser @@ -236,3 +240,25 @@ 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 probe_file_permission_as_user(file, user): + def probe_permission(q, file, user): + uid = pwd.getpwnam(user).pw_uid + gid = pwd.getpwnam(user).pw_gid + gids = [g.gr_gid for g in grp.getgrall() if user in g.gr_mem] + os.setgid(gid) + os.setgroups(gids) + os.setuid(uid) + try: + with open(file): + q.put((True, None)) + except Exception as e: + kimchi_log.debug(traceback.format_exc()) + q.put((False, e)) + + queue = Queue() + p = Process(target=probe_permission, args=(queue, file, user)) + p.start() + p.join() + return queue.get()
-- Thanks and best regards! Zhou Zheng Sheng / 周征晟 E-mail: zhshzhou@linux.vnet.ibm.com Telephone: 86-10-82454397

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> We want to fix the searchable permission for qemu user. But we should find the username of qemu process firstly. searchable permission is a known problem. We have discussed several times on IRC. Royce reports the qemu username is different on different distros Zhou Zheng Sheng gives a better way to find the qemu process ID Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- Makefile.am | 1 + src/kimchi/kvmusertests.py | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 src/kimchi/kvmusertests.py diff --git a/Makefile.am b/Makefile.am index 8442771..2fdafb1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -50,6 +50,7 @@ PEP8_WHITELIST = \ src/kimchi/featuretests.py \ src/kimchi/iscsi.py \ src/kimchi/isoinfo.py \ + src/kimchi/kvmusertests.py \ src/kimchi/mockmodel.py \ src/kimchi/model/*.py \ src/kimchi/osinfo.py \ diff --git a/src/kimchi/kvmusertests.py b/src/kimchi/kvmusertests.py new file mode 100644 index 0000000..1914a94 --- /dev/null +++ b/src/kimchi/kvmusertests.py @@ -0,0 +1,64 @@ +# Project Kimchi +# +# Copyright IBM, Corp. 2013 +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library 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 +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import psutil +import uuid + + +import libvirt + + +from kimchi.rollbackcontext import RollbackContext + + +class UserTests(object): + SIMPLE_VM_XML = """ + <domain type='kvm'> + <name>%s</name> + <uuid>%s</uuid> + <memory unit='KiB'>10240</memory> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + </domain>""" + + def __init__(self): + self.vm_uuid = uuid.uuid3(uuid.NAMESPACE_DNS, 'vm-test.kimchi.org') + self.vm_name = "kimchi_test_%s" % self.vm_uuid + + def probe_user(self): + xml = self.SIMPLE_VM_XML % (self.vm_name, self.vm_uuid) + user = None + with RollbackContext() as rollback: + conn = libvirt.open('qemu:///system') + rollback.prependDefer(conn.close) + dom = conn.defineXML(xml) + rollback.prependDefer(dom.undefine) + dom.create() + rollback.prependDefer(dom.destroy) + with open('/var/run/libvirt/qemu/%s.pid' % self.vm_name) as f: + pidStr = f.read() + p = psutil.Process(int(pidStr)) + user = p.username + return user + + +if __name__ == '__main__': + ut = UserTests() + print ut.probe_user() -- 1.8.4.2

Reviewed-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> on 2014/03/11 14:08, shaohef@linux.vnet.ibm.com wrote:
From: ShaoHe Feng <shaohef@linux.vnet.ibm.com>
We want to fix the searchable permission for qemu user. But we should find the username of qemu process firstly.
searchable permission is a known problem. We have discussed several times on IRC.
Royce reports the qemu username is different on different distros
Zhou Zheng Sheng gives a better way to find the qemu process ID
Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Signed-off-by: Zhou Zheng Sheng <zhshzhou@linux.vnet.ibm.com> --- Makefile.am | 1 + src/kimchi/kvmusertests.py | 64 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 src/kimchi/kvmusertests.py
diff --git a/Makefile.am b/Makefile.am index 8442771..2fdafb1 100644 --- a/Makefile.am +++ b/Makefile.am @@ -50,6 +50,7 @@ PEP8_WHITELIST = \ src/kimchi/featuretests.py \ src/kimchi/iscsi.py \ src/kimchi/isoinfo.py \ + src/kimchi/kvmusertests.py \ src/kimchi/mockmodel.py \ src/kimchi/model/*.py \ src/kimchi/osinfo.py \ diff --git a/src/kimchi/kvmusertests.py b/src/kimchi/kvmusertests.py new file mode 100644 index 0000000..1914a94 --- /dev/null +++ b/src/kimchi/kvmusertests.py @@ -0,0 +1,64 @@ +# Project Kimchi +# +# Copyright IBM, Corp. 2013 +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library 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 +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import psutil +import uuid + + +import libvirt + + +from kimchi.rollbackcontext import RollbackContext + + +class UserTests(object): + SIMPLE_VM_XML = """ + <domain type='kvm'> + <name>%s</name> + <uuid>%s</uuid> + <memory unit='KiB'>10240</memory> + <os> + <type arch='x86_64' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + </domain>""" + + def __init__(self): + self.vm_uuid = uuid.uuid3(uuid.NAMESPACE_DNS, 'vm-test.kimchi.org') + self.vm_name = "kimchi_test_%s" % self.vm_uuid + + def probe_user(self): + xml = self.SIMPLE_VM_XML % (self.vm_name, self.vm_uuid) + user = None + with RollbackContext() as rollback: + conn = libvirt.open('qemu:///system') + rollback.prependDefer(conn.close) + dom = conn.defineXML(xml) + rollback.prependDefer(dom.undefine) + dom.create() + rollback.prependDefer(dom.destroy) + with open('/var/run/libvirt/qemu/%s.pid' % self.vm_name) as f: + pidStr = f.read() + p = psutil.Process(int(pidStr)) + user = p.username + return user + + +if __name__ == '__main__': + ut = UserTests() + print ut.probe_user()
-- Thanks and best regards! Zhou Zheng Sheng / 周征晟 E-mail: zhshzhou@linux.vnet.ibm.com Telephone: 86-10-82454397

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> qemu can't use an ISO if it has no search permission on the directories containing the file. Return an error explaning this, and suggest the way to fix it. http://libvirt.org/drvqemu.html#securitydac In the "session" instance, the POSIX users/groups model restricts QEMU virtual machines (and libvirtd in general) to only have access to resources with the same user/group ID as the client application. There is no finer level of configuration possible for the "session" instances. If QEMU virtual machines from the "system" instance are being run as non-root, there will be greater restrictions on what host resources the QEMU process will be able to access. The libvirtd daemon will attempt to manage permissions on resources to minimise the likelihood of unintentional security denials Any files/devices used as guest disk images must be accessible to the user/group ID that QEMU guests are configured to run as. The libvirtd daemon will automatically set the ownership of the file/device path to the correct user/group ID. Applications / administrators must be aware though that the parent directory permissions may still deny access. 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'. 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. Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- src/kimchi/i18n.py | 5 +++++ src/kimchi/model/templates.py | 13 ++++++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 3108921..1ae3889 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -54,6 +54,11 @@ 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 set the search permission " + "to file access control lists for '%(user)s' user if possible, or add the " + "'%(user)s' to the ISO path group, or (not recommended) 'chmod -R o+x 'path_to_iso'." + "Details: %(err)s" ), "KCHVM0001E": _("Virtual machine %(name)s already exists"), "KCHVM0002E": _("Virtual machine %(name)s does not exist"), diff --git a/src/kimchi/model/templates.py b/src/kimchi/model/templates.py index 2e66a94..5376b6c 100644 --- a/src/kimchi/model/templates.py +++ b/src/kimchi/model/templates.py @@ -25,7 +25,9 @@ import libvirt from kimchi import xmlutils from kimchi.exception import InvalidOperation, InvalidParameter +from kimchi.kvmusertests import UserTests from kimchi.utils import pool_name_from_uri +from kimchi.utils import probe_file_permission_as_user from kimchi.vmtemplate import VMTemplate @@ -36,8 +38,17 @@ class TemplatesModel(object): def create(self, params): name = params.get('name', '').strip() + iso = params['cdrom'] + # check search permission + if iso.startswith('/') and os.path.isfile(iso): + user = UserTests().probe_user() + ret, excp = probe_file_permission_as_user(iso, user) + if ret is False: + raise InvalidParameter('KCHISO0008E', + {'filename': iso, 'user': user, + 'err': excp}) + if not name: - iso = params['cdrom'] iso_name = os.path.splitext(iso[iso.rfind('/') + 1:])[0] name = iso_name + str(int(time.time() * 1000)) params['name'] = name -- 1.8.4.2

From: ShaoHe Feng <shaohef@linux.vnet.ibm.com> Update test_model.py The src/kimchi/API.json says the cdrom is require for templates_create. Signed-off-by: ShaoHe Feng <shaohef@linux.vnet.ibm.com> --- tests/test_model.py | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index d661247..02944d3 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -44,9 +44,15 @@ from kimchi.utils import add_task class ModelTests(unittest.TestCase): def setUp(self): self.tmp_store = '/tmp/kimchi-store-test' + self.iso_path = '/tmp/kimchi-model-iso/' + if not os.path.exists(self.iso_path): + os.makedirs(self.iso_path) + self.kimchi_iso = self.iso_path + 'ubuntu12.04.iso' + iso_gen.construct_fake_iso(self.kimchi_iso, True, '12.04', 'ubuntu') def tearDown(self): os.unlink(self.tmp_store) + shutil.rmtree(self.iso_path) def test_vm_info(self): inst = model.Model('test:///default', self.tmp_store) @@ -73,7 +79,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: - params = {'name': 'test', 'disks': []} + params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') @@ -96,7 +102,7 @@ class ModelTests(unittest.TestCase): @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') def test_vm_graphics(self): inst = model.Model(objstore_loc=self.tmp_store) - params = {'name': 'test', 'disks': []} + params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso} inst.templates_create(params) with RollbackContext() as rollback: params = {'name': 'kimchi-vnc', 'template': '/templates/test'} @@ -123,7 +129,7 @@ class ModelTests(unittest.TestCase): def test_vm_ifaces(self): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: - params = {'name': 'test', 'disks': []} + params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') params = {'name': 'kimchi-ifaces', 'template': '/templates/test'} @@ -165,7 +171,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: vm_name = 'kimchi-cdrom' - params = {'name': 'test', 'disks': []} + params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') params = {'name': vm_name, 'template': '/templates/test'} @@ -232,7 +238,8 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: - params = {'name': 'test', 'disks': [{'size': 1}]} + params = {'name': 'test', 'disks': [{'size': 1}], + 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') @@ -376,7 +383,8 @@ class ModelTests(unittest.TestCase): if not os.path.exists(path): os.mkdir(path) - params = {'name': 'test', 'disks': [{'size': 1}]} + params = {'name': 'test', 'disks': [{'size': 1}], + 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') @@ -433,7 +441,8 @@ class ModelTests(unittest.TestCase): inst.networks_create(net_args) rollback.prependDefer(inst.network_delete, net_name) - params = {'name': 'test', 'memory': 1024, 'cpus': 1} + params = {'name': 'test', 'memory': 1024, 'cpus': 1, + 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') info = inst.template_lookup('test') @@ -488,7 +497,8 @@ class ModelTests(unittest.TestCase): inst = model.Model('qemu:///system', objstore_loc=self.tmp_store) with RollbackContext() as rollback: - orig_params = {'name': 'test-template', 'memory': 1024, 'cpus': 1} + orig_params = {'name': 'test-template', 'memory': 1024, + 'cpus': 1, 'cdrom': self.kimchi_iso} inst.templates_create(orig_params) orig_temp = inst.template_lookup(orig_params['name']) @@ -511,7 +521,8 @@ class ModelTests(unittest.TestCase): inst.networks_create(net_args) rollback.prependDefer(inst.network_delete, net_name) - orig_params = {'name': 'test', 'memory': 1024, 'cpus': 1} + orig_params = {'name': 'test', 'memory': 1024, 'cpus': 1, + 'cdrom': self.kimchi_iso} inst.templates_create(orig_params) params = {'name': 'new-test'} @@ -543,7 +554,8 @@ class ModelTests(unittest.TestCase): inst = model.Model('qemu:///system', objstore_loc=self.tmp_store) - orig_params = {'name': 'test', 'memory': '1024', 'cpus': '1'} + orig_params = {'name': 'test', 'memory': '1024', 'cpus': '1', + 'cdrom': self.kimchi_iso} inst.templates_create(orig_params) with RollbackContext() as rollback: @@ -782,7 +794,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: - params = {'name': u'test', 'disks': []} + params = {'name': u'test', 'disks': [], 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') @@ -805,7 +817,7 @@ class ModelTests(unittest.TestCase): inst = model.Model(objstore_loc=self.tmp_store) with RollbackContext() as rollback: - params = {'name': 'test', 'disks': []} + params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso} inst.templates_create(params) rollback.prependDefer(inst.template_delete, 'test') @@ -822,7 +834,7 @@ class ModelTests(unittest.TestCase): objstore_loc=self.tmp_store) with RollbackContext() as rollback: - params = {'name': 'test', 'disks': [], + params = {'name': 'test', 'disks': [], 'cdrom': self.kimchi_iso, 'storagepool': '/storagepools/default-pool', 'domain': 'test', 'arch': 'i686'} -- 1.8.4.2
participants (3)
-
Aline Manera
-
shaohef@linux.vnet.ibm.com
-
Zhou Zheng Sheng