Martin Pool has proposed merging lp:~mbp/launchpad/881237-dkim-error into
lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #881237 in Launchpad itself: "broken dkim key on qq.com causes mail to be
dropped"
https://bugs.launchpad.net/launchpad/+bug/881237
For more details, see:
https://code.launchpad.net/~mbp/launchpad/881237-dkim-error/+merge/80413
pydkim can sometimes raise eg a KeyError when given badly formatted data from
the network.
This puts a firewall in incoming.py so that those errors are treated as an
operational warning, but they don't cause the mail to be dropped.
We should also probably land an update to pydkim that will handle these things
more gracefully within itself.
--
https://code.launchpad.net/~mbp/launchpad/881237-dkim-error/+merge/80413
Your team Launchpad code reviewers is requested to review the proposed merge of
lp:~mbp/launchpad/881237-dkim-error into lp:launchpad.
=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py 2011-10-25 05:10:28 +0000
+++ lib/lp/services/mail/incoming.py 2011-10-26 02:36:24 +0000
@@ -131,26 +131,30 @@
signed_message['From'],
signed_message['Sender']))
signing_details = []
+ dkim_result = False
try:
- # NB: if this fails with a keyword argument error, you need the
- # python-dkim 0.3-3.2 that adds it
dkim_result = dkim.verify(
signed_message.parsed_string, dkim_log, details=signing_details)
except dkim.DKIMException, e:
log.warning('DKIM error: %r' % (e,))
- dkim_result = False
except dns.resolver.NXDOMAIN, e:
# This can easily happen just through bad input data, ie claiming to
# be signed by a domain with no visible key of that name. It's not an
# operational error.
log.info('DNS exception: %r' % (e,))
- dkim_result = False
except dns.exception.DNSException, e:
# many of them have lame messages, thus %r
log.warning('DNS exception: %r' % (e,))
- dkim_result = False
- else:
- log.info('DKIM verification result=%s' % (dkim_result,))
+ except Exception, e:
+ # DKIM leaks some errors when it gets bad input, as in bug 881237. We
+ # don't generally want them to cause the mail to be dropped entirely
+ # though. It probably is reasonable to treat them as potential
+ # operational errors, at least until they're handled properly, by
+ # making pydkim itself more defensive.
+ log.warning(
+ 'unexpected error in DKIM verification, treating as unsigned: %r'
+ % (e,))
+ log.info('DKIM verification result: trusted=%s' % (dkim_result,))
log.debug('DKIM debug log: %s' % (dkim_log.getvalue(),))
if not dkim_result:
return None
=== modified file 'lib/lp/services/mail/tests/test_dkim.py'
--- lib/lp/services/mail/tests/test_dkim.py 2011-09-26 06:30:07 +0000
+++ lib/lp/services/mail/tests/test_dkim.py 2011-10-26 02:36:24 +0000
@@ -119,6 +119,22 @@
if l.find(substring) == -1:
self.fail("didn't find %r in log: %s" % (substring, l))
+ def test_dkim_broken_pubkey(self):
+ """Handle a subtly-broken pubkey like qq.com, see bug 881237.
+
+ The message is not trusted but inbound message processing does not
+ abort either.
+ """
+ signed_message = self.fake_signing(plain_content)
+ self._dns_responses['example._domainkey.canonical.com.'] = \
+ sample_dns.replace(';', '')
+ principal = authenticateEmail(
+ signed_message_from_string(signed_message))
+ self.assertWeaklyAuthenticated(principal, signed_message)
+ self.assertEqual(principal.person.preferredemail.email,
+ '[email protected]')
+ self.assertDkimLogContains('unexpected error in DKIM verification')
+
def test_dkim_garbage_pubkey(self):
signed_message = self.fake_signing(plain_content)
self._dns_responses['example._domainkey.canonical.com.'] = \
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
