classification
Title: POP3 missing support for starttls
Type: enhancement Stage: resolved
Components: email, Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, giampaolo.rodola, janssen, jcea, lcatucci, pitrou, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2008-11-30 16:39 by lcatucci, last changed 2012-11-24 17:15 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
poplib_01_socket_shutdown_v3.diff lcatucci, 2012-07-03 20:50 review
poplib_02_server_capabilities_v4.diff lcatucci, 2012-11-18 14:51
poplib_03_starttls_v5.diff lcatucci, 2012-11-23 11:40
Messages (26)
msg76643 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2008-11-30 16:39
In the enclosed patch, there are four changes 
1. add support for the optional CAPA pop command, since it is needed
   for starttls support discovery
2. add support for the STLS pop command
3. replace home-grown file-like methods and replace them with ssl
   socket's makefile() in POP3_SSL
4. Properly shutdown sockets at close() time to avoid server-side pile-up
msg76645 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2008-11-30 16:46
The needed changes to documentation if the patch gets accepted
msg76713 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2008-12-01 23:00
You might split your patch into smaller patches, example:
  * patch 1: implement CAPA method
  * patch 2: POP3_SSL refatoring
  * patch 3: stls() method

Comments:
- Do you really need to keep a reference to the "raw" socket 
(self._sock) for a SSL connection?
- I don't understand what is sock.shutdown(SHUT_RDWR). When is it 
needed? Does it change the behaviour?
- Could you write a method to parse the capabilities because I hate 
long lambda function. You may write doctest for the new method :-)
msg76717 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2008-12-01 23:50
I understand the need to keep things simple, but this time the split
seemed a bit overkill. If needed, I'll redo the patch into a small
series. Thinking of it... I was unsure if it really made sense to split
out smtp/pop/imap patches instead of sending them all at once... :-)

As for the shutdown before close, it's needed to let the server know
we are leaving, instead of waiting until socket timeout. This is the
reason I need to keep the reference to the wrapped socket.

You don't usually configure maildrop servers to limit the number/rate of
connects as you do on smtp servers; still, you could get into problems
with stateful forewalls or the like.
msg76719 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2008-12-02 00:05
> As for the shutdown before close, it's needed to let the server know
> we are leaving, instead of waiting until socket timeout.

Extracts of an UNIX FAQ [1]:
"Generally the difference between close() and shutdown() is: close() 
closes the socket id for the process but the connection is still 
opened if another process shares this socket id."

"i have noticed on some (mostly non-unix) operating systems though a 
close by all processes (threads) is not enough to correctly flush 
data, (or threads) is not. A shutdown must be done, but on many 
systems it is superfulous."

So shutdown() is useless on most OS, but it looks like it's better to 
use it ;-)

> This is the reason I need to keep the reference to the wrapped 
socket.

You don't need to do that. The wrapper already calls shutdown() of the 
parent socket (see Lib/ssl.py).

[1] http://www.developerweb.net/forum/archive/index.php/t-2940.html
msg76734 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2008-12-02 12:31
I'm reasonably sure Victor is right about the original socket being
unneeded. How does the series look now?
msg76735 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2008-12-02 12:44
"lst[1:] if len(lst)>1 else []" is useless, lst[1:] alone does the 
same :-) So it can be simplify to:
  def _parsecap(line):
     lst = line.split()
     return lst[0], lst[1:]

You might add a real example in the capa() docstring.

About poplib_02_sock_shutdown.diff: I'm not sure that poplib is the 
right place to fix the issue... if there is really an issue. Why not 
calling shutdown directly in socket.close()? :-)

You removed self._sock, nice.
msg76744 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2008-12-02 14:26
Changed CAPA as requested: now there is a proper docstring, and the
useless if ... else ... is gone.
msg76745 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2008-12-02 14:31
As for the shutdown/close comment, I think there could be cases
where you are going to close but don't want to shutdown: e.g. you might
close a dup-ed socket, but the other owner would want to continue
working with his copy of the socket.
msg81327 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2009-02-07 00:49
There is a problem I forgot to state with test_anonlogin:
since I try and test both SSL and starttls, the DoS checker at cmu kicks
in and refuse the login attempt in PopSSLTest. Since I'd rather avoid
cheating, I'm leaning to try logging in as SomeUser, and expecting a
failure in both cases. Comments?
msg81330 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2009-02-07 01:04
I'm enclosing the expected-failure version of test_popnet. It's much
simpler to talk and comment about something you can see!
msg91094 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2009-07-30 13:24
How is going? Any hope for 2.7/3.2?
msg100776 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2010-03-10 01:07
Ping...

Any hope for 2.7/3.2?
msg163514 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-06-23 01:12
3.3 beta will be published next Sunday. Any hope?

Lorenzo, is your patch applicable to 3.3?
msg163817 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2012-06-24 18:36
I've refreshed the patches to apply on 3.3, and adapted the to the
unit-test framework by giampaolo.rodola.
msg164586 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2012-07-03 07:33
I've refreshed once more the patches, adding the implementation of the stls command in test_poplib.py.

IMHO, the changes as they stand now are low risk, and could as well go into 3.3.

With many thanks to Giampaolo for implementing the asynchat/asyncore pop3 server!
msg164601 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-07-03 13:46
Lorenzo, 3.3 is in beta mode now. No new features accepted :-(.

Please, fulfill a PSF contributor agreement http://www.python.org/psf/contrib/
msg164627 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2012-07-03 21:07
Jesús,
      as requested, I've scanned and sent the signed contributor agreement.

In the meanwhile, I updated once more patches 01 and 03:

01: stealed the socket shutdown check from Antoine Pitrou's r66134
03: adapt DummyPOP3_SSLHandler to use the handle_read() and
    the _do_tls_handshake() methods inherited from DummyPOP3Handler

As for the "no-new-features"... I know, I know... (that's the reason why I wrote that big "in my HUMBLE opionion" ;-)
msg175796 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-17 18:39
Hello,

Here are some comments about the patches:

1) poplib_01_socket_shutdown_v3.diff: looks fine to me

2) poplib_02_server_capabilities_v3.diff:

- please try to follow PEP 8 (i.e. `capa = {}` not `capa={}`)
- I think the capa() result should be a dict mapping str keys to str values (not bytes), since that part of the POP3 protocol seems to have a well-defined character set (ASCII)

3) poplib_03_starttls_v3.diff:

- same comment about PEP 8
- why did you change the signature of the _create_socket() method? it looks gratuitous
- the new method should be called starttls() as in other modules, not stls()
- starttls() should only take a context argument; no need to support separate keyfile and certfile arguments
- what is the point of catching errors like this:

+        except error_proto as _err:
+            resp = _err
msg175877 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2012-11-18 15:14
Updated 02 and 03 patches (mostly) in line with Antoine's review comments:

> 2) poplib_02_server_capabilities_v3.diff:
> - please try to follow PEP 8 (i.e. `capa = {}` not `capa={}`)
> - I think the capa() result should be a dict mapping str keys to str > values (not bytes), since that part of the POP3 protocol seems to
> have a well-defined character set (ASCII)

Completely right. Done.

> 3) poplib_03_starttls_v3.diff:
>
> - same comment about PEP 8

where?

> - why did you change the signature of the _create_socket() 
>  method? it looks gratuitous

undone

> - the new method should be called starttls() as in other modules, not >  stls()

Here I'm following: at :ref:`pop3-objects` in Doc/library/poplib.rst, 

  All POP3 commands are represented by methods of the same name, in
  lower-case; most return the response text sent by the server.

IMHO, having a single method with a name different than the underlying
POP command would just be confusing. For this reason, I'd rather avoid
adding an alias like in

    starttls = stls

or the reverse...

> - starttls() should only take a context argument; no need to support > separate keyfile and certfile arguments

Right, non need to mimick pre-SSLContext method signatures!

> - what is the point of catching errors like this:
> [...]

undone
msg176125 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-22 21:09
> where?

In `caps=self.capa()` (you need a space around the assignment operator).

> Here I'm following: at :ref:`pop3-objects` in Doc/library/poplib.rst, 

Ah, ok. Then I agree it makes sense to call it stls(). No need for an alias, IMO.
msg176164 - (view) Author: Lorenzo M. Catucci (lcatucci) * Date: 2012-11-23 11:40
OK, I'm uploading poplib_03_starttls_v5.diff; I only changed the 
"caps=self.capa()" into "caps = self.capa()" in the "@@ -352,21 +360,42 @@ class POP3:" hunk.

Thank you for pointing at the line; I tried to run pep8.py on poplib.py, but failed to find the line, since I was overwhelmed by the reported pre-existing pep8 inconsistencies...

I'm tempted to open a pep8 issue on poplib after we finish with stls...

Thank you very much for reviewing!
msg176214 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-23 19:03
Thank you Lorenzo. Your patch lacks a couple of details, such as "versionadded" and "versionchanged" tags, but I can handle this myself.
msg176216 - (view) Author: Roundup Robot (python-dev) Date: 2012-11-23 19:15
New changeset 79e33578dc05 by Antoine Pitrou in branch 'default':
Issue #4473: Ensure the socket is shutdown cleanly in POP3.close().
http://hg.python.org/cpython/rev/79e33578dc05

New changeset d30fd9834cec by Antoine Pitrou in branch 'default':
Issue #4473: Add a POP3.capa() method to query the capabilities advertised by the POP3 server.
http://hg.python.org/cpython/rev/d30fd9834cec

New changeset 2329f9198d7f by Antoine Pitrou in branch 'default':
Issue #4473: Add a POP3.stls() to switch a clear-text POP3 session into an encrypted POP3 session, on supported servers.
http://hg.python.org/cpython/rev/2329f9198d7f
msg176217 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-11-23 19:19
Patches committed. Thank you very much!
msg176296 - (view) Author: Roundup Robot (python-dev) Date: 2012-11-24 17:15
New changeset 75a146a657d9 by Antoine Pitrou in branch 'default':
Fix missing import (followup to #4473).
http://hg.python.org/cpython/rev/75a146a657d9
History
Date User Action Args
2012-11-24 17:15:19python-devsetmessages: + msg176296
2012-11-23 19:19:19pitrousetstatus: open -> closed
resolution: fixed
messages: + msg176217

stage: patch review -> resolved
2012-11-23 19:15:06python-devsetnosy: + python-dev
messages: + msg176216
2012-11-23 19:03:33pitrousetmessages: + msg176214
2012-11-23 11:41:13lcatuccisetfiles: - poplib_03_starttls_v4.diff
2012-11-23 11:40:54lcatuccisetfiles: + poplib_03_starttls_v5.diff

messages: + msg176164
2012-11-22 21:09:29pitrousetmessages: + msg176125
2012-11-18 15:14:51lcatuccisetmessages: + msg175877
2012-11-18 14:52:19lcatuccisetfiles: - poplib_03_starttls_v3.diff
2012-11-18 14:52:06lcatuccisetfiles: - poplib_02_server_capabilities_v3.diff
2012-11-18 14:51:55lcatuccisetfiles: + poplib_03_starttls_v4.diff
2012-11-18 14:51:36lcatuccisetfiles: + poplib_02_server_capabilities_v4.diff
2012-11-17 18:39:45pitrousetnosy: + pitrou
messages: + msg175796
2012-07-03 21:07:36lcatuccisetmessages: + msg164627
2012-07-03 20:51:49lcatuccisetfiles: - poplib_03_starttls.diff
2012-07-03 20:51:43lcatuccisetfiles: - poplib_02_server_capabilities.diff
2012-07-03 20:51:36lcatuccisetfiles: - poplib_01_socket_shutdown.diff
2012-07-03 20:51:17lcatuccisetfiles: + poplib_03_starttls_v3.diff
2012-07-03 20:51:07lcatuccisetfiles: + poplib_02_server_capabilities_v3.diff
2012-07-03 20:50:56lcatuccisetfiles: + poplib_01_socket_shutdown_v3.diff
2012-07-03 13:46:41jceasetmessages: + msg164601
2012-07-03 07:33:09lcatuccisetmessages: + msg164586
2012-07-03 07:26:47lcatuccisetfiles: + poplib_03_starttls.diff
2012-07-03 07:25:52lcatuccisetfiles: - poplib_03_starttls.diff
2012-07-03 07:19:05lcatuccisetfiles: + poplib_03_starttls.diff
2012-07-03 07:18:53lcatuccisetfiles: - poplib_03_starttls.diff
2012-07-03 07:18:37lcatuccisetfiles: - poplib_02_server_capabilities.diff
2012-07-03 07:18:23lcatuccisetfiles: + poplib_02_server_capabilities.diff
2012-06-24 20:13:40pitrousetversions: + Python 3.4, - Python 3.3
2012-06-24 18:36:56lcatuccisetmessages: + msg163817
2012-06-24 18:35:42lcatuccisetfiles: + poplib_03_starttls.diff
2012-06-24 18:35:26lcatuccisetfiles: + poplib_02_server_capabilities.diff
2012-06-24 18:34:55lcatuccisetfiles: + poplib_01_socket_shutdown.diff
2012-06-24 18:34:08lcatuccisetfiles: - test_popnet.py
2012-06-24 18:34:00lcatuccisetfiles: - test_popnet.py
2012-06-24 18:33:48lcatuccisetfiles: - poplib_01_CAPA.diff
2012-06-24 18:33:35lcatuccisetfiles: - poplib_04_STLS.diff
2012-06-24 18:33:20lcatuccisetfiles: - poplib_03_SSL_refactor.diff
2012-06-24 18:33:06lcatuccisetfiles: - poplib_02_sock_shutdown.diff
2012-06-24 18:32:50lcatuccisetfiles: - poplib.rst.patch
2012-06-23 03:12:54r.david.murraysetnosy: + r.david.murray, barry
components: + email
2012-06-23 01:12:41jceasetmessages: + msg163514
2011-03-14 12:09:29jceasetnosy: jcea, janssen, giampaolo.rodola, lcatucci
versions: + Python 3.3, - Python 2.7, Python 3.2
2010-03-12 19:13:38pitrousetversions: + Python 2.7, Python 3.2
nosy: + janssen

priority: normal
type: enhancement
stage: patch review
2010-03-10 01:07:59jceasetmessages: + msg100776
2009-07-30 13:24:23jceasetmessages: + msg91094
2009-07-30 13:19:56jceasetnosy: + jcea
2009-02-08 22:02:17hayposetnosy: - haypo
2009-02-07 01:04:17lcatuccisetfiles: + test_popnet.py
messages: + msg81330
2009-02-07 00:49:21lcatuccisetmessages: + msg81327
2009-02-07 00:26:20lcatuccisetcomponents: + Library (Lib)
2009-02-07 00:25:03lcatuccisetfiles: + test_popnet.py
2008-12-02 14:31:03lcatuccisetmessages: + msg76745
2008-12-02 14:26:45lcatuccisetfiles: - poplib_01_CAPA.diff
2008-12-02 14:26:25lcatuccisetfiles: + poplib_01_CAPA.diff
messages: + msg76744
2008-12-02 12:44:18hayposetmessages: + msg76735
2008-12-02 12:31:55lcatuccisetmessages: + msg76734
2008-12-02 12:29:21lcatuccisetfiles: - poplib.py.patch
2008-12-02 12:29:10lcatuccisetfiles: + poplib_04_STLS.diff
2008-12-02 12:28:52lcatuccisetfiles: + poplib_03_SSL_refactor.diff
2008-12-02 12:28:36lcatuccisetfiles: + poplib_02_sock_shutdown.diff
2008-12-02 12:28:17lcatuccisetfiles: + poplib_01_CAPA.diff
2008-12-02 00:05:08hayposetmessages: + msg76719
2008-12-01 23:50:35lcatuccisetmessages: + msg76717
2008-12-01 23:00:08hayposetnosy: + haypo
messages: + msg76713
2008-12-01 21:44:16giampaolo.rodolasetnosy: + giampaolo.rodola
2008-11-30 16:46:17lcatuccisetfiles: + poplib.rst.patch
messages: + msg76645
2008-11-30 16:39:02lcatuccicreate