From 659d23cb28ce4299be5826ed1b6113458c6d0a1a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Andr=C3=A9=20Pearce?= Date: Thu, 24 May 2018 04:47:35 +0100 Subject: [PATCH] ARTEMIS-1872 Fixing address security checks Ensure CREATE_ADDRESS is honored and behavior is consistent across protocols. --- .../amqp/broker/AMQPSessionCallback.java | 2 +- .../core/server/impl/ServerSessionImpl.java | 8 ++++- .../server/SecureConfigurationTest.java | 34 ++++++++++++++----- .../src/test/resources/multicast_topic.xml | 2 +- 4 files changed, 34 insertions(+), 12 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 df9b61e31c..aac3f2a26a 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 @@ -298,7 +298,7 @@ public class AMQPSessionCallback implements SessionCallback { } // if auto-create we will return whatever type was used before - if (!queueQueryResult.isAutoCreated() && queueQueryResult.getRoutingType() != routingType) { + if (queueQueryResult.isExists() && !queueQueryResult.isAutoCreated() && queueQueryResult.getRoutingType() != routingType) { throw new IllegalStateException("Incorrect Routing Type for queue, expecting: " + routingType); } diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java index 0c6838daa3..3057041827 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java @@ -583,9 +583,15 @@ public class ServerSessionImpl implements ServerSession, FailureListener { securityCheck(addressInfo.getName(), name, CheckType.CREATE_NON_DURABLE_QUEUE, this); } + AddressSettings as = server.getAddressSettingsRepository().getMatch(art.getName().toString()); + + if (as.isAutoCreateAddresses() && server.getAddressInfo(art.getName()) == null) { + securityCheck(addressInfo.getName(), name, CheckType.CREATE_ADDRESS, this); + } + server.checkQueueCreationLimit(getUsername()); - Queue queue = server.createQueue(art, unPrefixedName, filterString, SimpleString.toSimpleString(getUsername()), durable, temporary, autoCreated, maxConsumers, purgeOnNoConsumers, exclusive, lastValue, server.getAddressSettingsRepository().getMatch(art.getName().toString()).isAutoCreateAddresses()); + Queue queue = server.createQueue(art, unPrefixedName, filterString, SimpleString.toSimpleString(getUsername()), durable, temporary, autoCreated, maxConsumers, purgeOnNoConsumers, exclusive, lastValue, as.isAutoCreateAddresses()); if (temporary) { // Temporary queue in core simply means the queue will be deleted if 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 3a15ea9a5b..16447e331b 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 @@ -16,7 +16,6 @@ */ package org.apache.activemq.artemis.tests.integration.server; -import java.lang.IllegalStateException; import java.util.Arrays; import java.util.Collection; import javax.jms.Connection; @@ -45,7 +44,6 @@ import org.junit.After; import org.junit.Assert; import org.junit.Assume; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -56,7 +54,7 @@ public class SecureConfigurationTest extends ActiveMQTestBase { @Parameterized.Parameters(name = "{index}: protocol={0}") public static Collection parameters() { return Arrays.asList(new Object[][] { - {"CORE"}, {"AMQP"}, {"OPENWIRE"} + {"CORE"}, {"AMQP"}, {"OPENWIRE"} }); } @@ -168,9 +166,14 @@ public class SecureConfigurationTest extends ActiveMQTestBase { @Test public void testTemporaryQueue() throws Exception { - ConnectionFactory connectionFactory = getConnectionFactory("c", "c"); + ConnectionFactory connectionFactory = getConnectionFactory("a", "a"); String message = "blah"; + //Expect to be able to create subscriber on pre-defined/existing queue. + String messageRecieved = sendAndReceiveText(connectionFactory, "clientId", message, s -> s.createTemporaryQueue(), (d, s) -> s.createConsumer(d)); + Assert.assertEquals(message, messageRecieved); + + connectionFactory = getConnectionFactory("c", "c"); try { sendAndReceiveText(connectionFactory, "clientId", message, s -> s.createTemporaryQueue(), (d, s) -> s.createConsumer(d)); Assert.fail("Security exception expected, but did not occur, excepetion expected as not permissioned to create a temporary queue"); @@ -183,9 +186,14 @@ public class SecureConfigurationTest extends ActiveMQTestBase { @Test public void testTemporaryTopic() throws Exception { - ConnectionFactory connectionFactory = getConnectionFactory("c", "c"); + ConnectionFactory connectionFactory = getConnectionFactory("a", "a"); String message = "blah"; + //Expect to be able to create subscriber on pre-defined/existing queue. + String messageRecieved = sendAndReceiveText(connectionFactory, "clientId", message, s -> s.createTemporaryTopic(), (d, s) -> s.createConsumer(d)); + Assert.assertEquals(message, messageRecieved); + + connectionFactory = getConnectionFactory("c", "c"); try { sendAndReceiveText(connectionFactory, "clientId", message, s -> s.createTemporaryTopic(), (d, s) -> s.createConsumer(d)); Assert.fail("Security exception expected, but did not occur, excepetion expected as not permissioned to create a temporary queue"); @@ -198,8 +206,6 @@ public class SecureConfigurationTest extends ActiveMQTestBase { @Test public void testSecureQueue() throws Exception { - // Core & OpenWire are not creating the queue as the test expects.. just querying - Assume.assumeTrue(protocol.equals("AMQP")); ConnectionFactory connectionFactory = getConnectionFactory("b", "b"); String message = "blah"; @@ -207,12 +213,22 @@ public class SecureConfigurationTest extends ActiveMQTestBase { String messageRecieved = sendAndReceiveTextUsingQueue(connectionFactory, "clientId", message, "secured_queue", (q, s) -> s.createConsumer(q)); Assert.assertEquals(message, messageRecieved); + connectionFactory = getConnectionFactory("a", "a"); + messageRecieved = sendAndReceiveTextUsingQueue(connectionFactory, "clientId", message, "new-queue-1", (q, s) -> s.createConsumer(q)); + Assert.assertEquals(message, messageRecieved); + + connectionFactory = getConnectionFactory("b", "b"); try { - sendAndReceiveTextUsingQueue(connectionFactory, "clientId", message, "non-existent-queue", (q, s) -> s.createConsumer(q)); - Assert.fail("Security exception expected, but did not occur, excepetion expected as not permissioned to dynamically create queue"); + sendAndReceiveTextUsingQueue(connectionFactory, "clientId", message, "new-queue-2", (q, s) -> s.createConsumer(q)); + Assert.fail("Security exception expected, but did not occur, excepetion expected as not permissioned to dynamically create address, or queue"); } catch (JMSSecurityException j) { //Expected exception } + + connectionFactory = getConnectionFactory("a", "a"); + messageRecieved = sendAndReceiveTextUsingQueue(connectionFactory, "clientId", message, "new-queue-2", (q, s) -> s.createConsumer(q)); + Assert.assertEquals(message, messageRecieved); + } diff --git a/tests/integration-tests/src/test/resources/multicast_topic.xml b/tests/integration-tests/src/test/resources/multicast_topic.xml index 3f546e7634..a4891d0790 100644 --- a/tests/integration-tests/src/test/resources/multicast_topic.xml +++ b/tests/integration-tests/src/test/resources/multicast_topic.xml @@ -91,6 +91,7 @@ under the License. + @@ -139,7 +140,6 @@ under the License. - false DLQ ExpiryQueue 0