Packaging: Remove classpath ordering hack (#23596)

After the removal of the joda time hack we used to have, we can cleanup
the codebase handling in security, jarhell and plugins to be more picky
about uniqueness. This was originally in #18959 which was never merged.

closes #18959

Author: rjernst

core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java

@@ -31,6 +31,7 @@
 import java.security.Permissions;
 import java.security.Policy;
 import java.security.ProtectionDomain;
+import java.util.Collections;
 import java.util.Map;
 import java.util.function.Predicate;
 
@@ -50,7 +51,7 @@ final class ESPolicy extends Policy {
 
     ESPolicy(PermissionCollection dynamic, Map<String,Policy> plugins, boolean filterBadDefaults) {
         this.template = Security.readPolicy(getClass().getResource(POLICY_RESOURCE), JarHell.parseClassPath());
-        this.untrusted = Security.readPolicy(getClass().getResource(UNTRUSTED_RESOURCE), new URL[0]);
+        this.untrusted = Security.readPolicy(getClass().getResource(UNTRUSTED_RESOURCE), Collections.emptySet());
         if (filterBadDefaults) {
             this.system = new SystemPolicy(Policy.getPolicy());
         } else {

core/src/main/java/org/elasticsearch/bootstrap/JarHell.java

@@ -36,9 +36,11 @@
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Enumeration;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
@@ -93,7 +95,7 @@ public static void checkJarHell() throws IOException, URISyntaxException {
      * @return array of URLs
      * @throws IllegalStateException if the classpath contains empty elements
      */
-    public static URL[] parseClassPath()  {
+    public static Set<URL> parseClassPath()  {
         return parseClassPath(System.getProperty("java.class.path"));
     }
 
@@ -104,13 +106,12 @@ public static URL[] parseClassPath()  {
      * @throws IllegalStateException if the classpath contains empty elements
      */
     @SuppressForbidden(reason = "resolves against CWD because that is how classpaths work")
-    static URL[] parseClassPath(String classPath) {
+    static Set<URL> parseClassPath(String classPath) {
         String pathSeparator = System.getProperty("path.separator");
         String fileSeparator = System.getProperty("file.separator");
         String elements[] = classPath.split(pathSeparator);
-        URL urlElements[] = new URL[elements.length];
-        for (int i = 0; i < elements.length; i++) {
-            String element = elements[i];
+        Set<URL> urlElements = new LinkedHashSet<>(); // order is already lost, but some filesystems have it
+        for (String element : elements) {
             // Technically empty classpath element behaves like CWD.
             // So below is the "correct" code, however in practice with ES, this is usually just a misconfiguration,
             // from old shell scripts left behind or something:
@@ -136,21 +137,25 @@ static URL[] parseClassPath(String classPath) {
             }
             // now just parse as ordinary file
             try {
-                urlElements[i] = PathUtils.get(element).toUri().toURL();
+                URL url = PathUtils.get(element).toUri().toURL();
+                if (urlElements.add(url) == false) {
+                    throw new IllegalStateException("jar hell!" + System.lineSeparator() +
+                        "duplicate jar on classpath: " + classPath);
+                }
             } catch (MalformedURLException e) {
                 // should not happen, as we use the filesystem API
                 throw new RuntimeException(e);
             }
         }
-        return urlElements;
+        return Collections.unmodifiableSet(urlElements);
     }
 
     /**
      * Checks the set of URLs for duplicate classes
      * @throws IllegalStateException if jar hell was found
      */
     @SuppressForbidden(reason = "needs JarFile for speed, just reading entries")
-    public static void checkJarHell(URL urls[]) throws URISyntaxException, IOException {
+    public static void checkJarHell(Set<URL> urls) throws URISyntaxException, IOException {
         Logger logger = Loggers.getLogger(JarHell.class);
         // we don't try to be sneaky and use deprecated/internal/not portable stuff
         // like sun.boot.class.path, and with jigsaw we don't yet have a way to get
@@ -168,8 +173,8 @@ public static void checkJarHell(URL urls[]) throws URISyntaxException, IOExcepti
             }
             if (path.toString().endsWith(".jar")) {
                 if (!seenJars.add(path)) {
-                    logger.debug("excluding duplicate classpath element: {}", path);
-                    continue;
+                    throw new IllegalStateException("jar hell!" + System.lineSeparator() +
+                                                    "duplicate jar on classpath: " + path);
                 }
                 logger.debug("examining jar: {}", path);
                 try (JarFile file = new JarFile(path.toString())) {
@@ -198,8 +203,8 @@ public static void checkJarHell(URL urls[]) throws URISyntaxException, IOExcepti
                     public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
                         String entry = root.relativize(file).toString();
                         if (entry.endsWith(".class")) {
-                            // normalize with the os separator
-                            entry = entry.replace(sep, ".").substring(0,  entry.length() - 6);
+                            // normalize with the os separator, remove '.class'
+                            entry = entry.replace(sep, ".").substring(0,  entry.length() - ".class".length());
                             checkClass(clazzes, entry, path);
                         }
                         return super.visitFile(file, attrs);

core/src/main/java/org/elasticsearch/bootstrap/Security.java

@@ -48,8 +48,10 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 /**
  * Initializes SecurityManager with necessary permissions.
@@ -127,19 +129,23 @@ static void configure(Environment environment, boolean filterBadDefaults) throws
     @SuppressForbidden(reason = "proper use of URL")
     static Map<String,Policy> getPluginPermissions(Environment environment) throws IOException, NoSuchAlgorithmException {
         Map<String,Policy> map = new HashMap<>();
-        // collect up lists of plugins and modules
-        List<Path> pluginsAndModules = new ArrayList<>();
+        // collect up set of plugins and modules by listing directories.
+        Set<Path> pluginsAndModules = new LinkedHashSet<>(); // order is already lost, but some filesystems have it
         if (Files.exists(environment.pluginsFile())) {
             try (DirectoryStream<Path> stream = Files.newDirectoryStream(environment.pluginsFile())) {
                 for (Path plugin : stream) {
-                    pluginsAndModules.add(plugin);
+                    if (pluginsAndModules.add(plugin) == false) {
+                        throw new IllegalStateException("duplicate plugin: " + plugin);
+                    }
                 }
             }
         }
         if (Files.exists(environment.modulesFile())) {
             try (DirectoryStream<Path> stream = Files.newDirectoryStream(environment.modulesFile())) {
-                for (Path plugin : stream) {
-                    pluginsAndModules.add(plugin);
+                for (Path module : stream) {
+                    if (pluginsAndModules.add(module) == false) {
+                        throw new IllegalStateException("duplicate module: " + module);
+                    }
                 }
             }
         }
@@ -149,15 +155,18 @@ static Map<String,Policy> getPluginPermissions(Environment environment) throws I
             if (Files.exists(policyFile)) {
                 // first get a list of URLs for the plugins' jars:
                 // we resolve symlinks so map is keyed on the normalize codebase name
-                List<URL> codebases = new ArrayList<>();
+                Set<URL> codebases = new LinkedHashSet<>(); // order is already lost, but some filesystems have it
                 try (DirectoryStream<Path> jarStream = Files.newDirectoryStream(plugin, "*.jar")) {
                     for (Path jar : jarStream) {
-                        codebases.add(jar.toRealPath().toUri().toURL());
+                        URL url = jar.toRealPath().toUri().toURL();
+                        if (codebases.add(url) == false) {
+                            throw new IllegalStateException("duplicate module/plugin: " + url);
+                        }
                     }
                 }
 
                 // parse the plugin's policy file into a set of permissions
-                Policy policy = readPolicy(policyFile.toUri().toURL(), codebases.toArray(new URL[codebases.size()]));
+                Policy policy = readPolicy(policyFile.toUri().toURL(), codebases);
 
                 // consult this policy for each of the plugin's jars:
                 for (URL url : codebases) {
@@ -175,24 +184,33 @@ static Map<String,Policy> getPluginPermissions(Environment environment) throws I
     /**
      * Reads and returns the specified {@code policyFile}.
      * <p>
-     * Resources (e.g. jar files and directories) listed in {@code codebases} location
-     * will be provided to the policy file via a system property of the short name:
-     * e.g. <code>${codebase.joda-convert-1.2.jar}</code> would map to full URL.
+     * Jar files listed in {@code codebases} location will be provided to the policy file via
+     * a system property of the short name: e.g. <code>${codebase.joda-convert-1.2.jar}</code>
+     * would map to full URL.
      */
     @SuppressForbidden(reason = "accesses fully qualified URLs to configure security")
-    static Policy readPolicy(URL policyFile, URL codebases[]) {
+    static Policy readPolicy(URL policyFile, Set<URL> codebases) {
         try {
             try {
                 // set codebase properties
                 for (URL url : codebases) {
                     String shortName = PathUtils.get(url.toURI()).getFileName().toString();
-                    System.setProperty("codebase." + shortName, url.toString());
+                    if (shortName.endsWith(".jar") == false) {
+                        continue; // tests :(
+                    }
+                    String previous = System.setProperty("codebase." + shortName, url.toString());
+                    if (previous != null) {
+                        throw new IllegalStateException("codebase property already set: " + shortName + "->" + previous);
+                    }
                 }
                 return Policy.getInstance("JavaPolicy", new URIParameter(policyFile.toURI()));
             } finally {
                 // clear codebase properties
                 for (URL url : codebases) {
                     String shortName = PathUtils.get(url.toURI()).getFileName().toString();
+                    if (shortName.endsWith(".jar") == false) {
+                        continue; // tests :(
+                    }
                     System.clearProperty("codebase." + shortName);
                 }
             }

core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java

@@ -455,22 +455,23 @@ private PluginInfo verify(Terminal terminal, Path pluginRoot, boolean isBatch, E
     /** check a candidate plugin for jar hell before installing it */
     void jarHellCheck(Path candidate, Path pluginsDir) throws Exception {
         // create list of current jars in classpath
-        final List<URL> jars = new ArrayList<>();
-        jars.addAll(Arrays.asList(JarHell.parseClassPath()));
+        final Set<URL> jars = new HashSet<>(JarHell.parseClassPath());
 
         // read existing bundles. this does some checks on the installation too.
         PluginsService.getPluginBundles(pluginsDir);
 
         // add plugin jars to the list
         Path pluginJars[] = FileSystemUtils.files(candidate, "*.jar");
         for (Path jar : pluginJars) {
-            jars.add(jar.toUri().toURL());
+            if (jars.add(jar.toUri().toURL()) == false) {
+                throw new IllegalStateException("jar hell! duplicate plugin jar: " + jar);
+            }
         }
         // TODO: no jars should be an error
         // TODO: verify the classname exists in one of the jars!
 
         // check combined (current classpath + new jars to-be-added)
-        JarHell.checkJarHell(jars.toArray(new URL[jars.size()]));
+        JarHell.checkJarHell(jars);
     }
 
     /**