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 9eebf7eca1b22f7001aa10b9215f76478f4efd74..3ed21fc0ace5043795a2689740230acec1d812b3 100755 (executable)
--- 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 8d601e791e321e783bb91d3fff13c1ed40ee67b2..caa9a0400044680c41f83c5dac263e6175174bf2 100755 (executable)
--- 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: