upgrade_to_tls does not work correctly

[astrand]

Kind of follow up to #184. Explicit FTPS works now, however:

• Not possible to use the context API, since there is no way to call updgrade_to_tls() in between client.connect() and client.login().
• I was not able to download from pyftpdlib until I used this construction:

await client.upgrade_to_tls(self.ssl_context)
client._upgraded_to_tls = True # pylint: disable=protected-access
client._open_connection = functools.partial(asyncio.open_connection, ssl=client.ssl) # pylint: disable=protected-access

AFAIK, _upgraded_to_tls is never set in the actual code, and _open_connection is incorrect after update since it is not update with new ssl instance.

[pohmelie]

Thank you for the report. Will try to check this out in couple days.

[astrand]

Thanks. For testing, I am using this FTP server:

#!/usr/bin/env python3
"""FTPS server which requires explicit TLS"""
import os
import sys
from pyftpdlib.authorizers import DummyAuthorizer
from pyftpdlib.handlers import TLS_FTPHandler
from pyftpdlib.servers import FTPServer
def main():
    "Main"
    authorizer = DummyAuthorizer()
    authorizer.add_anonymous(".", perm="elradfmwMT")
    handler = TLS_FTPHandler
    handler.authorizer = authorizer
    handler.certfile = os.path.dirname(__file__) + "my.crt"
    handler.tls_control_required = True
    handler.tls_data_required = True
    server = FTPServer(("127.0.0.1", int(sys.argv[1])), handler)
    server.serve_forever()
if __name__ == "__main__":
    main()

[pohmelie]

Sorry for a delay. I think I fixed everything locally, but I realised that SSLSessionBoundContext is not used at all after my fixes, but a have a success with pyftpdlib with this:
Generate certificates:

openssl req -x509 -newkey rsa:4096 -sha256 -days 3650 \
  -nodes -keyout server.key -out server.cert -subj "/CN=localhost" \
  -addext "subjectAltName=DNS:localhost,DNS:*.localhost,IP:127.0.0.1" \

Server code:

from pathlib import Path
import sys
from pyftpdlib.authorizers import DummyAuthorizer
from pyftpdlib.handlers import TLS_FTPHandler
from pyftpdlib.servers import FTPServer
def main():
    authorizer = DummyAuthorizer()
    authorizer.add_anonymous(".", perm="elradfmwMT")
    handler = TLS_FTPHandler
    handler.authorizer = authorizer
    root = Path(__file__).parent
    handler.certfile = str(root / "server.cert")
    handler.keyfile = str(root / "server.key")
    handler.tls_control_required = True
    handler.tls_data_required = True
    server = FTPServer(("127.0.0.1", int(sys.argv[1])), handler)
    server.serve_forever()
if __name__ == "__main__":
    main()

python pyftpdlib-server.py 4000

Client code:

import asyncio
from aioftp import Client


async def main():
    c = Client()
    await c.connect("127.0.0.1", 4000)
    await c.upgrade_to_tls()
    await c.login()
    print(await c.list())
    await c.quit()


asyncio.run(main())

SSL_CERT_FILE=server.cert python pyftpdlib-client.py

And after that I actually do not get why we need at all this SSLSessionBoundContext?

@sammichaels @bellini666 need your assistance

[bellini666]

@pohmelie the solution I ported from the fork is working for me. I recently updated it in my codebase, but I had to do one adjustment which I was going to create an MR for (didn't have time yet)

In my situation, for the 3rd party server which I'm connecting, I had to call _send_tls_protection_commands() right after upgrade_to_tls(), but before login(), i.e.

c = Client()
await c.connect("127.0.0.1", 4000)
await c.upgrade_to_tls()
await c._send_tls_protection_commands()
await c.login()
...  # now this works

Maybe it should be called right after upgrade_to_tls(), without having to wait for the login? Like, without that it doesn't work for me, but I don't have control over the server which I'm connecting, so I'm not sure if those should still be separated commands. (then we would need to add a public API for it)

[sammichaels]

Hi.

The need for this is in the docstring. Some, if not most, FTP servers require TLS session reuse, but at the time of my fork, asyncio sockets don't support it out of the box. SSLSessionBoundContext was a workaround from a contributor to the CPython repository that I slightly modified for this codebase.

[pohmelie]

@bellini666
I got you. Will try to make version which covers this, refactoring a little right now. I will call for the review and testing with your server

@sammichaels
Yeah, after I wrote a message I understand the idea (as always, sigh). The idea is to be sure that connected to the data port person is the same person who did the request for this connection. How do you think, can we use session sharing mechanic for both modes: implicit and explicit? Sounds like it is more secure

[sammichaels]

@sammichaels Yeah, after I wrote a message I understand the idea (as always, sigh). The idea is to be sure that connected to the data port person is the same person who did the request for this connection. How do you think, can we use session sharing mechanic for both modes: implicit and explicit? Sounds like it is more secure

I recall the context of the data channel is made in get_stream.

Maybe removing the guard that checks for self.ssl is appropriate since ssl_object existing means the command channel is using TLS, which means the data channel must also upgrade to TLS. Since there is no distinction between implicit/explicit for the data channel, the existence of an SSL context is enough.

aioftp/src/aioftp/client.py

Lines 1243 to 1252 in 30b8764

	ssl_object = self.stream.writer.transport.get_extra_info("ssl_object")
	if ssl_object and not self.ssl:
	await writer.start_tls(
	sslcontext=SSLSessionBoundContext(
	ssl.PROTOCOL_TLS_CLIENT,
	context=ssl_object.context,
	session=ssl_object.session,
	),
	server_hostname=self.server_host,
	)

Changes to (untested, just suggesting based on glancing at the code):

        if ssl_object := self.stream.writer.transport.get_extra_info("ssl_object"):
            await writer.start_tls(
                sslcontext=SSLSessionBoundContext(
                    ssl.PROTOCOL_TLS_CLIENT,
                    context=ssl_object.context,
                    session=ssl_object.session,
                ),
                server_hostname=self.server_host,
            )

[pohmelie]

Yeah, sounds really simpler and more stable. Maybe there are servers, which encrypt only command connection. But it is another story. Will do PR and ping you to test code

[sammichaels]

@pohmelie I think it might also be worth investigating the behavior of get_passive_connection because it calls a function that passes the SSL context held in self.ssl. If the instance was started with that value being True, as in, use the default context, then I don't believe it would reuse the existing TLS session.

It may reduce some complexity to dig a little deeper and handle the data connection TLS in get_passive_connection or the wrapper to open_connection. By sending PROT P as part of the TLS upgrade, data channels are required to be TLS (RFC 4217 9). I don't think we'd encounter a scenario where the data channel would not be encrypted with either implicit or explicit TLS.

By passing the correct SSL context (with session info) directly to open_connection for data channels, we may be able to avoid the start_tls entirely.

[pohmelie]

@sammichaels yeah, I did already localy, thank you for highlighting this

[astrand]

#187

[astrand]

Ping, any progress?

[pohmelie]

We are waiting for @sammichaels response

[pohmelie]

Maybe merge #187 as-is? And @sammichaels will raise another issue if something will be found. What do you think? @astrand