[PATCH 0/9] authorization: Filter resources by users and groups
From: Aline Manera <alinefm@linux.vnet.ibm.com> Also move autorization mechanism to controller to be able to distinguish resource and collection configuration If we use UrlSubNode to also handle the authorization configuration, we won't be able to specify different configuration to collection and its resource as Kimchi uses the same base URL to both. Example: @UrlSubNode("vms", True, ["POST", "PUT", "DELETE"], 'guests') It meant that all the methods listed were exclusive for admin users. Which it is not correct, as a user assigned to a VM can also perform POST, PUT and DELETE actions. So fix it by moving the authorization mechanism to controller Aline Manera (5): authorization: Filter resources by users and groups authorization: Restrict Collection access based on admin_methods parameter authorization: Restrict access to Resource instance authorization: Update control files to set role_key and admin_methods authorization: Remove authorization config from UrlSubNode Crístian Viana (4): Return some groups for every user in mockmodel Move "fake_user" credentials to mockmodel List "admin" as a valid system user in mockmodel authorization: Update test cases based on last changes src/kimchi/auth.py | 16 +---------- src/kimchi/control/base.py | 56 +++++++++++++++++++++++++++++++----- src/kimchi/control/debugreports.py | 8 +++++- src/kimchi/control/host.py | 26 +++++++++++++++-- src/kimchi/control/interfaces.py | 6 +++- src/kimchi/control/networks.py | 6 +++- src/kimchi/control/storagepools.py | 6 +++- src/kimchi/control/storageservers.py | 8 +++++- src/kimchi/control/templates.py | 6 +++- src/kimchi/control/utils.py | 14 +++++---- src/kimchi/control/vms.py | 6 +++- src/kimchi/exception.py | 4 +++ src/kimchi/i18n.py | 1 + src/kimchi/mockmodel.py | 5 +++- src/kimchi/server.py | 4 --- tests/test_authorization.py | 30 +++++++++++++++++-- tests/test_rest.py | 19 ++++++------ tests/utils.py | 9 +++--- 18 files changed, 172 insertions(+), 58 deletions(-) -- 1.9.3
From: Aline Manera <alinefm@linux.vnet.ibm.com> Currently, every user with 'admin' role 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 'admin' role - 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: Aline Manera <alinefm@linux.vnet.ibm.com> Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index f8a5210..572f980 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -22,6 +22,7 @@ import kimchi.template +from kimchi.auth import USER_GROUPS, USER_NAME, USER_ROLES from kimchi.control.utils import get_class_name, internal_redirect, model_fn from kimchi.control.utils import parse_request, validate_method from kimchi.control.utils import validate_params @@ -53,6 +54,8 @@ def __init__(self, model, ident=None): self.ident = ident self.model_args = (ident,) self.update_params = [] + self.role_key = None + self.admin_methods = [] def _redirect(self, ident, code=303): if ident is not None and ident != self.ident: @@ -134,6 +137,22 @@ def index(self): except KimchiException, e: raise cherrypy.HTTPError(500, e.message) + def is_authorized(self): + user_name = cherrypy.session.get(USER_NAME, '') + user_groups = cherrypy.session.get(USER_GROUPS, []) + user_role = cherrypy.session.get(USER_ROLES, {}).get(self.role_key) + + users = self.data.get("users", None) + groups = self.data.get("groups", None) + + if (users is not None or groups is not None) and \ + user_role and user_role != 'admin' and \ + (user_name not in users or \ + (groups and list(set(user_groups) & set(groups)) == [])): + return False + + return True + def update(self): try: update = getattr(self.model, model_fn(self, 'update')) @@ -195,6 +214,8 @@ def __init__(self, model): self.resource = Resource self.resource_args = [] self.model_args = [] + self.role_key = None + self.admin_methods = [] def create(self, params, *args): try: @@ -239,6 +260,9 @@ def _cp_dispatch(self, vpath): def filter_data(self, resources, fields_filter): data = [] for res in resources: + if not res.is_authorized(): + continue + if all(key in res.data and res.data[key] == val for key, val in fields_filter.iteritems()): data.append(res.data) -- 1.9.3
From: Aline Manera <alinefm@linux.vnet.ibm.com> GET and POST are the allowed methods for a Collection If you want to restrict access to the Collection based on the request method, you must set the self.admin_method parameter accordingly Kimchi will restrict all the POST request to a Collection, ie, only the admin can create new resources in the Collection Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 4 +++- src/kimchi/control/utils.py | 8 +++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index 572f980..674c13b 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -284,7 +284,9 @@ def _split_filter(params): @cherrypy.expose def index(self, *args, **kwargs): - method = validate_method(('GET', 'POST')) + method = validate_method(('GET', 'POST'), + self.role_key, self.admin_methods) + try: if method == 'GET': filter_params = cherrypy.request.params diff --git a/src/kimchi/control/utils.py b/src/kimchi/control/utils.py index aa592ef..aa5f452 100644 --- a/src/kimchi/control/utils.py +++ b/src/kimchi/control/utils.py @@ -25,6 +25,7 @@ from jsonschema import Draft3Validator, ValidationError, FormatChecker +from kimchi.auth import USER_ROLES from kimchi.exception import InvalidParameter, OperationFailed from kimchi.utils import import_module, listPathModules @@ -41,10 +42,15 @@ def model_fn(cls, fn_name): return '%s_%s' % (get_class_name(cls), fn_name) -def validate_method(allowed): +def validate_method(allowed, role_key, admin_methods): method = cherrypy.request.method.upper() if method not in allowed: raise cherrypy.HTTPError(405) + + user_role = cherrypy.session.get(USER_ROLES, {}).get(role_key) + if user_role and user_role != 'admin' and method in admin_methods: + raise cherrypy.HTTPError(403) + return method -- 1.9.3
From: Aline Manera <alinefm@linux.vnet.ibm.com> An user only can perform actions to a Resource if it is not protected or if the logged user is listed in the resource users and groups parameters. Also update the tests to reflect those changes. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/control/base.py | 28 ++++++++++++++++++++++------ src/kimchi/control/host.py | 2 +- src/kimchi/exception.py | 4 ++++ src/kimchi/i18n.py | 1 + tests/test_authorization.py | 12 ++++++++++-- tests/test_rest.py | 2 +- 6 files changed, 39 insertions(+), 10 deletions(-) diff --git a/src/kimchi/control/base.py b/src/kimchi/control/base.py index 674c13b..ac24b3f 100644 --- a/src/kimchi/control/base.py +++ b/src/kimchi/control/base.py @@ -27,8 +27,8 @@ from kimchi.control.utils import parse_request, validate_method from kimchi.control.utils import validate_params from kimchi.exception import InvalidOperation, InvalidParameter -from kimchi.exception import KimchiException -from kimchi.exception import MissingParameter, NotFoundError, OperationFailed +from kimchi.exception import KimchiException, MissingParameter, NotFoundError +from kimchi.exception import OperationFailed, UnauthorizedError class Resource(object): @@ -65,8 +65,14 @@ def _redirect(self, ident, code=303): def generate_action_handler(self, action_name, action_args=None): def wrapper(*args, **kwargs): - validate_method(('POST')) + method = validate_method(('POST'), + self.role_key, self.admin_methods) + try: + self.lookup() + if not self.is_authorized(): + raise UnauthorizedError('KCHAPI0009E') + model_args = list(self.model_args) if action_args is not None: request = parse_request() @@ -87,10 +93,12 @@ def wrapper(*args, **kwargs): raise cherrypy.HTTPError(400, e.message) except InvalidOperation, e: raise cherrypy.HTTPError(400, e.message) - except OperationFailed, e: - raise cherrypy.HTTPError(500, e.message) + except UnauthorizedError, e: + raise cherrypy.HTTPError(403, e.message) except NotFoundError, e: raise cherrypy.HTTPError(404, e.message) + except OperationFailed, e: + raise cherrypy.HTTPError(500, e.message) except KimchiException, e: raise cherrypy.HTTPError(500, e.message) @@ -121,8 +129,14 @@ def delete(self): @cherrypy.expose def index(self): - method = validate_method(('GET', 'DELETE', 'PUT')) + method = validate_method(('GET', 'DELETE', 'PUT'), + self.role_key, self.admin_methods) + try: + self.lookup() + if not self.is_authorized(): + raise UnauthorizedError('KCHAPI0009E') + return {'GET': self.get, 'DELETE': self.delete, 'PUT': self.update}[method]() @@ -130,6 +144,8 @@ def index(self): raise cherrypy.HTTPError(400, e.message) except InvalidParameter, e: raise cherrypy.HTTPError(400, e.message) + except UnauthorizedError, e: + raise cherrypy.HTTPError(403, e.message) except NotFoundError, e: raise cherrypy.HTTPError(404, e.message) except OperationFailed, e: diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index e1971cc..c962472 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -42,7 +42,7 @@ def __init__(self, model, id=None): @cherrypy.expose def swupdate(self): - validate_method(('POST')) + validate_method(('POST'), self.role_key, self.admin_methods) try: task = self.model.host_swupdate() cherrypy.response.status = 202 diff --git a/src/kimchi/exception.py b/src/kimchi/exception.py index d84ddb9..9828223 100644 --- a/src/kimchi/exception.py +++ b/src/kimchi/exception.py @@ -92,3 +92,7 @@ class IsoFormatError(KimchiException): class TimeoutExpired(KimchiException): pass + + +class UnauthorizedError(KimchiException): + pass diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py index 0c76145..be6c532 100644 --- a/src/kimchi/i18n.py +++ b/src/kimchi/i18n.py @@ -31,6 +31,7 @@ "KCHAPI0006E": _("Unable to parse JSON request"), "KCHAPI0007E": _("This API only supports JSON"), "KCHAPI0008E": _("Parameters does not match requirement in schema: %(err)s"), + "KCHAPI0009E": _("You don't have permission to perform this operation."), "KCHASYNC0001E": _("Datastore is not initiated in the model object."), "KCHASYNC0002E": _("Unable to start task due error: %(err)s"), diff --git a/tests/test_authorization.py b/tests/test_authorization.py index 03f8a88..3d0b357 100644 --- a/tests/test_authorization.py +++ b/tests/test_authorization.py @@ -116,7 +116,15 @@ def test_nonroot_access(self): self.assertEquals(200, resp.status) resp = self.request('/vms', req, 'POST') self.assertEquals(403, resp.status) - resp = self.request('/vms', '{}', 'PUT') + + # Create a vm using mockmodel directly to test Resource access + model.templates_create({'name': 'test', 'cdrom': '/nonexistent.iso'}) + model.vms_create({'name': 'test', 'template': '/templates/test'}) + + resp = self.request('/vms/test', '{}', 'PUT') self.assertEquals(403, resp.status) - resp = self.request('/vms', '{}', 'DELETE') + resp = self.request('/vms/test', '{}', 'DELETE') self.assertEquals(403, resp.status) + + model.template_delete('test') + model.vm_delete('test') diff --git a/tests/test_rest.py b/tests/test_rest.py index 54209ef..3c8c537 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -1207,7 +1207,7 @@ def verify_template(t, res): # Test nonexistent fields, specify a field 'foo' isn't in the Template t['foo'] = "bar" req = json.dumps(t) - resp = self.request('/templates/%s' % oldname, req, 'PUT') + resp = self.request('/templates/%s' % tmpl_name, req, 'PUT') self.assertEquals(400, resp.status) # Delete the template -- 1.9.3
From: Aline Manera <alinefm@linux.vnet.ibm.com> Each resource or collection must specify the role_key and admin_methods values if it wants to be protected. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/control/debugreports.py | 6 ++++++ src/kimchi/control/host.py | 22 ++++++++++++++++++++++ src/kimchi/control/interfaces.py | 4 ++++ src/kimchi/control/networks.py | 4 ++++ src/kimchi/control/storagepools.py | 4 ++++ src/kimchi/control/storageservers.py | 6 ++++++ src/kimchi/control/templates.py | 4 ++++ src/kimchi/control/vms.py | 4 ++++ 8 files changed, 54 insertions(+) diff --git a/src/kimchi/control/debugreports.py b/src/kimchi/control/debugreports.py index f0d5dcf..a561b99 100644 --- a/src/kimchi/control/debugreports.py +++ b/src/kimchi/control/debugreports.py @@ -27,6 +27,8 @@ class DebugReports(AsyncCollection): def __init__(self, model): super(DebugReports, self).__init__(model) self.resource = DebugReport + self.role_key = 'host' + self.admin_methods = ['GET', 'POST'] def _get_resources(self, filter_params): res_list = super(DebugReports, self)._get_resources(filter_params) @@ -36,6 +38,8 @@ def _get_resources(self, filter_params): class DebugReport(Resource): def __init__(self, model, ident): super(DebugReport, self).__init__(model, ident) + self.role_key = 'host' + self.admin_methods = ['GET', 'PUT', 'POST'] self.update_params = ["name"] self.uri_fmt = '/debugreports/%s' self.content = DebugReportContent(model, ident) @@ -50,6 +54,8 @@ def data(self): class DebugReportContent(Resource): def __init__(self, model, ident): super(DebugReportContent, self).__init__(model, ident) + self.role_key = 'host' + self.admin_methods = ['GET'] def get(self): self.lookup() diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index c962472..1b55a29 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -29,6 +29,8 @@ class Host(Resource): def __init__(self, model, id=None): super(Host, self).__init__(model, id) + self.role_key = 'host' + self.admin_methods = ['GET', 'POST'] self.uri_fmt = '/host/%s' self.reboot = self.generate_action_handler('reboot') self.shutdown = self.generate_action_handler('shutdown') @@ -58,6 +60,8 @@ def data(self): class HostStats(Resource): def __init__(self, model, id=None): super(HostStats, self).__init__(model, id) + self.role_key = 'host' + self.admin_methods = ['GET'] self.history = HostStatsHistory(self.model) @property @@ -74,6 +78,8 @@ def data(self): class Partitions(Collection): def __init__(self, model): super(Partitions, self).__init__(model) + self.role_key = 'storage' + self.admin_methods = ['GET'] self.resource = Partition # Defining get_resources in order to return list of partitions in UI @@ -87,6 +93,8 @@ def _get_resources(self, flag_filter): class Partition(Resource): def __init__(self, model, id): + self.role_key = 'storage' + self.admin_methods = ['GET'] super(Partition, self).__init__(model, id) @property @@ -100,11 +108,15 @@ def data(self): class Devices(Collection): def __init__(self, model): super(Devices, self).__init__(model) + self.role_key = 'storage' + self.admin_methods = ['GET'] self.resource = Device class Device(Resource): def __init__(self, model, id): + self.role_key = 'storage' + self.admin_methods = ['GET'] super(Device, self).__init__(model, id) @property @@ -115,12 +127,16 @@ def data(self): class PackagesUpdate(Collection): def __init__(self, model): super(PackagesUpdate, self).__init__(model) + self.role_key = 'host' + self.admin_methods = ['GET'] self.resource = PackageUpdate class PackageUpdate(Resource): def __init__(self, model, id=None): super(PackageUpdate, self).__init__(model, id) + self.role_key = 'host' + self.admin_methods = ['GET'] @property def data(self): @@ -130,12 +146,16 @@ def data(self): class Repositories(Collection): def __init__(self, model): super(Repositories, self).__init__(model) + self.role_key = 'host' + self.admin_methods = ['GET', 'POST'] self.resource = Repository class Repository(Resource): def __init__(self, model, id): super(Repository, self).__init__(model, id) + self.role_key = 'host' + self.admin_methods = ['GET', 'PUT', 'POST', 'DELETE'] self.update_params = ["config", "baseurl"] self.uri_fmt = "/host/repositories/%s" self.enable = self.generate_action_handler('enable') @@ -149,8 +169,10 @@ def data(self): class Users(SimpleCollection): def __init__(self, model): super(Users, self).__init__(model) + self.role_key = 'guests' class Groups(SimpleCollection): def __init__(self, model): super(Groups, self).__init__(model) + self.role_key = 'guests' diff --git a/src/kimchi/control/interfaces.py b/src/kimchi/control/interfaces.py index 4aa77b6..944cae6 100644 --- a/src/kimchi/control/interfaces.py +++ b/src/kimchi/control/interfaces.py @@ -25,12 +25,16 @@ class Interfaces(Collection): def __init__(self, model): super(Interfaces, self).__init__(model) + self.role_key = 'network' + self.admin_methods = ['GET'] self.resource = Interface class Interface(Resource): def __init__(self, model, ident): super(Interface, self).__init__(model, ident) + self.role_key = 'network' + self.admin_methods = ['GET'] self.uri_fmt = "/interfaces/%s" @property diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index 6bcc871..f7696e7 100644 --- a/src/kimchi/control/networks.py +++ b/src/kimchi/control/networks.py @@ -25,12 +25,16 @@ class Networks(Collection): def __init__(self, model): super(Networks, self).__init__(model) + self.role_key = 'network' + self.admin_methods = ['POST'] self.resource = Network class Network(Resource): def __init__(self, model, ident): super(Network, self).__init__(model, ident) + self.role_key = 'network' + self.admin_methods = ['PUT', 'POST', 'DELETE'] self.uri_fmt = "/networks/%s" self.activate = self.generate_action_handler('activate') self.deactivate = self.generate_action_handler('deactivate') diff --git a/src/kimchi/control/storagepools.py b/src/kimchi/control/storagepools.py index 8c8b522..9e57992 100644 --- a/src/kimchi/control/storagepools.py +++ b/src/kimchi/control/storagepools.py @@ -32,6 +32,8 @@ class StoragePools(Collection): def __init__(self, model): super(StoragePools, self).__init__(model) + self.role_key = 'storage' + self.admin_methods = ['POST'] self.resource = StoragePool isos = IsoPool(model) setattr(self, ISO_POOL_NAME, isos) @@ -73,6 +75,8 @@ def _get_resources(self, filter_params): class StoragePool(Resource): def __init__(self, model, ident): super(StoragePool, self).__init__(model, ident) + self.role_key = 'storage' + self.admin_methods = ['PUT', 'POST', 'DELETE'] self.update_params = ["autostart", "disks"] self.uri_fmt = "/storagepools/%s" self.activate = self.generate_action_handler('activate') diff --git a/src/kimchi/control/storageservers.py b/src/kimchi/control/storageservers.py index 9c7bebc..e86f920 100644 --- a/src/kimchi/control/storageservers.py +++ b/src/kimchi/control/storageservers.py @@ -26,12 +26,16 @@ class StorageServers(Collection): def __init__(self, model): super(StorageServers, self).__init__(model) + self.role_key = 'storage' + self.admin_methods = ['GET'] self.resource = StorageServer class StorageServer(Resource): def __init__(self, model, ident): super(StorageServer, self).__init__(model, ident) + self.role_key = 'storage' + self.admin_methods = ['GET'] self.storagetargets = StorageTargets(self.model, self.ident.decode("utf-8")) @@ -43,6 +47,8 @@ def data(self): class StorageTargets(Collection): def __init__(self, model, server): super(StorageTargets, self).__init__(model) + self.role_key = 'storage' + self.admin_methods = ['GET'] self.server = server self.resource_args = [self.server, ] self.model_args = [self.server, ] diff --git a/src/kimchi/control/templates.py b/src/kimchi/control/templates.py index 907929f..167e19e 100644 --- a/src/kimchi/control/templates.py +++ b/src/kimchi/control/templates.py @@ -25,12 +25,16 @@ class Templates(Collection): def __init__(self, model): super(Templates, self).__init__(model) + self.role_key = 'templates' + self.admin_methods = ['GET', 'POST'] self.resource = Template class Template(Resource): def __init__(self, model, ident): super(Template, self).__init__(model, ident) + self.role_key = 'templates' + self.admin_methods = ['PUT', 'POST', 'DELETE'] self.update_params = ["name", "folder", "icon", "os_distro", "storagepool", "os_version", "cpus", "memory", "cdrom", "disks", "networks", diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index cf427fa..c36d72a 100644 --- a/src/kimchi/control/vms.py +++ b/src/kimchi/control/vms.py @@ -27,11 +27,14 @@ class VMs(Collection): def __init__(self, model): super(VMs, self).__init__(model) self.resource = VM + self.role_key = 'guests' + self.admin_methods = ['POST'] class VM(Resource): def __init__(self, model, ident): super(VM, self).__init__(model, ident) + self.role_key = 'guests' self.update_params = ["name", "users", "groups", "cpus", "memory"] self.screenshot = VMScreenShot(model, ident) self.uri_fmt = '/vms/%s' @@ -51,6 +54,7 @@ def data(self): class VMScreenShot(Resource): def __init__(self, model, ident): super(VMScreenShot, self).__init__(model, ident) + self.role_key = 'guests' def get(self): self.lookup() -- 1.9.3
From: Aline Manera <alinefm@linux.vnet.ibm.com> UrlSubNode is used to automatically load the application configuration and set kimchiauth tool when needed. If we use it to also handle the authorization configuration, we won't be able to specify different configuration to collection and its resource as Kimchi uses the same base URL to both. Example: @UrlSubNode("vms", True, ["POST", "PUT", "DELETE"], 'guests') It meant that all the methods listed were exclusive for admin users. Which it is not correct, as a user assigned to a VM can also perform POST, PUT and DELETE actions. To be able to distinguish the configuration for resource and collection, the autorization mechanism was moved to controller. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/auth.py | 16 +--------------- src/kimchi/control/debugreports.py | 2 +- src/kimchi/control/host.py | 2 +- src/kimchi/control/interfaces.py | 2 +- src/kimchi/control/networks.py | 2 +- src/kimchi/control/storagepools.py | 2 +- src/kimchi/control/storageservers.py | 2 +- src/kimchi/control/templates.py | 2 +- src/kimchi/control/utils.py | 6 +----- src/kimchi/control/vms.py | 2 +- src/kimchi/server.py | 4 ---- 11 files changed, 10 insertions(+), 32 deletions(-) diff --git a/src/kimchi/auth.py b/src/kimchi/auth.py index aabcb6c..93a47b3 100644 --- a/src/kimchi/auth.py +++ b/src/kimchi/auth.py @@ -243,27 +243,13 @@ def logout(): cherrypy.lib.sessions.close() -def has_permission(admin_methods, tab): - cherrypy.session.acquire_lock() - role = cherrypy.session.get(USER_ROLES, {}).get(tab, 'user') - cherrypy.session.release_lock() - - return not admin_methods or \ - cherrypy.request.method not in admin_methods or \ - (cherrypy.request.method in admin_methods and role == "admin") - - -def kimchiauth(admin_methods=None, tab=None): +def kimchiauth(): debug("Entering kimchiauth...") session_missing = cherrypy.session.missing if check_auth_session(): - if not has_permission(admin_methods, tab): - raise cherrypy.HTTPError(403) return if check_auth_httpba(): - if not has_permission(admin_methods, tab): - raise cherrypy.HTTPError(403) return # not a REST full request, redirect login page directly diff --git a/src/kimchi/control/debugreports.py b/src/kimchi/control/debugreports.py index a561b99..debc2eb 100644 --- a/src/kimchi/control/debugreports.py +++ b/src/kimchi/control/debugreports.py @@ -22,7 +22,7 @@ from kimchi.control.utils import UrlSubNode -@UrlSubNode('debugreports', True, ['GET', 'PUT', 'POST', 'DELETE'], 'host') +@UrlSubNode('debugreports', True) class DebugReports(AsyncCollection): def __init__(self, model): super(DebugReports, self).__init__(model) diff --git a/src/kimchi/control/host.py b/src/kimchi/control/host.py index 1b55a29..172f4fe 100644 --- a/src/kimchi/control/host.py +++ b/src/kimchi/control/host.py @@ -25,7 +25,7 @@ from kimchi.template import render -@UrlSubNode('host', True, ['GET', 'PUT', 'POST', 'DELETE'], 'host') +@UrlSubNode('host', True) class Host(Resource): def __init__(self, model, id=None): super(Host, self).__init__(model, id) diff --git a/src/kimchi/control/interfaces.py b/src/kimchi/control/interfaces.py index 944cae6..317cc6f 100644 --- a/src/kimchi/control/interfaces.py +++ b/src/kimchi/control/interfaces.py @@ -21,7 +21,7 @@ from kimchi.control.utils import UrlSubNode -@UrlSubNode('interfaces', True, ['GET'], 'network') +@UrlSubNode('interfaces', True) class Interfaces(Collection): def __init__(self, model): super(Interfaces, self).__init__(model) diff --git a/src/kimchi/control/networks.py b/src/kimchi/control/networks.py index f7696e7..760295c 100644 --- a/src/kimchi/control/networks.py +++ b/src/kimchi/control/networks.py @@ -21,7 +21,7 @@ from kimchi.control.utils import UrlSubNode -@UrlSubNode('networks', True, ['PUT', 'POST', 'DELETE'], 'network') +@UrlSubNode('networks', True) class Networks(Collection): def __init__(self, model): super(Networks, self).__init__(model) diff --git a/src/kimchi/control/storagepools.py b/src/kimchi/control/storagepools.py index 9e57992..c023505 100644 --- a/src/kimchi/control/storagepools.py +++ b/src/kimchi/control/storagepools.py @@ -28,7 +28,7 @@ from kimchi.control.utils import UrlSubNode -@UrlSubNode('storagepools', True, ['PUT', 'POST', 'DELETE'], 'storage') +@UrlSubNode('storagepools', True) class StoragePools(Collection): def __init__(self, model): super(StoragePools, self).__init__(model) diff --git a/src/kimchi/control/storageservers.py b/src/kimchi/control/storageservers.py index e86f920..4b70c39 100644 --- a/src/kimchi/control/storageservers.py +++ b/src/kimchi/control/storageservers.py @@ -22,7 +22,7 @@ from kimchi.control.utils import get_class_name, model_fn, UrlSubNode -@UrlSubNode('storageservers', True, ['GET'], 'storage') +@UrlSubNode('storageservers', True) class StorageServers(Collection): def __init__(self, model): super(StorageServers, self).__init__(model) diff --git a/src/kimchi/control/templates.py b/src/kimchi/control/templates.py index 167e19e..020902d 100644 --- a/src/kimchi/control/templates.py +++ b/src/kimchi/control/templates.py @@ -21,7 +21,7 @@ from kimchi.control.utils import UrlSubNode -@UrlSubNode('templates', True, ['GET', 'PUT', 'POST', 'DELETE'], 'templates') +@UrlSubNode('templates', True) class Templates(Collection): def __init__(self, model): super(Templates, self).__init__(model) diff --git a/src/kimchi/control/utils.py b/src/kimchi/control/utils.py index aa5f452..c39dbd8 100644 --- a/src/kimchi/control/utils.py +++ b/src/kimchi/control/utils.py @@ -113,21 +113,17 @@ def validate_params(params, instance, action): class UrlSubNode(object): - def __init__(self, name, auth=False, admin_methods=None, tab=None): + def __init__(self, name, auth=False): """ admin_methods must be None, or a list containing zero or more of the string values ['GET', 'POST', 'PUT', 'DELETE'] """ self.name = name self.auth = auth - self.tab = tab - self.admin_methods = admin_methods def __call__(self, fun): fun._url_sub_node_name = {"name": self.name} fun.url_auth = self.auth - fun.tab = self.tab - fun.admin_methods = self.admin_methods return fun diff --git a/src/kimchi/control/vms.py b/src/kimchi/control/vms.py index c36d72a..28ad775 100644 --- a/src/kimchi/control/vms.py +++ b/src/kimchi/control/vms.py @@ -22,7 +22,7 @@ from kimchi.control.vm import sub_nodes -@UrlSubNode('vms', True, ['POST', 'PUT', 'DELETE'], 'guests') +@UrlSubNode('vms', True) class VMs(Collection): def __init__(self, model): super(VMs, self).__init__(model) diff --git a/src/kimchi/server.py b/src/kimchi/server.py index b0e9474..3f49f6c 100644 --- a/src/kimchi/server.py +++ b/src/kimchi/server.py @@ -129,10 +129,6 @@ def __init__(self, options): cfg = self.configObj ident = "/%s" % ident cfg[ident] = {'tools.kimchiauth.on': True} - if node.admin_methods: - cfg[ident]['tools.kimchiauth.tab'] = node.tab - cfg[ident][ - 'tools.kimchiauth.admin_methods'] = node.admin_methods self.app = cherrypy.tree.mount(KimchiRoot(model_instance, dev_env), config=self.configObj) -- 1.9.3
From: Crístian Viana <vianac@linux.vnet.ibm.com> In the current mockmodel implementation, the "environment" returns None as the group list for all system users. In order to make the mockmodel environment more dynamic, every user will belong to the same list of groups - instead of none. Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- tests/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/utils.py b/tests/utils.py index 4853b7a..12961cc 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -155,7 +155,7 @@ def patch_auth(sudo=True): """ def _get_groups(self): - return None + return [ 'groupA', 'groupB', 'wheel' ] def _has_sudo(self, result): result.value = sudo -- 1.9.3
From: Crístian Viana <vianac@linux.vnet.ibm.com> The "fake_user" credentials are used along with the mockmodel. Instead of having it declared on a test file, move the declaration to a more generic place. Move the "fake_user" credentials from the test files to the mockmodel. Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 3 +++ tests/test_rest.py | 8 ++++---- tests/utils.py | 7 +++---- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index 0e45d1e..f8e33b7 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -57,6 +57,9 @@ from kimchi.vmtemplate import VMTemplate +fake_user = { 'admin': 'letmein!' } + + class MockModel(object): def __init__(self, objstore_loc=None): self.reset() diff --git a/tests/test_rest.py b/tests/test_rest.py index 3c8c537..935ed81 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -35,7 +35,7 @@ import kimchi.mockmodel import kimchi.server from kimchi.rollbackcontext import RollbackContext -from utils import fake_user, get_free_port, patch_auth, request +from utils import get_free_port, patch_auth, request from utils import run_server @@ -108,7 +108,7 @@ def test_404(self): # We must be authenticated first. Otherwise all requests will return # HTTP:401. Since HTTP Simple Auth is not allowed for text/html, we # need to use the login API and establish a session. - user, pw = fake_user.items()[0] + user, pw = kimchi.mockmodel.fake_user.items()[0] req = json.dumps({'username': user, 'password': pw}) resp = self.request('/login', req, 'POST') self.assertEquals(200, resp.status) @@ -1502,7 +1502,7 @@ def test_auth_unprotected(self): resp = self.request(uri, None, 'HEAD', hdrs) self.assertEquals(200, resp.status) - user, pw = fake_user.items()[0] + user, pw = kimchi.mockmodel.fake_user.items()[0] req = json.dumps({'username': user, 'password': pw}) resp = self.request('/login', req, 'POST', hdrs) self.assertEquals(200, resp.status) @@ -1548,7 +1548,7 @@ def test_auth_session(self): self.assertEquals(401, resp.status) # Execute a login call - user, pw = fake_user.items()[0] + user, pw = kimchi.mockmodel.fake_user.items()[0] req = json.dumps({'username': user, 'password': pw}) resp = self.request('/login', req, 'POST', hdrs) self.assertEquals(200, resp.status) diff --git a/tests/utils.py b/tests/utils.py index 12961cc..f452854 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -32,13 +32,12 @@ from lxml import etree +import kimchi.mockmodel import kimchi.server from kimchi.exception import OperationFailed _ports = {} -fake_user = {'admin': 'letmein!'} - # provide missing unittest decorators and API for python 2.6; these decorators # do not actually work, just avoid the syntax failure if sys.version_info[:2] == (2, 6): @@ -136,7 +135,7 @@ def _request(conn, path, data, method, headers): headers = {'Content-Type': 'application/json', 'Accept': 'application/json'} if 'AUTHORIZATION' not in headers.keys(): - user, pw = fake_user.items()[0] + user, pw = kimchi.mockmodel.fake_user.items()[0] hdr = "Basic " + base64.b64encode("%s:%s" % (user, pw)) headers['AUTHORIZATION'] = hdr conn.request(method, path, data, headers) @@ -162,7 +161,7 @@ def _has_sudo(self, result): def _authenticate(username, password, service="passwd"): try: - return fake_user[username] == password + return kimchi.mockmodel.fake_user[username] == password except KeyError, e: raise OperationFailed("KCHAUTH0001E", {'username': 'username', 'code': e.message}) -- 1.9.3
From: Crístian Viana <vianac@linux.vnet.ibm.com> The mockmodel environment reports a static list of users available on the system. Currently, it does not include the fake user which is used by the mockmodel authentication. In order to make the code consistent, the user which authenticates to the mockmodel environment should be listed as a valid system user. Signed-off-by: Crístian Viana <vianac@linux.vnet.ibm.com> --- src/kimchi/mockmodel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/kimchi/mockmodel.py b/src/kimchi/mockmodel.py index f8e33b7..94d7adf 100644 --- a/src/kimchi/mockmodel.py +++ b/src/kimchi/mockmodel.py @@ -867,7 +867,7 @@ def hoststatshistory_lookup(self, *name): 'net_sent_rate': random.sample(range(4000), 30)} def users_get_list(self): - return ["userA", "userB", "userC"] + return ["userA", "userB", "userC", "admin"] def groups_get_list(self): return ["groupA", "groupB", "groupC", "groupD"] -- 1.9.3
From: Crístian Viana <vianac@linux.vnet.ibm.com> --- tests/test_authorization.py | 20 ++++++++++++++++++-- tests/test_rest.py | 9 ++++++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/tests/test_authorization.py b/tests/test_authorization.py index 3d0b357..2fca62e 100644 --- a/tests/test_authorization.py +++ b/tests/test_authorization.py @@ -111,14 +111,24 @@ def test_nonroot_access(self): resp = self.request('/templates/test', '{}', 'DELETE') self.assertEquals(403, resp.status) - # Non-root users can only get vms + + # Non-root users can only get vms authorized to them + model.templates_create({'name': u'test', 'cdrom': '/nonexistent.iso'}) + + model.vms_create({'name': u'test-me', 'template': '/templates/test'}) + model.vm_update(u'test-me', {'users': [ kimchi.mockmodel.fake_user.keys()[0] ], 'groups': []}) + + model.vms_create({'name': u'test-usera', 'template': '/templates/test'}) + model.vm_update(u'test-usera', {'users': [ 'userA' ], 'groups': []}) + resp = self.request('/vms', '{}', 'GET') self.assertEquals(200, resp.status) + vms_data = json.loads(resp.read()) + self.assertEquals([ u'test-me' ], [ v['name'] for v in vms_data ]) resp = self.request('/vms', req, 'POST') self.assertEquals(403, resp.status) # Create a vm using mockmodel directly to test Resource access - model.templates_create({'name': 'test', 'cdrom': '/nonexistent.iso'}) model.vms_create({'name': 'test', 'template': '/templates/test'}) resp = self.request('/vms/test', '{}', 'PUT') @@ -126,5 +136,11 @@ def test_nonroot_access(self): resp = self.request('/vms/test', '{}', 'DELETE') self.assertEquals(403, resp.status) + # Non-root users can only update VMs authorized by them + resp = self.request('/vms/test-me/start', '{}', 'POST') + self.assertEquals(200, resp.status) + resp = self.request('/vms/test-usera/start', '{}', 'POST') + self.assertEquals(403, resp.status) + model.template_delete('test') model.vm_delete('test') diff --git a/tests/test_rest.py b/tests/test_rest.py index 935ed81..06d9f9e 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -175,10 +175,13 @@ def test_get_vms(self): resp = self.request('/templates', req, 'POST') self.assertEquals(201, resp.status) + test_users = [ 'user1', 'user2', 'root'] + test_groups = [ 'group1', 'group2', 'admin' ] # Now add a couple of VMs to the mock model for i in xrange(10): name = 'vm-%i' % i - req = json.dumps({'name': name, 'template': '/templates/test'}) + req = json.dumps({'name': name, 'template': '/templates/test', + 'users': test_users, 'groups': test_groups}) resp = self.request('/vms', req, 'POST') self.assertEquals(201, resp.status) @@ -188,8 +191,8 @@ def test_get_vms(self): vm = json.loads(self.request('/vms/vm-1').read()) self.assertEquals('vm-1', vm['name']) self.assertEquals('shutoff', vm['state']) - self.assertEquals(['user1', 'user2', 'root'], vm['users']) - self.assertEquals(['group1', 'group2', 'admin'], vm['groups']) + self.assertEquals(test_users, vm['users']) + self.assertEquals(test_groups, vm['groups']) def test_edit_vm(self): req = json.dumps({'name': 'test', 'cdrom': '/nonexistent.iso'}) -- 1.9.3
Reviewed-by: Crístian Viana <vianac@linux.vnet.ibm.com> On 23-07-2014 17:39, alinefm@linux.vnet.ibm.com wrote:
From: Aline Manera <alinefm@linux.vnet.ibm.com>
Also move autorization mechanism to controller to be able to distinguish resource and collection configuration
If we use UrlSubNode to also handle the authorization configuration, we won't be able to specify different configuration to collection and its resource as Kimchi uses the same base URL to both.
Example: @UrlSubNode("vms", True, ["POST", "PUT", "DELETE"], 'guests') It meant that all the methods listed were exclusive for admin users. Which it is not correct, as a user assigned to a VM can also perform POST, PUT and DELETE actions. So fix it by moving the authorization mechanism to controller
Aline Manera (5): authorization: Filter resources by users and groups authorization: Restrict Collection access based on admin_methods parameter authorization: Restrict access to Resource instance authorization: Update control files to set role_key and admin_methods authorization: Remove authorization config from UrlSubNode
Crístian Viana (4): Return some groups for every user in mockmodel Move "fake_user" credentials to mockmodel List "admin" as a valid system user in mockmodel authorization: Update test cases based on last changes
src/kimchi/auth.py | 16 +---------- src/kimchi/control/base.py | 56 +++++++++++++++++++++++++++++++----- src/kimchi/control/debugreports.py | 8 +++++- src/kimchi/control/host.py | 26 +++++++++++++++-- src/kimchi/control/interfaces.py | 6 +++- src/kimchi/control/networks.py | 6 +++- src/kimchi/control/storagepools.py | 6 +++- src/kimchi/control/storageservers.py | 8 +++++- src/kimchi/control/templates.py | 6 +++- src/kimchi/control/utils.py | 14 +++++---- src/kimchi/control/vms.py | 6 +++- src/kimchi/exception.py | 4 +++ src/kimchi/i18n.py | 1 + src/kimchi/mockmodel.py | 5 +++- src/kimchi/server.py | 4 --- tests/test_authorization.py | 30 +++++++++++++++++-- tests/test_rest.py | 19 ++++++------ tests/utils.py | 9 +++--- 18 files changed, 172 insertions(+), 58 deletions(-)
participants (3)
-
Aline Manera -
alinefm＠linux.vnet.ibm.com
-
Crístian Viana