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

Zombie client connections with SSL proxy server? #276

Open
maxcellent opened this issue Mar 4, 2016 · 4 comments
Open

Zombie client connections with SSL proxy server? #276

maxcellent opened this issue Mar 4, 2016 · 4 comments

Comments

@maxcellent
Copy link

LittleProxy version: 1.1.0-beta1

Hi,

We are running a SSL proxy and tracking client connection with ActivityTracker. Here are some observations. I believe not all of them are relevant, but I am no expert to proxy technology so trying to provide as much information as I can.

  1. ActivityTracker#clientConnected is invoked before ActivityTracker#clientSSLHandshakeSucceeded, which makes sense because we need a TCP connection established before SSL handshake.
  2. some times there are exact 10 seconds between ActivityTracker#clientConnected and ActivityTracker#clientDisconnected without any activities: nothing received from client, nothing received from server. I don't understand what's happened, maybe it's just ssl handshake failed? Why it will fail at all?
  3. some times none of ActivityTracker#clientSSLHandshakeSucceeded and ActivityTracker#clientDisconnected are invoked after ActivityTracker#clientConnected is invoked (note our server is SSL only). In that case, server keeps sending back 504 Gateway Timeout which doesn't make sense to me at all. What do you expect client to action on a plain HTTP response in a SSL connection? As a result, all these responses are simply dropped by client side. However, because server side is sending this every 70 seconds (the default idle connection timeout). The connection is actually never closed.
  4. checking the code, in ClientToProxyConnection#timedOut
        boolean clientReadMoreRecentlyThanServer =
                currentServerConnection == null
                        || this.lastReadTime > currentServerConnection.lastReadTime;
        if (clientReadMoreRecentlyThanServer) {
            LOG.debug("Server timed out: {}", currentServerConnection);
            currentFilters.serverToProxyResponseTimedOut();
            writeGatewayTimeout(currentRequest);
            // DO NOT call super.timedOut() if the server timed out, to avoid closing the connection unnecessarily
        } else {
            super.timedOut();
        }

because nothing has been sent / received from server (ssl handshake is not performed), currentServerConnection.lastReadTime is always 0, which means clientReadMoreRecentlyThanServer is always true, which then explains 3. Shall we change this to something like this?

        boolean isConnected = isTCPConnected && (isSSL ? isSSLConnected : true );
        boolean clientReadMoreRecentlyThanServer =
                currentServerConnection == null
                        || this.lastReadTime > currentServerConnection.lastReadTime;
        if (clientReadMoreRecentlyThanServer && isConnected) {
            LOG.debug("Server timed out: {}", currentServerConnection);
            currentFilters.serverToProxyResponseTimedOut();
            writeGatewayTimeout(currentRequest);
            // DO NOT call super.timedOut() if the server timed out, to avoid closing the connection unnecessarily
        } else {
            super.timedOut();
        }

I am happy to test and submit a patch. But would like to confirm if this is the desired behaviour first.

Thanks,

@ganskef
Copy link
Collaborator

ganskef commented Mar 5, 2016

Hi @maxcellent, thank you very much for your observations.

I'm using LittleProxy with a MitmManager and I think I've seen volatile connection problems too, I couldn't dig into. I use LittleProxy based on version: 1.1.0-beta2-SNAPSHOT. It's difficult to test a released version since I depend on modifications in #230. Using an ActivityTracker is a great idea.

I'm happy to test your suggested modification in 4. too - I assume it is working better for you. But I would like to reproduce it before to understand the problem(s).

In my experiences both Netty/NIO and SSL depends on the platform sometimes. What's your platform?

@ganskef
Copy link
Collaborator

ganskef commented Mar 5, 2016

The code in 4 seems not to be complete. Please push a PR if you think it's a solution or it reproduces the problem.

I've never used an ActivityTracker. Eventually you've added special logging or conditional debug to show your observations.

@ganskef
Copy link
Collaborator

ganskef commented May 22, 2016

@jekh I've noticed a blocking behavior in a situation easy possible in IO/TLS processing. It seems to be an undefined or incomplete exception handling in connection flow. Please verify.

At org.littleshoot.proxy.impl.ProxyToServerConnection.MitmEncryptClientChannel.new ConnectionFlowStep() {...}.execute() the sslEngine is null in my offline environment. I have to check this (#282). After merging the new initialRequest a NullPointerException is thrown.

The point is the proxy was blocking after logging the Exception. An Exception in the flow at this stage leads to a blocked connection.

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

No branches or pull requests

3 participants
@maxcellent @ganskef and others