Merge lp:~mbp/launchpad/dkim into lp:launchpad

There is an upstream bug in python-dkim, https://bugs.edge.launchpad.net/ubuntu/+source/pydkim/+bug/587783

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

> 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)
>
> Thi...

bug 587783 now has what I think is a good clean patch, and a script to demonstrate it, and I've just sent it upstream.

There is also a source package branch there that can build a .deb of the fixed version or I guess that can be turned into an egg if prefered.

There was some list discussion of this <https://lists.launchpad.net/launchpad-dev/msg03533.html>

The point was also raised that we might want a whitelist of domains who are trusted to have meaningful DKIM headers, so I'll add that and do stub's tweaks.

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

done

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

fixed

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

fixed; there was a bug in python-dkim but I also need to expose the raw bytes of the message for DKIM validation, not the ISignedMessage which doesn't give the contents back byte-for-byte

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

fixed

I think the main things to do now are

 * add a config option for a list of trusted domains and test it
 * possibly get a final check-over from @elmo
 * get the dependency included -- if you want, you could start that now in parallel?
 * deploy to staging and test there?

A couple more things:

 * We need to check the From address matches the signature; at the moment we don't but I now have a failing test for it
 * python-dkim only checks the first signature so a multiply signed message may be seen as untrusted. I think this is fine for now.

lp:~stub/launchpad/dkim contains the changes for building the patched pydkim with buildout

<https://code.edge.launchpad.net/~mbp/ubuntu/lucid/pydkim/details> gives pydkim the ability to return details about the signatures that it verified.

Sorry, could you please re-try with the import restored?

the equivalent was merged by stub