From 148db6f6c6e1d3980f1860bd280e85cbef5df8de Mon Sep 17 00:00:00 2001 From: Lucas Parry Date: Mon, 26 May 2025 10:45:32 +1000 Subject: [PATCH 1/3] Safer env-var expansion where available --- README.md | 4 ++ commands/push.sh | 9 ++++- lib/shared.bash | 31 +++++++++++++++ plugin.yml | 3 ++ tests/shared_lib.bats | 87 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 tests/shared_lib.bats diff --git a/README.md b/README.md index a4ffaa16..0a9f9e84 100644 --- a/README.md +++ b/README.md @@ -215,6 +215,10 @@ When set to true, it will activate interpolation of variables in the elements wi Note that rules regarding [environment variable interpolation](https://buildkite.com/docs/pipelines/environment-variables#runtime-variable-interpolation) apply here. That means that `$VARIABLE_NAME` is resolved at pipeline upload time, whereas `$$VARIABLE_NAME` will be at run time. All things being equal, you likely want to use `$$VARIABLE_NAME` on the variables mentioned in this option. +#### `expand-push-vars-allowlist` (push only, string, when using expand-push-vars on a system with envsubst) + +On a system with `envsubst`, that tool will be used instead of `eval`, and this variable can be used to define the commands `SHELL-FORMAT` argument, which defines the environment variable allowlist so that unexpected interpolations are not permitted. + #### `expand-volume-vars` (run only, boolean, unsafe) When set to true, it will activate interpolation of variables in the elements of the `volumes` configuration array. When turned off (the default), attempting to use variables will fail as the literal `$VARIABLE_NAME` string will be passed to the `-v` option. diff --git a/commands/push.sh b/commands/push.sh index 71558775..bec7988a 100755 --- a/commands/push.sh +++ b/commands/push.sh @@ -41,10 +41,17 @@ prebuilt_image_namespace="$(plugin_read_config PREBUILT_IMAGE_NAMESPACE 'docker- # Then we figure out what to push, and where for line in $(plugin_read_list PUSH) ; do if [[ "$(plugin_read_config EXPAND_PUSH_VARS 'false')" == "true" ]]; then - push_target=$(eval echo "$line") + allowlist="$(plugin_read_config EXPAND_PUSH_VARS_ALLOWLIST '__UNSET__')" + echo "allowlist=${allowlist}" + if [[ "$allowlist" == "__UNSET__" ]]; then + push_target="$(expand_var "$line")" + else + push_target="$(expand_var "$line" "$allowlist")" + fi else push_target="$line" fi + echo "pushtarget=${push_target}" IFS=':' read -r -a tokens <<< "$push_target" service_name=${tokens[0]} service_image="$(compose_image_for_service "$service_name")" diff --git a/lib/shared.bash b/lib/shared.bash index 303be82e..0d534c18 100644 --- a/lib/shared.bash +++ b/lib/shared.bash @@ -322,6 +322,37 @@ function retry { done } +# Expands the env vars in a string, using envsubst if present, falling back to +# eval when it's missing. +function expand_var() { + # Try to use the safest approach possible + if command -v envsubst > /dev/null; then + if [[ $# -eq 1 ]]; then + # safer + expand_var_with_envsubst "$1" + else + # safest + expand_var_with_envsubst "$1" "$2" + fi + else + # unsafe + expand_var_with_eval "$1" + fi +} + +function expand_var_with_envsubst() { + if [[ $# -eq 1 ]]; then + echo "$1" | envsubst + else + echo "$1" | envsubst "$2" + fi +} + +function expand_var_with_eval() { + echo "$(eval echo "$1")" +} + + function is_windows() { [[ "$OSTYPE" =~ ^(win|msys|cygwin) ]] } diff --git a/plugin.yml b/plugin.yml index 51251335..3756a108 100755 --- a/plugin.yml +++ b/plugin.yml @@ -96,6 +96,8 @@ configuration: type: string expand-push-vars: type: boolean + expand-push-vars-allowlist: + type: string expand-volume-vars: type: boolean graceful-shutdown: @@ -203,6 +205,7 @@ configuration: env-propagation-list: [ run ] environment: [ run ] expand-push-vars: [ push ] + expand-push-vars-allowlist: [ push ] expand-volume-vars: [ volumes ] graceful-shutdown: [ run ] leave-volumes: [ run ] diff --git a/tests/shared_lib.bats b/tests/shared_lib.bats new file mode 100644 index 00000000..62c589f3 --- /dev/null +++ b/tests/shared_lib.bats @@ -0,0 +1,87 @@ +#!/usr/bin/env bats + +load "${BATS_PLUGIN_PATH}/load.bash" +load '../lib/shared.bash' + + +@test "expand_var works" { + export MY_VAR="llamas" + export MY_STRING="foo:bar:\$MY_VAR" + + run expand_var "$MY_STRING" + + assert_success + assert_output "foo:bar:llamas" +} + +@test "expand_var works via envsubst" { + export MY_VAR="llamas" + export MY_STRING="foo:bar:\$MY_VAR" + + ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input" + stub envsubst "cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:llamas'" + + run expand_var_with_envsubst "$MY_STRING" + + assert_success + assert_output "foo:bar:llamas" + + run cat "$ENVSUB_STUB_STDIN" + + assert_success + assert_output "$MY_STRING" + + unstub envsubst +} + +@test "expand_var works via envsubst with an allowlist" { + export MY_VAR="llamas" + export MY_STRING="foo:bar:\$MY_VAR" + export ALLOWLIST="\$MY_VAR" + + ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input" + stub envsubst "'$ALLOWLIST' : cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:llamas'" + + run expand_var_with_envsubst "$MY_STRING" "$ALLOWLIST" + + assert_success + assert_output "foo:bar:llamas" + + run cat "$ENVSUB_STUB_STDIN" + + assert_success + assert_output "$MY_STRING" + + unstub envsubst +} + +@test "expand_var works via envsubst with an allowlist not including the var" { + export MY_VAR="llamas" + export MY_STRING="foo:bar:\$MY_VAR:\$MY_OTHER_VAR" + export ALLOWLIST="\$MY_OTHER_VAR" + + ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input" + stub envsubst "'$ALLOWLIST' : cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:\$MY_VAR:llamas'" + + run expand_var_with_envsubst "$MY_STRING" "$ALLOWLIST" + + assert_success + assert_output "foo:bar:\$MY_VAR:llamas" + + run cat "$ENVSUB_STUB_STDIN" + + assert_success + assert_output "$MY_STRING" + + unstub envsubst +} + +@test "expand_var works via eval" { + export MY_VAR="llamas" + export MY_STRING="foo:bar:\$MY_VAR" + + run expand_var_with_eval "$MY_STRING" + + assert_success + assert_output "foo:bar:llamas" +} From c9b9dd02886d8efa2bd715eb3107e4e81fddad56 Mon Sep 17 00:00:00 2001 From: Lucas Parry Date: Thu, 29 May 2025 13:50:05 +1000 Subject: [PATCH 2/3] Tidy up some mess --- commands/push.sh | 6 ++---- lib/shared.bash | 17 ++++------------- tests/shared_lib.bats | 2 ++ 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/commands/push.sh b/commands/push.sh index bec7988a..d2fcf701 100755 --- a/commands/push.sh +++ b/commands/push.sh @@ -41,9 +41,8 @@ prebuilt_image_namespace="$(plugin_read_config PREBUILT_IMAGE_NAMESPACE 'docker- # Then we figure out what to push, and where for line in $(plugin_read_list PUSH) ; do if [[ "$(plugin_read_config EXPAND_PUSH_VARS 'false')" == "true" ]]; then - allowlist="$(plugin_read_config EXPAND_PUSH_VARS_ALLOWLIST '__UNSET__')" - echo "allowlist=${allowlist}" - if [[ "$allowlist" == "__UNSET__" ]]; then + allowlist="$(plugin_read_config EXPAND_PUSH_VARS_ALLOWLIST '')" + if [[ "$allowlist" == "" ]]; then push_target="$(expand_var "$line")" else push_target="$(expand_var "$line" "$allowlist")" @@ -51,7 +50,6 @@ for line in $(plugin_read_list PUSH) ; do else push_target="$line" fi - echo "pushtarget=${push_target}" IFS=':' read -r -a tokens <<< "$push_target" service_name=${tokens[0]} service_image="$(compose_image_for_service "$service_name")" diff --git a/lib/shared.bash b/lib/shared.bash index 0d534c18..528d6e7c 100644 --- a/lib/shared.bash +++ b/lib/shared.bash @@ -327,25 +327,16 @@ function retry { function expand_var() { # Try to use the safest approach possible if command -v envsubst > /dev/null; then - if [[ $# -eq 1 ]]; then - # safer - expand_var_with_envsubst "$1" - else - # safest - expand_var_with_envsubst "$1" "$2" - fi + expand_var_with_envsubst "$@" else - # unsafe expand_var_with_eval "$1" fi } function expand_var_with_envsubst() { - if [[ $# -eq 1 ]]; then - echo "$1" | envsubst - else - echo "$1" | envsubst "$2" - fi + str="$1" + shift + envsubst "$@" <<<"${str}" } function expand_var_with_eval() { diff --git a/tests/shared_lib.bats b/tests/shared_lib.bats index 62c589f3..e5c6d31f 100644 --- a/tests/shared_lib.bats +++ b/tests/shared_lib.bats @@ -3,6 +3,8 @@ load "${BATS_PLUGIN_PATH}/load.bash" load '../lib/shared.bash' +GIT_STUB_STDIN="$BATS_TEST_TMPDIR/git_stdin" + @test "expand_var works" { export MY_VAR="llamas" From b36c0e2dc2a5b13657f0d86a7379110123194c03 Mon Sep 17 00:00:00 2001 From: Lucas Parry Date: Thu, 29 May 2025 15:09:51 +1000 Subject: [PATCH 3/3] Offer the safer option everywhere --- README.md | 8 ++++---- commands/push.sh | 7 +------ lib/run.bash | 2 +- lib/shared.bash | 9 ++++++--- plugin.yml | 4 ++-- tests/shared_lib.bats | 28 ++++++++++++++-------------- 6 files changed, 28 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 0a9f9e84..1a206b1d 100644 --- a/README.md +++ b/README.md @@ -211,19 +211,19 @@ Additionally, volumes may be specified via the agent environment variable `BUILD When set to true, it will activate interpolation of variables in the elements within the `push` configuration array. When turned off (the default), attempting to use variables will fail as the literal `$VARIABLE_NAME` string will be used as the target.. -:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline +:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline. On a system with `envsubst` available, that tool will be used in preference to `eval`. See `expand-vars-allowlist` to restrict the variables allowed to be substituted when using `envsubst`. Note that rules regarding [environment variable interpolation](https://buildkite.com/docs/pipelines/environment-variables#runtime-variable-interpolation) apply here. That means that `$VARIABLE_NAME` is resolved at pipeline upload time, whereas `$$VARIABLE_NAME` will be at run time. All things being equal, you likely want to use `$$VARIABLE_NAME` on the variables mentioned in this option. -#### `expand-push-vars-allowlist` (push only, string, when using expand-push-vars on a system with envsubst) +#### `expand-vars-allowlist` (push or run, string, when using one of the `expand-*-vars` options, and on a system with `envsubst`) -On a system with `envsubst`, that tool will be used instead of `eval`, and this variable can be used to define the commands `SHELL-FORMAT` argument, which defines the environment variable allowlist so that unexpected interpolations are not permitted. +This variable is used to define the commands `SHELL-FORMAT` argument to `envsubst`, which defines the environment variable allowlist so that unexpected interpolations are not permitted. It is only relevant when using `expand-push-vars` or `expand-volume-vars` #### `expand-volume-vars` (run only, boolean, unsafe) When set to true, it will activate interpolation of variables in the elements of the `volumes` configuration array. When turned off (the default), attempting to use variables will fail as the literal `$VARIABLE_NAME` string will be passed to the `-v` option. -:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline +:warning: **Important:** this is considered an unsafe option as the most compatible way to achieve this is to run the strings through `eval` which could lead to arbitrary code execution or information leaking if you don't have complete control of the pipeline. On a system with `envsubst` available, that tool will be used in preference to `eval`. See `expand-vars-allowlist` to restrict the variables allowed to be substituted when using `envsubst`. Note that rules regarding [environment variable interpolation](https://buildkite.com/docs/pipelines/environment-variables#runtime-variable-interpolation) apply here. That means that `$VARIABLE_NAME` is resolved at pipeline upload time, whereas `$$VARIABLE_NAME` will be at run time. All things being equal, you likely want to use `$$VARIABLE_NAME` on the variables mentioned in this option. diff --git a/commands/push.sh b/commands/push.sh index d2fcf701..3cfef25f 100755 --- a/commands/push.sh +++ b/commands/push.sh @@ -41,12 +41,7 @@ prebuilt_image_namespace="$(plugin_read_config PREBUILT_IMAGE_NAMESPACE 'docker- # Then we figure out what to push, and where for line in $(plugin_read_list PUSH) ; do if [[ "$(plugin_read_config EXPAND_PUSH_VARS 'false')" == "true" ]]; then - allowlist="$(plugin_read_config EXPAND_PUSH_VARS_ALLOWLIST '')" - if [[ "$allowlist" == "" ]]; then - push_target="$(expand_var "$line")" - else - push_target="$(expand_var "$line" "$allowlist")" - fi + push_target="$(expand_var "$line")" else push_target="$line" fi diff --git a/lib/run.bash b/lib/run.bash index f9a263eb..ae8e9dc8 100644 --- a/lib/run.bash +++ b/lib/run.bash @@ -92,7 +92,7 @@ expand_relative_volume_path() { local path if [[ "$(plugin_read_config EXPAND_VOLUME_VARS 'false')" == "true" ]]; then - path=$(eval echo "$1") + path="$(expand_var "$1")" else path="$1" fi diff --git a/lib/shared.bash b/lib/shared.bash index 528d6e7c..503e5d70 100644 --- a/lib/shared.bash +++ b/lib/shared.bash @@ -334,9 +334,12 @@ function expand_var() { } function expand_var_with_envsubst() { - str="$1" - shift - envsubst "$@" <<<"${str}" + allowlist="$(plugin_read_config 'EXPAND_VARS_ALLOWLIST' '')" + if [[ "$allowlist" == "" ]]; then + envsubst <<<"$1" + else + envsubst "$allowlist" <<<"$1" + fi } function expand_var_with_eval() { diff --git a/plugin.yml b/plugin.yml index 3756a108..2936a711 100755 --- a/plugin.yml +++ b/plugin.yml @@ -96,7 +96,7 @@ configuration: type: string expand-push-vars: type: boolean - expand-push-vars-allowlist: + expand-vars-allowlist: type: string expand-volume-vars: type: boolean @@ -205,7 +205,7 @@ configuration: env-propagation-list: [ run ] environment: [ run ] expand-push-vars: [ push ] - expand-push-vars-allowlist: [ push ] + expand-vars-allowlist: [ push, run ] expand-volume-vars: [ volumes ] graceful-shutdown: [ run ] leave-volumes: [ run ] diff --git a/tests/shared_lib.bats b/tests/shared_lib.bats index e5c6d31f..7a93a882 100644 --- a/tests/shared_lib.bats +++ b/tests/shared_lib.bats @@ -3,12 +3,9 @@ load "${BATS_PLUGIN_PATH}/load.bash" load '../lib/shared.bash' -GIT_STUB_STDIN="$BATS_TEST_TMPDIR/git_stdin" - - @test "expand_var works" { export MY_VAR="llamas" - export MY_STRING="foo:bar:\$MY_VAR" + MY_STRING="foo:bar:\$MY_VAR" run expand_var "$MY_STRING" @@ -18,7 +15,7 @@ GIT_STUB_STDIN="$BATS_TEST_TMPDIR/git_stdin" @test "expand_var works via envsubst" { export MY_VAR="llamas" - export MY_STRING="foo:bar:\$MY_VAR" + MY_STRING="foo:bar:\$MY_VAR" ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input" stub envsubst "cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:llamas'" @@ -38,13 +35,14 @@ GIT_STUB_STDIN="$BATS_TEST_TMPDIR/git_stdin" @test "expand_var works via envsubst with an allowlist" { export MY_VAR="llamas" - export MY_STRING="foo:bar:\$MY_VAR" - export ALLOWLIST="\$MY_VAR" + MY_STRING="foo:bar:\$MY_VAR" + ALLOWLIST='$MY_VAR' + export BUILDKITE_PLUGIN_DOCKER_COMPOSE_EXPAND_VARS_ALLOWLIST="$ALLOWLIST" ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input" stub envsubst "'$ALLOWLIST' : cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:llamas'" - run expand_var_with_envsubst "$MY_STRING" "$ALLOWLIST" + run expand_var_with_envsubst "$MY_STRING" assert_success assert_output "foo:bar:llamas" @@ -59,16 +57,18 @@ GIT_STUB_STDIN="$BATS_TEST_TMPDIR/git_stdin" @test "expand_var works via envsubst with an allowlist not including the var" { export MY_VAR="llamas" - export MY_STRING="foo:bar:\$MY_VAR:\$MY_OTHER_VAR" - export ALLOWLIST="\$MY_OTHER_VAR" + export MY_OTHER_VAR="more_llamas" + MY_STRING="foo:bar:\$MY_VAR:\$MY_OTHER_VAR" + ALLOWLIST='$MY_OTHER_VAR' + export BUILDKITE_PLUGIN_DOCKER_COMPOSE_EXPAND_VARS_ALLOWLIST="$ALLOWLIST" ENVSUB_STUB_STDIN="$BATS_TEST_TMPDIR/envsubst_input" - stub envsubst "'$ALLOWLIST' : cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:\$MY_VAR:llamas'" + stub envsubst "'$ALLOWLIST' : cat > '$ENVSUB_STUB_STDIN'; echo 'foo:bar:\$MY_VAR:more_llamas'" - run expand_var_with_envsubst "$MY_STRING" "$ALLOWLIST" + run expand_var_with_envsubst "$MY_STRING" assert_success - assert_output "foo:bar:\$MY_VAR:llamas" + assert_output "foo:bar:\$MY_VAR:more_llamas" run cat "$ENVSUB_STUB_STDIN" @@ -80,7 +80,7 @@ GIT_STUB_STDIN="$BATS_TEST_TMPDIR/git_stdin" @test "expand_var works via eval" { export MY_VAR="llamas" - export MY_STRING="foo:bar:\$MY_VAR" + MY_STRING="foo:bar:\$MY_VAR" run expand_var_with_eval "$MY_STRING"