launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:strip-more-changes-signatures into launchpad:master.

Commit message:
Strip PGP signature from .changes files in more places

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1915002 in Launchpad itself: "Signatures not stripped on mails to devel-changes"
  https://bugs.launchpad.net/launchpad/+bug/1915002

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/397665

We've done this for a long time in order to avoid replay attacks where people extract a .changes file and use it to upload the same thing to a different archive, but notifications for acceptances that didn't require manual approval weren't appropriately stripped.  Fix PackageUpload.notify to remove the signature.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:strip-more-changes-signatures into launchpad:master.

diff --git a/lib/lp/archiveuploader/tests/nascentupload-announcements.txt b/lib/lp/archiveuploader/tests/nascentupload-announcements.txt
index b0823b7..9e908d5 100644
--- a/lib/lp/archiveuploader/tests/nascentupload-announcements.txt
+++ b/lib/lp/archiveuploader/tests/nascentupload-announcements.txt
@@ -216,9 +216,6 @@ changes file is enclosed as an attachment.
     Content-Type: text/plain; charset="utf-8"
     Content-Transfer-Encoding: quoted-printable
     <BLANKLINE>
-    -----BEGIN PGP SIGNED MESSAGE-----
-    Hash: SHA1
-    <BLANKLINE>
     Format: 1.7
     Date: Thu, 16 Feb 2006 15:34:09 +0000
     Source: bar
@@ -243,14 +240,6 @@ changes file is enclosed as an attachment.
      fc1464e5985b962a042d5354452f361d 164 devel optional bar_1.0.orig.tar.gz
      1e35b810764f140af9616de8274e6e73 537 devel optional bar_1.0-1.diff.gz
     <BLANKLINE>
-    -----BEGIN PGP SIGNATURE-----
-    Version: GnuPG v1.4.3 (GNU/Linux)
-    <BLANKLINE>
-    iD8DBQFFt7D9jn63CGxkqMURAk1BAJwIQfOMS+l9lDDwPORtuZb3hFI2OgCaArNc
-    oH5uIHeKtedJa5Ekpcfi2bY=3D
-    =3DDMqI
-    -----END PGP SIGNATURE-----
-    <BLANKLINE>
 
 A PPA upload will contain the X-Launchpad-PPA header.
 
@@ -732,9 +721,6 @@ And what follows is the content of the attachment.
     Content-Type: text/plain; charset="utf-8"
     Content-Transfer-Encoding: quoted-printable
     <BLANKLINE>
-    -----BEGIN PGP SIGNED MESSAGE-----
-    Hash: SHA1
-    <BLANKLINE>
     Format: 1.7
     Date: Thu, 30 Mar 2006 01:36:14 +0100
     Source: bar
@@ -763,14 +749,6 @@ And what follows is the content of the attachment.
      a4932aa84fdb62819b49f3dda163fc0d 514 devel optional bar_1.0-10.dsc
      ac6b4efe44e31f47ec9f0d0fac6935f4 622 devel optional bar_1.0-10.diff.gz
     <BLANKLINE>
-    -----BEGIN PGP SIGNATURE-----
-    Version: GnuPG v1.4.6 (GNU/Linux)
-    <BLANKLINE>
-    iD8DBQFGjOzNjn63CGxkqMURAlDZAJ9CusNQWwbTJr7JHYV4Ka1JbWwmJACeLbs5
-    Hzkb2pxxwYzKs7uyCiFONlo=3D
-    =3D9dL/
-    -----END PGP SIGNATURE-----
-    <BLANKLINE>
 
 The attempt to upload a package with a malformed changes file name will
 result in a rejection email.
diff --git a/lib/lp/soyuz/doc/distroseriesqueue-notify.txt b/lib/lp/soyuz/doc/distroseriesqueue-notify.txt
index bc01590..2176890 100644
--- a/lib/lp/soyuz/doc/distroseriesqueue-notify.txt
+++ b/lib/lp/soyuz/doc/distroseriesqueue-notify.txt
@@ -199,6 +199,19 @@ The mail body contains the same list of files again:
     listed in this package's changelog.
     <BLANKLINE>
 
+All the emails have the PGP signature stripped from the .changes file to
+avoid replay attacks.
+
+    >>> print(msgs[0].get_payload(1).get_payload(decode=True).decode('UTF-8'))
+    Format: 1.7
+    ...
+    >>> print(msgs[1].get_payload(1).get_payload(decode=True).decode('UTF-8'))
+    Format: 1.7
+    ...
+    >>> print(msgs[2].get_payload(1).get_payload(decode=True).decode('UTF-8'))
+    Format: 1.7
+    ...
+
 notify() will also work without passing the changes_file_object
 parameter provided that everything is already committed to the database
 (which is not the case when nascent upload runs).  This example
diff --git a/lib/lp/soyuz/model/queue.py b/lib/lp/soyuz/model/queue.py
index c3452e2..e14e56a 100644
--- a/lib/lp/soyuz/model/queue.py
+++ b/lib/lp/soyuz/model/queue.py
@@ -891,12 +891,7 @@ class PackageUpload(SQLBase):
         if hasattr(changes_file_object, "seek"):
             changes_file_object.seek(0)
 
-        changes = parse_tagfile_content(changes_content)
-
-        # Leaving the PGP signature on a package uploaded
-        # leaves the possibility of someone hijacking the notification
-        # and uploading to any archive as the signer.
-        return changes, strip_pgp_signature(changes_content).splitlines(True)
+        return parse_tagfile_content(changes_content)
 
     def findSourcePublication(self):
         """Find the `SourcePackagePublishingHistory` for this build."""
@@ -937,9 +932,12 @@ class PackageUpload(SQLBase):
             PackageUploadStatus.ACCEPTED: 'accepted',
             PackageUploadStatus.DONE: 'accepted',
             }
-        changes, changes_lines = self._getChangesDict(changes_file_object)
+        changes = self._getChangesDict(changes_file_object)
         if changes_file_object is not None:
-            changesfile_content = changes_file_object.read()
+            # Strip any PGP signature so that the .changes file attached to
+            # the notification can't be used to reupload to another archive.
+            changesfile_content = strip_pgp_signature(
+                changes_file_object.read())
         else:
             changesfile_content = 'No changes file content available.'
         blamee = self.findPersonToNotify()