openstack/ironic
Code Issues Proposed changes

Merge "Allocation support for project scoped RBAC"

Browse Source
This commit is contained in:
Zuul 2021-03-15 19:15:34 +00:00 committed by Gerrit Code Review
commit dcfbd329c9
12 changed files with 461 additions and 85 deletions

27
doc/source/admin/secure-rbac.rst

@ -68,10 +68,12 @@ Supported Endpoints
* /nodes/<uuid>/portgroups
* /nodes/<uuid>/volume/connectors
* /nodes/<uuid>/volume/targets
* /nodes/<uuid>/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
--------------------

147
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/<uuid>/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") %

28
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

67
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
),
]

6
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]

64
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

16
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

15
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

10
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:

151
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

13
releasenotes/notes/allocations-restricted-rbac-create-2847943150656432.yaml Normal file

@ -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.

2
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.