diff --git a/doc/source/contributor/internals/policy.rst b/doc/source/contributor/internals/policy.rst index 636a00a30b5..1f0d3de1495 100644 --- a/doc/source/contributor/internals/policy.rst +++ b/doc/source/contributor/internals/policy.rst @@ -180,7 +180,13 @@ overrides the generic check performed by oslo_policy in this case. It uses for those cases where neutron needs to check whether the project submitting a request for a new resource owns the parent resource of the one being created. Current usages of ``OwnerCheck`` include, for instance, -creating and updating a subnet. +creating and updating a subnet. This class supports the extension parent +resources owner check which the parent resource introduced by +service plugins. Such as router and floatingip owner check for ``router`` +service plugin. Developers can register the extension resource name and service +plugin name which were registered in neutron-lib into +``EXT_PARENT_RESOURCE_MAPPING`` which is located in +``neutron.common.constants``. The check, performed in the ``__call__`` method, works as follows: @@ -192,7 +198,9 @@ The check, performed in the ``__call__`` method, works as follows: * if the previous check failed, extract a parent resource type and a parent field name from the target field. For instance ``networks:tenant_id`` identifies the ``tenant_id`` attribute of the - ``network`` resource; + ``network`` resource. For extension parent resource case, + ``ext_parent:tenant_id`` identifies the ``tenant_id`` attribute of the + registered extension resource in ``EXT_PARENT_RESOURCE_MAPPING``; * if no parent resource or target field could be identified raise a ``PolicyCheckError`` exception; * Retrieve a 'parent foreign key' from the ``_RESOURCE_FOREIGN_KEYS`` data diff --git a/etc/policy.json b/etc/policy.json index 9093c6abe77..b643aa692ff 100644 --- a/etc/policy.json +++ b/etc/policy.json @@ -13,6 +13,7 @@ "shared_address_scopes": "field:address_scopes:shared=True", "external": "field:networks:router:external=True", "default": "rule:admin_or_owner", + "admin_or_ext_parent_owner": "rule:context_is_admin or tenant_id:%(ext_parent:tenant_id)s", "create_subnet": "rule:admin_or_network_owner", "create_subnet:segment_id": "rule:admin_only", @@ -250,9 +251,9 @@ "update_log": "rule:admin_only", "delete_log": "rule:admin_only", - "create_floatingip_port_forwarding": "rule:regular_user", - "get_floatingip_port_forwarding": "rule:regular_user", - "get_floatingip_port_forwardings": "rule:regular_user", - "update_floatingip_port_forwarding": "rule:regular_user", - "delete_floatingip_port_forwarding": "rule:regular_user" + "create_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner", + "get_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner", + "get_floatingip_port_forwardings": "rule:admin_or_ext_parent_owner", + "update_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner", + "delete_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner" } diff --git a/neutron/api/v2/base.py b/neutron/api/v2/base.py index 6dd6211ca8e..eb5874ce14a 100644 --- a/neutron/api/v2/base.py +++ b/neutron/api/v2/base.py @@ -310,12 +310,15 @@ class Controller(object): # FIXME(salvatore-orlando): obj_getter might return references to # other resources. Must check authZ on them too. # Omit items from list that should not be visible - obj_list = [obj for obj in obj_list - if policy.check(request.context, - self._plugin_handlers[self.SHOW], - obj, - plugin=self._plugin, - pluralized=self._collection)] + tmp_list = [] + for obj in obj_list: + self._set_parent_id_into_ext_resources_request( + request, obj, parent_id, is_get=True) + if policy.check( + request.context, self._plugin_handlers[self.SHOW], + obj, plugin=self._plugin, pluralized=self._collection): + tmp_list.append(obj) + obj_list = tmp_list # Use the first element in the list for discriminating which attributes # should be filtered out because of authZ policies # fields_to_add contains a list of attributes added for request policy @@ -346,6 +349,8 @@ class Controller(object): kwargs[self._parent_id_name] = parent_id obj_getter = getattr(self._plugin, action) obj = obj_getter(request.context, id, **kwargs) + self._set_parent_id_into_ext_resources_request( + request, obj, parent_id, is_get=True) # Check authz # FIXME(salvatore-orlando): obj_getter might return references to # other resources. Must check authZ on them too. @@ -457,6 +462,11 @@ class Controller(object): for item in items: self._validate_network_tenant_ownership(request, item[self._resource]) + # For ext resources policy check, we support two types, such as + # parent_id is in request body, another type is parent_id is in + # request url, which we can get from kwargs. + self._set_parent_id_into_ext_resources_request( + request, item[self._resource], parent_id) policy.enforce(request.context, action, item[self._resource], @@ -632,6 +642,10 @@ class Controller(object): # Ensure policy engine is initialized policy.init() parent_id = kwargs.get(self._parent_id_name) + # If the parent_id exist, we should get orig_obj with + # self._parent_id_name field. + if parent_id and self._parent_id_name not in field_list: + field_list.append(self._parent_id_name) orig_obj = self._item(request, id, field_list=field_list, parent_id=parent_id) orig_object_copy = copy.copy(orig_obj) @@ -640,6 +654,10 @@ class Controller(object): # which attributes are set explicitly so that it can distinguish them # from the ones that are set to their default values. orig_obj[n_const.ATTRIBUTES_TO_UPDATE] = body[self._resource].keys() + # Then get the ext_parent_id, format to ext_parent_parent_resource_id + if self._parent_id_name in orig_obj: + self._set_parent_id_into_ext_resources_request( + request, orig_obj, parent_id) try: policy.enforce(request.context, action, @@ -759,6 +777,37 @@ class Controller(object): msg = _('The resource could not be found.') raise webob.exc.HTTPNotFound(msg) + def _set_parent_id_into_ext_resources_request( + self, request, resource_item, parent_id, is_get=False): + if not parent_id: + return + + # This will pass most create/update/delete cases + if not is_get and (request.context.is_admin or + request.context.is_advsvc or + self.parent['member_name'] not in + n_const.EXT_PARENT_RESOURCE_MAPPING or + resource_item.get(self._parent_id_name)): + return + + # Then we arrive here, that means the request or get obj contains + # ext_parent. If this func is called by list/get, and it contains + # _parent_id_name. We need to re-add the ex_parent prefix to policy. + if is_get: + if (not request.context.is_admin or + not request.context.is_advsvc and + self.parent['member_name'] in + n_const.EXT_PARENT_RESOURCE_MAPPING): + resource_item.setdefault( + "%s_%s" % (n_const.EXT_PARENT_PREFIX, + self._parent_id_name), + parent_id) + # If this func is called by create/update/delete, we just add. + else: + resource_item.setdefault( + "%s_%s" % (n_const.EXT_PARENT_PREFIX, self._parent_id_name), + parent_id) + def create_resource(collection, resource, plugin, params, allow_bulk=False, member_actions=None, parent=None, allow_pagination=False, diff --git a/neutron/common/constants.py b/neutron/common/constants.py index 1fb25a4c5df..ca91f3623fb 100644 --- a/neutron/common/constants.py +++ b/neutron/common/constants.py @@ -13,8 +13,9 @@ # See the License for the specific language governing permissions and # limitations under the License. +from neutron_lib.api.definitions import l3 from neutron_lib import constants as lib_constants - +from neutron_lib.plugins import constants as plugin_consts ROUTER_PORT_OWNERS = lib_constants.ROUTER_INTERFACE_OWNERS_SNAT + \ (lib_constants.DEVICE_OWNER_ROUTER_GW,) @@ -230,3 +231,16 @@ IEC_BASE = 1024 # Port bindings handling NO_ACTIVE_BINDING = 'no_active_binding' + +# Registered extension parent resource check mapping +# If we want to register some service plugin resources into policy and check +# the owner when operating their subresources. We can write here to use +# existing policy engine for parent resource owner check. +# Each entry here should be PARENT_RESOURCE_NAME: SERVICE_PLUGIN_NAME, +# PARENT_RESOURCE_NAME is usually from api definition. +# SERVICE_PLUGIN_NAME is the service plugin which introduced the resource and +# registered the service plugin name in neutron-lib. +EXT_PARENT_RESOURCE_MAPPING = { + l3.FLOATINGIP: plugin_consts.L3 +} +EXT_PARENT_PREFIX = 'ext_parent' diff --git a/neutron/policy.py b/neutron/policy.py index 995c206c72b..e6f1eb5ccae 100644 --- a/neutron/policy.py +++ b/neutron/policy.py @@ -224,7 +224,11 @@ class OwnerCheck(policy.Check): # resource is handled by the core plugin. It might be worth # having a way to map resources to plugins so to make this # check more general - f = getattr(directory.get_plugin(), 'get_%s' % resource_type) + plugin = directory.get_plugin() + if resource_type in const.EXT_PARENT_RESOURCE_MAPPING: + plugin = directory.get_plugin( + const.EXT_PARENT_RESOURCE_MAPPING[resource_type]) + f = getattr(plugin, 'get_%s' % resource_type) # f *must* exist, if not found it is better to let neutron # explode. Check will be performed with admin context try: @@ -272,6 +276,13 @@ class OwnerCheck(policy.Check): reason=err_reason) parent_foreign_key = _RESOURCE_FOREIGN_KEYS.get( "%ss" % parent_res, None) + if parent_res == const.EXT_PARENT_PREFIX: + for resource in const.EXT_PARENT_RESOURCE_MAPPING: + key = "%s_%s_id" % (const.EXT_PARENT_PREFIX, resource) + if key in target: + parent_foreign_key = key + parent_res = resource + break if not parent_foreign_key: err_reason = (_("Unable to verify match:%(match)s as the " "parent resource: %(res)s was not found") % diff --git a/neutron/tests/etc/policy.json b/neutron/tests/etc/policy.json index 9093c6abe77..b643aa692ff 100644 --- a/neutron/tests/etc/policy.json +++ b/neutron/tests/etc/policy.json @@ -13,6 +13,7 @@ "shared_address_scopes": "field:address_scopes:shared=True", "external": "field:networks:router:external=True", "default": "rule:admin_or_owner", + "admin_or_ext_parent_owner": "rule:context_is_admin or tenant_id:%(ext_parent:tenant_id)s", "create_subnet": "rule:admin_or_network_owner", "create_subnet:segment_id": "rule:admin_only", @@ -250,9 +251,9 @@ "update_log": "rule:admin_only", "delete_log": "rule:admin_only", - "create_floatingip_port_forwarding": "rule:regular_user", - "get_floatingip_port_forwarding": "rule:regular_user", - "get_floatingip_port_forwardings": "rule:regular_user", - "update_floatingip_port_forwarding": "rule:regular_user", - "delete_floatingip_port_forwarding": "rule:regular_user" + "create_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner", + "get_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner", + "get_floatingip_port_forwardings": "rule:admin_or_ext_parent_owner", + "update_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner", + "delete_floatingip_port_forwarding": "rule:admin_or_ext_parent_owner" } diff --git a/neutron/tests/unit/test_policy.py b/neutron/tests/unit/test_policy.py index 3984e4ebf50..21513787e49 100644 --- a/neutron/tests/unit/test_policy.py +++ b/neutron/tests/unit/test_policy.py @@ -599,3 +599,26 @@ class NeutronPolicyTestCase(base.BaseTestCase): result = policy._is_attribute_explicitly_set( attr, resource, target, action) self.assertFalse(result) + + @mock.patch("neutron.common.constants.EXT_PARENT_RESOURCE_MAPPING", + {'parentresource': 'registered_plugin_name'}) + @mock.patch("neutron_lib.plugins.directory.get_plugin") + def test_enforce_tenant_id_check_parent_resource_owner( + self, mock_get_plugin): + + def fakegetparent(*args, **kwargs): + return {'tenant_id': 'fake'} + mock_plugin = mock.Mock() + mock_plugin.get_parentresource = fakegetparent + mock_get_plugin.return_value = mock_plugin + + self._set_rules( + admin_or_ext_parent_owner="rule:context_is_admin or " + "tenant_id:%(ext_parent:tenant_id)s", + create_parentresource_subresource="rule:admin_or_ext_parent_owner") + self.fakepolicyinit() + action = 'create_parentresource_subresource' + target = {'ext_parent_parentresource_id': 'whatever', 'foo': 'bar'} + result = policy.enforce(self.context, action, target) + mock_get_plugin.assert_called_with('registered_plugin_name') + self.assertTrue(result) diff --git a/releasenotes/notes/extend-policy-for-extension-resource-owner-check-4a19b84889660506.yaml b/releasenotes/notes/extend-policy-for-extension-resource-owner-check-4a19b84889660506.yaml new file mode 100644 index 00000000000..76ae3c58b8b --- /dev/null +++ b/releasenotes/notes/extend-policy-for-extension-resource-owner-check-4a19b84889660506.yaml @@ -0,0 +1,9 @@ +--- +features: + - | + Introduces extension parent resources owner check in + ``neutron.policy.OwnerCheck``. It can be used by registering an extension + parent resource and service plugin which introduced the corresponding + parent resource into ``EXT_PARENT_RESOURCE_MAPPING`` located in + ``neutron.common.constants``. And introduces a new policy role + ``admin_or_ext_parent_owner`` into ``policy.json`` for this function.