HTTPCLIENT-2381: use system properties as defaults in builder classes

[ok2c]

This turned out to be a big change-set but mostly due to many test classes affected by the API changes.

@arturobernalg @rschmitt @garydgregory Please review. For context please see

https://issues.apache.org/jira/browse/HTTPCLIENT-2381
https://lists.apache.org/thread/n1ckyf4soz0djw0bfo2sy3bhsso30vmj

[arturobernalg]

@ok2c LGTM

[ok2c]

If I hear no loud objections I will merge the change-set in the coming days

[rschmitt]

I'm going to need some time to get caught up on this. I won't be able to do any non-trivial testing until I get back from vacation in about a week.

[rschmitt]

I think "defaults" are a red herring here. The Apache client defaults are never going to work for the original requester, because the client is obviously not going to use his particular corporate proxy as the default value for proxyHost and proxyPort. All he's asking for is a way to override the defaults, without changing code:

Should this project be changed so that by default without code you can use system properties to configure the proxy?

All we need to do to solve his problem is define a new system property for the Apache client that, when set, has the same effect as calling .useSystemProperties() everywhere. This fixes the actual problem (locus of control) in a fully backwards-compatible way and without all the API churn I'm seeing in this PR. We already seem to be in agreement that "the client never queries system properties by default" is a totally obsolete constraint (and the Java platform feature that motivated it is deprecated for removal), so we're well within our rights to add a new opt-in system property that meets this need. The API impact would be zero, and the code change would probably be small enough to backport to previous versions.

Regarding backwards compatibility: flipping the default behavior after twenty years is virtually guaranteed to cause problems. There may be applications with inert copy-pasted system properties that don't actually work. There may be applications that are using those system properties to configure other libraries, e.g. HttpsURLConnection, while supplying the equivalent configuration to Apache programmatically. There may be test suites with fixtures or parallelization configs that don't expect client configuration to be affected by system properties (global mutable state). There's just a huge list of things I don't have to worry about if we simply make this an opt-in behavior (which, as I pointed out, it already is -- it's not a "default" in the sense that a socket timeout value or a retry strategy is a default).

[ok2c]

Regarding backwards compatibility: flipping the default behavior after twenty years is virtually guaranteed to cause problems.

@rschmitt So, what are we going to have at the end of it? The old default behavior that we all agree is outdated and wrong will still be there but on top of it we are going to introduce some non-standard system property to flip the behavior. So, instead of fixing the problem we are going to make it even messier. Is this wise?

Every time we make a minor release there is always someone unhappy because it turns out they should have read the release notes and / or have moved off some deprecated APIs.

We should be fixing problems as we discover them even at the cost of upsetting a few folks. Fixing problems properly pays off in the long run.

[rschmitt]

@rschmitt So, what are we going to have at the end of it? The old default behavior that we all agree is outdated and wrong will still be there but on top of it we are going to introduce some non-standard system property to flip the behavior. So, instead of fixing the problem we are going to make it even messier. Is this wise?

Yes. It's wise, because it maintains backwards compatibility while following established conventions for Java library development. It's totally normal for Java libraries to define their own namespaces for system properties (e.g. io.netty for Netty, org.asynchttpclient for async-http-client). In fact, async-http-client did exactly what I'm proposing we do, in order to accommodate the same use case.

What I've conceded is both outdated and wrong is the insistence on not even querying system properties. Beyond that, it's not clear to me which system properties should be respected by default, even without taking backwards compatibility into account. The proxy-related properties seem reasonable; http.agent does not.

Every time we make a minor release there is always someone unhappy because it turns out they should have read the release notes and / or have moved off some deprecated APIs.

I don't like changes that presume upon our users' ability to "just" move off deprecated APIs. Not everyone controls their execution environment and their runtime classpath. The caller of some deprecated API may itself be a library, which doesn't get the final say in which versions of httpcore5 and httpclient5 are present at runtime. Or a user may be deploying code into a runtime image they don't control, which provides third-party dependencies like the Apache client (I've worked with EMR before, which is an example of this sort of thing). We shouldn't assume that users can upgrade their Apache client and modify their code to account for breaking changes in a single atomic update, and we shouldn't even assume that users can code against a single version of the Apache client as opposed to a whole range of versions.

We should be fixing problems as we discover them even at the cost of upsetting a few folks. Fixing problems properly pays off in the long run.

This brings me back to my original point: the actual problem we are trying to solve is one of locus of control, and this problem has an easy, 100% backwards-compatible fix. There's no need to get sidetracked arguing about what the defaults are, whether to change them, how many API symbols to add or deprecate, etc. My repeated experience dealing with software upgrades at scale is that most breaking changes are frivolous, in the sense that the goal they were aiming at could have been done nearly as easily in a backwards-compatible way. I'd rather save our breakage budget for the bugs that can't be fixed compatibly.

[ok2c]

@rschmitt I find "all cool kids do that" kind of argument completely unconvincing. Especially when async-http-client is used as an example. HttpClient should be using standard properties defined by the platform or use code configuration. I am strongly not in favor of defining project specific system properties. What we have now is outdated and wrong, but at least it is conceptually consistent. I rather keep what we have and drop this change-set and close the issue as WONTFIX.

[rschmitt]

HttpClient should be using standard properties defined by the platform or use code configuration.

My understanding is that the "standard properties" are part of the standard for the JDK itself. The documentation states that these properties "alter the mechanisms and behavior of the various classes of the java.net package." Third-party libraries are not called upon to do anything in particular with the java.net system properties.

I prefer configuration to generally be done through code (the use of system properties in java.net to control things like User-Agent and maxConnections is utterly archaic), but the unique advantage of system properties is that they allow the default behavior of the code to be overridden by someone who knows better config values than the author of the code. This is a really important special case for proxy configuration and TLS trust stores. I'm sympathetic to the idea that these system properties should be respected by default, but I think it's even more important that a Java API like ignoreSystemProperties() have a corresponding system property that allows end users to override that behavior. If everyone were to start using ignoreSystemProperties(), then we'd be right back where we started: end users can't get their applications working through their proxies without making code changes. That's the problem I want to solve.

I'd be willing to consider a default behavior change just for the proxy behavior. I'm concerned that categorically switching the default behavior for all system properties we handle is too broad a change; that would potentially affect a lot of unrelated things at three or four different OSI layers. But I'd also like you to consider that there are really good reasons why most libraries define their own system properties, and that "standardization" isn't really the right framework for making this decision. Imagine an HTTP client that only supported "standard" HTTP headers.

[garydgregory]

FWIW, I like the concept in general that system properties are the escape hatch of last resort.

I see the hierarchy in general as:

My Java code overrides the default behavior of objects constructed (using constructors, methods, and builders). My code can take system properties into account when calling a library's APIs. This let's my app decide whether or not to override what.

System properties provide default values for objects in the library. If I don't specify a value, it comes from a library's defaults which itself can take its value from a system property.

While defaults can be overriden from system properties, it should still be the case that my app can call the right APIs and hard-wire what I want.

My 2c.

[ok2c]

@garydgregory What you described above is exactly what this change-set intends to accomplish.

I'd be willing to consider a default behavior change just for the proxy behavior.

@rschmitt This cannot be just for the proxy behavior, because there is at the very least JSSE settings that are standard and are not archaic,

http.proxyHost
http.proxyPort
https.proxyHost
https.proxyPort
http.nonProxyHosts
https.proxyUser
http.proxyUser
https.proxyPassword
http.proxyPassword

http.keepAlive
http.agent
 
ssl.TrustManagerFactory.algorithm
javax.net.ssl.trustStoreType
javax.net.ssl.trustStore
javax.net.ssl.trustStoreProvider
javax.net.ssl.trustStorePassword
ssl.KeyManagerFactory.algorithm
javax.net.ssl.keyStoreType
javax.net.ssl.keyStore
javax.net.ssl.keyStoreProvider
javax.net.ssl.keyStorePassword
https.protocols
https.cipherSuites

I am fine with dropping 'http.keepAlive' and 'http.agent' entirely and making use of all other properties listed above by default without any API changes other than deprecation of useSystemProperties method. If that works for you, it works for me, though I have to say I find this approach potentially massively more disruptive than my current one.

[rschmitt]

Let me research this a bit more. I want to understand the potential impact of the TLS-related system properties. I can search around to see how widely used they are; I can also do some experiments to see if the change in behavior breaks people's tests.

I asked elsewhere about the java.net.useSystemProxies system property, which I believe is also respected by the default ProxySelector, right? This system property looks particularly useful, since it enables a bunch of non-trivial, platform-specific logic to integrate with system-wide proxy configuration. I don't think we need to change any code, we would just need to add it to the list of system properties we respect, in the interest of completeness.

[ok2c]

Let me research this a bit more. I want to understand the potential impact of the TLS-related system properties. I can search around to see how widely used they are; I can also do some experiments to see if the change in behavior breaks people's tests.

@rschmitt Fair enough

we would just need to add it to the list of system properties we respect, in the interest of completeness.

Will do so

[rschmitt]

I've been looking at this a bit today, comparing SSLContexts.createDefault() and SSLContexts.createSystemDefault(). It appears that the following system properties are always respected:

ssl.TrustManagerFactory.algorithm
javax.net.ssl.trustStoreType
javax.net.ssl.trustStore
javax.net.ssl.trustStoreProvider
javax.net.ssl.trustStorePassword

I tested this by setting up a test server with a self-signed cert, along with a corresponding trust store. I call the server using a modified ClientCustomSSL from the examples/ directory. What I found is that calls to the test endpoint succeed if and only if I pass -Djavax.net.ssl.trustStore=... and so forth; even SSLContexts.createDefault() respects the trust store system properties. Additionally, both of these methods respect respect system-wide trust store configuration: I'm able to hit my company's internal endpoints with either SSLContext, and in the debugger I see the same trust store array contents with about ~140 certs.

The practical difference between the two methods is in key store configuration, i.e. mutual TLS:

ssl.KeyManagerFactory.algorithm
javax.net.ssl.keyStoreType
javax.net.ssl.keyStore
javax.net.ssl.keyStoreProvider
javax.net.ssl.keyStorePassword

I used logging breakpoints to verify that the JDK only reads these properties if createSystemDefault() is called.

Finally, there are these oddball properties:

http.keepAlive
http.agent
https.protocols
https.cipherSuites

These are properties for the JDK's built-in HTTP client that we manually reimplement; SSLContext doesn't look at them.

This trust store behavior makes my other finding much less surprising: changing systemProperties to default to true doesn't seem to break any consumers. They all rebuild successfully. The change could still cause a runtime failure, or an integration test failure, but I still would have expected at least one unit test failure.

Based on the above, I'm much more comfortable with changing the default behavior, in principle. My current thinking is:

1. Use SystemDefaultRoutePlanner with the default ProxySelector by default. This brings proxy configuration behavior in line with trust store configuration behavior: use the JDK defaults (whether OS-wide or overridden by system property) unless programmatically configured not to do so (in this case, with a custom HttpRoutePlanner). The support for OS-specific, system-wide proxy configuration (gated behind -Djava.net.useSystemProxies=true) is worth noting because it's backed by a bunch of non-trivial native code (see for instance the Windows implementation).
2. Drop support for the https.protocols and https.cipherSuites system properties. We'll no longer check an arbitrary subset of the system properties respected by HttpsURLConnection.
3. Use SSLContexts.createSystemDefault() by default, or change SSLContexts.createDefault() to delegate to it. Anyone using mTLS is either already enabling systemProperties or is programmatically setting a custom SSLContext, and I don't think that adding key stores has any effect on non-mTLS handshakes.
4. Mark useSystemProperties() as @Deprecated and document that it is a no-op retained for compatibility.

The only loose end here is these proxy auth system properties that are checked by SystemDefaultCredentialsProvider. I'm not sure what's going on there. I can't find any references in documentation to the http.proxyUser and http.proxyPassword system properties; it looks like they've been dropped from the JDK entirely, and were previously an undocumented implementation detail of JAX-WS. They don't look like standard properties in any sense.

[jonenst]

Hi,
for http.proxyUser and http.proxyPassword (for http basic authentication), I have the same understanding as you (see https://github.com/openjdk/jdk/blob/7c4ed0b15baf45616db9ac302e945f223909b980/src/jdk.xml.bind/share/classes/com/sun/istack/internal/tools/DefaultAuthenticator.java#L71 ) (This was removed in jdk11 https://github.com/openjdk/jdk/commits/987c3a2d874fae6cfedf90d5bc920b66c275098a )

As far as I understand, for basic auth, it was always necessary to use some form of code like

Authenticator.setDefault(
  new Authenticator() {
    @Override
    public PasswordAuthentication getPasswordAuthentication() {
      return new PasswordAuthentication(authUser, authPassword.toCharArray());
    }
  }
);

although some users report that it's not the case (not sure how trustworthy this report is. maybe the user was unaware that something read those proxyUser/proxyPass system properties and did the right thing for them to register a default authenticator) : https://stackoverflow.com/questions/1626549/authenticated-http-proxy-with-java
(note that consequently, the basicauth proxy authentication already had the exact problem of locus of control, users could not run programs in environment where basicauth was mandatory when developpers didn't think of writing some code preventively)
(note2: Of course what you want to allow users to do without changing the code is totally arbitrary. For exampl, for ntlm proxyauthentication I often had to run my own cntlm proxy on localhost which would reexpose an authenticated proxy as unauthenticated; it would have been possible to do this for basicauth as well. I remember some quote I read somewhere "Users get angry when they can't run arbitrary javascript to chose the proxy automatically nowadays")

ps: https://github.com/openjdk/jdk/blob/d1c0417898c2dcbd6a2e6acc9c72b04f01a9880c/jdk/src/share/classes/com/sun/org/apache/xml/internal/security/utils/resolver/implementations/ResolverDirectHTTP.java#L71 removed in jdk9 also but I'm not sure if these were ever system properties or just plain programmatic configuration of the engine

Thanks for all this work !