Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ftplib: Add client-side SSL session resumption #63699

Open
YeWang mannequin opened this issue Nov 5, 2013 · 34 comments
Open

ftplib: Add client-side SSL session resumption #63699

YeWang mannequin opened this issue Nov 5, 2013 · 34 comments
Labels
3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@YeWang
Copy link
Mannequin

YeWang mannequin commented Nov 5, 2013

BPO 19500
Nosy @pitrou, @giampaolo, @tiran, @alex, @dstufft, @zhangyangyu, @rreilink, @larsschellhas
Files
  • ftplib-FTPS-bug.txt: Python source and command prompt input/output
  • reuse_session.diff: proof-of-concept session reuse
  • implement_ssl_session_reuse.patch: Patch which implements SSL session reuse and fixes corresponding FTP_TLS issue
  • implement_ssl_session_reuse_3.6.patch: Patch which implements SSL session reuse and fixes corresponding FTP_TLS issue targeted at 3.6
  • SSLSession-support.patch
  • sesstime.py
  • SSLSession-support-2.patch
  • SSL-client-side-SSL-session-resumption.patch
  • ftplib_session.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 = None
    created_at = <Date 2013-11-05.04:02:10.597>
    labels = ['type-feature', 'library', '3.10']
    title = 'ftplib: Add client-side SSL session resumption'
    updated_at = <Date 2020-09-23.19:34:39.319>
    user = 'https://bugs.python.org/YeWang'

    bugs.python.org fields:

    activity = <Date 2020-09-23.19:34:39.319>
    actor = 'larsschellhas'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Library (Lib)']
    creation = <Date 2013-11-05.04:02:10.597>
    creator = 'Ye.Wang'
    dependencies = []
    files = ['32505', '40666', '40708', '40716', '44362', '44368', '44376', '44480', '46409']
    hgrepos = []
    issue_num = 19500
    keywords = ['patch']
    message_count = 27.0
    messages = ['202193', '216674', '216679', '216680', '225078', '252205', '252467', '252468', '252527', '252530', '252543', '252635', '274367', '274391', '274394', '274396', '274417', '274893', '275161', '275703', '275704', '286190', '312885', '370001', '377412', '377413', '377419']
    nosy_count = 13.0
    nosy_names = ['janssen', 'pitrou', 'giampaolo.rodola', 'christian.heimes', 'alex', 'python-dev', 'dstufft', 'Ye.Wang', 'Mark.Ribau', 'xiang.zhang', 'robr', 'Florian Wickert', 'larsschellhas']
    pr_nums = []
    priority = 'low'
    resolution = None
    stage = 'needs patch'
    status = 'open'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue19500'
    versions = ['Python 3.10']

    @YeWang
    Copy link
    Mannequin Author

    YeWang mannequin commented Nov 5, 2013

    According to RFC4217 (Securing FTP with TLS, aka the FTPS spec),

    http://tools.ietf.org/html/rfc4217.html#section-10.2

    " It is reasonable for the server to insist that the data connection
    uses a TLS cached session. This might be a cache of a previous data
    connection or of a cleared control connection. If this is the reason
    for the refusal to allow the data transfer, then the '522' reply
    should indicate this.

    Note: This has an important impact on client design, but allows
    servers to minimize the cycles used during TLS negotiation by
    refusing to perform a full negotiation with a previously
    authenticated client."

    It appears that vsftpd server implemented exactly that by enforcing the "SSL session reuse between the control and data connection".

    http://scarybeastsecurity.blogspot.com/2009/02/vsftpd-210-released.html

    Looking at the source of Python core library ftplib.py, there isn't any regard to the idea of SSL session reuse between data connection vs. control connection (correct me if I am wrong here. I've tried FTP_TLS.transfercmd(cmd[, rest])¶, didn't work).

    This issue is well documented on other FTP clients that supports FTPS, I.E. WinSCP: http://winscp.net/tracker/show_bug.cgi?id=668

    See test log file attached. A vsftpd server with "require_ssl_reuse" set to true in vsftpd.conf would do the trick and can be reproduced.

    @YeWang YeWang mannequin added type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels Nov 5, 2013
    @giampaolo
    Copy link
    Contributor

    Interesting, I wasn't aware of this FTP(S) feature.
    Unfortunately RFC-4217 really doesn't say much about how this should be done but it definitively looks like something worth having.
    AFAIU this looks like something which should be implemented by servers though, not clients.

    @pitrou
    Copy link
    Member

    pitrou commented Apr 17, 2014

    Yuck. Is there a public FTP server available somewhere with this "feature"?

    @pitrou
    Copy link
    Member

    pitrou commented Apr 17, 2014

    The RFC is unhelpfully lousy. It's not enough to process a "522" error, since that can be triggered for different reasons. You also somehow have to interpret the error text to detect that session reuse is indeed mandated by the server.

    Regardless, to progress with this we would first need to implement client-side SSL session reuse, which necessitates a bunch of additional APIs (since which session is to be reused is a decision made by user code), and a new opaque type to carry SSL_SESSION objects...

    (see issue bpo-8106)

    @MarkRibau
    Copy link
    Mannequin

    MarkRibau mannequin commented Aug 8, 2014

    Adding Python v2.7 as also exhibiting this behavior.

    Some people over on Stack Overflow have done some things to work around the issue via subclassing, but I'm not sure their solutions are "correct", so much as have useful side effects. (For example, when only the server has a key/cert and the client does not, how is that handled for reuse?)

    http://stackoverflow.com/questions/12164470/python-ftp-tls-connection-issue

    @AlexWarhawk
    Copy link
    Mannequin

    AlexWarhawk mannequin commented Oct 3, 2015

    I encountered this problem recently and could not find a fix, so i tried fixing it myself.

    Note that the patch attached is my first contribution to cpython as well as the first time I used the C extension mechanism. Therefore I do not consider the patch polished enough to be just merged upstream.

    Maybe it helps in solving this issue.

    The attached patch is based on:
    changeset: 79113:ec373d762213
    branch: 2.7

    @AlexWarhawk
    Copy link
    Mannequin

    AlexWarhawk mannequin commented Oct 7, 2015

    Based on the proof-of-concept patch I submitted a few days ago I have built a more sophisticated patch. Please review it and let me know about necessary changes.

    @giampaolo
    Copy link
    Contributor

    This is supposed to be a new feature hence the patch should be targeted against Python 3.6, definitively not 2.7.

    @AlexWarhawk
    Copy link
    Mannequin

    AlexWarhawk mannequin commented Oct 8, 2015

    I have re-targeted the patch for 3.6. It is not a 1 to 1 port of the prior one, but quite similar.

    @tiran
    Copy link
    Member

    tiran commented Oct 8, 2015

    Thanks for your patch. There might be a simpler way. By default a SSLContext only caches server sessions. You can enable client session caching with:

      SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_CLIENT)

    This may be sufficient for FTP over TLS since both sockets are created from the same context.

    The new patch has a flaw. With the new SSLSession object a user could attempt to reuse a SSLSession with a different SSLContext. That's going to break OpenSSL.

    From SSL_set_session(3)

    NOTES
    SSL_SESSION objects keep internal link information about the session cache list, when being inserted into one SSL_CTX object's session cache. One SSL_SESSION object, regardless of its reference count, must therefore only be used with one SSL_CTX object (and the SSL objects created from this SSL_CTX object).

    @AlexWarhawk
    Copy link
    Mannequin

    AlexWarhawk mannequin commented Oct 8, 2015

    Thanks for the heads up Christian I'll try enabling client session caching. If this does not work I'll try to adapt the patch to only allow session reusing within the same context.

    @AlexWarhawk
    Copy link
    Mannequin

    AlexWarhawk mannequin commented Oct 9, 2015

    Even after enabling client cache one still has to call SSL_set_session. See documentation of SSL_CTX_set_session_cache_mode point SSL_SESS_CACHE_CLIENT.

    I started thinking about not exposing a SSL_SESSION object to the user but rather extending wrap_socket to take an already established socket as argument and use that socket's session object. This way I can ensure that both sockets share the same SSL context

    I am not really convinced by this idea myself, what do you think about this? Any better ideas?

    @tiran
    Copy link
    Member

    tiran commented Sep 4, 2016

    Here is my take on the SSLSession feature. The patch provides a SSLSession type, SSLSocket.session getter/setter and SSLSocket.session_reused getter. The setter makes sure that the session can only set for client sockets from the same SSLContext and before handshake. Tests and documentation need some improvements.

    https://github.com/tiran/cpython/commits/feature/openssl_session

    @tiran tiran changed the title Error when connecting to FTPS servers not supporting SSL session resuming Add client-side SSL session resumption Sep 4, 2016
    @tiran tiran added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Sep 4, 2016
    @tiran
    Copy link
    Member

    tiran commented Sep 5, 2016

    Performance improvements with session resumption is quite noticeable. I see between 17 and 21% improvement for 10 requests GET http://pypi.python.org/pypi in a simple benchmark:

    session resumption: 1.264sec
    no session: 1.535sec
    82.4%

    @zhangyangyu
    Copy link
    Member

    Patch LGTM. But one thing is that every time it returns a new instance of SSL.Session. That means ssl_sock.session == ssl_sock.session will always return False right now. Is it useful to make it comparable?

    @tiran
    Copy link
    Member

    tiran commented Sep 5, 2016

    Xiang, good point! I have added richcompare to SSLSession (based on session id). My branch on github implements a couple more fixes and improvements.

    @tiran
    Copy link
    Member

    tiran commented Sep 5, 2016

    Note to future me:
    Don't forget to take care of X.509 client authentication. A server is allowed to bypass client cert validation when a SSL session is resumed. SSLContext.load_cert_chain() should invalidate session caches. (CVE-2016-5419 https://curl.haxx.se/docs/adv_20160803A.html)

    @tiran
    Copy link
    Member

    tiran commented Sep 7, 2016

    Session resumption is currently broken in OpenSSL 1.1.0, openssl/openssl#1550

    @tiran
    Copy link
    Member

    tiran commented Sep 8, 2016

    This patch implements a workaround for OpenSSL 1.1.0.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 10, 2016

    New changeset 6f2644738876 by Christian Heimes in branch 'default':
    Issue bpo-19500: Add client-side SSL session resumption to the ssl module.
    https://hg.python.org/cpython/rev/6f2644738876

    @tiran
    Copy link
    Member

    tiran commented Sep 10, 2016

    I have committed the feature with rudimentary documentation. I will provide more documentation and an example before 3.6.0b2.

    @tiran tiran added the docs Documentation in the Doc dir label Sep 10, 2016
    @tiran tiran self-assigned this Sep 10, 2016
    @rreilink
    Copy link
    Mannequin

    rreilink mannequin commented Jan 24, 2017

    With this code in place, ftplib should / could also be updated to support session resumption. This would fix bugs with connections to FTP servers that require session resumption [1], [2]

    In ftplib.FTP_TLS.ntransfercmd, just add a reference to the current session in the wrap_socket call (maybe make this an option to do session resumption or not; I don't know if it could break something)

    Proposed patch is attached.

    [1] http://stackoverflow.com/questions/14659154/ftpes-session-reuse-required
    [2] https://forum.filezilla-project.org/viewtopic.php?t=36903

    @tiran
    Copy link
    Member

    tiran commented Feb 26, 2018

    It's now an ftplib issue.

    It's too late to land a new feature in 3.7 because we have reached feature freeze.

    @tiran tiran added 3.8 only security fixes and removed docs Documentation in the Doc dir topic-SSL labels Feb 26, 2018
    @tiran tiran changed the title Add client-side SSL session resumption ftplib: Add client-side SSL session resumption Feb 26, 2018
    @tiran tiran removed their assignment Feb 26, 2018
    @FlorianWickert
    Copy link
    Mannequin

    FlorianWickert mannequin commented May 26, 2020

    Using vsftpd 3.0.3 with ssl_enable=YES and require_ssl_reuse=YES I get the following error when I try to upload a file with conn.storbinary():

    FTP command: Client "192.168.178.115", "STOR something.bin"
    FTP response: Client "192.168.178.115", "150 Ok to send data."
    FTP response: Client "x.x.x.x", "150 Ok to send data."
    DEBUG: Client "x.x.x.x", "DATA connection terminated without SSL shutdown. Buggy client! Integrity of upload cannot be asserted."
    FTP response: Client "x.x.x.x", "426 Failure reading network stream."

    This also happens when SSL reuse is disabled on the server side.
    Uploading with FileZilla works and there is basically no difference in the log until the error happens.

    @larsschellhas
    Copy link
    Mannequin

    larsschellhas mannequin commented Sep 23, 2020

    Excuse me, but why is this issue still open and unfixed? There are already proposed fixes and this issue has been around for nearly 7 years now.
    Filezilla has forwarded the responsibility to us for this issue (https://trac.filezilla-project.org/ticket/10700), so please, let's just fix it.

    @tiran
    Copy link
    Member

    tiran commented Sep 23, 2020

    I have provided necessary infrastructure for TLS 1.2 session resumption in the ssl module a couple of releases ago. But nobody has taken ownership of this ftplib issue and provided a full fix with regression tests and documentation update.

    If you like to see ftplib enhanced to support session resumption, please provide a PR with regression tests. Demanding a fix won't accelerate work on the issue.

    @tiran tiran added 3.10 only security fixes and removed 3.8 only security fixes labels Sep 23, 2020
    @larsschellhas
    Copy link
    Mannequin

    larsschellhas mannequin commented Sep 23, 2020

    @christian Heimes, you are absolutely right. I'm sorry if I came off rude.

    Actually, I just had a rough day at work with the end of it being the realisation that this missing fix would solve my day-filling issue from today.

    Furthermore, I've actually returned here, just to read through the whole thread again, hoping to find a way to contribute to it's resolution.
    However, this is my first time contributing to Python, but I'm eager to learn and dive into the necessary resources.
    Could you give me a hint about where to start?

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @athish-t
    Copy link

    athish-t commented Oct 6, 2022

    Hi! Do we know if anyone is working on this? If not, maybe I can make this my first contribution to Python.

    @gbrault
    Copy link

    gbrault commented Sep 15, 2023

    @vstinner : could have a look and tell me if this option is already present or what I am doing wrong? Or just tell me if actually ftplib needs a fix like the one I am proposing? Thx in advance.


    @gvanrossum who should be the right guy to handle this fix? Please let me know for follow-up.

    I don't know if this is related, but with the latest Filezilla serveur, it is not anymore possible to connect with explicit TLS using FTP_TLS. We get

    *get* '425 Unable to build data connection: TLS session of data connection not resumed.\n'
    

    Using this code makes a work around

    import ftplib
    from ssl import SSLSocket
    
    #https://stackoverflow.com/questions/14659154/ftps-with-python-ftplib-session-reuse-required
    class ReusedSslSocket(SSLSocket):
        def unwrap(self):
            pass
     
    #MyFTP_TLS is derieved to support TLS_RESUME(filezilla server)
    class MyFTP_TLS(ftplib.FTP_TLS):
        """Explicit FTPS, with shared TLS session"""
     
        def ntransfercmd(self, cmd, rest=None):
            conn, size =ftplib.FTP.ntransfercmd(self,cmd, rest)
            if self._prot_p:
                conn = self.context.wrap_socket(conn,
                                                server_hostname=self.host,
                                                session=self.sock.session)  # reuses TLS session     
     
                conn.__class__ = ReusedSslSocket  # we should not close reused ssl socket when file transfers finish
     
            return conn,size
    
    ftp_host='your host IP or domain'
    ftp_port=21
    userid = 'username'
    password = 'password'
     
    # ftps = ftplib.FTP_TLS(host=ftp_host,timeout=15)
    ftps = MyFTP_TLS(host=ftp_host,timeout=15)
    ftps.set_debuglevel(2)
    ftps.auth()
    ftps.port=ftp_port
    ftps.login(userid, password)
     
    ftps.prot_p()
    ftps.set_pasv(1)
     
    print(ftps.welcome)
    print(ftps.cwd("/"))
    print(ftps.retrlines('LIST'))
    • I am not sure it is fully 'compliant' but it looks like field proven
    • I wonder why such a simple workaround is not implemented in the ftplib?
    • It could be a parameter in instance creation: (..., reuse=True), not to interfere with other implementation
    • defenitively, the latest Filezilla 1.7.3 will not work without it

    Of course, we could considere ftp protols as outdated, but they are still widely spread and TLS is paramount in this context.
    I have no experience patching the core Python, so I let people who have this skill investigate and implement this fix.

    @adiroiban
    Copy link

    adiroiban commented Sep 15, 2023

    @gbrault thanks for your input

    I wonder why such a simple workaround is not implemented in the ftplib?

    I guess that nobody created a PR request so far, with the fix and the automated tests.

    I think that when this ticket was first raised, in 2013, the ssl module didn't have support to set the session.

    Since 2016 / Python 3.6 the TLS session of a connection is now available. Done by Christian Heimes.


    It's important to test the behavior with both TLS 1.2(maybe 1.1) and TLS 1.3.

    More documentation about session changes between TLS 1.2 and TLS 1.3 can be found here https://www.openssl.org/docs/manmaster/man3/SSL_get_session.html


    We should also consider using SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_CLIENT)

    I think this can fix the TLS v1.2 part.

    But we need automated tests to demonstrate it.


    I am also interested in getting this implemented.

    I think the first step is to agree on the API.
    I think that passing it as an extra argument for __init__ is the best option.

    It can also be passed as a parameter to auth() but for Implicit FTPS, we don't have the auth command.

    We are left with deciding the name of the parameter.
    My suggestion is to call it ssl_reuse=True

    In vsftpd.conf the server-side option is called require_ssl_reuse

    We can argue about ssl vs tls so maybe session_reuse is better :)


    Happy to help with code review or writing the automated tests.

    @Neustradamus
    Copy link

    To follow this ticket

    @mhammack
    Copy link

    Don't know if this is related, I'm getting "425 Unable to open data connection" which might be and explicit/implicit thing. Using FTL_TLS, the login works fine but just trying to list the current directory gets the error.

    Following.

    @Pyroluk
    Copy link

    Pyroluk commented Apr 18, 2024

    @gbrault The provided code worked fine for a long time, but is now incompatible with the current FileZilla Server version 1.8.1 .

    STOR XXX.yaml XXX.yaml
    Results in the following error message on server and client:
    425 Error while transfering data: ECONNABORTED - Connection aborted

    FTL_TLS is now completely unusable with filezilla server, as they removed the option to disable the need for session resumption for security reasons after version 1.0 .

    Ftputil does support it, but crashes during large file transfers.

    I am losing my mind over this. Is Python the only language without actually working FTPS libraries?

    @sschwarzer
    Copy link

    Ftputil does support it, but crashes during large file transfers.

    ftputil uses ftplib under the hood, so I'm not sure if/how this ticket here is related to ftputil vs. ftplib. Just FYI, there's now a "corresponding" ftputil ticket. It's not yet clear to me whether the ftputil ticket is actually TLS-related.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests