Skip to content

Commit

Permalink
Use compilation as validation for painless role template (elastic#62845)
Browse files Browse the repository at this point in the history
Role template validation now performs only compilation if the script is painless.
It no longer attempts to execute the script with empty input which is problematic.
The compliation process will catch things like invalid syntax, undefined variables,
which still provide certain level of protection against ill-defined role templates.
Behaviour for Mustache script is unchanged.
  • Loading branch information
ywangd committed Sep 29, 2020
1 parent de08ba5 commit 79077fb
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.TemplateScript;
import org.elasticsearch.xpack.core.security.authc.support.mapper.expressiondsl.ExpressionModel;
import org.elasticsearch.xpack.core.security.support.MustacheTemplateEvaluator;

Expand Down Expand Up @@ -99,7 +101,13 @@ public List<String> getRoleNames(ScriptService scriptService, ExpressionModel mo

public void validate(ScriptService scriptService) {
try {
parseTemplate(scriptService, Collections.emptyMap());
final XContentParser parser = XContentHelper.createParser(
NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, template, XContentType.JSON);
final Script script = MustacheTemplateEvaluator.parseForScript(parser, Collections.emptyMap());
final TemplateScript compiledTemplate = scriptService.compile(script, TemplateScript.CONTEXT).newInstance(script.getParams());
if ("mustache".equals(script.getLang())) {
compiledTemplate.execute();
}
} catch (IllegalArgumentException e) {
throw e;
} catch (IOException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ private MustacheTemplateEvaluator() {
throw new UnsupportedOperationException("Cannot construct " + MustacheTemplateEvaluator.class);
}

public static String evaluate(ScriptService scriptService, XContentParser parser, Map<String, Object> extraParams) throws IOException {
public static Script parseForScript(XContentParser parser, Map<String, Object> extraParams) throws IOException {
Script script = Script.parse(parser);
// Add the user details to the params
Map<String, Object> params = new HashMap<>();
Expand All @@ -36,6 +36,11 @@ public static String evaluate(ScriptService scriptService, XContentParser parser
// Always enforce mustache script lang:
script = new Script(script.getType(), script.getType() == ScriptType.STORED ? null : "mustache", script.getIdOrCode(),
script.getOptions(), params);
return script;
}

public static String evaluate(ScriptService scriptService, XContentParser parser, Map<String, Object> extraParams) throws IOException {
Script script = parseForScript(parser, extraParams);
TemplateScript compiledTemplate = scriptService.compile(script, TemplateScript.CONTEXT).newInstance(script.getParams());
return compiledTemplate.execute();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.script.ScriptEngine;
import org.elasticsearch.script.ScriptException;
import org.elasticsearch.script.ScriptMetadata;
import org.elasticsearch.script.ScriptModule;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.StoredScriptSource;
import org.elasticsearch.script.TemplateScript;
import org.elasticsearch.script.mustache.MustacheScriptEngine;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.EqualsHashCodeTestUtils;
Expand All @@ -35,12 +37,20 @@
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.mockito.Matchers.any;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

public class TemplateRoleNameTests extends ESTestCase {
Expand Down Expand Up @@ -183,6 +193,49 @@ public void testValidateWillFailForSyntaxError() {
assertTrue(e.getCause() instanceof ScriptException);
}

public void testValidateWillCompileButNotExecutePainlessScript() {
final TemplateScript compiledScript = mock(TemplateScript.class);
doThrow(new IllegalStateException("Validate should not execute painless script")).when(compiledScript).execute();
final TemplateScript.Factory scriptFactory = mock(TemplateScript.Factory.class);
when(scriptFactory.newInstance(any())).thenReturn(compiledScript);

final ScriptEngine scriptEngine = mock(ScriptEngine.class);
when(scriptEngine.getType()).thenReturn("painless");
when(scriptEngine.compile(eq("valid"), eq("params.metedata.group"), any(),
eq(org.elasticsearch.common.collect.Map.of())))
.thenReturn(scriptFactory);
final ScriptException scriptException =
new ScriptException("exception", new IllegalStateException(), org.elasticsearch.common.collect.List.of(),
"bad syntax", "painless");
doThrow(scriptException)
.when(scriptEngine).compile(eq("invalid"), eq("bad syntax"), any(),
eq(org.elasticsearch.common.collect.Map.of()));

final ScriptService scriptService = new ScriptService(Settings.EMPTY,
org.elasticsearch.common.collect.Map.of("painless", scriptEngine), ScriptModule.CORE_CONTEXTS) {
@Override
protected StoredScriptSource getScriptFromClusterState(String id) {
if ("valid".equals(id)) {
return new StoredScriptSource("painless", "params.metedata.group",
org.elasticsearch.common.collect.Map.of());
} else {
return new StoredScriptSource("painless", "bad syntax",
org.elasticsearch.common.collect.Map.of());
}
}
};
// Validation succeeds if compilation is successful
new TemplateRoleName(new BytesArray("{ \"id\":\"valid\" }"), Format.STRING).validate(scriptService);
verify(scriptEngine, times(1))
.compile(eq("valid"), eq("params.metedata.group"), any(), eq(org.elasticsearch.common.collect.Map.of()));
verify(compiledScript, never()).execute();

// Validation fails if compilation fails
final IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
() -> new TemplateRoleName(new BytesArray("{ \"id\":\"invalid\" }"), Format.STRING).validate(scriptService));
assertSame(scriptException, e.getCause());
}

public void testValidationWillFailWhenInlineScriptIsNotEnabled() {
final Settings settings = Settings.builder().put("script.allowed_types", ScriptService.ALLOW_NONE).build();
final ScriptService scriptService = new ScriptService(settings,
Expand Down

0 comments on commit 79077fb

Please sign in to comment.