ARTEMIS-3081 Swap precedence of key/tr.store props

[inkarkat]

If an application wants to use a special key/truststore for Artemis but
have the remainder of the application use the default Java store, the
org.apache.activemq.ssl.keyStore needs to take precedence over Java's
javax.net.ssl.keyStore. However, the current implementation takes the
first non-null value from
System.getProperty(JAVAX_KEYSTORE_PATH_PROP_NAME),
System.getProperty(ACTIVEMQ_KEYSTORE_PATH_PROP_NAME),
keyStorePath

So if the default Java property is set, no override is possible. Swap
the order of the JAVAX_... and ACTIVEMQ_... property names so that the
ActiveMQ ones come first (as a component-specific overrides), the
standard Java ones comes second, and finally a local attribute value
(through Stream.of(...).firstFirst()).

(In our case the application uses the default Java truststore location
at $JAVA_HOME/lib/security/jssecacerts, and only supplies its password
in javax.net.ssl.trustStorePassword, and then uses a dedicated
truststore for Artemis. Defining both org.apache.activemq.ssl.trustStore
and org.apache.activemq.ssl.trustStorePassword now makes Artemis use the
dedicated truststore (javax.net.ssl.trustStore is not set as we use the
default location, so the second choice
org.apache.activemq.ssl.trustStore applies), but with the Java default
truststore password (first choice javax.net.ssl.trustStorePassword
applies instead of the second choice because it is set for the default
truststore). Obviously, this does not work unless both passwords are
identical!)

[brusdev]

The original NettyConnector behavior was to give the ActiveMQ-specific system properties precedence on javax.net.ssl properties. This precedence order was changed by ARTEMIS-1649 so LGTM revert it but the NettyConnectorTest.testSystemPropertyOverridesActiveMQ test should be fixed.

[inkarkat]

Thanks for the heads-up; I only built the affected Maven project itself, and didn't notice any test failures there; I'll adapt the test!

[jbertram]

This has come up before in https://issues.apache.org/jira/browse/ARTEMIS-2932 and https://issues.apache.org/jira/browse/ARTEMIS-1888. You can review those to see why the current behavior is the way it is. You should be able to get the behavior your want by using the forceSSLParameters URL parameter along with the relevant keystore & truststore parameters.

[gemmellr]

I think this proposal is actually somewhat different than the previous ones and warrants further consideration.

Previous proposals were primarily aiming for the more traditional behaviour of URI settings overriding System Properties, whereas this one does not and is just specific to behaviour of using the latter. In particular around the fact that if you set both the javax.ssl.* and org.apache.activemq.ssl.* system properties currently, the latter will surprisingly be ignored and not do anything. That makes those properties useless a lot of the time since folks using system properties for TLS config also seem likely be using javax.ssl.* as well for other components. The main reason to want to ever set both is to use different values, which would clearly suggest the Artemis ones should have higher precedence or else you cant do that and neednt bother ever setting the Artemis one.

As @brusdev noted, the order handling of these system properties was originally that the Artemis ones took precedence. That only looks to have changed in Artemis 2.5.0 during a config handling rewrite while adding OpenSSL support in https://issues.apache.org/jira/browse/ARTEMIS-1649. Its not clear to me that was a deliberate change, it was seemingly not mentioned or documented at the time, and per above makes little sense since there is a good chance it removes the ability to even use the system property. I think the change may have been an error in the port to using Stream handling for the config values.

The test that needs changed here only looks to be present because Chris added tests of the by-then-current wider behaviour of the TLS config handling whilst adding the override flag in Artemis 2.7.0 to allow the desired URI option precedence.

I think it makes sense to restore the more useful and original Artemis 1.0.0 - 2.4.0 behaviour for these system properties, rather than leaving the somewhat useless 2.5.0+ behaviour.

[inkarkat]

[...] if you set both the javax.ssl.* and org.apache.activemq.ssl.* system properties currently, the latter will surprisingly be ignored and not do anything. That makes those properties useless a lot of the time [...]

Exactly. In virtually every configuration mechanism I've encountered so far, the application-specific one would override the more general one, not the other way around. It is really unexpected that the precedence is reversed in Artemis.

Apart from that, I'd like to mention that using the suggested forceSSLParameters is less convenient in our case, as the actual instantiation of the NettyConnector is not directly done in our own code, but by JBoss. Now, there's likely a way to pass additional configuration in there (e.g. through configuration file or subclassing), but it's not as straightforward as setting the Java property (as part of the JBoss bootstrapping, which is what we're doing right now).

[clebertsuconic]

thanks a lot

[jbertram]

My apologies. I misunderstood the intention of this PR despite your thorough explanation 😄. Thanks for the contribution, @inkarkat.