feat: secure workspace services #1045

sparkoo · 2021-09-01T10:30:54Z

What does this PR do?

Secures workspace services with devworkspace engine on OpenShift.

There is a slight change in exposing logic. The ingresses/routes are not merged into one when exposing same port.

Beside that, it also always deploys with gateway on kubernetes when devworkspaces are enabled.

How it works?

Nowdays, workspace service is open to the cluster. It exposes all workspace ports and communication goes directly to target container, so anyone in the cluster can access it.

This patch adds traefik container into workspace pod and narrows the service to only single port (targeting to the traefik) for all endpoints that exposes on subpath. That means that all communication that is coming to the workspace service, must go through the traefik. The traefik is configured so it checks the requests with kube-rbac-proxy (in main che-gateway) using forwardAuth middleware (https://doc.traefik.io/traefik/middlewares/http/forwardauth/). Kube-rbac-proxy is checking whether token in authorization header can access the service in workspace's namespace. If this check is OK, traefik route the request to correct port inside the workspace pod.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

eclipse-che/che#20190

How to test this PR?

deploy with che-operator image quay.io/mvala/che-operator:gh20190-secureWsServicesTraefik
start a workspace with user1 and get the service name
start a workspace with user2 and try curl to the service of user1's workspace. It is now not possible without user1's token in authorization header

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

openshift-ci · 2021-09-01T10:30:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo · 2021-09-15T10:33:09Z

/retest

metlos

I've not tested it in a running cluster yet, but here's a couple of small comments...

controllers/devworkspace/solver/che_routing.go

tolusha · 2021-09-15T13:24:26Z

pkg/deploy/defaults.go

@@ -360,7 +360,7 @@ func DefaultPullPolicyFromDockerImage(dockerImage string) string {
 }

 func GetSingleHostExposureType(cr *orgv1.CheCluster) string {
-	if util.IsOpenShift {
+	if util.IsOpenShift || cr.Spec.DevWorkspace.Enable {


Why do we need this changes?
So, it means that gateway will be default for k8s when devworkspace is enabled.

Exactly. There was the agreement on the last cabal that we're doing that, if I remember correctly.

Then DefaultOpenShiftSingleHostExposureType const has no sense anymore.
Let's remove it and just return gateway

there will be only gateway with devworkspaces. native does not really work with native auth. Everything we want to protect with oauth-proxy and kube-rbac-proxy has to go through the gateway.

pkg/deploy/gateway/gateway.go

skabashnyuk · 2021-09-16T07:03:06Z

@sparkoo can you describe in the description how in general protection has been made.

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo · 2021-09-16T09:23:46Z

@sparkoo can you describe in the description how in general protection has been made.

added "How it works" paragraph

sparkoo · 2021-09-16T10:57:18Z

/retest

metlos · 2021-09-16T16:53:21Z

Btw I've read the description again and am I correct in saying that the request is checked using kube-rbac-proxy also in the workspace pod (essentially again?). This is important so that we guard against in-cluster access by other users. I thought I saw it in the code but I understood your description otherwise. Could you please clarify?

sparkoo · 2021-09-16T16:55:57Z

Btw I've read the description again and am I correct in saying that the request is checked using kube-rbac-proxy also in the workspace pod (essentially again?). This is important so that we guard against in-cluster access by other users. I thought I saw it in the code but I understood your description otherwise. Could you please clarify?

Yes, it's now checked twice. First in main che-gateway https://github.com/eclipse-che/che-operator/pull/1045/files#diff-ebca2eefe12f7ba4a722c53d574ba1b2adee412909da8cdbc974c8f7fcbfb02fR468 then second time in workspace gateway https://github.com/eclipse-che/che-operator/pull/1045/files#diff-ebca2eefe12f7ba4a722c53d574ba1b2adee412909da8cdbc974c8f7fcbfb02fR554

sleshchenko

Nothing really meaningful I can bring after reviewing changes.
Works fine for user1/user2 scenario

but seems cluster admin is able to open any workspace, I assume it's expected but it should be made clear in the documentation, because it's a change from the previous auth model

sleshchenko · 2021-09-17T07:35:07Z

controllers/devworkspace/solver/che_routing.go

@@ -533,7 +702,7 @@ func determineEndpointScheme(gatewayEnabled bool, e dw.Endpoint) string {
 	upgradeToSecure := e.Secure

 	// gateway is always on HTTPS, so if the endpoint is served through the gateway, we need to use the TLS'd variant.
-	if gatewayEnabled && e.Attributes.GetString(urlRewriteSupportedEndpointAttributeName, nil) == "true" {
+	if e.Attributes.GetString(urlRewriteSupportedEndpointAttributeName, nil) == "true" {


that's such a huge file... Not really related to the PR but it deserves refactoring.

sleshchenko · 2021-09-17T07:44:31Z

controllers/devworkspace/solver/che_routing.go

-}
-
-func (c *CheRoutingSolver) cheSpecObjects(cheManager *v2alpha1.CheCluster, routing *dwo.DevWorkspaceRouting, workspaceMeta solvers.DevWorkspaceMetadata) (solvers.RoutingObjects, error) {
+func (c *CheRoutingSolver) cheSpecObjects(cheCluster *v2alpha1.CheCluster, routing *dwo.DevWorkspaceRouting, workspaceMeta solvers.DevWorkspaceMetadata) (solvers.RoutingObjects, error) {


maybe it makes sense to move this method and its dependency to a separate file. But makes sense to move it out of the PR, since it would not make this PR reviewing easier

openshift-ci · 2021-09-17T10:10:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: metlos, sleshchenko, sparkoo, tolusha

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress label Sep 1, 2021

sparkoo force-pushed the gh20190-secureWsServicesTraefik branch 2 times, most recently from a41d73c to 9b56256 Compare September 7, 2021 20:39

openshift-ci bot added the needs-rebase label Sep 9, 2021

sparkoo force-pushed the gh20190-secureWsServicesTraefik branch from 9b56256 to 02d7589 Compare September 9, 2021 12:47

openshift-ci bot removed the needs-rebase label Sep 9, 2021

sparkoo force-pushed the gh20190-secureWsServicesTraefik branch from 02d7589 to b86d5bf Compare September 9, 2021 14:20

openshift-ci bot added the needs-rebase label Sep 9, 2021

sparkoo force-pushed the gh20190-secureWsServicesTraefik branch from 0a38a1f to f0a939c Compare September 9, 2021 15:30

openshift-ci bot removed the needs-rebase label Sep 9, 2021

sparkoo changed the title ~~Gh20190 secure ws services traefik~~ feat: Gh20190 secure ws services traefik Sep 9, 2021

sparkoo force-pushed the gh20190-secureWsServicesTraefik branch from acf4983 to b1e4f86 Compare September 10, 2021 17:26

sparkoo marked this pull request as ready for review September 13, 2021 10:47

sparkoo requested review from AndrienkoAleksandr, flacatus, mmorhun, nickboldt and tolusha as code owners September 13, 2021 10:47

openshift-ci bot removed the do-not-merge/work-in-progress label Sep 13, 2021

sparkoo requested review from metlos, mshaposhnik, skabashnyuk and xbaran4 September 13, 2021 10:48

sparkoo force-pushed the gh20190-secureWsServicesTraefik branch from 24495e1 to 8427238 Compare September 15, 2021 06:50

feat: devworkspace services auth

78b6367

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo force-pushed the gh20190-secureWsServicesTraefik branch from 8427238 to 78b6367 Compare September 15, 2021 07:16

sparkoo added 3 commits September 15, 2021 10:03

don't expose same path multiple times

31e6f04

Signed-off-by: Michal Vala <mvala@redhat.com>

cleanup

60cf838

Signed-off-by: Michal Vala <mvala@redhat.com>

fix multiple routes on same port

bb0bae5

Signed-off-by: Michal Vala <mvala@redhat.com>

sparkoo changed the title ~~feat: Gh20190 secure ws services traefik~~ feat: secure workspace services Sep 15, 2021

metlos reviewed Sep 15, 2021

View reviewed changes

tolusha reviewed Sep 15, 2021

View reviewed changes

pkg/deploy/gateway/gateway.go Show resolved Hide resolved

sparkoo added 2 commits September 16, 2021 09:29

cleanup

1ecbbb6

Signed-off-by: Michal Vala <mvala@redhat.com>

cleanup

9966ab0

Signed-off-by: Michal Vala <mvala@redhat.com>

tolusha approved these changes Sep 16, 2021

View reviewed changes

openshift-ci bot assigned tolusha Sep 16, 2021

openshift-ci bot added the lgtm label Sep 16, 2021

sleshchenko self-requested a review September 16, 2021 14:31

sparkoo mentioned this pull request Sep 16, 2021

fix: don't rewrite 127 to 0.0.0.0 eclipse-che/che-plugin-registry#1051

Merged

9 tasks

metlos approved these changes Sep 16, 2021

View reviewed changes

openshift-ci bot assigned metlos Sep 16, 2021

sleshchenko approved these changes Sep 17, 2021

View reviewed changes

openshift-ci bot assigned sleshchenko Sep 17, 2021

sparkoo merged commit 2ad6c4f into eclipse-che:main Sep 17, 2021

sparkoo deleted the gh20190-secureWsServicesTraefik branch September 17, 2021 10:37

sparkoo mentioned this pull request Sep 17, 2021

[che-auth] secure workspace services eclipse-che/che#20190

Closed

che-bot added this to the 7.37 milestone Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: secure workspace services #1045

feat: secure workspace services #1045

sparkoo commented Sep 1, 2021 •

edited

Loading

openshift-ci bot commented Sep 1, 2021

sparkoo commented Sep 15, 2021

metlos left a comment

tolusha Sep 15, 2021 •

edited

Loading

metlos Sep 15, 2021

tolusha Sep 15, 2021

sparkoo Sep 16, 2021

skabashnyuk commented Sep 16, 2021

sparkoo commented Sep 16, 2021

sparkoo commented Sep 16, 2021

metlos commented Sep 16, 2021

sparkoo commented Sep 16, 2021

sleshchenko left a comment

sleshchenko Sep 17, 2021

sleshchenko Sep 17, 2021

openshift-ci bot commented Sep 17, 2021

feat: secure workspace services #1045

feat: secure workspace services #1045

Conversation

sparkoo commented Sep 1, 2021 • edited Loading

What does this PR do?

How it works?

Screenshot/screencast of this PR

What issues does this PR fix or reference?

How to test this PR?

PR Checklist

Reviewers

openshift-ci bot commented Sep 1, 2021

sparkoo commented Sep 15, 2021

metlos left a comment

Choose a reason for hiding this comment

tolusha Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

metlos Sep 15, 2021

Choose a reason for hiding this comment

tolusha Sep 15, 2021

Choose a reason for hiding this comment

sparkoo Sep 16, 2021

Choose a reason for hiding this comment

skabashnyuk commented Sep 16, 2021

sparkoo commented Sep 16, 2021

sparkoo commented Sep 16, 2021

metlos commented Sep 16, 2021

sparkoo commented Sep 16, 2021

sleshchenko left a comment

Choose a reason for hiding this comment

sleshchenko Sep 17, 2021

Choose a reason for hiding this comment

sleshchenko Sep 17, 2021

Choose a reason for hiding this comment

openshift-ci bot commented Sep 17, 2021

sparkoo commented Sep 1, 2021 •

edited

Loading

tolusha Sep 15, 2021 •

edited

Loading