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

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

Thanks for the review.

>
> === 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.

I think for now we shouldn't send any mail on DKIM failures, because that would be a likely way for this to go wrong by spamming people. Let's get some experience on real data first by monitoring how many fail and why.

There are a few possibilities:

* message is gpg signed but has broken dkim
* message has a broken signature but it's ok for it to be low-trusted anyhow
* message left the user ok but was broken somewhere in transit
* message was broken in our DC on the way to the incoming queue (we don't know this won't happen)
* message failed to verify because of a transient dns error

If the user expects their messages to be trusted and they aren't, there should be a way for them to find out why. Conversely if they don't care about DKIM (and often it won't be under the user's control to fix) we shouldn't bother them about it. For the moment I think we should let them ask a question in the first case.

>
>
> + 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.

ok

>
> === 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.

ok

>
>
> + 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.

Yes, this needs to be fixed before landing; it's due in part to bug 587783.

> + 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.

It's out of date.
>
> === 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.

Thanks, I'll ping you when it's ready.