CHE-14398: Copy SSH keys with proper permissions #15167

vinokurig · 2019-11-11T15:44:54Z

What does this PR do?

The OpenSSH daemon requires 600 file mode access permissions for private keys.
Mounted keys have 640 permissions despite setting DefaultMode to 600.
As a workaround add an init container and set a command that copies the SSH keys to mounted to all workspace pod containers and manually set them needed permissions.

What issues does this PR fix or reference?

#14398

Release Notes

Docs PR

che-bot · 2019-11-11T15:56:37Z

✅ E2E Happy path tests succeed 🎉

See Details

Jenkins job
logs and configs
Happy path tests DevFile
image: maxura/che-server:15167

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

vzhukovs

LGTM, didn't test it, though

che-bot · 2019-11-11T17:00:51Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15167
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

vinokurig · 2019-11-12T06:55:51Z

ci-test

che-bot · 2019-11-12T08:11:29Z

E2E tests of Eclipse Che Multiuser on OCP has failed:

Build details
Test report
docker image: eclipseche/che-server:15167
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.
Use command "ci-test" to rerun the test.

che-bot · 2019-11-12T13:27:01Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15167

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

vinokurig · 2019-11-12T13:28:16Z

crw-ci-test

che-bot · 2019-11-12T13:44:31Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15167

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2019-11-12T14:08:47Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15167

vinokurig · 2019-11-12T15:17:15Z

crw-ci-test

che-bot · 2019-11-12T15:33:16Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15167

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

vinokurig · 2019-11-12T15:35:06Z

crw-ci-test

che-bot · 2019-11-12T15:51:12Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15167

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

amisevsk

I'm leaning towards -1 on this PR, do we really need to add yet another init container to pods? If we do have to, it should at least be handled in the same way as all other containers in workspaces and be provisioned using the existing flow. The current PR looks like a hack.

Also, has this been tested on a (non-minishift) OpenShift deployment? From what I understand, the arbitrary user ID default will mean that the key is not readable by anyone.

amisevsk · 2019-11-12T15:48:09Z

...ava/org/eclipse/che/workspace/infrastructure/kubernetes/provision/VcsSshKeysProvisioner.java

+    Container initContainer =
+        new ContainerBuilder()
+            .withName("ssh-private-keys-modifier")
+            .withImage("centos:centos7")


This is bad practice and will be a nightmare for airgap users.

Used a property

davidwindell · 2019-11-12T16:05:48Z

Please don't abandon this, it is the only way we can use Che on Kubernetes/Rancher because at the moment there are many issues preventing running Che as a non-root user in this environment.

vinokurig · 2019-11-13T07:16:29Z

crw-ci-test

vinokurig · 2019-11-13T07:30:02Z

@amisevsk

If we do have to, it should at least be handled in the same way as all other containers

We cannot override command in a common container, so I used init container.

Also, has this been tested on a (non-minishift) OpenShift deployment?

no

From what I understand, the arbitrary user ID default will mean that the key is not readable by anyone.

Do you mean the case when the workspace is shared?

che-bot · 2019-11-13T07:31:37Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15167

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2019-11-13T09:00:07Z

❌ E2E Happy path tests failed ❗

See Details

Jenkins job
test report
logs and configs
Happy path tests DevFile
image: maxura/che-server:15167

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

⚠️ https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

ℹ️ Use comment "crw-ci-test" to rerun happy path E2E test.

che-bot · 2019-11-13T09:41:23Z

E2E tests of Eclipse Che Multiuser on OCP has been successful:

Build details
Test report
docker image: eclipseche/che-server:15167

amisevsk · 2019-11-14T04:29:15Z

If we do have to, it should at least be handled in the same way as all other containers

We cannot override command in a common container, so I used init container.

Using an init container is fine, but hard-coding it in this way is not, IMO. To be clear, this will break offline/airgap.

From what I understand, the arbitrary user ID default will mean that the key is not readable by anyone.

Do you mean the case when the workspace is shared?

I haven't looked into this deeply and checked whether it causes issues, but containers running on OpenShift a) do not have root privileges, and b) start with a semi-randomly assigned user ID. Will the init container copy the ssh keys in with the correct owner?

vinokurig · 2019-11-14T07:03:40Z

@amisevsk

Using an init container is fine, but hard-coding it in this way is not, IMO. To be clear, this will break offline/airgap.

I have reworked it to use a property

I haven't looked into this deeply and checked whether it causes issues, but containers running on OpenShift a) do not have root privileges, and b) start with a semi-randomly assigned user ID. Will the init container copy the ssh keys in with the correct owner?

Basically this workaround is designed to work only when the che assembly is set up to have a root user @davidwindell Am I correct?

l0rd · 2019-11-14T08:24:55Z

@vinokurig @vparfonov I am in agreement with @amisevsk here. This look like a hack whereas we had a solution for @davidwindell problem and potentially for any permission related problem: Add a wsmaster property to specify workspaces default security context · Issue #15138 · eclipse/che.

vparfonov · 2019-11-14T08:39:28Z

@amisevsk, sorry but I don't follow what problem we will have in case offline/airgap, @vinokurig replace hard coded image id with property that already in use. (see commit)

@l0rd so should we close this PR and wait on proper fixes from #15138?

BTW issue #15138 has P2 priority

l0rd · 2019-11-14T10:00:48Z

@vparfonov I think that it makes more sense to work on #15138 yes! I made it P1

davidwindell · 2019-11-14T10:03:39Z

If you do abandon, this please note that it means we won't be able to use Che at all until #14330 is fixed also.

l0rd · 2019-11-14T10:12:16Z

If you do abandon, this please note that it means we won't be able to use Che at all until #14330 is fixed also.

@davidwindell but #15138 would fix this as well right? You will be able to run containers as root user or any user id you would like and that should fix the permissions problem.

vinokurig · 2019-11-14T10:16:02Z

@l0rd #15138 will not fix the problem with SSH keys, we still would need to copy them.

l0rd · 2019-11-14T10:47:00Z

@l0rd #15138 will not fix the problem with SSH keys, we still would need to copy them.

It doesn't look like we will need the copy, mounting should be enough kubernetes/kubernetes#81089 (comment)

vinokurig · 2019-11-14T14:45:39Z

@l0rd

It doesn't look like we will need the copy, mounting should be enough kubernetes/kubernetes#81089 (comment)

I don't understand why mounting should be enough. I have setup my minikube with:

CHE_INFRA_KUBERNETES_POD_SECURITY__CONTEXT_FS__GROUP: 0
CHE_INFRA_KUBERNETES_POD_SECURITY__CONTEXT_RUN__AS__USER: 0

and it worked as usual (the mounted key still has 640 permissions instead of 600)
BTW, from kubernetes/kubernetes#81089 (comment)

So in this scenario (which is not uncommon I would guess), there is little to no use for the defaultMode

amisevsk · 2019-11-14T19:29:36Z

@vparfonov

sorry but I don't follow what problem we will have in case offline/airgap, @vinokurig replace hard coded image id with property that already in use. (see commit)

I hadn't seen this commit and it hadn't been mentioned when I made my second comment -- apologies. While that removes the hard blocker for merging, IMO this PR is still a hack. Either way, I won't block merging if it's a hard requirement for some.

vinokurig · 2019-11-22T06:40:30Z

Is not actual any more

vinokurig requested review from vparfonov, vzhukovs and sleshchenko November 11, 2019 15:44

vinokurig requested review from amisevsk, l0rd, metlos, nickboldt and rhopp as code owners November 11, 2019 15:44

che-bot 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 Nov 11, 2019

vzhukovs approved these changes Nov 11, 2019

View reviewed changes

CHE-14398: Copy SSH keys with proper permissions

1c98c47

vinokurig force-pushed the CHE-14398 branch from 5c60d98 to 1c98c47 Compare November 12, 2019 13:10

amisevsk reviewed Nov 12, 2019

View reviewed changes

fixup! CHE-14398: Copy SSH keys with proper permissions

3f89219

vparfonov approved these changes Nov 13, 2019

View reviewed changes

vinokurig closed this Nov 22, 2019

vinokurig deleted the CHE-14398 branch February 18, 2020 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CHE-14398: Copy SSH keys with proper permissions #15167

CHE-14398: Copy SSH keys with proper permissions #15167

vinokurig commented Nov 11, 2019

che-bot commented Nov 11, 2019

vzhukovs left a comment

che-bot commented Nov 11, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

che-bot commented Nov 12, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

che-bot commented Nov 12, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

amisevsk left a comment

amisevsk Nov 12, 2019

vinokurig Nov 13, 2019

davidwindell commented Nov 12, 2019

vinokurig commented Nov 13, 2019

vinokurig commented Nov 13, 2019

che-bot commented Nov 13, 2019

che-bot commented Nov 13, 2019

che-bot commented Nov 13, 2019

amisevsk commented Nov 14, 2019

vinokurig commented Nov 14, 2019

l0rd commented Nov 14, 2019 •

edited

Loading

vparfonov commented Nov 14, 2019

l0rd commented Nov 14, 2019

davidwindell commented Nov 14, 2019

l0rd commented Nov 14, 2019

vinokurig commented Nov 14, 2019

l0rd commented Nov 14, 2019

vinokurig commented Nov 14, 2019

amisevsk commented Nov 14, 2019

vinokurig commented Nov 22, 2019

CHE-14398: Copy SSH keys with proper permissions #15167

CHE-14398: Copy SSH keys with proper permissions #15167

Conversation

vinokurig commented Nov 11, 2019

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

che-bot commented Nov 11, 2019

vzhukovs left a comment

Choose a reason for hiding this comment

che-bot commented Nov 11, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

che-bot commented Nov 12, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

che-bot commented Nov 12, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

vinokurig commented Nov 12, 2019

che-bot commented Nov 12, 2019

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Nov 12, 2019

Choose a reason for hiding this comment

vinokurig Nov 13, 2019

Choose a reason for hiding this comment

davidwindell commented Nov 12, 2019

vinokurig commented Nov 13, 2019

vinokurig commented Nov 13, 2019

che-bot commented Nov 13, 2019

che-bot commented Nov 13, 2019

che-bot commented Nov 13, 2019

amisevsk commented Nov 14, 2019

vinokurig commented Nov 14, 2019

l0rd commented Nov 14, 2019 • edited Loading

vparfonov commented Nov 14, 2019

l0rd commented Nov 14, 2019

davidwindell commented Nov 14, 2019

l0rd commented Nov 14, 2019

vinokurig commented Nov 14, 2019

l0rd commented Nov 14, 2019

vinokurig commented Nov 14, 2019

amisevsk commented Nov 14, 2019

vinokurig commented Nov 22, 2019

l0rd commented Nov 14, 2019 •

edited

Loading