honeymoose/OpenSearch
1
0
Fork 0
mirror of https://github.com/honeymoose/OpenSearch.git synced 2025-03-25 09:28:27 +00:00
Code Issues Packages Projects Releases Wiki Activity

Add data.path fast path for FilePermission (#61302)

Browse Source 
The recursive data.path FilePermission check is an extremely hot
codepath in Elasticsearch. Unfortunately the FilePermission check in
Java is extremely allocation heavy. As it iterates through different
file permissions, it allocates byte arrays for each Path component that
must be compared. This PR improves the situation by adding the recursive
data.path FilePermission it its own PermissionsCollection object which
is checked first.
This commit is contained in:
Tim Brooks 2020-08-21 17:38:40 -06:00
parent fd976e668c
commit e573fa9abc
No known key found for this signature in database
GPG Key ID: C2AA3BB91A889E77
7 changed files with 66 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
plugins/ingest-attachment/src/main/java/org/elasticsearch/ingest/attachment/TikaImpl.java
View File

@ -150,8 +150,8 @@ final class TikaImpl {
addReadPermissions(perms, set);
}
// jvm's java.io.tmpdir (needs read/write)
FilePermissionUtils.addDirectoryPath(perms, "java.io.tmpdir",
PathUtils.get(System.getProperty("java.io.tmpdir")), "read,readlink,write,delete");
FilePermissionUtils.addDirectoryPath(perms, "java.io.tmpdir", PathUtils.get(System.getProperty("java.io.tmpdir")),
"read,readlink,write,delete", false);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
@ -181,7 +181,7 @@ final class TikaImpl {
for (URL url : resources) {
Path path = PathUtils.get(url.toURI());
if (Files.isDirectory(path)) {
FilePermissionUtils.addDirectoryPath(perms, "class.path", path, "read,readlink");
FilePermissionUtils.addDirectoryPath(perms, "class.path", path, "read,readlink", false);
} else {
FilePermissionUtils.addSingleFilePath(perms, path, "read,readlink");
}

17
qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/ESPolicyUnitTests.java
View File

@ -52,7 +52,7 @@ public class ESPolicyUnitTests extends ESTestCase {
Permission all = new AllPermission();
PermissionCollection allCollection = all.newPermissionCollection();
allCollection.add(all);
ESPolicy policy = new ESPolicy(Collections.emptyMap(), allCollection, Collections.emptyMap(), true);
ESPolicy policy = new ESPolicy(Collections.emptyMap(), allCollection, Collections.emptyMap(), true, new Permissions());
// restrict ourselves to NoPermission
PermissionCollection noPermissions = new Permissions();
assertFalse(policy.implies(new ProtectionDomain(null, noPermissions), new FilePermission("foo", "read")));
@ -67,7 +67,7 @@ public class ESPolicyUnitTests extends ESTestCase {
public void testNullLocation() throws Exception {
assumeTrue("test cannot run with security manager", System.getSecurityManager() == null);
PermissionCollection noPermissions = new Permissions();
ESPolicy policy = new ESPolicy(Collections.emptyMap(), noPermissions, Collections.emptyMap(), true);
ESPolicy policy = new ESPolicy(Collections.emptyMap(), noPermissions, Collections.emptyMap(), true, new Permissions());
assertFalse(policy.implies(new ProtectionDomain(new CodeSource(null, (Certificate[]) null), noPermissions),
new FilePermission("foo", "read")));
}
@ -75,11 +75,22 @@ public class ESPolicyUnitTests extends ESTestCase {
public void testListen() {
assumeTrue("test cannot run with security manager", System.getSecurityManager() == null);
final PermissionCollection noPermissions = new Permissions();
final ESPolicy policy = new ESPolicy(Collections.emptyMap(), noPermissions, Collections.emptyMap(), true);
final ESPolicy policy = new ESPolicy(Collections.emptyMap(), noPermissions, Collections.emptyMap(), true, new Permissions());
assertFalse(
policy.implies(
new ProtectionDomain(ESPolicyUnitTests.class.getProtectionDomain().getCodeSource(), noPermissions),
new SocketPermission("localhost:" + randomFrom(0, randomIntBetween(49152, 65535)), "listen")));
}
@SuppressForbidden(reason = "to create FilePermission object")
public void testDataPathPermissionIsChecked() {
assumeTrue("test cannot run with security manager", System.getSecurityManager() == null);
final PermissionCollection dataPathPermission = new Permissions();
dataPathPermission.add(new FilePermission("/home/elasticsearch/data/-", "read"));
final ESPolicy policy = new ESPolicy(Collections.emptyMap(), new Permissions(), Collections.emptyMap(), true, dataPathPermission);
assertTrue(
policy.implies(
new ProtectionDomain(new CodeSource(null, (Certificate[]) null), new Permissions()),
new FilePermission("/home/elasticsearch/data/index/file.si", "read")));
}
}

2
qa/evil-tests/src/test/java/org/elasticsearch/bootstrap/EvilSecurityTests.java
View File

@ -216,7 +216,7 @@ public class EvilSecurityTests extends ESTestCase {
assumeNoException("test cannot create symbolic links with security manager enabled", e);
}
Permissions permissions = new Permissions();
FilePermissionUtils.addDirectoryPath(permissions, "testing", link, "read");
FilePermissionUtils.addDirectoryPath(permissions, "testing", link, "read", false);
assertExactPermissions(new FilePermission(link.toString(), "read"), permissions);
assertExactPermissions(new FilePermission(link.resolve("foo").toString(), "read"), permissions);
assertExactPermissions(new FilePermission(target.toString(), "read"), permissions);

11
server/src/main/java/org/elasticsearch/bootstrap/ESPolicy.java
View File

@ -47,10 +47,13 @@ final class ESPolicy extends Policy {
final Policy untrusted;
final Policy system;
final PermissionCollection dynamic;
final PermissionCollection dataPathPermission;
final Map<String,Policy> plugins;
ESPolicy(Map<String, URL> codebases, PermissionCollection dynamic, Map<String,Policy> plugins, boolean filterBadDefaults) {
ESPolicy(Map<String, URL> codebases, PermissionCollection dynamic, Map<String,Policy> plugins, boolean filterBadDefaults,
PermissionCollection dataPathPermission) {
this.template = Security.readPolicy(getClass().getResource(POLICY_RESOURCE), codebases);
this.dataPathPermission = dataPathPermission;
this.untrusted = Security.readPolicy(getClass().getResource(UNTRUSTED_RESOURCE), Collections.emptyMap());
if (filterBadDefaults) {
this.system = new SystemPolicy(Policy.getPolicy());
@ -98,6 +101,12 @@ final class ESPolicy extends Policy {
}
}
// The FilePermission to check access to the path.data is the hottest permission check in
// Elasticsearch, so we check it first.
if (permission instanceof FilePermission && dataPathPermission.implies(permission)) {
return true;
}
// otherwise defer to template + dynamic file permissions
return template.implies(domain, permission) || dynamic.implies(permission) || system.implies(domain, permission);
}

21
server/src/main/java/org/elasticsearch/bootstrap/FilePermissionUtils.java
View File

@ -54,15 +54,17 @@ public class FilePermissionUtils {
}
/**
* Add access to path (and all files underneath it); this also creates the directory if it does not exist.
* Add access to path with direct and/or recursive access. This also creates the directory if it does not exist.
*
* @param policy current policy to add permissions to
* @param configurationName the configuration name associated with the path (for error messages only)
* @param path the path itself
* @param permissions set of file permissions to grant to the path
* @param recursiveAccessOnly indicates if the permission should provide recursive access to files underneath
*/
@SuppressForbidden(reason = "only place where creating Java-9 compatible FilePermission objects is possible")
public static void addDirectoryPath(Permissions policy, String configurationName, Path path, String permissions) throws IOException {
public static void addDirectoryPath(Permissions policy, String configurationName, Path path, String permissions,
boolean recursiveAccessOnly) throws IOException {
// paths may not exist yet, this also checks accessibility
try {
Security.ensureDirectoryExists(path);
@ -70,8 +72,12 @@ public class FilePermissionUtils {
throw new IllegalStateException("Unable to access '" + configurationName + "' (" + path + ")", e);
}
// add each path twice: once for itself, again for files underneath it
policy.add(new FilePermission(path.toString(), permissions));
// For some file permissions (data.path) we create a Permissions object that only checks the concrete
// path. Adding the directory would only create more overhead for this fast path.
if (recursiveAccessOnly == false) {
// add access for path itself
policy.add(new FilePermission(path.toString(), permissions));
}
policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions));
/*
* The file permission model since JDK 9 requires this due to the removal of pathname canonicalization. See also
@ -79,9 +85,12 @@ public class FilePermissionUtils {
*/
final Path realPath = path.toRealPath();
if (path.toString().equals(realPath.toString()) == false) {
policy.add(new FilePermission(realPath.toString(), permissions));
if (recursiveAccessOnly == false) {
// add access for path itself
policy.add(new FilePermission(realPath.toString(), permissions));
}
// add access for files underneath
policy.add(new FilePermission(realPath.toString() + realPath.getFileSystem().getSeparator() + "-", permissions));
}
}
}

33
server/src/main/java/org/elasticsearch/bootstrap/Security.java
View File

@ -118,7 +118,8 @@ final class Security {
// enable security policy: union of template and environment-based paths, and possibly plugin permissions
Map<String, URL> codebases = getCodebaseJarMap(JarHell.parseClassPath());
Policy.setPolicy(new ESPolicy(codebases, createPermissions(environment), getPluginPermissions(environment), filterBadDefaults));
Policy.setPolicy(new ESPolicy(codebases, createPermissions(environment), getPluginPermissions(environment), filterBadDefaults,
createRecursiveDataPathPermission(environment)));
// enable security manager
final String[] classesThatCanExit =
@ -254,6 +255,14 @@ final class Security {
return policy;
}
private static Permissions createRecursiveDataPathPermission(Environment environment) throws IOException {
Permissions policy = new Permissions();
for (Path path : environment.dataFiles()) {
addDirectoryPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete", true);
}
return policy;
}
/** Adds access to classpath jars/classes for jar hell scan, etc */
@SuppressForbidden(reason = "accesses fully qualified URLs to configure security")
static void addClasspathPermissions(Permissions policy) throws IOException {
@ -268,7 +277,7 @@ final class Security {
}
// resource itself
if (Files.isDirectory(path)) {
addDirectoryPath(policy, "class.path", path, "read,readlink");
addDirectoryPath(policy, "class.path", path, "read,readlink", false);
} else {
addSingleFilePath(policy, path, "read,readlink");
}
@ -280,21 +289,21 @@ final class Security {
*/
static void addFilePermissions(Permissions policy, Environment environment) throws IOException {
// read-only dirs
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.binFile(), "read,readlink");
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.libFile(), "read,readlink");
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.modulesFile(), "read,readlink");
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink");
addDirectoryPath(policy, "path.conf'", environment.configFile(), "read,readlink");
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.binFile(), "read,readlink", false);
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.libFile(), "read,readlink", false);
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.modulesFile(), "read,readlink", false);
addDirectoryPath(policy, Environment.PATH_HOME_SETTING.getKey(), environment.pluginsFile(), "read,readlink", false);
addDirectoryPath(policy, "path.conf'", environment.configFile(), "read,readlink", false);
// read-write dirs
addDirectoryPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete");
addDirectoryPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete");
addDirectoryPath(policy, "java.io.tmpdir", environment.tmpFile(), "read,readlink,write,delete", false);
addDirectoryPath(policy, Environment.PATH_LOGS_SETTING.getKey(), environment.logsFile(), "read,readlink,write,delete", false);
if (environment.sharedDataFile() != null) {
addDirectoryPath(policy, Environment.PATH_SHARED_DATA_SETTING.getKey(), environment.sharedDataFile(),
"read,readlink,write,delete");
"read,readlink,write,delete", false);
}
final Set<Path> dataFilesPaths = new HashSet<>();
for (Path path : environment.dataFiles()) {
addDirectoryPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete");
addDirectoryPath(policy, Environment.PATH_DATA_SETTING.getKey(), path, "read,readlink,write,delete", false);
/*
* We have to do this after adding the path because a side effect of that is that the directory is created; the Path#toRealPath
* invocation will fail if the directory does not already exist. We use Path#toRealPath to follow symlinks and handle issues
@ -310,7 +319,7 @@ final class Security {
}
}
for (Path path : environment.repoFiles()) {
addDirectoryPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete");
addDirectoryPath(policy, Environment.PATH_REPO_SETTING.getKey(), path, "read,readlink,write,delete", false);
}
if (environment.pidFile() != null) {
// we just need permission to remove the file if its elsewhere.

4
test/framework/src/main/java/org/elasticsearch/bootstrap/BootstrapForTesting.java
View File

@ -109,7 +109,7 @@ public class BootstrapForTesting {
Permissions perms = new Permissions();
Security.addClasspathPermissions(perms);
// java.io.tmpdir
FilePermissionUtils.addDirectoryPath(perms, "java.io.tmpdir", javaTmpDir, "read,readlink,write,delete");
FilePermissionUtils.addDirectoryPath(perms, "java.io.tmpdir", javaTmpDir, "read,readlink,write,delete", false);
// custom test config file
if (Strings.hasLength(System.getProperty("tests.config"))) {
FilePermissionUtils.addSingleFilePath(perms, PathUtils.get(System.getProperty("tests.config")), "read,readlink");
@ -147,7 +147,7 @@ public class BootstrapForTesting {
addClassCodebase(codebases, "elasticsearch-rest-client", "org.elasticsearch.client.RestClient");
}
final Policy testFramework = Security.readPolicy(Bootstrap.class.getResource("test-framework.policy"), codebases);
final Policy esPolicy = new ESPolicy(codebases, perms, getPluginPermissions(), true);
final Policy esPolicy = new ESPolicy(codebases, perms, getPluginPermissions(), true, new Permissions());
Policy.setPolicy(new Policy() {
@Override
public boolean implies(ProtectionDomain domain, Permission permission) {