File permissions: Fixes and improvement

* Fix: `ShieldFiles.openAtomicMoveWriter()` always changed permissions to 600
  now changes back to original perms
* Fix: Required log message change by @skearns
* Improvement: When permissions change, before/after perms are now shown
* Improvement: Added more CheckFileCommand tests

Closes elastic/elasticsearch#634

Original commit: elastic/x-pack-elasticsearch@e44495aaff
This commit is contained in:
Alexander Reelsen 2015-01-25 12:01:43 +01:00
parent 4fb18bb65a
commit f1bff033cc
5 changed files with 158 additions and 92 deletions

View File

@ -15,9 +15,12 @@ import java.nio.file.Path;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFileAttributes;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
import java.util.Map;
import java.util.Set;
import static org.elasticsearch.common.cli.Terminal.Verbosity.SILENT;
/**
* helper command to check if file permissions or owner got changed by the command being executed
*/
@ -69,7 +72,9 @@ public abstract class CheckFileCommand extends CliTool.Command {
Set<PosixFilePermission> permissionsBeforeWrite = entry.getValue();
Set<PosixFilePermission> permissionsAfterWrite = Files.getPosixFilePermissions(entry.getKey());
if (!permissionsBeforeWrite.equals(permissionsAfterWrite)) {
terminal.printError("Permissions of [%s] differ after write. Please ensure correct permissions before going on!", entry.getKey());
terminal.println(SILENT, "WARN: The file permissions of [%s] have changed from [%s] to [%s]",
entry.getKey(), PosixFilePermissions.toString(permissionsBeforeWrite), PosixFilePermissions.toString(permissionsAfterWrite));
terminal.println(SILENT, "Please ensure that the user account running Elasticsearch has read access to this file!");
}
}
@ -78,7 +83,7 @@ public abstract class CheckFileCommand extends CliTool.Command {
String ownerBeforeWrite = entry.getValue();
String ownerAfterWrite = Files.getOwner(entry.getKey()).getName();
if (!ownerAfterWrite.equals(ownerBeforeWrite)) {
terminal.printError("Owner of file [%s] used to be [%s], but now is [%s]", entry.getKey(), ownerBeforeWrite, ownerAfterWrite);
terminal.println(SILENT, "WARN: Owner of file [%s] used to be [%s], but now is [%s]", entry.getKey(), ownerBeforeWrite, ownerAfterWrite);
}
}
@ -87,7 +92,7 @@ public abstract class CheckFileCommand extends CliTool.Command {
String groupBeforeWrite = entry.getValue();
String groupAfterWrite = Files.readAttributes(entry.getKey(), PosixFileAttributes.class).group().getName();
if (!groupAfterWrite.equals(groupBeforeWrite)) {
terminal.printError("Group of file [%s] used to be [%s], but now is [%s]", entry.getKey(), groupBeforeWrite, groupAfterWrite);
terminal.println(SILENT, "WARN: Group of file [%s] used to be [%s], but now is [%s]", entry.getKey(), groupBeforeWrite, groupAfterWrite);
}
}

View File

@ -10,6 +10,9 @@ import org.elasticsearch.common.base.Charsets;
import java.io.IOException;
import java.io.Writer;
import java.nio.file.*;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.util.Set;
public class ShieldFiles {
@ -44,11 +47,24 @@ public class ShieldFiles {
@Override
public void close() throws IOException {
writer.close();
// get original permissions
boolean supportsPosixPermissions = false;
Set<PosixFilePermission> posixFilePermissions = null;
if (Files.exists(path)) {
supportsPosixPermissions = Files.getFileStore(path).supportsFileAttributeView(PosixFileAttributeView.class);
if (supportsPosixPermissions) {
posixFilePermissions = Files.getPosixFilePermissions(path);
}
}
try {
Files.move(tempFile, path, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
} catch (AtomicMoveNotSupportedException e) {
Files.move(tempFile, path, StandardCopyOption.REPLACE_EXISTING);
}
// restore original permissions
if (supportsPosixPermissions && posixFilePermissions != null) {
Files.setPosixFilePermissions(path, posixFilePermissions);
}
}
};
}

View File

@ -19,10 +19,8 @@ import java.io.IOException;
import java.nio.file.FileSystem;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.GroupPrincipal;
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.UserPrincipal;
import java.nio.file.attribute.*;
import java.util.Set;
import static org.hamcrest.Matchers.*;
@ -36,57 +34,79 @@ public class CheckFileCommandTests extends ElasticsearchTestCase {
private Configuration jimFsConfiguration = Configuration.unix().toBuilder().setAttributeViews("basic", "owner", "posix", "unix").build();
private Configuration jimFsConfigurationWithoutPermissions = randomBoolean() ? Configuration.unix().toBuilder().setAttributeViews("basic").build() : Configuration.windows();
@Test
public void testThatCommandLogsErrorMessageOnFail() throws Exception {
executeCommand(jimFsConfiguration, new PermissionCheckFileCommand(captureOutputTerminal, true));
assertThat(captureOutputTerminal.getTerminalOutput(), hasItem(containsString("Please ensure correct permissions")));
private enum Mode {
CHANGE, KEEP, DISABLED
}
@Test
public void testThatCommandLogsNothingOnSuccess() throws Exception {
executeCommand(jimFsConfiguration, new PermissionCheckFileCommand(captureOutputTerminal, false));
public void testThatCommandLogsErrorMessageOnFail() throws Exception {
executeCommand(jimFsConfiguration, new PermissionCheckFileCommand(captureOutputTerminal, Mode.CHANGE));
assertThat(captureOutputTerminal.getTerminalOutput(), hasItem(containsString("Please ensure that the user account running Elasticsearch has read access to this file")));
}
@Test
public void testThatCommandLogsNothingWhenPermissionRemains() throws Exception {
executeCommand(jimFsConfiguration, new PermissionCheckFileCommand(captureOutputTerminal, Mode.KEEP));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsNothingWhenDisabled() throws Exception {
executeCommand(jimFsConfiguration, new PermissionCheckFileCommand(captureOutputTerminal, Mode.DISABLED));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsNothingIfFilesystemDoesNotSupportPermissions() throws Exception {
executeCommand(jimFsConfigurationWithoutPermissions, new PermissionCheckFileCommand(captureOutputTerminal, false));
executeCommand(jimFsConfigurationWithoutPermissions, new PermissionCheckFileCommand(captureOutputTerminal, Mode.DISABLED));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsOwnerChange() throws Exception {
executeCommand(jimFsConfiguration, new OwnerCheckFileCommand(captureOutputTerminal, true));
executeCommand(jimFsConfiguration, new OwnerCheckFileCommand(captureOutputTerminal, Mode.CHANGE));
assertThat(captureOutputTerminal.getTerminalOutput(), hasItem(allOf(containsString("Owner of file ["), containsString("] used to be ["), containsString("], but now is ["))));
}
@Test
public void testThatCommandLogsNothingIfOwnerIsNotChanged() throws Exception {
executeCommand(jimFsConfiguration, new OwnerCheckFileCommand(captureOutputTerminal, false));
public void testThatCommandLogsNothingIfOwnerRemainsSame() throws Exception {
executeCommand(jimFsConfiguration, new OwnerCheckFileCommand(captureOutputTerminal, Mode.KEEP));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsNothingIfOwnerIsDisabled() throws Exception {
executeCommand(jimFsConfiguration, new OwnerCheckFileCommand(captureOutputTerminal, Mode.DISABLED));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsNothingIfFileSystemDoesNotSupportOwners() throws Exception {
executeCommand(jimFsConfigurationWithoutPermissions, new OwnerCheckFileCommand(captureOutputTerminal, false));
executeCommand(jimFsConfigurationWithoutPermissions, new OwnerCheckFileCommand(captureOutputTerminal, Mode.DISABLED));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsIfGroupChanges() throws Exception {
executeCommand(jimFsConfiguration, new GroupCheckFileCommand(captureOutputTerminal, true));
executeCommand(jimFsConfiguration, new GroupCheckFileCommand(captureOutputTerminal, Mode.CHANGE));
assertThat(captureOutputTerminal.getTerminalOutput(), hasItem(allOf(containsString("Group of file ["), containsString("] used to be ["), containsString("], but now is ["))));
}
@Test
public void testThatCommandLogsNothingIfGroupIsNotChanged() throws Exception {
executeCommand(jimFsConfiguration, new GroupCheckFileCommand(captureOutputTerminal, false));
public void testThatCommandLogsNothingIfGroupRemainsSame() throws Exception {
executeCommand(jimFsConfiguration, new GroupCheckFileCommand(captureOutputTerminal, Mode.KEEP));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsNothingIfGroupIsDisabled() throws Exception {
executeCommand(jimFsConfiguration, new GroupCheckFileCommand(captureOutputTerminal, Mode.DISABLED));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@Test
public void testThatCommandLogsNothingIfFileSystemDoesNotSupportGroups() throws Exception {
executeCommand(jimFsConfigurationWithoutPermissions, new GroupCheckFileCommand(captureOutputTerminal, false));
executeCommand(jimFsConfigurationWithoutPermissions, new GroupCheckFileCommand(captureOutputTerminal, Mode.DISABLED));
assertThat(captureOutputTerminal.getTerminalOutput(), hasSize(0));
}
@ -98,13 +118,13 @@ public class CheckFileCommandTests extends ElasticsearchTestCase {
abstract class AbstractTestCheckFileCommand extends CheckFileCommand {
protected final boolean enabled;
protected final Mode mode;
protected FileSystem fs;
protected Path[] paths;
public AbstractTestCheckFileCommand(Terminal terminal, boolean enabled) throws IOException {
public AbstractTestCheckFileCommand(Terminal terminal, Mode mode) throws IOException {
super(terminal);
this.enabled = enabled;
this.mode = mode;
}
public CliTool.ExitStatus execute(FileSystem fs) throws Exception {
@ -130,15 +150,24 @@ public class CheckFileCommandTests extends ElasticsearchTestCase {
*/
class PermissionCheckFileCommand extends AbstractTestCheckFileCommand {
public PermissionCheckFileCommand(Terminal terminal, boolean enabled) throws IOException {
super(terminal, enabled);
public PermissionCheckFileCommand(Terminal terminal, Mode mode) throws IOException {
super(terminal, mode);
}
@Override
public CliTool.ExitStatus doExecute(Settings settings, Environment env) throws Exception {
if (enabled) {
int randomInt = randomInt(paths.length - 1);
Files.setPosixFilePermissions(paths[randomInt], Sets.newHashSet(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OTHERS_EXECUTE, PosixFilePermission.GROUP_EXECUTE));
int randomInt = randomInt(paths.length - 1);
Path randomPath = paths[randomInt];
switch (mode) {
case CHANGE:
Files.write(randomPath, randomAsciiOfLength(10).getBytes(Charsets.UTF_8));
Files.setPosixFilePermissions(randomPath, Sets.newHashSet(PosixFilePermission.OWNER_EXECUTE, PosixFilePermission.OTHERS_EXECUTE, PosixFilePermission.GROUP_EXECUTE));
break;
case KEEP:
Files.write(randomPath, randomAsciiOfLength(10).getBytes(Charsets.UTF_8));
Set<PosixFilePermission> posixFilePermissions = Files.getPosixFilePermissions(randomPath);
Files.setPosixFilePermissions(randomPath, posixFilePermissions);
break;
}
return CliTool.ExitStatus.OK;
}
@ -150,17 +179,27 @@ public class CheckFileCommandTests extends ElasticsearchTestCase {
*/
class OwnerCheckFileCommand extends AbstractTestCheckFileCommand {
public OwnerCheckFileCommand(Terminal terminal, boolean enabled) throws IOException {
super(terminal, enabled);
public OwnerCheckFileCommand(Terminal terminal, Mode mode) throws IOException {
super(terminal, mode);
}
@Override
public CliTool.ExitStatus doExecute(Settings settings, Environment env) throws Exception {
if (enabled) {
int randomInt = randomInt(paths.length - 1);
UserPrincipal owner = fs.getUserPrincipalLookupService().lookupPrincipalByName(randomAsciiOfLength(10));
Files.setOwner(paths[randomInt], owner);
int randomInt = randomInt(paths.length - 1);
Path randomPath = paths[randomInt];
switch (mode) {
case CHANGE:
Files.write(randomPath, randomAsciiOfLength(10).getBytes(Charsets.UTF_8));
UserPrincipal randomOwner = fs.getUserPrincipalLookupService().lookupPrincipalByName(randomAsciiOfLength(10));
Files.setOwner(randomPath, randomOwner);
break;
case KEEP:
Files.write(randomPath, randomAsciiOfLength(10).getBytes(Charsets.UTF_8));
UserPrincipal originalOwner = Files.getOwner(randomPath);
Files.setOwner(randomPath, originalOwner);
break;
}
return CliTool.ExitStatus.OK;
}
}
@ -170,17 +209,27 @@ public class CheckFileCommandTests extends ElasticsearchTestCase {
*/
class GroupCheckFileCommand extends AbstractTestCheckFileCommand {
public GroupCheckFileCommand(Terminal terminal, boolean enabled) throws IOException {
super(terminal, enabled);
public GroupCheckFileCommand(Terminal terminal, Mode mode) throws IOException {
super(terminal, mode);
}
@Override
public CliTool.ExitStatus doExecute(Settings settings, Environment env) throws Exception {
if (enabled) {
int randomInt = randomInt(paths.length - 1);
GroupPrincipal groupPrincipal = fs.getUserPrincipalLookupService().lookupPrincipalByGroupName(randomAsciiOfLength(10));
Files.getFileAttributeView(paths[randomInt], PosixFileAttributeView.class).setGroup(groupPrincipal);
int randomInt = randomInt(paths.length - 1);
Path randomPath = paths[randomInt];
switch (mode) {
case CHANGE:
Files.write(randomPath, randomAsciiOfLength(10).getBytes(Charsets.UTF_8));
GroupPrincipal randomPrincipal = fs.getUserPrincipalLookupService().lookupPrincipalByGroupName(randomAsciiOfLength(10));
Files.getFileAttributeView(randomPath, PosixFileAttributeView.class).setGroup(randomPrincipal);
break;
case KEEP:
Files.write(randomPath, randomAsciiOfLength(10).getBytes(Charsets.UTF_8));
GroupPrincipal groupPrincipal = Files.readAttributes(randomPath, PosixFileAttributes.class).group();
Files.getFileAttributeView(randomPath, PosixFileAttributeView.class).setGroup(groupPrincipal);
break;
}
return CliTool.ExitStatus.OK;
}
}

View File

@ -206,31 +206,6 @@ public class ESUsersToolTests extends CliToolTestCase {
assertThat(status, is(CliTool.ExitStatus.CODE_ERROR));
}
@Test
public void testUseradd_Cmd_LogsPermissionChange() throws Exception {
File userFile = writeFile("user1:hash1");
File userRolesFile = newTempFile();
assumePosixPermissionsAreSupported(userFile.toPath(), userRolesFile.toPath());
java.nio.file.Files.setPosixFilePermissions(userFile.toPath(), Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE));
java.nio.file.Files.setPosixFilePermissions(userRolesFile.toPath(), Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE));
Settings settings = ImmutableSettings.builder()
.put("shield.authc.realms.esusers.type", "esusers")
.put("shield.authc.realms.esusers.files.users", userFile)
.put("shield.authc.realms.esusers.files.users_roles", userRolesFile)
.build();
CaptureOutputTerminal captureOutputTerminal = new CaptureOutputTerminal();
ESUsersTool.Useradd cmd = new ESUsersTool.Useradd(captureOutputTerminal, "user2", SecuredStringTests.build("changeme"));
CliTool.ExitStatus status = execute(cmd, settings);
assertThat(status, is(CliTool.ExitStatus.OK));
assertThat(captureOutputTerminal.getTerminalOutput(), hasItem(containsString("Please ensure correct permissions")));
}
@Test
public void testUserdel_Parse() throws Exception {
ESUsersTool tool = new ESUsersTool();
@ -322,31 +297,6 @@ public class ESUsersToolTests extends CliToolTestCase {
assertThat(userRolesFile.exists(), is(false));
}
@Test
public void testUserdel_Cmd_LogsPermissionChange() throws Exception {
File userFile = writeFile("user1:hash1");
File userRolesFile = newTempFile();
assumePosixPermissionsAreSupported(userFile.toPath(), userRolesFile.toPath());
java.nio.file.Files.setPosixFilePermissions(userFile.toPath(), Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE));
java.nio.file.Files.setPosixFilePermissions(userRolesFile.toPath(), Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE, PosixFilePermission.GROUP_WRITE, PosixFilePermission.OTHERS_WRITE));
Settings settings = ImmutableSettings.builder()
.put("shield.authc.realms.esusers.type", "esusers")
.put("shield.authc.realms.esusers.files.users", userFile)
.put("shield.authc.realms.esusers.files.users_roles", userRolesFile)
.build();
CaptureOutputTerminal captureOutputTerminal = new CaptureOutputTerminal();
ESUsersTool.Userdel cmd = new ESUsersTool.Userdel(captureOutputTerminal, "user1");
CliTool.ExitStatus status = execute(cmd, settings);
assertThat(status, is(CliTool.ExitStatus.OK));
assertThat(captureOutputTerminal.getTerminalOutput(), hasItem(containsString("Please ensure correct permissions")));
}
@Test
public void testPasswd_Parse_AllOptions() throws Exception {
ESUsersTool tool = new ESUsersTool();

View File

@ -0,0 +1,46 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
package org.elasticsearch.shield.support;
import com.google.common.collect.Sets;
import org.elasticsearch.common.base.Charsets;
import org.elasticsearch.test.ElasticsearchTestCase;
import org.junit.Test;
import java.io.PrintWriter;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.PosixFilePermission;
import java.util.Locale;
import java.util.Set;
import static java.nio.file.attribute.PosixFilePermission.*;
import static org.elasticsearch.shield.support.ShieldFiles.openAtomicMoveWriter;
import static org.hamcrest.Matchers.is;
public class ShieldFilesTests extends ElasticsearchTestCase {
@Test
public void testThatOriginalPermissionsAreKept() throws Exception {
Path path = newTempFile().toPath();
Files.write(path, "foo".getBytes(Charsets.UTF_8));
Set<PosixFilePermission> perms = Sets.newHashSet(OWNER_READ, OWNER_WRITE);
if (randomBoolean()) perms.add(OWNER_EXECUTE);
if (randomBoolean()) perms.add(GROUP_EXECUTE);
if (randomBoolean()) perms.add(OTHERS_EXECUTE);
Files.setPosixFilePermissions(path, perms);
try (PrintWriter writer = new PrintWriter(openAtomicMoveWriter(path))) {
writer.printf(Locale.ROOT, "This is a test");
}
Set<PosixFilePermission> permissionsAfterWrite = Files.getPosixFilePermissions(path);
assertThat(permissionsAfterWrite, is(perms));
}
}