Issue 8739: Update to smtpd.py to RFC 5321 (original) (raw)

Created on 2010-05-17 13:34 by alfmel, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Messages (38)

msg105902 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-05-17 13:34

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.

msg105938 - (view)

Author: Giampaolo Rodola' (giampaolo.rodola) *

Date: 2010-05-17 21:12

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.

msg105941 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-05-17 21:48

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.

msg105943 - (view)

Author: Giampaolo Rodola' (giampaolo.rodola) *

Date: 2010-05-17 23:06

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.

msg106152 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-05-20 13:46

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.

msg106284 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-05-22 04:08

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.

msg109337 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-07-05 16:25

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?

msg109338 - (view)

Author: R. David Murray (r.david.murray) *

Date: 2010-07-05 16:41

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.

msg112553 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-08-02 21:29

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.

msg112557 - (view)

Author: Richard Jones (richard) *

Date: 2010-08-03 06:43

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

msg112947 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-08-05 06:28

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.

msg113939 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-08-15 04:07

Any more work I need to do on this patch?

msg113968 - (view)

Author: Giampaolo Rodola' (giampaolo.rodola) *

Date: 2010-08-15 15:19

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.

msg114050 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-08-16 15:53

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?

msg114057 - (view)

Author: Giampaolo Rodola' (giampaolo.rodola) *

Date: 2010-08-16 17:42

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?

msg114060 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-08-16 18:11

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.

msg114063 - (view)

Author: Barry A. Warsaw (barry) *

Date: 2010-08-16 18:58

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?

msg114071 - (view)

Author: Alberto Trevino (alfmel)

Date: 2010-08-16 19:50

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.

msg153244 - (view)

Author: R. David Murray (r.david.murray) *

Date: 2012-02-13 03:36

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.

msg153264 - (view)

Author: R. David Murray (r.david.murray) *

Date: 2012-02-13 12:31

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.

msg153282 - (view)

Author: Alberto Trevino (alfmel)

Date: 2012-02-13 17:21

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.

msg153283 - (view)

Author: R. David Murray (r.david.murray) *

Date: 2012-02-13 17:30

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.

msg155378 - (view)

Author: Juhana Jauhiainen (Juhana.Jauhiainen) *

Date: 2012-03-11 09:44

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.

msg155385 - (view)

Author: Michele Orrù (maker) *

Date: 2012-03-11 14:51

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.

msg155386 - (view)

Author: R. David Murray (r.david.murray) *

Date: 2012-03-11 15:30

Yes, cleanups would be better as a separate issue.

msg155390 - (view)

Author: R. David Murray (r.david.murray) *

Date: 2012-03-11 15:58

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.

msg155444 - (view)

Author: Juhana Jauhiainen (Juhana.Jauhiainen) *

Date: 2012-03-12 08:42

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.

msg155446 - (view)

Author: Michele Orrù (maker) *

Date: 2012-03-12 11:19

Patch attached. A few considerations: in case of syntax error, the server responds with " MAIL FROM: