Project

General

Profile

Bug #2913

Reverse proxy does not work with sandstorm

Added by blowfist about 1 month ago. Updated about 1 month ago.

Status:
Invalid
Priority:
Normal
Assignee:
-
Category:
core
Target version:
Start date:
2018-10-07
Due date:
% Done:

0%

Estimated time:
Missing in 1.5.x:

Description

When trying out sandstorm with lighttpd and using it's reverse proxy feature, the end result gives an encoding error in all the browsers (it didn't work at all). It turns out that the reverse proxy does not correctly handle "Transfer-Encoding: chunked" packets from the backend correctly. This header value is dropped and the client receives the rest of the packet as is (encoded as chunks).

I have pin pointed where in the code creates this issue and attached the patch with a fix. I also wrote a minimal python3 http server to better recreate this issue. And also included a minimal configuration file.

With this simple fix, lighttpd can successfully reverse proxy to sandstorm (see https://sandstorm.io) (and it works wonderfully)!

0001-core-Fixed-an-issue-when-a-proxied-backend-used-chun.patch (1005 Bytes) 0001-core-Fixed-an-issue-when-a-proxied-backend-used-chun.patch Patch with the fix blowfist, 2018-10-07 02:42
light.conf (796 Bytes) light.conf lighttpd minimal configuration to recreate the issue blowfist, 2018-10-07 02:43
serverEncodeChunked.py (1.16 KB) serverEncodeChunked.py minimal python3 backend http server to recreate the issue blowfist, 2018-10-07 02:43

Associated revisions

Revision d8259667 (diff)
Added by gstrauss about 1 month ago

[core] reject Transfer-Encoding from proxy (#2913)

reject Transfer-Encoding from backend for mod_proxy.
mod_proxy currently sends HTTP/1.0 requests to the backend,
for which Transfer-Encoding: chunked is not a valid response header.
Additionally, there is no value to Transfer-Encoding: chunked from
backend since lighttpd mod_proxy sends HTTP/1.0 request along with
Connection: close, so the backend closing the socket is the end of
the response from the backend.

x-ref:
"Reverse proxy does not work with sandstorm"
https://redmine.lighttpd.net/issues/2913

History

#1

Updated by gstrauss about 1 month ago

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.

#2

Updated by gstrauss about 1 month ago

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 lighttpy 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.

#3

Updated by gstrauss about 1 month ago

  • Status changed from New to Invalid

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.

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.

diff --git a/src/sandstorm/sandstorm-http-bridge.c++ b/src/sandstorm/sandstorm-http-bridge.c++
index 65a835a1..d921c944 100644
--- a/src/sandstorm/sandstorm-http-bridge.c++
+++ b/src/sandstorm/sandstorm-http-bridge.c++
@@ -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 +1044,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" 

#4

Updated by gstrauss about 1 month ago

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)

#5

Updated by blowfist about 1 month ago

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)

#6

Updated by gstrauss about 1 month ago

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.

#7

Updated by blowfist about 1 month ago

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
#8

Updated by gstrauss about 1 month ago

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" 
#9

Updated by blowfist about 1 month ago

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

#10

Updated by gstrauss about 1 month ago

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.

Also available in: Atom