From ec7f26a35270097210df7ff57e950d3e6b6fc3ec Mon Sep 17 00:00:00 2001 From: Roman Prykhodchenko Date: Thu, 2 Oct 2014 03:53:33 +0200 Subject: [PATCH] Refactoring for Ironic policy - Synchronised OSLO policy and its dependencies to the newest version. - Replace policy-related code in Ironic with a proper configuration of the common policy engine; related commit 07e9b32a95352c25a611a93d215878a8b9a36b71 commit b19af0806f0e2dffc83607d39a88e408928da72c commit 2324c77549f0affeda854ac3e7a500097450bb6a commit a51469326e84ed977ecc4e57fd3d46cdc21aa08f commit fde1e156a38633ce9018569145390bce2047fea8 commit e700d926f7d8fe2f57e53b93361aaf281bebc8ed commit 65e3d8c9773880094c0a4c164e046fae9cb7a5d9 commit 5d1f15a7785b2597eb9db5700ace9625bd2d44dd commit fcf517d72cb81f972fad20caa9ff0341e9b4aa9c commit e038d896174ada12c4d8b1ddafda2834d9ed0b14 Change-Id: I4ede79ec7e56a6a7c5ca3d69d3b4fb9d2f4ada22 Closes-Bug: #1288178 --- etc/ironic/ironic.conf.sample | 24 +- etc/ironic/policy.json | 7 +- ironic/api/app.py | 5 +- ironic/api/hooks.py | 39 +- ironic/common/context.py | 6 +- ironic/common/policy.py | 80 ++-- ironic/openstack/common/__init__.py | 17 - ironic/openstack/common/_i18n.py | 45 ++ ironic/openstack/common/fileutils.py | 4 +- ironic/openstack/common/log.py | 87 ++-- ironic/openstack/common/policy.py | 612 +++++++++++++++++---------- ironic/tests/fake_policy.py | 7 +- ironic/tests/policy_fixture.py | 11 +- ironic/tests/test_policy.py | 55 ++- 14 files changed, 629 insertions(+), 370 deletions(-) create mode 100644 ironic/openstack/common/_i18n.py diff --git a/etc/ironic/ironic.conf.sample b/etc/ironic/ironic.conf.sample index f2dce9cf9b..7bc145cd29 100644 --- a/etc/ironic/ironic.conf.sample +++ b/etc/ironic/ironic.conf.sample @@ -283,18 +283,6 @@ #state_path=$pybasedir -# -# Options defined in ironic.common.policy -# - -# JSON file representing policy. (string value) -#policy_file=policy.json - -# Rule checked when requested rule is not found. (string -# value) -#policy_default_rule=default - - # # Options defined in ironic.common.service # @@ -461,6 +449,18 @@ #run_external_periodic_tasks=true +# +# Options defined in ironic.openstack.common.policy +# + +# The JSON file that defines policies. (string value) +#policy_file=policy.json + +# Default rule. Enforced when a requested rule is not found. +# (string value) +#policy_default_rule=default + + [agent] # diff --git a/etc/ironic/policy.json b/etc/ironic/policy.json index 94ac3a5b80..cf42334d5d 100644 --- a/etc/ironic/policy.json +++ b/etc/ironic/policy.json @@ -1,5 +1,6 @@ { - "admin": "role:admin or role:administrator", - "admin_api": "is_admin:True", - "default": "rule:admin_api" + "admin_api": "role:admin or role:administrator", + "public_api": "is_public_api:True", + "trusted_call": "rule:admin_api or rule:public_api", + "default": "rule:trusted_call" } diff --git a/ironic/api/app.py b/ironic/api/app.py index 605e7c4a7a..0b692201cf 100644 --- a/ironic/api/app.py +++ b/ironic/api/app.py @@ -22,7 +22,6 @@ from ironic.api import acl from ironic.api import config from ironic.api import hooks from ironic.api import middleware -from ironic.common import policy auth_opts = [ cfg.StrOpt('auth_strategy', @@ -41,8 +40,6 @@ def get_pecan_config(): def setup_app(pecan_config=None, extra_hooks=None): - policy.init() - app_hooks = [hooks.ConfigHook(), hooks.DBHook(), hooks.ContextHook(pecan_config.app.acl_public_routes), @@ -55,7 +52,7 @@ def setup_app(pecan_config=None, extra_hooks=None): pecan_config = get_pecan_config() if pecan_config.app.enable_acl: - app_hooks.append(hooks.AdminAuthHook()) + app_hooks.append(hooks.TrustedCallHook()) pecan.configuration.set_config(dict(pecan_config), overwrite=True) diff --git a/ironic/api/hooks.py b/ironic/api/hooks.py index d706edf8db..cc0797ff87 100644 --- a/ironic/api/hooks.py +++ b/ironic/api/hooks.py @@ -21,9 +21,9 @@ from pecan import hooks from webob import exc from ironic.common import context +from ironic.common import policy from ironic.conductor import rpcapi from ironic.db import api as dbapi -from ironic.openstack.common import policy class ConfigHook(hooks.PecanHook): @@ -65,17 +65,20 @@ class ContextHook(hooks.PecanHook): super(ContextHook, self).__init__() def before(self, state): - user_id = state.request.headers.get('X-User-Id') - user_id = state.request.headers.get('X-User', user_id) - tenant = state.request.headers.get('X-Tenant-Id') - tenant = state.request.headers.get('X-Tenant', tenant) - domain_id = state.request.headers.get('X-User-Domain-Id') - domain_name = state.request.headers.get('X-User-Domain-Name') - auth_token = state.request.headers.get('X-Auth-Token') - creds = {'roles': state.request.headers.get('X-Roles', '').split(',')} + headers = state.request.headers + + user_id = headers.get('X-User-Id') + user_id = headers.get('X-User', user_id) + tenant = headers.get('X-Tenant-Id') + tenant = headers.get('X-Tenant', tenant) + domain_id = headers.get('X-User-Domain-Id') + domain_name = headers.get('X-User-Domain-Name') + auth_token = headers.get('X-Auth-Token') + roles = headers.get('X-Roles', '').split(',') is_public_api = state.request.environ.get('is_public_api', False) - is_admin = policy.check('admin', state.request.headers, creds) + creds = dict(headers) + is_admin = policy.enforce('admin_api', creds, creds) state.request.context = context.RequestContext( auth_token=auth_token, @@ -83,8 +86,9 @@ class ContextHook(hooks.PecanHook): tenant=tenant, domain_id=domain_id, domain_name=domain_name, + is_public_api=is_public_api, is_admin=is_admin, - is_public_api=is_public_api) + roles=roles) class RPCHook(hooks.PecanHook): @@ -94,19 +98,18 @@ class RPCHook(hooks.PecanHook): state.request.rpcapi = rpcapi.ConductorAPI() -class AdminAuthHook(hooks.PecanHook): +class TrustedCallHook(hooks.PecanHook): """Verify that the user has admin rights. - Checks whether the request context is an admin context and - rejects the request otherwise. + Checks whether the API call is performed against a public + resource or the user has admin privileges in the appropriate + tenant, domain or other administrative unit. """ def before(self, state): ctx = state.request.context - is_admin_api = policy.check('admin_api', {}, ctx.to_dict()) - - if not is_admin_api and not ctx.is_public_api: - raise exc.HTTPForbidden() + policy.enforce('trusted_call', ctx.to_dict(), ctx.to_dict(), + do_raise=True, exc=exc.HTTPForbidden) class NoExceptionTracebackHook(hooks.PecanHook): diff --git a/ironic/common/context.py b/ironic/common/context.py index 7cc58f0c41..52885e6290 100644 --- a/ironic/common/context.py +++ b/ironic/common/context.py @@ -20,18 +20,21 @@ class RequestContext(context.RequestContext): def __init__(self, auth_token=None, domain_id=None, domain_name=None, user=None, tenant=None, is_admin=False, is_public_api=False, - read_only=False, show_deleted=False, request_id=None): + read_only=False, show_deleted=False, request_id=None, + roles=None): """Stores several additional request parameters: :param domain_id: The ID of the domain. :param domain_name: The name of the domain. :param is_public_api: Specifies whether the request should be processed without authentication. + :param roles: List of user's roles if any. """ self.is_public_api = is_public_api self.domain_id = domain_id self.domain_name = domain_name + self.roles = roles or [] super(RequestContext, self).__init__(auth_token=auth_token, user=user, tenant=tenant, @@ -49,6 +52,7 @@ class RequestContext(context.RequestContext): 'show_deleted': self.show_deleted, 'request_id': self.request_id, 'domain_id': self.domain_id, + 'roles': self.roles, 'domain_name': self.domain_name, 'is_public_api': self.is_public_api} diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 88bc97b64d..381747aaab 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -15,53 +15,55 @@ """Policy Engine For Ironic.""" -import os.path - from oslo.config import cfg +from oslo_concurrency import lockutils -from ironic.common import exception -from ironic.common.i18n import _ -from ironic.common import utils from ironic.openstack.common import policy - -policy_opts = [ - cfg.StrOpt('policy_file', - default='policy.json', - help=_('JSON file representing policy.')), - cfg.StrOpt('policy_default_rule', - default='default', - help=_('Rule checked when requested rule is not found.')), - ] - +_ENFORCER = None CONF = cfg.CONF -CONF.register_opts(policy_opts) - -_POLICY_PATH = None -_POLICY_CACHE = {} -def reset(): - global _POLICY_PATH - global _POLICY_CACHE - _POLICY_PATH = None - _POLICY_CACHE = {} - policy.reset() +@lockutils.synchronized('policy_enforcer', 'ironic-') +def init_enforcer(policy_file=None, rules=None, + default_rule=None, use_conf=True): + """Synchronously initializes the policy enforcer + + :param policy_file: Custom policy file to use, if none is specified, + `CONF.policy_file` will be used. + :param rules: Default dictionary / Rules to use. It will be + considered just in the first instantiation. + :param default_rule: Default rule to use, CONF.default_rule will + be used if none is specified. + :param use_conf: Whether to load rules from config file. + + """ + global _ENFORCER + + if _ENFORCER: + return + + _ENFORCER = policy.Enforcer(policy_file=policy_file, + rules=rules, + default_rule=default_rule, + use_conf=use_conf) -def init(): - global _POLICY_PATH - global _POLICY_CACHE - if not _POLICY_PATH: - _POLICY_PATH = CONF.policy_file - if not os.path.exists(_POLICY_PATH): - _POLICY_PATH = CONF.find_file(_POLICY_PATH) - if not _POLICY_PATH: - raise exception.ConfigNotFound(path=CONF.policy_file) - utils.read_cached_file(_POLICY_PATH, _POLICY_CACHE, - reload_func=_set_rules) +def get_enforcer(): + """Provides access to the single instance of Policy enforcer.""" + + if not _ENFORCER: + init_enforcer() + + return _ENFORCER -def _set_rules(data): - default_rule = CONF.policy_default_rule - policy.set_rules(policy.Rules.load_json(data, default_rule)) +def enforce(rule, target, creds, do_raise=False, exc=None, *args, **kwargs): + """A shortcut for policy.Enforcer.enforce() + + Checks authorization of a rule against the target and credentials. + + """ + enforcer = get_enforcer() + return enforcer.enforce(rule, target, creds, do_raise=do_raise, + exc=exc, *args, **kwargs) diff --git a/ironic/openstack/common/__init__.py b/ironic/openstack/common/__init__.py index d1223eaf76..e69de29bb2 100644 --- a/ironic/openstack/common/__init__.py +++ b/ironic/openstack/common/__init__.py @@ -1,17 +0,0 @@ -# -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# 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 six - - -six.add_move(six.MovedModule('mox', 'mox', 'mox3.mox')) diff --git a/ironic/openstack/common/_i18n.py b/ironic/openstack/common/_i18n.py new file mode 100644 index 0000000000..21dcf4c89b --- /dev/null +++ b/ironic/openstack/common/_i18n.py @@ -0,0 +1,45 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""oslo.i18n integration module. + +See http://docs.openstack.org/developer/oslo.i18n/usage.html + +""" + +try: + import oslo.i18n + + # NOTE(dhellmann): This reference to o-s-l-o will be replaced by the + # application name when this module is synced into the separate + # repository. It is OK to have more than one translation function + # using the same domain, since there will still only be one message + # catalog. + _translators = oslo.i18n.TranslatorFactory(domain='ironic') + + # The primary translation function using the well-known name "_" + _ = _translators.primary + + # Translators for log levels. + # + # The abbreviated names are meant to reflect the usual use of a short + # name like '_'. The "L" is for "log" and the other letter comes from + # the level. + _LI = _translators.log_info + _LW = _translators.log_warning + _LE = _translators.log_error + _LC = _translators.log_critical +except ImportError: + # NOTE(dims): Support for cases where a project wants to use + # code from oslo-incubator, but is not ready to be internationalized + # (like tempest) + _ = _LI = _LW = _LE = _LC = lambda x: x diff --git a/ironic/openstack/common/fileutils.py b/ironic/openstack/common/fileutils.py index 89fd3ffba5..ec26eaf9cd 100644 --- a/ironic/openstack/common/fileutils.py +++ b/ironic/openstack/common/fileutils.py @@ -15,11 +15,11 @@ import contextlib import errno +import logging import os import tempfile -from ironic.openstack.common import excutils -from ironic.openstack.common import log as logging +from oslo.utils import excutils LOG = logging.getLogger(__name__) diff --git a/ironic/openstack/common/log.py b/ironic/openstack/common/log.py index 5345db0fcf..782e4d2327 100644 --- a/ironic/openstack/common/log.py +++ b/ironic/openstack/common/log.py @@ -27,28 +27,27 @@ It also allows setting of formatting information through conf. """ +import copy import inspect import itertools import logging import logging.config import logging.handlers import os +import socket import sys import traceback from oslo.config import cfg +from oslo.serialization import jsonutils +from oslo.utils import importutils import six from six import moves _PY26 = sys.version_info[0:2] == (2, 6) -from ironic.openstack.common.gettextutils import _ -from ironic.openstack.common import importutils -from ironic.openstack.common import jsonutils +from ironic.openstack.common._i18n import _ from ironic.openstack.common import local -# NOTE(flaper87): Pls, remove when graduating this module -# from the incubator. -from ironic.openstack.common.strutils import mask_password # noqa _DEFAULT_LOG_DATE_FORMAT = "%Y-%m-%d %H:%M:%S" @@ -126,7 +125,9 @@ DEFAULT_LOG_LEVELS = ['amqp=WARN', 'amqplib=WARN', 'boto=WARN', 'qpid=WARN', 'sqlalchemy=WARN', 'suds=INFO', 'oslo.messaging=INFO', 'iso8601=WARN', 'requests.packages.urllib3.connectionpool=WARN', - 'urllib3.connectionpool=WARN', 'websocket=WARN'] + 'urllib3.connectionpool=WARN', 'websocket=WARN', + "keystonemiddleware=WARN", "routes.middleware=WARN", + "stevedore=WARN"] log_opts = [ cfg.StrOpt('logging_context_format_string', @@ -174,6 +175,16 @@ CONF.register_cli_opts(logging_cli_opts) CONF.register_opts(generic_log_opts) CONF.register_opts(log_opts) + +def list_opts(): + """Entry point for oslo.config-generator.""" + return [(None, copy.deepcopy(common_cli_opts)), + (None, copy.deepcopy(logging_cli_opts)), + (None, copy.deepcopy(generic_log_opts)), + (None, copy.deepcopy(log_opts)), + ] + + # our new audit level # NOTE(jkoelker) Since we synthesized an audit level, make the logging # module aware of it so it acts like other levels. @@ -300,11 +311,10 @@ class ContextAdapter(BaseLoggerAdapter): self.warn(stdmsg, *args, **kwargs) def process(self, msg, kwargs): - # NOTE(mrodden): catch any Message/other object and - # coerce to unicode before they can get - # to the python logging and possibly - # cause string encoding trouble - if not isinstance(msg, six.string_types): + # NOTE(jecarey): If msg is not unicode, coerce it into unicode + # before it can get to the python logging and + # possibly cause string encoding trouble + if not isinstance(msg, six.text_type): msg = six.text_type(msg) if 'extra' not in kwargs: @@ -429,12 +439,12 @@ def set_defaults(logging_context_format_string=None, # later in a backwards in-compatible change if default_log_levels is not None: cfg.set_defaults( - log_opts, - default_log_levels=default_log_levels) + log_opts, + default_log_levels=default_log_levels) if logging_context_format_string is not None: cfg.set_defaults( - log_opts, - logging_context_format_string=logging_context_format_string) + log_opts, + logging_context_format_string=logging_context_format_string) def _find_facility_from_conf(): @@ -483,18 +493,6 @@ def _setup_logging_from_conf(project, version): for handler in log_root.handlers: log_root.removeHandler(handler) - if CONF.use_syslog: - facility = _find_facility_from_conf() - # TODO(bogdando) use the format provided by RFCSysLogHandler - # after existing syslog format deprecation in J - if CONF.use_syslog_rfc_format: - syslog = RFCSysLogHandler(address='/dev/log', - facility=facility) - else: - syslog = logging.handlers.SysLogHandler(address='/dev/log', - facility=facility) - log_root.addHandler(syslog) - logpath = _get_log_file_path() if logpath: filelog = logging.handlers.WatchedFileHandler(logpath) @@ -511,14 +509,9 @@ def _setup_logging_from_conf(project, version): log_root.addHandler(streamlog) if CONF.publish_errors: - try: - handler = importutils.import_object( - "ironic.openstack.common.log_handler.PublishErrorsHandler", - logging.ERROR) - except ImportError: - handler = importutils.import_object( - "oslo.messaging.notify.log_handler.PublishErrorsHandler", - logging.ERROR) + handler = importutils.import_object( + "oslo.messaging.notify.log_handler.PublishErrorsHandler", + logging.ERROR) log_root.addHandler(handler) datefmt = CONF.log_date_format @@ -553,6 +546,22 @@ def _setup_logging_from_conf(project, version): else: logger.setLevel(level_name) + if CONF.use_syslog: + try: + facility = _find_facility_from_conf() + # TODO(bogdando) use the format provided by RFCSysLogHandler + # after existing syslog format deprecation in J + if CONF.use_syslog_rfc_format: + syslog = RFCSysLogHandler(address='/dev/log', + facility=facility) + else: + syslog = logging.handlers.SysLogHandler(address='/dev/log', + facility=facility) + log_root.addHandler(syslog) + except socket.error: + log_root.error('Unable to add syslog handler. Verify that syslog ' + 'is running.') + _loggers = {} @@ -622,6 +631,12 @@ class ContextFormatter(logging.Formatter): def format(self, record): """Uses contextstring if request_id is set, otherwise default.""" + # NOTE(jecarey): If msg is not unicode, coerce it into unicode + # before it can get to the python logging and + # possibly cause string encoding trouble + if not isinstance(record.msg, six.text_type): + record.msg = six.text_type(record.msg) + # store project info record.project = self.project record.version = self.version diff --git a/ironic/openstack/common/policy.py b/ironic/openstack/common/policy.py index 8463600c4c..1e93e87639 100644 --- a/ironic/openstack/common/policy.py +++ b/ironic/openstack/common/policy.py @@ -1,5 +1,5 @@ -# vim: tabstop=4 shiftwidth=4 softtabstop=4 - +# -*- coding: utf-8 -*- +# # Copyright (c) 2012 OpenStack Foundation. # All Rights Reserved. # @@ -24,22 +24,43 @@ string written in the new policy language. In the list-of-lists representation, each check inside the innermost list is combined as with an "and" conjunction--for that check to pass, all the specified checks must pass. These innermost lists are then -combined as with an "or" conjunction. This is the original way of -expressing policies, but there now exists a new way: the policy -language. - -In the policy language, each check is specified the same way as in the -list-of-lists representation: a simple "a:b" pair that is matched to -the correct code to perform that check. However, conjunction -operators are available, allowing for more expressiveness in crafting -policies. - -As an example, take the following rule, expressed in the list-of-lists -representation:: +combined as with an "or" conjunction. As an example, take the following +rule, expressed in the list-of-lists representation:: [["role:admin"], ["project_id:%(project_id)s", "role:projectadmin"]] -In the policy language, this becomes:: +This is the original way of expressing policies, but there now exists a +new way: the policy language. + +In the policy language, each check is specified the same way as in the +list-of-lists representation: a simple "a:b" pair that is matched to +the correct class to perform that check:: + + +===========================================================================+ + | TYPE | SYNTAX | + +===========================================================================+ + |User's Role | role:admin | + +---------------------------------------------------------------------------+ + |Rules already defined on policy | rule:admin_required | + +---------------------------------------------------------------------------+ + |Against URL's¹ | http://my-url.org/check | + +---------------------------------------------------------------------------+ + |User attributes² | project_id:%(target.project.id)s | + +---------------------------------------------------------------------------+ + |Strings | :'xpto2035abc' | + | | 'myproject': | + +---------------------------------------------------------------------------+ + | | project_id:xpto2035abc | + |Literals | domain_id:20 | + | | True:%(user.enabled)s | + +===========================================================================+ + +¹URL checking must return 'True' to be valid +²User attributes (obtained through the token): user_id, domain_id or project_id + +Conjunction operators are available, allowing for more expressiveness +in crafting policies. So, in the policy language, the previous check in +list-of-lists becomes:: role:admin or (project_id:%(project_id)s and role:projectadmin) @@ -48,6 +69,17 @@ policy rule:: project_id:%(project_id)s and not role:dunce +Attributes sent along with API calls can be used by the policy engine +(on the right side of the expression), by using the following syntax:: + + :%(user.id)s + +Contextual attributes of objects identified by their IDs are loaded +from the database. They are also available to the policy engine and +can be checked through the `target` keyword:: + + :%(target.role.name)s + Finally, two special policy checks should be mentioned; the policy check "@" will always accept an access, and the policy check "!" will always reject an access. (Note that if a rule is either the empty @@ -57,34 +89,66 @@ as it allows particular rules to be explicitly disabled. """ import abc +import ast +import copy +import os import re -import urllib +from oslo.config import cfg +from oslo.serialization import jsonutils import six -import urllib2 +import six.moves.urllib.parse as urlparse +import six.moves.urllib.request as urlrequest -from ironic.openstack.common.gettextutils import _ -from ironic.openstack.common import jsonutils +from ironic.openstack.common import fileutils +from ironic.openstack.common._i18n import _, _LE, _LI from ironic.openstack.common import log as logging +policy_opts = [ + cfg.StrOpt('policy_file', + default='policy.json', + help=_('The JSON file that defines policies.')), + cfg.StrOpt('policy_default_rule', + default='default', + help=_('Default rule. Enforced when a requested rule is not ' + 'found.')), + cfg.MultiStrOpt('policy_dirs', + default=['policy.d'], + help=_('Directories where policy configuration files are ' + 'stored. They can be relative to any directory ' + 'in the search path defined by the config_dir ' + 'option, or absolute paths. The file defined by ' + 'policy_file must exist for these directories to ' + 'be searched.')), +] + +CONF = cfg.CONF +CONF.register_opts(policy_opts) + LOG = logging.getLogger(__name__) - -_rules = None _checks = {} +def list_opts(): + """Entry point for oslo.config-generator.""" + return [(None, copy.deepcopy(policy_opts))] + + +class PolicyNotAuthorized(Exception): + + def __init__(self, rule): + msg = _("Policy doesn't allow %s to be performed.") % rule + super(PolicyNotAuthorized, self).__init__(msg) + + class Rules(dict): - """ - A store for rules. Handles the default_rule setting directly. - """ + """A store for rules. Handles the default_rule setting directly.""" @classmethod def load_json(cls, data, default_rule=None): - """ - Allow loading of JSON rule data. - """ + """Allow loading of JSON rule data.""" # Suck in the JSON data and parse the rules rules = dict((k, parse_rule(v)) for k, v in @@ -101,12 +165,23 @@ class Rules(dict): def __missing__(self, key): """Implements the default rule handling.""" - # If the default rule isn't actually defined, do something - # reasonably intelligent - if not self.default_rule or self.default_rule not in self: + if isinstance(self.default_rule, dict): raise KeyError(key) - return self[self.default_rule] + # If the default rule isn't actually defined, do something + # reasonably intelligent + if not self.default_rule: + raise KeyError(key) + + if isinstance(self.default_rule, BaseCheck): + return self.default_rule + + # We need to check this or we can get infinite recursion + if self.default_rule not in self: + raise KeyError(key) + + elif isinstance(self.default_rule, six.string_types): + return self[self.default_rule] def __str__(self): """Dumps a string representation of the rules.""" @@ -124,87 +199,188 @@ class Rules(dict): return jsonutils.dumps(out_rules, indent=4) -# Really have to figure out a way to deprecate this -def set_rules(rules): - """Set the rules in use for policy checks.""" +class Enforcer(object): + """Responsible for loading and enforcing rules. - global _rules - - _rules = rules - - -# Ditto -def reset(): - """Clear the rules used for policy checks.""" - - global _rules - - _rules = None - - -def check(rule, target, creds, exc=None, *args, **kwargs): - """ - Checks authorization of a rule against the target and credentials. - - :param rule: The rule to evaluate. - :param target: As much information about the object being operated - on as possible, as a dictionary. - :param creds: As much information about the user performing the - action as possible, as a dictionary. - :param exc: Class of the exception to raise if the check fails. - Any remaining arguments passed to check() (both - positional and keyword arguments) will be passed to - the exception class. If exc is not provided, returns - False. - - :return: Returns False if the policy does not allow the action and - exc is not provided; otherwise, returns a value that - evaluates to True. Note: for rules using the "case" - expression, this True value will be the specified string - from the expression. + :param policy_file: Custom policy file to use, if none is + specified, `CONF.policy_file` will be + used. + :param rules: Default dictionary / Rules to use. It will be + considered just in the first instantiation. If + `load_rules(True)`, `clear()` or `set_rules(True)` + is called this will be overwritten. + :param default_rule: Default rule to use, CONF.default_rule will + be used if none is specified. + :param use_conf: Whether to load rules from cache or config file. + :param overwrite: Whether to overwrite existing rules when reload rules + from config file. """ - # Allow the rule to be a Check tree - if isinstance(rule, BaseCheck): - result = rule(target, creds) - elif not _rules: - # No rules to reference means we're going to fail closed - result = False - else: - try: - # Evaluate the rule - result = _rules[rule](target, creds) - except KeyError: - # If the rule doesn't exist, fail closed + def __init__(self, policy_file=None, rules=None, + default_rule=None, use_conf=True, overwrite=True): + self.default_rule = default_rule or CONF.policy_default_rule + self.rules = Rules(rules, self.default_rule) + + self.policy_path = None + self.policy_file = policy_file or CONF.policy_file + self.use_conf = use_conf + self.overwrite = overwrite + + def set_rules(self, rules, overwrite=True, use_conf=False): + """Create a new Rules object based on the provided dict of rules. + + :param rules: New rules to use. It should be an instance of dict. + :param overwrite: Whether to overwrite current rules or update them + with the new rules. + :param use_conf: Whether to reload rules from cache or config file. + """ + + if not isinstance(rules, dict): + raise TypeError(_("Rules must be an instance of dict or Rules, " + "got %s instead") % type(rules)) + self.use_conf = use_conf + if overwrite: + self.rules = Rules(rules, self.default_rule) + else: + self.rules.update(rules) + + def clear(self): + """Clears Enforcer rules, policy's cache and policy's path.""" + self.set_rules({}) + fileutils.delete_cached_file(self.policy_path) + self.default_rule = None + self.policy_path = None + + def load_rules(self, force_reload=False): + """Loads policy_path's rules. + + Policy file is cached and will be reloaded if modified. + + :param force_reload: Whether to reload rules from config file. + """ + + if force_reload: + self.use_conf = force_reload + + if self.use_conf: + if not self.policy_path: + self.policy_path = self._get_policy_path(self.policy_file) + + self._load_policy_file(self.policy_path, force_reload, + overwrite=self.overwrite) + for path in CONF.policy_dirs: + try: + path = self._get_policy_path(path) + except cfg.ConfigFilesNotFoundError: + LOG.info(_LI("Can not find policy directory: %s"), path) + continue + self._walk_through_policy_directory(path, + self._load_policy_file, + force_reload, False) + + @staticmethod + def _walk_through_policy_directory(path, func, *args): + # We do not iterate over sub-directories. + policy_files = next(os.walk(path))[2] + policy_files.sort() + for policy_file in [p for p in policy_files if not p.startswith('.')]: + func(os.path.join(path, policy_file), *args) + + def _load_policy_file(self, path, force_reload, overwrite=True): + reloaded, data = fileutils.read_cached_file( + path, force_reload=force_reload) + if reloaded or not self.rules or not overwrite: + rules = Rules.load_json(data, self.default_rule) + self.set_rules(rules, overwrite=overwrite, use_conf=True) + LOG.debug("Rules successfully reloaded") + + def _get_policy_path(self, path): + """Locate the policy json data file/path. + + :param path: It's value can be a full path or related path. When + full path specified, this function just returns the full + path. When related path specified, this function will + search configuration directories to find one that exists. + + :returns: The policy path + + :raises: ConfigFilesNotFoundError if the file/path couldn't + be located. + """ + policy_path = CONF.find_file(path) + + if policy_path: + return policy_path + + raise cfg.ConfigFilesNotFoundError((path,)) + + def enforce(self, rule, target, creds, do_raise=False, + exc=None, *args, **kwargs): + """Checks authorization of a rule against the target and credentials. + + :param rule: A string or BaseCheck instance specifying the rule + to evaluate. + :param target: As much information about the object being operated + on as possible, as a dictionary. + :param creds: As much information about the user performing the + action as possible, as a dictionary. + :param do_raise: Whether to raise an exception or not if check + fails. + :param exc: Class of the exception to raise if the check fails. + Any remaining arguments passed to enforce() (both + positional and keyword arguments) will be passed to + the exception class. If not specified, PolicyNotAuthorized + will be used. + + :return: Returns False if the policy does not allow the action and + exc is not provided; otherwise, returns a value that + evaluates to True. Note: for rules using the "case" + expression, this True value will be the specified string + from the expression. + """ + + self.load_rules() + + # Allow the rule to be a Check tree + if isinstance(rule, BaseCheck): + result = rule(target, creds, self) + elif not self.rules: + # No rules to reference means we're going to fail closed result = False + else: + try: + # Evaluate the rule + result = self.rules[rule](target, creds, self) + except KeyError: + LOG.debug("Rule [%s] doesn't exist" % rule) + # If the rule doesn't exist, fail closed + result = False - # If it is False, raise the exception if requested - if exc and result is False: - raise exc(*args, **kwargs) + # If it is False, raise the exception if requested + if do_raise and not result: + if exc: + raise exc(*args, **kwargs) - return result + raise PolicyNotAuthorized(rule) + + return result +@six.add_metaclass(abc.ABCMeta) class BaseCheck(object): - """ - Abstract base class for Check classes. - """ - - __metaclass__ = abc.ABCMeta + """Abstract base class for Check classes.""" @abc.abstractmethod def __str__(self): - """ - Retrieve a string representation of the Check tree rooted at - this node. - """ + """String representation of the Check tree rooted at this node.""" pass @abc.abstractmethod - def __call__(self, target, cred): - """ - Perform the check. Returns False to reject the access or a + def __call__(self, target, cred, enforcer): + """Triggers if instance of the class is called. + + Performs the check. Returns False to reject the access or a true value (not necessary True) to accept the access. """ @@ -212,44 +388,39 @@ class BaseCheck(object): class FalseCheck(BaseCheck): - """ - A policy check that always returns False (disallow). - """ + """A policy check that always returns False (disallow).""" def __str__(self): """Return a string representation of this check.""" return "!" - def __call__(self, target, cred): + def __call__(self, target, cred, enforcer): """Check the policy.""" return False class TrueCheck(BaseCheck): - """ - A policy check that always returns True (allow). - """ + """A policy check that always returns True (allow).""" def __str__(self): """Return a string representation of this check.""" return "@" - def __call__(self, target, cred): + def __call__(self, target, cred, enforcer): """Check the policy.""" return True class Check(BaseCheck): - """ - A base class to allow for user-defined policy checks. - """ + """A base class to allow for user-defined policy checks.""" def __init__(self, kind, match): - """ + """Initiates Check instance. + :param kind: The kind of the check, i.e., the field before the ':'. :param match: The match of the check, i.e., the field after @@ -266,14 +437,13 @@ class Check(BaseCheck): class NotCheck(BaseCheck): - """ + """Implements the "not" logical operator. + A policy check that inverts the result of another policy check. - Implements the "not" operator. """ def __init__(self, rule): - """ - Initialize the 'not' check. + """Initialize the 'not' check. :param rule: The rule to negate. Must be a Check. """ @@ -285,24 +455,23 @@ class NotCheck(BaseCheck): return "not %s" % self.rule - def __call__(self, target, cred): - """ - Check the policy. Returns the logical inverse of the wrapped - check. + def __call__(self, target, cred, enforcer): + """Check the policy. + + Returns the logical inverse of the wrapped check. """ - return not self.rule(target, cred) + return not self.rule(target, cred, enforcer) class AndCheck(BaseCheck): - """ - A policy check that requires that a list of other checks all - return True. Implements the "and" operator. + """Implements the "and" logical operator. + + A policy check that requires that a list of other checks all return True. """ def __init__(self, rules): - """ - Initialize the 'and' check. + """Initialize the 'and' check. :param rules: A list of rules that will be tested. """ @@ -314,20 +483,21 @@ class AndCheck(BaseCheck): return "(%s)" % ' and '.join(str(r) for r in self.rules) - def __call__(self, target, cred): - """ - Check the policy. Requires that all rules accept in order to - return True. + def __call__(self, target, cred, enforcer): + """Check the policy. + + Requires that all rules accept in order to return True. """ for rule in self.rules: - if not rule(target, cred): + if not rule(target, cred, enforcer): return False return True def add_check(self, rule): - """ + """Adds rule to be tested. + Allows addition of another rule to the list of rules that will be tested. Returns the AndCheck object for convenience. """ @@ -337,14 +507,14 @@ class AndCheck(BaseCheck): class OrCheck(BaseCheck): - """ + """Implements the "or" operator. + A policy check that requires that at least one of a list of other - checks returns True. Implements the "or" operator. + checks returns True. """ def __init__(self, rules): - """ - Initialize the 'or' check. + """Initialize the 'or' check. :param rules: A list of rules that will be tested. """ @@ -356,20 +526,20 @@ class OrCheck(BaseCheck): return "(%s)" % ' or '.join(str(r) for r in self.rules) - def __call__(self, target, cred): - """ - Check the policy. Requires that at least one rule accept in - order to return True. + def __call__(self, target, cred, enforcer): + """Check the policy. + + Requires that at least one rule accept in order to return True. """ for rule in self.rules: - if rule(target, cred): + if rule(target, cred, enforcer): return True - return False def add_check(self, rule): - """ + """Adds rule to be tested. + Allows addition of another rule to the list of rules that will be tested. Returns the OrCheck object for convenience. """ @@ -379,9 +549,7 @@ class OrCheck(BaseCheck): def _parse_check(rule): - """ - Parse a single base check rule into an appropriate Check object. - """ + """Parse a single base check rule into an appropriate Check object.""" # Handle the special checks if rule == '!': @@ -392,7 +560,7 @@ def _parse_check(rule): try: kind, match = rule.split(':', 1) except Exception: - LOG.exception(_("Failed to understand rule %(rule)s") % locals()) + LOG.exception(_LE("Failed to understand rule %s") % rule) # If the rule is invalid, we'll fail closed return FalseCheck() @@ -402,14 +570,14 @@ def _parse_check(rule): elif None in _checks: return _checks[None](kind, match) else: - LOG.error(_("No handler for matches of kind %s") % kind) + LOG.error(_LE("No handler for matches of kind %s") % kind) return FalseCheck() def _parse_list_rule(rule): - """ - Provided for backwards compatibility. Translates the old - list-of-lists syntax into a tree of Check objects. + """Translates the old list-of-lists syntax into a tree of Check objects. + + Provided for backwards compatibility. """ # Empty rule defaults to True @@ -424,7 +592,7 @@ def _parse_list_rule(rule): continue # Handle bare strings - if isinstance(inner_rule, basestring): + if isinstance(inner_rule, six.string_types): inner_rule = [inner_rule] # Parse the inner rules into Check objects @@ -450,8 +618,7 @@ _tokenize_re = re.compile(r'\s+') def _parse_tokenize(rule): - """ - Tokenizer for the policy language. + """Tokenizer for the policy language. Most of the single-character tokens are specified in the _tokenize_re; however, parentheses need to be handled specially, @@ -500,16 +667,16 @@ def _parse_tokenize(rule): class ParseStateMeta(type): - """ - Metaclass for the ParseState class. Facilitates identifying - reduction methods. + """Metaclass for the ParseState class. + + Facilitates identifying reduction methods. """ def __new__(mcs, name, bases, cls_dict): - """ - Create the class. Injects the 'reducers' list, a list of - tuples matching token sequences to the names of the - corresponding reduction methods. + """Create the class. + + Injects the 'reducers' list, a list of tuples matching token sequences + to the names of the corresponding reduction methods. """ reducers = [] @@ -526,10 +693,10 @@ class ParseStateMeta(type): def reducer(*tokens): - """ - Decorator for reduction methods. Arguments are a sequence of - tokens, in order, which should trigger running this reduction - method. + """Decorator for reduction methods. + + Arguments are a sequence of tokens, in order, which should trigger running + this reduction method. """ def decorator(func): @@ -545,11 +712,12 @@ def reducer(*tokens): return decorator +@six.add_metaclass(ParseStateMeta) class ParseState(object): - """ - Implement the core of parsing the policy language. Uses a greedy - reduction algorithm to reduce a sequence of tokens into a single - terminal, the value of which will be the root of the Check tree. + """Implement the core of parsing the policy language. + + Uses a greedy reduction algorithm to reduce a sequence of tokens into + a single terminal, the value of which will be the root of the Check tree. Note: error reporting is rather lacking. The best we can get with this parser formulation is an overall "parse failed" error. @@ -557,8 +725,6 @@ class ParseState(object): shouldn't be that big a problem. """ - __metaclass__ = ParseStateMeta - def __init__(self): """Initialize the ParseState.""" @@ -566,11 +732,11 @@ class ParseState(object): self.values = [] def reduce(self): - """ - Perform a greedy reduction of the token stream. If a reducer - method matches, it will be executed, then the reduce() method - will be called recursively to search for any more possible - reductions. + """Perform a greedy reduction of the token stream. + + If a reducer method matches, it will be executed, then the + reduce() method will be called recursively to search for any more + possible reductions. """ for reduction, methname in self.reducers: @@ -600,9 +766,9 @@ class ParseState(object): @property def result(self): - """ - Obtain the final result of the parse. Raises ValueError if - the parse failed to reduce to a single result. + """Obtain the final result of the parse. + + Raises ValueError if the parse failed to reduce to a single result. """ if len(self.values) != 1: @@ -619,35 +785,31 @@ class ParseState(object): @reducer('check', 'and', 'check') def _make_and_expr(self, check1, _and, check2): - """ - Create an 'and_expr' from two checks joined by the 'and' - operator. + """Create an 'and_expr'. + + Join two checks by the 'and' operator. """ return [('and_expr', AndCheck([check1, check2]))] @reducer('and_expr', 'and', 'check') def _extend_and_expr(self, and_expr, _and, check): - """ - Extend an 'and_expr' by adding one more check. - """ + """Extend an 'and_expr' by adding one more check.""" return [('and_expr', and_expr.add_check(check))] @reducer('check', 'or', 'check') def _make_or_expr(self, check1, _or, check2): - """ - Create an 'or_expr' from two checks joined by the 'or' - operator. + """Create an 'or_expr'. + + Join two checks by the 'or' operator. """ return [('or_expr', OrCheck([check1, check2]))] @reducer('or_expr', 'or', 'check') def _extend_or_expr(self, or_expr, _or, check): - """ - Extend an 'or_expr' by adding one more check. - """ + """Extend an 'or_expr' by adding one more check.""" return [('or_expr', or_expr.add_check(check))] @@ -659,7 +821,8 @@ class ParseState(object): def _parse_text_rule(rule): - """ + """Parses policy to the tree. + Translates a policy written in the policy language into a tree of Check objects. """ @@ -677,26 +840,23 @@ def _parse_text_rule(rule): return state.result except ValueError: # Couldn't parse the rule - LOG.exception(_("Failed to understand rule %(rule)r") % locals()) + LOG.exception(_LE("Failed to understand rule %s") % rule) # Fail closed return FalseCheck() def parse_rule(rule): - """ - Parses a policy rule into a tree of Check objects. - """ + """Parses a policy rule into a tree of Check objects.""" # If the rule is a string, it's in the policy language - if isinstance(rule, basestring): + if isinstance(rule, six.string_types): return _parse_text_rule(rule) return _parse_list_rule(rule) def register(name, func=None): - """ - Register a function or Check class as a policy check. + """Register a function or Check class as a policy check. :param name: Gives the name of the check type, e.g., 'rule', 'role', etc. If name is None, a default check type @@ -723,13 +883,11 @@ def register(name, func=None): @register("rule") class RuleCheck(Check): - def __call__(self, target, creds): - """ - Recursively checks credentials based on the defined rules. - """ + def __call__(self, target, creds, enforcer): + """Recursively checks credentials based on the defined rules.""" try: - return _rules[self.match](target, creds) + return enforcer.rules[self.match](target, creds, enforcer) except KeyError: # We don't have any matching rule; fail closed return False @@ -737,7 +895,7 @@ class RuleCheck(Check): @register("role") class RoleCheck(Check): - def __call__(self, target, creds): + def __call__(self, target, creds, enforcer): """Check that there is a matching role in the cred dict.""" return self.match.lower() in [x.lower() for x in creds['roles']] @@ -745,36 +903,60 @@ class RoleCheck(Check): @register('http') class HttpCheck(Check): - def __call__(self, target, creds): - """ - Check http: rules by calling to a remote server. + def __call__(self, target, creds, enforcer): + """Check http: rules by calling to a remote server. This example implementation simply verifies that the response is exactly 'True'. """ url = ('http:' + self.match) % target - data = {'target': jsonutils.dumps(target), + + # Convert instances of object() in target temporarily to + # empty dict to avoid circular reference detection + # errors in jsonutils.dumps(). + temp_target = copy.deepcopy(target) + for key in target.keys(): + element = target.get(key) + if type(element) is object: + temp_target[key] = {} + + data = {'target': jsonutils.dumps(temp_target), 'credentials': jsonutils.dumps(creds)} - post_data = urllib.urlencode(data) - f = urllib2.urlopen(url, post_data) + post_data = urlparse.urlencode(data) + f = urlrequest.urlopen(url, post_data) return f.read() == "True" @register(None) class GenericCheck(Check): - def __call__(self, target, creds): - """ - Check an individual match. + def __call__(self, target, creds, enforcer): + """Check an individual match. Matches look like: tenant:%(tenant_id)s role:compute:admin + True:%(user.enabled)s + 'Member':%(role.name)s """ - # TODO(termie): do dict inspection via dot syntax - match = self.match % target - if self.kind in creds: - return match == six.text_type(creds[self.kind]) - return False + try: + match = self.match % target + except KeyError: + # While doing GenericCheck if key not + # present in Target return false + return False + + try: + # Try to interpret self.kind as a literal + leftval = ast.literal_eval(self.kind) + except ValueError: + try: + kind_parts = self.kind.split('.') + leftval = creds + for kind_part in kind_parts: + leftval = leftval[kind_part] + except KeyError: + return False + return match == six.text_type(leftval) diff --git a/ironic/tests/fake_policy.py b/ironic/tests/fake_policy.py index ccd40e3259..dbd686492f 100644 --- a/ironic/tests/fake_policy.py +++ b/ironic/tests/fake_policy.py @@ -15,8 +15,9 @@ policy_data = """ { - "admin": "role:admin or role:administrator", - "admin_api": "is_admin:True", - "default": "rule:admin_api" + "admin_api": "role:admin or role:administrator", + "public_api": "is_public_api:True", + "trusted_call": "rule:admin_api or rule:public_api", + "default": "rule:trusted_call" } """ diff --git a/ironic/tests/policy_fixture.py b/ironic/tests/policy_fixture.py index 421e345cef..59e7b60173 100644 --- a/ironic/tests/policy_fixture.py +++ b/ironic/tests/policy_fixture.py @@ -18,7 +18,6 @@ import fixtures from oslo.config import cfg from ironic.common import policy as ironic_policy -from ironic.openstack.common import policy as common_policy from ironic.tests import fake_policy CONF = cfg.CONF @@ -34,11 +33,5 @@ class PolicyFixture(fixtures.Fixture): with open(self.policy_file_name, 'w') as policy_file: policy_file.write(fake_policy.policy_data) CONF.set_override('policy_file', self.policy_file_name) - ironic_policy.reset() - ironic_policy.init() - self.addCleanup(ironic_policy.reset) - - def set_rules(self, rules): - common_policy.set_rules(common_policy.Rules( - dict((k, common_policy.parse_rule(v)) - for k, v in rules.items()))) + ironic_policy._ENFORCER = None + self.addCleanup(ironic_policy.get_enforcer().clear) diff --git a/ironic/tests/test_policy.py b/ironic/tests/test_policy.py index 401a560183..1f9955dc06 100644 --- a/ironic/tests/test_policy.py +++ b/ironic/tests/test_policy.py @@ -15,19 +15,52 @@ # License for the specific language governing permissions and limitations # under the License. -from oslo.config import cfg - -from ironic.common import exception -from ironic.common import policy as ironic_policy +from ironic.common import policy from ironic.tests import base -CONF = cfg.CONF - - class PolicyTestCase(base.TestCase): + """Tests whether the configuration of the policy engine is corect.""" - def test_policy_file_not_found(self): - ironic_policy.reset() - CONF.set_override('policy_file', '/non/existent/policy/file') - self.assertRaises(exception.ConfigNotFound, ironic_policy.init) + def test_admin_api(self): + creds = ({'roles': [u'admin']}, + {'roles': ['administrator']}, + {'roles': ['admin', 'administrator']}) + + for c in creds: + self.assertTrue(policy.enforce('admin_api', c, c)) + + def test_public_api(self): + creds = {'is_public_api': 'True'} + self.assertTrue(policy.enforce('public_api', creds, creds)) + + def test_trusted_call(self): + creds = ({'roles': ['admin']}, + {'is_public_api': 'True'}, + {'roles': ['admin'], 'is_public_api': 'True'}, + {'roles': ['Member'], 'is_public_api': 'True'}) + + for c in creds: + self.assertTrue(policy.enforce('trusted_call', c, c)) + + +class PolicyTestCaseNegative(base.TestCase): + """Tests whether the configuration of the policy engine is corect.""" + + def test_admin_api(self): + creds = {'roles': ['Member']} + self.assertFalse(policy.enforce('admin_api', creds, creds)) + + def test_public_api(self): + creds = ({'is_public_api': 'False'}, {}) + + for c in creds: + self.assertFalse(policy.enforce('public_api', c, c)) + + def test_trusted_call(self): + creds = ({'roles': ['Member']}, + {'is_public_api': 'False'}, + {'roles': ['Member'], 'is_public_api': 'False'}) + + for c in creds: + self.assertFalse(policy.enforce('trusted_call', c, c))