Hi, does this issue also apply to the ftp:// wrapper in ext/standard?

This fix does not apply to that wrapper; that uses the SSL implementation in ext/openssl/xp_ssl.c for the SSL support. It does appear that the wrapper can pass a session stream down, and it looks like xp_ssl.c will copy the session ID to the new stream. I can run some tests to see if that's sufficient to satisfy FTP-S servers with this requirement, but I suspect not. If not, updating that code would be a larger undertaking requiring even more review.

Thank you for your clarification. The reason I ask is that I remember seeing a recent bug that only affected one of those two, as we have different implementations for ext/ftp and ftp:// (in ext/standard/ftp_fopen_wrapper.c) so it would be nice with some unification.

cc @Tyrael as the 5.6 RM

@KalleZ, I just fixed couple of issues with the TLS/SSL side of the ftp:// wrapper in ext/standard and agree that some unification is needed.

More related to this PR, ext/standard/ftp_fopen_wrapper.c doesn't support SSL session re-use for data channel with AUTH TLS. Based on the comments it looks like there is an attempt for this with AUTH SSL:

/* we must reuse the old SSL session id */
/* if we talk to an old ftpd-ssl */

In its current form ext/standard ftp:// wrapper fails to connect to many ftps servers running their standard configs. As an example, ftp:// wrapper fails with both ProFTPD and vsftpd when the servers run their out of the box Debian configs with TLS enabled. Both can be fixed by changing the server config not not require data channel session reuse.

Patch looks good to me. One question I have is whether we can drop the SSL_copy_session_id in the old_ssl branch now?

Do we have any possibility of testing this? I.e. enforcing in the ext/ftp server.inc that control and data use the same connection?

It seems like there's currently no ability to retrieve the SSL session information from PHP, unless I've missed it somewhere. If a test is desired, would the capture_session_meta/session_meta options(s) be an appropriate place to include that information?

Merged via dfadc5a, thanks!

It would still be nice to expose the necessary information (session_meta seems a reasonable place), but that's a separate change :)