Merge pull request #13638 from rmuir/more_realistic_unit_tests_for_plugins_like_this

Better simulate problematic plugins permissions in unit tests.

core/pom.xml

@@ -251,6 +251,7 @@
                             <includes>
                                 <include>org/elasticsearch/test/**/*</include>
                                 <include>org/elasticsearch/bootstrap/BootstrapForTesting.class</include>
+                                <include>org/elasticsearch/bootstrap/MockPluginPolicy.class</include>
                                 <include>org/elasticsearch/common/cli/CliToolTestCase.class</include>
                                 <include>org/elasticsearch/common/cli/CliToolTestCase$*.class</include>
                             </includes>
@@ -279,7 +280,7 @@
                                 <include>rest-api-spec/**/*</include>
                                 <include>org/elasticsearch/test/**/*</include>
                                 <include>org/elasticsearch/bootstrap/BootstrapForTesting.class</include>
-                                <include>org/elasticsearch/bootstrap/XTestSecurityManager*.class</include>
+                                <include>org/elasticsearch/bootstrap/MockPluginPolicy.class</include>
                                 <include>org/elasticsearch/common/cli/CliToolTestCase.class</include>
                                 <include>org/elasticsearch/common/cli/CliToolTestCase$*.class</include>
                                 <include>org/elasticsearch/cluster/MockInternalClusterInfoService.class</include>

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

@@ -30,13 +30,8 @@ import org.elasticsearch.common.logging.Loggers;
 import java.io.FilePermission;
 import java.net.URL;
 import java.nio.file.Path;
-import java.security.CodeSource;
-import java.security.Permission;
-import java.security.PermissionCollection;
 import java.security.Permissions;
 import java.security.Policy;
-import java.security.cert.Certificate;
-import java.util.Collections;
 import java.util.Objects;
 
 import static com.carrotsearch.randomizedtesting.RandomizedTest.systemPropertyAsBoolean;
@@ -119,14 +114,17 @@ public class BootstrapForTesting {
                     perms.add(new FilePermission(coverageDir.resolve("jacoco-it.exec").toString(), "read,write"));
                 }
 
-                // if its an insecure plugin, its not easy to simulate here, since we don't have a real plugin install.
+                final Policy policy;
-                // we just do our best so unit testing can work. integration tests for such plugins are essential.
+                // if its an insecure plugin, we use a wrapper policy impl to try
+                // to simulate what happens with a real distribution
                 String artifact = System.getProperty("tests.artifact");
                 String insecurePluginProp = Security.INSECURE_PLUGINS.get(artifact);
                 if (insecurePluginProp != null) {
-                    setInsecurePluginPermissions(perms, insecurePluginProp);
+                    policy = new MockPluginPolicy(perms, insecurePluginProp);
+                } else {
+                    policy = new ESPolicy(perms);
                 }
-                Policy.setPolicy(new ESPolicy(perms));
+                Policy.setPolicy(policy);
                 System.setSecurityManager(new TestSecurityManager());
                 Security.selfTest();
 
@@ -144,44 +142,6 @@ public class BootstrapForTesting {
         }
     }
 
-    /**
-     * with a real plugin install, we just set a property to plugin/foo*, which matches
-     * plugin code and all dependencies. when running unit tests, things are disorganized,
-     * and might even be on different filesystem roots (windows), so we can't even make
-     * a URL that will match everything. instead, add the extra permissions globally.
-     */
-    // TODO: maybe wrap with a policy so the extra permissions aren't applied to test classes/framework,
-    // so that stacks are always polluted and tests fail for missing AccessController blocks...
-    static void setInsecurePluginPermissions(Permissions permissions, String insecurePluginProp) throws Exception {
-        // the hack begins!
-        
-        // parse whole policy file, with and without the substitution, compute the delta, then add globally.
-        URL bogus = new URL("file:/bogus");
-        ESPolicy policy = new ESPolicy(new Permissions());
-        PermissionCollection small = policy.template.getPermissions(new CodeSource(bogus, (Certificate[])null));
-        System.setProperty(insecurePluginProp, bogus.toString());
-        policy = new ESPolicy(new Permissions());
-        System.clearProperty(insecurePluginProp);
-        PermissionCollection big = policy.template.getPermissions(new CodeSource(bogus, (Certificate[])null));
-        
-        PermissionCollection delta = delta(small, big);
-        for (Permission p : Collections.list(delta.elements())) {
-            permissions.add(p);
-        }
-    }
-    
-    // computes delta of small and big, the slow way
-    static PermissionCollection delta(PermissionCollection small, PermissionCollection big) {
-        Permissions extra = new Permissions();
-        for (Permission p : Collections.list(big.elements())) {
-            // check big too, to remove UnresolvedPermissions (acts like NaN)
-            if (big.implies(p) && small.implies(p) == false) {
-                extra.add(p);
-            }
-        }
-        return extra;
-    }
-
     // does nothing, just easy way to make sure the class is loaded.
     public static void ensureInitialized() {}
 }

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

@@ -0,0 +1,125 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.elasticsearch.bootstrap;
+
+import com.carrotsearch.randomizedtesting.RandomizedRunner;
+
+import org.apache.lucene.util.LuceneTestCase;
+import org.elasticsearch.common.io.PathUtils;
+import org.elasticsearch.common.logging.Loggers;
+import org.junit.Assert;
+
+import java.net.URL;
+import java.nio.file.Path;
+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;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Simulates in unit tests per-plugin permissions.
+ * Unit tests for plugins do not have a proper plugin structure,
+ * so we don't know which codebases to apply the permission to.
+ * <p>
+ * As an approximation, we just exclude es/test/framework classes,
+ * because they will be present in stacks and fail tests for the 
+ * simple case where an AccessController block is missing, because
+ * java security checks every codebase in the stacktrace, and we
+ * are sure to pollute it.
+ */
+final class MockPluginPolicy extends Policy {
+    final ESPolicy standardPolicy;
+    final PermissionCollection extraPermissions;
+    final Set<CodeSource> extraSources;
+
+    /**
+     * Create a new MockPluginPolicy with dynamic {@code permissions} and
+     * adding the extra plugin permissions from {@code insecurePluginProp} to
+     * all code except test classes.
+     */
+    MockPluginPolicy(Permissions permissions, String insecurePluginProp) 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);
+        }
+
+        // every element in classpath except test-classes/
+        extraSources = new HashSet<CodeSource>();
+        for (URL location : JarHell.parseClassPath()) {
+            Path path = PathUtils.get(location.toURI());
+            String baseName = path.getFileName().toString();
+            if (baseName.contains("test-classes") == false) {
+              extraSources.add(new CodeSource(location, (Certificate[])null));
+            }
+        }
+        // exclude some obvious places
+        // es core
+        extraSources.remove(Bootstrap.class.getProtectionDomain().getCodeSource());
+        // es test framework
+        extraSources.remove(getClass().getProtectionDomain().getCodeSource());
+        // lucene test framework
+        extraSources.remove(LuceneTestCase.class.getProtectionDomain().getCodeSource());
+        // test runner
+        extraSources.remove(RandomizedRunner.class.getProtectionDomain().getCodeSource());
+        // junit library
+        extraSources.remove(Assert.class.getProtectionDomain().getCodeSource());
+
+        Loggers.getLogger(getClass()).debug("Apply permissions [{}] to codebases [{}]", extraPermissions, extraSources);
+    }
+
+    @Override
+    public boolean implies(ProtectionDomain domain, Permission permission) {
+        if (standardPolicy.implies(domain, permission)) {
+            return true;
+        } else if (extraSources.contains(domain.getCodeSource())) {
+            return extraPermissions.implies(permission);
+        } else {
+            return false;
+        }
+    }
+}

plugins/cloud-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeServiceImpl.java

@@ -37,6 +37,7 @@ import org.elasticsearch.common.unit.TimeValue;
 import java.io.IOException;
 import java.security.AccessController;
 import java.security.GeneralSecurityException;
+import java.security.PrivilegedActionException;
 import java.security.PrivilegedExceptionAction;
 import java.util.*;
 
@@ -59,13 +60,20 @@ public class GceComputeServiceImpl extends AbstractLifecycleComponent<GceCompute
         logger.debug("get instances for project [{}], zones [{}]", project, zones);
         final List<Instance> instances = zones.stream().map((zoneId) -> {
             try {
-                Compute.Instances.List list = client().instances().list(project, zoneId);
+                // hack around code messiness in GCE code
-                InstanceList instanceList = list.execute();
+                // TODO: get this fixed
+                InstanceList instanceList = AccessController.doPrivileged(new PrivilegedExceptionAction<InstanceList>() {
+                    @Override
+                    public InstanceList run() throws Exception {
+                        Compute.Instances.List list = client().instances().list(project, zoneId);
+                        return list.execute();
+                    }
+                });
                 if (instanceList.isEmpty()) {
                     return Collections.EMPTY_LIST;
                 }
                 return instanceList.getItems();
-            } catch (IOException e) {
+            } catch (PrivilegedActionException e) {
                 logger.warn("Problem fetching instance list for zone {}", zoneId);
                 logger.debug("Full exception:", e);
                 return Collections.EMPTY_LIST;

plugins/cloud-gce/src/main/java/org/elasticsearch/discovery/gce/GceUnicastHostsProvider.java

@@ -242,8 +242,7 @@ public class GceUnicastHostsProvider extends AbstractComponent implements Unicas
 
             }
         } catch (Throwable e) {
-            logger.warn("Exception caught during discovery {} : {}", e.getClass().getName(), e.getMessage());
+            logger.warn("Exception caught during discovery: {}", e, e.getMessage());
-            logger.trace("Exception caught during discovery", e);
         }
 
         logger.debug("{} node(s) added", cachedDiscoNodes.size());