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

[Martin Pool]

[r=jml][ui=none][bug=316272,643200,643219] DKIM authentication for incoming email

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

[Māris Fogels (mars)]

Hi Martin,

I asked Deryck about who may have domain expertise in this:

<mars> deryck, I was wondering if someone on your team would have knowledge in that domain that could help Martin out?
<deryck> mars, perhaps gmb or allenap. But the code team should have some expertise there, too. abentley wrote BaseMailer, I believe.
<mars> deryck, great. thank you for the help
<deryck> np

I suggest you ping one of the people above. They are on either side of your day.

[Jonathan Lange (jml)]

Hey Martin,

I like the refactoring that extracts extractAndAuthenticateCommands.

As a general point, we insist on capitalized sentences with full stops in our comments and docstrings.

I'm finding it hard to grok test_NonGPGNewBug. Your comment there is helpful, but I think it would help me trust the test more if there were reasons for why you aren't doing the normal thing and why you are logging in directly instead.

Also, you'll need to change all of the first person references. Nothing more frustrating to wonder who "I" is.

I also don't know how to make a GPG signature with crazy timestamps. It would be nice to do that instead of checking that the checker is called. The test you've got is OK to land though.

jml

[Jonathan Lange (jml)]

I was going to say, I think I'd appreciate a second review from someone who knows mail (or bug mail) better before this lands.

review: Approve

[Martin Pool (mbp)]

I'll add this text, is it more clear:

        """Mail authenticated other than by gpg can create bugs.

        The incoming mail layer is responsible for authenticating the mail,
        and setting the current principal to the sender of the mail, either
        weakly or non-weakly authenticated. At the layer of the handler,
        which this class is testing, we shouldn't care by what mechanism we
        decided to act on behalf of the mail sender, only that we did.

        In bug 643219, Launchpad had a problem where the MaloneHandler code
        was puncturing that abstraction and directly looking at the GPG
        signature; this test checks it's fixed.
        """

[Graham Binns (gmb)]

Sorry Martin, I didn't get time to review this today. I'll try to take
a look at it tomorrow morning.

--
Graham Binns
http://grahambinns.com

[Graham Binns (gmb)]

I'm happy for this to land (apologies for taking so long to review it). I'm not convinced that I'm competent enough to test this interactively locally, so I suggest that we get the LP team to give the tyres a good kicking when it gets onto staging instead.

review: Approve (code)

[Martin Pool (mbp)]

@gmb Could you please land this for me? Thanks.

[Graham Binns (gmb)]

On 29 September 2010 03:31, Martin Pool <email address hidden> wrote:
> @gmb Could you please land this for me?  Thanks.

Sure thing.

[Martin Pool (mbp)]

OK, this is now updated with the following changes based on the full-testsuite failure, and the tests in bugs-emailinterface.txt now pass.

 * There is a lack of coherence between some things checking for a .signature and some checking the current security principal. the email doctests had some trouble with this; specifically they didn't make the principal weaklyauthenticated before testing that condition. This is now fixed.

 * Typically for doctests, state is run together across tests, so I cleared the mail queue so we'd get a more obvious failure.

 * The not-gpg-signed error template was misnamed and will become more so in future, so I renamed it. The error is not so much about being unsigned, but being unauthenticated when trying to create a new bug.

Here's the incremental diff:

=== renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
=== modified file 'lib/canonical/launchpad/mail/handlers.py'
--- lib/canonical/launchpad/mail/handlers.py 2010-09-21 05:21:10 +0000
+++ lib/canonical/launchpad/mail/handlers.py 2010-10-01 08:16:02 +0000
@@ -94,12 +94,15 @@
         commands = self.getCommands(signed_msg)
         to_user, to_host = to_addr.split('@')
         add_comment_to_bug = False
+ if to_user.lower() == 'new':
+ # Special failure message for unauthenticated attempts to create
+ # new bugs.
+ ensure_not_weakly_authenticated(signed_msg, CONTEXT,
+ 'unauthenticated-bug-creation.txt')
         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

=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py 2010-09-20 06:40:41 +0000
+++ lib/canonical/launchpad/mail/helpers.py 2010-10-01 08:16:02 +0000
@@ -211,7 +211,11 @@
 def ensure_not_weakly_authenticated(signed_msg, context,
                                     error_template='not-signed.txt',
                                     no_key_template='key-not-registered.txt'):
- """Make sure that the current principal is not weakly authenticated."""
+ """Make sure that the current principal is not weakly authenticated.
+
+ NB: This mixes checking properties of the message with properties of the
+ current principal; it's assumed that this message was just received.
+ """
     cur_principal = get_current_principal()
     # The security machinery doesn't know about
     # IWeaklyAuthenticatedPrincipal yet, so do a manual

=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt 2010-10-01 07:07:03 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2010-10-01 08:16:02 +0000
@@ -339,13 +339,15 @@
     ... IWeaklyAuthenticatedPrincipal)
...

[Martin Pool (mbp)]

I'll run the tests for this here; if I could get an additional review that would be nice.

[Martin Pool (mbp)]

There are further failures in emailauthentication.txt, apparently.

I tried running ./bin/test mail but that didn't seem to get this before.

[Jonathan Lange (jml)]

On Fri, Oct 1, 2010 at 9:20 AM, Martin Pool <email address hidden> wrote:
> OK, this is now updated with the following changes based on the full-testsuite failure, and the tests in bugs-emailinterface.txt now pass.
>
>  * There is a lack of coherence between some things checking for a .signature and some checking the current security principal.  the email doctests had some trouble with this; specifically they didn't make the principal weaklyauthenticated before testing that condition.  This is now fixed.
>
>  * Typically for doctests, state is run together across tests, so I cleared the mail queue so we'd get a more obvious failure.
>
>  * The not-gpg-signed error template was misnamed and will become more so in future, so I renamed it.  The error is not so much about being unsigned, but being unauthenticated when trying to create a new bug.
>
> Here's the incremental diff:
>

Thanks for persisting with this. There are a couple of points where
I'm not clear on what's going on and a couple of minor tweaks.

> === renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
> === modified file 'lib/canonical/launchpad/mail/handlers.py'
> --- lib/canonical/launchpad/mail/handlers.py    2010-09-21 05:21:10 +0000
> +++ lib/canonical/launchpad/mail/handlers.py    2010-10-01 08:16:02 +0000
> @@ -94,12 +94,15 @@
>         commands = self.getCommands(signed_msg)
>         to_user, to_host = to_addr.split('@')
>         add_comment_to_bug = False
> +        if to_user.lower() == 'new':
> +            # Special failure message for unauthenticated attempts to create
> +            # new bugs.
> +            ensure_not_weakly_authenticated(signed_msg, CONTEXT,
> +                'unauthenticated-bug-creation.txt')

Why do you need to two clauses for "to_user.lower() == 'new'"? I can't
figure it out from the code. Perhaps there should be a comment.

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

> === modified file 'lib/canonical/launchpad/mail/helpers.py'
> --- lib/canonical/launchpad/mail/helpers.py     2010-09-20 06:40:41 +0000
> +++ lib/canonical/launchpad/mail/helpers.py     2010-10-01 08:16:02 +0000
> @@ -211,7 +211,11 @@
>  def ensure_not_weakly_authenticated(signed_msg, context,
>                                     error_template='not-signed.txt',
>                                     no_key_template='key-not-registered.txt'):
> -    """Make sure that the current principal is not weakly authenticated."""
> +    """Make sure that the current principal is not weakly authenticated.
> +
> +    NB: This mixes checking properties of the message with properties of the
> +    current principal; it's assumed that this message was just received.
> +    "...

[Jonathan Lange (jml)]

See recent comments.

review: Needs Fixing

[Martin Pool (mbp)]

On 1 October 2010 23:33, Jonathan Lange <email address hidden> wrote:
> On Fri, Oct 1, 2010 at 9:20 AM, Martin Pool <email address hidden> wrote:
>> OK, this is now updated with the following changes based on the full-testsuite failure, and the tests in bugs-emailinterface.txt now pass.
>>
>>  * There is a lack of coherence between some things checking for a .signature and some checking the current security principal.  the email doctests had some trouble with this; specifically they didn't make the principal weaklyauthenticated before testing that condition.  This is now fixed.
>>
>>  * Typically for doctests, state is run together across tests, so I cleared the mail queue so we'd get a more obvious failure.
>>
>>  * The not-gpg-signed error template was misnamed and will become more so in future, so I renamed it.  The error is not so much about being unsigned, but being unauthenticated when trying to create a new bug.
>>
>> Here's the incremental diff:
>>
>
> Thanks for persisting with this. There are a couple of points where
> I'm not clear on what's going on and a couple of minor tweaks.
>
>> === renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
>> === modified file 'lib/canonical/launchpad/mail/handlers.py'
>> --- lib/canonical/launchpad/mail/handlers.py    2010-09-21 05:21:10 +0000
>> +++ lib/canonical/launchpad/mail/handlers.py    2010-10-01 08:16:02 +0000
>> @@ -94,12 +94,15 @@
>>         commands = self.getCommands(signed_msg)
>>         to_user, to_host = to_addr.split('@')
>>         add_comment_to_bug = False
>> +        if to_user.lower() == 'new':
>> +            # Special failure message for unauthenticated attempts to create
>> +            # new bugs.
>> +            ensure_not_weakly_authenticated(signed_msg, CONTEXT,
>> +                'unauthenticated-bug-creation.txt')
>
> Why do you need to two clauses for "to_user.lower() == 'new'"? I can't
> figure it out from the code. Perhaps there should be a comment.

I'll add a comment. It's because we need to check for authentication
on mails that either attempt to create a new bug or that have commands
that modify an existing bug, and we give a different error in either
case. After doing this, we must check the recipient address is
recognized, and that again includes the case of 'new@'.

Perhaps I should try harder to refactor this...?

>>         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
>>
>
>
>> === modified file 'lib/canonical/launchpad/mail/helpers.py'
>> --- lib/canonical/launchpad/mail/helpers.py     2010-09-20 06:40:41 +0000
>> +++ lib/canonical/launchpad/mail/helpers.py     2010-10-01 08:16:02 +0000

>> @@ -211,7 +211,11 @@
>>  def ensure_not_weakly_authenticat...

[Martin Pool (mbp)]

On 8 October 2010 12:19, Martin Pool <email address hidden> wrote:
> On 1 October 2010 23:33, Jonathan Lange <email address hidden> wrote:
>> On Fri, Oct 1, 2010 at 9:20 AM, Martin Pool <email address hidden> wrote:

>>> === renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
>>> === modified file 'lib/canonical/launchpad/mail/handlers.py'
>>> --- lib/canonical/launchpad/mail/handlers.py    2010-09-21 05:21:10 +0000
>>> +++ lib/canonical/launchpad/mail/handlers.py    2010-10-01 08:16:02 +0000
>>> @@ -94,12 +94,15 @@
>>>         commands = self.getCommands(signed_msg)
>>>         to_user, to_host = to_addr.split('@')
>>>         add_comment_to_bug = False
>>> +        if to_user.lower() == 'new':
>>> +            # Special failure message for unauthenticated attempts to create
>>> +            # new bugs.
>>> +            ensure_not_weakly_authenticated(signed_msg, CONTEXT,
>>> +                'unauthenticated-bug-creation.txt')
>>
>> Why do you need to two clauses for "to_user.lower() == 'new'"? I can't
>> figure it out from the code. Perhaps there should be a comment.
>
> I'll add a comment.  It's because we need to check for authentication
> on mails that either attempt to create a new bug or that have commands
> that modify an existing bug, and we give a different error in either
> case.   After doing this, we must check the recipient address is
> recognized, and that again includes the case of 'new@'.
>
> Perhaps I should try harder to refactor this...?

I think it's a bit simpler now. You could tear it apart entirely
which could be nice but this branch is old...

>>> @@ -211,7 +211,11 @@
>>>  def ensure_not_weakly_authenticated(signed_msg, context,
>>>                                     error_template='not-signed.txt',
>>>                                     no_key_template='key-not-registered.txt'):
>>> -    """Make sure that the current principal is not weakly authenticated."""
>>> +    """Make sure that the current principal is not weakly authenticated.
>>> +
>>> +    NB: This mixes checking properties of the message with properties of the
>>> +    current principal; it's assumed that this message was just received.
>>> +    """
>>
>> I can't say the expansion clears things up much for me.

Cleaner like this:

def ensure_not_weakly_authenticated(signed_msg, context,
                                    error_template='not-signed.txt',
                                    no_key_template='key-not-registered.txt'):
    """Make sure that the current principal is not weakly authenticated.

    NB: While handling an email, the authentication state is stored partly in
    properties of the message object, and partly in the current security
    principal. As a consequence this function will only work correctly if the
    message has just been passed through authenticateEmail -- you can't give
    it an arbitrary message object.
    """

>> Could you please use lp.testing.sampledata.USER_EMAIL instead of
>> '<email address hidden>'? We're trying to kill off some of our more
>> gratuitous magic literals.
>
> sure, thanks

I've removed all the occurr...

[Jonathan Lange (jml)]

Thanks Martin.

From your comments, things look good, but I don't see any new revisions pushed up. As soon as you'll push them up I'll give them the once over and probably approve for landing.

[Martin Pool (mbp)]

> Thanks Martin.
>
> From your comments, things look good, but I don't see any new revisions pushed
> up. As soon as you'll push them up I'll give them the once over and probably
> approve for landing.

I don't know why they didn't show up in the merge proposal discussion thread on the web page, but the new revisions (from Friday the 15th) are in the branch, are visible in 'unmerged revisions' and in the diff.

[Jonathan Lange (jml)]

review: Approve

[Martin Pool (mbp)]

Looking at it again, I'm not sure if the handling of new@ is really correct,
but the tests should tell us.

- Martin

On 19/10/2010 7:38 PM, "Jonathan Lange" <email address hidden> wrote:

Review: Approve

--
https://code.launchpad.net/~mbp/launchpad/dkim/+merge/35985
You are the owner of lp:~mbp/launch...

[Jonathan Lange (jml)]

Martin, what's the next step here?

[Martin Pool (mbp)]

On 5 November 2010 05:35, Jonathan Lange <email address hidden> wrote:
> Martin, what's the next step here?

I would like someone to review the currently proposed changes, and if
they are acceptable to sponsor landing. Thanks.

--
Martin

[Jonathan Lange (jml)]

I approved on 2010-10-19. Have there been changes since? If not, I'm happy to land this for you.

[Martin Pool (mbp)]

On 7 November 2010 22:34, Jonathan Lange <email address hidden> wrote:
> I approved on 2010-10-19. Have there been changes since? If not, I'm happy to land this for you.

Just confirming what I said on irc: there are no changes since. I'm
going to run the full test suite it in a VM, but if you could also
submit it in parallel with that, I'd appreciate it. If it fails,
please let me know.

--
Martin

[Martin Pool (mbp)]

My attempt to test it was thwarted by bug 629746 causing 'make check'
to fail. I'll try again with bin/test.

[Jonathan Lange (jml)]

Conflicts again!

Text conflict in lib/canonical/launchpad/mail/handlers.py
Text conflict in lib/lp/bugs/tests/bugs-emailinterface.txt
Text conflict in lib/lp/services/mail/tests/test_incoming.py

I can't run the tests on ec2 until they are fixed. Sorry.

[Jonathan Lange (jml)]

Still waiting for conflict resolution.

[Martin Pool (mbp)]

OK, now resolved, and I'm trying to persuade the tests to run.

[Martin Pool (mbp)]

This apparently has some doctest failures:

Failure in test lib/lp/services/mail/tests/incomingmail.txt
Traceback (most recent call last):
  File "/usr/lib/python2.6/unittest.py", line 279, in run
    testMethod()
  File "/usr/lib/python2.6/doctest.py", line 2152, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for incomingmail.txt
  File "lib/lp/services/mail/tests/incomingmail.txt", line 0

----------------------------------------------------------------------
File "lib/lp/services/mail/tests/incomingmail.txt", line 90, in incomingmail.txt
Failed example:
    handleMail(zopeless_transaction)
Differences (ndiff with -expected +actual):
    - WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
    - WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
    - WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
    - WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
    + ERROR:canonical.launchpad.mail:An exception was raised inside the handler:
    + http://localhost:58000/93/c2ab70bc-f39e-11df-af8f-52540048778d.txt
    + Traceback (most recent call last):
    + File "/home/mbp/launchpad/lp-branches/work/lib/lp/services/mail/incoming.py", line 370, in handleMail
    + principal = authenticateEmail(mail)
    + File "/home/mbp/launchpad/lp-branches/work/lib/lp/services/mail/incoming.py", line 221, in authenticateEmail
    + 'incoming mail verification')
    + File "/home/mbp/launchpad/lp-branches/work/lib/canonical/launchpad/mail/helpers.py", line 255, in ensure_sane_signature_timestamp
    + raise IncomingEmailError(error_message)
    + IncomingEmailError: The message you sent included commands to modify the incoming mail verification, but the
    + signature was (apparently) generated too far in the past or future.
    + <BLANKLINE>
....

[Martin Pool (mbp)]

More failures in http://sourcefrog.net/tmp/1y/launchpad-dkim-test-failures.txt

[Jonathan Lange (jml)]

I ran the tests against ec2 – you should have received the email – and have got the same errors.

Do you need help debugging them?

[Martin Pool (mbp)]

On 21 November 2010 05:28, Jonathan Lange <email address hidden> wrote:
> I ran the tests against ec2 – you should have received the email – and have got the same errors.
>
> Do you need help debugging them?

Thanks, I think I'm ok. Will try to do it Monday.

--
Martin

[Martin Pool (mbp)]

Thanks for all your patience and help. The updated branch now passes at least the two doctests that were identified as failing. The specific change is that I've gone back to the approach of passing in the timestamp-checking callback, but moved it to a more appropriate spot that (I think) covers all cases. This is imo cleaner than monkey patching, as I was doing.

I'm thinking of also retargeting this to devel. It has no database dependencies.

=== modified file 'lib/canonical/launchpad/doc/emailauthentication.txt'
--- lib/canonical/launchpad/doc/emailauthentication.txt 2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/doc/emailauthentication.txt 2010-11-24 06:49:53 +0000
@@ -20,12 +20,18 @@
     >>> commit()
     >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)

+For most of these tests, we don't care whether the timestamps are out of
+date:
+
+ >>> def accept_any_timestamp(timestamp, context_message):
+ ... pass
+
 Now Sample Person and Foo Bar have one OpenPGP key each. Next, let's get
 a test email that's signed and try to authenticate the user who sent it:

     >>> from canonical.launchpad.mail.ftests import read_test_message
     >>> msg = read_test_message('signed_detached.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)

 If the user isn't registered in Launchpad, None is return, if it
 succeeds the authenticated principal:
@@ -52,7 +58,7 @@
 message. Inline signatures are supported as well.

     >>> msg = read_test_message('signed_inline.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> principal is not None
     True
     >>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -65,7 +71,7 @@
 As well as signed multipart messages:

     >>> msg = read_test_message('signed_multipart.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> principal is not None
     True
     >>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -80,7 +86,7 @@
 to deal with it if we receive a dash escaped message.

     >>> msg = read_test_message('signed_dash_escaped.txt')
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> principal is not None
     True
     >>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -96,7 +102,7 @@
     >>> msg = read_test_message('signed_detached_invalid_signature.txt')
     >>> name, addr = email.Utils.parseaddr(msg['From'])
     >>> from_user = getUtility(IPersonSet).getByEmail(addr)
- >>> principal = authenticateEmail(msg)
+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
     Traceback (most recent call last):
       ...
     InvalidSignature:...
@@ -131,7 +137,7 @@
     ... msg = email.message_from_string(
     ... line_ending.join(msg_lines), _class=SignedMessage)
     ... msg.parsed_string = msg.as_string()
- ... principal = authenticateEmail(msg)
+ ... principal = authenticateEmail(msg, accept_any_timestamp)
     ... authenticated_person = IPers...

[Jonathan Lange (jml)]

The branch has massive sample data changes. What's up with that?

[Martin Pool (mbp)]

On 24 November 2010 22:35, Jonathan Lange <email address hidden> wrote:
> The branch has massive sample data changes. What's up with that?

I wish I knew. I started from db-devel (because of it being called
lp:launchpad), and I just merged devel again yesterday.

--
Martin

[Martin Pool (mbp)]

16th time's a charm?

This is now rebased on to devel, which should get rid of the sampledata changes caused by integrating db-devel. Aside from that there are few changes from the previously reviewed version, but another scan could be good.

It passes './bin/test -t mail' and I'm just running a local full bin/test to see if that finds any other problems.

[Jonathan Lange (jml)]

review: Approve

[Martin Pool (mbp)]

How silly. I did have these errors before, but the spurious errors from mailman caused me to overlook it. Could someone please resubmit it?

=== modified file 'lib/canonical/launchpad/doc/emailauthentication.txt'
--- lib/canonical/launchpad/doc/emailauthentication.txt	2010-10-18 22:24:59 +0000
+++ lib/canonical/launchpad/doc/emailauthentication.txt	2010-12-01 06:19:09 +0000
@@ -20,12 +20,18 @@
     >>> commit()
     >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
 
+For most of these tests, we don't care whether the timestamps are out of
+date:
+
+    >>> def accept_any_timestamp(timestamp, context_message):
+    ...     pass
+
 Now Sample Person and Foo Bar have one OpenPGP key each. Next, let's get
 a test email that's signed and try to authenticate the user who sent it:
 
     >>> from canonical.launchpad.mail.ftests import read_test_message
     >>> msg = read_test_message('signed_detached.txt')
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
 
 If the user isn't registered in Launchpad, None is return, if it
 succeeds the authenticated principal:
@@ -52,7 +58,7 @@
 message. Inline signatures are supported as well.
 
     >>> msg = read_test_message('signed_inline.txt')
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> principal is not None
     True
     >>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -65,7 +71,7 @@
 As well as signed multipart messages:
 
     >>> msg = read_test_message('signed_multipart.txt')
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> principal is not None
     True
     >>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -80,7 +86,7 @@
 to deal with it if we receive a dash escaped message.
 
     >>> msg = read_test_message('signed_dash_escaped.txt')
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> principal is not None
     True
     >>> name, addr = email.Utils.parseaddr(msg['From'])
@@ -96,7 +102,7 @@
     >>> msg = read_test_message('signed_detached_invalid_signature.txt')
     >>> name, addr = email.Utils.parseaddr(msg['From'])
     >>> from_user = getUtility(IPersonSet).getByEmail(addr)
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     Traceback (most recent call last):
       ...
     InvalidSignature:...
@@ -131,7 +137,7 @@
     ...     msg = email.message_from_string(
     ...         line_ending.join(msg_lines), _class=SignedMessage)
     ...     msg.parsed_string = msg.as_string()
-    ...     principal = authenticateEmail(msg)
+    ...     principal = authenticateEmail(msg, accept_any_timestamp)
     ...     authenticated_person = IPerson(principal)
     ...     print authenticated_person.preferredemail.email
     [e-mail]
@@ -156,7 +162,7 @@
     Content-Type: multipart/mixed; boundary="--------------------EuxKj2iCbKjpUGkD"
     ...
 
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> print IPerson(principal).displayname
     Sample Person
 
@@ -178,7 +184,7 @@
     >>> from canonical.launchpad.interfaces.mail import (
     ...     IWeaklyAuthenticatedPrincipal)
     >>> msg = read_test_message('unsigned_multipart.txt')
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
     True
 
@@ -191,7 +197,7 @@
 authenticated user:
 
     >>> msg = read_test_message('signed_key_not_registered.txt')
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
     True
 
@@ -205,7 +211,7 @@
 principal.
 
     >>> msg = read_test_message('signed_inline.txt')
-    >>> principal = authenticateEmail(msg)
+    >>> principal = authenticateEmail(msg, accept_any_timestamp)
     >>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
     False
 

=== renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
=== modified file 'lib/canonical/launchpad/mail/handlers.py'
--- lib/canonical/launchpad/mail/handlers.py	2010-11-08 13:11:30 +0000
+++ lib/canonical/launchpad/mail/handlers.py	2010-12-01 06:19:09 +0000
@@ -15,7 +15,6 @@
 from canonical.config import config
 from canonical.database.sqlbase import rollback
 from canonical.launchpad.helpers import get_email_template
-from canonical.launchpad.interfaces.gpghandler import IGPGHandler
 from canonical.launchpad.interfaces.mail import (
     EmailProcessingError,
     IBugEditEmailCommand,
@@ -31,7 +30,6 @@
     )
 from canonical.launchpad.mail.helpers import (
     ensure_not_weakly_authenticated,
-    ensure_sane_signature_timestamp,
     get_main_body,
     guess_bugtask,
     IncomingEmailError,
@@ -61,16 +59,6 @@
     )
 
 
-def extract_signature_timestamp(signed_msg):
-    # break import cycle
-    from lp.services.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.
 
@@ -90,47 +78,64 @@
                 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 there are any commands, we must have strong authentication.
+        # We send a different failure message for attempts to create a new
+        # bug.
+        if to_user.lower() == 'new':
+            ensure_not_weakly_authenticated(signed_msg, CONTEXT,
+                'unauthenticated-bug-creation.txt')
+        elif len(commands) > 0:
+            ensure_not_weakly_authenticated(signed_msg, CONTEXT)
+        if to_user.lower() == 'new':
+            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).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-11-08 13:11:30 +0000
+++ lib/canonical/launchpad/mail/helpers.py	2010-12-01 06:19:09 +0000
@@ -211,7 +211,14 @@
 def ensure_not_weakly_authenticated(signed_msg, context,
                                     error_template='not-signed.txt',
                                     no_key_template='key-not-registered.txt'):
-    """Make sure that the current principal is not weakly authenticated."""
+    """Make sure that the current principal is not weakly authenticated.
+
+    NB: While handling an email, the authentication state is stored partly in
+    properties of the message object, and partly in the current security
+    principal.  As a consequence this function will only work correctly if the
+    message has just been passed through authenticateEmail -- you can't give
+    it an arbitrary message object.
+    """
     cur_principal = get_current_principal()
     # The security machinery doesn't know about
     # IWeaklyAuthenticatedPrincipal yet, so do a manual
@@ -232,7 +239,18 @@
 
 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.
+
+    If the timestamp is wrong, the message is rejected rather than just
+    treated as untrusted, so that the user gets a chance to understand
+    the problem.  Therefore, this raises an error rather than returning
+    a value.
+
+    :param context: Short user-readable description of the place
+        the problem occurred.
+    :raises IncomingEmailError: if the timestamp is stale or implausible,
+        containing a message based on the context and template.
+    """
     fourty_eight_hours = 48 * 60 * 60
     ten_minutes = 10 * 60
     now = time.time()

=== modified file 'lib/canonical/launchpad/mail/tests/test_handlers.py'
--- lib/canonical/launchpad/mail/tests/test_handlers.py	2010-10-26 15:47:24 +0000
+++ lib/canonical/launchpad/mail/tests/test_handlers.py	2010-12-01 06:19:09 +0000
@@ -6,6 +6,7 @@
 from doctest import DocTestSuite
 import email
 import time
+import transaction
 import unittest
 
 from canonical.database.sqlbase import commit
@@ -42,6 +43,66 @@
         self.assertEqual('bug', commands[0].name)
         self.assertEqual(['foo'], commands[0].string_args)
 
+    def test_NonGPGAuthenticatedNewBug(self):
+        """Mail authenticated other than by gpg can create bugs.
+
+        The incoming mail layer is responsible for authenticating the mail,
+        and setting the current principal to the sender of the mail, either
+        weakly or non-weakly authenticated.  At the layer of the handler,
+        which this class is testing, we shouldn't care by what mechanism we
+        decided to act on behalf of the mail sender, only that we did.
+
+        In bug 643219, Launchpad had a problem where the MaloneHandler code
+        was puncturing that abstraction and directly looking at the GPG
+        signature; this test checks it's fixed.
+        """
+        # NB SignedMessage by default isn't actually signed, it just has the
+        # capability of knowing about signing.
+        message = self.factory.makeSignedMessage(body='  affects malone\nhi!')
+        self.assertEquals(message.signature, None)
+
+        # Pretend that the mail auth has given us a logged-in user.
+        handler = MaloneHandler()
+        with person_logged_in(self.factory.makePerson()):
+            mail_handled, add_comment_to_bug, commands = \
+                handler.extractAndAuthenticateCommands(message,
+                    '[e-mail]')
+        self.assertEquals(mail_handled, None)
+        self.assertEquals(map(str, commands), [
+            'bug new',
+            'affects malone',
+            ])
+
+    def test_mailToHelpFromUnknownUser(self):
+        """Mail from people of no account to help@ is simply dropped.
+        """
+        message = self.factory.makeSignedMessage()
+        handler = MaloneHandler()
+        mail_handled, add_comment_to_bug, commands = \
+            handler.extractAndAuthenticateCommands(message,
+                '[e-mail]')
+        self.assertEquals(mail_handled, True)
+        self.assertEquals(self.getSentMail(), [])
+
+    def test_mailToHelp(self):
+        """Mail to help@ generates a help command."""
+        message = self.factory.makeSignedMessage()
+        handler = MaloneHandler()
+        with person_logged_in(self.factory.makePerson()):
+            mail_handled, add_comment_to_bug, commands = \
+                handler.extractAndAuthenticateCommands(message,
+                    '[e-mail]')
+        self.assertEquals(mail_handled, True)
+        self.assertEquals(len(self.getSentMail()), 1)
+        # TODO: Check the right mail was sent. -- mbp 20100923
+
+    def getSentMail(self):
+        # Sending mail is (unfortunately) a side effect of parsing the
+        # commands, and unfortunately you must commit the transaction to get
+        # them sent.
+        transaction.commit()
+        return stub.test_emails[:]
+
 
 class FakeSignature:
 

=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
--- lib/lp/bugs/tests/bugs-emailinterface.txt	2010-11-05 14:17:11 +0000
+++ lib/lp/bugs/tests/bugs-emailinterface.txt	2010-12-01 06:19:09 +0000
@@ -50,6 +50,8 @@
 signed, so that the system can verify the sender. But to avoid having
 to sign each email, we'll create a class which fakes a signed email:
 
+    >>> from lp.testing import sampledata
+
     >>> import email.Message
     >>> class MockSignedMessage(email.Message.Message):
     ...     def __init__(self, *args, **kws):
@@ -77,14 +79,10 @@
     ...         msg['Message-Id'] = factory.makeUniqueRFC822MsgId()
     ...     return msg
 
-    >>> import time
-    >>> def fake_extract_signature_timestamp(signed_msg):
-    ...     return time.time()
-
     >>> def process_email(raw_mail):
     ...     msg = construct_email(raw_mail)
     ...     handler.process(msg, msg['To'],
-    ...         extract_signature_timestamp=fake_extract_signature_timestamp)
+    ...         )
 
     >>> process_email(submit_mail)
 
@@ -156,7 +154,7 @@
 If we would file a bug on Ubuntu instead, we would submit a mail like
 this:
 
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
     >>> submit_mail = """From: Sample Person <[e-mail]>
     ... To: [e-mail]
     ... Date: Fri Jun 17 10:20:23 BST 2005
@@ -342,13 +340,15 @@
     >>> from canonical.launchpad.interfaces.mail import IWeaklyAuthenticatedPrincipal
     >>> from zope.interface import directlyProvides, directlyProvidedBy
     >>> from zope.security.management import queryInteraction
-    >>> participations = queryInteraction().participations
-    >>> len(participations)
-    1
-    >>> current_principal = participations[0].principal
-    >>> directlyProvides(
-    ...     current_principal, directlyProvidedBy(current_principal),
-    ...     IWeaklyAuthenticatedPrincipal)
+
+    >>> def simulate_receiving_untrusted_mail():
+    ...     participations = queryInteraction().participations
+    ...     assert len(participations) == 1
+    ...     current_principal = participations[0].principal
+    ...     directlyProvides(
+    ...         current_principal, directlyProvidedBy(current_principal),
+    ...         IWeaklyAuthenticatedPrincipal)
+    >>> simulate_receiving_untrusted_mail()
 
 Now we send a comment containing commands.
 
@@ -385,6 +385,8 @@
 
     >>> def print_latest_email():
     ...     commit()
+    ...     if not stub.test_emails:
+    ...         raise AssertionError("No emails queued!")
     ...     from_addr, to_addrs, raw_message = stub.test_emails[-1]
     ...     sent_msg = email.message_from_string(raw_message)
     ...     error_mail, original_mail = sent_msg.get_payload()
@@ -411,7 +413,7 @@
     ...     comment_mail, _class=MockUnsignedMessage)
     >>> handler.process(
     ...     msg, msg['To'],
-    ...     extract_signature_timestamp=fake_extract_signature_timestamp)
+    ...     )
     True
     >>> commit()
 
@@ -457,12 +459,9 @@
     >>> added_message in bug_one.messages
     True
 
-Unmark the principal:
+In these tests, every time we log in, we're fully trusted again:
 
-    >>> provided_interfaces = directlyProvidedBy(current_principal)
-    >>> directlyProvides(
-    ...     current_principal,
-    ...     provided_interfaces - IWeaklyAuthenticatedPrincipal)
+    >>> login(sampledata.USER_EMAIL)
 
 
 Commands
@@ -485,7 +484,7 @@
     >>> def submit_command_email(msg):
     ...     handler.process(
     ...         msg, msg['To'],
-    ...         extract_signature_timestamp=fake_extract_signature_timestamp)
+    ...         )
     ...     commit()
     ...     sync(bug)
 
@@ -734,7 +733,7 @@
     >>> 'Foo Bar' in [subscription.person.displayname
     ...               for subscription in bug_four.subscriptions]
     False
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
     >>> submit_commands(bug_four, 'unsubscribe')
     >>> 'Sample Person' in [subscription.person.displayname
     ...                     for subscription in bug_four.subscriptions]
@@ -793,9 +792,9 @@
     ...  for subscriber in bug_five.getIndirectSubscribers()])
     [u'Sample Person', u'Ubuntu Team']
 
-(Log back in as [e-mail] for the tests that follow.)
+(Log back in for the tests that follow.)
 
-    >>> login("[e-mail]")
+    >>> login(sampledata.USER_EMAIL)
 
 If we specify a non-existant user, an error message will be sent:
 
@@ -1108,7 +1107,7 @@
 Attempting to set the milestone for a bug without sufficient
 permissions also elicits an error message:
 
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
     >>> bug = new_firefox_bug()
     >>> commit()
 
@@ -1144,13 +1143,13 @@
     >>>
     >>> login(ADMIN_EMAIL)
     >>> sample_person = getUtility(IPersonSet).getByEmail(
-    ...     "[e-mail]")
+    ...     sampledata.USER_EMAIL)
     >>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
     >>> ubuntu = removeSecurityProxy(ubuntu)
     >>> ubuntu.bug_supervisor = sample_person
     >>> logout()
 
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
 
 Like the web UI, we can assign a bug to nobody.
 
@@ -1246,10 +1245,10 @@
     >>> login('[e-mail]')
     >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
     >>> ubuntu.driver = getUtility(IPersonSet).getByEmail(
-    ...     '[e-mail]')
+    ...     sampledata.USER_EMAIL)
     >>> commit()
     >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
     >>> sync(bug)
 
 Now a new bugtask for the series will be create directly.
@@ -1302,7 +1301,7 @@
     >>> ubuntu.driver = None
     >>> commit()
     >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
 
     >>> bug = new_firefox_bug()
     >>> for bugtask in bug.bugtasks:
@@ -1329,7 +1328,7 @@
     ...     print driver.displayname
     Sample Person
 
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
     >>> submit_commands(bug, 'affects /firefox/trunk')
 
     >>> for bugtask in bug.bugtasks:
@@ -1367,7 +1366,7 @@
     ...     print nomination.target.bugtargetdisplayname
     Evolution trunk
 
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
 
 Let's take on the upstream task on bug four as well. This time we'll
 sneak in a 'subscribe' command between the 'affects' and the other
@@ -1642,7 +1641,7 @@
 The user is a bug supervisors of the upstream product
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
     >>> bug_one = getUtility(IBugSet).get(1)
     >>> submit_commands(
     ...     bug_one, 'status confirmed', 'assignee [e-mail]')
@@ -1746,6 +1745,7 @@
 If none of the bug tasks can be chosen, an error message is sent to the
 user, telling him that he has to use the 'affects' command.
 
+    >>> del stub.test_emails[:]
     >>> login('[e-mail]')
     >>> submit_commands(
     ...     bug_one, 'status new', 'assignee [e-mail]')
@@ -1776,7 +1776,9 @@
 him about the error. Let's start with trying to submit a bug without
 signing the mail:
 
-    >>> login('[e-mail]')
+    >>> del stub.test_emails[:]
+    >>> login(sampledata.USER_EMAIL)
+    >>> simulate_receiving_untrusted_mail()
 
     >>> from canonical.launchpad.mail import signed_message_from_string
     >>> msg = signed_message_from_string(submit_mail)
@@ -1784,7 +1786,7 @@
     >>> msg['Message-Id'] = email.Utils.make_msgid()
     >>> handler.process(
     ...     msg, msg['To'],
-    ...     extract_signature_timestamp=fake_extract_signature_timestamp)
+    ...     )
     True
     >>> print_latest_email()
     Subject: Submit Request Failure
@@ -1797,6 +1799,7 @@
 
 A submit without specifying on what we want to file the bug on:
 
+    >>> login(sampledata.USER_EMAIL)
     >>> submit_mail_no_bugtask = """From: [e-mail]
     ... To: new@malone
     ... Date: Fri Jun 17 10:20:23 BST 2005
@@ -2003,7 +2006,7 @@
     >>> from canonical.launchpad.mailnotification import (
     ...     send_process_error_notification)
     >>> send_process_error_notification(
-    ...     '[e-mail]', 'Some subject', 'Some error message.',
+    ...     sampledata.USER_EMAIL, 'Some subject', 'Some error message.',
     ...     msg, failing_command=['foo bar'])
 
 The To and Subject headers got set to the values we provided:
@@ -2067,7 +2070,7 @@
 
 First, we create a new firefox bug.
 
-    >>> login('[e-mail]')
+    >>> login(sampledata.USER_EMAIL)
     >>> submit_mail = """From: Sample Person <[e-mail]>
     ... To: [e-mail]
     ... Date: Fri Jun 17 10:20:23 BST 2006

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2010-10-23 16:45:43 +0000
+++ lib/lp/services/mail/incoming.py	2010-12-01 06:19:09 +0000
@@ -37,6 +37,9 @@
 from canonical.launchpad.interfaces.mailbox import IMailBox
 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,
     )
@@ -61,6 +64,7 @@
 # Match trailing whitespace.
 trailing_whitespace = re.compile(r'[ \t]*((?=\r\n)|$)')
 
+
 def canonicalise_line_endings(text):
     r"""Canonicalise the line endings to '\r\n'.
 
@@ -159,14 +163,23 @@
     return True
 
 
-def authenticateEmail(mail):
+def authenticateEmail(mail,
+    signature_timestamp_checker=None):
     """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.
+
+    :param signature_timestamp_checker: This callable is
+        passed the message signature timestamp, and it can raise an exception if
+        it dislikes it (for example as a replay attack.)  This parameter is
+        intended for use in tests.  If None, ensure_sane_signature_timestamp
+        is used.
     """
 
     signature = mail.signature
-    signed_content = mail.signedContent
 
     name, email_addr = parseaddr(mail['From'])
     authutil = getUtility(IPlacelessAuthUtility)
@@ -206,11 +219,19 @@
     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))
 
+    if signature_timestamp_checker is None:
+        signature_timestamp_checker = ensure_sane_signature_timestamp
+    # If this fails, we return an error to the user rather than just treating
+    # it as untrusted, so they can debug or understand the problem.
+    signature_timestamp_checker(
+        sig.timestamp,
+        'incoming mail verification')
+
     for gpgkey in person.gpg_keys:
         if gpgkey.fingerprint == sig.fingerprint:
             break
@@ -255,7 +276,8 @@
     return request.oopsid
 
 
-def handleMail(trans=transaction):
+def handleMail(trans=transaction,
+    signature_timestamp_checker=None):
     # First we define an error handler. We define it as a local
     # function, to avoid having to pass a lot of parameters.
     # pylint: disable-msg=W0631
@@ -358,7 +380,8 @@
                     continue
 
                 try:
-                    principal = authenticateEmail(mail)
+                    principal = authenticateEmail(
+                        mail, signature_timestamp_checker)
                 except InvalidSignature, error:
                     _handle_user_error(error, mail)
                     continue

=== modified file 'lib/lp/services/mail/tests/incomingmail.txt'
--- lib/lp/services/mail/tests/incomingmail.txt	2010-10-25 13:16:10 +0000
+++ lib/lp/services/mail/tests/incomingmail.txt	2010-12-01 06:19:09 +0000
@@ -62,6 +62,12 @@
     ...     msg.replace_header('To', '123@%s' % domain)
     ...     msgids[domain].append("<%s>" % sendmail(msg))
 
+handleMail will check the timestamp on signed messages, but the signatures
+on our testdata are old, and in these tests we don't care to be told.
+
+    >>> def accept_any_timestamp(timestamp, context_message):
+    ...     pass
+
 Since the User gets authenticated using OpenPGP signatures we have to
 import the keys before handleMail is called.
 
@@ -72,6 +78,11 @@
     >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
     >>> zopeless_transaction = LaunchpadZopelessLayer.txn
 
+    >>> handleMailForTest = lambda: handleMail(
+    ...     zopeless_transaction,
+    ...     signature_timestamp_checker=accept_any_timestamp)
+
+
 We temporarily override the error mails' From address, so that they will
 pass through the authentication stage:
 
@@ -87,7 +98,7 @@
 discussed below; this output merely shows that we emit warnings when the
 header is missing.)
 
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
     WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
     WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
@@ -139,7 +150,7 @@
     >>> moin_change = read_test_message('moin-change.txt')
     >>> moin_change['X-Launchpad-Original-To'] = '[e-mail]'
     >>> msgid = "<%s>" % sendmail(moin_change)
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid not in foo_handler.handledMails
     True
 
@@ -154,7 +165,7 @@
 
     >>> moin_change.replace_header('X-Launchpad-Original-To', '[e-mail]')
     >>> msgid = "<%s>" % sendmail(moin_change)
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid in bar_handler.handledMails
     True
 
@@ -176,13 +187,13 @@
 
     >>> msg = read_test_message('unsigned_inactive.txt')
     >>> msgid = sendmail(msg, ['edit@malone-domain'])
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid not in foo_handler.handledMails
     True
 
     >>> msg = read_test_message('invalid_signed_inactive.txt')
     >>> msgid = sendmail(msg, ['edit@malone-domain'])
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid not in foo_handler.handledMails
     True
 
@@ -198,7 +209,7 @@
     >>> msg['CC'] = '[e-mail]'
     >>> msg['X-Launchpad-Original-To'] = '[e-mail]'
     >>> msgid = '<%s>' % sendmail (msg, ['[e-mail]'])
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid in bar_handler.handledMails
     True
 
@@ -238,7 +249,7 @@
     ... doesn't matter
     ... """)
     >>> msgid = sendmail(msg, ['edit@malone-domain'])
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     ERROR:canonical.launchpad.mail:An exception was raised inside the handler:
     ...
     TestOopsException
@@ -286,7 +297,7 @@
     ... doesn't matter
     ... """)
     >>> msgid = sendmail(msg, ['edit@malone-domain'])
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     ERROR:canonical.launchpad.mail:An exception was raised inside the handler:
     ...
     Unauthorized
@@ -352,7 +363,7 @@
 If we call handleMail(), we'll see some useful error messages printed
 out:
 
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     ERROR:...:An exception was raised inside the handler: http://...
     Traceback (most recent call last):
     ...
@@ -389,7 +400,7 @@
     >>> len(stub.test_emails)
     2
 
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     ERROR:...:Upload to Librarian failed...
     ...
     UploadFailed: ...Connection refused...
@@ -416,7 +427,7 @@
     >>> msg.replace_header('To', '[e-mail]')
     >>> msg['Return-Path'] = '<>'
     >>> msgid = '<%s>' % sendmail(msg)
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid in foo_handler.handledMails
     False
 
@@ -437,7 +448,7 @@
     ...     'multipart/report; report-type=delivery-status;'
     ...     ' boundary="boundary"')
     >>> msgid = '<%s>' % sendmail(msg)
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid in foo_handler.handledMails
     False
 
@@ -452,7 +463,7 @@
     >>> msg['Return-Path'] = '<[e-mail]>'
     >>> msg['Precedence'] = 'bulk'
     >>> msgid = '<%s>' % sendmail(msg)
-    >>> handleMail(zopeless_transaction)
+    >>> handleMailForTest()
     >>> msgid in foo_handler.handledMails
     False
 

=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2010-10-15 20:44:53 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2010-12-01 06:19:09 +0000
@@ -10,17 +10,23 @@
 from zope.security.management import setSecurityPolicy
 
 from canonical.config import config
+from canonical.launchpad.ftests import import_secret_test_key
 from canonical.launchpad.mail.ftests.helpers import testmails_path
 from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
 from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
+from canonical.launchpad.mail import (
+    helpers,
+    )
+from canonical.testing.layers import LaunchpadZopelessLayer
 from lp.services.mail.incoming import (
+    authenticateEmail,
     handleMail,
     MailErrorUtility,
     )
-from canonical.testing.layers 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
 
 
@@ -81,6 +87,23 @@
         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 trivial to make a gpg signature with a bogus timestamp, so
+        # let's just treat everything as invalid, and trust that the regular
+        # implementation of extraction and checking of timestamps is correct,
+        # or at least tested.
+        def fail_all_timestamps(timestamp, context):
+            raise helpers.IncomingEmailError("fail!")
+        self.assertRaises(
+            helpers.IncomingEmailError,
+            authenticateEmail,
+            msg, fail_all_timestamps)
+
 
 def setUp(test):
     test._old_policy = setSecurityPolicy(LaunchpadSecurityPolicy)

=== modified file 'utilities/migrater/file-ownership.txt'
--- utilities/migrater/file-ownership.txt	2010-11-16 12:56:01 +0000
+++ utilities/migrater/file-ownership.txt	2010-12-01 06:19:09 +0000
@@ -1047,7 +1047,7 @@
     ./mail/errortemplates/num-arguments-mismatch.txt
     ./mail/errortemplates/no-such-bug.txt
     ./mail/errortemplates/security-parameter-mismatch.txt
-    ./mail/errortemplates/not-gpg-signed.txt
+    ./mail/errortemplates/unauthenticated-bug-creation.txt
     ./mail/errortemplates/key-not-registered.txt
     ./mail/errortemplates/oops.txt
     ./mail/errortemplates/branchmergeproposal-exists.txt