From 19f9a4bbd4ba9910f73a293bd3a13eedc32fa877 Mon Sep 17 00:00:00 2001 From: Andrew Laski Date: Mon, 13 Jun 2016 13:28:45 -0400 Subject: [PATCH] Policy-in-code servers rules This adds the basic framework for registering and using default policy rules. Rules should be defined and returned from a module in nova/policies/, and then added to the list in nova/policies/__init__.py. A new context.can() method has been added for policy enforcement of registered rules. It has the same parameters as the enforce() method currently being used. To establish the full pattern for usage the policy checks in the servers API module have been registered and converted to the new usage. Now that some policy checks are registered they're being used properly by tests. Some tests have been updated so that the instance project_id matches the context project_id in order to pass the 'admin_or_owner' check. Change-Id: I71b3d1233255125cb280a000b990329f5b03fdfd Partially-Implements: bp policy-in-code --- etc/nova/policy.json | 21 ------- nova/api/openstack/compute/servers.py | 47 ++++++++-------- nova/context.py | 6 ++ nova/policies/__init__.py | 24 ++++++++ nova/policies/base.py | 24 ++++++++ nova/policies/servers.py | 53 ++++++++++++++++++ nova/policy.py | 56 +++++++++++++++++++ .../api/openstack/compute/test_serversV21.py | 6 +- nova/tests/unit/api/openstack/fakes.py | 2 + nova/tests/unit/fake_policy.py | 21 ------- 10 files changed, 194 insertions(+), 66 deletions(-) create mode 100644 nova/policies/__init__.py create mode 100644 nova/policies/base.py create mode 100644 nova/policies/servers.py diff --git a/etc/nova/policy.json b/etc/nova/policy.json index 54408410eb0c..80380d5467a9 100644 --- a/etc/nova/policy.json +++ b/etc/nova/policy.json @@ -8,28 +8,7 @@ "admin_api": "is_admin:True", "network:attach_external_network": "is_admin:True", - "os_compute_api:servers:detail:get_all_tenants": "is_admin:True", - "os_compute_api:servers:index:get_all_tenants": "is_admin:True", - "os_compute_api:servers:confirm_resize": "rule:admin_or_owner", - "os_compute_api:servers:create": "rule:admin_or_owner", - "os_compute_api:servers:create:attach_network": "rule:admin_or_owner", - "os_compute_api:servers:create:attach_volume": "rule:admin_or_owner", - "os_compute_api:servers:create:forced_host": "rule:admin_api", - "os_compute_api:servers:delete": "rule:admin_or_owner", - "os_compute_api:servers:update": "rule:admin_or_owner", - "os_compute_api:servers:detail": "rule:admin_or_owner", - "os_compute_api:servers:index": "rule:admin_or_owner", - "os_compute_api:servers:reboot": "rule:admin_or_owner", - "os_compute_api:servers:rebuild": "rule:admin_or_owner", - "os_compute_api:servers:resize": "rule:admin_or_owner", - "os_compute_api:servers:revert_resize": "rule:admin_or_owner", - "os_compute_api:servers:show": "rule:admin_or_owner", "os_compute_api:servers:show:host_status": "rule:admin_api", - "os_compute_api:servers:create_image": "rule:admin_or_owner", - "os_compute_api:servers:create_image:allow_volume_backed": "rule:admin_or_owner", - "os_compute_api:servers:start": "rule:admin_or_owner", - "os_compute_api:servers:stop": "rule:admin_or_owner", - "os_compute_api:servers:trigger_crash_dump": "rule:admin_or_owner", "os_compute_api:servers:migrations:force_complete": "rule:admin_api", "os_compute_api:servers:migrations:delete": "rule:admin_api", "os_compute_api:servers:discoverable": "@", diff --git a/nova/api/openstack/compute/servers.py b/nova/api/openstack/compute/servers.py index 97d4b96df2bd..781029c2a011 100644 --- a/nova/api/openstack/compute/servers.py +++ b/nova/api/openstack/compute/servers.py @@ -43,6 +43,7 @@ from nova.i18n import _ from nova.i18n import _LW from nova.image import glance from nova import objects +from nova.policies import servers as server_policies from nova import utils ALIAS = 'servers' @@ -51,7 +52,6 @@ TAG_SEARCH_FILTERS = ('tags', 'tags-any', 'not-tags', 'not-tags-any') CONF = nova.conf.CONF LOG = logging.getLogger(__name__) -authorize = extensions.os_compute_authorizer(ALIAS) class ServersController(wsgi.Controller): @@ -273,7 +273,7 @@ class ServersController(wsgi.Controller): def index(self, req): """Returns a list of server names and ids for a given user.""" context = req.environ['nova.context'] - authorize(context, action="index") + context.can(server_policies.get_name('index')) try: servers = self._get_servers(req, is_detail=False) except exception.Invalid as err: @@ -284,7 +284,7 @@ class ServersController(wsgi.Controller): def detail(self, req): """Returns a list of server details for a given user.""" context = req.environ['nova.context'] - authorize(context, action="detail") + context.can(server_policies.get_name('detail')) try: servers = self._get_servers(req, is_detail=True) except exception.Invalid as err: @@ -383,9 +383,9 @@ class ServersController(wsgi.Controller): elevated = None if all_tenants: if is_detail: - authorize(context, action="detail:get_all_tenants") + context.can(server_policies.get_name('detail:get_all_tenants')) else: - authorize(context, action="index:get_all_tenants") + context.can(server_policies.get_name('index:get_all_tenants')) elevated = context.elevated() else: if context.project_id: @@ -538,7 +538,7 @@ class ServersController(wsgi.Controller): def show(self, req, id): """Returns server details by server id.""" context = req.environ['nova.context'] - authorize(context, action="show") + context.can(server_policies.get_name('show')) instance = self._get_server(context, req, id, is_detail=True) return self._view_builder.show(req, instance) @@ -585,7 +585,7 @@ class ServersController(wsgi.Controller): 'project_id': context.project_id, 'user_id': context.user_id, 'availability_zone': availability_zone} - authorize(context, target, 'create') + context.can(server_policies.get_name('create'), target) # TODO(Shao He, Feng) move this policy check to os-availabilty-zone # extension after refactor it. @@ -596,13 +596,14 @@ class ServersController(wsgi.Controller): except exception.InvalidInput as err: raise exc.HTTPBadRequest(explanation=six.text_type(err)) if host or node: - authorize(context, {}, 'create:forced_host') + context.can(server_policies.get_name('create:forced_host'), {}) block_device_mapping = create_kwargs.get("block_device_mapping") # TODO(Shao He, Feng) move this policy check to os-block-device-mapping # extension after refactor it. if block_device_mapping: - authorize(context, target, 'create:attach_volume') + context.can(server_policies.get_name('create:attach_volume'), + target) image_uuid = self._image_from_req_data(server_dict, create_kwargs) @@ -626,7 +627,8 @@ class ServersController(wsgi.Controller): requested_networks) if requested_networks and len(requested_networks): - authorize(context, target, 'create:attach_network') + context.can(server_policies.get_name('create:attach_network'), + target) try: flavor_id = self._flavor_id_from_req_data(body) @@ -801,7 +803,7 @@ class ServersController(wsgi.Controller): resize_schema['properties']['resize']['properties'].update(schema) def _delete(self, context, req, instance_uuid): - authorize(context, action='delete') + context.can(server_policies.get_name('delete')) instance = self._get_server(context, req, instance_uuid) if CONF.reclaim_instance_interval: try: @@ -823,7 +825,7 @@ class ServersController(wsgi.Controller): ctxt = req.environ['nova.context'] update_dict = {} - authorize(ctxt, action='update') + ctxt.can(server_policies.get_name('update')) if 'name' in body['server']: update_dict['display_name'] = common.normalize_name( @@ -857,7 +859,7 @@ class ServersController(wsgi.Controller): @wsgi.action('confirmResize') def _action_confirm_resize(self, req, id, body): context = req.environ['nova.context'] - authorize(context, action='confirm_resize') + context.can(server_policies.get_name('confirm_resize')) instance = self._get_server(context, req, id) try: self.compute_api.confirm_resize(context, instance) @@ -877,7 +879,7 @@ class ServersController(wsgi.Controller): @wsgi.action('revertResize') def _action_revert_resize(self, req, id, body): context = req.environ['nova.context'] - authorize(context, action='revert_resize') + context.can(server_policies.get_name('revert_resize')) instance = self._get_server(context, req, id) try: self.compute_api.revert_resize(context, instance) @@ -903,7 +905,7 @@ class ServersController(wsgi.Controller): reboot_type = body['reboot']['type'].upper() context = req.environ['nova.context'] - authorize(context, action='reboot') + context.can(server_policies.get_name('reboot')) instance = self._get_server(context, req, id) try: @@ -917,7 +919,7 @@ class ServersController(wsgi.Controller): def _resize(self, req, instance_id, flavor_id, **kwargs): """Begin the resize process with given instance/flavor.""" context = req.environ["nova.context"] - authorize(context, action='resize') + context.can(server_policies.get_name('resize')) instance = self._get_server(context, req, instance_id) try: @@ -1023,7 +1025,7 @@ class ServersController(wsgi.Controller): password = self._get_server_admin_password(rebuild_dict) context = req.environ['nova.context'] - authorize(context, action='rebuild') + context.can(server_policies.get_name('rebuild')) instance = self._get_server(context, req, id) attr_map = { @@ -1098,7 +1100,7 @@ class ServersController(wsgi.Controller): def _action_create_image(self, req, id, body): """Snapshot a server instance.""" context = req.environ['nova.context'] - authorize(context, action='create_image') + context.can(server_policies.get_name('create_image')) entity = body["createImage"] image_name = common.normalize_name(entity["name"]) @@ -1114,7 +1116,8 @@ class ServersController(wsgi.Controller): try: if compute_utils.is_volume_backed_instance(context, instance, bdms): - authorize(context, action="create_image:allow_volume_backed") + context.can(server_policies.get_name( + 'create_image:allow_volume_backed')) image = self.compute_api.snapshot_volume_backed( context, instance, @@ -1175,7 +1178,7 @@ class ServersController(wsgi.Controller): """Start an instance.""" context = req.environ['nova.context'] instance = self._get_instance(context, id) - authorize(context, instance, 'start') + context.can(server_policies.get_name('start'), instance) LOG.debug('start instance', instance=instance) try: self.compute_api.start(context, instance) @@ -1194,7 +1197,7 @@ class ServersController(wsgi.Controller): """Stop an instance.""" context = req.environ['nova.context'] instance = self._get_instance(context, id) - authorize(context, instance, 'stop') + context.can(server_policies.get_name('stop'), instance) LOG.debug('stop instance', instance=instance) try: self.compute_api.stop(context, instance) @@ -1215,7 +1218,7 @@ class ServersController(wsgi.Controller): """Trigger crash dump in an instance""" context = req.environ['nova.context'] instance = self._get_instance(context, id) - authorize(context, instance, 'trigger_crash_dump') + context.can(server_policies.get_name('trigger_crash_dump'), instance) try: self.compute_api.trigger_crash_dump(context, instance) except exception.InstanceInvalidState as state_error: diff --git a/nova/context.py b/nova/context.py index 86f3a141cc94..912a177653f8 100644 --- a/nova/context.py +++ b/nova/context.py @@ -218,6 +218,12 @@ class RequestContext(context.RequestContext): return context + def can(self, rule, target=None): + if target is None: + target = {'project_id': self.project_id, + 'user_id': self.user_id} + return policy.authorize(self, rule, target) + def __str__(self): return "" % self.to_dict() diff --git a/nova/policies/__init__.py b/nova/policies/__init__.py new file mode 100644 index 000000000000..8b59e32d72d1 --- /dev/null +++ b/nova/policies/__init__.py @@ -0,0 +1,24 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +import itertools + +from nova.policies import base +from nova.policies import servers + + +def list_rules(): + return itertools.chain( + base.list_rules(), + servers.list_rules() + ) diff --git a/nova/policies/base.py b/nova/policies/base.py new file mode 100644 index 000000000000..9cff0ef02de7 --- /dev/null +++ b/nova/policies/base.py @@ -0,0 +1,24 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from oslo_policy import policy + +rules = [ + policy.RuleDefault('context_is_admin', 'role:admin'), + policy.RuleDefault('admin_or_owner', + 'is_admin:True or project_id:%(project_id)s'), + policy.RuleDefault('admin_api', 'is_admin:True'), +] + + +def list_rules(): + return rules diff --git a/nova/policies/servers.py b/nova/policies/servers.py new file mode 100644 index 000000000000..c2f81b0aaae0 --- /dev/null +++ b/nova/policies/servers.py @@ -0,0 +1,53 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +from oslo_policy import policy + + +RULE_AOO = 'rule:admin_or_owner' + + +def get_name(action=None): + name = 'os_compute_api:servers' + if action: + name = name + ':%s' % action + return name + +rules = [ + policy.RuleDefault(get_name('index'), RULE_AOO), + policy.RuleDefault(get_name('detail'), RULE_AOO), + policy.RuleDefault(get_name('detail:get_all_tenants'), RULE_AOO), + policy.RuleDefault(get_name('index:get_all_tenants'), RULE_AOO), + policy.RuleDefault(get_name('show'), RULE_AOO), + policy.RuleDefault(get_name('create'), RULE_AOO), + policy.RuleDefault(get_name('create:forced_host'), RULE_AOO), + policy.RuleDefault(get_name('create:attach_volume'), RULE_AOO), + policy.RuleDefault(get_name('create:attach_network'), RULE_AOO), + policy.RuleDefault(get_name('delete'), RULE_AOO), + policy.RuleDefault(get_name('update'), RULE_AOO), + policy.RuleDefault(get_name('confirm_resize'), RULE_AOO), + policy.RuleDefault(get_name('revert_resize'), RULE_AOO), + policy.RuleDefault(get_name('reboot'), RULE_AOO), + policy.RuleDefault(get_name('resize'), RULE_AOO), + policy.RuleDefault(get_name('rebuild'), RULE_AOO), + policy.RuleDefault(get_name('create_image'), RULE_AOO), + policy.RuleDefault(get_name('create_image:allow_volume_backed'), + RULE_AOO), + policy.RuleDefault(get_name('start'), RULE_AOO), + policy.RuleDefault(get_name('stop'), RULE_AOO), + policy.RuleDefault(get_name('trigger_crash_dump'), RULE_AOO), +] + + +def list_rules(): + return rules diff --git a/nova/policy.py b/nova/policy.py index 8cde01607f53..bb41f12de0f9 100644 --- a/nova/policy.py +++ b/nova/policy.py @@ -22,6 +22,8 @@ from oslo_policy import policy from oslo_utils import excutils from nova import exception +from nova.i18n import _LE +from nova import policies CONF = cfg.CONF @@ -55,6 +57,7 @@ def init(policy_file=None, rules=None, default_rule=None, use_conf=True): rules=rules, default_rule=default_rule, use_conf=use_conf) + register_rules(_ENFORCER) def set_rules(rules, overwrite=True, use_conf=False): @@ -70,6 +73,8 @@ def set_rules(rules, overwrite=True, use_conf=False): _ENFORCER.set_rules(rules, overwrite, use_conf) +# TODO(alaski): All users of this method should move over to authorize() as +# policies are registered and ultimately this should be removed. def enforce(context, action, target, do_raise=True, exc=None): """Verifies that the action is valid on the target in this context. @@ -108,6 +113,53 @@ def enforce(context, action, target, do_raise=True, exc=None): return result +def authorize(context, action, target, do_raise=True, exc=None): + """Verifies that the action is valid on the target in this context. + + :param context: nova context + :param action: string representing the action to be checked + this should be colon separated for clarity. + i.e. ``compute:create_instance``, + ``compute:attach_volume``, + ``volume:attach_volume`` + :param target: dictionary representing the object of the action + for object creation this should be a dictionary representing the + location of the object e.g. ``{'project_id': context.project_id}`` + :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:`enforce` (both + positional and keyword arguments) will be passed to + the exception class. If not specified, + :class:`PolicyNotAuthorized` will be used. + + :raises nova.exception.PolicyNotAuthorized: if verification fails + and do_raise is True. Or if 'exc' is specified it will raise an + exception of that type. + + :return: returns a non-False value (not necessarily "True") if + authorized, and the exact value False if not authorized and + do_raise is False. + """ + init() + credentials = context.to_dict() + if not exc: + exc = exception.PolicyNotAuthorized + try: + result = _ENFORCER.authorize(action, target, credentials, + do_raise=do_raise, exc=exc, action=action) + except policy.PolicyNotRegistered: + with excutils.save_and_reraise_exception(): + LOG.exception(_LE('Policy not registered')) + except Exception: + credentials.pop('auth_token', None) + with excutils.save_and_reraise_exception(): + LOG.debug('Policy check for %(action)s failed with credentials ' + '%(credentials)s', + {'action': action, 'credentials': credentials}) + return result + + def check_is_admin(context): """Whether or not roles contains 'admin' role according to policy setting. @@ -140,3 +192,7 @@ class IsAdminCheck(policy.Check): def get_rules(): if _ENFORCER: return _ENFORCER.rules + + +def register_rules(enforcer): + enforcer.register_defaults(policies.list_rules()) diff --git a/nova/tests/unit/api/openstack/compute/test_serversV21.py b/nova/tests/unit/api/openstack/compute/test_serversV21.py index 5a1ed2162e3f..978196264326 100644 --- a/nova/tests/unit/api/openstack/compute/test_serversV21.py +++ b/nova/tests/unit/api/openstack/compute/test_serversV21.py @@ -1614,7 +1614,8 @@ class ServersControllerRebuildInstanceTest(ControllerTest): if uuid == 'test_inst': raise webob.exc.HTTPNotFound(explanation='fakeout') return fakes.stub_instance_obj(None, - vm_state=vm_states.ACTIVE) + vm_state=vm_states.ACTIVE, + project_id='fake') self.useFixture( fixtures.MonkeyPatch('nova.api.openstack.compute.servers.' @@ -2101,7 +2102,8 @@ class ServersControllerTriggerCrashDumpTest(ControllerTest): super(ServersControllerTriggerCrashDumpTest, self).setUp() self.instance = fakes.stub_instance_obj(None, - vm_state=vm_states.ACTIVE) + vm_state=vm_states.ACTIVE, + project_id='fake') def fake_get(ctrl, ctxt, uuid): if uuid != FAKE_UUID: diff --git a/nova/tests/unit/api/openstack/fakes.py b/nova/tests/unit/api/openstack/fakes.py index dff36394bc7b..822fac8fbe36 100644 --- a/nova/tests/unit/api/openstack/fakes.py +++ b/nova/tests/unit/api/openstack/fakes.py @@ -323,6 +323,8 @@ def get_fake_uuid(token=0): def fake_instance_get(**kwargs): def _return_server(context, uuid, columns_to_join=None, use_slave=False): + if 'project_id' not in kwargs: + kwargs['project_id'] = 'fake' return stub_instance(1, **kwargs) return _return_server diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py index 0cf521fbba1c..c76308219829 100644 --- a/nova/tests/unit/fake_policy.py +++ b/nova/tests/unit/fake_policy.py @@ -21,28 +21,7 @@ policy_data = """ "context_is_admin": "role:admin or role:administrator", - "os_compute_api:servers:confirm_resize": "", - "os_compute_api:servers:create": "", - "os_compute_api:servers:create:attach_network": "", - "os_compute_api:servers:create:attach_volume": "", - "os_compute_api:servers:create:forced_host": "", - "os_compute_api:servers:delete": "", - "os_compute_api:servers:detail": "", - "os_compute_api:servers:detail:get_all_tenants": "", - "os_compute_api:servers:index": "", - "os_compute_api:servers:index:get_all_tenants": "", - "os_compute_api:servers:reboot": "", - "os_compute_api:servers:rebuild": "", - "os_compute_api:servers:resize": "", - "os_compute_api:servers:revert_resize": "", - "os_compute_api:servers:show": "", "os_compute_api:servers:show:host_status": "", - "os_compute_api:servers:create_image": "", - "os_compute_api:servers:create_image:allow_volume_backed": "", - "os_compute_api:servers:update": "", - "os_compute_api:servers:start": "", - "os_compute_api:servers:stop": "", - "os_compute_api:servers:trigger_crash_dump": "", "os_compute_api:servers:migrations:delete": "rule:admin_api", "os_compute_api:servers:migrations:force_complete": "", "os_compute_api:servers:migrations:index": "rule:admin_api",