launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/643223-dkim-aliases into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~mbp/launchpad/643223-dkim-aliases/+merge/74061

  Accept as trusted mail signed by the Sender domain not the From address (bug 643223).
  
  This should allow people who use eg gmail (which signs from the Sender) but
  with their own domain in the From address to use Launchpad, as long as they
  have validated the Sender address.
  
  This also adds a process-one-mail.py script that reads a mail from stdin 
  and can more easily be used to locally test incoming mail code.

This is not exactly the approach discussed in bug 643223, but I think equally safe.
-- 
https://code.launchpad.net/~mbp/launchpad/643223-dkim-aliases/+merge/74061
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/643223-dkim-aliases into lp:launchpad.

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2011-08-12 15:57:11 +0000
+++ lib/lp/services/mail/incoming.py	2011-09-05 08:47:24 +0000
@@ -111,7 +111,10 @@
 
 
 def _authenticateDkim(signed_message):
-    """Attempt DKIM authentication of email; return True if known authentic
+    """Attempt DKIM authentication of email.
+
+    :returns: A string email address for the trusted sender, if there is one,
+    otherwise None.
 
     :param signed_message: ISignedMessage
     """
@@ -121,14 +124,17 @@
 
     if getFeatureFlag('mail.dkim_authentication.disabled'):
         log.info('dkim authentication feature disabled')
-        return False
+        return None
 
     # uncomment this for easier test debugging
     # log.addHandler(logging.FileHandler('/tmp/dkim.log'))
 
     dkim_log = cStringIO()
-    log.info('Attempting DKIM authentication of message %s from %s'
-        % (signed_message['Message-ID'], signed_message['From']))
+    log.info(
+        'Attempting DKIM authentication of message id=%s from=%s sender=%s'
+        % (signed_message['Message-ID'],
+            signed_message['From'],
+            signed_message['Sender']))
     signing_details = []
     try:
         # NB: if this fails with a keyword argument error, you need the
@@ -146,26 +152,36 @@
         log.info('DKIM verification result=%s' % (dkim_result,))
     log.debug('DKIM debug log: %s' % (dkim_log.getvalue(),))
     if not dkim_result:
-        return False
+        return None
     # in addition to the dkim signature being valid, we have to check that it
     # was actually signed by the user's domain.
     if len(signing_details) != 1:
         log.info(
             'expected exactly one DKIM details record: %r'
             % (signing_details,))
-        return False
+        return None
     signing_domain = signing_details[0]['d']
-    from_domain = extract_address_domain(signed_message['From'])
-    if signing_domain != from_domain:
-        log.info("DKIM signing domain %s doesn't match From address %s; "
-            "disregarding signature"
-            % (signing_domain, from_domain))
-        return False
     if not _isDkimDomainTrusted(signing_domain):
         log.info("valid DKIM signature from untrusted domain %s"
             % (signing_domain,))
-        return False
-    return True
+        return None
+    for origin in ['From', 'Sender']:
+        if origin is None: continue
+        name, addr = parseaddr(signed_message[origin])
+        try:
+            origin_domain = addr.split('@')[1]
+        except IndexError:
+            log.warning("couldn't extract domain from address %r" % signed_message[origin])
+        if signing_domain == origin_domain:
+            log.info(
+                "DKIM signing domain %s matches %s address %r"
+                % (signing_domain, origin, addr))
+            return addr
+    else:
+        log.info("DKIM signing domain %s doesn't match message origin; "
+            "disregarding signature"
+            % (signing_domain))
+        return None
 
 
 def authenticateEmail(mail,
@@ -184,12 +200,20 @@
         is used.
     """
 
+    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]
+
     signature = mail.signature
-
-    name, email_addr = parseaddr(mail['From'])
     authutil = getUtility(IPlacelessAuthUtility)
     principal = authutil.getPrincipalByLogin(email_addr)
-    log = logging.getLogger('process-mail')
 
     # Check that sender is registered in Launchpad and the email is signed.
     if principal is None:
@@ -207,17 +231,24 @@
         raise InactiveAccount(
             "Mail from a user with an inactive account.")
 
-    dkim_result = _authenticateDkim(mail)
-
-    if dkim_result:
-        if mail.signature is not None:
-            log.info('message has gpg signature, therefore not treating DKIM '
-                'success as conclusive')
-        else:
-            setupInteraction(principal, email_addr)
-            log.debug('message strongly authenticated by dkim')
-            return principal
-
+    if dkim_trusted_addr is not None:
+        log.debug('accepting dkim strongly authenticated mail')
+        setupInteraction(principal, dkim_trusted_addr)
+        return principal
+    else:
+        return _gpgAuthenticateEmail(mail, principal, signature_timestamp_checker)
+
+
+def _gpgAuthenticateEmail(mail, principal, signature_timestamp_checker):
+    """Check GPG signature.
+
+    :param principal: Claimed sender of the mail; to be checked against the actual
+        signature.
+    :returns: principal, either strongly or weakly authenticated.
+    """
+    log = logging.getLogger('process-mail')
+    signature = mail.signature
+    email_addr = parseaddr(mail['From'])[1]
     if signature is None:
         # Mark the principal so that application code can check that the
         # user was weakly authenticated.
@@ -464,7 +495,7 @@
             "No handler registered for '%s' " % (', '.join(addresses)))
 
     if principal is None and not handler.allow_unknown_users:
-        log.info('Unknown user: %s ' % mail['From'])
+        log.info('Mail from unknown users not permitted for this handler')
         return
 
     handled = handler.process(mail, email_addr, file_alias)

=== modified file 'lib/lp/services/mail/tests/test_dkim.py'
--- lib/lp/services/mail/tests/test_dkim.py	2011-08-13 04:07:10 +0000
+++ lib/lp/services/mail/tests/test_dkim.py	2011-09-05 08:47:24 +0000
@@ -261,3 +261,27 @@
         self.assertEqual(principal.person.preferredemail.email,
             'foo.bar@xxxxxxxxxxxxx')
         self.assertDkimLogContains('body hash mismatch')
+
+    def test_dkim_signed_by_other_address(self):
+        # If the message is From one of a person's addresses, and the Sender
+        # corresponds to another, and there is a DKIM signature for the Sender
+        # domain, this is valid - see bug 643223.  For this to be a worthwhile
+        # test  we need the two addresses to be in different domains.   It
+        # will be signed by canonical.com, so make that the sender.
+        person = self.factory.makePerson(
+            email='dkimtest@xxxxxxxxxxxxx',
+            name='dkimtest',
+            displayname='DKIM Test')
+        self.factory.makeEmail(
+            person=person,
+            address='dkimtest@xxxxxxxxxxx')
+        self._dns_responses['example._domainkey.canonical.com.'] = \
+            sample_dns            
+        tweaked_message = 'Sender: dkimtest@xxxxxxxxxxxxx\n' + \
+            plain_content.replace(
+                'From: Foo Bar <foo.bar@xxxxxxxxxxxxx>',
+                'From: DKIM Test <dkimtest@xxxxxxxxxxx>')
+        signed_message = self.fake_signing(tweaked_message)
+        principal = authenticateEmail(
+            signed_message_from_string(signed_message))
+        self.assertStronglyAuthenticated(principal, signed_message)
\ No newline at end of file

=== added file 'scripts/process-one-mail.py'
--- scripts/process-one-mail.py	1970-01-01 00:00:00 +0000
+++ scripts/process-one-mail.py	2011-09-05 08:47:24 +0000
@@ -0,0 +1,53 @@
+#!/usr/bin/python -S
+#
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Process one email message, read from stdin."""
+
+import _pythonpath
+
+import sys
+
+from zope.component.interfaces import ComponentLookupError
+
+from canonical.config import config
+from lp.services.scripts.base import (
+    LaunchpadScript, LaunchpadScriptFailure)
+from lp.services.mail.incoming import (
+    handle_one_mail)
+from canonical.launchpad.interfaces.mailbox import IMailBox
+from lp.services.mail.helpers import (
+    save_mail_to_librarian,
+    )
+from lp.services.mail.signedmessage import signed_message_from_string    
+
+class ProcessMail(LaunchpadScript):
+    usage = """%prog [options]
+
+    Process one incoming email, read from stdin.
+
+    """ + __doc__
+
+    def main(self):
+        self.txn.begin()
+        # NB: This somewhat duplicates handleMail, but there it's mixed in
+        # with handling a mailbox, which we're avoiding here.
+        self.logger.debug("reading message from stdin")
+        raw_mail = sys.stdin.read()
+        self.logger.debug("got %d bytes" % len(raw_mail))
+        file_alias = save_mail_to_librarian(raw_mail)
+        self.logger.debug("saved to librarian as %r" % (file_alias,))
+        parsed_mail = signed_message_from_string(raw_mail)
+        handle_one_mail(
+            self.logger, parsed_mail,
+            file_alias, file_alias.http_url,
+            signature_timestamp_checker=None)
+        self.txn.commit()
+
+
+if __name__ == '__main__':
+    script = ProcessMail('process-one-mail', dbuser=config.processmail.dbuser)
+    # No need to lock; you can run as many as you want as they use no global
+    # resources (like a mailbox).
+    script.run(use_web_security=True)