Fixes #10160 - Verify PROXY_AUTHENTICATION is sent to forward proxies #10162
Conversation
Now TunnelRequest.getURI() does not return null, so normalizeRequest() can properly apply the authentication headers. Signed-off-by: Simone Bordet <[email protected]>
|
All tests in |
… copied. * Fixed restore of destination in HttpProxy.HttpProxyClientConnectionFactory.newProxyConnection(): now doing it in the promise rather than in finally block. * Using the proxy destination (not the server's) to send subsequent CONNECT requests in case the first is not replied with 200. Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
Signed-off-by: Simone Bordet <[email protected]>
|
@lorban should be fixed now. |
|
|
||
| private boolean isHTTP2(String protocol) | ||
| { | ||
| return "h2".equalsIgnoreCase(protocol) || "h2c".equalsIgnoreCase(protocol); |
There was a problem hiding this comment.
With this implementation, if serverOrigin only supports h2c and HttpProxy only supports h2 there will be a match. Is that expected?
There was a problem hiding this comment.
Yes, it's expected.
The reason is that if server and proxy speak the same protocol, there is no need to wrap the server protocol into a CONNECT.
h2 is just h2c over TLS, so the protocol is the same.
| Request newRequest = client.copyRequest(request, request.getURI()); | ||
| if (HttpMethod.CONNECT.is(newRequest.getMethod())) | ||
| newRequest.path(request.getPath()); |
There was a problem hiding this comment.
I have a slight preference for something like
| Request newRequest = client.copyRequest(request, request.getURI()); | |
| if (HttpMethod.CONNECT.is(newRequest.getMethod())) | |
| newRequest.path(request.getPath()); | |
| Request newRequest = (HttpMethod.CONNECT.is(request.getMethod())) | |
| ? client.copyRequest(request, request.getPath()); | |
| : client.copyRequest(request, request.getURI()); |
Or if the right copyRequest semantic can't be easily provided, then at least below would be a little clearer to me:
| Request newRequest = client.copyRequest(request, request.getURI()); | |
| if (HttpMethod.CONNECT.is(newRequest.getMethod())) | |
| newRequest.path(request.getPath()); | |
| Request newRequest = client.copyRequest(request, request.getURI()); | |
| if (HttpMethod.CONNECT.is(request.getMethod())) | |
| newRequest.path(request.getPath()); |
There was a problem hiding this comment.
The first proposed change is not correct.
The second proposed change just replaces the variable newRequest with request, which I don't see how it is clearer? The method is not changed by copying the request.
There was a problem hiding this comment.
@sbordet the first proposal is pseudo code: check the method of the original request first and then build the new request appropriately.
The second proposal is clearer to me, as I don't have to do the double take of thinking: oh it is conditional on its own method... oh what is its method, oh it copied it from the original request, ah so it is just the method of the original request. It is clearer just to make it conditional on the method of the original request.
sbordet
deleted the
fix/jetty-10-10160-tunnel-request-proxy-authentication
branch
Now TunnelRequest.getURI() does not return null, so normalizeRequest() can properly apply the authentication headers.