Launchpad itself

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

  1. fixes-bug-451396
  2. Merge into devel
Proposed by Steve Kowalik
Status: Merged
Approved by: Steve Kowalik
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp:~stevenk/launchpad/fixes-bug-451396
Merge into: lp:launchpad
Diff against target: 212 lines (+75/-40)
5 files modified
lib/lp/archiveuploader/tests/test_ppauploadprocessor.py (+3/-20)
lib/lp/archiveuploader/tests/test_uploadprocessor.py (+50/-1)
lib/lp/registry/model/distroseries.py (+9/-10)
lib/lp/soyuz/model/publishing.py (+2/-2)
lib/lp/soyuz/model/queue.py (+11/-7)
To merge this branch: bzr merge lp:~stevenk/launchpad/fixes-bug-451396
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
William Grant Pending
Review via email: mp+21706@code.launchpad.net

Commit message

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.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :

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
Revision history for this message
Abel Deuring (adeuring) wrote :

Steve,

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

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
682 'https://help.launchpad.net/Packaging/PPA for more information.')682 'https://help.launchpad.net/Packaging/PPA for more information.')
683683
684 def testPGPSignatureNotPreserved(self):684 def testPGPSignatureNotPreserved(self):
685 """PGP signatures should be removed from PPA changesfiles.685 """PGP signatures should be removed from PPA .changes files.
686686
687 Email notifications and the librarian file for the changesfile should687 Email notifications and the librarian file for .changes file should
688 both have the PGP signature removed.688 both have the PGP signature removed.
689 """689 """
690 upload_dir = self.queueUpload("bar_1.0-1", "~name16/ubuntu")690 upload_dir = self.queueUpload("bar_1.0-1", "~name16/ubuntu")
691 self.processUpload(self.uploadprocessor, upload_dir)691 self.processUpload(self.uploadprocessor, upload_dir)
692692 self.PGPSignatureNotPreserved(archive=self.name16.archive)
693 # Check the email.
694 from_addr, to_addrs, raw_msg = stub.test_emails.pop()
695 msg = message_from_string(raw_msg)
696
697 # This is now a MIMEMultipart message.
698 body = msg.get_payload(0)
699 body = body.get_payload(decode=True)
700
701 self.assertTrue(
702 "-----BEGIN PGP SIGNED MESSAGE-----" not in body,
703 "Unexpected PGP header found")
704 self.assertTrue(
705 "-----BEGIN PGP SIGNATURE-----" not in body,
706 "Unexpected start of PGP signature found")
707 self.assertTrue(
708 "-----END PGP SIGNATURE-----" not in body,
709 "Unexpected end of PGP signature found")
710693
711 def doCustomUploadToPPA(self):694 def doCustomUploadToPPA(self):
712 """Helper method to do a custom upload to a PPA.695 """Helper method to do a custom upload to a PPA.
713696
=== 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 @@
7__all__ = [7__all__ = [
8 "MockOptions",8 "MockOptions",
9 "MockLogger",9 "MockLogger",
10 "TestUploadProcessorBase",
10 ]11 ]
1112
12import os13import os
@@ -44,7 +45,8 @@
44from lp.registry.interfaces.sourcepackage import SourcePackageFileType45from lp.registry.interfaces.sourcepackage import SourcePackageFileType
45from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet46from lp.soyuz.interfaces.archive import ArchivePurpose, IArchiveSet
46from lp.soyuz.interfaces.queue import PackageUploadStatus47from lp.soyuz.interfaces.queue import PackageUploadStatus
47from lp.soyuz.interfaces.publishing import PackagePublishingStatus48from lp.soyuz.interfaces.publishing import (
49 IPublishingSet, PackagePublishingStatus)
48from lp.soyuz.interfaces.queue import QueueInconsistentStateError50from lp.soyuz.interfaces.queue import QueueInconsistentStateError
49from canonical.launchpad.interfaces import ILibraryFileAliasSet51from canonical.launchpad.interfaces import ILibraryFileAliasSet
50from lp.soyuz.interfaces.packageset import IPackagesetSet52from lp.soyuz.interfaces.packageset import IPackagesetSet
@@ -294,6 +296,29 @@
294 content in body,296 content in body,
295 "Expect: '%s'\nGot:\n%s" % (content, body))297 "Expect: '%s'\nGot:\n%s" % (content, body))
296298
299 def PGPSignatureNotPreserved(self, archive=None):
300 """PGP signatures should be removed from .changes files.
301
302 Email notifications and the librarian file for .changes file should
303 both have the PGP signature removed.
304 """
305 bar = archive.getPublishedSources(
306 name='bar', version="1.0-1", exact_match=True)
307 changes_lfa = getUtility(IPublishingSet).getChangesFileLFA(
308 bar[0].sourcepackagerelease)
309 changes_file = changes_lfa.read()
310 self.assertTrue(
311 "Format: " in changes_file, "Does not look like a changes file")
312 self.assertTrue(
313 "-----BEGIN PGP SIGNED MESSAGE-----" not in changes_file,
314 "Unexpected PGP header found")
315 self.assertTrue(
316 "-----BEGIN PGP SIGNATURE-----" not in changes_file,
317 "Unexpected start of PGP signature found")
318 self.assertTrue(
319 "-----END PGP SIGNATURE-----" not in changes_file,
320 "Unexpected end of PGP signature found")
321
297322
298class TestUploadProcessor(TestUploadProcessorBase):323class TestUploadProcessor(TestUploadProcessorBase):
299 """Basic tests on uploadprocessor class.324 """Basic tests on uploadprocessor class.
@@ -1695,6 +1720,30 @@
1695 ]1720 ]
1696 self.assertEmail(contents, recipients=recipients)1721 self.assertEmail(contents, recipients=recipients)
16971722
1723 def testPGPSignatureNotPreserved(self):
1724 """PGP signatures should be removed from .changes files.
1725
1726 Email notifications and the librarian file for the .changes file
1727 should both have the PGP signature removed.
1728 """
1729 uploadprocessor = self.setupBreezyAndGetUploadProcessor(
1730 policy='insecure')
1731 upload_dir = self.queueUpload("bar_1.0-1")
1732 self.processUpload(uploadprocessor, upload_dir)
1733 # ACCEPT the upload
1734 queue_items = self.breezy.getQueueItems(
1735 status=PackageUploadStatus.NEW, name="bar",
1736 version="1.0-1", exact_match=True)
1737 self.assertEqual(queue_items.count(), 1)
1738 queue_item = queue_items[0]
1739 queue_item.setAccepted()
1740 pubrec = queue_item.sources[0].publish(self.log)
1741 pubrec.status = PackagePublishingStatus.PUBLISHED
1742 pubrec.datepublished = UTC_NOW
1743 queue_item.setDone()
1744 self.PGPSignatureNotPreserved(archive=self.breezy.main_archive)
1745
1746
1698def test_suite():1747def test_suite():
1699 return unittest.TestLoader().loadTestsFromName(__name__)1748 return unittest.TestLoader().loadTestsFromName(__name__)
17001749
17011750
=== 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 @@
1391 # at best, causing unpredictable corruption), and simply pass it1391 # at best, causing unpredictable corruption), and simply pass it
1392 # off to the librarian.1392 # off to the librarian.
13931393
1394 # The PGP signature is stripped from all changesfiles for PPAs1394 # The PGP signature is stripped from all changesfiles
1395 # to avoid replay attacks (see bug 159304).1395 # to avoid replay attacks (see bugs 159304 and 451396).
1396 if archive.is_ppa:1396 signed_message = signed_message_from_string(changesfilecontent)
1397 signed_message = signed_message_from_string(changesfilecontent)1397 if signed_message is not None:
1398 if signed_message is not None:1398 # Overwrite `changesfilecontent` with the text stripped
1399 # Overwrite `changesfilecontent` with the text stripped1399 # of the PGP signature.
1400 # of the PGP signature.1400 new_content = signed_message.signedContent
1401 new_content = signed_message.signedContent1401 if new_content is not None:
1402 if new_content is not None:1402 changesfilecontent = signed_message.signedContent
1403 changesfilecontent = signed_message.signedContent
14041403
1405 changes_file = getUtility(ILibraryFileAliasSet).create(1404 changes_file = getUtility(ILibraryFileAliasSet).create(
1406 changesfilename, len(changesfilecontent),1405 changesfilename, len(changesfilecontent),
14071406
=== 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 @@
1515 LibraryFileAlias,1515 LibraryFileAlias,
1516 LibraryFileAlias.id == PackageUpload.changesfileID,1516 LibraryFileAlias.id == PackageUpload.changesfileID,
1517 PackageUpload.status == PackageUploadStatus.DONE,1517 PackageUpload.status == PackageUploadStatus.DONE,
1518 PackageUpload.distroseriesID == spr.upload_distroseriesID,1518 PackageUpload.distroseriesID == spr.upload_distroseries.id,
1519 PackageUpload.archiveID == spr.upload_archiveID,1519 PackageUpload.archiveID == spr.upload_archive.id,
1520 PackageUpload.id == PackageUploadSource.packageuploadID,1520 PackageUpload.id == PackageUploadSource.packageuploadID,
1521 PackageUploadSource.sourcepackagereleaseID == spr.id)1521 PackageUploadSource.sourcepackagereleaseID == spr.id)
1522 return result_set.one()1522 return result_set.one()
15231523
=== 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 @@
398398
399 self.setAccepted()399 self.setAccepted()
400 changes_file_object = StringIO.StringIO(self.changesfile.read())400 changes_file_object = StringIO.StringIO(self.changesfile.read())
401 # We explicitly allow unsigned uploads here since the .changes file
402 # is pulled from the librarian which are stripped of their
403 # signature just before being stored.
401 self.notify(404 self.notify(
402 announce_list=announce_list, logger=logger, dry_run=dry_run,405 announce_list=announce_list, logger=logger, dry_run=dry_run,
403 changes_file_object=changes_file_object)406 changes_file_object=changes_file_object, allow_unsigned=True)
404 self.syncUpdate()407 self.syncUpdate()
405408
406 # If this is a single source upload we can create the409 # If this is a single source upload we can create the
@@ -431,9 +434,11 @@
431 """See `IPackageUpload`."""434 """See `IPackageUpload`."""
432 self.setRejected()435 self.setRejected()
433 changes_file_object = StringIO.StringIO(self.changesfile.read())436 changes_file_object = StringIO.StringIO(self.changesfile.read())
437 # We allow unsigned uploads since they come from the librarian,
438 # which are now stored unsigned.
434 self.notify(439 self.notify(
435 logger=logger, dry_run=dry_run,440 logger=logger, dry_run=dry_run,
436 changes_file_object=changes_file_object)441 changes_file_object=changes_file_object, allow_unsigned=True)
437 self.syncUpdate()442 self.syncUpdate()
438443
439 @property444 @property
@@ -700,11 +705,10 @@
700 unsigned = allow_unsigned705 unsigned = allow_unsigned
701 changes = parse_tagfile_lines(changes_lines, allow_unsigned=unsigned)706 changes = parse_tagfile_lines(changes_lines, allow_unsigned=unsigned)
702707
703 if self.isPPA():708 # Leaving the PGP signature on a package uploaded
704 # Leaving the PGP signature on a package uploaded to a PPA709 # leaves the possibility of someone hijacking the notification
705 # leaves the possibility of someone hijacking the notification710 # and uploading to any archive as the signer.
706 # and uploading to the Ubuntu archive as the signer.711 changes_lines = self._stripPgpSignature(changes_lines)
707 changes_lines = self._stripPgpSignature(changes_lines)
708712
709 return changes, changes_lines713 return changes, changes_lines
710714