From 2d0bc7fd506d1c6000bfff3f8713b36e11ed9776 Mon Sep 17 00:00:00 2001 From: Roman Prykhodchenko Date: Mon, 7 Oct 2013 16:48:06 +0300 Subject: [PATCH] Fix policies Policy file contained malformed content so the policy engine failed to parse it. That was the reason of rejecting all requests, if authentication was enabled. This patch also updates policies to get rid of unused policies and use GenericCheck to check for admin API. After changes mentioned above some unused code appeared in ironic.common.policy and so it was cleaned up. Closes-bug: #1236371 Change-Id: Ie1dbda11561a9e7068d240a19f9fb98eae121c94 --- etc/ironic/policy.json | 5 ++- ironic/api/acl.py | 17 ---------- ironic/api/app.py | 5 ++- ironic/api/hooks.py | 18 ++++++++++- ironic/common/policy.py | 63 ------------------------------------- ironic/tests/api/base.py | 2 -- ironic/tests/fake_policy.py | 7 ++--- ironic/tests/policy.json | 6 ---- 8 files changed, 26 insertions(+), 97 deletions(-) delete mode 100644 ironic/tests/policy.json diff --git a/etc/ironic/policy.json b/etc/ironic/policy.json index a889e0bb0f..94ac3a5b80 100644 --- a/etc/ironic/policy.json +++ b/etc/ironic/policy.json @@ -1,6 +1,5 @@ { + "admin": "role:admin or role:administrator", "admin_api": "is_admin:True", - "admin_or_owner": "is_admin:True or project_id:%(project_id)s", - "is_admin": "role:admin or role:administrator", - "default": "rule:admin_or_owner", + "default": "rule:admin_api" } diff --git a/ironic/api/acl.py b/ironic/api/acl.py index 85fa5325c9..453b67b465 100644 --- a/ironic/api/acl.py +++ b/ironic/api/acl.py @@ -20,11 +20,8 @@ from keystoneclient.middleware import auth_token as keystone_auth_token from oslo.config import cfg -from pecan import hooks -from webob import exc from ironic.api.middleware import auth_token -from ironic.common import policy OPT_GROUP_NAME = 'keystone_authtoken' @@ -56,17 +53,3 @@ def install(app, conf, public_routes): return auth_token.AuthTokenMiddleware(app, conf=keystone_config, public_api_routes=public_routes) - - -class AdminAuthHook(hooks.PecanHook): - """Verify that the user has admin rights. - - Checks whether the request context is an admin context and - rejects the request otherwise. - - """ - def before(self, state): - ctx = state.request.context - - if not policy.check_is_admin(ctx) and not ctx.is_public_api: - raise exc.HTTPForbidden() diff --git a/ironic/api/app.py b/ironic/api/app.py index 8bb865baa6..ec42b6c54e 100644 --- a/ironic/api/app.py +++ b/ironic/api/app.py @@ -23,6 +23,7 @@ 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,6 +42,8 @@ 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), @@ -52,7 +55,7 @@ def setup_app(pecan_config=None, extra_hooks=None): pecan_config = get_pecan_config() if pecan_config.app.enable_acl: - app_hooks.append(acl.AdminAuthHook()) + app_hooks.append(hooks.AdminAuthHook()) pecan.configuration.set_config(dict(pecan_config), overwrite=True) diff --git a/ironic/api/hooks.py b/ironic/api/hooks.py index 5162eba7fa..20739a9486 100644 --- a/ironic/api/hooks.py +++ b/ironic/api/hooks.py @@ -18,6 +18,7 @@ from oslo.config import cfg from pecan import hooks +from webob import exc from ironic.common import context from ironic.common import utils @@ -74,7 +75,7 @@ class ContextHook(hooks.PecanHook): auth_token = state.request.headers.get('X-Auth-Token', None) creds = {'roles': state.request.headers.get('X-Roles', '').split(',')} - is_admin = policy.check('is_admin', state.request.headers, creds) + is_admin = policy.check('admin', state.request.headers, creds) path = utils.safe_rstrip(state.request.path, '/') is_public_api = path in self.public_api_routes @@ -94,3 +95,18 @@ class RPCHook(hooks.PecanHook): def before(self, state): state.request.rpcapi = rpcapi.ConductorAPI() + + +class AdminAuthHook(hooks.PecanHook): + """Verify that the user has admin rights. + + Checks whether the request context is an admin context and + rejects the request otherwise. + + """ + 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() diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 1c764dacaa..4326556c83 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -66,66 +66,3 @@ def init(): def _set_rules(data): default_rule = CONF.policy_default_rule policy.set_rules(policy.Rules.load_json(data, default_rule)) - - -def enforce(context, action, target, do_raise=True): - """Verifies that the action is valid on the target in this context. - - :param context: ironic context - :param action: string representing the action to be checked - this should be colon separated for clarity. - i.e. ``compute:create_instance``, - ``compute:attach_volume``, - ``volume:attach_volume`` - :param target: dictionary representing the object of the action - for object creation this should be a dictionary representing the - location of the object e.g. ``{'project_id': context.project_id}`` - :param do_raise: if True (the default), raises PolicyNotAuthorized; - if False, returns False - - :raises ironic.exception.PolicyNotAuthorized: if verification fails - and do_raise is True. - - :return: returns a non-False value (not necessarily "True") if - authorized, and the exact value False if not authorized and - do_raise is False. - """ - init() - - credentials = context.to_dict() - - # Add the exception arguments if asked to do a raise - extra = {} - if do_raise: - extra.update(exc=exception.PolicyNotAuthorized, action=action) - - return policy.check(action, target, credentials, **extra) - - -def check_is_admin(context): - """Whether or not role contains 'admin' role according to policy setting. - - """ - init() - - credentials = context.to_dict() - target = credentials - - return policy.check('context_is_admin', target, credentials) - - -@policy.register('context_is_admin') -class IsAdminCheck(policy.Check): - """An explicit check for is_admin.""" - - def __init__(self, kind, match): - """Initialize the check.""" - - self.expected = (match.lower() == 'true') - - super(IsAdminCheck, self).__init__(kind, str(self.expected)) - - def __call__(self, target, creds): - """Determine whether is_admin matches the requested value.""" - - return creds['is_admin'] == self.expected diff --git a/ironic/tests/api/base.py b/ironic/tests/api/base.py index f79c4412cd..df227084ce 100644 --- a/ironic/tests/api/base.py +++ b/ironic/tests/api/base.py @@ -42,8 +42,6 @@ class FunctionalTest(base.DbTestCase): def setUp(self): super(FunctionalTest, self).setUp() cfg.CONF.set_override("auth_version", "v2.0", group=acl.OPT_GROUP_NAME) - cfg.CONF.set_override("policy_file", - self.path_get('tests/policy.json')) self.app = self._make_app() self.dbapi = dbapi.get_instance() diff --git a/ironic/tests/fake_policy.py b/ironic/tests/fake_policy.py index 15e19db95d..3851966b19 100644 --- a/ironic/tests/fake_policy.py +++ b/ironic/tests/fake_policy.py @@ -17,9 +17,8 @@ policy_data = """ { - "admin_api": "role:admin", - "admin_or_owner": "is_admin:True or project_id:%(project_id)s", - "is_admin": "role:admin or role:administrator", - "default": "rule:admin_or_owner" + "admin": "role:admin or role:administrator", + "admin_api": "is_admin:True", + "default": "rule:admin_api" } """ diff --git a/ironic/tests/policy.json b/ironic/tests/policy.json deleted file mode 100644 index a889e0bb0f..0000000000 --- a/ironic/tests/policy.json +++ /dev/null @@ -1,6 +0,0 @@ -{ - "admin_api": "is_admin:True", - "admin_or_owner": "is_admin:True or project_id:%(project_id)s", - "is_admin": "role:admin or role:administrator", - "default": "rule:admin_or_owner", -}