From 05a01332fef304cc73675a1fc48c8af6cc8a7fae Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 2 Oct 2019 15:23:17 +0100 Subject: [PATCH 01/16] Support ELASTIC_PASSWORD_FILE in Docker entrypoint Closes #43603. Allow a password to be specifed in an ES Docker container by specifying a file path in the ELASTIC_PASSWORD_FILE environment variable. --- .../src/docker/bin/docker-entrypoint.sh | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index 0fb5aa61e7cee..c83504805fddf 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -76,6 +76,29 @@ done < <(env) export ES_JAVA_OPTS="-Des.cgroups.hierarchy.override=/ $ES_JAVA_OPTS" if [[ -f bin/elasticsearch-users ]]; then + # Source the password from a file, if specified. This means that the + # password itself doesn't need to be specified as an env var when running + # the container. + if [[ -n "$ELASTIC_PASSWORD_FILE" ]]; then + if [[ -n "$ELASTIC_PASSWORD" ]]; then + echo "ERROR: Both ELASTIC_PASSWORD and ELASTIC_PASSWORD_FILE are set. These are mutually exclusive." >&2 + exit 1 + fi + + if [[ ! -e "$ELASTIC_PASSWORD_FILE" ]]; then + echo "ERROR: Password file $ELASTIC_PASSWORD_FILE does not exist" >&2 + exit 1 + fi + + if [[ ! -r "$ELASTIC_PASSWORD_FILE" ]]; then + echo "ERROR: Password file $ELASTIC_PASSWORD_FILE is not readable" >&2 + exit 1 + fi + + ELASTIC_PASSWORD="$(cat "$ELASTIC_PASSWORD_FILE")" + unset ELASTIC_PASSWORD_FILE + fi + # Check for the ELASTIC_PASSWORD environment variable to set the # bootstrap password for Security. # From 65974f96130a4283478b6c4027ee590277ad8a62 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 2 Oct 2019 16:56:59 +0100 Subject: [PATCH 02/16] Support all envs vars with a _FILE suffix Expand support for populating environment variables from file via vars with a _FILE suffix. --- .../src/docker/bin/docker-entrypoint.sh | 55 +++++++++++-------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index c83504805fddf..106f83e90d7b1 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -38,6 +38,38 @@ if [[ "$1" != "eswrapper" ]]; then fi fi +# Allow environment variables to be set by creating a file with the +# contents, and setting an environment variable with the suffix _FILE to +# point to it. This can be used to provide secrets to a container, without +# the values being specified explicitly when running the container. +for VAR_NAME_FILE in $(env | cut -f1 -d= | grep '_FILE$'); do + if [[ -n "$VAR_NAME_FILE" ]]; then + VAR_NAME="${VAR_NAME_FILE%_FILE}" + + if [[ -n "${!VAR_NAME}" ]]; then + echo "ERROR: Both $VAR_NAME_FILE and $VAR_NAME are set. These are mutually exclusive." >&2 + exit 1 + fi + + if [[ ! -e "${!VAR_NAME_FILE}" ]]; then + echo "ERROR: File ${!VAR_NAME_FILE} does not exist" >&2 + exit 1 + fi + + if [[ ! -r "${!VAR_NAME_FILE}" ]]; then + echo "ERROR: File ${!VAR_NAME_FILE} is not readable" >&2 + exit 1 + fi + + echo "Setting $VAR_NAME from ${!VAR_NAME_FILE}" >&2 + export "$VAR_NAME"="$(cat ${!VAR_NAME_FILE})" + + unset VAR_NAME + # Unset the suffixed environment variable + unset "$VAR_NAME_FILE" + fi +done + # Parse Docker env vars to customize Elasticsearch # # e.g. Setting the env var cluster.name=testcluster @@ -76,29 +108,6 @@ done < <(env) export ES_JAVA_OPTS="-Des.cgroups.hierarchy.override=/ $ES_JAVA_OPTS" if [[ -f bin/elasticsearch-users ]]; then - # Source the password from a file, if specified. This means that the - # password itself doesn't need to be specified as an env var when running - # the container. - if [[ -n "$ELASTIC_PASSWORD_FILE" ]]; then - if [[ -n "$ELASTIC_PASSWORD" ]]; then - echo "ERROR: Both ELASTIC_PASSWORD and ELASTIC_PASSWORD_FILE are set. These are mutually exclusive." >&2 - exit 1 - fi - - if [[ ! -e "$ELASTIC_PASSWORD_FILE" ]]; then - echo "ERROR: Password file $ELASTIC_PASSWORD_FILE does not exist" >&2 - exit 1 - fi - - if [[ ! -r "$ELASTIC_PASSWORD_FILE" ]]; then - echo "ERROR: Password file $ELASTIC_PASSWORD_FILE is not readable" >&2 - exit 1 - fi - - ELASTIC_PASSWORD="$(cat "$ELASTIC_PASSWORD_FILE")" - unset ELASTIC_PASSWORD_FILE - fi - # Check for the ELASTIC_PASSWORD environment variable to set the # bootstrap password for Security. # From 2b90d970e419db9088b4a341ab74f8f518555813 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Tue, 8 Oct 2019 12:53:42 +0100 Subject: [PATCH 03/16] Include _FILE names when checking files in Docker In the Docker entrypoint, when using _FILE-suffixed env vars to populate environment variables, include the _FILE var name for context. --- distribution/docker/src/docker/bin/docker-entrypoint.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index 106f83e90d7b1..37e7784a328d5 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -52,16 +52,16 @@ for VAR_NAME_FILE in $(env | cut -f1 -d= | grep '_FILE$'); do fi if [[ ! -e "${!VAR_NAME_FILE}" ]]; then - echo "ERROR: File ${!VAR_NAME_FILE} does not exist" >&2 + echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE does not exist" >&2 exit 1 fi if [[ ! -r "${!VAR_NAME_FILE}" ]]; then - echo "ERROR: File ${!VAR_NAME_FILE} is not readable" >&2 + echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE is not readable" >&2 exit 1 fi - echo "Setting $VAR_NAME from ${!VAR_NAME_FILE}" >&2 + echo "Setting $VAR_NAME from $VAR_NAME_FILE at ${!VAR_NAME_FILE}" >&2 export "$VAR_NAME"="$(cat ${!VAR_NAME_FILE})" unset VAR_NAME From 7e361ff790d350263e6d60a8fa6028c8053d05af Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 9 Oct 2019 09:54:40 +0100 Subject: [PATCH 04/16] Trying to write tests --- .../gradle/test/DistroTestPlugin.java | 2 +- distribution/docker/build.gradle | 3 + .../packaging/test/DockerTests.java | 58 ++++++++++++++++++- .../elasticsearch/packaging/util/Docker.java | 44 ++++++++------ .../packaging/util/ServerUtils.java | 32 +++++++--- 5 files changed, 112 insertions(+), 27 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java index 2f254b30466a3..ad4238a1d877f 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java @@ -320,7 +320,7 @@ private List configureDistributions(Project project, List upgradeDistros = new ArrayList<>(); // Docker disabled for https://github.com/elastic/elasticsearch/issues/47639 - for (Type type : Arrays.asList(Type.DEB, Type.RPM /*,Type.DOCKER*/)) { + for (Type type : Arrays.asList(Type.DEB, Type.RPM, Type.DOCKER)) { for (Flavor flavor : Flavor.values()) { for (boolean bundledJdk : Arrays.asList(true, false)) { // We should never add a Docker distro with bundledJdk == false diff --git a/distribution/docker/build.gradle b/distribution/docker/build.gradle index 0905bde750d00..4aca8a0c16d9a 100644 --- a/distribution/docker/build.gradle +++ b/distribution/docker/build.gradle @@ -202,6 +202,9 @@ subprojects { Project subProject -> def tarFile = "${parent.projectDir}/build/elasticsearch${oss ? '-oss' : ''}_test.${VersionProperties.elasticsearch}.docker.tar" final Task exportDockerImageTask = task(exportTaskName, type: LoggedExec) { + outputs.file(tarFile) + outputs.cacheIf { true } + executable 'docker' args "save", "-o", diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index daad67f7fb111..819b8bb2254c4 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -48,14 +48,16 @@ import static org.elasticsearch.packaging.util.FileUtils.getTempDir; import static org.elasticsearch.packaging.util.FileUtils.mkdir; import static org.elasticsearch.packaging.util.FileUtils.rm; +import static org.elasticsearch.packaging.util.ServerUtils.makeAuthenticatedRequest; import static org.elasticsearch.packaging.util.ServerUtils.makeRequest; import static org.elasticsearch.packaging.util.ServerUtils.waitForElasticsearch; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.emptyString; +import static org.hamcrest.Matchers.equalTo; import static org.junit.Assume.assumeTrue; -@Ignore("https://github.com/elastic/elasticsearch/issues/47639") +//@Ignore("https://github.com/elastic/elasticsearch/issues/47639") public class DockerTests extends PackagingTestCase { protected DockerShell sh; @@ -166,7 +168,8 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { // Restart the container removeContainer(); - runContainer(distribution(), tempConf, Map.of( + final Map volumes = Map.of(tempConf, Path.of("/usr/share/elasticsearch/config")); + runContainer(distribution(), volumes, Map.of( "ES_JAVA_OPTS", "-XX:-UseCompressedOops" )); @@ -180,6 +183,57 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { } } + /** + * Check that environment variables can be populated by setting variables with the suffix "_FILE", + * which point to files that hold the required values. + */ + public void test80SetEnvironmentVariablesUsingFiles() throws Exception { + final String xpackPassword = "hunter2"; + final Path secretsDir = getTempDir().resolve("secrets"); + + try { + mkdir(secretsDir); + + // ES_JAVA_OPTS_FILE + Files.writeString(secretsDir.resolve("esJavaOpts.txt"), "-XX:-UseCompressedOops\n"); + + // ELASTIC_PASSWORD_FILE + Files.writeString(secretsDir.resolve("password.txt"), xpackPassword + "\n"); + + // Make the temp directory and contents accessible when bind-mounted + Files.setPosixFilePermissions(secretsDir, fromString("rwxrwxrwx")); + + Map envVars = Map.of( + "ES_JAVA_OPTS_FILE", "/run/secrets/esJavaOptsPath.txt", + "ELASTIC_PASSWORD_FILE", "/run/secrets/password.txt", + // Enable security so that we can test that the password has been used + "xpack.security.enabled", "true" + ); + + final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); + + // Restart the container + removeContainer(); + runContainer(distribution(), volumes, envVars); + + // If we configured security correctly, then this call will only work if we specify the correct credentials. + waitForElasticsearch("green", null, installation, "elastic", "hunter2"); + + // Try to call `/_nodes` first without authentication, and ensure it's rejected. + final int statusCode = Request.Get("http://localhost:9200/_nodes").execute().returnResponse().getStatusLine().getStatusCode(); + assertThat("Expected server to require authentication", statusCode, equalTo(401)); + + // Now try again, but with credentials + final String nodesResponse = makeAuthenticatedRequest( + Request.Get("http://localhost:9200/_nodes"), + "elastic", + xpackPassword); + assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); + } finally { + rm(secretsDir); + } + } + /** * Check whether the elasticsearch-certutil tool has been shipped correctly, * and if present then it can execute. diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index d78b60236bc4a..d074d2f8da724 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -25,7 +25,6 @@ import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermission; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; @@ -80,7 +79,7 @@ public static void ensureImageIsLoaded(Distribution distribution) { * @param distribution details about the docker image being tested. */ public static Installation runContainer(Distribution distribution) throws Exception { - return runContainer(distribution, null, Collections.emptyMap()); + return runContainer(distribution, null, null); } /** @@ -88,10 +87,14 @@ public static Installation runContainer(Distribution distribution) throws Except * through a bind mount, and passing additional environment variables. * * @param distribution details about the docker image being tested. - * @param configPath the path to the config to bind mount, or null - * @param envVars environment variables to set when running the container + * @param volumes a map that declares any volume mappings to apply, or null + * @param envVars environment variables to set when running the container, or null */ - public static Installation runContainer(Distribution distribution, Path configPath, Map envVars) throws Exception { + public static Installation runContainer( + Distribution distribution, + Map volumes, + Map envVars + ) throws Exception { removeContainer(); final List args = new ArrayList<>(); @@ -104,7 +107,9 @@ public static Installation runContainer(Distribution distribution, Path configPa // Run the container in the background args.add("--detach"); - envVars.forEach((key, value) -> args.add("--env " + key + "=\"" + value + "\"")); + if (envVars != null) { + envVars.forEach((key, value) -> args.add("--env " + key + "=\"" + value + "\"")); + } // The container won't run without configuring discovery args.add("--env discovery.type=single-node"); @@ -113,9 +118,9 @@ public static Installation runContainer(Distribution distribution, Path configPa args.add("--publish 9200:9200"); args.add("--publish 9300:9300"); - if (configPath != null) { - // Bind-mount the config dir, if specified - args.add("--volume \"" + configPath + ":/usr/share/elasticsearch/config\""); + // Bind-mount any volumes + if (volumes != null) { + volumes.forEach((localPath, containerPath) -> args.add("--volume \"" + localPath + ":" + containerPath + "\"")); } args.add(distribution.flavor.name + ":test"); @@ -133,19 +138,24 @@ public static Installation runContainer(Distribution distribution, Path configPa * Waits for the Elasticsearch process to start executing in the container. * This is called every time a container is started. */ - private static void waitForElasticsearchToStart() throws InterruptedException { + private static void waitForElasticsearchToStart() { boolean isElasticsearchRunning = false; int attempt = 0; do { - String psOutput = dockerShell.run("ps ax").stdout; + try { + String psOutput = dockerShell.run("ps ax").stdout; - if (psOutput.contains("/usr/share/elasticsearch/jdk/bin/java -X")) { - isElasticsearchRunning = true; - break; - } + if (psOutput.contains("/usr/share/elasticsearch/jdk/bin/java -X")) { + isElasticsearchRunning = true; + break; + } - Thread.sleep(1000); + Thread.sleep(1000); + } + catch (Exception e) { + logger.info("Caught exception while waiting for ES to start", e); + } } while (attempt++ < 5); if (!isElasticsearchRunning) { @@ -167,7 +177,7 @@ public static void removeContainer() { if (result.isSuccess() == false) { // I'm not sure why we're already removing this container, but that's OK. - if (result.stderr.contains("removal of container " + " is already in progress") == false) { + if (result.stderr.contains("removal of container " + containerId + " is already in progress") == false) { throw new RuntimeException( "Command was not successful: [" + command + "] result: " + result.toString()); } diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java index 9d91b2a15bdfb..ec6af075e7672 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java @@ -27,6 +27,7 @@ import org.apache.logging.log4j.Logger; import java.io.IOException; +import java.util.Base64; import java.util.Objects; import java.util.concurrent.TimeUnit; @@ -41,10 +42,16 @@ public class ServerUtils { private static final long timeoutLength = TimeUnit.SECONDS.toMillis(10); public static void waitForElasticsearch(Installation installation) throws IOException { - waitForElasticsearch("green", null, installation); + waitForElasticsearch("green", null, installation, null, null); } - public static void waitForElasticsearch(String status, String index, Installation installation) throws IOException { + public static void waitForElasticsearch( + String status, + String index, + Installation installation, + String username, + String password + ) throws IOException { Objects.requireNonNull(status); @@ -55,11 +62,16 @@ public static void waitForElasticsearch(String status, String index, Installatio while (started == false && timeElapsed < waitTime) { try { - final HttpResponse response = Request.Get("http://localhost:9200/_cluster/health") + final Request request = Request.Get("http://localhost:9200/_cluster/health") .connectTimeout((int) timeoutLength) - .socketTimeout((int) timeoutLength) - .execute() - .returnResponse(); + .socketTimeout((int) timeoutLength); + + if (username != null) { + final String encodedCredentials = Base64.getEncoder().encodeToString((username + ":" + password).getBytes()); + request.setHeader("Authorization", "Basic " + encodedCredentials); + } + + final HttpResponse response = request.execute().returnResponse(); if (response.getStatusLine().getStatusCode() >= 300) { final String statusLine = response.getStatusLine().toString(); @@ -110,6 +122,13 @@ public static void runElasticsearchTests() throws IOException { makeRequest(Request.Delete("http://localhost:9200/_all")); } + public static String makeAuthenticatedRequest(Request request, String username, String password) throws IOException { + final String encodedCredentials = Base64.getEncoder().encodeToString((username + ":" + password).getBytes()); + request.setHeader("Authorization", "Basic " + encodedCredentials); + + return makeRequest(request); + } + public static String makeRequest(Request request) throws IOException { final HttpResponse response = request.execute().returnResponse(); final String body = EntityUtils.toString(response.getEntity()); @@ -119,6 +138,5 @@ public static String makeRequest(Request request) throws IOException { } return body; - } } From 64175d22ed3dbfeff226d72cb68150cc85c815aa Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 9 Oct 2019 14:24:19 +0100 Subject: [PATCH 05/16] Fix up new Docker test --- .../packaging/test/DockerTests.java | 20 +++++---- .../elasticsearch/packaging/util/Docker.java | 8 ++-- .../packaging/util/ServerUtils.java | 44 +++++++++++-------- 3 files changed, 40 insertions(+), 32 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 819b8bb2254c4..024c46027add7 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -48,7 +48,6 @@ import static org.elasticsearch.packaging.util.FileUtils.getTempDir; import static org.elasticsearch.packaging.util.FileUtils.mkdir; import static org.elasticsearch.packaging.util.FileUtils.rm; -import static org.elasticsearch.packaging.util.ServerUtils.makeAuthenticatedRequest; import static org.elasticsearch.packaging.util.ServerUtils.makeRequest; import static org.elasticsearch.packaging.util.ServerUtils.waitForElasticsearch; import static org.hamcrest.CoreMatchers.containsString; @@ -57,7 +56,7 @@ import static org.hamcrest.Matchers.equalTo; import static org.junit.Assume.assumeTrue; -//@Ignore("https://github.com/elastic/elasticsearch/issues/47639") +@Ignore("https://github.com/elastic/elasticsearch/issues/47639") public class DockerTests extends PackagingTestCase { protected DockerShell sh; @@ -190,22 +189,24 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { public void test80SetEnvironmentVariablesUsingFiles() throws Exception { final String xpackPassword = "hunter2"; final Path secretsDir = getTempDir().resolve("secrets"); + final String optionsFilename = "esJavaOpts.txt"; + final String passwordFilename = "password.txt"; try { mkdir(secretsDir); // ES_JAVA_OPTS_FILE - Files.writeString(secretsDir.resolve("esJavaOpts.txt"), "-XX:-UseCompressedOops\n"); + Files.writeString(secretsDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); // ELASTIC_PASSWORD_FILE - Files.writeString(secretsDir.resolve("password.txt"), xpackPassword + "\n"); + Files.writeString(secretsDir.resolve(passwordFilename), xpackPassword + "\n"); // Make the temp directory and contents accessible when bind-mounted Files.setPosixFilePermissions(secretsDir, fromString("rwxrwxrwx")); Map envVars = Map.of( - "ES_JAVA_OPTS_FILE", "/run/secrets/esJavaOptsPath.txt", - "ELASTIC_PASSWORD_FILE", "/run/secrets/password.txt", + "ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename, + "ELASTIC_PASSWORD_FILE", "/run/secrets/" + passwordFilename, // Enable security so that we can test that the password has been used "xpack.security.enabled", "true" ); @@ -213,21 +214,22 @@ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); // Restart the container - removeContainer(); runContainer(distribution(), volumes, envVars); // If we configured security correctly, then this call will only work if we specify the correct credentials. waitForElasticsearch("green", null, installation, "elastic", "hunter2"); // Try to call `/_nodes` first without authentication, and ensure it's rejected. + // We don't use the `makeRequest()` helper as it checks the status code. final int statusCode = Request.Get("http://localhost:9200/_nodes").execute().returnResponse().getStatusLine().getStatusCode(); assertThat("Expected server to require authentication", statusCode, equalTo(401)); - // Now try again, but with credentials - final String nodesResponse = makeAuthenticatedRequest( + // Now try again, but with credentials, to check that ES_JAVA_OPTS_FILE took effect + final String nodesResponse = makeRequest( Request.Get("http://localhost:9200/_nodes"), "elastic", xpackPassword); + assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); } finally { rm(secretsDir); diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index d074d2f8da724..6aeb1ef36c353 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -94,7 +94,7 @@ public static Installation runContainer( Distribution distribution, Map volumes, Map envVars - ) throws Exception { + ) { removeContainer(); final List args = new ArrayList<>(); @@ -126,7 +126,7 @@ public static Installation runContainer( args.add(distribution.flavor.name + ":test"); final String command = String.join(" ", args); - logger.debug("Running command: " + command); + logger.info("Running command: " + command); containerId = sh.run(command).stdout.trim(); waitForElasticsearchToStart(); @@ -154,11 +154,11 @@ private static void waitForElasticsearchToStart() { Thread.sleep(1000); } catch (Exception e) { - logger.info("Caught exception while waiting for ES to start", e); + logger.debug("Caught exception while waiting for ES to start", e); } } while (attempt++ < 5); - if (!isElasticsearchRunning) { + if (isElasticsearchRunning == false) { final String logs = sh.run("docker logs " + containerId).stdout; fail("Elasticsearch container did start successfully.\n\n" + logs); } diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java index ec6af075e7672..0a0f28d6d2894 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java @@ -19,7 +19,9 @@ package org.elasticsearch.packaging.util; +import org.apache.http.HttpHost; import org.apache.http.HttpResponse; +import org.apache.http.client.fluent.Executor; import org.apache.http.client.fluent.Request; import org.apache.http.entity.ContentType; import org.apache.http.util.EntityUtils; @@ -27,7 +29,6 @@ import org.apache.logging.log4j.Logger; import java.io.IOException; -import java.util.Base64; import java.util.Objects; import java.util.concurrent.TimeUnit; @@ -36,7 +37,7 @@ public class ServerUtils { - protected static final Logger logger = LogManager.getLogger(ServerUtils.class); + private static final Logger logger = LogManager.getLogger(ServerUtils.class); private static final long waitTime = TimeUnit.SECONDS.toMillis(60); private static final long timeoutLength = TimeUnit.SECONDS.toMillis(10); @@ -45,6 +46,17 @@ public static void waitForElasticsearch(Installation installation) throws IOExce waitForElasticsearch("green", null, installation, null, null); } + private static HttpResponse execute(Request request, String username, String password) throws IOException { + final Executor executor = Executor.newInstance(); + + if (username != null) { + executor.auth(username, password); + executor.authPreemptive(new HttpHost("localhost", 9200)); + } + + return executor.execute(request).returnResponse(); + } + public static void waitForElasticsearch( String status, String index, @@ -59,19 +71,15 @@ public static void waitForElasticsearch( final long startTime = System.currentTimeMillis(); long timeElapsed = 0; boolean started = false; + Exception lastException = null; + while (started == false && timeElapsed < waitTime) { try { - final Request request = Request.Get("http://localhost:9200/_cluster/health") .connectTimeout((int) timeoutLength) .socketTimeout((int) timeoutLength); - if (username != null) { - final String encodedCredentials = Base64.getEncoder().encodeToString((username + ":" + password).getBytes()); - request.setHeader("Authorization", "Basic " + encodedCredentials); - } - - final HttpResponse response = request.execute().returnResponse(); + final HttpResponse response = execute(request, username, password); if (response.getStatusLine().getStatusCode() >= 300) { final String statusLine = response.getStatusLine().toString(); @@ -82,7 +90,8 @@ public static void waitForElasticsearch( started = true; } catch (IOException e) { - logger.info("Got exception when waiting for cluster health", e); + lastException = e; + logger.debug("Got exception when waiting for cluster health", e); } timeElapsed = System.currentTimeMillis() - startTime; @@ -92,7 +101,7 @@ public static void waitForElasticsearch( if (installation != null) { FileUtils.logAllLogs(installation.logs, logger); } - throw new RuntimeException("Elasticsearch did not start"); + throw new RuntimeException("Elasticsearch did not start", lastException); } final String url; @@ -103,7 +112,7 @@ public static void waitForElasticsearch( } - final String body = makeRequest(Request.Get(url)); + final String body = makeRequest(Request.Get(url), username, password); assertThat("cluster health response must contain desired status", body, containsString(status)); } @@ -122,15 +131,12 @@ public static void runElasticsearchTests() throws IOException { makeRequest(Request.Delete("http://localhost:9200/_all")); } - public static String makeAuthenticatedRequest(Request request, String username, String password) throws IOException { - final String encodedCredentials = Base64.getEncoder().encodeToString((username + ":" + password).getBytes()); - request.setHeader("Authorization", "Basic " + encodedCredentials); - - return makeRequest(request); + public static String makeRequest(Request request) throws IOException { + return makeRequest(request, null, null); } - public static String makeRequest(Request request) throws IOException { - final HttpResponse response = request.execute().returnResponse(); + public static String makeRequest(Request request, String username, String password) throws IOException { + final HttpResponse response = execute(request, username, password); final String body = EntityUtils.toString(response.getEntity()); if (response.getStatusLine().getStatusCode() >= 300) { From 9b940cf2d350a25665513d105ec1afe3f93d0886 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 9 Oct 2019 14:25:54 +0100 Subject: [PATCH 06/16] Revert accidental changes --- .../groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java | 2 +- distribution/docker/build.gradle | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java index ad4238a1d877f..2f254b30466a3 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/DistroTestPlugin.java @@ -320,7 +320,7 @@ private List configureDistributions(Project project, List upgradeDistros = new ArrayList<>(); // Docker disabled for https://github.com/elastic/elasticsearch/issues/47639 - for (Type type : Arrays.asList(Type.DEB, Type.RPM, Type.DOCKER)) { + for (Type type : Arrays.asList(Type.DEB, Type.RPM /*,Type.DOCKER*/)) { for (Flavor flavor : Flavor.values()) { for (boolean bundledJdk : Arrays.asList(true, false)) { // We should never add a Docker distro with bundledJdk == false diff --git a/distribution/docker/build.gradle b/distribution/docker/build.gradle index 4aca8a0c16d9a..0905bde750d00 100644 --- a/distribution/docker/build.gradle +++ b/distribution/docker/build.gradle @@ -202,9 +202,6 @@ subprojects { Project subProject -> def tarFile = "${parent.projectDir}/build/elasticsearch${oss ? '-oss' : ''}_test.${VersionProperties.elasticsearch}.docker.tar" final Task exportDockerImageTask = task(exportTaskName, type: LoggedExec) { - outputs.file(tarFile) - outputs.cacheIf { true } - executable 'docker' args "save", "-o", From 7ca020cc903758d1c9dd851cb06252a4a710225c Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 9 Oct 2019 14:28:04 +0100 Subject: [PATCH 07/16] Tweaks --- .../org/elasticsearch/packaging/util/ServerUtils.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java index 0a0f28d6d2894..8d20b7b712b6f 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java @@ -46,10 +46,19 @@ public static void waitForElasticsearch(Installation installation) throws IOExce waitForElasticsearch("green", null, installation, null, null); } + /** + * Executes the supplied request, optionally applying HTTP basic auth if the + * username and pasword field are supplied. + * @param request the request to execute + * @param username the username to supply, or null + * @param password the password to supply, or null + * @return the response from the server + * @throws IOException if an error occurs + */ private static HttpResponse execute(Request request, String username, String password) throws IOException { final Executor executor = Executor.newInstance(); - if (username != null) { + if (username != null && password != null) { executor.auth(username, password); executor.authPreemptive(new HttpHost("localhost", 9200)); } From e564576a0555d3d4033b3d0ac730e6f19c110c8a Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Tue, 5 Nov 2019 11:50:49 +0000 Subject: [PATCH 08/16] Post-merge fixes --- .../org/elasticsearch/packaging/util/Docker.java | 4 ++-- .../elasticsearch/packaging/util/ServerUtils.java | 15 +++++++++------ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index f5290157bb0f0..981cfa7c77dea 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -142,14 +142,14 @@ private static void waitForElasticsearchToStart() { boolean isElasticsearchRunning = false; int attempt = 0; - String psOutput; + String psOutput = null; do { try { // Give the container a chance to crash out Thread.sleep(1000); - String psOutput = dockerShell.run("ps ax").stdout; + psOutput = dockerShell.run("ps ax").stdout; if (psOutput.contains("/usr/share/elasticsearch/jdk/bin/java")) { isElasticsearchRunning = true; diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java index f8ed2c2ca8cd4..f45a386eb12c8 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/ServerUtils.java @@ -88,15 +88,19 @@ public static void waitForElasticsearch( long timeElapsed = 0; boolean started = false; Throwable thrownException = null; + while (started == false && timeElapsed < waitTime) { if (System.currentTimeMillis() - lastRequest > requestInterval) { try { - final HttpResponse response = Request.Get("http://localhost:9200/_cluster/health") - .connectTimeout((int) timeoutLength) - .socketTimeout((int) timeoutLength) - .execute() - .returnResponse(); + final HttpResponse response = execute( + Request + .Get("http://localhost:9200/_cluster/health") + .connectTimeout((int) timeoutLength) + .socketTimeout((int) timeoutLength), + username, + password + ); if (response.getStatusLine().getStatusCode() >= 300) { final String statusLine = response.getStatusLine().toString(); @@ -133,7 +137,6 @@ public static void waitForElasticsearch( url = "http://localhost:9200/_cluster/health?wait_for_status=" + status + "&timeout=60s&pretty"; } else { url = "http://localhost:9200/_cluster/health/" + index + "?wait_for_status=" + status + "&timeout=60s&pretty"; - } final String body = makeRequest(Request.Get(url), username, password); From 6f1cf8253f12a5b6f9b6a048834dec35956dad4b Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Tue, 5 Nov 2019 15:05:17 +0000 Subject: [PATCH 09/16] Check env var file permissions Require files that are being used to populate env vars to have restricted file permissions. This is to attempt to limit the possibility of crafting a malicious command line for ES. --- .../src/docker/bin/docker-entrypoint.sh | 7 ++ .../packaging/test/DockerTests.java | 47 ++++++++++- .../elasticsearch/packaging/util/Docker.java | 79 +++++++++++++++++-- .../packaging/util/FileMatcher.java | 1 + 4 files changed, 123 insertions(+), 11 deletions(-) diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index 37e7784a328d5..b4fad6c73d59e 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -61,6 +61,13 @@ for VAR_NAME_FILE in $(env | cut -f1 -d= | grep '_FILE$'); do exit 1 fi + FILE_PERMS="$(stat -c '%a' ${!VAR_NAME_FILE})" + + if [[ "$FILE_PERMS" != "400" && "$FILE_PERMS" != 600 ]]; then + echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE must have file permissions 400 or 600" >&2 + exit 1 + fi + echo "Setting $VAR_NAME from $VAR_NAME_FILE at ${!VAR_NAME_FILE}" >&2 export "$VAR_NAME"="$(cat ${!VAR_NAME_FILE})" diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 2dc46a00efda7..6905fbb4b0056 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -40,8 +40,10 @@ import static org.elasticsearch.packaging.util.Docker.existsInContainer; import static org.elasticsearch.packaging.util.Docker.removeContainer; import static org.elasticsearch.packaging.util.Docker.runContainer; +import static org.elasticsearch.packaging.util.Docker.runContainerExpectingFailure; import static org.elasticsearch.packaging.util.Docker.verifyContainerInstallation; import static org.elasticsearch.packaging.util.Docker.waitForPathToExist; +import static org.elasticsearch.packaging.util.FileMatcher.p600; import static org.elasticsearch.packaging.util.FileMatcher.p660; import static org.elasticsearch.packaging.util.FileUtils.append; import static org.elasticsearch.packaging.util.FileUtils.getTempDir; @@ -72,7 +74,7 @@ public static void cleanup() { } @Before - public void setupTest() throws Exception { + public void setupTest() { sh = new DockerShell(); installation = runContainer(distribution()); } @@ -199,9 +201,6 @@ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { // ELASTIC_PASSWORD_FILE Files.writeString(secretsDir.resolve(passwordFilename), xpackPassword + "\n"); - // Make the temp directory and contents accessible when bind-mounted - Files.setPosixFilePermissions(secretsDir, fromString("rwxrwxrwx")); - Map envVars = Map.of( "ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename, "ELASTIC_PASSWORD_FILE", "/run/secrets/" + passwordFilename, @@ -209,6 +208,11 @@ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { "xpack.security.enabled", "true" ); + // File permissions need to be secured in order for the ES wrapper to accept + // them for populating env var values + Files.setPosixFilePermissions(secretsDir.resolve(optionsFilename), p600); + Files.setPosixFilePermissions(secretsDir.resolve(passwordFilename), p600); + final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); // Restart the container @@ -234,6 +238,41 @@ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { } } + /** + * Check that when populating environment variables by setting variables with the suffix "_FILE", + * the files' permissions are checked. + */ + public void test81EnvironmentVariablesUsingFilesHaveCorrectPermissions() throws Exception { + final Path secretsDir = getTempDir().resolve("secrets"); + final String optionsFilename = "esJavaOpts.txt"; + + try { + mkdir(secretsDir); + + // ES_JAVA_OPTS_FILE + Files.writeString(secretsDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); + + Map envVars = Map.of("ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename); + + // Set invalid file permissions + Files.setPosixFilePermissions(secretsDir.resolve(optionsFilename), p660); + + final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); + + // Restart the container + final Result dockerLogs = runContainerExpectingFailure(distribution(), volumes, envVars); + + assertThat( + dockerLogs.stderr, + containsString( + "ERROR: File /run/secrets/" + optionsFilename + " from ES_JAVA_OPTS_FILE must have file permissions 400 or 600" + ) + ); + } finally { + rm(secretsDir); + } + } + /** * Check whether the elasticsearch-certutil tool has been shipped correctly, * and if present then it can execute. diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index 981cfa7c77dea..6e8c549877001 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -78,7 +78,7 @@ public static void ensureImageIsLoaded(Distribution distribution) { * Runs an Elasticsearch Docker container. * @param distribution details about the docker image being tested. */ - public static Installation runContainer(Distribution distribution) throws Exception { + public static Installation runContainer(Distribution distribution) { return runContainer(distribution, null, null); } @@ -94,6 +94,40 @@ public static Installation runContainer( Distribution distribution, Map volumes, Map envVars + ) { + executeDockerRun(distribution, Cleanup.ALWAYS, volumes, envVars); + + waitForElasticsearchToStart(); + + return Installation.ofContainer(); + } + + /** + * Similar to {@link #runContainer(Distribution, Map, Map)} in that it runs an Elasticsearch Docker + * container, expect that the container expecting it to exit e.g. due to configuration problem. + * + * @param distribution details about the docker image being tested. + * @param volumes a map that declares any volume mappings to apply, or null + * @param envVars environment variables to set when running the container, or null + * @return the docker logs of the container + */ + public static Shell.Result runContainerExpectingFailure( + Distribution distribution, + Map volumes, + Map envVars + ) { + executeDockerRun(distribution, Cleanup.NEVER, volumes, envVars); + + waitForElasticsearchToExit(); + + return sh.run("docker logs " + containerId); + } + + private static void executeDockerRun( + Distribution distribution, + Cleanup cleanupMode, + Map volumes, + Map envVars ) { removeContainer(); @@ -101,8 +135,10 @@ public static Installation runContainer( args.add("docker run"); - // Remove the container once it exits - args.add("--rm"); + if (cleanupMode == Cleanup.ALWAYS) { + // Remove the container once it exits + args.add("--rm"); + } // Run the container in the background args.add("--detach"); @@ -128,10 +164,6 @@ public static Installation runContainer( final String command = String.join(" ", args); logger.info("Running command: " + command); containerId = sh.run(command).stdout.trim(); - - waitForElasticsearchToStart(); - - return Installation.ofContainer(); } /** @@ -167,6 +199,34 @@ private static void waitForElasticsearchToStart() { } } + /** + * Waits for the Elasticsearch container to exit. + */ + private static void waitForElasticsearchToExit() { + boolean isElasticsearchRunning = true; + int attempt = 0; + + do { + try { + // Give the container a chance to exit out + Thread.sleep(1000); + + if (sh.run("docker ps --quiet --no-trunc").stdout.contains(containerId) == false) { + isElasticsearchRunning = false; + break; + } + } + catch (Exception e) { + logger.warn("Caught exception while waiting for ES to exit", e); + } + } while (attempt++ < 5); + + if (isElasticsearchRunning) { + final String dockerLogs = sh.run("docker logs " + containerId).stdout; + fail("Elasticsearch container did exit.\n\n" + dockerLogs); + } + } + /** * Removes the currently running container. */ @@ -365,4 +425,9 @@ private static void verifyDefaultInstallation(Installation es) { "users_roles" ).forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p660)); } + + private enum Cleanup { + ALWAYS, + NEVER + } } diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/FileMatcher.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/FileMatcher.java index 89113ae098ea2..ba3671500e931 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/FileMatcher.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/FileMatcher.java @@ -50,6 +50,7 @@ public enum Fileness { File, Directory } public static final Set p750 = fromString("rwxr-x---"); public static final Set p660 = fromString("rw-rw----"); public static final Set p644 = fromString("rw-r--r--"); + public static final Set p600 = fromString("rw-------"); private final Fileness fileness; private final String owner; From 856270305660f6d951ca6a841c06256a281df540 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Wed, 6 Nov 2019 11:48:40 +0000 Subject: [PATCH 10/16] Capture more logging on failure --- .../packaging/test/DockerTests.java | 3 +- .../elasticsearch/packaging/util/Docker.java | 200 +++++++++--------- 2 files changed, 97 insertions(+), 106 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 6905fbb4b0056..3867ad7a5bcbd 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -42,6 +42,7 @@ import static org.elasticsearch.packaging.util.Docker.runContainer; import static org.elasticsearch.packaging.util.Docker.runContainerExpectingFailure; import static org.elasticsearch.packaging.util.Docker.verifyContainerInstallation; +import static org.elasticsearch.packaging.util.Docker.waitForElasticsearch; import static org.elasticsearch.packaging.util.Docker.waitForPathToExist; import static org.elasticsearch.packaging.util.FileMatcher.p600; import static org.elasticsearch.packaging.util.FileMatcher.p660; @@ -50,7 +51,6 @@ import static org.elasticsearch.packaging.util.FileUtils.mkdir; import static org.elasticsearch.packaging.util.FileUtils.rm; import static org.elasticsearch.packaging.util.ServerUtils.makeRequest; -import static org.elasticsearch.packaging.util.ServerUtils.waitForElasticsearch; import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.Matchers.emptyString; @@ -166,7 +166,6 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { Files.setPosixFilePermissions(tempConf, fromString("rwxrwxrwx")); // Restart the container - removeContainer(); final Map volumes = Map.of(tempConf, Path.of("/usr/share/elasticsearch/config")); runContainer(distribution(), volumes, Map.of( "ES_JAVA_OPTS", "-XX:-UseCompressedOops" diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java index 6e8c549877001..3920fc9bb3892 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/util/Docker.java @@ -90,12 +90,8 @@ public static Installation runContainer(Distribution distribution) { * @param volumes a map that declares any volume mappings to apply, or null * @param envVars environment variables to set when running the container, or null */ - public static Installation runContainer( - Distribution distribution, - Map volumes, - Map envVars - ) { - executeDockerRun(distribution, Cleanup.ALWAYS, volumes, envVars); + public static Installation runContainer(Distribution distribution, Map volumes, Map envVars) { + executeDockerRun(distribution, volumes, envVars); waitForElasticsearchToStart(); @@ -114,32 +110,22 @@ public static Installation runContainer( public static Shell.Result runContainerExpectingFailure( Distribution distribution, Map volumes, - Map envVars + Map envVars ) { - executeDockerRun(distribution, Cleanup.NEVER, volumes, envVars); + executeDockerRun(distribution, volumes, envVars); waitForElasticsearchToExit(); return sh.run("docker logs " + containerId); } - private static void executeDockerRun( - Distribution distribution, - Cleanup cleanupMode, - Map volumes, - Map envVars - ) { + private static void executeDockerRun(Distribution distribution, Map volumes, Map envVars) { removeContainer(); final List args = new ArrayList<>(); args.add("docker run"); - if (cleanupMode == Cleanup.ALWAYS) { - // Remove the container once it exits - args.add("--rm"); - } - // Run the container in the background args.add("--detach"); @@ -187,15 +173,21 @@ private static void waitForElasticsearchToStart() { isElasticsearchRunning = true; break; } - } - catch (Exception e) { + } catch (Exception e) { logger.warn("Caught exception while waiting for ES to start", e); } } while (attempt++ < 5); if (isElasticsearchRunning == false) { - final String dockerLogs = sh.run("docker logs " + containerId).stdout; - fail("Elasticsearch container did start successfully.\n\n" + psOutput + "\n\n" + dockerLogs); + final Shell.Result dockerLogs = sh.run("docker logs " + containerId); + fail( + "Elasticsearch container did not start successfully.\n\nps output:\n" + + psOutput + + "\n\nStdout:\n" + + dockerLogs.stdout + + "\n\nStderr:\n" + + dockerLogs.stderr + ); } } @@ -215,15 +207,14 @@ private static void waitForElasticsearchToExit() { isElasticsearchRunning = false; break; } - } - catch (Exception e) { + } catch (Exception e) { logger.warn("Caught exception while waiting for ES to exit", e); } } while (attempt++ < 5); if (isElasticsearchRunning) { - final String dockerLogs = sh.run("docker logs " + containerId).stdout; - fail("Elasticsearch container did exit.\n\n" + dockerLogs); + final Shell.Result dockerLogs = sh.run("docker logs " + containerId); + fail("Elasticsearch container did exit.\n\nStdout:\n" + dockerLogs.stdout + "\n\nStderr:\n" + dockerLogs.stderr); } } @@ -239,10 +230,12 @@ public static void removeContainer() { final Shell.Result result = sh.runIgnoreExitCode(command); if (result.isSuccess() == false) { + boolean isErrorAcceptable = result.stderr.contains("removal of container " + containerId + " is already in progress") + || result.stderr.contains("Error: No such container: " + containerId); + // I'm not sure why we're already removing this container, but that's OK. - if (result.stderr.contains("removal of container " + containerId + " is already in progress") == false) { - throw new RuntimeException( - "Command was not successful: [" + command + "] result: " + result.toString()); + if (isErrorAcceptable == false) { + throw new RuntimeException("Command was not successful: [" + command + "] result: " + result.toString()); } } } finally { @@ -273,11 +266,7 @@ public static class DockerShell extends Shell { protected String[] getScriptCommand(String script) { assert containerId != null; - return super.getScriptCommand("docker exec " + - "--user elasticsearch:root " + - "--tty " + - containerId + " " + - script); + return super.getScriptCommand("docker exec " + "--user elasticsearch:root " + "--tty " + containerId + " " + script); } } @@ -347,87 +336,90 @@ private static void verifyOssInstallation(Installation es) { final String homeDir = passwdResult.stdout.trim().split(":")[5]; assertThat(homeDir, equalTo("/usr/share/elasticsearch")); - Stream.of( - es.home, - es.data, - es.logs, - es.config - ).forEach(dir -> assertPermissionsAndOwnership(dir, p775)); + Stream.of(es.home, es.data, es.logs, es.config).forEach(dir -> assertPermissionsAndOwnership(dir, p775)); - Stream.of( - es.plugins, - es.modules - ).forEach(dir -> assertPermissionsAndOwnership(dir, p755)); + Stream.of(es.plugins, es.modules).forEach(dir -> assertPermissionsAndOwnership(dir, p755)); // FIXME these files should all have the same permissions - Stream.of( - "elasticsearch.keystore", -// "elasticsearch.yml", - "jvm.options" -// "log4j2.properties" - ).forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p660)); - - Stream.of( - "elasticsearch.yml", - "log4j2.properties" - ).forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p644)); - - assertThat( - dockerShell.run(es.bin("elasticsearch-keystore") + " list").stdout, - containsString("keystore.seed")); - - Stream.of( - es.bin, - es.lib - ).forEach(dir -> assertPermissionsAndOwnership(dir, p755)); - - Stream.of( - "elasticsearch", - "elasticsearch-cli", - "elasticsearch-env", - "elasticsearch-enve", - "elasticsearch-keystore", - "elasticsearch-node", - "elasticsearch-plugin", - "elasticsearch-shard" - ).forEach(executable -> assertPermissionsAndOwnership(es.bin(executable), p755)); - - Stream.of( - "LICENSE.txt", - "NOTICE.txt", - "README.textile" - ).forEach(doc -> assertPermissionsAndOwnership(es.home.resolve(doc), p644)); + Stream + .of( + "elasticsearch.keystore", + // "elasticsearch.yml", + "jvm.options" + // "log4j2.properties" + ) + .forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p660)); + + Stream + .of("elasticsearch.yml", "log4j2.properties") + .forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p644)); + + assertThat(dockerShell.run(es.bin("elasticsearch-keystore") + " list").stdout, containsString("keystore.seed")); + + Stream.of(es.bin, es.lib).forEach(dir -> assertPermissionsAndOwnership(dir, p755)); + + Stream + .of( + "elasticsearch", + "elasticsearch-cli", + "elasticsearch-env", + "elasticsearch-enve", + "elasticsearch-keystore", + "elasticsearch-node", + "elasticsearch-plugin", + "elasticsearch-shard" + ) + .forEach(executable -> assertPermissionsAndOwnership(es.bin(executable), p755)); + + Stream.of("LICENSE.txt", "NOTICE.txt", "README.textile").forEach(doc -> assertPermissionsAndOwnership(es.home.resolve(doc), p644)); } private static void verifyDefaultInstallation(Installation es) { - Stream.of( - "elasticsearch-certgen", - "elasticsearch-certutil", - "elasticsearch-croneval", - "elasticsearch-saml-metadata", - "elasticsearch-setup-passwords", - "elasticsearch-sql-cli", - "elasticsearch-syskeygen", - "elasticsearch-users", - "x-pack-env", - "x-pack-security-env", - "x-pack-watcher-env" - ).forEach(executable -> assertPermissionsAndOwnership(es.bin(executable), p755)); + Stream + .of( + "elasticsearch-certgen", + "elasticsearch-certutil", + "elasticsearch-croneval", + "elasticsearch-saml-metadata", + "elasticsearch-setup-passwords", + "elasticsearch-sql-cli", + "elasticsearch-syskeygen", + "elasticsearch-users", + "x-pack-env", + "x-pack-security-env", + "x-pack-watcher-env" + ) + .forEach(executable -> assertPermissionsAndOwnership(es.bin(executable), p755)); // at this time we only install the current version of archive distributions, but if that changes we'll need to pass // the version through here assertPermissionsAndOwnership(es.bin("elasticsearch-sql-cli-" + getCurrentVersion() + ".jar"), p755); - Stream.of( - "role_mapping.yml", - "roles.yml", - "users", - "users_roles" - ).forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p660)); + Stream + .of("role_mapping.yml", "roles.yml", "users", "users_roles") + .forEach(configFile -> assertPermissionsAndOwnership(es.config(configFile), p660)); + } + + public static void waitForElasticsearch(Installation installation) throws Exception { + withLogging(() -> ServerUtils.waitForElasticsearch(installation)); + } + + public static void waitForElasticsearch(String status, String index, Installation installation, String username, String password) + throws Exception { + withLogging(() -> ServerUtils.waitForElasticsearch(status, index, installation, username, password)); + } + + private static void withLogging(ThrowingRunnable r) throws Exception { + try { + r.run(); + } catch (Exception e) { + final Shell.Result logs = sh.run("docker logs " + containerId); + logger.warn("Elasticsearch container failed to start.\n\nStdout:\n" + logs.stdout + "\n\nStderr:\n" + logs.stderr); + throw e; + } } - private enum Cleanup { - ALWAYS, - NEVER + private interface ThrowingRunnable { + void run() throws Exception; } } From 7f616bdb24f953ed6bd3f0ebbc96358de7f783e5 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Fri, 8 Nov 2019 16:06:06 +0000 Subject: [PATCH 11/16] Split env var file tests The test test80SetEnvironmentVariablesUsingFiles relied on x-pack security being available, which isn't the case in the OSS image. --- .../packaging/test/DockerTests.java | 75 ++++++++++++------- 1 file changed, 47 insertions(+), 28 deletions(-) diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 3867ad7a5bcbd..b8096021d915f 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -156,10 +156,7 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { // we have to disable Log4j from using JMX lest it will hit a security // manager exception before we have configured logging; this will fail // startup since we detect usages of logging before it is configured - final String jvmOptions = - "-Xms512m\n" + - "-Xmx512m\n" + - "-Dlog4j2.disable.jmx=true\n"; + final String jvmOptions = "-Xms512m\n-Xmx512m\n-Dlog4j2.disable.jmx=true\n"; append(tempConf.resolve("jvm.options"), jvmOptions); // Make the temp directory and contents accessible when bind-mounted @@ -167,9 +164,7 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { // Restart the container final Map volumes = Map.of(tempConf, Path.of("/usr/share/elasticsearch/config")); - runContainer(distribution(), volumes, Map.of( - "ES_JAVA_OPTS", "-XX:-UseCompressedOops" - )); + runContainer(distribution(), volumes, Map.of("ES_JAVA_OPTS", "-XX:-UseCompressedOops")); waitForElasticsearch(installation); @@ -186,10 +181,8 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { * which point to files that hold the required values. */ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { - final String xpackPassword = "hunter2"; final Path secretsDir = getTempDir().resolve("secrets"); final String optionsFilename = "esJavaOpts.txt"; - final String passwordFilename = "password.txt"; try { mkdir(secretsDir); @@ -197,19 +190,55 @@ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { // ES_JAVA_OPTS_FILE Files.writeString(secretsDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); + Map envVars = Map.of("ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename); + + // File permissions need to be secured in order for the ES wrapper to accept + // them for populating env var values + Files.setPosixFilePermissions(secretsDir.resolve(optionsFilename), p600); + + final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); + + // Restart the container + runContainer(distribution(), volumes, envVars); + + waitForElasticsearch(installation); + + final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_nodes")); + + assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); + } finally { + rm(secretsDir); + } + } + + /** + * Check that the elastic user's password can be configured via a file and the ELASTIC_PASSWORD_FILE environment variable. + */ + public void test81ConfigurePasswordThroughEnvironmentVariableFile() throws Exception { + // Test relies on configuring security + assumeTrue(distribution.isDefault()); + + final String xpackPassword = "hunter2"; + final Path secretsDir = getTempDir().resolve("secrets"); + final String passwordFilename = "password.txt"; + + try { + mkdir(secretsDir); + // ELASTIC_PASSWORD_FILE Files.writeString(secretsDir.resolve(passwordFilename), xpackPassword + "\n"); - Map envVars = Map.of( - "ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename, - "ELASTIC_PASSWORD_FILE", "/run/secrets/" + passwordFilename, - // Enable security so that we can test that the password has been used - "xpack.security.enabled", "true" - ); + Map envVars = Map + .of( + "ELASTIC_PASSWORD_FILE", + "/run/secrets/" + passwordFilename, + // Enable security so that we can test that the password has been used + "xpack.security.enabled", + "true" + ); // File permissions need to be secured in order for the ES wrapper to accept // them for populating env var values - Files.setPosixFilePermissions(secretsDir.resolve(optionsFilename), p600); Files.setPosixFilePermissions(secretsDir.resolve(passwordFilename), p600); final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); @@ -220,18 +249,9 @@ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { // If we configured security correctly, then this call will only work if we specify the correct credentials. waitForElasticsearch("green", null, installation, "elastic", "hunter2"); - // Try to call `/_nodes` first without authentication, and ensure it's rejected. - // We don't use the `makeRequest()` helper as it checks the status code. + // Also check that an unauthenticated call fails final int statusCode = Request.Get("http://localhost:9200/_nodes").execute().returnResponse().getStatusLine().getStatusCode(); assertThat("Expected server to require authentication", statusCode, equalTo(401)); - - // Now try again, but with credentials, to check that ES_JAVA_OPTS_FILE took effect - final String nodesResponse = makeRequest( - Request.Get("http://localhost:9200/_nodes"), - "elastic", - xpackPassword); - - assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); } finally { rm(secretsDir); } @@ -313,7 +333,6 @@ public void test92ElasticsearchNodeCliPackaging() { final Installation.Executables bin = installation.executables(); final Result result = sh.run(bin.elasticsearchNode + " -h"); - assertThat(result.stdout, - containsString("A CLI tool to do unsafe cluster and index manipulations on current node")); + assertThat(result.stdout, containsString("A CLI tool to do unsafe cluster and index manipulations on current node")); } } From ebce9ef5660a6360c2d885f005dd09e11a4e448a Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Mon, 11 Nov 2019 11:42:30 +0000 Subject: [PATCH 12/16] Address review feedback --- .../src/docker/bin/docker-entrypoint.sh | 7 +- .../packaging/test/DockerTests.java | 178 ++++++++---------- 2 files changed, 83 insertions(+), 102 deletions(-) diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index b4fad6c73d59e..778ced4649876 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -56,15 +56,10 @@ for VAR_NAME_FILE in $(env | cut -f1 -d= | grep '_FILE$'); do exit 1 fi - if [[ ! -r "${!VAR_NAME_FILE}" ]]; then - echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE is not readable" >&2 - exit 1 - fi - FILE_PERMS="$(stat -c '%a' ${!VAR_NAME_FILE})" if [[ "$FILE_PERMS" != "400" && "$FILE_PERMS" != 600 ]]; then - echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE must have file permissions 400 or 600" >&2 + echo "ERROR: File ${!VAR_NAME_FILE} from $VAR_NAME_FILE must have file permissions 400 or 600, but actually has: $FILE_PERMS" >&2 exit 1 fi diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index b8096021d915f..691be2ca9c111 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -25,10 +25,12 @@ import org.elasticsearch.packaging.util.Installation; import org.elasticsearch.packaging.util.ServerUtils; import org.elasticsearch.packaging.util.Shell.Result; +import org.junit.After; import org.junit.AfterClass; import org.junit.Before; import org.junit.BeforeClass; +import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; import java.util.Map; @@ -48,7 +50,6 @@ import static org.elasticsearch.packaging.util.FileMatcher.p660; import static org.elasticsearch.packaging.util.FileUtils.append; import static org.elasticsearch.packaging.util.FileUtils.getTempDir; -import static org.elasticsearch.packaging.util.FileUtils.mkdir; import static org.elasticsearch.packaging.util.FileUtils.rm; import static org.elasticsearch.packaging.util.ServerUtils.makeRequest; import static org.hamcrest.CoreMatchers.containsString; @@ -59,6 +60,7 @@ public class DockerTests extends PackagingTestCase { protected DockerShell sh; + private Path tempDir; @BeforeClass public static void filterDistros() { @@ -74,9 +76,15 @@ public static void cleanup() { } @Before - public void setupTest() { + public void setupTest() throws IOException { sh = new DockerShell(); installation = runContainer(distribution()); + tempDir = Files.createTempDirectory(getTempDir(), DockerTests.class.getSimpleName()); + } + + @After + public void teardownTest() { + rm(tempDir); } /** @@ -146,34 +154,27 @@ public void test60AutoCreateKeystore() throws Exception { * Check that the default config can be overridden using a bind mount, and that env vars are respected */ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { - final Path tempConf = getTempDir().resolve("esconf-alternate"); + copyFromContainer(installation.config("elasticsearch.yml"), tempDir.resolve("elasticsearch.yml")); + copyFromContainer(installation.config("log4j2.properties"), tempDir.resolve("log4j2.properties")); - try { - mkdir(tempConf); - copyFromContainer(installation.config("elasticsearch.yml"), tempConf.resolve("elasticsearch.yml")); - copyFromContainer(installation.config("log4j2.properties"), tempConf.resolve("log4j2.properties")); - - // we have to disable Log4j from using JMX lest it will hit a security - // manager exception before we have configured logging; this will fail - // startup since we detect usages of logging before it is configured - final String jvmOptions = "-Xms512m\n-Xmx512m\n-Dlog4j2.disable.jmx=true\n"; - append(tempConf.resolve("jvm.options"), jvmOptions); - - // Make the temp directory and contents accessible when bind-mounted - Files.setPosixFilePermissions(tempConf, fromString("rwxrwxrwx")); - - // Restart the container - final Map volumes = Map.of(tempConf, Path.of("/usr/share/elasticsearch/config")); - runContainer(distribution(), volumes, Map.of("ES_JAVA_OPTS", "-XX:-UseCompressedOops")); - - waitForElasticsearch(installation); - - final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_nodes")); - assertThat(nodesResponse, containsString("\"heap_init_in_bytes\":536870912")); - assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); - } finally { - rm(tempConf); - } + // we have to disable Log4j from using JMX lest it will hit a security + // manager exception before we have configured logging; this will fail + // startup since we detect usages of logging before it is configured + final String jvmOptions = "-Xms512m\n-Xmx512m\n-Dlog4j2.disable.jmx=true\n"; + append(tempDir.resolve("jvm.options"), jvmOptions); + + // Make the temp directory and contents accessible when bind-mounted + Files.setPosixFilePermissions(tempDir, fromString("rwxrwxrwx")); + + // Restart the container + final Map volumes = Map.of(tempDir, Path.of("/usr/share/elasticsearch/config")); + runContainer(distribution(), volumes, Map.of("ES_JAVA_OPTS", "-XX:-UseCompressedOops")); + + waitForElasticsearch(installation); + + final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_nodes")); + assertThat(nodesResponse, containsString("\"heap_init_in_bytes\":536870912")); + assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); } /** @@ -181,34 +182,27 @@ public void test70BindMountCustomPathConfAndJvmOptions() throws Exception { * which point to files that hold the required values. */ public void test80SetEnvironmentVariablesUsingFiles() throws Exception { - final Path secretsDir = getTempDir().resolve("secrets"); final String optionsFilename = "esJavaOpts.txt"; - try { - mkdir(secretsDir); - - // ES_JAVA_OPTS_FILE - Files.writeString(secretsDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); + // ES_JAVA_OPTS_FILE + Files.writeString(tempDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); - Map envVars = Map.of("ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename); + Map envVars = Map.of("ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename); - // File permissions need to be secured in order for the ES wrapper to accept - // them for populating env var values - Files.setPosixFilePermissions(secretsDir.resolve(optionsFilename), p600); + // File permissions need to be secured in order for the ES wrapper to accept + // them for populating env var values + Files.setPosixFilePermissions(tempDir.resolve(optionsFilename), p600); - final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); + final Map volumes = Map.of(tempDir, Path.of("/run/secrets")); - // Restart the container - runContainer(distribution(), volumes, envVars); + // Restart the container + runContainer(distribution(), volumes, envVars); - waitForElasticsearch(installation); + waitForElasticsearch(installation); - final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_nodes")); + final String nodesResponse = makeRequest(Request.Get("http://localhost:9200/_nodes")); - assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); - } finally { - rm(secretsDir); - } + assertThat(nodesResponse, containsString("\"using_compressed_ordinary_object_pointers\":\"false\"")); } /** @@ -219,42 +213,43 @@ public void test81ConfigurePasswordThroughEnvironmentVariableFile() throws Excep assumeTrue(distribution.isDefault()); final String xpackPassword = "hunter2"; - final Path secretsDir = getTempDir().resolve("secrets"); final String passwordFilename = "password.txt"; - try { - mkdir(secretsDir); - - // ELASTIC_PASSWORD_FILE - Files.writeString(secretsDir.resolve(passwordFilename), xpackPassword + "\n"); + // ELASTIC_PASSWORD_FILE + Files.writeString(tempDir.resolve(passwordFilename), xpackPassword + "\n"); - Map envVars = Map - .of( - "ELASTIC_PASSWORD_FILE", - "/run/secrets/" + passwordFilename, - // Enable security so that we can test that the password has been used - "xpack.security.enabled", - "true" - ); + Map envVars = Map + .of( + "ELASTIC_PASSWORD_FILE", + "/run/secrets/" + passwordFilename, + // Enable security so that we can test that the password has been used + "xpack.security.enabled", + "true" + ); - // File permissions need to be secured in order for the ES wrapper to accept - // them for populating env var values - Files.setPosixFilePermissions(secretsDir.resolve(passwordFilename), p600); + // File permissions need to be secured in order for the ES wrapper to accept + // them for populating env var values + Files.setPosixFilePermissions(tempDir.resolve(passwordFilename), p600); - final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); + final Map volumes = Map.of(tempDir, Path.of("/run/secrets")); - // Restart the container - runContainer(distribution(), volumes, envVars); + // Restart the container + runContainer(distribution(), volumes, envVars); - // If we configured security correctly, then this call will only work if we specify the correct credentials. + // If we configured security correctly, then this call will only work if we specify the correct credentials. + try { waitForElasticsearch("green", null, installation, "elastic", "hunter2"); - - // Also check that an unauthenticated call fails - final int statusCode = Request.Get("http://localhost:9200/_nodes").execute().returnResponse().getStatusLine().getStatusCode(); - assertThat("Expected server to require authentication", statusCode, equalTo(401)); - } finally { - rm(secretsDir); + } catch (Exception e) { + throw new AssertionError( + "Failed to check whether Elasticsearch had started. This could be because " + + "authentication isn't working properly. Check the container logs", + e + ); } + + // Also check that an unauthenticated call fails + final int statusCode = Request.Get("http://localhost:9200/_nodes").execute().returnResponse().getStatusLine().getStatusCode(); + assertThat("Expected server to require authentication", statusCode, equalTo(401)); } /** @@ -262,34 +257,25 @@ public void test81ConfigurePasswordThroughEnvironmentVariableFile() throws Excep * the files' permissions are checked. */ public void test81EnvironmentVariablesUsingFilesHaveCorrectPermissions() throws Exception { - final Path secretsDir = getTempDir().resolve("secrets"); final String optionsFilename = "esJavaOpts.txt"; - try { - mkdir(secretsDir); - - // ES_JAVA_OPTS_FILE - Files.writeString(secretsDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); + // ES_JAVA_OPTS_FILE + Files.writeString(tempDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); - Map envVars = Map.of("ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename); + Map envVars = Map.of("ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename); - // Set invalid file permissions - Files.setPosixFilePermissions(secretsDir.resolve(optionsFilename), p660); + // Set invalid file permissions + Files.setPosixFilePermissions(tempDir.resolve(optionsFilename), p660); - final Map volumes = Map.of(secretsDir, Path.of("/run/secrets")); + final Map volumes = Map.of(tempDir, Path.of("/run/secrets")); - // Restart the container - final Result dockerLogs = runContainerExpectingFailure(distribution(), volumes, envVars); + // Restart the container + final Result dockerLogs = runContainerExpectingFailure(distribution(), volumes, envVars); - assertThat( - dockerLogs.stderr, - containsString( - "ERROR: File /run/secrets/" + optionsFilename + " from ES_JAVA_OPTS_FILE must have file permissions 400 or 600" - ) - ); - } finally { - rm(secretsDir); - } + assertThat( + dockerLogs.stderr, + containsString("ERROR: File /run/secrets/" + optionsFilename + " from ES_JAVA_OPTS_FILE must have file permissions 400 or 600") + ); } /** From 4054f2d240c5dff36742903ba045d292faa4a445 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Mon, 11 Nov 2019 14:46:56 +0000 Subject: [PATCH 13/16] Tighter checks for _FILE vs regular vars Ensure that regular environment variables are not even set at the same time as their _FILE equivalents. Previously they could be set so long as they were empty. --- .../src/docker/bin/docker-entrypoint.sh | 2 +- .../packaging/test/DockerTests.java | 30 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/distribution/docker/src/docker/bin/docker-entrypoint.sh b/distribution/docker/src/docker/bin/docker-entrypoint.sh index 778ced4649876..d95c451dead9d 100644 --- a/distribution/docker/src/docker/bin/docker-entrypoint.sh +++ b/distribution/docker/src/docker/bin/docker-entrypoint.sh @@ -46,7 +46,7 @@ for VAR_NAME_FILE in $(env | cut -f1 -d= | grep '_FILE$'); do if [[ -n "$VAR_NAME_FILE" ]]; then VAR_NAME="${VAR_NAME_FILE%_FILE}" - if [[ -n "${!VAR_NAME}" ]]; then + if env | grep "^${VAR_NAME}="; then echo "ERROR: Both $VAR_NAME_FILE and $VAR_NAME are set. These are mutually exclusive." >&2 exit 1 fi diff --git a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java index 691be2ca9c111..997d4b9758684 100644 --- a/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java +++ b/qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java @@ -252,11 +252,39 @@ public void test81ConfigurePasswordThroughEnvironmentVariableFile() throws Excep assertThat("Expected server to require authentication", statusCode, equalTo(401)); } + /** + * Check that environment variables cannot be used with _FILE environment variables. + */ + public void test81CannotUseEnvVarsAndFiles() throws Exception { + final String optionsFilename = "esJavaOpts.txt"; + + // ES_JAVA_OPTS_FILE + Files.writeString(tempDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n"); + + Map envVars = Map.of( + "ES_JAVA_OPTS", "-XX:+UseCompressedOops", + "ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename + ); + + // File permissions need to be secured in order for the ES wrapper to accept + // them for populating env var values + Files.setPosixFilePermissions(tempDir.resolve(optionsFilename), p600); + + final Map volumes = Map.of(tempDir, Path.of("/run/secrets")); + + final Result dockerLogs = runContainerExpectingFailure(distribution, volumes, envVars); + + assertThat( + dockerLogs.stderr, + containsString("ERROR: Both ES_JAVA_OPTS_FILE and ES_JAVA_OPTS are set. These are mutually exclusive.") + ); + } + /** * Check that when populating environment variables by setting variables with the suffix "_FILE", * the files' permissions are checked. */ - public void test81EnvironmentVariablesUsingFilesHaveCorrectPermissions() throws Exception { + public void test82EnvironmentVariablesUsingFilesHaveCorrectPermissions() throws Exception { final String optionsFilename = "esJavaOpts.txt"; // ES_JAVA_OPTS_FILE From 0a07f70223ecd4c8e8061638277f2abdb52c75c0 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Mon, 11 Nov 2019 17:01:51 +0000 Subject: [PATCH 14/16] Update docs for _FILE variables --- docs/reference/setup/install/docker.asciidoc | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/docs/reference/setup/install/docker.asciidoc b/docs/reference/setup/install/docker.asciidoc index fb28fbb988f75..4392a1c6623a2 100644 --- a/docs/reference/setup/install/docker.asciidoc +++ b/docs/reference/setup/install/docker.asciidoc @@ -290,7 +290,7 @@ https://docs.docker.com/engine/extend/plugins/#volume-plugins[Docker volume plug ===== Avoid using `loop-lvm` mode -If you are using the devicemapper storage driver, do not use the default `loop-lvm` mode. +If you are using the devicemapper storage driver, do not use the default `loop-lvm` mode. Configure docker-engine to use https://docs.docker.com/engine/userguide/storagedriver/device-mapper-driver/#configure-docker-with-devicemapper[direct-lvm]. @@ -312,7 +312,16 @@ over the configuration files in the image. You can set individual {es} configuration parameters using Docker environment variables. The <> and the -<> use this method. +<> use this method. + +If you are providing secrets e.g. passwords to {es} and you do not want to pass them +directly via environment variables, you can instead supply environment variables suffixed with +"_FILE". The variable value specifies a file, whose contents are used for the secret. + +For example, if you specified the environment variable "ELASTIC_PASSWORD_FILE" with value +"/run/secrets/password.txt", and bind-mounted a file at this location in the container with +the contents "PleaseChangeMe", then this would be equivalent to specifying the environment +variable "ELASTIC_PASSWORD" with the value "PleaseChangeMe". You can also override the default command for the image to pass {es} configuration parameters as command line options. For example: From 2a4808749f711f5c968cacf1b10930587ad72971 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Mon, 11 Nov 2019 17:24:05 +0000 Subject: [PATCH 15/16] Docs tweaks --- docs/reference/setup/install/docker.asciidoc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/reference/setup/install/docker.asciidoc b/docs/reference/setup/install/docker.asciidoc index 4392a1c6623a2..33e376849ebff 100644 --- a/docs/reference/setup/install/docker.asciidoc +++ b/docs/reference/setup/install/docker.asciidoc @@ -312,15 +312,15 @@ over the configuration files in the image. You can set individual {es} configuration parameters using Docker environment variables. The <> and the -<> use this method. +<> use this method. -If you are providing secrets e.g. passwords to {es} and you do not want to pass them -directly via environment variables, you can instead supply environment variables suffixed with -"_FILE". The variable value specifies a file, whose contents are used for the secret. +If you are providing secrets, such as passwords, to {es} and do not want to pass them +directly via environment variables, you can supply environment variables suffixed with +"_FILE". The variable value specifies a file whose contents are used for the secret. For example, if you specified the environment variable "ELASTIC_PASSWORD_FILE" with value -"/run/secrets/password.txt", and bind-mounted a file at this location in the container with -the contents "PleaseChangeMe", then this would be equivalent to specifying the environment +"/run/secrets/password.txt" and bind-mounted a file at this location in the container with +the contents "PleaseChangeMe", this would be equivalent to specifying the environment variable "ELASTIC_PASSWORD" with the value "PleaseChangeMe". You can also override the default command for the image to pass {es} configuration From 3a778a7a16f78c62292de623554ee2b6f351caa0 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Tue, 12 Nov 2019 10:02:53 +0000 Subject: [PATCH 16/16] Address further docs review feedback --- docs/reference/setup/install/docker.asciidoc | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/docs/reference/setup/install/docker.asciidoc b/docs/reference/setup/install/docker.asciidoc index 33e376849ebff..c527c5f21c3fe 100644 --- a/docs/reference/setup/install/docker.asciidoc +++ b/docs/reference/setup/install/docker.asciidoc @@ -314,14 +314,18 @@ You can set individual {es} configuration parameters using Docker environment va The <> and the <> use this method. -If you are providing secrets, such as passwords, to {es} and do not want to pass them -directly via environment variables, you can supply environment variables suffixed with -"_FILE". The variable value specifies a file whose contents are used for the secret. - -For example, if you specified the environment variable "ELASTIC_PASSWORD_FILE" with value -"/run/secrets/password.txt" and bind-mounted a file at this location in the container with -the contents "PleaseChangeMe", this would be equivalent to specifying the environment -variable "ELASTIC_PASSWORD" with the value "PleaseChangeMe". +To use the contents of a file to set an environment variable, suffix the environment +variable name with `_FILE`. This is useful for passing secrets such as passwords to {es} +without specifying them directly. + +For example, to set the {es} bootstrap password from a file, you can bind mount the +file and set the `ELASTIC_PASSWORD_FILE` environment variable to the mount location. +If you mount the password file to `/run/secrets/password.txt`, specify: + +[source,sh] +-------------------------------------------- +-e ELASTIC_PASSWORD_FILE=/run/secrets/bootstrapPassword.txt +-------------------------------------------- You can also override the default command for the image to pass {es} configuration parameters as command line options. For example: