Skip to content

Conversation

@HviorForgeFlow
Copy link
Contributor

@HviorForgeFlow HviorForgeFlow commented Dec 6, 2025

This change introduces support for implicit FTPS connections in the FTPFileSystem. Previously, only explicit FTPS was supported.

A new parameter security tls has been added to the FTPFileSystem constructor. This parameter allows users to specify the SSL/TLS protocol to be used for implicit FTPS connections,
such as TLSv1.2.

Key changes:

  • Introduced ImplicitFTPTLS class to handle automatic SSL wrapping for implicit FTPS.
  • Added security tls parameter to FTPFileSystem to allow selection of SSL/TLS protocols (e.g., "tlsv1_2").
  • Updated the connection logic to differentiate between explicit and implicit FTPS based on the security tls parameter.

This enhancement provides greater flexibility and compatibility for connecting to various FTP servers requiring implicit FTPS with specific security protocols.

@martindurant
Copy link
Member

Thank you for contributing. I expect to be able to look through this shortly.

@martindurant
Copy link
Member

This all looks very nice - thank you.

One question: why have the extra argument, when we already have one (tls=). Since the existing one is bool, it could still allow for all the str options you are adding here.

@HviorForgeFlow
Copy link
Contributor Author

Hello @martindurant in this apporach I just was ensuring not to break the current implementation, and yes it makes sense to reuse, I'm going to do a proposal here: 541f7f4

I will squash the commits if we move it forward

@martindurant
Copy link
Member

We normally squash-merge anyway, so don't worry about that. This is ready for me to look at again?

@HviorForgeFlow HviorForgeFlow force-pushed the ftp-implicit-over-tls-1.2 branch from 541f7f4 to d022539 Compare December 23, 2025 20:44
@HviorForgeFlow
Copy link
Contributor Author

Comment attended, ready to review.

Copy link
Member

@martindurant martindurant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good. I have one request for you to consider.

@HviorForgeFlow HviorForgeFlow force-pushed the ftp-implicit-over-tls-1.2 branch from b11601c to 47d1d27 Compare December 23, 2025 21:44
@HviorForgeFlow
Copy link
Contributor Author

Looks like the workflow approval got rejected when I squash + force push the commits.
Added a read check on the same test method.

@martindurant
Copy link
Member

I believe approval is required every time if you are not already a contributor. For future reference, we squash-merge in this repo anyway, so no need to consolidate your commits in the PR branch.

@martindurant martindurant merged commit 2951d1f into fsspec:master Dec 24, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants