classification
Title: SMTPD server does not enforce client starting mail transaction with HELO or EHLO
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Jason.Killen, fruitnuke, giampaolo.rodola, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2012-03-12 21:58 by fruitnuke, last changed 2012-03-20 20:49 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
failing_test.diff fruitnuke, 2012-03-12 22:35 A failing test that shows this bug. review
test_smtpd_helo-a-lot.patch Jason.Killen, 2012-03-20 14:38 this version uses HELO before every test
smtpd.patch Jason.Killen, 2012-03-20 16:50 error if HELO not issued before MAIL, RCPT, and DATA
test_smtpd_less_helo.patch Jason.Killen, 2012-03-20 16:53 this version has less HELOing
test_smtpd.patch Jason.Killen, 2012-03-20 17:23 check HELO and no HELO
test_smtpd.patch Jason.Killen, 2012-03-20 18:15 include checks to test for expected failure/success when not sending HELO
Messages (22)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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
History
Date User Action Args
2012-03-20 20:49:18r.david.murraysetmessages: + msg156461
2012-03-20 20:35:04Jason.Killensetmessages: + msg156459
2012-03-20 20:23:50r.david.murraysetstatus: open -> closed
resolution: fixed
messages: + msg156456

stage: patch review -> resolved
2012-03-20 20:16:50python-devsetnosy: + python-dev
messages: + msg156455
2012-03-20 18:15:00Jason.Killensetfiles: + test_smtpd.patch

messages: + msg156443
2012-03-20 17:44:17r.david.murraysetmessages: + msg156441
2012-03-20 17:23:16Jason.Killensetfiles: + test_smtpd.patch

messages: + msg156438
2012-03-20 17:02:53r.david.murraysetmessages: + msg156435
2012-03-20 16:53:31Jason.Killensetfiles: + test_smtpd_less_helo.patch

messages: + msg156434
2012-03-20 16:50:13Jason.Killensetfiles: + smtpd.patch
2012-03-20 16:49:42Jason.Killensetfiles: - smtpd.py
2012-03-20 16:49:30Jason.Killensetfiles: + smtpd.py
2012-03-20 16:47:40Jason.Killensetfiles: - test_smtpd_nogo.patch
2012-03-20 16:47:30Jason.Killensetfiles: - smtpd_nogo.patch
2012-03-20 16:47:21Jason.Killensetfiles: - smtpd_nogo.patch
2012-03-20 16:27:40Jason.Killensetmessages: + msg156431
2012-03-20 16:12:51r.david.murraysetmessages: + msg156429
2012-03-20 14:38:44Jason.Killensetfiles: + test_smtpd_helo-a-lot.patch

messages: + msg156413
2012-03-19 22:56:56r.david.murraysetmessages: + msg156367
2012-03-19 20:25:04Jason.Killensetfiles: + test_smtpd_nogo.patch

messages: + msg156359
2012-03-19 20:23:12Jason.Killensetfiles: + smtpd_nogo.patch

messages: + msg156358
2012-03-16 20:56:45r.david.murraysetmessages: + msg156084
2012-03-16 20:48:26Jason.Killensetmessages: + msg156083
2012-03-16 20:19:34r.david.murraysetmessages: + msg156076
2012-03-16 20:17:56r.david.murraysetnosy: + r.david.murray

messages: + msg156075
stage: patch review
2012-03-16 19:56:10terry.reedysetnosy: + giampaolo.rodola
2012-03-13 18:36:33Jason.Killensetfiles: + smtpd_nogo.patch
nosy: + Jason.Killen
messages: + msg155643

2012-03-12 22:35:20fruitnukesetfiles: + failing_test.diff
keywords: + patch
messages: + msg155501
2012-03-12 21:58:36fruitnukecreate