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 14c393d032..35c4c87417 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 @@ -98,14 +98,22 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC this.managementClusterUser = managementClusterUser; this.managementClusterPassword = managementClusterPassword; this.notificationService = notificationService; - authenticationCache = CacheBuilder.newBuilder() - .maximumSize(authenticationCacheSize) - .expireAfterWrite(invalidationInterval, TimeUnit.MILLISECONDS) - .build(); - authorizationCache = CacheBuilder.newBuilder() - .maximumSize(authorizationCacheSize) - .expireAfterWrite(invalidationInterval, TimeUnit.MILLISECONDS) - .build(); + if (authenticationCacheSize == 0) { + authenticationCache = null; + } else { + authenticationCache = CacheBuilder.newBuilder() + .maximumSize(authenticationCacheSize) + .expireAfterWrite(invalidationInterval, TimeUnit.MILLISECONDS) + .build(); + } + if (authorizationCacheSize == 0) { + authorizationCache = null; + } else { + authorizationCache = CacheBuilder.newBuilder() + .maximumSize(authorizationCacheSize) + .expireAfterWrite(invalidationInterval, TimeUnit.MILLISECONDS) + .build(); + } this.securityRepository.registerListener(this); } @@ -159,7 +167,7 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC boolean check = true; Subject subject = null; - Pair cacheEntry = authenticationCache.getIfPresent(createAuthenticationCacheKey(user, password, connection)); + Pair cacheEntry = getAuthenticationCacheEntry(user, password, connection); if (cacheEntry != null) { if (!cacheEntry.getA()) { // cached authentication failed previously so don't check again @@ -186,7 +194,7 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC if (securityManager instanceof ActiveMQSecurityManager5) { try { subject = ((ActiveMQSecurityManager5) securityManager).authenticate(user, password, connection, securityDomain); - authenticationCache.put(createAuthenticationCacheKey(user, password, connection), new Pair<>(subject != null, subject)); + putAuthenticationCacheEntry(user, password, connection, subject); validatedUser = getUserFromSubject(subject); } catch (NoCacheLoginException e) { handleNoCacheLoginException(e); @@ -313,12 +321,12 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC // if we get here we're granted, add to the cache ConcurrentHashSet set; String key = createAuthorizationCacheKey(user, checkType); - ConcurrentHashSet act = authorizationCache.getIfPresent(key); + ConcurrentHashSet act = getAuthorizationCacheEntry(key); if (act != null) { set = act; } else { set = new ConcurrentHashSet<>(); - authorizationCache.put(key, set); + putAuthorizationCacheEntry(set, key); } set.add(fqqn != null ? fqqn : bareAddress); } @@ -394,7 +402,7 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC * @return the authenticated Subject with all associated role principals */ private Subject getSubjectForAuthorization(SecurityAuth auth, ActiveMQSecurityManager5 securityManager) { - Pair cached = authenticationCache.getIfPresent(createAuthenticationCacheKey(auth.getUsername(), auth.getPassword(), auth.getRemotingConnection())); + Pair cached = getAuthenticationCacheEntry(auth.getUsername(), auth.getPassword(), auth.getRemotingConnection()); if (cached == null && auth.getUsername() == null && auth.getPassword() == null && auth.getRemotingConnection() instanceof ManagementRemotingConnection) { AccessControlContext accessControlContext = AccessController.getContext(); @@ -410,7 +418,7 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC if (cached == null) { try { Subject subject = securityManager.authenticate(auth.getUsername(), auth.getPassword(), auth.getRemotingConnection(), auth.getSecurityDomain()); - authenticationCache.put(createAuthenticationCacheKey(auth.getUsername(), auth.getPassword(), auth.getRemotingConnection()), new Pair<>(subject != null, subject)); + putAuthenticationCacheEntry(auth.getUsername(), auth.getPassword(), auth.getRemotingConnection(), subject); return subject; } catch (NoCacheLoginException e) { handleNoCacheLoginException(e); @@ -424,26 +432,71 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC logger.debug("Skipping authentication cache due to exception: {}", e.getMessage()); } + private void putAuthenticationCacheEntry(String user, + String password, + RemotingConnection connection, + Subject subject) { + if (authenticationCache != null) { + authenticationCache.put(createAuthenticationCacheKey(user, password, connection), new Pair<>(subject != null, subject)); + } + } + + private Pair getAuthenticationCacheEntry(String user, + String password, + RemotingConnection connection) { + if (authenticationCache == null) { + return null; + } else { + return authenticationCache.getIfPresent(createAuthenticationCacheKey(user, password, connection)); + } + } + + private void putAuthorizationCacheEntry(ConcurrentHashSet set, String key) { + if (authorizationCache != null) { + authorizationCache.put(key, set); + } + } + + private ConcurrentHashSet getAuthorizationCacheEntry(String key) { + if (authorizationCache == null) { + return null; + } else { + return authorizationCache.getIfPresent(key); + } + } + public void invalidateAuthorizationCache() { - authorizationCache.invalidateAll(); + if (authorizationCache != null) { + authorizationCache.invalidateAll(); + } } public void invalidateAuthenticationCache() { - authenticationCache.invalidateAll(); + if (authenticationCache != null) { + authenticationCache.invalidateAll(); + } } public long getAuthenticationCacheSize() { - return authenticationCache.size(); + if (authenticationCache == null) { + return 0; + } else { + return authenticationCache.size(); + } } public long getAuthorizationCacheSize() { - return authorizationCache.size(); + if (authorizationCache == null) { + return 0; + } else { + return authorizationCache.size(); + } } private boolean checkAuthorizationCache(final SimpleString dest, final String user, final CheckType checkType) { boolean granted = false; - ConcurrentHashSet act = authorizationCache.getIfPresent(createAuthorizationCacheKey(user, checkType)); + ConcurrentHashSet act = getAuthorizationCacheEntry(createAuthorizationCacheKey(user, checkType)); if (act != null) { granted = act.contains(dest); } @@ -458,4 +511,14 @@ public class SecurityStoreImpl implements SecurityStore, HierarchicalRepositoryC private String createAuthorizationCacheKey(String user, CheckType checkType) { return user + "." + checkType.name(); } + + // for testing + protected Cache> getAuthenticationCache() { + return authenticationCache; + } + + // for testing + protected Cache> getAuthorizationCache() { + return authorizationCache; + } } diff --git a/artemis-server/src/test/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImplTest.java b/artemis-server/src/test/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImplTest.java new file mode 100644 index 0000000000..ae22d1b812 --- /dev/null +++ b/artemis-server/src/test/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImplTest.java @@ -0,0 +1,94 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.activemq.artemis.core.security.impl; + +import javax.security.auth.Subject; +import java.util.Set; + +import org.apache.activemq.artemis.core.security.CheckType; +import org.apache.activemq.artemis.core.security.Role; +import org.apache.activemq.artemis.core.security.SecurityAuth; +import org.apache.activemq.artemis.core.settings.impl.HierarchicalObjectRepository; +import org.apache.activemq.artemis.spi.core.protocol.RemotingConnection; +import org.apache.activemq.artemis.spi.core.security.ActiveMQSecurityManager5; +import org.apache.activemq.artemis.spi.core.security.jaas.NoCacheLoginException; +import org.apache.activemq.artemis.utils.RandomUtil; +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +public class SecurityStoreImplTest { + + @Test + public void zeroCacheSizeTest() throws Exception { + ActiveMQSecurityManager5 securityManager = new ActiveMQSecurityManager5() { + @Override + public Subject authenticate(String user, + String password, + RemotingConnection remotingConnection, + String securityDomain) throws NoCacheLoginException { + return new Subject(); + } + + @Override + public boolean authorize(Subject subject, Set roles, CheckType checkType, String address) { + return true; + } + + @Override + public boolean validateUser(String user, String password) { + return false; + } + + @Override + public boolean validateUserAndRole(String user, String password, Set roles, CheckType checkType) { + return false; + } + }; + SecurityStoreImpl securityStore = new SecurityStoreImpl(new HierarchicalObjectRepository<>(), securityManager, 999, true, "", null, null, 0, 0); + assertNull(securityStore.getAuthenticationCache()); + securityStore.authenticate(RandomUtil.randomString(), RandomUtil.randomString(), null); + assertEquals(0, securityStore.getAuthenticationCacheSize()); + securityStore.invalidateAuthenticationCache(); // ensure this doesn't throw an NPE + + assertNull(securityStore.getAuthorizationCache()); + securityStore.check(RandomUtil.randomSimpleString(), CheckType.SEND, new SecurityAuth() { + @Override + public String getUsername() { + return RandomUtil.randomString(); + } + + @Override + public String getPassword() { + return RandomUtil.randomString(); + } + + @Override + public RemotingConnection getRemotingConnection() { + return null; + } + + @Override + public String getSecurityDomain() { + return null; + } + }); + assertEquals(0, securityStore.getAuthorizationCacheSize()); + securityStore.invalidateAuthorizationCache(); // ensure this doesn't throw an NPE + } +} \ No newline at end of file