Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[THREESCALE-4393] Add support to use Basic Authentication with the forward proxy #1409

Merged
merged 8 commits into from
Nov 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
### 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)
### Added

* Add support to use Basic Authentication with the forward proxy. [PR #1409](https://github.com/3scale/APIcast/pull/1409)

## [3.14.0] 2023-07-25

Expand Down
23 changes: 20 additions & 3 deletions gateway/src/apicast/http_proxy.lua
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ local resty_resolver = require 'resty.resolver'
local round_robin = require 'resty.balancer.round_robin'
local http_proxy = require 'resty.http.proxy'
local file_reader = require("resty.file").file_reader
local concat = table.concat

local _M = { }

Expand Down Expand Up @@ -81,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()

Expand All @@ -101,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
Expand Down Expand Up @@ -155,8 +157,23 @@ end

function _M.request(upstream, proxy_uri)
local uri = upstream.uri
local proxy_auth

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.
--
-- Also DO NOT set the header if using the camel proxy to avoid unintended leak of
-- Proxy-Authorization header in requests
if not ngx.var.http_proxy_authorization and proxy_auth and not upstream.skip_https_connect then
ngx.req.set_header("Proxy-Authorization", proxy_auth)
end

local err
local host = upstream:set_host_header()
upstream:use_host_header(host)
Expand All @@ -169,7 +186,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')
Expand Down
14 changes: 9 additions & 5 deletions gateway/src/apicast/policy/http_proxy/Readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ used.

## Configuration

The policy expect the URLS following the `http://[<username>[:<passwd>]@]<host>[:<port>]` format, e.g.:

```
"policy_chain": [
{
Expand All @@ -51,23 +53,25 @@ used.
{
"name": "apicast.policy.http_proxy",
"configuration": {
"all_proxy": "http://192.168.15.103:8888/",
"https_proxy": "https://192.168.15.103:8888/",
"http_proxy": "https://192.168.15.103:8888/"
"all_proxy": "http://foo:bar@192.168.15.103:8888/",
"https_proxy": "http://foo:bar@192.168.15.103:8888/",
"http_proxy": "http://foo:bar@192.168.15.103:8888/"
}
}
]
```

- If http_proxy or https_proxy is not defined the all_proxy will be taken.
- If http_proxy or https_proxy is not defined the all_proxy will be taken.
- The policy supports for proxy authentication via the `<username>` and `<passwd>` options.
- The `<username>` and `<passwd>` are optional, all other components are required.

## Caveats

- This policy will disable all load-balancing policies and traffic will be
always send to the proxy.
- In case of HTTP_PROXY, HTTPS_PROXY or ALL_PROXY parameters are defined, this
policy will overwrite those values.
- Proxy connection does not support authentication.
- 3scale currently does not support connecting to an HTTP proxy via TLS. For this reason, the scheme of the HTTPS_PROXY value is restricted to http.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3scale currently does not support connecting to an HTTP proxy via TLS. For this reason, the scheme of the HTTPS_PROXY value is restricted to http.

IMO not very accurate

3scale currently does not support connecting to an HTTP proxy via TLS

Correct.

For this reason, the scheme of the HTTPS_PROXY value is restricted to http.

All proxy URLs should be http based, never https based.

If upstream is http -> then HTTP_PROXY is used by apicast

if upstream is https -> then HTTPS_PROXY is used by apicast

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All proxy URLs should be http based, never https based.

That's exactly what I meant when I said For this reason, the HTTPS_PROXY value scheme is restricted to http. Maybe that causes more confusion, maybe we should delete that sentence?



## Example Use case
Expand Down
13 changes: 9 additions & 4 deletions gateway/src/resty/http/proxy.lua
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,22 @@ local function _connect_proxy_https(httpc, request, host, port)

local uri = request.uri

local ok, err = httpc:request({
local res, err = httpc:request({
method = 'CONNECT',
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.proxy_auth or ''
}
})
if not ok then return nil, err end
if not res then return nil, err end

ok, err = httpc:ssl_handshake(nil, uri.host, request.ssl_verify)
if not ok then return nil, err end
if res.status < 200 or res.status > 299 then
return nil, "failed to establish a tunnel through a proxy: " .. res.status
end

res, err = httpc:ssl_handshake(nil, uri.host, request.ssl_verify)
if not res then return nil, err end

return httpc
end
Expand Down
191 changes: 191 additions & 0 deletions t/apicast-policy-camel.t
Original file line number Diff line number Diff line change
Expand Up @@ -315,3 +315,194 @@ ETag: foobar
<<EOF
using proxy: http://127.0.0.1:$Test::Nginx::Util::PROXY_SSL_PORT,
EOF


=== TEST 5: API backend connection uses http proxy with Basic Auth
Check that the Proxy Authorization header is not sent
--- configuration
{
"services": [
{
"id": 42,
"backend_version": 1,
"backend_authentication_type": "service_token",
"backend_authentication_value": "token-value",
"proxy": {
"api_backend": "http://test-upstream.lvh.me:$TEST_NGINX_SERVER_PORT/",
"proxy_rules": [
{ "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
],
"policy_chain": [
{
"name": "apicast.policy.apicast"
},
{
"name": "apicast.policy.camel",
"configuration": {
"http_proxy": "http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT"
}
}
]
}
}
]
}
--- backend
location /transactions/authrep.xml {
content_by_lua_block {
ngx.exit(ngx.OK)
}
}
--- upstream
server_name test-upstream.lvh.me;
location / {
access_by_lua_block {
assert = require('luassert')
local proxy_auth = ngx.req.get_headers()['Proxy-Authorization']
assert.falsy(proxy_auth)
ngx.say("yay, api backend")
}
}
--- request
GET /?user_key=value
--- response_body
yay, api backend
--- error_code: 200
--- error_log env
using proxy: http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT

=== TEST 6: API backend using all_proxy with Basic Auth
Check that the Proxy Authorization header is not sent
--- configuration
{
"services": [
{
"id": 42,
"backend_version": 1,
"backend_authentication_type": "service_token",
"backend_authentication_value": "token-value",
"proxy": {
"api_backend": "http://test-upstream.lvh.me:$TEST_NGINX_SERVER_PORT/",
"proxy_rules": [
{ "pattern": "/", "http_method": "GET", "metric_system_name": "hits", "delta": 2 }
],
"policy_chain": [
{
"name": "apicast.policy.apicast"
},
{
"name": "apicast.policy.camel",
"configuration": {
"all_proxy": "http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT"
}
}
]
}
}
]
}
--- backend
location /transactions/authrep.xml {
content_by_lua_block {
ngx.exit(ngx.OK)
}
}
--- upstream
server_name test-upstream.lvh.me;
location / {
access_by_lua_block {
assert = require('luassert')
local proxy_auth = ngx.req.get_headers()['Proxy-Authorization']
assert.falsy(proxy_auth)
ngx.say("yay, api backend")
}
}
--- request
GET /?user_key=value
--- response_body
yay, api backend
--- error_code: 200
--- error_log env
using proxy: http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT


=== TEST 7: using HTTPS proxy for backend with Basic Auth.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how this works with the camel proxy when upstream is TLS.

You are adding the Proxy-Authorization header to the method _connect_proxy_https. But precisely, for the camel proxies, that method is not being used. Instead, _connect_tls_direct is being used. Correct me if I am wrong.

Does it even make sense to add authentication support on camel proxy when upstream is TLS?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I see your comment

The camel proxy policy is interesting here as we sent over HTTPS but skip CONNECT, so I wonder what is the correct approach here

    Set Proxy-Authorization header in the original request
    Don't set the header and update the documentation saying that camel proxy policy is incompatible with proxy authentication

Regarding Set Proxy-Authorization header in the original request, IMO it does not make sense to me. The header would not even be read by camel proxy and would end up in the upstream.

Regarding Don't set the header and update the documentation saying that camel proxy policy is incompatible with proxy authentication, we may need to investigate further how camel proxy works and if they define a use case for proxy auth on TLS connections. Maybe here? https://github.com/zregvart/camel-netty-proxy They say it is only an example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, the integration tests for the TLS upstream does not make sense as it is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I talked to few friends from the FuseSource team and no one really knows what's called a camel proxy.

The example you provided is built using Camel netty-http. And checking the source code of netty-http it looks like it doesn't support proxy authentication and CONNECT out of the box.

With that said, I believe the camel proxy's behavior will depend on the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of camel proxy is to proxy the request without CONNECT, otherwise they can just use the normal proxy.

I would vote to not support authentication with camel proxy. Let me know what you think and I will push a new patch

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is being supported feature https://access.redhat.com/documentation/en-us/red_hat_3scale_api_management/2.13/html/administering_the_api_gateway/apicast-policies#camel-service_standard-policies

IDK how many customers are using it, tho

What I would do is to add doc (a note somewhere) saying that camel proxying does not support basic client auth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixed the camel proxy integration tests

Check that the Proxy Authorization header is not sent
--- init eval
$Test::Nginx::Util::PROXY_SSL_PORT = Test::APIcast::get_random_port();
$Test::Nginx::Util::ENDPOINT_SSL_PORT = Test::APIcast::get_random_port();
--- configuration random_port env eval
<<EOF
{
"services": [
{
"backend_version": 1,
"proxy": {
"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 }
],
"policy_chain": [
{
"name": "apicast.policy.apicast"
},
{
"name": "apicast.policy.camel",
"configuration": {
"https_proxy": "http://foo:bar\@127.0.0.1:$Test::Nginx::Util::PROXY_SSL_PORT"
}
}
]
}
}
]
}
EOF
--- backend
location /transactions/authrep.xml {
content_by_lua_block {
ngx.exit(ngx.OK)
}
}
--- upstream eval
<<EOF
# Endpoint config
listen $Test::Nginx::Util::ENDPOINT_SSL_PORT ssl;

ssl_certificate $Test::Nginx::Util::ServRoot/html/server.crt;
ssl_certificate_key $Test::Nginx::Util::ServRoot/html/server.key;

server_name _ default_server;

location /test {
access_by_lua_block {
ngx.say("yay, endpoint backend")

}
}
}
server {
# Proxy config
listen $Test::Nginx::Util::PROXY_SSL_PORT ssl;

ssl_certificate $Test::Nginx::Util::ServRoot/html/server.crt;
ssl_certificate_key $Test::Nginx::Util::ServRoot/html/server.key;


server_name _ default_server;

location ~ /.* {
proxy_http_version 1.1;
proxy_pass https://\$http_host;
}
EOF
--- request
GET /test?user_key=test3
--- error_code: 200
--- user_files fixture=tls.pl eval
--- error_log eval
<<EOF
using proxy: http://foo:bar\@127.0.0.1:$Test::Nginx::Util::PROXY_SSL_PORT,
EOF
--- no_error_log eval
[qr/\[error\]/, qr/\got header line: Proxy-Authorization: Basic Zm9vOmJhcg==/]
Loading