add ftp-tls support to ftplib - RFC 4217

[gpshead]

BPO	2054
Nosy	@gpshead, @josiahcarlson, @orsenthil, @pitrou, @giampaolo, @merwok
Files	ftplib.diff: adds FTP over TLS support (RFC-4217) ftplib.patch: New patch which shutdown the SSL layer before closing FTP data channel ftplib.patch: includes changes described in message 94103 ftplib.patch: includes tests and doc ftplib.patch: implements changes described in msg 94225 ftplib.patch: adds SSL support as described in comment 95005 ftptls-py3k.patch ftptls-py3k-2.patch

^{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 2009-11-17.23:16:48.978>
created_at = <Date 2008-02-09.02:16:05.480>
labels = ['type-feature', 'library']
title = 'add ftp-tls support to ftplib - RFC 4217'
updated_at = <Date 2010-05-20.20:37:26.173>
user = 'https://github.com/gpshead'

bugs.python.org fields:

activity = <Date 2010-05-20.20:37:26.173>
actor = 'giampaolo.rodola'
assignee = 'none'
closed = True
closed_date = <Date 2009-11-17.23:16:48.978>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2008-02-09.02:16:05.480>
creator = 'gregory.p.smith'
dependencies = []
files = ['9471', '13242', '15147', '15159', '15163', '15343', '15349', '15351']
hgrepos = []
issue_num = 2054
keywords = ['patch']
message_count = 64.0
messages = ['62217', '62608', '62699', '63462', '63707', '64088', '64093', '64097', '64146', '64147', '64229', '64239', '64296', '66752', '68978', '69002', '69008', '69010', '69011', '69553', '71058', '71061', '82612', '82625', '82662', '82697', '82704', '82876', '83128', '94098', '94099', '94101', '94103', '94104', '94142', '94188', '94225', '94234', '94241', '94244', '94249', '94251', '94252', '94253', '94452', '94902', '94996', '95005', '95006', '95295', '95301', '95304', '95357', '95369', '95370', '95396', '95398', '95408', '96046', '96047', '102875', '102877', '102878', '102941']
nosy_count = 15.0
nosy_names = ['gregory.p.smith', 'josiahcarlson', 'janssen', 'orsenthil', 'pitrou', 'giampaolo.rodola', 'josiah.carlson', 'roberte', 'iElectric', 'eric.araujo', 'lszyba1', 'twhitema', 'jeffo', 'qwavel', 'lgedgar']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue2054'
versions = ['Python 3.2']

[gpshead]

ftplib does not support ftp over SSL / TLS as described in RFC 4217.
This would be a nice thing for someone wanting to contribute something
to add.

[giampaolo]

I've tried to work on this in the last 2 days and here is my
implementation attempt.
The patch in attachment provides a new FTP subclass which connects to
port 21 as usual leaving control and data channels implicitly unprotected.
Securing control and data channels requires user to explicitly ask for
it by using the new auth_tls() and prot_p() methods as recommended by
the standard RFC.

Usage example:

>>> from ftplib import FTP_TLS
>>> ftps = FTP_TLS('ftp.python.org')
>>> ftps.auth_tls()  # switch to secure control connection
'234 Using authentication type TLS'
>>> ftps.login()  # login anonimously
'230 Guest login ok, access restrictions apply.'
>>> ftps.prot_p()  # switch to secure data connection
'200 Protection level set to P'
>>> ftps.retrlines('LIST')  # list directory content securely
total 9
drwxr-xr-x   8 root     wheel        1024 Jan  3  1994 .
drwxr-xr-x   8 root     wheel        1024 Jan  3  1994 ..
drwxr-xr-x   2 root     wheel        1024 Jan  3  1994 bin
drwxr-xr-x   2 root     wheel        1024 Jan  3  1994 etc
d-wxrwxr-x   2 ftp      wheel        1024 Sep  5 13:43 incoming
drwxr-xr-x   2 root     wheel        1024 Nov 17  1993 lib
drwxr-xr-x   6 1094     wheel        1024 Sep 13 19:07 pub
drwxr-xr-x   3 root     wheel        1024 Jan  3  1994 usr
-rw-r--r--   1 root     root          312 Aug  1  1994 welcome.msg
'226 Transfer complete.'
>>> ftps.quit()
'221 Goodbye.'
>>>

It also provides a prot_c() method to switch back to a plain text data
channel.
Sorry in advance for any grammatical error I could have made in comments
and docstrings.

Comments are greatly appreciated.

[janssen]

This is a straightforward implementation of client-side use of SSL, but
it's missing a test case for evaluation. It should include a patch to
test_ftplib to test it.

Another thing to look at is what the useful arguments are to pass in for
TLS usage over FTP. If, for example, the client needs to validate the
server's certificate or identity, provision should be made for a file of
cacerts to be passed to the FTP_TLS instance. Passing in a keyfile and
certfile is usually only necessary when the client uses them to identify
itself to the server.

[roberte]

Damn I should have looked here earlier - so I implemented FTPS based on
ftplib myself yesterday (looked quite similar). I had a look at you
patch and got one question.
In FTP_TLS.__init__ you call FTP.__init__. The latter in turn calls
FTP.login if a username is supplied. Thus you end up trying to login
before issuing the AUTH TLS command. The result is, that username and
passwords are send unencrypted. Or do I miss a subtle trick here? I
solved that problem by wrapping FTP.connect in my subclass, essentially
calling an FTP.connect and then it would be in this case an auth_tls and
prot_p afterwards.

[iElectric]

The lib should give programmer choice wether to send login through TLS
or not. (as it is described in RFC 4217).

Also, there should be an optional parameter to specify port for ftp
connection.

[giampaolo]

This is a straightforward implementation of client-side use of SSL,
but it's missing a test case for evaluation. It should include a
patch to test_ftplib to test it.

I'm not sure how it could be tested, since we don't have an FTPS server
to test against. The current test suite itself only tests the new
timeout feature added in ftplib.FTP class in Python 2.6 and nothing else.

Another thing to look at is what the useful arguments are to pass in
for TLS usage over FTP. If, for example, the client needs to validate
the server's certificate or identity, provision should be made for a
file of cacerts to be passed to the FTP_TLS instance. Passing in a
keyfile and certfile is usually only necessary when the client uses
them to identify itself to the server.

I drew from the SSL classes defined in httplib, imaplib, poplib, smtplib
and urllib modules which accept a keyfile and a certfile in the class
constructor so I thought it was the "right way". Is there a reason why
the FTP protocol should behave differently as you have described?

In FTP_TLS.__init__ you call FTP.__init__. The latter in turn calls
FTP.login if a username is supplied. Thus you end up trying to login
before issuing the AUTH TLS command. The result is, that username and
passwords are send unencrypted. Or do I miss a subtle trick here?

You're right, I avoided doing that since the TLS encryption should be
requested specifically by the user. We could implicitly secure the
control connection if the "user" argument is provided when invoking the
class constructor and eventually add a "secure" kwarg to login method
that defaults to True.

The lib should give programmer choice wether to send login through TLS
or not. (as it is described in RFC 4217).

This is what it does if you use auth_tls() before login().

Also, there should be an optional parameter to specify port for ftp
connection.

This is already possible by using the original (inherited) connect() method.

[janssen]

As you point out, the other classes should be fixed. The old client-side
protocol was never very well thought out, IMHO. Continuing to propagate it
would be a mistake.
Bill

On Wed, Mar 19, 2008 at 12:22 PM, Giampaolo Rodola' <report@bugs.python.org>
wrote:

Giampaolo Rodola' <billiejoex@users.sourceforge.net> added the comment:
> Another thing to look at is what the useful arguments are to pass in
> for TLS usage over FTP. If, for example, the client needs to validate
> the server's certificate or identity, provision should be made for a
> file of cacerts to be passed to the FTP_TLS instance. Passing in a
> keyfile and certfile is usually only necessary when the client uses
> them to identify itself to the server.

I drew from the SSL classes defined in httplib, imaplib, poplib, smtplib
and urllib modules which accept a keyfile and a certfile in the class
constructor so I thought it was the "right way". Is there a reason why
the FTP protocol should behave differently as you have described?

[giampaolo]

As you point out, the other classes should be fixed. The old
client-side protocol was never very well thought out, IMHO.
Continuing to propagate it would be a mistake.

Ok, how do you think it would have be modified?
Could you provide an example or a new patch?

[janssen]

Probably what I should do is fix httplib, that would provide an example we
could extend to the rest of the modules.
Bill

On Wed, Mar 19, 2008 at 1:46 PM, Giampaolo Rodola' <report@bugs.python.org>
wrote:

Giampaolo Rodola' <billiejoex@users.sourceforge.net> added the comment:

> As you point out, the other classes should be fixed. The old
> client-side protocol was never very well thought out, IMHO.
> Continuing to propagate it would be a mistake.

Ok, how do you think it would have be modified?
Could you provide an example or a new patch?

---

Tracker <report@bugs.python.org>
<http://bugs.python.org/issue2054\>

---

[janssen]

Once I've got JCC working, and finished the SSL work for 2.6.

On Wed, Mar 19, 2008 at 1:46 PM, Giampaolo Rodola' <report@bugs.python.org>
wrote:

Giampaolo Rodola' <billiejoex@users.sourceforge.net> added the comment:

> As you point out, the other classes should be fixed. The old
> client-side protocol was never very well thought out, IMHO.
> Continuing to propagate it would be a mistake.

Ok, how do you think it would have be modified?
Could you provide an example or a new patch?

---

Tracker <report@bugs.python.org>
<http://bugs.python.org/issue2054\>

---

[pitrou]

FWIW, m2crypto already provides an FTP-TLS facility with an
ftplib-compatible API. See http://chandlerproject.org/Projects/MeTooCrypto

[roberte]

> FWIW, m2crypto already provides an FTP-TLS facility with an
> ftplib-compatible API. See
http://chandlerproject.org/Projects/MeTooCrypto

Right but m2crypto is not part of the standard library (and is not going
to be anytime soon). SSL is scheduled to be in 2.6. So it would be the
natural choice to use that SSL binding to extend the ftp library. Of
course we could have a look how ftps is implemented in m2crypto.

Concerning the plain-text login. I think a FTPS class should default to
encrypted login (you could use the ftp class if you dont want). In no
way should the login credentials be sent unencrypted on default. Using
another parameter might be a soulution to that, though I would prefer
the library to raise an error if establishing an FTPS connection did not
succeed. The main program could then catch it and decide how to proceed
(using plain ftp or aborting according to a given policy).

[janssen]

On Fri, Mar 21, 2008 at 5:43 AM, Robert E. <report@bugs.python.org> wrote:

Robert E. <robert@re-factory.de> added the comment:

Concerning the plain-text login. I think a FTPS class should default to
encrypted login (you could use the ftp class if you dont want). In no
way should the login credentials be sent unencrypted on default. Using
another parameter might be a soulution to that, though I would prefer
the library to raise an error if establishing an FTPS connection did not
succeed. The main program could then catch it and decide how to proceed
(using plain ftp or aborting according to a given policy).

Sounds reasonable to me.

Note that FTP is an old and somewhat gnarly protocol, and
doesn't work the way more recent application protocols do. The SSL
module is designed for TCP-based single-connection call-response
protocols, more or less. Doing FTPS right might mean we'd have to
extend it.

[giampaolo]

Bill, are there news about the fix to httplib?
I'd like to see this feature included in 2.6 if possible.

[janssen]

The 2.6/3.0 changes are now up-to-date. We could reconsider this
problem. My guess is that we still don't quite know what to do.

I think the issue is that we need a way to "unwrap" the SSL-secured
TCP stream, after it's been used. So we need to expose the SSL
shutdown mechanism (already in the _ssl.c module) in the Python
code. Something like

   socket = self.unwrap()

which would return a plain socket.socket instance.

[giampaolo]

Yes, I think that providing an "unwrap" method for the ssl module would
be good, independently from this issue.
With that implemented and httplib fixed in the way you were mentioning
in this same report I can go on with modifying the ftplib patch.

[janssen]

But httplib is far from fixed. It's a nasty tarball of interdependencies...

Bill

On Mon, Jun 30, 2008 at 4:12 AM, Giampaolo Rodola' <report@bugs.python.org>
wrote:

Giampaolo Rodola' <billiejoex@users.sourceforge.net> added the comment:

Yes, I think that providing an "unwrap" method for the ssl module would
be good, independently from this issue.
With that implemented and httplib fixed in the way you were mentioning
in this same report I can go on with modifying the ftplib patch.

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue2054\>

---

[giampaolo]

Ok, so let's leave httplib alone. Let's just add the unwrap() method to
ssl.SSLSocket so that it could be be possible to go ahead with the
current patch.
Modifying it consists in just adding a new prot_c method (I could do that).
As for the cacerts needed to be passed to FTP_TLS constructor you could
do that afterwards.

[giampaolo]

Could what I've just said be an idea?

[lszyba1]

Is the ftp-tls able to use certificate to connect to ftps server?
I currently need to connect to state's ftps server which requires
certificate to be present when authenticating.

Is that option available? What is the current status of this issue?

Thanks,
Lucas

[janssen]

I think I'm just going to bring the unwrap already in the _ssl.c code
out to the ssl.py module, that seems to be the simplest fix. Still not
sure you can do a proper fix to ftplib here, but that seems to be a good
thing to do anyway, rather than having people call directly into the
_ssl module to get at it.

[janssen]

OK, I think I've done the minimal fix necessary to the SSL module to
allow this work to proceed.

[jeffo]

Just wondering, has anyone done a patch since Bill made the necessary
changes to ssl.py in order to implement FTP TLS? If so, where can I find
it? I would love to test it out.

[giampaolo]

After Bill added SSL's unwrap() method I modified my previous patch so
that it shutdown the SSL layer before closing the data connection.
I successfully tested it against proftpd, vsftpd and pyftpdlib TLS
server [1].

If some python developer could give me an ok on the patch I could start
writing tests and documentation.

[1] http://code.google.com/p/pyftpdlib/source/browse/trunk/demo/tls_ftpd.py

[jeffo]

Thank you Giampaolo, it works just as I was hoping, =] I tested it on glftpd
using python 2.6.1.

[jeffo]

Actually I have encountered a possible bug. the close() method doesn't seem
to actually close the connection...

On Mon, Feb 23, 2009 at 11:56 PM, Jeff Oyama <report@bugs.python.org> wrote:

Jeff Oyama <jeff@oyama.org> added the comment:

Thank you Giampaolo, it works just as I was hoping, =] I tested it on
glftpd
using python 2.6.1.

Added file: http://bugs.python.org/file13161/unnamed

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue2054\>

---

[giampaolo]

Actually I have encountered a possible bug. the close()
method doesn't seem to actually close the connection...

Why? What happens exactly?

[pitrou]

The patch is ok to me. Perhaps Bill wants to take a look, otherwise I
think you can commit.

[pitrou]

A last problem:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: attribute name must be string, not 'classobj'

[giampaolo]

> A last problem:
> 
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> TypeError: attribute name must be string, not 'classobj'

Mmmm this doesn't say much.
When does it happen?
Is that the complete traceback message?

The patch is ok to me. Perhaps Bill wants to take a look.

I'd like the opinion of Bill too, specifically about what he was talking
about in comments 62699 and 64093.

otherwise I think you can commit.

I don't have commit privileges. Someone else should do it.

[pitrou]

Ah, sorry, roundup's e-mail interface ate part of the message.
The error happens when doing "from ftplib import *". Apparently __all__
contains a non-string value.

I don't have commit privileges. Someone else should do it.

Ok, I'll do it if Bill doesn't give a sign of life.

[pitrou]

Regarding msg64093, the only API change Bill's suggestion would entail
is an additional optional parameter to the constructor, so adding it
later would be backwards-compatible.

[giampaolo]

Ah, sorry, roundup's e-mail interface ate part of the message.
The error happens when doing "from ftplib import *". Apparently
__all__
contains a non-string value.

Oh, shame on me! You're right.
Thanks for the great review you're doing.

Regarding msg64093, the only API change Bill's suggestion would entail
is an additional optional parameter to the constructor, so adding it
later would be backwards-compatible.

Good.
Possibly there might be another change we might want to do in future,
which is adding support for CCC command, useful for reverting the
control connection back to clear-text.
I remember I tried to do that at the time I submitted the first patch
but there was a problem with ssl's unwrap() method (I really don't
remember what exactly).

Anyway, I'll try to put hands on that soon and let you know what
happens.
The change should be backward compatible as it just consists in adding a
ccc() method.

[iElectric]

What about AUTH SSL? Or is it too-deprecated?

[pitrou]

I noticed you were using ftp.python.org in the example strings, but that
service doesn't seem to be alive. I don't know if there's another public
FTP-TLS server you could rely on...?

[iElectric]

I've tested TLS with several private servers today, seems to work. I
cannot test patch against FTP SSL encryption, although it is obviously
not implemented

[giampaolo]

Sorry for delay in the response. The latest messages slipped under my
radar.

What about AUTH SSL? Or is it too-deprecated?

I'm not sure about this.
TLS is certainly preferred over SSL and RFC-4217 only refers to TLS
protocol, altough SSL is mentioned in some chapters.

RFC-4217 states:

As the SSL/TLS protocols self-negotiate their levels, there is no
need to distinguish between SSL and TLS in the application layer.
The mechanism name for negotiating TLS is the character string
identified in {TLS-PARM}.

[...]

{TLS-PARM} - The parameter for the AUTH command to indicate that TLS
is required. To request the TLS protocol in accordance with this
document, the client MUST use 'TLS'

If we want to support SSL we could change the current implementation by
renaming "auth_tls()" method to just "auth" and play with the
ssl_version attribute, like this:

class FTP_TLS(FTP):
    ssl_version = ssl.PROTOCOL_TLSv1

    def auth(self):
        if self.ssl_version == ssl.PROTOCOL_TLSv1:
            resp = self.voidcmd('AUTH TLS')
        else:
            resp = self.voidcmd('AUTH SSL')
        ...

The user willing to use SSL instead of TLS will have to change
ssl_version class attribute with "FTP_TLS.ssl_version =
ssl.PROTOCOL_TLSv1" and then call auth().

Deciding whether rejecting or accepting it will be up to the server
depending on how it has been configured (almost all recent FTP servers
reject SSLv2).

I noticed you were using ftp.python.org in the example strings, but
that service doesn't seem to be alive. I don't know if there's another
public FTP-TLS server you could rely on...?

Yeah, I know. I just copied from original FTP class docstring.
As of now I'm not aware of any public FTPS server we could use.

[giampaolo]

The user willing to use SSL instead of TLS will have to change
ssl_version class attribute with "FTP_TLS.ssl_version =
ssl.PROTOCOL_TLSv1" and then call auth().

Sorry but here I obviously meant "ssl.PROTOCOL_SSLv2/3"

[pitrou]

Giampaolo, do you plan to add something or is the patch ok to commit?

[giampaolo]

If we want to add SSL support then the patch in attachment modifies the
last one as I described in my previous comment.
I re-run the tests and they are ok so I guess you can go on with the
commit.

[pitrou]

The tests don't work under py3k, for some reason I can't figure out.
There's the following error and then the tests hang:

test_acct (test.test_ftplib.TestTLS_FTPClassMixin) ... Exception in
thread Thread-31:
Traceback (most recent call last):
  File "/home/antoine/py3k/__svn__/Lib/threading.py", line 521, in
_bootstrap_inner
    self.run()
  File "/home/antoine/py3k/__svn__/Lib/test/test_ftplib.py", line 214,
in run
    asyncore.loop(timeout=0.1, count=1)
  File "/home/antoine/py3k/__svn__/Lib/asyncore.py", line 210, in loop
    poll_fun(timeout, map)
  File "/home/antoine/py3k/__svn__/Lib/asyncore.py", line 136, in poll
    r, w, e = select.select(r, w, e, timeout)
select.error: (9, 'Bad file descriptor')

[giampaolo]

Can you attach the 3.x patch so that I can test it myself?
I tried to apply the current 2.x patch against the 3.x trunk but I get
conflicts.

[pitrou]

Here is the current py3k patch I have, after resolving conflicts and
cleaning up the obvious problems.
After tracing a bit, it seems that ssl.wrap_socket() changes the socket
fileno under py3k, while it doesn't under trunk.

[pitrou]

Ok, I now have a working patch. The main fix was to change
SSLConnection.secure_connection() to:

        def secure_connection(self):
            socket = ssl.wrap_socket([ ##etc. ])
            self.del_channel()
            self.set_socket(socket)
            self._ssl_accepting = True

Can you take a look?

[giampaolo]

Ok, I took a look and it seems ok to me but I still get some occasional
failures on Windows from time to time.
Because of the threading nature of our server I suspect that moving
del_channel() before ssl.wrap_socket() call, like this:

•        socket = ssl.wrap_socket([ ##etc. ])
  

•        self.del_channel()
  

•        self.set_socket(socket)
  

+ self.del_channel()
+ self.socket = ssl.wrap_socket(...)
+ self.set_socket(self.socket)

...makes more sense (ps: pay attention, it's "self.socket", not
"socket").
After I did that I stopped seeing the occasional failures (I'm not 100%
sure it's actually related, but...).

This is quite strange, anyway.
I suspect it has something to do with this:
http://entitycrisis.blogspot.com/2009/11/python-3-is-it-doomed.html

[pitrou]

Ok, I took a look and it seems ok to me but I still get some occasional
failures on Windows from time to time.
Because of the threading nature of our server I suspect that moving
del_channel() before ssl.wrap_socket() call, like this:

Ok, thanks!

...makes more sense (ps: pay attention, it's "self.socket", not
"socket").

set_socket() sets self.socket, so it should be the same.

I'm going to commit on py3k and watch the buildbots a bit.

[pitrou]

Buildbots are ok. Thank you!

[iElectric]

Nice! Any chance of merging with 2.7? Python3.2 is waaay too far in
future for such useful change to be actually useful.

[pitrou]

It's already in 2.7.

[giampaolo]

Thinking back about this, I wonder whether "FTPS" could be a better name to use instead of "FTP_TLS".
It's shorter, easier to remember, and also makes more sense since also SSL can be used, not only TLS.

[merwok]

It doesn’t look like a constant, too.

httplib.Client, ftplib.Client, ftplib.SecureClient would be much more descriptive than httplib.HTTP and ftplib.FTP. Any interest about adding aliases?

Regards

[pitrou]

Thinking back about this, I wonder whether "FTPS" could be a better name to use instead of "FTP_TLS".
It's shorter, easier to remember, and also makes more sense since also SSL can be used, not only TLS.

What do you mean by "also SSL can be used"?
Wikipedia has an interesting article about the subject:
http://en.wikipedia.org/wiki/FTPS

Secured FTP with explicit negotiation (what we are doing) is sometimes
called "FTPES" (that's how it's named in FileZilla indeed).
"FTPS" is more often used to describe secured FTP with implicit
negotiation, i.e. the SSL session is established before the FTP protocol
even kicks in.

I think FTP_TLS is a fine name. Perhaps we can simply make the above
distinction clearer in the docs.

[orsenthil]

On Sun, Apr 11, 2010 at 07:43:56PM +0000, Éric Araujo wrote:

httplib.Client, ftplib.Client, ftplib.SecureClient would be much more descriptive than httplib.HTTP and ftplib.FTP. Any interest about adding aliases?

Aliases would be a bad idea. -1. It is fine the way the issue is
moving forth now, FTP_TLS.