Code review comment for lp:~mbp/launchpad/dkim

Thanks for this. I'll certainly appreciate this feature :-)

=== modified file 'lib/canonical/launchpad/mail/incoming.py'

+def _authenticateDkim(mail):
+ """"Attempt DKIM authentication of email; return True if known authentic"""
+
+ try:
+ dkim_result = dkim.verify(mail.as_string(), dkim_log)
+ except dkim.DKIMException, e:
+ log.warning('DKIM error: %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

So this is failing DKIM authentication on DNS glitches, and DKIMException (invalid signature, messed up DNS etc.). I think users need to be informed of this. A the moment, they will just get the standard 'unsigned email' response with no hint what went wrong.

This might be too much refactoring for now, so perhaps a bug is in order and we can worry about it if it is a genuine problem. I think we would have to annotate the 'mail' object and have a higher level report failures to the user if strong authentication is required but not available.

+ if mail.signature:
+ log.info('message has gpg signature, therefore not treating DKIM as conclusive')
+ return False

I think this logic is better left to the callsite. Just return success/fail and leave it up to authenticateEmail what signature types should take precidence.

=== added file 'lib/lp/services/mail/tests/test_dkim.py'

+# reuse the handler that records log events
+from lp.services.sshserver.tests.test_events import ListHandler

You don't seem to be using this. If you do, ideally this should be moved somewhere like lp.testing.

+ def monkeypatch_dns(self):
+ self._dns_responses = {}
+ def my_lookup(name):
+ try:
+ return self._dns_responses[name]
+ except KeyError:
+ raise dns.resolver.NXDOMAIN()
+ orig_dnstxt = dkim.dnstxt
+ dkim.dnstxt = my_lookup
+ def restore():
+ dkim.dnstxt = orig_dnstxt
+ self.addCleanup(restore)

This might have been nicer using Mocker (http://labix.org/mocker), which is available as a dependency. I wouldn't worry about changing now though - this is fine and understandable.

+ def test_dkim_valid_strict(self):
+ # FIXME: this fails because some re-wrapping happens after the
+ # message comes in (probably going too/from SignedMessage)
+ # and pydkim correctly complains about that: we would need to
+ # validate against the raw bytes
+ return
+ signed_message = self.fake_signing(plain_content,
+ canonicalize=(dkim.Simple, dkim.Simple))
+ self._dns_responses['example._domainkey.canonical.com.'] = \
+ sample_dns
+ principal = authenticateEmail(signed_message_from_string(signed_message))
+ self.assertStronglyAuthenticated(principal, signed_message)

This test is still disabled. Does this need to be fixed before landing, as we would see this situation in real email too? If not, at a minimum the comment needs to change to an XXX referencing a Launchpad bug.

+ def test_dkim_body_mismatch(self):
+ # The message message has a syntactically valid DKIM signature that
+ # doesn't actually correspond to the sender. We log something about
+ # this but we don't want to drop the message.
+ #
+ # XXX: This test relies on having real DNS service to look up the
+ # signing key.

Is the XXX comment still valid now you are monkey patching the DNS responses? We might have to fix this before landing - I don't know if this is an allowed dependency in our tests.

=== modified file 'setup.py'
--- setup.py 2010-03-24 05:28:44 +0000
+++ setup.py 2010-05-28 10:18:36 +0000
@@ -29,6 +29,7 @@
         'chameleon.core',
         'chameleon.zpt',
         'cssutils',
+ # 'dkim',
         'feedvalidator',
         'funkload',
         'launchpadlib',

Yes, we should package an egg of the patched dkim and the dns libraries and use buildout. I can sort this before landing if you like.