Merge pull request #1403 from tkan145/THREESCALE-9542-chunked-request

[THREESCALE-9542] Part 2: Add support to proxy request with Transfer-Encoding: chunked

Author: tkan145

CHANGELOG.md

@@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - Fixed CVE-2023-44487 (HTTP/2 Rapid Reset) [PR #1417](https://github.com/3scale/apicast/pull/1417) [THREESCALE-10224](https://issues.redhat.com/browse/THREESCALE-10224)
 
+- Fixed issue where the proxy policy could not handle requests with "Transfer-Encoding: chunked" header [PR #1403](https://github.com/3scale/APIcast/pull/1403) [THREESCALE-9542](https://issues.redhat.com/browse/THREESCALE-9542)
+
 ### Added
 
 - Detect number of CPU shares when running on Cgroups V2 [PR #1410](https://github.com/3scale/apicast/pull/1410) [THREESCALE-10167](https://issues.redhat.com/browse/THREESCALE-10167)

gateway/src/apicast/http_proxy.lua

@@ -1,13 +1,29 @@
 local format = string.format
+local tostring = tostring
+local ngx_flush = ngx.flush
+local ngx_get_method = ngx.req.get_method
+local ngx_http_version = ngx.req.http_version
+local ngx_send_headers = ngx.send_headers
 
 local resty_url = require "resty.url"
 local resty_resolver = require 'resty.resolver'
 local http_proxy = require 'resty.http.proxy'
 local file_reader = require("resty.file").file_reader
+local file_size = require("resty.file").file_size
+local client_body_reader = require("resty.http.request_reader").get_client_body_reader
+local send_response = require("resty.http.response_writer").send_response
 local concat = table.concat
 
 local _M = { }
 
+local http_methods_with_body = {
+  POST = true,
+  PUT = true,
+  PATCH = true
+}
+
+local DEFAULT_CHUNKSIZE = 32 * 1024
+
 function _M.reset()
     _M.resolver = resty_resolver
     _M.http_backend = require('resty.http_ng.backend.resty')
@@ -41,15 +57,50 @@ local function absolute_url(uri)
     )
 end
 
-local function forward_https_request(proxy_uri, proxy_auth, uri, skip_https_connect)
-    -- This is needed to call ngx.req.get_body_data() below.
-    ngx.req.read_body()
+local function forward_https_request(proxy_uri, uri, proxy_opts)
+    local body, err
+    local sock
+    local opts = proxy_opts or {}
+    local req_method = ngx_get_method()
+    local encoding = ngx.req.get_headers()["Transfer-Encoding"]
+    local is_chunked = encoding and encoding:lower() == "chunked"
+    local content_type = ngx.req.get_headers()["Content-Type"]
+    local content_type_is_urlencoded = content_type and content_type:lower() == "application/x-www-form-urlencoded"
+    local raw = false
+
+    if http_methods_with_body[req_method] then
+
+      -- When the content type is "application/x-www-form-urlencoded" the body is always pre-read.
+      -- See: gateway/src/apicast/configuration/service.lua:214
+      --
+      -- Due to this, ngx.req.socket() will fail with "request body already exists" error or return
+      -- socket but hang on read in case of raw socket. Therefore, we only retrieve body from the
+      -- socket if the content type is not "application/x-www-form-urlencoded"
+      if opts.request_unbuffered and ngx_http_version() == 1.1 and not content_type_is_urlencoded then
+        if is_chunked then
+            -- The default ngx reader does not support chunked request
+            -- so we will need to get the raw request socket and manually
+            -- decode the chunked request
+            sock, err = ngx.req.socket(true)
+            raw = true
+        else
+            sock, err = ngx.req.socket()
+        end
 
-    local request = {
-        uri = uri,
-        method = ngx.req.get_method(),
-        headers = ngx.req.get_headers(0, true),
-        path = format('%s%s%s', ngx.var.uri, ngx.var.is_args, ngx.var.query_string or ''),
+        if not sock then
+            ngx.log(ngx.ERR, "unable to obtain request socket: ", err)
+            return ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
+        end
+
+        body = client_body_reader(sock, DEFAULT_CHUNKSIZE, is_chunked)
+      else
+        -- TODO: Due to ngx.req.read_body(). The current implementation will not work with grpc service
+        -- See: https://github.com/3scale/APIcast/pull/1419
+        -- Should we get the body from socket by default and only read buffered body if
+        -- "Content-Type: application/x-www-form-urlencoded"?
+        --
+        -- This is needed to call ngx.req.get_body_data() below.
+        ngx.req.read_body()
 
         -- We cannot use resty.http's .get_client_body_reader().
         -- In POST requests with HTTPS, the result of that call is nil, and it
@@ -60,26 +111,55 @@ local function forward_https_request(proxy_uri, proxy_auth, uri, skip_https_conn
         -- read and need to be cached in a local file. This request will return
         -- nil, so after this we need to read the temp file.
         -- https://github.com/openresty/lua-nginx-module#ngxreqget_body_data
-        body = ngx.req.get_body_data(),
-        proxy_uri = proxy_uri,
-        proxy_auth = proxy_auth
-    }
+        body = ngx.req.get_body_data()
+
+        if not body then
+            local temp_file_path = ngx.req.get_body_file()
+            ngx.log(ngx.INFO, "HTTPS Proxy: Request body is bigger than client_body_buffer_size, read the content from path='", temp_file_path, "'")
+
+            if temp_file_path then
+              body, err = file_reader(temp_file_path)
+              if err then
+                ngx.log(ngx.ERR, "HTTPS proxy: Failed to read temp body file, err: ", err)
+                ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
+              end
+
+              if is_chunked then
+                -- If the body is smaller than "client_boby_buffer_size" the Content-Length header is
+                -- set by openresty based on the size of the buffer. However, when the body is rendered
+                -- to a file, we will need to calculate and manually set the Content-Length header based
+                -- on the file size
+                local contentLength, err = file_size(temp_file_path)()
+                if err then
+                    ngx.log(ngx.ERR, "HTTPS proxy: Failed to set content length, err: ", err)
+                    ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
+                end
+
+                ngx.req.set_header("Content-Length", tostring(contentLength))
+              end
+            end
+        end
 
-    if not request.body then
-        local temp_file_path = ngx.req.get_body_file()
-        ngx.log(ngx.INFO, "HTTPS Proxy: Request body is bigger than client_body_buffer_size, read the content from path='", temp_file_path, "'")
-
-        if temp_file_path then
-          local body, err = file_reader(temp_file_path)
-          if err then
-            ngx.log(ngx.ERR, "HTTPS proxy: Failed to read temp body file, err: ", err)
-            ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR)
-          end
-          request.body = body
+        -- The whole request is buffered with chunked encoding removed, so remove the Transfer-Encoding: chunked
+        -- header, otherwise the upstream won't be able to read the body as it expected chunk encoded
+        -- body
+        if is_chunked then
+            ngx.req.set_header("Transfer-Encoding", nil)
         end
+      end
     end
 
-    local httpc, err = http_proxy.new(request, skip_https_connect)
+    local request = {
+        uri = uri,
+        method = ngx.req.get_method(),
+        headers = ngx.req.get_headers(0, true),
+        path = format('%s%s%s', ngx.var.uri, ngx.var.is_args, ngx.var.query_string or ''),
+        body = body,
+        proxy_uri = proxy_uri,
+        proxy_auth = opts.proxy_auth
+    }
+
+    local httpc, err = http_proxy.new(request, opts.skip_https_connect)
 
     if not httpc then
         ngx.log(ngx.ERR, 'could not connect to proxy: ',  proxy_uri, ' err: ', err)
@@ -91,8 +171,16 @@ local function forward_https_request(proxy_uri, proxy_auth, uri, skip_https_conn
     res, err = httpc:request(request)
 
     if res then
-        httpc:proxy_response(res)
-        httpc:set_keepalive()
+        if opts.request_unbuffered and raw then
+            local bytes, err = send_response(sock, res, DEFAULT_CHUNKSIZE)
+            if not bytes then
+                ngx.log(ngx.ERR, "failed to send response: ", err)
+                return sock:send("HTTP/1.1 502 Bad Gateway")
+            end
+        else
+            httpc:proxy_response(res)
+            httpc:set_keepalive()
+        end
     else
         ngx.log(ngx.ERR, 'failed to proxy request to: ', proxy_uri, ' err : ', err)
         return ngx.exit(ngx.HTTP_BAD_GATEWAY)
@@ -145,7 +233,13 @@ function _M.request(upstream, proxy_uri)
         return
     elseif uri.scheme == 'https' then
         upstream:rewrite_request()
-        forward_https_request(proxy_uri, proxy_auth, uri, upstream.skip_https_connect)
+        local proxy_opts = {
+            proxy_auth = proxy_auth,
+            skip_https_connect = upstream.skip_https_connect,
+            request_unbuffered = upstream.request_unbuffered
+        }
+
+        forward_https_request(proxy_uri, uri, proxy_opts)
         return ngx.exit(ngx.OK) -- terminate phase
     else
         ngx.log(ngx.ERR, 'could not connect to proxy: ',  proxy_uri, ' err: ', 'invalid request scheme')

gateway/src/apicast/policy/request_unbuffered/README.md

@@ -1,14 +1,97 @@
 # APICast Request Unbuffered
 
-This policy allows to disable request buffering
+## Description
+
+When enable this policy will dymanically sets the [`proxy_request_buffering: off`](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_request_buffering
+) directive per service.
+
+## Technical details
+
+By default, NGINX reads the entire request body into memory or buffers large requests to disk before forwarding them to the upstream server. Reading bodies can become expensive, especially when sending requests containing large payloads.
+
+For example, when the client sends 10GB, NGINX will buffer the entire 10GB to disk before sending anything to the upstream server.
+
+When the `request_unbuffered` is in the chain, request buffering is disabled, sending the request body to the proxied server immediately upon receiving it. This can help minimize time spent sending data to a service and disk I/O for requests with big body. However, there are caveats and corner cases applied, [**Caveats**](#caveats)
+
+The policy also provides a consistent behavior across multiple scenarios like:
+
+```
+- APIcast <> upstream HTTP 1.1 plain
+- APIcast <> upstream TLS
+- APIcast <> HTTP Proxy (env var) <> upstream HTTP 1.1 plain
+- APIcast <> HTTP Proxy (policy) <> upstream HTTP 1.1 plain
+- APIcast <> HTTP Proxy (camel proxy) <> upstream HTTP 1.1 plain
+- APIcast <> HTTP Proxy (env var) <> upstream TLS
+- APIcast <> HTTP Proxy (policy) <> upstream TLS
+- APIcast <> HTTP Proxy (camel proxy) <> upstream TLS
+```
+
+## Why don't we also support disable response buffering?
+
+The response buffering is enabled by default in NGINX (the [`proxy_buffering: on`]() directive). It does this to shield the backend against slow clients ([slowloris attack](https://en.wikipedia.org/wiki/Slowloris_(computer_security))).
+
+If the `proxy_buffering` is disabled, the upstream server keeps the connection open until all data is received by the client. NGINX [advises](https://www.nginx.com/blog/avoiding-top-10-nginx-configuration-mistakes/#proxy_buffering-off) against disabling `proxy_buffering` as it will potentially waste upstream server resources.
+
+## Why does upstream receive a "Content-Length" header when the original request is sent with "Transfer-Encoding: chunked"
+
+For a request with "small" body that fits into [`client_body_buffer_size`](https://nginx.org/en/docs/http/ngx_http_core_module.html#client_body_buffer_size) and with header "Transfer-Encoding: chunked", NGINX will always read and know the length of the body. Then it will send the request to upstream with the "Content-Length" header.
+
+If a client uses chunked transfer encoding with HTTP/1.0, NGINX will always buffer the request body
 
 ## Example configuration
 
 ```
-{
-    "name": "request_unbuffered",
-    "version": "builtin",
-    "configuration": {}
-}
+"policy_chain": [
+    {
+        "name": "request_unbuffered",
+        "version": "builtin",
+    },
+    {
+      "name": "apicast.policy.apicast"
+    }
+]
+```
+
+Use with Proxy policy
+
+```
+"policy_chain": [
+    {
+        "name": "request_unbuffered",
+        "version": "builtin",
+    },
+    {
+      "name": "apicast.policy.http_proxy",
+      "configuration": {
+          "all_proxy": "http://foo:[email protected]:8888/",
+          "https_proxy": "http://192.168.15.103:8888/",
+          "http_proxy": "http://192.168.15.103:8888/"
+      }
+    }
+]
+```
+
+Use with Camel Proxy policy
+
+```
+"policy_chain": [
+    {
+        "name": "request_unbuffered",
+        "version": "builtin",
+    },
+    {
+      "name": "apicast.policy.camel",
+      "configuration": {
+          "http_proxy": "http://192.168.15.103:8080/",
+          "https_proxy": "http://192.168.15.103:8443/",
+          "all_proxy": "http://192.168.15.103:8080/"
+      }
+    }
+]
 ```
 
+## Caveats
+
+- APIcast allows defining of mapping rules based on request content. For example, `POST /some_path?a_param={a_value}` will match a request like `POST "http://apicast_host:8080/some_path"` with a form URL-encoded body like: `a_param=abc`, requests with `Content-type: application/x-www-form-urlencoded` will always be buffered regardless of the
+  `request_unbuffered` policy is enabled or not.
+- Disable request buffering could potentially expose the backend to [slowloris attack](https://en.wikipedia.org/wiki/Slowloris_(computer_security)). Therefore, we recommend to only use this policy when needed.

gateway/src/apicast/upstream.lua

@@ -241,6 +241,7 @@ function _M:call(context)
           self:set_skip_https_connect_on_proxy();
         end
 
+        self.request_unbuffered = context.request_unbuffered
         http_proxy.request(self, proxy_uri)
     else
         local err = self:rewrite_request()

gateway/src/resty/file.lua

@@ -1,4 +1,5 @@
 local co_yield = coroutine._yield
+local co_wrap = coroutine._wrap
 local open = io.open
 
 local co_wrap_iter = require("resty.coroutines").co_wrap_iter
@@ -28,4 +29,22 @@ function _M.file_reader(filename)
     end)
 end
 
+function _M.file_size(filename)
+    return co_wrap(function()
+        local handle, err = open(filename)
+
+        if err then
+          return nil, err
+        end
+
+        local current = handle:seek()
+        local size = handle:seek("end")
+
+        handle:seek("set", current)
+        handle:close()
+
+        return size
+    end)
+end
+
 return _M