From 3b2c6ae72a0adc27e1cfc622ecdf4ec6827a6079 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Tue, 30 Apr 2024 14:15:08 +0200 Subject: [PATCH] createiso: Recompute .treeinfo checksums for images MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running xorriso to modify an ISO image can update content of included images such as images/eltorito.img, unless we explicitly update the image, which is undesirable (https://pagure.io/pungi/issue/1647). However, when the file is changed, the checksum changes and .treeinfo no longer matches. This patch implements a workaround: once the DVD is written, it looks for incorrect checksums, recalculates them and updates the .treeinfo on the DVD. Since only the checksum is changing and the size of the file remains the same, this seems to help fix the issue. An additional step for implanting MD5 is needed again, as that gets erased by the workaround. JIRA: RHELCMP-13664 Signed-off-by: Lubomír Sedlář --- pungi/createiso.py | 16 ++---- pungi/phases/createiso.py | 86 ++++++++++++++++++++++++++- pungi/phases/extra_isos.py | 1 + pungi/wrappers/iso.py | 18 ++++++ tests/helpers.py | 7 +++ tests/test_createiso_phase.py | 102 +++++++++++++++++++++++++++++++++ tests/test_extra_isos_phase.py | 20 +++++-- 7 files changed, 230 insertions(+), 20 deletions(-) diff --git a/pungi/createiso.py b/pungi/createiso.py index 26f7a643..e68932eb 100644 --- a/pungi/createiso.py +++ b/pungi/createiso.py @@ -159,15 +159,11 @@ def write_xorriso_commands(opts): script = os.path.join(opts.script_dir, "xorriso-%s.txt" % id(opts)) with open(script, "w") as f: - emit(f, "-indev %s" % opts.boot_iso) - emit(f, "-outdev %s" % os.path.join(opts.output_dir, opts.iso_name)) - emit(f, "-boot_image any replay") + for cmd in iso.xorriso_commands( + opts.arch, opts.boot_iso, os.path.join(opts.output_dir, opts.iso_name) + ): + emit(f, " ".join(cmd)) emit(f, "-volid %s" % opts.volid) - # isoinfo -J uses the Joliet tree, and it's used by virt-install - emit(f, "-joliet on") - # Support long filenames in the Joliet trees. Repodata is particularly - # likely to run into this limit. - emit(f, "-compliance joliet_long_names") with open(opts.graft_points) as gp: for line in gp: @@ -178,10 +174,6 @@ def write_xorriso_commands(opts): emit(f, "%s %s %s" % (cmd, fs_path, iso_path)) emit(f, "-chmod 0%o %s" % (_get_perms(fs_path), iso_path)) - if opts.arch == "ppc64le": - # This is needed for the image to be bootable. - emit(f, "-as mkisofs -U --") - emit(f, "-chown_r 0 /") emit(f, "-chgrp_r 0 /") emit(f, "-end") diff --git a/pungi/phases/createiso.py b/pungi/phases/createiso.py index dd197899..ec653ad6 100644 --- a/pungi/phases/createiso.py +++ b/pungi/phases/createiso.py @@ -14,6 +14,7 @@ # along with this program; if not, see . +import itertools import os import random import shutil @@ -23,7 +24,7 @@ import json import productmd.treeinfo from productmd.images import Image from kobo.threads import ThreadPool, WorkerThread -from kobo.shortcuts import run, relative_path +from kobo.shortcuts import run, relative_path, compute_file_checksums from six.moves import shlex_quote from pungi.wrappers import iso @@ -457,7 +458,14 @@ class CreateIsoThread(WorkerThread): try: run_createiso_command( - num, compose, bootable, arch, cmd["cmd"], mounts, log_file + num, + compose, + bootable, + arch, + cmd["cmd"], + mounts, + log_file, + cmd["iso_path"], ) except Exception: self.fail(compose, cmd, variant, arch) @@ -538,7 +546,9 @@ def add_iso_to_metadata( return img -def run_createiso_command(num, compose, bootable, arch, cmd, mounts, log_file): +def run_createiso_command( + num, compose, bootable, arch, cmd, mounts, log_file, iso_path +): packages = [ "coreutils", "xorriso" if compose.conf.get("createiso_use_xorrisofs") else "genisoimage", @@ -580,6 +590,76 @@ def run_createiso_command(num, compose, bootable, arch, cmd, mounts, log_file): weight=compose.conf["runroot_weights"].get("createiso"), ) + if bootable and compose.conf.get("createiso_use_xorrisofs"): + fix_treeinfo_checksums(compose, iso_path, arch) + + +def fix_treeinfo_checksums(compose, iso_path, arch): + """It is possible for the ISO to contain a .treefile with incorrect + checksums. By modifying the ISO (adding files) some of the images may + change. + + This function fixes that after the fact by looking for incorrect checksums, + recalculating them and updating the .treeinfo file. Since the size of the + file doesn't change, this seems to not change any images. + """ + modified = False + with iso.mount(iso_path, compose._logger) as mountpoint: + ti = productmd.TreeInfo() + ti.load(os.path.join(mountpoint, ".treeinfo")) + for image, (type_, expected) in ti.checksums.checksums.items(): + checksums = compute_file_checksums(os.path.join(mountpoint, image), [type_]) + actual = checksums[type_] + if actual == expected: + # Everything fine here, skip to next image. + continue + + compose.log_debug("%s: %s: checksum mismatch", iso_path, image) + # Update treeinfo with correct checksum + ti.checksums.checksums[image] = (type_, actual) + modified = True + + if not modified: + compose.log_debug("%s: All checksums match, nothing to do.", iso_path) + return + + try: + tmpdir = compose.mkdtemp(arch, prefix="fix-checksum-") + # Write modified .treeinfo + ti_path = os.path.join(tmpdir, ".treeinfo") + compose.log_debug("Storing modified .treeinfo in %s", ti_path) + ti.dump(ti_path) + # Write a modified DVD into a temporary path, that is atomically moved + # over the original file. + fixed_path = os.path.join(tmpdir, "fixed-checksum-dvd.iso") + cmd = ["xorriso"] + cmd.extend( + itertools.chain.from_iterable( + iso.xorriso_commands(arch, iso_path, fixed_path) + ) + ) + cmd.extend(["-map", ti_path, ".treeinfo"]) + run( + cmd, + logfile=compose.paths.log.log_file( + arch, "checksum-fix_generate_%s" % os.path.basename(iso_path) + ), + ) + # The modified ISO no longer has implanted MD5, so that needs to be + # fixed again. + compose.log_debug("Implanting new MD5 to %s fixed_path") + run( + iso.get_implantisomd5_cmd(fixed_path, compose.supported), + logfile=compose.paths.log.log_file( + arch, "checksum-fix_implantisomd5_%s" % os.path.basename(iso_path) + ), + ) + # All done, move the updated image to the final location. + compose.log_debug("Updating %s", iso_path) + os.rename(fixed_path, iso_path) + finally: + shutil.rmtree(tmpdir) + def split_iso(compose, arch, variant, no_split=False, logger=None): """ diff --git a/pungi/phases/extra_isos.py b/pungi/phases/extra_isos.py index 3b0e25ec..da396d2b 100644 --- a/pungi/phases/extra_isos.py +++ b/pungi/phases/extra_isos.py @@ -166,6 +166,7 @@ class ExtraIsosThread(WorkerThread): log_file=compose.paths.log.log_file( arch, "extraiso-%s" % os.path.basename(iso_path) ), + iso_path=iso_path, ) img = add_iso_to_metadata( diff --git a/pungi/wrappers/iso.py b/pungi/wrappers/iso.py index ab273027..1f017b7c 100644 --- a/pungi/wrappers/iso.py +++ b/pungi/wrappers/iso.py @@ -516,3 +516,21 @@ def mount(image, logger=None, use_guestmount=True): util.run_unmount_cmd(["fusermount", "-u", mount_dir], path=mount_dir) else: util.run_unmount_cmd(["umount", mount_dir], path=mount_dir) + + +def xorriso_commands(arch, input, output): + """List of xorriso commands to modify a bootable image.""" + commands = [ + ("-indev", input), + ("-outdev", output), + # isoinfo -J uses the Joliet tree, and it's used by virt-install + ("-joliet", "on"), + # Support long filenames in the Joliet trees. Repodata is particularly + # likely to run into this limit. + ("-compliance", "joliet_long_names"), + ("-boot_image", "any", "replay"), + ] + if arch == "ppc64le": + # This is needed for the image to be bootable. + commands.append(("-as", "mkisofs", "-U", "--")) + return commands diff --git a/tests/helpers.py b/tests/helpers.py index 41df06e8..5fc788bd 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -2,6 +2,7 @@ import difflib import errno +import hashlib import os import shutil import tempfile @@ -364,3 +365,9 @@ def fake_run_in_threads(func, params, threads=None): """Like run_in_threads from Kobo, but actually runs tasks serially.""" for num, param in enumerate(params): func(None, param, num) + + +def hash_string(alg, s): + m = hashlib.new(alg) + m.update(s.encode("utf-8")) + return m.hexdigest() diff --git a/tests/test_createiso_phase.py b/tests/test_createiso_phase.py index d135f811..bb9b6892 100644 --- a/tests/test_createiso_phase.py +++ b/tests/test_createiso_phase.py @@ -2,6 +2,7 @@ import logging +import contextlib try: from unittest import mock @@ -9,6 +10,7 @@ except ImportError: import mock import six +import productmd import os from tests import helpers @@ -1607,3 +1609,103 @@ class ComposeConfGetIsoLevelTest(helpers.PungiTestCase): compose, compose.variants["Client"], "x86_64" ), ) + + +def mk_mount(topdir, images): + @contextlib.contextmanager + def dummy_mount(path, logger): + treeinfo = [ + "[general]", + "family = Test", + "version = 1.0", + "arch = x86_64", + "variant = Server", + "[checksums]", + ] + for image in images: + helpers.touch(os.path.join(topdir, image.path), image.content) + treeinfo.append("%s = sha256:%s" % (image.path, image.checksum)) + helpers.touch(os.path.join(topdir, ".treeinfo"), "\n".join(treeinfo)) + yield topdir + + return dummy_mount + + +class _MockRun: + """This class replaces kobo.shortcuts.run and validates that the correct + two commands are called. The assertions can not be done after the tested + function finishes because it will clean up the .treeinfo file that needs to + be checked. + """ + + def __init__(self): + self.num_calls = 0 + self.asserts = [self._assert_xorriso, self._assert_implantisomd5] + + def __call__(self, cmd, logfile): + self.num_calls += 1 + self.asserts.pop(0)(cmd) + + def _assert_xorriso(self, cmd): + assert cmd[0] == "xorriso" + ti = productmd.TreeInfo() + input_iso = None + for i, arg in enumerate(cmd): + if arg == "-map": + ti.load(cmd[i + 1]) + if arg == "-outdev": + self.temp_iso = cmd[i + 1] + if arg == "-indev": + input_iso = cmd[i + 1] + assert self.input_iso == input_iso + assert ti.checksums.checksums[self.image_relative_path] == self.image_checksum + + def _assert_implantisomd5(self, cmd): + assert cmd[0] == "/usr/bin/implantisomd5" + assert cmd[-1] == self.temp_iso + + +class DummyImage: + def __init__(self, path, content, checksum=None): + self.path = path + self.content = content + self.checksum = checksum or helpers.hash_string("sha256", content) + + +@mock.patch("os.rename") +@mock.patch("pungi.phases.createiso.run", new_callable=_MockRun) +class FixChecksumsTest(helpers.PungiTestCase): + def test_checksum_matches(self, mock_run, mock_rename): + compose = helpers.DummyCompose(self.topdir, {}) + arch = "x86_64" + iso_path = "DUMMY_ISO" + + with mock.patch( + "pungi.wrappers.iso.mount", + new=mk_mount(self.topdir, [DummyImage("images/eltorito.img", "eltorito")]), + ): + createiso.fix_treeinfo_checksums(compose, iso_path, arch) + + self.assertEqual(mock_run.num_calls, 0) + self.assertEqual(mock_rename.call_args_list, []) + + def test_checksum_fix(self, mock_run, mock_rename): + compose = helpers.DummyCompose(self.topdir, {}) + arch = "x86_64" + img = "images/eltorito.img" + content = "eltorito" + iso_path = "DUMMY_ISO" + mock_run.input_iso = iso_path + mock_run.image_relative_path = "images/eltorito.img" + mock_run.image_checksum = ("sha256", helpers.hash_string("sha256", content)) + + with mock.patch( + "pungi.wrappers.iso.mount", + new=mk_mount(self.topdir, [DummyImage(img, content, "abc")]), + ): + createiso.fix_treeinfo_checksums(compose, iso_path, arch) + + # The new image was copied over the old one + self.assertEqual( + mock_rename.call_args_list, [mock.call(mock_run.temp_iso, iso_path)] + ) diff --git a/tests/test_extra_isos_phase.py b/tests/test_extra_isos_phase.py index 9d60eee3..b90deeb0 100644 --- a/tests/test_extra_isos_phase.py +++ b/tests/test_extra_isos_phase.py @@ -134,6 +134,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): ), ], ) + iso_path = os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso") self.assertEqual( rcc.call_args_list, [ @@ -152,6 +153,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): log_file=os.path.join( self.topdir, "logs/x86_64/extraiso-my.iso.x86_64.log" ), + iso_path=iso_path, ) ], ) @@ -162,7 +164,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): compose, server, "x86_64", - os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso"), + iso_path, True, additional_variants=["Client"], ) @@ -209,6 +211,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): ), ], ) + iso_path = os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso") self.assertEqual( rcc.call_args_list, [ @@ -227,6 +230,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): log_file=os.path.join( self.topdir, "logs/x86_64/extraiso-my.iso.x86_64.log" ), + iso_path=iso_path, ) ], ) @@ -237,7 +241,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): compose, server, "x86_64", - os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso"), + iso_path, True, additional_variants=["Client"], ) @@ -282,6 +286,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): ), ], ) + iso_path = os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso") self.assertEqual( rcc.call_args_list, [ @@ -300,6 +305,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): log_file=os.path.join( self.topdir, "logs/x86_64/extraiso-my.iso.x86_64.log" ), + iso_path=iso_path, ) ], ) @@ -310,7 +316,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): compose, server, "x86_64", - os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso"), + iso_path, True, additional_variants=["Client"], ) @@ -357,6 +363,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): ), ], ) + iso_path = os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso") self.assertEqual( rcc.call_args_list, [ @@ -375,6 +382,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): log_file=os.path.join( self.topdir, "logs/x86_64/extraiso-my.iso.x86_64.log" ), + iso_path=iso_path, ) ], ) @@ -385,7 +393,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): compose, server, "x86_64", - os.path.join(self.topdir, "compose/Server/x86_64/iso/my.iso"), + iso_path, False, additional_variants=["Client"], ) @@ -427,6 +435,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): ), ], ) + iso_path = os.path.join(self.topdir, "compose/Server/source/iso/my.iso") self.assertEqual( rcc.call_args_list, [ @@ -445,6 +454,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): log_file=os.path.join( self.topdir, "logs/src/extraiso-my.iso.src.log" ), + iso_path=iso_path, ) ], ) @@ -455,7 +465,7 @@ class ExtraIsosThreadTest(helpers.PungiTestCase): compose, server, "src", - os.path.join(self.topdir, "compose/Server/source/iso/my.iso"), + iso_path, False, additional_variants=["Client"], )