This patch updates smtpd.py to be more RFC 5321 compliant.  It contains:

 - EHLO support
 - Implement DATA size limit (32 MiB by default)
 - 8-bit mime extension plumbing (doesn't do anything, but accepts and
   records command)
 - Basic VRFY support
 - Basic HELP support
 - A few improvements to messages (more in line with RFC 5321 examples)
 - Misc. documentation updates

The patch is specific to Python 3.1.  I have not tried to backport the changes to 2.x.  If possible, I would like the patch to be considered for inclusion to 3.2.

Some comments:

-# This file implements the minimal SMTP protocol as defined in RFC 821.  It
+# This file implements the minimal SMTP protocol as defined in RFC 5321.  It

Is RFC 5321 completely implemented? Otherwise I would turn this in "as defined in RFC 821 and part of RFC 5321".

> - Implement DATA size limit (32 MiB by default)

I couldn't find any reference to this in RFC 5321. Is it recommended somewhere else maybe?
Also, issue 2518 and issue 1745035 are somewhat related with this specific problem.

> +        ## TODO: implement command help

Since you implemented HELP command in the first place it would be good to do it completely (I guess this means HELP should be able to accept arguments, as in FTP protocol).


Too bad smtpd currently lacks a test suite.
Before going any further with this issue I would recommend to write tests first.
I have a patch for adding a basic test suite for smtpd module I hope I'll be able to submit within this week.

> Giampaolo Rodola' <g.rodola@gmail.com> added the comment:
> 
> Some comments:
> 
> Is RFC 5321 completely implemented? Otherwise I would turn this in "as
> defined in RFC 821 and part of RFC 5321".

RFC 5321 obsoletes RFCs 821, 974, 1869 and 2821.  I don't think we should 
make reference to them anymore.  Perhaps say something like "implements RFC 
5321 which supersedes RFC 821."

As to how complete the implementation is, section 4.5.1 of RFC 5321 
specifies the following as the minimum set of commands that should be 
supported in a conforming specification: EHLO, HELO, MAIL, RCPT, DATA, RSET, 
NOOP, QUIT, VRFY.  The only gray area is VRFY which is supposed to see if a 
mailbox exists for a particular user.  Since that function cannot be easily 
performed in this proxy smtpd server, section 3.5.3 states a 252 reply code 
should be returned.  My patch returns code 252 if self.__getaddr returns 
true, or 502 if it returns false.

> > - Implement DATA size limit (32 MiB by default)
> I couldn't find any reference to this in RFC 5321. Is it recommended
> somewhere else maybe? Also, issue 2518 and issue 1745035 are somewhat
> related with this specific problem.

It is not, but just seemed like good practice to advertise the limit in EHLO 
and enforce it.  My patch doesn't do a good job of enforcing it since it 
enforces it before doing process_message.  The problems with 2518 and 
1745035 are still there.

> Since you implemented HELP command in the first place it would be good to
> do it completely.

RFC 5321 doesn't specify it must accept arguments, but I agree it is a good 
idea.  I'll work on that and submit a new patch.

> Too bad smtpd currently lacks a test suite.
> Before going any further with this issue I would recommend to write tests
> first. I have a patch for adding a basic test suite for smtpd module I
> hope I'll be able to submit within this week.

It would be great if you could implement a test suite.

> It is not, but just seemed like good practice to advertise the limit in 
> EHLO and enforce it.  My patch doesn't do a good job of enforcing it 
> since it enforces it before doing process_message.  The problems with 
> 2518 and 1745035 are still there.

Then I doubt it would be a good idea, also because the following comment added in issue 1745035 should still stand:

> The patch does not work as Giampaolo intends.  If the patch were 
> applied as-is, no emails longer than 998 bytes could be sent.

Personally I think there's no other way to gracefully solve this other than using a tempfile to store the data, but since I'm not a user of the module I'm going to let someone else comment about this.

> RFC 5321 doesn't specify it must accept arguments, but I agree it is 
> a good idea.  I'll work on that and submit a new patch.

If there's no RFC which states that, then I would provide arguments for HELP *only* if that is a common practice amongst smtp servers.

I have attached a version 2 of the patch.  This patch includes everything in the first version, and adds the following:

 - Support for help arguments (HELP MAIL, for example)
 - Support for setting the maximum message size from the command line

This last feature adds the -s or --size option to the command line.  It allows the user to specify the maximum size for the message.  It is set to 0 for the default, meaning no limit.  This mimics the original behavior of module.  If you specify a size (like --size 32768), it will reject messages larger than the specified number of bytes (32KiB in this case).  If you don't specify the size, the response of EHLP won't list SIZE as one of the extensions.  But, if a size is specified, then it will show it on EHLP.

Hopefully these two changes will address some of the concerns that have been brought up.

On Thursday 20 May 2010 07:46:43 am you wrote:
> If you don't specify the size, the response of EHLP won't list
> SIZE as one of the extensions.  But, if a size is specified, then it
> will show it on EHLP.

Sorry, the EHLP above should be EHLO.  Fat fingers, little sleep, talk about 
help... you get the idea.

What is the status of this patch? Is there anything else I need to do? Any remaining concerns that would stop this patch from being merged?

Yes, the fact that there are no unit tests for the new functionality.  As far as I can see the existing smtpd tests (which don't appear to be too extensive...but this module probably dates from before we had a firm unit test policy), are in test_smtplib.

On Monday, July 05, 2010 10:41:28 am you wrote:
> Yes, the fact that there are no unit tests for the new functionality.

Sorry to take so long to reply.  I have attached the latest version of the 
patch which does everything in rev. 2 of the patch, patches the setuid 
problem discussed in issue 9168, updates to the unit test to account for the 
changes and to test the new functionality, plus some little changes here and 
there.

Please review and advise.

The smtpd module now has a test suite. Please add your unit tests to test_smtpd.py

Sorry.  This is my first submission to Python, so I'm learning the process as I go.

This latest patch is done against today's SVN snapshot.  Just to summarize, it does the following:

  * Updates the main smtpd.py module to make it RFC 5321 compliant:
    - Adds EHLO support
    - Provides basic VRFY support
    - Updated messages (more in line with RFC 5321 examples)

  * Adds additional functionality to smtpd.py:
    - Adds HELP support
    - Implements DATA size limits (optional feature -- backward compatible)
    - 8BITMIME extension plubming

  * Fixes setuid bug in smtpd.py as explained in issue 9168

  * Updates test-smtpd.py to test new functionality

  * Updates test-smtpdlib.py to work with updates to smtpd.py

Again, please review and comment as necessary.

Any more work I need to do on this patch?

Patch no longer applies cleanly because smtpd.py changed in the meantime. A further comment:

-    def __init__(self, server, conn, addr):
+    def __init__(self, server, conn, addr, size = 0):
-    def __init__(self, localaddr, remoteaddr):
+    def __init__(self, localaddr, remoteaddr, size = 0):

This change breaks backward compatibility. I think it would be better to provide this as a SMTPChannel.size_limit class attribute.

On Sunday, August 15, 2010 09:19:27 am Giampaolo Rodola' wrote:
> Patch no longer applies cleanly because smtpd.py changed in the meantime.

Is someone going to fix that or I am expected to play daily catch-up until 
this gets merged?

> A further comment:
> 
> -    def __init__(self, server, conn, addr):
> +    def __init__(self, server, conn, addr, size = 0):
> -    def __init__(self, localaddr, remoteaddr):
> +    def __init__(self, localaddr, remoteaddr, size = 0):
> 
> This change breaks backward compatibility. I think it would be better to
> provide this as a SMTPChannel.size_limit class attribute.

Unfortunately, I don't have the time in the next few weeks to make that 
change.  Can someone else make it?

Re-adapted patch including size_limit change as described in my previous message is in attachment.
Barry and Alberto, could you take a final look at it before committing?

On Monday, August 16, 2010 11:42:51 am you wrote:
> Re-adapted patch including size_limit change as described in my previous
> message is in attachment. Barry and Alberto, could you take a final look
> at it before committing?

Looks good to me.  If the tests pass, then I'm good to go.

The one thing that looks weird to me is VRFY.  Since it never actually does verify the user, should we even claim to support the command?  Why not let subclasses claim support if they want to add it?

On Monday, August 16, 2010 12:58:07 pm Barry A. Warsaw wrote:
> The one thing that looks weird to me is VRFY.  Since it never actually
> does verify the user, should we even claim to support the command?  Why
> not let subclasses claim support if they want to add it?

RFC 5321 section 4.5.1 states VRFY should be implemented in order to be 
considered an RFC 5321-compliant implementation.  But, in section 3.5.3 
paragraph 2 it states that if the actual verification was not performed but 
syntax was checked similar to RCPT, then the response code should be 252.

So my purposes for providing the plumbing for VRFY are:

1. Provide a basic, valid implementation to be as RFC 5321-compliant as 
possible.

2. Let users know the command is there so that it can be reimplemented as 
they build their solutions.

Alberto, might you still interested in working on this?  I thought I'd do a quick update to current trunk and check it in, but in the process of doing that I found some issues.  I suspect it has been frustrating for you that nothing happened with this for 3.2, but it is a non-trivial patch since it involves RFC conformance.

I'm uploading what I got done of the update.  I've restored your introduction of a size argument to the smtp and channel classes (I don't know why Giampaolo objected, adding keyword arguments is not backward incompatible).  A max data size was introduced to deal with a DOS attack on the SMTPD server, called data_size_limit.  I changed that to be 'max_message_size' (rather than your size_limit) because this patch will make it be used for implementing that extended SMTP feature (as well as continuing to provide the DOS-preventing-limit by default).

There are still not enough tests.  In particular there are no tests for the smtpd command line functionality, and there was a bug in that in the last patch (it was still using the class argument you had eliminated at Giampaolo's request).  Writing those tests is of course a bit of a pain, but by combining the stuff from script_helpers with smtplib, it ought to be possible.  (But I wouldn't let a lack of command line tests prevent me from committing a completed patch.)

The more important problem is that your implementation of RFC 5321 left out a critical piece: parameters on the MAIL FROM and RCPT TO commands, and in particular the SIZE parameter to MAIL FROM.  Without that you aren't actually implementing RFC 1870 (or, for that matter, 5321 since a compliant server should reject unknown parameters as a syntax error).

I know it has been a long time, but are you still interested in working on this?  I haven't had much time to review stuff lately, much less do coding for the stdlib, but I'd really like to see this in 3.3, so if you are willing to work on it I'll commit to reviewing it in a timely fashion.

Gah, that's what I get for trying to do something quick.  By changing the name of that variable I introduced a backward incompatibility, since that change was released in 3.2.

David, I'd be happy to help, but I'm pretty busy for the next month. I read the description of your patch, and it sounds good to me. Anything that moves the project forward is always welcomed. Thanks for your work on this.

OK.  Maybe someone else will want to work on it, too.  I'll see if I can get it taken care of one way or another during the PyCon sprints.

I'm going to mark this as easy, because really other than expanding test coverage I think the only thing that needs done is to fix MAIL FROM/RCPT TO.  Another pair of eyes going through the RFC would be good too, of course.  Maybe someone from the core-mentorship list will be interested.

The patch I've attached adds minimal support for the SIZE parameter of MAIL command.

If the given message size exceeds the servers maximum size the server responds with error 552.

I'm currently working on this issue. 
A little cleanup would be appreciated, or it would be better to split that on another issue?
For what I saw, tests are in the form FooTest instead of TestFoo, smtpd imports modules used only in __main__, warnings can be handled the appropriate module, __import__ shall not be used.

Yes, cleanups would be better as a separate issue.

Juhana: thanks for the patch.  I see an issue with it, though.  What if the email address is something like JOHN.SIZER@example.com?

My thought is that there are two ways to handle this.  Either we do a full RFC address parse in __getaddr and have it return the remainder of the line, or we parse from the right hand side looking for keword=value elements (which if I remember the RFC right have a pretty constrained syntax).  

Ideally I'd like to see a general parsing solution so we can handle other parameters in the future easily.  So perhaps a function like

  _getkeywords(arg) -> (arg, kwdict)

that parses the keywords off from the right and returns them in a dict, along with whatever non-keyword argument text is left.

The other path, parsing the address fully, could theoretically use the parseaddr utility from the email package, but I'm not sure if it will return the endpoint of an address embedded in a longer string.

Since Michele has been already working on this I could help with the cleanup once it's separated as a new issue.

David: Thanks for your comments. I wasn't sure if I should make a general solution or not and ended up making this one.

Patch attached.
A few considerations: in case of syntax error, the server responds with " MAIL FROM:<address> [SP <mail-parameters> ] <CRLF>" according to http://tools.ietf.org/html/rfc5321#section-3.3 (instead of "MAIL FROM:<address>"). Note that this could break something, as far as backwards compatibility is concerned. 
Looking at http://tools.ietf.org/html/rfc3030#section-4.2 , I was wondering whether the size parameter on MAIL FROM should have implications on the RCPT TO command (i.e., we should check that the foreign email accepts that size).
Finally, right now the size parameter is not considered until greater than max_message_size. This could let the user push an email with a size greater than the on declared (but smaller than max_message_size).

Tests for command line and cleanup will be on another issue.

Waiting for directives :) ,
-- 
ù

I built on Michele's patch during the pycon sprint, addressing some of the concerns rightly raised in previous comment:

1. Turn off the SIZE extension if client uses HELO (though limits are still used internally to limit message size for DoS protection), and report syntax of MAIL/RCPT correctly depending on whether HELO is issued.
2. Extend max command size by 26 chars when SIZE extension is enabled, as required by RFC 1870.
3. First cut at adding parameters to RCPT command for SMTP extensions as required by RFC 1869/5321.
4. Return 555 for MAIL/RCPT parameters that are valid but not implemented.

and some other issues:

1. Fix typo 'maximium' -> 'maximum'
2. Add test to show failure to parse localpart of email address (left as expected failure as a correct parse will require version 6 of the email package).
3. Fix up command line arg for specifying SIZE extension.

Still some issues to resolve, I already came up with a couple in testing, and some of the (new) code can be cleaned up.

So far this server implements the SIZE and 8BITMIME extensions in a
tightly-coupled manner, and does not make it easy for subclasses to implement additional smtp extensions.  Ideally we would make SMTPChannel extensible in this way but that may require extensive refactoring and would be better raised and addressed as another issue.

By the way, Alberto, if you haven't already submitted a contributor agreement, could you do so please?  We have one from Dan from the sprints.  Michele, you aren't marked in the tracker as having submitted an agreement but you've been active long enough I would think you would have.  Have you and it just isn't reflected in the tracker?

OK, I've gone through Dan's update (thanks very much for the tests!).  I'm uploading a revised patch.  The major differences are:

I've refactored the parsing.  Now it is a three step process: peel off the extra keyword (FROM:, TO:) if there is one, then pull off the address, then parse the remainder as parameters, doing a best-effort parse (that is, ignore tokens that don't parse as parameters rather than failing the entire parse if one is bad).  This fixed a bug where a space between the : and the address would break command parsing.  When the RFC5322 parser from email6 lands, it will almost be a drop in replacement for _parseaddr.  (Oh, yeah, I took the opportunity to eliminate the last __ methods.  There's no reason for those parsing methods to be __ methods.)  I say almost because it will allow us to correctly implement the difference in the syntax of the address for VRFY verses MAIL and RCPT by using two different functions from that parser.

I've removed the 8BITMIME support.  It was not implemented correctly per its RFC, and the server as it exists does not, in fact, support receiving binary data (it decodes what it receives using the utf-8 codec...which means it will raise a decode error on binary data).  It would be possible to add 8BITMIME support, but since it is non trivial we'll leave that to another issue.

I reworked the extended command length support to facilitate adding additional extension support.  What I did may not be the most useful refactoring, though.  I considered just making the limit "large enough", but decided to keep the current behavior since it allows smtpd to be used to test clients handling smtp servers that enforce the limits.  Given that, a nice future feature would be to make the max command length limit settable as well.

I renamed max_message_size back to data_size_limit in order to maintain backward compatibility.

I updated the general help output to reflect the HELO/EHLO state.  I don't know what typical servers do for that (since HELP can be issued before them), but I think I've seen it work that way on other servers.

I've added a few more tests.

Unless someone wants to add tests for the smtpd command line, I think this patch is done.  Reviews welcome.

Note that this patch causes test_logging to fail/hang.  I've opened issue 14314 that mentions the hang (the issue is really about the lack of a timeout in logging's smtp handler) and updated issue 11959 about the issues involved in test_logging using smtpd the way it does.  I'm making fixing that issue a dependency for this one.

David: yes, I did. About two weeks ago.
Probably I'll take a look to those issues :)

David, can you tag this issue as dependency for issue14261 ?

Removing dependency on issue 11959.  Instead I'm going to fix the logging test by adding the necessary updates to its __init__ methods on the smtpd subclasses.  Then 11959 can be dealt with independently.

New changeset ec7456b3f4fe by R David Murray in branch 'default':
#8739: upgrade smtpd to RFC 5321 and 1870.
http://hg.python.org/cpython/rev/ec7456b3f4fe

Thanks very much to everyone who contributed to this patch.  It was a real team effort :)

New changeset 079c1942eedf by R David Murray in branch 'default':
#8739: fix omission of DEBUGSTREAM reset in new test in test_smtpd.
http://hg.python.org/cpython/rev/079c1942eedf