Skip to content

Commit

Permalink
[Scripting] Make Max Script Length Setting Dynamic (#35184)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jdconrad authored Nov 2, 2018
1 parent 0166388 commit 44f0871
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 41 deletions.
2 changes: 1 addition & 1 deletion docs/reference/modules/scripting/using.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -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
<<modules-scripting-engine,native script engine>> should be considered.
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,6 @@
*/
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.
*/
Expand Down Expand Up @@ -212,12 +207,6 @@ private static void addFactoryMethod(Map<String, Class<?>> additionalClasses, Cl
* @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);
Expand Down Expand Up @@ -248,12 +237,6 @@ Constructor<?> compile(Loader loader, MainMethodReserved reserved, String name,
* @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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import org.elasticsearch.script.ScriptException;

import java.lang.invoke.WrongMethodTypeException;
import java.util.Arrays;
import java.util.Collections;

import static java.util.Collections.emptyMap;
Expand Down Expand Up @@ -200,21 +199,6 @@ public void testLoopLimits() {
"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()");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ public static final class Builder {
* 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);
}

/**
Expand Down Expand Up @@ -352,6 +352,13 @@ public EnumSet<MetaData.XContentContext> context() {
return MetaData.ALL_CONTEXTS;
}

/**
* Returns the map of stored scripts.
*/
Map<String, StoredScriptSource> getStoredScripts() {
return scripts;
}

/**
* Retrieves a stored script based on a user-specified id.
*/
Expand Down
51 changes: 45 additions & 6 deletions server/src/main/java/org/elasticsearch/script/ScriptService.java
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@

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;
Expand Down Expand Up @@ -98,8 +99,7 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust
public static final Setting<TimeValue> SCRIPT_CACHE_EXPIRE_SETTING =
Setting.positiveTimeSetting("script.cache.expire", TimeValue.timeValueMillis(0), Property.NodeScope);
public static final Setting<Integer> SCRIPT_MAX_SIZE_IN_BYTES =
Setting.intSetting("script.max_size_in_bytes", 65535, Property.NodeScope);
// public Setting(String key, Function<Settings, String> defaultValue, Function<String, T> parser, Property... properties) {
Setting.intSetting("script.max_size_in_bytes", 65535, 0, Property.Dynamic, Property.NodeScope);
public static final Setting<Tuple<Integer, TimeValue>> SCRIPT_MAX_COMPILATIONS_RATE =
new Setting<>("script.max_compilations_rate", "75/5m", MAX_COMPILATION_RATE_FUNCTION, Property.Dynamic, Property.NodeScope);

Expand All @@ -123,6 +123,8 @@ public class ScriptService extends AbstractComponent implements Closeable, Clust

private ClusterState clusterState;

private int maxSizeInBytes;

private Tuple<Integer, TimeValue> rate;
private long lastInlineCompileTime;
private double scriptsPerTimeWindow;
Expand Down Expand Up @@ -221,10 +223,12 @@ public ScriptService(Settings settings, Map<String, ScriptEngine> engines, Map<S
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);
}

Expand All @@ -241,6 +245,22 @@ private ScriptEngine getEngine(String lang) {
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<String, StoredScriptSource> 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.
*
Expand Down Expand Up @@ -295,6 +315,13 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp
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);
}
Expand Down Expand Up @@ -391,6 +418,20 @@ public boolean isAnyContextEnabled() {
return contextsAllowed == null || contextsAllowed.isEmpty() == false;
}

Map<String, StoredScriptSource> 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);

Expand All @@ -409,10 +450,8 @@ StoredScriptSource getScriptFromClusterState(String id) {

public void putStoredScript(ClusterService clusterService, PutStoredScriptRequest request,
ActionListener<AcknowledgedResponse> 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() + "]");
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
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;
Expand Down Expand Up @@ -54,6 +55,7 @@ public class ScriptServiceTests extends ESTestCase {
private Map<String, ScriptContext<?>> contexts;
private ScriptService scriptService;
private Settings baseSettings;
private ClusterSettings clusterSettings;

@Before
public void setup() throws IOException {
Expand All @@ -78,12 +80,22 @@ public void setup() throws IOException {
private void buildScriptService(Settings additionalSettings) throws IOException {
Settings finalSettings = Settings.builder().put(baseSettings).put(additionalSettings).build();
scriptService = new ScriptService(finalSettings, engines, contexts) {
@Override
Map<String, StoredScriptSource> getScriptsFromClusterState() {
Map<String, StoredScriptSource> 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
Expand Down Expand Up @@ -305,6 +317,24 @@ public void testGetStoredScript() throws Exception {
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);
Expand Down

0 comments on commit 44f0871

Please sign in to comment.