Check image checksum when uploading or downloading images

app/pyproject.toml

@@ -3,7 +3,7 @@
 
 [project]
 name = "github-runner-image-builder"
-version = "0.14.1"
+version = "0.15.0"
 authors = [
     { name = "Canonical IS DevOps", email = "is-devops-team@canonical.com" },
 ]

app/src/github_runner_image_builder/errors.py

@@ -97,6 +97,10 @@ class UploadImageError(OpenstackBaseError):
     """Represents an error when uploading image to Openstack."""
 
 
+class DownloadImageError(OpenstackBaseError):
+    """Represents an error when downloading image from Openstack."""
+
+
 class OpenstackError(OpenstackBaseError):
     """Represents an error while communicating with Openstack."""
 

app/src/github_runner_image_builder/openstack_builder.py

@@ -85,6 +85,9 @@
 MIN_RAM = 1024  # M
 MIN_DISK = 20  # G
 
+TMP_SNAPSHOT_SUFFIX = "-tmp"
+HASH_BLOCK_SIZE = 4 * 1024 * 1024  # 4MiB
+
 # We saw an issue with arm noble images with the latest release date, so we are using a fixed date.
 NOBLE_ARM64_RELEASE_DATE = date(2025, 11, 13)
 
@@ -837,6 +840,68 @@ class _UploadCloudConfig:
     keep_revisions: int
 
 
+def _validate_downloaded_snapshot(
+    conn: openstack.connection.Connection, image_id: str, file_path: pathlib.Path
+) -> None:
+    """Check that the snapshot was downloaded in full and without corruption.
+
+    The snapshot is streamed from Glance and openstacksdk does not verify what it wrote. Without
+    this check a truncated or corrupted download would be uploaded to the other clouds and
+    published as a seemingly valid image.
+
+    Args:
+        conn: The OpenStack connection instance.
+        image_id: The ID of the snapshot image that was downloaded.
+        file_path: The path the snapshot was downloaded to.
+
+    Raises:
+        DownloadImageError: if the downloaded snapshot does not match the snapshot image.
+    """
+    # The image is refetched since the size and the hashes are not populated on the image returned
+    # when the snapshot is requested.
+    snapshot = conn.get_image(name_or_id=image_id)
+    downloaded_size = file_path.stat().st_size
+    if snapshot.size and downloaded_size != snapshot.size:
+        raise github_runner_image_builder.errors.DownloadImageError(
+            f"Incomplete snapshot download, expected {snapshot.size} bytes, "
+            f"got {downloaded_size} bytes."
+        )
+    algorithm, expected_hash = (
+        (snapshot.hash_algo, snapshot.hash_value)
+        # The algorithm is chosen by the cloud, fall back to the md5 checksum if it is one this
+        # runtime cannot compute.
+        if snapshot.hash_value and snapshot.hash_algo in hashlib.algorithms_available
+        else ("md5", snapshot.checksum)
+    )
+    if not expected_hash:
+        logger.warning("Snapshot %s exposes no hash, skipping download hash check.", image_id)
+        return
+    downloaded_hash = _hash_file(file_path=file_path, algorithm=algorithm)
+    if downloaded_hash != expected_hash:
+        raise github_runner_image_builder.errors.DownloadImageError(
+            f"Corrupt snapshot download, expected {algorithm}: {expected_hash}, "
+            f"got {downloaded_hash}."
+        )
+
+
+def _hash_file(file_path: pathlib.Path, algorithm: str) -> str:
+    """Compute the hash of a file.
+
+    Args:
+        file_path: The path of the file to hash.
+        algorithm: The name of the hash algorithm to use.
+
+    Returns:
+        The hexadecimal digest of the file.
+    """
+    # The hash is only used to detect a corrupt transfer, not for security purposes.
+    digest = hashlib.new(algorithm, usedforsecurity=False)
+    with open(file_path, "rb") as file:
+        for block in iter(lambda: file.read(HASH_BLOCK_SIZE), b""):
+            digest.update(block)
+    return digest.hexdigest()
+
+
 def _upload_to_clouds(
     conn: openstack.connection.Connection,
     image: openstack.image.v2.image.Image,
@@ -857,8 +922,16 @@ def _upload_to_clouds(
     if not upload_cloud_names:
         return (image,)
     file_path = pathlib.Path(f"{image.name}.snapshot")
-    logger.info("Downloading snapshot to %s.", file_path)
-    conn.download_image(name_or_id=image.id, output_file=file_path, stream=True)
+    # The snapshot is downloaded under a temporary name so that a partial download is never
+    # mistaken for a complete one, mirroring how the images themselves are uploaded.
+    tmp_file_path = file_path.with_name(f"{file_path.name}{TMP_SNAPSHOT_SUFFIX}")
+    try:
+        logger.info("Downloading snapshot to %s.", tmp_file_path)
+        conn.download_image(name_or_id=image.id, output_file=tmp_file_path, stream=True)
+        _validate_downloaded_snapshot(conn=conn, image_id=image.id, file_path=tmp_file_path)
+        tmp_file_path.replace(file_path)
+    finally:
+        tmp_file_path.unlink(missing_ok=True)
     images: list[openstack.image.v2.image.Image] = []
     for cloud_name in upload_cloud_names:
         logger.info("Uploading downloaded snapshot to %s.", cloud_name)

app/src/github_runner_image_builder/store.py

@@ -20,6 +20,11 @@
 
 # Timeout constants (in seconds)
 SNAPSHOT_CREATION_TIMEOUT = 60 * 30  # 30 minutes
+TMP_IMAGE_NAME_SUFFIX = "-tmp"
+FILE_MD5_PROPERTY = "owner_specified.openstack.md5"
+FILE_SHA256_PROPERTY = "owner_specified.openstack.sha256"
+ACTIVE_IMAGE_STATUS = "active"
+SHA256_ALGORITHM = "sha256"
 
 
 def create_snapshot(
@@ -74,19 +79,27 @@ def upload_image(
     Returns:
         The created image.
     """
+    tmp_image_name = f"{image_name}{TMP_IMAGE_NAME_SUFFIX}"
     with openstack.connect(cloud=cloud_name) as connection:
         try:
-            logger.info("Uploading image %s.", image_name)
+            _delete_images_by_name(connection=connection, image_name=tmp_image_name)
+
+            logger.info("Uploading image %s.", tmp_image_name)
             image_properties = {"architecture": arch.to_openstack()}
             # ignore type since the library does not provide correct type hinting but the docstring
             # does define the return type.
             image: Image = connection.create_image(
-                name=image_name,
+                name=tmp_image_name,
                 filename=str(image_path),
                 properties=image_properties,
                 allow_duplicates=True,
                 wait=True,
+                # Required for the locally computed hashes to be recorded on the image.
+                validate_checksum=True,
             )  # type: ignore
+            _validate_image_checksums(image=image)
+            logger.info("Renaming image %s to %s.", tmp_image_name, image_name)
+            image = cast(Image, connection.image.update_image(image, name=image_name))
             logger.info("Pruning older images %s, keeping %s.", image_name, keep_revisions)
             _prune_old_images(
                 connection=connection, image_name=image_name, num_revisions=keep_revisions
@@ -96,6 +109,66 @@ def upload_image(
         except openstack.exceptions.OpenStackCloudException as exc:
             logger.exception("Error while uploading image.")
             raise UploadImageError from exc
+        finally:
+            # The temporary image is renamed on success, meaning this only has an effect if the
+            # upload did not complete.
+            _delete_images_by_name_quietly(connection=connection, image_name=tmp_image_name)
+
+
+def _delete_images_by_name(connection: openstack.connection.Connection, image_name: str) -> None:
+    """Delete every image matching the given name.
+
+    Args:
+        connection: The connected openstack cloud instance.
+        image_name: The exact image name to delete.
+    """
+    for image in connection.image.images(name=image_name):
+        logger.info("Deleting image %s %s.", image_name, image.id)
+        connection.delete_image(image.id, wait=True)
+
+
+def _delete_images_by_name_quietly(
+    connection: openstack.connection.Connection, image_name: str
+) -> None:
+    """Delete every image matching the given name, logging instead of raising on failure.
+
+    Args:
+        connection: The connected openstack cloud instance.
+        image_name: The exact image name to delete.
+    """
+    try:
+        _delete_images_by_name(connection=connection, image_name=image_name)
+    except openstack.exceptions.OpenStackCloudException:
+        # Raising here would mask the original error, the leftover image is deleted on the next
+        # upload instead.
+        logger.exception("Failed to clean up image %s.", image_name)
+
+
+def _validate_image_checksums(image: Image) -> None:
+    """Compare the hashes Glance computed against the hashes computed locally during upload.
+
+    Args:
+        image: The uploaded image.
+
+    Raises:
+        UploadImageError: If the hashes are missing or do not match.
+    """
+    properties = image.properties or {}
+    local_md5 = properties.get(FILE_MD5_PROPERTY)
+    local_sha256 = properties.get(FILE_SHA256_PROPERTY)
+    if not local_md5 or not local_sha256:
+        raise UploadImageError(f"Image {image.name} is missing the locally computed hashes.")
+    if image.checksum != local_md5:
+        raise UploadImageError(
+            f"Checksum mismatch for image {image.name}, md5: {image.checksum} != {local_md5}."
+        )
+    # Glance computes the multihash with the algorithm it is configured with, which is not
+    # necessarily the sha256 that openstacksdk computes locally.
+    if image.hash_algo == SHA256_ALGORITHM and image.hash_value != local_sha256:
+        raise UploadImageError(
+            f"Checksum mismatch for image {image.name}, "
+            f"sha256: {image.hash_value} != {local_sha256}."
+        )
 
 
 def _prune_old_images(
@@ -111,7 +184,11 @@ def _prune_old_images(
     Raises:
         OpenstackError: if there was an error deleting the images.
     """
-    images = _get_sorted_images_by_created_at(connection=connection, image_name=image_name)
+    # Images left behind in a non-active status by a failed upload have to be pruned as well,
+    # otherwise they accumulate forever.
+    images = _get_sorted_images_by_created_at(
+        connection=connection, image_name=image_name, active_only=False
+    )
     if not images:
         return
     images_to_prune = images[num_revisions:]
@@ -140,6 +217,16 @@ def get_latest_build_id(cloud_name: str, image_name: str, active_only: bool = Tr
         images = _get_sorted_images_by_created_at(
             connection=connection, image_name=image_name, active_only=active_only
         )
+        if not active_only:
+            # An upload in progress is still under its temporary name.
+            images = _sort_images_by_created_at(
+                images
+                + _get_sorted_images_by_created_at(
+                    connection=connection,
+                    image_name=f"{image_name}{TMP_IMAGE_NAME_SUFFIX}",
+                    active_only=False,
+                )
+            )
         if not images:
             return ""
         # The type of ID is in string but the library does not provide correct type hints for it.
@@ -155,9 +242,8 @@ def _get_sorted_images_by_created_at(
 
     Args:
         connection: The connected openstack cloud instance.
-        image_name: The image name to search for.
-        active_only: If True (default), query only active images via search_images.
-            If False, query all images regardless of status via the image proxy API.
+        image_name: The exact image name to search for.
+        active_only: If True (default), only images that finished uploading are returned.
 
     Raises:
         OpenstackError: if there was an error fetching the images.
@@ -166,14 +252,24 @@ def _get_sorted_images_by_created_at(
         The images sorted by created_at date with latest first.
     """
     try:
-        if active_only:
-            images = cast(list[Image], connection.search_images(image_name))
-        else:
-            images = list(connection.image.images(name=image_name))
+        images = list(connection.image.images(name=image_name))
     except openstack.exceptions.OpenStackCloudException as exc:
         logger.exception("Failed to search images with name %s.", image_name)
         raise OpenstackError from exc
+    if active_only:
+        images = [image for image in images if image.status == ACTIVE_IMAGE_STATUS]
+    return _sort_images_by_created_at(images)
+
+
+def _sort_images_by_created_at(images: list[Image]) -> list[Image]:
+    """Sort images by their creation date, latest first.
+
+    Args:
+        images: The images to sort.
 
+    Returns:
+        The images sorted by created_at date with latest first.
+    """
     # The type of images are list[Image] but the library does not provide correct type hints for
     # it.
     return sorted(images, key=lambda image: image.created_at, reverse=True)  # type: ignore

app/tests/unit/factories.py

@@ -35,6 +35,14 @@ class Meta:  # pylint: disable=too-few-public-methods
 
     id: str  # UUID
     created_at = Faker("date")  # Example format: 2024-04-16T04:31:12Z
+    status = "active"
+    checksum = "test-md5"
+    hash_algo = "sha256"
+    hash_value = "test-sha256"
+    properties = {
+        "owner_specified.openstack.md5": "test-md5",
+        "owner_specified.openstack.sha256": "test-sha256",
+    }
 
 
 class MockRequestsReponseFactory(factory.Factory):