Update to smtpd.py to RFC 5321 #52985
Comments
|
This patch updates smtpd.py to be more RFC 5321 compliant. It contains:
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. |
alfmel
mannequin
added
stdlib
|
Some comments: -# This file implements the minimal SMTP protocol as defined in RFC 821. It Is RFC 5321 completely implemented? Otherwise I would turn this in "as defined in RFC 821 and part of RFC 5321".
I couldn't find any reference to this in RFC 5321. Is it recommended somewhere else maybe?
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. |
RFC 5321 obsoletes RFCs 821, 974, 1869 and 2821. I don't think we should As to how complete the implementation is, section 4.5.1 of RFC 5321
It is not, but just seemed like good practice to advertise the limit in EHLO
RFC 5321 doesn't specify it must accept arguments, but I agree it is a good
It would be great if you could implement a test suite. |
Then I doubt it would be a good idea, also because the following comment added in bpo-1745035 should still stand:
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.
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:
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:
Sorry, the EHLP above should be EHLO. Fat fingers, little sleep, talk about |
|
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:
Sorry to take so long to reply. I have attached the latest version of the 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:
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:
Is someone going to fix that or I am expected to play daily catch-up until
Unfortunately, I don't have the time in the next few weeks to make that |
|
Re-adapted patch including size_limit change as described in my previous message is in attachment. |
|
On Monday, August 16, 2010 11:42:51 am you wrote:
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:
RFC 5321 section 4.5.1 states VRFY should be implemented in order to be So my purposes for providing the plumbing for VRFY are:
|
|
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. |
|
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. 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:
and some other issues:
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 |
|
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 bpo-14314 that mentions the hang (the issue is really about the lack of a timeout in logging's smtp handler) and updated bpo-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. |
|
David, can you tag this issue as dependency for bpo-14261 ? |
|
Removing dependency on bpo-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': |
|
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': |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: