From ccfc7c24182126cfa33a078d084d307cd6e935e4 Mon Sep 17 00:00:00 2001 From: Hongbin Lu Date: Sun, 15 Apr 2018 19:50:23 +0000 Subject: [PATCH] Use policy for control the 'host' field In before, we manually remove the 'host' field from container if the request was not made by admin users. This patch allows operators to define a policy to decide whether to expose this field. This is more flexible. In addition, the field removal logic is moved to 'format_container' to ensure that the field was removed (by policy) in all HTTP methods (instead of 'GET' method only). The policy is of the form "container:get_one:". For example, if the field is 'host', the corresponding policy would be "container:get_one:host". If there is no policy defined for a field, this field is allowed. Otherwise, the visibility of this field is decided by the defined policy. The policy enforcement logic is extended to allow a field 'might_not_exist'. Change-Id: I40a0bfb4c26b8b46218f36ae0da9043b0f40b10b --- zun/api/controllers/v1/containers.py | 23 ++-- .../controllers/v1/views/containers_view.py | 7 +- zun/common/context.py | 8 +- zun/common/policies/container.py | 23 ++++ zun/common/policy.py | 16 ++- .../api/controllers/v1/test_containers.py | 125 +++++++++++++++++- 6 files changed, 181 insertions(+), 21 deletions(-) diff --git a/zun/api/controllers/v1/containers.py b/zun/api/controllers/v1/containers.py index d9872dfbd..83bb0d0ce 100644 --- a/zun/api/controllers/v1/containers.py +++ b/zun/api/controllers/v1/containers.py @@ -31,6 +31,7 @@ from zun.common import context as zun_context from zun.common import exception from zun.common.i18n import _ from zun.common import name_generator +from zun.common.policies import container as policies from zun.common import policy from zun.common import utils import zun.conf @@ -66,9 +67,10 @@ class ContainerCollection(collection.Collection): @staticmethod def convert_with_links(rpc_containers, limit, url=None, expand=False, **kwargs): + context = pecan.request.context collection = ContainerCollection() collection.containers = \ - [view.format_container(url, p) for p in rpc_containers] + [view.format_container(url, p, context) for p in rpc_containers] collection.next = collection.get_next(limit, url=url, **kwargs) return collection @@ -210,6 +212,8 @@ class ContainersController(base.Controller): filters = {} for filter_key in container_allowed_filters: if filter_key in kwargs: + policy_action = policies.CONTAINER % ('get_one:' + filter_key) + context.can(policy_action, might_not_exist=True) filter_value = kwargs.pop(filter_key) filters[filter_key] = filter_value marker_obj = None @@ -228,9 +232,6 @@ class ContainersController(base.Controller): sort_key, sort_dir, filters=filters) - if not context.is_admin: - for container in containers: - del container.host return ContainerCollection.convert_with_links(containers, limit, url=resource_url, expand=expand, @@ -257,9 +258,8 @@ class ContainersController(base.Controller): except exception.ContainerHostNotUp: raise exception.ServerNotUsable - if not context.is_admin: - del container.host - return view.format_container(pecan.request.host_url, container) + return view.format_container(pecan.request.host_url, container, + context) def _generate_name_for_container(self): """Generate a random name like: zeta-22-container.""" @@ -366,7 +366,8 @@ class ContainersController(base.Controller): pecan.response.location = link.build_url('containers', new_container.uuid) pecan.response.status = 202 - return view.format_container(pecan.request.host_url, new_container) + return view.format_container(pecan.request.host_url, new_container, + context) def _set_default_resource_limit(self, container_dict): # NOTE(kiennt): Default disk size will be set later. @@ -567,7 +568,8 @@ class ContainersController(base.Controller): context = pecan.request.context compute_api = pecan.request.compute_api container = compute_api.container_update(context, container, patch) - return view.format_container(pecan.request.host_url, container) + return view.format_container(pecan.request.host_url, container, + context) @base.Controller.api_version("1.1", "1.13") @pecan.expose('json') @@ -587,7 +589,8 @@ class ContainersController(base.Controller): container.name = name context = pecan.request.context container.save(context) - return view.format_container(pecan.request.host_url, container) + return view.format_container(pecan.request.host_url, container, + context) @pecan.expose('json') @exception.wrap_pecan_controller_exception diff --git a/zun/api/controllers/v1/views/containers_view.py b/zun/api/controllers/v1/views/containers_view.py index 5d1f2d640..a457f4c81 100644 --- a/zun/api/controllers/v1/views/containers_view.py +++ b/zun/api/controllers/v1/views/containers_view.py @@ -14,6 +14,7 @@ import itertools from zun.api.controllers import link +from zun.common.policies import container as policies _basic_keys = ( 'uuid', @@ -49,10 +50,14 @@ _basic_keys = ( ) -def format_container(url, container): +def format_container(url, container, context): def transform(key, value): if key not in _basic_keys: return + # strip the key if it is not allowed by policy + policy_action = policies.CONTAINER % ('get_one:%s' % key) + if not context.can(policy_action, fatal=False, might_not_exist=True): + return if key == 'uuid': yield ('uuid', value) yield ('links', [link.make_link( diff --git a/zun/common/context.py b/zun/common/context.py index 2529fd53f..2398461d1 100644 --- a/zun/common/context.py +++ b/zun/common/context.py @@ -122,7 +122,7 @@ class RequestContext(context.RequestContext): return context - def can(self, action, target=None, fatal=True): + def can(self, action, target=None, fatal=True, might_not_exist=False): """Verifies that the given action is valid on the target in this context. :param action: string representing the action to be checked. @@ -133,6 +133,9 @@ class RequestContext(context.RequestContext): {'project_id': self.project_id, 'user_id': self.user_id} :param fatal: if False, will return False when an exception.NotAuthorized occurs. + :param might_not_exist: If True the policy check is skipped (and the + function returns True) if the specified policy does not exist. + Defaults to false. :raises zun.common.exception.NotAuthorized: if verification fails and fatal is True. @@ -145,7 +148,8 @@ class RequestContext(context.RequestContext): 'user_id': self.user_id} try: - return policy.authorize(self, action, target) + return policy.authorize(self, action, target, + might_not_exist=might_not_exist) except exception.NotAuthorized: if fatal: raise diff --git a/zun/common/policies/container.py b/zun/common/policies/container.py index 98939d527..a80ff1369 100644 --- a/zun/common/policies/container.py +++ b/zun/common/policies/container.py @@ -72,6 +72,29 @@ rules = [ } ] ), + policy.DocumentedRuleDefault( + name=CONTAINER % 'get_one:host', + check_str=base.RULE_ADMIN_API, + description='Retrieve the host field of containers.', + operations=[ + { + 'path': '/v1/containers/{container_ident}', + 'method': 'GET' + }, + { + 'path': '/v1/containers', + 'method': 'GET' + }, + { + 'path': '/v1/containers', + 'method': 'POST' + }, + { + 'path': '/v1/containers/{container_ident}', + 'method': 'PATCH' + } + ] + ), policy.DocumentedRuleDefault( name=CONTAINER % 'get_one_all_projects', check_str=base.RULE_ADMIN_API, diff --git a/zun/common/policy.py b/zun/common/policy.py index 2519730b9..54014a163 100644 --- a/zun/common/policy.py +++ b/zun/common/policy.py @@ -102,7 +102,8 @@ def enforce(context, rule=None, target=None, do_raise=do_raise, exc=exc, *args, **kwargs) -def authorize(context, action, target, do_raise=True, exc=None): +def authorize(context, action, target, do_raise=True, exc=None, + might_not_exist=False): """Verifies that the action is valid on the target in this context. :param context: zun context @@ -115,10 +116,13 @@ def authorize(context, action, target, do_raise=True, exc=None): :param do_raise: if True (the default), raises PolicyNotAuthorized; if False, returns False :param exc: Class of the exception to raise if the check fails. - Any remaining arguments passed to :meth:`authorize` (both - positional and keyword arguments) will be passed to - the exception class. If not specified, - :class:`PolicyNotAuthorized` will be used. + Any remaining arguments passed to :meth:`authorize` (both + positional and keyword arguments) will be passed to + the exception class. If not specified, + :class:`PolicyNotAuthorized` will be used. + :param might_not_exist: If True the policy check is skipped (and the + function returns True) if the specified policy does not exist. + Defaults to false. :raises zun.common.exception.PolicyNotAuthorized: if verification fails and do_raise is True. Or if 'exc' is specified it will raise an @@ -131,6 +135,8 @@ def authorize(context, action, target, do_raise=True, exc=None): credentials = context.to_policy_values() if not exc: exc = exception.PolicyNotAuthorized + if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules): + return True try: result = _ENFORCER.enforce(action, target, credentials, do_raise=do_raise, exc=exc, action=action) diff --git a/zun/tests/unit/api/controllers/v1/test_containers.py b/zun/tests/unit/api/controllers/v1/test_containers.py index acb29c293..4b071b632 100644 --- a/zun/tests/unit/api/controllers/v1/test_containers.py +++ b/zun/tests/unit/api/controllers/v1/test_containers.py @@ -166,6 +166,29 @@ class TestContainerController(api_base.FunctionalTest): content_type='application/json') self.assertEqual(202, response.status_int) + self.assertNotIn('host', response.json) + self.assertTrue(mock_container_create.called) + self.assertTrue(mock_container_create.call_args[1]['run'] is False) + mock_neutron_get_network.assert_called_once() + + @patch('zun.common.context.RequestContext.can') + @patch('zun.network.neutron.NeutronAPI.get_available_network') + @patch('zun.compute.api.API.container_create') + @patch('zun.compute.api.API.image_search') + def test_create_container_by_admin( + self, mock_search, mock_container_create, mock_neutron_get_network, + mock_can): + mock_container_create.side_effect = lambda x, y, **z: y + mock_can.return_value = True + params = ('{"name": "MyDocker", "image": "ubuntu",' + '"command": "env", "memory": "512",' + '"environment": {"key1": "val1", "key2": "val2"}}') + response = self.post('/v1/containers/', + params=params, + content_type='application/json') + + self.assertEqual(202, response.status_int) + self.assertIn('host', response.json) self.assertTrue(mock_container_create.called) self.assertTrue(mock_container_create.call_args[1]['run'] is False) mock_neutron_get_network.assert_called_once() @@ -269,6 +292,7 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual(20, c.get('disk')) self.assertEqual({"Name": "no", "MaximumRetryCount": "0"}, c.get('restart_policy')) + self.assertNotIn('host', c) requested_networks = \ mock_container_create.call_args[1]['requested_networks'] self.assertEqual(1, len(requested_networks)) @@ -328,6 +352,7 @@ class TestContainerController(api_base.FunctionalTest): self.assertIsNone(c.get('runtime')) self.assertIsNone(c.get('hostname')) self.assertEqual({}, c.get('restart_policy')) + self.assertNotIn('host', c) mock_neutron_get_network.assert_called_once() requested_networks = \ mock_container_create.call_args[1]['requested_networks'] @@ -365,6 +390,7 @@ class TestContainerController(api_base.FunctionalTest): # [1] https://bugs.launchpad.net/zun/+bug/1746401 # self.assertEqual(10, c.get('disk')) self.assertEqual({}, c.get('environment')) + self.assertNotIn('host', c) mock_neutron_get_network.assert_called_once() extra_spec = \ mock_container_create.call_args[1]['extra_spec'] @@ -400,6 +426,7 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual('512', c.get('memory')) self.assertEqual({"Name": "no", "MaximumRetryCount": "0"}, c.get('restart_policy')) + self.assertNotIn('host', c) mock_neutron_get_network.assert_called_once() requested_networks = \ mock_container_create.call_args[1]['requested_networks'] @@ -436,6 +463,7 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual('512', c.get('memory')) self.assertEqual({"Name": "no", "MaximumRetryCount": "0"}, c.get('restart_policy')) + self.assertNotIn('host', c) mock_neutron_get_network.assert_called_once() requested_networks = \ mock_container_create.call_args[1]['requested_networks'] @@ -472,6 +500,7 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual('512', c.get('memory')) self.assertEqual({"Name": "unless-stopped", "MaximumRetryCount": "0"}, c.get('restart_policy')) + self.assertNotIn('host', c) mock_neutron_get_network.assert_called_once() requested_networks = \ mock_container_create.call_args[1]['requested_networks'] @@ -516,6 +545,7 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual('512', c.get('memory')) self.assertEqual({"key1": "val1", "key2": "val2"}, c.get('environment')) + self.assertNotIn('host', c) requested_networks = \ mock_container_create.call_args[1]['requested_networks'] self.assertEqual(1, len(requested_networks)) @@ -638,6 +668,7 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual('env', c.get('command')) self.assertEqual('Creating', c.get('status')) self.assertEqual('512', c.get('memory')) + self.assertIn('host', c) requested_networks = \ mock_container_create.call_args[1]['requested_networks'] self.assertEqual(1, len(requested_networks)) @@ -701,6 +732,29 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual(1, len(actual_containers)) self.assertEqual(test_container['uuid'], actual_containers[0].get('uuid')) + self.assertNotIn('host', actual_containers[0]) + + @patch('zun.common.context.RequestContext.can') + @patch('zun.objects.Container.list') + def test_get_all_containers_by_admin(self, mock_container_list, mock_can): + mock_can.return_value = True + test_container = utils.get_test_container() + containers = [objects.Container(self.context, **test_container)] + mock_container_list.return_value = containers + + response = self.get('/v1/containers/') + + mock_container_list.assert_called_once_with(mock.ANY, + 1000, None, 'id', 'asc', + filters={}) + context = mock_container_list.call_args[0][0] + self.assertIs(False, context.all_projects) + self.assertEqual(200, response.status_int) + actual_containers = response.json['containers'] + self.assertEqual(1, len(actual_containers)) + self.assertEqual(test_container['uuid'], + actual_containers[0].get('uuid')) + self.assertIn('host', actual_containers[0]) @patch('zun.common.policy.enforce') @patch('zun.objects.Container.list') @@ -776,6 +830,22 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual(test_container['uuid'], actual_containers[0].get('uuid')) + @patch('zun.objects.Container.list') + def test_get_all_containers_with_filter_disallow( + self, mock_container_list): + test_container = utils.get_test_container() + containers = [objects.Container(self.context, **test_container)] + mock_container_list.return_value = containers + + response = self.get('/v1/containers/?host=fake-name', + expect_errors=True) + self.assertEqual(403, response.status_int) + self.assertEqual('application/json', response.content_type) + rule = 'container:get_one:host' + self.assertEqual( + "Policy doesn't allow %s to be performed." % rule, + response.json['errors'][0]['detail']) + @patch('zun.objects.Container.list') def test_get_all_containers_with_unknown_parameter( self, mock_container_list): @@ -807,12 +877,10 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual(test_container['uuid'], actual_containers[0].get('uuid')) - @patch('zun.common.policy.enforce') @patch('zun.compute.api.API.container_show') @patch('zun.objects.Container.get_by_uuid') def test_get_one_by_uuid(self, mock_container_get_by_uuid, - mock_container_show, mock_policy): - mock_policy.return_value = True + mock_container_show): test_container = utils.get_test_container() test_container_obj = objects.Container(self.context, **test_container) mock_container_get_by_uuid.return_value = test_container_obj @@ -828,6 +896,30 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual(200, response.status_int) self.assertEqual(test_container['uuid'], response.json['uuid']) + self.assertNotIn('host', response.json) + + @patch('zun.common.context.RequestContext.can') + @patch('zun.compute.api.API.container_show') + @patch('zun.objects.Container.get_by_uuid') + def test_get_one_by_admin(self, mock_container_get_by_uuid, + mock_container_show, mock_can): + mock_can.return_value = True + test_container = utils.get_test_container() + test_container_obj = objects.Container(self.context, **test_container) + mock_container_get_by_uuid.return_value = test_container_obj + mock_container_show.return_value = test_container_obj + + response = self.get('/v1/containers/%s/' % test_container['uuid']) + + mock_container_get_by_uuid.assert_called_once_with( + mock.ANY, + test_container['uuid']) + context = mock_container_get_by_uuid.call_args[0][0] + self.assertIs(False, context.all_projects) + self.assertEqual(200, response.status_int) + self.assertEqual(test_container['uuid'], + response.json['uuid']) + self.assertIn('host', response.json) @patch('zun.common.policy.enforce') @patch('zun.compute.api.API.container_show') @@ -874,6 +966,33 @@ class TestContainerController(api_base.FunctionalTest): self.assertEqual(200, response.status_int) self.assertTrue(mock_update.called) + self.assertNotIn('host', response.json) + + @patch('zun.common.context.RequestContext.can') + @patch('zun.objects.ComputeNode.get_by_name') + @patch('zun.compute.api.API.container_update') + @patch('zun.objects.Container.get_by_uuid') + def test_patch_by_admin(self, mock_container_get_by_uuid, mock_update, + mock_computenode, mock_can): + mock_can.return_value = True + test_container = utils.get_test_container() + test_container_obj = objects.Container(self.context, **test_container) + mock_container_get_by_uuid.return_value = test_container_obj + mock_update.return_value = test_container_obj + test_host = utils.get_test_compute_node() + numa = objects.numa.NUMATopology._from_dict(test_host['numa_topology']) + test_host['numa_topology'] = numa + test_host_obj = objects.ComputeNode(self.context, **test_host) + mock_computenode.return_value = test_host_obj + params = {'cpu': 1} + container_uuid = test_container.get('uuid') + response = self.patch_json( + '/containers/%s/' % container_uuid, + params=params) + + self.assertEqual(200, response.status_int) + self.assertTrue(mock_update.called) + self.assertIn('host', response.json) def _action_test(self, test_container_obj, action, ident_field, mock_container_action, status_code, query_param=''):