-
Notifications
You must be signed in to change notification settings - Fork 170
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
Conversation
9590e66
to
a0131de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HAve you considered tests with HTTP(S)_PROXY env var?
Added tests for HTTP(S)_PROXY env var. To discuss:For HTTP connections sent through the proxy, the set Proxy-Authorization in the request itself. However when sent over HTTPS, the Proxy-Authorization header must be sent in the CONNECT request as the proxy has no visibility into further tunneled requests. The camel proxy policy is interesting here as we sent over HTTPS but skip CONNECT, so I wonder what is the correct approach here
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking super good!
Missing documentation.
At least, I would update the readme in the policies folder.
t/apicast-policy-http-proxy.t
Outdated
access_by_lua_block { | ||
local assert = require('luassert') | ||
local proxy_auth = ngx.req.get_headers()['Proxy-Authorization'] | ||
assert.equals(proxy_auth, "Basic Zm9vOmJhcg==") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but according to rfc7235#section-4.4,
A proxy MAY relay
the credentials from the client request to the next proxy if that is
the mechanism by which the proxies cooperatively authenticate a given
request.
So asserting in the upstream the existence of the Proxy-Authorization
maybe not be accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, the test should check that the header reaches the proxy instead. Can we add assertions at the proxy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation for http_proxy policy.
I tried extending /t/fixture/proxy.lua to enable proxy authentication checks for each test but couldn't figure out the best way to do so, I ended up printing the header in the debug log and assert the log, quick and easy.
using proxy: http://foo:[email protected]:$TEST_NGINX_HTTP_PROXY_PORT | ||
|
||
|
||
=== TEST 7: using HTTPS proxy for backend with Basic Auth. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already in the camel proxy Readme file
https://github.com/3scale/APIcast/blob/master/gateway/src/apicast/policy/camel/Readme.md?plain=1#L65
There was a problem hiding this comment.
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
|
||
## 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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
Changes looks good to me. I cannot approve because there are git conflicts and some integration tests are failing |
Ack, let me rebase and review the test log. It seems like a bunch of unrelated tests also failed |
Update tests to check if Proxy-Authorization reaches to the proxy server instead of asserting the header in the upstream block.
Proxy urls should all be http, https not supported
96282ef
to
4769da4
Compare
Conflict resolved but I'm still not sure why the tests are failing, it seems very random to me |
Yeah, there are few flaky tests we should be fixing sooner or later. After few tries, no more than 5, they should all pass |
What
Fixes https://issues.redhat.com/browse/THREESCALE-4393
This PR support the use of Basic Auth with forward proxy in the following scenarios:
APIcast <> forward HTTP proxy with Basic Auth <> HTTP upstream
APIcast <> forward HTTP proxy with Basic Auth <> HTTPS upstream
Verification steps
examples/forward-proxy/tinyproxy.conf
APIcast <> forward HTTP proxy with Basic Auth <> HTTP upstream
examples/forward-proxy/apicast-config.json
with the following:docker-compose.forward-proxy.yml
We can see that the the request is going through the proxy
APIcast <> forward HTTP proxy with Basic Auth <> HTTPS upstream
examples/forward-proxy/apicast-config.json
with the following:docker-compose.forward-proxy.yml
We can see that the the request is going through the proxy