Merge "Update rbac_rule_validation for multi-policy support"
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 63dee85..58a829d 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``.