From 2620d14c5fae378a5e8d7d550d5ae8efb263d63a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Douglas=20Mendiz=C3=A1bal?= Date: Thu, 20 Jan 2022 14:56:37 -0600 Subject: [PATCH] Allow secret delete by users with "creator" role Users with the "creator" role on a project can now delete secrets owned by the project even if the user is different than the user that originally created the secret. Previous to this fix a user with the "creator" role was only allowed to delete a secret owned by the project if they were also the same user that originally created, which was inconsistent with the way that deletes are handled by other OpenStack projects that integrate with Barbican. This change does not affect the policy for delting private secrets (i.e. secrets with the "project-access" flag set to "false"). Story: 2009791 Task: 44324 Change-Id: Ie3e3adc1ee02d770de050f5cfa8110774bb1f661 --- api-guide/source/acls.rst | 3 ++- barbican/common/policies/secrets.py | 2 ++ barbican/tests/api/test_resources_policy.py | 15 +++++++++++-- doc/source/admin/access_control.rst | 21 +++++++++++-------- .../api/v1/functional/test_secrets_rbac.py | 2 +- ...allow-creator-delete-06dd3eb670d0e624.yaml | 11 ++++++++++ 6 files changed, 41 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/fix-story-2009791-allow-creator-delete-06dd3eb670d0e624.yaml diff --git a/api-guide/source/acls.rst b/api-guide/source/acls.rst index 6f137fda2..24d97eac7 100644 --- a/api-guide/source/acls.rst +++ b/api-guide/source/acls.rst @@ -48,7 +48,8 @@ Following ACL rules are defined and used as `OR` in resource access policy: * ACL based access is allowed when token user is present in secret/container operation specific ACL user list e.g. token user present in `read` users list. * When secret/container resource is marked private, then project-level RBAC users access is not - allowed. + allowed. e.g. When a secret is marked private, only the user who created it + or a user with the "admin" role on the project will be able to remove it. .. note:: diff --git a/barbican/common/policies/secrets.py b/barbican/common/policies/secrets.py index 17e24aacf..5989e836b 100644 --- a/barbican/common/policies/secrets.py +++ b/barbican/common/policies/secrets.py @@ -72,6 +72,8 @@ rules = [ name='secret:delete', check_str='rule:secret_project_admin or ' + 'rule:secret_project_creator or ' + + '(rule:secret_project_creator_role and ' + + 'not rule:secret_private_read) or ' + f"({_PROJECT_MEMBER} and ({_SECRET_CREATOR} or " + f"{_SECRET_IS_NOT_PRIVATE})) or {_PROJECT_ADMIN}", scope_types=['project'], diff --git a/barbican/tests/api/test_resources_policy.py b/barbican/tests/api/test_resources_policy.py index bf6ab224a..6b1cd3a69 100644 --- a/barbican/tests/api/test_resources_policy.py +++ b/barbican/tests/api/test_resources_policy.py @@ -663,15 +663,26 @@ class WhenTestingSecretResource(BaseTestCase): content_type="application/octet-stream") def test_should_pass_delete_secret(self): - self._assert_pass_rbac(['admin'], self._invoke_on_delete, + self._assert_pass_rbac(['admin', 'creator'], self._invoke_on_delete, user_id=self.user_id, project_id=self.external_project_id) def test_should_raise_delete_secret(self): - """A non-admin user cannot delete other user's secret. + """Only admin and creator can delete secrets User id is different from initial user who has created the secret. """ + self._assert_fail_rbac([None, 'audit', 'observer', 'bogus'], + self._invoke_on_delete, + user_id=self.user_id, + project_id=self.external_project_id) + + def test_should_raise_delete_private_secret(self): + self.acl_list.pop() # remove read acl from default setup + acl_read = models.SecretACL(secret_id=self.secret_id, operation='read', + project_access=False, + user_ids=['anyRandomUserX', 'aclUser1']) + self.acl_list.append(acl_read) self._assert_fail_rbac([None, 'audit', 'observer', 'creator', 'bogus'], self._invoke_on_delete, user_id=self.user_id, diff --git a/doc/source/admin/access_control.rst b/doc/source/admin/access_control.rst index 8f5eae6d8..2d56cf895 100644 --- a/doc/source/admin/access_control.rst +++ b/doc/source/admin/access_control.rst @@ -58,11 +58,10 @@ admin by the project for which the admin role is scoped. creator - Users with this role are allowed to create new resources and can only - delete resources which are originally created (owned) by them. Users with - this role cannot delete other user's resources managed within same project. - They are also allowed full access to existing secrets owned by the project - in scope. + Users with this role are allowed to create new resources and can also + delete resources which are owned by the project for which the creator role + is scoped. They are also allowed full access to existing secrets owned by + the project in scope. observer Users with this role are allowed to access to existing resources but are @@ -76,10 +75,14 @@ Access Control List API ----------------------- There are some limitations that result from scoping ownership of a secret at the -project level. For example, there is no easy way for a user to upload a secret -for which only they have access. There is also no easy way to grant a user -access to only a single secret. +project level. For example, it is not possible to grant a user access to a +single secret, as granting a role on a project would allow access to all +all secrets owned by that project. -To address this limitations the Key Manager service includes an Access Control +Additionally, there is no easy way to upload a private secret (i.e. a secret +that only you have access to) without creating a new project for which only +you have roles assigned on it. + +To address these limitations the Key Manager service includes an Access Control List (ACL) API. For full details see the `ACL API User Guide `__ diff --git a/functionaltests/api/v1/functional/test_secrets_rbac.py b/functionaltests/api/v1/functional/test_secrets_rbac.py index 6fce00d52..e9d977fd4 100644 --- a/functionaltests/api/v1/functional/test_secrets_rbac.py +++ b/functionaltests/api/v1/functional/test_secrets_rbac.py @@ -90,7 +90,7 @@ test_data_rbac_delete_secret = { 'with_creator_a': {'user': creator_a, 'admin': creator_a, 'expected_return': 204}, 'with_creator_a_2': {'user': creator_a_2, 'admin': creator_a, - 'expected_return': 403}, + 'expected_return': 204}, 'with_observer_a': {'user': observer_a, 'admin': admin_a, 'expected_return': 403}, 'with_auditor_a': {'user': auditor_a, 'admin': admin_a, diff --git a/releasenotes/notes/fix-story-2009791-allow-creator-delete-06dd3eb670d0e624.yaml b/releasenotes/notes/fix-story-2009791-allow-creator-delete-06dd3eb670d0e624.yaml new file mode 100644 index 000000000..a2a837336 --- /dev/null +++ b/releasenotes/notes/fix-story-2009791-allow-creator-delete-06dd3eb670d0e624.yaml @@ -0,0 +1,11 @@ +--- +security: + - | + Fixed Story #2009791: Users with the "creator" role on a project can now + delete secrets owned by the project even if the user is different than + the user that originally created the secret. Previous to this fix a user + with the "creator" role was only allowed to delete a secret owned by the + project if they were also the same user that originally created, which + was inconsistent with the way that deletes are handled by other OpenStack + projects that integrate with Barbican. This change does not affect private + secrets (i.e. secrets with the "project-access" flag set to "false").