Merge "Checksum files before raw conversion" into stable/2024.1

doc/source/admin/security.rst

@@ -19,6 +19,40 @@ OpenStack deployment.
 
 .. TODO: add "Multi-tenancy Considerations" section
 
+Image Checksums
+===============
+
+Ironic has long provided a capacity to supply and check a checksum for disk
+images being deployed. However, one aspect which Ironic has not asserted is
+"Why?" in terms of "Is it for security?" or "Is it for data integrity?".
+
+The answer is both to ensure a higher level of security with remote
+image files, *and* provide faster feedback should a image being transferred
+happens to be corrupted.
+
+Normally checksums are verified by the ``ironic-python-agent`` **OR** the
+deployment interface responsible for overall deployment operation. That being
+said, not *every* deployment interface relies on disk images which have
+checksums, and those deployment interfaces are for specific use cases which
+Ironic users leverage, outside of the "general" use case capabilities provided
+by the ``direct`` deployment interface.
+
+.. NOTE::
+   Use of the node ``instance_info/image_checksum`` field is discouraged
+   for integrated OpenStack Users as usage of the matching Glance Image
+   Service field is also deprecated. That being said, Ironic retains this
+   feature by popular demand while also enabling also retain simplified
+   operator interaction.
+   The newer field values supported by Glance are also specifically
+   supported by Ironic as ``instance_info/image_os_hash_value`` for
+   checksum values and ``instance_info/image_os_hash_algo`` field for
+   the checksum algorithm.
+
+.. WARNING::
+   Setting a checksum value to a URL is supported, *however* doing this is
+   making a "tradeoff" with security as the remote checksum *can* change.
+   Conductor support this functionality can be disabled using the
+   :oslo.config:option:`conductor.disable_support_for_checksum_files` setting.
 
 REST API: user roles and policy settings
 ========================================

ironic/common/checksum_utils.py

@@ -0,0 +1,258 @@
+# Copyright (c) 2024 Red Hat, Inc.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import os
+import re
+import time
+from urllib import parse as urlparse
+
+from oslo_log import log as logging
+from oslo_utils import fileutils
+
+from ironic.common import exception
+from ironic.common.i18n import _
+from ironic.common import image_service
+from ironic.conf import CONF
+
+LOG = logging.getLogger(__name__)
+
+
+# REGEX matches for Checksum file payloads
+# If this list requires changes, it should be changed in
+# ironic-python-agent (extensions/standby.py) as well.
+
+MD5_MATCH = r"^([a-fA-F\d]{32})\s"  # MD5 at beginning of line
+MD5_MATCH_END = r"\s([a-fA-F\d]{32})$"  # MD5 at end of line
+MD5_MATCH_ONLY = r"^([a-fA-F\d]{32})$"  # MD5 only
+SHA256_MATCH = r"^([a-fA-F\d]{64})\s"  # SHA256 at beginning of line
+SHA256_MATCH_END = r"\s([a-fA-F\d]{64})$"  # SHA256 at end of line
+SHA256_MATCH_ONLY = r"^([a-fA-F\d]{64})$"  # SHA256 only
+SHA512_MATCH = r"^([a-fA-F\d]{128})\s"  # SHA512 at beginning of line
+SHA512_MATCH_END = r"\s([a-fA-F\d]{128})$"  # SHA512 at end of line
+SHA512_MATCH_ONLY = r"^([a-fA-F\d]{128})$"  # SHA512 only
+FILENAME_MATCH_END = r"\s[*]?{filename}$"  # Filename binary/text end of line
+FILENAME_MATCH_PARENTHESES = r"\s\({filename}\)\s"  # CentOS images
+
+CHECKSUM_MATCHERS = (MD5_MATCH, MD5_MATCH_END, SHA256_MATCH, SHA256_MATCH_END,
+                     SHA512_MATCH, SHA512_MATCH_END)
+CHECKSUM_ONLY_MATCHERS = (MD5_MATCH_ONLY, SHA256_MATCH_ONLY, SHA512_MATCH_ONLY)
+FILENAME_MATCHERS = (FILENAME_MATCH_END, FILENAME_MATCH_PARENTHESES)
+
+
+def validate_checksum(path, checksum, checksum_algo=None):
+    """Validate image checksum.
+
+    :param path: File path in the form of a string to calculate a checksum
+                 which is compared to the checksum field.
+    :param checksum: The supplied checksum value, a string, which will be
+                     compared to the file.
+    :param checksum_algo: The checksum type of the algorithm.
+    :raises: ImageChecksumError if the supplied data cannot be parsed or
+             if the supplied value does not match the supplied checksum
+             value.
+    """
+    # TODO(TheJilia): At some point, we likely need to compare
+    # the incoming checksum algorithm upfront, ut if one is invoked which
+    # is not supported, hashlib will raise ValueError.
+    use_checksum_algo = None
+    if ":" in checksum:
+        # A form of communicating the checksum algorithm is to delimit the
+        # type from the value. See ansible deploy interface where this
+        # is most evident.
+        split_checksum = checksum.split(":")
+        use_checksum = split_checksum[1]
+        use_checksum_algo = split_checksum[0]
+    else:
+        use_checksum = checksum
+    if not use_checksum_algo:
+        use_checksum_algo = checksum_algo
+    # If we have a zero length value, but we split it, we have
+    # invalid input. Also, checksum is what we expect, algorithm is
+    # optional. This guards against the split of a value which is
+    # image_checksum = "sha256:" which is a potential side effect of
+    # splitting the string.
+    if use_checksum == '':
+        raise exception.ImageChecksumError()
+
+    # Make everything lower case since we don't expect mixed case,
+    # but we may have human originated input on the supplied algorithm.
+    try:
+        if not use_checksum_algo:
+            # This is backwards compatible support for a bare checksum.
+            calculated = compute_image_checksum(path)
+        else:
+            calculated = compute_image_checksum(path,
+                                                use_checksum_algo.lower())
+    except ValueError:
+        # ValueError is raised when an invalid/unsupported/unknown
+        # checksum algorithm is invoked.
+        LOG.error("Failed to generate checksum for file %(path)s, possible "
+                  "invalid checksum algorithm: %(algo)s",
+                  {"path": path,
+                   "algo": use_checksum_algo})
+        raise exception.ImageChecksumAlgorithmFailure()
+    except OSError:
+        LOG.error("Failed to read file %(path)s to compute checksum.",
+                  {"path": path})
+        raise exception.ImageChecksumFileReadFailure()
+    if (use_checksum is not None
+        and calculated.lower() != use_checksum.lower()):
+        LOG.error("We were supplied a checksum value of %(supplied)s, but "
+                  "calculated a value of %(value)s. This is a fatal error.",
+                  {"supplied": use_checksum,
+                   "value": calculated})
+        raise exception.ImageChecksumError()
+
+
+def compute_image_checksum(image_path, algorithm='md5'):
+    """Compute checksum by given image path and algorithm.
+
+    :param image_path: The path to the file to undergo checksum calculation.
+    :param algorithm: The checksum algorithm to utilize. Defaults
+        to 'md5' due to historical support reasons in Ironic.
+    :returns: The calculated checksum value.
+    :raises: ValueError when the checksum algorithm is not supported
+       by the system.
+    """
+
+    time_start = time.time()
+    LOG.debug('Start computing %(algo)s checksum for image %(image)s.',
+              {'algo': algorithm, 'image': image_path})
+
+    checksum = fileutils.compute_file_checksum(image_path,
+                                               algorithm=algorithm)
+    time_elapsed = time.time() - time_start
+    LOG.debug('Computed %(algo)s checksum for image %(image)s in '
+              '%(delta).2f seconds, checksum value: %(checksum)s.',
+              {'algo': algorithm, 'image': image_path, 'delta': time_elapsed,
+               'checksum': checksum})
+    return checksum
+
+
+def get_checksum_and_algo(instance_info):
+    """Get and return the image checksum and algo.
+
+    :param instance_info: The node instance info, or newly updated/generated
+                          instance_info value.
+    :returns: A tuple containing two values, a checksum and algorithm,
+              if available.
+    """
+    checksum_algo = None
+    if 'image_os_hash_value' in instance_info.keys():
+        # A value set by image_os_hash_value supersedes other
+        # possible uses as it is specific.
+        checksum = instance_info.get('image_os_hash_value')
+        checksum_algo = instance_info.get('image_os_hash_algo')
+    else:
+        checksum = instance_info.get('image_checksum')
+        if is_checksum_url(checksum):
+            image_source = instance_info.get('image_source')
+            checksum = get_checksum_from_url(checksum, image_source)
+
+        # NOTE(TheJulia): This is all based on SHA-2 lengths.
+        # SHA-3 would require a hint and it would not be a fixed length.
+        # That said, SHA-2 is still valid and has not been withdrawn.
+        checksum_len = len(checksum)
+        if checksum_len == 128:
+            # SHA2-512 is 512 bits, 128 characters.
+            checksum_algo = "sha512"
+        elif checksum_len == 64:
+            checksum_algo = "sha256"
+
+        if checksum_len == 32 and not CONF.agent.allow_md5_checksum:
+            # MD5 not permitted and the checksum is the length of MD5
+            # and not otherwise defined.
+            LOG.error('Cannot compute the checksum as it uses MD5 '
+                      'and is disabled by configuration. If the checksum '
+                      'is *not* MD5, please specify the algorithm.')
+            raise exception.ImageChecksumAlgorithmFailure()
+
+    return checksum, checksum_algo
+
+
+def is_checksum_url(checksum):
+    """Identify if checksum is not a url.
+
+    :param checksum: The user supplied checksum value.
+    :returns: True if the checksum is a url, otherwise False.
+    :raises: ImageChecksumURLNotSupported should the conductor have this
+             support disabled.
+    """
+    if (checksum.startswith('http://') or checksum.startswith('https://')):
+        if CONF.conductor.disable_support_for_checksum_files:
+            raise exception.ImageChecksumURLNotSupported()
+        return True
+    else:
+        return False
+
+
+def get_checksum_from_url(checksum, image_source):
+    """Gets a checksum value based upon a remote checksum URL file.
+
+    :param checksum: The URL to the checksum URL content.
+    :param image_soource: The image source utilized to match with
+        the contents of the URL payload file.
+    :raises: ImageDownloadFailed when the checksum file cannot be
+        accessed or cannot be parsed.
+    """
+
+    LOG.debug('Attempting to download checksum from: %(checksum)s.',
+              {'checksum': checksum})
+
+    # Directly invoke the image service and get the checksum data.
+    resp = image_service.HttpImageService.get(checksum)
+    checksum_url = str(checksum)
+
+    # NOTE(TheJulia): The rest of this method is taken from
+    # ironic-python-agent. If a change is required here, it may
+    # be required in ironic-python-agent (extensions/standby.py).
+    lines = [line.strip() for line in resp.split('\n') if line.strip()]
+    if not lines:
+        raise exception.ImageDownloadFailed(image_href=checksum,
+                                            reason=_('Checksum file empty.'))
+    elif len(lines) == 1:
+        # Special case - checksums file with only the checksum itself
+        if ' ' not in lines[0]:
+            for matcher in CHECKSUM_ONLY_MATCHERS:
+                checksum = re.findall(matcher, lines[0])
+                if checksum:
+                    return checksum[0]
+            raise exception.ImageDownloadFailed(
+                image_href=checksum_url,
+                reason=(
+                    _("Invalid checksum file (No valid checksum found)")))
+    # FIXME(dtantsur): can we assume the same name for all images?
+    expected_fname = os.path.basename(urlparse.urlparse(
+        image_source).path)
+    for line in lines:
+        # Ignore comment lines
+        if line.startswith("#"):
+            continue
+
+        # Ignore checksums for other files
+        for matcher in FILENAME_MATCHERS:
+            if re.findall(matcher.format(filename=expected_fname), line):
+                break
+        else:
+            continue
+
+        for matcher in CHECKSUM_MATCHERS:
+            checksum = re.findall(matcher, line)
+            if checksum:
+                return checksum[0]
+
+    raise exception.ImageDownloadFailed(
+        image_href=checksum,
+        reason=(_("Checksum file does not contain name %s")
+                % expected_fname))

ironic/common/exception.py

@@ -897,3 +897,25 @@ class UnsupportedHardwareFeature(Invalid):
 
 class InvalidImage(ImageUnacceptable):
     _msg_fmt = _("The requested image is not valid for use.")
+
+
+class ImageChecksumError(InvalidImage):
+    """Exception indicating checksum failed to match."""
+    _msg_fmt = _("The supplied image checksum is invalid or does not match.")
+
+
+class ImageChecksumAlgorithmFailure(InvalidImage):
+    """Cannot load the requested or required checksum algorithm."""
+    _msg_fmt = _("The requested image checksum algorithm cannot be loaded.")
+
+
+class ImageChecksumURLNotSupported(InvalidImage):
+    """Exception indicating we cannot support the remote checksum file."""
+    _msg_fmt = _("Use of remote checksum files is not supported.")
+
+
+class ImageChecksumFileReadFailure(InvalidImage):
+    """An OSError was raised when trying to read the file."""
+    _msg_fmt = _("Failed to read the file from local storage "
+                 "to perform a checksum operation.")
+    code = http_client.SERVICE_UNAVAILABLE

ironic/common/image_service.py

@@ -287,6 +287,43 @@ class HttpImageService(BaseImageService):
             'no_cache': no_cache,
         }
 
+    @staticmethod
+    def get(image_href):
+        """Downloads content and returns the response text.
+
+        :param image_href: Image reference.
+        :raises: exception.ImageRefValidationFailed if GET request returned
+            response code not equal to 200.
+        :raises: exception.ImageDownloadFailed if:
+            * IOError happened during file write;
+            * GET request failed.
+        """
+
+        try:
+
+            verify = strutils.bool_from_string(CONF.webserver_verify_ca,
+                                               strict=True)
+        except ValueError:
+            verify = CONF.webserver_verify_ca
+
+        try:
+            auth = HttpImageService.gen_auth_from_conf_user_pass(image_href)
+            response = requests.get(image_href, stream=False, verify=verify,
+                                    timeout=CONF.webserver_connection_timeout,
+                                    auth=auth)
+            if response.status_code != http_client.OK:
+                raise exception.ImageRefValidationFailed(
+                    image_href=image_href,
+                    reason=_("Got HTTP code %s instead of 200 in response "
+                             "to GET request.") % response.status_code)
+
+            return response.text
+
+        except (OSError, requests.ConnectionError, requests.RequestException,
+                IOError) as e:
+            raise exception.ImageDownloadFailed(image_href=image_href,
+                                                reason=str(e))
+
 
 class FileImageService(BaseImageService):
     """Provides retrieval of disk images available locally on the conductor."""

ironic/common/images.py

@@ -28,6 +28,7 @@ from oslo_log import log as logging
 from oslo_utils import fileutils
 import pycdlib
 
+from ironic.common import checksum_utils
 from ironic.common import exception
 from ironic.common.glance_service import service_utils as glance_utils
 from ironic.common.i18n import _
@@ -386,9 +387,13 @@ def fetch_into(context, image_href, image_file):
               {'image_href': image_href, 'time': time.time() - start})
 
 
-def fetch(context, image_href, path, force_raw=False):
+def fetch(context, image_href, path, force_raw=False,
+          checksum=None, checksum_algo=None):
     with fileutils.remove_path_on_error(path):
         fetch_into(context, image_href, path)
+        if (not CONF.conductor.disable_file_checksum
+                and checksum):
+            checksum_utils.validate_checksum(path, checksum, checksum_algo)
     if force_raw:
         image_to_raw(image_href, path, "%s.part" % path)
 

ironic/conf/conductor.py

@@ -460,6 +460,28 @@ opts = [
                        'permitted for deployment with Ironic. If an image '
                        'format outside of this list is detected, the image '
                        'validation logic will fail the deployment process.')),
+    cfg.BoolOpt('disable_file_checksum',
+                default=False,
+                mutable=False,
+                help=_('Deprecated Security option: In the default case, '
+                       'image files have their checksums verified before '
+                       'undergoing additional conductor side actions such '
+                       'as image conversion. '
+                       'Enabling this option opens the risk of files being '
+                       'replaced at the source without the user\'s '
+                       'knowledge.'),
+                deprecated_for_removal=True),
+    cfg.BoolOpt('disable_support_for_checksum_files',
+                default=False,
+                mutable=False,
+                help=_('Security option: By default Ironic will attempt to '
+                       'retrieve a remote checksum file via HTTP(S) URL in '
+                       'order to validate an image download. This is '
+                       'functionality aligning with ironic-python-agent '
+                       'support for standalone users. Disabling this '
+                       'functionality by setting this option to True will '
+                       'create a more secure environment, however it may '
+                       'break users in an unexpected fashion.')),
 ]
 
 

ironic/drivers/modules/deploy_utils.py

@@ -16,7 +16,6 @@
 
 import os
 import re
-import time
 
 from ironic_lib import metrics_utils
 from ironic_lib import utils as il_utils
@@ -26,6 +25,7 @@ from oslo_utils import fileutils
 from oslo_utils import strutils
 
 from ironic.common import async_steps
+from ironic.common import checksum_utils
 from ironic.common import context
 from ironic.common import exception
 from ironic.common import faults
@@ -60,6 +60,7 @@ RESCUE_LIKE_STATES = (states.RESCUING, states.RESCUEWAIT, states.RESCUEFAIL,
 
 DISK_LAYOUT_PARAMS = ('root_gb', 'swap_mb', 'ephemeral_gb')
 
+
 # All functions are called from deploy() directly or indirectly.
 # They are split for stub-out.
 
@@ -206,7 +207,8 @@ def check_for_missing_params(info_dict, error_msg, param_prefix=''):
 
 
 def fetch_images(ctx, cache, images_info, force_raw=True,
-                 expected_format=None):
+                 expected_format=None, expected_checksum=None,
+                 expected_checksum_algo=None):
     """Check for available disk space and fetch images using ImageCache.
 
     :param ctx: context
@@ -215,6 +217,10 @@ def fetch_images(ctx, cache, images_info, force_raw=True,
     :param force_raw: boolean value, whether to convert the image to raw
                       format
     :param expected_format: The expected format of the image.
+    :param expected_checksum: The expected image checksum, to be used if we
+           need to convert the image to raw prior to deploying.
+    :param expected_checksum_algo: The checksum algo in use, if separately
+           set.
     :raises: InstanceDeployFailure if unable to find enough disk space
     :raises: InvalidImage if the supplied image metadata or contents are
              deemed to be invalid, unsafe, or not matching the expectations
@@ -233,9 +239,12 @@ def fetch_images(ctx, cache, images_info, force_raw=True,
     image_list = []
     for href, path in images_info:
         # NOTE(TheJulia): Href in this case can be an image UUID or a URL.
-        image_format = cache.fetch_image(href, path, ctx=ctx,
+        image_format = cache.fetch_image(
+            href, path, ctx=ctx,
             force_raw=force_raw,
-                                         expected_format=expected_format)
+            expected_format=expected_format,
+            expected_checksum=expected_checksum,
+            expected_checksum_algo=expected_checksum_algo)
         image_list.append((href, path, image_format))
     return image_list
 
@@ -1072,7 +1081,8 @@ class InstanceImageCache(image_cache.ImageCache):
 
 
 @METRICS.timer('cache_instance_image')
-def cache_instance_image(ctx, node, force_raw=None, expected_format=None):
+def cache_instance_image(ctx, node, force_raw=None, expected_format=None,
+                         expected_checksum=None, expected_checksum_algo=None):
     """Fetch the instance's image from Glance
 
     This method pulls the disk image and writes them to the appropriate
@@ -1082,6 +1092,10 @@ def cache_instance_image(ctx, node, force_raw=None, expected_format=None):
     :param node: an ironic node object
     :param force_raw: whether convert image to raw format
     :param expected_format: The expected format of the disk image contents.
+    :param expected_checksum: The expected image checksum, to be used if we
+           need to convert the image to raw prior to deploying.
+    :param expected_checksum_algo: The checksum algo in use, if separately
+           set.
     :returns: a tuple containing the uuid of the image and the path in
         the filesystem where image is cached.
     :raises: InvalidImage if the requested image is invalid and cannot be
@@ -1101,7 +1115,9 @@ def cache_instance_image(ctx, node, force_raw=None, expected_format=None):
               {'image': uuid, 'uuid': node.uuid})
 
     image_list = fetch_images(ctx, InstanceImageCache(), [(uuid, image_path)],
-                              force_raw, expected_format=expected_format)
+                              force_raw, expected_format=expected_format,
+                              expected_checksum=expected_checksum,
+                              expected_checksum_algo=expected_checksum_algo)
     return (uuid, image_path, image_list[0][2])
 
 
@@ -1119,17 +1135,11 @@ def destroy_images(node_uuid):
 @METRICS.timer('compute_image_checksum')
 def compute_image_checksum(image_path, algorithm='md5'):
     """Compute checksum by given image path and algorithm."""
-    time_start = time.time()
+    # NOTE(TheJulia): This likely wouldn't be removed, but if we do
-    LOG.debug('Start computing %(algo)s checksum for image %(image)s.',
+    # significant refactoring we could likely just change everything
-              {'algo': algorithm, 'image': image_path})
+    # over to the images common code, if we don't need the metrics
-    checksum = fileutils.compute_file_checksum(image_path,
+    # data anymore.
-                                               algorithm=algorithm)
+    return checksum_utils.compute_image_checksum(image_path, algorithm)
-    time_elapsed = time.time() - time_start
-    LOG.debug('Computed %(algo)s checksum for image %(image)s in '
-              '%(delta).2f seconds, checksum value: %(checksum)s.',
-              {'algo': algorithm, 'image': image_path, 'delta': time_elapsed,
-               'checksum': checksum})
-    return checksum
 
 
 def remove_http_instance_symlink(node_uuid):
@@ -1205,6 +1215,11 @@ def _validate_image_url(node, url, secret=False, inspect_image=None,
         # let it run the image validation checking as it's normal course
         # of action, and save what it tells us the image format is.
         # if there *was* a mismatch, it will raise the error.
+
+        # NOTE(TheJulia): We don't need to supply the checksum here, because
+        # we are not converting the image. The net result is the deploy
+        # interface or remote agent has the responsibility to checksum the
+        # image.
         _, image_path, img_format = cache_instance_image(
             ctx,
             node,
@@ -1226,10 +1241,14 @@ def _cache_and_convert_image(task, instance_info, image_info=None):
         initial_format = instance_info.get('image_disk_format')
     else:
         initial_format = image_info.get('disk_format')
+    checksum, checksum_algo = checksum_utils.get_checksum_and_algo(
+        instance_info)
     _, image_path, img_format = cache_instance_image(
         task.context, task.node,
         force_raw=force_raw,
-        expected_format=initial_format)
+        expected_format=initial_format,
+        expected_checksum=checksum,
+        expected_checksum_algo=checksum_algo)
     if force_raw or image_info is None:
         if force_raw:
             instance_info['image_disk_format'] = 'raw'
@@ -1265,7 +1284,8 @@ def _cache_and_convert_image(task, instance_info, image_info=None):
                       '%(node)s due to image conversion',
                       {'image': image_path, 'node': task.node.uuid})
             instance_info['image_checksum'] = None
-            hash_value = compute_image_checksum(image_path, os_hash_algo)
+            hash_value = checksum_utils.compute_image_checksum(image_path,
+                                                               os_hash_algo)
         else:
             instance_info['image_checksum'] = old_checksum
 
@@ -1363,6 +1383,11 @@ def build_instance_info_for_deploy(task):
         image_info = glance.show(image_source)
         LOG.debug('Got image info: %(info)s for node %(node)s.',
                   {'info': image_info, 'node': node.uuid})
+        # Values are explicitly set into the instance info field
+        # so IPA have the values available.
+        instance_info['image_checksum'] = image_info['checksum']
+        instance_info['image_os_hash_algo'] = image_info['os_hash_algo']
+        instance_info['image_os_hash_value'] = image_info['os_hash_value']
         if image_download_source == 'swift':
             # In this case, we are getting a file *from* swift for a glance
             # image which is backed by swift. IPA downloads the file directly
@@ -1376,14 +1401,9 @@ def build_instance_info_for_deploy(task):
             validate_results = _validate_image_url(
                 node, swift_temp_url, secret=True,
                 expected_format=image_format)
-            # Values are explicitly set into the instance info field
-            # so IPA have the values available.
             instance_info['image_url'] = swift_temp_url
-            instance_info['image_checksum'] = image_info['checksum']
             instance_info['image_disk_format'] = \
                 validate_results.get('disk_format', image_format)
-            instance_info['image_os_hash_algo'] = image_info['os_hash_algo']
-            instance_info['image_os_hash_value'] = image_info['os_hash_value']
         else:
             # In this case, we're directly downloading the glance image and
             # hosting it locally for retrieval by the IPA.

ironic/drivers/modules/image_cache.py

@@ -66,7 +66,8 @@ class ImageCache(object):
             fileutils.ensure_tree(master_dir)
 
     def fetch_image(self, href, dest_path, ctx=None, force_raw=True,
-                    expected_format=None):
+                    expected_format=None, expected_checksum=None,
+                    expected_checksum_algo=None):
         """Fetch image by given href to the destination path.
 
         Does nothing if destination path exists and is up to date with cache
@@ -82,16 +83,32 @@ class ImageCache(object):
         :param force_raw: boolean value, whether to convert the image to raw
                           format
         :param expected_format: The expected image format.
+        :param expected_checksum: The expected image checksum
+        :param expected_checksum_algo: The expected image checksum algorithm,
+                                       if needed/supplied.
         """
         img_download_lock_name = 'download-image'
         if self.master_dir is None:
             # NOTE(ghe): We don't share images between instances/hosts
+            # NOTE(TheJulia): These is a weird code path, because master_dir
+            # has to be None, which by default it never should be unless
+            # an operator forces it to None, which is a path we just never
+            # expect.
+            # TODO(TheJulia): This may be dead-ish code and likely needs
+            # to be removed. Likely originated *out* of just the iscsi
+            # deployment interface and local image caching.
             if not CONF.parallel_image_downloads:
                 with lockutils.lock(img_download_lock_name):
-                    _fetch(ctx, href, dest_path, force_raw)
+                    _fetch(ctx, href, dest_path, force_raw,
+                           expected_format=expected_format,
+                           expected_checksum=expected_checksum,
+                           expected_checksum_algo=expected_checksum_algo)
             else:
                 with _concurrency_semaphore:
-                    _fetch(ctx, href, dest_path, force_raw)
+                    _fetch(ctx, href, dest_path, force_raw,
+                           expected_format=expected_format,
+                           expected_checksum=expected_checksum,
+                           expected_checksum_algo=expected_checksum_algo)
             return
 
         # TODO(ghe): have hard links and counts the same behaviour in all fs
@@ -143,13 +160,17 @@ class ImageCache(object):
             self._download_image(
                 href, master_path, dest_path, img_info,
                 ctx=ctx, force_raw=force_raw,
-                expected_format=expected_format)
+                expected_format=expected_format,
+                expected_checksum=expected_checksum,
+                expected_checksum_algo=expected_checksum_algo)
 
         # NOTE(dtantsur): we increased cache size - time to clean up
         self.clean_up()
 
     def _download_image(self, href, master_path, dest_path, img_info,
-                        ctx=None, force_raw=True, expected_format=None):
+                        ctx=None, force_raw=True, expected_format=None,
+                        expected_checksum=None,
+                        expected_checksum_algo=None):
         """Download image by href and store at a given path.
 
         This method should be called with uuid-specific lock taken.
@@ -162,6 +183,8 @@ class ImageCache(object):
         :param force_raw: boolean value, whether to convert the image to raw
                           format
         :param expected_format: The expected original format for the image.
+        :param expected_checksum: The expected image checksum.
+        :param expected_checksum_algo: The expected image checksum algorithm.
         :raise ImageDownloadFailed: when the image cache and the image HTTP or
                                     TFTP location are on different file system,
                                     causing hard link to fail.
@@ -173,7 +196,9 @@ class ImageCache(object):
 
         try:
             with _concurrency_semaphore:
-                _fetch(ctx, href, tmp_path, force_raw, expected_format)
+                _fetch(ctx, href, tmp_path, force_raw, expected_format,
+                       expected_checksum=expected_checksum,
+                       expected_checksum_algo=expected_checksum_algo)
 
             if img_info.get('no_cache'):
                 LOG.debug("Caching is disabled for image %s", href)
@@ -337,13 +362,16 @@ def _free_disk_space_for(path):
     return stat.f_frsize * stat.f_bavail
 
 
-def _fetch(context, image_href, path, force_raw=False, expected_format=None):
+def _fetch(context, image_href, path, force_raw=False, expected_format=None,
+           expected_checksum=None, expected_checksum_algo=None):
     """Fetch image and convert to raw format if needed."""
     path_tmp = "%s.part" % path
     if os.path.exists(path_tmp):
         LOG.warning("%s exist, assuming it's stale", path_tmp)
         os.remove(path_tmp)
-    images.fetch(context, image_href, path_tmp, force_raw=False)
+    images.fetch(context, image_href, path_tmp, force_raw=False,
+                 checksum=expected_checksum,
+                 checksum_algo=expected_checksum_algo)
     # By default, the image format is unknown
     image_format = None
     disable_dii = CONF.conductor.disable_deep_image_inspection

ironic/tests/unit/common/test_checksum_utils.py

@@ -0,0 +1,211 @@
+# coding=utf-8
+
+# Copyright 2024 Red Hat, Inc.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from unittest import mock
+
+from oslo_config import cfg
+
+from ironic.common import checksum_utils
+from ironic.common import exception
+from ironic.common import image_service
+from ironic.tests import base
+
+CONF = cfg.CONF
+
+
+@mock.patch.object(checksum_utils, 'compute_image_checksum',
+                   autospec=True)
+class IronicChecksumUtilsValidateTestCase(base.TestCase):
+
+    def test_validate_checksum(self, mock_compute):
+        mock_compute.return_value = 'f00'
+        checksum_utils.validate_checksum('path', 'f00', 'algo')
+        mock_compute.assert_called_once_with('path', 'algo')
+
+    def test_validate_checksum_mixed_case(self, mock_compute):
+        mock_compute.return_value = 'f00'
+        checksum_utils.validate_checksum('path', 'F00', 'ALGO')
+        mock_compute.assert_called_once_with('path', 'algo')
+
+    def test_validate_checksum_mixed_md5(self, mock_compute):
+        mock_compute.return_value = 'f00'
+        checksum_utils.validate_checksum('path', 'F00')
+        mock_compute.assert_called_once_with('path')
+
+    def test_validate_checksum_mismatch(self, mock_compute):
+        mock_compute.return_value = 'a00'
+        self.assertRaises(exception.ImageChecksumError,
+                          checksum_utils.validate_checksum,
+                          'path', 'f00', 'algo')
+        mock_compute.assert_called_once_with('path', 'algo')
+
+    def test_validate_checksum_hashlib_not_supports_algo(self, mock_compute):
+        mock_compute.side_effect = ValueError()
+        self.assertRaises(exception.ImageChecksumAlgorithmFailure,
+                          checksum_utils.validate_checksum,
+                          'path', 'f00', 'algo')
+        mock_compute.assert_called_once_with('path', 'algo')
+
+    def test_validate_checksum_file_not_found(self, mock_compute):
+        mock_compute.side_effect = OSError()
+        self.assertRaises(exception.ImageChecksumFileReadFailure,
+                          checksum_utils.validate_checksum,
+                          'path', 'f00', 'algo')
+        mock_compute.assert_called_once_with('path', 'algo')
+
+    def test_validate_checksum_mixed_case_delimited(self, mock_compute):
+        mock_compute.return_value = 'f00'
+        checksum_utils.validate_checksum('path', 'algo:F00')
+        mock_compute.assert_called_once_with('path', 'algo')
+
+
+class IronicChecksumUtilsTestCase(base.TestCase):
+
+    def test_is_checksum_url_string(self):
+        self.assertFalse(checksum_utils.is_checksum_url('f00'))
+
+    def test_is_checksum_url_file(self):
+        self.assertFalse(checksum_utils.is_checksum_url('file://foo'))
+
+    def test_is_checksum_url(self):
+        urls = ['http://foo.local/file',
+                'https://foo.local/file']
+        for url in urls:
+            self.assertTrue(checksum_utils.is_checksum_url(url))
+
+    def test_get_checksum_and_algo_image_checksum(self):
+        value = 'c46f2c98efe1cd246be1796cd842246e'
+        i_info = {'image_checksum': value}
+        csum, algo = checksum_utils.get_checksum_and_algo(i_info)
+        self.assertEqual(value, csum)
+        self.assertIsNone(algo)
+
+    def test_get_checksum_and_algo_image_checksum_not_allowed(self):
+        CONF.set_override('allow_md5_checksum', False, group='agent')
+        value = 'c46f2c98efe1cd246be1796cd842246e'
+        i_info = {'image_checksum': value}
+        self.assertRaises(exception.ImageChecksumAlgorithmFailure,
+                          checksum_utils.get_checksum_and_algo,
+                          i_info)
+
+    def test_get_checksum_and_algo_image_checksum_glance(self):
+        value = 'c46f2c98efe1cd246be1796cd842246e'
+        i_info = {'image_os_hash_value': value,
+                  'image_os_hash_algo': 'foobar'}
+        csum, algo = checksum_utils.get_checksum_and_algo(i_info)
+        self.assertEqual(value, csum)
+        self.assertEqual('foobar', algo)
+
+    def test_get_checksum_and_algo_image_checksum_sha256(self):
+        value = 'a' * 64
+        i_info = {'image_checksum': value}
+        csum, algo = checksum_utils.get_checksum_and_algo(i_info)
+        self.assertEqual(value, csum)
+        self.assertEqual('sha256', algo)
+
+    def test_get_checksum_and_algo_image_checksum_sha512(self):
+        value = 'f' * 128
+        i_info = {'image_checksum': value}
+        csum, algo = checksum_utils.get_checksum_and_algo(i_info)
+        self.assertEqual(value, csum)
+        self.assertEqual('sha512', algo)
+
+    @mock.patch.object(checksum_utils, 'get_checksum_from_url', autospec=True)
+    def test_get_checksum_and_algo_image_checksum_http_url(self, mock_get):
+        value = 'http://checksum-url'
+        i_info = {
+            'image_checksum': value,
+            'image_source': 'image-ref'
+        }
+        mock_get.return_value = 'f' * 64
+        csum, algo = checksum_utils.get_checksum_and_algo(i_info)
+        mock_get.assert_called_once_with(value, 'image-ref')
+        self.assertEqual('f' * 64, csum)
+        self.assertEqual('sha256', algo)
+
+    @mock.patch.object(checksum_utils, 'get_checksum_from_url', autospec=True)
+    def test_get_checksum_and_algo_image_checksum_https_url(self, mock_get):
+        value = 'https://checksum-url'
+        i_info = {
+            'image_checksum': value,
+            'image_source': 'image-ref'
+        }
+        mock_get.return_value = 'f' * 128
+        csum, algo = checksum_utils.get_checksum_and_algo(i_info)
+        mock_get.assert_called_once_with(value, 'image-ref')
+        self.assertEqual('f' * 128, csum)
+        self.assertEqual('sha512', algo)
+
+
+@mock.patch.object(image_service.HttpImageService, 'get',
+                   autospec=True)
+class IronicChecksumUtilsGetChecksumTestCase(base.TestCase):
+
+    def test_get_checksum_from_url_empty_response(self, mock_get):
+        mock_get.return_value = ''
+        error = ('Failed to download image https://checksum-url, '
+                 'reason: Checksum file empty.')
+        self.assertRaisesRegex(exception.ImageDownloadFailed,
+                               error,
+                               checksum_utils.get_checksum_from_url,
+                               'https://checksum-url',
+                               'https://image-url/file')
+        mock_get.assert_called_once_with('https://checksum-url')
+
+    def test_get_checksum_from_url_one_line(self, mock_get):
+        mock_get.return_value = 'a' * 32
+        csum = checksum_utils.get_checksum_from_url(
+            'https://checksum-url', 'https://image-url/file')
+        mock_get.assert_called_once_with('https://checksum-url')
+        self.assertEqual('a' * 32, csum)
+
+    def test_get_checksum_from_url_nomatch_line(self, mock_get):
+        mock_get.return_value = 'foobar'
+        # For some reason assertRaisesRegex really doesn't like
+        # the error. Easiest path is just to assertTrue the compare.
+        exc = self.assertRaises(exception.ImageDownloadFailed,
+                                checksum_utils.get_checksum_from_url,
+                                'https://checksum-url',
+                                'https://image-url/file')
+        self.assertTrue(
+            'Invalid checksum file (No valid checksum found' in str(exc))
+        mock_get.assert_called_once_with('https://checksum-url')
+
+    def test_get_checksum_from_url_multiline(self, mock_get):
+        test_csum = ('f2ca1bb6c7e907d06dafe4687e579fce76b37e4e9'
+                     '3b7605022da52e6ccc26fd2')
+        mock_get.return_value = ('fee f00\n%s file\nbar fee\nf00' % test_csum)
+        # For some reason assertRaisesRegex really doesn't like
+        # the error. Easiest path is just to assertTrue the compare.
+        checksum = checksum_utils.get_checksum_from_url(
+            'https://checksum-url',
+            'https://image-url/file')
+        self.assertEqual(test_csum, checksum)
+        mock_get.assert_called_once_with('https://checksum-url')
+
+    def test_get_checksum_from_url_multiline_no_file(self, mock_get):
+        test_csum = 'a' * 64
+        error = ("Failed to download image https://checksum-url, reason: "
+                 "Checksum file does not contain name file")
+        mock_get.return_value = ('f00\n%s\nbar\nf00' % test_csum)
+        # For some reason assertRaisesRegex really doesn't like
+        # the error. Easiest path is just to assertTrue the compare.
+        self.assertRaisesRegex(exception.ImageDownloadFailed,
+                               error,
+                               checksum_utils.get_checksum_from_url,
+                               'https://checksum-url',
+                               'https://image-url/file')
+        mock_get.assert_called_once_with('https://checksum-url')

ironic/tests/unit/common/test_image_service.py

@@ -574,6 +574,40 @@ class HttpImageServiceTestCase(base.TestCase):
                                              verify=True,
                                              timeout=15, auth=None)
 
+    @mock.patch.object(requests, 'get', autospec=True)
+    def test_get_success(self, req_get_mock):
+        response_mock = req_get_mock.return_value
+        response_mock.status_code = http_client.OK
+        response_mock.text = 'value'
+        self.assertEqual('value', self.service.get('http://url'))
+        req_get_mock.assert_called_once_with('http://url', stream=False,
+                                             verify=True, timeout=60,
+                                             auth=None)
+
+    @mock.patch.object(requests, 'get', autospec=True)
+    def test_get_handles_exceptions(self, req_get_mock):
+        for exc in [OSError, requests.ConnectionError,
+                    requests.RequestException, IOError]:
+            req_get_mock.reset_mock()
+            req_get_mock.side_effect = exc
+            self.assertRaises(exception.ImageDownloadFailed,
+                              self.service.get,
+                              'http://url')
+            req_get_mock.assert_called_once_with('http://url', stream=False,
+                                                 verify=True, timeout=60,
+                                                 auth=None)
+
+    @mock.patch.object(requests, 'get', autospec=True)
+    def test_get_success_verify_false(self, req_get_mock):
+        cfg.CONF.set_override('webserver_verify_ca', False)
+        response_mock = req_get_mock.return_value
+        response_mock.status_code = http_client.OK
+        response_mock.text = 'value'
+        self.assertEqual('value', self.service.get('http://url'))
+        req_get_mock.assert_called_once_with('http://url', stream=False,
+                                             verify=False, timeout=60,
+                                             auth=None)
+
 
 class FileImageServiceTestCase(base.TestCase):
     def setUp(self):

ironic/tests/unit/common/test_images.py

@@ -23,6 +23,7 @@ from unittest import mock
 
 from oslo_concurrency import processutils
 from oslo_config import cfg
+from oslo_utils import fileutils
 
 from ironic.common import exception
 from ironic.common.glance_service import service_utils as glance_utils
@@ -73,6 +74,100 @@ class IronicImagesTestCase(base.TestCase):
         image_to_raw_mock.assert_called_once_with(
             'image_href', 'path', 'path.part')
 
+    @mock.patch.object(fileutils, 'compute_file_checksum',
+                       autospec=True)
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    @mock.patch.object(images, 'image_to_raw', autospec=True)
+    @mock.patch.object(builtins, 'open', autospec=True)
+    def test_fetch_image_service_force_raw_with_checksum(
+            self, open_mock, image_to_raw_mock,
+            image_service_mock, mock_checksum):
+        mock_file_handle = mock.MagicMock(spec=io.BytesIO)
+        mock_file_handle.__enter__.return_value = 'file'
+        open_mock.return_value = mock_file_handle
+        mock_checksum.return_value = 'f00'
+
+        images.fetch('context', 'image_href', 'path', force_raw=True,
+                     checksum='f00', checksum_algo='sha256')
+
+        mock_checksum.assert_called_once_with('path', algorithm='sha256')
+        open_mock.assert_called_once_with('path', 'wb')
+        image_service_mock.return_value.download.assert_called_once_with(
+            'image_href', 'file')
+        image_to_raw_mock.assert_called_once_with(
+            'image_href', 'path', 'path.part')
+
+    @mock.patch.object(fileutils, 'compute_file_checksum',
+                       autospec=True)
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    @mock.patch.object(images, 'image_to_raw', autospec=True)
+    @mock.patch.object(builtins, 'open', autospec=True)
+    def test_fetch_image_service_with_checksum_mismatch(
+            self, open_mock, image_to_raw_mock,
+            image_service_mock, mock_checksum):
+        mock_file_handle = mock.MagicMock(spec=io.BytesIO)
+        mock_file_handle.__enter__.return_value = 'file'
+        open_mock.return_value = mock_file_handle
+        mock_checksum.return_value = 'a00'
+
+        self.assertRaises(exception.ImageChecksumError,
+                          images.fetch, 'context', 'image_href',
+                          'path', force_raw=True,
+                          checksum='f00', checksum_algo='sha256')
+
+        mock_checksum.assert_called_once_with('path', algorithm='sha256')
+        open_mock.assert_called_once_with('path', 'wb')
+        image_service_mock.return_value.download.assert_called_once_with(
+            'image_href', 'file')
+        # If the checksum fails, then we don't attempt to convert the image.
+        image_to_raw_mock.assert_not_called()
+
+    @mock.patch.object(fileutils, 'compute_file_checksum',
+                       autospec=True)
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    @mock.patch.object(images, 'image_to_raw', autospec=True)
+    @mock.patch.object(builtins, 'open', autospec=True)
+    def test_fetch_image_service_force_raw_no_checksum_algo(
+            self, open_mock, image_to_raw_mock,
+            image_service_mock, mock_checksum):
+        mock_file_handle = mock.MagicMock(spec=io.BytesIO)
+        mock_file_handle.__enter__.return_value = 'file'
+        open_mock.return_value = mock_file_handle
+        mock_checksum.return_value = 'f00'
+
+        images.fetch('context', 'image_href', 'path', force_raw=True,
+                     checksum='f00')
+
+        mock_checksum.assert_called_once_with('path', algorithm='md5')
+        open_mock.assert_called_once_with('path', 'wb')
+        image_service_mock.return_value.download.assert_called_once_with(
+            'image_href', 'file')
+        image_to_raw_mock.assert_called_once_with(
+            'image_href', 'path', 'path.part')
+
+    @mock.patch.object(fileutils, 'compute_file_checksum',
+                       autospec=True)
+    @mock.patch.object(image_service, 'get_image_service', autospec=True)
+    @mock.patch.object(images, 'image_to_raw', autospec=True)
+    @mock.patch.object(builtins, 'open', autospec=True)
+    def test_fetch_image_service_force_raw_combined_algo(
+            self, open_mock, image_to_raw_mock,
+            image_service_mock, mock_checksum):
+        mock_file_handle = mock.MagicMock(spec=io.BytesIO)
+        mock_file_handle.__enter__.return_value = 'file'
+        open_mock.return_value = mock_file_handle
+        mock_checksum.return_value = 'f00'
+
+        images.fetch('context', 'image_href', 'path', force_raw=True,
+                     checksum='sha512:f00')
+
+        mock_checksum.assert_called_once_with('path', algorithm='sha512')
+        open_mock.assert_called_once_with('path', 'wb')
+        image_service_mock.return_value.download.assert_called_once_with(
+            'image_href', 'file')
+        image_to_raw_mock.assert_called_once_with(
+            'image_href', 'path', 'path.part')
+
     @mock.patch.object(image_format_inspector, 'detect_file_format',
                        autospec=True)
     def test_image_to_raw_not_permitted_format(self, detect_format_mock):

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

@@ -24,6 +24,7 @@ from oslo_utils import fileutils
 from oslo_utils import uuidutils
 
 from ironic.common import boot_devices
+from ironic.common import checksum_utils
 from ironic.common import exception
 from ironic.common import faults
 from ironic.common import image_service
@@ -604,10 +605,33 @@ class OtherFunctionTestCase(db_base.DbTestCase):
         utils.fetch_images(None, mock_cache, [('uuid', 'path')])
         mock_clean_up_caches.assert_called_once_with(None, 'master_dir',
                                                      [('uuid', 'path')])
-        mock_cache.fetch_image.assert_called_once_with('uuid', 'path',
+        mock_cache.fetch_image.assert_called_once_with(
+            'uuid', 'path',
             ctx=None,
             force_raw=True,
-                                                       expected_format=None)
+            expected_format=None,
+            expected_checksum=None,
+            expected_checksum_algo=None)
+
+    @mock.patch.object(image_cache, 'clean_up_caches', autospec=True)
+    def test_fetch_images_checksum(self, mock_clean_up_caches):
+
+        mock_cache = mock.MagicMock(
+            spec_set=['fetch_image', 'master_dir'], master_dir='master_dir')
+        utils.fetch_images(None, mock_cache, [('uuid', 'path')],
+                           force_raw=True,
+                           expected_format='qcow2',
+                           expected_checksum='f00',
+                           expected_checksum_algo='sha256')
+        mock_clean_up_caches.assert_called_once_with(None, 'master_dir',
+                                                     [('uuid', 'path')])
+        mock_cache.fetch_image.assert_called_once_with(
+            'uuid', 'path',
+            ctx=None,
+            force_raw=True,
+            expected_format='qcow2',
+            expected_checksum='f00',
+            expected_checksum_algo='sha256')
 
     @mock.patch.object(image_cache, 'clean_up_caches', autospec=True)
     def test_fetch_images_fail(self, mock_clean_up_caches):
@@ -2464,7 +2488,9 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
     @mock.patch.object(image_service, 'GlanceImageService', autospec=True)
     def _test_build_instance_info(self, glance_mock, validate_mock,
                                   image_info={}, expect_raw=False,
-                                  expect_format='qcow2'):
+                                  expect_format='qcow2',
+                                  expect_checksum='fake-sha512',
+                                  expect_checksum_algo='sha512'):
         glance_mock.return_value.show = mock.MagicMock(spec_set=[],
                                                        return_value=image_info)
         with task_manager.acquire(
@@ -2477,7 +2503,9 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
                 task.context,
                 task.node,
                 force_raw=expect_raw,
-                expected_format=expect_format)
+                expected_format=expect_format,
+                expected_checksum=expect_checksum,
+                expected_checksum_algo=expect_checksum_algo)
             symlink_dir = utils._get_http_image_symlink_dir_path()
             symlink_file = utils._get_http_image_symlink_file_path(
                 self.node.uuid)
@@ -2531,7 +2559,8 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         cfg.CONF.set_override('force_raw_images', True)
         self.image_info['os_hash_algo'] = 'md5'
         image_path, instance_info = self._test_build_instance_info(
-            image_info=self.image_info, expect_raw=True)
+            image_info=self.image_info, expect_raw=True,
+            expect_checksum_algo='md5')
 
         self.assertIsNone(instance_info['image_checksum'])
         self.assertEqual(instance_info['image_disk_format'], 'raw')
@@ -2543,7 +2572,8 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
         self.image_info['os_hash_algo'] = 'md5'
         self.image_info['disk_format'] = 'raw'
         image_path, instance_info = self._test_build_instance_info(
-            image_info=self.image_info, expect_raw=True, expect_format='raw')
+            image_info=self.image_info, expect_raw=True, expect_format='raw',
+            expect_checksum_algo='md5')
 
         self.assertEqual(instance_info['image_checksum'], 'aa')
         self.assertEqual(instance_info['image_disk_format'], 'raw')
@@ -2576,7 +2606,9 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             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_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(
@@ -2609,7 +2641,9 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             self.assertEqual('fake-checksum', info['image_os_hash_value'])
             self.cache_image_mock.assert_called_once_with(
                 task.context, task.node, force_raw=True,
-                expected_format=None)
+                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(
@@ -2645,7 +2679,9 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             self.assertEqual('fake-checksum', info['image_os_hash_value'])
             self.cache_image_mock.assert_called_once_with(
                 task.context, task.node, force_raw=True,
-                expected_format='qcow2')
+                expected_format='qcow2',
+                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(
@@ -2726,7 +2762,9 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             self.assertEqual('fake-checksum', info['image_os_hash_value'])
             self.cache_image_mock.assert_called_once_with(
                 task.context, task.node, force_raw=True,
-                expected_format=None)
+                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(
@@ -2755,7 +2793,6 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
                 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('aa', info['image_checksum'])
             self.assertEqual('raw', info['image_disk_format'])
@@ -2763,11 +2800,175 @@ class TestBuildInstanceInfoForHttpProvisioning(db_base.DbTestCase):
             self.assertIsNone(info['image_os_hash_value'])
             self.cache_image_mock.assert_called_once_with(
                 task.context, task.node, force_raw=True,
-                expected_format='raw')
+                expected_format='raw', expected_checksum='aa',
+                expected_checksum_algo=None)
             self.checksum_mock.assert_not_called()
             validate_href_mock.assert_called_once_with(
                 mock.ANY, expected_url, False)
 
+    @mock.patch.object(image_service.HttpImageService, 'get',
+                       autospec=True)
+    @mock.patch.object(image_service.HttpImageService, 'validate_href',
+                       autospec=True)
+    def test_build_instance_info_remote_checksum_image(self,
+                                                       validate_href_mock,
+                                                       get_mock):
+        # Test case where we would download both the image and the checksum
+        # and ultimately convert the image.
+        get_mock.return_value = 'd8e8fca2dc0f896fd7cb4cb0031ba249'
+        cfg.CONF.set_override('image_download_source', 'local', group='agent')
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        i_info['image_source'] = 'http://image-ref'
+        i_info['image_checksum'] = 'http://image-checksum'
+        i_info['root_gb'] = 10
+        i_info['image_disk_format'] = 'qcow2'
+        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('raw', info['image_disk_format'])
+            self.cache_image_mock.assert_called_once_with(
+                task.context, task.node, force_raw=True,
+                expected_format='qcow2',
+                expected_checksum='d8e8fca2dc0f896fd7cb4cb0031ba249',
+                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)
+            get_mock.assert_called_once_with('http://image-checksum')
+
+    @mock.patch.object(image_service.HttpImageService, 'get',
+                       autospec=True)
+    @mock.patch.object(image_service.HttpImageService, 'validate_href',
+                       autospec=True)
+    def test_build_instance_info_remote_checksum_sha256(self,
+                                                        validate_href_mock,
+                                                        get_mock):
+        # Test case where we would download both the image and the checksum
+        # and ultimately convert the image.
+        get_mock.return_value = 'a' * 64 + '\n'
+        cfg.CONF.set_override('image_download_source', 'local', group='agent')
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        i_info['image_source'] = 'https://image-ref'
+        i_info['image_checksum'] = 'https://image-checksum'
+        i_info['root_gb'] = 10
+        i_info['image_disk_format'] = 'qcow2'
+        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('raw', info['image_disk_format'])
+            self.cache_image_mock.assert_called_once_with(
+                task.context, task.node, force_raw=True,
+                expected_format='qcow2',
+                expected_checksum='a' * 64,
+                expected_checksum_algo='sha256')
+            self.checksum_mock.assert_called_once_with(
+                self.fake_path, algorithm='sha256')
+            validate_href_mock.assert_called_once_with(
+                mock.ANY, expected_url, False)
+            get_mock.assert_called_once_with('https://image-checksum')
+
+    @mock.patch.object(image_service.HttpImageService, 'get',
+                       autospec=True)
+    @mock.patch.object(image_service.HttpImageService, 'validate_href',
+                       autospec=True)
+    def test_build_instance_info_remote_checksum_sha512(self,
+                                                        validate_href_mock,
+                                                        get_mock):
+        # Test case where we would download both the image and the checksum
+        # and ultimately convert the image.
+        get_mock.return_value = 'a' * 128 + '\n'
+        cfg.CONF.set_override('image_download_source', 'local', group='agent')
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        i_info['image_source'] = 'https://image-ref'
+        i_info['image_checksum'] = 'https://image-checksum'
+        i_info['root_gb'] = 10
+        i_info['image_disk_format'] = 'qcow2'
+        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('raw', info['image_disk_format'])
+            self.cache_image_mock.assert_called_once_with(
+                task.context, task.node, force_raw=True,
+                expected_format='qcow2',
+                expected_checksum='a' * 128,
+                expected_checksum_algo='sha512')
+            self.checksum_mock.assert_called_once_with(
+                self.fake_path, algorithm='sha256')
+            validate_href_mock.assert_called_once_with(
+                mock.ANY, expected_url, False)
+            get_mock.assert_called_once_with('https://image-checksum')
+
+    @mock.patch.object(checksum_utils, 'get_checksum_from_url',
+                       autospec=True)
+    @mock.patch.object(image_service.HttpImageService, 'validate_href',
+                       autospec=True)
+    def test_build_instance_info_md5_not_permitted(
+            self,
+            validate_href_mock,
+            get_from_url_mock):
+        cfg.CONF.set_override('allow_md5_checksum', False, group='agent')
+        # Test case where we would download both the image and the checksum
+        # and ultimately convert the image.
+        cfg.CONF.set_override('image_download_source', 'local', group='agent')
+        i_info = self.node.instance_info
+        driver_internal_info = self.node.driver_internal_info
+        i_info['image_source'] = 'image-ref'
+        i_info['image_checksum'] = 'ad606d6a24a2dec982bc2993aaaf9160'
+        i_info['root_gb'] = 10
+        i_info['image_disk_format'] = 'qcow2'
+        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()
+
+        with task_manager.acquire(
+                self.context, self.node.uuid, shared=False) as task:
+
+            self.assertRaisesRegex(exception.ImageChecksumAlgorithmFailure,
+                                   'The requested image checksum algorithm '
+                                   'cannot be loaded',
+                                   utils.build_instance_info_for_deploy,
+                                   task)
+
+            self.cache_image_mock.assert_not_called()
+            self.checksum_mock.assert_not_called()
+            validate_href_mock.assert_not_called()
+            get_from_url_mock.assert_not_called()
+
 
 class TestStorageInterfaceUtils(db_base.DbTestCase):
     def setUp(self):

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

@@ -65,7 +65,8 @@ class TestImageCacheFetch(BaseTest):
         self.cache.fetch_image(self.uuid, self.dest_path)
         self.assertFalse(mock_download.called)
         mock_fetch.assert_called_once_with(
-            None, self.uuid, self.dest_path, True)
+            None, self.uuid, self.dest_path, True,
+            None, None, None)
         self.assertFalse(mock_clean_up.called)
         mock_image_service.assert_not_called()
 
@@ -82,7 +83,8 @@ class TestImageCacheFetch(BaseTest):
                           self.uuid, self.dest_path)
         self.assertFalse(mock_download.called)
         mock_fetch.assert_called_once_with(
-            None, self.uuid, self.dest_path, True)
+            None, self.uuid, self.dest_path, True,
+            None, None, None)
         self.assertFalse(mock_clean_up.called)
         mock_image_service.assert_not_called()
 
@@ -157,7 +159,8 @@ class TestImageCacheFetch(BaseTest):
         mock_download.assert_called_once_with(
             self.cache, self.uuid, self.master_path, self.dest_path,
             mock_image_service.return_value.show.return_value,
-            ctx=None, force_raw=True, expected_format=None)
+            ctx=None, force_raw=True, expected_format=None,
+            expected_checksum=None, expected_checksum_algo=None)
         mock_clean_up.assert_called_once_with(self.cache)
         mock_image_service.assert_called_once_with(self.uuid, context=None)
         mock_image_service.return_value.show.assert_called_once_with(self.uuid)
@@ -179,7 +182,8 @@ class TestImageCacheFetch(BaseTest):
         mock_download.assert_called_once_with(
             self.cache, self.uuid, self.master_path, self.dest_path,
             mock_image_service.return_value.show.return_value,
-            ctx=None, force_raw=True, expected_format=None)
+            ctx=None, force_raw=True, expected_format=None,
+            expected_checksum=None, expected_checksum_algo=None)
         mock_clean_up.assert_called_once_with(self.cache)
 
     def test_fetch_image_not_uuid(self, mock_download, mock_clean_up,
@@ -192,7 +196,8 @@ class TestImageCacheFetch(BaseTest):
         mock_download.assert_called_once_with(
             self.cache, href, master_path, self.dest_path,
             mock_image_service.return_value.show.return_value,
-            ctx=None, force_raw=True, expected_format=None)
+            ctx=None, force_raw=True, expected_format=None,
+            expected_checksum=None, expected_checksum_algo=None)
         self.assertTrue(mock_clean_up.called)
 
     def test_fetch_image_not_uuid_no_force_raw(self, mock_download,
@@ -201,11 +206,14 @@ class TestImageCacheFetch(BaseTest):
         href = u'http://abc.com/ubuntu.qcow2'
         href_converted = str(uuid.uuid5(uuid.NAMESPACE_URL, href))
         master_path = os.path.join(self.master_dir, href_converted)
-        self.cache.fetch_image(href, self.dest_path, force_raw=False)
+        self.cache.fetch_image(href, self.dest_path, force_raw=False,
+                               expected_checksum='f00',
+                               expected_checksum_algo='sha256')
         mock_download.assert_called_once_with(
             self.cache, href, master_path, self.dest_path,
             mock_image_service.return_value.show.return_value,
-            ctx=None, force_raw=False, expected_format=None)
+            ctx=None, force_raw=False, expected_format=None,
+            expected_checksum='f00', expected_checksum_algo='sha256')
         self.assertTrue(mock_clean_up.called)
 
 
@@ -213,7 +221,8 @@ class TestImageCacheFetch(BaseTest):
 class TestImageCacheDownload(BaseTest):
 
     def test__download_image(self, mock_fetch):
-        def _fake_fetch(ctx, uuid, tmp_path, *args):
+        def _fake_fetch(ctx, uuid, tmp_path, force_raw, expected_format,
+                        expected_checksum, expected_checksum_algo):
             self.assertEqual(self.uuid, uuid)
             self.assertNotEqual(self.dest_path, tmp_path)
             self.assertNotEqual(os.path.dirname(tmp_path), self.master_dir)
@@ -235,7 +244,8 @@ class TestImageCacheDownload(BaseTest):
         # Make sure we don't use any parts of the URL anywhere.
         url = "http://example.com/image.iso?secret=%s" % ("x" * 1000)
 
-        def _fake_fetch(ctx, href, tmp_path, *args):
+        def _fake_fetch(ctx, href, tmp_path, force_raw, expected_format,
+                        expected_checksum, expected_checksum_algo):
             self.assertEqual(url, href)
             self.assertNotEqual(self.dest_path, tmp_path)
             self.assertNotEqual(os.path.dirname(tmp_path), self.master_dir)
@@ -519,7 +529,8 @@ class TestImageCacheCleanUp(base.TestCase):
     @mock.patch.object(utils, 'rmtree_without_raise', autospec=True)
     @mock.patch.object(image_cache, '_fetch', autospec=True)
     def test_temp_images_not_cleaned(self, mock_fetch, mock_rmtree):
-        def _fake_fetch(ctx, uuid, tmp_path, *args):
+        def _fake_fetch(ctx, uuid, tmp_path, force_raw, expected_format,
+                        expected_checksum, expected_checksum_algo):
             with open(tmp_path, 'w') as fp:
                 fp.write("TEST" * 10)
 
@@ -773,9 +784,13 @@ class TestFetchCleanup(base.TestCase):
         mock_format_inspector.return_value = image_check
         mock_show.return_value = {}
         mock_size.return_value = 100
-        image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True)
+        image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True,
+                           expected_checksum='1234',
+                           expected_checksum_algo='md5')
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum='1234',
+                                           checksum_algo='md5')
         mock_clean.assert_called_once_with('/foo', 100)
         mock_raw.assert_called_once_with('fake-uuid', '/foo/bar',
                                          '/foo/bar.part')
@@ -807,7 +822,8 @@ class TestFetchCleanup(base.TestCase):
         mock_size.return_value = 100
         image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True)
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum=None, checksum_algo=None)
         mock_clean.assert_called_once_with('/foo', 100)
         mock_raw.assert_called_once_with('fake-uuid', '/foo/bar',
                                          '/foo/bar.part')
@@ -837,9 +853,13 @@ class TestFetchCleanup(base.TestCase):
         mock_exists.return_value = True
         mock_size.return_value = 100
         mock_image_show.return_value = {}
-        image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True)
+        image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True,
+                           expected_format=None, expected_checksum='f00',
+                           expected_checksum_algo='sha256')
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum='f00',
+                                           checksum_algo='sha256')
         mock_clean.assert_called_once_with('/foo', 100)
         mock_raw.assert_called_once_with('fake-uuid', '/foo/bar',
                                          '/foo/bar.part')
@@ -867,9 +887,13 @@ class TestFetchCleanup(base.TestCase):
         image_check.__str__.return_value = 'raw'
         image_check.safety_check.return_value = True
         mock_format_inspector.return_value = image_check
-        image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True)
+        image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True,
+                           expected_checksum='e00',
+                           expected_checksum_algo='sha256')
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum='e00',
+                                           checksum_algo='sha256')
         mock_clean.assert_not_called()
         mock_size.assert_not_called()
         mock_raw.assert_not_called()
@@ -897,9 +921,14 @@ class TestFetchCleanup(base.TestCase):
         self.assertRaises(exception.InvalidImage,
                           image_cache._fetch,
                           'fake', 'fake-uuid',
-                          '/foo/bar', force_raw=True)
+                          '/foo/bar', force_raw=True,
+                          expected_format=None,
+                          expected_checksum='a00',
+                          expected_checksum_algo='sha512')
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum='a00',
+                                           checksum_algo='sha512')
         mock_clean.assert_not_called()
         mock_size.assert_not_called()
         mock_raw.assert_not_called()
@@ -928,7 +957,8 @@ class TestFetchCleanup(base.TestCase):
                           'fake', 'fake-uuid',
                           '/foo/bar', force_raw=True)
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum=None, checksum_algo=None)
         mock_clean.assert_not_called()
         mock_size.assert_not_called()
         mock_raw.assert_not_called()
@@ -957,7 +987,8 @@ class TestFetchCleanup(base.TestCase):
 
         image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True)
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum=None, checksum_algo=None)
         mock_size.assert_has_calls([
             mock.call('/foo/bar.part', estimate=False),
             mock.call('/foo/bar.part', estimate=True),
@@ -994,7 +1025,8 @@ class TestFetchCleanup(base.TestCase):
         mock_size.return_value = 100
         image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True)
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum=None, checksum_algo=None)
         mock_clean.assert_not_called()
         mock_raw.assert_not_called()
         mock_remove.assert_not_called()
@@ -1025,7 +1057,8 @@ class TestFetchCleanup(base.TestCase):
         mock_size.return_value = 100
         image_cache._fetch('fake', 'fake-uuid', '/foo/bar', force_raw=True)
         mock_fetch.assert_called_once_with('fake', 'fake-uuid',
-                                           '/foo/bar.part', force_raw=False)
+                                           '/foo/bar.part', force_raw=False,
+                                           checksum=None, checksum_algo=None)
         mock_clean.assert_not_called()
         mock_raw.assert_not_called()
         mock_remove.assert_not_called()

releasenotes/notes/checksum-before-conversion-66d273b94fa2ba4d.yaml

@@ -0,0 +1,44 @@
+---
+security:
+  - |
+    An issue in Ironic has been resolved where image checksums would not be
+    checked prior to the conversion of an image to a ``raw`` format image from
+    another image format.
+
+    With default settings, this normally would not take place, however the
+    ``image_download_source`` option, which is available to be set at a
+    ``node`` level for a single deployment, by default for that baremetal node
+    in all cases, or via the ``[agent]image_download_source`` configuration
+    option when set to ``local``. By default, this setting is ``http``.
+
+    This was in concert with the ``[DEFAULT]force_raw_images`` when set to
+    ``True``, which caused Ironic to download and convert the file.
+
+    In a fully integrated context of Ironic's use in a larger OpenStack
+    deployment, where images are coming from the Glance image service, the
+    previous pattern was not problematic. The overall issue was introduced as
+    a result of the capability to supply, cache, and convert a disk image
+    provided as a URL by an authenticated user.
+
+    Ironic will now validate the user supplied checksum prior to image
+    conversion on the conductor. This can be disabled using the
+    ``[conductor]disable_file_checksum`` configuration option.
+fixes:
+  - |
+    Fixes a security issue where Ironic would fail to checksum disk image
+    files it downloads when Ironic had been requested to download and convert
+    the image to a raw image format. This required the
+    ``image_download_source`` to be explicitly set to ``local``, which is not
+    the default.
+
+    This fix can be disabled by setting
+    ``[conductor]disable_file_checksum`` to ``True``, however this
+    option will be removed in new major Ironic releases.
+
+    As a result of this, parity has been introduced to align Ironic to
+    Ironic-Python-Agent's support for checksums used by ``standalone``
+    users of Ironic. This includes support for remote checksum files to be
+    supplied by URL, in order to prevent breaking existing users which may
+    have inadvertently been leveraging the prior code path. This support can
+    be disabled by setting
+    ``[conductor]disable_support_for_checksum_files`` to ``True``.