From 0917bea7cad6f9e2cc671f391c255bfc638d781e Mon Sep 17 00:00:00 2001 From: Ghanshyam Mann Date: Wed, 1 Apr 2020 12:21:54 -0500 Subject: [PATCH] Correct security groups policy check_str security groups API policy is default to admin_or_owner[1] but API is allowed (which is expected) for everyone. This is because API does not pass the project_id in policy target so that oslo policy can decide the ownership[2]. If no target is passed then, policy.py add the default targets which is nothing but context.project_id (allow for everyone try to access) - https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/policy.py#L191 Passing the server project_id as target to make it admin_or_owner. [1] https://github.com/openstack/nova/blob/7b51647f17c88c7c1ae321c59ab8a98c586d4b67/nova/policies/security_groups.py#L27 [2] https://github.com/openstack/nova/blob/7b51647f17c88c7c1ae321c59ab8a98c586d4b67/nova/api/openstack/compute/security_groups.py#L427 [3] https://github.com/openstack/nova/blob/c16315165ce307c605cf4b608b2df3aa06f46982/nova/policy.py#L191 Change-Id: I7b09a8ff3ccbb74e5299bdf5775d286609bc5d4c Closes-bug: #18670226 --- nova/api/openstack/compute/security_groups.py | 25 ++++++++++--------- .../openstack/compute/test_security_groups.py | 13 ++++++++-- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/nova/api/openstack/compute/security_groups.py b/nova/api/openstack/compute/security_groups.py index 38f8096b2d3c..e1ebc1b6943d 100644 --- a/nova/api/openstack/compute/security_groups.py +++ b/nova/api/openstack/compute/security_groups.py @@ -367,9 +367,10 @@ class ServerSecurityGroupController(SecurityGroupControllerBase): @wsgi.expected_errors(404) def index(self, req, server_id): """Returns a list of security groups for the given instance.""" - context = _authorize_context(req) - + context = req.environ['nova.context'] instance = common.get_instance(self.compute_api, context, server_id) + context.can(sg_policies.BASE_POLICY_NAME, + target={'project_id': instance.project_id}) try: groups = security_group_api.get_instance_security_groups( context, instance, True) @@ -415,21 +416,19 @@ class SecurityGroupActionController(wsgi.Controller): return group_name - def _invoke(self, method, context, id, group_name): - instance = common.get_instance(self.compute_api, context, id) - method(context, instance, group_name) - @wsgi.expected_errors((400, 404, 409)) @wsgi.response(202) @wsgi.action('addSecurityGroup') def _addSecurityGroup(self, req, id, body): context = req.environ['nova.context'] - context.can(sg_policies.BASE_POLICY_NAME) + instance = common.get_instance(self.compute_api, context, id) + context.can(sg_policies.BASE_POLICY_NAME, + target={'project_id': instance.project_id}) group_name = self._parse(body, 'addSecurityGroup') try: - return self._invoke(security_group_api.add_to_instance, - context, id, group_name) + return security_group_api.add_to_instance(context, instance, + group_name) except (exception.SecurityGroupNotFound, exception.InstanceNotFound) as exp: raise exc.HTTPNotFound(explanation=exp.format_message()) @@ -443,13 +442,15 @@ class SecurityGroupActionController(wsgi.Controller): @wsgi.action('removeSecurityGroup') def _removeSecurityGroup(self, req, id, body): context = req.environ['nova.context'] - context.can(sg_policies.BASE_POLICY_NAME) + instance = common.get_instance(self.compute_api, context, id) + context.can(sg_policies.BASE_POLICY_NAME, + target={'project_id': instance.project_id}) group_name = self._parse(body, 'removeSecurityGroup') try: - return self._invoke(security_group_api.remove_from_instance, - context, id, group_name) + return security_group_api.remove_from_instance(context, instance, + group_name) except (exception.SecurityGroupNotFound, exception.InstanceNotFound) as exp: raise exc.HTTPNotFound(explanation=exp.format_message()) diff --git a/nova/tests/unit/api/openstack/compute/test_security_groups.py b/nova/tests/unit/api/openstack/compute/test_security_groups.py index 2b767cec832c..23acd962965a 100644 --- a/nova/tests/unit/api/openstack/compute/test_security_groups.py +++ b/nova/tests/unit/api/openstack/compute/test_security_groups.py @@ -13,6 +13,7 @@ # License for the specific language governing permissions and limitations # under the License. +import fixtures import mock from neutronclient.common import exceptions as n_exc from oslo_config import cfg @@ -34,7 +35,7 @@ from nova import objects from nova.objects import instance as instance_obj from nova import test from nova.tests.unit.api.openstack import fakes - +from nova.tests.unit import fake_instance CONF = cfg.CONF FAKE_UUID1 = 'a47ae74e-ab08-447f-8eee-ffd43fc46c16' @@ -395,13 +396,15 @@ class TestSecurityGroupsV21(test.TestCase): self.fake_id = '11111111-1111-1111-1111-111111111111' self.req = fakes.HTTPRequest.blank('') + project_id = self.req.environ['nova.context'].project_id self.admin_req = fakes.HTTPRequest.blank('', use_admin_context=True) self.stub_out('nova.compute.api.API.get', fakes.fake_compute_get( **{'power_state': 0x01, 'host': "localhost", 'uuid': UUID_SERVER, - 'name': 'asdf'})) + 'name': 'asdf', + 'project_id': project_id})) self.original_client = neutron_api.get_client neutron_api.get_client = get_client @@ -1583,6 +1586,12 @@ class PolicyEnforcementV21(test.NoDBTestCase): self.req = fakes.HTTPRequest.blank('') self.rule_name = "os_compute_api:os-security-groups" self.rule = {self.rule_name: "project:non_fake"} + context = self.req.environ['nova.context'] + self.mock_get = self.useFixture( + fixtures.MockPatch('nova.api.openstack.common.get_instance')).mock + self.instance = fake_instance.fake_instance_obj( + context, id=1, project_id=context.project_id) + self.mock_get.return_value = self.instance def _common_policy_check(self, func, *arg, **kwarg): self.policy.set_rules(self.rule)