Merge lp:~mbp/launchpad/881237-dkim-error into lp:launchpad

[Martin Pool]

[r=wgrant][bug=881237] Unexpected errors from dkim.verify log a warning but don't drop the mail

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.

[William Grant (wgrant)]

review: Approve (code)

=== 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,
+            '[e-mail]')
+        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.'] = \