[Kimchi-devel] [WIP] Filter VMs by users and groups

[Aline Manera]

On 07/11/2014 04:08 PM, Crístian Viana wrote:
> Currently, every user with sudo permissions can perform any operation on
> any virtual machine.
>
> In order to add more security, Kimchi will only allow users listed in the VM
> metadata - along with those with sudo permissions - to be able to perform
> actions on it. A VM may contain a list of system users and groups
> associated with it. If a user is not listed to access a VM, they will not be
> able to see it or to perform any operation on it.
>
> Signed-off-by: Crístian Viana <vianac at linux.vnet.ibm.com>
> ---
>   src/kimchi/control/base.py |  4 +++
>   src/kimchi/model/vms.py    | 63 ++++++++++++++++++++++++++++++++++++++++++++--
>   2 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py
> index f8a5210..6022472 100644
> --- a/src/kimchi/control/base.py
> +++ b/src/kimchi/control/base.py
> @@ -118,6 +118,10 @@ class Resource(object):
>
>       @cherrypy.expose
>       def index(self):
> +        authorized = getattr(self.model, model_fn(self, 'is_authorized'), None)
> +        if authorized is not None and not authorized(*self.model_args):
> +            raise cherrypy.HTTPError(403)
> +
>           method = validate_method(('GET', 'DELETE', 'PUT'))
>           try:
>               return {'GET': self.get,
> diff --git a/src/kimchi/model/vms.py b/src/kimchi/model/vms.py
> index 17bda04..00dcd0f 100644
> --- a/src/kimchi/model/vms.py
> +++ b/src/kimchi/model/vms.py
> @@ -19,16 +19,19 @@
>
>   from lxml.builder import E
>   import lxml.etree as ET
> +import grp
>   import os
>   import time
>   import uuid
>   from xml.etree import ElementTree
>
> +import cherrypy
>   import libvirt
>   from cherrypy.process.plugins import BackgroundTask
>
>   from kimchi import vnc
>   from kimchi import xmlutils
> +from kimchi.auth import USER_NAME, USER_SUDO
>   from kimchi.config import READONLY_POOL_TYPE
>   from kimchi.exception import InvalidOperation, InvalidParameter
>   from kimchi.exception import NotFoundError, OperationFailed
> @@ -232,8 +235,8 @@ class VMsModel(object):
>
>       @staticmethod
>       def get_vms(conn):
> -        conn = conn.get()
> -        names = [dom.name().decode('utf-8') for dom in conn.listAllDomains(0)]
> +        libvirtconn = conn.get()
> +        names = [dom.name().decode('utf-8') for dom in libvirtconn.listAllDomains(0) if VMModel._vm_is_authorized(dom.name(), conn)]

80 characters

>           return sorted(names, key=unicode.lower)
>
>
> @@ -526,6 +529,62 @@ class VMModel(object):
>               kimchi_log.error('Error trying to delete vm screenshot from '
>                                'database due error: %s', e.message)
>


> +    @staticmethod
> +    def _vm_get_access_users(name, conn):
> +        dom = conn.get().lookupByName(name)
> +        metadata = get_metadata_node(dom, 'access')
> +        if metadata == '':
> +            return []
> +
> +        users_xml = ET.fromstring(metadata).find('user')
> +        return [] if users_xml is None else [ users_xml.text ]
> +
> +    @staticmethod
> +    def _vm_get_access_groups(name, conn):
> +        dom = conn.get().lookupByName(name)
> +        metadata = get_metadata_node(dom, 'access')
> +        if metadata == '':
> +            return []
> +
> +        groups_xml = ET.fromstring(metadata).find('group')
> +        return [] if groups_xml is None else [ groups_xml.text ]
> +

Those 2 functions are equals!
I recommend to create just one that works in both case.
Example:

@staticmethod
def _vm_get_access_elements(name, conn, element):
	dom = conn.get().lookupByName(name)
         metadata = get_metadata_node(dom, 'access')
	if metadata == '';
	    return []

	xml = ET.fromstring(metadata)find(element)
         return [] if xml is None else [xml.text]

And then use:
_vm_get_access_elements(<my-vm>, <conn>, "user") 
_vm_get_access_elements(<my-vm>, <conn>, "group")


> +    @staticmethod
> +    def _vm_is_authorized(name, conn):
> +        try:
> +            user_name = cherrypy.session.get(USER_NAME, None)
> +            user_sudo = cherrypy.session.get(USER_SUDO, False)

According to last discussion, we decided to don't use USER_SUDO and 
instead of of that, use USER_ROLES

A user will be a role per tab, so it'd be better to check 
USER_ROLES['guests'] == 'admin' as in future we will support user role 
changes, ie, a non-root user can be a guest admin

> +        except AttributeError:
> +            return False
> +
> +        if user_sudo:
> +            return True
> +
> +        dom = conn.get().lookupByName(name)
> +        metadata = get_metadata_node(dom, 'access')
> +
> +        if user_name is None or metadata == '':
> +            return True
> +
> +        users = VMModel._vm_get_access_users(name, conn)
> +        groups = VMModel._vm_get_access_groups(name, conn)
> +
> +        users_in_groups = set()
> +        for vm_g in groups:
> +            try:
> +                for u in grp.getgrnam(vm_g).gr_mem:
> +                    users_in_groups.add(u)
> +            except KeyError:
> +                kimchi_log.warning("group '%s' is listed as authorized by VM '%s' but it doesn't exist"
> +                        % (vm_g, name))
> +
> +        if user_name in users or user_name in users_in_groups:
> +            return True
> +
> +        return False
> +
> +    def is_authorized(self, name):
> +        return VMModel._vm_is_authorized(name, self.conn)
>
>   class VMScreenshotModel(object):
>       def __init__(self, **kargs):
>