Update rbac_rule_validation for multi-policy support
Introduces foundational logic needed for multi-policy support
to rbac_rule_validation module. Patrole now offers support for
multiple policies. The ``rules`` argument has been added to the
``rbac_rule_validation.action`` decorator, which takes a list of
policy names which Patrole will use to determine the expected test
result. This allows Patrole to more accurately determine
whether RBAC is configured correctly, since some API endpoints
enforce multiple policies.
The new ``rules`` argument is implemented for
test_unlock_server_override test which corresponds to [0]
which enforces:
rules=["os_compute_api:os-lock-server:unlock:unlock",
"os_compute_api:os-lock-server:unlock:unlock_override"]
which is set for this test.
The ``rule`` argument in the ``rbac_rule_validation.action``
decorator has been deprecated in favor of ``rules``.
The following will be carried out in additional follow up patches:
* Renaming rule to rules
* Adding multi-policy support carefully for selected APIs
to be tracked via an etherpad
* Updating Patrole documentation with multi-policy support
details
[0] https://github.com/openstack/nova/blob/0ab78890c155f0b6ffc7c4148b26642f47aa7070/nova/api/openstack/compute/lock_server.py#L42
Partially Implements: bp rbac-testing-multiple-policies
Change-Id: Iec651aff1c1ef6acda19bcad2f57720f1dd3f8a0
diff --git a/patrole_tempest_plugin/rbac_exceptions.py b/patrole_tempest_plugin/rbac_exceptions.py
index 5ee65ae..e75b8ec 100644
--- a/patrole_tempest_plugin/rbac_exceptions.py
+++ b/patrole_tempest_plugin/rbac_exceptions.py
@@ -38,7 +38,7 @@
class RbacResourceSetupFailed(exceptions.TempestException):
- message = "Rbac resource setup failed"
+ message = "RBAC resource setup failed"
class RbacOverPermission(exceptions.TempestException):
diff --git a/patrole_tempest_plugin/rbac_rule_validation.py b/patrole_tempest_plugin/rbac_rule_validation.py
index d3213cf..69a43ea 100644
--- a/patrole_tempest_plugin/rbac_rule_validation.py
+++ b/patrole_tempest_plugin/rbac_rule_validation.py
@@ -17,6 +17,7 @@
import logging
import sys
+from oslo_log import versionutils
from oslo_utils import excutils
import six
@@ -36,7 +37,8 @@
RBACLOG = logging.getLogger('rbac_reporting')
-def action(service, rule='', expected_error_code=403, extra_target_data=None):
+def action(service, rule='', rules=None, expected_error_code=403,
+ extra_target_data=None):
"""A decorator for verifying OpenStack policy enforcement.
A decorator which allows for positive and negative RBAC testing. Given:
@@ -67,15 +69,18 @@
As such, negative and positive testing can be applied using this decorator.
- :param service: An OpenStack service. Examples: "nova" or "neutron".
- :param rule: A policy action defined in a policy.json file (or in
- code).
+ :param str service: An OpenStack service. Examples: "nova" or "neutron".
+ :param str rule: (DEPRECATED) A policy action defined in a policy.json file
+ or in code.
+ :param list rules: A list of policy actions defined in a policy.json file
+ or in code. The rules are logical-ANDed together to derive the expected
+ result.
.. note::
Patrole currently only supports custom JSON policy files.
- :param expected_error_code: Overrides default value of 403 (Forbidden)
+ :param int expected_error_code: Overrides default value of 403 (Forbidden)
with endpoint-specific error code. Currently only supports 403 and 404.
Support for 404 is needed because some services, like Neutron,
intentionally throw a 404 for security reasons.
@@ -85,11 +90,11 @@
A 404 should not be provided *unless* the endpoint masks a
``Forbidden`` exception as a ``NotFound`` exception.
- :param extra_target_data: Dictionary, keyed with ``oslo.policy`` generic
- check names, whose values are string literals that reference nested
- ``tempest.test.BaseTestCase`` attributes. Used by ``oslo.policy`` for
- performing matching against attributes that are sent along with the API
- calls. Example::
+ :param dict extra_target_data: Dictionary, keyed with ``oslo.policy``
+ generic check names, whose values are string literals that reference
+ nested ``tempest.test.BaseTestCase`` attributes. Used by
+ ``oslo.policy`` for performing matching against attributes that are
+ sent along with the API calls. Example::
extra_target_data={
"target.token.user_id":
@@ -113,6 +118,8 @@
if extra_target_data is None:
extra_target_data = {}
+ rules = _prepare_rules(rule, rules)
+
def decorator(test_func):
role = CONF.patrole.rbac_test_role
@@ -125,8 +132,14 @@
'`rbac_rule_validation` decorator can only be applied to '
'an instance of `tempest.test.BaseTestCase`.')
- allowed = _is_authorized(test_obj, service, rule,
- extra_target_data)
+ allowed = True
+ disallowed_rules = []
+ for rule in rules:
+ _allowed = _is_authorized(
+ test_obj, service, rule, extra_target_data)
+ if not _allowed:
+ disallowed_rules.append(rule)
+ allowed = allowed and _allowed
expected_exception, irregular_msg = _get_exception_type(
expected_error_code)
@@ -148,8 +161,12 @@
if irregular_msg:
LOG.warning(irregular_msg.format(rule, service))
if allowed:
- msg = ("Role %s was not allowed to perform %s." %
- (role, rule))
+ msg = ("Role %s was not allowed to perform the following "
+ "actions: %s. Expected allowed actions: %s. "
+ "Expected disallowed actions: %s." % (
+ role, sorted(rules),
+ sorted(set(rules) - set(disallowed_rules)),
+ sorted(disallowed_rules)))
LOG.error(msg)
raise exceptions.Forbidden(
"%s Exception was: %s" % (msg, e))
@@ -164,10 +181,14 @@
LOG.error(msg)
else:
if not allowed:
- LOG.error("Role %s was allowed to perform %s", role, rule)
- raise rbac_exceptions.RbacOverPermission(
- "OverPermission: Role %s was allowed to perform %s" %
- (role, rule))
+ msg = (
+ "OverPermission: Role %s was allowed to perform the "
+ "following disallowed actions: %s" % (
+ role, sorted(disallowed_rules)
+ )
+ )
+ LOG.error(msg)
+ raise rbac_exceptions.RbacOverPermission(msg)
finally:
if CONF.patrole_log.enable_reporting:
RBACLOG.info(
@@ -181,6 +202,23 @@
return decorator
+def _prepare_rules(rule, rules):
+ if rules is None:
+ rules = []
+ elif not isinstance(rules, (tuple, list)):
+ rules = [rules]
+ if rule:
+ deprecation_msg = (
+ "The `rule` argument has been deprecated in favor of `rules` "
+ "and will be removed in a future version.")
+ versionutils.report_deprecated_feature(LOG, deprecation_msg)
+ if rules:
+ LOG.debug("The `rules` argument will be used instead of `rule`.")
+ else:
+ rules.append(rule)
+ return rules
+
+
def _is_authorized(test_obj, service, rule, extra_target_data):
"""Validates whether current RBAC role has permission to do policy action.
diff --git a/patrole_tempest_plugin/tests/api/compute/test_server_misc_policy_actions_rbac.py b/patrole_tempest_plugin/tests/api/compute/test_server_misc_policy_actions_rbac.py
index d393a4c..8de74eb 100644
--- a/patrole_tempest_plugin/tests/api/compute/test_server_misc_policy_actions_rbac.py
+++ b/patrole_tempest_plugin/tests/api/compute/test_server_misc_policy_actions_rbac.py
@@ -395,7 +395,8 @@
@rbac_rule_validation.action(
service="nova",
- rule="os_compute_api:os-lock-server:unlock:unlock_override")
+ rules=["os_compute_api:os-lock-server:unlock",
+ "os_compute_api:os-lock-server:unlock:unlock_override"])
@decorators.idempotent_id('40dfeef9-73ee-48a9-be19-a219875de457')
def test_unlock_server_override(self):
"""Test force unlock server, part of os-lock-server.
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
index 85547e1..0a4c44b 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_rule_validation.py
@@ -13,6 +13,7 @@
# under the License.
import mock
+from oslo_config import cfg
from tempest.lib import exceptions
from tempest import manager
@@ -24,11 +25,13 @@
from patrole_tempest_plugin import rbac_utils
from patrole_tempest_plugin.tests.unit import fixtures
+CONF = cfg.CONF
-class RBACRuleValidationTest(base.TestCase):
+
+class BaseRBACRuleValidationTest(base.TestCase):
def setUp(self):
- super(RBACRuleValidationTest, self).setUp()
+ super(BaseRBACRuleValidationTest, self).setUp()
self.mock_test_args = mock.Mock(spec=test.BaseTestCase)
self.mock_test_args.os_primary = mock.Mock(spec=manager.Manager)
self.mock_test_args.rbac_utils = mock.Mock(
@@ -45,6 +48,12 @@
self.useFixture(
fixtures.ConfPatcher(enable_reporting=False, group='patrole_log'))
+
+class RBACRuleValidationTest(BaseRBACRuleValidationTest):
+ """Test suite for validating fundamental functionality for the
+ ``rbac_rule_validation`` decorator.
+ """
+
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
def test_rule_validation_have_permission_no_exc(self, mock_authority,
@@ -98,11 +107,11 @@
def test_policy(*args):
raise exceptions.Forbidden()
- test_re = "Role Member was not allowed to perform sentinel.action."
+ test_re = ("Role Member was not allowed to perform the following "
+ "actions: \[%s\].*" % (mock.sentinel.action))
self.assertRaisesRegex(exceptions.Forbidden, test_re, test_policy,
self.mock_test_args)
- mock_log.error.assert_called_once_with("Role Member was not allowed to"
- " perform sentinel.action.")
+ self.assertRegex(mock_log.error.mock_calls[0][1][0], test_re)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
@@ -138,11 +147,11 @@
def test_policy(*args):
raise rbac_exceptions.RbacMalformedResponse()
- test_re = "Role Member was not allowed to perform sentinel.action."
+ test_re = ("Role Member was not allowed to perform the following "
+ "actions: \[%s\].*" % (mock.sentinel.action))
self.assertRaisesRegex(exceptions.Forbidden, test_re, test_policy,
self.mock_test_args)
- mock_log.error.assert_called_once_with("Role Member was not allowed to"
- " perform sentinel.action.")
+ self.assertRegex(mock_log.error.mock_calls[0][1][0], test_re)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
@@ -179,11 +188,11 @@
def test_policy(*args):
raise rbac_exceptions.RbacConflictingPolicies()
- test_re = "Role Member was not allowed to perform sentinel.action."
+ test_re = ("Role Member was not allowed to perform the following "
+ "actions: \[%s\].*" % (mock.sentinel.action))
self.assertRaisesRegex(exceptions.Forbidden, test_re, test_policy,
self.mock_test_args)
- mock_log.error.assert_called_once_with("Role Member was not allowed to"
- " perform sentinel.action.")
+ self.assertRegex(mock_log.error.mock_calls[0][1][0], test_re)
@mock.patch.object(rbac_rv, 'LOG', autospec=True)
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
@@ -233,25 +242,26 @@
raise exceptions.NotFound()
expected_errors = [
- "Role Member was not allowed to perform sentinel.action.", None
+ ("Role Member was not allowed to perform the following "
+ "actions: \[%s\].*" % (mock.sentinel.action)),
+ None
]
for pos, allowed in enumerate([True, False]):
mock_authority.PolicyAuthority.return_value.allowed\
.return_value = allowed
- expected_error = expected_errors[pos]
+ error_re = expected_errors[pos]
- if expected_error:
- self.assertRaisesRegex(
- exceptions.Forbidden, '.* ' + expected_error, test_policy,
- self.mock_test_args)
- mock_log.error.assert_called_once_with(expected_error)
+ if error_re:
+ self.assertRaisesRegex(exceptions.Forbidden, error_re,
+ test_policy, self.mock_test_args)
+ self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
else:
test_policy(self.mock_test_args)
mock_log.error.assert_not_called()
- mock_log.warning.assert_called_once_with(
+ mock_log.warning.assert_called_with(
"NotFound exception was caught for policy action {0}. The "
"service {1} throws a 404 instead of a 403, which is "
"irregular.".format(mock.sentinel.action,
@@ -284,13 +294,10 @@
for test_policy in (
test_policy_expect_forbidden, test_policy_expect_not_found):
- error_re = (".* OverPermission: Role Member was allowed to perform"
- " sentinel.action")
+ error_re = ".*OverPermission: .* \[%s\]$" % mock.sentinel.action
self.assertRaisesRegex(rbac_exceptions.RbacOverPermission,
error_re, test_policy, self.mock_test_args)
- mock_log.error.assert_called_once_with(
- 'Role %s was allowed to perform %s', 'Member',
- mock.sentinel.action)
+ self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
mock_log.error.reset_mock()
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
@@ -405,3 +412,119 @@
mock.sentinel.action,
"Allowed",
"Allowed")
+
+
+class RBACRuleValidationTestMultiPolicy(BaseRBACRuleValidationTest):
+ """Test suite for validating multi-policy support for the
+ ``rbac_rule_validation`` decorator.
+ """
+
+ def _assert_policy_authority_called_with(self, rules, mock_authority):
+ m_authority = mock_authority.PolicyAuthority.return_value
+ m_authority.allowed.assert_has_calls([
+ mock.call(rule, CONF.patrole.rbac_test_role) for rule in rules
+ ])
+
+ @mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
+ def test_rule_validation_multi_policy_have_permission_success(
+ self, mock_authority):
+ """Test that when expected result is authorized and test passes that
+ the overall evaluation succeeds.
+ """
+ mock_authority.PolicyAuthority.return_value.allowed.\
+ return_value = True
+
+ rules = [mock.sentinel.action1, mock.sentinel.action2]
+
+ @rbac_rv.action(mock.sentinel.service, rules=rules)
+ def test_policy(*args):
+ pass
+
+ test_policy(self.mock_test_args)
+ self._assert_policy_authority_called_with(rules, mock_authority)
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ @mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
+ def test_rule_validation_multi_policy_overpermission_failure(
+ self, mock_authority, mock_log):
+ """Test that when expected result is unauthorized and test passes that
+ the overall evaluation results in an OverPermission getting raised.
+ """
+
+ rules = [
+ mock.sentinel.action1, mock.sentinel.action2, mock.sentinel.action3
+ ]
+
+ @rbac_rv.action(mock.sentinel.service, rules=rules)
+ def test_policy(*args):
+ pass
+
+ def _do_test(allowed_list, fail_on_action):
+ mock_authority.PolicyAuthority.return_value.allowed.side_effect = (
+ allowed_list)
+
+ error_re = ".*OverPermission: .* \[%s\]$" % fail_on_action
+ self.assertRaisesRegex(rbac_exceptions.RbacOverPermission,
+ error_re, test_policy, self.mock_test_args)
+ self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
+ mock_log.error.reset_mock()
+ self._assert_policy_authority_called_with(rules, mock_authority)
+
+ _do_test([True, True, False], mock.sentinel.action3)
+ _do_test([False, True, True], mock.sentinel.action1)
+ _do_test([True, False, True], mock.sentinel.action2)
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ @mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
+ def test_rule_validation_multi_policy_forbidden_success(
+ self, mock_authority, mock_log):
+ """Test that when the expected result is unauthorized and the test
+ fails that the overall evaluation results in success.
+ """
+
+ rules = [
+ mock.sentinel.action1, mock.sentinel.action2, mock.sentinel.action3
+ ]
+
+ @rbac_rv.action(mock.sentinel.service, rules=rules)
+ def test_policy(*args):
+ raise exceptions.Forbidden()
+
+ def _do_test(allowed_list):
+ mock_authority.PolicyAuthority.return_value.allowed.\
+ side_effect = allowed_list
+ test_policy(self.mock_test_args)
+ mock_log.error.assert_not_called()
+ self._assert_policy_authority_called_with(rules, mock_authority)
+
+ _do_test([True, True, False])
+ _do_test([False, True, True])
+ _do_test([True, False, True])
+
+ @mock.patch.object(rbac_rv, 'LOG', autospec=True)
+ @mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
+ def test_rule_validation_multi_policy_forbidden_failure(
+ self, mock_authority, mock_log):
+ """Test that when the expected result is authorized and the test
+ fails (with a Forbidden error code) that the overall evaluation
+ results a Forbidden getting raised.
+ """
+
+ # NOTE: Avoid mock.sentinel here due to weird sorting with them.
+ rules = ['action1', 'action2', 'action3']
+
+ @rbac_rv.action(mock.sentinel.service, rules=rules)
+ def test_policy(*args):
+ raise exceptions.Forbidden()
+
+ mock_authority.PolicyAuthority.return_value.allowed\
+ .return_value = True
+
+ error_re = ("Role Member was not allowed to perform the following "
+ "actions: %s. Expected allowed actions: %s. Expected "
+ "disallowed actions: []." % (rules, rules)).replace(
+ '[', '\[').replace(']', '\]')
+ self.assertRaisesRegex(exceptions.Forbidden, error_re, test_policy,
+ self.mock_test_args)
+ self.assertRegex(mock_log.error.mock_calls[0][1][0], error_re)
+ self._assert_policy_authority_called_with(rules, mock_authority)
diff --git a/releasenotes/notes/multi-policy-support-4e5c8b4e9e25ad9d.yaml b/releasenotes/notes/multi-policy-support-4e5c8b4e9e25ad9d.yaml
new file mode 100644
index 0000000..3d192d9
--- /dev/null
+++ b/releasenotes/notes/multi-policy-support-4e5c8b4e9e25ad9d.yaml
@@ -0,0 +1,13 @@
+---
+features:
+ - |
+ Patrole now offers support for multiple policies. The ``rules`` argument
+ has been added to the ``rbac_rule_validation.action`` decorator, which
+ takes a list of policy names which Patrole will use to determine the
+ expected test result. This allows Patrole to more accurately determine
+ whether RBAC is configured correctly, since some API endpoints enforce
+ multiple policies.
+deprecations:
+ - |
+ The ``rule`` argument in the ``rbac_rule_validation.action`` decorator has
+ been deprecated in favor of ``rules``.