diff --git a/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/BrokerView.java b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/BrokerView.java index 619912c01a0..a23680113dc 100644 --- a/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/BrokerView.java +++ b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/BrokerView.java @@ -44,6 +44,8 @@ public class BrokerView implements BrokerViewMBean { private static final Logger LOG = LoggerFactory.getLogger(BrokerView.class); + private static final Set DENIED_TRANSPORT_SCHEMES = Set.of("vm", "http"); + ManagedRegionBroker broker; private final BrokerService brokerService; @@ -402,7 +404,7 @@ public ObjectName[] getDynamicDestinationProducers() { @Override public String addConnector(String discoveryAddress) throws Exception { - // Verify VM transport is not used + // Verify a denied transport scheme is not used validateAllowedUrl(discoveryAddress); TransportConnector connector = brokerService.addConnector(discoveryAddress); if (connector == null) { @@ -414,7 +416,7 @@ public String addConnector(String discoveryAddress) throws Exception { @Override public String addNetworkConnector(String discoveryAddress) throws Exception { - // Verify VM transport is not used + // Verify a denied transport scheme is not used validateAllowedUrl(discoveryAddress); NetworkConnector connector = brokerService.addNetworkConnector(discoveryAddress); if (connector == null) { @@ -607,7 +609,7 @@ private static void validateAllowedUrl(String uriString) throws URISyntaxExcepti validateAllowedUri(new URI(uriString), 0); } - // Validate the URI does not contain VM transport + // Validate the URI does not contain a denied transport scheme private static void validateAllowedUri(URI uri, int depth) throws URISyntaxException { // Don't allow more than 5 nested URIs to prevent blowing the stack // If we are greater than 4 then this is the 5th level of composite @@ -635,10 +637,13 @@ private static void validateAllowedUri(URI uri, int depth) throws URISyntaxExcep } } - // We don't allow VM transport scheme to be used + // Check all denied schemes private static void validateAllowedScheme(String scheme) { - if (scheme.equals("vm")) { - throw new IllegalArgumentException("VM scheme is not allowed"); + for (String denied : DENIED_TRANSPORT_SCHEMES) { + // The schemes should be case-insensitive but ignore case as a precaution + if (scheme.equalsIgnoreCase(denied)) { + throw new IllegalArgumentException("Transport scheme '" + scheme + "' is not allowed"); + } } } } diff --git a/activemq-unit-tests/src/test/java/org/apache/activemq/broker/jmx/MBeanTest.java b/activemq-unit-tests/src/test/java/org/apache/activemq/broker/jmx/MBeanTest.java index 7559664ba13..e60f3b0a198 100644 --- a/activemq-unit-tests/src/test/java/org/apache/activemq/broker/jmx/MBeanTest.java +++ b/activemq-unit-tests/src/test/java/org/apache/activemq/broker/jmx/MBeanTest.java @@ -2060,34 +2060,50 @@ public void testSubscriptionViewProperties() throws Exception { assertTrue(subscription.isExclusive()); } - // Test to verify VM transport is not allowed to be added as a connector + // Test to verify http transport is not allowed to be added as a connector + // through the Broker MBean + public void testAddHttpConnectorBlockedBrokerView() throws Exception { + testAddTransportConnectorBlockedBrokerView("http"); + } + + // Test to verify vm transport is not allowed to be added as a connector // through the Broker MBean public void testAddVmConnectorBlockedBrokerView() throws Exception { + testAddTransportConnectorBlockedBrokerView("vm"); + } + + protected void testAddTransportConnectorBlockedBrokerView(String scheme) throws Exception { ObjectName brokerName = assertRegisteredObjectName(domain + ":type=Broker,brokerName=localhost"); BrokerViewMBean brokerView = MBeanServerInvocationHandler.newProxyInstance(mbeanServer, brokerName, BrokerViewMBean.class, true); try { - brokerView.addConnector("vm://localhost"); - fail("Should have failed trying to add vm connector"); + brokerView.addConnector(scheme + "://localhost"); + fail("Should have failed trying to add connector"); } catch (IllegalArgumentException e) { - assertEquals("VM scheme is not allowed", e.getMessage()); + assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage()); } try { // verify any composite URI is blocked as well - brokerView.addConnector("failover:(tcp://0.0.0.0:0,vm://" + brokerName + ")"); - fail("Should have failed trying to add vm connector"); + brokerView.addConnector("failover:(tcp://0.0.0.0:0," + scheme + "://" + brokerName + ")"); + fail("Should have failed trying to add connector"); } catch (IllegalArgumentException e) { - assertEquals("VM scheme is not allowed", e.getMessage()); + assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage()); } try { // verify nested composite URI is blocked - brokerView.addConnector("failover:(failover:(failover:(vm://localhost)))"); - fail("Should have failed trying to add vm connector"); + brokerView.addConnector("failover:(failover:(failover:(" + scheme + "://localhost)))"); + fail("Should have failed trying to add connector"); } catch (IllegalArgumentException e) { - assertEquals("VM scheme is not allowed", e.getMessage()); + assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage()); } + } + + // Test too many nested URIs + public void testNestedAddTransportConnector() throws Exception { + ObjectName brokerName = assertRegisteredObjectName(domain + ":type=Broker,brokerName=localhost"); + BrokerViewMBean brokerView = MBeanServerInvocationHandler.newProxyInstance(mbeanServer, brokerName, BrokerViewMBean.class, true); try { // verify nested composite URI with more than 5 levels is blocked @@ -2097,7 +2113,6 @@ public void testAddVmConnectorBlockedBrokerView() throws Exception { } catch (IllegalArgumentException e) { assertEquals("URI can't contain more than 5 nested composite URIs", e.getMessage()); } - } } diff --git a/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxCreateNCTest.java b/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxCreateNCTest.java index 1b3548432bc..f7f57b444f4 100644 --- a/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxCreateNCTest.java +++ b/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxCreateNCTest.java @@ -33,8 +33,6 @@ /** * This test shows that when we create a network connector via JMX, * the NC/bridge shows up in the MBean Server - * - * @author Christian Posta */ public class JmxCreateNCTest { @@ -82,35 +80,44 @@ public void testBridgeRegistration() throws Exception { @Test public void testVmBridgeBlocked() throws Exception { + testDeniedBridgeBlocked("vm"); + } + + @Test + public void testHttpBridgeBlocked() throws Exception { + testDeniedBridgeBlocked("http"); + } + + protected void testDeniedBridgeBlocked(String scheme) throws Exception { // Test composite network connector uri try { - proxy.addNetworkConnector("static:(vm://localhost)"); - fail("Should have failed trying to add vm connector bridge"); + proxy.addNetworkConnector("static:(" + scheme + "://localhost)"); + fail("Should have failed trying to add connector bridge"); } catch (IllegalArgumentException e) { - assertEquals("VM scheme is not allowed", e.getMessage()); + assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage()); } try { - proxy.addNetworkConnector("multicast:(vm://localhost)"); - fail("Should have failed trying to add vm connector bridge"); + proxy.addNetworkConnector("multicast:(" + scheme + "://localhost)"); + fail("Should have failed trying to add connector bridge"); } catch (IllegalArgumentException e) { - assertEquals("VM scheme is not allowed", e.getMessage()); + assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage()); } - // verify direct vm as well + // verify direct connector as well try { - proxy.addNetworkConnector("vm://localhost"); - fail("Should have failed trying to add vm connector bridge"); + proxy.addNetworkConnector(scheme + "://localhost"); + fail("Should have failed trying to add connector bridge"); } catch (IllegalArgumentException e) { - assertEquals("VM scheme is not allowed", e.getMessage()); + assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage()); } try { // verify nested composite URI is blocked - proxy.addNetworkConnector("static:(failover:(failover:(tcp://localhost:0,vm://localhost)))"); - fail("Should have failed trying to add vm connector bridge"); + proxy.addNetworkConnector("static:(failover:(failover:(tcp://localhost:0," + scheme + "://localhost)))"); + fail("Should have failed trying to add connector bridge"); } catch (IllegalArgumentException e) { - assertEquals("VM scheme is not allowed", e.getMessage()); + assertEquals("Transport scheme '" + scheme + "' is not allowed", e.getMessage()); } }