| msg155490 - (view) |
Author: Dan Boswell (fruitnuke) |
Date: 2012-03-12 21:58 |
| The current SMTP RFC (5321) states that 'a client MUST issue HELO or EHLO before starting a mail transaction'. The SMTP server should issue '503 Bad sequence of commands' if a client sends MAIL, RCPT or DATA commands before it sends an HELO/EHLO; currently it does not. To reproduce: 1. Start smtpd.py 2. Telnet to localhost 8025 3. Send 'MAIL from:<foo@example.com>' To which you'll see '250 OK' instead of '503 Bad sequence of commands' |
|
|
| msg155501 - (view) |
Author: Dan Boswell (fruitnuke) |
Date: 2012-03-12 22:35 |
| Attached a diff containing a unit-test for this behavior. |
|
|
| msg155643 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-13 18:36 |
| I've generated a patch that checks to see if seen_greeting is set before processing other commands. This is the first time I've submitted a patch so if there is something more I need to do let me know. |
|
|
| msg156075 - (view) |
Author: R. David Murray (r.david.murray) *  |
Date: 2012-03-16 20:17 |
| Thanks for the patch. I think this is fine. An alternate approach would be to introduce the concept of a state (like imaplib has), have a list of which commands are allowed in which state, and implement the check in the command processing function, but that may not be worth it at smtpd's current level of complexity. One change I'd like to see in the patch (and test): postfix in this case responds: 503 Error: send HELO/EHLO first And I think that is more useful than the text 'Bad sequence of commands'. |
|
|
| msg156076 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-16 20:19 |
| Oh, by the way the mention of EHLO in that message depends on issue 8739 going in. |
|
|
| msg156083 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-16 20:48 |
| On using a different approach: I weighed an approach like that and decided to go with what I thought was the clean/simple/easy approach. If there were more commands that smptd.py accepted I would have probably gone with more like what you mentioned. Change in error text: That's fine with me. I don't really care what it says. I'll make the change and generate a new patch but won't be able to get it done until Monday. Given that the inclusion of EHLO in the error text depends on another patch should I include EHLO, and maybe have to remove it later, or leave it out now and maybe have to put it in later? 2 patches, maybe, and we can pick when the time comes? So when patches like this get applied? If that's covered in an FAQ someplace please point me to it, I'm interested in some of the details. I poked around a bit but couldn't find what I was looking for. |
|
|
| msg156084 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-16 20:56 |
| When do patches get applied? The general answer is "when someone gets around to it" :) In this particular case the issue 8739 patch is waiting on the resolution of its dependency (logging uses smtpd in its tests, and the 8739 patch breaks the logging test). The fix for that issue is a bit ugly, so I haven't decided if I'm ready to apply that or not yet. For now, just leave the EHLO out. We can then apply this patch, and update the 8739 patch to include the addition of the EHLO. |
|
|
| msg156358 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-19 20:23 |
| Redone diff including wording preferred by R. David Murray. |
|
|
| msg156359 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-19 20:25 |
| Change of the smtpd test code to match the wording from the smtpd.patch. Also, added \r\n to the end of the expected string. |
|
|
| msg156367 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-19 22:56 |
| I applied both patches, and I get 16 test failures in test_smtpd. I haven't looked at the errors, but probably most of them are lack of a HELO in the test. |
|
|
| msg156413 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-20 14:38 |
| I'm adding a patch for test_smtpd.py. This version includes the changes in my previous patch and adds sending HELO before the commands in the test. Works good for me. |
|
|
| msg156429 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-20 16:12 |
| Hmm. I'm certain that QUIT shouldn't require HELO, and I wouldn't expect that NOOP would either. I just checked the RFC. The *only* command that requires HELO/EHLO is MAIL (and by implication RCPT, since it in turn requires MAIL). See http://tools.ietf.org/html/rfc5321#section-4.1.4. Sorry to keep bouncing this back. I appreciate the work you are doing. |
|
|
| msg156431 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-20 16:27 |
| smtpd.py does not require HELO before NOOP or QUIT. I added them to test_smtpd.py because I felt it made test_smtpd.py well written as opposed to doing the least the spec requires. That said I suppose I should have added QUITs to every test also if that's the way I really feel. I'll make the change and post a diff. I don't mind this going back and forth. It's my first time around and I'm glad I've been helpful. |
|
|
| msg156434 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-20 16:53 |
| Ok I think this quote from the spec referenced pretty much sums it up. "The NOOP, HELP, EXPN, VRFY, and RSET commands can be used at any time during a session, or without previously initializing a session. SMTP servers SHOULD process these normally (that is, not return a 503 code) even if no EHLO command has yet been received; clients SHOULD open a session with EHLO before sending these commands." All in all either way fits the spec, choose your favorite. |
|
|
| msg156435 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-20 17:02 |
| If you mean "either way in the test", that's true. But what we need to test is that the smtpd server we are providing matches the spec, which means that we need to confirm that it does *accept* QUIT, NOOP, etc without a previous HELO. Also testing that it accepts them after a HELO has been issued would be a bonus :) |
|
|
| msg156438 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-20 17:23 |
| Yes, "either way" in the test. The server shouldn't expect them and the client can do it either way. I'm adding a patch that tests with and without HELO for those commands that can be sent either way. |
|
|
| msg156441 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-20 17:44 |
| I think we're close. You new test 'test_HELO_RSET_syntax' should be named just 'test_HELO_RSET'. And could you please add tests for RCPT and DATA generating the 'no HELO' error? Thanks. |
|
|
| msg156443 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-20 18:15 |
| Removed '_syntax' apparently I fix that before but didn't regen the patch. Added tests for noHELO before some commands to test that failures happen as expected. |
|
|
| msg156455 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2012-03-20 20:16 |
| New changeset 241c33b6cb95 by R David Murray in branch 'default': #14269: smtpd now conforms to the RFC and requires HELO before MAIL. http://hg.python.org/cpython/rev/241c33b6cb95 |
|
|
| msg156456 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-20 20:23 |
| I tweaked a couple more of your test method names, but otherwise used your patch as is. Welcome to the ACKS file :) If you haven't already submitted a contributor agreement it would be great if you did. I'm hoping you'll want to continue making contributions. |
|
|
| msg156459 - (view) |
Author: Jason Killen (Jason.Killen) * |
Date: 2012-03-20 20:35 |
| Thanks and thanks for all you help. My method names are known to need some tweaking. Where is the agreement I need to sign? I looked around the documentation stuff and didn't see it. I'd like to continue contributing just have to find another bug I know how to fix. One last thing, what is the ACKS file? |
|
|
| msg156461 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2012-03-20 20:49 |
| Contributor agreement is here: http://python.org/psf/contrib/contrib-form-python/ ACKS file ships in every Python source tarball (not sure if goes out in the binary ones). Here it is in the repository: http://hg.python.org/cpython/file/default/Misc/ACKS |
|
|