From ba37ea322777bb1efb210f4cbb72cae2f2c541d2 Mon Sep 17 00:00:00 2001 From: Dan Smith Date: Fri, 11 Jun 2021 09:26:40 -0700 Subject: [PATCH] Check get_image(s) in the API This includes a change to catch Forbidden and convert to NotFound. The previous Forbidden handler was not only correct (it shoud hide the permissions error with "not found") but it was actually dead code, since the DB was performing its own checks and would never raise Forbidden. This also includes a change of the default policy for get_images to include the other states, like get_image does. I think this was just an oversight in the original RBAC patches, which didn't matter because they weren't really being honored strictly. Partially implements: blueprint policy-refactor Change-Id: I70100cd7f01da803e9740cea1f7ce7ae18ad6919 --- glance/api/v2/images.py | 32 ++++++++++--- glance/api/v2/policy.py | 3 ++ glance/policies/base.py | 18 +++++--- glance/tests/functional/v2/test_images.py | 2 +- .../functional/v2/test_images_api_policy.py | 44 ++++++++++++++++++ glance/tests/unit/test_policy.py | 15 +++--- glance/tests/unit/utils.py | 2 +- glance/tests/unit/v2/test_images_resource.py | 46 +++++++++++++++++-- glance/tests/unit/v2/test_v2_policy.py | 12 +++++ 9 files changed, 150 insertions(+), 24 deletions(-) diff --git a/glance/api/v2/images.py b/glance/api/v2/images.py index a65c8c5207..9faf374e72 100644 --- a/glance/api/v2/images.py +++ b/glance/api/v2/images.py @@ -514,14 +514,31 @@ class ImagesController(object): limit = CONF.limit_param_default limit = min(CONF.api_limit_max, limit) - image_repo = self.gateway.get_repo(req.context) + image_repo = self.gateway.get_repo(req.context, + authorization_layer=False) try: + # NOTE(danms): This is just a "do you have permission to + # list images" check. Each image is checked against + # get_image below. + target = {'project_id': req.context.project_id} + self.policy.enforce(req.context, 'get_images', target) + images = image_repo.list(marker=marker, limit=limit, sort_key=sort_key, sort_dir=sort_dir, filters=filters, member_status=member_status) - if len(images) != 0 and len(images) == limit: + db_image_count = len(images) + images = [image for image in images + if api_policy.ImageAPIPolicy(req.context, image, + self.policy + ).check('get_image')] + + # NOTE(danms): we need to include the next marker if the DB + # paginated. Since we filter images based on policy, we can + # not determine if pagination happened from the final list, + # so use the original count. + if len(images) != 0 and db_image_count == limit: result['next_marker'] = images[-1].image_id except (exception.NotFound, exception.InvalidSortKey, exception.InvalidFilterRangeValue, @@ -537,12 +554,13 @@ class ImagesController(object): return result def show(self, req, image_id): - image_repo = self.gateway.get_repo(req.context) + image_repo = self.gateway.get_repo(req.context, + authorization_layer=False) try: - return image_repo.get(image_id) - except exception.Forbidden as e: - LOG.debug("User not permitted to show image '%s'", image_id) - raise webob.exc.HTTPForbidden(explanation=e.msg) + image = image_repo.get(image_id) + api_policy.ImageAPIPolicy(req.context, image, + self.policy).get_image() + return image except exception.NotFound as e: raise webob.exc.HTTPNotFound(explanation=e.msg) except exception.NotAuthenticated as e: diff --git a/glance/api/v2/policy.py b/glance/api/v2/policy.py index a9b7fafb2a..7643928414 100644 --- a/glance/api/v2/policy.py +++ b/glance/api/v2/policy.py @@ -107,3 +107,6 @@ class ImageAPIPolicy(APIPolicyBase): def get_image(self): self._enforce('get_image') + + def get_images(self): + self._enforce('get_images') diff --git a/glance/policies/base.py b/glance/policies/base.py index 43fc3f261c..8518eb6736 100644 --- a/glance/policies/base.py +++ b/glance/policies/base.py @@ -29,14 +29,18 @@ IMAGE_MEMBER_CHECK = 'project_id:%(member_id)s' COMMUNITY_VISIBILITY_CHECK = '"community":%(visibility)s' # Check if the visibility of the image supplied in the target matches "public" PUBLIC_VISIBILITY_CHECK = '"public":%(visibility)s' +# Check if the visibility of the image supplied in the target matches "shared" +SHARED_VISIBILITY_CHECK = '"shared":%(visibility)s' -PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC = ( +PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED = ( f'role:member and (project_id:%(project_id)s or {IMAGE_MEMBER_CHECK} ' - f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK})' + f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK} ' + f'or {SHARED_VISIBILITY_CHECK})' ) -PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC = ( +PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED = ( f'role:reader and (project_id:%(project_id)s or {IMAGE_MEMBER_CHECK} ' - f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK})' + f'or {COMMUNITY_VISIBILITY_CHECK} or {PUBLIC_VISIBILITY_CHECK} ' + f'or {SHARED_VISIBILITY_CHECK})' ) # FIXME(lbragstad): These are composite check strings that represents glance's @@ -58,10 +62,12 @@ PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC = ( ADMIN_OR_PROJECT_MEMBER = f'role:admin or ({PROJECT_MEMBER})' ADMIN_OR_PROJECT_READER = f'role:admin or ({PROJECT_READER})' ADMIN_OR_PROJECT_READER_GET_IMAGE = ( - f'role:admin or ({PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC})' + f'role:admin or ' + f'({PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED})' ) ADMIN_OR_PROJECT_MEMBER_DOWNLOAD_IMAGE = ( - f'role:admin or ({PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC})' + f'role:admin or ' + f'({PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED})' ) diff --git a/glance/tests/functional/v2/test_images.py b/glance/tests/functional/v2/test_images.py index a74babf67e..4bdbcc0dbb 100644 --- a/glance/tests/functional/v2/test_images.py +++ b/glance/tests/functional/v2/test_images.py @@ -6857,7 +6857,7 @@ class TestCopyImagePermissions(functional.MultipleBackendFunctionalTest): 'content-type': 'application/json', }) headers = get_auth_header(TENANT2, TENANT2, - role='member', headers=headers) + role='reader,member', headers=headers) data = jsonutils.dumps( {'method': {'name': 'copy-image'}, 'stores': ['file2']}) diff --git a/glance/tests/functional/v2/test_images_api_policy.py b/glance/tests/functional/v2/test_images_api_policy.py index 19c7378fa8..60fc2eb88e 100644 --- a/glance/tests/functional/v2/test_images_api_policy.py +++ b/glance/tests/functional/v2/test_images_api_policy.py @@ -160,3 +160,47 @@ class TestImagesPolicy(functional.SynchronousAPIBase): self.assertEqual(2, len(self.api_get( '/v2/images/%s' % image_id).json['locations'])) + + def test_image_get(self): + self.start_server() + + image_id = self._create_and_upload() + + # Make sure we can get the image + image = self.api_get('/v2/images/%s' % image_id).json + self.assertEqual(image_id, image['id']) + + # Make sure we can list the image + images = self.api_get('/v2/images').json['images'] + self.assertEqual(1, len(images)) + self.assertEqual(image_id, images[0]['id']) + + # Now disable get_images but allow get_image + self.set_policy_rules({'get_images': '!', + 'get_image': ''}) + + # We should not be able to list, but still fetch the image by id + resp = self.api_get('/v2/images') + self.assertEqual(403, resp.status_code) + image = self.api_get('/v2/images/%s' % image_id).json + self.assertEqual(image_id, image['id']) + + # Now disable get_image but allow get_images + self.set_policy_rules({'get_images': '', + 'get_image': '!'}) + + # We should be able to list, but not actually see the image in the list + images = self.api_get('/v2/images').json['images'] + self.assertEqual(0, len(images)) + resp = self.api_get('/v2/images/%s' % image_id) + self.assertEqual(404, resp.status_code) + + # Now disable both get_image and get_images + self.set_policy_rules({'get_images': '!', + 'get_image': '!'}) + + # We should not be able to list or fetch by id + resp = self.api_get('/v2/images') + self.assertEqual(403, resp.status_code) + resp = self.api_get('/v2/images/%s' % image_id) + self.assertEqual(404, resp.status_code) diff --git a/glance/tests/unit/test_policy.py b/glance/tests/unit/test_policy.py index 0b29efee21..2857d7afac 100644 --- a/glance/tests/unit/test_policy.py +++ b/glance/tests/unit/test_policy.py @@ -275,8 +275,9 @@ class TestPolicyEnforcer(base.IsolatedUnitTest): def test_policy_file_default_rules_default_location(self): enforcer = glance.api.policy.Enforcer() - context = glance.context.RequestContext(roles=[]) - enforcer.enforce(context, 'get_image', {}) + context = glance.context.RequestContext(roles=['reader']) + enforcer.enforce(context, 'get_image', + {'project_id': context.project_id}) def test_policy_file_custom_rules_default_location(self): rules = {"get_image": '!'} @@ -1073,11 +1074,12 @@ class TestDefaultPolicyCheckStrings(base.IsolatedUnitTest): expected = ( 'role:member and (project_id:%(project_id)s or ' 'project_id:%(member_id)s or "community":%(visibility)s or ' - '"public":%(visibility)s)' + '"public":%(visibility)s or "shared":%(visibility)s)' ) self.assertEqual( expected, - base_policy.PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC + base_policy. + PROJECT_MEMBER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED ) def test_project_reader_check_string(self): @@ -1092,11 +1094,12 @@ class TestDefaultPolicyCheckStrings(base.IsolatedUnitTest): expected = ( 'role:reader and (project_id:%(project_id)s or ' 'project_id:%(member_id)s or "community":%(visibility)s or ' - '"public":%(visibility)s)' + '"public":%(visibility)s or "shared":%(visibility)s)' ) self.assertEqual( expected, - base_policy.PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC + base_policy. + PROJECT_READER_OR_IMAGE_MEMBER_OR_COMMUNITY_OR_PUBLIC_OR_SHARED ) diff --git a/glance/tests/unit/utils.py b/glance/tests/unit/utils.py index c2442f9508..ead1f08bb1 100644 --- a/glance/tests/unit/utils.py +++ b/glance/tests/unit/utils.py @@ -66,7 +66,7 @@ def sort_url_by_qs_keys(url): def get_fake_request(path='', method='POST', is_admin=False, user=USER1, roles=None, tenant=TENANT1): if roles is None: - roles = ['member'] + roles = ['member', 'reader'] req = wsgi.Request.blank(path) req.method = method diff --git a/glance/tests/unit/v2/test_images_resource.py b/glance/tests/unit/v2/test_images_resource.py index 10011e7e91..8b1309038d 100644 --- a/glance/tests/unit/v2/test_images_resource.py +++ b/glance/tests/unit/v2/test_images_resource.py @@ -388,6 +388,30 @@ class TestImagesController(base.IsolatedUnitTest): self.assertEqual(expected, actual) self.assertNotIn('next_marker', output) + def test_index_marker_would_be_disallowed(self): + self.config(limit_param_default=1, api_limit_max=10) + request = unit_test_utils.get_fake_request(is_admin=True) + + def fake_enforce(context, action, target=None, **kw): + assert target is not None + if target['project_id'] != TENANT1: + raise exception.Forbidden() + + # As admin, list three images. By default, this should leave + # us on UUID3 (and as next_marker), which is owned by TENANT3 + output = self.controller.index(request, sort_dir=['asc'], limit=3) + self.assertEqual(UUID3, output['next_marker']) + self.assertEqual(3, len(output['images'])) + + # Now sub in our fake policy that restricts us to TENANT1 images only. + # Even though we list with limit=3, we should only get two images back, + # and our next_marker should be UUID2 because we couldn't see UUID3. + with mock.patch.object(self.controller.policy, 'enforce', + new=fake_enforce): + output = self.controller.index(request, sort_dir=['asc'], limit=3) + self.assertEqual(UUID2, output['next_marker']) + self.assertEqual(2, len(output['images'])) + def test_index_with_id_filter(self): request = unit_test_utils.get_fake_request('/images?id=%s' % UUID1) output = self.controller.index(request, filters={'id': UUID1}) @@ -775,6 +799,19 @@ class TestImagesController(base.IsolatedUnitTest): self.assertRaises(webob.exc.HTTPNotFound, self.controller.show, request, UUID4) + def test_show_not_allowed_by_policy(self): + # Use admin so that we get past the check buried in the DB and + # only hit the policy check we are mocking. + request = unit_test_utils.get_fake_request(is_admin=True) + with mock.patch.object(self.controller.policy, 'enforce') as mock_enf: + mock_enf.side_effect = webob.exc.HTTPForbidden() + # Make sure we get NotFound instead of Forbidden + exc = self.assertRaises(webob.exc.HTTPNotFound, + self.controller.show, request, UUID4) + # Make sure we did not leak details of the original Forbidden + # error into the NotFound returned to the client. + self.assertEqual('The resource could not be found.', str(exc)) + def test_get_task_info(self): request = unit_test_utils.get_fake_request() output = self.controller.get_task_info(request, image_id=UUID1) @@ -1621,7 +1658,8 @@ class TestImagesController(base.IsolatedUnitTest): created_image = self.controller.create(request, image=image, extra_properties=extra_props, tags=[]) - another_request = unit_test_utils.get_fake_request(roles=['member']) + another_request = unit_test_utils.get_fake_request(roles=['reader', + 'member']) output = self.controller.show(another_request, created_image.image_id) self.assertEqual('bar', output.extra_properties['x_owner_foo']) @@ -1638,7 +1676,8 @@ class TestImagesController(base.IsolatedUnitTest): created_image = self.controller.create(request, image=image, extra_properties=extra_props, tags=[]) - another_request = unit_test_utils.get_fake_request(roles=['fake_role']) + another_request = unit_test_utils.get_fake_request(roles=['reader', + 'fake_role']) output = self.controller.show(another_request, created_image.image_id) self.assertRaises(KeyError, output.extra_properties.__getitem__, 'x_owner_foo') @@ -1759,7 +1798,8 @@ class TestImagesController(base.IsolatedUnitTest): created_image = self.controller.create(request, image=image, extra_properties=extra_props, tags=[]) - another_request = unit_test_utils.get_fake_request(roles=['member']) + another_request = unit_test_utils.get_fake_request(roles=['reader', + 'member']) output = self.controller.show(another_request, created_image.image_id) self.assertEqual('1', output.extra_properties['x_case_insensitive']) diff --git a/glance/tests/unit/v2/test_v2_policy.py b/glance/tests/unit/v2/test_v2_policy.py index 500541f17e..469cf95703 100644 --- a/glance/tests/unit/v2/test_v2_policy.py +++ b/glance/tests/unit/v2/test_v2_policy.py @@ -139,3 +139,15 @@ class APIImagePolicy(APIPolicyBase): mock_enf.assert_has_calls([ mock.call(mock.ANY, 'modify_image', mock.ANY), mock.call(mock.ANY, 'get_image', mock.ANY)]) + + def test_get_image(self): + self.policy.get_image() + self.enforcer.enforce.assert_called_once_with(self.context, + 'get_image', + mock.ANY) + + def test_get_images(self): + self.policy.get_images() + self.enforcer.enforce.assert_called_once_with(self.context, + 'get_images', + mock.ANY)