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
Add proxy authentication #1337
Add proxy authentication #1337
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
I signed the CLA |
I signed it!
|
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.
Thanks for putting up this! We have no real expertise in proxy, so we value your input. @GoogleContainerTools/java-tools
(So, not having expertise, if I am asking a question in my comments, it is not a rhetorical question.)
I'm not sure why the CLA bot cannot verify you signing CLA. Maybe try singing it again?
Lastly, please try to add some tests.
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProxyProvider.java
Outdated
Show resolved
Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProxyProvider.java
Outdated
Show resolved
Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProxyProvider.java
Outdated
Show resolved
Hide resolved
Integer.parseInt(System.getProperty("http.proxyPort"))), | ||
new UsernamePasswordCredentials( | ||
System.getProperty("http.proxyUser"), System.getProperty("http.proxyPassword"))); | ||
} else if (System.getProperty("https.proxyUser") != null) { |
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.
If both http.proxyUser
and https.proxyUser
are given, should we do this them all? That is, should we remove else
?
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.
Originally I thought to have separate sets of properties for http and https. But I just learned that Maven active proxy applies to both http and https, so we need only a single set of credential properties. I will change the code to only use http.proxyUser and http.proxyPassword, and apply them on AuthScope.ANY.
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.
Sorry if I'm not getting this correct:
What you suggested seems to make sense when they were given through the Maven settings file. What if both http.proxyUser
and https.proxyUser
are defined via command line system properties instead of the Maven settings file? (A Gradle project is an example, but it's also a possible scenario in Maven.)
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.
And isn't it that setting the scope with the given host is the correct way? I wonder if AuthScope.ANY
may interfere with Jib setting auth for container registries.
Related, what's the end result of calling .setCredentials()
? For example, does it end up with setting the Proxy-Authorization: Basic xxx
? (Or, the Authorization:
header?)
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.
Related, what's the end result of calling .setCredentials()? For example, does it end up with setting the Proxy-Authorization: Basic xxx? (Or, the Authorization: header?)
I found a way to test and check this locally: #1304 (comment)
So, to answer my question myself, it sets Proxy-Authorization
indeed in the proxy case. (Haven't verified myself, but it may depend on the response code and header being WWW-Authenticate
or Proxy-Authenticate
.) And because it's not the plain Authorization
and sent only to a proxy, I believe it won't interfere with target host authentication. We are good with .setCredentials()
here.
And from my testing, it is OK to call .setCredentials()
multiple times with different hosts (and their corresponding user name and password pairs). The Apache client maintains a map for multiple hosts. Therefore, I think the structure should be
// one for HTTP
if (System.getProperty("http.proxyUser") != null) {
}
// the other for HTTPS
if (System.getProperty("https.proxyUser") != null) {
}
instead of chaining the if
statements with else
.
In any case, the Javadoc of AuthScope.ANY
says
Default scope matching any host, port, realm and authentication scheme. In the future versions of HttpClient the use of this parameter will be discontinued.
so we should not use AuthScope.ANY
. Also, it doesn't make sense to send auth info to totally unrelated proxies for which no proxy settings are configured.
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.
Ok, indeed if organization has proxy server(s) with different credential pairs for accessing proxy over https, and accessing over http, then we would need to have 2 separate credential pairs. I guess in most cases - like my organization - it would be the same credential pair. So would be nice if you could also specify just a single credential pair indifferent of the protocol. I mean, by default use http.proxyUser for both protocols, unless https.proxyUser is explicitly specified. This would also align with sending just single credential pair (http.proxyUser) from active proxy in Maven settings.
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.
Suggestion:
private static void addProxyCredentials(HttpTransport transport) {
DefaultHttpClient httpClient =
(DefaultHttpClient) ((ApacheHttpTransport) transport).getHttpClient();
boolean httpProxy = System.getProperty("http.proxyHost") != null;
boolean httpCreds = System.getProperty("http.proxyUser") != null;
boolean httpsProxy = System.getProperty("https.proxyHost") != null;
boolean httpsCreds = System.getProperty("https.proxyUser") != null;
if (httpProxy && httpCreds) {
httpClient
.getCredentialsProvider()
.setCredentials(
new AuthScope(System.getProperty("http.proxyHost"), Integer.parseInt(System.getProperty("http.proxyPort"))),
new UsernamePasswordCredentials(
System.getProperty("http.proxyUser"), System.getProperty("http.proxyPassword")));
}
if (httpsProxy && (httpsCreds || httpCreds)) {
httpClient
.getCredentialsProvider()
.setCredentials(
new AuthScope(System.getProperty("https.proxyHost"), Integer.parseInt(System.getProperty("https.proxyPort"))),
new UsernamePasswordCredentials(
httpsCreds ? System.getProperty("https.proxyUser") : System.getProperty("http.proxyUser"),
httpsCreds ? System.getProperty("https.proxyPassword") : System.getProperty("http.proxyPassword")));
}
}
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.
Never mind my last remark on Maven active proxy. DefaultProxySelector currently relied on, selects proxy protocol properties matching request protocol. So indeed its better to take into account multiple active proxies for both http and https requests.
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.
Please have a look on my last commit. It takes into account multiple Maven active proxies and registers proxy credentials per protocol.
@mmmheeren I checked the CLA service and it does not appear to have you github username associated with any signing. Can you check your status here: https://cla.developers.google.com/clas And if you do not see yourself can you try signing it again: https://cla.developers.google.com/ |
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProxyProvider.java
Outdated
Show resolved
Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProxyProvider.java
Outdated
Show resolved
Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/BuildDockerMojo.java
Show resolved
Hide resolved
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProxyProvider.java
Outdated
Show resolved
Hide resolved
Obviously, I've been confused in this proxy business. I've been doing some research and got some knowledge. I am probably still wrong in many aspects, but I'd like to share some of things I think worth pointing out (hopefully they are right):
It doesn't seem possible to specify per-host proxies in Maven settings (or through |
changed my profile to hopefully fix the cla |
@mmmheeren our CLA system shows that you have signed the CLA. Try commenting "I signed it!" again to see if the CLA bot picks that up. |
The Maven settings description says:
But the Configuring a proxy doc does suggest that an expectation of a single proxy. |
I signed it! |
@mmmheeren looks like the CLA bot was correct that not everyone signed the CLA. In your initial commit (ec4c65e), I see that @heeremk also contributed the code. I guess you need to have @heeremk also sign the CLA and another comment "I signed it!" from them. |
@mmmheeren either than or you can rewrite/squash the commit history and force push a change to your fork? |
I confirm you can have multiple active proxies in Maven settings without problem. The way I use them here is to drive the DefaultProxySelector to select a proxy based on the target registry protocol . So you would need proxy settings like below to be able to access both http and https registries during a jib build.
Maven itself uses these settings to select a proxy for accessing maven repositories. Their proxy selector / matching algorithm is probably similar, i.e. matching by target url protocol (and not excluded by nonProxyHosts). In https://github.com/srcclrapache1/maven-aether/blob/master/aether-util/src/main/java/org/eclipse/aether/util/repository/DefaultProxySelector.java it seems they build a map with one proxy def per protocol. |
This made me really confused, so I did some digging. And I think the doc is not correct. For example, I had <proxies>
<proxy>
<id>myhttpsproxy</id>
<active>true</active>
<protocol>https</protocol>
<host>localhost</host>
<port>443</port>
<username>proxyuser</username>
<password>somepassword</password>
</proxy>
</proxies> where I deliberated specified the port as 443 (the default HTTPS port), which rules out the possibility that Maven may not talk to a proxy on HTTPS if it is not 443 or that Maven may auto-fall back to HTTP if an HTTPS connection attempt fails. Then, I listen on 443 with
And on the command line running
So, And actually, |
I also did some testing, and I think Maven uses only the first active proxy setting (per <proxies>
<proxy>
<id>myproxy1</id>
<active>true</active>
<protocol>https</protocol>
<host>localhost</host>
<port>8086</port>
</proxy>
<proxy>
<!-- Some people claimed it is important to use a different id. -->
<id>myproxy2</id>
<active>true</active>
<protocol>https</protocol>
<host>localhost</host>
<port>8087</port>
</proxy>
<proxy>
<id>myproxy3</id>
<active>true</active>
<protocol>http</protocol> <-- just trying "http" for possible back-up ->
<host>localhost</host>
<port>8088</port>
</proxy>
</proxies> And to see if Maven works in a smart way to try another proxy when the first one fails, I listened on port 8087 Then, if I have <proxies>
<proxy>
<id>myproxy1</id>
<active>true</active>
<protocol>http</protocol> <!-- note this is http -->
<host>localhost</host>
<port>8086</port>
</proxy>
<proxy>
<id>myproxy2</id>
<active>true</active>
<protocol>https</protocol>
<host>localhost</host>
<port>8087</port>
</proxy>
</proxies> then even if the |
@mmmheeren I think it makes sense when you define multiple active proxies with different protocols like in your current settings. However, from my testing #1337 (comment), having multiple active proxies with the same protocol doesn't seem to work. (The first proxy setting always wins.) Anyways, to summarize my conclusion:
1 & 2 are independent and they can happen simultaneously. |
jib-maven-plugin/src/main/java/com/google/cloud/tools/jib/maven/ProxyProvider.java
Outdated
Show resolved
Hide resolved
boolean httpCreds = System.getProperty("http.proxyUser") != null; | ||
boolean httpsProxy = System.getProperty("https.proxyHost") != null; | ||
boolean httpsCreds = System.getProperty("https.proxyUser") != null; | ||
if (httpProxy && httpCreds) { |
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.
Per #1337 (comment), I think the logic should be
if (httpProxy && httpCreds) {
httpClient.getCredentialsProvider().setCredentials(
new AuthScope(http.proxyHost, http.proxyPort),
new UsernamePasswordCredentials(http.proxyUser, http.proxyPassword));
}
if (httpsProxy && httpsCreds) {
httpClient.getCredentialsProvider().setCredentials(
new AuthScope(https.proxyHost, https.proxyPort),
new UsernamePasswordCredentials(https.proxyUser, https.proxyPassword));
}
http.proxyHost
and https.proxyHost
are independent, and I don't think it makes sense to set http.proxyUser
for https.proxyHost
. Agreed?
* @param name proxy system property name | ||
* @param value proxy system property value | ||
*/ | ||
private static void propagateProxyProperty(String name, String value) { |
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.
Use @Nullable
for value
: ...(String name, @Nullable String value) {
* @param value proxy system property value | ||
*/ | ||
private static void propagateProxyProperty(String name, String value) { | ||
if (value != null && System.getProperty(name) == null) { |
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 think the proxy updating should happen all or not. For example, someone can define https.proxyHost
from the command line but leave https.proxyPort
undefined, in which case the default port will be chosen. It is also possible to leave https.proxyUser
if the proxy doesn't require auth. So, I think the logic should basically be
if (only when none of http.x properties are defined) {
Proxy firstActiveHttpProxy = <first active HTTP-targeting proxy from settings.xml>;
System.setProperty(http.proxyHost, firstActiveHttpProxy.getHost());
System.setProperty(http.proxyPort, firstActiveHttpProxy.getPort());
...
}
if (only when none of https.x properties are defined) {
Proxy firstActiveHttpsProxy = <first active HTTPS-targeting proxy from settings.xml>;
System.setProperty(https.proxyHost, firstActiveHttpsProxy.getHost());
System.setProperty(https.proxyPort, firstActiveHttpsProxy.getPort());
...
}
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.
Ok, I will refactor with approach of first protocol maven proxy wins, unless one of protocol system properties were already set.
FYI, I am thinking of moving to custom proxy selector approach where eligible maven proxies are added to proxy map inside custom RegistryProxySelector. This RegistryProxySelector can then be used by Connection / HttpTransport, to select proxy matching registry url, and register proxy credentials on transport where needed.
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.
Ok, I will refactor with approach of first protocol maven proxy wins, unless one of protocol system properties were already set.
Sound good.
This RegistryProxySelector can then be used by Connection / HttpTransport, to select proxy matching registry url
I honestly think you don't have to do this. I don't see you can configure per-host (or per-container-registry) proxies through settings.xml
. You change the current a little bit, and I think we're good to go.
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.
No it would still be max 1 proxy per protocol. But I thought it could be a bit more elegant way to transfer proxy config from maven plugin into core. If you tell me not to bother I will only do the small refactor. In any case I probably will not be able to finish/test before Monday.
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.
Yeah, don't be bothered. In this case, I think we can keep things simple.
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.
Ok I will commit today but test finally on Monday
Configuration from either Maven settings or system properties
CLAs look good, thanks! |
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.
Overall, this is in the right direction. Thanks for fixing the issue!
@GoogleContainerTools/java-tools there are several things to fix, but I think I can take it over after merging this.
Configuration from either Maven settings or system properties
Issues: #525, #1304