fix: allowing for the usage of authenticated http proxies for https endpoints #6352
Conversation
shawkins
requested review from
manusa,
oscerd and
rohanKanojia
as code owners
|
Jetty tests are failing |
| @Override | ||
| public MockResponse peek() { | ||
| return new MockResponse().setSocketPolicy(SocketPolicy.EXPECT_CONTINUE); | ||
| return new MockResponse().setSocketPolicy(defaultSocketPolicy); |
There was a problem hiding this comment.
In #5632 this is removed since it's not necessary for the Vert.x server.
Is this required? I'm not sure how to port these changes to the other PR once we merge this.
There was a problem hiding this comment.
Is this required?
As far as I can tell that is needed to make the https tunnel actually work.
The test doesn't need to fully succeed to establish the tunnel - I've updated it to capture if the authentication header was sent we could just check for that and let the request fail fast.
Should be addressed now. |
closes: fabric8io#6350 Signed-off-by: Steve Hawkins <[email protected]>
|
|
This PR is modifying some of our exposed interfaces. |
|
As @manusa it would be super appreciated if we can get a fix that can be backported in a micro-release. |
Yes, the main problem with the PR as it is right now is that it has source breaking changes (binary too ;)) Since @shawkins has already worked on this maybe he can adjust it to remove all breakages. If not, we will implement a different fix -if possible-. Once this is in, we can proceed with #6253 |
Although unlikely it is possible that users are directly setting up the proxy authentication for their http clients - I wasn't sure how much of an api guarentee we would want to extend there. At least for 7.0+ it seems much more straight-forward to drop the old method.
Leaving the existing signature in a straight-forward way means that we'll need to support parsing the username and password. Not a huge deal, just a pain. I would add that specifically to the back port. |
|
To clarify things more. Supporting proxyAuthorization is more generally than just supporting basic. However only the OkHttp and JDK client (with an additional system property) seem to handle this situation as expected with https endpoints. Jetty and vertx form their connect requests without consulting the current headers. So this pr is a minimal alignment of support that is common across all the clients, but we do loose the general proxyAuthorization support. Any other authentication types are no longer supported. I'm not aware however of needing to support anything other than basic. So the full range of options for compatiblity are:
|
|
I think it would make sense. But I'm not sure when this could be done. (Cc @vietj ) |
#6371 retains proxyAuthorization and does not yet introduce new method(s). So closing with further work to be done under #6372 #6373
Took more of a look at the changes involved with this and our potential direction here and determined this isn't worth pursuing. We'll go down the path of deprecating proxyAuthorization. |
Description
closes: #6350
Switched from using the full proxy authorization to the username and password - this could be viewed as a breaking change, but it is more straight-forward than adding a routine the parse those values back out.
The jdk client works as is with the existing code, but the system property -Djdk.http.auth.tunneling.disabledSchemes="" must be set for the header to get propogated to the tunneling request so I didn't add that test subclass yet.
The ability to set the socket policy per response ended up not being needed so that can be removed if desired.
Type of change
test, version modification, documentation, etc.)
Checklist