From 4a09c2210b3c07343411a06c676c2d85aa0e214f Mon Sep 17 00:00:00 2001 From: Prashanth kumar reddy Date: Thu, 27 Oct 2016 07:09:01 -0400 Subject: [PATCH] Separate CRUD policy for server_groups The same policy rule (os_compute_api:os-server-groups) is being used for all actions (show, index, delete, create) for server_groups REST APIs. It is thus impossible to provide different RBAC for specific actions based on roles. To address this changes are made to have separate policy rules for each of action. It has been argued that index and show may not need separate policy rules, but most other places in nova (and OpenStack in general) do have separate policy rules for each action. This affords the ultimate flexibility to deployers, who can obviously use the same rule if that is what they want. One example where show and index may be different is that if show is restricted based on some criteria, such that a user is able to see some resources within the tenant but not others, then list would need to be disallowed to prevent the user from using list to see resources they cannot show. Change-Id: Ica9e07f6e80257902b4a0cc44b65fd6bad008bba Closes-Bug: #1636157 --- nova/api/openstack/compute/server_groups.py | 12 +- nova/policies/server_groups.py | 13 ++ .../openstack/compute/test_server_groups.py | 140 +++++++++++++++++- nova/tests/unit/fake_policy.py | 4 + nova/tests/unit/policy_fixture.py | 5 +- nova/tests/unit/test_policy.py | 4 + .../notes/bug-1636157-2148ea3675969a5d.yaml | 10 ++ 7 files changed, 172 insertions(+), 16 deletions(-) create mode 100644 releasenotes/notes/bug-1636157-2148ea3675969a5d.yaml diff --git a/nova/api/openstack/compute/server_groups.py b/nova/api/openstack/compute/server_groups.py index e5e2a4bb68de..412912288cdc 100644 --- a/nova/api/openstack/compute/server_groups.py +++ b/nova/api/openstack/compute/server_groups.py @@ -36,9 +36,9 @@ LOG = logging.getLogger(__name__) ALIAS = "os-server-groups" -def _authorize_context(req): +def _authorize_context(req, action): context = req.environ['nova.context'] - context.can(sg_policies.BASE_POLICY_NAME) + context.can(sg_policies.POLICY_ROOT % action) return context @@ -75,7 +75,7 @@ class ServerGroupController(wsgi.Controller): @extensions.expected_errors(404) def show(self, req, id): """Return data about the given server group.""" - context = _authorize_context(req) + context = _authorize_context(req, 'show') try: sg = objects.InstanceGroup.get_by_uuid(context, id) except nova.exception.InstanceGroupNotFound as e: @@ -86,7 +86,7 @@ class ServerGroupController(wsgi.Controller): @extensions.expected_errors(404) def delete(self, req, id): """Delete an server group.""" - context = _authorize_context(req) + context = _authorize_context(req, 'delete') try: sg = objects.InstanceGroup.get_by_uuid(context, id) except nova.exception.InstanceGroupNotFound as e: @@ -117,7 +117,7 @@ class ServerGroupController(wsgi.Controller): @extensions.expected_errors(()) def index(self, req): """Returns a list of server groups.""" - context = _authorize_context(req) + context = _authorize_context(req, 'index') project_id = context.project_id if 'all_projects' in req.GET and context.is_admin: sgs = objects.InstanceGroupList.get_all(context) @@ -135,7 +135,7 @@ class ServerGroupController(wsgi.Controller): @validation.schema(schema.create_v215, "2.15") def create(self, req, body): """Creates a new server group.""" - context = _authorize_context(req) + context = _authorize_context(req, 'create') quotas = objects.Quotas(context=context) try: diff --git a/nova/policies/server_groups.py b/nova/policies/server_groups.py index e8f79c5009d3..bd303a627de3 100644 --- a/nova/policies/server_groups.py +++ b/nova/policies/server_groups.py @@ -20,6 +20,7 @@ from nova.policies import base BASE_POLICY_NAME = 'os_compute_api:os-server-groups' POLICY_ROOT = 'os_compute_api:os-server-groups:%s' +BASE_POLICY_RULE = 'rule:%s' % BASE_POLICY_NAME server_groups_policies = [ @@ -29,6 +30,18 @@ server_groups_policies = [ policy.RuleDefault( name=BASE_POLICY_NAME, check_str=base.RULE_ADMIN_OR_OWNER), + policy.RuleDefault( + name=POLICY_ROOT % 'create', + check_str=BASE_POLICY_RULE), + policy.RuleDefault( + name=POLICY_ROOT % 'delete', + check_str=BASE_POLICY_RULE), + policy.RuleDefault( + name=POLICY_ROOT % 'index', + check_str=BASE_POLICY_RULE), + policy.RuleDefault( + name=POLICY_ROOT % 'show', + check_str=BASE_POLICY_RULE), ] diff --git a/nova/tests/unit/api/openstack/compute/test_server_groups.py b/nova/tests/unit/api/openstack/compute/test_server_groups.py index 3f7c9218c8dc..f916e6f3d118 100644 --- a/nova/tests/unit/api/openstack/compute/test_server_groups.py +++ b/nova/tests/unit/api/openstack/compute/test_server_groups.py @@ -22,8 +22,10 @@ from nova import context import nova.db from nova import exception from nova import objects +from nova.policies import server_groups as sg_policies from nova import test from nova.tests.unit.api.openstack import fakes +from nova.tests.unit import policy_fixture from nova.tests import uuidsentinel @@ -80,6 +82,9 @@ class ServerGroupTestV21(test.TestCase): super(ServerGroupTestV21, self).setUp() self._setup_controller() self.req = fakes.HTTPRequest.blank('') + self.admin_req = fakes.HTTPRequest.blank('', use_admin_context=True) + self.foo_req = fakes.HTTPRequest.blank('', project_id='foo') + self.policy = self.useFixture(policy_fixture.RealPolicyFixture()) def _setup_controller(self): self.controller = sg_v21.ServerGroupController() @@ -103,6 +108,36 @@ class ServerGroupTestV21(test.TestCase): for policy in policies: self._create_server_group_normal([policy]) + def test_create_server_group_rbac_default(self): + sgroup = server_group_template() + sgroup['policies'] = ['affinity'] + + # test as admin + self.controller.create(self.admin_req, body={'server_group': sgroup}) + + # test as non-admin + self.controller.create(self.req, body={'server_group': sgroup}) + + def test_create_server_group_rbac_admin_only(self): + sgroup = server_group_template() + sgroup['policies'] = ['affinity'] + + # override policy to restrict to admin + rule_name = sg_policies.POLICY_ROOT % 'create' + rules = {rule_name: 'is_admin:True'} + self.policy.set_rules(rules, overwrite=False) + + # check for success as admin + self.controller.create(self.admin_req, body={'server_group': sgroup}) + + # check for failure as non-admin + exc = self.assertRaises(exception.PolicyNotAuthorized, + self.controller.create, self.req, + body={'server_group': sgroup}) + self.assertEqual( + "Policy doesn't allow %s to be performed." % rule_name, + exc.format_message()) + def _create_instance(self, context): instance = objects.Instance(context=context, image_ref=uuidsentinel.fake_image_ref, @@ -182,12 +217,15 @@ class ServerGroupTestV21(test.TestCase): path = '/os-server-groups?all_projects=True' - req = fakes.HTTPRequest.blank(path, use_admin_context=True, - version=api_version) - res_dict = self.controller.index(req) + req = fakes.HTTPRequest.blank(path, version=api_version) + admin_req = fakes.HTTPRequest.blank(path, use_admin_context=True, + version=api_version) + + # test as admin + res_dict = self.controller.index(admin_req) self.assertEqual(all, res_dict) - req = fakes.HTTPRequest.blank(path, - version=api_version) + + # test as non-admin res_dict = self.controller.index(req) self.assertEqual(tenant_specific, res_dict) @@ -235,9 +273,8 @@ class ServerGroupTestV21(test.TestCase): return_get_by_project = return_server_groups() mock_get_by_project.return_value = return_get_by_project path = '/os-server-groups' - self.req = fakes.HTTPRequest.blank(path, - version=api_version) - res_dict = self.controller.index(self.req) + req = fakes.HTTPRequest.blank(path, version=api_version) + res_dict = self.controller.index(req) self.assertEqual(expected, res_dict) def test_display_members(self): @@ -269,6 +306,39 @@ class ServerGroupTestV21(test.TestCase): self.assertEqual(1, len(result_members)) self.assertIn(instances[0].uuid, result_members) + def test_display_members_rbac_default(self): + ctx = context.RequestContext('fake_user', 'fake') + ig_uuid = self._create_groups_and_instances(ctx)[0] + + # test as admin + self.controller.show(self.admin_req, ig_uuid) + + # test as non-admin, same project + self.controller.show(self.req, ig_uuid) + + # test as non-admin, different project + self.assertRaises(webob.exc.HTTPNotFound, + self.controller.show, self.foo_req, ig_uuid) + + def test_display_members_rbac_admin_only(self): + ctx = context.RequestContext('fake_user', 'fake') + ig_uuid = self._create_groups_and_instances(ctx)[0] + + # override policy to restrict to admin + rule_name = sg_policies.POLICY_ROOT % 'show' + rules = {rule_name: 'is_admin:True'} + self.policy.set_rules(rules, overwrite=False) + + # check for success as admin + self.controller.show(self.admin_req, ig_uuid) + + # check for failure as non-admin + exc = self.assertRaises(exception.PolicyNotAuthorized, + self.controller.show, self.req, ig_uuid) + self.assertEqual( + "Policy doesn't allow %s to be performed." % rule_name, + exc.format_message()) + def test_create_server_group_with_non_alphanumeric_in_name(self): # The fix for bug #1434335 expanded the allowable character set # for server group names to include non-alphanumeric characters @@ -384,6 +454,29 @@ class ServerGroupTestV21(test.TestCase): def test_list_server_group_all(self): self._test_list_server_group_all(api_version='2.1') + def test_list_server_groups_rbac_default(self): + # test as admin + self.controller.index(self.admin_req) + + # test as non-admin + self.controller.index(self.req) + + def test_list_server_groups_rbac_admin_only(self): + # override policy to restrict to admin + rule_name = sg_policies.POLICY_ROOT % 'index' + rules = {rule_name: 'is_admin:True'} + self.policy.set_rules(rules, overwrite=False) + + # check for success as admin + self.controller.index(self.admin_req) + + # check for failure as non-admin + exc = self.assertRaises(exception.PolicyNotAuthorized, + self.controller.index, self.req) + self.assertEqual( + "Policy doesn't allow %s to be performed." % rule_name, + exc.format_message()) + def test_delete_server_group_by_id(self): sg = server_group_template(id=uuidsentinel.sg1_id) @@ -416,6 +509,37 @@ class ServerGroupTestV21(test.TestCase): self.assertRaises(webob.exc.HTTPNotFound, self.controller.delete, self.req, 'invalid') + def test_delete_server_group_rbac_default(self): + ctx = context.RequestContext('fake_user', 'fake') + + # test as admin + ig_uuid = self._create_groups_and_instances(ctx)[0] + self.controller.delete(self.admin_req, ig_uuid) + + # test as non-admin + ig_uuid = self._create_groups_and_instances(ctx)[0] + self.controller.delete(self.req, ig_uuid) + + def test_delete_server_group_rbac_admin_only(self): + ctx = context.RequestContext('fake_user', 'fake') + + # override policy to restrict to admin + rule_name = sg_policies.POLICY_ROOT % 'delete' + rules = {rule_name: 'is_admin:True'} + self.policy.set_rules(rules, overwrite=False) + + # check for success as admin + ig_uuid = self._create_groups_and_instances(ctx)[0] + self.controller.delete(self.admin_req, ig_uuid) + + # check for failure as non-admin + ig_uuid = self._create_groups_and_instances(ctx)[0] + exc = self.assertRaises(exception.PolicyNotAuthorized, + self.controller.delete, self.req, ig_uuid) + self.assertEqual( + "Policy doesn't allow %s to be performed." % rule_name, + exc.format_message()) + class ServerGroupTestV213(ServerGroupTestV21): wsgi_api_version = '2.13' diff --git a/nova/tests/unit/fake_policy.py b/nova/tests/unit/fake_policy.py index 951df19d3fea..f3a896132df1 100644 --- a/nova/tests/unit/fake_policy.py +++ b/nova/tests/unit/fake_policy.py @@ -102,6 +102,10 @@ policy_data = """ "os_compute_api:os-server-tags:delete_all": "", "os_compute_api:os-server-usage": "", "os_compute_api:os-server-groups": "", + "os_compute_api:os-server-groups:show": "", + "os_compute_api:os-server-groups:index": "", + "os_compute_api:os-server-groups:create": "", + "os_compute_api:os-server-groups:delete": "", "os_compute_api:os-services": "", "os_compute_api:os-shelve:shelve": "", "os_compute_api:os-shelve:shelve_offload": "", diff --git a/nova/tests/unit/policy_fixture.py b/nova/tests/unit/policy_fixture.py index b112ac1a9e48..5f917ac6072f 100644 --- a/nova/tests/unit/policy_fixture.py +++ b/nova/tests/unit/policy_fixture.py @@ -54,9 +54,10 @@ class RealPolicyFixture(fixtures.Fixture): nova.policy.init() self.addCleanup(nova.policy.reset) - def set_rules(self, rules): + def set_rules(self, rules, overwrite=True): policy = nova.policy._ENFORCER - policy.set_rules(oslo_policy.Rules.from_dict(rules)) + policy.set_rules(oslo_policy.Rules.from_dict(rules), + overwrite=overwrite) def add_missing_default_rules(self, rules): """Adds default rules and their values to the given rules dict. diff --git a/nova/tests/unit/test_policy.py b/nova/tests/unit/test_policy.py index c6919a72ad6d..4c3d6cb8836d 100644 --- a/nova/tests/unit/test_policy.py +++ b/nova/tests/unit/test_policy.py @@ -402,6 +402,10 @@ class RealRolePolicyTestCase(test.NoDBTestCase): "os_compute_api:os-server-password", "os_compute_api:os-server-usage", "os_compute_api:os-server-groups", +"os_compute_api:os-server-groups:index", +"os_compute_api:os-server-groups:show", +"os_compute_api:os-server-groups:create", +"os_compute_api:os-server-groups:delete", "os_compute_api:os-shelve:shelve", "os_compute_api:os-shelve:unshelve", "os_compute_api:os-virtual-interfaces", diff --git a/releasenotes/notes/bug-1636157-2148ea3675969a5d.yaml b/releasenotes/notes/bug-1636157-2148ea3675969a5d.yaml new file mode 100644 index 000000000000..8069d72190f4 --- /dev/null +++ b/releasenotes/notes/bug-1636157-2148ea3675969a5d.yaml @@ -0,0 +1,10 @@ +--- +features: + - | + The same policy rule (os_compute_api:os-server-groups) + was being used for all actions (show, index, delete, create) + for server_groups REST APIs. It was thus impossible to provide + different RBAC for specific actions based on roles. To address + this changes are made to have separate policy rules for each + action. The original rule (os_compute_api:os-server-groups) is + left unchanged for backward compatibility.