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.