Merge pull request #29332 from esposem/ukify_simplify

ukify: automatically infer --signtool from the parameters given

src/ukify/test/test_ukify.py

@@ -266,6 +266,7 @@ def test_parse_sections():
 
 def test_config_priority(tmp_path):
     config = tmp_path / 'config1.conf'
+    # config: use pesign and give certdir + certname
     config.write_text(textwrap.dedent(
         f'''
         [UKI]
@@ -282,10 +283,8 @@ def test_config_priority(tmp_path):
         Stub = some/path4
         PCRBanks = sha512,sha1
         SigningEngine = engine1
-        SignTool = pesign
-        SecureBootPrivateKey = some/path5
-        SecureBootCertificate = some/path6
-        SecureBootCertificateDir = some/path7
+        SecureBootSigningTool = pesign
+        SecureBootCertificateDir = some/path5
         SecureBootCertificateName = some/name1
         SignKernel = no
 
@@ -295,6 +294,7 @@ def test_config_priority(tmp_path):
         Phases = {':'.join(ukify.KNOWN_PHASES)}
         '''))
 
+    # args: use sbsign and give key + cert, should override pesign
     opts = ukify.parse_args(
         ['build',
          '--linux=/ARG1',
@@ -311,11 +311,9 @@ def test_config_priority(tmp_path):
          '--pcr-public-key=PKEY2',
          '--pcr-banks=SHA1,SHA256',
          '--signing-engine=ENGINE',
-         '--signtool=pesign',
+         '--signtool=sbsign',
          '--secureboot-private-key=SBKEY',
          '--secureboot-certificate=SBCERT',
-         '--secureboot-certificate-dir=SBPATH',
-         '--secureboot-certificate-name=SBNAME',
          '--sign-kernel',
          '--no-sign-kernel',
          '--tools=TOOLZ///',
@@ -345,11 +343,11 @@ def test_config_priority(tmp_path):
                                     pathlib.Path('some/path8')]
     assert opts.pcr_banks == ['SHA1', 'SHA256']
     assert opts.signing_engine == 'ENGINE'
-    assert opts.signtool == 'pesign'
-    assert opts.sb_key == 'SBKEY'
-    assert opts.sb_cert == 'SBCERT'
-    assert opts.sb_certdir == 'SBPATH'
-    assert opts.sb_cert_name == 'SBNAME'
+    assert opts.signtool == 'sbsign' # from args
+    assert opts.sb_key == 'SBKEY' # from args
+    assert opts.sb_cert == 'SBCERT' # from args
+    assert opts.sb_certdir == 'some/path5' # from config
+    assert opts.sb_cert_name == 'some/name1' # from config
     assert opts.sign_kernel is False
     assert opts.tools == [pathlib.Path('TOOLZ/')]
     assert opts.output == pathlib.Path('OUTPUT')

src/ukify/ukify.py

@@ -1038,6 +1038,19 @@ class ConfigItem:
         if getattr(namespace, dest) is None:
             setattr(namespace, dest, value)
 
+    @staticmethod
+    def config_set(
+            namespace: argparse.Namespace,
+            group: Optional[str],
+            dest: str,
+            value: Any,
+    ) -> None:
+        "Set namespace.<dest> to value only if it was None"
+
+        assert not group
+
+        setattr(namespace, dest, value)
+
     @staticmethod
     def config_set_group(
             namespace: argparse.Namespace,
@@ -1279,8 +1292,9 @@ CONFIG_ITEMS = [
         '--signtool',
         choices = ('sbsign', 'pesign'),
         dest = 'signtool',
-        default = 'sbsign',
-        help = 'whether to use sbsign or pesign. Default is sbsign.',
+        help = 'whether to use sbsign or pesign. It will also be inferred by the other \
+        parameters given: when using --secureboot-{private-key/certificate}, sbsign \
+        will be used, otherwise pesign will be used',
         config_key = 'UKI/SecureBootSigningTool',
     ),
     ConfigItem(
@@ -1301,6 +1315,7 @@ CONFIG_ITEMS = [
         default = '/etc/pki/pesign',
         help = 'required by --signtool=pesign. Path to nss certificate database directory for PE signing. Default is /etc/pki/pesign',
         config_key = 'UKI/SecureBootCertificateDir',
+        config_push = ConfigItem.config_set
     ),
     ConfigItem(
         '--secureboot-certificate-name',
@@ -1315,6 +1330,7 @@ CONFIG_ITEMS = [
         default = 365 * 10,
         help = "period of validity (in days) for a certificate created by 'genkey'",
         config_key = 'UKI/SecureBootCertificateValidity',
+        config_push = ConfigItem.config_set
     ),
 
     ConfigItem(
@@ -1573,12 +1589,19 @@ def finalize_options(opts):
         if opts.sb_cert:
             opts.sb_cert = pathlib.Path(opts.sb_cert)
 
-    if opts.signtool == 'sbsign':
-        if bool(opts.sb_key) ^ bool(opts.sb_cert):
-            raise ValueError('--secureboot-private-key= and --secureboot-certificate= must be specified together when using --signtool=sbsign')
-    else:
-        if not bool(opts.sb_cert_name):
-            raise ValueError('--secureboot-certificate-name must be specified when using --signtool=pesign')
+    if bool(opts.sb_key) ^ bool(opts.sb_cert):
+        # one param only given, sbsign need boths
+        raise ValueError('--secureboot-private-key= and --secureboot-certificate= must be specified together')
+    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 != 'sbsign':
+            raise ValueError(f'Cannot provide --signtool={opts.signtool} with --secureboot-private-key= and --secureboot-certificate=')
+        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':
+            raise ValueError(f'Cannot provide --signtool={opts.signtool} with --secureboot-certificate-name=')
+        opts.signtool = 'pesign'
 
     if opts.sign_kernel and not opts.sb_key and not opts.sb_cert_name:
         raise ValueError('--sign-kernel requires either --secureboot-private-key= and --secureboot-certificate= (for sbsign) or --secureboot-certificate-name= (for pesign) to be specified')