From: Zbigniew Jędrzejewski-Szmek Date: Mon, 18 Nov 2024 12:35:38 +0000 (+0100) Subject: ukify: fix parsing of SignTool configuration option X-Git-Tag: v257-rc3~87 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=5e7e4e4d49f38f8ceeef95ae8ea026abfa72cf73;p=thirdparty%2Fsystemd.git ukify: fix parsing of SignTool configuration option This partially reverts 02eabaffe98c9a3b5dec1c4837968a4d3e2ff7db. As noted in https://github.com/systemd/systemd/pull/35211: > The configuration parsing simply stores the string as-is, rather than > creating the appropriate object One way to fix the issue would be to store the "appropriate object", i.e. actually the class. But that makes the code very verbose, with the conversion being done in two places. And that still doesn't fix the issue, because we need to map the class objects back to the original name in error messages. So instead, store the setting as a string and only map it to the class much later. This makes the code simpler and fixes the error messages too. Resolves https://github.com/systemd/systemd/pull/35193 --- diff --git a/src/ukify/test/test_ukify.py b/src/ukify/test/test_ukify.py index 9eebf7eca1b..3ed21fc0ace 100755 --- a/src/ukify/test/test_ukify.py +++ b/src/ukify/test/test_ukify.py @@ -363,7 +363,7 @@ def test_config_priority(tmp_path): assert opts.pcr_public_keys == ['PKEY2', 'some/path8'] assert opts.pcr_banks == ['SHA1', 'SHA256'] assert opts.signing_engine == 'ENGINE' - assert opts.signtool == ukify.SbSign # from args + assert opts.signtool == 'sbsign' # from args assert opts.sb_key == 'SBKEY' # from args assert opts.sb_cert == pathlib.Path('SBCERT') # from args assert opts.sb_certdir == 'some/path5' # from config diff --git a/src/ukify/ukify.py b/src/ukify/ukify.py index 8d601e791e3..caa9a040004 100755 --- a/src/ukify/ukify.py +++ b/src/ukify/ukify.py @@ -267,7 +267,7 @@ class UkifyConfig: signing_engine: Optional[str] signing_provider: Optional[str] certificate_provider: Optional[str] - signtool: Optional[type['SignTool']] + signtool: Optional[str] splash: Optional[Path] stub: Path summary: bool @@ -466,6 +466,17 @@ class SignTool: def verify(opts: UkifyConfig) -> bool: raise NotImplementedError() + @staticmethod + def from_string(name) -> type['SignTool']: + if name == 'pesign': + return PeSign + elif name == 'sbsign': + return SbSign + elif name == 'systemd-sbsign': + return SystemdSbSign + else: + raise ValueError(f'Invalid sign tool: {name!r}') + class PeSign(SignTool): @staticmethod @@ -1141,15 +1152,16 @@ def make_uki(opts: UkifyConfig) -> None: if opts.linux and sign_args_present: assert opts.signtool is not None + signtool = SignTool.from_string(opts.signtool) if not sign_kernel: # figure out if we should sign the kernel - sign_kernel = opts.signtool.verify(opts) + sign_kernel = signtool.verify(opts) if sign_kernel: linux_signed = tempfile.NamedTemporaryFile(prefix='linux-signed') linux = Path(linux_signed.name) - opts.signtool.sign(os.fspath(opts.linux), os.fspath(linux), opts=opts) + signtool.sign(os.fspath(opts.linux), os.fspath(linux), opts=opts) if opts.uname is None and opts.linux is not None: print('Kernel version not specified, starting autodetection 😖.') @@ -1310,7 +1322,9 @@ def make_uki(opts: UkifyConfig) -> None: if sign_args_present: assert opts.signtool is not None - opts.signtool.sign(os.fspath(unsigned_output), os.fspath(opts.output), opts) + signtool = SignTool.from_string(opts.signtool) + + signtool.sign(os.fspath(unsigned_output), os.fspath(opts.output), opts) # We end up with no executable bits, let's reapply them os.umask(umask := os.umask(0)) @@ -1663,26 +1677,6 @@ class ConfigItem: return (section_name, key, value) -class SignToolAction(argparse.Action): - def __call__( - self, - parser: argparse.ArgumentParser, - namespace: argparse.Namespace, - values: Union[str, Sequence[Any], None] = None, - option_string: Optional[str] = None, - ) -> None: - if values is None: - setattr(namespace, 'signtool', None) - elif values == 'sbsign': - setattr(namespace, 'signtool', SbSign) - elif values == 'pesign': - setattr(namespace, 'signtool', PeSign) - elif values == 'systemd-sbsign': - setattr(namespace, 'signtool', SystemdSbSign) - else: - raise ValueError(f"Unknown signtool '{values}' (this is unreachable)") - - VERBS = ('build', 'genkey', 'inspect') CONFIG_ITEMS = [ @@ -1856,7 +1850,6 @@ CONFIG_ITEMS = [ ConfigItem( '--signtool', choices=('sbsign', 'pesign', 'systemd-sbsign'), - action=SignToolAction, dest='signtool', help=( 'whether to use sbsign or pesign. It will also be inferred by the other ' @@ -2173,24 +2166,24 @@ def finalize_options(opts: argparse.Namespace) -> None: ) elif bool(opts.sb_key) and bool(opts.sb_cert): # both param given, infer sbsign and in case it was given, ensure signtool=sbsign - if opts.signtool and opts.signtool not in (SbSign, SystemdSbSign): + if opts.signtool and opts.signtool not in ('sbsign', 'systemd-sbsign'): raise ValueError( f'Cannot provide --signtool={opts.signtool} with --secureboot-private-key= and --secureboot-certificate=' # noqa: E501 ) if not opts.signtool: - opts.signtool = SbSign + opts.signtool = 'sbsign' elif bool(opts.sb_cert_name): # sb_cert_name given, infer pesign and in case it was given, ensure signtool=pesign - if opts.signtool and opts.signtool != PeSign: + if opts.signtool and opts.signtool != 'pesign': raise ValueError( f'Cannot provide --signtool={opts.signtool} with --secureboot-certificate-name=' ) - opts.signtool = PeSign + opts.signtool = 'pesign' - if opts.signing_provider and opts.signtool != SystemdSbSign: + if opts.signing_provider and opts.signtool != 'systemd-sbsign': raise ValueError('--signing-provider= can only be used with--signtool=systemd-sbsign') - if opts.certificate_provider and opts.signtool != SystemdSbSign: + if opts.certificate_provider and opts.signtool != 'systemd-sbsign': raise ValueError('--certificate-provider= can only be used with--signtool=systemd-sbsign') if opts.sign_kernel and not opts.sb_key and not opts.sb_cert_name: