From 2cea5f2b3822c5eecef75356c1e83580fbc59db4 Mon Sep 17 00:00:00 2001 From: Robert Muir Date: Tue, 12 May 2015 00:20:52 -0400 Subject: [PATCH] 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) {} + } }