From 9864ae05a27d4a4202bd92bbd7225b979ab60630 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 2 Mar 2016 23:16:50 -0800 Subject: [PATCH] Switch system key tool to use jopt-simple Original commit: elastic/x-pack-elasticsearch@c5c459c77a37312e52fef82215cf16159794278a --- .../shield/crypto/tool/SystemKeyTool.java | 143 +++++++----------- .../shield/support/FileAttributesChecker.java | 76 ++++++++++ .../crypto/tool/SystemKeyToolTests.java | 123 +++++---------- 3 files changed, 169 insertions(+), 173 deletions(-) create mode 100644 elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/support/FileAttributesChecker.java diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/crypto/tool/SystemKeyTool.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/crypto/tool/SystemKeyTool.java index 68ff4adba75..054145bd41e 100644 --- a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/crypto/tool/SystemKeyTool.java +++ b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/crypto/tool/SystemKeyTool.java @@ -5,123 +5,84 @@ */ package org.elasticsearch.shield.crypto.tool; -import org.apache.commons.cli.CommandLine; -import org.elasticsearch.common.SuppressForbidden; -import org.elasticsearch.common.cli.CheckFileCommand; -import org.elasticsearch.common.cli.CliTool; -import org.elasticsearch.common.cli.CliToolConfig; -import org.elasticsearch.common.cli.Terminal; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.common.util.set.Sets; -import org.elasticsearch.env.Environment; -import org.elasticsearch.shield.crypto.InternalCryptoService; - -import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.PosixFileAttributeView; import java.nio.file.attribute.PosixFilePermission; +import java.util.List; import java.util.Locale; import java.util.Set; -import static org.elasticsearch.common.cli.CliToolConfig.Builder.cmd; -import static org.elasticsearch.common.cli.CliToolConfig.config; +import joptsimple.OptionSet; +import joptsimple.OptionSpec; +import org.elasticsearch.cli.Command; +import org.elasticsearch.cli.ExitCodes; +import org.elasticsearch.cli.UserError; +import org.elasticsearch.common.cli.Terminal; +import org.elasticsearch.common.io.PathUtils; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.set.Sets; +import org.elasticsearch.env.Environment; +import org.elasticsearch.node.internal.InternalSettingsPreparer; +import org.elasticsearch.shield.crypto.InternalCryptoService; +import org.elasticsearch.shield.support.FileAttributesChecker; -/** - * - */ -public class SystemKeyTool extends CliTool { +public class SystemKeyTool extends Command { public static final Set PERMISSION_OWNER_READ_WRITE = Sets.newHashSet(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE); - public static void main(String[] args) throws Exception { - ExitStatus exitStatus = new SystemKeyTool().execute(args); - exit(exitStatus.status()); + Environment env = InternalSettingsPreparer.prepareEnvironment(Settings.EMPTY, Terminal.DEFAULT); + exit(new SystemKeyTool(env).main(args, Terminal.DEFAULT)); } - @SuppressForbidden(reason = "Allowed to exit explicitly from #main()") - private static void exit(int status) { - System.exit(status); - } + private final Environment env; + private final OptionSpec arguments; - private static final CliToolConfig CONFIG = config("syskey", SystemKeyTool.class) - .cmds(Generate.CMD) - .build(); - - public SystemKeyTool() { - super(CONFIG); + public SystemKeyTool(Environment env) { + super("Generates the system key"); + this.env = env; + this.arguments = parser.nonOptions("key path"); } @Override - protected Command parse(String cmd, CommandLine commandLine) throws Exception { - return Generate.parse(terminal, commandLine, env); + protected int execute(Terminal terminal, OptionSet options) throws Exception { + Path keyPath = null; + List args = arguments.values(options); + if (args.size() > 1) { + throw new UserError(ExitCodes.USAGE, "No more than one key path can be supplied"); + } else if (args.size() == 1) { + keyPath = PathUtils.get(args.get(0)); + } + execute(terminal, keyPath); + return ExitCodes.OK; } - static class Generate extends CheckFileCommand { + // pkg private for tests + void execute(Terminal terminal, Path keyPath) throws Exception { + if (keyPath == null) { + keyPath = InternalCryptoService.resolveSystemKey(env.settings(), env); + } + FileAttributesChecker attributesChecker = new FileAttributesChecker(keyPath); - private static final CliToolConfig.Cmd CMD = cmd("generate", Generate.class).build(); + // write the key + terminal.println(Terminal.Verbosity.VERBOSE, "generating..."); + byte[] key = InternalCryptoService.generateKey(); + terminal.println(String.format(Locale.ROOT, "Storing generated key in [%s]...", keyPath.toAbsolutePath())); + Files.write(keyPath, key, StandardOpenOption.CREATE_NEW); - final Path path; - - Generate(Terminal terminal, Path path) { - super(terminal); - this.path = path; + // set permissions to 600 + PosixFileAttributeView view = Files.getFileAttributeView(keyPath, PosixFileAttributeView.class); + if (view != null) { + view.setPermissions(PERMISSION_OWNER_READ_WRITE); + terminal.println("Ensure the generated key can be read by the user that Elasticsearch runs as, " + + "permissions are set to owner read/write only"); } - public static Command parse(Terminal terminal, CommandLine cl, Environment env) { - String[] args = cl.getArgs(); - if (args.length > 1) { - return exitCmd(ExitStatus.USAGE, terminal, "Too many arguments"); - } - Path path = args.length != 0 ? env.binFile().getParent().resolve(args[0]) : null; - return new Generate(terminal, path); - } - - @Override - protected Path[] pathsForPermissionsCheck(Settings settings, Environment env) { - Path path = this.path; - if (path == null) { - path = InternalCryptoService.resolveSystemKey(settings, env); - } - return new Path[]{path}; - } - - @Override - public ExitStatus doExecute(Settings settings, Environment env) throws Exception { - Path path = this.path; - try { - if (path == null) { - path = InternalCryptoService.resolveSystemKey(settings, env); - } - terminal.println(Terminal.Verbosity.VERBOSE, "generating..."); - byte[] key = InternalCryptoService.generateKey(); - terminal.println(String.format(Locale.ROOT, "Storing generated key in [%s]...", path.toAbsolutePath())); - Files.write(path, key, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING); - } catch (IOException ioe) { - terminal.println(Terminal.Verbosity.SILENT, - String.format(Locale.ROOT, "ERROR: Cannot generate and save system key file [%s]", path.toAbsolutePath())); - return ExitStatus.IO_ERROR; - } - - boolean supportsPosixPermissions = Environment.getFileStore(path).supportsFileAttributeView(PosixFileAttributeView.class); - if (supportsPosixPermissions) { - try { - Files.setPosixFilePermissions(path, PERMISSION_OWNER_READ_WRITE); - } catch (IOException ioe) { - terminal.println(Terminal.Verbosity.SILENT, - String.format(Locale.ROOT, "ERROR: Cannot set owner read/write permissions to generated system key file [%s]", - path.toAbsolutePath())); - return ExitStatus.IO_ERROR; - } - terminal.println("Ensure the generated key can be read by the user that Elasticsearch runs as, permissions are set to " + - "owner read/write only"); - } - - return ExitStatus.OK; - } + // check if attributes changed + attributesChecker.check(terminal); } } diff --git a/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/support/FileAttributesChecker.java b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/support/FileAttributesChecker.java new file mode 100644 index 00000000000..e2b9be3c2f9 --- /dev/null +++ b/elasticsearch/x-pack/shield/src/main/java/org/elasticsearch/shield/support/FileAttributesChecker.java @@ -0,0 +1,76 @@ +/* + * 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 java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.PosixFileAttributeView; +import java.nio.file.attribute.PosixFileAttributes; +import java.nio.file.attribute.PosixFilePermissions; + +import org.elasticsearch.common.cli.Terminal; + +/** + * A utility for cli tools to capture file attributes + * before writing files, and to warn if the permissions/group/owner changes. + */ +public class FileAttributesChecker { + + // the paths to check + private final Path[] paths; + + // captured attributes for each path + private final PosixFileAttributes[] attributes; + + /** Create a checker for the given paths, which will warn to the given terminal if changes are made. */ + public FileAttributesChecker(Path... paths) { + this.paths = paths; + this.attributes = new PosixFileAttributes[paths.length]; + + for (int i = 0; i < paths.length; ++i) { + if (Files.exists(paths[i]) == false) continue; + PosixFileAttributeView view = Files.getFileAttributeView(paths[i], PosixFileAttributeView.class); + if (view == null) continue; // not posix + try { + this.attributes[i] = view.readAttributes(); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + } + + /** Check if attributes of the paths have changed, warning to the given terminal if they have. */ + public void check(Terminal terminal) throws IOException { + for (int i = 0; i < paths.length; ++i) { + if (attributes[i] == null) { + // we couldn't get attributes in setup, so we can't check them now + continue; + } + + PosixFileAttributeView view = Files.getFileAttributeView(paths[i], PosixFileAttributeView.class); + PosixFileAttributes newAttributes = view.readAttributes(); + PosixFileAttributes oldAttributes = attributes[i]; + if (oldAttributes.permissions().equals(newAttributes.permissions()) == false) { + terminal.println(Terminal.Verbosity.SILENT, "WARNING: The file permissions of [" + paths[i] + "] have changed " + + "from [" + PosixFilePermissions.toString(oldAttributes.permissions()) + "] " + + "to [" + PosixFilePermissions.toString(newAttributes.permissions()) + "]"); + terminal.println(Terminal.Verbosity.SILENT, + "Please ensure that the user account running Elasticsearch has read access to this file!"); + } + if (oldAttributes.owner().getName().equals(newAttributes.owner().getName()) == false) { + terminal.println(Terminal.Verbosity.SILENT, "WARNING: Owner of file [" + paths[i] + "] " + + "used to be [" + oldAttributes.owner().getName() + "], " + + "but now is [" + newAttributes.owner().getName() + "]"); + } + if (oldAttributes.group().getName().equals(newAttributes.group().getName()) == false) { + terminal.println(Terminal.Verbosity.SILENT, "WARNING: Group of file [" + paths[i] + "] " + + "used to be [" + oldAttributes.group().getName() + "], " + + "but now is [" + newAttributes.group().getName() + "]"); + } + } + } +} diff --git a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/crypto/tool/SystemKeyToolTests.java b/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/crypto/tool/SystemKeyToolTests.java index e2ce527eb4c..d4015132bb3 100644 --- a/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/crypto/tool/SystemKeyToolTests.java +++ b/elasticsearch/x-pack/shield/src/test/java/org/elasticsearch/shield/crypto/tool/SystemKeyToolTests.java @@ -5,118 +5,77 @@ */ package org.elasticsearch.shield.crypto.tool; -import org.elasticsearch.common.cli.CliTool; -import org.elasticsearch.common.cli.CliToolTestCase; -import org.elasticsearch.common.cli.Terminal; -import org.elasticsearch.common.settings.Settings; -import org.elasticsearch.env.Environment; -import org.elasticsearch.shield.Shield; -import org.elasticsearch.shield.crypto.InternalCryptoService; -import org.elasticsearch.shield.crypto.tool.SystemKeyTool.Generate; -import org.junit.Before; - 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.Set; -import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasSize; -import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.nullValue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +import org.elasticsearch.common.cli.CliToolTestCase; +import org.elasticsearch.common.cli.Terminal; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.env.Environment; +import org.elasticsearch.shield.crypto.InternalCryptoService; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.XPackPlugin; +import org.junit.Before; -/** - * - */ -public class SystemKeyToolTests extends CliToolTestCase { +public class SystemKeyToolTests extends ESTestCase { private Terminal terminal; private Environment env; @Before public void init() throws Exception { - terminal = mock(Terminal.class); - env = mock(Environment.class); - Path tmpDir = createTempDir(); - when(env.binFile()).thenReturn(tmpDir.resolve("bin")); - } - - public void testParseNoArgs() throws Exception { - CliTool.Command cmd = new SystemKeyTool().parse("generate", args("")); - assertThat(cmd, instanceOf(Generate.class)); - Generate generate = (Generate) cmd; - assertThat(generate.path, nullValue()); - } - - public void testParseFileArg() throws Exception { - Path path = createTempFile(); - CliTool.Command cmd = new SystemKeyTool().parse("generate", new String[]{path.toAbsolutePath().toString()}); - assertThat(cmd, instanceOf(Generate.class)); - Generate generate = (Generate) cmd; - - // The test framework wraps paths so we can't compare path to path - assertThat(generate.path.toString(), equalTo(path.toString())); + terminal = new CliToolTestCase.CaptureOutputTerminal(); + Settings settings = Settings.builder() + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir()).build(); + env = new Environment(settings); } public void testGenerate() throws Exception { assumeTrue("test cannot run with security manager enabled", System.getSecurityManager() == null); - Path path = createTempFile(); - Generate generate = new Generate(terminal, path); - CliTool.ExitStatus status = generate.execute(Settings.EMPTY, env); - assertThat(status, is(CliTool.ExitStatus.OK)); + Path path = createTempDir().resolve("key"); + new SystemKeyTool(env).execute(terminal, path); byte[] bytes = Files.readAllBytes(path); - assertThat(bytes.length, is(InternalCryptoService.KEY_SIZE / 8)); + // TODO: maybe we should actually check the key is...i dunno...valid? + assertEquals(InternalCryptoService.KEY_SIZE / 8, bytes.length); + + Set perms = Files.getPosixFilePermissions(path); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_READ)); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_WRITE)); + assertEquals(perms.toString(), 2, perms.size()); } public void testGeneratePathInSettings() throws Exception { assumeTrue("test cannot run with security manager enabled", System.getSecurityManager() == null); - Path path = createTempFile(); - Settings settings = Settings.builder() - .put("shield.system_key.file", path.toAbsolutePath().toString()) - .build(); - Generate generate = new Generate(terminal, null); - CliTool.ExitStatus status = generate.execute(settings, env); - assertThat(status, is(CliTool.ExitStatus.OK)); + Path path = createTempDir().resolve("key"); + Settings settings = Settings.builder().put(env.settings()) + .put("shield.system_key.file", path.toAbsolutePath().toString()).build(); + env = new Environment(settings); + new SystemKeyTool(env).execute(terminal, (Path) null); byte[] bytes = Files.readAllBytes(path); - assertThat(bytes.length, is(InternalCryptoService.KEY_SIZE / 8)); + assertEquals(InternalCryptoService.KEY_SIZE / 8, bytes.length); } public void testGenerateDefaultPath() throws Exception { assumeTrue("test cannot run with security manager enabled", System.getSecurityManager() == null); - Path config = createTempDir(); - Path shieldConfig = config.resolve(Shield.NAME); - Files.createDirectories(shieldConfig); - Path path = shieldConfig.resolve("system_key"); - when(env.configFile()).thenReturn(config); - Generate generate = new Generate(terminal, null); - CliTool.ExitStatus status = generate.execute(Settings.EMPTY, env); - assertThat(status, is(CliTool.ExitStatus.OK)); - byte[] bytes = Files.readAllBytes(path); - assertThat(bytes.length, is(InternalCryptoService.KEY_SIZE / 8)); + Path keyPath = XPackPlugin.resolveConfigFile(env, "system_key"); + Files.createDirectories(keyPath.getParent()); + new SystemKeyTool(env).execute(terminal, (Path) null); + byte[] bytes = Files.readAllBytes(keyPath); + assertEquals(InternalCryptoService.KEY_SIZE / 8, bytes.length); } public void testThatSystemKeyMayOnlyBeReadByOwner() throws Exception { assumeTrue("test cannot run with security manager enabled", System.getSecurityManager() == null); - Path config = createTempDir(); - Path shieldConfig = config.resolve(Shield.NAME); - Files.createDirectories(shieldConfig); - Path path = shieldConfig.resolve("system_key"); + Path path = createTempDir().resolve("key"); + boolean isPosix = Files.getFileAttributeView(path.getParent(), PosixFileAttributeView.class) != null; + assumeTrue("posix filesystem", isPosix); - // no posix file permissions, nothing to test, done here - boolean supportsPosixPermissions = Environment.getFileStore(shieldConfig).supportsFileAttributeView(PosixFileAttributeView.class); - assumeTrue("Ignoring because posix file attributes are not supported", supportsPosixPermissions); - - when(env.configFile()).thenReturn(config); - Generate generate = new Generate(terminal, null); - CliTool.ExitStatus status = generate.execute(Settings.EMPTY, env); - assertThat(status, is(CliTool.ExitStatus.OK)); - - Set posixFilePermissions = Files.getPosixFilePermissions(path); - assertThat(posixFilePermissions, hasSize(2)); - assertThat(posixFilePermissions, containsInAnyOrder(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE)); + new SystemKeyTool(env).execute(terminal, path); + Set perms = Files.getPosixFilePermissions(path); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_READ)); + assertTrue(perms.toString(), perms.contains(PosixFilePermission.OWNER_WRITE)); + assertEquals(perms.toString(), 2, perms.size()); } }