Add Http discovery transport to denied list for JMX (#1918) (#1926)

This also prevents the Http discovery transport from being added as a
connector or network connector through JMX and Jolokia

(cherry picked from commit 5bda7d8)

Author: cshannon

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<String> DENIED_TRANSPORT_SCHEMES = Set.of("vm", "http");
+
     ManagedRegionBroker broker;
 
     private final BrokerService brokerService;
@@ -372,7 +374,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) {
@@ -384,7 +386,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) {
@@ -567,7 +569,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
@@ -595,10 +597,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");
+            }
         }
     }
 }

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());
         }
-
     }
 
 }

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 <a href="http://www.christianposta.com/blog">Christian Posta</a>
  */
 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());
         }
     }