Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Maven plugin 11.0.0 does not respect nonProxyHosts #7072

Closed
stklcode opened this issue Oct 22, 2024 · 10 comments
Closed

Maven plugin 11.0.0 does not respect nonProxyHosts #7072

stklcode opened this issue Oct 22, 2024 · 10 comments
Labels

Comments

@stklcode
Copy link
Contributor

stklcode commented Oct 22, 2024

Describe the bug
After updating the Maven Plugin from 10.0.4 to 11.0.0 the proxy logic seems to be broken.

Scenario:

  • internal cache for the NVD CVE datafeed at https://cache.example.com/nvdcve-{0}.json.gz
  • proxy for external connections that does not forward internal traffic configured in Maven settings.xml

So we have to rely on nonProxyHosts to properly route the connections.

With 11.0.0 we receive 503 responses from the proxy (the cache is online and available).
Disabling the global proxy makes these calls work again, but www.cisa.gov and jeremylong.github.io fail with connection errors (as expected without proxy)

With 10.0.4 it works as expected. I suspect some missing link from the HTTP client switch, but I didn't yet look deeper into it.

Version of dependency-check used
The problem occurs using version 11.0.0 of the maven plugin.

To Reproduce
Steps to reproduce the behavior:

  1. Clear the local database, if any
mvn dependency-check:purge
  1. Have a local cache, e.g.
 <nvdDatafeedUrl>https://cache.example.com/nvdcve-{0}.json.gz</nvdDatafeedUrl>
  1. Have a selective proxy configured in maven settings.xml
<proxies>
  <proxy>
    <id>example-proxy</id>
    <active>true</active>
    <protocol>http</protocol>
    <host>proxy.example.com</host>
    <port>8080</port>
    <nonProxyHosts>*.example.com</nonProxyHosts>
  </proxy>
</proxies>
  1. Run dependency check via Maven
mvn dependency-check:check

Same behavior without the Maven settings and explicit proxy properties

  1. Clear the local database, if any
  2. Have a local cache
  3. Run dependency check via Maven with proxy properties
mvn -Dhttp.proxyHost=proxy.example.com \
    -Dhttp.proxyPort=8080 \
    -Dhttp.nonProxyHosts="*.example.com" \
    dependency-check:check

Expected behavior
Local NVD CVE database should be updated from NVD cache, and external CISA resources and the checks run as usual.

Additional context
Tested with Java 17.0.12 and 21.0.4 and Apache Maven 3.9.9
No additional proxy-relatved environment variables or properties set.

@stklcode stklcode added the bug label Oct 22, 2024
@miniupnp
Copy link

I have the same problem...

@miniupnp
Copy link

I have the following properties set in the JDK net.properties :

  • http.nonProxyHosts
  • http.proxyHost
  • http.proxyPort
  • https.proxyHost
  • https.proxyPort

It looks like http[s].proxy[Host/Port] are enforced, but not http.nonProxyHosts

@aikebah
Copy link
Collaborator

aikebah commented Oct 22, 2024

http.nonProxyHosts was expected to be properly honored by the apache HTTPClient based downloader. I'll see whether I can reproduce the faulty behaviour to see where that is failing.

@aikebah
Copy link
Collaborator

aikebah commented Oct 22, 2024

@miniupnp as we use Apache HTTPClient I can imagine JDK net.properties not taking effect (those are meant to configure core JDK HTTP classes' default configuration)

@stklcode Can you check if on your side they also have the appropriate effect (mvn -D variant) when you change the properties to be the https.* variant? (see also http://jeremylong.github.io/DependencyCheck/data/proxy.html ). From my local experiment the Apache HTTP client is not taking the http.*into account for HTTPS endpoints (except for http.nonProxyHosts for which there is no https counterpart), not sure whether or not Oracle's core JDK classes would fall back to http variant of the variables, but Oracle documentation appears to suggest that they also require the https variant for the https connection handlers.

In my local testing I've used a squid docker container (see https://github.com/jeremylong/DependencyCheck/tree/main/src/test/manual-test-proxy-auth) and have seen both JAVA_TOOLS_OPTIONS and mvn -D with the https-properties taking the expected effects regarding http.nonProxyHosts for various resources (no nvd cache available locally to try that resource, but the various others I've seen to be honouring the http.nonProxyHosts using the likes of

bypassing proxy

JAVA_TOOL_OPTIONS="-Dhttp.nonProxyHosts='*.nvd.nist.gov' -Dhttps.proxyHost=localhost -Dhttps.proxyPort=53128 -Dhttps.proxyUser=proxy -Dhttps.proxyPassword=insecure" mvn verify -DnvdValidForHours=0

and

mvn -Dhttp.nonProxyHosts="*.nvd.nist.gov" -Dhttps.proxyHost=localhost -Dhttps.proxyPort=53128 -Dhttps.proxyUser=proxy -Dhttps.proxyPassword=insecure -DnvdValidForHours=0 verify

using proxy

JAVA_TOOL_OPTIONS="-Dhttp.nonProxyHosts='example.com' -Dhttps.proxyHost=localhost -Dhttps.proxyPort=53128 -Dhttps.proxyUser=proxy -Dhttps.proxyPassword=insecure" mvn verify -DnvdValidForHours=0

and

mvn -Dhttp.nonProxyHosts="example.com" -Dhttps.proxyHost=localhost -Dhttps.proxyPort=53128 -Dhttps.proxyUser=proxy -Dhttps.proxyPassword=insecure -DnvdValidForHours=0 verify

@stklcode
Copy link
Contributor Author

stklcode commented Oct 22, 2024

Tested all (reasonable) combinations of http.* and https.* properties (all target URLs are HTTPS)

proxy{Host,Port} nonProxyHosts proxy works nonProxy works
http http yes no
http https yes no
https http yes yes
https https yes no

... intersting 🤔

Results exactly the same for mvn -D... and JAVA_TOOL_OPTIONS="-D..."

Wondering why the property-based config works fine while BaseDependencyCheckMojo generates the same properties from Maven settings.xml

if (mavenProxy != null) {
final String existing = System.getProperty("https.proxyHost");
if (existing == null && mavenProxy.getHost() != null && !mavenProxy.getHost().isEmpty()) {
System.setProperty("https.proxyHost", mavenProxy.getHost());
if (mavenProxy.getPort() > 0) {
System.setProperty("https.proxyPort", String.valueOf(mavenProxy.getPort()));
}
if (mavenProxy.getUsername() != null && !mavenProxy.getUsername().isEmpty()) {
System.setProperty("https.proxyUser", mavenProxy.getUsername());
}
if (mavenProxy.getPassword() != null && !mavenProxy.getPassword().isEmpty()) {
System.setProperty("https.proxyPassword", mavenProxy.getPassword());
}
if (mavenProxy.getNonProxyHosts() != null && !mavenProxy.getNonProxyHosts().isEmpty()) {
System.setProperty("http.nonProxyHosts", mavenProxy.getNonProxyHosts());
}
}


Update:

Removing the settings.setString() calls makes the Maven MoJo work, too:

--- a/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java
+++ b/maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java
@@ -2219,24 +2219,6 @@ public abstract class BaseDependencyCheckMojo extends AbstractMojo implements Ma
                     System.setProperty("http.nonProxyHosts", mavenProxy.getNonProxyHosts());
                 }
             }
-
-            settings.setString(Settings.KEYS.PROXY_SERVER, mavenProxy.getHost());
-            settings.setString(Settings.KEYS.PROXY_PORT, Integer.toString(mavenProxy.getPort()));
-            final String userName = mavenProxy.getUsername();
-            String password = mavenProxy.getPassword();
-            if (password != null && !password.isEmpty()) {
-                if (settings.getBoolean(Settings.KEYS.PROXY_DISABLE_SCHEMAS, true)) {
-                    System.setProperty("jdk.http.auth.tunneling.disabledSchemes", "");
-                }
-                try {
-                    password = decryptPasswordFromSettings(password);
-                } catch (SecDispatcherException ex) {
-                    password = handleSecDispatcherException("proxy", mavenProxy.getId(), password, ex);
-                }
-            }
-            settings.setStringIfNotNull(Settings.KEYS.PROXY_USERNAME, userName);
-            settings.setStringIfNotNull(Settings.KEYS.PROXY_PASSWORD, password);
-            settings.setStringIfNotNull(Settings.KEYS.PROXY_NON_PROXY_HOSTS, mavenProxy.getNonProxyHosts());
         } else if (System.getProperty("http.proxyHost") != null) {
             //else use standard Java system properties
             settings.setString(Settings.KEYS.PROXY_SERVER, System.getProperty("http.proxyHost", ""));

Update 2:

The problem should be here:

// Legacy proxy configuration present
// So don't rely on the system properties for proxy; use the legacy settings configuration
final String proxyHost = settings.getString(Settings.KEYS.PROXY_SERVER);
final int proxyPort = settings.getInt(Settings.KEYS.PROXY_PORT, -1);
httpClientBuilder.setProxy(new HttpHost(proxyHost, proxyPort));

We explicitly override proxy server and credentials, but missing Settings.KEYS.PROXY_NON_PROXY_HOSTS.

Because of the override the system properties are ignored and the nonProxyHosts are missing.

So either we omit setting the "legacy" properties (see patch above) or we add a proper ProxySelector/RoutePlanner.

@aikebah
Copy link
Collaborator

aikebah commented Oct 22, 2024

@stklcode Right... that's anyhow a bug in the 'legacy compatibility mode' part of the Downloader. That should most certainly still honour the Settings.KEYS.PROXY_NON_PROXY_HOSTS

That it didn't work for all kinds of 'legacy' configurations went undetected for me as I have full access on my dev environment and during development only create a setup matching the documented modern proxy config with JAVA_TOOL_OPTIONS from the documentation.

@aikebah
Copy link
Collaborator

aikebah commented Oct 22, 2024

Thanks for digging a bit deeper, I hope to work on a fix tomorrow to ensure that the legacy compatibility code does take nonProxyHosts into account, not sure when Jeremy would have the time to cut a new release after that. Good to know that at least the -D option is also working for you in the documented proxy config.

stklcode added a commit to stklcode/DependencyCheck that referenced this issue Oct 22, 2024
The Apache HTTPClient based downloader supports http(s).proxy*
properties, so we do not need to use legacy logic. In legeacy mode
http.nonProxyHosts is not honored, so setting both leads to issues due
to missing proxy selectors.

Omit populating legacy properties resolves this issue. In addition we
have to move the password decryption from Maven settings up, so it
actually works.

Signed-off-by: Stefan Kalscheuer <stefan@stklcode.de>
@stklcode
Copy link
Contributor Author

stklcode commented Oct 22, 2024

Good to know that at least the -D option is also working for you in the documented proxy config.

Using http.proxyHost has the very same problem, because we also generate legacy settings in the second if-branch.
Removing both blocks makes http.proxyHost and https.proxyHost equally working - as expected.

You are right, the legacy stuff should work, too. But should we consider not populating both new and old configs?

Using Maven e.g. the password property is missing potential decryption when setting https.proxyPassword, so I guess it only works because this property is overwritten by legacy config anyway.

This commit does repair all valid scenarios from properties and Maven for me: stklcode@196b029 Can create a PR if desired.

stklcode added a commit to stklcode/DependencyCheck that referenced this issue Oct 22, 2024
The Apache HTTPClient based downloader supports http(s).proxy*
properties, so we do not need to use legacy logic. In legeacy mode
http.nonProxyHosts is not honored, so setting both leads to issues due
to missing proxy selectors.

Omit populating legacy properties resolves this issue. In addition we
have to move the password decryption from Maven settings up, so it
actually works.

Signed-off-by: Stefan Kalscheuer <stefan@stklcode.de>
@aikebah
Copy link
Collaborator

aikebah commented Oct 22, 2024

Sure PR would be welcomed, need a second look tomorrow with a fresh and energetic mind, but high-over review makes me suspect that on the second look I can accept your proposed change as the right patch for this issue.

stklcode added a commit to stklcode/DependencyCheck that referenced this issue Oct 23, 2024
…emylong#7074)

The Apache HTTPClient based downloader supports http(s).proxy*
properties, so we do not need to use legacy logic. In legacy mode
http.nonProxyHosts is not honored, so setting both leads to issues due
to missing proxy selectors.

Omit populating legacy properties resolves this issue. In addition, we
have to move the password decryption from Maven settings up, so it
actually works.

Signed-off-by: Stefan Kalscheuer <stefan@stklcode.de>
@stklcode
Copy link
Contributor Author

Fix confirmed, the initial scenario runs fine again with v11.1.0

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 4, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants