launchpad-reviewers team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #316272 launchpad should verify gmail or DomainKeys authenticators
  https://bugs.launchpad.net/bugs/316272
  #643200 check for stale gpg signatures only cover malone
  https://bugs.launchpad.net/bugs/643200
  #643219 dkim-signed mail to new@bugs doesn't work
  https://bugs.launchpad.net/bugs/643219


This fixes a few more things related to authentication of incoming mail:

 * gpg signature timestamp checks should cover all mail, not just that to malone (bug 643200)

 * checks on mail to new@ should make sure it's strongly authenticated, not specifically that it has a gpg signature (bug 643219)

 * there was no test that gpg mail with implausible timestamps was actually rejected afaics

I haven't interactively tested this and I'm not utterly confident in the existing test coverage, so please review carefully and test it interactively yourself if you know how.

MaloneHandler.process was a bit large so I split out the code that decides what if any commands will be executed.

Thanks
-- 
https://code.launchpad.net/~mbp/launchpad/dkim/+merge/35985
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/dkim into lp:launchpad.

=== modified file 'lib/canonical/launchpad/mail/handlers.py'
--- lib/canonical/launchpad/mail/handlers.py	2010-08-27 13:15:43 +0000
+++ lib/canonical/launchpad/mail/handlers.py	2010-09-20 06:46:48 +0000
@@ -32,14 +32,12 @@
     ISpecificationSet,
     QuestionStatus,
     )
-from canonical.launchpad.interfaces.gpghandler import IGPGHandler
 from canonical.launchpad.mail.commands import (
     BugEmailCommands,
     get_error_message,
     )
 from canonical.launchpad.mail.helpers import (
     ensure_not_weakly_authenticated,
-    ensure_sane_signature_timestamp,
     get_main_body,
     guess_bugtask,
     IncomingEmailError,
@@ -59,16 +57,6 @@
     )
 
 
-def extract_signature_timestamp(signed_msg):
-    # break import cycle
-    from canonical.launchpad.mail.incoming import (
-        canonicalise_line_endings)
-    signature = getUtility(IGPGHandler).getVerifiedSignature(
-        canonicalise_line_endings(signed_msg.signedContent),
-        signed_msg.signature)
-    return signature.timestamp
-
-
 class MaloneHandler:
     """Handles emails sent to Malone.
 
@@ -88,47 +76,61 @@
                 name, args in parse_commands(content,
                                              BugEmailCommands.names())]
 
-    def process(self, signed_msg, to_addr, filealias=None, log=None,
-                extract_signature_timestamp=extract_signature_timestamp):
-        """See IMailHandler."""
+    def extractAndAuthenticateCommands(self, signed_msg, to_addr):
+        """Extract commands and handle special destinations.
+
+        NB: The authentication is carried out against the current principal,
+        not directly against the message.  authenticateEmail must previously
+        have been called on this thread.
+
+        :returns: (final_result, add_comment_to_bug, commands)
+            If final_result is non-none, stop processing and return this value
+            to indicate whether the message was dealt with or not.
+            If add_comment_to_bug, add the contents to the first bug
+            selected.
+            commands is a list of bug commands.
+        """
+        CONTEXT = 'bug report'
         commands = self.getCommands(signed_msg)
-        user, host = to_addr.split('@')
+        to_user, to_host = to_addr.split('@')
         add_comment_to_bug = False
-        signature = signed_msg.signature
+        if len(commands) > 0:
+            ensure_not_weakly_authenticated(signed_msg, CONTEXT)
+        if to_user.lower() == 'new':
+            # A submit request.
+            ensure_not_weakly_authenticated(signed_msg, CONTEXT,
+                'not-gpg-signed.txt')
+            commands.insert(0, BugEmailCommands.get('bug', ['new']))
+        elif to_user.isdigit():
+            # A comment to a bug. We set add_comment_to_bug to True so
+            # that the comment gets added to the bug later. We don't add
+            # the comment now, since we want to let the 'bug' command
+            # handle the possible errors that can occur while getting
+            # the bug.
+            add_comment_to_bug = True
+            commands.insert(0, BugEmailCommands.get('bug', [to_user]))
+        elif to_user.lower() == 'help':
+            from_user = getUtility(ILaunchBag).to_user
+            if from_user is not None:
+                preferredemail = from_user.preferredemail
+                if preferredemail is not None:
+                    to_address = str(preferredemail.email)
+                    self.sendHelpEmail(to_address)
+            return True, False, None
+        elif to_user.lower() != 'edit':
+            # Indicate that we didn't handle the mail.
+            return False, False, None
+        return None, add_comment_to_bug, commands
+
+    def process(self, signed_msg, to_addr, filealias=None, log=None):
+        """See IMailHandler."""
 
         try:
-            if len(commands) > 0:
-                CONTEXT = 'bug report'
-                ensure_not_weakly_authenticated(signed_msg, CONTEXT)
-                if signature is not None:
-                    ensure_sane_signature_timestamp(
-                        extract_signature_timestamp(signed_msg), CONTEXT)
-
-            if user.lower() == 'new':
-                # A submit request.
-                commands.insert(0, BugEmailCommands.get('bug', ['new']))
-                if signature is None:
-                    raise IncomingEmailError(
-                        get_error_message('not-gpg-signed.txt'))
-            elif user.isdigit():
-                # A comment to a bug. We set add_comment_to_bug to True so
-                # that the comment gets added to the bug later. We don't add
-                # the comment now, since we want to let the 'bug' command
-                # handle the possible errors that can occur while getting
-                # the bug.
-                add_comment_to_bug = True
-                commands.insert(0, BugEmailCommands.get('bug', [user]))
-            elif user.lower() == 'help':
-                from_user = getUtility(ILaunchBag).user
-                if from_user is not None:
-                    preferredemail = from_user.preferredemail
-                    if preferredemail is not None:
-                        to_address = str(preferredemail.email)
-                        self.sendHelpEmail(to_address)
-                return True
-            elif user.lower() != 'edit':
-                # Indicate that we didn't handle the mail.
-                return False
+            (final_result, add_comment_to_bug,
+                commands, ) = self.extractAndAuthenticateCommands(
+                    signed_msg, to_addr)
+            if final_result is not None:
+                return final_result
 
             bug = None
             bug_event = None

=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py	2010-08-27 13:15:43 +0000
+++ lib/canonical/launchpad/mail/helpers.py	2010-09-20 06:46:48 +0000
@@ -232,7 +232,10 @@
 
 def ensure_sane_signature_timestamp(timestamp, context,
                                     error_template='old-signature.txt'):
-    """Ensure the signature was generated recently but not in the future."""
+    """Ensure the signature was generated recently but not in the future.
+
+    :raises IncomingEmailError: if the timestamp is stale or implausible.
+    """
     fourty_eight_hours = 48 * 60 * 60
     ten_minutes = 10 * 60
     now = time.time()

=== modified file 'lib/canonical/launchpad/mail/incoming.py'
--- lib/canonical/launchpad/mail/incoming.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/incoming.py	2010-09-20 06:46:48 +0000
@@ -38,6 +38,9 @@
     )
 from canonical.launchpad.mail.commands import get_error_message
 from canonical.launchpad.mail.handlers import mail_handlers
+from canonical.launchpad.mail.helpers import (
+    ensure_sane_signature_timestamp,
+    )
 from canonical.launchpad.mailnotification import (
     send_process_error_notification,
     )
@@ -149,7 +152,7 @@
             % (signing_domain, from_domain))
         return False
     if not _isDkimDomainTrusted(signing_domain):
-        log.warning("valid DKIM signature from untrusted domain %s" 
+        log.warning("valid DKIM signature from untrusted domain %s"
             % (signing_domain,))
         return False
     return True
@@ -159,10 +162,12 @@
     """Authenticates an email by verifying the PGP signature.
 
     The mail is expected to be an ISignedMessage.
+
+    If this completes, it will set the current security principal to be the
+    message sender.
     """
 
     signature = mail.signature
-    signed_content = mail.signedContent
 
     name, email_addr = parseaddr(mail['From'])
     authutil = getUtility(IPlacelessAuthUtility)
@@ -202,11 +207,15 @@
     gpghandler = getUtility(IGPGHandler)
     try:
         sig = gpghandler.getVerifiedSignature(
-            canonicalise_line_endings(signed_content), signature)
+            canonicalise_line_endings(mail.signedContent), signature)
     except GPGVerificationError, e:
         # verifySignature failed to verify the signature.
         raise InvalidSignature("Signature couldn't be verified: %s" % str(e))
 
+    ensure_sane_signature_timestamp(
+        sig.timestamp,
+        'incoming mail verification')
+
     for gpgkey in person.gpg_keys:
         if gpgkey.fingerprint == sig.fingerprint:
             break

=== modified file 'lib/canonical/launchpad/mail/tests/test_handlers.py'
--- lib/canonical/launchpad/mail/tests/test_handlers.py	2010-09-06 09:11:43 +0000
+++ lib/canonical/launchpad/mail/tests/test_handlers.py	2010-09-20 06:46:48 +0000
@@ -44,6 +44,31 @@
         self.assertEqual('bug', commands[0].name)
         self.assertEqual(['foo'], commands[0].string_args)
 
+    def test_NonGPGNewBug(self):
+        """Mail authenticated other than by gpg can create bugs.
+
+        Previously, the MaloneHandler was too specific about checking that it
+        had a GPG signature <http://pad.lv/643219>.
+        """
+        # NB SignedMessage by default isn't actually signed, only knows if it
+        # could be signed
+        message = self.factory.makeSignedMessage(body='  affects malone\nhi!')
+        self.assertEquals(message.signature, None)
+        # normally, here, we'd run authenticateEmail(message) which would make
+        # us either strongly or weakly authenticated depending on the gpg and
+        # or dkim signatures.  instead, we'll directly log in as if we were
+        # strongly authenticated
+        handler = MaloneHandler()
+        with person_logged_in(self.factory.makePerson()):
+            final_result, add_comment_to_bug, commands = \
+                handler.extractAndAuthenticateCommands(message,
+                    'new@xxxxxxxxxxxxxxxxxx')
+        self.assertEquals(final_result, None)
+        self.assertEquals(map(str, commands),
+            [ 'bug new',
+              'affects malone',
+              ])
+
 
 class FakeSignature:
 

=== modified file 'lib/canonical/launchpad/mail/tests/test_incoming.py'
--- lib/canonical/launchpad/mail/tests/test_incoming.py	2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/tests/test_incoming.py	2010-09-20 06:46:48 +0000
@@ -7,15 +7,25 @@
 
 import transaction
 
+from canonical.launchpad.ftests import import_secret_test_key
+from canonical.launchpad.interfaces import (
+    IWeaklyAuthenticatedPrincipal,
+    )
 from canonical.launchpad.mail.ftests.helpers import testmails_path
+from canonical.launchpad.mail import (
+    helpers,
+    )
 from canonical.launchpad.mail.incoming import (
+    authenticateEmail,
     handleMail,
     MailErrorUtility,
     )
+from canonical.launchpad.webapp.interaction import get_current_principal
 from canonical.testing import LaunchpadZopelessLayer
 from lp.services.mail.sendmail import MailController
 from lp.services.mail.stub import TestMailer
 from lp.testing import TestCaseWithFactory
+from lp.testing.factory import GPGSigningContext
 from lp.testing.mail_helpers import pop_notifications
 
 
@@ -76,6 +86,32 @@
         else:
             self.assertEqual(old_oops.id, current_oops.id)
 
+    def test_bad_signature_timestamp(self):
+        """If the signature is nontrivial future-dated, it's not trusted."""
+
+        signing_context = GPGSigningContext(
+            import_secret_test_key().fingerprint, password='test')
+        msg = self.factory.makeSignedMessage(signing_context=signing_context)
+        # it's not easy (for me) to make a gpg signature with a bogus
+        # timestamp, so instead we'll just make sure that it is in fact
+        # checked
+        self._hook_timestamp_check()
+        authenticateEmail(msg)
+        self.assertTrue(
+            IWeaklyAuthenticatedPrincipal.providedBy(get_current_principal()))
+
+    def _hook_timestamp_check(self):
+        saved = helpers.ensure_sane_signature_timestamp
+
+        def restore():
+            helpers.ensure_sane_signature_timestamp = saved
+
+        def fail(timestamp, context):
+            raise helpers.IncomingEmailError("fail!")
+
+        self.addCleanup(restore)
+        helpers.ensure_sane_signature_timestamp = fail
+
 
 def test_suite():
     suite = unittest.TestSuite()