From fc0f669decd3a7c6de2e7b4b01a727764b927a3b Mon Sep 17 00:00:00 2001
From: Goutham Pacha Ravi <gouthampravi@gmail.com>
Date: Mon, 1 Mar 2021 23:05:56 -0800
Subject: [PATCH] RBAC tightening for share access rule
Non privileged users of unrelated projects
must not be able to retrieve details of an
access rule. We can add a further check to
/share-access-rules APIs to validate that
the caller has access to the share that these
rules pertain to.
Change-Id: I0009a3d682ee5d9a946821c3f82dfd90faa886aa
Closes-Bug: #1917417
Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>
---
manila/share/api.py | 4 +++
manila/tests/api/v2/test_share_accesses.py | 31 +++++++++++++++++++
manila/tests/share/test_api.py | 6 ++--
...n-share-access-rules-efdddaf9e6f68fdf.yaml | 7 +++++
4 files changed, 46 insertions(+), 2 deletions(-)
create mode 100644 releasenotes/notes/bug-1917417-fix-rbac-check-on-share-access-rules-efdddaf9e6f68fdf.yaml
diff --git a/manila/share/api.py b/manila/share/api.py
index bea2845707..7f4d8f377d 100644
--- a/manila/share/api.py
+++ b/manila/share/api.py
@@ -1987,6 +1987,10 @@ class API(base.Base):
"""Returns access rule with the id."""
policy.check_policy(context, 'share', 'access_get')
rule = self.db.share_access_get(context, access_id)
+ # NOTE(gouthamr): Check if the caller has access to the share that
+ # the rule belongs to:
+ self.get(context, rule['share_id'])
+
return rule
@policy.wrap_check_policy('share')
diff --git a/manila/tests/api/v2/test_share_accesses.py b/manila/tests/api/v2/test_share_accesses.py
index 4163729a19..2f3aa33baf 100644
--- a/manila/tests/api/v2/test_share_accesses.py
+++ b/manila/tests/api/v2/test_share_accesses.py
@@ -113,6 +113,37 @@ class ShareAccessesAPITest(test.TestCase):
version="2.45")
self.assertRaises(exc.HTTPBadRequest, self.controller.index, req)
+ def test_show_access_not_authorized(self):
+ share = db_utils.create_share(
+ project_id='c3c5ec1ccc4640d0af1914cbf11f05ad',
+ is_public=False)
+ access = db_utils.create_access(
+ id='76699c6b-f3da-47d7-b468-364f1347ba04',
+ share_id=share['id'])
+ req = fakes.HTTPRequest.blank(
+ '/v2/share-access-rules/%s' % access['id'],
+ version="2.45")
+ self.mock_object(
+ policy, 'check_policy',
+ mock.Mock(side_effect=[None, None, exception.NotAuthorized]))
+
+ self.assertRaises(exception.NotAuthorized,
+ self.controller.show,
+ req,
+ access['id'])
+ policy.check_policy.assert_has_calls([
+ mock.call(req.environ['manila.context'],
+ 'share_access_rule', 'get'),
+ mock.call(req.environ['manila.context'],
+ 'share', 'access_get'),
+ mock.call(req.environ['manila.context'],
+ 'share', 'get', mock.ANY)])
+ policy_check_call_args_list = policy.check_policy.call_args_list[2][0]
+ share_being_checked = policy_check_call_args_list[3]
+ self.assertEqual('c3c5ec1ccc4640d0af1914cbf11f05ad',
+ share_being_checked['project_id'])
+ self.assertIs(False, share_being_checked['is_public'])
+
def test_show_access_not_found(self):
self.assertRaises(
exc.HTTPNotFound,
diff --git a/manila/tests/share/test_api.py b/manila/tests/share/test_api.py
index 815396291e..074aad6389 100644
--- a/manila/tests/share/test_api.py
+++ b/manila/tests/share/test_api.py
@@ -2695,11 +2695,13 @@ class ShareAPITestCase(test.TestCase):
def test_access_get(self):
with mock.patch.object(db_api, 'share_access_get',
- mock.Mock(return_value='fake')):
+ mock.Mock(return_value={'share_id': 'fake'})):
+ self.mock_object(self.api, 'get')
rule = self.api.access_get(self.context, 'fakeid')
- self.assertEqual('fake', rule)
+ self.assertEqual({'share_id': 'fake'}, rule)
db_api.share_access_get.assert_called_once_with(
self.context, 'fakeid')
+ self.api.get.assert_called_once_with(self.context, 'fake')
def test_access_get_all(self):
share = db_utils.create_share(id='fakeid')
diff --git a/releasenotes/notes/bug-1917417-fix-rbac-check-on-share-access-rules-efdddaf9e6f68fdf.yaml b/releasenotes/notes/bug-1917417-fix-rbac-check-on-share-access-rules-efdddaf9e6f68fdf.yaml
new file mode 100644
index 0000000000..371eb4cfef
--- /dev/null
+++ b/releasenotes/notes/bug-1917417-fix-rbac-check-on-share-access-rules-efdddaf9e6f68fdf.yaml
@@ -0,0 +1,7 @@
+---
+security:
+ - |
+ An RBAC policy check has been enforced against the GET /share-access-rules
+ API to ensure that users are permitted to access the share that the
+ access rule belongs to. See `bug 1917417
+ <https://launchpad.net/bugs/1917417>`_ for more details.