From 3a5971ec81984a091123c21fd1b9ac6e777fc7cf Mon Sep 17 00:00:00 2001 From: Clebert Suconic Date: Wed, 23 May 2018 14:52:07 -0400 Subject: [PATCH] ARTEMIS-1872 Improving Security Checks on AMQP Protocol Also improving test coverage on SecureConfigurationTest This commit will fix JMSConnectionWithSecurityTest. --- .../amqp/broker/AMQPSessionCallback.java | 2 +- .../proton/ProtonServerReceiverContext.java | 3 + .../server/SecureConfigurationTest.java | 89 ++++++++++--------- .../src/test/resources/multicast_topic.xml | 12 +-- 4 files changed, 58 insertions(+), 48 deletions(-) diff --git a/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java b/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java index 1301f0bfb8..df9b61e31c 100644 --- a/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java +++ b/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java @@ -248,7 +248,7 @@ public class AMQPSessionCallback implements SessionCallback { try { serverSession.createQueue(address, queueName, routingType, filter, true, false); } catch (ActiveMQSecurityException se) { - throw ActiveMQAMQPProtocolMessageBundle.BUNDLE.securityErrorCreatingConsumer(se.getMessage()); + throw ActiveMQAMQPProtocolMessageBundle.BUNDLE.securityErrorCreatingTempDestination(se.getMessage()); } } diff --git a/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java b/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java index 0036004c4d..aad89a8d0c 100644 --- a/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java +++ b/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/proton/ProtonServerReceiverContext.java @@ -29,6 +29,7 @@ import org.apache.activemq.artemis.protocol.amqp.broker.AMQPSessionCallback; import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPException; import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPInternalErrorException; import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPNotFoundException; +import org.apache.activemq.artemis.protocol.amqp.exceptions.ActiveMQAMQPSecurityException; import org.apache.activemq.artemis.protocol.amqp.logger.ActiveMQAMQPProtocolMessageBundle; import org.apache.activemq.artemis.protocol.amqp.sasl.PlainSASLResult; import org.apache.activemq.artemis.protocol.amqp.sasl.SASLResult; @@ -108,6 +109,8 @@ public class ProtonServerReceiverContext extends ProtonInitializable implements try { sessionSPI.createTemporaryQueue(address, defRoutingType); + } catch (ActiveMQAMQPSecurityException e) { + throw e; } catch (ActiveMQSecurityException e) { throw ActiveMQAMQPProtocolMessageBundle.BUNDLE.securityErrorCreatingTempDestination(e.getMessage()); } catch (Exception e) { diff --git a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java index fd0a12efdb..615ffcc80c 100644 --- a/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java +++ b/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java @@ -25,10 +25,13 @@ import org.apache.activemq.artemis.jms.client.ActiveMQConnectionFactory; import org.apache.activemq.artemis.jms.server.config.impl.FileJMSConfiguration; import org.apache.activemq.artemis.spi.core.security.ActiveMQJAASSecurityManager; import org.apache.activemq.artemis.spi.core.security.jaas.InVMLoginModule; +import org.apache.activemq.artemis.tests.integration.IntegrationTestLogger; import org.apache.activemq.artemis.tests.util.ActiveMQTestBase; import org.apache.qpid.jms.JmsConnectionFactory; +import org.junit.After; import org.junit.Assert; import org.junit.Assume; +import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -58,24 +61,30 @@ public class SecureConfigurationTest extends ActiveMQTestBase { @Parameterized.Parameter(0) public String protocol; + ActiveMQServer server; + + @Before + public void startSever() throws Exception { + server = getActiveMQServer("multicast_topic.xml"); + server.start(); + } + + @After + public void stopServer() throws Exception { + try { + if (server != null) { + server.stop(); + } + } catch (Throwable e) { + e.printStackTrace(); + } + } + @Test public void testSecureSharedDurableSubscriber() throws Exception { //This is because OpenWire does not support JMS 2.0 Assume.assumeFalse(protocol.equals("OPENWIRE")); - - ActiveMQServer server = getActiveMQServer("multicast_topic.xml"); - try { - server.start(); - internal_testSecureSharedDurableSubscriber(getConnectionFactory("b", "b")); - } finally { - try { - server.stop(); - } catch (Exception e) { - } - } - } - - private void internal_testSecureSharedDurableSubscriber(ConnectionFactory connectionFactory) throws JMSException { + ConnectionFactory connectionFactory = getConnectionFactory("b", "b"); String message = "blah"; //Expect to be able to create subscriber on pre-defined/existing queue. @@ -101,20 +110,7 @@ public class SecureConfigurationTest extends ActiveMQTestBase { public void testSecureSharedSubscriber() throws Exception { //This is because OpenWire does not support JMS 2.0 Assume.assumeFalse(protocol.equals("OPENWIRE")); - - ActiveMQServer server = getActiveMQServer("multicast_topic.xml"); - try { - server.start(); - internal_testSecureSharedSubscriber(getConnectionFactory("b", "b")); - } finally { - try { - server.stop(); - } catch (Exception e) { - } - } - } - - private void internal_testSecureSharedSubscriber(ConnectionFactory connectionFactory) throws JMSException { + ConnectionFactory connectionFactory = getConnectionFactory("b", "b"); String message = "blah"; //Expect to be able to create subscriber on pre-defined/existing queue. @@ -138,19 +134,7 @@ public class SecureConfigurationTest extends ActiveMQTestBase { @Test public void testSecureDurableSubscriber() throws Exception { - ActiveMQServer server = getActiveMQServer("multicast_topic.xml"); - try { - server.start(); - internal_testSecureDurableSubscriber(getConnectionFactory("b", "b")); - } finally { - try { - server.stop(); - } catch (Exception e) { - } - } - } - - private void internal_testSecureDurableSubscriber(ConnectionFactory connectionFactory) throws JMSException { + ConnectionFactory connectionFactory = getConnectionFactory("b", "b"); String message = "blah"; //Expect to be able to create subscriber on pre-defined/existing queue. @@ -177,8 +161,31 @@ public class SecureConfigurationTest extends ActiveMQTestBase { } catch (JMSSecurityException j) { //Expected exception } + + Connection connection = null; + + try { + connection = connectionFactory.createConnection(); + Session session = connection.createSession(false, Session.AUTO_ACKNOWLEDGE); + + try { + session.createTemporaryQueue(); + Assert.fail("Security exception expected, but did not occur, excepetion expected as not permissioned to create a temporary queue"); + } catch (JMSSecurityException jmsse) { + IntegrationTestLogger.LOGGER.info("Client should have thrown a JMSSecurityException but only threw JMSException"); + } catch (JMSException e) { + e.printStackTrace(); + Assert.fail("thrown a JMSEXception instead of a JMSSEcurityException"); + } + + // Should not be fatal + assertNotNull(connection.createSession(false, Session.AUTO_ACKNOWLEDGE)); + } finally { + connection.close(); + } } + private ConnectionFactory getConnectionFactory(String user, String password) { switch (protocol) { case "CORE": return getActiveMQConnectionFactory(user, password); diff --git a/tests/integration-tests/src/test/resources/multicast_topic.xml b/tests/integration-tests/src/test/resources/multicast_topic.xml index cf5430e352..2535ad3684 100644 --- a/tests/integration-tests/src/test/resources/multicast_topic.xml +++ b/tests/integration-tests/src/test/resources/multicast_topic.xml @@ -85,13 +85,13 @@ under the License. - - - - + + + + - - + +