diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 9ea4b7a38b..55730915f4 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -2981,11 +2981,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) @@ -3051,20 +3049,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 f70c24093d..80889c12dd 100644 --- a/ironic/api/controllers/v1/port.py +++ b/ironic/api/controllers/v1/port.py @@ -689,12 +689,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 @@ -705,7 +702,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'): @@ -720,16 +720,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 ec1035e112..9c30a90006 100644 --- a/ironic/api/controllers/v1/portgroup.py +++ b/ironic/api/controllers/v1/portgroup.py @@ -472,29 +472,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 244940ab48..629cdcecba 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. @@ -1574,6 +1555,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 a94d4282ba..e7683d0630 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 e7f477c85e..7d50649c68 100644 --- a/ironic/api/controllers/v1/volume_target.py +++ b/ironic/api/controllers/v1/volume_target.py @@ -353,9 +353,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.OperationNotPermitted() @@ -369,29 +368,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/cmd/status.py b/ironic/cmd/status.py index 63cfcc746f..dc735cda84 100644 --- a/ironic/cmd/status.py +++ b/ironic/cmd/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.cmd 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 @@ -181,6 +183,38 @@ def _check_hardware_types_interfaces(self): else: return upgradecheck.Result(upgradecheck.Code.SUCCESS) + 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 @@ -199,6 +233,9 @@ def _check_hardware_types_interfaces(self): (common_checks.check_policy_json, {'conf': CONF})), (_('Hardware Types and Interfaces Check'), _check_hardware_types_interfaces), + # Cases of CVE-2026-44918 + (_('Mismatch between Child and Parent Owners'), + _check_parent_child_owners), ) diff --git a/ironic/conf/api.py b/ironic/conf/api.py index 450695a901..3e27c0b4fe 100644 --- a/ironic/conf/api.py +++ b/ironic/conf/api.py @@ -104,7 +104,7 @@ def __call__(self, value): help=_("Specifies a list of boot modes that are not allowed " "during enrollment. Eg: ['bios']")), cfg.ListOpt('disallow_deploy_steps', - default=[], + default=['vendor.send_raw'], mutable=True, help=_("List of steps not allowed across the deploy " "workflow. Each entry should be in 'interface.step' " @@ -112,7 +112,7 @@ def __call__(self, value): "Applies to user-requested steps, deploy template " "steps, and driver steps alike.")), cfg.ListOpt('disallow_service_steps', - default=[], + default=['vendor.send_raw'], mutable=True, help=_("List of steps not allowed across the service " "workflow. Each entry should be in 'interface.step' " @@ -121,7 +121,7 @@ def __call__(self, value): "Applies to user-requested steps and driver steps " "alike.")), cfg.ListOpt('disallow_clean_steps', - default=[], + default=['vendor.send_raw'], mutable=True, help=_("List of steps not allowed across the clean " "workflow. Each entry should be in 'interface.step' " diff --git a/ironic/conf/pxe.py b/ironic/conf/pxe.py index e84438a47e..8dbe2c2b12 100644 --- a/ironic/conf/pxe.py +++ b/ironic/conf/pxe.py @@ -135,11 +135,23 @@ cfg.DictOpt('pxe_bootfile_name_by_arch', default={}, help=_('Bootfile DHCP parameter per node architecture. ' - 'For example: aarch64:grubaa64.efi')), + 'Keys can be a cpu_arch value (e.g. aarch64) or a ' + 'composite key of arch-boot_mode (e.g. ' + 'x86_64-uefi, x86_64-bios). The composite key is ' + 'tried first, then the arch-only key as a ' + 'fallback. ' + 'For example: aarch64:grubaa64.efi or ' + 'x86_64-bios:pxelinux.0,x86_64-uefi:bootx64.efi')), cfg.DictOpt('ipxe_bootfile_name_by_arch', default={}, help=_('Bootfile DHCP parameter per node architecture. ' - 'For example: aarch64:ipxe_aa64.efi')), + 'Keys can be a cpu_arch value (e.g. aarch64) or a ' + 'composite key of arch-boot_mode (e.g. ' + 'x86_64-uefi, x86_64-bios). The composite key is ' + 'tried first, then the arch-only key as a ' + 'fallback. ' + 'For example: aarch64:ipxe_aa64.efi or ' + 'x86_64-bios:undionly.kpxe,x86_64-uefi:snponly.efi')), cfg.StrOpt('ipxe_boot_script', default=os.path.join( '$pybasedir', 'drivers/modules/boot.ipxe'), diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 2f06f4a520..612e0ec626 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/ironic/drivers/modules/deploy_utils.py @@ -400,16 +400,23 @@ def get_pxe_boot_file(node): """Return the PXE boot file name requested for deploy. This method returns PXE boot file name to be used for deploy. - Architecture specific boot file is searched first. BIOS/UEFI - boot file is used if no valid architecture specific file found. + Architecture and boot mode specific boot file is searched first + using a composite key of ``arch-boot_mode`` (e.g. ``x86_64-uefi``). + If not found, architecture-only key is tried for backward + compatibility. BIOS/UEFI boot file is used if no valid + architecture specific file found. :param node: A single Node. :returns: The PXE boot file name. """ cpu_arch = node.properties.get('cpu_arch') - boot_file = CONF.pxe.pxe_bootfile_name_by_arch.get(cpu_arch) + boot_mode = boot_mode_utils.get_boot_mode(node) + boot_file = CONF.pxe.pxe_bootfile_name_by_arch.get( + '%s-%s' % (cpu_arch, boot_mode)) if boot_file is None: - if boot_mode_utils.get_boot_mode(node) == 'uefi': + boot_file = CONF.pxe.pxe_bootfile_name_by_arch.get(cpu_arch) + if boot_file is None: + if boot_mode == 'uefi': boot_file = CONF.pxe.uefi_pxe_bootfile_name else: boot_file = CONF.pxe.pxe_bootfile_name @@ -421,8 +428,11 @@ def get_ipxe_boot_file(node): """Return the iPXE boot file name requested for deploy. This method returns iPXE boot file name to be used for deploy. - Architecture specific boot file is searched first. BIOS/UEFI - boot file is used if no valid architecture specific file found. + Architecture and boot mode specific boot file is searched first + using a composite key of ``arch-boot_mode`` (e.g. ``x86_64-bios``). + If not found, architecture-only key is tried for backward + compatibility. BIOS/UEFI boot file is used if no valid + architecture specific file found. If no valid value is found, the default reverts to the ``get_pxe_boot_file`` method and thus the @@ -433,9 +443,13 @@ def get_ipxe_boot_file(node): :returns: The iPXE boot file name. """ cpu_arch = node.properties.get('cpu_arch') - boot_file = CONF.pxe.ipxe_bootfile_name_by_arch.get(cpu_arch) + boot_mode = boot_mode_utils.get_boot_mode(node) + boot_file = CONF.pxe.ipxe_bootfile_name_by_arch.get( + '%s-%s' % (cpu_arch, boot_mode)) + if boot_file is None: + boot_file = CONF.pxe.ipxe_bootfile_name_by_arch.get(cpu_arch) if boot_file is None: - if boot_mode_utils.get_boot_mode(node) == 'uefi': + if boot_mode == 'uefi': boot_file = CONF.pxe.uefi_ipxe_bootfile_name else: boot_file = CONF.pxe.ipxe_bootfile_name diff --git a/ironic/tests/unit/api/controllers/v1/test_node.py b/ironic/tests/unit/api/controllers/v1/test_node.py index d7f0fe0050..9056cd1216 100644 --- a/ironic/tests/unit/api/controllers/v1/test_node.py +++ b/ironic/tests/unit/api/controllers/v1/test_node.py @@ -2843,6 +2843,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 1f12798749..4bf1414e65 100644 --- a/ironic/tests/unit/api/controllers/v1/test_utils.py +++ b/ironic/tests/unit/api/controllers/v1/test_utils.py @@ -931,24 +931,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/cmd/test_status.py b/ironic/tests/unit/cmd/test_status.py index 42ebe339c0..700e6f7787 100644 --- a/ironic/tests/unit/cmd/test_status.py +++ b/ironic/tests/unit/cmd/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.cmd 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/ironic/tests/unit/conductor/test_manager.py b/ironic/tests/unit/conductor/test_manager.py index d4b28f4778..5584b8e5e9 100644 --- a/ironic/tests/unit/conductor/test_manager.py +++ b/ironic/tests/unit/conductor/test_manager.py @@ -2174,6 +2174,30 @@ def test_do_node_deploy_disallowed_step_raises(self, mock_start, node.refresh() self.assertEqual(states.AVAILABLE, node.provision_state) + @mock.patch.object(deployments, 'start_deploy', autospec=True) + def test_do_node_deploy_disallowed_step_raises_send_raw( + self, mock_start, mock_iwdi): + mock_iwdi.return_value = False + self._start_service() + deploy_steps = [ + {'step': 'send_raw', + 'priority': 7, + 'interface': 'vendor'} + ] + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.AVAILABLE, + target_provision_state=states.NOSTATE) + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.do_node_deploy, + self.context, node.uuid, + deploy_steps=deploy_steps) + self.assertEqual(exception.StepNotAllowed, exc.exc_info[0]) + # start_deploy must NOT have been called + self.assertFalse(mock_start.called) + node.refresh() + self.assertEqual(states.AVAILABLE, node.provision_state) + @mock.patch.object(deployments, 'start_deploy', autospec=True) def test_do_node_deploy_allowed_step_proceeds(self, mock_start, mock_iwdi): @@ -3290,6 +3314,36 @@ def test_do_node_clean_disallowed_step_raises(self, mock_power_valid, # Node stays in original state self.assertEqual(states.MANAGEABLE, node.provision_state) + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + def test_do_node_clean_disallowed_step_raises_send_raw( + self, mock_power_valid, + mock_network_valid, + mock_process): + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.MANAGEABLE, + target_provision_state=states.NOSTATE) + self._start_service() + clean_steps = [ + {'step': 'send_raw', + 'priority': 7, + 'interface': 'vendor'} + ] + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.do_node_clean, + self.context, node.uuid, clean_steps) + self.assertEqual(exception.StepNotAllowed, exc.exc_info[0]) + # process_event must NOT have been called + self.assertFalse(mock_process.called) + node.refresh() + # Node stays in original state + self.assertEqual(states.MANAGEABLE, node.provision_state) + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', autospec=True) @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', @@ -3509,7 +3563,10 @@ def test_do_node_service(self, mock_pv, mock_nv, mock_event): target_provision_state=states.NOSTATE) self._start_service() self.service.do_node_service(self.context, - node.uuid, {'foo': 'bar'}) + node.uuid, + [{'step': 'foo', + 'priority': 7, + 'interface': 'management'}]) self.assertTrue(mock_pv.called) self.assertTrue(mock_nv.called) mock_event.assert_called_once_with( @@ -3517,7 +3574,8 @@ def test_do_node_service(self, mock_pv, mock_nv, mock_event): 'service', callback=mock.ANY, call_args=(servicing.do_node_service, mock.ANY, - {'foo': 'bar'}, False), + [{'step': 'foo', 'priority': 7, + 'interface': 'management'}], False), err_handler=mock.ANY, target_state='active') @mock.patch('ironic.conductor.manager.ConductorManager._spawn_worker', @@ -3565,6 +3623,33 @@ def test_do_node_service_disallowed_step_raises(self, mock_pv, mock_nv, node.refresh() self.assertEqual(states.ACTIVE, node.provision_state) + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', + autospec=True) + @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', + autospec=True) + @mock.patch('ironic.drivers.modules.fake.FakePower.validate', + autospec=True) + def test_do_node_service_disallowed_step_raises_on_send_raw( + self, mock_pv, mock_nv, + mock_event): + node = obj_utils.create_test_node( + self.context, driver='fake-hardware', + provision_state=states.ACTIVE, + target_provision_state=states.NOSTATE) + self._start_service() + service_steps = [ + {'step': 'send_raw', + 'priority': 7, + 'interface': 'vendor'} + ] + exc = self.assertRaises(messaging.rpc.ExpectedException, + self.service.do_node_service, + self.context, node.uuid, service_steps) + self.assertEqual(exception.StepNotAllowed, exc.exc_info[0]) + self.assertFalse(mock_event.called) + node.refresh() + self.assertEqual(states.ACTIVE, node.provision_state) + @mock.patch('ironic.conductor.task_manager.TaskManager.process_event', autospec=True) @mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.validate', diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 3b0a8d59ef..e7acf6452d 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -342,6 +342,34 @@ def test_get_pxe_boot_file_cpu_in_by_arch(self): result = utils.get_pxe_boot_file(self.node) self.assertEqual('aarch64-bootfile', result) + def test_get_pxe_boot_file_arch_boot_mode_composite_key(self): + bootfile_by_arch = {'x86_64-bios': 'pxelinux.0', + 'x86_64-uefi': 'bootx64.efi', + 'aarch64': 'grubaa64.efi'} + properties = {'cpu_arch': 'x86_64', 'capabilities': 'boot_mode:uefi'} + self.node.properties = properties + self.config(pxe_bootfile_name_by_arch=bootfile_by_arch, group='pxe') + result = utils.get_pxe_boot_file(self.node) + self.assertEqual('bootx64.efi', result) + + def test_get_pxe_boot_file_arch_boot_mode_composite_key_bios(self): + bootfile_by_arch = {'x86_64-bios': 'pxelinux.0', + 'x86_64-uefi': 'bootx64.efi'} + properties = {'cpu_arch': 'x86_64', 'capabilities': 'boot_mode:bios'} + self.node.properties = properties + self.config(pxe_bootfile_name_by_arch=bootfile_by_arch, group='pxe') + result = utils.get_pxe_boot_file(self.node) + self.assertEqual('pxelinux.0', result) + + def test_get_pxe_boot_file_arch_boot_mode_fallback_to_arch_only(self): + bootfile_by_arch = {'x86_64-bios': 'pxelinux.0', + 'aarch64': 'grubaa64.efi'} + properties = {'cpu_arch': 'aarch64', 'capabilities': 'boot_mode:uefi'} + self.node.properties = properties + self.config(pxe_bootfile_name_by_arch=bootfile_by_arch, group='pxe') + result = utils.get_pxe_boot_file(self.node) + self.assertEqual('grubaa64.efi', result) + def test_get_pxe_config_template_cpu_in_by_arch(self): properties = {'cpu_arch': 'aarch64', 'capabilities': 'boot_mode:uefi'} self.node.properties = properties @@ -392,6 +420,34 @@ def test_get_ipxe_boot_file_other_arch(self): result = utils.get_ipxe_boot_file(self.node) self.assertEqual('ipxe-aa64.efi', result) + def test_get_ipxe_boot_file_arch_boot_mode_composite_key(self): + arch_names = {'x86_64-bios': 'undionly.kpxe', + 'x86_64-uefi': 'snponly.efi', + 'aarch64': 'ipxe-aa64.efi'} + self.config(ipxe_bootfile_name_by_arch=arch_names, group='pxe') + properties = {'cpu_arch': 'x86_64', 'capabilities': 'boot_mode:uefi'} + self.node.properties = properties + result = utils.get_ipxe_boot_file(self.node) + self.assertEqual('snponly.efi', result) + + def test_get_ipxe_boot_file_arch_boot_mode_composite_key_bios(self): + arch_names = {'x86_64-bios': 'undionly.kpxe', + 'x86_64-uefi': 'snponly.efi'} + self.config(ipxe_bootfile_name_by_arch=arch_names, group='pxe') + properties = {'cpu_arch': 'x86_64', 'capabilities': 'boot_mode:bios'} + self.node.properties = properties + result = utils.get_ipxe_boot_file(self.node) + self.assertEqual('undionly.kpxe', result) + + def test_get_ipxe_boot_file_arch_boot_mode_fallback_to_arch_only(self): + arch_names = {'x86_64-bios': 'undionly.kpxe', + 'aarch64': 'ipxe-aa64.efi'} + self.config(ipxe_bootfile_name_by_arch=arch_names, group='pxe') + properties = {'cpu_arch': 'aarch64', 'capabilities': 'boot_mode:uefi'} + self.node.properties = properties + result = utils.get_ipxe_boot_file(self.node) + self.assertEqual('ipxe-aa64.efi', result) + def test_get_ipxe_boot_file_fallback(self): self.config(ipxe_bootfile_name=None, group='pxe') self.config(uefi_ipxe_bootfile_name=None, group='pxe') 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/releasenotes/notes/bootfile-by-arch-boot-mode-composite-key-2b0a10764f3b71a9.yaml b/releasenotes/notes/bootfile-by-arch-boot-mode-composite-key-2b0a10764f3b71a9.yaml new file mode 100644 index 0000000000..4000a874f2 --- /dev/null +++ b/releasenotes/notes/bootfile-by-arch-boot-mode-composite-key-2b0a10764f3b71a9.yaml @@ -0,0 +1,13 @@ +--- +fixes: + - | + The ``[pxe]pxe_bootfile_name_by_arch`` and + ``[pxe]ipxe_bootfile_name_by_arch`` options now support composite keys + in the format ``arch-boot_mode`` (e.g. ``x86_64-bios:undionly.kpxe``, + ``x86_64-uefi:snponly.efi``). The composite key is tried first, then + the arch-only key as a fallback, preserving backward compatibility. + Previously it was not possible to specify different boot files for the + same architecture in different boot modes (e.g. x86_64 in both UEFI + and BIOS), because the boot mode check was skipped when a by-arch + entry matched. + Fixes `bug 2157027 `_. diff --git a/releasenotes/notes/disable_send_raw-f92217bf07c3eb38.yaml b/releasenotes/notes/disable_send_raw-f92217bf07c3eb38.yaml new file mode 100644 index 0000000000..654bcab0af --- /dev/null +++ b/releasenotes/notes/disable_send_raw-f92217bf07c3eb38.yaml @@ -0,0 +1,20 @@ +--- +security: + - | + The IPMI Vendor-Passthru interface method ``send_raw`` step has been + disabled by default due to security implications. A malicious user with + sufficient Ironic access could utilzie this interface to make manual + changes to BMCs when the ``ipmitool`` vendor-passthru interface was + enabled. +fixes: + - | + Fixes a bug where ironic allowed the ability for operators to send + raw commands when the ``ipmitool`` vendor interface was enabled + and configured for a baremetal node utilizing the IPMI protocol. + + The Ironic project generally recommends the use of ``redfish`` + instead of IPMI, however recognizes that is not universally possible + for all operators. + + More information can be found in `bug 2150458 + `_. diff --git a/tox.ini b/tox.ini index bb8ac9446e..669d842fab 100644 --- a/tox.ini +++ b/tox.ini @@ -169,7 +169,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.