Decentralize plugin permissions (modulo docs and pluginmanager work)

Author: rmuir

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

@@ -29,6 +29,7 @@
 import java.security.Policy;
 import java.security.ProtectionDomain;
 import java.security.URIParameter;
+import java.util.Map;
 
 /** custom policy for union of static and dynamic permissions */
 final class ESPolicy extends Policy {
@@ -41,13 +42,15 @@ final class ESPolicy extends Policy {
     final Policy template;
     final Policy untrusted;
     final PermissionCollection dynamic;
+    final Map<String,PermissionCollection> plugins;
 
-    public ESPolicy(PermissionCollection dynamic) throws Exception {
+    public ESPolicy(PermissionCollection dynamic, Map<String,PermissionCollection> plugins) throws Exception {
         URI policyUri = getClass().getResource(POLICY_RESOURCE).toURI();
         URI untrustedUri = getClass().getResource(UNTRUSTED_RESOURCE).toURI();
         this.template = Policy.getInstance("JavaPolicy", new URIParameter(policyUri));
         this.untrusted = Policy.getInstance("JavaPolicy", new URIParameter(untrustedUri));
         this.dynamic = dynamic;
+        this.plugins = plugins;
     }
 
     @Override @SuppressForbidden(reason = "fast equals check is desired")
@@ -66,6 +69,11 @@ public boolean implies(ProtectionDomain domain, Permission permission) {
             if (BootstrapInfo.UNTRUSTED_CODEBASE.equals(location.getFile())) {
                 return untrusted.implies(domain, permission);
             }
+            // check for an additional plugin permission
+            PermissionCollection plugin = plugins.get(location.getFile());
+            if (plugin != null && plugin.implies(permission)) {
+                return true;
+            }
         }
 
         // Special handling for broken AWS code which destroys all SSL security

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

@@ -23,15 +23,19 @@
 import org.elasticsearch.env.Environment;
 
 import java.io.*;
+import java.net.URI;
 import java.net.URL;
 import java.nio.file.AccessMode;
 import java.nio.file.DirectoryStream;
 import java.nio.file.FileAlreadyExistsException;
 import java.nio.file.Files;
 import java.nio.file.NotDirectoryException;
 import java.nio.file.Path;
+import java.security.NoSuchAlgorithmException;
+import java.security.PermissionCollection;
 import java.security.Permissions;
 import java.security.Policy;
+import java.security.URIParameter;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.IdentityHashMap;
@@ -65,7 +69,7 @@
  * when they are so dangerous that general code should not be granted the
  * permission, but there are extenuating circumstances.
  * <p>
- * Groovy scripts are assigned no permissions. This does not provide adequate
+ * Scripts (groovy, javascript, python) are assigned minimal permissions. This does not provide adequate
  * sandboxing, as these scripts still have access to ES classes, and could
  * modify members, etc that would cause bad things to happen later on their
  * behalf (no package protections are yet in place, this would need some
@@ -81,7 +85,7 @@
  * <h1>Debugging Security</h1>
  * A good place to start when there is a problem is to turn on security debugging:
  * <pre>
- * JAVA_OPTS="-Djava.security.debug=access:failure" bin/elasticsearch
+ * JAVA_OPTS="-Djava.security.debug=access,failure" bin/elasticsearch
  * </pre>
  * See <a href="https://docs.oracle.com/javase/7/docs/technotes/guides/security/troubleshooting-security.html">
  * Troubleshooting Security</a> for information.
@@ -97,11 +101,9 @@ private Security() {}
     static void configure(Environment environment) throws Exception {
         // set properties for jar locations
         setCodebaseProperties();
-        // set properties for problematic plugins
-        setPluginCodebaseProperties(environment);
 
-        // enable security policy: union of template and environment-based paths.
-        Policy.setPolicy(new ESPolicy(createPermissions(environment)));
+        // enable security policy: union of template and environment-based paths, and possibly plugin permissions
+        Policy.setPolicy(new ESPolicy(createPermissions(environment), getPluginPermissions(environment)));
 
         // enable security manager
         System.setSecurityManager(new SecurityManager() {
@@ -157,70 +159,39 @@ static void setCodebaseProperties() {
         }
     }
 
-    // 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<String,String> SPECIAL_PLUGINS;
-    static {
-        Map<String,String> m = new HashMap<>();
-        m.put("repository-s3",       "org.elasticsearch.plugin.repository.s3.S3RepositoryPlugin");
-        m.put("discovery-ec2",       "org.elasticsearch.plugin.discovery.ec2.Ec2DiscoveryPlugin");
-        m.put("discovery-gce",       "org.elasticsearch.plugin.discovery.gce.GceDiscoveryPlugin");
-        m.put("lang-expression",     "org.elasticsearch.script.expression.ExpressionPlugin");
-        m.put("lang-groovy",         "org.elasticsearch.script.groovy.GroovyPlugin");
-        m.put("lang-javascript",     "org.elasticsearch.plugin.javascript.JavaScriptPlugin");
-        m.put("lang-python",         "org.elasticsearch.plugin.python.PythonPlugin");
-        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);
-    }
-
     /**
      * Sets properties (codebase URLs) for policy files.
      * we look for matching plugins and set URLs to fit
      */
     @SuppressForbidden(reason = "proper use of URL")
-    static void setPluginCodebaseProperties(Environment environment) throws IOException {
+    static Map<String,PermissionCollection> getPluginPermissions(Environment environment) throws IOException, NoSuchAlgorithmException {
+        Map<String,PermissionCollection> map = new HashMap<>();
         if (Files.exists(environment.pluginsFile())) {
             try (DirectoryStream<Path> stream = Files.newDirectoryStream(environment.pluginsFile())) {
                 for (Path plugin : stream) {
-                    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));
+                    Path policyFile = plugin.resolve("plugin-security.policy");
+                    if (Files.exists(policyFile)) {
+                        // parse the plugin's policy file into a set of permissions
+                        Policy policy = Policy.getInstance("JavaPolicy", new URIParameter(policyFile.toUri()));
+                        PermissionCollection permissions = policy.getPermissions(Security.class.getProtectionDomain());
+                        // this method is supported with the specific implementation we use, but just check for safety.
+                        if (permissions == Policy.UNSUPPORTED_EMPTY_COLLECTION) {
+                            throw new UnsupportedOperationException("JavaPolicy implementation does not support retrieving permissions");
+                        }
+                        // grant the permissions to each jar in the plugin
+                        try (DirectoryStream<Path> jarStream = Files.newDirectoryStream(plugin, "*.jar")) {
+                            for (Path jar : jarStream) {
+                                if (map.put(jar.toUri().toURL().getFile(), permissions) != null) {
+                                    // just be paranoid ok?
+                                    throw new IllegalStateException("per-plugin permissions already granted for jar file: " + jar);
+                                }
+                            }
                         }
-                        System.setProperty(prop, plugin.toUri().toURL().toString() + "*");
                     }
                 }
             }
         }
-        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"
-            }
-        }
+        return map;
     }
 
     /** returns dynamic Permissions to configured paths */

core/src/main/resources/org/elasticsearch/bootstrap/security.policy

@@ -37,52 +37,6 @@ grant codeBase "${es.security.jar.lucene.core}" {
   permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
 };
 
-//// 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.plugin.repository-s3}" {
-  // needed because of problems in aws-sdk
-  permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
-};
-
-grant codeBase "${es.security.plugin.discovery-ec2}" {
-  // needed because of problems in aws-sdk
-  permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
-};
-
-grant codeBase "${es.security.plugin.discovery-gce}" {
-  // needed because of problems in discovery-gce
-  permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
-};
-
-grant codeBase "${es.security.plugin.lang-expression}" {
-  // needed to generate runtime classes
-  permission java.lang.RuntimePermission "createClassLoader";
-};
-
-grant codeBase "${es.security.plugin.lang-groovy}" {
-  // needed to generate runtime classes
-  permission java.lang.RuntimePermission "createClassLoader";
-  // needed by groovy engine
-  permission java.lang.RuntimePermission "accessClassInPackage.sun.reflect";
-  // needed by GroovyScriptEngineService to close its classloader (why?)
-  permission java.lang.RuntimePermission "closeClassLoader";
-  // Allow executing groovy scripts with codesource of /untrusted
-  permission groovy.security.GroovyCodeSourcePermission "/untrusted";
-};
-
-grant codeBase "${es.security.plugin.lang-javascript}" {
-  // needed to generate runtime classes
-  permission java.lang.RuntimePermission "createClassLoader";
-};
-
-grant codeBase "${es.security.plugin.lang-python}" {
-  // needed to generate runtime classes
-  permission java.lang.RuntimePermission "createClassLoader";
-};
-
 //// test framework permissions.
 //// These are mock objects and test management that we allow test framework libs
 //// to provide on our behalf. But tests themselves cannot do this stuff!

core/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java

@@ -25,15 +25,21 @@
 import org.elasticsearch.bootstrap.Security;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.common.io.PathUtils;
-import org.elasticsearch.common.logging.Loggers;
 
 import java.io.FilePermission;
+import java.io.InputStream;
+import java.net.URI;
 import java.net.URL;
 import java.nio.file.Path;
+import java.security.Permission;
+import java.security.PermissionCollection;
 import java.security.Permissions;
 import java.security.Policy;
-import java.util.Map;
+import java.security.URIParameter;
+import java.util.Collections;
+import java.util.List;
 import java.util.Objects;
+import java.util.Properties;
 
 import static com.carrotsearch.randomizedtesting.RandomizedTest.systemPropertyAsBoolean;
 
@@ -117,33 +123,43 @@ public class BootstrapForTesting {
                 final Policy policy;
                 // 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) {
-                    // look for plugin classname as a resource to determine what project we are.
-                    // while gross, this will work with any IDE.
-                    for (Map.Entry<String,String> kv : Security.SPECIAL_PLUGINS.entrySet()) {
-                        String resource = kv.getValue().replace('.', '/') + ".class";
-                        if (BootstrapForTesting.class.getClassLoader().getResource(resource) != null) {
-                            artifact = kv.getKey();
-                            break;
+                List<URL> pluginPolicies = Collections.list(BootstrapForTesting.class.getClassLoader().getResources("plugin-security.policy"));
+                if (!pluginPolicies.isEmpty()) {
+                    Permissions extra = new Permissions();
+                    for (URL url : pluginPolicies) {
+                        URI uri = url.toURI();
+                        Policy pluginPolicy = Policy.getInstance("JavaPolicy", new URIParameter(uri));
+                        PermissionCollection permissions = pluginPolicy.getPermissions(BootstrapForTesting.class.getProtectionDomain());
+                        // this method is supported with the specific implementation we use, but just check for safety.
+                        if (permissions == Policy.UNSUPPORTED_EMPTY_COLLECTION) {
+                            throw new UnsupportedOperationException("JavaPolicy implementation does not support retrieving permissions");
+                        }
+                        for (Permission permission : Collections.list(permissions.elements())) {
+                            extra.add(permission);
                         }
                     }
-                }
-                String pluginProp = Security.getPluginProperty(artifact);
-                if (pluginProp != null) {
-                    policy = new MockPluginPolicy(perms, pluginProp);
+                    // TODO: try to get rid of this class now that the world is simpler?
+                    policy = new MockPluginPolicy(perms, extra);
                 } else {
-                    policy = new ESPolicy(perms);
+                    policy = new ESPolicy(perms, Collections.emptyMap());
                 }
                 Policy.setPolicy(policy);
                 System.setSecurityManager(new TestSecurityManager());
                 Security.selfTest();
 
-                if (pluginProp != null) {
-                    // initialize the plugin class, in case it has one-time hacks (unit tests often won't do this)
-                    String clazz = Security.getPluginClass(artifact);
-                    Class.forName(clazz);
+                // guarantee plugin classes are initialized first, in case they have one-time hacks.
+                // this just makes unit testing more realistic
+                for (URL url : Collections.list(BootstrapForTesting.class.getClassLoader().getResources("plugin-descriptor.properties"))) {
+                    Properties properties = new Properties();
+                    try (InputStream stream = url.openStream()) {
+                        properties.load(stream);
+                    }
+                    if (Boolean.parseBoolean(properties.getProperty("jvm"))) {
+                        String clazz = properties.getProperty("classname");
+                        if (clazz != null) {
+                            Class.forName(clazz);
+                        }
+                    }
                 }
             } catch (Exception e) {
                 throw new RuntimeException("unable to install test security manager", e);

core/src/test/java/org/elasticsearch/bootstrap/ESPolicyTests.java

@@ -32,6 +32,7 @@
 import java.security.PrivilegedAction;
 import java.security.ProtectionDomain;
 import java.security.cert.Certificate;
+import java.util.Collections;
 
 /** 
  * Tests for ESPolicy
@@ -54,7 +55,7 @@ public void testNullCodeSource() throws Exception {
         Permission all = new AllPermission();
         PermissionCollection allCollection = all.newPermissionCollection();
         allCollection.add(all);
-        ESPolicy policy = new ESPolicy(allCollection);
+        ESPolicy policy = new ESPolicy(allCollection, Collections.emptyMap());
         // restrict ourselves to NoPermission
         PermissionCollection noPermissions = new Permissions();
         assertFalse(policy.implies(new ProtectionDomain(null, noPermissions), new FilePermission("foo", "read")));
@@ -68,7 +69,7 @@ public void testNullCodeSource() throws Exception {
     public void testNullLocation() throws Exception {
         assumeTrue("test cannot run with security manager", System.getSecurityManager() == null);
         PermissionCollection noPermissions = new Permissions();
-        ESPolicy policy = new ESPolicy(noPermissions);
+        ESPolicy policy = new ESPolicy(noPermissions, Collections.emptyMap());
         assertFalse(policy.implies(new ProtectionDomain(new CodeSource(null, (Certificate[])null), noPermissions), new FilePermission("foo", "read")));
     }
 

core/src/test/java/org/elasticsearch/bootstrap/MockPluginPolicy.java

@@ -29,7 +29,6 @@
 import java.security.CodeSource;
 import java.security.Permission;
 import java.security.PermissionCollection;
-import java.security.Permissions;
 import java.security.Policy;
 import java.security.ProtectionDomain;
 import java.security.cert.Certificate;
@@ -58,33 +57,11 @@ final class MockPluginPolicy extends Policy {
      * adding the extra plugin permissions from {@code insecurePluginProp} to
      * all code except test classes.
      */
-    MockPluginPolicy(Permissions permissions, String insecurePluginProp) throws Exception {
+    MockPluginPolicy(PermissionCollection standard, PermissionCollection extra) throws Exception {
         // the hack begins!
 
-        // parse whole policy file, with and without the substitution, compute the delta
-        standardPolicy = new ESPolicy(permissions);
-
-        URL bogus = new URL("file:/bogus"); // its "any old codebase" this time: generic permissions
-        PermissionCollection smallPermissions = standardPolicy.template.getPermissions(new CodeSource(bogus, (Certificate[])null)); 
-        Set<Permission> small = new HashSet<>(Collections.list(smallPermissions.elements()));
-
-        // set the URL for the property substitution, this time it will also have special permissions
-        System.setProperty(insecurePluginProp, bogus.toString());
-        ESPolicy biggerPolicy = new ESPolicy(permissions);
-        System.clearProperty(insecurePluginProp);
-        PermissionCollection bigPermissions = biggerPolicy.template.getPermissions(new CodeSource(bogus, (Certificate[])null));
-        Set<Permission> big = new HashSet<>(Collections.list(bigPermissions.elements()));
-
-        // compute delta to remove all the generic permissions
-        // we want equals() vs implies() for this check, in case we need 
-        // to pass along any UnresolvedPermission to the plugin
-        big.removeAll(small);
-
-        // build collection of the special permissions for easy checking
-        extraPermissions = new Permissions();
-        for (Permission p : big) {
-            extraPermissions.add(p);
-        }
+        this.standardPolicy = new ESPolicy(standard, Collections.emptyMap());
+        this.extraPermissions = extra;
 
         excludedSources = new HashSet<CodeSource>();
         // exclude some obvious places
@@ -101,7 +78,7 @@ final class MockPluginPolicy extends Policy {
         // scripts
         excludedSources.add(new CodeSource(new URL("file:" + BootstrapInfo.UNTRUSTED_CODEBASE), (Certificate[])null));
 
-        Loggers.getLogger(getClass()).debug("Apply permissions [{}] excluding codebases [{}]", extraPermissions, excludedSources);
+        Loggers.getLogger(getClass()).debug("Apply extra permissions [{}] excluding codebases [{}]", extraPermissions, excludedSources);
     }
 
     @Override