Merge lp:~mbp/launchpad/925597-dkim-from into lp:launchpad

[Martin Pool]

cope with mail dkim-signed by addresses that aren't know to launchpad

This fixes bug <https://bugs.launchpad.net/launchpad/+bug/925597> which is hit if you are sending mail from gmail, but your gmail address is not registered with lp.

I wonder if I also need to test the case that your gmail address is known but not verified? I probably should.

[Brad Crittenden (bac)]

Thanks for the fix Martin. Your changes are clear and the new test case is quite readable.

'make lint' shows a couple of valid items along with the usual suspects of non-issues. If you have a second to address those it would be nice.

Again, thanks for this fix and for championing the DKIM support.

review: Approve (code)

[Martin Pool (mbp)]

Thanks for the prompt friendly review!

I think it will be worth adding the other test case, so I'll try to do that
today or tomorrow, then get an incremental review and merge it myself.

[Martin Pool (mbp)]

I cleaned it up a bit more, and added a test for mail signed by unverified addresses, which was a case that needed to be handled.

[Ian Booth (wallyworld)]

Looks great. Thanks for making the changes suggested on irc.

review: Approve

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2012-01-04 03:23:19 +0000
+++ lib/lp/services/mail/incoming.py	2012-04-13 03:28:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functions dealing with mails coming into Launchpad."""
@@ -33,6 +33,10 @@
     IGPGHandler,
     )
 from lp.services.identity.interfaces.account import AccountStatus
+from lp.services.identity.interfaces.emailaddress import (
+    EmailAddressStatus,
+    IEmailAddressSet,
+    )
 from lp.services.librarian.interfaces.client import UploadFailed
 from lp.services.mail.handlers import mail_handlers
 from lp.services.mail.helpers import (
@@ -103,8 +107,8 @@
     return domain in _trusted_dkim_domains
 
 
-def _authenticateDkim(signed_message):
-    """Attempt DKIM authentication of email.
+def _verifyDkimOrigin(signed_message):
+    """Find a From or Sender address for which there's a DKIM signature.
 
     :returns: A string email address for the trusted sender, if there is one,
     otherwise None.
@@ -190,6 +194,46 @@
         return None
 
 
+def _getPrincipalByDkim(mail):
+    """Determine the security principal from DKIM, if possible.
+
+    To qualify:
+        * there must be a dkim signature from a trusted domain
+        * the From or Sender must be in that domain
+        * the address in this header must be verified for a person
+
+    :returns: (None, None), or (principal, trusted_addr).
+    """
+    log = logging.getLogger('mail-authenticate-dkim')
+    authutil = getUtility(IPlacelessAuthUtility)
+
+    dkim_trusted_address = _verifyDkimOrigin(mail)
+    if dkim_trusted_address is None:
+        return None, None
+
+    log.debug('authenticated DKIM mail origin %s' % dkim_trusted_address)
+    address = getUtility(IEmailAddressSet).getByEmail(dkim_trusted_address)
+    if address is None:
+        log.debug("valid dkim signature, but not from a known email address, "
+            "therefore disregarding it")
+        return None, None
+    elif address.status not in (EmailAddressStatus.VALIDATED,
+            EmailAddressStatus.PREFERRED):
+        log.debug("valid dkim signature, "
+            "but not from an active email address, "
+            "therefore disregarding it")
+        return None, None
+    if address.person is None:
+        log.debug("address is not associated with a person")
+        return None, None
+    account = address.person.account
+    if account is None:
+        log.debug("person does not have an account")
+        return None, None
+    dkim_principal = authutil.getPrincipal(account.id)
+    return (dkim_principal, dkim_trusted_address)
+
+
 def authenticateEmail(mail,
     signature_timestamp_checker=None):
     """Authenticates an email by verifying the PGP signature.
@@ -207,20 +251,13 @@
     """
 
     log = logging.getLogger('process-mail')
-
-    dkim_trusted_addr = _authenticateDkim(mail)
-    if dkim_trusted_addr is not None:
-        # The Sender field, if signed by a trusted domain, is the strong
-        # authenticator for this mail.
-        log.debug('trusted DKIM mail from %s' % dkim_trusted_addr)
-        email_addr = dkim_trusted_addr
-    else:
-        email_addr = parseaddr(mail['From'])[1]
-
     authutil = getUtility(IPlacelessAuthUtility)
-    principal = authutil.getPrincipalByLogin(email_addr)
-
-    # Check that sender is registered in Launchpad and the email is signed.
+
+    principal, dkim_trusted_address = _getPrincipalByDkim(mail)
+    if dkim_trusted_address is None:
+        from_addr = parseaddr(mail['From'])[1]
+        principal = authutil.getPrincipalByLogin(from_addr)
+
     if principal is None:
         setupInteraction(authutil.unauthenticatedPrincipal())
         return None
@@ -230,9 +267,9 @@
         raise InactiveAccount(
             "Mail from a user with an inactive account.")
 
-    if dkim_trusted_addr is not None:
+    if dkim_trusted_address:
         log.debug('accepting dkim strongly authenticated mail')
-        setupInteraction(principal, dkim_trusted_addr)
+        setupInteraction(principal, dkim_trusted_address)
         return principal
     else:
         log.debug("attempt gpg authentication for %r" % person)

=== modified file 'lib/lp/services/mail/tests/test_dkim.py'
--- lib/lp/services/mail/tests/test_dkim.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/mail/tests/test_dkim.py	2012-04-13 03:28:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test DKIM-signed messages"""
@@ -12,8 +12,13 @@
 import dns.resolver
 
 from lp.services.features.testing import FeatureFixture
+from lp.services.identity.interfaces.account import AccountStatus
+from lp.services.identity.interfaces.emailaddress import EmailAddressStatus
 from lp.services.mail import incoming
-from lp.services.mail.incoming import authenticateEmail
+from lp.services.mail.incoming import (
+    authenticateEmail,
+    InactiveAccount,
+    )
 from lp.services.mail.interfaces import IWeaklyAuthenticatedPrincipal
 from lp.services.mail.signedmessage import signed_message_from_string
 from lp.testing import TestCaseWithFactory
@@ -46,17 +51,6 @@
 b/mPfjC0QJTocVBq6Za/PlzfV+Py92VaCak19F4WrbVTK5Gg5tW220MCAwEAAQ=="""
 
 
-plain_content = """\
-From: Foo Bar <[e-mail]>
-Date: Fri, 1 Apr 2010 00:00:00 +1000
-Subject: yet another comment
-To: [e-mail]
-
-  importance critical
-
-Why isn't this fixed yet?"""
-
-
 class TestDKIM(TestCaseWithFactory):
     """Messages can be strongly authenticated by DKIM."""
 
@@ -101,6 +95,24 @@
 
         self.addCleanup(restore)
 
+    def preload_dns_response(self, response_type='valid'):
+        """Configure a fake DNS key response.
+
+        :param response_type: Describes what response to give back as the
+        key.  The default, 'valid', is to give the valid test signing key.
+        'broken' gives a key that's almost but not quite valid, 'garbage'
+        gives one that doesn't look valid at all.
+        """
+        if response_type == 'valid':
+            key = sample_dns
+        elif response_type == 'broken':
+            key = sample_dns.replace(';', '')
+        elif response_type == 'garbage':
+            key = 'abcdefg'
+        else:
+            raise ValueError(response_type)
+        self._dns_responses['example._domainkey.canonical.com.'] = key
+
     def get_dkim_log(self):
         return self._log_output.getvalue()
 
@@ -119,15 +131,29 @@
         if l.find(substring) == -1:
             self.fail("didn't find %r in log: %s" % (substring, l))
 
+    def makeMessageText(self, sender=None, from_address=None):
+        if from_address is None:
+            from_address = "Foo Bar <[e-mail]>"
+        text = ("From: " + from_address + "\n" + """\
+Date: Fri, 1 Apr 2010 00:00:00 +1000
+Subject: yet another comment
+To: [e-mail]
+
+  importance critical
+
+Why isn't this fixed yet?""")
+        if sender is not None:
+            text = "Sender: " + sender + "\n" + text
+        return text
+
     def test_dkim_broken_pubkey(self):
         """Handle a subtly-broken pubkey like qq.com, see bug 881237.
 
         The message is not trusted but inbound message processing does not
         abort either.
         """
-        signed_message = self.fake_signing(plain_content)
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns.replace(';', '')
+        signed_message = self.fake_signing(self.makeMessageText())
+        self.preload_dns_response('broken')
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertWeaklyAuthenticated(principal, signed_message)
@@ -136,9 +162,8 @@
         self.assertDkimLogContains('unexpected error in DKIM verification')
 
     def test_dkim_garbage_pubkey(self):
-        signed_message = self.fake_signing(plain_content)
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            'aothuaonu'
+        signed_message = self.fake_signing(self.makeMessageText())
+        self.preload_dns_response('garbage')
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertWeaklyAuthenticated(principal, signed_message)
@@ -155,10 +180,9 @@
             self.test_dkim_valid_strict)
 
     def test_dkim_valid_strict(self):
-        signed_message = self.fake_signing(plain_content,
+        signed_message = self.fake_signing(self.makeMessageText(),
             canonicalize=(dkim.Simple, dkim.Simple))
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns
+        self.preload_dns_response()
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertStronglyAuthenticated(principal, signed_message)
@@ -166,9 +190,8 @@
             '[e-mail]')
 
     def test_dkim_valid(self):
-        signed_message = self.fake_signing(plain_content)
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns
+        signed_message = self.fake_signing(self.makeMessageText())
+        self.preload_dns_response()
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertStronglyAuthenticated(principal, signed_message)
@@ -177,9 +200,8 @@
 
     def test_dkim_untrusted_signer(self):
         # Valid signature from an untrusted domain -> untrusted
-        signed_message = self.fake_signing(plain_content)
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns
+        signed_message = self.fake_signing(self.makeMessageText())
+        self.preload_dns_response()
         saved_domains = incoming._trusted_dkim_domains[:]
 
         def restore():
@@ -198,11 +220,10 @@
         # that of the From-sender, if that domain is relaying the message.
         # However, we shouldn't then trust the purported sender, because they
         # might have just made it up rather than relayed it.
-        tweaked_message = plain_content.replace('[e-mail]',
-            '[e-mail]')
+        tweaked_message = self.makeMessageText(
+            from_address='[e-mail]')
         signed_message = self.fake_signing(tweaked_message)
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns
+        self.preload_dns_response()
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertWeaklyAuthenticated(principal, signed_message)
@@ -215,9 +236,8 @@
         #  We still treat this as weakly authenticated by the purported
         # From-header sender, though perhaps in future we would prefer
         # to reject these messages.
-        signed_message = self.fake_signing(plain_content)
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns
+        signed_message = self.fake_signing(self.makeMessageText())
+        self.preload_dns_response()
         fiddled_message = signed_message.replace(
             'From: Foo Bar <[e-mail]>',
             'From: Carlos <[e-mail]>')
@@ -230,9 +250,8 @@
 
     def test_dkim_changed_from_realname(self):
         # If the real name part of the message has changed, it's detected.
-        signed_message = self.fake_signing(plain_content)
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns
+        signed_message = self.fake_signing(self.makeMessageText())
+        self.preload_dns_response()
         fiddled_message = signed_message.replace(
             'From: Foo Bar <[e-mail]>',
             'From: Evil Foo <[e-mail]>')
@@ -246,7 +265,7 @@
     def test_dkim_nxdomain(self):
         # If there's no DNS entry for the pubkey it should be handled
         # decently.
-        signed_message = self.fake_signing(plain_content)
+        signed_message = self.fake_signing(self.makeMessageText())
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertWeaklyAuthenticated(principal, signed_message)
@@ -258,8 +277,8 @@
         # treated as weakly authenticated.
         # The library doesn't log anything if there's no header at all.
         principal = authenticateEmail(
-            signed_message_from_string(plain_content))
-        self.assertWeaklyAuthenticated(principal, plain_content)
+            signed_message_from_string(self.makeMessageText()))
+        self.assertWeaklyAuthenticated(principal, self.makeMessageText())
         self.assertEqual(principal.person.preferredemail.email,
             '[e-mail]')
 
@@ -267,10 +286,9 @@
         # The message has a syntactically valid DKIM signature that
         # doesn't actually correspond to what was signed.  We log
         # something about this but we don't want to drop the message.
-        signed_message = self.fake_signing(plain_content)
+        signed_message = self.fake_signing(self.makeMessageText())
         signed_message += 'blah blah'
-        self._dns_responses['example._domainkey.canonical.com.'] = \
-            sample_dns
+        self.preload_dns_response()
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertWeaklyAuthenticated(principal, signed_message)
@@ -291,12 +309,85 @@
         self.factory.makeEmail(
             person=person,
             address='[e-mail]')
-        self._dns_responses['example._domainkey.canonical.com.'] = sample_dns
-        tweaked_message = (
-            "Sender: [e-mail]\n" + plain_content.replace(
-                "From: Foo Bar <[e-mail]>",
-                "From: DKIM Test <[e-mail]>"))
+        self.preload_dns_response()
+        tweaked_message = self.makeMessageText(
+            sender="[e-mail]",
+            from_address="DKIM Test <[e-mail]>")
         signed_message = self.fake_signing(tweaked_message)
         principal = authenticateEmail(
             signed_message_from_string(signed_message))
         self.assertStronglyAuthenticated(principal, signed_message)
+
+    def test_dkim_signed_but_from_unknown_address(self):
+        """Sent from trusted dkim address, but only the From address is known.
+
+        See https://bugs.launchpad.net/launchpad/+bug/925597
+        """
+        self.factory.makePerson(
+            email='[e-mail]',
+            name='dkimtest',
+            displayname='DKIM Test')
+        self.preload_dns_response()
+        tweaked_message = self.makeMessageText(
+            sender="[e-mail]",
+            from_address="DKIM Test <[e-mail]>")
+        signed_message = self.fake_signing(tweaked_message)
+        principal = authenticateEmail(
+            signed_message_from_string(signed_message))
+        self.assertEqual(principal.person.preferredemail.email,
+            '[e-mail]')
+        self.assertWeaklyAuthenticated(principal, signed_message)
+        self.assertDkimLogContains(
+            'valid dkim signature, but not from a known email address')
+
+    def test_dkim_signed_but_from_unverified_address(self):
+        """Sent from trusted dkim address, but only the From address is known.
+
+        The sender is a known, but unverified address.
+
+        See https://bugs.launchpad.net/launchpad/+bug/925597
+        """
+        from_address = "[e-mail]"
+        sender_address = "[e-mail]"
+        person = self.factory.makePerson(
+            email=from_address,
+            name='dkimtest',
+            displayname='DKIM Test')
+        self.factory.makeEmail(sender_address, person, EmailAddressStatus.NEW)
+        self.preload_dns_response()
+        tweaked_message = self.makeMessageText(
+            sender=sender_address,
+            from_address="DKIM Test <[e-mail]>")
+        signed_message = self.fake_signing(tweaked_message)
+        principal = authenticateEmail(
+            signed_message_from_string(signed_message))
+        self.assertEqual(principal.person.preferredemail.email,
+            from_address)
+        self.assertWeaklyAuthenticated(principal, signed_message)
+        self.assertDkimLogContains(
+            'valid dkim signature, but not from an active email address')
+
+    def test_dkim_signed_from_person_without_account(self):
+        """You can have a person with no account.
+
+        We don't accept mail from them.
+
+        See https://bugs.launchpad.net/launchpad/+bug/925597
+        """
+        from_address = "[e-mail]"
+        # This is not quite the same as having account=None, but it seems as
+        # close as the factory lets us get? -- mbp 2012-04-13
+        self.factory.makePerson(
+            email=from_address,
+            name='dkimtest',
+            displayname='DKIM Test',
+            account_status=AccountStatus.NOACCOUNT)
+        self.preload_dns_response()
+        message_text = self.makeMessageText(
+            sender=from_address,
+            from_address=from_address)
+        signed_message = self.fake_signing(message_text)
+        self.assertRaises(
+            InactiveAccount,
+            authenticateEmail,
+            signed_message_from_string(signed_message))