From 52055c2e0f8877623e4235493bb08cc149a5e965 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Mon, 4 May 2026 17:14:07 +0200 Subject: [PATCH] CVE-2026-44918: Prevent rehoming resources to nodes with different owner Ironic has a few similar patterns where only the source node is verified during a create or update operation: 1. A node can be created with a parent from a different project 2. Node can be updated to have a parent from a different project 3. Ports, port groups, volume targets and volume connectors can be patched to be owned by a node from a different project. This patch addresses all these cases and introduces a single helper to cover all of them. Also makes sure that baremetal:node:get is checked even if the parent node ID is a UUID (it was missing before). Related-Bug: 2150450 Change-Id: Iaa512f380926a9fc032eefea5bcdcbc5040a4739 Signed-off-by: Julia Kreger Signed-off-by: Jay Faulkner (cherry picked from commit b58d4adc43551323b7ae8d5252636d796e32b949) --- ironic/api/controllers/v1/node.py | 30 ++-- ironic/api/controllers/v1/port.py | 18 +-- ironic/api/controllers/v1/portgroup.py | 21 +-- ironic/api/controllers/v1/utils.py | 76 +++++++--- ironic/api/controllers/v1/volume_connector.py | 21 +-- ironic/api/controllers/v1/volume_target.py | 31 ++--- ironic/command/status.py | 37 +++++ .../unit/api/controllers/v1/test_node.py | 9 ++ .../unit/api/controllers/v1/test_utils.py | 18 --- .../unit/api/test_rbac_project_scoped.yaml | 130 +++++++++++++++++- ironic/tests/unit/command/test_status.py | 56 ++++++++ .../2150450-owners-b4019d5fa63a8bc1.yaml | 21 +++ tox.ini | 2 +- 13 files changed, 342 insertions(+), 128 deletions(-) create mode 100644 releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 35a9bd5882..f98850d608 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -2980,11 +2980,9 @@ def post(self, node): chassis = _replace_chassis_uuid_with_id(node) chassis_uuid = chassis and chassis.uuid or None - if ('parent_node' in node - and not uuidutils.is_uuid_like(node['parent_node'])): - - parent_node = api_utils.check_node_policy_and_retrieve( - 'baremetal:node:get', node['parent_node']) + if node.get('parent_node'): + parent_node = api_utils.authorize_node_link( + node, node['parent_node'], 'parent_node') node['parent_node'] = parent_node.uuid new_node = objects.Node(context, **node) @@ -3050,20 +3048,6 @@ def _validate_patch(self, patch, reset_interfaces): for network_data in network_data_fields: validate_network_data(network_data) - parent_node = api_utils.get_patch_values(patch, '/parent_node') - if parent_node: - # At this point, if there *is* a parent_node, there is a value - # in the patch value list. - try: - # Verify we can see the parent node - req_parent_node = api_utils.check_node_policy_and_retrieve( - 'baremetal:node:get', parent_node[0]) - # If we can't see the node, an exception gets raised. - except Exception: - msg = _("Unable to apply the requested parent_node. " - "Requested value was invalid.") - raise exception.Invalid(msg) - corrected_values['parent_node'] = req_parent_node.uuid return corrected_values def _authorize_patch_and_get_node(self, node_ident, patch): @@ -3152,6 +3136,14 @@ def patch(self, node_ident, reset_interfaces=None, patch=None): context = api.request.context rpc_node = self._authorize_patch_and_get_node(node_ident, patch) + if parent_node := api_utils.authorize_node_link_patch( + rpc_node, patch, 'parent_node'): + # parent_node would be set to the UUID to verify that the request + # is authorized against the returned parent_node.uuid, and is then + # set as the authorized to node. IFF there is a mismatch, then + # we need to force override the parent node to the result. + if parent_node.uuid != rpc_node.uuid: + corrected_values['parent_node'] = parent_node.uuid remove_inst_uuid_patch = [{'op': 'remove', 'path': '/instance_uuid'}] if rpc_node.maintenance and patch == remove_inst_uuid_patch: diff --git a/ironic/api/controllers/v1/port.py b/ironic/api/controllers/v1/port.py index 69de6fde7b..511c5e7208 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -751,12 +751,9 @@ def patch(self, port_ident, patch): 'baremetal:port:update', port_ident) port_dict = rpc_port.as_dict() - # NOTE(lucasagomes): - # 1) Remove node_id because it's an internal value and + # NOTE(lucasagomes): Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid port_dict.pop('node_id', None) - port_dict['node_uuid'] = rpc_node.uuid # NOTE(vsaienko): # 1) Remove portgroup_id because it's an internal value and # not present in the API object @@ -767,7 +764,10 @@ def patch(self, port_ident, patch): context, port_dict.pop('portgroup_id')) port_dict['portgroup_uuid'] = portgroup and portgroup.uuid or None + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') port_dict = api_utils.apply_jsonpatch(port_dict, patch) + port_dict['node_uuid'] = rpc_node.uuid try: if api_utils.is_path_updated(patch, '/portgroup_uuid'): @@ -782,16 +782,6 @@ def patch(self, port_ident, patch): e.code = http_client.BAD_REQUEST # BadRequest raise - try: - if port_dict['node_uuid'] != rpc_node.uuid: - rpc_node = objects.Node.get( - api.request.context, port_dict['node_uuid']) - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a PATCH request to change a Port - e.code = http_client.BAD_REQUEST # BadRequest - raise - api_utils.patched_validate_with_schema( port_dict, PORT_PATCH_SCHEMA, PORT_PATCH_VALIDATOR) diff --git a/ironic/api/controllers/v1/portgroup.py b/ironic/api/controllers/v1/portgroup.py index 47da30e483..449ed69b3a 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -515,29 +515,20 @@ def patch(self, portgroup_ident, patch): portgroup_dict = rpc_portgroup.as_dict() - # NOTE: - # 1) Remove node_id because it's an internal value and + # NOTE: Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid portgroup_dict.pop('node_id') - portgroup_dict['node_uuid'] = rpc_node.uuid + + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') + portgroup_dict = api_utils.apply_jsonpatch(portgroup_dict, patch) + portgroup_dict['node_uuid'] = rpc_node.uuid if 'mode' not in portgroup_dict: msg = _("'mode' is a mandatory attribute and can not be removed") raise exception.ClientSideError(msg) - try: - if portgroup_dict['node_uuid'] != rpc_node.uuid: - rpc_node = objects.Node.get(api.request.context, - portgroup_dict['node_uuid']) - - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a POST request to patch a Portgroup - e.code = http_client.BAD_REQUEST # BadRequest - raise - api_utils.patched_validate_with_schema( portgroup_dict, PORTGROUP_PATCH_SCHEMA, PORTGROUP_PATCH_VALIDATOR) diff --git a/ironic/api/controllers/v1/utils.py b/ironic/api/controllers/v1/utils.py index 200c4213a3..a058f802d7 100644 --- a/ironic/api/controllers/v1/utils.py +++ b/ironic/api/controllers/v1/utils.py @@ -26,6 +26,7 @@ from jsonschema import exceptions as json_schema_exc import os_traits from oslo_config import cfg +from oslo_log import log from oslo_policy import policy as oslo_policy from oslo_utils import uuidutils from pecan import rest @@ -47,6 +48,8 @@ CONF = cfg.CONF +LOG = log.getLogger(__name__) + _JSONPATCH_EXCEPTIONS = (jsonpatch.JsonPatchConflict, jsonpatch.JsonPatchException, @@ -325,28 +328,6 @@ def replace_node_uuid_with_id(to_dict): return node -def replace_node_id_with_uuid(to_dict): - """Replace ``node_id`` dict value with ``node_uuid`` - - ``node_uuid`` is found by fetching the node by id lookup. - - :param to_dict: Dict to set ``node_uuid`` value on - :returns: The node object from the lookup - :raises: NodeNotFound with status_code set to 400 BAD_REQUEST - when node is not found. - """ - try: - node = objects.Node.get_by_id(api.request.context, - to_dict.pop('node_id')) - to_dict['node_uuid'] = node.uuid - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for requests acting on non-nodes - e.code = http_client.BAD_REQUEST # BadRequest - raise - return node - - def patch_update_changed_fields(from_dict, rpc_object, fields, schema, id_map=None): """Update rpc object based on changed fields in a dict. @@ -1613,6 +1594,57 @@ def check_policy_true(policy_name): return policy.check_policy(policy_name, cdict, api.request.context) +def authorize_node_link(orig_node, new_uuid, field_name): + """Authorize creating or changing a link to the node on a resource. + + :param orig_node: Node that currently owns the resource. + :param new_uuid: UUID of the new node. + :param field_name: Human-readable field name for logging and error message. + :raises: Invalid on access error + :returns: The new Node object + """ + msg = _("Unable to apply the requested %s '%s'. " + "Requested value was invalid.") + + try: + new_node = check_node_policy_and_retrieve( + 'baremetal:node:get', new_uuid) + except Exception as exc: + LOG.debug("Rejecting %s %s: %s", field_name, new_uuid, exc) + # Important: do not disclose if the node exists + raise exception.Invalid(msg % (field_name, new_uuid)) + + if isinstance(orig_node, objects.Node): + orig_node = orig_node.as_dict() # adjust for different callers + + if orig_node.get('owner') != new_node.owner: + LOG.warning("Project mismatch on setting or changing %(field)s: " + "current owner '%(orig)s', new %(field)s owner '%(new)s'", + {'orig': orig_node.get('owner'), 'new': new_node.owner, + 'field': field_name}) + # Important: same error message as when the node does not exist + raise exception.Invalid(msg % (field_name, new_uuid)) + + return new_node + + +def authorize_node_link_patch(orig_node, patch, field_name): + """Authorize changing a link to the node on a resource. + + :param orig_node: Node that currently owns the resource. + :param patch: JSON patch being applied. + :param field_name: Field names that contains the link. + :raises: Invalid on access error + :returns: The new Node object or the old one if not changed + """ + new_node_id = get_patch_values(patch, f'/{field_name}') + if new_node_id: + return authorize_node_link( + orig_node, new_node_id[0], field_name) + + return orig_node + + def duplicate_steps(name, value): """Argument validator to check template for duplicate steps""" # TODO(mgoddard): Determine the consequences of allowing duplicate diff --git a/ironic/api/controllers/v1/volume_connector.py b/ironic/api/controllers/v1/volume_connector.py index 47df13021b..79ac61b135 100644 --- a/ironic/api/controllers/v1/volume_connector.py +++ b/ironic/api/controllers/v1/volume_connector.py @@ -332,23 +332,16 @@ def patch(self, connector_uuid, patch): raise exception.InvalidUUID(message=message) connector_dict = rpc_connector.as_dict() - # NOTE(smoriya): - # 1) Remove node_id because it's an internal value and + # NOTE(smoriya): Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid - rpc_node = api_utils.replace_node_id_with_uuid(connector_dict) + connector_dict.pop('node_id', None) + # NOTE(dtantsur): Patch won't apply if the field does not exist. + connector_dict['node_uuid'] = None + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') connector_dict = api_utils.apply_jsonpatch(connector_dict, patch) - - try: - if connector_dict['node_uuid'] != rpc_node.uuid: - rpc_node = objects.Node.get( - api.request.context, connector_dict['node_uuid']) - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a PATCH request to change a Port - e.code = http_client.BAD_REQUEST # BadRequest - raise + connector_dict['node_uuid'] = rpc_node.uuid api_utils.patched_validate_with_schema( connector_dict, CONNECTOR_SCHEMA, CONNECTOR_VALIDATOR) diff --git a/ironic/api/controllers/v1/volume_target.py b/ironic/api/controllers/v1/volume_target.py index e24d74d915..65bb286380 100644 --- a/ironic/api/controllers/v1/volume_target.py +++ b/ironic/api/controllers/v1/volume_target.py @@ -345,9 +345,8 @@ def patch(self, target_uuid, patch): """ context = api.request.context - api_utils.check_volume_policy_and_retrieve('baremetal:volume:update', - target_uuid, - target=True) + rpc_target, rpc_node = api_utils.check_volume_policy_and_retrieve( + 'baremetal:volume:update', target_uuid, target=True) if self.parent_node_ident: raise exception.MalformedRequestURI() @@ -361,29 +360,17 @@ def patch(self, target_uuid, patch): "%(uuid)s.") % {'uuid': str(value)} raise exception.InvalidUUID(message=message) - rpc_target = objects.VolumeTarget.get_by_uuid(context, target_uuid) target_dict = rpc_target.as_dict() - # NOTE(smoriya): - # 1) Remove node_id because it's an internal value and + # NOTE(smoriya): Remove node_id because it's an internal value and # not present in the API object - # 2) Add node_uuid - rpc_node = api_utils.replace_node_id_with_uuid(target_dict) + target_dict.pop('node_id', None) + # NOTE(dtantsur): Patch won't apply if the field does not exist. + target_dict['node_uuid'] = None + rpc_node = api_utils.authorize_node_link_patch( + rpc_node, patch, 'node_uuid') target_dict = api_utils.apply_jsonpatch(target_dict, patch) - - try: - if target_dict['node_uuid'] != rpc_node.uuid: - - # TODO(TheJulia): I guess the intention is to - # permit the mapping to be changed - # should we even allow this at all? - rpc_node = objects.Node.get( - api.request.context, target_dict['node_uuid']) - except exception.NodeNotFound as e: - # Change error code because 404 (NotFound) is inappropriate - # response for a PATCH request to change a volume target - e.code = http_client.BAD_REQUEST # BadRequest - raise + target_dict['node_uuid'] = rpc_node.uuid api_utils.patched_validate_with_schema( target_dict, TARGET_SCHEMA, TARGET_VALIDATOR) diff --git a/ironic/command/status.py b/ironic/command/status.py index a15c1f2edb..12c3fbc45d 100644 --- a/ironic/command/status.py +++ b/ironic/command/status.py @@ -20,6 +20,7 @@ from oslo_upgradecheck import common_checks from oslo_upgradecheck import upgradecheck import sqlalchemy +from sqlalchemy import orm from ironic.command import dbsync from ironic.common import driver_factory @@ -27,6 +28,7 @@ from ironic.common import policy # noqa importing to load policy config. import ironic.conf from ironic.db import api as db_api +from ironic.db.sqlalchemy import models CONF = ironic.conf.CONF @@ -220,6 +222,38 @@ def _check_ilo_driver_usage(self): ) return upgradecheck.Result(upgradecheck.Code.WARNING, details=msg) + def _check_parent_child_owners(self): + engine = enginefacade.reader.get_engine() + parent_node = orm.aliased(models.Node) + broken = [] + with engine.connect() as conn, conn.begin(): + # NOTE(dtantsur): since NULL handling in SQL is insane, we need to + # handle 3 possible cases separately. + conditions = ( + (parent_node.owner != models.Node.owner) + | (models.Node.owner.is_not(None) + & parent_node.owner.is_(None)) + | (models.Node.owner.is_(None) + & parent_node.owner.is_not(None)) + ) + query = (sqlalchemy.select(models.Node.uuid, + models.Node.owner, + parent_node.uuid, + parent_node.owner) + .join(parent_node, + models.Node.parent_node == parent_node.uuid) + .where(conditions)) + res = conn.execute(query) + for child_uuid, child_owner, parent_uuid, parent_owner in res: + broken.append(f"- {child_uuid}: owner {child_owner}, " + f"parent {parent_uuid} owner {parent_owner}") + if broken: + msg = ("Some nodes have an owner mismatch with parents:\n" + + "\n".join(broken)) + return upgradecheck.Result(upgradecheck.Code.WARNING, details=msg) + else: + return upgradecheck.Result(upgradecheck.Code.SUCCESS) + # A tuple of check tuples of (, ). # The name of the check will be used in the output of this command. # The check function takes no arguments and returns an @@ -240,6 +274,9 @@ def _check_ilo_driver_usage(self): _check_hardware_types_interfaces), (_('iLO/iLO5 Driver Usage Check'), _check_ilo_driver_usage), + # Cases of CVE-2026-44918 + (_('Mismatch between Child and Parent Owners'), + _check_parent_child_owners), ) diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index 0a9d626035..eea46947a9 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -2862,6 +2862,15 @@ def test_update_ok(self, mock_notify): timeutils.parse_isotime(response.json['updated_at'])) self.mock_update_node.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, 'test-topic', None) + # NOTE(TheJulia) As we save a hydrated database object back, we need + # to double check what gets saved back out to the database to validate + # handling is proper. + updated_node = self.mock_update_node.call_args.args[2] + self.assertEqual(updated_node.instance_uuid, + 'aaaaaaaa-1111-bbbb-2222-cccccccccccc') + # NOTE(TheJulia): Checking parent node here is to ensure we don't + # accidentally regress with auto-setting logic. + self.assertIsNone(updated_node.parent_node) mock_notify.assert_has_calls([mock.call(mock.ANY, mock.ANY, 'update', obj_fields.NotificationLevel.INFO, obj_fields.NotificationStatus.START, diff --git a/ironic/tests/unit/api/controllers/v1/test_utils.py b/ironic/tests/unit/api/controllers/v1/test_utils.py index 1d7ed547fb..bd3866ca8c 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/ironic/tests/unit/api/controllers/v1/test_utils.py @@ -1019,24 +1019,6 @@ def test_replace_node_uuid_with_id_not_found(self, mock_gbu, mock_pr): utils.replace_node_uuid_with_id, to_dict) self.assertEqual(400, e.code) - @mock.patch.object(objects.Node, 'get_by_id', autospec=True) - def test_replace_node_id_with_uuid(self, mock_gbi, mock_pr): - node = obj_utils.get_test_node(self.context, uuid=self.valid_uuid) - mock_gbi.return_value = node - to_dict = {'node_id': 1} - - self.assertEqual(node, utils.replace_node_id_with_uuid(to_dict)) - self.assertEqual({'node_uuid': self.valid_uuid}, to_dict) - - @mock.patch.object(objects.Node, 'get_by_id', autospec=True) - def test_replace_node_id_with_uuid_not_found(self, mock_gbi, mock_pr): - to_dict = {'node_id': 1} - mock_gbi.side_effect = exception.NodeNotFound(node=1) - - e = self.assertRaises(exception.NodeNotFound, - utils.replace_node_id_with_uuid, to_dict) - self.assertEqual(400, e.code) - class TestVendorPassthru(base.TestCase): diff --git a/ironic/tests/unit/api/test_rbac_project_scoped.yaml b/ironic/tests/unit/api/test_rbac_project_scoped.yaml index 32782b1cf5..6f5c4606a5 100644 --- a/ironic/tests/unit/api/test_rbac_project_scoped.yaml +++ b/ironic/tests/unit/api/test_rbac_project_scoped.yaml @@ -2468,6 +2468,30 @@ third_party_admin_cannot_modify_portgroup: body: *portgroup_patch_body assert_status: 404 +owner_admin_cannot_rehome_portgroup: + path: '/v1/portgroups/{owner_portgroup_ident}' + method: patch + headers: *owner_admin_headers + body: &other_node_portgroup_patch_body + - op: replace + path: /node_uuid + value: 573208e5-cd41-4e26-8f06-ef44022b3793 + assert_status: 400 + +owner_manager_cannot_rehome_portgroup: + path: '/v1/portgroups/{owner_portgroup_ident}' + method: patch + headers: *owner_manager_headers + body: *other_node_portgroup_patch_body + assert_status: 400 + +owner_service_cannot_rehome_portgroup: + path: '/v1/portgroups/{owner_portgroup_ident}' + method: patch + headers: *service_headers_owner_project + body: *other_node_portgroup_patch_body + assert_status: 400 + owner_admin_can_delete_portgroup: path: '/v1/portgroups/{owner_portgroup_ident}' method: delete @@ -2768,6 +2792,30 @@ owner_admin_can_delete_port: headers: *owner_admin_headers assert_status: 503 +owner_admin_cannot_rehome_port: + path: '/v1/ports/{owner_port_ident}' + method: patch + headers: *owner_admin_headers + body: &other_node_port_patch_body + - op: replace + path: /node_uuid + value: 573208e5-cd41-4e26-8f06-ef44022b3793 + assert_status: 400 + +owner_manager_cannot_rehome_port: + path: '/v1/ports/{owner_port_ident}' + method: patch + headers: *owner_manager_headers + body: *other_node_port_patch_body + assert_status: 400 + +owner_service_cannot_rehome_port: + path: '/v1/ports/{owner_port_ident}' + method: patch + headers: *service_headers_owner_project + body: *other_node_port_patch_body + assert_status: 400 + owner_manager_can_delete_port: path: '/v1/ports/{owner_port_ident}' method: delete @@ -3044,6 +3092,30 @@ third_party_admin_cannot_patch_volume_connectors: body: *connector_patch_body assert_status: 404 +owner_admin_cannot_rehome_volume_connector: + path: '/v1/volume/connectors/{volume_connector_ident}' + method: patch + headers: *owner_admin_headers + body: &other_node_connector_patch_body + - op: replace + path: /node_uuid + value: 573208e5-cd41-4e26-8f06-ef44022b3793 + assert_status: 400 + +owner_manager_cannot_rehome_volume_connector: + path: '/v1/volume/connectors/{volume_connector_ident}' + method: patch + headers: *owner_manager_headers + body: *other_node_connector_patch_body + assert_status: 400 + +owner_service_cannot_rehome_volume_connector: + path: '/v1/volume/connectors/{volume_connector_ident}' + method: patch + headers: *service_headers_owner_project + body: *other_node_connector_patch_body + assert_status: 400 + owner_admin_can_delete_volume_connectors: path: '/v1/volume/connectors/{volume_connector_ident}' method: delete @@ -3245,6 +3317,30 @@ service_cannot_patch_volume_target: headers: *service_headers assert_status: 404 +owner_admin_cannot_rehome_volume_target: + path: '/v1/volume/targets/{volume_target_ident}' + method: patch + headers: *owner_admin_headers + body: &other_node_target_patch_body + - op: replace + path: /node_uuid + value: 573208e5-cd41-4e26-8f06-ef44022b3793 + assert_status: 400 + +owner_manager_cannot_rehome_volume_target: + path: '/v1/volume/targets/{volume_target_ident}' + method: patch + headers: *owner_manager_headers + body: *other_node_target_patch_body + assert_status: 400 + +owner_service_cannot_rehome_volume_target: + path: '/v1/volume/targets/{volume_target_ident}' + method: patch + headers: *service_headers_owner_project + body: *other_node_target_patch_body + assert_status: 400 + owner_admin_can_delete_volume_target: path: '/v1/volume/targets/{volume_target_ident}' method: delete @@ -3962,6 +4058,35 @@ shard_patch_set_node_shard_disallowed: value: 'TestShard' assert_status: 403 +# Create node with parent_node field + +admin_can_create_node_with_parent: + path: '/v1/nodes' + method: post + headers: *owner_admin_headers + body: + driver: fake + parent_node: *owned_node_ident + assert_status: 503 + +admin_cannot_create_node_with_non_existing_parent: + path: '/v1/nodes' + method: post + headers: *owner_admin_headers + body: + driver: fake + parent_node: 'f11853c7-fa9c-4db3-a477-c9d8e0dbbf13' + assert_status: 400 + +admin_cannot_create_node_with_parent_they_cannot_see: + path: '/v1/nodes' + method: post + headers: *owner_admin_headers + body: + driver: fake + parent_node: '{node_ident}' + assert_status: 400 + # Update node parent_node field - baremetal:node:update:parent_node parent_node_patch_by_admin: @@ -3996,8 +4121,7 @@ parent_node_patch_by_manager: assert_status: 403 parent_node_patch_by_cannot_see_node: - # This node cannot be seen, and also just doesn't exist. - # Just to verify we return a 400 on a node we can change. + # Avoid exposing whether the node exists if not authorized path: '/v1/nodes/{lessee_node_ident}' method: patch headers: *owner_admin_headers @@ -4005,7 +4129,7 @@ parent_node_patch_by_cannot_see_node: - op: replace path: /parent_node value: 'f11853c7-fa9c-4db3-a477-c9d8e0dbbf13' - assert_status: 400 + assert_status: 403 parent_node_children_can_get_list_of_children: path: '/v1/nodes/{owner_node_ident}/children' diff --git a/ironic/tests/unit/command/test_status.py b/ironic/tests/unit/command/test_status.py index 80e7bac1d2..1eb42a5a8c 100644 --- a/ironic/tests/unit/command/test_status.py +++ b/ironic/tests/unit/command/test_status.py @@ -16,6 +16,7 @@ from oslo_db import sqlalchemy from oslo_upgradecheck.upgradecheck import Code +from oslo_utils import uuidutils from sqlalchemy.engine import url as sa_url from ironic.command import dbsync @@ -146,3 +147,58 @@ def test__check_allocations_table_myiasm_both(self, mock_reader): 'table engine to utilize InnoDB, and reload the ' 'allocations table to utilize the InnoDB engine.') self.assertEqual(expected_msg, check_result.details) + + +class TestUpgradeChecksOwnerMismatch(db_base.DbTestCase): + + def setUp(self): + super().setUp() + self.cmd = status.Checks() + + def test_correct_nodes(self): + parents = [ + # Parent without an owner + {'uuid': uuidutils.generate_uuid()}, + # Parent with an owner + {'uuid': uuidutils.generate_uuid(), + 'owner': 'abcd'}, + ] + children = [ + {'parent_node': parents[0]['uuid']}, + {'parent_node': parents[1]['uuid'], + 'owner': parents[1]['owner']}, + ] + for node in parents + children: + self.dbapi.create_node(node) + + result = self.cmd._check_parent_child_owners() + self.assertEqual(Code.SUCCESS, result.code) + + def test_mismatch(self): + parents = [ + # Parent without an owner + {'uuid': uuidutils.generate_uuid()}, + # Parent with an owner + {'uuid': uuidutils.generate_uuid(), + 'owner': 'abcd'}, + ] + children = [ + # Owned child of a parent without owner + {'uuid': uuidutils.generate_uuid(), + 'parent_node': parents[0]['uuid'], + 'owner': 'abcd'}, + # Child of an owned node without owner + {'uuid': uuidutils.generate_uuid(), + 'parent_node': parents[1]['uuid']}, + # Mismatched owners + {'uuid': uuidutils.generate_uuid(), + 'parent_node': parents[1]['uuid'], + 'owner': parents[1]['owner'] + 'bad'}, + ] + for node in parents + children: + self.dbapi.create_node(node) + + result = self.cmd._check_parent_child_owners() + self.assertEqual(Code.WARNING, result.code) + for child in children: + self.assertIn(child['uuid'], result.details) diff --git a/releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml b/releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml new file mode 100644 index 0000000000..9d7e04e0de --- /dev/null +++ b/releasenotes/notes/2150450-owners-b4019d5fa63a8bc1.yaml @@ -0,0 +1,21 @@ +--- +security: + - | + Addresses a potential security issues where an admin of a project could + create nodes with ``parent_node`` set to a node from a different project. + - | + Prevents changing the ``node_uuid`` of ports, port groups, volume targets, + and volume connectors to point to a node with a different owner from the + initial node. In some cases this is not normally permitted due to the + database model, but additional access checking was added across these + similar resources for consistency in the event the Ironic project fixes + `12150252 `_. +issues: + - | + System operators should note that changing the owner field on a node does + not affect its child or parent nodes, potentially resulting in these nodes + being in different projects. +upgrade: + - | + Adds an upgrade check that issues a warning when any node has a different + owner from its parent node. diff --git a/tox.ini b/tox.ini index c1ced0b895..dca639a016 100644 --- a/tox.ini +++ b/tox.ini @@ -168,7 +168,7 @@ filename = *.py,app.wsgi exclude=.*,dist,doc,*lib/python*,*egg,build import-order-style = pep8 application-import-names = ironic -max-complexity=20 +max-complexity=21 # [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.