Skip to content

Conversation

@vhuk
Copy link
Contributor

@vhuk vhuk commented Aug 5, 2016

This is to prevent FTPS issues with IIS, ProFTPD and essentially fixes the mistake I did on PR #2032.

Sorry about that.

… NLST command.

This is to prevent issues with IIS and ProFTPD.
@nikic
Copy link
Member

nikic commented Aug 5, 2016

This is what RFC 4217 has to say on the topic:

              Client                                 Server
     control          data                   data               control
   ====================================================================

     PASV -------------------------------------------------------->
                                             socket()
                                             bind()
         <------------------------------------------ 227 (w,x,y,z,a,b)
                      socket()
     STOR file --------------------------------------------------->
                      connect()  ----------> accept()
         <-------------------------------------------------------- 150
                      TLSneg()   <---------> TLSneg()
                      TLSwrite()  ---------> TLSread()
                      TLSshutdown() -------> TLSshutdown()
                      close()     ---------> close()
         <-------------------------------------------------------- 226

So yeah, it looks like TLS negotiation is supposed to occur after receiving the NLST response.

What I find peculiar however, is that this diagram seems to match what we were doing originally (i.e. first NLST, then open data connection always and only then read response).

@vhuk
Copy link
Contributor Author

vhuk commented Aug 5, 2016

That's interesting indeed. It basically means to revert PR #2033 and leave it up to the non-compliant servers to fix the issue (IIS FTP 7.5+). I can update the test cases if we go that route.

@nikic
Copy link
Member

nikic commented Aug 5, 2016

Merged as 65056e9 into 5.6+.

If the implementation as it is now works with IIS (and nobody else complains) I think we should keep it... (Also we're now really in line with ext/ftp -- I did not notice before that it does the TLS initialization only after getting the response as well.)

@nikic nikic closed this Aug 5, 2016
@nikic
Copy link
Member

nikic commented Aug 5, 2016

Am I understanding it correctly that prior to the s/stream/datastream patch opendir over ftps did not work at all, so we do not actually regress anything if a version without the change in this PR is released?

@vhuk
Copy link
Contributor Author

vhuk commented Aug 6, 2016

That's correct, opendir over ftps didn't work at all prior to fixing bug 54431. opendir over ftp work correctly on 5.6.25RC1, at least against ProFTPD, vsftpd. Will test additional platforms at Monday.

There is a slight change in the failure mode but that's about it. Before 5.6.25RC1 opendir over ftps failed immediately, now it'll fail after the negotiation times out.

@vhuk
Copy link
Contributor Author

vhuk commented Aug 8, 2016

I ran some additional test for this to confirm 5.6.25RC1 works fine over ftp:// without this PR, but will fail using ftps://.

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