diff --git a/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerFactory.java b/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerFactory.java index 9dfb85952e3..3d635edf9af 100644 --- a/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerFactory.java +++ b/activemq-broker/src/main/java/org/apache/activemq/broker/BrokerFactory.java @@ -31,14 +31,16 @@ */ public final class BrokerFactory { - private static final FactoryFinder BROKER_FACTORY_HANDLER_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/broker/"); + private static final FactoryFinder BROKER_FACTORY_HANDLER_FINDER + = new FactoryFinder<>("META-INF/services/org/apache/activemq/broker/", BrokerFactoryHandler.class, + null); private BrokerFactory() { } public static BrokerFactoryHandler createBrokerFactoryHandler(String type) throws IOException { try { - return (BrokerFactoryHandler)BROKER_FACTORY_HANDLER_FINDER.newInstance(type); + return BROKER_FACTORY_HANDLER_FINDER.newInstance(type); } catch (Throwable e) { throw IOExceptionSupport.create("Could not load " + type + " factory:" + e, e); } diff --git a/activemq-broker/src/main/java/org/apache/activemq/broker/region/group/GroupFactoryFinder.java b/activemq-broker/src/main/java/org/apache/activemq/broker/region/group/GroupFactoryFinder.java index 718b37d7082..78092eb3163 100644 --- a/activemq-broker/src/main/java/org/apache/activemq/broker/region/group/GroupFactoryFinder.java +++ b/activemq-broker/src/main/java/org/apache/activemq/broker/region/group/GroupFactoryFinder.java @@ -25,7 +25,9 @@ import org.apache.activemq.util.URISupport; public class GroupFactoryFinder { - private static final FactoryFinder GROUP_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/groups/"); + private static final FactoryFinder GROUP_FACTORY_FINDER = + new FactoryFinder<>("META-INF/services/org/apache/activemq/groups/", MessageGroupMapFactory.class, + null); private GroupFactoryFinder() { } @@ -40,7 +42,7 @@ public static MessageGroupMapFactory createMessageGroupMapFactory(String type) t factoryType = factoryType.substring(0,p); properties = URISupport.parseQuery(propertiesString); } - MessageGroupMapFactory result = (MessageGroupMapFactory)GROUP_FACTORY_FINDER.newInstance(factoryType); + MessageGroupMapFactory result = GROUP_FACTORY_FINDER.newInstance(factoryType); if (properties != null && result != null){ IntrospectionSupport.setProperties(result,properties); } diff --git a/activemq-broker/src/main/java/org/apache/activemq/transport/auto/AutoTcpTransportServer.java b/activemq-broker/src/main/java/org/apache/activemq/transport/auto/AutoTcpTransportServer.java index bcd73653d0a..b2ea7c06c16 100644 --- a/activemq-broker/src/main/java/org/apache/activemq/transport/auto/AutoTcpTransportServer.java +++ b/activemq-broker/src/main/java/org/apache/activemq/transport/auto/AutoTcpTransportServer.java @@ -80,15 +80,19 @@ public class AutoTcpTransportServer extends TcpTransportServer { protected int maxConnectionThreadPoolSize = Integer.MAX_VALUE; protected int protocolDetectionTimeOut = 30000; - private static final FactoryFinder TRANSPORT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/"); + private static final FactoryFinder TRANSPORT_FACTORY_FINDER = + new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/", TransportFactory.class, + null); private final ConcurrentMap transportFactories = new ConcurrentHashMap(); - private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/wireformat/"); + private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = + new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class, + null); public WireFormatFactory findWireFormatFactory(String scheme, Map> options) throws IOException { WireFormatFactory wff = null; try { - wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(scheme); + wff = WIREFORMAT_FACTORY_FINDER.newInstance(scheme); if (options != null) { final Map wfOptions = new HashMap<>(); if (options.get(AutoTransportUtils.ALL) != null) { @@ -117,7 +121,7 @@ public TransportFactory findTransportFactory(String scheme, Map optio if (tf == null) { // Try to load if from a META-INF property. try { - tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme); + tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme); if (options != null) { IntrospectionSupport.setProperties(tf, options); } diff --git a/activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java b/activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java index 00ca2154b36..ddc2306c9bc 100644 --- a/activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java +++ b/activemq-broker/src/main/java/org/apache/activemq/util/osgi/Activator.java @@ -172,7 +172,15 @@ protected void unregister(long bundleId) { // ================================================================ @Override - public Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException { + public Object create(String path) + throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException { + throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls"); + } + + @SuppressWarnings("unchecked") + @Override + public T create(String path, Class requiredType, Set> allowedImpls) + throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException { Class clazz = serviceCache.get(path); if (clazz == null) { StringBuilder warnings = new StringBuilder(); @@ -199,6 +207,10 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx continue; } + // no reason to cache if invalid so validate before caching + // the class inside of serviceCache + FactoryFinder.validateClass(clazz, requiredType, allowedImpls); + // Yay.. the class was found. Now cache it. serviceCache.put(path, clazz); wrapper.cachedServices.add(path); @@ -214,10 +226,17 @@ public Object create(String path) throws IllegalAccessException, InstantiationEx } throw new IOException(msg); } + } else { + // Validate again (even for previously cached classes) in case + // a path is re-used with a different requiredType. + // This object factory is shared by all factory finder instances, so it would be + // possible (although probably a mistake) to use the same + // path again with a different requiredType in a different FactoryFinder + FactoryFinder.validateClass(clazz, requiredType, allowedImpls); } try { - return clazz.getConstructor().newInstance(); + return (T) clazz.getConstructor().newInstance(); } catch (InvocationTargetException | NoSuchMethodException e) { throw new InstantiationException(e.getMessage()); } diff --git a/activemq-client/src/main/java/org/apache/activemq/transport/TransportFactory.java b/activemq-client/src/main/java/org/apache/activemq/transport/TransportFactory.java index acfc1b0389a..184a5d63911 100644 --- a/activemq-client/src/main/java/org/apache/activemq/transport/TransportFactory.java +++ b/activemq-client/src/main/java/org/apache/activemq/transport/TransportFactory.java @@ -36,8 +36,12 @@ public abstract class TransportFactory { - private static final FactoryFinder TRANSPORT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/"); - private static final FactoryFinder WIREFORMAT_FACTORY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/wireformat/"); + private static final FactoryFinder TRANSPORT_FACTORY_FINDER + = new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/", TransportFactory.class, + null); + private static final FactoryFinder WIREFORMAT_FACTORY_FINDER + = new FactoryFinder<>("META-INF/services/org/apache/activemq/wireformat/", WireFormatFactory.class, + null); private static final ConcurrentMap TRANSPORT_FACTORYS = new ConcurrentHashMap(); private static final String WRITE_TIMEOUT_FILTER = "soWriteTimeout"; @@ -179,7 +183,7 @@ public static TransportFactory findTransportFactory(URI location) throws IOExcep if (tf == null) { // Try to load if from a META-INF property. try { - tf = (TransportFactory)TRANSPORT_FACTORY_FINDER.newInstance(scheme); + tf = TRANSPORT_FACTORY_FINDER.newInstance(scheme); TRANSPORT_FACTORYS.put(scheme, tf); } catch (Throwable e) { throw IOExceptionSupport.create("Transport scheme NOT recognized: [" + scheme + "]", e); @@ -201,7 +205,7 @@ protected WireFormatFactory createWireFormatFactory(Map options) } try { - WireFormatFactory wff = (WireFormatFactory)WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat); + WireFormatFactory wff = WIREFORMAT_FACTORY_FINDER.newInstance(wireFormat); IntrospectionSupport.setProperties(wff, options, "wireFormat."); return wff; } catch (Throwable e) { diff --git a/activemq-client/src/main/java/org/apache/activemq/transport/discovery/DiscoveryAgentFactory.java b/activemq-client/src/main/java/org/apache/activemq/transport/discovery/DiscoveryAgentFactory.java index 65f3ee6933c..46894d47291 100644 --- a/activemq-client/src/main/java/org/apache/activemq/transport/discovery/DiscoveryAgentFactory.java +++ b/activemq-client/src/main/java/org/apache/activemq/transport/discovery/DiscoveryAgentFactory.java @@ -26,7 +26,9 @@ public abstract class DiscoveryAgentFactory { - private static final FactoryFinder DISCOVERY_AGENT_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/discoveryagent/"); + private static final FactoryFinder DISCOVERY_AGENT_FINDER = + new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/discoveryagent/", + DiscoveryAgentFactory.class, null); private static final ConcurrentMap DISCOVERY_AGENT_FACTORYS = new ConcurrentHashMap(); /** @@ -43,7 +45,7 @@ private static DiscoveryAgentFactory findDiscoveryAgentFactory(URI uri) throws I if (daf == null) { // Try to load if from a META-INF property. try { - daf = (DiscoveryAgentFactory)DISCOVERY_AGENT_FINDER.newInstance(scheme); + daf = DISCOVERY_AGENT_FINDER.newInstance(scheme); DISCOVERY_AGENT_FACTORYS.put(scheme, daf); } catch (Throwable e) { throw IOExceptionSupport.create("DiscoveryAgent scheme NOT recognized: [" + scheme + "]", e); diff --git a/activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java b/activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java index 596d15ea00f..594af7e2578 100644 --- a/activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java +++ b/activemq-client/src/main/java/org/apache/activemq/util/FactoryFinder.java @@ -20,14 +20,22 @@ import java.io.IOException; import java.io.InputStream; import java.lang.reflect.InvocationTargetException; +import java.net.UnknownServiceException; +import java.nio.file.Path; +import java.security.Security; +import java.util.Arrays; +import java.util.Objects; +import java.util.Optional; import java.util.Properties; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.stream.Collectors; /** * */ -public class FactoryFinder { +public class FactoryFinder { /** * The strategy that the FactoryFinder uses to find load and instantiate Objects @@ -44,51 +52,66 @@ public interface ObjectFactory { * @param path the full service path * @return */ - public Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException; + Object create(String path) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException; + /** + * This method loads objects by searching for classes in the given path. + * A requiredType and Set of allowed implementations are provided for + * implementations to use for validation. Note is up to the actual implementations + * that implement {@link ObjectFactory} to decide how to use both parameters. + * By default, the method just delegates to {@link #create(String)} + * + * @param path the full service path + * @param requiredType the requiredType any objects must implement + * @param allowedImpls The set of allowed impls + * @return + */ + @SuppressWarnings("unchecked") + default T create(String path, Class requiredType, Set> allowedImpls) + throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException { + return (T) create(path); + } } /** * The default implementation of Object factory which works well in standalone applications. */ protected static class StandaloneObjectFactory implements ObjectFactory { - final ConcurrentMap classMap = new ConcurrentHashMap(); + final ConcurrentMap> classMap = new ConcurrentHashMap<>(); + + @Override + public Object create(final String path) + throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException { + throw new UnsupportedOperationException("Create is not supported without requiredType and allowed impls"); + } + @SuppressWarnings("unchecked") @Override - public Object create(final String path) throws InstantiationException, IllegalAccessException, ClassNotFoundException, IOException { - Class clazz = classMap.get(path); + public T create(String path, Class requiredType, Set> allowedImpls) throws InstantiationException, IllegalAccessException, ClassNotFoundException, IOException { + Class clazz = classMap.get(path); if (clazz == null) { clazz = loadClass(loadProperties(path)); + // no reason to cache if invalid so validate before caching + validateClass(clazz, requiredType, allowedImpls); classMap.put(path, clazz); + } else { + // Validate again (even for previously cached classes) in case + // a path is re-used with a different requiredType. + // This object factory is static and shared by all factory finder instances by default, + // so it would be possible (although probably a mistake) to use the same + // path again with a different requiredType in a different FactoryFinder + validateClass(clazz, requiredType, allowedImpls); } - + try { - return clazz.getConstructor().newInstance(); + return (T) clazz.getConstructor().newInstance(); } catch (NoSuchMethodException | InvocationTargetException e) { throw new InstantiationException(e.getMessage()); } } - static public Class loadClass(Properties properties) throws ClassNotFoundException, IOException { - - String className = properties.getProperty("class"); - if (className == null) { - throw new IOException("Expected property is missing: class"); - } - Class clazz = null; - ClassLoader loader = Thread.currentThread().getContextClassLoader(); - if (loader != null) { - try { - clazz = loader.loadClass(className); - } catch (ClassNotFoundException e) { - // ignore - } - } - if (clazz == null) { - clazz = FactoryFinder.class.getClassLoader().loadClass(className); - } - - return clazz; + static Class loadClass(Properties properties) throws ClassNotFoundException, IOException { + return FactoryFinder.loadClass(properties.getProperty("class")); } static public Properties loadProperties(String uri) throws IOException { @@ -138,11 +161,45 @@ public static void setObjectFactory(ObjectFactory objectFactory) { // Instance methods and properties // ================================================================ private final String path; + private final Class requiredType; + private final Set> allowedImpls; - public FactoryFinder(String path) { - this.path = path; + /** + * + * @param path The path to search for impls + * @param requiredType Required interface type that any impl must implement + * @param allowedImpls The list of allowed implementations. If null or asterisk + * then all impls of the requiredType are allowed. + */ + public FactoryFinder(String path, Class requiredType, String allowedImpls) { + this.path = Objects.requireNonNull(path); + this.requiredType = Objects.requireNonNull(requiredType); + this.allowedImpls = loadAllowedImpls(requiredType, allowedImpls); } + @SuppressWarnings("unchecked") + private static Set> loadAllowedImpls(Class requiredType, String allowedImpls) { + // If allowedImpls is either null or an asterisk (allow all wild card) then set to null so we don't filter + // If allowedImpls is only an empty string we return an empty set meaning allow none + // Otherwise split/trim all values + return allowedImpls != null && !allowedImpls.equals("*") ? + Arrays.stream(allowedImpls.split("\\s*,\\s*")) + .filter(s -> !s.isEmpty()) + .map(s -> { + try { + final Class clazz = FactoryFinder.loadClass(s); + if (!requiredType.isAssignableFrom(clazz)) { + throw new IllegalArgumentException( + "Class " + clazz + " is not assignable to " + requiredType); + } + return (Class)clazz; + } catch (ClassNotFoundException | IOException e) { + throw new IllegalArgumentException(e); + } + }).collect(Collectors.toUnmodifiableSet()) : null; + } + + /** * Creates a new instance of the given key * @@ -150,9 +207,71 @@ public FactoryFinder(String path) { * the factory name * @return a newly created instance */ - public Object newInstance(String key) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException { - return objectFactory.create(path+key); + public T newInstance(String key) throws IllegalAccessException, InstantiationException, IOException, ClassNotFoundException { + return objectFactory.create(resolvePath(key), requiredType, allowedImpls); } + Set> getAllowedImpls() { + return allowedImpls; + } + + Class getRequiredType() { + return requiredType; + } + + private String resolvePath(final String key) throws InstantiationException { + // Normalize the base path with the given key. This + // will resolve/remove any relative ".." sections of the path. + // Example: "/dir1/dir2/dir3/../file" becomes "/dir1/dir2/file" + final Path resolvedPath = Path.of(path).resolve(key).normalize(); + + // Validate the resolved path is still within the original defined + // root path and throw an error of it is not. + if (!resolvedPath.startsWith(path)) { + throw new InstantiationException("Provided key escapes the FactoryFinder configured directory"); + } + + return resolvedPath.toString(); + } + + public static String buildAllowedImpls(Class...classes) { + return Arrays.stream(Objects.requireNonNull(classes, "List of allowed classes may not be null")) + .map(Class::getName).collect(Collectors.joining(",")); + } + + public static void validateClass(Class clazz, Class requiredType, + Set> allowedImpls) throws InstantiationException { + // Validate the loaded class is an allowed impl + if (allowedImpls != null && !allowedImpls.contains(clazz)) { + throw new InstantiationException("Class " + clazz + " is not an allowed implementation " + + "of type " + requiredType); + } + // Validate the loaded class is a subclass of the right type + // The allowedImpls may not be used so also check requiredType. Even if set + // generics can be erased and this is an extra safety check + if (!requiredType.isAssignableFrom(clazz)) { + throw new InstantiationException("Class " + clazz + " is not assignable to " + requiredType); + } + } + + static Class loadClass(String className) throws ClassNotFoundException, IOException { + if (className == null) { + throw new IOException("Expected property is missing: class"); + } + Class clazz = null; + ClassLoader loader = Thread.currentThread().getContextClassLoader(); + if (loader != null) { + try { + clazz = loader.loadClass(className); + } catch (ClassNotFoundException e) { + // ignore + } + } + if (clazz == null) { + clazz = FactoryFinder.class.getClassLoader().loadClass(className); + } + + return clazz; + } } diff --git a/activemq-client/src/test/java/org/apache/activemq/util/FactoryFinderTest.java b/activemq-client/src/test/java/org/apache/activemq/util/FactoryFinderTest.java new file mode 100644 index 00000000000..3e775b21616 --- /dev/null +++ b/activemq-client/src/test/java/org/apache/activemq/util/FactoryFinderTest.java @@ -0,0 +1,202 @@ +/* + * 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.util; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import java.io.IOException; +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; +import org.apache.activemq.transport.TransportFactory; +import org.apache.activemq.transport.nio.NIOTransportFactory; +import org.apache.activemq.transport.tcp.SslTransport; +import org.apache.activemq.transport.tcp.SslTransportFactory; +import org.apache.activemq.transport.tcp.TcpTransportFactory; +import org.apache.activemq.util.FactoryFinder.ObjectFactory; +import org.apache.activemq.wireformat.WireFormatFactory; +import org.junit.Test; + +public class FactoryFinderTest { + + private static final String TRANSPORT_FACTORY_PATH = "META-INF/services/org/apache/activemq/transport/"; + private static final String WIREFORMAT_FACTORY_PATH = "META-INF/services/org/apache/activemq/wireformat/"; + + // Test path traversal attempts will throw an error + @Test + public void testPathTraversal() throws Exception { + FactoryFinder finder + = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, TransportFactory.class, null); + assertNull(finder.getAllowedImpls()); + + try { + finder.newInstance("../../tcp"); + fail("should have failed instantiation"); + } catch (InstantiationException e) { + assertEquals("Provided key escapes the FactoryFinder configured directory", + e.getMessage()); + } + } + + // WireFormatFactory is not assignable to TransportFactory + // So the constructor should throw an IllegalArgumentException + @Test(expected = IllegalArgumentException.class) + public void testAllowedImplsMatchInterface() { + new FactoryFinder<>(TRANSPORT_FACTORY_PATH, TransportFactory.class, + FactoryFinder.buildAllowedImpls(WireFormatFactory.class)); + } + + @Test(expected = IllegalArgumentException.class) + public void testAllowedImplsNotFound() { + new FactoryFinder<>(TRANSPORT_FACTORY_PATH, TransportFactory.class, "some.invalid.ClassName"); + } + + @Test(expected = IOException.class) + public void testLoadClassNull() throws IOException, ClassNotFoundException { + FactoryFinder.loadClass(null); + } + + @Test(expected = ClassNotFoundException.class) + public void testLoadClassNotFound() throws IOException, ClassNotFoundException { + FactoryFinder.loadClass("some.invalid.ClassName"); + } + + // The default factory should throw UnsupportedOperationException + // if using legacy create() method + @Test(expected = UnsupportedOperationException.class) + public void testDefaultObjectFinder() throws Exception { + ObjectFactory factory = FactoryFinder.getObjectFactory(); + factory.create("path"); + } + + @Test + public void testObjectFinderDefaultMethod() throws Exception { + AtomicBoolean called = new AtomicBoolean(); + ObjectFactory factory = path -> { + called.set(true); + return null; + }; + // test that new method defaults to legacy method if not implemented + factory.create(null, null, null); + assertTrue(called.get()); + } + + // Verify interface check works if not using an allowed list to at least verify that + // any classes loaded will implement/extend the required type + @Test + public void testInterfaceMismatch() throws Exception { + // use wrong interface type, WIREFORMAT_FACTORY_PATH should be of type WireFormatFactory + // and not TransportFactory + FactoryFinder factory = new FactoryFinder<>(WIREFORMAT_FACTORY_PATH, TransportFactory.class, + null); + // This is a valid impl in the wireformat directory, but it isn't a TransportFactory type + try { + factory.newInstance("default"); + } catch (InstantiationException e) { + assertTrue(e.getMessage().contains("OpenWireFormatFactory is not assignable to class " + + TransportFactory.class.getName())); + } + } + + // Test filtering by allowed implementations of the given interface + @Test + public void testAllowedImplsFilter() throws Exception { + Set> allowedImpls = Set.of(TcpTransportFactory.class, NIOTransportFactory.class); + FactoryFinder finder = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, TransportFactory.class, + FactoryFinder.buildAllowedImpls(allowedImpls.toArray(new Class[0]))); + assertEquals(TransportFactory.class, finder.getRequiredType()); + assertEquals(allowedImpls, finder.getAllowedImpls()); + // tcp and nio are the only class in the allow list + assertNotNull(finder.newInstance("tcp")); + + try { + // ssl transport factory was not in the allow list + finder.newInstance("ssl"); + fail("should have failed instantiation"); + } catch (InstantiationException e) { + assertTrue(e.getMessage().contains("is not an allowed implementation of type")); + } + } + + // empty array is used so we should deny everything + @Test + public void testAllowedImplsFilterDenyAll() throws Exception { + FactoryFinder finder = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, + TransportFactory.class, ""); + assertEquals(TransportFactory.class, finder.getRequiredType()); + assertEquals(Set.of(), finder.getAllowedImpls()); + + try { + // nothing allowed, tcp exists but should be blocked + finder.newInstance("tcp"); + fail("should have failed instantiation"); + } catch (InstantiationException e) { + assertTrue(e.getMessage().contains("is not an allowed implementation of type")); + } + } + + // Test that impls are not filtered if the list is null + @Test + public void testAllowedImplsFilterAllowAll() throws Exception { + // check with constructor that should default to null + FactoryFinder finder = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, + TransportFactory.class, null); + assertNull(finder.getAllowedImpls()); + assertNotNull(finder.newInstance("tcp")); + assertNotNull(finder.newInstance("ssl")); + + // check again with explicit null + finder = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, + TransportFactory.class, null); + assertNull(finder.getAllowedImpls()); + assertNotNull(finder.newInstance("tcp")); + assertNotNull(finder.newInstance("ssl")); + + try { + // abc is allowed because we are not filtering by allowed impls but + // we should not be able to find it + finder.newInstance("abc"); + fail("should have failed instantiation"); + } catch (IOException e) { + assertTrue(e.getMessage().contains("Could not find factory class for resource")); + } + } + + @Test + public void testAllowedImplsTrimWhiteSpace() throws Exception { + // Build CSV with white space between all classes, including two white space + // only elements + String implsWithWhiteSpace = String.join(", ", + TcpTransportFactory.class.getName(), SslTransportFactory.class.getName(), + " ", NIOTransportFactory.class.getName(), ""); + + FactoryFinder finder = new FactoryFinder<>(TRANSPORT_FACTORY_PATH, + TransportFactory.class, implsWithWhiteSpace); + + // all white space should have been trimmed and all empty values skipped + // leading to the 3 valid classes loaded + assertEquals(Set.of(TcpTransportFactory.class, SslTransportFactory.class, + NIOTransportFactory.class), finder.getAllowedImpls()); + assertNotNull(finder.newInstance("tcp")); + assertNotNull(finder.newInstance("ssl")); + assertNotNull(finder.newInstance("nio")); + } +} diff --git a/activemq-jdbc-store/src/main/java/org/apache/activemq/store/jdbc/JDBCPersistenceAdapter.java b/activemq-jdbc-store/src/main/java/org/apache/activemq/store/jdbc/JDBCPersistenceAdapter.java index eaddfb8340f..23f3bc48d42 100644 --- a/activemq-jdbc-store/src/main/java/org/apache/activemq/store/jdbc/JDBCPersistenceAdapter.java +++ b/activemq-jdbc-store/src/main/java/org/apache/activemq/store/jdbc/JDBCPersistenceAdapter.java @@ -75,10 +75,10 @@ public class JDBCPersistenceAdapter extends DataSourceServiceSupport implements PersistenceAdapter { private static final Logger LOG = LoggerFactory.getLogger(JDBCPersistenceAdapter.class); - private static FactoryFinder adapterFactoryFinder = new FactoryFinder( - "META-INF/services/org/apache/activemq/store/jdbc/"); - private static FactoryFinder lockFactoryFinder = new FactoryFinder( - "META-INF/services/org/apache/activemq/store/jdbc/lock/"); + private static final FactoryFinder adapterFactoryFinder = new FactoryFinder<>( + "META-INF/services/org/apache/activemq/store/jdbc/", JDBCAdapter.class, null); + private static final FactoryFinder lockFactoryFinder = new FactoryFinder<>( + "META-INF/services/org/apache/activemq/store/jdbc/lock/", Locker.class, null); public static final long DEFAULT_LOCK_KEEP_ALIVE_PERIOD = 30 * 1000; @@ -456,7 +456,7 @@ protected JDBCAdapter createAdapter() throws IOException { return adapter; } - private Object loadAdapter(FactoryFinder finder, String kind) throws IOException { + private Object loadAdapter(FactoryFinder finder, String kind) throws IOException { Object adapter = null; TransactionContext c = getTransactionContext(); try { diff --git a/activemq-mqtt/src/main/java/org/apache/activemq/transport/mqtt/MQTTProtocolConverter.java b/activemq-mqtt/src/main/java/org/apache/activemq/transport/mqtt/MQTTProtocolConverter.java index 5e9b6234dba..badd9912173 100644 --- a/activemq-mqtt/src/main/java/org/apache/activemq/transport/mqtt/MQTTProtocolConverter.java +++ b/activemq-mqtt/src/main/java/org/apache/activemq/transport/mqtt/MQTTProtocolConverter.java @@ -126,7 +126,9 @@ public class MQTTProtocolConverter { public int version; - private final FactoryFinder STRATAGY_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/strategies/"); + private final FactoryFinder STRATEGY_FINDER = + new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/strategies/", + MQTTSubscriptionStrategy.class, null); /* * Subscription strategy configuration element. @@ -861,7 +863,7 @@ protected boolean containsMqttWildcard(String value) { protected MQTTSubscriptionStrategy findSubscriptionStrategy() throws IOException { if (subsciptionStrategy == null) { - synchronized (STRATAGY_FINDER) { + synchronized (STRATEGY_FINDER) { if (subsciptionStrategy != null) { return subsciptionStrategy; } @@ -869,7 +871,7 @@ protected MQTTSubscriptionStrategy findSubscriptionStrategy() throws IOException MQTTSubscriptionStrategy strategy = null; if (subscriptionStrategyName != null && !subscriptionStrategyName.isEmpty()) { try { - strategy = (MQTTSubscriptionStrategy) STRATAGY_FINDER.newInstance(subscriptionStrategyName); + strategy = STRATEGY_FINDER.newInstance(subscriptionStrategyName); LOG.debug("MQTT Using subscription strategy: {}", subscriptionStrategyName); if (strategy instanceof BrokerServiceAware) { ((BrokerServiceAware)strategy).setBrokerService(brokerService); diff --git a/activemq-spring/pom.xml b/activemq-spring/pom.xml index ed1a740610a..f4b4b059d59 100644 --- a/activemq-spring/pom.xml +++ b/activemq-spring/pom.xml @@ -240,7 +240,7 @@ ${basedir}/target/classes/activemq.xsd ${basedir}/target/classes false - org.apache.activemq.broker.jmx.AnnotatedMBean,org.apache.activemq.broker.jmx.DestinationViewMBean + org.apache.activemq.broker.jmx.AnnotatedMBean,org.apache.activemq.broker.jmx.DestinationViewMBean,org.apache.activemq.util.FactoryFinder mapping diff --git a/activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java b/activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java index 70456a7cd6c..4d96cf9eff8 100644 --- a/activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java +++ b/activemq-stomp/src/main/java/org/apache/activemq/transport/stomp/ProtocolConverter.java @@ -84,6 +84,10 @@ public class ProtocolConverter { private static final String BROKER_VERSION; private static final StompFrame ping = new StompFrame(Stomp.Commands.KEEPALIVE); + public static final String DEFAULT_ALLOWED_TRANSLATORS = FactoryFinder.buildAllowedImpls( + JmsFrameTranslator.class, LegacyFrameTranslator.class); + public static final String FRAME_TRANSLATOR_CLASSES_PROP = "org.apache.activemq.stomp.FRAME_TRANSLATOR_CLASSES"; + static { String version = "5.6.0"; try(InputStream in = ProtocolConverter.class.getResourceAsStream("/org/apache/activemq/version.txt")) { @@ -108,7 +112,7 @@ public class ProtocolConverter { private final LongSequenceGenerator transactionIdGenerator = new LongSequenceGenerator(); private final LongSequenceGenerator tempDestinationGenerator = new LongSequenceGenerator(); - private final ConcurrentMap resposeHandlers = new ConcurrentHashMap<>(); + private final ConcurrentMap responseHandlers = new ConcurrentHashMap<>(); private final ConcurrentMap subscriptionsByConsumerId = new ConcurrentHashMap<>(); private final ConcurrentMap subscriptions = new ConcurrentHashMap<>(); private final ConcurrentMap tempDestinations = new ConcurrentHashMap<>(); @@ -121,13 +125,13 @@ public class ProtocolConverter { // Read-Only view used in this class to enforce the separation of read vs update of the global index. private final Map pendingAcks = Collections.unmodifiableMap(pendingAcksTracker); - private final Object commnadIdMutex = new Object(); + private final Object commandIdMutex = new Object(); private int lastCommandId; private final AtomicBoolean connected = new AtomicBoolean(false); - private final FrameTranslator frameTranslator = new LegacyFrameTranslator(); - private ConcurrentMap jmsFrameTranslators=new ConcurrentHashMap(); - - private final FactoryFinder FRAME_TRANSLATOR_FINDER = new FactoryFinder("META-INF/services/org/apache/activemq/transport/frametranslator/"); + private final FrameTranslator defaultFrameTranslator = new LegacyFrameTranslator(); + private final Map jmsFrameTranslators = new ConcurrentHashMap<>(); + + private final FactoryFinder frameTranslatorFinder; private final BrokerContext brokerContext; private String version = "1.0"; private long hbReadInterval; @@ -138,10 +142,12 @@ public class ProtocolConverter { public ProtocolConverter(StompTransport stompTransport, BrokerContext brokerContext) { this.stompTransport = stompTransport; this.brokerContext = brokerContext; + this.frameTranslatorFinder = new FactoryFinder<>("META-INF/services/org/apache/activemq/transport/frametranslator/", + FrameTranslator.class, System.getProperty(FRAME_TRANSLATOR_CLASSES_PROP, DEFAULT_ALLOWED_TRANSLATORS)); } protected int generateCommandId() { - synchronized (commnadIdMutex) { + synchronized (commandIdMutex) { return lastCommandId++; } } @@ -174,7 +180,7 @@ protected void sendToActiveMQ(Command command, ResponseHandler handler) { command.setCommandId(generateCommandId()); if (handler != null) { command.setResponseRequired(true); - resposeHandlers.put(command.getCommandId(), handler); + responseHandlers.put(command.getCommandId(), handler); } stompTransport.sendToActiveMQ(command); } @@ -188,29 +194,21 @@ protected FrameTranslator findTranslator(String header) { } protected FrameTranslator findTranslator(String header, ActiveMQDestination destination, boolean advisory) { - FrameTranslator translator = frameTranslator; + FrameTranslator translator = null; try { if (header != null) { - translator=jmsFrameTranslators.get(header); - if(translator==null) { - LOG.info("Creating a new FrameTranslator to convert "+header); - translator = (FrameTranslator) FRAME_TRANSLATOR_FINDER.newInstance(header); - if(translator!=null) { - LOG.info("Created a new FrameTranslator to convert "+header); - jmsFrameTranslators.put(header,translator); - }else { - LOG.error("Failed in creating FrameTranslator to convert "+header); - } - } - } else { - if (destination != null && (advisory || AdvisorySupport.isAdvisoryTopic(destination))) { - translator = new JmsFrameTranslator(); - } + translator = loadTranslator(header); + } else if (destination != null && (advisory || AdvisorySupport.isAdvisoryTopic(destination))) { + translator = new JmsFrameTranslator(); } } catch (Exception ignore) { // if anything goes wrong use the default translator LOG.debug("Failed in getting a FrameTranslator to convert ", ignore); - translator = frameTranslator; + } + + // fallback to default if we can't find/load one + if (translator == null) { + translator = defaultFrameTranslator; } if (translator instanceof BrokerContextAware) { @@ -219,6 +217,23 @@ protected FrameTranslator findTranslator(String header, ActiveMQDestination dest return translator; } + + protected FrameTranslator loadTranslator(String header) throws Exception { + FrameTranslator translator = jmsFrameTranslators.get(header); + + if (translator == null) { + LOG.info("Creating a new FrameTranslator to convert {}", header); + translator = frameTranslatorFinder.newInstance(header); + if (translator != null) { + LOG.info("Created a new FrameTranslator to convert {}", header); + jmsFrameTranslators.put(header, translator); + } else { + LOG.error("Failed in creating FrameTranslator to convert {}", header); + } + } + + return translator; + } /** * Convert a STOMP command @@ -617,9 +632,11 @@ protected void onStompSubscribe(StompFrame command) throws ProtocolException { StompSubscription stompSubscription; if (!consumerInfo.isBrowser()) { - stompSubscription = new StompSubscription(this, subscriptionId, consumerInfo, headers.get(Stomp.Headers.TRANSFORMATION), pendingAcksTracker); + stompSubscription = new StompSubscription(this, subscriptionId, consumerInfo, headers.get( + Stomp.Headers.TRANSFORMATION), pendingAcksTracker); } else { - stompSubscription = new StompQueueBrowserSubscription(this, subscriptionId, consumerInfo, headers.get(Stomp.Headers.TRANSFORMATION), pendingAcksTracker); + stompSubscription = new StompQueueBrowserSubscription(this, subscriptionId, consumerInfo, headers.get( + Stomp.Headers.TRANSFORMATION), pendingAcksTracker); } stompSubscription.setDestination(actualDest); @@ -863,7 +880,7 @@ protected void checkConnected() throws ProtocolException { public void onActiveMQCommand(Command command) throws IOException, JMSException { if (command.isResponse()) { Response response = (Response)command; - ResponseHandler rh = resposeHandlers.remove(response.getCorrelationId()); + ResponseHandler rh = responseHandlers.remove(response.getCorrelationId()); if (rh != null) { rh.onResponse(this, response); } else { @@ -895,7 +912,7 @@ public ActiveMQMessage convertMessage(StompFrame command) throws IOException, JM public StompFrame convertMessage(ActiveMQMessage message, boolean ignoreTransformation) throws IOException, JMSException { if (ignoreTransformation == true) { - return frameTranslator.convertMessage(this, message); + return defaultFrameTranslator.convertMessage(this, message); } else { FrameTranslator translator = findTranslator( message.getStringProperty(Stomp.Headers.TRANSFORMATION), message.getDestination(), message.isAdvisory()); diff --git a/activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/ProtocolConverterTest.java b/activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/ProtocolConverterTest.java new file mode 100644 index 00000000000..17d293e3d89 --- /dev/null +++ b/activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/ProtocolConverterTest.java @@ -0,0 +1,192 @@ +/* + * 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.transport.stomp; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; + +import org.apache.activemq.transport.stomp.StompTest.TestTranslator; +import org.apache.activemq.util.FactoryFinder; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category(ParallelTest.class) +public class ProtocolConverterTest { + + private final StompTransport transport = mock(StompTransport.class); + // Before each test reset to defaults + @Before + public void setUp() { + resetDefaultAllowedTranslators(); + } + + // clean up after all tests are done finished by resetting the property back before exiting + @AfterClass + public static void tearDown() { + resetDefaultAllowedTranslators(); + } + + @Test + public void testLoadMissingTranslator() throws Exception { + ProtocolConverter converter = new ProtocolConverter(transport, null); + + // Should fallback to default + assertEquals(LegacyFrameTranslator.class, converter.findTranslator("abc").getClass()); + + try { + converter.loadTranslator("abc"); + fail("should have failed"); + } catch (Exception e) { + assertTrue(e.getMessage().contains("Could not find factory class for resource")); + } + } + + @Test + public void testAllowedTranslators() throws Exception { + // first test defaults + ProtocolConverter converter = new ProtocolConverter(transport, null); + assertNotNull(converter.loadTranslator("jms-json")); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-json").getClass()); + assertNotNull(converter.loadTranslator("jms-xml")); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-xml").getClass()); + + // test specific allowed + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, + JmsFrameTranslator.class.getName()); + converter = new ProtocolConverter(transport, null); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-json").getClass()); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-byte").getClass()); + + // test all allowed + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, "*"); + converter = new ProtocolConverter(transport, null); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-json").getClass()); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-byte").getClass()); + // our custom is also allowed + assertNotNull(converter.loadTranslator("stomp-test")); + assertEquals(TestTranslator.class, converter.findTranslator("stomp-test").getClass()); + } + + @Test + public void testAllowedTranslatorsErrors() throws Exception { + // Disable all and allow none + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, ""); + ProtocolConverter converter = new ProtocolConverter(transport, null); + + assertTranslatorNotAllowed(converter, "jms-json"); + + // should fall back to default legacy on error + assertEquals(LegacyFrameTranslator.class, converter.findTranslator("jms-json").getClass()); + + // test custom translator not in allowed list + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, JmsFrameTranslator.class.getName()); + converter = new ProtocolConverter(transport, null); + assertTranslatorNotAllowed(converter, "stomp-test"); + + } + + @Test + public void testCustomAllowedTranslator() throws Exception { + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, + FactoryFinder.buildAllowedImpls(JmsFrameTranslator.class, TestTranslator.class)); + ProtocolConverter converter = new ProtocolConverter(transport, null); + + // JmsFrameTranslator is allowed + assertNotNull(converter.loadTranslator("jms-json")); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-json").getClass()); + + // our custom is also allowed + assertNotNull(converter.loadTranslator("stomp-test")); + assertEquals(TestTranslator.class, converter.findTranslator("stomp-test").getClass()); + } + + @Test + public void testWhiteSpaceAllowedTranslator() throws Exception { + // white space should be trimmed + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, + String.join(", ", JmsFrameTranslator.class.getName(), + TestTranslator.class.getName())); + ProtocolConverter converter = new ProtocolConverter(transport, null); + + // JmsFrameTranslator is allowed + assertNotNull(converter.loadTranslator("jms-json")); + assertEquals(JmsFrameTranslator.class, converter.findTranslator("jms-json").getClass()); + + // our custom is also allowed + assertNotNull(converter.loadTranslator("stomp-test")); + assertEquals(TestTranslator.class, converter.findTranslator("stomp-test").getClass()); + } + + @Test + public void testMissingInterface() throws Exception { + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, "*"); + ProtocolConverter converter = new ProtocolConverter(transport, null); + + try { + converter.loadTranslator("stomp-test-invalid"); + fail("should have failed"); + } catch (InstantiationException e) { + assertTrue(e.getMessage().contains(BadTranslator.class.getName() + " is not assignable to interface " + + FrameTranslator.class.getName())); + } + + // fallback + assertEquals(LegacyFrameTranslator.class, converter.findTranslator("stomp-test-invalid").getClass()); + } + + @Test + public void testPathTraversal() throws Exception { + ProtocolConverter converter = new ProtocolConverter(transport, null); + + try { + converter.loadTranslator("../stomp-test"); + fail("should have failed"); + } catch (InstantiationException e) { + assertTrue(e.getMessage().contains("rovided key escapes the FactoryFinder configured directory")); + } + + // fallback + assertEquals(LegacyFrameTranslator.class, converter.findTranslator("../stomp-test").getClass()); + } + + private void assertTranslatorNotAllowed(ProtocolConverter converter, String key) throws Exception { + try { + converter.loadTranslator(key); + fail("Should have failed"); + } catch (InstantiationException e) { + assertTrue(e.getMessage().contains("is not an allowed implementation of type interface " + + FrameTranslator.class.getName())); + } + // should fall back to default legacy on error + assertEquals(LegacyFrameTranslator.class, converter.findTranslator(key).getClass()); + } + + private static void resetDefaultAllowedTranslators() { + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, + ProtocolConverter.DEFAULT_ALLOWED_TRANSLATORS); + } + + // don't implement the correct interface + public static class BadTranslator { + + } +} diff --git a/activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/StompTest.java b/activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/StompTest.java index 218dda65249..70fa9ed4f84 100644 --- a/activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/StompTest.java +++ b/activemq-stomp/src/test/java/org/apache/activemq/transport/stomp/StompTest.java @@ -23,6 +23,7 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import jakarta.jms.Destination; import java.io.IOException; import java.io.StringReader; import java.net.SocketTimeoutException; @@ -32,6 +33,7 @@ import java.util.Map; import java.util.UUID; import java.util.concurrent.TimeUnit; +import java.util.function.Consumer; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -59,6 +61,7 @@ import org.apache.activemq.broker.region.policy.PolicyEntry; import org.apache.activemq.broker.region.policy.PolicyMap; import org.apache.activemq.command.ActiveMQDestination; +import org.apache.activemq.command.ActiveMQMessage; import org.apache.activemq.command.ActiveMQQueue; import org.apache.activemq.command.ActiveMQTextMessage; import org.apache.activemq.util.Wait; @@ -1241,6 +1244,89 @@ public void testTransformationReceiveObject() throws Exception { compareFrameXML(frame, xmlObject); } + @Test(timeout = 60000) + public void testTransformationSuccess() throws Exception { + // transformation is allowed, use default configured transformers + assertStompTransformation(null, + Stomp.Transformations.JMS_OBJECT_XML.toString(), + frame -> assertTrue(frame.contains(""))); + + // transformation is allowed + assertStompTransformation(ProtocolConverter.DEFAULT_ALLOWED_TRANSLATORS, + Stomp.Transformations.JMS_OBJECT_XML.toString(), + frame -> assertTrue(frame.contains(""))); + + // transformation is allowed with wildcard, xml tag should exist + assertStompTransformation("*", + Stomp.Transformations.JMS_OBJECT_XML.toString(), + frame -> assertTrue(frame.contains(""))); + + // verify TestTranslator was used and appended header + assertStompTransformation(TestTranslator.class.getName(), + "stomp-test", + frame -> assertTrue(frame.contains("stomp-test-translator-header"))); + } + + @Test(timeout = 60000) + public void testTransformationFailuresFallbackToDefault() throws Exception { + // path traversal is not allowed so given translator should fail + assertStompTransformation(ProtocolConverter.DEFAULT_ALLOWED_TRANSLATORS, + "../" + Stomp.Transformations.JMS_OBJECT_XML, + frame -> assertEquals(-1, frame.indexOf(""))); + + // xml translator maps to JmsFrameTranslator.class which is not allowed + assertStompTransformation(LegacyFrameTranslator.class.getName(), + Stomp.Transformations.JMS_OBJECT_XML.toString(), + frame -> assertEquals(-1, frame.indexOf(""))); + + // Allowed translators are set to none so translator should fail + assertStompTransformation("", Stomp.Transformations.JMS_OBJECT_XML.toString(), + frame -> assertEquals(-1, frame.indexOf(""))); + + // TestTranslator is not allowed so won't have our header is it will fall back to + // the legacy default translator + assertStompTransformation(ProtocolConverter.DEFAULT_ALLOWED_TRANSLATORS, + "stomp-test", + frame -> assertFalse(frame.contains("stomp-test-translator-header"))); + } + + private void assertStompTransformation(String allowedTranslators, + String transformation, Consumer verifier) throws Exception { + try { + if (allowedTranslators != null) { + tearDown(); + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, + allowedTranslators); + setUp(); + } + + MessageProducer producer = session.createProducer( + new ActiveMQQueue("USERS." + getQueueName())); + ObjectMessage message = session.createObjectMessage( + new SamplePojo("Dejan", "Belgrade")); + producer.send(message); + + String frame = "CONNECT\n" + "login:system\n" + "passcode:manager\n\n" + Stomp.NULL; + stompConnection.sendFrame(frame); + + frame = stompConnection.receiveFrame(); + assertTrue(frame.startsWith("CONNECTED")); + + frame = "SUBSCRIBE\n" + "destination:/queue/USERS." + getQueueName() + "\n" + "ack:auto" + + "\n" + "transformation:" + transformation + "\n\n" + + Stomp.NULL; + stompConnection.sendFrame(frame); + frame = stompConnection.receiveFrame(); + + verifier.accept(frame); + } finally { + tearDown(); + System.setProperty(ProtocolConverter.FRAME_TRANSLATOR_CLASSES_PROP, + ProtocolConverter.DEFAULT_ALLOWED_TRANSLATORS); + setUp(); + } + } + private void compareFrameXML(String frame, String xmlObject) { String xmlReceived = frame.trim().substring(frame.indexOf("")); @@ -2554,4 +2640,35 @@ private Map createMapObject(HierarchicalStreamReader in) throws Map map = (Map)xstream.unmarshal(in); return map; } + + public static class TestTranslator implements FrameTranslator { + private final JmsFrameTranslator translator = new JmsFrameTranslator(); + + @Override + public ActiveMQMessage convertFrame(ProtocolConverter converter, StompFrame command) + throws JMSException, ProtocolException { + return translator.convertFrame(converter, command); + } + + @Override + public StompFrame convertMessage(ProtocolConverter converter, ActiveMQMessage message) + throws IOException, JMSException { + StompFrame frame = translator.convertMessage(converter, message); + // add a header to make it easy to detect if translator was used + frame.getHeaders().put("stomp-test-translator-header", "header-value"); + return frame; + } + + @Override + public String convertDestination(ProtocolConverter converter, Destination d) { + return translator.convertDestination(converter, d); + } + + @Override + public ActiveMQDestination convertDestination(ProtocolConverter converter, String name, + boolean forceFallback) throws ProtocolException { + return translator.convertDestination(converter, name, forceFallback); + } + } + } diff --git a/activemq-stomp/src/test/resources/META-INF/services/org/apache/activemq/transport/frametranslator/stomp-test b/activemq-stomp/src/test/resources/META-INF/services/org/apache/activemq/transport/frametranslator/stomp-test new file mode 100644 index 00000000000..377559fb27a --- /dev/null +++ b/activemq-stomp/src/test/resources/META-INF/services/org/apache/activemq/transport/frametranslator/stomp-test @@ -0,0 +1,17 @@ +## --------------------------------------------------------------------------- +## 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. +## ----------------------------- +class=org.apache.activemq.transport.stomp.StompTest$TestTranslator diff --git a/activemq-stomp/src/test/resources/META-INF/services/org/apache/activemq/transport/frametranslator/stomp-test-invalid b/activemq-stomp/src/test/resources/META-INF/services/org/apache/activemq/transport/frametranslator/stomp-test-invalid new file mode 100644 index 00000000000..6a94af94060 --- /dev/null +++ b/activemq-stomp/src/test/resources/META-INF/services/org/apache/activemq/transport/frametranslator/stomp-test-invalid @@ -0,0 +1,17 @@ +## --------------------------------------------------------------------------- +## 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. +## ----------------------------- +class=org.apache.activemq.transport.stomp.ProtocolConverterTest$BadTranslator diff --git a/activemq-web/src/main/java/org/apache/activemq/web/QueueBrowseServlet.java b/activemq-web/src/main/java/org/apache/activemq/web/QueueBrowseServlet.java index 7215ddd1b50..435ffdc6c83 100644 --- a/activemq-web/src/main/java/org/apache/activemq/web/QueueBrowseServlet.java +++ b/activemq-web/src/main/java/org/apache/activemq/web/QueueBrowseServlet.java @@ -35,6 +35,8 @@ import org.apache.activemq.util.FactoryFinder; import org.apache.activemq.util.IntrospectionSupport; import org.apache.activemq.web.view.MessageRenderer; +import org.apache.activemq.web.view.RssMessageRenderer; +import org.apache.activemq.web.view.SimpleMessageRenderer; import org.apache.activemq.web.view.XmlMessageRenderer; /** @@ -46,10 +48,20 @@ *
  • selector - specifies the SQL 92 selector to apply to the queue
    • * * - * + * */ public class QueueBrowseServlet extends HttpServlet { - private static FactoryFinder factoryFinder = new FactoryFinder("META-INF/services/org/apache/activemq/web/view/"); + + public static final String QUEUE_BROWSE_VIEWS_PROP = "org.apache.activemq.web.view.QUEUE_BROWSE_CLASSES"; + public static final String DEFAULT_ALLOWED_VIEWS = FactoryFinder.buildAllowedImpls( + RssMessageRenderer.class, XmlMessageRenderer.class, SimpleMessageRenderer.class); + + private final FactoryFinder factoryFinder; + + public QueueBrowseServlet() { + this.factoryFinder = new FactoryFinder<>("META-INF/services/org/apache/activemq/web/view/", + MessageRenderer.class, System.getProperty(QUEUE_BROWSE_VIEWS_PROP, DEFAULT_ALLOWED_VIEWS)); + } // Implementation methods // ------------------------------------------------------------------------- @@ -96,24 +108,17 @@ protected MessageRenderer getMessageRenderer(HttpServletRequest request) throws style = "simple"; } try { - return (MessageRenderer) factoryFinder.newInstance(style); - } - catch (IllegalAccessException e) { - throw new NoSuchViewStyleException(style, e); - } - catch (InstantiationException e) { - throw new NoSuchViewStyleException(style, e); + return factoryFinder.newInstance(style); } - catch (ClassNotFoundException e) { + catch (IllegalAccessException | InstantiationException | ClassNotFoundException e) { throw new NoSuchViewStyleException(style, e); } } - @SuppressWarnings("unchecked") protected void configureRenderer(HttpServletRequest request, MessageRenderer renderer) { - Map properties = new HashMap(); + Map properties = new HashMap<>(); for (Enumeration iter = request.getParameterNames(); iter.hasMoreElements();) { - String name = (String) iter.nextElement(); + String name = iter.nextElement(); properties.put(name, request.getParameter(name)); } IntrospectionSupport.setProperties(renderer, properties); diff --git a/activemq-web/src/test/java/org/apache/activemq/web/QueueBrowseServletTest.java b/activemq-web/src/test/java/org/apache/activemq/web/QueueBrowseServletTest.java new file mode 100644 index 00000000000..8abea6d40ae --- /dev/null +++ b/activemq-web/src/test/java/org/apache/activemq/web/QueueBrowseServletTest.java @@ -0,0 +1,230 @@ +/* + * 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.web; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import jakarta.jms.JMSException; +import jakarta.jms.Message; +import jakarta.jms.QueueBrowser; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import java.io.PrintWriter; +import org.apache.activemq.util.FactoryFinder; +import org.apache.activemq.web.view.MessageRenderer; +import org.apache.activemq.web.view.RssMessageRenderer; +import org.apache.activemq.web.view.SimpleMessageRenderer; +import org.apache.activemq.web.view.XmlMessageRenderer; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.Test; + +public class QueueBrowseServletTest { + + private final HttpServletRequest request = mock(HttpServletRequest.class); + + // Before each test reset to defaults + @Before + public void setUp() { + resetDefaultAllowedViews(); + } + + // clean up after all tests are done finished by resetting the property back before exiting + @AfterClass + public static void tearDown() { + resetDefaultAllowedViews(); + } + + @Test + public void testDefaultViews() throws Exception { + QueueBrowseServlet servlet = new QueueBrowseServlet(); + + when(request.getParameter("view")).thenReturn("simple"); + assertEquals(SimpleMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + + when(request.getParameter("view")).thenReturn(null); + assertEquals(SimpleMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + + when(request.getParameter("view")).thenReturn("xml"); + assertEquals(XmlMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + + when(request.getParameter("view")).thenReturn("rss"); + assertEquals(RssMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + } + + @Test + public void testPathTraversal() throws Exception { + QueueBrowseServlet servlet = new QueueBrowseServlet(); + // illegal path traversal + when(request.getParameter("view")).thenReturn("../../simple"); + try { + servlet.getMessageRenderer(request); + fail("Should have thrown an exception"); + } catch (NoSuchViewStyleException e) { + Throwable rootCause = e.getRootCause(); + assertTrue(rootCause instanceof InstantiationException); + // view is in allow list but wrong interface type + assertEquals(rootCause.getMessage(), "Provided key escapes the FactoryFinder configured directory"); + } + } + + @Test + public void testViewAllowlistSimpleOnly() throws Exception { + // only allow simple and rss view + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, + FactoryFinder.buildAllowedImpls(SimpleMessageRenderer.class, + RssMessageRenderer.class)); + + QueueBrowseServlet servlet = new QueueBrowseServlet(); + + // simple/rss are allowed + when(request.getParameter("view")).thenReturn("simple"); + assertEquals(SimpleMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + when(request.getParameter("view")).thenReturn("rss"); + assertEquals(RssMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + + // XmlMessageRenderer is not in the allow list so this should be blocked + assertViewAllowListFailure(servlet, "xml"); + } + + @Test + public void testViewAllowlistNone() throws Exception { + // Set to none allowed + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, ""); + QueueBrowseServlet servlet = new QueueBrowseServlet(); + assertViewAllowListFailure(servlet, "simple"); + assertViewAllowListFailure(servlet, "xml"); + } + + @Test + public void testViewAllowAll() throws Exception { + // Allow all + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, "*"); + QueueBrowseServlet servlet = new QueueBrowseServlet(); + + when(request.getParameter("view")).thenReturn("simple"); + assertEquals(SimpleMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + } + + @Test + public void testMissingInterface() throws Exception { + // Add class with wrong interface to allow list + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, InvalidMessageRender.class.getName()); + + try { + // servlet should fail on creation + new QueueBrowseServlet(); + fail("Should have thrown an exception"); + } catch (IllegalArgumentException e) { + assertTrue(e.getMessage().contains(InvalidMessageRender.class.getName() + + " is not assignable to interface " + MessageRenderer.class.getName())); + } + when(request.getParameter("view")).thenReturn("invalid-view"); + + // set to allow all + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, "*"); + QueueBrowseServlet servlet = new QueueBrowseServlet(); + + // should fail on lookup with same interface issue + try { + when(request.getParameter("view")).thenReturn("invalid-view"); + servlet.getMessageRenderer(request); + fail("Should not be allowed"); + } catch (NoSuchViewStyleException e) { + Throwable rootCause = e.getRootCause(); + assertTrue(rootCause instanceof InstantiationException); + // view is in allow list but wrong interface type + assertTrue(rootCause.getMessage().contains(InvalidMessageRender.class.getName() + + " is not assignable to interface " + MessageRenderer.class.getName())); + } + } + + @Test + public void testViewCustomAllow() throws Exception { + // default view settings + QueueBrowseServlet servlet = new QueueBrowseServlet(); + when(request.getParameter("view")).thenReturn("valid-view"); + + // custom view is not in the allow list + assertViewAllowListFailure(servlet, "valid-view"); + + // reset with view added to allow list + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, ValidMessageRender.class.getName()); + servlet = new QueueBrowseServlet(); + + assertEquals(ValidMessageRender.class, servlet.getMessageRenderer(request).getClass()); + } + + @Test + public void testViewsWhiteSpace() throws Exception { + // add views with extra white space which should get trimmed + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, + String.join(", ", SimpleMessageRenderer.class.getName(), + RssMessageRenderer.class.getName())); + + QueueBrowseServlet servlet = new QueueBrowseServlet(); + when(request.getParameter("view")).thenReturn("simple"); + assertEquals(SimpleMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + when(request.getParameter("view")).thenReturn("rss"); + assertEquals(RssMessageRenderer.class, servlet.getMessageRenderer(request).getClass()); + } + + private void assertViewAllowListFailure(QueueBrowseServlet servlet, String view) throws Exception { + try { + when(request.getParameter("view")).thenReturn(view); + servlet.getMessageRenderer(request); + fail("Should not be allowed"); + } catch (NoSuchViewStyleException e) { + Throwable rootCause = e.getRootCause(); + assertTrue(rootCause instanceof InstantiationException); + assertTrue(rootCause.getMessage().contains("is not an allowed " + + "implementation of type interface " + MessageRenderer.class.getName())); + } + } + + private static void resetDefaultAllowedViews() { + System.setProperty(QueueBrowseServlet.QUEUE_BROWSE_VIEWS_PROP, + QueueBrowseServlet.DEFAULT_ALLOWED_VIEWS); + } + + // Does not implement the right interface + public static class InvalidMessageRender { + + } + + public static class ValidMessageRender implements MessageRenderer { + + @Override + public void renderMessages(HttpServletRequest request, HttpServletResponse response, + QueueBrowser browser) throws IOException, JMSException, ServletException { + + } + + @Override + public void renderMessage(PrintWriter writer, HttpServletRequest request, + HttpServletResponse response, QueueBrowser browser, Message message) + throws JMSException, ServletException { + + } + } +} diff --git a/activemq-web/src/test/resources/META-INF/services/org/apache/activemq/web/view/invalid-view b/activemq-web/src/test/resources/META-INF/services/org/apache/activemq/web/view/invalid-view new file mode 100644 index 00000000000..6338f3286f4 --- /dev/null +++ b/activemq-web/src/test/resources/META-INF/services/org/apache/activemq/web/view/invalid-view @@ -0,0 +1,19 @@ +## --------------------------------------------------------------------------- +## 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. +## --------------------------------------------------------------------------- + +# use a class that does not implement MessageRenderer to test validation +class=org.apache.activemq.web.QueueBrowseServletTest$InvalidMessageRender diff --git a/activemq-web/src/test/resources/META-INF/services/org/apache/activemq/web/view/valid-view b/activemq-web/src/test/resources/META-INF/services/org/apache/activemq/web/view/valid-view new file mode 100644 index 00000000000..af1e9e360b5 --- /dev/null +++ b/activemq-web/src/test/resources/META-INF/services/org/apache/activemq/web/view/valid-view @@ -0,0 +1,19 @@ +## --------------------------------------------------------------------------- +## 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. +## --------------------------------------------------------------------------- + +# use a class that does not implement MessageRenderer to test validation +class=org.apache.activemq.web.QueueBrowseServletTest$ValidMessageRender