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

Reuse Session key on data connection #5087

Closed
cyberduck opened this issue Aug 7, 2010 · 23 comments
Closed

Reuse Session key on data connection #5087

cyberduck opened this issue Aug 7, 2010 · 23 comments

Comments

@cyberduck
Copy link
Collaborator

@cyberduck cyberduck commented Aug 7, 2010

018bff7 created the issue

I saw that it's described as a known issue in the Wiki, but still, could this be implemented?

From the ProFTPd documentation (http://www.proftpd.org/docs/contrib/mod_tls.html)

  • NoSessionReuseRequired
As of ProFTPD 1.3.3rc1, mod_tls only accepts SSL/TLS data connections that reuse the SSL session of the control connection, as a security measure. Unfortunately, there are some clients (e.g. curl) which do not reuse SSL sessions.

I see this incompatibility with my Debian ProFTPd 1.3.3a.

Thanks

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Aug 28, 2010

@dkocher commented

The issue is that our SSL session cache is by both host and port number. Using PASV to initiate the download will give a different port number than the control connection on port 21 and therefore uses a new SSL session context.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Aug 30, 2010

018bff7 commented

Thanks for your feedback. Haven't looked at the code myself, but couldn't we retrieve and clone the session key from the cache and create a new related entry in the session cache instead of doing a renegotiation? I guess at the time of creating the new connection for the download it's still known what the related control connection is.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Aug 30, 2010

@dkocher commented

Replying to [comment:3 abrax5]:

Thanks for your feedback. Haven't looked at the code myself, but couldn't we retrieve and clone the session key from the cache and create a new related entry in the session cache instead of doing a renegotiation? I guess at the time of creating the new connection for the download it's still known what the related control connection is.

The session caching is in a private class in the sun.security package I cannot access.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 16, 2010

@dkocher commented

Issue is documented in the wiki.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 16, 2010

@dkocher commented

Cross referencing to bug reports in thirdparty software:

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 17, 2010

018bff7 commented

I looked at GNU classpath, and there's a file called ClientHandshake.java:


            case WRITE_CLIENT_HELLO:
            {
              ClientHelloBuilder hello = new ClientHelloBuilder();
              AbstractSessionContext ctx = (AbstractSessionContext)
                engine.contextImpl.engineGetClientSessionContext();
              continued = (SessionImpl) ctx.getSession(engine.getPeerHost(),
                                                       engine.getPeerPort());

Maybe Cyberduck could use a modified version of GNU classpath for this use case and modify the AbstractSessionContext implementation so that it returns a previously used session for the same host even though the port is different?

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 17, 2010

018bff7 commented

I'm not really that familiar with Java, but looking at this:
http://download.oracle.com/javase/6/docs/api/javax/net/ssl/SSLContext.html

The protected constructor for an SSLContext allows you to specify an SSLContextSpi. Maybe we could override SSLContext implementation with one that uses an SSLContextSpi derived from the default one, that just differs in what it would return for engineGetClientSessionContext(). This method returns an SSLSessionContext which allows enumerating SessionIDs.

These are just some rough ideas, but may through this path we could tell the TLS engine to try reusing a session. We don't really need access to the masterSecret of the session, I think. We just need to make sure that the TLS engine can find the old ID and advertise the reuse ID to the server in the CLIENT_HELLO msg.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 17, 2010

@dkocher commented

Replying to [comment:7 abrax5]:

I looked at GNU classpath, and there's a file called ClientHandshake.java:


            case WRITE_CLIENT_HELLO:
            {
              ClientHelloBuilder hello = new ClientHelloBuilder();
              AbstractSessionContext ctx = (AbstractSessionContext)
                engine.contextImpl.engineGetClientSessionContext();
              continued = (SessionImpl) ctx.getSession(engine.getPeerHost(),
                                                       engine.getPeerPort());

Maybe Cyberduck could use a modified version of GNU classpath for this use case and modify the AbstractSessionContext implementation so that it returns a previously used session for the same host even though the port is different?

Yes, the peer port is what is causing a new session being created. The solution must somehow overwrite the peer port.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 17, 2010

@dkocher commented

Replying to [comment:8 abrax5]:

We have a problem delegating to the default context because everything has protected access. Also a new SSLContext is created using the factory method getInstance. Have to dig further.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 17, 2010

018bff7 commented

In OpenJDK, this code is in ClientHandshaker.java: (from http://download.java.net/openjdk/jdk6/)

    HandshakeMessage getKickstartMessage() throws SSLException {
        ClientHello mesg = new ClientHello(sslContext.getSecureRandom(),
                                        protocolVersion);
        maxProtocolVersion = protocolVersion;

        clnt_random = mesg.clnt_random;

        //
        // Try to resume an existing session.  This might be mandatory,
        // given certain API options.
        //
        session = ((SSLSessionContextImpl)sslContext
                        .engineGetClientSessionContext())
                        .get(getHostSE(), getPortSE());

So it'd be really useful if we can hook in there to replace the SSLSessionContext with our own implementation.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 17, 2010

@dkocher commented

Replying to [comment:11 abrax5]:

In OpenJDK, this code is in ClientHandshaker.java: (from http://download.java.net/openjdk/jdk6/)

Yes, that is what I meant above that the relevant code is in sun.security. We would need to replace a very large part of the JDK with custom implementations if we want to replace that implementation.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Sep 20, 2010

@dkocher commented

Because of the proposal to switch to a secure connection if the server advertises AUTH TLS in its features, this has become a major issue for many. See #5168.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Oct 5, 2010

@dkocher commented

Added configuration note to wiki.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Oct 5, 2010

@dkocher commented

#5285 closed as duplicate.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Nov 28, 2010

@dkocher commented

A workaround if the server allows would be to open the data socket without TLS. Uncheck Try to use TLS for data channel in Preferences → FTP → Data Channel Security.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Nov 29, 2010

@dkocher commented

Replying to [comment:11 abrax5]:

In OpenJDK, this code is in ClientHandshaker.java

Let me know if you have found any resolution for this. I am clueless how this could be best achieved.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Nov 30, 2010

@dkocher commented

#5484 closed as duplicate.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Jan 25, 2011

@dkocher commented

#5658 closed as duplicate.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented May 30, 2011

@dkocher commented

Blocked by NET-408.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Jun 23, 2011

@dkocher commented

#6027 closed as duplicate.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Mar 16, 2013

@dkocher commented

In a9a6621.

@cyberduck cyberduck closed this Mar 16, 2013
@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Apr 10, 2013

@dkocher commented

Addendum in aea9445.

@cyberduck
Copy link
Collaborator Author

@cyberduck cyberduck commented Jan 12, 2021

@dkocher commented

Addendum for running with latest runtime in c0efa1f.

@iterate-ch iterate-ch locked as resolved and limited conversation to collaborators Nov 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants