Jetty 9 - Keep-Alive not respected/persistent connection forcefuly closed when input was not consumed

[Adebski]

In an application that I work on we have upgraded to Jetty 9 recently. Overall the process was pretty smooth but from time to time some problems comes out due to the differences between how 8 and 9 handles things.

One of the responsibilities of the application is to serve devices with TR-069 protocol (https://en.wikipedia.org/wiki/TR-069) that uses digest authentication for devices. In one particular case we observed that when device came with initial request w/o authorization headers (and this initial request payload contained not-so-small SOAP message) the Jetty

• received the request and after receiving the headers started processing request in handlers
• control reached our custom subclass of Authenticator that set headers required by digest authentication and responded with UNAUTHORIZED (401) status code
• Jetty then closed the connection on server side (FIN, ACK observed in the Wireshark) despite using HTTP 1.1 and Keep-Alive header in the request AND did not set Connection: close header in the response

In this example the content that was being sent by the device was not consumed by anyone.

After the investigation on why the connection was being closed i discovered code in the HttpConnection class:

// Finish consuming the request
        // If we are still expecting
        if (_channel.isExpecting100Continue())
        {
            // close to seek EOF
            _parser.close();
        }
        else if (_parser.inContentState() && _generator.isPersistent())
        {
            // If we are async, then we have problems to complete neatly
            if (_channel.getRequest().getHttpInput().isAsync())
            {
                if (LOG.isDebugEnabled())
                    LOG.debug("unconsumed async input {}", this);
                _channel.abort(new IOException("unconsumed input"));
            }
            else
            {
                if (LOG.isDebugEnabled())
                    LOG.debug("unconsumed input {}", this);
                // Complete reading the request
                if (!_channel.getRequest().getHttpInput().consumeAll())
                    _channel.abort(new IOException("unconsumed input"));
            }
        }

in my case isAsync was returning false so consumeAll method was being invoked

public boolean consumeAll()
    {
        synchronized (_inputQ)
        {
            try
            {
                while (!isFinished())
                {
                    Content item = nextContent();
                    if (item == null)
                        break; // Let's not bother blocking

                    skip(item, remaining(item));
                }
                return isFinished() && !isError();
            }
            catch (IOException e)
            {
                LOG.debug(e);
                return false;
            }
        }
    }

as you can see in case of returning false current channel is being aborted (and there is no trace of the exception in the logs or information that connection was closed because not the whole input was consumed - the exception message is never logged).

I also observed in Wireshark that the last part of the content arrived after server responded with 401 unauthorized.

My hypothesis is that in a situation where we did not receive the whole HTTP payload yet (and because of that nextContent() returned null) Jetty is closing the connection by not waiting for the rest of the content (and our experiments suggest that the situation was different in Jetty 8 where it was consuming all the content regardless if it had to wait for a second for the rest of it).

I understand the reasoning behind the decision here but it would be nice to have a way to specify that we want to consume the content and keep the connection alive even if it would require blocking for some time (with configurable timeout).

For now I created external handler that consumes all the content that is left after all the handlers have finished the job:

super.handle(target, baseRequest, request, response);
        if (response.getStatus() == HttpServletResponse.SC_UNAUTHORIZED && !baseRequest.isAsyncStarted()) {
            LOGGER.debug("Consuming all the leftover content for target {}, request {}", target, baseRequest);
            JettyUtils.skipAllContent(skipChunkSize, request.getInputStream());
        }

[sbordet]

Why don't you establish the credentials with an initial request that does not have a body ?

[Adebski]

Can't be done - that is a part of the protocol that initial request can contain SOAP message that contains information that changed from the last time given device came to the system.

[sbordet]

If the initial request contains a body and is not authenticated, your application handling should consume the request body.

If I understand correctly, the first place where your code is hit is your custom Authenticator, so that would be the right place to consume the request body before responding with a 401.

Would that work ?

[Adebski]

Yea it is a solution but there are other places that can use standard Authenticators and I can't easily modify those (e.g. CXF using Jetty).

That is why I was thinking it shouldn't be the responsibility of the authenticator to know that it has to consume all the content or otherwise something bad will happen.

[sbordet]

@gregw, your opinion on this ?

[gregw]

My thoughts are as follows:

• Connection: Keep-alive is old HTTP/1.0 style, but it is a persistent connection request
• Connections in HTTP/1.1 are persistent by default, no need for keep-alive
• Persistent connections do not mean that the server is not entitle to close the connection whenever it wants, for what ever reason it wishes.
• We have looked at many ways to deal with consumeAll and ultimately they risk being DOS vectors. Thus the implementation has decided that if the application does not want to read the body of the request, then the server has to heuristically decide if it will bother or not. That decision is now implemented as it will consumeAll IFF it can do so without blocking or invoking async logic.
• It is simple for an application to avoid this server heuristic. If it cares about all the content being consumed, then it should consume all of the content.
• If an authenticator is generating a 401 response based on headers that may be in a large message, then I don't think it would be unreasonable for that authenticator to consume the message. It is after all the one that has decided the message will be ignored.
• If you really don't want consumeAll logic in the authenticator, then putting it in a filter or handler is a reasonable alternative. That separates the concern of authentication from preserving persistent connections.
• If this protocol really needs to send large messages that may fail authentication, then it should perhaps look at using expect 100 continues to avoid sending the body unless it will be consumed properly... although rejecting a 100 continues also involves closing the connection (unless HTTP/2 is used).
• If the request body is really large, then it may well be better to let the heuristic occur, as it may be best to stop the sending of wasted data rather than let it be sent and discarded. Re-opening a new connection may be the less of too evils. The situation where the connection is closed, but the full response body arrives in the TCP/IP buffers anyway, must just be a Goldilocks message size: not too small to arrive without blocking; not too big to still completely arrive even if the connection is closed.
• As we all move to HTTP/2, it will not matter if "connections" are closed as they are only streams.

So I'm happy with our current behaviour and there is a valid/easy alternative for the user. So I'm closing this now. If there is something I've missed or an alternate point of view, please re-open.

cheers

[Adebski]

After testing for few days the fix that involved consuming content in outermost handler after handling it turns out it was not a great idea.

We had a problem in a situation where initial request was sent not with Content-Length header but with Transfer-Encoding: chunked. It resulted in exchange like

• device sends initial request with payload, no authorization information, chunked encoding
• we respond to it after reading headers with 401
• device receives the response and at the same time we start to read the payload in outermost handler
• device starts to send second request with authorization headers
• Jetty sends 500 internal server error because it expected to receive chunked data.

TCP stream captured from wireshark looks something like

POST /some/url HTTP/1.1
Content-Type: text/xml; charset=ISO-8859-1
Host: 1.2.3.4
SOAPAction:
Transfer-Encoding: chunked

HTTP/1.1 401 Unauthorized
Date: Wed, 22 Jun 2016 12:32:22 GMT
WWW-Authenticate: <authorization-data>
Cache-Control: must-revalidate,no-cache,no-store
Content-Type: text/html;charset=iso-8859-1
Content-Length: 332
Server: Jetty(9.3.8.v20160314)

<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 401 </title>
</head>
<body>
<h2>HTTP ERROR: 401</h2>
<p>Problem accessing <url> Reason:
<pre>    Unauthorized</pre></p>
<hr /><a href="http://eclipse.org/jetty">Powered by Jetty:// 9.3.8.v20160314</a><hr/>
</body>
</html>
POSTHTTP/1.1 500 Server Error
Connection: close
Server: Jetty(9.3.8.v20160314)

 /some/url HTTP/1.1
Content-Type: text/xml; charset=ISO-8859-1
Host: 1.2.3.4
SOAPAction:
Transfer-Encoding: chunked
Authorization: <authorization-data>

you can see the second request (POST) immediately before 500 internal server error. Looking at the server logs Jetty complained that it received letter P instead of number that would designate the length of the chunk.

The authenticators perform the response send in their validateRequest method so unfortunately I had to copy the code of digest and basic authenticators directly from Jetty codebase and add single line for them.

I wanted to add this information to this ticket for others that encounter this kind of a problem and also clarify that it is not THAT easy to fix this problem in a nice way.

[gregw]

@Adebski The client is breaking the RFC by acting like that. Once it has indicated that it is sending a chunked body, it must either complete the chunking or close the connection. It cannot just start sending another request in the middle of the previous one.

If you have to work with such clients, the I would advise that you update your Authenticator to consume all the input before sending the 401.

[Adebski]

Unfortunately we have no way to change the clients so we will just cope with that in our codebase. Just wanted to leave the information here how we ultimately fixed our issue.

[gregw]

We are happy to change to code base to make it easier to extend those authenticators without copy and pasting. But I thought you had your own authenticator rather than extending ours?

If you can share more of your authenticator then we can see what we can do to avoid the cut and paste!

[Adebski]

In one instance we are using copied DigestAuthenticator - it was copied and modified slightly long time ago to cope with another strange implementation of HTTP protocol in the wild. In other case we were using stock BasicAuthenticator (we didn't encounter a situation where we had to serve client with improper implementation of basic authentication).

I added another modification to the already copied DigestAuthenticator:

 if (!DeferredAuthentication.isDeferred(response)) {
    JettyUtils.skipAllContent(skipChunkSize, req.getInputStream());
        String domain = request.getContextPath();
    if (domain == null)
        domain = "/";
    response.setHeader(HttpHeader.WWW_AUTHENTICATE.asString(), "Digest realm=\"" + _loginService.getName()
        + "\", domain=\""
        + domain
        + "\", nonce=\""
        + newNonce((Request) request)
        + "\", algorithm=MD5, qop=\"auth\","
        + " stale=" + stale);
    response.sendError(HttpServletResponse.SC_UNAUTHORIZED);

    return Authentication.SEND_CONTINUE;
}

and also copied BasicAuthenticator and added

if (DeferredAuthentication.isDeferred(response))
    return Authentication.UNAUTHENTICATED;

JettyUtils.skipAllContent(skipChunkSize, req.getInputStream());
response.setHeader(HttpHeader.WWW_AUTHENTICATE.asString(), "basic realm=\"" + _loginService.getName() + '"');
response.sendError(HttpServletResponse.SC_UNAUTHORIZED);
return Authentication.SEND_CONTINUE;

in both of those cases I added line JettyUtils.skipAllContent(skipChunkSize, req.getInputStream()); that skips the content of ServletInputStream

public static long skipAllContent(long skipChunkSize, ServletInputStream is) throws IOException {
    long bytesRead = 0;
    while (!is.isFinished()) {
        long skipped = is.skip(skipChunkSize);
        if (skipped > 0) {
            bytesRead += skipped;
        } else if (skipped == 0) {
            // due to the contract of skip method we do not have any guarantees that there is no more data so we
            // have to use read() method to be sure
            if (is.read() == -1) {
                // EOF
                break;
            } else {
                bytesRead += 1;
            }
        } else {
            throw new IOException("skip() returned a negative value when skipping all the request content");
        }
    }
    return bytesRead;
}

[gregw]

I had another look at this and I can't see that we can consume content in our Authenticators as it opens us up to and easy DOS even if the app is written totally async.

I think you can avoid cut&paste simply by extending. The skipAllContent can be done after the response is sent, just so long as it is done before the container tries.