From 2bf8f4b0bc6ba2eece5833a59b989b12d7f86138 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Wed, 16 Aug 2017 12:50:39 -0400 Subject: [PATCH] Remove print writer wrapping for users tools When writing the users and users_roles files, we wrap a custom writer in a print writer. There is a problem with this though: when print writer closes it closes our underlying custom writer and the close implementation for our custom writer is not trivial, it executes code that can throw an I/O exception. When print writer invokes this close and an I/O exception is thrown, it swallows that exception and sets the status on the print writer to error. One would think that we could simply check this status but alas print writer is broken here. The act of checking the status causes print writer to try to flush the underyling stream which is going to be completely undefined because the underlying stream might or might not be closed. This might cause another exception to be thrown, losing the original. Print writer screwed the pooch here, there is no good reason to try to do any I/O after the underlying writer entered a failed state. To address this we remove the use of print writer, we use our custom writer directly. This allows any thrown exceptions to bubble up. Relates elastic/x-pack-elasticsearch#2288 Original commit: elastic/x-pack-elasticsearch@11b8dd56412c22bd0906f890bbb9b0b122620ead --- .../security/authc/file/FileUserPasswdStore.java | 8 +++++--- .../security/authc/file/FileUserRolesStore.java | 12 ++++++++---- .../xpack/security/support/SecurityFilesTests.java | 7 +++---- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java b/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java index 3b5246e3aed..715b19aab42 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserPasswdStore.java @@ -27,7 +27,7 @@ import org.elasticsearch.xpack.security.support.Validation.Users; import org.elasticsearch.xpack.security.user.User; import java.io.IOException; -import java.io.PrintWriter; +import java.io.Writer; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -169,9 +169,11 @@ public class FileUserPasswdStore { } public static void writeFile(Map users, Path path) { - try (PrintWriter writer = new PrintWriter(openAtomicMoveWriter(path))) { + try (Writer writer = openAtomicMoveWriter(path)) { for (Map.Entry entry : users.entrySet()) { - writer.printf(Locale.ROOT, "%s:%s%s", entry.getKey(), new String(entry.getValue()), System.lineSeparator()); + final String message = + String.format(Locale.ROOT, "%s:%s%s", entry.getKey(), new String(entry.getValue()), System.lineSeparator()); + writer.write(message); } } catch (IOException ioe) { throw new ElasticsearchException("could not write file [{}], please check file permissions", ioe, path.toAbsolutePath()); diff --git a/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java b/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java index af8c0bbd15e..ddb969fafc1 100644 --- a/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java +++ b/plugin/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java @@ -21,7 +21,7 @@ import org.elasticsearch.xpack.security.support.NoOpLogger; import org.elasticsearch.xpack.security.support.Validation; import java.io.IOException; -import java.io.PrintWriter; +import java.io.Writer; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; @@ -193,10 +193,14 @@ public class FileUserRolesStore { } } - try (PrintWriter writer = new PrintWriter(openAtomicMoveWriter(path))) { + try (Writer writer = openAtomicMoveWriter(path)) { for (Map.Entry> entry : roleToUsers.entrySet()) { - writer.printf(Locale.ROOT, "%s:%s%s", entry.getKey(), Strings.collectionToCommaDelimitedString(entry.getValue()), - System.lineSeparator()); + final String message = String.format( + Locale.ROOT, + "%s:%s%s", + entry.getKey(), + Strings.collectionToCommaDelimitedString(entry.getValue()), System.lineSeparator()); + writer.write(message); } } catch (IOException ioe) { throw new ElasticsearchException("could not write file [" + path.toAbsolutePath() + "], please check file permissions", ioe); diff --git a/plugin/src/test/java/org/elasticsearch/xpack/security/support/SecurityFilesTests.java b/plugin/src/test/java/org/elasticsearch/xpack/security/support/SecurityFilesTests.java index f546fbfda53..e1a0067eb96 100644 --- a/plugin/src/test/java/org/elasticsearch/xpack/security/support/SecurityFilesTests.java +++ b/plugin/src/test/java/org/elasticsearch/xpack/security/support/SecurityFilesTests.java @@ -11,14 +11,13 @@ import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.env.Environment; import org.elasticsearch.test.ESTestCase; -import java.io.PrintWriter; +import java.io.Writer; import java.nio.charset.StandardCharsets; import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.attribute.PosixFileAttributeView; import java.nio.file.attribute.PosixFilePermission; -import java.util.Locale; import java.util.Set; import static java.nio.file.attribute.PosixFilePermission.GROUP_EXECUTE; @@ -49,8 +48,8 @@ public class SecurityFilesTests extends ESTestCase { Files.setPosixFilePermissions(path, perms); - try (PrintWriter writer = new PrintWriter(openAtomicMoveWriter(path))) { - writer.printf(Locale.ROOT, "This is a test"); + try (Writer writer = openAtomicMoveWriter(path)) { + writer.write("This is a test"); } Set permissionsAfterWrite = Files.getPosixFilePermissions(path);