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 4307bbeeb5e..3cd78c687a2 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 @@ -30,6 +30,8 @@ public interface SecurityStore { void check(SimpleString address, SimpleString queue, CheckType checkType, SecurityAuth session) throws Exception; + boolean hasPermission(SimpleString address, SimpleString queue, CheckType checkType, SecurityAuth session) throws Exception; + boolean isSecurityEnabled(); void setSecurityEnabled(boolean securityEnabled); 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 c07d7bafef7..0ba198aaa5d 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 @@ -297,22 +297,42 @@ public void check(final SimpleString address, } @Override - public void check(final SimpleString address, - final SimpleString queue, - final CheckType checkType, - final SecurityAuth session) throws Exception { - if (securityEnabled) { + public boolean hasPermission(final SimpleString address, + final SimpleString queue, + final CheckType checkType, + final SecurityAuth session) throws Exception { + if (!securityEnabled) { + return true; + } + + // bypass permission checks for management cluster user + String user = session.getUsername(); + if (managementClusterUser.equals(user) && session.getPassword().equals(managementClusterPassword)) { + return true; + } + + // Special case: detect authentication failure for ActiveMQSecurityManager5 + // This must throw an authentication exception, not an authorization exception + Subject subjectForManager5 = null; + if (securityManager instanceof ActiveMQSecurityManager5 manager5) { + subjectForManager5 = getSubjectForAuthorization(session, manager5); + /* + * A user may authenticate successfully at first, but then later when their Subject is evicted from the + * local cache re-authentication may fail. This could happen, for example, if the user was removed from LDAP + * or the user's token expired. + * + * If the subject is null then authorization will *always* fail. + */ + if (subjectForManager5 == null) { + authenticationFailed(session.getUsername(), session.getRemotingConnection()); + } + } + try { SimpleString bareAddress = CompositeAddress.extractAddressName(address); SimpleString bareQueue = CompositeAddress.extractQueueName(queue); logger.trace("checking access permissions to {}", bareAddress); - // bypass permission checks for management cluster user - String user = session.getUsername(); - if (managementClusterUser.equals(user) && session.getPassword().equals(managementClusterPassword)) { - AUTHORIZATION_SUCCESS_COUNT_UPDATER.incrementAndGet(this); - return; - } Set roles = securityRepository.getMatch(bareAddress.toString()); @@ -325,27 +345,14 @@ public void check(final SimpleString address, } } - if (checkAuthorizationCache(fqqn != null ? fqqn : bareAddress, user, checkType)) { - AUTHORIZATION_SUCCESS_COUNT_UPDATER.incrementAndGet(this); - return; + if (checkAuthorizationCache(fqqn != null ? fqqn : bareAddress, user, checkType)) { + return true; } final Boolean validated; if (securityManager instanceof ActiveMQSecurityManager5 manager5) { - Subject subject = getSubjectForAuthorization(session, manager5); - - /* - * A user may authenticate successfully at first, but then later when their Subject is evicted from the - * local cache re-authentication may fail. This could happen, for example, if the user was removed from LDAP - * or the user's token expired. - * - * If the subject is null then authorization will *always* fail. - */ - if (subject == null) { - authenticationFailed(user, session.getRemotingConnection()); - } - - validated = manager5.authorize(subject, roles, checkType, fqqn != null ? fqqn.toString() : bareAddress.toString()); + // Reuse subject from earlier authentication check + validated = manager5.authorize(subjectForManager5, roles, checkType, fqqn != null ? fqqn.toString() : bareAddress.toString()); } else if (securityManager instanceof ActiveMQSecurityManager4 manager4) { validated = manager4.validateUserAndRole(user, session.getPassword(), roles, checkType, bareAddress.toString(), session.getRemotingConnection(), session.getSecurityDomain()) != null; } else if (securityManager instanceof ActiveMQSecurityManager3 manager3) { @@ -356,52 +363,67 @@ public void check(final SimpleString address, validated = securityManager.validateUserAndRole(user, session.getPassword(), roles, checkType); } - if (!validated) { - if (notificationService != null) { - TypedProperties props = new TypedProperties(); - - props.putSimpleStringProperty(ManagementHelper.HDR_ADDRESS, bareAddress); - props.putSimpleStringProperty(ManagementHelper.HDR_CHECK_TYPE, SimpleString.of(checkType.toString())); - props.putSimpleStringProperty(ManagementHelper.HDR_USER, SimpleString.of(getCaller(user, session.getRemotingConnection().getSubject()))); - - Notification notification = new Notification(null, CoreNotificationType.SECURITY_PERMISSION_VIOLATION, props); - - notificationService.sendNotification(notification); - } - - Exception ex; - if (bareQueue == null) { - ex = ActiveMQMessageBundle.BUNDLE.userNoPermissions(getCaller(user, session.getRemotingConnection().getSubject()), checkType, bareAddress); + if (validated && user != null) { + // if we get here we're granted, add to the cache + ConcurrentHashSet set; + String key = createAuthorizationCacheKey(user, checkType); + ConcurrentHashSet act = getAuthorizationCacheEntry(key); + if (act != null) { + set = act; } else { - ex = ActiveMQMessageBundle.BUNDLE.userNoPermissionsQueue(getCaller(user, session.getRemotingConnection().getSubject()), checkType, bareQueue, bareAddress); + set = new ConcurrentHashSet<>(); + putAuthorizationCacheEntry(set, key); } - AuditLogger.securityFailure(session.getRemotingConnection().getSubject(), session.getRemotingConnection().getRemoteAddress(), ex.getMessage(), ex); - AUTHORIZATION_FAILURE_COUNT_UPDATER.incrementAndGet(this); - throw ex; + set.add(Objects.requireNonNullElse(fqqn, bareAddress)); } - // if we get here we're granted, add to the cache + return validated; + } catch (Exception e) { + logger.debug("Permission check failed", e); + return false; + } + } + + @Override + public void check(final SimpleString address, + final SimpleString queue, + final CheckType checkType, + final SecurityAuth session) throws Exception { + if (!securityEnabled) { + return; + } + if (hasPermission(address, queue, checkType, session)) { AUTHORIZATION_SUCCESS_COUNT_UPDATER.incrementAndGet(this); + return; + } - if (user == null) { - // should get all user/pass into a subject and only cache subjects - // till then when subject is in play, the user may be null and - // we cannot cache as we don't have a unique key - return; - } + // Permission denied - handle side effects + SimpleString bareAddress = CompositeAddress.extractAddressName(address); + SimpleString bareQueue = CompositeAddress.extractQueueName(queue); + String user = session.getUsername(); - ConcurrentHashSet set; - String key = createAuthorizationCacheKey(user, checkType); - ConcurrentHashSet act = getAuthorizationCacheEntry(key); - if (act != null) { - set = act; - } else { - set = new ConcurrentHashSet<>(); - putAuthorizationCacheEntry(set, key); - } - set.add(Objects.requireNonNullElse(fqqn, bareAddress)); + if (notificationService != null) { + TypedProperties props = new TypedProperties(); + + props.putSimpleStringProperty(ManagementHelper.HDR_ADDRESS, bareAddress); + props.putSimpleStringProperty(ManagementHelper.HDR_CHECK_TYPE, SimpleString.of(checkType.toString())); + props.putSimpleStringProperty(ManagementHelper.HDR_USER, SimpleString.of(getCaller(user, session.getRemotingConnection().getSubject()))); + + Notification notification = new Notification(null, CoreNotificationType.SECURITY_PERMISSION_VIOLATION, props); + + notificationService.sendNotification(notification); + } + + Exception ex; + if (bareQueue == null) { + ex = ActiveMQMessageBundle.BUNDLE.userNoPermissions(getCaller(user, session.getRemotingConnection().getSubject()), checkType, bareAddress); + } else { + ex = ActiveMQMessageBundle.BUNDLE.userNoPermissionsQueue(getCaller(user, session.getRemotingConnection().getSubject()), checkType, bareQueue, bareAddress); } + AuditLogger.securityFailure(session.getRemotingConnection().getSubject(), session.getRemotingConnection().getRemoteAddress(), ex.getMessage(), ex); + AUTHORIZATION_FAILURE_COUNT_UPDATER.incrementAndGet(this); + throw ex; } @Override diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/management/ArtemisRbacInvocationHandler.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/management/ArtemisRbacInvocationHandler.java index 9453d90e5bc..40b225ebd4f 100644 --- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/management/ArtemisRbacInvocationHandler.java +++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/server/management/ArtemisRbacInvocationHandler.java @@ -329,17 +329,12 @@ SimpleString addressFrom(ObjectName objectName, String methodName) { } private boolean viewPermissionCheckFails(Object candidate) { - boolean failed = false; ObjectName objectName = candidate instanceof ObjectInstance oi ? oi.getObjectName() : (ObjectName) candidate; - if (!isUncheckedDomain(objectName)) { - try { - final SimpleString rbacAddress = addressFrom(objectName); - securityStoreCheck(rbacAddress, CheckType.VIEW); - } catch (Exception checkFailed) { - failed = true; - } + if (isUncheckedDomain(objectName)) { + return false; } - return failed; + final SimpleString rbacAddress = addressFrom(objectName); + return !hasPermission(rbacAddress, CheckType.VIEW); } private void securityStoreCheck(SimpleString rbacAddress, CheckType checkType) throws Exception { @@ -347,6 +342,15 @@ private void securityStoreCheck(SimpleString rbacAddress, CheckType checkType) t activeMQServer.getSecurityStore().check(rbacAddress, checkType, delegateToAccessController); } + private boolean hasPermission(SimpleString rbacAddress, CheckType checkType) { + // use accessor as security store can be updated on config reload + try { + return activeMQServer.getSecurityStore().hasPermission(rbacAddress, null, checkType, delegateToAccessController); + } catch (Exception notAuthenticated) { + return false; + } + } + // sufficiently empty to delegate to use of AccessController // ideally AccessController should be the source of truth private final SecurityAuth delegateToAccessController = new SecurityAuth() {