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.