From 2cea5f2b3822c5eecef75356c1e83580fbc59db4 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Tue, 12 May 2015 00:20:52 -0400 Subject: [PATCH 1/3] Improve path management on init: * Properly support symlinks (e.g. /tmp -> /mnt/tmp) * Check all configured paths up front and deliver the best exception we can when things are wrong * Initialize securitymanager earlier * Fix too-loud error logging of Natives root check --- .../elasticsearch/bootstrap/Bootstrap.java | 11 ++-- .../org/elasticsearch/bootstrap/Security.java | 40 +++++++++-- .../org/elasticsearch/common/jna/Natives.java | 6 +- .../bootstrap/SecurityTests.java | 66 +++++++++++++++++++ 4 files changed, 111 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java b/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java index e4a86baf9ab..cda029f84f7 100644 --- a/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java +++ b/src/main/java/org/elasticsearch/bootstrap/Bootstrap.java @@ -58,7 +58,7 @@ public class Bootstrap { private static volatile Bootstrap INSTANCE; - private Node node; + private volatile Node node; private final CountDownLatch keepAliveLatch = new CountDownLatch(1); private final Thread keepAliveThread; @@ -144,19 +144,22 @@ public class Bootstrap { initializeNatives(settings.getAsBoolean("bootstrap.mlockall", false), settings.getAsBoolean("bootstrap.ctrlhandler", true)); - NodeBuilder nodeBuilder = NodeBuilder.nodeBuilder().settings(settings).loadConfigSettings(false); - node = nodeBuilder.build(); if (addShutdownHook) { Runtime.getRuntime().addShutdownHook(new Thread() { @Override public void run() { - node.close(); + if (node != null) { + node.close(); + } } }); } // install SM after natives, shutdown hooks, etc. setupSecurity(settings, environment); + + NodeBuilder nodeBuilder = NodeBuilder.nodeBuilder().settings(settings).loadConfigSettings(false); + node = nodeBuilder.build(); } /** diff --git a/src/main/java/org/elasticsearch/bootstrap/Security.java b/src/main/java/org/elasticsearch/bootstrap/Security.java index 7874f8002b0..dafc82fc02c 100644 --- a/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -23,7 +23,10 @@ import org.elasticsearch.common.SuppressForbidden; import org.elasticsearch.env.Environment; import java.io.*; +import java.nio.file.AccessMode; +import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; +import java.nio.file.NotDirectoryException; import java.nio.file.Path; import java.security.Permissions; import java.security.Policy; @@ -76,20 +79,47 @@ public class Security { /** Add access to path (and all files underneath it */ public static void addPath(Permissions policy, Path path, String permissions) throws IOException { // paths may not exist yet - Files.createDirectories(path); + ensureDirectoryExists(path); + // add each path twice: once for itself, again for files underneath it policy.add(new FilePermission(path.toString(), permissions)); policy.add(new FilePermission(path.toString() + path.getFileSystem().getSeparator() + "-", permissions)); } + + /** + * Ensures configured directory {@code path} exists. + * @throws IOException if {@code path} exists, but is not a directory, not accessible, or broken symbolic link. + */ + static void ensureDirectoryExists(Path path) throws IOException { + // this isn't atomic, but neither is createDirectories. + if (Files.isDirectory(path)) { + // verify access, following links (throws exception if something is wrong) + // we only check READ as a sanity test + path.getFileSystem().provider().checkAccess(path.toRealPath(), AccessMode.READ); + } else { + // doesn't exist, or not a directory + try { + Files.createDirectories(path); + } catch (FileAlreadyExistsException e) { + // convert optional specific exception so the context is clear + IOException e2 = new NotDirectoryException(path.toString()); + e2.addSuppressed(e); + throw e2; + } + } + } /** Simple checks that everything is ok */ @SuppressForbidden(reason = "accesses jvm default tempdir as a self-test") - public static void selfTest() { + public static void selfTest() throws IOException { // check we can manipulate temporary files try { - Files.delete(Files.createTempFile(null, null)); - } catch (IOException ignored) { - // potentially virus scanner + Path p = Files.createTempFile(null, null); + try { + Files.delete(p); + } catch (IOException ignored) { + // potentially virus scanner + } } catch (SecurityException problem) { throw new SecurityException("Security misconfiguration: cannot access java.io.tmpdir", problem); } diff --git a/src/main/java/org/elasticsearch/common/jna/Natives.java b/src/main/java/org/elasticsearch/common/jna/Natives.java index 69f412bda6c..fa8e074713a 100644 --- a/src/main/java/org/elasticsearch/common/jna/Natives.java +++ b/src/main/java/org/elasticsearch/common/jna/Natives.java @@ -72,9 +72,9 @@ public class Natives { } try { return CLibrary.geteuid() == 0; - } catch (Throwable error) { - logger.warn("unable to determine euid", error); - return false; // don't know + } catch (UnsatisfiedLinkError e) { + // this will have already been logged by Kernel32Library, no need to repeat it + return false; } } diff --git a/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java b/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java index f50342b4e9e..1bf833ac25c 100644 --- a/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java +++ b/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java @@ -25,6 +25,8 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.test.ElasticsearchTestCase; import java.io.FilePermission; +import java.io.IOException; +import java.nio.file.Files; import java.nio.file.Path; import java.security.Permissions; @@ -110,4 +112,68 @@ public class SecurityTests extends ElasticsearchTestCase { // PID file: r/w assertTrue(permissions.implies(new FilePermission(environment.pidFile().toString(), "read,readlink,write,delete"))); } + + public void testEnsureExists() throws IOException { + Path p = createTempDir(); + + // directory exists + Path exists = p.resolve("exists"); + Files.createDirectory(exists); + Security.ensureDirectoryExists(exists); + Files.createTempFile(exists, null, null); + } + + public void testEnsureNotExists() throws IOException { + Path p = createTempDir(); + + // directory does not exist: create it + Path notExists = p.resolve("notexists"); + Security.ensureDirectoryExists(notExists); + Files.createTempFile(notExists, null, null); + } + + public void testEnsureRegularFile() throws IOException { + Path p = createTempDir(); + + // regular file + Path regularFile = p.resolve("regular"); + Files.createFile(regularFile); + try { + Security.ensureDirectoryExists(regularFile); + fail("didn't get expected exception"); + } catch (IOException expected) {} + } + + public void testEnsureSymlink() throws IOException { + Path p = createTempDir(); + + Path exists = p.resolve("exists"); + Files.createDirectory(exists); + + // symlink + Path linkExists = p.resolve("linkExists"); + try { + Files.createSymbolicLink(linkExists, exists); + } catch (UnsupportedOperationException e) { + assumeNoException("test requires filesystem that supports symbolic links", e); + } + Security.ensureDirectoryExists(linkExists); + Files.createTempFile(linkExists, null, null); + } + + public void testEnsureBrokenSymlink() throws IOException { + Path p = createTempDir(); + + // broken symlink + Path brokenLink = p.resolve("brokenLink"); + try { + Files.createSymbolicLink(brokenLink, p.resolve("nonexistent")); + } catch (UnsupportedOperationException e) { + assumeNoException("test requires filesystem that supports symbolic links", e); + } + try { + Security.ensureDirectoryExists(brokenLink); + fail("didn't get expected exception"); + } catch (IOException expected) {} + } } From 019a9410a355f085b28a03ef623a0cb5c9ab06b3 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Tue, 12 May 2015 00:34:02 -0400 Subject: [PATCH 2/3] SecurityBootstrap -> BootstrapForTesting, and make less things public --- pom.xml | 8 ++++---- src/main/java/org/elasticsearch/bootstrap/Security.java | 6 +++--- .../BootstrapForTesting.java} | 9 +++++---- .../org/elasticsearch/test/ElasticsearchTestCase.java | 4 +++- .../test/ElasticsearchTokenStreamTestCase.java | 3 ++- 5 files changed, 17 insertions(+), 13 deletions(-) rename src/test/java/org/elasticsearch/{test/SecurityBootstrap.java => bootstrap/BootstrapForTesting.java} (93%) diff --git a/pom.xml b/pom.xml index 2b6ff1ccab0..dbd6be657a6 100644 --- a/pom.xml +++ b/pom.xml @@ -774,7 +774,9 @@ org/elasticsearch/test/**/* - org/apache/lucene/util/AbstractRandomizedTest.java + org/elasticsearch/bootstrap/BootstrapForTesting.class + org/elasticsearch/common/cli/CliToolTestCase.class + org/elasticsearch/common/cli/CliToolTestCase$*.class @@ -1535,9 +1537,7 @@ org/elasticsearch/test/**/* - org/elasticsearch/cache/recycler/MockPageCacheRecycler.class - org/apache/lucene/util/AbstractRandomizedTest.class - org/apache/lucene/util/AbstractRandomizedTest$*.class + org/elasticsearch/bootstrap/BootstrapForTesting.class org/elasticsearch/common/cli/CliToolTestCase.class org/elasticsearch/common/cli/CliToolTestCase$*.class diff --git a/src/main/java/org/elasticsearch/bootstrap/Security.java b/src/main/java/org/elasticsearch/bootstrap/Security.java index dafc82fc02c..4dccd1843e1 100644 --- a/src/main/java/org/elasticsearch/bootstrap/Security.java +++ b/src/main/java/org/elasticsearch/bootstrap/Security.java @@ -37,7 +37,7 @@ import java.security.Policy; * We use a template file (the one we test with), and add additional * permissions based on the environment (data paths, etc) */ -public class Security { +class Security { /** * Initializes securitymanager for the environment @@ -77,7 +77,7 @@ public class Security { } /** Add access to path (and all files underneath it */ - public static void addPath(Permissions policy, Path path, String permissions) throws IOException { + static void addPath(Permissions policy, Path path, String permissions) throws IOException { // paths may not exist yet ensureDirectoryExists(path); @@ -111,7 +111,7 @@ public class Security { /** Simple checks that everything is ok */ @SuppressForbidden(reason = "accesses jvm default tempdir as a self-test") - public static void selfTest() throws IOException { + static void selfTest() throws IOException { // check we can manipulate temporary files try { Path p = Files.createTempFile(null, null); diff --git a/src/test/java/org/elasticsearch/test/SecurityBootstrap.java b/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java similarity index 93% rename from src/test/java/org/elasticsearch/test/SecurityBootstrap.java rename to src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java index 21ad1e7ac19..42eb82db4f2 100644 --- a/src/test/java/org/elasticsearch/test/SecurityBootstrap.java +++ b/src/test/java/org/elasticsearch/bootstrap/BootstrapForTesting.java @@ -17,7 +17,7 @@ * under the License. */ -package org.elasticsearch.test; +package org.elasticsearch.bootstrap; import org.apache.lucene.util.TestSecurityManager; import org.elasticsearch.bootstrap.Bootstrap; @@ -33,13 +33,14 @@ import java.util.Objects; import static com.carrotsearch.randomizedtesting.RandomizedTest.systemPropertyAsBoolean; /** - * Installs test security manager (ensures it happens regardless of which + * Initializes natives and installs test security manager + * (init'd early by base classes to ensure it happens regardless of which * test case happens to be first, test ordering, etc). *

* The idea is to mimic as much as possible what happens with ES in production * mode (e.g. assign permissions and install security manager the same way) */ -class SecurityBootstrap { +public class BootstrapForTesting { // TODO: can we share more code with the non-test side here // without making things complex??? @@ -77,5 +78,5 @@ class SecurityBootstrap { } // does nothing, just easy way to make sure the class is loaded. - static void ensureInitialized() {} + public static void ensureInitialized() {} } diff --git a/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java b/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java index a56db25f075..299a60a9715 100644 --- a/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java +++ b/src/test/java/org/elasticsearch/test/ElasticsearchTestCase.java @@ -30,12 +30,14 @@ import com.carrotsearch.randomizedtesting.generators.RandomPicks; import com.carrotsearch.randomizedtesting.generators.RandomStrings; import com.carrotsearch.randomizedtesting.rules.TestRuleAdapter; import com.google.common.base.Predicate; + import org.apache.lucene.uninverting.UninvertingReader; import org.apache.lucene.util.LuceneTestCase; import org.apache.lucene.util.LuceneTestCase.SuppressCodecs; import org.apache.lucene.util.TestUtil; import org.apache.lucene.util.TimeUnits; import org.elasticsearch.Version; +import org.elasticsearch.bootstrap.BootstrapForTesting; import org.elasticsearch.client.Requests; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.cluster.routing.DjbHashFunction; @@ -93,7 +95,7 @@ import static com.google.common.collect.Lists.newArrayList; public abstract class ElasticsearchTestCase extends LuceneTestCase { static { - SecurityBootstrap.ensureInitialized(); + BootstrapForTesting.ensureInitialized(); } protected final ESLogger logger = Loggers.getLogger(getClass()); diff --git a/src/test/java/org/elasticsearch/test/ElasticsearchTokenStreamTestCase.java b/src/test/java/org/elasticsearch/test/ElasticsearchTokenStreamTestCase.java index 8374472dba8..e5fd76b518a 100644 --- a/src/test/java/org/elasticsearch/test/ElasticsearchTokenStreamTestCase.java +++ b/src/test/java/org/elasticsearch/test/ElasticsearchTokenStreamTestCase.java @@ -26,6 +26,7 @@ import org.apache.lucene.analysis.BaseTokenStreamTestCase; import org.apache.lucene.util.LuceneTestCase; import org.apache.lucene.util.TimeUnits; import org.elasticsearch.Version; +import org.elasticsearch.bootstrap.BootstrapForTesting; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.settings.ImmutableSettings; import org.elasticsearch.test.junit.listeners.ReproduceInfoPrinter; @@ -43,7 +44,7 @@ import org.elasticsearch.test.junit.listeners.ReproduceInfoPrinter; public abstract class ElasticsearchTokenStreamTestCase extends BaseTokenStreamTestCase { static { - SecurityBootstrap.ensureInitialized(); + BootstrapForTesting.ensureInitialized(); } public static Version randomVersion() { From d1defef69d245daf8a139c2498cce1dde5b75b9c Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Tue, 12 May 2015 00:53:51 -0400 Subject: [PATCH 3/3] Properly handle the case where symlinks are supported, but the user is not a windows administrator (can throw IOE in this case) --- src/test/java/org/elasticsearch/bootstrap/SecurityTests.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java b/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java index 1bf833ac25c..a0b2f420330 100644 --- a/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java +++ b/src/test/java/org/elasticsearch/bootstrap/SecurityTests.java @@ -154,7 +154,7 @@ public class SecurityTests extends ElasticsearchTestCase { Path linkExists = p.resolve("linkExists"); try { Files.createSymbolicLink(linkExists, exists); - } catch (UnsupportedOperationException e) { + } catch (UnsupportedOperationException | IOException e) { assumeNoException("test requires filesystem that supports symbolic links", e); } Security.ensureDirectoryExists(linkExists); @@ -168,7 +168,7 @@ public class SecurityTests extends ElasticsearchTestCase { Path brokenLink = p.resolve("brokenLink"); try { Files.createSymbolicLink(brokenLink, p.resolve("nonexistent")); - } catch (UnsupportedOperationException e) { + } catch (UnsupportedOperationException | IOException e) { assumeNoException("test requires filesystem that supports symbolic links", e); } try {