Minor refactor to avoid Proxy-Authorization leak

3scale · Sep 27, 2023 · 5c0edbc · 5c0edbc
1 parent d3b86b9
commit 5c0edbc
Show file tree

Hide file tree

Showing 5 changed files with 119 additions and 32 deletions.
diff --git a/gateway/src/apicast/http_proxy.lua b/gateway/src/apicast/http_proxy.lua
@@ -82,7 +82,7 @@ local function absolute_url(uri)
     )
 end
 
-local function forward_https_request(proxy_uri, uri, skip_https_connect)
+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()
 
@@ -102,7 +102,8 @@ local function forward_https_request(proxy_uri, uri, skip_https_connect)
         -- 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_uri = proxy_uri,
+        proxy_auth = proxy_auth
     }
 
     if not request.body then
@@ -156,15 +157,20 @@ end
 
 function _M.request(upstream, proxy_uri)
     local uri = upstream.uri
+    local proxy_auth
 
-    if not ngx.var.http_proxy_authorization then
-        if proxy_uri.user or proxy_uri.password then
-            local proxy_auth = "Basic " .. ngx.encode_base64(concat({ proxy_uri.user or '', proxy_uri.password or '' }, ':'))
-            ngx.req.set_header("Proxy-Authorization", proxy_auth)
-        end
+    if proxy_uri.user or proxy_uri.password then
+        proxy_auth = "Basic " .. ngx.encode_base64(concat({ proxy_uri.user or '', proxy_uri.password or '' }, ':'))
     end
 
     if uri.scheme == 'http' then -- rewrite the request to use http_proxy
+        -- Only set "Proxy-Authorization" when sending HTTP request. When sent over HTTPS,
+        -- the `Proxy-Authorization` header must be sent in the CONNECT request as the proxy has
+        -- no visibility into the tunneled request.
+        if not ngx.var.http_proxy_authorization and proxy_auth then
+            ngx.req.set_header("Proxy-Authorization", proxy_auth)
+        end
+
         local err
         local host = upstream:set_host_header()
         upstream:use_host_header(host)
@@ -177,7 +183,7 @@ function _M.request(upstream, proxy_uri)
         return
     elseif uri.scheme == 'https' then
         upstream:rewrite_request()
-        forward_https_request(proxy_uri, uri, upstream.skip_https_connect)
+        forward_https_request(proxy_uri, proxy_auth, uri, upstream.skip_https_connect)
         return ngx.exit(ngx.OK) -- terminate phase
     else
         ngx.log(ngx.ERR, 'could not connect to proxy: ',  proxy_uri, ' err: ', 'invalid request scheme')

diff --git a/gateway/src/resty/http/proxy.lua b/gateway/src/resty/http/proxy.lua
@@ -61,7 +61,7 @@ local function _connect_proxy_https(httpc, request, host, port)
         path = format('%s:%s', host, port or default_port(uri)),
         headers = {
             ['Host'] = request.headers.host or format('%s:%s', uri.host, default_port(uri)),
-            ['Proxy-Authorization'] = request.headers["Proxy-Authorization"] or ''
+            ['Proxy-Authorization'] = request.proxy_auth or ''
         }
     })
     if not res then return nil, err end

diff --git a/t/apicast-policy-camel.t b/t/apicast-policy-camel.t
@@ -389,7 +389,7 @@ using proxy: http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT
             "name": "apicast.policy.apicast"
           },
           {
-            "name": "apicast.policy.http_proxy",
+            "name": "apicast.policy.camel",
             "configuration": {
                 "all_proxy": "http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT"
             }
@@ -425,7 +425,6 @@ using proxy: http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT
 
 
 === TEST 7: using HTTPS proxy for backend with Basic Auth.
---- ONLY
 --- init eval
 $Test::Nginx::Util::PROXY_SSL_PORT = Test::APIcast::get_random_port();
 $Test::Nginx::Util::ENDPOINT_SSL_PORT = Test::APIcast::get_random_port();
@@ -436,7 +435,7 @@ $Test::Nginx::Util::ENDPOINT_SSL_PORT = Test::APIcast::get_random_port();
     {
       "backend_version":  1,
       "proxy": {
-        "api_backend": "https://localhost:$Test::Nginx::Util::ENDPOINT_SSL_PORT",
+        "api_backend": "https://127.0.0.1:$Test::Nginx::Util::ENDPOINT_SSL_PORT",
         "proxy_rules": [
           { "pattern": "/test", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
         ],
@@ -476,15 +475,8 @@ EOF
     access_by_lua_block {
       assert = require('luassert')
       local proxy_auth = ngx.req.get_headers()['Proxy-Authorization']
-      assert.equals(proxy_auth, "Basic Zm9vOmJhcg==")
-
-      assert.equal('https', ngx.var.scheme)
-      assert.equal('$Test::Nginx::Util::ENDPOINT_SSL_PORT', ngx.var.server_port)
-      assert.equal('localhost', ngx.var.ssl_server_name)
-      assert.equal(ngx.var.request_uri, '/test?user_key=test3')
+      assert.falsy(proxy_auth)
 
-      local host = ngx.req.get_headers()["Host"]
-      assert.equal(host, 'localhost:$Test::Nginx::Util::ENDPOINT_SSL_PORT')
       ngx.say("yay, endpoint backend")
 
     }
@@ -507,9 +499,6 @@ server {
 EOF
 --- request
 GET /test?user_key=test3
---- more_headers
-User-Agent: Test::APIcast::Blackbox
-ETag: foobar
 --- error_code: 200
 --- user_files fixture=tls.pl eval
 --- error_log eval

diff --git a/t/apicast-policy-http-proxy.t b/t/apicast-policy-http-proxy.t
@@ -244,13 +244,11 @@ server_name test-upstream.lvh.me;
 GET /?user_key=value
 --- error_code: 200
 --- error_log env
-proxy request: CONNECT test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT HTTP/1.1
---- error_log env
 using proxy: http://foo:bar@127.0.0.1:$TEST_NGINX_HTTP_PROXY_PORT
 
 
 === TEST 5: using all_proxy with Basic Auth
---- configuration
+--- configuration random_port env
 {
   "services": [
     {
@@ -297,8 +295,6 @@ server_name test-upstream.lvh.me;
 GET /?user_key=value
 --- error_code: 200
 --- error_log env
-proxy request: CONNECT test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT HTTP/1.1
---- error_log env
 using proxy: http://foo:bar@127.0.0.1:$TEST_NGINX_HTTP_PROXY_PORT
 
 
@@ -347,16 +343,16 @@ location /test {
     echo_end;
 
     access_by_lua_block {
-      assert = require('luassert')
+      local assert = require('luassert')
       local proxy_auth = ngx.req.get_headers()['Proxy-Authorization']
-      assert.equals(proxy_auth, "Basic Zm9vOmJhcg==")
+      assert.falsy(proxy_auth)
     }
 }
 --- request
 GET /test?user_key=test3
 --- error_code: 200
---- error_log env
-proxy request: CONNECT test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT HTTP/1.1
 --- user_files fixture=tls.pl eval
 --- error_log env
 using proxy: http://foo:bar@127.0.0.1:$TEST_NGINX_HTTP_PROXY_PORT
+proxy request: CONNECT test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT HTTP/1.1
+got header line: Proxy-Authorization: Basic Zm9vOmJhcg==
diff --git a/t/http-proxy.t b/t/http-proxy.t
@@ -1227,3 +1227,99 @@ proxy request: CONNECT test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT HTTP/1.1
 --- no_error_log
 [error]
 --- user_files fixture=tls.pl eval
+
+
+=== TEST 23: upstream API connection uses http proxy with BasicAuth
+--- env eval
+(
+  "http_proxy" => "http://foo:bar\@127.0.0.1:$ENV{TEST_NGINX_HTTP_PROXY_PORT}",
+  'BACKEND_ENDPOINT_OVERRIDE' => "http://test_backend.lvh.me:$ENV{TEST_NGINX_SERVER_PORT}"
+)
+--- configuration
+{
+  "services": [
+    {
+      "backend_version":  1,
+      "proxy": {
+        "api_backend": "http://test-upstream.lvh.me:$TEST_NGINX_SERVER_PORT",
+        "proxy_rules": [
+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ]
+      }
+    }
+  ]
+}
+--- backend
+server_name test_backend.lvh.me;
+  location /transactions/authrep.xml {
+    content_by_lua_block {
+      ngx.exit(ngx.OK)
+    }
+  }
+--- upstream
+server_name test-upstream.lvh.me;
+  location / {
+    access_by_lua_block {
+      local assert = require('luassert')
+      local proxy_auth = ngx.req.get_headers()['Proxy-Authorization']
+      assert.equals(proxy_auth, "Basic Zm9vOmJhcg==")
+    }
+  }
+--- request
+GET /?user_key=value
+--- error_code: 200
+--- error_log env
+using proxy: http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT
+--- no_error_log
+[error]
+
+
+=== TEST 24: upstream API connection uses proxy for https with BasicAuth
+--- env eval
+(
+  "https_proxy" => "http://foo:bar\@127.0.0.1:$ENV{TEST_NGINX_HTTP_PROXY_PORT}",
+  'BACKEND_ENDPOINT_OVERRIDE' => "http://test_backend.lvh.me:$ENV{TEST_NGINX_SERVER_PORT}"
+)
+--- configuration random_port env
+{
+  "services": [
+    {
+      "backend_version":  1,
+      "proxy": {
+        "api_backend": "https://test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT",
+        "proxy_rules": [
+          { "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
+        ]
+      }
+    }
+  ]
+}
+--- backend
+server_name test_backend.lvh.me;
+
+  location /transactions/authrep.xml {
+    content_by_lua_block {
+      ngx.exit(ngx.OK)
+    }
+  }
+--- upstream env
+server_name test-upstream.lvh.me;
+
+listen $TEST_NGINX_RANDOM_PORT ssl;
+ssl_certificate $TEST_NGINX_SERVER_ROOT/html/server.crt;
+ssl_certificate_key $TEST_NGINX_SERVER_ROOT/html/server.key;
+location / {
+    echo_foreach_split '\r\n' $echo_client_request_headers;
+    echo $echo_it;
+    echo_end;
+}
+--- request
+GET /test?user_key=test3
+--- error_code: 200
+--- error_log env
+using proxy: http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT
+proxy request: CONNECT test-upstream.lvh.me:$TEST_NGINX_RANDOM_PORT HTTP/1.1
+got header line: Proxy-Authorization: Basic Zm9vOmJhcg==
+--- no_error_log
+[error]
+--- user_files fixture=tls.pl eval