From 6f1df1ff3cf399f3b69a3c15b348e2337bc49a5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Dec 2023 19:01:27 +0100 Subject: [PATCH 1/4] test_ukify: use Path-based fixtures Quoting https://docs.pytest.org/en/stable/how-to/tmp_path.html#the-default-base-temporary-directory: > The tmpdir and tmpdir_factory fixtures are similar to tmp_path and > tmp_path_factory, but use/return legacy py.path.local objects rather than > standard pathlib.Path objects. > > These days, it is preferred to use tmp_path and tmp_path_factory. --- src/ukify/test/test_ukify.py | 92 ++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/src/ukify/test/test_ukify.py b/src/ukify/test/test_ukify.py index 3a60a21f55..842888f5b1 100755 --- a/src/ukify/test/test_ukify.py +++ b/src/ukify/test/test_ukify.py @@ -418,11 +418,11 @@ def test_check_splash(): with pytest.raises(OSError): ukify.check_splash(os.devnull) -def test_basic_operation(kernel_initrd, tmpdir): +def test_basic_operation(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') - output = f'{tmpdir}/basic.efi' + output = f'{tmp_path}/basic.efi' opts = ukify.parse_args([ 'build', *kernel_initrd, @@ -438,11 +438,11 @@ def test_basic_operation(kernel_initrd, tmpdir): # let's check that objdump likes the resulting file subprocess.check_output(['objdump', '-h', output]) -def test_sections(kernel_initrd, tmpdir): +def test_sections(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') - output = f'{tmpdir}/basic.efi' + output = f'{tmp_path}/basic.efi' opts = ukify.parse_args([ 'build', *kernel_initrd, @@ -466,8 +466,8 @@ def test_sections(kernel_initrd, tmpdir): for sect in 'text osrel cmdline linux initrd uname test'.split(): assert re.search(fr'^\s*\d+\s+\.{sect}\s+[0-9a-f]+', dump, re.MULTILINE) -def test_addon(tmpdir): - output = f'{tmpdir}/addon.efi' +def test_addon(tmp_path): + output = f'{tmp_path}/addon.efi' args = [ 'build', f'--output={output}', @@ -530,7 +530,7 @@ def test_uname_scraping(kernel_initrd): assert re.match(r'\d+\.\d+\.\d+', uname) @pytest.mark.parametrize("days", [365*10, None]) -def test_efi_signing_sbsign(days, kernel_initrd, tmpdir): +def test_efi_signing_sbsign(days, kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') if not shutil.which('sbsign'): @@ -540,7 +540,7 @@ def test_efi_signing_sbsign(days, kernel_initrd, tmpdir): cert = unbase64(ourdir / 'example.signing.crt.base64') key = unbase64(ourdir / 'example.signing.key.base64') - output = f'{tmpdir}/signed.efi' + output = f'{tmp_path}/signed.efi' args = [ 'build', *kernel_initrd, @@ -572,13 +572,13 @@ def test_efi_signing_sbsign(days, kernel_initrd, tmpdir): assert 'Signature verification OK' in dump -def test_efi_signing_pesign(kernel_initrd, tmpdir): +def test_efi_signing_pesign(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') if not shutil.which('pesign'): pytest.skip('pesign not found') - nss_db = f'{tmpdir}/nss_db' + nss_db = f'{tmp_path}/nss_db' name = 'Test_Secureboot' author = 'systemd' @@ -588,7 +588,7 @@ def test_efi_signing_pesign(kernel_initrd, tmpdir): cmd = f'efikeygen -d {nss_db} -S -k -c CN={author} -n {name}'.split(' ') subprocess.check_call(cmd) - output = f'{tmpdir}/signed.efi' + output = f'{tmp_path}/signed.efi' opts = ukify.parse_args([ 'build', *kernel_initrd, @@ -615,7 +615,7 @@ def test_efi_signing_pesign(kernel_initrd, tmpdir): assert f"The signer's common name is {author}" in dump -def test_inspect(kernel_initrd, tmpdir, capsys): +def test_inspect(kernel_initrd, tmp_path, capsys): if kernel_initrd is None: pytest.skip('linux+initrd not found') if not shutil.which('sbsign'): @@ -625,7 +625,7 @@ def test_inspect(kernel_initrd, tmpdir, capsys): cert = unbase64(ourdir / 'example.signing.crt.base64') key = unbase64(ourdir / 'example.signing.key.base64') - output = f'{tmpdir}/signed2.efi' + output = f'{tmp_path}/signed2.efi' uname_arg='1.2.3' osrel_arg='Linux' cmdline_arg='ARG1 ARG2 ARG3' @@ -661,7 +661,7 @@ def test_inspect(kernel_initrd, tmpdir, capsys): assert expected_linux in text -def test_pcr_signing(kernel_initrd, tmpdir): +def test_pcr_signing(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') if systemd_measure() is None: @@ -671,7 +671,7 @@ def test_pcr_signing(kernel_initrd, tmpdir): pub = unbase64(ourdir / 'example.tpm2-pcr-public.pem.base64') priv = unbase64(ourdir / 'example.tpm2-pcr-private.pem.base64') - output = f'{tmpdir}/signed.efi' + output = f'{tmp_path}/signed.efi' args = [ 'build', *kernel_initrd, @@ -708,23 +708,23 @@ def test_pcr_signing(kernel_initrd, tmpdir): # So let's just call it with a dummy output argument. subprocess.check_call([ 'objcopy', - *(f'--dump-section=.{n}={tmpdir}/out.{n}' for n in ( + *(f'--dump-section=.{n}={tmp_path}/out.{n}' for n in ( 'pcrpkey', 'pcrsig', 'osrel', 'uname', 'cmdline')), output, - tmpdir / 'dummy', + tmp_path / 'dummy', ], text=True) - assert open(tmpdir / 'out.pcrpkey').read() == open(pub.name).read() - assert open(tmpdir / 'out.osrel').read() == 'ID=foobar\n' - assert open(tmpdir / 'out.uname').read() == '1.2.3' - assert open(tmpdir / 'out.cmdline').read() == 'ARG1 ARG2 ARG3' - sig = open(tmpdir / 'out.pcrsig').read() + assert open(tmp_path / 'out.pcrpkey').read() == open(pub.name).read() + assert open(tmp_path / 'out.osrel').read() == 'ID=foobar\n' + assert open(tmp_path / 'out.uname').read() == '1.2.3' + assert open(tmp_path / 'out.cmdline').read() == 'ARG1 ARG2 ARG3' + sig = open(tmp_path / 'out.pcrsig').read() sig = json.loads(sig) assert list(sig.keys()) == ['sha1'] assert len(sig['sha1']) == 4 # four items for four phases -def test_pcr_signing2(kernel_initrd, tmpdir): +def test_pcr_signing2(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') if systemd_measure() is None: @@ -737,10 +737,10 @@ def test_pcr_signing2(kernel_initrd, tmpdir): priv2 = unbase64(ourdir / 'example.tpm2-pcr-private2.pem.base64') # simulate a microcode file - with open(f'{tmpdir}/microcode', 'wb') as microcode: + with open(f'{tmp_path}/microcode', 'wb') as microcode: microcode.write(b'1234567890') - output = f'{tmpdir}/signed.efi' + output = f'{tmp_path}/signed.efi' assert kernel_initrd[0] == '--linux' opts = ukify.parse_args([ 'build', @@ -776,35 +776,35 @@ def test_pcr_signing2(kernel_initrd, tmpdir): subprocess.check_call([ 'objcopy', - *(f'--dump-section=.{n}={tmpdir}/out.{n}' for n in ( + *(f'--dump-section=.{n}={tmp_path}/out.{n}' for n in ( 'pcrpkey', 'pcrsig', 'osrel', 'uname', 'cmdline', 'initrd')), output, - tmpdir / 'dummy', + tmp_path / 'dummy', ], text=True) - assert open(tmpdir / 'out.pcrpkey').read() == open(pub2.name).read() - assert open(tmpdir / 'out.osrel').read() == 'ID=foobar\n' - assert open(tmpdir / 'out.uname').read() == '1.2.3' - assert open(tmpdir / 'out.cmdline').read() == 'ARG1 ARG2 ARG3' - assert open(tmpdir / 'out.initrd', 'rb').read(10) == b'1234567890' + assert open(tmp_path / 'out.pcrpkey').read() == open(pub2.name).read() + assert open(tmp_path / 'out.osrel').read() == 'ID=foobar\n' + assert open(tmp_path / 'out.uname').read() == '1.2.3' + assert open(tmp_path / 'out.cmdline').read() == 'ARG1 ARG2 ARG3' + assert open(tmp_path / 'out.initrd', 'rb').read(10) == b'1234567890' - sig = open(tmpdir / 'out.pcrsig').read() + sig = open(tmp_path / 'out.pcrsig').read() sig = json.loads(sig) assert list(sig.keys()) == ['sha1'] assert len(sig['sha1']) == 6 # six items for six phases paths -def test_key_cert_generation(tmpdir): +def test_key_cert_generation(tmp_path): opts = ukify.parse_args([ 'genkey', - f"--pcr-public-key={tmpdir / 'pcr1.pub.pem'}", - f"--pcr-private-key={tmpdir / 'pcr1.priv.pem'}", + f"--pcr-public-key={tmp_path / 'pcr1.pub.pem'}", + f"--pcr-private-key={tmp_path / 'pcr1.priv.pem'}", '--phases=enter-initrd enter-initrd:leave-initrd', - f"--pcr-public-key={tmpdir / 'pcr2.pub.pem'}", - f"--pcr-private-key={tmpdir / 'pcr2.priv.pem'}", + f"--pcr-public-key={tmp_path / 'pcr2.pub.pem'}", + f"--pcr-private-key={tmp_path / 'pcr2.priv.pem'}", '--phases=sysinit ready', - f"--secureboot-private-key={tmpdir / 'sb.priv.pem'}", - f"--secureboot-certificate={tmpdir / 'sb.cert.pem'}", + f"--secureboot-private-key={tmp_path / 'sb.priv.pem'}", + f"--secureboot-certificate={tmp_path / 'sb.cert.pem'}", ]) assert opts.verb == 'genkey' ukify.check_cert_and_keys_nonexistent(opts) @@ -816,9 +816,9 @@ def test_key_cert_generation(tmpdir): if not shutil.which('openssl'): return - for key in (tmpdir / 'pcr1.priv.pem', - tmpdir / 'pcr2.priv.pem', - tmpdir / 'sb.priv.pem'): + for key in (tmp_path / 'pcr1.priv.pem', + tmp_path / 'pcr2.priv.pem', + tmp_path / 'sb.priv.pem'): out = subprocess.check_output([ 'openssl', 'rsa', '-in', key, @@ -828,8 +828,8 @@ def test_key_cert_generation(tmpdir): assert 'Private-Key' in out assert '2048 bit' in out - for pub in (tmpdir / 'pcr1.pub.pem', - tmpdir / 'pcr2.pub.pem'): + for pub in (tmp_path / 'pcr1.pub.pem', + tmp_path / 'pcr2.pub.pem'): out = subprocess.check_output([ 'openssl', 'rsa', '-pubin', @@ -842,7 +842,7 @@ def test_key_cert_generation(tmpdir): out = subprocess.check_output([ 'openssl', 'x509', - '-in', tmpdir / 'sb.cert.pem', + '-in', tmp_path / 'sb.cert.pem', '-text', '-noout', ], text = True) From ef0a69eb90292ec3fa11564fe6005ed06121f260 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Dec 2023 19:10:09 +0100 Subject: [PATCH 2/4] test_ukify: explicitly remove big temporary directories pytest intentionally keeps around a limited number of the previous test temporary directories [1]. This is generally OK, but in our tests that generate initrds, we create a few very large files (both the initrd and kernel in a few copies), which quickly adds up. I had a particularly large initrd (because of some mkosi-initrd shenanigans), and I unded up with dozens of gigabytes of temporary files from the tests. Let's just nuke the dirs where we write kernel data. [1] https://github.com/pytest-dev/pytest/issues/543 --- src/ukify/test/test_ukify.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/ukify/test/test_ukify.py b/src/ukify/test/test_ukify.py index 842888f5b1..bd721da666 100755 --- a/src/ukify/test/test_ukify.py +++ b/src/ukify/test/test_ukify.py @@ -438,6 +438,8 @@ def test_basic_operation(kernel_initrd, tmp_path): # let's check that objdump likes the resulting file subprocess.check_output(['objdump', '-h', output]) + shutil.rmtree(tmp_path) + def test_sections(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') @@ -466,6 +468,8 @@ def test_sections(kernel_initrd, tmp_path): for sect in 'text osrel cmdline linux initrd uname test'.split(): assert re.search(fr'^\s*\d+\s+\.{sect}\s+[0-9a-f]+', dump, re.MULTILINE) + shutil.rmtree(tmp_path) + def test_addon(tmp_path): output = f'{tmp_path}/addon.efi' args = [ @@ -572,6 +576,8 @@ def test_efi_signing_sbsign(days, kernel_initrd, tmp_path): assert 'Signature verification OK' in dump + shutil.rmtree(tmp_path) + def test_efi_signing_pesign(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') @@ -615,6 +621,8 @@ def test_efi_signing_pesign(kernel_initrd, tmp_path): assert f"The signer's common name is {author}" in dump + shutil.rmtree(tmp_path) + def test_inspect(kernel_initrd, tmp_path, capsys): if kernel_initrd is None: pytest.skip('linux+initrd not found') @@ -660,6 +668,7 @@ def test_inspect(kernel_initrd, tmp_path, capsys): expected_linux = '.linux:\n size:' assert expected_linux in text + shutil.rmtree(tmp_path) def test_pcr_signing(kernel_initrd, tmp_path): if kernel_initrd is None: @@ -724,6 +733,8 @@ def test_pcr_signing(kernel_initrd, tmp_path): assert list(sig.keys()) == ['sha1'] assert len(sig['sha1']) == 4 # four items for four phases + shutil.rmtree(tmp_path) + def test_pcr_signing2(kernel_initrd, tmp_path): if kernel_initrd is None: pytest.skip('linux+initrd not found') @@ -794,6 +805,8 @@ def test_pcr_signing2(kernel_initrd, tmp_path): assert list(sig.keys()) == ['sha1'] assert len(sig['sha1']) == 6 # six items for six phases paths + shutil.rmtree(tmp_path) + def test_key_cert_generation(tmp_path): opts = ukify.parse_args([ 'genkey', From fd9ccc07a69994e05b85f1dbfb94e6e2b04f22a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Dec 2023 19:15:50 +0100 Subject: [PATCH 3/4] test_ukify: formatting --- src/ukify/test/test_ukify.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/ukify/test/test_ukify.py b/src/ukify/test/test_ukify.py index bd721da666..7db7c6ba61 100755 --- a/src/ukify/test/test_ukify.py +++ b/src/ukify/test/test_ukify.py @@ -517,14 +517,12 @@ baz,3 assert found is True - def unbase64(filename): tmp = tempfile.NamedTemporaryFile() base64.decode(filename.open('rb'), tmp) tmp.flush() return tmp - def test_uname_scraping(kernel_initrd): if kernel_initrd is None: pytest.skip('linux+initrd not found') @@ -692,8 +690,9 @@ def test_pcr_signing(kernel_initrd, tmp_path): f'--pcr-private-key={priv.name}', ] + arg_tools - # If the public key is not explicitly specified, it is derived automatically. Let's make sure everything - # works as expected both when the public keys is specified explicitly and when it is derived from the + # If the public key is not explicitly specified, it is derived + # automatically. Let's make sure everything works as expected both when the + # public keys is specified explicitly and when it is derived from the # private key. for extra in ([f'--pcrpkey={pub.name}', f'--pcr-public-key={pub.name}'], []): opts = ukify.parse_args(args + extra) From 6bc99ec29b8512a668f0c82b7e2731f0335c91d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 8 Dec 2023 19:17:14 +0100 Subject: [PATCH 4/4] test_ukify: raise timeout With a sufficiently large initrd, the tests take 25 s on my laptop. Normally, they'd be quicker, but since we use what we find on the system, we don't control this. Let's raise the timeout to reduce the chances of a spurious failure. --- src/ukify/test/meson.build | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ukify/test/meson.build b/src/ukify/test/meson.build index 5870ccc06c..2df196b477 100644 --- a/src/ukify/test/meson.build +++ b/src/ukify/test/meson.build @@ -16,5 +16,6 @@ if want_ukify and want_tests != 'false' files('test_ukify.py'), args: args, env : test_env, + timeout : 120, suite : 'ukify') endif