openstack
/
patrole
Code Issues Proposed changes

Dynamic policy file discovery 

Patrole should eventually support other services like Heat
and Murano, not just the Big Tent services included in Tempest.

Patrole then should be able to dynamically discover custom
policy files. While the solution this commit implements is
not perfect, it will allow more services' policy file to
be discovered by Patrole. The policy files will still
have to be located on the same host as Patrole.

This commit removes the service-specific policy path
CONF options in favor of a new CONF option called
``[rbac] custom_policy_files`` which is a ListOpt that
includes paths for each custom policy file. Each
policy path assumes that the service name is included in
the path. The paths should be ordered by precedence, with
high-priority paths before low-priority paths. The first
path that is found to contain the service's policy file
will be used.

This commit refactors unit tests and rbac_policy_parser
as needed to work with the changes.

Change-Id: Ia929b77223b54906888af6cd324f0cfa0fafda8f
Implements blueprint: dynamic-policy-file-discovery
This commit is contained in:
Felipe Monteiro 2017-07-05 22:25:34 +01:00
parent 2bf66db706
commit 3ab2c357e8
4 changed files with 136 additions and 102 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

50
patrole_tempest_plugin/config.py
View File

@ -21,33 +21,61 @@ rbac_group = cfg.OptGroup(name='rbac',
RbacGroup = [
cfg.StrOpt('rbac_test_role',
default='admin',
help="The current RBAC role against which to run"
" Patrole tests."),
help="""The current RBAC role against which to run Patrole
tests."""),
cfg.BoolOpt('enable_rbac',
default=True,
help="Enables RBAC tests."),
cfg.BoolOpt('strict_policy_check',
default=False,
help="If true, throws RbacParsingException for"
" policies which don't exist. If false, "
"throws skipException."),
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
# other hosts. It may be possible to leverage the v3 identity policy API.
cfg.ListOpt('custom_policy_files',
default=['/etc/%s/policy.json'],
help="""List of the paths to search for policy files. Each
policy path assumes that the service name is included in the path once. Also
assumes Patrole is on the same host as the policy files. The paths should be
ordered by precedence, with high-priority paths before low-priority paths. The
first path that is found to contain the service's policy file will be used.
"""),
cfg.StrOpt('cinder_policy_file',
default='/etc/cinder/policy.json',
help="Location of the neutron policy file."),
help="""Location of the Cinder policy file. Assumed to be on
the same host as Patrole.""",
deprecated_for_removal=True,
deprecated_reason="It is better to use `custom_policy_files` "
"which supports any OpenStack service."),
cfg.StrOpt('glance_policy_file',
default='/etc/glance/policy.json',
help="Location of the glance policy file."),
help="""Location of the Glance policy file. Assumed to be on
the same host as Patrole.""",
deprecated_for_removal=True,
deprecated_reason="It is better to use `custom_policy_files` "
"which supports any OpenStack service."),
cfg.StrOpt('keystone_policy_file',
default='/etc/keystone/policy.json',
help="Location of the keystone policy file."),
help="""Location of the custom Keystone policy file. Assumed to
be on the same host as Patrole.""",
deprecated_for_removal=True,
deprecated_reason="It is better to use `custom_policy_files` "
"which supports any OpenStack service."),
cfg.StrOpt('neutron_policy_file',
default='/etc/neutron/policy.json',
help="Location of the neutron policy file."),
help="""Location of the Neutron policy file. Assumed to be on
the same host as Patrole.""",
deprecated_for_removal=True,
deprecated_reason="It is better to use `custom_policy_files` "
"which supports any OpenStack service."),
cfg.StrOpt('nova_policy_file',
default='/etc/nova/policy.json',
help="Location of the nova policy file."),
help="""Location of the custom Nova policy file. Assumed to be
on the same host as Patrole.""",
deprecated_for_removal=True,
deprecated_reason="It is better to use `custom_policy_files` "
"which supports any OpenStack service."),
cfg.BoolOpt('test_custom_requirements',
default=False,
help="""

53
patrole_tempest_plugin/rbac_policy_parser.py
View File

@ -58,26 +58,27 @@ class RbacPolicyParser(RbacAuthority):
the custom policy file over the default policy implementation is
prioritized.
:param project_id: type uuid
:param user_id: type uuid
:param service: type string
:param path: type string
:param uuid project_id: project_id of object performing API call
:param uuid user_id: user_id of object performing API call
:param string service: service of the policy file
:param dict extra_target_data: dictionary containing additional object
data needed by oslo.policy to validate generic checks
"""
if extra_target_data is None:
extra_target_data = {}
# First check if the service is valid.
self.validate_service(service)
# Use default path in /etc/<service_name/policy.json if no path
# is provided.
path = getattr(CONF.rbac, '%s_policy_file' % str(service), None)
if not path:
LOG.info("No config option found for %s,"
" using default path", str(service))
path = os.path.join('/etc', service, 'policy.json')
self.path = path
# Prioritize dynamically searching for policy files over relying on
# deprecated service-specific policy file locations.
if CONF.rbac.custom_policy_files:
self.discover_policy_files()
self.path = self.policy_files.get(service)
else:
self.path = getattr(CONF.rbac, '%s_policy_file' % str(service),
None)
self.rules = policy.Rules.load(self._get_policy_data(service),
'default')
self.project_id = project_id
@ -86,7 +87,7 @@ class RbacPolicyParser(RbacAuthority):
@classmethod
def validate_service(cls, service):
"""Validate whether the service passed to ``init`` exists."""
"""Validate whether the service passed to ``__init__`` exists."""
service = service.lower().strip() if service else None
# Cache the list of available services in memory to avoid needlessly
@ -102,6 +103,19 @@ class RbacPolicyParser(RbacAuthority):
raise rbac_exceptions.RbacInvalidService(
"%s is NOT a valid service." % service)
@classmethod
def discover_policy_files(cls):
# Dynamically discover the policy file for each service in
# ``cls.available_services``. Pick the first ``candidate_path`` found
# out of the potential paths in ``CONF.rbac.custom_policy_files``.
if not hasattr(cls, 'policy_files'):
cls.policy_files = {}
for service in cls.available_services:
for candidate_path in CONF.rbac.custom_policy_files:
if os.path.isfile(candidate_path % service):
cls.policy_files.setdefault(service,
candidate_path % service)
def allowed(self, rule_name, role):
is_admin_context = self._is_admin_context(role)
is_allowed = self._allowed(
@ -115,8 +129,8 @@ class RbacPolicyParser(RbacAuthority):
mgr_policy_data = {}
policy_data = {}
# Check whether policy file exists.
if os.path.isfile(self.path):
# Check whether policy file exists and attempt to read it.
if self.path and os.path.isfile(self.path):
try:
with open(self.path, 'r') as policy_file:
file_policy_data = policy_file.read()
@ -158,8 +172,11 @@ class RbacPolicyParser(RbacAuthority):
elif mgr_policy_data:
policy_data = mgr_policy_data
else:
error_message = 'Policy file for {0} service neither found in '\
'code nor at {1}.'.format(service, self.path)
error_message = (
'Policy file for {0} service neither found in code nor at {1}.'
.format(service, [loc % service for loc in
CONF.rbac.custom_policy_files])
)
raise rbac_exceptions.RbacParsingException(error_message)
try:

113
patrole_tempest_plugin/tests/unit/test_rbac_policy_parser.py
View File

@ -28,12 +28,21 @@ CONF = config.CONF
class RbacPolicyTest(base.TestCase):
services = {
'services': [
{'name': 'custom_rbac_policy'},
{'name': 'admin_rbac_policy'},
{'name': 'alt_admin_rbac_policy'},
{'name': 'tenant_rbac_policy'},
{'name': 'test_service'}
]
}
def setUp(self):
super(RbacPolicyTest, self).setUp()
self.mock_admin_mgr = mock.patch.object(
rbac_policy_parser, 'credentials').start()
self.mock_path = mock.patch.object(
rbac_policy_parser, 'os').start()
mock_admin_mgr = self.patchobject(rbac_policy_parser, 'credentials')
mock_admin_mgr.AdminManager().identity_services_v3_client.\
list_services.return_value = self.services
current_directory = os.path.dirname(os.path.realpath(__file__))
self.custom_policy_file = os.path.join(current_directory,
@ -48,35 +57,13 @@ class RbacPolicyTest(base.TestCase):
self.tenant_policy_file = os.path.join(current_directory,
'resources',
'tenant_rbac_policy.json')
services = {
'services': [
{'name': 'cinder', 'links': 'link', 'enabled': True,
'type': 'volume', 'id': 'id',
'description': 'description'},
{'name': 'glance', 'links': 'link', 'enabled': True,
'type': 'image', 'id': 'id',
'description': 'description'},
{'name': 'nova', 'links': 'link', 'enabled': True,
'type': 'compute', 'id': 'id',
'description': 'description'},
{'name': 'keystone', 'links': 'link', 'enabled': True,
'type': 'identity', 'id': 'id',
'description': 'description'},
{'name': 'heat', 'links': 'link', 'enabled': True,
'type': 'orchestration', 'id': 'id',
'description': 'description'},
{'name': 'neutron', 'links': 'link', 'enabled': True,
'type': 'networking', 'id': 'id',
'description': 'description'},
{'name': 'test_service', 'links': 'link', 'enabled': True,
'type': 'unit_test', 'id': 'id',
'description': 'description'}
]
}
self.mock_admin_mgr.AdminManager.return_value.\
identity_services_v3_client.list_services.return_value = \
services
CONF.set_override(
'custom_policy_files',
[os.path.join(current_directory, 'resources', '%s.json')],
group='rbac')
self.addCleanup(CONF.clear_override, 'custom_policy_files',
group='rbac')
def _get_fake_policy_rule(self, name, rule):
fake_rule = mock.Mock(check=rule)
@ -90,9 +77,8 @@ class RbacPolicyTest(base.TestCase):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.custom_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "custom_rbac_policy")
expected = {
'policy_action_1': ['two', 'four', 'six', 'eight'],
@ -113,9 +99,8 @@ class RbacPolicyTest(base.TestCase):
def test_admin_policy_file_with_admin_role(self):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.admin_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "admin_rbac_policy")
role = 'admin'
allowed_rules = [
@ -134,9 +119,8 @@ class RbacPolicyTest(base.TestCase):
def test_admin_policy_file_with_member_role(self):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.admin_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "admin_rbac_policy")
role = 'Member'
allowed_rules = [
@ -153,12 +137,11 @@ class RbacPolicyTest(base.TestCase):
allowed = parser.allowed(rule, role)
self.assertFalse(allowed)
def test_admin_policy_file_with_context_is_admin(self):
def test_alt_admin_policy_file_with_context_is_admin(self):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.alt_admin_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "alt_admin_rbac_policy")
role = 'fake_admin'
allowed_rules = ['non_admin_rule']
@ -193,9 +176,8 @@ class RbacPolicyTest(base.TestCase):
"""
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.tenant_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "tenant_rbac_policy")
# Check whether Member role can perform expected actions.
allowed_rules = ['rule1', 'rule2', 'rule3', 'rule4']
@ -254,7 +236,7 @@ class RbacPolicyTest(base.TestCase):
service)
m_log.debug.assert_called_once_with(
'%s is NOT a valid service.', 'invalid_service')
'%s is NOT a valid service.', service)
@mock.patch.object(rbac_policy_parser, 'LOG', autospec=True)
def test_service_is_none_raises_exception(self, m_log):
@ -274,9 +256,8 @@ class RbacPolicyTest(base.TestCase):
def test_invalid_policy_rule_throws_rbac_parsing_exception(self, m_log):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.custom_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "custom_rbac_policy")
fake_rule = 'fake_rule'
expected_message = "Policy action: {0} not found in policy file: {1}."\
@ -291,10 +272,9 @@ class RbacPolicyTest(base.TestCase):
def test_unknown_exception_throws_rbac_parsing_exception(self, m_log):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.custom_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "custom_rbac_policy")
parser.rules = mock.MagicMock(
**{'__getitem__.return_value.side_effect': Exception(
mock.sentinel.error)})
@ -327,12 +307,10 @@ class RbacPolicyTest(base.TestCase):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.tenant_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, "tenant_rbac_policy")
policy_data = parser._get_policy_data('fake_service')
self.assertIsInstance(policy_data, str)
actual_policy_data = json.loads(policy_data)
@ -346,7 +324,6 @@ class RbacPolicyTest(base.TestCase):
"rule4": "user_id:%(user_id)s",
"admin_tenant_rule": "role:admin and tenant_id:%(tenant_id)s",
"admin_user_rule": "role:admin and user_id:%(user_id)s"
}
self.assertEqual(expected_policy_data, actual_policy_data)
@ -371,12 +348,10 @@ class RbacPolicyTest(base.TestCase):
test_tenant_id = mock.sentinel.tenant_id
test_user_id = mock.sentinel.user_id
self.mock_path.path.join.return_value = self.tenant_policy_file
parser = rbac_policy_parser.RbacPolicyParser(
test_tenant_id, test_user_id, "test_service")
test_tenant_id, test_user_id, 'tenant_rbac_policy')
policy_data = parser._get_policy_data('fake_service')
self.assertIsInstance(policy_data, str)
actual_policy_data = json.loads(policy_data)
@ -392,23 +367,18 @@ class RbacPolicyTest(base.TestCase):
self.assertEqual(expected_policy_data, actual_policy_data)
@mock.patch.object(rbac_policy_parser, 'credentials', autospec=True)
@mock.patch.object(rbac_policy_parser, 'stevedore', autospec=True)
def test_get_policy_data_cannot_find_policy(self, mock_stevedore,
mock_creds):
def test_get_policy_data_cannot_find_policy(self, mock_stevedore):
mock_stevedore.named.NamedExtensionManager.return_value = None
mock_creds.AdminManager.return_value.identity_services_v3_client.\
list_services.return_value = {
'services': [{'name': 'test_service'}]}
self.mock_path.path.join.return_value = '/etc/test_service/policy.json'
e = self.assertRaises(rbac_exceptions.RbacParsingException,
rbac_policy_parser.RbacPolicyParser,
None, None, 'test_service')
expected_error = \
'Policy file for {0} service neither found in code '\
'nor at {1}.'.format('test_service',
'/etc/test_service/policy.json')
'nor at {1}.'.format(
'test_service',
[CONF.rbac.custom_policy_files[0] % 'test_service'])
self.assertIn(expected_error, str(e))
@ -416,8 +386,6 @@ class RbacPolicyTest(base.TestCase):
@mock.patch.object(rbac_policy_parser, 'stevedore', autospec=True)
def test_get_policy_data_without_valid_policy(self, mock_stevedore,
mock_json):
self.mock_path.path.isfile.return_value = False
test_policy_action = mock.Mock(check='rule:bar')
test_policy_action.configure_mock(name='foo')
@ -450,13 +418,12 @@ class RbacPolicyTest(base.TestCase):
mock_json):
mock_stevedore.named.NamedExtensionManager.return_value = None
mock_json.loads.side_effect = ValueError
self.mock_path.path.join.return_value = self.tenant_policy_file
e = self.assertRaises(rbac_exceptions.RbacParsingException,
rbac_policy_parser.RbacPolicyParser,
None, None, 'test_service')
expected_error = 'Policy file for {0} service neither found in code '\
'nor at {1}.'.format('test_service',
self.tenant_policy_file)
None, None, 'tenant_rbac_policy')
expected_error = (
'Policy file for {0} service neither found in code nor at {1}.'
.format('tenant_rbac_policy', [CONF.rbac.custom_policy_files[0]
% 'tenant_rbac_policy']))
self.assertIn(expected_error, str(e))

22
releasenotes/notes/dynamic-policy-file-discovery-104cbfc64b55d605.yaml Normal file
View File

@ -0,0 +1,22 @@
---
features:
- |
Add new configuration option ``[rbac] custom_policy_files``,
allowing users to specify list of the paths to search for custom
policy files. Each policy path assumes that the service name is
included in the path once. Also assumes Patrole is on the same host
as the policy files. The paths should be ordered by precedence, with
high-priority paths before low-priority paths. The first path that
is found to contain the service's policy file will be used.
deprecations:
- |
Deprecate the following configuration options from ``[rbac]`` group:
* cinder_policy_file
* glance_policy_file
* keystone_policy_file
* neutron_policy_file
* nova_policy_file
It is better to use ``[rbac] custom_policy_files`` which supports
any OpenStack service.