Add support for Docker SecurityOpt (allowing GDB Debugging on unprivileged docker containers) #6856

hkolvenbach · 2017-10-20T06:47:17Z

Could someone please review the use of config variables? I am not entirely sure if I understand the translation from CHE_DOCKER_SECURITYOPT in che.env to @Named('che.docker.securityopt') correctly.
This also needs #6855 to be fixed to be tested

What does this PR do?

This PR allows to specify the securityOpt parameter in HostConfig to pass parameters to the docker daemon during container creation. the securityOpt parameter is equal to the command line option --security-opt of the command line. The value seccomp:unconfined allows gdb to run in unprivileged containers. Kubernetes uses seccomp:unconfined by default, but docker does not.

What issues does this PR fix or reference?

Related issues:
#4719
#4524
#5254
#4284

Add new config variable che.docker.securityopt / CHE_DOCKER_SECURITYOPT
Support for GDB in unprivileged docker containers

Release Notes

Support for GDB in unprivileged docker containers

Docs PR

eclipse-che/che-docs#305

codenvy-ci · 2017-10-20T07:52:13Z

Build # 4255 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/4255/ to view the results.

benoitf · 2017-10-20T08:04:43Z

@hkolvenbach could you please run the command

mvn -o com.coveo:fmt-maven-plugin:2.0.0:format

jenkins CI job is failing to invalid format in your modified files.
thanks

hkolvenbach · 2017-10-20T10:39:52Z

@benoitf sorry, probably still formatted with the old version. should be fixed now

… configuration variable Signed-off-by: [email protected] <[email protected]>

Signed-off-by: Hanno Kolvenbach <[email protected]>

benoitf · 2017-10-20T10:51:33Z

@hkolvenbach yes thanks.
This is related to the new java formatter change I introduced yesterday

Signed-off-by: Hanno Kolvenbach <[email protected]>

codenvy-ci · 2017-10-20T14:51:17Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4267/

skabashnyuk · 2017-10-20T16:09:25Z

Any volunteers to port to che6? @hkolvenbach @tolusha ?

garagatyi

Nice PR!
You used properties injection correctly.
Can you take a look at my inlined comments?

garagatyi · 2017-10-21T15:03:43Z

dockerfiles/init/manifests/che.env

+# Host SecurityOpt Parameters
+# This parameter allows to specify text values for the docker "--security-opt" parameter
+# or the HostConfig{SecurityOpt:[]} value of the docker machine API 
+# (https://docs.docker.com/engine/api/v1.33/#operation/ContainerCreate) 


We use 1.21 API by default for compatibility with older docker versions. Can you point to that version instead?

garagatyi · 2017-10-21T15:05:28Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

+# This parameter allows to specify custom security options for the created docker container
+# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed
+# for debugging with gdbserver
+che.docker.securityopt=seccomp:unconfined


Do we want to change default settings or just allow to change it? @hkolvenbach @tolusha

I would say yes to behave in the same way as the kubernetes / openshift implementation does. Any comments on this @tolusha?

garagatyi · 2017-10-21T15:06:52Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

+# This parameter allows to specify custom security options for the created docker container
+# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed
+# for debugging with gdbserver
+che.docker.securityopt=seccomp:unconfined


Have you tried it on systems that do not support seccomp? Is it just ignored?

@garagatyi didn't try it yet, any specific system I should test with?

I don't know where to test it and whether it is worth checking, just thought how it can influence users.
I tried to put an invalid value of seccomp profile and another security option. Both failed. So I assume that this would prevent Che starting workspaces if users host doesn't support seccomp.

thanks for the feedback, i will test with different values for securityopt and see how it behaves. If unsupported values fail, i will leave it empty as default

garagatyi · 2017-10-21T15:08:50Z

...n-docker-client/src/main/java/org/eclipse/che/plugin/docker/client/json/ContainerConfig.java

@@ -333,14 +333,18 @@ public ContainerConfig withExposedPorts(Map<String, Map<String, String>> exposed
 return this;
 }

+ // there is no "SecurityOpts" in the docker specification, only HostConfig{"SecurityOpt" .[]}


Please, use javadocs comments style to provide documentation, e.g.

/* * There is no "SecurityOpts" in the docker specification, only HostConfig{"SecurityOpt" .[]} */

garagatyi · 2017-10-21T15:13:13Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

@@ -161,6 +161,11 @@ che.docker.always_pull_image=true
 # to be able to launch their own runtimes which are embedded Docker containers.
 che.docker.privileged=false

+# This parameter allows to specify custom security options for the created docker container
+# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed
+# for debugging with gdbserver


Since it is supposed to be injected as an array can you add comment that values are separated by commas

garagatyi · 2017-10-21T15:21:34Z

assembly/assembly-wsmaster-war/src/main/webapp/WEB-INF/classes/codenvy/che.properties

+# This parameter allows to specify custom security options for the created docker container
+# seccomp:unconfined is the default for kubernetes, but not for docker. This is needed
+# for debugging with gdbserver
+che.docker.securityopt=seccomp:unconfined


Can you elaborate on how to set this option to default docker behavior?

@garagatyi default would be an empty parameter

garagatyi · 2017-10-21T15:29:42Z

dockerfiles/init/manifests/che.env

+# This is useful if you want to run e.g. Cpp debugging within the container through gdbserver
+# See https://github.com/eclipse/che/issues/4284 for details. This is only necessary for docker,
+# openshift seems to set "seccomp:unconfined" by default
+#CHE_DOCKER_SECURITYOPT="seccomp:unconfined"


I think that quotes should not be used here

hkolvenbach · 2017-10-23T06:58:06Z

Thanks for the remarks, I will fix them. Regarding the Che6 port: I can look into it, but not sure if I will manage to do it this week.

Signed-off-by: Hanno Kolvenbach <[email protected]>

garagatyi · 2017-10-24T10:52:06Z

@hkolvenbach Che6 has this area massively refactored. If you need any help you can contact me

garagatyi · 2017-10-24T11:38:13Z

@hkolvenbach I think that in case CHE_DOCKER_SECURITYOPT is set to empty value, e.g.:

CHE_DOCKER_SECURITYOPT=

the current default behavior will be used, right?
I'm afraid container creation would fail in that case. Can you check? If I'm correct, please, take a look at this class for the hint.

hkolvenbach · 2017-10-24T22:02:26Z

@garagatyi thanks for the support, i will come back to it when i am working on the che6 port.

i did a few more tests and can confirm that docker will definitely fail if any unknown value is provided. so i will change it do not provide anything on default. null or empty string seems to work though (at least on linux).

i will also check if an empty string will result in the default behavior.

hkolvenbach · 2017-10-24T22:34:36Z

@garagatyi i just confirmed that container creation will fail with an empty string, only null is allowed. do i understand correctly, that i need to implement a SecurityOptProvider similar to the dnsResolversProvider and create a seperate named parameter for it? or is there any easier way to check the che.docker.securityopt parameter for empty strings and set it to null instead?

garagatyi · 2017-10-25T06:13:39Z

I think that unconfined might be OK as a default value. I just wanted to mention it to hear a feedback from the community if any. Wrong values in other places of che.env would definitely brake workspaces start also, so it is not the case.
But the empty value has to be fixed because it is definitely what someone might use. Yes, you need a provider similar to DnsResolversprovider to fix that issue. But you don't have to inject provider as named string - just inject provider directly and get value from it in the constructor of MachineProviderImpl.

hkolvenbach · 2017-10-26T13:18:42Z

@garagatyi @eivantsov from my side I would consider my implementation for che 5.x ready. I have tested it with empty values, single values and multiple values for CHE_DOCKER_HOSTCONFIG_SECURITYOPT and this works for docker.

i will now start to look into the che 6 port.

garagatyi · 2017-10-27T09:42:29Z

@hkolvenbach take a look on direct usage of provider in tests garagatyi@2685f22

hkolvenbach · 2017-10-27T13:22:50Z

@garagatyi thanks, i integrated your changes and adapted everything to use CHE_DOCKER_SECURITYOPT again. somehow your commit doesn't pass validation, should I squash and sign it?

hkolvenbach · 2017-10-30T10:18:47Z

@garagatyi I am a bit lost in the Che6 source code. Could you point me to the file where the configuration for the docker container is generated? I hope i can take it from there

garagatyi · 2017-10-30T16:28:52Z

@hkolvenbach yes, you can squash my commit. I've signed Eclipse agreement so there are no legal issues with that.
Take a look at how DNS settings are done in Che6 https://github.com/eclipse/che/blob/che6/infrastructures/docker/infrastructure/src/main/java/org/eclipse/che/workspace/infrastructure/docker/provisioner/dns/DnsSettingsProvisioner.java#L24-L24
https://github.com/eclipse/che/blob/che6/infrastructures/docker/infrastructure/src/main/java/org/eclipse/che/workspace/infrastructure/docker/provisioner/dns/DnsResolversProvider.java#L31-L31
https://github.com/eclipse/che/blob/che6/infrastructures/docker/infrastructure/src/main/java/org/eclipse/che/workspace/infrastructure/docker/DockerInfraModule.java#L43-L43

ghost · 2017-10-31T12:35:24Z

Just checked gdb on OpenShift. I was able to start gdb and debug a binary using command line.

garagatyi · 2017-10-31T13:10:39Z

Great! @eivantsov maybe we should change defaults to that behavior?

ghost · 2017-10-31T13:15:51Z

@garagatyi i think it's safe enough to do so.

Rework injection Signed-off-by: Hanno Kolvenbach <[email protected]>

Signed-off-by: Hanno Kolvenbach <[email protected]>

hkolvenbach · 2017-11-06T11:27:38Z

@garagatyi I finally found time to port this pull request to che6. Tested it with empty configuration, 1 configuration value seccomp:unconfined and 2 values seccomp:unconfined,apparmor:unconfined

here it is #7203

garagatyi · 2017-11-06T15:27:13Z

@skabashnyuk there is a port for che6 already

garagatyi · 2017-11-07T07:41:16Z

@benoitf can you take a look?

benoitf · 2017-11-07T08:52:06Z

ci-build

benoitf · 2017-11-07T11:16:24Z

thanks @hkolvenbach

codenvy-ci · 2017-11-07T11:34:12Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/4313/

hkolvenbach requested review from benoitf, garagatyi, riuvshin and skabashnyuk as code owners October 20, 2017 06:47

hkolvenbach mentioned this pull request Oct 20, 2017

added description for CHE_DOCKER_SECURITYOPT parameter to debugging docs eclipse-che/che-docs#305

Merged

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/bug Outline of a bug - must adhere to the bug report template. labels Oct 20, 2017

[email protected] added 2 commits October 20, 2017 12:46

added SecurityOpt to container config and CHE_DOCKER_HOST_SECURITYOPT…

f123bbd

… configuration variable Signed-off-by: [email protected] <[email protected]>

changed from hostSecurityOpt to securityOpt because serialization failed

e69a2e5

Signed-off-by: Hanno Kolvenbach <[email protected]>

hkolvenbach force-pushed the kolvenbach.add-securityopt branch from d40f376 to 33f6079 Compare October 20, 2017 10:51

updated code formatting

6ef729e

Signed-off-by: Hanno Kolvenbach <[email protected]>

hkolvenbach force-pushed the kolvenbach.add-securityopt branch from 33f6079 to 6ef729e Compare October 20, 2017 10:54

benoitf added kind/enhancement A feature request - must adhere to the feature request template. team/platform and removed kind/bug Outline of a bug - must adhere to the bug report template. labels Oct 20, 2017

garagatyi reviewed Oct 21, 2017

View reviewed changes

proper documentation

93ff4d4

Signed-off-by: Hanno Kolvenbach <[email protected]>

hkolvenbach force-pushed the kolvenbach.add-securityopt branch from bb7a7d3 to 93ff4d4 Compare October 24, 2017 08:33

Hanno Kolvenbach added 2 commits November 2, 2017 09:23

added example with multiple values for securityopt

ca36be5

Rework injection Signed-off-by: Hanno Kolvenbach <[email protected]>

adapted to use provider and revert back to CHE_DOCKER_SECURITYOPT

d60e2d5

Signed-off-by: Hanno Kolvenbach <[email protected]>

hkolvenbach force-pushed the kolvenbach.add-securityopt branch from 9b8f2a2 to d60e2d5 Compare November 2, 2017 08:24

hkolvenbach mentioned this pull request Nov 6, 2017

Add support for Docker HostConfig SecurityOpt through che.env #7203

Merged

garagatyi approved these changes Nov 6, 2017

View reviewed changes

riuvshin approved these changes Nov 6, 2017

View reviewed changes

skabashnyuk approved these changes Nov 7, 2017

View reviewed changes

benoitf changed the title ~~Allow GDB Debugging on unprivileged docker containers~~ Add support for Docker SecurityOpt (allowing GDB Debugging on unprivileged docker containers) Nov 7, 2017

benoitf approved these changes Nov 7, 2017

View reviewed changes

benoitf merged commit 2c8cd59 into eclipse-che:master Nov 7, 2017

benoitf assigned hkolvenbach Nov 7, 2017

benoitf added this to the 5.21.0 milestone Nov 7, 2017

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Nov 7, 2017

This was referenced Nov 7, 2017

Update for Eclipse Che changes on PR https://github.com/eclipse/che/pull/6856 codenvy/codenvy#2521

Merged

Add new properties required in upstream che codenvy/codenvy#2524

Merged

hkolvenbach deleted the kolvenbach.add-securityopt branch March 12, 2018 23:47

Add support for Docker SecurityOpt (allowing GDB Debugging on unprivileged docker containers) #6856

Add support for Docker SecurityOpt (allowing GDB Debugging on unprivileged docker containers) #6856

Conversation

hkolvenbach commented Oct 20, 2017 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

codenvy-ci commented Oct 20, 2017

benoitf commented Oct 20, 2017

hkolvenbach commented Oct 20, 2017 • edited Loading

benoitf commented Oct 20, 2017

codenvy-ci commented Oct 20, 2017

skabashnyuk commented Oct 20, 2017

garagatyi left a 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

hkolvenbach commented Oct 23, 2017

garagatyi commented Oct 24, 2017

garagatyi commented Oct 24, 2017

hkolvenbach commented Oct 24, 2017 • edited Loading

hkolvenbach commented Oct 24, 2017 • edited Loading

garagatyi commented Oct 25, 2017

hkolvenbach commented Oct 26, 2017

garagatyi commented Oct 27, 2017

hkolvenbach commented Oct 27, 2017

hkolvenbach commented Oct 30, 2017

garagatyi commented Oct 30, 2017

ghost commented Oct 31, 2017

garagatyi commented Oct 31, 2017

ghost commented Oct 31, 2017

hkolvenbach commented Nov 6, 2017 • edited Loading

garagatyi commented Nov 6, 2017

garagatyi commented Nov 7, 2017

benoitf commented Nov 7, 2017

benoitf commented Nov 7, 2017

codenvy-ci commented Nov 7, 2017

hkolvenbach commented Oct 20, 2017 •

edited

Loading

hkolvenbach commented Oct 20, 2017 •

edited

Loading

hkolvenbach commented Oct 24, 2017 •

edited

Loading

hkolvenbach commented Oct 24, 2017 •

edited

Loading

hkolvenbach commented Nov 6, 2017 •

edited

Loading