launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Definitely getting there, but still some things to fix, mostly in the tests.

Diff comments:

> 
> === modified file 'lib/lp/archivepublisher/tests/test_uefi.py'
> --- lib/lp/archivepublisher/tests/test_uefi.py	2016-02-05 16:51:12 +0000
> +++ lib/lp/archivepublisher/tests/test_uefi.py	2016-04-27 16:25:56 +0000
> @@ -16,6 +16,20 @@
>  from lp.services.tarfile_helpers import LaunchpadWriteTarFile
>  from lp.testing import TestCase
>  from lp.testing.fakemethod import FakeMethod
> +from fixtures import MonkeyPatch

PEP-8 recommends dividing imports into three blocks: standard library, third-party, and local.  This is third-party and so should be in the block before all the lp.* imports.  utilities/format-new-and-modified-imports can help with putting everything in the right place.

> +
> +
> +class FakeMethodGenUefiKeys(FakeMethod):
> +    """A fake command executer."""

Normally "executor", I believe.

> +    def __init__(self, *args, **kwargs):
> +        self.testcase = kwargs.pop('testcase', None)
> +        super(FakeMethodGenUefiKeys, self).__init__(*args, **kwargs)

I'd spell this:

    def __init__(self, testcase=None, *args, **kwargs):
        super(FakeMethodGenUefiKeys, self).__init__(*args, **kwargs)
        self.testcase = testcase

Also, I feel that this would all be clearer if you stopped setting key and cert on the test case object, and instead used the ones that are set on instances of UefiUpload by its setTargetDirectory method, so you'd pass a UefiUpload here rather than a TestCase.  Using more of the code under test where possible is usually a good idea.

> +
> +    def __call__(self, *args, **kwargs):
> +        super(FakeMethodGenUefiKeys, self).__call__(*args, **kwargs)
> +
> +        write_file(self.testcase.key, "")
> +        write_file(self.testcase.cert, "")
>  
>  
>  class FakeConfig:
> @@ -37,11 +52,40 @@
>          old_umask = os.umask(0o022)
>          self.addCleanup(os.umask, old_umask)
>  
> -    def setUpKeyAndCert(self):
> +    def setUpAutoKey(self):
> +        self.pubconf.uefiautokey = True
> +
> +    def setUpKeyAndCert(self, create=True):
>          self.key = os.path.join(self.uefi_dir, "uefi.key")
>          self.cert = os.path.join(self.uefi_dir, "uefi.crt")
> -        write_file(self.key, "")
> -        write_file(self.cert, "")
> +        if create:
> +            write_file(self.key, "")
> +            write_file(self.cert, "")
> +
> +    def validateCmdUefiKeygen(self, call):

s/validate/assert/

This is also only used once, so I'd just inline it into its caller.

> +        args = call[0][0]
> +
> +        archive_root = self.pubconf.archiveroot
> +        archive_name = os.path.basename(archive_root)
> +        owner_name = os.path.basename(os.path.dirname(archive_root))
> +        common_name = '/CN=PPA ' + owner_name + ' ' + archive_name + '/'

This is almost a copy of the code under test, which makes it too easy to commit the same mistake in both.  I think you should instead construct a separate FakeConfig for this test that has a PPA-like archiveroot (i.e. constructed in the same way as getPubConfig does), and then you can assert that you get specifically the owner and archive names that you started with rather than having to derive them from the archiveroot.

> +
> +        expected_cmd = [
> +            'openssl', 'req', '-new', '-x509', '-newkey', 'rsa:2048',
> +            '-subj', common_name, '-keyout', self.key, '-out', self.cert,
> +            '-days', '3650', '-nodes', '-sha256'

We normally put a comma after the last item in a multi-line list like this, because it makes it easier to make future additions with smaller diffs and less chance of error.

> +            ]
> +        self.assertEqual(expected_cmd, args)
> +
> +    def validateCmdSbsign(self, call, filename):

s/validate/assert/

> +        args = call[0][0]
> +
> +        if len(args) >= 6:
> +            args[5] = os.path.basename(args[5])

This can be removed once you apply my comment below about signUefiCall.  At that point, the only time the test suite will call assertCmdSbsign is when it's testing the behaviour of the signUefi method itself, and so there are no os.path.join-type transformations to handle.

I'd probably also just inline this assertion into its caller at this point.

> +
> +        expected_cmd = [
> +            'sbsign', '--key', self.key, '--cert', self.cert, filename]
> +        self.assertEqual(expected_cmd, args)
>  
>      def openArchive(self, loader_type, version, arch):
>          self.path = os.path.join(
> @@ -52,9 +96,15 @@
>      def process(self):
>          self.archive.close()
>          self.buffer.close()
> +        fake_call = FakeMethod()
>          upload = UefiUpload()
> -        upload.sign = FakeMethod()
> -        upload.process(self.pubconf, self.path, self.suite)
> +        upload.signUefiCall = FakeMethod()

As per other comments here, this should be mocking signUefi rather than signUefiCall.

> +        upload.genUefiKeys = FakeMethodGenUefiKeys(testcase=self)
> +        with self.useFixture(MonkeyPatch("subprocess.call", fake_call)):
> +            upload.process(self.pubconf, self.path, self.suite)
> +        # Under no circumstances is it safe to exectute actual commands.

s/exectute/execute/

> +        self.assertEqual(0, fake_call.call_count)
> +
>          return upload
>  
>      def getUefiPath(self, loader_type, arch):
> @@ -104,15 +158,29 @@
>          os.umask(0o002)  # cleanup already handled by setUp
>          self.assertRaises(CustomUploadBadUmask, self.process)
>  
> -    def test_correct_signing_command(self):
> -        # getSigningCommand returns the correct command.
> +    def test_correct_uefi_signing_command_executed(self):
> +        # Check that calling sign() will generate the expected command.

You've renamed the method to signUefi, so update the comment too.

>          self.setUpKeyAndCert()
> -        upload = UefiUpload()
> -        upload.setTargetDirectory(
> -            self.pubconf, "test_1.0_amd64.tar.gz", "distroseries")
> -        expected_command = [
> -            "sbsign", "--key", self.key, "--cert", self.cert, "t.efi"]
> -        self.assertEqual(expected_command, upload.getSigningCommand("t.efi"))
> +        fake_call = FakeMethod()
> +        with self.useFixture(MonkeyPatch("subprocess.call", fake_call)):
> +            upload = UefiUpload()
> +            upload.setTargetDirectory(
> +                self.pubconf, "test_1.0_amd64.tar.gz", "distroseries")
> +            upload.signUefi('t.efi')
> +        self.assertEqual(1, fake_call.call_count)
> +        self.validateCmdSbsign(fake_call.calls[0], 't.efi')
> +
> +    def test_correct_uefi_keygen_command_executed(self):
> +        # Check that calling genUefiKeys() will generate the expected command.
> +        self.setUpKeyAndCert(create=False)
> +        fake_call = FakeMethod()
> +        with self.useFixture(MonkeyPatch("subprocess.call", fake_call)):
> +            upload = UefiUpload()
> +            upload.setTargetDirectory(
> +                self.pubconf, "test_1.0_amd64.tar.gz", "distroseries")
> +            upload.genUefiKeys()
> +        self.assertEqual(1, fake_call.call_count)
> +        self.validateCmdUefiKeygen(fake_call.calls[0])
>  
>      def test_signs_image(self):
>          # Each image in the tarball is signed.
> @@ -120,10 +188,8 @@
>          self.openArchive("test", "1.0", "amd64")
>          self.archive.add_file("1.0/empty.efi", "")
>          upload = self.process()
> -        self.assertEqual(1, upload.sign.call_count)
> -        self.assertEqual(1, len(upload.sign.calls[0][0]))
> -        self.assertEqual(
> -            "empty.efi", os.path.basename(upload.sign.calls[0][0][0]))
> +        self.assertEqual(1, upload.signUefiCall.call_count)
> +        self.validateCmdSbsign(upload.signUefiCall.calls[0], "empty.efi")

As per other comments here, this should instead be testing how upload.process calls upload.signUefi, at which point the assertions should look as before but with s/sign/signUefi/g.

>  
>      def test_installed(self):
>          # Files in the tarball are installed correctly.
> 
> === modified file 'lib/lp/archivepublisher/uefi.py'
> --- lib/lp/archivepublisher/uefi.py	2013-07-25 16:45:20 +0000
> +++ lib/lp/archivepublisher/uefi.py	2016-04-27 16:25:56 +0000
> @@ -102,17 +95,76 @@
>                  if filename.endswith(".efi"):
>                      yield os.path.join(dirpath, filename)
>  
> -    def getSigningCommand(self, image):
> -        """Return the command used to sign an image."""
> -        return ["sbsign", "--key", self.key, "--cert", self.cert, image]
> -
> -    def sign(self, image):
> -        """Sign an image."""
> -        if subprocess.call(self.getSigningCommand(image)) != 0:
> +    def genUefiKeys(self):

"genUefiKeys" and "getUefiKeys" are visually very similar.  I'd rename this one to "generateUefiKeys" so that it's easier to tell them apart.

> +        """Generate new UEFI Keys for this archive."""
> +        directory = os.path.dirname(self.key)
> +        if not os.path.exists(directory):
> +            os.makedirs(directory)
> +
> +        # XXX: pull out the PPA owner and name to seed key CN
> +        archive_name = os.path.basename(self.archiveroot)
> +        owner_name = os.path.basename(os.path.dirname(self.archiveroot))
> +        common_name = '/CN=PPA ' + owner_name + ' ' + archive_name + '/'

This is all unfortunately magical, as the XXX comment indicates, but it's difficult to do better with the current custom upload publication API.  I suspect the right answer here (not for this branch!) is to refactor how custom upload processors are called so that they receive an Archive directly and get the publisher configuration from them by themselves.  Indeed there are a couple of XXX comments in lp.soyuz.model.queue that suggest that sort of change as part of avoiding imports from lp.archivepublisher.

> +
> +        old_mask = os.umask(0o077)
> +        try:
> +            new_key_cmd = [
> +                'openssl', 'req', '-new', '-x509', '-newkey', 'rsa:2048',
> +                '-subj', common_name, '-keyout', self.key, '-out', self.cert,
> +                '-days', '3650', '-nodes', '-sha256'

Same again re commas.

> +                ]
> +            if subprocess.call(new_key_cmd) != 0:
> +                # Just log this rather than failing, since custom upload errors
> +                # tend to make the publisher rather upset.
> +                if self.logger is not None:
> +                    self.logger.warning(
> +                        "Failed to generate UEFI signing keys for %s" %
> +                        common_name)
> +        finally:
> +            os.umask(old_mask)
> +
> +        if os.path.exists(self.cert):
> +            os.chmod(self.cert, 0o644)
> +
> +    def getUefiKeys(self):
> +        """Validate and return the uefi key and cert for encryption."""
> +
> +        if self.key and self.cert:
> +            # If neither of the key files exists then attempt to
> +            # generate them.
> +            if (self.autokey and not os.path.exists(self.key)
> +                and not os.path.exists(self.cert)):
> +                self.genUefiKeys()
> +
> +            # If we have keys, but cannot read them they are dead to us.
> +            if not os.access(self.key, os.R_OK):
> +                if self.logger is not None:
> +                    self.logger.warning(
> +                        "UEFI private key %s not readable" % self.key)
> +                self.key = None
> +            if not os.access(self.cert, os.R_OK):
> +                if self.logger is not None:
> +                    self.logger.warning(
> +                        "UEFI certificate %s not readable" % self.cert)
> +                self.cert = None
> +
> +        return (self.key, self.cert)
> +
> +    def signUefiCall(self, cmdl):
> +        if subprocess.call(cmdl) != 0:
>              # Just log this rather than failing, since custom upload errors
>              # tend to make the publisher rather upset.
>              if self.logger is not None:
> -                self.logger.warning("Failed to sign %s" % image)
> +                self.logger.warning("UEFI Signing Failed '%s'" %
> +                    " ".join(cmdl))
> +
> +    def signUefi(self, image):
> +        """Attempt to sign an image."""
> +        (key, cert) = self.getUefiKeys()
> +        if not key or not cert:
> +            return
> +        cmdl = ["sbsign", "--key", key, "--cert", cert, image]
> +        self.signUefiCall(cmdl)

For the same reasons as explained in my last review, there's no need for this level of indirection.  Just call subprocess.call directly.  There are basically two cases needed in the test suite:

 1) Call signUefi with a mock subprocess.call; the purpose of this is to test that signing a single image works by asserting that the signUefi method calls subprocess.call with the expected arguments.
 2) Call some higher-level method with a mock signUefi; the purpose of this is to test that the higher-level method signs the correct set of images by asserting that it calls signUefi with the expected arguments.

In neither case do we need signUefiCall.

>  
>      def extract(self):
>          """Copy the custom upload to a temporary directory, and sign it.


-- 
https://code.launchpad.net/~apw/launchpad/uefi-auto-key/+merge/293087
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.