POP3 missing support for starttls

[lmctv]

BPO	4473
Nosy	@warsaw, @jcea, @pitrou, @giampaolo, @lmctv, @bitdancer
Files	poplib_01_socket_shutdown_v3.diff poplib_02_server_capabilities_v4.diff poplib_03_starttls_v5.diff

^{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:

assignee = None
closed_at = <Date 2012-11-23.19:19:19.250>
created_at = <Date 2008-11-30.16:39:02.356>
labels = ['type-feature', 'library', 'expert-email']
title = 'POP3 missing support for starttls'
updated_at = <Date 2012-11-24.17:15:19.773>
user = 'https://github.com/lmctv'

bugs.python.org fields:

activity = <Date 2012-11-24.17:15:19.773>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2012-11-23.19:19:19.250>
closer = 'pitrou'
components = ['Library (Lib)', 'email']
creation = <Date 2008-11-30.16:39:02.356>
creator = 'lcatucci'
dependencies = []
files = ['26250', '28025', '28085']
hgrepos = []
issue_num = 4473
keywords = ['patch']
message_count = 26.0
messages = ['76643', '76645', '76713', '76717', '76719', '76734', '76735', '76744', '76745', '81327', '81330', '91094', '100776', '163514', '163817', '164586', '164601', '164627', '175796', '175877', '176125', '176164', '176214', '176216', '176217', '176296']
nosy_count = 8.0
nosy_names = ['barry', 'jcea', 'janssen', 'pitrou', 'giampaolo.rodola', 'lcatucci', 'r.david.murray', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue4473'
versions = ['Python 3.4']

[lmctv]

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

[lmctv]

The needed changes to documentation if the patch gets accepted

[vstinner]

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 :-)

[lmctv]

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.

[vstinner]

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

[lmctv]

I'm reasonably sure Victor is right about the original socket being
unneeded. How does the series look now?

[vstinner]

"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.

[lmctv]

Changed CAPA as requested: now there is a proper docstring, and the
useless if ... else ... is gone.

[lmctv]

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.

[lmctv]

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?

[lmctv]

I'm enclosing the expected-failure version of test_popnet. It's much
simpler to talk and comment about something you can see!

[jcea]

How is going? Any hope for 2.7/3.2?

[jcea]

Ping...

Any hope for 2.7/3.2?