launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

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,
+            'foo.bar@xxxxxxxxxxxxx')
+        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.'] = \