Fedora Account System
Red Hat Associate
Red Hat Customer
Description: Chen YuXiang from the Institute of Computing Technology, Chinese Academy of Sciences reported a vulnerability in Manila's public share authorization handling. The "is_public" flag grants a share API visibility to all projects without transferring management authority, and the controller authorizes write operations against the caller's own project rather than the target share's project. By performing management operations on a public share owned by another project, a project member may add, update, or revoke access rules, modify export-location metadata, create backups or replicas, and soft-delete or restore the share, resulting in unauthorized cross-project data access and the potential to deny the owner access to their own share. Only deployments in which at least one share is marked public are affected. Proposed patch: See attached patches. Unless a flaw is discovered in them, these patches will be merged to their corresponding branches on the public disclosure date. One patch targets manila-tempest-plugin: it updates two negative tests to expect 404 instead of 403 for unauthorized access to another project's share, matching the corrected authorization behavior; it will follow the core fixes once they merge. CVE: CVE-2026-71195 Proposed public disclosure date/time: 2026-10-20, 1500UTC Please do not make the issue public (or release public patches) before this coordinated embargo date. Original private report: https://launchpad.net/bugs/2161412 For access to read and comment on this report, please reply to me with your Launchpad username and I will subscribe you. -- Goutham Pacha Ravi OpenStack Vulnerability Management Team https://security.openstack.org/vmt.html * Patch From 9c862f231be81c9b0dfa2645cbcd0dab7e0d6a40 Mon Sep 17 00:00:00 2001 From: Carlos da Silva <ces.eduardo98> Date: Fri, 7 Aug 2026 20:55:09 -0300 Subject: [PATCH] Fix policy control for public shares A public share is visible to all tenants, but users other than the owner should not be able to modify it. Several operations skipped the ownership policy check for public shares. Also, the export location metadata endpoints resolved the export location globally, instead of within the share in the request URL. This commit enforces the ownership checks when manipulating access rules, creating backups or replicas, and soft deleting or restoring public shares. Export location metadata operations are now scoped to the share in the request URL; export locations that do not belong to that share now return 404. Change-Id: Ie283dce157f9c90f5d2ab38c1e74daa7df3ac118 Signed-off-by: Carlos da Silva <ces.eduardo98> Closes-Bug: #2161412 Closes-Bug: #2168962 #+begin_src diff --- manila/api/v2/metadata.py | 5 +- manila/api/v2/share_accesses.py | 3 +- manila/api/v2/share_backups.py | 3 +- manila/api/v2/share_export_locations.py | 35 ++++--- manila/api/v2/share_replicas.py | 3 +- manila/api/v2/shares.py | 8 +- manila/share/api.py | 8 +- manila/tests/api/v2/test_metadata.py | 41 ++++++++ manila/tests/api/v2/test_share_accesses.py | 55 +++++++++-- manila/tests/api/v2/test_share_backups.py | 25 ++++- .../api/v2/test_share_export_locations.py | 35 ++++++- manila/tests/api/v2/test_share_replicas.py | 47 ++++++++- manila/tests/api/v2/test_shares.py | 96 +++++++++++++++++-- manila/tests/share/test_api.py | 58 +++++++++++ ...cks-on-public-shares-6cb60f669bb20f6c.yaml | 11 +++ 15 files changed, 380 insertions(+), 53 deletions(-) create mode 100644 releasenotes/notes/bug-2161412-enforce-policy-checks-on-public-shares-6cb60f669bb20f6c.yaml diff --git a/manila/api/v2/metadata.py b/manila/api/v2/metadata.py index 174be16..ad07ec4 100644 --- a/manila/api/v2/metadata.py +++ b/manila/api/v2/metadata.py @@ -99,7 +99,10 @@ class MetadataController(object): get_res_method = getattr( db, self.resource_get[self.resource_name]) if parent_id is not None: - kwargs["parent_id"] = parent_id + if self.resource_name == 'share_export_location': + kwargs["share_id"] = parent_id + else: + kwargs["parent_id"] = parent_id if self.resource_name == 'share_replica': kwargs['with_share_data'] = True res = get_res_method(context, resource_id, **kwargs) diff --git a/manila/api/v2/share_accesses.py b/manila/api/v2/share_accesses.py index 78148a5..ece6e63 100644 --- a/manila/api/v2/share_accesses.py +++ b/manila/api/v2/share_accesses.py @@ -168,7 +168,8 @@ class ShareAccessesController(wsgi.Controller, wsgi.AdminActionsMixin): if access_level == share_access.access_level: return self._view_builder.view(req, share_access) - share = self.share_api.get(context, share_access.share_id) + share = self.share_api.get( + context, share_access.share_id, for_modification=True) values = { 'access_level': access_level, } diff --git a/manila/api/v2/share_backups.py b/manila/api/v2/share_backups.py index afae556..4f82c06 100644 --- a/manila/api/v2/share_backups.py +++ b/manila/api/v2/share_backups.py @@ -158,7 +158,8 @@ class ShareBackupController(wsgi.Controller, wsgi.AdminActionsMixin): raise exc.HTTPBadRequest(explanation=msg) try: - share = self.share_api.get(context, share_id) + share = self.share_api.get( + context, share_id, for_modification=True) except exception.NotFound: msg = _("No share exists with ID %s.") raise exc.HTTPBadRequest(explanation=msg % share_id) diff --git a/manila/api/v2/share_export_locations.py b/manila/api/v2/share_export_locations.py index 3106d3d..48157e7 100644 --- a/manila/api/v2/share_export_locations.py +++ b/manila/api/v2/share_export_locations.py @@ -26,6 +26,7 @@ from manila.db import api as db_api from manila import exception from manila.i18n import _ from manila import policy +from manila.share import api as share_api LOG = log.getLogger(__name__) CONF = cfg.CONF @@ -43,12 +44,13 @@ class ShareExportLocationController(wsgi.Controller, self._conf_admin_only_metadata_keys = getattr( CONF, 'admin_only_el_metadata', [] ) + self.share_api = share_api.API() - def _verify_share(self, context, share_id): + def _verify_share(self, context, share_id, for_modification=False): try: - share = db_api.share_get(context, share_id) - if not share['is_public']: - policy.check_policy(context, 'share', 'get', share) + self.share_api.get( + context, share_id, for_modification=for_modification + ) except exception.NotFound: msg = _("Share '%s' not found.") % share_id raise exc.HTTPNotFound(explanation=msg) @@ -165,7 +167,7 @@ class ShareExportLocationController(wsgi.Controller, """Returns the list of metadata for a given share export location.""" context = req.environ['manila.context'] self._verify_share(context, share_id) - return self._index_metadata(req, resource_id) + return self._index_metadata(req, resource_id, parent_id=share_id) @wsgi.Controller.api_version("2.87") @wsgi.Controller.authorize("update_metadata") @@ -178,8 +180,9 @@ class ShareExportLocationController(wsgi.Controller, delete=False) body['metadata'] = _metadata context = req.environ['manila.context'] - self._verify_share(context, share_id) - return self._create_metadata(req, resource_id, body) + self._verify_share(context, share_id, for_modification=True) + return self._create_metadata( + req, resource_id, body, parent_id=share_id) @wsgi.Controller.api_version("2.87") @wsgi.Controller.authorize("update_metadata") @@ -191,8 +194,9 @@ class ShareExportLocationController(wsgi.Controller, body['metadata']) body['metadata'] = _metadata context = req.environ['manila.context'] - self._verify_share(context, share_id) - return self._update_all_metadata(req, resource_id, body) + self._verify_share(context, share_id, for_modification=True) + return self._update_all_metadata( + req, resource_id, body, parent_id=share_id) @wsgi.Controller.api_version("2.87") @wsgi.Controller.authorize("update_metadata") @@ -205,8 +209,9 @@ class ShareExportLocationController(wsgi.Controller, delete=False) body['metadata'] = _metadata context = req.environ['manila.context'] - self._verify_share(context, share_id) - return self._update_metadata_item(req, resource_id, body, key) + self._verify_share(context, share_id, for_modification=True) + return self._update_metadata_item( + req, resource_id, body, key, parent_id=share_id) @wsgi.Controller.api_version("2.87") @wsgi.Controller.authorize("get_metadata") @@ -216,7 +221,8 @@ class ShareExportLocationController(wsgi.Controller, """Show metadata for a given share export location.""" context = req.environ['manila.context'] self._verify_share(context, share_id) - return self._show_metadata(req, resource_id, key) + return self._show_metadata( + req, resource_id, key, parent_id=share_id) @wsgi.Controller.api_version("2.87") @wsgi.Controller.authorize("delete_metadata") @@ -224,8 +230,9 @@ class ShareExportLocationController(wsgi.Controller, def delete_metadata(self, req, share_id, resource_id, key): """Delete metadata for a given share export location.""" context = req.environ['manila.context'] - self._verify_share(context, share_id) - return self._delete_metadata(req, resource_id, key) + self._verify_share(context, share_id, for_modification=True) + return self._delete_metadata( + req, resource_id, key, parent_id=share_id) def create_resource(): diff --git a/manila/api/v2/share_replicas.py b/manila/api/v2/share_replicas.py index c7cce66..d29983d 100644 --- a/manila/api/v2/share_replicas.py +++ b/manila/api/v2/share_replicas.py @@ -206,7 +206,8 @@ class ShareReplicationController(wsgi.Controller, metadata.MetadataController, raise exc.HTTPBadRequest(explanation=msg) try: - share_ref = db.share_get(context, share_id) + share_ref = self.share_api.get( + context, share_id, for_modification=True) except exception.NotFound: msg = _("No share exists with ID %s.") raise exc.HTTPNotFound(explanation=msg % share_id) diff --git a/manila/api/v2/shares.py b/manila/api/v2/shares.py index c3c0708..0baac35 100644 --- a/manila/api/v2/shares.py +++ b/manila/api/v2/shares.py @@ -432,7 +432,7 @@ class ShareController( LOG.debug("Soft delete share with id: %s", id, context=context) try: - share = self.share_api.get(context, id) + share = self.share_api.get(context, id, for_modification=True) self.share_api.soft_delete(context, share) except exception.NotFound: raise exc.HTTPNotFound() @@ -457,7 +457,7 @@ class ShareController( LOG.debug("Restore share with id: %s", id, context=context) try: - share = self.share_api.get(context, id) + share = self.share_api.get(context, id, for_modification=True) except exception.NotFound: msg = _("No share exists with ID %s.") raise exc.HTTPNotFound(explanation=msg % id) @@ -792,7 +792,7 @@ class ShareController( access_data = body.get('allow_access', body.get('os-allow_access')) if not enable_metadata: access_data.pop('metadata', None) - share = self.share_api.get(context, id) + share = self.share_api.get(context, id, for_modification=True) if share.get('is_soft_deleted'): msg = _("Cannot allow access for share '%s' " @@ -937,7 +937,7 @@ class ShareController( self._check_for_access_rule_locks(context, access_data, access_id, id) - share = self.share_api.get(context, id) + share = self.share_api.get(context, id, for_modification=True) if share.get('is_soft_deleted'): msg = _("Cannot deny access for share '%s' " diff --git a/manila/share/api.py b/manila/share/api.py index 96b657a..f3ee109 100644 --- a/manila/share/api.py +++ b/manila/share/api.py @@ -2427,11 +2427,13 @@ class API(base.Base): def snapshot_update(self, context, snapshot, fields): return self.db.share_snapshot_update(context, snapshot['id'], fields) - def get(self, context, share_id): + def get(self, context, share_id, for_modification=False): share = self.db.share_get(context, share_id) - if not share['is_public']: + share_is_public = share['is_public'] + if not share_is_public or for_modification: authorized = policy.check_policy( - context, 'share', 'get', share, do_raise=False) + context, 'share', 'get', share, + do_raise=share_is_public and for_modification) if not authorized: raise exception.NotFound() if share['status'] in ( diff --git a/manila/tests/api/v2/test_metadata.py b/manila/tests/api/v2/test_metadata.py index d443f0c..b031afa 100644 --- a/manila/tests/api/v2/test_metadata.py +++ b/manila/tests/api/v2/test_metadata.py @@ -17,6 +17,7 @@ import webob from manila.api.v2 import metadata from manila import context +from manila import db from manila import exception from manila import policy from manila import test @@ -81,6 +82,46 @@ class MetadataAPITest(test.TestCase): policy_resource_obj = mock_policy_check.call_args[0][3] self.assertEqual(share_obj['id'], policy_resource_obj['id']) + def test__get_resource_export_location_scoped_to_share(self): + # Export location lookups must be scoped to the share in the + # request URL, so that the metadata of an export location + # belonging to a different share cannot be retrieved. + self.controller.resource_name = 'share_export_location' + fake_context = context.RequestContext('fake', 'fake') + export_location = {'id': 'fake-el-uuid'} + mock_get_by_uuid = self.mock_object( + db, 'export_location_get_by_uuid', + mock.Mock(return_value=export_location)) + + res = self.controller._get_resource( + fake_context, 'fake-el-uuid', for_modification=True, + parent_id='fake-share-id') + + self.assertEqual(export_location, res) + mock_get_by_uuid.assert_called_once_with( + fake_context, 'fake-el-uuid', share_id='fake-share-id') + self.mock_policy_check.assert_not_called() + + def test__get_resource_export_location_not_in_share_not_found(self): + # An export location that does not belong to the share in the + # request URL must not be accessible, regardless of the type + # of the share it belongs to. + self.controller.resource_name = 'share_export_location' + fake_context = context.RequestContext('fake', 'fake') + self.mock_object( + db, 'export_location_get_by_uuid', + mock.Mock(side_effect=exception.ExportLocationNotFound( + uuid='fake-el-uuid'))) + + self.assertRaises( + webob.exc.HTTPNotFound, + self.controller._get_resource, + fake_context, + 'fake-el-uuid', + for_modification=True, + parent_id='fake-share-id', + ) + def test_create_index_metadata(self): url = self._get_request() body = {'metadata': {'test_key1': 'test_v1', 'test_key2': 'test_v2'}} diff --git a/manila/tests/api/v2/test_share_accesses.py b/manila/tests/api/v2/test_share_accesses.py index bb84d23..0b8319f 100644 --- a/manila/tests/api/v2/test_share_accesses.py +++ b/manila/tests/api/v2/test_share_accesses.py @@ -15,7 +15,6 @@ from unittest import mock -import copy import ddt from webob import exc @@ -247,17 +246,55 @@ class ShareAccessesAPITest(test.TestCase): return req def test_update_access_level(self): - update_share_access = copy.deepcopy(self.access) - update_share_access.update({'access_level': 'ro'}) - self.mock_object( + fake_access = mock.Mock( + access_level='rw', share_id=self.share['id']) + updated_access = mock.Mock( + access_level='ro', share_id=self.share['id']) + mock_get_access = self.mock_object( + self.controller, '_get_share_access', + mock.Mock(return_value=fake_access)) + mock_get = self.mock_object( + self.controller.share_api, 'get', + mock.Mock(return_value=self.share)) + mock_update = self.mock_object( self.controller.share_api, 'update_access', - mock.Mock(return_value=update_share_access)) + mock.Mock(return_value=updated_access)) + mock_view = self.mock_object( + self.controller._view_builder, 'view', + mock.Mock(return_value={'access': {'access_level': 'ro'}})) body = {'update_access': {'access_level': 'ro'}} - url = self._get_update_request() - ret = self.controller.update(url, self.access['id'], body=body) - self.assertEqual(update_share_access['access_level'], - ret['access']['access_level']) + req = self._get_update_request() + context = req.environ['manila.context'] + ret = self.controller.update(req, self.access['id'], body=body) + + self.assertEqual('ro', ret['access']['access_level']) + mock_get_access.assert_called_once_with( + context, self.access['id']) + mock_get.assert_called_once_with( + context, self.share['id'], for_modification=True) + mock_update.assert_called_once_with( + context, self.share, fake_access, {'access_level': 'ro'}) + mock_view.assert_called_once_with(req, updated_access) + + def test_update_policy_not_authorized(self): + fake_access = mock.Mock( + access_level='rw', share_id=self.share['id']) + self.mock_object( + self.controller, '_get_share_access', + mock.Mock(return_value=fake_access)) + self.mock_object( + self.controller.share_api, 'get', + mock.Mock(side_effect=exception.PolicyNotAuthorized( + action='share:get'))) + + body = {'update_access': {'access_level': 'ro'}} + req = self._get_update_request() + + self.assertRaises( + exception.PolicyNotAuthorized, + self.controller.update, + req, self.access['id'], body=body) def test_update_access_level_invalid_access_level(self): body = {'access': {'access_level': 'fake_access'}} diff --git a/manila/tests/api/v2/test_share_backups.py b/manila/tests/api/v2/test_share_backups.py index 7e1e820..015d0ae 100644 --- a/manila/tests/api/v2/test_share_backups.py +++ b/manila/tests/api/v2/test_share_backups.py @@ -362,19 +362,38 @@ class ShareBackupsApiTest(test.TestCase): def test_create(self): fake_backup, expected_backup = self._get_fake_backup() + share_id = 'FAKE_SHAREID' body = { 'share_backup': { - 'share_id': 'FAKE_SHAREID', + 'share_id': share_id, } } - self.mock_object(share.API, 'get', - mock.Mock(return_value={'id': 'FAKE_SHAREID'})) + mock_get = self.mock_object( + share.API, 'get', mock.Mock(return_value={'id': share_id})) self.mock_object(share.API, 'create_share_backup', mock.Mock(return_value=fake_backup)) req = self.backups_req res_dict = self.controller.create(req, body) self.assertEqual(expected_backup, res_dict['share_backup']) + mock_get.assert_called_once_with( + self.backups_req.environ['manila.context'], share_id, + for_modification=True) + + def test_create_policy_not_authorized(self): + body = { + 'share_backup': { + 'share_id': 'FAKE_SHAREID', + } + } + self.mock_object( + share.API, 'get', + mock.Mock(side_effect=exception.PolicyNotAuthorized( + action='share:get'))) + + self.assertRaises(exception.PolicyNotAuthorized, + self.controller.create, + self.backups_req, body) def test_delete_invalid_backup(self): fake_exception = exception.ShareBackupNotFound( diff --git a/manila/tests/api/v2/test_share_export_locations.py b/manila/tests/api/v2/test_share_export_locations.py index 65d4e22..f96829f 100644 --- a/manila/tests/api/v2/test_share_export_locations.py +++ b/manila/tests/api/v2/test_share_export_locations.py @@ -339,7 +339,8 @@ class ShareExportLocationsAPITest(test.TestCase): self.assertEqual(body, res) mock_validate.assert_called_once_with(req, el_id, body['metadata'], delete=False) - mock_create.assert_called_once_with(req, el_id, body) + mock_create.assert_called_once_with( + req, el_id, body, parent_id=self.share['id']) def test_update_all_metadata(self): index_result = self.controller.index(self.req, self.share['id']) @@ -361,7 +362,34 @@ class ShareExportLocationsAPITest(test.TestCase): body=body) self.assertEqual(body, res) mock_validate.assert_called_once_with(req, el_id, body['metadata']) - mock_update.assert_called_once_with(req, el_id, body) + mock_update.assert_called_once_with( + req, el_id, body, parent_id=self.share['id']) + + def test__verify_share_public_share_skips_policy_for_read(self): + public_share = db_utils.create_share(is_public=True) + + self.controller._verify_share( + self.ctxt['user'], public_share['id']) + + self.mock_policy_check.assert_not_called() + + def test__verify_share_public_share_enforces_policy_for_modification(self): + public_share = db_utils.create_share(is_public=True) + + self.controller._verify_share( + self.ctxt['user'], public_share['id'], for_modification=True) + + self.mock_policy_check.assert_called_once_with( + self.ctxt['user'], 'share', 'get', mock.ANY, do_raise=True) + + def test__verify_share_private_share_always_enforces_policy(self): + private_share = db_utils.create_share(is_public=False) + + self.controller._verify_share( + self.ctxt['user'], private_share['id']) + + self.mock_policy_check.assert_called_once_with( + self.ctxt['user'], 'share', 'get', mock.ANY, do_raise=False) def test_delete_metadata(self): index_result = self.controller.index(self.req, self.share['id']) @@ -375,4 +403,5 @@ class ShareExportLocationsAPITest(test.TestCase): version="2.87", use_admin_context=True) self.controller.delete_metadata(req, self.share['id'], el_id, 'fake_key') - mock_delete.assert_called_once_with(req, el_id, 'fake_key') + mock_delete.assert_called_once_with( + req, el_id, 'fake_key', parent_id=self.share['id']) diff --git a/manila/tests/api/v2/test_share_replicas.py b/manila/tests/api/v2/test_share_replicas.py index 4936a99..4ca7f21 100644 --- a/manila/tests/api/v2/test_share_replicas.py +++ b/manila/tests/api/v2/test_share_replicas.py @@ -384,6 +384,43 @@ class ShareReplicasApiTest(test.TestCase): self.mock_policy_check.assert_called_once_with( self.member_context, self.resource_name, 'create') + def test_create_share_from_other_project_not_found(self): + # A share that belongs to another project is not visible to the + # caller, so the ownership lookup must fail with 404. + body = { + 'share_replica': { + 'share_id': 'FAKE_SHAREID', + 'availability_zone': 'FAKE_AZ' + } + } + self.mock_object(share.API, 'get', + mock.Mock(side_effect=exception.NotFound)) + + self.assertRaises(exc.HTTPNotFound, + self.controller.create, + self.replicas_req, body) + self.mock_policy_check.assert_called_once_with( + self.member_context, self.resource_name, 'create') + + def test_create_share_from_other_project_not_authorized(self): + # A public share that belongs to another project: the ownership + # policy check inside the share lookup must raise + # PolicyNotAuthorized, which the WSGI layer converts to 403. + body = { + 'share_replica': { + 'share_id': 'FAKE_SHAREID', + 'availability_zone': 'FAKE_AZ' + } + } + self.mock_object(share.API, 'get', mock.Mock( + side_effect=exception.PolicyNotAuthorized(action='share:get'))) + + self.assertRaises(exception.PolicyNotAuthorized, + self.controller.create, + self.replicas_req, body) + self.mock_policy_check.assert_called_once_with( + self.member_context, self.resource_name, 'create') + def test_create_has_been_soft_deleted(self): share_ref = fake_share.fake_share(is_soft_deleted=True) body = { @@ -395,7 +432,7 @@ class ShareReplicasApiTest(test.TestCase): mock__view_builder_call = self.mock_object( share_replicas.replication_view.ReplicationViewBuilder, 'detail_list') - self.mock_object(share_replicas.db, 'share_get', + self.mock_object(share.API, 'get', mock.Mock(return_value=share_ref)) self.assertRaises(exc.HTTPForbidden, @@ -416,7 +453,7 @@ class ShareReplicasApiTest(test.TestCase): mock__view_builder_call = self.mock_object( share_replicas.replication_view.ReplicationViewBuilder, 'detail_list') - self.mock_object(share_replicas.db, 'share_get', + self.mock_object(share.API, 'get', mock.Mock(return_value=share_ref)) self.assertRaises(exc.HTTPForbidden, @@ -442,7 +479,7 @@ class ShareReplicasApiTest(test.TestCase): mock__view_builder_call = self.mock_object( share_replicas.replication_view.ReplicationViewBuilder, 'detail_list') - self.mock_object(share_replicas.db, 'share_get', + self.mock_object(share.API, 'get', mock.Mock(return_value=fake_replica)) self.mock_object(share_replicas.db, 'share_network_get', mock.Mock(side_effect=exception.ShareNetworkNotFound( @@ -471,7 +508,7 @@ class ShareReplicasApiTest(test.TestCase): } } exc_args = {'id': 'xyz', 'reason': 'abc'} - self.mock_object(share_replicas.db, 'share_get', + self.mock_object(share.API, 'get', mock.Mock(return_value=fake_replica)) self.mock_object(share.API, 'create_share_replica', mock.Mock(side_effect=exception_type(**exc_args))) @@ -513,7 +550,7 @@ class ShareReplicasApiTest(test.TestCase): if self.is_microversion_ge(microversion, '2.95'): body["share_replica"].update({"metadata": {"foo": "bar"}}) - self.mock_object(share_replicas.db, 'share_get', + self.mock_object(share.API, 'get', mock.Mock(return_value=fake_replica)) self.mock_object(share.API, 'create_share_replica', mock.Mock(return_value=fake_replica)) diff --git a/manila/tests/api/v2/test_shares.py b/manila/tests/api/v2/test_shares.py index 170f825..531db4c 100644 --- a/manila/tests/api/v2/test_shares.py +++ b/manila/tests/api/v2/test_shares.py @@ -1995,8 +1995,14 @@ class ShareAPITest(test.TestCase): req = fakes.HTTPRequest.blank('/v2/fake/shares/1/action', version='2.69') body = {"soft_delete": None} + mock_get = self.mock_object( + share_api.API, 'get', mock.Mock(return_value=self.share)) + resp = self.controller.share_soft_delete(req, 1, body=body) + self.assertEqual(202, resp.status_int) + mock_get.assert_called_once_with( + req.environ['manila.context'], 1, for_modification=True) def test_share_soft_delete_has_been_soft_deleted_already(self): req = fakes.HTTPRequest.blank('/v2/fake/shares/1/action', @@ -2029,10 +2035,15 @@ class ShareAPITest(test.TestCase): req = fakes.HTTPRequest.blank('/v2/fake/shares/1/action', version='2.69') body = {"restore": None} - self.mock_object(share_api.API, 'get', - mock.Mock(return_value=self.share_in_recycle_bin)) + mock_get = self.mock_object( + share_api.API, 'get', + mock.Mock(return_value=self.share_in_recycle_bin)) + resp = self.controller.share_restore(req, 1, body=body) + self.assertEqual(202, resp.status_int) + mock_get.assert_called_once_with( + req.environ['manila.context'], 1, for_modification=True) def test_share_restore_with_deleting_status(self): req = fakes.HTTPRequest.blank('/v2/fake/shares/1/action', @@ -2045,6 +2056,34 @@ class ShareAPITest(test.TestCase): webob.exc.HTTPForbidden, self.controller.share_restore, req, 1, body=body) + def test_share_soft_delete_policy_not_authorized(self): + req = fakes.HTTPRequest.blank('/v2/fake/shares/1/action', + version='2.69') + body = {"soft_delete": None} + self.mock_object( + share_api.API, 'get', + mock.Mock(side_effect=exception.PolicyNotAuthorized( + action='share:get'))) + + self.assertRaises( + exception.PolicyNotAuthorized, + self.controller.share_soft_delete, + req, 1, body=body) + + def test_share_restore_policy_not_authorized(self): + req = fakes.HTTPRequest.blank('/v2/fake/shares/1/action', + version='2.69') + body = {"restore": None} + self.mock_object( + share_api.API, 'get', + mock.Mock(side_effect=exception.PolicyNotAuthorized( + action='share:get'))) + + self.assertRaises( + exception.PolicyNotAuthorized, + self.controller.share_restore, + req, 1, body=body) + def test_share_delete(self): req = fakes.HTTPRequest.blank('/v2/fake/shares/1') resp = self.controller.delete(req, 1) @@ -2975,7 +3014,8 @@ class ShareActionsTest(test.TestCase): mock.Mock(return_value={'fake': 'fake'})) self.mock_object(self.controller._access_view_builder, 'view', mock.Mock(return_value={'access': {'fake': 'fake'}})) - self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) + mock_get = self.mock_object( + share_api.API, 'get', mock.Mock(return_value=share)) id = 'fake_share_id' body = {'os-allow_access': access} @@ -2985,6 +3025,8 @@ class ShareActionsTest(test.TestCase): res = self.controller._allow_access(req, id, body) self.assertEqual(expected, res) + mock_get.assert_called_once_with( + req.environ['manila.context'], id, for_modification=True) self.mock_policy_check.assert_called_once_with( req.environ['manila.context'], 'share', 'allow_access') @@ -3249,7 +3291,8 @@ class ShareActionsTest(test.TestCase): self.mock_object(share_api.API, "access_get", _fake_access_get) share_network = db_utils.create_share_network() share = db_utils.create_share(share_network_id=share_network['id']) - self.mock_object(share_api.API, 'get', mock.Mock(return_value=share)) + mock_get = self.mock_object( + share_api.API, 'get', mock.Mock(return_value=share)) self.mock_object(self.controller, '_check_for_access_rule_locks') id = 'fake_share_id' @@ -3260,12 +3303,15 @@ class ShareActionsTest(test.TestCase): res = self.controller._deny_access(req, id, body) self.assertEqual(202, res.status_int) + context = req.environ['manila.context'] + mock_get.assert_has_calls([ + mock.call(context, id, for_modification=True), + mock.call(context, id), + ]) self.controller._check_for_access_rule_locks.assert_called_once_with( - req.environ['manila.context'], access_data, - access_data['access_id'], id - ) + context, access_data, access_data['access_id'], id) self.mock_policy_check.assert_called_once_with( - req.environ['manila.context'], 'share', 'deny_access') + context, 'share', 'deny_access') def test_deny_access_not_found(self): def _stub_deny_access(*args, **kwargs): @@ -3307,6 +3353,40 @@ class ShareActionsTest(test.TestCase): context, body['deny_access'], body_data['access_id'], id ) + def test__allow_access_policy_not_authorized_for_modification(self): + mock_get = self.mock_object( + share_api.API, 'get', + mock.Mock(side_effect=exception.PolicyNotAuthorized( + action='share:get'))) + id = 'fake_share_id' + body = {'os-allow_access': { + 'access_type': 'ip', 'access_to': '127.0.0.1'}} + req = fakes.HTTPRequest.blank( + '/v2/tenant1/shares/%s/action' % id) + + self.assertRaises( + exception.PolicyNotAuthorized, + self.controller._allow_access, req, id, body) + mock_get.assert_called_once_with( + req.environ['manila.context'], id, for_modification=True) + + def test__deny_access_policy_not_authorized_for_modification(self): + mock_get = self.mock_object( + share_api.API, 'get', + mock.Mock(side_effect=exception.PolicyNotAuthorized( + action='share:get'))) + self.mock_object(self.controller, '_check_for_access_rule_locks') + id = 'fake_share_id' + body = {"os-deny_access": {"access_id": 'fake_acces_id'}} + req = fakes.HTTPRequest.blank( + '/v2/tenant1/shares/%s/action' % id) + + self.assertRaises( + exception.PolicyNotAuthorized, + self.controller._deny_access, req, id, body) + mock_get.assert_called_once_with( + req.environ['manila.context'], id, for_modification=True) + def test__check_for_access_rule_locks_no_locks(self): self.mock_object( resource_locks.API, "get_all", mock.Mock(return_value=([], 0))) diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py index cbf2050..2183a05 100644 --- a/manila/tests/share/test_api.py +++ b/manila/tests/share/test_api.py @@ -3414,6 +3414,64 @@ class ShareAPITestCase(test.TestCase): ctx, 'share', 'get', share, do_raise=False) db_api.share_get.assert_called_once_with(ctx, share['id']) + def test_get_public_share_skips_policy_check(self): + fake_share = db_utils.create_share(is_public=True) + self.mock_object(db_api, 'share_get', + mock.Mock(return_value=fake_share)) + + result = self.api.get(self.context, 'fakeid') + + self.assertEqual(fake_share, result) + share_api.policy.check_policy.assert_not_called() + + def test_get_public_share_for_modification_checks_policy(self): + fake_share = db_utils.create_share(is_public=True) + share_api.policy.check_policy.return_value = True + self.mock_object(db_api, 'share_get', + mock.Mock(return_value=fake_share)) + + result = self.api.get( + self.context, 'fakeid', for_modification=True) + + self.assertEqual(fake_share, result) + share_api.policy.check_policy.assert_called_once_with( + self.context, 'share', 'get', fake_share, do_raise=True) + + def test_get_public_share_for_modification_not_authorized(self): + fake_share = db_utils.create_share( + is_public=True, + project_id='df6d65cc1f8946ba86be06b8140ec4b3') + self.mock_object( + share_api.policy, 'check_policy', + mock.Mock(side_effect=exception.PolicyNotAuthorized( + action='share:get'))) + self.mock_object(db_api, 'share_get', + mock.Mock(return_value=fake_share)) + ctx = context.RequestContext('fake_uid', 'fake_pid', is_admin=False) + + self.assertRaises(exception.PolicyNotAuthorized, + self.api.get, + ctx, + fake_share['id'], + for_modification=True) + + def test_get_private_share_for_modification_not_authorized(self): + fake_share = db_utils.create_share( + is_public=False, + project_id='df6d65cc1f8946ba86be06b8140ec4b3') + share_api.policy.check_policy.return_value = False + self.mock_object(db_api, 'share_get', + mock.Mock(return_value=fake_share)) + ctx = context.RequestContext('fake_uid', 'fake_pid', is_admin=False) + + self.assertRaises(exception.NotFound, + self.api.get, + ctx, + fake_share['id'], + for_modification=True) + share_api.policy.check_policy.assert_called_once_with( + ctx, 'share', 'get', fake_share, do_raise=False) + @mock.patch.object(db_api, 'share_snapshot_get_all_by_project', mock.Mock()) def test_get_all_snapshots_admin_not_all_tenants(self): diff --git a/releasenotes/notes/bug-2161412-enforce-policy-checks-on-public-shares-6cb60f669bb20f6c.yaml b/releasenotes/notes/bug-2161412-enforce-policy-checks-on-public-shares-6cb60f669bb20f6c.yaml new file mode 100644 index 0000000..8709e6e --- /dev/null +++ b/releasenotes/notes/bug-2161412-enforce-policy-checks-on-public-shares-6cb60f669bb20f6c.yaml @@ -0,0 +1,11 @@ +--- +security: + - | + Any user was able to manipulate the access rules, create backups or + replicas, soft delete or restore public shares they did not own, + and read or manipulate the export location metadata of shares they + did not own. These operations now enforce ownership, and export + location metadata is scoped to the share in the request URL. For + details, see `Launchpad bug #2161412 + <https://bugs.launchpad.net/manila/+bug/2161412>`_ and `Launchpad + bug #2168962 <https://bugs.launchpad.net/manila/+bug/2168962>`_. -- 2.54.0 (Apple Git-157) #+end_src