From 3504fefb6b343c79a98e13a5f78c6747d10b9ae8 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Thu, 17 Sep 2015 23:51:51 -0400 Subject: [PATCH 1/3] Add Intellij support for plugins with extra permissions. graduate this from a hack for insecure plugins to something we can live with for per-module/plugin permissions, it now works reasonably in unit tests and with Intellij and Eclipse IDEs. remove security warnings: we will deal with these issues in a secure way, if we cannot, then the plugin shouldn't be in our core codebase. --- .../bootstrap/BootstrapInfo.java | 2 +- .../org/elasticsearch/bootstrap/Security.java | 46 +++++++++++++------ .../elasticsearch/bootstrap/security.policy | 14 +++--- .../bootstrap/BootstrapForTesting.java | 31 +++++++------ pom.xml | 3 +- 5 files changed, 60 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java index abaa784d0b9..fc7e28534c5 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java @@ -54,6 +54,6 @@ public final class BootstrapInfo { * that require additional privileges as a workaround. */ public static Set getInsecurePluginList() { - return Collections.unmodifiableSet(Security.INSECURE_PLUGINS.keySet()); + return Collections.unmodifiableSet(Security.SPECIAL_PLUGINS.keySet()); } } diff --git a/core/src/main/java/org/elasticsearch/bootstrap/Security.java b/core/src/main/java/org/elasticsearch/bootstrap/Security.java index a8ed511be13..df2f6481739 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -20,8 +20,6 @@ package org.elasticsearch.bootstrap; import org.elasticsearch.common.SuppressForbidden; -import org.elasticsearch.common.logging.ESLogger; -import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.env.Environment; import java.io.*; @@ -159,15 +157,39 @@ final class Security { } } - // mapping of insecure plugins to codebase properties + // mapping of plugins to plugin class name. see getPluginClass why we need this. + // plugin codebase property is always implicit (es.security.plugin.foobar) // note that this is only read once, when policy is parsed. - static final Map INSECURE_PLUGINS; + static final Map SPECIAL_PLUGINS; static { Map m = new HashMap<>(); - m.put("repository-s3", "es.security.insecure.plugin.repository-s3"); - m.put("discovery-ec2", "es.security.insecure.plugin.discovery-ec2"); - m.put("cloud-gce", "es.security.insecure.plugin.cloud-gce" ); - INSECURE_PLUGINS = Collections.unmodifiableMap(m); + m.put("repository-s3", "org.elasticsearch.plugin.repository.s3.S3RepositoryPlugin"); + m.put("discovery-ec2", "org.elasticsearch.plugin.discovery.ec2.Ec2DiscoveryPlugin"); + m.put("cloud-gce", "org.elasticsearch.plugin.cloud.gce.CloudGcePlugin"); + SPECIAL_PLUGINS = Collections.unmodifiableMap(m); + } + + /** + * Returns policy property for plugin, if it has special permissions. + * otherwise returns null. + */ + static String getPluginProperty(String pluginName) { + if (SPECIAL_PLUGINS.containsKey(pluginName)) { + return "es.security.plugin." + pluginName; + } else { + return null; + } + } + + /** + * Returns plugin class name, if it has special permissions. + * otherwise returns null. + */ + // this is only here to support the intellij IDE + // it sucks to duplicate information, but its worth the tradeoff: sanity + // if it gets out of sync, tests will fail. + static String getPluginClass(String pluginName) { + return SPECIAL_PLUGINS.get(pluginName); } /** @@ -179,20 +201,18 @@ final class Security { if (Files.exists(environment.pluginsFile())) { try (DirectoryStream stream = Files.newDirectoryStream(environment.pluginsFile())) { for (Path plugin : stream) { - String prop = INSECURE_PLUGINS.get(plugin.getFileName().toString()); + String prop = getPluginProperty(plugin.getFileName().toString()); if (prop != null) { if (System.getProperty(prop) != null) { throw new IllegalStateException("property: " + prop + " is unexpectedly set: " + System.getProperty(prop)); } System.setProperty(prop, plugin.toUri().toURL().toString() + "*"); - ESLogger logger = Loggers.getLogger(Security.class); - logger.warn("Adding permissions for insecure plugin [{}]", plugin.getFileName()); - logger.warn("There are unresolved issues with third-party code that may reduce the security of the system"); } } } } - for (String prop : INSECURE_PLUGINS.values()) { + for (String plugin : SPECIAL_PLUGINS.keySet()) { + String prop = getPluginProperty(plugin); if (System.getProperty(prop) == null) { System.setProperty(prop, "file:/dev/null"); // no chance to be interpreted as "all" } diff --git a/core/src/main/resources/org/elasticsearch/bootstrap/security.policy b/core/src/main/resources/org/elasticsearch/bootstrap/security.policy index 72ccc19acc3..c21e58d1d53 100644 --- a/core/src/main/resources/org/elasticsearch/bootstrap/security.policy +++ b/core/src/main/resources/org/elasticsearch/bootstrap/security.policy @@ -37,22 +37,22 @@ grant codeBase "${es.security.jar.lucene.core}" { permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; }; -//// Insecure plugin permissions: -//// These are dangerous permissions due to problems in third-party library code -//// We try our best to contain and protect the issues, but ultimately warn the user -//// when installing these plugins that it can introduce a security risk +//// Special plugin permissions: +//// These are dangerous permissions only needed by special plugins that we don't +//// want to grant in general. Some may be due to problems in third-party library code, +//// others may just be more obscure integrations. -grant codeBase "${es.security.insecure.plugin.repository-s3}" { +grant codeBase "${es.security.plugin.repository-s3}" { // needed because of problems in aws-sdk permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; }; -grant codeBase "${es.security.insecure.plugin.discovery-ec2}" { +grant codeBase "${es.security.plugin.discovery-ec2}" { // needed because of problems in aws-sdk permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; }; -grant codeBase "${es.security.insecure.plugin.cloud-gce}" { +grant codeBase "${es.security.plugin.cloud-gce}" { // needed because of problems in cloud-gce permission java.lang.reflect.ReflectPermission "suppressAccessChecks"; }; diff --git a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index 8e155fa6915..5b2d9de9af6 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -32,6 +32,7 @@ import java.net.URL; import java.nio.file.Path; import java.security.Permissions; import java.security.Policy; +import java.util.Map; import java.util.Objects; import static com.carrotsearch.randomizedtesting.RandomizedTest.systemPropertyAsBoolean; @@ -115,16 +116,24 @@ public class BootstrapForTesting { } final Policy policy; - // if its an insecure plugin, we use a wrapper policy impl to try + // if its a plugin with special permissions, we use a wrapper policy impl to try // to simulate what happens with a real distribution String artifact = System.getProperty("tests.artifact"); // in case we are running from the IDE: - if (artifact == null || System.getProperty("tests.maven") == null) { - artifact = PathUtils.get(System.getProperty("user.dir")).toAbsolutePath().getFileName().toString(); + if (artifact == null && System.getProperty("tests.maven") == null) { + // look for plugin classname as a resource to determine what project we are. + // while gross, this will work with any IDE. + for (Map.Entry kv : Security.SPECIAL_PLUGINS.entrySet()) { + String resource = kv.getValue().replace('.', '/') + ".class"; + if (BootstrapForTesting.class.getClassLoader().getResource(resource) != null) { + artifact = kv.getKey(); + break; + } + } } - String insecurePluginProp = Security.INSECURE_PLUGINS.get(artifact); - if (insecurePluginProp != null) { - policy = new MockPluginPolicy(perms, insecurePluginProp); + String pluginProp = Security.getPluginProperty(artifact); + if (pluginProp != null) { + policy = new MockPluginPolicy(perms, pluginProp); } else { policy = new ESPolicy(perms); } @@ -132,14 +141,10 @@ public class BootstrapForTesting { System.setSecurityManager(new TestSecurityManager()); Security.selfTest(); - if (insecurePluginProp != null) { + if (pluginProp != null) { // initialize the plugin class, in case it has one-time hacks (unit tests often won't do this) - String clazz = System.getProperty("tests.plugin.classname"); - if (clazz != null) { - Class.forName(clazz); - } else if (System.getProperty("tests.maven") != null) { - throw new IllegalStateException("plugin classname is needed for insecure plugin unit tests: something wrong with build"); - } + String clazz = Security.getPluginClass(artifact); + Class.forName(clazz); } } catch (Exception e) { throw new RuntimeException("unable to install test security manager", e); diff --git a/pom.xml b/pom.xml index 5a24723776f..6c23819ff4d 100644 --- a/pom.xml +++ b/pom.xml @@ -664,9 +664,8 @@ ${tests.cluster} ${tests.iters} ${project.groupId}:${project.artifactId} - + ${project.artifactId} - ${elasticsearch.plugin.classname} ${tests.maxfailures} ${tests.failfast} ${tests.class} From a5127bb6b059eadc857599e19c4afc4ac6752c59 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Fri, 18 Sep 2015 00:16:01 -0400 Subject: [PATCH 2/3] Fix intellij test logging --- .../org/elasticsearch/bootstrap/BootstrapForTesting.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index 5b2d9de9af6..1d7aabe7d34 100644 --- a/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java +++ b/core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -114,6 +114,11 @@ public class BootstrapForTesting { // in case we get fancy and use the -integration goals later: perms.add(new FilePermission(coverageDir.resolve("jacoco-it.exec").toString(), "read,write")); } + // intellij hack: intellij test runner wants setIO and will + // screw up all test logging without it! + if (System.getProperty("tests.maven") == null) { + perms.add(new RuntimePermission("setIO")); + } final Policy policy; // if its a plugin with special permissions, we use a wrapper policy impl to try From 47025defa689cf4af9f79f39f2cf45ba18b7e10b Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Fri, 18 Sep 2015 00:23:06 -0400 Subject: [PATCH 3/3] Remove this: we are gonna manage these right --- .../org/elasticsearch/bootstrap/BootstrapInfo.java | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java index fc7e28534c5..829c45f074e 100644 --- a/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java +++ b/core/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java @@ -19,9 +19,6 @@ package org.elasticsearch.bootstrap; -import java.util.Collections; -import java.util.Set; - /** * Exposes system startup information */ @@ -46,14 +43,4 @@ public final class BootstrapInfo { public static boolean isMemoryLocked() { return Natives.isMemoryLocked(); } - - /** - * Returns set of insecure plugins. - *

- * These are plugins with unresolved issues in third-party libraries, - * that require additional privileges as a workaround. - */ - public static Set getInsecurePluginList() { - return Collections.unmodifiableSet(Security.SPECIAL_PLUGINS.keySet()); - } }