launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02017
[Merge] lp:~mbp/launchpad/dkim into lp:launchpad
Martin Pool has proposed merging lp:~mbp/launchpad/dkim into lp:launchpad.
Requested reviews:
Jonathan Lange (jml)
Launchpad code reviewers (launchpad-reviewers)
Graham Binns (gmb): code
Related bugs:
#316272 launchpad should verify gmail or DomainKeys authenticators
https://bugs.launchpad.net/bugs/316272
#643200 check for stale gpg signatures only cover malone
https://bugs.launchpad.net/bugs/643200
#643219 dkim-signed mail to new@bugs doesn't work
https://bugs.launchpad.net/bugs/643219
This fixes a few more things related to authentication of incoming mail:
* gpg signature timestamp checks should cover all mail, not just that to malone (bug 643200)
* checks on mail to new@ should make sure it's strongly authenticated, not specifically that it has a gpg signature (bug 643219)
* there was no test that gpg mail with implausible timestamps was actually rejected afaics
I haven't interactively tested this and I'm not utterly confident in the existing test coverage, so please review carefully and test it interactively yourself if you know how.
MaloneHandler.process was a bit large so I split out the code that decides what if any commands will be executed.
Thanks
--
https://code.launchpad.net/~mbp/launchpad/dkim/+merge/41819
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/dkim into lp:launchpad.
=== modified file 'lib/canonical/launchpad/doc/emailauthentication.txt'
--- lib/canonical/launchpad/doc/emailauthentication.txt 2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/doc/emailauthentication.txt 2010-11-25 04:47:03 +0000
@@ -20,12 +20,18 @@
>>> commit()
>>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
+For most of these tests, we don't care whether the timestamps are out of
+date:
+
+ >>> def accept_any_timestamp(timestamp, context_message):
+ ... pass
+
Now Sample Person and Foo Bar have one OpenPGP key each. Next, let's get
a test email that's signed and try to authenticate the user who sent it:
>>> from canonical.launchpad.mail.ftests import read_test_message
>>> msg = read_test_message('signed_detached.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
If the user isn't registered in Launchpad, None is return, if it
succeeds the authenticated principal:
@@ -52,7 +58,7 @@
message. Inline signatures are supported as well.
>>> msg = read_test_message('signed_inline.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
>>> principal is not None
True
>>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -65,7 +71,7 @@
As well as signed multipart messages:
>>> msg = read_test_message('signed_multipart.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
>>> principal is not None
True
>>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -80,7 +86,7 @@
to deal with it if we receive a dash escaped message.
>>> msg = read_test_message('signed_dash_escaped.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
>>> principal is not None
True
>>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -96,7 +102,7 @@
>>> msg = read_test_message('signed_detached_invalid_signature.txt')
>>> name, addr = email.Utils.parseaddr(msg['From'])
>>> from_user = getUtility(IPersonSet).getByEmail(addr)
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
Traceback (most recent call last):
...
InvalidSignature:...
@@ -131,7 +137,7 @@
... msg = email.message_from_string(
... line_ending.join(msg_lines), _class=SignedMessage)
... msg.parsed_string = msg.as_string()
- ... principal = authenticateEmail(msg)
+ ... principal = authenticateEmail(msg, accept_any_timestamp)
... authenticated_person = IPerson(principal)
... print authenticated_person.preferredemail.email
test@xxxxxxxxxxxxx
@@ -156,7 +162,7 @@
Content-Type: multipart/mixed; boundary="--------------------EuxKj2iCbKjpUGkD"
...
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
>>> print IPerson(principal).displayname
Sample Person
@@ -178,7 +184,7 @@
>>> from canonical.launchpad.interfaces.mail import (
... IWeaklyAuthenticatedPrincipal)
>>> msg = read_test_message('unsigned_multipart.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
>>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
True
@@ -191,7 +197,7 @@
authenticated user:
>>> msg = read_test_message('signed_key_not_registered.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
>>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
True
@@ -205,7 +211,7 @@
principal.
>>> msg = read_test_message('signed_inline.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
>>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
False
=== renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
=== modified file 'lib/canonical/launchpad/mail/handlers.py'
--- lib/canonical/launchpad/mail/handlers.py 2010-11-08 13:11:30 +0000
+++ lib/canonical/launchpad/mail/handlers.py 2010-11-25 04:47:03 +0000
@@ -15,7 +15,6 @@
from canonical.config import config
from canonical.database.sqlbase import rollback
from canonical.launchpad.helpers import get_email_template
-from canonical.launchpad.interfaces.gpghandler import IGPGHandler
from canonical.launchpad.interfaces.mail import (
EmailProcessingError,
IBugEditEmailCommand,
@@ -31,7 +30,6 @@
)
from canonical.launchpad.mail.helpers import (
ensure_not_weakly_authenticated,
- ensure_sane_signature_timestamp,
get_main_body,
guess_bugtask,
IncomingEmailError,
@@ -61,16 +59,6 @@
)
-def extract_signature_timestamp(signed_msg):
- # break import cycle
- from lp.services.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.
@@ -90,47 +78,64 @@
name, args in parse_commands(content,
BugEmailCommands.names())]
- def process(self, signed_msg, to_addr, filealias=None, log=None,
- extract_signature_timestamp=extract_signature_timestamp):
- """See IMailHandler."""
+ def extractAndAuthenticateCommands(self, signed_msg, to_addr):
+ """Extract commands and handle special destinations.
+
+ NB: The authentication is carried out against the current principal,
+ not directly against the message. authenticateEmail must previously
+ have been called on this thread.
+
+ :returns: (final_result, add_comment_to_bug, commands)
+ If final_result is non-none, stop processing and return this value
+ to indicate whether the message was dealt with or not.
+ If add_comment_to_bug, add the contents to the first bug
+ selected.
+ commands is a list of bug commands.
+ """
+ CONTEXT = 'bug report'
commands = self.getCommands(signed_msg)
- user, host = to_addr.split('@')
+ to_user, to_host = to_addr.split('@')
add_comment_to_bug = False
- signature = signed_msg.signature
+ # If there are any commands, we must have strong authentication.
+ # We send a different failure message for attempts to create a new
+ # bug.
+ if to_user.lower() == 'new':
+ ensure_not_weakly_authenticated(signed_msg, CONTEXT,
+ 'unauthenticated-bug-creation.txt')
+ elif len(commands) > 0:
+ ensure_not_weakly_authenticated(signed_msg, CONTEXT)
+ if to_user.lower() == 'new':
+ commands.insert(0, BugEmailCommands.get('bug', ['new']))
+ elif to_user.isdigit():
+ # A comment to a bug. We set add_comment_to_bug to True so
+ # that the comment gets added to the bug later. We don't add
+ # the comment now, since we want to let the 'bug' command
+ # handle the possible errors that can occur while getting
+ # the bug.
+ add_comment_to_bug = True
+ commands.insert(0, BugEmailCommands.get('bug', [to_user]))
+ elif to_user.lower() == 'help':
+ from_user = getUtility(ILaunchBag).user
+ if from_user is not None:
+ preferredemail = from_user.preferredemail
+ if preferredemail is not None:
+ to_address = str(preferredemail.email)
+ self.sendHelpEmail(to_address)
+ return True, False, None
+ elif to_user.lower() != 'edit':
+ # Indicate that we didn't handle the mail.
+ return False, False, None
+ return None, add_comment_to_bug, commands
+
+ def process(self, signed_msg, to_addr, filealias=None, log=None):
+ """See IMailHandler."""
try:
- if len(commands) > 0:
- CONTEXT = 'bug report'
- ensure_not_weakly_authenticated(signed_msg, CONTEXT)
- if signature is not None:
- ensure_sane_signature_timestamp(
- extract_signature_timestamp(signed_msg), CONTEXT)
-
- if user.lower() == 'new':
- # A submit request.
- commands.insert(0, BugEmailCommands.get('bug', ['new']))
- if signature is None:
- raise IncomingEmailError(
- get_error_message('not-gpg-signed.txt'))
- elif user.isdigit():
- # A comment to a bug. We set add_comment_to_bug to True so
- # that the comment gets added to the bug later. We don't add
- # the comment now, since we want to let the 'bug' command
- # handle the possible errors that can occur while getting
- # the bug.
- add_comment_to_bug = True
- commands.insert(0, BugEmailCommands.get('bug', [user]))
- elif user.lower() == 'help':
- from_user = getUtility(ILaunchBag).user
- if from_user is not None:
- preferredemail = from_user.preferredemail
- if preferredemail is not None:
- to_address = str(preferredemail.email)
- self.sendHelpEmail(to_address)
- return True
- elif user.lower() != 'edit':
- # Indicate that we didn't handle the mail.
- return False
+ (final_result, add_comment_to_bug,
+ commands, ) = self.extractAndAuthenticateCommands(
+ signed_msg, to_addr)
+ if final_result is not None:
+ return final_result
bug = None
bug_event = None
=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py 2010-11-08 13:11:30 +0000
+++ lib/canonical/launchpad/mail/helpers.py 2010-11-25 04:47:03 +0000
@@ -211,7 +211,14 @@
def ensure_not_weakly_authenticated(signed_msg, context,
error_template='not-signed.txt',
no_key_template='key-not-registered.txt'):
- """Make sure that the current principal is not weakly authenticated."""
+ """Make sure that the current principal is not weakly authenticated.
+
+ NB: While handling an email, the authentication state is stored partly in
+ properties of the message object, and partly in the current security
+ principal. As a consequence this function will only work correctly if the
+ message has just been passed through authenticateEmail -- you can't give
+ it an arbitrary message object.
+ """
cur_principal = get_current_principal()
# The security machinery doesn't know about
# IWeaklyAuthenticatedPrincipal yet, so do a manual
@@ -232,7 +239,18 @@
def ensure_sane_signature_timestamp(timestamp, context,
error_template='old-signature.txt'):
- """Ensure the signature was generated recently but not in the future."""
+ """Ensure the signature was generated recently but not in the future.
+
+ If the timestamp is wrong, the message is rejected rather than just
+ treated as untrusted, so that the user gets a chance to understand
+ the problem. Therefore, this raises an error rather than returning
+ a value.
+
+ :param context: Short user-readable description of the place
+ the problem occurred.
+ :raises IncomingEmailError: if the timestamp is stale or implausible,
+ containing a message based on the context and template.
+ """
fourty_eight_hours = 48 * 60 * 60
ten_minutes = 10 * 60
now = time.time()
=== modified file 'lib/canonical/launchpad/mail/tests/test_handlers.py'
--- lib/canonical/launchpad/mail/tests/test_handlers.py 2010-10-26 15:47:24 +0000
+++ lib/canonical/launchpad/mail/tests/test_handlers.py 2010-11-25 04:47:03 +0000
@@ -6,6 +6,7 @@
from doctest import DocTestSuite
import email
import time
+import transaction
import unittest
from canonical.database.sqlbase import commit
@@ -42,6 +43,66 @@
self.assertEqual('bug', commands[0].name)
self.assertEqual(['foo'], commands[0].string_args)
+ def test_NonGPGAuthenticatedNewBug(self):
+ """Mail authenticated other than by gpg can create bugs.
+
+ The incoming mail layer is responsible for authenticating the mail,
+ and setting the current principal to the sender of the mail, either
+ weakly or non-weakly authenticated. At the layer of the handler,
+ which this class is testing, we shouldn't care by what mechanism we
+ decided to act on behalf of the mail sender, only that we did.
+
+ In bug 643219, Launchpad had a problem where the MaloneHandler code
+ was puncturing that abstraction and directly looking at the GPG
+ signature; this test checks it's fixed.
+ """
+ # NB SignedMessage by default isn't actually signed, it just has the
+ # capability of knowing about signing.
+ message = self.factory.makeSignedMessage(body=' affects malone\nhi!')
+ self.assertEquals(message.signature, None)
+
+ # Pretend that the mail auth has given us a logged-in user.
+ handler = MaloneHandler()
+ with person_logged_in(self.factory.makePerson()):
+ mail_handled, add_comment_to_bug, commands = \
+ handler.extractAndAuthenticateCommands(message,
+ 'new@xxxxxxxxxxxxxxxxxx')
+ self.assertEquals(mail_handled, None)
+ self.assertEquals(map(str, commands), [
+ 'bug new',
+ 'affects malone',
+ ])
+
+ def test_mailToHelpFromUnknownUser(self):
+ """Mail from people of no account to help@ is simply dropped.
+ """
+ message = self.factory.makeSignedMessage()
+ handler = MaloneHandler()
+ mail_handled, add_comment_to_bug, commands = \
+ handler.extractAndAuthenticateCommands(message,
+ 'help@xxxxxxxxxxxxxxxxxx')
+ self.assertEquals(mail_handled, True)
+ self.assertEquals(self.getSentMail(), [])
+
+ def test_mailToHelp(self):
+ """Mail to help@ generates a help command."""
+ message = self.factory.makeSignedMessage()
+ handler = MaloneHandler()
+ with person_logged_in(self.factory.makePerson()):
+ mail_handled, add_comment_to_bug, commands = \
+ handler.extractAndAuthenticateCommands(message,
+ 'help@xxxxxxxxxxxxxxxxxx')
+ self.assertEquals(mail_handled, True)
+ self.assertEquals(len(self.getSentMail()), 1)
+ # TODO: Check the right mail was sent. -- mbp 20100923
+
+ def getSentMail(self):
+ # Sending mail is (unfortunately) a side effect of parsing the
+ # commands, and unfortunately you must commit the transaction to get
+ # them sent.
+ transaction.commit()
+ return stub.test_emails[:]
+
class FakeSignature:
=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt 2010-11-05 14:17:11 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2010-11-25 04:47:03 +0000
@@ -50,6 +50,8 @@
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:
+ >>> from lp.testing import sampledata
+
>>> import email.Message
>>> class MockSignedMessage(email.Message.Message):
... def __init__(self, *args, **kws):
@@ -77,14 +79,10 @@
... 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'],
- ... extract_signature_timestamp=fake_extract_signature_timestamp)
+ ... )
>>> process_email(submit_mail)
@@ -156,7 +154,7 @@
If we would file a bug on Ubuntu instead, we would submit a mail like
this:
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> submit_mail = """From: Sample Person <test@xxxxxxxxxxxxx>
... To: new@xxxxxxxxxxxxxxxxxx
... Date: Fri Jun 17 10:20:23 BST 2005
@@ -342,13 +340,15 @@
>>> from canonical.launchpad.interfaces.mail import IWeaklyAuthenticatedPrincipal
>>> from zope.interface import directlyProvides, directlyProvidedBy
>>> from zope.security.management import queryInteraction
- >>> participations = queryInteraction().participations
- >>> len(participations)
- 1
- >>> current_principal = participations[0].principal
- >>> directlyProvides(
- ... current_principal, directlyProvidedBy(current_principal),
- ... IWeaklyAuthenticatedPrincipal)
+
+ >>> def simulate_receiving_untrusted_mail():
+ ... participations = queryInteraction().participations
+ ... assert len(participations) == 1
+ ... current_principal = participations[0].principal
+ ... directlyProvides(
+ ... current_principal, directlyProvidedBy(current_principal),
+ ... IWeaklyAuthenticatedPrincipal)
+ >>> simulate_receiving_untrusted_mail()
Now we send a comment containing commands.
@@ -385,6 +385,8 @@
>>> def print_latest_email():
... commit()
+ ... if not stub.test_emails:
+ ... raise AssertionError("No emails queued!")
... from_addr, to_addrs, raw_message = stub.test_emails[-1]
... sent_msg = email.message_from_string(raw_message)
... error_mail, original_mail = sent_msg.get_payload()
@@ -411,7 +413,7 @@
... comment_mail, _class=MockUnsignedMessage)
>>> handler.process(
... msg, msg['To'],
- ... extract_signature_timestamp=fake_extract_signature_timestamp)
+ ... )
True
>>> commit()
@@ -457,12 +459,9 @@
>>> added_message in bug_one.messages
True
-Unmark the principal:
+In these tests, every time we log in, we're fully trusted again:
- >>> provided_interfaces = directlyProvidedBy(current_principal)
- >>> directlyProvides(
- ... current_principal,
- ... provided_interfaces - IWeaklyAuthenticatedPrincipal)
+ >>> login(sampledata.USER_EMAIL)
Commands
@@ -485,7 +484,7 @@
>>> def submit_command_email(msg):
... handler.process(
... msg, msg['To'],
- ... extract_signature_timestamp=fake_extract_signature_timestamp)
+ ... )
... commit()
... sync(bug)
@@ -734,7 +733,7 @@
>>> 'Foo Bar' in [subscription.person.displayname
... for subscription in bug_four.subscriptions]
False
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> submit_commands(bug_four, 'unsubscribe')
>>> 'Sample Person' in [subscription.person.displayname
... for subscription in bug_four.subscriptions]
@@ -793,9 +792,9 @@
... for subscriber in bug_five.getIndirectSubscribers()])
[u'Sample Person', u'Ubuntu Team']
-(Log back in as test@xxxxxxxxxxxxx for the tests that follow.)
+(Log back in for the tests that follow.)
- >>> login("test@xxxxxxxxxxxxx")
+ >>> login(sampledata.USER_EMAIL)
If we specify a non-existant user, an error message will be sent:
@@ -1108,7 +1107,7 @@
Attempting to set the milestone for a bug without sufficient
permissions also elicits an error message:
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> bug = new_firefox_bug()
>>> commit()
@@ -1144,13 +1143,13 @@
>>>
>>> login(ADMIN_EMAIL)
>>> sample_person = getUtility(IPersonSet).getByEmail(
- ... "test@xxxxxxxxxxxxx")
+ ... sampledata.USER_EMAIL)
>>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
>>> ubuntu = removeSecurityProxy(ubuntu)
>>> ubuntu.bug_supervisor = sample_person
>>> logout()
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
Like the web UI, we can assign a bug to nobody.
@@ -1246,10 +1245,10 @@
>>> login('foo.bar@xxxxxxxxxxxxx')
>>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
>>> ubuntu.driver = getUtility(IPersonSet).getByEmail(
- ... 'test@xxxxxxxxxxxxx')
+ ... sampledata.USER_EMAIL)
>>> commit()
>>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> sync(bug)
Now a new bugtask for the series will be create directly.
@@ -1302,7 +1301,7 @@
>>> ubuntu.driver = None
>>> commit()
>>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> bug = new_firefox_bug()
>>> for bugtask in bug.bugtasks:
@@ -1329,7 +1328,7 @@
... print driver.displayname
Sample Person
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> submit_commands(bug, 'affects /firefox/trunk')
>>> for bugtask in bug.bugtasks:
@@ -1367,7 +1366,7 @@
... print nomination.target.bugtargetdisplayname
Evolution trunk
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
Let's take on the upstream task on bug four as well. This time we'll
sneak in a 'subscribe' command between the 'affects' and the other
@@ -1642,7 +1641,7 @@
The user is a bug supervisors of the upstream product
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> bug_one = getUtility(IBugSet).get(1)
>>> submit_commands(
... bug_one, 'status confirmed', 'assignee test@xxxxxxxxxxxxx')
@@ -1746,6 +1745,7 @@
If none of the bug tasks can be chosen, an error message is sent to the
user, telling him that he has to use the 'affects' command.
+ >>> del stub.test_emails[:]
>>> login('stuart.bishop@xxxxxxxxxxxxx')
>>> submit_commands(
... bug_one, 'status new', 'assignee foo.bar@xxxxxxxxxxxxx')
@@ -1776,7 +1776,9 @@
him about the error. Let's start with trying to submit a bug without
signing the mail:
- >>> login('test@xxxxxxxxxxxxx')
+ >>> del stub.test_emails[:]
+ >>> login(sampledata.USER_EMAIL)
+ >>> simulate_receiving_untrusted_mail()
>>> from canonical.launchpad.mail import signed_message_from_string
>>> msg = signed_message_from_string(submit_mail)
@@ -1784,7 +1786,7 @@
>>> msg['Message-Id'] = email.Utils.make_msgid()
>>> handler.process(
... msg, msg['To'],
- ... extract_signature_timestamp=fake_extract_signature_timestamp)
+ ... )
True
>>> print_latest_email()
Subject: Submit Request Failure
@@ -1797,6 +1799,7 @@
A submit without specifying on what we want to file the bug on:
+ >>> login(sampledata.USER_EMAIL)
>>> submit_mail_no_bugtask = """From: test@xxxxxxxxxxxxx
... To: new@malone
... Date: Fri Jun 17 10:20:23 BST 2005
@@ -2003,7 +2006,7 @@
>>> from canonical.launchpad.mailnotification import (
... send_process_error_notification)
>>> send_process_error_notification(
- ... 'test@xxxxxxxxxxxxx', 'Some subject', 'Some error message.',
+ ... sampledata.USER_EMAIL, 'Some subject', 'Some error message.',
... msg, failing_command=['foo bar'])
The To and Subject headers got set to the values we provided:
@@ -2067,7 +2070,7 @@
First, we create a new firefox bug.
- >>> login('test@xxxxxxxxxxxxx')
+ >>> login(sampledata.USER_EMAIL)
>>> submit_mail = """From: Sample Person <test@xxxxxxxxxxxxx>
... To: new@xxxxxxxxxxxxxxxxxx
... Date: Fri Jun 17 10:20:23 BST 2006
=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py 2010-10-23 16:45:43 +0000
+++ lib/lp/services/mail/incoming.py 2010-11-25 04:47:03 +0000
@@ -37,6 +37,9 @@
from canonical.launchpad.interfaces.mailbox import IMailBox
from canonical.launchpad.mail.commands import get_error_message
from canonical.launchpad.mail.handlers import mail_handlers
+from canonical.launchpad.mail.helpers import (
+ ensure_sane_signature_timestamp,
+ )
from canonical.launchpad.mailnotification import (
send_process_error_notification,
)
@@ -61,6 +64,7 @@
# Match trailing whitespace.
trailing_whitespace = re.compile(r'[ \t]*((?=\r\n)|$)')
+
def canonicalise_line_endings(text):
r"""Canonicalise the line endings to '\r\n'.
@@ -159,14 +163,23 @@
return True
-def authenticateEmail(mail):
+def authenticateEmail(mail,
+ signature_timestamp_checker=None):
"""Authenticates an email by verifying the PGP signature.
The mail is expected to be an ISignedMessage.
+
+ If this completes, it will set the current security principal to be the
+ message sender.
+
+ :param signature_timestamp_checker: This callable is
+ passed the message signature timestamp, and it can raise an exception if
+ it dislikes it (for example as a replay attack.) This parameter is
+ intended for use in tests. If None, ensure_sane_signature_timestamp
+ is used.
"""
signature = mail.signature
- signed_content = mail.signedContent
name, email_addr = parseaddr(mail['From'])
authutil = getUtility(IPlacelessAuthUtility)
@@ -206,11 +219,19 @@
gpghandler = getUtility(IGPGHandler)
try:
sig = gpghandler.getVerifiedSignature(
- canonicalise_line_endings(signed_content), signature)
+ canonicalise_line_endings(mail.signedContent), signature)
except GPGVerificationError, e:
# verifySignature failed to verify the signature.
raise InvalidSignature("Signature couldn't be verified: %s" % str(e))
+ if signature_timestamp_checker is None:
+ signature_timestamp_checker = ensure_sane_signature_timestamp
+ # If this fails, we return an error to the user rather than just treating
+ # it as untrusted, so they can debug or understand the problem.
+ signature_timestamp_checker(
+ sig.timestamp,
+ 'incoming mail verification')
+
for gpgkey in person.gpg_keys:
if gpgkey.fingerprint == sig.fingerprint:
break
@@ -255,7 +276,8 @@
return request.oopsid
-def handleMail(trans=transaction):
+def handleMail(trans=transaction,
+ signature_timestamp_checker=None):
# First we define an error handler. We define it as a local
# function, to avoid having to pass a lot of parameters.
# pylint: disable-msg=W0631
@@ -358,7 +380,8 @@
continue
try:
- principal = authenticateEmail(mail)
+ principal = authenticateEmail(
+ mail, signature_timestamp_checker)
except InvalidSignature, error:
_handle_user_error(error, mail)
continue
=== modified file 'lib/lp/services/mail/tests/incomingmail.txt'
--- lib/lp/services/mail/tests/incomingmail.txt 2010-10-25 13:16:10 +0000
+++ lib/lp/services/mail/tests/incomingmail.txt 2010-11-25 04:47:03 +0000
@@ -62,6 +62,12 @@
... msg.replace_header('To', '123@%s' % domain)
... msgids[domain].append("<%s>" % sendmail(msg))
+handleMail will check the timestamp on signed messages, but the signatures
+on our testdata are old, and in these tests we don't care to be told.
+
+ >>> def accept_any_timestamp(timestamp, context_message):
+ ... pass
+
Since the User gets authenticated using OpenPGP signatures we have to
import the keys before handleMail is called.
@@ -72,6 +78,11 @@
>>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
>>> zopeless_transaction = LaunchpadZopelessLayer.txn
+ >>> handleMailForTest = lambda: handleMail(
+ ... zopeless_transaction,
+ ... signature_timestamp_checker=accept_any_timestamp)
+
+
We temporarily override the error mails' From address, so that they will
pass through the authentication stage:
@@ -87,7 +98,7 @@
discussed below; this output merely shows that we emit warnings when the
header is missing.)
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
@@ -139,7 +150,7 @@
>>> moin_change = read_test_message('moin-change.txt')
>>> moin_change['X-Launchpad-Original-To'] = '123@xxxxxxx'
>>> msgid = "<%s>" % sendmail(moin_change)
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid not in foo_handler.handledMails
True
@@ -154,7 +165,7 @@
>>> moin_change.replace_header('X-Launchpad-Original-To', '123@xxxxxxx')
>>> msgid = "<%s>" % sendmail(moin_change)
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid in bar_handler.handledMails
True
@@ -176,13 +187,13 @@
>>> msg = read_test_message('unsigned_inactive.txt')
>>> msgid = sendmail(msg, ['edit@malone-domain'])
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid not in foo_handler.handledMails
True
>>> msg = read_test_message('invalid_signed_inactive.txt')
>>> msgid = sendmail(msg, ['edit@malone-domain'])
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid not in foo_handler.handledMails
True
@@ -198,7 +209,7 @@
>>> msg['CC'] = '123@xxxxxxx'
>>> msg['X-Launchpad-Original-To'] = '123@xxxxxxx'
>>> msgid = '<%s>' % sendmail (msg, ['123@xxxxxxx'])
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid in bar_handler.handledMails
True
@@ -238,7 +249,7 @@
... doesn't matter
... """)
>>> msgid = sendmail(msg, ['edit@malone-domain'])
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
ERROR:canonical.launchpad.mail:An exception was raised inside the handler:
...
TestOopsException
@@ -286,7 +297,7 @@
... doesn't matter
... """)
>>> msgid = sendmail(msg, ['edit@malone-domain'])
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
ERROR:canonical.launchpad.mail:An exception was raised inside the handler:
...
Unauthorized
@@ -352,7 +363,7 @@
If we call handleMail(), we'll see some useful error messages printed
out:
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
ERROR:...:An exception was raised inside the handler: http://...
Traceback (most recent call last):
...
@@ -389,7 +400,7 @@
>>> len(stub.test_emails)
2
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
ERROR:...:Upload to Librarian failed...
...
UploadFailed: ...Connection refused...
@@ -416,7 +427,7 @@
>>> msg.replace_header('To', '123@xxxxxxx')
>>> msg['Return-Path'] = '<>'
>>> msgid = '<%s>' % sendmail(msg)
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid in foo_handler.handledMails
False
@@ -437,7 +448,7 @@
... 'multipart/report; report-type=delivery-status;'
... ' boundary="boundary"')
>>> msgid = '<%s>' % sendmail(msg)
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid in foo_handler.handledMails
False
@@ -452,7 +463,7 @@
>>> msg['Return-Path'] = '<not@xxxxxxxxx>'
>>> msg['Precedence'] = 'bulk'
>>> msgid = '<%s>' % sendmail(msg)
- >>> handleMail(zopeless_transaction)
+ >>> handleMailForTest()
>>> msgid in foo_handler.handledMails
False
=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py 2010-10-15 20:44:53 +0000
+++ lib/lp/services/mail/tests/test_incoming.py 2010-11-25 04:47:03 +0000
@@ -10,17 +10,23 @@
from zope.security.management import setSecurityPolicy
from canonical.config import config
+from canonical.launchpad.ftests import import_secret_test_key
from canonical.launchpad.mail.ftests.helpers import testmails_path
from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
+from canonical.launchpad.mail import (
+ authenticateEmail,
+ helpers,
+ )
+from canonical.testing.layers import LaunchpadZopelessLayer
from lp.services.mail.incoming import (
handleMail,
MailErrorUtility,
)
-from canonical.testing.layers import LaunchpadZopelessLayer
from lp.services.mail.sendmail import MailController
from lp.services.mail.stub import TestMailer
from lp.testing import TestCaseWithFactory
+from lp.testing.factory import GPGSigningContext
from lp.testing.mail_helpers import pop_notifications
@@ -81,6 +87,23 @@
else:
self.assertEqual(old_oops.id, current_oops.id)
+ def test_bad_signature_timestamp(self):
+ """If the signature is nontrivial future-dated, it's not trusted."""
+
+ signing_context = GPGSigningContext(
+ import_secret_test_key().fingerprint, password='test')
+ msg = self.factory.makeSignedMessage(signing_context=signing_context)
+ # It's not trivial to make a gpg signature with a bogus timestamp, so
+ # let's just treat everything as invalid, and trust that the regular
+ # implementation of extraction and checking of timestamps is correct,
+ # or at least tested.
+ def fail_all_timestamps(timestamp, context):
+ raise helpers.IncomingEmailError("fail!")
+ self.assertRaises(
+ helpers.IncomingEmailError,
+ authenticateEmail,
+ msg, fail_all_timestamps)
+
def setUp(test):
test._old_policy = setSecurityPolicy(LaunchpadSecurityPolicy)
=== modified file 'utilities/migrater/file-ownership.txt'
--- utilities/migrater/file-ownership.txt 2010-11-16 12:56:01 +0000
+++ utilities/migrater/file-ownership.txt 2010-11-25 04:47:03 +0000
@@ -1047,7 +1047,7 @@
./mail/errortemplates/num-arguments-mismatch.txt
./mail/errortemplates/no-such-bug.txt
./mail/errortemplates/security-parameter-mismatch.txt
- ./mail/errortemplates/not-gpg-signed.txt
+ ./mail/errortemplates/unauthenticated-bug-creation.txt
./mail/errortemplates/key-not-registered.txt
./mail/errortemplates/oops.txt
./mail/errortemplates/branchmergeproposal-exists.txt
Follow ups
