Launchpad itself

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

  1. dkim
  2. Merge into devel
Proposed by Martin Pool
Status: Merged
Approved by: Jonathan Lange
Approved revision: no longer in the source branch.
Merged at revision: 12008
Proposed branch: lp:~mbp/launchpad/dkim
Merge into: lp:launchpad
Diff against target: 914 lines (+267/-117)
9 files modified
lib/canonical/launchpad/doc/emailauthentication.txt (+16/-10)
lib/canonical/launchpad/mail/handlers.py (+54/-49)
lib/canonical/launchpad/mail/helpers.py (+20/-2)
lib/canonical/launchpad/mail/tests/test_handlers.py (+61/-0)
lib/lp/bugs/tests/bugs-emailinterface.txt (+39/-36)
lib/lp/services/mail/incoming.py (+28/-5)
lib/lp/services/mail/tests/incomingmail.txt (+24/-13)
lib/lp/services/mail/tests/test_incoming.py (+24/-1)
utilities/migrater/file-ownership.txt (+1/-1)
To merge this branch: bzr merge lp:~mbp/launchpad/dkim
Reviewer Review Type Date Requested Status
Jonathan Lange (community) Approve
Graham Binns code Pending
Launchpad code reviewers Pending
Review via email: mp+41819@code.launchpad.net

This proposal supersedes a proposal from 2010-11-24.

Commit message

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

Description of the change

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

To post a comment you must log in.
Revision history for this message
Māris Fogels (mars) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Graham Binns (gmb) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Graham Binns (gmb) wrote : Posted in a previous version of this proposal

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)
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Graham Binns (gmb) wrote : Posted in a previous version of this proposal

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

Sure thing.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal
Download full text (6.3 KiB)

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

Read more...

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

There are further failures in emailauthentication.txt, apparently.

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

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal
Download full text (4.1 KiB)

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.
> +    "...

Read more...

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

See recent comments.

review: Needs Fixing
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal
Download full text (5.2 KiB)

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

Read more...

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal
Download full text (3.4 KiB)

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

Read more...

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jonathan Lange (jml) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

Martin, what's the next step here?

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

Still waiting for conflict resolution.

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

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

Do you need help debugging them?

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal
Download full text (14.7 KiB)

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

Revision history for this message
Jonathan Lange (jml) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Martin Pool (mbp) wrote :

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.

Revision history for this message
Jonathan Lange (jml) :
review: Approve
Revision history for this message
Martin Pool (mbp) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/doc/emailauthentication.txt'
2--- lib/canonical/launchpad/doc/emailauthentication.txt 2010-10-18 22:24:59 +0000
3+++ lib/canonical/launchpad/doc/emailauthentication.txt 2010-12-01 06:19:09 +0000
4@@ -20,12 +20,18 @@
5 >>> commit()
6 >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
7
8+For most of these tests, we don't care whether the timestamps are out of
9+date:
10+
11+ >>> def accept_any_timestamp(timestamp, context_message):
12+ ... pass
13+
14 Now Sample Person and Foo Bar have one OpenPGP key each. Next, let's get
15 a test email that's signed and try to authenticate the user who sent it:
16
17 >>> from canonical.launchpad.mail.ftests import read_test_message
18 >>> msg = read_test_message('signed_detached.txt')
19- >>> principal = authenticateEmail(msg)
20+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
21
22 If the user isn't registered in Launchpad, None is return, if it
23 succeeds the authenticated principal:
24@@ -52,7 +58,7 @@
25 message. Inline signatures are supported as well.
26
27 >>> msg = read_test_message('signed_inline.txt')
28- >>> principal = authenticateEmail(msg)
29+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
30 >>> principal is not None
31 True
32 >>> name, addr = email.Utils.parseaddr(msg['From'])
33@@ -65,7 +71,7 @@
34 As well as signed multipart messages:
35
36 >>> msg = read_test_message('signed_multipart.txt')
37- >>> principal = authenticateEmail(msg)
38+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
39 >>> principal is not None
40 True
41 >>> name, addr = email.Utils.parseaddr(msg['From'])
42@@ -80,7 +86,7 @@
43 to deal with it if we receive a dash escaped message.
44
45 >>> msg = read_test_message('signed_dash_escaped.txt')
46- >>> principal = authenticateEmail(msg)
47+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
48 >>> principal is not None
49 True
50 >>> name, addr = email.Utils.parseaddr(msg['From'])
51@@ -96,7 +102,7 @@
52 >>> msg = read_test_message('signed_detached_invalid_signature.txt')
53 >>> name, addr = email.Utils.parseaddr(msg['From'])
54 >>> from_user = getUtility(IPersonSet).getByEmail(addr)
55- >>> principal = authenticateEmail(msg)
56+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
57 Traceback (most recent call last):
58 ...
59 InvalidSignature:...
60@@ -131,7 +137,7 @@
61 ... msg = email.message_from_string(
62 ... line_ending.join(msg_lines), _class=SignedMessage)
63 ... msg.parsed_string = msg.as_string()
64- ... principal = authenticateEmail(msg)
65+ ... principal = authenticateEmail(msg, accept_any_timestamp)
66 ... authenticated_person = IPerson(principal)
67 ... print authenticated_person.preferredemail.email
68 test@canonical.com
69@@ -156,7 +162,7 @@
70 Content-Type: multipart/mixed; boundary="--------------------EuxKj2iCbKjpUGkD"
71 ...
72
73- >>> principal = authenticateEmail(msg)
74+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
75 >>> print IPerson(principal).displayname
76 Sample Person
77
78@@ -178,7 +184,7 @@
79 >>> from canonical.launchpad.interfaces.mail import (
80 ... IWeaklyAuthenticatedPrincipal)
81 >>> msg = read_test_message('unsigned_multipart.txt')
82- >>> principal = authenticateEmail(msg)
83+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
84 >>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
85 True
86
87@@ -191,7 +197,7 @@
88 authenticated user:
89
90 >>> msg = read_test_message('signed_key_not_registered.txt')
91- >>> principal = authenticateEmail(msg)
92+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
93 >>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
94 True
95
96@@ -205,7 +211,7 @@
97 principal.
98
99 >>> msg = read_test_message('signed_inline.txt')
100- >>> principal = authenticateEmail(msg)
101+ >>> principal = authenticateEmail(msg, accept_any_timestamp)
102 >>> IWeaklyAuthenticatedPrincipal.providedBy(principal)
103 False
104
105
106=== renamed file 'lib/canonical/launchpad/mail/errortemplates/not-gpg-signed.txt' => 'lib/canonical/launchpad/mail/errortemplates/unauthenticated-bug-creation.txt'
107=== modified file 'lib/canonical/launchpad/mail/handlers.py'
108--- lib/canonical/launchpad/mail/handlers.py 2010-11-08 13:11:30 +0000
109+++ lib/canonical/launchpad/mail/handlers.py 2010-12-01 06:19:09 +0000
110@@ -15,7 +15,6 @@
111 from canonical.config import config
112 from canonical.database.sqlbase import rollback
113 from canonical.launchpad.helpers import get_email_template
114-from canonical.launchpad.interfaces.gpghandler import IGPGHandler
115 from canonical.launchpad.interfaces.mail import (
116 EmailProcessingError,
117 IBugEditEmailCommand,
118@@ -31,7 +30,6 @@
119 )
120 from canonical.launchpad.mail.helpers import (
121 ensure_not_weakly_authenticated,
122- ensure_sane_signature_timestamp,
123 get_main_body,
124 guess_bugtask,
125 IncomingEmailError,
126@@ -61,16 +59,6 @@
127 )
128
129
130-def extract_signature_timestamp(signed_msg):
131- # break import cycle
132- from lp.services.mail.incoming import (
133- canonicalise_line_endings)
134- signature = getUtility(IGPGHandler).getVerifiedSignature(
135- canonicalise_line_endings(signed_msg.signedContent),
136- signed_msg.signature)
137- return signature.timestamp
138-
139-
140 class MaloneHandler:
141 """Handles emails sent to Malone.
142
143@@ -90,47 +78,64 @@
144 name, args in parse_commands(content,
145 BugEmailCommands.names())]
146
147- def process(self, signed_msg, to_addr, filealias=None, log=None,
148- extract_signature_timestamp=extract_signature_timestamp):
149- """See IMailHandler."""
150+ def extractAndAuthenticateCommands(self, signed_msg, to_addr):
151+ """Extract commands and handle special destinations.
152+
153+ NB: The authentication is carried out against the current principal,
154+ not directly against the message. authenticateEmail must previously
155+ have been called on this thread.
156+
157+ :returns: (final_result, add_comment_to_bug, commands)
158+ If final_result is non-none, stop processing and return this value
159+ to indicate whether the message was dealt with or not.
160+ If add_comment_to_bug, add the contents to the first bug
161+ selected.
162+ commands is a list of bug commands.
163+ """
164+ CONTEXT = 'bug report'
165 commands = self.getCommands(signed_msg)
166- user, host = to_addr.split('@')
167+ to_user, to_host = to_addr.split('@')
168 add_comment_to_bug = False
169- signature = signed_msg.signature
170+ # If there are any commands, we must have strong authentication.
171+ # We send a different failure message for attempts to create a new
172+ # bug.
173+ if to_user.lower() == 'new':
174+ ensure_not_weakly_authenticated(signed_msg, CONTEXT,
175+ 'unauthenticated-bug-creation.txt')
176+ elif len(commands) > 0:
177+ ensure_not_weakly_authenticated(signed_msg, CONTEXT)
178+ if to_user.lower() == 'new':
179+ commands.insert(0, BugEmailCommands.get('bug', ['new']))
180+ elif to_user.isdigit():
181+ # A comment to a bug. We set add_comment_to_bug to True so
182+ # that the comment gets added to the bug later. We don't add
183+ # the comment now, since we want to let the 'bug' command
184+ # handle the possible errors that can occur while getting
185+ # the bug.
186+ add_comment_to_bug = True
187+ commands.insert(0, BugEmailCommands.get('bug', [to_user]))
188+ elif to_user.lower() == 'help':
189+ from_user = getUtility(ILaunchBag).user
190+ if from_user is not None:
191+ preferredemail = from_user.preferredemail
192+ if preferredemail is not None:
193+ to_address = str(preferredemail.email)
194+ self.sendHelpEmail(to_address)
195+ return True, False, None
196+ elif to_user.lower() != 'edit':
197+ # Indicate that we didn't handle the mail.
198+ return False, False, None
199+ return None, add_comment_to_bug, commands
200+
201+ def process(self, signed_msg, to_addr, filealias=None, log=None):
202+ """See IMailHandler."""
203
204 try:
205- if len(commands) > 0:
206- CONTEXT = 'bug report'
207- ensure_not_weakly_authenticated(signed_msg, CONTEXT)
208- if signature is not None:
209- ensure_sane_signature_timestamp(
210- extract_signature_timestamp(signed_msg), CONTEXT)
211-
212- if user.lower() == 'new':
213- # A submit request.
214- commands.insert(0, BugEmailCommands.get('bug', ['new']))
215- if signature is None:
216- raise IncomingEmailError(
217- get_error_message('not-gpg-signed.txt'))
218- elif user.isdigit():
219- # A comment to a bug. We set add_comment_to_bug to True so
220- # that the comment gets added to the bug later. We don't add
221- # the comment now, since we want to let the 'bug' command
222- # handle the possible errors that can occur while getting
223- # the bug.
224- add_comment_to_bug = True
225- commands.insert(0, BugEmailCommands.get('bug', [user]))
226- elif user.lower() == 'help':
227- from_user = getUtility(ILaunchBag).user
228- if from_user is not None:
229- preferredemail = from_user.preferredemail
230- if preferredemail is not None:
231- to_address = str(preferredemail.email)
232- self.sendHelpEmail(to_address)
233- return True
234- elif user.lower() != 'edit':
235- # Indicate that we didn't handle the mail.
236- return False
237+ (final_result, add_comment_to_bug,
238+ commands, ) = self.extractAndAuthenticateCommands(
239+ signed_msg, to_addr)
240+ if final_result is not None:
241+ return final_result
242
243 bug = None
244 bug_event = None
245
246=== modified file 'lib/canonical/launchpad/mail/helpers.py'
247--- lib/canonical/launchpad/mail/helpers.py 2010-11-08 13:11:30 +0000
248+++ lib/canonical/launchpad/mail/helpers.py 2010-12-01 06:19:09 +0000
249@@ -211,7 +211,14 @@
250 def ensure_not_weakly_authenticated(signed_msg, context,
251 error_template='not-signed.txt',
252 no_key_template='key-not-registered.txt'):
253- """Make sure that the current principal is not weakly authenticated."""
254+ """Make sure that the current principal is not weakly authenticated.
255+
256+ NB: While handling an email, the authentication state is stored partly in
257+ properties of the message object, and partly in the current security
258+ principal. As a consequence this function will only work correctly if the
259+ message has just been passed through authenticateEmail -- you can't give
260+ it an arbitrary message object.
261+ """
262 cur_principal = get_current_principal()
263 # The security machinery doesn't know about
264 # IWeaklyAuthenticatedPrincipal yet, so do a manual
265@@ -232,7 +239,18 @@
266
267 def ensure_sane_signature_timestamp(timestamp, context,
268 error_template='old-signature.txt'):
269- """Ensure the signature was generated recently but not in the future."""
270+ """Ensure the signature was generated recently but not in the future.
271+
272+ If the timestamp is wrong, the message is rejected rather than just
273+ treated as untrusted, so that the user gets a chance to understand
274+ the problem. Therefore, this raises an error rather than returning
275+ a value.
276+
277+ :param context: Short user-readable description of the place
278+ the problem occurred.
279+ :raises IncomingEmailError: if the timestamp is stale or implausible,
280+ containing a message based on the context and template.
281+ """
282 fourty_eight_hours = 48 * 60 * 60
283 ten_minutes = 10 * 60
284 now = time.time()
285
286=== modified file 'lib/canonical/launchpad/mail/tests/test_handlers.py'
287--- lib/canonical/launchpad/mail/tests/test_handlers.py 2010-10-26 15:47:24 +0000
288+++ lib/canonical/launchpad/mail/tests/test_handlers.py 2010-12-01 06:19:09 +0000
289@@ -6,6 +6,7 @@
290 from doctest import DocTestSuite
291 import email
292 import time
293+import transaction
294 import unittest
295
296 from canonical.database.sqlbase import commit
297@@ -42,6 +43,66 @@
298 self.assertEqual('bug', commands[0].name)
299 self.assertEqual(['foo'], commands[0].string_args)
300
301+ def test_NonGPGAuthenticatedNewBug(self):
302+ """Mail authenticated other than by gpg can create bugs.
303+
304+ The incoming mail layer is responsible for authenticating the mail,
305+ and setting the current principal to the sender of the mail, either
306+ weakly or non-weakly authenticated. At the layer of the handler,
307+ which this class is testing, we shouldn't care by what mechanism we
308+ decided to act on behalf of the mail sender, only that we did.
309+
310+ In bug 643219, Launchpad had a problem where the MaloneHandler code
311+ was puncturing that abstraction and directly looking at the GPG
312+ signature; this test checks it's fixed.
313+ """
314+ # NB SignedMessage by default isn't actually signed, it just has the
315+ # capability of knowing about signing.
316+ message = self.factory.makeSignedMessage(body=' affects malone\nhi!')
317+ self.assertEquals(message.signature, None)
318+
319+ # Pretend that the mail auth has given us a logged-in user.
320+ handler = MaloneHandler()
321+ with person_logged_in(self.factory.makePerson()):
322+ mail_handled, add_comment_to_bug, commands = \
323+ handler.extractAndAuthenticateCommands(message,
324+ 'new@bugs.launchpad.net')
325+ self.assertEquals(mail_handled, None)
326+ self.assertEquals(map(str, commands), [
327+ 'bug new',
328+ 'affects malone',
329+ ])
330+
331+ def test_mailToHelpFromUnknownUser(self):
332+ """Mail from people of no account to help@ is simply dropped.
333+ """
334+ message = self.factory.makeSignedMessage()
335+ handler = MaloneHandler()
336+ mail_handled, add_comment_to_bug, commands = \
337+ handler.extractAndAuthenticateCommands(message,
338+ 'help@bugs.launchpad.net')
339+ self.assertEquals(mail_handled, True)
340+ self.assertEquals(self.getSentMail(), [])
341+
342+ def test_mailToHelp(self):
343+ """Mail to help@ generates a help command."""
344+ message = self.factory.makeSignedMessage()
345+ handler = MaloneHandler()
346+ with person_logged_in(self.factory.makePerson()):
347+ mail_handled, add_comment_to_bug, commands = \
348+ handler.extractAndAuthenticateCommands(message,
349+ 'help@bugs.launchpad.net')
350+ self.assertEquals(mail_handled, True)
351+ self.assertEquals(len(self.getSentMail()), 1)
352+ # TODO: Check the right mail was sent. -- mbp 20100923
353+
354+ def getSentMail(self):
355+ # Sending mail is (unfortunately) a side effect of parsing the
356+ # commands, and unfortunately you must commit the transaction to get
357+ # them sent.
358+ transaction.commit()
359+ return stub.test_emails[:]
360+
361
362 class FakeSignature:
363
364
365=== modified file 'lib/lp/bugs/tests/bugs-emailinterface.txt'
366--- lib/lp/bugs/tests/bugs-emailinterface.txt 2010-11-05 14:17:11 +0000
367+++ lib/lp/bugs/tests/bugs-emailinterface.txt 2010-12-01 06:19:09 +0000
368@@ -50,6 +50,8 @@
369 signed, so that the system can verify the sender. But to avoid having
370 to sign each email, we'll create a class which fakes a signed email:
371
372+ >>> from lp.testing import sampledata
373+
374 >>> import email.Message
375 >>> class MockSignedMessage(email.Message.Message):
376 ... def __init__(self, *args, **kws):
377@@ -77,14 +79,10 @@
378 ... msg['Message-Id'] = factory.makeUniqueRFC822MsgId()
379 ... return msg
380
381- >>> import time
382- >>> def fake_extract_signature_timestamp(signed_msg):
383- ... return time.time()
384-
385 >>> def process_email(raw_mail):
386 ... msg = construct_email(raw_mail)
387 ... handler.process(msg, msg['To'],
388- ... extract_signature_timestamp=fake_extract_signature_timestamp)
389+ ... )
390
391 >>> process_email(submit_mail)
392
393@@ -156,7 +154,7 @@
394 If we would file a bug on Ubuntu instead, we would submit a mail like
395 this:
396
397- >>> login('test@canonical.com')
398+ >>> login(sampledata.USER_EMAIL)
399 >>> submit_mail = """From: Sample Person <test@canonical.com>
400 ... To: new@bugs.canonical.com
401 ... Date: Fri Jun 17 10:20:23 BST 2005
402@@ -342,13 +340,15 @@
403 >>> from canonical.launchpad.interfaces.mail import IWeaklyAuthenticatedPrincipal
404 >>> from zope.interface import directlyProvides, directlyProvidedBy
405 >>> from zope.security.management import queryInteraction
406- >>> participations = queryInteraction().participations
407- >>> len(participations)
408- 1
409- >>> current_principal = participations[0].principal
410- >>> directlyProvides(
411- ... current_principal, directlyProvidedBy(current_principal),
412- ... IWeaklyAuthenticatedPrincipal)
413+
414+ >>> def simulate_receiving_untrusted_mail():
415+ ... participations = queryInteraction().participations
416+ ... assert len(participations) == 1
417+ ... current_principal = participations[0].principal
418+ ... directlyProvides(
419+ ... current_principal, directlyProvidedBy(current_principal),
420+ ... IWeaklyAuthenticatedPrincipal)
421+ >>> simulate_receiving_untrusted_mail()
422
423 Now we send a comment containing commands.
424
425@@ -385,6 +385,8 @@
426
427 >>> def print_latest_email():
428 ... commit()
429+ ... if not stub.test_emails:
430+ ... raise AssertionError("No emails queued!")
431 ... from_addr, to_addrs, raw_message = stub.test_emails[-1]
432 ... sent_msg = email.message_from_string(raw_message)
433 ... error_mail, original_mail = sent_msg.get_payload()
434@@ -411,7 +413,7 @@
435 ... comment_mail, _class=MockUnsignedMessage)
436 >>> handler.process(
437 ... msg, msg['To'],
438- ... extract_signature_timestamp=fake_extract_signature_timestamp)
439+ ... )
440 True
441 >>> commit()
442
443@@ -457,12 +459,9 @@
444 >>> added_message in bug_one.messages
445 True
446
447-Unmark the principal:
448+In these tests, every time we log in, we're fully trusted again:
449
450- >>> provided_interfaces = directlyProvidedBy(current_principal)
451- >>> directlyProvides(
452- ... current_principal,
453- ... provided_interfaces - IWeaklyAuthenticatedPrincipal)
454+ >>> login(sampledata.USER_EMAIL)
455
456
457 Commands
458@@ -485,7 +484,7 @@
459 >>> def submit_command_email(msg):
460 ... handler.process(
461 ... msg, msg['To'],
462- ... extract_signature_timestamp=fake_extract_signature_timestamp)
463+ ... )
464 ... commit()
465 ... sync(bug)
466
467@@ -734,7 +733,7 @@
468 >>> 'Foo Bar' in [subscription.person.displayname
469 ... for subscription in bug_four.subscriptions]
470 False
471- >>> login('test@canonical.com')
472+ >>> login(sampledata.USER_EMAIL)
473 >>> submit_commands(bug_four, 'unsubscribe')
474 >>> 'Sample Person' in [subscription.person.displayname
475 ... for subscription in bug_four.subscriptions]
476@@ -793,9 +792,9 @@
477 ... for subscriber in bug_five.getIndirectSubscribers()])
478 [u'Sample Person', u'Ubuntu Team']
479
480-(Log back in as test@canonical.com for the tests that follow.)
481+(Log back in for the tests that follow.)
482
483- >>> login("test@canonical.com")
484+ >>> login(sampledata.USER_EMAIL)
485
486 If we specify a non-existant user, an error message will be sent:
487
488@@ -1108,7 +1107,7 @@
489 Attempting to set the milestone for a bug without sufficient
490 permissions also elicits an error message:
491
492- >>> login('test@canonical.com')
493+ >>> login(sampledata.USER_EMAIL)
494 >>> bug = new_firefox_bug()
495 >>> commit()
496
497@@ -1144,13 +1143,13 @@
498 >>>
499 >>> login(ADMIN_EMAIL)
500 >>> sample_person = getUtility(IPersonSet).getByEmail(
501- ... "test@canonical.com")
502+ ... sampledata.USER_EMAIL)
503 >>> ubuntu = getUtility(IDistributionSet).getByName("ubuntu")
504 >>> ubuntu = removeSecurityProxy(ubuntu)
505 >>> ubuntu.bug_supervisor = sample_person
506 >>> logout()
507
508- >>> login('test@canonical.com')
509+ >>> login(sampledata.USER_EMAIL)
510
511 Like the web UI, we can assign a bug to nobody.
512
513@@ -1246,10 +1245,10 @@
514 >>> login('foo.bar@canonical.com')
515 >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
516 >>> ubuntu.driver = getUtility(IPersonSet).getByEmail(
517- ... 'test@canonical.com')
518+ ... sampledata.USER_EMAIL)
519 >>> commit()
520 >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
521- >>> login('test@canonical.com')
522+ >>> login(sampledata.USER_EMAIL)
523 >>> sync(bug)
524
525 Now a new bugtask for the series will be create directly.
526@@ -1302,7 +1301,7 @@
527 >>> ubuntu.driver = None
528 >>> commit()
529 >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
530- >>> login('test@canonical.com')
531+ >>> login(sampledata.USER_EMAIL)
532
533 >>> bug = new_firefox_bug()
534 >>> for bugtask in bug.bugtasks:
535@@ -1329,7 +1328,7 @@
536 ... print driver.displayname
537 Sample Person
538
539- >>> login('test@canonical.com')
540+ >>> login(sampledata.USER_EMAIL)
541 >>> submit_commands(bug, 'affects /firefox/trunk')
542
543 >>> for bugtask in bug.bugtasks:
544@@ -1367,7 +1366,7 @@
545 ... print nomination.target.bugtargetdisplayname
546 Evolution trunk
547
548- >>> login('test@canonical.com')
549+ >>> login(sampledata.USER_EMAIL)
550
551 Let's take on the upstream task on bug four as well. This time we'll
552 sneak in a 'subscribe' command between the 'affects' and the other
553@@ -1642,7 +1641,7 @@
554 The user is a bug supervisors of the upstream product
555 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
556
557- >>> login('test@canonical.com')
558+ >>> login(sampledata.USER_EMAIL)
559 >>> bug_one = getUtility(IBugSet).get(1)
560 >>> submit_commands(
561 ... bug_one, 'status confirmed', 'assignee test@canonical.com')
562@@ -1746,6 +1745,7 @@
563 If none of the bug tasks can be chosen, an error message is sent to the
564 user, telling him that he has to use the 'affects' command.
565
566+ >>> del stub.test_emails[:]
567 >>> login('stuart.bishop@canonical.com')
568 >>> submit_commands(
569 ... bug_one, 'status new', 'assignee foo.bar@canonical.com')
570@@ -1776,7 +1776,9 @@
571 him about the error. Let's start with trying to submit a bug without
572 signing the mail:
573
574- >>> login('test@canonical.com')
575+ >>> del stub.test_emails[:]
576+ >>> login(sampledata.USER_EMAIL)
577+ >>> simulate_receiving_untrusted_mail()
578
579 >>> from canonical.launchpad.mail import signed_message_from_string
580 >>> msg = signed_message_from_string(submit_mail)
581@@ -1784,7 +1786,7 @@
582 >>> msg['Message-Id'] = email.Utils.make_msgid()
583 >>> handler.process(
584 ... msg, msg['To'],
585- ... extract_signature_timestamp=fake_extract_signature_timestamp)
586+ ... )
587 True
588 >>> print_latest_email()
589 Subject: Submit Request Failure
590@@ -1797,6 +1799,7 @@
591
592 A submit without specifying on what we want to file the bug on:
593
594+ >>> login(sampledata.USER_EMAIL)
595 >>> submit_mail_no_bugtask = """From: test@canonical.com
596 ... To: new@malone
597 ... Date: Fri Jun 17 10:20:23 BST 2005
598@@ -2003,7 +2006,7 @@
599 >>> from canonical.launchpad.mailnotification import (
600 ... send_process_error_notification)
601 >>> send_process_error_notification(
602- ... 'test@canonical.com', 'Some subject', 'Some error message.',
603+ ... sampledata.USER_EMAIL, 'Some subject', 'Some error message.',
604 ... msg, failing_command=['foo bar'])
605
606 The To and Subject headers got set to the values we provided:
607@@ -2067,7 +2070,7 @@
608
609 First, we create a new firefox bug.
610
611- >>> login('test@canonical.com')
612+ >>> login(sampledata.USER_EMAIL)
613 >>> submit_mail = """From: Sample Person <test@canonical.com>
614 ... To: new@bugs.canonical.com
615 ... Date: Fri Jun 17 10:20:23 BST 2006
616
617=== modified file 'lib/lp/services/mail/incoming.py'
618--- lib/lp/services/mail/incoming.py 2010-10-23 16:45:43 +0000
619+++ lib/lp/services/mail/incoming.py 2010-12-01 06:19:09 +0000
620@@ -37,6 +37,9 @@
621 from canonical.launchpad.interfaces.mailbox import IMailBox
622 from canonical.launchpad.mail.commands import get_error_message
623 from canonical.launchpad.mail.handlers import mail_handlers
624+from canonical.launchpad.mail.helpers import (
625+ ensure_sane_signature_timestamp,
626+ )
627 from canonical.launchpad.mailnotification import (
628 send_process_error_notification,
629 )
630@@ -61,6 +64,7 @@
631 # Match trailing whitespace.
632 trailing_whitespace = re.compile(r'[ \t]*((?=\r\n)|$)')
633
634+
635 def canonicalise_line_endings(text):
636 r"""Canonicalise the line endings to '\r\n'.
637
638@@ -159,14 +163,23 @@
639 return True
640
641
642-def authenticateEmail(mail):
643+def authenticateEmail(mail,
644+ signature_timestamp_checker=None):
645 """Authenticates an email by verifying the PGP signature.
646
647 The mail is expected to be an ISignedMessage.
648+
649+ If this completes, it will set the current security principal to be the
650+ message sender.
651+
652+ :param signature_timestamp_checker: This callable is
653+ passed the message signature timestamp, and it can raise an exception if
654+ it dislikes it (for example as a replay attack.) This parameter is
655+ intended for use in tests. If None, ensure_sane_signature_timestamp
656+ is used.
657 """
658
659 signature = mail.signature
660- signed_content = mail.signedContent
661
662 name, email_addr = parseaddr(mail['From'])
663 authutil = getUtility(IPlacelessAuthUtility)
664@@ -206,11 +219,19 @@
665 gpghandler = getUtility(IGPGHandler)
666 try:
667 sig = gpghandler.getVerifiedSignature(
668- canonicalise_line_endings(signed_content), signature)
669+ canonicalise_line_endings(mail.signedContent), signature)
670 except GPGVerificationError, e:
671 # verifySignature failed to verify the signature.
672 raise InvalidSignature("Signature couldn't be verified: %s" % str(e))
673
674+ if signature_timestamp_checker is None:
675+ signature_timestamp_checker = ensure_sane_signature_timestamp
676+ # If this fails, we return an error to the user rather than just treating
677+ # it as untrusted, so they can debug or understand the problem.
678+ signature_timestamp_checker(
679+ sig.timestamp,
680+ 'incoming mail verification')
681+
682 for gpgkey in person.gpg_keys:
683 if gpgkey.fingerprint == sig.fingerprint:
684 break
685@@ -255,7 +276,8 @@
686 return request.oopsid
687
688
689-def handleMail(trans=transaction):
690+def handleMail(trans=transaction,
691+ signature_timestamp_checker=None):
692 # First we define an error handler. We define it as a local
693 # function, to avoid having to pass a lot of parameters.
694 # pylint: disable-msg=W0631
695@@ -358,7 +380,8 @@
696 continue
697
698 try:
699- principal = authenticateEmail(mail)
700+ principal = authenticateEmail(
701+ mail, signature_timestamp_checker)
702 except InvalidSignature, error:
703 _handle_user_error(error, mail)
704 continue
705
706=== modified file 'lib/lp/services/mail/tests/incomingmail.txt'
707--- lib/lp/services/mail/tests/incomingmail.txt 2010-10-25 13:16:10 +0000
708+++ lib/lp/services/mail/tests/incomingmail.txt 2010-12-01 06:19:09 +0000
709@@ -62,6 +62,12 @@
710 ... msg.replace_header('To', '123@%s' % domain)
711 ... msgids[domain].append("<%s>" % sendmail(msg))
712
713+handleMail will check the timestamp on signed messages, but the signatures
714+on our testdata are old, and in these tests we don't care to be told.
715+
716+ >>> def accept_any_timestamp(timestamp, context_message):
717+ ... pass
718+
719 Since the User gets authenticated using OpenPGP signatures we have to
720 import the keys before handleMail is called.
721
722@@ -72,6 +78,11 @@
723 >>> LaunchpadZopelessLayer.switchDbUser(config.processmail.dbuser)
724 >>> zopeless_transaction = LaunchpadZopelessLayer.txn
725
726+ >>> handleMailForTest = lambda: handleMail(
727+ ... zopeless_transaction,
728+ ... signature_timestamp_checker=accept_any_timestamp)
729+
730+
731 We temporarily override the error mails' From address, so that they will
732 pass through the authentication stage:
733
734@@ -87,7 +98,7 @@
735 discussed below; this output merely shows that we emit warnings when the
736 header is missing.)
737
738- >>> handleMail(zopeless_transaction)
739+ >>> handleMailForTest()
740 WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
741 WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
742 WARNING:lp.services.mail:No X-Launchpad-Original-To header was present ...
743@@ -139,7 +150,7 @@
744 >>> moin_change = read_test_message('moin-change.txt')
745 >>> moin_change['X-Launchpad-Original-To'] = '123@foo.com'
746 >>> msgid = "<%s>" % sendmail(moin_change)
747- >>> handleMail(zopeless_transaction)
748+ >>> handleMailForTest()
749 >>> msgid not in foo_handler.handledMails
750 True
751
752@@ -154,7 +165,7 @@
753
754 >>> moin_change.replace_header('X-Launchpad-Original-To', '123@bar.com')
755 >>> msgid = "<%s>" % sendmail(moin_change)
756- >>> handleMail(zopeless_transaction)
757+ >>> handleMailForTest()
758 >>> msgid in bar_handler.handledMails
759 True
760
761@@ -176,13 +187,13 @@
762
763 >>> msg = read_test_message('unsigned_inactive.txt')
764 >>> msgid = sendmail(msg, ['edit@malone-domain'])
765- >>> handleMail(zopeless_transaction)
766+ >>> handleMailForTest()
767 >>> msgid not in foo_handler.handledMails
768 True
769
770 >>> msg = read_test_message('invalid_signed_inactive.txt')
771 >>> msgid = sendmail(msg, ['edit@malone-domain'])
772- >>> handleMail(zopeless_transaction)
773+ >>> handleMailForTest()
774 >>> msgid not in foo_handler.handledMails
775 True
776
777@@ -198,7 +209,7 @@
778 >>> msg['CC'] = '123@foo.com'
779 >>> msg['X-Launchpad-Original-To'] = '123@bar.com'
780 >>> msgid = '<%s>' % sendmail (msg, ['123@bar.com'])
781- >>> handleMail(zopeless_transaction)
782+ >>> handleMailForTest()
783 >>> msgid in bar_handler.handledMails
784 True
785
786@@ -238,7 +249,7 @@
787 ... doesn't matter
788 ... """)
789 >>> msgid = sendmail(msg, ['edit@malone-domain'])
790- >>> handleMail(zopeless_transaction)
791+ >>> handleMailForTest()
792 ERROR:canonical.launchpad.mail:An exception was raised inside the handler:
793 ...
794 TestOopsException
795@@ -286,7 +297,7 @@
796 ... doesn't matter
797 ... """)
798 >>> msgid = sendmail(msg, ['edit@malone-domain'])
799- >>> handleMail(zopeless_transaction)
800+ >>> handleMailForTest()
801 ERROR:canonical.launchpad.mail:An exception was raised inside the handler:
802 ...
803 Unauthorized
804@@ -352,7 +363,7 @@
805 If we call handleMail(), we'll see some useful error messages printed
806 out:
807
808- >>> handleMail(zopeless_transaction)
809+ >>> handleMailForTest()
810 ERROR:...:An exception was raised inside the handler: http://...
811 Traceback (most recent call last):
812 ...
813@@ -389,7 +400,7 @@
814 >>> len(stub.test_emails)
815 2
816
817- >>> handleMail(zopeless_transaction)
818+ >>> handleMailForTest()
819 ERROR:...:Upload to Librarian failed...
820 ...
821 UploadFailed: ...Connection refused...
822@@ -416,7 +427,7 @@
823 >>> msg.replace_header('To', '123@foo.com')
824 >>> msg['Return-Path'] = '<>'
825 >>> msgid = '<%s>' % sendmail(msg)
826- >>> handleMail(zopeless_transaction)
827+ >>> handleMailForTest()
828 >>> msgid in foo_handler.handledMails
829 False
830
831@@ -437,7 +448,7 @@
832 ... 'multipart/report; report-type=delivery-status;'
833 ... ' boundary="boundary"')
834 >>> msgid = '<%s>' % sendmail(msg)
835- >>> handleMail(zopeless_transaction)
836+ >>> handleMailForTest()
837 >>> msgid in foo_handler.handledMails
838 False
839
840@@ -452,7 +463,7 @@
841 >>> msg['Return-Path'] = '<not@empty.com>'
842 >>> msg['Precedence'] = 'bulk'
843 >>> msgid = '<%s>' % sendmail(msg)
844- >>> handleMail(zopeless_transaction)
845+ >>> handleMailForTest()
846 >>> msgid in foo_handler.handledMails
847 False
848
849
850=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
851--- lib/lp/services/mail/tests/test_incoming.py 2010-10-15 20:44:53 +0000
852+++ lib/lp/services/mail/tests/test_incoming.py 2010-12-01 06:19:09 +0000
853@@ -10,17 +10,23 @@
854 from zope.security.management import setSecurityPolicy
855
856 from canonical.config import config
857+from canonical.launchpad.ftests import import_secret_test_key
858 from canonical.launchpad.mail.ftests.helpers import testmails_path
859 from canonical.launchpad.testing.systemdocs import LayeredDocFileSuite
860 from canonical.launchpad.webapp.authorization import LaunchpadSecurityPolicy
861+from canonical.launchpad.mail import (
862+ helpers,
863+ )
864+from canonical.testing.layers import LaunchpadZopelessLayer
865 from lp.services.mail.incoming import (
866+ authenticateEmail,
867 handleMail,
868 MailErrorUtility,
869 )
870-from canonical.testing.layers import LaunchpadZopelessLayer
871 from lp.services.mail.sendmail import MailController
872 from lp.services.mail.stub import TestMailer
873 from lp.testing import TestCaseWithFactory
874+from lp.testing.factory import GPGSigningContext
875 from lp.testing.mail_helpers import pop_notifications
876
877
878@@ -81,6 +87,23 @@
879 else:
880 self.assertEqual(old_oops.id, current_oops.id)
881
882+ def test_bad_signature_timestamp(self):
883+ """If the signature is nontrivial future-dated, it's not trusted."""
884+
885+ signing_context = GPGSigningContext(
886+ import_secret_test_key().fingerprint, password='test')
887+ msg = self.factory.makeSignedMessage(signing_context=signing_context)
888+ # It's not trivial to make a gpg signature with a bogus timestamp, so
889+ # let's just treat everything as invalid, and trust that the regular
890+ # implementation of extraction and checking of timestamps is correct,
891+ # or at least tested.
892+ def fail_all_timestamps(timestamp, context):
893+ raise helpers.IncomingEmailError("fail!")
894+ self.assertRaises(
895+ helpers.IncomingEmailError,
896+ authenticateEmail,
897+ msg, fail_all_timestamps)
898+
899
900 def setUp(test):
901 test._old_policy = setSecurityPolicy(LaunchpadSecurityPolicy)
902
903=== modified file 'utilities/migrater/file-ownership.txt'
904--- utilities/migrater/file-ownership.txt 2010-11-16 12:56:01 +0000
905+++ utilities/migrater/file-ownership.txt 2010-12-01 06:19:09 +0000
906@@ -1047,7 +1047,7 @@
907 ./mail/errortemplates/num-arguments-mismatch.txt
908 ./mail/errortemplates/no-such-bug.txt
909 ./mail/errortemplates/security-parameter-mismatch.txt
910- ./mail/errortemplates/not-gpg-signed.txt
911+ ./mail/errortemplates/unauthenticated-bug-creation.txt
912 ./mail/errortemplates/key-not-registered.txt
913 ./mail/errortemplates/oops.txt
914 ./mail/errortemplates/branchmergeproposal-exists.txt