Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Groovy as a scripting language, add groovy sandboxing #6233

Merged
merged 1 commit into from Jun 20, 2014
Merged

Add Groovy as a scripting language, add groovy sandboxing #6233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions dev-tools/tests.policy
View file
Open in desktop
Expand Up @@ -27,10 +27,11 @@ grant {
permission java.io.FilePermission "${junit4.childvm.cwd}", "read,execute,write";
permission java.io.FilePermission "${junit4.childvm.cwd}${/}-", "read,execute,write,delete";
permission java.io.FilePermission "${junit4.tempDir}${/}*", "read,execute,write,delete";

permission groovy.security.GroovyCodeSourcePermission "/groovy/script";

// Allow connecting to the internet anywhere
permission java.net.SocketPermission "*", "accept,listen,connect,resolve";

// Basic permissions needed for Lucene / Elasticsearch to work:
permission java.util.PropertyPermission "*", "read,write";
permission java.lang.reflect.ReflectPermission "*";
Expand Down
36 changes: 21 additions & 15 deletions pom.xml
View file
Open in desktop
Expand Up @@ -208,13 +208,6 @@
<scope>compile</scope>
</dependency>

<dependency>
<groupId>org.mvel</groupId>
<artifactId>mvel2</artifactId>
<version>2.2.0.Final</version>
<scope>compile</scope>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-core</artifactId>
Expand Down Expand Up @@ -265,6 +258,22 @@
</dependency>
<!-- END: dependencies that are shaded -->

<dependency>
<groupId>org.mvel</groupId>
<artifactId>mvel2</artifactId>
<version>2.2.0.Final</version>
<scope>compile</scope>
<optional>true</optional>
</dependency>

<dependency>
<groupId>org.codehaus.groovy</groupId>
<artifactId>groovy-all</artifactId>
<version>2.3.2</version>
<scope>compile</scope>
<optional>true</optional>
</dependency>

<dependency>
<groupId>log4j</groupId>
<artifactId>log4j</artifactId>
Expand Down Expand Up @@ -648,7 +657,6 @@
<includes>
<include>com.google.guava:guava</include>
<include>com.carrotsearch:hppc</include>
<include>org.mvel:mvel2</include>
<include>com.fasterxml.jackson.core:jackson-core</include>
<include>com.fasterxml.jackson.dataformat:jackson-dataformat-smile</include>
<include>com.fasterxml.jackson.dataformat:jackson-dataformat-yaml</include>
Expand All @@ -674,10 +682,6 @@
<pattern>jsr166e</pattern>
<shadedPattern>org.elasticsearch.common.util.concurrent.jsr166e</shadedPattern>
</relocation>
<relocation>
<pattern>org.mvel2</pattern>
<shadedPattern>org.elasticsearch.common.mvel2</shadedPattern>
</relocation>
<relocation>
<pattern>com.fasterxml.jackson</pattern>
<shadedPattern>org.elasticsearch.common.jackson</shadedPattern>
Expand Down Expand Up @@ -870,7 +874,7 @@
</data>
<data>
<src>${project.build.directory}/lib</src>
<includes>lucene*, log4j*, jna*, spatial4j*, jts*</includes>
<includes>lucene*, log4j*, jna*, spatial4j*, jts*, groovy*, mvel*</includes>
<type>directory</type>
<mapper>
<type>perm</type>
Expand Down Expand Up @@ -1070,6 +1074,8 @@
<include>jna*</include>
<include>spatial4j*</include>
<include>jts*</include>
<include>groovy*</include>
<include>mvel*</include>
</includes>
</source>
<source>
Expand Down Expand Up @@ -1393,7 +1399,7 @@
<version>0.6.4.201312101107</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to shade groovy as we did with mvel? and remove mvel?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what shading benefits us? Is there a reason to shade groovy?

As for removing mvel, I think that should be a PR from this one so we can discuss whether it goes to 1.3 or just 2.0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe @kimchy can comment on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dakrone If it's not shaded, and a Groovy client uses any of the popular Elasticsearch Groovy clients (only use Groovy with Gradle myself, so I am not aware of the popular ones), then will it cause a conflict/collision?

I'd expect not unless the Groovy scripts were interpreted on the client, which would be odd. Assuming it's not needed in some way by the client, then shading seems unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dakrone If it's not shaded, and a Groovy client uses any of the popular Elasticsearch Groovy clients (only use Groovy with Gradle myself, so I am not aware of the popular ones), then will it cause a conflict/collision?

I think this'd still happen. The JVM normally loads each class only once [1] so you'll just get whichever groovy it finds first. Shading fixes this by copying the class to a new name.

[1]: technically its one per class loader and class loaders are hierarchical....

So, yeah, I'm +1 for shading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nik9000 Just to add: The JVM is very good about loading classes from the same jar only once. However, it is possible to get into situations where different versions of the same jar(s) are on the classpath that cause ClassNotFound issues (because of a lack of isolation, which can be solved with class loaders) because the different versions cannot coexist. My major point that I was trying to get at is that I believe this is a server side dependency only, and that should mean that there will never be a chance for collision except in the embedded use case (ignoring any potential forked projects that use Groovy, which should just drop whichever one they want to drop).

I think the embedded concern is what makes me push +1 for shading as well, although I think it would be preferable if we could avoid shading because it would be lighter weight given its intended-to-be optional nature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My major point that I was trying to get at is that I believe this is a server side dependency only

it would be preferable if we could avoid shading because it would be lighter weight given its intended-to-be optional nature.

Makes sense to me. Well put.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this and decided not to shade groovy as it negates the "optional" nature of it and it will reduce the jar size for clients using only the transport client.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good! I also wonder if we want to remove mvel from shading as well and mark it optional as well?

<scope>test</scope>
</dependency>
</dependencies>
</dependencies>
<build>
<plugins>
<plugin>
Expand All @@ -1418,7 +1424,7 @@
<id>default-check</id>
<goals>
<goal>check</goal>
</goals>
</goals>
</execution>
</executions>
<configuration>
Expand Down
2 changes: 2 additions & 0 deletions src/main/assemblies/common-bin.xml
View file
Open in desktop
Expand Up @@ -9,6 +9,8 @@
<include>net.java.dev.jna:jna</include>
<include>com.spatial4j:spatial4j</include>
<include>com.vividsolutions:jts</include>
<include>org.codehaus.groovy:groovy-all</include>
<include>org.mvel:mvel2</include>
</includes>
</dependencySet>
<dependencySet>
Expand Down
12 changes: 10 additions & 2 deletions src/main/java/org/elasticsearch/script/ScriptModule.java
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@
import org.elasticsearch.common.inject.multibindings.Multibinder;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.script.groovy.GroovyScriptEngineService;
import org.elasticsearch.script.mustache.MustacheScriptEngineService;
import org.elasticsearch.script.mvel.MvelScriptEngineService;

Expand Down Expand Up @@ -77,16 +78,23 @@ protected void configure() {

Multibinder<ScriptEngineService> multibinder = Multibinder.newSetBinder(binder(), ScriptEngineService.class);
multibinder.addBinding().to(NativeScriptEngineService.class);

try {
multibinder.addBinding().to(GroovyScriptEngineService.class);
} catch (Throwable t) {
Loggers.getLogger(GroovyScriptEngineService.class).debug("failed to load groovy", t);
}

try {
multibinder.addBinding().to(MvelScriptEngineService.class);
} catch (Throwable t) {
// no MVEL
Loggers.getLogger(MvelScriptEngineService.class).debug("failed to load mvel", t);
}

try {
multibinder.addBinding().to(MustacheScriptEngineService.class);
} catch (Throwable t) {
Loggers.getLogger(MustacheScriptEngineService.class).trace("failed to load mustache", t);
Loggers.getLogger(MustacheScriptEngineService.class).debug("failed to load mustache", t);
}

for (Class<? extends ScriptEngineService> scriptEngine : scriptEngines) {
Expand Down
56 changes: 48 additions & 8 deletions src/main/java/org/elasticsearch/script/ScriptService.java
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentMap;
Expand All @@ -54,6 +55,10 @@
*/
public class ScriptService extends AbstractComponent {

public static final String DEFAULT_SCRIPTING_LANGUAGE_SETTING = "script.default_lang";
public static final String DISABLE_DYNAMIC_SCRIPTING_SETTING = "script.disable_dynamic";
public static final String DISABLE_DYNAMIC_SCRIPTING_DEFAULT = "sandbox";

private final String defaultLang;

private final ImmutableMap<String, ScriptEngineService> scriptEngines;
Expand All @@ -63,7 +68,38 @@ public class ScriptService extends AbstractComponent {
private final Cache<CacheKey, CompiledScript> cache;
private final File scriptsDirectory;

private final boolean disableDynamic;
private final DynamicScriptDisabling dynamicScriptingDisabled;

/**
* Enum defining the different dynamic settings for scripting, either
* ONLY_DISK_ALLOWED (scripts must be placed on disk), EVERYTHING_ALLOWED
* (all dynamic scripting is enabled), or SANDBOXED_ONLY (only sandboxed
* scripting languages are allowed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance we can make this a constant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will make everything a constant that can be for this.

*/
enum DynamicScriptDisabling {
EVERYTHING_ALLOWED,
ONLY_DISK_ALLOWED,
SANDBOXED_ONLY;

public static final DynamicScriptDisabling parse(String s) {
switch (s.toLowerCase(Locale.ROOT)) {
// true for "disable_dynamic" means only on-disk scripts are enabled
case "true":
case "all":
return ONLY_DISK_ALLOWED;
// false for "disable_dynamic" means all scripts are enabled
case "false":
case "none":
return EVERYTHING_ALLOWED;
// only sandboxed scripting is enabled
case "sandbox":
case "sandboxed":
return SANDBOXED_ONLY;
default:
throw new ElasticsearchIllegalArgumentException("Unrecognized script allowance setting: [" + s + "]");
}
}
}

@Inject
public ScriptService(Settings settings, Environment env, Set<ScriptEngineService> scriptEngines,
Expand All @@ -74,8 +110,8 @@ public ScriptService(Settings settings, Environment env, Set<ScriptEngineService
TimeValue cacheExpire = componentSettings.getAsTime("cache.expire", null);
logger.debug("using script cache with max_size [{}], expire [{}]", cacheMaxSize, cacheExpire);

this.defaultLang = componentSettings.get("default_lang", "mvel");
this.disableDynamic = componentSettings.getAsBoolean("disable_dynamic", true);
this.defaultLang = settings.get(DEFAULT_SCRIPTING_LANGUAGE_SETTING, "mvel");
this.dynamicScriptingDisabled = DynamicScriptDisabling.parse(settings.get(DISABLE_DYNAMIC_SCRIPTING_SETTING, DISABLE_DYNAMIC_SCRIPTING_DEFAULT));

CacheBuilder cacheBuilder = CacheBuilder.newBuilder();
if (cacheMaxSize >= 0) {
Expand Down Expand Up @@ -130,7 +166,7 @@ public CompiledScript compile(String lang, String script) {
lang = defaultLang;
}
if (!dynamicScriptEnabled(lang)) {
throw new ScriptException("dynamic scripting disabled");
throw new ScriptException("dynamic scripting for [" + lang + "] disabled");
}
CacheKey cacheKey = new CacheKey(lang, script);
compiled = cache.getIfPresent(cacheKey);
Expand Down Expand Up @@ -180,12 +216,16 @@ private boolean dynamicScriptEnabled(String lang) {
if (service == null) {
throw new ElasticsearchIllegalArgumentException("script_lang not supported [" + lang + "]");
}
// Templating languages and native scripts are always allowed
// "native" executions are registered through plugins
if (service.sandboxed() || "native".equals(lang)) {

// Templating languages (mustache) and native scripts are always
// allowed, "native" executions are registered through plugins
if (this.dynamicScriptingDisabled == DynamicScriptDisabling.EVERYTHING_ALLOWED || "native".equals(lang) || "mustache".equals(lang)) {
return true;
} else if (this.dynamicScriptingDisabled == DynamicScriptDisabling.ONLY_DISK_ALLOWED) {
return false;
} else {
return service.sandboxed();
}
return !disableDynamic;
}

private class ScriptChangesListener extends FileChangesListener {
Expand Down