Merge lp:~stevenk/launchpad/fixes-bug-451396 into lp:launchpad

[Steve Kowalik]

Strips PGP signatures unilaterally, instead of just from PPA uploads, which means users can't download code from the ACCEPTED/REJECTED Ubuntu queue and throw it to the sponsors/uploaders PPA.

This branch strips PGP signatures unilaterally, instead of just from PPA uploads, which means users can't download code from the ACCEPTED/REJECTED Ubuntu queue and throw it to the sponsors/uploaders PPA.

[Abel Deuring (adeuring)]

Hi Steve,

out of curiosity, I reverted the change in lib/lp/soyuz/model/queue.py and then ran "./bin/test -vvt test_uploadprocessor". The result: All tests passed. So, either your test is wrong, or the signature is already stripped elsewhere...

And a few nitpicks:

I think you don't need the super(ThisClass, self).method(...) pattern here. A simple self.method(...) should work fine.

You should include TestUploadProcessorBase in the __all__ list of lib/lp/archiveuploader/tests/test_uploadprocessor.py , because you import this class lib/lp/archiveuploader/tests/test_ppauploadprocessor.py.

Please remove the trailing blank in this line:

+ # Leaving the PGP signature on a package uploaded

review: Needs Fixing

[Abel Deuring (adeuring)]

Steve,

thanks for you patience with all my complaints. the branch fine now.

review: Approve (code)

=== modified file 'lib/lp/archiveuploader/tests/test_ppauploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_ppauploadprocessor.py	2010-04-15 17:18:25 +0000
+++ lib/lp/archiveuploader/tests/test_ppauploadprocessor.py	2010-04-22 16:42:37 +0000
@@ -682,31 +682,14 @@
             'https://help.launchpad.net/Packaging/PPA for more information.')
 
     def testPGPSignatureNotPreserved(self):
-        """PGP signatures should be removed from PPA changesfiles.
+        """PGP signatures should be removed from PPA .changes files.
 
-        Email notifications and the librarian file for the changesfile should
+        Email notifications and the librarian file for .changes file should
         both have the PGP signature removed.
         """
         upload_dir = self.queueUpload("bar_1.0-1", "~name16/ubuntu")
         self.processUpload(self.uploadprocessor, upload_dir)
-
-        # Check the email.
-        from_addr, to_addrs, raw_msg = stub.test_emails.pop()
-        msg = message_from_string(raw_msg)
-
-        # This is now a MIMEMultipart message.
-        body = msg.get_payload(0)
-        body = body.get_payload(decode=True)
-
-        self.assertTrue(
-            "-----BEGIN PGP SIGNED MESSAGE-----" not in body,
-            "Unexpected PGP header found")
-        self.assertTrue(
-            "-----BEGIN PGP SIGNATURE-----" not in body,
-            "Unexpected start of PGP signature found")
-        self.assertTrue(
-            "-----END PGP SIGNATURE-----" not in body,
-            "Unexpected end of PGP signature found")
+        self.PGPSignatureNotPreserved(archive=self.name16.archive)
 
     def doCustomUploadToPPA(self):
         """Helper method to do a custom upload to a PPA.

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-02-26 16:52:46 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2010-04-22 16:42:37 +0000
@@ -7,6 +7,7 @@
 __all__ = [
     "MockOptions",
     "MockLogger",
+    "TestUploadProcessorBase",
     ]
 
 import os
@@ -44,7 +45,8 @@
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
 from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
 from lp.soyuz.interfaces.queue import PackageUploadStatus
-from lp.soyuz.interfaces.publishing import PackagePublishingStatus
+from lp.soyuz.interfaces.publishing import (
+    IPublishingSet, PackagePublishingStatus)
 from lp.soyuz.interfaces.queue import QueueInconsistentStateError
 from canonical.launchpad.interfaces import ILibraryFileAliasSet
 from lp.soyuz.interfaces.packageset import IPackagesetSet
@@ -294,6 +296,29 @@
                 content in body,
                 "Expect: '%s'\nGot:\n%s" % (content, body))
 
+    def PGPSignatureNotPreserved(self, archive=None):
+        """PGP signatures should be removed from .changes files.
+
+        Email notifications and the librarian file for .changes file should
+        both have the PGP signature removed.
+        """
+        bar = archive.getPublishedSources(
+            name='bar', version="1.0-1", exact_match=True)
+        changes_lfa = getUtility(IPublishingSet).getChangesFileLFA(
+            bar[0].sourcepackagerelease)
+        changes_file = changes_lfa.read()
+        self.assertTrue(
+            "Format: " in changes_file, "Does not look like a changes file")
+        self.assertTrue(
+            "-----BEGIN PGP SIGNED MESSAGE-----" not in changes_file,
+            "Unexpected PGP header found")
+        self.assertTrue(
+            "-----BEGIN PGP SIGNATURE-----" not in changes_file,
+            "Unexpected start of PGP signature found")
+        self.assertTrue(
+            "-----END PGP SIGNATURE-----" not in changes_file,
+            "Unexpected end of PGP signature found")
+
 
 class TestUploadProcessor(TestUploadProcessorBase):
     """Basic tests on uploadprocessor class.
@@ -1695,6 +1720,30 @@
             ]
         self.assertEmail(contents, recipients=recipients)
 
+    def testPGPSignatureNotPreserved(self):
+        """PGP signatures should be removed from .changes files.
+
+        Email notifications and the librarian file for the .changes file
+        should both have the PGP signature removed.
+        """
+        uploadprocessor = self.setupBreezyAndGetUploadProcessor(
+	    policy='insecure')
+        upload_dir = self.queueUpload("bar_1.0-1")
+        self.processUpload(uploadprocessor, upload_dir)
+        # ACCEPT the upload
+        queue_items = self.breezy.getQueueItems(
+            status=PackageUploadStatus.NEW, name="bar",
+            version="1.0-1", exact_match=True)
+        self.assertEqual(queue_items.count(), 1)
+        queue_item = queue_items[0]
+        queue_item.setAccepted()
+        pubrec = queue_item.sources[0].publish(self.log)
+        pubrec.status = PackagePublishingStatus.PUBLISHED
+        pubrec.datepublished = UTC_NOW
+        queue_item.setDone()
+        self.PGPSignatureNotPreserved(archive=self.breezy.main_archive)
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)
 

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-04-09 15:46:09 +0000
+++ lib/lp/registry/model/distroseries.py	2010-04-22 16:42:37 +0000
@@ -1391,16 +1391,15 @@
         # at best, causing unpredictable corruption), and simply pass it
         # off to the librarian.
 
-        # The PGP signature is stripped from all changesfiles for PPAs
-        # to avoid replay attacks (see bug 159304).
-        if archive.is_ppa:
-            signed_message = signed_message_from_string(changesfilecontent)
-            if signed_message is not None:
-                # Overwrite `changesfilecontent` with the text stripped
-                # of the PGP signature.
-                new_content = signed_message.signedContent
-                if new_content is not None:
-                    changesfilecontent = signed_message.signedContent
+        # The PGP signature is stripped from all changesfiles
+        # to avoid replay attacks (see bugs 159304 and 451396).
+        signed_message = signed_message_from_string(changesfilecontent)
+        if signed_message is not None:
+            # Overwrite `changesfilecontent` with the text stripped
+            # of the PGP signature.
+            new_content = signed_message.signedContent
+            if new_content is not None:
+                changesfilecontent = signed_message.signedContent
 
         changes_file = getUtility(ILibraryFileAliasSet).create(
             changesfilename, len(changesfilecontent),

=== modified file 'lib/lp/soyuz/model/publishing.py'
--- lib/lp/soyuz/model/publishing.py	2010-04-12 11:37:48 +0000
+++ lib/lp/soyuz/model/publishing.py	2010-04-22 16:42:37 +0000
@@ -1515,8 +1515,8 @@
             LibraryFileAlias,
             LibraryFileAlias.id == PackageUpload.changesfileID,
             PackageUpload.status == PackageUploadStatus.DONE,
-            PackageUpload.distroseriesID == spr.upload_distroseriesID,
-            PackageUpload.archiveID == spr.upload_archiveID,
+            PackageUpload.distroseriesID == spr.upload_distroseries.id,
+            PackageUpload.archiveID == spr.upload_archive.id,
             PackageUpload.id == PackageUploadSource.packageuploadID,
             PackageUploadSource.sourcepackagereleaseID == spr.id)
         return result_set.one()

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2010-04-14 11:45:13 +0000
+++ lib/lp/soyuz/model/queue.py	2010-04-22 16:42:37 +0000
@@ -398,9 +398,12 @@
 
         self.setAccepted()
         changes_file_object = StringIO.StringIO(self.changesfile.read())
+        # We explicitly allow unsigned uploads here since the .changes file
+        # is pulled from the librarian which are stripped of their
+        # signature just before being stored.
         self.notify(
             announce_list=announce_list, logger=logger, dry_run=dry_run,
-            changes_file_object=changes_file_object)
+            changes_file_object=changes_file_object, allow_unsigned=True)
         self.syncUpdate()
 
         # If this is a single source upload we can create the
@@ -431,9 +434,11 @@
         """See `IPackageUpload`."""
         self.setRejected()
         changes_file_object = StringIO.StringIO(self.changesfile.read())
+        # We allow unsigned uploads since they come from the librarian,
+        # which are now stored unsigned.
         self.notify(
             logger=logger, dry_run=dry_run,
-            changes_file_object=changes_file_object)
+            changes_file_object=changes_file_object, allow_unsigned=True)
         self.syncUpdate()
 
     @property
@@ -700,11 +705,10 @@
             unsigned = allow_unsigned
         changes = parse_tagfile_lines(changes_lines, allow_unsigned=unsigned)
 
-        if self.isPPA():
-            # Leaving the PGP signature on a package uploaded to a PPA
-            # leaves the possibility of someone hijacking the notification
-            # and uploading to the Ubuntu archive as the signer.
-            changes_lines = self._stripPgpSignature(changes_lines)
+        # Leaving the PGP signature on a package uploaded
+        # leaves the possibility of someone hijacking the notification
+        # and uploading to any archive as the signer.
+        changes_lines = self._stripPgpSignature(changes_lines)
 
         return changes, changes_lines