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

Fix hang when using ExplicitTLS to certain servers. #283

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

ncw
Copy link
Contributor

@ncw ncw commented Sep 16, 2022

In #282 it was discovered that doing the tls Handshake immediately on connection causes some FTP servers (proftpd and pureftpd) to hang.

The exact cause of this is unknown, but this patch works around the problem by not doing the Handsake initially, and only doing it at the end if we were attempting to upload a zero length file.

Doing the Handshake at the end was originally added in a4e9650 however it got reverted in 212daf2 which used tls.DialWithDialer to do the handshake. Unfortunately tls.DialWithDialer seems to trigger the hanging bug.

See: https://forum.rclone.org/t/rclone-ftps-explicit-rclone-touch-empty-files-proftpd-unable-to-build-data-connection-operation-not-permitted/22522
See: rclone/rclone#6426 (comment)
Fixes #282

@coveralls
Copy link

coveralls commented Sep 16, 2022

Coverage Status

Coverage decreased (-2.0%) to 72.521% when pulling 5da3769 on ncw:fix-282-tls-handshake-hang into 6512c2a on jlaffaye:master.

@jlaffaye
Copy link
Owner

Can you please add a comment in the code to explain why we setup the connection with tls.Client and not tls.Dial ?
So we dont forget about this in the future.

@ncw
Copy link
Contributor Author

ncw commented Sep 20, 2022

I've put a comment in explaining the reasoning.

@eliasdaler
Copy link

Encountered this bug today with pure-ftpd server - the proposed fix worked for me.

@dany74q
Copy link

dany74q commented Oct 14, 2022

Tackled this as well - any chance we can merge the PR ?

@ncw ncw force-pushed the fix-282-tls-handshake-hang branch from 2ead61e to c31e428 Compare October 14, 2022 10:56
@ncw
Copy link
Contributor Author

ncw commented Oct 14, 2022

I have updated the code with a bit more documentation.

Unfortunately I'm going to have to hard fork flaffaye for the rclone v1.60 release with this patch in - I'll move the rclone back as soon as this gets merged and released.

In jlaffaye#282 it was discovered that doing the tls Handshake immediately on
connection causes some FTP servers (proftpd and pureftpd) to hang.

The exact cause of this is unknown, but this patch works around the
problem by not doing the Handsake initially, and only doing it at the
end if we were attempting to upload a zero length file.

Doing the Handshake at the end was originally added in
a4e9650 however it got reverted in 212daf2 which
used tls.DialWithDialer to do the handshake. Unfortunately
tls.DialWithDialer seems to trigger the hanging bug.

See: https://forum.rclone.org/t/rclone-ftps-explicit-rclone-touch-empty-files-proftpd-unable-to-build-data-connection-operation-not-permitted/22522
See: rclone/rclone#6426 (comment)
Fixes jlaffaye#282
@ncw ncw force-pushed the fix-282-tls-handshake-hang branch from c31e428 to 5da3769 Compare October 14, 2022 10:58
ncw added a commit to rclone/rclone that referenced this pull request Oct 14, 2022
It was discovered that doing the tls Handshake immediately on
connection causes some FTP servers (proftpd and pureftpd) to hang.

This imports a fix for it by temporarily hard forking jlaffaye/ftp to
include the fix submitted as a pull request.

See: https://forum.rclone.org/t/rclone-ftps-explicit-rclone-touch-empty-files-proftpd-unable-to-build-data-connection-operation-not-permitted/22522
See: #6426 (comment)
See: jlaffaye/ftp#283
See: jlaffaye/ftp#282
@mohsek
Copy link

mohsek commented Oct 25, 2022

facing the same issue, @ncw When are you planning to merge this one?

@ncw
Copy link
Contributor Author

ncw commented Oct 25, 2022

@mohsek not my call! I have merged a fix for this in rclone 1.60 github.com/rclone/ftp@v0.0.0-20221014110213-e44dedbc76c6

@theGuen
Copy link

theGuen commented Nov 22, 2022

I had the same error with the hanging connection

So i tried to use this one
No i get an EOF error... Filezilla has no problems uploading files with Explicit TLS.
But i am not sure if this is the server or the lib....

Server is pureftp

`
220-You are user number 2 of 5 allowed.

220-Local time is now 17:46. Server port: 21.

220-This is a private system - No anonymous login

220-IPv6 connections are also welcome on this server.

220 You will be disconnected after 15 minutes of inactivity.

AUTH TLS

234 AUTH TLS OK.

USER xxxx

331 User admin OK. Password required

PASS xxxx

230 OK. Current directory is /

FEAT

211-Extensions supported:

EPRT

IDLE

MDTM

SIZE

MFMT

REST STREAM

MLST type*;size*;sizd*;modify*;UNIX.mode*;UNIX.uid*;UNIX.gid*;unique*;

MLSD

AUTH TLS

PBSZ

PROT

UTF8

TVFS

ESTA

PASV

EPSV

SPSV

211 End.

TYPE I

200 TYPE is now 8-bit binary

OPTS UTF8 ON

200 OK, UTF-8 enabled

PBSZ 0

200 PBSZ=0

PROT P

200 Data protection level set to "private"

EPSV

229 Extended Passive mode OK (|||30005|)

STOR myfile

150 Accepted data connection

1 error occurred:
* EOF
`

@funkyshu
Copy link

@jlaffaye any chance we could get this merged?

@jlaffaye jlaffaye merged commit 9e39e2c into jlaffaye:master Feb 8, 2023
@jlaffaye
Copy link
Owner

jlaffaye commented Feb 8, 2023

Oops totally forgot about this PR

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.

Data connections with ExplicitTLS hang forever
8 participants