You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@activemq.apache.org by gtully <gi...@git.apache.org> on 2018/03/21 13:58:57 UTC
[GitHub] activemq-artemis pull request #1961: [ARTEMIS-1758] support SASL EXTERNAL wi...
GitHub user gtully opened a pull request:
https://github.com/apache/activemq-artemis/pull/1961
[ARTEMIS-1758] support SASL EXTERNAL with TextCertLoginModule
- rework proton handler to use saslListener
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gtully/activemq-artemis ARTEMIS-1758
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/activemq-artemis/pull/1961.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1961
----
commit a7d4d6e8a18e4ff5164bcee03d677342d5eeabb9
Author: gtully <ga...@...>
Date: 2018-03-13T17:21:06Z
[ARTEMIS-1758] support SASL EXTERNAL with TextCertLoginModule
- rework proton handler to use saslListener
----
---
[GitHub] activemq-artemis pull request #1961: [ARTEMIS-1758] support SASL EXTERNAL wi...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1961#discussion_r176719562
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallback.java ---
@@ -113,7 +116,20 @@ public ServerSASL getServerSASL(final String mechanism) {
result = gssapiServerSASL;
break;
+ case ExternalServerSASL.NAME:
+ // validate ssl cert present
+ Principal principal = CertificateUtil.getPeerPrincipalFromConnection(protonConnectionDelegate);
+ if (principal != null) {
+ ExternalServerSASL externalServerSASL = new ExternalServerSASL();
+ externalServerSASL.setPrincipal(principal);
+ result = externalServerSASL;
+ } else {
+ logger.debug("SASL EXTERNAL mechanism requires a TLS peer principal");
+ }
+ break;
+
default:
+ logger.debug("Mo matching mechanism found for: " + mechanism);
--- End diff --
Same again on exception vs log.
---
[GitHub] activemq-artemis issue #1961: [ARTEMIS-1758] support SASL EXTERNAL with Text...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on the issue:
https://github.com/apache/activemq-artemis/pull/1961
Thanks Tim, artemis will treat the key/trust-store string as a url first and then revert to file.
qpid-jms expects a file only. I found some certs that work among the existing resources in the integrations tests. Also added a sanity check of the outbound support.
also learned the need for -DskipIntegrationTests=false!
---
[GitHub] activemq-artemis pull request #1961: [ARTEMIS-1758] support SASL EXTERNAL wi...
Posted by gtully <gi...@git.apache.org>.
Github user gtully commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1961#discussion_r176762803
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallback.java ---
@@ -113,7 +116,20 @@ public ServerSASL getServerSASL(final String mechanism) {
result = gssapiServerSASL;
break;
+ case ExternalServerSASL.NAME:
+ // validate ssl cert present
+ Principal principal = CertificateUtil.getPeerPrincipalFromConnection(protonConnectionDelegate);
+ if (principal != null) {
+ ExternalServerSASL externalServerSASL = new ExternalServerSASL();
+ externalServerSASL.setPrincipal(principal);
+ result = externalServerSASL;
+ } else {
+ logger.debug("SASL EXTERNAL mechanism requires a TLS peer principal");
--- End diff --
that is fair. there is currently not much logic around what mechanisms are supported, if a list is configured it is returned to the client.
The result of returning null here fails at
https://github.com/apache/activemq-artemis/pull/1961/files/c2869ca6598b7d17a56aee451daebfb7cb01fa0b#diff-f80f5c57a928d9c39f6dd7f7ea8028dfR316
---
[GitHub] activemq-artemis pull request #1961: [ARTEMIS-1758] support SASL EXTERNAL wi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/activemq-artemis/pull/1961
---
[GitHub] activemq-artemis pull request #1961: [ARTEMIS-1758] support SASL EXTERNAL wi...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1961#discussion_r176719439
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallback.java ---
@@ -113,7 +116,20 @@ public ServerSASL getServerSASL(final String mechanism) {
result = gssapiServerSASL;
break;
+ case ExternalServerSASL.NAME:
+ // validate ssl cert present
+ Principal principal = CertificateUtil.getPeerPrincipalFromConnection(protonConnectionDelegate);
+ if (principal != null) {
+ ExternalServerSASL externalServerSASL = new ExternalServerSASL();
+ externalServerSASL.setPrincipal(principal);
+ result = externalServerSASL;
+ } else {
+ logger.debug("SASL EXTERNAL mechanism requires a TLS peer principal");
--- End diff --
This feels like it should be an exception rather than just a log message. Things shouldn't get here if the connection cant actually do EXTERNAL, since the server shouldn't offer it in that case (as it can tell before offering that it cant work), and should fail before here if the client selected it when it wasn't actually offered.
---
[GitHub] activemq-artemis issue #1961: [ARTEMIS-1758] support SASL EXTERNAL with Text...
Posted by tabish121 <gi...@git.apache.org>.
Github user tabish121 commented on the issue:
https://github.com/apache/activemq-artemis/pull/1961
When running in the IDE the new test works fine, however when I run the AMQP tests from the command line the new test produces an error as below which seems to indicate the test environment isn't getting configured correctly.
> Test
testConnection(org.apache.activemq.artemis.tests.integration.amqp.JMSSaslExternalTest) Time elapsed: 0.773 sec <<< ERROR!
javax.jms.JMSException: file:/home/timothy/.m2/repository/org/apache/activemq/tests/unit-tests/2.6.0-SNAPSHOT/unit-tests-2.6.0-SNAPSHOT-tests.jar!/client-side-keystore.jks (No such file or directory)
at org.apache.qpid.jms.exceptions.JmsExceptionSupport.create(JmsExceptionSupport.java:86)
at org.apache.qpid.jms.exceptions.JmsExceptionSupport.create(JmsExceptionSupport.java:108)
at org.apache.qpid.jms.JmsConnection.connect(JmsConnection.java:168)
at org.apache.qpid.jms.JmsConnectionFactory.createConnection(JmsConnectionFactory.java:203)
at org.apache.activemq.artemis.tests.integration.amqp.JMSSaslExternalTest.testConnection(JMSSaslExternalTest.java:116)
Caused by: java.io.FileNotFoundException: file:/home/timothy/.m2/repository/org/apache/activemq/tests/unit-tests/2.6.0-SNAPSHOT/unit-tests-2.6.0-SNAPSHOT-tests.jar!/client-side-keystore.jks (No such file or directory)
at java.io.FileInputStream.open0(Native Method)
at java.io.FileInputStream.open(FileInputStream.java:195)
at java.io.FileInputStream.<init>(FileInputStream.java:138)
at org.apache.qpid.jms.transports.TransportSupport.loadStore(TransportSupport.java:287)
at org.apache.qpid.jms.transports.TransportSupport.loadKeyManagers(TransportSupport.java:250)
at org.apache.qpid.jms.transports.TransportSupport.createSslContext(TransportSupport.java:99)
at org.apache.qpid.jms.transports.TransportSupport.createSslHandler(TransportSupport.java:74)
at org.apache.qpid.jms.transports.netty.NettyTcpTransport.connect(NettyTcpTransport.java:136)
at org.apache.qpid.jms.provider.amqp.AmqpProvider$1.run(AmqpProvider.java:203)
at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
at java.util.concurrent.FutureTask.run(FutureTask.java:266)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$201(ScheduledThreadPoolExecutor.java:180)
at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:293)
at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
at java.lang.Thread.run(Thread.java:748)
---
[GitHub] activemq-artemis pull request #1961: [ARTEMIS-1758] support SASL EXTERNAL wi...
Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/1961#discussion_r176769932
--- Diff: artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPConnectionCallback.java ---
@@ -113,7 +116,20 @@ public ServerSASL getServerSASL(final String mechanism) {
result = gssapiServerSASL;
break;
+ case ExternalServerSASL.NAME:
+ // validate ssl cert present
+ Principal principal = CertificateUtil.getPeerPrincipalFromConnection(protonConnectionDelegate);
+ if (principal != null) {
+ ExternalServerSASL externalServerSASL = new ExternalServerSASL();
+ externalServerSASL.setPrincipal(principal);
+ result = externalServerSASL;
+ } else {
+ logger.debug("SASL EXTERNAL mechanism requires a TLS peer principal");
--- End diff --
I noticed that it fails eventually when returning null, though without indication why unless the debug logging is on, but the main thing is it shouldn't get to this bit of code without being able to succeed, except through malicious intent on a clients part. EXTERNAL can be supported by the broker but still not be used/usable by all clients, so it really shouldn't be offered to those that cant actually do it.
---