From a0ed6398c93c943fd34df7d43cfae3db9edfd47a 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 0297ba5f498..537ba8f5c83 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 String sendMessage(final Map headers, 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 987faad5a91..301356e1d78 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 f4a7013acaf..4ef5d5f6225 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 String authenticate(final String user, 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 void check(final SimpleString address, 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 bd0e8cfa1a9..2bd5db3a36c 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 @@ IllegalStateException invalidRoutingTypeUpdate(String queueName, @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 ce1f3547160..300ddefdf75 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 @@ -1823,9 +1823,9 @@ public void destroyQueue(final SimpleString queueName, 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 ed1a1710d21..7f0988782bf 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 @@ private void securityCheck(SimpleString address, CheckType checkType, SecurityAu } } + 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 ServerConsumer createConsumer(final long consumerID, 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 Queue createQueue(final SimpleString address, 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 void createSharedQueue(SimpleString address, address = removePrefix(address); - securityCheck(address, CheckType.CREATE_NON_DURABLE_QUEUE, this); + securityCheck(address, name, CheckType.CREATE_NON_DURABLE_QUEUE, this); server.checkQueueCreationLimit(getUsername());