Lighttpd

lighttpd 1.4 mod_proxy is not an HTTP/1.1 proxy and does not handle Transfer-Encoding: chunked. Your backend is non-compliant (broken) if it is sending Transfer-Encoding: chunked in response to an HTTP/1.0 request. You don't have to do that at all, and you shouldn't. If you stream the data to lighttpd, lighttpd will chunk it back to the client if the client made an HTTP/1.1 request and you have set lighttpd.conf server.stream-response-body = 1 (or 2)

Your patch makes lighttpd blindly pass bytes, which may break other things, and so the patch as-is is unlikely to be accepted.

The solution is for the backend to simply not send Transfer-Encoding: chunked. Just stream the response body as-is.

Transfer-Encoding is a hop-by-hop header, so mod_proxy would ideally validate the chunked encoding if mod_proxy were modified to send HTTP/1.1 requests to backend, and to handle Transfer-Encoding: chunked responses. If lighttpd is not streaming the response body back to the client, then there is no use for Transfer-Encoding: chunked, since lighttpd mod_proxy is going to wait for a completed response. lighttpd mod_proxy does not reuse connections to backends (no keep-alive), and sends HTTP/1.0 requests and sends Connection: close.

I have pin pointed where in the code creates this issue and attached the patch with a fix.

Just to be clear: that line that your patch removed was intentional and specific to proxying, or else it would not be in the code with such specificity.

While the solution you suggested was rejected, thank you for looking into the code and putting together your test case (even though it, too, was invalid for HTTP/1.0)

gstrauss wrote:

This is not a bug in lighttpd, though maybe I'll add some trace to lighttpd to log to error log that backend proxy is sending an invalid response to an HTTP/1.0 request.

Yes, I think this would be the best course of action for this kind of situation, because otherwise we are led to think this is a bug in lighttpd due to the garbled result we get from a browser. Since otherwise the chunked packets (minus the header which states the encoding) are directly sent as is to the client.

I did check sandstorm's code to see how they implemented this and it turns out they just hardcoded HTTP/1.1 responses...

Maybe we could also send an error page to the client which clearly states the situation too? (or just a default error so as to make the admin check the error logs)

The code I committed to lighttpd makes this situation an error, sending 502 Bad Gateway to the client, and will send trace to the error log "proxy backend sent invalid response header (Transfer-Encoding) to HTTP/1.0 request"

It is valid for sandstorm to send HTTP/1.1 back in the status line, even if the request from lighttpd is HTTP/1.0.
However, it is not compliant with HTTP RFCs for sandstorm to send Transfer-Encoding: chunked in response to an HTTP/1.0 request.

gstrauss wrote:

The code I committed to lighttpd makes this situation an error, sending 502 Bad Gateway to the client, and will send trace to the error log "proxy backend sent invalid response header (Transfer-Encoding) to HTTP/1.0 request"

It is valid for sandstorm to send HTTP/1.1 back in the status line, even if the request from lighttpd is HTTP/1.0.
However, it is not compliant with HTTP RFCs for sandstorm to send Transfer-Encoding: chunked in response to an HTTP/1.0 request.

Ah that's very good to know.

gstrauss wrote:

Here's a completely untested patch for the bug in sandstorm which sends Transfer-Encoding: chunked in response to an HTTP/1.0 request. If this works for you, I'll submit a pull request upstream.
[...]

I wasn't expecting a patch for sandstorm, this is very nice, thank you. I have had issues compiling sandstorm manually but I finally managed to today. Then I applied your patch and recompiled and it gave some errors... It seems that the base class http_parser is private so it won't allow access to http_major and http_minor, here's the full output :
(I'd try to fix it but I'm a C coder, not a C++ one, so I'm flat footed there... But I'll still try to find a way to fix this)

✘ install: sandstorm/sandstorm.ekam-manifest
    sandstorm/sandstorm-http-bridge: not found
✘ compile: sandstorm/sandstorm-http-bridge.c++
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:947:9: error: cannot cast '
      sandstorm::HttpParser' to its private base class 'http_parser'
        if (parser->http_major == 1 && parser->http_minor == 0) isChunked = false;
            ^
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:159:19: note: declared private here
                      private http_parser,
                      ^~~~~~~~~~~~~~~~~~~
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:947:17: error: 'http_major' is a 
      private member of 'http_parser'
        if (parser->http_major == 1 && parser->http_minor == 0) isChunked = false;
                    ^
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:159:19: note: constrained by 
      private inheritance here
                      private http_parser,
                      ^~~~~~~~~~~~~~~~~~~
    /ekam-provider/c++header/joyent-http/http_parser.h:206:18: note: member is declared here
      unsigned short http_major;
                     ^
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:947:36: error: cannot cast '
      sandstorm::HttpParser' to its private base class 'http_parser'
        if (parser->http_major == 1 && parser->http_minor == 0) isChunked = false;
                                       ^
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:159:19: note: declared private here
                      private http_parser,
                      ^~~~~~~~~~~~~~~~~~~
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:947:44: error: 'http_minor' is a 
      private member of 'http_parser'
        if (parser->http_major == 1 && parser->http_minor == 0) isChunked = false;
                                               ^
    /ekam-provider/canonical/sandstorm/sandstorm-http-bridge.c++:159:19: note: constrained by 
    ...(log truncated; use -l to increase log limit)...
make: *** [Makefile:256: tmp/.ekam-run] Error 1

Let's break some encapsulation. WCGW? Change the 'private' to 'public'. There should be little impact elsewhere as this class definition is inside the .c++ file, not in a header file.

diff --git a/src/sandstorm/sandstorm-http-bridge.c++ b/src/sandstorm/sandstorm-http-bridge.c++
index 65a835a1..ee893e29 100644
--- a/src/sandstorm/sandstorm-http-bridge.c++
+++ b/src/sandstorm/sandstorm-http-bridge.c++
@@ -156,7 +156,7 @@ const HeaderWhitelist RESPONSE_HEADER_WHITELIST(*WebSession::Response::HEADER_WH
 #pragma clang diagnostic pop

 class HttpParser: public sandstorm::Handle::Server,
-                  private http_parser,
+                  public http_parser,
                   private kj::TaskSet::ErrorHandler {
 public:
   HttpParser(sandstorm::ByteStream::Client responseStream)
@@ -944,6 +944,7 @@ public:
     // desires.

     auto parser = kj::heap<HttpParser>(responseStream);
+    if (parser->http_major == 1 && parser->http_minor == 0) isChunked = false;
     auto results = context.getResults();

     return parser->readResponse(*stream).then(
@@ -1043,7 +1045,7 @@ private:
             reqString.slice(0, reqString.size() - 2),
             "Content-Length: ", *l, "\r\n" 
             "\r\n");
-      } else {
+      } else if (isChunked) {
         reqString = kj::str(
             reqString.slice(0, reqString.size() - 2),
             "Transfer-Encoding: chunked\r\n" 

gstrauss wrote:

Let's break some encapsulation. WCGW? Change the 'private' to 'public'. There should be little impact elsewhere as this class definition is inside the .c++ file, not in a header file.

[...]

yeah, I did something similar :

diff --git a/src/sandstorm/sandstorm-http-bridge.c++ b/src/sandstorm/sandstorm-http-bridge.c++
index 65a835a1..b0319378 100644
--- a/src/sandstorm/sandstorm-http-bridge.c++
+++ b/src/sandstorm/sandstorm-http-bridge.c++
@@ -221,6 +221,14 @@ public:
     });
   }

+  unsigned short getResponseHTTPMajor() {
+         return this->http_major;
+  }
+
+  unsigned short getResponseHTTPMinor() {
+         return this->http_minor;
+  }
+
@@ -944,6 +952,16 @@ public:
     // desires.

     auto parser = kj::heap<HttpParser>(responseStream);
+    if (parser->getResponseHTTPMajor() == 1 && parser->getResponseHTTPMinor() == 0) {
+           isChunked = false;
+           KJ_LOG(WARNING, "HTTP/1.0 detected, deactivating chunked encoding");
+    }

I tested many different things, even bypassing the functions to no avail... The resulting HTML is always served with Transfer-Encoding: chunked. So I think this bridge code is only used for a specific purpose instead of all the packets passing through it. I think the actual implementation uses either joyent http in nodejs or something similar (like meteor).

I also have no idea how this logging system works, there is no way to get these outputs from anywhere I looked...

I really doubt a web server like the one provided by nodejs would have such a blatant bug though.

edit:
Oh and by the way I tested your patch and it works great :) (hopefully my little python code helped testing this)

2nd edit:
I meant your lighttpd patch

That the lighttpd patch rejects the response from sandstorm is probably not the solution you were going for when you filed this ticket. At least there is a message in the error log.

sandstorm is truckload of code and dependencies, and as C++, takes forever to build. Sorry that I won't be able to dig further.

gstrauss wrote:

That the lighttpd patch rejects the response from sandstorm is probably not the solution you were going for when you filed this ticket. At least there is a message in the error log.

sandstorm is truckload of code and dependencies, and as C++, takes forever to build. Sorry that I won't be able to dig further.

Notice: This also breaks proxying websocket servers written using python-aiohttp, which also sends an uncoditional Transfer-Encoding: Chunked header:

https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_ws.py#L182

Maybe a more pragmatic solution is just to ignore this header, like it was done earlier?

An HTTP server which responds to an HTTP/1.0 request with an HTTP/1.1 header such as Transfer-Encoding: chunked is broken. FULL STOP. Not lighttpd.

lighttpd is not broken. lighttpd sends HTTP/1.0 requests to backends and expects HTTP/1.0-compliant responses. That's how protocols work. That's how RFC-compliance works. lighttpd will not be made broken by violating the HTTP specifications (your self-described "pragmatic" solution fails the test). If you would like to implement HTTP/1.1 support for mod_proxy, please do so and I'll be happy to review the patches, but not in this ticket.

This bug is invalid, as has been clearly stated multiple times, and further illogical posts here will be ignored.

Based on the original poster's description, sandstorm violates the HTTP/1.0 RFC.

websockets is a different protocol which follows Connection: upgrade, Upgrade: websocket. lighttpd mod_proxy makes an exception and sends HTTP/1.1 requests to backends only when mod_proxy is explicitly configured to pass the Upgrade header, since Upgrade is part of the HTTP/1.1 protocol, not HTTP/1.0. If python-aiohttp response to Upgrade: websocket requires Transfer-Encoding: chunked, then that would be unfortunate, since a successfully upgraded connection will be running the upgraded protocol (e.g. websockets) after a short, affirmative HTTP response that the connection has been upgraded.

Most people using websockets are encapsulating a different RESTful protocol, and can instead use lighttpd mod_wstunnel to be the websocket encapsulation endpoint.

gstrauss wrote:

An HTTP server which responds to an HTTP/1.0 request with an HTTP/1.1 header such as Transfer-Encoding: chunked is broken. FULL STOP.

Completely agreed, my comment was explicitly about websocket and not about sandstorm,

websockets is a different protocol which follows Connection: upgrade, Upgrade: websocket. lighttpd mod_proxy makes an exception and sends HTTP/1.1 requests to backends only when mod_proxy is explicitly configured to pass the Upgrade header, since Upgrade is part of the HTTP/1.1 protocol, not HTTP/1.0. If python-aiohttp response to Upgrade: websocket requires Transfer-Encoding: chunked, then that would be unfortunate, since a successfully upgraded connection will be running the upgraded protocol (e.g. websockets) after a short, affirmative HTTP response that the connection has been upgraded.

Indeed. With upgrade=enabled, connection to an aiohttp based backed works with older lighttpd versions but is broken by the more strict check introduced in 1.4.50 as the response to the Connection: upgrade request contains Transfer-Encoding: chunked

https://github.com/aio-libs/aiohttp/blob/master/aiohttp/web_ws.py#L182

Most people using websockets are encapsulating a different RESTful protocol, and can instead use lighttpd mod_wstunnel to be the websocket encapsulation endpoint.

This backend unfortunately provides a mix of HTTP and websocket, so that is somewhat more cumbersome to do here.

Hi! Any solution for aiohttp-based websockets? The fix looks like to be trivial, just remove the validation of HTTP_HEADER_TRANSFER_ENCODING.

Update: looks like my version of aiohttp is old, they already removed the header (https://github.com/aio-libs/aiohttp/commit/8b8f6b8a8b57db1ba20b402d0fbb0d3d3749d466#diff-3add5885ec3edaeee3d5cf9cef4cae69)