diff --git a/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java b/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java index e04b772c37c..d911237fe82 100644 --- a/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java +++ b/core/src/main/java/org/apache/accumulo/core/classloader/ClassLoaderUtil.java @@ -47,6 +47,7 @@ public static synchronized void initContextFactory(AccumuloConfiguration conf) { LOG.info("Using default {}, which is subject to change in a future release", ContextClassLoaderFactory.class.getName()); FACTORY = new URLContextClassLoaderFactory(); + FACTORY.init(() -> new ConfigurationImpl(conf)); } else { // load user's selected implementation and provide it with the service environment try { diff --git a/core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java b/core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java index 32681e6e5d0..ffafd75ef60 100644 --- a/core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java +++ b/core/src/main/java/org/apache/accumulo/core/classloader/URLContextClassLoaderFactory.java @@ -18,13 +18,17 @@ */ package org.apache.accumulo.core.classloader; +import static com.google.common.base.Preconditions.checkArgument; + import java.io.UncheckedIOException; import java.net.MalformedURLException; -import java.net.URI; import java.net.URL; import java.net.URLClassLoader; import java.util.Arrays; +import java.util.regex.Pattern; +import org.apache.accumulo.core.conf.Property; +import org.apache.accumulo.core.spi.common.ContextClassLoaderEnvironment; import org.apache.accumulo.core.spi.common.ContextClassLoaderFactory; import org.apache.accumulo.core.util.cache.Caches; import org.apache.accumulo.core.util.cache.Caches.CacheName; @@ -42,6 +46,9 @@ public class URLContextClassLoaderFactory implements ContextClassLoaderFactory { private static final Logger LOG = LoggerFactory.getLogger(URLContextClassLoaderFactory.class); + public static final String URL_PATTERN_PROPERTY = + Property.GENERAL_ARBITRARY_PROP_PREFIX + "factory.class.loader.url.allowed.patterns"; + // Cache the class loaders for re-use // WeakReferences are used so that the class loaders can be cleaned up when no longer needed // Classes that are loaded contain a reference to the class loader used to load them @@ -49,8 +56,25 @@ public class URLContextClassLoaderFactory implements ContextClassLoaderFactory { private final Cache classloaders = Caches.getInstance().createNewBuilder(CacheName.CLASSLOADERS, true).weakValues().build(); + private volatile Pattern urlPattern = null; + @Override - public ClassLoader getClassLoader(String context) { + public void init(ContextClassLoaderEnvironment env) { + String urlPatternProperty = env.getConfiguration().get(URL_PATTERN_PROPERTY); + if (urlPatternProperty == null) { + LOG.warn("Property " + URL_PATTERN_PROPERTY + " not set, no contexts are allowed"); + } else { + urlPattern = Pattern.compile(urlPatternProperty); + } + } + + @Override + public ClassLoader getClassLoader(String context) throws ContextClassLoaderException { + if (urlPattern == null) { + throw new ContextClassLoaderException( + "Property " + URL_PATTERN_PROPERTY + " not set, no contexts are allowed"); + } + if (context == null) { throw new IllegalArgumentException("Unknown context"); } @@ -59,7 +83,11 @@ public ClassLoader getClassLoader(String context) { LOG.debug("Creating URLClassLoader for context, uris: {}", context); return new URLClassLoader(Arrays.stream(context.split(",")).map(p -> { try { - return URI.create(p).toURL(); + URL url = new URL(p); + checkArgument(urlPattern.matcher(url.toExternalForm()).matches(), + "Context %s URL (%s) not allowed by pattern (%s)", context, url.toExternalForm(), + urlPattern.pattern()); + return url; } catch (MalformedURLException e) { throw new UncheckedIOException(e); } diff --git a/core/src/test/java/org/apache/accumulo/core/classloader/ContextClassLoaderFactoryTest.java b/core/src/test/java/org/apache/accumulo/core/classloader/ContextClassLoaderFactoryTest.java index 440d6ca72b4..11c264eda4b 100644 --- a/core/src/test/java/org/apache/accumulo/core/classloader/ContextClassLoaderFactoryTest.java +++ b/core/src/test/java/org/apache/accumulo/core/classloader/ContextClassLoaderFactoryTest.java @@ -19,7 +19,10 @@ package org.apache.accumulo.core.classloader; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import java.net.MalformedURLException; import java.net.URL; import java.net.URLClassLoader; import java.nio.file.Files; @@ -37,7 +40,8 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; -@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "paths not set by user input") +@SuppressFBWarnings(value = {"PATH_TRAVERSAL_IN", "NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"}, + justification = "paths not set by user input") public class ContextClassLoaderFactoryTest extends WithTestNames { @TempDir @@ -45,6 +49,7 @@ public class ContextClassLoaderFactoryTest extends WithTestNames { private URL uri1; private URL uri2; + private String tempFolderPattern; @BeforeEach public void setup() throws Exception { @@ -70,6 +75,41 @@ public void setup() throws Exception { propsFile2.toFile()); uri2 = propsFile2.toUri().toURL(); + tempFolderPattern = tempFolder.toUri().toURL().toExternalForm() + ".*"; + } + + @Test + public void urlContextPatternNotSet() { + ConfigurationCopy cc = new ConfigurationCopy(); + cc.set(Property.GENERAL_CONTEXT_CLASSLOADER_FACTORY.getKey(), + URLContextClassLoaderFactory.class.getName()); + ClassLoaderUtil.resetContextFactoryForTests(); + ClassLoaderUtil.initContextFactory(cc); + ContextClassLoaderException ex = assertThrows(ContextClassLoaderException.class, () -> { + @SuppressWarnings("unused") + URLClassLoader classloader = + (URLClassLoader) ClassLoaderUtil.getContextFactory().getClassLoader(uri1.toString()); + }); + assertEquals( + "Error getting classloader for context: Property general.custom.factory.class.loader.url.allowed.patterns not set, no contexts are allowed", + ex.getMessage()); + } + + @Test + public void urlContextPatternDoesNotMath() throws MalformedURLException { + ConfigurationCopy cc = new ConfigurationCopy(); + cc.set(Property.GENERAL_CONTEXT_CLASSLOADER_FACTORY.getKey(), + URLContextClassLoaderFactory.class.getName()); + cc.set(URLContextClassLoaderFactory.URL_PATTERN_PROPERTY, + new URL("file:///path/to/unknown/folder/.*").toExternalForm()); + ClassLoaderUtil.resetContextFactoryForTests(); + ClassLoaderUtil.initContextFactory(cc); + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> { + @SuppressWarnings("unused") + URLClassLoader classloader = + (URLClassLoader) ClassLoaderUtil.getContextFactory().getClassLoader(uri1.toString()); + }); + assertTrue(ex.getMessage().contains(" not allowed by pattern ")); } @Test @@ -78,6 +118,7 @@ public void differentContexts() throws ContextClassLoaderException { ConfigurationCopy cc = new ConfigurationCopy(); cc.set(Property.GENERAL_CONTEXT_CLASSLOADER_FACTORY.getKey(), URLContextClassLoaderFactory.class.getName()); + cc.set(URLContextClassLoaderFactory.URL_PATTERN_PROPERTY, tempFolderPattern); ClassLoaderUtil.resetContextFactoryForTests(); ClassLoaderUtil.initContextFactory(cc); diff --git a/minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterClasspathTest.java b/minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterClasspathTest.java index 88881dee9a0..d10faf953b9 100644 --- a/minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterClasspathTest.java +++ b/minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterClasspathTest.java @@ -29,6 +29,7 @@ import java.util.Map; import java.util.Map.Entry; +import org.apache.accumulo.core.classloader.URLContextClassLoaderFactory; import org.apache.accumulo.core.client.Accumulo; import org.apache.accumulo.core.client.AccumuloClient; import org.apache.accumulo.core.client.BatchWriter; @@ -76,6 +77,8 @@ public static void setupMiniCluster() throws Exception { config.setZooKeeperPort(0); HashMap site = new HashMap<>(); site.put(Property.TSERV_WAL_MAX_SIZE.getKey(), "1G"); + site.put(URLContextClassLoaderFactory.URL_PATTERN_PROPERTY, + jarFile.toURI().toURL().toExternalForm()); config.setSiteConfig(site); accumulo = new MiniAccumuloCluster(config); accumulo.start(); @@ -95,8 +98,8 @@ public void testPerTableClasspath() throws Exception { final String tableName = testName(); var ntc = new NewTableConfiguration(); - ntc.setProperties( - Map.of(Property.TABLE_CLASSLOADER_CONTEXT.getKey(), jarFile.toURI().toString())); + ntc.setProperties(Map.of(Property.TABLE_CLASSLOADER_CONTEXT.getKey(), + jarFile.toURI().toURL().toExternalForm())); ntc.attachIterator( new IteratorSetting(100, "foocensor", "org.apache.accumulo.test.FooFilter")); diff --git a/test/src/main/java/org/apache/accumulo/test/compaction/ClassLoaderContextCompactionIT.java b/test/src/main/java/org/apache/accumulo/test/compaction/ClassLoaderContextCompactionIT.java index 2162b4eb7af..761b51e5ac6 100644 --- a/test/src/main/java/org/apache/accumulo/test/compaction/ClassLoaderContextCompactionIT.java +++ b/test/src/main/java/org/apache/accumulo/test/compaction/ClassLoaderContextCompactionIT.java @@ -31,6 +31,7 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.net.MalformedURLException; import java.net.URL; import java.util.EnumSet; import java.util.List; @@ -39,6 +40,7 @@ import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; +import org.apache.accumulo.core.classloader.URLContextClassLoaderFactory; import org.apache.accumulo.core.client.Accumulo; import org.apache.accumulo.core.client.AccumuloClient; import org.apache.accumulo.core.client.IteratorSetting; @@ -95,6 +97,13 @@ public static void after() throws Exception { @Override public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration coreSite) { ExternalCompactionTestUtils.configureMiniCluster(cfg, coreSite); + try { + cfg.setProperty(URLContextClassLoaderFactory.URL_PATTERN_PROPERTY, + new URL("file:" + cfg.getDir().toString() + "/accumulo/classpath/.*").toExternalForm()); + } catch (MalformedURLException e) { + throw new RuntimeException(e); + } + // After 1 failure start backing off by 5s. // After 3 failures, terminate the Compactor cfg.setProperty(Property.COMPACTOR_FAILURE_BACKOFF_THRESHOLD, "1"); @@ -220,7 +229,7 @@ public void testClassLoaderContextErrorKillsCompactor() throws Exception { // Set the context on the table client.tableOperations().setProperty(table1, Property.TABLE_CLASSLOADER_CONTEXT.getKey(), - dst.toUri().toString()); + dst.toUri().toURL().toExternalForm()); final IteratorSetting cfg = new IteratorSetting(101, "FooFilter", "org.apache.accumulo.test.FooFilter"); diff --git a/test/src/main/java/org/apache/accumulo/test/functional/ScannerContextIT.java b/test/src/main/java/org/apache/accumulo/test/functional/ScannerContextIT.java index 7c7ea5d8490..2d9cb0aab20 100644 --- a/test/src/main/java/org/apache/accumulo/test/functional/ScannerContextIT.java +++ b/test/src/main/java/org/apache/accumulo/test/functional/ScannerContextIT.java @@ -25,11 +25,14 @@ import static org.junit.jupiter.api.Assumptions.assumeTrue; import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URL; import java.time.Duration; import java.util.Collections; import java.util.Iterator; import java.util.Map.Entry; +import org.apache.accumulo.core.classloader.URLContextClassLoaderFactory; import org.apache.accumulo.core.client.Accumulo; import org.apache.accumulo.core.client.AccumuloClient; import org.apache.accumulo.core.client.BatchScanner; @@ -44,7 +47,9 @@ import org.apache.accumulo.core.data.Value; import org.apache.accumulo.core.security.Authorizations; import org.apache.accumulo.miniclusterImpl.MiniAccumuloClusterImpl; +import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl; import org.apache.accumulo.test.harness.AccumuloClusterHarness; +import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.junit.jupiter.api.BeforeEach; @@ -64,6 +69,16 @@ protected Duration defaultTimeout() { return Duration.ofMinutes(2); } + @Override + public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) { + try { + cfg.setProperty(URLContextClassLoaderFactory.URL_PATTERN_PROPERTY, + new URL(CONTEXT_DIR + "/.*").toExternalForm()); + } catch (MalformedURLException e) { + throw new RuntimeException(e); + } + } + @BeforeEach public void checkCluster() throws Exception { assumeTrue(getClusterType() == ClusterType.MINI); @@ -131,7 +146,7 @@ public void testScanContextOverridesTableContext() throws Exception { // that contains nothing. The ScanContextIT context will point to the test iterators jar String tableContextProperty = Property.TABLE_CLASSLOADER_CONTEXT.getKey(); String tableContextDir = "file://" + System.getProperty("user.dir") + "/target"; - String tableContextClasspath = tableContextDir + "/TestFoo.jar"; + String tableContextClasspath = new URL(tableContextDir + "/TestFoo.jar").toExternalForm(); // Set the ScanContextIT context on the namespace c.namespaceOperations().setProperty(Namespace.DEFAULT.name(), tableContextProperty, CONTEXT); @@ -194,7 +209,7 @@ public void testOneScannerDoesntInterfereWithAnother() throws Exception { IteratorSetting cfg = new IteratorSetting(21, "reverse", "org.apache.accumulo.test.functional.ValueReversingIterator"); one.addScanIterator(cfg); - one.setClassLoaderContext(CONTEXT); + one.setClassLoaderContext(new URL(CONTEXT).toExternalForm()); Iterator> iterator = one.iterator(); for (int i = 0; i < ITERATIONS; i++) { @@ -236,7 +251,7 @@ public void testClearContext() throws Exception { IteratorSetting cfg = new IteratorSetting(21, "reverse", "org.apache.accumulo.test.functional.ValueReversingIterator"); one.addScanIterator(cfg); - one.setClassLoaderContext(CONTEXT); + one.setClassLoaderContext(new URL(CONTEXT).toExternalForm()); Iterator> iterator = one.iterator(); for (int i = 0; i < ITERATIONS; i++) { @@ -264,7 +279,7 @@ private void scanCheck(AccumuloClient c, String tableName, IteratorSetting cfg, String expected) throws Exception { try (Scanner bs = c.createScanner(tableName, Authorizations.EMPTY)) { if (context != null) { - bs.setClassLoaderContext(context); + bs.setClassLoaderContext(new URL(context).toExternalForm()); } if (cfg != null) { bs.addScanIterator(cfg); @@ -284,7 +299,7 @@ private void batchCheck(AccumuloClient c, String tableName, IteratorSetting cfg, try (BatchScanner bs = c.createBatchScanner(tableName)) { bs.setRanges(Collections.singleton(new Range())); if (context != null) { - bs.setClassLoaderContext(context); + bs.setClassLoaderContext(new URL(context).toExternalForm()); } if (cfg != null) { bs.addScanIterator(cfg); diff --git a/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java b/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java index 4283c8d837d..ea0fec9a9c4 100644 --- a/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java +++ b/test/src/main/java/org/apache/accumulo/test/shell/ShellServerIT.java @@ -35,6 +35,8 @@ import java.io.File; import java.io.IOException; import java.io.PrintWriter; +import java.net.MalformedURLException; +import java.net.URL; import java.nio.file.Files; import java.time.Duration; import java.util.ArrayList; @@ -51,6 +53,7 @@ import java.util.regex.Pattern; import org.apache.accumulo.core.Constants; +import org.apache.accumulo.core.classloader.URLContextClassLoaderFactory; import org.apache.accumulo.core.client.Accumulo; import org.apache.accumulo.core.client.AccumuloClient; import org.apache.accumulo.core.client.AccumuloException; @@ -129,6 +132,13 @@ public class ShellServerIT extends SharedMiniClusterBase { private static class ShellServerITConfigCallback implements MiniClusterConfigurationCallback { @Override public void configureMiniCluster(MiniAccumuloConfigImpl cfg, Configuration coreSite) { + try { + cfg.setProperty(URLContextClassLoaderFactory.URL_PATTERN_PROPERTY, + new URL("file://" + System.getProperty("user.dir") + "/target/.*").toExternalForm()); + } catch (MalformedURLException e) { + throw new RuntimeException(e); + } + // Only one tserver to avoid race conditions on ZK propagation (auths and configuration) cfg.getClusterServerConfiguration().setNumDefaultTabletServers(1); // Set the min span to 0 so we will definitely get all the traces back. See ACCUMULO-4365 @@ -1756,7 +1766,8 @@ public void verifyPerTableClasspath(final String table, final File fooConstraint File fooFilterJar = initJar("/org/apache/accumulo/test/FooFilter.jar", "FooFilter", rootPath); - String context = fooFilterJar.toURI() + "," + fooConstraintJar.toURI(); + String context = fooFilterJar.toURI().toURL().toExternalForm() + "," + + fooConstraintJar.toURI().toURL().toExternalForm(); ts.exec("createtable " + table, true); ts.exec( @@ -1959,10 +1970,11 @@ public void scansWithClassLoaderContext() throws IOException { make10(); setupFakeContextPath(); + String fakeCtx = new URL(FAKE_CONTEXT).toExternalForm(); result = ts.exec("config -t " + tableName + " -s " + Property.TABLE_CLASSLOADER_CONTEXT.getKey() - + "=" + FAKE_CONTEXT); + + "=" + fakeCtx); assertEquals("root@miniInstance " + tableName + "> config -t " + tableName + " -s " - + Property.TABLE_CLASSLOADER_CONTEXT.getKey() + "=" + FAKE_CONTEXT + "\n", result); + + Property.TABLE_CLASSLOADER_CONTEXT.getKey() + "=" + fakeCtx + "\n", result); result = ts.exec("setshelliter -pn baz -n reverse -p 21 -class " + VALUE_REVERSING_ITERATOR); assertTrue(result.contains("The iterator class does not implement OptionDescriber")); @@ -1991,25 +2003,26 @@ public void scansWithClassLoaderContext() throws IOException { // Override the table classloader context with the REAL implementation of // ValueReversingIterator, which does reverse the value. - result = ts.exec("scan -pn baz -b row1 -e row1 -cc " + REAL_CONTEXT); + String realCtx = new URL(REAL_CONTEXT).toExternalForm(); + result = ts.exec("scan -pn baz -b row1 -e row1 -cc " + realCtx); assertEquals(2, result.split("\n").length); assertTrue(result.contains("eulav")); assertFalse(result.contains("value")); - result = ts.exec("scan -pn baz -b row3 -e row5 -cc " + REAL_CONTEXT); + result = ts.exec("scan -pn baz -b row3 -e row5 -cc " + realCtx); assertEquals(4, result.split("\n").length); assertTrue(result.contains("eulav")); assertFalse(result.contains("value")); - result = ts.exec("scan -pn baz -r row3 -cc " + REAL_CONTEXT); + result = ts.exec("scan -pn baz -r row3 -cc " + realCtx); assertEquals(2, result.split("\n").length); assertTrue(result.contains("eulav")); assertFalse(result.contains("value")); - result = ts.exec("scan -pn baz -b row: -cc " + REAL_CONTEXT); + result = ts.exec("scan -pn baz -b row: -cc " + realCtx); assertEquals(1, result.split("\n").length); - result = ts.exec("scan -pn baz -b row -cc " + REAL_CONTEXT); + result = ts.exec("scan -pn baz -b row -cc " + realCtx); assertEquals(11, result.split("\n").length); assertTrue(result.contains("eulav")); assertFalse(result.contains("value")); - result = ts.exec("scan -pn baz -e row: -cc " + REAL_CONTEXT); + result = ts.exec("scan -pn baz -e row: -cc " + realCtx); assertEquals(11, result.split("\n").length); assertTrue(result.contains("eulav")); assertFalse(result.contains("value"));