You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/01/22 08:45:15 UTC
[GitHub] [activemq-artemis] inkarkat opened a new pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
inkarkat opened a new pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416
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!)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram closed pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
jbertram closed pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-767002542
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] inkarkat commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
inkarkat commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-765629263
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!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] brusdev commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
brusdev commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-765301146
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-772676832
My apologies. I misunderstood the intention of this PR despite your thorough explanation :smile:. Thanks for the contribution, @inkarkat.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-767002542
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-772667020
thanks a lot
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] inkarkat commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
inkarkat commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-769377822
> [...] 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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] asfgit closed pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram closed pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
jbertram closed pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] gemmellr commented on pull request #3416: ARTEMIS-3081 Swap precedence of key/tr.store props
Posted by GitBox <gi...@apache.org>.
gemmellr commented on pull request #3416:
URL: https://github.com/apache/activemq-artemis/pull/3416#issuecomment-769155367
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org