Avoid using `sudo` in agent launchers when the current user is not a sudoer #5835

l0rd · 2017-07-28T10:55:30Z

What does this PR do?

Add a check in agent launchers to verify if the user is a sudoer. Then, if the user is not a sudoer, variable $SUDO is kept empty.

To check if a user is a sudoer we use the following bash function:

is_current_user_sudoer() {
    sudo -n true >& /dev/null && return 0 || return 1
}

I have added some bats files to tests if is_current_user_sudoer() works fine with all eclipse/che workspace docker images.

This modification has been tested on both Docker and OpenShift (creation of workspace).

What issues does this PR fix or reference?

Starting an agent with a user that was not a sudoer (e.g. UID 10000) was impossible.

Changelog

Allowing agents bootstrap by non privileged users

Release Notes

Allowing agents bootstrap by non privileged users

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

codenvy-ci · 2017-07-28T10:56:01Z

Can one of the admins verify this patch?

codenvy-ci · 2017-07-28T10:59:01Z

Can one of the admins verify this patch?

benoitf · 2017-07-28T12:50:36Z

agents/che-core-api-agent/src/test/resources/run_launcher_bats_tests.sh

+#   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/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)


is there a way to grab list of images automatically by asking dockerhub or github repo ?

Probably parsing stacks.json. That may be done in another PR.

benoitf · 2017-07-28T13:17:30Z

agents/exec/src/main/resources/org.eclipse.che.exec.script.sh

+
+is_current_user_sudoer() {
+    sudo -n true >& /dev/null && return 0 || return 1
+}


& is bash
you should use

is_current_user_sudoer() { sudo -n true > /dev/null 2>&1 && return 0 || return 1 }

and maybe it could be simplified to

is_current_user_sudoer() { sudo -n true > /dev/null 2>&1 }

yes let me fix that

benoitf · 2017-07-28T13:33:43Z

agents/exec/src/main/resources/org.eclipse.che.exec.script.sh

+is_current_user_root() {
+    test "$(id -u)" = 0 && return 0 || return 1
+}
+


maybe we could avoid the return operations ?

is_current_user_root() { test "$(id -u)" = 0 }

yes that's better. let me change it

benoitf · 2017-07-28T13:36:10Z

agents/exec/src/main/resources/org.eclipse.che.exec.script.sh

@@ -23,7 +32,7 @@ if [ ${CURL_INSTALLED} = false ] && [ ${WGET_INSTALLED} = false ]; then
  CURL_INSTALLED=true
 fi

-test "$(id -u)" = 0 || SUDO="sudo -E"
+if is_current_user_root && is_current_user_sudoer; then SUDO="sudo -E"; fi


is_current_user_root && is_current_user_sudoer || SUDO="sudo -E"

?

Ok your version is more consistent with the rest of the script. Will change that

benoitf · 2017-07-28T13:37:09Z

agents/che-core-api-agent/src/test/resources/run_launcher_bats_tests.sh

@@ -0,0 +1,43 @@
+#!/bin/bash


is there a way to launch tests as part of the maven build ?

any suggestion?

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

That would be cool but I can't see how to do docker exec using the docker-maven-plugin. It doesn't look so trivial anyway. Do you mind if we address that in a separate PR?

As a general tendency, if something is postponed then it won't be done at all :(
docker-maven-plugin allows 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?

separate PR is fine for me as this PR should unblock the usage of a specific branch of Che for OpenShift.

As a general tendency, if something is postponed then it won't be done at all :(

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.

May be I am mistaken but could you clarify what issue you faced here?

In the fabric8 docker-maven-plugin I've seen a docker:start goal but not a docker: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). Maybe docker:watch may help but not sure.

Thanks @benoitf. At least someone has been indulgent today :-) However I will give it a try and tell you how it goes

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

garagatyi · 2017-07-31T11:48:14Z

agents/exec/src/main/resources/org.eclipse.che.exec.script.sh

@@ -23,7 +32,7 @@ if [ ${CURL_INSTALLED} = false ] && [ ${WGET_INSTALLED} = false ]; then
  CURL_INSTALLED=true
 fi

-test "$(id -u)" = 0 || SUDO="sudo -E"
+is_current_user_root && is_current_user_sudoer || SUDO="sudo -E"


Can you elaborate on what will happen if a user is root but sudo command is not installed in a container?

@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 }

Yes, it is what I had expected. In your example return value of function is_current_user_sudoer should return 1 since sudo is not installed. Then

is_current_user_root && is_current_user_sudoer || SUDO="sudo -E" echo SUDO=${SUDO}

echos SUDO=sudo -E

Now the logic of set_sudo_command function looks good.

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

…sudoer (eclipse-che#5835) Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

Avoid using sudo in agent launchers when the user is not a sudoer

60d2721

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

l0rd requested review from ashumilova and tolusha as code owners July 28, 2017 10:55

l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label Jul 28, 2017

l0rd requested a review from benoitf July 28, 2017 11:46

benoitf reviewed Jul 28, 2017

View reviewed changes

Fixup after Florent review

6014596

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

skabashnyuk requested a review from garagatyi July 31, 2017 06:30

garagatyi reviewed Jul 31, 2017

View reviewed changes

Fixup after Alex review

aa11357

Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

tolusha approved these changes Aug 1, 2017

View reviewed changes

garagatyi approved these changes Aug 1, 2017

View reviewed changes

l0rd merged commit a419690 into eclipse-che:master Aug 1, 2017

l0rd deleted the improve-launchers-sudo branch August 1, 2017 06:42

l0rd mentioned this pull request Aug 1, 2017

Add Che debug commands for IO quick start project types openshiftio/openshift.io#454

Closed

2 tasks

slemeur added this to the 5.16.0 milestone Aug 9, 2017

dmytro-ndp pushed a commit to dmytro-ndp/che that referenced this pull request Aug 16, 2017

Avoid using sudo in agent launchers when the current user is not a …

172f6ad

…sudoer (eclipse-che#5835) Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

JPinkney pushed a commit to JPinkney/che that referenced this pull request Aug 17, 2017

Avoid using sudo in agent launchers when the current user is not a …

2c20b1d

…sudoer (eclipse-che#5835) Signed-off-by: Mario Loriedo <mloriedo@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid using `sudo` in agent launchers when the current user is not a sudoer #5835

Avoid using `sudo` in agent launchers when the current user is not a sudoer #5835

l0rd commented Jul 28, 2017

codenvy-ci commented Jul 28, 2017

codenvy-ci commented Jul 28, 2017

benoitf Jul 28, 2017

l0rd Jul 28, 2017

benoitf Jul 28, 2017

l0rd Jul 28, 2017

l0rd Jul 30, 2017

benoitf Jul 28, 2017

l0rd Jul 28, 2017

l0rd Jul 30, 2017

benoitf Jul 28, 2017

l0rd Jul 28, 2017

l0rd Jul 30, 2017

benoitf Jul 28, 2017

l0rd Jul 28, 2017

benoitf Jul 28, 2017

l0rd Jul 30, 2017

tolusha Jul 31, 2017

benoitf Jul 31, 2017

l0rd Jul 31, 2017

l0rd Jul 31, 2017

garagatyi Jul 31, 2017

l0rd Jul 31, 2017 •

edited

Loading

garagatyi Aug 1, 2017

Avoid using sudo in agent launchers when the current user is not a sudoer #5835

Avoid using sudo in agent launchers when the current user is not a sudoer #5835

Conversation

l0rd commented Jul 28, 2017

What does this PR do?

What issues does this PR fix or reference?

Changelog

Release Notes

codenvy-ci commented Jul 28, 2017

codenvy-ci commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l0rd Jul 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avoid using `sudo` in agent launchers when the current user is not a sudoer #5835

Avoid using `sudo` in agent launchers when the current user is not a sudoer #5835

l0rd Jul 31, 2017 •

edited

Loading