Repository: activemq-artemis Updated Branches: refs/heads/master 0746ea8ac -> e6d260749
ARTEMIS-1872 Fixing address security checks Ensure CREATE_ADDRESS is honored and behavior is consistent across protocols. Project: http://git-wip-us.apache.org/repos/asf/activemq-artemis/repo Commit: http://git-wip-us.apache.org/repos/asf/activemq-artemis/commit/659d23cb Tree: http://git-wip-us.apache.org/repos/asf/activemq-artemis/tree/659d23cb Diff: http://git-wip-us.apache.org/repos/asf/activemq-artemis/diff/659d23cb Branch: refs/heads/master Commit: 659d23cb28ce4299be5826ed1b6113458c6d0a1a Parents: 0746ea8 Author: Michael André Pearce <michael.andre.pea...@me.com> Authored: Thu May 24 04:47:35 2018 +0100 Committer: Michael André Pearce <michael.andre.pea...@me.com> Committed: Thu May 24 05:23:41 2018 +0100 ---------------------------------------------------------------------- .../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(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/659d23cb/artemis-protocols/artemis-amqp-protocol/src/main/java/org/apache/activemq/artemis/protocol/amqp/broker/AMQPSessionCallback.java ---------------------------------------------------------------------- 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 df9b61e..aac3f2a 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); } http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/659d23cb/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ServerSessionImpl.java ---------------------------------------------------------------------- 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 0c6838d..3057041 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 http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/659d23cb/tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/server/SecureConfigurationTest.java ---------------------------------------------------------------------- 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 3a15ea9..16447e3 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<Object[]> 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); + } http://git-wip-us.apache.org/repos/asf/activemq-artemis/blob/659d23cb/tests/integration-tests/src/test/resources/multicast_topic.xml ---------------------------------------------------------------------- diff --git a/tests/integration-tests/src/test/resources/multicast_topic.xml b/tests/integration-tests/src/test/resources/multicast_topic.xml index 3f546e7..a4891d0 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. <security-settings> <security-setting match="#"> + <permission type="createAddress" roles="a" /> <permission type="createNonDurableQueue" roles="a,b"/> <permission type="deleteNonDurableQueue" roles="a,b"/> <permission type="createDurableQueue" roles="a,b"/> @@ -139,7 +140,6 @@ under the License. <address-settings> <!--default for catch all--> <address-setting match="#"> - <auto-create-queues>false</auto-create-queues> <dead-letter-address>DLQ</dead-letter-address> <expiry-address>ExpiryQueue</expiry-address> <redelivery-delay>0</redelivery-delay>
