You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by jborgland <gi...@git.apache.org> on 2018/12/12 15:14:13 UTC
[GitHub] httpcomponents-client pull request #123: Better handling of http(s).proxyUse...
GitHub user jborgland opened a pull request:
https://github.com/apache/httpcomponents-client/pull/123
Better handling of http(s).proxyUser and http(s).proxyPassword
Suggested fix for HTTPCLIENT-1955.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jborgland/httpcomponents-client 4.5.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/httpcomponents-client/pull/123.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #123
----
commit 44b60dd691a62e112f9b80390db7db055bb91bd5
Author: Jens Borgland <jb...@...>
Date: 2018-12-12T14:58:39Z
Better handling of http(s).proxyUser and http(s).proxyPassword
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client issue #123: Better handling of http(s).proxyUser and h...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-client/pull/123
@jborgland I will commit the change-set to 4.5.x and master as soon as HC 5.0-beta3 release has been closed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client issue #123: Better handling of http(s).proxyUser and h...
Posted by jborgland <gi...@git.apache.org>.
Github user jborgland commented on the issue:
https://github.com/apache/httpcomponents-client/pull/123
@ok2c thank you! Sorry about the Checkstyle warnings, I was in a bit of hurry and didn't pay attention to Checkstyle not working in my (brand new) environment.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #123: Better handling of http(s).proxyUse...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/123#discussion_r241344569
--- Diff: httpclient/src/main/java/org/apache/http/impl/client/SystemDefaultCredentialsProvider.java ---
@@ -116,25 +116,16 @@ public Credentials getCredentials(final AuthScope authscope) {
if (systemcreds == null) {
systemcreds = getSystemCreds(protocol, authscope, Authenticator.RequestorType.PROXY);
}
- if (systemcreds == null) {
- final String proxyHost = System.getProperty(protocol + ".proxyHost");
- if (proxyHost != null) {
- final String proxyPort = System.getProperty(protocol + ".proxyPort");
- if (proxyPort != null) {
- try {
- final AuthScope systemScope = new AuthScope(proxyHost, Integer.parseInt(proxyPort));
- if (authscope.match(systemScope) >= 0) {
- final String proxyUser = System.getProperty(protocol + ".proxyUser");
- if (proxyUser != null) {
- final String proxyPassword = System.getProperty(protocol + ".proxyPassword");
- systemcreds = new PasswordAuthentication(proxyUser, proxyPassword != null ? proxyPassword.toCharArray() : new char[] {});
- }
- }
- } catch (final NumberFormatException ex) {
- }
- }
- }
- }
+ if (systemcreds == null) {
+ // Look for values given using http.proxyUser/http.proxyPassword or
+ // https.proxyUser/https.proxyPassword. We cannot simply use the protocol from
+ // the origin since a proxy retrieved from https.proxyHost/https.proxyPort will
+ // still use http as protocol
+ systemcreds = getProxyCredentials("http", authscope);
+ if (systemcreds == null) {
+ systemcreds = getProxyCredentials("https", authscope);
+ }
--- End diff --
@jborgland Feel free to propose API changes in master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #123: Better handling of http(s).proxyUse...
Posted by michael-o <gi...@git.apache.org>.
Github user michael-o commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/123#discussion_r241075113
--- Diff: httpclient/src/main/java/org/apache/http/impl/client/SystemDefaultCredentialsProvider.java ---
@@ -116,25 +116,16 @@ public Credentials getCredentials(final AuthScope authscope) {
if (systemcreds == null) {
systemcreds = getSystemCreds(protocol, authscope, Authenticator.RequestorType.PROXY);
}
- if (systemcreds == null) {
- final String proxyHost = System.getProperty(protocol + ".proxyHost");
- if (proxyHost != null) {
- final String proxyPort = System.getProperty(protocol + ".proxyPort");
- if (proxyPort != null) {
- try {
- final AuthScope systemScope = new AuthScope(proxyHost, Integer.parseInt(proxyPort));
- if (authscope.match(systemScope) >= 0) {
- final String proxyUser = System.getProperty(protocol + ".proxyUser");
- if (proxyUser != null) {
- final String proxyPassword = System.getProperty(protocol + ".proxyPassword");
- systemcreds = new PasswordAuthentication(proxyUser, proxyPassword != null ? proxyPassword.toCharArray() : new char[] {});
- }
- }
- } catch (final NumberFormatException ex) {
- }
- }
- }
- }
+ if (systemcreds == null) {
+ // Look for values given using http.proxyUser/http.proxyPassword or
+ // https.proxyUser/https.proxyPassword. We cannot simply use the protocol from
+ // the origin since a proxy retrieved from https.proxyHost/https.proxyPort will
+ // still use http as protocol
+ systemcreds = getProxyCredentials("http", authscope);
+ if (systemcreds == null) {
+ systemcreds = getProxyCredentials("https", authscope);
+ }
--- End diff --
I haven't found any better solution yet, but this looks really redudant in most cases. Shouldn't it fall back for `https` => `http` only?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client issue #123: Better handling of http(s).proxyUser and h...
Posted by ok2c <gi...@git.apache.org>.
Github user ok2c commented on the issue:
https://github.com/apache/httpcomponents-client/pull/123
@jborgland The proposed change-set loos reasonable to me. Please _do_ fix the style check violations and make sure the build is successful and all tests pass.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] httpcomponents-client pull request #123: Better handling of http(s).proxyUse...
Posted by jborgland <gi...@git.apache.org>.
Github user jborgland commented on a diff in the pull request:
https://github.com/apache/httpcomponents-client/pull/123#discussion_r241202086
--- Diff: httpclient/src/main/java/org/apache/http/impl/client/SystemDefaultCredentialsProvider.java ---
@@ -116,25 +116,16 @@ public Credentials getCredentials(final AuthScope authscope) {
if (systemcreds == null) {
systemcreds = getSystemCreds(protocol, authscope, Authenticator.RequestorType.PROXY);
}
- if (systemcreds == null) {
- final String proxyHost = System.getProperty(protocol + ".proxyHost");
- if (proxyHost != null) {
- final String proxyPort = System.getProperty(protocol + ".proxyPort");
- if (proxyPort != null) {
- try {
- final AuthScope systemScope = new AuthScope(proxyHost, Integer.parseInt(proxyPort));
- if (authscope.match(systemScope) >= 0) {
- final String proxyUser = System.getProperty(protocol + ".proxyUser");
- if (proxyUser != null) {
- final String proxyPassword = System.getProperty(protocol + ".proxyPassword");
- systemcreds = new PasswordAuthentication(proxyUser, proxyPassword != null ? proxyPassword.toCharArray() : new char[] {});
- }
- }
- } catch (final NumberFormatException ex) {
- }
- }
- }
- }
+ if (systemcreds == null) {
+ // Look for values given using http.proxyUser/http.proxyPassword or
+ // https.proxyUser/https.proxyPassword. We cannot simply use the protocol from
+ // the origin since a proxy retrieved from https.proxyHost/https.proxyPort will
+ // still use http as protocol
+ systemcreds = getProxyCredentials("http", authscope);
+ if (systemcreds == null) {
+ systemcreds = getProxyCredentials("https", authscope);
+ }
--- End diff --
You mean to only do it if `protocol` is `http`? Sure.
One could imagine other completely different types of fixes as well. I tried to make a fix that was completely local to the SystemDefaultCredentialsProvider but when it's called (from AuthenticationStrategyImpl) there's more information available - like the fact that it is in fact a proxy that we're connecting to, and the HTTP status code (one could for example decide to use these system properties as the primary source if the status code is 407).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org