From 88673f1e949a41cfe804ff919e16935c6dca11e1 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 2 Mar 2021 15:42:19 -0800 Subject: [PATCH] Allocation support for project scoped RBAC Adds policy scope based RBAC handling for the allocations endpoing which enables admins to create allocations if they have baremetal nodes which are available to them. Change-Id: I60e273afaf344fded9bdb8c4c8e143efc9971fc1 --- doc/source/admin/secure-rbac.rst | 27 ++++ ironic/api/controllers/v1/allocation.py | 147 +++++++++++++++-- ironic/api/controllers/v1/utils.py | 28 +++- ironic/common/policy.py | 67 ++++++-- ironic/db/sqlalchemy/api.py | 6 +- .../api/controllers/v1/test_allocation.py | 64 +++++++- .../unit/api/controllers/v1/test_utils.py | 16 +- ironic/tests/unit/api/test_acl.py | 15 +- ironic/tests/unit/api/test_rbac_legacy.yaml | 10 +- .../unit/api/test_rbac_project_scoped.yaml | 151 ++++++++++++++---- ...stricted-rbac-create-2847943150656432.yaml | 13 ++ tox.ini | 2 +- 12 files changed, 461 insertions(+), 85 deletions(-) create mode 100644 releasenotes/notes/allocations-restricted-rbac-create-2847943150656432.yaml diff --git a/doc/source/admin/secure-rbac.rst b/doc/source/admin/secure-rbac.rst index f25aa6a96d..075dd1181b 100644 --- a/doc/source/admin/secure-rbac.rst +++ b/doc/source/admin/secure-rbac.rst @@ -68,10 +68,12 @@ Supported Endpoints * /nodes//portgroups * /nodes//volume/connectors * /nodes//volume/targets +* /nodes//allocation * /ports * /portgroups * /volume/connectors * /volume/targets +* /allocations How Project Scoped Works ------------------------ @@ -147,6 +149,31 @@ change the owner. More information is available on these fields in :doc:`/configuration/policy`. +Allocations +~~~~~~~~~~~ + +The ``allocations`` endpoint of the API is somewhat different than other +other endpoints as it allows for the allocation of physical machines to +an admin. In this context, there is not already an ``owner`` or ``project_id`` +to leverage to control access for the creation process, any project admin +does have the inherent prilege of requesting an allocation. That being said, +their allocation request will require physical nodes to be owned or leased +to the ``project_id`` through the ``node`` fields ``owner`` or ``lessee``. + +Ability to override the owner is restricted to system scoped users by default +and any new allocation being requested with a specific owner, if made in +``project`` scope, will have the ``project_id`` recorded as the owner of +the allocation. + +.. WARNING:: The allocation endpoint's use is restricted to project scoped + interactions until ``[oslo_policy]enforce_new_defaults`` has been set + to ``True`` using the ``baremetal:allocation:create_pre_rbac`` policy + rule. This is in order to prevent endpoint misuse. Afterwards all + project scoped allocations will automatically populate an owner. + System scoped request are not subjected to this restriction, + and operators may change the default restriction via the + ``baremetal:allocation:create_restricted`` policy. + Pratical differences -------------------- diff --git a/ironic/api/controllers/v1/allocation.py b/ironic/api/controllers/v1/allocation.py index 14a7201fcc..845a5dec2b 100644 --- a/ironic/api/controllers/v1/allocation.py +++ b/ironic/api/controllers/v1/allocation.py @@ -13,6 +13,7 @@ from http import client as http_client from ironic_lib import metrics_utils +from oslo_config import cfg from oslo_utils import uuidutils import pecan from webob import exc as webob_exc @@ -28,8 +29,10 @@ from ironic.common import exception from ironic.common.i18n import _ from ironic import objects -METRICS = metrics_utils.get_metrics_logger(__name__) +CONF = cfg.CONF + +METRICS = metrics_utils.get_metrics_logger(__name__) ALLOCATION_SCHEMA = { 'type': 'object', @@ -133,7 +136,8 @@ class AllocationsController(pecan.rest.RestController): def _get_allocations_collection(self, node_ident=None, resource_class=None, state=None, owner=None, marker=None, limit=None, sort_key='id', sort_dir='asc', - resource_url=None, fields=None): + resource_url=None, fields=None, + parent_node=None): """Return allocations collection. :param node_ident: UUID or name of a node. @@ -145,6 +149,9 @@ class AllocationsController(pecan.rest.RestController): :param fields: Optional, a list with a specified set of fields of the resource to be returned. :param owner: project_id of owner to filter by + :param parent_node: The explicit parent node uuid to return if + the controller is being accessed as a + sub-resource. i.e. /v1/nodes//allocation """ limit = api_utils.validate_limit(limit) sort_dir = api_utils.validate_sort_dir(sort_dir) @@ -154,17 +161,48 @@ class AllocationsController(pecan.rest.RestController): _("The sort_key value %(key)s is an invalid field for " "sorting") % {'key': sort_key}) + node_uuid = None + # If the user is not allowed to see everything, we need to filter + # based upon access rights. + cdict = api.request.context.to_policy_values() + if cdict.get('system_scope') != 'all' and not parent_node: + # The user is a project scoped, and there is not an explicit + # parent node which will be returned. + if not api_utils.check_policy_true( + 'baremetal:allocation:list_all'): + # If the user cannot see everything via the policy, + # we need to filter the view down to only what they should + # be able to see in the database. + owner = cdict.get('project_id') + else: + # The controller is being accessed as a sub-resource. + # Cool, but that means there can only be one result. + node_uuid = parent_node + # Override if any node_ident was submitted in since this + # is a subresource query. + node_ident = parent_node + marker_obj = None if marker: marker_obj = objects.Allocation.get_by_uuid(api.request.context, marker) - if node_ident: try: - node_uuid = api_utils.get_rpc_node(node_ident).uuid + # Check ability to access the associated node or requested + # node to filter by. + rpc_node = api_utils.get_rpc_node(node_ident) + api_utils.check_owner_policy('node', 'baremetal:node:get', + rpc_node.owner, + lessee=rpc_node.lessee, + conceal_node=False) + node_uuid = rpc_node.uuid except exception.NodeNotFound as exc: exc.code = http_client.BAD_REQUEST raise + except exception.NotAuthorized as exc: + if not parent_node: + exc.code = http_client.BAD_REQUEST + raise exception.NotFound() else: node_uuid = None @@ -179,13 +217,16 @@ class AllocationsController(pecan.rest.RestController): for key, value in possible_filters.items(): if value is not None: filters[key] = value - allocations = objects.Allocation.list(api.request.context, limit=limit, marker=marker_obj, sort_key=sort_key, sort_dir=sort_dir, filters=filters) + for allocation in allocations: + api_utils.check_owner_policy('allocation', + 'baremetal:allocation:get', + allocation.owner) return list_convert_with_links(allocations, limit, url=resource_url, fields=fields, @@ -267,18 +308,33 @@ class AllocationsController(pecan.rest.RestController): def _authorize_create_allocation(self, allocation): try: + # PRE-RBAC this rule was logically restricted, it is more-unlocked + # post RBAC, but we need to ensure it is not abused. api_utils.check_policy('baremetal:allocation:create') self._check_allowed_allocation_fields(allocation) + if (not CONF.oslo_policy.enforce_new_defaults + and not allocation.get('owner')): + # Even if permitted, we need to go ahead and check if this is + # restricted for now until scoped interaction is the default + # interaction. + api_utils.check_policy('baremetal:allocation:create_pre_rbac') + # TODO(TheJulia): This can be removed later once we + # move entirely to scope based checking. This requires + # that if the scope enforcement is not enabled, that + # any user can't create an allocation until the deployment + # is in a new operating mode *where* owner will be added + # automatically if not a privilged user. except exception.HTTPForbidden: cdict = api.request.context.to_policy_values() - owner = cdict.get('project_id') - if not owner or (allocation.get('owner') - and owner != allocation.get('owner')): + project = cdict.get('project_id') + if (project and allocation.get('owner') + and project != allocation.get('owner')): raise + if project and not CONF.oslo_policy.enforce_new_defaults: + api_utils.check_policy('baremetal:allocation:create_pre_rbac') api_utils.check_policy('baremetal:allocation:create_restricted') self._check_allowed_allocation_fields(allocation) - allocation['owner'] = owner - + allocation['owner'] = project return allocation @METRICS.timer('AllocationsController.post') @@ -291,6 +347,7 @@ class AllocationsController(pecan.rest.RestController): :param allocation: an allocation within the request body. """ context = api.request.context + cdict = context.to_policy_values() allocation = self._authorize_create_allocation(allocation) if (allocation.get('name') @@ -299,11 +356,47 @@ class AllocationsController(pecan.rest.RestController): "'%(name)s'") % {'name': allocation['name']} raise exception.Invalid(msg) + # TODO(TheJulia): We need to likely look at refactoring post + # processing for allocations as pep8 says it is a complexity of 19, + # although it is not actually that horrible since it is phased out + # just modifying/assembling the allocation. Given that, it seems + # not great to try for a full method rewrite at the same time as + # RBAC work, so the complexity limit is being raised. :( + if (CONF.oslo_policy.enforce_new_defaults + and cdict.get('system_scope') != 'all'): + # if not a system scope originated request, we need to check/apply + # an owner - But we can only do this with when new defaults are + # enabled. + project_id = cdict.get('project_id') + req_alloc_owner = allocation.get('owner') + if req_alloc_owner: + if not api_utils.check_policy_true( + 'baremetal:allocation:create_restricted'): + if req_alloc_owner != project_id: + msg = _("Cannot create allocation with an owner " + "Project ID value %(req_owner)s not matching " + "the requestor Project ID %(project)s. " + "Policy baremetal:allocation:create_restricted" + " is required for this capability." + ) % {'req_owner': req_alloc_owner, + 'project': project_id} + raise exception.NotAuthorized(msg) + # NOTE(TheJulia): IF not restricted, i.e. else above, + # their supplied allocation owner is okay, they are allowed + # to provide an override by policy. + else: + # An allocation owner was not supplied, we need to save one. + allocation['owner'] = project_id + node = None if allocation.get('node'): if api_utils.allow_allocation_backfill(): try: node = api_utils.get_rpc_node(allocation['node']) + api_utils.check_owner_policy( + 'node', 'baremetal:node:get', + node.owner, node.lessee, + conceal_node=allocation['node']) except exception.NodeNotFound as exc: exc.code = http_client.BAD_REQUEST raise @@ -323,8 +416,17 @@ class AllocationsController(pecan.rest.RestController): if allocation.get('candidate_nodes'): # Convert nodes from names to UUIDs and check their validity try: + owner = None + if not api_utils.check_policy_true( + 'baremetal:allocation:create_restricted'): + owner = cdict.get('project_id') + # Filter the candidate search by the requestor project ID + # if any. The result is processes authenticating with system + # scope will not be impacted, where as project scoped requests + # will need additional authorization. converted = api.request.dbapi.check_node_list( - allocation['candidate_nodes']) + allocation['candidate_nodes'], + project=owner) except exception.NodeNotFound as exc: exc.code = http_client.BAD_REQUEST raise @@ -458,10 +560,12 @@ class NodeAllocationController(pecan.rest.RestController): @method.expose() @args.validate(fields=args.string_list) def get_all(self, fields=None): - api_utils.check_policy('baremetal:allocation:get') + parent_node = self.parent_node_ident + result = self.inner._get_allocations_collection( + parent_node, + fields=fields, + parent_node=parent_node) - result = self.inner._get_allocations_collection(self.parent_node_ident, - fields=fields) try: return result['allocations'][0] except IndexError: @@ -473,15 +577,26 @@ class NodeAllocationController(pecan.rest.RestController): @method.expose(status_code=http_client.NO_CONTENT) def delete(self): context = api.request.context - api_utils.check_policy('baremetal:allocation:delete') rpc_node = api_utils.get_rpc_node_with_suffix(self.parent_node_ident) + # Check the policy, and 404 if not authorized. + api_utils.check_owner_policy('node', 'baremetal:node:get', + rpc_node.owner, lessee=rpc_node.lessee, + conceal_node=self.parent_node_ident) + + # A project ID is associated, thus we should filter + # our search using it. + filters = {'node_uuid': rpc_node.uuid} allocations = objects.Allocation.list( api.request.context, - filters={'node_uuid': rpc_node.uuid}) + filters=filters) try: rpc_allocation = allocations[0] + allocation_owner = allocations[0]['owner'] + api_utils.check_owner_policy('allocation', + 'baremetal:allocation:delete', + allocation_owner) except IndexError: raise exception.AllocationNotFound( _("Allocation for node %s was not found") % diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 90a2a258c5..1fea853d0a 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -1507,9 +1507,23 @@ def check_policy(policy_name): # effectively a noop from an authorization perspective because the values # we're comparing are coming from the same place. cdict = api.request.context.to_policy_values() + policy.authorize(policy_name, cdict, api.request.context) +def check_policy_true(policy_name): + """Check if the specified policy is authorised for this request. + + :policy_name: Name of the policy to check. + :returns: True if policy is matched, otherwise false. + """ + # NOTE(lbragstad): Mapping context attributes into a target dictionary is + # effectively a noop from an authorization perspective because the values + # we're comparing are coming from the same place. + cdict = api.request.context.to_policy_values() + return policy.check_policy(policy_name, cdict, api.request.context) + + def check_owner_policy(object_type, policy_name, owner, lessee=None, conceal_node=False): """Check if the policy authorizes this request on an object. @@ -1592,13 +1606,13 @@ def check_allocation_policy_and_retrieve(policy_name, allocation_ident): try: rpc_allocation = get_rpc_allocation_with_suffix( allocation_ident) - except exception.AllocationNotFound: - # don't expose non-existence unless requester - # has generic access to policy - cdict = api.request.context.to_policy_values() - policy.authorize(policy_name, cdict, api.request.context) - raise - + # If the user is not allowed to view the allocation, then + # we need to check that and respond with a 404. + check_owner_policy('allocation', 'baremetal:allocation:get', + rpc_allocation['owner']) + except exception.NotAuthorized: + raise exception.AllocationNotFound(allocation=allocation_ident) + # The primary policy check for allocation. check_owner_policy('allocation', policy_name, rpc_allocation['owner']) return rpc_allocation diff --git a/ironic/common/policy.py b/ironic/common/policy.py index 05b2d68be7..5a2612f0f2 100644 --- a/ironic/common/policy.py +++ b/ironic/common/policy.py @@ -92,6 +92,10 @@ PROJECT_OWNER_MEMBER = ('role:member and project_id:%(node.owner)s') PROJECT_OWNER_READER = ('role:reader and project_id:%(node.owner)s') PROJECT_LESSEE_ADMIN = ('role:admin and project_id:%(node.lessee)s') +ALLOCATION_OWNER_ADMIN = ('role:admin and project_id:%(allocation.owner)s') +ALLOCATION_OWNER_MEMBER = ('role:member and project_id:%(allocation.owner)s') +ALLOCATION_OWNER_READER = ('role:reader and project_id:%(allocation.owner)s') + SYSTEM_OR_OWNER_MEMBER_AND_LESSEE_ADMIN = ( '(' + SYSTEM_MEMBER + ') or (' + PROJECT_OWNER_MEMBER + ') or (' + PROJECT_LESSEE_ADMIN + ')' # noqa ) @@ -117,6 +121,25 @@ SYSTEM_MEMBER_OR_OWNER_LESSEE_ADMIN = ( ) +# The system has rights to manage the allocations for users, in a sense +# a delegated management since they are not creating a real object or asset, +# but allocations of assets. +ALLOCATION_ADMIN = ( + '(' + SYSTEM_MEMBER + ') or (' + ALLOCATION_OWNER_ADMIN + ')' +) + +ALLOCATION_MEMBER = ( + '(' + SYSTEM_MEMBER + ') or (' + ALLOCATION_OWNER_MEMBER + ')' +) + +ALLOCATION_READER = ( + '(' + SYSTEM_READER + ') or (' + ALLOCATION_OWNER_READER + ')' +) + +ALLOCATION_CREATOR = ( + '(' + SYSTEM_MEMBER + ') or (role:admin)' +) + # Special purpose aliases for things like "ability to access the API # as a reader, or permission checking that does not require node # owner relationship checking @@ -1497,7 +1520,7 @@ deprecated_allocation_list = policy.DeprecatedRule( ) deprecated_allocation_list_all = policy.DeprecatedRule( name='baremetal:allocation:list_all', - check_str='rule:baremetal:allocation:get' + check_str='rule:baremetal:allocation:get and is_admin_project:True' ) deprecated_allocation_create = policy.DeprecatedRule( name='baremetal:allocation:create', @@ -1523,8 +1546,8 @@ roles. allocation_policies = [ policy.DocumentedRuleDefault( name='baremetal:allocation:get', - check_str=SYSTEM_READER, - scope_types=['system'], + check_str=ALLOCATION_READER, + scope_types=['system', 'project'], description='Retrieve Allocation records', operations=[ {'path': '/allocations/{allocation_id}', 'method': 'GET'}, @@ -1536,8 +1559,8 @@ allocation_policies = [ ), policy.DocumentedRuleDefault( name='baremetal:allocation:list', - check_str=SYSTEM_READER, - scope_types=['system'], + check_str=API_READER, + scope_types=['system', 'project'], description='Retrieve multiple Allocation records, filtered by owner', operations=[{'path': '/allocations', 'method': 'GET'}], deprecated_rule=deprecated_allocation_list, @@ -1547,7 +1570,7 @@ allocation_policies = [ policy.DocumentedRuleDefault( name='baremetal:allocation:list_all', check_str=SYSTEM_READER, - scope_types=['system'], + scope_types=['system', 'project'], description='Retrieve multiple Allocation records', operations=[{'path': '/allocations', 'method': 'GET'}], deprecated_rule=deprecated_allocation_list_all, @@ -1556,8 +1579,8 @@ allocation_policies = [ ), policy.DocumentedRuleDefault( name='baremetal:allocation:create', - check_str=SYSTEM_MEMBER, - scope_types=['system'], + check_str=ALLOCATION_CREATOR, + scope_types=['system', 'project'], description='Create Allocation records', operations=[{'path': '/allocations', 'method': 'POST'}], deprecated_rule=deprecated_allocation_create, @@ -1567,9 +1590,9 @@ allocation_policies = [ policy.DocumentedRuleDefault( name='baremetal:allocation:create_restricted', check_str=SYSTEM_MEMBER, - scope_types=['system'], + scope_types=['system', 'project'], description=( - 'Create Allocation records that are restricted to an owner' + 'Create Allocation records with a specific owner.' ), operations=[{'path': '/allocations', 'method': 'POST'}], deprecated_rule=deprecated_allocation_create_restricted, @@ -1578,8 +1601,8 @@ allocation_policies = [ ), policy.DocumentedRuleDefault( name='baremetal:allocation:delete', - check_str=SYSTEM_MEMBER, - scope_types=['system'], + check_str=ALLOCATION_ADMIN, + scope_types=['system', 'project'], description='Delete Allocation records', operations=[ {'path': '/allocations/{allocation_id}', 'method': 'DELETE'}, @@ -1590,8 +1613,8 @@ allocation_policies = [ ), policy.DocumentedRuleDefault( name='baremetal:allocation:update', - check_str=SYSTEM_MEMBER, - scope_types=['system'], + check_str=ALLOCATION_MEMBER, + scope_types=['system', 'project'], description='Change name and extra fields of an allocation', operations=[ {'path': '/allocations/{allocation_id}', 'method': 'PATCH'}, @@ -1600,6 +1623,22 @@ allocation_policies = [ deprecated_reason=deprecated_allocation_reason, deprecated_since=versionutils.deprecated.WALLABY ), + policy.DocumentedRuleDefault( + name='baremetal:allocation:create_pre_rbac', + check_str=('rule:is_member and role:baremetal_admin or ' + 'is_admin_project:True and role:admin'), + scope_types=['project'], + description=('Logical restrictor to prevent legacy allocation rule ' + 'missuse - Requires blank allocations to originate from ' + 'the legacy baremetal_admin.'), + operations=[ + {'path': '/allocations/{allocation_id}', 'method': 'PATCH'}, + ], + deprecated_reason=deprecated_allocation_reason, + deprecated_for_removal=True, + deprecated_since=versionutils.deprecated.WALLABY + ), + ] diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 6f38c4b8f4..2029d89425 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/ironic/db/sqlalchemy/api.py @@ -439,7 +439,7 @@ class Connection(api.Connection): return _paginate_query(models.Node, limit, marker, sort_key, sort_dir, query) - def check_node_list(self, idents): + def check_node_list(self, idents, project=None): mapping = {} if idents: idents = set(idents) @@ -459,6 +459,10 @@ class Connection(api.Connection): sql.or_(models.Node.uuid.in_(uuids), models.Node.name.in_(names)) ) + if project: + query = query.filter((models.Node.owner == project) + | (models.Node.lessee == project)) + for row in query: if row[0] in idents: mapping[row[0]] = row[0] diff --git a/ironic/tests/unit/api/controllers/v1/test_allocation.py b/ironic/tests/unit/api/controllers/v1/test_allocation.py index d73592cb35..e6cca71ddc 100644 --- a/ironic/tests/unit/api/controllers/v1/test_allocation.py +++ b/ironic/tests/unit/api/controllers/v1/test_allocation.py @@ -15,10 +15,12 @@ Tests for the API /allocations/ methods. import datetime from http import client as http_client +import json from unittest import mock from urllib import parse as urlparse import fixtures +from keystonemiddleware import auth_token from oslo_config import cfg from oslo_utils import timeutils from oslo_utils import uuidutils @@ -391,6 +393,15 @@ class TestListAllocations(test_api_base.BaseApiTest): owner=owner, uuid=uuidutils.generate_uuid(), name='allocation%s' % i) + # NOTE(TheJulia): Force the cast of the action to a system scoped + # scoped request. System scoped is allowed to view everything, + # where as project scoped requests are actually filtered with the + # secure-rbac work. This was done in troubleshooting the code, + # so may not be necessary, but filtered views are checked in + # the RBAC testing. + headers = self.headers + headers['X-Roles'] = "member,reader" + headers['OpenStack-System-Scope'] = "all" data = self.get_json("/allocations?owner=12345", headers=self.headers) self.assertEqual(3, len(data['allocations'])) @@ -994,6 +1005,57 @@ class TestPost(test_api_base.BaseApiTest): self.assertEqual('application/json', response.content_type) self.assertEqual(http_client.NOT_ACCEPTABLE, response.status_int) + @mock.patch.object(auth_token.AuthProtocol, 'process_request', + autospec=True) + def test_create_allocation_owner_not_my_projet_id(self, mock_auth_req): + # This is only enforced, test wise with the new oslo policy rbac + # model and enforcement. Likely can be cleaned up past the Xena cycle. + cfg.CONF.set_override('enforce_scope', True, group='oslo_policy') + cfg.CONF.set_override('enforce_new_defaults', True, + group='oslo_policy') + # Tests normally run in noauth, but we need policy + # enforcement to run completely here to ensure the logic is followed. + cfg.CONF.set_override('auth_strategy', 'keystone') + self.headers['X-Project-ID'] = '0987' + self.headers['X-Roles'] = 'admin,member,reader' + owner = '12345' + adict = apiutils.allocation_post_data(owner=owner) + response = self.post_json('/allocations', adict, expect_errors=True, + headers=self.headers) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + expected_faultstring = ('Cannot create allocation with an owner ' + 'Project ID value 12345 not matching the ' + 'requestor Project ID 0987. Policy ' + 'baremetal:allocation:create_restricted ' + 'is required for this capability.') + error_body = json.loads(response.json['error_message']) + self.assertEqual(expected_faultstring, + error_body.get('faultstring')) + + def test_create_allocation_owner_auto_filled(self): + cfg.CONF.set_override('enforce_new_defaults', True, + group='oslo_policy') + self.headers['X-Project-ID'] = '123456' + adict = apiutils.allocation_post_data() + self.post_json('/allocations', adict, headers=self.headers) + result = self.get_json('/allocations/%s' % adict['uuid'], + headers=self.headers) + self.assertEqual('123456', result['owner']) + + def test_create_allocation_is_restricted_until_scope_enforcement(self): + cfg.CONF.set_override('enforce_new_defaults', False, + group='oslo_policy') + cfg.CONF.set_override('auth_strategy', 'keystone') + # We're setting ourselves to be a random ID and member + # which is allowed to create an allocation. + self.headers['X-Project-ID'] = '1135' + self.headers['X-Roles'] = 'admin, member, reader' + self.headers['X-Is-Admin-Project'] = 'False' + adict = apiutils.allocation_post_data() + response = self.post_json('/allocations', adict, expect_errors=True, + headers=self.headers) + self.assertEqual(http_client.FORBIDDEN, response.status_int) + def test_backfill(self): node = obj_utils.create_test_node(self.context) adict = apiutils.allocation_post_data(node=node.uuid) @@ -1092,7 +1154,6 @@ class TestPost(test_api_base.BaseApiTest): raise exception.HTTPForbidden(resource='fake') return True mock_authorize.side_effect = mock_authorize_function - owner = '12345' adict = apiutils.allocation_post_data() del adict['owner'] @@ -1126,7 +1187,6 @@ class TestPost(test_api_base.BaseApiTest): raise exception.HTTPForbidden(resource='fake') return True mock_authorize.side_effect = mock_authorize_function - owner = '12345' adict = apiutils.allocation_post_data(owner=owner) adict['owner'] = owner diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py index 5826af89b8..8c948f255d 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/ironic/tests/unit/api/controllers/v1/test_utils.py @@ -1253,14 +1253,18 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase): 'fake_policy', self.valid_allocation_uuid ) mock_graws.assert_called_once_with(self.valid_allocation_uuid) - mock_authorize.assert_called_once_with( - 'fake_policy', expected_target, fake_context) + + expected = [mock.call('baremetal:allocation:get', + expected_target, fake_context), + mock.call('fake_policy', expected_target, + fake_context)] + mock_authorize.assert_has_calls(expected) self.assertEqual(self.allocation, rpc_allocation) @mock.patch.object(api, 'request', spec_set=["context"]) @mock.patch.object(policy, 'authorize', spec=True) @mock.patch.object(utils, 'get_rpc_allocation_with_suffix', autospec=True) - def test_check_alloc_policy_and_retrieve_no_alloc_policy_forbidden( + def test_check_alloc_policy_and_retrieve_no_alloc_policy_not_found( self, mock_graws, mock_authorize, mock_pr ): mock_pr.context.to_policy_values.return_value = {} @@ -1269,7 +1273,7 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase): allocation=self.valid_allocation_uuid) self.assertRaises( - exception.HTTPForbidden, + exception.AllocationNotFound, utils.check_allocation_policy_and_retrieve, 'fake-policy', self.valid_allocation_uuid @@ -1295,7 +1299,7 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase): @mock.patch.object(api, 'request', spec_set=["context", "version"]) @mock.patch.object(policy, 'authorize', spec=True) @mock.patch.object(utils, 'get_rpc_allocation_with_suffix', autospec=True) - def test_check_allocation_policy_and_retrieve_policy_forbidden( + def test_check_allocation_policy_and_retrieve_policy_not_found( self, mock_graws, mock_authorize, mock_pr ): mock_pr.version.minor = 50 @@ -1304,7 +1308,7 @@ class TestCheckAllocationPolicyAndRetrieve(base.TestCase): mock_graws.return_value = self.allocation self.assertRaises( - exception.HTTPForbidden, + exception.AllocationNotFound, utils.check_allocation_policy_and_retrieve, 'fake-policy', self.valid_allocation_uuid diff --git a/ironic/tests/unit/api/test_acl.py b/ironic/tests/unit/api/test_acl.py index 023c47c8c0..508b18c3d0 100644 --- a/ironic/tests/unit/api/test_acl.py +++ b/ironic/tests/unit/api/test_acl.py @@ -398,9 +398,15 @@ class TestRBACProjectScoped(TestACLBase): uuid='65ea0296-219b-4635-b0c8-a6e055da878d', node_id=owned_node['id'], connector_id='iqn.2012-06.org.openstack.magic') + fake_owner_allocation = db_utils.create_test_allocation( + node_id=owned_node['id'], + owner=owner_project_id, + resource_class="CUSTOM_TEST") # Leased nodes + fake_allocation_id = 61 leased_node = db_utils.create_test_node( + allocation_id=fake_allocation_id, uuid=lessee_node_ident, owner=owner_project_id, lessee=lessee_project_id, @@ -416,6 +422,11 @@ class TestRBACProjectScoped(TestACLBase): node_id=leased_node['id']) fake_trait = 'CUSTOM_MEOW' fake_vif_port_id = "0e21d58f-5de2-4956-85ff-33935ea1ca01" + fake_leased_allocation = db_utils.create_test_allocation( + id=fake_allocation_id, + node_id=leased_node['id'], + owner=lessee_project_id, + resource_class="CUSTOM_LEASED") # Random objects that shouldn't be project visible other_port = db_utils.create_test_port( @@ -447,7 +458,9 @@ class TestRBACProjectScoped(TestACLBase): 'other_port_ident': other_port['uuid'], 'owner_portgroup_ident': owner_pgroup['uuid'], 'other_portgroup_ident': other_pgroup['uuid'], - 'driver_name': 'fake-driverz'}) + 'driver_name': 'fake-driverz', + 'owner_allocation': fake_owner_allocation['uuid'], + 'lessee_allocation': fake_leased_allocation['uuid']}) @ddt.file_data('test_rbac_project_scoped.yaml') @ddt.unpack diff --git a/ironic/tests/unit/api/test_rbac_legacy.yaml b/ironic/tests/unit/api/test_rbac_legacy.yaml index deda21757c..5b77a98490 100644 --- a/ironic/tests/unit/api/test_rbac_legacy.yaml +++ b/ironic/tests/unit/api/test_rbac_legacy.yaml @@ -1938,7 +1938,7 @@ allocations_allocation_id_get_member: path: '/v1/allocations/{allocation_ident}' method: get headers: *member_headers - assert_status: 403 + assert_status: 404 deprecated: true allocations_allocation_id_get_observer: @@ -1964,7 +1964,7 @@ allocations_allocation_id_patch_member: method: patch headers: *member_headers body: *allocation_patch - assert_status: 403 + assert_status: 404 deprecated: true allocations_allocation_id_patch_observer: @@ -1986,7 +1986,7 @@ allocations_allocation_id_delete_member: path: '/v1/allocations/{allocation_ident}' method: delete headers: *member_headers - assert_status: 403 + assert_status: 404 deprecated: true allocations_allocation_id_delete_observer: @@ -2008,7 +2008,7 @@ nodes_allocation_get_member: path: '/v1/nodes/{allocated_node_ident}/allocation' method: get headers: *member_headers - assert_status: 403 + assert_status: 404 deprecated: true nodes_allocation_get_observer: @@ -2029,7 +2029,7 @@ nodes_allocation_delete_member: path: '/v1/nodes/{allocated_node_ident}/allocation' method: delete headers: *member_headers - assert_status: 403 + assert_status: 404 deprecated: true nodes_allocation_delete_observer: diff --git a/ironic/tests/unit/api/test_rbac_project_scoped.yaml b/ironic/tests/unit/api/test_rbac_project_scoped.yaml index f3cc919413..76a2463894 100644 --- a/ironic/tests/unit/api/test_rbac_project_scoped.yaml +++ b/ironic/tests/unit/api/test_rbac_project_scoped.yaml @@ -2272,60 +2272,153 @@ third_party_admin_cannot_get_conductors: # This is a system scoped endpoint, everything should fail in this section. -owner_reader_cannot_get_allocations: +owner_reader_can_get_allocations: path: '/v1/allocations' method: get - headers: *lessee_admin_headers - assert_status: 403 - skip_reason: policy not implemented + headers: *lessee_reader_headers + assert_status: 200 + assert_list_length: + allocations: 1 -lessee_reader_cannot_get_allocations: +lessee_reader_can_get_allocations: path: '/v1/allocations' method: get - headers: *lessee_admin_headers - assert_status: 403 - skip_reason: policy not implemented + headers: *lessee_reader_headers + assert_status: 200 + assert_list_length: + allocations: 1 -third_party_admin_cannot_get_allocations: +owner_reader_can_get_their_allocation: + path: '/v1/allocations/{owner_allocation}' + method: get + headers: *owner_reader_headers + assert_status: 200 + assert_dict_contains: + resource_class: CUSTOM_TEST + +lessee_reader_can_get_their_allocation: + path: '/v1/allocations/{lessee_allocation}' + method: get + headers: *lessee_reader_headers + assert_status: 200 + assert_dict_contains: + resource_class: CUSTOM_LEASED + +owner_admin_can_delete_their_allocation: + path: '/v1/allocations/{owner_allocation}' + method: delete + headers: *owner_admin_headers + assert_status: 503 + +lessee_admin_can_delete_their_allocation: + path: '/v1/allocations/{lessee_allocation}' + method: delete + headers: *lessee_admin_headers + assert_status: 503 + +owner_member_cannot_delete_their_allocation: + path: '/v1/allocations/{owner_allocation}' + method: delete + headers: *owner_member_headers + assert_status: 403 + +lessee_member_cannot_delete_their_allocation: + path: '/v1/allocations/{lessee_allocation}' + method: delete + headers: *lessee_member_headers + assert_status: 403 + +owner_member_can_patch_allocation: + path: '/v1/allocations/{owner_allocation}' + method: patch + headers: *owner_member_headers + body: &allocation_patch + - op: replace + path: /extra + value: {'test': 'testing'} + assert_status: 200 + +lessee_member_can_patch_allocation: + path: '/v1/allocations/{lessee_allocation}' + method: patch + headers: *lessee_member_headers + body: *allocation_patch + assert_status: 200 + +third_party_admin_can_get_allocations: path: '/v1/allocations' method: get headers: *third_party_admin_headers - assert_status: 403 - skip_reason: policy not implemented + assert_status: 200 + assert_list_length: + allocations: 0 -third_party_admin_cannot_create_allocation: +third_party_admin_can_create_allocation: + # This is distinctly different than most other behavior, + # should be applied to filter this, however this is also handled + # in the conductor, the only case where a user *should* be able + # to pass a UUID directly in though is a special case which + # should not be possible unless the user is the owner of the + # owner or lessee of the node. path: '/v1/allocations' method: post headers: *third_party_admin_headers body: &allocation_body resource_class: CUSTOM_TEST - assert_status: 403 - skip_reason: policy not implemented + assert_status: 503 + +third_party_admin_cannot_create_allocation_with_owner_node: + path: '/v1/allocations' + method: post + headers: *third_party_admin_headers + body: + resource_class: CUSTOM_TEST + node: 1ab63b9e-66d7-4cd7-8618-dddd0f9f7881 + assert_status: 400 + +third_party_admin_cannot_create_allocation_with_candidates_not_owned: + path: '/v1/allocations' + method: post + headers: *third_party_admin_headers + body: + resource_class: CUSTOM_TEST + candidate_nodes: + - 1ab63b9e-66d7-4cd7-8618-dddd0f9f7881 + - 38d5abed-c585-4fce-a57e-a2ffc2a2ec6f + assert_status: 400 + +owner_admin_can_create_allocation_with_their_uuid: + # NOTE(TheJulia): Owner/Lessee are equivelent in + # this context, so testing only one is fine. + path: '/v1/allocations' + method: post + headers: *owner_admin_headers + body: + resource_class: CUSTOM_TEST + node: 1ab63b9e-66d7-4cd7-8618-dddd0f9f7881 + assert_status: 503 third_party_admin_cannot_read_an_allocation: - path: '/v1/allocations/{allocation_ident}' + path: '/v1/allocations/{lessee_allocation}' method: get headers: *third_party_admin_headers - assert_status: 403 - skip_reason: policy not implemented + assert_status: 404 third_party_admin_cannot_patch_an_allocation: - path: '/v1/allocations/{allocation_ident}' + path: '/v1/allocations/{owner_allocation}' method: patch headers: *third_party_admin_headers body: - op: replace path: /extra value: {'test': 'testing'} - assert_status: 403 - skip_reason: policy not implemented + assert_status: 404 third_party_admin_cannot_delete_an_allocation: - path: '/v1/allocations/{allocation_ident}' + path: '/v1/allocations/{owner_allocation}' method: delete headers: *third_party_admin_headers - assert_status: 403 - skip_reason: policy not implemented + assert_status: 404 # Allocations ( Node level) - https://docs.openstack.org/api-ref/baremetal/#node-allocation-allocations-nodes @@ -2334,42 +2427,36 @@ owner_reader_can_read_node_allocation: method: get headers: *owner_reader_headers assert_status: 200 - skip_reason: policy not implemented lessee_reader_can_read_node_allocation: path: '/v1/nodes/{lessee_node_ident}/allocation' method: get headers: *lessee_reader_headers assert_status: 200 - skip_reason: policy not implemented third_party_admin_cannot_read_node_allocation: path: '/v1/nodes/{owner_node_ident}/allocation' method: get headers: *third_party_admin_headers assert_status: 404 - skip_reason: policy not implemented owner_admin_can_delete_allocation: path: '/v1/nodes/{owner_node_ident}/allocation' method: delete headers: *owner_admin_headers assert_status: 503 - skip_reason: policy not implemented -lessee_admin_cannot_delete_allocation: +lessee_admin_not_delete_allocation: path: '/v1/nodes/{allocated_node_ident}/allocation' method: delete headers: *lessee_admin_headers - assert_status: 403 - skip_reason: policy not implemented + assert_status: 503 third_party_admin_cannot_delete_allocation: path: '/v1/nodes/{allocated_node_ident}/allocation' method: delete headers: *third_party_admin_headers - assert_status: 403 - skip_reason: policy not implemented + assert_status: 404 # Deploy Templates - https://docs.openstack.org/api-ref/baremetal/#deploy-templates-deploy-templates diff --git a/releasenotes/notes/allocations-restricted-rbac-create-2847943150656432.yaml b/releasenotes/notes/allocations-restricted-rbac-create-2847943150656432.yaml new file mode 100644 index 0000000000..68e4b5bd0c --- /dev/null +++ b/releasenotes/notes/allocations-restricted-rbac-create-2847943150656432.yaml @@ -0,0 +1,13 @@ +--- +security: + - | + Ability to create an allocation has been restricted by a new policy rule + ``baremetal::allocation::create_pre_rbac`` which prevents creation of + allocations by any project administrator when operating with the new + Role Based Access Control model. The use and enforcement of this + rule is disabled when ``[oslo_policy]enforce_new_defaults`` is set which + also makes the population of a ``owner`` field for allocations to + become automatically populated. Most deployments should not encounter any + issues with this security change, and the policy rule will be removed + when support for the legacy ``baremetal_admin`` custom role has been + removed. diff --git a/tox.ini b/tox.ini index 73d0fbabcf..dea812fd37 100644 --- a/tox.ini +++ b/tox.ini @@ -127,7 +127,7 @@ filename = *.py,app.wsgi exclude = .venv,.git,.tox,dist,doc,*lib/python*,*egg,build import-order-style = pep8 application-import-names = ironic -max-complexity=18 +max-complexity=19 # [H106] Don't put vim configuration in source files. # [H203] Use assertIs(Not)None to check for None. # [H204] Use assert(Not)Equal to check for equality.