Skip to content

Support _FILE suffixed env vars in Docker entrypoint #47573

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
05a0133
Support ELASTIC_PASSWORD_FILE in Docker entrypoint
pugnascotia Oct 2, 2019
65974f9
Support all envs vars with a _FILE suffix
pugnascotia Oct 2, 2019
2b90d97
Include _FILE names when checking files in Docker
pugnascotia Oct 8, 2019
1f03dad
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Oct 8, 2019
7e361ff
Trying to write tests
pugnascotia Oct 9, 2019
64175d2
Fix up new Docker test
pugnascotia Oct 9, 2019
9b940cf
Revert accidental changes
pugnascotia Oct 9, 2019
7ca020c
Tweaks
pugnascotia Oct 9, 2019
21c1104
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 5, 2019
e564576
Post-merge fixes
pugnascotia Nov 5, 2019
6f1cf82
Check env var file permissions
pugnascotia Nov 5, 2019
8562703
Capture more logging on failure
pugnascotia Nov 6, 2019
7f616bd
Split env var file tests
pugnascotia Nov 8, 2019
0162ef2
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 10, 2019
052d46f
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 11, 2019
ebce9ef
Address review feedback
pugnascotia Nov 11, 2019
4054f2d
Tighter checks for _FILE vs regular vars
pugnascotia Nov 11, 2019
0a07f70
Update docs for _FILE variables
pugnascotia Nov 11, 2019
2a48087
Docs tweaks
pugnascotia Nov 11, 2019
3a778a7
Address further docs review feedback
pugnascotia Nov 12, 2019
4ac0a9e
Merge remote-tracking branch 'upstream/master' into 43603-docker-entr…
pugnascotia Nov 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions distribution/docker/src/docker/bin/docker-entrypoint.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,45 @@ 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} from $VAR_NAME_FILE does not exist" >&2
exit 1
fi

if [[ ! -r "${!VAR_NAME_FILE}" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth combining the checks on permissions into a single one.
We could get into the situation when we tell the user it's not readable, then we tell him it's too broadly readable.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe including what permissions it actually has might be useful to the user sometimes ?

The permissions are a bit on the strict size. We could technically be asserting that no write permissions are available to anyone besides the user, but since it's expected that these files will mostly be used for secrets I guess this is fine.

exit 1
fi

echo "Setting $VAR_NAME from $VAR_NAME_FILE at ${!VAR_NAME_FILE}" >&2
export "$VAR_NAME"="$(cat ${!VAR_NAME_FILE})"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be stripping a possible newline at the end of the file here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The newline is already stripped thanks to bash. An earlier implementation read the file without using cat, but didn't strip the newline.


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
Expand Down
137 changes: 125 additions & 12 deletions qa/os/src/test/java/org/elasticsearch/packaging/test/DockerTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,21 @@
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.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;
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.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;

public class DockerTests extends PackagingTestCase {
Expand All @@ -71,7 +74,7 @@ public static void cleanup() {
}

@Before
public void setupTest() throws Exception {
public void setupTest() {
sh = new DockerShell();
installation = runContainer(distribution());
}
Expand Down Expand Up @@ -153,20 +156,15 @@ 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
Files.setPosixFilePermissions(tempConf, fromString("rwxrwxrwx"));

// Restart the container
removeContainer();
runContainer(distribution(), tempConf, Map.of(
"ES_JAVA_OPTS", "-XX:-UseCompressedOops"
));
final Map<Path, Path> volumes = Map.of(tempConf, Path.of("/usr/share/elasticsearch/config"));
runContainer(distribution(), volumes, Map.of("ES_JAVA_OPTS", "-XX:-UseCompressedOops"));

waitForElasticsearch(installation);

Expand All @@ -178,6 +176,122 @@ 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 Path secretsDir = getTempDir().resolve("secrets");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would pull this into an instance variable and use @Before and @After to set it and clean up the dir.
It's more readable and less repetitive avoiding the try, finally is particularly beneficial imho.

final String optionsFilename = "esJavaOpts.txt";

try {
mkdir(secretsDir);

// ES_JAVA_OPTS_FILE
Files.writeString(secretsDir.resolve(optionsFilename), "-XX:-UseCompressedOops\n");

Map<String, String> 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<Path, Path> 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<String, String> 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);

final Map<Path, Path> volumes = Map.of(secretsDir, Path.of("/run/secrets"));

// Restart the container
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to wrap the exception here to add some additional context.
Imagine this fails 6 months from now, will it be obvious from the failure that the passwrod was not applied or will it look as if elasticsearch didn't start ? I'm not sure what the error looks like, guess it's better if it's an authentication failure, but the additional context certainly doesn't hurt.


// 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);
}
}

/**
* 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<String, String> envVars = Map.of("ES_JAVA_OPTS_FILE", "/run/secrets/" + optionsFilename);

// Set invalid file permissions
Files.setPosixFilePermissions(secretsDir.resolve(optionsFilename), p660);

final Map<Path, Path> 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.
Expand Down Expand Up @@ -219,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"));
}
}
Loading