← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~wgrant/launchpad/bug-783855 into lp:launchpad

 

William Grant has proposed merging lp:~wgrant/launchpad/bug-783855 into lp:launchpad with lp:~wgrant/launchpad/bug-783855-1 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-783855/+merge/61708

The final, meaty part of the signature refactoring series, this branch switches parse_tagfiles_content to use apt_pkg.ParseTagFile, fixes SignableTagFile to verify the signature before processing verified content, and tests that the prefix attack is thwarted

The previously duplicated verification logic in ChangesFile/DSCFile.__init__ is now in SignableTagFile.parse. If requested to verify the signature, it now checks it first and runs the parser over only the verified content.

parse_tagfiles_content has had its custom signature-stripping parser removed, replaced with calls to strip_pgp_signature and apt_pkg.ParseTagFile. It has also had its dsc_whitespace_rules argument removed, as apt_pkg.ParseTagFile doesn't support such needless pedantry.

test_changesfile has been extended with three new tests that test behaviour of good, bad, and attack signatures.

test_tagfiles has had two failure tests (testCheckParseBadChanges and testCheckParseMalformedMultiline) neutralized, since apt_pkg.ParseTagFile isn't that pedantic. The former seems to be protection against a dpkg bug that was fixed several years ago, while the latter checks for something that dpkg will reject for later on anyway. Continuing to reject these two exceptionally rare cases early on is not worth the extra code of the parse_tagfiles_content reimplementation. Instead of checking for an exception, I just check that they process OK and don't crash in bad ways.

test_uploadprocessor had two tests fixed to accept new OOPS messages. With signature verification now being performed first, a missing signature results in a more helpful error than "Unable to find mandatory field 'Files' in the changes file".
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-783855/+merge/61708
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-783855 into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/changesfile.py'
--- lib/lp/archiveuploader/changesfile.py	2011-05-20 08:49:54 +0000
+++ lib/lp/archiveuploader/changesfile.py	2011-05-20 08:49:56 +0000
@@ -32,10 +32,6 @@
     UploadError,
     UploadWarning,
     )
-from lp.archiveuploader.tagfiles import (
-    parse_tagfile,
-    TagFileParseError,
-    )
 from lp.archiveuploader.utils import (
     determine_binary_file_type,
     determine_source_file_type,
@@ -103,11 +99,7 @@
         self.policy = policy
         self.logger = logger
 
-        try:
-            self._dict = parse_tagfile(self.filepath)
-        except (IOError, TagFileParseError), error:
-            raise UploadError("Unable to parse the changes %s: %s" % (
-                self.filename, error))
+        self.parse(verify_signature=not policy.unsigned_changes_ok)
 
         for field in self.mandatory_fields:
             if field not in self._dict:
@@ -126,11 +118,6 @@
                 "Format out of acceptable range for changes file. Range "
                 "1.5 - 2.0, format %g" % format)
 
-        if policy.unsigned_changes_ok:
-            self.logger.debug("Changes file can be unsigned.")
-        else:
-            self.processSignature()
-
     def checkFileName(self):
         """Make sure the changes file name is well-formed.
 

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2011-05-20 08:49:54 +0000
+++ lib/lp/archiveuploader/dscfile.py	2011-05-20 08:49:56 +0000
@@ -42,7 +42,7 @@
     UploadWarning,
     )
 from lp.archiveuploader.tagfiles import (
-    parse_tagfile,
+    parse_tagfile_content,
     TagFileParseError,
     )
 from lp.archiveuploader.utils import (
@@ -111,42 +111,62 @@
 class SignableTagFile:
     """Base class for signed file verification."""
 
-    fingerprint = None
     signingkey = None
+    parsed_content = None
 
     @property
     def signer(self):
         if self.signingkey is not None:
             return self.signingkey.owner
 
-    @property
-    def raw_content(self):
-        """Return files section contents."""
-        return self._dict['filecontents']
-
-    def processSignature(self):
-        """Verify the signature on the filename.
-
-        Stores the fingerprint, the IGPGKey used to sign, the owner of
-        the key and a dictionary containing
+    def parse(self, verify_signature=True):
+        """Parse the tag file, optionally verifying the signature.
+
+        If verify_signature is True, signingkey will be set to the signing
+        `IGPGKey`, and only the verified content will be parsed. Otherwise,
+        any signature will be stripped and the contained content parsed.
+
+        Will raise an `UploadError` if the tag file was unparsable,
+        or if signature verification was requested but failed.
+        """
+        try:
+            with open(self.filepath, 'rb') as f:
+                self.raw_content = f.read()
+        except IOError, error:
+            raise UploadError(
+                "Unable to read %s: %s" % (self.filename, error))
+
+        if verify_signature:
+            self.signingkey, self.parsed_content = self.verifySignature(
+                self.raw_content, self.filepath)
+        else:
+            self.logger.debug("%s can be unsigned." % self.filename)
+            self.parsed_content = self.raw_content
+        try:
+            self._dict = parse_tagfile_content(
+                self.parsed_content, filename=self.filepath)
+        except TagFileParseError, error:
+            raise UploadError(
+                "Unable to parse %s: %s" % (self.filename, error))
+
+    def verifySignature(self, content, filename):
+        """Verify the signature on the file content.
 
         Raise UploadError if the signing key cannot be found in launchpad
         or if the GPG verification failed for any other reason.
 
-        Returns the key owner (person object), the key (gpgkey object) and
-        the pyme signature as a three-tuple
+        Returns a tuple of the key (`IGPGKey` object) and the verified
+        cleartext data.
         """
-        self.logger.debug("Verifying signature on %s" % self.filename)
-        assert os.path.exists(self.filepath), (
-            "File not found: %s" % self.filepath)
+        self.logger.debug("Verifying signature on %s" % filename)
 
         try:
             sig = getUtility(IGPGHandler).getVerifiedSignatureResilient(
-                file(self.filepath, "rb").read())
+                content)
         except GPGVerificationError, error:
             raise UploadError(
                 "GPG verification of %s failed: %s" % (
-                self.filename, str(error)))
+                filename, str(error)))
 
         key = getUtility(IGPGKeySet).getByFingerprint(sig.fingerprint)
         if key is None:
@@ -155,9 +175,9 @@
 
         if key.active == False:
             raise UploadError("File %s is signed with a deactivated key %s"
-                              % (self.filename, key.keyid))
+                              % (filename, key.keyid))
 
-        self.signingkey = key
+        return (key, sig.plain_data)
 
     def parseAddress(self, addr, fieldname="Maintainer"):
         """Parse an address, using the policy to decide if we should add a
@@ -222,7 +242,6 @@
         "Build-Conflicts-Indep",
         "Format",
         "Standards-Version",
-        "filecontents",
         "homepage",
         ]))
 
@@ -247,11 +266,7 @@
         SourceUploadFile.__init__(
             self, filepath, digest, size, component_and_section, priority,
             package, version, changes, policy, logger)
-        try:
-            self._dict = parse_tagfile(self.filepath, dsc_whitespace_rules=1)
-        except (IOError, TagFileParseError), error:
-            raise UploadError(
-                "Unable to parse the dsc %s: %s" % (self.filename, error))
+        self.parse(verify_signature=not policy.unsigned_dsc_ok)
 
         self.logger.debug("Performing DSC verification.")
         for mandatory_field in self.mandatory_fields:
@@ -272,10 +287,6 @@
             raise EarlyReturnUploadError(
                 "Unsupported source format: %s" % self._dict['Format'])
 
-        if self.policy.unsigned_dsc_ok:
-            self.logger.debug("DSC file can be unsigned.")
-        else:
-            self.processSignature()
 
     #
     # Useful properties.
@@ -309,7 +320,6 @@
         """Return the DSC claimed binary line."""
         return self._dict['Binary']
 
-
     #
     # DSC file checks.
     #

=== modified file 'lib/lp/archiveuploader/tagfiles.py'
--- lib/lp/archiveuploader/tagfiles.py	2011-05-20 08:49:54 +0000
+++ lib/lp/archiveuploader/tagfiles.py	2011-05-20 08:49:56 +0000
@@ -10,171 +10,57 @@
     ]
 
 
-import re
+import tempfile
+
+import apt_pkg
+
+from lp.services.mail.signedmessage import strip_pgp_signature
 
 
 class TagFileParseError(Exception):
     """This exception is raised if parse_changes encounters nastiness"""
     pass
 
-re_single_line_field = re.compile(r"^(\S*)\s*:\s*(.*)")
-re_multi_line_field = re.compile(r"^(\s.*)")
-
-
-def parse_tagfile_content(content, dsc_whitespace_rules=0, filename=None):
+
+def parse_tagfile_content(content, filename=None):
     """Parses a tag file and returns a dictionary where each field is a key.
 
     The mandatory first argument is the contents of the tag file as a
     string.
 
-    dsc_whitespace_rules is an optional boolean argument which defaults
-    to off.  If true, it turns on strict format checking to avoid
-    allowing in source packages which are unextracable by the
-    inappropriately fragile dpkg-source.
-
-    The rules are:
-
-    o The PGP header consists of '-----BEGIN PGP SIGNED MESSAGE-----'
-      followed by any PGP header data and must end with a blank line.
-
-    o The data section must end with a blank line and must be followed by
-      '-----BEGIN PGP SIGNATURE-----'.
+    An OpenPGP cleartext signature will be stripped before parsing if
+    one is present.
     """
-    lines = content.splitlines(True)
-
-    error = ""
-
-    changes = {}
-
-    # Reindex by line number so we can easily verify the format of
-    # .dsc files...
-    index = 0
-    indexed_lines = {}
-    for line in lines:
-        index += 1
-        indexed_lines[index] = line[:-1]
-
-    inside_signature = 0
-
-    num_of_lines = len(indexed_lines.keys())
-    index = 0
-    first_value_for_newline_delimited_field = False
-    more_values_can_follow = False
-    while index < num_of_lines:
-        index += 1
-        line = indexed_lines[index]
-
-        # If the line is empty and we're not strictly enforcing whitespace
-        # rules, then just continue.
-        # If we're enforcing the rules, then check those rules, and maybe
-        # complain.
-        if line == "":
-            if dsc_whitespace_rules:
-                index += 1
-                if index > num_of_lines:
-                    raise TagFileParseError(
-                        "%s: invalid .dsc file at line %d" % (
-                            filename, index))
-                line = indexed_lines[index]
-                if not line.startswith("-----BEGIN PGP SIGNATURE"):
-                    raise TagFileParseError(
-                        "%s: invalid .dsc file at line %d -- "
-                        "expected PGP signature; got '%s'" % (
-                            filename, index,line))
-                inside_signature = 0
-                break
-            else:
-                continue
-        if line.startswith("-----BEGIN PGP SIGNATURE"):
-            break
-
-        # If we're at the start of a signed section, then consume the
-        # signature information, and remember that we're inside the signed
-        # data.
-        if line.startswith("-----BEGIN PGP SIGNED MESSAGE"):
-            inside_signature = 1
-            if dsc_whitespace_rules:
-                while index < num_of_lines and line != "":
-                    index += 1
-                    line = indexed_lines[index]
-            continue
-        slf = re_single_line_field.match(line)
-        if slf:
-            field = slf.groups()[0]
-            changes[field] = slf.groups()[1]
-
-            # If there is no value on this line, we assume this is
-            # the first line of a multiline field, such as the 'files'
-            # field.
-            if changes[field] == '':
-                first_value_for_newline_delimited_field = True
-
-            # Either way, more values for this field could follow
-            # on the next line.
-            more_values_can_follow = True
-            continue
-        if line.rstrip() == " .":
-            changes[field] += '\n' + line
-            continue
-        mlf = re_multi_line_field.match(line)
-        if mlf:
-            if more_values_can_follow is False:
-                raise TagFileParseError(
-                    "%s: could not parse .changes file line %d: '%s'\n"
-                    " [Multi-line field continuing on from nothing?]" % (
-                        filename, index,line))
-
-            # XXX Michael Nelson 20091001 bug=440014
-            # Is there any reason why we're not simply using
-            # apt_pkg.ParseTagFile instead of this looong function.
-            # If we can get rid of this code that is trying to mimic
-            # what ParseTagFile does out of the box, it would be a good
-            # thing.
-
-            # The first value for a newline delimited field, such as
-            # the 'files' field, has its leading spaces stripped. Other
-            # fields (such as a 'binary' field spanning multiple lines)
-            # should *not* be l-stripped of their leading spaces otherwise
-            # they will be re-parsed incorrectly by apt_get.ParseTagFiles()
-            # (for example, from a Source index).
-            value = mlf.groups()[0]
-            if first_value_for_newline_delimited_field:
-                changes[field] = value.lstrip()
-            else:
-                changes[field] += '\n' + value
-
-            first_value_for_newline_delimited_field = False
-            continue
-        error += line
-
-    if dsc_whitespace_rules and inside_signature:
-        raise TagFileParseError(
-            "%s: invalid .dsc format at line %d" % (filename, index))
-
-    changes["filecontents"] = "".join(lines)
-
-    if error:
-        raise TagFileParseError(
-            "%s: unable to parse .changes file: %s" % (filename, error))
-
-    return changes
-
-
-def parse_tagfile(filename, dsc_whitespace_rules=0):
+
+    with tempfile.TemporaryFile() as f:
+        f.write(strip_pgp_signature(content))
+        f.seek(0)
+        stanzas = list(apt_pkg.ParseTagFile(f))
+    if len(stanzas) != 1:
+        raise TagFileParseError(
+            "%s: multiple stanzas where only one is expected" % filename)
+
+    [stanza] = stanzas
+
+    # We can't do this sensibly with dict() or update(), as it has some
+    # keys without values.
+    trimmed_dict = {}
+    for key in stanza.keys():
+        try:
+            trimmed_dict[key] = stanza[key]
+        except KeyError:
+            pass
+    return trimmed_dict
+
+
+def parse_tagfile(filename):
     """Parses a tag file and returns a dictionary where each field is a key.
 
     The mandatory first argument is the filename of the tag file, and
     the contents of that file is passed on to parse_tagfile_content.
-
-    See parse_tagfile_content's docstring for description of the
-    dsc_whitespace_rules argument.
     """
-    changes_in = open(filename, "r")
-    content = changes_in.read()
-    changes_in.close()
+    with open(filename, "r") as changes_in:
+        content = changes_in.read()
     if not content:
-        raise TagFileParseError( "%s: empty file" % filename )
-    return parse_tagfile_content(
-        content, dsc_whitespace_rules=dsc_whitespace_rules,
-        filename=filename)
-
+        raise TagFileParseError("%s: empty file" % filename)
+    return parse_tagfile_content(content, filename=filename)

=== added directory 'lib/lp/archiveuploader/tests/data/signatures'
=== added file 'lib/lp/archiveuploader/tests/data/signatures/prefixed.changes'
--- lib/lp/archiveuploader/tests/data/signatures/prefixed.changes	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/data/signatures/prefixed.changes	2011-05-20 08:49:56 +0000
@@ -0,0 +1,56 @@
+-----BEGIN PGP SIGNED MESSAGE
+Hash: SHA1
+
+Format: 1.6
+Date: Thu, 06 Jun 2006 06:06:06 +0000
+Source: evil
+Binary: evil
+Architecture: source
+Version: 1.0-1
+Distribution: evil
+Urgency: low
+Maintainer: Evil person <owner@xxxxxxxxxxxxxxxx>
+Changed-By: Evil minion <minion@xxxxxxxxxxxxxxxx>
+Description: 
+ evil        - Stuff for evil
+Changes: 
+ evil (1.0-1) evil; urgency=low
+ .
+   * Evil prefixed version.
+Files: 
+ eeeeeeeeeeeeeeeeeeeeeeeeeeeeevil 512 devel optional evil_1.0-1.dsc
+ eeeeeeeeeeeeeeeeeeeeeeeeeeeeevil 160 devel optional evil_1.0.orig.tar.gz
+ eeeeeeeeeeeeeeeeeeeeeeeeeeeeevil 563 devel optional evil_1.0-1.diff.gz
+
+-----BEGIN PGP SIGNATURE
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA1
+
+Format: 1.7
+Date: Thu, 16 Feb 2006 15:34:09 +0000
+Source: foo
+Binary: foo
+Architecture: source
+Version: 1.0-1
+Distribution: breezy
+Urgency: low
+Maintainer: Launchpad team <launchpad@xxxxxxxxxxxxxxxxxxx>
+Changed-By: Daniel Silverstone <daniel.silverstone@xxxxxxxxxxxxx>
+Description: 
+ foo        - Stuff for testing
+Changes: 
+ foo (1.0-1) breezy; urgency=low
+ .
+   * Initial version
+Files: 
+ 7a7c9ecf9c5aa615e47813ac61118e65 512 devel optional foo_1.0-1.dsc
+ 4a7a1ab59ed8f9f1021825ba4b7ea826 160 devel optional foo_1.0.orig.tar.gz
+ d2093cb3512e747d92b7f7342a7cbb15 563 devel optional foo_1.0-1.diff.gz
+
+-----BEGIN PGP SIGNATURE-----
+Version: GnuPG v1.4.3 (GNU/Linux)
+
+iD8DBQFFt77djn63CGxkqMURAholAKCd1ZfLy3T2PfOYT1cLx5bcm/fm6QCgh1HJ
+vtYGI10xoIYvU3E/XRshbPA=
+=yVGM
+-----END PGP SIGNATURE-----

=== added file 'lib/lp/archiveuploader/tests/data/signatures/signed.changes'
--- lib/lp/archiveuploader/tests/data/signatures/signed.changes	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/data/signatures/signed.changes	2011-05-20 08:49:56 +0000
@@ -0,0 +1,31 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA1
+
+Format: 1.7
+Date: Thu, 16 Feb 2006 15:34:09 +0000
+Source: foo
+Binary: foo
+Architecture: source
+Version: 1.0-1
+Distribution: breezy
+Urgency: low
+Maintainer: Launchpad team <launchpad@xxxxxxxxxxxxxxxxxxx>
+Changed-By: Daniel Silverstone <daniel.silverstone@xxxxxxxxxxxxx>
+Description: 
+ foo        - Stuff for testing
+Changes: 
+ foo (1.0-1) breezy; urgency=low
+ .
+   * Initial version
+Files: 
+ 7a7c9ecf9c5aa615e47813ac61118e65 512 devel optional foo_1.0-1.dsc
+ 4a7a1ab59ed8f9f1021825ba4b7ea826 160 devel optional foo_1.0.orig.tar.gz
+ d2093cb3512e747d92b7f7342a7cbb15 563 devel optional foo_1.0-1.diff.gz
+
+-----BEGIN PGP SIGNATURE-----
+Version: GnuPG v1.4.3 (GNU/Linux)
+
+iD8DBQFFt77djn63CGxkqMURAholAKCd1ZfLy3T2PfOYT1cLx5bcm/fm6QCgh1HJ
+vtYGI10xoIYvU3E/XRshbPA=
+=yVGM
+-----END PGP SIGNATURE-----

=== added file 'lib/lp/archiveuploader/tests/data/signatures/unsigned.changes'
--- lib/lp/archiveuploader/tests/data/signatures/unsigned.changes	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/data/signatures/unsigned.changes	2011-05-20 08:49:56 +0000
@@ -0,0 +1,20 @@
+Format: 1.7
+Date: Thu, 16 Feb 2006 15:34:09 +0000
+Source: foo
+Binary: foo
+Architecture: source
+Version: 1.0-1
+Distribution: breezy
+Urgency: low
+Maintainer: Launchpad team <launchpad@xxxxxxxxxxxxxxxxxxx>
+Changed-By: Daniel Silverstone <daniel.silverstone@xxxxxxxxxxxxx>
+Description: 
+ foo        - Stuff for testing
+Changes: 
+ foo (1.0-1) breezy; urgency=low
+ .
+   * Initial version
+Files: 
+ 7a7c9ecf9c5aa615e47813ac61118e65 512 devel optional foo_1.0-1.dsc
+ 4a7a1ab59ed8f9f1021825ba4b7ea826 160 devel optional foo_1.0.orig.tar.gz
+ d2093cb3512e747d92b7f7342a7cbb15 563 devel optional foo_1.0-1.diff.gz

=== modified file 'lib/lp/archiveuploader/tests/test_changesfile.py'
--- lib/lp/archiveuploader/tests/test_changesfile.py	2010-12-24 20:08:16 +0000
+++ lib/lp/archiveuploader/tests/test_changesfile.py	2011-05-20 08:49:56 +0000
@@ -8,8 +8,13 @@
 import os
 
 from debian.deb822 import Changes
+from zope.component import getUtility
 
-from canonical.testing.layers import LaunchpadZopelessLayer
+from canonical.launchpad.ftests import import_public_test_keys
+from canonical.testing.layers import (
+    LaunchpadZopelessLayer,
+    ZopelessDatabaseLayer,
+    )
 from lp.archiveuploader.changesfile import (
     CannotDetermineFileTypeError,
     ChangesFile,
@@ -23,9 +28,15 @@
     UdebBinaryUploadFile,
     UploadError,
     )
-from lp.archiveuploader.tests import AbsolutelyAnythingGoesUploadPolicy
+from lp.archiveuploader.tests import (
+    AbsolutelyAnythingGoesUploadPolicy,
+    datadir,
+    )
+from lp.archiveuploader.uploadpolicy import InsecureUploadPolicy
+from lp.registry.interfaces.person import IPersonSet
 from lp.services.log.logger import BufferLogger
 from lp.testing import TestCase
+from lp.testing.keyserver import KeyServerTac
 
 
 class TestDetermineFileClassAndName(TestCase):
@@ -188,3 +199,43 @@
         self.assertRaises(
             UploadError,
             self.createChangesFile, "mypkg_0.1_i386.changes", contents)
+
+
+class TestSignatureVerification(TestCase):
+
+    layer = ZopelessDatabaseLayer
+
+    def setUp(self):
+        super(TestSignatureVerification, self).setUp()
+        self.useFixture(KeyServerTac())
+        import_public_test_keys()
+
+    def test_valid_signature_accepted(self):
+        path = datadir('signatures/signed.changes')
+        parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+        self.assertEqual(
+            getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx'),
+            parsed.signer)
+        expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            expected,
+            parsed.parsed_content)
+
+    def test_no_signature_rejected(self):
+        path = datadir('signatures/unsigned.changes')
+        self.assertRaises(
+            UploadError,
+            ChangesFile, path, InsecureUploadPolicy(), BufferLogger())
+
+    def test_prefix_ignored(self):
+        path = datadir('signatures/prefixed.changes')
+        parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+        self.assertEqual(
+            getUtility(IPersonSet).getByEmail('foo.bar@xxxxxxxxxxxxx'),
+            parsed.signer)
+        expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
+        self.assertTextMatchesExpressionIgnoreWhitespace(
+            expected,
+            parsed.parsed_content)
+        self.assertEqual("breezy", parsed.suite_name)
+        self.assertNotIn("evil", parsed.changes_comment)

=== modified file 'lib/lp/archiveuploader/tests/test_tagfiles.py'
--- lib/lp/archiveuploader/tests/test_tagfiles.py	2011-05-20 08:49:54 +0000
+++ lib/lp/archiveuploader/tests/test_tagfiles.py	2011-05-20 08:49:56 +0000
@@ -22,34 +22,37 @@
         """lp.archiveuploader.tagfiles.parse_tagfile should work on a good
            changes file
         """
-        p = parse_tagfile(datadir("good-signed-changes"))
-
-    def testCheckParseBadChangesRaises(self):
-        """lp.archiveuploader.tagfiles.parse_chantges should raise
-           TagFileParseError on failure
-        """
-        self.assertRaises(TagFileParseError,
-                          parse_tagfile, datadir("badformat-changes"), 1)
+        parse_tagfile(datadir("good-signed-changes"))
+
+    def testCheckParseBadChanges(self):
+        """Malformed but somewhat readable files do not raise an exception.
+
+        We let apt_pkg make of them what it can, and dpkg-source will
+        reject them if it can't understand.
+        """
+        parse_tagfile(datadir("bad-multiline-changes"))
+
+    def testCheckParseMalformedMultiline(self):
+        """Malformed but somewhat readable files do not raise an exception.
+
+        We let apt_pkg make of them what it can, and dpkg-source will
+        reject them if it can't understand.
+        """
+        parse_tagfile(datadir("bad-multiline-changes"))
 
     def testCheckParseEmptyChangesRaises(self):
         """lp.archiveuploader.tagfiles.parse_chantges should raise
            TagFileParseError on empty
         """
         self.assertRaises(TagFileParseError,
-                          parse_tagfile, datadir("empty-file"), 1)
+                          parse_tagfile, datadir("empty-file"))
 
     def testCheckParseMalformedSigRaises(self):
         """lp.archiveuploader.tagfiles.parse_chantges should raise
            TagFileParseError on malformed signatures
         """
         self.assertRaises(TagFileParseError,
-                          parse_tagfile, datadir("malformed-sig-changes"), 1)
-
-    def testCheckParseMalformedMultilineRaises(self):
-        """lp.archiveuploader.tagfiles.parse_chantges should raise
-           TagFileParseError on malformed continuation lines"""
-        self.assertRaises(TagFileParseError,
-                          parse_tagfile, datadir("bad-multiline-changes"), 1)
+                          parse_tagfile, datadir("malformed-sig-changes"))
 
     def testCheckParseUnterminatedSigRaises(self):
         """lp.archiveuploader.tagfiles.parse_changes should raise
@@ -57,8 +60,7 @@
         """
         self.assertRaises(TagFileParseError,
                           parse_tagfile,
-                          datadir("unterminated-sig-changes"),
-                          1)
+                          datadir("unterminated-sig-changes"))
 
     def testParseChangesNotVulnerableToArchExploit(self):
         """lp.archiveuploader.tagfiles.parse_tagfile should not be vulnerable

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-05-17 13:48:09 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2011-05-20 08:49:56 +0000
@@ -1179,9 +1179,9 @@
         fp = StringIO()
         error_report.write(fp)
         error_text = fp.getvalue()
-        self.assertIn(
-            "Unable to find mandatory field 'Files' "
-            "in the changes file", error_text)
+        expected_explanation = (
+            "Verification failed 3 times: ['No data', 'No data', 'No data']")
+        self.assertIn(expected_explanation, error_text)
 
         # Housekeeping so the next test won't fail.
         shutil.rmtree(upload_dir)
@@ -1348,8 +1348,10 @@
             'Expected Exception type not found in OOPS report:\n%s'
             % error_text)
 
+        # The upload policy requires a signature but none is present, so
+        # we get gpg verification errors.
         expected_explanation = (
-            "Unable to find mandatory field 'Files' in the changes file.")
+            "Verification failed 3 times: ['No data', 'No data', 'No data']")
         self.failUnless(
             error_text.find(expected_explanation) >= 0,
             'Expected Exception text not found in OOPS report:\n%s'