From d02a8d836c560a3571b0a2f17f102e25bc7c5301 Mon Sep 17 00:00:00 2001 From: Mykola Yakovliev Date: Tue, 30 Oct 2018 21:35:20 -0500 Subject: [PATCH] RequirementsAuthority multi role support enhancement This patchset eliminates different behaviour between policy_authority and requirements_authority. Problem description: `rbac_test_roles = [member,]` Policy authority: `update_port: role:member and role:viewer` Results in 403/False (we are member but not viewer). Requirements authority: ``` req_auth: update_port: - member - viewer ``` Results in 200/True (member in update_port list). Proposed solution: Change requirements_authority file sytax to support comma separated roles to be considered as logical and. Depends-On: https://review.openstack.org/#/c/606110/ Change-Id: I2e2a4a2020f5e85af15f1836d69386bc91a2d2ec Co-Authored-By: Felipe Monteiro --- .../framework/requirements_authority.rst | 89 ++++++++++--- .../requirements_authority.py | 34 ++++- .../tests/unit/resources/rbac_roles.yaml | 4 + .../tests/unit/test_requirements_authority.py | 121 ++++++++++++++++-- ...y-multi-role-support-0fe53fc49567e595.yaml | 37 ++++++ 5 files changed, 253 insertions(+), 32 deletions(-) create mode 100644 releasenotes/notes/requirements-authority-multi-role-support-0fe53fc49567e595.yaml diff --git a/doc/source/framework/requirements_authority.rst b/doc/source/framework/requirements_authority.rst index 628f0c07..cf7c51cd 100644 --- a/doc/source/framework/requirements_authority.rst +++ b/doc/source/framework/requirements_authority.rst @@ -7,8 +7,9 @@ Overview -------- Requirements-driven approach to declaring the expected RBAC test results -referenced by Patrole. Uses a high-level YAML syntax to crystallize policy -requirements concisely and unambiguously. +referenced by Patrole. These requirements express the *intention* behind the +policy. A high-level YAML syntax is used to concisely and clearly map each +policy action to the list of associated roles. .. note:: @@ -29,10 +30,6 @@ expected test results by performing lookups against a :ref:`custom-requirements-file` which precisely defines the cloud's RBAC requirements. -Using a high-level declarative language, the requirements are captured -unambiguously in the :ref:`custom-requirements-file`, allowing operators to -validate their requirements against their OpenStack cloud. - This validation approach should be used when: * The cloud has heavily customized policy files that require careful validation @@ -71,31 +68,87 @@ File path of the YAML file that defines your RBAC requirements. This file must be located on the same host that Patrole runs on. The YAML file should be written as follows: + .. code-block:: yaml + + : + : + - + - + : + - , + : + : + - + +Where: + +* ``service`` - the service that is being tested (Cinder, Nova, etc.). +* ``api_action`` - the policy action that is being tested. Examples: + + * volume:create + * os_compute_api:servers:start + * add_image + +* ``allowed_role`` - the ``oslo.policy`` role that is allowed to perform the + API. + +Each item under ``logical_or_example`` is "logical OR"-ed together. Each role +in the comma-separated string under ``logical_and_example`` is "logical AND"-ed +together. And each item prefixed with "!" under ``logical_not_example`` is +"logical negated". + +.. note:: + + The custom requirements file only allows policy actions to be mapped to + the associated roles that define it. Complex ``oslo.policy`` constructs + like ``literals`` or ``GenericChecks`` are not supported. For more + information, reference the `oslo.policy documentation`_. + +.. _oslo.policy documentation: https://docs.openstack.org/oslo.policy/latest/reference/api/oslo_policy.policy.html#policy-rule-expressions + +Examples +~~~~~~~~ + +Items within ``api_action`` are considered as logical or, so you may read: + .. code-block:: yaml : + # "api_action_a: allowed_role_1 or allowed_role_2 or allowed_role_3" : - - - - : - - - - - : - : + +as `` or or ``. + +Roles within comma-separated items are considered as logic and, so you may +read: + +.. code-block:: yaml + + : + # "api_action_a: (allowed_role_1 and allowed_role_2) or allowed_role_3" + : + - , - -Where: +as `` and or ``. -service = the service that is being tested (Cinder, Nova, etc.). +Also negative roles may be defined with an exclamation mark ahead of role: -api_action = the policy action that is being tested. Examples: +.. code-block:: yaml -* volume:create -* os_compute_api:servers:start -* add_image + : + # "api_action_a: (allowed_role_1 and allowed_role_2 and not + # disallowed_role_4) or allowed_role_3" + : + - , , ! + - + +This example must be read as `` and and not + or ``. -allowed_role = the ``oslo.policy`` role that is allowed to perform the API. Implementation -------------- diff --git a/patrole_tempest_plugin/requirements_authority.py b/patrole_tempest_plugin/requirements_authority.py index 57caf79e..4697c3bc 100644 --- a/patrole_tempest_plugin/requirements_authority.py +++ b/patrole_tempest_plugin/requirements_authority.py @@ -12,6 +12,7 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +import copy import yaml from oslo_log import log as logging @@ -50,7 +51,7 @@ class RequirementsParser(object): : : - - - + - , - : - @@ -67,7 +68,16 @@ class RequirementsParser(object): try: for section in RequirementsParser.Inner._rbac_map: if component in section: - return section[component] + rules = copy.copy(section[component]) + + for rule in rules: + rules[rule] = [ + roles.split(',') for roles in rules[rule]] + + for i, role_pack in enumerate(rules[rule]): + rules[rule][i] = [r.strip() for r in role_pack] + + return rules except yaml.parser.ParserError: LOG.error("Error while parsing the requirements YAML file. Did " "you pass a valid component name from the test case?") @@ -115,8 +125,24 @@ class RequirementsAuthority(RbacAuthority): "empty. Ensure the requirements YAML file is correctly " "formatted.") try: - _api = self.roles_dict[rule_name] - return all(role in _api for role in roles) + requirement_roles = self.roles_dict[rule_name] + + for role_reqs in requirement_roles: + required_roles = [ + role for role in role_reqs if not role.startswith("!")] + forbidden_roles = [ + role[1:] for role in role_reqs if role.startswith("!")] + + # User must have all required roles + required_passed = all([r in roles for r in required_roles]) + # User must not have any forbidden roles + forbidden_passed = all([r not in forbidden_roles + for r in roles]) + + if required_passed and forbidden_passed: + return True + + return False except KeyError: raise KeyError("'%s' API is not defined in the requirements YAML " "file" % rule_name) diff --git a/patrole_tempest_plugin/tests/unit/resources/rbac_roles.yaml b/patrole_tempest_plugin/tests/unit/resources/rbac_roles.yaml index c5436d01..357e0e6b 100644 --- a/patrole_tempest_plugin/tests/unit/resources/rbac_roles.yaml +++ b/patrole_tempest_plugin/tests/unit/resources/rbac_roles.yaml @@ -4,3 +4,7 @@ Test: - _member_ test:create2: - test_member + test:create3: + - test_member, _member_ + test:create4: + - test_member, !_member_ \ No newline at end of file diff --git a/patrole_tempest_plugin/tests/unit/test_requirements_authority.py b/patrole_tempest_plugin/tests/unit/test_requirements_authority.py index 4b75ef5a..0f7310e4 100644 --- a/patrole_tempest_plugin/tests/unit/test_requirements_authority.py +++ b/patrole_tempest_plugin/tests/unit/test_requirements_authority.py @@ -20,16 +20,25 @@ from tempest.tests import base from patrole_tempest_plugin import requirements_authority as req_auth -class RequirementsAuthorityTest(base.TestCase): +class BaseRequirementsAuthorityTest(base.TestCase): def setUp(self): - super(RequirementsAuthorityTest, self).setUp() + super(BaseRequirementsAuthorityTest, self).setUp() self.rbac_auth = req_auth.RequirementsAuthority() self.current_directory = os.path.dirname(os.path.realpath(__file__)) self.yaml_test_file = os.path.join(self.current_directory, 'resources', 'rbac_roles.yaml') - self.expected_result = {'test:create': ['test_member', '_member_'], - 'test:create2': ['test_member']} + self.expected_result = {'test:create': [['test_member'], ['_member_']], + 'test:create2': [['test_member']], + 'test:create3': [['test_member', '_member_']], + 'test:create4': [['test_member', '!_member_']]} + self.expected_rbac_map = {'test:create': ['test_member', '_member_'], + 'test:create2': ['test_member'], + 'test:create3': ['test_member, _member_'], + 'test:create4': ['test_member, !_member_']} + + +class RequirementsAuthorityTest(BaseRequirementsAuthorityTest): def test_requirements_auth_init(self): rbac_auth = req_auth.RequirementsAuthority(self.yaml_test_file, 'Test') @@ -41,11 +50,11 @@ class RequirementsAuthorityTest(base.TestCase): self.rbac_auth.allowed, "", [""]) def test_auth_allowed_role_in_api(self): - self.rbac_auth.roles_dict = {'api': ['_member_']} + self.rbac_auth.roles_dict = {'api': [['_member_']]} self.assertTrue(self.rbac_auth.allowed("api", ["_member_"])) def test_auth_allowed_role_not_in_api(self): - self.rbac_auth.roles_dict = {'api': ['_member_']} + self.rbac_auth.roles_dict = {'api': [['_member_']]} self.assertFalse(self.rbac_auth.allowed("api", "support_member")) def test_parser_get_allowed_except_keyerror(self): @@ -55,12 +64,12 @@ class RequirementsAuthorityTest(base.TestCase): def test_parser_init(self): req_auth.RequirementsParser(self.yaml_test_file) - self.assertEqual([{'Test': self.expected_result}], + self.assertEqual([{'Test': self.expected_rbac_map}], req_auth.RequirementsParser.Inner._rbac_map) def test_parser_role_in_api(self): req_auth.RequirementsParser.Inner._rbac_map = \ - [{'Test': self.expected_result}] + [{'Test': self.expected_rbac_map}] self.rbac_auth.roles_dict = req_auth.RequirementsParser.parse("Test") self.assertEqual(self.expected_result, self.rbac_auth.roles_dict) @@ -69,7 +78,7 @@ class RequirementsAuthorityTest(base.TestCase): def test_parser_role_not_in_api(self): req_auth.RequirementsParser.Inner._rbac_map = \ - [{'Test': self.expected_result}] + [{'Test': self.expected_rbac_map}] self.rbac_auth.roles_dict = req_auth.RequirementsParser.parse("Test") self.assertEqual(self.expected_result, self.rbac_auth.roles_dict) @@ -77,10 +86,102 @@ class RequirementsAuthorityTest(base.TestCase): def test_parser_except_invalid_configuration(self): req_auth.RequirementsParser.Inner._rbac_map = \ - [{'Test': self.expected_result}] + [{'Test': self.expected_rbac_map}] self.rbac_auth.roles_dict = \ req_auth.RequirementsParser.parse("Failure") self.assertIsNone(self.rbac_auth.roles_dict) self.assertRaises(exceptions.InvalidConfiguration, self.rbac_auth.allowed, "", [""]) + + def test_auth_allowed_exclamation_mark_syntax_single_role(self): + """Ensure that exclamation mark in front of role is dropped, and not + considered as part of role itself. + """ + + self.rbac_auth.roles_dict = {'api': [['!admin']]} + self.assertTrue(self.rbac_auth.allowed("api", ["member"])) + self.assertTrue(self.rbac_auth.allowed("api", ["!admin"])) + self.assertFalse(self.rbac_auth.allowed("api", ["admin"])) + + +class RequirementsAuthorityMultiRoleTest(BaseRequirementsAuthorityTest): + + def test_auth_allowed_exclamation_mark_syntax_multi_role(self): + """Ensure that exclamation mark in front of role is dropped, and not + considered as part of role itself. + """ + + self.rbac_auth.roles_dict = {'api': [['member', '!admin']]} + self.assertFalse(self.rbac_auth.allowed("api", ["member", "admin"])) + self.assertTrue(self.rbac_auth.allowed("api", ["member", "!admin"])) + + def test_auth_allowed_single_rule_scenario(self): + # member and support and not admin and not manager + self.rbac_auth.roles_dict = {'api': [['member', 'support', + '!admin', '!manager']]} + + # User is member and support and not manager or admin + self.assertTrue(self.rbac_auth.allowed("api", ["member", + "support"])) + + # User is member and not manager or admin, but not support + self.assertFalse(self.rbac_auth.allowed("api", ["member"])) + + # User is support and not manager or admin, but not member + self.assertFalse(self.rbac_auth.allowed("api", ["support"])) + + # User is member and support and not manager, but have admin role + self.assertFalse(self.rbac_auth.allowed("api", ["member", + "support", + "admin"])) + + # User is member and not manager, but have admin role and not support + self.assertFalse(self.rbac_auth.allowed("api", ["member", + "admin"])) + + # User is member and support, but have manager and admin roles + self.assertFalse(self.rbac_auth.allowed("api", ["member", + "support", + "admin", + "manager"])) + + def test_auth_allowed_multi_rule_scenario(self): + rules = [ + ['member', 'support', '!admin', '!manager'], + ['member', 'admin'], + ["manager"] + ] + self.rbac_auth.roles_dict = {'api': rules} + + # Not a single role allows viewer + self.assertFalse(self.rbac_auth.allowed("api", ["viewer"])) + # We have no rule that allows support and admin + self.assertFalse(self.rbac_auth.allowed("api", ["support", + "admin"])) + # There is no rule that requires member without additional requirements + self.assertFalse(self.rbac_auth.allowed("api", ["member"])) + + # Pass with rules[2] + self.assertTrue(self.rbac_auth.allowed("api", ["manager"])) + # Pass with rules[0] + self.assertTrue(self.rbac_auth.allowed("api", ["member", + "support"])) + # Pass with rules[1] + self.assertTrue(self.rbac_auth.allowed("api", ["member", + "admin"])) + # Pass with rules[2] + self.assertTrue(self.rbac_auth.allowed("api", ["manager", + "admin"])) + # Pass with rules[1] + self.assertTrue(self.rbac_auth.allowed("api", ["member", + "support", + "admin"])) + # Pass with rules[1] + self.assertTrue(self.rbac_auth.allowed("api", ["member", + "support", + "admin", + "manager"])) + # Pass with rules[2] + self.assertTrue(self.rbac_auth.allowed("api", ["admin", + "manager"])) diff --git a/releasenotes/notes/requirements-authority-multi-role-support-0fe53fc49567e595.yaml b/releasenotes/notes/requirements-authority-multi-role-support-0fe53fc49567e595.yaml new file mode 100644 index 00000000..ffbae0ae --- /dev/null +++ b/releasenotes/notes/requirements-authority-multi-role-support-0fe53fc49567e595.yaml @@ -0,0 +1,37 @@ +--- +features: + - | + The ``requirements_authority`` module now supports the following 3 cases: + + * logical or operation of roles (existing functionality) + * logical and operation of roles (new functionality) + * logical not operation of roles (new functionality) + + .. code-block:: yaml + + : + : + - + - + : + - , + : + : + - + + Each item under ``logical_or_example`` is "logical OR"-ed together. Each + role in the comma-separated string under ``logical_and_example`` is + "logical AND"-ed together. And each item prefixed with "!" under + ``logical_not_example`` is "logical negated". + + This allows for expressing many more complex cases using the + ``requirements_authority`` YAML syntax. For example, the policy rule + (i.e. what may exist in a ``policy.yaml`` file):: + + "foo_rule: (role:a and not role:b) or role:c" + + May now be expressed using the YAML syntax as:: + + foo_rule: + - a, !b + - c