From 45ea110f6223bcc17418a234dc6526746261c6c2 Mon Sep 17 00:00:00 2001 From: Michael Andre Pearce Date: Wed, 23 Aug 2017 08:24:05 +0100 Subject: [PATCH] ARTEMIS 1369: Include queue name in security errors Add new error in message bundle to include queue update security check to support taking optional queue update code that is operating on queues to pass the queue name during check so queue name could be in the error log if security issue. --- .../management/impl/QueueControlImpl.java | 2 +- .../artemis/core/security/SecurityStore.java | 2 ++ .../core/security/impl/SecurityStoreImpl.java | 14 ++++++++++++- .../core/server/ActiveMQMessageBundle.java | 3 +++ .../core/server/impl/ActiveMQServerImpl.java | 4 ++-- .../core/server/impl/ServerSessionImpl.java | 20 ++++++++++++------- 6 files changed, 34 insertions(+), 11 deletions(-) diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java index 0297ba5f49..537ba8f5c8 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/management/impl/QueueControlImpl.java @@ -744,7 +744,7 @@ public class QueueControlImpl extends AbstractControl implements QueueControl { final String user, final String password) throws Exception { try { - securityStore.check(queue.getAddress(), CheckType.SEND, new SecurityAuth() { + securityStore.check(queue.getAddress(), queue.getName(), CheckType.SEND, new SecurityAuth() { @Override public String getUsername() { return user; diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/SecurityStore.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/SecurityStore.java index 987faad5a9..301356e1d7 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/SecurityStore.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/SecurityStore.java @@ -25,6 +25,8 @@ public interface SecurityStore { void check(SimpleString address, CheckType checkType, SecurityAuth session) throws Exception; + void check(SimpleString address, SimpleString queue, CheckType checkType, SecurityAuth session) throws Exception; + boolean isSecurityEnabled(); void stop(); diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java index f4a7013aca..4ef5d5f622 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java @@ -162,6 +162,14 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC public void check(final SimpleString address, final CheckType checkType, final SecurityAuth session) throws Exception { + check(address, null, checkType, session); + } + + @Override + public void check(final SimpleString address, + final SimpleString queue, + final CheckType checkType, + final SecurityAuth session) throws Exception { if (securityEnabled) { if (logger.isTraceEnabled()) { logger.trace("checking access permissions to " + address); @@ -206,7 +214,11 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC notificationService.sendNotification(notification); } - throw ActiveMQMessageBundle.BUNDLE.userNoPermissions(session.getUsername(), checkType, saddress); + if (queue == null) { + throw ActiveMQMessageBundle.BUNDLE.userNoPermissions(session.getUsername(), checkType, saddress); + } else { + throw ActiveMQMessageBundle.BUNDLE.userNoPermissionsQueue(session.getUsername(), checkType, queue.toString(), saddress); + } } // if we get here we're granted, add to the cache ConcurrentHashSet set = new ConcurrentHashSet<>(); diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java index bd0e8cfa1a..2bd5db3a36 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/ActiveMQMessageBundle.java @@ -433,4 +433,7 @@ public interface ActiveMQMessageBundle { @Message(id = 119212, value = "Invalid deletion policy type {0}", format = Message.Format.MESSAGE_FORMAT) IllegalArgumentException invalidDeletionPolicyType(String val); + @Message(id = 119213, value = "User: {0} does not have permission=''{1}'' for queue {2} on address {3}", format = Message.Format.MESSAGE_FORMAT) + ActiveMQSecurityException userNoPermissionsQueue(String username, CheckType checkType, String squeue, String saddress); + } diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java index 08066263c8..79c381f865 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/impl/ActiveMQServerImpl.java @@ -1861,9 +1861,9 @@ public class ActiveMQServerImpl implements ActiveMQServer { if (queue.isDurable()) { // make sure the user has privileges to delete this queue - securityStore.check(address, CheckType.DELETE_DURABLE_QUEUE, session); + securityStore.check(address, queueName, CheckType.DELETE_DURABLE_QUEUE, session); } else { - securityStore.check(address, CheckType.DELETE_NON_DURABLE_QUEUE, session); + securityStore.check(address, queueName, CheckType.DELETE_NON_DURABLE_QUEUE, session); } } 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 ed1a1710d2..7f0988782b 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 @@ -415,6 +415,12 @@ public class ServerSessionImpl implements ServerSession, FailureListener { } } + private void securityCheck(SimpleString address, SimpleString queue, CheckType checkType, SecurityAuth auth) throws Exception { + if (securityEnabled) { + securityStore.check(address, queue, checkType, auth); + } + } + @Override public ServerConsumer createConsumer(final long consumerID, final SimpleString queueName, @@ -441,15 +447,15 @@ public class ServerSessionImpl implements ServerSession, FailureListener { SimpleString address = removePrefix(binding.getAddress()); if (browseOnly) { try { - securityCheck(address, CheckType.BROWSE, this); + securityCheck(address, queueName, CheckType.BROWSE, this); } catch (Exception e) { - securityCheck(address.concat(".").concat(unPrefixedQueueName), CheckType.BROWSE, this); + securityCheck(address.concat(".").concat(unPrefixedQueueName), queueName, CheckType.BROWSE, this); } } else { try { - securityCheck(address, CheckType.CONSUME, this); + securityCheck(address, queueName, CheckType.CONSUME, this); } catch (Exception e) { - securityCheck(address.concat(".").concat(unPrefixedQueueName), CheckType.CONSUME, this); + securityCheck(address.concat(".").concat(unPrefixedQueueName), queueName, CheckType.CONSUME, this); } } @@ -553,9 +559,9 @@ public class ServerSessionImpl implements ServerSession, FailureListener { if (durable) { // make sure the user has privileges to create this queue - securityCheck(address, CheckType.CREATE_DURABLE_QUEUE, this); + securityCheck(address, name, CheckType.CREATE_DURABLE_QUEUE, this); } else { - securityCheck(address, CheckType.CREATE_NON_DURABLE_QUEUE, this); + securityCheck(address, name, CheckType.CREATE_NON_DURABLE_QUEUE, this); } server.checkQueueCreationLimit(getUsername()); @@ -631,7 +637,7 @@ public class ServerSessionImpl implements ServerSession, FailureListener { address = removePrefix(address); - securityCheck(address, CheckType.CREATE_NON_DURABLE_QUEUE, this); + securityCheck(address, name, CheckType.CREATE_NON_DURABLE_QUEUE, this); server.checkQueueCreationLimit(getUsername());