Skip to content

Commit 5d001d1

Browse files
committed
Decentralize plugin security
* Add ability for plugins to declare additional permissions with a custom plugin-security.policy file and corresponding AccessController logic. See the plugin author's guide for more information. * Add warning messages to users for extra plugin permissions in bin/plugin. * When bin/plugin is run interactively (stdin is a controlling terminal and -b/--batch not supplied), require user confirmation. * Improve unit test and IDE support for plugins with additional permissions by exposing plugin's metadata as a maven test resource. Closes #14108 Squashed commit of the following: commit cf8ace6 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 13:36:05 2015 -0400 fix new unit test from master merge commit 9be3c5a Merge: 2f168b8 7368231 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:58:31 2015 -0400 Merge branch 'master' into off_my_back commit 2f168b8 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:56:04 2015 -0400 improve plugin author documentation commit 6e6c2bf Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 12:52:14 2015 -0400 move security confirmation after 'plugin already installed' check, to prevent user from answering unnecessary questions. commit 08233a2 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 05:36:42 2015 -0400 Add documentation and pluginmanager support commit 05dad86 Author: Robert Muir <rmuir@apache.org> Date: Wed Oct 14 02:22:24 2015 -0400 Decentralize plugin permissions (modulo docs and pluginmanager work)
1 parent 7368231 commit 5d001d1

File tree

26 files changed

+687
-170
lines changed

26 files changed

+687
-170
lines changed

core/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import java.security.Policy;
3030
import java.security.ProtectionDomain;
3131
import java.security.URIParameter;
32+
import java.util.Map;
3233

3334
/** custom policy for union of static and dynamic permissions */
3435
final class ESPolicy extends Policy {
@@ -41,13 +42,15 @@ final class ESPolicy extends Policy {
4142
final Policy template;
4243
final Policy untrusted;
4344
final PermissionCollection dynamic;
45+
final Map<String,PermissionCollection> plugins;
4446

45-
public ESPolicy(PermissionCollection dynamic) throws Exception {
47+
public ESPolicy(PermissionCollection dynamic, Map<String,PermissionCollection> plugins) throws Exception {
4648
URI policyUri = getClass().getResource(POLICY_RESOURCE).toURI();
4749
URI untrustedUri = getClass().getResource(UNTRUSTED_RESOURCE).toURI();
4850
this.template = Policy.getInstance("JavaPolicy", new URIParameter(policyUri));
4951
this.untrusted = Policy.getInstance("JavaPolicy", new URIParameter(untrustedUri));
5052
this.dynamic = dynamic;
53+
this.plugins = plugins;
5154
}
5255

5356
@Override @SuppressForbidden(reason = "fast equals check is desired")
@@ -66,6 +69,11 @@ public boolean implies(ProtectionDomain domain, Permission permission) {
6669
if (BootstrapInfo.UNTRUSTED_CODEBASE.equals(location.getFile())) {
6770
return untrusted.implies(domain, permission);
6871
}
72+
// check for an additional plugin permission
73+
PermissionCollection plugin = plugins.get(location.getFile());
74+
if (plugin != null && plugin.implies(permission)) {
75+
return true;
76+
}
6977
}
7078

7179
// Special handling for broken AWS code which destroys all SSL security

core/src/main/java/org/elasticsearch/bootstrap/Security.java

Lines changed: 28 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import org.elasticsearch.common.SuppressForbidden;
2323
import org.elasticsearch.env.Environment;
24+
import org.elasticsearch.plugins.PluginInfo;
2425

2526
import java.io.*;
2627
import java.net.URL;
@@ -30,8 +31,11 @@
3031
import java.nio.file.Files;
3132
import java.nio.file.NotDirectoryException;
3233
import java.nio.file.Path;
34+
import java.security.NoSuchAlgorithmException;
35+
import java.security.PermissionCollection;
3336
import java.security.Permissions;
3437
import java.security.Policy;
38+
import java.security.URIParameter;
3539
import java.util.Collections;
3640
import java.util.HashMap;
3741
import java.util.IdentityHashMap;
@@ -65,7 +69,7 @@
6569
* when they are so dangerous that general code should not be granted the
6670
* permission, but there are extenuating circumstances.
6771
* <p>
68-
* Groovy scripts are assigned no permissions. This does not provide adequate
72+
* Scripts (groovy, javascript, python) are assigned minimal permissions. This does not provide adequate
6973
* sandboxing, as these scripts still have access to ES classes, and could
7074
* modify members, etc that would cause bad things to happen later on their
7175
* behalf (no package protections are yet in place, this would need some
@@ -81,7 +85,7 @@
8185
* <h1>Debugging Security</h1>
8286
* A good place to start when there is a problem is to turn on security debugging:
8387
* <pre>
84-
* JAVA_OPTS="-Djava.security.debug=access:failure" bin/elasticsearch
88+
* JAVA_OPTS="-Djava.security.debug=access,failure" bin/elasticsearch
8589
* </pre>
8690
* See <a href="https://docs.oracle.com/javase/7/docs/technotes/guides/security/troubleshooting-security.html">
8791
* Troubleshooting Security</a> for information.
@@ -97,11 +101,9 @@ private Security() {}
97101
static void configure(Environment environment) throws Exception {
98102
// set properties for jar locations
99103
setCodebaseProperties();
100-
// set properties for problematic plugins
101-
setPluginCodebaseProperties(environment);
102104

103-
// enable security policy: union of template and environment-based paths.
104-
Policy.setPolicy(new ESPolicy(createPermissions(environment)));
105+
// enable security policy: union of template and environment-based paths, and possibly plugin permissions
106+
Policy.setPolicy(new ESPolicy(createPermissions(environment), getPluginPermissions(environment)));
105107

106108
// enable security manager
107109
System.setSecurityManager(new SecurityManager() {
@@ -157,70 +159,39 @@ static void setCodebaseProperties() {
157159
}
158160
}
159161

160-
// mapping of plugins to plugin class name. see getPluginClass why we need this.
161-
// plugin codebase property is always implicit (es.security.plugin.foobar)
162-
// note that this is only read once, when policy is parsed.
163-
static final Map<String,String> SPECIAL_PLUGINS;
164-
static {
165-
Map<String,String> m = new HashMap<>();
166-
m.put("repository-s3", "org.elasticsearch.plugin.repository.s3.S3RepositoryPlugin");
167-
m.put("discovery-ec2", "org.elasticsearch.plugin.discovery.ec2.Ec2DiscoveryPlugin");
168-
m.put("discovery-gce", "org.elasticsearch.plugin.discovery.gce.GceDiscoveryPlugin");
169-
m.put("lang-expression", "org.elasticsearch.script.expression.ExpressionPlugin");
170-
m.put("lang-groovy", "org.elasticsearch.script.groovy.GroovyPlugin");
171-
m.put("lang-javascript", "org.elasticsearch.plugin.javascript.JavaScriptPlugin");
172-
m.put("lang-python", "org.elasticsearch.plugin.python.PythonPlugin");
173-
SPECIAL_PLUGINS = Collections.unmodifiableMap(m);
174-
}
175-
176-
/**
177-
* Returns policy property for plugin, if it has special permissions.
178-
* otherwise returns null.
179-
*/
180-
static String getPluginProperty(String pluginName) {
181-
if (SPECIAL_PLUGINS.containsKey(pluginName)) {
182-
return "es.security.plugin." + pluginName;
183-
} else {
184-
return null;
185-
}
186-
}
187-
188-
/**
189-
* Returns plugin class name, if it has special permissions.
190-
* otherwise returns null.
191-
*/
192-
// this is only here to support the intellij IDE
193-
// it sucks to duplicate information, but its worth the tradeoff: sanity
194-
// if it gets out of sync, tests will fail.
195-
static String getPluginClass(String pluginName) {
196-
return SPECIAL_PLUGINS.get(pluginName);
197-
}
198-
199162
/**
200163
* Sets properties (codebase URLs) for policy files.
201164
* we look for matching plugins and set URLs to fit
202165
*/
203166
@SuppressForbidden(reason = "proper use of URL")
204-
static void setPluginCodebaseProperties(Environment environment) throws IOException {
167+
static Map<String,PermissionCollection> getPluginPermissions(Environment environment) throws IOException, NoSuchAlgorithmException {
168+
Map<String,PermissionCollection> map = new HashMap<>();
205169
if (Files.exists(environment.pluginsFile())) {
206170
try (DirectoryStream<Path> stream = Files.newDirectoryStream(environment.pluginsFile())) {
207171
for (Path plugin : stream) {
208-
String prop = getPluginProperty(plugin.getFileName().toString());
209-
if (prop != null) {
210-
if (System.getProperty(prop) != null) {
211-
throw new IllegalStateException("property: " + prop + " is unexpectedly set: " + System.getProperty(prop));
172+
Path policyFile = plugin.resolve(PluginInfo.ES_PLUGIN_POLICY);
173+
if (Files.exists(policyFile)) {
174+
// parse the plugin's policy file into a set of permissions
175+
Policy policy = Policy.getInstance("JavaPolicy", new URIParameter(policyFile.toUri()));
176+
PermissionCollection permissions = policy.getPermissions(Security.class.getProtectionDomain());
177+
// this method is supported with the specific implementation we use, but just check for safety.
178+
if (permissions == Policy.UNSUPPORTED_EMPTY_COLLECTION) {
179+
throw new UnsupportedOperationException("JavaPolicy implementation does not support retrieving permissions");
180+
}
181+
// grant the permissions to each jar in the plugin
182+
try (DirectoryStream<Path> jarStream = Files.newDirectoryStream(plugin, "*.jar")) {
183+
for (Path jar : jarStream) {
184+
if (map.put(jar.toUri().toURL().getFile(), permissions) != null) {
185+
// just be paranoid ok?
186+
throw new IllegalStateException("per-plugin permissions already granted for jar file: " + jar);
187+
}
188+
}
212189
}
213-
System.setProperty(prop, plugin.toUri().toURL().toString() + "*");
214190
}
215191
}
216192
}
217193
}
218-
for (String plugin : SPECIAL_PLUGINS.keySet()) {
219-
String prop = getPluginProperty(plugin);
220-
if (System.getProperty(prop) == null) {
221-
System.setProperty(prop, "file:/dev/null"); // no chance to be interpreted as "all"
222-
}
223-
}
194+
return Collections.unmodifiableMap(map);
224195
}
225196

226197
/** returns dynamic Permissions to configured paths */

core/src/main/java/org/elasticsearch/plugins/PluginInfo.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
public class PluginInfo implements Streamable, ToXContent {
3737

3838
public static final String ES_PLUGIN_PROPERTIES = "plugin-descriptor.properties";
39+
public static final String ES_PLUGIN_POLICY = "plugin-security.policy";
3940

4041
static final class Fields {
4142
static final XContentBuilderString NAME = new XContentBuilderString("name");

core/src/main/java/org/elasticsearch/plugins/PluginManager.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ public PluginManager(Environment environment, URL url, OutputMode outputMode, Ti
100100
this.timeout = timeout;
101101
}
102102

103-
public void downloadAndExtract(String name, Terminal terminal) throws IOException {
103+
public void downloadAndExtract(String name, Terminal terminal, boolean batch) throws IOException {
104104
if (name == null && url == null) {
105105
throw new IllegalArgumentException("plugin name or url must be supplied with install.");
106106
}
@@ -124,7 +124,7 @@ public void downloadAndExtract(String name, Terminal terminal) throws IOExceptio
124124
}
125125

126126
Path pluginFile = download(pluginHandle, terminal);
127-
extract(pluginHandle, terminal, pluginFile);
127+
extract(pluginHandle, terminal, pluginFile, batch);
128128
}
129129

130130
private Path download(PluginHandle pluginHandle, Terminal terminal) throws IOException {
@@ -207,7 +207,7 @@ private Path download(PluginHandle pluginHandle, Terminal terminal) throws IOExc
207207
return pluginFile;
208208
}
209209

210-
private void extract(PluginHandle pluginHandle, Terminal terminal, Path pluginFile) throws IOException {
210+
private void extract(PluginHandle pluginHandle, Terminal terminal, Path pluginFile, boolean batch) throws IOException {
211211
// unzip plugin to a staging temp dir, named for the plugin
212212
Path tmp = Files.createTempDirectory(environment.tmpFile(), null);
213213
Path root = tmp.resolve(pluginHandle.name);
@@ -232,6 +232,13 @@ private void extract(PluginHandle pluginHandle, Terminal terminal, Path pluginFi
232232
throw new IOException("plugin directory " + extractLocation.toAbsolutePath() + " already exists. To update the plugin, uninstall it first using 'remove " + pluginHandle.name + "' command");
233233
}
234234

235+
// read optional security policy (extra permissions)
236+
// if it exists, confirm or warn the user
237+
Path policy = root.resolve(PluginInfo.ES_PLUGIN_POLICY);
238+
if (Files.exists(policy)) {
239+
PluginSecurity.readPolicy(policy, terminal, environment, batch);
240+
}
241+
235242
// install plugin
236243
FileSystemUtils.copyDirectoryRecursively(root, extractLocation);
237244
terminal.println("Installed %s into %s", pluginHandle.name, extractLocation.toAbsolutePath());
@@ -335,7 +342,7 @@ private static void setPosixFileAttributes(Path path, UserPrincipal owner, Group
335342
fileAttributeView.setPermissions(permissions);
336343
}
337344

338-
private void tryToDeletePath(Terminal terminal, Path ... paths) {
345+
static void tryToDeletePath(Terminal terminal, Path ... paths) {
339346
for (Path path : paths) {
340347
try {
341348
IOUtils.rm(path);

core/src/main/java/org/elasticsearch/plugins/PluginManagerCliParser.java

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,7 @@ static class Install extends Command {
180180

181181
private static final CliToolConfig.Cmd CMD = cmd(NAME, Install.class)
182182
.options(option("t", "timeout").required(false).hasArg(false))
183+
.options(option("b", "batch").required(false))
183184
.build();
184185

185186
static Command parse(Terminal terminal, CommandLine cli) {
@@ -210,21 +211,28 @@ static Command parse(Terminal terminal, CommandLine cli) {
210211
if (cli.hasOption("v")) {
211212
outputMode = OutputMode.VERBOSE;
212213
}
214+
215+
boolean batch = System.console() == null;
216+
if (cli.hasOption("b")) {
217+
batch = true;
218+
}
213219

214-
return new Install(terminal, name, outputMode, optionalPluginUrl, timeout);
220+
return new Install(terminal, name, outputMode, optionalPluginUrl, timeout, batch);
215221
}
216222

217223
final String name;
218224
private OutputMode outputMode;
219225
final URL url;
220226
final TimeValue timeout;
227+
final boolean batch;
221228

222-
Install(Terminal terminal, String name, OutputMode outputMode, URL url, TimeValue timeout) {
229+
Install(Terminal terminal, String name, OutputMode outputMode, URL url, TimeValue timeout, boolean batch) {
223230
super(terminal);
224231
this.name = name;
225232
this.outputMode = outputMode;
226233
this.url = url;
227234
this.timeout = timeout;
235+
this.batch = batch;
228236
}
229237

230238
@Override
@@ -235,7 +243,7 @@ public ExitStatus execute(Settings settings, Environment env) throws Exception {
235243
} else {
236244
terminal.println("-> Installing from " + URLDecoder.decode(url.toString(), "UTF-8") + "...");
237245
}
238-
pluginManager.downloadAndExtract(name, terminal);
246+
pluginManager.downloadAndExtract(name, terminal, batch);
239247
return ExitStatus.OK;
240248
}
241249
}

0 commit comments

Comments
 (0)