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 e4151eaecab..4972f263261 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 @@ -16,10 +16,11 @@ */ package org.apache.activemq.broker.jmx; +import static org.apache.activemq.util.TransportValidationUtils.validateAllowedUrl; + import java.io.File; import java.io.IOException; import java.net.URI; -import java.net.URISyntaxException; import java.util.*; import java.util.concurrent.atomic.AtomicInteger; @@ -36,7 +37,6 @@ import org.apache.activemq.command.*; import org.apache.activemq.network.NetworkConnector; import org.apache.activemq.util.BrokerSupport; -import org.apache.activemq.util.URISupport; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -44,10 +44,6 @@ public class BrokerView implements BrokerViewMBean { private static final Logger LOG = LoggerFactory.getLogger(BrokerView.class); - public static final Set DENIED_TRANSPORT_SCHEMES = Set.of("vm", "http", "https", - "multicast", "zeroconf", "discovery", "fanout", "mock", "peer", "failover", - "proxy", "reliable", "simple", "udp", "masterslave"); - ManagedRegionBroker broker; private final BrokerService brokerService; @@ -567,58 +563,4 @@ public long getTotalMaxUncommittedExceededCount() { return safeGetBroker().getDestinationStatistics().getMaxUncommittedExceededCount().getCount(); } - private static void validateAllowedUrl(String uriString) throws URISyntaxException { - validateAllowedUri(new URI(uriString), 0); - } - - // 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 (depth > 5) { - throw new IllegalArgumentException("URI can't contain more than 5 nested composite URIs"); - } - - // First check the main URI scheme - validateAllowedScheme(uri.getScheme()); - - // We need to check if the URI is composite and/or contains nested URIs - // The utility method URISupport#isCompositeURI is not good enough here - // because it misses if there are no parentheses and also is primarily meant - // for checking comma separated URIs and not nested URIs. - // - // The best way to handle all cases is to use the same logic that the transports - // use to process the URIs and that is to simply attempt to parse it and check each - // of the parsed components. This wll correctly handle the case when there - // are parentheses and also when the parentheses are skipped. - final URISupport.CompositeData data; - try { - data = URISupport.parseComposite(uri); - } catch (URISyntaxException e) { - // If this is not a valid URI then we can stop checking - // This can happen when parsing a nested URI and at the last portion - return; - } - - if (data.getComponents() != null) { - depth++; - for (URI component : data.getComponents()) { - // Each URI could be a nested and/or composite URI so call validateAllowedUri() - // to validate it. If the scheme is null then the original URI is not composite - // or nested so we can skip the check, and we are finished. - if (component.getScheme() != null) { - validateAllowedUri(component, depth); - } - } - } - } - - // Check all denied schemes - private static void validateAllowedScheme(String scheme) { - 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-broker/src/main/java/org/apache/activemq/network/LdapNetworkConnector.java b/activemq-broker/src/main/java/org/apache/activemq/network/LdapNetworkConnector.java index 5efd82b750f..293091f2936 100644 --- a/activemq-broker/src/main/java/org/apache/activemq/network/LdapNetworkConnector.java +++ b/activemq-broker/src/main/java/org/apache/activemq/network/LdapNetworkConnector.java @@ -35,6 +35,7 @@ import javax.naming.event.NamingExceptionEvent; import javax.naming.event.ObjectChangeListener; +import org.apache.activemq.util.TransportValidationUtils; import org.apache.activemq.util.URISupport; import org.apache.activemq.util.URISupport.CompositeData; import org.slf4j.Logger; @@ -307,6 +308,10 @@ protected synchronized void addConnector(SearchResult result) throws Exception { // want to map/manage these differently in the future // boolean useJMX = getBrokerService().isUseJmx(); // getBrokerService().setUseJmx(false); + + // Validate LDAP provided URI + TransportValidationUtils.validateAllowedUri(connectorURI); + NetworkConnector connector = getBrokerService().addNetworkConnector(connectorURI); // getBrokerService().setUseJmx(useJMX); diff --git a/activemq-broker/src/main/java/org/apache/activemq/util/TransportValidationUtils.java b/activemq-broker/src/main/java/org/apache/activemq/util/TransportValidationUtils.java new file mode 100644 index 00000000000..b0fed2e7cbf --- /dev/null +++ b/activemq-broker/src/main/java/org/apache/activemq/util/TransportValidationUtils.java @@ -0,0 +1,99 @@ +/* + * 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 java.net.URI; +import java.net.URISyntaxException; +import java.util.Set; + +public class TransportValidationUtils { + + public static final Set DENIED_TRANSPORT_SCHEMES = Set.of("vm", "http", "https", + "multicast", "zeroconf", "discovery", "fanout", "mock", "peer", "failover", + "proxy", "reliable", "simple", "udp", "masterslave"); + + /** + * Used to validate externally provided url is not using a denied scheme + * + * @param uriString + * @throws URISyntaxException + */ + public static void validateAllowedUrl(String uriString) throws URISyntaxException { + validateAllowedUri(new URI(uriString)); + } + + /** + * Used to validate externally provided URI is not using a denied scheme + * + * @param uri + * @throws URISyntaxException + */ + public static void validateAllowedUri(URI uri) throws URISyntaxException { + validateAllowedUri(uri, 0); + } + + // 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 (depth > 5) { + throw new IllegalArgumentException("URI can't contain more than 5 nested composite URIs"); + } + + // First check the main URI scheme + validateAllowedScheme(uri.getScheme()); + + // We need to check if the URI is composite and/or contains nested URIs + // The utility method URISupport#isCompositeURI is not good enough here + // because it misses if there are no parentheses and also is primarily meant + // for checking comma separated URIs and not nested URIs. + // + // The best way to handle all cases is to use the same logic that the transports + // use to process the URIs and that is to simply attempt to parse it and check each + // of the parsed components. This wll correctly handle the case when there + // are parentheses and also when the parentheses are skipped. + final URISupport.CompositeData data; + try { + data = URISupport.parseComposite(uri); + } catch (URISyntaxException e) { + // If this is not a valid URI then we can stop checking + // This can happen when parsing a nested URI and at the last portion + return; + } + + if (data.getComponents() != null) { + depth++; + for (URI component : data.getComponents()) { + // Each URI could be a nested and/or composite URI so call validateAllowedUri() + // to validate it. If the scheme is null then the original URI is not composite + // or nested so we can skip the check, and we are finished. + if (component.getScheme() != null) { + validateAllowedUri(component, depth); + } + } + } + } + + // Check all denied schemes + private static void validateAllowedScheme(String scheme) { + 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-broker/src/test/java/org/apache/activemq/network/LdapNetworkConnectorTest.java b/activemq-broker/src/test/java/org/apache/activemq/network/LdapNetworkConnectorTest.java new file mode 100644 index 00000000000..660860d4934 --- /dev/null +++ b/activemq-broker/src/test/java/org/apache/activemq/network/LdapNetworkConnectorTest.java @@ -0,0 +1,72 @@ +/* + * 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.network; + + +import static org.apache.activemq.util.TransportValidationUtils.DENIED_TRANSPORT_SCHEMES; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.fail; + +import javax.naming.directory.BasicAttributes; +import javax.naming.directory.SearchResult; +import org.apache.activemq.broker.BrokerService; +import org.junit.Test; + +public class LdapNetworkConnectorTest { + + @Test + public void testValidation() throws Exception { + LdapNetworkConnector connector = new LdapNetworkConnector(); + connector.setBrokerService(new BrokerService()); + + for (String deniedScheme : DENIED_TRANSPORT_SCHEMES) { + try { + connector.addConnector(newSearchResult(deniedScheme, "localhost")); + fail("Should have failed trying to add connector with scheme: " + deniedScheme); + } catch (IllegalArgumentException e) { + assertEquals("Transport scheme '" + deniedScheme + "' is not allowed", e.getMessage()); + } + } + } + + @Test + public void testCompositeValidation() throws Exception { + LdapNetworkConnector connector = new LdapNetworkConnector(); + connector.setBrokerService(new BrokerService()); + + for (String deniedScheme : DENIED_TRANSPORT_SCHEMES) { + try { + connector.addConnector(newSearchResult("tcp", "static:(tcp://localhost," + + deniedScheme + "://localhost)")); + fail("Should have failed trying to add connector with scheme: " + deniedScheme); + } catch (IllegalArgumentException e) { + assertEquals("Transport scheme '" + deniedScheme + "' is not allowed", e.getMessage()); + } + } + } + + private SearchResult newSearchResult(String protocol, String host) { + BasicAttributes attrs = new BasicAttributes(); + attrs.put("ipserviceprotocol", protocol); + attrs.put("iphostnumber", host); + attrs.put("ipserviceport", "0"); + SearchResult result = new SearchResult("name", null, attrs); + result.setNameInNamespace("fullname"); + return result; + } + +} 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 b5f31eede6b..0cb9a556837 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 @@ -16,7 +16,8 @@ */ package org.apache.activemq.broker.jmx; -import static org.apache.activemq.broker.jmx.BrokerView.DENIED_TRANSPORT_SCHEMES; + +import static org.apache.activemq.util.TransportValidationUtils.DENIED_TRANSPORT_SCHEMES; import java.io.BufferedReader; import java.io.InputStreamReader; 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 b20518e884c..e3ef738c98a 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 @@ -27,7 +27,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static org.apache.activemq.broker.jmx.BrokerView.DENIED_TRANSPORT_SCHEMES; +import static org.apache.activemq.util.TransportValidationUtils.DENIED_TRANSPORT_SCHEMES; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.fail;