From afe4f480905fb80cb7cfe98c43f15b03ea011fea Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Tue, 17 Sep 2024 09:57:19 -0700 Subject: [PATCH] Checksum files before raw conversion While working another issue, we discovered that support added to the ironic-conductor process combined the image_download_source option of "local" with the "force_raw" option resulted in a case where Ironic had no concept to checksum the files *before* the conductor process triggered an image format conversion and then records new checksum values. In essence, this opened the user requested image file to be suspetible to a theoretical man-in-the-middle attack OR the remote server replacing the content with an unknown file, such as a new major version. The is at odds with Ironic's security model where we do want to ensure the end user of ironic is asserting a known checksum for the image artifact they are deploying, so they are aware of the present state. Due to the risk, we chose to raise this as a CVE, as infrastructure operators should likely apply this patch. As a note, if your *not* forcing all images to be raw format through the conductor, then this issue is likely not a major issue for you, but you should still apply the patch. This is being tracked as CVE-2024-47211. Closes-Bug: 2076289 Change-Id: Id6185b317aa6e4f4363ee49f77e688701995323a Signed-off-by: Julia Kreger --- doc/source/admin/security.rst | 34 +++ ironic/common/checksum_utils.py | 258 ++++++++++++++++++ ironic/common/exception.py | 22 ++ ironic/common/image_service.py | 37 +++ ironic/common/images.py | 7 +- ironic/conf/conductor.py | 22 ++ ironic/drivers/modules/deploy_utils.py | 70 +++-- ironic/drivers/modules/image_cache.py | 44 ++- .../tests/unit/common/test_checksum_utils.py | 211 ++++++++++++++ .../tests/unit/common/test_image_service.py | 34 +++ ironic/tests/unit/common/test_images.py | 95 +++++++ .../unit/drivers/modules/test_deploy_utils.py | 229 +++++++++++++++- .../unit/drivers/modules/test_image_cache.py | 79 ++++-- ...um-before-conversion-66d273b94fa2ba4d.yaml | 44 +++ 14 files changed, 1115 insertions(+), 71 deletions(-) create mode 100644 ironic/common/checksum_utils.py create mode 100644 ironic/tests/unit/common/test_checksum_utils.py create mode 100644 releasenotes/notes/checksum-before-conversion-66d273b94fa2ba4d.yaml diff --git a/doc/source/admin/security.rst b/doc/source/admin/security.rst index 5d82e4099b..9287f21480 100644 --- a/doc/source/admin/security.rst +++ b/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 ======================================== diff --git a/ironic/common/checksum_utils.py b/ironic/common/checksum_utils.py new file mode 100644 index 0000000000..7d43a1f890 --- /dev/null +++ b/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)) diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 470e4d5641..6ff4494ce8 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -909,3 +909,25 @@ class BootModeNotAllowed(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 diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index 6197b1962e..e2841586e1 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -287,6 +287,43 @@ def show(self, image_href): '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.""" diff --git a/ironic/common/images.py b/ironic/common/images.py index 0f070684b5..88daf8fb55 100644 --- a/ironic/common/images.py +++ b/ironic/common/images.py @@ -28,6 +28,7 @@ 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) diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index 809c48cf79..525c7aceec 100644 --- a/ironic/conf/conductor.py +++ b/ironic/conf/conductor.py @@ -493,6 +493,28 @@ '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.')), ] diff --git a/ironic/drivers/modules/deploy_utils.py b/ironic/drivers/modules/deploy_utils.py index 5bcf199b47..4848e2fb7e 100644 --- a/ironic/drivers/modules/deploy_utils.py +++ b/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 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 @@ 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, - force_raw=force_raw, - expected_format=expected_format) + image_format = cache.fetch_image( + href, path, ctx=ctx, + force_raw=force_raw, + 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 @@ def __init__(self): @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() - 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 + # NOTE(TheJulia): This likely wouldn't be removed, but if we do + # significant refactoring we could likely just change everything + # over to the images common code, if we don't need the metrics + # data anymore. + return checksum_utils.compute_image_checksum(image_path, algorithm) 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. diff --git a/ironic/drivers/modules/image_cache.py b/ironic/drivers/modules/image_cache.py index 02588a0a5b..0a8255fae5 100644 --- a/ironic/drivers/modules/image_cache.py +++ b/ironic/drivers/modules/image_cache.py @@ -66,7 +66,8 @@ def __init__(self, master_dir, cache_size, cache_ttl): 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 @@ def fetch_image(self, href, dest_path, ctx=None, force_raw=True, :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 @@ def fetch_image(self, href, dest_path, ctx=None, force_raw=True, 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 @@ def _download_image(self, href, master_path, dest_path, img_info, :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 @@ def _download_image(self, href, master_path, dest_path, img_info, 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 diff --git a/ironic/tests/unit/common/test_checksum_utils.py b/ironic/tests/unit/common/test_checksum_utils.py new file mode 100644 index 0000000000..5c0964dc94 --- /dev/null +++ b/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') diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py index a2451fedd9..6a2d738b3a 100644 --- a/ironic/tests/unit/common/test_image_service.py +++ b/ironic/tests/unit/common/test_image_service.py @@ -574,6 +574,40 @@ def test_download_success_custom_timeout( 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): diff --git a/ironic/tests/unit/common/test_images.py b/ironic/tests/unit/common/test_images.py index 5b6ba2d3e8..d8915de6b4 100644 --- a/ironic/tests/unit/common/test_images.py +++ b/ironic/tests/unit/common/test_images.py @@ -23,6 +23,7 @@ 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 @@ def test_fetch_image_service_force_raw(self, open_mock, image_to_raw_mock, 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): diff --git a/ironic/tests/unit/drivers/modules/test_deploy_utils.py b/ironic/tests/unit/drivers/modules/test_deploy_utils.py index 534180012e..cb9ec1b2b6 100644 --- a/ironic/tests/unit/drivers/modules/test_deploy_utils.py +++ b/ironic/tests/unit/drivers/modules/test_deploy_utils.py @@ -24,6 +24,7 @@ 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 @@ def test_fetch_images(self, mock_clean_up_caches): 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', - ctx=None, - force_raw=True, - expected_format=None) + mock_cache.fetch_image.assert_called_once_with( + 'uuid', 'path', + ctx=None, + force_raw=True, + 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 @@ def setUp(self): @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 @@ def _test_build_instance_info(self, glance_mock, validate_mock, 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 @@ def test_build_instance_info_force_raw_drops_md5(self): 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 @@ def test_build_instance_info_already_raw_keeps_md5(self): 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 @@ def test_build_instance_info_file_image(self, validate_href_mock): 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 @@ def test_build_instance_info_local_image(self, validate_href_mock): 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 @@ def test_build_instance_info_local_image_via_iinfo(self, 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 @@ def test_build_instance_info_local_image_via_dinfo(self, 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 @@ def test_build_instance_info_local_image_already_raw(self, 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 @@ def test_build_instance_info_local_image_already_raw(self, 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): diff --git a/ironic/tests/unit/drivers/modules/test_image_cache.py b/ironic/tests/unit/drivers/modules/test_image_cache.py index 3edf56868d..edb151eda2 100644 --- a/ironic/tests/unit/drivers/modules/test_image_cache.py +++ b/ironic/tests/unit/drivers/modules/test_image_cache.py @@ -66,7 +66,8 @@ def test_fetch_image_no_master_dir(self, mock_fetch, mock_download, 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() @@ -83,7 +84,8 @@ def test_fetch_image_no_master_dir_memory_low(self, 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() @@ -158,7 +160,8 @@ def test_fetch_image_master_out_of_date( 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) @@ -180,7 +183,8 @@ def test_fetch_image_both_master_and_dest_out_of_date( 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, @@ -193,7 +197,8 @@ def test_fetch_image_not_uuid(self, mock_download, mock_clean_up, 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, @@ -202,11 +207,14 @@ def test_fetch_image_not_uuid_no_force_raw(self, mock_download, 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) @@ -214,7 +222,8 @@ def test_fetch_image_not_uuid_no_force_raw(self, mock_download, 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) @@ -236,7 +245,8 @@ def test__download_image_large_url(self, mock_fetch): # 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) @@ -520,7 +530,8 @@ def test_clean_up_cache_still_large(self, mock_clean_ttl, mock_log): @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) @@ -774,9 +785,13 @@ def test__fetch( 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') @@ -808,7 +823,8 @@ def test__fetch_deep_inspection_disabled( 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') @@ -838,9 +854,13 @@ def test__fetch_part_already_exists( 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') @@ -868,9 +888,13 @@ def test__fetch_already_raw( 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() @@ -898,9 +922,14 @@ def test__fetch_format_does_not_match_glance( 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() @@ -929,7 +958,8 @@ def test__fetch_not_safe_image( '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() @@ -958,7 +988,8 @@ def test__fetch_estimate_fallback( 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), @@ -995,7 +1026,8 @@ def test__fetch_ramdisk_kernel( 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() @@ -1026,7 +1058,8 @@ def test__fetch_ramdisk_image( 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() diff --git a/releasenotes/notes/checksum-before-conversion-66d273b94fa2ba4d.yaml b/releasenotes/notes/checksum-before-conversion-66d273b94fa2ba4d.yaml new file mode 100644 index 0000000000..5460b844d2 --- /dev/null +++ b/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``.