Make Theia sidecar/stack secure, add an ability to use new JWT auth on a per-workspace basis #11410

garagatyi · 2018-09-30T09:59:24Z

What does this PR do?

This PR is designed to be reviewed in a commit-by-commit manner. The scope of each commit is very limited to the purpose described in its message.

Description of why:
In sidecar-based workspaces with Theia sidecar, we can not use current security mechanism. To overcome it Che platform team implemented JWTProxy based implementation. We can switch these implementations for the whole Che, but this is not enough in a case when old workspaces are mostly used but sidecar-based workspaces evaluation is needed. Since JWTProxy based auth wasn't tested well enough as it is a non-default option I decided to switch only sidecar-based workspaces to it.

Description of what:
Adds an ability to enable workspace security implementation on a per-workspace basis.
Enables JWTProxy based security to sidecar-based workspaces if authentication is enabled in Che.
Adds secure and cookiesAuthEnabled attributes to Theia stacks, so they are secure on a multiuser Che in contrast to the current state in master.
Adds other small fixes, such as fixing warning codes for workspace starting components.

Description of how:
In ServersConverter I used SecureServerExposerFactoryProvider to create SecureServerExposerFactory directly and removed its binding from the DI container.
Then I added an argument to the get() method of the provider. It accepts KubernetesEnvironment as an argument. The provider now does the deccision on what security implementation should be used not just by checking global Che property but also by looking at KuberneterEnvironment attributes KuberneterEnvironment.getAttributes(). When attribute che.server.secure_exposer is present in the environment and contains bound implementation the provider uses this implementation instead of the globally configured with the property with the same name. If there is no attribute or implementation not found the provider falls back to the globally configured implementation.
Then in che.server.secure_exposer I added code that adds attribute che.server.secure_exposer with value jwtproxy if authentication is enabled in Che.

How to test it:
Prerequsites:
Theia will work in secure sidecar-based workspaces only after applying this PR and populating it to your che-plugin-registry instance and applying PR to the plugin broker.

Start Che in multiuser mode.
Create and start sidecar-based workspace.
Create and start regular Che 6 workspace, but add attribute che.server.secure_exposer with value jwtproxy to workspace config attributes.
Create workspace from Theia stack.
All 3 workspaces from the previous steps are secured with JWTProxy instead of being completely unsecure as it is in master branch.

What issues does this PR fix or reference?

To vitness working sidecar-based Theia secured in multi-user Che PRs need to be merged:
eclipse-che/che-plugin-broker#11
https://github.com/ws-skeleton/che-editor-theia/pull/7
Fixes redhat-developer/rh-che#853

Release Notes

Docs PR

…binding time Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi · 2018-09-30T09:59:36Z

ci-test

riuvshin · 2018-09-30T10:17:13Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11410
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

garagatyi · 2018-09-30T10:52:16Z

ci-test

riuvshin · 2018-09-30T13:45:56Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:11410
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

garagatyi · 2018-10-01T06:20:47Z

@eclipse/eclipse-che-qa Please, take a look at the report

sleshchenko · 2018-10-01T06:57:20Z

...e/che/workspace/infrastructure/kubernetes/provision/restartpolicy/RestartPolicyRewriter.java

@@ -27,6 +27,8 @@
 * @author Alexander Garagatyi
 */
 public class RestartPolicyRewriter implements ConfigurationProvisioner {
+  public static final int RESTART_POLICY_SET_TO_NEVER = 4104;


I think it makes sense to place warning codes in one place, like Constants class.

Make sense, but it is not related to this particular PR I suppose. Can you create an issue for that?

I could but I afraid it won't get needed priority to be fixed.
I think it makes sense to do it only in part of a code that is changed in this PR. But I'm OK to leave it as it is.

sleshchenko · 2018-10-01T07:05:45Z

...he/workspace/infrastructure/kubernetes/server/secure/SecureServerExposerFactoryProvider.java

 import org.eclipse.che.inject.ConfigurationException;
 import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;

 /**
 * Provides implementation of {@link SecureServerExposerFactory} according to configuration property
- * with name `che.server.secure_exposer`.
+ * or workspace config attribute with name `che.server.secure_exposer`.


It makes sense to rewrite this java doc to make clear priority of different configuration sources.

sleshchenko · 2018-10-01T07:08:25Z

...lipse/che/workspace/infrastructure/kubernetes/wsplugins/KubernetesPluginsToolingApplier.java

+
+    if (isAuthEnabled) {
+      // enable per-workspace security with JWT proxy for sidecar based WS
+      // because it is the only WS security implementation supported for now


please, don't use WS abbreviation where it is possible since there is a collision with Workspace and WebSocket meaning.

SkorikSergey · 2018-10-02T09:02:24Z

Selenium tests execution on Eclipse Che Multiuser on OCP (https://ci.codenvycorp.com/job/che-pullrequests-test-ocp/892/) doesn't show any regression against this Pull Request.

sleshchenko

LGTM

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Rework SecureServerExposerFactoryProvider to be used at runtime, not …

bb43e5a

…binding time Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi requested review from ibuziuk, skabashnyuk, mshaposhnik and sleshchenko September 30, 2018 09:59

garagatyi requested a review from ashumilova as a code owner September 30, 2018 09:59

benoitf added the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 30, 2018

benoitf added the kind/enhancement A feature request - must adhere to the feature request template. label Sep 30, 2018

sleshchenko reviewed Oct 1, 2018

View reviewed changes

mshaposhnik approved these changes Oct 1, 2018

View reviewed changes

skabashnyuk approved these changes Oct 2, 2018

View reviewed changes

vkuznyetsov mentioned this pull request Oct 2, 2018

QE-2018-10-02 (Sprint: 155) #10992

Closed

47 tasks

ibuziuk approved these changes Oct 2, 2018

View reviewed changes

sleshchenko approved these changes Oct 2, 2018

View reviewed changes

Oleksandr Garagatyi added 5 commits October 3, 2018 09:07

Add an ability to enable JWT security exposer from WS config attribute

b157c63

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Fix missing constants

80923a2

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Set JWT server exposer attribute to environment if auth is enabled

1ddaeba

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Make Chee 6 Theia stack secure

d91cc60

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

Change error code cause it is occupied

b698aec

Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>

garagatyi force-pushed the secureWSNEXT branch from 2f13706 to b698aec Compare October 3, 2018 06:07

garagatyi merged commit eb6b28d into eclipse-che:master Oct 3, 2018

garagatyi deleted the secureWSNEXT branch October 3, 2018 06:08

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 Oct 3, 2018

benoitf added this to the 6.12.0 milestone Oct 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Theia sidecar/stack secure, add an ability to use new JWT auth on a per-workspace basis #11410

Make Theia sidecar/stack secure, add an ability to use new JWT auth on a per-workspace basis #11410

garagatyi commented Sep 30, 2018 •

edited

Loading

garagatyi commented Sep 30, 2018

riuvshin commented Sep 30, 2018

garagatyi commented Sep 30, 2018

riuvshin commented Sep 30, 2018

garagatyi commented Oct 1, 2018

sleshchenko Oct 1, 2018

garagatyi Oct 1, 2018

sleshchenko Oct 2, 2018

sleshchenko Oct 1, 2018

garagatyi Oct 1, 2018

sleshchenko Oct 1, 2018

garagatyi Oct 1, 2018

SkorikSergey commented Oct 2, 2018

sleshchenko left a comment

Make Theia sidecar/stack secure, add an ability to use new JWT auth on a per-workspace basis #11410

Make Theia sidecar/stack secure, add an ability to use new JWT auth on a per-workspace basis #11410

Conversation

garagatyi commented Sep 30, 2018 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

garagatyi commented Sep 30, 2018

riuvshin commented Sep 30, 2018

garagatyi commented Sep 30, 2018

riuvshin commented Sep 30, 2018

garagatyi commented Oct 1, 2018

sleshchenko Oct 1, 2018

Choose a reason for hiding this comment

garagatyi Oct 1, 2018

Choose a reason for hiding this comment

sleshchenko Oct 2, 2018

Choose a reason for hiding this comment

sleshchenko Oct 1, 2018

Choose a reason for hiding this comment

garagatyi Oct 1, 2018

Choose a reason for hiding this comment

sleshchenko Oct 1, 2018

Choose a reason for hiding this comment

garagatyi Oct 1, 2018

Choose a reason for hiding this comment

SkorikSergey commented Oct 2, 2018

sleshchenko left a comment

Choose a reason for hiding this comment

garagatyi commented Sep 30, 2018 •

edited

Loading