launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00797
[Merge] lp:~benji/launchpad/bug-580035 into lp:launchpad/devel
Benji York has proposed merging lp:~benji/launchpad/bug-580035 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#580035 launchpad doesn't guard against replay attacks with signed mail
https://bugs.launchpad.net/bugs/580035
During QA this branch caused exceptions when processing GPG signed
emails.
I found that my understanding of the data structures involved was faulty
so the tests I wrote were not representative. I reworked the tests to
be realistic and added the data I needed to fix the bug to
PymeSignature.
You can run all the email tests that pertain to authentication that I
could find with this command:
bin/test -ct emailauthentication.txt -t bugs-emailinterface.txt \
-t signed_messages -t test_handlers -t test_helpers
--
https://code.launchpad.net/~benji/launchpad/bug-580035/+merge/33911
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~benji/launchpad/bug-580035 into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/interfaces/gpghandler.py'
--- lib/canonical/launchpad/interfaces/gpghandler.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/interfaces/gpghandler.py 2010-08-27 13:45:57 +0000
@@ -38,7 +38,7 @@
self.fingerprint = fingerprint
self.pubkey = pubkey
super(GPGKeyNotFoundError, self).__init__(
- "No GPG key found with the given content: %s" % (fingerprint,))
+ "No GPG key found with the given content: %s" % (fingerprint, ))
class GPGKeyRevoked(Exception):
@@ -47,7 +47,7 @@
def __init__(self, key):
self.key = key
super(GPGKeyRevoked, self).__init__(
- "%s has been publicly revoked" % (key.keyid,))
+ "%s has been publicly revoked" % (key.keyid, ))
class GPGKeyExpired(Exception):
@@ -55,7 +55,7 @@
def __init__(self, key):
self.key = key
- super(GPGKeyExpired, self).__init__("%s has expired" % (key.keyid,))
+ super(GPGKeyExpired, self).__init__("%s has expired" % (key.keyid, ))
class SecretGPGKeyImportDetected(Exception):
@@ -275,6 +275,7 @@
fingerprint = Attribute("Signer Fingerprint.")
plain_data = Attribute("Plain Signed Text.")
+ timestamp = Attribute("The time at which the message was signed.")
class IPymeKey(Interface):
=== modified file 'lib/canonical/launchpad/mail/handlers.py'
--- lib/canonical/launchpad/mail/handlers.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/handlers.py 2010-08-27 13:45:57 +0000
@@ -32,6 +32,7 @@
ISpecificationSet,
QuestionStatus,
)
+from canonical.launchpad.interfaces.gpghandler import IGPGHandler
from canonical.launchpad.mail.commands import (
BugEmailCommands,
get_error_message,
@@ -58,6 +59,16 @@
)
+def extract_signature_timestamp(signed_msg):
+ # break import cycle
+ from canonical.launchpad.mail.incoming import (
+ canonicalise_line_endings)
+ signature = getUtility(IGPGHandler).getVerifiedSignature(
+ canonicalise_line_endings(signed_msg.signedContent),
+ signed_msg.signature)
+ return signature.timestamp
+
+
class MaloneHandler:
"""Handles emails sent to Malone.
@@ -77,7 +88,8 @@
name, args in parse_commands(content,
BugEmailCommands.names())]
- def process(self, signed_msg, to_addr, filealias=None, log=None):
+ def process(self, signed_msg, to_addr, filealias=None, log=None,
+ extract_signature_timestamp=extract_signature_timestamp):
"""See IMailHandler."""
commands = self.getCommands(signed_msg)
user, host = to_addr.split('@')
@@ -89,7 +101,8 @@
CONTEXT = 'bug report'
ensure_not_weakly_authenticated(signed_msg, CONTEXT)
if signature is not None:
- ensure_sane_signature_timestamp(signature, CONTEXT)
+ ensure_sane_signature_timestamp(
+ extract_signature_timestamp(signed_msg), CONTEXT)
if user.lower() == 'new':
# A submit request.
=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/helpers.py 2010-08-27 13:45:57 +0000
@@ -230,7 +230,7 @@
raise IncomingEmailError(error_message)
-def ensure_sane_signature_timestamp(signature, context,
+def ensure_sane_signature_timestamp(timestamp, context,
error_template='old-signature.txt'):
"""Ensure the signature was generated recently but not in the future."""
fourty_eight_hours = 48 * 60 * 60
@@ -239,7 +239,7 @@
fourty_eight_hours_ago = now - fourty_eight_hours
ten_minutes_in_the_future = now + ten_minutes
- if (signature.timestamp < fourty_eight_hours_ago
- or signature.timestamp > ten_minutes_in_the_future):
+ if (timestamp < fourty_eight_hours_ago
+ or timestamp > ten_minutes_in_the_future):
error_message = get_error_message(error_template, context=context)
raise IncomingEmailError(error_message)
=== modified file 'lib/canonical/launchpad/mail/tests/test_handlers.py'
--- lib/canonical/launchpad/mail/tests/test_handlers.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/tests/test_handlers.py 2010-08-27 13:45:57 +0000
@@ -9,6 +9,7 @@
import unittest
from canonical.database.sqlbase import commit
+from canonical.launchpad.ftests import import_secret_test_key
from canonical.launchpad.mail.commands import BugEmailCommand
from canonical.launchpad.mail.handlers import MaloneHandler
from canonical.testing import LaunchpadFunctionalLayer
@@ -17,6 +18,7 @@
person_logged_in,
TestCaseWithFactory,
)
+from lp.testing.factory import GPGSigningContext
class TestMaloneHandler(TestCaseWithFactory):
@@ -68,39 +70,21 @@
layer = LaunchpadFunctionalLayer
- def test_far_past_gpg_signature_timestamp(self):
- # If an email message's GPG signature's timestamp is too far in the
- # past and the message contains a command, the command will fail and
- # send the user an email telling them what happened.
-
- msg = self.factory.makeSignedMessage(body=' security no')
- now = time.time()
- one_week = 60 * 60 * 24 * 7
- msg.signature = FakeSignature(timestamp=now-one_week)
- handler = MaloneHandler()
- with person_logged_in(self.factory.makePerson()):
- success = handler.process(msg, msg['To'])
- commit()
- email = get_last_email()
- self.assertEqual(email['subject'], 'Submit Request Failure')
- self.assertIn(BAD_SIGNATURE_TIMESTAMP_MESSAGE, email['body'])
-
- def test_far_future_gpg_signature_timestamp(self):
- # If an email message's GPG signature's timestamp is too far in the
- # future and the message contains a command, the command will fail and
- # send the user an email telling them what happened.
-
- msg = self.factory.makeSignedMessage(body=' security no')
- now = time.time()
- one_week = 60 * 60 * 24 * 7
- msg.signature = FakeSignature(timestamp=now+one_week)
- handler = MaloneHandler()
- with person_logged_in(self.factory.makePerson()):
- success = handler.process(msg, msg['To'])
- commit()
- email = get_last_email()
- self.assertEqual(email['subject'], 'Submit Request Failure')
- self.assertIn(BAD_SIGNATURE_TIMESTAMP_MESSAGE, email['body'])
+ def test_good_signature_timestamp(self):
+ # An email message's GPG signature's timestamp checked to be sure it
+ # isn't too far in the future or past. This test shows that a
+ # signature with a timestamp of appxoimately now will be accepted.
+ signing_context = GPGSigningContext(
+ import_secret_test_key().fingerprint, password='test')
+ msg = self.factory.makeSignedMessage(
+ body=' security no', signing_context=signing_context)
+ handler = MaloneHandler()
+ with person_logged_in(self.factory.makePerson()):
+ success = handler.process(msg, msg['To'])
+ commit()
+ # Since there were no commands in the poorly-timestamped message, no
+ # error emails were generated.
+ self.assertEqual(stub.test_emails, [])
def test_bad_timestamp_but_no_commands(self):
# If an email message's GPG signature's timestamp is too far in the
=== modified file 'lib/canonical/launchpad/mail/tests/test_helpers.py'
--- lib/canonical/launchpad/mail/tests/test_helpers.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/tests/test_helpers.py 2010-08-27 13:45:57 +0000
@@ -91,12 +91,6 @@
parse_commands(' command:', ['command']))
-class FakeSignature:
-
- def __init__(self, timestamp):
- self.timestamp = timestamp
-
-
class TestEnsureSaneSignatureTimestamp(unittest.TestCase):
"""Tests for ensure_sane_signature_timestamp"""
@@ -104,35 +98,31 @@
# signature timestamps shouldn't be too old
now = time.time()
one_week = 60 * 60 * 24 * 7
- signature = FakeSignature(timestamp=now-one_week)
self.assertRaises(
IncomingEmailError, ensure_sane_signature_timestamp,
- signature, 'bug report')
+ now-one_week, 'bug report')
def test_future_timestamp(self):
# signature timestamps shouldn't be (far) in the future
now = time.time()
one_week = 60 * 60 * 24 * 7
- signature = FakeSignature(timestamp=now+one_week)
self.assertRaises(
IncomingEmailError, ensure_sane_signature_timestamp,
- signature, 'bug report')
+ now+one_week, 'bug report')
def test_near_future_timestamp(self):
# signature timestamps in the near future are OK
now = time.time()
one_minute = 60
- signature = FakeSignature(timestamp=now+one_minute)
# this should not raise an exception
- ensure_sane_signature_timestamp(signature, 'bug report')
+ ensure_sane_signature_timestamp(now+one_minute, 'bug report')
def test_recent_timestamp(self):
# signature timestamps in the recent past are OK
now = time.time()
one_hour = 60 * 60
- signature = FakeSignature(timestamp=now-one_hour)
# this should not raise an exception
- ensure_sane_signature_timestamp(signature, 'bug report')
+ ensure_sane_signature_timestamp(now-one_hour, 'bug report')
class TestEnsureNotWeaklyAuthenticated(TestCaseWithFactory):
=== modified file 'lib/canonical/launchpad/utilities/gpghandler.py'
--- lib/canonical/launchpad/utilities/gpghandler.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/utilities/gpghandler.py 2010-08-27 13:45:57 +0000
@@ -218,8 +218,10 @@
"Unable to map subkey: %s" % signature.fpr)
# return the signature container
- return PymeSignature(fingerprint=key.fingerprint,
- plain_data=plain.getvalue())
+ return PymeSignature(
+ fingerprint=key.fingerprint,
+ plain_data=plain.getvalue(),
+ timestamp=signature.timestamp)
def importPublicKey(self, content):
"""See IGPGHandler."""
@@ -550,10 +552,11 @@
"""See IPymeSignature."""
implements(IPymeSignature)
- def __init__(self, fingerprint=None, plain_data=None):
+ def __init__(self, fingerprint=None, plain_data=None, timestamp=None):
"""Initialized a signature container."""
self.fingerprint = fingerprint
self.plain_data = plain_data
+ self.timestamp = timestamp
class PymeKey:
=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt 2010-08-18 18:23:37 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2010-08-27 13:45:57 +0000
@@ -50,16 +50,11 @@
signed, so that the system can verify the sender. But to avoid having
to sign each email, we'll create a class which fakes a signed email:
- >>> import time
- >>> class MockSignature(object):
- ... def __init__(self):
- ... self.timestamp = time.time()
-
>>> import email.Message
>>> class MockSignedMessage(email.Message.Message):
... def __init__(self, *args, **kws):
... email.Message.Message.__init__(self, *args, **kws)
- ... self.signature = MockSignature()
+ ... self.signature = 'fake'
... @property
... def signedMessage(self):
... return self
@@ -82,9 +77,14 @@
... msg['Message-Id'] = factory.makeUniqueRFC822MsgId()
... return msg
+ >>> import time
+ >>> def fake_extract_signature_timestamp(signed_msg):
+ ... return time.time()
+
>>> def process_email(raw_mail):
... msg = construct_email(raw_mail)
- ... handler.process(msg, msg['To'])
+ ... handler.process(msg, msg['To'],
+ ... extract_signature_timestamp=fake_extract_signature_timestamp)
>>> process_email(submit_mail)
@@ -410,7 +410,9 @@
... signature = None
>>> msg = email.message_from_string(
... comment_mail, _class=MockUnsignedMessage)
- >>> handler.process(msg, msg['To'])
+ >>> handler.process(
+ ... msg, msg['To'],
+ ... extract_signature_timestamp=fake_extract_signature_timestamp)
True
>>> commit()
@@ -482,7 +484,9 @@
... return construct_email(edit_mail)
>>> def submit_command_email(msg):
- ... handler.process(msg, msg['To'])
+ ... handler.process(
+ ... msg, msg['To'],
+ ... extract_signature_timestamp=fake_extract_signature_timestamp)
... commit()
... sync(bug)
@@ -1756,7 +1760,9 @@
>>> msg = signed_message_from_string(submit_mail)
>>> import email.Utils
>>> msg['Message-Id'] = email.Utils.make_msgid()
- >>> handler.process(msg, msg['To'])
+ >>> handler.process(
+ ... msg, msg['To'],
+ ... extract_signature_timestamp=fake_extract_signature_timestamp)
True
>>> print_latest_email()
Subject: Submit Request Failure
