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

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