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