Remove already-deprecated strict_policy_check option
The option ``[patrole].strict_policy_check`` was deprecated during the last release cycle (Queens). This was because the option could be set to False in order to skip tests which might result in false positives. This PS, then, removes strict_policy_check references in the code, updates documentation, and adds a releasenote. Change-Id: I7f7eda39c0472bd3d70892c801fc4d14db0c0426
This commit is contained in:
Felipe Monteiro
parent
f1c6757160
commit
4ef7e5309c
|
|
@ -35,21 +35,6 @@ Enable RBAC
|
|||
Given the value of ``enable_rbac``, enables or disables Patrole tests. If
|
||||
``enable_rbac`` is ``False``, then Patrole tests are skipped.
|
||||
|
||||
Strict Policy Check
|
||||
-------------------
|
||||
|
||||
Currently, many services define their "default" rule to be "anyone allowed".
|
||||
If a policy action is not explicitly defined in a policy file, then
|
||||
``oslo.policy`` will fall back to the "default" rule. This implies that if
|
||||
there's a typo in a policy action specified in a Patrole test, ``oslo.policy``
|
||||
can report that the ``rbac_test_role`` will be able to perform the
|
||||
non-existent policy action. For a testing framework, this is undesirable
|
||||
behavior.
|
||||
|
||||
Hence, ``strict_policy_check``, if ``True``, will throw an error in the event
|
||||
that a non-existent or bogus policy action is passed to a Patrole test. If
|
||||
``False``, however, a ``self.skipException`` will be raised.
|
||||
|
||||
Custom Policy Files
|
||||
-------------------
|
||||
|
||||
|
|
@ -70,4 +55,3 @@ keyword.
|
|||
|
||||
Patrole currently does not support policy files located on a host different
|
||||
than the one on which it is running.
|
||||
..
|
||||
|
|
|
|||
|
|
@ -14,18 +14,6 @@
|
|||
# Enables RBAC tests. (boolean value)
|
||||
#enable_rbac = true
|
||||
|
||||
# DEPRECATED: If true, throws RbacParsingException for policies which
|
||||
# don't exist or are not included in the service's policy file. If
|
||||
# false, throws
|
||||
# skipException. (boolean value)
|
||||
# This option is deprecated for removal.
|
||||
# Its value may be silently ignored in the future.
|
||||
# Reason: This option allows for the possibility
|
||||
# of false positives. As a testing framework, Patrole should fail any
|
||||
# test that
|
||||
# passes in an invalid policy.
|
||||
#strict_policy_check = true
|
||||
|
||||
# List of the paths to search for policy files. Each
|
||||
# policy path assumes that the service name is included in the path
|
||||
# once. Also
|
||||
|
|
|
|||
|
|
@ -27,15 +27,6 @@ tests."""),
|
|||
cfg.BoolOpt('enable_rbac',
|
||||
default=True,
|
||||
help="Enables RBAC tests."),
|
||||
cfg.BoolOpt('strict_policy_check',
|
||||
default=True,
|
||||
deprecated_for_removal=True,
|
||||
deprecated_reason="""This option allows for the possibility
|
||||
of false positives. As a testing framework, Patrole should fail any test that
|
||||
passes in an invalid policy.""",
|
||||
help="""If true, throws RbacParsingException for policies which
|
||||
don't exist or are not included in the service's policy file. If false, throws
|
||||
skipException."""),
|
||||
# TODO(rb560u): There needs to be support for reading these JSON files from
|
||||
# other hosts. It may be possible to leverage the v3 identity policy API.
|
||||
cfg.ListOpt('custom_policy_files',
|
||||
|
|
|
|||
|
|
@ -158,6 +158,8 @@ class PolicyAuthority(RbacAuthority):
|
|||
|
||||
:param string rule_name: Rule to be checked using ``oslo.policy``.
|
||||
:param bool is_admin: Whether admin context is used.
|
||||
:raises RbacParsingException: If `rule_name`` does not exist in the
|
||||
cloud (in policy file or among registered in-code policy defaults).
|
||||
"""
|
||||
is_admin_context = self._is_admin_context(role)
|
||||
is_allowed = self._allowed(
|
||||
|
|
@ -215,9 +217,11 @@ class PolicyAuthority(RbacAuthority):
|
|||
policy_data = mgr_policy_data
|
||||
else:
|
||||
error_message = (
|
||||
'Policy file for {0} service neither found in code nor at {1}.'
|
||||
.format(service, [loc % service for loc in
|
||||
CONF.patrole.custom_policy_files])
|
||||
'Policy file for {0} service was not found among the '
|
||||
'registered in-code policies or in any of the possible policy '
|
||||
'files: {1}.'.format(service,
|
||||
[loc % service for loc in
|
||||
CONF.patrole.custom_policy_files])
|
||||
)
|
||||
raise rbac_exceptions.RbacParsingException(error_message)
|
||||
|
||||
|
|
|
|||
|
|
@ -163,8 +163,7 @@ def action(service, rule='', expected_error_code=403, extra_target_data=None):
|
|||
LOG.error(msg)
|
||||
else:
|
||||
if not allowed:
|
||||
LOG.error("Role %s was allowed to perform %s",
|
||||
role, rule)
|
||||
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))
|
||||
|
|
@ -200,10 +199,6 @@ def _is_authorized(test_obj, service, rule, extra_target_data):
|
|||
|
||||
:raises RbacResourceSetupFailed: If `project_id` or `user_id` are missing
|
||||
from the `auth_provider` attribute in `test_obj`.
|
||||
:raises RbacParsingException: if ``[patrole] strict_policy_check`` is True
|
||||
and the ``rule`` does not exist in the system.
|
||||
:raises skipException: If ``[patrole] strict_policy_check`` is False and
|
||||
the ``rule`` does not exist in the system.
|
||||
"""
|
||||
|
||||
try:
|
||||
|
|
@ -215,33 +210,27 @@ def _is_authorized(test_obj, service, rule, extra_target_data):
|
|||
LOG.error(msg)
|
||||
raise rbac_exceptions.RbacResourceSetupFailed(msg)
|
||||
|
||||
try:
|
||||
role = CONF.patrole.rbac_test_role
|
||||
# Test RBAC against custom requirements. Otherwise use oslo.policy.
|
||||
if CONF.patrole.test_custom_requirements:
|
||||
authority = requirements_authority.RequirementsAuthority(
|
||||
CONF.patrole.custom_requirements_file, service)
|
||||
else:
|
||||
formatted_target_data = _format_extra_target_data(
|
||||
test_obj, extra_target_data)
|
||||
authority = policy_authority.PolicyAuthority(
|
||||
project_id, user_id, service,
|
||||
extra_target_data=formatted_target_data)
|
||||
is_allowed = authority.allowed(rule, role)
|
||||
role = CONF.patrole.rbac_test_role
|
||||
# Test RBAC against custom requirements. Otherwise use oslo.policy.
|
||||
if CONF.patrole.test_custom_requirements:
|
||||
authority = requirements_authority.RequirementsAuthority(
|
||||
CONF.patrole.custom_requirements_file, service)
|
||||
else:
|
||||
formatted_target_data = _format_extra_target_data(
|
||||
test_obj, extra_target_data)
|
||||
authority = policy_authority.PolicyAuthority(
|
||||
project_id, user_id, service,
|
||||
extra_target_data=formatted_target_data)
|
||||
is_allowed = authority.allowed(rule, role)
|
||||
|
||||
if is_allowed:
|
||||
LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule,
|
||||
role)
|
||||
else:
|
||||
LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
|
||||
rule, role)
|
||||
return is_allowed
|
||||
except rbac_exceptions.RbacParsingException as e:
|
||||
if CONF.patrole.strict_policy_check:
|
||||
raise e
|
||||
else:
|
||||
raise testtools.TestCase.skipException(str(e))
|
||||
return False
|
||||
if is_allowed:
|
||||
LOG.debug("[Action]: %s, [Role]: %s is allowed!", rule,
|
||||
role)
|
||||
else:
|
||||
LOG.debug("[Action]: %s, [Role]: %s is NOT allowed!",
|
||||
rule, role)
|
||||
|
||||
return is_allowed
|
||||
|
||||
|
||||
def _get_exception_type(expected_error_code=403):
|
||||
|
|
|
|||
|
|
@ -387,12 +387,11 @@ class PolicyAuthorityTest(base.TestCase):
|
|||
policy_authority.PolicyAuthority,
|
||||
None, None, 'test_service')
|
||||
|
||||
expected_error = \
|
||||
'Policy file for {0} service neither found in code '\
|
||||
'nor at {1}.'.format(
|
||||
'test_service',
|
||||
[CONF.patrole.custom_policy_files[0] % 'test_service'])
|
||||
|
||||
expected_error = (
|
||||
'Policy file for {0} service was not found among the registered '
|
||||
'in-code policies or in any of the possible policy files: {1}.'
|
||||
.format('test_service',
|
||||
[CONF.patrole.custom_policy_files[0] % 'test_service']))
|
||||
self.assertIn(expected_error, str(e))
|
||||
|
||||
@mock.patch.object(policy_authority, 'json', autospec=True)
|
||||
|
|
@ -436,7 +435,8 @@ class PolicyAuthorityTest(base.TestCase):
|
|||
None, None, 'tenant_rbac_policy')
|
||||
|
||||
expected_error = (
|
||||
'Policy file for {0} service neither found in code nor at {1}.'
|
||||
'Policy file for {0} service was not found among the registered '
|
||||
'in-code policies or in any of the possible policy files: {1}.'
|
||||
.format('tenant_rbac_policy', [CONF.patrole.custom_policy_files[0]
|
||||
% 'tenant_rbac_policy']))
|
||||
self.assertIn(expected_error, str(e))
|
||||
|
|
@ -485,9 +485,10 @@ class PolicyAuthorityTest(base.TestCase):
|
|||
policy_parser.policy_files['test_service'])
|
||||
|
||||
def test_discover_policy_files_with_no_valid_files(self):
|
||||
expected_error = ("Policy file for test_service service neither found "
|
||||
"in code nor at %s." %
|
||||
[self.conf_policy_path % 'test_service'])
|
||||
expected_error = (
|
||||
'Policy file for {0} service was not found among the registered '
|
||||
'in-code policies or in any of the possible policy files: {1}.'
|
||||
.format('test_service', [self.conf_policy_path % 'test_service']))
|
||||
|
||||
e = self.assertRaises(rbac_exceptions.RbacParsingException,
|
||||
policy_authority.PolicyAuthority,
|
||||
|
|
|
|||
|
|
@ -13,7 +13,6 @@
|
|||
# under the License.
|
||||
|
||||
import mock
|
||||
import testtools
|
||||
|
||||
from tempest.lib import exceptions
|
||||
from tempest import manager
|
||||
|
|
@ -297,12 +296,8 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
|
||||
def test_invalid_policy_rule_raises_parsing_exception(
|
||||
self, mock_authority):
|
||||
"""Test that invalid policy action causes test to be fail with
|
||||
``[patrole] strict_policy_check`` set to True.
|
||||
"""Test that invalid policy action causes test to raise an exception.
|
||||
"""
|
||||
self.useFixture(
|
||||
fixtures.ConfPatcher(strict_policy_check=True, group='patrole'))
|
||||
|
||||
mock_authority.PolicyAuthority.return_value.allowed.\
|
||||
side_effect = rbac_exceptions.RbacParsingException
|
||||
|
||||
|
|
@ -318,30 +313,6 @@ class RBACRuleValidationTest(base.TestCase):
|
|||
mock.sentinel.project_id, mock.sentinel.user_id,
|
||||
mock.sentinel.service, extra_target_data={})
|
||||
|
||||
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
|
||||
def test_invalid_policy_rule_raises_skip_exception(
|
||||
self, mock_authority):
|
||||
"""Test that invalid policy action causes test to be skipped with
|
||||
``[patrole] strict_policy_check`` set to False.
|
||||
"""
|
||||
self.useFixture(
|
||||
fixtures.ConfPatcher(strict_policy_check=False, group='patrole'))
|
||||
|
||||
mock_authority.PolicyAuthority.return_value.allowed.side_effect = (
|
||||
rbac_exceptions.RbacParsingException)
|
||||
|
||||
@rbac_rv.action(mock.sentinel.service, mock.sentinel.action)
|
||||
def test_policy(*args):
|
||||
pass
|
||||
|
||||
error_re = 'Attempted to test an invalid policy file or action'
|
||||
self.assertRaisesRegex(testtools.TestCase.skipException, error_re,
|
||||
test_policy, self.mock_test_args)
|
||||
|
||||
mock_authority.PolicyAuthority.assert_called_once_with(
|
||||
mock.sentinel.project_id, mock.sentinel.user_id,
|
||||
mock.sentinel.service, extra_target_data={})
|
||||
|
||||
@mock.patch.object(rbac_rv, 'policy_authority', autospec=True)
|
||||
def test_get_exception_type_404(self, _):
|
||||
"""Test that getting a 404 exception type returns NotFound."""
|
||||
|
|
|
|||
|
|
@ -0,0 +1,6 @@
|
|||
---
|
||||
upgrade:
|
||||
- |
|
||||
The ``[patrole].strict_policy_check`` was deprecated during the Queens
|
||||
release cycle. It is removed in this release cycle because Patrole should
|
||||
always fail on invalid policies.
|
||||
Loading…
Reference in New Issue