Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Packaging: Remove classpath ordering hack #23596

Merged
merged 2 commits into from
Mar 21, 2017
Merged

Packaging: Remove classpath ordering hack #23596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3b7546b
Packaging: Remove classpath ordering hack
rjernst Mar 15, 2017
f0d6df7
address comments
rjernst Mar 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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 {
Expand Down
29 changes: 17 additions & 12 deletions core/src/main/java/org/elasticsearch/bootstrap/JarHell.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"));
}

Expand All @@ -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:
Expand All @@ -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
Expand All @@ -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())) {
Expand Down Expand Up @@ -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);
Expand Down
44 changes: 31 additions & 13 deletions core/src/main/java/org/elasticsearch/bootstrap/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
}
}
Expand All @@ -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) {
Expand All @@ -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);
}
}
Expand Down
9 changes: 5 additions & 4 deletions core/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/**
Expand Down
Loading