launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03689
[Merge] lp:~wgrant/launchpad/bug-783855-0 into lp:launchpad
William Grant has proposed merging lp:~wgrant/launchpad/bug-783855-0 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-783855-0/+merge/61710
As the first part of the big signature verification refactoring, this branch makes a few changes to parse_tagfile_lines. Firstly, none of the callsites actually want to give it lines, so it now takes a single string instead. Secondly, allow_unsigned is gone and always assumed to be True, since signature checking is done by processSignature. Thirdly, PackageUpload._stripPgpSignature is moved into lp.services.mail.signedmessage, because it's generally useful. Lastly, the unused TagFile and TagStanza have been removed.
Note that the second point (removal of allow_unsigned) opens up some attack vectors similar to the one that this series of branches is intended to fix. This will be fixed in the last branch of the series by only parsing the verified content.
--
https://code.launchpad.net/~wgrant/launchpad/bug-783855-0/+merge/61710
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-783855-0 into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/changesfile.py'
--- lib/lp/archiveuploader/changesfile.py 2010-10-03 15:30:06 +0000
+++ lib/lp/archiveuploader/changesfile.py 2011-05-20 08:27:13 +0000
@@ -104,8 +104,7 @@
self.logger = logger
try:
- self._dict = parse_tagfile(
- self.filepath, allow_unsigned=self.policy.unsigned_changes_ok)
+ self._dict = parse_tagfile(self.filepath)
except (IOError, TagFileParseError), error:
raise UploadError("Unable to parse the changes %s: %s" % (
self.filename, error))
=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py 2011-03-21 12:55:50 +0000
+++ lib/lp/archiveuploader/dscfile.py 2011-05-20 08:27:13 +0000
@@ -243,9 +243,7 @@
self, filepath, digest, size, component_and_section, priority,
package, version, changes, policy, logger)
try:
- self._dict = parse_tagfile(
- self.filepath, dsc_whitespace_rules=1,
- allow_unsigned=self.policy.unsigned_dsc_ok)
+ 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))
=== modified file 'lib/lp/archiveuploader/tagfiles.py'
--- lib/lp/archiveuploader/tagfiles.py 2010-08-27 12:21:25 +0000
+++ lib/lp/archiveuploader/tagfiles.py 2011-05-20 08:27:13 +0000
@@ -4,71 +4,15 @@
"""Utility classes for parsing Debian tag files."""
__all__ = [
- 'TagFile',
- 'TagStanza',
'TagFileParseError',
'parse_tagfile',
- 'parse_tagfile_lines'
+ 'parse_tagfile_content'
]
-import apt_pkg
import re
-class TagFile(object):
- """Provide an iterable interface to the apt_pkg.TagFile object"""
-
- def __init__(self, f):
- """Initialise apt_pkg and parse the tagfile provided by f"""
- if not isinstance(f, file):
- raise ValueError()
- apt_pkg.init()
- self.stanzas = apt_pkg.ParseTagFile(f)
-
- def __iter__(self):
- """Iterate across the stanzas in the tagfile"""
- self.stanzas.Jump(0)
- yield TagStanza(self.stanzas.Section)
- while self.stanzas.Step():
- yield TagStanza(self.stanzas.Section)
-
- def __getitem__(self, item):
- """Implement the [] operator"""
- self.stanzas.Jump(item)
- return TagStanza(self.stanzas.Section)
-
-class TagStanza(object):
- """Provide an iterable interface to apt_pkg.TagStanza"""
-
- def __init__(self, stanza):
- """Initialise given a stanza (usually from TagFile.__iter__)"""
- self.stanza = stanza
-
- def __getitem__(self, item):
- """The [] operator"""
- return self.stanza[item]
-
- def __iter__(self):
- """Iterate across keys"""
- for k in self.stanza.keys():
- yield k
-
- def keys(self):
- """Expose the .keys() method"""
- return self.stanza.keys()
-
- def has_key(self, key):
- """Expose a dicty has_key"""
- return key in self.stanza.keys()
-
- # Enables (foo in bar) functionality.
- __contains__ = has_key
-
- def items(self):
- """Allows for k,v in foo.items()"""
- return [ (k, self.stanza[k]) for k in self.stanza.keys() ]
-
class TagFileParseError(Exception):
"""This exception is raised if parse_changes encounters nastiness"""
pass
@@ -77,12 +21,11 @@
re_multi_line_field = re.compile(r"^(\s.*)")
-def parse_tagfile_lines(lines, dsc_whitespace_rules=0, allow_unsigned=False,
- filename=None):
+def parse_tagfile_content(content, dsc_whitespace_rules=0, 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
- list of lines.
+ string.
dsc_whitespace_rules is an optional boolean argument which defaults
to off. If true, it turns on strict format checking to avoid
@@ -97,6 +40,8 @@
o The data section must end with a blank line and must be followed by
'-----BEGIN PGP SIGNATURE-----'.
"""
+ lines = content.splitlines(True)
+
error = ""
changes = {}
@@ -153,10 +98,6 @@
index += 1
line = indexed_lines[index]
continue
- # If we're not inside the signed data, don't process anything, unless
- # we've decided to allow unsigned files.
- if not (inside_signature or allow_unsigned):
- continue
slf = re_single_line_field.match(line)
if slf:
field = slf.groups()[0]
@@ -219,21 +160,21 @@
return changes
-def parse_tagfile(filename, dsc_whitespace_rules=0, allow_unsigned=False):
+def parse_tagfile(filename, dsc_whitespace_rules=0):
"""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_lines.
+ the contents of that file is passed on to parse_tagfile_content.
- See parse_tagfile_lines's docstring for description of the
- dsc_whitespace_rules and allow_unsigned arguments.
+ See parse_tagfile_content's docstring for description of the
+ dsc_whitespace_rules argument.
"""
changes_in = open(filename, "r")
- lines = changes_in.readlines()
+ content = changes_in.read()
changes_in.close()
- if not lines:
+ if not content:
raise TagFileParseError( "%s: empty file" % filename )
- return parse_tagfile_lines(
- lines, dsc_whitespace_rules=dsc_whitespace_rules,
- allow_unsigned=allow_unsigned, filename=filename)
+ return parse_tagfile_content(
+ content, dsc_whitespace_rules=dsc_whitespace_rules,
+ filename=filename)
=== modified file 'lib/lp/archiveuploader/tests/test_tagfiles.py'
--- lib/lp/archiveuploader/tests/test_tagfiles.py 2010-08-27 12:21:25 +0000
+++ lib/lp/archiveuploader/tests/test_tagfiles.py 2011-05-20 08:27:13 +0000
@@ -11,7 +11,6 @@
from lp.archiveuploader.tagfiles import (
parse_tagfile,
- TagFile,
TagFileParseError,
)
from lp.archiveuploader.tests import datadir
@@ -19,29 +18,6 @@
class Testtagfiles(unittest.TestCase):
- def testTagFileOnSingular(self):
- """lp.archiveuploader.tagfiles.TagFile should parse a singular stanza
- """
- f = TagFile(file(datadir("singular-stanza"), "r"))
- seenone = False
- for stanza in f:
- self.assertEquals(seenone, False)
- seenone = True
- self.assertEquals("Format" in stanza, True)
- self.assertEquals("Source" in stanza, True)
- self.assertEquals("FooBar" in stanza, False)
-
- def testTagFileOnSeveral(self):
- """TagFile should parse multiple stanzas."""
- f = TagFile(file(datadir("multiple-stanzas"), "r"))
- seen = 0
- for stanza in f:
- seen += 1
- self.assertEquals("Format" in stanza, True)
- self.assertEquals("Source" in stanza, True)
- self.assertEquals("FooBar" in stanza, False)
- self.assertEquals(seen > 1, True)
-
def testCheckParseChangesOkay(self):
"""lp.archiveuploader.tagfiles.parse_tagfile should work on a good
changes file
@@ -104,8 +80,7 @@
self.apt_pkg_parsed_version = apt_pkg.ParseTagFile(tagfile)
self.apt_pkg_parsed_version.Step()
- self.parse_tagfile_version = parse_tagfile(
- tagfile_path, allow_unsigned = True)
+ self.parse_tagfile_version = parse_tagfile(tagfile_path)
def test_parse_tagfile_with_multiline_values(self):
"""parse_tagfile should not leave trailing '\n' on multiline values.
=== modified file 'lib/lp/services/mail/signedmessage.py'
--- lib/lp/services/mail/signedmessage.py 2010-12-02 16:13:51 +0000
+++ lib/lp/services/mail/signedmessage.py 2011-05-20 08:27:13 +0000
@@ -8,6 +8,7 @@
__all__ = [
'SignedMessage',
'signed_message_from_string',
+ 'strip_pgp_signature',
]
import email
@@ -150,3 +151,13 @@
"""
signature, signed_content = self._getSignatureAndSignedContent()
return signature
+
+
+def strip_pgp_signature(text):
+ """Strip any PGP signature from the supplied text."""
+ signed_message = signed_message_from_string(text)
+ # For unsigned text the signedContent will be None.
+ if signed_message.signedContent is not None:
+ return signed_message.signedContent
+ else:
+ return text
=== modified file 'lib/lp/soyuz/adapters/notification.py'
--- lib/lp/soyuz/adapters/notification.py 2011-05-19 09:07:30 +0000
+++ lib/lp/soyuz/adapters/notification.py 2011-05-20 08:27:13 +0000
@@ -69,7 +69,7 @@
# For now, it's just easier to re-read the original file if the caller
# requires us to do that instead of using the librarian's copy.
changes, changes_lines = packageupload._getChangesDict(
- changes_file_object, allow_unsigned=allow_unsigned)
+ changes_file_object)
# "files" will contain a list of tuples of filename,component,section.
# If files is empty, we don't need to send an email if this is not
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2011-05-19 09:07:30 +0000
+++ lib/lp/soyuz/model/queue.py 2011-05-20 08:27:13 +0000
@@ -41,9 +41,6 @@
)
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from canonical.launchpad.interfaces.lpstorm import IMasterStore
-from canonical.launchpad.mail import (
- signed_message_from_string,
- )
from canonical.librarian.interfaces import DownloadFailed
from canonical.librarian.utils import copy_and_close
from lp.app.errors import NotFoundError
@@ -52,13 +49,14 @@
# that it needs a bit of redesigning here around the publication stuff.
from lp.archivepublisher.config import getPubConfig
from lp.archivepublisher.customupload import CustomUploadError
-from lp.archiveuploader.tagfiles import parse_tagfile_lines
+from lp.archiveuploader.tagfiles import parse_tagfile_content
from lp.archiveuploader.utils import safe_fix_maintainer
from lp.registry.interfaces.person import IPersonSet
from lp.registry.interfaces.pocket import (
PackagePublishingPocket,
pocketsuffix,
)
+from lp.services.mail.signedmessage import strip_pgp_signature
from lp.services.propertycache import cachedproperty
from lp.soyuz.adapters.notification import notify
from lp.soyuz.enums import (
@@ -412,7 +410,7 @@
# signature just before being stored.
self.notify(
announce_list=announce_list, logger=logger, dry_run=dry_run,
- changes_file_object=changes_file_object, allow_unsigned=True)
+ changes_file_object=changes_file_object)
self.syncUpdate()
# If this is a single source upload we can create the
@@ -447,7 +445,7 @@
# which are now stored unsigned.
self.notify(
logger=logger, dry_run=dry_run,
- changes_file_object=changes_file_object, allow_unsigned=True)
+ changes_file_object=changes_file_object)
self.syncUpdate()
@property
@@ -651,7 +649,7 @@
self.notify(
announce_list=self.distroseries.changeslist,
changes_file_object=changes_file_object,
- allow_unsigned=True, logger=logger)
+ logger=logger)
self.syncUpdate()
self.setDone()
@@ -681,53 +679,29 @@
"""See `IPackageUpload`."""
return self.archive.is_ppa
- def _stripPgpSignature(self, changes_lines):
- """Strip any PGP signature from the supplied changes lines."""
- text = "".join(changes_lines)
- signed_message = signed_message_from_string(text)
- # For unsigned '.changes' files we'll get a None `signedContent`.
- if signed_message.signedContent is not None:
- return signed_message.signedContent.splitlines(True)
- else:
- return changes_lines
-
- def _getChangesDict(self, changes_file_object=None, allow_unsigned=None):
+ def _getChangesDict(self, changes_file_object=None):
"""Return a dictionary with changes file tags in it."""
- changes_lines = None
if changes_file_object is None:
changes_file_object = self.changesfile
- changes_lines = self.changesfile.read().splitlines(True)
- else:
- changes_lines = changes_file_object.readlines()
+ changes_content = changes_file_object.read()
# Rewind the file so that the next read starts at offset zero. Please
# note that a LibraryFileAlias does not support seek operations.
if hasattr(changes_file_object, "seek"):
changes_file_object.seek(0)
- # When the 'changesfile' content comes from a different
- # `PackageUpload` instance (e.g. when dealing with delayed copies)
- # we need to be able to specify the "allow unsigned" flag explicitly.
- # In that case the presence of the signing key is immaterial.
- if allow_unsigned is None:
- unsigned = not self.signing_key
- else:
- unsigned = allow_unsigned
- changes = parse_tagfile_lines(changes_lines, allow_unsigned=unsigned)
+ 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.
- changes_lines = self._stripPgpSignature(changes_lines)
-
- return changes, changes_lines
+ return changes, strip_pgp_signature(changes_content).splitlines(True)
def notify(self, announce_list=None, summary_text=None,
- changes_file_object=None, logger=None, dry_run=False,
- allow_unsigned=None):
+ changes_file_object=None, logger=None, dry_run=False):
"""See `IPackageUpload`."""
notify(self, announce_list, summary_text, changes_file_object,
- logger, dry_run, allow_unsigned)
+ logger, dry_run)
# XXX julian 2007-05-21:
# This method should really be IPersonSet.getByUploader but requires
=== modified file 'lib/lp/soyuz/scripts/gina/handlers.py'
--- lib/lp/soyuz/scripts/gina/handlers.py 2011-03-08 05:50:48 +0000
+++ lib/lp/soyuz/scripts/gina/handlers.py 2011-05-20 08:27:13 +0000
@@ -524,7 +524,7 @@
log.debug("Found a source package for %s (%s) in %s" % (sp_name,
sp_version, sp_component))
- dsc_contents = parse_tagfile(dsc_path, allow_unsigned=True)
+ dsc_contents = parse_tagfile(dsc_path)
dsc_contents = dict([
(name.lower(), value) for
(name, value) in dsc_contents.iteritems()])
=== modified file 'lib/lp/soyuz/scripts/processaccepted.py'
--- lib/lp/soyuz/scripts/processaccepted.py 2011-05-03 04:39:43 +0000
+++ lib/lp/soyuz/scripts/processaccepted.py 2011-05-20 08:27:13 +0000
@@ -25,7 +25,7 @@
ScriptRequest,
)
from lp.app.errors import NotFoundError
-from lp.archiveuploader.tagfiles import parse_tagfile_lines
+from lp.archiveuploader.tagfiles import parse_tagfile_content
from lp.bugs.interfaces.bug import IBugSet
from lp.bugs.interfaces.bugtask import BugTaskStatus
from lp.registry.interfaces.distribution import IDistributionSet
@@ -52,9 +52,7 @@
The bugs is specified in the Launchpad-bugs-fixed header, and are
separated by a space character. Nonexistent bug ids are ignored.
"""
- contents = changes_file.read()
- changes_lines = contents.splitlines(True)
- tags = Deb822Dict(parse_tagfile_lines(changes_lines, allow_unsigned=True))
+ tags = Deb822Dict(parse_tagfile_content(changes_file.read()))
bugs_fixed_line = tags.get('Launchpad-bugs-fixed', '')
bugs = []
for bug_id in bugs_fixed_line.split():
=== modified file 'lib/lp/soyuz/scripts/tests/test_sync_source.py'
--- lib/lp/soyuz/scripts/tests/test_sync_source.py 2011-01-17 21:51:09 +0000
+++ lib/lp/soyuz/scripts/tests/test_sync_source.py 2011-05-20 08:27:13 +0000
@@ -333,8 +333,7 @@
"Couldn't find %s." % expected_changesfile)
# Parse the generated unsigned changesfile.
- parsed_changes = parse_tagfile(
- expected_changesfile, allow_unsigned=True)
+ parsed_changes = parse_tagfile(expected_changesfile)
# It refers to the right source/version.
self.assertEqual(parsed_changes['Source'], 'bar')
@@ -408,8 +407,7 @@
"Couldn't find %s." % expected_changesfile)
# Parse the generated unsigned changesfile.
- parsed_changes = parse_tagfile(
- expected_changesfile, allow_unsigned=True)
+ parsed_changes = parse_tagfile(expected_changesfile)
# It refers to the right source/version.
self.assertEqual(parsed_changes['Source'], 'sample1')
