From 44f08717bac83af48660e5d98928814b066788fa Mon Sep 17 00:00:00 2001 From: Jack Conradson Date: Fri, 2 Nov 2018 16:07:54 -0700 Subject: [PATCH] [Scripting] Make Max Script Length Setting Dynamic (#35184) This changes the current script.max_size_in_bytes to be dynamic so it can be set through the cluster settings API. This setting is also applied to inline scripts in the compile method of ScriptService to prevent excessively long inline scripts from being compiled. The script length limit is removed from Painless as this is no longer necessary with the protection in compile. --- .../modules/scripting/using.asciidoc | 2 +- .../org/elasticsearch/painless/Compiler.java | 17 ------- .../painless/WhenThingsGoWrongTests.java | 16 ------ .../elasticsearch/script/ScriptMetaData.java | 9 +++- .../elasticsearch/script/ScriptService.java | 51 ++++++++++++++++--- .../script/ScriptServiceTests.java | 30 +++++++++++ 6 files changed, 84 insertions(+), 41 deletions(-) diff --git a/docs/reference/modules/scripting/using.asciidoc b/docs/reference/modules/scripting/using.asciidoc index 725808e1936..86202a98dd5 100644 --- a/docs/reference/modules/scripting/using.asciidoc +++ b/docs/reference/modules/scripting/using.asciidoc @@ -204,7 +204,7 @@ you can change this behavior by using the `script.cache.expire` setting. You can configure the size of this cache by using the `script.cache.max_size` setting. By default, the cache size is `100`. -NOTE: The size of stored scripts is limited to 65,535 bytes. This can be +NOTE: The size of scripts is limited to 65,535 bytes. This can be changed by setting `script.max_size_in_bytes` setting to increase that soft limit, but if scripts are really large then a <> should be considered. diff --git a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java index 81cc802916d..0ea3d4af81f 100644 --- a/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java +++ b/modules/lang-painless/src/main/java/org/elasticsearch/painless/Compiler.java @@ -48,11 +48,6 @@ import static org.elasticsearch.painless.node.SSource.MainMethodReserved; */ final class Compiler { - /** - * The maximum number of characters allowed in the script source. - */ - static final int MAXIMUM_SOURCE_LENGTH = 16384; - /** * Define the class with lowest privileges. */ @@ -212,12 +207,6 @@ final class Compiler { * @return An executable script that implements both a specified interface and is a subclass of {@link PainlessScript} */ Constructor compile(Loader loader, MainMethodReserved reserved, String name, String source, CompilerSettings settings) { - if (source.length() > MAXIMUM_SOURCE_LENGTH) { - throw new IllegalArgumentException("Scripts may be no longer than " + MAXIMUM_SOURCE_LENGTH + - " characters. The passed in script is " + source.length() + " characters. Consider using a" + - " plugin if a script longer than this length is a requirement."); - } - ScriptClassInfo scriptClassInfo = new ScriptClassInfo(painlessLookup, scriptClass); SSource root = Walker.buildPainlessTree(scriptClassInfo, reserved, name, source, settings, painlessLookup, null); @@ -248,12 +237,6 @@ final class Compiler { * @return The bytes for compilation. */ byte[] compile(String name, String source, CompilerSettings settings, Printer debugStream) { - if (source.length() > MAXIMUM_SOURCE_LENGTH) { - throw new IllegalArgumentException("Scripts may be no longer than " + MAXIMUM_SOURCE_LENGTH + - " characters. The passed in script is " + source.length() + " characters. Consider using a" + - " plugin if a script longer than this length is a requirement."); - } - ScriptClassInfo scriptClassInfo = new ScriptClassInfo(painlessLookup, scriptClass); SSource root = Walker.buildPainlessTree(scriptClassInfo, new MainMethodReserved(), name, source, settings, painlessLookup, debugStream); diff --git a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java index 32d74d0837c..d1db6606c86 100644 --- a/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java +++ b/modules/lang-painless/src/test/java/org/elasticsearch/painless/WhenThingsGoWrongTests.java @@ -24,7 +24,6 @@ import org.apache.lucene.util.Constants; import org.elasticsearch.script.ScriptException; import java.lang.invoke.WrongMethodTypeException; -import java.util.Arrays; import java.util.Collections; import static java.util.Collections.emptyMap; @@ -200,21 +199,6 @@ public class WhenThingsGoWrongTests extends ScriptTestCase { "The maximum number of statements that can be executed in a loop has been reached.")); } - public void testSourceLimits() { - final char[] tooManyChars = new char[Compiler.MAXIMUM_SOURCE_LENGTH + 1]; - Arrays.fill(tooManyChars, '0'); - - IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, false, () -> { - exec(new String(tooManyChars)); - }); - assertTrue(expected.getMessage().contains("Scripts may be no longer than")); - - final char[] exactlyAtLimit = new char[Compiler.MAXIMUM_SOURCE_LENGTH]; - Arrays.fill(exactlyAtLimit, '0'); - // ok - assertEquals(0, exec(new String(exactlyAtLimit))); - } - public void testIllegalDynamicMethod() { IllegalArgumentException expected = expectScriptThrows(IllegalArgumentException.class, () -> { exec("def x = 'test'; return x.getClass().toString()"); diff --git a/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java b/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java index 2d8e7f5ed6b..52a06db5829 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptMetaData.java @@ -68,7 +68,7 @@ public final class ScriptMetaData implements MetaData.Custom, Writeable, ToXCont * is no existing {@link ScriptMetaData}. */ public Builder(ScriptMetaData previous) { - this.scripts = previous == null ? new HashMap<>() :new HashMap<>(previous.scripts); + this.scripts = previous == null ? new HashMap<>() : new HashMap<>(previous.scripts); } /** @@ -352,6 +352,13 @@ public final class ScriptMetaData implements MetaData.Custom, Writeable, ToXCont return MetaData.ALL_CONTEXTS; } + /** + * Returns the map of stored scripts. + */ + Map getStoredScripts() { + return scripts; + } + /** * Retrieves a stored script based on a user-specified id. */ diff --git a/server/src/main/java/org/elasticsearch/script/ScriptService.java b/server/src/main/java/org/elasticsearch/script/ScriptService.java index a21e21eafec..7c4011a63c4 100644 --- a/server/src/main/java/org/elasticsearch/script/ScriptService.java +++ b/server/src/main/java/org/elasticsearch/script/ScriptService.java @@ -49,6 +49,7 @@ import org.elasticsearch.core.internal.io.IOUtils; import java.io.Closeable; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.Collections; import java.util.HashSet; import java.util.List; @@ -98,8 +99,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust public static final Setting SCRIPT_CACHE_EXPIRE_SETTING = Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.NodeScope); public static final Setting SCRIPT_MAX_SIZE_IN_BYTES = - Setting.intSetting("script.max_size_in_bytes", 65535, Property.NodeScope); - // public Setting(String key, Function defaultValue, Function parser, Property... properties) { + Setting.intSetting("script.max_size_in_bytes", 65535, 0, Property.Dynamic, Property.NodeScope); public static final Setting> SCRIPT_MAX_COMPILATIONS_RATE = new Setting<>("script.max_compilations_rate", "75/5m", MAX_COMPILATION_RATE_FUNCTION, Property.Dynamic, Property.NodeScope); @@ -123,6 +123,8 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust private ClusterState clusterState; + private int maxSizeInBytes; + private Tuple rate; private long lastInlineCompileTime; private double scriptsPerTimeWindow; @@ -221,10 +223,12 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust this.cache = cacheBuilder.removalListener(new ScriptCacheRemovalListener()).build(); this.lastInlineCompileTime = System.nanoTime(); + this.setMaxSizeInBytes(SCRIPT_MAX_SIZE_IN_BYTES.get(settings)); this.setMaxCompilationRate(SCRIPT_MAX_COMPILATIONS_RATE.get(settings)); } void registerClusterSettingsListeners(ClusterSettings clusterSettings) { + clusterSettings.addSettingsUpdateConsumer(SCRIPT_MAX_SIZE_IN_BYTES, this::setMaxSizeInBytes); clusterSettings.addSettingsUpdateConsumer(SCRIPT_MAX_COMPILATIONS_RATE, this::setMaxCompilationRate); } @@ -241,6 +245,22 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust return scriptEngine; } + /** + * Changes the maximum number of bytes a script's source is allowed to have. + * @param newMaxSizeInBytes The new maximum number of bytes. + */ + void setMaxSizeInBytes(int newMaxSizeInBytes) { + for (Map.Entry source : getScriptsFromClusterState().entrySet()) { + if (source.getValue().getSource().getBytes(StandardCharsets.UTF_8).length > newMaxSizeInBytes) { + throw new IllegalArgumentException("script.max_size_in_bytes cannot be set to [" + newMaxSizeInBytes + "], " + + "stored script [" + source.getKey() + "] exceeds the new value with a size of " + + "[" + source.getValue().getSource().getBytes(StandardCharsets.UTF_8).length + "]"); + } + } + + maxSizeInBytes = newMaxSizeInBytes; + } + /** * This configures the maximum script compilations per five minute window. * @@ -295,6 +315,13 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust throw new IllegalArgumentException("cannot execute scripts using [" + context.name + "] context"); } + if (type == ScriptType.INLINE) { + if (idOrCode.getBytes(StandardCharsets.UTF_8).length > maxSizeInBytes) { + throw new IllegalArgumentException("exceeded max allowed inline script size in bytes [" + maxSizeInBytes + "] " + + "with size [" + idOrCode.getBytes(StandardCharsets.UTF_8).length + "] for script [" + idOrCode + "]"); + } + } + if (logger.isTraceEnabled()) { logger.trace("compiling lang: [{}] type: [{}] script: {}", lang, type, idOrCode); } @@ -391,6 +418,20 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust return contextsAllowed == null || contextsAllowed.isEmpty() == false; } + Map getScriptsFromClusterState() { + if (clusterState == null) { + return Collections.emptyMap(); + } + + ScriptMetaData scriptMetadata = clusterState.metaData().custom(ScriptMetaData.TYPE); + + if (scriptMetadata == null) { + return Collections.emptyMap(); + } + + return scriptMetadata.getStoredScripts(); + } + StoredScriptSource getScriptFromClusterState(String id) { ScriptMetaData scriptMetadata = clusterState.metaData().custom(ScriptMetaData.TYPE); @@ -409,10 +450,8 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust public void putStoredScript(ClusterService clusterService, PutStoredScriptRequest request, ActionListener listener) { - int max = SCRIPT_MAX_SIZE_IN_BYTES.get(settings); - - if (request.content().length() > max) { - throw new IllegalArgumentException("exceeded max allowed stored script size in bytes [" + max + "] with size [" + + if (request.content().length() > maxSizeInBytes) { + throw new IllegalArgumentException("exceeded max allowed stored script size in bytes [" + maxSizeInBytes + "] with size [" + request.content().length() + "] for script [" + request.id() + "]"); } diff --git a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java index 22a250710ba..271007f9978 100644 --- a/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java +++ b/server/src/test/java/org/elasticsearch/script/ScriptServiceTests.java @@ -27,6 +27,7 @@ import org.elasticsearch.common.breaker.CircuitBreakingException; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.settings.ClusterSettings; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentFactory; @@ -54,6 +55,7 @@ public class ScriptServiceTests extends ESTestCase { private Map> contexts; private ScriptService scriptService; private Settings baseSettings; + private ClusterSettings clusterSettings; @Before public void setup() throws IOException { @@ -78,12 +80,22 @@ public class ScriptServiceTests extends ESTestCase { private void buildScriptService(Settings additionalSettings) throws IOException { Settings finalSettings = Settings.builder().put(baseSettings).put(additionalSettings).build(); scriptService = new ScriptService(finalSettings, engines, contexts) { + @Override + Map getScriptsFromClusterState() { + Map scripts = new HashMap<>(); + scripts.put("test1", new StoredScriptSource("test", "1+1", Collections.emptyMap())); + scripts.put("test2", new StoredScriptSource("test", "1", Collections.emptyMap())); + return scripts; + } + @Override StoredScriptSource getScriptFromClusterState(String id) { //mock the script that gets retrieved from an index return new StoredScriptSource("test", "1+1", Collections.emptyMap()); } }; + clusterSettings = new ClusterSettings(finalSettings, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS); + scriptService.registerClusterSettingsListeners(clusterSettings); } // even though circuit breaking is allowed to be configured per minute, we actually weigh this over five minutes @@ -305,6 +317,24 @@ public class ScriptServiceTests extends ESTestCase { assertNull(scriptService.getStoredScript(cs, new GetStoredScriptRequest("_id"))); } + public void testMaxSizeLimit() throws Exception { + buildScriptService(Settings.builder().put(ScriptService.SCRIPT_MAX_SIZE_IN_BYTES.getKey(), 4).build()); + scriptService.compile(new Script(ScriptType.INLINE, "test", "1+1", Collections.emptyMap()), randomFrom(contexts.values())); + IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> { + scriptService.compile(new Script(ScriptType.INLINE, "test", "10+10", Collections.emptyMap()), randomFrom(contexts.values())); + }); + assertEquals("exceeded max allowed inline script size in bytes [4] with size [5] for script [10+10]", iae.getMessage()); + clusterSettings.applySettings(Settings.builder().put(ScriptService.SCRIPT_MAX_SIZE_IN_BYTES.getKey(), 6).build()); + scriptService.compile(new Script(ScriptType.INLINE, "test", "10+10", Collections.emptyMap()), randomFrom(contexts.values())); + clusterSettings.applySettings(Settings.builder().put(ScriptService.SCRIPT_MAX_SIZE_IN_BYTES.getKey(), 5).build()); + scriptService.compile(new Script(ScriptType.INLINE, "test", "10+10", Collections.emptyMap()), randomFrom(contexts.values())); + iae = expectThrows(IllegalArgumentException.class, () -> { + clusterSettings.applySettings(Settings.builder().put(ScriptService.SCRIPT_MAX_SIZE_IN_BYTES.getKey(), 2).build()); + }); + assertEquals("script.max_size_in_bytes cannot be set to [2], stored script [test1] exceeds the new value with a size of [3]", + iae.getMessage()); + } + private void assertCompileRejected(String lang, String script, ScriptType scriptType, ScriptContext scriptContext) { try { scriptService.compile(new Script(scriptType, lang, script, Collections.emptyMap()), scriptContext);