Make script.groovy.sandbox.method_blacklist_patch truly append-only

Additionally, this setting can be specified in elasticsearch.yml if
desired, to pre-populate the list of methods to be added to the default
blacklist.

When making a change to this setting dynamically, the entire blacklist
is logged as well.

Conflicts:
	src/main/java/org/elasticsearch/script/ScriptService.java

Author: dakrone

src/main/java/org/elasticsearch/script/ScriptService.java

@@ -71,6 +71,7 @@
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.util.Arrays;
+import java.nio.file.Files;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
@@ -227,10 +228,10 @@ public void onRefreshSettings(Settings settings) {
             GroovyScriptEngineService engine = (GroovyScriptEngineService) ScriptService.this.scriptEngines.get("groovy");
             if (engine != null) {
                 String[] patches = settings.getAsArray(GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH, Strings.EMPTY_ARRAY);
-                if (Arrays.equals(patches, engine.blacklistAdditions()) == false) {
-                    logger.info("updating [{}] from {} to {}", GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH,
-                            engine.blacklistAdditions(), patches);
-                    engine.blacklistAdditions(patches);
+                boolean blacklistChanged = engine.addToBlacklist(patches);
+                if (blacklistChanged) {
+                    logger.info("adding {} to [{}], new blacklisted methods: {}", patches,
+                            GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH, engine.blacklistAdditions());
                     engine.reloadConfig();
                     // Because the GroovyScriptEngineService knows nothing about the
                     // cache, we need to clear it here if the setting changes

src/main/java/org/elasticsearch/script/groovy/GroovySandboxExpressionChecker.java

@@ -53,7 +53,7 @@ public class GroovySandboxExpressionChecker implements SecureASTCustomizer.Expre
     private final Set<String> packageWhitelist;
     private final Set<String> classWhitelist;
 
-    public GroovySandboxExpressionChecker(Settings settings, String[] blacklistAdditions) {
+    public GroovySandboxExpressionChecker(Settings settings, Set<String> blacklistAdditions) {
         this.methodBlacklist = ImmutableSet.copyOf(settings.getAsArray(GROOVY_SANDBOX_METHOD_BLACKLIST, defaultMethodBlacklist, true));
         this.additionalMethodBlacklist = ImmutableSet.copyOf(blacklistAdditions);
         this.packageWhitelist = ImmutableSet.copyOf(settings.getAsArray(GROOVY_SANDBOX_PACKAGE_WHITELIST, defaultPackageWhitelist, true));
@@ -147,7 +147,7 @@ public boolean isAuthorized(Expression expression) {
      * Returns a customized ASTCustomizer that includes the whitelists and
      * expression checker.
      */
-    public static SecureASTCustomizer getSecureASTCustomizer(Settings settings, String[] blacklistAdditions) {
+    public static SecureASTCustomizer getSecureASTCustomizer(Settings settings, Set<String> blacklistAdditions) {
         SecureASTCustomizer scz = new SecureASTCustomizer();
         // Closures are allowed
         scz.setClosuresAllowed(true);

src/main/java/org/elasticsearch/script/groovy/GroovyScriptEngineService.java

@@ -19,6 +19,7 @@
 
 package org.elasticsearch.script.groovy;
 
+import com.google.common.collect.ImmutableSet;
 import groovy.lang.Binding;
 import groovy.lang.GroovyClassLoader;
 import groovy.lang.Script;
@@ -52,7 +53,9 @@
 import java.io.IOException;
 import java.math.BigDecimal;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicLong;
 
 /**
@@ -66,21 +69,32 @@ public class GroovyScriptEngineService extends AbstractComponent implements Scri
     private final AtomicLong counter = new AtomicLong();
     private final boolean sandboxed;
     private volatile GroovyClassLoader loader;
-    private volatile String[] blacklistAdditions = Strings.EMPTY_ARRAY;
+    private volatile Set<String> blacklistAdditions;
 
     @Inject
     public GroovyScriptEngineService(Settings settings) {
         super(settings);
         this.sandboxed = settings.getAsBoolean(GROOVY_SCRIPT_SANDBOX_ENABLED, true);
+        this.blacklistAdditions = ImmutableSet.copyOf(settings.getAsArray(GROOVY_SCRIPT_BLACKLIST_PATCH, Strings.EMPTY_ARRAY));
         reloadConfig();
     }
 
-    public String[] blacklistAdditions() {
+    public Set<String> blacklistAdditions() {
         return this.blacklistAdditions;
     }
 
-    public void blacklistAdditions(String[] additions) {
-        this.blacklistAdditions = additions;
+    /**
+     * Appends the additional blacklisted methods to the current blacklist,
+     * returns true if the black list has changed
+     */
+    public boolean addToBlacklist(String... additions) {
+        Set<String> newBlackList = new HashSet<>(blacklistAdditions);
+        for (String addition : additions) {
+            newBlackList.add(addition);
+        }
+        boolean changed = this.blacklistAdditions.equals(newBlackList) == false;
+        this.blacklistAdditions = ImmutableSet.copyOf(newBlackList);
+        return changed;
     }
 
     public void reloadConfig() {

src/test/java/org/elasticsearch/script/GroovySandboxScriptTests.java

@@ -110,17 +110,6 @@ public void testDynamicBlacklist() throws Exception {
                 "Expression [MethodCallExpression] is not allowed: [doc[foo].value, 3, 4].isEmpty()");
         testFailure("[doc['foo'].value, 3, 4].size()",
                 "Expression [MethodCallExpression] is not allowed: [doc[foo].value, 3, 4].size()");
-
-        // Undo the blacklist addition and make sure the scripts still work
-        Settings emptyBlacklistSettings = ImmutableSettings.builder()
-                .put(GroovyScriptEngineService.GROOVY_SCRIPT_BLACKLIST_PATCH, "")
-                .build();
-
-        client().admin().cluster().prepareUpdateSettings().setTransientSettings(emptyBlacklistSettings).get();
-
-        testSuccess("[doc['foo'].value, 3, 4].isEmpty()");
-        testSuccess("[doc['foo'].value, 3, 4].size()");
-
     }
 
     public void testSuccess(String script) {