Adds SSL/TLS toggle to FTP connection

[bdelcamp]

This adds basic yes/no to the FTP connection enabling basic ftps functionality. An underlying issue is that promise-ftp uses an out of date version of @icetee/ftp which has a bug which causes ftps connect to fail. I have a pull request open here realtymaps/promise-ftp#47 to update that library. Updating manually on the local node_modules works, and all tests still pass.

[CLAassistant]

All committers have signed the CLA.

[Joffcom]

Hey @bdelcamp,

Thanks for the PR, This is something that I have felt we have been missing for a while. I guess we will need to wait for the other PR to be merged first.

There are a couple of tweaks for this one as well, There is the toggle for SSL/TLS but it is just set to true for the credential test instead of using the credential value. It would also be worth adding a toggle to ignore invalid / self signed certificates as well and setting secureOptions to include rejectUnauthorized.

I do very much like what you have started to do here though, Nice work 🙌🏻

... quick edit: I wonder if it would be worth changing the toggle to a dropdown so we can have Plain, Implicit and Explicit as options.

[bdelcamp]

I'll definitely put those changes in place especially since it looks like there's going to be time to get it done before promise-ftp is ready. The true setting looks like I missed it from when I was testing before I put the toggle in... my bad.

…

On Wed, Oct 12, 2022 at 6:15 AM Jonathan Bennetts ***@***.***> wrote: Hey @bdelcamp <https://github.com/bdelcamp>, Thanks for the PR, This is something that I have felt we have been missing for a while. I guess we will need to wait for the other PR to be merged first. There are a couple of tweaks for this one as well, There is the toggle for SSL/TLS but it is just set to true for the credential test instead of using the credential value. It would also be worth adding a toggle to ignore invalid / self signed certificates as well and setting secureOptions to include rejectUnauthorized. I do very much like what you have started to do here though, Nice work 🙌🏻 — Reply to this email directly, view it on GitHub <#4318 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA7UZGQ2MBCF3DF2R2DH3TWC2FT7ANCNFSM6AAAAAARCREREA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bdelcamp]

Hey @bdelcamp,

Thanks for the PR, This is something that I have felt we have been missing for a while. I guess we will need to wait for the other PR to be merged first.

There are a couple of tweaks for this one as well, There is the toggle for SSL/TLS but it is just set to true for the credential test instead of using the credential value. It would also be worth adding a toggle to ignore invalid / self signed certificates as well and setting secureOptions to include rejectUnauthorized.

I do very much like what you have started to do here though, Nice work 🙌🏻

... quick edit: I wonder if it would be worth changing the toggle to a dropdown so we can have Plain, Implicit and Explicit as options.

Ha, not sure when you edited that, but I committed that change yesterday morning:)

[j2bd]

Hi guys,
Thanks @bdelcamp @Joffcom for your great work.
IT could be great to have SSL/TLS to FTP node soon...

[bdelcamp]

It doesn't seem like promise-ftp will be updated, not sure what other alternative there would be other than to fork it and include it directly...

[flosky]

@bdelcamp please let me know if you plan to create a fork. We also have an interest in upgrading the dependencies to the latest version to remove some deprecation warnings and possible vulnerabilities.

[IceBreaker8]

@bdelcamp I have clone the fix you made, npm install, npm run build and npm run start, but the problem isn't fixed for me.
We are trying to establish an FTP connection to an outside server via n8n.
When we do so, we get an SSL/TLS connection error.

[bdelcamp]

@bdelcamp I have clone the fix you made, npm install, npm run build and npm run start, but the problem isn't fixed for me. We are trying to establish an FTP connection to an outside server via n8n. When we do so, we get an SSL/TLS connection error.

@IceBreaker8 Did you see that it also requires a patched version of realtymaps/promise-ftp ?

[bdelcamp]

@bdelcamp please let me know if you plan to create a fork. We also have an interest in upgrading the dependencies to the latest version to remove some deprecation warnings and possible vulnerabilities.

@flosky I don't know if I can devote the time to maintaining a fork of that library, I would propose that the n8n project create the fork, that way anyone can directly contribute to it, and it can be directly included in the n8n builds.

[IceBreaker8]

@bdelcamp I did upgrade that dependency manually... Do you have a working github project repo that works that I can clone?

[Joffcom]

@bdelcamp my worry there is I don't know if we would have the time to maintain a fork of the library either.

I have some other thoughts though which I will look into early next year.

[bdelcamp]

@Joffcom To be fair, it's really not maintained at present either, but if the project forks it, then any contributor can create pull requests against it to keep it relevant to the project itself.

[morphar]

Could this be fixed with overrides ?

[morphar]

I'm thinking something like this in n8n's package.json:

{
  ...
  "overrides": {
    "@icetee/ftp": "^1.0.8"
  }
}

[morphar]

I don't know exactly what to expect with regards to errors, but I have set up a dev env and tried adding the above overrides to package.json in nodes-base and the changes made in this pull request.
So far I've succeeded in listing files and downloading a file from an FTPS server.

Just to be on the safe side: I am aware of the difference between SFTP and FTPS ;-)
And I am 100% sure that I am now talking to an FTPS server that I could not connect with, before these updates.

[bdelcamp]

@morphar Good idea, that didn't even occur to me... This seems like the most maintainable solution at the moment until the upstream maintainer wants to update their dependencies.

[dncpax]

hey there. is this going to happen? @Joffcom is this still on your radar?

[Joffcom]

Hey @dncpax,

This is on my radar but it is something that I think needs to be done as part of an overhaul so we can make sure we support both Implicit and Explicit FTPS and some of the other options. The best way I can think of to do this would be to change to a different package which won't be a quick job.

[umbearto]

Hello, any update on this?

[Nionor]

Would also be interested in this.
I'm self-hosting this in docker, would the fixes from @bdelcamp and @morphar be something that could be changed on a running container as a temporary fix or does it require a full rebuild from source?

[Joffcom]

Hey @bdelcamp,

It looks like the PR has still not been merged on the other end, What I have done internally is created a dev ticket to move to a different package so that we can implement this properly in the future.

Because of this planned change it does sadly mean that I will need to close this PR as it will no longer be relevant as such, Thanks for sticking with us on this one.

I don't have a timeline yet but I want to make sure we get this done this year.