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

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.

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

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.

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

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

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.

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

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

Sure thing.

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

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

There are further failures in emailauthentication.txt, apparently.

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

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

See recent comments.

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

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

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.

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

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

Martin, what's the next step here?

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

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

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

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

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.

Still waiting for conflict resolution.

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

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

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

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

Do you need help debugging them?

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

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