launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #02004
Re: [Merge] lp:~mbp/launchpad/dkim into lp:launchpad
Thanks for all your patience and help. The updated branch now passes at least the two doctests that were identified as failing. The specific change is that I've gone back to the approach of passing in the timestamp-checking callback, but moved it to a more appropriate spot that (I think) covers all cases. This is imo cleaner than monkey patching, as I was doing.
I'm thinking of also retargeting this to devel. It has no database dependencies.
=== 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-24 06:49:53 +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
=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py 2010-11-19 04:17:09 +0000
+++ lib/canonical/launchpad/mail/helpers.py 2010-11-24 06:49:53 +0000
@@ -241,7 +241,15 @@
error_template='old-signature.txt'):
"""Ensure the signature was generated recently but not in the future.
- :raises IncomingEmailError: if the timestamp is stale or implausible.
+ 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
=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py 2010-11-19 04:17:09 +0000
+++ lib/lp/services/mail/incoming.py 2010-11-24 06:49:53 +0000
@@ -64,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'.
@@ -162,13 +163,20 @@
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
@@ -216,7 +224,11 @@
# verifySignature failed to verify the signature.
raise InvalidSignature("Signature couldn't be verified: %s" % str(e))
- ensure_sane_signature_timestamp(
+ 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')
@@ -264,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
@@ -367,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-24 06:49:53 +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-11-19 04:47:05 +0000
+++ lib/lp/services/mail/tests/test_incoming.py 2010-11-24 06:49:53 +0000
@@ -11,16 +11,12 @@
from canonical.config import config
from canonical.launchpad.ftests import import_secret_test_key
-from canonical.launchpad.interfaces.mail import (
- IWeaklyAuthenticatedPrincipal,
- )
from canonical.launchpad.mail.ftests.helpers import testmails_path
from canonical.launchpad.mail import (
helpers,
)
from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
-from canonical.launchpad.webapp.interaction import get_current_principal
from canonical.testing.layers import LaunchpadZopelessLayer
from lp.services.mail.incoming import (
authenticateEmail,
@@ -97,25 +93,16 @@
signing_context = GPGSigningContext(
import_secret_test_key().fingerprint, password='test')
msg = self.factory.makeSignedMessage(signing_context=signing_context)
- # it's not easy (for me) to make a gpg signature with a bogus
- # timestamp, so instead we'll just make sure that it is in fact
- # checked
- self._hook_timestamp_check()
- authenticateEmail(msg)
- self.assertTrue(
- IWeaklyAuthenticatedPrincipal.providedBy(get_current_principal()))
-
- def _hook_timestamp_check(self):
- saved = helpers.ensure_sane_signature_timestamp
-
- def restore():
- helpers.ensure_sane_signature_timestamp = saved
-
- def fail(timestamp, 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.addCleanup(restore)
- helpers.ensure_sane_signature_timestamp = fail
+ self.assertRaises(
+ helpers.IncomingEmailError,
+ authenticateEmail,
+ msg, fail_all_timestamps)
def setUp(test):
--
https://code.launchpad.net/~mbp/launchpad/dkim/+merge/35985
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/dkim into lp:launchpad.
References
