Synchronise 2025.1 with upstream

ironic/common/image_service.py

@@ -909,8 +909,9 @@ def validate_href(self, image_href):
             doesn't exist, is in a blocked path, or is not in an allowed path.
         :returns: Path to image file if it exists and is allowed.
         """
+        # TODO(TheJulia): Validate there are *THREE* slashes in the file path
+        # URL, otherwise urlparse doesn't split it properly.
         image_path = urlparse.urlparse(image_href).path
-
         # Check if the path is in the blocklist
         rpath = os.path.abspath(image_path)
 

ironic/drivers/modules/deploy_utils.py

@@ -1254,6 +1254,11 @@ def _validate_image_url(node, url, secret=False, inspect_image=None,
 def _cache_and_convert_image(task, instance_info, image_info=None):
     """Cache an image locally and convert it to RAW if needed.
 
+    Caches the supplied image as defined in the request, and converts it
+    to raw if required. This method should only be called once initial
+    first pass validation has been performed and can be called on multiple
+    code paths where the file contents must be downloaded.
+
     :param task: The Taskmanager object related to this action.
     :param instance_info: The instance_info field being used in
                           association with this method call.
@@ -1515,16 +1520,23 @@ def build_instance_info_for_deploy(task):
         di_info['image_source'] = image_source
         node.driver_internal_info = di_info
     if not is_glance_image:
-        if (image_source.startswith('file://')
-                or image_download_source == 'local'):
+        is_file_url = image_source.startswith('file://')
+        if (is_file_url or image_download_source == 'local'):
             # In this case, we're explicitly downloading (or copying a file)
             # hosted locally so IPA can download it directly from Ironic.
 
             # NOTE(TheJulia): Intentionally only supporting file:/// as image
             # based deploy source since we don't want to, nor should we be in
             # in the business of copying large numbers of files as it is a
             # huge performance impact.
-
+            if is_file_url:
+                # In this case, we need to validate the URL first before
+                # moving on to _cache_and_convert_image, because it's whole
+                # existence is to download, checksum, convert, etc.
+                image_service.FileImageService().validate_href(
+                    image_href=image_source)
+            # Either the file is local, or the file needs to be downloaded.
+            # _cache_and_convert_image handles both cases
             _cache_and_convert_image(task, instance_info)
         else:
             # This is the "all other cases" logic for aspects like the user

ironic/tests/unit/drivers/modules/test_deploy_utils.py

@@ -2664,6 +2664,54 @@ def test_build_instance_info_for_deploy_source_redirect_not_path(
             self.assertEqual('https://image-url/file',
                              task.node.instance_info['image_source'])
 
+    @mock.patch.object(os.path, 'isfile', autospec=True)
+    @mock.patch.object(utils, '_cache_and_convert_image', autospec=True)
+    def test_build_instance_info_for_deploy_file_url_valid(
+            self, mock_cache_image, mock_isfile):
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        i_info['image_source'] = 'file:///var/lib/ironic/files/foo/bar'
+        driver_internal_info['is_whole_disk_image'] = True
+        self.node.instance_info = i_info
+        self.node.driver_internal_info = driver_internal_info
+        self.node.save()
+        mock_isfile.return_value = True
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+
+            utils.build_instance_info_for_deploy(task)
+            mock_cache_image.assert_called_once_with(
+                mock.ANY,
+                {'configdrive': 'TG9yZW0gaXBzdW0gZG9sb3Igc2l0IGFtZXQ=',
+                 'image_url': None,
+                 'foo': 'bar',
+                 'image_source': 'file:///var/lib/ironic/files/foo/bar',
+                 'image_type': 'whole-disk'})
+
+    @mock.patch.object(utils, 'cache_instance_image', autospec=True)
+    def test_build_instance_info_for_deploy_file_url_invalid(
+            self, mock_cache_image):
+        mock_cache_image.return_value = ('fake', '/tmp/foo', 'qcow2')
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        url = 'file:///dev/zero'
+        i_info['image_source'] = url
+        self.node.instance_info = i_info
+        driver_internal_info['is_whole_disk_image'] = True
+        self.node.driver_internal_info = driver_internal_info
+        self.node.save()
+
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+            self.assertRaisesRegex(
+                exception.ImageRefValidationFailed,
+                'Validation of image href file:///dev/zero failed, reason: '
+                'Security: Path /dev/zero is not allowed for image source '
+                'file URLs',
+                utils.build_instance_info_for_deploy, task)
+
+            mock_cache_image.assert_not_called()
+
 
 class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
     def setUp(self):
@@ -2804,41 +2852,6 @@ def test_build_instance_info_already_raw_keeps_md5(self):
         self.assertEqual(instance_info['image_disk_format'], 'raw')
         self.checksum_mock.assert_not_called()
 
-    @mock.patch.object(image_service.HttpImageService, 'validate_href',
-                       autospec=True)
-    def test_build_instance_info_file_image(self, validate_href_mock):
-        i_info = self.node.instance_info
-        driver_internal_info = self.node.driver_internal_info
-        i_info['image_source'] = 'file://image-ref'
-        i_info['image_checksum'] = 'aa'
-        i_info['root_gb'] = 10
-        driver_internal_info['is_whole_disk_image'] = True
-        self.node.instance_info = i_info
-        self.node.driver_internal_info = driver_internal_info
-        self.node.save()
-
-        expected_url = (
-            'http://172.172.24.10:8080/agent_images/%s' % self.node.uuid)
-
-        with task_manager.acquire(
-                self.context, self.node.uuid, shared=False) as task:
-
-            info = utils.build_instance_info_for_deploy(task)
-
-            self.assertEqual(expected_url, info['image_url'])
-            self.assertEqual('sha256', info['image_os_hash_algo'])
-            self.assertEqual('fake-checksum', info['image_os_hash_value'])
-            self.assertEqual('raw', info['image_disk_format'])
-            self.cache_image_mock.assert_called_once_with(
-                task.context, task.node, force_raw=True,
-                expected_format=None,
-                expected_checksum='aa',
-                expected_checksum_algo=None)
-            self.checksum_mock.assert_called_once_with(
-                self.fake_path, algorithm='sha256')
-            validate_href_mock.assert_called_once_with(
-                mock.ANY, expected_url, False)
-
     @mock.patch.object(image_service.HttpImageService, 'validate_href',
                        autospec=True)
     def test_build_instance_info_local_image(self, validate_href_mock):

releasenotes/notes/fix-file-url-validation-5fb819ef5ae74ddc.yaml

@@ -0,0 +1,15 @@
+---
+fixes:
+  - |
+    Fixes an issue in the url handling logic of instance deploy logic where
+    file paths validity was not checked upfront, and the conductor service
+    would directly begin calculating checksums. This highlighted a DoS issue
+    where an attacker could exhaust conductor threads by attempting to request,
+    "file:///dev/zero", which would never return the thread. This is because
+    file URLs don't require downloading first, and validity checking logic
+    was all in the file access logic, not checksum logic.
+
+    Ironic now explicitly invokes the URL/validity checking logic prior to
+    calling the checksum logic which effectively prevents this issue.
+
+    This issue is tracked as CVE-2026-44919.

zuul.d/project.yaml

@@ -23,87 +23,22 @@
         - ironic-tempest-uefi-redfish-https
         - ironic-tempest-ovn-uefi-ipmi-pxe
         - ironic-tempest-ovn-uefi-ipxe-ipv6
-        # NOTE(TheJulia) Marking multinode non-voting on 20210311
-        # Due to a high failure rate on limestone where the compute1
-        # machine never appears to be able to communicate across the
-        # vxlan tunnel, possible mtu issue, but non-voting until we
-        # understand it in mind for the upcoming release.
-        - ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode:
-            voting: false
-        - ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode-shard:
-            voting: false
         - ironic-tempest-bios-ipmi-direct-tinyipa
         - ironic-tempest-bfv
         - ironic-tempest-ipa-partition-uefi-pxe-grub2
         - ironic-tempest-uefi-redfish-vmedia-4k
-        # NOTE(rpittau): Currently broken because of an issue with parted
-        - metalsmith-integration-glance-centos9-uefi:
-            voting: false
-        #####################################################################
-        # Grenade should be removed in advance of the unmaintained branches #
-        # as it doesn't know how to upgrade from an unmaintained branch.    #
-        #####################################################################
-        - ironic-grenade
-        - ironic-grenade-skip-level:
-            voting: false
-        ###############################################################
-        # CI Jobs Below this line may be *removed* on Stable Branches #
-        ###############################################################
-        # NOTE(TheJulia): At present, metal3 doesn't leverage
-        # stable branches, and as far as we are aware these jobs
-        # can be removed once this branch is made stable.
-        # - metal3-integration
-        # Non-voting jobs
-        - ironic-inspector-tempest:
-            voting: false
-        - ironic-inspector-tempest-uefi-redfish-vmedia:
-            voting: false
-        - ironic-standalone-aarch64:
-            voting: false
-        - ironic-tempest-ipa-wholedisk-bios-ipmi-direct-dib:
-            voting: false
-        - ironic-standalone-anaconda:
-            voting: false
-        - bifrost-integration-tinyipa-ubuntu-noble:
-            voting: false
-        - bifrost-integration-redfish-vmedia-uefi-centos-9:
-            voting: false
     gate:
       jobs:
         - ironic-tox-unit-mysql-migrations
         - ironic-tox-unit-with-driver-libs
         - ironic-tempest-functional-python3
-        - ironic-grenade
-        # NOTE(JayF): Disabling standalone jobs on 20240228 from voting, there's a
-        # dnsmasq bug only exposed on these jobs.
-        #- ironic-standalone-redfish
         - ironic-tempest-bios-redfish-pxe
         - ironic-tempest-uefi-redfish-vmedia
         - ironic-tempest-ramdisk-bios-snmp-pxe
         - ironic-tempest-uefi-redfish-https
         - ironic-tempest-ovn-uefi-ipmi-pxe
         - ironic-tempest-ovn-uefi-ipxe-ipv6
-        # NOTE(TheJulia): Disabled multinode on 20210311 due to Limestone
-        # seeming to be
-        # - ironic-tempest-ipa-wholedisk-direct-tinyipa-multinode
         - ironic-tempest-bios-ipmi-direct-tinyipa
         - ironic-tempest-bfv
         - ironic-tempest-ipa-partition-uefi-pxe-grub2
         - ironic-tempest-uefi-redfish-vmedia-4k
-        # NOTE(rpittau): Currently broken because of an issue with parted
-        #- metalsmith-integration-glance-centos9-uefi
-        # NOTE(TheJulia): At present, metal3 doesn't leverage
-        # stable branches, and as far as we are aware these jobs
-        # can be removed once this branch is made stable.
-        # - metal3-integration
-    experimental:
-      jobs:
-        # NOTE(dtantsur): this job is rarely used, no need to run it always.
-        - bifrost-benchmark-ironic:
-            voting: false
-        # TODO(dtantsur): these jobs are useful but currently hopelessly
-        # broken. Fix them and bring back to the gate.
-        - ironic-grenade-multinode-multitenant:
-            voting: false
-        - ironic-inspector-tempest-discovery-fast-track:
-            voting: false