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	2016-05-31 12:40:38 +0000
> +++ lib/lp/archivepublisher/signing.py	2016-05-31 16:45:48 +0000
> @@ -286,6 +287,28 @@
>          except OSError as exc:
>              raise SigningUploadPackError(tarfilename, exc)
>  
> +    def checksumSha256(self, datafd):
> +        """Calculate the SHA256 checksum for the passed file descriptor."""
> +        file_hash = hashlib.sha256()
> +        for chunk in iter(lambda: datafd.read(256 * 1024), ""):
> +            file_hash.update(chunk)
> +        return file_hash.hexdigest()
> +
> +    def generateChecksums(self):
> +        """Generate SHA256 checksums for the custom upload."""
> +        versiondir = os.path.join(self.tmpdir, self.version)
> +        checksum_file = os.path.join(self.tmpdir, "SHA256SUMS.tmp")

It might be good to use something like this instead, obviously with better indentation:

  RepositoryIndexFile(os.path.join(versiondir, "SHA256SUMS"), self.tmpdir, [IndexCompressionType.UNCOMPRESSED])

For bonus points, beef up RepositoryIndexFile so that it can be used as a context manager, and maybe also to default compressors to [IndexCompressionType.UNCOMPRESSED] if it isn't passed explicitly; that way we'd have a nice "write this file atomically" helper.

> +        prefix_len = len(versiondir) + 1
> +        with open(checksum_file, "w") as sfd:
> +            for dirpath, dirnames, filenames in os.walk(versiondir):
> +                for filename in filenames:
> +                    disk_name = os.path.join(dirpath, filename)
> +                    with open(disk_name) as dfd:

open(disk_name, "rb") rather than assuming text.  Doesn't matter in Python 2 on Unix but we should get this right in new code.

> +                        checksum = self.checksumSha256(dfd)
> +                    print("%s  %s" % (checksum, disk_name[prefix_len:]),
> +                        file=sfd)

It would be slightly better to replace the second space with '*', since the files here are typically binary.

> +        os.rename(checksum_file, os.path.join(versiondir, "SHA256SUMS"))

I feel like all this is too hardcoded to SHA-256.  I'd suggest refactoring this around lp.archivepublisher.publishing.archive_hashes, adding a new field to IArchiveHash which says whether we want to use the hash to generate checksums for signed uploads (write_signed or something), and then writing a checksums file for each element of archive_hashes where write_signed is True, using hash_factory rather than hardcoding hashlib.sha256.  You'd probably also need another field name for the "SHA256SUMS" name.

> +
>      def extract(self):
>          """Copy the custom upload to a temporary directory, and sign it.
>  
> 
> === modified file 'lib/lp/archivepublisher/tests/test_signing.py'
> --- lib/lp/archivepublisher/tests/test_signing.py	2016-05-31 12:23:15 +0000
> +++ lib/lp/archivepublisher/tests/test_signing.py	2016-05-31 16:45:48 +0000
> @@ -8,6 +8,7 @@
>  import os
>  import stat
>  import tarfile
> +from cStringIO import StringIO

Sort imports, but maybe also "from io import BytesIO" instead for future friendliness to Python 3.

>  
>  from fixtures import MonkeyPatch
>  
> @@ -602,6 +620,48 @@
>          self.assertEqual(stat.S_IMODE(os.stat(self.kmod_pem).st_mode), 0o600)
>          self.assertEqual(stat.S_IMODE(os.stat(self.kmod_x509).st_mode), 0o644)
>  
> +    def test_sha256_basic_summing(self):
> +        upload = SigningUpload()
> +
> +        data = StringIO("Hello World!\n")
> +        self.assertEqual(
> +            '03ba204e50d126e4674c005e04d82e84c21366780af1f43bd54a37816b6ab340',

I normally prefer to write hashlib.sha256(...).hexdigest() in tests rather than hardcoding the expected output, just because it's more explicit to the next person editing the code about what's intended.  Likewise further down.

> +            upload.checksumSha256(data))
> +
> +        data = StringIO("Something somewhat longer?\n" * 100)
> +        self.assertEqual(
> +            '8bdc49d049287ac937e524afe6c7ba014169b7aff11c78f05467487cc50f6f67',
> +            upload.checksumSha256(data))
> +
> +    def test_sha256_multiblock_summing(self):
> +        upload = SigningUpload()
> +
> +        line = "A" * 1024 + "\n"
> +        data = StringIO(line * 300)
> +        self.assertEqual(
> +            'e4d4650544a18a1dd837ee70c68137b553e3d9455e941ec71465cd5557ef0e20',
> +            upload.checksumSha256(data))
> +
> +    def test_checksumming_tree(self):
> +        # Specifying no options should leave us with an open tree,
> +        # confirm it is checksummed.
> +        self.setUpUefiKeys()
> +        self.setUpKmodKeys()
> +        self.openArchive("test", "1.0", "amd64")
> +        self.archive.add_file("1.0/empty.efi", "")
> +        self.archive.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))
> +        H = 'e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855'
> +        expected = [[H, 'empty.efi'], [H, 'empty.efi.signed'],
> +            [H, 'empty.ko'], [H, 'empty.ko.sig'],
> +            ]
> +        with open(sha256file) as sfd:
> +            content = [line.split() for line in sfd]
> +        self.assertContentEqual(expected, content)
> +
>  
>  class TestUefi(TestSigningHelpers):
>  


-- 
https://code.launchpad.net/~apw/launchpad/signing-add-sha256-checksums/+merge/295615
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.