launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'lib/lp/archivepublisher/signing.py'
> --- lib/lp/archivepublisher/signing.py	2019-06-06 10:55:17 +0000
> +++ lib/lp/archivepublisher/signing.py	2019-08-22 17:03:12 +0000
> @@ -82,6 +82,23 @@
>          self.package, self.version, self.arch = self.parsePath(
>              tarfile_path)
>  
> +    def getSeriesPath(self, signingroot, key_name, archive, autokey):

I think I'd be inclined towards having this take a pubconf parameter rather than signingroot and autokey; it would be more concise at call sites, and I don't think we ever need to pass them separately.  (For tests that require autokey=True, use self.setUpPPA() first to arrange for self.archive to be a PPA whose publisher configuration will have signingautokey=True.)

> +        # If there are no series, use the one at the root of the filesystem
> +        # autokeys are generally created manually, so use the root
> +        if not archive.distribution.series or autokey:
> +            return os.path.join(signingroot, key_name)

autokey controls whether a key should be created automatically if it doesn't exist; it shouldn't stop per-series keys being used if they already exist.  The archive.distribution.series check here also seems redundant, since if there are no series then the for loop below will just perform zero iterations.

Can you perhaps just delete this if block?

> +        # Walk the series backwards, looking for a key
> +        for series in archive.distribution.series:
> +            path = os.path.join(
> +                signingroot,
> +                series.name,
> +                key_name
> +                )
> +            if os.path.exists(path):
> +                return path
> +        # If we have exhausted all available series, return the root
> +        return os.path.join(signingroot, key_name)
> +
>      def setTargetDirectory(self, archive, tarfile_path, suite):
>          self.archive = archive
>          pubconf = getPubConfig(archive)
> @@ -101,8 +118,20 @@
>              self.fit_cert = None
>              self.autokey = False
>          else:
> -            self.uefi_key = os.path.join(pubconf.signingroot, "uefi.key")
> -            self.uefi_cert = os.path.join(pubconf.signingroot, "uefi.crt")
> +            # Per series uefi signing keys. This can be walked backwards
> +            # in the series list if it doesn't exist in the current series

Something like this should probably be a docstring for getSeriesPath instead.  Do tighten up the text a bit, since I'm not sure it's totally grammatical right now.

Is there any reason why this is specific to UEFI here?  It's true that UEFI is the only use case we have right now, but I can't think of a good reason not to make the change uniformly for all key types, since having UEFI be an exception is probably going to confuse somebody at some point.

> +            self.uefi_key = self.getSeriesPath(
> +                pubconf.signingroot,
> +                "uefi.key",
> +                archive,
> +                pubconf.signingautokey
> +                )
> +            self.uefi_cert = self.getSeriesPath(
> +                pubconf.signingroot,
> +                "uefi.crt",
> +                archive,
> +                pubconf.signingautokey
> +                )
>              self.kmod_pem = os.path.join(pubconf.signingroot, "kmod.pem")
>              self.kmod_x509 = os.path.join(pubconf.signingroot, "kmod.x509")
>              self.opal_pem = os.path.join(pubconf.signingroot, "opal.pem")
> 
> === modified file 'lib/lp/archivepublisher/tests/test_signing.py'
> --- lib/lp/archivepublisher/tests/test_signing.py	2019-06-06 10:55:17 +0000
> +++ lib/lp/archivepublisher/tests/test_signing.py	2019-08-22 17:03:12 +0000
> @@ -970,6 +974,31 @@
>          upload = self.process()
>          self.assertEqual(1, upload.signUefi.call_count)
>  
> +    def test_signs_uefi_image_per_series(self):
> +        """Check that signing can be per series.
> +        This should fall through to the first series,
> +        as the second does not have keys.
> +        """
> +        first_series = self.factory.makeDistroSeries(
> +            self.distro,
> +            name="existing-keys"
> +            )
> +        self.factory.makeDistroSeries(
> +            self.distro,
> +            name="no-keys"
> +            )
> +        # Each image in the tarball is signed.
> +        self.setUpUefiKeys()
> +        self.setUpUefiKeys(series=first_series)
> +        self.openArchive("test", "1.0", "amd64")
> +        self.tarfile.add_file("1.0/empty.efi", b"")
> +        upload = self.process_emulate()
> +        expected_callers = [('UEFI signing', 1),]
> +        self.assertContentEqual(expected_callers, upload.callLog.caller_list())
> +        # Check the correct series name appears in the call arguments
> +        self.assertTrue(
> +            "existing-keys" in upload.callLog.extract_args()[0][1][2])

This would be better written using self.assertIn.

> +
>      def test_signs_fit_image(self):
>          # Each image in the tarball is signed.
>          self.setUpFitKeys()
> @@ -1351,6 +1380,71 @@
>              "SUITE": Equals(self.suite),
>              }))
>  
> +    def test_getSeriesKeyName_no_series(self):
> +        upload = SigningUpload()
> +        config = getPubConfig(self.archive)
> +        result = upload.getSeriesPath(
> +            config.signingroot, 'key.key', self.archive, False)
> +        expected_path = os.path.join(config.signingroot, 'key.key')
> +        self.assertEqual(expected_path, result)
> +
> +    def test_getSeriesKeyName_autokey(self):
> +        self.factory.makeDistroSeries(self.distro, name='newdistro')
> +        upload = SigningUpload()
> +        config = getPubConfig(self.archive)
> +        result = upload.getSeriesPath(
> +            config.signingroot, "uefi.key", self.archive, True)
> +        expected_path = os.path.join(config.signingroot, "uefi.key")
> +        self.assertEqual(expected_path, result)
> +
> +    def test_getSeriesKeyName_one_distro(self):
> +        self.setUpUefiKeys(
> +            series=self.factory.makeDistroSeries(
> +                self.distro, name="newdistro"))
> +        upload = SigningUpload()
> +        config = getPubConfig(self.archive)
> +        result = upload.getSeriesPath(
> +            config.signingroot, "uefi.key", self.archive, False)
> +        expected_path = os.path.join(
> +            config.signingroot,
> +            "newdistro",
> +            "uefi.key",
> +            )
> +        self.assertEqual(expected_path, result)
> +
> +    def test_getSeriesKeyName_two_distro(self):
> +        self.setUpUefiKeys(
> +            series=self.factory.makeDistroSeries(
> +                self.distro, name="newdistro"))
> +        self.setUpUefiKeys(
> +            series=self.factory.makeDistroSeries(
> +                self.distro, name="seconddistro"))
> +        upload = SigningUpload()
> +        config = getPubConfig(self.archive)
> +        result = upload.getSeriesPath(
> +            config.signingroot, "uefi.key", self.archive, False)
> +        expected_path = os.path.join(
> +            config.signingroot,
> +            "seconddistro",
> +            "uefi.key",
> +            )
> +        self.assertEqual(expected_path, result)

The names of these two tests should say "distroseries" rather than "distro"; similarly, throughout your tests, I'd recommend not making the names of test distroseries be "foodistro", since "distro" is short for "distribution" which is the next layer up.

> +
> +    def test_getSeriesKeyName_two_distro_fallthrough(self):
> +        self.setUpUefiKeys(
> +            series=self.factory.makeDistroSeries(
> +                self.distro, name="newdistro"))
> +        upload = SigningUpload()
> +        config = getPubConfig(self.archive)
> +        result = upload.getSeriesPath(
> +            config.signingroot, "uefi.key", self.archive, False)
> +        expected_path = os.path.join(
> +            config.signingroot,
> +            "newdistro",
> +            "uefi.key",
> +            )
> +        self.assertEqual(expected_path, result)

This seems to be an exact copy of test_getSeriesKeyName_one_distro.

> +
>  
>  class TestUefi(TestSigningHelpers):
>  


-- 
https://code.launchpad.net/~twom/launchpad/per-series-signing-keys/+merge/371675
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.