launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

Looks mostly good, but one bit of interface I'd prefer to see adjusted for safety.

Diff comments:

> === modified file 'lib/lp/archivepublisher/archivesigningkey.py'
> --- lib/lp/archivepublisher/archivesigningkey.py	2016-03-23 17:55:39 +0000
> +++ lib/lp/archivepublisher/archivesigningkey.py	2016-06-15 06:57:40 +0000
> @@ -144,3 +144,20 @@
>              os.path.join(suite_path, 'InRelease'), 'w')
>          inline_release_file.write(inline_release)
>          inline_release_file.close()
> +
> +    def signFile(self, path):
> +        """See `IArchiveSigningKey`."""
> +        assert self.archive.signing_key is not None, (
> +            "No signing key available for %s" % self.archive.displayname)
> +
> +        secret_key_export = open(
> +            self.getPathForSecretKey(self.archive.signing_key)).read()
> +        gpghandler = getUtility(IGPGHandler)
> +        secret_key = gpghandler.importSecretKey(secret_key_export)
> +
> +        file_content = open(path).read()

The interface definition says slightly ambiguously that this is a "path within dists", but this code seems to assume that it's an absolute path.  I'd be more comfortable if it were declared as a path relative to the archive root, and if signFile did os.path.join(self._archive_root_path, path).

While you're here, it would be good to convert to the context manager form instead of open().read(), perhaps also elsewhere in this file.  Production code shouldn't really be relying on the garbage collector to close file objects.

> +        signature = gpghandler.signContent(
> +            file_content, secret_key, mode=gpgme.SIG_MODE_DETACH)
> +
> +        with open(os.path.join(path + '.gpg'), 'w') as signature_file:
> +            signature_file.write(signature)
> 
> === modified file 'lib/lp/archivepublisher/publishing.py'
> --- lib/lp/archivepublisher/publishing.py	2016-06-07 17:05:57 +0000
> +++ lib/lp/archivepublisher/publishing.py	2016-06-15 06:57:40 +0000
> @@ -1478,16 +1478,17 @@
>  class DirectoryHash:
>      """Represents a directory hierarchy for hashing."""
>  
> -    def __init__(self, root, tmpdir, log):
> +    def __init__(self, root, tmpdir, signer=None, logger=None):
>          self.root = root
>          self.tmpdir = tmpdir
> -        self.log = log
> +        self.logger = logger

Maybe just get rid of the logger, since you aren't actually using it.  It can always be added later if need be.

> +        self.signer = signer
>          self.checksum_hash = []
>  
>          for usable in self._usable_archive_hashes:
> -            checksum_file = os.path.join(self.root, usable.dh_name)
> -            self.checksum_hash.append(
> -                (RepositoryIndexFile(checksum_file, self.tmpdir), usable))
> +            csum_path = os.path.join(self.root, usable.dh_name)
> +            self.checksum_hash.append((csum_path,
> +                RepositoryIndexFile(csum_path, self.tmpdir), usable))
>  
>      def __enter__(self):
>          return self
> 
> === modified file 'lib/lp/archivepublisher/tests/test_publisher.py'
> --- lib/lp/archivepublisher/tests/test_publisher.py	2016-06-06 17:13:42 +0000
> +++ lib/lp/archivepublisher/tests/test_publisher.py	2016-06-15 06:57:40 +0000
> @@ -105,6 +102,7 @@
>      LaunchpadZopelessLayer,
>      ZopelessDatabaseLayer,
>      )
> +from lp.archivepublisher.interfaces.publisherconfig import IPublisherConfigSet

Sort imports.

>  
>  
>  RELEASE = PackagePublishingPocket.RELEASE
> @@ -3194,6 +3192,23 @@
>                          file_list.append(line.strip().split(' '))
>          return result
>  
> +    def fetchSigs(self, rootdir):
> +        result = {}
> +        for dh_file in self.all_hash_files:
> +            checksum_sig = os.path.join(rootdir, dh_file) + '.gpg'
> +            if os.path.exists(checksum_sig):
> +                with open(checksum_sig, "r") as sfd:
> +                    for line in sfd:
> +                        sig_lines = result.setdefault(dh_file, [])
> +                        sig_lines.append(line)

I personally prefer:

    from collections import defaultdict
    ...
    result = defaultdict(list)
    result[dh_file].append(line)

... but up to you if you prefer that.

> +        return result
> +
> +
> +class TestDirectoryHash(TestDirectoryHashHelpers):
> +    """Unit tests for DirectoryHash object."""
> +
> +    layer = ZopelessDatabaseLayer
> +
>      def test_checksum_files_created(self):
>          tmpdir = unicode(self.makeTemporaryDirectory())
>          rootdir = unicode(self.makeTemporaryDirectory())
> 
> === modified file 'lib/lp/archivepublisher/tests/test_signing.py'
> --- lib/lp/archivepublisher/tests/test_signing.py	2016-06-14 13:29:07 +0000
> +++ lib/lp/archivepublisher/tests/test_signing.py	2016-06-15 06:57:40 +0000
> @@ -111,6 +116,15 @@
>              self.temp_dir, "signing", "signing-owner", "testing")
>          self.testcase_cn = '/CN=PPA signing-owner testing/'
>  
> +    def setUpArchiveKey(self):
> +        tac = KeyServerTac()
> +        tac.setUp()
> +
> +        key_path = os.path.join(gpgkeysdir, 'ppa-sample@xxxxxxxxxxxxxxxxx')
> +        IArchiveSigningKey(self.archive).setSigningKey(key_path)
> +
> +        tac.tearDown()

I think you can just do:

   with KeyServerTac():
       ...

... rather than explicit setUp/tearDown.

> +
>      def setUpUefiKeys(self, create=True):
>          self.key = os.path.join(self.signing_dir, "uefi.key")
>          self.cert = os.path.join(self.signing_dir, "uefi.crt")


-- 
https://code.launchpad.net/~apw/launchpad/signing-gpg-sign-checksum-files/+merge/297177
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.