Replace atomic move writer
I noticed this while working on a previous issue with atomic move writer (silent swallowing of exceptions). Namely, atomic move writer has dangerous semantics. The problem is as follows: atomic move writer works by writing lines to a temporary file, and then in its close method it replaces the target path with the temporary file. However, the close method is invoked whether or not all writes to the temporary file succeeded (because writers obtained from atomic move writer are used in try-with-resources blocks, as they should be). There is no way to distinguish that the writer is being closed in a successful scenario versus a failure scenario. In the close method for atomic move writer, the target file is replaced by the temporary file. This means that the target file is replaced whether or not writing to the temporary file actually succeeded. Since these atomic move writers are used for user configuration files (users and user_roles), a failure here can lead to data loss for the user, a tragedy! There is another (less serious) problem with the atomic move writer. Since the close method tries to move the temporary file in place of the existing file, the temporary file can be left behind if there is another failure in the close method (e.g., closing the underlying file after writing, or setting the permissions on the temporary file). This means that in some situations, atomic move writer will leave temporary files behind (which is not definitively not atomic). This commit replaces the atomic move writer with a safer mechanism. We still perform the write atomically in the following sense: we write to a temporary file. Either writing to that file succeeds or it fails. If writing succeeds, we replace the existing file with the temporary file. If writing fails, we clean up the temporary file and the existing file remains in place. Relates elastic/x-pack-elasticsearch#2299 Original commit: elastic/x-pack-elasticsearch@3199decb0a
This commit is contained in:
Jason Tedor
GitHub
parent
e428c58dff
commit
8a5be5b58e
|
|
@ -22,12 +22,12 @@ import org.elasticsearch.xpack.security.authc.AuthenticationResult;
|
|||
import org.elasticsearch.xpack.security.authc.RealmConfig;
|
||||
import org.elasticsearch.xpack.security.authc.support.Hasher;
|
||||
import org.elasticsearch.xpack.security.support.NoOpLogger;
|
||||
import org.elasticsearch.xpack.security.support.SecurityFiles;
|
||||
import org.elasticsearch.xpack.security.support.Validation;
|
||||
import org.elasticsearch.xpack.security.support.Validation.Users;
|
||||
import org.elasticsearch.xpack.security.user.User;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.io.Writer;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
|
|
@ -40,7 +40,6 @@ import java.util.concurrent.CopyOnWriteArrayList;
|
|||
|
||||
import static java.util.Collections.emptyMap;
|
||||
import static java.util.Collections.unmodifiableMap;
|
||||
import static org.elasticsearch.xpack.security.support.SecurityFiles.openAtomicMoveWriter;
|
||||
|
||||
public class FileUserPasswdStore {
|
||||
|
||||
|
|
@ -169,15 +168,10 @@ public class FileUserPasswdStore {
|
|||
}
|
||||
|
||||
public static void writeFile(Map<String, char[]> users, Path path) {
|
||||
try (Writer writer = openAtomicMoveWriter(path)) {
|
||||
for (Map.Entry<String, char[]> entry : users.entrySet()) {
|
||||
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());
|
||||
}
|
||||
SecurityFiles.writeFileAtomically(
|
||||
path,
|
||||
users,
|
||||
e -> String.format(Locale.ROOT, "%s:%s", e.getKey(), new String(e.getValue())));
|
||||
}
|
||||
|
||||
void notifyRefresh() {
|
||||
|
|
|
|||
|
|
@ -18,10 +18,10 @@ import org.elasticsearch.watcher.ResourceWatcherService;
|
|||
import org.elasticsearch.xpack.XPackPlugin;
|
||||
import org.elasticsearch.xpack.security.authc.RealmConfig;
|
||||
import org.elasticsearch.xpack.security.support.NoOpLogger;
|
||||
import org.elasticsearch.xpack.security.support.SecurityFiles;
|
||||
import org.elasticsearch.xpack.security.support.Validation;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.io.Writer;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
|
|
@ -36,7 +36,7 @@ import java.util.regex.Pattern;
|
|||
|
||||
import static java.util.Collections.emptyMap;
|
||||
import static java.util.Collections.unmodifiableMap;
|
||||
import static org.elasticsearch.xpack.security.support.SecurityFiles.openAtomicMoveWriter;
|
||||
import static org.elasticsearch.common.Strings.collectionToCommaDelimitedString;
|
||||
|
||||
public class FileUserRolesStore {
|
||||
|
||||
|
|
@ -193,18 +193,10 @@ public class FileUserRolesStore {
|
|||
}
|
||||
}
|
||||
|
||||
try (Writer writer = openAtomicMoveWriter(path)) {
|
||||
for (Map.Entry<String, List<String>> entry : roleToUsers.entrySet()) {
|
||||
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);
|
||||
}
|
||||
SecurityFiles.writeFileAtomically(
|
||||
path,
|
||||
roleToUsers,
|
||||
e -> String.format(Locale.ROOT, "%s:%s", e.getKey(), collectionToCommaDelimitedString(e.getValue())));
|
||||
}
|
||||
|
||||
void notifyRefresh() {
|
||||
|
|
|
|||
|
|
@ -5,18 +5,27 @@
|
|||
*/
|
||||
package org.elasticsearch.xpack.security.support;
|
||||
|
||||
import org.apache.lucene.util.IOUtils;
|
||||
import org.elasticsearch.env.Environment;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.io.UncheckedIOException;
|
||||
import java.io.Writer;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.nio.file.AtomicMoveNotSupportedException;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.nio.file.StandardCopyOption;
|
||||
import java.nio.file.StandardOpenOption;
|
||||
import java.nio.file.attribute.PosixFileAttributeView;
|
||||
import java.nio.file.attribute.PosixFileAttributes;
|
||||
import java.util.Locale;
|
||||
import java.util.Map;
|
||||
import java.util.function.Function;
|
||||
|
||||
import static java.nio.file.StandardCopyOption.ATOMIC_MOVE;
|
||||
import static java.nio.file.StandardCopyOption.REPLACE_EXISTING;
|
||||
import static java.nio.file.StandardOpenOption.CREATE;
|
||||
import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING;
|
||||
import static java.nio.file.StandardOpenOption.WRITE;
|
||||
|
||||
public class SecurityFiles {
|
||||
|
||||
|
|
@ -24,49 +33,48 @@ public class SecurityFiles {
|
|||
}
|
||||
|
||||
/**
|
||||
* This writer opens a temporary file instead of the specified path and
|
||||
* tries to move the create tempfile to specified path on close. If possible
|
||||
* this move is tried to be atomic, but it will fall back to just replace the
|
||||
* existing file if the atomic move fails.
|
||||
* <p>
|
||||
* If the destination path exists, it is overwritten
|
||||
* Atomically writes to the specified file a line per entry in the specified map using the specified transform to convert each entry to
|
||||
* a line. The writing is done atomically in the following sense: first the lines are written to a temporary file and if the writing
|
||||
* succeeds then the temporary file is moved to the specified path, replacing the file if it exists. If a failure occurs, any existing
|
||||
* file is preserved, and the temporary file is cleaned up.
|
||||
*
|
||||
* @param path The path of the destination file
|
||||
* @param <K> the key type of the map entries
|
||||
* @param <V> the value type of the map entries
|
||||
* @param path the path
|
||||
* @param map the map whose entries to transform into lines
|
||||
* @param transform the transform to convert each map entry to a line
|
||||
*/
|
||||
public static final Writer openAtomicMoveWriter(final Path path) throws IOException {
|
||||
final Path tempFile = Files.createTempFile(path.getParent(), path.getFileName().toString(), "tmp");
|
||||
final Writer writer = Files.newBufferedWriter(tempFile, StandardCharsets.UTF_8, StandardOpenOption.CREATE, StandardOpenOption
|
||||
.TRUNCATE_EXISTING, StandardOpenOption.WRITE);
|
||||
return new Writer() {
|
||||
@Override
|
||||
public void write(char[] cbuf, int off, int len) throws IOException {
|
||||
writer.write(cbuf, off, len);
|
||||
}
|
||||
|
||||
@Override
|
||||
public void flush() throws IOException {
|
||||
writer.flush();
|
||||
}
|
||||
|
||||
@Override
|
||||
public void close() throws IOException {
|
||||
writer.close();
|
||||
// get original permissions
|
||||
if (Files.exists(path)) {
|
||||
boolean supportsPosixAttributes =
|
||||
Environment.getFileStore(path).supportsFileAttributeView(PosixFileAttributeView.class);
|
||||
if (supportsPosixAttributes) {
|
||||
setPosixAttributesOnTempFile(path, tempFile);
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
Files.move(tempFile, path, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
|
||||
} catch (AtomicMoveNotSupportedException e) {
|
||||
Files.move(tempFile, path, StandardCopyOption.REPLACE_EXISTING);
|
||||
public static <K, V> void writeFileAtomically(final Path path, final Map<K, V> map, final Function<Map.Entry<K, V>, String> transform) {
|
||||
Path tempFile = null;
|
||||
try {
|
||||
tempFile = Files.createTempFile(path.getParent(), path.getFileName().toString(), "tmp");
|
||||
try (Writer writer = Files.newBufferedWriter(tempFile, StandardCharsets.UTF_8, CREATE, TRUNCATE_EXISTING, WRITE)) {
|
||||
for (final Map.Entry<K, V> entry : map.entrySet()) {
|
||||
final StringBuilder sb = new StringBuilder();
|
||||
final String line = sb.append(transform.apply(entry)).append(System.lineSeparator()).toString();
|
||||
writer.write(line);
|
||||
}
|
||||
}
|
||||
};
|
||||
// get original permissions
|
||||
if (Files.exists(path)) {
|
||||
boolean supportsPosixAttributes =
|
||||
Environment.getFileStore(path).supportsFileAttributeView(PosixFileAttributeView.class);
|
||||
if (supportsPosixAttributes) {
|
||||
setPosixAttributesOnTempFile(path, tempFile);
|
||||
}
|
||||
}
|
||||
|
||||
try {
|
||||
Files.move(tempFile, path, REPLACE_EXISTING, ATOMIC_MOVE);
|
||||
} catch (final AtomicMoveNotSupportedException e) {
|
||||
Files.move(tempFile, path, REPLACE_EXISTING);
|
||||
}
|
||||
} catch (final IOException e) {
|
||||
throw new UncheckedIOException(String.format(Locale.ROOT, "could not write file [%s]", path.toAbsolutePath()), e);
|
||||
} finally {
|
||||
// we are ignoring exceptions here, so we do not need handle whether or not tempFile was initialized nor if the file exists
|
||||
IOUtils.deleteFilesIgnoringExceptions(tempFile);
|
||||
}
|
||||
}
|
||||
|
||||
static void setPosixAttributesOnTempFile(Path path, Path tempFile) throws IOException {
|
||||
|
|
|
|||
|
|
@ -11,22 +11,33 @@ import org.elasticsearch.common.util.set.Sets;
|
|||
import org.elasticsearch.env.Environment;
|
||||
import org.elasticsearch.test.ESTestCase;
|
||||
|
||||
import java.io.Writer;
|
||||
import java.io.IOException;
|
||||
import java.io.UncheckedIOException;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.nio.file.FileSystem;
|
||||
import java.nio.file.FileVisitResult;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.nio.file.SimpleFileVisitor;
|
||||
import java.nio.file.attribute.BasicFileAttributes;
|
||||
import java.nio.file.attribute.PosixFileAttributeView;
|
||||
import java.nio.file.attribute.PosixFilePermission;
|
||||
import java.util.Collections;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
import java.util.TreeMap;
|
||||
import java.util.concurrent.atomic.AtomicBoolean;
|
||||
|
||||
import static java.nio.file.attribute.PosixFilePermission.GROUP_EXECUTE;
|
||||
import static java.nio.file.attribute.PosixFilePermission.OTHERS_EXECUTE;
|
||||
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
|
||||
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
|
||||
import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
|
||||
import static org.elasticsearch.xpack.security.support.SecurityFiles.openAtomicMoveWriter;
|
||||
import static org.hamcrest.Matchers.containsString;
|
||||
import static org.hamcrest.Matchers.equalTo;
|
||||
import static org.hamcrest.Matchers.hasSize;
|
||||
import static org.hamcrest.Matchers.hasToString;
|
||||
import static org.hamcrest.Matchers.is;
|
||||
import static org.hamcrest.Matchers.not;
|
||||
|
||||
|
|
@ -48,14 +59,79 @@ public class SecurityFilesTests extends ESTestCase {
|
|||
|
||||
Files.setPosixFilePermissions(path, perms);
|
||||
|
||||
try (Writer writer = openAtomicMoveWriter(path)) {
|
||||
writer.write("This is a test");
|
||||
}
|
||||
final Map<String, String> map = new TreeMap<>();
|
||||
map.put("This is the first", "line");
|
||||
map.put("This is the second", "line");
|
||||
SecurityFiles.writeFileAtomically(path, map, e -> e.getKey() + " " + e.getValue());
|
||||
|
||||
final List<String> lines = Files.readAllLines(path);
|
||||
assertThat(lines, hasSize(2));
|
||||
assertThat(lines.get(0), equalTo("This is the first line"));
|
||||
assertThat(lines.get(1), equalTo("This is the second line"));
|
||||
|
||||
Set<PosixFilePermission> permissionsAfterWrite = Files.getPosixFilePermissions(path);
|
||||
assertThat(permissionsAfterWrite, is(perms));
|
||||
}
|
||||
|
||||
public void testFailure() throws IOException {
|
||||
final Path path = createTempFile("existing", "file");
|
||||
|
||||
Files.write(path, "foo".getBytes(StandardCharsets.UTF_8));
|
||||
|
||||
final Visitor innerVisitor = new Visitor(path);
|
||||
final RuntimeException re = expectThrows(RuntimeException.class, () -> SecurityFiles.writeFileAtomically(
|
||||
path,
|
||||
Collections.singletonMap("foo", "bar"),
|
||||
e -> {
|
||||
try {
|
||||
Files.walkFileTree(path.getParent(), innerVisitor);
|
||||
} catch (final IOException inner) {
|
||||
throw new UncheckedIOException(inner);
|
||||
}
|
||||
throw new RuntimeException(e.getKey() + " " + e.getValue());
|
||||
}
|
||||
));
|
||||
|
||||
assertThat(re, hasToString(containsString("foo bar")));
|
||||
|
||||
// assert the temporary file was created while trying to write the file
|
||||
assertTrue(innerVisitor.found());
|
||||
|
||||
final Visitor visitor = new Visitor(path);
|
||||
Files.walkFileTree(path.getParent(), visitor);
|
||||
|
||||
// now assert the temporary file was cleaned up after the write failed
|
||||
assertFalse(visitor.found());
|
||||
|
||||
// finally, assert the original file contents remain
|
||||
final List<String> lines = Files.readAllLines(path);
|
||||
assertThat(lines, hasSize(1));
|
||||
assertThat(lines.get(0), equalTo("foo"));
|
||||
}
|
||||
|
||||
static final class Visitor extends SimpleFileVisitor<Path> {
|
||||
|
||||
private final Path path;
|
||||
private final AtomicBoolean found = new AtomicBoolean();
|
||||
|
||||
Visitor(final Path path) {
|
||||
this.path = path;
|
||||
}
|
||||
|
||||
public boolean found() {
|
||||
return found.get();
|
||||
}
|
||||
|
||||
@Override
|
||||
public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
|
||||
if (file.getFileName().toString().startsWith(path.getFileName().toString()) && file.getFileName().toString().endsWith("tmp")) {
|
||||
found.set(true);
|
||||
return FileVisitResult.TERMINATE;
|
||||
}
|
||||
return FileVisitResult.CONTINUE;
|
||||
}
|
||||
}
|
||||
|
||||
public void testThatOwnerAndGroupAreChanged() throws Exception {
|
||||
Configuration jimFsConfiguration = Configuration.unix().toBuilder().setAttributeViews("basic", "owner", "posix", "unix").build();
|
||||
try (FileSystem fs = Jimfs.newFileSystem(jimFsConfiguration)) {
|
||||
|
|
|
|||
Loading…
Reference in New Issue