Avoid using sudo in agent launchers when the current user is not a sudoer
#5835
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| #!/usr/bin/env bats | ||
| # Copyright (c) 2012-2017 Red Hat, Inc | ||
| # All rights reserved. This program and the accompanying materials | ||
| # are made available under the terms of the Eclipse Public License v1.0 | ||
| # which accompanies this distribution, and is available at | ||
| # http://www.eclipse.org/legal/epl-v10.html | ||
| # | ||
| # Contributors: | ||
| # Mario | ||
| # | ||
| # How to run this script: | ||
| # cd <root of che local git repository> | ||
| # export CHE_BASE_DIR=$(pwd) | ||
| # export LAUNCHER_SCRIPT_TO_TEST=wsagent/agent/src/main/resources/org.eclipse.che.ws-agent.script.sh | ||
| # export BATS_TEST_SCRIPT=agents/che-core-api-agent/src/test/resources/agents-launchers-tests-arbitraryuser.bats | ||
| # export DOCKER_IMAGE=rhche/centos_jdk8 | ||
| # docker run -ti --rm -e CHE_BASE_DIR -e LAUNCHER_SCRIPT_TO_TEST -e DOCKER_IMAGE \ | ||
| # -v ${CHE_BASE_DIR}/${BATS_TEST_SCRIPT}:/scripts/launcher_tests.bats \ | ||
| # -v ${CHE_BASE_DIR}/dockerfiles:/dockerfiles \ | ||
| # -v /var/run/docker.sock:/var/run/docker.sock \ | ||
| # eclipse/che-bats bats /scripts/launcher_tests.bats | ||
| # | ||
|
|
||
| load '/bats-support/load.bash' | ||
| load '/bats-assert/load.bash' | ||
| . /dockerfiles/cli/tests/test_base.sh | ||
|
|
||
| CONTAINER_NAME="test" | ||
|
|
||
| script_host_path=${CHE_BASE_DIR}/${LAUNCHER_SCRIPT_TO_TEST} | ||
|
|
||
| root_msg="I am root" | ||
| not_root_msg="I am a not root" | ||
| sudoer_msg="I am a sudoer" | ||
| not_sudoer_msg="I am a not a sudoer" | ||
| test_snippet="source <(grep -iE -A3 'is_current_user_root\(\)|is_current_user_sudoer\(\)' /launch.sh | grep -v -- "^--$"); is_current_user_root && echo -n '${root_msg} ' || echo -n '${not_root_msg} '; is_current_user_sudoer && echo '${sudoer_msg}' || echo '${not_sudoer_msg}'" | ||
| user="100000" | ||
|
|
||
| # Kill running che server instance if there is any to be able to run tests | ||
| setup() { | ||
| kill_running_named_container ${CONTAINER_NAME} | ||
| remove_named_container ${CONTAINER_NAME} | ||
| docker run --security-opt no-new-privileges --user=${user} --name="${CONTAINER_NAME}" -d -v ${script_host_path}:/launch.sh "${DOCKER_IMAGE}" | ||
| } | ||
|
|
||
| teardown() { | ||
| kill_running_named_container "${CONTAINER_NAME}" | ||
| remove_named_container ${CONTAINER_NAME} | ||
| } | ||
|
|
||
| @test "should deduce that's not a sudoer nor root when ${LAUNCHER_SCRIPT_TO_TEST} is run as an arbitrary user" { | ||
| #GIVEN | ||
| expected_msg="${not_root_msg} ${not_sudoer_msg}" | ||
|
|
||
| #WHEN | ||
| run docker exec --user=${user} "${CONTAINER_NAME}" bash -c "${test_snippet}" | ||
|
|
||
| #THEN | ||
| assert_success | ||
| assert_output --partial ${expected_msg} | ||
| } | ||
|
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| #!/usr/bin/env bats | ||
| # Copyright (c) 2012-2017 Red Hat, Inc | ||
| # All rights reserved. This program and the accompanying materials | ||
| # are made available under the terms of the Eclipse Public License v1.0 | ||
| # which accompanies this distribution, and is available at | ||
| # http://www.eclipse.org/legal/epl-v10.html | ||
| # | ||
| # Contributors: | ||
| # Mario | ||
| # | ||
| # How to run this script: | ||
| # cd <root of che local git repository> | ||
| # export CHE_BASE_DIR=$(pwd) | ||
| # export LAUNCHER_SCRIPT_TO_TEST=wsagent/agent/src/main/resources/org.eclipse.che.ws-agent.script.sh | ||
| # export BATS_TEST_SCRIPT=agents/che-core-api-agent/src/test/resources/agents-launchers-tests.bats | ||
| # export DOCKER_IMAGE=eclipse/centos_jdk8 | ||
| # docker run -ti --rm -e CHE_BASE_DIR -e LAUNCHER_SCRIPT_TO_TEST -e DOCKER_IMAGE \ | ||
| # -v ${CHE_BASE_DIR}/${BATS_TEST_SCRIPT}:/scripts/launcher_tests.bats \ | ||
| # -v ${CHE_BASE_DIR}/dockerfiles:/dockerfiles \ | ||
| # -v /var/run/docker.sock:/var/run/docker.sock \ | ||
| # eclipse/che-bats bats /scripts/launcher_tests.bats | ||
| # | ||
|
|
||
| load '/bats-support/load.bash' | ||
| load '/bats-assert/load.bash' | ||
| . /dockerfiles/cli/tests/test_base.sh | ||
|
|
||
| CONTAINER_NAME="batssshscripttest" | ||
|
|
||
| script_host_path=${CHE_BASE_DIR}/${LAUNCHER_SCRIPT_TO_TEST} | ||
| root_msg="I am root" | ||
| not_root_msg="I am a not root" | ||
| sudoer_msg="I am a sudoer" | ||
| not_sudoer_msg="I am a not a sudoer" | ||
| test_snippet="source <(grep -iE -A3 'is_current_user_root\(\)|is_current_user_sudoer\(\)' /launch.sh | grep -v -- "^--$"); is_current_user_root && echo -n '${root_msg} ' || echo -n '${not_root_msg} '; is_current_user_sudoer && echo '${sudoer_msg}' || echo '${not_sudoer_msg}'" | ||
|
|
||
| # Kill running che server instance if there is any to be able to run tests | ||
| setup() { | ||
| kill_running_named_container ${CONTAINER_NAME} | ||
| remove_named_container ${CONTAINER_NAME} | ||
| docker run --name="${CONTAINER_NAME}" -d -v ${script_host_path}:/launch.sh "${DOCKER_IMAGE}" | ||
| } | ||
|
|
||
| teardown() { | ||
| kill_running_named_container "${CONTAINER_NAME}" | ||
| remove_named_container ${CONTAINER_NAME} | ||
| } | ||
|
|
||
| @test "should deduce that's root and sudoer when ${LAUNCHER_SCRIPT_TO_TEST} is run as root" { | ||
| #GIVEN | ||
| user="root" | ||
| expected_msg="${root_msg} ${sudoer_msg}" | ||
|
|
||
| #WHEN | ||
| run docker exec --user=${user} "${CONTAINER_NAME}" bash -c "${test_snippet}" | ||
|
|
||
| #THEN | ||
| assert_success | ||
| assert_output --partial ${expected_msg} | ||
| } | ||
|
|
||
| @test "should deduce that's not root but sudoer when ${LAUNCHER_SCRIPT_TO_TEST} is run as user with UID 1000" { | ||
| #GIVEN | ||
| user="1000" | ||
| expected_msg="${not_root_msg} ${sudoer_msg}" | ||
|
|
||
| #WHEN | ||
| run docker exec --user=${user} "${CONTAINER_NAME}" bash -c "${test_snippet}" | ||
|
|
||
| #THEN | ||
| assert_success | ||
| assert_output --partial ${expected_msg} | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| #!/bin/bash | ||
| # Copyright (c) 2012-2017 Red Hat, Inc | ||
| # All rights reserved. This program and the accompanying materials | ||
| # are made available under the terms of the Eclipse Public License v1.0 | ||
| # which accompanies this distribution, and is available at | ||
| # http://www.eclipse.org/legal/epl-v10.html | ||
| # | ||
| # Contributors: | ||
| # Mario | ||
| # | ||
| # How to run this script: | ||
| # cd <root of che local git repository> | ||
| # sh agents/che-core-api-agent/src/test/resources/run_launcher_bats_tests.sh | ||
| # | ||
|
|
||
| images=(bitnami/che-codeigniter:3.1.3-r6 bitnami/che-express:4.15.3-r2 bitnami/che-java-play:1.3.12-r3 bitnami/che-laravel:5.4.23-r1 bitnami/che-rails:5.1.2-r0 bitnami/che-swift:3.1.1-r0 bitnami/che-symfony:3.3.2-r0 eclipse/centos_jdk8 eclipse/cpp_gcc eclipse/debian_jdk8 eclipse/debian_jre eclipse/dotnet_core eclipse/hadoop-dev eclipse/kotlin eclipse/node eclipse/php eclipse/php:5.6 eclipse/php:gae eclipse/selenium eclipse/ubuntu_android eclipse/ubuntu_go eclipse/ubuntu_jdk8 eclipse/ubuntu_jre eclipse/ubuntu_python:2.7 eclipse/ubuntu_python:gae_python2.7 eclipse/ubuntu_python:latest eclipse/ubuntu_rails kaloyanraev/che-zendserver registry.centos.org/che-stacks/centos-go registry.centos.org/che-stacks/centos-nodejs registry.centos.org/che-stacks/spring-boot registry.centos.org/che-stacks/vertx registry.centos.org/che-stacks/wildfly-swarm tomitribe/ubuntu_tomee_173_jdk8 registry.centos.org/che-stacks/centos-git) | ||
| arbitrary_images=(rhche/centos_jdk8 rhche/vertx rhche/ubuntu_jdk8 rhche/centos-nodejs rhche/spring-boot rhche/wildfly-swarm) | ||
|
|
||
| run_bats_test() { | ||
| export CHE_BASE_DIR=$(pwd) | ||
| export BATS_TEST_SCRIPT=${1} | ||
| export LAUNCHER_SCRIPT_TO_TEST=${2} | ||
| export DOCKER_IMAGE=${3} | ||
| docker run -ti --rm -e CHE_BASE_DIR -e LAUNCHER_SCRIPT_TO_TEST -e DOCKER_IMAGE \ | ||
| -v ${CHE_BASE_DIR}/${BATS_TEST_SCRIPT}:/scripts/launcher_tests.bats \ | ||
| -v ${CHE_BASE_DIR}/dockerfiles:/dockerfiles \ | ||
| -v /var/run/docker.sock:/var/run/docker.sock \ | ||
| eclipse/che-bats bats /scripts/launcher_tests.bats | ||
| } | ||
|
|
||
| for image in "${images[@]}"; do | ||
| launcher_script_to_test="wsagent/agent/src/main/resources/org.eclipse.che.ws-agent.script.sh" | ||
| bats_test_script="agents/che-core-api-agent/src/test/resources/agents-launchers-tests.bats" | ||
| echo "RUNNING LAUNCHER BATS TESTS FOR IMAGE ${image}" | ||
| run_bats_test "${bats_test_script}" "${launcher_script_to_test}" "${image}" | ||
| done | ||
|
|
||
| for arbitrary_image in "${arbitrary_images[@]}"; do | ||
| launcher_script_to_test="wsagent/agent/src/main/resources/org.eclipse.che.ws-agent.script.sh" | ||
| bats_test_script="agents/che-core-api-agent/src/test/resources/agents-launchers-tests-arbitraryuser.bats" | ||
| echo "RUNNING LAUNCHER BATS TESTS FOR IMAGE ${arbitrary_image}" | ||
| run_bats_test "${bats_test_script}" "${launcher_script_to_test}" "${arbitrary_image}" | ||
| done | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,15 @@ | |
| # Codenvy, S.A. - initial API and implementation | ||
| # | ||
|
|
||
|
|
||
| is_current_user_root() { | ||
| test "$(id -u)" = 0 | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. maybe we could avoid the return operations ? There was a problem hiding this comment. yes that's better. let me change it |
||
| is_current_user_sudoer() { | ||
| sudo -n true > /dev/null 2>&1 | ||
| } | ||
|
There was a problem hiding this comment.
and maybe it could be simplified to |
||
|
|
||
| unset PACKAGES | ||
| unset SUDO | ||
| command -v tar >/dev/null 2>&1 || { PACKAGES=${PACKAGES}" tar"; } | ||
|
|
@@ -23,7 +32,7 @@ if [ ${CURL_INSTALLED} = false ] && [ ${WGET_INSTALLED} = false ]; then | |
| CURL_INSTALLED=true | ||
| fi | ||
|
|
||
| is_current_user_root && is_current_user_sudoer || SUDO="sudo -E" | ||
|
There was a problem hiding this comment. Can you elaborate on what will happen if a user is root but sudo command is not installed in a container? There was a problem hiding this comment. @garagatyi if current user is root this code is not setting SUDO variable. Your scenario should work just fine. I've just tested with: unset SUDO
is_current_user_root() {
return 0
}
is_current_user_sudoer() {
return 0
}
is_current_user_root && is_current_user_sudoer || SUDO="sudo -E"
echo SUDO=${SUDO}
# output sould be:
# SUDO=But since it doesn't look easy to understand to me neither I've extracted it to a function and made it more readable and covered by the tests (and found a bug in the process): set_sudo_command() {
if is_current_user_sudoer && ! is_current_user_root; then SUDO="sudo -E"; else unset SUDO; fi
}There was a problem hiding this comment. Yes, it is what I had expected. In your example return value of function echos SUDO=sudo -E Now the logic of |
||
|
|
||
| CHE_DIR=$HOME/che | ||
| LOCAL_AGENT_BINARIES_URI='/mnt/che/exec-agent/exec-agent-${PREFIX}.tar.gz' | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to launch tests as part of the maven build ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it could be run as part of the profile integration tests and using docker containers
https://github.com/eclipse/che/blob/185273f8b92fe5523e676255cd7be172e90ee418/plugins/plugin-java-debugger/che-plugin-java-debugger-server/pom.xml#L234-L235
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be cool but I can't see how to do
docker execusing the docker-maven-plugin. It doesn't look so trivial anyway. Do you mind if we address that in a separate PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general tendency, if something is postponed then it won't be done at all :(
docker-maven-pluginallows starting container in privileged mode and then we can execute tests script inside it.May be I am mistaken but could you clarify what issue you faced here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate PR is fine for me as this PR should unblock the usage of a specific branch of Che for OpenShift.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to admit that you are right @tolusha :-) I was hoping in some softness since that's an improvement of an improvement I added to the codebase (the bats files). But man you don't seem to be much indulgent today so let's try to fix that ;-)
Let me try to address that today. If takes too long I will ask you again to be indulgent hoping to have more luck the second time.
In the fabric8 docker-maven-plugin I've seen a
docker:startgoal but not adocker:exec. And in the tests I've written we first start a container (docker run) and then executes the tests inside the container (docker exec). Maybedocker:watchmay help but not sure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @benoitf. At least someone has been indulgent today :-) However I will give it a try and tell you how it goes