Skip to content
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

Support _FILE suffixed env vars in Docker entrypoint #47573

Merged

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Oct 4, 2019

Closes #43603. Allow environment variables to be passed to ES in a Docker container via a file, by setting an environment variable with the _FILE suffix that points to the file with the intended value of the env var.

For example, the following command runs an ES container and sets the bootstrap password via the /run/secrets/bootstrapPassword.txt file, by bind-mounting a directory as /run/secrets/.

docker run \
  -it \
  --rm \
  -p 9200:9200 \
  -p 9300:9300 \
  -e ELASTIC_PASSWORD_FILE=/run/secrets/bootstrapPassword.txt \
  -v $PWD/secrets:/run/secrets \
  -e "discovery.type=single-node" \
  -e xpack.security.enabled=true \
  elasticsearch:test

Closes elastic#43603. Allow a password to be specifed in an ES Docker container
by specifying a file path in the ELASTIC_PASSWORD_FILE environment
variable.
Expand support for populating environment variables from file via vars
with a _FILE suffix.
@pugnascotia pugnascotia added :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts WIP labels Oct 4, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Packaging)

Copy link
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

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

We should add some tests to verify this behavior.

fi

if [[ ! -e "${!VAR_NAME_FILE}" ]]; then
echo "ERROR: File ${!VAR_NAME_FILE} does not exist" >&2
Copy link
Contributor

Choose a reason for hiding this comment

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

May be worth extending the message to also include the variable name pointing to that file

fi

echo "Setting $VAR_NAME from ${!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.

In the Docker entrypoint, when using _FILE-suffixed env vars to populate
environment variables, include the _FILE var name for context.
@pugnascotia
Copy link
Contributor Author

@atorok thanks for the review, I've addressed your points. Of course, the new test doesn't run yet in CI due to #47639, so I've been running it locally.

Copy link
Contributor

@alpar-t alpar-t left a comment

Choose a reason for hiding this comment

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

Thanks this LGTM now.

I'm wondering if we should enforce safe permissions on these files.
If i'm reading this right, the env vars and thus the contents of the file,
could be used to perform command injection in the container.
Files make this worse, because all it takes is for the file to be writable by someone else
and it could trick the administrator into running those commands, granted in the container but that might still be bad enough as one could access the data directory.

@pugnascotia pugnascotia changed the title [WIP] Support _FILE suffixed env vars in Docker entrypoint Support _FILE suffixed env vars in Docker entrypoint Oct 10, 2019
@pugnascotia pugnascotia removed the WIP label Oct 10, 2019
@pugnascotia
Copy link
Contributor Author

@atorok

If i'm reading this right, the env vars and thus the contents of the file,
could be used to perform command injection in the container.

Where do you see the potential command injection?

@alpar-t
Copy link
Contributor

alpar-t commented Oct 10, 2019

Suppose a file value.txt exists and we have some FILE_SETTING variable configured to point to it.
If value.txt is word writable, one could edit it to something like

some value" && rm -Rf /path/to/data/dir # 

So this will eventually run something like :

run_as_other_user_if_needed /usr/share/elasticsearch/bin/elasticsearch "-Esetting=some value" && rm -Rf /path/to/data/dir # any other arguments here don't matter  

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging

@pugnascotia
Copy link
Contributor Author

I'm waiting to merge #47640 before finishing this off.

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.
@pugnascotia
Copy link
Contributor Author

GitHub seems to have forgotten that this PR needs CI. I'll try closing and reopening.

@pugnascotia pugnascotia closed this Nov 5, 2019
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

Very nicely done! I left a minor comment up for discussion.

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.
@@ -312,7 +312,16 @@ over the configuration files in the image.

You can set individual {es} configuration parameters using Docker environment variables.
The <<docker-compose-file, sample compose file>> and the
<<docker-cli-run-dev-mode, single-node example>> use this method.
<<docker-cli-run-dev-mode, single-node example>> use this method.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<<docker-cli-run-dev-mode, single-node example>> use this method.
<<docker-cli-run-dev-mode, single-node example>> use this method.

<<docker-cli-run-dev-mode, single-node example>> use this method.
<<docker-cli-run-dev-mode, single-node example>> use this method.

If you are providing secrets e.g. passwords to {es} and you do not want to pass them
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you are providing secrets e.g. passwords to {es} and you do not want to pass them
If you are providing secrets, such as passwords, to {es} and do not want to pass them

<<docker-cli-run-dev-mode, single-node example>> 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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
directly via environment variables, you can instead supply environment variables suffixed with
directly via environment variables, you can supply environment variables suffixed with


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

Choose a reason for hiding this comment

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

Suggested change
"_FILE". The variable value specifies a file, whose contents are used for the secret.
"_FILE". The variable value specifies a file whose contents are used for the secret.

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

Choose a reason for hiding this comment

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

Suggested change
"/run/secrets/password.txt", and bind-mounted a file at this location in the container with
"/run/secrets/password.txt" and bind-mounted a file at this location in the container with


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

Choose a reason for hiding this comment

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

Suggested change
the contents "PleaseChangeMe", then this would be equivalent to specifying the environment
the contents "PleaseChangeMe", this would be equivalent to specifying the environment

Copy link
Contributor

@debadair debadair left a comment

Choose a reason for hiding this comment

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

A couple editorial comments, but LGTM.


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

Choose a reason for hiding this comment

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

I'd flip this around and lead with the fact that you can set env vars with a file. Something like:

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, 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".
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I'd recommend sticking to present tense for examples. In this case, I'd go with something like:

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:

-e ELASTIC_PASSWORD_FILE=/run/secrets/bootstrapPassword.txt \

Also note that the quotes should be backticks to render the env var and path in code font.

@pugnascotia
Copy link
Contributor Author

Thanks @debadair, I've incorporated your feedback.

@dliappis dliappis self-requested a review November 12, 2019 10:11
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@pugnascotia
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-matrix

@pugnascotia pugnascotia merged commit 2a4e101 into elastic:master Nov 12, 2019
@pugnascotia pugnascotia deleted the 43603-docker-entrypoint-file-suffix branch November 12, 2019 14:20
pugnascotia added a commit to pugnascotia/elasticsearch that referenced this pull request Nov 15, 2019
Backport of elastic#47573.

Closes elastic#43603. Allow environment variables to be passed to ES in a Docker
container via a file, by setting an environment variable with the `_FILE`
suffix that points to the file with the intended value of the env var.
pugnascotia added a commit that referenced this pull request Nov 18, 2019
Backport of #47573.

Closes #43603. Allow environment variables to be passed to ES in a Docker
container via a file, by setting an environment variable with the `_FILE`
suffix that points to the file with the intended value of the env var.
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team v7.6.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Support _FILE suffix in docker images
8 participants