launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/dkimpy into lp:launchpad.

Commit message:
Upgrade from pydkim 0.3 to dkimpy 0.5.4. dkimpy is a maintained fork.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/dkimpy/+merge/197637

Upgrade from pydkim 0.3 to dkimpy 0.5.4. dkimpy is a maintained fork of pydkim, including the patches we had on top of pydkim and various other improvements.
-- 
https://code.launchpad.net/~wgrant/launchpad/dkimpy/+merge/197637
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/dkimpy into lp:launchpad.

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2013-08-12 03:12:44 +0000
+++ lib/lp/services/mail/incoming.py	2013-12-04 03:27:10 +0000
@@ -5,7 +5,6 @@
 
 __metaclass__ = type
 
-from cStringIO import StringIO as cStringIO
 import email.errors
 from email.utils import (
     getaddresses,
@@ -36,6 +35,7 @@
     IEmailAddressSet,
     )
 from lp.services.librarian.interfaces.client import UploadFailed
+from lp.services.log.logger import BufferLogger
 from lp.services.mail.handlers import mail_handlers
 from lp.services.mail.helpers import (
     ensure_sane_signature_timestamp,
@@ -129,17 +129,17 @@
     # uncomment this for easier test debugging
     # log.addHandler(logging.FileHandler('/tmp/dkim.log'))
 
-    dkim_log = cStringIO()
+    dkim_log = BufferLogger()
     log.info(
         'Attempting DKIM authentication of message id=%r from=%r sender=%r'
         % (signed_message['Message-ID'],
            signed_message['From'],
            signed_message['Sender']))
-    signing_details = []
+    dkim_checker = None
     dkim_result = False
     try:
-        dkim_result = dkim.verify(
-            signed_message.parsed_string, dkim_log, details=signing_details)
+        dkim_checker = dkim.DKIM(signed_message.parsed_string, logger=dkim_log)
+        dkim_result = dkim_checker.verify()
     except dkim.DKIMException as e:
         log.warning('DKIM error: %r' % (e,))
     except dns.resolver.NXDOMAIN as e:
@@ -160,17 +160,12 @@
             '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(),))
+    log.debug('DKIM debug log: %s' % (dkim_log.getLogBuffer(),))
     if not dkim_result:
         return None
     # in addition to the dkim signature being valid, we have to check that it
     # was actually signed by the user's domain.
-    if len(signing_details) != 1:
-        log.info(
-            'expected exactly one DKIM details record: %r'
-            % (signing_details,))
-        return None
-    signing_domain = signing_details[0]['d']
+    signing_domain = dkim_checker.domain
     if not _isDkimDomainTrusted(signing_domain):
         log.info("valid DKIM signature from untrusted domain %s"
             % (signing_domain,))

=== modified file 'lib/lp/services/mail/tests/test_dkim.py'
--- lib/lp/services/mail/tests/test_dkim.py	2012-04-13 03:25:30 +0000
+++ lib/lp/services/mail/tests/test_dkim.py	2013-12-04 03:27:10 +0000
@@ -9,7 +9,7 @@
 from StringIO import StringIO
 
 import dkim
-import dns.resolver
+import dkim.dnsplug
 
 from lp.services.features.testing import FeatureFixture
 from lp.services.identity.interfaces.account import AccountStatus
@@ -28,27 +28,37 @@
 # rsa -pubout'.  Not really the key for canonical.com ;-)
 sample_privkey = """\
 -----BEGIN RSA PRIVATE KEY-----
-MIIBOwIBAAJBANmBe10IgY+u7h3enWTukkqtUD5PR52Tb/mPfjC0QJTocVBq6Za/
-PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQJAYFUKsD+uMlcFu1D3YNaR
-EGYGXjJ6w32jYGJ/P072M3yWOq2S1dvDthI3nRT8MFjZ1wHDAYHrSpfDNJ3v2fvZ
-cQIhAPgRPmVYn+TGd59asiqG1SZqh+p+CRYHW7B8BsicG5t3AiEA4HYNOohlgWan
-8tKgqLJgUdPFbaHZO1nDyBgvV8hvWZUCIQDDdCq6hYKuKeYUy8w3j7cgJq3ih922
-2qNWwdJCfCWQbwIgTY0cBvQnNe0067WQIpj2pG7pkHZR6qqZ9SE+AjNTHX0CIQCI
-Mgq55Y9MCq5wqzy141rnxrJxTwK9ABo3IAFMWEov3g==
+MIICWwIBAAKBgQC7ozYozzZuLYQi2DXSMtI3wWzWd7tAJfg+zwbOcNS4Aib6lo3R
+y6ansi+fOhHSwgeOrkBGKzgHi2T8iDPzpUFhAZuOFsQaVY6yHzhXwPFi/nKYtFxU
+X0DE4/GxkmNDgBOPqIpyEUQJvf5+byvb5mI85AS09em90EQGpf/a/O1Y0QIDAQAB
+AoGABTPEN6NvFeTrKfAmpdpE28jgFJ4jMecbl9ozjRuxuhxNKltsOSnVSAb3rQl2
+HwrEHN+V5pwiJItn1FyOXC3zvwmeKfX1J290+dleI4kNfXf97eYtJyOVVdRcreNA
+7qUROmFgN8sLOWsPgvYq3IRf+QxBXD/BPVEhuHBbCJBFq8UCQQDcJ6oDk2D2+8W2
+5f+3nrTBIrFtQBlApJF6q26zTOmwvOM6wrx+9wa4gAcVD8dbH2eklekTfLE3k7cU
+r1WHzN5HAkEA2jAuctbruPfmTP2KctjgeEokaOq7ym5aVwcbSlTHtJZLIOjvlPmu
+BhFFqZj0Yy3H1LhpYpGHrvG+uGp07h6kJwJAcIqeMKHAacGe+rZsmJM615hClxSz
+VAZMkCbeui3RMJX+muU9srHY76wS8sNUJ9LQCqTPtzSA62ZJqvtOf9NMtQJAZgVp
+cqE0D4U61n0nI5RtQVHJvJUlwf3fmBnmlNcXmkU8U+MXQ52L1aJ15Ft0yns5mSmx
+fTl3LEI1X53HlyAUuQJALHmpSB2Qq0DqmgcAGXkDsh3GRfMv91a7u99VDT/fe+J0
+2KXtp+K0MdWdAe/83icQ/WlYKIDz0Asm5FZcsTNapw==
 -----END RSA PRIVATE KEY-----
 """
 
 sample_pubkey = """\
 -----BEGIN PUBLIC KEY-----
-MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANmBe10IgY+u7h3enWTukkqtUD5PR52T
-b/mPfjC0QJTocVBq6Za/PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQ==
+MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC7ozYozzZuLYQi2DXSMtI3wWzW
+d7tAJfg+zwbOcNS4Aib6lo3Ry6ansi+fOhHSwgeOrkBGKzgHi2T8iDPzpUFhAZuO
+FsQaVY6yHzhXwPFi/nKYtFxUX0DE4/GxkmNDgBOPqIpyEUQJvf5+byvb5mI85AS0
+9em90EQGpf/a/O1Y0QIDAQAB
 -----END PUBLIC KEY-----
 """
 
 sample_dns = """\
 k=rsa; \
-p=MFwwDQYJKoZIhvcNAQEBBQADSwAwSAJBANmBe10IgY+u7h3enWTukkqtUD5PR52T\
-b/mPfjC0QJTocVBq6Za/PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQ=="""
+p=MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC7ozYozzZuLYQi2DXSMtI3wWzW\
+d7tAJfg+zwbOcNS4Aib6lo3Ry6ansi+fOhHSwgeOrkBGKzgHi2T8iDPzpUFhAZuO\
+FsQaVY6yHzhXwPFi/nKYtFxUX0DE4/GxkmNDgBOPqIpyEUQJvf5+byvb5mI85AS0\
+9em90EQGpf/a/O1Y0QIDAQAB"""
 
 
 class TestDKIM(TestCaseWithFactory):
@@ -61,9 +71,9 @@
         TestCaseWithFactory.setUp(self, 'admin@xxxxxxxxxxxxx')
         self._log_output = StringIO()
         handler = logging.StreamHandler(self._log_output)
-        logger = logging.getLogger('mail-authenticate-dkim')
-        logger.addHandler(handler)
-        self.addCleanup(lambda: logger.removeHandler(handler))
+        self.logger = logging.getLogger('mail-authenticate-dkim')
+        self.logger.addHandler(handler)
+        self.addCleanup(lambda: self.logger.removeHandler(handler))
         self.monkeypatch_dns()
 
     def fake_signing(self, plain_message, canonicalize=None):
@@ -73,7 +83,7 @@
             selector='example',
             domain='canonical.com',
             privkey=sample_privkey,
-            debuglog=self._log_output,
+            logger=self.logger,
             canonicalize=canonicalize)
         assert dkim_line[-1] == '\n'
         return dkim_line + plain_message
@@ -85,13 +95,13 @@
             try:
                 return self._dns_responses[name]
             except KeyError:
-                raise dns.resolver.NXDOMAIN()
+                return None
 
-        orig_dnstxt = dkim.dnstxt
-        dkim.dnstxt = my_lookup
+        orig_get_txt = dkim.dnsplug._get_txt
+        dkim.dnsplug._get_txt = my_lookup
 
         def restore():
-            dkim.dnstxt = orig_dnstxt
+            dkim.dnsplug._get_txt = orig_get_txt
 
         self.addCleanup(restore)
 
@@ -111,7 +121,7 @@
             key = 'abcdefg'
         else:
             raise ValueError(response_type)
-        self._dns_responses['example._domainkey.canonical.com.'] = key
+        self._dns_responses[u'example._domainkey.canonical.com.'] = key
 
     def get_dkim_log(self):
         return self._log_output.getvalue()
@@ -159,7 +169,8 @@
         self.assertWeaklyAuthenticated(principal, signed_message)
         self.assertEqual(principal.person.preferredemail.email,
             'foo.bar@xxxxxxxxxxxxx')
-        self.assertDkimLogContains('unexpected error in DKIM verification')
+        self.assertDkimLogContains(
+            "DKIM error: KeyFormatError('incomplete public key:")
 
     def test_dkim_garbage_pubkey(self):
         signed_message = self.fake_signing(self.makeMessageText())
@@ -169,7 +180,7 @@
         self.assertWeaklyAuthenticated(principal, signed_message)
         self.assertEqual(principal.person.preferredemail.email,
             'foo.bar@xxxxxxxxxxxxx')
-        self.assertDkimLogContains('invalid format in _domainkey txt record')
+        self.assertDkimLogContains("DKIM error: KeyFormatError(InvalidTagSpec")
 
     def test_dkim_disabled(self):
         """With disabling flag set, mail isn't trusted."""

=== modified file 'setup.py'
--- setup.py	2013-09-09 08:53:37 +0000
+++ setup.py	2013-12-04 03:27:10 +0000
@@ -32,7 +32,8 @@
         'BeautifulSoup',
         'bzr',
         'cssutils',
-        # Required for pydkim
+        'dkimpy',
+        # Required for dkimpy
         'dnspython',
         'fixtures',
         'FeedParser',
@@ -71,7 +72,6 @@
         'psycopg2',
         'python-memcached',
         'pyasn1',
-        'pydkim',
         'pystache',
         'python-openid',
         'pytz',

=== modified file 'versions.cfg'
--- versions.cfg	2013-09-09 10:40:35 +0000
+++ versions.cfg	2013-12-04 03:27:10 +0000
@@ -22,7 +22,8 @@
 cssutils = 0.9.10
 d2to1 = 0.2.10
 Django = 1.4
-# Required by pydkim
+dkimpy = 0.5.4
+# Required by dkimpy
 dnspython = 1.10.0
 elementtree = 1.2.6-20050316
 epydoc = 3.0.1
@@ -76,7 +77,6 @@
 psycopg2 = 2.4.6
 pyasn1 = 0.1.6
 pycrypto = 2.6
-pydkim = 0.3-mbp-r7
 Pygments = 1.6
 pygpgme = 0.2
 pyinotify = 0.9.4