search

theophilusx / ssh2-sftp-client

a client for SSH2 SFTP
Apache License 2.0
816 stars 200 forks source link

Connection does not get closed in rare case #358

Closed TomasZamecnik closed 2 years ago

TomasZamecnik commented 3 years ago

Situation

When ssh2-sftp-client is trying to connect to server where SFTP is out of order, client does not close SSH connection regardless of whether end() method is called - connection remains open.

Idea

Underlying method this.client.end() never gets called in this situation.

How to simulate remote with SSH, but without SFTP

In the server, in system file /etc/ssh/sshd_config remove line: Subsystem sftp /usr/lib/openssh/sftp-server

Debugging

In src/index.js:SftpClient.end(): Condition if (haveConnection(this, 'end', reject)) { fails, which causes that this.client.end(); is not called.

theophilusx commented 3 years ago

I'm not sure your analysis is correct. If no connection is made, what connection can be released with end()?

Note that the if no connection is made, this.client will be undefined, so you cannot call this.client.end(). If this.client is defined, then hasConnection() will return true and this.client.end() will be called. The only way to end an sftp connection is with this.client.end(). We cannot call end if the connection failed and no sftp object was returned.

TomasZamecnik commented 3 years ago

sftp.connect() fails, but when I run netstat -aptn in bash to check the network situation, there is still TCP/SSH connection in the state ESTABLISHED and is never closed (until the whole program closes). I tried to add try/catch around await sftp.connect and call sftp.end in catch block, but it failed, because inner call of hasConnection returned false - it seems that sftp.endis not intended to be called when sftp.connect fails. Ok, but how can I close the connection? I think, that when sftp.connect fails, there should not be open connection.

sftp.connect ends with the following exception: Error: Unable to start subsystem: sftp

This issue can be reproduced also with the bulk-get.js example.

theophilusx commented 3 years ago

OK, I think I now see what your talking about.

What platform and what version of node are you running?

I think I know how to fix this, but I need to reproduce it, so need your node and platform.

The problem is, you cannot call sftpClient.end() because that calls sftpClient.sftp.end() and since you don't have an sftp connection, the sftp object is undefined (the isConnected just looks to see if sftpCleint.sft is defined, so that is not the issue - even if that call wasn't there sftpClient.end() would fail when it tries to call sftpClient.sftp.end().

What we need to do is call sftpClient.client.end(), but we must make sure we don't call that if sftpClient.sftp.end() has already been called or we will get an error thrown. The situation is further complicated because of the retry support as we also need to make sure we don't call sftpClient.client.end() if there isn't a connection as that will also throw an error.

I think I know a way around this, but will need to test my solution. This may take a few days as I have other problems here today (we just had a tornado rip through my location and while my place is OK, other friends are in trouble and I'll be busy helping them out for the next few days - probably be a week or more before I can make the changes and do the testing).

TomasZamecnik commented 3 years ago

Linux Debian 10 Buster, NodeJS v14.17.6 Now I am using that workaround - calling sftpClient.client.end() directly, when sftpClient.connect() fails.

I am sorry about the tornado and hope everybody is safe now! By coincidence we are developing the weather forecasting service - Windy.com And the SFTP bug I am dealing with now is in script communicating with one public meteo server. Our workaround with direct call of underlying end method is sufficient for our case, but I wanted to write about the issue, because maybe it should be fixed in the ssh2-sftp-client module itself.

theophilusx commented 3 years ago

Coincidentally, I took over the maintenance and extended this module when I was also working on weather data - used it to transfer large NetCDF data file from our Bureau of Meteorology for software used to predict flystrike and sudden chills for sheep farming and prediction software for cropping etc.

We don't normally get tornados in Australia - there might be some climate change occurring :-) Sadly, I have a few family members and friends who are now homeless, but luckily nobody badly hurt or dead.

I'll let you know once I've made the change and you can try out the new version from the repo and see if it fixes the issue.

TomasZamecnik commented 3 years ago

Coincidentally, that public meteo server I mentioned, is Australian BOM...and it was bombed by our unterminated connections :-) On tornadoes: I live in the Czech republic and tornadoes are very rare in history of our country. This year we experienced a huge one (probably biggest ever in CZ) which completely destroyed a few villages. Fortunately I know it only from TV. I wish you and your family to get over this horrific situation!

theophilusx commented 3 years ago

I have pushed a basic change to the master branch which I think should fix this issue. Can you please test and see if it helps? If it does, I'll release a new version.

theophilusx commented 2 years ago

Did you manage to try the master version to se if it addresses the issue you encountered?

theophilusx commented 2 years ago

Fix pushed to master and will be in v7.2.0