launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

This generally seems OK; just some nits in the tests.

Diff comments:

> 
> === modified file 'lib/lp/archivepublisher/tests/test_signing.py'
> --- lib/lp/archivepublisher/tests/test_signing.py	2017-07-24 10:06:38 +0000
> +++ lib/lp/archivepublisher/tests/test_signing.py	2017-08-01 06:17:20 +0000
> @@ -37,6 +43,29 @@
>  from lp.testing.layers import ZopelessDatabaseLayer
>  
>  
> +class SignedMatches(Matcher):
> +    """Matches if a signing result directory is valid."""
> +
> +    def __init__(self, expected):
> +        self.expected = expected
> +
> +    def match(self, signed):
> +        content = []
> +        for root, dirs, files in os.walk(signed):
> +            content += [os.path.join(root, f)[len(signed) + 1:] for f in files]

This took me a few reads to understand.  Maybe this would be more explicit about its intent and thus clearer?

  content.extend([os.path.relpath(os.path.join(root, f), signed) for f in files])

> +
> +        left_over = list(set(content) - set(self.expected))
> +        missing = list(set(self.expected) - set(content))

Maybe sorted() rather than list() in these two so that the result is predictable.

> +        if left_over != [] or missing != []:
> +            missmatch = ''

s/missmatch/mismatch/ (also below)

> +            if left_over:
> +                missmatch += " unexpected files: " + str(left_over)
> +            if missing:
> +                missmatch += " missing files: " + str(missing)
> +            return Mismatch("SignedMatches:" + missmatch)
> +        return None
> +
> +
>  class FakeMethodCallLog(FakeMethod):
>      """Fake execution general commands."""
>      def __init__(self, upload=None, *args, **kwargs):
> @@ -85,6 +128,9 @@
>      def caller_count(self, caller):
>          return self.callers.get(caller, 0)
>  
> +    def caller_list(self):
> +        return [(c, n) for (c, n) in self.callers.items() if n != 0]

I'd probably say "caller" rather than "c" here to make it slightly less telegraphic, but nice simplification of tests.

> +
>  
>  class TestSigningHelpers(TestCaseWithFactory):
>  
> @@ -547,6 +612,72 @@
>              ]
>          self.assertEqual(expected_cmd, args)
>  
> +    def test_correct_opal_signing_command_executed(self):
> +        # Check that calling signKmod() will generate the expected command

signOpal

> +        # when appropriate keys are present.
> +        self.setUpOpalKeys()
> +        fake_call = FakeMethod(result=0)
> +        self.useFixture(MonkeyPatch("subprocess.call", fake_call))
> +        upload = SigningUpload()
> +        upload.generateOpalKeys = FakeMethod()
> +        upload.setTargetDirectory(
> +            self.archive, "test_1.0_amd64.tar.gz", "distroseries")
> +        upload.signOpal('t.opal')
> +        self.assertEqual(1, fake_call.call_count)
> +        # Assert command form.
> +        args = fake_call.calls[0][0][0]
> +        expected_cmd = [
> +            'kmodsign', '-D', 'sha512', self.opal_pem, self.opal_x509,
> +            't.opal', 't.opal.sig'
> +            ]
> +        self.assertEqual(expected_cmd, args)
> +        self.assertEqual(0, upload.generateOpalKeys.call_count)
> +
> +    def test_correct_opal_signing_command_executed_no_keys(self):
> +        # Check that calling signKmod() will generate no commands when

signOpal

> +        # no keys are present.
> +        self.setUpOpalKeys(create=False)
> +        fake_call = FakeMethod(result=0)
> +        self.useFixture(MonkeyPatch("subprocess.call", fake_call))
> +        upload = SigningUpload()
> +        upload.generateOpalKeys = FakeMethod()
> +        upload.setTargetDirectory(
> +            self.archive, "test_1.0_amd64.tar.gz", "distroseries")
> +        upload.signOpal('t.opal')
> +        self.assertEqual(0, fake_call.call_count)
> +        self.assertEqual(0, upload.generateOpalKeys.call_count)
> +
> +    def test_correct_opal_keygen_command_executed(self):
> +        # Check that calling generateUefiKeys() will generate the

generateOpalKeys

> +        # expected command.
> +        self.setUpPPA()
> +        self.setUpOpalKeys(create=False)
> +        fake_call = FakeMethod(result=0)
> +        self.useFixture(MonkeyPatch("subprocess.call", fake_call))
> +        upload = SigningUpload()
> +        upload.setTargetDirectory(
> +            self.archive, "test_1.0_amd64.tar.gz", "distroseries")
> +        upload.generateOpalKeys()
> +        self.assertEqual(2, fake_call.call_count)
> +        # Assert the actual command matches.
> +        args = fake_call.calls[0][0][0]
> +        # Sanitise the keygen tmp file.
> +        if args[11].endswith('.keygen'):
> +            args[11] = 'XXX.keygen'
> +        expected_cmd = [
> +            'openssl', 'req', '-new', '-nodes', '-utf8', '-sha512',
> +            '-days', '3650', '-batch', '-x509',
> +            '-config', 'XXX.keygen', '-outform', 'PEM',
> +            '-out', self.opal_pem, '-keyout', self.opal_pem
> +            ]
> +        self.assertEqual(expected_cmd, args)
> +        args = fake_call.calls[1][0][0]
> +        expected_cmd = [
> +            'openssl', 'x509', '-in', self.opal_pem, '-outform', 'DER',
> +            '-out', self.opal_x509
> +            ]
> +        self.assertEqual(expected_cmd, args)
> +
>      def test_signs_uefi_image(self):
>          # Each image in the tarball is signed.
>          self.setUpUefiKeys()
> @@ -702,14 +884,44 @@
>          yield self.setUpArchiveKey()
>          self.setUpUefiKeys()
>          self.setUpKmodKeys()
> -        self.openArchive("test", "1.0", "amd64")
> -        self.tarfile.add_file("1.0/empty.efi", "")
> -        self.tarfile.add_file("1.0/empty.ko", "")
> -        self.process_emulate()
> -        sha256file = os.path.join(self.getSignedPath("test", "amd64"),
> -             "1.0", "SHA256SUMS")
> -        self.assertTrue(os.path.exists(sha256file))
> -        self.assertTrue(os.path.exists(sha256file + '.gpg'))
> +        self.setUpOpalKeys()
> +        self.openArchive("test", "1.0", "amd64")
> +        self.tarfile.add_file("1.0/empty.efi", "")
> +        self.tarfile.add_file("1.0/empty.ko", "")
> +        self.tarfile.add_file("1.0/empty.opal", "")
> +        self.process_emulate()
> +        sha256file = os.path.join(self.getSignedPath("test", "amd64"),
> +             "1.0", "SHA256SUMS")
> +        self.assertTrue(os.path.exists(sha256file))
> +        self.assertTrue(os.path.exists(sha256file + '.gpg'))
> +
> +    @defer.inlineCallbacks
> +    def test_checksumming_tree_signed_options_tarball(self):
> +        # Specifying no options should leave us with an open tree,
> +        # confirm it is checksummed.  Supply an archive signing key
> +        # which should trigger signing of the checksum file.
> +        yield self.setUpArchiveKey()
> +        self.setUpUefiKeys()
> +        self.setUpKmodKeys()
> +        self.setUpOpalKeys()
> +        self.openArchive("test", "1.0", "amd64")
> +        self.tarfile.add_file("1.0/control/options", "tarball")
> +        self.tarfile.add_file("1.0/empty.efi", "")
> +        self.tarfile.add_file("1.0/empty.ko", "")
> +        self.tarfile.add_file("1.0/empty.opal", "")
> +        self.process_emulate()
> +        sha256file = os.path.join(self.getSignedPath("test", "amd64"),
> +             "1.0", "SHA256SUMS")
> +        self.assertTrue(os.path.exists(sha256file))
> +        self.assertTrue(os.path.exists(sha256file + '.gpg'))
> +
> +        tarfilename = os.path.join(self.getSignedPath("test", "amd64"),
> +            "1.0", "signed.tar.gz")
> +        with tarfile.open(tarfilename) as tarball:
> +            self.assertThat(tarball.getnames(), Not(Contains([
> +                "1.0/SHA256SUMS", "1.0/SHA256SUMS.gpg",
> +                '1.0/empty.ko',
> +                ])))

That tests that the supplied list isn't itself an element of tarball.getnames(), which isn't what you want.  If you prefer not to assert on each one individually, then maybe something like:

  self.assertThat(tarball.getnames(), Not(MatchesAny(
      Contains(name) for name in [
          "1.0/SHA256SUMS", "1.0/SHA256SUMS.gpg",
          "1.0/empty.ko",
          ])))

>  
>  
>  class TestUefi(TestSigningHelpers):


-- 
https://code.launchpad.net/~apw/launchpad/signing-opal/+merge/328108
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.